Skip to content

fix(metadata-fs): register written paths with the watcher instead of trusting its scan (#7282) - #7336

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-7282-selfwrites-content-keyed
Aug 10, 2026
Merged

fix(metadata-fs): register written paths with the watcher instead of trusting its scan (#7282)#7336
os-zhuang merged 1 commit into
mainfrom
claude/issue-7282-selfwrites-content-keyed

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #7282

The proposed mechanism is falsified; the real one is worse

The card's mechanism — a put() and a later external edit coalescing into one
poll event that is then swallowed by the fixed 200 ms selfWrites timer — does
not happen. Measured, instrumented, over 360+ iterations under deliberate
event-loop starvation
:

selfWrites still present at external-edit time:                0/360
handleFsChange entries hitting the selfWrites suppression:     0

The suppression never fired once, in passing or failing iterations alike. It
cannot fire in these cases: the suppression timer is scheduled before the
test's own pre-edit sleep() and with a shorter delay, and Node runs expired
timers in due-time order in a single timers-phase pass — so the moment the
sleep resolves, the suppression entry is provably already gone.

What the failing iterations actually show is that handleFsChange is never
entered at all
, because chokidar never emits anything for the path.

The confirmed mechanism

Reproduced locally at ~4% (7 failures in 168 iterations) by running six probe
processes concurrently on a 4-core box with a busy-loop starving each event
loop. The instrumented timeline of a failure, taken from chokidar's own
internals:

[   7ms] start() done watcher=armed
[   7ms] put() begin
[  14ms] _handleRead CALL   probe3-zN8D48                 (root)
[  20ms] _handleRead DONE   probe3-zN8D48
[  20ms] _watchWithNodeFs   probe3-zN8D48
[  29ms] _handleRead CALL   probe3-zN8D48/view
[  30ms] _handleRead DONE   probe3-zN8D48/view -> items=[]   ← read EMPTY
[  30ms] _watchWithNodeFs   probe3-zN8D48/view               ← baseline taken
[  31ms] put() done                                          ← rename had landed
[ 565ms] external write BEGIN
[1316ms] external write DONE
[1566ms] chokidar RAW change probe3-zN8D48                (the ROOT, not view/)
[7583ms] FINAL getWatched={... "probe3-zN8D48/view":[] }   ← still empty, 7.5s later

chokidar's initial scan is asynchronous, and every write path in this file can
run 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 (#7000). With usePolling that produces a
permanently-blinding interleaving:

  1. chokidar reads the type directory 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 the directory and libuv takes its polling
    baseline stat, which already reflects step 2.

The directory's stat then never changes again. No poll ever fires for it,
_handleRead never re-runs, the item file is never added to the watched set,
no per-file watcher is created, and parent.has(basename) stays false — which
_handleFile's listener requires before it will emit change at all. chokidar
emits neither add nor change for that path for the life of the process.

This fits every observation on the card, and explains the two spent routes
rather than merely agreeing with them:

  • 20 s and 25541 ms deadlines changed nothing — there is no later event to
    wait for.
  • A 400 ms pre-edit sleep changed nothing — the damage is done at
    watcher-arm time, before the sleep starts.
  • Lowering interval (direction 3) would change nothing either — a shorter
    poll re-compares against the same unchanged directory stat. This is stronger
    than the card's "narrows the window without closing it": for this mechanism
    the effect is exactly zero.
  • Green in isolation — the losing interleaving needs the rename to fall
    between a readdirp stream end and a watchFile() call, a gap that only
    opens up under scheduling pressure.

This is not only a test flake. In production MetadataPlugin attaches the
repository and the first put() can race the same scan, after which
MetadataManager.subscribe() silently never sees out-of-process edits to that
item — a hand edit, or a git checkout bringing metadata JSON in.

The change

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 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. No timer is widened, and no consumer-side tolerance is added.

add() is idempotent here (_handleFile returns early when the parent already
tracks the basename) and emits nothing (chokidar treats an explicit add() as
an initial add, and ignoreInitial is set). Its effect is the one needed:
_watchWithNodeFs registers the basename with the parent directory and starts
the per-file poll.

File surface (the real one)

  • packages/metadata-fs/src/repository.ts — one call in put() plus the new
    private trackWrittenPath. The watcher options are unchanged.
  • packages/metadata-fs/test/watch-write-registration.test.ts — new pin.
  • .changeset/metadata-fs-watch-write-registration.md.

selfWrites and the chokidar ignored matcher (#7150) are untouched. No
quarantine was needed, so packages/metadata's suite is unchanged too.

Verification under load, not in isolation

A green local run proves nothing here, so the fix was measured against the
reproduction rather than against a quiet checkout. Identical harness, identical
load, my own base as the control:

build failures
base (origin/main @ 62b6a2fb2) 7 / 168
with the fix 0 / 360

Plus the real suites: packages/metadata-fs and packages/metadata run
concurrently, six rounds, with three CPU burners saturating the box —
31/31 and 593/593 every round, 0 failed rounds.

Reverse verification

Predicted: reverting the fix turns the new pin red on the registration
assertion, and does so deterministically rather than under load — the pin
measures registration latency, and the base's only route to registration is a
1000 ms poll tick that the case deliberately phase-anchors away from.

Result, 5 consecutive runs on base:

× registers a put() path with the watcher without waiting for a poll
AssertionError: expected [ 'anchor.json', 'seed.json' ] to include 'fresh.json'
Tests  1 failed | 30 passed (31)

5/5 red, same assertion, same message. So this red is not load-dependent,
unlike the defect it stands for — that difference is the point of the design and
is written into the test's header.

Honest categories:

  • Missed prediction. The pin's first draft made its anchor write immediately
    after start() and the event never arrived — I read that as a second
    manifestation of the defect. It is not: a file that lands while chokidar is
    still walking is treated as pre-existing and, under ignoreInitial, correctly
    emits nothing (measured 3/3 with the write at delay 0, seen 3/3 at delay
    5 ms or after ready). The anchor now waits for chokidar's ready, and the
    finding is recorded in the test header.
  • Green in both directions (a guard, not evidence). The case's closing
    liveness control — an external edit to the freshly written path must reach
    subscribers — passes with and without the fix on a quiet machine. It is
    labelled as a control in the file.
  • Left unmeasured. I did not attempt to reproduce the failure inside the
    merge queue itself; the local reproduction is a proxy chosen to match the
    described conditions (full-monorepo concurrency), not the queue.

Not folded in


Generated by Claude Code

…trusting its scan (#7282)

`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 `<root>/<type>/` 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw
@vercel

vercel Bot commented Aug 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 10, 2026 7:23am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-fs.

1 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx (via @objectstack/metadata-fs)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

Copy link
Copy Markdown
Contributor Author

ACCEPT — PM review (step 7), anchored to head 0910427fbf7e64e25bfdea16bf34186a97d6ce90

File surface vs declaration

Declared three files; the diff is exactly those three, nothing else:

file status matches declaration
packages/metadata-fs/src/repository.ts modified yes — one call in put() + the new private trackWrittenPath; watcher options untouched
packages/metadata-fs/test/watch-write-registration.test.ts added yes — the pin
.changeset/metadata-fs-watch-write-registration.md added yes

selfWrites is verifiably untouched (its setTimeout(… delete) is context, not a changed line). The chokidar ignored matcher (#7150) is untouched. No content/docs/releases/ change, no docs/adr/** change, no quarantine, no consumer-side tolerance widened anywhere.

CI, per job

25 check runs, all completed: 23 success, 2 skipped (Build Docs, Console Pin Gate — both path-filtered and correctly inert for a non-docs PR). No failure, no neutral, nothing still running. Named gates: ESLint success, TypeScript Type Check success, Build Core success, Test Core 3/3 + rollup success, Dogfood Regression Gate 3/3 + rollup success, Dogfood Verify CLI success, Temporal Conformance (live PG + MySQL) success, Check Changeset success, ADR maintainer approval success.

What I am accepting, stated plainly

The dev did not implement the direction the card recommended — it falsified the card's mechanism first. The card proposed that a put() and a later external edit coalesce into one poll event which the fixed 200 ms selfWrites timer then swallows, and named content-keying that suppression as direction 1. Measurement says that never happens: 0/360 iterations had a selfWrites entry alive at external-edit time and the suppression branch was entered 0 times, in failing and passing iterations alike — with an argument for why it cannot fire here (the timer is scheduled before the test's own pre-edit sleep() and with a shorter delay, and Node drains expired timers in due-time order). In the failures handleFsChange is never entered at all. The branch name claude/issue-7282-selfwrites-content-keyed is therefore a self-declared misnomer; it is left as-is because renaming a branch mid-PR costs more than the confusion it saves, and this comment plus the PR body are the record.

The mechanism it found instead is strictly worse than the card's, and I accept the evidence for it. chokidar's asynchronous initial scan races the repository's own first write: the type directory is read while writeJsonAtomic's rename has not landed, then watchFile() takes that directory's polling baseline after it lands, so the stat never changes again, the directory is never re-read, parent.has(basename) stays false, and neither add nor change is ever emitted for that path for the life of the process. getWatched() reporting the type directory as [] for 7.5 s with the file sitting in it is the observation that settles it. This explains the two mitigations already spent on this family rather than merely agreeing with them — a 20 s deadline and a 25541 ms deadline both waited on an event that was never coming — and it converts direction 3 from "narrows the window" to effect exactly zero, since a shorter poll re-compares the same unchanged stat. Two claims about the codebase I had assumed and that this corrects: I treated the flake as bounded by the test harness, and I treated the poll interval as a tunable that would at least help. Both wrong.

It is a production defect, not only a test flake. MetadataPlugin attaches the repository and the first put() can race the same scan, after which MetadataManager.subscribe() silently never sees out-of-process edits to that item — a hand edit, or a git checkout bringing metadata JSON in. The changeset carries that as the user-visible effect, at patch, which is the right framing and the right bump.

Verification was done under load, as I required, not in isolation. Base origin/main @ 62b6a2fb2 failed 7/168 probe iterations with the CI signature; with the fix, 0/360 under identical harness and identical load, plus six concurrent rounds of the real metadata-fs and metadata suites (31/31, 593/593, zero failed rounds) with the box saturated. The control is the dev's own base, not a moving origin/main.

Reverse verification is honest in all four categories. Predicted red, and predicted it would be deterministic rather than load-dependent because the pin phase-anchors the poll — measured 5/5 red on the same assertion and the same message. One missed prediction reported rather than buried: the pin's first draft anchored immediately after start() and saw nothing, which the dev initially read as a second manifestation of the defect; it is not — a file landing during the initial walk is treated as pre-existing and under ignoreInitial correctly emits nothing (3/3 at delay 0, 3/3 the other way at 5 ms or after ready), so the anchor now waits for ready and the finding is written into the test header. One assertion green in both directions — the closing external-edit liveness check — is labelled in the file as a control, not as evidence. One item left unmeasured and said so: the failure was not reproduced inside the merge queue itself; the local reproduction is a stated proxy.

Splitting #7335 out was the right call and I am endorsing it explicitly. The selfWrites window is time-keyed and genuinely can lose an event when an external edit lands within ~60 ms of a put() with a poll tick in between — but it is a different defect, still unobserved in the wild, and folding it in would have left this PR's reverse verification attributable to two changes instead of one. Filed, not fixed here.

Ruling

ACCEPT. Flipping out of draft and enabling auto-merge (SQUASH); it lands through the merge queue in the normal way. This closes the head-of-queue flake that ejected verified-green PRs four times across three PRs in roughly two hours.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 10, 2026 07:43
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 10, 2026
Merged via the queue into main with commit ab07b53 Aug 10, 2026
26 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-7282-selfwrites-content-keyed branch August 10, 2026 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

metadata-fs watcher tests are merge-queue flaky, and #7208's 20s deadline hardening did NOT fix it — the event is suppressed, not late

2 participants