diff --git a/.changeset/22782-objectql-after-hook-abort-rolls-back.md b/.changeset/22782-objectql-after-hook-abort-rolls-back.md new file mode 100644 index 00000000000..f02b523a08c --- /dev/null +++ b/.changeset/22782-objectql-after-hook-abort-rolls-back.md @@ -0,0 +1,19 @@ +--- +'@objectstack/objectql': minor +--- + +An `after*` hook whose `onError` is `abort` (the default) now rolls its write back on the default write door, as `HookSchema.onError` says ("abort: Rollback transaction (if blocking)") + +Clause-②: yes (narrowing) + + + +**BREAKING** stored-outcome narrowing, shipped as `minor` under the repo's launch-window convention for breaking changes (ADR-0087, the ratified pre-launch launch-window exemption, as amended 2026-09-13, #18003). + +- **FROM:** `insert` / `update` / `delete` (REST `createData` / `updateData` / `deleteData`, `ctx.api`, the engine door) on an object with a blocking (not `async`) `after*` hook bound from metadata whose `onError` is `abort` or omitted. When that hook threw, the caller was refused, but the row stayed stored, changed or deleted, and so did every write the hook chain had already made through `ctx.api`. A client that retried the refused create made a duplicate. +- **TO:** that write runs inside a unit of work, so the abort rolls back the row and the hook chain's engine writes together. The caller is refused exactly as before. Hook count and order are unchanged. +- **Unchanged:** a write already inside a unit (a caller's `transaction()`, an atomic batch or cascade) joins it, and that unit's owner still decides. No unit is opened for an `onError: 'log'` hook, an `async: true` hook, a hook registered in code with `registerHook` (no declared `onError`), a write with `skipAutomations`, an object whose datasource is not the default one, or a default driver with no transactions (ADR-0119 D1, such as a libSQL remote transport). On those the row stays stored after an abort, as before. +- **Inside the new unit, the rules of any unit apply.** An `after*` hook on such an object that writes a business row to an object on another datasource is now refused with `CrossDatasourceTransactionWriteError`, which the abort turns into a refused write. A MongoDB default store needs a replica set for this unit, as it does for `transaction()` already. If either applies to you, mark the hook `onError: 'log'` or `async: true`, or keep the objects on one datasource. +- **A sandboxed hook body's own `ctx.api.transaction`** on such an object, when no transaction is open around the write, used to open and own its transaction, so a throw from it rolled back only the body's own writes and the triggering row stayed stored. It now joins the door's unit, as it joins any open unit, and the abort rolls back the triggering row too. +- **Work a unit starts and does not await** (an `async: true` hook) no longer inherits the unit's finished transaction handle. It used to fail with "Transaction query already complete" and lose its write. It now runs on its own connection, as it would outside any unit. +- **Cost:** one transaction per write on the objects above only. Measured on the in-process SQLite driver: about 1.36x the per-insert time of the same object with an `onError: 'log'` hook. Objects without such a hook pay one map lookup. diff --git a/.changeset/22782-spec-hook-onerror-abort-text.md b/.changeset/22782-spec-hook-onerror-abort-text.md new file mode 100644 index 00000000000..c583ceb1b10 --- /dev/null +++ b/.changeset/22782-spec-hook-onerror-abort-text.md @@ -0,0 +1,12 @@ +--- +'@objectstack/spec': patch +--- + +`HookSchema.onError` and `HookEvent` now say where an `after*` hook's `abort` rolls the write back, and what it means elsewhere + +Clause-②: no + +Text only: no key, shape, default or enum changes, and every hook that parsed before parses the same. + +- **`onError`** (its TSDoc and `describe()`): `abort` refuses the operation. A blocking hook's abort rolls the write back whenever the write runs inside a unit of work. Those units are a caller's `transaction()`, an atomic batch or cascade, and the unit the write door now opens for a blocking `abort` `after*` hook on an object on the default datasource whose driver declares transactions (`@objectstack/objectql`, same release). On an object on another datasource, or a driver without transactions, an `after*` hook's abort refuses the caller while the row stays stored. +- **`HookEvent`'s JSDoc** listed three ways a write ends up inside a unit of work. It now lists four: the write door's own unit is the new one. diff --git a/content/docs/api/data-flow.mdx b/content/docs/api/data-flow.mdx index 4fd7b8427cf..a20b0fe2582 100644 --- a/content/docs/api/data-flow.mdx +++ b/content/docs/api/data-flow.mdx @@ -311,7 +311,7 @@ The lifecycle events are defined by the `HookEvent` enum (8 events). Reads fire | Hook | Phase | Can Modify? | Can Abort? | |:---|:---|:---|:---| | `beforeInsert/Update/Delete` | Pre-persist | ✅ Yes (mutate `input`) | ✅ Yes (throw with `onError: 'abort'`) | -| `afterInsert/Update/Delete` | Post-persist | ✅ Yes (mutate `result`) | ❌ No (record is saved) | +| `afterInsert/Update/Delete` | Post-persist, before the unit of work commits | ✅ Yes (mutate `result`) | ✅ Yes (a blocking hook that throws with `onError: 'abort'`, the default): the write is refused and rolled back. On an object outside the default datasource, or on a driver with no transactions, the caller is refused but the row stays stored — see [After hooks run inside the unit of work](/docs/automation/hooks#after-hooks-run-inside-the-unit-of-work) | **Side Effects:** Place side effects (emails, notifications, external API calls) in `after*` hooks. Set `async: true` so they run fire-and-forget without blocking the operation, and use `onError: 'log'` so a failing side effect doesn't roll back the write. diff --git a/content/docs/automation/hooks.mdx b/content/docs/automation/hooks.mdx index 3169f6201a2..c83fa539b83 100644 --- a/content/docs/automation/hooks.mdx +++ b/content/docs/automation/hooks.mdx @@ -292,11 +292,12 @@ has been requested and will happen unless this unit of work is undone**. enclosing transaction commits, so a later refusal in the same unit can roll the row back after your handler has already run. -Three ordinary operations put a write inside such a unit: +Four ordinary operations put a write inside such a unit: | Operation | What is inside the transaction | | :--- | :--- | -| A by-id `delete()` that cascades to dependent records | Each **cascaded child's** `afterDelete`. The parent's own `afterDelete` runs after that unit closes, so it is unaffected | +| Any write to an object with a blocking `after*` hook whose `onError` is `abort` (the default) | The write and every `after*` hook it dispatches, so that hook's abort rolls the row back. Not opened on an object outside the default datasource, or on a driver with no transactions: there the row stays stored after an abort | +| A by-id `delete()` that cascades to dependent records | Each **cascaded child's** `afterDelete`. The parent's own `afterDelete` runs after that unit closes, unless the row above opened one around the whole delete | | `batchData` / `deleteManyData` with `atomic: true` | Every member's `after*` — the batch aborts and rolls back on the first failure | | Any write you wrapped yourself in `ctx.api.transaction(...)` or `engine.transaction(...)` | Everything in the callback | diff --git a/content/docs/kernel/events.mdx b/content/docs/kernel/events.mdx index 12716d6e3bb..40ea34b09c1 100644 --- a/content/docs/kernel/events.mdx +++ b/content/docs/kernel/events.mdx @@ -112,7 +112,7 @@ engine.registerHook('beforeInsert', async (ctx) => { Hooks declared as **`Hook` metadata** additionally honour an **`onError`** policy (`'abort'` | `'log'`, default `'abort'`). It is a metadata field — `registerHook()` has no `onError` option: -- `onError: 'abort'` (default) — the error propagates, rolling back the transaction (when the hook is blocking) and cancelling the operation. +- `onError: 'abort'` (default) — the error propagates, rolling back the transaction (when the hook is blocking) and cancelling the operation. For an `after*` hook the write door opens that transaction itself when none is open, but only for an object on the default datasource whose driver declares transactions; elsewhere the operation is refused while its row stays stored. - `onError: 'log'` — the error is logged and swallowed; the operation is **not** cancelled. ### Ordering diff --git a/content/docs/references/data/hook.mdx b/content/docs/references/data/hook.mdx index 34145cfdcdf..e1a712d27ec 100644 --- a/content/docs/references/data/hook.mdx +++ b/content/docs/references/data/hook.mdx @@ -41,7 +41,7 @@ const result = HookSchema.parse(data); | **retryPolicy** | `{ maxRetries?: number; backoffMs?: number }` | optional | Retry policy for failed hook executions | | **timeoutMs** | `number` | optional | Maximum execution time in milliseconds before the hook is aborted | | **timeout** | `never` | optional | [REMOVED] `hook.timeout` was removed in @objectstack/spec 17 — its unit (milliseconds) lived only in the description, beside a body-level `timeoutMs` and a `retryPolicy.backoffMs` that spell theirs, so the same number read as two conventions on one surface. Rename the key to `timeoutMs`; the value (milliseconds) is unchanged. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | -| **onError** | `Enum<'abort' \| 'log'>` | optional (default: `"abort"`) | Error handling strategy | +| **onError** | `Enum<'abort' \| 'log'>` | optional (default: `"abort"`) | Error handling strategy. abort: refuse the operation; a blocking hook's abort rolls the write back inside a unit of work, which the write door opens for an after* hook on an object on the default datasource whose driver declares transactions (elsewhere the caller is refused and the row stays stored). log: log the error and continue | | **runAs** | `Enum<'system' \| 'user' \| 'inherit'>` | optional (default: `"inherit"`) | Execution identity for the hook's ctx.api data operations: system = elevated (bypasses RLS), user = the triggering user (RLS-respecting), inherit = the context of the write that fired the hook (the pre-runAs behaviour; the default). A hook with no trigger user has no identity to scope to, so under user its ctx.api data operations are REFUSED — declare system to make the elevation explicit. This covers any hook fired by a write that carried no user (an isSystem plugin/service write; a system-elevated flow node). Scope: ctx.api only — condition evaluation, the readonly strip on ctx.input, ctx.session and async are unchanged. | | **_lock** | `Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>` | optional | Item-level lock — controls overlay & delete (ADR-0010). | | **_lockReason** | `string` | optional | Human-readable reason shown when a write is refused by _lock. | diff --git a/packages/objectql/src/engine-after-hook-abort-unit.test.ts b/packages/objectql/src/engine-after-hook-abort-unit.test.ts new file mode 100644 index 00000000000..fdae4550fa3 --- /dev/null +++ b/packages/objectql/src/engine-after-hook-abort-unit.test.ts @@ -0,0 +1,170 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #22782 — an `after*` hook whose `onError` is `abort` (the default) rolls its + * write back, as `HookSchema.onError` says ("abort: Rollback transaction"). + * + * Before this, a plain write opened no unit of work, so a throwing + * `afterInsert` refused the caller while the row, and the `ctx.api` write an + * earlier hook made, stayed stored. The write door now opens a unit for exactly + * that case (`inAfterAbortUnit`); every other write opens none, which the + * `begun` ledger below measures. + * + * The driver rolls back for real (a snapshot per transaction): a stub whose + * `rollback()` is a no-op cannot tell "wrapped" from "not wrapped". + */ + +import { describe, it, expect } from 'vitest'; +import type { Hook } from '@objectstack/spec/data'; +import { ObjectQL } from './engine.js'; +import { bindHooksToEngine } from './hook-binder.js'; + +function makeDriver(name: string, opts: { transactional?: boolean; failParentUpdate?: boolean } = {}) { + let stores = new Map>>(); + const storeFor = (o: string) => stores.get(o) ?? (stores.set(o, new Map()), stores.get(o)!); + const matches = (row: Record, where: any) => + !where || Object.entries(where).every(([k, v]) => k.startsWith('$') || row[k] === v); + const begun: unknown[] = []; + const rolledBack: unknown[] = []; + const snapshots = new Map(); + let n = 0; + const driver: any = { + name, version: '0.0.0', supports: {}, begun, rolledBack, + rowsOf: (o: string) => Array.from(storeFor(o).values()), + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async syncSchema() {}, + async find(o: string, ast: any) { + const rows = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + return typeof ast?.limit === 'number' ? rows.slice(0, ast.limit) : rows; + }, + async findOne(o: string, ast: any) { return (await this.find(o, ast))[0] ?? null; }, + async create(o: string, data: Record) { + const row = { ...data, id: (data.id as string) ?? `${o}_${++n}` }; + storeFor(o).set(row.id, row); + return row; + }, + async update(o: string, id: string, data: Record) { + if (opts.failParentUpdate && o === 'inv') throw new Error('ETIMEDOUT'); + const row = { ...storeFor(o).get(String(id)), ...data, id }; + storeFor(o).set(String(id), row); + return row; + }, + async delete(o: string, id: string) { return storeFor(o).delete(String(id)); }, + async count(o: string, ast: any) { return (await this.find(o, ast)).length; }, + async bulkCreate(o: string, rows: Record[]) { return Promise.all(rows.map((r) => this.create(o, r))); }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + }; + if (opts.transactional !== false) { + driver.beginTransaction = async () => { + const handle = { trx: begun.length + 1 }; + snapshots.set(handle, new Map(Array.from(stores, ([o, s]) => [o, new Map(Array.from(s, ([k, v]) => [k, { ...v }]))]))); + begun.push(handle); + return handle; + }; + driver.commit = async (h: unknown) => { snapshots.delete(h); }; + driver.rollback = async (h: unknown) => { stores = snapshots.get(h) ?? stores; rolledBack.push(h); }; + } + return driver; +} + +const obj = (name: string, extra: Record = {}) => ({ + name, label: name, fields: { id: { name: 'id', type: 'text', primaryKey: true }, name: { name: 'name', type: 'text' }, ...extra }, +}); +const hook = (name: string, events: string[], handler: (ctx: any) => unknown, extra: Partial = {}) => + ({ name, object: 'case', events, handler, ...extra }) as unknown as Hook; +const boom = () => { throw new Error('refused by the hook'); }; +/** The earlier `afterInsert` the card measured: it writes a note through `ctx.api` first. */ +const noteWriter = hook('note_writer', ['afterInsert'], (ctx) => ctx.api.object('note').insert({ name: 'from-hook' }), { priority: 10 }); + +async function makeEngine(hooks: Hook[], opts: { transactional?: boolean; caseDriver?: string; failParentUpdate?: boolean } = {}) { + const warns: string[] = []; + const log = () => {}; + const engine = new ObjectQL({ logger: { debug: log, info: log, error: log, warn: (m: string) => void warns.push(String(m)) } } as any); + const driver = makeDriver('primary', opts); + engine.registerDriver(driver, true); + const side = makeDriver('side'); + engine.registerDriver(side); + await engine.init(); + engine.registry.registerObject({ ...obj('case'), ...(opts.caseDriver ? { datasource: opts.caseDriver } : {}) } as any, '__test__'); + engine.registry.registerObject(obj('note') as any, '__test__'); + bindHooksToEngine(engine, hooks, { packageId: 'app:t' }); + return { engine, driver, side, warns }; +} + +describe('#22782 — an aborting after* hook rolls its write back', () => { + it('a throwing afterInsert with the default onError: refused, the row and the chain\'s ctx.api write absent', async () => { + const { engine, driver } = await makeEngine([noteWriter, hook('veto', ['afterInsert'], boom, { priority: 20 })]); + await expect(engine.insert('case', { name: 'c1' })).rejects.toThrow('refused by the hook'); + expect(driver.rowsOf('case')).toEqual([]); + expect(driver.rowsOf('note')).toEqual([]); + expect(driver.begun).toHaveLength(1); + expect(driver.rolledBack).toHaveLength(1); + }); + + it('afterUpdate and afterDelete: the stored row is what it was before the refused write', async () => { + const { engine, driver } = await makeEngine([hook('veto', ['afterUpdate', 'afterDelete'], boom)]); + const row = await engine.insert('case', { name: 'before' }); + await expect(engine.update('case', { name: 'after' }, { where: { id: row.id } } as any)).rejects.toThrow('refused by the hook'); + await expect(engine.delete('case', { where: { id: row.id } } as any)).rejects.toThrow('refused by the hook'); + expect(driver.rowsOf('case')).toMatchObject([{ id: row.id, name: 'before' }]); + expect(driver.begun).toHaveLength(2); + }); + + it('CONTROL onError: \'log\' — the row is stored and no unit is opened', async () => { + const { engine, driver } = await makeEngine([hook('veto', ['afterInsert'], boom, { onError: 'log' })]); + await engine.insert('case', { name: 'c1' }); + expect(driver.rowsOf('case')).toMatchObject([{ name: 'c1' }]); + expect(driver.begun).toHaveLength(0); + }); + + it('CONTROL a throwing before* hook — refused, nothing stored, no unit opened', async () => { + const { engine, driver } = await makeEngine([hook('veto', ['beforeInsert'], boom)]); + await expect(engine.insert('case', { name: 'c1' })).rejects.toThrow('refused by the hook'); + expect(driver.rowsOf('case')).toEqual([]); + expect(driver.begun).toHaveLength(0); + }); + + it('CONTROL inside a caller-opened transaction() — it joins (one unit), and the caller decides', async () => { + const { engine, driver } = await makeEngine([hook('veto', ['afterInsert'], boom)]); + await engine.transaction(async (trx: any) => { + await expect(engine.insert('case', { name: 'kept' }, { context: trx })).rejects.toThrow('refused by the hook'); + }); + expect(driver.begun).toHaveLength(1); + expect(driver.rolledBack).toHaveLength(0); + expect(driver.rowsOf('case')).toMatchObject([{ name: 'kept' }]); + }); + + it('no unit for a hook that cannot abort the write: async, code-registered, or skipped automations', async () => { + const { engine, driver } = await makeEngine([hook('fire_and_forget', ['afterInsert'], () => {}, { async: true })]); + engine.registerHook('afterInsert', async () => {}, { object: 'case' }); + await engine.insert('case', { name: 'c1' }); + const vetoed = await makeEngine([hook('veto', ['afterInsert'], boom)]); + await vetoed.engine.insert('case', { name: 'c2' }, { context: { isSystem: true, skipAutomations: true } as any }); + expect(driver.begun).toHaveLength(0); + expect(vetoed.driver.begun).toHaveLength(0); + }); + + it('no unit where one cannot hold — off the default driver, or a driver with no transactions — and no warning', async () => { + const off = await makeEngine([hook('veto', ['afterInsert'], boom)], { caseDriver: 'side' }); + await expect(off.engine.insert('case', { name: 'c1' })).rejects.toThrow('refused by the hook'); + expect(off.side.rowsOf('case')).toMatchObject([{ name: 'c1' }]); + const plain = await makeEngine([hook('veto', ['afterInsert'], boom)], { transactional: false }); + await expect(plain.engine.insert('case', { name: 'c2' })).rejects.toThrow('refused by the hook'); + expect(plain.driver.rowsOf('case')).toMatchObject([{ name: 'c2' }]); + expect([...off.driver.begun, ...off.side.begun]).toHaveLength(0); + expect(plain.warns.filter((w) => w.includes('transaction'))).toEqual([]); + }); + + it('a roll-up failure inside the unit still means "the records WERE written": committed, then thrown', async () => { + const { engine, driver } = await makeEngine([], { failParentUpdate: true }); + (engine as any).summaryRetryOptions = { sleep: async () => {}, backoffBaseMs: 0 }; + engine.registry.registerObject({ name: 'inv', fields: { total: { type: 'summary', summaryOperations: { object: 'inv_line', field: 'amount', function: 'sum' } } } } as any); + engine.registry.registerObject({ name: 'inv_line', fields: { amount: { type: 'number' }, inv: { type: 'master_detail', reference: 'inv' } } } as any); + bindHooksToEngine(engine, [{ name: 'observe', object: 'inv_line', events: ['afterInsert'], handler: () => {} } as unknown as Hook], { packageId: 'app:s' }); + const parent = await engine.insert('inv', {}); + await expect(engine.insert('inv_line', { inv: parent.id, amount: 1 })).rejects.toMatchObject({ code: 'ERR_SUMMARY_RECOMPUTE' }); + expect(driver.begun).toHaveLength(1); + expect(driver.rolledBack).toHaveLength(0); + expect(driver.rowsOf('inv_line')).toHaveLength(1); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 9905baf9fb8..1fef9c8fc2a 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -400,12 +400,16 @@ export interface AdmittedValueShapeViolationTally { * push a handler's own `ctx.api` writes outside the transaction the write ran * in, and an in-engine audit hook depends on landing inside it. * - * Three ordinary paths open such a unit around the dispatch: + * Four ordinary paths open such a unit around the dispatch: + * - the write door itself, when the object has a blocking metadata-bound + * `after*` hook whose `onError` is `abort` — so that abort rolls the row + * back as `HookSchema.onError` says (#22782, `opensAfterAbortUnit`); * - a by-id {@link ObjectQL.delete} whose cascade is `'atomic'` — each * dependent's own `afterDelete` fires inside the wrap the parent opened, * and the parent's row removal can still refuse afterwards (#7413). The - * PARENT's `afterDelete` is outside that wrap by construction, so it is - * unaffected; the cascaded CHILDREN's are not; + * PARENT's `afterDelete` is outside that wrap by construction (inside the + * door's own unit when the first path opened one); the cascaded + * CHILDREN's are not; * - `runAtomicBatch` in `@objectstack/metadata-protocol` — * `batchData`/`deleteManyData` with `atomic: true` runs every member's * `after*` inside one transaction that aborts on the first failure @@ -4677,6 +4681,52 @@ export class ObjectQL implements IObjectQLEngine { return entries.some((entry) => hookMatchesObject(entry, object)); } + /** + * [#22782] Does this write need its own unit of work? Yes when its `after*` + * dispatch can ABORT and no unit is open, so the abort rolls back what + * `HookSchema.onError` says: the row and every `ctx.api` write the chain made. "Can abort" + * is a metadata-bound hook (`meta` — the only entries with a declared + * `onError`) on `event` for `object`, blocking (not `async`), `onError` + * `abort` (the default), automations not skipped. Hook count and order are + * untouched (#7413); only the boundary around them moves. + * + * No unit — the pre-#22782 path — when one is open (ambient or threaded: the + * caller's unit decides), when `object` is off the default driver + * (`transaction()` covers that one alone and would refuse the write), or when + * that driver has no transactions (ADR-0119 D1; asking would only warn). + * `SummaryRecomputeError` means "the records WERE written": it is rethrown + * after the commit, never through the rollback. + */ + private opensAfterAbortUnit(event: 'afterInsert' | 'afterUpdate' | 'afterDelete', object: string, context: ExecutionContext | undefined): boolean { + if (context?.transaction !== undefined || this.txStore.getStore()?.transaction !== undefined) return false; + if ((context as { skipAutomations?: unknown } | undefined)?.skipAutomations) return false; + const aborting = (this.hooks.get(event) ?? []).some((entry) => + entry.meta && !entry.meta.async && (entry.meta.onError ?? 'abort') === 'abort' && hookMatchesObject(entry, object)); + if (!aborting) return false; + const defaultDriver = this.defaultDriver ? this.drivers.get(this.defaultDriver) : undefined; + try { + return this.getDriver(object) === defaultDriver && driverSupportsTransactions(defaultDriver); + } catch { + return false; // the write itself reports an unroutable object + } + } + + /** [#22782] The unit {@link opensAfterAbortUnit} asked for; `write` re-enters the door, which joins it. */ + private async inAfterAbortUnit(write: () => Promise): Promise { + let written: SummaryRecomputeError | undefined; + const result = await this.transaction(async () => { + try { + return await write(); + } catch (e) { + if (!(e instanceof SummaryRecomputeError)) throw e; + written = e; + return undefined as T; + } + }); + if (written) throw written; + return result; + } + /** * [#5038] The per-row after-hook contexts a predicate (`multi: true`) write * dispatches, one per matched row. @@ -13990,6 +14040,10 @@ export class ObjectQL implements IObjectQLEngine { */ async insert(object: string, data: any | any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise { object = this.resolveObjectName(object); + // [#22782] An aborting `afterInsert` rolls this write back — see `opensAfterAbortUnit`. + if (this.opensAfterAbortUnit('afterInsert', object, options?.context)) { + return this.inAfterAbortUnit(() => this.insert(object, data, options)); + } this.logger.debug('Insert operation starting', { object, isBatch: Array.isArray(data) }); this.assertWriteAllowed(object, 'insert'); // [#8844] The organization set is about to change. Dropped BEFORE the write @@ -15165,6 +15219,10 @@ export class ObjectQL implements IObjectQLEngine { */ async update(object: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise | number | null> { object = this.resolveObjectName(object); + // [#22782] An aborting `afterUpdate` rolls this write back — see `opensAfterAbortUnit`. + if (this.opensAfterAbortUnit('afterUpdate', object, options?.context)) { + return this.inAfterAbortUnit(() => this.update(object, data, options)); + } this.logger.debug('Update operation starting', { object }); this.assertWriteAllowed(object, 'update'); // [#8844] See the insert face — an organization row that changes identity @@ -18144,6 +18202,10 @@ export class ObjectQL implements IObjectQLEngine { async delete(object: string, options?: EngineDeleteOptions): Promise { object = this.resolveObjectName(object); + // [#22782] An aborting `afterDelete` rolls this write back — see `opensAfterAbortUnit`. + if (this.opensAfterAbortUnit('afterDelete', object, options?.context)) { + return this.inAfterAbortUnit(() => this.delete(object, options)); + } this.logger.debug('Delete operation starting', { object }); this.assertWriteAllowed(object, 'delete'); // [#8844] See the insert face — deleting the install's only organization @@ -19451,13 +19513,16 @@ export class ObjectQL implements IObjectQLEngine { } const trx = await drv.beginTransaction(); const trxCtx = { ...(baseContext ?? {}), transaction: trx }; + const ambientEntry: { transaction: unknown; scope?: TransactionScope } = + { transaction: trx, scope: this.newTransactionScope(driver!) }; try { // Run the callback inside the ambient transaction store so internal // queries during writes reuse this transaction's connection (ADR-0034). - const result = await this.txStore.run( - { transaction: trx, scope: this.newTransactionScope(driver!) }, - () => callback(trxCtx, { owned: true }), - ); + // [#22782] CLOSED once the callback settles, before the commit: work it + // started and did not await (an `async: true` hook) inherits this store, + // ran on the finished handle and lost its write. Now it runs unenrolled. + const result = await this.txStore.run(ambientEntry, () => callback(trxCtx, { owned: true })) + .finally(() => { ambientEntry.transaction = undefined; ambientEntry.scope = undefined; }); if (drv.commit) await drv.commit(trx); else if (drv.commitTransaction) await drv.commitTransaction(trx); return result; diff --git a/packages/qa/dogfood/test/hook-after-abort-rollback.dogfood.test.ts b/packages/qa/dogfood/test/hook-after-abort-rollback.dogfood.test.ts new file mode 100644 index 00000000000..22991429189 --- /dev/null +++ b/packages/qa/dogfood/test/hook-after-abort-rollback.dogfood.test.ts @@ -0,0 +1,116 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #22782 — `HookSchema.onError`: "abort: Rollback transaction (if blocking)", +// default `abort`. Measured on a booted stack before the fix: an `afterInsert` +// hook that threw refused the caller while the row stayed stored, and so did +// the row an earlier `afterInsert` hook wrote through `ctx.api` — a plain write +// opened no unit of work, so the abort had nothing to roll back. A client that +// retries the refused create makes a duplicate. +// +// Pinned on the two published doors the card names: REST `POST /data/:object` +// (`createData`) and the engine door (`hooks.run`), each read back through the +// engine. The three controls are the triage's: `onError: 'log'` stores the +// row; a caller-opened `transaction()` stays the caller's to decide; a throwing +// `before*` hook refuses exactly as before. A plain `Error` from a function +// hook answers REST's sanitised `500 INTERNAL_ERROR` (its terminal branch) on +// both sides of the fix; what this file pins is what is STORED. +// +// One more case the unit made necessary: an `async: true` hook is started +// inside the unit and writes after it commits. It used to inherit the finished +// handle and lose its write ("Transaction query already complete", measured). + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { defineStack } from '@objectstack/spec'; +import { ObjectSchema, Field } from '@objectstack/spec/data'; + +const CASE = 'har_case'; +const NOTE = 'har_note'; +const LOGGED = 'har_logged'; + +const named = (name: string) => ObjectSchema.create({ + name, label: name, sharingModel: 'public_read_write', + fields: { name: Field.text({ label: 'Name', required: true }), case_name: Field.text({ label: 'Case' }) }, +}); +const veto = (prefix: string) => (ctx: any) => { + if (String(ctx.input?.data?.name ?? '').startsWith(prefix)) throw new Error(`refused: ${prefix}`); +}; + +const harStack = defineStack({ + manifest: { + id: 'com.dogfood.hook-after-abort-rollback', namespace: 'har', version: '0.0.0', type: 'app', + name: 'After-hook abort rollback fixture', description: 'An afterInsert abort, its log twin, and a beforeInsert veto.', + }, + objects: [named(CASE), named(NOTE), named(LOGGED)], + hooks: [ + // The earlier hook of the card's measurement: it writes through ctx.api first. + { name: 'har_note_writer', object: CASE, events: ['afterInsert'], priority: 10, + handler: (ctx: any) => ctx.api.object(NOTE).insert({ name: 'note', case_name: ctx.input.data.name }) }, + { name: 'har_after_veto', object: CASE, events: ['afterInsert'], priority: 20, handler: veto('veto') }, + { name: 'har_before_veto', object: CASE, events: ['beforeInsert'], handler: veto('block') }, + { name: 'har_logged_veto', object: LOGGED, events: ['afterInsert'], onError: 'log', handler: veto('veto') }, + { name: 'har_late_note', object: CASE, events: ['afterInsert'], async: true, onError: 'log', + handler: async (ctx: any) => { + await new Promise((r) => setTimeout(r, 40)); + await ctx.api.object(NOTE).insert({ name: 'late', case_name: ctx.input.data.name }); + } }, + ], +} as never); + +describe('an aborting afterInsert rolls the write back (#22782)', () => { + let stack: VerifyStack; + let token: string; + const uniq = (prefix: string) => `${prefix}-${Math.random().toString(36).slice(2, 8)}`; + const stored = async (object: string, name: string) => (await stack.rows(object, { name })).length; + const notes = async (name: string, note = 'note') => (await stack.rows(NOTE, { case_name: name, name: note })).length; + const refusal = async (res: Response) => ({ status: res.status, code: ((await res.json()) as any).code }); + + beforeAll(async () => { + stack = await bootStack(harStack); + token = await stack.signIn(); + }, 120_000); + afterAll(async () => { await stack?.stop(); }); + + it('REST createData: the refused create leaves neither the row nor the hook chain\'s ctx.api write', async () => { + const name = uniq('veto'); + expect(await refusal(await stack.apiAs(token, 'POST', `/data/${CASE}`, { name }))).toEqual({ status: 500, code: 'INTERNAL_ERROR' }); + expect(await stored(CASE, name)).toBe(0); + expect(await notes(name)).toBe(0); + }); + + it('the engine door (hooks.run): refused, and nothing stored', async () => { + const name = uniq('veto'); + await expect(stack.hooks.run(CASE, 'insert', { name }, { system: true })).rejects.toThrow('refused: veto'); + expect(await stored(CASE, name)).toBe(0); + expect(await notes(name)).toBe(0); + }); + + it('positive control: an insert no hook refuses stores the row, the note, and the async hook\'s late note', async () => { + const name = uniq('ok'); + expect((await stack.apiAs(token, 'POST', `/data/${CASE}`, { name })).status).toBe(201); + expect([await stored(CASE, name), await notes(name)]).toEqual([1, 1]); + for (let i = 0; i < 40 && (await notes(name, 'late')) === 0; i++) await new Promise((r) => setTimeout(r, 50)); + expect(await notes(name, 'late')).toBe(1); + }); + + it('CONTROL onError: \'log\' — the hook throws and the row is stored', async () => { + const name = uniq('veto'); + expect((await stack.apiAs(token, 'POST', `/data/${LOGGED}`, { name })).status).toBe(201); + expect(await stored(LOGGED, name)).toBe(1); + }); + + it('CONTROL a caller-opened transaction(): the caller that swallows the refusal and commits keeps the row', async () => { + const name = uniq('veto'); + const ql = await stack.kernel.getServiceAsync('objectql'); + await ql.transaction(async (trx: any) => { + await expect(ql.insert(CASE, { name }, { context: { ...trx, isSystem: true } })).rejects.toThrow('refused: veto'); + }); + expect([await stored(CASE, name), await notes(name)]).toEqual([1, 1]); + }); + + it('CONTROL a throwing beforeInsert: refused, nothing stored', async () => { + const name = uniq('block'); + expect(await refusal(await stack.apiAs(token, 'POST', `/data/${CASE}`, { name }))).toEqual({ status: 500, code: 'INTERNAL_ERROR' }); + expect(await stored(CASE, name)).toBe(0); + }); +}); diff --git a/packages/runtime/src/sandbox/transaction-ambient-join.integration.test.ts b/packages/runtime/src/sandbox/transaction-ambient-join.integration.test.ts index 862929daad4..fa2d3472464 100644 --- a/packages/runtime/src/sandbox/transaction-ambient-join.integration.test.ts +++ b/packages/runtime/src/sandbox/transaction-ambient-join.integration.test.ts @@ -206,28 +206,52 @@ describe('#6406 sandbox ctx.api.transaction joins the host transaction (real eng expect(committedNames('thing')).toEqual([]); }, 30000); - it('with NO host transaction the sandbox still OPENS one — unchanged behaviour', async () => { + it("with NO host transaction the door's unit owns the one begin, the sandbox body joins it, and the door commits both rows (#22782)", async () => { await wire(TX_BODY); await engine.insert('thing', { name: 'outer' }); - // The outer write is not in any transaction; the body's is, and owns it. + // Since #22782 the begin belongs to the WRITE DOOR, not the body: the hook + // declares no `onError`, so its `afterInsert` can abort and the door opens a + // unit around the outer write. The body's `ctx.api.transaction` JOINS that + // unit (#6406) and the door commits it once. Before #22782 the outer write + // was in no transaction and the body opened and owned this one. expect(seen.begins).toHaveLength(1); expect(seen.commits).toHaveLength(1); expect(seen.commits[0]).toBe(seen.begins[0]); expect(committedNames('thing')).toEqual(['inner', 'outer']); }, 30000); - it('with NO host transaction a throwing body still ROLLS BACK its own — unchanged', async () => { + /** + * #22782 (triage 6106449072): an `after*` hook whose `onError` is `abort` — the + * default, which this body's hook takes — rolls the write back on the default + * write door, as `HookSchema.onError` says. + * + * FROM: the outer write ran in no unit; the body opened and owned its own + * transaction and rolled back only `inner`, so `outer` stayed + * committed while the caller was refused. + * TO: the door opens a unit around the outer write; the body's + * `ctx.api.transaction` JOINS it, its `__txRollback` ABSTAINS for the + * joined handle and re-throws, and the abort rolls the door's unit + * back — `outer` and `inner` together. + * + * The #6406 subject is unchanged and asserted here: one begin (no second + * connection), the body's write on that same handle, and exactly one + * rollback — the owner's, never the joined body's. + */ + it("with NO host transaction the door's unit owns the begin, the body joins it, and the abort rolls the outer row back too (#22782)", async () => { await wire(THROWING_TX_BODY); await expect(engine.insert('thing', { name: 'outer' })).rejects.toThrow(/body boom/); - expect(seen.begins).toHaveLength(1); - expect(seen.rollbacks).toHaveLength(1); + expect(seen.begins).toHaveLength(1); // one begin: no second connection + expect(seen.creates).toHaveLength(2); + expect(seen.creates[0].transaction).toBe(seen.begins[0]); // the outer write is inside the door's unit + expect(seen.creates[1].transaction).toBe(seen.begins[0]); // the body's write JOINED it + expect(seen.rollbacks).toHaveLength(1); // the joined rollback abstained + expect(seen.rollbacks[0]).toBe(seen.begins[0]); // the one rollback is the owner's expect(seen.commits).toHaveLength(0); - // The body's write is discarded; the outer one was never transactional. - expect(committedNames('thing')).toEqual(['outer']); + expect(committedNames('thing')).toEqual([]); // FROM ['outer'] — the refused write left its row }, 30000); /** diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index 2ec3e511e54..321bf811dc5 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -151,7 +151,12 @@ function refuseBodyOnStoredMetadataTarget( * hook has already run. Ruled on #7477 (2026-08-11) as the declared semantics, * not an implementation detail to be re-timed later. * - * Three ordinary ways a write ends up inside such a unit: + * Four ordinary ways a write ends up inside such a unit: + * - the write door itself, when the object has a blocking (not `async`) + * `after*` hook whose `onError` is `abort` (the default) — limited to an + * object on the default datasource whose driver declares transactions, so + * that hook's abort rolls the row back (#22782); see `onError` below for + * what `abort` means where this unit is not opened; * - a by-id `delete()` whose cascade is atomic — each dependent's own * `afterDelete` runs inside the wrap the parent opened (#7413); * - a `batchData`/`deleteManyData` call with `atomic: true` — every member's @@ -419,10 +424,23 @@ export const HookSchema = lazySchema(() => strictObject( /** * Error Policy * What to do if the hook throws an exception? - * - abort: Rollback transaction (if blocking) + * - abort: refuse the operation. A blocking (not `async`) hook's abort rolls + * the write back — its row and the `ctx.api` writes the hook chain made + * through the engine — whenever the write runs inside a unit of work: a + * caller's `transaction()`, an atomic batch or cascade, or the unit the + * write door opens for a blocking `abort` `after*` hook (#22782). The door + * opens that unit only for an object on the default datasource whose + * driver declares transactions. Elsewhere — an object on another + * datasource, or a driver without transactions — an `after*` hook's abort + * refuses the caller while the row stays stored. A `before*` hook's abort + * refuses the write before it is issued, so nothing is stored either way. * - log: Log error and continue */ - onError: z.enum(['abort', 'log']).default('abort').describe('Error handling strategy'), + onError: z.enum(['abort', 'log']).default('abort').describe( + 'Error handling strategy. abort: refuse the operation; a blocking hook\'s abort rolls the write back inside a unit of work, ' + + 'which the write door opens for an after* hook on an object on the default datasource whose driver declares transactions ' + + '(elsewhere the caller is refused and the row stays stored). log: log the error and continue', + ), /** * Execution identity for the hook's `ctx.api` data operations (#14010;