Retry the atomic rename when Windows briefly locks the temp file

[rzilem]

The problem

On Windows, a virus scanner or indexer can hold a short-lived handle on a file the moment it is created. A rename against that handle fails with EPERM, EACCES or EBUSY.

atomicWrite is unusually exposed to this: it creates a temp file and immediately renames it over state.json. That is the narrowest possible window for a scanner to still be holding the file it just saw appear.

This is not a concurrency problem. It happens with a single process writing sequentially. On my machine (Windows 11, Node 24, McAfee real-time protection):

500 sequential atomicWrite calls, system temp dir : 19 failed, EPERM on rename
500 sequential atomicWrite calls, ~/.human-review : 23 failed, EPERM on rename

Roughly one in twenty-five writes, and each one is a batch of comments or edits the user had already typed into the browser.

I first noticed it because npm test was failing differently on each run — the failures moved between state.test.js, concurrent.test.js and others, which is what sent me looking at atomicWrite rather than at any one test.

The fix

The handle clears within a few milliseconds, so the write is not really lost — it just has to be asked for again. The rename retries with a short exponential backoff. Same measurement after the change:

500 sequential atomicWrite calls : 0 failed, 14 needed a retry, worst case 2 retries (~3ms)

Two things beyond the obvious retry:

Cleanup needs it too. The handle that blocks the rename blocks the unlink for exactly as long, so the single cleanup attempt could strand a .human-review.tmp file containing the newest state. Nothing ever sweeps that directory — prune() only ages out entries inside the JSON — so it would sit there indefinitely. Cleanup now retries on the same schedule, then gives up quietly: a stuck temp file must never mask the error the caller is already throwing.

Neither sleep can escape. For the same reason, an exception from the backoff would otherwise surface in place of the real write error.

What is deliberately unchanged

• The exported signature. atomicWrite(file, data) is exactly as it was. The injection the tests use is private, so no caller can hand it a rename that quietly does nothing and reports a successful write. Exports remain atomicWrite, Store, resolveAsset.
• Non-retryable errors still fail on the first attempt. ENOSPC and friends do not sleep and do not retry.
• The retry is bounded, so a genuinely stuck file still surfaces its error rather than hanging.
• The security properties: exclusive wx create, unguessable temp name, symlink resistance.

Tests

Ten new tests in test/state.test.js. They reach the retry by patching the calls atomicWrite makes — fs.renameSync, fs.unlinkSync, and Atomics.wait (which only sleepSync uses) — so every one of them drives the real two-argument atomicWrite rather than a test-only seam.

I checked they actually fail without each fix, by reverting one piece at a time:

Reverted	Result
One-shot rename (no retry)	7 tests fail
RENAME_TRIES 5 → 6	3 tests fail
EBUSY dropped from the retryable set	1 test fails
Single-attempt cleanup	2 tests fail
Cleanup's final-attempt guard	1 test fails
Cleanup sleep containment	1 test fails
Retry sleep containment	1 test fails
sleepSync → no-op	7 tests fail

node --test → 101 tests, 100 pass, 1 skipped (the existing symlink test, which needs Windows privileges).

One note on running the suite

npm test reports 3 files as failing on Windows:

Assertion failed: !(handle->flags & UV_HANDLE_CLOSING), file src\win\async.c, line 76

Every assertion inside those files passes — it is --test-force-exit crashing libuv during teardown for the files that open a server (asset-paste, command-cache, url-review). Plain node --test exits cleanly. Unrelated to this change and not something I touched, but it may be worth knowing if Windows CI ever gets set up.