fix(config): read root config synchronously and bound rename retries by wall clock

[kriszyp]

The Windows EPERM: rename harper-config.yaml.tmp was never a budget that was too short: the worker was blocked against a descriptor its own root-config watcher held. libuv opens a fsPromises.readFile descriptor on the threadpool but closes it from JS, which cannot run while the same thread sits in atomicWriteFile's Atomics.wait — so no rename budget could have been long enough, and the extra 6.4 seconds an earlier head bought was pure event-loop stall on a path set_configuration reaches from a live request thread.

So the write side goes back to the 3,630 ms window it had before this branch, and the read side stops holding a handle across a yield: both root-config watchers read through readConfigFileSync, and a read that still loses is retried from a timer, which holds no descriptor either.

Review follow-up tightened two boundaries around that rule. An OptionsWatcher only blocks when it is explicitly the root watcher, so an application config remains asynchronous even when the component calls it harper-config.yaml; the production-shaped regression pins that distinction. And a terminal read fallback still discards an earlier staged value, while a watcher scan error preserves a successfully staged config because no later read superseded it; the mirrored boot tests cover both outcomes.

A later round closed the last path that could reach Scope without settling its barrier, and CI found the last test still encoding the contract that changed under it. #handleUnlink cancels the read ladder — the deletion is what a pending read was retrying — so a deletion landing while the first read was still on that ladder cancelled the only thing that would ever have emitted ready, and Scope, still inside await scope.ready, answered the remove by requesting a restart of a component that never booted. A boot-window deletion now settles on the defaults and reports remove only after ready, the ruling the ENOENT read path beside it already applies, and it claims the current read sequence so a rung already in flight on the asynchronous path cannot report the same deletion again behind it. On the test side should handle default config resolution still waited for a change where a scope that booted on its truthy default now takes the ready its arrival is; with timeout: 0 in .mocharc.json that wedged the whole unit job on all three Node versions rather than failing the one case.

main shipped an independent fix for this same root cause while the branch was open — #2339, merged as 99169ebca. Both make the root-config read synchronous; they disagree on the machinery, and landing both is not an option, so this branch is now rebased onto it and reconciled rather than merged alongside it. This branch's mechanism is kept because it is the superset — a shared per-thread blocking window, a wall-clock ladder instead of PartialReadRetry's attempt count, an arm gate over chokidar's unarmed window, credential-scrubbed parse errors, and a boot barrier every terminal read outcome settles — and PartialReadRetry / isPartialReadError / watcherFallback's additions go with it. What main had and this branch did not is ported rather than lost: a read that parses to nothing is a mid-write read like an empty one; atomicWriteFile reports the attempts and elapsed time behind a rename it could not complete; and configReadHandleLifetime.test.js keeps pinning the descriptor lifetime, against this branch's API. Which implementation wins is the first item below.

Making the read synchronous exposed three states the threadpool round-trip had been hiding, each of which ends with a worker holding a config nothing will ever correct:

• A write can land before the watch is armed. ready used to be emitted from inside chokidar's initial add; on darwin a write in that window is lost outright (0 ms lost, 5 ms delivered, as measured on this PR). ready is now gated on chokidar's own ready plus a darwin-only grace, and arming always re-reads rather than publishing what an earlier read staged.
• A non-atomic writer truncates before it writes, and the synchronous read is fast enough to land in that window — while chokidar throttles away the event carrying the content as a duplicate. An empty read now takes the same bounded ladder a locked one does, in both watchers. On Linux this was already reproducible: should instantiate and watch the root Harper config file failed 4 of 6 runs at this branch's earlier head and 0 of 6 at its merge base.
• A read can end with no config at all — the ladder spent, the file unparseable, no file to read. harper_logger.start() and Scope.ready await those barriers with no timeout, so each outcome now boots on defaults and logs what failed instead of hanging the worker. It settles carrying nothing rather than {}, because updateLogSettings reads an empty config as "rotation off, console off" — the one boot that could not read its config is the worst one to silently disable logging on.

A config that arrives after a scope has settled is the other half of settling early: nothing downstream re-runs on it, because componentLoader is long past its await scope.ready. Scope now answers a repeat ready the same way it answers remove — by requesting a restart — so a worker that read the file in a truncate window cannot keep serving the defaults while its siblings serve the operator's config.

Two smaller ones on the way through: a parse failure goes through parseConfigFile with logLevel: 'error', so neither yaml's framed error nor its process.emitWarning path can put config source — credentials included — into a log line; and a chokidar callback queued before close() is dropped on entry instead of reading a file a shutting-down worker no longer wants. DESIGN.md carries all of these invariants so neither watcher gets "modernized" back to fsPromises.readFile.

For the human reviewer

Framing-Verdict: chosen-approach-sound

