From e89a644d37f75f45c39c586066bf61340046b18f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 08:44:55 +0000 Subject: [PATCH 1/2] fix(objectql): the roll-up summary index's registry read propagates, and a failed read is never cached (#9154) `buildSummaryIndex()` answered a registry read that could not run with an invented "no object declares a roll-up", and `ensureSummaryIndexes()` then MEMOIZED that invention stamped with the registry's current `objectRevision`. Since `objectRevision` moves only on a metadata mutation and never on a data write, the failure outlived its cause: every parent roll-up silently stopped recomputing until a restart or an unrelated publish. Both halves of the swallow are gone -- the `catch` and the optional call `?.()`, which absorbed a registry that omits `getAllObjects` entirely without ever throwing -- matching the canonical shape #9002 landed for the two delete-cascade seams. And a build that throws now clears any cached index and resets the revision stamp before rethrowing unchanged, so the next call rebuilds: a poisoned cache entry must not survive the read that poisoned it. Structural close, not a live defect: `SchemaRegistry.getAllObjects()` has no throwing path on today's tree (re-measured). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj --- .../summary-index-registry-read-propagates.md | 48 +++ ...ummary-index-registry-read-failure.test.ts | 375 ++++++++++++++++++ packages/objectql/src/engine.ts | 73 +++- 3 files changed, 491 insertions(+), 5 deletions(-) create mode 100644 .changeset/summary-index-registry-read-propagates.md create mode 100644 packages/objectql/src/engine-summary-index-registry-read-failure.test.ts diff --git a/.changeset/summary-index-registry-read-propagates.md b/.changeset/summary-index-registry-read-propagates.md new file mode 100644 index 0000000000..f1df889463 --- /dev/null +++ b/.changeset/summary-index-registry-read-propagates.md @@ -0,0 +1,48 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): the roll-up summary index's registry read propagates, and a failed read is never cached as an empty index (#9154) + +`ObjectQL.buildSummaryIndex()` opened with + +```ts +try { objects = (this._registry as any).getAllObjects?.() ?? []; } catch { objects = []; } +``` + +and `ensureSummaryIndexes()` MEMOIZES what that build returns, stamped with the +registry's current `objectRevision`. So a read that could not run was answered +with an invented *"no object declares a roll-up"* — and then remembered as if it +had been measured. `recomputeSummaries()` consults that index after every insert, +update and delete to decide which parent roll-ups a child write must recompute, +so an empty index means no roll-up is ever recomputed: every parent summary field +keeps a stale value, nothing is logged, and every write reports success. + +Two changes, because the cache is the half that made this worse than the +identical seams fixed in #9002: + +- **The read propagates.** Same family as #8895 (*discriminate or propagate*) and + #9002, same reasoning: discrimination needs a benign failure class and there is + none — an unreadable registry is never truthfully "no roll-ups". Both halves of + the swallow are gone, the `catch` and the optional call `?.()`, which absorbed a + registry that does not implement `getAllObjects` at all — the structural + omission that never throws and is therefore invisible. +- **A failed build leaves no cache entry.** `objectRevision` moves only on a + metadata mutation (`registerObject`, `unregisterObject`, + `unregisterObjectsByPackage`, `removeObjectOverlay`, `invalidate`, + `invalidateAll`, `reset`) and never on a data write, so the invented emptiness + outlived the condition that caused it — a steady-state deployment performs none + of those, leaving every parent roll-up frozen until a restart or an unrelated + publish. The build now runs to completion into a local before anything is + published to the instance, the revision stamp is written last, and a throw + clears any cached index and resets the stamp before rethrowing unchanged: the + next call rebuilds. *A poisoned cache entry must not survive the read that + poisoned it.* + +**No shipped behaviour changes.** `SchemaRegistry.getAllObjects()` is a walk over +in-memory `Map`s calling `resolveObject()` — which returns `undefined` on every +failure branch it models — over a fold that is spreads and comparisons. No I/O, +no driver, no `throw` on the measured path, re-derived on today's tree. This is a +structural close of a fail-open shape, pinned by tests, so that the day the +registry read grows a throwing path it fails loudly instead of silently freezing +every roll-up in the deployment. diff --git a/packages/objectql/src/engine-summary-index-registry-read-failure.test.ts b/packages/objectql/src/engine-summary-index-registry-read-failure.test.ts new file mode 100644 index 0000000000..02ea658179 --- /dev/null +++ b/packages/objectql/src/engine-summary-index-registry-read-failure.test.ts @@ -0,0 +1,375 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#9154] The roll-up summary index's registry read must not answer a failed + * read with an invented "no object declares a roll-up" — and, the limb that + * makes this one worse than its two #9002 siblings, must not CACHE that + * invention under the registry's current `objectRevision`. + * + * `ObjectQL.buildSummaryIndex()`'s first statement used to be + * + * ```ts + * try { objects = (this._registry as any).getAllObjects?.() ?? []; } catch { objects = []; } + * ``` + * + * and `ensureSummaryIndexes()` memoizes what it returns, stamped with the + * registry's `objectRevision`. So a single failed read did not degrade one + * write: it installed an EMPTY roll-up index and recorded it as measured, and + * `recomputeSummaries()` — which every insert / update / delete consults to + * decide which parent roll-ups to recompute — then found nothing to do, on + * every subsequent write, silently, with each write reporting success. + * + * `objectRevision` moves only on a metadata MUTATION (`registerObject`, + * `unregisterObject`, `unregisterObjectsByPackage`, `removeObjectOverlay`, + * `invalidate`, `invalidateAll`, `reset`) and never on a data write, so the + * invented emptiness outlived the condition that caused it — until a restart or + * an unrelated publish. That lifetime is measured here rather than asserted: + * every recovery test below re-reads `registry.objectRevision` and pins it + * UNCHANGED across the failure, so the recovery it proves cannot be the + * accidental one an unrelated registry change would have produced. + * + * ⚠️ Like #9002's pin this closes a STRUCTURAL hole, not a live defect. + * `SchemaRegistry.getAllObjects()` is a walk over in-memory `Map`s calling + * `resolveObject()`, which returns `undefined` on every failure branch it models + * and never throws; the fold below it is spreads and comparisons. No I/O, no + * driver, no `throw` on the measured path — re-derived on this tree. The failure + * is therefore injected at the registry method itself, and the injection IS the + * statement that nothing shipped reaches the seam today. + * + * Two handles are used deliberately, because they answer different questions: + * `getOwnedSummaryDescriptors()` is the engine's own PUBLIC read of the index + * (#6063) and reaches `buildSummaryIndex()` through exactly one registry read, + * so it isolates THIS seam from every other `getAllObjects()` consumer on the + * write path; the `insert()` tests then show the same failure end-to-end, where + * the consequence — a parent roll-up that silently stops recomputing — actually + * lands. Every roll-up expectation is read out of the driver's own store, never + * back out of the engine. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import type { ServiceObject } from '@objectstack/spec/data'; +import { ObjectQL } from './engine.js'; + +/** The package id every fixture below is registered under. */ +const OWNER_PACKAGE = 'test-9154'; + +/* + * Fixtures are typed as `ServiceObject` (and registered WITH their `packageId`) + * rather than left to inference, so this file adds nothing to + * `@objectstack/objectql`'s TEST_DEBT ledger — a shrink-only ratchet (#5278). + */ +const inv: ServiceObject = { + name: 'inv', + label: 'Invoice', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + line_total: { + name: 'line_total', + label: 'Line total', + type: 'summary' as const, + summaryOperations: { object: 'inv_line', field: 'amount', function: 'sum' as const }, + }, + line_count: { + name: 'line_count', + label: 'Line count', + type: 'summary' as const, + summaryOperations: { object: 'inv_line', field: 'amount', function: 'count' as const }, + }, + }, +}; +const invLine: ServiceObject = { + name: 'inv_line', + label: 'Invoice line', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + amount: { name: 'amount', label: 'Amount', type: 'number' as const }, + inv: { name: 'inv', label: 'Invoice', type: 'master_detail' as const, reference: 'inv' }, + }, +}; + +/** A minimal in-memory driver — no read-failure injection here, deliberately: + * the failure this file is about happens in the REGISTRY, so a driver that + * always succeeds is what makes the roll-up's silence visible. */ +function makeStubDriver() { + const stores = new Map>>(); + const storeFor = (o: string): Map> => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + let nextId = 0; + const checkOp = (value: unknown, cond: unknown): boolean => { + if (cond === null || typeof cond !== 'object' || Array.isArray(cond) || cond instanceof Date) { + return value === cond; + } + return Object.entries(cond as Record).every(([op, target]) => { + switch (op) { + case '$eq': return value === target; + case '$ne': return value !== target; + case '$in': return Array.isArray(target) && target.includes(value); + default: return true; + } + }); + }; + const matches = (row: Record, where: unknown): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where as Record).every(([k, v]) => { + if (k === '$and') return (v as unknown[]).every((w) => matches(row, w)); + if (k === '$or') return (v as unknown[]).some((w) => matches(row, w)); + if (k === '$not') return !matches(row, v); + return checkOp(row?.[k], v); + }); + }; + const driver: Record = { + name: 'memory', version: '0.0.0', supports: {}, + async connect(): Promise {}, async disconnect(): Promise {}, + async checkHealth(): Promise { return true; }, + async execute(): Promise { return null; }, + async find(o: string, ast: { where?: unknown }): Promise[]> { + return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(o: string, ast: { where?: unknown }): Promise | null> { + for (const r of storeFor(o).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(o: string, data: Record): Promise> { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(o).set(id, row); + return row; + }, + async update(o: string, id: string, data: Record): Promise> { + const s = storeFor(o); + const row = { ...s.get(id), ...data, id }; + s.set(id, row); + return row; + }, + async delete(o: string, id: string): Promise { return storeFor(o).delete(id); }, + async count(o: string, ast: { where?: unknown }): Promise { + return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)).length; + }, + async bulkCreate(o: string, rows: Record[]): Promise[]> { + const out: Record[] = []; + for (const r of rows) out.push(await (driver.create as (a: string, b: Record) => Promise>)(o, r)); + return out; + }, + async bulkUpdate(): Promise[]> { return []; }, + async bulkDelete(): Promise {}, + async beginTransaction(): Promise> { + return { commit: async (): Promise => {}, rollback: async (): Promise => {} }; + }, + async commit(): Promise {}, async rollback(): Promise {}, + }; + return { driver, stores }; +} + +/** + * Make the engine's registry throw `error` from `getAllObjects()` while it is + * ARMED, and report how many reads it has served. + * + * A toggle rather than #9002's nth-call counter, because the point of this card + * is what happens AFTER the failing read is over: the registry is healed with + * `heal()` and nothing else about the engine or the registry is touched — in + * particular `objectRevision` does not move — so a subsequent correct roll-up + * can only come from a rebuild that was NOT short-circuited by a cached answer. + */ +function injectRegistryReadFailure( + engine: ObjectQL, + error: unknown, +): { arm: () => void; heal: () => void; calls: () => number } { + const registry = engine.registry as unknown as { + getAllObjects: (packageId?: string) => ServiceObject[]; + }; + const real = registry.getAllObjects.bind(registry); + let armed = false; + let calls = 0; + registry.getAllObjects = (packageId?: string): ServiceObject[] => { + calls += 1; + if (armed) throw error; + return real(packageId); + }; + return { arm: () => { armed = true; }, heal: () => { armed = false; }, calls: () => calls }; +} + +describe('[#9154] the roll-up summary index must not invent — or cache — "no roll-ups"', () => { + let engine: ObjectQL; + let stores: Map>>; + + beforeEach(async () => { + engine = new ObjectQL(); + const stub = makeStubDriver(); + stores = stub.stores; + engine.registerDriver(stub.driver as never, true); + await engine.init(); + for (const o of [inv, invLine]) { + engine.registry.registerObject(o, OWNER_PACKAGE); + } + }); + + /** The parent row as the DRIVER holds it — never read back through the engine. */ + const parent = (id: string): Record => + stores.get('inv')?.get(id) ?? {}; + + const injected = (): Error & { code: string; status: number } => + Object.assign(new Error('registry unreadable: contributor fold failed'), { + code: 'REGISTRY_READ_FAILED', + status: 500, + }); + + // ── POSITIVE CONTROLS — a readable registry really does maintain the + // roll-ups in this harness. Without these, every "the roll-up is correct" + // assertion below could pass on a harness that never rolled anything up. + + it('control: a readable registry recomputes the parent roll-up on each child insert', async () => { + const p = await engine.insert('inv', { name: 'INV-1' }); + await engine.insert('inv_line', { inv: p.id, amount: 10 }); + await engine.insert('inv_line', { inv: p.id, amount: 32 }); + + expect(parent(p.id as string).line_total).toBe(42); + expect(parent(p.id as string).line_count).toBe(2); + }); + + it('control: a readable registry reports the parent-side descriptors it owns', () => { + const owned = engine.getOwnedSummaryDescriptors('inv'); + expect(owned.map((d) => d.summaryField).sort()).toEqual(['line_count', 'line_total']); + expect(owned.every((d) => d.childObject === 'inv_line' && d.fkField === 'inv')).toBe(true); + }); + + // ── THE SEAM — `buildSummaryIndex()`'s registry read, isolated. + + it('the failed registry read surfaces instead of becoming an empty index', () => { + const err = injected(); + const probe = injectRegistryReadFailure(engine, err); + probe.arm(); + + let thrown: unknown; + try { engine.getOwnedSummaryDescriptors('inv'); } catch (e) { thrown = e; } + + // No new code and no new response field is minted here: the envelope the + // read failed with is the envelope the caller receives. + expect(thrown).toBe(err); + expect((thrown as { code: string }).code).toBe('REGISTRY_READ_FAILED'); + expect((thrown as { status: number }).status).toBe(500); + expect((thrown as Error).message).toBe('registry unreadable: contributor fold failed'); + expect(probe.calls()).toBe(1); + }); + + it('a registry that does not implement getAllObjects fails loudly, not emptily', () => { + // The `?.()` half of the swallow. A double that simply OMITS the method + // never throws, so pre-fix it produced a permanently empty roll-up index + // with nothing to notice — the exact structural omission a shipped test + // double really did carry (#9002 found one when it removed its swallows). + (engine.registry as unknown as { getAllObjects?: unknown }).getAllObjects = undefined; + + expect(() => engine.getOwnedSummaryDescriptors('inv')).toThrow(TypeError); + expect(() => engine.getOwnedSummaryDescriptors('inv')).toThrow(/not a function/); + }); + + // ── ⭐ THE CACHE LIMB — what makes this card worse than #9002's two seams. + + it('the failed read leaves NO cached index: the very next read rebuilds, with objectRevision unmoved', () => { + const revisionBefore = (engine.registry as unknown as { objectRevision: number }).objectRevision; + + const probe = injectRegistryReadFailure(engine, injected()); + probe.arm(); + expect(() => engine.getOwnedSummaryDescriptors('inv')).toThrow(); + probe.heal(); + + // Nothing about the registry's CONTENT changed — the poisoning read and + // the healed read see the same revision. Pre-fix the first call stamped + // an empty index with exactly this number and the second call returned + // it, so this equality is what makes the recovery below non-accidental. + const revisionAfter = (engine.registry as unknown as { objectRevision: number }).objectRevision; + expect(revisionAfter).toBe(revisionBefore); + + const owned = engine.getOwnedSummaryDescriptors('inv'); + expect(owned.map((d) => d.summaryField).sort()).toEqual(['line_count', 'line_total']); + }); + + /* + * ⚠️ Measured while writing these: the index is built LAZILY and the failing + * read must land on a real BUILD, not on a cache hit. `beforeEach` registers + * the fixtures and stops; nothing has built the index yet, so the poisoning + * read below is the FIRST one — which is precisely the shape a booting + * deployment has, and the one the card describes. Arming the failure after a + * successful write instead would prove nothing: the write already cached a + * good index and no further registry read happens at all (measured: an + * `insert` over a warm index makes ZERO `getAllObjects()` calls). + */ + + it('a write after the failed read still recomputes the roll-up — the poisoned entry does not outlive its cause', async () => { + const revisionBefore = (engine.registry as unknown as { objectRevision: number }).objectRevision; + + const probe = injectRegistryReadFailure(engine, injected()); + probe.arm(); + expect(() => engine.getOwnedSummaryDescriptors('inv')).toThrow(); + probe.heal(); + + const p = await engine.insert('inv', { name: 'INV-1' }); + await engine.insert('inv_line', { inv: p.id, amount: 7 }); + + // Pre-fix: the failed read installed an empty index stamped at + // `revisionBefore`; the parent insert seeded nothing, this child insert + // found no descriptors for `inv_line`, and the parent's roll-up was + // never written — while both writes reported success. Every later write + // did the same, for the life of the process. + expect((engine.registry as unknown as { objectRevision: number }).objectRevision).toBe(revisionBefore); + expect(parent(p.id as string).line_total).toBe(7); + expect(parent(p.id as string).line_count).toBe(1); + }); + + it('the roll-up keeps recomputing on EVERY later write, not just the first one after the failure', async () => { + const revisionBefore = (engine.registry as unknown as { objectRevision: number }).objectRevision; + + const probe = injectRegistryReadFailure(engine, injected()); + probe.arm(); + expect(() => engine.getOwnedSummaryDescriptors('inv')).toThrow(); + probe.heal(); + + const p = await engine.insert('inv', { name: 'INV-1' }); + // The insert-time seed (#5749) is the parent-side view of the same + // index, so it is poisoned by the same read. + expect(parent(p.id as string).line_count).toBe(0); + + await engine.insert('inv_line', { inv: p.id, amount: 10 }); + await engine.insert('inv_line', { inv: p.id, amount: 32 }); + await engine.insert('inv_line', { inv: p.id, amount: 5 }); + + expect((engine.registry as unknown as { objectRevision: number }).objectRevision).toBe(revisionBefore); + expect(parent(p.id as string).line_total).toBe(47); + expect(parent(p.id as string).line_count).toBe(3); + }); + + // ── END-TO-END — the write path itself, while the registry is unreadable. + + it('a write during the failed read does not report success over a skipped roll-up', async () => { + const err = injected(); + const probe = injectRegistryReadFailure(engine, err); + probe.arm(); + + // The write path reaches the index through the insert-time seed, so the + // read's failure reaches the CALLER. Pre-fix this insert RESOLVED, with + // its roll-up fields silently unseeded and the poisoned index left + // behind for every write after it. + const caught: unknown = await engine.insert('inv', { name: 'INV-1' }).catch((e: unknown) => e); + expect(caught).toBe(err); + expect((caught as { code: string }).code).toBe('REGISTRY_READ_FAILED'); + expect((caught as { status: number }).status).toBe(500); + + // …and once the registry is readable again the next writes are correct, + // with no registry mutation in between. + probe.heal(); + const p = await engine.insert('inv', { name: 'INV-2' }); + await engine.insert('inv_line', { inv: p.id, amount: 10 }); + await engine.insert('inv_line', { inv: p.id, amount: 32 }); + + const lines = Array.from(stores.get('inv_line')?.values() ?? []) + .filter((r) => r.inv === p.id); + const expected = lines.reduce((sum, r) => sum + Number(r.amount ?? 0), 0); + expect(expected).toBe(42); + expect(parent(p.id as string).line_total).toBe(expected); + expect(parent(p.id as string).line_count).toBe(lines.length); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index b772e0cf07..abb7824092 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -6940,16 +6940,39 @@ export class ObjectQL implements IObjectQLEngine { * — by the child object they aggregate and by the parent object that owns * them — resolving the child→parent FK field. One scan, two views of the * identical descriptor objects, so the two indexes can never disagree about - * which roll-ups exist. */ + * which roll-ups exist. + * + * [#9154] The registry read PROPAGATES. It used to be + * `try { (registry as any).getAllObjects?.() ?? [] } catch { [] }`, which + * answered a read that could not run with an invented *"no object declares a + * roll-up"* — the same shape #8895 ruled on (*discriminate or propagate*) + * and #9002 removed from the two delete-cascade seams, and there is no + * benign failure class to discriminate here either: an unreadable registry + * is never truthfully "no roll-ups". An empty index means + * {@link recomputeSummaries} recomputes nothing after every insert / update + * / delete, so every parent summary field keeps a stale value while every + * write reports success and nothing is logged. + * + * Both halves of the swallow are gone, not just the `catch`: the optional + * call `?.()` absorbed a registry that does not implement `getAllObjects` at + * all — the same structural omission a test double really did ship, and one + * that is invisible precisely because it never throws. A plain call fails + * loudly on it. + * + * ⚠️ This is a STRUCTURAL close, not a live defect: `SchemaRegistry`'s + * `getAllObjects()` is a walk over in-memory `Map`s calling `resolveObject()` + * — which returns `undefined` on every failure branch it models — over a + * fold that is spreads and comparisons. No I/O, no driver, no `throw` on the + * measured path. The pin lives in + * `engine-summary-index-registry-read-failure.test.ts`. */ private buildSummaryIndex(): { byChild: Map; byParent: Map; } { const index = new Map(); const byParent = new Map(); - let objects: any[] = []; - try { objects = (this._registry as any).getAllObjects?.() ?? []; } catch { objects = []; } - for (const parent of objects) { + const objects: ServiceObject[] = this._registry.getAllObjects(); + for (const parent of objects as any[]) { const fields = parent?.fields; if (!fields || typeof fields !== 'object' || Array.isArray(fields)) continue; for (const [summaryField, def] of Object.entries(fields)) { @@ -7005,6 +7028,36 @@ export class ObjectQL implements IObjectQLEngine { * Ensure both roll-up indexes are present and current. Split out of * {@link getSummaryDescriptors} so the parent-side view (#5749) shares the * exact same staleness rule instead of re-deriving one. + * + * ## [#9154] A failed build leaves NO cache entry — *a poisoned cache entry + * must not survive the read that poisoned it* + * + * This is the limb that made the swallow removed from + * {@link buildSummaryIndex} worse than its two #9002 siblings. Those invented + * an answer ONCE, per delete. Here the answer is MEMOIZED and stamped with the + * registry's current `objectRevision` — so one failed read did not degrade one + * write, it installed an empty index that every subsequent write then read as + * a measured answer. And `objectRevision` moves only on a metadata MUTATION + * (`registerObject`, `unregisterObject`, `unregisterObjectsByPackage`, + * `removeObjectOverlay`, `invalidate`, `invalidateAll`, `reset`) — never on a + * data write. A steady-state deployment performs none of those, so the + * invented emptiness outlived its cause for the whole process lifetime, + * ending only at a restart or at an unrelated publish. The failure was not + * "briefly wrong"; it was wrong until an unrelated event. + * + * Two things hold the invariant, and both are load-bearing — do not reorder: + * + * 1. the build runs to completion into a LOCAL before anything is published + * to `this`, so a throw cannot leave a half-written pair of indexes, and + * the revision stamp is written LAST — an unstamped failure is retried on + * the very next call rather than being remembered as an answer; + * 2. the `catch` below clears whatever was cached and resets the stamp before + * rethrowing. Behaviourally that is a no-op today (nothing reads the + * fields without coming through here first, and the stale entry would be + * rebuilt anyway) — it is here so the guarantee is stated in code instead + * of resting on the survey in (1). ⛔ It swallows nothing: the error is + * rethrown unchanged, and `engine-summary-index-registry-read-failure.test.ts` + * pins both the propagation and the retry. */ private ensureSummaryIndexes(): void { // Rebuild whenever the REGISTRY's object set has moved since the index was @@ -7020,7 +7073,17 @@ export class ObjectQL implements IObjectQLEngine { const revision = (this._registry as unknown as { objectRevision?: number })?.objectRevision; const stale = typeof revision === 'number' && revision !== this.summaryIndexRevision; if (!this.summaryIndex || !this.summaryIndexByParent || stale) { - const built = this.buildSummaryIndex(); + let built: { byChild: Map; byParent: Map }; + try { + built = this.buildSummaryIndex(); + } catch (err) { + // [#9154] Nothing measured came back, so nothing is remembered — not + // even the entry that was already here, which the staleness test above + // has just judged out of date. The next call rebuilds from scratch. + this.invalidateSummaryIndex(); + this.summaryIndexRevision = -1; + throw err; + } this.summaryIndex = built.byChild; this.summaryIndexByParent = built.byParent; if (typeof revision === 'number') this.summaryIndexRevision = revision; From 65a5e2afab5ec3509619e103b60d86b1b6f8f43e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 10:36:25 +0000 Subject: [PATCH 2/2] test(objectql): pin the summary-index registry-read seam and its cache; repair nine registry doubles (#9154) The new pin file measures both halves: the read propagates, and the failed read leaves no cached index -- every recovery assertion re-reads `registry.objectRevision` and pins it UNCHANGED across the failure, so the recovery it proves cannot be the accidental one a metadata mutation would have produced. Nine suites went red the moment the optional call `?.()` was removed, all with `TypeError: this._registry.getAllObjects is not a function`: their `vi.mock('./registry')` doubles never modelled the method, and the swallow made an incomplete double indistinguishable from an empty registry. The prior green was vacuous -- the write path was silently skipping the insert-time roll-up seed and the post-write recompute in all nine. Each double now declares `getAllObjects: () => []`, which is the truthful body there: none of the nine declares a `summary` field. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj --- .../src/engine-autonumber-default-format.test.ts | 11 +++++++++++ packages/objectql/src/engine-autonumber-defer.test.ts | 11 +++++++++++ .../objectql/src/engine-autonumber-resync.test.ts | 11 +++++++++++ .../src/engine-autonumber-seed-outage.test.ts | 11 +++++++++++ .../objectql/src/engine-autonumber-seed-scan.test.ts | 11 +++++++++++ .../src/engine-autonumber-seed-suffix.test.ts | 11 +++++++++++ packages/objectql/src/engine-filter-tokens.test.ts | 11 +++++++++++ .../objectql/src/engine-multivalue-normalize.test.ts | 11 +++++++++++ .../objectql/src/engine-validation-locale.test.ts | 11 +++++++++++ 9 files changed, 99 insertions(+) diff --git a/packages/objectql/src/engine-autonumber-default-format.test.ts b/packages/objectql/src/engine-autonumber-default-format.test.ts index cfaa18b0a0..5e224ab7ab 100644 --- a/packages/objectql/src/engine-autonumber-default-format.test.ts +++ b/packages/objectql/src/engine-autonumber-default-format.test.ts @@ -50,6 +50,17 @@ vi.mock('./registry', () => { const instance: any = { getObject: vi.fn(), resolveObject: vi.fn((n: string) => instance.getObject(n)), + // [#9154] This double used to OMIT `getAllObjects`, and every test here + // passed anyway: the engine's roll-up summary index read it as + // `getAllObjects?.() ?? []`, so a double that does not model the method + // was indistinguishable from a registry with nothing in it — the write + // path silently skipped the insert-time roll-up seed (#5749) and the + // post-write recompute. With the optional call gone the omission is a + // hard `TypeError`, which is the point: the double now has to model the + // method the engine actually calls. Empty is the truthful body for THIS + // suite — it declares no `summary` field, so the roll-up index over it is + // empty either way, and now it says so instead of the engine inventing it. + getAllObjects: vi.fn(() => []), registerObject: vi.fn(), getObjectOwner: vi.fn(), registerNamespace: vi.fn(), diff --git a/packages/objectql/src/engine-autonumber-defer.test.ts b/packages/objectql/src/engine-autonumber-defer.test.ts index 04626693b8..588dbbf478 100644 --- a/packages/objectql/src/engine-autonumber-defer.test.ts +++ b/packages/objectql/src/engine-autonumber-defer.test.ts @@ -22,6 +22,17 @@ vi.mock('./registry', () => { const instance: any = { getObject: vi.fn(), resolveObject: vi.fn((n: string) => instance.getObject(n)), + // [#9154] This double used to OMIT `getAllObjects`, and every test here + // passed anyway: the engine's roll-up summary index read it as + // `getAllObjects?.() ?? []`, so a double that does not model the method + // was indistinguishable from a registry with nothing in it — the write + // path silently skipped the insert-time roll-up seed (#5749) and the + // post-write recompute. With the optional call gone the omission is a + // hard `TypeError`, which is the point: the double now has to model the + // method the engine actually calls. Empty is the truthful body for THIS + // suite — it declares no `summary` field, so the roll-up index over it is + // empty either way, and now it says so instead of the engine inventing it. + getAllObjects: vi.fn(() => []), registerObject: vi.fn(), getObjectOwner: vi.fn(), registerNamespace: vi.fn(), diff --git a/packages/objectql/src/engine-autonumber-resync.test.ts b/packages/objectql/src/engine-autonumber-resync.test.ts index 4a26f9c246..0df5eac42f 100644 --- a/packages/objectql/src/engine-autonumber-resync.test.ts +++ b/packages/objectql/src/engine-autonumber-resync.test.ts @@ -95,6 +95,17 @@ vi.mock('./registry', () => { const instance: any = { getObject: vi.fn(), resolveObject: vi.fn((n: string) => instance.getObject(n)), + // [#9154] This double used to OMIT `getAllObjects`, and every test here + // passed anyway: the engine's roll-up summary index read it as + // `getAllObjects?.() ?? []`, so a double that does not model the method + // was indistinguishable from a registry with nothing in it — the write + // path silently skipped the insert-time roll-up seed (#5749) and the + // post-write recompute. With the optional call gone the omission is a + // hard `TypeError`, which is the point: the double now has to model the + // method the engine actually calls. Empty is the truthful body for THIS + // suite — it declares no `summary` field, so the roll-up index over it is + // empty either way, and now it says so instead of the engine inventing it. + getAllObjects: vi.fn(() => []), registerObject: vi.fn(), getObjectOwner: vi.fn(), registerNamespace: vi.fn(), diff --git a/packages/objectql/src/engine-autonumber-seed-outage.test.ts b/packages/objectql/src/engine-autonumber-seed-outage.test.ts index fc8e5ab3db..b4aecfad62 100644 --- a/packages/objectql/src/engine-autonumber-seed-outage.test.ts +++ b/packages/objectql/src/engine-autonumber-seed-outage.test.ts @@ -41,6 +41,17 @@ vi.mock('./registry', () => { const instance: any = { getObject: vi.fn(), resolveObject: vi.fn((n: string) => instance.getObject(n)), + // [#9154] This double used to OMIT `getAllObjects`, and every test here + // passed anyway: the engine's roll-up summary index read it as + // `getAllObjects?.() ?? []`, so a double that does not model the method + // was indistinguishable from a registry with nothing in it — the write + // path silently skipped the insert-time roll-up seed (#5749) and the + // post-write recompute. With the optional call gone the omission is a + // hard `TypeError`, which is the point: the double now has to model the + // method the engine actually calls. Empty is the truthful body for THIS + // suite — it declares no `summary` field, so the roll-up index over it is + // empty either way, and now it says so instead of the engine inventing it. + getAllObjects: vi.fn(() => []), registerObject: vi.fn(), getObjectOwner: vi.fn(), registerNamespace: vi.fn(), diff --git a/packages/objectql/src/engine-autonumber-seed-scan.test.ts b/packages/objectql/src/engine-autonumber-seed-scan.test.ts index ee29560f84..563c366765 100644 --- a/packages/objectql/src/engine-autonumber-seed-scan.test.ts +++ b/packages/objectql/src/engine-autonumber-seed-scan.test.ts @@ -45,6 +45,17 @@ vi.mock('./registry', () => { const instance: any = { getObject: vi.fn(), resolveObject: vi.fn((n: string) => instance.getObject(n)), + // [#9154] This double used to OMIT `getAllObjects`, and every test here + // passed anyway: the engine's roll-up summary index read it as + // `getAllObjects?.() ?? []`, so a double that does not model the method + // was indistinguishable from a registry with nothing in it — the write + // path silently skipped the insert-time roll-up seed (#5749) and the + // post-write recompute. With the optional call gone the omission is a + // hard `TypeError`, which is the point: the double now has to model the + // method the engine actually calls. Empty is the truthful body for THIS + // suite — it declares no `summary` field, so the roll-up index over it is + // empty either way, and now it says so instead of the engine inventing it. + getAllObjects: vi.fn(() => []), registerObject: vi.fn(), getObjectOwner: vi.fn(), registerNamespace: vi.fn(), diff --git a/packages/objectql/src/engine-autonumber-seed-suffix.test.ts b/packages/objectql/src/engine-autonumber-seed-suffix.test.ts index a6a0857f2f..a240e95e74 100644 --- a/packages/objectql/src/engine-autonumber-seed-suffix.test.ts +++ b/packages/objectql/src/engine-autonumber-seed-suffix.test.ts @@ -42,6 +42,17 @@ vi.mock('./registry', () => { const instance: any = { getObject: vi.fn(), resolveObject: vi.fn((n: string) => instance.getObject(n)), + // [#9154] This double used to OMIT `getAllObjects`, and every test here + // passed anyway: the engine's roll-up summary index read it as + // `getAllObjects?.() ?? []`, so a double that does not model the method + // was indistinguishable from a registry with nothing in it — the write + // path silently skipped the insert-time roll-up seed (#5749) and the + // post-write recompute. With the optional call gone the omission is a + // hard `TypeError`, which is the point: the double now has to model the + // method the engine actually calls. Empty is the truthful body for THIS + // suite — it declares no `summary` field, so the roll-up index over it is + // empty either way, and now it says so instead of the engine inventing it. + getAllObjects: vi.fn(() => []), registerObject: vi.fn(), getObjectOwner: vi.fn(), registerNamespace: vi.fn(), diff --git a/packages/objectql/src/engine-filter-tokens.test.ts b/packages/objectql/src/engine-filter-tokens.test.ts index 15730be7a6..88b27791b6 100644 --- a/packages/objectql/src/engine-filter-tokens.test.ts +++ b/packages/objectql/src/engine-filter-tokens.test.ts @@ -21,6 +21,17 @@ vi.mock('./registry', () => { const instance: any = { getObject: vi.fn(), resolveObject: vi.fn((n: string) => instance.getObject(n)), + // [#9154] This double used to OMIT `getAllObjects`, and every test here + // passed anyway: the engine's roll-up summary index read it as + // `getAllObjects?.() ?? []`, so a double that does not model the method + // was indistinguishable from a registry with nothing in it — the write + // path silently skipped the insert-time roll-up seed (#5749) and the + // post-write recompute. With the optional call gone the omission is a + // hard `TypeError`, which is the point: the double now has to model the + // method the engine actually calls. Empty is the truthful body for THIS + // suite — it declares no `summary` field, so the roll-up index over it is + // empty either way, and now it says so instead of the engine inventing it. + getAllObjects: vi.fn(() => []), registerObject: vi.fn(), getObjectOwner: vi.fn(), registerNamespace: vi.fn(), diff --git a/packages/objectql/src/engine-multivalue-normalize.test.ts b/packages/objectql/src/engine-multivalue-normalize.test.ts index f1f226a3aa..520824eb0f 100644 --- a/packages/objectql/src/engine-multivalue-normalize.test.ts +++ b/packages/objectql/src/engine-multivalue-normalize.test.ts @@ -22,6 +22,17 @@ vi.mock('./registry', () => { const instance: any = { getObject: vi.fn(), resolveObject: vi.fn((n: string) => instance.getObject(n)), + // [#9154] This double used to OMIT `getAllObjects`, and every test here + // passed anyway: the engine's roll-up summary index read it as + // `getAllObjects?.() ?? []`, so a double that does not model the method + // was indistinguishable from a registry with nothing in it — the write + // path silently skipped the insert-time roll-up seed (#5749) and the + // post-write recompute. With the optional call gone the omission is a + // hard `TypeError`, which is the point: the double now has to model the + // method the engine actually calls. Empty is the truthful body for THIS + // suite — it declares no `summary` field, so the roll-up index over it is + // empty either way, and now it says so instead of the engine inventing it. + getAllObjects: vi.fn(() => []), registerObject: vi.fn(), getObjectOwner: vi.fn(), registerNamespace: vi.fn(), diff --git a/packages/objectql/src/engine-validation-locale.test.ts b/packages/objectql/src/engine-validation-locale.test.ts index efb8e701ff..e051ad19ce 100644 --- a/packages/objectql/src/engine-validation-locale.test.ts +++ b/packages/objectql/src/engine-validation-locale.test.ts @@ -19,6 +19,17 @@ vi.mock('./registry', () => { const instance: any = { getObject: vi.fn(), resolveObject: vi.fn((n: string) => instance.getObject(n)), + // [#9154] This double used to OMIT `getAllObjects`, and every test here + // passed anyway: the engine's roll-up summary index read it as + // `getAllObjects?.() ?? []`, so a double that does not model the method + // was indistinguishable from a registry with nothing in it — the write + // path silently skipped the insert-time roll-up seed (#5749) and the + // post-write recompute. With the optional call gone the omission is a + // hard `TypeError`, which is the point: the double now has to model the + // method the engine actually calls. Empty is the truthful body for THIS + // suite — it declares no `summary` field, so the roll-up index over it is + // empty either way, and now it says so instead of the engine inventing it. + getAllObjects: vi.fn(() => []), registerObject: vi.fn(), getObjectOwner: vi.fn(), registerNamespace: vi.fn(),