[mweinbach]

Problem

Closing a terminal on Windows can stall for seconds.

• node-pty's windowsPtyAgent.js forks conpty_console_list_agent and only listens for message. If the helper exits or fails to spawn before replying, the kill waits for the full 5,000ms fallback timeout.
• TerminalManager.runKillEscalation sends SIGTERM and then always sleeps the full processKillGraceMs before SIGKILL, even when the process has already exited.

Change

• patches/node-pty@1.2.0-beta.15.patch: resolve the console-list promise on the agent's close and error events as well as message. close is used instead of exit because Node can emit exit before the helper's final IPC message is delivered, with a single finish guard that clears the timeout. The fork also passes windowsHide: true, and ignores malformed messages.
• pnpm-lock.yaml: updates only the node-pty@1.2.0-beta.15 patch hash.
• apps/server/src/terminal/Manager.ts: runKillEscalation subscribes to process.onExit before sending SIGTERM, then races exit against processKillGraceMs. SIGKILL is sent only if the process is still alive when the grace period ends. The exit subscription is released with Effect.ensuring, so it is also cleaned up when the kill fiber is interrupted.

Scope and approval

Split from #18508 under the one-problem-per-PR rule in CONTRIBUTING.md. This PR covers only the Windows terminal kill hang. The other changes from #18508 will come in separate PRs.

Verification

Manager.runKillEscalation (apps/server/src/terminal/Manager.test.ts, 92/92 pass):

• New test skips SIGKILL when the process exits during the grace period: with a 10s grace, the PTY exits right after SIGTERM, and the clock is then advanced past the grace. Signals sent: ["SIGTERM"]. The same test against the pre-fix Manager.ts fails with expected [ 'SIGTERM', 'SIGKILL' ] to deeply equal [ 'SIGTERM' ].
• Existing test escalates terminal shutdown to SIGKILL when process does not exit in time still passes: a terminal that ignores SIGTERM gets SIGKILL after the grace period.
• New test releases the exit listener when kill escalation is interrupted: closing the manager scope during the grace period leaves 0 exit listeners. The previous try/finally version left 1.

node-pty console-list helper in CI (apps/server/src/terminal/nodePtyConsoleList.test.ts, 4/4 pass). It stubs child_process.fork and runs the patched _getConsoleProcessList on every platform. Cases: reply then close returns the full list; exit before the IPC message still returns the full list; close without a reply returns [shellPid]; spawn error returns [shellPid]. Against unpatched node-pty, both fallback cases time out. Against an exit-based fallback, the exit-before-message case fails.

node-pty console-list helper (Windows 11 arm64, Node 24.21.0). WindowsPtyAgent.prototype._getConsoleProcessList was run from patched and unpatched copies of node-pty/lib:

Case	Unpatched	Patched
Helper exits before replying (conpty_console_list_agent stubbed to process.exit(1))	resolved [4242] in 5034–5133ms	resolved [4242] in 415–1331ms (Node startup time; close lands within ~5ms of exit)
Helper fails to spawn (ENOENT)	unhandled 'error' event crashes the process	resolved [4242] in 2–14ms
Helper replies then exits immediately	n/a	full 3-PID list received in 50/50 runs

Also: vp lint on the changed files (no new warnings) and tsc-rs -p apps/server/tsconfig.json --noEmit (0 errors).

Model: Claude 5.5 Opus

🤖 Generated with Claude Code

Latest cleanup-race verification at bf163d4924 (macOS ARM64 / Node 24.21.0): completed kill fibers are excluded during synchronized registration; the child finalizer still removes fibers completing afterward. A synchronous-exit regression across scheduler budgets fails on bad0c627af at budget 50 and passes after the fix. All 121 focused terminal tests and server typecheck pass. Targeted formatting/lint pass with one pre-existing warning. Exact-head CI 38307516086 passed. No fresh live Windows validation is claimed.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-13T00:23:29.127963Z	ba130c3	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

• Last reviewed commit: bf163d4
• Latest commit, not reviewed: aadd9da

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Path: .coderabbit.config.ts
• Review profile: CHILL
• Plan: Advanced
• Run ID: cda26643-e609-42c4-988c-6a9d0377d16a

📥 Commits

Reviewing files that changed from the base of the PR and between bad0c62 and bf163d4.

📒 Files selected for processing (2)

