From 0ce8b07d23200514ab3ba5442d06ddb0f7c26fda Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 27 Aug 2026 02:34:49 -0700 Subject: [PATCH] fix(ssh): a relay that reports a PTY absent must authorise a fresh spawn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reattachSshPtySession flattened the relay's `PTY "" not found` into a bare `SSH_SESSION_EXPIRED`, discarding the evidence. isPtyAlreadyGoneError classifies on that message via /PTY ".+" not found/, so it could never match — and attachStablePaneOwner's existing already-gone fallback (retire the pane binding, onPtyExit, spawn fresh) is gated on exactly that predicate. The recovery machinery was already correct; only the classification was broken, which is why `pty:spawn` rejected outright after a relay restart renumbered its PTYs from pty-1. SshPtyAbsentFromRelayError carries the verdict the message cannot: a reachable relay answered for this exact id and reported it absent. Both thrown messages stay byte-identical to the previous ones, so all six message-based consumers are untouched; only callers that can act on the stronger verdict test the class. Deliberately not raised for a transport failure, timeout, disposed multiplexer, identity mismatch, or restoreRequired — none of those observe the process, and treating them as absence would orphan live remote work. --- .../stable-pane-relay-absence-respawn.test.ts | 63 +++++++++++++++ src/main/ipc/pty/provider/liveness.ts | 13 +++- src/main/providers/ssh-pty-errors.ts | 22 ++++++ .../ssh-pty-relay-absence-verdict.test.ts | 78 +++++++++++++++++++ .../providers/ssh-pty-session-reattach.ts | 15 +++- 5 files changed, 185 insertions(+), 6 deletions(-) create mode 100644 src/main/ipc/pty/pane/stable-pane-relay-absence-respawn.test.ts create mode 100644 src/main/providers/ssh-pty-relay-absence-verdict.test.ts diff --git a/src/main/ipc/pty/pane/stable-pane-relay-absence-respawn.test.ts b/src/main/ipc/pty/pane/stable-pane-relay-absence-respawn.test.ts new file mode 100644 index 00000000000..c33bd6b6a9a --- /dev/null +++ b/src/main/ipc/pty/pane/stable-pane-relay-absence-respawn.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it, vi } from 'vitest' +import { + SSH_SESSION_EXPIRED_ERROR, + SshPtyAbsentFromRelayError +} from '../../../providers/ssh-pty-errors' +import type { IPtyProvider } from '../../../providers/types' +import { spawnForStablePane, type StablePaneOwner } from './stable-owner' + +const OWNER: StablePaneOwner = { + tabId: 'tab-1', + leafId: '1b3f2c4d-5e6a-4b7c-8d9e-0f1a2b3c4d5e', + ptyId: 'ssh:conn-1@@pty-1' +} + +function spawnAfterAttachRejection(error: unknown): { + run: () => ReturnType + spawn: ReturnType +} { + const spawn = vi + .fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce({ id: 'ssh:conn-1@@pty-1', isReattach: false }) + return { + spawn, + run: () => + spawnForStablePane({ + runtime: undefined, + provider: { spawn } as unknown as IPtyProvider, + spawnOptions: { cols: 80, rows: 24 }, + owner: OWNER, + connectionId: 'conn-1', + resolveOwner: () => null + }) + } +} + +describe('stable pane adoption after the relay reports the PTY absent', () => { + it('spawns fresh once the relay has positively answered for that id', async () => { + const { run, spawn } = spawnAfterAttachRejection( + new SshPtyAbsentFromRelayError(`${SSH_SESSION_EXPIRED_ERROR}: pty-1`) + ) + + const result = await run() + + expect(spawn).toHaveBeenCalledTimes(2) + expect(spawn.mock.calls[0]?.[0]).toMatchObject({ attachOnly: true }) + expect(spawn.mock.calls[1]?.[0]).not.toHaveProperty('sessionId') + expect(result.owner).toBeNull() + }) + + // A restarted relay renumbers from pty-1, so the message alone cannot distinguish absence from a + // lost link — only the class may authorise abandoning the binding. + it.each([ + ['an expired session with no relay evidence', `${SSH_SESSION_EXPIRED_ERROR}: pty-1`], + ['a lost link', 'SSH connection lost, reconnecting...'], + ['a request timeout', 'Request "pty.attach" timed out after 10000ms'] + ])('keeps the binding and refuses to respawn after %s', async (_label, message) => { + const { run, spawn } = spawnAfterAttachRejection(new Error(message)) + + await expect(run()).rejects.toThrow(message) + expect(spawn).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/ipc/pty/provider/liveness.ts b/src/main/ipc/pty/provider/liveness.ts index a8c005cce06..a5ff137f914 100644 --- a/src/main/ipc/pty/provider/liveness.ts +++ b/src/main/ipc/pty/provider/liveness.ts @@ -3,7 +3,10 @@ import type { AgentSessionOwnerBinding } from '../../../../shared/agent-session- import { agentSessionOwnerBindingsEqual } from '../../../../shared/claimed-agent-pty-owner' import { addNodePtyRecoveryHint } from '../../../daemon/node-pty-error-hints' import type { Store } from '../../../persistence' -import { isSshPtyNotFoundError } from '../../../providers/ssh-pty-errors' +import { + isSshPtyAbsentFromRelayError, + isSshPtyNotFoundError +} from '../../../providers/ssh-pty-errors' import type { IPtyProvider } from '../../../providers/types' import { markClaudePtyExited } from '../../../claude-accounts/live-pty-gate' import { ptyIncarnationById, ptyOwnership } from './ownership-state' @@ -54,7 +57,13 @@ export function normalizeNodePtySpawnError(err: unknown): Error { export function isPtyAlreadyGoneError(err: unknown): boolean { const message = err instanceof Error ? err.message : String(err) - return isSshPtyNotFoundError(err) || /Session not found/i.test(message) + return ( + isSshPtyNotFoundError(err) || + // Why: the reattach path rewrites the relay's wording to SSH_SESSION_EXPIRED and only this + // class preserves that the relay itself answered "absent" rather than the link dropping. + isSshPtyAbsentFromRelayError(err) || + /Session not found/i.test(message) + ) } export function delay(ms: number): Promise { diff --git a/src/main/providers/ssh-pty-errors.ts b/src/main/providers/ssh-pty-errors.ts index a8c551f228b..fb1244c7b4b 100644 --- a/src/main/providers/ssh-pty-errors.ts +++ b/src/main/providers/ssh-pty-errors.ts @@ -10,3 +10,25 @@ export function isSshPtyIdentityMismatchError(error: unknown): boolean { const message = error instanceof Error ? error.message : String(error) return message.includes(SSH_PTY_IDENTITY_MISMATCH_ERROR) || /identity mismatch/i.test(message) } + +/** + * A reachable relay answered for this exact PTY id and reported it absent — positive evidence of + * absence from the execution host, so `exited` rather than `unverifiable` + * (docs/reference/ssh-execution-boundary.md). Deliberately NOT raised for a transport failure, a + * request timeout, a disposed multiplexer, an identity mismatch (the id names a live PTY belonging + * to another pane), or `restoreRequired` (the PTY is live, only its source stream is not) — none of + * those observe the process, and treating them as absence orphans live remote work. + * + * Carries the same `SSH_SESSION_EXPIRED` message so message-based consumers are unaffected; only + * callers that can act on the stronger verdict test the class. + */ +export class SshPtyAbsentFromRelayError extends Error { + constructor(message: string) { + super(message) + this.name = 'SshPtyAbsentFromRelayError' + } +} + +export function isSshPtyAbsentFromRelayError(error: unknown): boolean { + return error instanceof SshPtyAbsentFromRelayError +} diff --git a/src/main/providers/ssh-pty-relay-absence-verdict.test.ts b/src/main/providers/ssh-pty-relay-absence-verdict.test.ts new file mode 100644 index 00000000000..9e5b9a555ac --- /dev/null +++ b/src/main/providers/ssh-pty-relay-absence-verdict.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it, vi } from 'vitest' +import { + SSH_PTY_IDENTITY_MISMATCH_ERROR, + SSH_SESSION_EXPIRED_ERROR, + isSshPtyAbsentFromRelayError +} from './ssh-pty-errors' +import { SshPtyProvider } from './ssh-pty-provider' + +function providerRejectingAttachWith(error: unknown): SshPtyProvider { + const mux = { + request: vi.fn().mockRejectedValue(error), + notify: vi.fn(), + onNotification: vi.fn().mockReturnValue(vi.fn()) + } + return new SshPtyProvider('conn-1', mux as never) +} + +async function reattachRejection(error: unknown): Promise { + const provider = providerRejectingAttachWith(error) + return await provider.spawn({ cols: 80, rows: 24, sessionId: 'pty-1' }).then( + () => undefined, + (rejection: unknown) => rejection + ) +} + +describe('SSH PTY relay absence verdict', () => { + it('reports a relay-delivered not-found as positive absence', async () => { + const rejection = await reattachRejection(new Error('PTY "pty-1" not found')) + + expect(isSshPtyAbsentFromRelayError(rejection)).toBe(true) + expect((rejection as Error).message).toBe(`${SSH_SESSION_EXPIRED_ERROR}: pty-1`) + }) + + it('does not report absence when the id names a live PTY owned by another pane', async () => { + const rejection = await reattachRejection( + new Error('PTY "pty-1" not found (identity mismatch)') + ) + + expect(isSshPtyAbsentFromRelayError(rejection)).toBe(false) + // Byte-identical to the pre-change message: only the class is new, so the toast humanizer, + // both spawn-execute lease paths, and the renderer's substring checks are untouched. + expect((rejection as Error).message).toBe( + `${SSH_SESSION_EXPIRED_ERROR}: pty-1 ${SSH_PTY_IDENTITY_MISMATCH_ERROR}` + ) + }) + + // Loss of contact never observes the process: each of these must stay `unverifiable`. + it.each([ + ['a lost link', 'SSH connection lost, reconnecting...'], + ['a disposed multiplexer', 'Multiplexer disposed'], + ['a request timeout', 'Request "pty.attach" timed out after 10000ms'] + ])('does not report absence for %s', async (_label, message) => { + const rejection = await reattachRejection(new Error(message)) + + expect(isSshPtyAbsentFromRelayError(rejection)).toBe(false) + expect((rejection as Error).message).toBe(message) + }) + + it('does not report absence when the PTY is live but its source needs restoring', async () => { + const mux = { + request: vi.fn().mockResolvedValue({ + incarnationId: 'incarnation-1', + sourceRecovery: { status: 'restoreRequired', reason: 'checkpointUnavailable' } + }), + notify: vi.fn(), + onNotification: vi.fn().mockReturnValue(vi.fn()) + } + const provider = new SshPtyProvider('conn-1', mux as never) + + const rejection = await provider.spawn({ cols: 80, rows: 24, sessionId: 'pty-1' }).then( + () => undefined, + (error: unknown) => error + ) + + expect(isSshPtyAbsentFromRelayError(rejection)).toBe(false) + expect((rejection as Error).message).toContain(SSH_SESSION_EXPIRED_ERROR) + }) +}) diff --git a/src/main/providers/ssh-pty-session-reattach.ts b/src/main/providers/ssh-pty-session-reattach.ts index 3a7b8b15c79..d6da283cab9 100644 --- a/src/main/providers/ssh-pty-session-reattach.ts +++ b/src/main/providers/ssh-pty-session-reattach.ts @@ -3,6 +3,7 @@ import { isPtyIncarnationId, type PtyIncarnationId } from '../../shared/pty-inca import { SSH_PTY_IDENTITY_MISMATCH_ERROR, SSH_SESSION_EXPIRED_ERROR, + SshPtyAbsentFromRelayError, isSshPtyIdentityMismatchError, isSshPtyNotFoundError } from './ssh-pty-errors' @@ -225,10 +226,16 @@ export async function reattachSshPtySession(args: { // Why: an expired relay lease must be surfaced distinctly so the renderer clears its binding. console.warn(`[ssh-pty] pty.attach FAILED for ${args.sessionId}:`, error) if (isSshPtyNotFoundError(error)) { - const mismatchMarker = isSshPtyIdentityMismatchError(error) - ? ` ${SSH_PTY_IDENTITY_MISMATCH_ERROR}` - : '' - throw new Error(`${SSH_SESSION_EXPIRED_ERROR}: ${relaySessionId}${mismatchMarker}`) + if (isSshPtyIdentityMismatchError(error)) { + // The id names a LIVE PTY owned by another pane, so this is not evidence of absence. + throw new Error( + `${SSH_SESSION_EXPIRED_ERROR}: ${relaySessionId} ${SSH_PTY_IDENTITY_MISMATCH_ERROR}` + ) + } + // Why the class: the relay answered for this exact id, so callers holding a pane binding may + // retire it and spawn fresh. Plain `SSH_SESSION_EXPIRED` cannot say that — a restarted relay + // renumbers from pty-1, so the message alone is indistinguishable from a lost link. + throw new SshPtyAbsentFromRelayError(`${SSH_SESSION_EXPIRED_ERROR}: ${relaySessionId}`) } throw error }