From d2b8d45d2031738f902586b748972c997ebb53a6 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Fri, 25 Sep 2026 03:11:42 +0000 Subject: [PATCH 1/2] test: migrate recursive rmSync to removeTree(Sync) and gate rmSync, rmdir, and maxRetries: 0 --- .../tests/check-test-remove-tree.test.ts | 37 +++++++++++++++++++ .../tests/playground-service.test.ts | 6 +-- .../tests/support/remove-tree.test.ts | 13 ++++++- .../agent-bundle/tests/support/remove-tree.ts | 6 +++ .../agent-bundle/tests/support/shared-pack.ts | 4 +- .../rsc-runtime/tests/plugin-root.test.ts | 7 ++-- scripts/check-test-remove-tree.mjs | 24 +++++++++--- 7 files changed, 81 insertions(+), 16 deletions(-) diff --git a/packages/agent-bundle/tests/check-test-remove-tree.test.ts b/packages/agent-bundle/tests/check-test-remove-tree.test.ts index 21665c6c9..183fc11c4 100644 --- a/packages/agent-bundle/tests/check-test-remove-tree.test.ts +++ b/packages/agent-bundle/tests/check-test-remove-tree.test.ts @@ -484,3 +484,40 @@ it('flags fs.promises.rm and wrapped callees or callee objects', () => { `await fs.other.rm(root, { ${recursiveTrue} });`, ]))).toEqual([]); }); + +it('flags recursive rmSync, rmdir, and rmdirSync with the same rules', () => { + expect(recursiveRmCalls(sample([ + "import { rmSync, rmdirSync as removeDirSync } from 'node:fs';", + "import { rmdir } from 'node:fs/promises';", + "import * as fs from 'node:fs';", + `rmSync(root, { force: true, ${recursiveTrue} });`, + `removeDirSync(root, { ${recursiveTrue} });`, + `await rmdir(root, { ${recursiveTrue} });`, + `fs.rmSync(root, { ${recursiveTrue} });`, + `await fs.promises.rmdir(root, { ${recursiveTrue} });`, + `(fs.rmdirSync)!(root, { ${recursiveTrue} });`, + ]))).toEqual([4, 5, 6, 7, 8, 9].map((line) => expect.objectContaining({ hasRetries: false, line }))); + + expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', sample([ + "import { rmSync, rmdirSync } from 'node:fs';", + 'rmSync(file);', + `rmSync(root, { ${recursiveTrue}, maxRetries: 5 });`, + `rmdirSync(root, { ${recursiveTrue}, maxRetries: 5 });`, + 'rmdirSync(root);', + `const cleanup = (rmSync) => rmSync(root, { ${recursiveTrue} });`, + `other.rmSync(root, { ${recursiveTrue} });`, + ]))).toEqual([]); +}); + +it('treats maxRetries: 0 as not retried', () => { + expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', sample([ + "import { rm } from 'node:fs/promises';", + "import { rmSync } from 'node:fs';", + `await rm(root, { ${recursiveTrue}, maxRetries: 0 });`, + `rmSync(root, { ${recursiveTrue}, "maxRetries": 0x0 });`, + `await rm(root, { ${recursiveTrue}, maxRetries: 1 });`, + ]))).toEqual([ + 'packages/agent-bundle/tests/example.test.ts:3 bare recursive rm. Use removeTree.', + 'packages/agent-bundle/tests/example.test.ts:4 bare recursive rm. Use removeTree.', + ]); +}); diff --git a/packages/agent-bundle/tests/playground-service.test.ts b/packages/agent-bundle/tests/playground-service.test.ts index 36ca65655..d5475e08d 100644 --- a/packages/agent-bundle/tests/playground-service.test.ts +++ b/packages/agent-bundle/tests/playground-service.test.ts @@ -1,4 +1,4 @@ -import { mkdirSync, readdirSync, renameSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { mkdirSync, readdirSync, renameSync, symlinkSync, writeFileSync } from 'node:fs'; import { appendFile, link, mkdtemp, mkdir, readdir, readFile, rm, symlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; @@ -15,7 +15,7 @@ import { type PlaygroundServiceOptions, type PlaygroundTraceEvent, } from '../src/dev/playground/playground-store.ts'; -import { removeTree } from './support/remove-tree.ts'; +import { removeTree, removeTreeSync } from './support/remove-tree.ts'; interface SessionIndex { readonly kind: 'agent-bundle-playground-session-index'; @@ -1690,7 +1690,7 @@ it('rolls back a failed pre-publication object without changing an unrelated dir const pendingRoot = join(fixture.storageRoot, 'session-index', '.pending'); const blocked = new PlaygroundService({ now: () => { - rmSync(pendingRoot, { force: true, recursive: true }); + removeTreeSync(pendingRoot); writeFileSync(pendingRoot, 'blocked\n', 'utf8'); return new Date('2026-08-16T00:00:00.000Z'); }, diff --git a/packages/agent-bundle/tests/support/remove-tree.test.ts b/packages/agent-bundle/tests/support/remove-tree.test.ts index 7f431b01a..0168c69bb 100644 --- a/packages/agent-bundle/tests/support/remove-tree.test.ts +++ b/packages/agent-bundle/tests/support/remove-tree.test.ts @@ -1,10 +1,10 @@ -import { mkdtemp, rm as removeDirectory, stat, writeFile } from 'node:fs/promises'; +import { mkdir, mkdtemp, rm as removeDirectory, stat, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { expect, it } from '@rstest/core'; -import { removeTree, type TreeRemoval } from './remove-tree.ts'; +import { removeTree, removeTreeSync, type TreeRemoval } from './remove-tree.ts'; const emptyError = Object.assign(new Error('ENOTEMPTY: directory not empty, rmdir'), { code: 'ENOTEMPTY' }); @@ -37,3 +37,12 @@ it('removeTree surfaces a persistent ENOTEMPTY', async () => { expect((await stat(root)).isDirectory()).toBe(true); await removeTree(root); }); + +it('removeTreeSync deletes a nested tree and tolerates a missing path', async () => { + const root = await mkdtemp(join(tmpdir(), 'remove-tree-sync-')); + await mkdir(join(root, 'nested')); + await writeFile(join(root, 'nested', 'kept.txt'), 'x\n'); + removeTreeSync(root); + await expect(stat(root)).rejects.toMatchObject({ code: 'ENOENT' }); + removeTreeSync(root); +}); diff --git a/packages/agent-bundle/tests/support/remove-tree.ts b/packages/agent-bundle/tests/support/remove-tree.ts index e164d4329..d12465458 100644 --- a/packages/agent-bundle/tests/support/remove-tree.ts +++ b/packages/agent-bundle/tests/support/remove-tree.ts @@ -1,3 +1,4 @@ +import { rmSync } from 'node:fs'; import { rm as removeDirectory } from 'node:fs/promises'; const nodeRetryCodes = new Set(['EBUSY', 'EMFILE', 'ENFILE', 'ENOTEMPTY', 'EPERM']); @@ -28,3 +29,8 @@ export const removeTree = async (path: string, fs: TreeRemoval = defaultRemoval) } } }; + +/** For callers that cannot await, such as `exit` handlers. Node retries the same codes with the same linear backoff. */ +export const removeTreeSync = (path: string): void => { + rmSync(path, { force: true, maxRetries, recursive: true, retryDelay }); +}; diff --git a/packages/agent-bundle/tests/support/shared-pack.ts b/packages/agent-bundle/tests/support/shared-pack.ts index dee1e81d1..d8a9a629b 100644 --- a/packages/agent-bundle/tests/support/shared-pack.ts +++ b/packages/agent-bundle/tests/support/shared-pack.ts @@ -1,5 +1,4 @@ import { execFile as executeFile } from 'node:child_process'; -import { rmSync } from 'node:fs'; import { mkdir, mkdtemp, readFile, readdir, symlink } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -8,6 +7,7 @@ import { promisify } from 'node:util'; import { isolatedCommandEnvironment } from '../../../../rstest.worker-isolation.ts'; import { pnpmPack, type PnpmPackOutput as SharedPackOutput } from '../../../../scripts/pnpm-pack.mjs'; import { packOutputFromJson } from '../../src/build/pack-inventory.ts'; +import { removeTreeSync } from './remove-tree.ts'; const execFile = promisify(executeFile); const workspaceRoot = process.cwd(); @@ -124,7 +124,7 @@ const packOnce = async (packageName: SharedPackPackage): Promise => } const destination = await mkdtemp(join(tmpdir(), 'agent-bundle-shared-pack-')); process.once('exit', () => { - rmSync(destination, { force: true, recursive: true }); + removeTreeSync(destination); }); return pnpmPack({ cwd: join(workspaceRoot, 'packages', sharedPackDirectories[packageName]), diff --git a/packages/rsc-runtime/tests/plugin-root.test.ts b/packages/rsc-runtime/tests/plugin-root.test.ts index 94a8b7d5b..cec8ca6ca 100644 --- a/packages/rsc-runtime/tests/plugin-root.test.ts +++ b/packages/rsc-runtime/tests/plugin-root.test.ts @@ -1,5 +1,5 @@ import { createHash } from 'node:crypto'; -import { mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, realpathSync, symlinkSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { basename, join, resolve } from 'node:path'; @@ -14,6 +14,7 @@ import { userDataStateRoot, userStateHome, } from '../src/plugin-root.js'; +import { removeTree } from '../../agent-bundle/tests/support/remove-tree.ts'; const digest16 = (path: string): string => createHash('sha256').update(path).digest('hex').slice(0, 16); @@ -197,7 +198,7 @@ describe('resolvePluginRoot (#468)', () => { expect(pluginStateSegment(root)).toBe(`plugin-${digest16(resolve(root))}`); }); - it('digests real roots canonically and missing roots by their resolved spelling', () => { + it('digests real roots canonically and missing roots by their resolved spelling', async () => { const directory = mkdtempSync(join(tmpdir(), 'agent-bundle-plugin-root-')); try { const root = join(directory, 'curator'); @@ -214,7 +215,7 @@ describe('resolvePluginRoot (#468)', () => { const resolvedMissing = resolve(missing); expect(pluginStateSegment(missing)).toBe(`${basename(resolvedMissing)}-${digest16(resolvedMissing)}`); } finally { - rmSync(directory, { force: true, recursive: true }); + await removeTree(directory); } }); }); diff --git a/scripts/check-test-remove-tree.mjs b/scripts/check-test-remove-tree.mjs index d6db27b84..ffedc9f0f 100644 --- a/scripts/check-test-remove-tree.mjs +++ b/scripts/check-test-remove-tree.mjs @@ -1,11 +1,18 @@ /** - * Test teardown that deletes a tree calls `removeTree`. A bare `rm` with - * `recursive: true` and no `maxRetries` races a late writer and flakes with ENOTEMPTY. + * Test teardown that deletes a tree calls `removeTree` (`removeTreeSync` where it + * cannot await). A bare `rm` with `recursive: true` and no nonzero `maxRetries` + * races a late writer and flakes with ENOTEMPTY. `rmSync`, `rmdir`, and + * `rmdirSync` follow the same rules as `rm`. * * Catches bare `rm(`, aliased `import { rm as remove }` calls, and `ns.rm(` or * `ns.promises.rm(` when `ns` is a namespace/default import from node:fs, fs, or * their /promises forms. The same wrappers as the options argument are unwrapped * around the callee and its object, and `?.` member access counts. + * ponytail: only literal options and direct import bindings are read; options held + * in a variable or spread, a non-literal `recursive`, a non-literal `maxRetries` + * (counted as retried), and indirect bindings (local + * aliases, destructuring, dynamic import/require, `ns['rm']`, `.call`) are not + * followed. Closing that needs data-flow analysis, not a wider AST match. * * Call, option, and import-binding detection is parser-backed (typescript-5): * only real node:fs(/promises) ImportDeclaration bindings count, only Node-bound @@ -36,6 +43,8 @@ const roots = [ const nodeFsSpecifier = /^(?:node:)?fs(?:\/promises)?$/u; +const removalNames = new Set(['rm', 'rmSync', 'rmdir', 'rmdirSync']); + const isRemoveTreeHelper = (file) => /(?:^|\/)remove-tree\.ts$/u.test(file.replaceAll('\\', '/')); const walk = async (directory, files) => { @@ -54,7 +63,7 @@ const walk = async (directory, files) => { }; /** - * Named/aliased rm bindings and namespace/default/`promises` bindings that expose .rm. + * Named/aliased removal bindings and namespace/default/`promises` bindings that expose them. * Import bindings are collected from the TypeScript AST so comments and local * identifiers cannot forge Node fs.rm bindings. */ @@ -99,7 +108,7 @@ export const removalBindings = (text, fileName = 'bindings.ts') => { ? element.name.text : element.propertyName.text; const localName = element.name.text; - if (importedName === 'rm') { + if (removalNames.has(importedName)) { bareNames.add(localName); continue; } @@ -254,7 +263,10 @@ const optionsFlags = (optionsArg) => { if (key === 'recursive' && property.initializer.kind === ts.SyntaxKind.TrueKeyword) { recursive = true; } - if (key === 'maxRetries') hasRetries = true; + if (key === 'maxRetries') { + const retries = unwrapExpression(property.initializer); + hasRetries = !ts.isNumericLiteral(retries) || Number(retries.text) !== 0; + } } return { recursive, hasRetries }; }; @@ -265,7 +277,7 @@ const isNodeBoundRmCall = (callee, bareNames, namespaceNames) => { if (ts.isIdentifier(expression)) { return bareNames.has(expression.text) && !identifierIsLocallyShadowed(expression); } - if (!ts.isPropertyAccessExpression(expression) || expression.name.text !== 'rm') return false; + if (!ts.isPropertyAccessExpression(expression) || !removalNames.has(expression.name.text)) return false; let object = unwrapExpression(expression.expression); if (ts.isPropertyAccessExpression(object) && object.name.text === 'promises') { object = unwrapExpression(object.expression); From 429ab7e4b354133f28fe0ce286121c9d2d59e647 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Fri, 25 Sep 2026 03:16:22 +0000 Subject: [PATCH 2/2] test: give removeTreeSync removeTree's retry loop and name it in the gate message --- .../tests/check-test-remove-tree.test.ts | 18 ++++++------ .../tests/support/remove-tree.test.ts | 23 +++++++++++++++ .../agent-bundle/tests/support/remove-tree.ts | 28 +++++++++++++++---- scripts/check-test-remove-tree.mjs | 2 +- 4 files changed, 56 insertions(+), 15 deletions(-) diff --git a/packages/agent-bundle/tests/check-test-remove-tree.test.ts b/packages/agent-bundle/tests/check-test-remove-tree.test.ts index 183fc11c4..848160d9e 100644 --- a/packages/agent-bundle/tests/check-test-remove-tree.test.ts +++ b/packages/agent-bundle/tests/check-test-remove-tree.test.ts @@ -70,7 +70,7 @@ it('exempts the canonical removeTree helper and formats lint failures', () => { `await fs.rm(root, { ${recursiveTrue} });`, ]); expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', escaped)).toEqual([ - 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree.', + 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree (removeTreeSync if it cannot await).', ]); }); @@ -110,7 +110,7 @@ it('matches $-suffixed removal aliases literally', () => { "import { rm as remove$ } from 'node:fs/promises';", `await remove$(root, { ${recursiveTrue} })`, ]))).toEqual([ - 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree.', + 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree (removeTreeSync if it cannot await).', ]); }); @@ -211,7 +211,7 @@ it('still gates aliased and namespace Node fs.rm without maxRetries', () => { "import { rm as remove } from 'node:fs/promises';", `await remove(path, { ${recursiveTrue} });`, ]))).toEqual([ - 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree.', + 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree (removeTreeSync if it cannot await).', ]); const aliasedRetried = recursiveRmCalls(sample([ @@ -350,13 +350,13 @@ it('parses TSX and JS test files with their own script kind', () => { `await rm(root, { ${recursiveTrue} });`, ]); expect(bareRecursiveRmFailures('packages/workbench/tests/view.test.tsx', tsx)).toEqual([ - 'packages/workbench/tests/view.test.tsx:3 bare recursive rm. Use removeTree.', + 'packages/workbench/tests/view.test.tsx:3 bare recursive rm. Use removeTree (removeTreeSync if it cannot await).', ]); expect(bareRecursiveRmFailures('packages/agent-bundle/tests/fixture.mjs', sample([ "import * as fs from 'node:fs/promises';", `await fs.rm(root, { ${recursiveTrue} });`, ]))).toEqual([ - 'packages/agent-bundle/tests/fixture.mjs:2 bare recursive rm. Use removeTree.', + 'packages/agent-bundle/tests/fixture.mjs:2 bare recursive rm. Use removeTree (removeTreeSync if it cannot await).', ]); }); @@ -379,7 +379,7 @@ it('flags promises-namespace and asserted options without maxRetries', () => { "import { promises as fs } from 'node:fs';", `await fs.rm(path, { ${recursiveTrue} });`, ]))).toEqual([ - 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree.', + 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree (removeTreeSync if it cannot await).', ]); expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', sample([ "import { promises as fs } from 'node:fs';", @@ -395,7 +395,7 @@ it('flags promises-namespace and asserted options without maxRetries', () => { "import { rm } from 'node:fs/promises';", `await rm(root, { ${recursiveTrue} } as const);`, ]))).toEqual([ - 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree.', + 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree (removeTreeSync if it cannot await).', ]); expect(recursiveRmCalls(sample([ @@ -517,7 +517,7 @@ it('treats maxRetries: 0 as not retried', () => { `rmSync(root, { ${recursiveTrue}, "maxRetries": 0x0 });`, `await rm(root, { ${recursiveTrue}, maxRetries: 1 });`, ]))).toEqual([ - 'packages/agent-bundle/tests/example.test.ts:3 bare recursive rm. Use removeTree.', - 'packages/agent-bundle/tests/example.test.ts:4 bare recursive rm. Use removeTree.', + 'packages/agent-bundle/tests/example.test.ts:3 bare recursive rm. Use removeTree (removeTreeSync if it cannot await).', + 'packages/agent-bundle/tests/example.test.ts:4 bare recursive rm. Use removeTree (removeTreeSync if it cannot await).', ]); }); diff --git a/packages/agent-bundle/tests/support/remove-tree.test.ts b/packages/agent-bundle/tests/support/remove-tree.test.ts index 0168c69bb..7a4d3c885 100644 --- a/packages/agent-bundle/tests/support/remove-tree.test.ts +++ b/packages/agent-bundle/tests/support/remove-tree.test.ts @@ -1,3 +1,4 @@ +import { rmSync } from 'node:fs'; import { mkdir, mkdtemp, rm as removeDirectory, stat, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -46,3 +47,25 @@ it('removeTreeSync deletes a nested tree and tolerates a missing path', async () await expect(stat(root)).rejects.toMatchObject({ code: 'ENOENT' }); removeTreeSync(root); }); + +it('removeTreeSync retries ENOTEMPTY with backoff and surfaces a persistent one', async () => { + const root = await mkdtemp(join(tmpdir(), 'remove-tree-sync-retry-')); + await writeFile(join(root, 'kept.txt'), 'x\n'); + let attempts = 0; + const started = Date.now(); + removeTreeSync(root, (path, options) => { + attempts += 1; + if (attempts === 1) throw emptyError; + rmSync(path, options); + }); + expect(attempts).toBe(2); + expect(Date.now() - started).toBeGreaterThanOrEqual(45); + await expect(stat(root)).rejects.toMatchObject({ code: 'ENOENT' }); + + let persistent = 0; + expect(() => removeTreeSync(root, () => { + persistent += 1; + throw emptyError; + })).toThrow(emptyError); + expect(persistent).toBe(6); +}); diff --git a/packages/agent-bundle/tests/support/remove-tree.ts b/packages/agent-bundle/tests/support/remove-tree.ts index d12465458..50e99c90b 100644 --- a/packages/agent-bundle/tests/support/remove-tree.ts +++ b/packages/agent-bundle/tests/support/remove-tree.ts @@ -9,10 +9,21 @@ export type TreeRemoval = { readonly rm: (path: string, options: { readonly force: true; readonly recursive: true }) => Promise; }; +export type SyncTreeRemoval = (path: string, options: { readonly force: true; readonly recursive: true }) => void; + const delay = (milliseconds: number): Promise => new Promise((resolve) => { setTimeout(resolve, milliseconds); }); +const delaySync = (milliseconds: number): void => { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, milliseconds); +}; + +const isRetryable = (error: unknown): boolean => { + const code = typeof error === 'object' && error !== null && 'code' in error ? error.code : undefined; + return typeof code === 'string' && nodeRetryCodes.has(code); +}; + const defaultRemoval: TreeRemoval = { rm: (path, options) => removeDirectory(path, options), }; @@ -23,14 +34,21 @@ export const removeTree = async (path: string, fs: TreeRemoval = defaultRemoval) await fs.rm(path, { force: true, recursive: true }); return; } catch (error) { - const code = typeof error === 'object' && error !== null && 'code' in error ? error.code : undefined; - if (attempt === maxRetries || typeof code !== 'string' || !nodeRetryCodes.has(code)) throw error; + if (attempt === maxRetries || !isRetryable(error)) throw error; await delay(retryDelay * (attempt + 1)); } } }; -/** For callers that cannot await, such as `exit` handlers. Node retries the same codes with the same linear backoff. */ -export const removeTreeSync = (path: string): void => { - rmSync(path, { force: true, maxRetries, recursive: true, retryDelay }); +/** `removeTree` for callers that cannot await, such as `exit` handlers. */ +export const removeTreeSync = (path: string, remove: SyncTreeRemoval = rmSync): void => { + for (let attempt = 0; attempt <= maxRetries; attempt += 1) { + try { + remove(path, { force: true, recursive: true }); + return; + } catch (error) { + if (attempt === maxRetries || !isRetryable(error)) throw error; + delaySync(retryDelay * (attempt + 1)); + } + } }; diff --git a/scripts/check-test-remove-tree.mjs b/scripts/check-test-remove-tree.mjs index ffedc9f0f..2d1fbdaae 100644 --- a/scripts/check-test-remove-tree.mjs +++ b/scripts/check-test-remove-tree.mjs @@ -329,7 +329,7 @@ export const bareRecursiveRmFailures = (file, text) => { const failures = []; for (const call of recursiveRmCalls(text, file)) { if (call.hasRetries) continue; - failures.push(`${file}:${call.line} bare recursive rm. Use removeTree.`); + failures.push(`${file}:${call.line} bare recursive rm. Use removeTree (removeTreeSync if it cannot await).`); } return failures; };