starred/orca
1
0
Fork 0
mirror of https://github.com/stablyai/orca.git synced 2026-10-24 00:04:01 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(ssh): classify every ambiguous exit race

Browse Source
This commit is contained in:
Brennan Benson
2026-08-16 23:33:07 -07:00
parent cc84b69865
commit bd4c391c68
9 changed files with 102 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/main/providers/ssh-agent-session-create-operation.ts
+2 -2
View File
@@ -87,7 +87,7 @@ export async function spawnFreshSshPty(args: {
) => SshPtyReceivingActivationLease
rememberPtyIncarnation: (relayPtyId: string, incarnationId: unknown) => void
acceptLivePty: (appPtyId: string) => void
acceptUnverifiablePty: (appPtyId: string) => void
acceptAmbiguousExitPty: (appPtyId: string) => void
toAppPtyId: (relayPtyId: string) => string
}): Promise<PtySpawnResult> {
const operation = args.exitRaceTracker.begin()
@@ -118,7 +118,7 @@ export async function spawnFreshSshPty(args: {
}
if (exitOutcome === 'unverifiable') {
const id = args.toAppPtyId(spawnResult.id)
args.acceptUnverifiablePty(id)
args.acceptAmbiguousExitPty(id)
throw Object.assign(new Error(`${SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR}: ${spawnResult.id}`), {
agentSessionOperationOutcome: 'unknown' as const
})
src/main/providers/ssh-pty-provider-exit-race.test.ts
+6
View File
@@ -93,6 +93,7 @@ it('keeps a fresh spawn with a legacy exit unverifiable', async () => {
isDisposed: vi.fn().mockReturnValue(false)
}
const provider = new SshPtyProvider('conn-1', mux as never)
const acceptAmbiguousExitPty = vi.spyOn(provider, 'acceptAmbiguousExitPty')
const exitListener = vi.fn()
provider.onExit(exitListener)
mux.request.mockImplementation(async (method: string, _params, options) => {
@@ -110,6 +111,7 @@ it('keeps a fresh spawn with a legacy exit unverifiable', async () => {
SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR
)
expect(exitListener).not.toHaveBeenCalled()
expect(acceptAmbiguousExitPty).toHaveBeenCalledExactlyOnceWith('ssh:conn-1@@pty-raced')
await expect(provider.probePtyLiveness('ssh:conn-1@@pty-raced')).resolves.toBeNull()
})
@@ -169,6 +171,7 @@ it('keeps a legacy exit unverifiable while reattach resolves its incarnation', a
isDisposed: vi.fn().mockReturnValue(false)
}
const provider = new SshPtyProvider('conn-1', mux as never)
const acceptAmbiguousExitPty = vi.spyOn(provider, 'acceptAmbiguousExitPty')
const exitListener = vi.fn()
provider.onExit(exitListener)
let resolveAttach: (() => void) | undefined
@@ -198,6 +201,7 @@ it('keeps a legacy exit unverifiable while reattach resolves its incarnation', a
await expect(spawn).rejects.toThrow(SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR)
expect(exitListener).not.toHaveBeenCalled()
expect(acceptAmbiguousExitPty).toHaveBeenCalledExactlyOnceWith('ssh:conn-1@@pty-existing')
await expect(provider.probePtyLiveness('ssh:conn-1@@pty-existing')).resolves.toBeNull()
notify?.('pty.exit', {
@@ -223,6 +227,7 @@ it('keeps an incarnation-less attach and exit race unverifiable', async () => {
isDisposed: vi.fn().mockReturnValue(false)
}
const provider = new SshPtyProvider('conn-1', mux as never)
const acceptAmbiguousExitPty = vi.spyOn(provider, 'acceptAmbiguousExitPty')
const exitListener = vi.fn()
provider.onExit(exitListener)
mux.request.mockImplementation(async (method: string, _params, options) => {
@@ -239,6 +244,7 @@ it('keeps an incarnation-less attach and exit race unverifiable', async () => {
provider.spawn({ cols: 80, rows: 24, sessionId: 'ssh:conn-1@@pty-existing' })
).rejects.toThrow(SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR)
expect(exitListener).not.toHaveBeenCalled()
expect(acceptAmbiguousExitPty).toHaveBeenCalledExactlyOnceWith('ssh:conn-1@@pty-existing')
await expect(provider.probePtyLiveness('ssh:conn-1@@pty-existing')).resolves.toBeNull()
})
src/main/providers/ssh-pty-provider.ts
+2 -1
View File
@@ -102,6 +102,7 @@ export class SshPtyProvider implements IPtyProvider {
this.outputState.acceptPtyIncarnation(relayPtyId, incarnationId),
acceptLivePty: (relayPtyId) => this.acceptLivePty(relayPtyId),
acceptUnverifiablePty: (relayPtyId) => this.acceptUnverifiablePty(relayPtyId),
acceptAmbiguousExitPty: (relayPtyId) => this.acceptAmbiguousExitPty(relayPtyId),
acceptExitedPty: (relayPtyId) => this.acceptExitedPty(relayPtyId)
})
}
@@ -130,7 +131,7 @@ export class SshPtyProvider implements IPtyProvider {
rememberPtyIncarnation: (id, incarnation) =>
this.outputState.rememberPtyIncarnation(id, incarnation),
acceptLivePty: (id) => this.acceptLivePty(id),
acceptUnverifiablePty: (id) => this.acceptUnverifiablePty(id),
acceptAmbiguousExitPty: (id) => this.acceptAmbiguousExitPty(id),
toAppPtyId: this.toAppPtyId
})
}
src/main/providers/ssh-pty-session-reattach.ts
+10 -47
View File
@@ -23,6 +23,7 @@ import {
type PtySourceReceivingActivation
} from '../../shared/pty-source-receiving-activation'
import type { SshPtyReceivingActivationLease } from './ssh-pty-notification-routing'
import { parseSshPtySourceRecoveryResult } from './ssh-pty-source-recovery-result'
export type SshPtyAttachResult = {
replay?: string
@@ -57,7 +58,7 @@ export function parseSshPtyAttachResult(value: unknown): SshPtyAttachResult {
// Why: a present-but-invalid identity cannot safely fence delayed exits from a reused relay id.
throw new Error('Invalid SSH PTY attach incarnation')
}
const sourceRecovery = parseSourceRecoveryResult(result.sourceRecovery)
const sourceRecovery = parseSshPtySourceRecoveryResult(result.sourceRecovery)
const sourceActivation = parsePtySourceReceivingActivation(result.sourceActivation)
const activation =
sourceActivation ?? (sourceRecovery?.status === 'pending' ? sourceRecovery : undefined)
@@ -119,50 +120,6 @@ export async function requestSshPtyAttach(args: {
}
}
function parseSourceRecoveryResult(value: unknown): PtySourceRecoveryResult | undefined {
if (value === undefined) {
return undefined
}
if (typeof value !== 'object' || value === null || Array.isArray(value)) {
throw new Error('Invalid SSH PTY source recovery response')
}
const input = value as Record<string, unknown>
if (input.status === 'restoreRequired' && typeof input.reason === 'string') {
return Object.freeze({ status: 'restoreRequired', reason: input.reason })
}
if (
input.status !== 'pending' ||
typeof input.deliveryToken !== 'string' ||
input.deliveryToken.length === 0 ||
typeof input.ptyIncarnation !== 'string' ||
input.ptyIncarnation.length === 0 ||
!positiveInteger(input.clientGeneration) ||
!positiveInteger(input.ownerGeneration) ||
!nonNegativeInteger(input.checkpointSourceEndSu) ||
!nonNegativeInteger(input.recoveryEndSu) ||
Number(input.recoveryEndSu) < Number(input.checkpointSourceEndSu)
) {
throw new Error('Invalid SSH PTY source recovery response')
}
return Object.freeze({
status: 'pending',
deliveryToken: input.deliveryToken,
ptyIncarnation: input.ptyIncarnation,
clientGeneration: Number(input.clientGeneration),
ownerGeneration: Number(input.ownerGeneration),
checkpointSourceEndSu: Number(input.checkpointSourceEndSu),
recoveryEndSu: Number(input.recoveryEndSu)
})
}
function positiveInteger(value: unknown): boolean {
return Number.isSafeInteger(value) && Number(value) > 0
}
function nonNegativeInteger(value: unknown): boolean {
return Number.isSafeInteger(value) && Number(value) >= 0
}
function sameSourceActivation(
left: PtySourceReceivingActivation,
right: PtySourceReceivingActivation
@@ -270,6 +227,7 @@ export async function reattachSshPtySessionForSpawn(
args: Parameters<typeof reattachSshPtySessionWithExitFence>[0] & {
acceptLivePty: (relayPtyId: string) => void
acceptUnverifiablePty: (relayPtyId: string) => void
acceptAmbiguousExitPty: (relayPtyId: string) => void
acceptExitedPty: (relayPtyId: string) => void
}
): Promise<PtySpawnResult> {
@@ -301,8 +259,13 @@ export async function reattachSshPtySessionForSpawn(
args.acceptExitedPty(result?.id ?? toAppSshPtyId(args.connectionId, args.sessionId))
} else if (!isSshPtyIdentityMismatchError(error) && !isSshPtyRestoreRequiredError(error)) {
const id = result?.id ?? toAppSshPtyId(args.connectionId, args.sessionId)
args.acceptUnverifiablePty(id)
if (!isSshPtyLivenessUnverifiableError(error)) {
const ambiguousExit = isSshPtyLivenessUnverifiableError(error)
if (ambiguousExit) {
args.acceptAmbiguousExitPty(id)
} else {
args.acceptUnverifiablePty(id)
}
if (!ambiguousExit) {
throw new Error(
`${SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR}: ${toRelaySshPtyId(args.connectionId, id)}`,
{ cause: error }
src/main/providers/ssh-pty-source-recovery-result.ts
+47
View File
@@ -0,0 +1,47 @@
import type { PtySourceRecoveryResult } from '../../shared/pty-source-recovery-contract'
export function parseSshPtySourceRecoveryResult(
value: unknown
): PtySourceRecoveryResult | undefined {
if (value === undefined) {
return undefined
}
if (typeof value !== 'object' || value === null || Array.isArray(value)) {
throw new Error('Invalid SSH PTY source recovery response')
}
const input = value as Record<string, unknown>
if (input.status === 'restoreRequired' && typeof input.reason === 'string') {
return Object.freeze({ status: 'restoreRequired', reason: input.reason })
}
if (
input.status !== 'pending' ||
typeof input.deliveryToken !== 'string' ||
input.deliveryToken.length === 0 ||
typeof input.ptyIncarnation !== 'string' ||
input.ptyIncarnation.length === 0 ||
!positiveInteger(input.clientGeneration) ||
!positiveInteger(input.ownerGeneration) ||
!nonNegativeInteger(input.checkpointSourceEndSu) ||
!nonNegativeInteger(input.recoveryEndSu) ||
Number(input.recoveryEndSu) < Number(input.checkpointSourceEndSu)
) {
throw new Error('Invalid SSH PTY source recovery response')
}
return Object.freeze({
status: 'pending',
deliveryToken: input.deliveryToken,
ptyIncarnation: input.ptyIncarnation,
clientGeneration: Number(input.clientGeneration),
ownerGeneration: Number(input.ownerGeneration),
checkpointSourceEndSu: Number(input.checkpointSourceEndSu),
recoveryEndSu: Number(input.recoveryEndSu)
})
}
function positiveInteger(value: unknown): boolean {
return Number.isSafeInteger(value) && Number(value) > 0
}
function nonNegativeInteger(value: unknown): boolean {
return Number.isSafeInteger(value) && Number(value) >= 0
}
src/main/ssh/ssh-relay-session-incarnation.test.ts
+6 -6
View File
@@ -307,7 +307,7 @@ describe('SSH relay PTY incarnation exits', () => {
onData: ReturnType<typeof vi.fn>
onExit: ReturnType<typeof vi.fn>
acceptLivePty: ReturnType<typeof vi.fn>
acceptUnverifiablePty: ReturnType<typeof vi.fn>
acceptAmbiguousExitPty: ReturnType<typeof vi.fn>
settleLivePtyEvidence: ReturnType<typeof vi.fn>
}
vi.mocked(getSshPtyProvider).mockReturnValue(provider as never)
@@ -340,7 +340,7 @@ describe('SSH relay PTY incarnation exits', () => {
settleOutput()
await vi.waitFor(() => expect(provider.settleLivePtyEvidence).toHaveBeenCalledOnce())
expect(provider.acceptUnverifiablePty).toHaveBeenCalledExactlyOnceWith(id)
expect(provider.acceptAmbiguousExitPty).toHaveBeenCalledExactlyOnceWith(id)
expect(provider.acceptLivePty).not.toHaveBeenCalled()
})
@@ -350,7 +350,7 @@ describe('SSH relay PTY incarnation exits', () => {
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>
acceptAmbiguousExitPty: ReturnType<typeof vi.fn>
}
const onExit = provider.onExit.mock.calls[0]?.[0] as (payload: {
id: string
@@ -367,7 +367,7 @@ describe('SSH relay PTY incarnation exits', () => {
ptyIncarnation: 'incarnation-current'
})
expect(provider.acceptUnverifiablePty).toHaveBeenCalledExactlyOnceWith(
expect(provider.acceptAmbiguousExitPty).toHaveBeenCalledExactlyOnceWith(
'ssh:target-1@@pty-current'
)
expect(acceptOutputExitMock).not.toHaveBeenCalled()
@@ -379,7 +379,7 @@ describe('SSH relay PTY incarnation exits', () => {
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>
acceptAmbiguousExitPty: ReturnType<typeof vi.fn>
}
const onExit = provider.onExit.mock.calls[0]?.[0] as (payload: {
id: string
@@ -395,7 +395,7 @@ describe('SSH relay PTY incarnation exits', () => {
ptyIncarnation: 'legacy:31:1:pty-legacy'
})
expect(provider.acceptUnverifiablePty).toHaveBeenCalledExactlyOnceWith(
expect(provider.acceptAmbiguousExitPty).toHaveBeenCalledExactlyOnceWith(
'ssh:target-1@@pty-legacy'
)
expect(acceptOutputExitMock).not.toHaveBeenCalled()
src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts
+10 -4
View File
@@ -108,12 +108,13 @@ vi.mock('../providers/ssh-pty-provider', () => ({
}
}
)
acceptUnverifiablePty = vi.fn(() => {
acceptUnverifiablePty = vi.fn((_id: string) => {
for (const evidence of this.pendingLiveEvidence) {
evidence.valid = false
}
this.pendingLiveEvidence.clear()
})
acceptAmbiguousExitPty = vi.fn((id: string) => this.acceptUnverifiablePty(id))
acceptExitedPty = vi.fn()
dispose = vi.fn()
@@ -636,6 +637,7 @@ describe('SshRelaySession reconnect incarnation ordering', () => {
const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps()
const sourceActivationLease = { commit: vi.fn(), rollback: vi.fn() }
const acceptUnverifiablePty = vi.fn()
const acceptAmbiguousExitPty = vi.fn()
const acceptExitedPty = vi.fn()
vi.mocked(getSshPtyProvider).mockReturnValue({
attachForReconnect: vi.fn().mockImplementation(async () => {
@@ -643,6 +645,7 @@ describe('SshRelaySession reconnect incarnation ordering', () => {
return { sourceActivationLease }
}),
acceptUnverifiablePty,
acceptAmbiguousExitPty,
acceptExitedPty,
dispose: vi.fn()
} as unknown as ReturnType<typeof getSshPtyProvider>)
@@ -653,8 +656,8 @@ describe('SshRelaySession reconnect incarnation ordering', () => {
await session.establish(mockConn)
expect(acceptUnverifiablePty).toHaveBeenCalledOnce()
expect(acceptUnverifiablePty).toHaveBeenCalledWith(APP_PTY_ID)
expect(acceptAmbiguousExitPty).toHaveBeenCalledExactlyOnceWith(APP_PTY_ID)
expect(acceptUnverifiablePty).not.toHaveBeenCalled()
expect(acceptExitedPty).not.toHaveBeenCalled()
expect(acceptOutputExitMock).not.toHaveBeenCalled()
expect(setPtyOwnership).not.toHaveBeenCalled()
@@ -720,6 +723,7 @@ describe('SshRelaySession reconnect incarnation ordering', () => {
const { mockConn, mockStore, mockPortForward, getMainWindow, mockWindow } = createMockDeps()
const sourceActivationLease = { commit: vi.fn(), rollback: vi.fn() }
const acceptUnverifiablePty = vi.fn()
const acceptAmbiguousExitPty = vi.fn()
const acceptExitedPty = vi.fn()
vi.mocked(getSshPtyProvider).mockReturnValue({
attachForReconnect: vi.fn().mockImplementation(async () => {
@@ -731,6 +735,7 @@ describe('SshRelaySession reconnect incarnation ordering', () => {
}
}),
acceptUnverifiablePty,
acceptAmbiguousExitPty,
acceptExitedPty,
dispose: vi.fn()
} as unknown as ReturnType<typeof getSshPtyProvider>)
@@ -741,7 +746,8 @@ describe('SshRelaySession reconnect incarnation ordering', () => {
await session.establish(mockConn)
expect(acceptUnverifiablePty).toHaveBeenCalledExactlyOnceWith(APP_PTY_ID)
expect(acceptAmbiguousExitPty).toHaveBeenCalledExactlyOnceWith(APP_PTY_ID)
expect(acceptUnverifiablePty).not.toHaveBeenCalled()
expect(acceptExitedPty).not.toHaveBeenCalled()
expect(acceptOutputExitMock).not.toHaveBeenCalled()
expect(setPtyOwnership).not.toHaveBeenCalled()
src/main/ssh/ssh-relay-session-recovery-races.test.ts
+8 -4
View File
@@ -76,7 +76,9 @@ vi.mock('../providers/ssh-pty-provider', () => ({
return () => {}
})
attachForReconnect = attachForReconnectMock
acceptLivePty = vi.fn()
acceptUnverifiablePty = vi.fn()
acceptAmbiguousExitPty = vi.fn((id: string) => this.acceptUnverifiablePty(id))
setPtyDeliveryPauseAdapter = vi.fn()
dispose = ptyProviderDisposeMock
}
@@ -313,10 +315,10 @@ describe('SshRelaySession recovery race fencing', () => {
await session.reconnect(deps.mockConn)
const provider = vi.mocked(registerSshPtyProvider).mock.calls.at(-1)?.[1] as unknown as {
acceptUnverifiablePty: ReturnType<typeof vi.fn>
acceptAmbiguousExitPty: ReturnType<typeof vi.fn>
}
expect(provider.acceptUnverifiablePty).toHaveBeenCalledWith(`ssh:${targetId}@@pty-1`)
expect(provider.acceptAmbiguousExitPty).toHaveBeenCalledWith(`ssh:${targetId}@@pty-1`)
expect(acceptOutputExitMock).not.toHaveBeenCalled()
expect(recoveryActivationLease.commit).not.toHaveBeenCalled()
expect(recoveryActivationLease.retire).toHaveBeenCalledOnce()
@@ -732,7 +734,6 @@ describe('SshRelaySession recovery race fencing', () => {
return { incarnationId: 'incarnation-1', sourceRecovery: pendingRecovery(4) }
})
await session.reconnect(deps.mockConn)
expect(acceptOutputDataMock).toHaveBeenCalledOnce()
// The pre-migration identity is gone from the intake, so only the checkpoint
// recorded after recovery can answer the next reconnect.
@@ -743,12 +744,15 @@ describe('SshRelaySession recovery race fencing', () => {
})
await session.reconnect(deps.mockConn)
expect(attachForReconnectMock).toHaveBeenCalledTimes(2)
expect(attachForReconnectMock.mock.calls.at(-1)?.[2]).toMatchObject({
status: 'checkpoint',
deliveryToken: 'new-token',
acceptedSourceEndSu: 8
})
const provider = vi.mocked(registerSshPtyProvider).mock.calls.at(-1)?.[1] as unknown as {
acceptLivePty: ReturnType<typeof vi.fn>
}
expect(provider.acceptLivePty).toHaveBeenCalledExactlyOnceWith(`ssh:${targetId}@@pty-1`)
})
it('keeps a stale overlapping recovery from canceling or mutating its replacement', async () => {
src/main/ssh/ssh-relay-session.ts
+11 -4
View File
@@ -2382,6 +2382,7 @@ export class SshRelaySession {
targetedDeliveryRecovery
} = args
const appPtyId = toAppSshPtyId(this.targetId, ptyId)
const acceptAmbiguousExit = (): void => ptyProvider.acceptAmbiguousExitPty?.(appPtyId)
const pendingReattach: PendingPtyReattach = {
mux,
providerGeneration,
@@ -2415,7 +2416,7 @@ export class SshRelaySession {
}
const exitDuringAttach = this.classifyPendingExit(pendingReattach, attachResult.incarnationId)
if (exitDuringAttach?.status === 'unverifiable') {
ptyProvider.acceptUnverifiablePty?.(appPtyId)
acceptAmbiguousExit()
return
}
if (exitDuringAttach?.status === 'exited' && !recoveryRequest) {
@@ -2485,17 +2486,23 @@ export class SshRelaySession {
}
}
if (recoveryExit.status === 'unverifiable') {
ptyProvider.acceptUnverifiablePty?.(appPtyId)
acceptAmbiguousExit()
} else {
this.preparePtyIncarnationForExit(appPtyId, attachResult.incarnationId)
await this.acceptPtyExit(recoveryExit.exit)
}
} else if (
attachResult.sourceRecovery?.status === 'restoreRequired' &&
shouldContinue() &&
this.ownsPtyRecoveryAttempt(appPtyId, pendingReattach)
) {
ptyProvider.acceptLivePty?.(appPtyId)
}
return
}
const recoveryExit = this.classifyPendingExit(pendingReattach, attachResult.incarnationId)
if (recoveryExit?.status === 'unverifiable') {
ptyProvider.acceptUnverifiablePty?.(appPtyId)
acceptAmbiguousExit()
return
}
if (recoveryExit?.status === 'exited') {
@@ -2535,7 +2542,7 @@ export class SshRelaySession {
attachResult.incarnationId
)
if (exitAfterActivation?.status === 'unverifiable') {
ptyProvider.acceptUnverifiablePty?.(appPtyId)
acceptAmbiguousExit()
return
}
pendingReattach.activated = true