Skip to content

fix(metadata-fs): give an external write more than one delivery attempt — content-keyed reconciliation behind the watcher poll (#9339) - #9656

Merged
os-sam merged 3 commits into
mainfrom
claude/issue-9339-external-write-delivery-retry
Aug 18, 2026
Merged

fix(metadata-fs): give an external write more than one delivery attempt — content-keyed reconciliation behind the watcher poll (#9339)#9656
os-sam merged 3 commits into
mainfrom
claude/issue-9339-external-write-delivery-retry

Conversation

@os-sam

@os-samos-sam commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Fixes#9339

Read-seam discrimination in the sweep (#8895 — discriminate or propagate)

Added after PM review. Both readdir seams in resync() originally answered every errno with "there is nothing here":

try{entries=awaitfs.readdir(root,{withFileTypes: true});}catch{return;}try{files=awaitfs.readdir(dir);}catch{continue;}

That is invented emptiness, at root and at type granularity — an unreadable type directory silently stopped reconciling every item of that type. The second catch had no rationale at all; the first reasoned about ENOENT and swallowed EACCES, EIO and EMFILE/ENFILE with it.

The sharp case is fd exhaustion. This sweep is a backstop for a load-dependent loss, and EMFILE/ENFILE degrade this readdir and chokidar's own fs.watchFile polling at the same time and for the same reason. Swallowing it made the backstop silently absent for the life of the process exactly under the conditions it exists to catch — the failure mode of the repair was the failure mode it repairs.

Dispositions.ENOENT is the one truthful empty answer — a path that does not exist holds no items — and stays silent, with the errno named in the comment rather than the English paraphrase. Every other code means the read could not run, and is reported.

⛔ 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.

Level: error, argued rather than assumed. AGENTS.md decides it 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, and the repository's index quietly stops tracking disk. That is the rule's second limb verbatim ("persisted state and runtime state disagree"), not the functional limb — no capability is visibly smaller, so nobody finds out by using the missing thing. The honest counter-argument is that this is only a backstop and the watcher is still the fast path; it does not survive the failing errno, because under EMFILE the fast path is not an independent fallback. AGENTS.md's warning against over-applying error is answered by the ledger, not a quieter level: faults are latched per CODE @ path, so a standing fault is said once and re-armed only when that path reads again — an unlatched report would fire every 2s, the mirror-image failure the same rule names. The message carries the consequence and the fix, as the rule requires. (This neither pre-empts nor contradicts #9609's open warn-vs-error question about driver-sql's boot widening — different site, same rule applied on its own facts.)

Channel.console.error. FileSystemRepository has no logger: nothing is injected through FileSystemRepositoryOptions, and widening that public surface to carry one is out of scope here. Flagging rather than inventing a channel.

One correctness consequence, fixed with it. A type directory that could not be read is now excluded from the delete pass. Without that, its keys are missing from the on-disk set for a reason that is not "the files are gone", and every item of that type would be retired as an external delete — inventing emptiness a second time, with data-shaped consequences.

⚠️The gate's silence was not evidence.READ_SEAM_SCAN_ROOTS in scripts/check-durability-degradation-log-level.mjs is packages/metadata/src, packages/metadata-protocol/src, packages/objectql/srcpackages/metadata-fs is outside all three. Widening the roots would not have caught it either: the read vocabulary is anchored to IDataDriver's find/findOne/count, so an fs.readdir seam is invisible to the rule at any scope. That check:durability-log-level did not appear in the derived gate union is consistent with the scan roots, not with the code being clean.

The defect is structural: an external write gets exactly ONE delivery attempt

FileSystemRepository's watcher gave an externally-written file one chance to be noticed, and losing that chance was permanent and silent for the life of the process. Under usePolling, chokidar re-reads a directory only when its stat strictly advances. An external write advances the type directory's mtime once, so polls 2..N compare an unchanged stat and can never rediscover the file.

Re-measured on today's main with the #9339 fault-injection harness: with that single read suppressed, 20 further poll ticks never find the fileeventsDelivered: [], anchorInWatchedSet: false, pollsWaited: 20. A 20s deadline and a 200s deadline buy the same one attempt. That is the mechanism behind #7282's empirical "the signature of an event that is never delivered, not one that is slow", and it is 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, chokidar's readdir throttle and readdir snapshot, and chokidar's emit gates. Each produces a byte-identical observable: no event, ever, for that path. They are indistinguishable at the point of failure, which is exactly why #7282 was closed on one member of the family and reopened as this card.

put() was never exposed: trackWrittenPath calls watcher.add directly and bypasses the whole chain. PR #7336 shut the put() half by routing around the fragile path rather than repairing it, which is why the external-write half stayed live.

The fix never asks which gate fired

A bounded, content-keyed reconciliation sweep runs alongside the watcher and compares what is on disk against heads — the index that already defines what this repository believes it holds — publishing any divergence through the same handleFsChange the watcher feeds. Its only premise is that the bytes on disk stopped matching the index.

⭐ That is what makes it mechanism-independent: if all six gates are one-shot gates on a single attempt, a second attempt keyed on content is robust across all six by construction, and equally across a seventh nobody has found. Nothing in the design branches on which gate fired — the property the card asked to preserve.

  • Cadence — one pass over ROOT/TYPE/NAME.json every 2s (twice the poll interval), the same walk start() already performs once. Sweeps are chained, not intervalled, so they can never overlap or stack behind a slow disk; a saturated runner degrades to fewer sweeps, not a backlog. The timer is unrefed 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 (metadata-fs: selfWrites suppression is time-keyed, so a poll tick landing inside the 200 ms window can swallow an external edit #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, actor, because they are produced by the same code.
  • A recovered path is re-armed with the watcher through the seam put() already uses, so a loss upstream of _handleFile does not leave that file dependent on the sweep forever. It is called only when a divergence was actually published, so it cannot muddy test(metadata-fs): triage snapshot on the #9339 anchor-event failure message #9400's watched-set triage message.
  • Discovery is by content, never by stat: a stat pre-filter would reintroduce a time key of exactly the kind this replaces.

The delete face of handleFsChange was extracted verbatim as publishExternalDelete so the sweep reuses it instead of growing a second copy of the event shape. The sweep's delete half re-checks file existence under the per-key mutex, because its enumeration runs outside the lock and a concurrent put() would otherwise be reported as an external delete.

Injector table — pre-fix and post-fix, mode by mode

Same harness, today's tree, --wait=6000. Pre-fix rows measured against origin/main's repository.ts; post-fix rows 3/3 runs each at 5936072c1, re-confirmed 7/7 at the final commit 76ad364b5 (which is comment-only on top of it).

forced gatepre-fix eventspre-fix in watched setpost-fix eventspost-fix in watched set
none (control)["anchor:create"]yes["anchor:create"]yes
mtime-tie[]no["anchor:create"]yes
readdir-throttle[]no["anchor:create"]yes
readdirp-miss[]no["anchor:create"]yes
add-throttle[]yes["anchor:create"]yes
pending-write[]yes["anchor:create"]yes
awf-enoent[]yes["anchor:create"]yes

No mode is still live. Every row delivers exactly one event, so the sweep does not double-publish in the control either.

Permanence, readdir-throttle --wait=20000 (throttle released at 5s, 20 poll ticks): pre-fix []; post-fix ["anchor:create"].

Latency, n=8 each: watcher fast path median 1067ms (8/8), backstop with the watcher blinded median 2002ms (8/8). The watcher still wins every healthy run, so the pin test's poll-phase anchoring is unchanged — its own execution time measured 2.03s, identical across runs.

⚠️ The bound on this claim

The six gates are forced fault injections, not the CI mechanism, which was never identified and may be a seventh. The claim this PR makes is:

the fix converts six of six forced one-shot gates from permanent loss to delivery.

⛔ It does not claim the flake is fixed. Local green proves nothing on this card — the prior seats measured 3/3, 6/6, 6/6, 8/8 and 48 runs under 6-way CPU starvation, all green on the broken code. Only the injector discriminates, and the injector only knows the gates somebody thought of.

What is not ruled out: which gate (if any of these six) fires in CI; whether a seventh exists; and whether the sweep's own tree walk can itself be starved past a subscriber's deadline on a saturated runner. The design answer to the last one is that it retries while the repository is open rather than getting one attempt, but no CI measurement backs that.

Reverse verification — four ablation legs

Instrument leg — the table above, both directions, with dist/ proven at each leg by scripts/ablation-dist-preflight.mjs (--absent on the ablated leg, present on the restored leg).

Ordinary leg — reverted only the fix seam (git checkout origin/main -- packages/metadata-fs/src/repository.ts), rebuilt, confirmed the marker was absent from dist/, ran the new pin file:

× recovers an external create, update and delete the watcher never delivered 15046ms
AssertionError: expected [] to have a length of 1 but got +0
× retires the sweep on close(), so a closed repository schedules no further work 10ms
AssertionError: expected undefined to be true
Tests 2 failed | 1 passed (3)

Predicted vs observed matched, including the case that stayed green: "publishes each external change exactly once, and never republishes our own put()"passes under ablation, because it is a non-regression guard against over-delivery, not a pin of the fix.

Read-seam legs — each ablation bites its own case and only its own case:

ablationpredictedobserved
type-dir catch back to a silent swallowthe EACCES case goes red× reports an unreadable type directory … and says it onceAssertionError: expected [] to have a length of 1; ENOENT case still green
report every errno (drop the ENOENT discriminator)the ENOENT case goes red× stays silent when the directory is genuinely gone (ENOENT)AssertionError: expected [ Array(1) ] to deeply equal []; EACCES case still green

Restored with git checkout HEAD -- after each leg, git diff HEAD empty, rebuilt, marker present, full suite green.

⚠️ Finding: one injector mode had stopped reproducing, and the instrument was at fault

On first re-run against today's tree, mtime-tie delivered ["anchor:create"] — 5/5 — where the prior baseline recorded []. The gate had not closed; the injection had.fs.utimes(dir, Date, Date) truncates the sub-millisecond mtime to a Date and the seconds-float round-trip rounds it back up, so the forced "tie" landed above the pre-write mtime and chokidar's currmtime greater-than prev.mtimeMs filter still fired. Measured directly: before=…409.5535afterUtimes=…410, and the harness's own dirMtimeAdvanced field read true.

Passing numeric seconds floored strictly below the pre-write value restores it: [], 3/3, dirMtimeAdvanced: false. Both readings are in the table above; the repair is one line in the throwaway harness and no product code depends on it. Recording it because the un-repaired mode reads exactly like "that gate is already fixed" — the false-green shape this card exists to avoid.

What is deliberately untouched

  • ⛔ No quarantine, skip, .skip, .todo, retry-wrapping or disabling of watch-write-registration.test.ts. That file is byte-identical.
  • EVENT_WAIT_MS stays at 20_000.
  • ⛔ No production constant tuned: awaitWriteFinish: {stabilityThreshold: 50, pollInterval: 20}, usePolling: true, interval: 1000, binaryInterval: 2000 are all byte-identical.
  • test(metadata-fs): triage snapshot on the #9339 anchor-event failure message #9400's failure-message snapshot is not re-added; it is already on main as e0ff5b742.
  • put()'s direct registration is unchanged, and so is scanHeads — the disambiguation matters, because scanHeads holds a readdir block textually identical to the sweep's.
  • No packages/spec file, no contract accept/reject behaviour, no error-code ledger entry. No public surface widened — the generated dist/index.d.ts gains only private members; FileSystemRepositoryOptions is unchanged.
  • Docs: the docs-drift advisory names content/docs/concepts/metadata-lifecycle.mdx. Read it — FileSystemRepository appears there in a package-location table, a shipped-status row, and the FS-overlay row saying an edited .json is appended to the change log. None of that describes how delivery is detected, and every documented statement still holds verbatim: the same event, the same shape, the same log. No change needed.

Verification

Gate union re-derived at the final commit 76ad364b5 with node scripts/pm/dispatch-gates.mjs (no path arguments, off the merge base b057e53f4), working tree clean, unchanged at 3 paths. Each gate captured as cmd > log 2>&1; EXIT=$? and read from the log.

gateEXITreading
pnpm --filter '@objectstack/metadata-fs' test -- --maxWorkers=207 files / 56 tests passed (was 6 / 51 on main)
pnpm --filter '@objectstack/metadata-fs' typecheck0tsc --noEmit + tsc --noEmit -p tsconfig.test.json
pnpm exec eslint on both changed files0clean, no warnings
node scripts/check-nul-bytes.mjs06173 text files, no raw control bytes
node scripts/check-adr-0087-registration.mjs0no declared-breaking changeset
node scripts/check-changeset-no-major.mjs0no major bump
node scripts/check-empty-changeset.mjs01 declaring changeset added
node scripts/docs-audit/check-affected-docs.mjs0242 self-test cases
pnpm check:changeset-gate-self-tests0446 assertions across the three self-tests
pnpm check:objectui-changeset0all checks passed
pnpm check:query-options-erasure0ratchet holds, 67 unswept sites, none new
pnpm check:engine-double-contract0319 pinned / 133 DEBT / 2 exempt, none new
pnpm check:where-matcher0255 matchers, 0 silently-wrong, none new
pnpm check:type-check-coverage064/77 packages type-checked, self-test green
check-type-check-coverage.mjs --re-measure033 ledger entries re-measured in 353.1s, none above its recorded number (workspace closure built first, 70/70 turbo tasks)

Also swept the direct downstream consumer, pnpm --filter '@objectstack/metadata' test — 31 files / 603 tests passed, EXIT 0.

Premise re-verification

The card's premise holds on today's main (b057e53f4, ~20 merges past the investigation branch): the one-attempt structure, the six-gate family, and the permanence at 20 poll ticks all reproduce. e0ff5b742 landed after that branch was cut and touches only the failure message, which this PR leaves in place.


Generated by Claude Code

…pt — content-keyed reconciliation behind the watcher poll (#9339)
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qYPmkKEsfbWY1yVg83p8F
@github-actions

github-actionsBot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-fs, touching 14 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via FileSystemRepository (symbol))
What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)

Coarse fallback — 1 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 3b3f67d31073cb8fcd2f92b2fb642a0cfb6146c6packageMentionDocs.

Which tree this was computed on

This run read content/docs from 2c89ebacb7c3c138e3018a76d13ab38032e974b6 — the merge of head 76ad364b51f7546dbb21d3f64ec8a7e6a1f524e0 into base 3b3f67d31073cb8fcd2f92b2fb642a0cfb6146c6, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 2c89ebacb7c3c138e3018a76d13ab38032e974b6 && git checkout 2c89ebacb7c3c138e3018a76d13ab38032e974b6
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 3b3f67d31073cb8fcd2f92b2fb642a0cfb6146c6 76ad364b51f7546dbb21d3f64ec8a7e6a1f524e0 && git checkout -B drift-repro 3b3f67d31073cb8fcd2f92b2fb642a0cfb6146c6 && git merge --no-ff 76ad364b51f7546dbb21d3f64ec8a7e6a1f524e0
node scripts/docs-audit/affected-docs.mjs --json 3b3f67d31073cb8fcd2f92b2fb642a0cfb6146c6

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 3b3f67d31073cb8fcd2f92b2fb642a0cfb6146c6 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 18, 2026
…lures instead of inventing an empty listing (#8895)
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qYPmkKEsfbWY1yVg83p8F
…er 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qYPmkKEsfbWY1yVg83p8F
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sam@claude