From 74239e7f56c150b42dbd9e9edf52ca51b6614ff9 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 13 Jul 2026 23:09:04 -0700 Subject: [PATCH] fix terminal close retry persistence ownership --- src/main/ipc/pty.test.ts | 45 ++++++++++- src/main/ipc/pty.ts | 32 ++++++-- ...runtime-terminal-close-retry-owner.test.ts | 75 +++++++++++++++++++ .../ipc/runtime-terminal-close-retry-owner.ts | 72 ++++++++++-------- 4 files changed, 183 insertions(+), 41 deletions(-) diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 70bcb2ae336..d0443ac732f 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -3549,7 +3549,8 @@ describe('registerPtyHandlers', () => { }, markSshRemotePtyShutdownRequested: () => { throw new Error('SSH intent disk full') - } + }, + markSshRemotePtyLease: vi.fn() } as never) try { @@ -3575,6 +3576,42 @@ describe('registerPtyHandlers', () => { } }) + it('retries durable local intent removal without repeating provider shutdown', async () => { + vi.useFakeTimers() + const shutdown = vi.fn().mockResolvedValue(undefined) + const remove = vi + .fn() + .mockImplementationOnce(() => { + throw new Error('intent removal disk full') + }) + .mockReturnValue(undefined) + setLocalPtyProvider({ + shutdown, + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}) + } as never) + handlers.clear() + registerPtyHandlers(mainWindow as never, undefined, undefined, undefined, undefined, { + getPendingLocalPtyShutdowns: () => [], + upsertPendingLocalPtyShutdown: vi.fn(), + removePendingLocalPtyShutdown: remove + } as never) + + await expect(handlers.get('pty:kill')!(null, { id: 'local-remove-fail' })).rejects.toThrow( + 'intent removal disk full' + ) + expect(shutdown).toHaveBeenCalledOnce() + expect(remove).toHaveBeenCalledOnce() + await vi.advanceTimersByTimeAsync(250) + + expect(shutdown).toHaveBeenCalledOnce() + expect(remove).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(30_000) + expect(shutdown).toHaveBeenCalledOnce() + expect(remove).toHaveBeenCalledTimes(2) + }) + it('keeps accepted local kills until provider readback proves physical exit', async () => { vi.useFakeTimers() const shutdown = vi.fn().mockResolvedValue(undefined) @@ -3994,6 +4031,9 @@ describe('registerPtyHandlers', () => { const fallbackShutdown = vi.fn().mockResolvedValue(undefined) const daemonShutdown = vi.fn().mockResolvedValue(undefined) const removePendingLocalPtyShutdown = vi.fn() + const upsertPendingLocalPtyShutdown = vi.fn(() => { + throw new Error('startup replay must not rewrite durable intent') + }) setLocalPtyProvider(makeProvider(fallbackShutdown)) handlers.clear() registerPtyHandlers( @@ -4004,7 +4044,7 @@ describe('registerPtyHandlers', () => { undefined, { getPendingLocalPtyShutdowns: () => [{ ptyId: 'daemon-pty', requestedAt: 1 }], - upsertPendingLocalPtyShutdown: vi.fn(), + upsertPendingLocalPtyShutdown, removePendingLocalPtyShutdown } as never, { awaitLocalPtyStartup: () => startup.promise } @@ -4017,6 +4057,7 @@ describe('registerPtyHandlers', () => { await vi.advanceTimersByTimeAsync(0) expect(fallbackShutdown).not.toHaveBeenCalled() + expect(upsertPendingLocalPtyShutdown).not.toHaveBeenCalled() expect(daemonShutdown).toHaveBeenCalledOnce() expect(removePendingLocalPtyShutdown).toHaveBeenCalledWith('daemon-pty') await vi.advanceTimersByTimeAsync(30_000) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 3589b44a9c2..d880c0d8f8c 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -184,6 +184,8 @@ type RetainedPtyShutdown = { durable: boolean attempts: number shutdownAccepted: boolean + providerShutdownComplete: boolean + providerExitObserved: boolean requireExitProof: boolean nextRetryAt: number inFlight: Promise | null @@ -593,7 +595,7 @@ function finishPtyShutdown( ): void { clearProviderPtyState(id) if (connectionId) { - store?.markSshRemotePtyLease(connectionId, id, 'terminated') + store?.markSshRemotePtyLease?.(connectionId, id, 'terminated') } ptyOwnership.delete(id) markClaudePtyExited(id) @@ -643,11 +645,11 @@ function completeRetainedPtyShutdown( if (ownerMap.get(retained.id) !== retained) { return } - ownerMap.delete(retained.id) - finishPtyShutdown(retained.id, retained.connectionId, retained.store ?? undefined) if (!retained.connectionId) { retained.store?.removePendingLocalPtyShutdown?.(retained.id) } + finishPtyShutdown(retained.id, retained.connectionId, retained.store ?? undefined) + ownerMap.delete(retained.id) if (!providerExitObserved) { notifyRetainedPtyExit?.(retained.id) } @@ -688,6 +690,10 @@ function attemptRetainedPtyShutdown(retained: RetainedPtyShutdown): Promise { + if (retained.providerShutdownComplete) { + completeRetainedPtyShutdown(retained, retained.providerExitObserved) + return + } if (retained.shutdownAccepted) { const stopped = await verifyPtyStopped(currentProvider, retained.id, retained.options) if (retained.provider !== currentProvider) { @@ -697,6 +703,8 @@ function attemptRetainedPtyShutdown(retained: RetainedPtyShutdown): Promise { @@ -868,7 +880,8 @@ function retainLocalPtyShutdown( id: string, options: PtyShutdownOptions, attempts = 0, - deferUntilProviderReady = false + deferUntilProviderReady = false, + persist = true ): RetainedPtyShutdown { let retained = pendingLocalShutdownRetries.get(id) if (!retained || ptyShutdownIdentityConflicts(retained.options, options)) { @@ -883,6 +896,8 @@ function retainLocalPtyShutdown( durable: true, attempts, shutdownAccepted: false, + providerShutdownComplete: false, + providerExitObserved: false, requireExitProof: false, nextRetryAt: deferUntilProviderReady ? Number.POSITIVE_INFINITY @@ -899,7 +914,7 @@ function retainLocalPtyShutdown( // Why: disk failure must not strand the live process merely because its // crash-durable intent could not be refreshed. scheduleRetainedShutdownRetries() - if (typeof activePtyStore?.upsertPendingLocalPtyShutdown === 'function') { + if (persist && typeof activePtyStore?.upsertPendingLocalPtyShutdown === 'function') { activePtyStore.upsertPendingLocalPtyShutdown({ ptyId: id, ...(retained.options.expectedPaneKey @@ -942,6 +957,8 @@ function retainSshPtyShutdown( durable: persisted, attempts: 0, shutdownAccepted: false, + providerShutdownComplete: false, + providerExitObserved: false, requireExitProof: false, nextRetryAt: provider ? 0 : Number.POSITIVE_INFINITY, inFlight: null, @@ -1621,6 +1638,8 @@ function retryPersistedSshShutdowns(connectionId: string, provider: IPtyProvider durable: true, attempts: 0, shutdownAccepted: false, + providerShutdownComplete: false, + providerExitObserved: false, requireExitProof: false, nextRetryAt: 0, inFlight: null, @@ -2097,7 +2116,8 @@ export function registerPtyHandlers( ...(request.expectedTabId ? { expectedTabId: request.expectedTabId } : {}) }, 0, - Boolean(startupReady) + Boolean(startupReady), + false ) const retryWithReadyProvider = (): void => { if (pendingLocalShutdownRetries.get(retained.id) !== retained) { 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 c667aaa0feb..2ef914ab4ba 100644 --- a/src/main/ipc/runtime-terminal-close-retry-owner.test.ts +++ b/src/main/ipc/runtime-terminal-close-retry-owner.test.ts @@ -206,4 +206,79 @@ describe('runtime terminal close retry ownership', () => { expect(remove).toHaveBeenCalledWith('environment-1', 'terminal-1') expect(vi.getTimerCount()).toBe(0) }) + + it('hydrates persisted closes without rewriting their durable intent', async () => { + vi.useFakeTimers() + const upsert = vi.fn(() => { + throw new Error('startup replay must not rewrite durable intent') + }) + initializeRuntimeTerminalCloseRetryOwner( + { + getPendingRuntimeTerminalCloses: () => [ + { environmentId: 'environment-1', handle: 'terminal-1', runtimeId: 'runtime-a' } + ], + upsertPendingRuntimeTerminalClose: upsert, + removePendingRuntimeTerminalClose: vi.fn() + } as never, + '/tmp/orca-runtime-owner-test' + ) + getRuntimeEnvironmentStatusMock.mockResolvedValue({ + id: 'status-a', + ok: true, + result: { runtimeId: 'runtime-a' }, + _meta: { runtimeId: 'runtime-a' } + }) + callRuntimeEnvironmentMock.mockResolvedValue({ + id: 'close-a', + ok: true, + result: { close: true }, + _meta: { runtimeId: 'runtime-a' } + }) + + await vi.advanceTimersByTimeAsync(0) + + expect(upsert).not.toHaveBeenCalled() + expect(callRuntimeEnvironmentMock).toHaveBeenCalledOnce() + }) + + it('retries durable removal without repeating an accepted runtime close', async () => { + vi.useFakeTimers() + const remove = vi + .fn() + .mockImplementationOnce(() => { + throw new Error('runtime removal disk full') + }) + .mockReturnValue(undefined) + 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.mockResolvedValue({ + id: 'close-a', + ok: true, + result: { close: true }, + _meta: { runtimeId: 'runtime-a' } + }) + + await expect( + closeRuntimeTerminalWithRetryOwnership('environment-1', 'terminal-1', 'runtime-a') + ).rejects.toThrow('runtime removal disk full') + expect(callRuntimeEnvironmentMock).toHaveBeenCalledOnce() + expect(remove).toHaveBeenCalledOnce() + await vi.advanceTimersByTimeAsync(250) + + expect(callRuntimeEnvironmentMock).toHaveBeenCalledOnce() + expect(remove).toHaveBeenCalledTimes(2) + 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 74f0c9120e9..8b24a9d075a 100644 --- a/src/main/ipc/runtime-terminal-close-retry-owner.ts +++ b/src/main/ipc/runtime-terminal-close-retry-owner.ts @@ -12,6 +12,7 @@ type RetainedTerminalClose = { handle: string runtimeId: string | null attempts: number + closeComplete: boolean nextRetryAt: number inFlight: Promise> | null } @@ -38,8 +39,9 @@ function release(close: RetainedTerminalClose): void { if (!isCurrentOwner(close)) { return } - retainedCloses.delete(key(close.environmentId, close.handle)) + close.closeComplete = true store?.removePendingRuntimeTerminalClose?.(close.environmentId, close.handle) + retainedCloses.delete(key(close.environmentId, close.handle)) schedule() } @@ -88,38 +90,40 @@ function attempt(close: RetainedTerminalClose): Promise { - if (!isCurrentOwner(close)) { - return noOpResponse(status._meta?.runtimeId) - } - if (!status.ok) { - throw Object.assign(new Error(status.error.message), { response: status }) - } - const currentRuntimeId = status._meta.runtimeId - if (close.runtimeId && close.runtimeId !== currentRuntimeId) { - release(close) - return { - id: 'terminal.close', - ok: true as const, - result: { close: false, reason: 'runtime_replaced' }, - _meta: { runtimeId: currentRuntimeId } + const closeAttempt = close.closeComplete + ? Promise.resolve(noOpResponse(close.runtimeId)) + : getRuntimeEnvironmentStatus(userDataPath, close.environmentId).then((status) => { + if (!isCurrentOwner(close)) { + return noOpResponse(status._meta?.runtimeId) } - } - if (!close.runtimeId) { - close.runtimeId = currentRuntimeId - store?.upsertPendingRuntimeTerminalClose?.({ - environmentId: close.environmentId, - handle: close.handle, - runtimeId: currentRuntimeId, - requestedAt: Date.now() + if (!status.ok) { + throw Object.assign(new Error(status.error.message), { response: status }) + } + const currentRuntimeId = status._meta.runtimeId + if (close.runtimeId && close.runtimeId !== currentRuntimeId) { + release(close) + return { + id: 'terminal.close', + ok: true as const, + result: { close: false, reason: 'runtime_replaced' }, + _meta: { runtimeId: currentRuntimeId } + } + } + if (!close.runtimeId) { + close.runtimeId = currentRuntimeId + store?.upsertPendingRuntimeTerminalClose?.({ + environmentId: close.environmentId, + handle: close.handle, + runtimeId: currentRuntimeId, + requestedAt: Date.now() + }) + } + return callRuntimeEnvironment(userDataPath, close.environmentId, 'terminal.close', { + terminal: close.handle, + expectedRuntimeId: close.runtimeId }) - } - return callRuntimeEnvironment(userDataPath, close.environmentId, 'terminal.close', { - terminal: close.handle, - expectedRuntimeId: close.runtimeId }) - }) + const request = closeAttempt .then((response) => { if (!isCurrentOwner(close)) { return noOpResponse(response._meta?.runtimeId) @@ -187,7 +191,8 @@ function attempt(close: RetainedTerminalClose): Promise