Skip to content

fix(metadata-protocol): close() terminates watch iterators instead of emitting a drain event - #11136

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-11021-close-drains-filtered-watchers
Aug 22, 2026
Merged

fix(metadata-protocol): close() terminates watch iterators instead of emitting a drain event#11136
os-zhuang merged 1 commit into
mainfrom
claude/issue-11021-close-drains-filtered-watchers

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#11021

SysMetadataRepository.close() modelled shutdown as a metadata event, and an event is exactly what a filtered or numeric-since subscriber is entitled to drop. It broadcast a synthetic { seq: -1, ref: { org: '', type: 'view', name: '_close' } } through the same dispatch closure real events pass, then cleared the watcher registry. Both of that closure's guards reject it, and once the registry is cleared nothing else can settle the parked promise.

The design fork, decided by measurement

Triage recommended shape 2 (close() resolves pending iterators with done: true directly) as a route to verify, with an explicit stop-condition: if a real consumer depends on receiving the synthetic drain event, shape 1 (exempt it from the filters) is the compatible answer instead. Nothing depends on receiving it:

evidencemeasurement
the _close sentinelappears in exactly one place in the tree — the producer at sys-metadata-repository.ts:1228. No consumer names it, counts it, or branches on it.
MetadataManager.startRepositoryWatch() (repo.watch({}), the drained subscriber)hands it to applyRepoEvent(), which cannot tell it from a real one: invalidateForForeignWrite('view', '_close'), then notifyWatchers() re-emits it downstream as a deletedview named _close at seq: -1 — into the HMR SSE route and Studio's status badge. It is misread, not depended on. Its loop condition is while (!this.repoWatchClosed), which the event does not change, so receiving it does not end the loop either.
MetadataCache.start()applyEvent() invalidates the cache key for a ref that never existed, then loops. Same shape.
the two sibling implementationsalready terminate rather than emit: InMemoryRepository's iterator close() and metadata-fs's createWatchIterableclose() both settle the waiter with { value: undefined, done: true }. There is no synthetic event anywhere else in the codebase.
LayeredRepository.multiplexWatchtreats a child's done: true as "that layer finished" and ends when all children are done — shape 2 composes through it; shape 1 would forward the phantom _close to a multiplexed consumer wearing a layer label.

So shape 2, for the reason triage gave: giving shutdown a seq of -1 is what makes it collide with the since comparison in the first place.

The matrix, measured before and after

Pinned in sys-metadata-repository.contract.test.ts. "before" is the same three cases run against unmodified close().

subscriptionbeforeafter
watch({org:'system'}, a.seq)⛔ parked next() still unsettled 500ms after close(){ value: undefined, done: true }
watch({org:'system'})no since at all⛔ still unsettled{ value: undefined, done: true }
watch({})⚠️ settled, but with { done: false, value: { seq: -1, op: 'delete', ref: { org: '', type: 'view', name: '_close' }, source: 'sys-metadata-repo-close' } } — and the next pull then hung{ value: undefined, done: true }, and so does every later pull

⭐ The middle row is the one that proves the org-filter half bites on its own: no since is involved, and it is the shape MetadataCache.start() takes for any non-empty watchFilter. A fix tested only against the since half would look complete and leave it hanging.

⚠️ The third row is a refinement of the card's matrix, which recorded it as drained. Only the single parked promise was drained; the iterator was not finished, and what it delivered was a phantom event. The new cases assert the second pull too, which is the half that hid it.

What changed

The watcher registry holds each subscription's terminator next to its event sink (WatchSubscription), and close() runs the terminator — the identical routine iterator.return() runs. A consumer that breaks its own loop and a consumer whose repository shut down under it now observe the same thing, so neither has to special-case the other.

On the "both dispatch paths" instruction — measured, and the premise refines. :1077-1078 (in replayFromHistory) and :1144-1145 (in dispatch) do both apply the since drop and matchesFilter, but only the second is on the drain event's path: replayFromHistory filters rows read out of sys_metadata_history, and the synthetic event was never written there — it went straight from close() into dispatch. Under shape 2 the question dissolves, because there is no event to filter on any path. Both filter sites are deliberately untouched and still apply to real events, which the existing invariant-6 cases pin.

The contract, now stated

Invariant 8 in packages/metadata-core/src/repository.ts"shutdown terminates; it does not emit" — says what a repository-level close() owes a pending iterator: end every live iterator with done: true and no value, the same observation return() produces; queued or unreplayed events MAY be dropped; shutdown MUST NOT be delivered as an event, with both measured reasons written down. It is conditional because close() is not on the MetadataRepository interface, and it records where each of today's three implementations stands.

Out of scope, filed not fixed

#11127FileSystemRepository.close() retires the chokidar watcher and the resync sweep but never reaches its event broker, so a parked watch() iterator stays parked. Same defect class, different package; filed with its confidence stated (code read, no runtime probe) rather than fixed here. Invariant 8 names it as the one measured non-conformance rather than quietly omitting it. Also left alone: listDrafts and the org-scope path in this file, which belong to the neighbour queued behind this card.

Verification

