fix(ssh): keep ambiguous exit races unverifiable

This commit is contained in:
Brennan Benson
2026-08-16 22:43:25 -07:00
parent 2ed57ae61b
commit 1fd2fe8639
9 changed files with 212 additions and 59 deletions
@@ -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<PtySpawnResult> {
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)
@@ -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(),
+1
View File
@@ -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
})
}
@@ -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
})
+5 -14
View File
@@ -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 {
@@ -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<typeof vi.fn>
acceptUnverifiablePty: ReturnType<typeof vi.fn>
}
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()
})
})
@@ -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<typeof getSshPtyProvider>)
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'
@@ -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<typeof vi.fn>
}
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
+57 -40
View File
@@ -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<void>((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'
}