1. This branch replaces main's shipped Fix intermittent set_configuration 500s on Windows caused by the config write's rename retry #2339 implementation rather than layering on it. The reconciliation removes PartialReadRetry, isPartialReadError and warnWatcherListenerError and returns utility/watcherFallback.ts to its pre-Fix intermittent set_configuration 500s on Windows caused by the config write's rename retry #2339 shape, because the two answer the same invariant with different machinery — an attempt count against a wall clock — and running both ladders over one read is worse than either. The narrower alternative is to keep Fix intermittent set_configuration 500s on Windows caused by the config write's rename retry #2339's read and land only the arm gate and boot-barrier work on top of it, which drops the shared blocking window (point 10) and the wall-clock budget the original review thread asked for. Everything Fix intermittent set_configuration 500s on Windows caused by the config write's rename retry #2339 had that this branch lacked is carried over commit-by-commit in 13af84b8; that commit is the one to read if you want to check nothing was dropped in the trade.
2. A root-config read that gives up is now reported per scope, not once per file. main's PartialReadRetry carried a module-level partialReadWarned set keyed by file path, so exactly one watcher's give-up was ever logged for harper-config.yaml; the reconciliation drops that set with the rest of that machinery, and each of the 10+ per-worker OptionsWatchers now warns under its own scope tag. Deliberate rather than dropped: on this branch a give-up is not only "the read failed", it is "this scope settled on the defaults", which is a per-scope consequence an operator needs named — but the cost is 10+ tagged lines per worker per incident where main emitted one, and a shared gate would silence 9 of them. Reinstating one is a module-level Map<path, boolean> cleared on the next successful read; the reviewer asked for it explicitly.
3. Root-config watchers now block the event loop to read (readFileSync, plus up to 500 ms of Atomics.wait on win32). The alternative is to make the writer stop blocking — an async rename retry, or moving set_configuration's write off the request thread — which leaves the 3.63 s synchronous stall this change preserves at the other end. That is the larger API change, and reversing this one later means re-touching both watchers and DESIGN.md.
4. ready is not once-per-watcher, and a repeat now costs a restart. OptionsWatcher emits it whenever a scope goes from having no config of its own to having one — the recreated-config-file path, and a scope that booted while the file was unreadable — and Scope converts a repeat into requestRestart(), on the same terms as its remove listener (a plugin with its own ready handler owns the response). The narrower alternative is to leave the arrival inert and accept per-worker config divergence until the next restart. Worth a look: #applyScopedConfig tracks whether the scope was configured separately from the value it holds, so a re-read of an unchanged falsy scope value (myPlugin: with no body) is not a transition, and the six scopes DEFAULT_CONFIG names do not read their own boot fallback as a block the file supplied.
5. The arming re-read deliberately does not report a deletion. Its job is the write that landed while the watch was unarmed; answering its ENOENT with remove reported the deletion ahead of the unlink that would confirm it, and on darwin ahead of chokidar finishing its own teardown — so a config recreated on the strength of that early remove landed where its add was not observed at all. It now goes back to the loop once and chokidar's own unlink cancels it, which is what fixed the reviewer's 3/3 macOS failure of does not write an applied config into the shared defaults. It is still a deferral rather than an observable signal: a platform where chokidar never emits unlink for a real deletion would see one extra read and no remove.
6. The read gates its retry to win32; the rename does not (same three codes, any platform). Deliberate: a misclassified read falls through to the timer ladder and still recovers, a rename has nothing to fall through to, and process.platform does not answer whether this filesystem can replace an open file (WSL drvfs, CIFS/SMB, Docker Desktop bind mounts all report linux). The cost is that a genuine Linux EACCES on rename spends the 3.63 s budget before surfacing. One-line flip either way. ENOENT is excluded from the read ladder entirely — a missing file is not a lock, and OptionsWatcher has always settled it at once as the install window.
7. A terminal failure in the boot window starts the scope on defaults, rather than failing the boot closed. It matches the ENOENT branch beside it, but a worker can now boot a plugin on defaults where it previously would have waited — and a Harper running on the wrong config is arguably worse than one that refuses to start. That policy call is the same in both watchers by design; changing it is a one-place change in each.
8. The read-retry ladder gives up permanently. After 500 ms of blocking plus a 3.1 s ladder the watcher warns and keeps the last valid config, and no further event is coming — the rename already fired. A lock outliving ~3.6 s therefore leaves that one worker on the old config until the next write or a restart, while its siblings apply the new one, and set_configuration still reports success. Strictly better than main's silent swallow; a slow re-arm or a periodic reconciliation would close it and is purely additive.
9. An empty file past the ladder is kept by a running worker and taken as defaults by a booting one. Both reviewers have now asked for a ruling on this asymmetry. An empty read that outlives the ladder is believed, and at that point #settleUnconfigured is a no-op on a watcher that has already emitted ready — so an operator who truncates harper-config.yaml at runtime gets a warning and keeps the config in force, while any worker that boots against that same empty file starts on the defaults. Keeping what is applied is the safer half (an empty file is far more often a truncate window than an intent); making the two halves agree means either believing the emptying at runtime, which hands a truncate window the power to reset every scope, or holding the boot open, which is the hang this branch exists to remove.
10. One 500 ms read deadline per path per thread, with a one-budget grace after it expires. A worker holds 10+ OptionsWatchers over the same root config, so a per-call budget would serialize into N × 500 ms of blocked event loop; sharing it means the first watcher spends the window and its siblings fail fast into the ladder. What remains per config event is N synchronous reads of one small file, which used to run on the threadpool. The grace is why a second lock arriving inside one budget of the first one's expiry blocks not at all and goes straight to the ladder — recoverable, but it is a real corner.
11. The 20 ms darwin arming grace is one measurement on one machine. 0 ms loses the write, 5 ms delivers it, and chokidar's own ready alone does not close the race (the reviewer tested it). A slower or loaded darwin host silently reopens the window. An observable arming signal would remove the timing assumption; chokidar does not expose one. Two things make it auditable from Linux CI rather than only from a mac: the value is a function of the platform, and ArmGate takes it as a constructor argument, so the timer branch itself is exercised on a host whose own grace is 0. Before that, forcing the grace to 0 unconditionally left the arming suite green.
12. OptionsWatcher shares the arming re-read, but not the arming barrier. This PR's switch to synchronous reads is what opened the unarmed window for the 10+ root-config OptionsWatchers componentLoader creates per worker (at the merge base they all read via fsPromises.readFile, whose threadpool round-trip deferred past it), so the gate is now shared rather than left to RootConfigWatcher. What OptionsWatcher takes is the re-read that recovers the otherwise-undeliverable write; its ready still goes out on the first read, so it means "the config has been read", not "armed". The ordering difference is safe because Scope attaches its listeners in its constructor, before any read — the recovered write arrives as a post-ready change rather than being lost. Holding ready behind arming too would need every terminal outcome to open a second barrier per scope, with a boot hang as the failure mode. Still declined and left for a follow-up: the lock ladder stays gated to root-config filenames, so an application config.yaml held by AV on Windows goes stale with only a log line. That one is genuinely pre-existing — an async read of a locked file failed the same way — and closing it means touching every component scope's readiness.
13. Three new test seams. OptionsWatcher gains _refreshForTests() and a read counter — they match the class's existing members and are what lets a test tell a ladder rung from a chokidar event (chokidar reports the unlocking chmod as a change of its own), but _refreshForTests() does expose a synchronous config re-read to any caller. harper_logger gains _applyLogSettingsForTests: both guards this branch added to updateLogSettings were unreachable from the suite, and the watcher that function builds resolves a process-wide path, so the only alternative was the rewire AGENTS.md forbids. The split is behaviour-preserving — the watcher wiring stays in updateLogSettings, the applying half moves down into applyLogSettings(rootConfigObject) — but it is a new export on a module almost everything imports. ArmGate's grace becomes a constructor argument for the same reason (point 11); every production site passes none.
14. atomicWriteFile now throws RangeError on non-finite/negative retry options instead of clamping. A new throw on the config-write path, whose only non-default callers are tests.
15. Two writer tests stub renameSync and writeFileSync (Sinon), which AGENTS.md tells new tests not to do, and a third now spends the whole 3.63 s default budget. Deliberate on both counts: neither stubbed case can be provoked from a real Linux filesystem — a POSIX rename over an open file succeeds, and a part-way ENOSPC write cannot be staged — and the default-budget case exists because every other case overrides retryBudgetMs, so widening the shipped default to effectively unbounded left the whole suite green. The reader-side tests take the no-stub route (a real mode-000 file). One watcher case also drives its own re-read through _refreshForTests() rather than the chokidar event, because recreating a watched file inside chokidar's unlink teardown is not reliably reported on any platform — see ## Verification.
16. A late exhaustion error from the generation that already failed is attributed to its replacement. Both watchers now guard the terminal "the replacement failed too" branch on "no reopen is coming" rather than #openCount > 1 (which left a watcher polling from construction with nothing to settle its barrier at all), but an ENOSPC still draining from generation 1 settles the barrier as a failure even though the polling watcher is healthy. The consequence is bounded: the settle is on the defaults, and the replacement's own arming re-read then publishes the real config as a change, so it costs an early settle rather than a blind scope. Distinguishing them properly means tagging errors with the generation that emitted them, in both watchers — worth doing, not worth doing here.
17. An intentionally empty config costs a 3.1 s boot. An empty read is believed only after the full ladder, so an operator who deliberately leaves harper-config.yaml empty delays harper_logger.start() and every root scope by the whole budget, once per boot. The alternative is distinguishing a truncate window from an empty file by size or mtime, which is a detection scheme rather than a constant — cheap to retune, not cheap to replace.
18. The atomic write neither preserves the file's mode nor fsyncs it. Both are pre-existing and both were declined here rather than folded into a branch this size. (a) writeFileSync creates the temp file at 0666 & ~umask and renameSync carries that inode's mode onto the destination, so an operator's chmod 600 harper-config.yaml is silently reverted to 0644 by the next set_configuration — below blocker under the one-process-per-container threat model, but the file holds authentication material. The fix is a statSync + chmodSync on the temp path before the rename, plus a decision about what to do when the destination does not exist yet. (b) No fsync on the temp descriptor or the parent directory, so on ext4 with delayed allocation a crash shortly after set_configuration can leave a present, zero-length config file — which the new empty-read handling downgrades from a hang to "boot on defaults with a warning", but the operator's configuration is silently not in force until someone rewrites it. An fsync on the set_configuration request thread is its own latency decision.
19. A deletion in the boot window is now a ready on the defaults, not a remove (#handleUnlink). This is the ruling the last round left open. remove before the first ready is not a coherent report in either direction: nothing consumes it — componentLoader is inside await scope.ready — and Scope answers it with requestRestart(), so the same deletion that cancelled the ladder also asks for a restart of a component that never started. Settling on the defaults matches the ENOENT branch beside it and every other terminal outcome on this branch. The alternative ruling — that a boot-window deletion means remove, and the barrier is settled some other way — is a one-line flip here plus a second settle path. What it costs as written: an operator who deletes the config inside the boot window gets a worker on defaults with a warning, where a running worker would have got a restart.
20. A deletion the asynchronous read observes first still reports twice, and that half is unchanged. A rung's readFile can reject ENOENT before chokidar delivers its unlink (held back ~100 ms by its own atomic-write window): the read settles boot ready on the defaults, and the unlink then finds #readyEmitted and emits remove, so one deletion produces ready + a restart request. That ordering predates this branch and is untouched by it — item 19 only changes what happens when the unlink wins. Making it idempotent properly means gating both remove sites on #scopeConfigured captured before the reset, rather than on #rootConfig's truthiness, which is the same unification #applyContents already applies to a scope removed from a file that still exists; it changes what a deletion means for a scope that was never configured, which is why it is not folded in here. Raised by the pre-push reviewer in both orders across two rounds.
21. The in-flight-read guard has no direct test. #handleUnlink claims the read sequence so an asynchronous rung still in flight is outranked instead of reporting the deletion a second time. Provoking exactly that order in a test means holding a readFile past chokidar's ~100 ms unlink delay, which needs libuv's threadpool saturated and pins the test to UV_THREADPOOL_SIZE; I judged that worse than the two lines it would pin. The guard is a strict narrowing — only a read issued before the deletion is suppressed, and every read issued after it still applies — and the synchronous half of the same handler is covered by the boot-window deletion regression.
22. EBUSY is retryable on every platform, and on Linux rename(2) returns it permanently for a mount point. A single-file bind mount (-v ./harper-config.yaml:/…/harper-config.yaml) is a documented Docker pattern, so set_configuration on such a deployment now blocks the calling worker for the full 3,630 ms budget before throwing the error it used to throw at once, and repeated calls compound. Pre-existing on this branch rather than new in this round, and the same one-line platform-gating question as item 6 — raised independently by the pre-push reviewer in both rounds.
23. main also shipped an independent, unrelated fix while this branch was open — #2364, the Windows lost-native-watch (EPERM, async) guard — and it touched the exact call sites this branch restructured. #2364 adds guardedWatch()/claimLostNativeWatchError() to utility/watcherFallback.ts and wires both into #openWatcher()/handleError() on OptionsWatcher and RootConfigWatcher — the same two methods this branch already rewrote for the generation-tracked reopen and the ConfigReadRetry/ArmGate machinery. Rebasing onto it was a merge, not a choice between implementations (unlike Fix intermittent set_configuration 500s on Windows caused by the config write's rename retry #2339 at item 1): guardedWatch() wraps the underlying chokidar.watch() call this branch already made, and claimLostNativeWatchError(error) is now the first line of both handleError methods, ahead of this branch's own exhaustion/closed-watcher handling. Nothing from either side was dropped; git diff against origin/main shows both features present together.

Verification

• npm run build, npm run lint, npx prettier --check — pass at this head (lint reports 13 warnings, 0 errors, all in integrationTests/, bin/ and unitTests/ files this branch does not touch; they reproduce on the merge base).

• npm run test:unit:main — 5243 passing, 195 pending, 2 failing at this head. Both failures are environmental and outside this diff: configValidator's domain-socket path-length warning, which trips on the long worktree path this ran in, and gitCredentials' grants the credential environment to the spawn that clones, which sees the GIT_CONFIG_GLOBAL/GIT_EDITOR this session's harness exports — it passes with those two unset. Neither file, nor anything either imports, is touched by this branch.

• npm run test:unit:logging — 141 passing, 14 pending, 0 failing, including two new cases that pin applyLogSettings against a bare myComponent: key and against a barrier that settles carrying no config. Both fail on a build with the guards removed.

• npm run test:unit:resources — 1 failing, unitTests/resources/vectorIndex.test.js's HNSW greedy routing assertion. Untouched by this diff, and the same assertion failed and then passed on re-run in this branch's own CI history.

• npx mocha "unitTests/components/**/*test*.js" — 1578 passing, 1 pending; npx mocha "unitTests/config/**/*.js" — 325 passing. Together these cover both watchers, Scope, componentLoader, configUtils, the arm gate and the env overlay.

• CI's unit job hung at a5d5b7f36 on all three Node versions, and that is what this round fixes. The wedge is should handle default config resolution, which waits for a change where a scope that booted on its truthy DEFAULT_CONFIG value now takes the ready its first source config is — the contract b0c788592 introduced for exactly that case. .mocharc.json sets timeout: 0, so the event that never comes hangs the run instead of failing the case, which is why the job died on a 10-minute step timeout with a green last line. Reproduced locally 3/3 at that head (npx mocha unitTests/components/OptionsWatcher.test.js never returns, and the same test alone hangs), and green 3/3 here. Earlier in the branch the same job wedged on does not write an applied config into the shared defaults, which recreated the config file from a microtask of chokidar's own unlink dispatch; that case now drives its post-recreate read through _refreshForTests() and carries its own timeout, and delivery of a recreate stays asserted by should continue to watch if file is removed and recreated.

• Every check is green at this head, and Unit Test (Node.js v22) now completes inside its cap with 55 s to spare — but the reason it was slow is not the one recorded above. The two cancels at 10m17s/10m18s were duration, not a hang; re-measured at 4929b4fa the job runs 9m05s against .github/workflows/unit-test.yml's job-level timeout-minutes: 10, which covers install, build and every test phase. Step timings across the matrix put the cost where it actually is:

  job	total	Run tests	test:types	install
  v22	9m05s	502 s	1 s	19 s
  v24	8m39s	469 s	—	25 s
  v26	8m28s	454 s	—	25 s

  So v22 is not the slowest because it alone runs npm run test:types — that step costs 1 second. It is 33 s slower than v24 in Run tests alone, on the same suite, which is Node 22's own runtime. main runs the same job in 7m01s–7m47s, and what this branch adds is about 45 s of deliberate wall clock: roughly fourteen unit tests let ConfigReadRetry's 3,100 ms budget expire in real time, plus configUtils' 3,631 ms shipped-default case and a 6,202 ms two-budget case. Measured locally at this head, unitTests/config/** takes 33 s and unitTests/components/** 52 s, of which ~47 s is budget expiry. The decision is unchanged and still yours — raise the cap (one line; the job is 92 % Run tests, so a hung run would burn the extra minutes), or make the read budget injectable so those tests stop spending it, which is another test seam of the kind this PR's review has already pushed back on, across fourteen cases. Not done here either way. Worth knowing that 55 s of headroom is inside runner variance: the same commit has now produced 8m11s, 9m05s and two 10m+ cuts.

• Integration Tests 5/6 (Bun) failed once at this head on Jobs - Delete Jobs_test schema (a 45 s drop_schema timeout that cascaded through the rest of the shard) and passed on re-run. Nothing in that path touches config reads.

• Each new regression test was checked against the un-fixed build: reverting the fix in dist/ makes it fail, so none of them pass vacuously. Worth knowing when reading earlier verification claims on this branch: #src/* resolves to ./dist/*.js unless NODE_OPTIONS=--conditions=typestrip, so a unit run without a preceding npm run build exercises the previous build, not the working tree.

• The boot-window deletion regression was checked the same way: with the settle reverted in the built #handleUnlink, settles ready when the file is deleted before any read had a config to give times out at 10 s rather than passing, so it pins the settle and not the ladder. It waits on the ladder's own read count rather than a wall clock, so a loaded runner moves the delete with it; three consecutive runs took 924 ms each.

• npm run test:integration -- integrationTests/apiTests/configuration.test.mjs — 25 passed on an earlier head of this branch; the config surface has not changed since. The full integration gate is CI's, and the Windows shard is the only end-to-end oracle for the platform this targets.

• The deadline test fails on origin/main as expected: the attempt-count loop it replaced takes ~3.63 s, past the test's upper bound.

• Linux unit runs do not exercise Windows sharing semantics. The retry cases fabricate a denied read with a mode-000 file, which is inert on Windows and ignored by root, so those cases skip on exactly the platform the fix targets. The Windows integration job — which reproduced the original EPERM exhaustion on an earlier head — is the only end-to-end oracle here.

• Rebase re-verification (this round, onto origin/main at 9a3c75013): npm run build clean; npm run test:unit:main — 5303 passing, 196 pending, 2 failing, same two environmental failures as above (configValidator domain-socket path length under this run's long worktree path, gitCredentials git-env-var leakage from this session's harness) and neither file touched by this diff; targeted config/component/logging suites green. The two conflicts from the rebase (OptionsWatcher.ts, RootConfigWatcher.ts — see item 23) were resolved by hand and covered by the existing watcher suites, not by a new test.

• Two of this PR's own CI checks are red, and both predate this branch — they already fail identically on origin/main's own tip. Format Check fails on unitTests/resources/query-array-scoping.test.js (from Pin element-scoping semantics of queries over array-valued properties #2437, untouched by this diff; npx prettier --check reproduces it against origin/main directly). Unit Test (Windows, Node.js v24) fails 7 cases in unitTests/utility/watcherFallback.test.js's "lost native watch guard" suite — the harness subprocess these tests spawn crashes with a native libuv assertion (Assertion failed: !_wcsnicmp(filename, dir, dirlen), file src\win\fs-event.c, line 72, exit 3221226505) instead of exercising the guard. git diff shows this file and its test are byte-identical between this branch and origin/main, and origin/main's own CI has been red on this same job since e966f2787 (the commit that merged Stop a deleted watched path from raising an uncaught EPERM (Windows) #2364) — run 33591393232 and the current tip's run 33592149944 both show it. Neither is this PR's regression to fix.

One CI datum worth a second look, because a Linux run cannot produce it. An earlier CI pass on this branch failed Integration Tests 2/6 (Windows) — describe-metadata-upgrade.test.ts, whose restart_service probe never became ready inside 120 s. It passed on the re-run and every other shard was green, so it is a flake by the usual test, but the mechanism is close enough to this change to name: the Harper log for the failed run carries JavaScript execution has taken too long and is not allowing proper event queue cycling on main/0 covering 17.5 s, starting the moment Restarting http_workers was logged. The same test passes on main (255 s vs the 383 s timeout here). 17.5 s is far past any single budget this branch sets (500 ms of read blocking, 3.63 s of rename retry), so if it is this change it would have to be a burst of config events each taking a fresh blocking window on the main thread rather than one long wait — which is point 10's shared-deadline behaviour under a rename storm. One green re-run does not settle that; the next Windows run that stalls should be read with this in mind.

One reviewer nit is partly declined. The in-source rationale for RENAME_RETRY_BUDGET_MS reads as narration against Harper's zero-new-comment default, but an earlier review thread on this PR asked for exactly that ("neither budget constant is explained in source any more"), so it stays and the mechanics live in DESIGN.md. The // Test-only: markers on the _*ForTests seams stay for the same reason — they were dropped once on this branch and restored on review — and each says what the seam is for (telling a ladder rung from a chokidar event; reading now rather than at whatever granularity an event arrives), which the accessor name does not. This round's pre-push review raised the same nit again on the rebase; the two accessors that added nothing beyond their own name (_usingPollingForTests, and half of _openCountForTests's comment) were trimmed in both watchers, and the ones defended above were left as they were.

Refs #2339

Complexity: complicated

_{Review-Coverage: authored=codex; ran=gemini,claude; blocked=domain(timeout); declined=cursor-grok,cursor-composer; rounds=8 @ 131a0c6}

_{Human-Review-Need: 4 @ 131a0c6}

[gemini-code-assist]

Code Review

This pull request increases the maximum retry attempts for renaming files (RENAME_RETRY_MAX_ATTEMPTS) from 12 to 25 in config/configUtils.ts to extend the retry budget to approximately 10 seconds. The corresponding unit test in unitTests/config/configUtils.test.js has been updated to expect 26 total attempts instead of 13. There are no review comments, and I have no additional feedback to provide.

[claude]

Reviewed; no blockers found.

[cb1kenobi]

Reviewed ca9c7d12 — no issues found. This PR looks good, nice job!

—
Generated by Barber AI

[kriszyp]

Rebased onto fb762a365 (force-push, fc15ab216 → b16103994). The branch now sits on a main that had rewritten the same two functions this PR does, so the conflict was resolved by hand rather than replayed.

What main landed in the meantime: #2245 (boot-path config persistence for #847) split atomicWriteFile into atomicWriteFile + renameWithRetry and added isStorageExhausted / persistConfigDuringBoot, a skipIfUnchanged write skip, an isMainThread persist guard, and the two-artifact env-config commit protocol in harperConfigEnvVars.ts.

How it composes with this PR: #2245's structure is kept whole; this PR's retry work is grafted into renameWithRetry's loop alone — the RENAME_RETRY_BUDGET_MS wall-clock deadline (attempt count demoted to a degenerate-option guard), isRetryableRenameError with EBUSY, and the option validation, lifted into validateRenameRetryOptions so it also covers renameWithRetry's one direct caller (the state-sidecar promotion) and still runs before the temp file is written. Nothing of #2245's exhaustion handling, write-skip, isMainThread guard or state commit is dropped.

DESIGN.md merged as intended: this branch's "Root config watchers must read synchronously" replaces the section it supersedes, and main's "Boot-path config persistence" and "Query-plan range estimation" sections are untouched. components/Scope.ts and utility/logging/harper_logger.ts auto-merged.

Because 17 of the branch's 47 commits touched config/configUtils.ts, the branch is re-expressed as two commits instead of replaying them. The non-configUtils.ts half of the PR diff is byte-identical to the pre-rebase diff (verified by diffing the two PR diffs), so nothing else was re-derived by hand.

Verified at this head: npm run build clean; test:unit:config 349 passing; test:unit:resources 1872 passing; test:unit:main 5235 passing with the one known environmental configValidator domain-socket-path failure (long worktree path, untouched by this diff); test:integration -- integrationTests/apiTests/configuration.test.mjs 35/35.

One finding from the pre-push review that is worth your call, not mine (it predates the rebase and I did not change behaviour to fix it): OptionsWatcher.#handleUnlink calls #readRetry.reset() and then takes the remove branch without settling the boot barrier. If the first read is empty/unparseable the ladder is armed with ready unemitted, and a deletion landing inside that window cancels the only thing that would have emitted ready — #resetConfig() leaves #rootConfig truthy, so the follow-up ENOENT read emits remove again and never ready, and sequentiallyHandleApplication's scope.ready.then(...) never runs. #handleReadError's ENOENT branch already guards exactly this ("remove … nothing consumes at boot → ready would hang forever"); #handleUnlink is the surface that did not get the same guard. The symmetric fix is if (this.#readyEmitted) this.#emitRemove(); else this.#emitReady(this.#scopedConfig); after #resetConfig() — but which of ready/remove a boot-window deletion should mean is the same contract question your decision ledger already has open, so I left it to you.

Also worth knowing: EBUSY being retryable on every platform means a Linux single-file bind mount (-v ./harper-config.yaml:/…/harper-config.yaml, a documented Docker pattern) now blocks the calling worker for the full 3,630 ms before throwing the error it used to throw immediately — on set_configuration that is a live request thread.

_{Rebase run by Claude Sonnet 5 via dev-agent dispatch. Pre-push review at this head: gemini + harper-domain, independent=true.}

[cb1kenobi]

Barbarian reviewed a5d5b7f and found no blocking issues. No new blocking findings were confirmed at this commit. The latest changes correctly distinguish fallback defaults from source configuration and trigger recovery.

—
Generated by Barber AI

[kriszyp]

CI's unit job was hanging at a5d5b7f36, on all three Node versions — not flaking, wedging. The last test to report was green, so the job died on the 10-minute step timeout with nothing red in it. The wedge is should handle default config resolution: it waits for a change where a scope that booted on its truthy DEFAULT_CONFIG value now takes the ready its first source config is, which is the contract b0c788592 introduced for exactly that case. .mocharc.json sets timeout: 0, so an event that never arrives hangs the run rather than failing the one case. Reproduced locally 3/3 at that head, green 3/3 here. The test now pins the arrival as a ready carrying the written value, with no accompanying change.

And the #handleUnlink gap raised above is closed rather than left open. It reads as a contract question, but remove before the first ready is not a coherent report in either direction: nothing consumes it — componentLoader is still inside await scope.ready — and Scope answers it with requestRestart(), so the same deletion that cancelled the ladder (via #readRetry.reset()) also asks for a restart of a component that never started, with nothing left to settle the barrier. A boot-window deletion now settles on the defaults and reports remove only after ready, which is the ruling the ENOENT branch beside it already applies and the one every other terminal outcome on this branch takes. Reverting that settle in the built #handleUnlink makes the new regression time out at 10 s, so it pins the settle and not the ladder. If you want the other ruling it is a one-line flip plus a second settle path — item 19 in the description says what it costs as written.

Two follow-ups from the pre-push review, both in the description rather than the diff:

• A deletion the asynchronous read observes first still reports twice (ready on defaults, then remove from the delayed unlink). That ordering predates this branch and is untouched by it; the guard added here covers the other order, where the unlink wins, by having it claim the current read sequence. Making both orders idempotent means gating the remove sites on #scopeConfigured rather than #rootConfig's truthiness, which changes what a deletion means for a scope that was never configured — item 20.
• The EBUSY-on-Linux bind-mount cost you flagged was raised independently by the reviewer in both rounds. Recorded as item 22, same one-line platform-gating question as item 6.

npm run test:unit:main at this head: 5243 passing, 195 pending, 2 failing — configValidator's domain-socket path-length warning (long worktree path) and gitCredentials seeing this harness's GIT_CONFIG_GLOBAL/GIT_EDITOR; both pass with the environment cleaned and neither file is touched by this branch.

_{Pre-push review at this head: codex + harper-domain, independent=true; gemini blocked on auth and both Cursor legs pruned by the branch round cap. Run by Claude Opus via dev-agent dispatch.}

[cb1kenobi]

Barbarian reviewed 4929b4f and found no blocking issues. No new blocking findings were confirmed at this commit. The boot-window deletion handling settles readiness and suppresses superseded in-flight reads.

—
Generated by Barber AI

[kriszyp]

CI at 4929b4fa: everything green except Unit Test (Node.js v22), which is now over its job timeout rather than hung. Worth separating from the wedge it replaced.

	main (33472193263)	this head
Node 22	7m47s pass	10m17s / 10m18s cut (re-run twice)
Node 24	7m12s pass	8m39s pass
Node 26	7m01s pass	8m28s pass

Both v22 attempts were cancelled at .github/workflows/unit-test.yml's timeout-minutes: 10, at a different point each time (bin/exitListeners, then the lmdb pass) and with tests still completing 0.3 s before the cancel — so it is duration, not a hang. v22 is the slowest of the three because it alone also runs npm run test:types.

What the branch adds is ≈45 s of deliberate wall clock: about fourteen unit tests let ConfigReadRetry's 3,100 ms budget expire in real time, plus configUtils' 3,631 ms shipped-default case and a 6,202 ms two-budget case. On main the job already sits at 7–8 minutes, so there was ~2 minutes of headroom for the whole repo and this branch spends most of it.

Two ways out, and I did neither because both are yours:

1. Raise the cap in unit-test.yml — one line, and it also buys back headroom main no longer has. The cost is that a genuinely hung run (.mocharc.json sets timeout: 0, so hangs are the failure mode we just saw twice) burns 15 minutes instead of 10 before reporting.
2. Make the read budget injectable so those tests stop spending it in real time. That is another test seam of the kind this PR's review has already pushed back on, and it touches ~14 cases.

Recorded in the description's ## Verification as well.

_{Run by Claude Opus via dev-agent dispatch.}

[cb1kenobi]

Barbarian reviewed 4929b4f and found no blocking issues. No new blocking findings were confirmed at this commit. Existing discussion already covers the known tradeoffs and CI-duration issue.

—
Generated by Barber AI

[kriszyp]

All 43 checks are green at 4929b4fa. The one red check was Unit Test (Node.js v22), and it was not this branch: it failed in the lmdb pass of unitTests/apiTests/mqtt-test.mjs on subscribe to retained record with patch operations, asserting messages[1].count was 2 where 3 was expected — the second message was a duplicate of the first, which is Flaky unit test: MQTT 'subscribe to retained record with patch operations' — duplicate retained/live delivery fails as uncaught assertion (~10% under contention) exactly. The same case passed in the rocksdb pass of the same job. Re-run: green.

The v22 duration note needs one correction, and it removes an option. Re-measured from the green run's step timings:

job	total	Run tests	test:types	install
v22	9m05s	502 s	1 s	19 s
v24	8m39s	469 s	—	25 s
v26	8m28s	454 s	—	25 s

v22 is not slower because it alone runs npm run test:types — that step costs one second. It is 33 s slower than v24 inside Run tests on the same suite, which is Node 22's own runtime, so moving or dropping test:types buys nothing. The cap is job-level (timeout-minutes: 10 on the matrix job in .github/workflows/unit-test.yml) and 92 % of the job is Run tests, so raising it costs the extra minutes only on a genuine hang. Headroom at this head is 55 s, and this same commit has now produced 8m11s, 9m05s and two 10m+ cuts — that is inside runner variance, not a margin. The choice between raising the cap and making the read budget injectable is unchanged and still yours; ## Verification now carries these numbers.

No unresolved review threads remain — all 21 are resolved, and the three automated rounds at this head report no blocking findings.

_{Run by Claude Opus via dev-agent dispatch. No commits: nothing needed a code change.}

[cb1kenobi]

Barbarian reviewed 4929b4f and found no blocking issues. No new blocking findings were confirmed on changed lines. Existing discussion already covers the known tradeoffs and failure modes.

—
Generated by Barber AI

[claude]

Suggestion (non-blocking): claiming #readSequence here so an in-flight asynchronous read is outranked instead of reporting the deletion twice has no test exercising that branch. The one test this fix touches ('settles ready when the file is deleted before any read had a config to give') constructs the watcher with isRootConfig: true, which forces synchronous reads (#synchronousRead = true) and never reaches this async path.

The PR description (item 21) already discloses this ("The in-flight-read guard has no direct test") with a reasonable justification — reliably provoking the race needs UV_THREADPOOL_SIZE tuning to hold a readFile past chokidar's unlink delay. Given the guard is a genuine behavior change (not just refactoring) in an area this PR is otherwise very thorough about regression-testing, a targeted async-mode test (construct without isRootConfig, induce an in-flight read, delete before it resolves, assert the stale read doesn't clobber the post-deletion reset) still seems worth the two lines it would pin — flagging for a second look rather than blocking on it.

[claude]

Suggestion (non-blocking): making EBUSY retryable unconditionally (previously only EPERM/EACCES were retried, and the read side still gates retries to win32) means a Linux single-file bind mount — a documented Docker pattern — now blocks the calling worker for the full RENAME_RETRY_BUDGET_MS (3,630ms) on every set_configuration call, where it used to fail fast, since rename(2) returns EBUSY permanently for that mount point rather than transiently.

This is explicitly disclosed in the PR description (items 6 and 22) as a deliberate, undecided trade-off — "the same one-line platform-gating question... left to you" — and a prior review thread on this line addressed the budget's magnitude but the platform-gating question itself was carried forward rather than resolved. Since it's a live-request-thread stall on a supported deployment shape rather than a hypothetical, surfacing it again here to make sure it gets the explicit human call the PR description is asking for before merge.