Fix intermittent set_configuration 500s on Windows caused by the config write's rename retry

[kriszyp]

set_configuration on Windows intermittently returned HTTP 500 with EPERM: operation not permitted, rename, after stalling the operation's thread for ~3.6 s. The root config is now read synchronously, so a config read can no longer hold the descriptor that blocks its own atomic write.

The retry loop was self-defeating. Windows rename() over an existing destination fails while any descriptor is open on it, and Harper's own root-config watchers are that holder: every thread runs a RootConfigWatcher and one OptionsWatcher per root-config component scope, each reacting to a change with fsPromises.readFile on harper-config.yaml, whose close() continuation runs on the owning thread's event loop. atomicWriteFile then retries renameSync with Atomics.wait on that same thread — so when the holder is an in-flight read there, its close() can never be scheduled while we wait. The holder's lifetime becomes exactly the retry budget, every attempt is guaranteed to fail, and the operation ends in a 500. That is why widening this budget in #1714 and again in #2036 changed nothing but the duration of the failure: the backoff sums to 3630 ms, and the failing request in the linked run took 3750 ms with the main thread logging nothing in between.

The premise was verified rather than assumed, on the Windows CI runner (Node v24.19.0), identically on two independently dispatched runs (1, 2):

case	result
control: rename over destination, no handles open	ok
destination held by a single Node fs.openSync(dest, 'r') descriptor	EPERM
same, immediately after closing that descriptor	ok
destination held by an in-flight fsPromises.readFile while the production retry loop blocks the thread	EPERM on all 13 attempts
same, immediately after awaiting that read	ok
source (the .tmp) held open by a descriptor	ok
destination watched by fs.watch (file), fs.watch (dir), or chokidar	ok
chokidar watching + in-flight read, loop blocking	EPERM on all 13 attempts

A Node read descriptor on the destination does block Windows rename, notwithstanding libuv opening files with FILE_SHARE_DELETE. The watch handle is innocent, which is why the failure is intermittent rather than permanent, and holding the source open does not block, so antivirus scanning the freshly written temp file is not the mechanism.

The invariant this enforces: no OS descriptor on the root config may outlive a single synchronous operation. RootConfigWatcher.handleChange and the root-config branch of OptionsWatcher now read with readFileSync, bounding the descriptor to one syscall. Application configs are written in place by fs.outputFile and never by rename-over, so they keep the non-blocking read and the #pendingReads drain that close() depends on; the parse/apply and error-routing logic is shared by both branches so they cannot diverge.

One consequence had to be fixed alongside it. A synchronous read can catch an in-place writer between its truncate and its write, and chokidar may emit nothing further for that write — so the watcher would adopt an empty read, or drop it, and then serve stale config indefinitely. Measured at 7 missed changes in 40 against 0 for the promise-based read it replaced. An unusable read is now re-read on a later turn, bounded so a genuinely empty or corrupt file cannot spin, and logged when the budget runs out; the read itself stays synchronous, so the descriptor still never outlives its turn. Harper's own writer is atomic and never produces this state — external tooling that rewrites the file in place does.

An unusable read is recognised by value, not by length: '', a lone newline and a truncated document all parse to null, and adopting that dropped the whole configuration — for a scope, via the remove that restarts it. That test runs on the file's own parse, deliberately before the env overlay: overlayRootEnvConfig turns any parse into a non-null object whenever a config env var is set (the norm in containers), so judging completeness after it would launder a half-written file into a valid-looking env-only config. A file that is still unusable once the retry budget is spent is taken at face value, so emptying a config file reaches remove exactly as it always did.

