Skip to content

fix(windows): widen atomicWriteFile rename-retry backoff for EPERM/EACCES#1714

Open
kriszyp wants to merge 1 commit into
mainfrom
kris/win-config-rename-eperm
Open

fix(windows): widen atomicWriteFile rename-retry backoff for EPERM/EACCES#1714
kriszyp wants to merge 1 commit into
mainfrom
kris/win-config-rename-eperm

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

Widens the retry-on-EPERM/EACCES loop in config/configUtils.ts's atomicWriteFile (the write-temp-file-then-rename helper used by every config write path: createConfigFile, checkForUpdatedConfig, updateConfigValue, addConfig, deleteConfigFromFile, applyRuntimeEnvVarConfig) from a 50ms total budget to exponential backoff (~910ms worst case), switches the busy-wait clock to a monotonic source, and makes the retry parameters injectable for fast unit tests.

Root cause

Discovered incidentally while investigating an unrelated PR (#1713, kris/hnsw-delete-ep-flake) — a Windows CI job failed with:

EPERM: operation not permitted, rename '...harper-config.yaml.9072.0.413a8514.tmp' -> '...harper-config.yaml'

Rerunning the same job passed clean, confirming a low-frequency flake rather than a deterministic break.

atomicWriteFile already had a retry-on-EPERM/EACCES loop, added in a prior commit for this exact class of Windows issue ("atomic renames on Windows will throw EPERM if the target file is momentarily held open for reading by another process or thread"). That loop was under-budgeted: 5 retries × a fixed 10ms sleep = 50ms total.

Windows has no POSIX-style "replace an open file" semantics — rename() fails with EPERM/EACCES if the destination is momentarily open elsewhere. Every Harper worker thread runs its own RootConfigWatcher (chokidar-based hot-reload watcher, see config/RootConfigWatcher.ts), which does an async readFile on the config file whenever it changes. A config write on one thread therefore routinely races a hot-reload read on a sibling thread; Windows Defender / AV real-time scanning can hold a similar transient handle. 50ms just isn't always enough for the reader to close its handle.

Fix

  • Widen the retry budget to exponential backoff: 8 retries, 10ms initial delay, doubling, capped at 200ms/attempt (~910ms worst case for a persistently-held handle).
  • Switch the busy-wait clock from Date.now() to performance.now() — monotonic, so a backward wall-clock jump (e.g. NTP sync) can't turn the spin into an unbounded stall (caught in cross-model review).
  • Add an options parameter (maxRetries/initialDelayMs/maxDelayMs, all defaulting to the production values) so unit tests can exercise the real default retry count without paying the real ~900ms wall-clock cost for the exhaustion path.
  • Add unit tests: retry-then-succeed, exhaust-retries-cleans-up-temp-file-and-rethrows, non-retryable-error-does-not-retry.

Where to look / accepted tradeoffs

  • Worst-case stall widened 50ms → ~910ms. Under persistent EPERM/EACCES, the synchronous busy-loop now blocks the calling worker thread's event loop for up to ~910ms instead of ~50ms. This only affects the contended-write path (rare: admin config changes, boot-time config reconciliation) — the happy path is unaffected. Accepted as the right tradeoff to reliably ride out the Windows race; flagging so a reviewer doesn't need to rediscover it.
  • Same-thread holder is still an unresolved edge case. If the handle is held by an async operation on the same thread (rather than a sibling worker thread), the busy-wait blocks that operation's own completion callback, guaranteeing all retries burn before throwing. This isn't a new regression — the synchronous-busy-wait design predates this change — and it isn't the failure mode this PR fixes (the real-world contention is cross-thread RootConfigWatcher reads / AV, which do ride out the wait). Worth being aware of if this flake recurs after this fix ships.

Reviewed via cross-model review (thorough mode: Gemini + Codex legs + Harper-domain adjudication) — no blockers; the two items above are the adjudicated open callouts.

Generated with Claude Sonnet 5.

…CCES

The existing retry-on-EPERM/EACCES loop in atomicWriteFile only budgeted
5 retries x fixed 10ms (50ms total) - too tight for the actual race: every
worker thread runs its own RootConfigWatcher (chokidar), which does an
async readFile whenever the config file changes, so a write on one thread
routinely races a hot-reload read on a sibling thread on Windows (no
POSIX-style atomic-replace-open-file semantics), plus possible AV scanner
interference. Widen to exponential backoff (8 retries, 10ms initial,
doubling, capped at 200ms/attempt, ~910ms worst case), switch the busy-wait
clock to performance.now() (monotonic, avoids a backward NTP jump turning
the spin into an unbounded stall), and make the retry params injectable so
tests can exercise the real default retry count without paying the real
wall-clock cost.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kriszyp kriszyp requested review from DavidCockerill and heskew July 8, 2026 13:11
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kriszyp kriszyp marked this pull request as ready for review July 8, 2026 17:04
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.

1 participant