diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 39d238ea37c..1312d06be44 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -6672,6 +6672,7 @@ describe('registerWorktreeHandlers', () => { }) expect(runHookMock).not.toHaveBeenCalled() expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(runtimeStub.verifyTerminalsStoppedForRemoval).toHaveBeenCalledWith(worktreeId) expect(removeWorktreeMock).not.toHaveBeenCalled() expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { cwd: '/workspace/repo' diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 10bf17f75a1..8115df95de9 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -1639,6 +1639,7 @@ export function registerWorktreeHandlers( removedMeta && (await isAlreadyRemovedWorktreePath(repo, canonicalWorktreePath, localWorktreeGitOptions)) ) { + await verifyTerminalsStopped(args.worktreeId) const removalResult = await removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ canonicalWorktreePath, repoPath: repo.path, @@ -1780,8 +1781,8 @@ export function registerWorktreeHandlers( await closeLocalWatcherForRemoval(canonicalWorktreePath) + await verifyTerminalsStopped(args.worktreeId) if (shouldTearDownPtys) { - await verifyTerminalsStopped(args.worktreeId) // Why: once preflight proves normal deletion is clean, kill PTYs before // git-level removal so Windows handles cannot keep the directory busy. await killAllProcessesForWorktree(args.worktreeId, { diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index c00dbc80895..3b26a3872d9 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -28081,6 +28081,7 @@ describe('OrcaRuntimeService', () => { const worktreeId = `${TEST_REPO_ID}::${missingWorktreePath}` const { runtimeStore, removeWorktreeMeta } = createStaleRuntimeWorktreeStore(worktreeId) const runtime = new OrcaRuntimeService(runtimeStore as never) + const verifyStopped = vi.spyOn(runtime, 'verifyTerminalsStoppedForRemoval') const registeredWorktrees = [ { path: TEST_REPO_PATH, @@ -28117,6 +28118,7 @@ describe('OrcaRuntimeService', () => { }) expect(runHook).not.toHaveBeenCalled() expect(removeWorktree).not.toHaveBeenCalled() + expect(verifyStopped).toHaveBeenCalledWith(worktreeId) expect(gitSpy).toHaveBeenCalledWith(['worktree', 'prune'], { cwd: TEST_REPO_PATH }) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 3a2135b60a6..907787c80e4 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -17315,6 +17315,7 @@ export class OrcaRuntimeService { removedMeta && (await isRuntimeWorktreePathMissing(repo, canonicalWorktreePath, localWorktreeGitOptions)) ) { + await this.verifyTerminalsStoppedForRemoval(removalTarget.id) const removalResult = await removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ canonicalWorktreePath, repoPath: repo.path, @@ -17434,9 +17435,7 @@ export class OrcaRuntimeService { await closeLocalWatcherForWorktreePath(canonicalWorktreePath).catch((err) => { console.warn(`[filesystem-watcher] failed to close ${canonicalWorktreePath}:`, err) }) - if (shouldTearDownPtys) { - await this.verifyTerminalsStoppedForRemoval(removalTarget.id) - } + await this.verifyTerminalsStoppedForRemoval(removalTarget.id) if (localProvider && shouldTearDownPtys) { // Why: once preflight proves normal deletion is clean, kill PTYs before // git-level removal so Windows handles cannot keep the directory busy. This also diff --git a/src/renderer/src/lib/agent-background-session-cleanup.ts b/src/renderer/src/lib/agent-background-session-cleanup.ts index 3c07e96ec48..c20ea9f64d7 100644 --- a/src/renderer/src/lib/agent-background-session-cleanup.ts +++ b/src/renderer/src/lib/agent-background-session-cleanup.ts @@ -1,4 +1,5 @@ import { killPtyRetainingRetryOwnership } from '@/lib/pty-kill-retry-ownership' +import { closeRuntimeTerminalRetainingRetryOwnership } from '@/lib/runtime-terminal-close-retry-ownership' export function runBestEffortAgentBackgroundCleanups(...actions: (() => void)[]): void { for (const action of actions) { @@ -15,3 +16,13 @@ export function killFailedAgentBackgroundPty(ptyId: string, tabId: string): Prom expectedTabId: tabId }) } + +export function closeFailedAgentBackgroundRuntimeTerminal( + environmentId: string, + handle: string +): Promise { + return closeRuntimeTerminalRetainingRetryOwnership( + { kind: 'environment', environmentId }, + handle + ) +} diff --git a/src/renderer/src/lib/launch-agent-background-session.test.ts b/src/renderer/src/lib/launch-agent-background-session.test.ts index 3b7eef1ce45..de5d96b5eca 100644 --- a/src/renderer/src/lib/launch-agent-background-session.test.ts +++ b/src/renderer/src/lib/launch-agent-background-session.test.ts @@ -793,8 +793,25 @@ describe('launchAgentBackgroundSession', () => { activeRuntimeEnvironmentId: 'env-1', terminalMainSideEffectAuthority: undefined } + let closeAttempts = 0 + mockRuntimeEnvironmentCall.mockImplementation(async (args) => { + if (args.method === 'terminal.close') { + closeAttempts += 1 + if (closeAttempts === 1) { + throw new Error('runtime disconnected') + } + return { ok: true, result: { status: 'closed' } } + } + return { + ok: true, + result: { terminal: { handle: 'terminal-1', worktreeId: 'wt-1', title: null } } + } + }) mockRuntimeEnvironmentSubscribe.mockRejectedValueOnce(new Error('subscription failed')) const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + const { retryRetainedRuntimeTerminalCloses } = await import( + './runtime-terminal-close-retry-ownership' + ) await expect( launchAgentBackgroundSession({ @@ -810,6 +827,8 @@ describe('launchAgentBackgroundSession', () => { params: { terminal: 'terminal-1' }, timeoutMs: undefined }) + retryRetainedRuntimeTerminalCloses() + await vi.waitFor(() => expect(closeAttempts).toBe(2)) expect(state.clearTabPtyId).toHaveBeenCalledWith('tab-1', 'remote:env-1@@terminal-1') expect(state.clearAgentLaunchConfig).toHaveBeenCalledWith(expect.stringMatching(/^tab-1:/)) expect(mockCloseTab).toHaveBeenCalledWith('tab-1', { recordInteraction: false }) diff --git a/src/renderer/src/lib/launch-agent-background-session.ts b/src/renderer/src/lib/launch-agent-background-session.ts index 72e5dd8585d..bc85031b547 100644 --- a/src/renderer/src/lib/launch-agent-background-session.ts +++ b/src/renderer/src/lib/launch-agent-background-session.ts @@ -319,9 +319,7 @@ export async function launchAgentBackgroundSession( if (ptyId) { try { if (runtimeTarget.kind === 'environment' && runtimeTerminalHandle) { - await callRuntimeRpc(runtimeTarget, 'terminal.close', { - terminal: runtimeTerminalHandle - }) + await backgroundCleanup.closeFailedAgentBackgroundRuntimeTerminal(runtimeTarget.environmentId, runtimeTerminalHandle) } else if (runtimeTarget.kind === 'local') { await backgroundCleanup.killFailedAgentBackgroundPty(ptyId, tab.id) } diff --git a/src/renderer/src/lib/pty-kill-retry-ownership.test.ts b/src/renderer/src/lib/pty-kill-retry-ownership.test.ts index be2f0f0469d..77d815f4660 100644 --- a/src/renderer/src/lib/pty-kill-retry-ownership.test.ts +++ b/src/renderer/src/lib/pty-kill-retry-ownership.test.ts @@ -1,8 +1,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { killPtyRetainingRetryOwnership, - releaseRetainedPtyKillOwnership, - retryRetainedPtyKills + releaseRetainedPtyKillOwnership } from './pty-kill-retry-ownership' const IDS = Array.from({ length: 65 }, (_, index) => `pty-retained-${index}`) @@ -14,9 +13,11 @@ describe('PTY kill retry ownership', () => { } vi.unstubAllGlobals() vi.restoreAllMocks() + vi.useRealTimers() }) - it('retains every live id and retries at most one per lifecycle event', async () => { + it('retains every live id and retries all due owners after bounded backoff', async () => { + vi.useFakeTimers() const kill = vi.fn().mockRejectedValue(new Error('provider disconnected')) vi.stubGlobal('window', { api: { pty: { kill } } }) vi.spyOn(console, 'warn').mockImplementation(() => {}) @@ -26,15 +27,28 @@ describe('PTY kill retry ownership', () => { } kill.mockResolvedValue(undefined) - for (const _id of IDS) { - retryRetainedPtyKills() - await Promise.resolve() - await Promise.resolve() - } + await vi.advanceTimersByTimeAsync(250) expect(kill).toHaveBeenCalledTimes(IDS.length * 2) for (const id of IDS) { expect(kill.mock.calls.filter(([calledId]) => calledId === id)).toHaveLength(2) } }) + + it('retries after a second failure without requiring another lifecycle event', async () => { + vi.useFakeTimers() + const kill = vi + .fn() + .mockRejectedValueOnce(new Error('offline')) + .mockRejectedValueOnce(new Error('still offline')) + .mockResolvedValue(undefined) + vi.stubGlobal('window', { api: { pty: { kill } } }) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + + await killPtyRetainingRetryOwnership(IDS[0], '[pty] failed').catch(() => {}) + await vi.advanceTimersByTimeAsync(250) + expect(kill).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(500) + expect(kill).toHaveBeenCalledTimes(3) + }) }) diff --git a/src/renderer/src/lib/pty-kill-retry-ownership.ts b/src/renderer/src/lib/pty-kill-retry-ownership.ts index 5b910020553..699af1b508b 100644 --- a/src/renderer/src/lib/pty-kill-retry-ownership.ts +++ b/src/renderer/src/lib/pty-kill-retry-ownership.ts @@ -13,6 +13,34 @@ export type PtyKillIdentity = { const MAX_RETRY_BACKOFF_MS = 30_000 const retainedPtyKills = new Map() +let retryTimer: ReturnType | null = null + +function scheduleRetainedPtyKillRetries(): void { + if (retryTimer) { + clearTimeout(retryTimer) + retryTimer = null + } + let nextRetryAt = Number.POSITIVE_INFINITY + for (const retained of retainedPtyKills.values()) { + if (!retained.inFlight) { + nextRetryAt = Math.min(nextRetryAt, retained.nextRetryAt) + } + } + if (!Number.isFinite(nextRetryAt)) { + return + } + retryTimer = setTimeout(() => { + retryTimer = null + const now = Date.now() + for (const [id, retained] of retainedPtyKills) { + if (!retained.inFlight && retained.nextRetryAt <= now) { + void killPtyRetainingRetryOwnership(id, retained.diagnostic, retained.options).catch( + () => {} + ) + } + } + }, Math.max(0, nextRetryAt - Date.now())) +} /** * Keep exact PTY identity until the owning provider accepts shutdown. A rejected @@ -47,17 +75,20 @@ export function killPtyRetainingRetryOwnership( .catch((error: unknown) => { retained.attempts += 1 retained.nextRetryAt = - retained.attempts === 1 - ? 0 - : Date.now() + - Math.min(MAX_RETRY_BACKOFF_MS, 250 * 2 ** Math.min(retained.attempts - 1, 7)) - console.warn(retained.diagnostic, error) + Date.now() + + Math.min(MAX_RETRY_BACKOFF_MS, 250 * 2 ** Math.min(retained.attempts - 1, 7)) + // Why: automatic retries can outlive a disconnect; bound diagnostics so + // a long outage cannot become a log-backpressure loop. + if (retained.attempts <= 2 || retained.attempts === 8) { + console.warn(retained.diagnostic, error) + } throw error }) .finally(() => { if (retained.inFlight === attempt) { retained.inFlight = null } + scheduleRetainedPtyKillRetries() }) retained.inFlight = attempt return attempt @@ -65,9 +96,8 @@ export function killPtyRetainingRetryOwnership( /** Retry on the next PTY lifecycle event; no polling or permanent timer is added. */ export function retryRetainedPtyKills(): void { - const now = Date.now() for (const [id, retained] of retainedPtyKills) { - if (!retained.inFlight && retained.nextRetryAt <= now) { + if (!retained.inFlight) { void killPtyRetainingRetryOwnership(id, retained.diagnostic, retained.options).catch(() => {}) return } @@ -76,4 +106,5 @@ export function retryRetainedPtyKills(): void { export function releaseRetainedPtyKillOwnership(id: string): void { retainedPtyKills.delete(id) + scheduleRetainedPtyKillRetries() } diff --git a/src/renderer/src/lib/runtime-terminal-close-retry-ownership.test.ts b/src/renderer/src/lib/runtime-terminal-close-retry-ownership.test.ts new file mode 100644 index 00000000000..e480a78ce48 --- /dev/null +++ b/src/renderer/src/lib/runtime-terminal-close-retry-ownership.test.ts @@ -0,0 +1,39 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' + +const callRuntimeRpc = vi.hoisted(() => vi.fn()) + +vi.mock('@/runtime/runtime-rpc-client', () => ({ callRuntimeRpc })) + +import { + closeRuntimeTerminalRetainingRetryOwnership, + releaseRetainedRuntimeTerminalClose +} from './runtime-terminal-close-retry-ownership' + +const TARGET = { kind: 'environment' as const, environmentId: 'env-1' } + +describe('runtime terminal close retry ownership', () => { + afterEach(() => { + releaseRetainedRuntimeTerminalClose(TARGET, 'terminal-1') + vi.restoreAllMocks() + vi.useRealTimers() + }) + + it('retries a retained handle after repeated failure without another lifecycle event', async () => { + vi.useFakeTimers() + callRuntimeRpc + .mockRejectedValueOnce(new Error('offline')) + .mockRejectedValueOnce(new Error('still offline')) + .mockResolvedValue(undefined) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + + await closeRuntimeTerminalRetainingRetryOwnership(TARGET, 'terminal-1').catch(() => {}) + await vi.advanceTimersByTimeAsync(250) + expect(callRuntimeRpc).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(500) + + expect(callRuntimeRpc).toHaveBeenCalledTimes(3) + expect(callRuntimeRpc).toHaveBeenLastCalledWith(TARGET, 'terminal.close', { + terminal: 'terminal-1' + }) + }) +}) diff --git a/src/renderer/src/lib/runtime-terminal-close-retry-ownership.ts b/src/renderer/src/lib/runtime-terminal-close-retry-ownership.ts new file mode 100644 index 00000000000..a173ab8f965 --- /dev/null +++ b/src/renderer/src/lib/runtime-terminal-close-retry-ownership.ts @@ -0,0 +1,106 @@ +import { + callRuntimeRpc, + type RuntimeClientTarget +} from '@/runtime/runtime-rpc-client' + +type EnvironmentTarget = Extract + +type RetainedRuntimeTerminalClose = { + target: EnvironmentTarget + handle: string + inFlight: Promise | null + attempts: number + nextRetryAt: number +} + +const MAX_RETRY_BACKOFF_MS = 30_000 +const retainedCloses = new Map() +let retryTimer: ReturnType | null = null + +function retainedCloseKey(target: EnvironmentTarget, handle: string): string { + return `${target.environmentId}\0${handle}` +} + +function scheduleRuntimeTerminalCloseRetries(): void { + if (retryTimer) { + clearTimeout(retryTimer) + retryTimer = null + } + let nextRetryAt = Number.POSITIVE_INFINITY + for (const retained of retainedCloses.values()) { + if (!retained.inFlight) { + nextRetryAt = Math.min(nextRetryAt, retained.nextRetryAt) + } + } + if (!Number.isFinite(nextRetryAt)) { + return + } + retryTimer = setTimeout(() => { + retryTimer = null + const now = Date.now() + for (const retained of retainedCloses.values()) { + if (!retained.inFlight && retained.nextRetryAt <= now) { + void closeRuntimeTerminalRetainingRetryOwnership(retained.target, retained.handle).catch( + () => {} + ) + } + } + }, Math.max(0, nextRetryAt - Date.now())) +} + +export function closeRuntimeTerminalRetainingRetryOwnership( + target: EnvironmentTarget, + handle: string +): Promise { + const key = retainedCloseKey(target, handle) + let retained = retainedCloses.get(key) + if (!retained) { + retained = { target, handle, inFlight: null, attempts: 0, nextRetryAt: 0 } + retainedCloses.set(key, retained) + } + if (retained.inFlight) { + return retained.inFlight + } + + const attempt = callRuntimeRpc(target, 'terminal.close', { terminal: handle }) + .then(() => { + retainedCloses.delete(key) + }) + .catch((error: unknown) => { + retained.attempts += 1 + retained.nextRetryAt = + Date.now() + + Math.min(MAX_RETRY_BACKOFF_MS, 250 * 2 ** Math.min(retained.attempts - 1, 7)) + if (retained.attempts <= 2 || retained.attempts === 8) { + console.warn('[terminal] Failed to close retained runtime terminal', error) + } + throw error + }) + .finally(() => { + if (retained.inFlight === attempt) { + retained.inFlight = null + } + scheduleRuntimeTerminalCloseRetries() + }) + retained.inFlight = attempt + return attempt +} + +export function retryRetainedRuntimeTerminalCloses(): void { + for (const retained of retainedCloses.values()) { + if (!retained.inFlight) { + void closeRuntimeTerminalRetainingRetryOwnership(retained.target, retained.handle).catch( + () => {} + ) + return + } + } +} + +export function releaseRetainedRuntimeTerminalClose( + target: EnvironmentTarget, + handle: string +): void { + retainedCloses.delete(retainedCloseKey(target, handle)) + scheduleRuntimeTerminalCloseRetries() +}