From 968f91714cd92039e34e0c71f43e2627acc9a14d Mon Sep 17 00:00:00 2001 From: os-sam Date: Mon, 24 Aug 2026 02:24:57 +0000 Subject: [PATCH] perf(security): batch the curated capability existence read and gate the reconcile (#11451) `bootstrapSystemCapabilities` issued one `SELECT ... WHERE name = ? LIMIT 1` per curated definition and then an `UPDATE` that fired whether or not `label`/`description` differed. The curated half now costs ONE batched `$in` read, and the reconcile is equality-gated for both halves. The #8470 predicate (`managed_by: 'platform'` + `organization_id: null`) travels INSIDE the batched query rather than filtering its answer: post-#8461 a name can carry a row per organization, so the wide question returns an unbounded set against a page capped at one row per name, and a truncated page reads as "absent", which inserts. The derived half keeps its per-item read - its question is cross-organization by construction and its counters derive from the lowest-id row installation- wide. Filed as #11520 rather than decided here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 --- ...batch-curated-capability-existence-read.md | 59 ++++++ .../src/bootstrap-seed-round-trips.test.ts | 159 +++++++++++++++ .../src/bootstrap-system-capabilities.test.ts | 128 +++++++++++- .../src/bootstrap-system-capabilities.ts | 184 ++++++++++++++++-- .../plugin-security/src/seed-name-lookup.ts | 47 ++++- 5 files changed, 558 insertions(+), 19 deletions(-) create mode 100644 .changeset/batch-curated-capability-existence-read.md diff --git a/.changeset/batch-curated-capability-existence-read.md b/.changeset/batch-curated-capability-existence-read.md new file mode 100644 index 0000000000..c26f5d8cd1 --- /dev/null +++ b/.changeset/batch-curated-capability-existence-read.md @@ -0,0 +1,59 @@ +--- +'@objectstack/plugin-security': patch +--- + +Seed the curated platform capabilities with ONE batched existence read, and stop +rewriting rows that already match + +`bootstrapSystemCapabilities` built its whole definition set in memory and then +issued a separate `SELECT … WHERE name = ? LIMIT 1` per definition, followed by +an `UPDATE` that fired whether or not `label`/`description` had changed. On a +local file database that loop is invisible; on the remote libsql/Turso database +every hosted environment runs, each leg is its own sequential HTTP request, +competing for the same boot request budget as everything else. On a stock +installation that is 8 reads plus 8 writes, every `kernel:ready`, to store bytes +already there. + +The curated half's existence read is now one batched `$in`, and the reconcile is +equality-gated. On a steady-state rebuild the curated half costs **1 round trip** +instead of 16, and the write gate sits after the derived-ownership guard, so it +removes the redundant `UPDATE` from **both** halves. + +**The #8470 predicate travels inside the batched query, not applied to its +answer.** The curated half does not ask "is there a row with this name" — it asks +for the platform's own organization-less row (`managed_by: 'platform'` + +`organization_id: null`), and since `sys_capability.name` became unique per +ORGANIZATION those are different questions. Batching the wide question and +filtering afterwards reads every organization's row for every curated name — a +set bounded only by the number of organizations — against a page capped at one +row per name, so the page truncates, and a truncated page reads as "absent", +which inserts. Both harms are pinned as tests rather than argued: without the +predicate the shared name resolves to an organization's row, and two curated +names whose platform rows demonstrably exist come back absent. + +**An unreadable database now declines instead of guessing.** Hoisting a read out +of a loop changes what a failure means: per item a failed read fell through to an +insert the unique index refused, for that one name; batched, one failure speaks +for the whole set. `unknown` is therefore never read as "absent" — the affected +definitions are left entirely alone, counted in the new `unreadable`, and warned +once. This also retires a misdiagnosis: an unreadable database used to make this +half attempt an insert per curated name and then report a `blockedCurated` +collision for each, describing a blocking row nobody ever saw. + +`CapabilitySeedResult` gains `unchanged` and `unreadable`. Reporting "wrote +nothing because nothing differed" separately from "wrote nothing because the +writes stopped working" is what keeps the round-trip count from being satisfiable +by an implementation that simply stopped reconciling. + +**The derived half keeps its per-item read**, and not because it is the smaller +one — it is the half that grows. Its lookup is cross-organization by +construction, and `skippedAuthored` and the `platformStampedInOrg` anomaly signal +are computed from the lowest-id row installation-wide; narrowing it to the +platform bucket answers a different question and would silently reverse part of a +maintainer ruling, while batching it unnarrowed needs an unbounded read. Filed +rather than taken. + +No speedup is claimed. The hosted boot-curve rig lives in another repository and +its axes are permission sets / positions / objects, not this one. What is +established here is the round-trip count and the identity of the row each leg +reads and writes, both pinned in-repo. 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 74930f2f71..1f2dc708e1 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 @@ -45,6 +45,7 @@ import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; import { bootstrapDeclaredPermissions } from './bootstrap-declared-permissions.js'; import { bootstrapDeclaredPositions } from './bootstrap-declared-positions.js'; import { bootstrapDeclaredCapabilities } from './bootstrap-declared-capabilities.js'; +import { bootstrapSystemCapabilities, KNOWN_CAPABILITIES } from './bootstrap-system-capabilities.js'; interface CountingQl { rows: any[]; @@ -83,6 +84,13 @@ function makeCountingQl( if (key.startsWith('$')) { throw new Error(`counting driver: unsupported combinator ${key}`); } + // [#11451] A `null` comparand is IS NULL, not `=== null`. `driver-sql` + // compiles `{ field: null }` to `IS NULL`; a column never written is NULL + // in the database and `undefined` in this double, and strict equality + // matches neither. Inert for every describe above — none of them issues a + // null comparand — and required by the curated capability read, which is + // predicated on `organization_id: null`. + if (cond === null) return row[key] == null; if (cond && typeof cond === 'object' && !Array.isArray(cond)) { const inList = (cond as any).$in; if (Array.isArray(inList)) return inList.includes(row[key]); @@ -735,3 +743,154 @@ describe('#11096 — a read that CANNOT ANSWER is not the answer "none exist"', expect(r.seeded).toBe(3); }); }); + +/** + * [#11451] `bootstrapSystemCapabilities` — the OTHER capability seeder, whose + * definition set is the union of every `systemPermissions[]` string rather than + * the explicit `defineCapability` declarations. + * + * ## What is pinned, and what is deliberately NOT + * + * Two halves, and only ONE of them is batched: + * + * - the CURATED half (`KNOWN_CAPABILITIES`) now 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. + * + * ⚠️ 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', () => { + 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 () => { + const measure = async (d: number) => { + const ql = capQl(); + await bootstrapSystemCapabilities(ql, derivedSets(d)); // first boot: seeds + ql.reset(); + const r = await bootstrapSystemCapabilities(ql, derivedSets(d)); // REBUILD + 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 }; + }; + + 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]); + // ⬅ The unconditional UPDATE is gone from BOTH halves. + expect(rows.map((x) => x.updates)).toEqual([0, 0, 0]); + }); + + it('the batched read carries the #8470 predicate IN the query, with no other keys', async () => { + const ql = capQl(); + await bootstrapSystemCapabilities(ql, []); + ql.reset(); + await bootstrapSystemCapabilities(ql, []); + expect(ql.calls.find).toBe(1); + expect(ql.wheres[0]).toEqual({ + name: { $in: CURATED_NAMES }, + managed_by: 'platform', + organization_id: null, + }); + // `toEqual` ignores `undefined`-valued properties, so the KEY SET is pinned + // separately: a predicate leaking in as `key: undefined` would pass the + // assertion above while changing what every other caller emits. + expect(Object.keys(ql.wheres[0]).sort()).toEqual(['managed_by', 'name', 'organization_id']); + }); + + 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 + // those keys plus `undefined`-valued ones. + const ql = permissionQl(declaredSets(3)); + await bootstrapDeclaredPermissions(ql, undefined); + ql.reset(); + await bootstrapDeclaredPermissions(ql, undefined); + expect(Object.keys(ql.wheres[0])).toEqual(['name']); + }); + + it('⭐ pins IDENTITY, not just the count: the rebuild re-reads and re-writes the same rows', async () => { + const ql = capQl(); + await bootstrapSystemCapabilities(ql, derivedSets(2)); + const idsAfterFirstBoot = ql.rows.map((r) => r.id).sort(); + expect(idsAfterFirstBoot).toHaveLength(CURATED_NAMES.length + 2); + + ql.reset(); + await bootstrapSystemCapabilities(ql, derivedSets(2)); + // Same rows, same ids — no row was re-created, and no offsetting pair of + // "one dropped, one inserted" is hiding behind a constant count. + expect(ql.rows.map((r) => r.id).sort()).toEqual(idsAfterFirstBoot); + expect(ql.calls.insert).toBe(0); + + // …and when a curated row DRIFTS, the UPDATE lands on THAT row's id. + const target = ql.rows.find((r) => r.name === CURATED_NAMES[0])!; + target.label = 'Hand-edited'; + ql.reset(); + const r = await bootstrapSystemCapabilities(ql, derivedSets(2)); + expect(r.updated).toBe(1); + expect(r.unchanged).toBe(CURATED_NAMES.length + 1); + expect(ql.calls.update).toBe(1); + expect(ql.rows.find((x) => x.id === target.id)!.label) + .toBe(KNOWN_CAPABILITIES.find((c) => c.name === CURATED_NAMES[0])!.label); + }); + + /** + * ⚠️ LOAD-BEARING. Without this, an implementation that simply stopped writing + * would satisfy every count above while reconciling nothing at all. + */ + it('a curated row whose stored label/description drifted STILL gets its UPDATE', async () => { + const ql = capQl(); + await bootstrapSystemCapabilities(ql, []); + const row = ql.rows.find((r) => r.name === CURATED_NAMES[1])!; + row.label = 'Renamed'; + row.description = 'new text'; + ql.reset(); + const r = await bootstrapSystemCapabilities(ql, []); + expect(r.updated).toBe(1); + expect(ql.calls.update).toBe(1); + expect(row.label).toBe(KNOWN_CAPABILITIES.find((c) => c.name === CURATED_NAMES[1])!.label); + expect(row.description).toBe(KNOWN_CAPABILITIES.find((c) => c.name === CURATED_NAMES[1])!.description); + }); + + it('a genuinely absent curated name is still created — "absent" is not turned into "present"', async () => { + const ql = capQl(); + await bootstrapSystemCapabilities(ql, []); + const dropped = ql.rows.findIndex((r) => r.name === CURATED_NAMES[2]); + ql.rows.splice(dropped, 1); + ql.reset(); + const r = await bootstrapSystemCapabilities(ql, []); + expect(r.seeded).toBe(1); + expect(r.unchanged).toBe(CURATED_NAMES.length - 1); + expect(ql.roundTrips()).toBe(2); // one batched read + one insert + expect(ql.rows.find((x) => x.name === CURATED_NAMES[2])).toBeDefined(); + }); + + it('a read that CANNOT ANSWER is not the answer "none exist" — nothing is re-created', async () => { + 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 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. + const r = await bootstrapSystemCapabilities(broken, [], { logger: { warn: (m) => warns.push(m) } }); + expect(r.unreadable).toBe(KNOWN_CAPABILITIES.length); + expect(r.seeded).toBe(0); + expect(broken.calls.insert).toBe(0); // ⛔ no blind insert + expect(broken.rows).toHaveLength(KNOWN_CAPABILITIES.length); + 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); + }); +}); diff --git a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts index 23750ad006..0f72478f4b 100644 --- a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi } from 'vitest'; import { bootstrapSystemCapabilities, KNOWN_CAPABILITIES } from './bootstrap-system-capabilities.js'; +import { buildExistingByName } from './seed-name-lookup.js'; /** * Minimal in-memory ql for sys_capability seeding. @@ -29,6 +30,12 @@ import { bootstrapSystemCapabilities, KNOWN_CAPABILITIES } from './bootstrap-sys * `value == condition`; MongoDB matches null-or-missing. Strict `===`, which * this double used, matches NONE of them and would have made * `organization_id: null` unsatisfiable here while working in production. + * 4. **`$in` membership**, because the real engine has it (`security-plugin.ts` + * already reads `sys_permission_set` with `{ name: { $in: names } }`) and + * the curated half's batched existence read (#11451) uses it. A double that + * refused it would be pinning the double's limits, not the seeder's + * behaviour. Every OTHER value-level operator is still REFUSED rather than + * read as a column comparison. * 3. **`insert` enforces the declared unique key**, `(COALESCE(organization_id, * '__global__'), name)` — measured rejecting a second platform-bucket row * for a name, and admitting an organization's row for that same name. @@ -47,7 +54,17 @@ function makeQl() { const where = q?.where ?? {}; const matched = rows.filter((r) => // (2) `null` is IS NULL, not `=== null`. - Object.entries(where).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); return (v === null ? r[k] == null : r[k] === v); }), + Object.entries(where).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + // (4) `$in` is modelled; anything else object-shaped is refused. + if (v !== null && typeof v === 'object' && !Array.isArray(v)) { + const list = (v as { $in?: unknown }).$in; + if (!Array.isArray(list)) throw new Error(`fake driver: unsupported operator ${Object.keys(v as object).join(',')}`); + return list.includes(r[k]); + } + // (2) `null` is IS NULL, not `=== null`. + return v === null ? r[k] == null : r[k] === v; + }), ); if (q?.limit === undefined) return matched; // (1) The #4363 tie-breaker. BINARY collation, as SQLite compares ids. @@ -935,3 +952,112 @@ describe('[#8751] a platform-STAMPED row inside an organization is not the platf expect(warn).not.toHaveBeenCalled(); }); }); + +/** + * [#11451] WHY THE #8470 PREDICATE TRAVELS INSIDE THE BATCHED READ. + * + * The filing offered "two batched reads, the curated half post-filtering on + * `managed_by`/`organization_id` in memory" as the cheap option. It is not + * cheap; it is wrong, and this is the measurement rather than the argument. + * + * Both harms are visible on the SAME fixture, and both are properties of the + * shipped drivers that this double models deliberately (see `makeQl`'s header): + * `limit` orders by `id` ascending (#4363's pagination tie-breaker), and the + * unscoped batched page is capped at one row per requested name. + */ +describe('#11451 — the batched curated read must carry its predicate, not filter afterwards', () => { + const CURATED_NAMES = KNOWN_CAPABILITIES.map((c) => c.name); + const CURATED_LOOKUP = { managed_by: 'platform', organization_id: null }; + /** The name two organizations also hold — ADR-0066 D1 "admins EXTEND". */ + const SHARED = CURATED_NAMES[0]; + + /** Every curated name in the platform bucket, plus two organizations' rows. */ + function fixture() { + const ql = makeQl(); + // Organization ids sort BEFORE the platform's, so they win the tie-breaker. + ql.rows.push( + { id: 'aaa_org_jia', organization_id: 'org_jia', name: SHARED, label: 'Jia copy', description: 'jia', scope: 'org', managed_by: 'admin', active: true }, + { id: 'aab_org_yi', organization_id: 'org_yi', name: SHARED, label: 'Yi copy', description: 'yi', scope: 'org', managed_by: 'admin', active: true }, + ); + KNOWN_CAPABILITIES.forEach((c, i) => ql.rows.push({ + id: `cap_${i}`, name: c.name, label: c.label, description: c.description, + scope: c.scope, managed_by: 'platform', organization_id: null, active: true, + })); + return ql; + } + + it('POSITIVE CONTROL: with the predicate, every curated name resolves to the PLATFORM row', async () => { + const ql = fixture(); + const index = await buildExistingByName(ql, 'sys_capability', CURATED_NAMES, undefined, undefined, CURATED_LOOKUP); + for (const name of CURATED_NAMES) { + const found = await index.get(name); + expect(found.status).toBe('present'); + expect((found as { row: any }).row.managed_by).toBe('platform'); + expect((found as { row: any }).row.organization_id).toBeNull(); + } + // The predicate makes the result a provable singleton per name, so the page + // cap is exactly right and nothing falls off the end. + const shared = await index.get(SHARED); + expect((shared as { row: any }).row.id).toBe('cap_0'); + }); + + it('WITHOUT the predicate the same read returns an ORGANIZATION\'s row for the shared name', async () => { + const ql = fixture(); + const index = await buildExistingByName(ql, 'sys_capability', CURATED_NAMES); + const found = await index.get(SHARED); + expect(found.status).toBe('present'); + // …and it is NOT the platform's row. Reconciling this one is #8470 exactly: + // an organization's authored copy overwritten with the platform's, every + // boot, while the platform's own row is never the one addressed. + expect((found as { row: any }).row.id).toBe('aaa_org_jia'); + expect((found as { row: any }).row.managed_by).toBe('admin'); + }); + + it('WITHOUT the predicate the page also TRUNCATES, and a truncated page reads as "absent"', async () => { + const ql = fixture(); + // 10 rows carry the 8 requested names; the unscoped page is capped at 8, so + // the two highest-id rows fall off — and their names are platform rows that + // demonstrably exist. + const index = await buildExistingByName(ql, 'sys_capability', CURATED_NAMES); + const missing: string[] = []; + for (const name of CURATED_NAMES) { + if ((await index.get(name)).status === 'absent') missing.push(name); + } + expect(missing).toEqual([CURATED_NAMES[6], CURATED_NAMES[7]]); + // Their rows are right there. "Absent" would send the curated half to its + // insert branch, where the unique key refuses the write and the seeder + // reports a `blockedCurated` collision — every boot, on a healthy install. + for (const name of missing) { + expect(ql.rows.some((r) => r.name === name && r.managed_by === 'platform')).toBe(true); + } + + // …and the predicated read over the same fixture loses nobody. + const scoped = await buildExistingByName(ql, 'sys_capability', CURATED_NAMES, undefined, undefined, CURATED_LOOKUP); + for (const name of CURATED_NAMES) expect((await scoped.get(name)).status).toBe('present'); + }); + + it('the seeder itself is unharmed by the fixture that breaks the unpredicated read', async () => { + const ql = fixture(); + const warn = vi.fn(); + const out = await bootstrapSystemCapabilities(ql, [], { logger: { warn } }); + expect(out.seeded).toBe(0); + expect(out.blockedCurated).toBe(0); + expect(out.unchanged).toBe(KNOWN_CAPABILITIES.length); // nothing differed… + expect(out.updated).toBe(0); // …so nothing was written + expect(warn).not.toHaveBeenCalled(); + // The organizations' rows are untouched. + expect(ql.rows.find((r) => r.id === 'aaa_org_jia')).toMatchObject({ label: 'Jia copy' }); + }); + + it('an unreadable database leaves every curated definition ALONE, and says so once', async () => { + const ql = Object.assign(fixture(), { + async find() { throw new Error('fake driver: read unavailable'); }, + }); + const warn = vi.fn(); + const out = await bootstrapSystemCapabilities(ql, [], { logger: { warn } }); + expect(out.unreadable).toBe(KNOWN_CAPABILITIES.length); + expect(out.seeded).toBe(0); + expect(out.blockedCurated).toBe(0); // ⛔ never a collision that nobody saw + expect(warn.mock.calls.map((c) => c[0]).some((m) => String(m).includes('could not be read'))).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 dcc596c87b..382ee3d645 100644 --- a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts +++ b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts @@ -163,12 +163,57 @@ * platform's definition for this name is missing from sys_capability — instead * of firing on every authored row, which is the state #4632 declined to alarm * about and which remains counted in `skippedAuthored`. + * + * [#11451] ROUND TRIPS. The curated half's existence read is now ONE batched + * `$in`, and the reconcile is equality-gated for both halves. The design choice + * the filing asked to be made deliberately is recorded here so it is not + * re-litigated from the diff: + * + * - The predicate is carried INTO the batched read ({@link CURATED_LOOKUP}), + * not applied to its answer. Batching the wide question `{ name }` and + * filtering afterwards would read every organization's row for every curated + * name — a set bounded only by the number of organizations — against a page + * capped at one row per name, so it would truncate, and a truncated page + * reads as "absent", which inserts. It would also not avoid widening + * `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. + * + * ⚠️ NOTHING here claims a measured speedup. The hosted `bootstrap-curve.mjs` + * rig lives in `objectstack-ai/cloud` and its axes are permission sets / + * positions / objects, not this one. What is pinned in-repo is the round-trip + * COUNT and the identity of the row each leg reads and writes + * (`bootstrap-seed-round-trips.test.ts`). */ import { PLATFORM_CAPABILITIES, type PlatformCapability } from '@objectstack/spec/security'; +import { buildExistingByName } from './seed-name-lookup.js'; const SYSTEM_CTX = { isSystem: true }; +/** + * [#8470] The two facts that jointly define "the platform's OWN row", as ONE + * named constant so the batched read below and any future reader see the same + * predicate rather than two copies of it. + * + * ⛔ Do not relax this to `managed_by` alone or `organization_id` alone. The + * module header says why at length; the short version is that each half fails a + * different deployment (a platform-stamped row inside an organization; an + * admin's row in a single-organization deployment), and only the CONJUNCTION + * makes the result a provable singleton — the NULL-organization bucket admits + * one row per name under the declared key `(COALESCE(organization_id, …), name)`. + * That singleton property is also what makes the batched read below safe to cap + * at one row per name. + */ +const CURATED_LOOKUP = { managed_by: 'platform', organization_id: null } as const; + function genId(prefix: string): string { const rand = Math.random().toString(36).slice(2, 10); const ts = Date.now().toString(36); @@ -301,6 +346,26 @@ export interface CapabilitySeedResult { * silent. */ blockedCurated: number; + /** + * [#11451] Rows found ALREADY MATCHING the platform's definition, so no + * `UPDATE` was issued. Reported rather than folded into `updated`, for the + * same reason #10946 reports it on the sibling seeders: without it, "wrote + * nothing because nothing differed" and "wrote nothing because the writes + * stopped working" are the same observation. + */ + unchanged: number; + /** + * [#11451] Curated definitions left ENTIRELY alone because the existence read + * could not answer — not read as absent, and therefore never inserted. + * + * ⛔ 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 + * insert the unique index refused, for that one name; batched, one failure + * speaks for the whole set, so `unknown` is declined instead of guessed + * (#10946). Nothing is missing that was not already missing, and the next boot + * with a readable database seeds it. + */ + unreadable: number; /** Definitions considered this pass (curated + derived). */ total: number; } @@ -313,7 +378,7 @@ export async function bootstrapSystemCapabilities( if (!ql || typeof ql.find !== 'function' || typeof ql.insert !== 'function') { return { seeded: 0, updated: 0, skippedAuthored: 0, unseededDerived: 0, - platformStampedInOrg: 0, blockedCurated: 0, total: 0, + platformStampedInOrg: 0, blockedCurated: 0, unchanged: 0, unreadable: 0, total: 0, }; } @@ -344,17 +409,75 @@ export async function bootstrapSystemCapabilities( let unseededDerived = 0; let platformStampedInOrg = 0; let blockedCurated = 0; + let unchanged = 0; + let unreadable = 0; + + // [#11451] ONE batched existence read for the CURATED half, hoisted out of the + // loop below — the shape #10946 established and #11096 carried to the seeder + // next door. Each curated definition used to cost its own sequential + // `SELECT … LIMIT 1`: invisible on a local file database, one separate awaited + // HTTP request on the remote libsql/Turso database every hosted environment + // runs, competing for the same request budget as the rest of boot. + // + // ⚠️ The predicate travels WITH the `$in`; it is NOT applied afterwards in + // memory. The curated question is the platform's own organization-less row + // ({@link CURATED_LOOKUP}, #8470), and post-#8461 that is strictly narrower + // than "a row with this name". Asking the wide question and filtering the + // answer would read every organization's row for every curated name — a set + // bounded only by the number of organizations — against a page capped at one + // row per name, so the page would TRUNCATE, and a truncated page reads as + // `absent`, which inserts. Keeping the predicate in the query keeps the result + // a provable singleton per name, which is the property `sys-capability.object + // .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. + const curatedExisting = await buildExistingByName( + ql, + 'sys_capability', + KNOWN_CAPABILITIES.map((c) => c.name), + options.logger, + undefined, + CURATED_LOOKUP, + ); + 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. - const lookup = isDerived - ? { name: def.name } - : { name: def.name, managed_by: 'platform', organization_id: null }; - const existing = await tryFind(ql, 'sys_capability', lookup, 1); - const row = existing[0]; + 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; + } if (row?.id) { // [#5876] Reconcile display fields only where THIS pass owns the copy. // @@ -494,11 +617,25 @@ export async function bootstrapSystemCapabilities( // sys_capability), so it is seed-once: written on insert, never // refreshed (#2909 T3). A curated scope change in a new platform version // needs a data migration — recorded in the ADR-0094 addendum. - if (await tryUpdate(ql, 'sys_capability', { id: row.id, label: def.label, description: def.description })) { + // + // [#11451] …and only where they actually DIFFER. This write used to fire + // on every boot for every row the pass owns, storing bytes already there — + // one more sequential request per definition on a remote database. The + // gate sits AFTER the derived-ownership guard above, so it removes write + // round trips from BOTH halves, including the one whose read stays + // per-item. + // + // Compared on exactly the two columns this pass owns and nothing else: + // `scope` is seed-once (#2909 T3) and the provenance columns are never + // rewritten here, so a row already matching on `label` and `description` + // is a row this pass has nothing to say about. + if (row.label === def.label && row.description === def.description) { + unchanged += 1; + } else if (await tryUpdate(ql, 'sys_capability', { id: row.id, label: def.label, description: def.description })) { updated += 1; } } else { - const created = await tryInsert(ql, 'sys_capability', { + const payload = { id: genId('cap'), name: def.name, label: def.label, @@ -506,9 +643,18 @@ export async function bootstrapSystemCapabilities( scope: def.scope, managed_by: 'platform', active: true, - }); - if (created) seeded += 1; - else if (!isDerived) { + }; + const created = await tryInsert(ql, 'sys_capability', payload); + if (created) { + seeded += 1; + // [#10946] Recording the row is what keeps hoisting the read out of the + // loop behaviour-preserving: the per-item read saw rows this same loop + // had just inserted, and a snapshot taken before it cannot. `byName` is + // 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); + } 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 // scoped lookup did not match. Report it: a curated capability missing @@ -569,12 +715,24 @@ export async function bootstrapSystemCapabilities( } } } + if (unreadable > 0) { + // [#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 + // reconciled; the next boot with a readable database does both. + options.logger?.warn?.( + '[security] curated capabilities left untouched — their sys_capability rows could not be read', + { unreadable, total: KNOWN_CAPABILITIES.length }, + ); + } options.logger?.info?.('[security] system capabilities seeded into sys_capability (ADR-0066 D1)', { seeded, updated, skippedAuthored, unseededDerived, platformStampedInOrg, blockedCurated, - total: byName.size, + unchanged, unreadable, total: byName.size, }); return { seeded, updated, skippedAuthored, unseededDerived, platformStampedInOrg, blockedCurated, - total: byName.size, + unchanged, unreadable, total: byName.size, }; } diff --git a/packages/plugins/plugin-security/src/seed-name-lookup.ts b/packages/plugins/plugin-security/src/seed-name-lookup.ts index 98ddaa5320..ce86471f2a 100644 --- a/packages/plugins/plugin-security/src/seed-name-lookup.ts +++ b/packages/plugins/plugin-security/src/seed-name-lookup.ts @@ -158,13 +158,18 @@ async function readNamePage( object: string, names: string[], organizationId?: string, + equals?: Readonly>, ): Promise { let rows: any; try { rows = await ql.find( object, { - where: { name: { $in: names } }, + // [#11451] `...(equals ?? {})` spreads NOTHING when no predicate was + // given, so a caller that passes none emits the exact key set it + // emitted before — not the same keys plus `undefined`-valued ones, + // which `toEqual` would have quietly accepted. + where: { name: { $in: names }, ...(equals ?? {}) }, // [#10103] `names.length` was exactly right while one row existed per // name. Once the catalog is per organization the driver returns this // organization's rows AND any organization-less ones, so that cap @@ -190,7 +195,12 @@ async function readNamePage( * `remember` is a deliberate no-op: this oracle re-reads the database on every * call, so it already sees rows the loop inserted a moment ago. */ -function perItemIndex(ql: any, object: string, organizationId?: string): ExistingByNameIndex { +function perItemIndex( + ql: any, + object: string, + organizationId?: string, + equals?: Readonly>, +): ExistingByNameIndex { return { async get(name: string): Promise { let rows: any; @@ -198,9 +208,14 @@ function perItemIndex(ql: any, object: string, organizationId?: string): Existin // Limit 5, not 1, when scoped: a single row would be whichever the // driver ordered first, and this read must be able to tell this // organization's row from an organization-less leftover. + // + // [#11451] The predicate rides the DEGRADATION read too. A fallback + // that dropped it would ask a WIDER question than the batched read it + // is standing in for — and for the caller that needs one, wider is not + // "slower but the same": it is a different row. rows = await ql.find( object, - { where: { name }, limit: organizationId ? 5 : 1 }, + { where: { name, ...(equals ?? {}) }, limit: organizationId ? 5 : 1 }, { context: lookupCtx(organizationId) }, ); } catch { @@ -250,6 +265,28 @@ export async function buildExistingByName( * installation-wide question, which is what a `single`-posture pass wants. */ organizationId?: string, + /** + * [#11451] An extra EQUALITY predicate ANDed onto the `$in`, for a caller + * whose existence question is narrower than "a row with this name". + * + * `bootstrapSystemCapabilities`' curated half asks for the platform's OWN + * organization-less row (`managed_by: 'platform'` + `organization_id: null`, + * #8470), not the first row that happens to share the name. Post-#8461 those + * are different questions: `sys_capability.name` is unique per ORGANIZATION, + * so one name can have a row per organization plus the platform's. + * + * ⚠️ PRECONDITION, and it is the caller's to discharge: the predicate must + * keep the result a SINGLETON per name. `readNamePage` caps an unscoped page + * at `names.length`, so a question that can return more than one row per name + * truncates — and a truncated page reads as `absent`, which INSERTS. The + * curated predicate discharges this by construction: the declared unique key + * is `(COALESCE(organization_id, '__global__'), name)` (ADR-0120 D3), so the + * NULL-organization bucket admits at most one row per name — "exactly the + * bucket this key part keeps a singleton", as `sys-capability.object.ts` puts + * it. Narrowing can only SHRINK a page, so passing a predicate never makes + * truncation likelier than the unpredicated read it replaces. + */ + equals?: Readonly>, ): Promise { // Every row the page carried for a name, so the organization split can be // judged per name rather than by arrival order. @@ -279,7 +316,7 @@ export async function buildExistingByName( if (wanted.length === 0) return fromIndex; for (let i = 0; i < wanted.length; i += NAME_CHUNK_SIZE) { - const page = await readNamePage(ql, object, wanted.slice(i, i + NAME_CHUNK_SIZE), organizationId); + const page = await readNamePage(ql, object, wanted.slice(i, i + NAME_CHUNK_SIZE), organizationId, equals); if (page === null) { // ⛔ NOT "none of them exist" — see the module header. Fall back to the // per-item read so behaviour is exactly what it was before the hoist. @@ -287,7 +324,7 @@ export async function buildExistingByName( '[security] batched seed existence read failed — falling back to one read per item', { object, names: wanted.length, ...(organizationId ? { organization: organizationId } : {}) }, ); - return perItemIndex(ql, object, organizationId); + return perItemIndex(ql, object, organizationId, equals); } for (const row of page) { const name = row?.name;