fix(terminal): a reattach not-found is not proof the shell is gone

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.
This commit is contained in:
Neil
2026-08-09 02:30:28 -07:00
parent 460e68630d
commit 7cd7fef927
3 changed files with 129 additions and 35 deletions
@@ -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<string, unknown> = {}) {
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<typeof vi.fn>).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')
@@ -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)
})
})
@@ -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
}