fix(windows): ride out longer AV holds in atomicWriteFile's rename retry - #2041
Merged
Conversation
… spinning The ~910ms backoff budget (8 retries, 200ms cap) was exhausted twice in a row by the same test on a Windows CI runner (harper#2036) - an AV real-time scan can hold harper-config.yaml for over a second. Widen to 12 retries with a 500ms cap (~3.6s worst case), and replace the performance.now() busy-spin with a timeout-only Atomics.wait so the wait is CPU-idle, which is what makes the longer budget affordable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cb1kenobi
requested review from
Ethan-Arrowood and
kriszyp
and removed request for
Ethan-Arrowood
August 1, 2026 00:42
Contributor
There was a problem hiding this comment.
Code Review
This pull request optimizes the synchronous retry mechanism in configUtils by replacing a CPU-spinning busy-loop with Atomics.wait on a shared buffer, yielding the thread to the OS during delays. Additionally, the retry budget has been increased from 8 to 12 attempts and the maximum delay from 200ms to 500ms to better handle transient file locks. Corresponding unit tests have been updated to reflect the new retry count. There are no review comments to address, so I have no further feedback to provide.
Contributor
|
Reviewed; no blockers found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The Windows Integration 6/6 job on #2036 (chore(deps): Update rocksdb-js to 2.6.1) failed twice in a row (then passed on a third run) with:
The failing test's duration (~949ms) matches
atomicWriteFile's full retry budget (~910ms: 8 retries, 200ms cap) — the budget added in #1714 (fix(windows): widen atomicWriteFile rename-retry backoff for EPERM/EACCES) was exhausted, not skipped. Node readers can't cause this (libuv opens with full sharing flags); the holder is almost certainly Defender/AV real-time scanning, and a single scan pass can exceed a second.Change
Atomics.waiton a module-levelSharedArrayBufferinstead of aperformance.now()busy-spin — the thread yields to the OS instead of burning CPU, which is what makes the longer worst case affordable. Happy path is unchanged (no wait unless a rename actually fails).Worst case still blocks the calling thread's event loop for ~3.6s, but only on the path that previously threw after ~0.9s of spinning; call sites are synchronous by design.
Verification
unitTests/config/configUtils.test.js(72 passing) — including the retry-exhaustion test updated to pin the new production budget (13 rename calls).Atomics.waittimed sleep verified on the main thread on Node 24 (returnstimed-outafter the requested delay).Atomics.wait, timeout-only safety of the never-notified buffer, and the backoff arithmetic; its one finding (unusedperformanceimport breaking lint) is fixed.Generated by Claude (Fable 5).
🤖 Generated with Claude Code