That recovery is the same in both watchers, which is a change in its own right. OptionsWatcher previously routed a read or parse failure straight to its error path, whose ENOENT arm answers with a remove that restarts the scope, while RootConfigWatcher retried the same failure on the same file for the same chokidar event. On Windows — the platform this targets — a transient read failure during a replace could leave one component serving pre-change options while the logger's watcher picked the change up: divergent config inside one process. Both now re-read first and fall through to the error path only once the budget is spent. A missing file is deliberately excluded: that is unambiguous, and its existing semantics (env-only fallback at boot, remove afterwards) are correct. Application configs get the same guard on their async path, since they are the ones actually rewritten in place. Applying the parsed config, by contrast, is past the point where an incomplete file could explain a failure — so a throwing change listener keeps each watcher's established error route rather than being replayed as a partial read.

Two smaller writer-side corrections ride along. The temp-file write moved inside atomicWriteFile's cleanup boundary, because a write that fails partway (ENOSPC, EIO) creates a partial temp holding configuration values that nothing then removed. And when a rename genuinely cannot be completed, the attempt count and elapsed time are logged — the two facts that distinguish a holder that never released from one that merely lost a race, and neither is recoverable from the rethrown error.

For the human reviewer

The synchronous read is not gated to Windows, and three of the four review lenses wanted it to be. The planning round returned Framing-Verdict: better-alternative-exists on exactly this point — gate on process.platform === 'win32' && #isRootConfig so non-Windows behavior is bit-for-bit unchanged. I did not adopt it, on a measurement the objection rests against. The concern is a blocking read-and-parse burst per config write; but the yaml.parse and overlayRootEnvConfig in that handler already ran synchronously on the event loop in the promise version — only the read moved. Measured on Linux against the shipped default config: readFileSync 2.2 µs, yaml.parse of the same content ~450 µs — so on Linux the change adds ~0.5% to a handler that already blocks for the other 99.5%, and it removes a 33 µs async round-trip per read.

Corrected after re-measuring on real Windows hardware (Node v24.14.0), because the Linux figure does not transfer. Against a real 1585-byte harper-config.yaml: readFileSync 169 µs in an ordinary directory, 1469 µs under Defender-scanned %TEMP%, fsPromises.readFile round trip 1928 µs, and yaml.parse 2518 µs. So on Windows the added blocking is roughly 7–58% of an already-blocking handler, not 0.5% — three orders of magnitude more than the Linux read once AV real-time scanning is in the path, and %TEMP% is exactly where CI and the reproducer live.

That does not change the decision, but it inverts the reasoning, so the honest form of the argument is: the platform where the synchronous read is expensive is the platform that needs it, and the platform that gains nothing pays almost nothing. Leaving it ungated costs Linux ~0.5% of a handler that already blocks, and buys Windows correctness where the cost is real but the alternative is a guaranteed 500. Against that, a Windows-only branch would have zero automated coverage: Harper's unit tests run only on ubuntu-latest, so the regression test proving the fix would never execute in CI. The adjudicating lens reached the same conclusion independently, downgrading it to minor and noting that "violates the non-Windows compatibility requirement" is not a repo invariant, and that most root scopes request a restart on a root-config change anyway. If you would still rather have the platform gate, it is a one-line change to the branch condition.

The mid-write temp cleanup has no unit test. Reaching it requires writeFileSync to fail after creating the file (a full disk), which is not reproducible without stubbing, and AGENTS.md forbids new sinon/rewire. The existing retry-exhaustion test covers the other failure path through the same finally.

One observable ordering change. For the root config, change/ready now fire synchronously inside the chokidar callback rather than on a later microtask — early enough that an in-process writer can see the event before its own await writeFile resolves. Production consumers (components/Scope.ts, utility/logging/harper_logger.ts) register listeners at construction and are unaffected, but it did surface in rootConfigWatcher.test.js, which subscribed after writing; that test now subscribes first.

OptionsWatcher._handleChangeForTests is a new test-only entry point, matching the two that already exist on that class. The read's timing relative to its caller is the behavior under test and a chokidar event cannot be observed at that granularity from outside.

