fix(objectql): ObjectQL.delete's by-id cascade is one unit of work (#7413) - #7476
Conversation
…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
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 15 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also reference the affected code. These are read-only:
|
Closes #7413.
The defect
delete()'s by-id branch rancascadeDeleteRelationsand thendriver.deletewith no transaction around either, and the cascade re-entersthis.delete()/this.update()per dependent row — so every child committed as it executed. A refusal partway (the engine's ownrestrictbranch, a child's permission check, a later child'sbeforeDeletehook) left an arbitrary prefix of the children deleted while the caller received 409/403 and reasonably concluded nothing had happened. Children are visited ingetAllObjects()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 —
atomichonoured 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:transaction()publishes its handle into the ambienttxStoreand joins an already-open one (ADR-0067 D2 / [spec] engine.transaction 契约收紧:opts.requirefail-closed、跨驱动拒绝、owned-vs-joined 信号(#4619 的契约半边,维护者已批 P2) #5696), so each childdelete()re-entering the wrap joins withowned: falseinstead of opening a second driver transaction on a second connection. Measured, not assumed — a multi-level cascade opens exactly onebeginTransaction, pinned.driver.deletewould have executed outside the transaction wrapping its cascade.buildDriverOptionsonly fills keys stillundefined, so re-running it adds the handle and changes nothing else.planCascadeAtomicitydecides when to open one — a pure registry walk, no I/O, three verdicts. See below.The one open choice:
require: truevs. default-degrade → degradeArgued from a census, as the pre-dispatch assessment asked:
beginTransactiondriver-memorydriver-sqldriver-sqlite-wasmextends SqlDriver)driver-tursosuper, remote via transportdriver-mongodbClientSessionbeginTransactionis a required (non-optional) member ofIDataDriver(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: truewould refuse is the non-conforming population: ~50 in-tree files register a driver double with nobeginTransaction, plus any embedder's partial driver. Every plaindelete()of a record with any declared relation would start throwingTransactionUnsupportedError.delete()never asked for atomicity —require: trueis 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 passingatomic: 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, becauseenforceTransactionOriginthrowsCrossDatasourceTransactionWriteErrorfor 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 — soplanCascadeAtomicityreturns'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/afterDeletestay outside the wrap (it is placed between them), and a cascaded child'safterDeletefiring 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.restrictrefusal ⇒ zero children deleted (the card's repro, inverted — the core assertion), plus the app-hook variant it actually measured;set_nullchildren restored on rollback — the FK is not left cleared;beginTransactionfor a multi-level cascade, and every write — including the parent's own — carries that one handle;Reverse-verified: against
origin/main'sengine.tsthese 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 (handleand every write'stransactionwere bothundefined).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.mjsrecords the launch-window convention that the fixed lockstep group ships breaking asminor, so nomajorand therefore no ADR-0087 trio. Nopackages/spectouch.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-configon both changed files.Suites:
@objectstack/objectql178 files / 3148 tests green;@objectstack/metadata-protocol70 files / 1035 tests green. Full CI decides.Refs #7413 #4620 #5696 #4619 #5351 #7477 ADR-0119 ADR-0067.