From b8cf8a095ac1cadd86baf035d03f7b5687c09bfb Mon Sep 17 00:00:00 2001 From: Brennan Benson Date: Sun, 12 Jul 2026 20:37:00 -0700 Subject: [PATCH] fence teardown replay by provider owner --- config/reliability-gates.jsonc | 9 ++-- src/main/ipc/pty.test.ts | 77 ++++++++++++++++++++++++-- src/main/ipc/pty.ts | 78 ++++++++++++++++++++------- src/main/runtime/orca-runtime.test.ts | 33 ++++++++++-- src/main/runtime/orca-runtime.ts | 13 +++-- 5 files changed, 172 insertions(+), 38 deletions(-) diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index adbc540ea36..2d3228ea02e 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -3771,7 +3771,7 @@ "https://github.com/stablyai/orca/pull/8279" ], "invariant": "A Windows PTY closed before its first output executes exactly one no-argument node-pty shutdown immediately; Orca never deletes ownership without accepted native shutdown or an observed exit, and a later retry can target only the same relay generation and pane while project removal fences both worktree and terminal creation.", - "oracle": "Load the installed patched WindowsTerminal prototype in a pre-ready state and assert kill and destroy each close the terminal and native agent once with zero deferred work, while signal arguments still throw. Build both Windows SSH relay artifacts, reproduce vanilla npm node-pty with close=0/native-kill=0/deferred=1, apply the shipped patch, and assert close=1/native-kill=1/deferred=0, AttachConsole failure falls back to the shell pid, and the artifact version derives from all patch bytes. Assert remote npm install executes patch enforcement before its runtime probe and never finalizes a failed patch. In local and relay providers, assert native throws retain exact ownership/listeners and retry succeeds, while a synchronous onExit is accepted as independent death proof; provider absence must retain the SSH lease and reject shutdown. Persist relay generation with every durable lease, expire mismatches before attach, and preserve generation through target migration. Reject local daemon and remote-runtime teardown, reload the durable store, replace the startup provider, and prove capped retries drain exact identities with zero final retry timers; persisted close intent for an already-removed runtime must drain without an unhandled rejection or retry wake loop. Replace SSH relay and remote-runtime generations while a retry is retained and prove the new owner is never closed. Start worktree and terminal creation surfaces before whole-project and host-scoped project removal and assert removal performs no ownership/liveness snapshot until they publish; removal-first and failed/concurrent admissions must reject or release without deadlock. Start a real worktree removal, pause its preflight, and assert a late renderer terminal admission is rejected; assert every successful registered, orphaned, leftover, already-missing, and stale-registration removal path verifies shutdown before mutation or metadata purge; assert desktop repo removal, worktree create/remove, and PTY spawn handlers all enter their runtime-owned transactions before mutation or provider work.", + "oracle": "Load the installed patched WindowsTerminal prototype in a pre-ready state and assert kill and destroy each close the terminal and native agent once with zero deferred work, while signal arguments still throw. Build both Windows SSH relay artifacts, reproduce vanilla npm node-pty with close=0/native-kill=0/deferred=1, apply the shipped patch, and assert close=1/native-kill=1/deferred=0, AttachConsole failure falls back to the shell pid, and the artifact version derives from all patch bytes. Assert remote npm install executes patch enforcement before its runtime probe and never finalizes a failed patch. In local and relay providers, assert native throws retain exact ownership/listeners and retry succeeds, while a synchronous onExit is accepted as independent death proof; provider absence must retain the SSH lease and reject shutdown. Persist relay generation with every durable lease, expire mismatches before attach, and preserve generation through target migration. Reject local daemon and remote-runtime teardown, reload the durable store, wait for the startup provider before replay, replace an in-flight provider, and prove capped retries drain exact identities with zero final retry timers; persisted close intent for an already-removed runtime must drain without an unhandled rejection or retry wake loop. Replace SSH relay and remote-runtime generations while a retry is retained and prove the new owner is never closed. Start worktree and terminal creation surfaces before whole-project and host-scoped project removal and assert removal performs no ownership/liveness snapshot until they publish; host-scoped liveness must query only its selected provider so an unrelated SSH outage cannot veto removal. Removal-first and failed/concurrent admissions must reject or release without deadlock. Start a real worktree removal, pause its preflight, and assert a late renderer terminal admission is rejected; assert every successful registered, orphaned, leftover, already-missing, and stale-registration removal path verifies shutdown before mutation or metadata purge; assert desktop repo removal, worktree create/remove, and PTY spawn handlers all enter their runtime-owned transactions before mutation or provider work.", "commands": [ "pnpm exec vitest run --config config/vitest.config.ts config/scripts/build-relay-windows-node-pty-patch.test.mjs src/main/ssh/ssh-relay-native-deps-install.test.ts src/relay/node-pty-windows-pre-ready-shutdown.test.ts src/relay/pty-handler.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts src/renderer/src/store/slices/repos.test.ts src/renderer/src/store/slices/repos-runtime-vm-removal.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts", "pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts", @@ -3889,6 +3889,7 @@ "project removal drains create-first worktree creation before its ownership snapshot and rejects removal-first worktree creation", "host-scoped project removal drains create-first work and rejects removal-first creation before mutating one duplicate-host row", "duplicate repo and worktree ids isolate admissions and PTY liveness by execution host while whole-project removal still drains every host scope", + "host-scoped terminal stop succeeds when an unrelated provider liveness path is unavailable", "worktree removal rejects a late renderer terminal admission while destructive preflight is still pending", "already-missing exceptional removal verifies terminal stop before metadata purge", "failed and concurrent admissions release without deadlocking project removal", @@ -3960,7 +3961,9 @@ "renderer PTY spawn qualifies local and SSH admissions with the owning execution host", "disconnected SSH kill and stop retain the lease and reject without a synthetic exit", "provider reconnect retries durable shutdown intent with its generation and pane identity", - "persisted local shutdown rebinds from the startup fallback to the daemon provider and drains all retry timers", + "persisted local shutdown waits for daemon startup instead of accepting unknown-id success from the fallback provider", + "an in-flight replaced local provider cannot complete or delete the newer shutdown owner", + "host-scoped liveness lists only the selected provider instead of fanning out to unrelated SSH hosts", "relay generation reuse removes stale retry ownership without touching the new PTY" ] }, @@ -4159,7 +4162,7 @@ }, "performanceBudget": { "required": true, - "evidence": "Shutdown removes indefinite deferred retention and preserves at most one in-flight native kill plus one existing graceful timer per PTY request. Failed local and SSH kills coalesce in main by exact live PTY id, while failed remote-runtime closes coalesce by environment, runtime generation, and handle: distinct durable records correspond one-for-one with still-unconfirmed provider resources and survive renderer/app restart. Each main-owned retry map owns one shared one-shot timer for its earliest deadline, retries all due owners, caps backoff at 30 seconds, and logs only attempts 1, 2, and 8 to prevent outage backpressure. Successful SSH-target and runtime-environment removal release unreachable records and cancel the last retry timer; provider replacement rebinds the same generation but relay/runtime generation replacement drops stale intent before touching a reused id. Remote-runtime close adds one generation status probe per teardown attempt and is not on input, output, render, polling, or startup-visible paths. Pre-handler exits remain capped at 64. Worktree-create, worktree-removal, whole-project, and host-scoped admissions retain one promise only per active operation; removal drains only the target worktree's current admissions. Targeted runtime stop still performs one provider verification per target PTY plus two liveness snapshots, while foreground probes are limited to the target worktree." + "evidence": "Shutdown removes indefinite deferred retention and preserves at most one in-flight native kill plus one existing graceful timer per PTY request. Failed local and SSH kills coalesce in main by exact live PTY id, while failed remote-runtime closes coalesce by environment, runtime generation, and handle: distinct durable records correspond one-for-one with still-unconfirmed provider resources and survive renderer/app restart. Each main-owned retry map owns one shared one-shot timer for its earliest deadline, retries all due owners, caps backoff at 30 seconds, and logs only attempts 1, 2, and 8 to prevent outage backpressure. Successful SSH-target and runtime-environment removal release unreachable records and cancel the last retry timer; provider replacement rebinds the same generation but relay/runtime generation replacement drops stale intent before touching a reused id. Remote-runtime close adds one generation status probe per teardown attempt and is not on input, output, render, polling, or startup-visible paths. Pre-handler exits remain capped at 64. Worktree-create, worktree-removal, whole-project, and host-scoped admissions retain one promise only per active operation; removal drains only the target worktree's current admissions. Targeted runtime stop performs two liveness snapshots against only the selected provider, while foreground probes are limited to the target worktree; whole-project inventory retains the all-provider fanout." }, "promotionCriteria": [ "Run the deterministic command across the required CI platforms without flakes.", diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 568572a595c..ae48f61811d 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -3355,6 +3355,30 @@ describe('registerPtyHandlers', () => { deletePtyOwnership('missing-pty') }) + it('lists only the selected provider for host-scoped liveness proof', async () => { + const localSessions = [{ id: 'local-pty', cwd: '/local', title: 'Shell' }] + const localListProcesses = vi.fn().mockResolvedValue(localSessions) + const unrelatedSshListProcesses = vi.fn().mockRejectedValue(new Error('unrelated SSH down')) + setLocalPtyProvider({ + listProcesses: localListProcesses, + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}) + } as never) + registerSshPtyProvider('ssh-b', { listProcesses: unrelatedSshListProcesses } as never) + const runtime = { setPtyController: vi.fn() } + handlers.clear() + registerPtyHandlers(mainWindow as never, runtime as never) + const controller = runtime.setPtyController.mock.calls[0]?.[0] as { + listProcesses: (connectionId?: string | null) => Promise + } + + await expect(controller.listProcesses(null)).resolves.toEqual(localSessions) + expect(localListProcesses).toHaveBeenCalledOnce() + expect(unrelatedSshListProcesses).not.toHaveBeenCalled() + await expect(controller.listProcesses('ssh-b')).rejects.toThrow('unrelated SSH down') + }) + it('persists local daemon shutdown and retries after renderer loss', async () => { vi.useFakeTimers() const shutdown = vi @@ -3425,6 +3449,7 @@ describe('registerPtyHandlers', () => { it('rebinds a persisted local shutdown after async daemon startup replaces the provider', async () => { vi.useFakeTimers() + const startup = makeDeferred() const makeProvider = (shutdown: ReturnType) => ({ spawn: vi.fn(), @@ -3436,11 +3461,53 @@ describe('registerPtyHandlers', () => { onReplay: vi.fn(() => () => {}), onExit: vi.fn(() => () => {}) }) as never - const fallbackShutdown = vi.fn().mockRejectedValue(new Error('temporary provider')) + const fallbackShutdown = vi.fn().mockResolvedValue(undefined) const daemonShutdown = vi.fn().mockResolvedValue(undefined) const removePendingLocalPtyShutdown = vi.fn() setLocalPtyProvider(makeProvider(fallbackShutdown)) handlers.clear() + registerPtyHandlers( + mainWindow as never, + undefined, + undefined, + undefined, + undefined, + { + getPendingLocalPtyShutdowns: () => [{ ptyId: 'daemon-pty', requestedAt: 1 }], + upsertPendingLocalPtyShutdown: vi.fn(), + removePendingLocalPtyShutdown + } as never, + { awaitLocalPtyStartup: () => startup.promise } + ) + await vi.advanceTimersByTimeAsync(0) + expect(fallbackShutdown).not.toHaveBeenCalled() + + setLocalPtyProvider(makeProvider(daemonShutdown)) + startup.resolve() + await vi.advanceTimersByTimeAsync(0) + + expect(fallbackShutdown).not.toHaveBeenCalled() + expect(daemonShutdown).toHaveBeenCalledOnce() + expect(removePendingLocalPtyShutdown).toHaveBeenCalledWith('daemon-pty') + await vi.advanceTimersByTimeAsync(30_000) + expect(vi.getTimerCount()).toBe(0) + vi.useRealTimers() + }) + + it('does not let an in-flight old provider complete persisted shutdown ownership', async () => { + vi.useFakeTimers() + const oldShutdown = makeDeferred() + const makeProvider = (shutdown: (id: string) => Promise) => + ({ + shutdown, + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}) + }) as never + const newShutdown = vi.fn().mockResolvedValue(undefined) + const removePendingLocalPtyShutdown = vi.fn() + setLocalPtyProvider(makeProvider(() => oldShutdown.promise)) + handlers.clear() registerPtyHandlers(mainWindow as never, undefined, undefined, undefined, undefined, { getPendingLocalPtyShutdowns: () => [{ ptyId: 'daemon-pty', requestedAt: 1 }], upsertPendingLocalPtyShutdown: vi.fn(), @@ -3448,12 +3515,12 @@ describe('registerPtyHandlers', () => { } as never) await vi.advanceTimersByTimeAsync(0) - setLocalPtyProvider(makeProvider(daemonShutdown)) + setLocalPtyProvider(makeProvider(newShutdown)) + oldShutdown.resolve() await vi.advanceTimersByTimeAsync(0) - expect(fallbackShutdown).toHaveBeenCalledOnce() - expect(daemonShutdown).toHaveBeenCalledOnce() - expect(removePendingLocalPtyShutdown).toHaveBeenCalledWith('daemon-pty') + expect(newShutdown).toHaveBeenCalledOnce() + expect(removePendingLocalPtyShutdown).toHaveBeenCalledOnce() await vi.advanceTimersByTimeAsync(30_000) expect(vi.getTimerCount()).toBe(0) vi.useRealTimers() diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 06f13bc29d1..e570bc2e52e 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -633,7 +633,13 @@ function attemptRetainedPtyShutdown(retained: RetainedPtyShutdown): Promise completeRetainedPtyShutdown(retained, providerExitObserved)) + .then((providerExitObserved) => { + // Why: provider replacement can finish while an older shutdown is in flight. + if (retained.provider !== currentProvider) { + return + } + completeRetainedPtyShutdown(retained, providerExitObserved) + }) .catch((error: unknown) => { const ownerMap = retained.connectionId ? pendingSshShutdownRetries @@ -666,7 +672,8 @@ function attemptRetainedPtyShutdown(retained: RetainedPtyShutdown): Promise | undefined + const getLocalPtyStartupPromise = (connectionId?: string | null): Promise | undefined => { + if (connectionId) { + return undefined + } + // Why: cold-start replay and local spawn must wait for the final daemon owner. + localPtyStartupPromise ??= options?.awaitLocalPtyStartup?.() + return localPtyStartupPromise + } notifyRetainedPtyExit = (id) => { runtime?.onPtyExit(id, -1) rememberSyntheticKillExit(id) @@ -1724,12 +1744,30 @@ export function registerPtyHandlers( ? store.getPendingLocalPtyShutdowns() : [] for (const request of persistedLocalShutdowns) { - const retained = retainLocalPtyShutdown(request.ptyId, { - immediate: true, - ...(request.expectedPaneKey ? { expectedPaneKey: request.expectedPaneKey } : {}), - ...(request.expectedTabId ? { expectedTabId: request.expectedTabId } : {}) - }) - void attemptRetainedPtyShutdown(retained) + const startupReady = getLocalPtyStartupPromise() + const retained = retainLocalPtyShutdown( + request.ptyId, + { + immediate: true, + ...(request.expectedPaneKey ? { expectedPaneKey: request.expectedPaneKey } : {}), + ...(request.expectedTabId ? { expectedTabId: request.expectedTabId } : {}) + }, + 0, + Boolean(startupReady) + ) + const retryWithReadyProvider = (): void => { + if (pendingLocalShutdownRetries.get(retained.id) !== retained) { + return + } + retained.nextRetryAt = 0 + void attemptRetainedPtyShutdown(retained) + } + // Why: a rejected startup barrier means the in-process fallback is the final owner. + if (startupReady) { + void startupReady.then(retryWithReadyProvider, retryWithReadyProvider) + } else { + retryWithReadyProvider() + } } for (const [connectionId, provider] of sshProviders) { retryPersistedSshShutdowns(connectionId, provider) @@ -1742,16 +1780,6 @@ export function registerPtyHandlers( resetRendererDeliveryAccountingForLifecycleReset = () => {} registerRendererLifecycleResetHandlers(mainWindow.webContents) - const getLocalPtyStartupPromise = (connectionId?: string | null): Promise | undefined => { - if (connectionId) { - return undefined - } - // Why: during desktop cold start the daemon provider swap now overlaps - // first paint. Local spawns must wait before resolving getProvider(), while - // SSH/headless paths do not use the desktop daemon. - return options?.awaitLocalPtyStartup?.() - } - // Remove any previously registered handlers so we can re-register them // (e.g. when macOS re-activates the app and creates a new window). ipcMain.removeHandler('pty:spawn') @@ -3682,7 +3710,17 @@ export function registerPtyHandlers( /* best effort: renderer clear still handles local PTYs */ } }, - listProcesses: async () => { + listProcesses: async (connectionId?: string | null) => { + if (connectionId === null) { + return localProvider.listProcesses() + } + if (connectionId) { + const provider = sshProviders.get(connectionId) + if (!provider) { + throw new Error(`SSH PTY provider not available: ${connectionId}`) + } + return provider.listProcesses() + } const providerSessions = await Promise.all([ localProvider.listProcesses(), ...Array.from(sshProviders.values(), (provider) => provider.listProcesses()) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 41006cbc873..1fc32eecb87 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -22233,6 +22233,33 @@ describe('OrcaRuntimeService', () => { expect(getForegroundProcess).not.toHaveBeenCalledWith('pty-unrelated') }) + it('scopes host-owned terminal stop liveness away from unrelated providers', async () => { + const runtime = new OrcaRuntimeService(store) + const processLists = [[{ id: 'pty-1', cwd: '/tmp/worktree-a', title: 'Shell' }], []] + const listProcesses = vi.fn(async (connectionId?: string | null) => { + if (connectionId !== null) { + throw new Error('unrelated SSH provider unavailable') + } + return processLists.shift() ?? [] + }) + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait: async (ptyId) => { + runtime.onPtyExit(ptyId, -1) + return true + }, + getForegroundProcess: async () => null, + listProcesses + }) + syncSinglePty(runtime, 'pty-1') + + await expect( + runtime.stopTerminalsForWorktree('id:repo-1::/tmp/worktree-a', { hostId: 'local' }) + ).resolves.toEqual({ stopped: 1 }) + expect(listProcesses.mock.calls).toEqual([[null], [null]]) + }) + it('blocks paired terminal admission until project stop and removal are atomic', async () => { const removeProject = vi.fn() const runtime = new OrcaRuntimeService({ ...store, removeProject } as never) @@ -22371,11 +22398,7 @@ describe('OrcaRuntimeService', () => { await Promise.resolve() await expect( - runtime.runWithTerminalCreateAdmission( - TEST_WORKTREE_ID, - async () => undefined, - 'local' - ) + runtime.runWithTerminalCreateAdmission(TEST_WORKTREE_ID, async () => undefined, 'local') ).rejects.toThrow('terminal_admission_blocked_for_project_removal') await expect( runtime.runWithTerminalCreateAdmission( diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 7b0565846ca..895ea239db9 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1199,7 +1199,7 @@ type RuntimePtyController = { hasChildProcesses?(ptyId: string): Promise clearBuffer?(ptyId: string): Promise resize?(ptyId: string, cols: number, rows: number): boolean - listProcesses?(): Promise + listProcesses?(connectionId?: string | null): Promise serializeBuffer?( ptyId: string, opts?: { scrollbackRows?: number; altScreenForcesZeroRows?: boolean } @@ -19207,7 +19207,8 @@ export class OrcaRuntimeService { } const refreshedPtyLiveness = await this.refreshPtyWorktreeRecordsFromController( resolvedWorktrees, - worktree.id + worktree.id, + connectionId ) if (!refreshedPtyLiveness) { throw new Error('terminal_liveness_unavailable') @@ -19229,7 +19230,8 @@ export class OrcaRuntimeService { } const postStopLiveness = await this.refreshPtyWorktreeRecordsFromController( resolvedWorktrees, - worktree.id + worktree.id, + connectionId ) if (!postStopLiveness) { throw new Error('terminal_stop_postcheck_unavailable') @@ -20600,13 +20602,14 @@ export class OrcaRuntimeService { */ private async refreshPtyWorktreeRecordsFromController( resolvedWorktrees: ResolvedWorktree[], - targetWorktreeId: string | null = null + targetWorktreeId: string | null = null, + connectionId?: string | null ): Promise | null> { if (!this.ptyController?.listProcesses) { return null } const sessionsResult = await withTimeoutResult( - this.ptyController.listProcesses(), + this.ptyController.listProcesses(connectionId), PTY_CONTROLLER_LIST_TIMEOUT_MS ) if (!sessionsResult.ok) {