diff --git a/src/main/providers/ssh-agent-session-create-operation.ts b/src/main/providers/ssh-agent-session-create-operation.ts index adc0eb264d8..d8115de9ea4 100644 --- a/src/main/providers/ssh-agent-session-create-operation.ts +++ b/src/main/providers/ssh-agent-session-create-operation.ts @@ -4,6 +4,7 @@ import { isPtyIncarnationId } from '../../shared/pty-incarnation' import type { PtySpawnResult } from './pty-spawn-result' import type { PtySpawnOptions } from './types' import type { SshPtySpawnExitRaceTracker } from './ssh-pty-spawn-exit-race' +import { SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR } from './ssh-pty-errors' import type { SshPtyReceivingActivationLease } from './ssh-pty-notification-routing' import { parsePtySourceReceivingActivation, @@ -86,6 +87,7 @@ export async function spawnFreshSshPty(args: { ) => SshPtyReceivingActivationLease rememberPtyIncarnation: (relayPtyId: string, incarnationId: unknown) => void acceptLivePty: (appPtyId: string) => void + acceptUnverifiablePty: (appPtyId: string) => void toAppPtyId: (relayPtyId: string) => string }): Promise { const operation = args.exitRaceTracker.begin() @@ -97,18 +99,30 @@ export async function spawnFreshSshPty(args: { signal: args.options.signal, params: args.params, beforeResolve: (value) => { - sourceActivationLease = installSpawnSourceActivation(value, args.installSourceActivation) + const prepared = parseSshPtySpawnResult(value) + if (typeof prepared.id === 'string' && prepared.id.length > 0) { + args.exitRaceTracker.bind(operation, prepared.id) + } + sourceActivationLease = installSpawnSourceActivation(prepared, args.installSourceActivation) } }) if (args.options.agentSessionCreateOperationId) { assertSshAgentSessionCreateResult(result) } const spawnResult = parseSshPtySpawnResult(result) - if (args.exitRaceTracker.didMatchingExitArrive(operation, spawnResult)) { + const exitOutcome = args.exitRaceTracker.classifyPendingExit(operation, spawnResult) + if (exitOutcome === 'exited') { throw Object.assign(new Error('agent_session_exited_during_start'), { agentSessionOperationOutcome: 'unknown' as const }) } + if (exitOutcome === 'unverifiable') { + const id = args.toAppPtyId(spawnResult.id) + args.acceptUnverifiablePty(id) + throw Object.assign(new Error(`${SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR}: ${spawnResult.id}`), { + agentSessionOperationOutcome: 'unknown' as const + }) + } const claimed = spawnResult.agentSessionEnsure if (args.options.agentSessionEnsure) { const validation = validateClaimedSshSpawn(spawnResult, args.options.agentSessionEnsure) 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 28a37bf85bb..dc6e86cc2df 100644 --- a/src/main/providers/ssh-pty-provider-exit-race.test.ts +++ b/src/main/providers/ssh-pty-provider-exit-race.test.ts @@ -84,6 +84,35 @@ it('rejects a fresh SSH PTY whose exit shares the spawn response batch', async ( }) }) +it('keeps a fresh spawn with a legacy exit 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.spawn') { + return undefined + } + const result = { id: 'pty-raced', incarnationId: 'incarnation-current' } + options?.beforeResolve?.(result) + const notify = mux.onNotification.mock.calls[0]?.[0] + notify?.('pty.exit', { id: 'pty-raced', code: 0 }) + return result + }) + + await expect(provider.spawn({ cols: 80, rows: 24 })).rejects.toThrow( + SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR + ) + expect(exitListener).not.toHaveBeenCalled() + await expect(provider.probePtyLiveness('ssh:conn-1@@pty-raced')).resolves.toBeNull() +}) + it('rejects an SSH reattach whose matching exit shares the attach reply batch', async () => { const mux = { request: vi.fn(), diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index f0316129300..6fb8d0da68b 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -130,6 +130,7 @@ export class SshPtyProvider implements IPtyProvider { rememberPtyIncarnation: (id, incarnation) => this.outputState.rememberPtyIncarnation(id, incarnation), acceptLivePty: (id) => this.acceptLivePty(id), + acceptUnverifiablePty: (id) => this.acceptUnverifiablePty(id), toAppPtyId: this.toAppPtyId }) } diff --git a/src/main/providers/ssh-pty-session-reattach.ts b/src/main/providers/ssh-pty-session-reattach.ts index 809f7d45354..51d1d037bf9 100644 --- a/src/main/providers/ssh-pty-session-reattach.ts +++ b/src/main/providers/ssh-pty-session-reattach.ts @@ -244,7 +244,7 @@ export async function reattachSshPtySessionWithExitFence( try { result = await reattachSshPtySession(args) const relayPtyId = toRelaySshPtyId(args.connectionId, result.id) - const exitOutcome = args.exitRaceTracker.classifyReattachExit(operation, { + const exitOutcome = args.exitRaceTracker.classifyPendingExit(operation, { id: relayPtyId, incarnationId: result.incarnationId }) diff --git a/src/main/providers/ssh-pty-spawn-exit-race.ts b/src/main/providers/ssh-pty-spawn-exit-race.ts index 42393eab220..475699e294b 100644 --- a/src/main/providers/ssh-pty-spawn-exit-race.ts +++ b/src/main/providers/ssh-pty-spawn-exit-race.ts @@ -20,6 +20,10 @@ export class SshPtySpawnExitRaceTracker { return operation } + bind(operation: PendingSshPtySpawn, relayPtyId: string): void { + operation.relayPtyId = relayPtyId + } + recordExit(relayPtyId: string, incarnationId: unknown, publish?: () => void): boolean { let quarantined = false let published = false @@ -41,20 +45,7 @@ export class SshPtySpawnExitRaceTracker { return quarantined } - didMatchingExitArrive( - operation: PendingSshPtySpawn, - result: { id: string; incarnationId?: PtyIncarnationId } - ): boolean { - const matchingExit = operation.exits.find( - (exit) => - exit.relayPtyId === result.id && - (!result.incarnationId || exit.incarnationId === result.incarnationId) - ) - matchingExit?.publish?.() - return matchingExit !== undefined - } - - classifyReattachExit( + classifyPendingExit( operation: PendingSshPtySpawn, result: { id: string; incarnationId?: PtyIncarnationId } ): SshPtyPendingExitOutcome { diff --git a/src/main/ssh/ssh-relay-session-incarnation.test.ts b/src/main/ssh/ssh-relay-session-incarnation.test.ts index b3a52b0798d..88713bf23f0 100644 --- a/src/main/ssh/ssh-relay-session-incarnation.test.ts +++ b/src/main/ssh/ssh-relay-session-incarnation.test.ts @@ -240,4 +240,32 @@ describe('SSH relay PTY incarnation exits', () => { ) expect(acceptOutputExitMock).not.toHaveBeenCalled() }) + + it('marks a legacy exit unverifiable without incarnation ownership', async () => { + const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + await session.establish(mockConn) + const provider = vi.mocked(registerSshPtyProvider).mock.calls[0]?.[1] as unknown as { + onExit: ReturnType + acceptUnverifiablePty: ReturnType + } + const onExit = provider.onExit.mock.calls[0]?.[0] as (payload: { + id: string + code: number + providerGeneration: number + ptyIncarnation: string + }) => void + + onExit({ + id: 'ssh:target-1@@pty-legacy', + code: 0, + providerGeneration: 31, + ptyIncarnation: 'legacy:31:1:pty-legacy' + }) + + expect(provider.acceptUnverifiablePty).toHaveBeenCalledExactlyOnceWith( + 'ssh:target-1@@pty-legacy' + ) + expect(acceptOutputExitMock).not.toHaveBeenCalled() + }) }) diff --git a/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts b/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts index 67c4c24a220..742c5a7769a 100644 --- a/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts +++ b/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts @@ -571,7 +571,7 @@ describe('SshRelaySession reconnect incarnation ordering', () => { expect(sourceActivationLease.commit).not.toHaveBeenCalled() }) - it('ignores an older incarnation exit while reconnecting a reused PTY id', async () => { + it('ignores an explicitly older incarnation exit while reconnecting a reused PTY id', async () => { const { mockConn, mockStore, mockPortForward, getMainWindow, mockWindow } = createMockDeps() const currentIncarnationId = 'incarnation-current' const runtime = { @@ -583,7 +583,6 @@ describe('SshRelaySession reconnect incarnation ordering', () => { const sourceActivationLease = { commit: vi.fn(), rollback: vi.fn() } vi.mocked(getSshPtyProvider).mockReturnValue({ attachForReconnect: vi.fn().mockImplementation(async () => { - emitExitDuringAttach({ id: APP_PTY_ID, code: 0 }) emitExitDuringAttach({ id: APP_PTY_ID, code: 0, @@ -625,6 +624,41 @@ describe('SshRelaySession reconnect incarnation ordering', () => { }) }) + it('keeps a legacy exit unverifiable when reconnect resolves an incarnation', async () => { + const { mockConn, mockStore, mockPortForward, getMainWindow, mockWindow } = createMockDeps() + const sourceActivationLease = { commit: vi.fn(), rollback: vi.fn() } + const acceptUnverifiablePty = vi.fn() + const acceptExitedPty = vi.fn() + vi.mocked(getSshPtyProvider).mockReturnValue({ + attachForReconnect: vi.fn().mockImplementation(async () => { + emitExitDuringAttach({ id: APP_PTY_ID, code: 0, ptyIncarnation: undefined }) + return { + incarnationId: 'incarnation-current', + replay: 'ambiguous-output', + sourceActivationLease + } + }), + acceptUnverifiablePty, + acceptExitedPty, + dispose: vi.fn() + } as unknown as ReturnType) + vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([detachedLease()] as ReturnType< + typeof mockStore.getSshRemotePtyLeases + >) + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + + await session.establish(mockConn) + + expect(acceptUnverifiablePty).toHaveBeenCalledExactlyOnceWith(APP_PTY_ID) + expect(acceptExitedPty).not.toHaveBeenCalled() + expect(acceptOutputExitMock).not.toHaveBeenCalled() + expect(setPtyOwnership).not.toHaveBeenCalled() + expect(mockStore.markSshRemotePtyLeasesAttachedAsync).not.toHaveBeenCalled() + expect(sourceActivationLease.rollback).toHaveBeenCalledOnce() + expect(sourceActivationLease.commit).not.toHaveBeenCalled() + expect(mockWindow.webContents.send).not.toHaveBeenCalledWith('pty:replay', expect.anything()) + }) + it('keeps the attached PTY when incarnation backfill persistence fails', async () => { const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() const incarnationId = 'incarnation-reconnect' diff --git a/src/main/ssh/ssh-relay-session-recovery-races.test.ts b/src/main/ssh/ssh-relay-session-recovery-races.test.ts index 26dd798ac5a..8705eba0350 100644 --- a/src/main/ssh/ssh-relay-session-recovery-races.test.ts +++ b/src/main/ssh/ssh-relay-session-recovery-races.test.ts @@ -76,6 +76,7 @@ vi.mock('../providers/ssh-pty-provider', () => ({ return () => {} }) attachForReconnect = attachForReconnectMock + acceptUnverifiablePty = vi.fn() setPtyDeliveryPauseAdapter = vi.fn() dispose = ptyProviderDisposeMock } @@ -285,6 +286,44 @@ describe('SshRelaySession recovery race fencing', () => { expect(deps.mockStore.markSshRemotePtyLeasesAttachedAsync).not.toHaveBeenCalled() }) + it('keeps a legacy exit during source recovery unverifiable', async () => { + const targetId = 'legacy-exit-during-recovery' + const { session, deps } = await prepareRecovery(targetId) + const recoveryActivationLease = { commit: vi.fn(), retire: vi.fn() } + const sourceActivationLease = { + commit: vi.fn(), + rollback: vi.fn(async () => true), + transferToRecovery: vi.fn(() => { + queueMicrotask(() => { + ptyExitHandlerRef.current?.({ + id: `ssh:${targetId}@@pty-1`, + code: 0, + providerGeneration: 23, + ptyIncarnation: 'incarnation-1' + }) + }) + return recoveryActivationLease + }) + } + attachForReconnectMock.mockResolvedValue({ + incarnationId: 'incarnation-1', + sourceRecovery: pendingRecovery(4), + sourceActivationLease + }) + + await session.reconnect(deps.mockConn) + const provider = vi.mocked(registerSshPtyProvider).mock.calls.at(-1)?.[1] as unknown as { + acceptUnverifiablePty: ReturnType + } + + expect(provider.acceptUnverifiablePty).toHaveBeenCalledWith(`ssh:${targetId}@@pty-1`) + expect(acceptOutputExitMock).not.toHaveBeenCalled() + expect(recoveryActivationLease.commit).not.toHaveBeenCalled() + expect(recoveryActivationLease.retire).toHaveBeenCalledOnce() + expect(setPtyOwnership).not.toHaveBeenCalled() + expect(deps.mockStore.markSshRemotePtyLeasesAttachedAsync).not.toHaveBeenCalled() + }) + it('settles exact cancellation before publishing an exit with incomplete recovery data', async () => { const targetId = 'exit-with-incomplete-private-body' let settleCancellation: ((proof: unknown) => void) | undefined diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 56d292d78f2..12bf83b7879 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -180,6 +180,11 @@ type PendingPtyReattach = { activated: boolean } +type PendingPtyExitOutcome = + | { status: 'exited'; exit: SshPtyExitPayload } + | { status: 'unverifiable' } + | null + type RemoteCliBridgeEnv = { remoteHome: string binDir: string @@ -1777,10 +1782,11 @@ export class SshRelaySession { this.wakeRecovery(pendingReattach) return } + if (!payload.incarnationId) { + ptyProvider.acceptUnverifiablePty(payload.id) + return + } if (!isCurrentPtyExit(payload)) { - if (!payload.incarnationId) { - ptyProvider.acceptUnverifiablePty(payload.id) - } return } void this.acceptPtyExit(payload).catch(() => {}) @@ -2385,20 +2391,17 @@ export class SshRelaySession { if (!shouldContinue()) { return } - if (!attachResult.incarnationId && pendingReattach.exits.length > 0) { + const exitDuringAttach = this.classifyPendingExit(pendingReattach, attachResult.incarnationId) + if (exitDuringAttach?.status === 'unverifiable') { ptyProvider.acceptUnverifiablePty?.(appPtyId) return } - const exitDuringAttach = this.findExactPendingExit( - pendingReattach, - attachResult.incarnationId - ) - if (exitDuringAttach && !recoveryRequest) { + if (exitDuringAttach?.status === 'exited' && !recoveryRequest) { if (attachResult.incarnationId) { restorePtyIncarnation(appPtyId, attachResult.incarnationId) this.runtime?.acceptPtyIncarnationForExit(appPtyId, attachResult.incarnationId) } - await this.acceptPtyExit(exitDuringAttach) + await this.acceptPtyExit(exitDuringAttach.exit) return } const existingDeliveryConfirmed = @@ -2441,10 +2444,7 @@ export class SshRelaySession { } ) if (!recovered) { - const recoveryExit = this.findExactPendingExit( - pendingReattach, - attachResult.incarnationId - ) + const recoveryExit = this.classifyPendingExit(pendingReattach, attachResult.incarnationId) if ( recoveryExit && shouldContinue() && @@ -2462,18 +2462,26 @@ export class SshRelaySession { ) } } - this.preparePtyIncarnationForExit(appPtyId, attachResult.incarnationId) - await this.acceptPtyExit(recoveryExit) + if (recoveryExit.status === 'unverifiable') { + ptyProvider.acceptUnverifiablePty?.(appPtyId) + } else { + this.preparePtyIncarnationForExit(appPtyId, attachResult.incarnationId) + await this.acceptPtyExit(recoveryExit.exit) + } } return } - const recoveryExit = this.findExactPendingExit(pendingReattach, attachResult.incarnationId) - if (recoveryExit) { + const recoveryExit = this.classifyPendingExit(pendingReattach, attachResult.incarnationId) + if (recoveryExit?.status === 'unverifiable') { + ptyProvider.acceptUnverifiablePty?.(appPtyId) + return + } + if (recoveryExit?.status === 'exited') { this.preparePtyIncarnationForExit(appPtyId, attachResult.incarnationId) pendingReattach.activated = true recoveryActivationLease?.commit() recoveryActivationLease = undefined - await this.acceptPtyExit(recoveryExit) + await this.acceptPtyExit(recoveryExit.exit) return } } @@ -2489,10 +2497,6 @@ export class SshRelaySession { activeLeaseByPtyId.get(ptyId) ) } - attachedLeaseIds.add(ptyId) - pendingReattach.activated = true - recoveryActivationLease?.commit() - recoveryActivationLease = undefined if (targetedDeliveryRecovery) { if (targetedDeliveryRecovery === 'fresh-activation') { this.retiredSourceDeliveries.activate(ptyId) @@ -2503,16 +2507,26 @@ export class SshRelaySession { while (pendingReattach.queuedData.length > 0) { await this.acceptPtyData(pendingReattach.queuedData.shift()!) } - pendingReattach.livePassthrough = true } - const exitAfterActivation = this.findExactPendingExit( + const exitAfterActivation = this.classifyPendingExit( pendingReattach, attachResult.incarnationId ) - if (exitAfterActivation) { - await this.acceptPtyExit(exitAfterActivation) + if (exitAfterActivation?.status === 'unverifiable') { + ptyProvider.acceptUnverifiablePty?.(appPtyId) return } + pendingReattach.activated = true + if (exitAfterActivation?.status === 'exited') { + recoveryActivationLease?.commit() + recoveryActivationLease = undefined + await this.acceptPtyExit(exitAfterActivation.exit) + return + } + attachedLeaseIds.add(ptyId) + recoveryActivationLease?.commit() + recoveryActivationLease = undefined + pendingReattach.livePassthrough = targetedDeliveryRecovery !== undefined if (!recoveryRequest && !targetedDeliveryRecovery) { this.forwardReattachReplay(appPtyId, attachResult.replay ?? '') } @@ -2536,18 +2550,21 @@ export class SshRelaySession { } } - private findExactPendingExit( + private classifyPendingExit( pending: PendingPtyReattach, ptyIncarnation: string | undefined - ): SshPtyExitPayload | undefined { - if (!ptyIncarnation) { - return undefined - } - return pending.exits.find( - (exit) => - exit.providerGeneration === pending.providerGeneration && - exit.ptyIncarnation === ptyIncarnation + ): PendingPtyExitOutcome { + const exits = pending.exits.filter( + (exit) => exit.providerGeneration === pending.providerGeneration ) + if (!ptyIncarnation) { + return exits.length > 0 ? { status: 'unverifiable' } : null + } + const exact = exits.find((exit) => exit.incarnationId === ptyIncarnation) + if (exact) { + return { status: 'exited', exit: exact } + } + return exits.some((exit) => !exit.incarnationId) ? { status: 'unverifiable' } : null } private preparePtyIncarnationForExit(appPtyId: string, ptyIncarnation: string | undefined): void { @@ -2819,8 +2836,8 @@ export class SshRelaySession { this.routeQuarantinedReattachData(pending, payload) } await this.waitForRecoveryFence(pending, shouldContinue) - const exactExit = this.findExactPendingExit(pending, acceptedRecovery.ptyIncarnation) - const complete = pending.recoveryComplete ?? (exactExit ? acceptedRecovery : undefined) + const pendingExit = this.classifyPendingExit(pending, acceptedRecovery.ptyIncarnation) + const complete = pending.recoveryComplete ?? (pendingExit ? acceptedRecovery : undefined) if ( !shouldContinue() || pending.restoreRequired || @@ -2906,7 +2923,7 @@ export class SshRelaySession { shouldContinue() && !pending.recoveryComplete && !pending.restoreRequired && - !this.findExactPendingExit(pending, pending.recovery?.ptyIncarnation) && + !this.classifyPendingExit(pending, pending.recovery?.ptyIncarnation) && Date.now() < deadline ) { await new Promise((resolve) => { @@ -2928,7 +2945,7 @@ export class SshRelaySession { if ( !pending.recoveryComplete && !pending.restoreRequired && - !this.findExactPendingExit(pending, pending.recovery?.ptyIncarnation) + !this.classifyPendingExit(pending, pending.recovery?.ptyIncarnation) ) { pending.restoreRequired = 'recoveryFenceTimeout' }