diff --git a/config/scripts/rebuild-native-deps.test.mjs b/config/scripts/rebuild-native-deps.test.mjs index 5ebc39dfcaa..de5c5e9c8b1 100644 --- a/config/scripts/rebuild-native-deps.test.mjs +++ b/config/scripts/rebuild-native-deps.test.mjs @@ -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) } } ) diff --git a/src/main/agent-hooks/windows-hook-payload-delivery.test.ts b/src/main/agent-hooks/windows-hook-payload-delivery.test.ts index aabf2545172..487353f5ac2 100644 --- a/src/main/agent-hooks/windows-hook-payload-delivery.test.ts +++ b/src/main/agent-hooks/windows-hook-payload-delivery.test.ts @@ -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 = '' } }) diff --git a/src/main/cli/wsl-cli-powershell-boundary.test.ts b/src/main/cli/wsl-cli-powershell-boundary.test.ts index 7e706a0ed12..3b9a02ef4e8 100644 --- a/src/main/cli/wsl-cli-powershell-boundary.test.ts +++ b/src/main/cli/wsl-cli-powershell-boundary.test.ts @@ -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) } } ) diff --git a/src/main/cursor/hook-service.test.ts b/src/main/cursor/hook-service.test.ts index c6aef9aefc5..113705fe602 100644 --- a/src/main/cursor/hook-service.test.ts +++ b/src/main/cursor/hook-service.test.ts @@ -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) } } ) diff --git a/src/main/orca-profiles/profile-index-store.test.ts b/src/main/orca-profiles/profile-index-store.test.ts index 06f2efe0903..ce4d03af98c 100644 --- a/src/main/orca-profiles/profile-index-store.test.ts +++ b/src/main/orca-profiles/profile-index-store.test.ts @@ -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 () => { diff --git a/src/main/runtime/repo-worktree-admin-fingerprint.test.ts b/src/main/runtime/repo-worktree-admin-fingerprint.test.ts index a8f2fc9d8e4..f1c1e184090 100644 --- a/src/main/runtime/repo-worktree-admin-fingerprint.test.ts +++ b/src/main/runtime/repo-worktree-admin-fingerprint.test.ts @@ -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) }) diff --git a/src/main/windows/windows-host-job.win32.test.ts b/src/main/windows/windows-host-job.win32.test.ts index 94e9c323860..941d81fd4d3 100644 --- a/src/main/windows/windows-host-job.win32.test.ts +++ b/src/main/windows/windows-host-job.win32.test.ts @@ -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 () => { diff --git a/src/shared/child-process/windows-command-line.win32.test.ts b/src/shared/child-process/windows-command-line.win32.test.ts index 64ed8322ec9..14688f8cc88 100644 --- a/src/shared/child-process/windows-command-line.win32.test.ts +++ b/src/shared/child-process/windows-command-line.win32.test.ts @@ -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[] { diff --git a/src/shared/secure-file-fsync-flags.test.ts b/src/shared/secure-file-fsync-flags.test.ts index 15505151aef..55cf10a3fed 100644 --- a/src/shared/secure-file-fsync-flags.test.ts +++ b/src/shared/secure-file-fsync-flags.test.ts @@ -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) } }) diff --git a/src/shared/windows-lane-tree-removal-boundary.test.ts b/src/shared/windows-lane-tree-removal-boundary.test.ts new file mode 100644 index 00000000000..a4b94204c56 --- /dev/null +++ b/src/shared/windows-lane-tree-removal-boundary.test.ts @@ -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 = /(? { + 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([]) + }) +}) diff --git a/src/shared/windows-transient-lock-removal.test.ts b/src/shared/windows-transient-lock-removal.test.ts new file mode 100644 index 00000000000..223c760592e --- /dev/null +++ b/src/shared/windows-transient-lock-removal.test.ts @@ -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() + } + }) +}) diff --git a/src/shared/windows-transient-lock-removal.ts b/src/shared/windows-transient-lock-removal.ts new file mode 100644 index 00000000000..f2d28fb2c04 --- /dev/null +++ b/src/shared/windows-transient-lock-removal.ts @@ -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 { + await rm(targetPath, transientLockRemovalOptions()) +}