diff --git a/.changeset/adr-0118-plugin-reachable-transactions.md b/.changeset/adr-0118-plugin-reachable-transactions.md new file mode 100644 index 0000000000..1a18f5dbc8 --- /dev/null +++ b/.changeset/adr-0118-plugin-reachable-transactions.md @@ -0,0 +1,59 @@ +--- +"@objectstack/spec": minor +"@objectstack/metadata-protocol": minor +--- + +feat(spec,metadata-protocol): `IObjectQLEngine.transaction` joins the slot contract, and `batchData`'s `atomic` flag becomes real — rollback or refusal, never silent best-effort (ADR-0118 D1/D4, #4612) + +**D1 — the contract fix.** `ObjectQL.transaction()` — ADR-0034's ambient +transaction, shipped since v8.0.0 — was reachable from plugin space only +through `as unknown as` casts: the metadata protocol's atomic publish and its +`transactionalBatch` discovery probe, and the sys-metadata repository's +`withTxn`, each declared a private structural slice of an engine none of them +import. It is now declared on `IObjectQLEngine`, required per that contract's +own rule, with its caveats written into the TSDoc as part of the declared +meaning rather than left to be discovered: it covers the **default driver +only**, and when that driver has no `beginTransaction` the callback runs with +no transaction and no rollback. `MetadataHostEngine` and the sys-metadata +repository's engine surface now type their optional member as +`IObjectQLEngine['transaction']`, so a narrow host surface can no longer drift +from the real signature. Runtime `typeof === 'function'` probes stay — that is +test-double defence the type system does not replace. + +**D4 — the honesty fix.** `batchData`'s `options.atomic` promised "rollback +entire batch on any failure (transaction mode)" and delivered a `break` +statement. Every write before the failure stayed committed, and — the part that +did the real damage — the response reported those rows `success: true` under +the one flag whose job is to guarantee they were undone. + +Now an explicitly atomic batch runs inside ONE `engine.transaction()`: the +first failure rolls back every prior write, and the response says so +(`succeeded: 0`, with rows marked `ROLLED_BACK:` / the causal error / +`NOT_ATTEMPTED:`, and no row reporting success). On a runtime that cannot roll +back — no `transaction()`, or a default driver without `beginTransaction` — an +atomic request is **refused** with `501 NOT_IMPLEMENTED` rather than silently +degrading, matching the cross-object `/batch` route. `atomic` takes precedence +over `continueOnError`, whose own description already scoped it to +`atomic=false`. In atomic mode the upsert path no longer falls back to an +insert when its update throws: inside an aborted transaction that fallback can +only fail with a secondary error that buries the real cause. + +**Aligned declaration.** `BatchOptionsSchema.atomic` declared `.default(true)` +while no enforcement site delivered atomicity — and the REST route forwards the +original request body rather than the parsed output, so the declared default +never reached the loop at all. The default is now `false`: the declaration is +aligned down to what every site already does, rather than up to what none of +them did. Honouring the old `true` would have silently flipped the failure +semantics of every existing batch caller and hard-failed ordinary batches on +any driver that cannot transact. Callers who were explicitly sending +`atomic: true` now get what they always asked for; callers sending nothing keep +today's behaviour exactly. + +If you were passing `atomic: true` and relying on partial results surviving a +failure, that was the bug — switch to `atomic: false` (or omit it) for +best-effort semantics. + +ADR-0118 also rules on two items landing separately: D2 specifies a +framework-owned migration-journal runner for multi-step migrations too large +for one transaction, and D3 retires the declared-but-unimplemented +`IDataEngine.batch?`. diff --git a/content/docs/api/client-sdk.mdx b/content/docs/api/client-sdk.mdx index 188d3656a4..0bcb015c45 100644 --- a/content/docs/api/client-sdk.mdx +++ b/content/docs/api/client-sdk.mdx @@ -504,9 +504,13 @@ The `find` method accepts an options object with **canonical** (recommended) fie | Property | Type | Default | Description | |:---------|:-----|:--------|:------------| -| `atomic` | `boolean` | `true` | Rollback entire batch on any failure | +| `atomic` | `boolean` | `false` | Run the batch in one transaction and roll every write back on the first failure. Refused with `501 NOT_IMPLEMENTED` where the driver cannot roll back, rather than degrading to best-effort. Takes precedence over `continueOnError` | | `returnRecords` | `boolean` | `false` | Include full records in response | -| `continueOnError` | `boolean` | `false` | Continue after errors (when atomic=false) | +| `continueOnError` | `boolean` | `false` | Continue after errors (when atomic is false) | + +A rolled-back atomic batch reports `succeeded: 0`, with each row carrying +`ROLLED_BACK:`, the causal error, or `NOT_ATTEMPTED:` — no row is reported as a +success, because none of them survived. --- diff --git a/content/docs/api/data-api.mdx b/content/docs/api/data-api.mdx index e720c5ba2a..d4524ea04c 100644 --- a/content/docs/api/data-api.mdx +++ b/content/docs/api/data-api.mdx @@ -228,6 +228,8 @@ Execute a batch operation (create / update / upsert / delete) on multiple record **Response**: `BatchUpdateResponse` with `succeeded`, `failed`, `total`, and a per-record `results` array. Each entry in `results` has `id`, `success`, an optional `errors` array, and optional `data` (the full record, present when `returnRecords` is `true`). +`options.atomic` defaults to `false` (sequential best-effort, stopping at the first failure). Set it to `true` and the whole batch runs inside one transaction: the first failure rolls back every prior write, and the response reports `succeeded: 0` with each row marked `ROLLED_BACK:`, the causal error, or `NOT_ATTEMPTED:`. A deployment whose driver cannot roll back rejects an atomic request with `501 NOT_IMPLEMENTED` instead of running it best-effort — probe `capabilities.transactionalBatch` on `/discovery` first. `atomic` takes precedence over `continueOnError`. + ### `POST /data/:object/createMany` Batch create multiple records. @@ -260,16 +262,20 @@ ignored, on this route and on `deleteMany`. Batch delete records by ID list. -**Body**: `{ "ids": ["1", "2", "3"], "options": { "atomic": true } }` — `options` is +**Body**: `{ "ids": ["1", "2", "3"], "options": { "continueOnError": true } }` — `options` is the same `BatchOptions` bag `/batch` takes. The body is validated against the contract and unknown keys are dropped: the id list is the *only* thing that selects rows, so no body key can widen the delete into a filter. **Response**: `BatchUpdateResponse` — one `results` entry per id. Records are deleted one at a time by primary key, so each honours `deleteBehavior` -(`cascade` / `set_null` / `restrict`) on relations pointing at it. `atomic` -(default) stops the run at the first failure; `atomic: false` with -`continueOnError: true` processes the remaining ids and reports the failures. +(`cascade` / `set_null` / `restrict`) on relations pointing at it. The run stops +at the first failure; `continueOnError: true` processes the remaining ids and +reports the failures instead. + +Note that on this route `atomic` only stops the run — deletes already performed +are **not** rolled back. Unlike `/batch`, `deleteMany` has no wrapping +transaction yet. ### Batch size diff --git a/content/docs/api/wire-format.mdx b/content/docs/api/wire-format.mdx index 1ba1806f9d..d948370fb9 100644 --- a/content/docs/api/wire-format.mdx +++ b/content/docs/api/wire-format.mdx @@ -471,7 +471,11 @@ release group); any code reading them directly should move to `error.code`. **`POST /api/v1/data/task/batch`** -Process many records of a **single** operation type in one request. The body carries one `operation` (`create`, `update`, `upsert`, or `delete`) plus a `records` array. 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. Set `options.atomic: false` (with `options.continueOnError: true`) to keep processing every record and collect a full partial-success report. +Process many records of a **single** operation type in one request. The body carries one `operation` (`create`, `update`, `upsert`, or `delete`) plus a `records` array. + +By default (`options.atomic` omitted or `false`) processing stops at the first failing record, and records written earlier in the same batch are **not** rolled back — there is no wrapping transaction. Add `options.continueOnError: true` to keep going instead and collect a full partial-success report. + +Send `options.atomic: true` to run the whole batch inside one database transaction: the first failure rolls back every prior write, and the response reports zero successes with each row marked `ROLLED_BACK:`, the causal error, or `NOT_ATTEMPTED:`. A deployment whose driver cannot roll back **rejects** an atomic request with `501 NOT_IMPLEMENTED` rather than quietly running it best-effort — probe `capabilities.transactionalBatch` on `/discovery` to know in advance. `atomic` takes precedence over `continueOnError`. ### Request @@ -483,7 +487,7 @@ Process many records of a **single** operation type in one request. The body car { "id": "tsk_01HQ4B8C0E4G6H9K3L5M", "data": { "status": "done" } } ], "options": { - "atomic": true, + "atomic": false, "continueOnError": false } } @@ -509,7 +513,7 @@ The response is the `BatchUpdateResponse` envelope: a top-level `success` flag p ### Partial Failure Response -When `options.atomic: false` and some records fail, the failing entries carry a single `error` message string (not an array): +When the batch is not atomic and some records fail, the failing entries carry a single `error` message string (not an array). An atomic batch never returns this shape — it either commits everything or reports every row as failed: ```json { diff --git a/content/docs/references/api/batch.mdx b/content/docs/references/api/batch.mdx index 57a4347762..38e6799d54 100644 --- a/content/docs/references/api/batch.mdx +++ b/content/docs/references/api/batch.mdx @@ -86,7 +86,7 @@ const result = BatchConfigSchema.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **atomic** | `boolean` | ✅ | If true, rollback entire batch on any failure (transaction mode) | +| **atomic** | `boolean` | ✅ | Opt-in all-or-nothing. When explicitly true the whole batch runs inside ONE engine transaction: the first failure rolls back every prior write, and the response reports zero successes with rows marked ROLLED_BACK / NOT_ATTEMPTED. A runtime that cannot roll back REFUSES the request (501 NOT_IMPLEMENTED) rather than silently degrading to best-effort — probe `capabilities.transactionalBatch` on /discovery first. Takes precedence over continueOnError. Default false: sequential best-effort. | | **returnRecords** | `boolean` | ✅ | If true, return full record data in response | | **continueOnError** | `boolean` | ✅ | If true (and atomic=false), continue processing remaining records after errors | | **validateOnly** | `any` | optional | [REMOVED] `options.validateOnly` was removed from BatchOptions in @objectstack/spec (#4052). It was never implemented: the batch surfaces persisted regardless, so a "dry-run" would have silently executed. There is no dry-run today — drop the key. If you need to preview a batch without writing, open an issue so it can be designed (no-commit cascade / constraint semantics) and reintroduced as a flag that actually holds. | diff --git a/docs/adr/0118-plugin-reachable-transactions-and-honest-atomic-batch.md b/docs/adr/0118-plugin-reachable-transactions-and-honest-atomic-batch.md new file mode 100644 index 0000000000..1b73187113 --- /dev/null +++ b/docs/adr/0118-plugin-reachable-transactions-and-honest-atomic-batch.md @@ -0,0 +1,157 @@ +# ADR-0118: Multi-write atomicity is reachable through the contract, `atomic` means atomic or refuses, and migrations too big for one transaction get a journal runner + +**Status**: Accepted (2026-08-02) — D1/D4 implemented in this PR; D2 tracked in [#4617](https://github.com/objectstack-ai/objectstack/issues/4617); D3 tracked in [#4618](https://github.com/objectstack-ai/objectstack/issues/4618) +**Deciders**: ObjectStack Protocol Architects +**Builds on**: [ADR-0034](./0034-transactional-writes-and-ambient-transaction.md) (the ambient `AsyncLocalStorage` transaction D1 declares — this ADR adds no mechanism to it), [ADR-0067](./0067-commit-history-and-rollback-for-ai-authoring.md) (D2 — the join-don't-nest rule that makes an outer transaction the sole owner of commit/rollback), [ADR-0049](./0049-no-unenforced-security-properties.md) (enforce-or-remove — the disposition method applied to `batch?` in D3 and to the `atomic` flag in D4), [ADR-0087](./0087-metadata-protocol-upgrade-contract.md) (D3's replayable migration chain — the metadata-side analogue of the data-side runner D2 specifies), [ADR-0008](./0008-metadata-repository-and-change-log.md) (the JSONL change log — the journal shape D2 deliberately does *not* reuse), [ADR-0060](./0060-conformance-ledger-platform-pattern.md) (framework-owned ledger pattern — the precedent for `sys_migration_journal`), [ADR-0117](./0117-owning-business-unit-record-stamp.md) (D8 — backfill plus a fail-closed enable gate, the migration posture D2 and D4 both inherit), [ADR-0078](./0078-no-silently-inert-metadata.md) (no silently inert declarations — why D2 rejects a pluggable journal store) +**Consumers**: `@objectstack/spec` (`contracts/objectql-engine.ts`, `api/batch.zod.ts`), `@objectstack/metadata-protocol` (`protocol.ts`, `host-engine.ts`, `sys-metadata-repository.ts`), `@objectstack/objectql` (the implementation — unchanged by D1), `@objectstack/rest` (the `/batch` routes — unchanged), and for D2: `@objectstack/core`, `@objectstack/platform-objects` +**Surfaced by**: [#4612](https://github.com/objectstack-ai/objectstack/issues/4612) — four migration-class tools queued behind the same missing primitive, each hand-rolling journal + compensation + +--- + +## TL;DR + +#4612 asks the platform to choose a debt: transactionalize the engine, bless a shared migration-journal primitive, or rule both out. Scoping the question found that the first option's expensive half was paid three majors ago — ADR-0034's ambient transaction is implemented, and hook/validation/internal reads already join it — while the *cheap* half was never done: `transaction` was never declared on the `objectql` slot contract, so plugin-space reaches it through structural casts and tools cannot reach it at all. + +So this ADR does not pick one of the three. It separates what the four consumers actually need into what is already built (transactions), what is a five-line contract fix (D1), what genuinely needs new machinery because transactions cannot span it (D2 — the journal runner), and what is rot found on the way (D3, D4). + +- **D1** — `transaction` joins `IObjectQLEngine`. The mechanism ships; only the declaration was missing. +- **D2** — a framework-owned migration-journal runner, specified here, implemented in #4617. Transactions are the per-chunk unit; the journal is what survives a crash. +- **D3** — `IDataEngine.batch?` is retired: declared, implemented by nobody, called by nobody (#4618). +- **D4** — `batchData`'s `atomic` flag becomes real or refuses. It opened no transaction; it broke a loop. + +## Context + +### The premise that did not survive scoping + +#4612 prices option 1 as "a real design project, not a plumbing task", naming middleware/hook semantics inside a transaction, cache invalidation on rollback, and cross-driver support. That was the correct price in the abstract. It is not the price here, because ADR-0034 already paid it: + +`ObjectQL.transaction()` (`packages/objectql/src/engine.ts:4934-4973`) opens a driver transaction and runs the callback inside an `AsyncLocalStorage` store; `buildDriverOptions` (`:1257-1275`) lifts that ambient handle onto **every** driver call. The consequence is precisely the hard part the issue budgets for: a hook body, a validation predicate, an FK-resolution read, or any nested `engine.*` call issued during a transactional write automatically binds to that transaction's connection. ADR-0034 exists because *not* doing this deadlocked SQLite's single-connection pool — the failure was found, fixed, and pinned. + +The issue's second premise is also stale in this repo: it cites `driver-turso` primitives at `turso-driver.ts:764-776` and `remote-transport.ts:430-443` as evidence the capability exists but is unsurfaced. There is no Turso driver here — only `docs/design/driver-turso.md` (Status: Proposal). The in-repo drivers need no surfacing work either: `beginTransaction` / `commit` / `rollback` are **required** members of `IDataDriver` (`packages/spec/src/contracts/data-driver.ts:221-235`), implemented by driver-sql (`sql-driver.ts:2214+`), driver-memory (`:595+`), and driver-mongodb (`:545+`). + +### The gap that is real: declared reach + +`IObjectQLEngine` (`packages/spec/src/contracts/objectql-engine.ts`) is the contract of the `objectql` service slot — what a plugin sees through `ctx.getService('objectql')`. It declares 25 members and not `transaction`. `IDataEngine`, behind the `data` slot, has no transaction member either. So a plugin typed to either contract cannot see the primitive, and the consumers that need it reach around the type system: + +- `packages/metadata-protocol/src/protocol.ts:2289` — the `/discovery` capability probe, `typeof (this.engine as { transaction?: unknown })?.transaction === 'function'`; +- `:7455-7458` — `publishPackageDrafts`' `inTxn`, a `typeof` probe plus `as unknown as { transaction: (fn) => Promise }`; +- `packages/metadata-protocol/src/sys-metadata-repository.ts` — a hand-declared local `transaction?` member on its own engine stub. + +Each is an honest but **unchecked** claim about a class none of them import. This is the exact problem the contract file was created to end: its header records seven such local surfaces being merged into one checkable declaration, and states the bar for admitting a member — *"a member is declared here only where a CROSS-PACKAGE consumer already calls it through the service slot … Widening this is for whoever needs more, with the call site to prove it."* Three call sites already prove it. The evidence was there; nobody recorded it. + +Below plugin space it is worse. A migration-class tool runs behind `McpDataBridge` (`packages/mcp/src/mcp-http-tools.ts:48-81`) — one record per call, no transaction, no batch — or behind `IObjectQLEngine`. The wire has an atomic cross-object route (`POST {basePath}/batch`, ADR-0034's D4 deliverable), but a route is not callable in-process. Hence the issue's accurate observation that plugin-visible multi-row mutation is a JS loop of single-row writes. + +### Why transactions alone still do not close #4612 + +Granting every consumer `transaction()` would not let the four queued tools delete their journals: + +- A **million-row backfill** (ADR-0117 D8's shape) cannot hold one write transaction for its duration — on SQLite that is the single writer lock for the whole run. +- `driver-memory.beginTransaction` deep-clones the entire database (`memory-driver.ts:595-630`), so it is O(database) per begin. +- `ObjectQL.transaction()` binds only the **default** driver (`engine.ts:4950`). Objects routed elsewhere by `setDatasourceMapping` are written outside the transaction, silently. +- A **process crash** — as opposed to a thrown error — defeats in-process compensation entirely. The issue says this plainly and it is the decisive point: no amount of transaction plumbing produces recoverability across a `SIGKILL` mid-run. + +The honest shape is therefore *both*: the transaction is the unit of atomicity for a chunk; a durable journal is what makes a multi-chunk run recoverable. That is what the four consumers keep independently rediscovering, and what D2 standardizes. + +### Rot found on the way + +**`IDataEngine.batch?`** (`packages/spec/src/contracts/data-engine.ts:97-100`) — the member #4612 opens with. Optional, so nothing must implement it; no engine does. Its entire specification is the three-word comment `Batch Operations (Transactional)` — nothing about partial failure, ordering, cross-object references, rollback scope, or what `transaction: false` means. `DataEngineRequest` is imported by exactly one non-spec file: the contract declaring it. Its request union (`packages/spec/src/data/data-engine.zod.ts:706-717`) even nests batches recursively, a shape nobody designed against because nobody built it. The only test (`contracts/data-engine.test.ts:162-177`) constructs an ad-hoc literal with a `batch` property and asserts it is defined — it pins the type, not an implementation. + +**`batchData`'s `atomic` flag** (`packages/metadata-protocol/src/protocol.ts:5220-5323`) — advertised as *"rollback entire batch on any failure (transaction mode)"*. It opens no transaction. It breaks the loop (`:5302-5305`): + +```ts +if (options?.atomic) { + // Abort remaining operations on first failure in atomic mode + break; +} +``` + +Everything already written stays written, and the response reports those rows `success: true` under a flag whose one job is to guarantee they were undone. This is #4346's class exactly — a write-path guarantee that is declared and not enforced, silent and destructive when it matters — and #4612 cites #4346 as evidence this write path is sharp. It is the same edge. + +The declaration is inconsistent with itself too: `BatchOptionsSchema.atomic` declares `.default(true)` (`packages/spec/src/api/batch.zod.ts:62`) while no enforcement site delivers atomicity, and the REST route deliberately forwards the original body rather than the parsed output, so the declared default never reaches the loop. The same file already tombstoned `validateOnly` for this exact shape — a flag promising a data-safety guarantee it did not keep — calling it *"the worst shape of 'declared ≠ enforced'"*. + +## Decision + +### D1 — `transaction` joins the `objectql` slot contract + +`IObjectQLEngine` declares: + +```ts +transaction(callback: (trxCtx: any) => Promise, baseContext?: any): Promise; +``` + +verbatim from the class, so `ObjectQL implements IObjectQLEngine` continues to type-check and **no engine behaviour changes**. The member is REQUIRED, not optional, per the contract's own rule: it describes the slot's actual occupant, not a hypothetical minimal engine, and optional members only push guarded callers back toward the `any` this contract exists to remove. Callers that tolerate test doubles or foreign engines keep their `typeof === 'function'` runtime probes — that is defence the type system does not replace. + +The three cast sites drop their casts. The two narrow host surfaces that must stay tolerant of stubs — `MetadataHostEngine` and the sys-metadata repository's engine — declare their member as `transaction?: IObjectQLEngine['transaction']`, optional locally but **typed from the contract**, so a narrow surface can no longer drift from the real signature. + +Two caveats are written into the contract TSDoc as part of the member's declared meaning rather than left as behaviour a caller discovers: `transaction()` covers only the **default driver**, and when that driver lacks `beginTransaction` the callback runs **without a transaction and without rollback** (`engine.ts:4952-4954`). Declaring a caveat is not fixing it; tightening these is #4619. A caller that cannot tolerate silent degradation must fail closed itself — which is what D4 does. + +### D2 — the migration-journal runner is framework-owned (specified here, implemented in #4617) + +For multi-step data reshaping that cannot fit in one transaction — long backfills, DDL interleaved with DML, steps spanning datasources — the framework owns the runner. Migration tools consume it; they do not hand-roll compensation loops. The design, in enough detail to implement from: + +1. **Preflight dry-run.** Each plan step declares a read-only validator. The runner runs all validators before any write and refuses to start if any fails — ADR-0117 D8's fail-closed enable gate, generalized. +2. **Persistent journal.** A `sys_migration_journal` platform object, rows keyed `(run_id, seq)`, event kinds `run_started` (carrying the plan hash and chunk plan), `chunk_started(i)`, `chunk_done(i)`, `compensated(i)`, `run_done`, `run_failed`. +3. **Chunked writes.** Rows are chunked per the `packages/core/src/utils/bulk-write.ts` discipline. Each chunk's writes run inside `engine.transaction()`, and **`chunk_done(i)` is written inside that same transaction**, so `done ⇔ committed` is not a race. `chunk_started(i)` is written autonomously *before* it, making `started ∧ ¬done` mean exactly "outcome unknown" — the state a crash leaves behind. +4. **LIFO compensation.** On failure, walk completed chunks newest-first, running each step's declared `compensate` in its own transaction with a `compensated(i)` marker. A compensation failure journals and halts loudly; it is never swallowed. +5. **Re-entrant forward recovery.** On restart, scan for `run_started ∧ ¬run_done ∧ ¬fully-compensated` and resume forward from the first chunk lacking `chunk_done`, under a per-plan `onCrash: 'resume' | 'compensate'` policy. +6. **At-least-once, with idempotency made the caller's explicit job.** Forward and compensate callbacks receive an `attempt` counter; `attempt > 1` means the previous outcome is unknown and the callback must recheck by natural key before re-writing. This is verbatim the contract `bulk-write.ts` already documents — reuse it rather than re-deriving a second delivery-semantics story. +7. **Capability gate.** The runner refuses to start where real transactions are unavailable, using D4's probe. + +The runner lives in `@objectstack/core` beside `bulk-write.ts`, typed against `IObjectQLEngine` (which D1 makes sufficient); the object lives in `@objectstack/platform-objects`. + +**The journal is an engine-persisted platform object, not a caller-supplied `JournalStore` interface.** This is a decision, not an implementation detail, on three grounds. Recovery has to be re-entrant and discoverable with **zero host wiring** — a boot scanner needs one authoritative place to look for half-finished migrations, and a pluggable store fragments that authority per caller, which is ADR-0078's silently-inert failure mode in another costume: a journal nobody re-reads is a journal that does not exist. The journal is also data-plane state *about* the data plane, so keeping it in the same store puts it inside the same backup/restore and transaction boundary as the rows it describes — a side-file journal in the ADR-0008 JSONL shape desyncs from the database on restore, and ADR-0008 solved audit, which is a different problem from recovery authority. Finally, framework-owned `sys_*` tables with a registered schema are the established ledger pattern (ADR-0060). + +### D3 — `IDataEngine.batch?` is retired + +Declared, implemented by no engine, called by no caller, specified by three words. Under ADR-0049's enforce-or-remove posture the choice is implement it or delete it, and there is nothing to preserve: D1 gives in-process callers `transaction()`, D4 gives them an honest object-scoped atomic batch, and `POST {basePath}/batch` has served the wire since ADR-0034. Implementing it would mean designing partial-failure and nesting semantics for a shape no caller has ever asked for. + +Mechanical removal is #4618, following the `spec-property-retirement` playbook's contract-member route: it is a TypeScript contract member, not an authorable metadata key, so there is no `retiredKey` tombstone to leave — nothing can author it, and the removal is visible only to TypeScript consumers, who get the FROM → TO prescription in the changeset and upgrade guide. + +### D4 — `atomic` is real, or it is refused + +`batchData` honours `options.atomic === true` by running the whole batch inside one `engine.transaction()`. The first failure aborts and rolls back every prior write, and — the part that makes it honest — **the response says so**: `succeeded: 0`, and every row reports failure, with rows before the failure marked `ROLLED_BACK:`, the failing row carrying its causal error, and rows never reached marked `NOT_ATTEMPTED:`. A response claiming `success: true` for a row that was rolled back is the bug, not merely a missing transaction. + +Where the runtime **cannot** roll back — no `transaction()` on the engine, or a default driver without `beginTransaction` — an atomic request is **refused** with `501 NOT_IMPLEMENTED` rather than silently degrading to best-effort. This follows the cross-object `/batch` route's existing precedent (`rest-server.ts:7166-7171`) and uses the standard error catalog, so no new code enters the ADR-0112 ledger. Refusing is the whole point: silent degradation is how the flag came to lie in the first place, and a caller that asked for atomicity is exactly the caller who must not receive best-effort without being told. + +`atomic` takes precedence over `continueOnError` — whose own description already scopes it to `atomic=false`, making this precedence documented rather than new. In atomic mode the upsert path's defensive `catch { insert }` fallback rethrows instead of retrying, because inside an aborted transaction the fallback insert can only fail with a secondary error that masks the real cause. + +The declared default is aligned to the enforced one: `BatchOptionsSchema.atomic` becomes `.default(false)`. The direction matters. Making the runtime honour the declared `true` would flip every existing batch caller's failure semantics silently and fail-close every non-transactional deployment's ordinary batches; aligning the declaration to what every enforcement site already does changes nobody's behaviour, while ending the schema's false claim. Callers who were explicitly sending `atomic: true` now get what they always asked for. + +## Alternatives rejected + +**Declare `transaction` optional on `IObjectQLEngine`.** The contract header already settled this: optional members turn every guarded call into a `possibly undefined` error and push consumers back to `any`. The slot's occupant implements it; the contract describes that occupant. + +**Make the runtime honour `atomic`'s declared `true` default.** Changes the failure semantics of every existing batch caller without opt-in, and turns ordinary batches into hard failures on any deployment whose driver cannot transact. Aligning declaration to enforcement is the change that costs nobody. + +**Keep `atomic` best-effort with a documented caveat.** The flag's only job is the guarantee. A documented caveat on a data-safety flag is the `validateOnly` mistake this file already tombstoned. + +**A caller-supplied `JournalStore` for D2.** See D2 — it fragments recovery authority and reintroduces the silently-inert failure mode the journal exists to prevent. + +**Implement `IDataEngine.batch?` rather than retire it.** No caller has ever wanted its shape; D1 and D4 deliver its stated purpose with semantics somebody actually specified. + +**Do nothing and bless the hand-rolled pattern (#4612's option 3).** Rejected on the issue's own evidence: four consumers converging independently on the same shape is a platform gap, and the copies differ in exactly the places that matter — `ImportUndoLog` (`packages/rest/src/import-runner.ts:48-59`) journals per-row before-images; the publish path (`protocol.ts:7400-7424`) captures a revert plan; `batchData` captured nothing at all and said it did. + +## Consequences + +- **Positive.** Plugin-space reaches multi-write atomicity through a checked contract rather than three unchecked casts; a rename of the engine method now breaks the build instead of breaking production. `batchData`'s atomic mode either delivers or refuses. The `/discovery` `transactionalBatch` capability becomes a probe clients can trust, because the thing it probes is now contract-declared. The four queued consumers get a specified runner to collapse onto (#4617), and D13 can proceed on its hand-rolled pattern meanwhile, as #4612 anticipated. +- **Negative / cost.** Callers who were sending `atomic: true` and silently getting best-effort will now see rollbacks and `501`s — that is the fix working, but it is a behaviour change for anyone who had adapted to the bug. `@objectstack/spec` and `@objectstack/metadata-protocol` take a minor bump. +- **Risk.** D4's rolled-back response asserts prior operations were undone, which is locally guaranteed only when `batchData` **owns** the transaction. Under ADR-0067 D2 a nested call joins an outer transaction and does not own its rollback; today every `batchData` caller is top-level, so the claim holds, but nothing enforces that it keeps holding — tracked as part of #4619. +- **Deferred, deliberately.** `transaction()`'s silent degrade, default-driver-only scope, and missing owned-vs-joined signal (#4619); the same fake-atomic in `deleteManyData` and `updateManyData`, plus the per-row result shape's divergence from `BatchOperationResultSchema` (#4620). Declaring D1 must not wait on perfecting the caveats. + +## Test plan + +Unit (`packages/metadata-protocol/src/protocol.batch-atomic.test.ts`): + +1. Atomic batch whose second operation throws → the transaction is rolled back; the response reports `succeeded: 0` with `ROLLED_BACK:` / causal / `NOT_ATTEMPTED:` rows and no row reporting success; the third operation is never attempted. +2. Atomic batch that succeeds → committed once, and every operation received the open transaction handle in its context. +3. Engine without `transaction`, and engine whose default driver lacks `beginTransaction` → both refuse with `501 NOT_IMPLEMENTED`, having attempted **zero** writes. +4. `atomic` beats `continueOnError` when both are set. +5. Non-atomic batches behave exactly as before (no transaction opened; prior successes retained). +6. Atomic upsert whose update throws propagates rather than falling back to insert. + +Integration against a real `ObjectQL` with a transaction-capable driver (`packages/objectql/src/protocol-batch-atomic.test.ts`), covering ADR-0034's test-plan spirit end to end: + +7. Atomic multi-row batch commits every row, with one commit and one shared handle. +8. A poisoned row rolls back the whole batch — zero rows persisted. +9. An atomic upsert's internal `findOne` runs on the transaction's connection (the no-deadlock pin ADR-0034's absence of coverage originally cost us). +10. The engine typed as `IObjectQLEngine` can call `.transaction` with no cast — a compile-time pin on D1. diff --git a/packages/metadata-protocol/src/host-engine.ts b/packages/metadata-protocol/src/host-engine.ts index 6c1f4143a4..5952e660e0 100644 --- a/packages/metadata-protocol/src/host-engine.ts +++ b/packages/metadata-protocol/src/host-engine.ts @@ -1,6 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import type { IDataEngine } from '@objectstack/core'; +import type { IDataEngine, IObjectQLEngine } from '@objectstack/core'; /** * The engine surface the metadata protocol needs from its host (ADR-0076). @@ -19,6 +19,19 @@ export interface MetadataHostEngine extends IDataEngine { syncObjectSchema(...args: any[]): Promise; /** DDL: drop the physical table for an object schema. */ dropObjectSchema(...args: any[]): Promise; + /** + * ObjectQL's ambient transaction (ADR-0034), typed off the `objectql` slot + * contract (ADR-0118 D1) so this narrow host surface cannot drift from the + * real signature. Declared explicitly because an explicit member beats the + * index signature below — structurally `[key: string]: any` would type it + * `any` and hide exactly the mistakes this contract exists to catch. + * + * Optional HERE, unlike on `IObjectQLEngine` where it is required: a host may + * be a test double or a metadata-only store. Callers keep their runtime + * probes and must say what degrading means at their seam — a caller that + * cannot lose atomicity silently fails closed (see `batchData`, ADR-0118 D4). + */ + transaction?: IObjectQLEngine['transaction']; // Protocol accesses additional engine members structurally; keep it permissive // for this relocation (behavior unchanged — the concrete engine is injected). [key: string]: any; diff --git a/packages/metadata-protocol/src/protocol.batch-atomic.test.ts b/packages/metadata-protocol/src/protocol.batch-atomic.test.ts new file mode 100644 index 0000000000..aa217fb8f4 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.batch-atomic.test.ts @@ -0,0 +1,294 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// ADR-0118 D4 (#4612) — `batchData`'s `atomic` flag is REAL or REFUSED. +// +// It used to be neither. The flag advertised "rollback entire batch on any +// failure (transaction mode)" and opened no transaction at all: it `break`-ed +// the loop, so every write before the failure stayed committed while the +// response reported those rows `success: true` under a flag whose one job is +// to guarantee they were undone. Same class as #4346 — a write-path guarantee +// declared and not enforced, silent and destructive exactly when it matters. +// +// These pins are the regression net that class of bug needs, and the write-path +// ones matter most: a regression here is invisible until someone's half-batch +// is already in the database. + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { + name: 'invoice', + fields: { + title: { name: 'title', type: 'text' }, + }, +}; + +/** Marks a row the fake engine should reject, so a failure lands at a chosen index. */ +const POISON = '__explode__'; + +/** + * A fake engine that transacts. `transaction()` mirrors the real + * `ObjectQL.transaction` contract: run the callback with a handle-carrying + * context, commit on resolve, roll back and re-throw on reject. + */ +function makeTransactionalEngine(opts: { driverCanTransact?: boolean } = {}) { + const { driverCanTransact = true } = opts; + const commits: unknown[] = []; + const rollbacks: unknown[] = []; + const handle = { id: 'trx-1' }; + + const insert = vi.fn(async (_object: string, data: any) => { + if (data?.title === POISON) throw new Error('insert exploded'); + return { id: `rec-${insert.mock.calls.length}`, ...data }; + }); + const update = vi.fn(async (_object: string, data: any, options?: any) => { + if (data?.title === POISON) throw new Error('update exploded'); + return { id: options?.where?.id, ...data }; + }); + const findOne = vi.fn(async (_object: string, options?: any) => ({ id: options?.where?.id })); + const del = vi.fn(async () => ({ deleted: 1 })); + + const engine: any = { + registry: { getObject: () => SCHEMA }, + insert, + update, + findOne, + delete: del, + getDefaultDriverName: () => 'default', + getDriverByName: () => (driverCanTransact ? { beginTransaction: async () => handle } : {}), + transaction: vi.fn(async (callback: (ctx: any) => Promise, baseContext?: any) => { + const trxCtx = { ...(baseContext ?? {}), transaction: handle }; + try { + const result = await callback(trxCtx); + commits.push(handle); + return result; + } catch (err) { + rollbacks.push(handle); + throw err; + } + }), + }; + return { engine, insert, update, findOne, del, commits, rollbacks, handle }; +} + +describe('batchData atomic — rollback is real and the response admits it (ADR-0118 D4)', () => { + it('rolls back the whole batch on the first failure and reports ZERO successes', async () => { + const t = makeTransactionalEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'invoice', + request: { + operation: 'create', + records: [{ data: { title: 'A' } }, { data: { title: POISON } }, { data: { title: 'C' } }], + options: { atomic: true }, + }, + } as any); + + // The transaction was opened once and ROLLED BACK — not committed. + expect(t.engine.transaction).toHaveBeenCalledTimes(1); + expect(t.rollbacks).toHaveLength(1); + expect(t.commits).toHaveLength(0); + + // The third row is never attempted: two inserts, not three. + expect(t.insert).toHaveBeenCalledTimes(2); + + // The heart of the fix. Row 0 DID succeed against the engine, but the + // rollback undid it — so the response must not call it a success. + expect(res.success).toBe(false); + expect(res.succeeded).toBe(0); + expect(res.failed).toBe(3); + expect(res.total).toBe(3); + expect(res.results.every((r: any) => r.success === false)).toBe(true); + + expect(res.results[0].error).toMatch(/^ROLLED_BACK:/); + expect(res.results[0].error).toContain('insert exploded'); // carries the cause + expect(res.results[1].error).toBe('insert exploded'); // the causal row, verbatim + expect(res.results[2].error).toMatch(/^NOT_ATTEMPTED:/); + }); + + it('commits when every row succeeds, and every operation runs on the transaction handle', async () => { + const t = makeTransactionalEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + const ctx = { userId: 'u1' }; + + const res: any = await p.batchData({ + object: 'invoice', + request: { + operation: 'create', + records: [{ data: { title: 'A' } }, { data: { title: 'B' } }, { data: { title: 'C' } }], + options: { atomic: true }, + }, + context: ctx, + } as any); + + expect(t.commits).toHaveLength(1); + expect(t.rollbacks).toHaveLength(0); + expect(res.success).toBe(true); + expect(res.succeeded).toBe(3); + expect(res.failed).toBe(0); + + // Every write must carry the OPEN transaction, not the caller's bare + // context — otherwise it commits outside the batch and the rollback + // above would silently spare it. + expect(t.insert).toHaveBeenCalledTimes(3); + for (const call of t.insert.mock.calls) { + expect(call[2].context.transaction).toBe(t.handle); + expect(call[2].context.userId).toBe('u1'); // caller identity preserved + } + }); + + it('rolls back an update batch too, restoring nothing as successful', async () => { + const t = makeTransactionalEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'invoice', + request: { + operation: 'update', + records: [ + { id: 'rec-1', data: { title: 'A' } }, + { id: 'rec-2', data: { title: POISON } }, + ], + options: { atomic: true }, + }, + } as any); + + expect(t.rollbacks).toHaveLength(1); + expect(res.succeeded).toBe(0); + expect(res.results[0].error).toMatch(/^ROLLED_BACK:/); + expect(res.results[0].id).toBe('rec-1'); // ids survive so a caller can reconcile + expect(res.results[1].error).toBe('update exploded'); + }); +}); + +describe('batchData atomic — refuses rather than degrading (ADR-0118 D4)', () => { + it('refuses with 501 when the engine has no transaction(), attempting NO writes', async () => { + const t = makeTransactionalEngine(); + delete t.engine.transaction; + const p = new ObjectStackProtocolImplementation(t.engine); + + await expect(p.batchData({ + object: 'invoice', + request: { operation: 'create', records: [{ data: { title: 'A' } }], options: { atomic: true } }, + } as any)).rejects.toMatchObject({ status: 501, code: 'NOT_IMPLEMENTED' }); + + // Refusing means refusing: not one row may land before the complaint. + expect(t.insert).not.toHaveBeenCalled(); + }); + + it('refuses when the default driver cannot begin a transaction, even though the engine exposes transaction()', async () => { + // The subtle case: `engine.transaction()` silently runs the callback + // with NO transaction when the driver lacks `beginTransaction` + // (ADR-0118 D1's declared caveat). Probing only the engine would let + // "atomic" go back to meaning best-effort precisely here. + const t = makeTransactionalEngine({ driverCanTransact: false }); + const p = new ObjectStackProtocolImplementation(t.engine); + + await expect(p.batchData({ + object: 'invoice', + request: { operation: 'create', records: [{ data: { title: 'A' } }], options: { atomic: true } }, + } as any)).rejects.toMatchObject({ status: 501, code: 'NOT_IMPLEMENTED' }); + + expect(t.insert).not.toHaveBeenCalled(); + expect(t.engine.transaction).not.toHaveBeenCalled(); + }); +}); + +describe('batchData atomic — precedence and opt-in (ADR-0118 D4)', () => { + it('atomic outranks continueOnError: the batch aborts and rolls back instead of continuing', async () => { + const t = makeTransactionalEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'invoice', + request: { + operation: 'create', + records: [{ data: { title: POISON } }, { data: { title: 'B' } }, { data: { title: 'C' } }], + options: { atomic: true, continueOnError: true }, + }, + } as any); + + expect(t.insert).toHaveBeenCalledTimes(1); // rows 2 and 3 never attempted + expect(t.rollbacks).toHaveLength(1); + expect(res.succeeded).toBe(0); + expect(res.results[1].error).toMatch(/^NOT_ATTEMPTED:/); + }); + + it('an atomic upsert whose update throws propagates instead of falling back to insert', async () => { + // Inside an aborted transaction the fallback insert can only fail with + // a secondary error that buries the real cause. + const t = makeTransactionalEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'invoice', + request: { + operation: 'upsert', + records: [{ id: 'rec-1', data: { title: POISON } }], + options: { atomic: true }, + }, + } as any); + + expect(t.rollbacks).toHaveLength(1); + expect(t.insert).not.toHaveBeenCalled(); // no blind fallback + expect(res.results[0].error).toBe('update exploded'); // the real cause survives + }); +}); + +describe('batchData non-atomic — unchanged (ADR-0118 D4 regression net)', () => { + it('opens no transaction and keeps prior successes when atomic is absent', async () => { + const t = makeTransactionalEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'invoice', + request: { + operation: 'create', + records: [{ data: { title: 'A' } }, { data: { title: POISON } }, { data: { title: 'C' } }], + }, + } as any); + + expect(t.engine.transaction).not.toHaveBeenCalled(); + expect(res.succeeded).toBe(1); + expect(res.failed).toBe(1); + expect(res.results[0].success).toBe(true); // committed, and honestly reported + expect(res.results[1].success).toBe(false); + expect(res.results).toHaveLength(2); // stops without continueOnError + }); + + it('atomic: false is best-effort, not a refusal, even on a non-transactional engine', async () => { + const t = makeTransactionalEngine(); + delete t.engine.transaction; + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'invoice', + request: { + operation: 'create', + records: [{ data: { title: 'A' } }], + options: { atomic: false }, + }, + } as any); + + expect(res.succeeded).toBe(1); + }); + + it('continueOnError still processes every row when not atomic', async () => { + const t = makeTransactionalEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'invoice', + request: { + operation: 'create', + records: [{ data: { title: POISON } }, { data: { title: 'B' } }, { data: { title: 'C' } }], + options: { continueOnError: true }, + }, + } as any); + + expect(t.insert).toHaveBeenCalledTimes(3); + expect(res.succeeded).toBe(2); + expect(res.failed).toBe(1); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index f564e36adf..e8132efb33 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -810,6 +810,18 @@ function mergeDroppedFieldEvents(events: DroppedFieldsEvent[]): DroppedFieldsEve return Array.from(byKey.values()).map((b) => ({ object: b.object, fields: Array.from(b.fields), reason: b.reason })); } +/** + * One row of a `batchData` result. Deliberately the shape the implementation + * has always emitted (`error: string`, `record`), which diverges from + * `BatchOperationResultSchema`'s `errors: ApiError[]` / `data` — reconciling + * the two is a wire-visible change that must not ride along on a bug fix + * (ADR-0118 D4; tracked separately). + */ +type BatchDataRowResult = { id?: string; success: boolean; error?: string; record?: any; droppedFields?: DroppedFieldsEvent[] }; + +/** What one pass of the `batchData` record loop produced (ADR-0118 D4). */ +type BatchDataLoopOutcome = { results: BatchDataRowResult[]; succeeded: number; failed: number }; + /** * The canonical `QueryAST` surface (`spec/data/query.zod.ts`), enumerated. * @@ -2286,7 +2298,9 @@ export class ObjectStackProtocolImplementation implements // honour a transaction, so `declared === enforced` (Prime Directive // #10). The rest-server producer ANDs this with `api.enableBatch` so // a server that doesn't mount the route reports `false` at its layer. - transactionalBatch: typeof (this.engine as { transaction?: unknown })?.transaction === 'function', + // (ADR-0118 D1: `transaction` is contract-declared, so this probe + // no longer needs a structural cast to ask the question.) + transactionalBatch: typeof this.engine?.transaction === 'function', }; // Convert flat booleans → hierarchical capability objects @@ -5221,9 +5235,6 @@ export class ObjectStackProtocolImplementation implements const { object, request: batchReq, context } = request; this.assertObjectRegistered(object); // [#3770] const { operation, records, options } = batchReq; - const results: Array<{ id?: string; success: boolean; error?: string; record?: any; droppedFields?: DroppedFieldsEvent[] }> = []; - let succeeded = 0; - let failed = 0; // [#3043] The batch endpoint is an external ingress: strip forged // read-only columns on create. [#3455] It DOES resolve an execution @@ -5232,6 +5243,118 @@ export class ObjectStackProtocolImplementation implements // system caller is correctly exempt (the pre-#3455 code hard-coded the // strip context to `undefined`, treating every batch create as non-system). const batchSchema = this.engine.registry?.getObject(object); + + // ADR-0118 D4 — `atomic` is REAL or REFUSED, never silent best-effort. + // This flag used to only `break` the loop: every write before the + // failure stayed COMMITTED while the response called itself atomic and + // reported those rows `success: true`. Same class as #4346 — a + // write-path guarantee declared but not enforced, silent and + // destructive exactly when it matters. + // + // Opt-in is an explicit `=== true`. `BatchOptionsSchema` declared + // `.default(true)` while no enforcement site ever delivered atomicity + // (REST forwards the original body, so the parsed default never reached + // this loop), so treating "absent" as atomic would silently flip every + // existing caller's failure semantics. The declaration is aligned to + // the enforced value instead; see the schema's note. + if (options?.atomic === true) { + return await this.runAtomicBatchData({ object, operation, records, options, batchSchema, context }); + } + + const outcome = await this.runBatchDataLoop({ object, operation, records, options, batchSchema, context, atomic: false }); + return this.buildBatchDataResponse(operation, records, options, outcome); + } + + /** + * The atomic arm of {@link batchData} (ADR-0118 D4): the whole batch runs + * inside ONE `engine.transaction()`, so the first failure rolls back every + * prior write — and the response says so, rather than reporting rows that + * no longer exist as successes. + */ + private async runAtomicBatchData(args: { + object: string; + operation: BatchUpdateRequest['operation']; + records: BatchUpdateRequest['records']; + options: BatchUpdateRequest['options']; + batchSchema: any; + context: any; + }): Promise { + const { object, operation, records, options, batchSchema, context } = args; + + const engineTx = typeof this.engine?.transaction === 'function' + ? this.engine.transaction.bind(this.engine) + : undefined; + // Two-level probe. `engine.transaction()` runs the callback with NO + // transaction and NO rollback when the default driver lacks + // `beginTransaction` — a declared caveat of the contract member + // (ADR-0118 D1), and one that would turn "atomic" back into a lie + // precisely where it matters. So where the driver registry is + // inspectable, the driver is checked too; where it is not (test + // doubles), the engine-level probe is all there is. + const defaultDriverName = this.engine.getDefaultDriverName?.(); + const defaultDriver = defaultDriverName ? this.engine.getDriverByName?.(defaultDriverName) : undefined; + const driverCanTransact = !defaultDriver || typeof (defaultDriver as any).beginTransaction === 'function'; + + if (!engineTx || !driverCanTransact) { + // REFUSE, do not degrade. A caller that asked for atomicity is + // exactly the caller who must not silently receive best-effort — + // silent degradation is how this flag came to lie in the first + // place. Mirrors the cross-object /batch route's refusal; the + // condition is generic, so it uses the standard error catalog + // rather than registering an ADR-0112 synonym. + const err: any = new Error( + `Atomic batch on '${object}' requires engine transaction support; this runtime cannot roll back. ` + + `Retry without options.atomic, or probe capabilities.transactionalBatch on /discovery first.`, + ); + err.status = 501; + err.code = 'NOT_IMPLEMENTED'; + throw err; + } + + // Identity-checked sentinel: aborting the transaction is how a rollback + // is requested, but the abort itself is not an error to propagate. + const ABORT = new Error('atomic batch aborted — rolled back'); + let aborted: BatchDataLoopOutcome | undefined; + try { + return await engineTx(async (trxCtx: any) => { + const outcome = await this.runBatchDataLoop({ object, operation, records, options, batchSchema, context: trxCtx, atomic: true }); + if (outcome.failed > 0) { + aborted = outcome; + throw ABORT; + } + return this.buildBatchDataResponse(operation, records, options, outcome); + }, context); + } catch (err) { + if (err === ABORT && aborted) { + return this.buildRolledBackBatchResponse(operation, records, aborted); + } + throw err; + } + } + + /** + * The per-record loop, shared by both arms of {@link batchData} (ADR-0118 + * D4) so atomic and non-atomic cannot drift apart. `atomic` changes exactly + * two things: it aborts on the first failure regardless of + * `continueOnError` (whose own contract text already scopes it to + * `atomic=false`), and it forbids the upsert fallback — inside an aborted + * transaction a fallback insert can only fail with a secondary error that + * masks the real cause. + */ + private async runBatchDataLoop(args: { + object: string; + operation: BatchUpdateRequest['operation']; + records: BatchUpdateRequest['records']; + options: BatchUpdateRequest['options']; + batchSchema: any; + context: any; + atomic: boolean; + }): Promise { + const { object, operation, records, options, batchSchema, context, atomic } = args; + const results: BatchDataRowResult[] = []; + let succeeded = 0; + let failed = 0; + // Spread form for options objects that already carry `where`/`onFieldsDropped` // (`{}` spread is a safe no-op); arg form for `insert`, whose whole options // arg is `undefined` when there is no context — exact parity with createData. @@ -5274,7 +5397,13 @@ export class ObjectStackProtocolImplementation implements const created = await this.engine.insert(object, { id: record.id, ...(record.data || {}) }, insertCtx as any); results.push({ id: created.id, success: true, record: created }); } - } catch { + } catch (err) { + // ADR-0118 D4 — no blind fallback inside a + // transaction: once the failing statement has + // aborted it, this insert can only fail with a + // secondary error ("current transaction is + // aborted") that buries the real cause. + if (atomic) throw err; const created = await this.engine.insert(object, { id: record.id, ...(record.data || {}) }, insertCtx as any); results.push({ id: created.id, success: true, record: created }); } @@ -5299,8 +5428,10 @@ export class ObjectStackProtocolImplementation implements } catch (err: any) { results.push({ id: record.id, success: false, error: err.message }); failed++; - if (options?.atomic) { - // Abort remaining operations on first failure in atomic mode + if (atomic) { + // Abort on the first failure; the caller rolls back. Atomic + // outranks `continueOnError` — there is nothing to continue + // toward when every write so far is about to be undone. break; } if (!options?.continueOnError) { @@ -5309,6 +5440,17 @@ export class ObjectStackProtocolImplementation implements } } + return { results, succeeded, failed }; + } + + /** The ordinary (committed) batch response — every row reports what it did. */ + private buildBatchDataResponse( + operation: BatchUpdateRequest['operation'], + records: BatchUpdateRequest['records'], + options: BatchUpdateRequest['options'], + outcome: BatchDataLoopOutcome, + ): BatchUpdateResponse { + const { results, succeeded, failed } = outcome; return { success: failed === 0, operation, @@ -5321,7 +5463,49 @@ export class ObjectStackProtocolImplementation implements results: options?.returnRecords !== false ? results : results.map(r => ({ id: r.id, success: r.success, error: r.error, ...(r.droppedFields ? { droppedFields: r.droppedFields } : {}) })), } as BatchUpdateResponse; } - + + /** + * The response for an atomic batch that rolled back (ADR-0118 D4). + * + * Nothing persisted, so nothing may report success — the old code's real + * damage was not the missing transaction alone but telling the caller that + * rows it had just undone were `success: true`. Rows are classified from + * what actually happened: a row that had succeeded is now `ROLLED_BACK`, + * the row that failed keeps its causal error, and rows the abort never + * reached are `NOT_ATTEMPTED`. `returnRecords` is moot — no record exists + * to return, and a `droppedFields` warning about a reverted write would + * only mislead. + */ + private buildRolledBackBatchResponse( + operation: BatchUpdateRequest['operation'], + records: BatchUpdateRequest['records'], + outcome: BatchDataLoopOutcome, + ): BatchUpdateResponse { + const attempted = outcome.results; + const causeIndex = attempted.findIndex(r => !r.success); + const cause = causeIndex >= 0 ? attempted[causeIndex]?.error : undefined; + + const results: BatchDataRowResult[] = records.map((record, i) => { + const attempt = attempted[i]; + if (!attempt) { + return { id: record.id, success: false, error: `NOT_ATTEMPTED: atomic batch aborted by record ${causeIndex}` }; + } + if (attempt.success) { + return { id: attempt.id ?? record.id, success: false, error: `ROLLED_BACK: record ${causeIndex} failed — ${cause ?? 'unknown error'}` }; + } + return { id: attempt.id ?? record.id, success: false, error: attempt.error }; + }); + + return { + success: false, + operation, + total: records.length, + succeeded: 0, + failed: records.length, + results, + } as BatchUpdateResponse; + } + async createManyData(request: { object: string, records: any[], context?: any }): Promise { this.assertObjectRegistered(request.object); // [#3770] // [#3043] Ingress-level static-`readonly` strip (per row) — mirrors @@ -7452,10 +7636,14 @@ export class ObjectStackProtocolImplementation implements const promoted: PromotedDraft[] = []; // (assigned inside the transaction closure — keep the wide type) let commit = null as { commitId: string } | null; + // ADR-0118 D1 — `transaction` is contract-declared, so this reaches it + // by name instead of through a structural cast. Bound once up front: + // the probe and the call must agree on one resolved function. + const engineTx = typeof this.engine?.transaction === 'function' + ? this.engine.transaction.bind(this.engine) + : undefined; const inTxn: (cb: () => Promise) => Promise = - typeof (this.engine as { transaction?: unknown })?.transaction === 'function' - ? (cb) => (this.engine as unknown as { transaction: (fn: () => Promise) => Promise }).transaction(() => cb()) - : (cb) => cb(); + engineTx ? (cb) => engineTx(() => cb()) : (cb) => cb(); try { await inTxn(async () => { for (const d of ordered) { diff --git a/packages/metadata-protocol/src/sys-metadata-repository.ts b/packages/metadata-protocol/src/sys-metadata-repository.ts index 2c1ab47210..20bcbacddf 100644 --- a/packages/metadata-protocol/src/sys-metadata-repository.ts +++ b/packages/metadata-protocol/src/sys-metadata-repository.ts @@ -68,6 +68,7 @@ import type { } from '@objectstack/metadata-core'; import { DEFAULT_METADATA_TYPE_REGISTRY } from '@objectstack/spec/kernel'; import { PLURAL_TO_SINGULAR, SINGULAR_TO_PLURAL } from '@objectstack/spec/shared'; +import type { IObjectQLEngine } from '@objectstack/core'; /** * Overlay-row lifecycle state. @@ -126,8 +127,11 @@ export interface SysMetadataEngine { * underlying driver lacks ACID support (matches the real * `ObjectQL.transaction` semantics). Repository code must not rely on * rollback for correctness against in-memory drivers. + * + * Typed off the `objectql` slot contract (ADR-0118 D1) rather than restated + * by hand, so this stub surface cannot drift from `ObjectQL.transaction`. */ - transaction?(callback: (trxCtx: any) => Promise, baseContext?: any): Promise; + transaction?: IObjectQLEngine['transaction']; } export interface SysMetadataRepositoryOptions { diff --git a/packages/objectql/src/protocol-batch-atomic.test.ts b/packages/objectql/src/protocol-batch-atomic.test.ts new file mode 100644 index 0000000000..8f4a1f8d4d --- /dev/null +++ b/packages/objectql/src/protocol-batch-atomic.test.ts @@ -0,0 +1,243 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// ADR-0118 (#4612), end-to-end: the real `ObjectQL` engine + the real metadata +// protocol + a driver whose transactions actually roll back. The unit pins in +// `metadata-protocol/src/protocol.batch-atomic.test.ts` prove `batchData` asks +// for the right things; these prove the stack delivers them — that the rows are +// genuinely gone after a rollback, and that internal reads issued during a +// transactional write bind to the open transaction rather than reaching for a +// second connection. +// +// That last one is the coverage whose absence ADR-0034 was written about: a +// nested query taking another connection deadlocks the single-connection SQLite +// pool, and no unit test with a fake engine can catch it. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import type { IObjectQLEngine } from '@objectstack/spec/contracts'; + +/** + * A driver with real transaction semantics: `beginTransaction` snapshots every + * table, `rollback` restores the snapshot wholesale, `commit` drops it — the + * same shape `driver-memory` uses, small enough to assert against. + */ +function makeSnapshotDriver() { + const stores = new Map>(); + const seen = { + create: [] as Array<{ object: string; transaction: unknown }>, + findOne: [] as Array<{ object: string; transaction: unknown }>, + begin: [] as unknown[], + commit: [] as unknown[], + rollback: [] as unknown[], + }; + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + const snapshots = new Map>>(); + let nextId = 0; + let nextTrx = 0; + + const driver: any = { + name: 'snapshot', + version: '0.0.0', + supports: { transactions: true }, + async connect() { }, + async disconnect() { }, + async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string) { return Array.from(storeFor(object).values()); }, + findStream() { throw new Error('not implemented'); }, + async findOne(object: string, ast: any, options: any) { + seen.findOne.push({ object, transaction: options?.transaction }); + const id = ast?.where?.id ?? ast?.filters?.id; + if (id) return storeFor(object).get(id) ?? null; + for (const r of storeFor(object).values()) return r; + return null; + }, + async create(object: string, data: Record, options: any) { + seen.create.push({ object, transaction: options?.transaction }); + // The poison row fails at the driver, the way a constraint would. + if (data.title === '__explode__') throw new Error('constraint violated'); + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const row = { ...s.get(id), ...data, id }; + s.set(id, row); + return row; + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r, undefined))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() { }, + async beginTransaction() { + nextTrx += 1; + const trx = { __trx: nextTrx }; + const snap = new Map>(); + for (const [name, rows] of stores) snap.set(name, new Map(rows)); + snapshots.set(trx, snap); + seen.begin.push(trx); + return trx; + }, + async commit(trx: unknown) { snapshots.delete(trx); seen.commit.push(trx); }, + async rollback(trx: unknown) { + const snap = snapshots.get(trx); + if (snap) { + stores.clear(); + for (const [name, rows] of snap) stores.set(name, new Map(rows)); + } + snapshots.delete(trx); + seen.rollback.push(trx); + }, + }; + return { driver, seen, rowsOf: (o: string) => Array.from(storeFor(o).values()) }; +} + +describe('atomic batchData over the real engine (ADR-0118 D4 / ADR-0034)', () => { + let engine: ObjectQL; + let protocol: ObjectStackProtocolImplementation; + let d: ReturnType; + + beforeEach(async () => { + engine = new ObjectQL(); + d = makeSnapshotDriver(); + engine.registerDriver(d.driver, true); + await engine.init(); + engine.registry.registerObject({ name: 'invoice', fields: { title: { type: 'text' } } } as any); + protocol = new ObjectStackProtocolImplementation(engine as any); + }); + + it('commits every row on success, on ONE transaction', async () => { + const res: any = await protocol.batchData({ + object: 'invoice', + request: { + operation: 'create', + records: [{ data: { title: 'A' } }, { data: { title: 'B' } }, { data: { title: 'C' } }], + options: { atomic: true }, + }, + } as any); + + expect(res.succeeded).toBe(3); + expect(d.rowsOf('invoice')).toHaveLength(3); + expect(d.seen.begin).toHaveLength(1); + expect(d.seen.commit).toHaveLength(1); + expect(d.seen.rollback).toHaveLength(0); + + // One transaction, shared by every write — not three. + const handle = d.seen.begin[0]; + for (const c of d.seen.create) expect(c.transaction).toBe(handle); + }); + + it('a failing row rolls the whole batch back — ZERO rows persist', async () => { + const res: any = await protocol.batchData({ + object: 'invoice', + request: { + operation: 'create', + records: [{ data: { title: 'A' } }, { data: { title: 'B' } }, { data: { title: '__explode__' } }], + options: { atomic: true }, + }, + } as any); + + // The two rows that succeeded against the driver are gone. + expect(d.rowsOf('invoice')).toHaveLength(0); + expect(d.seen.rollback).toHaveLength(1); + expect(d.seen.commit).toHaveLength(0); + + // …and the response does not claim them. + expect(res.success).toBe(false); + expect(res.succeeded).toBe(0); + expect(res.results.every((r: any) => r.success === false)).toBe(true); + expect(res.results[0].error).toMatch(/^ROLLED_BACK:/); + expect(res.results[2].error).toContain('constraint violated'); + }); + + it('leaves rows written BEFORE the batch untouched when it rolls back', async () => { + await engine.insert('invoice', { title: 'pre-existing' }); + expect(d.rowsOf('invoice')).toHaveLength(1); + + await protocol.batchData({ + object: 'invoice', + request: { + operation: 'create', + records: [{ data: { title: 'A' } }, { data: { title: '__explode__' } }], + options: { atomic: true }, + }, + } as any); + + // Rollback undoes the batch, not the world. + const rows = d.rowsOf('invoice'); + expect(rows).toHaveLength(1); + expect(rows[0].title).toBe('pre-existing'); + }); + + it('an internal read during the batch binds to the open transaction (no second connection)', async () => { + // The upsert path issues a findOne before writing. Under ADR-0034's + // ambient store it must run on the batch's transaction; if it asked the + // pool for another connection this is where a single-connection driver + // would deadlock. + await protocol.batchData({ + object: 'invoice', + request: { + operation: 'upsert', + records: [{ id: 'inv-1', data: { title: 'A' } }], + options: { atomic: true }, + }, + } as any); + + const handle = d.seen.begin[0]; + expect(d.seen.findOne.length).toBeGreaterThan(0); + for (const r of d.seen.findOne) expect(r.transaction).toBe(handle); + }); + + it('refuses atomic when the driver cannot transact, and writes nothing', async () => { + const bare = new ObjectQL(); + const plain = makeSnapshotDriver(); + delete plain.driver.beginTransaction; + bare.registerDriver(plain.driver, true); + await bare.init(); + bare.registry.registerObject({ name: 'invoice', fields: { title: { type: 'text' } } } as any); + const p = new ObjectStackProtocolImplementation(bare as any); + + await expect(p.batchData({ + object: 'invoice', + request: { operation: 'create', records: [{ data: { title: 'A' } }], options: { atomic: true } }, + } as any)).rejects.toMatchObject({ status: 501, code: 'NOT_IMPLEMENTED' }); + + expect(plain.rowsOf('invoice')).toHaveLength(0); + }); +}); + +describe('ADR-0118 D1 — transaction is reachable through the contract', () => { + it('calls transaction() on an engine typed as IObjectQLEngine, with no cast', async () => { + const engine = new ObjectQL(); + const d = makeSnapshotDriver(); + engine.registerDriver(d.driver, true); + await engine.init(); + engine.registry.registerObject({ name: 'invoice', fields: { title: { type: 'text' } } } as any); + + // The point of the pin is the TYPE, not the runtime: before ADR-0118 D1 + // this line could not compile — `transaction` was absent from the + // contract, so every cross-package consumer reached it through + // `as unknown as { transaction: ... }`. + const contract: IObjectQLEngine = engine; + const result = await contract.transaction(async (trxCtx: any) => { + expect(trxCtx.transaction).toBeTruthy(); + await engine.insert('invoice', { title: 'via-contract' }); + return 'ok'; + }); + + expect(result).toBe('ok'); + expect(d.seen.commit).toHaveLength(1); + expect(d.rowsOf('invoice')).toHaveLength(1); + }); +}); diff --git a/packages/spec/src/api/batch.test.ts b/packages/spec/src/api/batch.test.ts index 65c9f39b3a..6de9b107ce 100644 --- a/packages/spec/src/api/batch.test.ts +++ b/packages/spec/src/api/batch.test.ts @@ -54,7 +54,11 @@ describe('BatchOptionsSchema', () => { it('should use default values', () => { const options = BatchOptionsSchema.parse({}); - expect(options.atomic).toBe(true); + // ADR-0118 D4 — `atomic` defaults to FALSE. It declared `true` for as long + // as no batch surface honoured it; the declaration was aligned down to the + // enforced behaviour so that opting in is explicit and nobody's failure + // semantics changed silently. + expect(options.atomic).toBe(false); expect(options.returnRecords).toBe(false); expect(options.continueOnError).toBe(false); }); diff --git a/packages/spec/src/api/batch.zod.ts b/packages/spec/src/api/batch.zod.ts index c4ad12029a..ddc16534a7 100644 --- a/packages/spec/src/api/batch.zod.ts +++ b/packages/spec/src/api/batch.zod.ts @@ -59,7 +59,23 @@ export type BatchRecord = z.infer; * Configuration options for batch operations */ export const BatchOptionsSchema = lazySchema(() => z.object({ - atomic: z.boolean().optional().default(true).describe('If true, rollback entire batch on any failure (transaction mode)'), + // ADR-0118 D4. `atomic` declared `.default(true)` while NO enforcement site + // delivered atomicity: `batchData` merely broke its loop, leaving every prior + // write committed, and the REST route deliberately forwards the ORIGINAL body + // rather than the parsed output, so this default never reached the loop at + // all. Now that the flag is real, the declaration is aligned DOWN to what + // every site already does rather than up to what none of them did — honouring + // the old `true` would silently flip the failure semantics of every existing + // batch caller and hard-fail ordinary batches on any driver that cannot + // transact. Callers who were explicitly sending `atomic: true` now get what + // they asked for; callers sending nothing keep today's behaviour exactly. + atomic: z.boolean().optional().default(false).describe( + 'Opt-in all-or-nothing. When explicitly true the whole batch runs inside ONE engine transaction: ' + + 'the first failure rolls back every prior write, and the response reports zero successes with rows ' + + 'marked ROLLED_BACK / NOT_ATTEMPTED. A runtime that cannot roll back REFUSES the request ' + + '(501 NOT_IMPLEMENTED) rather than silently degrading to best-effort — probe ' + + '`capabilities.transactionalBatch` on /discovery first. Takes precedence over continueOnError. ' + + 'Default false: sequential best-effort.'), returnRecords: z.boolean().optional().default(false).describe('If true, return full record data in response'), continueOnError: z.boolean().optional().default(false).describe('If true (and atomic=false), continue processing remaining records after errors'), // `validateOnly` promised a dry-run — "validate records without persisting" — diff --git a/packages/spec/src/contracts/objectql-engine.ts b/packages/spec/src/contracts/objectql-engine.ts index b5470e8655..ec04cbe44e 100644 --- a/packages/spec/src/contracts/objectql-engine.ts +++ b/packages/spec/src/contracts/objectql-engine.ts @@ -163,4 +163,39 @@ export interface IObjectQLEngine extends IDataEngine { wasDatastoreCreatedFromEmpty(): boolean; /** Drop the memoized migration-flag reads (the attestation may race a fast boot's first read). */ invalidateDataMigrationFlags(): void; + + // ── Transactions (ADR-0118 D1) ─────────────────────────────────────── + /** + * Run `callback` inside ONE driver transaction — the ADR-0034 ambient + * transaction. The callback receives a context carrying the handle, which + * callers thread to downstream engine calls as `{ context: trxCtx }`; + * operations issued during the callback ALSO bind to it ambiently + * (`AsyncLocalStorage`), so hook bodies, validation predicates and internal + * reference reads reuse the transaction's connection without threading it + * by hand. Commit on resolve, rollback and re-throw on reject. A nested + * call JOINS the open transaction rather than opening a second one, leaving + * the outermost caller the sole owner of commit/rollback (ADR-0067 D2). + * + * Declared here under this file's evidence bar — three cross-package + * consumers already call it through the slot, each having reached around + * the type system to do so: the metadata protocol's atomic publish + * (`publishPackageDrafts`) and its `transactionalBatch` discovery probe, + * and the sys-metadata repository's `withTxn`. REQUIRED per this file's + * header; callers that tolerate test doubles keep their runtime + * `typeof === 'function'` probes, which types do not replace. + * + * TWO CAVEATS ARE PART OF THE DECLARED MEANING (ADR-0118 D1), not + * behaviour to be discovered: this covers the DEFAULT driver only — objects + * routed elsewhere by `setDatasourceMapping` are written outside it — and + * when that driver has no `beginTransaction` the callback runs with NO + * transaction and NO rollback. A caller that cannot tolerate silently + * losing atomicity must fail closed itself rather than assume it held; see + * `batchData`'s atomic gate (ADR-0118 D4). Tightening both is tracked by + * the ADR's follow-up. + * + * `trxCtx`/`baseContext` are the engine-local execution-context shape, left + * loose here per this file's edge-typing rule; consumers narrow at the call + * site. + */ + transaction(callback: (trxCtx: any) => Promise, baseContext?: any): Promise; } diff --git a/scripts/adr-anchors.json b/scripts/adr-anchors.json index 09d154078d..4f7a6aaf09 100644 --- a/scripts/adr-anchors.json +++ b/scripts/adr-anchors.json @@ -75,6 +75,16 @@ "file": "packages/plugins/plugin-security/src/permission-evaluator.ts", "adrs": ["ADR-0057", "ADR-0066"], "invariant": "The superuser bypass derives solely from a resolved set carrying the `objects['*']` wildcard with `viewAllRecords`/`modifyAllRecords` (ADR-0066 D2) — no stored boolean, no role fast-path. Scope depth (own/unit/unit_and_below/org) resolves to an effective access depth per operation class (ADR-0057 D1); an unenforceable depth is a compile error, never silent fail-open (ADR-0049)." + }, + { + "file": "packages/spec/src/contracts/objectql-engine.ts", + "adrs": ["ADR-0118"], + "invariant": "`transaction` is DECLARED on the `objectql` slot contract — plugin space reaches ADR-0034's ambient transaction by name, not through `as unknown as` casts. Required, not optional, per this file's own rule. Its two caveats (default-driver only; the callback runs with NO transaction when the driver lacks `beginTransaction`) are part of the declared meaning, so a caller that cannot lose atomicity silently must fail closed rather than assume it held." + }, + { + "file": "packages/metadata-protocol/src/protocol.ts", + "adrs": ["ADR-0118"], + "invariant": "`batchData`'s `atomic` is REAL or REFUSED, never silent best-effort. An explicitly atomic batch runs inside ONE `engine.transaction()`, so a failure rolls back every prior write and the response reports zero successes (rows marked ROLLED_BACK / NOT_ATTEMPTED — reporting a rolled-back row as `success: true` was the original bug, not merely the missing transaction). A runtime that cannot roll back gets 501 NOT_IMPLEMENTED; degrading to best-effort is how the flag came to lie." } ] }