diff --git a/src/cli/terminal-format.test.ts b/src/cli/terminal-format.test.ts index 014f6339823..5d3f647e522 100644 --- a/src/cli/terminal-format.test.ts +++ b/src/cli/terminal-format.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { formatTerminalFocus } from './terminal-format' +import { formatTerminalClose, formatTerminalFocus } from './terminal-format' describe('formatTerminalFocus', () => { it('distinguishes superseded navigation from a winning focus', () => { @@ -22,3 +22,40 @@ describe('formatTerminalFocus', () => { ).toBe('Focused terminal term_winner (tab tab-winner).') }) }) + +describe('formatTerminalClose', () => { + it('prints "PTY killed." only for a confirmed kill', () => { + expect( + formatTerminalClose({ close: { handle: 'term_local', tabId: 'tab-1', ptyKilled: true } }) + ).toBe('Closed terminal term_local. PTY killed.') + }) + + it('says the remote process was not confirmed stopped instead of claiming a kill', () => { + expect( + formatTerminalClose({ + close: { + handle: 'term_remote', + tabId: 'tab-1', + ptyKilled: false, + ptyStopVerdict: 'unverifiable', + ptyStopReason: 'its SSH provider is no longer registered' + } + }) + ).toBe( + 'Closed terminal term_remote. The PTY was not confirmed stopped: its SSH provider is no longer registered.' + ) + }) + + it('names a PTY still known to be running', () => { + expect( + formatTerminalClose({ + close: { + handle: 'term_live', + tabId: 'tab-1', + ptyKilled: false, + ptyStopVerdict: 'live' + } + }) + ).toBe('Closed terminal term_live. The PTY is still running.') + }) +}) diff --git a/src/cli/terminal-format.ts b/src/cli/terminal-format.ts index ae126fa6001..ffd1f740426 100644 --- a/src/cli/terminal-format.ts +++ b/src/cli/terminal-format.ts @@ -1,3 +1,4 @@ +import { PTY_STILL_RUNNING_NOTE, describeUnconfirmedStop } from '../shared/pty-liveness-verdict' import type { RuntimeTerminalClose, RuntimeTerminalCreate, @@ -170,12 +171,25 @@ export function formatTerminalFocus(result: { focus: RuntimeTerminalFocus }): st return `Focused terminal ${result.focus.handle} (tab ${result.focus.tabId}).` } +/** "PTY killed." is a claim of observed death, so only a confirmed kill earns it. */ +function describePtyStop(close: RuntimeTerminalClose): string { + if (close.ptyKilled) { + return ' PTY killed.' + } + if (close.ptyStopVerdict === 'live') { + return ` ${PTY_STILL_RUNNING_NOTE}` + } + if (close.ptyStopVerdict === 'unverifiable') { + return ` ${describeUnconfirmedStop(close.ptyStopReason ?? 'its host could not be reached')}` + } + return '' +} + export function formatTerminalClose(result: { close: RuntimeTerminalClose }): string { if (result.close.closeMode === 'tab') { return `Closed terminal tab ${result.close.tabId} (${result.close.handle}).` } - const ptyNote = result.close.ptyKilled ? ' PTY killed.' : '' - return `Closed terminal ${result.close.handle}.${ptyNote}` + return `Closed terminal ${result.close.handle}.${describePtyStop(result.close)}` } export function formatTerminalWait(result: { wait: RuntimeTerminalWait }): string { diff --git a/src/main/ipc/pty-daemon-controller-teardown.test.ts b/src/main/ipc/pty-daemon-controller-teardown.test.ts index 25d0aa48277..7f14ea730ae 100644 --- a/src/main/ipc/pty-daemon-controller-teardown.test.ts +++ b/src/main/ipc/pty-daemon-controller-teardown.test.ts @@ -319,7 +319,9 @@ describe('registerPtyHandlers', () => { kill: (ptyId: string) => boolean } - expect(controller.kill('ssh:ssh-1@@relay-pty')).toBe(true) + // The lease is tombstoned, but nothing observed the detached relay PTY + // stop, so the stop itself is reported as unconfirmed. + expect(controller.kill('ssh:ssh-1@@relay-pty')).toBe(false) expect(localShutdown).not.toHaveBeenCalled() expect(store.markSshRemotePtyLease).toHaveBeenCalledWith('ssh-1', 'relay-pty', 'terminated') @@ -347,7 +349,7 @@ describe('registerPtyHandlers', () => { kill: (ptyId: string) => boolean } - expect(controller.kill('remote-pty')).toBe(true) + expect(controller.kill('remote-pty')).toBe(false) expect(store.markSshRemotePtyLease).toHaveBeenCalledWith( 'ssh-1', diff --git a/src/main/ipc/pty-ssh-stop-verdict.test.ts b/src/main/ipc/pty-ssh-stop-verdict.test.ts new file mode 100644 index 00000000000..f22dc825a52 --- /dev/null +++ b/src/main/ipc/pty-ssh-stop-verdict.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, it, vi } from 'vitest' +import { setupPtyIpcSuite } from './pty-ipc-test-harness' +import { + registerPtyHandlers, + deletePtyOwnership, + setPtyOwnership, + getLocalPtyProvider +} from './pty' + +vi.mock('electron', () => import('./pty-ipc-mock-registry').then((m) => m.electronModuleMock())) +vi.mock('fs', () => import('./pty-ipc-mock-registry').then((m) => m.fsModuleMock())) +vi.mock('node-pty', () => import('./pty-ipc-mock-registry').then((m) => m.nodePtyModuleMock())) +vi.mock('node:child_process', async (importOriginal) => + (await import('./pty-ipc-mock-registry')).childProcessModuleMock(await importOriginal()) +) +vi.mock('../opencode/hook-service', () => + import('./pty-ipc-mock-registry').then((m) => m.openCodeHookServiceModuleMock()) +) +vi.mock('../mimo/hook-service', () => + import('./pty-ipc-mock-registry').then((m) => m.mimoHookServiceModuleMock()) +) +vi.mock('../agent-hooks/server', () => + import('./pty-ipc-mock-registry').then((m) => m.agentHookServerModuleMock()) +) +vi.mock('../pi/titlebar-extension-service', () => + import('./pty-ipc-mock-registry').then((m) => m.piTitlebarExtensionModuleMock()) +) +vi.mock('../pwsh', () => import('./pty-ipc-mock-registry').then((m) => m.pwshModuleMock())) +vi.mock('../wsl', async (importOriginal) => + (await import('./pty-ipc-mock-registry')).wslModuleMock(await importOriginal()) +) +vi.mock('../telemetry/client', () => + import('./pty-ipc-mock-registry').then((m) => m.telemetryClientModuleMock()) +) +vi.mock('../telemetry/classify-error', () => + import('./pty-ipc-mock-registry').then((m) => m.classifyErrorModuleMock()) +) +vi.mock('../cli/linux-terminal-orca-cli-shim', () => + import('./pty-ipc-mock-registry').then((m) => m.linuxCliShimModuleMock()) +) +vi.mock('../memory/pty-registry', () => + import('./pty-ipc-mock-registry').then((m) => m.ptyRegistryModuleMock()) +) +vi.mock('../agent-hooks/migration-unsupported-pty-state', () => + import('./pty-ipc-mock-registry').then((m) => m.migrationUnsupportedPtyModuleMock()) +) +vi.mock('../codex/codex-pane-account-registry', () => + import('./pty-ipc-mock-registry').then((m) => m.codexPaneAccountRegistryModuleMock()) +) +vi.mock('../codex/codex-state-db-backfill-recovery', () => + import('./pty-ipc-mock-registry').then((m) => m.codexBackfillRecoveryModuleMock()) +) + +// A detached relay PTY is designed to outlive the provider that addressed it, so +// "the SSH provider is gone" is never evidence that the remote process stopped. +describe('stopping a PTY whose SSH provider is unregistered', () => { + const { handlers, mainWindow, installObservableDaemonTestProvider } = setupPtyIpcSuite() + + function installController(): { + controller: { + kill: (ptyId: string) => boolean + stopAndWait: (ptyId: string, opts?: { deadlineMs?: number }) => Promise + } + runtime: { + setPtyController: ReturnType + onPtyExit: ReturnType + markPtyLivenessUnverifiable: ReturnType + } + } { + const runtime = { + setPtyController: vi.fn(), + onPtyExit: vi.fn(), + markPtyLivenessUnverifiable: vi.fn() + } + handlers.clear() + registerPtyHandlers(mainWindow as never, runtime as never) + return { + controller: runtime.setPtyController.mock.calls[0]?.[0] as never, + runtime + } + } + + it('reports an unconfirmed stop instead of a fabricated kill', () => { + setPtyOwnership('ssh-detached-pty', 'ssh-dropped') + const { controller, runtime } = installController() + + expect(controller.kill('ssh-detached-pty')).toBe(false) + // The local lease is still tombstoned so reconnect cannot revive the pane. + expect(runtime.onPtyExit).toHaveBeenCalledWith('ssh-detached-pty', -1, undefined) + expect(runtime.markPtyLivenessUnverifiable).toHaveBeenCalledWith( + 'ssh-detached-pty', + expect.stringContaining('SSH') + ) + deletePtyOwnership('ssh-detached-pty') + }) + + it('reports an unconfirmed exact stop instead of a fabricated teardown', async () => { + setPtyOwnership('ssh-detached-stop', 'ssh-dropped') + const { controller, runtime } = installController() + + await expect(controller.stopAndWait('ssh-detached-stop')).resolves.toBe(false) + expect(runtime.onPtyExit).toHaveBeenCalledWith('ssh-detached-stop', -1, undefined) + expect(runtime.markPtyLivenessUnverifiable).toHaveBeenCalledWith( + 'ssh-detached-stop', + expect.stringContaining('SSH') + ) + deletePtyOwnership('ssh-detached-stop') + }) + + it('still confirms a stop the owning provider actually performed', async () => { + const daemon = installObservableDaemonTestProvider() + vi.spyOn(getLocalPtyProvider(), 'listProcesses').mockResolvedValue([]) + const { controller, runtime } = installController() + + await expect(controller.stopAndWait('wt-1@@local-session')).resolves.toBe(true) + expect(daemon.shutdown).toHaveBeenCalledWith( + 'wt-1@@local-session', + expect.objectContaining({ immediate: true }) + ) + expect(runtime.markPtyLivenessUnverifiable).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 81ed2eb307d..0ef5ceaff60 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -15,6 +15,7 @@ export { getBashShellReadyRcfileContent } from '../providers/local-pty-shell-rea import type { OrcaRuntimeService } from '../runtime/orca-runtime' import type { PtyBindingSourceExpectation, Store } from '../persistence' import { retireTerminalSurfaceFromPersistence } from '../runtime/mobile-session-terminal-persistence-retirement' +import { SSH_PROVIDER_UNREGISTERED_REASON } from '../../shared/pty-liveness-verdict' import type { GlobalSettings } from '../../shared/global-settings-types' import type { TuiAgent } from '../../shared/tui-agent' import { toSshExecutionHostId } from '../../shared/execution-host' @@ -5518,12 +5519,15 @@ export function registerPtyHandlers( if (connectionId) { // Why: runtime/CLI close can target a detached SSH PTY after its // provider was unregistered. Tombstone the lease so reconnect does - // not revive a terminal the user explicitly closed. + // not revive a terminal the user explicitly closed — but a detached + // relay PTY outlives its provider, so report an unconfirmed stop + // rather than a kill nobody performed. const incarnationId = finishPtyShutdown(ptyId, connectionId, store) runtime?.onPtyExit(ptyId, -1, incarnationId) rememberSyntheticKillExit(ptyId) sendPtyExitToRenderer({ id: ptyId, code: -1 }) - return true + runtime?.markPtyLivenessUnverifiable?.(ptyId, SSH_PROVIDER_UNREGISTERED_REASON) + return false } return false } @@ -5648,13 +5652,14 @@ export function registerPtyHandlers( provider = connectionId ? getProvider(connectionId) : getProviderForPty(ptyId) } catch { if (connectionId) { - // Why: an absent SSH provider means there is no live target left to - // await, but the relay lease must still be tombstoned. + // Why: the relay lease must still be tombstoned, but an absent SSH + // provider is lost contact — the remote PTY is designed to survive it, + // so nothing here observed an exit to report as a confirmed stop. const incarnationId = finishPtyShutdown(ptyId, connectionId, store) runtime?.onPtyExit(ptyId, -1, incarnationId) rememberSyntheticKillExit(ptyId) sendPtyExitToRenderer({ id: ptyId, code: -1 }) - return true + runtime?.markPtyLivenessUnverifiable?.(ptyId, SSH_PROVIDER_UNREGISTERED_REASON) } return false } diff --git a/src/main/runtime/orca-runtime-terminal-close-continuity.test.ts b/src/main/runtime/orca-runtime-terminal-close-continuity.test.ts index 6d22718dcae..0f36529663d 100644 --- a/src/main/runtime/orca-runtime-terminal-close-continuity.test.ts +++ b/src/main/runtime/orca-runtime-terminal-close-continuity.test.ts @@ -365,6 +365,42 @@ describe('terminal close and handle incarnation continuity', () => { expect(harness.kill).not.toHaveBeenCalled() }) + it('reports an unconfirmed stop on the close receipt rather than a bare uncertain false', async () => { + const harness = createHarness() + const [{ handle }] = (await harness.runtime.listTerminals(`id:${WORKTREE_ID}`)).terminals + // Mirrors pty.ts when the SSH provider is gone: the lease is tombstoned, the + // stop reports false, and the PTY is marked as contact we lost — not a kill. + harness.kill.mockReturnValue(false) + harness.setVerifiedStopResult(false) + harness.runtime.markPtyLivenessUnverifiable(PTY_ID, 'its SSH provider is no longer registered') + + const closing = harness.runtime.closeTerminal(handle) + await vi.waitFor(() => expect(harness.closeTerminalTab).toHaveBeenCalled()) + harness.retirePersistedTab() + harness.acknowledged.resolve() + + await expect(closing).resolves.toMatchObject({ + ptyKilled: false, + ptyStopVerdict: 'unverifiable', + ptyStopReason: 'its SSH provider is no longer registered' + }) + }) + + it('leaves a confirmed kill receipt free of any stop verdict', async () => { + const harness = createHarness() + const [{ handle }] = (await harness.runtime.listTerminals(`id:${WORKTREE_ID}`)).terminals + + const closing = harness.runtime.closeTerminal(handle) + await vi.waitFor(() => expect(harness.closeTerminalTab).toHaveBeenCalled()) + harness.retirePersistedTab() + harness.acknowledged.resolve() + + const close = await closing + expect(close.ptyKilled).toBe(true) + expect(close.ptyStopVerdict).toBeUndefined() + expect(close.ptyStopReason).toBeUndefined() + }) + it('falls back to kill when verified teardown rejects after retirement', async () => { const harness = createHarness() const [{ handle }] = (await harness.runtime.listTerminals(`id:${WORKTREE_ID}`)).terminals diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 6276266127e..2b399e71353 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -657,6 +657,10 @@ import { type RetiredTerminalSurface } from './mobile-session-terminal-retirement' import { retireTerminalSurfaceFromPersistence } from './mobile-session-terminal-persistence-retirement' +import { + NO_OBSERVING_PROVIDER_REASON, + type PtyLivenessVerdict +} from '../../shared/pty-liveness-verdict' import { advanceTerminalTopologyRevision, hasHostAuthoritativeTerminalMembership @@ -1553,6 +1557,9 @@ function isAgentSessionOperationOutcomeUnknown(error: unknown): boolean { ) } +// Bounded because verdicts outlive their pty records; oldest-first eviction. +const MAX_TRACKED_PTY_LIVENESS_VERDICTS = 256 + const AGENT_SESSION_OPERATION_PER_CLIENT_LIMIT = 512 const AGENT_SESSION_OPERATION_GLOBAL_LIMIT = 4_096 @@ -3125,6 +3132,11 @@ export class OrcaRuntimeService { // iterates them all. Listeners are cleaned up via subscriptionCleanups. private notificationListeners = new Set<(event: MobileNotificationEvent) => void>() private ptysById = new Map() + // Why a separate map: `connected` is a wire field that any inventory gap + // clears, so it cannot distinguish an observed exit from lost contact. This + // records the last liveness verdict we actually earned, and outlives the pty + // record so a close/stop receipt can still say the stop was unconfirmed. + private ptyLivenessVerdictByPtyId = new Map() private readonly pairedRendererSessionOwnedPtyIds = new Set() private wslDistroByPtyId = new Map() private titleObservationSequence = 0 @@ -14830,6 +14842,11 @@ export class OrcaRuntimeService { this.setPairedRendererSessionOwnership(pty.ptyId, false) pty.disconnectedAt = Date.now() pty.lastExitCode = exitCode + if (exitCode >= 0) { + // A real wait status from the owning host is the death certificate; the + // synthetic -1 we emit on a failed/unroutable stop is not. + this.forgetPtyLivenessVerdict(ptyId) + } // Why: the exited process's live frames say nothing about a replacement. // A same-id respawn makes the leaf writable again before any new title, // so leaving this true would let push delivery type into the new process @@ -17518,6 +17535,46 @@ export class OrcaRuntimeService { return `${this.runtimeId}:${record.ptyId}:${record.ptyGeneration}` } + /** + * Records that we lost contact with a PTY's owning host. Callers must never + * read this as an exit: a detached relay PTY is designed to outlive the + * provider that addressed it. + */ + markPtyLivenessUnverifiable(ptyId: string, reason: string): void { + this.rememberPtyLivenessVerdict(ptyId, { status: 'unverifiable', reason }) + } + + /** Null when nothing has been observed either way, so callers keep their own default. */ + getPtyLivenessVerdict(ptyId: string): PtyLivenessVerdict | null { + return this.ptyLivenessVerdictByPtyId.get(ptyId) ?? null + } + + getTerminalLivenessVerdict(handle: string): PtyLivenessVerdict | null { + const record = this.getLivePtyForHandle(handle)?.record ?? this.handles.get(handle) + return record?.ptyId ? this.getPtyLivenessVerdict(record.ptyId) : null + } + + private rememberPtyLivenessVerdict(ptyId: string, verdict: PtyLivenessVerdict): void { + if (verdict.status === 'exited') { + // An earned death certificate ends the question; nothing left to remember. + this.ptyLivenessVerdictByPtyId.delete(ptyId) + return + } + this.ptyLivenessVerdictByPtyId.delete(ptyId) + this.ptyLivenessVerdictByPtyId.set(ptyId, verdict) + while (this.ptyLivenessVerdictByPtyId.size > MAX_TRACKED_PTY_LIVENESS_VERDICTS) { + const oldest = this.ptyLivenessVerdictByPtyId.keys().next() + if (oldest.done) { + return + } + this.ptyLivenessVerdictByPtyId.delete(oldest.value) + } + } + + private forgetPtyLivenessVerdict(ptyId: string): void { + this.ptyLivenessVerdictByPtyId.delete(ptyId) + } + getExactWorkerProviderSession( handle: string, observedAfter: number @@ -28642,13 +28699,13 @@ export class OrcaRuntimeService { this.notifier.closeTerminal?.(tabId) } const ptyKilled = await this.stopExplicitlyClosedTabPtys(ptyIdsToKill, pty.pty.ptyId) - return { handle, tabId, ptyKilled } + return this.describeTerminalClose(handle, tabId, pty.pty.ptyId, ptyKilled) } if (siblingCount <= 1 && !surface && pty.pty.tabId && this.notifier?.closeTerminalTab) { const ptyIdsToKill = this.getPtyIdsForExplicitTabClose(pty.pty.worktreeId, tabId) await this.notifier.closeTerminalTab(tabId, { localPtyTeardownOwnedExternally: true }) const ptyKilled = await this.stopExplicitlyClosedTabPtys(ptyIdsToKill, pty.pty.ptyId) - return { handle, tabId, ptyKilled } + return this.describeTerminalClose(handle, tabId, pty.pty.ptyId, ptyKilled) } const ptyKilled = this.ptyController?.kill(pty.pty.ptyId) ?? false if (!ptyKilled || siblingCount <= 1) { @@ -28666,7 +28723,7 @@ export class OrcaRuntimeService { this.notifier?.closeTerminal(tabId) } } - return { handle, tabId, ptyKilled } + return this.describeTerminalClose(handle, tabId, pty.pty.ptyId, ptyKilled) } this.assertGraphReady() const { leaf } = this.getLiveLeafForHandle(handle) @@ -28689,7 +28746,36 @@ export class OrcaRuntimeService { if (siblingCount > 1 ? !ptyKilled : !this.notifier?.closeTerminalTab) { this.notifier?.closeTerminal(leaf.tabId, leaf.paneRuntimeId) } - return { handle, tabId: leaf.tabId, ptyKilled } + return this.describeTerminalClose(handle, leaf.tabId, leaf.ptyId ?? null, ptyKilled) + } + + /** + * A close receipt must not read as a kill nobody performed: when the stop was + * not confirmed, the receipt carries why so the CLI and callers can say so. + */ + private describeTerminalClose( + handle: string, + tabId: string, + ptyId: string | null, + ptyKilled: boolean + ): RuntimeTerminalClose { + if (ptyKilled || !ptyId) { + return { handle, tabId, ptyKilled } + } + const verdict = this.getPtyLivenessVerdict(ptyId) + if (verdict?.status === 'unverifiable') { + return { + handle, + tabId, + ptyKilled, + ptyStopVerdict: 'unverifiable', + ptyStopReason: verdict.reason + } + } + if (verdict?.status === 'live') { + return { handle, tabId, ptyKilled, ptyStopVerdict: 'live' } + } + return { handle, tabId, ptyKilled } } async closeTerminalTab(handle: string): Promise { @@ -31425,7 +31511,8 @@ export class OrcaRuntimeService { continue } if (!allLivePtyIds.has(pty.ptyId) && !this.leafExistsForPty(pty.ptyId)) { - if (this.ptyController.hasPty?.(pty.ptyId) === true) { + const observed = this.ptyController.hasPty?.(pty.ptyId) + if (observed === true) { // Why: an SSH spawn can become addressable before an overlapping relay list includes it. allLivePtyIds.add(pty.ptyId) if ( @@ -31436,10 +31523,19 @@ export class OrcaRuntimeService { } pty.connected = true pty.disconnectedAt = null + this.forgetPtyLivenessVerdict(pty.ptyId) continue } pty.connected = false pty.disconnectedAt ??= Date.now() + // Why: this list only enumerates registered providers, so a dropped relay + // clears `connected` for every one of its PTYs at once. Only `false` here + // is an observed absence; `null` means no provider could be asked. + if (observed === false) { + this.forgetPtyLivenessVerdict(pty.ptyId) + } else if (observed === null) { + this.markPtyLivenessUnverifiable(pty.ptyId, NO_OBSERVING_PROVIDER_REASON) + } } } this.pruneDisconnectedPtyRecords() diff --git a/src/main/runtime/pty-inventory-liveness-verdict.test.ts b/src/main/runtime/pty-inventory-liveness-verdict.test.ts new file mode 100644 index 00000000000..61b73c884e3 --- /dev/null +++ b/src/main/runtime/pty-inventory-liveness-verdict.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it, vi } from 'vitest' +import { OrcaRuntimeService } from './orca-runtime' +import { getDefaultWorkspaceSession } from '../../shared/constants' + +// The aggregate inventory only enumerates registered providers, so a dropped +// relay clears `connected` for every one of its PTYs at once. Only the +// provider's own answer separates an observed exit from lost contact. + +const WORKTREE_ID = 'repo-1::/tmp/inventory-verdict' +const REMOTE_PTY_ID = 'ssh:conn-1@@relay-9' + +function makeStore() { + const session = getDefaultWorkspaceSession() + return { + getWorkspaceSession: vi.fn(() => session), + setWorkspaceSession: vi.fn(), + getRepos: vi.fn(() => [ + { + id: 'repo-1', + path: '/tmp/inventory-verdict', + displayName: 'inventory-verdict', + badgeColor: '#000000', + addedAt: 0 + } + ]), + getAllWorktreeMeta: vi.fn(() => ({})), + getWorktreeMeta: vi.fn(() => undefined), + setWorktreeMeta: vi.fn(), + removeWorktreeMeta: vi.fn(), + getSettings: vi.fn(() => ({ workspaceDir: '/tmp/workspaces' })), + getProjects: vi.fn(() => []) + } +} + +function makeRuntimeMissingFromInventory(hasPty: () => boolean | null): OrcaRuntimeService { + const runtime = new OrcaRuntimeService(makeStore() as never) + runtime.setPtyController({ + write: () => true, + kill: () => true, + hasPty, + listProcesses: vi.fn(async () => []), + getForegroundProcess: async () => null + } as never) + runtime.attachWindow(1) + runtime.syncWindowGraph(1, { tabs: [], leaves: [] }) + runtime.registerPty(REMOTE_PTY_ID, WORKTREE_ID, 'conn-1') + return runtime +} + +describe('inventory sweep liveness verdicts', () => { + it('records lost contact when no provider can answer for the PTY', async () => { + const runtime = makeRuntimeMissingFromInventory(() => null) + + await runtime.listTerminals(`id:${WORKTREE_ID}`) + + expect(runtime.getPtyLivenessVerdict(REMOTE_PTY_ID)).toEqual({ + status: 'unverifiable', + reason: 'no registered provider can observe its host' + }) + }) + + it('records no doubt when the owning provider reports the PTY absent', async () => { + const runtime = makeRuntimeMissingFromInventory(() => false) + + await runtime.listTerminals(`id:${WORKTREE_ID}`) + + // An observed absence is the death certificate callers already act on. + expect(runtime.getPtyLivenessVerdict(REMOTE_PTY_ID)).toBeNull() + }) +}) diff --git a/src/main/runtime/rpc/methods/orchestration-worker-observation.ts b/src/main/runtime/rpc/methods/orchestration-worker-observation.ts index 7148fe32dcc..8ea4bba730c 100644 --- a/src/main/runtime/rpc/methods/orchestration-worker-observation.ts +++ b/src/main/runtime/rpc/methods/orchestration-worker-observation.ts @@ -10,7 +10,9 @@ export async function inspectWorkerTerminal( ): Promise<{ terminal: Awaited> | null exact: boolean - status: 'unattached' | 'missing' | 'identity_changed' | 'running' | 'exited' + status: 'unattached' | 'missing' | 'identity_changed' | 'running' | 'exited' | 'unverifiable' + /** Set with `unverifiable`; names what we lost contact with. */ + reason?: string }> { const worker = db.getWorkerDispatch(dispatchId) if (!worker?.agent_terminal_handle) { @@ -25,10 +27,23 @@ export async function inspectWorkerTerminal( paneKey: runtime.getTerminalPaneKey(worker.agent_terminal_handle), processIncarnation: runtime.getTerminalProcessIncarnation(worker.agent_terminal_handle) }) + if (!exact) { + return { terminal, exact, status: 'identity_changed' } + } + // Why: the aggregate inventory only iterates registered providers, so a dropped + // relay clears `connected` for every remote PTY at once. Lost contact is not a + // death certificate, and the verdict is the only field that can tell them apart. + const verdict = runtime.getTerminalLivenessVerdict?.(worker.agent_terminal_handle) ?? null + if (verdict?.status === 'unverifiable') { + return { terminal, exact, status: 'unverifiable', reason: verdict.reason } + } + if (verdict?.status === 'live') { + return { terminal, exact, status: 'running' } + } return { terminal, exact, - status: exact ? (terminal.connected === false ? 'exited' : 'running') : 'identity_changed' + status: terminal.connected === false ? 'exited' : 'running' } } diff --git a/src/main/runtime/rpc/methods/orchestration-worker-stop-liveness-verdict.test.ts b/src/main/runtime/rpc/methods/orchestration-worker-stop-liveness-verdict.test.ts new file mode 100644 index 00000000000..640257296b9 --- /dev/null +++ b/src/main/runtime/rpc/methods/orchestration-worker-stop-liveness-verdict.test.ts @@ -0,0 +1,98 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { OrcaRuntimeService } from '../../orca-runtime' +import { OrchestrationDb } from '../../orchestration/db' +import { ORCHESTRATION_METHODS } from './orchestration' + +// The aggregate terminal inventory only iterates registered providers, so a +// dropped relay clears `connected` for every remote PTY at once. That is lost +// contact, not a death certificate. +describe('worker-stop against a terminal we lost contact with', () => { + let db: OrchestrationDb + let runtime: OrcaRuntimeService + + beforeEach(() => { + db = new OrchestrationDb(':memory:') + runtime = new OrcaRuntimeService() + runtime.setOrchestrationDb(db) + vi.spyOn(runtime, 'getTerminalPaneKey').mockReturnValue( + 'tab_worker:bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb' + ) + vi.spyOn(runtime, 'getTerminalProcessIncarnation').mockReturnValue('runtime:pty:1') + vi.spyOn(runtime, 'showTerminal').mockResolvedValue({ + handle: 'term_worker', + worktreeId: 'repo::worktree', + connected: false, + status: 'exited' + } as never) + }) + + afterEach(() => db.close()) + + async function call(name: string, params: Record) { + const method = ORCHESTRATION_METHODS.find((candidate) => candidate.name === name) + if (!method) { + throw new Error(`Method not found: ${name}`) + } + return method.handler(method.params!.parse(params), { runtime }) + } + + function createWorker() { + const run = db.createRun({ + objective: 'Verdict', + coordinatorHandle: 'term_coord', + coordinatorPaneKey: 'tab_coord:aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa' + }) + const task = db.createTask({ spec: 'stop worker', runId: run.id }) + const started = db.createStartingWorkerDispatch({ + taskId: task.id, + startOptions: {}, + runtimeEpoch: runtime.getRuntimeId() + }) + db.prepareStartingWorkerAuthority({ + dispatchId: started.dispatch.id, + handle: 'term_worker', + paneKey: 'tab_worker:bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb', + processIncarnation: 'runtime:pty:1', + worktreeId: 'repo::worktree', + setupState: 'not_applicable', + effects: [{ kind: 'terminal', action: 'created', id: 'term_worker' }] + }) + db.markWorkerDispatchReady(started.dispatch.id) + return started.dispatch + } + + it('reports the process as unverifiable, not exited, when the link dropped', async () => { + vi.spyOn(runtime, 'getTerminalLivenessVerdict').mockReturnValue({ + status: 'unverifiable', + reason: 'its SSH provider is no longer registered' + }) + const dispatch = createWorker() + + await expect( + call('orchestration.workerShow', { dispatch: dispatch.id }) + ).resolves.toMatchObject({ + observation: { status: 'unverifiable', exactWorker: true } + }) + + const stopped = (await call('orchestration.workerStop', { dispatch: dispatch.id })) as { + lastError: string + } + expect(stopped.lastError).toContain('could not be confirmed stopped') + expect(stopped.lastError).not.toContain('exited') + }) + + it('still reports a locally observed exit as exited', async () => { + const dispatch = createWorker() + + await expect( + call('orchestration.workerShow', { dispatch: dispatch.id }) + ).resolves.toMatchObject({ + observation: { status: 'exited', exactWorker: true } + }) + + const stopped = (await call('orchestration.workerStop', { dispatch: dispatch.id })) as { + lastError: string + } + expect(stopped.lastError).toBe('The recorded worker process is exited; no terminal was closed.') + }) +}) diff --git a/src/main/runtime/rpc/methods/orchestration-worker-stop.ts b/src/main/runtime/rpc/methods/orchestration-worker-stop.ts index 0802fac3954..478557c1635 100644 --- a/src/main/runtime/rpc/methods/orchestration-worker-stop.ts +++ b/src/main/runtime/rpc/methods/orchestration-worker-stop.ts @@ -106,13 +106,35 @@ export const ORCHESTRATION_WORKER_STOP_METHODS: RpcMethod[] = [ params.dispatch, db.markWorkerStopUnknown( params.dispatch, - `The recorded worker process is ${observation.status}; no terminal was closed.` + observation.status === 'unverifiable' + ? // Why: losing the link to the worker's host is not a death certificate, + // so this must not settle as a stop or read as an observed exit. + `The recorded worker process could not be confirmed stopped: ${ + observation.reason ?? 'its host could not be reached' + }; no terminal was closed.` + : `The recorded worker process is ${observation.status}; no terminal was closed.` ), 'none' ) } try { const close = await runtime.closeTerminal(handle) + if (close.ptyStopVerdict) { + // The tab is retired, but the agent process was never confirmed stopped — + // settling here is the false success this receipt exists to prevent. + return unknownReceipt( + params.dispatch, + db.markWorkerStopUnknown( + params.dispatch, + `The agent terminal was closed but its process could not be confirmed stopped: ${ + close.ptyStopVerdict === 'live' + ? 'it is still running' + : (close.ptyStopReason ?? 'its host could not be reached') + }.` + ), + 'closed_agent_terminal' + ) + } const worker = db.settleWorkerStop(params.dispatch) runtime.notifyMessageArrived(`dispatch:${params.dispatch}`, 'status') return { diff --git a/src/main/runtime/unstopped-pty-verification.ts b/src/main/runtime/unstopped-pty-verification.ts index 2dc9aac5960..50ecfbb7a4b 100644 --- a/src/main/runtime/unstopped-pty-verification.ts +++ b/src/main/runtime/unstopped-pty-verification.ts @@ -1,18 +1,17 @@ import type { IPtyProvider } from '../providers/types' +import type { OrcaRuntimeService } from './orca-runtime' import { UNSTOPPED_PTY_DETAIL_SEPARATOR, UNSTOPPED_PTY_LIVE_DETAIL_PREFIX, UNSTOPPED_PTY_REMOVAL_PREFIX } from '../../shared/worktree/removal' +import type { PtyLivenessVerdict } from '../../shared/pty-liveness-verdict' import { settleBeforeDeadline } from './settle-before-deadline' // Floor for the verification window when the sweep ran on a very short budget. export const WORKTREE_TEARDOWN_VERIFY_GRACE_MS = 2_000 -export type UnstoppedPtyVerdict = - | { status: 'exited' } - | { status: 'live'; ptyIds: string[] } - | { status: 'unverifiable'; reason: string } +export type UnstoppedPtyVerdict = PtyLivenessVerdict /** * Re-lists the provider's processes to decide what a failed stop RPC actually @@ -56,6 +55,24 @@ export async function verifyUnstoppedPtys( return stillLive.length > 0 ? { status: 'live', ptyIds: stillLive } : { status: 'exited' } } +/** + * A stop that lost contact with the PTY's own host stays unverifiable: the + * surviving provider's inventory is silent about a host it cannot reach, and + * silence is not evidence of an exit. Force Delete is still the escape hatch. + */ +export function unverifiableStopVerdict( + failedPtyIds: readonly string[], + runtime: OrcaRuntimeService | undefined +): UnstoppedPtyVerdict | null { + for (const ptyId of failedPtyIds) { + const verdict = runtime?.getPtyLivenessVerdict?.(ptyId) + if (verdict?.status === 'unverifiable') { + return verdict + } + } + return null +} + /** Names the blocking PTYs so a wedged removal is diagnosable, not just refused. */ export function describeUnstoppedPtys( worktreeId: string, diff --git a/src/main/runtime/worktree-teardown-unstopped-pty.test.ts b/src/main/runtime/worktree-teardown-unstopped-pty.test.ts index 31ba390c3ec..1da82f8f49b 100644 --- a/src/main/runtime/worktree-teardown-unstopped-pty.test.ts +++ b/src/main/runtime/worktree-teardown-unstopped-pty.test.ts @@ -175,6 +175,48 @@ describe('destructive teardown when a PTY stop cannot be proven', () => { expect(error.message).not.toContain('w1@@gone-2') }) + // Why: the client's own provider list is silent about an SSH host, so a stop + // it could not confirm must stay unverifiable instead of reading as exited — + // otherwise removal walks straight past a live remote agent. + it("blocks removal when the stop lost contact with the PTY's own host", async () => { + // The surviving provider lists nothing for a host it cannot reach, so without + // the recorded verdict this empty list reads as "exited" and removal walks + // straight past a live remote agent. + const localProvider = createProviderStub(async () => []) + listRegisteredPtysMock.mockReturnValue([]) + const runtime = { + stopTerminalsForWorktree: async ( + _worktreeId: string, + opts: { + stopPty?: ( + ptyId: string, + stop: () => boolean | Promise + ) => Promise<{ stopped: boolean; owner: boolean }> + } + ) => { + await opts.stopPty?.('ssh:conn-1@@relay-9', () => false) + return { stopped: 0 } + }, + getPtyLivenessVerdict: (ptyId: string) => + ptyId === 'ssh:conn-1@@relay-9' + ? { status: 'unverifiable', reason: 'its SSH provider is no longer registered' } + : null + } + + await expect( + killAllProcessesForWorktree('w1', { + runtime: runtime as never, + localProvider, + resolvedConnectionId: 'conn-1', + includeProviderInventory: false, + includeLocalRegistry: false, + requirePhysicalStop: true + }) + ).rejects.toThrow( + /could not verify[\s\S]*ssh:conn-1@@relay-9[\s\S]*SSH provider is no longer registered[\s\S]*--force/ + ) + }) + it('reports unverifiable separately from live when the process list fails', async () => { const localProvider = createProviderStub(async () => { throw new Error('daemon socket closed') diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index ed0df69e8f7..1f318685d29 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -15,6 +15,7 @@ import { describeError, describeFailedPtySweep, describeUnstoppedPtys, + unverifiableStopVerdict, verifyUnstoppedPtys, type UnstoppedPtyVerdict } from './unstopped-pty-verification' @@ -234,7 +235,8 @@ export async function killAllProcessesForWorktree( const verdict: UnstoppedPtyVerdict = failedPtyIds.length === 0 ? { status: 'exited' } - : await verifyUnstoppedPtys(failedPtyIds, deps.localProvider, sweepBudgetMs) + : (unverifiableStopVerdict(failedPtyIds, deps.runtime) ?? + (await verifyUnstoppedPtys(failedPtyIds, deps.localProvider, sweepBudgetMs))) if (verdict.status === 'exited') { for (const ptyId of failedPtyIds) { clearStoppedPtyState(ptyId, deps.onPtyStopped) diff --git a/src/shared/pty-liveness-verdict.ts b/src/shared/pty-liveness-verdict.ts new file mode 100644 index 00000000000..e7e5b8c8811 --- /dev/null +++ b/src/shared/pty-liveness-verdict.ts @@ -0,0 +1,22 @@ +/** + * The one vocabulary Orca uses to talk about whether a PTY is still running. + * + * `exited` requires positive evidence of absence from the owning host. Losing + * contact with that host — an unregistered SSH provider, a dropped relay, an + * inventory that only enumerates registered providers — is `unverifiable`, never + * a death certificate and never a successful stop. + */ +export type PtyLivenessVerdict = + | { status: 'exited' } + | { status: 'live'; ptyIds: string[] } + | { status: 'unverifiable'; reason: string } + +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' + +/** The one sentence every surface uses to admit a stop was not confirmed. */ +export function describeUnconfirmedStop(reason: string): string { + return `The PTY was not confirmed stopped: ${reason}.` +} + +export const PTY_STILL_RUNNING_NOTE = 'The PTY is still running.' diff --git a/src/shared/runtime-types.ts b/src/shared/runtime-types.ts index d6b257ca85f..e33afeb7219 100644 --- a/src/shared/runtime-types.ts +++ b/src/shared/runtime-types.ts @@ -717,6 +717,14 @@ export type RuntimeTerminalClose = { /** Present for the durable whole-tab lifecycle without changing legacy receipts. */ closeMode?: 'tab' ptyKilled: boolean + /** + * Why the PTY was not killed, when we know. Absent means today's answer — + * nothing observed either way — so older clients reading only `ptyKilled` are + * unaffected. `exited` never appears here: that is what `ptyKilled` reports. + */ + ptyStopVerdict?: 'live' | 'unverifiable' + /** Set with `ptyStopVerdict: 'unverifiable'`; names what we lost contact with. */ + ptyStopReason?: string } export type RuntimeTerminalWaitCondition = 'exit' | 'tui-idle'