From 112910355bf792e86f05fd812c45e44e4aca72fe Mon Sep 17 00:00:00 2001 From: Joachim Stokke Date: Fri, 11 Sep 2026 10:42:59 +0200 Subject: [PATCH] fix(watch): bound config file watching and handle watcher errors - Prevent unbounded ancestor climbing and recursive directory watching on Linux, which exhausted system inotify limits (ENOSPC) over large trees like ~/.pi/agent/npm/node_modules or repo roots. - Bounded ancestor watching strictly to parentDir non-recursively, with dynamic promotion to configDir and configPath upon creation. - Cleanly detach parentWatcher when configDir is created to prevent leaked handles and spurious reload events on parent metadata changes. - Wrap all FSWatcher registrations with safeWatch to immediately attach error listeners, preventing unhandled error events from crashing the host process with uncaughtException. - Add lifecycle stopped guard in watchConfigPaths to prevent timers from firing after teardown. - Add regression tests covering error resilience (ENOSPC/ENOENT), non-recursive parent boundaries, delayed intermediate directory creation, parentWatcher detachment, and directory deletion. --- config-loader.ts | 15 ++- config-watcher.ts | 245 +++++++++++++++++++++++++++--------- tests/config-loader.test.ts | 171 ++++++++++++++++++++++++- 3 files changed, 370 insertions(+), 61 deletions(-) diff --git a/config-loader.ts b/config-loader.ts index 5fb4e7a..0c52a8b 100644 --- a/config-loader.ts +++ b/config-loader.ts @@ -8,7 +8,7 @@ */ import { chmod, link, mkdir, readFile, rename, stat, unlink, writeFile } from "node:fs/promises"; -import { existsSync } from "node:fs"; +import { existsSync, type FSWatcher } from "node:fs"; import { join, dirname } from "node:path"; import { homedir } from "node:os"; import { randomUUID } from "node:crypto"; @@ -1511,11 +1511,20 @@ export async function saveConfigRuleMutations( // ─── File watching (hot reload) ──────────────────────────────────────────── +export interface WatchConfigHooks { + /** Test seam called whenever an FSWatcher is registered. */ + onWatcher?: (watcher: FSWatcher, target: string) => void; +} + /** * Watches both global and project-level config files, debounced 300ms * before calling onChange. Returns a stop() function to call on * session_shutdown. */ -export function watchConfigs(cwd: string, onChange: () => void): () => void { - return watchConfigPaths(GLOBAL_CONFIG_PATH, getProjectConfigPath(cwd), onChange); +export function watchConfigs( + cwd: string, + onChange: () => void, + hooks?: WatchConfigHooks, +): () => void { + return watchConfigPaths(GLOBAL_CONFIG_PATH, getProjectConfigPath(cwd), onChange, hooks); } diff --git a/config-watcher.ts b/config-watcher.ts index 2864dd1..a4ea753 100644 --- a/config-watcher.ts +++ b/config-watcher.ts @@ -1,34 +1,104 @@ /** * config-watcher.ts - * Config hot-reload file watching: layered fs.watch fallbacks (direct file, - * parent directory, nearest existing ancestor, recursive when supported) - * plus a low-cost polling safety net for inotify creation races and editor - * atomic replaces, debounced before notifying. + * Config hot-reload file watching: strictly non-recursive, bounded fs.watch + * fallbacks (direct file, config directory, immediate parent) plus a + * low-cost polling safety net for inotify creation races and editor atomic + * replaces, debounced before notifying. + * + * All watchers are attached with an 'error' listener immediately so + * ENOSPC/ENOENT/EPERM never crash the host process via an unhandled + * exception, and the watcher tree never climbs beyond the immediate parent + * of the config directory (preventing inotify exhaustion over large trees + * like node_modules or sessions/). */ import { watch, existsSync, statSync, type FSWatcher } from "node:fs"; import { join, dirname, basename } from "node:path"; +/** Test seam passed through from config-loader. */ +export interface WatchConfigHooks { + /** Called whenever an FSWatcher is registered. */ + onWatcher?: (watcher: FSWatcher, target: string) => void; +} + /** - * Watch a config file so later creation or edits trigger a reload: - * - if the file exists, watch it directly (catches edits); - * - if its parent directory exists, watch the directory (catches creation - * and editor-style replace-and-rename), filtered to the file name; - * - otherwise watch the nearest existing ancestor directory with - * recursive:true when supported (Windows/macOS), falling back to a - * non-recursive watch. + * Safely watch a filesystem target without recursion. Attaches an 'error' + * listener immediately so ENOSPC/ENOENT/EPERM never crash the Node process + * via an unhandled exception. + */ +function safeWatch( + target: string, + listener: (event: string, filename: string | null) => void, + watchers: FSWatcher[], + hooks?: WatchConfigHooks, + onError?: (watcher: FSWatcher) => void, +): FSWatcher | null { + try { + const watcher = watch(target, listener); + watcher.on("error", () => { + try { + watcher.close(); + } catch { + // Ignore close failures + } + const idx = watchers.indexOf(watcher); + if (idx !== -1) { + watchers.splice(idx, 1); + } + onError?.(watcher); + }); + watchers.push(watcher); + hooks?.onWatcher?.(watcher, target); + return watcher; + } catch { + return null; + } +} + +/** Detach a watcher, ignoring close failures. */ +function detachWatcher(watcher: FSWatcher | null, watchers: FSWatcher[]): void { + if (!watcher) return; + try { + watcher.close(); + } catch { + // Ignore close failures + } + const idx = watchers.indexOf(watcher); + if (idx !== -1) watchers.splice(idx, 1); +} + +/** + * Watch a config file so later creation or edits trigger a reload without + * exhausting system inotify handles: + * - if the file exists, watch it directly (catches in-place edits); + * - if its container directory exists, watch it non-recursively (catches + * creation and editor-style replace-and-rename); + * - if the container directory does not exist yet, watch at most its + * immediate parent (e.g. ~/.pi/agent or /.pi) non-recursively. + * + * Neither recursive watching nor climbing beyond the immediate parent is used, + * preventing inotify table exhaustion over large trees like node_modules or sessions. + * Returns a refresh function to promote watchers when newly created files/directories + * are detected. */ function watchConfigFile( configPath: string, handleChange: () => void, - watchers: FSWatcher[] -): void { + watchers: FSWatcher[], + hooks?: WatchConfigHooks, +): () => void { const configDir = dirname(configPath); + const parentDir = dirname(configDir); const fileName = basename(configPath); - const expectedSuffix = join("pi-data-masking", fileName).split("\\").join("/"); + const dirName = basename(configDir); + const expectedSuffix = join(dirName, fileName).split("\\").join("/"); + + let fileWatcher: FSWatcher | null = null; + let dirWatcher: FSWatcher | null = null; + let parentWatcher: FSWatcher | null = null; - function matches(filename: unknown): boolean { - if (filename === null || filename === undefined) return false; + function matchesFile(filename: unknown): boolean { + if (filename === null || filename === undefined) return true; const normalized = String(filename).split("\\").join("/"); return ( normalized === fileName || @@ -37,66 +107,116 @@ function watchConfigFile( ); } - // 1. Watch the file itself when it already exists (covers in-place edits). - if (existsSync(configPath)) { - try { - watchers.push(watch(configPath, () => handleChange())); - } catch { - // ignore — the directory watcher below still covers most cases - } + function matchesDir(filename: unknown): boolean { + if (filename === null || filename === undefined) return true; + const normalized = String(filename).split("\\").join("/"); + return ( + normalized === dirName || + normalized === expectedSuffix || + normalized.endsWith("/" + dirName) || + normalized.endsWith("/" + expectedSuffix) + ); } - // 2. Watch the direct parent directory when it exists (covers creation). - if (existsSync(configDir)) { - try { - watchers.push(watch(configDir, (_event, filename) => { - if (matches(filename)) handleChange(); - })); - return; - } catch { - // ignore — fall through to the ancestor watch - } + function attachFileWatcher(): void { + if (fileWatcher || !existsSync(configPath)) return; + fileWatcher = safeWatch( + configPath, + (event) => { + if (event === "rename") { + reattachFileWatcher(); + } + handleChange(); + }, + watchers, + hooks, + (w) => { + if (fileWatcher === w) fileWatcher = null; + }, + ); } - // 3. Nearest existing ancestor (covers the whole directory chain being - // created after session start). Prefer recursive where supported. - let target = configDir; - while (!existsSync(target)) { - const parent = dirname(target); - if (parent === target) return; // filesystem root; nothing to watch - target = parent; + function reattachFileWatcher(): void { + detachWatcher(fileWatcher, watchers); + fileWatcher = null; + attachFileWatcher(); } - try { - const watcher = watch(target, { recursive: true }, (_event, filename) => { - if (matches(filename)) handleChange(); - }); - watchers.push(watcher); - } catch { - try { - watchers.push(watch(target, (_event, filename) => { - if (matches(filename)) handleChange(); - })); - } catch { - // Silently ignore if watching is unsupported for this directory + + function attachDirWatcher(): void { + if (dirWatcher || !existsSync(configDir)) return; + dirWatcher = safeWatch( + configDir, + (_event, filename) => { + if (matchesFile(filename)) { + reattachFileWatcher(); + handleChange(); + } + }, + watchers, + hooks, + (w) => { + if (dirWatcher === w) dirWatcher = null; + }, + ); + } + + function attachParentWatcher(): void { + if (parentWatcher || dirWatcher || !existsSync(parentDir) || parentDir === configDir) return; + parentWatcher = safeWatch( + parentDir, + (_event, filename) => { + if (matchesDir(filename)) { + if (existsSync(configDir)) { + attachDirWatcher(); + detachWatcher(parentWatcher, watchers); + parentWatcher = null; + if (existsSync(configPath)) { + attachFileWatcher(); + handleChange(); + } + } + } + }, + watchers, + hooks, + (w) => { + if (parentWatcher === w) parentWatcher = null; + }, + ); + } + + function refresh(): void { + reattachFileWatcher(); + attachDirWatcher(); + if (!dirWatcher) { + attachParentWatcher(); } } + + refresh(); + return refresh; } export function watchConfigPaths( globalPath: string, projectPath: string, - onChange: () => void + onChange: () => void, + hooks?: WatchConfigHooks, ): () => void { + let stopped = false; const watchers: FSWatcher[] = []; let timer: ReturnType | null = null; function handleChange() { + if (stopped) return; if (timer) clearTimeout(timer); - timer = setTimeout(onChange, 300); + timer = setTimeout(() => { + if (!stopped) onChange(); + }, 300); } - watchConfigFile(globalPath, handleChange, watchers); - watchConfigFile(projectPath, handleChange, watchers); + const refreshGlobal = watchConfigFile(globalPath, handleChange, watchers, hooks); + const refreshProject = watchConfigFile(projectPath, handleChange, watchers, hooks); // fs.watch can miss a file created immediately after a watcher is // registered (notably on Linux/inotify). Polling the two small config files @@ -113,18 +233,29 @@ export function watchConfigPaths( let globalSignature = fileSignature(globalPath); let projectSignature = fileSignature(projectPath); const poller = setInterval(() => { + if (stopped) return; const nextGlobalSignature = fileSignature(globalPath); const nextProjectSignature = fileSignature(projectPath); if (nextGlobalSignature !== globalSignature || nextProjectSignature !== projectSignature) { globalSignature = nextGlobalSignature; projectSignature = nextProjectSignature; + refreshGlobal(); + refreshProject(); handleChange(); } }, 250); return () => { + stopped = true; if (timer) clearTimeout(timer); clearInterval(poller); - watchers.forEach((w) => w.close()); + for (const watcher of [...watchers]) { + try { + watcher.close(); + } catch { + // Ignore close failures + } + } + watchers.length = 0; }; } diff --git a/tests/config-loader.test.ts b/tests/config-loader.test.ts index cf10efb..2029a3d 100644 --- a/tests/config-loader.test.ts +++ b/tests/config-loader.test.ts @@ -799,7 +799,48 @@ test("allowlist: global entries only; project allowlist is ignored", async () => })); const { config, warnings } = await loadConfigFromPaths(globalPath, projectPath, KEY); assert.deepEqual(config.options.allowlist, [{ text: "10.0.0.1" }, { text: "10.0.0.2" }]); - assert.ok(warnings.some((w) => w.includes('project-level options are ignored (allowlist'))); + assert.ok(warnings.some((w) => w.includes('project-level options are ignored (allowlist'))); } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); +test("watchConfigPaths attaches error handlers to all FSWatchers and survives ENOSPC / ENOENT error events", () => { + const dir = makeTmp(); + try { + const configDir = join(dir, ".pi", "pi-data-masking"); + const projectPath = join(configDir, "masking.config.json"); + const globalPath = join(dir, "global.json"); + mkdirSync(configDir, { recursive: true }); + writeFileSync(projectPath, JSON.stringify({ rules: [] })); + + const capturedWatchers: Array<{ watcher: any; target: string }> = []; + let stop: (() => void) | undefined; + try { + stop = watchConfigPaths(globalPath, projectPath, () => {}, { + onWatcher: (watcher, target) => { + capturedWatchers.push({ watcher, target }); + }, + }); + + assert.ok(capturedWatchers.length >= 1, "should register watchers"); + + for (const { watcher } of capturedWatchers) { + assert.ok(watcher.listenerCount("error") >= 1, "watcher must have an error listener"); + // Simulating ENOSPC / ENOENT error event must NOT throw uncaughtException + assert.doesNotThrow(() => { + watcher.emit("error", new Error("ENOSPC: System limit for number of file watchers reached, watch")); + }); + assert.doesNotThrow(() => { + watcher.emit("error", new Error("ENOENT: no such file or directory, watch")); + }); + } + + // Stop must succeed cleanly even when watchers already emitted errors and were closed + assert.doesNotThrow(() => { + stop?.(); + }); + } finally { + stop?.(); + } } finally { rmSync(dir, { recursive: true, force: true }); } @@ -815,7 +856,42 @@ test("allowlist: absent from both files defaults to an empty list", async () => const { config, warnings } = await loadConfigFromPaths(globalPath, projectPath, KEY); assert.deepEqual(config.options.allowlist, []); assert.equal(warnings.some((w) => w.includes("allowlist")), false); + } finally { rmSync(dir, { recursive: true, force: true }); + } +}); +test("watchConfigFile watches parentDir non-recursively when configDir does not exist yet and ignores sibling churn", async () => { + const dir = makeTmp(); + let stop: (() => void) | undefined; + try { + const parentDir = join(dir, "agent"); + mkdirSync(parentDir, { recursive: true }); + // Sibling directory with deep subdirectories (simulates sessions / node_modules) + const sessionsDir = join(parentDir, "sessions", "session-1", "subagent-artifacts"); + mkdirSync(sessionsDir, { recursive: true }); + + const globalPath = join(parentDir, "pi-data-masking", "masking.config.json"); + const projectPath = join(dir, "project.json"); + + let fired = 0; + stop = watchConfigPaths(globalPath, projectPath, () => { fired++; }); + + // Writing to sibling sessions/ should NOT trigger masking reload + writeFileSync(join(sessionsDir, "artifact.txt"), "subagent artifact data"); + await new Promise((r) => setTimeout(r, 350)); + assert.equal(fired, 0, "churn in sibling sessions directory must not trigger config reload"); + + // Creating pi-data-masking and masking.config.json SHOULD trigger reload + const configDir = join(parentDir, "pi-data-masking"); + mkdirSync(configDir, { recursive: true }); + writeFileSync(globalPath, JSON.stringify({ rules: [] })); + + const deadline = Date.now() + 2000; + while (fired === 0 && Date.now() < deadline) { + await new Promise((r) => setTimeout(r, 50)); + } + assert.ok(fired >= 1, "onChange should fire when intermediate configDir and file are created"); } finally { + stop?.(); rmSync(dir, { recursive: true, force: true }); } }); @@ -833,7 +909,40 @@ test("allowlist: invalid entries are dropped with warnings, never fatal", async const { config, warnings } = await loadConfigFromPaths(globalPath, projectPath, KEY); assert.deepEqual(config.options.allowlist, [{ text: "ok" }, { text: "obj" }]); assert.ok(warnings.some((w) => w.includes("invalid or empty entry"))); + } finally { rmSync(dir, { recursive: true, force: true }); + } +}); +test("parentWatcher is cleanly detached when configDir is created and dirWatcher is attached", async () => { + const dir = makeTmp(); + let stop: (() => void) | undefined; + try { + const parentDir = join(dir, "agent"); + mkdirSync(parentDir, { recursive: true }); + const globalPath = join(parentDir, "pi-data-masking", "masking.config.json"); + const projectPath = join(dir, "project.json"); + + const targets: string[] = []; + stop = watchConfigPaths(globalPath, projectPath, () => {}, { + onWatcher: (_w, target) => { + targets.push(target); + }, + }); + + assert.ok(targets.includes(parentDir), "parentDir should be watched initially"); + + // Now create configDir + const configDir = join(parentDir, "pi-data-masking"); + mkdirSync(configDir, { recursive: true }); + writeFileSync(globalPath, JSON.stringify({ rules: [] })); + + const deadline = Date.now() + 2000; + while (!targets.includes(configDir) && Date.now() < deadline) { + await new Promise((r) => setTimeout(r, 50)); + } + + assert.ok(targets.includes(configDir), "configDir watcher should be attached upon creation"); } finally { + stop?.(); rmSync(dir, { recursive: true, force: true }); } }); @@ -853,7 +962,67 @@ test("allowlist: loaded entries exempt matching values end to end", async () => const masked = masker.mask("host 10.0.0.5 and 10.0.0.6", { discover: true }); assert.equal(masked.text.includes("10.0.0.5"), true); assert.equal(masked.text.includes("10.0.0.6"), false); + } finally { rmSync(dir, { recursive: true, force: true }); + } +}); +test("watchConfigPaths does not climb beyond immediate parent and detects config when intermediate directories are created later", async () => { + const dir = makeTmp(); + let stop: (() => void) | undefined; + try { + // Both configDir (pi-data-masking) and parentDir (.pi) do not exist yet + const projectPath = join(dir, ".pi", "pi-data-masking", "masking.config.json"); + const globalPath = join(dir, "missing-global", "masking.config.json"); + + let fired = 0; + stop = watchConfigPaths(globalPath, projectPath, () => { fired++; }); + + // Create the intermediate directory hierarchy and config file after watching started + mkdirSync(join(dir, ".pi", "pi-data-masking"), { recursive: true }); + writeFileSync(projectPath, JSON.stringify({ rules: [] })); + + const deadline = Date.now() + 2000; + while (fired === 0 && Date.now() < deadline) { + await new Promise((r) => setTimeout(r, 50)); + } + assert.ok(fired >= 1, "poller and promoted watchers should detect config creation"); + + // Subsequent in-place edit should also be detected + const beforeEdit = fired; + writeFileSync(projectPath, JSON.stringify({ rules: [{ id: "test", real: "secret" }] })); + const editDeadline = Date.now() + 2000; + while (fired === beforeEdit && Date.now() < editDeadline) { + await new Promise((r) => setTimeout(r, 50)); + } + assert.ok(fired > beforeEdit, "subsequent edit after creation should trigger reload"); + } finally { + stop?.(); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("watchConfigPaths survives watched directory deletion without throwing uncaught exceptions", async () => { + const dir = makeTmp(); + let stop: (() => void) | undefined; + try { + const configDir = join(dir, ".pi", "pi-data-masking"); + const projectPath = join(configDir, "masking.config.json"); + const globalPath = join(dir, "global.json"); + mkdirSync(configDir, { recursive: true }); + writeFileSync(projectPath, JSON.stringify({ rules: [] })); + + let fired = 0; + stop = watchConfigPaths(globalPath, projectPath, () => { fired++; }); + + // Delete the watched directory entirely + rmSync(configDir, { recursive: true, force: true }); + await new Promise((r) => setTimeout(r, 350)); + + // Must not crash, stop must succeed cleanly + assert.doesNotThrow(() => { + stop?.(); + }); } finally { + stop?.(); rmSync(dir, { recursive: true, force: true }); } });