Repository navigation
fix(claude): end a Claude chat on its root's exit even when a descendant survived - #22946
Conversation
…ant survived When Claude's own process exited but a descendant it started survived the close ladder (for example an MCP server that ignores SIGTERM and was born in the second the tree was snapshotted, which the ladder never force-kills), the adapter withheld `ended`, and both the lease release and every later start refused with "provider close unproven". The chat stayed stuck until Orca restarted. The root is the conversation's only writer and the lease follows it, so a first-hand root exit now ends the session whatever the tree verdict. `claudeRootExitObserved` is the one place that decides it, and crash publication, close finalization and acquisition all read it. A descendant seen alive is logged, and never reported as gone.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughAn observed Claude root exit now counts as observed regardless of the descendant-tree verdict. When exit proof fails and the root has exited while a descendant remains live, Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to A Claude root can exit while a descendant continues workspace work, yet the lease is released and a replacement may start alongside it. Keep known-live descendants in recovery before merging. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Predicate widened —
claudeRootExitObserved(claude-structured-session-close.ts:24) now keys only on a first-hand root exit, so a session is released and restarted even when the descendant-tree verdict islive. - Survivor logged —
openClaudeStreamJsonConnection.close(claude-stream-json-connection.ts:317) warns with the root pid when the root exited but a descendant survived the close ladder; the latched verdict stayslive. - Diagnostic wording — lease death evidence now reads "its descendants were not proven gone", with matching updates to the error-class, predicate, tree-verdict, and
stopAgentSessionProviderRootcomments. - Tests parameterized — the five acquisition/close/recovery cases now run over
unverifiableandliveand assert release plus restart for both.
ℹ️ The sole-writer premise is unverified for workflow-runtime agents
The change rests on the root being the conversation's only writer, which the author asked reviewers to challenge. I found no counterexample for the descendants Claude Code can leave running: subagents are in-process (their transcripts are separate files, and the main JSONL's sidechain rows are written by the root), agent-team teammates are the only separate Claude Code process and are not spawned in the -p/Agent-SDK mode Orca uses, and MCP servers, background bash tasks, and hooks communicate with the CLI over stdio rather than appending to the transcript. No action is required; the one residual is noted below.
Technical details
# Sole-writer premise for a surviving descendant
## What was checked
- Subagents (`claude-subagent-roster.ts`, `claude-child-work-evidence.ts`) are in-process; the root writes the sidechain rows in the main transcript (`claude-transcript-branch-graph.ts`).
- Teammates (a separate Claude Code process) require interactive mode; the Agent SDK / `query()` path in `claude-stream-json-connection.ts` does not spawn them.
- MCP servers, background bash tasks (`claude-background-task-*.ts`), and hooks speak stdio to the CLI; the root turns that into transcript rows.
## Residual (no action required)
- Workflow-runtime agents may be OS processes and are relaunch-guarded per session, but nothing documents those agents appending to the provider JSONL. If a future CLI release makes one a transcript writer, this predicate would need the tree verdict back.
- A surviving background bash task can still mutate the working tree while the resumed root edits — the same tradeoff #22802 accepted for `unverifiable` descendants, now extended to ones Orca can see but not kill.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
![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 GitHub limitations.
🟠 Major · Do not release the lease while the descendant tree is… · claude-structured-session-close.ts:22-27
src/main/claude/claude-structured-session-close.ts:22-27
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not release the lease while the descendant tree is live.
When
exitVerdict.rootisexitedandexitVerdict.treeislive, this predicate returnstrue. The acquisition path then settles the old session and starts a replacement in the same workspace. Supported Bash tool processes can outlive their parent turn, and no inspected cancellation or workspace-isolation guard stops a surviving process from mutating that workspace. Exclude theliveverdict at this shared boundary.Suggested fix
- return connection?.exitVerdict.root === 'exited' + const verdict = connection?.exitVerdict + return verdict?.root === 'exited' && verdict.tree !== 'live'
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 7707bb7a-49f2-43c9-a727-334deebd72a3
📒 Files selected for processing (6)
src/main/claude/claude-agent-sdk-exit-proof.test.tssrc/main/claude/claude-structured-session-adapter.test.tssrc/main/claude/claude-structured-session-close.tssrc/main/claude/claude-structured-session-startup.test.tssrc/main/runtime/agent-session-acquisition-failure-settlement.tssrc/main/runtime/claude-structured-session-integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/runtime/agent-session-acquisition-failure-settlement.ts
- src/main/claude/claude-structured-session-close.ts
- src/main/claude/claude-structured-session-adapter.test.ts
- src/main/runtime/claude-structured-session-integration.test.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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Keep a live descendant in recovery. · claude-structured-session-close.ts:22-27
src/main/claude/claude-structured-session-close.ts:22-27
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep a live descendant in recovery.
When
exitVerdictis{ root: 'exited', tree: 'live' },claudeRootExitObservedreturnstrue. The attach-failure path maps that result toroot-exit-observed, and settlement clears the owner and marks the leasereleased. A later attach can then start a replacement while the descendant remains live after bounded cleanup. The runtime fence updates lease metadata only; it does not fence the surviving process or its workspace writes.Suggested fix
export function claudeRootExitObserved( connection: ClaudeStreamJsonConnection | null | undefined ): boolean { - return connection?.exitVerdict.root === 'exited' + const verdict = connection?.exitVerdict + return verdict?.root === 'exited' && verdict.tree === 'unverifiable' }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: d9bb59c5-34e4-425e-ada1-213b6cb00f4b
📒 Files selected for processing (1)
src/main/claude/claude-structured-acquisition-release.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
Review status: ready to merge (head 081386a, CI green: 21 passed, 11 skipped)What this fixes. When Claude's own process exited but a helper it started survived Orca's cleanup, the chat got stuck until Orca restarted. An example is an MCP server that ignores SIGTERM and was started in the same second the process tree was recorded. Messages hung on "Working" with "Message delivery is unconfirmed". A first-hand exit of the Claude root now ends the session, whatever happened to its helpers. The chat restarts on its saved session, and a surviving helper is logged as Review loop (independent reviewers, each told to fix only meaningful issues):
Live QA (macOS, isolated background dev instance, real Claude CLI; screenshots in the PR body):
Scope held: one predicate, a diagnostic log line, comment and doc corrections, and tests parameterized over Known, disclosed, not caused by this PR:
Suggested follow-ups (out of scope here):
Merge order: independent of the other structured-chat PRs, based on |
ELI5
When Claude crashed or was stopped, Orca tried to make sure every helper process Claude had started was gone. If one helper refused to die (for example a tool server that ignores the polite "please exit" signal), Orca refused to consider the chat over. The chat then stayed stuck: messages failed, and it could not restart until you restarted Orca. Now Claude's own process exiting is what ends the chat. A helper that outlived it is logged, and the chat restarts normally.
What Changed
endedevent. The lease release refused, and every later start of that chat refused with "provider close unproven". For an open chat, sends failed with the crash message and nothing restarted Claude until the app restarted.claudeRootExitObserved(claude-structured-session-close.ts) is the one place that decides whether a first-hand root exit ends a Claude session. It used to requiretree === 'unverifiable', which excludedtree === 'live'. It now depends only on the root's first-hand exit. Crash publication (observeClaudeSessionExit), close finalization (finalizeClaudePublishedSession), acquisition cleanup and the host'sstopAgentSessionProviderRootall read it, so they change together. There is no new state, timer or retry.livelogs[claude-stream-json] root exited but a descendant survived the closewith the root pid. The verdict stayslive: nothing reports that descendant as gone.How a descendant stays
live, reproduced with real processes onmain: the root starts a helper that ignores SIGTERM, and the tree snapshot is taken in the same wall-clock second the helper was born. The root then crashes. The ladder sends SIGTERM but never SIGKILL, becausepsstart times have one-second resolution and it will not force-kill a pid it cannot tell apart from a reused one (hasUnambiguousStartIdentity). The retained snapshot's capture time never changes, and a dead root cannot be re-walked, so no later close can ever force it either. A process the user's account cannot signal, or one stuck in the kernel, ends the same way.Why
The lease is keyed on the root process's pid and start time (#20502), and the root is the Claude CLI itself for the launches Orca resolves: the native installer, npm and Homebrew installs, and the version-manager shims that exec into the CLI. Once Orca has seen that process exit first-hand, a surviving descendant is not a writer of the conversation, so holding the chat for it only blocks the user. That was already the rule for descendants Orca could not see (#22802). This PR applies it to descendants Orca could see but not kill.
Where the root is a wrapper that forks rather than execs, the CLI is itself a descendant. That applies to a Volta shim, an unrecognised Windows
.cmdrun throughcmd.exe, or a hand-written script. This PR does not open a new path to a second writer there. On a crash, such a wrapper waits for the CLI, so the root cannot exit first. On a stop, Orca closes the CLI's stdin and sends it SIGTERM, which the CLI exits on, so aliveCLI after the ladder needs a CLI that ignores both. The same wrapper shape already reaches theunverifiablerule from #22802, which this PR does not change. Making the root always be the CLI is separate work.How this differs from a plain stop-on-exit design: Orca still runs its descendant ladder (an identity-checked SIGTERM, then a SIGKILL only for processes it can identify unambiguously) and reports the verdict. It no longer waits on that verdict. It does not signal the whole process group.
Alternatives considered:
Deliberate behaviour change: main pinned the old rule in four tests. The rule was "a live descendant still holds the provider session: releasing would hand a second writer to it". Those tests are now parameterized over
unverifiableandliveand assert the new behaviour: a surviving descendant no longer holds the chat, and it is logged aslive. These are not weakened tests: the lease follows the root, so the surviving descendant no longer holds it.Out of scope: a root Orca did NOT see exit (an unproven stop) still fails closed everywhere, unchanged.
Linked Issue
No separate issue. This is the remaining piece of #22641, which is closed as superseded by #22802. #22802 fixed the unverifiable-tree case. This PR covers a descendant seen alive, which #22802 intentionally left failing closed.
Visual Proof
Live Electron QA on macOS: an isolated background dev instance with CDP screenshots and a real Claude Code CLI. A stand-in
claudelauncher started a helper that ignores SIGTERM in the same second the tree snapshot is taken, then exec'd the real CLI. The Claude root was then killed withkill -9after its first answer. The BEFORE arm is this branch with onlyclaude-structured-session-close.tsswapped back to main, and the built predicate was checked inout/main/index.js.[claude-stream-json] root exited but a descendant survived the close. Sending the next message started a new Claude root with--resume=<same session>, which answered. The history readsONEthenTWO, with no error row, no duplicate and no stuck turn.After: right after the crash, and the no-crash regression chat
Testing
Automated tests, each run over tree
unverifiableandlive:claude-structured-session-integration.test.ts, production-wired host:claude-structured-session-adapter.test.ts: a failed start and a post-publish cleanup classify the exit as a root exit, not as unproven.Ablation: with only the predicate reverted, all five
livecases fail. The crash-restart case fails exactly the user-visible way: no restart, and the in-flight message never settles. Theunverifiablecases stay green when run on their own (-t unverifiable). In a whole-file run, a failedlivecase leaves the shared test host unable to shut down, so later cases in the integration file fail during teardown as well.The real-process reachability check above was a scratch test, not committed: its outcome depends on wall-clock second boundaries and on process-table reads that time out under load.
pnpm tc:node,oxlinton the touched paths andcheck:code-quality:changedare clean. The Claude adapter, close, startup, connection, root-exit and settlement suites pass.AI Disclosure
Review
Please challenge the claim the change rests on: that nothing but Claude's own process writes the conversation, so a surviving descendant never makes a restart unsafe.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
live/unverifiable/exited. Loss of contact is still never treated as death: only a root exit Orca saw first-hand ends the session.liveis likelier there. This PR turns those cases from a stuck chat into a logged leftover process. Killing that snapshot is separate work in the shared kill code.Checklist
N/Awith reason