fix(ssh): preserve liveness evidence across teardown

This commit is contained in:
Brennan Benson
2026-08-16 19:37:22 -07:00
parent d4a0e7f1af
commit 2ada9c07f9
10 changed files with 209 additions and 35 deletions
@@ -153,7 +153,7 @@ describe('registerPtyHandlers', () => {
})
expect(runtime.onPtyExit).not.toHaveBeenCalled()
})
it('does not accept an incarnation-less exit as proof that the current PTY stopped', async () => {
it('uses inventory, not an incarnation-less exit, to confirm the current PTY stopped', async () => {
const exitListeners = new Set<
(payload: { id: string; code: number; incarnationId?: string }) => void
>()
@@ -203,7 +203,7 @@ describe('registerPtyHandlers', () => {
await controller.spawn({ cols: 80, rows: 24 })
await expect(controller.stopAndWait('local-incarnated')).resolves.toBe(true)
expect(runtime.onPtyExit).toHaveBeenCalledWith('local-incarnated', -1, 'incarnation-live')
expect(runtime.onPtyExit).toHaveBeenCalledWith('local-incarnated', 0, 'incarnation-live')
})
it('runtime controller kill routes app-scoped SSH ids through the parsed provider when ownership is absent', async () => {
const localShutdown = vi.fn()
@@ -358,14 +358,15 @@ describe('registerPtyHandlers', () => {
)
expect(runtime.onPtyExit).toHaveBeenCalledWith('remote-pty', -1, undefined)
})
it('retires a rejected SSH PTY after generic kill shutdown fails transiently', async () => {
it('keeps a rejected SSH PTY unverifiable after kill shutdown fails transiently', async () => {
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
const store = {
markSshRemotePtyLease: vi.fn()
}
const runtime = {
setPtyController: vi.fn(),
onPtyExit: vi.fn()
onPtyExit: vi.fn(),
markPtyLivenessUnverifiable: vi.fn()
}
registerSshPtyProvider('ssh-1', {
spawn: vi.fn(),
@@ -401,7 +402,7 @@ describe('registerPtyHandlers', () => {
)
const controller = runtime.setPtyController.mock.calls[0]?.[0] as {
kill: (ptyId: string) => boolean
retireRejectedPty: (ptyId: string) => void
retireRejectedPty: (ptyId: string, stopConfirmed: boolean) => void
}
try {
@@ -412,19 +413,23 @@ describe('registerPtyHandlers', () => {
'remote-pty',
'terminated'
)
controller.retireRejectedPty('remote-pty')
controller.retireRejectedPty('remote-pty', false)
} finally {
warnSpy.mockRestore()
deletePtyOwnership('remote-pty')
}
expect(store.markSshRemotePtyLease).toHaveBeenCalledWith(
expect(store.markSshRemotePtyLease).not.toHaveBeenCalledWith(
'ssh-1',
'remote-pty',
'terminated'
)
expect(runtime.markPtyLivenessUnverifiable).toHaveBeenCalledWith(
'remote-pty',
'a follow-up stop was issued but its outcome could not be verified'
)
expect(runtime.onPtyExit).toHaveBeenCalledWith('remote-pty', -1, undefined)
expect(runtime.onPtyExit).toHaveBeenCalledWith('remote-pty', 0, undefined)
expect(runtime.onPtyExit).not.toHaveBeenCalledWith('remote-pty', 0, undefined)
})
it('strips ORCA_PANE_KEY/TAB_ID/WORKTREE_ID from SSH spawn env when remote agent hooks are disabled', async () => {
const sshSpawn = vi.fn(async (_opts: { env: Record<string, string> }) => ({
+79
View File
@@ -62,6 +62,8 @@ describe('stopping a PTY whose SSH provider is unregistered', () => {
function installController(): {
controller: {
kill: (ptyId: string) => boolean
listProcesses: (connectionId?: string | null) => Promise<{ id: string }[]>
retireRejectedPty: (ptyId: string, stopConfirmed: boolean) => void
stopAndWait: (ptyId: string, opts?: { deadlineMs?: number }) => Promise<boolean>
}
runtime: {
@@ -129,6 +131,24 @@ describe('stopping a PTY whose SSH provider is unregistered', () => {
}
})
it('retires a rejected split without asserting an unconfirmed exit', () => {
const ptyId = 'ssh-rejected-split'
setPtyOwnership(ptyId, 'ssh-dropped')
const { controller, runtime } = installController()
try {
controller.retireRejectedPty(ptyId, false)
expect(runtime.markPtyLivenessUnverifiable).toHaveBeenCalledWith(
ptyId,
'a follow-up stop was issued but its outcome could not be verified'
)
expect(runtime.onPtyExit).toHaveBeenCalledWith(ptyId, -1, undefined)
expect(runtime.onPtyExit).not.toHaveBeenCalledWith(ptyId, 0, expect.anything())
} finally {
deletePtyOwnership(ptyId)
}
})
it('still confirms a stop the owning provider actually performed', async () => {
const daemon = installObservableDaemonTestProvider()
vi.spyOn(getLocalPtyProvider(), 'listProcesses').mockResolvedValue([])
@@ -142,6 +162,28 @@ describe('stopping a PTY whose SSH provider is unregistered', () => {
expect(runtime.markPtyLivenessUnverifiable).not.toHaveBeenCalled()
})
it('records provider-confirmed absence when the exit event was missed', async () => {
const connectionId = 'ssh-confirmed-absent'
const ptyId = 'ssh-confirmed-absent-pty'
const provider = {
onExit: vi.fn(() => () => {}),
shutdown: vi.fn(async () => {}),
listProcesses: vi.fn(async () => [])
}
registerSshPtyProvider(connectionId, provider as never)
setPtyOwnership(ptyId, connectionId)
try {
const { controller, runtime } = installController()
await expect(controller.stopAndWait(ptyId)).resolves.toBe(true)
expect(runtime.onPtyExit).toHaveBeenCalledWith(ptyId, 0, undefined)
expect(runtime.markPtyLivenessUnverifiable).not.toHaveBeenCalled()
} finally {
deletePtyOwnership(ptyId)
unregisterSshPtyProvider(connectionId)
}
})
it('reports lost contact when a registered provider drops during the stop', async () => {
const connectionId = 'ssh-mid-stop-drop'
const ptyId = 'ssh-mid-stop-pty'
@@ -195,6 +237,43 @@ describe('stopping a PTY whose SSH provider is unregistered', () => {
}
})
it('isolates a failed SSH inventory from healthy providers', async () => {
const failedConnectionId = 'ssh-inventory-failed'
const healthyConnectionId = 'ssh-inventory-healthy'
const failedPtyId = 'ssh-inventory-failed-pty'
const healthyPtyId = 'ssh-inventory-healthy-pty'
registerSshPtyProvider(failedConnectionId, {
listProcesses: vi.fn(async () => {
throw new Error('inventory transport failed')
})
} as never)
registerSshPtyProvider(healthyConnectionId, {
listProcesses: vi.fn(async () => [{ id: healthyPtyId }])
} as never)
setPtyOwnership(failedPtyId, failedConnectionId)
setPtyOwnership(healthyPtyId, healthyConnectionId)
try {
const { controller, runtime } = installController()
await expect(controller.listProcesses()).resolves.toEqual(
expect.arrayContaining([{ id: healthyPtyId }])
)
expect(runtime.markPtyLivenessUnverifiable).toHaveBeenCalledWith(
failedPtyId,
'inventory transport failed'
)
expect(runtime.markPtyLivenessUnverifiable).not.toHaveBeenCalledWith(
healthyPtyId,
expect.anything()
)
} finally {
deletePtyOwnership(failedPtyId)
deletePtyOwnership(healthyPtyId)
unregisterSshPtyProvider(failedConnectionId)
unregisterSshPtyProvider(healthyConnectionId)
}
})
it('reports a provider-observed survivor as live', async () => {
const connectionId = 'ssh-still-live'
const ptyId = 'ssh-still-live-pty'
+42 -7
View File
@@ -4445,6 +4445,15 @@ export function registerPtyHandlers(
}
}
const markSshInventoryUnverifiable = (connectionId: string, error: unknown): void => {
const reason = error instanceof Error ? error.message : String(error)
for (const [ptyId, ownerConnectionId] of ptyOwnership) {
if (ownerConnectionId === connectionId) {
runtime?.markPtyLivenessUnverifiable?.(ptyId, reason)
}
}
}
// Why: route through getProviderForPty() so CLI commands work for remote PTYs too; localProvider would silently fail for them.
runtime?.setPtyController({
claimStablePaneCreate: (args) => {
@@ -5591,11 +5600,23 @@ export function registerPtyHandlers(
}
return killWithCurrentProvider()
},
retireRejectedPty: (ptyId) => {
retireRejectedPty: (ptyId, stopConfirmed) => {
rememberRetiredRejectedPty(ptyId)
if (!stopConfirmed) {
runtime?.markPtyLivenessUnverifiable?.(
ptyId,
'a follow-up stop was issued but its outcome could not be verified'
)
if (!ptyOwnership.has(ptyId)) {
return
}
runtime?.onPtyExit(ptyId, -1, ptyIncarnationById.get(ptyId))
rememberSyntheticKillExit(ptyId)
sendPtyExitToRenderer({ id: ptyId, code: -1 })
return
}
// Why: a completed stop already cleared provider state, tombstoned the lease and told the
// renderer; repeating that double-fires the exit IPC. The runtime still needs code 0 so an
// SSH pane retires for good instead of staying preserved by the stop's negative exit.
// renderer; repeating that double-fires the exit IPC.
if (!ptyOwnership.has(ptyId)) {
runtime?.onPtyExit(ptyId, 0, ptyIncarnationById.get(ptyId))
return
@@ -5717,9 +5738,11 @@ export function registerPtyHandlers(
}
const incarnationId = finishPtyShutdown(ptyId, connectionId, store)
if (!providerExitObserved) {
runtime?.onPtyExit(ptyId, -1, incarnationId)
// The owning provider's fresh inventory observed absence, so this is a
// death certificate even when its exit event was missed.
runtime?.onPtyExit(ptyId, 0, incarnationId)
rememberSyntheticKillExit(ptyId)
sendPtyExitToRenderer({ id: ptyId, code: -1 })
sendPtyExitToRenderer({ id: ptyId, code: 0 })
}
return true
},
@@ -5776,11 +5799,23 @@ export function registerPtyHandlers(
return localProvider.listProcesses()
}
if (connectionId !== undefined) {
return getProvider(connectionId).listProcesses()
try {
return await getProvider(connectionId).listProcesses()
} catch (error) {
markSshInventoryUnverifiable(connectionId, error)
throw error
}
}
const providerSessions = await Promise.all([
localProvider.listProcesses(),
...Array.from(sshProviders.values(), (provider) => provider.listProcesses())
...Array.from(sshProviders, async ([sshConnectionId, provider]) => {
try {
return await provider.listProcesses()
} catch (error) {
markSshInventoryUnverifiable(sshConnectionId, error)
return []
}
})
])
return providerSessions.flat()
},
@@ -35,12 +35,12 @@ describe('terminal process incarnation liveness', () => {
).resolves.toBe('live')
await expect(
runtime.inspectTerminalProcessIncarnationLiveness('remote:ssh-1:pty-1:inc-old', SSH_SCOPE)
).resolves.toBe('dead')
).resolves.toBe('exited')
expect(listProcesses).toHaveBeenNthCalledWith(1, 'ssh-1')
expect(listProcesses).toHaveBeenNthCalledWith(2, 'ssh-1')
})
it('keeps missing or malformed identity and unavailable inventory unknown', async () => {
it('keeps missing or malformed identity and unavailable inventory unverifiable', async () => {
const listProcesses = vi
.fn()
.mockResolvedValueOnce([{ id: 'remote:ssh-1:pty-1', cwd: '', title: 'worker' }])
@@ -52,13 +52,13 @@ describe('terminal process incarnation liveness', () => {
await expect(
runtime.inspectTerminalProcessIncarnationLiveness(PROCESS_INCARNATION, SSH_SCOPE)
).resolves.toBe('unknown')
).resolves.toBe('unverifiable')
await expect(
runtime.inspectTerminalProcessIncarnationLiveness(PROCESS_INCARNATION, SSH_SCOPE)
).resolves.toBe('unknown')
).resolves.toBe('unverifiable')
await expect(
runtime.inspectTerminalProcessIncarnationLiveness(PROCESS_INCARNATION, SSH_SCOPE)
).resolves.toBe('unknown')
).resolves.toBe('unverifiable')
})
it('does not inspect an unproven host scope', async () => {
@@ -67,10 +67,10 @@ describe('terminal process incarnation liveness', () => {
await expect(
runtime.inspectTerminalProcessIncarnationLiveness(PROCESS_INCARNATION, null)
).resolves.toBe('unknown')
).resolves.toBe('unverifiable')
await expect(
runtime.inspectTerminalProcessIncarnationLiveness(PROCESS_INCARNATION, '{"kind":"ssh"}')
).resolves.toBe('unknown')
).resolves.toBe('unverifiable')
expect(listProcesses).not.toHaveBeenCalled()
})
@@ -83,7 +83,7 @@ describe('terminal process incarnation liveness', () => {
await expect(
runtime.inspectTerminalProcessIncarnationLiveness('local-pty:inc-1', JSON.stringify(scope))
).resolves.toBe('dead')
).resolves.toBe('exited')
expect(listProcesses).toHaveBeenCalledWith(connectionId)
})
})
@@ -348,7 +348,7 @@ describe('remote runtime terminal split authority', () => {
expect.objectContaining({ deadlineMs: expect.any(Number) })
)
expect(harness.kill).not.toHaveBeenCalled()
expect(harness.retireRejectedPty).toHaveBeenCalledWith(SPLIT_PTY_ID)
expect(harness.retireRejectedPty).toHaveBeenCalledWith(SPLIT_PTY_ID, true)
})
it('revalidates a projected paired-runtime source after renderer adoption', async () => {
@@ -374,7 +374,7 @@ describe('remote runtime terminal split authority', () => {
expect.objectContaining({ deadlineMs: expect.any(Number) })
)
expect(harness.kill).toHaveBeenCalledWith(SPLIT_PTY_ID)
expect(harness.retireRejectedPty).toHaveBeenCalledWith(SPLIT_PTY_ID)
expect(harness.retireRejectedPty).toHaveBeenCalledWith(SPLIT_PTY_ID, false)
})
it('preserves the split error when kill and retirement throw', async () => {
@@ -398,6 +398,6 @@ describe('remote runtime terminal split authority', () => {
await expect(split).rejects.toThrow('terminal_split_source_not_found')
expect(harness.kill).toHaveBeenCalledWith(SPLIT_PTY_ID)
expect(harness.retireRejectedPty).toHaveBeenCalledWith(SPLIT_PTY_ID)
expect(harness.retireRejectedPty).toHaveBeenCalledWith(SPLIT_PTY_ID, false)
})
})
+18 -5
View File
@@ -659,6 +659,7 @@ import {
import { retireTerminalSurfaceFromPersistence } from './mobile-session-terminal-persistence-retirement'
import {
NO_OBSERVING_PROVIDER_REASON,
SSH_EXIT_UNCONFIRMED_REASON,
SSH_PROVIDER_UNREGISTERED_REASON,
type PtyLivenessVerdict
} from '../../shared/pty-liveness-verdict'
@@ -1863,7 +1864,7 @@ type RuntimePtyController = {
* False on doubt (absent session, SSH-scoped id, non-daemon provider). */
attach?(ptyId: string): Promise<boolean>
kill(ptyId: string): boolean
retireRejectedPty?(ptyId: string): void
retireRejectedPty?(ptyId: string, stopConfirmed: boolean): void
stopAndWait?(
ptyId: string,
opts?: { keepHistory?: boolean; deadlineMs?: number }
@@ -14719,6 +14720,9 @@ export class OrcaRuntimeService {
const preservesAbnormalSshSurface =
this.isSshOwnedPtyId(ptyId) && pty?.connectionId != null && exitCode < 0
if (preservesAbnormalSshSurface) {
if (this.getPtyLivenessVerdict(ptyId)?.status !== 'unverifiable') {
this.markPtyLivenessUnverifiable(ptyId, SSH_EXIT_UNCONFIRMED_REASON)
}
this.restoredOrchestrationAuthorityByPtyId.delete(ptyId)
} else {
this.retirePtyAgentLaunchAuthority(ptyId)
@@ -16686,17 +16690,17 @@ export class OrcaRuntimeService {
async inspectTerminalProcessIncarnationLiveness(
processIncarnation: string,
serializedHostScope: string | null
): Promise<'live' | 'dead' | 'unknown'> {
): Promise<'live' | 'exited' | 'unverifiable'> {
const hostScope = parseWorkerTerminalHostScope(serializedHostScope)
if (!hostScope || !this.ptyController?.listProcesses) {
return 'unknown'
return 'unverifiable'
}
const listed = await withTimeoutResult(
this.ptyController.listProcesses(hostScope.kind === 'ssh' ? hostScope.targetId : null),
PTY_CONTROLLER_LIST_TIMEOUT_MS
)
if (!listed.ok) {
return 'unknown'
return 'unverifiable'
}
return classifyWorkerTerminalProcessIncarnation(processIncarnation, listed.value)
}
@@ -29058,7 +29062,7 @@ export class OrcaRuntimeService {
}
}
try {
this.ptyController.retireRejectedPty?.(result.id)
this.ptyController.retireRejectedPty?.(result.id, stopped)
} catch {
// Best-effort cleanup; preserve the original split authority error.
}
@@ -31566,6 +31570,15 @@ export class OrcaRuntimeService {
continue
}
if (!allLivePtyIds.has(pty.ptyId) && !this.leafExistsForPty(pty.ptyId)) {
const currentVerdict = this.ptyLivenessVerdictByPtyId.get(pty.ptyId)
if (
currentVerdict?.observedAt > livenessObservationAtStart &&
currentVerdict.verdict.status === 'unverifiable'
) {
pty.connected = false
pty.disconnectedAt ??= Date.now()
continue
}
const observed = this.ptyController.hasPty?.(pty.ptyId)
if (observed === true) {
// Why: an SSH spawn can become addressable before an overlapping relay list includes it.
@@ -39,7 +39,7 @@ export function parseWorkerTerminalHostScope(value: string | null): WorkerTermin
export function classifyWorkerTerminalProcessIncarnation(
processIncarnation: string,
sessions: readonly PtyProcessInfo[]
): 'live' | 'dead' | 'unknown' {
): 'live' | 'exited' | 'unverifiable' {
const possibleMatches = sessions.filter((session) =>
processIncarnation.startsWith(`${session.id}:`)
)
@@ -57,6 +57,6 @@ export function classifyWorkerTerminalProcessIncarnation(
return possibleMatches.some(
(session) => !session.incarnationId || session.incarnationId !== session.incarnationId.trim()
)
? 'unknown'
: 'dead'
? 'unverifiable'
: 'exited'
}
@@ -1,6 +1,7 @@
import { describe, expect, it, vi } from 'vitest'
import { OrcaRuntimeService } from './orca-runtime'
import { getDefaultWorkspaceSession } from '../../shared/constants'
import { SSH_EXIT_UNCONFIRMED_REASON } from '../../shared/pty-liveness-verdict'
// The aggregate inventory only enumerates registered providers, so a dropped
// relay clears `connected` for every one of its PTYs at once. Only the
@@ -59,6 +60,29 @@ function makeRuntimeMissingFromInventory(
}
describe('inventory sweep liveness verdicts', () => {
it('records an abnormal SSH exit as unverifiable at the runtime boundary', () => {
const runtime = makeRuntimeMissingFromInventory(() => null)
runtime.onPtyExit(REMOTE_PTY_ID, -1)
expect(runtime.getPtyLivenessVerdict(REMOTE_PTY_ID)).toEqual({
status: 'unverifiable',
reason: SSH_EXIT_UNCONFIRMED_REASON
})
})
it('preserves a more specific lost-contact reason across an abnormal SSH exit', () => {
const runtime = makeRuntimeMissingFromInventory(() => null)
runtime.markPtyLivenessUnverifiable(REMOTE_PTY_ID, 'inventory transport failed')
runtime.onPtyExit(REMOTE_PTY_ID, -1)
expect(runtime.getPtyLivenessVerdict(REMOTE_PTY_ID)).toEqual({
status: 'unverifiable',
reason: 'inventory transport failed'
})
})
it('records lost contact when no provider can answer for the PTY', async () => {
const runtime = makeRuntimeMissingFromInventory(() => null)
@@ -112,6 +136,23 @@ describe('inventory sweep liveness verdicts', () => {
})
})
it('does not let a partial inventory overwrite a concurrent provider failure', async () => {
const inventory = deferred<{ id: string; worktreeId: string }[]>()
const listProcesses = vi.fn(() => inventory.promise)
const runtime = makeRuntimeMissingFromInventory(() => false, listProcesses)
const listing = runtime.listTerminals(`id:${WORKTREE_ID}`)
await vi.waitFor(() => expect(listProcesses).toHaveBeenCalled())
runtime.markPtyLivenessUnverifiable(REMOTE_PTY_ID, 'inventory transport failed')
inventory.resolve([])
await listing
expect(runtime.getPtyLivenessVerdict(REMOTE_PTY_ID)).toEqual({
status: 'unverifiable',
reason: 'inventory transport failed'
})
})
it('clears stale doubt when a new PTY lifecycle is positively registered', () => {
const runtime = makeRuntimeMissingFromInventory(() => null)
runtime.markPtyLivenessUnverifiable(REMOTE_PTY_ID, 'old incarnation lost contact')
@@ -61,7 +61,7 @@ export const ORCHESTRATION_WORKER_RELEASE_METHODS: RpcMethod[] = [
(await runtime.inspectTerminalProcessIncarnationLiveness(
processIncarnation,
resource.host_scope
)) === 'dead'
)) === 'exited'
) {
const reconciled = db.settleDeadWorkerTerminalRelease({
requestingDispatchId: params.dispatch,
+1
View File
@@ -13,6 +13,7 @@ export type PtyLivenessVerdict =
export const SSH_PROVIDER_UNREGISTERED_REASON = 'its SSH provider is no longer registered'
export const NO_OBSERVING_PROVIDER_REASON = 'no registered provider can observe its host'
export const SSH_EXIT_UNCONFIRMED_REASON = 'the owning SSH host did not confirm the PTY exit'
export const PTY_LIVE_NOTE = 'The PTY is live.'
/** The one sentence every surface uses to admit a stop was not confirmed. */