The two watchers answer a genuinely-empty config differently, on purpose. Once the retry budget is spent, OptionsWatcher takes the file at face value (so > config.yaml still reaches remove), while RootConfigWatcher keeps the last usable config and warns. That preserves each one's prior contract — the old RootConfigWatcher ignored an empty read too — and it is not symmetric on purpose: an empty root config parses to null, and handing that to updateLogSettings would throw. A reviewer may reasonably want them unified; unifying downward is the unsafe direction.

PartialReadRetry distinguishes three outcomes, and the distinction is load-bearing. A usable read (settled()) withdraws the file's report and restores the budget; giving up (gaveUp()) restores the budget but leaves the report standing, because the file has not recovered and the report is shared with every other watcher of it; closing (cancel()) is terminal. Each of those was a separate review finding — a give-up that cleared the report let N scopes each warn about one bad file, and a give-up that kept a spent budget meant the write that repairs the file, which can itself be observed mid-write, had no re-read left to catch it. Every branch is pinned in unitTests/utility/partialReadRetry.test.js, and each of those tests was checked against the shape it replaces.

Accepted cost, flagged rather than fixed: one config write still drives N synchronous reads of the same file, one per root-config scope per thread — measured at roughly 1–3 ms of blocked loop for 10–40 scopes on Linux. On Windows the same fan-out is larger — at the 1469 µs Defender-scanned read above, 40 scopes add ~59 ms of blocking on top of the ~100 ms of yaml.parse main already blocks for. It is not one contiguous stall: each scope's chokidar event is its own turn, so this is 40 separate ~4 ms blocks rather than 40 separate ~2.5 ms blocks. The same N reads happened before, just without blocking, and config writes are administrative. Collapsing them to one per-thread reader that scopes subscribe to is the real fix and is out of scope here.

Pre-existing flake, not introduced here: OptionsWatcher.test.js → "with nested object values > should handle deleting" times out roughly 1 run in 6. Measured at 1/6 on unmodified main as well, and it exercises an application config, which this change does not touch.

This overlaps #2309 (the #2234 fix) in three files, and the two are complementary. #2309 canonicalizes the path handed to a native watch; this PR changes how the watched file is read. They touch the same lines only in the import list and the constructor of OptionsWatcher/RootConfigWatcher, plus a new DESIGN.md section each — all purely additive. Verified by merging the two heads locally: three conflicts, every one resolved by keeping both sides, then tsc --project tsconfig.json --noEmit clean, npm run build clean, and unitTests/config/**, unitTests/components/OptionsWatcher*, unitTests/utility/partialReadRetry.test.js, unitTests/utility/watchPath.test.js, unitTests/server/threads/watchDirFallback.test.js → 340 passing, both PRs' suites together. Semantically they stay on opposite sides of the seam: #2309's canonical path is used only for chokidar.watch, while reads, logging and the give-up report keep the original #filePath, and this PR's #handleChange re-reads that path itself rather than the event's argument. resolveWatchTarget's realpathSync.native is a single synchronous call at construction, so it does not violate this PR's descriptor-lifetime invariant, and #2309's mustPoll fallback stats rather than opening the file. #2309's repo-wide guard test (native watch sites are exactly the files that canonicalize their watch path) passes on the merged tree, so this PR adds no watch site that would escape it. Whichever lands second owns the (mechanical) conflict resolution.

Rebased onto main now that #2309 has merged, and re-verified on real Windows hardware. #2309 landed as d5039c900, so this branch's 16 commits now rebase directly onto current main instead of onto its pre-merge head — same three mechanical conflicts as before (two import lists, two constructors, one DESIGN.md section), all resolved keep-both, and the incremental diff over main is identical in shape to the standalone diff. On unmodified main (pre-#2309, pre-this-PR), integrationTests/apiTests/configuration.test.mjs failed 3 of 3 local runs (2–3 tests each, ~3.9 s per failure, every one EPERM ... rename harper-config.yaml.tmp); rebased onto current main it passes 35/35 locally in this pass, including back-to-back set_configuration calls all land. The probe table above reproduced row for row on that machine. All six Windows CI shards previously passed on the equivalent stacked state on the first attempt, while #2309 alone still failed shard 6/6 with this PR's EPERM signature — direct evidence that neither PR subsumes the other; CI will re-confirm on this head. Full detail in the verification comment below.

