fix(tools): close the async test-cleanup invariant's gaps, and stop asserting one Node version's rmSync bug

tools/invariants/test-cleanup-retries-asynchronously.mjs

@@ -1,23 +1,38 @@
 /**
  * Test cleanup that asks for retries gets them.
  *
- * On Windows, `rmSync(path, { recursive: true, force: true, maxRetries })` reports a held directory as `EPERM` or
- * `EBUSY` at once: the retries never run (measured on Node 22 and 24). `fs.promises.rm` with the same options does retry.
- * A spec that cleans up with a retrying `rmSync` therefore fails on Windows as soon as an antivirus, an indexer or a
- * child process that just exited holds a file for a moment, which is exactly what the retries were written for. Test
- * code removes such a directory with `removeTestDirectory()` from `tools/test-cleanup.ts` (or awaits `rm` from
- * `node:fs/promises`) instead.
+ * On Windows, `rmSync(path, { recursive: true, force: true, maxRetries })` cannot be relied on to wait out a directory
+ * that an antivirus, an indexer or a child process that just exited holds for a moment, which is exactly what the
+ * retries are written for:
+ *
+ * - On Node 22, and on Node 24 before 24.21, it reports the held directory as `EBUSY` or `EPERM` at once: the retries
+ *   never run.
+ * - From Node 24.21 (nodejs/node#64698) the retries run, but each wait sleeps the main thread. The event loop stands
+ *   still for the whole budget, seconds of it, so nothing the test process would do meanwhile happens: no timer fires,
+ *   no child's exit is handled, and a handle the test itself holds is not let go.
+ *
+ * `fs.promises.rm` with the same options retries on every supported version without blocking. Test code removes such
+ * a directory with `removeTestDirectory()` from `tools/test-cleanup.ts` (or awaits `rm` from `node:fs/promises`) instead.
  *
  * How the check reads a file:
  *
  * - Comments and the text of string literals are blanked first, so a call named in a comment or a string is not a call,
  *   and a `(` in a string does not unbalance the call around it. Code inside a template literal's `${...}` is still read.
- *   A regular expression literal is read as code, so one holding a quote can blank what follows; none does today.
- * - A call is read whole by balancing parentheses, so an options object that wraps across lines is still seen.
+ * - A call is read whole by balancing parentheses, so an options object that wraps across lines is still seen. An
+ *   optional call (`fs.rmSync?.(...)`) and bracket access with a literal key (`fs["rmSync"](...)`) count as calls.
  * - `rmSync` reached under another name is seen when the name is given in the file: `rmSync as name` in an import,
- *   `{ rmSync: name }` in a destructuring, or `const name = rmSync`. Options passed as a variable are not seen.
+ *   `{ rmSync: name }` in a destructuring, or `const name = rmSync` (also `fs.rmSync`, `require("node:fs").rmSync` or
+ *   `fs["rmSync"]`). Options passed as a variable are not seen.
  * - A call that is meant to show the failing form carries `invariant-allow: sync-rm-retries` in a comment on its line or
- *   the line above.
+ *   the line above. The marker in a string literal does not count.
+ *
+ * Known limits, since the check reads text and does not parse:
+ *
+ * - A regular expression literal is read as code, so one holding a quote can blank what follows on its line; none does
+ *   today.
+ * - JSX text is read as code. A quote right after a letter or digit (`<p>Don't</p>`) is taken as an apostrophe, never
+ *   valid code there, so it hides nothing. A quote in JSX text after a space or a tag (`<p>It is 'odd</p>`,
+ *   `<p>"Quoted</p>`) still opens a string, and blanks what follows on its line.
  *
  * It covers test code: specs and the helpers beside them in `test/` and `e2e/` folders, plus the test tooling CI runs on
  * every OS (`TEST_TOOLING`).
@@ -27,25 +42,33 @@ import { join } from "node:path";
 import { readFileSync } from "./context.mjs";
 
 const TEST_ROOTS = ["apps", "packages", "packs", "tools", "examples"];
-const SOURCE = /\.(?:[cm]?[jt]s|tsx)$/;
+const EXTENSION = String.raw`\.(?:[cm]?[jt]s|[jt]sx)$`;
+const SOURCE = new RegExp(EXTENSION);
+const SPEC = new RegExp(String.raw`\.(?:spec|test)${EXTENSION}`);
 /** Test tooling outside `test/` folders that CI runs, Windows included. */
 const TEST_TOOLING = new Set(["tools/smoke-widget-tooling.mjs"]);
 const ALLOW = "invariant-allow: sync-rm-retries";
 
 /** Whether a repo-relative path is test code. */
 export function isTestPath(path) {
-  return TEST_TOOLING.has(path) || /(?:^|\/)(?:test|e2e)\//.test(path) || /\.(?:spec|test)\.[cm]?[jt]sx?$/.test(path);
+  return TEST_TOOLING.has(path) || /(?:^|\/)(?:test|e2e)\//.test(path) || SPEC.test(path);
 }
 
 /**
- * `source` with comments and string-literal text replaced by spaces, newlines kept, so every index and line still
- * points at the same place. Template literals are followed into their `${...}` expressions.
+ * `source` read two ways, each the same length with newlines kept, so every index and line still points at the same
+ * place: `code` has comments and string-literal text replaced by spaces, and `comments` keeps only the comments.
+ * Template literals are followed into their `${...}` expressions.
  */
-export function blankCommentsAndStrings(source) {
+function scan(source) {
   const out = source.split("");
+  const comments = source.replace(/[^\n]/g, " ").split("");
   const blank = (index) => {
     if (out[index] !== "\n") out[index] = " ";
   };
+  const blankComment = (index) => {
+    comments[index] = source[index];
+    blank(index);
+  };
   /** What encloses the current position: a template literal, or a `${` expression with its own brace depth. */
   const stack = [];
   let index = 0;
@@ -71,11 +94,14 @@ export function blankCommentsAndStrings(source) {
       continue;
     }
     if (character === "/" && next === "/") {
-      while (index < source.length && source[index] !== "\n") blank(index++);
+      while (index < source.length && source[index] !== "\n") blankComment(index++);
     } else if (character === "/" && next === "*") {
       const end = source.indexOf("*/", index + 2);
       const stop = end === -1 ? source.length : end + 2;
-      while (index < stop) blank(index++);
+      while (index < stop) blankComment(index++);
+    } else if ((character === '"' || character === "'") && /[\w$]/.test(source[index - 1] ?? "")) {
+      // A quote right after a name or a number cannot open a string in code; it is an apostrophe in JSX text.
+      index += 1;
     } else if (character === '"' || character === "'") {
       index += 1;
       while (index < source.length && source[index] !== character && source[index] !== "\n") {
@@ -98,6 +124,29 @@ export function blankCommentsAndStrings(source) {
       index += 1;
     }
   }
+  return { code: out.join(""), comments: comments.join("") };
+}
+
+/** `source` with comments and string-literal text replaced by spaces, newlines kept. */
+export function blankCommentsAndStrings(source) {
+  return scan(source).code;
+}
+
+/**
+ * `code` with each bracket access by a literal key, `["rmSync"]`, written as `.rmSync` padded to the same length, so the
+ * rest of the check reads it as member access. `code` has the key blanked, so `source` names it.
+ */
+function bracketAccessAsMember(code, source) {
+  const out = code.split("");
+  for (const match of source.matchAll(/\[(\s*)(["'`])rmSync\2\s*\]/g)) {
+    const quote = match.index + 1 + match[1].length;
+    // Only where the brackets and quotes are code: not inside a comment or a string.
+    if (code[match.index] !== "[" || code[quote] !== match[2] || code[quote + 7] !== match[2]) continue;
+    const replacement = ".rmSync".padEnd(match[0].length, " ");
+    for (let offset = 0; offset < match[0].length; offset += 1) {
+      if (out[match.index + offset] !== "\n") out[match.index + offset] = replacement[offset];
+    }
+  }
   return out.join("");
 }
 
@@ -118,7 +167,8 @@ function callText(code, open) {
 /** The names `rmSync` goes by in `code`: its own, and any alias the file gives it. */
 function rmSyncNames(code) {
   const names = new Set(["rmSync"]);
-  for (const pattern of [/\brmSync\s+as\s+([A-Za-z_$][\w$]*)/g, /\brmSync\s*:\s*([A-Za-z_$][\w$]*)/g, /\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:[\w$]+\.)?rmSync\b(?!\s*\()/g]) {
+  const assigned = /\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:[\w$]+\s*(?:\([^()]*\))?\s*\.\s*)*rmSync\b(?!\s*(?:\?\.\s*)?\()/g;
+  for (const pattern of [/\brmSync\s+as\s+([A-Za-z_$][\w$]*)/g, /\brmSync\s*:\s*([A-Za-z_$][\w$]*)/g, assigned]) {
     for (const match of code.matchAll(pattern)) names.add(match[1]);
   }
   return [...names];
@@ -128,17 +178,18 @@ const escapeName = (name) => name.replace(/\$/g, "\\$");
 
 /** The 1-based line of each `rmSync(...)` call in `source` that passes `maxRetries`. */
 export function retryingRmSyncLines(source) {
-  const code = blankCommentsAndStrings(source);
-  const sourceLines = source.split("\n");
+  const scanned = scan(source);
+  const code = bracketAccessAsMember(scanned.code, source);
+  const commentLines = scanned.comments.split("\n");
   const names = rmSyncNames(code).map(escapeName).join("|");
   const lines = [];
-  for (const match of code.matchAll(new RegExp(`(?<![\\w$])(?:${names})\\s*\\(`, "g"))) {
+  for (const match of code.matchAll(new RegExp(`(?<![\\w$])(?:${names})\\s*(?:\\?\\.\\s*)?\\(`, "g"))) {
     // A declaration of the name (`function rmSync(`) is not a call; neither is the import or alias itself.
     if (/\bfunction\s+$/.test(code.slice(Math.max(0, match.index - 12), match.index))) continue;
     const open = match.index + match[0].length - 1;
     if (!/\bmaxRetries\b/.test(callText(code, open))) continue;
     const line = code.slice(0, match.index).split("\n").length;
-    if (sourceLines[line - 1]?.includes(ALLOW) || sourceLines[line - 2]?.includes(ALLOW)) continue;
+    if (commentLines[line - 1]?.includes(ALLOW) || commentLines[line - 2]?.includes(ALLOW)) continue;
     lines.push(line);
   }
   return lines;
@@ -153,7 +204,7 @@ export default function run(ctx) {
   for (const path of files) {
     for (const line of retryingRmSyncLines(readFileSync(join(repoRoot, path), "utf8"))) {
       c.failures.push(
-        `${path}:${String(line)} calls rmSync with maxRetries, which never retries on Windows; await removeTestDirectory() from tools/test-cleanup.ts`,
+        `${path}:${String(line)} calls rmSync with maxRetries, which on Windows fails at once or blocks the event loop while it retries; await removeTestDirectory() from tools/test-cleanup.ts`,
       );
     }
   }

tools/test-cleanup.ts

@@ -13,9 +13,10 @@ import { it } from "vitest";
  * A process that just exited can keep a handle open for a moment: a closed Chromium's helper processes, or a git that
  * had the directory as its working directory. Removal then fails with `EPERM` until the handle is gone.
  *
- * This is the promise form on purpose. On Windows (Node 22 and 24 alike), `rmSync` reports a locked file as `EPERM` or
- * `EBUSY` at once and ignores `maxRetries`, so a synchronous removal with retries is a removal without them. `fs.promises.rm` waits
- * `retryDelay` longer after each failed attempt, about five seconds over ten attempts.
+ * This is the promise form on purpose. On Windows, `rmSync` with `maxRetries` reports a held directory as `EPERM` or
+ * `EBUSY` at once on Node 22 and on Node 24 before 24.21, so the retries never run. From Node 24.21 it retries, but it
+ * sleeps the main thread between attempts, so the event loop stands still for the whole wait. `fs.promises.rm` waits
+ * `retryDelay` longer after each failed attempt, about five seconds over ten attempts, without blocking.
  */
 export async function removeTestDirectory(path: string): Promise<void> {
   await rm(path, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 });

tools/test/test-cleanup-retries.spec.ts

@@ -1,11 +1,12 @@
 import { spawn } from "node:child_process";
-import { existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
-import { join } from "node:path";
+import { dirname, join } from "node:path";
 
 import { describe, expect, it } from "vitest";
 
-import { isTestPath, retryingRmSyncLines } from "../invariants/test-cleanup-retries-asynchronously.mjs";
+import { repoRelativePath, walk } from "../invariants/context.mjs";
+import runCheck, { isTestPath, retryingRmSyncLines } from "../invariants/test-cleanup-retries-asynchronously.mjs";
 import { removeTestDirectory } from "../test-cleanup.ts";
 
 // The sources below are strings, which the check blanks before it reads a file, so this file does not read as the calls
@@ -85,6 +86,70 @@ describe("the check on retrying synchronous removal in test code", () => {
     expect(retryingRmSyncLines(source)).toEqual([5]);
   });
 
+  it("finds rmSync taken from require under another name", () => {
+    const source = [
+      'const wipe = require("node:fs").rmSync;',
+      "const erase = require('fs').rmSync",
+      "wipe(dir, { maxRetries: 3 });",
+      "erase(dir, { maxRetries: 3 });",
+    ].join("\n");
+    expect(retryingRmSyncLines(source)).toEqual([3, 4]);
+  });
+
+  it("finds an optional call", () => {
+    const source = ["fs.rmSync?.(dir, { maxRetries: 3 });", "rmSync ?. (dir, { maxRetries: 3 });", "fs.rmSync?.(dir, { recursive: true });"].join("\n");
+    expect(retryingRmSyncLines(source)).toEqual([1, 2]);
+  });
+
+  it("finds a call by bracket access, and an alias taken that way", () => {
+    const source = [
+      'fs["rmSync"](dir, { maxRetries: 3 });',
+      "fs[ 'rmSync' ](dir, {",
+      "  maxRetries: 3 });",
+      'const wipe = fs["rmSync"];',
+      "wipe(dir, { maxRetries: 3 });",
+      'fs["rmSync"](dir, { recursive: true });',
+      'const text = \'fs["rmSync"](dir, { maxRetries: 3 })\';',
+      '// fs["rmSync"](dir, { maxRetries: 3 })',
+    ].join("\n");
+    expect(retryingRmSyncLines(source)).toEqual([1, 2, 5]);
+  });
+
+  it("counts the marker only in a comment, not in a string", () => {
+    const source = [
+      'const note = "invariant-allow: sync-rm-retries";',
+      "rmSync(dir, { maxRetries: 3 });",
+      "rmSync(dir, { maxRetries: 3, label: 'invariant-allow: sync-rm-retries' });",
+      "/* invariant-allow: sync-rm-retries */ rmSync(dir, { maxRetries: 3 });",
+    ].join("\n");
+    expect(retryingRmSyncLines(source)).toEqual([2, 3]);
+  });
+
+  it("does not let an apostrophe in JSX text hide a call later on its line", () => {
+    const source = [
+      "const view = <p>Don't</p>; rmSync(dir, { maxRetries: 3 });",
+      'const size = <p>6" wide</p>; rmSync(dir, { maxRetries: 3 });',
+      "const name = 'it\\'s'; rmSync(dir, { recursive: true });",
+    ].join("\n");
+    expect(retryingRmSyncLines(source)).toEqual([1, 2]);
+  });
+
+  it("reads every file it counts as test code, a .jsx spec included", async () => {
+    const root = mkdtempSync(join(tmpdir(), "clarkcant-cleanup-check-"));
+    try {
+      const call = "rmSync(dir, { maxRetries: 3 });\n";
+      for (const path of ["apps/web/src/view.spec.jsx", "apps/web/test/helper.jsx", "apps/web/src/view.jsx", "apps/web/test/notes.md"]) {
+        mkdirSync(dirname(join(root, path)), { recursive: true });
+        writeFileSync(join(root, path), call);
+      }
+      const result = { failures: [] as string[], notes: [] as string[] };
+      runCheck({ repoRoot: root, walk, relative: (target: string) => repoRelativePath(root, target), check: () => result });
+      expect(result.failures.map((failure) => failure.split(":")[0]).sort()).toEqual(["apps/web/src/view.spec.jsx", "apps/web/test/helper.jsx"]);
+    } finally {
+      await removeTestDirectory(root);
+    }
+  });
+
   it("covers specs, the helpers in test and e2e folders and CI's widget tooling smoke, not product code", () => {
     expect(isTestPath("apps/runtime/test/live-nodes.ts")).toBe(true);
     expect(isTestPath("apps/web/e2e/fixtures/server.mjs")).toBe(true);
@@ -124,20 +189,21 @@ async function holdAsWorkingDirectory(dir: string): Promise<{ release: () => Pro
  * say anything.
  */
 describe.runIf(process.platform === "win32")("removing a directory Windows still holds", () => {
-  it("fails at once in the synchronous form, though it asks for retries", async () => {
+  // Node 22 and Node 24 before 24.21 fail at once without retrying; from 24.21 the retries run but sleep the main thread.
+  // Either way the release the test schedules cannot happen during the call, so the outcome is the same on every version.
+  it("fails in the synchronous form though it asks for retries, because the release it waits for cannot run", async () => {
     const dir = mkdtempSync(join(tmpdir(), "clarkcant-cleanup-retry-"));
     writeFileSync(join(dir, "file.txt"), "held");
     const held = await holdAsWorkingDirectory(dir);
+    // Let go 100 ms in, well within the retry budget below (100 + 200 + 300 + 400 ms), as removeTestDirectory's test does.
+    const released = new Promise<void>((done) => setTimeout(() => void held.release().then(done), 100));
     try {
-      const started = Date.now();
       // The failing form, on purpose: the check this file tests would otherwise flag it as test cleanup.
       // invariant-allow: sync-rm-retries
-      expect(() => rmSync(dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 })).toThrow(/EPERM|EBUSY/);
-      // Ten retries 100 ms apart and longer each time would take seconds; none ran.
-      expect(Date.now() - started).toBeLessThan(500);
+      expect(() => rmSync(dir, { recursive: true, force: true, maxRetries: 4, retryDelay: 100 })).toThrow(/EPERM|EBUSY/);
       expect(existsSync(dir)).toBe(true);
     } finally {
-      await held.release();
+      await released;
       await removeTestDirectory(dir);
     }
   });