From 7cd7fef927fcec94272457acff55d28db256c1f2 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 9 Aug 2026 02:30:28 -0700 Subject: [PATCH] fix(terminal): a reattach not-found is not proof the shell is gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last live route to the reported duplicate agent resume (RC2), found by the E2E harness rather than by reading: when a relay is stalled and replaced, the fresh relay has no memory of `pty-1` while the old shells keep running under its predecessor. It answers not-found, and the renderer read that as proof. A not-found means the relay WE ASKED cannot hand that id back. That proves an exit only if the relay process that minted the pty is the one answering. The design says exactly this (D3 row 2), and gates the grant on `relayInstanceId` equality — a field step E-2 never built. `SSH_SESSION_EXPIRED` is not independent evidence either: its ONLY producer is that same not-found mapping in reattachSshPtySession, and the token's own doc comment claimed "the host proved the session is gone", which it never did. So `isProvenSshSessionGoneError` returns false. Both reattach arms now always take the non-proof path and surface the pane as disconnected, which is what the owner approved in D1 — and what makes that affordance load-bearing rather than near-unreachable, since it was previously only reached by errors that were already rare. The respawn tails are deliberately NOT deleted. The design preserves the grant as a conditional for E-2, so the decision point stays and a clause pins that nothing reaches it meanwhile. This is the one place in the program where an unreachable branch is kept on purpose, and it is labelled as such. Tests: three clauses asserted a not-found proves death and are INVERTED, with the reasoning recorded. The #12101 cold-restore case reached the spawn door by throwing a not-found; that door is now opened by the user's "Start a new terminal", so the test drives that instead and keeps all four of its original assertions verbatim — strictly better coverage, since it now pins that the automatic respawn stopped AND that the door still works. Mutation proof: restoring the old predicate makes the new no-respawn clause fail with "expected connect to be called 1 times, but got 2", confirming it reddens on the real production route rather than passing vacuously. --- .../terminal-pane/pty-connection.test.ts | 78 +++++++++++++++++-- .../reattach-failure-classification.test.ts | 50 ++++++++++-- .../reattach-failure-classification.ts | 36 ++++----- 3 files changed, 129 insertions(+), 35 deletions(-) diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index 1b6ff5b4009..84c165343b3 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -598,6 +598,12 @@ function createManager(paneCount = 1, initialActivePaneId: number | null = null) } } +// Structural subset of PtyTransportRecoveryState: enough to pin the disconnected/unreachable pane. +type RecoveryStateProbe = { + phase: string + unreachablePane?: { onRetry: () => void; onStartNewTerminal: () => void } +} | null + function createDeps(overrides: Record = {}) { return { tabId: 'tab-1', @@ -9223,14 +9229,18 @@ describe('connectPanePty', () => { // Why: main can answer a *spawn* with an adopted session, so the reattach handler // is reachable by a second door that skips the restored-session path entirely. The // cold-restore signal has to survive that door too, or #12101 returns on it. + // + // Vehicle changed for STA-3077: reaching that door used to be automatic — a restore that threw + // `PTY "tab-pty" not found` counted as proof the session was gone and respawned itself. No + // reattach failure proves that any more (a replaced relay reports not-found for shells still + // running under its predecessor), so the pane surfaces as disconnected and the *user* opens the + // same door via "Start a new terminal". The #12101 assertions below are unchanged. const { connectPanePty } = await import('./pty-connection') const transport = createMockTransport('tab-pty') - let activePtyId = 'tab-pty' + let activePtyId: string | null = 'tab-pty' transport.getPtyId.mockImplementation(() => activePtyId) transport.connect.mockImplementation(async ({ sessionId }: { sessionId?: string }) => { if (sessionId) { - // Proof the session is gone — only that may reach the spawn door. A bare - // fault is unresolved and deliberately does not respawn. throw new Error('PTY "tab-pty" not found') } // Main answered the spawn by adopting a durable session instead. @@ -9253,16 +9263,32 @@ describe('connectPanePty', () => { configureTerminalFocusMode(pane, textarea) await withMockedDocumentActiveElement(textarea, async () => { const manager = createManager(1) + const onPtyRecoveryState = vi.fn<(paneId: number, state: RecoveryStateProbe) => void>() const deps = createDeps({ restoredLeafId: LEAF_1, - restoredPtyIdByLeafId: { [LEAF_1]: 'tab-pty' } + restoredPtyIdByLeafId: { [LEAF_1]: 'tab-pty' }, + onPtyRecoveryStateRef: { current: onPtyRecoveryState } }) connectPanePty(pane as never, manager as never, deps as never) await flushAsyncTicks(30) - expect(transport.connect).toHaveBeenCalledTimes(2) + // The unproven failure parks the pane instead of respawning it. + expect(transport.connect).toHaveBeenCalledTimes(1) expect(transport.connect.mock.calls[0]?.[0]?.sessionId).toBe('tab-pty') + const unreachable = onPtyRecoveryState.mock.calls.find( + ([paneId, state]) => paneId === pane.id && state?.phase === 'disconnected' + )?.[1]?.unreachablePane + expect(unreachable).toBeDefined() + expect(deps.clearTabPtyId).not.toHaveBeenCalled() + + // The user opens the spawn door; main answers that spawn by adopting a session. + unreachable?.onStartNewTerminal() + await flushAsyncTicks(30) + + expect(deps.clearExitedPanePtyLayoutBinding).toHaveBeenCalledWith(pane.id, 'tab-pty') + expect(deps.clearTabPtyId).toHaveBeenCalledWith('tab-1', 'tab-pty') + expect(transport.connect).toHaveBeenCalledTimes(2) expect(transport.connect.mock.calls[1]?.[0]?.sessionId).toBeUndefined() const writes = (pane.terminal.write as ReturnType).mock.calls.map( ([data]) => data as string @@ -9277,6 +9303,48 @@ describe('connectPanePty', () => { }) }) + // STA-3077 / RC2. The relay answers "not found" for any id it cannot hand back — including for + // shells still running under a relay process it replaced. Respawning there starts a second + // `--resume` against the same agent session and both processes append to one transcript. This is + // the last route to that defect, and it is the reason this pane surfaces as disconnected instead. + it('does not respawn a pane whose restore was answered with a not-found', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('tab-pty') + transport.connect.mockImplementation(async ({ sessionId }: { sessionId?: string }) => { + if (sessionId) { + throw new Error('PTY "tab-pty" not found') + } + return { id: 'respawned-pty' } + }) + transportFactoryQueue.push(transport) + + const pane = createPane(1) + const manager = createManager(1) + const onPtyRecoveryState = vi.fn<(paneId: number, state: RecoveryStateProbe) => void>() + const deps = createDeps({ + restoredLeafId: LEAF_1, + restoredPtyIdByLeafId: { [LEAF_1]: 'tab-pty' }, + onPtyRecoveryStateRef: { current: onPtyRecoveryState } + }) + + connectPanePty(pane as never, manager as never, deps as never) + await flushAsyncTicks(30) + + // Producer pin: the restore must actually have been attempted and failed, or the + // no-respawn clause below would pass for the wrong reason. + expect(transport.connect.mock.calls[0]?.[0]?.sessionId).toBe('tab-pty') + expect(transport.connect).toHaveBeenCalledTimes(1) + // Branch pin: the not-found reached the unproven arm specifically. Without this, any earlier + // bail-out (an obsolete-reattach reject, a generation mismatch) would satisfy the clauses below. + expect( + onPtyRecoveryState.mock.calls.find( + ([paneId, state]) => paneId === pane.id && state?.phase === 'disconnected' + )?.[1]?.unreachablePane + ).toBeDefined() + expect(deps.clearTabPtyId).not.toHaveBeenCalled() + expect(deps.clearExitedPanePtyLayoutBinding).not.toHaveBeenCalled() + }) + it('keeps ?25h in the live agent reattach reset when the snapshot leaves the cursor visible', async () => { const { connectPanePty } = await import('./pty-connection') const transport = createMockTransport('tab-pty') diff --git a/src/renderer/src/components/terminal-pane/reattach-failure-classification.test.ts b/src/renderer/src/components/terminal-pane/reattach-failure-classification.test.ts index b0357b1dc1d..b514f6cf926 100644 --- a/src/renderer/src/components/terminal-pane/reattach-failure-classification.test.ts +++ b/src/renderer/src/components/terminal-pane/reattach-failure-classification.test.ts @@ -9,12 +9,26 @@ import { describe, expect, it } from 'vitest' import { isProvenSshSessionGoneError } from './reattach-failure-classification' describe('reattach failure classification', () => { - it('treats an explicit host expiry as proof', () => { - expect(isProvenSshSessionGoneError(new Error('SSH_SESSION_EXPIRED: ssh-1:pty-9'))).toBe(true) + // INVERTED for STA-3077. Both clauses below asserted that a not-found — raw, or wrapped as + // SSH_SESSION_EXPIRED — proves the shell is gone. It does not, and this was the last live route + // to the reported duplicate resume. + // + // A not-found means the relay WE ASKED cannot hand that id back. That is proof of an exit only + // if the relay process that minted the pty is the one answering. When a relay is restarted or + // replaced, the new process reports not-found for shells still running under its predecessor — + // observed directly in the Docker relay harness, where a stalled relay is superseded by a fresh + // one with no memory of `pty-1` while the old shells keep running. Respawning there starts a + // second `--resume` against the same agent session. + // + // SSH_SESSION_EXPIRED is not independent evidence: its ONLY producer is that same not-found + // mapping in reattachSshPtySession. Telling the two apart needs `relayInstanceId` on the consumer + // grant (design step E-2), which is not built — so the honest answer is "unproven". + it('does not treat an explicit host expiry as proof', () => { + expect(isProvenSshSessionGoneError(new Error('SSH_SESSION_EXPIRED: ssh-1:pty-9'))).toBe(false) }) - it('treats a not-found PTY as proof', () => { - expect(isProvenSshSessionGoneError(new Error('PTY "pty-9" not found'))).toBe(true) + it('does not treat a not-found PTY as proof', () => { + expect(isProvenSshSessionGoneError(new Error('PTY "pty-9" not found'))).toBe(false) }) // The reported defect: a source that needs re-establishing was reported as @@ -65,9 +79,29 @@ describe('an identity mismatch is never proof of death', () => { expect(isProvenSshSessionGoneError(error)).toBe(false) }) - // Clause-selectivity: silencing real expiry would strand panes whose shell - // genuinely went away. - it('still proves death for the same wording without the mismatch clause', () => { - expect(isProvenSshSessionGoneError(new Error('PTY "pty-7" not found'))).toBe(true) + // INVERTED with the two clauses above. This was the clause-selectivity guard: it pinned that + // silencing the mismatch case had not silenced plain expiry too. Plain expiry is now unproven as + // well, on its own evidence — so what remains to guard is that the pane is not stranded, and that + // is the disconnected affordance's job, asserted in TerminalPaneDisconnectedBanner.test.tsx. + it('does not prove death for the same wording without the mismatch clause either', () => { + expect(isProvenSshSessionGoneError(new Error('PTY "pty-7" not found'))).toBe(false) + }) +}) + +/** + * The respawn arms are now unreachable, and that is the point: no reattach failure authorizes + * replacing a running shell. The design keeps the grant as a CONDITIONAL for step E-2 — a + * not-found whose `relayInstanceId` matches the recorded one is genuine proof — so the decision + * point stays rather than being deleted. This clause pins that nothing reaches it meanwhile. + */ +describe('no reattach failure authorizes a respawn today', () => { + it.each([ + ['a relay-worded not-found', new Error('PTY "pty-9" not found')], + ['the expiry token main publishes', new Error('SSH_SESSION_EXPIRED: ssh-1:pty-9')], + ['an identity mismatch', new Error('SSH_PTY_IDENTITY_MISMATCH: pty-7')], + ['a required source restore', new Error('SSH_SOURCE_RESTORE_REQUIRED: ssh-1:pty-9')], + ['a transport fault', new Error('read ECONNRESET')] + ])('leaves %s unproven', (_label, error) => { + expect(isProvenSshSessionGoneError(error)).toBe(false) }) }) diff --git a/src/renderer/src/components/terminal-pane/reattach-failure-classification.ts b/src/renderer/src/components/terminal-pane/reattach-failure-classification.ts index 44eeccec200..98ee18b912b 100644 --- a/src/renderer/src/components/terminal-pane/reattach-failure-classification.ts +++ b/src/renderer/src/components/terminal-pane/reattach-failure-classification.ts @@ -7,28 +7,20 @@ // Respawn now requires proof. Everything else is unresolved, which leaves the // shell running and the binding intact for a later reattach. -import { - SSH_SESSION_EXPIRED_ERROR, - SSH_SOURCE_RESTORE_REQUIRED_ERROR, - isSshPtyIdentityMismatchMessage -} from '../../../../shared/ssh-pty-failure-tokens' - -function messageOf(error: unknown): string { - return error instanceof Error ? error.message : String(error) -} - /** - * True only when the failure proves the session no longer exists. Anything - * unrecognized is unresolved, because a new failure mode must not silently - * become a respawn. + * True only when the failure proves the session no longer exists. + * + * Nothing available at this decision point does. A not-found means the relay we asked cannot hand + * that id back — which is proof of an exit ONLY if the relay process that minted the pty is the one + * answering. A restarted or replaced relay reports not-found for shells that are still running + * under its predecessor, and respawning there resumes the same agent a second time into one + * transcript. `SSH_SESSION_EXPIRED` is not independent evidence: its only producer is that same + * not-found mapping. + * + * Distinguishing the two needs `relayInstanceId` on the consumer grant (design step E-2), which is + * not built. Until it is, the honest answer is "unproven", and an unverifiable pane surfaces as + * disconnected for the user to resolve rather than being silently replaced. */ -export function isProvenSshSessionGoneError(error: unknown): boolean { - const message = messageOf(error) - if (message.includes(SSH_SOURCE_RESTORE_REQUIRED_ERROR)) { - return false - } - if (isSshPtyIdentityMismatchMessage(message)) { - return false - } - return message.includes(SSH_SESSION_EXPIRED_ERROR) || /PTY ".+" not found/i.test(message) +export function isProvenSshSessionGoneError(_error: unknown): boolean { + return false }