feat(spec,metadata-protocol): ADR-0118 — transactions reach plugin space through the contract, and atomic stops lying (#4612) - #4623
Merged
Conversation
…ace through the contract, and `atomic` stops lying (#4612) #4612 asks the platform to choose a debt: transactionalize the engine, bless a shared migration-journal primitive, or rule both out. Scoping it found the first option's expensive half was paid three majors ago — ADR-0034's ambient transaction is implemented, and hooks/validation/internal reads already join it — while the cheap half was never done. So this records ADR-0118, which splits the ask along where the work actually is. D1 (here) — `transaction` joins `IObjectQLEngine`. The mechanism ships; only the declaration was missing, so the metadata protocol's atomic publish, its `transactionalBatch` probe, and the sys-metadata repository each reached it through `as unknown as` casts — unchecked claims about a class none of them import. The contract's own evidence bar was met three times over. Its caveats (default-driver only; runs without a transaction when the driver lacks `beginTransaction`) go into the TSDoc as declared meaning, not behaviour to be discovered. Narrow host surfaces now type their optional member as `IObjectQLEngine['transaction']` so they cannot drift from it. D4 (here) — `batchData`'s `atomic` opened no transaction. It broke a loop: prior writes stayed committed while the response reported them `success: true` under the one flag guaranteeing they were undone — #4346's class, silent and destructive. It now runs the batch inside one `engine.transaction()`, and a failure rolls back everything and says so (`succeeded: 0`, rows marked ROLLED_BACK / NOT_ATTEMPTED). A runtime that cannot roll back is REFUSED with 501, never degraded — silent degradation is how the flag came to lie. `BatchOptionsSchema.atomic`'s declared `.default(true)` is aligned down to the enforced `false`, so opting in is explicit and no existing caller's failure semantics change underneath them. D2 (#4617) specifies a framework-owned migration-journal runner: transactions cannot span a million-row backfill, a whole-DB-snapshot driver, a multi- datasource write, or a process crash, so the four queued consumers need journal + compensation + resume on top of transactions, not instead of them. D3 (#4618) retires `IDataEngine.batch?` — declared, implemented by nobody, called by nobody. Caveat hardening is #4619; the sibling fake-atomics in `deleteManyData`/`updateManyData` are #4620. Tests: 10 unit pins on the atomic contract (rollback shape, 501 refusal with zero writes attempted, precedence, non-atomic regression) and 6 integration pins over a real engine + transacting driver, including that an internal read during the batch binds to the open transaction — the no-deadlock coverage whose absence ADR-0034 was written about. Closes #4612 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NKcGqCYCCpMkB5UW8jNPXx
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 107 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
The docs-drift check flagged the hand-written API docs, and three of them were genuinely stale — they documented the bug ADR-0118 D4 fixes: - `wire-format.mdx` stated it outright: "By default (`options.atomic: true`) processing stops at the first failing record — records already written earlier in the same batch are **not** rolled back, since there is no wrapping database transaction." Accurate before, wrong on both halves now. - `client-sdk.mdx`'s options table listed `atomic` default `true` with "Rollback entire batch on any failure" — the default moved, and the rollback claim only became true with this change. - `data-api.mdx` described `atomic` as the default on `/batch` and `deleteMany`. Each now describes what the route actually does, including the 501 refusal and the rolled-back response shape (`succeeded: 0`, ROLLED_BACK / NOT_ATTEMPTED). Kept honest about scope: `deleteMany` and `updateMany` did NOT get D4's treatment, so `deleteMany`'s note now says plainly that its `atomic` only stops the run and rolls nothing back (#4620 tracks the fix). Documenting the fixed behaviour on routes that still carry the bug would have replaced one inaccuracy with another. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NKcGqCYCCpMkB5UW8jNPXx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4612. Records ADR-0118 (
docs/adr/0118-plugin-reachable-transactions-and-honest-atomic-batch.md) and lands its two implementable decisions.Why this isn't one of the issue's three options
#4612 asks the platform to pick a debt: (1) surface driver transactions through the engine, (2) bless a migration-journal primitive, or (3) rule both out. Scoping it overturned the premise the pricing rested on.
Option 1's expensive half was paid three majors ago. ADR-0034 (
docs/adr/0034-transactional-writes-and-ambient-transaction.md) is Accepted — implemented (v8.0.0):ObjectQL.transaction()(packages/objectql/src/engine.ts:4934-4973) opens a driver transaction inside anAsyncLocalStoragestore, andbuildDriverOptions(:1257-1275) lifts that ambient handle onto every driver call. That is precisely the "middleware/hook semantics inside a transaction" design project the issue budgets for — a hook body, validation predicate or FK-resolution read issued during a transactional write already binds to the transaction's connection. ADR-0034 exists because not doing so deadlocked SQLite's single-connection pool.The issue's other premise is also stale here: it cites
driver-tursoprimitives as unsurfaced capability, but there is no Turso driver in this repo (onlydocs/design/driver-turso.md, Status: Proposal), and every in-repo driver already implementsbeginTransaction/commit/rollbackas required members ofIDataDriver.What was actually missing is declared reach.
IObjectQLEngine— theobjectqlslot contract — declares 25 members and nottransaction. So consumers reached around the type system:protocol.ts:2289(discovery probe),:7455-7458(inTxn), and a hand-declared member insys-metadata-repository.ts. Three unchecked structural claims about a class none of them import — exactly what that contract file was created to end, and its own evidence bar ("declared here only where a CROSS-PACKAGE consumer already calls it through the service slot") was met three times over.But transactions alone still don't close the issue. A million-row backfill can't hold one write-lock;
driver-memory.beginTransactiondeep-clones the whole database;transaction()binds only the default driver so multi-datasource writes escape silently; and a process crash defeats in-process compensation entirely. The four queued consumers need journal + compensation + resume on top of transactions. So the ADR splits the ask along where the work actually is, rather than choosing between two things that solve different halves.What lands here
D1 —
transactionjoinsIObjectQLEngine. Signature verbatim from the class, soimplementsstill checks and no engine behaviour changes. Required, not optional, per the contract's own rule. The three cast sites drop their casts; the two narrow host surfaces (MetadataHostEngine, the sys-metadata repository's engine) type their optional member asIObjectQLEngine['transaction']so they can't drift from the real signature. Both caveats — default-driver-only, and the silent no-rollback fallback when the driver lacksbeginTransaction— go into the TSDoc as declared meaning rather than behaviour to be discovered.D4 —
batchData'satomicbecomes real or refuses. It advertised "rollback entire batch on any failure (transaction mode)" and opened no transaction; it broke the loop (protocol.ts:5302-5305). Everything already written stayed written — and the response reported those rowssuccess: trueunder the one flag whose job is guaranteeing they were undone. That reporting, not just the missing transaction, was the damage. Now:engine.transaction(); first failure rolls back every prior write;ROLLED_BACK:/ the causal error /NOT_ATTEMPTED:;transaction(), or a default driver withoutbeginTransaction) is refused with501 NOT_IMPLEMENTEDrather than degraded — silent degradation is how the flag came to lie;atomicoutrankscontinueOnError(whose own description already scoped it toatomic=false);Declaration aligned to enforcement.
BatchOptionsSchema.atomicdeclared.default(true)while no site delivered atomicity, and REST forwards the original body so the parsed default never reached the loop. It becomesfalse— aligned down to what every site already does, not up to what none of them did. Honouring the oldtruewould have silently flipped every existing caller's failure semantics and hard-failed ordinary batches on non-transactional drivers. This is the same shape as thevalidateOnlytombstone already in that file.Behaviour change
Callers explicitly sending
atomic: truenow get real rollback, real501s on runtimes that can't transact, and a response that no longer credits rolled-back rows as successes. If you were passingatomic: trueand relying on partial results surviving a failure, that was the bug — useatomic: false(or omit it) for best-effort. Callers sending nothing are unaffected.A second commit trues up three hand-written API docs the docs-drift check flagged. They had accurately documented the old behaviour —
wire-format.mdxstated the non-rollback outright — so leaving them would have shipped the fix alongside docs describing the bug.deleteMany's note stays honest that itsatomicstill rolls nothing back, since that route did not get D4's treatment (#4620).Tests
10 unit pins (
packages/metadata-protocol/src/protocol.batch-atomic.test.ts): rollback shape and zero-success reporting, commit-path transaction threading,501refusal with zero writes attempted in both the no-transaction()and driver-can't-transact cases, atomic-beats-continueOnError, atomic upsert rethrow, and non-atomic regression pins.6 integration pins (
packages/objectql/src/protocol-batch-atomic.test.ts) over a realObjectQLplus a snapshot-transaction driver: rows genuinely gone after rollback, pre-existing rows untouched, one shared handle across every write, and an internal read during the batch binding to the open transaction — the no-deadlock coverage whose absence ADR-0034 was written about.Gates:
pnpm typecheck122/122; spec 7292, objectql 1612, metadata-protocol 216, rest 554 tests green;check:generated8/8 (the.describe()change regeneratedcontent/docs/references/api/batch.mdx);check:adr-anchorsOK with two new anchors.Follow-ups filed
sys_migration_journal+ runner in core): preflight → journal → chunked writes withchunk_donewritten inside each chunk's transaction → LIFO compensation → re-entrant resume. Specified in ADR-0118 D2 in implementable detail.IDataEngine.batch?(declared, implemented by nobody, called by nobody).engine.transactioncaveat hardening — silent degrade, default-driver-only, no owned-vs-joined signal.deleteManyDatahas the same fake-atomic;updateManyDataignoresatomicentirely; per-row result shape diverges fromBatchOperationResultSchema.D13 proceeds on its hand-rolled pattern and collapses onto the runner when #4617 lands, as the issue anticipated.