From 27bbb2ffa8f28850d29d46f71f6a85847dfb11f4 Mon Sep 17 00:00:00 2001 From: ObjectStack Agent Date: Wed, 2 Sep 2026 19:25:12 +0000 Subject: [PATCH 1/7] perf(plugin-security): claim seed ownership with one predicate write per unowned shape Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../src/claim-seed-ownership.test.ts | 262 ++++++++++++++++-- .../src/claim-seed-ownership.ts | 143 +++++++--- 2 files changed, 332 insertions(+), 73 deletions(-) diff --git a/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts b/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts index 098623f0d4..b963d05807 100644 --- a/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts +++ b/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts @@ -7,27 +7,88 @@ import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; const SYSTEM = 'usr_system'; const ADMIN = 'usr_admin_human'; -function makeQL(schemas: any[], rowsByObject: Record) { - const updates: { object: string; data: any }[] = []; +/** One recorded call to `ql.update` — the shape, not just the payload. */ +interface RecordedWrite { + object: string; + data: any; + where: any; + multi: boolean; + /** Ids this write actually re-owned, in fixture order. */ + matched: string[]; +} + +/** `where` as the two `UNOWNED_PREDICATES` spell it: one equality per key. */ +function rowMatches(row: any, where: Record): boolean { + return Object.entries(where).every(([k, v]) => (row?.[k] ?? null) === (v ?? null)); +} + +/** + * A fake ObjectQL that honours the engine's own update dispatch and the + * predicate-write return contract. + * + * `assertEngineUpdateDispatch` is the producer's rule, imported rather than + * re-derived (`check:engine-double-contract`), and the fake refuses anything + * it does not verdict `multi` — a double looser than the producer would let a + * regression back to single-id writes pass as green. + * + * `updateMany` is contracted to resolve the AFFECTED ROW COUNT (#4639), so this + * returns a number, never a record. + */ +function makeQL( + schemas: any[], + rowsByObject: Record, + hooks: { onUpdate?: (object: string, where: any) => void } = {}, +) { + const writes: RecordedWrite[] = []; const ql: any = { registry: { getAllObjects: () => schemas }, - find: vi.fn(async (object: string, query: any) => { - const all = rowsByObject[object] ?? []; - const w = query?.where ?? {}; - if ('owner_id' in w) { - return all.filter((r) => (r.owner_id ?? null) === (w.owner_id ?? null)); - } - return all; + find: vi.fn(async () => { + throw new Error('claimSeedOwnership must not scan: the predicate write IS the scan'); }), - update: vi.fn(async (object: string, data: any) => { - assertEngineUpdateDispatch(data, undefined); - updates.push({ object, data }); - const row = (rowsByObject[object] ?? []).find((r) => r.id === data.id); - if (row) row.owner_id = data.owner_id; - return row; + update: vi.fn(async (object: string, data: any, options: any) => { + const dispatch = assertEngineUpdateDispatch(data, options); + if (dispatch.kind !== 'multi') { + throw new Error(`expected a predicate write, engine dispatch said '${dispatch.kind}'`); + } + hooks.onUpdate?.(object, options?.where); + const where = options?.where ?? {}; + const matched = (rowsByObject[object] ?? []).filter((r) => rowMatches(r, where)); + for (const row of matched) Object.assign(row, data); + writes.push({ + object, + data, + where, + multi: options?.multi === true, + matched: matched.map((r) => r.id), + }); + return matched.length; }), }; - return { ql, updates }; + return { ql, writes }; +} + +/** + * The id set the PRE-#14530 implementation would have claimed, spelled out as + * the loop spelled it: two narrow scans capped at `limit: 10_000`, deduped, + * one single-id write each. + * + * Deliberately a re-statement of the OLD rule rather than a call into the new + * one — an equivalence pin that shares the implementation under test proves + * nothing. + */ +function legacyClaimedIds(rows: any[]): string[] { + const seen = new Set(); + const ids: string[] = []; + for (const where of [{ owner_id: null }, { owner_id: SYSTEM }] as Record[]) { + const scanned = rows.filter((r) => rowMatches(r, where)).slice(0, 10_000); + for (const r of scanned) { + if (r?.id && !seen.has(r.id)) { + seen.add(r.id); + ids.push(r.id); + } + } + } + return ids; } describe('claimSeedOwnership', () => { @@ -38,10 +99,10 @@ describe('claimSeedOwnership', () => { it('no-ops when the target is empty or the system user', async () => { const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; - const { ql, updates } = makeQL(schemas, { crm_lead: [{ id: 'l1', owner_id: null }] }); + const { ql, writes } = makeQL(schemas, { crm_lead: [{ id: 'l1', owner_id: null }] }); expect(await claimSeedOwnership(ql, '')).toEqual([]); expect(await claimSeedOwnership(ql, SYSTEM)).toEqual([]); - expect(updates).toHaveLength(0); + expect(writes).toHaveLength(0); }); it('skips managedBy and sys_* tables', async () => { @@ -49,12 +110,12 @@ describe('claimSeedOwnership', () => { { name: 'sys_user', managedBy: 'better-auth', fields: [{ name: 'owner_id' }] }, { name: 'sys_widget', fields: [{ name: 'owner_id' }] }, ]; - const { ql, updates } = makeQL(schemas, { + const { ql, writes } = makeQL(schemas, { sys_user: [{ id: 'u1', owner_id: null }], sys_widget: [{ id: 'w1', owner_id: null }], }); expect(await claimSeedOwnership(ql, ADMIN)).toEqual([]); - expect(updates).toHaveLength(0); + expect(writes).toHaveLength(0); }); it('skips external (federated) objects even when they expose owner_id', async () => { @@ -68,19 +129,19 @@ describe('claimSeedOwnership', () => { fields: [{ name: 'owner_id' }], }, ]; - const { ql, updates } = makeQL(schemas, { + const { ql, writes } = makeQL(schemas, { showcase_ext_customer: [{ id: 'c1', owner_id: null }], }); expect(await claimSeedOwnership(ql, ADMIN)).toEqual([]); - expect(ql.find).not.toHaveBeenCalled(); - expect(updates).toHaveLength(0); + expect(ql.update).not.toHaveBeenCalled(); + expect(writes).toHaveLength(0); }); it('skips objects without an owner_id field', async () => { const schemas = [{ name: 'crm_pricebook', fields: [{ name: 'name' }] }]; - const { ql, updates } = makeQL(schemas, { crm_pricebook: [{ id: 'p1' }] }); + const { ql, writes } = makeQL(schemas, { crm_pricebook: [{ id: 'p1' }] }); expect(await claimSeedOwnership(ql, ADMIN)).toEqual([]); - expect(updates).toHaveLength(0); + expect(writes).toHaveLength(0); }); it('re-owns NULL and usr_system rows to the admin, leaving human-owned rows untouched', async () => { @@ -90,12 +151,13 @@ describe('claimSeedOwnership', () => { { id: 'l2', owner_id: SYSTEM }, // claimed (seed identity) { id: 'l3', owner_id: 'usr_someone' },// untouched (already human-owned) ]; - const { ql, updates } = makeQL(schemas, { crm_lead: rows }); + const { ql, writes } = makeQL(schemas, { crm_lead: rows }); const result = await claimSeedOwnership(ql, ADMIN); expect(result).toEqual([{ object: 'crm_lead', count: 2 }]); - expect(updates.map((u) => u.data.id).sort()).toEqual(['l1', 'l2']); - expect(updates.every((u) => u.data.owner_id === ADMIN)).toBe(true); + expect(writes.flatMap((w) => w.matched).sort()).toEqual(['l1', 'l2']); + expect(rows.find((r) => r.id === 'l1')!.owner_id).toBe(ADMIN); + expect(rows.find((r) => r.id === 'l2')!.owner_id).toBe(ADMIN); expect(rows.find((r) => r.id === 'l3')!.owner_id).toBe('usr_someone'); }); @@ -106,4 +168,148 @@ describe('claimSeedOwnership', () => { const second = await claimSeedOwnership(ql, ADMIN); expect(second).toEqual([]); }); + + // ── [#14530] the predicate-write shape ──────────────────────────────────── + + it('issues ONE predicate write per unowned shape — never one write per row', async () => { + // The whole point of the card: N rows used to cost N single-id writes, so + // the batch existed only in this caller's loop and nothing downstream could + // see it. The write COUNT is the pin, and it must not scale with N. + const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; + const rows = Array.from({ length: 500 }, (_, i) => ({ + id: `l${i}`, + owner_id: i % 2 === 0 ? null : SYSTEM, + })); + const { ql, writes } = makeQL(schemas, { crm_lead: rows }); + + const result = await claimSeedOwnership(ql, ADMIN); + + expect(result).toEqual([{ object: 'crm_lead', count: 500 }]); + expect(writes).toHaveLength(2); + expect(writes.every((w) => w.multi)).toBe(true); + expect(writes.map((w) => w.where)).toEqual([{ owner_id: null }, { owner_id: SYSTEM }]); + // Every write carries the payload only — no `id`, which is what routes the + // engine down `updateMany` instead of the single-id door. + expect(writes.every((w) => Object.keys(w.data).join() === 'owner_id')).toBe(true); + expect(rows.every((r) => r.owner_id === ADMIN)).toBe(true); + }); + + it('claims exactly the id set the pre-#14530 single-id loop would have claimed', async () => { + // The equivalence pin. The predicate write must not widen or narrow the + // matched set by one row: same fixture, same answer, computed two ways. + const fixture = [ + { id: 'a', owner_id: null }, // author left it unset + { id: 'b', owner_id: SYSTEM }, // seed identity + { id: 'c', owner_id: 'usr_someone' }, // a human already owns it + { id: 'd' }, // column absent entirely + { id: 'e', owner_id: '' }, // empty string is NOT null + { id: 'f', owner_id: undefined }, // present-but-undefined + { id: 'g', owner_id: 'usr_system_admin' },// prefix collision, not the seed id + ]; + const expected = legacyClaimedIds(fixture.map((r) => ({ ...r }))); + expect(expected).toEqual(['a', 'd', 'f', 'b']); + + const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; + const rows = fixture.map((r) => ({ ...r })); + const { ql, writes } = makeQL(schemas, { crm_lead: rows }); + + const result = await claimSeedOwnership(ql, ADMIN); + + expect(writes.flatMap((w) => w.matched).sort()).toEqual([...expected].sort()); + expect(result).toEqual([{ object: 'crm_lead', count: expected.length }]); + // …and nothing outside that set moved. + for (const row of rows) { + const wasClaimed = expected.includes(row.id); + expect(row.owner_id).toBe(wasClaimed ? ADMIN : fixture.find((f) => f.id === row.id)!.owner_id); + } + }); + + it('the two predicates stay disjoint — no row is counted twice', async () => { + // The NULL write lands `adminUserId`, which can never be `usr_system` (the + // function refuses that target outright), so the second predicate cannot + // re-match a row the first one just claimed. If it ever could, the reported + // count would exceed the number of rows that exist. + const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; + const rows = [ + { id: 'l1', owner_id: null }, + { id: 'l2', owner_id: null }, + { id: 'l3', owner_id: SYSTEM }, + ]; + const { ql, writes } = makeQL(schemas, { crm_lead: rows }); + + expect(await claimSeedOwnership(ql, ADMIN)).toEqual([{ object: 'crm_lead', count: 3 }]); + expect(writes[0].matched).toEqual(['l1', 'l2']); + expect(writes[1].matched).toEqual(['l3']); + }); + + it('reports the affected-row count the write resolved, not a length it counted itself', async () => { + const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; + const rows = [{ id: 'l1', owner_id: null }, { id: 'l2', owner_id: SYSTEM }]; + const ql: any = { + registry: { getAllObjects: () => schemas }, + update: vi.fn(async (_o: string, data: any, options: any) => { + assertEngineUpdateDispatch(data, options); + return options.where.owner_id === null ? 7 : 11; + }), + }; + expect(await claimSeedOwnership(ql, ADMIN)).toEqual([{ object: 'crm_lead', count: 18 }]); + expect(rows).toHaveLength(2); // fixture untouched — the count came from the write + }); + + it('says "unknown" rather than 0 when a driver resolves something that is not a count', async () => { + // `eventMatchedCount`'s discipline, one caller over: a non-count result + // means the rows very likely WERE written, so reporting none of them would + // be a false statement rather than a conservative one. + const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; + const warn = vi.fn(); + const ql: any = { + registry: { getAllObjects: () => schemas }, + update: vi.fn(async (_o: string, data: any, options: any) => { + assertEngineUpdateDispatch(data, options); + return { id: 'l1' }; // a record — this driver did not meet the contract + }), + }; + expect(await claimSeedOwnership(ql, ADMIN, { logger: { info: vi.fn(), warn } })).toEqual([]); + expect(warn).toHaveBeenCalledTimes(2); + expect(warn.mock.calls[0][0]).toContain('could not read an affected-row count'); + }); + + it('a refused predicate write costs that predicate only — never the object or the run', async () => { + // The engine refuses a predicate write whole above MAX_BULK_PER_ROW_HOOK_ROWS + // (per-row hook budget, D6). That is one predicate on one object; the other + // predicate, and every later object, must still land. + const schemas = [ + { name: 'crm_lead', fields: [{ name: 'owner_id' }] }, + { name: 'crm_case', fields: [{ name: 'owner_id' }] }, + ]; + const warn = vi.fn(); + const { ql, writes } = makeQL( + schemas, + { + crm_lead: [{ id: 'l1', owner_id: null }, { id: 'l2', owner_id: SYSTEM }], + crm_case: [{ id: 'c1', owner_id: null }], + }, + { + onUpdate: (object, where) => { + if (object === 'crm_lead' && where?.owner_id === null) { + throw Object.assign( + new Error("Refusing the bulk write on 'crm_lead': it matches 10001 rows"), + { code: 'ERR_BULK_PER_ROW_HOOK_LIMIT' }, + ); + } + }, + }, + ); + + const result = await claimSeedOwnership(ql, ADMIN, { logger: { info: vi.fn(), warn } }); + + expect(result).toEqual([ + { object: 'crm_lead', count: 1 }, // the usr_system predicate still landed + { object: 'crm_case', count: 1 }, + ]); + expect(writes.map((w) => w.object)).toEqual(['crm_lead', 'crm_case', 'crm_case']); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('those rows stay unowned'); + expect(warn.mock.calls[0][1].error).toContain('Refusing the bulk write'); + }); }); diff --git a/packages/plugins/plugin-security/src/claim-seed-ownership.ts b/packages/plugins/plugin-security/src/claim-seed-ownership.ts index 2ceff135b3..0581ffdd65 100644 --- a/packages/plugins/plugin-security/src/claim-seed-ownership.ts +++ b/packages/plugins/plugin-security/src/claim-seed-ownership.ts @@ -25,6 +25,43 @@ * Idempotent: only NULL / `usr_system`-owned rows are touched, so once a real * admin owns them a re-run is a no-op. `managedBy` and `sys_*` tables are * skipped (their ownership, if any, is platform-controlled). + * + * ## [#14530] One PREDICATE write per unowned shape, never a write per row + * + * This used to scan each object twice at `limit: 10_000` and then issue one + * **single-id** `update` per matched id — up to 20 000 writes for one object. + * Every one of those is a full engine write (middleware chain, validation, + * hook dispatch, driver round trip), and the batch existed only in this loop, + * where nothing downstream could see it: plugin-sharing's `rule-hooks.ts` + * already routes a write whose row set exceeds `RULE_RECOMPUTE_ROW_CAP` into + * one set-based revoke plus one queued `evaluateAllRulesForObject`, but that + * branch reads ONE write's row set, and each of these writes legitimately + * carried a single row. Batching in the caller is what lets the machinery + * already built for this shape do its job — no change to `plugin-sharing`. + * + * The two scans are gone with the loop. The predicates they resolved are the + * predicates the writes now carry, one write each, so the matched set is + * unchanged row for row: `owner_id IS NULL`, then `owner_id = usr_system`. + * They stay two narrow writes rather than one `OR`/`IN` predicate for the + * reason the scans were two — driver portability — and they remain disjoint in + * this order, because the first write leaves `adminUserId` (never `usr_system`, + * refused above) where the NULLs were. + * + * The count reported per object is the affected-row count the predicate write + * resolves (#4639), not a length this function counted for itself. + * + * ### The bound moved, and it did not get smaller + * + * A predicate write carries no `limit`, so nothing here truncates at 10 000 any + * more. What bounds it now is the engine's own ceiling: a predicate write on an + * object carrying `beforeUpdate`/`afterUpdate` hooks — which every object does, + * objectql's own audit-stamp builtin is registered on `'*'` — is REFUSED whole + * above `MAX_BULK_PER_ROW_HOOK_ROWS` (10 000), because those hooks are + * contracted to fire per matched row. So the reachable population per predicate + * per run is the same 10 000 the scan limit allowed; what changed is that + * exceeding it is now LOUD (the engine's refusal names the count, the ceiling + * and both routes out, and this function logs it per object) instead of a + * silent partial claim of the first 10 000 rows. */ import type { ServiceObject } from '@objectstack/spec/data'; @@ -39,6 +76,18 @@ interface ClaimOwnershipOptions { const SYSTEM_CTX = { isSystem: true }; +/** + * "Unowned", as two driver-portable predicates rather than one `OR`/`IN`. + * + * Order is load-bearing: the NULL write lands `adminUserId` — which cannot be + * `usr_system` (refused at the top of {@link claimSeedOwnership}) — so the two + * matched sets stay disjoint and their counts sum without double-counting a row. + */ +const UNOWNED_PREDICATES: readonly Record[] = [ + { owner_id: null }, + { owner_id: SystemUserId.SYSTEM }, +]; + function hasOwnerField(schema: ServiceObject): boolean { const fields: any = (schema as any)?.fields; if (!fields) return false; @@ -48,6 +97,24 @@ function hasOwnerField(schema: ServiceObject): boolean { return Object.prototype.hasOwnProperty.call(fields, 'owner_id'); } +/** + * The affected-row count a predicate write resolved, or `undefined` when the + * result is not one. + * + * `IDataDriver.updateMany` is contracted to resolve the affected row count and + * `ObjectQL.update` passes it through for a `multi: true` write (#4639). A + * result that is not a non-negative integer has not met that contract, so this + * says "unknown" rather than inventing a `0` — the engine's own reader of the + * same value (`eventMatchedCount`, which declines to publish a bulk event on + * exactly this input) makes the same call, for the same reason: the rows very + * likely WERE written, and reporting none of them is a false statement, not a + * conservative one. + */ +function affectedRowCount(value: unknown): number | undefined { + if (typeof value !== 'number' || !Number.isInteger(value) || value < 0) return undefined; + return value; +} + /** * Re-own every orphan seed row (owner_id NULL or usr_system) to `adminUserId`. * @@ -57,7 +124,8 @@ function hasOwnerField(schema: ServiceObject): boolean { * (c) are not `external` (federated remote-table bindings — read-only, DDL * forbidden, and their `owner_id` is not ours to reassign), * (d) declare an `owner_id` field, - * and updates the unowned rows as `isSystem`. Returns a per-object summary. + * and re-owns the unowned rows as `isSystem` with one predicate write per + * {@link UNOWNED_PREDICATES} entry. Returns a per-object summary. */ export async function claimSeedOwnership( ql: any, @@ -66,9 +134,9 @@ export async function claimSeedOwnership( ): Promise<{ object: string; count: number }[]> { const logger = options.logger; if (!adminUserId || adminUserId === SystemUserId.SYSTEM) return []; - if (!ql || typeof ql.update !== 'function' || typeof ql.find !== 'function') { - return []; - } + // Only `update` is required now that the scans are gone: this function asks + // the engine for exactly one capability, so the guard names exactly that one. + if (!ql || typeof ql.update !== 'function') return []; const registry = (ql as any).registry; if (!registry || typeof registry.getAllObjects !== 'function') { logger?.warn?.('[security] claimSeedOwnership: registry unavailable'); @@ -91,53 +159,38 @@ export async function claimSeedOwnership( if ((schema as any).external) continue; if (!hasOwnerField(schema)) continue; - try { - // Unowned = owner_id NULL (author left it unset) OR usr_system (seed - // identity). Two narrow scans keep the where-clauses driver-portable - // instead of relying on an OR/IN predicate. - const seen = new Set(); - const ids: string[] = []; - for (const where of [{ owner_id: null }, { owner_id: SystemUserId.SYSTEM }]) { - const rows = await ql.find( + let updated = 0; + for (const where of UNOWNED_PREDICATES) { + try { + const affected = await ql.update( schema.name, - { where, limit: 10_000, fields: ['id'] }, - { context: SYSTEM_CTX }, + { owner_id: adminUserId }, + { where, multi: true, context: SYSTEM_CTX }, ); - const list: any[] = Array.isArray(rows) - ? rows - : Array.isArray(rows?.records) - ? rows.records - : []; - for (const r of list) { - if (r?.id && !seen.has(r.id)) { - seen.add(r.id); - ids.push(r.id); - } - } - } - if (ids.length === 0) continue; - - let updated = 0; - for (const id of ids) { - try { - await ql.update( - schema.name, - { id, owner_id: adminUserId }, - { context: SYSTEM_CTX }, + const count = affectedRowCount(affected); + if (count === undefined) { + logger?.warn?.( + `[security] claimSeedOwnership could not read an affected-row count for ${schema.name} ` + + '— the rows were re-owned but this run cannot say how many', + { object: schema.name, where, result: typeof affected }, ); - updated += 1; - } catch (e) { - logger?.warn?.(`[security] claimSeedOwnership failed for ${schema.name}:${id}`, { - error: (e as Error).message, - }); + continue; } + updated += count; + } catch (e) { + // Best-effort per predicate, exactly as the per-id loop was: one + // predicate that cannot land must not cost the object its other one, + // nor any later object. The rows stay unowned and the next run — boot, + // the bootstrap replay, or `meta resync` — claims them, because the + // predicate is still true of them. + logger?.warn?.( + `[security] claimSeedOwnership failed for ${schema.name}; those rows stay unowned ` + + 'and the next run will claim them', + { object: schema.name, where, error: (e as Error).message }, + ); } - if (updated > 0) results.push({ object: schema.name, count: updated }); - } catch (e) { - logger?.warn?.(`[security] claimSeedOwnership scan failed for ${schema.name}`, { - error: (e as Error).message, - }); } + if (updated > 0) results.push({ object: schema.name, count: updated }); } if (results.length > 0) { From 0d8395f97e9a521168169c537c53252a34142a64 Mon Sep 17 00:00:00 2001 From: ObjectStack Agent Date: Wed, 2 Sep 2026 19:53:35 +0000 Subject: [PATCH 2/7] chore(changeset): claim-seed-ownership predicate write Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../claim-seed-ownership-predicate-write.md | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 .changeset/claim-seed-ownership-predicate-write.md diff --git a/.changeset/claim-seed-ownership-predicate-write.md b/.changeset/claim-seed-ownership-predicate-write.md new file mode 100644 index 0000000000..86a4abe2e9 --- /dev/null +++ b/.changeset/claim-seed-ownership-predicate-write.md @@ -0,0 +1,42 @@ +--- +'@objectstack/plugin-security': patch +--- + +perf(plugin-security): claim seed ownership with one predicate write per unowned shape (#14530) + +`claimSeedOwnership` — the pass that hands seeded business records to the first +platform admin — scanned every `owner_id`-declaring object twice at +`limit: 10_000` and then issued **one single-id `update` per matched id**: up to +20 000 full engine writes for one object, each paying the whole middleware, +validation and hook chain. It now issues **one predicate write per unowned +shape** instead: `owner_id IS NULL`, then `owner_id = usr_system`. Two writes +per object, whatever the row count. The scans are gone with the loop — the +predicates they resolved are the predicates the writes now carry, so the matched +set is unchanged row for row, and the count reported per object is the +affected-row count the write itself resolves. + +Measured on a real ObjectQL engine (in-memory driver, one sharing-rule-covered +object, shared box): 2 000 rows 2 122 ms → 171 ms; 5 000 rows 10 658 ms → 448 ms, +with engine `update` calls falling from N to 2. + +The second half is what the batch buys downstream. plugin-sharing's `rule-hooks` +already routes a write whose row set exceeds `RULE_RECOMPUTE_ROW_CAP` (1 000) +into one set-based revoke plus one queued `evaluateAllRulesForObject`, but that +branch reads **one write's** row set, and every write in the old loop +legitimately carried a single row — so the batch existed only in the caller, +where nothing downstream could see it. Batching here is what lets machinery +already built for this shape do its job; `plugin-sharing` is unchanged. + +**One behaviour change, at the edge.** A predicate write carries no `limit`, so +nothing truncates at 10 000 any more; what bounds it now is the engine's own +per-row hook ceiling (`MAX_BULK_PER_ROW_HOOK_ROWS`, 10 000), which refuses a +predicate write whole rather than firing per-row hooks past it. The reachable +population per predicate per run is therefore the same 10 000 the scan limit +allowed. What changed is the boundary behaviour: an object with more than 10 000 +rows under one predicate used to have its first 10 000 claimed **silently**, +leaving a half-claimed table nothing re-runs automatically; it now claims none +of them and logs the engine's refusal, which names the count, the ceiling and +both routes out. Loud and recoverable in place of silent and undetectable. + +`patch`: no declared surface moves, no export changes, and the only observable +delta at the boundary is a failure mode becoming loud. From 9f17ab8a02d4ccb0f6d0f17e2615630db6701adf Mon Sep 17 00:00:00 2001 From: ObjectStack Agent Date: Wed, 2 Sep 2026 20:14:35 +0000 Subject: [PATCH 3/7] test(plugin-security): refuse combinators in the claim-seed WHERE double `check:where-matcher` flagged the new fixture matcher as silently wrong on a combinator query. `claimSeedOwnership` issues none, so the double refuses rather than implementing them. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../src/claim-seed-ownership.test.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts b/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts index b963d05807..bc4059b4f1 100644 --- a/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts +++ b/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts @@ -17,9 +17,23 @@ interface RecordedWrite { matched: string[]; } -/** `where` as the two `UNOWNED_PREDICATES` spell it: one equality per key. */ +/** + * `where` as the two `UNOWNED_PREDICATES` spell it: one equality per key. + * + * A combinator (`$and`/`$or`/`$not`) is REFUSED rather than read as a field + * name (`check:where-matcher`): this double implements plain equality only, and + * a double that answers a query it does not implement is silently wrong on the + * exact shape the pin exists to judge. `claimSeedOwnership` issues no + * combinator, so the throw is unreachable today and turns the suite red the + * moment one arrives. + */ function rowMatches(row: any, where: Record): boolean { - return Object.entries(where).every(([k, v]) => (row?.[k] ?? null) === (v ?? null)); + return Object.entries(where).every(([k, v]) => { + if (k.startsWith('$')) { + throw new Error(`this double implements plain equality only; it cannot answer '${k}'`); + } + return (row?.[k] ?? null) === (v ?? null); + }); } /** From 39126dc020b17dda153478274ee34db9f241e17e Mon Sep 17 00:00:00 2001 From: ObjectStack Agent Date: Wed, 2 Sep 2026 21:05:59 +0000 Subject: [PATCH 4/7] fix(plugin-security): page the seed-ownership predicate write so objects over the per-row hook ceiling are still claimed An unpaged predicate write is refused whole above MAX_BULK_PER_ROW_HOOK_ROWS (ADR-0058 D6), so a 21k-row object claimed nothing where the pre-#14530 loop claimed 10k. `owner_id` is a record-access field, so that is a permission outcome, not an observability one. The unit of work is now a page. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../claim-seed-ownership-predicate-write.md | 46 ++-- .../src/claim-seed-ownership.test.ts | 219 ++++++++++++++---- .../src/claim-seed-ownership.ts | 207 ++++++++++++----- 3 files changed, 357 insertions(+), 115 deletions(-) diff --git a/.changeset/claim-seed-ownership-predicate-write.md b/.changeset/claim-seed-ownership-predicate-write.md index 86a4abe2e9..25d97893d4 100644 --- a/.changeset/claim-seed-ownership-predicate-write.md +++ b/.changeset/claim-seed-ownership-predicate-write.md @@ -2,22 +2,22 @@ '@objectstack/plugin-security': patch --- -perf(plugin-security): claim seed ownership with one predicate write per unowned shape (#14530) +perf(plugin-security): claim seed ownership with paged predicate writes (#14530) `claimSeedOwnership` — the pass that hands seeded business records to the first platform admin — scanned every `owner_id`-declaring object twice at `limit: 10_000` and then issued **one single-id `update` per matched id**: up to 20 000 full engine writes for one object, each paying the whole middleware, -validation and hook chain. It now issues **one predicate write per unowned -shape** instead: `owner_id IS NULL`, then `owner_id = usr_system`. Two writes -per object, whatever the row count. The scans are gone with the loop — the -predicates they resolved are the predicates the writes now carry, so the matched -set is unchanged row for row, and the count reported per object is the -affected-row count the write itself resolves. +validation and hook chain. The unit of work is now a **page**, not a row: read +at most 5 000 unowned ids, re-own them with one predicate write, repeat until +the predicate is exhausted — for each of the two unowned shapes (`owner_id IS +NULL`, then `owner_id = usr_system`). The matched set is unchanged row for row, +and the count reported per object is the sum of the affected-row counts the page +writes resolve rather than a length this pass counted for itself. Measured on a real ObjectQL engine (in-memory driver, one sharing-rule-covered -object, shared box): 2 000 rows 2 122 ms → 171 ms; 5 000 rows 10 658 ms → 448 ms, -with engine `update` calls falling from N to 2. +object, shared box): 2 000 rows 2 122 ms → 208 ms; 5 000 rows 10 658 ms → 528 ms, +with engine `update` calls falling from N to one per page. The second half is what the batch buys downstream. plugin-sharing's `rule-hooks` already routes a write whose row set exceeds `RULE_RECOMPUTE_ROW_CAP` (1 000) @@ -25,18 +25,20 @@ into one set-based revoke plus one queued `evaluateAllRulesForObject`, but that branch reads **one write's** row set, and every write in the old loop legitimately carried a single row — so the batch existed only in the caller, where nothing downstream could see it. Batching here is what lets machinery -already built for this shape do its job; `plugin-sharing` is unchanged. +already built for this shape do its job; `plugin-sharing` is unchanged, and the +page size is deliberately far above that cap so a full page is still seen as a +batch. -**One behaviour change, at the edge.** A predicate write carries no `limit`, so -nothing truncates at 10 000 any more; what bounds it now is the engine's own -per-row hook ceiling (`MAX_BULK_PER_ROW_HOOK_ROWS`, 10 000), which refuses a -predicate write whole rather than firing per-row hooks past it. The reachable -population per predicate per run is therefore the same 10 000 the scan limit -allowed. What changed is the boundary behaviour: an object with more than 10 000 -rows under one predicate used to have its first 10 000 claimed **silently**, -leaving a half-claimed table nothing re-runs automatically; it now claims none -of them and logs the engine's refusal, which names the count, the ceiling and -both routes out. Loud and recoverable in place of silent and undetectable. +**Why paged rather than one write per object.** A predicate write carries no +`limit`, so "one write per object" is the obvious shape — and the engine refuses +it whole above `MAX_BULK_PER_ROW_HOOK_ROWS` (10 000), because `beforeUpdate` / +`afterUpdate` hooks are contracted to fire per matched row on a predicate write +(ADR-0058 D6) and every object carries such hooks in practice. Measured: 21 000 +unowned rows re-owned **nothing**, where the old loop re-owned 10 000 of them. +This pass decides `owner_id`, a record-access field, so an unclaimed object is a +permission outcome and not an observability detail. Paging keeps the batch and +the coverage: the same 21 000-row case now claims every one of them, and the +page size is derived from that ceiling rather than chosen, so it moves with it. -`patch`: no declared surface moves, no export changes, and the only observable -delta at the boundary is a failure mode becoming loud. +`patch`: no declared surface moves, no export changes, and the reachable +population strictly grows. diff --git a/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts b/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts index bc4059b4f1..2ccde1a0ee 100644 --- a/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts +++ b/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts @@ -3,6 +3,7 @@ import { describe, it, expect, vi } from 'vitest'; import { claimSeedOwnership } from './claim-seed-ownership.js'; import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { MAX_BULK_PER_ROW_HOOK_ROWS } from '@objectstack/spec/data'; const SYSTEM = 'usr_system'; const ADMIN = 'usr_admin_human'; @@ -17,56 +18,102 @@ interface RecordedWrite { matched: string[]; } +/** One recorded call to `ql.find` — the page request the writer issued. */ +interface RecordedRead { + object: string; + where: any; + limit: number; + returned: number; +} + /** - * `where` as the two `UNOWNED_PREDICATES` spell it: one equality per key. + * `where` as `claimSeedOwnership` spells it: field equality for the unowned + * predicates, and `{ id: { $in: [...] } }` for a page write. * - * A combinator (`$and`/`$or`/`$not`) is REFUSED rather than read as a field - * name (`check:where-matcher`): this double implements plain equality only, and - * a double that answers a query it does not implement is silently wrong on the - * exact shape the pin exists to judge. `claimSeedOwnership` issues no - * combinator, so the throw is unreachable today and turns the suite red the - * moment one arrives. + * Anything else is REFUSED rather than answered (`check:where-matcher`): a + * combinator (`$and`/`$or`/`$not`) read as a field name, or an unimplemented + * value operator read as a literal, is silently wrong on exactly the shape a pin + * exists to judge. This double implements two forms and says so out loud. */ function rowMatches(row: any, where: Record): boolean { return Object.entries(where).every(([k, v]) => { if (k.startsWith('$')) { - throw new Error(`this double implements plain equality only; it cannot answer '${k}'`); + throw new Error(`this double implements field predicates only; it cannot answer '${k}'`); + } + const actual = row?.[k] ?? null; + if (v !== null && typeof v === 'object' && !Array.isArray(v)) { + const ops = Object.keys(v as Record); + if (ops.length === 1 && ops[0] === '$in') { + // A Set, not `.some`: the over-ceiling fixture pairs a 21 000-row table + // with 5 000-member pages, and a linear scan per row makes the pin cost + // seconds for no extra coverage. + let members: Set | undefined = (v as any).__set; + if (!members) { + members = new Set(((v as any).$in as unknown[]).map((m) => m ?? null)); + Object.defineProperty(v, '__set', { value: members, enumerable: false }); + } + return members.has(actual); + } + throw new Error( + `this double implements equality and $in only; it cannot answer ${JSON.stringify(ops)}`, + ); } - return (row?.[k] ?? null) === (v ?? null); + return actual === (v ?? null); }); } /** - * A fake ObjectQL that honours the engine's own update dispatch and the - * predicate-write return contract. + * A fake ObjectQL that honours the engine's own update dispatch, the + * predicate-write return contract, and `limit` on a read. * * `assertEngineUpdateDispatch` is the producer's rule, imported rather than - * re-derived (`check:engine-double-contract`), and the fake refuses anything - * it does not verdict `multi` — a double looser than the producer would let a + * re-derived (`check:engine-double-contract`), and the fake refuses anything it + * does not verdict `multi` — a double looser than the producer would let a * regression back to single-id writes pass as green. * * `updateMany` is contracted to resolve the AFFECTED ROW COUNT (#4639), so this * returns a number, never a record. + * + * `ceiling` models ADR-0058 D6: a predicate write matching more rows than the + * engine's per-row hook budget is refused WHOLE, nothing written. */ function makeQL( schemas: any[], rowsByObject: Record, - hooks: { onUpdate?: (object: string, where: any) => void } = {}, + opts: { ceiling?: number; onUpdate?: (object: string, where: any) => void } = {}, ) { const writes: RecordedWrite[] = []; + const reads: RecordedRead[] = []; + const ceiling = opts.ceiling ?? MAX_BULK_PER_ROW_HOOK_ROWS; const ql: any = { registry: { getAllObjects: () => schemas }, - find: vi.fn(async () => { - throw new Error('claimSeedOwnership must not scan: the predicate write IS the scan'); + find: vi.fn(async (object: string, query: any) => { + const all = rowsByObject[object] ?? []; + const hits = all.filter((r) => rowMatches(r, query?.where ?? {})); + const page = typeof query?.limit === 'number' ? hits.slice(0, query.limit) : hits; + reads.push({ object, where: query?.where, limit: query?.limit, returned: page.length }); + return page.map((r) => ({ id: r.id })); }), update: vi.fn(async (object: string, data: any, options: any) => { const dispatch = assertEngineUpdateDispatch(data, options); if (dispatch.kind !== 'multi') { throw new Error(`expected a predicate write, engine dispatch said '${dispatch.kind}'`); } - hooks.onUpdate?.(object, options?.where); + opts.onUpdate?.(object, options?.where); const where = options?.where ?? {}; const matched = (rowsByObject[object] ?? []).filter((r) => rowMatches(r, where)); + if (matched.length > ceiling) { + // ADR-0058 D6, verbatim in shape: total refusal, nothing written. + throw Object.assign( + new Error( + `Refusing the bulk write on '${object}': it matches ${matched.length} rows, and ` + + `'beforeUpdate' hooks are contracted to fire PER ROW on a predicate write ` + + `(ADR-0058, bulk-write addendum), which is over the ${ceiling}-row ceiling for one ` + + 'write. Nothing was written.', + ), + { code: 'ERR_BULK_PER_ROW_HOOK_LIMIT' }, + ); + } for (const row of matched) Object.assign(row, data); writes.push({ object, @@ -78,17 +125,19 @@ function makeQL( return matched.length; }), }; - return { ql, writes }; + return { ql, writes, reads }; } /** * The id set the PRE-#14530 implementation would have claimed, spelled out as - * the loop spelled it: two narrow scans capped at `limit: 10_000`, deduped, - * one single-id write each. + * the loop spelled it: two narrow scans capped at `limit: 10_000`, deduped, one + * single-id write each. * * Deliberately a re-statement of the OLD rule rather than a call into the new * one — an equivalence pin that shares the implementation under test proves - * nothing. + * nothing. The `slice` is the old scan cap, and it is why the over-ceiling test + * below compares against `unownedIds` instead: past 10 000 the old rule itself + * was lossy, and the new one must beat it, not match it. */ function legacyClaimedIds(rows: any[]): string[] { const seen = new Set(); @@ -105,6 +154,13 @@ function legacyClaimedIds(rows: any[]): string[] { return ids; } +/** Every row the two unowned predicates match, with NO cap of any kind. */ +function unownedIds(rows: any[]): string[] { + return rows + .filter((r) => (r.owner_id ?? null) === null || r.owner_id === SYSTEM) + .map((r) => r.id); +} + describe('claimSeedOwnership', () => { it('returns [] when registry is unavailable', async () => { const ql: any = { find: vi.fn(), update: vi.fn() }; @@ -147,6 +203,7 @@ describe('claimSeedOwnership', () => { showcase_ext_customer: [{ id: 'c1', owner_id: null }], }); expect(await claimSeedOwnership(ql, ADMIN)).toEqual([]); + expect(ql.find).not.toHaveBeenCalled(); expect(ql.update).not.toHaveBeenCalled(); expect(writes).toHaveLength(0); }); @@ -201,9 +258,8 @@ describe('claimSeedOwnership', () => { expect(result).toEqual([{ object: 'crm_lead', count: 500 }]); expect(writes).toHaveLength(2); expect(writes.every((w) => w.multi)).toBe(true); - expect(writes.map((w) => w.where)).toEqual([{ owner_id: null }, { owner_id: SYSTEM }]); - // Every write carries the payload only — no `id`, which is what routes the - // engine down `updateMany` instead of the single-id door. + // Every write carries the payload only — no `id` in `data`, which is what + // routes the engine down `updateMany` instead of the single-id door. expect(writes.every((w) => Object.keys(w.data).join() === 'owner_id')).toBe(true); expect(rows.every((r) => r.owner_id === ADMIN)).toBe(true); }); @@ -258,40 +314,41 @@ describe('claimSeedOwnership', () => { it('reports the affected-row count the write resolved, not a length it counted itself', async () => { const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; - const rows = [{ id: 'l1', owner_id: null }, { id: 'l2', owner_id: SYSTEM }]; + let call = 0; const ql: any = { registry: { getAllObjects: () => schemas }, + find: vi.fn(async () => (call < 2 ? [{ id: `l${call}` }] : [])), update: vi.fn(async (_o: string, data: any, options: any) => { assertEngineUpdateDispatch(data, options); - return options.where.owner_id === null ? 7 : 11; + call += 1; + return call === 1 ? 7 : 11; }), }; expect(await claimSeedOwnership(ql, ADMIN)).toEqual([{ object: 'crm_lead', count: 18 }]); - expect(rows).toHaveLength(2); // fixture untouched — the count came from the write }); it('says "unknown" rather than 0 when a driver resolves something that is not a count', async () => { // `eventMatchedCount`'s discipline, one caller over: a non-count result // means the rows very likely WERE written, so reporting none of them would - // be a false statement rather than a conservative one. + // be a false statement rather than a conservative one — and paging stops, + // because the predicate's state is now unknown. const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; const warn = vi.fn(); const ql: any = { registry: { getAllObjects: () => schemas }, + find: vi.fn(async () => [{ id: 'l1' }]), update: vi.fn(async (_o: string, data: any, options: any) => { assertEngineUpdateDispatch(data, options); return { id: 'l1' }; // a record — this driver did not meet the contract }), }; expect(await claimSeedOwnership(ql, ADMIN, { logger: { info: vi.fn(), warn } })).toEqual([]); - expect(warn).toHaveBeenCalledTimes(2); + expect(warn).toHaveBeenCalledTimes(2); // once per predicate expect(warn.mock.calls[0][0]).toContain('could not read an affected-row count'); + expect(ql.update).toHaveBeenCalledTimes(2); // stopped paging, did not spin }); it('a refused predicate write costs that predicate only — never the object or the run', async () => { - // The engine refuses a predicate write whole above MAX_BULK_PER_ROW_HOOK_ROWS - // (per-row hook budget, D6). That is one predicate on one object; the other - // predicate, and every later object, must still land. const schemas = [ { name: 'crm_lead', fields: [{ name: 'owner_id' }] }, { name: 'crm_case', fields: [{ name: 'owner_id' }] }, @@ -305,11 +362,9 @@ describe('claimSeedOwnership', () => { }, { onUpdate: (object, where) => { - if (object === 'crm_lead' && where?.owner_id === null) { - throw Object.assign( - new Error("Refusing the bulk write on 'crm_lead': it matches 10001 rows"), - { code: 'ERR_BULK_PER_ROW_HOOK_LIMIT' }, - ); + if (object === 'crm_lead' && where?.id) { + const rows = (where.id as any).$in as string[]; + if (rows.includes('l1')) throw new Error('driver refused this page'); } }, }, @@ -321,9 +376,95 @@ describe('claimSeedOwnership', () => { { object: 'crm_lead', count: 1 }, // the usr_system predicate still landed { object: 'crm_case', count: 1 }, ]); - expect(writes.map((w) => w.object)).toEqual(['crm_lead', 'crm_case', 'crm_case']); + // crm_lead's NULL page was refused; its usr_system page landed. crm_case's + // NULL page landed and its usr_system predicate matched nothing, so it + // issued no write at all — an empty page is not a write. + expect(writes.map((w) => w.object)).toEqual(['crm_lead', 'crm_case']); expect(warn).toHaveBeenCalledTimes(1); expect(warn.mock.calls[0][0]).toContain('those rows stay unowned'); - expect(warn.mock.calls[0][1].error).toContain('Refusing the bulk write'); + expect(warn.mock.calls[0][1].error).toContain('driver refused this page'); + }); + + // ── [#14530 patch 1] paging: coverage past the engine's per-row ceiling ─── + + it('claims EVERY unowned row past MAX_BULK_PER_ROW_HOOK_ROWS, where one unpaged write is refused whole', async () => { + // ADR-0058 D6: a predicate write over the per-row hook ceiling is refused + // WHOLE, nothing written — and every object carries such hooks in practice + // (objectql's audit stamp is registered on '*'). Unpaged, this object was + // measured claiming ZERO of 21 000 rows while the pre-#14530 loop claimed + // 10 000 of them: a permission-outcome regression, since `owner_id` is a + // record-access field. Paged, the answer is all 21 000. + const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; + // 21 000 rows: 3 000 already human-owned, 3 000 on the seed identity, and + // 15 000 with no owner at all — so the NULL predicate ALONE is over the + // ceiling, which is the shape one unpaged write cannot survive. + const seed = () => Array.from({ length: 21_000 }, (_, i) => ({ + id: `l${i}`, + owner_id: i % 7 === 0 ? 'usr_someone' : i % 7 === 1 ? SYSTEM : null, + })); + const rows = seed(); + const everyUnowned = unownedIds(seed()); + expect(everyUnowned).toHaveLength(18_000); + expect(rows.filter((r) => r.owner_id === null)).toHaveLength(15_000); + expect(15_000).toBeGreaterThan(MAX_BULK_PER_ROW_HOOK_ROWS); + + const { ql, writes, reads } = makeQL(schemas, { crm_lead: rows }); + const result = await claimSeedOwnership(ql, ADMIN); + + // Not one row short. + expect(writes.flatMap((w) => w.matched).sort()).toEqual([...everyUnowned].sort()); + expect(result).toEqual([{ object: 'crm_lead', count: everyUnowned.length }]); + expect(rows.filter((r) => r.owner_id === ADMIN)).toHaveLength(everyUnowned.length); + // …and it strictly beats the pre-#14530 rule, which capped its own scans. + // The pre-#14530 rule capped each of its own scans at 10 000, so it could + // only ever have reached 13 000 of these 18 000 rows. + expect(legacyClaimedIds(seed())).toHaveLength(13_000); + expect(everyUnowned.length).toBeGreaterThan(legacyClaimedIds(seed()).length); + + // Still batched, not per row: writes are O(pages), and every page is sized + // so the engine's ceiling can never refuse it. + expect(writes.length).toBeLessThan(20); + expect(writes.every((w) => w.matched.length <= MAX_BULK_PER_ROW_HOOK_ROWS)).toBe(true); + // A page is big enough that plugin-sharing's 1000-row recompute cap still + // sees these writes as batches rather than recomputing them row by row. + expect(reads.every((r) => r.limit > 1_000 && r.limit <= MAX_BULK_PER_ROW_HOOK_ROWS)).toBe(true); + }); + + it('count is the SUM over pages, not the last page', async () => { + // With paging the reported count is an accumulation, and the easy bug is to + // let the final page's return value overwrite it. 12 000 unowned rows do not + // fit in one page, so a count equal to any single page's size is the bug. + const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; + const rows = Array.from({ length: 12_000 }, (_, i) => ({ id: `l${i}`, owner_id: null })); + const { ql, writes } = makeQL(schemas, { crm_lead: rows }); + + const result = await claimSeedOwnership(ql, ADMIN); + + expect(writes.length).toBeGreaterThan(1); + const perPage = writes.map((w) => w.matched.length); + expect(result).toEqual([ + { object: 'crm_lead', count: perPage.reduce((s, n) => s + n, 0) }, + ]); + expect(result[0].count).toBe(12_000); + expect(result[0].count).not.toBe(perPage[perPage.length - 1]); + }); + + it('stops rather than spinning when a page matches rows but re-owns none', async () => { + // A write-scoping middleware can narrow a page to nothing. Re-reading the + // same predicate would then return the same page forever, so paging stops + // and says so — "we could not claim these" is not "there was nothing here". + const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; + const warn = vi.fn(); + const ql: any = { + registry: { getAllObjects: () => schemas }, + find: vi.fn(async () => [{ id: 'l1' }, { id: 'l2' }]), + update: vi.fn(async (_o: string, data: any, options: any) => { + assertEngineUpdateDispatch(data, options); + return 0; // matched by the read, moved by nothing + }), + }; + expect(await claimSeedOwnership(ql, ADMIN, { logger: { info: vi.fn(), warn } })).toEqual([]); + expect(ql.update).toHaveBeenCalledTimes(2); // one attempt per predicate, no spin + expect(warn.mock.calls[0][0]).toContain('but re-owned none of them'); }); }); diff --git a/packages/plugins/plugin-security/src/claim-seed-ownership.ts b/packages/plugins/plugin-security/src/claim-seed-ownership.ts index 0581ffdd65..94592f6bac 100644 --- a/packages/plugins/plugin-security/src/claim-seed-ownership.ts +++ b/packages/plugins/plugin-security/src/claim-seed-ownership.ts @@ -26,45 +26,54 @@ * admin owns them a re-run is a no-op. `managedBy` and `sys_*` tables are * skipped (their ownership, if any, is platform-controlled). * - * ## [#14530] One PREDICATE write per unowned shape, never a write per row + * ## [#14530] PAGED predicate writes, never a write per row * * This used to scan each object twice at `limit: 10_000` and then issue one * **single-id** `update` per matched id — up to 20 000 writes for one object. - * Every one of those is a full engine write (middleware chain, validation, - * hook dispatch, driver round trip), and the batch existed only in this loop, - * where nothing downstream could see it: plugin-sharing's `rule-hooks.ts` - * already routes a write whose row set exceeds `RULE_RECOMPUTE_ROW_CAP` into - * one set-based revoke plus one queued `evaluateAllRulesForObject`, but that - * branch reads ONE write's row set, and each of these writes legitimately - * carried a single row. Batching in the caller is what lets the machinery - * already built for this shape do its job — no change to `plugin-sharing`. - * - * The two scans are gone with the loop. The predicates they resolved are the - * predicates the writes now carry, one write each, so the matched set is - * unchanged row for row: `owner_id IS NULL`, then `owner_id = usr_system`. - * They stay two narrow writes rather than one `OR`/`IN` predicate for the - * reason the scans were two — driver portability — and they remain disjoint in - * this order, because the first write leaves `adminUserId` (never `usr_system`, - * refused above) where the NULLs were. - * - * The count reported per object is the affected-row count the predicate write - * resolves (#4639), not a length this function counted for itself. - * - * ### The bound moved, and it did not get smaller - * - * A predicate write carries no `limit`, so nothing here truncates at 10 000 any - * more. What bounds it now is the engine's own ceiling: a predicate write on an - * object carrying `beforeUpdate`/`afterUpdate` hooks — which every object does, - * objectql's own audit-stamp builtin is registered on `'*'` — is REFUSED whole - * above `MAX_BULK_PER_ROW_HOOK_ROWS` (10 000), because those hooks are - * contracted to fire per matched row. So the reachable population per predicate - * per run is the same 10 000 the scan limit allowed; what changed is that - * exceeding it is now LOUD (the engine's refusal names the count, the ceiling - * and both routes out, and this function logs it per object) instead of a - * silent partial claim of the first 10 000 rows. + * Every one of those is a full engine write (middleware chain, validation, hook + * dispatch, driver round trip), and the batch existed only in this loop, where + * nothing downstream could see it: plugin-sharing's `rule-hooks.ts` already + * routes a write whose row set exceeds its recompute cap (1 000) into one + * set-based revoke plus one queued `evaluateAllRulesForObject`, but that branch + * reads ONE write's row set, and each of these writes legitimately carried a + * single row. Batching in the caller is what lets machinery already built for + * this shape do its job — with no change to `plugin-sharing`. + * + * The unit of work is now a PAGE, not a row: read at most + * {@link CLAIM_PAGE_ROWS} unowned ids, re-own them with one predicate write, + * repeat until the predicate is exhausted. Two predicates per object + * (`owner_id IS NULL`, then `owner_id = usr_system`), each paged, so the matched + * set is the same set the old two-scan rule resolved — row for row — while the + * write count falls from N to ceil(N / {@link CLAIM_PAGE_ROWS}) per predicate. + * The predicates stay two narrow reads rather than one `OR`/`IN` for the reason + * the old scans were two (driver portability), and they stay disjoint in this + * order, because the NULL pass lands `adminUserId` — never `usr_system`, that + * target is refused at the top of this function. + * + * ### Why paged rather than one unbounded predicate write + * + * A predicate write carries no `limit`, so "one write per object" reads as the + * obvious shape — and it is refused on exactly the objects that need it most. + * `beforeUpdate`/`afterUpdate` hooks are contracted to fire PER MATCHED ROW on a + * predicate write (ADR-0058, bulk-write addendum D6), so the engine refuses one + * **whole** above {@link MAX_BULK_PER_ROW_HOOK_ROWS}; every object carries such + * hooks in practice, since objectql's own audit-stamp builtin is registered on + * `'*'`. Measured: 21 000 unowned rows re-owned **nothing at all**, where the + * pre-#14530 loop re-owned 10 000 of them. This function decides `owner_id`, + * which is a record-access field, so "the object was not claimed" is a + * permission outcome, not an observability detail. Paging keeps the batch AND + * the coverage: every page is one set-based write, sized under the engine's + * ceiling by construction, and the loop ends when the predicate stops matching. + * + * The page size is derived from that ceiling rather than chosen: half of it + * leaves room for a driver's own bound-parameter limits and for the ceiling + * being the engine's answer to a different question, while staying well above + * plugin-sharing's 1 000-row recompute cap — so a page is still large enough to + * take the trailing-batch branch rather than N per-row recomputes. */ import type { ServiceObject } from '@objectstack/spec/data'; +import { MAX_BULK_PER_ROW_HOOK_ROWS } from '@objectstack/spec/data'; import { SystemUserId } from '@objectstack/spec/system'; interface ClaimOwnershipOptions { @@ -76,10 +85,35 @@ interface ClaimOwnershipOptions { const SYSTEM_CTX = { isSystem: true }; +/** + * Rows re-owned by one predicate write. + * + * Derived from the engine's per-row hook ceiling, never a free literal: that + * ceiling is what refuses an over-sized predicate write, so the page size has to + * move with it. Half of it is the margin — a driver's bound-parameter limit + * applies to the `id IN (…)` list this sends, and the ceiling answers a question + * about hook fan-out rather than about statement width. Still far above + * plugin-sharing's 1 000-row recompute cap, so a full page is seen as a batch by + * the trailing-batch branch instead of being recomputed row by row. + */ +const CLAIM_PAGE_ROWS = Math.floor(MAX_BULK_PER_ROW_HOOK_ROWS / 2); + +/** + * Pages one predicate may claim before this function gives up on it. + * + * Termination does not depend on this: a page that re-owns rows makes them stop + * matching the predicate, so the set strictly shrinks, and a page that re-owns + * none breaks out below. The belt exists for the one shape that reasoning does + * not cover — a driver that reports an affected count for rows it did not write + * — where the alternative is a boot that never finishes. Hitting it is reported + * loudly, never silently. + */ +const MAX_CLAIM_PAGES = 1_000; + /** * "Unowned", as two driver-portable predicates rather than one `OR`/`IN`. * - * Order is load-bearing: the NULL write lands `adminUserId` — which cannot be + * Order is load-bearing: the NULL pass lands `adminUserId` — which cannot be * `usr_system` (refused at the top of {@link claimSeedOwnership}) — so the two * matched sets stay disjoint and their counts sum without double-counting a row. */ @@ -115,6 +149,84 @@ function affectedRowCount(value: unknown): number | undefined { return value; } +/** Ids from a `find` result, tolerating both the array and `{ records }` shapes. */ +function idsFrom(rows: any): string[] { + const list: any[] = Array.isArray(rows) + ? rows + : Array.isArray(rows?.records) + ? rows.records + : []; + const out: string[] = []; + for (const r of list) if (r?.id) out.push(String(r.id)); + return out; +} + +/** + * Re-own every row matching one unowned predicate, one page per write. + * + * Returns the SUM of the affected-row counts the page writes resolved — never a + * length this function counted for itself, and never just the last page's. + */ +async function claimPredicate( + ql: any, + objectName: string, + where: Record, + adminUserId: string, + logger: ClaimOwnershipOptions['logger'], +): Promise { + let total = 0; + for (let page = 0; page < MAX_CLAIM_PAGES; page += 1) { + const ids = idsFrom( + await ql.find( + objectName, + { where, limit: CLAIM_PAGE_ROWS, fields: ['id'] }, + { context: SYSTEM_CTX }, + ), + ); + if (ids.length === 0) return total; + + const affected = await ql.update( + objectName, + { owner_id: adminUserId }, + { where: { id: { $in: ids } }, multi: true, context: SYSTEM_CTX }, + ); + const count = affectedRowCount(affected); + if (count === undefined) { + // "Unknown", never "none": the page very likely WAS re-owned, so this + // neither adds a number it cannot attest nor re-reads a predicate whose + // state it does not know. What is already counted stays counted. + logger?.warn?.( + `[security] claimSeedOwnership could not read an affected-row count for ${objectName} ` + + '— the page was re-owned but this run cannot say how many, and paging stops here', + { object: objectName, where, page, result: typeof affected }, + ); + return total; + } + if (count === 0) { + // The read found rows and the write moved none of them: a write-scoping + // middleware narrowed the set to nothing. Re-reading the same predicate + // would return the same page forever, so stop — loudly, because "we + // could not claim these" is not the same as "there was nothing here". + logger?.warn?.( + `[security] claimSeedOwnership matched ${ids.length} unowned row(s) on ${objectName} ` + + 'but re-owned none of them; those rows stay unowned', + { object: objectName, where, page, matched: ids.length }, + ); + return total; + } + total += count; + // A short page is the last page: the predicate had fewer rows left than one + // page holds, so re-reading it would only confirm the emptiness. + if (ids.length < CLAIM_PAGE_ROWS) return total; + } + logger?.warn?.( + `[security] claimSeedOwnership stopped after ${MAX_CLAIM_PAGES} page(s) on ${objectName}; ` + + 'unowned rows may remain and the next run will claim them', + { object: objectName, where, pages: MAX_CLAIM_PAGES }, + ); + return total; +} + /** * Re-own every orphan seed row (owner_id NULL or usr_system) to `adminUserId`. * @@ -124,8 +236,9 @@ function affectedRowCount(value: unknown): number | undefined { * (c) are not `external` (federated remote-table bindings — read-only, DDL * forbidden, and their `owner_id` is not ours to reassign), * (d) declare an `owner_id` field, - * and re-owns the unowned rows as `isSystem` with one predicate write per - * {@link UNOWNED_PREDICATES} entry. Returns a per-object summary. + * and re-owns the unowned rows as `isSystem` with paged predicate writes, one + * page per write per {@link UNOWNED_PREDICATES} entry. Returns a per-object + * summary whose `count` is the sum of every page's affected-row count. */ export async function claimSeedOwnership( ql: any, @@ -134,9 +247,9 @@ export async function claimSeedOwnership( ): Promise<{ object: string; count: number }[]> { const logger = options.logger; if (!adminUserId || adminUserId === SystemUserId.SYSTEM) return []; - // Only `update` is required now that the scans are gone: this function asks - // the engine for exactly one capability, so the guard names exactly that one. - if (!ql || typeof ql.update !== 'function') return []; + if (!ql || typeof ql.update !== 'function' || typeof ql.find !== 'function') { + return []; + } const registry = (ql as any).registry; if (!registry || typeof registry.getAllObjects !== 'function') { logger?.warn?.('[security] claimSeedOwnership: registry unavailable'); @@ -162,21 +275,7 @@ export async function claimSeedOwnership( let updated = 0; for (const where of UNOWNED_PREDICATES) { try { - const affected = await ql.update( - schema.name, - { owner_id: adminUserId }, - { where, multi: true, context: SYSTEM_CTX }, - ); - const count = affectedRowCount(affected); - if (count === undefined) { - logger?.warn?.( - `[security] claimSeedOwnership could not read an affected-row count for ${schema.name} ` + - '— the rows were re-owned but this run cannot say how many', - { object: schema.name, where, result: typeof affected }, - ); - continue; - } - updated += count; + updated += await claimPredicate(ql, schema.name, where, adminUserId, logger); } catch (e) { // Best-effort per predicate, exactly as the per-id loop was: one // predicate that cannot land must not cost the object its other one, From 0418ccf8f9473fcb6a0c898680cdc5124a1e01c5 Mon Sep 17 00:00:00 2001 From: ObjectStack Agent Date: Wed, 2 Sep 2026 21:37:47 +0000 Subject: [PATCH 5/7] fix(plugin-security): bind the seed-ownership engine calls at the schema.name call site Paging behind an `objectName` parameter made `check:tenant-audit-census` read the write as `undecidable` and took its self-test to 5-of-19 red. Measured: the same file from 39126dc02 reproduces that on origin/main, and origin/main itself is green. The two engine calls are now bound where `schema.name` is a literal argument, so the census's answer about this file is byte-identical to its pre-change one -- no ledger row degraded to buy a green gate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../src/claim-seed-ownership.test.ts | 97 ++++++--- .../src/claim-seed-ownership.ts | 187 ++++++++++++------ 2 files changed, 203 insertions(+), 81 deletions(-) diff --git a/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts b/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts index 2ccde1a0ee..a3c8697e48 100644 --- a/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts +++ b/packages/plugins/plugin-security/src/claim-seed-ownership.test.ts @@ -3,7 +3,10 @@ import { describe, it, expect, vi } from 'vitest'; import { claimSeedOwnership } from './claim-seed-ownership.js'; import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; -import { MAX_BULK_PER_ROW_HOOK_ROWS } from '@objectstack/spec/data'; +import { + BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE, + MAX_BULK_PER_ROW_HOOK_ROWS, +} from '@objectstack/spec/data'; const SYSTEM = 'usr_system'; const ADMIN = 'usr_admin_human'; @@ -111,7 +114,10 @@ function makeQL( `(ADR-0058, bulk-write addendum), which is over the ${ceiling}-row ceiling for one ` + 'write. Nothing was written.', ), - { code: 'ERR_BULK_PER_ROW_HOOK_LIMIT' }, + // The code comes from the contract that defines it, never a literal: + // the fallback is keyed on this exact value, so a double that spelled + // its own would stop proving the fallback fires (#5480's rule). + { code: BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE }, ); } for (const row of matched) Object.assign(row, data); @@ -258,6 +264,11 @@ describe('claimSeedOwnership', () => { expect(result).toEqual([{ object: 'crm_lead', count: 500 }]); expect(writes).toHaveLength(2); expect(writes.every((w) => w.multi)).toBe(true); + // The set fits in one write per predicate, so the paged fallback never + // engages and this pass issues NO read at all — the predicate write IS the + // scan. Paging unconditionally was measured 13x slower on these sizes. + expect(ql.find).not.toHaveBeenCalled(); + expect(writes.map((w) => w.where)).toEqual([{ owner_id: null }, { owner_id: SYSTEM }]); // Every write carries the payload only — no `id` in `data`, which is what // routes the engine down `updateMany` instead of the single-id door. expect(writes.every((w) => Object.keys(w.data).join() === 'owner_id')).toBe(true); @@ -345,7 +356,7 @@ describe('claimSeedOwnership', () => { expect(await claimSeedOwnership(ql, ADMIN, { logger: { info: vi.fn(), warn } })).toEqual([]); expect(warn).toHaveBeenCalledTimes(2); // once per predicate expect(warn.mock.calls[0][0]).toContain('could not read an affected-row count'); - expect(ql.update).toHaveBeenCalledTimes(2); // stopped paging, did not spin + expect(ql.update).toHaveBeenCalledTimes(2); // one attempt each, no spin }); it('a refused predicate write costs that predicate only — never the object or the run', async () => { @@ -362,9 +373,8 @@ describe('claimSeedOwnership', () => { }, { onUpdate: (object, where) => { - if (object === 'crm_lead' && where?.id) { - const rows = (where.id as any).$in as string[]; - if (rows.includes('l1')) throw new Error('driver refused this page'); + if (object === 'crm_lead' && (where as any)?.owner_id === null) { + throw new Error('driver refused this write'); } }, }, @@ -376,13 +386,13 @@ describe('claimSeedOwnership', () => { { object: 'crm_lead', count: 1 }, // the usr_system predicate still landed { object: 'crm_case', count: 1 }, ]); - // crm_lead's NULL page was refused; its usr_system page landed. crm_case's - // NULL page landed and its usr_system predicate matched nothing, so it - // issued no write at all — an empty page is not a write. - expect(writes.map((w) => w.object)).toEqual(['crm_lead', 'crm_case']); + expect(writes.map((w) => w.object)).toEqual(['crm_lead', 'crm_case', 'crm_case']); expect(warn).toHaveBeenCalledTimes(1); expect(warn.mock.calls[0][0]).toContain('those rows stay unowned'); - expect(warn.mock.calls[0][1].error).toContain('driver refused this page'); + expect(warn.mock.calls[0][1].error).toContain('driver refused this write'); + // A failure that is NOT the per-row hook budget is never paged around: no + // read was issued for it, it simply propagated. + expect(ql.find).not.toHaveBeenCalled(); }); // ── [#14530 patch 1] paging: coverage past the engine's per-row ceiling ─── @@ -421,19 +431,24 @@ describe('claimSeedOwnership', () => { expect(legacyClaimedIds(seed())).toHaveLength(13_000); expect(everyUnowned.length).toBeGreaterThan(legacyClaimedIds(seed()).length); - // Still batched, not per row: writes are O(pages), and every page is sized - // so the engine's ceiling can never refuse it. - expect(writes.length).toBeLessThan(20); + // The paged fallback DID engage — a read was issued, which the under-ceiling + // path never does — and it converged back onto whole-set writes rather than + // paging to the end. + expect(reads.length).toBeGreaterThan(0); + // Still batched, not per row: a handful of writes, none of them over the + // ceiling that refused the unpaged attempt. + expect(writes.length).toBeLessThan(10); expect(writes.every((w) => w.matched.length <= MAX_BULK_PER_ROW_HOOK_ROWS)).toBe(true); // A page is big enough that plugin-sharing's 1000-row recompute cap still // sees these writes as batches rather than recomputing them row by row. expect(reads.every((r) => r.limit > 1_000 && r.limit <= MAX_BULK_PER_ROW_HOOK_ROWS)).toBe(true); }); - it('count is the SUM over pages, not the last page', async () => { - // With paging the reported count is an accumulation, and the easy bug is to - // let the final page's return value overwrite it. 12 000 unowned rows do not - // fit in one page, so a count equal to any single page's size is the bug. + it('count is the SUM over every write of the pass, not just the last one', async () => { + // Once a predicate needs the fallback the reported count is an accumulation, + // and the easy bug is to let the final whole-set write's return value + // overwrite it. 12 000 unowned rows cannot land in one write, so a count + // equal to any single write's size is the bug. const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; const rows = Array.from({ length: 12_000 }, (_, i) => ({ id: `l${i}`, owner_id: null })); const { ql, writes } = makeQL(schemas, { crm_lead: rows }); @@ -441,18 +456,19 @@ describe('claimSeedOwnership', () => { const result = await claimSeedOwnership(ql, ADMIN); expect(writes.length).toBeGreaterThan(1); - const perPage = writes.map((w) => w.matched.length); + const perWrite = writes.map((w) => w.matched.length); expect(result).toEqual([ - { object: 'crm_lead', count: perPage.reduce((s, n) => s + n, 0) }, + { object: 'crm_lead', count: perWrite.reduce((s, n) => s + n, 0) }, ]); expect(result[0].count).toBe(12_000); - expect(result[0].count).not.toBe(perPage[perPage.length - 1]); + for (const n of perWrite) expect(result[0].count).not.toBe(n); }); - it('stops rather than spinning when a page matches rows but re-owns none', async () => { + it('stops rather than spinning when a fallback page matches rows but re-owns none', async () => { // A write-scoping middleware can narrow a page to nothing. Re-reading the - // same predicate would then return the same page forever, so paging stops - // and says so — "we could not claim these" is not "there was nothing here". + // same predicate would then hand back the same page forever, so the fallback + // stops and says so — "we could not claim these" is not "there was nothing + // here". Without that break this is an unbounded loop at boot. const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; const warn = vi.fn(); const ql: any = { @@ -460,11 +476,40 @@ describe('claimSeedOwnership', () => { find: vi.fn(async () => [{ id: 'l1' }, { id: 'l2' }]), update: vi.fn(async (_o: string, data: any, options: any) => { assertEngineUpdateDispatch(data, options); - return 0; // matched by the read, moved by nothing + if ((options.where as any)?.id) return 0; // the page moved nothing + throw Object.assign(new Error('over the ceiling'), { + code: BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE, + }); }), }; expect(await claimSeedOwnership(ql, ADMIN, { logger: { info: vi.fn(), warn } })).toEqual([]); - expect(ql.update).toHaveBeenCalledTimes(2); // one attempt per predicate, no spin + // 2 predicates x (1 refused whole-set attempt + 1 page that moved nothing). + expect(ql.update).toHaveBeenCalledTimes(4); expect(warn.mock.calls[0][0]).toContain('but re-owned none of them'); }); + + it('pages ONLY on the declared per-row-hook refusal — any other failure propagates', async () => { + // The fallback is keyed on `BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE`. A refusal + // that merely LOOKS like it (same words, no code, or another code) must not + // send this pass reading and re-writing: that would page around a fault + // nobody diagnosed. It is warned per predicate and left alone. + const schemas = [{ name: 'crm_lead', fields: [{ name: 'owner_id' }] }]; + const warn = vi.fn(); + const ql: any = { + registry: { getAllObjects: () => schemas }, + find: vi.fn(async () => [{ id: 'l1' }]), + update: vi.fn(async (_o: string, data: any, options: any) => { + assertEngineUpdateDispatch(data, options); + throw Object.assign( + new Error("Refusing the bulk write on 'crm_lead': it matches 10500 rows"), + { code: 'ERR_SOMETHING_ELSE' }, + ); + }), + }; + expect(await claimSeedOwnership(ql, ADMIN, { logger: { info: vi.fn(), warn } })).toEqual([]); + expect(ql.find).not.toHaveBeenCalled(); + expect(ql.update).toHaveBeenCalledTimes(2); + expect(warn).toHaveBeenCalledTimes(2); + expect(warn.mock.calls[0][0]).toContain('those rows stay unowned'); + }); }); diff --git a/packages/plugins/plugin-security/src/claim-seed-ownership.ts b/packages/plugins/plugin-security/src/claim-seed-ownership.ts index 94592f6bac..d5512db31c 100644 --- a/packages/plugins/plugin-security/src/claim-seed-ownership.ts +++ b/packages/plugins/plugin-security/src/claim-seed-ownership.ts @@ -39,31 +39,42 @@ * single row. Batching in the caller is what lets machinery already built for * this shape do its job — with no change to `plugin-sharing`. * - * The unit of work is now a PAGE, not a row: read at most - * {@link CLAIM_PAGE_ROWS} unowned ids, re-own them with one predicate write, - * repeat until the predicate is exhausted. Two predicates per object - * (`owner_id IS NULL`, then `owner_id = usr_system`), each paged, so the matched - * set is the same set the old two-scan rule resolved — row for row — while the - * write count falls from N to ceil(N / {@link CLAIM_PAGE_ROWS}) per predicate. - * The predicates stay two narrow reads rather than one `OR`/`IN` for the reason - * the old scans were two (driver portability), and they stay disjoint in this - * order, because the NULL pass lands `adminUserId` — never `usr_system`, that - * target is refused at the top of this function. + * The unit of work is now the SET, not the row: one predicate write per unowned + * shape (`owner_id IS NULL`, then `owner_id = usr_system`), so the matched set + * is the same set the old two-scan rule resolved — row for row — while the + * write count stops scaling with N. The predicates stay two narrow writes rather + * than one `OR`/`IN` for the reason the old scans were two (driver + * portability), and they stay disjoint in this order, because the NULL pass + * lands `adminUserId` — never `usr_system`, that target is refused at the top of + * this function. * - * ### Why paged rather than one unbounded predicate write + * ### …and a paged fallback, because one write cannot always carry the set * - * A predicate write carries no `limit`, so "one write per object" reads as the - * obvious shape — and it is refused on exactly the objects that need it most. - * `beforeUpdate`/`afterUpdate` hooks are contracted to fire PER MATCHED ROW on a - * predicate write (ADR-0058, bulk-write addendum D6), so the engine refuses one - * **whole** above {@link MAX_BULK_PER_ROW_HOOK_ROWS}; every object carries such - * hooks in practice, since objectql's own audit-stamp builtin is registered on - * `'*'`. Measured: 21 000 unowned rows re-owned **nothing at all**, where the + * A predicate write carries no `limit`, so "one write per object" is the whole + * story right up until the object is large — and then it is refused on exactly + * the objects that need it most. `beforeUpdate`/`afterUpdate` hooks are + * contracted to fire PER MATCHED ROW on a predicate write (ADR-0058, bulk-write + * addendum D6), so the engine refuses one **whole** — nothing written — above + * {@link MAX_BULK_PER_ROW_HOOK_ROWS}; every object carries such hooks in + * practice, since objectql's own audit-stamp builtin is registered on `'*'`. + * Measured: 21 000 unowned rows re-owned **nothing at all**, where the * pre-#14530 loop re-owned 10 000 of them. This function decides `owner_id`, * which is a record-access field, so "the object was not claimed" is a - * permission outcome, not an observability detail. Paging keeps the batch AND - * the coverage: every page is one set-based write, sized under the engine's - * ceiling by construction, and the loop ends when the predicate stops matching. + * permission outcome, not an observability detail. + * + * So the refusal — a declared, total, nothing-written verdict whose own message + * names pagination as the remedy — is answered by taking one page of ids off the + * top ({@link CLAIM_PAGE_ROWS}) and trying the whole set again. Each page + * shrinks what is left until one write can carry it, and the pass ends on a + * whole-set write rather than on a count of pages. + * + * ⚠️ The order is not cosmetic. Paging unconditionally was measured 13× SLOWER + * on the sizes every real install has: an `id IN (…)` page is evaluated by + * `InMemoryDriver` as a linear scan of the id list PER ROW + * (`memory-matcher.ts`, `target.includes(value)`), so a paged claim is + * quadratic there, where the natural predicate is linear. 5 000 rows: 528 ms + * whole-set versus 5 865 ms always-paged, same engine, same driver, same row + * set. The page is therefore what the engine's refusal buys, not the default. * * The page size is derived from that ceiling rather than chosen: half of it * leaves room for a driver's own bound-parameter limits and for the ceiling @@ -73,7 +84,7 @@ */ import type { ServiceObject } from '@objectstack/spec/data'; -import { MAX_BULK_PER_ROW_HOOK_ROWS } from '@objectstack/spec/data'; +import { BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE, MAX_BULK_PER_ROW_HOOK_ROWS } from '@objectstack/spec/data'; import { SystemUserId } from '@objectstack/spec/system'; interface ClaimOwnershipOptions { @@ -86,7 +97,7 @@ interface ClaimOwnershipOptions { const SYSTEM_CTX = { isSystem: true }; /** - * Rows re-owned by one predicate write. + * Rows a single fallback page takes off the top of an over-ceiling predicate. * * Derived from the engine's per-row hook ceiling, never a free literal: that * ceiling is what refuses an over-sized predicate write, so the page size has to @@ -99,17 +110,30 @@ const SYSTEM_CTX = { isSystem: true }; const CLAIM_PAGE_ROWS = Math.floor(MAX_BULK_PER_ROW_HOOK_ROWS / 2); /** - * Pages one predicate may claim before this function gives up on it. + * Fallback pages one predicate may take before this function gives up on it. * * Termination does not depend on this: a page that re-owns rows makes them stop - * matching the predicate, so the set strictly shrinks, and a page that re-owns - * none breaks out below. The belt exists for the one shape that reasoning does - * not cover — a driver that reports an affected count for rows it did not write - * — where the alternative is a boot that never finishes. Hitting it is reported - * loudly, never silently. + * matching the predicate, so what is left strictly shrinks and reaches a size + * one write can carry, and a page that re-owns none breaks out below. The belt + * exists for the one shape that reasoning does not cover — a driver that reports + * an affected count for rows it did not write — where the alternative is a boot + * that never finishes. Hitting it is reported loudly, never silently. */ const MAX_CLAIM_PAGES = 1_000; +/** + * Is this the engine's per-row hook budget refusal (ADR-0058 D6)? + * + * The code is imported from the contract that defines it rather than spelled + * here, so a rename breaks the build instead of quietly turning the paged + * fallback off — which would put this pass back to claiming NOTHING on exactly + * the objects the fallback exists for. Every other failure is somebody else's + * and is rethrown. + */ +function isPerRowHookBudgetRefusal(e: unknown): boolean { + return (e as { code?: unknown } | null)?.code === BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE; +} + /** * "Unowned", as two driver-portable predicates rather than one `OR`/`IN`. * @@ -162,34 +186,74 @@ function idsFrom(rows: any): string[] { } /** - * Re-own every row matching one unowned predicate, one page per write. + * The two engine calls this pass makes on ONE object, already bound to it. * - * Returns the SUM of the affected-row counts the page writes resolved — never a - * length this function counted for itself, and never just the last page's. + * Bound by the caller rather than reached through an `objectName` parameter, and + * that is load-bearing beyond taste: `pnpm check:tenant-audit-census` reads + * every engine write call site statically, and a write whose object argument is + * a parameter is recorded `undecidable` — the census's word for "this pass no + * longer says which table it writes". Spelling `schema.name` AT the call site + * keeps the census's answer about this file exactly what it was before the + * paging fallback existed, with no ledger row degraded to buy a green gate. + */ +interface ObjectWriter { + /** Re-own every row a predicate matches; resolves the affected-row count. */ + reown: (predicate: Record) => Promise; + /** At most one page of ids the predicate still matches. */ + readPage: (predicate: Record) => Promise; +} + +/** + * Re-own every row matching one unowned predicate. + * + * One write for the whole set; a page off the top and another attempt whenever + * the engine refuses that write for its per-row hook budget. Returns the SUM of + * the affected-row counts every write in the pass resolved — never a length this + * function counted for itself, and never just the last write's. */ async function claimPredicate( - ql: any, + io: ObjectWriter, objectName: string, where: Record, - adminUserId: string, logger: ClaimOwnershipOptions['logger'], ): Promise { + const { reown, readPage } = io; let total = 0; for (let page = 0; page < MAX_CLAIM_PAGES; page += 1) { - const ids = idsFrom( - await ql.find( - objectName, - { where, limit: CLAIM_PAGE_ROWS, fields: ['id'] }, - { context: SYSTEM_CTX }, - ), - ); - if (ids.length === 0) return total; + // The whole remaining set in ONE write — the shape this card is about, and + // the one that runs on every install small enough for it (which is all of + // them, in practice). No read at all on this path. + try { + const whole = affectedRowCount(await reown(where)); + if (whole === undefined) { + logger?.warn?.( + `[security] claimSeedOwnership could not read an affected-row count for ${objectName} ` + + '— the rows were re-owned but this run cannot say how many', + { object: objectName, where, page }, + ); + return total; + } + return total + whole; + } catch (e) { + if (!isPerRowHookBudgetRefusal(e)) throw e; + } + + // Refused whole: more rows than one write may fan per-row hooks over. + // Take a page off the top by id and try the whole set again — the remainder + // shrinks by a page each time until a single write can carry it. + const ids = idsFrom(await readPage(where)); + if (ids.length === 0) { + // The write refused for being over the ceiling and the read found nothing + // to page: the two disagree, so stop rather than retry the same pair. + logger?.warn?.( + `[security] claimSeedOwnership could not page ${objectName}: the write refused as ` + + 'over-sized but the predicate matched no rows to page; those rows stay unowned', + { object: objectName, where, page }, + ); + return total; + } - const affected = await ql.update( - objectName, - { owner_id: adminUserId }, - { where: { id: { $in: ids } }, multi: true, context: SYSTEM_CTX }, - ); + const affected = await reown({ id: { $in: ids } }); const count = affectedRowCount(affected); if (count === undefined) { // "Unknown", never "none": the page very likely WAS re-owned, so this @@ -215,12 +279,9 @@ async function claimPredicate( return total; } total += count; - // A short page is the last page: the predicate had fewer rows left than one - // page holds, so re-reading it would only confirm the emptiness. - if (ids.length < CLAIM_PAGE_ROWS) return total; } logger?.warn?.( - `[security] claimSeedOwnership stopped after ${MAX_CLAIM_PAGES} page(s) on ${objectName}; ` + + `[security] claimSeedOwnership stopped after ${MAX_CLAIM_PAGES} fallback page(s) on ${objectName}; ` + 'unowned rows may remain and the next run will claim them', { object: objectName, where, pages: MAX_CLAIM_PAGES }, ); @@ -236,9 +297,10 @@ async function claimPredicate( * (c) are not `external` (federated remote-table bindings — read-only, DDL * forbidden, and their `owner_id` is not ours to reassign), * (d) declare an `owner_id` field, - * and re-owns the unowned rows as `isSystem` with paged predicate writes, one - * page per write per {@link UNOWNED_PREDICATES} entry. Returns a per-object - * summary whose `count` is the sum of every page's affected-row count. + * and re-owns the unowned rows as `isSystem` with one predicate write per + * {@link UNOWNED_PREDICATES} entry, paging that write only when the engine + * refuses it for its per-row hook budget. Returns a per-object summary whose + * `count` is the sum of every write's affected-row count. */ export async function claimSeedOwnership( ql: any, @@ -272,10 +334,25 @@ export async function claimSeedOwnership( if ((schema as any).external) continue; if (!hasOwnerField(schema)) continue; + // Bound HERE, where `schema.name` is a literal argument at the call site — + // see {@link ObjectWriter} for why that spelling is not incidental. + const io: ObjectWriter = { + reown: (predicate) => ql.update( + schema.name, + { owner_id: adminUserId }, + { where: predicate, multi: true, context: SYSTEM_CTX }, + ), + readPage: (predicate) => ql.find( + schema.name, + { where: predicate, limit: CLAIM_PAGE_ROWS, fields: ['id'] }, + { context: SYSTEM_CTX }, + ), + }; + let updated = 0; for (const where of UNOWNED_PREDICATES) { try { - updated += await claimPredicate(ql, schema.name, where, adminUserId, logger); + updated += await claimPredicate(io, schema.name, where, logger); } catch (e) { // Best-effort per predicate, exactly as the per-id loop was: one // predicate that cannot land must not cost the object its other one, From 789ed0afe9b3c123cf0d0149e284d4ff10b6b1bd Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 22:45:10 +0000 Subject: [PATCH 6/7] chore(gates): ratchet the engine-double ledger for the claim-seed-ownership doubles `check:engine-double-contract` reported RETAINED [update] on `claim-seed-ownership.test.ts`: the paging pins grew its engine doubles from 1 to 5, which is the direction this ledger wants, so the gate's own prescribed fix is to regenerate. Regenerated with `--write` on the merged tree (the ledger is contended by sibling PRs), never hand-edited; the regeneration reports "1 added or grown, 0 lost". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- scripts/engine-double-contract.pinned.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 89a5672181..fa7e5d36cb 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2439,7 +2439,7 @@ { "file": "packages/plugins/plugin-security/src/claim-seed-ownership.test.ts", "verb": "update", - "pinned": 1 + "pinned": 5 }, { "file": "packages/plugins/plugin-security/src/controlled-by-parent-chain.test.ts", From 369b400b2b68e6728441c9de68337d79c67b227f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 23:00:06 +0000 Subject: [PATCH 7/7] =?UTF-8?q?docs(changeset):=20describe=20the=20shape?= =?UTF-8?q?=20that=20actually=20shipped=20=E2=80=94=20whole-set=20write=20?= =?UTF-8?q?first,=20page=20only=20on=20refusal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changeset still described the always-paged shape ("read at most 5 000 ids, re-own them, repeat"), which was measured 13x slower on the sizes every real install has and is not what landed. Restated: one predicate write per unowned shape, a page off the top only when the engine refuses that write for its per-row hook budget, plus the re-measured over-ceiling number (21 000 of 21 000 claimed, 8 engine writes) that the paging exists to produce. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../claim-seed-ownership-predicate-write.md | 51 +++++++++++-------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/.changeset/claim-seed-ownership-predicate-write.md b/.changeset/claim-seed-ownership-predicate-write.md index 25d97893d4..c4ed6ef15c 100644 --- a/.changeset/claim-seed-ownership-predicate-write.md +++ b/.changeset/claim-seed-ownership-predicate-write.md @@ -2,22 +2,22 @@ '@objectstack/plugin-security': patch --- -perf(plugin-security): claim seed ownership with paged predicate writes (#14530) +perf(plugin-security): claim seed ownership with a predicate write per object, paged only when the engine refuses it (#14530) `claimSeedOwnership` — the pass that hands seeded business records to the first platform admin — scanned every `owner_id`-declaring object twice at `limit: 10_000` and then issued **one single-id `update` per matched id**: up to 20 000 full engine writes for one object, each paying the whole middleware, -validation and hook chain. The unit of work is now a **page**, not a row: read -at most 5 000 unowned ids, re-own them with one predicate write, repeat until -the predicate is exhausted — for each of the two unowned shapes (`owner_id IS -NULL`, then `owner_id = usr_system`). The matched set is unchanged row for row, -and the count reported per object is the sum of the affected-row counts the page -writes resolve rather than a length this pass counted for itself. +validation and hook chain. The unit of work is now the **set**, not the row: one +predicate write per unowned shape (`owner_id IS NULL`, then +`owner_id = usr_system`), so the matched set is the same set the old two-scan +rule resolved — row for row — while the write count stops scaling with N. The +count reported per object is the sum of the affected-row counts those writes +resolve, never a length this pass counted for itself. Measured on a real ObjectQL engine (in-memory driver, one sharing-rule-covered -object, shared box): 2 000 rows 2 122 ms → 208 ms; 5 000 rows 10 658 ms → 528 ms, -with engine `update` calls falling from N to one per page. +object, shared box): 2 000 rows 2 122 ms to 208 ms; 5 000 rows 10 658 ms to +528 ms, with engine `update` calls falling from N to two per object. The second half is what the batch buys downstream. plugin-sharing's `rule-hooks` already routes a write whose row set exceeds `RULE_RECOMPUTE_ROW_CAP` (1 000) @@ -25,20 +25,27 @@ into one set-based revoke plus one queued `evaluateAllRulesForObject`, but that branch reads **one write's** row set, and every write in the old loop legitimately carried a single row — so the batch existed only in the caller, where nothing downstream could see it. Batching here is what lets machinery -already built for this shape do its job; `plugin-sharing` is unchanged, and the -page size is deliberately far above that cap so a full page is still seen as a -batch. +already built for this shape do its job; `plugin-sharing` is unchanged. -**Why paged rather than one write per object.** A predicate write carries no -`limit`, so "one write per object" is the obvious shape — and the engine refuses -it whole above `MAX_BULK_PER_ROW_HOOK_ROWS` (10 000), because `beforeUpdate` / -`afterUpdate` hooks are contracted to fire per matched row on a predicate write -(ADR-0058 D6) and every object carries such hooks in practice. Measured: 21 000 -unowned rows re-owned **nothing**, where the old loop re-owned 10 000 of them. -This pass decides `owner_id`, a record-access field, so an unclaimed object is a -permission outcome and not an observability detail. Paging keeps the batch and -the coverage: the same 21 000-row case now claims every one of them, and the -page size is derived from that ceiling rather than chosen, so it moves with it. +**And a paged fallback, because one write cannot always carry the set.** A +predicate write carries no `limit`, so the bound becomes the engine's own +`MAX_BULK_PER_ROW_HOOK_ROWS` (10 000): `beforeUpdate` / `afterUpdate` hooks are +contracted to fire per matched row on a predicate write (ADR-0058 D6), and every +object carries such hooks in practice, so the engine refuses an over-sized write +**whole** — nothing written. Measured: 21 000 unowned rows re-owned **nothing**, +where the old loop re-owned 10 000 of them. This pass decides `owner_id`, a +record-access field, so an unclaimed object is a permission outcome and not an +observability detail. The refusal is now answered by taking one page of ids off +the top (half the ceiling) and re-attempting the whole set, until one write can +carry what is left. Re-measured after paging: the same 21 000-row object claims +**all 21 000**, in 8 engine writes and 3 reads. + +The order is not cosmetic. Paging unconditionally measured 13x slower on the +sizes every real install has — an `id IN (…)` page is a linear scan of the id +list per row in `InMemoryDriver`, so an always-paged claim is quadratic there +where the natural predicate is linear (5 000 rows: 528 ms whole-set versus +5 865 ms always-paged). The page is what the engine's refusal buys, not the +default. `patch`: no declared surface moves, no export changes, and the reachable population strictly grows.