test(windows): give the Windows lane's teardowns the repo's rm retry policy

`config/scripts/rebuild-native-deps.test.mjs` fails on `package (windows)` with
EPERM from the `finally` of "rebuilds a loadable ConPTY native that lacks Orca
job ownership". Every assertion in that test had already passed; the throw is
the cleanup. The test's whole job is to build and load a ConPTY native, and on
Windows a freshly dlopen'd DLL keeps a handle on the directory it came from for
a moment after the child exits — so `rmSync(projectDir, { recursive: true,
force: true })`, which is `maxRetries: 0`, loses the race.

The repo already knows this: `WINDOWS_RM_MAX_RETRIES = 8` in
`src/main/host-tree-removal.ts` and again in `codex-accounts/service.ts`. Only
product code was using it.

Adds `src/shared/windows-transient-lock-removal.ts` as one home for the policy
and moves all 21 raw recursive removals in the Windows CI lane onto it, plus a
boundary test that reads the lane's spec list out of `.github/workflows/pr.yml`
so a new spec cannot hand-roll the removal and reintroduce this.

Census, since the pattern is not confined to that lane: 1,633 recursive
removals across 1,028 files pass no `maxRetries` — 1,368 in tests and fixtures,
265 in product code. Only the Windows lane's 21 are changed here.
This commit is contained in:
Brennan Benson
2026-08-29 20:12:26 -07:00
committed by Merge Sim
parent 8ff8e8a5f3
commit b8c7117c1e
12 changed files with 205 additions and 29 deletions
+12 -12
View File
@@ -6,13 +6,13 @@ import {
mkdirSync,
mkdtempSync,
readFileSync,
rmSync,
writeFileSync
} from 'node:fs'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { fileURLToPath } from 'node:url'
import { describe, expect, it } from 'vitest'
import { removeTreeSync } from '../../src/shared/windows-transient-lock-removal.ts'
const sourceScriptPath = fileURLToPath(new URL('./rebuild-native-deps.mjs', import.meta.url))
const sourceInstallScriptPath = fileURLToPath(
@@ -46,7 +46,7 @@ describe('rebuild-native-deps Electron install fallback', () => {
'download attempted\n'
)
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
})
@@ -70,7 +70,7 @@ describe('rebuild-native-deps Electron install fallback', () => {
'Continuing postinstall because Electron binary installation failed'
)
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
})
@@ -91,7 +91,7 @@ describe('rebuild-native-deps Electron install fallback', () => {
'Continuing postinstall because Electron binary installation failed'
)
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
})
@@ -121,7 +121,7 @@ describe('rebuild-native-deps Electron install fallback', () => {
'partial cleared\ndownload attempted\n'
)
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
})
})
@@ -154,7 +154,7 @@ describe('rebuild-native-deps patched node-pty rebuild', () => {
)
expect(existsSync(rebuildLogPath)).toBe(false)
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
}
)
@@ -179,7 +179,7 @@ describe('rebuild-native-deps patched node-pty rebuild', () => {
expect(readFileSync(join(runtimeDir, 'conpty.dll'), 'utf8')).toBe('conpty.dll x64')
expect(readFileSync(join(runtimeDir, 'OpenConsole.exe'), 'utf8')).toBe('OpenConsole.exe x64')
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
})
@@ -207,7 +207,7 @@ describe('rebuild-native-deps patched node-pty rebuild', () => {
const rebuildCall = JSON.parse(readFileSync(rebuildLogPath, 'utf8').trim())
expect(rebuildCall.onlyModules).toEqual(['windows-native-registry'])
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
}
)
@@ -237,7 +237,7 @@ describe('rebuild-native-deps patched node-pty rebuild', () => {
const rebuildCall = JSON.parse(readFileSync(rebuildLogPath, 'utf8').trim())
expect(rebuildCall.onlyModules).toEqual(['node-pty'])
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
}
)
@@ -268,7 +268,7 @@ describe('rebuild-native-deps patched node-pty rebuild', () => {
expect(rebuildCall.ignoreModules).toEqual(['cpu-features'])
expect(rebuildCall.force).toBe(true)
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
}
)
@@ -296,7 +296,7 @@ describe('rebuild-native-deps patched node-pty rebuild', () => {
)
expect(existsSync(rebuildLogPath)).toBe(false)
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
}
)
@@ -326,7 +326,7 @@ describe('rebuild-native-deps patched node-pty rebuild', () => {
expect(rebuildCall.onlyModules).toEqual(['node-pty'])
expect(rebuildCall.force).toBe(true)
} finally {
rmSync(projectDir, { recursive: true, force: true })
removeTreeSync(projectDir)
}
}
)
@@ -7,7 +7,7 @@
import { afterEach, describe, expect, it, vi } from 'vitest'
import { spawn } from 'node:child_process'
import { createServer, type Server } from 'node:http'
import { mkdtempSync, readFileSync, rmSync } from 'node:fs'
import { mkdtempSync, readFileSync } from 'node:fs'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import type * as osModule from 'node:os'
@@ -33,6 +33,7 @@ vi.mock('os', async (importOriginal) => {
import { ClaudeHookService } from '../claude/hook-service'
import { getConfigPath, getWindowsManagedLifecycleHook } from '../claude/hook-settings'
import { findGitBash } from './windows-git-bash-path.test-fixture'
import { removeTreeSync } from '../../shared/windows-transient-lock-removal'
const PANE_KEY = 'tab-1:leaf-1'
const HOOK_TOKEN = 'payload-delivery-token'
@@ -149,7 +150,7 @@ describe.skipIf(process.platform !== 'win32')('Windows managed hook payload deli
server = null
homedirMock.mockImplementation(() => process.env.HOME ?? tmpdir())
if (home) {
rmSync(home, { recursive: true, force: true })
removeTreeSync(home)
home = ''
}
})
@@ -1,9 +1,10 @@
import { spawnSync } from 'node:child_process'
import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'
import { mkdir, mkdtemp, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { describe, expect, it } from 'vitest'
import { buildWslBridgeScript, buildWslLauncher } from './wsl-cli-scripts'
import { removeTree } from '../../shared/windows-transient-lock-removal'
const FORWARDED_ARGS = [
'terminal',
@@ -113,7 +114,7 @@ describe('WSL CLI PowerShell boundary', () => {
expect(exitResult.error).toBeUndefined()
expect(exitResult.status).toBe(23)
} finally {
await rm(root, { recursive: true, force: true })
await removeTree(root)
}
}
)
+4 -3
View File
@@ -1,5 +1,5 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { mkdirSync, mkdtempSync, readFileSync, rmSync, unlinkSync, writeFileSync } from 'node:fs'
import { mkdirSync, mkdtempSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs'
import { tmpdir } from 'node:os'
import { dirname, join } from 'node:path'
import { spawnSync } from 'node:child_process'
@@ -20,6 +20,7 @@ vi.mock('os', async () => {
import { CursorHookService } from './hook-service'
import { POSIX_HOOK_STDIN_READER } from '../agent-hooks/hook-stdin-contract'
import { CURSOR_EVENTS, type CursorEvent } from './hook-events'
import { removeTreeSync } from '../../shared/windows-transient-lock-removal'
const CURSOR_SCRIPT_FILE_NAME = process.platform === 'win32' ? 'cursor-hook.cmd' : 'cursor-hook.sh'
const WINDOWS_POWERSHELL_LAUNCHER =
@@ -89,7 +90,7 @@ describe('CursorHookService', () => {
afterEach(() => {
vi.clearAllMocks()
rmSync(homeDir, { recursive: true, force: true })
removeTreeSync(homeDir)
})
it('installs Cursor Agent hooks with the documented top-level command schema', () => {
@@ -155,7 +156,7 @@ describe('CursorHookService', () => {
expect(command).toMatch(WINDOWS_POWERSHELL_LAUNCHER)
}
} finally {
rmSync(spaceHome, { recursive: true, force: true })
removeTreeSync(spaceHome)
}
}
)
@@ -1,6 +1,6 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { installFakeAppEnvironment } from '../../../config/scripts/vitest-host-ports-setup'
import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync, mkdirSync } from 'node:fs'
import { existsSync, mkdtempSync, readFileSync, writeFileSync, mkdirSync } from 'node:fs'
import { join } from 'node:path'
import { tmpdir } from 'node:os'
import {
@@ -10,6 +10,7 @@ import {
ORCA_PROFILE_INDEX_SCHEMA_VERSION,
type OrcaProfileIndex
} from '../../shared/orca-profiles'
import { removeTreeSync } from '../../shared/windows-transient-lock-removal'
const testState = { dir: '' }
@@ -35,7 +36,7 @@ describe('profile index store', () => {
})
afterEach(() => {
rmSync(testState.dir, { recursive: true, force: true })
removeTreeSync(testState.dir)
})
it('creates the default local profile and copies legacy state without deleting it', async () => {
@@ -4,12 +4,13 @@
// Windows path resolution, CRLF in `HEAD`/`gitdir`/`commondir`, and whether `worktree move`/`lock`
// and deleting a live checkout behave as they do on POSIX. The Linux shards cannot reach any of it.
import { execFile } from 'node:child_process'
import { mkdir, mkdtemp, realpath, rm, writeFile } from 'node:fs/promises'
import { mkdir, mkdtemp, realpath, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { promisify } from 'node:util'
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
import { readRepoWorktreeAdminFingerprint } from './repo-worktree-admin-fingerprint'
import { removeTree } from '../../shared/windows-transient-lock-removal'
const execFileAsync = promisify(execFile)
@@ -43,7 +44,7 @@ beforeEach(async () => {
})
afterEach(async () => {
await rm(scratchDir, { recursive: true, force: true })
await removeTree(scratchDir)
})
describe('readRepoWorktreeAdminFingerprint', () => {
@@ -71,7 +72,7 @@ describe('readRepoWorktreeAdminFingerprint', () => {
it('changes when a worktree directory is deleted outside Git', async () => {
// The admin dir is untouched by `rm -rf`, but the row's `prunable` flag flips.
const before = await fingerprint()
await rm(worktreePath, { recursive: true, force: true })
await removeTree(worktreePath)
expect(await fingerprint()).not.toBe(before)
})
@@ -1,8 +1,9 @@
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
import { mkdtempSync, writeFileSync } from 'node:fs'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { spawn } from 'node:child_process'
import { afterAll, beforeAll, describe, expect, it } from 'vitest'
import { removeTreeSync } from '../../shared/windows-transient-lock-removal'
/**
* The second half of the two-job design.
@@ -50,7 +51,7 @@ describeOnWindows('host job reaps the tree when the host dies', () => {
})
afterAll(() => {
rmSync(dir, { recursive: true, force: true })
removeTreeSync(dir)
})
it('kills a pty and its detached grandchild when the host is force-killed', async () => {
@@ -7,6 +7,7 @@ import {
WINDOWS_ARGUMENT_CORPUS,
WINDOWS_ARGUMENT_CORPUS_ENV
} from './__fixtures__/windows-argument-corpus'
import { removeTreeSync } from '../windows-transient-lock-removal'
/**
* The other half of the encoding proof: the unit test checks the bytes against
@@ -37,7 +38,7 @@ describeOnWindows('Windows .cmd argument round-trip', () => {
})
afterAll(() => {
rmSync(dir, { recursive: true, force: true })
removeTreeSync(dir)
})
function decode(stdout: string): string[] {
+3 -2
View File
@@ -1,4 +1,4 @@
import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'
import { chmodSync, mkdtempSync, writeFileSync } from 'node:fs'
import type * as NodeFs from 'node:fs'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
@@ -28,13 +28,14 @@ import {
fsyncFileSync,
writeDurableSecureJsonFile
} from './secure-file'
import { removeTreeSync } from './windows-transient-lock-removal'
const createdPaths: string[] = []
afterEach(() => {
openedPaths.length = 0
for (const path of createdPaths.splice(0)) {
rmSync(path, { recursive: true, force: true })
removeTreeSync(path)
}
})
@@ -0,0 +1,97 @@
import { readFileSync } from 'node:fs'
import { join } from 'node:path'
import { describe, expect, it } from 'vitest'
/**
* The Windows CI lane runs a fixed list of specs on `windows-2022`, and every one of them removes
* a temporary tree when it is done. On Windows those removals race a handle the OS has not
* released yet — a just-exited child, an indexer, a dlopen'd native module — so a raw
* `rmSync(dir, { recursive: true, force: true })` throws EPERM after the test's assertions have
* all passed, and the lane reports a green test as a failure.
*
* `removeTree`/`removeTreeSync` carry the repo's `maxRetries: 8` policy. This keeps the lane on
* them: a new spec that hand-rolls the removal fails here rather than intermittently on Windows.
*/
const REPO_ROOT = join(__dirname, '..', '..')
const WORKFLOW_PATH = join(REPO_ROOT, '.github', 'workflows', 'pr.yml')
const WINDOWS_STEP_NAME = 'Test Windows-specific boundaries'
/** The spec paths the `package (windows)` job passes to vitest, read from the workflow itself. */
function readWindowsLaneSpecs(): string[] {
const workflow = readFileSync(WORKFLOW_PATH, 'utf8')
const stepIndex = workflow.indexOf(`- name: ${WINDOWS_STEP_NAME}`)
expect(stepIndex, `${WORKFLOW_PATH} no longer has a "${WINDOWS_STEP_NAME}" step`).toBeGreaterThan(
-1
)
const nextStepIndex = workflow.indexOf('\n - name:', stepIndex + 1)
const step = workflow.slice(stepIndex, nextStepIndex === -1 ? undefined : nextStepIndex)
return step
.split('\n')
.map((line) => line.trim())
.filter((line) => /^(src|tests|config)\/.+\.(test|spec)\.(ts|tsx|mjs)$/.test(line))
}
/** Every recursive removal that does not go through the retrying helper. */
function findRawRecursiveRemovals(source: string): number[] {
const offenders: number[] = []
const call = /(?<![\w$.])(?:fs\.)?rm(?:Sync)?\(/g
let match: RegExpExecArray | null
while ((match = call.exec(source)) !== null) {
// Read to the call's closing paren so multi-line option objects are covered.
let depth = 0
let end = match.index + match[0].length - 1
for (; end < source.length; end += 1) {
if (source[end] === '(') {
depth += 1
} else if (source[end] === ')') {
depth -= 1
if (depth === 0) {
break
}
}
}
const args = source.slice(match.index, end + 1)
if (!args.includes('recursive')) {
continue
}
if (args.includes('maxRetries')) {
continue
}
offenders.push(source.slice(0, match.index).split('\n').length)
}
return offenders
}
describe('windows lane tree removal', () => {
const specs = readWindowsLaneSpecs()
it('reads a non-trivial spec list out of the workflow', () => {
// A parser that silently matched nothing would make every assertion below vacuous.
expect(specs.length).toBeGreaterThan(10)
expect(specs).toContain('config/scripts/rebuild-native-deps.test.mjs')
})
it('actually detects a raw recursive removal', () => {
// Without this the scan below passes for any reason at all, including not scanning.
expect(findRawRecursiveRemovals('rmSync(dir, { recursive: true, force: true })')).toEqual([1])
expect(
findRawRecursiveRemovals(
'await rm(dir, {\n recursive: true,\n force: true,\n maxRetries: 8\n})'
)
).toEqual([])
// A single-file removal is not this rule's business.
expect(findRawRecursiveRemovals('rmSync(file, { force: true })')).toEqual([])
})
it('removes trees through the retrying helper, never a raw recursive rm', () => {
const offenders = specs.flatMap((spec) => {
const source = readFileSync(join(REPO_ROOT, spec), 'utf8')
return findRawRecursiveRemovals(source).map((line) => `${spec}:${line}`)
})
expect(
offenders,
'these teardowns can throw EPERM on Windows after their assertions have passed; use removeTree/removeTreeSync from src/shared/windows-transient-lock-removal.ts'
).toEqual([])
})
})
@@ -0,0 +1,40 @@
import { afterEach, describe, expect, it, vi } from 'vitest'
import {
WINDOWS_RM_MAX_RETRIES,
WINDOWS_RM_RETRY_DELAY_MS,
transientLockRemovalOptions
} from './windows-transient-lock-removal'
function withPlatform(platform: NodeJS.Platform): void {
vi.stubGlobal('process', { ...process, platform })
}
describe('transient lock removal options', () => {
afterEach(() => {
vi.unstubAllGlobals()
})
it('retries on Windows, where a late handle release is the whole problem', () => {
withPlatform('win32')
expect(transientLockRemovalOptions()).toEqual({
recursive: true,
force: true,
maxRetries: WINDOWS_RM_MAX_RETRIES,
retryDelay: WINDOWS_RM_RETRY_DELAY_MS
})
})
it('matches the repo policy of eight attempts', () => {
// Same number src/main/host-tree-removal.ts and codex-accounts/service.ts already use.
expect(WINDOWS_RM_MAX_RETRIES).toBe(8)
})
it('asks for no retries where removal is not raced by the OS', () => {
for (const platform of ['darwin', 'linux'] as const) {
withPlatform(platform)
expect(transientLockRemovalOptions()).toEqual({ recursive: true, force: true })
vi.unstubAllGlobals()
}
})
})
@@ -0,0 +1,31 @@
// Why: Windows releases handles late. Antivirus, the search indexer, a just-exited child and a
// freshly dlopen'd DLL all keep a tree Node has just emptied locked for a few milliseconds, which
// surfaces as EBUSY/ENOTEMPTY/EPERM. Node's own `maxRetries` absorbs exactly that, and the repo
// already settled on 8 attempts — but only product code was using it, so test teardown kept
// failing tests whose assertions had already passed.
import type { RmOptions } from 'node:fs'
import { rmSync } from 'node:fs'
import { rm } from 'node:fs/promises'
export const WINDOWS_RM_MAX_RETRIES = 8
export const WINDOWS_RM_RETRY_DELAY_MS = 150
/** `rm`/`rmSync` options for a recursive removal that must survive a late handle release. */
export function transientLockRemovalOptions(): RmOptions {
const base = { recursive: true, force: true }
if (process.platform !== 'win32') {
return base
}
return { ...base, maxRetries: WINDOWS_RM_MAX_RETRIES, retryDelay: WINDOWS_RM_RETRY_DELAY_MS }
}
/** Recursively remove a directory, retrying the transient Windows locks. */
export function removeTreeSync(targetPath: string): void {
rmSync(targetPath, transientLockRemovalOptions())
}
/** Recursively remove a directory, retrying the transient Windows locks. */
export async function removeTree(targetPath: string): Promise<void> {
await rm(targetPath, transientLockRemovalOptions())
}