Two edge cases raised by this rebase's review, left as-is. RootConfigWatcher.handleChange adopts any typeof === 'object' parse, so a top-level YAML array root config would pass the completeness guard and get emitted as #config — main had no such guard at all before this PR (it accepted anything parse() returned, arrays included), so this is a pre-existing edge case this diff narrows rather than introduces or worsens; not fixed here. Separately, both watchers construct PartialReadRetry from their own raw filePath/#configFilePath rather than a normalized path — RootConfigWatcher always uses getConfigFilePath()'s absolute path and OptionsWatcher has exactly one production call site (components/Scope.ts:156) passing it a single consistent value per scope, so the theoretical divergent-spelling double-warning this could cause has no live call path today; flagging rather than adding normalization speculatively.

Verification

Route: extended integration test plus a new unit regression, both executed.

• unitTests/config/configReadHandleLifetime.test.js (new) pins the lifetime rather than the platform rule, so it is meaningful on every platform and needs no stubbing. RootConfigWatcher applies a change before handleChange returns fails on unmodified main (asserted the stale value) and passes here. Companions cover the preserved error contract — missing file and malformed YAML must not throw into the chokidar callback — and pin that an application config still reads without blocking.
• integrationTests/apiTests/configuration.test.mjs gains back-to-back set_configuration calls all land: add a component key (the largest watcher fan-out, since it creates a scope), then four immediate successive writes, then assert the last value persisted. That is the failing CI sequence, and it runs on the Windows shard where the OS rule applies.
• npx mocha "unitTests/config/**/*.js" "unitTests/components/OptionsWatcher*.js" "unitTests/utility/partialReadRetry.test.js" → 320 passing, run 6× for stability (an earlier revision of this branch flaked 1-in-8 here; that was the half-written-read regression above, and it is what led to finding it).
• npm run test:integration -- integrationTests/apiTests/configuration.test.mjs → 35/35 passing.
• npm run test:unit:main could not complete in this worktree: it aborts during module load in security/auth.ts against a stale local database (Cannot read properties of undefined (reading 'toJSON') opening database /home/kzyp/dev/tmp/hdb50node24final/database/system.mdb), before any test runs. Unrelated to this diff — CI runs the gate.
• Both OptionsWatcher recovery tests were re-checked with the completeness guard deleted and fail there, so neither is asserting on a read that had not happened. OptionsWatcher reports a file it gave up on once, not once per scope was likewise checked against the warn-before-settle ordering it replaces.
• OptionsWatcher does not let an env overlay launder a half-written root config was checked against the overlay-first ordering it replaces and fails there, so it pins the regression rather than just the current shape.
• unitTests/utility/partialReadRetry.test.js (new) pins the retry controller itself: one re-read per unusable read rather than per event, a usable read disarming an already-scheduled re-read, a bounded budget that reports its own exhaustion, and no re-read after close.
• The half-written-read regression was measured directly: 40 in-place rewrites observed by a live RootConfigWatcher, counting missed change events — 0/40 on the promise-based read, 7/40 on the first synchronous revision, 0/80 after the bounded re-read. RootConfigWatcher recovers a change it first observed as a half-written file pins it.
• The Windows shard confirms the fix. Integration Tests 6/6 (Windows, Node.js v24) on this PR: ✔ back-to-back set_configuration calls all land (142ms) and ✔ Configuration (21484ms) — including set_configuration still writes an operator-named component _package entry, the test that failed in the linked run. EPERM: operation not permitted, rename appears zero times across all six Windows shards.
• CI is otherwise green (41 checks). Integration Tests 2/6 (Windows) fails on Probe /SeoPageCache/ did not become ready within 120000ms … after restart_service, which is Windows CI shard 2/6 native abort() during component load with file watchers #2234 — already fixed by Canonicalize watch paths so a Windows 8.3 short path cannot abort the process #2309 — not this change. The probe reports ECONNREFUSED because the Harper process is gone: the shard-2 log artifact for the latest run (32932092722, artifact 9608015892) ends in a native libuv abort, Assertion failed: !_wcsnicmp(filename, dir, dirlen), file src\win\fs-event.c, line 72, raised while a component's file watchers start on an 8.3 short instance path. It carries no EPERM, and it reproduces byte-for-byte on main: run 32941944844 (sha df5355a) fails the same shard with the same two tests and the same probe message, and Windows shard 2/6 is the failing job in five of the last six failing main Integration Tests runs. It also failed again identically on re-run here. The Unit Test (Node.js v24) and two uWS HTTP failures in the first run were flakes — a 50 ms timing race in unitTests/resources/txn-tracking.test.js, a sub-second-TTL rate-limiter, and an LMDB TTL sweep, none of which touch config — and all three passed on re-run.

