diff --git a/.changeset/derived-capability-existence-read-batched.md b/.changeset/derived-capability-existence-read-batched.md new file mode 100644 index 0000000000..5b6cf7b5cf --- /dev/null +++ b/.changeset/derived-capability-existence-read-batched.md @@ -0,0 +1,43 @@ +--- +"@objectstack/plugin-security": patch +--- + +perf(security): batch the derived half of `bootstrapSystemCapabilities`, unnarrowed (#11520) + +`bootstrapSystemCapabilities` reconciles two halves. #11451 batched the CURATED +half into one `$in` read carrying the #8470 predicate and left the DERIVED +half — the union of every `systemPermissions` string that nothing declares — +reading one row at a time, so a rebuild cost `1 + derived` round trips. + +That residue was filed rather than fixed for a reason that has since expired. +Two objections stood: narrowing the derived read to the platform bucket answers +a different question and reverses ruled ground, and batching it *unnarrowed* +needed an unbounded read. #11518 removed the second one — `readNamePage` now +asks for one row more than its page budget and reports the overflow as +`truncated` = "could not answer", degrading loudly to the per-item read — so +the wide batched read became bounded without becoming a different question. + +The derived half now consults its own `buildExistingByName` index, built with +**no predicate**: the read emits `{ name: { $in: … } }` under `seedCtx()` +(`{ isSystem: true }`, the same context the per-item read used), and unscoped +`resolveOwnOrganizationRow` returns the FIRST row with no bucket filter — so +the index resolves to the same lowest-`id` row installation-wide that +`tryFind(…, 1)[0]` returned under #4363's `ORDER BY id ASC`. A steady-state +rebuild costs 2 reads at every derived size instead of `1 + derived`. + +⛔ The first objection still stands and is now pinned rather than only +documented: the derived read is **not** narrowed to `organization_id: null`. +Doing so would silence #8751's `platformStampedInOrg` anomaly signal in exactly +the case its doc says it is counted for, and would seed the platform bucket in +the case #8552 ruled must be left alone. A new test asserts the derived read's +key set is `name` and nothing else. + +One behaviour change, in the direction #10946 chose deliberately for the +curated half: a derived name whose existence read **cannot answer** is now +DECLINED (counted in `unreadable`) instead of being read as absent. The old +`tryFind` swallowed a failed read into `[]`, which routed the name to its +insert branch — a duplicate placeholder wherever the read failed but the write +did not, refused only where the unique index happens to exist, and silent +either way because the `blockedCurated` diagnostic is curated-only. The +`unreadable` counter and its summary warning now cover both halves; the warning +reports the whole definition set as its total rather than the curated count. diff --git a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts index 1f2dc708e1..a3c42c4132 100644 --- a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts @@ -751,27 +751,39 @@ describe('#11096 — a read that CANNOT ANSWER is not the answer "none exist"', * * ## What is pinned, and what is deliberately NOT * - * Two halves, and only ONE of them is batched: + * Two halves, and since #11520 BOTH are batched — as two SEPARATE reads asking + * two different questions, which is the whole subtlety: * - * - the CURATED half (`KNOWN_CAPABILITIES`) now costs ONE batched `$in` read + * - the CURATED half (`KNOWN_CAPABILITIES`) costs ONE batched `$in` read * carrying the #8470 predicate, and zero writes on a steady-state rebuild; - * - the DERIVED half keeps its per-item read, because its question is - * cross-organization by construction and its counters are computed from the - * lowest-id row installation-wide (see the seeder's header). The pins below - * state that residue as `1 + derived` rather than hiding it — a later card - * that batches it is expected to move these numbers deliberately. + * - the DERIVED half costs ONE batched `$in` read carrying NO predicate. It + * stays wide because its question is cross-organization by construction and + * its counters are computed from the lowest-id row installation-wide (see the + * seeder's header). ⛔ A "simplification" that folds the two into one read — + * or that narrows the derived one to the platform bucket to make folding + * possible — reverses #8552 and #8751 by read shape; the predicate pin below + * and the `platformStampedInOrg` suite in `bootstrap-system-capabilities.test + * .ts` are what stop it. + * + * ⚠️ These counts MOVED in #11520, deliberately: this doc previously stated the + * derived residue as `1 + derived` and said "a later card that batches it is + * expected to move these numbers deliberately". #11518 removed the objection + * that kept it per-item (the page cap became a measurement, so an unnarrowed + * batched read that truncates degrades loudly instead of silently reading + * `absent` and inserting), so the residue is now a second constant read rather + * than a linear one. * * ⚠️ NO speedup is claimed. The hosted `bootstrap-curve.mjs` rig lives in * `objectstack-ai/cloud` and its axes are permission sets / positions / objects, * not this one. These tests count round trips and pin WHICH ROW each leg * touched; nothing here measures wall time. */ -describe('#11451 — the curated half is O(1) round trips, the derived half is the filed residue', () => { +describe('#11451/#11520 — BOTH halves are O(1) round trips, as two differently-shaped reads', () => { const CURATED_NAMES = KNOWN_CAPABILITIES.map((c) => c.name); const derivedSets = (n: number) => [{ systemPermissions: Array.from({ length: n }, (_, i) => `app.cap.${i}`) }]; const capQl = (behaviour = {}) => makeCountingQl('sys_capability', 'capability', [], behaviour); - it('the curated existence read is ONE round trip at every derived size', async () => { + it('the existence reads are O(1) at every derived size — the residue is GONE', async () => { const measure = async (d: number) => { const ql = capQl(); await bootstrapSystemCapabilities(ql, derivedSets(d)); // first boot: seeds @@ -780,14 +792,24 @@ describe('#11451 — the curated half is O(1) round trips, the derived half is t expect(r.seeded).toBe(0); expect(r.updated).toBe(0); // ⬅ the gate expect(r.unchanged).toBe(CURATED_NAMES.length + d); - return { finds: ql.calls.find, updates: ql.calls.update, derived: d }; + return { finds: ql.calls.find, updates: ql.calls.update, unchanged: r.unchanged, derived: d }; }; const rows = [await measure(0), await measure(5), await measure(20)]; - // The curated half contributes exactly 1 read at every size; the remaining - // `d` are the derived half's per-item reads, which this card does not batch. - expect(rows.map((x) => x.finds - x.derived)).toEqual([1, 1, 1]); - expect(rows.map((x) => x.finds)).toEqual([1, 6, 21]); + // [#11520] FLAT, not `1 + d`. One read for the curated half, one for the + // derived half — and at d=0 the derived read is not issued at all, because + // `buildExistingByName` returns before reading when no name survives its + // filter. That asymmetry is the reason the expectation is written out per + // size rather than as a single constant. + expect(rows.map((x) => x.finds)).toEqual([1, 2, 2]); + // ⭐ The anti-vacuity half lives in `measure` itself, and it has to: a + // `finds` of 2 reached by SKIPPING the derived half would satisfy the line + // above. `expect(r.unchanged).toBe(CURATED_NAMES.length + d)` there is what + // rules that out — every derived name was looked up, judged ours, and found + // already correct. Restated here so the count and the work are read together: + expect(rows.map((x) => x.unchanged)).toEqual([ + CURATED_NAMES.length, CURATED_NAMES.length + 5, CURATED_NAMES.length + 20, + ]); // ⬅ The unconditional UPDATE is gone from BOTH halves. expect(rows.map((x) => x.updates)).toEqual([0, 0, 0]); }); @@ -809,6 +831,36 @@ describe('#11451 — the curated half is O(1) round trips, the derived half is t expect(Object.keys(ql.wheres[0]).sort()).toEqual(['managed_by', 'name', 'organization_id']); }); + /** + * ⭐ [#11520] The RULED pin on the derived read's SHAPE. `bootstrap-system- + * capabilities.test.ts` pins the consequences (#8751's `platformStampedInOrg`, + * #8552's untouched bucket); this pins the cause, because the cheap fix that + * reverses both is a one-key edit right here. + * + * The derived question is "the lowest-id row for this name, installation-wide" + * (`X`). Adding `organization_id: null` asks for the bucket occupant (`B`) + * instead — a different row whenever an organization's row sorts lower — which + * silently stops #8751's signal and turns #8552's deliberate decline-to-seed + * into an insert. Neither has a maintainer ruling. So the derived read carries + * `name` and NOTHING else. + */ + it('⭐ the DERIVED read is UNNARROWED — `name` only, no bucket predicate (#8552/#8751)', async () => { + const ql = capQl(); + await bootstrapSystemCapabilities(ql, derivedSets(3)); + ql.reset(); + await bootstrapSystemCapabilities(ql, derivedSets(3)); + + // Two reads, in loop order: curated (predicated) then derived (wide). + expect(ql.calls.find).toBe(2); + const derivedWhere = ql.wheres[1]; + expect(derivedWhere).toEqual({ name: { $in: ['app.cap.0', 'app.cap.1', 'app.cap.2'] } }); + // The KEY SET separately, for the same reason the curated pin above does it: + // `toEqual` ignores `undefined`-valued properties, so a leaked + // `organization_id: undefined` would pass the assertion above while changing + // the question the driver is asked. + expect(Object.keys(derivedWhere).sort()).toEqual(['name']); + }); + it('the existing callers still emit their exact key set — no predicate leaked in', async () => { // The measurement fence: `buildExistingByName` gained an optional predicate, // and a caller that passes none must emit the keys it emitted before, not @@ -883,8 +935,8 @@ describe('#11451 — the curated half is O(1) round trips, the derived half is t scope: c.scope, managed_by: 'platform', organization_id: null, active: true, }))); const warns: string[] = []; - // No derived names: the derived half's `tryFind` swallows a failed read by - // design, so mixing one in would measure that half instead of this one. + // No derived names here: this pin is about the CURATED half, and #11520 adds + // the derived counterpart as its own test below rather than widening this one. const r = await bootstrapSystemCapabilities(broken, [], { logger: { warn: (m) => warns.push(m) } }); expect(r.unreadable).toBe(KNOWN_CAPABILITIES.length); expect(r.seeded).toBe(0); @@ -893,4 +945,45 @@ describe('#11451 — the curated half is O(1) round trips, the derived half is t expect(warns.some((w) => w.includes('batched seed existence read failed'))).toBe(true); expect(warns.some((w) => w.includes('could not be read'))).toBe(true); }); + + /** + * ⭐ [#11520] The derived counterpart — and the one place this card changes + * observable behaviour, pinned so the change is a decision rather than a + * side effect. + * + * BEFORE: the derived half read through `tryFind`, which catches and returns + * `[]`. An unreadable database therefore 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 where + * the insert failed too it was silent, because the `blockedCurated` diagnostic + * is curated-only. + * + * AFTER: `unknown` is declined, exactly as the shared oracle's module header + * requires of every other caller. Strictly stricter, in the direction #10946 + * chose deliberately for the curated half and #11518 extended to truncation. + */ + it('⭐ [#11520] a DERIVED name whose read cannot answer is DECLINED, never blind-inserted', async () => { + const DERIVED = 2; + const broken = capQl({ findThrows: true }); + broken.rows.push(...KNOWN_CAPABILITIES.map((c, i) => ({ + id: `cap_${i}`, name: c.name, label: c.label, description: c.description, + scope: c.scope, managed_by: 'platform', organization_id: null, active: true, + }))); + const before = broken.rows.length; + const warns: string[] = []; + const r = await bootstrapSystemCapabilities(broken, derivedSets(DERIVED), { + logger: { warn: (m) => warns.push(m) }, + }); + + // BOTH halves decline — every definition is left entirely alone. + expect(r.unreadable).toBe(KNOWN_CAPABILITIES.length + DERIVED); + expect(r.seeded).toBe(0); + // ⛔ LOAD-BEARING: the derived insert that used to happen here does not. + expect(broken.calls.insert).toBe(0); + expect(broken.rows).toHaveLength(before); + // …and the summary warning covers both halves, so its total is the whole + // definition set rather than the curated count it would otherwise exceed. + expect(warns.some((w) => w.includes('capabilities left untouched'))).toBe(true); + }); }); diff --git a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts index 382ee3d645..0e2dfb4f6c 100644 --- a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts +++ b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts @@ -41,6 +41,12 @@ * organizations by construction — which is correct for a seeder, and is exactly * why the predicate has to say which row it means. * + * [#11520] The two batched reads inherit that property rather than re-deriving + * it: `seed-name-lookup.ts` reads under `seedCtx(organizationId)`, and with no + * organization threaded that is `{ isSystem: true }` — the same object literal as + * `SYSTEM_CTX`. Both batched reads and the surviving `tryFind` bucket reads + * therefore ask under one context. + * * Since #8461 made `sys_capability.name` unique per ORGANIZATION rather than per * installation (ADR-0120 D1, the cross-tenant existence oracle #8323 reports), an * admin may author `manage_users` inside their organization while the platform @@ -178,13 +184,24 @@ * `seed-name-lookup.ts`: that module's index answers with ONE row chosen by * arrival order, so filtering needs an all-rows accessor, which hands every * caller its own spelling of "which row is mine" — the shape #10103 repaired. - * - The DERIVED half keeps its per-item read. Not because it is smaller (it is - * the half that grows) but because its question is cross-organization by - * construction and its counters — `skippedAuthored`, and #8751's - * `platformStampedInOrg` — are computed from the lowest-id row installation- - * wide. Narrowing it to the platform bucket answers a different question and - * silently reverses part of the #8552 ruling; batching it unnarrowed needs an - * unbounded read. Filed rather than taken. + * - The DERIVED half kept its per-item read, filed rather than taken. Not + * because it is smaller (it is the half that grows) but because its question + * is cross-organization by construction and its counters — `skippedAuthored`, + * and #8751's `platformStampedInOrg` — are computed from the lowest-id row + * installation-wide. Narrowing it to the platform bucket answers a different + * question and silently reverses part of the #8552 ruling; batching it + * unnarrowed needed an unbounded read. + * + * [#11520] The derived half is now batched too — UNNARROWED, on the second of + * those two objections being removed rather than accepted. #11518 turned the + * page cap from a promise into a measurement (`readNamePage` asks for one row + * more than its budget and calls the overflow `truncated` = "could not answer", + * degrading to the per-item read), so "batching it unnarrowed needs an unbounded + * read" stopped being true. The FIRST objection is untouched and still binding: + * ⛔ the derived read is not narrowed to the platform bucket, because that + * answers a different question and reverses #8552 and #8751 by read shape. The + * seeder therefore issues two batched reads that ask different questions — the + * curated one predicated (#8470), the derived one deliberately wide. * * ⚠️ NOTHING here claims a measured speedup. The hosted `bootstrap-curve.mjs` * rig lives in `objectstack-ai/cloud` and its axes are permission sets / @@ -355,8 +372,16 @@ export interface CapabilitySeedResult { */ unchanged: number; /** - * [#11451] Curated definitions left ENTIRELY alone because the existence read - * could not answer — not read as absent, and therefore never inserted. + * [#11451] Definitions left ENTIRELY alone because the existence read could + * not answer — not read as absent, and therefore never inserted. + * + * [#11520] Counts BOTH halves since the derived read was batched. It was + * curated-only while the derived half swallowed a failed read into `[]` and + * went on to attempt an insert; that half now declines on `unknown` like every + * other caller of the shared oracle, so its unanswerable names land here. One + * counter rather than two, deliberately: the fact reported is "this pass could + * not find out", and which half asked does not change the remedy (the next + * boot with a readable database does the work). * * ⛔ This is the counter that exists because hoisting a read out of a loop * changes what a failure means. Per item, a failed read fell through to an @@ -431,19 +456,8 @@ export async function bootstrapSystemCapabilities( // .ts`'s own index comment names ("exactly the bucket this key part keeps a // singleton"). // - // ⛔ The DERIVED half is deliberately NOT batched, and the reason is not that - // it is the smaller half — it is the half that GROWS. Its lookup is `{ name }` - // across organizations by construction, and everything it does with the row it - // finds depends on WHICH row that is: `derivedRowIsOurs`, `skippedAuthored` - // and the #8751 `platformStampedInOrg` anomaly signal are all computed from - // the lowest-id row installation-wide. Narrowing that read to the platform - // bucket answers a DIFFERENT question — it would stop counting an - // organization's platform-stamped row whenever our own bucket row also exists, - // and it would start seeding the bucket in the very case #8552 ruled must be - // left alone. Batching it WITHOUT narrowing it needs an unbounded read. Both - // are decisions above this card, so both are filed rather than taken. What - // this card does remove from the derived half is its WRITE: the reconcile - // below is now equality-gated for both halves. + // [#11451] What this half does NOT do is narrow the derived read — see the + // derived index below, which #11520 batched on the terms #11451 could not. const curatedExisting = await buildExistingByName( ql, 'sys_capability', @@ -453,31 +467,95 @@ export async function bootstrapSystemCapabilities( CURATED_LOOKUP, ); + // [#11520] The DERIVED half, batched — UNNARROWED, which is the only shape + // that preserves what it computes. #11451 filed this rather than taking it, + // and the two objections it recorded resolved in opposite ways: one was + // removed by #11518, the other still stands and still forbids the cheap fix. + // + // ## Why the batched read is the SAME question, not a cheaper one + // + // Everything this half does depends on WHICH row it finds: `derivedRowIsOurs`, + // `skippedAuthored` and #8751's `platformStampedInOrg` are all read off `X` — + // the lowest-id row for the name, installation-wide. Three properties, each + // checked against the shared module rather than assumed, make the index answer + // with that same `X`: + // + // - NO predicate is passed, so `readNamePage` emits `{ name: { $in: … } }` — + // the set-widening of the per-item `{ name }`, with no key added (the + // `...(equals ?? {})` spread contributes nothing when omitted); + // - the read runs under `seedCtx(undefined)` — `{ isSystem: true }`, byte- + // identical to this file's `SYSTEM_CTX`, so it stays unscoped and + // cross-organization; + // - unscoped, `resolveOwnOrganizationRow` returns the FIRST row and applies + // NO bucket filter, so the index resolves to the row `tryFind(…, 1)[0]` + // returned under #4363's `ORDER BY id ASC`. The sibling seeder on this very + // table 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"). + // + // ## What #11518 removed + // + // The blocking objection was the PAGE CAP, not the question: this set is + // bounded only by the number of organizations, against a page that was capped + // at one row per name — so it truncated, and a truncated page reads as + // `absent`, which INSERTS. `readNamePage` now asks for one row MORE than its + // budget and reports the overflow as `truncated` — "could not answer" — + // degrading, loudly, to exactly the per-item read this half used to do + // unconditionally. So the unbounded-read trade #11451 declined no longer + // exists: the worst case is the old cost plus a warning naming the budget. + // + // ## What is still forbidden + // + // ⛔ NARROWING this to the platform bucket (`organization_id: null`). It + // answers a DIFFERENT question — `B`, the bucket occupant, not `X` — and the + // two diverge on ruled ground in both directions: it would stop counting an + // organization's platform-stamped row whenever our own bucket row also exists + // (#8751's signal, in precisely the case its doc says it is counted for), and + // it would seed the bucket in the very case #8552 ruled must be left alone. + // Both are maintainer decisions and neither has been made, so the read stays + // wide and pays a page budget instead. + const derivedExisting = await buildExistingByName( + ql, + 'sys_capability', + [...derivedNames], + options.logger, + ); + for (const def of byName.values()) { const isDerived = derivedNames.has(def.name); - // [#8470] CURATED: address the platform's OWN row. DERIVED: unchanged — its - // own `managed_by` guard below is what keeps it off rows it does not own - // (#5876), and narrowing its lookup here would change a half this card - // deliberately leaves alone. - let row: any; - if (isDerived) { - row = (await tryFind(ql, 'sys_capability', { name: def.name }, 1))[0]; - } else { - const found = await curatedExisting.get(def.name); - if (found.status === 'unknown') { - // ⛔ [#10946] "I could not find out" is not the answer "it is not there". - // The per-item shape was accidentally immune to the conflation — a failed - // read fell through to an insert the unique index refused, for that one - // name — and a batched read is not, because one failure now speaks for - // the entire set. Declining is also STRICTER than the code it replaces, - // deliberately: on an unreadable database this half used to attempt an - // insert per curated name and then report a `blockedCurated` collision - // for each, describing a row nobody ever saw. - unreadable += 1; - continue; - } - row = found.status === 'present' ? found.row : undefined; + // [#8470] CURATED: address the platform's OWN row. DERIVED: still the WIDE + // question — its own `managed_by` guard below is what keeps it off rows it + // does not own (#5876), and narrowing its lookup is the thing #8552/#8751 + // forbid rather than an optimisation left undone. + // [#11520] ONE consumption idiom for both halves. They consult DIFFERENT + // indexes — the curated one carries the #8470 predicate, the derived one is + // deliberately unpredicated — but "what does a lookup answer mean" is one + // question with one answer, and a second spelling of it is how the two halves + // would drift apart again. + const existing = isDerived ? derivedExisting : curatedExisting; + const found = await existing.get(def.name); + if (found.status === 'unknown') { + // ⛔ [#10946] "I could not find out" is not the answer "it is not there". + // The per-item shape was accidentally immune to the conflation — a failed + // read fell through to an insert the unique index refused, for that one + // name — and a batched read is not, because one failure now speaks for + // the entire set. Declining is also STRICTER than the code it replaces, + // deliberately: on an unreadable database this half used to attempt an + // insert per curated name and then report a `blockedCurated` collision + // for each, describing a row nobody ever saw. + // + // [#11520] The DERIVED half reaches this branch now too, and there the + // strictness replaces something worse than a phantom diagnostic: its + // `tryFind` swallowed a failed read into `[]`, which reads as absent, so + // the half went on to attempt an insert. Where the read failed but the + // write did not — a transient read timeout, a replica lagging — that + // insert is a DUPLICATE placeholder, refused only if the unique index + // happens to exist. Declining is the answer the module header already + // requires of every other caller of this oracle. + unreadable += 1; + continue; } + const row: any = found.status === 'present' ? found.row : undefined; if (row?.id) { // [#5876] Reconcile display fields only where THIS pass owns the copy. // @@ -653,7 +731,12 @@ export async function bootstrapSystemCapabilities( // keyed by name, so a curated name cannot repeat within one pass — this // records the row anyway rather than making the batched read depend on // an invariant that lives somewhere else. - if (!isDerived) curatedExisting.remember(def.name, payload); + // + // [#11520] …and now the derived half records on ITS index for the same + // reason. `byName` is keyed by name so no name repeats within one pass + // in either half; both record anyway, because "the read cannot see rows + // this loop just inserted" is a property of the hoist, not of the half. + existing.remember(def.name, payload); } else if (!isDerived) { // [#8470] The curated row is absent AND could not be written — the // NULL-organization bucket already holds the name under a row the @@ -719,12 +802,17 @@ export async function bootstrapSystemCapabilities( // [#11096] Said ONCE with the count, like the sibling seeders: a per-name // warn on a database that is down is a log flood that buries its own // meaning. The consequence is spelled out because "unreadable" alone does - // not state one — these curated definitions were left ENTIRELY alone, so one - // that is genuinely absent has not been seeded and a drifted one has not been + // not state one — these definitions were left ENTIRELY alone, so one that is + // genuinely absent has not been seeded and a drifted one has not been // reconciled; the next boot with a readable database does both. + // + // [#11520] "capabilities", not "curated capabilities": the derived half can + // land here too now. `total` is the whole definition set for the same + // reason — measured against `KNOWN_CAPABILITIES.length` a count that + // included derived names could exceed its own total. options.logger?.warn?.( - '[security] curated capabilities left untouched — their sys_capability rows could not be read', - { unreadable, total: KNOWN_CAPABILITIES.length }, + '[security] capabilities left untouched — their sys_capability rows could not be read', + { unreadable, total: byName.size }, ); } options.logger?.info?.('[security] system capabilities seeded into sys_capability (ADR-0066 D1)', {