• apps/server/src/terminal/Manager.test.ts
• apps/server/src/terminal/Manager.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Terminal shutdown escalation now waits for process exit or the grace timeout after SIGTERM. The Windows console-list helper changes its launch settings and response handling.

Changes: Terminal Shutdown Escalation

Layer / File(s)	Summary
Exit-aware kill escalation apps/server/src/terminal/Manager.ts, apps/server/src/terminal/Manager.test.ts	runKillEscalation listens for process exit before sending SIGTERM. It sends SIGKILL only if the process remains active after the grace period. Kill-fiber registration skips completed fibers. Tests cover process exit, fiber retention, and listener cleanup on interruption.

Windows Console Process Listing

Layer / File(s)	Summary
Guarded console-list helper patches/node-pty@1.2.0-beta.15.patch, apps/server/src/terminal/nodePtyConsoleList.test.ts	The helper starts with WATCH_REPORT_DEPENDENCIES removed, empty execArgv, and windowsHide: true. It accepts array-valued consoleProcessList responses and resolves once. On helper error, close, or timeout, it returns the shell PID; the timeout also kills the helper. Tests cover replies, close, and spawn errors.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to bf163

The terminal shutdown and Windows helper changes appear ready to merge after normal checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to bf163

The reviewed shutdown changes preserve process identity and existing termination authority while avoiding unnecessary delays and repeated signals. No introduced or materially worsened security risk was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The manager change affects terminal termination across supported platforms; the helper change is Windows-specific. The existing sensitive sink is process termination: node-pty iterates console-list PIDs through process.kill and performs native PTY teardown. This PR changes completion timing, not the sink or its execution identity.

Trust Boundaries and Controls

• observed — The inspected helper IPC path obtains process IDs through native console enumeration using the supplied shell PID. The receiver accepts only array-shaped replies, and error or close falls back to that shell PID. Array-element validation is not added, but that acceptance boundary predates this PR; the comparison introduces no new PID source or caller on this path.

Resilience and Maintainability Implications

• observed — Failure containment remains best-effort: SIGTERM failure is logged and returns, while a successful signal without observed exit still escalates after grace. Scope interruption can stop escalation after SIGTERM, as before; the new exit subscription is released rather than left attached to a terminated or detached session.

Pre-merge checks: 4

Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title is concise, uses the conventional commit format, and clearly describes the primary Windows terminal kill timeout fix.
Description check		The description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the root cause, implementation, focused tests, platform-specific validation, limitatio…

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[juliusmarminge]

Note

This comment is posted by Julius' dot

The patch installation and formatting checks don't show that terminal shutdown stops waiting. Please add focused results for a helper exiting or failing before its reply, a terminal exiting during the grace period, and a terminal that still needs SIGKILL. The verification rule requires evidence of the changed behavior. I'll move this to draft; mark it ready when that evidence is attached.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b9490eddb3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[macroscopeapp]

Approvability

Verdict: Approved at b9490ed

Macroscope's review found this PR approvable — This is a narrowly scoped Windows terminal shutdown bug fix that removes unnecessary helper and process-exit timeout waits while preserving the existing escalation behavior for processes that remain alive. The lockfile change only selects the updated dependency patch, with no new capability, schema change, deployment change, or sensitive-area impact.

^{You can add or adjust custom eligibility rules. Learn more.}

[mweinbach]

Thanks. I added the evidence to the PR description's Verification section:

• Helper exits before replying: unpatched resolves after 5034ms, patched after 137ms.
• Helper fails to spawn: unpatched crashes on an unhandled 'error' event, patched resolves in 2ms.
• Terminal exits during the grace period: a new test, skips SIGKILL when the process exits during the grace period, checks that only SIGTERM is sent. It fails against the pre-fix Manager.ts.
• Terminal still needs SIGKILL: the existing escalation test still passes.

Marking ready for review.

[AdEx-Partners-DE]

A no-regression data point from a second Windows machine; we adopted this into a fork today.

I could not reproduce the stall here, so this only shows the healthy path is unchanged. Timed with a small script against the real node-pty in apps/server: spawn cmd.exe in a pty, wait 0.8 s, call kill(), measure until onExit. Five rounds each, Windows 11 Pro, Node 24.13:

	kill to exit
before (current patch)	79, 63, 64, 79, 79 ms
after (vp i with your patch; windowsPtyAgent.js contains windowsHide)	76, 80, 80, 77, 81 ms

apps/server/src/terminal/Manager.test.ts: 90 passed before, 91 after. tsc --noEmit in apps/server is clean.

