Repository navigation
Conversation
A relay answer of sourceRecovery.status === 'restoreRequired' is a SUCCESSFUL RPC that proves the PTY is alive - only its output delivery was retired. The spawn reattach path raised SSH_SESSION_EXPIRED for it, which is the one token that authorises replacing the pane's PTY: both spawn-execute sites then call markSshRemotePtyLease(..., 'expired') and deletePtyOwnership(), and the renderer cold-starts a fresh agent over the same worktree. A live remote agent was orphaned by a request that had just observed it running. Split the vocabulary so the two verdicts cannot be confused (docs/reference/ssh-execution-boundary.md), retry the attach first since a retired delivery is repairable, and give the token its own user-facing copy so it never inherits the "open a new terminal" advice. Second root cause, same symptom: the relay's delivery record was keyed on client id alone, so a re-bound primary channel matched a delivery bound to the dead sink and got 'existing' back - the pane repaints and never receives frames. Key it on the transport generation too, as the dispatcher's own publication ledger already does, and scope every release/retire path in activate() to a record the caller still owns.
|
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 (22)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change centralizes SSH PTY failure tokens and adds a dedicated restore-required token. Reattachment retries restore-required responses up to three times and rolls back provisional leases before retrying. Relay delivery records now track transport generations to reject stale transport ownership. Restore-required handling moves into shared relay helpers. Renderer error handling distinguishes expired sessions from live sessions that require output restoration. Tests cover retry limits, lease rollback, transport changes, and terminal messaging. Merge Risk: 🔵 Low · up to The PR prevents live SSH sessions from being mistaken for expired sessions and fences delivery ownership across reconnects. A bounded recovery risk remains because some rejection paths do not confirm remote delivery cancellation before retiring local state; the change is mergeable with explicit owner awareness and follow-up. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed ELI5, change summary, rationale, implementation details, sequencing guidance, and testing results. However, it does not provide the required Linked Issue entry using a Fixes reference, the required Visual Proof section or an explicit N/A statement, the Agent skill upstream boundary checkbox, or the checklist confirmations. Resolution Add a valid linked issue using the required Fixes # format. Add BEFORE and AFTER visual proof, or write N/A with a brief reason if no visual or interaction change applies. Complete the Agent skill upstream boundary and checklist sections, and confirm the required validation commands or state that CI will cover them. Full details: Docstring CoverageExplanation Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 21 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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.
ℹ️ One minor suggestion — the fix itself is sound, well-reasoned, and well-tested.
Reviewed changes — one commit reworking SSH PTY reattach failure signalling and relay delivery ownership around the ssh-execution-boundary live/unverifiable/exited vocabulary.
- Disjoint failure tokens — a new
src/shared/ssh-pty-failure-tokens.tsdefines threeincludes()-disjointSSH_*_ERRORtokens;ssh-pty-errors.tsre-exports them and the renderer consumers import them instead of re-declaring the string. - Correct reattach throw —
reattachSshPtySessionForSpawn(moved tossh-pty-session-reattach-for-spawn.ts) throwsSSH_SOURCE_RESTORE_REQUIREDrather thanSSH_SESSION_EXPIREDwhensourceRecoveryreportsrestoreRequired, so theSSH_SESSION_EXPIRED-matching consumers leave a live PTY alone. - Bounded restore retry —
reattachSshPtySessionretriespty.attachup to three times onrestoreRequired, advancing only when the relay retires the retired delivery. - Transport-generation ownership —
RequestContextgainstransportGenerationand delivery records store it;sameClientTransportkeys ownership on${clientId}:${generation}so a re-bound primary (setWrite()keeping its id) no longer gets a falseexisting. - Renderer toast —
TerminalErrorToastrenders the new token as a distinct "reconnecting / still running" line, excluded from the "open a new terminal" copy and added toisExplainedTerminalError.
Verified end to end: the disjoint tokens genuinely split the respawn-authorising includes(SSH_SESSION_EXPIRED) from the live-PTY token, the transport-generation keying correctly matches the dispatcher-capacity-signals ledger, and the renderer retry test has a real positive control so "did not respawn" is an actual assertion rather than a build where nothing respawns.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| // restoreRequired proves the PTY is live and only its delivery was retired, so retrying the | ||
| // attach is the repair. Cancel the provisional delivery first, or the retry races the lease | ||
| // it is meant to replace; if cancellation cannot be confirmed, stop rather than stack leases. | ||
| const staleDeliveryCanceled = attachResult.sourceActivationLease |
There was a problem hiding this comment.
This provisional-delivery rollback is unreachable in production: a restoreRequired response carries no sourceActivation (the relay returns only { status: 'restoreRequired', reason }), so requestSshPtyAttach never installs a lease and sourceActivationLease is always undefined here. The retry's safety rests entirely on the relay deleting the retired record in registerCanceledPtySourceRetirement's onResponseSettled; the comment two lines up claims the client cancels a provisional delivery it never holds.
| const staleDeliveryCanceled = attachResult.sourceActivationLease | |
| // restoreRequired proves the PTY is live and only its delivery was retired, so retrying the | |
| // attach is the repair. The relay already canceled and queued the retired delivery for | |
| // deletion on response-settle; the lease below is only present when a sourceActivation was | |
| // returned (never on restoreRequired), so it is a defensive guard, not the cancellation itself. | |
| const staleDeliveryCanceled = attachResult.sourceActivationLease | |
| ? await attachResult.sourceActivationLease.rollback() | |
| : true |
Duplicate implementation — #17951's stack already ships this fixFound while integrating branches into the combined adhoc: this PR and #18029 both fix the same defect, independently, with different token names and different retry counts. They conflict on six files.
The two differ:
Conflicting files: This is a semantic conflict, not a textual one. Merging them mechanically would produce a vocabulary that is half one scheme and half the other — exactly the failure mode this sweep has been cataloguing. What needs decidingOne of the two should carry the fix. Points in favour of each: This PR has the relay delivery-ownership fix applied at all three release/cancel sites in The #17951 stack version is older, already reviewed in place, and sits under four PRs that build on it — so replacing it means rebasing that stack. My recommendation: keep the #17951 stack's token as the vocabulary (it is upstream of more work), and port from here the three things it does not have — the three-site ownership guard, the toast pattern, and the render-time humanization. Then close this PR or reduce it to those hunks. This also revises the sequencing note in this PR's description. It said "build this first, before #17957 and #18013." That is wrong now: #17951 is below #17957, so the vocabulary already lands ahead of both. The real constraint is that this PR and #18029 must not both land. Flagging rather than resolving, since choosing which token survives affects four other PRs. |
|
Superseded by #18038. This PR built a parallel vocabulary for a fix the #17951 lease stack already carries — #18038 ports only what the stack genuinely lacked, in the stack's own vocabulary. Two findings from that port worth recording here: The ownership guard was missing entirely, not partially. I reported that this PR's version guarded one of three release/cancel sites. In fact Render-time humanization was already present, so that third item needed no code at all — Two things from here were deliberately not carried over, and remain available if wanted: The underlying defect this PR was built to fix — |
Salvaged from #17786 (see the review there). This is the one genuine root-cause win in that PR, extracted clean. Sequence this before #17957 and #18013 — it changes the token they gate on.
ELI5
A successful RPC that proved the process alive was authorising a cold restart over the same worktree.
Chain, verified end to end on
main:relay-pty-source-activation.ts:80returnsrestoreRequiredfor a live PTY (only its output delivery was retired) →reattachSshPtySessionForSpawnthrewSSH_SESSION_EXPIRED→ bothspawn-execute.tssites setisExpiredSshSession, callclearProviderPtyState,deletePtyOwnership(),markSshRemotePtyLease(…, 'expired')→ipc-pty-connect.ts:186returns{sessionExpired: true}→reattach-result-handler.ts:135callsstartFreshColdRestoreAgentResume(…, { forceBlankRestoredViewport: true }).That is the exact inversion
docs/reference/ssh-execution-boundary.mdexists to prevent, and it is reached on positive evidence of life, not loss of contact. No other PR in the sweep touchesssh-pty-session-reattach.ts.The fix
A new
src/shared/ssh-pty-failure-tokens.tswith three deliberately disjoint tokens — disjoint because every consumer matches withincludes(). Plus the throw-site change, the bounded 3-attempt retry, andsameClientTransport+transportGenerationonRequestContext.Because the tokens are disjoint,
'SSH_SOURCE_RESTORE_REQUIRED'.includes('SSH_SESSION_EXPIRED')isfalse, so bothspawn-execute.tssites andremote-runtime-pty-transport.ts:1603already do the right thing — no respawn. That is the point of the split, and it is why the "unreachable consumers" concern raised in review needed no code change there.The one real gap was the toast. It deliberately does not go in
UNREATTACHABLE_SESSION_SOURCES— that copy says "Open a new terminal", which here would abandon a running agent. It gets its own pattern, its own copy, and an entry inisExplainedTerminalErrorto suppress the issue link.The clear-matching regression #17786 had is gone by construction:
describeReattachFailurewas not taken at all. Humanizing at render inTerminalErrorToast— the existing canonical mechanism — means the store holds the raw string on both the append and clear paths, so they match.use-terminal-pane-title-state.tsis untouched and there is no compensation to drop.isProvenSshSessionGoneErrorwas deliberately left out. Once the tokens are disjoint it is behaviourally identical to main'sisSshSessionExpiredError. Its only added value is the identity-mismatch gate (that is #18013's change) and aPTY not foundbroadening that widens respawn authorisation.The second root cause — and #17786's fix for it does not work
Delivery ownership was keyed on client id alone, so a re-bound primary channel (
setWrite()→resetClient()bumpsgeneration, keeps the id) got'existing'back for a delivery bound to the dead sink. The pane repaints and then never receives frames.dispatcher-capacity-signals.ts:96already keys the publication ledger on${id}:${generation}; the delivery record was the outlier.#17786 guards one of the three release/cancel sites in
activate(). That was confirmed insufficient by experiment, not argument:AssertionError: expected true to be falseThe path actually taken is site B (
unadmitted/subscriber). I also rewrote the transport-generation test soisStale()staysfalseas production reports it — #17786's version hard-coded ittrue, which is what hid site B.User-facing regressions
None expected. The change narrows what authorises a respawn; it never widens it.
Testing
New/changed tests against main sources:
Test Files 5 failed (5) / Tests 10 failed | 41 passed (51). After:Test Files 6 passed (6) / Tests 62 passed (62). Full suites:src/relay/+src/main/providers/2364 passed;terminal-pane/4113 passed.pnpm tcclean,check:code-quality:changed0 findings, catalog synced (1 key).One honest limitation: the ported renderer retry test still passes on
main, because a renderer-only test cannot distinguish this — the change is in what the provider emits. A positive control was added (same scenario withSSH_SESSION_EXPIRED, assertingclearTabPtyIdis called) so "did not respawn" is a real result rather than a build where nothing respawns. The main→branch delta is proven by the provider tests.Sequencing
Build this first. #17957 changes
ipc-pty-connect.ts:186toisSshSessionGoneError(message); #18013 deletes the same localSSH_SESSION_EXPIRED_ERRORconst inremote-runtime-pty-transport.ts. This PR deletes both consts too, importing from the shared module — a textual conflict at exactly those import lines. Both PRs should then defineisSshSessionGoneErrorover the shared tokens and excludeSSH_SOURCE_RESTORE_REQUIRED, rather than re-deriving the vocabulary.