fix(terminal): stop respawning a shell that is still running

A pane that failed to reattach spawned a fresh shell. Because the
restored session id came along, the replacement resumed the same agent
session, and two processes appended to one transcript — reported
repeatedly, up to five concurrent resumes of a single session.

Two defects fed it.

The relay reported a source that merely needed re-establishing as
`SSH_SESSION_EXPIRED`. The shell was still running; only its output
source was gone. Give that outcome its own error so it stops reading as
"the session no longer exists".

The reattach failure handler then treated every error as proof of death.
It checked for expiry and, in the else branch, took the identical
action — so the check bought nothing and a transport fault, a timed-out
call, or a wedged relay all respawned. Respawn now requires proof: an
explicit host expiry or a not-found PTY. Anything else, including an
error we have never seen before, is unresolved, leaves the shell
running, and keeps the binding for a later reattach.

Two existing tests asserted the old behavior. One threw a bare error as
scaffolding to reach the spawn-adoption door; it now throws proof, which
is what it meant. The other pinned the expiry mapping itself, and now
asserts the outcome fails closed *without* being reported as expiry.

Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Neil
2026-08-08 19:11:34 -07:00
co-authored by Orca
parent ae134995c1
commit eec807d759
7 changed files with 108 additions and 14 deletions
+8
View File
@@ -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)
@@ -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)
})
})
+5 -2
View File
@@ -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)
@@ -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'
@@ -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<string, Promise<string | null>>()
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
})
@@ -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)
})
})
@@ -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)
}