test(runtime): stop worker-recovery retries from scanning inside later tests (#22567)

The legacy worker terminal recovery retry re-arms itself on a 1s..30s backoff
for as long as a dispatch stays deferred. Nothing in the runtime suite ever
resolves one, so a single test armed a loop that kept re-running recovery --
and the worktree scans it issues -- through the shared `listWorktrees` stub for
the rest of the file's run, landing inside whichever test was executing when the
timer fired. That is what made `lineage-and-scan-cache-part-03`'s per-repo TTL
test see 4 scans instead of 3 only under CI load.

Give the controller a `cancelAllRetries`, track controllers while they have a
timer armed, and cancel them from the shared runtime test lifecycle reset.

Measured over the whole 1299-test file with an afterEach probe: 25 stray
post-test `listWorktrees` calls before, 0 after.
This commit is contained in:
Neil
2026-09-23 16:44:01 -07:00
committed by GitHub
parent 6847390c0a
commit 12a7ffdc64
5 changed files with 125 additions and 1 deletions
@@ -7,7 +7,8 @@ const { addGitHubPRReviewCommentReplyMock, addGitLabIssueCommentMock, addGitLabM
const { addGitLabMRInlineCommentMock, addSparseWorktree, addWorktree, advertisedUrlWatcher } = mocks
const { afterEach, applyAgentStatusHooksEnabledMock, assertWorktreeCleanForRemoval, beforeEach } =
mocks
const { clearConfiguredWorktreeSharedDirectoriesCacheForTests, closeGitLabMRMock } = mocks
const { cancelLegacyWorkerTerminalRecoveryRetriesForTests, closeGitLabMRMock } = mocks
const { clearConfiguredWorktreeSharedDirectoriesCacheForTests } = mocks
const { closeLocalWatcherForWorktreePathMock, closeRemoteWatcherForWorktreePathMock } = mocks
const { computeWorktreePathMock, countGitHubWorkItemsMock, createGitHubIssueMock } = mocks
const { createGitLabIssueMock, createHostedReviewMock, createSetupRunnerScript } = mocks
@@ -82,6 +83,9 @@ function resetRuntimeTestMocks(): void {
getPath: () => electronMocks.app.getPath(),
isPackaged: () => electronMocks.app.isPackaged
})
// Why: a worker-recovery retry re-arms itself for as long as a deferred worker exists, so one
// left armed keeps rescanning worktrees through the shared git stubs for the rest of the run.
cancelLegacyWorkerTerminalRecoveryRetriesForTests()
clearConfiguredWorktreeSharedDirectoriesCacheForTests()
_resetTerminalViewAttributesForTest()
advertisedUrlWatcher.clear()
@@ -61,6 +61,8 @@ export const beforeEach = importedValues.exportedBeforeEach
export const beginWatcherInstall = importedValues.exportedBeginWatcherInstall
export const buildAgentPromptPasteBytes = importedValues.exportedBuildAgentPromptPasteBytes
export const buildPreview = importedValues.exportedBuildPreview
export const cancelLegacyWorkerTerminalRecoveryRetriesForTests =
importedValues.exportedCancelLegacyWorkerTerminalRecoveryRetriesForTests
export const clearConfiguredWorktreeSharedDirectoriesCacheForTests =
importedValues.exportedClearConfiguredWorktreeSharedDirectoriesCacheForTests
export const clearSubmodulePathsCacheForTests =
@@ -7,6 +7,7 @@ import {
setRuntimeBrowserCommandsFactory,
setRuntimeBrowserUnavailableCause
} from '../runtime-browser-commands-factory'
import { __cancelLegacyWorkerTerminalRecoveryRetriesForTests } from '../runtime-legacy-worker-terminal-recovery-controller'
import { setRuntimeTerminalUnavailableCause } from '../native-terminal-availability'
import { setRuntimeDesktopSurface } from '../runtime-desktop-surface'
import { installFakeAppEnvironment } from '../../../../config/scripts/vitest-host-ports-setup'
@@ -178,6 +179,8 @@ export const exportedBeforeEach = beforeEach
export const exportedBeginWatcherInstall = beginWatcherInstall
export const exportedBuildAgentPromptPasteBytes = buildAgentPromptPasteBytes
export const exportedBuildPreview = buildPreview
export const exportedCancelLegacyWorkerTerminalRecoveryRetriesForTests =
__cancelLegacyWorkerTerminalRecoveryRetriesForTests
export const exportedClearConfiguredWorktreeSharedDirectoriesCacheForTests =
clearConfiguredWorktreeSharedDirectoriesCacheForTests
export const exportedClearSubmodulePathsCacheForTests = clearSubmodulePathsCacheForTests
@@ -14,6 +14,19 @@ type RecoveryRetry = {
timer: ReturnType<typeof setTimeout> | null
}
// Why a module-level set: a retry re-arms itself until its deferred worker materializes, so a host
// that never resolves one keeps a recovery loop running with no handle on it. A controller joins
// only while it has a timer armed and leaves as soon as it has none, so nothing is retained past
// the loop it belongs to.
const controllersWithArmedRetries = new Set<RuntimeLegacyWorkerTerminalRecoveryController>()
/** Stop every armed recovery retry. Test-only: a retry loop must not outlive the test that armed it. */
export function __cancelLegacyWorkerTerminalRecoveryRetriesForTests(): void {
for (const controller of Array.from(controllersWithArmedRetries)) {
controller.cancelAllRetries()
}
}
export class RuntimeLegacyWorkerTerminalRecoveryController {
private queue: Promise<void> = Promise.resolve()
private readonly retries = new Map<string, RecoveryRetry>()
@@ -48,6 +61,15 @@ export class RuntimeLegacyWorkerTerminalRecoveryController {
clearTimeout(retry.timer)
}
this.retries.delete(scopeKey)
if (this.retries.size === 0) {
controllersWithArmedRetries.delete(this)
}
}
cancelAllRetries(): void {
for (const scopeKey of Array.from(this.retries.keys())) {
this.cancelScope(scopeKey)
}
}
updateRetry(
@@ -126,5 +148,6 @@ export class RuntimeLegacyWorkerTerminalRecoveryController {
})
}, delayMs)
retry.timer.unref?.()
controllersWithArmedRetries.add(this)
}
}
@@ -0,0 +1,92 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import {
RuntimeLegacyWorkerTerminalRecoveryController,
__cancelLegacyWorkerTerminalRecoveryRetriesForTests
} from './runtime-legacy-worker-terminal-recovery-controller'
import type {
LegacyWorkerRecoveryPorts,
LegacyWorkerTerminalRecoveryResult
} from './runtime-legacy-worker-terminal-recovery-types'
import type { LegacyWorkerTerminalRecoveryPlan } from './orchestration/orchestration-legacy-worker-terminal-recovery'
const DEFERRED_DISPATCH_ID = 'dispatch-1'
const DEFERRED_PLAN: LegacyWorkerTerminalRecoveryPlan = {
ambiguousDispatchIds: [],
candidates: [
{
dispatchId: DEFERRED_DISPATCH_ID,
dispatchStatus: 'dispatched',
contractVersion: 1,
taskId: 'task-1',
worktreeId: 'repo-1::/tmp/worktree-a',
terminalHandle: 'handle-1',
paneKey: 'tab-1:pane-1',
tabId: 'tab-1',
leafId: 'pane-1',
processIncarnation: 'pty-1:inc-1',
ptyId: 'pty-1',
incarnationId: 'inc-1'
}
]
}
const EMPTY_RESULT: LegacyWorkerTerminalRecoveryResult = {
adoptedDispatchIds: [],
exitedDispatchIds: [],
deferredDispatchIds: [DEFERRED_DISPATCH_ID]
}
function armedController(): {
controller: RuntimeLegacyWorkerTerminalRecoveryController
reconcile: ReturnType<typeof vi.fn>
} {
const reconcile = vi.fn(async () => EMPTY_RESULT)
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the retry timer only ever reaches `ports.reconcile`; the rest of the port surface is unreachable from `updateRetry`.
const ports = { reconcile } as unknown as LegacyWorkerRecoveryPorts
const controller = new RuntimeLegacyWorkerTerminalRecoveryController(ports)
controller.updateRetry(DEFERRED_PLAN, new Set([DEFERRED_DISPATCH_ID]), {})
return { controller, reconcile }
}
describe('RuntimeLegacyWorkerTerminalRecoveryController retry loop', () => {
beforeEach(() => {
vi.useFakeTimers()
})
afterEach(() => {
__cancelLegacyWorkerTerminalRecoveryRetriesForTests()
vi.useRealTimers()
})
it('keeps retrying recovery while a worker stays deferred', async () => {
const { reconcile } = armedController()
await vi.advanceTimersByTimeAsync(1_000)
expect(reconcile).toHaveBeenCalledTimes(1)
})
it('stops a controller retry loop once its scopes are cancelled', async () => {
const { controller, reconcile } = armedController()
controller.cancelAllRetries()
await vi.advanceTimersByTimeAsync(60_000)
expect(reconcile).not.toHaveBeenCalled()
})
it('reaches every armed controller from the test cancel hook', async () => {
// Why the hook exists: the retry re-arms itself for as long as a worker stays deferred, so a
// suite that never resolves one keeps a recovery loop — and the worktree scans it issues —
// running inside whichever later test happens to be executing when the timer fires.
const first = armedController()
const second = armedController()
__cancelLegacyWorkerTerminalRecoveryRetriesForTests()
await vi.advanceTimersByTimeAsync(60_000)
expect(first.reconcile).not.toHaveBeenCalled()
expect(second.reconcile).not.toHaveBeenCalled()
})
})