Skip to content

fix(metadata-fs): FileSystemRepository.close() terminates every live watch() iterator (#11127) - #11198

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-11127-filesystem-close-terminates
Aug 23, 2026
Merged

fix(metadata-fs): FileSystemRepository.close() terminates every live watch() iterator (#11127)#11198
os-zhuang merged 3 commits into
mainfrom
claude/issue-11127-filesystem-close-terminates

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #11127

Before → after

Before. FileSystemRepository.close() retired the chokidar watcher and the resync sweep and stopped there. It never reached this.broker, and the broker had no teardown of its own — subscribe/unsubscribe add to and delete from a plain Set, and nothing else emptied it. Each watch() iterator parks its pending next() on a waiter that only two things can settle: a broker push, or the iterator's own terminator, which ran from iterator.return()/throw() and from nowhere else. After close() the chokidar source was gone so no push could arrive, and the subscriber was still registered with nothing left to run its terminator. A parked next() never settled.

Unlike the sibling defect in SysMetadataRepository (#11021), this was not filter-dependent — there was no drain attempt at all, so every subscription shape hung, watch({}) included. MetadataManager.startRepositoryWatch(), which awaits iter.next() in a while loop, is exactly the shape that hung.

After. The broker holds each subscription's terminator next to its event sink, and close() runs every terminator — the same routine the consumer's own iterator.return() runs. A parked next() settles with { value: undefined, done: true }, and so does every later one. Shutdown is deliberately not delivered as an event: a synthetic drain event is subject to the very filters watch() applies to real ones, and delivering an event has never ended an iterator (invariant 8). Termination runs before await watcher.close(), so a rejecting watcher.close() cannot leave a consumer's for await parked.

Measured, same test file both times (packages/metadata-fs/test/close-terminates-watch.test.ts, committed red at 3e545f41 before the fix):

metadata-fs suite
baseline, unmodified source Tests 9 failed | 56 passed (65) — every one of the nine new cases returned Symbol(still-pending) 2s after close()
after the fix (609147d8) Tests 65 passed (65)

Invariant 8's exception text — yes, it needed updating

Invariant 8 in packages/metadata-core/src/repository.ts named FileSystemRepository as the one measured non-conformance among today's three implementations. That sentence is stale as of this PR, so it is rewritten in the same change: the row now records that SysMetadataRepository conforms (#11021), FileSystemRepository conforms (#11127), InMemoryRepository offers no repository-level shutdown at all — no declared exceptions — plus a line saying a new implementation offering close() joins that list or it does not conform. No semantics were touched: the MUST, the MUST NOT, and the "events queued at that moment MAY be dropped" allowance are unchanged.

The implementation was cross-checked against SysMetadataRepository's post-#11021 shape and deliberately mirrors it — WatchSubscription { dispatch, terminate } there, BrokerSubscriber { push, terminate } here; snapshot-then-clear before terminating in both, so a terminate() that unregisters itself cannot make the set skip its neighbour; try/catch per terminator in both, so one wedged consumer cannot strand the rest. That symmetry is the whole reason the invariant exists.

One bounded repair beyond the parked-iterator case, named here because it is not in the card's text

watch() returns a deferred iterable: the subscriber is registered only once the eager log read resolves. A close() landing inside that window swept a broker the subscription had not yet joined, so the subscription arrived afterwards and parked forever — the same defect by a different route, in the same function, and a case the card's own acceptance shape cannot reach. watch() now carries the close generation it was opened under (arrivesClosed), and a subscription that arrives after a shutdown terminates on arrival. A counter rather than a boolean because start() may follow close() — a restart must not poison the watchers opened after it.

It is load-bearing, not decoration. Ablation (arrivesClosed wiring deleted, terminateAll() kept): mutation confirmed on disk (arrivesClosed occurrences 1 → 0, git diff showing the single deleted line), no dist involved — the pin imports ../src/index.js, and the one package-name import (@objectstack/metadata-core) is untouched by the mutation. Result: Tests 1 failed \| 64 passed — exactly the deferred-window case, no other. Restore leg confirmed (arrivesClosed back to 1, git status clean).

Verification — all at 609147d8, the head of this branch

  • pnpm --filter @objectstack/metadata-fs test65 passed (8 files); baseline on the same committed pin → 9 failed / 56 passed.
  • pnpm --filter @objectstack/metadata-fs --filter @objectstack/metadata-core typecheck → both Done (each runs tsc --noEmit && tsc --noEmit -p tsconfig.test.json).
  • Downstream behaviour (... prefix = dependents): the FS-repo consumers in @objectstack/metadata, including the real-LiteKernel shutdown test that drives close() through MetadataPlugin.destroy()metadata-repository-fs-dot-root, plugin-shutdown-releases-repository, plugin-no-metadata-root-on-boot10 passed (3 files).
  • Published type surface is unchanged, measured rather than asserted: built dist/index.d.ts at this HEAD vs. at origin/main's packages/metadata-fs/src — the only non-comment line in the diff is private closeGeneration;. EventBroker/BrokerSubscriber stay module-internal (index.ts exports ./repository.js, JsonlLog, FsLayout only), and nothing in the repo extends or structurally implements FileSystemRepository.
  • Gates, derived from the real change set with node scripts/pm/dispatch-gates.mjs (not from a hand-built diff), each read from the gate's own verdict line — all green: check:nul-bytes · check:changeset-gate-self-tests · check:objectui-changeset · check:slot-lookup ("ratchet holds … none new") · check:test-source-alias · check:type-source-resolution · check-adr-0087-registration · check-changeset-no-major ("introduces no major bump") · check-ci-filter-parity · check-empty-changeset · check-plugin-teardown-shape · check-affected-docs · and the convention-triggered set for a new test file: check:query-options-erasure · check:engine-double-contract ("OK — 384 pinned") · check:where-matcher · check:type-check-coverage.

Two declared narrowings

  1. pnpm lint repo-wide is CI's run. Locally: eslint --no-inline-config over the 5 changed source files → 0 errors, 0 warnings, 5 of 5 actually linted (count read from --format json, so none was silently ignored). Untouched files cannot move: this repo runs one eslint.config.mjs which never enables type-aware linting for any file ("no parserOptions.project, no typed @typescript-eslint rules", the config's own words at line 328), so no verdict elsewhere is a function of this diff.
  2. check:type-check-debt --re-measure not run locally — it requires the whole workspace built, which does not fit the container's foreground ceiling. Its population is the ledger literals in scripts/check-type-check-coverage.mjs (DEBT at line 495, TEST_DEBT at 709), and neither touched package appears in either@objectstack/metadata-fs and @objectstack/metadata-core both type-check at zero residue and carry no entry, so no counted number is a function of this diff. The one route by which a ledgered dependent (@objectstack/metadata) could drift is the published .d.ts, measured unchanged above. The structural half, check:type-check-coverage — the one a new test file outside every tsc program moves — ran green.

Not in scope

No new test declares an engine double, so assertEngineUpdateDispatch/assertEngineDeleteDispatch are not involved. packages/spec, driver-sql, packages/core/src/fallbacks/** and content/docs/releases/ are untouched. No test was skipped, disabled or quarantined; no ratchet ceiling was raised.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/metadata-core, @objectstack/metadata-fs, touching 9 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
  • 1 changed file(s) yielded no anchor (packages/metadata-core/src/repository.ts) — pages documenting those are invisible to this run
  • 3 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 3 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 52a41b72ee01bc24bdd6ac8eb1a37485eb2b4946packageMentionDocs.

Which tree this was computed on

This run read content/docs from 10783626122ea976d25f2944fc7cee9a847905bd — the merge of head 609147d823d39248d3da7536d80f5d8a7246aa54 into base 52a41b72ee01bc24bdd6ac8eb1a37485eb2b4946, 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 10783626122ea976d25f2944fc7cee9a847905bd && git checkout 10783626122ea976d25f2944fc7cee9a847905bd
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 52a41b72ee01bc24bdd6ac8eb1a37485eb2b4946 609147d823d39248d3da7536d80f5d8a7246aa54 && git checkout -B drift-repro 52a41b72ee01bc24bdd6ac8eb1a37485eb2b4946 && git merge --no-ff 609147d823d39248d3da7536d80f5d8a7246aa54

node scripts/docs-audit/affected-docs.mjs --json 52a41b72ee01bc24bdd6ac8eb1a37485eb2b4946

⚠️ 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 52a41b72ee01bc24bdd6ac8eb1a37485eb2b4946 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Aug 23, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review August 23, 2026 02:50
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 23, 2026
Merged via the queue into main with commit 46644e2 Aug 23, 2026
32 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-11127-filesystem-close-terminates branch August 23, 2026 03:05
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.

FileSystemRepository.close() never reaches its event broker — a parked watch() iterator stays parked after shutdown

2 participants