Repository navigation
fix(session): scope agent resume to the host that captured the session - #21288
Conversation
A provider session id names a transcript in one machine's agent state directory. Nothing in the resume path compared that machine against the one the resume executes on, so a record captured on host A reached a `--resume` run on host B, which answers `No conversation found with session ID`. Three things make the drift reachable: `worktreeId` is `repoId::path` with no host component, sleeping records are `'sleepingAgentKeyed'` so boot-time host-contention parking never arbitrates them and every partition merges into one map without retaining provenance, and both issuers resolve their launch target from the current catalog. Both issuers are gated. The activation sweep hands `quit`/`live` records whose pane still exists to the pane's own cold restore, so gating the sweep alone changed nothing in the SSH lane. Declines rather than guesses: the record is preserved and remains resumable by hand. A refused resume is recoverable, a forked transcript is not. The predicate fails open on anything it cannot positively rule out -- an unstamped record, an empty stamp, or a `runtime:` host, which a paired client uses to relabel its host's own SSH workspaces. The cold-restore gate consults both the pane's transport and the catalog. The transport alone was racy: it is unresolved on an early reattach frame, and that frame is exactly when a wrong resume escaped.
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change adds execution-host scope checks for sleeping-agent session resumption. Foreign records are skipped or left as plain shells instead of issuing a resume. Legacy and native records remain resumable. Unit tests cover host identifiers and resume batches. A new SSH Docker end-to-end suite verifies foreign and owning host behavior across restart, and the runner executes the suite. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Valid sessions can fail to restore, and a local sleeping session can be removed without being resumed when a newer foreign record shares its claim. These resume regressions should be resolved before merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the bug, implementation, rationale, and testing in detail, but it omits several required template sections: What Changed, Why, Linked Issue, Visual Proof or an explicit N/A explanation, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist. Resolution Add the missing required sections and complete them. Include the linked issue using
✨ 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: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: c46070fb-d824-494c-b5fe-c6371a59c301
📒 Files selected for processing (6)
config/scripts/run-ssh-docker-e2e.mjssrc/renderer/src/components/terminal-pane/pty-connection/cold-restore-resume-startup.tssrc/renderer/src/lib/resume-sleeping-agent-session-execution-host-scope.test.tssrc/renderer/src/lib/resume-sleeping-agent-session.tssrc/renderer/src/lib/sleeping-record-execution-host-scope.tstests/e2e/ssh-stale-resume-execution-host-scope.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| useLiveEntry ? entry.connectionId : sleepingRecord?.connectionId, | ||
| session.executionHostId | ||
| ) || | ||
| (sleepingRecord && sleepingRecordNamesAnotherExecutionHost(sleepingRecord, state)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,120p' src/renderer/src/components/terminal-pane/pty-connection/cold-restore-resume-startup.ts
rg -n 'buildColdRestoreAgentResumeStartup|useLiveEntry|sleepingRecord' src/renderer/src/components/terminal-paneRepository: stablyai/orca
Length of output: 13542
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- startup remainder and type ---'
sed -n '110,145p' src/renderer/src/components/terminal-pane/pty-connection/cold-restore-resume-startup.ts
sed -n '1,45p' src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-types.ts
printf '%s\n' '--- getSleepingRecordForPane declarations/usages ---'
rg -n -C 4 'getSleepingRecordForPane' src/renderer/src
printf '%s\n' '--- host-scope helpers ---'
sed -n '1,180p' src/renderer/src/lib/sleeping-record-execution-host-scope.ts
printf '%s\n' '--- relevant record/status declarations ---'
rg -n -C 5 'interface SleepingAgentSessionRecord|type SleepingAgentSessionRecord|agentStatusByPaneKey|interface AgentStatus|type AgentStatus' src sharedRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact method files ---'
rg -l --glob '*.ts' --glob '*.tsx' --glob '!*.test.ts' --glob '!*.test.tsx' 'getSleepingRecordForPane' src/renderer/src/components/terminal-pane
printf '%s\n' '--- exact method matches ---'
rg -n -C 8 --glob '*.ts' --glob '*.tsx' --glob '!*.test.ts' --glob '!*.test.tsx' 'getSleepingRecordForPane' src/renderer/src/components/terminal-pane
printf '%s\n' '--- host helper ---'
rg -n -C 10 --glob '*.ts' --glob '!*.test.ts' --glob '!*.test.tsx' 'function (sleepingRecordNamesAnotherExecutionHost|agentResumeOriginNamesAnotherExecutionHost)|const (sleepingRecordNamesAnotherExecutionHost|agentResumeOriginNamesAnotherExecutionHost)' src/renderer/src
printf '%s\n' '--- record/status type files ---'
rg -l --glob '*.ts' --glob '*.tsx' --glob '!*.test.ts' --glob '!*.test.tsx' 'SleepingAgentSessionRecord|AgentStatusEntry' src/renderer/src shared | head -30Repository: stablyai/orca
Length of output: 20830
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- sleeping-record-access.ts ---'
sed -n '1,95p' src/renderer/src/components/terminal-pane/pty-connection/sleeping-record-access.ts
printf '%s\n' '--- session identity declarations ---'
rg -n -C 5 'cacheKey:|cacheKey =' src/renderer/src/components/terminal-pane/pty-connection/connect-pane-pty-session.ts src/renderer/src/components/terminal-pane/pty-connection/*.ts
printf '%s\n' '--- status and record declarations ---'
rg -n --glob '*.ts' --glob '!*.test.ts' 'SleepingAgentSessionRecord|AgentStatusEntry' src shared 2>/dev/null | head -100
printf '%s\n' '--- relevant record writes ---'
rg -n -C 5 --glob '*.ts' --glob '!*.test.ts' 'sleepingAgentSessionsByPaneKey|agentStatusByPaneKey' src/renderer/src/lib src/renderer/src/store/slices | head -240Repository: stablyai/orca
Length of output: 39566
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- live-resume anchor logic ---'
sed -n '1,130p' src/renderer/src/lib/live-resume-anchor-record.ts
printf '%s\n' '--- shared type declarations ---'
rg -n -C 8 'export (interface|type) (SleepingAgentSessionRecord|AgentStatusEntry)|interface SleepingAgentSessionRecord|type SleepingAgentSessionRecord' shared
printf '%s\n' '--- cold restore candidates ---'
fd -i 'cold-restore|resume-startup' src tests | head -80
printf '%s\n' '--- identity checks near cold restore ---'
rg -n -C 6 --glob '*.ts' --glob '*.tsx' --glob '!*.test.ts' --glob '!*.test.tsx' 'isLiveResumeAnchorForCompletedAgent|agentProviderSessionsEqual\(.*providerSession|entry\.providerSession.*sleeping|sleepingRecord\.providerSession.*entry' src/renderer/src shared | head -200Repository: stablyai/orca
Length of output: 2251
Do not let an unrelated sleeping record veto the live session.
When useLiveEntry is true, the startup uses entry.agentType and entry.providerSession. getSleepingRecordForPane independently returns a record by pane key without matching those fields. The unconditional sleepingRecordNamesAnotherExecutionHost check can therefore return null for a valid live-entry resume when the sleeping record belongs to another agent or provider session.
Apply the sleeping-record host check only when the record matches the selected agent and provider session, as the launch-config selection already does.
| if (sleepingRecordNamesAnotherExecutionHost(record, currentState)) { | ||
| continue |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,65p' src/renderer/src/lib/sleeping-agent-pane-ownership.ts
sed -n '140,185p' src/renderer/src/store/slices/agent-status-provider-session.test.tsRepository: stablyai/orca
Length of output: 4117
Exclude foreign records from claim deduplication.
getProviderSessionClaimKey uses worktreeId, agent, provider-session fields, and selected transcript paths. It does not include connectionId or execution-host identity. Therefore, native and foreign records can share a claim key.
The claim maps are built before sleepingRecordNamesAnotherExecutionHost skips foreign records. If the foreign record is newer, the native record fails the later newest-record check and is cleared without being resumed. Build the claim maps from non-foreign records, while retaining foreign records in worktreeRecords so the loop leaves them uncleared.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor test-robustness note inline.
Reviewed changes — the full 6-file diff at fa7030a (2 commits).
- Host-scope predicate (
src/renderer/src/lib/sleeping-record-execution-host-scope.ts): newagentResumeOriginNamesAnotherExecutionHostcompares a record's raw SSH target id against the resolved execution host, failing open onundefined/empty/runtime:*/unparseable/unresolved host. Validated against every producer: SSH relay stamps the raw target id, the WSL relay'swsl:<distro>is normalized to null inagent-status-event-applicator.ts, and local/structured/runtime PTYs are null — no legitimate same-host resume is refused. - Sweep gate (
src/renderer/src/lib/resume-sleeping-agent-session.ts:271): a foreign record nowcontinues instead of launching, and instead of being cleared, so it stays resumable by hand. - Cold-restore gate (
src/renderer/src/components/terminal-pane/pty-connection/cold-restore-resume-startup.ts:54-62): the second--resumeissuer consults both the pane transport and the catalog, covering the four call sites that share the cached builder result. - Tests: a 213-line unit file pinning the predicate, the fail-open cases, and the sweep with a positive control; a Docker SSH e2e that replays the incident across a relay-kill restart and asserts on the argv ledger read from the container; plus the spec's registration in
run-ssh-docker-e2e.mjs.
The e2e is not theatre: the negative case asserts the ledger still holds Orca's --version probe, and the positive control requires the resume to land, so "resumes nothing" cannot masquerade as a pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
The worktree form of the resume gate resolved the current host through getExecutionHostIdForWorktree, which answers 'local' for a worktree the catalog has no row for. Read as a host, that made every SSH-stamped record look foreign until its repo row landed, contradicting the module's own contract that it reports only a positively-known disagreement. Add getKnownExecutionHostIdForWorktree, which returns null in that silence (no repo row for a git worktree, no folder-workspace row for a folder workspace), and route the gate through it; the pair form already fails open on a null host. The routing resolver keeps its default unchanged. The CI red on the control case was a separate spec race: the ledger wait returned as soon as the ledger was non-empty, and it already held the first launch's `--version` probe, so the control read two probes and gave up before the cold-restore had typed `--resume` (the failure screenshot shows the command running in the pane). The spec now reads only the lines the relaunch appended, anchors on the relaunch's PTY binding and its own probe, and then waits for `--resume` for the control case or a bounded grace for the refusal case.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the incremental delta at 766369e (1 commit since the prior pullfrog review at fa7030a).
- Catalog silence is distinguished from
'local'— newgetKnownExecutionHostIdForWorktreereturnsnullwhen no row names an owner (git worktree without a repo row, folder workspace without a folder-workspace row), andgetExecutionHostIdForWorktreenow delegates to it plus the focused-runtime/local fallback. The resume gate therefore fails open on the unhydrated-catalog window instead of reading the routing resolver's'local'default as a positive foreign-host verdict and refusing every SSH record until its repo row lands. - E2E read is tied to the resume decision —
ledgerLinesSincescopes the read to the post-quit ledger delta andsettleRemoteArgvLedgerpolls a predicate until satisfied or budget-exhausted, afterwaitForActivePanePtyIdconfirms the cold-restore decision has already been made. - Tests — added fail-open cases for the missing-catalog state (both SSH and local/runtime records) plus a positive refusal once a repo row names the worktree local, and pinned
getKnownExecutionHostIdForWorktreeagainst the routing resolver's'local'substitution.
The getExecutionHostIdForWorktree refactor is behavior-preserving for every caller: the folder branch's 'local' implies no focused runtime (so the ?? fallback also yields 'local'), and the no-repo-row branch already fell through to that same default. Verified locally: 58 unit tests across the two changed test files pass, pnpm tc:web exits 0, and the changed-lines code-quality gate reports 0 findings across 8 files. The prior inline note on the e2e wait helper is addressed and the thread resolved.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
stablyai#21288) * fix(session): scope agent resume to the host that captured the session A provider session id names a transcript in one machine's agent state directory. Nothing in the resume path compared that machine against the one the resume executes on, so a record captured on host A reached a `--resume` run on host B, which answers `No conversation found with session ID`. Three things make the drift reachable: `worktreeId` is `repoId::path` with no host component, sleeping records are `'sleepingAgentKeyed'` so boot-time host-contention parking never arbitrates them and every partition merges into one map without retaining provenance, and both issuers resolve their launch target from the current catalog. Both issuers are gated. The activation sweep hands `quit`/`live` records whose pane still exists to the pane's own cold restore, so gating the sweep alone changed nothing in the SSH lane. Declines rather than guesses: the record is preserved and remains resumable by hand. A refused resume is recoverable, a forked transcript is not. The predicate fails open on anything it cannot positively rule out -- an unstamped record, an empty stamp, or a `runtime:` host, which a paired client uses to relabel its host's own SSH workspaces. The cold-restore gate consults both the pane's transport and the catalog. The transport alone was racy: it is unresolved on an early reattach frame, and that frame is exactly when a wrong resume escaped. * docs(session): name the inverted fail-open direction at the resume gate * fix(session): keep an unresolved catalog out of the resume host verdict The worktree form of the resume gate resolved the current host through getExecutionHostIdForWorktree, which answers 'local' for a worktree the catalog has no row for. Read as a host, that made every SSH-stamped record look foreign until its repo row landed, contradicting the module's own contract that it reports only a positively-known disagreement. Add getKnownExecutionHostIdForWorktree, which returns null in that silence (no repo row for a git worktree, no folder-workspace row for a folder workspace), and route the gate through it; the pair form already fails open on a null host. The routing resolver keeps its default unchanged. The CI red on the control case was a separate spec race: the ledger wait returned as soon as the ledger was non-empty, and it already held the first launch's `--version` probe, so the control read two probes and gave up before the cold-restore had typed `--resume` (the failure screenshot shows the command running in the pane). The spec now reads only the lines the relaunch appended, anchors on the relaunch's PTY binding and its own probe, and then waits for `--resume` for the control case or a bounded grace for the refusal case.
1 participant
ELI5
A coding-agent session id names a transcript in one machine's state directory. Orca was replaying one against a machine that never wrote it, so the agent answered
No conversation found with session ID. This teaches the resume path to check which machine a record came from, and to decline rather than guess.The bug, reported by a user
SSH target, remote worktree, multiple panes. The user updated to a newer hourly, and on the next launch the agent auto-resumed with:
Two things were wrong at once: it resumed when it should not have, and the identifier was meaningless on that host.
Does
mainreproduce it?Yes. The new Docker SSH lane makes
mainat09622f0c289emit, on the remote machine:That argv is read out of the container's own filesystem, so it is not an inference about intent — it is the command that actually landed on a host that never wrote that transcript.
Root cause
Three things let a record drift across machines, and nothing compared them:
worktreeIdisrepoId::pathwith no host component.src/shared/worktree/host-qualified-identity.ts:20-25states outright that one repo registered on two hosts publishes the same id twice.sleepingAgentSessionsByPaneKeyis classified'sleepingAgentKeyed'(src/shared/workspace-session-host-field-ownership.ts:50) — the only class in that map that bypasses host-contention arbitration. Every partition's records merge at boot, last slice wins, provenance dropped.There are two issuers, and gating only the first changes nothing
This is the part that is not reconstructable from the diff. The activation sweep does not launch
quit/liverecords whose pane still exists — it deliberately hands them to the pane's own cold restore:with the contract stated on the type itself (
src/shared/agent-session-resume.ts:65-69): "only the pane's own cold-restore path may consume them."So the first version of this fix gated
resumeSleepingAgentSessionsForWorktreealone — and the e2e still failed, with the resume still reaching the remote.buildColdRestoreAgentResumeStartupis the issuer that actually fires. A reviewer looking at a one-issuer fix would reasonably assume it was sufficient; it is not.Gating the builder covers four call sites at once, because all of them consume one value computed once at
deferred-session-reattach-connect.ts:27: the three rungs of the reattach failure ladder (:110,:170,:182) pluswakeHibernatedAgentPane, which uses the builder as its default argument (deferred-cold-restore-and-snapshot.ts:69).Why #19882 did not already prevent this
Its release arm — a finished agent whose shell is still running releases its session and does not auto-resume — is gated on
stablePaneHasLivePty. After an app restart there is no live PTY, so by design it cannot apply. #19882 is not bypassed or regressed here; it simply does not cover the restart case.The fix
New
src/renderer/src/lib/sleeping-record-execution-host-scope.ts, consulted by both issuers.It declines; it never guesses. The record is preserved and stays resumable by hand. A refused resume is recoverable; a forked transcript is not.
It fails open on anything it cannot positively rule out — an unstamped record (
undefined, which #9030 leaves on SSH orphans), an empty stamp, or aruntime:host (a paired client relabels its host's own SSH workspaces into that namespace, so it is no evidence either way).The catalog consult is load-bearing, not belt-and-braces
The cold-restore gate checks both the pane's transport and the catalog. The transport alone was racy: two e2e runs on an identical bundle disagreed, because
session.executionHostIdis still unresolved on an early reattach frame — and that frame is exactly when a wrong resume escapes. With both sources the lane is deterministic over 3 repetitions. Please do not simplify this to one source.On "fail open"
workspace-session-terminal-buffers.ts:33-38resolves the same unhydrated-catalog window in the opposite direction, and both are correct: there the destructive act is declining to capture (the scrollback is the only copy), here it is attempting the resume. The second commit records that asymmetry at both ends so a future reader does not pattern-match one onto the other.Testing
FAIL/✘lines--dangerously-skip-permissions --resume <id>renderer/src/lib/+terminal-pane/pnpm tcORCA_CODE_QUALITY_BASE=origin/main check-changed-code-quality.mjsMutation-tested: 8 mutations applied to the predicate, zero survivors across all unit assertions, plus the e2e mutation above. Every assertion was shown to fail when the behaviour it claims to catch is reintroduced.
The positive control (
still resumes a session captured on the SSH host that owns the workspace) is what stops the negative test passing vacuously — without it, the suite would pass on an app that resumes nothing at all, which is precisely the failure mode a refuse-everything gate would ship. The negative test also asserts the ledger still contains Orca's ordinary--versionprobe, so "the agent never ran at all" cannot masquerade as a pass.The one unit failure is pre-existing on
main.terminal-pane-hook-order-parity.test.tsfails211 vs 209on a clean tree; I reverted my changes and confirmed it independently. It hashes onlyTerminalPane.tsxanduse-terminal-pane-*.ts, none of which this PR touches.Run it yourself:
The spec is registered in
config/scripts/run-ssh-docker-e2e.mjs, without whichpr-e2e-gate-contract.test.mjsfails (42 passed with it).Known-open, stated rather than buried
connectionIdon their record in thessh:<target>partition.connectionId: undefinedremain fail-open by design.resume-sleeping-agent-session.ts:297-311). It requires identical provider ids across hosts, so it is left alone.findUnhydratedHostMirrorForPaneis not implicated — this reproduces on a single-pane tab — and is untouched.Relationship to other work
resume-sleeping-agent-session.tshunk only widens cross-workspace suppression; it never compares a record's originating host to the executing host. Nothing from it is reused and it does not need reopening.sleepingAgentSessionsByPaneKey; different field, different ownership class, different mechanism. Either can land first.No data was destroyed
The resume failed before starting a session, and
--dangerously-skip-permissionsonly skips the permission prompt — it writes nothing. If the id was minted on another host, that transcript still exists there under that id. This is an identity bug, not data loss.