Skip to content

fix(objectql): ObjectQL.delete's by-id cascade is one unit of work (#7413) - #7476

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-7413-delete-cascade-transactional
Aug 10, 2026
Merged

fix(objectql): ObjectQL.delete's by-id cascade is one unit of work (#7413)#7476
os-zhuang merged 1 commit into
mainfrom
claude/issue-7413-delete-cascade-transactional

Conversation

@os-zhuang

@os-zhuang os-zhuang commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Closes #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 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 ruled principle — atomic honoured for real, or refused — applied to the path that never got it.

The change

The by-id delete and its whole cascade now run inside one engine.transaction(). Three things were needed beyond the wrap itself:

  1. The recursion joins, it does not nest. transaction() publishes its handle into the ambient txStore and joins an already-open one (ADR-0067 D2 / [spec] engine.transaction 契约收紧:opts.require fail-closed、跨驱动拒绝、owned-vs-joined 信号(#4619 的契约半边,维护者已批 P2) #5696), so each child delete() re-entering the wrap joins with owned: false instead of opening a second driver transaction on a second connection. Measured, not assumed — a multi-level cascade opens exactly one beginTransaction, pinned.
  2. Driver options are rebuilt inside the callback. The options computed before the try block were built when no transaction existed, so the parent's own driver.delete would have executed outside the transaction wrapping its cascade. buildDriverOptions only fills keys still undefined, so re-running it adds the handle and changes nothing else.
  3. planCascadeAtomicity decides when to open one — a pure registry walk, no I/O, three verdicts. See below.

The one open choice: require: true vs. default-degrade → degrade

Argued from a census, as the pre-dispatch assessment asked:

driver beginTransaction
driver-memory ✅ snapshot rollback
driver-sql ✅ knex
driver-sqlite-wasm ✅ inherited (extends SqlDriver)
driver-turso ✅ local via super, remote via transport
driver-mongodb ClientSession

beginTransaction is a required (non-optional) member of IDataDriver (packages/spec/src/contracts/data-driver.ts), so every conforming driver has it and the degrade path is unreachable on any conforming runtime — the fix is real everywhere it ships.

What require: true would refuse is the non-conforming population: ~50 in-tree files register a driver double with no beginTransaction, plus any embedder's partial driver. Every plain delete() of a record with any declared relation would start throwing TransactionUnsupportedError. delete() never asked for atomicity — require: true is documented as the opt-in for "a caller who cannot live with the degrade" (that is exactly how #4620 uses it: gated on the caller passing atomic: true). Generalizing it to an operation nobody opted into buys nothing on real runtimes and breaks non-conforming ones.

The losing option's cost, stated plainly: on a driver that cannot roll back, the partial-cascade window remains exactly as before — reported as a once-per-driver warn (#4619) rather than a refusal. That is pinned as a test, so a later edit has to argue against it rather than drift into it.

Second limit: a cross-datasource cascade keeps its old answer

Not in the original scope, and it is a regression this PR had to avoid rather than a feature it adds. transaction() opens on the default driver and covers that one connection (ADR-0119 D1 — no two-phase commit). Wrapping a cascade that reaches an object routed elsewhere would not make it atomic; it would make it fail, because enforceTransactionOrigin throws CrossDatasourceTransactionWriteError for a business write inside a transaction that does not cover it (#5351 / #5696 point 2, the 2026-08-06 ruling). Such a delete works today, and a hard refusal is strictly worse than the non-atomic answer it has always had — so planCascadeAtomicity returns 'split' and it runs unwrapped, warned once per object.

The third verdict, 'none' (nothing references the object), keeps the pre-existing path byte-identical: no transaction opened, no warning, one driver write carrying no handle.

Hooks inside a rollback-able scope — declared, not new

The assessment flagged this as a possible stop-condition. It is not a new fork: the parent's own beforeDelete/afterDelete stay outside the wrap (it is placed between them), and a cascaded child's afterDelete firing for a row the rollback then restores 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. It is also the strictly better half of the trade: before this card the hook fired and the row stayed gone. Hook count and order are pinned unchanged on success and on refusal.

Re-timing after* to fire post-commit is a separate question and is filed as #7477, not folded in here.

Pins — packages/objectql/src/engine-cascade-delete-atomic.test.ts (13)

The stub driver has real snapshot rollback; a no-op rollback() cannot tell "wrapped" from "not wrapped" and every restored-row assertion would pass against the unfixed engine.

  • mid-cascade restrict refusal ⇒ zero children deleted (the card's repro, inverted — the core assertion), plus the app-hook variant it actually measured;
  • set_null children restored on rollback — the FK is not left cleared;
  • multi-level cascade all-or-nothing; whole cascade commits when nothing refuses;
  • ambient join: exactly one beginTransaction for a multi-level cascade, and every write — including the parent's own — carries that one handle;
  • transactionless driver: still deletes (does not refuse), warns once per driver;
  • controls: plain non-cascade delete opens no transaction and emits no warning; the predicate branch is untouched;
  • hook count/order unchanged on success and on refusal.

Reverse-verified: against origin/main's engine.ts these fail 7 / pass 6 — the 6 are the controls, which pin unchanged behaviour and must pass both ways. One assertion was strengthened after that run because it passed vacuously on the unfixed engine (handle and every write's transaction were both undefined).

Changeset

minor, in .changeset/delete-cascade-one-unit-of-work.md. The behaviour of a public API changes under failure (partial → none), which is breaking-shaped; check-changeset-no-major.mjs records the launch-window convention that the fixed lockstep group ships breaking as minor, so no major and therefore no ADR-0087 trio. No packages/spec touch.

Gates run locally (all pass)

check:adr-anchors · check:durability-log-level · check:engine-double-contract · check:stack-collection-maps · check:nul-bytes · scripts/check-engine-split-ratio.mjs · check-changeset-no-major.mjs · check-empty-changeset.mjs · eslint --no-inline-config on both changed files.

Suites: @objectstack/objectql 178 files / 3148 tests green; @objectstack/metadata-protocol 70 files / 1035 tests green. Full CI decides.

Refs #7413 #4620 #5696 #4619 #5351 #7477 ADR-0119 ADR-0067.

…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 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TAeyg4nJ3yeePVmCbdfxd5
@vercel

vercel Bot commented Aug 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 10, 2026 4:20pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/objectql.

15 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx (via @objectstack/objectql)
  • content/docs/data-modeling/formulas.mdx (via packages/objectql)
  • content/docs/deployment/migration-from-objectql.mdx (via @objectstack/objectql)
  • content/docs/deployment/vercel.mdx (via @objectstack/objectql)
  • content/docs/kernel/contracts/data-engine.mdx (via @objectstack/objectql)
  • content/docs/kernel/runtime-services/examples.mdx (via packages/objectql)
  • content/docs/kernel/services-checklist.mdx (via @objectstack/objectql)
  • content/docs/kernel/services.mdx (via @objectstack/objectql)
  • content/docs/permissions/authentication.mdx (via @objectstack/objectql)
  • content/docs/permissions/system-context.mdx (via packages/objectql)
  • content/docs/plugins/index.mdx (via @objectstack/objectql)
  • content/docs/plugins/packages.mdx (via @objectstack/objectql)
  • content/docs/protocol/kernel/index.mdx (via @objectstack/objectql)
  • content/docs/protocol/objectql/query-syntax.mdx (via packages/objectql)
  • content/docs/protocol/objectql/state-machine.mdx (via @objectstack/objectql)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx (via @objectstack/objectql)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants