Skip to content

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

Merged
mrgoonie merged 1 commit into
mainfrom
fix/629-test-cleanup-invariant-gaps
Oct 8, 2026
Merged

mrgoonie merged 1 commit into
mainfrom
fix/629-test-cleanup-invariant-gaps

Conversation

@mrgoonie

@mrgoonie mrgoonie commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #629. Refs #563, #622.

This also fixes the verify (windows-latest, node 24) failure seen on PRs #627 and #630, in tools/test/test-cleanup-retries.spec.ts: fails at once in the synchronous form, though it asks for retries (expected 6628 to be less than 500 on #630, expected 6606 to be less than 500 on #627).

The CI failure: not a flake, a Node change

Where Node What rmSync(held, { maxRetries, retryDelay: 100 }) does on Windows
Node 22 (22.19 and 22.23.2 checked) JS rimraf throws EBUSY at once; EBUSY is not retried
Node 24 before 24.21 (local: 24.11.0, also 24.20.0 source) C++ RmSync throws EPERM at once: a held directory comes back as permission_denied, which was not in the retryable set, and the Windows wait was Sleep(i * retryDelay / 1000), which is 0 ms
Node 24.21.0 and later (CI's setup-node with node-version: 24 resolved to v24.21.0) C++ RmSync retries for real: permission_denied became retryable and the wait became Sleep(i * retryDelay) in milliseconds

The change is nodejs/node#64698 (commit b269616936, "fs: treat std::errc::permission_denied as EPERM error"), which fixes nodejs/node#64016 and first shipped in v24.21.0. It is not in v25.0.0 or v25.2.0. With maxRetries: 10, retryDelay: 100, the waits add up to 5.5 s, which with the attempts matches the 6.6 s CI saw.

So the old test asserted one Node version's bug. The invariant still holds, for an accurate reason:

  • on Node 22 and on Node 24 before 24.21, the synchronous form never retries a held directory;
  • from Node 24.21 it retries, but Sleep runs on the main thread, so the event loop stands still for the whole budget. No timer fires and no child's exit is handled during it, so a hold the test process itself would release is never released.

fs.promises.rm retries on every supported version without blocking.

The test now holds the directory with a child process and schedules the release from the test's own event loop 100 ms in, well inside the retry budget (maxRetries: 4, retryDelay: 100, so 1 s of waits on 24.21). The synchronous call blocks the event loop, so the release cannot run during it on any version: it throws EPERM/EBUSY and the directory is still there. There is no wall-clock assertion left. The existing test beside it still shows that removeTestDirectory() waits out the same kind of hold and removes the directory.

The module's doc comment, its failure message, and the doc comment of removeTestDirectory() in tools/test-cleanup.ts now state this reason instead of "never retries". docs/ does not repeat the claim.

The gaps from #629

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

Gap Change
const x = require("node:fs").rmSync The alias pattern accepts any member chain, with call parentheses, before .rmSync.
fs.rmSync?.(...) A call may have ?. before its (. An alias is not taken from an optional call either.
fs["rmSync"](...), const x = fs["rmSync"] Bracket access with a literal key (", ' or `) is rewritten to .rmSync, padded to the same length, where the brackets and quotes are code. The same text inside a string or a comment is left alone.
The marker inside a string literal The scan now also keeps a copy with only the comments. The marker counts only there.
<p>Don't</p> hiding a later call A ' or " right after a letter, digit, _ or $ cannot open a string in code, so it is read as an apostrophe. A quote in JSX text after a space or a tag (<p>It is 'odd</p>) still opens a string. That remaining case is listed under the module's known limits, next to the regular-expression-literal limit.
.spec.jsx accepted, .jsx never walked One extension pattern now drives both the walk and the spec filter, and .jsx is included. No .jsx file exists in the repo today.

The object-key nit ({ rmSync: spy } makes spy an alias) is unchanged. The issue calls it harmless: it can only add a name, never hide a call.

Tests

Six new tests in tools/test/test-cleanup-retries.spec.ts, one per gap. The .jsx test runs the check's run() over a temporary tree.

All six fail against main's module. The spec was run with only tools/invariants/test-cleanup-retries-asynchronously.mjs checked out from origin/main (Windows 11, Node v24.11.0):

Test On main
finds rmSync taken from require under another name expected [] to deeply equal [ 3, 4 ]
finds an optional call expected [] to deeply equal [ 1, 2 ]
finds a call by bracket access, and an alias taken that way expected [] to deeply equal [ 1, 2, 5 ]
counts the marker only in a comment, not in a string expected [] to deeply equal [ 2, 3 ]
does not let an apostrophe in JSX text hide a call later on its line expected [] to deeply equal [ 1, 2 ]
reads every file it counts as test code, a .jsx spec included expected [] to deeply equal [ 'apps/web/src/view.spec.jsx', …(1) ]

Verification (Windows 11)

Check Result
tools/test/test-cleanup-retries.spec.ts, Node v24.11.0, 6 runs 16/16 each run
The same spec on Node v22.23.2 16/16
The same spec on Node v24.21.0 not run locally (not installed); this PR's own Windows CI job runs it
pnpm invariants 15/15 pass; 707 test files checked, no new findings
pnpm typecheck exit 0
eslint on the 3 changed files exit 0
pnpm verify exit 0: 573 files passed, 1 skipped; 7761 tests passed

Overlap with open PRs

No open PR touches these three files. #631 (#626) changes production rmSync sites in apps/runtime, outside this check's scope.

…eanup invariant

The check now sees an alias taken from require, an optional call and bracket
access with a literal key, counts the exemption marker only inside a comment,
reads an apostrophe in JSX text as text, and walks .jsx files as its spec
filter already expected.

The Windows test of the synchronous form no longer asserts a timing that only
held before Node 24.21, where rmSync started to retry with real delays while
blocking the event loop. It now holds the directory until a release the test
schedules, which cannot run during the synchronous call on any version.
@mrgoonie

mrgoonie commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Review attestation: ready to merge at cc8bceace6680c40ff86f9f7631b4d6e265b6a90, reviewed by agent:code-reviewer.

A push to this PR makes this attestation stale; the new head needs its own review.

@mrgoonie
mrgoonie enabled auto-merge (squash) October 8, 2026 01:59
@mrgoonie
mrgoonie merged commit 432344b into main Oct 8, 2026
22 checks passed
@mrgoonie
mrgoonie deleted the fix/629-test-cleanup-invariant-gaps branch October 8, 2026 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Close the remaining gaps in the async test-cleanup invariant Inconsistent retryDelay in fs.rmSync and asynchronous fs.rm (on Windows)

1 participant