fix(terminal): hold an observed PTY exit until its owner receives it, and settle workers only on that proof by Jinwoo-H · Pull Request #19665 · stablyai/orca

Jinwoo-H · 2026-09-09T02:13:53Z

	Files	Added	Deleted	Net
Test	18	$\color{#1a7f37}{\Huge{\mathbf{+}}}$1971	$\color{#cf222e}{\Huge{\mathbf{−}}}$102	$\color{#1a7f37}{\Huge{\mathbf{+}}}$1869
Prod	32	$\color{#1a7f37}{\Huge{\mathbf{+}}}$485	$\color{#cf222e}{\Huge{\mathbf{−}}}$258	$\color{#1a7f37}{\Huge{\mathbf{+}}}$227

What the user notices

Before. An orchestration worker whose shell ends while Orca is closed or disconnected is settled by a guess. Over SSH the guess is "the terminal is not in my listing," which is a client-side copy of the relay's state and is wrong after every reconnect, reattach refusal, and app update (#13852), so live workers were declared dead and their panes torn down. The relay did keep a 5-second memory of a death, but the same map was also written by a bookkeeping sweep and by relay shutdown with the same shape, so a reader could not tell a real death from housekeeping, and the memory expired long before a disconnected client came back. Locally the daemon had the same 2-second memory.

After. Each execution host keeps the exit it observed until the process's owner has received it, and answers "exited" only when asked about that exact process incarnation. Main's recovery settles a worker only on that answer. Everything else is unverifiable and takes the worker-show / worker-abandon path the guide already documents. A worker that finishes while Orca is closed, locally or over SSH, is settled on reopen from the host's own observation.

The change

Relay. The tombstone map, its 5-second TTL, its shared prune module, and the admission-time delete are gone. The relay already held an observed exit until publication to the owner settles (pendingExitByPty); pty.inspectProcess and missing-id pty.attach now read that, matched on the caller's expectedIncarnationId. The reaper is split so the record-torn-down sweep cannot write a death or mark a physical exit. clearPtyFlowState no longer deletes the pending exit, which was erasing an ESRCH death before anyone could be told. ESRCH deaths are spelled pty_exit_status_unknown instead of a fabricated code.

Daemon. Same rule, with no held-exit concept at all: an exited session is simply a state of its record. reapSession frees the emulator and nothing else; the record leaves sessions only when its owner recreates the id or the host is disposed. Every reader already gates on isAlive, so an exited record is invisible to listing, attach, and the idle timer. inspectProcess answers from the session's own incarnationId and exitCode for an exact incarnation match. The 2-second tombstone and its module are deleted. Two forwarding layers that dropped expectedIncarnationId (DaemonPtyRouter.adapterForInspection, DegradedDaemonPtyProvider.inspectProcess) now pass it through; an unclaimed id is routed to the current daemon only when an incarnation is named, and a daemon that never held it answers not-found.

Main. runtime-legacy-worker-terminal-recovery-candidate.ts settles exited only when the provider's inspectProcess answers for the stored incarnation, via a new proveTerminalExited port that deliberately bypasses the hasPty gate (the whole point is an id this process no longer has cached). Listing absence, client-inventory incarnation mismatch, thrown transport, terminal_gone, and remote: ids all defer. On an identity mismatch the adopted pane surface is still rolled back (that is this client's tab bookkeeping, not a verdict) while the dispatch defers.

Policy change to state plainly. Before, a different process holding the same terminal id was read as proof the old one died. Relay restarts, revive races, and stale client inventory all produce that mismatch while the remembered process can still be alive, so it now defers. A worker that genuinely died behind an orphaned relay (#13852) stays unverifiable until worker-abandon instead of settling from absence; that absence-settlement was the liveness violation.

What this does not add

No new map, ledger, cache, timer, count cap, cleanup pass, probe RPC, owner tracking, mint-epoch comparison, or version field. Relay and client are always the same build (hashed install dir plus handshake refusal), so no old-relay branch. Bound on held exits: a disconnected client cannot create sessions, so the set is at most the terminals it had open; a relay or daemon with no live sessions retires on its own idle policy and the held exits go with it, which is the same bound the hosts already apply to the terminals themselves.

Verification

tc:node, tc:cli clean; changed-code quality and react-doctor gates 0 findings; max-lines ratchet OK.
src/main/daemon src/main/providers src/main/ipc src/main/runtime src/relay tests/e2e: 1579 files, 15858 tests pass.
Two independent review passes on the relay/main commits and one on the daemon commit, each with reachability sentences per finding. The daemon pass found the daemon deleting the held exit on read where the relay only deletes on delivery; fixed in the last commit.
Mutations killed by checked-in tests: torn-down sweep writes a death; reader ignores incarnation; delivered exit retained (TTL regression); clearPtyFlowState erases the exit; ESRCH spelled as a code; candidate settles from absence, from mismatch, or from a thrown transport; remote: fence removed; attach drops the incarnation; daemon reaps on exit; daemon reader ignores incarnation; router routes unclaimed ids without an incarnation; degraded provider drops options; rollback dropped on mismatch.

Supersedes the relay half of #19347 (closed). Plan: relay-exit-evidence-PLAN-v4 in the review archive.

…dence The relay kept two records of "this process exited": `pendingExitByPty`, held until the owning client received it, and `retiredIncarnations`, a 5-second tombstone written by three writers with different meanings. The readers that answered `exited` looked only at the tombstone, so an SSH worker that died while the laptop was disconnected was only settled if the client reconnected inside 5 seconds; otherwise the coordinator saw it running forever. One of those writers was a bookkeeping sweep, which could answer `exited` for a process nobody watched die. Delete the second copy. `pty.inspectProcess` and `pty.attach` on a missing id now read `pendingExitByPty`, matching the incarnation the caller names, so an observed death waits as long as the disconnect lasts and disappears the moment the owner receives it. The ESRCH probe keeps the death write; the torn-down record sweep is split out and can no longer reach it. Main settles a legacy worker only on the relay's `inspectProcess` answer for the stored incarnation. Policy change: settlement from listing absence and from client-inventory identity mismatch is deleted. Absence is what every disconnected client sees, and a recycled id is not a death — both now defer to the next sweep and take the documented `worker-abandon` path. Daemon behaviour is unchanged; its copy of the prune helper moves out of `src/shared` next to its only remaining caller. Refs docs/reference/ssh-execution-boundary.md

…tch defers An identity mismatch under a candidate's PTY id means the id now routes to a different shell. Deferring the dispatch is right — nothing observed this worker's shell end — but leaving the pane bound to the replacement is not: that binding is this client's own tab bookkeeping and carries no verdict. Roll the surface back on both mismatch branches, then defer, so a later sweep re-reads the identity instead of re-deferring a pane pointed at the wrong process.

The daemon's only death evidence was `retiredIncarnations`, a 2-second tombstone written in `onSessionExit` from a session the host reaps in the same callback. Two seconds is a window no reopening app can hit, so once main stopped settling a worker from listing absence, a local worker whose shell ended while Orca was closed was never settled: it stayed dispatched on every sweep until `worker-abandon`. Give the daemon the relay's rule instead of a second record of the same fact. `TerminalHost.reapSession` already disposes the exited session's emulator and drops it from `sessions`; it now drops it only when `broadcastExit` reached an attached client. With none attached nobody received the exit, so the emulator-free record stays, and `inspectProcess` hands it over as `verdict: 'exited'` for an exact `expectedIncarnationId` match and deletes it in the same breath. A caller with the wrong incarnation, or none, still gets `SessionNotFoundError`, and a session recreated under the same id replaces the record rather than inheriting it. Every other reader of `sessions` already gates on `isAlive`, so listing, kill, snapshots, sizes and liveness probes are unchanged. The daemon still retires exactly when it did before: `onSessionReaped` fires on exit whether or not the record was dropped. Both daemon-fronting providers were answering `terminal_gone` from their own routing tables for exactly the ids this question is about, and the degraded one dropped `expectedIncarnationId` and `steadyState` entirely. An unclaimed id now routes to the current daemon when — and only when — the caller names a remembered incarnation; a daemon that never held it answers not-found, so routing cannot manufacture an exit, and an ordinary poll still may not borrow a route it never owned. Deletes `retired-pty-incarnations.ts`, its test and the TTL. Refs docs/reference/ssh-execution-boundary.md

The relay keeps an undelivered exit until publication settles; the daemon was deleting it on the first matching inspect. A recovery sweep can read the proof and then fail to persist the settlement, which would leave the next sweep with nothing. Keep the record until its owner recreates the id or the host is disposed, matching the relay. Also free a disposed plane's unreachable pending-output buffer.

coderabbitai · 2026-09-09T02:23:48Z

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.

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

📝 Walkthrough

Walkthrough

The change adds incarnation-specific PTY inspection across daemon routing, runtime controllers, providers, SSH reattach, relay handling, and legacy worker recovery. Undelivered exits remain available for matching incarnation checks. Recovery now defers when the host cannot prove that the stored incarnation exited. Tests cover routing, retained exit evidence, recycled PTY ids, relay behavior, and recovery convergence.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to 1cadd

Incarnation fencing prevents stale terminal actions from affecting replacements, but a stale kill can still cancel a replacement process while it is starting. This can disrupt terminal startup and should be fixed before merge.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description gives a detailed explanation of the behavior, implementation, policy change, and verification. However, it omits several required template sections, including an explicit ELI5 section,…	Add all required template sections. Include an issue reference using `Fixes #...`; state `N/A` with a reason for Visual Proof if applicable; document manual and automated testing with platform coverage; complete or mark the AI Disclosure, R…
Docstring Coverage	⚠️ Warning	Docstring coverage is 24.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 36 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: retaining observed PTY exits and settling workers only after exact-incarnation proof.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description gives a detailed explanation of the behavior, implementation, policy change, and verification. However, it omits several required template sections, including an explicit ELI5 section, a Linked Issue with a Fixes #... reference, Visual Proof or N/A, the Testing checklist, AI Disclosure, Review, Agent skill upstream boundary, Notes, and the final Checklist.

Resolution

Add all required template sections. Include an issue reference using Fixes #...; state N/A with a reason for Visual Proof if applicable; document manual and automated testing with platform coverage; complete or mark the AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist items.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

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.

❤️ Share

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

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: f2b1b7d4-f162-45dd-a8e1-e5a73f85fe43

📥 Commits

Reviewing files that changed from the base of the PR and between e80fae0 and 08f1387.

📒 Files selected for processing (27)

src/main/daemon/daemon-inspection-incarnation-routing.test.ts
src/main/daemon/daemon-pty-router.ts
src/main/daemon/degraded-daemon-pty-provider.ts
src/main/daemon/degraded-daemon-session-routing.ts
src/main/daemon/session-output-plane.ts
src/main/daemon/terminal-host-process-inspection.test.ts
src/main/daemon/terminal-host-process-inspection.ts
src/main/daemon/terminal-host.ts
src/main/ipc/pty/runtime/controller.ts
src/main/ipc/pty/runtime/operations.ts
src/main/ipc/pty/runtime/remembered-incarnation-exit.test.ts
src/main/providers/pty-process-inspection.ts
src/main/providers/ssh-pty-reattach-absence-discrimination.test.ts
src/main/providers/ssh-pty-session-reattach.ts
src/main/runtime/orca-runtime-preserved-branch-cleanup.ts
src/main/runtime/orca-runtime-test-scenario-builders.spec.ts
src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-03.spec.ts
src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-04.spec.ts
src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-05.spec.ts
src/main/runtime/runtime-legacy-worker-terminal-recovery-candidate.ts
src/main/runtime/runtime-legacy-worker-terminal-recovery-types.ts
src/main/runtime/runtime-pty-controller-contract.ts
src/relay/pty-handler-observed-exit-evidence.test.ts
src/relay/pty-handler-ssh-recovery-convergence.test.ts
src/relay/pty-handler.ts
src/shared/retired-pty-incarnations.test.ts
src/shared/retired-pty-incarnations.ts

💤 Files with no reviewable changes (2)

src/shared/retired-pty-incarnations.test.ts
src/shared/retired-pty-incarnations.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

pullfrog

✅ No new issues found.

Reviewed changes

Relay exit evidence — replaces the 5-second retiredIncarnations tombstone with pendingExitByPty, held until the owning client accepts delivery; pty.inspectProcess and a missing-id pty.attach answer exited from it, matched on expectedIncarnationId; reapExitedPty is split into a bookkeeping-only dropTornDownPtyRecord and an ESRCH-backed reapPtyProvenExited; ESRCH reads as pty_exit_status_unknown; clearPtyFlowState no longer erases a pending exit.
Daemon hold-until-received — a session that exits with no attached client stays in sessions (emulator freed) until its id is recreated or the host disposes; inspectProcess answers from undeliveredExitFor; the 2-second tombstone and pruneRetiredPtyIncarnations are deleted.
Main recovery deferral — runtime-legacy-worker-terminal-recovery-candidate.ts settles exited only via the new proveTerminalExited port (→ inspectExitedIncarnation → providerObservedIncarnationExit) which bypasses the hasPty cache; listing absence, incarnation mismatch, thrown transport, terminal_gone, and remote: ids all defer; identity-mismatch paths roll back the pane surface but defer the dispatch.
Incarnation pass-through — DaemonPtyRouter and DegradedDaemonPtyProvider carry expectedIncarnationId to a daemon that no longer claims the id, and SSH reattach forwards it so the relay can mark a proven-exited refusal.
Tests — ~1133 net-new lines, including an end-to-end convergence test (relay + SSH provider + recovery candidate) and a shipped-proof-chain local test through the daemon router; assertions pin exact verdicts (exited/unverifiable) and deferral outcomes rather than loose truthiness.

The change consistently errs toward unverifiable/deferral rather than certifying a death from absence or a recycled id, which is the correct direction for the SSH execution boundary. The held-exit retention is unbounded in time but bounded in count, and its memory footprint is addressed (markDisposed clears pending output; the relay keeps only {id, code, incarnationId}). No liveness or correctness defects found.

^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

`reapSession` was deciding the fate of a session record from `session.hasAttachedClients` -- "was someone subscribed at the instant the exit fanned out?" -- and deleting the record when the answer was yes. That is a second lifecycle layered on the session's own `running`/`exited` state, keyed on a signal that means "subscribed", not "received", and it is the seam that produced the delete-on-read bug the previous commit had to fix. Drop it. `reapSession` disposes the session (freeing the emulator and the subprocess handle) and fires `onSessionReaped`; the record stays in `sessions` as an exited record. It leaves when its owner recreates the id -- create already disposes and deletes the dead `existing` -- or when the host is disposed. Every other reader of `sessions` already gates on `isAlive`, so listing, kill, snapshots, sizes, foreground confirms, checkpoints and the idle timer are unchanged; `onSessionReaped` still fires on exit exactly as before. `inspectProcess` reads the exit off the session's own fields instead of through a lookup with its own delivery doctrine: a non-alive record answers `exited` to a caller naming that exact incarnation, and `SessionNotFoundError` to anyone else. `undeliveredExitFor` is gone; the inspection option is now `exitedSession`, which is what it always was.

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 56132566-e678-4431-8835-da746bfb95c0

📥 Commits

Reviewing files that changed from the base of the PR and between 08f1387 and 733e094.

📒 Files selected for processing (5)

src/main/daemon/session.ts
src/main/daemon/terminal-host-process-inspection.test.ts
src/main/daemon/terminal-host-process-inspection.ts
src/main/daemon/terminal-host-session-reaping-leak.test.ts
src/main/daemon/terminal-host.ts

🚧 Files skipped from review as they are similar to previous changes (1)

src/main/daemon/terminal-host-process-inspection.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

pullfrog

✅ No new issues found.

Reviewed changes

The delta since the prior Pullfrog review is commit 733e094 ("an exited session is a state, not a second lifecycle"), a daemon-only refactor.

Collapsed the exited-session lifecycle — reapSession no longer keys record deletion on session.hasAttachedClients ("was someone subscribed at the instant the exit fanned out?"). It now always disposes the session (freeing the emulator and the subprocess handle) and leaves the record in sessions as an exited record, which leaves only when its owner recreates the id or the host is disposed.
Inlined undeliveredExitFor into inspectProcess — the exit is read straight off the session's own fields (exitCode, incarnationId) as the exitedSession inspection option, so a delivered exit stays answerable to a caller naming that exact incarnation, and SessionNotFoundError to anyone else.
Tests — reworked the attached-exit case to assert the record remains answerable after a delivered broadcast (with an exact verdict: 'exited' / reason: 'pty_exit_0'), and updated the leak-test rationale to reflect that the emulator is the retained bytes while the emulator-free record is a handful of fields.

The refactor removes the second, hasAttachedClients-keyed lifecycle that was the seam behind the earlier delete-on-read bug, and errs further toward deferral rather than certifying a death. I verified the record stays answerable after dispose() (which preserves exitCode/incarnationId), the recreate path deletes dead records, host dispose clears the map, and every other reader of sessions already gates on isAlive. The daemon tests pass (terminal-host-process-inspection, terminal-host-session-reaping-leak).

^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

An exited record is kept as evidence for an owner that was away. A kill is the owner acting, so there is nothing left to tell it; keeping that record would grow the map by one entry per closed terminal for the daemon's life, since main mints a fresh id per terminal and the daemon outlives app restarts. The kill tombstone the host already records is the fact; read it at reap.

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: aa808f8d-48f8-4ec2-a0fd-0e339acb42d5

📥 Commits

Reviewing files that changed from the base of the PR and between 733e094 and 8c79b50.

📒 Files selected for processing (2)

src/main/daemon/terminal-host-process-inspection.test.ts
src/main/daemon/terminal-host.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

pullfrog

✅ No new issues found.

Reviewed changes

The delta since the prior Pullfrog review is commit 8c79b508d3 ("drop the record of a session its owner asked to end"), a focused daemon retention fix.

Reap deletes only owner-requested exits — reapSession now removes the exited record from sessions only when the session's kill tombstone is present, so a terminal the owner closed via kill leaves nothing behind while a natural exit stays as answerable evidence for a caller naming that exact incarnation.
Tombstone recorded before the signal — kill() records the tombstone ahead of killSession (so a subprocess that exits synchronously inside the signal is still recognized at reap) and clears it in a catch when the graceful signal is refused, preserving the isKilled admission invariant.
Tests — added a case pinning that a killed, detached session's record is gone (inspectProcess throws SessionNotFoundError) after exit, alongside the existing natural-exit-retention and refused-signal cases.

The change errs toward retention rather than premature deletion, reads the kill tombstone the host already maintains as the "owner asked" fact, and strictly reduces the per-terminal record growth the commit message identifies. Verified the daemon tests pass (terminal-host-process-inspection, terminal-host-session-reaping-leak, terminal-host).

^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

…s durable The host keeps an observed exit until its owner has acted on it. Main now says so: after the surface persists and the orchestration write lands, it sends the same shutdown a closed pane sends, and the daemon drops the record. A settlement that failed at either write sends nothing, so the next sweep reads the same evidence. Reading is never delivery; the owner's own act is. The two daemon recovery cases move to part-08 to stay under the test line cap.

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: d036e1d1-ab23-4040-aa44-fdfc2c1a7d61

📥 Commits

Reviewing files that changed from the base of the PR and between 8c79b50 and 3269ab9.

📒 Files selected for processing (11)

src/main/daemon/terminal-host-process-inspection.test.ts
src/main/daemon/terminal-host.ts
src/main/ipc/pty/runtime/controller.ts
src/main/ipc/pty/runtime/operations.ts
src/main/runtime/orca-runtime-preserved-branch-cleanup.ts
src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-04.spec.ts
src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-08.spec.ts
src/main/runtime/orca-runtime.test.ts
src/main/runtime/runtime-legacy-worker-terminal-recovery-runner.ts
src/main/runtime/runtime-legacy-worker-terminal-recovery-types.ts
src/main/runtime/runtime-pty-controller-contract.ts

💤 Files with no reviewable changes (1)

src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-04.spec.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

pullfrog

✅ No new issues found.

Reviewed changes

The delta since the prior Pullfrog review is commit 3269ab9 ("release a proven exit only after its settlement is durable"), which adds the consume half of the held-exit contract.

Added the release half of the held-exit protocol — a new releaseProvenExit recovery port (releaseExitedIncarnationFromRuntimeController → getProviderForPty(id).shutdown(id, { immediate: true })) fires in the runner only after persist and reconcileMissing both succeed, so a settlement that failed to persist leaves the host's evidence intact for the next sweep.
Daemon kill() now consumes a naturally-exited record — killing a session that already ended on its own deletes its record and resolves (no tombstone recorded, isKilled stays false), the daemon-side counterpart to the relay's delete-on-delivery.
Relay release is a deliberate no-op — pty.shutdown answers a missing id silently, since the relay already dropped its pending exit on delivery.
Tests — moved the daemon recovery cases into a new part-08 file and pinned the release ordering with exact assertions: a reconcileMissingWorkerTerminal throw and a flushPendingOrThrowAsync rejection both retain the exit (verdict: 'exited') and defer the dispatch, while a durable settlement drops it (inspectProcess throws).

Verified the focused suites pass (terminal-host-process-inspection, remembered-incarnation-exit, terminal-host-session-reaping-leak, pty-handler-observed-exit-evidence, pty-handler-ssh-recovery-convergence, and the full orca-runtime.test.ts harness, 1262 passing). The release gating matches the durable-settlement requirement, and the daemon consume-half errs toward retention rather than premature deletion.

^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

…he session The release main sends after settling a proven exit named only the pane id. A pane that respawned onto its stable id between the proof read and the release would have had its new shell killed. The release now names the incarnation it settled; the daemon kill and the relay shutdown both refuse when the id carries a newer one. The owner-kill tombstone map (write-only on main, capped at 1000 with eviction) is gone. Whether the owner asked a session to end is a fact about that session, so it is a flag the termination controller sets and the reaper reads. Claude-Session: 1fec75fd-224b-46ab-95fe-d88e0f3d9ff9

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 4f27b9e1-167e-4ca9-a67f-67d52f3ec6f4

📥 Commits

Reviewing files that changed from the base of the PR and between 3269ab9 and 1cadd1d.

📒 Files selected for processing (20)

src/main/daemon/daemon-pty-adapter-kill-incarnation-fence.test.ts
src/main/daemon/daemon-pty-router.ts
src/main/daemon/daemon-pty-session-control.ts
src/main/daemon/daemon-request-router.ts
src/main/daemon/degraded-daemon-pty-provider.ts
src/main/daemon/session-termination-controller.ts
src/main/daemon/session.ts
src/main/daemon/terminal-host-options.ts
src/main/daemon/terminal-host-process-inspection.test.ts
src/main/daemon/terminal-host-session-create.ts
src/main/daemon/terminal-host-session-reaping-leak.test.ts
src/main/daemon/terminal-host-tombstones.ts
src/main/daemon/terminal-host.test.ts
src/main/daemon/terminal-host.ts
src/main/daemon/types.ts
src/main/ipc/pty/runtime/controller.ts
src/main/ipc/pty/runtime/operations.ts
src/main/runtime/orca-runtime-preserved-branch-cleanup.ts
src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-08.spec.ts
src/main/runtime/runtime-pty-controller-contract.ts

💤 Files with no reviewable changes (4)

src/main/daemon/terminal-host-options.ts
src/main/daemon/terminal-host-tombstones.ts
src/main/daemon/terminal-host-session-reaping-leak.test.ts
src/main/daemon/terminal-host-session-create.ts

🚧 Files skipped from review as they are similar to previous changes (6)

src/main/ipc/pty/runtime/operations.ts
src/main/daemon/terminal-host-process-inspection.test.ts
src/main/daemon/terminal-host.ts
src/main/runtime/orca-runtime-preserved-branch-cleanup.ts
src/main/runtime/runtime-pty-controller-contract.ts
src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-08.spec.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

coderabbitai · 2026-09-09T05:44:11Z

@@ -194,13 +199,14 @@ export class DaemonRequestRouter {
  private async kill(
    clientId: string,
    sessionId: string,
-    immediate: boolean | undefined
+    immediate: boolean | undefined,
+    expectedIncarnationId: string | undefined
  ): Promise<Record<string, never>> {
    const canceledPendingSpawn = this.options.preparations.cancel(sessionId)


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not cancel a replacement spawn before the incarnation fence.

A stale kill request can reach Line 205 after the same sessionId starts a new creation. cancel(sessionId) has no incarnation check, so it cancels the replacement before host.kill rejects the stale expectedIncarnationId at Line 209.

When expectedIncarnationId is present, do not cancel a pending preparation by ID alone.

Proposed fix

- const canceledPendingSpawn = this.options.preparations.cancel(sessionId) + const canceledPendingSpawn = + expectedIncarnationId === undefined && this.options.preparations.cancel(sessionId)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

const canceledPendingSpawn = this.options.preparations.cancel(sessionId)

const canceledPendingSpawn =

expectedIncarnationId === undefined && this.options.preparations.cancel(sessionId)

pullfrog

✅ No new issues found.

Reviewed changes

The delta since the prior Pullfrog review is commit 1cadd1df ("fence exit release by incarnation; kill state lives on the session").

Fenced the exit-release path by incarnation — releaseExitedIncarnationFromRuntimeController now takes the incarnation and threads expectedIncarnationId through shutdown → the daemon kill RPC → TerminalHost.kill(), which throws PTY incarnation mismatch when the id now carries a newer shell. This closes the one carry-forward risk from the prior review (a recycled id between prove and release could getAliveSession-kill the new session).
Moved kill state onto the session — the "owner asked to end" fact now lives on SessionTerminationController._killRequested (survives the exit, reset only on a refused signal), replacing the TerminalHostTombstones map, TerminalHost.isKilled, and maxTombstones, all deleted.
Widened shutdown signatures — DaemonPtyRouter, DaemonPtySessionControl, and DegradedDaemonPtyProvider now type their shutdown options as Parameters<IPtyProvider['shutdown']>[1] and forward expectedIncarnationId.
Tests — wire-level (daemon-pty-adapter-kill-incarnation-fence), host-level ("refuses to end a newer shell…"), and end-to-end part-08 ("never ends a shell that took the pane id between the proof and the release") each assert the fence refuses the kill (kill/forceKill not called) and leaves the replacement shell alive.

I traced the full release chain end to end (candidate → releaseProvenExit → controller → operations → getProviderForPty().shutdown → daemon/SSH provider → host.kill) and verified the fence holds for both the daemon and SSH paths; a refused release leaves the (already-recreated-and-consumed) record alone and never ends a live shell. killRequested is set before the signal is issued (so a subprocess that exits synchronously inside the signal is still recognized at reap) and reset only on a refused signal, preserving the admission invariant.

^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

pullfrog

✅ No new issues found.

Reviewed changes

The delta since the prior Pullfrog review is commit 0cc8672 ("retain compact records for exited sessions"), which replaces the retained Session object with a scalar ExitedSession record in the daemon host map.

Compact exited-session records — TerminalHost.sessions is now Map<string, Session | ExitedSession>, and reapSession disposes the live Session then swaps in a compact {incarnationId, code} record. A naturally-exited terminal no longer pins its Session object graph (xterm scrollback grid, subprocess handle) for the daemon's lifetime; the owner-requested (killRequested) path still deletes outright.
sessionFromRecord / exitFromRecord helpers — new terminal-host-session-record.ts centralizes the union discrimination. Every reader of the map (listing, create/attach, shutdown, teardown, inspectProcess, kill) routes through getSession, and exitFromRecord answers exit evidence from either a compact record or a just-exited Session (the exit broadcast precedes reaping).
Incarnation fence reads the record — kill()'s expectedIncarnationId check now compares record.incarnationId (present on both union members), so a stale release naming an old incarnation still throws PTY incarnation mismatch instead of consuming a compact natural-exit record.
Tests — the leak regression pins that five create/exit cycles leave only plain-object scalar records (Object.getPrototypeOf(record) === Object.prototype) with one emulator disposal each, and a new inspection test pins identical proof during the exit broadcast and after reaping.

^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

pullfrog

✅ No new issues found.

Reviewed changes

The delta since the prior Pullfrog review is commit 5fd67b5 ("separate exit receipt consumption from shutdown"), which decouples releasing a proven exit from terminal shutdown.

Replaced shutdown-based release with a dedicated receipt RPC — releaseExitedIncarnationFromRuntimeController now calls the new optional IPtyProvider.consumeExitReceipt?.(id, incarnationId) instead of shutdown(id, { immediate: true, expectedIncarnationId }). This is a correctness fix, not a rename: shutdown/kill was the wrong tool for a natural exit — it recorded the session in killedSessionTombstones (claiming the owner killed it), removed history, canceled a replacement client's pending spawn, and could have ended a live shell.
Added the consumeExitReceipt wire opcode — a new daemon request routed through DaemonPtyRouter/DegradedDaemonPtyProvider/DaemonPtyProcessInspection to TerminalHost.consumeExitReceipt, which deletes only a compact ExitedSession record whose incarnationId matches. A predecessor daemon rejects the unknown request and the controller swallows the rejection, so evidence stays retained rather than falling back to shutdown.
Extracted reapSessionRecord — the reap body moved from TerminalHost.reapSession into a pure helper in terminal-host-session-record.ts (behavior-preserving); the incarnation fence and non-alive deletion in kill() are unchanged.
Tests — daemon-exit-receipt-cleanup.test.ts pins replacement preservation (including a "legacy without kill fence" host that strips expectedIncarnationId from kill), spawn-preparation non-cancellation, live-process non-kill, incarnation-scoped idempotent consumption, old-host-rejection evidence retention, and history/tombstone preservation.

^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

Evict the oldest unconsumed natural-exit evidence once the daemon reaches its receipt budget while preserving live sessions and exit-time ordering. Consolidate session-record imports so the strict static-analysis job passes.

pullfrog

✅ No new issues found.

Reviewed changes

The delta since the prior Pullfrog review is commit 6c84220 ("bound retained exit receipts"), which adds a hard cap to the daemon's held-exit retention.

Bounded retained exit receipts — terminal-host-session-record.ts caps unconsumed exits at MAX_EXIT_RECEIPTS = 1024. reapSessionRecord deletes-then-reinserts each reaped exit so the map is ordered by exit time (not process-creation time), then calls pruneExitReceipts, which evicts oldest-first and only ExitedSession records ('code' in record) — a live Session is never counted or evicted.
Eviction errs toward deferral, not false death — an evicted receipt fails the exact-incarnation match, so inspectProcess answers not-found and the recovery candidate defers to unverifiable/worker-abandon. Eviction can only withhold proof; it can never declare a live worker dead, which is consistent with the PR's deferral direction.
Tests — terminal-host-exit-receipt-budget.test.ts pins the eviction math (bounds churn without evicting live sessions, orders receipts by exit time, ignores live/acknowledged records against the budget, and never evicts evidence for an explicitly-killed process), and an integration case asserts a proof read after eviction is false while a late consumeExitReceipt is a safe no-op.

One minor note: the PR body's "What this does not add" bullets still list "No … count cap, cleanup pass", which this commit now does add — cosmetic, since the body is hand-maintained.

^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

Jinwoo-H added 4 commits September 8, 2026 18:00

coderabbitai Bot reviewed Sep 9, 2026

View reviewed changes

Comment thread src/relay/pty-handler-ssh-recovery-convergence.test.ts