From 9df37438b475ea63c09aa1098f641bec393a8127 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 05:51:30 +0000 Subject: [PATCH 1/2] fix(metadata-protocol,spec): report NOT_ATTEMPTED for a stopped bulk write's tail, and make the counters reconcile (#7539) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A non-atomic `/batch` that stopped at the first failure answered with a truncated `results` array and counters that did not add up: two results for three records, no entry for the un-attempted record, and `succeeded + failed != total`. The skipped record was invisible twice over — no `results[]` entry and counted in neither bucket — so the arithmetic mismatch was its only trace. `buildBatchDataResponse` read `total` from the request while `results`, `succeeded` and `failed` came from a loop that had stopped early. `buildUpdateManyResponse` and `buildDeleteManyResponse` under-reported the same way. All three now share one reconciler that pads the outcome out to the request length with `NOT_ATTEMPTED` rows — the registered ADR-0112 code the atomic arm has emitted since #4793 — and returns `failed` as the count of every non-success row, so `succeeded + failed === total === results.length` on both arms. The stop itself is unchanged, per `BatchOptionsSchema.continueOnError` ("If true (and atomic=false), continue processing remaining records after errors") and ADR-0119 D4, whose test plan holds non-atomic batches to "behave exactly as before". This is a reporting fix; `continueOnError` remains the flag that buys continuation. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016gd2bypaK4KYs78q8RP38G --- .../batch-not-attempted-tail-reporting.md | 58 ++++ .../src/protocol.batch-atomic.test.ts | 13 +- .../src/protocol.batch-not-attempted.test.ts | 328 ++++++++++++++++++ .../src/protocol.delete-many.test.ts | 10 +- .../src/protocol.many-data-atomic.test.ts | 12 +- .../src/protocol.record-not-found.test.ts | 7 +- packages/metadata-protocol/src/protocol.ts | 81 ++++- packages/spec/src/api/batch.zod.ts | 11 +- .../spec/src/api/error-code-ledger.zod.ts | 2 +- 9 files changed, 510 insertions(+), 12 deletions(-) create mode 100644 .changeset/batch-not-attempted-tail-reporting.md create mode 100644 packages/metadata-protocol/src/protocol.batch-not-attempted.test.ts diff --git a/.changeset/batch-not-attempted-tail-reporting.md b/.changeset/batch-not-attempted-tail-reporting.md new file mode 100644 index 0000000000..fb04bf7aee --- /dev/null +++ b/.changeset/batch-not-attempted-tail-reporting.md @@ -0,0 +1,58 @@ +--- +"@objectstack/spec": patch +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol,spec): a bulk write that STOPS now reports every record — `NOT_ATTEMPTED` rows instead of a truncated `results` array, and counters that reconcile (#7539) + +`POST /data/:object/batch` with no `options` (so `atomic` defaults `false`, +ADR-0119 D4) and three records — valid, failing, valid — answered: + +``` +200 { "total": 3, "succeeded": 1, "failed": 1, + "results": [ { idx 0: ok }, { idx 1: VALIDATION_FAILED } ] } +``` + +Two results for three records, no entry for idx 2, and `succeeded + failed` (2) +`!= total` (3). The un-attempted record was invisible **twice over**: it +produced no `results[]` entry and was counted in neither bucket, so the only +trace of it was an arithmetic mismatch a client had to notice and interpret. + +`buildBatchDataResponse` read `total` from the REQUEST (`records.length`) while +`results` / `succeeded` / `failed` came from a loop that had stopped early. Its +two siblings under-reported identically — the same defect on `updateManyData` +and `deleteManyData`, whose per-object bulk counters lost the tail whenever a +row failed without `continueOnError`. All three now go through one shared +reconciler rather than a fourth copy of the same arithmetic. + +**What changed is the REPORT, not the semantics.** Every record now gets a row +saying what happened to it: records after the failure carry +`errors[0].code === 'NOT_ATTEMPTED'` — the same registered ADR-0112 code the +atomic arm has emitted since #4793, because "never ran" means the same thing to +a client whether the batch stopped to roll back or stopped because it was told +to. The message names the causal row index and `continueOnError`, since on this +arm the caller's next action is a flag rather than a fixed row. `results` now +always covers all `total` records, and `succeeded` / `failed` partition it, so +`succeeded + failed === total === results.length` on both arms. + +**The stop itself is unchanged, deliberately.** Without `continueOnError` the +first failure still ends the run, records written before it stay written +(nothing is rolled back on this arm), and the tail is still not attempted. +That is the declared contract, not an accident: +`BatchOptionsSchema.continueOnError` reads *"If true (and atomic=false), +continue processing remaining records after errors"*, ADR-0119 D4 scopes the +flag to exactly `atomic=false`, and D4's test plan holds non-atomic batches to +"behave exactly as before". If `atomic: false` alone continued past a failure, +`continueOnError` would be inert. Callers who want every valid row to land +should send `continueOnError: true` — unchanged, and now the only difference +between the two is whether the tail is attempted, not whether it is reported. + +**Upgrade note.** A non-atomic batch that stops now returns more `results` rows +and a larger `failed` count than before, for the same request and the same +writes. `failed` counts every row that is not a success — matching the atomic +rollback response, which has always counted never-reached rows this way. A +client that summed `succeeded + failed` and compared it to `total` to detect +truncation no longer needs to; one that treated `failed` as "rows the server +tried and could not write" should branch on `errors[0].code` instead, where +`NOT_ATTEMPTED` distinguishes "skipped" from "attempted and failed". No schema +field was added or removed. diff --git a/packages/metadata-protocol/src/protocol.batch-atomic.test.ts b/packages/metadata-protocol/src/protocol.batch-atomic.test.ts index 969e1044a2..8a2b381d10 100644 --- a/packages/metadata-protocol/src/protocol.batch-atomic.test.ts +++ b/packages/metadata-protocol/src/protocol.batch-atomic.test.ts @@ -255,10 +255,19 @@ describe('batchData non-atomic — unchanged (ADR-0119 D4 regression net)', () = 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 + // Still stops without `continueOnError` — two inserts, never three. + expect(t.insert).toHaveBeenCalledTimes(2); + // [#7539] But the STOP is now reported rather than inferred from a + // counter mismatch. This block used to assert `failed: 1` and + // `results.length === 2` against `total: 3` — the truncated `results` + // array and the `succeeded + failed != total` arithmetic that were the + // card's entire symptom. + expect(res.failed).toBe(2); + expect(res.results).toHaveLength(3); + expect(res.succeeded + res.failed).toBe(res.total); + expect(res.results[2].errors[0].code).toBe('NOT_ATTEMPTED'); }); it('atomic: false is best-effort, not a refusal, even on a non-transactional engine', async () => { diff --git a/packages/metadata-protocol/src/protocol.batch-not-attempted.test.ts b/packages/metadata-protocol/src/protocol.batch-not-attempted.test.ts new file mode 100644 index 0000000000..f4b44c3393 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.batch-not-attempted.test.ts @@ -0,0 +1,328 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7539] A bulk write that STOPS must still report on every record it was + * given. + * + * `POST /data/:object/batch` with no `options` (so `atomic` defaults false, + * ADR-0119 D4) and three records — valid, failing, valid — answered: + * + * 200 { total: 3, succeeded: 1, failed: 1, + * results: [ {idx 0 ok}, {idx 1 VALIDATION_FAILED} ] } + * + * Four defects in one body, all of them REPORTING defects: + * * 2 `results` entries for 3 records; + * * no row for idx 2, so the caller cannot tell it was skipped; + * * `succeeded + failed` (2) != `total` (3) — the counters do not reconcile; + * * the only trace of the skipped record is that arithmetic mismatch. + * + * The loop's `break` is NOT the defect. `BatchOptionsSchema.continueOnError` + * declares itself as "If true (and atomic=false), continue processing remaining + * records after errors", and ADR-0119 D4 scopes it to exactly `atomic=false` + * while its test plan item 5 holds non-atomic batches to "behave exactly as + * before". So stopping is the DECLARED default and `continueOnError` is the + * knob that buys continuation; if `atomic:false` alone continued, the flag + * would be inert. What was never declared anywhere is a truncated `results` + * array and counters that do not add up. + * + * `buildBatchDataResponse` reported `total = records.length` while `results`, + * `succeeded` and `failed` came from a loop that had stopped early — so the + * un-attempted tail was invisible twice over. Its two siblings + * (`updateManyData`, `deleteManyData`) under-reported identically: the card's + * "per-object bulk counters under-report whenever a row fails without + * `continueOnError`". + * + * The shape being restored is the one the ATOMIC arm has delivered since #4793: + * `results.length === total`, every row saying what happened to it, rows never + * reached carrying `errors[0].code === 'NOT_ATTEMPTED'`, and + * `succeeded + failed === total` because `failed` counts every row that is not + * a success. The two same-run controls from the issue are kept below as GUARDs. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { + name: 'showcase_private_note', + fields: { + title: { name: 'title', type: 'text' }, + }, +}; + +/** The row the fake engine rejects, standing in for the issue's failing record. */ +const POISON = '__invalid__'; + +/** + * A validation failure shaped like the real one: a classified `code` + + * `status`, so `toRowApiError` renders `VALIDATION_FAILED`/400 rather than + * falling back to `INTERNAL_ERROR`. + */ +function validationFailure(): Error { + const err: any = new Error('title is invalid'); + err.code = 'VALIDATION_FAILED'; + err.status = 400; + return err; +} + +/** + * In-memory store with real snapshot/rollback transaction semantics — the + * harness shape the #4620 / #4793 / #5088 suites use, so every row asserted + * below is produced by the actual loops, builders and rollback classifier, and + * "did the record LAND?" is answerable by reading the store. + */ +function makeStoreEngine() { + const rows = new Map([ + ['t1', { id: 't1', title: 'stored one' }], + ['t2', { id: 't2', title: 'stored two' }], + ['t3', { id: 't3', title: 'stored three' }], + ]); + const handle = { id: 'trx-1' }; + + const insert = vi.fn(async (_object: string, data: any) => { + if (data?.title === POISON) throw validationFailure(); + const rec = { id: data.id ?? `new-${rows.size + 1}`, ...data }; + rows.set(rec.id, rec); + return rec; + }); + const update = vi.fn(async (_object: string, data: any, options?: any) => { + const id = options?.where?.id; + if (data?.title === POISON) throw validationFailure(); + const next = { ...rows.get(id), ...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 findOne = vi.fn(async (_object: string, options?: any) => rows.get(options?.where?.id) ?? null); + + const engine: any = { + registry: { getObject: (n: string) => (n === 'showcase_private_note' ? SCHEMA : undefined) }, + insert, + update, + delete: del, + findOne, + getDefaultDriverName: () => 'default', + getDriverByName: () => ({ beginTransaction: async () => handle }), + transaction: vi.fn(async (callback: (ctx: any) => Promise, baseContext?: any) => { + const snapshot = new Map(rows); + try { + return await callback({ ...(baseContext ?? {}), transaction: handle }); + } catch (err) { + rows.clear(); + for (const [k, v] of snapshot) rows.set(k, v); + throw err; + } + }), + }; + return { engine, rows, insert, update, del, findOne }; +} + +/** `errors[0].code` per row — the machine-readable outcome (#4793). */ +const codesOf = (res: any): Array => + res.results.map((r: any) => (r.success ? undefined : r.errors?.[0]?.code)); + +/** + * The whole point of the card, asserted as an EQUATION over the response rather + * than as the presence of one new entry. + * + * `notAttempted` is DERIVED from the rows rather than read from a new envelope + * field: `BatchUpdateResponseSchema` declares `{ total, succeeded, failed, + * results }` and nothing else, and the atomic arm that already gets this right + * (`buildRolledBackBatchResponse`) counts a never-reached row in `failed` — so + * `failed` means "rows that are not successes", and the three-term identity is + * checked against the codes the rows actually carry. + */ +function expectCountersReconcile(res: any, total: number) { + const notAttempted = res.results.filter((r: any) => r.errors?.[0]?.code === 'NOT_ATTEMPTED').length; + const attemptedFailed = res.results.filter((r: any) => r.success === false).length - notAttempted; + + // 1. Every record the caller sent gets a row back. + expect(res.total).toBe(total); + expect(res.results).toHaveLength(total); + // 2. The envelope's own two buckets account for the whole batch. + expect(res.succeeded + res.failed).toBe(res.total); + // 3. Three-term identity, with every term derived from the rows: no row is + // double-counted and none is invisible. + expect(res.succeeded + attemptedFailed + notAttempted).toBe(res.total); + // 4. The buckets agree with the rows they claim to summarise. + expect(res.succeeded).toBe(res.results.filter((r: any) => r.success === true).length); + expect(res.failed).toBe(res.results.filter((r: any) => r.success === false).length); + // 5. `index` still correlates rows to the request array (#4793). + expect(res.results.map((r: any) => r.index)).toEqual([...Array(total).keys()]); +} + +describe('[#7539] batchData non-atomic — a stopped batch reports every record', () => { + /** The issue's exact repro: no `options` key at all. */ + const threeRecords = [ + { data: { title: 'first valid' } }, + { data: { title: POISON } }, + { data: { title: 'third valid' } }, + ]; + + it('answers 3 results for 3 records, with idx 2 marked NOT_ATTEMPTED', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { operation: 'create', records: threeRecords }, + } as any); + + expectCountersReconcile(res, 3); + // The per-index outcomes by identity, not by "something new is present". + expect(codesOf(res)).toEqual([undefined, 'VALIDATION_FAILED', 'NOT_ATTEMPTED']); + expect(res.results.map((r: any) => r.success)).toEqual([true, false, false]); + expect(res.succeeded).toBe(1); + expect(res.failed).toBe(2); + expect(res.success).toBe(false); + + // The causal row keeps its own error verbatim — NOT_ATTEMPTED must not + // overwrite the diagnosis the caller needs. + expect(res.results[1].errors[0].message).toBe('title is invalid'); + expect(res.results[1].errors[0].httpStatus).toBe(400); + // The skipped row says WHICH record stopped the batch, and how to make + // the run continue — the caller's next action is in the message. + expect(res.results[2].errors[0].message).toContain('1'); + expect(res.results[2].errors[0].message).toContain('continueOnError'); + // A skipped row is not a success and carries no record payload. + expect(res.results[2].success).toBe(false); + expect(res.results[2].data).toBeUndefined(); + }); + + it('GUARD — the stop itself is unchanged: idx 2 is never attempted and never lands', async () => { + // ADR-0119 D4 test plan item 5 ("non-atomic batches behave exactly as + // before") plus `continueOnError`'s own contract text. The fix is a + // REPORTING fix; if this flips, the semantics moved and the flag went + // inert. + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { operation: 'create', records: threeRecords }, + } as any); + + expect(t.engine.transaction).not.toHaveBeenCalled(); // no transaction on this arm + expect(t.insert).toHaveBeenCalledTimes(2); // idx 2 never reached the engine + expect(t.insert.mock.calls.map((c: any[]) => c[1].title)).toEqual(['first valid', POISON]); + // Prior successes are RETAINED (nothing rolled back), the tail is absent. + expect([...t.rows.values()].map((r: any) => r.title)).toContain('first valid'); + expect([...t.rows.values()].map((r: any) => r.title)).not.toContain('third valid'); + // Deliberately asserts NOTHING about `results[2]`: this control must be + // green in BOTH directions, and the skipped row's very existence is the + // thing under repair. Its shape is pinned by the reporting test above. + expect(res).toBeDefined(); + }); + + it('GUARD (control 1) — continueOnError:true still yields 3 results and lands BOTH valid rows', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { operation: 'create', records: threeRecords, options: { continueOnError: true } }, + } as any); + + expectCountersReconcile(res, 3); + expect(codesOf(res)).toEqual([undefined, 'VALIDATION_FAILED', undefined]); + expect(res.succeeded).toBe(2); + expect(res.failed).toBe(1); + expect(t.insert).toHaveBeenCalledTimes(3); + const titles = [...t.rows.values()].map((r: any) => r.title); + expect(titles).toContain('first valid'); + expect(titles).toContain('third valid'); + }); + + it('GUARD (control 2) — atomic:true still yields ROLLED_BACK / causal / NOT_ATTEMPTED', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { operation: 'create', records: threeRecords, options: { atomic: true } }, + } as any); + + expectCountersReconcile(res, 3); + expect(codesOf(res)).toEqual(['ROLLED_BACK', 'VALIDATION_FAILED', 'NOT_ATTEMPTED']); + expect(res.succeeded).toBe(0); + expect(res.failed).toBe(3); + // Nothing persisted — the rollback is real, and the first row is gone. + expect([...t.rows.values()].map((r: any) => r.title)).not.toContain('first valid'); + }); + + it('a batch that never fails is untouched by the reconciliation', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { operation: 'create', records: [{ data: { title: 'a' } }, { data: { title: 'b' } }] }, + } as any); + + expectCountersReconcile(res, 2); + expect(res.success).toBe(true); + expect(res.succeeded).toBe(2); + expect(res.failed).toBe(0); + }); + + it('`returnRecords: false` still drops `data` and keeps the NOT_ATTEMPTED row', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { operation: 'create', records: threeRecords, options: { returnRecords: false } }, + } as any); + + expectCountersReconcile(res, 3); + expect(codesOf(res)).toEqual([undefined, 'VALIDATION_FAILED', 'NOT_ATTEMPTED']); + expect(res.results.every((r: any) => r.data === undefined)).toBe(true); + }); +}); + +describe('[#7539] the same under-report on the two sibling bulk faces', () => { + it('updateManyData stops without continueOnError — and now says so for every row', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.updateManyData({ + object: 'showcase_private_note', + records: [ + { id: 't1', data: { title: 'renamed one' } }, + { id: 't2', data: { title: POISON } }, + { id: 't3', data: { title: 'renamed three' } }, + ], + } as any); + + expectCountersReconcile(res, 3); + expect(codesOf(res)).toEqual([undefined, 'VALIDATION_FAILED', 'NOT_ATTEMPTED']); + expect(res.succeeded).toBe(1); + expect(res.failed).toBe(2); + // Semantics unchanged: the tail is still not written. + expect(t.rows.get('t3').title).toBe('stored three'); + }); + + it('deleteManyData stops without continueOnError — and now says so for every id', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.deleteManyData({ + object: 'showcase_private_note', + ids: ['t1', 'definitely_missing', 't3'], + } as any); + + expectCountersReconcile(res, 3); + expect(codesOf(res)).toEqual([undefined, 'RECORD_NOT_FOUND', 'NOT_ATTEMPTED']); + expect(res.succeeded).toBe(1); + expect(res.failed).toBe(2); + // The skipped id is echoed back, so the caller can retry exactly it. + expect(res.results[2].id).toBe('t3'); + // Semantics unchanged: t3 is still there. + expect(t.rows.has('t3')).toBe(true); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.delete-many.test.ts b/packages/metadata-protocol/src/protocol.delete-many.test.ts index 9cf9ccffba..01beeb6a98 100644 --- a/packages/metadata-protocol/src/protocol.delete-many.test.ts +++ b/packages/metadata-protocol/src/protocol.delete-many.test.ts @@ -129,13 +129,21 @@ describe('deleteManyData — partial-failure semantics (#3897)', () => { const res: any = await p.deleteManyData({ object: 'invoice', ids: ['a', 'b', 'c'] } as any); expect(del).toHaveBeenCalledTimes(2); - expect(res).toMatchObject({ success: false, total: 3, succeeded: 1, failed: 1 }); + // [#7539] The STOP is unchanged — two deletes attempted, `c` untouched. + // What changed is the REPORT: `c` is accounted for instead of dropped, and + // `failed` counts every non-success row, so the counters reconcile against + // `total`. This line used to read `failed: 1` against `total: 3`. + expect(res).toMatchObject({ success: false, total: 3, succeeded: 1, failed: 2 }); + expect(res.results).toHaveLength(3); + expect(res.succeeded + res.failed).toBe(res.total); expect(res.results[1]).toEqual({ id: 'b', success: false, index: 1, // A thrown error with no code of its own maps to the unclassified-500 // row form; the message survives verbatim (#4793). errors: [{ code: 'INTERNAL_ERROR', message: 'RLS: not visible' }], }); + expect(res.results[2]).toMatchObject({ id: 'c', success: false, index: 2 }); + expect(res.results[2].errors[0].code).toBe('NOT_ATTEMPTED'); }); it('continueOnError keeps going and still marks the batch unsuccessful', async () => { diff --git a/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts b/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts index fd94a32cd2..d68cd65645 100644 --- a/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts +++ b/packages/metadata-protocol/src/protocol.many-data-atomic.test.ts @@ -303,10 +303,18 @@ describe('many-data non-atomic — unchanged (#4620 regression net)', () => { 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 + // [#7539] The run still STOPS at `b` — `c` is never written (asserted + // below). What changed is that the response now says so: a row for `c` + // and counters that reconcile, where this used to report `failed: 1` + // against `total: 3` with `c` missing from `results` entirely. + expect(res).toMatchObject({ success: false, operation: 'update', total: 3, succeeded: 1, failed: 2 }); + expect(res.results).toHaveLength(3); + expect(res.succeeded + res.failed).toBe(res.total); expect(res.results[0]).toMatchObject({ id: 'a', success: true, index: 0 }); expect(res.results[1]).toMatchObject({ id: 'b', success: false, index: 1, errors: [{ message: 'update exploded' }] }); + expect(res.results[2]).toMatchObject({ id: 'c', success: false, index: 2 }); + expect(res.results[2].errors[0].code).toBe('NOT_ATTEMPTED'); + expect(t.rows.get('c')).toEqual({ id: 'c', title: 'c-old' }); // stops without continueOnError }); it('updateManyData continueOnError still processes every row', async () => { diff --git a/packages/metadata-protocol/src/protocol.record-not-found.test.ts b/packages/metadata-protocol/src/protocol.record-not-found.test.ts index a092f3ad5d..c4714d88b6 100644 --- a/packages/metadata-protocol/src/protocol.record-not-found.test.ts +++ b/packages/metadata-protocol/src/protocol.record-not-found.test.ts @@ -197,7 +197,12 @@ describe('[#4435] deleteManyData reports per id, not per request', () => { it('a missing id stops the run without continueOnError, as a failure always has', async () => { const { p, del } = makeProtocol({ b: { id: 'b' } }); const res: any = await p.deleteManyData({ object: 'task', ids: ['a', 'b'] } as any); - expect(res).toMatchObject({ success: false, succeeded: 0, failed: 1 }); + // [#7539] Still stops after `a` (one delete attempted) — but `b` is now + // reported NOT_ATTEMPTED rather than silently dropped, so `succeeded + + // failed === total` instead of the old `0 + 1 != 2`. + expect(res).toMatchObject({ success: false, succeeded: 0, failed: 2, total: 2 }); + expect(res.results).toHaveLength(2); + expect(res.results[1].errors[0].code).toBe('NOT_ATTEMPTED'); expect(del).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7ee497373a..0c81032f7a 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -7442,6 +7442,74 @@ export class ObjectStackProtocolImplementation implements return { results, succeeded, failed }; } + /** + * [#7539] Reconciles a STOPPED loop's outcome with the request it answers, + * shared by the ordinary (committed) response of all three bulk-write + * surfaces. + * + * Without `continueOnError` a failure ends the run — the declared default + * (`BatchOptionsSchema.continueOnError`: *"If true (and atomic=false), + * continue processing remaining records after errors"*), and ADR-0119 D4 + * left it deliberately untouched ("non-atomic batches behave exactly as + * before"). Stopping was never the bug. Reporting the stop was: the three + * builders read `total` from the REQUEST (`records.length`) while `results`, + * `succeeded` and `failed` came from a loop that had stopped early, so an + * un-attempted record was invisible **twice over** — it produced no + * `results[]` entry and was counted in neither bucket. The only trace of it + * was `succeeded + failed != total`: an arithmetic mismatch no client + * should have to notice, let alone interpret. + * + * So every record gets a row saying what happened to it, and the counters + * add up. The classification is the one the ATOMIC arm has emitted since + * #4793 — `NOT_ATTEMPTED` as `errors[0].code`, registered in the ADR-0112 + * ledger, the message carrying the human-readable cause and the causal row + * index — because "never ran" means the same thing to a client whether the + * batch stopped to roll back or stopped because it was told to. The message + * additionally names `continueOnError`, since on THIS arm the caller's next + * action is a flag rather than a fixed row. + * + * `failed` therefore counts every row that is not a success, exactly as + * {@link buildRolledBackBatchResponse} already does, keeping ONE reading of + * the envelope across both arms: `succeeded` and `failed` partition + * `results`, and `succeeded + failed === total === results.length`. A new + * `notAttempted` envelope field would have bought the same information at + * the price of two different meanings for `failed` on two arms of one + * endpoint — which is the kind of drift that separated the rows from the + * counters here in the first place. + * + * A no-op when the loop ran to completion: every all-success path, every + * `continueOnError` run, and the atomic arm's `onCommit`, which by + * construction only ever sees `failed === 0`. + */ + private reconcileStoppedBatch( + records: ReadonlyArray<{ id?: string }>, + outcome: BatchDataLoopOutcome, + ): BatchDataLoopOutcome { + if (outcome.results.length >= records.length) return outcome; + + const causeIndex = outcome.results.findIndex(r => !r.success); + const cause = causeIndex >= 0 ? outcome.results[causeIndex]?.errors?.[0]?.message : undefined; + + const results = outcome.results.slice(); + for (let index = results.length; index < records.length; index++) { + results.push({ + // Echoed back so a caller can retry exactly the skipped rows. + id: records[index]?.id, + success: false, + index, + errors: [{ + code: 'NOT_ATTEMPTED' as const, + message: `record ${causeIndex} failed — ${cause ?? 'unknown error'}; the batch stopped there. ` + + 'Set options.continueOnError to process the remaining records.', + }], + }); + } + + // Every padded row is a non-success, so this stays a PARTITION of + // `results` rather than a second tally free to drift from it. + return { results, succeeded: outcome.succeeded, failed: results.length - outcome.succeeded }; + } + /** The ordinary (committed) batch response — every row reports what it did. */ private buildBatchDataResponse( operation: BatchUpdateRequest['operation'], @@ -7449,7 +7517,7 @@ export class ObjectStackProtocolImplementation implements options: BatchUpdateRequest['options'], outcome: BatchDataLoopOutcome, ): BatchUpdateResponse { - const { results, succeeded, failed } = outcome; + const { results, succeeded, failed } = this.reconcileStoppedBatch(records, outcome); // [#7539] return { success: failed === 0, operation, @@ -7729,7 +7797,11 @@ export class ObjectStackProtocolImplementation implements records: UpdateManyDataRequest['records'], outcome: BatchDataLoopOutcome, ): BatchUpdateResponse { - const { results, succeeded, failed } = outcome; + // [#7539] Same under-report as `batchData`, ten lines away — the card's + // "per-object bulk counters under-report whenever a row fails without + // `continueOnError`". Fixed through the one shared reconciler, because + // a second copy is how these three drifted apart before (#4620). + const { results, succeeded, failed } = this.reconcileStoppedBatch(records, outcome); return { success: failed === 0, operation: 'update', @@ -7889,7 +7961,10 @@ export class ObjectStackProtocolImplementation implements /** The ordinary (committed) `deleteMany` response — every id reports what it did. */ private buildDeleteManyResponse(ids: unknown[], outcome: BatchDataLoopOutcome): BatchUpdateResponse { - const { results, succeeded, failed } = outcome; + // [#7539] Same reconciliation as the other two faces; `ids` are mapped + // to the `{ id }` row shape the atomic arm already hands the rollback + // builder, so a skipped id comes back echoed and retryable. + const { results, succeeded, failed } = this.reconcileStoppedBatch(ids.map((id) => ({ id: String(id) })), outcome); return { success: failed === 0, operation: 'delete', diff --git a/packages/spec/src/api/batch.zod.ts b/packages/spec/src/api/batch.zod.ts index 6b7138aa62..bc7adbf84e 100644 --- a/packages/spec/src/api/batch.zod.ts +++ b/packages/spec/src/api/batch.zod.ts @@ -78,7 +78,11 @@ export const BatchOptionsSchema = lazySchema(() => z.object({ + '`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'), + continueOnError: z.boolean().optional().default(false).describe( + 'If true (and atomic=false), continue processing remaining records after errors. ' + + 'Default false: the first failure ENDS the run — records before it stay written (nothing is rolled ' + + 'back on this arm), and every record after it is reported `errors[0].code` NOT_ATTEMPTED rather than ' + + 'omitted, so `results` always covers all `total` records and `succeeded + failed === total` (#7539).'), // `validateOnly` promised a dry-run — "validate records without persisting" — // but no batch surface ever read it (`updateManyData` / `deleteManyData` / // `batchData` all persist regardless). A caller sending `validateOnly: true` @@ -193,7 +197,10 @@ export const BatchOperationResultSchema = lazySchema(() => z.object({ errors: z.array(ApiErrorSchema).optional().describe( 'Array of errors if operation failed. Branch on `errors[0].code` — an atomic batch that rolled back ' + 'marks rows that were written then undone with code ROLLED_BACK and rows never reached with ' - + 'NOT_ATTEMPTED, while the causal row keeps its own error (#4793).'), + + 'NOT_ATTEMPTED, while the causal row keeps its own error (#4793). A NON-atomic batch that stopped ' + + '(the `continueOnError: false` default) marks its un-attempted tail with the same NOT_ATTEMPTED code ' + + '— rows before the failure stay written and keep reporting success, since nothing was rolled back ' + + '(#7539).'), data: RecordDataSchema.optional().describe('Full record data (if returnRecords=true)'), index: z.number().optional().describe('Index of the record in the request array'), droppedFields: z.array(DroppedFieldsEventSchema).optional().describe( diff --git a/packages/spec/src/api/error-code-ledger.zod.ts b/packages/spec/src/api/error-code-ledger.zod.ts index 0bcdff0ae5..cb67445756 100644 --- a/packages/spec/src/api/error-code-ledger.zod.ts +++ b/packages/spec/src/api/error-code-ledger.zod.ts @@ -264,7 +264,7 @@ export const ERROR_CODE_LEDGER = { 'METADATA_CONFLICT', 'NAMESPACE_PREFIX', // name violates the package namespace-prefix rule 'NO_DRAFT', - 'NOT_ATTEMPTED', // atomic data-batch row never ran — an earlier row's failure aborted the batch (#4793) + 'NOT_ATTEMPTED', // data-batch row never ran — an earlier row's failure stopped the batch, to roll back (atomic, #4793) or because continueOnError was unset (#7539) 'NOT_CREATABLE', 'NOT_OVERRIDABLE', 'OBJECT_OVERLAY_PACKAGE_MISMATCH', // [ADR-0029 D9.9] object overlay row bound to a package that does not own the object From 7f79c8b00aaa3adeadb799b0509de48dc1f088e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 05:55:14 +0000 Subject: [PATCH 2/2] docs(api): describe the stopped-batch NOT_ATTEMPTED tail on the non-atomic arm (#7539) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `data-api.mdx` described `atomic: false` as "sequential best-effort, stopping at the first failure" without saying what the response contains for the records it never reached — the shape the fix makes explicit. `batch.mdx` is regenerated from the two `.describe()` strings this change touches. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016gd2bypaK4KYs78q8RP38G --- content/docs/api/data-api.mdx | 6 ++++-- content/docs/references/api/batch.mdx | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/content/docs/api/data-api.mdx b/content/docs/api/data-api.mdx index 26e4e1394b..2d83970fe5 100644 --- a/content/docs/api/data-api.mdx +++ b/content/docs/api/data-api.mdx @@ -241,7 +241,7 @@ 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`, `index` (the row's position in the request array), an optional `errors` array (`ApiError[]` — read `errors[0].message`, branch on `errors[0].code`), 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` — each row's `errors[0].code` says what happened: `ROLLED_BACK` (written, then undone), the causal row's own error code, or `NOT_ATTEMPTED` (never reached). 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`. +`options.atomic` defaults to `false`: sequential best-effort that stops at the first failure. Records written before the failure stay written — nothing is rolled back on this arm — and every record after it is reported with `errors[0].code` `NOT_ATTEMPTED` rather than omitted, so `results` always covers all `total` records and `succeeded + failed === total` (#7539). Send `continueOnError: true` to process the remaining records instead of stopping. Set `atomic` to `true` and the whole batch runs inside one transaction: the first failure rolls back every prior write, and the response reports `succeeded: 0` — each row's `errors[0].code` says what happened: `ROLLED_BACK` (written, then undone), the causal row's own error code, or `NOT_ATTEMPTED` (never reached). 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` @@ -284,7 +284,9 @@ selects rows, so no body key can widen the delete into a filter. deleted one at a time by primary key, so each honours `deleteBehavior` (`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. +reports the failures instead. Either way every id gets a `results` entry — the +ids a stopped run never reached carry `errors[0].code` `NOT_ATTEMPTED`, so the +counters reconcile against `total` (#7539). `options.atomic: true` is honoured here the same way as on `/batch` (#4620): the whole id list runs inside one transaction, the first failure rolls back every diff --git a/content/docs/references/api/batch.mdx b/content/docs/references/api/batch.mdx index ae3cf883f3..2af91a31ee 100644 --- a/content/docs/references/api/batch.mdx +++ b/content/docs/references/api/batch.mdx @@ -55,7 +55,7 @@ const result = BatchConfigSchema.parse(data); | :--- | :--- | :--- | :--- | | **id** | `string` | optional | Record ID if operation succeeded | | **success** | `boolean` | ✅ | Whether this record was processed successfully | -| **errors** | `{ code: Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| … +259 more>; message: string; category?: string; httpStatus?: integer; … }[]` | optional | Array of errors if operation failed. Branch on `errors[0].code` — an atomic batch that rolled back marks rows that were written then undone with code ROLLED_BACK and rows never reached with NOT_ATTEMPTED, while the causal row keeps its own error (#4793). | +| **errors** | `{ code: Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| … +259 more>; message: string; category?: string; httpStatus?: integer; … }[]` | optional | Array of errors if operation failed. Branch on `errors[0].code` — an atomic batch that rolled back marks rows that were written then undone with code ROLLED_BACK and rows never reached with NOT_ATTEMPTED, while the causal row keeps its own error (#4793). A NON-atomic batch that stopped (the `continueOnError: false` default) marks its un-attempted tail with the same NOT_ATTEMPTED code — rows before the failure stay written and keep reporting success, since nothing was rolled back (#7539). | | **data** | `Record` | optional | Full record data (if returnRecords=true) | | **index** | `number` | optional | Index of the record in the request array | | **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when' \| 'primary_key'> }[]` | optional | Write-observability (#3407/#3431/#3455): caller-supplied fields LEGALLY stripped from THIS row before it was written — static `readonly` (#2948) / TRUE `readonlyWhen` (#3042) on update, or the #3043 create-ingress strip. Per-row because a batch can drop different fields on different rows (`readonlyWhen` is record-state-dependent). Present ONLY when ≥1 field was dropped for this row; the row still succeeded (success unchanged). A single response header cannot express per-row drops, so this body field is the canonical bulk channel — REST does not emit `X-ObjectStack-Dropped-Fields` for batches. Optional — omit-when-empty keeps the shape backward-compatible. | @@ -83,7 +83,7 @@ const result = BatchConfigSchema.parse(data); | :--- | :--- | :--- | :--- | | **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 — each row carries `errors[0].code` ROLLED_BACK (written, then undone), the causal row its own error, and rows never reached 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 | +| **continueOnError** | `boolean` | ✅ | If true (and atomic=false), continue processing remaining records after errors. Default false: the first failure ENDS the run — records before it stay written (nothing is rolled back on this arm), and every record after it is reported `errors[0].code` NOT_ATTEMPTED rather than omitted, so `results` always covers all `total` records and `succeeded + failed === total` (#7539). | | **validateOnly** | `never` | 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. |