Skip to content

Commit 5337ef1

Browse files
os-warrenclaude
andauthored
perf(plugin-security): batch the identity boot seeds' existence read and skip no-op writes (#11116)
* perf(plugin-security): batch the identity boot seeds' existence read and skip no-op writes Every declared permission set and every declared position cost 4 sequential DB round trips on every kernel boot, 2 of them an UPDATE that fired when nothing had changed. Hoist ONE $in existence read out of each loop and write only when the stored row actually differs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * test(plugin-security): pin the counting double to ObjectQL.update's dispatch predicate check:engine-double-contract and check:where-matcher both reddened on the new counting double: its update() did not route through assertEngineUpdateDispatch, and its WHERE matcher read a combinator as a field name. Fixed in the double — the shrink-only baseline is untouched; only the pinned (tightening) ledger grew. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * test(plugin-security): type the seed fixture so the TEST_DEBT ratchet stays at 11 check:type-check-debt --re-measure went 11 -> 12: the upgrade fixture widens a grant literal, which the inferred type rejects (TS2353). Fixed by declaring the fixture's type — the ledger entry is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 266654d commit 5337ef1

10 files changed

Lines changed: 934 additions & 32 deletions
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
---
2+
"@objectstack/plugin-security": minor
3+
---
4+
5+
Batch the identity boot seeds' existence read and stop re-writing rows that
6+
already match the declaration (#10946).
7+
8+
Every permission set and every position an environment declared cost **exactly
9+
4 sequential database round trips on every kernel boot** — measured on a real
10+
per-environment kernel build with every `@libsql/client` call counted: slope
11+
4.0000, R² = 1.000000 on both axes, with a per-statement histogram naming the
12+
four legs (2 × existence `SELECT`, 1 × `UPDATE`, 1 × `SELECT`). Two of the four
13+
were an `UPDATE` that fired even when nothing had changed. On a local file
14+
database the loop is invisible; on a remote libsql/Turso database — every hosted
15+
environment — each leg is its own sequential HTTP request. Schema sync had
16+
already been batched (`TursoDriver.supports.batchSchemaSync`), which is why
17+
objects, views and artifact seeds add 0.00 round trips each on the same rig;
18+
identity content was the one content axis still paying per item.
19+
20+
Both loops now hoist **one** `{ name: { $in: [...] } }` existence read out of the
21+
loop — the declaration is known in full before the loop starts — and write only
22+
when the stored row actually differs from what would be written. A steady-state
23+
rebuild of both loops is now O(1) round trips: measured in-repo against a
24+
call-counting ObjectQL double, a rebuild of 1, 5, 20 and 40 declared items costs
25+
1 round trip in every case, for permission sets and positions alike.
26+
27+
Three things the change is careful **not** to become:
28+
29+
- **Drift still reconciles.** The skip is on equality, never on "we have seen
30+
this name": a row whose stored value differs — a package version bump, a
31+
hand-edit, a partially applied write — still gets its `UPDATE`. An
32+
implementation that skipped all writes would show the same round-trip curve
33+
and silently stop reconciling, so the round-trip pins are paired one-for-one
34+
with drift pins over the same fixtures.
35+
- **A read that could not answer is not the answer "none exist."** A batched
36+
read fails for the whole set at once, so swallowing its failure into `[]`
37+
would make every boot conclude nothing is seeded and re-create everything. The
38+
seam is judged on whether the driver returned a result set, never on whether
39+
the array came back empty; a failed batched read degrades to the per-item read
40+
(loudly warned), and a name whose record cannot be read at all is declined
41+
rather than inserted. That last step is deliberately stricter than the code it
42+
replaces, which turned a failed read into an insert attempt and leaned on the
43+
`name` unique index to refuse it.
44+
- **A converged publish is still a successful publish.** `PermissionSeedOutcome`
45+
gains `unchanged` (rows that already matched) and `unreadable` (names declined
46+
because their record could not be read). The ADR-0086 P2 publish materializer
47+
asks "did the record end up matching the published body", which was
48+
accidentally identical to "was a write issued" only because the seeder always
49+
wrote; it now reads `seeded + updated + unchanged`, so every case that reported
50+
a materialization before still reports one. A re-publish of a byte-identical
51+
body reports `inserted: 0, updated: 0` instead of `updated: 1` — the one
52+
reporting difference, and the truthful reading.
53+
54+
`bootstrapDeclaredPositions` likewise returns `unchanged` and `unreadable`
55+
alongside `seeded`/`updated`.

packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,19 @@ function makeQl(declared: any[] = []) {
1515
async find(object: string, q: any) {
1616
if (object !== 'sys_permission_set') return [];
1717
const where = q?.where ?? {};
18-
return rows.filter((r) => Object.entries(where).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); return r[k] === v; }));
18+
// Membership is modelled because the real engine supports it and the
19+
// #10946 boot seeders now hoist ONE `$in` existence read out of their
20+
// loop. A double that silently answered `[]` to `$in` would report
21+
// "nothing is seeded" and make every re-seed look like a first boot.
22+
return rows.filter((r) => Object.entries(where).every(([k, v]) => {
23+
if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`);
24+
if (v && typeof v === 'object' && !Array.isArray(v)) {
25+
const inList = (v as any).$in;
26+
if (Array.isArray(inList)) return inList.includes(r[k]);
27+
throw new Error(`fake driver: unsupported operator ${Object.keys(v).join(',')}`);
28+
}
29+
return r[k] === v;
30+
}));
1931
},
2032
async insert(object: string, data: any) {
2133
if (object !== 'sys_permission_set') return null;

packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts

Lines changed: 98 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,17 +36,41 @@
3636
*/
3737

3838
import {
39+
SYSTEM_CTX,
3940
genId,
4041
permissionSetRowFields,
41-
tryFind,
42+
recordDiffersFromBody,
4243
tryInsert,
4344
tryUpdate,
4445
type PermissionSeedOutcome,
4546
type ProjectionLogger,
4647
} from './permission-set-projection.js';
48+
import {
49+
buildExistingByName,
50+
type ExistingByNameIndex,
51+
type ExistingLookupResult,
52+
} from './seed-name-lookup.js';
4753

4854
export type { PermissionSeedOutcome } from './permission-set-projection.js';
4955

56+
/**
57+
* The per-name existence read used when no batched oracle was supplied (the
58+
* ADR-0086 P2 publish materializer, which upserts exactly one set). Reports the
59+
* same three outcomes the batched oracle does — a failed read must not read as
60+
* "absent" on this path either.
61+
*/
62+
async function defaultLookup(ql: any, name: string): Promise<ExistingLookupResult> {
63+
let rows: any;
64+
try {
65+
rows = await ql.find('sys_permission_set', { where: { name }, limit: 1 }, { context: SYSTEM_CTX });
66+
} catch {
67+
return { status: 'unknown' };
68+
}
69+
const list = Array.isArray(rows) ? rows : Array.isArray(rows?.records) ? rows.records : null;
70+
if (list === null) return { status: 'unknown' };
71+
return list[0] ? { status: 'present', row: list[0] } : { status: 'absent' };
72+
}
73+
5074
interface SeedOptions {
5175
logger?: ProjectionLogger;
5276
}
@@ -96,8 +120,17 @@ export async function upsertPackagePermissionSet(
96120
ps: any,
97121
packageId: string | null | undefined,
98122
logger?: SeedOptions['logger'],
123+
opts?: {
124+
/**
125+
* Existence oracle to consult instead of this function's own per-name
126+
* `SELECT` (#10946). The boot loop passes ONE batched read covering every
127+
* declared name; the publish materializer, which upserts a single set,
128+
* passes nothing and keeps the per-name read.
129+
*/
130+
existingByName?: ExistingByNameIndex;
131+
},
99132
): Promise<PermissionSeedOutcome> {
100-
const out: PermissionSeedOutcome = { seeded: 0, updated: 0, skippedEnvAuthored: 0, skippedForeign: 0 };
133+
const out: PermissionSeedOutcome = { seeded: 0, updated: 0, unchanged: 0, unreadable: 0, skippedEnvAuthored: 0, skippedForeign: 0 };
101134
if (!ps?.name) return out;
102135
// A `managed_by:'package'` row without a `package_id` would make uninstall
103136
// undefined again — the exact ambiguity ADR-0086 D3 exists to remove — so a
@@ -107,25 +140,59 @@ export async function upsertPackagePermissionSet(
107140
return out;
108141
}
109142

110-
const existing = (await tryFind(ql, 'sys_permission_set', { name: ps.name }, 1))[0];
143+
// ⛔ Three outcomes, not two (#10946 / #3807). `unknown` — the read FAILED —
144+
// is not "no such row": inserting on it would re-create a set that already
145+
// exists, and on a batched read one failure speaks for every declared name at
146+
// once. Declining is the answer; the caller's warn reports it.
147+
const lookup = opts?.existingByName
148+
? await opts.existingByName.get(String(ps.name))
149+
: await defaultLookup(ql, String(ps.name));
150+
if (lookup.status === 'unknown') {
151+
out.unreadable += 1;
152+
return out;
153+
}
154+
const existing = lookup.status === 'present' ? lookup.row : undefined;
111155
if (!existing?.id) {
112-
const created = await tryInsert(ql, 'sys_permission_set', {
156+
const row = {
113157
id: genId('ps'),
114158
name: ps.name,
115159
...permissionSetRowFields(ps),
116160
active: true,
117161
package_id: packageId,
118162
managed_by: 'package',
119-
});
120-
if (created) out.seeded += 1;
163+
};
164+
const created = await tryInsert(ql, 'sys_permission_set', row);
165+
if (created) {
166+
out.seeded += 1;
167+
// A batched oracle is a snapshot taken before the loop — tell it about
168+
// the row we just made, so a name declared twice in one batch still
169+
// reaches the collision branch below instead of a second insert.
170+
opts?.existingByName?.remember(String(ps.name), row);
171+
}
121172
return out;
122173
}
123174

124175
if (existing.managed_by === 'package') {
125176
if (existing.package_id === packageId) {
126177
// Our own row — re-seed so the record always reflects the shipped/published
127178
// declaration (idempotent; covers version bumps without bookkeeping).
128-
if (await tryUpdate(ql, 'sys_permission_set', { id: existing.id, ...permissionSetRowFields(ps) })) {
179+
//
180+
// [#10946] "Idempotent" was implemented as "write the same columns every
181+
// time", which on a remote libsql/Turso database is two HTTP round trips
182+
// per set on every boot to change nothing. `recordDiffersFromBody` is the
183+
// SAME comparison the ADR-0094 boot reconciler already trusts to decide
184+
// whether a record drifted, over exactly the columns
185+
// `permissionSetRowFields` writes — so a row it calls equal is a row this
186+
// UPDATE could not have changed.
187+
//
188+
// ⚠️ The skip is on EQUALITY, never on "we have seen this name". A row
189+
// whose stored value differs — a version bump, a hand-edit, a partially
190+
// applied write — still gets its UPDATE, because dropping that leg would
191+
// turn the loop into a no-op that reconciles nothing while showing a
192+
// beautiful round-trip curve.
193+
if (!recordDiffersFromBody(existing, ps)) {
194+
out.unchanged += 1;
195+
} else if (await tryUpdate(ql, 'sys_permission_set', { id: existing.id, ...permissionSetRowFields(ps) })) {
129196
out.updated += 1;
130197
}
131198
} else {
@@ -151,7 +218,7 @@ export async function bootstrapDeclaredPermissions(
151218
metadataService: any,
152219
options: SeedOptions = {},
153220
): Promise<PermissionSeedOutcome> {
154-
const out: PermissionSeedOutcome = { seeded: 0, updated: 0, skippedEnvAuthored: 0, skippedForeign: 0 };
221+
const out: PermissionSeedOutcome = { seeded: 0, updated: 0, unchanged: 0, unreadable: 0, skippedEnvAuthored: 0, skippedForeign: 0 };
155222
if (!ql || typeof ql.find !== 'function' || typeof ql.insert !== 'function') return out;
156223

157224
let sets: any[] = readDeclared(ql, 'permission');
@@ -163,18 +230,40 @@ export async function bootstrapDeclaredPermissions(
163230
}
164231
if (!Array.isArray(sets) || sets.length === 0) return out;
165232

233+
// [#10946] ONE existence read for the whole declaration, before the loop —
234+
// the set of names is known in full here. See `seed-name-lookup.ts` for why
235+
// a read that cannot ANSWER must not be read as "none of them exist".
236+
const existingByName = await buildExistingByName(
237+
ql,
238+
'sys_permission_set',
239+
sets.map((ps) => ps?.name),
240+
options.logger,
241+
);
242+
166243
for (const ps of sets) {
167244
if (!ps?.name) continue;
168245
// Registry provenance first (ADR-0010 `_packageId`), author-declared
169246
// spec `packageId` (ADR-0086 D3) as fallback.
170247
const packageId: string | undefined = ps._packageId ?? ps.packageId ?? undefined;
171-
const r = await upsertPackagePermissionSet(ql, ps, packageId, options.logger);
248+
const r = await upsertPackagePermissionSet(ql, ps, packageId, options.logger, { existingByName });
172249
out.seeded += r.seeded;
173250
out.updated += r.updated;
251+
out.unchanged += r.unchanged;
252+
out.unreadable += r.unreadable;
174253
out.skippedEnvAuthored += r.skippedEnvAuthored;
175254
out.skippedForeign += r.skippedForeign;
176255
}
177256

257+
if (out.unreadable > 0) {
258+
// Said once, with the count: these sets were neither seeded nor reconciled
259+
// because the record could not be READ. Silence here would read exactly
260+
// like "everything was already in order".
261+
options.logger?.warn?.(
262+
'[security] declared permission sets left untouched — their records could not be read',
263+
{ unreadable: out.unreadable, total: sets.length },
264+
);
265+
}
266+
178267
options.logger?.info?.('[security] declared permission sets seeded into sys_permission_set (ADR-0086 D5)', {
179268
...out, total: sets.length,
180269
});

packages/plugins/plugin-security/src/bootstrap-declared-positions.test.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,19 @@ function makeQl(declared: any[] = []) {
2626
async find(object: string, q: any) {
2727
if (object !== 'sys_position') return [];
2828
const where = q?.where ?? {};
29-
return rows.filter((r) => Object.entries(where).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); return r[k] === v; }));
29+
// Membership is modelled because the real engine supports it and the
30+
// #10946 boot seeders now hoist ONE `$in` existence read out of their
31+
// loop. A double that silently answered `[]` to `$in` would report
32+
// "nothing is seeded" and make every re-seed look like a first boot.
33+
return rows.filter((r) => Object.entries(where).every(([k, v]) => {
34+
if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`);
35+
if (v && typeof v === 'object' && !Array.isArray(v)) {
36+
const inList = (v as any).$in;
37+
if (Array.isArray(inList)) return inList.includes(r[k]);
38+
throw new Error(`fake driver: unsupported operator ${Object.keys(v).join(',')}`);
39+
}
40+
return r[k] === v;
41+
}));
3042
},
3143
async insert(object: string, data: any) {
3244
if (object !== 'sys_position') return null;

0 commit comments

Comments
 (0)