From 2a4fd2f263decfa582cd53869fe608d64e4d1097 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 29 Aug 2026 09:02:42 -0700 Subject: [PATCH] refactor(windows): state the rm retry policy once, and widen the guard past `fs.` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on this PR. The policy was about to be stated three times. `host-tree-removal.ts` and `codex-accounts/service.ts` already each declared `WINDOWS_RM_MAX_RETRIES = 8`, and this PR's `transientLockRemovalOptions` made a third — in a change whose whole point is "one retry policy". Both existing sites now import it, and a ratchet in the module's own spec fails on a fourth declaration. `removeManagedHomeTreeSync` keeps its unconditional shape deliberately: it removes WSL-hosted managed homes from this process, so its retries are not gated on the host being Windows the way `transientLockRemovalOptions()` is. The lane guard's matcher only understood a bare `rm(` or a literal `fs.` prefix, so `fsp.rm`, `fsPromises.rm` and `nodeFs.rmSync` — all spellings this repo already uses — walked past it, and the guard would have gone on reporting zero offenders. It now accepts any namespace prefix and reads renamed bindings out of the import clause, with a planted spelling of each form as the proof. --- src/main/codex-accounts/service.ts | 10 ++- src/main/host-tree-removal.ts | 22 +----- ...windows-lane-tree-removal-boundary.test.ts | 75 ++++++++++++++++++- .../windows-transient-lock-removal.test.ts | 51 +++++++++++++ 4 files changed, 135 insertions(+), 23 deletions(-) diff --git a/src/main/codex-accounts/service.ts b/src/main/codex-accounts/service.ts index 97a3985c701..ebdc5e2bb9b 100644 --- a/src/main/codex-accounts/service.ts +++ b/src/main/codex-accounts/service.ts @@ -63,13 +63,13 @@ import { ManagedCodexHomeTemporarilyUnavailableError } from './host-codex-managed-home-ownership' import { isDefinitiveAbsence } from '../../shared/definitive-filesystem-absence' +import { + WINDOWS_RM_MAX_RETRIES, + WINDOWS_RM_RETRY_DELAY_MS +} from '../../shared/windows-transient-lock-removal' const LOGIN_TIMEOUT_MS = 120_000 const MAX_LOGIN_OUTPUT_CHARS = 4_000 -// Why: mirrors the Windows rm retry policy in local-worktree-filesystem — a -// just-terminated codex login can briefly keep handles inside a managed home. -const WINDOWS_RM_MAX_RETRIES = 8 -const WINDOWS_RM_RETRY_DELAY_MS = 150 const WINDOWS_LOGIN_AUTH_POLL_INTERVAL_MS = 500 const WINDOWS_LOGIN_POST_AUTH_EXIT_GRACE_MS = 5_000 const WINDOWS_LOGIN_TREE_KILL_TIMEOUT_MS = 5_000 @@ -205,6 +205,8 @@ function removeManagedHomeTreeSync(targetPath: string): void { // Why: codex login descendants can briefly keep Windows handles on files in // the managed home (e.g. log/codex-login.log); bounded retries absorb the // transient lock instead of failing with ENOTEMPTY and orphaning the home. + // Why unconditional, unlike transientLockRemovalOptions(): a WSL-hosted managed home is removed + // by this host process, so the lock this absorbs is not gated on the host being Windows. rmSync(targetPath, { recursive: true, force: true, diff --git a/src/main/host-tree-removal.ts b/src/main/host-tree-removal.ts index dcc8a2f2a43..8619b9b69bc 100644 --- a/src/main/host-tree-removal.ts +++ b/src/main/host-tree-removal.ts @@ -2,14 +2,12 @@ // generations) hits the same Windows stickiness — AV/indexers/late handle releases surface transient // EBUSY/ENOTEMPTY/EPERM on a tree Node just emptied. One helper so no call site forgets the retries. -import type { RmOptions } from 'node:fs' import { rm } from 'node:fs/promises' import { win32 } from 'node:path' import { setTimeout as delay } from 'node:timers/promises' +import { transientLockRemovalOptions } from '../shared/windows-transient-lock-removal' const WINDOWS_REMOVE_RETRY_DELAYS_MS = [250, 500, 1_000, 2_000] -const WINDOWS_RM_MAX_RETRIES = 8 -const WINDOWS_RM_RETRY_DELAY_MS = 150 export function toHostRemovalPath(targetPath: string): string { // Why: Git for Windows can fail long recursive deletes even after Orca has @@ -17,20 +15,6 @@ export function toHostRemovalPath(targetPath: string): string { return process.platform === 'win32' ? win32.toNamespacedPath(targetPath) : targetPath } -function getHostRemovalOptions(): RmOptions { - const base = { recursive: true, force: true } - if (process.platform !== 'win32') { - return base - } - return { - ...base, - // Why: large Windows trees commonly surface transient ENOTEMPTY/EPERM while - // Node walks and removes nested directories. - maxRetries: WINDOWS_RM_MAX_RETRIES, - retryDelay: WINDOWS_RM_RETRY_DELAY_MS - } -} - function isTransientWindowsRemovalError(error: unknown): boolean { if (process.platform !== 'win32' || typeof error !== 'object' || error === null) { return false @@ -47,7 +31,9 @@ function isTransientWindowsRemovalError(error: unknown): boolean { export async function removeHostTree(targetPath: string): Promise { const removalPath = toHostRemovalPath(targetPath) const retryDelays = process.platform === 'win32' ? WINDOWS_REMOVE_RETRY_DELAYS_MS : [] - const rmOptions = getHostRemovalOptions() + // Why: large Windows trees commonly surface transient ENOTEMPTY/EPERM while Node walks and + // removes nested directories; Node's own retries absorb that before the loop below has to. + const rmOptions = transientLockRemovalOptions() let attempt = 0 while (true) { diff --git a/src/shared/windows-lane-tree-removal-boundary.test.ts b/src/shared/windows-lane-tree-removal-boundary.test.ts index a4b94204c56..2719cef180f 100644 --- a/src/shared/windows-lane-tree-removal-boundary.test.ts +++ b/src/shared/windows-lane-tree-removal-boundary.test.ts @@ -31,10 +31,41 @@ function readWindowsLaneSpecs(): string[] { .filter((line) => /^(src|tests|config)\/.+\.(test|spec)\.(ts|tsx|mjs)$/.test(line)) } +/** `node:fs` and `node:fs/promises`, spelled with or without the `node:` prefix. */ +const FS_SPECIFIER = String.raw`['"](?:node:)?fs(?:/promises)?['"]` +/** The `{ … }` clause of an fs import or require, which is where a rename would be declared. */ +const FS_BINDING_CLAUSE = new RegExp( + String.raw`\{([^}]*)\}\s*(?:from\s*${FS_SPECIFIER}|=\s*(?:await\s+import|require)\(\s*${FS_SPECIFIER})`, + 'g' +) +/** `rm as removeDir` or `rmSync: dropTree` — the two ways a binding gets a local name. */ +const RENAMED_REMOVAL = /\brm(?:Sync)?\s*(?:as|:)\s*([A-Za-z0-9_$]+)/g + +/** + * The local names a recursive removal can be called by in `source`. + * + * Namespaced spellings are covered by the optional `.` prefix in the matcher rather + * than by listing names, so `fsp.rm` and `fsPromises.rm` are caught without anyone having to teach + * the rule that spelling first. Renames are the one form that prefix cannot see, so they are read + * out of the import clause. + */ +function collectRemovalNames(source: string): string[] { + const names = new Set(['rmSync', 'rm']) + for (const clause of source.matchAll(FS_BINDING_CLAUSE)) { + for (const rename of clause[1].matchAll(RENAMED_REMOVAL)) { + names.add(rename[1]) + } + } + return [...names] +} + /** Every recursive removal that does not go through the retrying helper. */ function findRawRecursiveRemovals(source: string): number[] { const offenders: number[] = [] - const call = /(? { expect(findRawRecursiveRemovals('rmSync(file, { force: true })')).toEqual([]) }) + it('detects the removal whatever the import spelled it', () => { + // Why: a rule that only reads one import style stops catching violations the moment someone + // writes the next one differently — and the guard goes on reporting zero offenders. + const spellings: [string, string][] = [ + ['bare named import', "import { rmSync } from 'node:fs'\nrmSync(DIR"], + ['fs namespace', "import * as fs from 'node:fs'\nfs.rmSync(DIR"], + ['fsp namespace', "import * as fsp from 'node:fs/promises'\nawait fsp.rm(DIR"], + [ + 'fsPromises namespace', + "import * as fsPromises from 'node:fs/promises'\nawait fsPromises.rm(DIR" + ], + ['nodeFs namespace', "import * as nodeFs from 'node:fs'\nnodeFs.rmSync(DIR"], + ['unprefixed fs specifier', "import * as fs from 'fs'\nfs.rmSync(DIR"], + [ + 'renamed named import', + "import { rm as removeDir } from 'node:fs/promises'\nawait removeDir(DIR" + ], + ['renamed require', "const { rmSync: dropTree } = require('node:fs')\ndropTree(DIR"] + ] + + for (const [label, prelude] of spellings) { + const source = `${prelude}, { recursive: true, force: true })` + expect(findRawRecursiveRemovals(source), `${label} slipped past the scan`).toEqual([2]) + } + }) + + it('still exempts the retrying spellings and single-file removals', () => { + // The widened matcher must not start reporting the calls the rule is asking people to write. + expect( + findRawRecursiveRemovals( + "import * as fsp from 'node:fs/promises'\nawait fsp.rm(dir, { recursive: true, maxRetries: 8 })" + ) + ).toEqual([]) + expect( + findRawRecursiveRemovals( + "import { rm as removeDir } from 'node:fs/promises'\nawait removeDir(file, { force: true })" + ) + ).toEqual([]) + // `rm` inside a longer identifier is not a removal call. + expect(findRawRecursiveRemovals('confirmRemoval(dir, { recursive: 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') diff --git a/src/shared/windows-transient-lock-removal.test.ts b/src/shared/windows-transient-lock-removal.test.ts index 223c760592e..0d0145ac973 100644 --- a/src/shared/windows-transient-lock-removal.test.ts +++ b/src/shared/windows-transient-lock-removal.test.ts @@ -1,3 +1,5 @@ +import { readFileSync, readdirSync, statSync } from 'node:fs' +import { join, relative } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' import { WINDOWS_RM_MAX_RETRIES, @@ -9,6 +11,37 @@ function withPlatform(platform: NodeJS.Platform): void { vi.stubGlobal('process', { ...process, platform }) } +const SOURCE_ROOT = join(__dirname, '..') +const OWNING_MODULE = join('shared', 'windows-transient-lock-removal.ts') +const IGNORED_DIRECTORIES = new Set(['node_modules', 'dist', 'out', 'build', '.git']) +/** A `const WINDOWS_RM_… =` line, i.e. a file stating the policy rather than importing it. */ +const POLICY_DECLARATION = + /^\s*(?:export\s+)?const\s+WINDOWS_RM_(?:MAX_RETRIES|RETRY_DELAY_MS)\s*=/m + +function collectSourceFiles(root: string): string[] { + const found: string[] = [] + for (const entry of readdirSync(root)) { + if (IGNORED_DIRECTORIES.has(entry)) { + continue + } + const path = join(root, entry) + if (statSync(path).isDirectory()) { + found.push(...collectSourceFiles(path)) + } else if (/\.tsx?$/.test(entry)) { + found.push(path) + } + } + return found +} + +/** Every file that declares the retry policy instead of importing it. */ +function findPolicyDeclarations(): string[] { + return collectSourceFiles(SOURCE_ROOT) + .filter((path) => POLICY_DECLARATION.test(readFileSync(path, 'utf8'))) + .map((path) => relative(SOURCE_ROOT, path)) + .sort() +} + describe('transient lock removal options', () => { afterEach(() => { vi.unstubAllGlobals() @@ -37,4 +70,22 @@ describe('transient lock removal options', () => { vi.unstubAllGlobals() } }) + + it('actually detects a file that states the policy', () => { + // Without this the scan below passes for any reason at all, including not scanning. + expect(POLICY_DECLARATION.test('const WINDOWS_RM_MAX_RETRIES = 8')).toBe(true) + expect(POLICY_DECLARATION.test(' export const WINDOWS_RM_RETRY_DELAY_MS = 150')).toBe(true) + // Importing the policy is the thing this rule is asking for, not a violation of it. + expect(POLICY_DECLARATION.test('import { WINDOWS_RM_MAX_RETRIES } from x')).toBe(false) + expect(POLICY_DECLARATION.test(' retryDelay: WINDOWS_RM_RETRY_DELAY_MS')).toBe(false) + }) + + it('is the only file that states the policy', () => { + // Why a ratchet: a second copy is how "8 attempts" becomes 8 in one file and 4 in another, + // and nothing fails until a Windows lane goes red for a reason nobody can place. + expect( + findPolicyDeclarations(), + 'declare the retry policy once, in src/shared/windows-transient-lock-removal.ts, and import it' + ).toEqual([OWNING_MODULE]) + }) })