Uh oh!
There was an error while loading. Please reload this page.
fix(objectql): the roll-up summary index's registry read propagates, and a failed read is never cached as an empty index (#9154) - #9284
Merged
Conversation
…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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
…e; 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
Contributor
📓 Docs Drift Check2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ✅ What this run could not see
Coarse fallback — 14 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
os-zhuang
marked this pull request as ready for review
August 17, 2026 11:46
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#9154
ObjectQL.buildSummaryIndex()opened by answering a registry read that could notrun with an invented "no object declares a roll-up", and
ensureSummaryIndexes()then memoized that invention, stamped with the registry's current
objectRevision.recomputeSummaries()consults that index after every insert /update / 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.
What changed
1. The read propagates — the canonical shape PR #9163 landed for the two
delete-cascade seams:
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
catchand the optional call?.(), which absorbed a registry that omitsgetAllObjectsentirely — the structural omission that never throws and is thereforeinvisible.
2. A failed build leaves no cache entry — the limb that made this card worse than
its two siblings. 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. A poisoned cache
entry must not survive the read that poisoned it.
Measured, not assumed
PM assumption 1 —
getAllObjects()has no throwing path — HOLDS. Re-derived ontoday's tree:
SchemaRegistry.getAllObjects()walksobjectContributorsand callsresolveObject(), which returnsundefinedon every failure branch it models (nocontributors, no owner — the orphan-overlay branch after a
console.warn); below itfoldExtendersOntoDefinition→mergeObjectDefinitionsis spreads and conditionals,and
scalarOverridesPackagedBaseis three comparisons. No I/O, no driver, nothrow.This is a structural close, not a live defect.
PM assumption 2 — the poisoned cache's lifetime — HOLDS, and is now measured
rather than argued.
objectRevisionmoves on exactly seven sites, all metadatamutations:
registerObject,removeObjectOverlay,unregisterObjectsByPackage,unregisterObject,invalidate,invalidateAll,reset. None is on a data write.A probe run against the pre-fix engine — poison the first index build, heal the
registry, then perform four ordinary writes with no registry mutation at all:
Pre-fix the parent row carries no roll-up keys at all — not even the insert-time
seed of 0 — while three children summing 47 sit in the store and every write returned
success. The registry was healthy for all four writes; only the cached emptiness kept
it wrong. So the severity claim is right: not "briefly wrong", but wrong until a
metadata mutation or a process restart.
PM assumption 3 —
check:durability-log-levelis blind to this — HOLDS. Censusmeasured on both trees, identical: 26 durability-critical catch seams, 66 read seams.
The gate's green certifies nothing here (the #8845 blind spot), and it was not
touched. Note the new
catch/rethrow inensureSummaryIndexesdid not enter thecensus either.
PM assumption 4 — a swallow removal exposes vacuous tests — HOLDS, nine files
worth. Removing the optional call turned 83 tests red across 9 suites, every one
with
TypeError: this._registry.getAllObjects is not a function. Theirvi.mock('./registry')doubles never modelled the method, and the swallow made anincomplete double indistinguishable from an empty registry — so the write path in all
nine was silently skipping the insert-time roll-up seed (#5749) and the post-write
recompute, and no assertion could see it. Each double now declares
getAllObjects: vi.fn(() => []), which is the truthful body there: none of the ninedeclares a
summaryfield, so the roll-up index over it is empty either way, and nowit says so instead of the engine inventing it. Repaired, not suppressed.
Reverse verification
Direction predicted in writing before running: controls green, seam and cache tests
red. Observed exactly that — with the fix reverted and the pins kept, 6 failed / 2
passed; the two that stayed green are the positive controls (a readable registry
recomputes the roll-up; the parent-side descriptors are reported), so no assertion
below them can pass vacuously.
One thing the reverse run taught, and the test file records it: the index is built
lazily, so the failing read must land on a real build, not a cache hit. Arming the
failure after a successful write proves nothing — an
insertover a warm index makeszero
getAllObjects()calls. The pins therefore poison the FIRST build, which is theshape a booting deployment has.
Tests
packages/objectql/src/engine-summary-index-registry-read-failure.test.ts— 8 tests:2 positive controls, the seam's propagation (error identity plus literal
code/status/ message, and the read count), the missing-method case, and four cache-limbtests that re-read
registry.objectRevisionand pin it unchanged across thefailure, so the recovery they prove cannot be the accidental one a metadata mutation
would have produced.
Verification at
65a5e2afa(this PR's head)pnpm --filter @objectstack/objectql test— 214 files, 3763 tests passed(pristine
origin/mainbaseline measured for comparison: 213 files, 3755 tests).pnpm --filter @objectstack/objectql typecheck— clean.@objectstack/metadata-protocol116 files / 1606 tests,@objectstack/rest122files / 1896 tests,
@objectstack/runtime165 files / 2464 tests — all green. (Thefirst
restandruntimeruns showed suite-LOAD failures naming unbuilt siblingpackages, zero test failures; building each package's own dependency closure cleared
them, so they were an artefact of a scoped build, not of this change.)
scripts/pm/dispatch-gates.mjsat this head, all green:check:changeset-gate-self-tests,check:durability-log-level,check:objectui-changeset,check:stack-collection-maps,check:query-options-erasure,check:engine-double-contract,check:where-matcher,check:nul-bytes,check:type-check-coverage,check:cross-package-test-inputs,check-engine-split-ratio.check:type-check-debt --re-measurerefuses without a fully built workspace, so theledger delta was measured directly instead, with objectql's closure built and its
tests included in the program: 355 before, 355 after — the gate's exact recorded
number for
@objectstack/objectql, unmoved. No ledger entry is raised by this PR.Changeset:
.changeset/summary-index-registry-read-propagates.md(patch).Generated by Claude Code