From a2617e36ab65a0efc4b8e49f9b481fced0c66351 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 16:18:52 +0000 Subject: [PATCH] fix(objectql): ObjectQL.delete's by-id cascade is one unit of work (#7413) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delete()`'s by-id branch ran `cascadeDeleteRelations` and then `driver.delete` with no transaction around either, and the cascade re-enters `this.delete()` / `this.update()` per dependent row — so every child committed as it executed. A refusal partway (the engine's own `restrict` branch, a child's permission check, a later child's `beforeDelete` hook) left an arbitrary prefix of the children deleted while the caller received 409/403 and reasonably concluded nothing had happened. Children are visited in `getAllObjects()` order, so which rows were gone was arbitrary from the caller's point of view, and a partial delete has no natural undo. This applies #4620's ruled principle — atomic honoured for real, or refused — to the path that never got it. The by-id delete and its whole cascade now run inside one `engine.transaction()`; the recursion JOINS that transaction rather than nesting under it (ADR-0067 D2 / #5696), so a multi-level cascade opens exactly one driver transaction on one connection. `planCascadeAtomicity` decides WHEN to open one, from a pure registry walk: no declared dependents keeps the pre-existing path exactly (the control), and a cascade reaching an object routed off the default datasource keeps its old non-atomic answer rather than becoming a `CrossDatasourceTransactionWriteError` refusal (#5351 / #5696 point 2), warned once per object. No `require: true`: `beginTransaction` is a required member of `IDataDriver` and all five in-tree drivers implement it, so failing closed would only make plain deletes refuse on non-conforming doubles while buying nothing on real runtimes. The declared degrade (ADR-0119 D1) stands and warns once per driver (#4619). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TAeyg4nJ3yeePVmCbdfxd5 --- .changeset/delete-cascade-one-unit-of-work.md | 55 ++ .../src/engine-cascade-delete-atomic.test.ts | 513 ++++++++++++++++++ packages/objectql/src/engine.ts | 203 ++++++- 3 files changed, 769 insertions(+), 2 deletions(-) create mode 100644 .changeset/delete-cascade-one-unit-of-work.md create mode 100644 packages/objectql/src/engine-cascade-delete-atomic.test.ts diff --git a/.changeset/delete-cascade-one-unit-of-work.md b/.changeset/delete-cascade-one-unit-of-work.md new file mode 100644 index 0000000000..c28afb390e --- /dev/null +++ b/.changeset/delete-cascade-one-unit-of-work.md @@ -0,0 +1,55 @@ +--- +"@objectstack/objectql": minor +--- + +fix(objectql): `ObjectQL.delete`'s by-id cascade is one unit of work (#7413) + +`delete()`'s by-id branch ran `cascadeDeleteRelations` and then `driver.delete` +with **no transaction around either**, and the cascade re-enters +`this.delete()` / `this.update()` per dependent row — so every child committed +as it executed. A refusal partway (the engine's own `restrict` branch, a child's +permission check, a later child's `beforeDelete` hook) left an arbitrary +**prefix** of the children deleted while the caller received 409/403 and +reasonably concluded nothing had happened. Children are visited in +`getAllObjects()` order, so *which* rows were gone was arbitrary from the +caller's point of view, and a partial delete has no natural undo. + +This is #4620's principle — *`atomic` honoured for real, or refused* — applied to +the path that never got it. The by-id delete and its whole cascade now run +inside one `engine.transaction()`: a refusal anywhere rolls back every child +delete and every `set_null` FK clear, so a 409 honestly means nothing changed. +The recursion **joins** that transaction rather than nesting under it (ADR-0067 +D2 / #5696), so a multi-level cascade opens exactly one driver transaction on +one connection instead of one per record. + +**Behaviour change**, and the reason for the `minor`: a by-id delete that +refuses mid-cascade used to leave earlier children deleted and now leaves +nothing deleted. Callers that (knowingly or not) depended on the partial effect +— e.g. treating a 409 as "some children were cleaned up" — see the prefix +restored instead. + +Two deliberate limits, both chosen from a driver census rather than from taste: + +- **No `require: true`.** `transaction()` can fail closed on a driver without + `beginTransaction` (#5696 point 1), and this call does not use it. `delete()` + never asked for atomicity, so it must not *start refusing* on a runtime that + cannot roll back; `require: true` here would turn an ordinary delete into a + `TransactionUnsupportedError` on every non-conforming driver — ~50 in-tree + test doubles and any embedder's partial driver — while buying nothing on real + ones, because `beginTransaction` is a **required** member of `IDataDriver` and + all five in-tree drivers (memory, sql, sqlite-wasm, turso, mongodb) implement + it. The declared degrade (ADR-0119 D1) stands, and since #4619 it warns once + per driver. **The cost of that choice, stated plainly:** on a driver that + cannot roll back, the partial-cascade window remains exactly as before, now + reported as a `warn` line rather than a refusal. +- **A cross-datasource cascade keeps its old, non-atomic answer.** A transaction + covers one driver's connection (ADR-0119 D1 — no two-phase commit), so when + the cascade reaches an object routed off the default datasource, wrapping it + would not make it atomic; it would make it *fail*, because + `enforceTransactionOrigin` refuses a business write inside a transaction that + does not cover it (#5351 / #5696 point 2). Such a delete works today, and a + hard refusal is strictly worse than the non-atomic answer it has always had, + so it runs unwrapped and says so once per object. + +A delete with nothing referencing the object opens no transaction and emits no +warning — that path is unchanged, and is pinned as the control. diff --git a/packages/objectql/src/engine-cascade-delete-atomic.test.ts b/packages/objectql/src/engine-cascade-delete-atomic.test.ts new file mode 100644 index 0000000000..13dee7f029 --- /dev/null +++ b/packages/objectql/src/engine-cascade-delete-atomic.test.ts @@ -0,0 +1,513 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `ObjectQL.delete`'s by-id cascade is ONE unit of work (#7413). + * + * The defect: `delete()`'s by-id branch ran `cascadeDeleteRelations` and then + * `driver.delete` with no transaction around either, and the cascade re-enters + * `this.delete()` / `this.update()` per dependent — each committing as it + * executed. A refusal partway (the engine's own `restrict` branch, a child's + * permission check, a later child's `beforeDelete` hook) left an arbitrary + * PREFIX of the children deleted while the caller received 409/403 and + * reasonably read it as "nothing happened". Child visit order is + * `getAllObjects()` order, so *which* prefix was gone was arbitrary from the + * caller's point of view. + * + * The governing principle is #4620's changeset, ruled for the batch path: + * *`atomic` — honoured for real, or refused… a partial delete has no natural + * undo.* These pins are that principle for the single-id path. + * + * The driver below has REAL rollback (a deep snapshot per transaction, the same + * shape `driver-memory` uses), because a stub whose `rollback()` is a no-op + * cannot tell "wrapped in a transaction" from "not wrapped" — every assertion + * about restored rows would pass against the unfixed engine. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; + +type Recorded = { level: 'debug' | 'info' | 'warn' | 'error'; message: string }; + +function recordingLogger() { + const records: Recorded[] = []; + const push = (level: Recorded['level']) => (message: string) => + void records.push({ level, message: String(message) }); + return { + records, + logger: { debug: push('debug'), info: push('info'), warn: push('warn'), error: push('error') }, + at(level: Recorded['level']) { + return records.filter((r) => r.level === level); + }, + }; +} + +/** + * A driver with snapshot rollback and a full write/transaction ledger. + * + * `transactional: false` omits `beginTransaction` entirely — the shape the + * declared degrade (ADR-0119 D1) exists for. Every in-tree driver implements + * it; test doubles and foreign engines are what reach that path. + */ +function makeDriver(opts: { transactional?: boolean } = {}) { + const stores = new Map>>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + /** Every driver-level write, with the transaction handle it carried. */ + const writes: Array<{ object: string; op: 'create' | 'update' | 'delete'; transaction: unknown }> = []; + /** One entry per `beginTransaction` — the ambient-join measurement. */ + const begun: unknown[] = []; + const committed: unknown[] = []; + const rolledBack: unknown[] = []; + const snapshots = new Map>>>(); + let nextId = 0; + + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const exp = v && typeof v === 'object' && '$eq' in (v as any) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (exp ?? null)) return false; + } + return true; + }; + + const driver: any = { + name: 'primary', + version: '0.0.0', + supports: {}, + writes, + begun, + committed, + 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) { + return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(o: string, ast: any) { + for (const r of storeFor(o).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(o: string, data: Record, options: any) { + writes.push({ object: o, op: 'create', transaction: options?.transaction }); + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(o).set(id, row); + return row; + }, + async update(o: string, id: string, data: Record, options: any) { + writes.push({ object: o, op: 'update', transaction: options?.transaction }); + const s = storeFor(o); + const cur = s.get(String(id)); + if (!cur) throw new Error(`not found ${o}/${id}`); + const up = { ...cur, ...data, id }; + s.set(String(id), up); + return up; + }, + async upsert(o: string, data: Record) { + const id = data.id as string | undefined; + return id && storeFor(o).has(id) ? this.update(o, id, data, undefined) : this.create(o, data, undefined); + }, + async delete(o: string, id: string, options: any) { + writes.push({ object: o, op: 'delete', transaction: options?.transaction }); + return storeFor(o).delete(String(id)); + }, + async deleteMany(o: string, ast: any, options: any) { + const doomed = await this.find(o, ast); + for (const r of doomed) { + writes.push({ object: o, op: 'delete', transaction: options?.transaction }); + storeFor(o).delete(String((r as any).id)); + } + return doomed.length; + }, + 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, undefined))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + }; + + if (opts.transactional !== false) { + driver.beginTransaction = async () => { + const handle = { __trx: begun.length + 1 }; + const snap = new Map>>(); + for (const [o, s] of stores) { + snap.set(o, new Map(Array.from(s, ([k, v]) => [k, { ...v }]))); + } + snapshots.set(handle, snap); + begun.push(handle); + return handle; + }; + driver.commit = async (handle: unknown) => { + snapshots.delete(handle); + committed.push(handle); + }; + driver.rollback = async (handle: unknown) => { + const snap = snapshots.get(handle); + if (snap) { + stores.clear(); + for (const [o, s] of snap) stores.set(o, s); + } + snapshots.delete(handle); + rolledBack.push(handle); + }; + } + return driver; +} + +/** parent ← 3 cascade children (`kid`) + 1 `set_null` child (`link`). */ +const parent = { + name: 'parent', + label: 'Parent', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + }, +}; +const kid = { + name: 'kid', + label: 'Kid', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + parent: { name: 'parent', type: 'lookup' as const, reference: 'parent', deleteBehavior: 'cascade' }, + }, +}; +const link = { + name: 'link', + label: 'Link', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + parent: { name: 'parent', type: 'lookup' as const, reference: 'parent' }, + }, +}; +/** + * The engine's OWN refusal source, needing no app code: a REQUIRED lookup whose + * defaulted `set_null` escalates to `restrict` (see `cascadeDeleteRelations`). + * The issue names this as the variant reachable with nothing outside + * `packages/objectql`. + */ +const blocker = { + name: 'zz_blocker', + label: 'Blocker', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + parent: { name: 'parent', type: 'lookup' as const, reference: 'parent', required: true }, + }, +}; +/** Second cascade level: `grandkid` hangs off `kid`. */ +const grandkid = { + name: 'grandkid', + label: 'Grandkid', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + kid: { name: 'kid', type: 'lookup' as const, reference: 'kid', deleteBehavior: 'cascade' }, + }, +}; +/** No relation points at it — the control for "plain non-cascade delete". */ +const loner = { + name: 'loner', + label: 'Loner', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + }, +}; + +async function makeEngine( + objects: any[], + opts: { transactional?: boolean } = {}, +) { + const rec = recordingLogger(); + const engine = new ObjectQL({ logger: rec.logger } as any); + const driver = makeDriver(opts); + engine.registerDriver(driver, true); + await engine.init(); + for (const o of objects) engine.registry.registerObject(o, '__test__'); + return { engine, driver, rec }; +} + +// --------------------------------------------------------------------------- +// 1. The card's repro, inverted: a mid-cascade refusal deletes ZERO children. +// --------------------------------------------------------------------------- + +describe('a refusal mid-cascade rolls the whole delete back (#7413)', () => { + it('leaves ZERO children deleted when the engine\'s own restrict branch refuses', async () => { + // `zz_blocker` sorts last, so the cascade reaches `kid` FIRST and deletes + // all three rows before the restrict refusal lands — the exact ordering + // that made the defect observable. + const { engine, driver } = await makeEngine([parent, kid, blocker]); + const p = await engine.insert('parent', { name: 'P' }); + for (const n of ['a', 'b', 'c']) await engine.insert('kid', { name: n, parent: p.id }); + await engine.insert('zz_blocker', { parent: p.id }); + + await expect(engine.delete('parent', { where: { id: p.id } } as any)) + .rejects.toMatchObject({ code: 'DELETE_RESTRICTED', status: 409 }); + + // THE core assertion. Before this card: 0. The 409 now honestly means + // "nothing changed". + expect(driver.rowsOf('kid')).toHaveLength(3); + expect(driver.rowsOf('parent')).toHaveLength(1); + expect(driver.rolledBack).toHaveLength(1); + expect(driver.committed).toHaveLength(0); + }); + + it('leaves ZERO children deleted when a later child\'s beforeDelete hook refuses', async () => { + // The card's MEASURED variant: an app hook, not the engine's own branch. + const { engine, driver } = await makeEngine([parent, kid, grandkid]); + engine.registerHook( + 'beforeDelete', + async () => { + throw Object.assign(new Error('nope'), { status: 409 }); + }, + { object: 'grandkid' }, + ); + + const p = await engine.insert('parent', { name: 'P' }); + const k1 = await engine.insert('kid', { name: 'k1', parent: p.id }); + const k2 = await engine.insert('kid', { name: 'k2', parent: p.id }); + await engine.insert('grandkid', { kid: k2.id }); + + await expect(engine.delete('parent', { where: { id: p.id } } as any)).rejects.toThrow(); + + // k1 was already cascaded away before the hook on k2's grandchild threw. + expect(driver.rowsOf('kid').map((r: any) => r.id).sort()).toEqual([k1.id, k2.id].sort()); + expect(driver.rowsOf('grandkid')).toHaveLength(1); + expect(driver.rowsOf('parent')).toHaveLength(1); + expect(driver.rolledBack).toHaveLength(1); + }); + + it('restores set_null children too — the FK is not left cleared', async () => { + const { engine, driver } = await makeEngine([parent, link, blocker]); + const p = await engine.insert('parent', { name: 'P' }); + const l = await engine.insert('link', { parent: p.id }); + await engine.insert('zz_blocker', { parent: p.id }); + + await expect(engine.delete('parent', { where: { id: p.id } } as any)) + .rejects.toMatchObject({ code: 'DELETE_RESTRICTED' }); + + // `link.parent` was nulled mid-cascade; the rollback puts it back. Without + // it the child survives pointing at nothing, which is the same silent + // corruption one field over. + const restored = driver.rowsOf('link').find((r: any) => r.id === l.id); + expect(restored?.parent).toBe(p.id); + }); + + it('is all-or-nothing across MULTIPLE cascade levels', async () => { + const { engine, driver } = await makeEngine([parent, kid, grandkid, blocker]); + const p = await engine.insert('parent', { name: 'P' }); + const k = await engine.insert('kid', { name: 'k', parent: p.id }); + await engine.insert('grandkid', { kid: k.id }); + await engine.insert('grandkid', { kid: k.id }); + await engine.insert('zz_blocker', { parent: p.id }); + + await expect(engine.delete('parent', { where: { id: p.id } } as any)) + .rejects.toMatchObject({ code: 'DELETE_RESTRICTED' }); + + expect(driver.rowsOf('grandkid')).toHaveLength(2); + expect(driver.rowsOf('kid')).toHaveLength(1); + expect(driver.rowsOf('parent')).toHaveLength(1); + }); + + it('commits the whole cascade when nothing refuses', async () => { + const { engine, driver } = await makeEngine([parent, kid, grandkid, link]); + const p = await engine.insert('parent', { name: 'P' }); + const k = await engine.insert('kid', { name: 'k', parent: p.id }); + await engine.insert('grandkid', { kid: k.id }); + const l = await engine.insert('link', { parent: p.id }); + + await engine.delete('parent', { where: { id: p.id } } as any); + + expect(driver.rowsOf('parent')).toHaveLength(0); + expect(driver.rowsOf('kid')).toHaveLength(0); + expect(driver.rowsOf('grandkid')).toHaveLength(0); + expect(driver.rowsOf('link').find((r: any) => r.id === l.id)?.parent).toBeNull(); + expect(driver.committed).toHaveLength(1); + expect(driver.rolledBack).toHaveLength(0); + }); +}); + +// --------------------------------------------------------------------------- +// 2. Ambient join (ADR-0067 D2 / #5696) — measured, not assumed. +// --------------------------------------------------------------------------- + +describe('the recursive cascade JOINS the ambient transaction (#5696)', () => { + it('opens exactly ONE driver transaction for a multi-level cascade', async () => { + const { engine, driver } = await makeEngine([parent, kid, grandkid, link]); + const p = await engine.insert('parent', { name: 'P' }); + const k1 = await engine.insert('kid', { name: 'k1', parent: p.id }); + const k2 = await engine.insert('kid', { name: 'k2', parent: p.id }); + await engine.insert('grandkid', { kid: k1.id }); + await engine.insert('grandkid', { kid: k2.id }); + await engine.insert('link', { parent: p.id }); + + await engine.delete('parent', { where: { id: p.id } } as any); + + // Each child `delete()` re-enters the wrap. If the join were not honoured + // this would be one `beginTransaction` per cascaded record — a second + // connection per level (the deadlock ADR-0067 D2 exists to avoid), and + // nested handles the outer rollback would not cover. + expect(driver.begun).toHaveLength(1); + expect(driver.committed).toHaveLength(1); + }); + + it('routes every cascade write onto that ONE transaction handle', async () => { + const { engine, driver } = await makeEngine([parent, kid, grandkid, link]); + const p = await engine.insert('parent', { name: 'P' }); + const k = await engine.insert('kid', { name: 'k', parent: p.id }); + await engine.insert('grandkid', { kid: k.id }); + await engine.insert('link', { parent: p.id }); + driver.writes.length = 0; + + await engine.delete('parent', { where: { id: p.id } } as any); + + // Asserted before the loop below, which would otherwise pass VACUOUSLY + // against an unwrapped engine: no transaction means `handle` is + // `undefined` and every write's `transaction` is `undefined` too. + expect(driver.begun).toHaveLength(1); + const handle = driver.begun[0]; + expect(handle).toBeDefined(); + expect(driver.writes.length).toBeGreaterThan(0); + // Including the PARENT's own `driver.delete`, whose options are rebuilt + // inside the callback precisely so it does not execute outside the + // transaction that wraps its cascade. + for (const w of driver.writes) expect(w.transaction).toBe(handle); + expect(driver.writes.some((w: any) => w.object === 'parent' && w.op === 'delete')).toBe(true); + expect(driver.writes.some((w: any) => w.object === 'link' && w.op === 'update')).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// 3. Control: a delete with nothing referencing it is byte-identical. +// --------------------------------------------------------------------------- + +describe('a plain non-cascade delete is unchanged (#7413 control)', () => { + it('opens NO transaction and emits no warning when no relation points at the object', async () => { + const { engine, driver, rec } = await makeEngine([loner]); + const l = await engine.insert('loner', { name: 'solo' }); + driver.writes.length = 0; + + await engine.delete('loner', { where: { id: l.id } } as any); + + expect(driver.begun).toHaveLength(0); + expect(driver.rowsOf('loner')).toHaveLength(0); + // The one driver write still carries no transaction handle, exactly as + // before this card. + expect(driver.writes).toHaveLength(1); + expect(driver.writes[0].transaction).toBeUndefined(); + expect(rec.at('warn')).toHaveLength(0); + }); + + it('opens no transaction when the object HAS dependents declared but the delete is by predicate', async () => { + // The predicate branch never called `cascadeDeleteRelations` and still does + // not — the wrap is the by-id branch's alone. + const { engine, driver } = await makeEngine([parent, kid]); + await engine.insert('parent', { name: 'P' }); + driver.writes.length = 0; + + await engine.delete('parent', { where: { name: 'P' }, multi: true } as any); + + expect(driver.begun).toHaveLength(0); + }); +}); + +// --------------------------------------------------------------------------- +// 4. The transactionless runtime — the DEGRADE was chosen over `require: true`. +// --------------------------------------------------------------------------- + +describe('a driver without beginTransaction degrades, it does not refuse (#4619 / ADR-0119 D1)', () => { + it('still performs the delete instead of throwing TransactionUnsupportedError', async () => { + const { engine, driver } = await makeEngine([parent, kid], { transactional: false }); + const p = await engine.insert('parent', { name: 'P' }); + await engine.insert('kid', { name: 'k', parent: p.id }); + + // `delete()` never asked for atomicity, so it must not START refusing on a + // runtime that cannot roll back. `require: true` would have made this line + // throw — the losing option's cost, pinned so a later edit has to argue it. + await engine.delete('parent', { where: { id: p.id } } as any); + + expect(driver.rowsOf('parent')).toHaveLength(0); + expect(driver.rowsOf('kid')).toHaveLength(0); + }); + + it('warns ONCE per driver that the cascade is running without rollback', async () => { + const { engine, rec } = await makeEngine([parent, kid], { transactional: false }); + const p1 = await engine.insert('parent', { name: 'P1' }); + await engine.insert('kid', { name: 'k1', parent: p1.id }); + const p2 = await engine.insert('parent', { name: 'P2' }); + await engine.insert('kid', { name: 'k2', parent: p2.id }); + + await engine.delete('parent', { where: { id: p1.id } } as any); + await engine.delete('parent', { where: { id: p2.id } } as any); + + expect(rec.at('warn').filter((r) => r.message.includes('has no beginTransaction'))).toHaveLength(1); + }); +}); + +// --------------------------------------------------------------------------- +// 5. Hooks: count and order are NOT silently changed by the wrap. +// --------------------------------------------------------------------------- + +describe('hook firing is unchanged by the transaction wrap (#7413)', () => { + const trace = (engine: ObjectQL, events: string[]) => { + for (const object of ['parent', 'kid']) { + for (const event of ['beforeDelete', 'afterDelete'] as const) { + engine.registerHook(event, async () => void events.push(`${object}:${event}`), { object }); + } + } + }; + + it('fires the same before/after hooks, in the same order, on SUCCESS', async () => { + const events: string[] = []; + const { engine } = await makeEngine([parent, kid]); + trace(engine, events); + const p = await engine.insert('parent', { name: 'P' }); + await engine.insert('kid', { name: 'k', parent: p.id }); + events.length = 0; + + await engine.delete('parent', { where: { id: p.id } } as any); + + // The parent's `beforeDelete` runs before the cascade; the child's pair + // runs inside it; the parent's `afterDelete` closes. Unchanged by the wrap + // — which is placed INSIDE the parent's hook pair on purpose. + expect(events).toEqual([ + 'parent:beforeDelete', + 'kid:beforeDelete', + 'kid:afterDelete', + 'parent:afterDelete', + ]); + }); + + it('fires the same hooks on a REFUSAL — the rollback does not un-fire them', async () => { + const events: string[] = []; + const { engine } = await makeEngine([parent, kid, blocker]); + trace(engine, events); + const p = await engine.insert('parent', { name: 'P' }); + await engine.insert('kid', { name: 'k', parent: p.id }); + await engine.insert('zz_blocker', { parent: p.id }); + events.length = 0; + + await expect(engine.delete('parent', { where: { id: p.id } } as any)) + .rejects.toMatchObject({ code: 'DELETE_RESTRICTED' }); + + // DECLARED, not incidental: the cascaded child's `afterDelete` HAS fired + // for a row the rollback then restored. That is the established shape of + // every atomic write path in this engine — `runAtomicBatch` (#4620) fires + // per-row delete hooks inside the same rollback-able scope — and it is the + // strictly better half of the trade: before this card the hook fired AND + // the row stayed gone. Re-timing `afterDelete` to fire after commit is a + // separate question, filed rather than folded in here. + expect(events).toEqual([ + 'parent:beforeDelete', + 'kid:beforeDelete', + 'kid:afterDelete', + ]); + expect(events.filter((e) => e === 'parent:afterDelete')).toHaveLength(0); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 5d25647e90..826be6866b 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1541,6 +1541,15 @@ export class ObjectQL implements IObjectQLEngine { */ private readonly transactionUnsupportedReported = new Set(); + /** + * Objects already reported by {@link warnCascadeNotAtomic} — the `'split'` + * verdict of {@link planCascadeAtomicity} — so a cascade that cannot be one + * unit of work says so ONCE per engine instance per object rather than on + * every delete (#7413). Same "say it once" discipline, and same reason, as + * {@link transactionUnsupportedReported} above. + */ + private readonly cascadeNotAtomicReported = new Set(); + // Datasource mapping rules (imported from defineStack) private datasourceMapping: Array<{ namespace?: string; @@ -8394,6 +8403,143 @@ export class ObjectQL implements IObjectQLEngine { return opCtx.result; } + /** + * Can the by-id delete of `object` and its whole cascade run as ONE unit of + * work? [#7413] + * + * `delete()`'s by-id branch ran `cascadeDeleteRelations` and then + * `driver.delete` with no transaction around either, and the cascade + * RE-ENTERS `this.delete()` / `this.update()` per dependent row — so every + * child committed on its own. A refusal partway (the `restrict` branch below, + * a child's permission check, a later child's `beforeDelete` hook) left an + * arbitrary PREFIX of the children deleted while the caller received 409/403 + * and reasonably read it as "nothing happened". #4620 already ruled the + * principle for the batch path — *atomic honoured for real, or refused; a + * partial delete has no natural undo* — and this path never got it. + * + * Three verdicts, because "wrap it in a transaction" is only unconditionally + * right in one of them: + * + * - **`'none'`** — no registered object declares a `master_detail`/`lookup` + * pointing at `object`, so `cascadeDeleteRelations` cannot write anything + * and there is no multi-write unit to make atomic. The delete runs exactly + * as it did before this card: one `driver.delete`, no transaction opened, + * no degrade warning. A plain non-cascade delete is the CONTROL for this + * change and must stay byte-identical. + * - **`'split'`** — dependents exist, but at least one participant (the + * parent or a transitively-referencing child) resolves to a driver other + * than the engine's DEFAULT one. `transaction()` opens on the default + * driver and covers that connection alone (ADR-0119 D1 — no two-phase + * commit on `IDataDriver`), so opening one here would not buy atomicity; it + * would REFUSE the cascade outright, because `enforceTransactionOrigin` + * throws {@link CrossDatasourceTransactionWriteError} for a business write + * inside a transaction that does not cover it (#5351 / #5696 point 2, the + * 2026-08-06 ruling). A cross-datasource cascade delete works today, and + * turning it into a hard refusal is a strictly worse answer than the + * non-atomic one it has always had. So it keeps that answer and says so + * once — see {@link warnCascadeNotAtomic}. + * - **`'atomic'`** — dependents exist and every participant sits on the + * default driver. This is the shape the card measured and the shape + * virtually every deployment has; the caller gets one unit of work. + * + * A pure registry walk — no I/O, no driver call. It answers from the SCHEMA + * (which objects *could* be reached), never from row counts, so the verdict + * is stable for a given schema and costs nothing on the common path. The walk + * is the transitive closure of "objects referencing X" because a cascade + * recurses into each child's own cascade; it is bounded by + * {@link ObjectQL.MAX_CASCADE_DEPTH}, the ceiling the recursion itself carries. + * + * Fails toward `'split'` on anything it cannot resolve (an unregistered + * reference, a `getDriver` throw): the degrade is today's behaviour, while a + * wrong `'atomic'` would manufacture the refusal this verdict exists to + * avoid. + */ + private planCascadeAtomicity(object: string): 'none' | 'split' | 'atomic' { + let objects: ServiceObject[]; + try { + objects = this._registry.getAllObjects(); + } catch { + // Same swallow as `cascadeDeleteRelations` — an unreadable registry + // cascades nothing, so there is nothing to make atomic. + return 'none'; + } + + // Which objects reference `name` via a relation the cascade would follow. + // The `master_detail`/`lookup` + `reference` test is `cascadeDeleteRelations`'s + // own, so the two cannot disagree about who participates. `restrict` fields + // are INCLUDED deliberately: a restrict refusal is the card's core repro, + // and it must roll back the siblings already cascaded before it. + const referencing = (name: string): string[] => { + const out: string[] = []; + for (const child of objects) { + const childName = (child as any)?.name as string | undefined; + const fields = (child as any)?.fields as Record | undefined; + if (!childName || !fields) continue; + for (const fdef of Object.values(fields)) { + if (!fdef || (fdef.type !== 'master_detail' && fdef.type !== 'lookup')) continue; + const ref = fdef.reference; + if (!ref) continue; + let resolvedRef: string | undefined; + try { resolvedRef = this.resolveObjectName(ref); } catch { resolvedRef = undefined; } + if (ref !== name && resolvedRef !== name) continue; + out.push(childName); + break; + } + } + return out; + }; + + const firstLevel = referencing(object); + if (firstLevel.length === 0) return 'none'; + + const defaultDriver = this.defaultDriver ? this.drivers.get(this.defaultDriver) : undefined; + if (!defaultDriver) return 'split'; + const onDefaultDriver = (name: string): boolean => { + try { return this.getDriver(name) === defaultDriver; } catch { return false; } + }; + + if (!onDefaultDriver(object)) return 'split'; + const seen = new Set([object]); + let frontier = firstLevel; + for (let depth = 0; depth < ObjectQL.MAX_CASCADE_DEPTH && frontier.length > 0; depth++) { + const next: string[] = []; + for (const name of frontier) { + if (seen.has(name)) continue; + seen.add(name); + if (!onDefaultDriver(name)) return 'split'; + next.push(...referencing(name)); + } + frontier = next; + } + return 'atomic'; + } + + /** + * The `'split'` verdict of {@link planCascadeAtomicity}, said out loud — once + * per object per engine instance (#7413). + * + * Same reasoning as {@link warnTransactionUnsupported}, which this mirrors + * deliberately: a capability is not available, so the delete keeps the + * non-atomic behaviour it has always had. `warn`, not `error` — at this + * moment nothing claimed-persisted has failed to land; what is missing is the + * ability to undo the cascade if a later child refuses. Escalating it would + * train readers to skim `error`, which AGENTS.md names as the mirror-image + * mistake. + */ + private warnCascadeNotAtomic(object: string): void { + if (this.cascadeNotAtomicReported.has(object)) return; + this.cascadeNotAtomicReported.add(object); + this.logger.warn( + `Cascade delete of '${object}' cannot run as one unit of work: the cascade reaches an object routed ` + + `to a datasource other than the default one ('${this.defaultDriver ?? ''}'), and a transaction ` + + "covers one driver's connection only (ADR-0119 D1 — no two-phase commit). The cascade therefore runs " + + 'UNWRAPPED, exactly as it did before #7413: if a later dependent refuses the delete, the rows already ' + + 'removed stay removed while the call rejects. Route the cascading objects to one datasource to get the ' + + 'atomic path. Reported once per object per engine instance.', + { object, defaultDatasource: this.defaultDriver ?? undefined }, + ); + } + /** * Apply referential delete behavior for relations pointing AT this record, * before it is removed. For every registered object with a `master_detail` @@ -8828,8 +8974,61 @@ export class ObjectQL implements IObjectQLEngine { if (isByIdDelete) { // Honor referential delete behavior (cascade/set_null/restrict) // for relations pointing at this record before removing it. - await this.cascadeDeleteRelations(object, hookContext.input.id as string | number, opCtx.context); - result = await driver.delete(object, hookContext.input.id as string, hookContext.input.options as any); + // + // [#7413] ONE UNIT OF WORK — the cascade and the parent's own row + // removal, or neither. These two statements used to run bare, and + // the cascade re-enters `this.delete()`/`this.update()` per + // dependent, each committing as it executed; a refusal partway + // (the `restrict` branch, a child's permission check, a later + // child's `beforeDelete` hook) stranded an arbitrary prefix of the + // children deleted while the caller got 409/403 and read it as + // "nothing happened". #4620's changeset already ruled the shape + // for the batch path: atomic honoured for real, or refused — a + // partial delete has no natural undo. + // + // The recursion COMPOSES rather than deadlocking because + // `transaction()` publishes its handle into the ambient `txStore` + // and joins an already-open one (ADR-0067 D2 / #5696): the child + // `delete()` calls below receive `trxContext` explicitly AND see + // the ambient entry, so a grandchild's own wrap JOINS with + // `owned: false` instead of opening a second driver transaction on + // a second connection. + // + // The driver options are rebuilt INSIDE the callback on purpose: + // the ones computed above were built before any transaction + // existed, so they carry no handle and the parent's own + // `driver.delete` would execute outside the very transaction + // wrapping its cascade. `buildDriverOptions` only fills keys that + // are still `undefined`, so re-running it adds the handle and + // changes nothing else. + const runByIdDelete = async (writeContext?: ExecutionContext) => { + await this.cascadeDeleteRelations(object, hookContext.input.id as string | number, writeContext); + return await driver.delete( + object, + hookContext.input.id as string, + this.buildDriverOptions(object, writeContext, hookContext.input.options as any), + ); + }; + // WHEN to open one is `planCascadeAtomicity`'s call — see there. + // `'none'` (nothing references this object) keeps the pre-#7413 + // path exactly, so a plain non-cascade delete opens no + // transaction and emits no degrade warning; `'split'` keeps it + // too, because a transaction on the default driver cannot cover a + // cross-datasource cascade and would refuse it outright. + const plan = this.planCascadeAtomicity(object); + if (plan === 'atomic') { + // No `require: true` — see the changeset. `delete()` never + // asked for atomicity, so it must not start REFUSING on a + // runtime that cannot roll back; `transaction()`'s declared + // degrade (ADR-0119 D1) already warns once per driver (#4619). + result = await this.transaction( + async (trxCtx: any) => await runByIdDelete(trxCtx as ExecutionContext), + opCtx.context, + ); + } else { + if (plan === 'split') this.warnCascadeNotAtomic(object); + result = await runByIdDelete(opCtx.context); + } } else { // [#2982] The AST asserted present and already used for the // pre-phase row read above.