Repository navigation
fix(resource-manager): never destroy a session Orca cannot prove is idle (#8459) - #10893
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between f8726b2e2e03653415333a4f7634ad38fe43e0a3 and 53a1db4. 📒 Files selected for processing (20)
🚧 Files skipped from review as they are similar to previous changes (19)
📝 WalkthroughWalkthroughPTY session listings now use a shared 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/src/components/status-bar/resource-session-bindings.ts (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense repeated ownership-policy comments.
Keep the invariant, but remove repeated incident history and implementation walkthroughs.
src/renderer/src/components/status-bar/resource-session-bindings.ts#L8-L14: reduce to a concise description of known-live deferred SSH bindings.src/renderer/src/components/status-bar/resource-session-bindings.ts#L87-L102: retain the selector’s safety invariant in one short comment.src/renderer/src/components/status-bar/resource-session-kill-confirmation.ts#L3-L10: retain only the confirmation rule and reason.As per coding guidelines, comments must be concise, limited to non-obvious information, and preferably one line.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dd1bb0ad-a65f-4cae-b534-7ba88cb67e39
📒 Files selected for processing (19)
src/main/ipc/pty.test.tssrc/main/ipc/pty.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/components/status-bar/ResourceUsageStatusSegment.rows.test.tsxsrc/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsxsrc/renderer/src/components/status-bar/mergeSnapshotAndSessions.test.tssrc/renderer/src/components/status-bar/mergeSnapshotAndSessions.tssrc/renderer/src/components/status-bar/resource-session-bindings.test.tssrc/renderer/src/components/status-bar/resource-session-bindings.tssrc/renderer/src/components/status-bar/resource-session-inventory.test.tssrc/renderer/src/components/status-bar/resource-session-kill-confirmation.test.tssrc/renderer/src/components/status-bar/resource-session-kill-confirmation.tssrc/renderer/src/components/status-bar/resource-usage-merge-types.tssrc/renderer/src/components/status-bar/resource-usage-open-slices.test.tssrc/renderer/src/components/status-bar/resource-usage-open-slices.tssrc/renderer/src/components/status-bar/use-resource-session-inventory.test.tsxsrc/shared/pty-listed-session.tstests/e2e/resource-manager-unbound-session-safety.spec.ts
…dle (#8459) Resource Manager decided a session was an "orphan" from the absence of a renderer binding, then force-killed it with no prompt. Absence of a binding is not evidence a session is idle — during restore the binding map is legitimately empty, and deferred SSH sessions never appear in it at all. Live agent sessions were destroyed this way, losing unrecoverable work. Three gaps, one rule: only positive evidence authorizes destruction. - `pty:listSessions` dropped `agentSessionOwners` at the IPC boundary, so the renderer could not see the one fact that proves work is running. It now reports `hasAgentOwner`, typed once in `shared/pty-listed-session.ts` so the main handler, both preload surfaces, and the renderer cannot drift. - The binding index ignored `deferredSshSessionIdsByTabId` — sessions restore knows are live on an SSH host but has not reattached. No other binding source can see them. - The bulk-kill handler filtered sessions separately from the button's count, so the set killed could differ from the set advertised. Both now call `selectUnboundDaemonSessions`. The single-row kill path had the same defect: it skipped confirmation whenever `bound` was false. `requiresKillConfirmation` now also holds for agent-owned sessions, and snapshot-derived rows carry ownership across from the daemon list rather than reporting `false`.
…bsence Adversarial review of the previous commit found it committed the same class of error it was fixing: it collapsed "no agent owns this" and "this provider cannot tell me" into one boolean `false`, and both destructive paths read that as proof. A daemon generation below the claim protocol, an older SSH relay, or the in-process local fallback all list no owners for a session that may well have one. `pty.ts` already encodes the rule at :613 — "only providers that serialize claims may make listing absence authoritative" — and the new IPC row ignored it. So after upgrading with a legacy daemon still holding a live agent terminal, bulk cleanup would have destroyed it: exactly #8459, one layer down. `hasAgentOwner: boolean` is now `agentOwnership: 'present' | 'absent' | 'unknown'`, derived via `providesAgentSessionOwnerListings`. Only `absent` authorizes destruction, so `unknown` protects and confirms. Second defect, found independently by four review lenses: the deferred-SSH bindings reached the bulk selector but not `mergeSnapshotAndSessions`, because the merge call site re-listed the binding fields instead of reusing the object. A deferred SSH session therefore rendered `bound: false`, and its single-row kill skipped confirmation while bulk cleanup correctly spared it. The call site now spreads `resourceSessionBindings`, and a parity test fails if any binding field is re-listed inline — the drift itself is now impossible to reintroduce quietly. The e2e ownership assertion was also weak: it checked only that a boolean arrived. It now asserts the exact arm, and that the live local provider reports `absent` rather than `unknown`, so a degenerate all-unknown implementation fails.
OrcaWin
force-pushed
the
session-liveness-unbound-safety
branch
from
July 27, 2026 18:34
f8726b2 to
53a1db4
Compare
8 of 9 tasks
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
…dle (stablyai#8459) (stablyai#10893) * fix(resource-manager): never destroy a session Orca cannot prove is idle (stablyai#8459) Resource Manager decided a session was an "orphan" from the absence of a renderer binding, then force-killed it with no prompt. Absence of a binding is not evidence a session is idle — during restore the binding map is legitimately empty, and deferred SSH sessions never appear in it at all. Live agent sessions were destroyed this way, losing unrecoverable work. Three gaps, one rule: only positive evidence authorizes destruction. - `pty:listSessions` dropped `agentSessionOwners` at the IPC boundary, so the renderer could not see the one fact that proves work is running. It now reports `hasAgentOwner`, typed once in `shared/pty-listed-session.ts` so the main handler, both preload surfaces, and the renderer cannot drift. - The binding index ignored `deferredSshSessionIdsByTabId` — sessions restore knows are live on an SSH host but has not reattached. No other binding source can see them. - The bulk-kill handler filtered sessions separately from the button's count, so the set killed could differ from the set advertised. Both now call `selectUnboundDaemonSessions`. The single-row kill path had the same defect: it skipped confirmation whenever `bound` was false. `requiresKillConfirmation` now also holds for agent-owned sessions, and snapshot-derived rows carry ownership across from the daemon list rather than reporting `false`. * fix(resource-manager): distinguish unprovable ownership from proven absence Adversarial review of the previous commit found it committed the same class of error it was fixing: it collapsed "no agent owns this" and "this provider cannot tell me" into one boolean `false`, and both destructive paths read that as proof. A daemon generation below the claim protocol, an older SSH relay, or the in-process local fallback all list no owners for a session that may well have one. `pty.ts` already encodes the rule at :613 — "only providers that serialize claims may make listing absence authoritative" — and the new IPC row ignored it. So after upgrading with a legacy daemon still holding a live agent terminal, bulk cleanup would have destroyed it: exactly stablyai#8459, one layer down. `hasAgentOwner: boolean` is now `agentOwnership: 'present' | 'absent' | 'unknown'`, derived via `providesAgentSessionOwnerListings`. Only `absent` authorizes destruction, so `unknown` protects and confirms. Second defect, found independently by four review lenses: the deferred-SSH bindings reached the bulk selector but not `mergeSnapshotAndSessions`, because the merge call site re-listed the binding fields instead of reusing the object. A deferred SSH session therefore rendered `bound: false`, and its single-row kill skipped confirmation while bulk cleanup correctly spared it. The call site now spreads `resourceSessionBindings`, and a parity test fails if any binding field is re-listed inline — the drift itself is now impossible to reintroduce quietly. The e2e ownership assertion was also weak: it checked only that a boolean arrived. It now asserts the exact arm, and that the live local provider reports `absent` rather than `unknown`, so a degenerate all-unknown implementation fails. --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8459.
What was happening
Resource Manager's "Kill orphan terminals" decided a session was an orphan from the absence of a renderer binding, then force-killed it with no prompt.
Absence of a binding is not evidence a session is idle. Two cases where it is routinely wrong:
workspaceSessionReadyis alreadytrue.deferredSshSessionIdsByTabIdand skips reattach while the relay is disconnected.Both describe sessions that are alive. Live agent sessions were destroyed this way, and an agent's work is not recoverable.
The rule
Only positive evidence authorizes destruction. Three gaps closed:
1. Ownership was discarded at the IPC boundary.
PtyProcessInfocarriesagentSessionOwners, butpty:listSessionsnarrowed each row to{id, cwd, title}— so the renderer never saw the one fact that proves work is running. It now reportshasAgentOwner, typed once insrc/shared/pty-listed-session.tsso the main handler, both preload surfaces, and the renderer can't drift.2. Deferred SSH sessions weren't in the binding index.
buildResourceSessionBindingIndexconsulted live PTYs, tab wake hints, and layouts — but notdeferredSshSessionIdsByTabId. Sessions whose tab no longer exists are still ignored, so a stranded id can't read as liveness.3. The count and the kill set were computed separately. The button rendered
countUnboundDaemonSessions(...)while the handler ran its ownsessions.filter(...). Two expressions that must agree but nothing made them. Both now callselectUnboundDaemonSessions, and a test asserts they stay identical.The single-row path had the same defect.
handleKillSessionskipped confirmation wheneverboundwas false — the same unproven classification.requiresKillConfirmationnow also holds for agent-owned sessions, and snapshot-derived rows carry ownership across from the daemon list rather than defaulting tofalse.Verification
Every guard mutation-tested — reverted individually, confirmed the mutation applied, confirmed specific tests fail:
hasAgentOwneralwaysfalseat IPChasAgentOwnershort-circuit removed from selectorrequiresKillConfirmation→session.boundonlyhasAgentOwner: falseE2E through a real quit/relaunch —
tests/e2e/resource-manager-unbound-session-safety.spec.tsstarts a real terminal, quits the app so the detached daemon keeps owning the PTY (the #8459 precondition), relaunches, and asserts the live session is bound once restore reports ready.That spec was itself mutation-tested: removing
hasAgentOwnerfrom the IPC payload and rebuilding makes it fail withpty:listSessions dropped hasAgentOwner across the real IPC boundary. It fails for the right reason, not vacuously.Suites: 603 tests across status-bar +
pty.test.ts; 5,871 across status-bar, pty IPC, store, and shared.pnpm run typecheckclean across all three projects.oxlintexits 0 with no findings in touched files. max-lines ratchet, reliability gates, and locale parity all pass.resource-usage-warm-reattach.spec.tsstill passes.Scope
No new user-facing strings, so no locale changes. No UI layout or styling changes — the only visible difference is that agent-owned and deferred-SSH sessions stop being counted as orphans, and killing one now asks first. Cross-platform: no platform-specific code paths touched. SSH is the primary case this fixes.