From a8972140eb5ce84dce784d45da5c49b4c835db90 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 27 Aug 2026 06:24:47 -0600 Subject: [PATCH 1/7] Stop a deleted watched path from raising an uncaught EPERM (Windows) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. An asynchronous watch failure — on Windows, deleting or replacing the watched directory — 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, which is why only our watchers hit it. Still unfixed as of chokidar 5.0.0, and reproducible with every shape Harper watches. Add `guardedWatch()` to utility/watcherFallback.ts: `chokidar.watch()` plus an idempotent, prepended process-level listener that claims only this error shape (`syscall === 'watch'` with EPERM/ENOENT), marks it `isHandled` so the thread-level handlers stay quiet, and logs once. Anything it does not claim is left exactly as fatal as Node would have made it, including when the guard is the only `uncaughtException` listener. Route all five chokidar call sites through it, and classify the same error as benign in the three watcher error handlers for the day chokidar does deliver it. Verified on Windows 11 / Node v24.14.0 / chokidar 4.0.3: each watcher shape (EntryHandler's cwd-relative base, the config-file watchers, manageThreads' directory watcher) crashed with `EPERM: operation not permitted, watch` before and survives after. Co-Authored-By: Claude Opus 5 --- DESIGN.md | 11 ++ components/EntryHandler.ts | 76 +++++----- components/OptionsWatcher.ts | 16 ++- config/RootConfigWatcher.ts | 16 ++- security/keys.ts | 12 +- server/threads/manageThreads.js | 6 +- server/threads/socketRouter.ts | 3 + unitTests/security/keys.test.js | 12 +- .../server/threads/watchDirFallback.test.js | 6 +- .../utility/fixtures/lostWatchHarness.cjs | 43 ++++++ unitTests/utility/watchPath.test.js | 14 +- unitTests/utility/watcherFallback.test.js | 100 +++++++++++++ utility/watcherFallback.ts | 136 ++++++++++++++++++ 13 files changed, 389 insertions(+), 62 deletions(-) create mode 100644 unitTests/utility/fixtures/lostWatchHarness.cjs diff --git a/DESIGN.md b/DESIGN.md index ecc83e5cc0..5e2e2d7921 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1242,6 +1242,17 @@ New watch sites must go through it. As of this writing the sites are `components `server/threads/manageThreads.js`, and `resources/blob.ts`. `fs.watchFile` (`utility/logging/readLog.ts`) is stat polling with no fs-event handle and is outside this invariant. +Five of those six sites arm the watch through `guardedWatch()` (`utility/watcherFallback.ts`) rather +than calling `chokidar.watch`/`fs.watch` directly — it installs a process-level guard for a second, +unrelated failure (a watched path deleted out from under a non-persistent chokidar watcher raises an +unhandled `EPERM`/`ENOENT`; see that file's header comment) but does no canonicalization of its own. +Every caller still resolves its own path first and passes the resolved path in, exactly as when they +called `chokidar.watch` directly, so the invariant holds through the wrapper. `utility/watcherFallback.ts` +itself is the one file that touches `chokidar` without canonicalizing — the watch-sites source scan +(`unitTests/utility/watchPath.test.js`) lists it as a native watch site (it does arm one) but exempts +it from the per-file canonicalization check, since canonicalizing is its callers' job, not its own — +the same relationship raw `chokidar.watch` has to the other five sites. + Two consequences worth knowing before adding a caller. `EntryHandler` is the one place where the canonical path is load-bearing past the `fs.watch` call: chokidar's `ignored` predicate receives absolute paths built from `cwd`, so its bases must be derived from the same spelling, while event diff --git a/components/EntryHandler.ts b/components/EntryHandler.ts index dab5ef2ac4..73acb4661b 100644 --- a/components/EntryHandler.ts +++ b/components/EntryHandler.ts @@ -4,7 +4,7 @@ import { createHash } from 'node:crypto'; import type { Stats } from 'node:fs'; import { EventEmitter } from 'node:events'; import { Component, FileAndURLPathConfig } from './Component.ts'; -import chokidar, { FSWatcher, FSWatcherEventMap } from 'chokidar'; +import { FSWatcher, FSWatcherEventMap } from 'chokidar'; import { isAbsolute, join } from 'node:path'; import { readFile } from 'node:fs/promises'; import { FilesOption } from './deriveGlobOptions.ts'; @@ -12,6 +12,8 @@ import { deriveURLPath } from './deriveURLPath.ts'; import { isMatch } from 'micromatch'; import { DIRECTORY_POLLING_FALLBACK_OPTIONS, + claimLostNativeWatchError, + guardedWatch, isWatcherExhaustionError, warnWatcherFallback, } from '../utility/watcherFallback.ts'; @@ -392,6 +394,11 @@ export class EntryHandler extends EventEmitter { } #handleWatcherError(error: unknown): void { + // A lost native watch handle (the watched tree was deleted or replaced) is + // benign and not actionable by a consumer. chokidar's non-persistent branch + // never routes it here today — installLostNativeWatchGuard() catches it at + // the process instead — but the polling branch and a fixed chokidar would. + if (claimLostNativeWatchError(error)) return; if (isWatcherExhaustionError(error)) { // Swallow every exhaustion error — chokidar can emit several before the // failed native watcher closes, and we don't want a flurry of ENOSPC to @@ -555,40 +562,39 @@ export class EntryHandler extends EventEmitter { } this.#openCount++; - const watcher = (this.#watcher = chokidar - .watch(watchPattern, { - cwd: watchDirectory, - persistent: false, - followSymlinks: false, - ...(this.#usingPolling ? DIRECTORY_POLLING_FALLBACK_OPTIONS : {}), - ignored: (path) => { - const normalizedPath = path.replace(/\\/g, '/'); - - // Determine the path relative to the component directory. Leading '/' is preserved - // (or empty when the path *is* the component directory) so the regex anchors below - // can use `(?:^|/)` to match the first segment without false positives on names - // that merely contain the same substring (e.g. `mynode_modules`, `notgit`). - const relativePath = normalizedPath.startsWith(normalizedDirectory) - ? normalizedPath.slice(normalizedDirectory.length) - : normalizedPath; - - // Skip node_modules at any depth. This allows plugins loaded from node_modules - // to still watch their own component files while ignoring their dependencies. - if (/(?:^|\/)node_modules(?:\/|$)/.test(relativePath)) return true; - - // Skip transient package manager and VCS artifacts. Without these, an in-place - // `npm install` during a component deploy writes log files and atomic-rename - // temp directories that fire change events and drive an auto-reload restart - // storm — see harper#488. - if (/(?:^|\/)\.git(?:\/|$)/.test(relativePath)) return true; - if (/(?:^|\/)\.tmp-/.test(relativePath)) return true; - if (/(?:^|\/)(?:npm-debug|yarn-error|yarn-debug|pnpm-debug)\.log(?:\/|$)/.test(relativePath)) return true; - - return ( - normalizedPath !== normalizedDirectory && normalizedBases.every((base) => !normalizedPath.startsWith(base)) - ); - }, - }) + const watcher = (this.#watcher = guardedWatch(watchPattern, { + cwd: watchDirectory, + persistent: false, + followSymlinks: false, + ...(this.#usingPolling ? DIRECTORY_POLLING_FALLBACK_OPTIONS : {}), + ignored: (path) => { + const normalizedPath = path.replace(/\\/g, '/'); + + // Determine the path relative to the component directory. Leading '/' is preserved + // (or empty when the path *is* the component directory) so the regex anchors below + // can use `(?:^|/)` to match the first segment without false positives on names + // that merely contain the same substring (e.g. `mynode_modules`, `notgit`). + const relativePath = normalizedPath.startsWith(normalizedDirectory) + ? normalizedPath.slice(normalizedDirectory.length) + : normalizedPath; + + // Skip node_modules at any depth. This allows plugins loaded from node_modules + // to still watch their own component files while ignoring their dependencies. + if (/(?:^|\/)node_modules(?:\/|$)/.test(relativePath)) return true; + + // Skip transient package manager and VCS artifacts. Without these, an in-place + // `npm install` during a component deploy writes log files and atomic-rename + // temp directories that fire change events and drive an auto-reload restart + // storm — see harper#488. + if (/(?:^|\/)\.git(?:\/|$)/.test(relativePath)) return true; + if (/(?:^|\/)\.tmp-/.test(relativePath)) return true; + if (/(?:^|\/)(?:npm-debug|yarn-error|yarn-debug|pnpm-debug)\.log(?:\/|$)/.test(relativePath)) return true; + + return ( + normalizedPath !== normalizedDirectory && normalizedBases.every((base) => !normalizedPath.startsWith(base)) + ); + }, + }) .on('all', (...args) => this.#handleAll(generation, ...args)) .on('error', (error) => { if (generation === this.#watchGeneration) this.#handleWatcherError(error); diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index 7f84dfe6f5..2430bced56 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -2,7 +2,7 @@ import { type Logger } from '../utility/logging/logger.ts'; import { loggerWithTag } from '../utility/logging/harper_logger.ts'; import { EventEmitter, once } from 'events'; import yaml from 'yaml'; -import chokidar, { type FSWatcher } from 'chokidar'; +import { type FSWatcher } from 'chokidar'; import { readFile } from 'node:fs/promises'; import { readFileSync } from 'node:fs'; import { isDeepStrictEqual } from 'util'; @@ -11,6 +11,8 @@ import { cloneDeep } from 'lodash'; import { POLLING_FALLBACK_OPTIONS, PartialReadRetry, + claimLostNativeWatchError, + guardedWatch, isPartialReadError, isWatcherExhaustionError, warnWatcherFallback, @@ -126,11 +128,10 @@ export class OptionsWatcher extends EventEmitter { #openWatcher() { this.#openCount++; - this.#watcher = chokidar - .watch(this.#watchPath, { - persistent: false, - ...(this.#usingPolling ? POLLING_FALLBACK_OPTIONS : {}), - }) + this.#watcher = guardedWatch(this.#watchPath, { + persistent: false, + ...(this.#usingPolling ? POLLING_FALLBACK_OPTIONS : {}), + }) .on('add', this.#handleChange.bind(this)) .on('change', this.#handleChange.bind(this)) .on('error', this.#handleError.bind(this)) @@ -256,6 +257,9 @@ export class OptionsWatcher extends EventEmitter { } #handleError(error: unknown) { + // See EntryHandler.#handleWatcherError: a lost native watch handle is benign + // and must not be surfaced to consumers as a config-watch failure. + if (claimLostNativeWatchError(error)) return; if (isWatcherExhaustionError(error)) { // Swallow every exhaustion error — chokidar can emit several before the // failed native watcher closes, and we don't want a flurry of ENOSPC to diff --git a/config/RootConfigWatcher.ts b/config/RootConfigWatcher.ts index 6f05ebf7f8..d440ea08a8 100644 --- a/config/RootConfigWatcher.ts +++ b/config/RootConfigWatcher.ts @@ -1,4 +1,4 @@ -import chokidar, { FSWatcher } from 'chokidar'; +import { FSWatcher } from 'chokidar'; import { readFileSync } from 'node:fs'; import { getConfigFilePath } from './configUtils.ts'; import { EventEmitter, once } from 'node:events'; @@ -6,6 +6,8 @@ import { parse } from 'yaml'; import { POLLING_FALLBACK_OPTIONS, PartialReadRetry, + claimLostNativeWatchError, + guardedWatch, isPartialReadError, isWatcherExhaustionError, warnWatcherFallback, @@ -38,11 +40,10 @@ export class RootConfigWatcher extends EventEmitter { #openWatcher() { this.#openCount++; - this.#watcher = chokidar - .watch(this.#watchPath, { - persistent: false, - ...(this.#usingPolling ? POLLING_FALLBACK_OPTIONS : {}), - }) + this.#watcher = guardedWatch(this.#watchPath, { + persistent: false, + ...(this.#usingPolling ? POLLING_FALLBACK_OPTIONS : {}), + }) .on('add', this.handleChange.bind(this)) .on('change', this.handleChange.bind(this)) .on('error', this.handleError.bind(this)); @@ -66,6 +67,9 @@ export class RootConfigWatcher extends EventEmitter { } handleError(error: unknown) { + // See EntryHandler.#handleWatcherError: a lost native watch handle is benign + // and must not be surfaced to consumers as a config-watch failure. + if (claimLostNativeWatchError(error)) return; if (isWatcherExhaustionError(error)) { // Swallow every exhaustion error — chokidar can emit several before the // failed native watcher closes, and we don't want a flurry of ENOSPC to diff --git a/security/keys.ts b/security/keys.ts index 7be716284c..80f17364ba 100644 --- a/security/keys.ts +++ b/security/keys.ts @@ -1,7 +1,6 @@ 'use strict'; import * as path from 'path'; -import { watch } from 'chokidar'; import * as fs from 'fs-extra'; import * as forge from 'node-forge'; import * as net from 'net'; @@ -33,7 +32,13 @@ export const getPrivateKeys = () => privateKeys; import { readFileSync, statSync } from 'node:fs'; import { getTicketKeys, onMessageFromWorkers } from '../server/threads/manageThreads.js'; import { isMainThread } from 'worker_threads'; -import { POLLING_FALLBACK_OPTIONS, isWatcherExhaustionError, warnWatcherFallback } from '../utility/watcherFallback.ts'; +import { + POLLING_FALLBACK_OPTIONS, + claimLostNativeWatchError, + guardedWatch, + isWatcherExhaustionError, + warnWatcherFallback, +} from '../utility/watcherFallback.ts'; import { resolveWatchTarget } from '../utility/watchPath.ts'; import { TLSSocket } from 'node:tls'; @@ -361,7 +366,7 @@ function loadAndWatch(path, loadCert, type) { let usingPolling = watchTarget.mustPoll; let liveWatcher; const openWatcher = () => { - const opened = (liveWatcher = watch(watchTarget.path, { + const opened = (liveWatcher = guardedWatch(watchTarget.path, { persistent: false, ...(usingPolling ? POLLING_FALLBACK_OPTIONS : {}), })); @@ -371,6 +376,7 @@ function loadAndWatch(path, loadCert, type) { .on('change', () => loadFile(path)) // chokidar emits 'error' unguarded for anything but ENOENT/ENOTDIR. .on('error', (error) => { + if (claimLostNativeWatchError(error)) return; if (isWatcherExhaustionError(error)) { if (usingPolling || liveWatcher !== opened) return; warnWatcherFallback(path); diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 13d302c8da..afd50062f6 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -26,6 +26,8 @@ const { getConfigPath } = require('../../config/configUtils.ts'); const { resolveWatchTarget } = require('../../utility/watchPath.ts'); const { DIRECTORY_POLLING_FALLBACK_OPTIONS, + claimLostNativeWatchError, + guardedWatch, isWatcherExhaustionError, warnWatcherFallback, } = require('../../utility/watcherFallback.ts'); @@ -49,7 +51,6 @@ function getRequireModules() { ); return requireModules; } -const chokidar = require('chokidar'); const isBun = typeof globalThis.Bun !== 'undefined'; const MB = 1024 * 1024; const workers = []; // these are our child workers that we are managing @@ -1506,7 +1507,7 @@ if (isMainThread) { let usingPolling = watchTarget.mustPoll; let liveWatcher; const openWatcher = () => { - const opened = (liveWatcher = chokidar.watch(watchTarget.path, { + const opened = (liveWatcher = guardedWatch(watchTarget.path, { persistent: false, ...(usingPolling ? DIRECTORY_POLLING_FALLBACK_OPTIONS : {}), ignored: (path) => { @@ -1517,6 +1518,7 @@ if (isMainThread) { // This runs on the thread that owns every worker, and chokidar emits 'error' unguarded for // anything but ENOENT/ENOTDIR. .on('error', (error) => { + if (claimLostNativeWatchError(error)) return; if (isWatcherExhaustionError(error)) { if (usingPolling || liveWatcher !== opened) return; warnWatcherFallback(dir); diff --git a/server/threads/socketRouter.ts b/server/threads/socketRouter.ts index ba91d5cc97..8610d14745 100644 --- a/server/threads/socketRouter.ts +++ b/server/threads/socketRouter.ts @@ -18,6 +18,9 @@ let sweptSocketsDirectory = false; if (isMainThread) { process.on('uncaughtException', (error) => { // TODO: Maybe we should try to log the first of each type of error + // Same `isHandled` contract as threadServer.js: an error another handler has already + // classified (a lost native file watch, for instance) must not be logged again here. + if ((error as any).isHandled) return; if ((error as any).code === 'ECONNRESET') return; // that's what network connections do if ((error as any).code === 'EIO') { // that means the terminal is closed diff --git a/unitTests/security/keys.test.js b/unitTests/security/keys.test.js index b8301091ee..fdb60bcfe3 100644 --- a/unitTests/security/keys.test.js +++ b/unitTests/security/keys.test.js @@ -1077,7 +1077,7 @@ describe('Test keys module', () => { const watchPollers = keys.__get__('certificateWatchPollers'); const localSandbox = sinon.createSandbox(); const chokidar = require('chokidar'); - const realChokidarWatch = chokidar.watch; + const realChokidarWatch = chokidar.default.watch; let watchPath; // Never open a real watcher here: these tests exercise only the poll/reopen paths, and real @@ -1094,13 +1094,13 @@ describe('Test keys module', () => { }; beforeEach(() => { - chokidar.watch = () => fakeWatcher(); + chokidar.default.watch = () => fakeWatcher(); watchPath = path.join(test_dir, `watch-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2)}.pem`); fs.writeFileSync(watchPath, 'PEM-V1'); }); afterEach(() => { - chokidar.watch = realChokidarWatch; + chokidar.default.watch = realChokidarWatch; localSandbox.restore(); // warnWatcherFallback's first-fallback gate is process-global; leaving it set would make a // later suite's warning assertion silently observe nothing. @@ -1134,7 +1134,7 @@ describe('Test keys module', () => { // chokidar v4 defaults alwaysStat:false, so the 'change' handler is called with undefined // stats; loadFile must stat the file itself rather than throw and silently skip the reload. let changeHandler; - chokidar.watch = () => + chokidar.default.watch = () => fakeWatcher((event, handler) => { if (event === 'change') changeHandler = handler; }); @@ -1159,7 +1159,7 @@ describe('Test keys module', () => { const openedOptions = []; const errorHandlers = []; const exhausted = () => Object.assign(new Error('inotify watch limit reached'), { code: 'ENOSPC' }); - chokidar.watch = (_watchedPath, options) => { + chokidar.default.watch = (_watchedPath, options) => { openedOptions.push(options); return fakeWatcher((event, handler) => { if (event === 'error') errorHandlers.push(handler); @@ -1189,7 +1189,7 @@ describe('Test keys module', () => { // of reopening on polling. const openedOptions = []; const errorHandlers = []; - chokidar.watch = (_watchedPath, options) => { + chokidar.default.watch = (_watchedPath, options) => { openedOptions.push(options); const watcher = { on: (event, handler) => { diff --git a/unitTests/server/threads/watchDirFallback.test.js b/unitTests/server/threads/watchDirFallback.test.js index 71369b0b30..d142778f3b 100644 --- a/unitTests/server/threads/watchDirFallback.test.js +++ b/unitTests/server/threads/watchDirFallback.test.js @@ -7,10 +7,10 @@ const { watchDir } = require('#src/server/threads/manageThreads'); const { _resetForTests: resetWatcherFallbackWarning } = require('#src/utility/watcherFallback'); describe('watchDir watcher fallback', () => { - const realWatch = chokidar.watch; + const realWatch = chokidar.default.watch; afterEach(() => { - chokidar.watch = realWatch; + chokidar.default.watch = realWatch; // warnWatcherFallback's first-fallback gate is process-global; leaving it set would make a // later suite's warning assertion silently observe nothing. resetWatcherFallbackWarning(); @@ -23,7 +23,7 @@ describe('watchDir watcher fallback', () => { const stubChokidar = (close) => { const openedOptions = []; const errorHandlers = []; - chokidar.watch = (_watchedPath, options) => { + chokidar.default.watch = (_watchedPath, options) => { openedOptions.push(options); const watcher = { on: (event, handler) => { diff --git a/unitTests/utility/fixtures/lostWatchHarness.cjs b/unitTests/utility/fixtures/lostWatchHarness.cjs new file mode 100644 index 0000000000..00d3f1f54a --- /dev/null +++ b/unitTests/utility/fixtures/lostWatchHarness.cjs @@ -0,0 +1,43 @@ +'use strict'; + +// Child-process harness for the lost-native-watch guard. It deliberately does NOT +// register an 'uncaughtException' listener of its own: the whole point is that the +// guard installed by guardedWatch() is the only thing standing between a deleted +// watched path and a dead process, and a listener here would mask that. + +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const { guardedWatch, _lostNativeWatchCountForTests } = require('#src/utility/watcherFallback'); + +const mode = process.argv[2]; + +const root = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-lost-watch-')); +const watched = path.join(root, 'component'); +fs.mkdirSync(path.join(watched, 'resources'), { recursive: true }); +fs.writeFileSync(path.join(watched, 'config.yaml'), 'name: fixture\n'); +fs.writeFileSync(path.join(watched, 'resources', 'index.js'), '// fixture\n'); + +// Matches EntryHandler's shape: a relative base resolved against `cwd`, which is +// what a component watcher actually opens. +const watcher = guardedWatch('.', { cwd: watched, persistent: false, followSymlinks: false }); + +watcher.on('ready', () => { + if (mode === 'unrelated-throw') { + // The guard is installed now. An unrelated uncaught exception must still be + // fatal — a guard that swallows everything turns crashes into silent hangs. + setTimeout(() => { + throw new Error('unrelated harness failure'); + }, 10); + return; + } + + // The delete is what raises `EPERM: operation not permitted, watch` on Windows, + // asynchronously, on a native watcher chokidar never attached a listener to. + fs.rmSync(watched, { recursive: true, force: true }); + setTimeout(() => { + process.stdout.write(`survived lostWatchCount=${_lostNativeWatchCountForTests()}\n`); + process.exit(0); + }, 1500); +}); diff --git a/unitTests/utility/watchPath.test.js b/unitTests/utility/watchPath.test.js index 372fcfeab4..126e4554b9 100644 --- a/unitTests/utility/watchPath.test.js +++ b/unitTests/utility/watchPath.test.js @@ -155,8 +155,16 @@ describe('watchPath', () => { 'resources/blob.ts', 'security/keys.ts', 'server/threads/manageThreads.js', + 'utility/watcherFallback.ts', ]; + // Sites whose own file canonicalization call is exempt: `guardedWatch` in + // utility/watcherFallback.ts is the shared primitive underneath every + // `guardedWatch(...)` call site below, taking an already-resolved path from + // its caller rather than resolving one of its own — the same relationship + // raw `chokidar.watch` has to the other listed sites. + const CANONICALIZES_VIA_CALLER = new Set(['utility/watcherFallback.ts']); + // `watch`, not `watchFile`: the latter is stat polling with no fs-event handle, so it is outside // the invariant (`utility/logging/readLog.ts`). `node:fs/promises` counts — its async iterator // is a real fs-event watch. @@ -168,6 +176,7 @@ describe('watchPath', () => { new RegExp(`\\{[^}]*\\bwatch\\b[^}]*\\}\\s*=\\s*require\\(\\s*['"]${FS_MODULE}['"]\\s*\\)`), new RegExp(`require\\(\\s*['"]${FS_MODULE}['"]\\s*\\)\\s*\\.watch\\s*\\(`), /\bfs(?:Promises|p)?\.watch\s*\(/, + /\bguardedWatch\s*\(/, ]; const repoRoot = join(__dirname, '..', '..'); @@ -212,9 +221,12 @@ describe('watchPath', () => { }); // File-level: catches a site that never canonicalizes, not a second raw watch beside a - // canonicalized one. + // canonicalized one. A CANONICALIZES_VIA_CALLER site is skipped here: its own callers are + // each listed in NATIVE_WATCH_SITES too (none of them exempted), so this same loop already + // requires every one of them to canonicalize before calling it. it('all route their path through the canonicalization helper', () => { for (const site of NATIVE_WATCH_SITES) { + if (CANONICALIZES_VIA_CALLER.has(site)) continue; const source = readFileSync(join(repoRoot, site), 'utf8'); assert.ok( /\bresolveWatchTarget\b|\bcanonicalizeWatchPath\b/.test(source), diff --git a/unitTests/utility/watcherFallback.test.js b/unitTests/utility/watcherFallback.test.js index 1cf2fc1a18..2f4e0c39e2 100644 --- a/unitTests/utility/watcherFallback.test.js +++ b/unitTests/utility/watcherFallback.test.js @@ -1,10 +1,14 @@ const { + claimLostNativeWatchError, + isLostNativeWatchError, isWatcherExhaustionError, POLLING_FALLBACK_OPTIONS, warnWatcherFallback, _resetForTests, } = require('#src/utility/watcherFallback'); const assert = require('node:assert'); +const { spawn } = require('node:child_process'); +const { once } = require('node:events'); describe('watcherFallback', () => { describe('isWatcherExhaustionError', () => { @@ -57,4 +61,100 @@ describe('watcherFallback', () => { assert.doesNotThrow(() => warnWatcherFallback('/some/other/path')); }); }); + + describe('isLostNativeWatchError', () => { + const watchError = (code) => Object.assign(new Error(`${code}: watch`), { code, syscall: 'watch', errno: -4048 }); + + it('identifies the Windows EPERM raised when a watched path is deleted', () => { + assert.equal(isLostNativeWatchError(watchError('EPERM')), true); + }); + + it('identifies an ENOENT watch failure', () => { + assert.equal(isLostNativeWatchError(watchError('ENOENT')), true); + }); + + // The guard swallows whatever this claims, process-wide, so the two neighbouring + // error shapes it must never claim are worth pinning: an EPERM from a config + // rename is a real failure the caller has to see, and exhaustion belongs to the + // polling-fallback route above, not here. + it('rejects an EPERM from a syscall other than watch', () => { + assert.equal( + isLostNativeWatchError(Object.assign(new Error('EPERM: rename'), { code: 'EPERM', syscall: 'rename' })), + false + ); + }); + + it('rejects watcher exhaustion errors', () => { + assert.equal(isLostNativeWatchError(watchError('ENOSPC')), false); + assert.equal(isLostNativeWatchError(watchError('EMFILE')), false); + }); + + it('rejects an EPERM with no syscall', () => { + assert.equal(isLostNativeWatchError(Object.assign(new Error('boom'), { code: 'EPERM' })), false); + }); + + it('rejects non-error values', () => { + assert.equal(isLostNativeWatchError(null), false); + assert.equal(isLostNativeWatchError(undefined), false); + assert.equal(isLostNativeWatchError('EPERM'), false); + }); + }); + + describe('claimLostNativeWatchError', () => { + afterEach(() => { + _resetForTests(); + }); + + it('claims a lost watch and marks it handled so the thread-level handler stays quiet', () => { + const error = Object.assign(new Error('EPERM: watch'), { code: 'EPERM', syscall: 'watch' }); + assert.equal(claimLostNativeWatchError(error), true); + assert.equal(error.isHandled, true); + }); + + it('leaves an unrelated error alone', () => { + const error = Object.assign(new Error('boom'), { code: 'EACCES' }); + assert.equal(claimLostNativeWatchError(error), false); + assert.equal(error.isHandled, undefined); + }); + }); + + // chokidar never attaches an 'error' listener to the underlying Node + // FSWatcher when `persistent: false` (which every Harper watcher uses), so an async + // watch failure is an uncaughtException rather than something the watcher can route. + // These run in a child process because the harness must be the only thing standing + // between the error and process death — mocha's own uncaughtException listener would + // otherwise absorb it and the test would pass regardless. + describe('lost native watch guard', () => { + const runHarness = async (mode) => { + const harness = spawn(process.execPath, [require.resolve('./fixtures/lostWatchHarness.cjs'), mode], { + stdio: ['ignore', 'pipe', 'pipe'], + }); + let stdout = ''; + let stderr = ''; + harness.stdout.on('data', (chunk) => (stdout += chunk)); + harness.stderr.on('data', (chunk) => (stderr += chunk)); + const [code] = await once(harness, 'close'); + return { code, stdout, stderr }; + }; + + it('survives deletion of a watched directory', async function () { + this.timeout(30000); + const { code, stdout, stderr } = await runHarness('delete-watched-dir'); + assert.equal(code, 0, `harness exited ${code}: ${stderr}`); + assert.match(stdout, /survived lostWatchCount=(\d+)/); + if (process.platform === 'win32') { + // Only Windows actually raises the error; elsewhere this case is a smoke + // test that the guard doesn't disturb ordinary watching. + const claimed = Number(stdout.match(/lostWatchCount=(\d+)/)[1]); + assert.ok(claimed > 0, 'expected the guard to have claimed at least one lost watch on Windows'); + } + }); + + it('leaves an unrelated uncaught exception fatal', async function () { + this.timeout(30000); + const { code, stderr } = await runHarness('unrelated-throw'); + assert.equal(code, 1); + assert.match(stderr, /unrelated harness failure/); + }); + }); }); diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 01daf64eb8..51a9140044 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -5,6 +5,7 @@ // events. Polling-based watching doesn't consume inotify handles or per-watcher // file descriptors, so we fall back to it once and warn — see harper#488. +import chokidar, { type ChokidarOptions, type FSWatcher } from 'chokidar'; import { loggerWithTag } from './logging/harper_logger.ts'; // One-time process-wide warning so a thundering herd of failing watchers doesn't @@ -72,6 +73,141 @@ export function warnWatcherFallback(watchedPath: string): void { // Test-only hook to reset the one-time warning gate between cases. export function _resetForTests(): void { exhaustionWarned = false; + lostNativeWatchCount = 0; +} + +// --------------------------------------------------------------------------- +// Lost native watch (predominantly Windows) +// --------------------------------------------------------------------------- +// +// Every Harper watcher runs chokidar with `persistent: false` so a watcher never +// holds the event loop open. That option takes chokidar down a code path that +// never attaches an 'error' listener to the underlying Node `fs.FSWatcher` +// (chokidar `handler.js`, `setFsWatchListener`): +// +// if (!options.persistent) { +// watcher = createFsWatchInstance(path, options, listener, errHandler, rawEmitter); +// if (!watcher) return; +// return watcher.close.bind(watcher); // <-- no watcher.on('error', ...) +// } +// +// `errHandler` is only consulted for a *synchronous* throw out of `fs.watch()` +// (which is how ENOSPC/EMFILE reach `isWatcherExhaustionError` above). An +// *asynchronous* watch failure — on Windows, deleting or replacing the watched +// directory — is delivered by `node:internal/fs/watchers` as +// `this.emit('error', err)` on an emitter with no listener, so Node turns it +// into an uncaughtException. The error never reaches chokidar's wrapper, so +// attaching `.on('error')` to the chokidar `FSWatcher` we hold does not help. +// +// chokidar's `persistent: true` branch does attach a listener and swallows this +// exact error (its node#4337 workaround), which is why only Harper's watchers +// see it. The upstream fix is one line — `watcher.on('error', errHandler)` in +// the non-persistent branch — and is still missing as of chokidar 5.0.0. +// +// Until then `installLostNativeWatchGuard()` supplies the missing listener at +// the only place the error is observable: the process. It claims *only* this +// error shape and leaves every other uncaught exception exactly as Node would +// have handled it. + +// Watch failures that mean "this native watch handle is gone". The watched path +// has been deleted or replaced; there is nothing to recover and nothing the +// operator can do, so the error is benign. Kept deliberately narrow — anything +// matched here is swallowed process-wide. +const LOST_NATIVE_WATCH_CODES = new Set(['EPERM', 'ENOENT']); + +/** + * Returns `true` for the asynchronous "the watched path went away" error raised + * by Node's `fs.FSWatcher`. On Windows this is + * `EPERM: operation not permitted, watch` (errno -4048) and it fires whenever a + * watched directory is removed or swapped out — a component redeploy, a test + * fixture teardown, an `npm install` that replaces a tree. + * + * Deliberately distinct from {@link isWatcherExhaustionError}: exhaustion means + * the host ran out of watch capacity and polling is a useful degradation; a lost + * native watch means the thing being watched no longer exists, and polling would + * only burn CPU on a path that isn't there. + */ +export function isLostNativeWatchError(error: unknown): boolean { + if (typeof error !== 'object' || error === null) return false; + const { code, syscall } = error as { code?: unknown; syscall?: unknown }; + // `syscall === 'watch'` is only ever set by fs watch handles, so the pair is + // specific enough to claim without also inspecting the stack (whose frame + // text is a Node internal we don't want to depend on). + return syscall === 'watch' && typeof code === 'string' && LOST_NATIVE_WATCH_CODES.has(code); +} + +let lostNativeWatchCount = 0; + +/** + * If `error` is a lost native watch error, mark it handled, log it, and report + * that it has been claimed. Exported so the per-watcher error routes can give it + * the same benign treatment on the day chokidar does deliver it to the wrapper + * (its polling and `persistent: true` branches already would). + */ +export function claimLostNativeWatchError(error: unknown): boolean { + if (!isLostNativeWatchError(error)) return false; + // server/threads/threadServer.js skips errors already marked handled, so the + // benign case doesn't also get logged there as a worker-level uncaughtException. + (error as { isHandled?: boolean }).isHandled = true; + lostNativeWatchCount++; + if (lostNativeWatchCount === 1) { + fallbackLogger.warn?.( + `A file watch handle was lost because the watched path was deleted or replaced ` + + `(${(error as { code?: string }).code}, syscall=watch). This is expected on Windows during ` + + `component redeploys and is not actionable; the watcher for that path stops reporting changes ` + + `and is re-established the next time the path is watched. Further occurrences log at trace.` + ); + } else { + fallbackLogger.trace?.(`Lost file watch handle (occurrence ${lostNativeWatchCount}):`, error); + } + return true; +} + +let lostNativeWatchGuardInstalled = false; + +function handleUncaughtException(error: unknown): void { + if (claimLostNativeWatchError(error)) return; + // Not ours. Node suppresses its default fatal handling as soon as *any* + // 'uncaughtException' listener exists, so if this guard is the only listener + // its mere presence would turn unrelated crashes into silent hangs. Step out + // of the way and let the exception be fatal exactly as it would have been. + if (process.listenerCount('uncaughtException') > 1) return; + process.removeListener('uncaughtException', handleUncaughtException); + // Re-raising on the next tick (rather than throwing from inside the handler) + // keeps Node's normal report and exit code 1; a throw from within the handler + // exits 7 instead. + process.nextTick(() => { + throw error; + }); +} + +/** + * Idempotently install the process-level listener that supplies the 'error' + * handler chokidar omits for non-persistent watchers. Called by + * {@link guardedWatch} so no watcher call site can forget it. + */ +export function installLostNativeWatchGuard(): void { + if (lostNativeWatchGuardInstalled) return; + lostNativeWatchGuardInstalled = true; + // prepend, not append: threadServer.js already has an 'uncaughtException' + // listener, and the `isHandled` mark above only suppresses its log line if + // this guard runs first. + process.prependListener('uncaughtException', handleUncaughtException); +} + +/** + * `chokidar.watch()` with the lost-native-watch guard installed. Every Harper + * chokidar watcher must go through this — a raw `chokidar.watch(..., { persistent: false })` + * can take down the thread the first time its path is deleted. + */ +export function guardedWatch(paths: string | string[], options?: ChokidarOptions): FSWatcher { + installLostNativeWatchGuard(); + return chokidar.watch(paths, options); +} + +// Test-only: number of lost native watch errors claimed so far. +export function _lostNativeWatchCountForTests(): number { + return lostNativeWatchCount; } /** From ee715b23483b92bc46570bf6273957bc407f2c60 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sun, 30 Aug 2026 06:49:59 -0600 Subject: [PATCH 2/7] Narrow the lost-watch guard to the async failure, and stop it going silent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-model review raised that the process-global guard classifies by error shape alone, so it could swallow an unrelated failure and leave a subsystem silently unwatched. Two of its three parts hold up, and both are now closed. The claimed set was too wide. A synchronous `fs.watch()` throw — the ordinary "watch a path that isn't there" misconfiguration — shares `syscall: 'watch'`, and with ENOENT in the set it matched exactly. Node distinguishes the two: the thrown error carries `path`, the one delivered to the handle's 'error' event does not (it carries `filename: null` and nothing else locating it). Claim only EPERM, and only when `path` is absent. An async ENOENT from a watch handle has never been observed; a synchronous one now stays fatal, with a child-process case proving it. Repeat occurrences were invisible. The default log level is `warn` and only the first occurrence logged there, so a watcher that stopped reporting after the first would never surface. Warn on the first and at each tenfold increase, trace on every one — bounded against a delete storm without suppressing the signal. The message no longer asserts a cause it cannot know: Node gives this error no path, so it says the failure cannot be attributed to a specific watcher and names the Windows deleted-directory case as the usual, not certain, explanation. Not adopted: restricting the exemption to registered guarded roots. It is not implementable — the error carries no path, which is the same fact that makes per-watcher routing impossible in the first place. Co-Authored-By: Claude Opus 5 --- .../utility/fixtures/lostWatchHarness.cjs | 10 +++ unitTests/utility/watcherFallback.test.js | 32 +++++++++- utility/watcherFallback.ts | 61 ++++++++++++------- 3 files changed, 78 insertions(+), 25 deletions(-) diff --git a/unitTests/utility/fixtures/lostWatchHarness.cjs b/unitTests/utility/fixtures/lostWatchHarness.cjs index 00d3f1f54a..3988d592a3 100644 --- a/unitTests/utility/fixtures/lostWatchHarness.cjs +++ b/unitTests/utility/fixtures/lostWatchHarness.cjs @@ -24,6 +24,16 @@ fs.writeFileSync(path.join(watched, 'resources', 'index.js'), '// fixture\n'); const watcher = guardedWatch('.', { cwd: watched, persistent: false, followSymlinks: false }); watcher.on('ready', () => { + if (mode === 'sync-watch-failure') { + // A synchronous fs.watch() throw shares the guard's syscall ('watch') but carries a `path`, + // which is what keeps an ordinary "watching a path that isn't there" misconfiguration fatal + // instead of being mistaken for a benign lost watch and swallowed. + setTimeout(() => { + fs.watch(path.join(root, 'no-such-directory'), { persistent: false }, () => {}); + }, 10); + return; + } + if (mode === 'unrelated-throw') { // The guard is installed now. An unrelated uncaught exception must still be // fatal — a guard that swallows everything turns crashes into silent hangs. diff --git a/unitTests/utility/watcherFallback.test.js b/unitTests/utility/watcherFallback.test.js index 2f4e0c39e2..cc52ea193a 100644 --- a/unitTests/utility/watcherFallback.test.js +++ b/unitTests/utility/watcherFallback.test.js @@ -63,14 +63,30 @@ describe('watcherFallback', () => { }); describe('isLostNativeWatchError', () => { - const watchError = (code) => Object.assign(new Error(`${code}: watch`), { code, syscall: 'watch', errno: -4048 }); + // The async shape Node delivers to the watch handle's 'error' event: no `path`, and a + // `filename` that is null. + const watchError = (code) => + Object.assign(new Error(`${code}: watch`), { code, syscall: 'watch', errno: -4048, filename: null }); it('identifies the Windows EPERM raised when a watched path is deleted', () => { assert.equal(isLostNativeWatchError(watchError('EPERM')), true); }); - it('identifies an ENOENT watch failure', () => { - assert.equal(isLostNativeWatchError(watchError('ENOENT')), true); + // Whatever this claims is swallowed process-wide, so the shapes it must NOT claim matter + // as much as the one it must. A synchronous fs.watch() throw carries a `path`; an async + // ENOENT from a watch handle has never been observed, while a synchronous one is the + // ordinary "watch a path that isn't there" misconfiguration and has to stay fatal. + it('rejects a synchronous fs.watch throw, which carries a path', () => { + assert.equal( + isLostNativeWatchError( + Object.assign(new Error('EPERM: watch'), { code: 'EPERM', syscall: 'watch', path: '/some/dir' }) + ), + false + ); + }); + + it('rejects an ENOENT watch failure', () => { + assert.equal(isLostNativeWatchError(watchError('ENOENT')), false); }); // The guard swallows whatever this claims, process-wide, so the two neighbouring @@ -156,5 +172,15 @@ describe('watcherFallback', () => { assert.equal(code, 1); assert.match(stderr, /unrelated harness failure/); }); + + // The guard sees every uncaught exception in the process, so the bound that keeps it from + // masking real failures is the error shape. A misconfigured raw fs.watch() shares its + // syscall and would be swallowed if the shape check were any looser. + it('leaves a synchronous fs.watch failure on a missing path fatal', async function () { + this.timeout(30000); + const { code, stderr } = await runHarness('sync-watch-failure'); + assert.equal(code, 1); + assert.match(stderr, /ENOENT/); + }); }); }); diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 51a9140044..9739d0ae0f 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -74,6 +74,7 @@ export function warnWatcherFallback(watchedPath: string): void { export function _resetForTests(): void { exhaustionWarned = false; lostNativeWatchCount = 0; + lostNativeWatchWarnThreshold = 1; } // --------------------------------------------------------------------------- @@ -109,34 +110,41 @@ export function _resetForTests(): void { // error shape and leaves every other uncaught exception exactly as Node would // have handled it. -// Watch failures that mean "this native watch handle is gone". The watched path -// has been deleted or replaced; there is nothing to recover and nothing the -// operator can do, so the error is benign. Kept deliberately narrow — anything -// matched here is swallowed process-wide. -const LOST_NATIVE_WATCH_CODES = new Set(['EPERM', 'ENOENT']); - /** * Returns `true` for the asynchronous "the watched path went away" error raised - * by Node's `fs.FSWatcher`. On Windows this is - * `EPERM: operation not permitted, watch` (errno -4048) and it fires whenever a - * watched directory is removed or swapped out — a component redeploy, a test - * fixture teardown, an `npm install` that replaces a tree. + * by Node's `fs.FSWatcher`: `EPERM: operation not permitted, watch` (errno + * -4048), which fires on Windows whenever a watched directory is removed or + * swapped out — a component redeploy, a test fixture teardown, an `npm install` + * that replaces a tree. * * Deliberately distinct from {@link isWatcherExhaustionError}: exhaustion means * the host ran out of watch capacity and polling is a useful degradation; a lost * native watch means the thing being watched no longer exists, and polling would * only burn CPU on a path that isn't there. + * + * The three conditions are all load-bearing, because whatever this claims is + * swallowed process-wide: + * + * - `syscall === 'watch'` is only ever set by fs watch handles. + * - `EPERM` only. 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, which must stay fatal. + * - no `path`. This is what 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 (which + * carries `filename: null` and nothing else locating it). Without this a + * raw `fs.watch(missingPath)` escaping into an uncaughtException would be + * mistaken for a benign lost watch and silently swallowed. */ export function isLostNativeWatchError(error: unknown): boolean { if (typeof error !== 'object' || error === null) return false; - const { code, syscall } = error as { code?: unknown; syscall?: unknown }; - // `syscall === 'watch'` is only ever set by fs watch handles, so the pair is - // specific enough to claim without also inspecting the stack (whose frame - // text is a Node internal we don't want to depend on). - return syscall === 'watch' && typeof code === 'string' && LOST_NATIVE_WATCH_CODES.has(code); + const { code, syscall, path } = error as { code?: unknown; syscall?: unknown; path?: unknown }; + return syscall === 'watch' && code === 'EPERM' && path == null; } let lostNativeWatchCount = 0; +// Warn on occurrence 1, then 10, 100, … — see claimLostNativeWatchError(). +let lostNativeWatchWarnThreshold = 1; /** * If `error` is a lost native watch error, mark it handled, log it, and report @@ -150,15 +158,24 @@ export function claimLostNativeWatchError(error: unknown): boolean { // benign case doesn't also get logged there as a worker-level uncaughtException. (error as { isHandled?: boolean }).isHandled = true; lostNativeWatchCount++; - if (lostNativeWatchCount === 1) { + fallbackLogger.trace?.(`Lost native file watch handle (occurrence ${lostNativeWatchCount}):`, error); + // Never go fully silent. Node gives this error no path, so it cannot be attributed to the + // watcher that raised it — ours or a dependency's — and a subsystem that has quietly stopped + // being watched is exactly what an operator needs to see. The default log level is `warn`, so + // trace alone would make every occurrence after the first invisible. Warn on the first and + // then at each decade, which keeps a delete storm (one error per directory in a removed tree) + // bounded without ever suppressing the signal outright. + if (lostNativeWatchCount >= lostNativeWatchWarnThreshold) { + lostNativeWatchWarnThreshold *= 10; fallbackLogger.warn?.( - `A file watch handle was lost because the watched path was deleted or replaced ` + - `(${(error as { code?: string }).code}, syscall=watch). This is expected on Windows during ` + - `component redeploys and is not actionable; the watcher for that path stops reporting changes ` + - `and is re-established the next time the path is watched. Further occurrences log at trace.` + `A native file watch handle failed asynchronously (EPERM, syscall=watch) and was suppressed to ` + + `keep the thread alive; occurrence ${lostNativeWatchCount}. Node reports no path for this error, ` + + `so it cannot be attributed to a specific watcher — whatever was watching that path has stopped ` + + `reporting changes until it is re-established. On Windows this is usually a watched directory ` + + `being deleted or replaced (a component redeploy, a package install, a test teardown). If file ` + + `changes stop being picked up somewhere, this is why. Subsequent occurrences log at trace, with ` + + `a warning at each tenfold increase.` ); - } else { - fallbackLogger.trace?.(`Lost file watch handle (occurrence ${lostNativeWatchCount}):`, error); } return true; } From 0ac414b274d349dfa1f2ec880f8ba41d23642dd5 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sun, 30 Aug 2026 07:26:30 -0600 Subject: [PATCH 3/7] Re-arm the guard after self-removal, and prove the ordering it depends on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second cross-model round. Codex, run with repository read access, verified every claim against the code and found no blocker or significant issue; Gemini raised five, of which three hold. Adopted: - The guard clears its installed flag when it steps aside for an unrelated exception. Stepping aside is meant to be terminal, but the flag said otherwise: a process that somehow survived would have run unguarded for the rest of its life rather than re-arming on the next guardedWatch(). - `_resetForTests` drops the process listener it may have installed, instead of leaving one attached to the test runner for every case that follows. - `claimLostNativeWatchError` no longer counts an error instance twice, which would inflate the tally and trip the decade warning early. - Named `watch` import rather than the default export. The default export is real in chokidar 4 and 5, so this changes nothing, but it removes a question a reader has to answer. Rejected: that `require('../../utility/watcherFallback.ts')` in a CJS module throws MODULE_NOT_FOUND. `manageThreads.js` already has eight such requires, and the build rewrites them to `.js`. Two tests from Codex's suggestions, both of which failed to exist rather than failing to pass: - Prepend ordering was the mechanism the `isHandled` mark depends on and nothing proved it. The harness now registers a threadServer-style listener *before* the first watcher and asserts it observes `isHandled=true` — appending would leave it false. - The warn cadence had no coverage. Twelve claims must produce exactly two warnings, at occurrences 1 and 10. Co-Authored-By: Claude Opus 5 --- .../utility/fixtures/lostWatchHarness.cjs | 37 ++++++++++++++++++- unitTests/utility/watcherFallback.test.js | 21 +++++++++++ utility/watcherFallback.ts | 22 ++++++++--- 3 files changed, 74 insertions(+), 6 deletions(-) diff --git a/unitTests/utility/fixtures/lostWatchHarness.cjs b/unitTests/utility/fixtures/lostWatchHarness.cjs index 3988d592a3..af145597a8 100644 --- a/unitTests/utility/fixtures/lostWatchHarness.cjs +++ b/unitTests/utility/fixtures/lostWatchHarness.cjs @@ -9,10 +9,36 @@ const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); -const { guardedWatch, _lostNativeWatchCountForTests } = require('#src/utility/watcherFallback'); +const { + claimLostNativeWatchError, + guardedWatch, + _lostNativeWatchCountForTests, +} = require('#src/utility/watcherFallback'); const mode = process.argv[2]; +if (mode === 'warn-threshold') { + // No watcher needed: claim a run of synthetic lost-watch errors and let the caller count the + // warnings that reach the log. Warn is the default level, so this is the visible cadence. + for (let i = 0; i < 12; i++) { + claimLostNativeWatchError( + Object.assign(new Error('EPERM: watch'), { code: 'EPERM', syscall: 'watch', filename: null }) + ); + } + process.stdout.write(`claimed=${_lostNativeWatchCountForTests()}\n`); + process.exit(0); +} + +if (mode === 'prepend-ordering') { + // Stands in for the handlers in server/threads/threadServer.js and socketRouter.ts, which are + // registered at module load — before any watcher exists. They skip errors already marked + // handled, which only works if the guard's listener runs ahead of them. + process.on('uncaughtException', (error) => { + process.stdout.write(`thread-handler saw isHandled=${error.isHandled}\n`); + process.exit(0); + }); +} + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-lost-watch-')); const watched = path.join(root, 'component'); fs.mkdirSync(path.join(watched, 'resources'), { recursive: true }); @@ -24,6 +50,15 @@ fs.writeFileSync(path.join(watched, 'resources', 'index.js'), '// fixture\n'); const watcher = guardedWatch('.', { cwd: watched, persistent: false, followSymlinks: false }); watcher.on('ready', () => { + if (mode === 'prepend-ordering') { + fs.rmSync(watched, { recursive: true, force: true }); + setTimeout(() => { + process.stdout.write('thread-handler never ran\n'); + process.exit(3); + }, 1500); + return; + } + if (mode === 'sync-watch-failure') { // A synchronous fs.watch() throw shares the guard's syscall ('watch') but carries a `path`, // which is what keeps an ordinary "watching a path that isn't there" misconfiguration fatal diff --git a/unitTests/utility/watcherFallback.test.js b/unitTests/utility/watcherFallback.test.js index cc52ea193a..b9e85593b2 100644 --- a/unitTests/utility/watcherFallback.test.js +++ b/unitTests/utility/watcherFallback.test.js @@ -173,6 +173,27 @@ describe('watcherFallback', () => { assert.match(stderr, /unrelated harness failure/); }); + // The `isHandled` marker only suppresses the thread-level handlers if the guard's listener + // runs before theirs — which is why it is prepended, not appended. Node calls every + // uncaughtException listener regardless, so ordering is the whole mechanism. + it('runs ahead of a thread-level handler registered before the first watcher', async function () { + this.timeout(30000); + const { code, stdout, stderr } = await runHarness('prepend-ordering'); + if (process.platform !== 'win32') return this.skip(); // only Windows raises the error + assert.equal(code, 0, `harness exited ${code}: ${stderr}`); + assert.match(stdout, /thread-handler saw isHandled=true/); + }); + + it('warns on the first occurrence and at each tenfold increase', async function () { + this.timeout(30000); + const { code, stdout, stderr } = await runHarness('warn-threshold'); + assert.equal(code, 0, `harness exited ${code}: ${stderr}`); + assert.match(stdout, /claimed=12/); + // 12 claims => warnings at occurrence 1 and 10, and no others. + const warnings = `${stdout}${stderr}`.match(/failed asynchronously/g) ?? []; + assert.equal(warnings.length, 2, `expected 2 warnings across 12 claims, got ${warnings.length}`); + }); + // The guard sees every uncaught exception in the process, so the bound that keeps it from // masking real failures is the error shape. A misconfigured raw fs.watch() shares its // syscall and would be swallowed if the shape check were any looser. diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 9739d0ae0f..db9b8ca1c4 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -5,7 +5,7 @@ // events. Polling-based watching doesn't consume inotify handles or per-watcher // file descriptors, so we fall back to it once and warn — see harper#488. -import chokidar, { type ChokidarOptions, type FSWatcher } from 'chokidar'; +import { watch as chokidarWatch, type ChokidarOptions, type FSWatcher } from 'chokidar'; import { loggerWithTag } from './logging/harper_logger.ts'; // One-time process-wide warning so a thundering herd of failing watchers doesn't @@ -75,6 +75,10 @@ export function _resetForTests(): void { exhaustionWarned = false; lostNativeWatchCount = 0; lostNativeWatchWarnThreshold = 1; + // Also drop the process listener, so a case that installed the guard can't leave one + // attached to the test runner's process for every case that follows. + process.removeListener('uncaughtException', handleUncaughtException); + lostNativeWatchGuardInstalled = false; } // --------------------------------------------------------------------------- @@ -154,9 +158,13 @@ let lostNativeWatchWarnThreshold = 1; */ export function claimLostNativeWatchError(error: unknown): boolean { if (!isLostNativeWatchError(error)) return false; - // server/threads/threadServer.js skips errors already marked handled, so the - // benign case doesn't also get logged there as a worker-level uncaughtException. - (error as { isHandled?: boolean }).isHandled = true; + const claimed = error as { isHandled?: boolean }; + // Already claimed. Counting the same instance twice would inflate the tally and trip the + // decade warning early, so report it as claimed and stop. + if (claimed.isHandled) return true; + // server/threads/threadServer.js and server/threads/socketRouter.ts skip errors already + // marked handled, so the benign case doesn't also get logged there as an uncaughtException. + claimed.isHandled = true; lostNativeWatchCount++; fallbackLogger.trace?.(`Lost native file watch handle (occurrence ${lostNativeWatchCount}):`, error); // Never go fully silent. Node gives this error no path, so it cannot be attributed to the @@ -190,6 +198,10 @@ function handleUncaughtException(error: unknown): void { // of the way and let the exception be fatal exactly as it would have been. if (process.listenerCount('uncaughtException') > 1) return; process.removeListener('uncaughtException', handleUncaughtException); + // Stepping aside is meant to be terminal, but say so in the state rather than assuming it: + // clearing the flag means a process that somehow survives re-arms on its next guardedWatch() + // instead of running unguarded for the rest of its life. + lostNativeWatchGuardInstalled = false; // Re-raising on the next tick (rather than throwing from inside the handler) // keeps Node's normal report and exit code 1; a throw from within the handler // exits 7 instead. @@ -219,7 +231,7 @@ export function installLostNativeWatchGuard(): void { */ export function guardedWatch(paths: string | string[], options?: ChokidarOptions): FSWatcher { installLostNativeWatchGuard(); - return chokidar.watch(paths, options); + return chokidarWatch(paths, options); } // Test-only: number of lost native watch errors claimed so far. From 7a8acb2b899cd74b6667f5845d58bea6fefbfc3f Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sun, 30 Aug 2026 07:38:04 -0600 Subject: [PATCH 4/7] =?UTF-8?q?Keep=20guardedWatch=20on=20the=20default=20?= =?UTF-8?q?export=20=E2=80=94=20it=20is=20a=20test=20seam?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The named `watch` import was adopted last commit as a free tidy-up. It was not free: unitTests/server/threads/watchDirFallback.test.js drives the reopen-on-exhaustion chain by swapping `chokidar.default.watch`, and a named import binds past that seam, so both of its cases saw zero watcher opens. That test arrived with the rebase onto current main and did not exist on the branch's old base, which is how the change looked harmless. Back to the property access, with a comment saying why it has to stay one. Co-Authored-By: Claude Opus 5 --- utility/watcherFallback.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index db9b8ca1c4..4fe7de7ca1 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -5,7 +5,7 @@ // events. Polling-based watching doesn't consume inotify handles or per-watcher // file descriptors, so we fall back to it once and warn — see harper#488. -import { watch as chokidarWatch, type ChokidarOptions, type FSWatcher } from 'chokidar'; +import chokidar, { type ChokidarOptions, type FSWatcher } from 'chokidar'; import { loggerWithTag } from './logging/harper_logger.ts'; // One-time process-wide warning so a thundering herd of failing watchers doesn't @@ -231,7 +231,10 @@ export function installLostNativeWatchGuard(): void { */ export function guardedWatch(paths: string | string[], options?: ChokidarOptions): FSWatcher { installLostNativeWatchGuard(); - return chokidarWatch(paths, options); + // Deliberately a property access on the default export rather than a named `watch` import: + // unitTests/server/threads/watchDirFallback.test.js drives the reopen-on-exhaustion chain by + // swapping `chokidar.default.watch`, and a named import binds past that seam. + return chokidar.watch(paths, options); } // Test-only: number of lost native watch errors claimed so far. From 4107f0d5f5198da206955017b4de2511e1569bf4 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sun, 30 Aug 2026 18:27:28 -0600 Subject: [PATCH 5/7] Keep a frozen error from turning the guard into the crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard's listener is prepended, so it classifies every uncaught exception before Harper's own handlers see one. Classification mutates the error (`isHandled = true`), and on an error that is frozen — or with a throwing property getter — that mutation throws from inside an 'uncaughtException' listener: Node then reports the guard's TypeError instead of the original error and exits 7, and threadServer/socketRouter never get their turn. Classify inside a try/catch and treat a failure to classify as "not ours", which leaves the original exception fatal exactly as it would have been unguarded. The new child-process case pins it with a frozen lost-watch-shaped error, so it runs on the ubuntu unit-test CI rather than only on Windows. Also stop the two fixed 1.5s waits in the harness from racing Windows delivery: the deadline is now 5s on win32 (where the error is the thing being waited for and CI runners are loaded) and unchanged elsewhere, and the delete case polls and exits as soon as it observes the claim instead of always sleeping. The prepend-ordering case now skips before spawning off Windows, where it could only ever wait out the deadline and skip. Co-Authored-By: Claude Opus 5 --- .../utility/fixtures/lostWatchHarness.cjs | 42 ++++++++++++++++--- unitTests/utility/watcherFallback.test.js | 15 ++++++- utility/watcherFallback.ts | 13 +++++- 3 files changed, 63 insertions(+), 7 deletions(-) diff --git a/unitTests/utility/fixtures/lostWatchHarness.cjs b/unitTests/utility/fixtures/lostWatchHarness.cjs index af145597a8..ba4305213f 100644 --- a/unitTests/utility/fixtures/lostWatchHarness.cjs +++ b/unitTests/utility/fixtures/lostWatchHarness.cjs @@ -17,6 +17,21 @@ const { const mode = process.argv[2]; +// How long to wait for an asynchronous watch failure. On Windows it is the error itself we are +// waiting for, and delivery competes with everything else on a loaded CI runner; elsewhere nothing +// will ever arrive and this is only a settle period, so it stays short. Cases that can observe the +// error poll for it and exit early, so the deadline is a ceiling, not a sleep. +const DELIVERY_DEADLINE_MS = process.platform === 'win32' ? 5000 : 1500; + +function waitForDelivery(isDelivered, onDeadline) { + const deadline = Date.now() + DELIVERY_DEADLINE_MS; + const poll = setInterval(() => { + if (!isDelivered() && Date.now() < deadline) return; + clearInterval(poll); + onDeadline(); + }, 50); +} + if (mode === 'warn-threshold') { // No watcher needed: claim a run of synthetic lost-watch errors and let the caller count the // warnings that reach the log. Warn is the default level, so this is the visible cadence. @@ -52,10 +67,24 @@ const watcher = guardedWatch('.', { cwd: watched, persistent: false, followSymli watcher.on('ready', () => { if (mode === 'prepend-ordering') { fs.rmSync(watched, { recursive: true, force: true }); + // The handler above exits as soon as it runs, so reaching this is the failure. setTimeout(() => { process.stdout.write('thread-handler never ran\n'); process.exit(3); - }, 1500); + }, DELIVERY_DEADLINE_MS); + return; + } + + if (mode === 'frozen-claim') { + // A lost-watch-shaped error the guard cannot mark handled: `isHandled` is unassignable on a + // frozen object, so classifying it throws. That throw happens inside the guard's + // 'uncaughtException' listener, which runs first, so an unprotected guard would replace this + // error's report with a TypeError and exit 7 instead of leaving it fatal on its own terms. + setTimeout(() => { + throw Object.freeze( + Object.assign(new Error('frozen lost watch'), { code: 'EPERM', syscall: 'watch', filename: null }) + ); + }, 10); return; } @@ -81,8 +110,11 @@ watcher.on('ready', () => { // The delete is what raises `EPERM: operation not permitted, watch` on Windows, // asynchronously, on a native watcher chokidar never attached a listener to. fs.rmSync(watched, { recursive: true, force: true }); - setTimeout(() => { - process.stdout.write(`survived lostWatchCount=${_lostNativeWatchCountForTests()}\n`); - process.exit(0); - }, 1500); + waitForDelivery( + () => _lostNativeWatchCountForTests() > 0, + () => { + process.stdout.write(`survived lostWatchCount=${_lostNativeWatchCountForTests()}\n`); + process.exit(0); + } + ); }); diff --git a/unitTests/utility/watcherFallback.test.js b/unitTests/utility/watcherFallback.test.js index b9e85593b2..40a3304e07 100644 --- a/unitTests/utility/watcherFallback.test.js +++ b/unitTests/utility/watcherFallback.test.js @@ -178,8 +178,8 @@ describe('watcherFallback', () => { // uncaughtException listener regardless, so ordering is the whole mechanism. it('runs ahead of a thread-level handler registered before the first watcher', async function () { this.timeout(30000); - const { code, stdout, stderr } = await runHarness('prepend-ordering'); if (process.platform !== 'win32') return this.skip(); // only Windows raises the error + const { code, stdout, stderr } = await runHarness('prepend-ordering'); assert.equal(code, 0, `harness exited ${code}: ${stderr}`); assert.match(stdout, /thread-handler saw isHandled=true/); }); @@ -203,5 +203,18 @@ describe('watcherFallback', () => { assert.equal(code, 1); assert.match(stderr, /ENOENT/); }); + + // The guard runs before every other uncaughtException listener, so a throw out of its own + // classification would cost the process both the original error's report (exit 7, with the + // guard's TypeError in its place) and the thread-level handlers' turn. A frozen error of + // exactly the claimed shape is the reachable version of that: reading it succeeds, marking + // it handled does not. + it('stays out of the way when it cannot mark an error handled', async function () { + this.timeout(30000); + const { code, stderr } = await runHarness('frozen-claim'); + assert.equal(code, 1, `expected the original error to stay fatal, got exit ${code}: ${stderr}`); + assert.match(stderr, /frozen lost watch/); + assert.doesNotMatch(stderr, /not extensible/); + }); }); }); diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 4fe7de7ca1..77d608cb7f 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -191,7 +191,18 @@ export function claimLostNativeWatchError(error: unknown): boolean { let lostNativeWatchGuardInstalled = false; function handleUncaughtException(error: unknown): void { - if (claimLostNativeWatchError(error)) return; + let claimed = false; + try { + claimed = claimLostNativeWatchError(error); + } catch { + // Classifying must never be the thing that kills the process. A throw from inside an + // 'uncaughtException' listener replaces Node's report with this one and exits 7, and this + // listener runs first, so it would also cost the thread-level handlers their turn. A frozen + // error (`isHandled` unassignable) or a throwing property getter lands here; "not ours" is + // the safe reading, leaving the original exception fatal exactly as it would have been. + claimed = false; + } + if (claimed) return; // Not ours. Node suppresses its default fatal handling as soon as *any* // 'uncaughtException' listener exists, so if this guard is the only listener // its mere presence would turn unrelated crashes into silent hangs. Step out From 566bb60458ef54b855bcdcc5e52d8109b3018f38 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sun, 30 Aug 2026 18:33:09 -0600 Subject: [PATCH 6/7] Draw the guard's boundary at classification, not at bookkeeping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last commit made the process guard survive an error it could not mark handled by treating the failure as "not ours", which was too blunt in one direction and too narrow in the other. Too blunt: the mark and the log lines are bookkeeping, and losing them is no reason to hand a failure we just classified as benign back to Node as fatal — worse, if the mark landed and only the log threw, the guard would step aside while threadServer/socketRouter still skipped the error as handled, so nothing reported it at all. Too narrow: the watcher `.on('error')` routes call claimLostNativeWatchError directly, outside that try/catch, so a frozen error reaching one of them still threw. Move the boundary into claimLostNativeWatchError. Classification decides whether the error is ours; counting, logging and marking follow inside a try/catch and cannot un-claim it, for the direct callers as much as for the guard. Marking goes last so a frozen error is still counted and logged. The guard's own try/catch now covers only classification — an error whose shape cannot even be read is not ours, and stays fatal. Tests follow the corrected split: the frozen child case now asserts survival with the claim counted, a new case pins that an error the guard cannot classify stays fatal without the guard's own throw replacing it, and a unit case covers the direct `.on('error')` route. Co-Authored-By: Claude Opus 5 --- .../utility/fixtures/lostWatchHarness.cjs | 28 +++++++-- unitTests/utility/watcherFallback.test.js | 35 ++++++++--- utility/watcherFallback.ts | 61 +++++++++++-------- 3 files changed, 85 insertions(+), 39 deletions(-) diff --git a/unitTests/utility/fixtures/lostWatchHarness.cjs b/unitTests/utility/fixtures/lostWatchHarness.cjs index ba4305213f..b0db382c58 100644 --- a/unitTests/utility/fixtures/lostWatchHarness.cjs +++ b/unitTests/utility/fixtures/lostWatchHarness.cjs @@ -76,15 +76,35 @@ watcher.on('ready', () => { } if (mode === 'frozen-claim') { - // A lost-watch-shaped error the guard cannot mark handled: `isHandled` is unassignable on a - // frozen object, so classifying it throws. That throw happens inside the guard's - // 'uncaughtException' listener, which runs first, so an unprotected guard would replace this - // error's report with a TypeError and exit 7 instead of leaving it fatal on its own terms. + // A lost-watch-shaped error the guard cannot mark handled, because `isHandled` is + // unassignable on a frozen object. Marking is bookkeeping, so the claim still stands and the + // process lives; an unprotected guard would instead throw from inside its own + // 'uncaughtException' listener, replacing the report with a TypeError and exiting 7. setTimeout(() => { throw Object.freeze( Object.assign(new Error('frozen lost watch'), { code: 'EPERM', syscall: 'watch', filename: null }) ); }, 10); + setTimeout(() => { + process.stdout.write(`survived lostWatchCount=${_lostNativeWatchCountForTests()}\n`); + process.exit(0); + }, 200); + return; + } + + if (mode === 'unclassifiable-throw') { + // Classification is the one step left that can throw: reading the shape runs this getter. + // An error the guard cannot classify is not its to claim, so it has to stay fatal rather + // than take the process down with a TypeError from inside the guard's own listener. + setTimeout(() => { + throw { + syscall: 'watch', + code: 'EPERM', + get path() { + throw new Error('probe getter'); + }, + }; + }, 10); return; } diff --git a/unitTests/utility/watcherFallback.test.js b/unitTests/utility/watcherFallback.test.js index 40a3304e07..0aef0fff78 100644 --- a/unitTests/utility/watcherFallback.test.js +++ b/unitTests/utility/watcherFallback.test.js @@ -132,6 +132,15 @@ describe('watcherFallback', () => { assert.equal(claimLostNativeWatchError(error), false); assert.equal(error.isHandled, undefined); }); + + // The watcher 'error' routes in EntryHandler, OptionsWatcher, RootConfigWatcher, keys.ts and + // manageThreads.js call this directly, outside the process guard's own try/catch, so marking + // an error it cannot mark has to be survivable here rather than only there. + it('claims an error it cannot mark handled instead of throwing', () => { + const error = Object.freeze(Object.assign(new Error('EPERM: watch'), { code: 'EPERM', syscall: 'watch' })); + assert.equal(claimLostNativeWatchError(error), true); + assert.equal(error.isHandled, undefined); + }); }); // chokidar never attaches an 'error' listener to the underlying Node @@ -204,17 +213,25 @@ describe('watcherFallback', () => { assert.match(stderr, /ENOENT/); }); - // The guard runs before every other uncaughtException listener, so a throw out of its own - // classification would cost the process both the original error's report (exit 7, with the - // guard's TypeError in its place) and the thread-level handlers' turn. A frozen error of - // exactly the claimed shape is the reachable version of that: reading it succeeds, marking - // it handled does not. - it('stays out of the way when it cannot mark an error handled', async function () { + // The guard runs before every other uncaughtException listener, so a throw out of the guard + // itself costs the process both the original error's report (exit 7, with the guard's own + // TypeError in its place) and the thread-level handlers' turn. These two pin the boundary + // either side of the classification: bookkeeping that fails must not un-claim a benign + // error, and a shape the guard cannot read is not its error to claim. + it('still claims a lost watch it cannot mark handled', async function () { this.timeout(30000); - const { code, stderr } = await runHarness('frozen-claim'); - assert.equal(code, 1, `expected the original error to stay fatal, got exit ${code}: ${stderr}`); - assert.match(stderr, /frozen lost watch/); + const { code, stdout, stderr } = await runHarness('frozen-claim'); + assert.equal(code, 0, `harness exited ${code}: ${stderr}`); + assert.match(stdout, /survived lostWatchCount=1/); assert.doesNotMatch(stderr, /not extensible/); }); + + it('leaves an error it cannot classify fatal', async function () { + this.timeout(30000); + const { code, stderr } = await runHarness('unclassifiable-throw'); + assert.equal(code, 1, `expected the original throw to stay fatal, got exit ${code}: ${stderr}`); + assert.match(stderr, /EPERM/); + assert.doesNotMatch(stderr, /probe getter/); // the guard's own throw, had it not caught + }); }); }); diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 77d608cb7f..8e2ff2f869 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -162,28 +162,37 @@ export function claimLostNativeWatchError(error: unknown): boolean { // Already claimed. Counting the same instance twice would inflate the tally and trip the // decade warning early, so report it as claimed and stop. if (claimed.isHandled) return true; - // server/threads/threadServer.js and server/threads/socketRouter.ts skip errors already - // marked handled, so the benign case doesn't also get logged there as an uncaughtException. - claimed.isHandled = true; lostNativeWatchCount++; - fallbackLogger.trace?.(`Lost native file watch handle (occurrence ${lostNativeWatchCount}):`, error); - // Never go fully silent. Node gives this error no path, so it cannot be attributed to the - // watcher that raised it — ours or a dependency's — and a subsystem that has quietly stopped - // being watched is exactly what an operator needs to see. The default log level is `warn`, so - // trace alone would make every occurrence after the first invisible. Warn on the first and - // then at each decade, which keeps a delete storm (one error per directory in a removed tree) - // bounded without ever suppressing the signal outright. - if (lostNativeWatchCount >= lostNativeWatchWarnThreshold) { - lostNativeWatchWarnThreshold *= 10; - fallbackLogger.warn?.( - `A native file watch handle failed asynchronously (EPERM, syscall=watch) and was suppressed to ` + - `keep the thread alive; occurrence ${lostNativeWatchCount}. Node reports no path for this error, ` + - `so it cannot be attributed to a specific watcher — whatever was watching that path has stopped ` + - `reporting changes until it is re-established. On Windows this is usually a watched directory ` + - `being deleted or replaced (a component redeploy, a package install, a test teardown). If file ` + - `changes stop being picked up somewhere, this is why. Subsequent occurrences log at trace, with ` + - `a warning at each tenfold increase.` - ); + // The claim is the classification above; everything from here is bookkeeping, and bookkeeping + // must not throw. This runs from a watcher's own 'error' route as well as from the process + // guard, so a frozen error (`isHandled` unassignable) or a logger that throws would otherwise + // turn a failure we just decided was benign into a fatal one. + try { + fallbackLogger.trace?.(`Lost native file watch handle (occurrence ${lostNativeWatchCount}):`, error); + // Never go fully silent. Node gives this error no path, so it cannot be attributed to the + // watcher that raised it — ours or a dependency's — and a subsystem that has quietly stopped + // being watched is exactly what an operator needs to see. The default log level is `warn`, so + // trace alone would make every occurrence after the first invisible. Warn on the first and + // then at each decade, which keeps a delete storm (one error per directory in a removed tree) + // bounded without ever suppressing the signal outright. + if (lostNativeWatchCount >= lostNativeWatchWarnThreshold) { + lostNativeWatchWarnThreshold *= 10; + fallbackLogger.warn?.( + `A native file watch handle failed asynchronously (EPERM, syscall=watch) and was suppressed to ` + + `keep the thread alive; occurrence ${lostNativeWatchCount}. Node reports no path for this error, ` + + `so it cannot be attributed to a specific watcher — whatever was watching that path has stopped ` + + `reporting changes until it is re-established. On Windows this is usually a watched directory ` + + `being deleted or replaced (a component redeploy, a package install, a test teardown). If file ` + + `changes stop being picked up somewhere, this is why. Subsequent occurrences log at trace, with ` + + `a warning at each tenfold increase.` + ); + } + // Last, because it is the only step whose failure costs nothing that has not already + // happened: server/threads/threadServer.js and server/threads/socketRouter.ts skip errors + // already marked handled, so this only keeps the benign case from being logged twice. + claimed.isHandled = true; + } catch { + // Marked or not, logged or not, the error stays claimed. } return true; } @@ -195,11 +204,11 @@ function handleUncaughtException(error: unknown): void { try { claimed = claimLostNativeWatchError(error); } catch { - // Classifying must never be the thing that kills the process. A throw from inside an - // 'uncaughtException' listener replaces Node's report with this one and exits 7, and this - // listener runs first, so it would also cost the thread-level handlers their turn. A frozen - // error (`isHandled` unassignable) or a throwing property getter lands here; "not ours" is - // the safe reading, leaving the original exception fatal exactly as it would have been. + // Only classification can still throw here (a property getter that does), and it must not + // be what kills the process: a throw from inside an 'uncaughtException' listener replaces + // Node's report with this one and exits 7, and this listener runs first, so it would also + // cost the thread-level handlers their turn. An error we could not classify is not ours, + // and stays fatal exactly as it would have been unguarded. claimed = false; } if (claimed) return; From edbacf047b0a25b302cd6ffca57e920aa1d18a35 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sun, 30 Aug 2026 18:41:07 -0600 Subject: [PATCH 7/7] Cut the guard's comments back to what the code can't say Three review lenses flagged the comment volume against the house default of none-unless-it-carries-a-why. The chokidar header keeps the mechanism (why a process-level listener rather than watcher.on('error')) and loses the source quote and the history; the rest lose their narration and keep one line each. DESIGN.md said the guard covers `EPERM`/`ENOENT`; ENOENT was dropped from the claimed set two commits ago and the doc did not follow. Co-Authored-By: Claude Opus 5 --- DESIGN.md | 2 +- .../utility/fixtures/lostWatchHarness.cjs | 19 ++- unitTests/utility/watcherFallback.test.js | 12 +- utility/watcherFallback.ts | 110 ++++++------------ 4 files changed, 48 insertions(+), 95 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 5e2e2d7921..71aa64dd36 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1245,7 +1245,7 @@ is stat polling with no fs-event handle and is outside this invariant. Five of those six sites arm the watch through `guardedWatch()` (`utility/watcherFallback.ts`) rather than calling `chokidar.watch`/`fs.watch` directly — it installs a process-level guard for a second, unrelated failure (a watched path deleted out from under a non-persistent chokidar watcher raises an -unhandled `EPERM`/`ENOENT`; see that file's header comment) but does no canonicalization of its own. +unhandled async `EPERM`; see that file's header comment) but does no canonicalization of its own. Every caller still resolves its own path first and passes the resolved path in, exactly as when they called `chokidar.watch` directly, so the invariant holds through the wrapper. `utility/watcherFallback.ts` itself is the one file that touches `chokidar` without canonicalizing — the watch-sites source scan diff --git a/unitTests/utility/fixtures/lostWatchHarness.cjs b/unitTests/utility/fixtures/lostWatchHarness.cjs index b0db382c58..0c49950025 100644 --- a/unitTests/utility/fixtures/lostWatchHarness.cjs +++ b/unitTests/utility/fixtures/lostWatchHarness.cjs @@ -17,10 +17,9 @@ const { const mode = process.argv[2]; -// How long to wait for an asynchronous watch failure. On Windows it is the error itself we are -// waiting for, and delivery competes with everything else on a loaded CI runner; elsewhere nothing -// will ever arrive and this is only a settle period, so it stays short. Cases that can observe the -// error poll for it and exit early, so the deadline is a ceiling, not a sleep. +// A ceiling, not a sleep: observers poll and exit early. Windows is where the error actually +// arrives, and delivery competes with everything else on a loaded runner; elsewhere nothing ever +// arrives and this is only a settle period. const DELIVERY_DEADLINE_MS = process.platform === 'win32' ? 5000 : 1500; function waitForDelivery(isDelivered, onDeadline) { @@ -76,10 +75,9 @@ watcher.on('ready', () => { } if (mode === 'frozen-claim') { - // A lost-watch-shaped error the guard cannot mark handled, because `isHandled` is - // unassignable on a frozen object. Marking is bookkeeping, so the claim still stands and the - // process lives; an unprotected guard would instead throw from inside its own - // 'uncaughtException' listener, replacing the report with a TypeError and exiting 7. + // `isHandled` is unassignable on a frozen error, and marking is only bookkeeping, so the claim + // stands and the process lives. An unprotected guard would throw from inside its own + // 'uncaughtException' listener instead, replacing the report with a TypeError and exiting 7. setTimeout(() => { throw Object.freeze( Object.assign(new Error('frozen lost watch'), { code: 'EPERM', syscall: 'watch', filename: null }) @@ -93,9 +91,8 @@ watcher.on('ready', () => { } if (mode === 'unclassifiable-throw') { - // Classification is the one step left that can throw: reading the shape runs this getter. - // An error the guard cannot classify is not its to claim, so it has to stay fatal rather - // than take the process down with a TypeError from inside the guard's own listener. + // Reading the shape runs this getter, which is the one step left that can throw. An error the + // guard cannot classify is not its to claim and has to stay fatal on its own terms. setTimeout(() => { throw { syscall: 'watch', diff --git a/unitTests/utility/watcherFallback.test.js b/unitTests/utility/watcherFallback.test.js index 0aef0fff78..93ab30116d 100644 --- a/unitTests/utility/watcherFallback.test.js +++ b/unitTests/utility/watcherFallback.test.js @@ -133,9 +133,7 @@ describe('watcherFallback', () => { assert.equal(error.isHandled, undefined); }); - // The watcher 'error' routes in EntryHandler, OptionsWatcher, RootConfigWatcher, keys.ts and - // manageThreads.js call this directly, outside the process guard's own try/catch, so marking - // an error it cannot mark has to be survivable here rather than only there. + // The five watcher 'error' routes call this directly, outside the process guard's try/catch. it('claims an error it cannot mark handled instead of throwing', () => { const error = Object.freeze(Object.assign(new Error('EPERM: watch'), { code: 'EPERM', syscall: 'watch' })); assert.equal(claimLostNativeWatchError(error), true); @@ -213,11 +211,9 @@ describe('watcherFallback', () => { assert.match(stderr, /ENOENT/); }); - // The guard runs before every other uncaughtException listener, so a throw out of the guard - // itself costs the process both the original error's report (exit 7, with the guard's own - // TypeError in its place) and the thread-level handlers' turn. These two pin the boundary - // either side of the classification: bookkeeping that fails must not un-claim a benign - // error, and a shape the guard cannot read is not its error to claim. + // The two sides of the boundary: bookkeeping that fails must not un-claim a benign error, and + // a shape the guard cannot read is not its error to claim. A throw out of the guard itself + // would cost the process the original report (exit 7) and the thread-level handlers' turn. it('still claims a lost watch it cannot mark handled', async function () { this.timeout(30000); const { code, stdout, stderr } = await runHarness('frozen-claim'); diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 8e2ff2f869..3639c1c788 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -85,60 +85,34 @@ export function _resetForTests(): void { // Lost native watch (predominantly Windows) // --------------------------------------------------------------------------- // -// Every Harper watcher runs chokidar with `persistent: false` so a watcher never -// holds the event loop open. That option takes chokidar down a code path that -// never attaches an 'error' listener to the underlying Node `fs.FSWatcher` -// (chokidar `handler.js`, `setFsWatchListener`): -// -// if (!options.persistent) { -// watcher = createFsWatchInstance(path, options, listener, errHandler, rawEmitter); -// if (!watcher) return; -// return watcher.close.bind(watcher); // <-- no watcher.on('error', ...) -// } -// -// `errHandler` is only consulted for a *synchronous* throw out of `fs.watch()` -// (which is how ENOSPC/EMFILE reach `isWatcherExhaustionError` above). An -// *asynchronous* watch failure — on Windows, deleting or replacing the watched -// directory — is delivered by `node:internal/fs/watchers` as -// `this.emit('error', err)` on an emitter with no listener, so Node turns it -// into an uncaughtException. The error never reaches chokidar's wrapper, so -// attaching `.on('error')` to the chokidar `FSWatcher` we hold does not help. -// -// chokidar's `persistent: true` branch does attach a listener and swallows this -// exact error (its node#4337 workaround), which is why only Harper's watchers -// see it. The upstream fix is one line — `watcher.on('error', errHandler)` in -// the non-persistent branch — and is still missing as of chokidar 5.0.0. -// -// Until then `installLostNativeWatchGuard()` supplies the missing listener at -// the only place the error is observable: the process. It claims *only* this -// error shape and leaves every other uncaught exception exactly as Node would -// have handled it. +// Every Harper watcher runs `persistent: false`, which is the one chokidar +// branch that never attaches an 'error' listener to the underlying Node +// `fs.FSWatcher` (chokidar `handler.js`, `setFsWatchListener`; still so at +// 5.0.0). Its `errHandler` covers only a synchronous throw out of `fs.watch()` +// — the ENOSPC/EMFILE route above. An asynchronous failure is emitted on a +// handle with no listener, so Node makes it an uncaughtException without it +// ever reaching chokidar; `.on('error')` on the wrapper we hold cannot see it. +// The process is therefore the only place it is observable, which is what +// `installLostNativeWatchGuard()` uses. /** * Returns `true` for the asynchronous "the watched path went away" error raised - * by Node's `fs.FSWatcher`: `EPERM: operation not permitted, watch` (errno - * -4048), which fires on Windows whenever a watched directory is removed or - * swapped out — a component redeploy, a test fixture teardown, an `npm install` - * that replaces a tree. - * - * Deliberately distinct from {@link isWatcherExhaustionError}: exhaustion means - * the host ran out of watch capacity and polling is a useful degradation; a lost - * native watch means the thing being watched no longer exists, and polling would - * only burn CPU on a path that isn't there. + * by Node's `fs.FSWatcher` — on Windows, `EPERM: operation not permitted, watch` + * (errno -4048) when a watched directory is removed or swapped out. Unlike + * {@link isWatcherExhaustionError} there is nothing to degrade to: polling a + * path that no longer exists only burns CPU. * - * The three conditions are all load-bearing, because whatever this claims is + * All three conditions are load-bearing, because whatever this claims is * swallowed process-wide: * * - `syscall === 'watch'` is only ever set by fs watch handles. * - `EPERM` only. 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, which must stay fatal. - * - no `path`. This is what 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 (which - * carries `filename: null` and nothing else locating it). Without this a - * raw `fs.watch(missingPath)` escaping into an uncaughtException would be - * mistaken for a benign lost watch and silently swallowed. + * - no `path`. Node populates `path` on the error it throws out of + * `fs.watch()` and leaves it absent on the one it delivers to the handle, + * so without this a raw `fs.watch(missingPath)` reaching the guard would be + * mistaken for a benign lost watch and swallowed. */ export function isLostNativeWatchError(error: unknown): boolean { if (typeof error !== 'object' || error === null) return false; @@ -147,7 +121,6 @@ export function isLostNativeWatchError(error: unknown): boolean { } let lostNativeWatchCount = 0; -// Warn on occurrence 1, then 10, 100, … — see claimLostNativeWatchError(). let lostNativeWatchWarnThreshold = 1; /** @@ -159,22 +132,17 @@ let lostNativeWatchWarnThreshold = 1; export function claimLostNativeWatchError(error: unknown): boolean { if (!isLostNativeWatchError(error)) return false; const claimed = error as { isHandled?: boolean }; - // Already claimed. Counting the same instance twice would inflate the tally and trip the - // decade warning early, so report it as claimed and stop. + // Counting one instance twice would trip the warn threshold early. if (claimed.isHandled) return true; lostNativeWatchCount++; - // The claim is the classification above; everything from here is bookkeeping, and bookkeeping - // must not throw. This runs from a watcher's own 'error' route as well as from the process - // guard, so a frozen error (`isHandled` unassignable) or a logger that throws would otherwise - // turn a failure we just decided was benign into a fatal one. + // The claim is the classification above; the bookkeeping that follows must not be able to + // undo it. A frozen error (`isHandled` unassignable) or a throwing logger would otherwise + // make a failure just classified as benign fatal — here, or in a caller's 'error' route. try { fallbackLogger.trace?.(`Lost native file watch handle (occurrence ${lostNativeWatchCount}):`, error); - // Never go fully silent. Node gives this error no path, so it cannot be attributed to the - // watcher that raised it — ours or a dependency's — and a subsystem that has quietly stopped - // being watched is exactly what an operator needs to see. The default log level is `warn`, so - // trace alone would make every occurrence after the first invisible. Warn on the first and - // then at each decade, which keeps a delete storm (one error per directory in a removed tree) - // bounded without ever suppressing the signal outright. + // A subsystem that has silently stopped being watched is what an operator needs to see, and + // the default level is `warn`, so trace alone would hide every occurrence after the first. + // Warning at each decade keeps a delete storm bounded without ever going fully silent. if (lostNativeWatchCount >= lostNativeWatchWarnThreshold) { lostNativeWatchWarnThreshold *= 10; fallbackLogger.warn?.( @@ -187,9 +155,8 @@ export function claimLostNativeWatchError(error: unknown): boolean { `a warning at each tenfold increase.` ); } - // Last, because it is the only step whose failure costs nothing that has not already - // happened: server/threads/threadServer.js and server/threads/socketRouter.ts skip errors - // already marked handled, so this only keeps the benign case from being logged twice. + // Last: threadServer.js and socketRouter.ts skip errors already marked handled, so failing + // to mark one costs only a duplicate log line. claimed.isHandled = true; } catch { // Marked or not, logged or not, the error stays claimed. @@ -204,27 +171,20 @@ function handleUncaughtException(error: unknown): void { try { claimed = claimLostNativeWatchError(error); } catch { - // Only classification can still throw here (a property getter that does), and it must not - // be what kills the process: a throw from inside an 'uncaughtException' listener replaces - // Node's report with this one and exits 7, and this listener runs first, so it would also - // cost the thread-level handlers their turn. An error we could not classify is not ours, - // and stays fatal exactly as it would have been unguarded. + // Classification is all that can still throw (a property getter that does), and a throw + // from inside an 'uncaughtException' listener would replace Node's report with it and + // exit 7. An error that cannot be classified is not ours to claim. claimed = false; } if (claimed) return; - // Not ours. Node suppresses its default fatal handling as soon as *any* - // 'uncaughtException' listener exists, so if this guard is the only listener - // its mere presence would turn unrelated crashes into silent hangs. Step out - // of the way and let the exception be fatal exactly as it would have been. + // Node suppresses its default fatal handling as soon as *any* 'uncaughtException' listener + // exists, so being the only listener would turn unrelated crashes into silent hangs. if (process.listenerCount('uncaughtException') > 1) return; process.removeListener('uncaughtException', handleUncaughtException); - // Stepping aside is meant to be terminal, but say so in the state rather than assuming it: - // clearing the flag means a process that somehow survives re-arms on its next guardedWatch() - // instead of running unguarded for the rest of its life. + // A process that somehow survives re-arms on its next guardedWatch() rather than running + // unguarded thereafter. lostNativeWatchGuardInstalled = false; - // Re-raising on the next tick (rather than throwing from inside the handler) - // keeps Node's normal report and exit code 1; a throw from within the handler - // exits 7 instead. + // nextTick, not a throw from inside the handler: that keeps Node's report and exit code 1. process.nextTick(() => { throw error; });