From 6ff0c6b0526210dcf3694a8c5256f9b3ae429af6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 09:44:42 +0000 Subject: [PATCH] fix(metadata-fs): confirm absence on disk before publishing an external delete A watcher unlink is a claim of absence, not absence. chokidar reaches its removal path from failed stats as well as from real removals, so under filesystem pressure it retires files that are still there; publishing those claims produced a durable delete/create pair for an item nobody removed. Confirm against the disk under the same per-key lock the reconciliation sweep already used for this, and fall through to the content path when the file is still present, so a spurious unlink alongside a real external edit surfaces as the update it always was. Part of #7369 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01DKWDdUJ2XNRESVVWUvcpnh --- .changeset/watcher-unlink-requires-absence.md | 9 + packages/metadata-fs/src/repository.ts | 58 ++++- .../external-delete-requires-absence.test.ts | 245 ++++++++++++++++++ 3 files changed, 311 insertions(+), 1 deletion(-) create mode 100644 .changeset/watcher-unlink-requires-absence.md create mode 100644 packages/metadata-fs/test/external-delete-requires-absence.test.ts diff --git a/.changeset/watcher-unlink-requires-absence.md b/.changeset/watcher-unlink-requires-absence.md new file mode 100644 index 0000000000..5fcfde9245 --- /dev/null +++ b/.changeset/watcher-unlink-requires-absence.md @@ -0,0 +1,9 @@ +--- +'@objectstack/metadata-fs': patch +--- + +The file watcher no longer publishes a `delete` for an item that is still on disk. + +A watcher `unlink` is a claim of absence, not absence: chokidar reaches its removal path from failed stats as well as from real removals, so under filesystem pressure it can retire a file that is still there. `FileSystemRepository` published those claims straight through as `delete` events — appended to the change log and broadcast to every subscriber, which drops the item from the metadata registry and the `list()` cache — and the reconciliation sweep then republished the untouched file as a `create`. A failed stat therefore produced a durable delete/create pair for an item nobody removed, with a window in between where live metadata had disappeared. + +The removal face now confirms the absence against the disk under the same per-key lock the reconciliation sweep already used for this, and an `unlink` for a path that still exists falls through to the content comparison — so a spurious unlink that arrived alongside a real external edit surfaces as the `update` it always was. Genuine external removals are unaffected and are still published on the first delivery. diff --git a/packages/metadata-fs/src/repository.ts b/packages/metadata-fs/src/repository.ts index 5585e859b6..68ee5554fd 100644 --- a/packages/metadata-fs/src/repository.ts +++ b/packages/metadata-fs/src/repository.ts @@ -901,9 +901,36 @@ export class FileSystemRepository implements MetadataRepository { * - `unlink` — `!currentHead` drops the event when the index already * agrees the item is gone. `delete()` retires the head *before* it * unlinks, precisely because this face gets no `awaitWriteFinish` delay. + * This is the whole of the *self-write* answer on this face, and it is + * not the whole of the face — see the section below it. * * Both faces are pinned together in `test/self-write-suppression.test.ts`. * + * ## A removal is confirmed against the disk before it is published (#7369) + * + * Those two checks answer "is this event OURS". Neither answers "did this + * happen at all", and the `unlink` face needs that second question asked + * because its input is a third party's inference: chokidar decides a file is + * gone from a *stat that failed*, not only from a file that went away, and + * `!currentHead` cannot tell the two apart because a spurious unlink leaves + * the index exactly as valid as it was. + * + * The cost of getting it wrong is not a dropped notification, which the + * sweep would repair. A `delete` is appended to the change log and broadcast + * to every subscriber, and `MetadataManager` drops the item from the + * registry and the `list()` cache on receipt. The sweep then finds the file + * still on disk and republishes it as a `create` — so a failed stat becomes + * a durable, permanently recorded delete/create pair for an item that never + * changed, and every consumer sees the item disappear in between. That is + * the shape ADR-0008's log is least able to walk back. + * + * So `existsSync` under the same per-key lock the sweep uses, and the same + * decision it makes: absent ⇒ publish the removal; present ⇒ this was a + * change, answered by the content path below. Genuine removals pay nothing — + * `delete()` is still suppressed by `!currentHead`, and an external `rm` is + * still published on the first delivery, because for those the file really + * is gone. Pinned in `test/external-delete-requires-absence.test.ts`. + * * Note the deliberate limit: identity is judged on what round-trips through * the file, so a spec whose in-memory form does not (a `Date`, which * canonicalises to `{}` in memory but to an ISO string once written and @@ -922,7 +949,36 @@ export class FileSystemRepository implements MetadataRepository { }; const key = refKey(ref); await this.mutex.run(key, async () => { - if (kind === 'unlink') { + // A watcher `unlink` is a CLAIM of absence, not absence — so it is + // confirmed against the disk before a `delete` is published (#7369). + // `!currentHead` inside `publishExternalDelete` cannot do this job: it + // compares against the INDEX, which is exactly what a spurious unlink + // leaves intact. The reconciliation sweep already re-checks disk truth + // under this same lock before retiring a key, for a reason it states in + // place; the watcher face was the one path that published a removal on + // the observer's word alone. + // + // chokidar reaches its removal path from failed *stats* as well as from + // real removals, and says so at both sites (chokidar 5 `handler.js`): + // `_handleFile`'s poll listener re-stats a file whose watched stat came + // back zeroed and calls `_remove` from the catch — under the comment + // "Fix issues where mtime is null but file is still present" — with no + // discrimination on errno, so EMFILE/ENFILE retires a file that is + // there; and `_handleRead`'s snapshot diff `_remove`s every previously + // tracked entry its readdirp pass did not enumerate, which includes the + // entries whose per-entry `lstat` failed rather than only the ones that + // are gone. Both faults are load-shaped, which is why the merge queue — + // the only context that runs the FULL suite — is where this surfaced, + // twice, on a case that touches nothing else. + // + // Falling through is the repair, not just skipping: when the path is + // still there the honest reading of the event is "something happened to + // this file", which is the content path's question. It answers with the + // same `currentHead === hash` comparison used everywhere else, so a + // spurious unlink that accompanied a real in-place edit still surfaces + // as the `update` it always was, in the same tick, rather than as the + // `delete` + `create` pair the index-only check produced. + if (kind === 'unlink' && !existsSync(absPath)) { await this.publishExternalDelete(ref, key); return; } diff --git a/packages/metadata-fs/test/external-delete-requires-absence.test.ts b/packages/metadata-fs/test/external-delete-requires-absence.test.ts new file mode 100644 index 0000000000..a0cffdc80c --- /dev/null +++ b/packages/metadata-fs/test/external-delete-requires-absence.test.ts @@ -0,0 +1,245 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7369 — a watcher `unlink` is a CLAIM of absence. Only the disk settles it. + * + * ## The defect + * + * `handleFsChange`'s removal face used to publish on the observer's word: + * + * if (kind === 'unlink') { await this.publishExternalDelete(ref, key); return; } + * + * and the only suppression inside `publishExternalDelete` is `!currentHead` — + * a comparison against the **index**, which a spurious unlink leaves exactly + * as valid as it was. So an event that merely *claimed* a file was gone became + * a `delete`: appended to the change log, broadcast to every subscriber, and + * acted on by `MetadataManager`, which drops the item from the registry and + * the `list()` cache on receipt. The reconciliation sweep then found the file + * still on disk and republished it as a `create`. A failed stat therefore + * produced a durable delete/create pair for an item that was never removed, + * with a window in between where live metadata had vanished. + * + * The repository already held the opposite discipline one method away: the + * sweep's delete pass re-checks `existsSync` under the per-key lock before + * retiring a key, and `external-write-resync.test.ts` pins that a false + * absence handed to the sweep must produce **zero** `delete` events. The + * watcher face was the one path that skipped the check. + * + * ## Why chokidar's unlink is not evidence of removal + * + * chokidar reaches its removal path from failed **stats** as well as from real + * removals, and both sites are load-shaped (chokidar 5, `handler.js`): + * + * - `_handleFile`'s poll listener runs when `fs.watchFile` reports a zeroed + * stat, re-stats the file, and calls `_remove` from the `catch` — under + * the comment "Fix issues where mtime is null but file is still present", + * with no discrimination on errno. EMFILE/ENFILE retires a file that is + * there. + * - `_handleRead`'s snapshot diff `_remove`s every previously tracked entry + * that its readdirp pass did not enumerate — which includes entries whose + * per-entry `lstat` failed, not only entries that are gone. + * + * That is why this surfaced in the merge queue and nowhere else: the queue is + * the only context that runs the FULL suite, and `watch-dot-root.test.ts` case + * 1 has been ejected from it twice by a delivery fault it did not cause. The + * second ejection (2026-08-27, run 33057527457, `Test Core (2/6)`) failed with + * `expected 'delete' to be 'update'` at `watch-dot-root.test.ts:268` — the + * exact-count assertion on the line above it PASSED, so exactly one event + * arrived, typed `delete`, for a file the case never removes. + * + * ## Why these cases assert at the handler seam + * + * The mechanism upstream is a failed stat under resource pressure, which + * cannot be summoned on demand and would be a wall-clock race to wait for. + * This package has now been ejected from the merge queue three times by + * wall-clock watcher assertions, so the cases follow the discipline + * `self-write-suppression.test.ts` set for the same reason: enter at the seam + * immediately below chokidar, with exactly the arguments a spurious unlink + * delivers, and assert the contract rather than the race. + * + * The contract, stated once: + * + * **a removal is published only for an item that is actually absent from + * disk; an `unlink` for a path that is still there is a change.** + * + * Both directions are asserted, because each alone has a trivial wrong fix: a + * repository that published nothing on `unlink` would pass the first two cases + * and fail the third, and today's code passes the third and fails the first + * two. + */ + +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)); + +/** + * Enter the watcher handler exactly as chokidar's `unlink` listener does — + * `w.on('unlink', (p) => void this.handleFsChange(p, 'unlink'))` in + * `startWatcher`. The same reach is made by `self-write-suppression.test.ts`. + */ +type Kind = 'add' | 'change' | 'unlink'; +const deliver = (repo: FileSystemRepository, file: string, kind: Kind): Promise => + (repo as unknown as { + handleFsChange(p: string, k: Kind): Promise; + }).handleFsChange(file, kind); + +/** + * Enough turns for anything the delivery above would publish to reach the + * subscriber. It is not a race budget: `deliver` is awaited, so the publish has + * already happened or is never going to — this only lets the broker's queue + * drain into the array. + */ +const drain = () => sleep(50); + +describe('#7369 an external delete is published only for an item that is really gone', () => { + let root: string; + let repo: FileSystemRepository | null = null; + let events: MetadataEvent[] = []; + let stop: (() => Promise) | null = null; + + /** + * The watcher is disabled throughout: these cases supply the event + * themselves, and a live poller would race them. It also retires the + * reconciliation sweep (armed inside `startWatcher`), so every event + * observed here came from the delivery the case made — which is the whole + * point, since the sweep is precisely what used to paper over the defect by + * republishing the item as a `create` two seconds later. + */ + async function start(): Promise { + repo = new FileSystemRepository({ root, org: 'system', disableWatch: true }); + await repo.start(); + const iter = repo.watch({ org: 'system' }, 999)[Symbol.asyncIterator](); + let stopped = false; + void (async () => { + while (!stopped) { + const next = await iter.next(); + if (next.done) return; + events.push(next.value as MetadataEvent); + } + })(); + stop = async () => { + stopped = true; + await iter.return?.(undefined); + }; + } + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'os-unlink-claim-')); + events = []; + }); + + afterEach(async () => { + if (stop) await stop(); + stop = null; + if (repo) await repo.close(); + repo = null; + await fs.rm(root, { recursive: true, force: true }); + }); + + it('an unlink for a path that still exists, edited externally, is the update it always was', async () => { + await start(); + const r = ref('case_grid'); + const file = path.join(root, 'view', 'case_grid.json'); + const first = await repo!.put(r, { label: 'original' }, { + parentVersion: null, + actor: 'tester', + }); + // Let `put()`'s own event land before clearing, so it cannot arrive + // afterwards and be read as something the delivery below produced. + await drain(); + events.length = 0; + + // The queue failure, reproduced at the seam: an external in-place edit + // lands, and the delivery chokidar makes for it is `unlink`. + await fs.writeFile(file, JSON.stringify({ label: 'externally edited' }, null, 2) + '\n'); + await deliver(repo!, file, 'unlink'); + await drain(); + + expect(events).toHaveLength(1); + // Pre-fix this was 'delete'. That is the assertion that ejected two + // unrelated PRs from the merge queue. + expect(events[0]!.op).toBe('update'); + expect(events[0]!.ref.name).toBe('case_grid'); + expect(events[0]!.source).toBe('fs'); + expect(events[0]!.actor).toBe('fs'); + expect(events[0]!.parentHash).toBe(first.version); + expect(events[0]!.hash).not.toBeNull(); + + // And the item is still there — the index was never retired behind the + // subscriber's back. + const item = await repo!.get(r); + expect(item).not.toBeNull(); + expect(item!.body).toEqual({ label: 'externally edited' }); + }); + + it('a purely spurious unlink — nothing on disk changed — publishes nothing at all', async () => { + await start(); + const r = ref('untouched'); + const file = path.join(root, 'view', 'untouched.json'); + const first = await repo!.put(r, { label: 'stable' }, { + parentVersion: null, + actor: 'tester', + }); + // Let `put()`'s own event land before clearing, so it cannot arrive + // afterwards and be read as something the delivery below produced. + await drain(); + events.length = 0; + + // The pure fault: chokidar's stat failed, the file never moved. + await deliver(repo!, file, 'unlink'); + await drain(); + + // Not "a delete then a create". Nothing happened, so nothing is published, + // and no phantom pair is written to the change log for a consumer to + // replay forever. + expect(events).toEqual([]); + const item = await repo!.get(r); + expect(item).not.toBeNull(); + expect(item!.hash).toBe(first.version); + + // The change log must be equally clean: `delete` is durable, and the + // history is what a consumer rebuilding from disk reads. + const history: MetadataEvent[] = []; + for await (const evt of repo!.history(r, {})) history.push(evt); + expect(history.map((h) => h.op)).toEqual(['create']); + }); + + it('a genuine external removal is still published on the first delivery', async () => { + await start(); + const r = ref('really_gone'); + const file = path.join(root, 'view', 'really_gone.json'); + const first = await repo!.put(r, { label: 'here' }, { + parentVersion: null, + actor: 'tester', + }); + // Let `put()`'s own event land before clearing, so it cannot arrive + // afterwards and be read as something the delivery below produced. + await drain(); + events.length = 0; + + // Somebody ran `rm`. The file is absent, so the claim is true. + await fs.rm(file); + await deliver(repo!, file, 'unlink'); + await drain(); + + // ⚠️ The complementary direction, and the reason the fix is a disk check + // rather than "ignore unlink": without this case, dropping every unlink + // would pass the two above. External removals are a real capability — + // `external-write-resync.test.ts` pins the sweep's recovery of one. + expect(events).toHaveLength(1); + expect(events[0]!.op).toBe('delete'); + expect(events[0]!.ref.name).toBe('really_gone'); + expect(events[0]!.hash).toBeNull(); + expect(events[0]!.parentHash).toBe(first.version); + expect(events[0]!.source).toBe('fs'); + expect(events[0]!.actor).toBe('fs'); + + expect(await repo!.get(r)).toBeNull(); + }); +});