diff --git a/.changeset/hook-dispatch-marker.md b/.changeset/hook-dispatch-marker.md new file mode 100644 index 0000000000..b5c11c5919 --- /dev/null +++ b/.changeset/hook-dispatch-marker.md @@ -0,0 +1,77 @@ +--- +"@objectstack/spec": minor +"@objectstack/objectql": minor +"@objectstack/plugin-sharing": patch +"@objectstack/service-storage": patch +--- + +fix(spec,objectql,sharing,storage): a hook can tell a per-row bulk dispatch from a single-record write again (#6966) + +A predicate (`multi: true`) write dispatches its lifecycle hooks **once per +matched row** — `after*` since #5038, `before*` since #5574 — on a context +deliberately indistinguishable from a single-id write's, so a handler written +for one record works unchanged on a batch. That indistinguishability is the +feature, and it also erased the only signal several handlers had. + +Before #5574 a bulk `before*` fired once with `input.id` present-but-`undefined`, +so "`input.id` is empty" meant "this call stands for N rows". Guards across the +platform were written on it. Every one of them **silently inverted** rather than +failing: a per-row context has an id, so the guard now answers "single write" for +every row of a batch. Two further assumptions broke with it — that the engine +reuses one `HookContext` across a write's before/after pair, and that `after*` +work keyed on the write's row set runs once. + +### New: `HookContext.dispatch` + +The engine now states the fact rather than leaving it to be inferred: + +```ts +ctx.dispatch // { mode: 'record' | 'per-row', index: number, scope: object } | undefined +``` + +- `mode` — `'record'` when the call is the caller's whole write; `'per-row'` + when it is one of N. +- `index` — position in the fan-out. `index === 0` is how a handler does + batch-scoped work once instead of N times. +- `scope` — scratch shared by **every** dispatch of one write, both phases, same + object identity. This is the seam handlers used to get by stashing on the + context itself, which only ever worked because a single-id write reuses one + context across its pair. + +Bound at every write dispatch site — insert, update, delete, both phases. +Optional, and an absent marker reads as "not a per-row dispatch", so a handler +reads `ctx.dispatch?.mode === 'per-row'` and existing code keeps its behaviour. +Reads carry no marker: a read has no fan-out. + +It is deliberately **not** the `isPredicateBulkWrite` discriminator #5574 +retired. That one was removed under ADR-0049 for having neither a producer nor a +reachable consumer — it inferred "bulk" from `input.id` and `options.multi` at +the consumer, which is exactly what `asScalarId` stays unexported to prevent +(#4434 / #4550). This one is produced by the engine at the point the dispatch +ladder is decided, and the platform's own handlers read it. + +### Behaviour fixed + +**Sharing rules and the record-share cascade (`@objectstack/plugin-sharing`).** +The `before*` hook stashes the write's affected row set for the `after*` hook to +act on. On a predicate write that stash was landing on a per-row context the +`after` phase never saw, so `readAffectedRows` answered `resolve-failed` and both +subscribers took their safe branch: every bulk update or delete on a ruled object +revoked **all** of that object's rule grants and queued a full asynchronous +re-grant — once per matched row, with the repeats racing each other's re-grants. +Access was never widened (the trade is the ruling's "over-granting is an +incident, under-granting is a wobble" direction), but a bounded write now takes +the bounded path again: the rows are unioned as the engine hands them over, the +cap still applies to the union, and the `after*` work runs once per write. + +**File-reference ownership (`@objectstack/service-storage`).** The `beforeDelete` +hook that pre-resolved ids for a `where`-shaped delete was dead on every path, +and `afterDelete` was falling back to one `sys_file` lookup **per row** where the +batch fits one `$in`. Both are fixed by the marker, and the pre-resolution query +is gone entirely — the engine has already matched the rows and hands them over. +The `beforeUpdate` copy-on-claim pass no longer runs once per row against a +batch-scoped payload, which also removes a row-conditioned rewrite of a shared +`SET` clause (out of contract under ADR-0058 Addendum II D3). + +No authored metadata changes, and no write's result, event or return contract +changes. diff --git a/content/docs/references/data/hook.mdx b/content/docs/references/data/hook.mdx index f9f0bfa882..00f63e9b3d 100644 --- a/content/docs/references/data/hook.mdx +++ b/content/docs/references/data/hook.mdx @@ -36,6 +36,7 @@ const result = HookContextSchema.parse(data); | **input** | `Record` | ✅ | Mutable input parameters | | **result** | `any` | optional | Operation result (After hooks only) | | **previous** | `Record` | optional | Record state before operation | +| **dispatch** | `{ mode: Enum<'record' \| 'per-row'>; index: integer; scope: Record }` | optional | How this hook call relates to the caller's write (engine-produced; #6966) | | **session** | `{ userId?: string; actor?: string; organizationId?: string; accessToken?: string; … }` | optional | Current session context | | **provenance** | `{ flowRunId?: string; attributedUserId?: string }` | optional | Server-stamped write provenance (never client-supplied, never an authorization input) | | **transaction** | `any` | optional | Database transaction handle | diff --git a/docs/audits/2026-07-unknown-key-strictness-ledger.counts.md b/docs/audits/2026-07-unknown-key-strictness-ledger.counts.md index c6ed9f6704..6208f2a43a 100644 --- a/docs/audits/2026-07-unknown-key-strictness-ledger.counts.md +++ b/docs/audits/2026-07-unknown-key-strictness-ledger.counts.md @@ -21,8 +21,8 @@ regenerate. | Measure | Value | |---|---| | Triaged directories | 5 | -| Object sites in them | 436 | -| Still-open (strip) sites | 180 | +| Object sites in them | 437 | +| Still-open (strip) sites | 181 | | Files carrying at least one | 27 | Remaining strip sites by class: @@ -31,7 +31,7 @@ Remaining strip sites by class: |---|---| | authorable — the ruling's forced scope | 41 | | unresolved — needs a per-schema verdict | 33 | -| wire / open — out of forced scope | 104 | +| wire / open — out of forced scope | 105 | | no door — no carrier, ADR-0049 territory | 1 | | no gate — carrier live, no parse | 0 | | covered — no carrier, no parse, guarded at every consumer | 1 | @@ -45,11 +45,11 @@ The `strict` column is the one the campaign schedules against; it counts both th | Dir | Sites | strict | passthrough | catchall | strip | |---|---|---|---|---|---| | `ui/` | 160 | 118 | 5 | 0 | 37 | -| `data/` | 164 | 56 | 1 | 0 | 107 | +| `data/` | 165 | 56 | 1 | 0 | 108 | | `automation/` | 65 | 42 | 0 | 0 | 23 | | `security/` | 20 | 7 | 0 | 0 | 13 | | `studio/` | 27 | 27 | 0 | 0 | 0 | -| **total** | **436** | **250** | **6** | **0** | **180** | +| **total** | **437** | **250** | **6** | **0** | **181** | ## File-level triage — site counts @@ -102,14 +102,14 @@ classify and is not listed (it becomes reportable the day it grows its first sit | `field.zod.ts` | 11 | | `filter.zod.ts` | 11 | | `hook-body.zod.ts` | 2 | -| `hook.zod.ts` | 6 | +| `hook.zod.ts` | 7 | | `mapping.zod.ts` | 3 | | `object.zod.ts` | 20 | | `query.zod.ts` | 5 | | `seed-loader.zod.ts` | 12 | | `seed.zod.ts` | 1 | | `validation.zod.ts` | 6 | -| **total** | **164** | +| **total** | **165** | ### `automation/` — sites @@ -179,7 +179,7 @@ over it is here. ### `data/` — open -**107 strip of 164**, in 16 file(s). +**108 strip of 165**, in 16 file(s). | File | Strip | Sites | |---|---|---| @@ -195,17 +195,17 @@ over it is here. | `field-value.zod.ts` | 1 | 2 | | `field.zod.ts` | 3 | 11 | | `filter.zod.ts` | 11 | 11 | -| `hook.zod.ts` | 4 | 6 | +| `hook.zod.ts` | 5 | 7 | | `object.zod.ts` | 1 | 20 | | `query.zod.ts` | 4 | 5 | | `seed-loader.zod.ts` | 12 | 12 | -| **total** | **107** | **164** | +| **total** | **108** | **165** | | Bucket | Sites | |---|---| | authorable — the ruling's forced scope | 9 | | unresolved — needs a per-schema verdict | 33 | -| wire / open — out of forced scope | 65 | +| wire / open — out of forced scope | 66 | | no door — no carrier, ADR-0049 territory | 0 | | no gate — carrier live, no parse | 0 | | covered — no carrier, no parse, guarded at every consumer | 0 | diff --git a/packages/objectql/src/bulk-write-per-row-hooks.test.ts b/packages/objectql/src/bulk-write-per-row-hooks.test.ts index ab56a1e1bb..da4151e3e9 100644 --- a/packages/objectql/src/bulk-write-per-row-hooks.test.ts +++ b/packages/objectql/src/bulk-write-per-row-hooks.test.ts @@ -921,6 +921,183 @@ describe('[#5574 / D7] the matched row set is read ONCE, and serves everything', }); }); +/* ──────────────────────────────────────────────────────────────────────────── + * 8. [#6966] The dispatch marker — the fact D1/D2's indistinguishability erased + * + * Sections 1–7 make a per-row context deliberately indistinguishable from a + * single-id one, which is what lets a handler written for one record work + * unchanged on a batch. The cost is that "am I one of N?" became unanswerable, + * and the guards that had been answering it from `input.id`'s shape — "no id + * means bulk" — silently inverted rather than failing. `HookContext.dispatch` + * is that question restored as an engine-stated fact. + * + * Pinned here rather than in a file of its own for section 7's reason: the + * marker exists to describe THIS fan-out, and a marker that drifts from the + * dispatch it describes is worse than none. + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#6966] every dispatched context carries the marker, on all four write paths', () => { + const seenOn = async (event: string, drive: (engine: ObjectQL, ids: string[]) => Promise) => { + const seen: Array<{ mode: unknown; index: unknown; id: unknown }> = []; + const { engine } = await boot([hook('marker', event, (ctx) => { + const d = (ctx as any).dispatch; + seen.push({ mode: d?.mode, index: d?.index, id: (ctx.input as any)?.id }); + })]); + const rows = await seedTasks(engine, [ + { title: 'a', status: 'todo' }, + { title: 'b', status: 'todo' }, + ]); + await drive(engine, rows.map((r) => String(r.id))); + return seen; + }; + + it('single-id UPDATE ⇒ one dispatch, mode "record", index 0', async () => { + for (const event of ['beforeUpdate', 'afterUpdate']) { + const seen = await seenOn(event, (engine, ids) => + engine.update('task', { status: 'done' }, { where: { id: ids[0] } })); + expect(seen, event).toEqual([{ mode: 'record', index: 0, id: seen[0].id }]); + } + }); + + it('single-id DELETE ⇒ one dispatch, mode "record", index 0', async () => { + for (const event of ['beforeDelete', 'afterDelete']) { + const seen = await seenOn(event, (engine, ids) => engine.delete('task', { where: { id: ids[0] } } as any)); + expect(seen, event).toEqual([{ mode: 'record', index: 0, id: seen[0].id }]); + } + }); + + it('predicate UPDATE ⇒ N dispatches, mode "per-row", index counting 0..N-1', async () => { + for (const event of ['beforeUpdate', 'afterUpdate']) { + const seen = await seenOn(event, (engine) => + engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } })); + expect(seen.map((s) => s.mode), event).toEqual(['per-row', 'per-row']); + // The INDEX is the member a consumer keys "do this once per batch" on, so + // it must count rather than repeat the batch context's 0. + expect(seen.map((s) => s.index), event).toEqual([0, 1]); + expect(new Set(seen.map((s) => s.id)).size, event).toBe(2); + } + }); + + it('predicate DELETE ⇒ N dispatches, mode "per-row", index counting 0..N-1', async () => { + for (const event of ['beforeDelete', 'afterDelete']) { + const seen = await seenOn(event, (engine) => + engine.delete('task', { multi: true, where: { status: 'todo' } } as any)); + expect(seen.map((s) => s.mode), event).toEqual(['per-row', 'per-row']); + expect(seen.map((s) => s.index), event).toEqual([0, 1]); + expect(new Set(seen.map((s) => s.id)).size, event).toBe(2); + } + }); + + it('a batch INSERT is per-row too, and a single insert is not', async () => { + const batch: any[] = []; + const { engine } = await boot([hook('marker', 'beforeInsert', (ctx) => { + batch.push((ctx as any).dispatch); + })]); + await engine.insert('task', [{ title: 'a' }, { title: 'b' }, { title: 'c' }] as any); + expect(batch.map((d) => [d.mode, d.index])).toEqual([['per-row', 0], ['per-row', 1], ['per-row', 2]]); + + batch.length = 0; + await engine.insert('task', { title: 'solo' } as any); + expect(batch.map((d) => [d.mode, d.index])).toEqual([['record', 0]]); + }); + + it('NO handler is ever dispatched on a context without the marker', async () => { + // The contract the JSDoc states, pinned as an invariant rather than + // path-by-path. It is what makes `ctx.dispatch?.mode` safe to read without + // a "what if the engine did not bind it" branch — and it covers the one + // context a reader might worry about: `update()`/`delete()` keep a + // BATCH-scoped `hookContext` on the predicate path, and the claim is that + // no handler ever sees it (the per-row loop runs whenever `hasHooksFor` is + // true, and when it is false no handler runs at all). + const unmarked: string[] = []; + const events = [ + 'beforeInsert', 'afterInsert', + 'beforeUpdate', 'afterUpdate', + 'beforeDelete', 'afterDelete', + ]; + const { engine } = await boot(events.map((e) => hook(`probe_${e}`, e, (ctx) => { + const d = (ctx as any).dispatch; + if (!d || (d.mode !== 'record' && d.mode !== 'per-row') || typeof d.index !== 'number' || !d.scope) { + unmarked.push(`${e}:${JSON.stringify(d)}`); + } + }))); + + // Every write shape this engine can take, in one pass. + await engine.insert('task', { title: 'solo', status: 'todo' } as any); + const batch: any = await engine.insert('task', [ + { title: 'a', status: 'todo' }, + { title: 'b', status: 'todo' }, + ] as any); + await engine.update('task', { status: 'mid' }, { where: { id: batch[0].id } }); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); + await engine.delete('task', { where: { id: batch[0].id } } as any); + await engine.delete('task', { multi: true, where: { status: 'done' } } as any); + + expect(unmarked).toEqual([]); + }); +}); + +describe('[#6966] `scope` is one object per WRITE, spanning both phases', () => { + it('carries a before-phase stash to the after phase of a predicate update', async () => { + // The regression this closes: a per-row `before*` context is freshly built, + // so a handler stashing on the CONTEXT (which is what every consumer did, + // because a single-id write reuses one context across its pair) wrote to an + // object the after phase never sees. + const arrived: unknown[] = []; + const { engine } = await boot([ + hook('stash', 'beforeUpdate', (ctx) => { + const scope = (ctx as any).dispatch.scope as Record; + ((scope.ids ??= []) as unknown[]).push((ctx.input as any).id); + }), + hook('read', 'afterUpdate', (ctx) => { + arrived.push(JSON.stringify(((ctx as any).dispatch.scope as any).ids)); + }), + ]); + const rows = await seedTasks(engine, [ + { title: 'a', status: 'todo' }, + { title: 'b', status: 'todo' }, + ]); + const ids = rows.map((r) => String(r.id)); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); + + // Every after dispatch sees the WHOLE batch's collection — all before + // dispatches complete before the write, so the union is closed by then. + expect(arrived).toEqual([JSON.stringify(ids), JSON.stringify(ids)]); + }); + + it('is per WRITE — a second call gets a fresh scope, never the first call\'s', async () => { + const scopes: unknown[] = []; + const { engine } = await boot([hook('mark', 'beforeUpdate', (ctx) => { + const scope = (ctx as any).dispatch.scope as Record; + scopes.push(scope); + scope.touched = true; + })]); + const rows = await seedTasks(engine, [{ title: 'a', status: 'todo' }]); + + await engine.update('task', { status: 'mid' }, { where: { id: rows[0].id } }); + await engine.update('task', { status: 'done' }, { where: { id: rows[0].id } }); + + expect(scopes).toHaveLength(2); + expect(scopes[0]).not.toBe(scopes[1]); + }); + + it('is the SAME object across a single-id write\'s before/after pair', async () => { + const scopes: unknown[] = []; + const grab = (ctx: HookContext) => { scopes.push((ctx as any).dispatch.scope); }; + const { engine } = await boot([ + hook('b', 'beforeUpdate', grab), + hook('a', 'afterUpdate', grab), + ]); + const rows = await seedTasks(engine, [{ title: 'a', status: 'todo' }]); + + await engine.update('task', { status: 'done' }, { where: { id: rows[0].id } }); + + expect(scopes).toHaveLength(2); + expect(scopes[0]).toBe(scopes[1]); + }); +}); + /* ──────────────────────────────────────────────────────────────────────────── * Harness * ──────────────────────────────────────────────────────────────────────────── */ diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index e23ae3539b..ee6b47942f 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1737,12 +1737,16 @@ export class ObjectQL implements IObjectQLEngine { ): HookContext[] { const schema = this._registry.getObject(object); const options = (batchCtx.input as { options?: unknown } | undefined)?.options; - return rows.map((row) => ({ + return rows.map((row, index) => ({ ...batchCtx, event, input: payload ? { id: (row as { id?: unknown }).id, data: { ...payload }, options } : { id: (row as { id?: unknown }).id, options }, + // [#6966] `mode` and `scope` ride over from the batch context; only the + // position differs. Spreading `batchCtx` would carry index 0 onto every + // row, which is the one member a per-row consumer keys "do this once" on. + dispatch: { ...(batchCtx.dispatch as object), index } as HookContext['dispatch'], previous: coerceBooleanFields(schema as any, row as any), result: payload ? coerceBooleanFields(schema as any, { ...row, ...payload } as any) @@ -1811,12 +1815,19 @@ export class ObjectQL implements IObjectQLEngine { ): Promise { const schema = this._registry.getObject(object); const carriesPayload = event === 'beforeUpdate'; - for (const row of rows) { + for (let index = 0; index < rows.length; index++) { + const row = rows[index]; const rowId = (row as { id?: unknown }).id; const options = (batchCtx.input as { options?: unknown }).options; const rowCtx = { ...batchCtx, event, + // [#6966] See `buildPerRowAfterContexts` — same rule. The `scope` + // identity carried over from `batchCtx` is what lets a `before*` + // handler leave something an `after*` handler can still find: a per-row + // context is a fresh object, so a stash written on the context itself + // dies with the row that held it. + dispatch: { ...(batchCtx.dispatch as object), index } as HookContext['dispatch'], // D3: THE payload, read fresh so a previous row's REPLACEMENT is what // this row sees. Never a copy. input: carriesPayload @@ -6386,11 +6397,19 @@ export class ObjectQL implements IObjectQLEngine { // consumer built for the single shape — the flat-input proxy read // `undefined`s, declarative `condition`s evaluated against an array, // audit rows and flow-trigger contexts came out mangled (#2922). + // + // [#6966] Which is exactly why the fan-out has to be STATED rather than + // inferred: the shape is deliberately identical, so nothing about a + // context tells a handler whether it is one row of a batch. The scratch + // is created once per CALL and shared by every row's context, before and + // after — see `HookContext.dispatch`. + const insertScope: Record = {}; const rowHookContexts: HookContext[] = (isBatch ? (defaultedData as any[]) : [defaultedData]).map( - (row) => ({ + (row, rowIndex) => ({ object, event: 'beforeInsert', input: { data: row, options: opCtx.options }, + dispatch: { mode: isBatch ? 'per-row' : 'record', index: rowIndex, scope: insertScope }, session: this.buildSession(opCtx.context), provenance: this.buildProvenance(opCtx.context), user: this.buildUser(opCtx.context), @@ -6959,6 +6978,19 @@ export class ObjectQL implements IObjectQLEngine { throw new Error(ENGINE_UPDATE_REJECT_MESSAGE); } + // [#6966] The ladder verdict, stated on the contract. Bound HERE and + // nowhere else: this is the one point that knows which branch the write + // takes, and re-deriving it downstream is what `asScalarId` stays + // unexported to prevent (#4434 / #4550). The batch context carries index + // 0; `dispatchPerRowBeforeHooks` and `buildPerRowAfterContexts` override + // only `index`, so `mode` and — load-bearing — the `scope` IDENTITY are + // shared by every dispatch of this call, in both phases. + hookContext.dispatch = { + mode: isPredicatePath ? 'per-row' : 'record', + index: 0, + scope: {}, + }; + const updateSchema = this._registry.getObject(object); // Pre-update snapshot. Exposed to hooks via `hookContext.previous` in // BOTH phases now (the HookContext contract documents `previous` for @@ -7891,6 +7923,13 @@ export class ObjectQL implements IObjectQLEngine { throw new Error(ENGINE_DELETE_REJECT_MESSAGE); } + // [#6966] See update()'s twin — same rule, same single binding point. + hookContext.dispatch = { + mode: isPredicatePath ? 'per-row' : 'record', + index: 0, + scope: {}, + }; + if (isByIdDelete) { if (wantsPreImage) { priorRecord = await readPreImage(id); diff --git a/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts b/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts index 23b2e8aa6d..4cbe42ee7f 100644 --- a/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts +++ b/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts @@ -33,6 +33,8 @@ import { RuleRegrantQueue, resolveAffectedRows, idsFromHookInput, + stashAffectedRows, + readAffectedRows, } from './bulk-recompute.js'; interface Row { [k: string]: any } @@ -182,6 +184,67 @@ function makeEngine() { ctx.event = 'afterDelete'; await engine.fire('afterDelete', object, ctx); }, + + /** + * [#6966] A predicate update run the way the engine ACTUALLY runs one + * today, which `simulateBulkUpdate` above no longer describes: since + * #5038/#5574 BOTH phases dispatch once per matched row, each on a freshly + * built single-record-shaped context carrying that row's `input.id` and the + * `dispatch` marker, with ONE `scope` object shared by all of them. + * + * The distinction is this section's whole point. `simulateBulkUpdate` + * models the pre-#5574 batch dispatch (one context, no `input.id`), so + * assertions written against it are silent about what the engine does now. + * It is kept rather than replaced because the same "one context across the + * pair" shape is exactly what the single-id path still does. + */ + async simulatePerRowUpdate(object: string, where: any, data: Row, session: any = ADMIN_SESSION) { + const t = ensure(object); + const matched = t.filter((r) => where == null || matches(r, where)).map((r) => ({ ...r })); + const scope: Record = {}; + const rowCtx = (event: string, row: Row, index: number) => ({ + object, + event, + input: { id: row.id, data, options: { where, multi: true } }, + previous: { ...row }, + dispatch: { mode: 'per-row' as const, index, scope }, + session, + }); + for (let i = 0; i < matched.length; i++) { + await engine.fire('beforeUpdate', object, rowCtx('beforeUpdate', matched[i], i)); + } + for (let i = 0; i < t.length; i++) { + if (where != null && !matches(t[i], where)) continue; + t[i] = { ...t[i], ...data }; + } + for (let i = 0; i < matched.length; i++) { + await engine.fire('afterUpdate', object, rowCtx('afterUpdate', matched[i], i)); + } + return matched.length; + }, + + /** [#6966] The delete twin of `simulatePerRowUpdate`. */ + async simulatePerRowDelete(object: string, where: any, session: any = ADMIN_SESSION) { + const t = ensure(object); + const doomed = t.filter((r) => matches(r, where)).map((r) => ({ ...r })); + const scope: Record = {}; + const rowCtx = (event: string, row: Row, index: number) => ({ + object, + event, + input: { id: row.id, options: { where, multi: true } }, + previous: { ...row }, + dispatch: { mode: 'per-row' as const, index, scope }, + session, + }); + for (let i = 0; i < doomed.length; i++) { + await engine.fire('beforeDelete', object, rowCtx('beforeDelete', doomed[i], i)); + } + for (let i = t.length - 1; i >= 0; i--) if (matches(t[i], where)) t.splice(i, 1); + for (let i = 0; i < doomed.length; i++) { + await engine.fire('afterDelete', object, rowCtx('afterDelete', doomed[i], i)); + } + return doomed.length; + }, }; return engine; } @@ -377,6 +440,85 @@ describe('#4779 predicate (multi) writes recompute sharing rules', () => { }); }); + /** + * [#6966] The same contract, over the dispatch the engine actually performs. + * + * Everything above drives `simulateBulkUpdate`, which models the pre-#5574 + * batch dispatch: ONE context, no `input.id`, reused across the pair. Since + * #5574 a predicate write dispatches `before*` per matched row on a FRESHLY + * BUILT context, so the stash `stashAffectedRows` wrote never reached the + * `after` phase. `readAffectedRows` then answered `resolve-failed` and every + * bulk write took the unbounded branch — a full revoke plus a queued + * re-grant, once per matched row. Safe, but not what any of these tests say. + * + * Revert-proof: park the stash back on the context (or read it from there) + * and the bounded cases below fall into the unbounded branch — `_deleteCalls` + * grows a set-based revoke per row where these expect none. + */ + describe('[#6966] the real per-row dispatch takes the same bounded path', () => { + it('a per-row bulk update revokes exactly the rows that left the criteria', async () => { + seed(2, 'east'); + await rules.evaluateRule('srule_east', SYS); + expect(ruleShares(engine)).toHaveLength(2); + + await engine.simulatePerRowUpdate('opportunity', { region: 'east' }, { region: 'west' }); + + expect(ruleShares(engine)).toEqual([]); + // BOUNDED: the row set was known, so no object-wide revoke was needed. + // Before this fix there was one per matched row. + expect(engine._deleteCalls.filter((c) => c.object === 'sys_record_share' && c.options?.multi)) + .toHaveLength(0); + }); + + it('a per-row bulk update INTO the criteria grants every matched row, not just the first', async () => { + // The failure this guards against is subtler than "no stash": resolving + // once and reusing the answer would freeze the batch to row 0's id, + // because `resolveAffectedRows` short-circuits on a bound `input.id`. + seed(3, 'west'); + await rules.evaluateRule('srule_east', SYS); + expect(ruleShares(engine)).toEqual([]); + + await engine.simulatePerRowUpdate('opportunity', { region: 'west' }, { region: 'east' }); + + expect(ruleShares(engine).map((s) => s.record_id).sort()).toEqual(['opp0', 'opp1', 'opp2']); + }); + + it('a per-row bulk delete revokes every deleted row\'s grants', async () => { + seed(3, 'east'); + await rules.evaluateRule('srule_east', SYS); + expect(ruleShares(engine)).toHaveLength(3); + + engine._deleteCalls.length = 0; + await engine.simulatePerRowDelete('opportunity', { region: 'east' }); + + expect(ruleShares(engine)).toEqual([]); + // ONE revoke for the write, scoped to the deleted ids — not one per row, + // and not the object-wide `unbounded` revoke the missing stash produced. + const revokes = engine._deleteCalls.filter((c) => c.object === 'sys_record_share'); + expect(revokes).toHaveLength(1); + expect(revokes[0].options).toMatchObject({ + where: { source: 'rule', object_name: 'opportunity', record_id: { $in: ['opp0', 'opp1', 'opp2'] } }, + multi: true, + }); + }); + + it('the union is still capped — over the cap falls back to the unbounded branch', async () => { + // The engine's own per-row ceiling is higher than this module's, so a + // batch it lets through can still be more than a per-row recompute will + // take. That verdict must stay `over-cap`, not become "N rows, fine". + seed(RULE_RECOMPUTE_ROW_CAP + 1, 'east'); + await engine.simulatePerRowUpdate('opportunity', { region: 'east' }, { stage: 'won' }); + + const revokes = engine._deleteCalls.filter((c) => c.object === 'sys_record_share'); + expect(revokes).toHaveLength(1); + expect(revokes[0].options).toMatchObject({ + where: { source: 'rule', object_name: 'opportunity' }, + multi: true, + }); + await ruleRegrantQueue.whenIdle(); + }); + }); + describe('afterDelete — the orphaned-grant tail of the issue', () => { it('revokes the deleted records grants (nothing can ever reconcile them)', async () => { seed(3, 'east'); @@ -469,6 +611,58 @@ describe('resolveAffectedRows', () => { }); }); +describe('[#6966] readAffectedRows on a per-row dispatch', () => { + /** One shared scope, as the engine builds it for a single write. */ + const perRow = (scope: Record, id: unknown, index: number) => ({ + object: 'opportunity', + event: 'beforeUpdate', + input: { id, options: { where: { region: 'east' }, multi: true } }, + dispatch: { mode: 'per-row' as const, index, scope }, + }); + + it('unions the rows the engine hands over, without re-querying the predicate', async () => { + const scope: Record = {}; + const engine = { find: vi.fn(async () => []) }; + for (const [i, id] of ['opp0', 'opp1', 'opp2'].entries()) { + await stashAffectedRows(engine as any, 'opportunity', perRow(scope, id, i)); + } + expect(readAffectedRows(perRow(scope, 'opp0', 0))).toEqual({ kind: 'rows', ids: ['opp0', 'opp1', 'opp2'] }); + // The engine already matched the rows — asking again would be a second scan + // AND a different question, since the write may already have landed. + expect(engine.find).not.toHaveBeenCalled(); + }); + + it('deduplicates across subscribers — two packages stash on every row', async () => { + // `rule-hooks` and `record-share-cascade` both register this on the same + // write, so every row is offered twice. Appending would double every id. + const scope: Record = {}; + const engine = { find: vi.fn(async () => []) }; + for (const [i, id] of ['opp0', 'opp1'].entries()) { + await stashAffectedRows(engine as any, 'opportunity', perRow(scope, id, i)); + await stashAffectedRows(engine as any, 'opportunity', perRow(scope, id, i)); + } + expect(readAffectedRows(perRow(scope, 'opp0', 0))).toEqual({ kind: 'rows', ids: ['opp0', 'opp1'] }); + }); + + it('reads UNBOUNDED, not "no rows", when a per-row dispatch bound no ids at all', async () => { + // "We do not know" must never degrade into "nothing changed" — that is the + // direction that silently skips cleanup (#4757). An accumulator that exists + // but is empty means every id handed over was null, which is the former. + const scope: Record = {}; + await stashAffectedRows({ find: async () => [] } as any, 'opportunity', perRow(scope, null, 0)); + expect(readAffectedRows(perRow(scope, null, 0))).toMatchObject({ kind: 'unbounded' }); + }); + + it('applies the cap to the UNION, not to any one row', async () => { + const scope: Record = {}; + const engine = { find: vi.fn(async () => []) }; + for (let i = 0; i <= RULE_RECOMPUTE_ROW_CAP; i++) { + await stashAffectedRows(engine as any, 'opportunity', perRow(scope, `opp${i}`, i)); + } + expect(readAffectedRows(perRow(scope, 'opp0', 0))).toEqual({ kind: 'unbounded', reason: 'over-cap' }); + }); +}); + describe('idsFromHookInput', () => { it('accepts the shapes that name primary keys', () => { expect(idsFromHookInput('a')).toEqual(['a']); diff --git a/packages/plugins/plugin-sharing/src/bulk-recompute.ts b/packages/plugins/plugin-sharing/src/bulk-recompute.ts index 714cc49831..f1adf3a6fb 100644 --- a/packages/plugins/plugin-sharing/src/bulk-recompute.ts +++ b/packages/plugins/plugin-sharing/src/bulk-recompute.ts @@ -13,7 +13,12 @@ * if (!id) return; // ← every predicate write, silently * ``` * - * and `ObjectQL.update()` only populates `input.id` for a scalar `where.id`. + * and `ObjectQL.update()` — AT THE TIME — only populated `input.id` for a + * scalar `where.id`. ([#6966] That is no longer how a predicate write is + * dispatched: since #5038/#5574 both phases fire once per matched row with + * `input.id` bound, so the guard above would now pass per row rather than skip. + * The defect it caused is unchanged history; the mechanism sentence is not + * current, and is kept only to explain what was fixed.) * A predicate (`multi: true`) update therefore recomputed NOTHING: an admin * could bulk-move a thousand records out of a sharing rule's criteria and * every `sys_record_share` row the rule had issued stayed in the table, @@ -202,9 +207,6 @@ export async function resolveAffectedRows( * a rule's criteria makes them unfindable by the write's own predicate the * instant it lands, and a delete removes them outright — so `afterUpdate` / * `afterDelete` are structurally too late to ask "which rows was this?". - * `ObjectQL.update()` / `.delete()` reuse ONE `HookContext` instance across - * each before/after pair (they mutate `ctx.event` in place), which is the same - * seam `primary-bu-projection.ts`'s `__primaryBuUserId` rides on. * * [#5103] Lives HERE, next to the resolver, rather than in `rule-hooks.ts` * where it started: two independent hook packages now need the same answer for @@ -212,9 +214,49 @@ export async function resolveAffectedRows( * and each resolving it separately would double the predicate query on every * bulk write for no gain — the row set is a property of the WRITE, not of * either subscriber. + * + * ## [#6966] Where the stash actually lives, and why that had to change + * + * This used to say: "`ObjectQL.update()` / `.delete()` reuse ONE `HookContext` + * instance across each before/after pair (they mutate `ctx.event` in place)." + * That stopped being true for a predicate (`multi: true`) write when #5574 + * made the `before*` phase dispatch PER ROW — each row gets a freshly built + * context, so a stash written on it died with that row and the `after*` phase + * found nothing. `readAffectedRows` then answered `unbounded/resolve-failed`, + * and both subscribers took their safe branch: every bulk update or delete on + * a ruled object revoked ALL of that object's rule grants and queued a full + * asynchronous re-grant — once per matched row. Safe (the ruling's + * "over-granting is an incident, under-granting is a wobble" direction), but + * a large and silent behaviour change nobody asked for. + * + * The seam is now the engine's own: `HookContext.dispatch.scope` is one object + * shared by every dispatch of one caller write, across both phases. See + * {@link stashHost} for the fallback that keeps hand-built contexts working. */ export const AFFECTED_ROWS_STASH_KEY = '__sharingAffectedRows'; +/** + * [#6966] Accumulator for the per-row dispatch: the ids handed over one at a + * time by the engine's per-row `before*` fan-out. A SET, not an array, because + * two independent subscribers ({@link stashAffectedRows} is registered by both + * `rule-hooks` and `record-share-cascade`) each run on every row — appending + * would double every id. + */ +const AFFECTED_ROW_IDS_KEY = '__sharingAffectedRowIds'; + +/** + * Where a before→after stash for ONE write lives. + * + * The engine's per-write scratch when the marker is present; the context + * itself otherwise. The fallback is not legacy tolerance — a `HookContext` + * built by hand (tests, an embedding that calls a handler directly) carries no + * `dispatch`, and for a single-id write the context IS shared across the pair, + * so stashing on it remains correct there. + */ +function stashHost(hookCtx: any): any { + return hookCtx?.dispatch?.scope ?? hookCtx; +} + /** * Resolve (or reuse) the row set a `before` hook's write is about to change and * park it on the shared `HookContext`. @@ -227,6 +269,16 @@ export const AFFECTED_ROWS_STASH_KEY = '__sharingAffectedRows'; * Never throws: `resolveAffectedRows` already fails safe to `unbounded`, and * this adds the belt for a genuinely unexpected throw. "Unknown" must never * degrade to "no rows" — that is the direction that silently skips cleanup. + * + * ## [#6966] The per-row dispatch does not resolve at all — it accumulates + * + * On a predicate write the engine has ALREADY matched the rows and dispatches + * this hook once per row with `input.id` bound, so re-asking the predicate + * would be both wasteful and wrong: `resolveAffectedRows` short-circuits on a + * bound `input.id` (step 1), so the "reuse" branch above would have frozen the + * whole batch's answer to the FIRST row's id — under-revoking every other row, + * which is the fail-OPEN direction this module exists to avoid. Rows are + * therefore unioned as they arrive, and the cap is applied to the union. */ export async function stashAffectedRows( engine: RecomputeEngine | { find?: RecomputeEngine['find'] }, @@ -234,7 +286,14 @@ export async function stashAffectedRows( hookCtx: any, logger?: MinimalLogger, ): Promise { - const already = hookCtx?.[AFFECTED_ROWS_STASH_KEY] as AffectedRows | undefined; + const host = stashHost(hookCtx); + if (hookCtx?.dispatch?.mode === 'per-row') { + const rowId = hookCtx?.input?.id; + const seen: Set = host[AFFECTED_ROW_IDS_KEY] ?? (host[AFFECTED_ROW_IDS_KEY] = new Set()); + if (rowId != null) seen.add(String(rowId)); + return readAffectedRows(hookCtx); + } + const already = host?.[AFFECTED_ROWS_STASH_KEY] as AffectedRows | undefined; if (already) return already; let resolved: AffectedRows; try { @@ -244,7 +303,7 @@ export async function stashAffectedRows( } catch (err: any) { resolved = { kind: 'unbounded', reason: 'resolve-failed', detail: err?.message }; } - if (hookCtx && typeof hookCtx === 'object') hookCtx[AFFECTED_ROWS_STASH_KEY] = resolved; + if (host && typeof host === 'object') host[AFFECTED_ROWS_STASH_KEY] = resolved; return resolved; } @@ -252,9 +311,31 @@ export async function stashAffectedRows( * What an `after` hook should act on. A missing stash means no `before` hook of * ours ran for this write, which is not "nothing changed" — it is "we do not * know", and it reads as `unbounded` so the caller takes its safe branch. + * + * [#6966] The per-row union is materialised HERE rather than on every `before` + * dispatch, so N rows × 2 subscribers cost N set inserts instead of 2N array + * copies. The cap is re-applied to the union: a batch the engine's own ceiling + * lets through can still exceed what a per-row recompute is willing to do, and + * that verdict must be the same `over-cap` the predicate resolver returns. */ export function readAffectedRows(hookCtx: any): AffectedRows { - return (hookCtx?.[AFFECTED_ROWS_STASH_KEY] as AffectedRows | undefined) + const host = stashHost(hookCtx); + const seen = host?.[AFFECTED_ROW_IDS_KEY] as Set | undefined; + if (seen) { + if (seen.size === 0) { + // Dispatched per row and yet nothing was learned — the accumulator only + // exists because a `before*` dispatch created it, so every id it was + // handed was null. That is "we do not know", NOT "no rows changed", and + // this module's rule is that the two must never be confused: reading it + // as an empty row set would silently skip the cleanup entirely, which is + // the direction #4757 was filed for. + return { kind: 'unbounded', reason: 'resolve-failed', detail: 'per-row dispatch bound no ids' }; + } + return seen.size > RULE_RECOMPUTE_ROW_CAP + ? { kind: 'unbounded', reason: 'over-cap' } + : { kind: 'rows', ids: [...seen] }; + } + return (host?.[AFFECTED_ROWS_STASH_KEY] as AffectedRows | undefined) ?? { kind: 'unbounded', reason: 'resolve-failed', detail: 'no before-hook stash' }; } diff --git a/packages/plugins/plugin-sharing/src/record-share-cascade.ts b/packages/plugins/plugin-sharing/src/record-share-cascade.ts index 6dc2cecc19..2d2227fbc4 100644 --- a/packages/plugins/plugin-sharing/src/record-share-cascade.ts +++ b/packages/plugins/plugin-sharing/src/record-share-cascade.ts @@ -332,6 +332,12 @@ export function bindRecordShareCascade( const objectName = String(ctx?.object ?? ''); const t = targets(objectName); if (!t.shares && !t.links) return; + // [#6966] Once per WRITE, not once per row. `readAffectedRows` returns the + // whole batch's id set and both branches below act on all of it — so a + // predicate delete's per-row `afterDelete` fan-out (#5038) would repeat the + // batch's revoke N times, and the unbounded branch's queued walk with it. + // The row set is a property of the write; so is this work. + if (ctx?.dispatch?.mode === 'per-row' && ctx.dispatch.index !== 0) return; // Belt around everything OUTSIDE the two halves (each of which has its own // `attempt`): a delete that already landed must never be failed by this // hook, whatever goes wrong in it. diff --git a/packages/plugins/plugin-sharing/src/rule-hooks.ts b/packages/plugins/plugin-sharing/src/rule-hooks.ts index 0d5c4958e8..03ff94f37d 100644 --- a/packages/plugins/plugin-sharing/src/rule-hooks.ts +++ b/packages/plugins/plugin-sharing/src/rule-hooks.ts @@ -99,8 +99,13 @@ export const ruleRegrantQueue = new RuleRegrantQueue(); * * - `afterInsert` — recompute the inserted row (unchanged behaviour). * - `beforeUpdate` / `beforeDelete` — resolve the affected row set and stash - * it on the shared `HookContext` ({@link STASH_KEY}). Must be `before`: - * the write is what makes those rows unfindable. + * it for the `after` half (`AFFECTED_ROWS_STASH_KEY`). Must be + * `before`: the write is what makes those rows unfindable. + * [#6966] The stash rides `HookContext.dispatch.scope`, the engine's + * per-write scratch — NOT the context object. A predicate write dispatches + * `before*` per row (#5574) and builds a fresh context for each, so the + * older "the engine reuses one HookContext across the pair" assumption held + * only for single-id writes and silently dropped every bulk write's stash. * - `afterUpdate` — recompute per row when the set is bounded (which grants * AND revokes, so a bulk update INTO a rule's criteria is covered as well * as one out of it); otherwise revoke the object's rule grants set-based @@ -238,6 +243,22 @@ export function bindRuleHooks( /** What the `after` hook should act on when no `before` hook ran. */ const affectedFrom = (ctx: any): AffectedRows => readAffectedRows(ctx); + /** + * [#6966] Has this write's `after` work already been done by an earlier row + * of the same fan-out? + * + * What the `after` hooks below act on is the WRITE's row set, not the row + * they happen to be dispatched for — `affectedFrom` returns the whole + * union, and both branches (per-row recompute, object-wide revoke) are + * batch-scoped. A predicate write dispatches them once per matched row, so + * running them unguarded does the batch's work N times: N identical + * object-wide revokes on the unbounded branch, and N×N `recomputeRow` calls + * on the bounded one — quadratic in the batch size, which for a write at + * the cap is a million recomputes for a thousand rows. + */ + const alreadyHandledThisWrite = (ctx: any): boolean => + ctx?.dispatch?.mode === 'per-row' && ctx.dispatch.index !== 0; + engine.registerHook('afterInsert', async (ctx: any) => { if ((ctx?.session as any)?.isSystem) { // [#6783] The skip stays exactly as it was; it just stops being silent. @@ -265,6 +286,7 @@ export function bindRuleHooks( noteSystemWriteSkipped(objectName); return; } + if (alreadyHandledThisWrite(ctx)) return; try { const affected = affectedFrom(ctx); if (affected.kind === 'rows') { @@ -287,6 +309,7 @@ export function bindRuleHooks( // writes on its own account (#5103) — and by the boot orphan sweep, so // an INFO line here would point an operator at a repair that cannot run. if ((ctx?.session as any)?.isSystem) return; + if (alreadyHandledThisWrite(ctx)) return; try { const affected = affectedFrom(ctx); if (affected.kind === 'rows') { diff --git a/packages/services/service-storage/src/file-reference-lifecycle.test.ts b/packages/services/service-storage/src/file-reference-lifecycle.test.ts index 87aefd1d14..6133188c1c 100644 --- a/packages/services/service-storage/src/file-reference-lifecycle.test.ts +++ b/packages/services/service-storage/src/file-reference-lifecycle.test.ts @@ -108,8 +108,18 @@ type Engine = ReturnType; /** Drive an engine-shaped insert: beforeInsert → driver write → afterInsert, * with the same ctx object throughout and `input.data` as the persisted row * (exactly what engine.ts hands the driver). */ +/** + * [#6966] The engine's dispatch marker as a single-record write carries it. + * A hand-built context without one already reads as "not a per-row dispatch", + * so stating it here changes no verdict — it makes the per-row driver's marker + * a visible difference rather than a hidden one. + */ +function recordDispatch(scope: Record = {}) { + return { mode: 'record' as const, index: 0, scope }; +} + async function driveInsert(engine: Engine, object: string, data: Record, id: string) { - const ctx: any = { object, event: 'beforeInsert', input: { data } }; + const ctx: any = { object, event: 'beforeInsert', input: { data }, dispatch: recordDispatch() }; await engine.trigger('beforeInsert', ctx); const row = { ...(ctx.input.data as Record), id }; (engine.tables[object] ??= []).push(row); @@ -120,7 +130,7 @@ async function driveInsert(engine: Engine, object: string, data: Record) { - const ctx: any = { object, event: 'beforeUpdate', input: { id, data } }; + const ctx: any = { object, event: 'beforeUpdate', input: { id, data }, dispatch: recordDispatch() }; await engine.trigger('beforeUpdate', ctx); const row = (engine.tables[object] ?? []).find((r) => String(r.id) === String(id)); if (row) Object.assign(row, ctx.input.data); @@ -130,22 +140,59 @@ async function driveUpdate(engine: Engine, object: string, id: string, data: Rec return row; } +/** + * Drive an engine-shaped delete. + * + * [#6966] A `where`-shaped delete is driven the way the engine has actually + * driven it since #5038/#5574 — the doomed rows are matched FIRST, then + * `beforeDelete` and `afterDelete` each fire once per matched row on a + * single-record-shaped context carrying that row's `input.id` and the + * `dispatch` marker, with one `scope` object shared by all of them. + * + * This driver used to model the pre-#5574 batch dispatch instead: ONE context + * with no `id`, carrying only `options.where`. The engine stopped producing + * that shape two releases ago, so the release-on-multi-delete case below was + * passing against a dispatch that no longer exists. + */ async function driveDelete(engine: Engine, object: string, input: any) { - const ctx: any = { object, event: 'beforeDelete', input }; - await engine.trigger('beforeDelete', ctx); const where = input?.options?.where; - if (input?.id != null) { - const ids = typeof input.id === 'object' ? input.id.$in : [input.id]; - engine.tables[object] = (engine.tables[object] ?? []).filter( - (r) => !ids.some((i: unknown) => String(i) === String(r.id)), - ); - } else if (where) { - engine.tables[object] = (engine.tables[object] ?? []).filter( - (r) => !Object.entries(where).every(([k, v]) => r[k] === v), - ); + const byId = input?.id != null; + const doomed = byId + ? (() => { + const ids = typeof input.id === 'object' ? input.id.$in : [input.id]; + return (engine.tables[object] ?? []).filter((r) => ids.some((i: unknown) => String(i) === String(r.id))); + })() + : where + ? (engine.tables[object] ?? []).filter((r) => Object.entries(where).every(([k, v]) => r[k] === v)) + : []; + + const drop = () => { + const gone = new Set(doomed.map((r) => String(r.id))); + engine.tables[object] = (engine.tables[object] ?? []).filter((r) => !gone.has(String(r.id))); + }; + + if (byId) { + // Single-id path: ONE context, reused across the pair, `mode: 'record'`. + const ctx: any = { object, event: 'beforeDelete', input, dispatch: recordDispatch() }; + await engine.trigger('beforeDelete', ctx); + drop(); + ctx.event = 'afterDelete'; + await engine.trigger('afterDelete', ctx); + return; } - ctx.event = 'afterDelete'; - await engine.trigger('afterDelete', ctx); + + // Predicate path: per-row fan-out, one shared scope, fresh context per row. + const scope: Record = {}; + const rowCtx = (event: string, row: any, index: number) => ({ + object, + event, + input: { id: row.id, options: input?.options }, + previous: row, + dispatch: { mode: 'per-row' as const, index, scope }, + }); + for (let i = 0; i < doomed.length; i++) await engine.trigger('beforeDelete', rowCtx('beforeDelete', doomed[i], i)); + drop(); + for (let i = 0; i < doomed.length; i++) await engine.trigger('afterDelete', rowCtx('afterDelete', doomed[i], i)); } function install(engine: Engine, storage: any = fakeStorage()) { @@ -282,7 +329,7 @@ describe('File Reference Ownership (ADR-0104 D3 wave 2)', () => { }); }); - it('releases via the beforeDelete stash for a where-shaped multi delete', async () => { + it('releases every row of a where-shaped multi delete, in ONE release pass', async () => { const engine = fakeEngine({ files: [ file({ ref_object: 'product', ref_id: 'p1', ref_field: 'image' }), @@ -300,6 +347,19 @@ describe('File Reference Ownership (ADR-0104 D3 wave 2)', () => { await driveDelete(engine, 'product', { options: { where: { archived: true } } }); expect(engine.tables.sys_file.every((f) => f.ref_id === null)).toBe(true); + + // [#6966] Both rows released by ONE `sys_file` lookup over an `$in`, not + // one lookup per row. The per-row `afterDelete` fan-out is what made the + // naive spelling N queries; `dispatch.index === 0` plus the ids the + // `before` phase collected onto the shared scope is what collapses it. + const ownershipReads = engine.calls.filter((c) => c.op === 'find' && c.object === 'sys_file'); + expect(ownershipReads).toHaveLength(1); + expect(ownershipReads[0].arg).toMatchObject({ ref_id: { $in: ['p1', 'p2'] } }); + + // And nothing re-queried the deleted object to learn its ids: the engine + // already handed them over row by row. (The pre-#6966 hook ran one + // `engine.find(object, { where })` here.) + expect(engine.calls.filter((c) => c.op === 'find' && c.object === 'product')).toHaveLength(0); }); it('releases the old file and claims the new one when a field is swapped', async () => { diff --git a/packages/services/service-storage/src/file-reference-lifecycle.ts b/packages/services/service-storage/src/file-reference-lifecycle.ts index 7ac670481f..4f56219be6 100644 --- a/packages/services/service-storage/src/file-reference-lifecycle.ts +++ b/packages/services/service-storage/src/file-reference-lifecycle.ts @@ -61,18 +61,42 @@ const PACKAGE_ID = 'com.objectstack.service.storage'; // every internal page). Inert on write contexts. const SYSTEM_CTX = { isSystem: true, [RAW_FILE_VALUES_CONTEXT_KEY]: true } as const; -/** Bound on records resolved per where-shaped multi-delete — matches the - * attachment lifecycle's posture: bound one pass, converge across sweeps. */ -const MULTI_DELETE_RESOLVE_LIMIT = 1_000; - /** Bound on owned files released per record delete. */ const RELEASE_BATCH_LIMIT = 1_000; -/** Key under which beforeDelete stashes the ids of records about to die (the - * engine passes the SAME HookContext object to before/after delete). Only - * needed for where-shaped deletes, where afterDelete has no id list. */ +/** + * Key under which `beforeDelete` collects the ids of the records about to die, + * for `afterDelete` to release in ONE pass. + * + * [#6966] It lives on `HookContext.dispatch.scope` — the engine's per-write + * scratch, one object shared by every dispatch of one caller write across both + * phases — NOT on the context. This comment used to say "the engine passes the + * SAME HookContext object to before/after delete"; that has been false for a + * predicate (`multi: true`) delete since #5574 made the `before*` phase + * dispatch per row, each row on a freshly built context. The stash died with + * the row that wrote it, and the guard that decided whether to write one had + * inverted as well (see the hook below), so this whole path was dead code. + * + * Only needed for predicate deletes: a single-id delete's `afterDelete` reads + * `input.id` directly. + */ const STASH_KEY = '__fileRefDeletedIds'; +/** + * [#6966] Is this dispatch one row of a predicate write's fan-out? + * + * Asked of the ENGINE's marker, never inferred from `input.id`. Before #5574 a + * bulk write dispatched `before*` once with `input.id` present-but-`undefined`, + * so "no id" was a reliable bulk signal and the guards here were written on it; + * every per-row context now carries an id, so that inference answers "single + * write" for every row of a batch. An ABSENT marker reads as `false`, which is + * the single-write direction these guards already default to. + */ +function perRowDispatch(ctx: any): { index: number; scope: Record } | null { + const d = ctx?.dispatch; + return d?.mode === 'per-row' && d.scope ? { index: d.index, scope: d.scope } : null; +} + /** Engine surface these installers need — duck-typed like the other * service-storage seams so tests can fake it. */ export interface FileReferenceEngine { @@ -555,10 +579,22 @@ export function installFileReferenceHooks( if (!object || !data || typeof data !== 'object') return; const fileFields = activeFileFields(engine, object); if (fileFields.length === 0) return; - const ids = asIdList(ctx?.input?.id); // A multi-update writing a file id would give the same id to every // matched record; only a single-record update can own one. Copy-on-claim // needs a definite owner, so skip — `claimFile` then refuses to steal. + // + // [#6966] "Is this a multi-update?" is the engine's marker, not the shape + // of `input.id`. A per-row context names ITS row, so the old + // `ids.length === 1` test answered "single update" on every row of a + // batch — and then ran N times over ONE batch-scoped payload, aiming a + // row-conditioned rewrite at a shared SET clause, which ADR-0058 + // Addendum II D3 names as out of contract. Both halves are fixed by + // asking the right question once: rows after the first have nothing left + // to do, because the payload they would inspect is the same object the + // first row already reconciled. + const perRow = perRowDispatch(ctx); + if (perRow && perRow.index !== 0) return; + const ids = perRow ? null : asIdList(ctx?.input?.id); const recordId = ids && ids.length === 1 ? String(ids[0]) : null; await applyCopyOnClaim(engine, getStorage, logger, object, recordId, data, fileFields); }, @@ -602,7 +638,24 @@ export function installFileReferenceHooks( const fileFields = activeFileFields(engine, object); if (fileFields.length === 0) return; const ids = asIdList(ctx?.input?.id); - if (!ids || ids.length !== 1) return; // see beforeUpdate — no definite owner + // [#6966] Deliberately NOT switched to the dispatch marker, unlike its + // `beforeUpdate` twin. This test no longer means what its old comment + // ("see beforeUpdate — no definite owner") said — since #5038 a predicate + // write's `afterUpdate` fires per row with one bound id, so this passes + // per row instead of skipping — but the reconciliation it now performs is + // the SAFER of the two outcomes, and restoring the skip would regress: + // the first row claims the copy `beforeUpdate` made, later rows leave it + // alone (`claimFile` never steals, it warns), whereas skipping would + // leave that copy owned by NOBODY while N records still reference it — + // exactly the unreferenced-but-referenced state the sweep may collect. + // + // What is genuinely wrong here is older and wider than this card: a + // predicate update writing a file field gives N records one file id under + // an EXCLUSIVE-ownership model, so N−1 of them end up referencing bytes + // they do not own whichever branch is taken. That is a service-storage + // defect in its own right — filed as #7102 rather than smuggled into a + // dispatch-contract change. + if (!ids || ids.length !== 1) return; const recordId = String(ids[0]); for (const field of fileFields) { @@ -630,32 +683,33 @@ export function installFileReferenceHooks( { packageId: PACKAGE_ID }, ); - // ── beforeDelete: resolve ids for where-shaped deletes ──────────── - // A delete keyed by id needs nothing here; a `where` delete has no id list - // in afterDelete, and by then the records are gone. + // ── beforeDelete: collect the batch's ids for a predicate delete ── + // + // A delete keyed by id needs nothing here — `afterDelete` reads `input.id`. + // + // [#6966] A predicate delete used to need a QUERY here: the batch dispatched + // once with no id, so the only way to learn which rows were about to die was + // to re-run the caller's `where` before the write landed. It does not any + // more. The engine has already matched the doomed set and hands it over one + // row at a time (#5574), so this collects what it is given and issues no + // query at all. + // + // What was here before was dead on every path: its guard was + // `if (asIdList(ctx?.input?.id)) return;`, and a per-row context HAS an id, + // so it returned before doing anything — and had it not, the stash it wrote + // went onto a per-row context the `after` phase never sees. engine.registerHook( 'beforeDelete', async (ctx: any) => { const object: string = ctx?.object; if (!object) return; if (activeFileFields(engine, object).length === 0) return; - if (asIdList(ctx?.input?.id)) return; // afterDelete can read it directly - const where = ctx?.input?.options?.where; - if (!where) return; - try { - const rows = await engine.find(object, { - where, - limit: MULTI_DELETE_RESOLVE_LIMIT, - context: { ...SYSTEM_CTX }, - }); - ctx[STASH_KEY] = (rows ?? []).map((r) => r?.id).filter((id) => id != null); - } catch (err) { - logger.warn( - `[storage] file reference: failed to resolve ids before a where-delete on ${object} ` + - `(${(err as Error)?.message ?? err})`, - ); - ctx[STASH_KEY] = []; - } + const perRow = perRowDispatch(ctx); + if (!perRow) return; // single-id delete — afterDelete reads it directly + const rowIds = asIdList(ctx?.input?.id); + if (!rowIds) return; + const collected = (perRow.scope[STASH_KEY] ??= []) as Array; + for (const rowId of rowIds) collected.push(rowId); }, { packageId: PACKAGE_ID }, ); @@ -669,8 +723,22 @@ export function installFileReferenceHooks( const object: string = ctx?.object; if (!object) return; if (activeFileFields(engine, object).length === 0) return; - const stashed = Array.isArray(ctx?.[STASH_KEY]) ? ctx[STASH_KEY] : null; - const ids = stashed ?? asIdList(ctx?.input?.id); + // [#6966] One release pass per WRITE, not per row. A predicate delete + // fires this per matched row (#5038); releasing on each of them meant N + // `sys_file` queries of one id apiece, where the whole batch fits in one + // `$in`. The first row's dispatch does the work for all of them, because + // by then `beforeDelete` has collected every id onto the shared scope. + // + // The fallback is not decoration: it covers a per-row dispatch whose + // `beforeDelete` never ran (this object gained file fields between the + // two phases is impossible, but a directly-invoked handler in a test has + // no `before` half) and it keeps the answer per row rather than empty. + const perRow = perRowDispatch(ctx); + if (perRow && perRow.index !== 0) return; + const collected = perRow && Array.isArray(perRow.scope[STASH_KEY]) + ? (perRow.scope[STASH_KEY] as Array) + : null; + const ids = (collected?.length ? collected : null) ?? asIdList(ctx?.input?.id); if (!ids || ids.length === 0) return; try { const owned = await engine.find('sys_file', { diff --git a/packages/spec/api-surface/data.json b/packages/spec/api-surface/data.json index f8ee2834ca..490530976e 100644 --- a/packages/spec/api-surface/data.json +++ b/packages/spec/api-surface/data.json @@ -297,6 +297,7 @@ "HookBodySchema (const)", "HookContext (type)", "HookContextSchema (const)", + "HookDispatch (type)", "HookEvent (const)", "HookEventType (type)", "HookParsed (type)", diff --git a/packages/spec/authorable-surface/data.json b/packages/spec/authorable-surface/data.json index c9e4f67c45..d0eaca5dd5 100644 --- a/packages/spec/authorable-surface/data.json +++ b/packages/spec/authorable-surface/data.json @@ -456,6 +456,7 @@ "data/FullTextSearch:operator", "data/FullTextSearch:query", "data/HookContext:api", + "data/HookContext:dispatch", "data/HookContext:event", "data/HookContext:id", "data/HookContext:input", diff --git a/packages/spec/export-origins/data.json b/packages/spec/export-origins/data.json index 7c019bfd9b..19082dbd30 100644 --- a/packages/spec/export-origins/data.json +++ b/packages/spec/export-origins/data.json @@ -297,6 +297,7 @@ "HookBodySchema": "src/data/hook-body.zod.ts#HookBodySchema (const)", "HookContext": "src/data/hook.zod.ts#HookContext (type)", "HookContextSchema": "src/data/hook.zod.ts#HookContextSchema (const)", + "HookDispatch": "src/data/hook.zod.ts#HookDispatch (type)", "HookEvent": "src/data/hook.zod.ts#HookEvent (const)", "HookEventType": "src/data/hook.zod.ts#HookEventType (type)", "HookParsed": "src/data/hook.zod.ts#HookParsed (type)", diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index ec25ccc901..dd826cbca4 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -421,6 +421,74 @@ export const HookContextSchema = lazySchema(() => z.object({ */ previous: z.record(z.string(), z.unknown()).optional().describe('Record state before operation'), + /** + * Dispatch Marker + * How THIS hook call relates to the caller's write — and the one place a + * handler may keep state that survives from the `before*` phase to the + * `after*` phase of the same write. + * + * ## Why it exists (#6966) + * + * Since #5038 (`after*`) and #5574 (`before*`) a predicate (`multi: true`) + * write dispatches ONCE PER MATCHED ROW, on a context deliberately + * indistinguishable from the single-id shape — `input.id` names the row, + * `previous` is its pre-image. That indistinguishability is the feature: a + * handler written for one record needs no bulk-aware branch. + * + * It also erased the only signal several handlers had. Before #5574, + * "`input.id` is empty" meant "this one call stands for N rows", and guards + * across the platform were written on it. Every one of them silently + * inverted: a per-row context has an id, so the guard now answers "single + * write" for every row of a bulk write. This key restores the question as a + * FACT the engine states, rather than an inference from the shape of + * another key. + * + * ## Why the engine has to state it + * + * A handler cannot rebuild the answer. The verdict is the engine's dispatch + * ladder (`isByIdWrite` / `isPredicatePath`), which also consults driver + * capability (`updateMany`/`deleteMany` presence) — and `asScalarId` is + * deliberately unexported to stop the plugin side from re-deriving it from + * `options.multi` (#4434 / #4550). So the marker is produced where the + * verdict is made, once, and never re-derived. + * + * ## Not the retired `isPredicateBulkWrite` (#5574, ADR-0049) + * + * A bulk discriminator was removed from `hook-wrappers.ts` because it had + * neither a producer nor a reachable consumer: its whole test was "no + * `input.id` and `options.multi`", which answered `false` everywhere once + * every dispatch carried an id. This key is the opposite on both counts and + * must stay that way — it is bound by the engine at EVERY write dispatch + * site (insert, update, delete; both phases), and the platform's own + * handlers read it. If it ever loses its readers, retire it; do not leave it + * declared. + * + * - `mode` — `'record'` when this call is the caller's whole write (a + * single-id update/delete, a non-batch insert); `'per-row'` when it is + * one of N dispatches for one caller write. + * - `index` — 0-based position within that fan-out; always 0 when + * `mode` is `'record'`. `index === 0` is how a handler does batch-scoped + * work exactly once instead of N times. + * - `scope` — scratch shared by EVERY dispatch of one caller write, both + * phases, same object identity. Handlers used to stash on the context + * itself, which worked only because a single-id write reuses one + * `HookContext` across its before/after pair; a per-row dispatch builds a + * fresh context per row, so those stashes silently stopped arriving + * (#6966). This is that seam, named and contractual. + * + * OPTIONAL for the same reason `api` is: making it required would start + * rejecting the partial contexts `HookContextSchema.parse` accepts today. + * Read it as `ctx.dispatch?.mode === 'per-row'` — an ABSENT marker reads as + * "not a per-row dispatch", which is the back-compatible direction. + * + * Reads (`beforeFind`/`afterFind`) carry no marker: a read has no fan-out. + */ + dispatch: z.object({ + mode: z.enum(['record', 'per-row']).describe("'record' = this call is the caller's whole write; 'per-row' = one of N dispatches for one write"), + index: z.number().int().nonnegative().describe('0-based position in the per-row fan-out; always 0 when mode is "record"'), + scope: z.record(z.string(), z.unknown()).describe('Scratch shared by every dispatch of one caller write, across both phases (same object identity)'), + }).optional().describe('How this hook call relates to the caller\'s write (engine-produced; #6966)'), + /** * Execution Session * Contains authentication and organization/tenancy information. @@ -719,6 +787,12 @@ export type HookParsed = z.infer; export type ResolvedHook = z.output; export type HookEventType = z.input; export type HookContext = z.input; +/** + * The engine's dispatch marker (#6966) — see `HookContext.dispatch`. Named so + * a handler can bind it once (`const d = ctx.dispatch;`) and narrow, instead of + * spelling `NonNullable` at every read site. + */ +export type HookDispatch = NonNullable; /** * Type-safe factory for a lifecycle hook. Validates at authoring time via