Repository navigation
fix(claude): let a Claude chat start again after its root exited with unverifiable descendants - #22802
Conversation
…gain When the idle release clock stopped a Claude chat whose root process exited but whose descendants could not be verified, the host released the lease and forgot the session, as designed. The adapter, though, kept its session indexed as not yet closed. Every later start of that chat first tried to close the stale session again, got the same verdict, and was refused with "provider close unproven". The chat then read as "Could not load conversation" until the app restarted. A proven root exit or processless close now finalizes the adapter session like a proven close: it persists the resume point, emits the end, and drops the entry, and it still reports the verdict to the caller. Only a genuinely unknown exit stays indexed for a retry.
…rt again The same refusal had a second emitter. A Claude root that crashed with unverifiable descendants stays recorded as an exit, and the next start of the chat re-checked that exit and refused on the root-exit verdict, although that verdict is what let the host release the lease. The start now proceeds and settles the recorded exit; only an exit this host cannot vouch for still blocks it.
… exit go ahead Closing the previous session finalizes it before reporting the root-exit verdict, so the start that closed it must not fail on that verdict, the same as a retained root exit.
…op site Idle eviction already treated a stop whose provider root was seen to exit as a stop, but handing a chat to the terminal and recovering from a journal write failure treated the same verdict as a failure. Now that the Claude adapter finalizes the session before reporting that verdict, those two paths left the host holding a child the adapter had already ended: the first hand-off failed with "provider close unproven" and sends failed until the chat was evicted. One function now reads a stop's result for eviction, hand-off, sink-failure recovery and the adapter's own start-over-a-live-session path.
…nts went unverified A Claude crash was published to the host only once its whole process tree was proven gone, or when it failed during startup. A crash whose root was seen to exit but whose descendants could not be verified was held back, so the chat stayed marked live: sends failed with the crash message and nothing restarted Claude until the idle clock stopped the chat. The owner already releases the lease on a first-hand root exit, so the crash now publishes like a proven one and the host reconciles and recovers at once.
…ants went unverified The crash-publication test only asserted the lease was released, so nothing showed the restart the fix exists for. It now holds the chat as an open surface does and asserts Claude comes back on the saved session with a live lease.
…ss a root-exit crash restart The restart test now also covers the sends around the crash: a message Claude took but never echoed settles as exited before acknowledgement instead of waiting forever, and a send after the restart is accepted by the new process.
Review statusThe chat-reopen bug is fixed and verified in automated tests. Code review found no remaining problems. Live Electron QA is still outstanding: screenshots will be attached from the separate QA run. What the PR fixes
What review changed
Deliberate behavior changeChild processes that couldn't be verified at the first cleanup are not signalled again later. A leftover background command can keep running while the restarted Claude works. That was already the case after an idle stop followed by a reopen. Validation
Known gaps, left for a follow-up PRThe follow-up makes a stop return its result as a value instead of throwing an error after it succeeds. That closes two narrow windows:
Older issues, not caused by this PR
|
Live Electron QA (whole stack)Isolated dev instance (hidden window, driven over CDP), macOS, real Claude Code sessions. The idle window was shortened to 45 s in the QA build only. A "before" build swaps one fix's files back to
The scenario that crashes Claude mid-chat was not driven live; the #22802 integration tests cover it. Windows, Linux and SSH were not run live: the changes are host-side and not platform-specific, and no wire format changed. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Adapter now agrees a root-exit close is final (
claude-structured-session-close.ts):finalizeClaudePublishedSessionpersists the resume cursor, emitsended, and drops the session entry when the close returned false with a root-exit-observed or processless verdict, then re-throws the verdict; only a genuinely unknown exit stays indexed. A later close short-circuits oncloseFinalized, so the stale entry no longer refuses every subsequent start. - Start proceeds over a stopped/released session (
claude-structured-acquisition-launch.ts): the launch wraps close withstopAgentSessionProviderRoot, and a retained exit that cannot be proven is settled instead of throwing unless the verdict is genuinely unproven. - Crash with unverifiable descendants is published (
claude-structured-session-exit-lifecycle.ts): dropping thestartupFailedqualifier means any first-hand root exit settles the in-flight turn and restarts an open chat; startup crashes keepstartupUnprovenand still wait for the next send. - One normalized stop reading (
structured-agent-session-adapter.ts): newstopAgentSessionProviderRoottreats root-exit/processless as stopped and rethrows everything else (known-live descendants stay unproven), wired into idle eviction, terminal hand-off, journal-sink recovery, and the adapter's own start. - Tests: adapter, close, root-exit eviction, surface-lifetime, and integration suites cover the reopen, hand-off, crash-restart, and journal-failure paths.
Verification performed in this review
pnpm teston the five changed test files: 83 passed.pnpm tc:node: clean.- Reverted the
observeClaudeSessionExitchange in isolation and re-ran the adapter + integration suites: 5 tests failed, confirming the new tests actually pin the publication behavior.
The only noteworthy tradeoff — a retained root-exit record is no longer kept, so a later close no longer re-runs the tree check on leftover descendants — is deliberate and documented in the PR body. tree: 'live' still fails closed at every site.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughA shared helper now interprets provider-root stop results across eviction, event recovery, and host handoff. Claude session closing retains proven root-exit or processless verdicts until finalization completes. Observed root exits can settle as unexpected exits even when descendant closure is unproven. Tests cover reacquisition, session handoff, journal-sink recovery, and recovery after a crash during send. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The changed exit handling supports restarting Claude after a confirmed root exit, and no merge-blocking issue remains identified. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to A chat can now restart or hand off after Claude’s main process exits even when its other processes cannot be verified. A known-running child still blocks release, but the remaining processes’ access in the unverifiable case has not been established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the user impact, mechanism, rationale, testing, visual proof, and cross-platform considerations. However, the required Linked Issue section does not provide a linked issue in the required
✨ 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 |
Main removed the terminal handoff (#22783), so its root-exit stop reading and the handoff integration test are dropped with it.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review, this PR merged origin/main (d443320), which removed the terminal handoff subsystem (#22783). The net change to this PR is a scope reduction: the stopAgentSessionProviderRoot wiring at the now-deleted terminal hand-off suspendNative site and its handoff integration test are dropped.
- Dropped the obsolete hand-off stop site (
structured-agent-session-host-handoff.ts): the file is gone upstream, so itsstopAgentSessionProviderRootwiring went with it. The three remaining production stop sites — idle eviction, journal-sink failure recovery, and the adapter's own start — still route through the shared helper. - Dropped the corresponding integration test (
structured-agent-session-surface-lifetime.test.ts): the "hands a chat to the terminal…" case exercised the removed suspend path; nothing replaces it because the path no longer exists.
All other production and test changes are byte-for-byte identical to the version already reviewed and approved. The merge did not silently drop any intended change, and the newly-added tests still compile and pass against the merged tree.
One stale-prose note for whoever edits the description: the PR body still lists structured-agent-session-host-handoff.ts suspendNative as one of the wired stop sites. Code and tests are correct; only the prose needs the bullet removed.
Verification performed in this review
pnpm teston the five changed test files: 80 passed.pnpm tc:node: clean.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