The probe that produced the Windows measurements above was removed before this PR; its results are recorded here and in #2313 so the next person to touch the retry budget does not have to re-derive them.

Refs #2313

Complexity: complicated

_{Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=15 @ 367322f}

_{Human-Review-Need: 4 @ 367322f}

[gemini-code-assist]

Code Review

This pull request addresses Windows-specific file locking issues (EPERM/EACCES) during configuration updates by ensuring that root configuration reads are performed synchronously, preventing file descriptors from outliving an event-loop turn. It introduces a 'PartialReadRetry' helper to safely retry transient, incomplete, or empty reads, and adds comprehensive unit and integration tests to validate these lifetime guarantees and recovery behaviors. I have no feedback to provide as there are no review comments to assess.

[claude]

Reviewed; no blockers found.

[kriszyp]

Windows verification on real hardware — reproduced on main, fixed here

This PR is now stacked on #2309 (base retargeted to fix/windows-short-path-watch-abort, its 16 commits rebased on top), so the two are tested together as the description recommended. Three conflicts, all mechanical (two import lists, two constructors, one DESIGN.md section), every one resolved keep-both. The incremental diff over the #2309 layer is identical in shape to this PR's standalone diff — same 9 files, same ±line counts — so nothing was lost or duplicated in the rebase.

Everything below was run on a real Windows 11 machine (Node v24.14.0) whose os.tmpdir() is C:\Users\KRISZY~1\AppData\Local\Temp — the same 8.3 shape as the runner.

The failure is near-deterministic locally, not 1-in-N

