From 2384370d2552d1e776c2b5bab1cf4c2fb90f355b Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 13 Jul 2026 11:43:21 -0700 Subject: [PATCH] fix: fence teardown from replacement sessions --- config/reliability-gates.jsonc | 34 ++++++- src/main/daemon/session.ts | 10 +++ src/main/daemon/terminal-host.test.ts | 28 ++++++ src/main/daemon/terminal-host.ts | 17 ++-- src/main/ipc/pty.test.ts | 89 +++++++++++++++++++ src/main/ipc/pty.ts | 11 +++ ...runtime-terminal-close-retry-owner.test.ts | 57 +++++++++++- .../ipc/runtime-terminal-close-retry-owner.ts | 39 +++++--- 8 files changed, 262 insertions(+), 23 deletions(-) diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index e066bd4a1e9..b5e37aec9c0 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -3686,7 +3686,8 @@ "file": "src/main/daemon/terminal-host.test.ts", "assertions": [ "a reused live daemon session rejects attach and kill from a replacement pane identity", - "native immediate force-kill failure preserves the live daemon session for retry" + "native immediate force-kill failure preserves the live daemon session for retry", + "same-id create waits for physical exit proof without disposing the terminating session owner" ] }, { @@ -3786,6 +3787,7 @@ "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 waits for daemon startup instead of accepting unknown-id success from the fallback provider", + "renderer and runtime same-id restore reject while startup teardown replay retains that exact PTY owner", "an in-flight replaced local provider cannot complete or delete the newer shutdown owner", "local and SSH stale-provider not-found rejections preserve replacement ownership and coalesced callers fail closed", "direct local and SSH kill requests share the retained owner and re-drive after in-flight provider replacement", @@ -3827,7 +3829,8 @@ { "file": "src/main/ipc/runtime-terminal-close-retry-owner.test.ts", "assertions": [ - "a late runtime close completion cannot delete a replacement-generation retry owner" + "a late runtime close completion cannot delete a replacement-generation retry owner", + "same-generation terminal_handle_stale releases durable ownership and cancels the last retry timer" ] }, { @@ -4163,6 +4166,33 @@ "result": "passed", "durationSeconds": 1.43, "summary": "219 tests passed including packaged relay patch parity, vanilla pre-ready red control, one-shot accepted close ownership, and delayed OS-death inventory reaping." + }, + { + "date": "2026-07-12", + "runner": "local", + "platform": "macos", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/providers/local-pty-provider.test.ts src/main/daemon/daemon-pty-adapter.test.ts src/main/daemon/daemon-pty-router.test.ts src/main/daemon/degraded-daemon-pty-provider.test.ts src/main/daemon/session.test.ts src/main/daemon/pty-subprocess.test.ts src/main/daemon/terminal-host.test.ts src/main/daemon/terminal-host-session-reaping-leak.test.ts src/main/daemon/daemon-server.test.ts src/main/ipc/pty.test.ts src/main/ipc/runtime-terminal-close-retry-owner.test.ts src/main/runtime/orca-runtime.test.ts", + "result": "passed", + "durationSeconds": 9.17, + "summary": "1442 tests passed: terminating same-id daemon sessions deferred creation until physical exit, renderer/runtime startup restore rejected retained teardown identities, and same-generation stale runtime handles released persistence plus the last timer." + }, + { + "date": "2026-07-12", + "runner": "local", + "platform": "macos", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts src/main/ipc/worktrees.test.ts src/main/ipc/pty.test.ts src/main/ipc/runtime-environments.test.ts src/main/runtime/rpc/terminal-close-generation.test.ts src/main/persistence.test.ts src/main/ssh/ssh-relay-session.test.ts src/main/ipc/ssh.test.ts src/renderer/src/store/slices/worktrees.test.ts", + "result": "passed", + "durationSeconds": 9.63, + "summary": "1891 tests passed across app-start teardown replay, local/SSH/runtime identity ownership, selected-provider liveness, and durable close settlement." + }, + { + "date": "2026-07-12", + "runner": "local", + "platform": "macos", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/shared/ssh-pty-id.test.ts src/shared/ssh-types.test.ts src/main/providers/local-pty-provider.test.ts src/main/providers/ssh-pty-provider.test.ts src/main/persistence.test.ts src/main/ssh/ssh-relay-session.test.ts src/main/ssh/ssh-relay-generation-reattach.test.ts src/main/ssh/ssh-target-id-migration.test.ts src/relay/pty-handler.test.ts src/relay/subprocess.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts src/renderer/src/lib/pty-kill-retry-ownership.test.ts src/renderer/src/lib/runtime-terminal-close-retry-ownership.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts src/renderer/src/lib/launch-agent-background-session.test.ts src/renderer/src/store/slices/repos.test.ts src/renderer/src/store/slices/repos-host-identity-routing.test.ts src/main/runtime/orca-runtime.test.ts src/main/ipc/pty.test.ts", + "result": "passed", + "durationSeconds": 10.49, + "summary": "1800 tests passed with exact same-id startup replay admission, retained pre-handler/liveness bounds, and cross-provider retry identity preserved." } ], "runtimeBudget": { diff --git a/src/main/daemon/session.ts b/src/main/daemon/session.ts index 318d8d6cc35..37e335e3031 100644 --- a/src/main/daemon/session.ts +++ b/src/main/daemon/session.ts @@ -139,6 +139,8 @@ export class Session { private producerPaused = false private producerPauseFailsafeTimer: ReturnType | null = null private readonly _historySeeded: boolean | undefined + private readonly exitProofPromise: Promise + private resolveExitProof!: () => void constructor(opts: SessionOptions) { this.sessionId = opts.sessionId @@ -148,6 +150,9 @@ export class Session { this.launchAgent = opts.launchAgent ?? null this.subprocess = opts.subprocess this.onSessionExit = opts.onExit + this.exitProofPromise = new Promise((resolve) => { + this.resolveExitProof = resolve + }) const size = normalizePtySize(opts.cols, opts.rows) this.emulator = new HeadlessEmulator({ cols: size.cols, @@ -202,6 +207,10 @@ export class Session { return this._isTerminating } + waitForExitProof(): Promise { + return this._state === 'exited' ? Promise.resolve() : this.exitProofPromise + } + get pid(): number { return this.subprocess.pid } @@ -653,6 +662,7 @@ export class Session { this._exitCode = code this._state = 'exited' + this.resolveExitProof() // Why resume:false — the child is reaped, so there is nothing to unblock; // only the failsafe timer must not outlive the session. this.releaseProducerPause({ resume: false }) diff --git a/src/main/daemon/terminal-host.test.ts b/src/main/daemon/terminal-host.test.ts index 2b71f97739d..d1987881684 100644 --- a/src/main/daemon/terminal-host.test.ts +++ b/src/main/daemon/terminal-host.test.ts @@ -128,6 +128,34 @@ describe('TerminalHost', () => { expect(spawnFn).toHaveBeenCalledOnce() }) + it('retains a terminating same-id session until physical exit proof', async () => { + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + const terminatingSubprocess = lastSubprocess + + host.kill('session-1', { immediate: true }) + const replacement = host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + await Promise.resolve() + + expect(spawnFn).toHaveBeenCalledOnce() + expect(terminatingSubprocess.forceKill).toHaveBeenCalledOnce() + expect(terminatingSubprocess.dispose).not.toHaveBeenCalled() + expect(host.listSessions()).toHaveLength(1) + + terminatingSubprocess._onExitCb?.(0) + await expect(replacement).resolves.toMatchObject({ isNew: true }) + expect(spawnFn).toHaveBeenCalledTimes(2) + }) + it('returns snapshot when attaching to existing session', async () => { await host.createOrAttach({ sessionId: 'session-1', diff --git a/src/main/daemon/terminal-host.ts b/src/main/daemon/terminal-host.ts index 3cfda1241b6..e56b92e8134 100644 --- a/src/main/daemon/terminal-host.ts +++ b/src/main/daemon/terminal-host.ts @@ -61,14 +61,15 @@ export class TerminalHost { * already deliver them through shell launch arguments. */ async createOrAttach(opts: CreateOrAttachOptions): Promise { - const existing = this.sessions.get(opts.sessionId) + let existing = this.sessions.get(opts.sessionId) - // Why: a session that has been asked to terminate (kill() called but the - // subprocess hasn't exited yet) must not be reattached. Reattaching would - // hand the caller a handle that races with the in-flight exit, and any - // subsequent operation (write/kill/resize) would fail once the subprocess - // finally exits. Treat terminating sessions the same as fully-exited ones. - if (existing && existing.isAlive && !existing.isTerminating) { + // Why: replacing a terminating session would dispose its exit listener and + // retry timer before physical exit proof, orphaning the old native process. + while (existing?.isAlive && existing.isTerminating) { + await existing.waitForExitProof() + existing = this.sessions.get(opts.sessionId) + } + if (existing?.isAlive) { assertPtyPaneIdentity( opts.sessionId, { paneKey: existing.paneKey, tabId: existing.tabId }, @@ -89,7 +90,7 @@ export class TerminalHost { } } - // Clean up dead session if present + // Clean up only a physically exited session if its exit reaper has not run yet. if (existing) { existing.dispose() this.sessions.delete(opts.sessionId) diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 06dfc248356..cfb02d1c9d9 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -3923,6 +3923,95 @@ describe('registerPtyHandlers', () => { vi.useRealTimers() }) + it('blocks renderer same-id restore until startup teardown replay completes', async () => { + const startup = makeDeferred() + const shutdownGate = makeDeferred() + const spawn = vi.fn().mockResolvedValue({ id: 'daemon-pty' }) + const shutdown = vi.fn(() => shutdownGate.promise) + const removePendingLocalPtyShutdown = vi.fn() + setLocalPtyProvider({ + spawn, + shutdown, + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}) + } as never) + 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 } + ) + + const restore = handlers.get('pty:spawn')!(null, { + sessionId: 'daemon-pty', + cols: 80, + rows: 24 + }) as Promise + startup.resolve() + + await expect(restore).rejects.toThrow('shutdown is still pending') + expect(shutdown).toHaveBeenCalledOnce() + expect(spawn).not.toHaveBeenCalled() + + shutdownGate.resolve() + await vi.waitFor(() => expect(removePendingLocalPtyShutdown).toHaveBeenCalledOnce()) + await expect( + handlers.get('pty:spawn')!(null, { sessionId: 'daemon-pty', cols: 80, rows: 24 }) + ).resolves.toMatchObject({ id: 'daemon-pty' }) + expect(spawn).toHaveBeenCalledOnce() + }) + + it('blocks runtime same-id restore while startup teardown replay owns the PTY', async () => { + const startup = makeDeferred() + const shutdownGate = makeDeferred() + const spawn = vi.fn().mockResolvedValue({ id: 'daemon-pty' }) + const shutdown = vi.fn(() => shutdownGate.promise) + const removePendingLocalPtyShutdown = vi.fn() + setLocalPtyProvider({ + spawn, + shutdown, + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}) + } as never) + const runtime = { setPtyController: vi.fn(), onPtyExit: vi.fn() } + handlers.clear() + registerPtyHandlers( + mainWindow as never, + runtime as never, + undefined, + undefined, + undefined, + { + getPendingLocalPtyShutdowns: () => [{ ptyId: 'daemon-pty', requestedAt: 1 }], + upsertPendingLocalPtyShutdown: vi.fn(), + removePendingLocalPtyShutdown + } as never, + { awaitLocalPtyStartup: () => startup.promise } + ) + const controller = runtime.setPtyController.mock.calls[0]?.[0] as { + spawn: (args: { sessionId: string; cols: number; rows: number }) => Promise + } + + const restore = controller.spawn({ sessionId: 'daemon-pty', cols: 80, rows: 24 }) + startup.resolve() + + await expect(restore).rejects.toThrow('shutdown is still pending') + expect(shutdown).toHaveBeenCalledOnce() + expect(spawn).not.toHaveBeenCalled() + shutdownGate.resolve() + await vi.waitFor(() => expect(removePendingLocalPtyShutdown).toHaveBeenCalledOnce()) + }) + it('does not let an in-flight old provider complete persisted shutdown ownership', async () => { vi.useFakeTimers() let rejectOldShutdown!: (error: Error) => void diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 63b1f478399..3ac5c72d5b2 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -195,6 +195,15 @@ const providerProcessListsInFlight = new WeakMap void>() let retainedShutdownRetryTimer: ReturnType | null = null const MAX_RETAINED_SHUTDOWN_BACKOFF_MS = 30_000 + +function assertPtySpawnHasNoRetainedShutdown(id: string | undefined): void { + if (id && (pendingLocalShutdownRetries.has(id) || pendingSshShutdownRetries.has(id))) { + // Why: replacing or attaching this exact id would strand the retained + // teardown owner on the older native process/provider generation. + throw new Error(`PTY "${id}" shutdown is still pending`) + } +} + const SYNTHETIC_KILL_EXIT_DUPLICATE_WINDOW_MS = 30_000 // Why: producer flow control changes terminal physics — a flooding shell now // blocks on write instead of buffering in main. Kill switch: flip this one @@ -3695,6 +3704,7 @@ export function registerPtyHandlers( : undefined } + assertPtySpawnHasNoRetainedShutdown(effectiveSessionAppId) const existingPaneSpawn = materializedPaneKey ? paneSpawnReservationsByPaneKey.get(materializedPaneKey) : undefined @@ -4592,6 +4602,7 @@ export function registerPtyHandlers( ? (getSettings()?.terminalWindowsPowerShellImplementation ?? 'auto') : undefined } + assertPtySpawnHasNoRetainedShutdown(effectiveSessionAppId) const existingPaneSpawn = reservationPaneKey ? paneSpawnReservationsByPaneKey.get(reservationPaneKey) : undefined diff --git a/src/main/ipc/runtime-terminal-close-retry-owner.test.ts b/src/main/ipc/runtime-terminal-close-retry-owner.test.ts index a7b1c80ad04..4e42378d93f 100644 --- a/src/main/ipc/runtime-terminal-close-retry-owner.test.ts +++ b/src/main/ipc/runtime-terminal-close-retry-owner.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { callRuntimeEnvironmentMock, getRuntimeEnvironmentStatusMock } = vi.hoisted(() => ({ callRuntimeEnvironmentMock: vi.fn(), @@ -24,6 +24,11 @@ function deferred(): { promise: Promise; resolve: (value: T) => void } { } describe('runtime terminal close retry ownership', () => { + beforeEach(() => { + callRuntimeEnvironmentMock.mockReset() + getRuntimeEnvironmentStatusMock.mockReset() + }) + afterEach(() => { initializeRuntimeTerminalCloseRetryOwner( { getPendingRuntimeTerminalCloses: () => [] } as never, @@ -115,4 +120,54 @@ describe('runtime terminal close retry ownership', () => { expect(remove).toHaveBeenCalledOnce() expect(vi.getTimerCount()).toBe(0) }) + + it('releases a same-runtime stale handle without another retry', async () => { + vi.useFakeTimers() + const remove = vi.fn() + initializeRuntimeTerminalCloseRetryOwner( + { + getPendingRuntimeTerminalCloses: () => [], + upsertPendingRuntimeTerminalClose: vi.fn(), + removePendingRuntimeTerminalClose: remove + } as never, + '/tmp/orca-runtime-owner-test' + ) + getRuntimeEnvironmentStatusMock.mockResolvedValue({ + id: 'status-a', + ok: true, + result: { runtimeId: 'runtime-a' }, + _meta: { runtimeId: 'runtime-a' } + }) + callRuntimeEnvironmentMock + .mockResolvedValueOnce({ + id: 'close-transient', + ok: false, + error: { code: 'runtime_unavailable', message: 'runtime unavailable' }, + _meta: { runtimeId: 'runtime-a' } + }) + .mockResolvedValueOnce({ + id: 'close-a', + ok: false, + error: { code: 'terminal_handle_stale', message: 'terminal_handle_stale' }, + _meta: { runtimeId: 'runtime-a' } + }) + + await expect( + closeRuntimeTerminalWithRetryOwnership('environment-1', 'terminal-1', 'runtime-a') + ).resolves.toMatchObject({ ok: false, error: { code: 'runtime_unavailable' } }) + expect(vi.getTimerCount()).toBe(1) + + await expect( + closeRuntimeTerminalWithRetryOwnership('environment-1', 'terminal-1', 'runtime-a') + ).resolves.toMatchObject({ + ok: true, + result: { close: false, reason: 'terminal_handle_stale' } + }) + expect(vi.getTimerCount()).toBe(0) + await vi.advanceTimersByTimeAsync(60_000) + + expect(callRuntimeEnvironmentMock).toHaveBeenCalledTimes(2) + expect(remove).toHaveBeenCalledWith('environment-1', 'terminal-1') + expect(vi.getTimerCount()).toBe(0) + }) }) diff --git a/src/main/ipc/runtime-terminal-close-retry-owner.ts b/src/main/ipc/runtime-terminal-close-retry-owner.ts index e628c16ae3d..63b8162da4d 100644 --- a/src/main/ipc/runtime-terminal-close-retry-owner.ts +++ b/src/main/ipc/runtime-terminal-close-retry-owner.ts @@ -34,6 +34,15 @@ function isCurrentOwner(close: RetainedTerminalClose): boolean { ) } +function release(close: RetainedTerminalClose): void { + if (!isCurrentOwner(close)) { + return + } + retainedCloses.delete(key(close.environmentId, close.handle)) + store?.removePendingRuntimeTerminalClose?.(close.environmentId, close.handle) + schedule() +} + function schedule(): void { if (retryTimer) { clearTimeout(retryTimer) @@ -67,10 +76,12 @@ function attempt(close: RetainedTerminalClose): Promise ({ id: 'terminal.close', ok: true as const, @@ -87,8 +98,7 @@ function attempt(close: RetainedTerminalClose): Promise { @@ -140,8 +156,7 @@ function attempt(close: RetainedTerminalClose): Promise