Uh oh!
There was an error while loading. Please reload this page.
perf(security): batch the derived half of bootstrapSystemCapabilities, unnarrowed - #12521
Conversation
…, unnarrowed
The curated half was batched into one predicated `$in` read; the derived half —
the union of every `systemPermissions` string nothing declares — still read one
row at a time, so a rebuild cost `1 + derived` round trips.
Two objections had kept it per-item. Narrowing the read to the platform bucket
answers a different question and reverses ruled ground; batching it unnarrowed
needed an unbounded read. The second expired when `readNamePage` started asking
for one row more than its page budget and reporting the overflow as
"could not answer", degrading loudly to the per-item read.
The derived half now consults its own unpredicated `buildExistingByName` index.
Measured equivalence to the read it replaces: no predicate, so the emitted
where is `{ name: { $in } }` and nothing else; `seedCtx()` is the same
`{ isSystem: true }` context; and unscoped `resolveOwnOrganizationRow` returns
the first row with no bucket filter, so the index resolves to the same
lowest-id row installation-wide the per-item read returned. Steady-state
rebuild: 2 reads at every derived size.
The first objection stands and is now pinned, not merely documented — a new
test asserts the derived read's key set is `name` alone. Narrowing it would
silence the platform-stamped-in-org anomaly signal in exactly the case its doc
says it is counted for, and would seed the platform bucket in the case ruled
must be left alone.
One behaviour change: a derived name whose read cannot answer is now declined
and counted in `unreadable`, not read as absent. The old path swallowed a
failed read into `[]` and attempted an insert — a duplicate placeholder
wherever the read failed but the write did not.
Co-authored-by: Claude📓 Docs Drift CheckThis PR changes 1 package(s): 3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also name something this change touched. These are read-only:
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): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 50dbe4f03aa79a3660004135edc6c28eac9a257f && git checkout 50dbe4f03aa79a3660004135edc6c28eac9a257f
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 14b114553c3e7c61672e3d25a1d15036f7561e27 1c752670517f0af4ae965f3a6094406ff59f0053 && git checkout -B drift-repro 14b114553c3e7c61672e3d25a1d15036f7561e27 && git merge --no-ff 1c752670517f0af4ae965f3a6094406ff59f0053
node scripts/docs-audit/affected-docs.mjs --json 14b114553c3e7c61672e3d25a1d15036f7561e27
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#11520
Batches the DERIVED half of
bootstrapSystemCapabilities, unnarrowed — option 1 of the three the card enumerated. ⛔ Option 2 (bucket-scoped read) is not taken, and this PR adds a pin that stops it being taken by accident.The measurement that decided the card, done first
The card's premise is that the derived half needs
X= the lowest-idrow for the name, unscoped and installation-wide, while the shared index inseed-name-lookup.tswas built for the curated half, which carries the #8470 predicate inside its query. So: canbuildExistingByName/readNamePageanswer the unscoped question as they stand?Yes, with no change to the shared file. Four properties, each read off the merged source rather than inferred:
whereis{ name: { $in } }and nothing elsereadNamePage:{ name: { $in: names }, ...(equals ?? {}) }— the spread contributes nothing when omitted{ name }, with no key addedseedCtx(undefined)is{ isSystem: true }per-organization-catalog.tsSYSTEM_CTX; the read stays unscoped and cross-organizationresolveOwnOrganizationRowreturnslist[0]with no bucket filterper-organization-catalog.tsX, notB— this is the single fact that decides the cardreadNamePagepasseslimit;buildExistingByNameaccumulatesindex.get(name)[0]is the same rowtryFind(…, 1)[0]returned under #4363'sORDER BY id ASCThe sibling seeder on this very table already records the same equivalence for its own unscoped adoption —
bootstrap-declared-capabilities.ts: "an unscoped lookup is EXACTLY the question the per-item read asked —resolveOwnOrganizationRowreturns the first row when no organization is given, which is whattryFind(…, 1)[0]returned."Why the card's own objection expired
The card recommended option 1 but noted it "costs new machinery in a file four callers now share". That cost was paid by PR #11962:
readNamePagenow asks for one row more than its page budget and reports the overflow astruncated= "could not answer", degrading — loudly — to the per-item read. So "batching it unnarrowed needs an unbounded read" stopped being true. The worst case is now the old per-item cost plus a warning naming the budget.What did NOT change, and is now pinned
⛔ The derived read is not narrowed to
organization_id: null. Narrowing asks forB, the bucket occupant, which differs fromXwhenever an organization's row sorts lower — and the two divergences land on ruled ground in both directions: it would stop #8751'splatformStampedInOrgsignal firing in exactly the case its doc says it is counted for, and it would turn #8552's deliberate decline-to-seed into an insert. Neither has a maintainer ruling.New test:
⭐ the DERIVED read is UNNARROWED — name only, no bucket predicate (#8552/#8751), asserting the emittedwhereand its key set separately (a leakedorganization_id: undefinedwould passtoEqualwhile changing the question the driver is asked).One behaviour change, named rather than buried
A derived name whose existence read cannot answer is now DECLINED (counted in
unreadable) instead of being read as absent.Previously the derived half read through
tryFind, which catches and returns[]— so an unreadable database read as "absent" and routed every derived name to its INSERT branch. Where the read failed but the write did not (a transient read timeout, a lagging replica) that is a duplicate placeholder, refused only where the unique index happens to exist, and silent either way because theblockedCurateddiagnostic is curated-only. Declining is what the shared oracle's module header already requires of every other caller, and is the direction #10946 chose deliberately for the curated half.Consequently
unreadableand its summary warning now cover both halves; the warning reports the whole definition set as its total rather than the curated count it would otherwise exceed.Round trips
bootstrap-seed-round-trips.test.tsstated the residue as1 + derivedand its own doc said "a later card that batches it is expected to move these numbers deliberately." Moved, with the reason recorded in the suite doc:finds=[1, 6, 21]at derived sizes[0, 5, 20]finds=[1, 2, 2]— one curated read, one derived read, and at size 0 the derived read is not issued at all becausebuildExistingByNamereturns before reading when no name survives its filter.The anti-vacuity half is kept explicit: a
findsof 2 reached by skipping the derived half would satisfy that line, sounchangedis pinned alongside it (CURATED + dat every size) to prove every derived name was looked up, judged ours, and found already correct.Dissolution verification of both new negative pins
Each mutation was proven on disk (
git hash-objectbefore/after, anchor matched exactly once), run, then restored withgit checkout HEAD --against the absolute repo-root path under anEXIT INT TERMtrap, withgit diff HEADandgit status --porcelainboth empty afterwards.1 — narrow the derived read to the platform bucket (option 2's exact shape): 12 tests RED, and they are the right twelve:
That is empirical confirmation that option 2 really does reverse both rulings, that the new pin catches it, and that what ships here is not it.
2 — restore the old swallow (
unknown && !isDerived): exactly 1 test RED, the new tri-state pin,AssertionError: expected 8 to be 10(8 curated names vs 8 + 2 derived).Verification
Gate union re-derived on the final tree with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack(no hand-built path list): 3 paths vs merge base7c368e85c, three-dot. All 19 matched families plus the convention-triggered set ran green on head1c752670.pnpm --filter @objectstack/plugin-security run test— 83 files, 1541 tests passedpnpm --filter @objectstack/plugin-security run typecheck— exit 0OK ObjectQL double limit conformance holds … none new·check-i18n-bundles: OK (9 packages — all bundles in sync)check:i18nfirst returnedPREREQUISITE NOT MET — the workspace CLI is not built, which the gate itself states checks nothing. Read as NOT MEASURED, not as a pass: the CLI was built and the gate re-run to get the real verdict above.typecheckexcludes its test files (itstsconfig.jsonsays so) and the package carries no test-typecheck ledger, so the types of the edited test file are NOT MEASURED here — it is exercised at runtime by vitest. Verified withtsc --listFiles: the source file is in the program, the test file is not.--no-inline-config --format json): 2 files, 0 errors, 0 warnings. The narrowing is sound becauseeslint.config.mjsstates in its own text that this repo "never enables type-aware linting (noparserOptions.project, no typed@typescript-eslintrules) for ANY file, test or not" — there is no cross-file program, so this diff cannot move the verdict on any untouched file.⛔
bootstrap-platform-admin.tswas not touched (read-coupled to the pin in #12512, which has since merged — this branch is based on7c368e85, that merge). Zeropackages/spec.Generated by Claude Code