refactor(windows): state the rm retry policy once, and widen the guard past fs.

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.
This commit is contained in:
Brennan Benson
2026-08-29 20:12:26 -07:00
committed by Merge Sim
parent b8c7117c1e
commit 2a4fd2f263
4 changed files with 135 additions and 23 deletions
+6 -4
View File
@@ -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,
+4 -18
View File
@@ -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<void> {
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) {
@@ -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 `<identifier>.` 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 = /(?<![\w$.])(?:fs\.)?rm(?:Sync)?\(/g
const call = new RegExp(
String.raw`(?<![\w$.])(?:[\w$]+\.)?(?:${collectRemovalNames(source).join('|')})\s*\(`,
'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.
@@ -83,6 +114,48 @@ describe('windows lane tree removal', () => {
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')
@@ -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])
})
})