diff --git a/.changeset/19452-batch-causal-row-located-by-fault.md b/.changeset/19452-batch-causal-row-located-by-fault.md new file mode 100644 index 00000000000..d11523bb1a2 --- /dev/null +++ b/.changeset/19452-batch-causal-row-located-by-fault.md @@ -0,0 +1,25 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +fix(metadata-protocol): a stopped or rolled-back bulk batch names the row that actually failed + +`reconcileStoppedBatch` and `buildRolledBackBatchResponse` — the two builders every one of the three bulk-write faces (`batchData`, `updateManyData`, `deleteManyData`) reports through — located the causal row with `findIndex(r => !r.success)`. That encoded an invariant: **`!success` means this row failed, and it carries `errors[0]`.** + +That invariant stopped holding when a matched-but-deliberately-not-removed row started answering `success: false` with no `errors` entry — correctly, because a surviving record is an outcome, not a fault. The locator could then land on that survivor, `errors?.[0]?.message` was `undefined`, and the message named the **wrong index** while calling the real error — sitting in the same array — 「unknown error」. + +Measured on the unfixed tree: + +- non-atomic `deleteMany ['survivor', 'missing', 'other']` — the un-attempted row answered `NOT_ATTEMPTED` *"record 0 failed — unknown error; the batch stopped there. …"* while record **1** is what threw; +- atomic `[t1, survivor, t3]` — the rolled-back rows answered `ROLLED_BACK` *"record 1 failed — unknown error"* for a row that **survived**; +- atomic `[t3, survivor, missing, t2]` — `ROLLED_BACK` said *"record 1 failed — unknown error"* and `NOT_ATTEMPTED` said *"atomic batch aborted by record 1"*, both naming the survivor while record **2** threw. + +Both builders now share one locator, `locateBatchCause`, which finds the row by its recorded **fault** — the row's `errors[]` entry. That is the one per-row value whose declared meaning is a failure: `BatchOperationResultSchema.errors` is documented as *"Array of errors if operation failed"*, and the v17 migration entry publishes `row.errors?.[0]?.message` / `.code` to consumers as exactly that read. Its codes are drawn from the closed `StandardErrorCode ∪ ERROR_CODE_LEDGER` vocabulary, so an unregistered code fails `BatchOperationResultSchema.parse` — giving a non-fault ending an `errors[]` entry is a ledger widening in `packages/spec`, not something a call site can do on its own. `ApiError.message` is required, so a located cause always has text and the 「unknown error」 fallback is **deleted** rather than merely unreached. + +The scan runs from the end of the attempted rows, because a run ends *at* the row it stops on: every stop is a `break` in a loop's `catch`, immediately after that row was pushed. A fault that does not stop the run (the `Unknown operation:` arm records one and keeps going) therefore cannot shadow the row that did. + +One ending has no fault to quote at all — an atomic batch aborted by a lone survivor, where `runAtomicBatch` rolls back on `failed > 0` and nothing ever threw. The rolled-back rows now read *"record 1 did not succeed"*: the row that stopped the batch committing, named as what it is rather than as a failure with an unknown cause. + +No envelope field, per-row code, status or count changes; `succeeded` and `failed` still partition `results`. What changes is which row two message strings name, and both of them stop inventing an error that is not there. Clients branch on `errors[0].code`, which is unchanged — the row classification itself was never wrong. + +`Clause-②: no` — nothing authorable moves: no `packages/spec` key, export, accept set or stored shape changes, and the per-row code vocabulary is untouched. diff --git a/packages/metadata-protocol/src/protocol.batch-causal-row.test.ts b/packages/metadata-protocol/src/protocol.batch-causal-row.test.ts new file mode 100644 index 00000000000..1aef2450f56 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.batch-causal-row.test.ts @@ -0,0 +1,324 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#19452] A stopped or rolled-back bulk batch must name the row that actually + * failed — and must never call a real error 「unknown」 while it is sitting in + * the same array. + * + * `reconcileStoppedBatch` and `buildRolledBackBatchResponse` located the causal + * row with `findIndex(r => !r.success)`, which encoded the invariant + * **`!success` ⇒ this row failed and carries `errors[0]`**. #19412 broke that + * invariant deliberately and correctly: a row that MATCHED and was NOT removed + * (`IDataEngine.delete` answering the count arm's `0`) now reports + * `success: false` with ⛔ no `errors` entry, because a surviving record is an + * OUTCOME, not a fault. + * + * ⇒ the locator landed on that survivor, `errors?.[0]?.message` was + * `undefined`, and the message named the WRONG index while falling back to + * 「unknown error」. Measured on the unfixed tree, with the harness below: + * + * deleteMany ['t1'(survives), 'missing'(throws), 't3'] + * -> results[2] NOT_ATTEMPTED "record 0 failed — unknown error; the batch + * stopped there. ..." ⚠️ record 1 is what threw + * batchData atomic [t1, t2(survives), t3] + * -> results[0] ROLLED_BACK "record 1 failed — unknown error" + * ⚠️ record 1 SURVIVED; nothing failed + * + * The negative case — a batch whose FIRST non-success row is a survivor — is + * what these pins exist for. The ordinary batches at the bottom are the + * positive controls: the same assertions on a run with no survivor in it, so a + * locator that simply stopped naming anything cannot pass this file. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { + name: 'showcase_private_note', + fields: { + title: { name: 'title', type: 'text' }, + }, +}; + +/** The row the fake engine rejects — a classified failure, as `toRowApiError` expects. */ +const POISON = '__invalid__'; + +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 same + * harness shape the #4620 / #4793 / #7539 suites use, so every row asserted + * here is produced by the actual loops, builders and rollback classifier. + * + * `delete` speaks the COUNT arm of `IDataEngine.delete`: an unknown id keeps + * the contract's `false` (which the loop turns into a thrown + * `RECORD_NOT_FOUND`), the nominated `survivor` matches and is deliberately + * kept (`0`), everything else really goes (`1`). + */ +function makeCountingEngine(survivor: string) { + 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) => { + assertEngineUpdateDispatch(data, options); + const id = options?.where?.id; + if (data?.title === POISON) throw validationFailure(); + const next = { ...rows.get(id), ...data }; + rows.set(id, next); + return next; + }); + const del = vi.fn(async (_object: string, options?: any) => { + assertEngineDeleteDispatch(options); + const id = options?.where?.id; + if (!rows.has(id)) return false; // [#4435] the positive not-found value + if (id === survivor) return 0; // [#19412] matched, deliberately NOT removed + rows.delete(id); + return 1; + }); + const findOne = vi.fn(async (_object: string, options?: any) => { + assertEngineFindOnePredicate(_object, options); + return 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 }; +} + +const codesOf = (res: any): Array => + res.results.map((r: any) => r.errors?.[0]?.code); + +/** + * The card's acceptance, as ONE assertion usable on every message that names a + * causal row: it names the row that really failed, it carries that row's own + * error text, and it never says 「unknown error」 while that text exists. + * + * `causalIndex` is read from the response rather than hard-coded, so the pin + * asserts an AGREEMENT between the message and the rows beside it. + */ +function expectAttributedTo(message: string, res: any, causalIndex: number) { + const causalText = res.results[causalIndex].errors[0].message; + expect(causalText).toBeTruthy(); + expect(message).toContain(`record ${causalIndex} failed`); + expect(message).toContain(causalText); + expect(message).not.toContain('unknown error'); +} + +describe('[#19452] a stopped batch attributes the stop to the row that threw, not to a survivor', () => { + it('deleteManyData: a leading survivor does not become the cause', async () => { + const t = makeCountingEngine('t1'); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.deleteManyData({ + object: 'showcase_private_note', + ids: ['t1', 'definitely_missing', 't3'], + } as any); + + // Row 0 survived (no `errors`), row 1 threw, row 2 was never attempted. + expect(codesOf(res)).toEqual([undefined, 'RECORD_NOT_FOUND', 'NOT_ATTEMPTED']); + expect(res.results[0]).toMatchObject({ id: 't1', success: false }); + expect(res.results[0].errors).toBeUndefined(); // why `!success` lies here + expect(t.rows.has('t1')).toBe(true); // it really is still there + + // Pre-fix: "record 0 failed — unknown error; the batch stopped there. ..." + const message = res.results[2].errors[0].message; + expectAttributedTo(message, res, 1); + expect(message).not.toContain('record 0'); + expect(message).toContain('continueOnError'); + }); + + it('batchData delete: the same run through the other non-atomic face', async () => { + const t = makeCountingEngine('t1'); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { operation: 'delete', records: [{ id: 't1' }, { id: 'definitely_missing' }, { id: 't3' }] }, + } as any); + + expect(codesOf(res)).toEqual([undefined, 'RECORD_NOT_FOUND', 'NOT_ATTEMPTED']); + expect(res.results[0].errors).toBeUndefined(); + + const message = res.results[2].errors[0].message; + expectAttributedTo(message, res, 1); + expect(message).not.toContain('record 0'); + }); +}); + +describe('[#19452] a rolled-back atomic batch attributes the abort to the row that threw', () => { + /** + * One run that reaches all THREE message sites: a committed-then-undone + * row, a survivor, the row that threw, and a row never reached. The two + * interpolations of the causal index get SEPARATE tests, so neither can + * mask the other's reading when this file runs red. + */ + const atomicRunReachingBothInterpolations = async () => { + const t = makeCountingEngine('t1'); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { + operation: 'delete', + records: [{ id: 't3' }, { id: 't1' }, { id: 'definitely_missing' }, { id: 't2' }], + options: { atomic: true }, + }, + } as any); + + expect(res).toMatchObject({ success: false, total: 4, succeeded: 0, failed: 4 }); + expect(codesOf(res)).toEqual(['ROLLED_BACK', undefined, 'RECORD_NOT_FOUND', 'NOT_ATTEMPTED']); + return { t, res }; + }; + + it('the ROLLED_BACK message names record 2, not the survivor at record 1', async () => { + const { t, res } = await atomicRunReachingBothInterpolations(); + + // Pre-fix: "record 1 failed — unknown error" — record 1 SURVIVED. + expectAttributedTo(res.results[0].errors[0].message, res, 2); + expect(res.results[0].errors[0].message).not.toContain('record 1'); + + // The rollback is real, and the survivor still carries no `errors`. + expect(res.results[1].errors).toBeUndefined(); + expect(t.rows.has('t1')).toBe(true); + expect(t.rows.has('t2')).toBe(true); + expect(t.rows.has('t3')).toBe(true); + }); + + it('the NOT_ATTEMPTED message names record 2 too — it reads the same index', async () => { + // ⛔ Not assumed to ride along with the two 「unknown error」 sites. This + // wording never says 「failed」 and never quotes a cause, so its only + // defect was the INDEX — but that index is the shared one, so it moves + // with the locator. Pre-fix this read "atomic batch aborted by record 1". + const { res } = await atomicRunReachingBothInterpolations(); + + expect(res.results[3].errors[0].message).toBe('atomic batch aborted by record 2'); + }); + + it('a rollback caused by a survivor ALONE reports no failure at all', async () => { + // Nothing threw: `runAtomicBatch` aborted on `outcome.failed > 0`, which + // a lone survivor satisfies. There is no causal ERROR to quote, so the + // message must not invent one — and must not call the survivor a + // failure either. + const t = makeCountingEngine('t2'); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { + operation: 'delete', + records: [{ id: 't1' }, { id: 't2' }, { id: 't3' }], + options: { atomic: true }, + }, + } as any); + + expect(codesOf(res)).toEqual(['ROLLED_BACK', undefined, 'ROLLED_BACK']); + // No row in the whole response carries a fault entry of its own. + expect(res.results.some((r: any) => r.errors?.[0]?.code === 'RECORD_NOT_FOUND')).toBe(false); + + // Pre-fix: "record 1 failed — unknown error". + for (const row of [res.results[0], res.results[2]]) { + expect(row.errors[0].message).toBe('record 1 did not succeed'); + expect(row.errors[0].message).not.toContain('unknown error'); + expect(row.errors[0].message).not.toContain('failed'); + } + expect(t.rows.has('t2')).toBe(true); + }); +}); + +describe('[#19452] CONTROLS — an ordinary batch, with no survivor in it, attributes exactly as before', () => { + const threeCreates = [ + { data: { title: 'first valid' } }, + { data: { title: POISON } }, + { data: { title: 'third valid' } }, + ]; + + it('batchData create: the stopped tail still names record 1 and quotes its error', async () => { + // The positive control for `expectAttributedTo`: this leg is green on + // the unfixed tree too, so a green above is about the survivor case and + // ⛔ not about an assertion that can no longer fail. + const t = makeCountingEngine('none_of_them'); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { operation: 'create', records: threeCreates }, + } as any); + + expect(codesOf(res)).toEqual([undefined, 'VALIDATION_FAILED', 'NOT_ATTEMPTED']); + expectAttributedTo(res.results[2].errors[0].message, res, 1); + expect(res.results[2].errors[0].message).toContain('title is invalid'); + }); + + it('batchData create atomic: ROLLED_BACK still names record 1 and quotes its error', async () => { + const t = makeCountingEngine('none_of_them'); + const p = new ObjectStackProtocolImplementation(t.engine); + + const res: any = await p.batchData({ + object: 'showcase_private_note', + request: { operation: 'create', records: threeCreates, options: { atomic: true } }, + } as any); + + expect(codesOf(res)).toEqual(['ROLLED_BACK', 'VALIDATION_FAILED', 'NOT_ATTEMPTED']); + expectAttributedTo(res.results[0].errors[0].message, res, 1); + expect(res.results[2].errors[0].message).toBe('atomic batch aborted by record 1'); + }); + + it('updateManyData: the third bulk face cannot produce an errors-less non-success row', async () => { + // This face is covered by REASONING rather than by a survivor pin: it + // has no producer of a non-success row without `errors` — every push in + // `runUpdateManyLoop` is either `success: true` or a `toRowApiError` + // row — so `!success` and "carries a fault" still coincide on it. The + // reading is asserted, not asserted-about: every non-success row here + // carries an `errors` entry, which is exactly what the delete faces + // above violate. + const t = makeCountingEngine('none_of_them'); + 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); + + expect(codesOf(res)).toEqual([undefined, 'VALIDATION_FAILED', 'NOT_ATTEMPTED']); + expect(res.results.filter((r: any) => r.success === false).every((r: any) => (r.errors?.length ?? 0) > 0)).toBe(true); + // And it shares the two builders, so the attribution moves with them. + expectAttributedTo(res.results[2].errors[0].message, res, 1); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index be9dd23ad9c..2204cd26953 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -12700,8 +12700,7 @@ export class ObjectStackProtocolImplementation implements ): 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 cause = this.locateBatchCause(outcome.results); const results = outcome.results.slice(); for (let index = results.length; index < records.length; index++) { @@ -12712,7 +12711,7 @@ export class ObjectStackProtocolImplementation implements index, errors: [{ code: 'NOT_ATTEMPTED' as const, - message: `record ${causeIndex} failed — ${cause ?? 'unknown error'}; the batch stopped there. ` + message: (cause ? `${cause.clause}; the batch stopped there. ` : 'the batch stopped before this record. ') + 'Set options.continueOnError to process the remaining records.', }], }); @@ -12723,6 +12722,51 @@ export class ObjectStackProtocolImplementation implements return { results, succeeded: outcome.succeeded, failed: results.length - outcome.succeeded }; } + /** + * The row that ENDED the run, plus the clause both builders interpolate. + * + * ⛔ NOT `findIndex(r => !r.success)`. `success` is the envelope's + * *outcome* bit, and its false arm is open by construction: it means "this + * row is not a success", which since #19412 also covers a row that MATCHED + * and was deliberately NOT removed — an outcome, not a fault, carrying no + * `errors[]` entry. Locating the cause with it named that survivor and then + * called the real error, sitting in the same array, "unknown" (#19452). + * + * The discriminator is the row's `errors[]` entry, which is the ONE per-row + * value whose declared meaning is a fault: + * `BatchOperationResultSchema.errors` is documented as *"Array of errors if + * operation failed"*, and the ADR-0087 v17 migration entry publishes + * `row.errors?.[0]?.message` / `.code` to consumers as exactly that read. + * It cannot widen the way the boolean did: every entry must carry an + * `ApiError.code` drawn from the closed `StandardErrorCode ∪ + * ERROR_CODE_LEDGER` vocabulary, so an unregistered code fails + * `BatchOperationResultSchema.parse` — giving a non-fault ending an + * `errors[]` entry is a ledger widening in `packages/spec`, which is + * precisely the step both survivor sites declined to take. `message` is + * REQUIRED on `ApiErrorSchema`, so a located cause always has one and the + * 「unknown error」 fallback is gone rather than merely unreached. + * + * Scanned from the END because a run ends AT the row it stops on: every + * stop is a `break` in a loop's `catch`, immediately after that row was + * pushed. A fault that does NOT stop the run (the `Unknown operation:` arm + * records one and keeps going) must not be able to shadow the row that did. + * + * When no row recorded a fault at all the batch still ended for a reason — + * `runAtomicBatch` aborts on `outcome.failed > 0`, which a lone survivor + * satisfies — so the non-success row is named as what it is, ⛔ never as a + * failure and ⛔ never as an 「unknown error」. + */ + private locateBatchCause( + rows: ReadonlyArray, + ): { index: number; clause: string } | undefined { + for (let index = rows.length - 1; index >= 0; index--) { + const fault = rows[index]?.errors?.[0]; + if (fault) return { index, clause: `record ${index} failed — ${fault.message}` }; + } + const stalled = rows.findIndex(r => !r.success); + return stalled >= 0 ? { index: stalled, clause: `record ${stalled} did not succeed` } : undefined; + } + /** The ordinary (committed) batch response — every row reports what it did. */ private buildBatchDataResponse( operation: BatchUpdateRequest['operation'], @@ -12771,21 +12815,25 @@ export class ObjectStackProtocolImplementation implements outcome: BatchDataLoopOutcome, ): BatchUpdateResponse { const attempted = outcome.results; - const causeIndex = attempted.findIndex(r => !r.success); - const cause = causeIndex >= 0 ? attempted[causeIndex]?.errors?.[0]?.message : undefined; + // [#19452] Same locator as the stopped-batch arm, for the same reason: + // `!success` stopped meaning "this row failed" when #19412 widened it. + // Both this builder's interpolations of the causal index read it, so + // neither the ROLLED_BACK message nor the NOT_ATTEMPTED one can name a + // row that merely survived — the two could not be fixed apart. + const cause = this.locateBatchCause(attempted); const results: BatchDataRowResult[] = records.map((record, index) => { const attempt = attempted[index]; if (!attempt) { return { id: record.id, success: false, index, - errors: [{ code: 'NOT_ATTEMPTED' as const, message: `atomic batch aborted by record ${causeIndex}` }], + errors: [{ code: 'NOT_ATTEMPTED' as const, message: cause ? `atomic batch aborted by record ${cause.index}` : 'atomic batch aborted' }], }; } if (attempt.success) { return { id: attempt.id ?? record.id, success: false, index, - errors: [{ code: 'ROLLED_BACK' as const, message: `record ${causeIndex} failed — ${cause ?? 'unknown error'}` }], + errors: [{ code: 'ROLLED_BACK' as const, message: cause ? cause.clause : 'the atomic batch rolled back' }], }; } return { id: attempt.id ?? record.id, success: false, index, errors: attempt.errors }; diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 3a63859e226..f44d5c7c242 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -266,6 +266,21 @@ "verb": "update", "pinned": 2 }, + { + "file": "packages/metadata-protocol/src/protocol.batch-causal-row.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/metadata-protocol/src/protocol.batch-causal-row.test.ts", + "verb": "findOne", + "pinned": 2 + }, + { + "file": "packages/metadata-protocol/src/protocol.batch-causal-row.test.ts", + "verb": "update", + "pinned": 2 + }, { "file": "packages/metadata-protocol/src/protocol.batch-not-attempted.test.ts", "verb": "delete",