fix(claude): let the lease release close the adapter's session so an evicted chat resumes - #22641
brennanb2025 wants to merge 3 commits into
Conversation
|
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 (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughClaude session release now retires provider-side session state and adopts released connections for bounded cleanup attempts. Unexpected exits are retained and settled through lifecycle helpers. Acquisition checks for superseded sessions before selecting a session to resume. Release acknowledgments carry fences through the router and host paths, where stale acknowledgments do not remove newer routes. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A resumed Claude session can reuse its provider handle while an earlier child is still live. Resolve that retirement path before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Fence checks protect newer sessions from stale releases, but a resumed chat can overlap with helpers from its previous session when those helpers cannot be verified as stopped. Cleanup retries are bounded and may end without that verification. 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 is detailed and covers the required explanation, changes, rationale, testing, visual proof, compatibility considerations, and checklist. However, the required Linked Issue section states "None," while the template requires an issue link.
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 02f51ba2-36db-45e6-a314-5edbc38e6320
📒 Files selected for processing (12)
src/main/claude/claude-released-child-cleanup.test.tssrc/main/claude/claude-released-child-cleanup.tssrc/main/claude/claude-structured-acquisition-launch.tssrc/main/claude/claude-structured-session-adapter.tssrc/main/claude/claude-structured-session-exit-lifecycle.tssrc/main/claude/claude-structured-session-release.test.tssrc/main/claude/claude-structured-session-retirement.tssrc/main/claude/claude-structured-session-state.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter.tssrc/main/native-chat/agent-session-wire/structured-agent-session-claude-root-exit.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| export function retireClaudeSessionSupersededByFence( | ||
| input: ClaudeSessionRetirementInput & { fence: number } | ||
| ): ClaudeSession | undefined { | ||
| const indexed = input.sessions.get(input.sessionId) ?? input.exits.get(input.sessionId)?.session | ||
| if (!indexed || indexed.fence >= input.fence || indexed.connection.exitVerdict.root === 'live') { | ||
| return undefined | ||
| } | ||
| return retireClaudeReleasedSession(input) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not skip the tree check when a descendant was seen alive.
The guard rejects only exitVerdict.root === 'live'. It still retires an exit with { root: 'exited', tree: 'live' }. That verdict is positive evidence that a descendant is still running. settleClaudeUnexpectedExit withholds ended for this verdict. Before this change, acquisition threw AgentSessionAcquisitionExitUnprovenError for it. Now resolveClaudeAcquisitionLaunch resumes the same provider session while the old descendant can still write to its transcript. A later fence is not proof that the old holder stopped: a lease can expire or be released without the process exiting. Also reject tree === 'live' here.
Proposed fix
- if (!indexed || indexed.fence >= input.fence || indexed.connection.exitVerdict.root === 'live') {
+ const verdict = indexed?.connection.exitVerdict
+ if (
+ !indexed ||
+ indexed.fence >= input.fence ||
+ verdict?.root === 'live' ||
+ verdict?.tree === 'live'
+ ) {
return undefined
}Based on learnings: an expired-lease takeover "does not guarantee the previous holder actually stopped working."
📝 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.
| export function retireClaudeSessionSupersededByFence( | |
| input: ClaudeSessionRetirementInput & { fence: number } | |
| ): ClaudeSession | undefined { | |
| const indexed = input.sessions.get(input.sessionId) ?? input.exits.get(input.sessionId)?.session | |
| if (!indexed || indexed.fence >= input.fence || indexed.connection.exitVerdict.root === 'live') { | |
| return undefined | |
| } | |
| return retireClaudeReleasedSession(input) | |
| } | |
| export function retireClaudeSessionSupersededByFence( | |
| input: ClaudeSessionRetirementInput & { fence: number } | |
| ): ClaudeSession | undefined { | |
| const indexed = input.sessions.get(input.sessionId) ?? input.exits.get(input.sessionId)?.session | |
| const verdict = indexed?.connection.exitVerdict | |
| if ( | |
| !indexed || | |
| indexed.fence >= input.fence || | |
| verdict?.root === 'live' || | |
| verdict?.tree === 'live' | |
| ) { | |
| return undefined | |
| } | |
| return retireClaudeReleasedSession(input) | |
| } |
Source: Learnings
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
retireClaudeSessionSupersededByFence retires on root !== 'live', so it also retires a child whose verdict is { root: 'exited', tree: 'live' } — a state this PR deliberately treats as non-resumable elsewhere. Details inline.
Reviewed changes
- Lease release closes the adapter session —
acknowledgeSessionReleasenow forwards through the router to the owning adapter, which drops the session/exit from its index and settles what they owned. - Fence-based retirement on resume —
resolveClaudeAcquisitionLaunchretires an indexed child at an older fence instead of re-closing it, and deletes a settled retained exit. - Bounded descendant cleanup — the new
ClaudeReleasedChildCleanupre-runs the close ladder on unref'd timers, then reports and drops; nothing reads it before acquiring. - Unexpected unverifiable exit publishes
ended— a root exit withtree: 'unverifiable'now emitsendedand keeps the exit indexed as evidence;tree: 'live'still withholds. - Tests — new cleanup, release, router, and root-exit eviction tests; the exit lifecycle moved into
claude-structured-session-exit-lifecycle.ts.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| input: ClaudeSessionRetirementInput & { fence: number } | ||
| ): ClaudeSession | undefined { | ||
| const indexed = input.sessions.get(input.sessionId) ?? input.exits.get(input.sessionId)?.session | ||
| if (!indexed || indexed.fence >= input.fence || indexed.connection.exitVerdict.root === 'live') { |
There was a problem hiding this comment.
This guard retires on root !== 'live', so a verdict of { root: 'exited', tree: 'live' } — root gone, descendant observed alive — is retired too. This PR otherwise treats that state as non-resumable: retainClaudeUnexpectedExit withholds ended for it, and src/main/runtime/claude-structured-session-integration.test.ts:458 keeps the lease reserved/manual-recovery so a live descendant cannot be handed a second writer. The premise that a later fence implies a lease release does not hold in general, so a re-attach can resume on top of the live descendant.
Technical details
# Fence retirement ignores a live descendant
## Affected sites
- `src/main/claude/claude-structured-session-retirement.ts:56` — guard is only `exitVerdict.root === 'live'`; `tree === 'live'` passes.
- `src/main/claude/claude-structured-session-exit-lifecycle.ts:75-84` — `{root:'exited', tree:'live'}` deliberately returns without `settle`, so no `ended` and the exit stays indexed.
- `src/shared/agent-session-lease-adjudication.ts:163-174` — `evaluateAgentSessionAcquisition` grants `nextFence` whenever the recorded root probe is proven dead (`pid-absent` / `identity-mismatch`), with no `released` transition.
- `src/main/runtime/agent-session-lease-transitions.ts:76-97` — the reservation overwrites the lease with `claimStatus: 'reserved'`, `ownerProcess: null`, `deathEvidence: null`.
- `src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts:79-81` — every attach probes the recorded owner (the root) before reserving.
## Repro shape
1. Publish at fence 7; root exits with `tree: 'live'` → session moves to `exits`, no `ended`, lease stays `live` at 7.
2. Client re-attaches at `expectedRuntimeFence: 7`; the root probe is `pid-absent` → reservation grants fence 8.
3. `adapter.acquire({ fence: 8 })` → `retireSuperseded(..., 8)` sees `7 < 8` and `root === 'exited'` → retires the exit.
4. `retainedExit` is now `undefined`, so the `if (!proven) throw claudeAcquisitionCleanupError(...)` refusal is skipped and the resume proceeds.
Pre-PR the same acquire hit that throw and was refused with `agent_session_ownership_unknown`.
## Required outcome
- `retireClaudeSessionSupersededByFence` must not retire a child whose `exitVerdict.tree === 'live'`, so the close-ladder refusal runs as it does for a live root.
## Suggested approach
- Extend the guard to bail when `indexed.connection.exitVerdict.tree === 'live'` (reuse the `treeProven` shape from `claude-released-child-cleanup.ts`), and add a test that acquires at a later fence over a retained `{root:'exited', tree:'live'}` exit and asserts the acquire is refused.
There was a problem hiding this comment.
ℹ️ No new issues in the follow-up commit. It closes a stale-acknowledgement hole in the release path cleanly; the one substantive concern from the prior review (
retireClaudeSessionReleasedThroughstill retires a{ root: 'exited', tree: 'live' }child) is unchanged by these commits and remains tracked in its open thread.
Reviewed changes
- The acknowledged release is now fenced —
StructuredAgentSessionAdapterRouterrecords the acquisition fence per route and only forwardsacknowledgeSessionRelease(sessionId, releasedFence)(and drops the route) whenreleasedFenceis at or above that fence, so a duplicate or late release cannot retire a route acquired since. - Adapter retires by released fence —
retireClaudeSessionSupersededByFencebecameretireClaudeSessionReleasedThrough({ releasedFence }); the resume path calls it withreleasedFence: fence - 1, preserving its "strictly older child" semantics while letting the acknowledgement retire at its own fence. - Call sites pass the pre-release fence — host eviction, host teardown, and the native handoff forward now carry the fence captured before the lease advanced, so a later acquisition at that fence is never mistaken for the released one.
- Tests — new router and adapter cases asserting a stale release leaves a newer route/child alone, and the release/teardown/handoff assertions now pin the fence argument they expect.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in the follow-up commit. It turns a withheld
endedinto bounded, self-terminating cleanup; the one substantive concern from the prior review — the fence rule still retires a{ root: 'exited', tree: 'live' }child — is unchanged by these commits and remains tracked in its open thread.
Reviewed changes
- A withheld exit is now re-checked by the bounded cleanup —
retainClaudeUnexpectedExithands a{ root: 'exited', tree: 'live' }exit toClaudeReleasedChildCleanup, which publishesendedwhen a retry proves the tree gone and at give-up otherwise, so a surviving descendant can no longer wedge the chat until restart. endedpublication is modelled as a state —endedWithTreeUnproven?: booleanbecameended?: 'withheld' | 'published';settleClaudeUnexpectedExitdeletes the exit unless it published, andproviderHistoryWindowreads the new state forhasLiveSession.- The cleanup reports its outcome once —
adoptaccepts a one-shotonSettled(treeProven)callback, invoked on proof (true) or give-up (false), clearing itself before calling so it cannot fire twice. - The fence rule moved to a provider-neutral module —
isAgentSessionChildReleasedThroughFenceinstructured-agent-session-fence-retirement.ts; the guard's logic is identical to the inline check it replaced. - Tests — new cleanup-settlement case and two withheld-exit publication cases (proof and give-up);
pnpm testonclaude-released-child-cleanup.test.tsandclaude-structured-session-release.test.tspasses locally.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…evicted chat resumes Eviction releases a Claude lease once the provider root has exited, even when its descendants are unverifiable. The Claude adapter kept that session indexed until the whole tree was proven gone, and every later acquire re-ran the same close first, so a hidden chat (or an orchestration mail wake) could not resume until the app restarted. - The host's lease release is now the adapter's close decision: the router forwards acknowledgeSessionRelease to the owning adapter, and the Claude adapter drops and settles the session and any retained exit. - An acquire at a later fence than an indexed child retires it instead of closing it again; a child whose root was never seen to exit still goes through the close ladder. - Unproven descendants move to a bounded, connection-keyed cleanup that no acquire consults. It retries the close ladder on a fixed schedule, then reports the last verdict instead of claiming the tree gone. - An unexpected exit whose root exited with an unverifiable tree now publishes `ended`, so the host releases and recovers the session. The exit stays indexed as evidence until the release retires it, so acquisition cleanup keeps reporting a root exit and never a proven one.
…re a newer child The host's release acknowledgement now carries the fence it released. The router and the Claude adapter both retire only a child at or below that fence, through the same fence rule an acquisition uses, so a stale acknowledgement cannot reach a child acquired since. The rule's comment now names both ways the lease grants a later fence.
…t wedge the chat An unexpected exit whose close saw a descendant alive withheld `ended` with nothing re-checking it, so the host kept a dead child and the visible chat stayed wedged until restart. The withheld exit now joins the bounded released-child cleanup: a retry that proves the tree publishes `ended`, and giving up publishes it with the live verdict reported. The exit's publication is modelled as a state instead of a boolean, and the fence rule moved to a provider-neutral module so Codex can reuse it.
0c8b6c9 to
f9fad0b
Compare
This branch has not been deployed
1 participant
ELI5
When you hide a Claude chat, Orca stops its Claude process to save resources and hands the chat's "lease" back so it can be restarted later. On many machines Orca can see Claude itself exit but can't prove every helper process it started is gone. The lease was handed back anyway, but the Claude side of Orca still remembered the old process and refused to start a new one until it could prove the old helpers were gone. It never could. So re-opening the chat, or waking it with orchestration mail, failed until you restarted the app. Now handing back the lease is what "closed" means everywhere. The leftover helper check keeps running in the background for a limited time and never blocks a restart.
What Changed
Before: a hidden structured Claude chat evicted with an unverifiable process tree went
reserved→releasedon every later hold, send or mail wake, withprovider close unproven, until Orca restarted.After: the chat resumes in the same run. The mechanism:
StructuredAgentSessionAdapterRouter.acknowledgeSessionReleasenow forwards the host's release to the owning adapter before dropping the route. The acknowledgement carries the fence it released. The router and the adapter both retire only a child at or below that fence, through the same fence rule an acquisition uses, so a late acknowledgement cannot retire a child acquired since.ClaudeStructuredSessionAdapter.acknowledgeSessionReleaseremoves the session and any retained exit from its index and settles what they owned: dispatch waiters, prompts, translator, reading control and background tasks (claude-structured-session-retirement.ts). Codex has no such method, so its path is unchanged.resolveClaudeAcquisitionLaunchretires an indexed child whose fence is older than the acquisition's fence instead of closing it again. The lease grants a later fence in two ways. One is after a release. The other is over an unreleased lease whose owner probe proved the recorded process dead (evaluateAgentSessionAcquisition,src/shared/agent-session-lease-adjudication.ts). Either way the old fence no longer owns the session, so this also covers release paths that never send an acknowledgement, such as acquisition-failure settlement. Neither grant is evidence about this particular connection, so a child whose root was never seen to exit still goes through the close ladder first.isAgentSessionChildReleasedThroughFencelives innative-chat/agent-session-wire/structured-agent-session-fence-retirement.ts, so a Codex follow-up can reuse it rather than build a parallel one.ClaudeReleasedChildCleanupkeys unproven children by connection, not session id, and no acquisition reads it. It re-runs the connection's own close ladder at 5s, 30s and 120s. After that it logs the last{ root, tree }verdict and drops the child.closeAllgives each child one final attempt and reports, without throwing. It never says a tree is gone unless it saw it go.ended. I confirmed the suspected gap: on main, a Claude CLI that exits on its own without a tree proof never publishesended, so the host keepshasProviderChildand a visible chat stays attached to a dead child. The exit now publishesendedon the root's first-hand exit, so the host settles the turn, releases the lease and runs recovery. A descendant seen alive (tree: 'live') only defers it. The withheld exit joins the same bounded cleanup schedule. A retry that proves the tree gone publishesended. At give-up,endedis published and the report names the descendant aslive, never gone. Without this, a killed Claude root with a surviving MCP server would leave the host holding a dead child, and the release clock never retries a failed eviction, so the visible chat stayed wedged until restart. The exit's publication is modelled as a state (ended: 'withheld' | 'published', and a proven exit leaves the map) rather than a boolean. Apublishedexit stays indexed as evidence until the release or a later fence retires it. Until then, acquisition cleanup keeps reporting a root exit and never a proven one.providerHistoryWindowstops reporting a turn in flight for an exit that already publishedended.handleExitandsettleUnexpectedExitmoved intoclaude-structured-session-exit-lifecycle.tsso the adapter stays under the line cap. Their behaviour is unchanged apart from the two points above.Why
Why a fence rule and not just the acknowledgement. The fence is the fact every grant path already carries. An acquisition at fence F proves F-1 no longer owns the session, however the lease got there: an eviction, an acquisition-failure settlement, an unexpected-exit settlement, or a probe that proved the owner dead. Deriving "released" from it means no release site has to remember to notify the adapter. The acknowledgement then only makes retirement prompt, and it uses the same rule, bounded by the fence it names.
The window on release paths that send no acknowledgement. Acquisition-failure settlement releases the lease without an acknowledgement. Between that release and the next acquire, the adapter still holds the old entry, so its synchronous readers can serve a dead child's state until the next acquire retires it. Those readers are
backgroundTaskState,readCommands,recordsContextUsageandproviderHistoryWindow. This gates no user action and ends at the next acquire or quit. It is the residue of deriving "released" at acquire time rather than pushing it from every release site.Codex keeps the opposite lease semantics for now. Its close returns
falsewhen the exit is unproven, so eviction aborts and the lease stays live. A hidden Codex chat with an unprovable close therefore still latches, and so can a Codex coordinator's mail wake. Fixing that changes shared eviction semantics and needs its own ablations. The follow-up should adopt the same shape: the fenced acknowledgement, the provider-neutral fence rule above, and a bounded cleanup.How a retained exit ends. An exit that published
endedwith an unproven tree stays indexed only as evidence. It ends in one of three ways, and none of them waits on the tree:closeAllat quit gives it one final close attempt and reports."Closed" had two owners. The host's lease had already decided the child was closed (#20502: ownership follows the provider root). The adapter's session-id index kept a stricter, tree-proven notion of closed, and made it a precondition for acquisition. This PR removes the second owner instead of adding a guard around it. The index entry now ends with the lease. The descendant obligation still exists, but it has an exit: a bounded schedule and a report. It no longer blocks a user action. An index entry that ends with the close, whatever the close proved, and a restart that never waits on proof of the old tree, is the established pattern for provider session registries.
Alternatives considered:
Deviation, stated plainly: Orca still verifies the descendant tree, which a plain "delete on close" registry does not. It now does so only as bounded background bookkeeping.
Linked Issue
None — part of the structured chat status/orchestration program. This unblocks the orchestration mail wake in #22631, whose wake hold failed with
provider close unproven.Visual Proof
N/A. There is no UI change. The fix is to lease and process lifecycle. Live evidence from the app is below.
Testing
ORCA_BACKGROUND_LAUNCH=1, CDP only)Regression tests. The new and changed tests are red on main (main's modified sources with this branch's tests) and green here:
closeSessionafter release (main's pinned assertion, flipped)provider exitedtrueRootExitObservedError: provider close unproven, 1 connectionended[]ended(unexpected-exit, fence 7)endedis outturnInFlight: truefalseprovider close unprovencloseCount1Ablations. Each piece was deleted on the committed branch, with the listed tests going red:
providerHistoryWindowchange deleted: 1 red.Real binary (scratch test, not committed, using the installed
claudeCLI and the real connection and close ladder). On this Mac an orderly close of a real Claude child returnsroot exited / tree unverifiable.AgentSessionAcquisitionRootExitObservedError: provider close unproven, with and without an acknowledgement.Live in the app (this branch, CDP only):
OK).releasedat fence 2.agentSession.holdRPC, the same path a surface hold or mail wake takes. The lease wentliveat fence 3 with a second (resumed) link, and a new Claude pid started.released child tree was not verified gone { sessionId: 'claude_4a3a…', pid: 61596, verdict: { root: 'exited', tree: 'unverifiable' } }.This shows the evicted child took the unverifiable path, and the resume did not wait on its tree.
Suites run:
src/main/claude,src/main/native-chat,src/main/runtime,src/main/codex, with 13,411 tests passing. The only failures are 5 real-CLI tests (claude-structured-real-cli,claude-tui-resume-real-binary). They fail identically on main on this machine, because theircloseAllhitsprovider close unprovenagainst the real binary. They are skipped in CI, where no real binary is installed.pnpm tc:nodepasses, fullpnpm exec oxlintexits 0, andcheck:code-quality:changedandaudit:anti-slopare clean.Not verified:
desktop-chathold, and its cleanup sendsagentSession.release. This PR touches no renderer or hold code, so it neither causes nor masks that. It needs its own investigation. The two suspects are the hold hook silently swallowing a failedagentSession.release, and a retaining read-stream hold kept open while undelivered sends exist.Review
Please challenge two things:
retireClaudeSessionSupersededByFence, which retires only when the indexed fence is older and the root is notlive.Agent skill upstream boundary
Notes
supportsClaudeStructuredLocation). The verdict vocabulary stayslive/unverifiable/exited. Nothing here treats loss of contact or a newer fence as evidence of death: a root not seen to exit still has to be closed before a resume.falsewhen unproven, so eviction aborts and the lease stays live. Codex defines noacknowledgeSessionRelease, so the router forward is a no-op for it. Codex tests pass unchanged. Its latch remains a separate issue.endedWithTreeUnprovenexists only in memory.Merge order
This PR is independent of the structured-chat stacks and is based directly on
main.It overlaps with #22364 (open), which edits
claude-structured-session-adapter.ts,claude-structured-session-state.ts,claude-structured-session-close.tsandstructured-agent-session-claude-root-exit.test.ts. Expect textual conflicts in the adapter (itshandleExit/settleUnexpectedExitedits, now inclaude-structured-session-exit-lifecycle.ts), in the state types, and in the root-exit test (providerChildPhase: 'ready'on the host session). The semantic overlap is small:endedonly for a failed startup whose root exited. This PR publishes it for any root exit with an unverifiable tree, which subsumes that case. The resolution should use fix(claude): open structured chat without a startup deadline, and make Retry start fresh #22364'sclaudeRootExitObservedpredicate inretainClaudeUnexpectedExit, keep itsfailClaudeStartupGate,settledExitErrorsandstartupUnprovenadditions, and keep this PR's evidence retention.claude-structured-launch-resolution.ts.Checklist
N/Afor screenshots, with reason