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 }