fix(server): chokidar fallback watches subfolders (#760) (#2904)

.changeset/chokidar-fallback-subfolder-watching.md

@@ -0,0 +1,5 @@
+---
+"@inkeep/open-knowledge": patch
+---
+
+Fixed external edits to files in subfolders not being picked up when the server runs on its chokidar file-watching fallback (inkeep/open-knowledge#760). The server prefers `@parcel/watcher` but falls back to chokidar whenever that native module can't load. The fallback's ignore predicate misclassified any subdirectory that chokidar probed without file stats — routing it through the file-only exclusion rule, which rejects anything that isn't a Markdown doc or linkable asset — so chokidar pruned every content subfolder and silently stopped watching it. The result: editing a note in a subfolder with an external editor (or a `git pull` landing changes there) never reached the server, and the graph, backlinks, and dead-link views stayed stale until the next restart re-read everything from disk. The predicate now resolves whether a stats-less path is a directory before deciding, so subfolders are watched again; a path that can't be stat'd (for example a file deleted mid-scan) is admitted rather than pruned, so its delete still propagates. Excluded trees such as `node_modules/` and `.git/` stay pruned as before.

packages/server/src/file-watcher-chokidar-fallback.test.ts

@@ -0,0 +1,187 @@
+/**
+ * Regression: the chokidar fallback (used whenever @parcel/watcher can't be
+ * loaded — every packaged desktop build today) must watch SUBDIRECTORIES.
+ *
+ * The `ignored` predicate used to route a stats-less directory through the
+ * file-only `isExcluded`, which default-excludes any non-`.md`/non-asset name,
+ * so chokidar pruned every content subfolder and no external edit under one
+ * ever reached the server (graph / backlinks / dead-links stayed stale until a
+ * restart rebuilt from disk). inkeep/open-knowledge#760.
+ */
+import { lstatSync, mkdirSync, statSync, writeFileSync } from 'node:fs';
+import { mkdtemp, rm } from 'node:fs/promises';
+import { tmpdir } from 'node:os';
+import { resolve } from 'node:path';
+import { afterEach, beforeEach, describe, expect, test } from 'vitest';
+import { createContentFilter } from './content-filter.ts';
+import {
+  type DiskEvent,
+  isChokidarPathIgnored,
+  lastKnownHash,
+  startWatcher,
+  writeTracker,
+} from './file-watcher.ts';
+
+describe('isChokidarPathIgnored — stats matrix', () => {
+  let tmpDir: string;
+  let contentDir: string;
+
+  beforeEach(async () => {
+    tmpDir = await mkdtemp(resolve(tmpdir(), 'ok-chokidar-ignored-'));
+    contentDir = resolve(tmpDir, 'content');
+    mkdirSync(resolve(contentDir, 'sub'), { recursive: true });
+    mkdirSync(resolve(contentDir, 'node_modules', 'pkg'), { recursive: true });
+    writeFileSync(resolve(contentDir, 'sub', 'note.md'), '# Note\n');
+  });
+
+  afterEach(async () => {
+    await rm(tmpDir, { recursive: true, force: true });
+  });
+
+  test('the content dir root is never ignored', () => {
+    const filter = createContentFilter({ projectDir: tmpDir, contentDir });
+    expect(isChokidarPathIgnored(contentDir, filter, contentDir, statSync(contentDir))).toBe(false);
+  });
+
+  test('a content subdirectory is NOT ignored — with stats and (the bug) without', () => {
+    const filter = createContentFilter({ projectDir: tmpDir, contentDir });
+    const subDir = resolve(contentDir, 'sub');
+    // With directory stats: routed through isDirExcluded → descendable.
+    expect(isChokidarPathIgnored(contentDir, filter, subDir, statSync(subDir))).toBe(false);
+    // WITHOUT stats (chokidar's subwatch gate): pre-fix this fell through to
+    // the file-only isExcluded and returned true, pruning the subtree.
+    expect(isChokidarPathIgnored(contentDir, filter, subDir, undefined)).toBe(false);
+  });
+
+  test('a markdown file is not ignored; a stats-less non-content file routes through isExcluded', () => {
+    const filter = createContentFilter({ projectDir: tmpDir, contentDir });
+    const mdFile = resolve(contentDir, 'sub', 'note.md');
+    expect(isChokidarPathIgnored(contentDir, filter, mdFile, lstatSync(mdFile))).toBe(false);
+    // A real, extension-less file: `isExcluded` excludes it (not a doc/asset)
+    // while `isDirExcluded` would admit it, so a `true` here proves the
+    // stats-less path lstat'd to the FILE branch, not the directory branch.
+    writeFileSync(resolve(contentDir, 'sub', 'Makefile'), 'x');
+    const plain = resolve(contentDir, 'sub', 'Makefile');
+    expect(filter.isExcluded('sub/Makefile')).toBe(true);
+    expect(filter.isDirExcluded('sub/Makefile')).toBe(false);
+    expect(isChokidarPathIgnored(contentDir, filter, plain, undefined)).toBe(true);
+  });
+
+  test('excluded directories stay pruned even without stats (node_modules)', () => {
+    const filter = createContentFilter({ projectDir: tmpDir, contentDir });
+    const nm = resolve(contentDir, 'node_modules');
+    expect(isChokidarPathIgnored(contentDir, filter, nm, undefined)).toBe(true);
+  });
+
+  test('a nonexistent path with no stats is admitted (never prune on uncertainty)', () => {
+    const filter = createContentFilter({ projectDir: tmpDir, contentDir });
+    const gone = resolve(contentDir, 'sub', 'was-just-deleted.md');
+    expect(isChokidarPathIgnored(contentDir, filter, gone, undefined)).toBe(false);
+  });
+});
+
+describe('chokidar backend — live subfolder watching (forceBackend)', () => {
+  let tmpDir: string;
+  let contentDir: string;
+
+  beforeEach(async () => {
+    tmpDir = await mkdtemp(resolve(tmpdir(), 'ok-chokidar-live-'));
+    contentDir = resolve(tmpDir, 'content');
+    mkdirSync(resolve(contentDir, 'sub'), { recursive: true });
+    writeFileSync(resolve(contentDir, 'root.md'), '# Root\n');
+    writeFileSync(resolve(contentDir, 'sub', 'note.md'), '# Note\n\n[Root](./root)\n');
+    lastKnownHash.clear();
+    writeTracker.clear();
+  });
+
+  afterEach(async () => {
+    await rm(tmpDir, { recursive: true, force: true });
+  });
+
+  async function until(predicate: () => boolean, timeoutMs = 4000): Promise<boolean> {
+    const deadline = Date.now() + timeoutMs;
+    while (Date.now() < deadline) {
+      if (predicate()) return true;
+      await new Promise((r) => setTimeout(r, 40));
+    }
+    return predicate();
+  }
+
+  test('edits to a pre-existing subfolder doc dispatch a DiskEvent', async () => {
+    const filter = createContentFilter({ projectDir: tmpDir, contentDir });
+    const events: DiskEvent[] = [];
+    const handle = await startWatcher(contentDir, async (e) => void events.push(e), filter, {
+      forceBackend: 'chokidar',
+    });
+    try {
+      // Sanity: a root-level edit is detected (this worked pre-fix too).
+      writeFileSync(resolve(contentDir, 'root.md'), '# Root edited\n');
+      expect(
+        await until(() => events.some((e) => e.kind === 'update' && e.docName === 'root')),
+      ).toBe(true);
+
+      // The regression: a subfolder edit must also dispatch.
+      writeFileSync(resolve(contentDir, 'sub', 'note.md'), '# Note edited\n\n[Gone](./gone)\n');
+      expect(
+        await until(() => events.some((e) => e.kind === 'update' && e.docName === 'sub/note')),
+      ).toBe(true);
+    } finally {
+      await handle.unsubscribe();
+    }
+  });
+
+  test('a doc created in a NEW subfolder after watch start dispatches a DiskEvent', async () => {
+    const filter = createContentFilter({ projectDir: tmpDir, contentDir });
+    const events: DiskEvent[] = [];
+    const handle = await startWatcher(contentDir, async (e) => void events.push(e), filter, {
+      forceBackend: 'chokidar',
+    });
+    try {
+      mkdirSync(resolve(contentDir, 'fresh'));
+      // The folder-create dispatch proves chokidar's addDir was processed, so
+      // the sub-watch is armed before we write into the directory. A fixed
+      // sleep would race a loaded runner and idle on a fast one.
+      expect(
+        await until(() =>
+          events.some((e) => e.kind === 'folder-create' && e.relativePath === 'fresh'),
+        ),
+      ).toBe(true);
+      writeFileSync(resolve(contentDir, 'fresh', 'child.md'), '# Child\n');
+      expect(
+        await until(() =>
+          events.some(
+            (e) => (e.kind === 'create' || e.kind === 'update') && e.docName === 'fresh/child',
+          ),
+        ),
+      ).toBe(true);
+    } finally {
+      await handle.unsubscribe();
+    }
+  });
+
+  test('excluded subtrees stay pruned — node_modules edits do not dispatch', async () => {
+    const filter = createContentFilter({ projectDir: tmpDir, contentDir });
+    const events: DiskEvent[] = [];
+    const handle = await startWatcher(contentDir, async (e) => void events.push(e), filter, {
+      forceBackend: 'chokidar',
+    });
+    try {
+      // Write into the excluded subtree, then edit a watched file as a
+      // sentinel. Waiting for the sentinel to dispatch proves the watcher was
+      // live through the window — so the absent node_modules event is a real
+      // negative, not a vacuous "nothing fired yet" (node_modules is never
+      // watched, so its write can't race ahead of the sentinel).
+      mkdirSync(resolve(contentDir, 'node_modules', 'dep'), { recursive: true });
+      writeFileSync(resolve(contentDir, 'node_modules', 'dep', 'readme.md'), '# Dep\n');
+      writeFileSync(resolve(contentDir, 'root.md'), '# Root sentinel\n');
+      expect(
+        await until(() => events.some((e) => e.kind === 'update' && e.docName === 'root')),
+      ).toBe(true);
+      expect(
+        events.some((e) => 'docName' in e && String(e.docName).startsWith('node_modules')),
+      ).toBe(false);
+    } finally {
+      await handle.unsubscribe();
+    }
+  });
+});

packages/server/src/file-watcher.ts

@@ -1779,6 +1779,49 @@ async function startParcelWatcher(
 
 // ─── Backend: chokidar ──────────────────────────────────────────────────────
 
+/**
+ * chokidar `ignored` predicate — decides both whether to emit an event for a
+ * path AND (load-bearing) whether to descend into a directory.
+ *
+ * chokidar invokes this WITHOUT `stats` at the subwatch gate that decides
+ * whether a freshly-discovered directory gets its own watcher (readdirp entries
+ * carry lstat stats, but that gate does not). With no stats we cannot tell a
+ * directory from a file, and routing a real subdirectory through the file-only
+ * `isExcluded` — which default-excludes any non-`.md`/non-asset name — prunes
+ * the whole subtree, so the fallback silently stops watching every content
+ * subfolder. `lstatSync` (not `statSync`, to match `followSymlinks: false`)
+ * resolves the type; any stat error admits the path (return `false`) so a
+ * just-deleted file still emits its delete and a transiently-missing path is
+ * never pruned.
+ */
+export function isChokidarPathIgnored(
+  contentDir: string,
+  contentFilter: ContentFilter,
+  filePath: string,
+  stats?: Stats,
+): boolean {
+  const rel = toPosix(relative(contentDir, filePath));
+  if (rel === '' || rel === '.') return false;
+  let isDirectory: boolean;
+  if (stats) {
+    isDirectory = stats.isDirectory();
+  } else {
+    try {
+      isDirectory = lstatSync(filePath).isDirectory();
+    } catch {
+      return false;
+    }
+  }
+  return isDirectory ? contentFilter.isDirExcluded(rel) : contentFilter.isExcluded(rel);
+}
+
+/**
+ * Upper bound on how long `startWatcher` waits for chokidar's `ready` event
+ * before returning anyway. `ready` normally fires in well under a second; the
+ * cap only guards against a pathological initial scan wedging server boot.
+ */
+const CHOKIDAR_READY_TIMEOUT_MS = 10_000;
+
 async function startChokidarWatcher(
   contentDir: string,
   contentFilter: ContentFilter | undefined,
@@ -1800,12 +1843,8 @@ async function startChokidarWatcher(
     // contentDir from sourcing events for an arbitrary location on disk.
     followSymlinks: false,
     ignored: contentFilter
-      ? (filePath: string, stats?: import('node:fs').Stats) => {
-          const rel = toPosix(relative(contentDir, filePath));
-          if (rel === '' || rel === '.') return false;
-          if (stats?.isDirectory()) return contentFilter.isDirExcluded(rel);
-          return contentFilter.isExcluded(rel);
-        }
+      ? (filePath: string, stats?: Stats) =>
+          isChokidarPathIgnored(contentDir, contentFilter, filePath, stats)
       : undefined,
   });
 
@@ -1845,6 +1884,24 @@ async function startChokidarWatcher(
   watcher.on('addDir', (path) => queueEvent('create', path));
   watcher.on('unlinkDir', (path) => queueEvent('delete', path));
 
+  // chokidar's `watch()` returns before its initial scan and per-directory
+  // watch registration finish; unlike @parcel/watcher's `subscribe()`, the
+  // returned watcher is not yet observing. Awaiting `ready` makes `startWatcher`
+  // resolve only once external edits are actually detectable — otherwise an
+  // edit landing in the setup window (routine on slower inotify hosts) is
+  // silently missed. Bounded so a pathological scan never wedges boot: past the
+  // cap we proceed anyway (the watcher keeps arming in the background).
+  await new Promise<void>((resolveReady) => {
+    let settled = false;
+    const settle = () => {
+      if (settled) return;
+      settled = true;
+      resolveReady();
+    };
+    watcher.once('ready', settle);
+    setTimeout(settle, CHOKIDAR_READY_TIMEOUT_MS).unref();
+  });
+
   return {
     unsubscribe: () => {
       if (batchTimer) {
@@ -1876,6 +1933,7 @@ export async function startWatcher(
   contentDirRaw: string,
   onDiskEvent: (event: DiskEvent) => Promise<void>,
   contentFilter?: ContentFilter,
+  opts: { forceBackend?: 'parcel' | 'chokidar' } = {},
 ): Promise<WatcherHandle> {
   let contentDir: string;
   try {
@@ -1922,19 +1980,29 @@ export async function startWatcher(
   let subscription: AsyncSubscription;
   let backend: WatcherBackend;
   try {
-    const parcelSub = await startParcelWatcher(
-      contentDir,
-      contentFilter,
-      fileIndex,
-      folderIndex,
-      onDiskEvent,
-      aliasMap,
-      bumpFileIndexGeneration,
-    );
+    // `forceBackend` is a test seam (mirrors head-watcher): 'chokidar' skips
+    // the parcel attempt so the fallback can be exercised on a host where the
+    // native module IS resolvable; 'parcel' throws instead of degrading so a
+    // test can't silently pass on the wrong backend.
+    const parcelSub =
+      opts.forceBackend === 'chokidar'
+        ? null
+        : await startParcelWatcher(
+            contentDir,
+            contentFilter,
+            fileIndex,
+            folderIndex,
+            onDiskEvent,
+            aliasMap,
+            bumpFileIndexGeneration,
+          );
     if (parcelSub) {
       subscription = parcelSub;
       backend = 'parcel';
     } else {
+      if (opts.forceBackend === 'parcel') {
+        throw new Error('@parcel/watcher unavailable (forced backend)');
+      }
       subscription = await startChokidarWatcher(
         contentDir,
         contentFilter,