This turned out to be a better reproducer than CI. On unmodified main, integrationTests/apiTests/configuration.test.mjs:

	main	stacked (#2309 + this PR)
run 1	32 pass / 2 fail	35 pass / 0 fail
run 2	31 pass / 3 fail	35 pass / 0 fail
run 3	31 pass / 3 fail	35 pass / 0 fail
run 4	—	35 pass / 0 fail
back-to-back set_configuration calls all land	n/a	245–568 ms

Every failure was EPERM: operation not permitted, rename ...harper-config.yaml.<pid>.<tid>.<hex>.tmp -> ...harper-config.yaml from server/operationsServer.ts:345, each consuming ~3.9 s — the full 3630 ms retry budget plus overhead, exactly as the description predicts for a holder pinned on the blocked thread.

The measurement table reproduces row for row

I re-derived the probe independently rather than taking it on trust. Same results on this machine as on the runner:

case	result
control: rename over destination, no handles open	ok
destination held by a single fs.openSync(dest, 'r') descriptor	EPERM on all 13 attempts
same, immediately after closing that descriptor	ok
source (the .tmp) held open by a descriptor	ok
destination watched by fs.watch (file)	ok
destination watched by fs.watch (dir)	ok
in-flight fsPromises.readFile while the retry loop blocks the thread	EPERM on all 13 attempts
same, immediately after awaiting that read	ok
chokidar watching + in-flight read, loop blocking	EPERM on all 13 attempts

The two conclusions the fix rests on both hold: a Node read descriptor on the destination does block Windows rename-over, and the watch handle is innocent — which is what makes the failure intermittent in CI rather than permanent.

CI proof that both PRs are necessary

The #2309 push produced a clean natural experiment in one run. On #2309 alone, attempt 1:

• Integration Tests 2/6 (Windows) — the libuv 8.3 shard — passed
• Integration Tests 6/6 (Windows) — failed on set_configuration still writes an operator-named component _package entry with the same EPERM ... rename harper-config.yaml.tmp signature

On this stacked branch, all six Windows shards pass on the first attempt:

shard	result
Integration Tests 1/6 (Windows)	pass 3m29s
Integration Tests 2/6 (Windows)	pass 10m07s
Integration Tests 3/6 (Windows)	pass 7m33s
Integration Tests 4/6 (Windows)	pass 4m03s
Integration Tests 5/6 (Windows)	pass 4m58s
Integration Tests 6/6 (Windows)	pass 4m58s

So neither PR subsumes the other, and the stack is the configuration that turns Windows green.

Local unit results on Windows

• unitTests/config/configReadHandleLifetime.test.js — 12 passing (this PR's key regression test, on the platform the OS rule applies to)
• unitTests/utility/partialReadRetry.test.js — 9 passing
• unitTests/config/rootConfigWatcher.test.js — 7 passing
• unitTests/components/OptionsWatcher.test.js — 28 passing
• Clean npm run build, npm run typecheck, npm run lint:required, prettier --check

I did not see the pre-existing OptionsWatcher.test.js → "with nested object values > should handle deleting" flake the description reports at ~1-in-6; that suite was 28/28 on every run here.

The performance argument in the description does not hold up — correction proposed

The "For the human reviewer" section declines the Windows gate partly on this measurement: readFileSync 2.2 µs vs yaml.parse ~450 µs, so "the change adds ~0.5% to a handler that already blocks for the other 99.5%".

Measured on Windows against a real 1585-byte harper-config.yaml:

operation	cost
readFileSync — repo directory	169 µs
readFileSync — Defender-scanned %TEMP%	1469 µs
fsPromises.readFile round trip	1928 µs
yaml.parse (already synchronous on main)	2518 µs

So on Windows the added blocking is roughly 7–58% of an already-blocking handler, not 0.5% — the sync read costs three orders of magnitude more than the quoted figure once AV real-time scanning is in the path, and %TEMP% is exactly where CI and the reproducer live.

The conclusion survives; the justification does not. The right argument is the inverse of the one given: the platform where the sync read is expensive is the platform that needs it, and on Linux a page-cache-warm 1.5 KB readFileSync genuinely is microseconds — so leaving it ungated costs Linux almost nothing while buying Windows correctness. I have updated that paragraph in the description to say this, and folded in the 40-scope figure with the honest Windows numbers. Flagging it because a reviewer who re-measures on Windows would otherwise trip on it, and because "confirmed by measurement" carries weight it had not earned on this platform.

Notes on the rest

• The ungated sync read is the right call, for the reason above. Worth adding that the "a Windows-only branch would have zero automated coverage" argument is contingent: it holds only while .github/workflows/unit-test.yml pins ubuntu-latest. Windows unit-test coverage is filed separately.
• PartialReadRetry on the async application-config path is the one piece of this diff that changes non-Windows behaviour without being required by the Windows fix — an emptied app config now spends up to 10 × 20 ms of re-reads before reaching remove. Low risk and defensible on consistency grounds (the description's argument about divergent config inside one process is sound), but it is the honest answer to "what could regress Linux", so it deserves to be named rather than folded into the Windows rationale.
• The atomicWriteFile restructure is a real fix beyond the EPERM issue: moving the temp write inside the cleanup boundary means a write that fails partway (ENOSPC, EIO) no longer leaves a partial temp file holding configuration values. Cross-platform improvement.

🤖 Verification performed with Claude Code on Windows 11 / Node v24.14.0

The base branch was changed.