diff --git a/src/main/providers/ssh-pty-errors.ts b/src/main/providers/ssh-pty-errors.ts index a8c551f228b..e78c6861b8f 100644 --- a/src/main/providers/ssh-pty-errors.ts +++ b/src/main/providers/ssh-pty-errors.ts @@ -1,5 +1,13 @@ export const SSH_SESSION_EXPIRED_ERROR = 'SSH_SESSION_EXPIRED' export const SSH_PTY_IDENTITY_MISMATCH_ERROR = 'SSH_PTY_IDENTITY_MISMATCH' +/** The output source must be re-established. The shell is still running — this + * is explicitly NOT expiry, because callers respawn on expiry. */ +export const SSH_SOURCE_RESTORE_REQUIRED_ERROR = 'SSH_SOURCE_RESTORE_REQUIRED' + +export function isSshSourceRestoreRequiredError(error: unknown): boolean { + const message = error instanceof Error ? error.message : String(error) + return message.includes(SSH_SOURCE_RESTORE_REQUIRED_ERROR) +} export function isSshPtyNotFoundError(error: unknown): boolean { const message = error instanceof Error ? error.message : String(error) diff --git a/src/main/providers/ssh-pty-provider-reattach-incarnation.test.ts b/src/main/providers/ssh-pty-provider-reattach-incarnation.test.ts index 74989dc34e5..57bfc4c3f06 100644 --- a/src/main/providers/ssh-pty-provider-reattach-incarnation.test.ts +++ b/src/main/providers/ssh-pty-provider-reattach-incarnation.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from 'vitest' -import { SSH_SESSION_EXPIRED_ERROR } from './ssh-pty-errors' +import { SSH_SESSION_EXPIRED_ERROR, SSH_SOURCE_RESTORE_REQUIRED_ERROR } from './ssh-pty-errors' import { SshPtyProvider } from './ssh-pty-provider' describe('SSH PTY provider session reattach incarnation', () => { @@ -44,8 +44,10 @@ describe('SSH PTY provider session reattach incarnation', () => { } const provider = new SshPtyProvider('conn-1', mux as never) - await expect(provider.spawn({ cols: 80, rows: 24, sessionId: 'pty-old' })).rejects.toThrow( - `${SSH_SESSION_EXPIRED_ERROR}: pty-old` - ) + // Fails closed, but NOT as expiry: the shell is still running, and callers + // respawn on expiry — which duplicate-resumed the live agent session. + const spawn = provider.spawn({ cols: 80, rows: 24, sessionId: 'pty-old' }) + await expect(spawn).rejects.toThrow(`${SSH_SOURCE_RESTORE_REQUIRED_ERROR}: pty-old`) + await expect(spawn).rejects.not.toThrow(SSH_SESSION_EXPIRED_ERROR) }) }) diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index 2bc52fe491d..dfdb3d21846 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -22,7 +22,7 @@ import { buildSshPtySpawnRequest } from './ssh-pty-spawn-request' import { SshPtySpawnExitRaceTracker } from './ssh-pty-spawn-exit-race' import { SshAgentSessionCapabilities } from './ssh-agent-session-capabilities' import type { PtyProcessInspection } from './pty-process-inspection' -import { SSH_SESSION_EXPIRED_ERROR } from './ssh-pty-errors' +import { SSH_SOURCE_RESTORE_REQUIRED_ERROR } from './ssh-pty-errors' // Why: sequential relay teardown calls share one absolute budget; convert to the mux-relative timeout only at dispatch. function relayTimeoutOptions(deadlineMs: number | undefined): { timeoutMs: number } | undefined { @@ -101,8 +101,11 @@ export class SshPtyProvider implements IPtyProvider { this.outputState.rememberPtyIncarnation(relayPtyId, incarnationId) }) if (result.sourceRecovery?.status === 'restoreRequired') { + // Why not SSH_SESSION_EXPIRED: the shell is still running, only its + // output source needs re-establishing. Reporting expiry made the pane + // respawn and resume the same agent session twice into one transcript. throw new Error( - `${SSH_SESSION_EXPIRED_ERROR}: ${toRelaySshPtyId(this.connectionId, result.id)}` + `${SSH_SOURCE_RESTORE_REQUIRED_ERROR}: ${toRelaySshPtyId(this.connectionId, result.id)}` ) } this.livePtyIds.add(result.id) 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 cf834fab568..1b6ff5b4009 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -9229,7 +9229,9 @@ describe('connectPanePty', () => { transport.getPtyId.mockImplementation(() => activePtyId) transport.connect.mockImplementation(async ({ sessionId }: { sessionId?: string }) => { if (sessionId) { - throw new Error('restored session is gone') + // 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. activePtyId = 'adopted-pty' diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index dea08c2af92..19676f48afc 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -335,6 +335,7 @@ import { } from './renderer-owned-agent-status-registry' import type { DirectSshPaneRetryAttempt } from '@/store/slices/direct-ssh-terminal-recovery' import { directSshAuthoritiesEqual } from '@/store/slices/direct-ssh-terminal-authority-ledger' +import { isProvenSshSessionGoneError } from './reattach-failure-classification' const pendingSpawnByPaneKey = new Map>() const SSH_SESSION_EXPIRED_ERROR = 'SSH_SESSION_EXPIRED' @@ -9074,15 +9075,19 @@ export function connectPanePty( ptyId: deferredReattachSessionId, reason: message }) - deps.clearExitedPanePtyLayoutBinding(pane.id, deferredReattachSessionId) - deps.clearTabPtyId(deps.tabId, deferredReattachSessionId) - if (connectionId && isSshSessionExpiredError(err)) { - startFreshColdRestoreAgentResume(coldRestoreStartup, { - forceBlankRestoredViewport: true - }) + // Why: only proof that the session is gone may respawn. A transport + // fault, a wedged call, or a source that merely needs re-establishing + // leaves the shell running, and respawning there resumes the same + // agent session a second time into one transcript. + if (!isProvenSshSessionGoneError(err)) { + reportError(message) return } - reportError(message) + deps.clearExitedPanePtyLayoutBinding(pane.id, deferredReattachSessionId) + deps.clearTabPtyId(deps.tabId, deferredReattachSessionId) + if (!(connectionId && isSshSessionExpiredError(err))) { + reportError(message) + } startFreshColdRestoreAgentResume(coldRestoreStartup, { forceBlankRestoredViewport: true }) 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 new file mode 100644 index 00000000000..041f62d0470 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/reattach-failure-classification.test.ts @@ -0,0 +1,44 @@ +/** + * Duplicate-agent oracles. Reattach failure used to converge on "spawn a fresh + * shell", so a transient fault started a second `--resume` against the same + * agent session and both processes appended to one transcript. + * + * Respawn requires proof the session is gone. These pin which failures qualify. + */ +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) + }) + + it('treats a not-found PTY as proof', () => { + expect(isProvenSshSessionGoneError(new Error('PTY "pty-9" not found'))).toBe(true) + }) + + // The reported defect: a source that needs re-establishing was reported as + // expiry, so the pane respawned and duplicate-resumed a live agent session. + it('does not treat a required source restore as proof', () => { + expect(isProvenSshSessionGoneError(new Error('SSH_SOURCE_RESTORE_REQUIRED: ssh-1:pty-9'))).toBe( + false + ) + }) + + it.each([ + ['a transport fault', new Error('read ECONNRESET')], + ['a timed-out call', new Error('relay request timed out')], + ['a disconnected client', new Error('client_disconnected')], + ['an unavailable owner', new Error('execution_owner_unavailable')], + ['an identity mismatch', new Error('SSH_PTY_IDENTITY_MISMATCH')], + ['an empty message', new Error('')], + ['a non-Error rejection', 'something went wrong'] + ])('does not treat %s as proof', (_label, error) => { + expect(isProvenSshSessionGoneError(error)).toBe(false) + }) + + // A new failure mode must not silently become a respawn. + it('defaults an unrecognized failure to unresolved', () => { + expect(isProvenSshSessionGoneError(new Error('SOME_FUTURE_RELAY_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 new file mode 100644 index 00000000000..d21673b2d63 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/reattach-failure-classification.ts @@ -0,0 +1,30 @@ +// Why this module exists: reattach failure used to converge on one action — +// spawn a fresh shell. A transport fault and a genuinely-gone session were +// indistinguishable at the decision point, so a transient error respawned the +// pane and resumed the same agent session a second time; both processes then +// appended to one transcript. +// +// Respawn now requires proof. Everything else is unresolved, which leaves the +// shell running and the binding intact for a later reattach. + +/** The host said the session is gone. */ +const SSH_SESSION_EXPIRED_ERROR = 'SSH_SESSION_EXPIRED' +/** The shell is alive; only its output source must be re-established. */ +const SSH_SOURCE_RESTORE_REQUIRED_ERROR = 'SSH_SOURCE_RESTORE_REQUIRED' + +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. + */ +export function isProvenSshSessionGoneError(error: unknown): boolean { + const message = messageOf(error) + if (message.includes(SSH_SOURCE_RESTORE_REQUIRED_ERROR)) { + return false + } + return message.includes(SSH_SESSION_EXPIRED_ERROR) || /PTY ".+" not found/i.test(message) +}