diff --git a/DESIGN.md b/DESIGN.md index ecc83e5cc0..71aa64dd36 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 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 +(`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..0c49950025 --- /dev/null +++ b/unitTests/utility/fixtures/lostWatchHarness.cjs @@ -0,0 +1,137 @@ +'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 { + claimLostNativeWatchError, + guardedWatch, + _lostNativeWatchCountForTests, +} = require('#src/utility/watcherFallback'); + +const mode = process.argv[2]; + +// 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) { + 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. + 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 }); +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 === '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); + }, DELIVERY_DEADLINE_MS); + return; + } + + if (mode === 'frozen-claim') { + // `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 }) + ); + }, 10); + setTimeout(() => { + process.stdout.write(`survived lostWatchCount=${_lostNativeWatchCountForTests()}\n`); + process.exit(0); + }, 200); + return; + } + + if (mode === 'unclassifiable-throw') { + // 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', + code: 'EPERM', + get path() { + throw new Error('probe getter'); + }, + }; + }, 10); + 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 + // 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. + 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 }); + waitForDelivery( + () => _lostNativeWatchCountForTests() > 0, + () => { + process.stdout.write(`survived lostWatchCount=${_lostNativeWatchCountForTests()}\n`); + process.exit(0); + } + ); +}); 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..93ab30116d 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,173 @@ describe('watcherFallback', () => { assert.doesNotThrow(() => warnWatcherFallback('/some/other/path')); }); }); + + describe('isLostNativeWatchError', () => { + // 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); + }); + + // 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 + // 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); + }); + + // 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); + 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/); + }); + + // 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); + 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/); + }); + + 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. + 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/); + }); + + // 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'); + 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 01daf64eb8..3639c1c788 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,153 @@ 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; + 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; +} + +// --------------------------------------------------------------------------- +// Lost native watch (predominantly Windows) +// --------------------------------------------------------------------------- +// +// 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` — 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. + * + * 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`. 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; + const { code, syscall, path } = error as { code?: unknown; syscall?: unknown; path?: unknown }; + return syscall === 'watch' && code === 'EPERM' && path == null; +} + +let lostNativeWatchCount = 0; +let lostNativeWatchWarnThreshold = 1; + +/** + * 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; + const claimed = error as { isHandled?: boolean }; + // Counting one instance twice would trip the warn threshold early. + if (claimed.isHandled) return true; + lostNativeWatchCount++; + // 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); + // 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?.( + `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: 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. + } + return true; +} + +let lostNativeWatchGuardInstalled = false; + +function handleUncaughtException(error: unknown): void { + let claimed = false; + try { + claimed = claimLostNativeWatchError(error); + } catch { + // 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; + // 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); + // A process that somehow survives re-arms on its next guardedWatch() rather than running + // unguarded thereafter. + lostNativeWatchGuardInstalled = false; + // nextTick, not a throw from inside the handler: that keeps Node's report and exit code 1. + 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(); + // 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. +export function _lostNativeWatchCountForTests(): number { + return lostNativeWatchCount; } /**