From 2ed57ae61b9e46723a95378ceef94e25760f817a Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 16 Aug 2026 21:44:16 -0700 Subject: [PATCH] fix(ssh): quarantine ambiguous direct reattach exits --- .../ssh-pty-provider-exit-race.test.ts | 39 +++++++++++++++---- .../providers/ssh-pty-session-reattach.ts | 30 +++++++------- src/main/providers/ssh-pty-spawn-exit-race.ts | 18 +++++++++ 3 files changed, 65 insertions(+), 22 deletions(-) diff --git a/src/main/providers/ssh-pty-provider-exit-race.test.ts b/src/main/providers/ssh-pty-provider-exit-race.test.ts index 257ff1dd378..28a37bf85bb 100644 --- a/src/main/providers/ssh-pty-provider-exit-race.test.ts +++ b/src/main/providers/ssh-pty-provider-exit-race.test.ts @@ -1,5 +1,6 @@ import { expect, it, vi } from 'vitest' import { SshPtyProvider } from './ssh-pty-provider' +import { SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR } from './ssh-pty-errors' function sourceActivation(ptyIncarnation: string) { return { @@ -130,7 +131,7 @@ it('rejects an SSH reattach whose matching exit shares the attach reply batch', }) }) -it('does not classify a legacy exit as current while reattach resolves its incarnation', async () => { +it('keeps a legacy exit unverifiable while reattach resolves its incarnation', async () => { const mux = { request: vi.fn(), notify: vi.fn(), @@ -166,13 +167,9 @@ it('does not classify a legacy exit as current while reattach resolves its incar expect(exitListener).not.toHaveBeenCalled() resolveAttach?.() - await expect(spawn).resolves.toMatchObject({ - id: 'ssh:conn-1@@pty-existing', - incarnationId: 'incarnation-current', - isReattach: true - }) + await expect(spawn).rejects.toThrow(SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR) expect(exitListener).not.toHaveBeenCalled() - await expect(provider.probePtyLiveness('ssh:conn-1@@pty-existing')).resolves.toBe(true) + await expect(provider.probePtyLiveness('ssh:conn-1@@pty-existing')).resolves.toBeNull() notify?.('pty.exit', { id: 'pty-existing', @@ -188,6 +185,34 @@ it('does not classify a legacy exit as current while reattach resolves its incar }) }) +it('keeps an incarnation-less attach and exit race unverifiable', async () => { + const mux = { + request: vi.fn(), + notify: vi.fn(), + onNotification: vi.fn(), + dispose: vi.fn(), + isDisposed: vi.fn().mockReturnValue(false) + } + const provider = new SshPtyProvider('conn-1', mux as never) + const exitListener = vi.fn() + provider.onExit(exitListener) + mux.request.mockImplementation(async (method: string, _params, options) => { + if (method !== 'pty.attach') { + return undefined + } + options?.beforeResolve?.({}) + const notify = mux.onNotification.mock.calls[0]?.[0] + notify?.('pty.exit', { id: 'pty-existing', code: 0 }) + return {} + }) + + await expect( + provider.spawn({ cols: 80, rows: 24, sessionId: 'ssh:conn-1@@pty-existing' }) + ).rejects.toThrow(SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR) + expect(exitListener).not.toHaveBeenCalled() + await expect(provider.probePtyLiveness('ssh:conn-1@@pty-existing')).resolves.toBeNull() +}) + it('returns a provisional source activation lease to reconnect authority', async () => { const mux = { request: vi.fn(), diff --git a/src/main/providers/ssh-pty-session-reattach.ts b/src/main/providers/ssh-pty-session-reattach.ts index 5fc255efe41..809f7d45354 100644 --- a/src/main/providers/ssh-pty-session-reattach.ts +++ b/src/main/providers/ssh-pty-session-reattach.ts @@ -244,14 +244,16 @@ export async function reattachSshPtySessionWithExitFence( try { result = await reattachSshPtySession(args) const relayPtyId = toRelaySshPtyId(args.connectionId, result.id) - if ( - args.exitRaceTracker.didMatchingExitArrive(operation, { - id: relayPtyId, - incarnationId: result.incarnationId - }) - ) { + const exitOutcome = args.exitRaceTracker.classifyReattachExit(operation, { + id: relayPtyId, + incarnationId: result.incarnationId + }) + if (exitOutcome === 'exited') { throw new Error('agent_session_exited_during_start') } + if (exitOutcome === 'unverifiable') { + throw new Error(`${SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR}: ${relayPtyId}`) + } return result } catch (error) { result?.sourceActivationLease?.rollback() @@ -294,17 +296,15 @@ export async function reattachSshPtySessionForSpawn( await result?.sourceActivationLease?.rollback() if (isSshPtyExitedEvidenceError(error)) { args.acceptExitedPty(result?.id ?? toAppSshPtyId(args.connectionId, args.sessionId)) - } else if ( - !isSshPtyIdentityMismatchError(error) && - !isSshPtyRestoreRequiredError(error) && - !isSshPtyLivenessUnverifiableError(error) - ) { + } else if (!isSshPtyIdentityMismatchError(error) && !isSshPtyRestoreRequiredError(error)) { const id = result?.id ?? toAppSshPtyId(args.connectionId, args.sessionId) args.acceptUnverifiablePty(id) - throw new Error( - `${SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR}: ${toRelaySshPtyId(args.connectionId, id)}`, - { cause: error } - ) + if (!isSshPtyLivenessUnverifiableError(error)) { + throw new Error( + `${SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR}: ${toRelaySshPtyId(args.connectionId, id)}`, + { cause: error } + ) + } } throw error } diff --git a/src/main/providers/ssh-pty-spawn-exit-race.ts b/src/main/providers/ssh-pty-spawn-exit-race.ts index ecb63b59083..42393eab220 100644 --- a/src/main/providers/ssh-pty-spawn-exit-race.ts +++ b/src/main/providers/ssh-pty-spawn-exit-race.ts @@ -9,6 +9,8 @@ type PendingSshPtySpawn = { }[] } +type SshPtyPendingExitOutcome = 'exited' | 'unverifiable' | null + export class SshPtySpawnExitRaceTracker { private pending = new Set() @@ -52,6 +54,22 @@ export class SshPtySpawnExitRaceTracker { return matchingExit !== undefined } + classifyReattachExit( + operation: PendingSshPtySpawn, + result: { id: string; incarnationId?: PtyIncarnationId } + ): SshPtyPendingExitOutcome { + const sameIdExits = operation.exits.filter((exit) => exit.relayPtyId === result.id) + if (!result.incarnationId) { + return sameIdExits.length > 0 ? 'unverifiable' : null + } + const matchingExit = sameIdExits.find((exit) => exit.incarnationId === result.incarnationId) + if (matchingExit) { + matchingExit.publish?.() + return 'exited' + } + return sameIdExits.some((exit) => !exit.incarnationId) ? 'unverifiable' : null + } + finish(operation: PendingSshPtySpawn): void { this.pending.delete(operation) }