Stop a deleted watched path from raising an uncaught EPERM (Windows)

[kriszyp]

The bug

Windows-only. Deleting or replacing a watched path raises an uncaught EPERM: operation not permitted, watch (errno -4048, syscall: 'watch') from node:internal/fs/watchers. Distinct from the 8.3 short-path abort (#2309) and the config-write EPERM rename (#2339), both of which have since landed and are in this branch's base.

Every Harper chokidar watcher runs with persistent: false so it never holds the event loop open. That option takes chokidar down the one branch of setFsWatchListener that never attaches an 'error' listener to the underlying Node fs.FSWatcher:

if (!options.persistent) {
    watcher = createFsWatchInstance(path, options, listener, errHandler, rawEmitter);
    if (!watcher) return;
    return watcher.close.bind(watcher);      // <-- no watcher.on('error', ...)
}

errHandler there is only consulted for a synchronous throw out of fs.watch() — which is how ENOSPC/EMFILE already reach the polling fallback added in #488. An asynchronous watch failure is delivered as emit('error', err) on an emitter with no listener, so Node turns it into an uncaughtException. It never reaches chokidar's wrapper, so .on('error') on the FSWatcher we hold cannot see it. chokidar's persistent: true branch does attach a listener and swallows this exact error (its node#4337 workaround), which is why only our watchers hit it.

Live in CI: Integration Tests 4/6 (Windows, Node.js v24) on main logs repeated uncaughtException Error: EPERM: operation not permitted, watch during Redeploy runtime-equivalence proof, which deletes and replaces component directories.

Why not the obvious fixes

• Upgrade chokidar — 5.0.0 has the identical bug. Verified by running the repro against a 5.0.0 install: same uncaught EPERM, exit 9; the same script with persistent: true prints SURVIVED. No upstream issue covers it (Flaky: Unit Test (Node.js v24) SIGSEGV (exit 139) in schema-change / index-rebuild test group — v24-only #1380 is a different EPERM: syscall: 'rename' from the caller's own renameSync). The upstream fix is one line, watcher.on('error', errHandler) in the non-persistent branch.
• Patch chokidar locally — no postinstall exists today, and patch-package patches would not apply for anyone installing harper from npm, so production Windows users would still crash.
• Wrap fs.watch — appears to work (chokidar's ESM facade for node:fs is snapshotted lazily at first ESM import, so a CJS-first patch does reach it), but any earlier import … from 'node:fs' in the graph silently defeats it and we have dozens. Verified both the working and the defeated ordering.
• persistent: true — deliberately not touched; the option exists so watchers don't hold the event loop open, and chokidar exposes no way to unref the handle.
• Route it to a specific watcher — impossible. The error carries filename: null and no path, so it cannot be attributed to the watcher that died.

The fix

guardedWatch() in utility/watcherFallback.ts: chokidar.watch() plus an idempotent, prepended process-level listener that supplies the missing handler at the only place the error is observable. All five chokidar call sites route through it — EntryHandler, OptionsWatcher, RootConfigWatcher, security/keys.ts, manageThreads.js — so no call site can forget.

Three conditions bound what it claims, and all are load-bearing, because whatever it claims is swallowed process-wide:

• syscall === 'watch' — only ever set by fs watch handles.
• code === 'EPERM' only. Not ENOENT: an async ENOENT from a watch handle has never been observed, whereas a synchronous one is the ordinary "watch a path that isn't there" misconfiguration.
• no path. This separates the async failure from a synchronous fs.watch() throw — Node populates path on the thrown error but leaves it absent on the one delivered to the handle's 'error' event. Without it, a raw fs.watch(missingPath) escaping into an uncaughtException would be mistaken for a benign lost watch and silently swallowed.

It also does not become a blanket crash suppressor: for anything it does not claim, if the guard is the only uncaughtException listener it removes itself, clears its installed flag, and re-raises on nextTick, preserving Node's report and exit code 1 (a plain rethrow from inside the handler gives exit 7).

The claim is the classification; the bookkeeping is best-effort. Counting, logging and the isHandled mark all run inside a try, in that order, and cannot un-claim an error the shape check already accepted — the same function is called directly by the five watchers' own 'error' routes, outside the guard. A frozen error or a logger that threw would otherwise turn a failure just classified as benign into a fatal one, and this listener runs first, so its throw would take Node's report (exit 7) and the thread-level handlers' turn with it. Only classification can still throw, and an error whose shape cannot even be read is not the guard's to claim: it stays fatal.

Claimed errors are marked isHandled, so threadServer.js stops logging them as worker-level uncaughtExceptions. socketRouter.ts was missing that same check — added, matching the contract threadServer already documents. Prepending is the mechanism: Node calls every listener regardless of order, so the mark only suppresses those handlers if the guard runs first.

Logging never goes fully silent. The default level is warn, so a warn on the first occurrence and trace thereafter would hide every subsequent one. It warns on the first and at each tenfold increase, traces every one. The message states only what is known — that the failure cannot be attributed to a specific watcher — and names the Windows deleted-directory case as the usual, not certain, cause.

DESIGN.md's watch-site section records the wrapper's relationship to the canonicalization invariant: guardedWatch arms the watch but resolves no path of its own, so every caller still canonicalizes before calling it.

No polling fallback for this case, deliberately. Polling a path that no longer exists just burns CPU. The recovery case that would have justified it does not need it: an atomic rename-replace of a watched config file does not kill the watcher (add, change, then a live change on a subsequent in-place write). The trigger is specifically removal of the watched directory, where the component is being torn down and the watcher recreated anyway.

For the human reviewer

The pre-push rounds ran without the domain adjudicator (auto policy pruned it on a low-risk delta), so this is the ledger written from the standing findings rather than a generated one.

1. The recovery contract — swallow and warn, with no re-arm. Every review round raises this as its one open major: a claimed error means that watcher is gone, and nothing re-arms it or falls back to polling, so a long-lived config or component watcher whose directory was replaced stays silent until something re-establishes it. The alternative — re-arm the affected watcher — is not implementable at this layer: Node gives this error no path and filename: null, so it cannot be attributed to a watcher, and the trigger is the watched directory being removed, where polling would burn CPU on a path that no longer exists. What makes it liveable is that the case where it would matter (an atomic rename-replace of a watched file) does not kill the watcher at all, and the warn line names the symptom. A "no" here means per-call-site re-arming — each site already owns a reopen chain for exhaustion, so it is a contained follow-up, not a redesign of this PR.
2. Where the guard's boundary sits. Classification decides; the mark and the log lines are bookkeeping that cannot un-claim the error. The alternative is fail-closed: treat any failure — a frozen error, a logger that throws — as "not ours" and let it stay fatal. Chosen against, because the shape check is the decision and losing a log line does not make the failure less benign, and because the five watchers' 'error' routes call the same function with no guard around them. Cost of a "no": a frozen lost-watch-shaped error takes the thread down. One commit either way.
3. socketRouter.ts reads isHandled off a possibly-primitive error. Two lenses flag it each round. Left as a plain property read to match the pre-existing .code read on the next line: throw null crashed that handler before this PR too, so ?. on one line moves the crash rather than removing it. Making that handler null-safe is a separate change with its own semantics to decide.
4. The chokidar.default.watch test seam. One lens calls this a blocker every round, arguing the CJS mock cannot intercept guardedWatch. It demonstrably does: adopting the suggested named watch import made both watchDirFallback cases see zero watcher opens, which is why 7a8acb2 reverted it, and the suites pass as they stand. Ruling on it here would stop it being re-litigated per round.
5. Comment density. Trimmed in the last commit against the house default; what remains is the chokidar mechanism, the three load-bearing conditions, and the two ordering constraints. Lenses still call some of it narration — say the word and it comes down further.

Verification (Windows 11, Node v24.14.0, chokidar 4.0.3)

Every shape Harper actually watches crashed before and survives after:

shape	before	after
watch('.', {cwd, persistent:false, followSymlinks:false, ignored}) — EntryHandler	uncaught EPERM	survives
watch(file, {persistent:false}) — OptionsWatcher / RootConfigWatcher / keys.ts	uncaught EPERM	survives
watch(dir, {persistent:false, ignored}) — manageThreads	uncaught EPERM	survives

Tests

unitTests/utility/watcherFallback.test.js — 23 passing, 1 pending. Predicate cases pin the shapes it must not claim: a synchronous throw (carries path), ENOENT, EPERM with syscall: 'rename', and the exhaustion codes that belong to the polling route. A unit case covers the watchers' direct 'error' route: an error the claim cannot mark is claimed anyway rather than thrown back at the caller.

Seven child-process cases via fixtures/lostWatchHarness.cjs. Child process is required: mocha's own uncaughtException listener would absorb the crash and the test would pass either way.

• a deleted watched directory does not kill the process
• an unrelated uncaught exception is still fatal (exit 1, original message)
• a synchronous fs.watch failure on a missing path is still fatal
• a threadServer-style listener registered before the first watcher observes isHandled=true — appending would leave it false, so this is what pins the ordering (win32 only; it is the one pending case elsewhere)
• twelve claims produce exactly two warnings, at occurrences 1 and 10
• a frozen lost-watch error is still claimed and counted — the process lives, where an unprotected guard would exit 7 with its own TypeError
• an error the guard cannot classify stays fatal — a throwing path getter, re-raised as itself rather than replaced by the guard's throw

The delete case asserts survival on every platform and lostWatchCount > 0 on win32, so it still runs as a smoke test on the ubuntu-only unit-test CI; the two cases above raise their own errors and so assert real behavior on every platform. The harness's fixed 1.5s waits are now a 5s ceiling on win32 with early exit once delivery is observed, rather than a sleep that could race a loaded runner.

Linux (Node v26.2.0), this branch: watcherFallback + watchPath + watchDirFallback + security/keys — 100 passing, 1 pending, 0 failing. The Windows table above was measured at the first commit; the three commits since change only the guard's exception path and its tests, and touch no watch-arming behavior.

Cross-model review

Reviewed by Codex (with repository read access) and Gemini. Codex found no blocker or significant issue and verified each claim against the code. Acted on, in the commits after the first:

• the claimed set was too wide — ENOENT dropped, absence of path required
• repeat occurrences were invisible at the default log level
• the guard's installed flag was not cleared when it steps aside, so a process that somehow survived would have run unguarded thereafter
• _resetForTests left a process listener attached to the test runner
• claimLostNativeWatchError could count one error instance twice
• prepend ordering and the warn cadence had no test coverage

Rejected with evidence: that require('../../utility/watcherFallback.ts') throws MODULE_NOT_FOUND (that file already has eight such requires and the build rewrites them to .js), and that restricting the exemption to registered guarded roots would work (not implementable — the error carries no path).

One reviewer suggestion was adopted and then reverted: switching to a named watch import binds past the seam unitTests/server/threads/watchDirFallback.test.js uses to stub chokidar.default.watch. The property access is load-bearing and now says so.

Four further delta rounds (codex graded + Gemini) covered the commits since. Fixed in response:

• the guard could throw from inside its own uncaughtException listener while classifying or marking an error — exit 7 with a TypeError in place of the real report, and the thread-level handlers never reached
• with the mark and the log inside one boundary, a logger that threw after the mark landed would have left the error unclaimed here but skipped as handled everywhere else, so nothing would have reported it
• the same hazard reached the watchers' direct 'error' routes, which call the claim outside the guard
• DESIGN.md still described the guard as covering EPERM/ENOENT after ENOENT was dropped from the claimed set
• comment volume against the house default

What each round left open is in For the human reviewer above.

Not in scope

• EntryHandler emits Windows backslash-separated entry paths — the first of the two pre-existing EntryHandler.test.js failures. Filed separately; the second is a cascade of the first, since the aborted test never reaches its close() before the fixture directory is removed.

🤖 Generated with Claude Code

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

_{Human-Review-Need: 4 @ edbacf0}

[gemini-code-assist]

Code Review

This pull request introduces a process-level guard (guardedWatch) to safely handle benign, asynchronous "lost native watch" errors (such as EPERM on Windows when a watched directory is deleted) that occur when using Chokidar with persistent: false. It updates various watchers across the codebase to use this guarded wrapper and adds comprehensive unit tests. The review feedback is highly constructive, pointing out two important edge cases in utility/watcherFallback.ts: resetting the guard state when the listener is removed on an unrelated exception to prevent subsequent watchers from being left unguarded, and cleaning up the global listener in _resetForTests() to ensure proper test isolation.

[kriszyp]

Rebased onto latest origin/main (was 7 commits behind, including #2309 which added Windows-path canonicalization + ENOSPC/EMFILE retry directly around the same 5 chokidar.watch(...) call sites this PR touches). Conflicts resolved by composing both fixes: kept #2309's canonicalization/exhaustion-retry structure verbatim in each file and swapped only the raw chokidar.watch(...)/watch(...) call for guardedWatch(...). git diff origin/main...HEAD is now the same 13-file, ~450-line diff as before the conflict, just replayed on top.

One real fix needed beyond textual merging: unitTests/utility/watchPath.test.js's NATIVE_WATCH_SITES source-scan tripwire blind-spotted security/keys.ts/manageThreads.js once they stopped textually referencing chokidar (routed through guardedWatch instead), while newly (and correctly) flagging utility/watcherFallback.ts itself. Updated the scan and added a documented CANONICALIZES_VIA_CALLER exemption for the wrapper — see DESIGN.md's canonicalization section for the reasoning. Also fixed 2 sibling tests (keys.test.js, watchDirFallback.test.js) whose chokidar.watch stubs needed to become chokidar.default.watch to match how guardedWatch's default import resolves — same pattern already used correctly in OptionsWatcher.test.js/rootConfigWatcher.test.js.

Also closed two small gaps the independent review caught: security/keys.ts and manageThreads.js's watcher error handlers were missing the claimLostNativeWatchError() early-return the other three sites have (defense-in-depth for the day chokidar's polling/persistent branches do deliver this error shape to the wrapper), and merged a duplicate require('.../watcherFallback.ts') in each of those two files.

Build, test:unit:main (5089/5097 passing — the 8 failures are pre-existing/environmental, unrelated modules), and test:unit:resources (1767/1767) all pass. Ran 3 full independent-review rounds (codex + gemini + cursor-grok + Harper-domain adjudication) across the rebase; Human-Review-Need: 4 throughout, converging on the same decision ledger each time:

• swallow-vs-rearm — a claimed lost watch is logged and abandoned rather than closed+reopened (the way the adjacent ENOSPC/EMFILE path already does). For RootConfigWatcher/OptionsWatcher/WATCH_DIR, that's permanent silent loss of hot-reload until restart if the watched path is deleted/replaced while the process stays up — worth confirming this matches the intended contract (the PR body's "no polling fallback, deliberately" note covers the polling question but not re-arming a native watch).
• claim-scope — LOST_NATIVE_WATCH_CODES (EPERM+ENOENT, syscall==='watch') also matches a synchronous arm-time permission failure (chokidar delivers that to 'error' too), which the guard would then swallow as if it were a benign deletion — a real watcher-never-armed case reported as "expected on Windows during redeploys."
• process-wide guard scope — installLostNativeWatchGuard() claims this error shape process-wide once any Harper watcher opens, including watches an in-process component/app owns itself.
• CI coverage — the Windows-specific assertion in the new child-process harness never executes on CI (unit tests are ubuntu-only); the harness's fixed 1.5s sleep-then-assert also matches the flakiness shape AGENTS.md flags (Replace fixed-delay 'sleep-then-assert' timing races in unit tests (umbrella) #1138) as needing waitFor.

None of these are rebase artifacts — all three rounds confirm they're pre-existing in the original diff. Flagging per the decision ledger rather than acting on them, since they're your call as the PR's designer. Full review artifacts available if useful; happy to implement whichever direction you pick.

— Claude Sonnet 5 (dev-agent rebase)

[claude]

1. Lost native watch errors are claimed but never rearmed for process-lifetime watchers

File: utility/watcherFallback.ts:147 (also config/RootConfigWatcher.ts:72, server/threads/manageThreads.js:1521)

What: claimLostNativeWatchError() marks the error handled, logs it once, and returns — it never closes and reopens the watch it just claimed. The sibling exhaustion path in the same handlers does the opposite: RootConfigWatcher.handleError (config/RootConfigWatcher.ts:73-91) and manageThreads.js's watchDir error handler (server/threads/manageThreads.js:1522-1534) both explicitly close() then reopen the watcher on ENOSPC/EMFILE, but neither does anything after if (claimLostNativeWatchError(error)) return; claims the lost-watch case.

Why it matters: RootConfigWatcher is a process-wide singleton created once at boot (utility/logging/harper_logger.ts:148, if (!rootConfig) rootConfig = new RootConfigWatcher()) and never recreated, and manageThreads.js's WATCH_DIR watcher is opened once at module load (server/threads/manageThreads.js:1551) with no equivalent lifecycle hook. Per this PR's own verification table, deleting/replacing the watched file itself (not just a directory) reproduces this exact error for both. Before this PR that crashed the process — loud, but the watcher came back on restart. After this PR the process survives, but that watch is gone for good: Harper's root config (or, for WATCH_DIR, component-reload detection) silently stops hot-reloading, recoverable only by a full process restart, and the only diagnostic is one anonymous warn-level log that doesn't say which path was lost. security/keys.ts is largely covered by its independent periodic re-read safety net (tls.certificateWatchInterval, default 5m) and EntryHandler/OptionsWatcher instances are torn down and recreated on component redeploy, but RootConfigWatcher and WATCH_DIR have neither mitigation. None of the per-class watcher suites (rootConfigWatcher.test.js, watchDirFallback.test.js) exercise claimLostNativeWatchError, so this gap isn't covered either way. This matches the "swallow-vs-rearm" item flagged in the PR's own rebase-conversation decision ledger as still open.

Suggested fix: On a claimed lost-watch error, close the dead watcher and call openWatcher() again — mirroring the exhaustion path already used for ENOSPC/EMFILE in the same handlers — instead of only logging and returning.

[claude]

One blocker carries over, still unaddressed: claimLostNativeWatchError() never closes/reopens the watch it claims, so RootConfigWatcher/manageThreads.js's WATCH_DIR permanently and silently lose hot-reload after a Windows delete/replace (see inline thread). This push's commits (frozen-error handling, classification/bookkeeping boundary, comment cleanup) don't touch it. The two Gemini-flagged guard-reset issues are now fixed.

[claude]

Still open from the prior review (thread on this same line): claimLostNativeWatchError() marks the error handled and returns without closing/reopening the watch, while the sibling ENOSPC/EMFILE path a few lines below in RootConfigWatcher.handleError (config/RootConfigWatcher.ts:72) and manageThreads.js's watchDir handler (server/threads/manageThreads.js:1521) both explicitly reopen. This push's three commits fixed a related-but-different gap (the guard's own lostNativeWatchGuardInstalled flag not resetting on self-removal) — they don't touch this. RootConfigWatcher is a boot-time singleton never recreated, and WATCH_DIR in manageThreads.js is opened once at module load with no other lifecycle hook, so a Windows delete/replace of the watched file still permanently and silently disables config hot-reload / component-reload detection, recoverable only by a full restart. Suggested fix unchanged: on a claimed lost-watch error, close the dead watcher and call openWatcher() again, mirroring the exhaustion path in the same handler.

[claude]

Still open (see the two prior threads on this line): claimLostNativeWatchError() still never closes/reopens the watch it claims. This push's commits (frozen-error handling, classification/bookkeeping boundary, comment trim) don't touch that gap — RootConfigWatcher (config/RootConfigWatcher.ts:72) and manageThreads.js's WATCH_DIR (server/threads/manageThreads.js:1521) still permanently lose hot-reload after a Windows delete/replace, recoverable only by a full restart. Not re-litigating the mechanism again here — flagging so it stays visible since the PR author's own decision ledger (top-level PR comment) named this "your call as the PR's designer" rather than resolving it.

[cb1kenobi]

Barbarian reviewed edbacf0 and found no blocking issues. No new blocking findings were confirmed on changed lines. The existing discussion already covers the lost-watch recovery concern, so it was not repeated.