From 0aad6111bac991b4e2fd3476939bfde3ebb7c77f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 13:36:39 +0000 Subject: [PATCH 1/3] =?UTF-8?q?fix(metadata-fs):=20give=20an=20external=20?= =?UTF-8?q?write=20more=20than=20one=20delivery=20attempt=20=E2=80=94=20co?= =?UTF-8?q?ntent-keyed=20reconciliation=20behind=20the=20watcher=20poll=20?= =?UTF-8?q?(#9339)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The watcher gave an externally-written file exactly ONE chance to be noticed, and losing it was permanent and silent. Under `usePolling`, chokidar re-reads a directory only when its stat strictly advances; an external write advances the type directory's mtime once, so every later poll compares an unchanged stat and can never rediscover the file. At least six independent one-shot gates sit on that single attempt, spanning three layers, and all six produce a byte-identical observable — which is why #7282 was closed on one member of the family and reopened as #9339. The fix never asks which gate fired. A bounded, content-keyed reconciliation sweep compares what is on disk against `heads` and publishes the divergence through the same handler the watcher feeds, so it is robust across all six by construction. `put()`'s direct registration (#7336) and every production watcher constant are untouched. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017qYPmkKEsfbWY1yVg83p8F --- .../metadata-fs-external-write-resync.md | 54 ++++ packages/metadata-fs/src/repository.ts | 217 ++++++++++++++-- .../test/external-write-resync.test.ts | 233 ++++++++++++++++++ 3 files changed, 488 insertions(+), 16 deletions(-) create mode 100644 .changeset/metadata-fs-external-write-resync.md create mode 100644 packages/metadata-fs/test/external-write-resync.test.ts diff --git a/.changeset/metadata-fs-external-write-resync.md b/.changeset/metadata-fs-external-write-resync.md new file mode 100644 index 0000000000..2c4b8e2774 --- /dev/null +++ b/.changeset/metadata-fs-external-write-resync.md @@ -0,0 +1,54 @@ +--- +"@objectstack/metadata-fs": patch +--- + +fix(metadata-fs): an external write reaches subscribers even when the watcher's single delivery attempt is lost — content-keyed reconciliation behind the poll (#9339) + +`FileSystemRepository`'s watcher gave an externally-written file **exactly one** +chance to be noticed, and losing it was permanent and silent. Under +`usePolling`, chokidar re-reads a directory only when its stat *strictly* +advances; an external write advances the type directory's mtime once, so poll +#2..#N compare an unchanged stat and can never rediscover the file. Measured on +#9339 with a fault-injection harness: with that single read suppressed, fifteen +further poll ticks never find the file — a 20s deadline and a 200s deadline buy +the same one attempt. That is the structural reason behind #7282's empirical +finding that the event is *"never delivered, not slow"*, and why widening the +deadline (#7208) and lowering `interval` were both spent before they were tried. + +**At least six independent one-shot gates sit on that attempt**, spanning three +layers — the kernel timestamp (the directory mtime does not strictly advance), +chokidar's readdir throttle and readdir snapshot, and chokidar's emit gates +(`_throttle('add')`, a stale `_pendingWrites` entry, the `awaitWriteFinish` +ENOENT early return). Each produces a byte-identical observable: no event, ever, +for that path. They are indistinguishable at the point of failure, which is why +#7282's close — picked from that family — covered one member and reopened. + +**The fix does not name a member.** A bounded, content-keyed reconciliation +sweep runs alongside the watcher and compares what is on disk against `heads`, +the index that already defines what the repository believes it holds, publishing +any divergence through the *same* handler the watcher feeds. Its only premise is +that the bytes on disk stopped matching the index, so it is robust across all six +by construction — and equally across a seventh nobody has found. + +- **Cadence** — one pass over `//*.json` every 2s (twice the poll + interval), the same walk `start()` already performs once. Sweeps are chained + rather than intervalled, so they can never overlap or stack behind a slow + disk; the timer is `unref`ed and is retired by `close()`; and it is armed only + alongside the watcher, so a `disableWatch` repository pays nothing. +- **Exactly-once is preserved.** Suppression stays content-keyed (`#7335`): the + sweep republishes nothing the watcher already delivered, and recognises this + repository's own `put()` by content rather than by a clock. +- **Events are indistinguishable from the fast path** — same `op`, + `parentHash`, `source: 'fs'` and actor, because they are produced by the same + code. A subscriber cannot be made to care which path noticed. +- **A recovered path is re-armed** with the watcher through the seam `put()` + already uses, so a loss upstream of chokidar's `_handleFile` does not leave + the file dependent on the sweep forever. +- `put()`'s existing direct registration (#7336) is unchanged, as are + `usePolling`, `interval`, and `awaitWriteFinish`. + +⚠️ **Bound on the claim.** The six gates are *forced fault injections*, not the +CI mechanism, which was never identified and may be a seventh. What is measured +is that the fix converts **six of six** forced one-shot gates from permanent +loss to delivery (3/3 runs each), where all six returned an empty event list +before it. That is not the same statement as "the flake is fixed". diff --git a/packages/metadata-fs/src/repository.ts b/packages/metadata-fs/src/repository.ts index 003adb0e5a..36aca6a08c 100644 --- a/packages/metadata-fs/src/repository.ts +++ b/packages/metadata-fs/src/repository.ts @@ -80,6 +80,21 @@ const matchRefFilter = ( const matchEvent = (evt: MetadataEvent, filter: WatchFilter): boolean => matchRefFilter(evt.ref, filter); +/** + * Cadence of the content-keyed reconciliation sweep (#9339). + * + * Twice the watcher's own 1000ms poll interval: long enough that the watcher + * normally delivers first and the sweep finds nothing to do, short enough that + * a delivery the watcher lost is recovered in the same order of magnitude as a + * poll rather than at the next process restart. + * + * It is deliberately NOT derived from `interval` at runtime. The two are + * independent knobs — the poll interval sets detection latency for the fast + * path, this sets the worst-case latency of the backstop — and coupling them + * would make a future change to one silently retune the other. + */ +const RESYNC_INTERVAL_MS = 2_000; + export class FileSystemRepository implements MetadataRepository { private readonly layout: FsLayout; private readonly org: string; @@ -96,6 +111,10 @@ export class FileSystemRepository implements MetadataRepository { private nextSeq = 1; private watcher: FSWatcher | null = null; private started = false; + /** Pending reconciliation sweep (#9339). Chained, never overlapping. */ + private resyncTimer: ReturnType | null = null; + /** False before the watcher is armed and from `close()` onwards. */ + private resyncEnabled = false; constructor(opts: FileSystemRepositoryOptions) { this.org = opts.org; @@ -160,6 +179,9 @@ export class FileSystemRepository implements MetadataRepository { } async close(): Promise { + // Retire the sweep BEFORE awaiting the watcher, so a sweep that lands + // during `watcher.close()` cannot reschedule itself behind our back. + this.stopResync(); if (this.watcher) { await this.watcher.close(); this.watcher = null; @@ -511,6 +533,184 @@ export class FileSystemRepository implements MetadataRepository { w.on('change', (p) => void this.handleFsChange(p, 'change')); w.on('unlink', (p) => void this.handleFsChange(p, 'unlink')); this.watcher = w; + // The watcher is the fast path, not the guarantee (#9339). See `resync`. + this.startResync(); + } + + /** + * Publish the `delete` face of an externally-observed removal. + * + * Extracted from `handleFsChange` unchanged so the reconciliation sweep + * (#9339) can reuse it **verbatim** rather than growing a second copy of the + * event shape. The one-line invariant: the caller already holds the per-key + * mutex, and `!currentHead` is the content-keyed suppression that makes our + * own `delete()` a no-op here. + */ + private async publishExternalDelete(ref: MetaRef, key: string): Promise { + const currentHead = this.heads.get(key) ?? null; + if (!currentHead) return; + this.heads.delete(key); + const seq = this.nextSeq++; + const evt: MetadataEvent = { + seq, + op: 'delete', + ref: { ...ref, version: undefined }, + hash: null, + parentHash: currentHead, + actor: this.fsActor, + ts: this.now().toISOString(), + source: 'fs', + }; + await this.log.append(evt); + this.broker.publish(evt); + } + + private startResync(): void { + this.resyncEnabled = true; + this.scheduleResync(); + } + + private stopResync(): void { + this.resyncEnabled = false; + if (this.resyncTimer) { + clearTimeout(this.resyncTimer); + this.resyncTimer = null; + } + } + + /** + * Schedule the next sweep — chained, never `setInterval` (#9339). + * + * A chained timeout cannot stack: the next sweep is armed only once the + * previous one has finished, so a saturated runner degrades to *fewer* + * sweeps instead of a growing backlog of overlapping tree walks. The timer + * is `unref`ed because a backstop must never be the reason a process stays + * alive. + */ + private scheduleResync(): void { + if (!this.resyncEnabled || this.resyncTimer) return; + const timer = setTimeout(() => { + this.resyncTimer = null; + void this.resync().finally(() => this.scheduleResync()); + }, RESYNC_INTERVAL_MS); + timer.unref?.(); + this.resyncTimer = timer; + } + + /** + * Content-keyed reconciliation sweep — the backstop that makes external-edit + * detection a guarantee rather than a single chance (#9339, #7282). + * + * ## Why the watcher alone cannot be the guarantee + * + * An external write to `//.json` reaches a subscriber only + * if chokidar notices it, and under `usePolling` it gets **exactly one** + * opportunity to do so: the write advances the type directory's mtime once, + * and chokidar re-reads a directory only when its stat *strictly advances*, + * so every later poll compares an unchanged stat and can never rediscover + * the file. Measured on #9339 with a fault-injection harness: with the one + * read suppressed, fifteen further poll ticks never find the new file, and a + * 20s deadline and a 200s deadline buy the same single attempt. That is the + * structural reason behind #7282's empirical finding that the event is + * "never delivered, not slow", and why widening the deadline (#7208) and + * lowering `interval` were both spent before they were tried. + * + * At least six independent one-shot gates sit on that single attempt, + * spanning three layers — the kernel timestamp (the directory mtime does not + * strictly advance), chokidar's readdir throttle and readdir snapshot, and + * chokidar's emit gates (`_throttle('add')`, a stale `_pendingWrites` entry, + * the `awaitWriteFinish` ENOENT early return). Each one produces a + * byte-identical observable: no event, ever, for that path. + * + * ## Why this shape, and not a narrower one + * + * ⚠️ The six are indistinguishable at the point of failure, so **any fix + * that has to name which gate fired is a fix for one member of a family** — + * which is exactly how #7282 was closed and exactly why it reopened. This + * sweep never asks. It compares what is on disk against `heads`, the index + * that already defines what this repository believes it holds, and publishes + * the divergence through the same `handleFsChange` the watcher feeds. It is + * therefore robust across all six *by construction*, and equally across a + * seventh nobody has found: the only property it relies on is that the bytes + * on disk stopped matching the index. + * + * `put()` is unaffected and keeps its direct registration (`trackWrittenPath` + * calls `watcher.add` and bypasses the whole chain, which is why the `put()` + * half of this family was already closed by #7336 and the external-write half + * was not). + * + * ## Cost, and why it is bounded + * + * One pass over `//*.json` per sweep — the same walk `start()` + * already performs once — with no retry loop inside it and no work at all + * when nothing diverged. Sweeps are chained, so they cannot overlap; the + * timer is `unref`ed and dies with `close()`; and it is armed only alongside + * the watcher, so a `disableWatch` repository pays nothing. + * + * Discovery is by content, never by stat: a stat pre-filter would reintroduce + * a time key of exactly the kind this replaces. + */ + private async resync(): Promise { + const root = this.layout.root; + let entries: import('node:fs').Dirent[] = []; + try { + entries = await fs.readdir(root, { withFileTypes: true }); + } catch { + // A root that has gone away is not a divergence we can report on; the + // next sweep sees whatever replaces it. + return; + } + const onDisk = new Set(); + for (const entry of entries) { + if (!entry.isDirectory()) continue; + // Same dot-entry rule as `scanHeads` and `isIgnoredWatchPath`, so the + // boot scan, the watcher and this sweep agree on what the repository + // contains (#7150). + if (entry.name.startsWith('.')) continue; + const dir = path.join(root, entry.name); + let files: string[] = []; + try { + files = await fs.readdir(dir); + } catch { + continue; + } + for (const file of files) { + if (!file.endsWith('.json') || file.startsWith('.')) continue; + const abs = path.join(dir, file); + const parsed = parseItemPath(this.layout, abs); + if (!parsed) continue; + const ref: MetaRef = { + org: this.org, + type: parsed.type as MetadataType, + name: parsed.name, + }; + const key = refKey(ref); + onDisk.add(key); + const before = this.heads.get(key); + await this.handleFsChange(abs, 'add'); + if (this.heads.get(key) !== before) { + // We just published a change the watcher never delivered, so the + // watcher may not know this path at all (the loss can be upstream of + // chokidar's `_handleFile`). Re-arm it through the same seam `put()` + // uses, so the fast path is restored instead of leaving every future + // edit to this file dependent on the sweep. + this.trackWrittenPath(abs); + } + } + } + for (const key of [...this.heads.keys()]) { + if (onDisk.has(key)) continue; + const ref = parseRefKey(key); + if (!ref) continue; + const file = itemPath(this.layout, ref.type, ref.name); + await this.mutex.run(key, async () => { + // Re-checked UNDER the lock. The enumeration above ran outside it, so + // a `put()` that created this file in between would otherwise be + // reported as an external delete. + if (existsSync(file)) return; + await this.publishExternalDelete(ref, key); + }); + } } /** @@ -572,22 +772,7 @@ export class FileSystemRepository implements MetadataRepository { const key = refKey(ref); await this.mutex.run(key, async () => { if (kind === 'unlink') { - const currentHead = this.heads.get(key) ?? null; - if (!currentHead) return; - this.heads.delete(key); - const seq = this.nextSeq++; - const evt: MetadataEvent = { - seq, - op: 'delete', - ref: { ...ref, version: undefined }, - hash: null, - parentHash: currentHead, - actor: this.fsActor, - ts: this.now().toISOString(), - source: 'fs', - }; - await this.log.append(evt); - this.broker.publish(evt); + await this.publishExternalDelete(ref, key); return; } const body = await readJson(absPath); diff --git a/packages/metadata-fs/test/external-write-resync.test.ts b/packages/metadata-fs/test/external-write-resync.test.ts new file mode 100644 index 0000000000..d9a4f9c2f4 --- /dev/null +++ b/packages/metadata-fs/test/external-write-resync.test.ts @@ -0,0 +1,233 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #9339 — an external write must reach subscribers even when the watcher's + * one delivery attempt is lost. + * + * ## What this pins, and why it is not the same pin as #7282's + * + * `watch-write-registration.test.ts` pins the `put()` half of this family: a + * path we wrote is registered with the watcher directly (`trackWrittenPath`), + * so it never depends on a directory scan. PR #7336 closed that half by + * *routing around* the fragile path. This file pins the half that route left + * open — a write made by somebody else, which has no choice but to traverse it. + * + * The traversal gets **exactly one** attempt. Under `usePolling`, chokidar + * re-reads a directory only when its stat strictly advances; an external write + * advances the type directory's mtime once, so poll #2..#N compare an unchanged + * stat and can never rediscover the file. Measured on #9339 with a + * fault-injection harness: with that single read suppressed, fifteen further + * poll ticks never find the file, and a 20s deadline and a 200s deadline buy + * the same one attempt. + * + * At least six independent one-shot gates sit on that attempt, spanning three + * layers — the kernel timestamp (`mtime-tie`), chokidar's readdir throttle and + * readdir snapshot (`readdir-throttle`, `readdirp-miss`), and chokidar's emit + * gates (`add-throttle`, `pending-write`, `awf-enoent`). Every one of them + * produces a byte-identical observable: no event, ever, for that path. + * + * ## Why the cases blind the watcher rather than force a named gate + * + * ⚠️ The six are indistinguishable at the point of failure, so a case that + * forced one of them would pin a fix for one member of a family — which is how + * #7282 was closed and why it reopened. What the six have in common is not a + * mechanism, it is a **boundary condition**: chokidar's `add`/`change`/`unlink` + * callback never fires for the path. That is what the cases below reproduce, + * deterministically and without reaching into chokidar's internals, by + * detaching the repository's own listeners. A fix that survives it survives all + * six by construction, and equally a seventh nobody has found. + * + * ⛔ These cases are NOT a claim about the CI mechanism, which was never + * identified. They pin the repository's guarantee, not chokidar's behaviour. + */ + +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)); + +/** + * Deadline for a recovery that the sweep should reach in ~2s. Wide enough that + * a saturated runner cannot turn a working backstop red, and narrow enough that + * a broken one cannot pass by exhausting the case timeout. + */ +const RECOVERY_WAIT_MS = 15_000; + +/** Comfortably more than two sweeps, for the "did NOT republish" cases. */ +const QUIET_WINDOW_MS = 5_000; + +const CASE_TIMEOUT_MS = 60_000; + +/** + * The repository's private watcher handle. `removeAllListeners` is + * `EventEmitter`'s own API; only the handle is internal, and the same reach is + * already made by `watch-write-registration.test.ts`. + */ +interface WatcherHandle { + watcher: { + once(event: 'ready', listener: () => void): unknown; + removeAllListeners(event: string): unknown; + } | null; + resyncTimer: unknown; + resyncEnabled: boolean; +} + +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; +}; + +/** + * Reproduce the boundary condition all six measured gates share: chokidar never + * calls back for anything under the root. Everything downstream of that seam — + * whether the file reached chokidar's watched set, whether an emit was + * throttled, whether `awaitWriteFinish` leaked a pending entry — is exactly the + * detail the failing assertion cannot see, so the case declines to depend on it. + */ +const blindWatcher = (repo: FileSystemRepository): void => { + const w = handleOf(repo); + w.removeAllListeners('add'); + w.removeAllListeners('change'); + w.removeAllListeners('unlink'); +}; + +describe('FileSystemRepository — external writes survive a lost watcher event (#9339)', () => { + let root: string; + let viewDir: string; + let repo: FileSystemRepository | undefined; + let events: MetadataEvent[]; + let stopDrain: (() => Promise) | undefined; + + const waitFor = async (pred: () => boolean, ms: number): Promise => { + const deadline = Date.now() + ms; + while (!pred() && Date.now() < deadline) await sleep(25); + }; + + const subscribe = (): void => { + const iter = repo!.watch({ org: 'system' }, 999)[Symbol.asyncIterator](); + const drain = async () => { + for (;;) { + const next = await iter.next(); + if (next.done) return; + events.push(next.value as MetadataEvent); + } + }; + void drain(); + stopDrain = async () => { + await iter.return?.(undefined); + }; + }; + + const start = async (): Promise => { + repo = new FileSystemRepository({ root, org: 'system' }); // watcher ENABLED + await repo.start(); + // The initial walk must finish first: a file that lands while it is still + // running is treated as pre-existing and, under `ignoreInitial`, emits + // nothing at all. What follows measures the steady state. + const scanned = new Promise((res) => { handleOf(repo!).once('ready', res); }); + subscribe(); + await Promise.race([scanned, sleep(RECOVERY_WAIT_MS)]); + }; + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'objectstack-fs9339-')); + viewDir = path.join(root, 'view'); + await fs.mkdir(viewDir, { recursive: true }); + await fs.writeFile(path.join(viewDir, 'seed.json'), JSON.stringify({ label: 'seed' }, null, 2)); + events = []; + }); + + afterEach(async () => { + await stopDrain?.().catch(() => undefined); + stopDrain = undefined; + if (repo) await repo.close().catch(() => undefined); + repo = undefined; + await fs.rm(root, { recursive: true, force: true }); + }); + + it('recovers an external create, update and delete the watcher never delivered', async () => { + await start(); + // From here on chokidar can observe whatever it likes; none of it reaches + // the repository. This is the state all six one-shot gates leave behind. + blindWatcher(repo!); + + // ── create ─────────────────────────────────────────────────────────── + const file = path.join(viewDir, 'late.json'); + await fs.writeFile(file, JSON.stringify({ label: 'late' }, null, 2)); + await waitFor(() => events.some((e) => e.ref.name === 'late'), RECOVERY_WAIT_MS); + + const created = events.filter((e) => e.ref.name === 'late'); + expect(created).toHaveLength(1); + expect(created[0]!.op).toBe('create'); + // The backstop must be indistinguishable from the fast path: a subscriber + // cannot be made to care which one noticed. + expect(created[0]!.source).toBe('fs'); + expect(created[0]!.actor).toBe('fs'); + expect(created[0]!.parentHash).toBeNull(); + + // ── update ─────────────────────────────────────────────────────────── + await fs.writeFile(file, JSON.stringify({ label: 'late, edited' }, null, 2)); + await waitFor(() => events.some((e) => e.ref.name === 'late' && e.op === 'update'), RECOVERY_WAIT_MS); + + const updated = events.filter((e) => e.ref.name === 'late' && e.op === 'update'); + expect(updated).toHaveLength(1); + expect(updated[0]!.parentHash).toBe(created[0]!.hash); + + // ── delete ─────────────────────────────────────────────────────────── + await fs.rm(file); + await waitFor(() => events.some((e) => e.ref.name === 'late' && e.op === 'delete'), RECOVERY_WAIT_MS); + + const deleted = events.filter((e) => e.ref.name === 'late' && e.op === 'delete'); + expect(deleted).toHaveLength(1); + expect(deleted[0]!.hash).toBeNull(); + expect(deleted[0]!.parentHash).toBe(updated[0]!.hash); + + // And the repository's own view agrees, so the index did not merely emit. + expect(await repo!.get(ref('late'))).toBeNull(); + }, CASE_TIMEOUT_MS); + + it('publishes each external change exactly once, and never republishes our own put()', async () => { + await start(); + + // A live watcher: the fast path delivers, and the sweep must then find + // nothing to say. Anything else would double every external edit. + const external = path.join(viewDir, 'once.json'); + await fs.writeFile(external, JSON.stringify({ label: 'once' }, null, 2)); + await waitFor(() => events.some((e) => e.ref.name === 'once'), RECOVERY_WAIT_MS); + + // Our own write. `put()` publishes its own event; the sweep reads the same + // bytes back and must recognise them as ours by content, not by a clock. + await repo!.put(ref('mine'), { label: 'mine' }, { parentVersion: null, actor: 'tester' }); + + // Several sweeps' worth of quiet. + await sleep(QUIET_WINDOW_MS); + + expect(events.filter((e) => e.ref.name === 'once')).toHaveLength(1); + + const mine = events.filter((e) => e.ref.name === 'mine'); + expect(mine).toHaveLength(1); + expect(mine[0]!.actor).toBe('tester'); + expect(mine[0]!.op).toBe('create'); + }, CASE_TIMEOUT_MS); + + it('retires the sweep on close(), so a closed repository schedules no further work', async () => { + await start(); + const priv = repo as unknown as WatcherHandle; + expect(priv.resyncEnabled).toBe(true); + + await repo!.close(); + + // A backstop that outlives its repository is a leaked timer holding a + // closed watcher and a stale index — the failure shape would be a CI + // worker that never exits, diagnosed nowhere near this file. + expect(priv.resyncEnabled).toBe(false); + expect(priv.resyncTimer).toBeNull(); + expect(priv.watcher).toBeNull(); + }, CASE_TIMEOUT_MS); +}); From 5936072c157918e2287eafb91e0ef06c70d8e16e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 14:45:17 +0000 Subject: [PATCH 2/3] fix(metadata-fs): the reconciliation sweep discriminates its read failures instead of inventing an empty listing (#8895) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both `readdir` seams in `resync()` swallowed every errno and answered with "there is nothing here". ENOENT is the one truthful empty answer — a path that does not exist holds no items — and it stays silent. Every other code means the read could not RUN: EACCES, EIO, and above all EMFILE/ENFILE, which degrade this read and chokidar's own `fs.watchFile` polling at the same time and for the same reason. Silence there made the backstop absent for the life of the process exactly under the load-dependent conditions it exists to catch. Reported at `error` per AGENTS.md's judgement question — the system keeps looking healthy while its index drifts from disk — naming the consequence and the fix, and latched per path+errno so a standing fault is said once rather than every 2s. It deliberately does not throw: this runs on a background timer. An unreadable type directory is also excluded from the delete pass, which would otherwise read "could not look" as "the files are gone" and retire every item of that type. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017qYPmkKEsfbWY1yVg83p8F --- packages/metadata-fs/src/repository.ts | 109 +++++++++++++++++- .../test/external-write-resync.test.ts | 71 +++++++++++- 2 files changed, 175 insertions(+), 5 deletions(-) diff --git a/packages/metadata-fs/src/repository.ts b/packages/metadata-fs/src/repository.ts index 36aca6a08c..b8e3f0cb64 100644 --- a/packages/metadata-fs/src/repository.ts +++ b/packages/metadata-fs/src/repository.ts @@ -95,6 +95,17 @@ const matchEvent = (evt: MetadataEvent, filter: WatchFilter): boolean => matchRe */ const RESYNC_INTERVAL_MS = 2_000; +/** + * The ONE errno that is a truthful "there is nothing here" for a directory + * read, as opposed to "the read could not run" (#8895 — discriminate or + * propagate). A path that does not exist holds no items, so answering with an + * empty listing states a fact. Every other code — EACCES, EIO, ENOTDIR, and + * above all EMFILE/ENFILE under fd exhaustion — means the answer was never + * obtained, and inventing an empty one there is the defect itself. + */ +const isEnoent = (err: unknown): boolean => + (err as NodeJS.ErrnoException | null)?.code === 'ENOENT'; + export class FileSystemRepository implements MetadataRepository { private readonly layout: FsLayout; private readonly org: string; @@ -115,6 +126,12 @@ export class FileSystemRepository implements MetadataRepository { private resyncTimer: ReturnType | null = null; /** False before the watcher is armed and from `close()` onwards. */ private resyncEnabled = false; + /** + * Sweep read faults already reported, keyed `CODE @ path`, so a standing + * fault is announced once rather than every 2s (AGENTS.md: say it once, at + * the first degradation). An entry is cleared when that path reads again. + */ + private readonly resyncFaults = new Set(); constructor(opts: FileSystemRepositoryOptions) { this.org = opts.org; @@ -650,17 +667,91 @@ export class FileSystemRepository implements MetadataRepository { * Discovery is by content, never by stat: a stat pre-filter would reintroduce * a time key of exactly the kind this replaces. */ + /** + * Announce a sweep read that could not run — the non-silence half of #8895's + * "discriminate or propagate". + * + * ## Why `error` and not `warn` + * + * AGENTS.md decides the level with one question: *after the degradation, does + * the system still look "normal" from the outside while something it claims + * is persisted has not actually landed?* Here it does. Nothing throws, the + * watcher stays armed, `getWatched()` stays populated, `start()` succeeded — + * and the repository's index quietly stops tracking what is on disk. That is + * the rule's second limb verbatim ("persisted state and runtime state + * disagree"), not the functional-degradation limb: no capability is visibly + * smaller, so nobody finds out by using the missing thing. + * + * The counter-argument — *this is only a backstop, the watcher is still the + * fast path* — is why the level is arguable, and it does not survive the + * failing errno. The sharp case is fd exhaustion: EMFILE/ENFILE break this + * `readdir` and chokidar's `fs.watchFile` polling **at the same time and for + * the same reason**, so the fast path is not an independent fallback under + * precisely the load that produces this fault. A backstop that is silently + * absent whenever it is most needed is a durability-shaped degradation. + * + * ⚠️ AGENTS.md also warns against over-applying `error`, and the discipline + * that answers it is the ledger, not a quieter level: an `error` owes the + * consequence and the fix, said **once** at the first degradation rather than + * once per failed read. A sweep runs every 2s forever, so an unlatched + * `console.error` here would be the mirror-image failure the same rule names. + * + * ⛔ It deliberately does NOT throw. This runs on a background timer; taking + * a process down on a transient EACCES would be worse than the bug. The bar + * met here is non-silence, not propagation. + * + * The channel is `console.error` because this class has no logger: nothing is + * injected through `FileSystemRepositoryOptions`, and widening that public + * surface to carry one is out of scope for this fix. + */ + private reportResyncFault(target: string, err: unknown): void { + const code = (err as NodeJS.ErrnoException | null)?.code ?? 'UNKNOWN'; + const key = `${code} @ ${target}`; + if (this.resyncFaults.has(key)) return; + this.resyncFaults.add(key); + console.error( + `[FileSystemRepository] metadata reconciliation sweep could not read ${target} (${code}). ` + + `CONSEQUENCE: external edits under this path are no longer reconciled, so this ` + + `repository's index and its watch() subscribers can drift from what is on disk while ` + + `everything keeps reporting healthy. The chokidar watcher is not an independent ` + + `fallback here — fd exhaustion degrades both. ` + + `FIX: restore read access to the path; the sweep recovers by itself on the first ` + + `successful read. Reported once per path and error code.`, + ); + } + + /** Re-arm reporting for a path that reads again, so a recurrence is heard. */ + private clearResyncFault(target: string): void { + if (this.resyncFaults.size === 0) return; + const suffix = ` @ ${target}`; + for (const key of this.resyncFaults) { + if (key.endsWith(suffix)) this.resyncFaults.delete(key); + } + } + private async resync(): Promise { const root = this.layout.root; let entries: import('node:fs').Dirent[] = []; try { entries = await fs.readdir(root, { withFileTypes: true }); - } catch { - // A root that has gone away is not a divergence we can report on; the - // next sweep sees whatever replaces it. + this.clearResyncFault(root); + } catch (err) { + // ENOENT is truthful: a root that does not exist holds nothing to + // reconcile, and the next sweep sees whatever replaces it. Any other + // errno means the read could not RUN, and staying silent about that + // would make this backstop absent for the life of the process exactly + // when the load-dependent loss it exists to catch is most likely — + // EMFILE/ENFILE degrade this read and chokidar's own polling together. + if (!isEnoent(err)) this.reportResyncFault(root, err); return; } const onDisk = new Set(); + /** + * Type directories whose listing could not be obtained. Their keys are + * missing from `onDisk` for a reason that is NOT "the files are gone", so + * the delete pass below must not read that absence as a removal. + */ + const unreadableTypes = new Set(); for (const entry of entries) { if (!entry.isDirectory()) continue; // Same dot-entry rule as `scanHeads` and `isIgnoredWatchPath`, so the @@ -671,7 +762,15 @@ export class FileSystemRepository implements MetadataRepository { let files: string[] = []; try { files = await fs.readdir(dir); - } catch { + this.clearResyncFault(dir); + } catch (err) { + // The same discrimination at type granularity. An unreadable type + // directory silently stops reconciling EVERY item of that type, which + // is exactly the invented-emptiness shape #8895 rules on. + if (!isEnoent(err)) { + this.reportResyncFault(dir, err); + unreadableTypes.add(entry.name); + } continue; } for (const file of files) { @@ -702,6 +801,8 @@ export class FileSystemRepository implements MetadataRepository { if (onDisk.has(key)) continue; const ref = parseRefKey(key); if (!ref) continue; + // Absent from `onDisk` because we could not look, not because it is gone. + if (unreadableTypes.has(ref.type)) continue; const file = itemPath(this.layout, ref.type, ref.name); await this.mutex.run(key, async () => { // Re-checked UNDER the lock. The enumeration above ran outside it, so diff --git a/packages/metadata-fs/test/external-write-resync.test.ts b/packages/metadata-fs/test/external-write-resync.test.ts index d9a4f9c2f4..8ea7b5ecaa 100644 --- a/packages/metadata-fs/test/external-write-resync.test.ts +++ b/packages/metadata-fs/test/external-write-resync.test.ts @@ -41,7 +41,7 @@ * identified. They pin the repository's guarantee, not chokidar's behaviour. */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import fs from 'node:fs/promises'; import path from 'node:path'; import os from 'node:os'; @@ -216,6 +216,75 @@ describe('FileSystemRepository — external writes survive a lost watcher event expect(mine[0]!.op).toBe('create'); }, CASE_TIMEOUT_MS); + it('reports an unreadable type directory instead of inventing an empty one, and says it once', async () => { + await start(); + // A tracked item, so the delete pass has something it could wrongly retire. + await repo!.put(ref('kept'), { label: 'kept' }, { parentVersion: null, actor: 'tester' }); + await sleep(200); + + const errors: string[] = []; + const errSpy = vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + errors.push(args.map(String).join(' ')); + }); + // Injected at the seam rather than with `chmod 000`, which is a no-op for a + // process running as root — the shape this must survive is "the read could + // not run", not one particular way of arranging that. + const realReaddir = fs.readdir; + const readdirSpy = vi.spyOn(fs, 'readdir').mockImplementation(((p: unknown, opts?: unknown) => { + if (p === viewDir) { + const denied: NodeJS.ErrnoException = new Error(`EACCES: permission denied, scandir '${viewDir}'`); + denied.code = 'EACCES'; + return Promise.reject(denied); + } + return (realReaddir as (a: unknown, b?: unknown) => Promise)(p, opts); + }) as typeof fs.readdir); + + try { + await waitFor(() => errors.length > 0, RECOVERY_WAIT_MS); + // Keep failing across several more sweeps: a standing fault must not + // reprint every 2s for the life of the process. + await sleep(QUIET_WINDOW_MS); + } finally { + readdirSpy.mockRestore(); + errSpy.mockRestore(); + } + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain(viewDir); + expect(errors[0]).toContain('EACCES'); + // ⛔ And it must not have answered the unreadable listing with "empty": + // every item of that type would otherwise be retired as an external delete. + expect(events.filter((e) => e.op === 'delete')).toHaveLength(0); + expect(await repo!.get(ref('kept'))).not.toBeNull(); + }, CASE_TIMEOUT_MS); + + it('stays silent when the directory is genuinely gone (ENOENT), which is a truthful empty answer', async () => { + await start(); + + const errors: string[] = []; + const errSpy = vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + errors.push(args.map(String).join(' ')); + }); + const realReaddir = fs.readdir; + const readdirSpy = vi.spyOn(fs, 'readdir').mockImplementation(((p: unknown, opts?: unknown) => { + if (p === viewDir) { + const gone: NodeJS.ErrnoException = new Error(`ENOENT: no such file or directory, scandir '${viewDir}'`); + gone.code = 'ENOENT'; + return Promise.reject(gone); + } + return (realReaddir as (a: unknown, b?: unknown) => Promise)(p, opts); + }) as typeof fs.readdir); + + try { + await sleep(QUIET_WINDOW_MS); + } finally { + readdirSpy.mockRestore(); + errSpy.mockRestore(); + } + + expect(errors).toEqual([]); + }, CASE_TIMEOUT_MS); + it('retires the sweep on close(), so a closed repository schedules no further work', async () => { await start(); const priv = repo as unknown as WatcherHandle; From 76ad364b51f7546dbb21d3f64ec8a7e6a1f524e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 15:31:32 +0000 Subject: [PATCH 3/3] docs(metadata-fs): put the sweep's rationale back above resync() rather than stacked on reportResyncFault (#9339) Comment-only. The read-seam commit inserted the new helpers above resync() and left its doc block attached to the first of them, so two doc comments stacked on reportResyncFault and the sweep itself read undocumented. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017qYPmkKEsfbWY1yVg83p8F --- packages/metadata-fs/src/repository.ts | 106 ++++++++++++------------- 1 file changed, 53 insertions(+), 53 deletions(-) diff --git a/packages/metadata-fs/src/repository.ts b/packages/metadata-fs/src/repository.ts index b8e3f0cb64..d32661e0eb 100644 --- a/packages/metadata-fs/src/repository.ts +++ b/packages/metadata-fs/src/repository.ts @@ -614,59 +614,6 @@ export class FileSystemRepository implements MetadataRepository { this.resyncTimer = timer; } - /** - * Content-keyed reconciliation sweep — the backstop that makes external-edit - * detection a guarantee rather than a single chance (#9339, #7282). - * - * ## Why the watcher alone cannot be the guarantee - * - * An external write to `//.json` reaches a subscriber only - * if chokidar notices it, and under `usePolling` it gets **exactly one** - * opportunity to do so: the write advances the type directory's mtime once, - * and chokidar re-reads a directory only when its stat *strictly advances*, - * so every later poll compares an unchanged stat and can never rediscover - * the file. Measured on #9339 with a fault-injection harness: with the one - * read suppressed, fifteen further poll ticks never find the new file, and a - * 20s deadline and a 200s deadline buy the same single attempt. That is the - * structural reason behind #7282's empirical finding that the event is - * "never delivered, not slow", and why widening the deadline (#7208) and - * lowering `interval` were both spent before they were tried. - * - * At least six independent one-shot gates sit on that single attempt, - * spanning three layers — the kernel timestamp (the directory mtime does not - * strictly advance), chokidar's readdir throttle and readdir snapshot, and - * chokidar's emit gates (`_throttle('add')`, a stale `_pendingWrites` entry, - * the `awaitWriteFinish` ENOENT early return). Each one produces a - * byte-identical observable: no event, ever, for that path. - * - * ## Why this shape, and not a narrower one - * - * ⚠️ The six are indistinguishable at the point of failure, so **any fix - * that has to name which gate fired is a fix for one member of a family** — - * which is exactly how #7282 was closed and exactly why it reopened. This - * sweep never asks. It compares what is on disk against `heads`, the index - * that already defines what this repository believes it holds, and publishes - * the divergence through the same `handleFsChange` the watcher feeds. It is - * therefore robust across all six *by construction*, and equally across a - * seventh nobody has found: the only property it relies on is that the bytes - * on disk stopped matching the index. - * - * `put()` is unaffected and keeps its direct registration (`trackWrittenPath` - * calls `watcher.add` and bypasses the whole chain, which is why the `put()` - * half of this family was already closed by #7336 and the external-write half - * was not). - * - * ## Cost, and why it is bounded - * - * One pass over `//*.json` per sweep — the same walk `start()` - * already performs once — with no retry loop inside it and no work at all - * when nothing diverged. Sweeps are chained, so they cannot overlap; the - * timer is `unref`ed and dies with `close()`; and it is armed only alongside - * the watcher, so a `disableWatch` repository pays nothing. - * - * Discovery is by content, never by stat: a stat pre-filter would reintroduce - * a time key of exactly the kind this replaces. - */ /** * Announce a sweep read that could not run — the non-silence half of #8895's * "discriminate or propagate". @@ -729,6 +676,59 @@ export class FileSystemRepository implements MetadataRepository { } } + /** + * Content-keyed reconciliation sweep — the backstop that makes external-edit + * detection a guarantee rather than a single chance (#9339, #7282). + * + * ## Why the watcher alone cannot be the guarantee + * + * An external write to `//.json` reaches a subscriber only + * if chokidar notices it, and under `usePolling` it gets **exactly one** + * opportunity to do so: the write advances the type directory's mtime once, + * and chokidar re-reads a directory only when its stat *strictly advances*, + * so every later poll compares an unchanged stat and can never rediscover + * the file. Measured on #9339 with a fault-injection harness: with the one + * read suppressed, fifteen further poll ticks never find the new file, and a + * 20s deadline and a 200s deadline buy the same single attempt. That is the + * structural reason behind #7282's empirical finding that the event is + * "never delivered, not slow", and why widening the deadline (#7208) and + * lowering `interval` were both spent before they were tried. + * + * At least six independent one-shot gates sit on that single attempt, + * spanning three layers — the kernel timestamp (the directory mtime does not + * strictly advance), chokidar's readdir throttle and readdir snapshot, and + * chokidar's emit gates (`_throttle('add')`, a stale `_pendingWrites` entry, + * the `awaitWriteFinish` ENOENT early return). Each one produces a + * byte-identical observable: no event, ever, for that path. + * + * ## Why this shape, and not a narrower one + * + * ⚠️ The six are indistinguishable at the point of failure, so **any fix + * that has to name which gate fired is a fix for one member of a family** — + * which is exactly how #7282 was closed and exactly why it reopened. This + * sweep never asks. It compares what is on disk against `heads`, the index + * that already defines what this repository believes it holds, and publishes + * the divergence through the same `handleFsChange` the watcher feeds. It is + * therefore robust across all six *by construction*, and equally across a + * seventh nobody has found: the only property it relies on is that the bytes + * on disk stopped matching the index. + * + * `put()` is unaffected and keeps its direct registration (`trackWrittenPath` + * calls `watcher.add` and bypasses the whole chain, which is why the `put()` + * half of this family was already closed by #7336 and the external-write half + * was not). + * + * ## Cost, and why it is bounded + * + * One pass over `//*.json` per sweep — the same walk `start()` + * already performs once — with no retry loop inside it and no work at all + * when nothing diverged. Sweeps are chained, so they cannot overlap; the + * timer is `unref`ed and dies with `close()`; and it is armed only alongside + * the watcher, so a `disableWatch` repository pays nothing. + * + * Discovery is by content, never by stat: a stat pre-filter would reintroduce + * a time key of exactly the kind this replaces. + */ private async resync(): Promise { const root = this.layout.root; let entries: import('node:fs').Dirent[] = [];