From 5f3b221f929c1a14c6c2c44b88fc329af16e490c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 03:59:46 +0000 Subject: [PATCH] =?UTF-8?q?test(metadata):=20=E8=AE=A9=20watcher=20?= =?UTF-8?q?=E9=A1=BA=E5=BA=8F=E7=94=A8=E4=BE=8B=E9=92=89=E4=BD=8F=E9=A1=BA?= =?UTF-8?q?=E5=BA=8F=E6=9C=AC=E8=BA=AB,=E8=80=8C=E4=B8=8D=E6=98=AF=20get()?= =?UTF-8?q?=20=E7=9A=84=E5=BE=AE=E4=BB=BB=E5=8A=A1=E6=B7=B1=E5=BA=A6=20(#6?= =?UTF-8?q?043)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `register-notifies-watchers.test.ts` 的「announces AFTER the write lands」 用例,通过在回调里 `await manager.get(...)`、并在 `await register(...)` 之后读取变量来断言顺序。`notifyWatchers` 以 `void callback(event)` 派发、 从不 await 回调,所以这个写法实际测的是 `get()` 有几个 await 帧,而不是 它声称的顺序保证。 实测两个方向都成立: * 把 `get()` 换成语义等价的 `(await this.getDiagnosed(...)).data` 委托写法 ——纯等价重构——用例立刻变红(#5840 因此放弃该写法,保留三行重复); * 把广播挪到 registry 写入之后、realtime 发布与 writable loader 保存之前 ——真实违反 `register()` 文档的顺序——用例 15/15 全绿。 改为按 issue 的 (b) 方案:在回调里**同步**读 registry,断言广播那一刻写已 落地;并保留 (a) 方案覆盖「订阅者重读看得见新 body」——捕获订阅者的 promise 并由测试 await,而不是假定它已结算。新增 overwrite 变体,使早广播的失败信息 直接读作 V1/V2,而不是与「promise 未结算」同形的 undefined。 同步复核 #6051 留在 `get()` 上的 TSDoc:该常驻理由已过期,改为陈述现状 (仅注释改动,不重构 `get()`)。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDU3qAuJyajAQm3GkUXdfA --- packages/metadata/src/metadata-manager.ts | 34 +++++-- .../src/register-notifies-watchers.test.ts | 97 +++++++++++++++++-- 2 files changed, 114 insertions(+), 17 deletions(-) diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index 42c1b27f22..a55537a0d8 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -787,16 +787,30 @@ export class MetadataManager implements IMetadataService { * has with {@link loadDiagnosed}, so every existing caller keeps its exact * behaviour and only callers that ASK for the verdict pay for it. * - * [#5840] Deliberately NOT expressed as `(await getDiagnosed(…)).data`, - * although that is what it computes. The obvious delegation adds one - * `await` hop, and a registry hit here is observed one microtask sooner than - * it would be through a second async frame — which `register()`'s watchers - * depend on, because `notifyWatchers` does not await its handlers and - * ObjectQL's bridge re-reads through `get()` on the event rather than - * trusting the payload (`register-notifies-watchers.test.ts` pins it, and - * went red on the delegating version). The duplication is three lines and is - * pinned from the other side: `get()` and `getDiagnosed().data` are asserted - * to agree on every case in `metadata-manager-get-diagnosed.test.ts`. + * Not expressed as `(await getDiagnosed(…)).data`, although that is what it + * computes — and the reason has CHANGED, so do not read the duplication as a + * standing constraint. + * + * [#5840] recorded the delegation as unsafe: it adds one `await` hop, and + * `register-notifies-watchers.test.ts` went red on the delegating version, so + * three lines were duplicated to hold the frame count fixed. [#6043] measured + * that test and found it was pinning this method's microtask depth rather than + * the ordering guarantee it named — `notifyWatchers` never awaits its handlers, + * so a subscriber's `await get(…)` had simply been settling inside the + * microtasks `await register(…)` yields. That case now asserts the ordering + * synchronously against the registry and does not observe this method's frame + * count at all; the whole `@objectstack/metadata` suite was re-measured on the + * delegating version and stayed green. + * + * What survives is a plain, local reason: the registry hit is the hot path and + * answering it without a second async frame is worth three lines. Nothing + * external depends on the hop count any more. Consolidating the two into one + * delegation is therefore a viable, deliberately un-taken change (#6043 was + * test-scoped) — if you take it, note that `get()`'s callers outside this + * package were never surveyed for timing sensitivity, only this package's + * tests. Either way the two stay pinned to each other from the other side: + * `get()` and `getDiagnosed().data` are asserted to agree on every case in + * `metadata-manager-get-diagnosed.test.ts`. */ async get(type: string, name: string): Promise { // Check in-memory registry first diff --git a/packages/metadata/src/register-notifies-watchers.test.ts b/packages/metadata/src/register-notifies-watchers.test.ts index 227a1e5c42..35fb33fa24 100644 --- a/packages/metadata/src/register-notifies-watchers.test.ts +++ b/packages/metadata/src/register-notifies-watchers.test.ts @@ -13,6 +13,28 @@ * * These tests pin both halves of the contract: announcing is the DEFAULT, and * silence is only ever opt-in. + * + * [#6043] The "announces AFTER the write lands" case below used to assert an + * ORDERING guarantee by awaiting `get()` inside the callback and reading the + * result back after `register()` resolved. `notifyWatchers` dispatches with + * `void callback(event)` and never awaits its handlers, so what that shape + * actually measured was how many `await` hops `get()` happens to have — fewer + * than the microtasks `await register(...)` yields, and the assertion passed. + * It was a real cost, not a theoretical one: #5840 abandoned an equivalent, + * tidier `getDiagnosed` delegation in `get()` because this case went red on it, + * and kept three duplicated lines to preserve the frame count. Measured the + * other way, hoisting the announcement above the realtime publish and the + * writable-loader save loop — a violation of the ordering `register()` + * documents — left the case fully GREEN. The invariant is worth pinning; the + * shape could not pin it. It is now asserted where the ordering is decided: + * SYNCHRONOUSLY, inside the callback, against the registry `register()` writes. + * + * Scope note: `register()` claims the announcement follows the write into the + * registry AND into every writable loader. Only the registry half is pinned + * here — this fixture's `MemoryLoader` declares `memory:`, and `register()` + * persists to `datasource:` loaders only, so no loader in this file is ever + * written to and the second half is not observable from it. Tracked separately; + * it needs a writable-datasource fixture rather than another assertion. */ import { describe, it, expect, vi, beforeEach } from 'vitest'; @@ -29,6 +51,23 @@ vi.mock('@objectstack/core', () => ({ }), })); +/** + * [#6043] Peek at the manager's private in-memory registry — the state + * `register()` writes FIRST and the one every re-reading subscriber resolves + * against (it outranks every loader in `get()`). Internal by design, and + * load-bearing here precisely because it is the only observation that is + * immune to `get()`'s implementation: the ordering claim must not be restated + * in terms of the method whose refactors keep breaking it. Same narrow cast the + * sibling internal-state peeks use (`metadata-manager-unregister-invalidate-order.test.ts`). + */ +type InternalRegistry = Map>; +const registryOf = (mgr: MetadataManager): InternalRegistry => + (mgr as unknown as { registry: InternalRegistry }).registry; +const registryHas = (mgr: MetadataManager, type: string, name: string): boolean => + registryOf(mgr).get(type)?.has(name) ?? false; +const registryPeek = (mgr: MetadataManager, type: string, name: string): unknown => + registryOf(mgr).get(type)?.get(name); + describe('#3112 — register()/unregister() notify subscribe() watchers', () => { let manager: MetadataManager; @@ -69,17 +108,61 @@ describe('#3112 — register()/unregister() notify subscribe() watchers', () => expect(seen[0].data).toEqual({ name: 'account', label: 'V2' }); }); - it('announces AFTER the write lands, so a subscriber that re-reads sees the new body', async () => { - // The ordering guarantee that makes the event useful: ObjectQL's bridge - // re-reads via get() on the event rather than trusting the payload. - let readBack: unknown; - manager.subscribe('object', async () => { - readBack = await manager.get('object', 'account'); + it('announces AFTER the write lands — the registry already holds the new body at broadcast time', async () => { + // [#6043] The literal reading of the method's promise, asserted at the one + // instant it is decidable: the moment the callback runs. Everything here is + // synchronous, so no `await` hop of any consumer method can move the + // verdict — hoist the announcement above the write and this reads + // `{ had: false }`, which is what the regression looks like. + const atBroadcast: Array<{ had: boolean; body: unknown }> = []; + manager.subscribe('object', () => { + atBroadcast.push({ + had: registryHas(manager, 'object', 'account'), + body: registryPeek(manager, 'object', 'account'), + }); + }); + + await manager.register('object', 'account', { name: 'account', label: 'Fresh' }); + + expect(atBroadcast).toHaveLength(1); + expect(atBroadcast[0].had).toBe(true); + expect(atBroadcast[0].body).toEqual({ name: 'account', label: 'Fresh' }); + }); + + it('announces AFTER the write lands on an OVERWRITE too — never with the pre-write body still in place', async () => { + // [#6043] The overwrite half exists so the failure is READABLE. On a first + // registration an early announcement and an unsettled promise both surface + // as `undefined`; here the pre-write body is a distinct value, so + // announcing too early fails with 'V1' where 'V2' was required and names + // the defect on sight. + await manager.register('object', 'account', { name: 'account', label: 'V1' }, { notify: false }); + + let bodyAtBroadcast: unknown; + manager.subscribe('object', () => { + bodyAtBroadcast = registryPeek(manager, 'object', 'account'); + }); + + await manager.register('object', 'account', { name: 'account', label: 'V2' }); + + expect(bodyAtBroadcast).toEqual({ name: 'account', label: 'V2' }); + }); + + it('a subscriber that re-reads through get() sees the new body — awaited, not raced', async () => { + // [#6043] The consumer-shaped half of the same guarantee: ObjectQL's bridge + // re-reads via get() on the event rather than trusting the payload. The + // subscriber's promise is CAPTURED and awaited by the test, because + // `notifyWatchers` dispatches with `void callback(event)` — nothing else + // ever awaits it, so an assertion that just reads a variable afterwards is + // asserting against `get()`'s frame count instead of against the read. + const reads: Promise[] = []; + manager.subscribe('object', () => { + reads.push(manager.get('object', 'account')); }); await manager.register('object', 'account', { name: 'account', label: 'Fresh' }); - expect(readBack).toEqual({ name: 'account', label: 'Fresh' }); + expect(reads).toHaveLength(1); + await expect(reads[0]).resolves.toEqual({ name: 'account', label: 'Fresh' }); }); it('is silent when the caller opts out with { notify: false }', async () => {