Repository navigation
fix(ssh): pick the eligible expired lease, not the first one matching a pane - #18366
Conversation
… a pane `getRecentExpiredSshLease` selected the first `expired` lease matching the pane coordinates and left eligibility to the caller. Only `recoverTerminalPane` asked, and it asks id-qualified, where lease identity `(targetId, ptyId)` already makes the match unique -- so that check could never fire on a lease a different one shadowed. The two unqualified callers never asked at all: `workspaceSessionWorktreeHasRuntimeOwnedPtyCandidate` and `hasRecentExpiredSshLeasePane` both take a bare `!== null`. `(worktreeId, tabId, leafId)` is not unique. `supersedeSiblingLeasesForPane` exists because a pane accumulates leases as it re-leases under new relay ids, and it stamps `supersededBy` on an already-expired predecessor precisely so the predecessor stops counting. Inside the 30s SSH_PANE_RECOVERY_GRACE_MS window those two readers still counted it: a pane whose only recent lease is a superseded or relay-id-recycled corpse was reported as runtime-owned and preserved for recovery, and `recoverTerminalPane` then refuses it. Where an eligible successor also exists, the predecessor is stored first and shadowed it. Apply the existing `sshRemotePtyLeaseAllowsReattach` inside the selection, so the reader answers with the first ELIGIBLE orphan or nothing, and all three callers agree on what `expired` authorizes. `recoverTerminalPane`'s own check becomes unreachable and is folded into the comment on the branch that now covers it. Scope: an over-report in headless/mobile reconciliation, not a wrong-route readoption -- the id-qualified recovery path already refused these leases. No wire change and no host-semantics change: `expired` still says only that the client lost its route, and nothing here asserts a remote shell died. Coverage lives in a `*.test.ts`: config/vitest.config.ts, the config CI runs, includes only `*.test.ts`, so the orca-runtime-tests/*.spec.ts neighbours would never execute.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughExpired SSH lease selection now excludes leases that cannot be reattached. Tests cover eligible orphan leases, superseded leases, relay-recycled leases, and predecessor ordering. Terminal recovery now treats the expired-lease path as directly non-recoverable. Merge Risk: ⚪ Minimal · up to Expired SSH lease reconciliation now ignores superseded and relay-recycled leases while selecting eligible orphan leases, preventing stale lease records from preserving panes as runtime-owned. The covered behavior change has no remaining merge-readiness risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed problem, fix, severity, compatibility, and testing context, but it does not follow the required template. It omits the required Linked Issue format, explicit Visual Proof response, checklist confirmations, Notes, Review, and Agent skill upstream boundary sections. Resolution Rewrite the description using all required template headings. Add a
✨ 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
getRecentExpiredSshLeasenow appliessshRemotePtyLeaseAllowsReattachinside its selection — the eligibility predicate is folded into the reader's.find, so superseded and relay-id-recycled leases are refused before they can shadow an eligible successor. All three callers (recoverTerminalPane,hasRecentExpiredSshLeasePane,workspaceSessionWorktreeHasRuntimeOwnedPtyCandidate) now agree on whatexpiredauthorizes.recoverTerminalPanedefers to the reader's eligibility — its post-hoc check and import are dropped, and the thrown-error comment is rewritten to cover the folded predicate. The behavior is unchanged for this path since lease identity(targetId, ptyId)is unique.- Adds the candidacy test — four cases pin the eligible-orphan selection and the two false positives it eliminates; three of them exercise the predicate that was previously misapplied.
I verified the underlying invariants the fix leans on: (targetId, ptyId) is the findIndex upsert key (ssh-pty-lease-operations.ts:140-142), and supersedeSiblingLeasesForPane marks an already-expired predecessor (leaving updatedAt untouched) while converting live siblings, so at most one eligible orphan exists per pane — array order in .find is therefore safe. The test file runs under config/vitest.config.ts (.test.ts, unlike the non-executed .spec.ts neighbours), and I confirmed 4 passed locally with lint clean. The change deletes no genuinely recoverable path — a superseded lease was always refused by recoverTerminalPane, so this only removes false positives.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
… a pane (stablyai#18366) `getRecentExpiredSshLease` selected the first `expired` lease matching the pane coordinates and left eligibility to the caller. Only `recoverTerminalPane` asked, and it asks id-qualified, where lease identity `(targetId, ptyId)` already makes the match unique -- so that check could never fire on a lease a different one shadowed. The two unqualified callers never asked at all: `workspaceSessionWorktreeHasRuntimeOwnedPtyCandidate` and `hasRecentExpiredSshLeasePane` both take a bare `!== null`. `(worktreeId, tabId, leafId)` is not unique. `supersedeSiblingLeasesForPane` exists because a pane accumulates leases as it re-leases under new relay ids, and it stamps `supersededBy` on an already-expired predecessor precisely so the predecessor stops counting. Inside the 30s SSH_PANE_RECOVERY_GRACE_MS window those two readers still counted it: a pane whose only recent lease is a superseded or relay-id-recycled corpse was reported as runtime-owned and preserved for recovery, and `recoverTerminalPane` then refuses it. Where an eligible successor also exists, the predecessor is stored first and shadowed it. Apply the existing `sshRemotePtyLeaseAllowsReattach` inside the selection, so the reader answers with the first ELIGIBLE orphan or nothing, and all three callers agree on what `expired` authorizes. `recoverTerminalPane`'s own check becomes unreachable and is folded into the comment on the branch that now covers it. Scope: an over-report in headless/mobile reconciliation, not a wrong-route readoption -- the id-qualified recovery path already refused these leases. No wire change and no host-semantics change: `expired` still says only that the client lost its route, and nothing here asserts a remote shell died. Coverage lives in a `*.test.ts`: config/vitest.config.ts, the config CI runs, includes only `*.test.ts`, so the orca-runtime-tests/*.spec.ts neighbours would never execute.
The ticket's stated mechanism does not reproduce
STA-6536 (F9, filed against #17969) says pane recovery picks the first lease matching an id rather than the first eligible one, and can therefore readopt a pane onto a superseded route. On the path it names —
recoverTerminalPane→getRecentExpiredSshLease(..., pty.ptyId)— that cannot happen:(targetId, ptyId), enforced by thefindIndexupsert key atsrc/main/persistence/leasing-ssh-ptys/ssh-pty-lease-operations.ts:140-142, so at most one lease matches per target.toComparableRelaySshPtyIdreturns the app-form id unchanged when it names a different target (src/shared/ssh-pty-id.ts:74-80), so a lease on another host can never match either.With one candidate, "first match" and "eligible match" are the same lease, and the post-hoc
sshRemotePtyLeaseAllowsReattachcheck #17969 added could never be shadowed. No wrong-route readoption was reachable.What is actually wrong
The same reader has two other callers that pass no
ptyId:workspaceSessionWorktreeHasRuntimeOwnedPtyCandidate(orca-runtime-reconcile-headless-mobile-session-browser-tabs.ts:93)hasRecentExpiredSshLeasePane(:166), consumed byhasLiveOrPersistedServeOrSshOwnedPtyBindingand by theonlyRuntimeOwnedTerminalshydrate filterBoth take a bare
!== null, and neither applies the eligibility predicate — before or after.(worktreeId, tabId, leafId)is not unique:supersedeSiblingLeasesForPaneexists because a pane accumulates leases as it re-leases under new relay ids, and it stampssupersededByon an already-expired predecessor precisely so that predecessor stops counting.So inside the 30s
SSH_PANE_RECOVERY_GRACE_MSwindow, a pane whose only recent lease is a superseded or relay-id-recycled corpse was reported as runtime-owned and preserved for recovery — a recoveryrecoverTerminalPanethen refuses. Where an eligible successor also exists, the predecessor is stored first and shadowed it.Fix
Apply the existing
sshRemotePtyLeaseAllowsReattachinside the selection, matching the shape the twossh-relay-session.tssites already use (:2038predicate-in-find,:2330.filter). The reader now answers with the first eligible orphan or nothing, and all three callers agree on whatexpiredauthorizes.recoverTerminalPane's own check becomes unreachable and is folded into the comment on the branch that now covers it.No new predicate, no new resolver.
Severity
An over-report in headless/mobile reconciliation, not a wrong-route readoption — the id-qualified recovery path already refused these leases. The ticket currently reads as much worse than it is.
Tests
src/main/runtime/expired-ssh-lease-pane-candidacy.test.ts— 3 of 4 fail onmain, all pass here; the 4th is the plain-orphan control that must keep passing both ways:In a
*.test.tsdeliberately:config/vitest.config.ts— the config CI runs — includes only*.test.ts, so theorca-runtime-tests/*.spec.tsneighbours never execute.Compatibility
No wire change and no host-semantics change.
expiredstill says only that the client lost its route; nothing here asserts a remote shell died.Verification against #18013
#18013 merged during this triage and does not close it: it moved
getRecentExpiredSshLeaseand changed it to resolve the leaf's current tab viafindCurrentTerminalTabIdForLeafinstead of the lease's frozentabId, but added no predicate to the.find. This is based on720c3299ba7.