fix(windows): serialize worktree terminal teardown - #8284
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe runtime coordinates worktree-scoped PTY admission and teardown across terminal spawning, splitting, and deletion. Teardown awaits verified PTY shutdown via 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
brennanb2025
left a comment
brennanb2025
left a comment
There was a problem hiding this comment.
Reviewed through a clean two-lane Codex gpt-5.6-sol high loop on exact pushed head 60e7cb3. Terminal lifecycle, SSH fail-closed behavior, bounded teardown performance, platform simulations, and live macOS isolation evidence are documented in the PR body.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/providers/ssh-pty-provider.ts (1)
252-258: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not mark graceful shutdowns as stopped.
When
pty.hasPtyis unavailable, this cache update makeshasPtyAsync()returnfalseimmediately after any successful shutdown request. The relay’s graceful path only sendsSIGTERMand retains the managed PTY until it exits or its fallback fires, so teardown can be falsely “verified” while the PTY remains alive. Only mark it stopped for synchronous immediate shutdown; otherwise wait forpty.exit.Proposed fix
await this.mux.request('pty.shutdown', { id: this.toRelayPtyId(id), immediate: opts.immediate ?? false, keepHistory: opts.keepHistory ?? false }) - this.ptyLiveness.markStopped(id) + if (opts.immediate) { + this.ptyLiveness.markStopped(id) + }
🧹 Nitpick comments (2)
src/main/daemon/shutdown-verification-owner-cache.ts (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a dedicated unit test for this class.
No test file for
ShutdownVerificationOwnerCachewas included in this batch. Given this cache underpins the shutdown-fencing correctness across bothDaemonPtyRouterandDegradedDaemonPtyProvider, a small dedicated test (verifyingremember/take/eviction at thelimitboundary, and that re-remembering an existing id doesn't grow size) would guard against regressions independent of the two consumer test suites.Also applies to: 23-28
src/main/daemon/daemon-pty-router.ts (1)
235-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate reconciliation loop — reuse the new shared helper.
This
reconcileOnStartupstill hand-rolls the same alive/killed aggregation loop thatdegraded-daemon-pty-provider.tsnow delegates toreconcileDegradedDaemonSessions(from the newdegraded-daemon-session-reconciliation.ts). Since that helper is generic overDaemonPtyAdapter[], it can be reused here too, removing the duplicated loop and keeping the two shutdown-ownership invalidation paths from drifting apart in the future.♻️ Proposed refactor
+import { reconcileDegradedDaemonSessions } from './degraded-daemon-session-reconciliation' + async reconcileOnStartup(validWorktreeIds: Set<string>): Promise<{ alive: string[] killed: string[] }> { - const alive: string[] = [] - const killed: string[] = [] - for (const adapter of this.allAdapters()) { - const result = await adapter.reconcileOnStartup(validWorktreeIds) - for (const id of result.alive) { - alive.push(id) - } - for (const id of result.killed) { - killed.push(id) - } - for (const id of result.alive) { - this.sessionAdapters.set(id, adapter) - this.shutdownVerificationAdapters.delete(id) - } - for (const id of result.killed) { - this.sessionAdapters.delete(id) - this.shutdownVerificationAdapters.delete(id) - } - } - return { alive, killed } + return reconcileDegradedDaemonSessions( + this.allAdapters(), + validWorktreeIds, + (id, adapter) => { + this.sessionAdapters.set(id, adapter) + this.shutdownVerificationAdapters.delete(id) + }, + (id) => { + this.sessionAdapters.delete(id) + this.shutdownVerificationAdapters.delete(id) + } + ) }Note: the helper's name (
reconcileDegradedDaemonSessions) is specific to the degraded-provider context; consider renaming it to something adapter-agnostic (e.g.reconcileDaemonAdapterSessions) if reused here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 87bea1f9-9455-4cf6-a868-7f74050edc97
📒 Files selected for processing (25)
src/main/daemon/daemon-pty-router.test.tssrc/main/daemon/daemon-pty-router.tssrc/main/daemon/degraded-daemon-pty-provider.test.tssrc/main/daemon/degraded-daemon-pty-provider.tssrc/main/daemon/degraded-daemon-session-reconciliation.tssrc/main/daemon/provider-replay-subscription.tssrc/main/daemon/shutdown-verification-owner-cache.tssrc/main/ipc/pty-startup-barrier-ordering.test.tssrc/main/ipc/pty.test.tssrc/main/ipc/pty.tssrc/main/ipc/worktrees.test.tssrc/main/ipc/worktrees.tssrc/main/providers/ssh-pty-liveness.tssrc/main/providers/ssh-pty-provider.test.tssrc/main/providers/ssh-pty-provider.tssrc/main/providers/types.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/terminal-stop.test.tssrc/main/runtime/worktree-pty-admission.test.tssrc/main/runtime/worktree-pty-admission.tssrc/main/runtime/worktree-teardown.test.tssrc/main/runtime/worktree-teardown.tssrc/relay/pty-handler.test.tssrc/relay/pty-handler.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1075a4af-26ae-4d76-900f-2ce4755985a3
📒 Files selected for processing (10)
src/main/ipc/worktrees.test.tssrc/main/ipc/worktrees.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/worktree-teardown.test.tssrc/main/runtime/worktree-teardown.tssrc/relay/pty-handler.test.tssrc/relay/pty-handler.tssrc/relay/pty-session-kill.test.tssrc/relay/pty-session-kill.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/relay/pty-session-kill.test.ts
- src/relay/pty-handler.ts
- src/main/runtime/worktree-teardown.test.ts
- src/main/ipc/worktrees.ts
- src/main/runtime/orca-runtime.test.ts
- src/main/runtime/orca-runtime.ts
|
Taking a look! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/relay/pty-session-kill.ts (1)
222-244: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPoll unresolved PIDs until the deadline
verifyProcessesStoppedreturnsfalseon the first livepssnapshot, sokillPosixPtySessioncan fail on normal SIGKILL reaping races and surface as a teardown error. Retry the remaining PIDs untilPTY_SESSION_VERIFY_TIMEOUT_MSexpires.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ab147757-f7c9-489a-be88-9764511a4143
📒 Files selected for processing (9)
src/main/daemon/daemon-foreground-confirmation-protocol.test.tssrc/main/daemon/types.tssrc/main/ipc/pty.tssrc/main/runtime/pty-stop-concurrency.test.tssrc/main/runtime/pty-stop-concurrency.tssrc/main/runtime/worktree-teardown.test.tssrc/main/runtime/worktree-teardown.tssrc/relay/pty-session-kill.test.tssrc/relay/pty-session-kill.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/runtime/pty-stop-concurrency.ts
- src/main/runtime/worktree-teardown.ts
- src/main/runtime/worktree-teardown.test.ts
- src/main/ipc/pty.ts
|
Final exact-head CI is green for authoritative head
The verify job includes lint, typecheck, full tests, unpacked app build, and packaged CLI smoke. Local/fork/upstream PR heads are identical; the PR is MERGEABLE and APPROVED. The temporary fork validation PR is closed and its base branch removed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
# Conflicts: # src/main/runtime/orca-runtime.ts
958a3a2 to
0bf7c87
Compare
|
Head change notice (maintainer): this branch was force-updated from After
Verification on |
The fail-closed teardown path threw internal pty ids ("Failed to stop
local worktree terminals: <id>@@daemon") straight into the delete toast
and CLI. Replace both throws with an actionable sentence (retry, or
close the terminal) and log the pty ids for triage instead. Also strip
Electron's "Error invoking remote method '…': Error:" IPC wrapper in the
renderer delete path so the toast shows the message, not transport
plumbing.
|
Deep review follow-up pushed at Found and fixed one Windows SSH relay fail-closed gap: after a graceful ConPTY close had not produced its native exit event, the 5s fallback retained the internal map entry but marked it disposed and emitted synthetic Red/green regression now proves that before native exit: the owner remains counted, Exact-head verification:
Physical Windows, native WSL, and live SSH remain the same explicit environment gaps documented in the PR. |
…adopt main's stablyai#8706 for POSIX descendants Reconciles the fail-closed worktree-teardown fix with main's newly-merged teardown work so the two compose instead of collide. Kept (the actual stablyai#8275 fix — Windows batch `worktree rm` killing the shared daemon and unrelated ConPTY panes): - fail-closed verified worktree teardown + legacy-daemon verification gate - worktree PTY admission fence (no spawn races an in-progress deletion) - Windows ConPTY verified teardown / daemon preservation (retain owner until native exit; never double-close ConPTY) - support glue (owner cache, bounded stop concurrency, teardown timeouts) Dropped the PR's parallel POSIX relay kill-path rewrite (pty-session-kill / -membership / -exit-verification): main's stablyai#8706 (pty-descendant-termination) already reaps detached agent descendants on POSIX, so a second freeze-sweep engine that ran on every immediate shutdown was redundant, riskier scope. Integrated instead: - POSIX agent-descendant reaping comes from stablyai#8706, including inside our verified daemon force path (Session.forceKillAndDisposeSubprocessAndWait snapshots + terminates descendants before the verified root kill) - daemon POSIX force-kill is a leader SIGKILL + await-native-exit shim (zombie-safe via existing exit waiters); relay POSIX teardown is a plain SIGKILL (SSH fail-closed enforced at the runtime liveness layer) Follow-up: descendant-level POSIX verification for non-agent / pre-reparented survivors on the delete path (leader-level fail-closed holds today).
|
Re-scoped to the #8275 core and rebased onto current While this PR was open, Why this was still needed: I verified that #8275's actual bug — batch Kept (the #8275 fix):
Dropped: the parallel relay POSIX kill-path rewrite ( Integrated instead: POSIX descendant reaping now comes from #8706, including inside this PR's verified daemon force path ( Net: the PR shrank from ~104 files / +14k to 50 files / +5.2k, drops the highest-risk code, and no longer duplicates #8706. Typecheck, lint, and the full teardown matrix (4,666 tests, including #8706's descendant suite) pass. Follow-up: descendant-level POSIX verification for non-agent / pre-reparented survivors on the delete path (leader-level fail-closed holds today). A live Windows batch- |
|
Thanks for this, @bbingz — and I'm sorry it sat in review for as long as it did. 🙏 The important part: the bug you reported in #8275 is fixed, and it all ships in the next release. I ended up fixing the root cause across a couple of separate PRs, so I'm closing this one as superseded rather than landing a second implementation on top. What went in:
So your exact scenario — batch Really appreciate the detailed repro and the depth of work here. It pinned down a genuinely nasty teardown bug, and it directly shaped how thoroughly I ended up hardening that path. Closing as superseded by #8661. |
3 participants
Summary
terminal.stop, while worktree deletion uses awaited, verified teardown before Git worktree removal, recursive worktree deletion, or registry/metadata removalRoot cause and invariant
The original removal path could begin graceful asynchronous PTY shutdown and then immediately enter provider and registry fallback sweeps. On Windows, overlapping ConPTY teardown against a shared daemon could terminate unrelated sessions.
Once removal owns a worktree, no new PTY for that worktree may enter or finish registration. Already-admitted spawns drain, every authoritative owned PTY is stopped and verified, and only then may destructive worktree mutation begin. Generic
terminal.stopremains graceful and still permits shell EXIT traps. Local and daemon fallbacks are deduplicated and verified against an authoritative post-stop inventory; any unverified local, daemon, or SSH stop blocks deletion.Configured symlink cleanup and the existing pre-removal archive hook can still run before PTY teardown. The safety claim begins before Git worktree removal, recursive worktree deletion, and registry/metadata removal; this PR does not claim that teardown precedes every filesystem operation.
Performance and resource bounds
pty.hasPtychecks and zerolistProcessesinventorieslistProcessesinventory across multiple PTYspkill -s <sid>, while Darwin uses a targeted TTY inventory and descendant-first signals after root ownership verificationScreenshots
No visual change.
Testing
Final pushed HEAD:
8a745733d(author implementation plus deep-review hardening and the durable reliability gate)pnpm linton the final local HEADpnpm typecheckon the final local HEADpnpm test— 29,951 passed and 42 skipped on the final source using the Node 26 localStorage compatibility flag and a CI-equivalent clean Git-config environmentpnpm build—pnpm build:desktoppassed on the final local HEAD; exact-headbuild:unpackis delegated to PR CI because it is the repository's packaging gate.Additional focused verification on the final local HEAD:
reviewed and retained maintainer hardening commit
960ba8c1: protocol v22, PID/TTY ownership revalidation, child-parent revalidation, all-session stop settlement, and authoritative fallback state retirement17 affected test files: 1,622/1,622 passed
relay Windows/POSIX teardown files: 91/91 passed
git diff --checkpassedPrior live evidence retained by the implementation:
AI Review Report
The review rechecked the current PR diff, removal ordering, admission fencing, daemon/local/SSH ownership, bounded liveness verification, Windows ConPTY semantics, POSIX session teardown, WSL-shaped providers, and duplicate remove/forget ownership. CodeRabbit correctly identified that the POSIX fail-closed test inherited the host platform; its earlier “addressed” marker was a false positive after a baseline merge. The test now explicitly selects Linux and restores
process.platform. Merging the latest main exposed a second Windows test that did not emit ConPTYonExit; that regression was reproduced as a 30-second timeout and corrected to await the native exit event.Cross-platform review covered macOS, Linux, Windows, WSL, and SSH. This PR does not change shortcuts, shortcut labels, or user-visible paths.
Security Audit
The review checked command construction, PID/TTY validation, process-selection scope, path ownership, IPC trust boundaries, cache bounds, and fail-closed behavior. POSIX process commands use bounded argument arrays rather than shell interpolation; Darwin validates the PTY name and verifies that the captured TTY still owns the root PID before signalling descendants. Missing tools, malformed output, timeouts, ambiguous ownership, and disconnected relays block destructive removal. No auth, secrets, dependencies, or external network behavior are added.
Notes
Closes #8275
Maintainer update (2026-07-14)
The branch was reset to the author-verified head
73dd9161, merged with latestmain, and extended with a small audited fix set. An interim batch of maintainer review-hardening commits previously on this branch was withdrawn after an audit found it introduced regressions; those items move to a follow-up PR. Verification on the new head:pnpm typecheckandpnpm lintpass; 4,591 targeted tests acrosssrc/relay,src/main/daemon,src/main/providers,src/main/ipc, andsrc/main/runtimepass.Maintainer deep-review update (2026-07-14)
A final correctness, cross-platform, SSH, cleanup, security, and performance audit found and fixed four additional failure-path issues before merge:
pschecks under the existing 500 ms deadline.Reliability contract
terminal-session.worktree-removal-isolation— once removal owns a host/worktree, no PTY may enter or finish registration; every admitted target PTY must be fully verified stopped before destructive Git/filesystem/metadata mutation; ambiguity preserves the worktree and all unrelated sessions.terminal-session.worktree-removal-isolationinconfig/reliability-gates.jsonc; its exact 15-file command passed 966/966 tests in 11.56s.Final verification
pnpm lintpassed (pre-existing warnings only)pnpm typecheckpassed for node, CLI, and webgit diff --check, max-lines ratchet, and reliability-gate validation passed