Repository navigation
fix(ssh): stop respawning an agent onto a PTY the relay just proved alive - #17951
Conversation
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (12)
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 (1)
🚧 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; 7 remain after this review. 📝 WalkthroughWalkthroughThe provider now retries live PTY source restoration twice, rolls back pending activation leases between attempts, and reports a dedicated restore-required error after failure. Renderer code now uses Merge Risk: ⚪ Minimal · up to The change prevents live PTYs involved in identity mismatches from triggering an unnecessary cold restore; no actionable merge-blocking risk remains beyond normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides detailed context, explains the failure and fix, links the relevant issues, documents testing, and identifies out-of-scope findings. It omits the repository template headings, checklist confirmations, and explicit visual-proof or N/A entry, but the substantive content is mostly complete. Full details: Linked Issues checkExplanation The changes address Full details: Out of Scope Changes checkExplanation The production changes and tests remain focused on SSH PTY source restoration, reattachment error classification, identity mismatch handling, and duplicate agent prevention. The discussion of other potential gates does not introduce unrelated code changes. ✨ 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: Team
Run ID: be84e965-a75f-4ad0-b7aa-88d87270d19d
📒 Files selected for processing (11)
src/main/providers/ssh-pty-errors.tssrc/main/providers/ssh-pty-live-source-restore-respawn.test.tssrc/main/providers/ssh-pty-provider-reattach-incarnation.test.tssrc/main/providers/ssh-pty-relay-absence-verdict.test.tssrc/main/providers/ssh-pty-session-reattach.tssrc/renderer/src/components/terminal-pane/ipc-pty-connect.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-attach.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.tssrc/renderer/src/components/terminal-pane/pty-connection/pty-connect-limits.tssrc/renderer/src/components/terminal-pane/pty-connection/ssh-session-connect.tssrc/renderer/src/components/terminal-pane/pty-connection/ssh-session-gone-verdict.test.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/components/terminal-pane/pty-connection/ssh-session-connect.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| const message = error instanceof Error ? error.message : String(error) | ||
| return ( | ||
| message.includes(SSH_SESSION_EXPIRED_ERROR) && | ||
| !message.includes(SSH_PTY_IDENTITY_MISMATCH_ERROR) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Exclude textual identity-mismatch errors.
Line 22 excludes only SSH_PTY_IDENTITY_MISMATCH. Main also accepts /identity mismatch/i as an identity mismatch. An IPC-wrapped error that contains SSH_SESSION_EXPIRED and textual identity mismatch, but not this marker, returns true here. The renderer then marks the live PTY expired and can start a second cold restore.
Match the main-process verdict semantics and add a test for the textual variant.
Proposed fix
- !message.includes(SSH_PTY_IDENTITY_MISMATCH_ERROR)
+ !message.includes(SSH_PTY_IDENTITY_MISMATCH_ERROR) &&
+ !/identity mismatch/i.test(message)📝 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.
| !message.includes(SSH_PTY_IDENTITY_MISMATCH_ERROR) | |
| !message.includes(SSH_PTY_IDENTITY_MISMATCH_ERROR) && | |
| !/identity mismatch/i.test(message) |
![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 observation, nothing blocking.
Reviewed changes
- New
SSH_PTY_SOURCE_RESTORE_REQUIREDverdict —restoreRequiredno longer mintsSSH_SESSION_EXPIRED;reattachSshPtySessionForSpawnretries once (rolling back the source-activation lease when its cancellation is confirmed) and only then throws a token that claims nothing about PTY absence. - Renderer gate narrowed to host-attested absence —
isSshSessionGoneErrorreplaces the duplicatedincludes('SSH_SESSION_EXPIRED')tests and excludes theSSH_PTY_IDENTITY_MISMATCHsuffix, so an id naming another pane's live PTY no longer triggers a cold-restore respawn. - Tests — two new suites (respawn retry, renderer verdict) plus two retargeted tests pin the new contract with exact assertions.
The fix is well-reasoned and the relay-side premise checks out: pty-handler.ts attach() re-verifies isProcessAlive before any restoreRequired can be produced, and relay-pty-source-publication.ts retires the stale delivery as it answers, so attempt 2 reopens a fresh one with full replay. I also confirmed the lease && !(await lease.rollback()) branch is reachable in production — a plain spawn reattach can hit the activate() branch that answers restoreRequired alongside a sourceActivation when a checkpoint recovery is mid-flight — so that guard and its test are not dead code.
ℹ️ The new restore-required error surfaces to the user as an unhumanized token
When the retry still lands on restoreRequired (checkpoint genuinely unavailable), the thrown SSH_PTY_SOURCE_RESTORE_REQUIRED: <relayId> <reason> reaches the renderer's onError/reportError path and is rendered by TerminalErrorToast. The toast's humanizer (UNREATTACHABLE_SESSION_SOURCES) only knows SSH_SESSION_EXPIRED and PTY "..." not found, so this new token is displayed raw with a "file an issue" link instead of the friendly copy the expired-session path gets. Before this change that same failure was an SSH_SESSION_EXPIRED message the humanizer already covered, so this is a small UX regression on an otherwise-strict improvement.
Technical details
# Humanize the source-restore token in the terminal error toast
## Affected sites
- src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx — `UNREATTACHABLE_SESSION_SOURCES` has no entry for `SSH_PTY_SOURCE_RESTORE_REQUIRED`, so `isExplainedTerminalError` returns false and the raw token renders with a "file an issue" link.
## Required outcome
- A user-facing `SSH_PTY_SOURCE_RESTORE_REQUIRED: ...` failure shows copy equivalent to the other "couldn't reattach to this pane's session" verdicts rather than the raw internal token.
## Suggested approach (optional)
- Add `SSH_PTY_SOURCE_RESTORE_REQUIRED:[ \t]*\S*` (or a dedicated pattern) to `UNREATTACHABLE_SESSION_SOURCES` in `TerminalErrorToast.tsx` and extend the corresponding test in `TerminalErrorToast.test.ts`.DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
A `pty.attach` reply carrying `sourceRecovery: restoreRequired` is the relay answering for a PTY it just found in its pool and proved alive with `isProcessAlive`; only the stale output delivery was retired. Main converted that into `SSH_SESSION_EXPIRED`, which is the token every caller uses to retire the pane binding and cold-restore the agent, so a transient reconnect started a second `claude --resume` over a running one's transcript and left the previous remote PTY detached — one more per reconnect until the host refused to fork. Retry the attach once (the relay retires the stale delivery as it answers, so the next attach opens a fresh one with full replay), then fail with a restore-required verdict that makes no claim about absence. Callers already route anything short of absence to the unverifiable pane-recovery path. Also tighten the renderer's expiry verdict, which was a bare substring test: an identity mismatch names a LIVE PTY owned by another pane and observes nothing about this one, and main's own gate already refuses to respawn on it. Refs #11006, #9034
The refusal now carries SSH_PTY_SOURCE_RESTORE_REQUIRED, so the ratchet asserts the discriminating token instead of the one it no longer shares.
89b32e0 to
5f259a7
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Retargeted the
restoreRequiredrefusal test —ssh-pty-reattach-absence-discrimination.test.tsnow asserts the discriminatingSSH_PTY_SOURCE_RESTORE_REQUIREDtoken (toContain) and its absence ofSSH_SESSION_EXPIRED(not.toContain), replacing the assertion that pinned the expiry text the code no longer emits on that path.
The retarget is correct against the production code: reattachSshPtySessionForSpawn throws SSH_PTY_SOURCE_RESTORE_REQUIRED: <relayId> <reason> after exhausting the retry, and the fixture's stable attach reply drives both attempts without a source-activation lease, so the not.toContain(SSH_SESSION_EXPIRED_ERROR) assertion would fail if a future change re-wore the expiry token here. The four main-side suites pass locally (14 tests).
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…live (stablyai#17951) * fix(ssh): stop reporting live relay PTYs as expired sessions A `pty.attach` reply carrying `sourceRecovery: restoreRequired` is the relay answering for a PTY it just found in its pool and proved alive with `isProcessAlive`; only the stale output delivery was retired. Main converted that into `SSH_SESSION_EXPIRED`, which is the token every caller uses to retire the pane binding and cold-restore the agent, so a transient reconnect started a second `claude --resume` over a running one's transcript and left the previous remote PTY detached — one more per reconnect until the host refused to fork. Retry the attach once (the relay retires the stale delivery as it answers, so the next attach opens a fresh one with full replay), then fail with a restore-required verdict that makes no claim about absence. Callers already route anything short of absence to the unverifiable pane-recovery path. Also tighten the renderer's expiry verdict, which was a bare substring test: an identity mismatch names a LIVE PTY owned by another pane and observes nothing about this one, and main's own gate already refuses to respawn on it. Refs stablyai#11006, stablyai#9034 * fix(lint): merge the duplicate pty-connect-limits import * test(ssh): stop pinning the expiry token on a restoreRequired refusal The refusal now carries SSH_PTY_SOURCE_RESTORE_REQUIRED, so the ratchet asserts the discriminating token instead of the one it no longer shares. --------- Co-authored-by: Neil <neil@example.com>
Corruption-class: two
claude --resumeprocesses writing one transcript file.#11006 — the gate was never host-attested, and #12448's fix could not reach it
#12448 hardened only the main-side gate (
isSshPtyAbsentFromRelayError→isPtyAlreadyGoneError→spawnForStablePane). It deliberately kept the error message byte-identical — and the renderer's respawn gate reads the message, not the class.The chain:
relay-pty-source-publication.ts:117-127answersrestoreRequired— but only afterpty-handler.ts:1006-1015found the managed PTY and confirmedisProcessAlive(managed.pty.pid). That reply is positive host evidence of liveness.ssh-pty-session-reattach.ts:288-292converted it toError("SSH_SESSION_EXPIRED: <relayId>").spawn-execute.ts:143-172matched the substring → cleared provider state, deleted ownership, marked the leaseexpired.ipc-pty-connect.ts:186→{ sessionExpired: true }.startFreshColdRestoreAgentResume→ a fresh PTY plus the agent resume command. Twoclaude --resumeon one transcript.A second gate of the same class:
reattachSshPtySession:231emits that same token for an identity mismatch — whichssh-pty-errors.ts:14-24itself documents as "the id names a live PTY belonging to another pane". Main refuses to respawn on it; the renderer's.includes('SSH_SESSION_EXPIRED')did not.Fixed at the gate, not with a downstream dedupe.
restoreRequirednow retries once (the relay retires the stale delivery as it answers, so attempt 2 opens a fresh one with full replay) and then throwsSSH_PTY_SOURCE_RESTORE_REQUIRED, which claims nothing about absence — existing callers already route non-absence to a remount-and-reattach with no shell restart. The renderer's verdict is nowisSshSessionGoneError, which excludes the identity-mismatch suffix, replacing a duplicated substring test in two files.#9034 — the same bug's resource consequence, fixed by the same change
Not separate. The
sessionExpiredbranch drops the client binding and marks the leaseexpiredbut never kills the remote PTY — correctly, since there is no evidence it should. So every reconnect hittingrestoreRequiredleft one more live detached PTY on the host and spawned a replacement, untilposix_spawnp failed. Fixing the gate removes the source. No dedupe added.#12447 — not reproducible, verified by mutation rather than by reading
I disabled the
isRetiredReattachedPtySurfaceguard and droppedmayReviveRetiredSurface: falsefrom the reattach bind, then ran the suites:Both partitions and both relay generations are pinned, with negative controls ("reattaches a moved pane despite a … tombstone") so the guard cannot over-fire.
persistPtyBindingrefuses before any mutation — confirmed, the only code above it is pure reads. Guards restored; nothing landed.But the
claude --resume-on-weeks-old-sessions half of #12447 did reach production, through #11006'ssessionExpiredbranch, and is closed by this commit.Why #12448's regression net did not catch this
Reverting its classification does fail two tests — so the class is guarded. But the message it deliberately left unchanged is what #11006 travelled on, and
stable-pane-relay-absence-respawn.test.tskept passing under the revert because it injects errors directly and never sees the message path.Two existing tests pinned the buggy contract and were retargeted, with reasoning in-comment.
Three weaker-than-host-attested respawn gates found and NOT touched
Each deserves its own change; recording them so they are not lost:
ipc-pty-connect.ts:187—SSH_PTY_CONNECTION_MISMATCH_MARKERis thrown purely client-side byssh-pty-id.ts:51,64and still mintssessionExpired: true→ respawn + resume. No host in the loop at all.orca-runtime-resolve-terminal-pane.ts:73-97— accepts!pty.connected(loss of contact) plus anexpiredlease within a 30s grace as authority tocreateTerminal, i.e. the host spawns a new remote shell. Several writers of thatexpiredstate are themselves client-side.direct-ssh-pane-retry-ledger.ts:77-96—if (!args.tabPtyId) return true: absence from a client-side map arms a generation bump that lands on the fresh-spawn path.Verification
One pre-existing unrelated failure (
run-electron-vite-dev.test.ts, mac dev-app symlink copy), confirmed failing identically on a clean stash.Fixes #11006, #9034
Refs #12447, #12448