fix: retry retained terminal shutdowns

This commit is contained in:
Brennan Benson
2026-07-12 17:54:43 -07:00
parent dead0c7cd2
commit e257e1fd81
11 changed files with 243 additions and 22 deletions
+1
View File
@@ -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'
+2 -1
View File
@@ -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, {
+2
View File
@@ -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
})
+2 -3
View File
@@ -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
@@ -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<void> {
return closeRuntimeTerminalRetainingRetryOwnership(
{ kind: 'environment', environmentId },
handle
)
}
@@ -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 })
@@ -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)
}
@@ -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)
})
})
@@ -13,6 +13,34 @@ export type PtyKillIdentity = {
const MAX_RETRY_BACKOFF_MS = 30_000
const retainedPtyKills = new Map<string, RetainedPtyKill>()
let retryTimer: ReturnType<typeof setTimeout> | 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()
}
@@ -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'
})
})
})
@@ -0,0 +1,106 @@
import {
callRuntimeRpc,
type RuntimeClientTarget
} from '@/runtime/runtime-rpc-client'
type EnvironmentTarget = Extract<RuntimeClientTarget, { kind: 'environment' }>
type RetainedRuntimeTerminalClose = {
target: EnvironmentTarget
handle: string
inFlight: Promise<void> | null
attempts: number
nextRetryAt: number
}
const MAX_RETRY_BACKOFF_MS = 30_000
const retainedCloses = new Map<string, RetainedRuntimeTerminalClose>()
let retryTimer: ReturnType<typeof setTimeout> | 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<void> {
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()
}