diff --git a/.changeset/claim-seed-ownership-predicate-write.md b/.changeset/claim-seed-ownership-predicate-write.md new file mode 100644 index 0000000000..c4ed6ef15c --- /dev/null +++ b/.changeset/claim-seed-ownership-predicate-write.md @@ -0,0 +1,51 @@ +--- +'@objectstack/plugin-security': patch +--- + +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 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 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) +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 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. 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..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,31 +3,168 @@ import { describe, it, expect, vi } from 'vitest'; import { claimSeedOwnership } from './claim-seed-ownership.js'; import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +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'; -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[]; +} + +/** One recorded call to `ql.find` — the page request the writer issued. */ +interface RecordedRead { + object: string; + where: any; + limit: number; + returned: number; +} + +/** + * `where` as `claimSeedOwnership` spells it: field equality for the unowned + * predicates, and `{ id: { $in: [...] } }` for a page write. + * + * 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 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 actual === (v ?? null); + }); +} + +/** + * 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 + * 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, + 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 (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; + 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) => { - 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}'`); + } + 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.', + ), + // 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); + writes.push({ + object, + data, + where, + multi: options?.multi === true, + matched: matched.map((r) => r.id), + }); + return matched.length; }), }; - return { ql, updates }; + 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. + * + * 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. 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(); + 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; +} + +/** 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', () => { @@ -38,10 +175,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 +186,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 +205,20 @@ 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 +228,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 +245,271 @@ 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); + // 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); + 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' }] }]; + 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); + call += 1; + return call === 1 ? 7 : 11; + }), + }; + expect(await claimSeedOwnership(ql, ADMIN)).toEqual([{ object: 'crm_lead', count: 18 }]); + }); + + 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 — 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); // once per predicate + expect(warn.mock.calls[0][0]).toContain('could not read an affected-row count'); + 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 () => { + 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 as any)?.owner_id === null) { + throw new Error('driver refused this write'); + } + }, + }, + ); + + 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('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 ─── + + 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); + + // 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 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 }); + + const result = await claimSeedOwnership(ql, ADMIN); + + expect(writes.length).toBeGreaterThan(1); + const perWrite = writes.map((w) => w.matched.length); + expect(result).toEqual([ + { object: 'crm_lead', count: perWrite.reduce((s, n) => s + n, 0) }, + ]); + expect(result[0].count).toBe(12_000); + for (const n of perWrite) expect(result[0].count).not.toBe(n); + }); + + 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 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 = { + registry: { getAllObjects: () => schemas }, + find: vi.fn(async () => [{ id: 'l1' }, { id: 'l2' }]), + update: vi.fn(async (_o: string, data: any, options: any) => { + assertEngineUpdateDispatch(data, options); + 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([]); + // 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 2ceff135b3..d5512db31c 100644 --- a/packages/plugins/plugin-security/src/claim-seed-ownership.ts +++ b/packages/plugins/plugin-security/src/claim-seed-ownership.ts @@ -25,9 +25,66 @@ * 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] 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 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 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. + * + * ### …and a paged fallback, because one write cannot always carry the set + * + * 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. + * + * 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 + * 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 { 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 { @@ -39,6 +96,56 @@ interface ClaimOwnershipOptions { const SYSTEM_CTX = { isSystem: true }; +/** + * 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 + * 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); + +/** + * 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 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`. + * + * 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. + */ +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 +155,139 @@ 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; +} + +/** 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; +} + +/** + * The two engine calls this pass makes on ONE object, already bound to it. + * + * 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( + io: ObjectWriter, + objectName: string, + where: Record, + logger: ClaimOwnershipOptions['logger'], +): Promise { + const { reown, readPage } = io; + let total = 0; + for (let page = 0; page < MAX_CLAIM_PAGES; page += 1) { + // 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 reown({ id: { $in: ids } }); + 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; + } + logger?.warn?.( + `[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 }, + ); + return total; +} + /** * Re-own every orphan seed row (owner_id NULL or usr_system) to `adminUserId`. * @@ -57,7 +297,10 @@ 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, 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, @@ -91,53 +334,39 @@ 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( - schema.name, - { where, limit: 10_000, fields: ['id'] }, - { context: SYSTEM_CTX }, + // 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(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, + // 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 }, ); - 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 }, - ); - updated += 1; - } catch (e) { - logger?.warn?.(`[security] claimSeedOwnership failed for ${schema.name}:${id}`, { - 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) { 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",