All at 2314ff6f9, the final commit.

  • pnpm --filter @objectstack/metadata-protocol --filter @objectstack/metadata-core --filter @objectstack/metadata test1859 passed | 10 skipped, 165 passed, 615 passed. The two consumer packages are here because this changes what their watch loops observe at shutdown.
  • pnpm --filter @objectstack/metadata-core typecheckDone. @objectstack/metadata-protocol and @objectstack/metadata declare notypecheck script (a --filter that matches no script exits 0 having run nothing); they are covered instead by the ratchet below.
  • pnpm check:type-check-debt on the built closure (turbo run build --filter='./packages/*' --filter='./packages/*/*', 70/70 successful) — OK — 33 ledger entr(ies) re-measured in 338.6s, 1895 raw tsc error(s) total, none above its recorded number. @objectstack/metadata-protocol is a ledger entry (frozen at 63), so the new test code was re-measured, not assumed.
  • pnpm lint (eslint . --no-inline-config, repo-wide) — clean, 124s. No narrowing claimed.
  • Gate families from node scripts/pm/dispatch-gates.mjs re-derived against the real changed paths, all exit 0: check:changeset-gate-self-tests, check:cross-package-test-inputs, check:durability-log-level, check:objectui-changeset, check:slot-lookup, check:test-source-alias, check:type-source-resolution, check:query-options-erasure, check:engine-double-contract, check:where-matcher, check:type-check-coverage, check:nul-bytes, check-adr-0087-registration.mjs, check-changeset-no-major.mjs, check-ci-filter-parity.mjs, check-cross-package-test-inputs.mjs, check-empty-changeset.mjs, check-plugin-teardown-shape.mjs, docs-audit/check-affected-docs.mjs.
  • The derivation added five changeset families and the five convention-triggered test-file families the dispatch lead did not carry; those were run too.

Generated by Claude Code

… emitting a drain event (#11021)
`SysMetadataRepository.close()` modelled shutdown as a metadata event —
`{ seq: -1, ref: { org: '', type: 'view', name: '_close' } }` broadcast
through the same dispatch closure real events pass, then `watchers.clear()`.
Both of that closure's guards reject it: `matchesFilter` drops it for any
subscription naming an org (the synthetic ref's org is the empty string), a
type other than `view`, or a name; and `evt.seq <= since` holds for -1 against
every real seq. Dropped and then unsubscribed, nothing could settle the parked
promise and the consumer's `for await` never returned.
The subscriptions that passed both guards were no better off: they received the
synthetic event as a real one — a `view` named `_close`, deleted, at seq -1,
which MetadataManager turns into a cache invalidation and re-emits to Studio's
HMR stream — and hung on the next pull anyway, because delivering an event does
not end an iterator.
The watcher registry now holds each subscription's terminator next to its event
sink, and `close()` runs the terminator — the same routine `iterator.return()`
runs. Invariant 8 in metadata-core's repository.ts states the contract that was
unstated, and records FileSystemRepository's non-conformance (#11127).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/metadata-core, @objectstack/metadata-protocol, touching 3 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 MetadataRepository (symbol), SysMetadataRepository (symbol))
What this run could not see
  • 6 name(s) were too generic to anchor anything (single lowercase words)

Coarse fallback — 9 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 afe1c4e0a1794a56817acbfd5f4f6e957ea023cepackageMentionDocs.

Which tree this was computed on

This run read content/docs from e4287723823872204fbdea54646d09489f6e68f6 — the merge of head 2314ff6f97e00a893091215c8cc83b745875b228 into base afe1c4e0a1794a56817acbfd5f4f6e957ea023ce, 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 e4287723823872204fbdea54646d09489f6e68f6 && git checkout e4287723823872204fbdea54646d09489f6e68f6
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin afe1c4e0a1794a56817acbfd5f4f6e957ea023ce 2314ff6f97e00a893091215c8cc83b745875b228 && git checkout -B drift-repro afe1c4e0a1794a56817acbfd5f4f6e957ea023ce && git merge --no-ff 2314ff6f97e00a893091215c8cc83b745875b228
node scripts/docs-audit/affected-docs.mjs --json afe1c4e0a1794a56817acbfd5f4f6e957ea023ce

⚠️ 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 afe1c4e0a1794a56817acbfd5f4f6e957ea023ce → 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 22, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review August 22, 2026 20:28
@os-zhuang
os-zhuang added this pull request to the merge queueAug 22, 2026
Merged via the queue into main with commit 7d81c88Aug 22, 2026
32 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-11021-close-drains-filtered-watchers branch August 22, 2026 20:43
@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32596788489 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Console Pin Gate — 失败步骤: Build the Console SPA at the pinned objectui SHA

    ✗ Build failed in 6.08s
    

↳ 失败原因 是判读的关键:超时Test timed out in … / Hook timed out in …)多半是负载/时序,不是本 PR 的回归;
断言AssertionError: …)才指向真实的行为改变。两者的 FAIL 行长得一模一样,只有这一行能区分。

跨 PR 相同签名(24h,按失败测试文件聚合):

  • ⚠️本次没有可用的聚合签名(日志里没有能解析出测试文件名的 FAIL 行)—— 这不是「没有同签名的其他 PR」,是这一轮没测到。跨 PR 聚合本次不可用,请手工比对其他 PR 的同类评论。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 99 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 看上面的「跨 PR 相同签名」;已有汇总 issue ⇒ flaky/环境问题实锤,去那张 issue 上谈,修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-zhuang@claude