diff --git a/.changeset/many-data-atomic-real-or-refused.md b/.changeset/many-data-atomic-real-or-refused.md new file mode 100644 index 0000000000..6b44eed594 --- /dev/null +++ b/.changeset/many-data-atomic-real-or-refused.md @@ -0,0 +1,40 @@ +--- +"@objectstack/metadata-protocol": minor +--- + +fix(metadata-protocol): `deleteMany` / `updateMany` honour `atomic` for real, or refuse it (#4620) + +ADR-0119 D4 made `batchData`'s `atomic` flag a real guarantee. Its two siblings +in the same file were out of that PR's confirmed scope and kept the defect: + +- **`deleteManyData` was fake-atomic.** `atomic: true` opened no transaction; it + only `break`-ed the loop, so every row deleted before the failure stayed + **deleted** while the response called itself atomic and reported those rows + `success: true`. Worse than the `batchData` case it was copied from, because a + partial delete has no natural undo — a client cannot reconstruct the rows from + its own request. +- **`updateManyData` ignored `atomic` entirely.** The option was accepted, + declared in `BatchOptionsSchema` with an all-or-nothing contract, and never + read: a caller asking for atomicity silently got best-effort, with no signal. + +Both now run the **same** atomic arm as `batchData`, extracted into one shared +runner so a fourth copy of transaction handling cannot drift into a fourth lie: + +- `atomic: true` runs the whole batch inside ONE `engine.transaction()`; the + first failure rolls back every prior write. +- A rolled-back batch reports **zero successes**. Rows that had succeeded are + marked `ROLLED_BACK: record failed — `, rows never reached are + `NOT_ATTEMPTED: atomic batch aborted by record `, and the causal row keeps + its own error — so a client can tell "attempted, undone" from "never ran". +- `atomic` outranks `continueOnError`, whose contract text already scoped it to + `atomic=false`. + +**Behaviour change to be aware of:** a runtime that cannot roll back (no +`engine.transaction()`, or a default driver without `beginTransaction`) now +**refuses** an `atomic: true` `deleteMany` / `updateMany` with `501 +NOT_IMPLEMENTED` instead of silently running best-effort — the same fail-closed +gate `batchData` uses. That silent downgrade is the defect class this fixes; if +you want best-effort, ask for it (`atomic: false`, or omit the option), or probe +the runtime's transaction support before sending. Non-atomic behaviour of both +endpoints — including the `continueOnError` interaction and their response +shapes — is unchanged. diff --git a/packages/metadata-protocol/src/protocol.delete-many.test.ts b/packages/metadata-protocol/src/protocol.delete-many.test.ts index ffb6656df9..56b2dae9ea 100644 --- a/packages/metadata-protocol/src/protocol.delete-many.test.ts +++ b/packages/metadata-protocol/src/protocol.delete-many.test.ts @@ -145,15 +145,19 @@ describe('deleteManyData — partial-failure semantics (#3897)', () => { expect(res).toMatchObject({ success: false, total: 3, succeeded: 2, failed: 1 }); }); - it('atomic aborts the remaining ids on the first failure', async () => { + // [#4620] This used to pin the fake-atomic: `atomic: true` merely broke the + // loop, so `a` stayed DELETED and the response reported `succeeded: 1` under + // a flag whose one job is to guarantee it was undone. On this engine — no + // `transaction()` at all — the honest answer is a refusal, not a half-batch. + // Real rollback is pinned in protocol.many-data-atomic.test.ts. + it('atomic REFUSES on an engine that cannot roll back, deleting nothing (#4620)', async () => { const { p, del } = failOn('b'); - const res: any = await p.deleteManyData({ + await expect(p.deleteManyData({ object: 'invoice', ids: ['a', 'b', 'c'], options: { atomic: true, continueOnError: true }, - } as any); + } as any)).rejects.toMatchObject({ status: 501, code: 'NOT_IMPLEMENTED' }); - expect(del).toHaveBeenCalledTimes(2); - expect(res.succeeded).toBe(1); + expect(del).not.toHaveBeenCalled(); }); }); diff --git a/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts b/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts new file mode 100644 index 0000000000..2aba39edd1 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts @@ -0,0 +1,353 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #4620 — `deleteManyData` and `updateManyData` get the same `atomic` contract +// ADR-0119 D4 gave `batchData`: REAL or REFUSED, never silent best-effort. +// +// Before this, the two siblings of the fixed method — in the same file, sharing +// its response type — were: +// +// * `deleteManyData`: fake-atomic. `if (options?.atomic) break;` opened no +// transaction; every row deleted before the failure stayed deleted while the +// response called itself atomic. Worse than the `batchData` case it was +// copied from, because a partial delete has no natural undo — the caller +// cannot reconstruct the rows from the request. +// * `updateManyData`: `atomic` never appeared in the method at all. Accepted, +// never read, no signal — declared ≠ enforced on a write-path guarantee. +// +// The pins that matter most here are the STATE ones: `succeeded: 0` in a +// response is cheap to produce; rows that are still readable after a failed +// atomic delete are the actual guarantee. So the fake engine below is a real +// little store with snapshot/restore transaction semantics, and each rollback +// test reads the store back afterwards. + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { name: 'invoice', fields: { title: { name: 'title', type: 'text' } } }; + +/** Marks an update the fake engine should reject, so a failure lands at a chosen index. */ +const POISON = '__explode__'; + +/** + * A fake engine backed by an in-memory store whose `transaction()` mirrors the + * real `ObjectQL.transaction` contract: run the callback on a handle-carrying + * context, COMMIT on resolve, ROLL BACK (restore the pre-call snapshot) and + * re-throw on reject. Anything that survives a rollback here would survive it + * in a database too. + */ +function makeStoreEngine(opts: { driverCanTransact?: boolean; hasTransaction?: boolean } = {}) { + const { driverCanTransact = true, hasTransaction = true } = opts; + const rows = new Map([ + ['a', { id: 'a', title: 'a-old' }], + ['b', { id: 'b', title: 'b-old' }], + ['c', { id: 'c', title: 'c-old' }], + ]); + const commits: unknown[] = []; + const rollbacks: unknown[] = []; + const handle = { id: 'trx-1' }; + + const update = vi.fn(async (_object: string, data: any, options?: any) => { + const id = options?.where?.id; + const current = rows.get(id); + if (!current) throw new Error(`no such record: ${id}`); + if (data?.title === POISON) throw new Error('update exploded'); + const next = { ...current, ...data }; + rows.set(id, next); + return next; + }); + // Contract per #4435: `false` is the positive not-found value. + const del = vi.fn(async (_object: string, options?: any) => { + const id = options?.where?.id; + if (!rows.has(id)) return false; + rows.delete(id); + return { deleted: 1 }; + }); + + const engine: any = { + registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) }, + update, + delete: del, + findOne: vi.fn(async (_object: string, options?: any) => rows.get(options?.where?.id)), + getDefaultDriverName: () => 'default', + getDriverByName: () => (driverCanTransact ? { beginTransaction: async () => handle } : {}), + }; + if (hasTransaction) { + engine.transaction = vi.fn(async (callback: (ctx: any) => Promise, baseContext?: any) => { + const snapshot = new Map(rows); + const trxCtx = { ...(baseContext ?? {}), transaction: handle }; + try { + const result = await callback(trxCtx); + commits.push(handle); + return result; + } catch (err) { + rows.clear(); + for (const [k, v] of snapshot) rows.set(k, v); + rollbacks.push(handle); + throw err; + } + }); + } + return { engine, update, del, rows, commits, rollbacks, handle }; +} + +describe('deleteManyData atomic — the deletes are actually undone (#4620)', () => { + it('rolls the whole batch back on the first failure: the earlier rows are STILL THERE', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + // 'missing' does not exist, so `engine.delete` answers `false` and the + // row fails — after 'a' has already been deleted inside the transaction. + const res: any = await p.deleteManyData({ + object: 'invoice', + ids: ['a', 'missing', 'c'], + options: { atomic: true }, + } as any); + + // THE CRUX. A partial delete has no natural undo, so this — not the + // response body — is what `atomic` was promising all along. + expect(t.rows.has('a')).toBe(true); + expect(t.rows.get('a')).toEqual({ id: 'a', title: 'a-old' }); + expect(t.rows.has('c')).toBe(true); + + expect(t.engine.transaction).toHaveBeenCalledTimes(1); + expect(t.rollbacks).toHaveLength(1); + expect(t.commits).toHaveLength(0); + expect(t.del).toHaveBeenCalledTimes(2); // 'c' never attempted + + // Nothing persisted, so nothing may report 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); + + // A client must be able to tell "attempted, undone" from "never ran". + expect(res.results[0].id).toBe('a'); + expect(res.results[0].error).toMatch(/^ROLLED_BACK:/); + expect(res.results[1].error).toMatch(/not found/i); // the causal row, verbatim + expect(res.results[2].error).toMatch(/^NOT_ATTEMPTED:/); + }); + + it('commits when every id deletes, and every delete runs on the transaction handle', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.deleteManyData({ + object: 'invoice', + ids: ['a', 'b'], + options: { atomic: true }, + context: { userId: 'u1' }, + } as any); + + expect(t.commits).toHaveLength(1); + expect(t.rollbacks).toHaveLength(0); + expect(t.rows.has('a')).toBe(false); + expect(t.rows.has('b')).toBe(false); + expect(res).toMatchObject({ success: true, operation: 'delete', total: 2, succeeded: 2, failed: 0 }); + + // Writes must carry the OPEN transaction, not the caller's bare context — + // otherwise they commit outside the batch and a rollback would spare them. + for (const call of t.del.mock.calls) { + expect(call[1].context.transaction).toBe(t.handle); + expect(call[1].context.userId).toBe('u1'); + } + }); + + it('atomic outranks continueOnError: the rest is never attempted and nothing lands', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.deleteManyData({ + object: 'invoice', + ids: ['missing', 'b', 'c'], + options: { atomic: true, continueOnError: true }, + } as any); + + expect(t.del).toHaveBeenCalledTimes(1); + expect(t.rows.size).toBe(3); + expect(res.succeeded).toBe(0); + expect(res.results[1].error).toMatch(/^NOT_ATTEMPTED:/); + }); +}); + +describe('updateManyData atomic — the option is finally read (#4620)', () => { + it('rolls back prior updates to their previous values', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.updateManyData({ + object: 'invoice', + records: [ + { id: 'a', data: { title: 'a-new' } }, + { id: 'b', data: { title: POISON } }, + { id: 'c', data: { title: 'c-new' } }, + ], + options: { atomic: true }, + } as any); + + // Row 'a' DID update against the engine; the rollback restored it. + expect(t.rows.get('a')).toEqual({ id: 'a', title: 'a-old' }); + expect(t.rows.get('c')).toEqual({ id: 'c', title: 'c-old' }); + + expect(t.engine.transaction).toHaveBeenCalledTimes(1); + expect(t.rollbacks).toHaveLength(1); + expect(t.update).toHaveBeenCalledTimes(2); // 'c' never attempted + + expect(res.success).toBe(false); + expect(res.operation).toBe('update'); + expect(res.succeeded).toBe(0); + expect(res.failed).toBe(3); + expect(res.results[0].id).toBe('a'); + expect(res.results[0].error).toMatch(/^ROLLED_BACK:/); + expect(res.results[0].error).toContain('update exploded'); // carries the cause + expect(res.results[1].error).toBe('update exploded'); // the causal row, verbatim + expect(res.results[2].error).toMatch(/^NOT_ATTEMPTED:/); + // Nothing persisted, so no reverted write may be reported as a success + // or carry a record payload. + expect(res.results.every((r: any) => r.success === false)).toBe(true); + }); + + it('commits when every row updates, on the transaction handle', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.updateManyData({ + object: 'invoice', + records: [{ id: 'a', data: { title: 'a-new' } }, { id: 'b', data: { title: 'b-new' } }], + options: { atomic: true }, + context: { userId: 'u1' }, + } as any); + + expect(t.commits).toHaveLength(1); + expect(t.rows.get('a')).toEqual({ id: 'a', title: 'a-new' }); + expect(res).toMatchObject({ success: true, total: 2, succeeded: 2, failed: 0 }); + for (const call of t.update.mock.calls) { + expect(call[2].context.transaction).toBe(t.handle); + expect(call[2].context.userId).toBe('u1'); + } + }); + + it('atomic outranks continueOnError here too', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.updateManyData({ + object: 'invoice', + records: [ + { id: 'a', data: { title: POISON } }, + { id: 'b', data: { title: 'b-new' } }, + ], + options: { atomic: true, continueOnError: true }, + } as any); + + expect(t.update).toHaveBeenCalledTimes(1); + expect(t.rows.get('b')).toEqual({ id: 'b', title: 'b-old' }); + expect(res.succeeded).toBe(0); + expect(res.results[1].error).toMatch(/^NOT_ATTEMPTED:/); + }); +}); + +describe('many-data atomic — refuses rather than degrading (#4620)', () => { + // The silent downgrade is the exact defect class this issue is about: a + // caller who asked for atomicity is precisely the caller who must not + // receive best-effort without being told. + for (const [label, opts] of [ + ['the engine has no transaction()', { hasTransaction: false }], + ['the default driver cannot beginTransaction()', { driverCanTransact: false }], + ] as const) { + it(`deleteManyData refuses with 501 when ${label}, deleting nothing`, async () => { + const t = makeStoreEngine(opts); + const p = new ObjectStackProtocolImplementation(t.engine); + + await expect(p.deleteManyData({ + object: 'invoice', ids: ['a', 'b'], options: { atomic: true }, + } as any)).rejects.toMatchObject({ status: 501, code: 'NOT_IMPLEMENTED' }); + + expect(t.del).not.toHaveBeenCalled(); + expect(t.rows.size).toBe(3); + }); + + it(`updateManyData refuses with 501 when ${label}, updating nothing`, async () => { + const t = makeStoreEngine(opts); + const p = new ObjectStackProtocolImplementation(t.engine); + + await expect(p.updateManyData({ + object: 'invoice', + records: [{ id: 'a', data: { title: 'a-new' } }], + options: { atomic: true }, + } as any)).rejects.toMatchObject({ status: 501, code: 'NOT_IMPLEMENTED' }); + + expect(t.update).not.toHaveBeenCalled(); + expect(t.rows.get('a')).toEqual({ id: 'a', title: 'a-old' }); + }); + } +}); + +describe('many-data non-atomic — unchanged (#4620 regression net)', () => { + it('updateManyData opens no transaction and keeps prior successes when atomic is absent', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.updateManyData({ + object: 'invoice', + records: [ + { id: 'a', data: { title: 'a-new' } }, + { id: 'b', data: { title: POISON } }, + { id: 'c', data: { title: 'c-new' } }, + ], + } as any); + + expect(t.engine.transaction).not.toHaveBeenCalled(); + expect(t.rows.get('a')).toEqual({ id: 'a', title: 'a-new' }); // committed, and kept + expect(res).toMatchObject({ success: false, operation: 'update', total: 3, succeeded: 1, failed: 1 }); + expect(res.results).toHaveLength(2); // stops without continueOnError + expect(res.results[0]).toMatchObject({ id: 'a', success: true }); + expect(res.results[1]).toMatchObject({ id: 'b', success: false, error: 'update exploded' }); + }); + + it('updateManyData continueOnError still processes every row', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.updateManyData({ + object: 'invoice', + records: [ + { id: 'a', data: { title: POISON } }, + { id: 'b', data: { title: 'b-new' } }, + { id: 'c', data: { title: 'c-new' } }, + ], + options: { continueOnError: true }, + } as any); + + expect(t.engine.transaction).not.toHaveBeenCalled(); + expect(t.update).toHaveBeenCalledTimes(3); + expect(res).toMatchObject({ succeeded: 2, failed: 1 }); + }); + + it('deleteManyData atomic:false is best-effort, not a refusal, even with no transaction()', async () => { + const t = makeStoreEngine({ hasTransaction: false }); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.deleteManyData({ + object: 'invoice', ids: ['a', 'missing', 'c'], options: { atomic: false, continueOnError: true }, + } as any); + + expect(t.del).toHaveBeenCalledTimes(3); + expect(t.rows.has('a')).toBe(false); // stays deleted — that is the contract when not atomic + expect(res).toMatchObject({ success: false, total: 3, succeeded: 2, failed: 1 }); + }); + + it('updateManyData atomic:false is best-effort on a non-transactional engine too', async () => { + const t = makeStoreEngine({ hasTransaction: false }); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.updateManyData({ + object: 'invoice', + records: [{ id: 'a', data: { title: 'a-new' } }], + options: { atomic: false }, + } as any); + + expect(res.succeeded).toBe(1); + expect(t.rows.get('a')).toEqual({ id: 'a', title: 'a-new' }); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index f3c29cc456..57f7e68361 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -5349,6 +5349,41 @@ export class ObjectStackProtocolImplementation implements context: any; }): Promise { const { object, operation, records, options, batchSchema, context } = args; + return await this.runAtomicBatch({ + object, + context, + runLoop: (trxCtx) => this.runBatchDataLoop({ object, operation, records, options, batchSchema, context: trxCtx, atomic: true }), + onCommit: (outcome) => this.buildBatchDataResponse(operation, records, options, outcome), + onRollback: (outcome) => this.buildRolledBackBatchResponse(operation, records, outcome), + }); + } + + /** + * The atomic arm, shared by every bulk-write surface on this protocol + * (#4620): `batchData` (ADR-0119 D4), `updateManyData` and + * `deleteManyData`. + * + * D4 fixed `batchData` alone, and its two siblings in this file carried the + * same class of defect — `deleteManyData` `break`-ed the loop and left every + * prior delete committed under a response that called itself atomic (worse + * than the `batchData` case: a partial delete has no natural undo), while + * `updateManyData` never read `atomic` at all. Fixed by giving all three ONE + * runner rather than a third and fourth copy of transaction handling — + * copies are exactly how the next sibling drifts back into a lie. + * + * Each caller supplies only what differs: the per-record loop, and the two + * response builders. Everything that makes `atomic` a guarantee — the + * fail-closed capability gate, the single `engine.transaction()`, the abort + * sentinel, the zero-successes rollback response — lives here, once. + */ + private async runAtomicBatch(args: { + object: string; + context: any; + runLoop: (trxCtx: any) => Promise; + onCommit: (outcome: BatchDataLoopOutcome) => BatchUpdateResponse; + onRollback: (outcome: BatchDataLoopOutcome) => BatchUpdateResponse; + }): Promise { + const { object, context, runLoop, onCommit, onRollback } = args; // Two-level probe, shared with the ADR-0119 D2 migration-journal runner // as `engineCanRollBack` (#4617). `engine.transaction()` runs the @@ -5389,16 +5424,16 @@ export class ObjectStackProtocolImplementation implements 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 }); + const outcome = await runLoop(trxCtx); if (outcome.failed > 0) { aborted = outcome; throw ABORT; } - return this.buildBatchDataResponse(operation, records, options, outcome); + return onCommit(outcome); }, context); } catch (err) { if (err === ABORT && aborted) { - return this.buildRolledBackBatchResponse(operation, records, aborted); + return onRollback(aborted); } throw err; } @@ -5550,7 +5585,11 @@ export class ObjectStackProtocolImplementation implements */ private buildRolledBackBatchResponse( operation: BatchUpdateRequest['operation'], - records: BatchUpdateRequest['records'], + // Only `length` and `id` are read, so `updateManyData`'s rows and the + // id list `deleteManyData` rolls back reuse this verbatim (#4620) — + // the marking a client reconciles against must be one implementation, + // not three that agree today. + records: ReadonlyArray<{ id?: string }>, outcome: BatchDataLoopOutcome, ): BatchUpdateResponse { const attempted = outcome.results; @@ -5661,7 +5700,46 @@ export class ObjectStackProtocolImplementation implements async updateManyData(request: UpdateManyDataRequest & { context?: any }): Promise { const { object, records, options, context } = request; this.assertObjectRegistered(object); // [#3770] - const results: Array<{ id?: string; success: boolean; error?: string; record?: any; droppedFields?: DroppedFieldsEvent[] }> = []; + + // [#4620] `atomic` used not to appear ANYWHERE in this method: the + // option was accepted, declared in `BatchOptionsSchema` with a contract + // that promises all-or-nothing, and never read — a caller asking for + // atomicity silently got best-effort with no signal at all. Same + // enforcement shape as `batchData` (ADR-0119 D4), same runner, so the + // three bulk-write surfaces cannot drift apart again. `=== true` is the + // deliberate opt-in from D4: absent/false keeps today's semantics + // exactly. + if (options?.atomic === true) { + return await this.runAtomicBatch({ + object, + context, + runLoop: (trxCtx) => this.runUpdateManyLoop({ object, records, options, context: trxCtx, atomic: true }), + onCommit: (outcome) => this.buildUpdateManyResponse(records, outcome), + onRollback: (outcome) => this.buildRolledBackBatchResponse('update', records, outcome), + }); + } + + const outcome = await this.runUpdateManyLoop({ object, records, options, context, atomic: false }); + return this.buildUpdateManyResponse(records, outcome); + } + + /** + * The per-record loop of {@link updateManyData}, shared by both arms + * (#4620) so atomic and non-atomic cannot drift apart. `atomic` changes + * exactly one thing: it aborts on the first failure regardless of + * `continueOnError` — whose own contract text already scopes it to + * `atomic=false`, and which has nothing to continue toward when every write + * so far is about to be undone. + */ + private async runUpdateManyLoop(args: { + object: string; + records: UpdateManyDataRequest['records']; + options: UpdateManyDataRequest['options']; + context: any; + atomic: boolean; + }): Promise { + const { object, records, options, context, atomic } = args; + const results: BatchDataRowResult[] = []; let succeeded = 0; let failed = 0; @@ -5683,12 +5761,30 @@ export class ObjectStackProtocolImplementation implements } catch (err: any) { results.push({ id: record.id, success: false, error: err.message }); failed++; + if (atomic) { + // Abort on the first failure; the caller rolls back. + break; + } if (!options?.continueOnError) { break; } } } + return { results, succeeded, failed }; + } + + /** + * The ordinary (committed) `updateMany` response. Deliberately NOT + * {@link buildBatchDataResponse}: this surface has never honoured + * `returnRecords`, and quietly starting to would change the default + * (non-atomic) path's payload while fixing an unrelated bug (#4620). + */ + private buildUpdateManyResponse( + records: UpdateManyDataRequest['records'], + outcome: BatchDataLoopOutcome, + ): BatchUpdateResponse { + const { results, succeeded, failed } = outcome; return { success: failed === 0, operation: 'update', @@ -5750,6 +5846,12 @@ export class ObjectStackProtocolImplementation implements * - the declared {@link BatchUpdateResponse} contract (per-record results, * `atomic` / `continueOnError`) was unimplementable from a bulk row * count. It is now actually delivered. + * + * [#4620] That last bullet over-claimed for one member: `atomic` was + * per-record, but it only stopped the loop — no transaction, no rollback, + * every earlier delete left committed under a response titled atomic. It is + * delivered for real now, through the shared {@link runAtomicBatch}, and + * refused (501 `NOT_IMPLEMENTED`) on a runtime that cannot roll back. */ async deleteManyData(request: DeleteManyDataRequest & { context?: any }): Promise { const { object, options, context } = request; @@ -5771,7 +5873,40 @@ export class ObjectStackProtocolImplementation implements throw err; } - const results: Array<{ id?: string; success: boolean; error?: string }> = []; + // [#4620] `atomic` here was the same fake-atomic `batchData` carried + // before ADR-0119 D4 — it only `break`-ed the loop, so every row deleted + // before the failure stayed DELETED while the response called itself + // atomic. Worse than the `batchData` case, because a partial delete has + // no natural undo: the caller cannot reconstruct the rows from the + // request. Same runner, same fail-closed gate, same row marking. + if (options?.atomic === true) { + const rows = ids.map((id) => ({ id: String(id) })); + return await this.runAtomicBatch({ + object, + context, + runLoop: (trxCtx) => this.runDeleteManyLoop({ object, ids, options, context: trxCtx, atomic: true }), + onCommit: (outcome) => this.buildDeleteManyResponse(ids, outcome), + onRollback: (outcome) => this.buildRolledBackBatchResponse('delete', rows, outcome), + }); + } + + const outcome = await this.runDeleteManyLoop({ object, ids, options, context, atomic: false }); + return this.buildDeleteManyResponse(ids, outcome); + } + + /** + * The per-id loop of {@link deleteManyData}, shared by both arms (#4620) — + * see {@link runUpdateManyLoop} for why `atomic` outranks `continueOnError`. + */ + private async runDeleteManyLoop(args: { + object: string; + ids: unknown[]; + options: DeleteManyDataRequest['options']; + context: any; + atomic: boolean; + }): Promise { + const { object, ids, options, context, atomic } = args; + const results: BatchDataRowResult[] = []; let succeeded = 0; let failed = 0; const ctxOpt = context !== undefined ? { context } : {}; @@ -5787,20 +5922,29 @@ export class ObjectStackProtocolImplementation implements // single-record path: the contract's positive not-found value, // never an inference from a falsy return. const deleted = await this.engine.delete(object, { where: { id }, ...ctxOpt } as any); - if (deleted === false) throw recordNotFoundError(object, id); + // `id` is `unknown` to this helper only because the caller's + // fail-closed `isScalarId` guard is what proves it scalar. + if (deleted === false) throw recordNotFoundError(object, id as string | number); results.push({ id: String(id), success: true }); succeeded++; } catch (err: any) { results.push({ id: String(id), success: false, error: err?.message }); failed++; // Same stop semantics as `batchData`: `atomic` aborts the rest on - // the first failure, and without `continueOnError` a failure ends - // the run rather than silently ploughing on. - if (options?.atomic) break; + // the first failure (the caller rolls back), and without + // `continueOnError` a failure ends the run rather than silently + // ploughing on. + if (atomic) break; if (!options?.continueOnError) break; } } + return { results, succeeded, failed }; + } + + /** The ordinary (committed) `deleteMany` response — every id reports what it did. */ + private buildDeleteManyResponse(ids: unknown[], outcome: BatchDataLoopOutcome): BatchUpdateResponse { + const { results, succeeded, failed } = outcome; return { success: failed === 0, operation: 'delete',