ELI5
Orca stops a Claude chat's background process after a while with nobody looking at it, and starts it again when you come back. When Claude's main process had exited but Orca couldn't confirm that every process it had started was gone, the chat could never be started again. Coming back showed "Could not load conversation", and every message failed until you restarted Orca. Now the chat starts again normally.
What Changed
agent_session_operation_invalid. The saved failure reads "provider close unproven". The pane showed "Could not load conversation", and sends failed too.observeClaudeSessionExitinclaude-structured-session-exit-lifecycle.ts), so the host settles the turn, releases the chat and, while the chat is open, restarts Claude on its saved session at once. A crash during startup still waits for the next send, as before. A start that lands before that publication settles the recorded exit itself instead of refusing (resolveClaudeAcquisitionLaunch). Only an exit this host cannot vouch for (Claude's own process not confirmed gone) is still held back and still blocks the start.resolveClaudeAcquisitionLaunch.stopAgentSessionProviderRootinstructured-agent-session-adapter.ts, now reads a stop's result for idle eviction, journal-failure recovery and the adapter's own start over a still-recorded session, so they cannot disagree again. Only Claude reports this verdict from a close, so other agents are unaffected.finalizeClaudePublishedSessioninclaude-structured-session-close.ts: when a close sees the root exit but cannot prove the descendants gone, the host already treats it as stopped. It releases the session's lease and forgets the session (fix(native-chat): release sessions after provider root exit #20502: the lease follows the provider root). The Claude adapter, however, returned early and kept its own session entry. Each later start of that chat first tried to close that stale entry again, got the same verdict, and threw. The close now finishes like a proven close: it saves the resume point, emits the end, and drops the entry, and it still reports the root-exit verdict to the caller. Only a genuinely unknown exit, where the root itself is unverified, stays indexed for a retry.Why
The host and the adapter disagreed about whether the session was over. The host's rule is the deliberate one from #20502: seeing the provider root exit first-hand is enough to release a root-owned lease. The adapter was the side contradicting it, so the fix is to make the adapter agree, not to add a retry or a special case at re-attach.
Alternative considered: tolerate the root-exit verdict at re-attach and drop the stale entry there. That leaves the session half-finalized: no end event, no saved resume point, and a translator still bound. It also has to be repeated at every place that closes a stale session.
What this does not change: a Claude process the host has not seen exit, or one with a child process seen still running, still fails closed everywhere.
Linked Issue
No separate issue. Found in live QA of #22794 and #22801: the first chat the idle clock stopped on desktop could not be reopened. It is reachable on
maintoday wherever the idle clock stops a Claude chat, for example after a phone leaves a chat. #22801 would make it reachable from every desktop chat you leave, so it must land before #22801.Visual Proof
Live Electron QA on an isolated dev instance (hidden window, CDP), macOS, real Claude Code sessions. The idle window was shortened to 45 s in the QA build only. "Before" builds swap in one file set from
mainat a time and restart the app. Process liveness comes frompson the provider pid the app spawned.Testing
claude-structured-session-close.test.ts: a close that sees the root exit with unverifiable descendants still rejects with the root-exit verdict. A second acquisition of the same chat then opens a new connection, and the old one is not closed again. Fails without the fix with "provider close unproven".claude-structured-session-close.test.ts: a start over a still-recorded session whose close sees the root exit opens a new connection, and the old session ends exactly once. Fails without the start-side change with "provider close unproven", and without the close change because no end is emitted.claude-structured-session-adapter.test.ts: a crash whose root exited with unverifiable descendants publishes its end to the host, and a later release reports the session stopped. A start made before that publication opens a new connection instead of refusing. Each fails without its change.claude-structured-session-integration.test.ts: production-wired host. With the chat open and a message in flight, the same crash settles that message as "exited before acknowledgement", restarts Claude on the saved session, and the next send is accepted by the new process. Without the publication change the in-flight message never settles and the test times out.structured-agent-session-surface-lifetime.test.ts: a failed journal write whose stop reports the root-exit verdict still settles the exit and releases the lease. Fails without the recovery change.Each production change was reverted on its own and a test above failed for each. The eviction change only moves existing logic into the shared function, so reverting it stays green; removing the function's tolerance there fails the existing root-exit eviction test and the reopen test.
claude-structured-session-integration.test.ts: production-wired host and RPC. Create a Claude chat, stop it with that exit verdict, and hold it again as a returning chat surface does: a new provider connection starts and the session is back. Fails without the fix withagent_session_operation_invalid, the same failure seen in the live app.The existing root-exit eviction test now asserts that the adapter agrees the session is over after eviction; it used to assert that the crash record was kept. The Claude adapter suites (including the real-CLI suite), the integration suite and all 113 structured-session host test files pass: 1880 tests. There are two exceptions. "leaves unlisted shell exports out when inheritance is off" fails identically without this change: my shell's own Anthropic key leaks into it. The real-CLI "proves a pre-minted session" test needs a local Claude CLI and fails on
maintoo.pnpm tc:node, oxlint andcheck:code-quality:changedare clean.I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
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
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)