Not checked: the failing-helper case your patch is for (the console-list agent answered every time here), and runKillEscalation with a real process inside the running app.

Posted by Claude (Opus 5.5, Claude Code via T3 Code) on behalf of @AdEx-Partners-DE.

[mweinbach]

Thanks for the second machine. One note: 60e115f changed the helper fallback from exit to close, because Node can emit exit before the final IPC message arrives. If you pulled the patch before that commit, re-run vp i to get it. Your healthy-path test should look the same, since close arrives within about 5ms of exit.

[chatgpt-codex-connector]

💡 Codex Review

t3code/apps/server/src/terminal/Manager.ts

Lines 1729 to 1730 in 666889a

	} finally {
	unsubscribeExit();

Use an Effect finalizer for the exit subscription

When a kill fiber is interrupted, such as by clearKillFiber or Scope.close(workerScope) while it is awaiting the grace period, Effect does not resume the generator, so this JavaScript finally is never evaluated. The cancellation effect returned by waitForProcessExit removes only its second listener; the listener created at the start of runKillEscalation remains attached until the PTY exits, which can be indefinite for the SIGTERM-ignoring process that needs escalation. Move this unsubscribe into Effect.ensuring or a scoped release so interruption also cleans it up.

---

t3code/patches/node-pty@1.2.0-beta.15.patch

Lines 44 to 48 in 666889a

	+ agent.on('error', function () {
	+ finish([_this._innerPid]);
	+ });
	+ agent.on('close', function () {
	+ finish([_this._innerPid]);

Add automated coverage for the helper failure fallbacks

The new error and close-without-message paths are the behavior that prevents the production five-second kill stall, but a repo-wide search for _getConsoleProcessList and conpty_console_list_agent finds only this patch; the added Manager test uses FakePtyProcess and cannot execute either helper branch. Add a focused test that stubs the forked helper for spawn failure and close-without-message, plus the send-then-close ordering, so this backend fix is exercised in CI rather than only by the manual Windows check.

AGENTS.md reference: AGENTS.md:L101-L101

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[mweinbach]

Both Codex P2 findings from the 666889a review are fixed in 0170683.

• Exit listener on interruption (real): a JS finally doesn't run when an Effect fiber is interrupted, so an interrupted kill escalation left its listener attached. runKillEscalation now uses a single onExit subscription that completes a Deferred, and releases it with Effect.ensuring. A new test closes the manager scope during the grace period and checks that 0 listeners remain. The old version left 1.
• Helper fallback coverage: nodePtyConsoleList.test.ts stubs child_process.fork and runs the patched _getConsoleProcessList in CI on all platforms. It covers reply then close, exit before the IPC message, close without a reply, and spawn error. The fallback cases time out against unpatched node-pty, and the exit-before-message case fails against an exit-based fallback.

[AdEx-Partners-DE]

Re-ran on the same Windows host with 01706834a (the earlier numbers were already on 60e115f):

• apps/server/src/terminal: 115 passed in 3 files before, 120 in 4 files after. nodePtyConsoleList.test.ts also passes when run alone (4 passed).
• The real pty kill is unchanged: 79, 79, 76, 79, 78 ms from kill() to onExit.
• tsc --noEmit in apps/server is clean.

Still not checked here: a real helper that fails to answer, outside the stubbed test.

Posted by Claude (Opus 5.5, Claude Code via T3 Code) on behalf of @AdEx-Partners-DE.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bad0c627af

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[mweinbach]

Rebased head ba130c3 is based on main b03534f. The Build job stopped before compilation while downloading Electron v44.4.5: curl exited 6 (DNS resolution failure). The 121 focused terminal tests and server typecheck pass locally on macOS ARM64. GitHub refuses my rerun request, and my upstream repository access is read-only. Could a maintainer rerun the failed Build job? https://github.com/pingdotgg/t3code/actions/runs/38326609854/job/115066632103

[AdEx-Partners-DE]

In case it helps while the Build job waits for a rerun: I applied ba130c38e on top of our fork's copy and ran the terminal tests on Windows 11 (Node 24.13). apps/server/src/terminal: 121 passed in 4 files (120 before this commit), and tsc --noEmit in apps/server is clean. That is the same count you report from macOS.

Not run here: the Electron build that failed in CI.

Posted by Claude (Opus 5.5, Claude Code via T3 Code) on behalf of @AdEx-Partners-DE.