From 0910427fbf7e64e25bfdea16bf34186a97d6ce90 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 07:20:38 +0000 Subject: [PATCH] fix(metadata-fs): register written paths with the watcher instead of trusting its scan (#7282) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FileSystemRepository`'s watcher could go permanently blind to a single item: external edits to that file produced no `MetadataEvent` for the life of the process. The window is a race between chokidar's asynchronous initial scan and the repository's own first write, which both `start()` and `ensureRoot()` can open. Measured on chokidar 5 with this repository's options (`usePolling`, `interval: 1000`): chokidar reads `//` while the atomic `rename` in `writeJsonAtomic` has not landed, then takes the directory's polling baseline stat *after* it lands. The directory's stat never changes again, so it is never re-read, the item is never registered, no per-file watcher is created, and neither `add` nor `change` is ever emitted for that path — `getWatched()` reports the type directory as `[]` while the file sits in it. That is why the two time-based mitigations tried on this family could not have worked: the event is never delivered, not late. This fix widens no timer — the only writer that can be inside the window is us, so `put()` now tells the watcher explicitly about the path it created. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw --- .../metadata-fs-watch-write-registration.md | 41 ++++ packages/metadata-fs/src/repository.ts | 50 +++++ .../test/watch-write-registration.test.ts | 183 ++++++++++++++++++ 3 files changed, 274 insertions(+) create mode 100644 .changeset/metadata-fs-watch-write-registration.md create mode 100644 packages/metadata-fs/test/watch-write-registration.test.ts diff --git a/.changeset/metadata-fs-watch-write-registration.md b/.changeset/metadata-fs-watch-write-registration.md new file mode 100644 index 0000000000..4597e2d762 --- /dev/null +++ b/.changeset/metadata-fs-watch-write-registration.md @@ -0,0 +1,41 @@ +--- +"@objectstack/metadata-fs": patch +--- + +fix(metadata-fs): register every written path with the watcher, so an item created while chokidar is still scanning is not invisible forever (#7282) + +`FileSystemRepository`'s watcher could go **permanently blind to a single +item** — external edits to that file produced no `MetadataEvent` for the whole +life of the process, and nothing recovered short of a restart. The window is a +race between chokidar's asynchronous initial scan and the repository's own +first write, and both `start()` (which arms the watcher, after which the caller +may `put()` on the next tick) and `ensureRoot()` (which arms it in the middle +of the very first write, #7000) can open it. + +Measured on chokidar 5 with this repository's options (`usePolling`, +`interval: 1000`): + +1. chokidar reads `//` and finds it EMPTY — the atomic `rename` in + `writeJsonAtomic` has not landed yet; +2. the rename lands, changing the directory's mtime; +3. chokidar calls `watchFile()` on that directory and libuv takes its polling + baseline stat, which already reflects step 2. + +The directory's stat then never changes again, so no poll ever fires for it, +the directory is never re-read, the item file is never added to the watched +set, and no per-file watcher is created. `getWatched()` reports the type +directory as `[]` while the file sits in it, and neither `add` nor `change` is +ever emitted for that path. + +The fix does not widen any timer. The only writer that can be inside that +window is the repository itself, so `put()` now tells the watcher explicitly +about the path it created instead of depending on a directory scan that may +never notice it. Registration is idempotent and emits nothing. + +User-visible effect: `MetadataManager.subscribe()` (and every consumer of +`repo.watch()`) now reliably sees out-of-process edits — a hand edit, or a +`git checkout` bringing metadata JSON in — to items written earlier in the same +process. This was also the cause of four merge-queue ejections across three +PRs; the two time-based mitigations tried before it (a 20s/25s event deadline +and a wider pre-edit sleep) could not have worked, because the event was never +delivered rather than late. diff --git a/packages/metadata-fs/src/repository.ts b/packages/metadata-fs/src/repository.ts index 7426a3fa49..1fa4f23636 100644 --- a/packages/metadata-fs/src/repository.ts +++ b/packages/metadata-fs/src/repository.ts @@ -305,6 +305,9 @@ export class FileSystemRepository implements MetadataRepository { // we keep it in selfWrites for one debounce tick. setTimeout(() => this.selfWrites.delete(file), 200); } + // The watcher must not depend on its own directory scan to notice a + // path we created ourselves (#7282). See `trackWrittenPath`. + this.trackWrittenPath(file); this.heads.set(key, hash); const evt: MetadataEvent = { @@ -433,6 +436,53 @@ export class FileSystemRepository implements MetadataRepository { return last; } + /** + * Register a path this repository just wrote with the watcher (#7282). + * + * chokidar's initial scan is asynchronous, and every write path here can be + * running **while it is still walking the tree** — `start()` arms the watcher + * and the caller may `put()` on the next tick, and `ensureRoot()` arms it in + * the middle of the very first write. With `usePolling` that combination has + * a permanently-blinding interleaving, measured on chokidar 5 with this + * repository's own options: + * + * 1. chokidar reads `//` and finds it EMPTY — the atomic + * `rename` in `writeJsonAtomic` has not landed yet. + * 2. the rename lands; the directory's mtime changes. + * 3. chokidar calls `watchFile()` on that directory, and libuv takes its + * polling baseline stat — which already reflects step 2. + * + * From then on the directory's stat never changes again, so no poll ever + * fires for it, `_handleRead` never re-runs, the item file is never added to + * the watched set, and no per-file watcher is ever created. chokidar emits + * neither `add` nor `change` for that path **for the life of the process** — + * `getWatched()` reports the type directory as `[]` forever while the file + * sits in it. That is the whole of #7282: the four merge-queue ejections all + * waited out their deadlines (20s, then 25541ms against 25s) on an event that + * was never going to be delivered, which is why widening the deadline and + * widening the pre-edit sleep both changed nothing, and why lowering + * `interval` would change nothing either — a shorter poll re-compares against + * the same unchanged directory stat. + * + * The window is exactly "files that exist at baseline time but were absent + * from the snapshot read a moment earlier", and the only writer that can be + * inside it is us. So we close it at the source: tell the watcher explicitly + * about every path we create, instead of hoping its scan happened to see it. + * + * `add()` is idempotent here — `_handleFile` returns early when the parent + * directory already tracks the basename — and it emits nothing, because + * chokidar treats an explicit `add()` as an initial add and `ignoreInitial` + * is set. Its effect is the one we need: `_watchWithNodeFs` registers the + * basename with the parent directory (without which chokidar drops `change` + * events for the file) and starts the per-file poll. + */ + private trackWrittenPath(file: string): void { + const w = this.watcher; + // `add()` clears `closed`, so never hand a closing watcher a new path. + if (!w || w.closed) return; + w.add(file); + } + private startWatcher(): void { const root = this.layout.root; const w = chokidar.watch(root, { diff --git a/packages/metadata-fs/test/watch-write-registration.test.ts b/packages/metadata-fs/test/watch-write-registration.test.ts new file mode 100644 index 0000000000..548ed4f5e4 --- /dev/null +++ b/packages/metadata-fs/test/watch-write-registration.test.ts @@ -0,0 +1,183 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7282 — a path this repository writes must be registered with the watcher + * **without waiting for a poll interval**. + * + * The defect this pins is a permanently-blinding interleaving between + * chokidar's asynchronous initial scan and our own first write, measured on + * chokidar 5 with this repository's options (`usePolling`, `interval: 1000`): + * + * 1. chokidar reads `//` and finds it EMPTY — `writeJsonAtomic`'s + * `rename` has not landed yet. + * 2. the rename lands; the directory's mtime changes. + * 3. chokidar calls `watchFile()` on the directory and libuv takes its + * polling baseline stat, which already reflects step 2. + * + * The directory's stat then never changes again, so the directory is never + * re-read, the item file is never registered, no per-file watcher is created, + * and neither `add` nor `change` is emitted for it **for the life of the + * process**. Measured directly: `getWatched()` reported the type directory as + * `[]` for the full 7.5s of a probe run while the file sat in it, and + * `handleFsChange` was never entered once. + * + * That is why both time-based mitigations tried on this family failed: the + * event is never delivered, so a 20s deadline (#7208) and a 25541ms one (#7255) + * both ran out with an empty array, and a wider pre-edit sleep cannot help + * either — the damage is done before the sleep starts. Lowering `interval` + * cannot help for the same reason: a shorter poll re-compares against the same + * unchanged directory stat. + * + * ## Why this file asserts registration rather than the race + * + * The interleaving above lives inside chokidar, between a `readdirp` stream end + * and a `watchFile()` call, so it cannot be forced from the outside without a + * test seam. What CAN be pinned deterministically is the property the fix + * establishes, which is what actually closes the window: after `put()` returns, + * the watcher knows the path **from us**, not from a directory scan that may + * never happen. + * + * The two outcomes are separated structurally, not by a lucky margin: + * + * - with the fix, registration costs one `stat` (single-digit ms); + * - without it, the earliest the watcher can learn the path is its next + * directory poll — a full `interval` (1000ms) plus the `awaitWriteFinish` + * stability window. + * + * The case anchors the poll phase before measuring so that "one poll away" is + * genuinely a full interval away: it lets an out-of-process file land first and + * waits for the repository to report it, which happens only when a poll tick + * for the type directory has just fired. `REGISTRATION_BUDGET_MS` then sits an + * order of magnitude below the remaining interval. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import fs from 'node:fs/promises'; +import path from 'node:path'; +import os from 'node:os'; +import type { MetaRef, MetadataEvent } from '@objectstack/metadata-core'; +import { FileSystemRepository } from '../src/index.js'; + +const ref = (name: string): MetaRef => ({ org: 'system', type: 'view', name }); +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +/** + * How long registration may take after `put()` resolves. Comfortably above one + * `stat` under a saturated runner and comfortably below the watcher's 1000ms + * poll interval, which is the only other way the path could get registered. + */ +const REGISTRATION_BUDGET_MS = 400; + +/** Deadline for the positive waits that anchor the poll phase and control the case. */ +const EVENT_WAIT_MS = 20_000; + +const CASE_TIMEOUT_MS = 60_000; + +/** + * chokidar's watched-path registry, reached through the repository's private + * watcher handle. `getWatched()` is chokidar's own public API; only the handle + * is internal, and no public surface reports it. + */ +interface WatcherHandle { + watcher: { + getWatched(): Record; + once(event: 'ready', listener: () => void): unknown; + } | null; +} + +const handleOf = (repo: FileSystemRepository) => { + const w = (repo as unknown as WatcherHandle).watcher; + if (!w) throw new Error('watcher not armed — the case cannot measure anything'); + return w; +}; + +const watchedIn = (repo: FileSystemRepository, dir: string): string[] => + handleOf(repo).getWatched()[dir] ?? []; + +describe('FileSystemRepository watcher — writes register their own path (#7282)', () => { + let root: string; + let repo: FileSystemRepository | undefined; + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'objectstack-fs7282-')); + }); + + afterEach(async () => { + if (repo) await repo.close().catch(() => undefined); + repo = undefined; + await fs.rm(root, { recursive: true, force: true }); + }); + + it('registers a put() path with the watcher without waiting for a poll', async () => { + const viewDir = path.join(root, 'view'); + // The type directory exists and is non-empty before the watcher arms, so + // chokidar's scan cannot be blamed for anything measured below. + await fs.mkdir(viewDir, { recursive: true }); + await fs.writeFile(path.join(viewDir, 'seed.json'), JSON.stringify({ label: 'seed' }, null, 2)); + + repo = new FileSystemRepository({ root, org: 'system' }); // watcher ENABLED + await repo.start(); + // Attach before yielding. `start()` arms the watcher as its last statement + // and chokidar cannot finish its walk without at least one async stat, so + // `ready` provably has not fired yet. Waiting for it matters: a file that + // lands while the initial walk is still running is treated as pre-existing + // and, under `ignoreInitial`, emits nothing at all — measured 3/3. The + // anchor below must not be subject to that. + const scanned = new Promise((res) => { handleOf(repo!).once('ready', res); }); + + const iter = repo.watch({ org: 'system' }, 999)[Symbol.asyncIterator](); + const events: MetadataEvent[] = []; + let resolveNext: (() => void) | null = null; + void (async () => { + for (;;) { + const next = await iter.next(); + if (next.done) return; + events.push(next.value as MetadataEvent); + resolveNext?.(); + } + })(); + const nextEvent = () => new Promise((res) => { resolveNext = res; }); + + await Promise.race([scanned, sleep(EVENT_WAIT_MS)]); + // The walk reached the type directory, so what follows is measuring the + // steady state and not the initial scan. + expect(watchedIn(repo, viewDir)).toContain('seed.json'); + + // ── Anchor the poll phase ──────────────────────────────────────────── + // An out-of-process file lands in the type directory; the repository can + // only report it once a poll tick for that directory has fired. Returning + // from this wait therefore means we are at the START of a fresh interval, + // so "one poll away" below really is ~1000ms away and not ~5ms away. + const anchored = nextEvent(); + await fs.writeFile(path.join(viewDir, 'anchor.json'), JSON.stringify({ label: 'anchor' }, null, 2)); + await Promise.race([anchored, sleep(EVENT_WAIT_MS)]); + expect(events.map((e) => e.ref.name)).toContain('anchor'); + + // ── The measurement ────────────────────────────────────────────────── + await repo.put(ref('fresh'), { label: 'fresh' }, { parentVersion: null, actor: 'tester' }); + + const deadline = Date.now() + REGISTRATION_BUDGET_MS; + while (!watchedIn(repo, viewDir).includes('fresh.json') && Date.now() < deadline) { + await sleep(10); + } + expect(watchedIn(repo, viewDir)).toContain('fresh.json'); + + // ── Liveness control ───────────────────────────────────────────────── + // ⚠️ Green in BOTH directions on a quiet machine — the registration race is + // load-dependent, so this cannot be the case's evidence. It is here to + // prove the registration above is the real thing and not bookkeeping: an + // external edit to the freshly written path must still reach subscribers. + const edited = nextEvent(); + await fs.writeFile( + path.join(viewDir, 'fresh.json'), + JSON.stringify({ label: 'fresh, edited on disk' }, null, 2), + ); + await Promise.race([edited, sleep(EVENT_WAIT_MS)]); + await iter.return?.(undefined); + + const updates = events.filter((e) => e.ref.name === 'fresh' && e.op === 'update'); + expect(updates).toHaveLength(1); + expect(updates[0]!.source).toBe('fs'); + expect(updates[0]!.actor).toBe('fs'); + }, CASE_TIMEOUT_MS); +});