fix(metadata-protocol): a stopped or rolled-back bulk batch names the row that actually failed - #19700
Conversation
…corded fault, not by `!success` `reconcileStoppedBatch` and `buildRolledBackBatchResponse` both found the row that ended a bulk run with `findIndex(r => !r.success)`, encoding the invariant "`!success` means this row failed and carries `errors[0]`". #19412 broke that invariant on purpose: a row that MATCHED and was deliberately NOT removed now answers `success: false` with no `errors` entry, because a surviving record is an outcome rather than a fault. So the locator could land on that survivor: the message named the wrong index and called the real error — sitting in the same array — "unknown error". Both builders now share one locator that reads the row's `errors[]` entry, the one per-row value whose declared meaning is a fault (`BatchOperationResultSchema` documents it as "Array of errors if operation failed", and its codes are drawn from the closed StandardErrorCode + ledger vocabulary). `ApiError.message` is required, so the "unknown error" fallback is deleted rather than merely unreached. When nothing recorded a fault at all — an atomic batch aborted by a lone survivor — the message names the row that did not succeed instead of inventing a failure. Claude-Session: https://claude.ai/code/session_01NcPSwnmJHczmTu6FG7NMjE Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NcPSwnmJHczmTu6FG7NMjE Co-authored-by: Claude <noreply@anthropic.com>
…ledger Regenerated with `node scripts/check-engine-double-contract.mjs --write`: 3 rows added, 0 lost, all naming the new pin file. Claude-Session: https://claude.ai/code/session_01NcPSwnmJHczmTu6FG7NMjE Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift CheckThis PR changes 1 package(s): 4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 11 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin f3ab3dc27c506a32ad3f5ce6083dbc6c18b56f8b && git checkout f3ab3dc27c506a32ad3f5ce6083dbc6c18b56f8b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 7e1b048a1dbd7142bd6aa2cd9cbcccf7e1c69345 56e5fd90c02c46c35cef5afb492aecb15e49e3a4 && git checkout -B drift-repro 7e1b048a1dbd7142bd6aa2cd9cbcccf7e1c69345 && git merge --no-ff 56e5fd90c02c46c35cef5afb492aecb15e49e3a4
node scripts/docs-audit/affected-docs.mjs --json 7e1b048a1dbd7142bd6aa2cd9cbcccf7e1c69345
|
|
commit on main |
Test Core (5/6) |
rollup |
|---|---|---|
a251aaa19 — this PR |
⛔ failure | failure |
4fba5036f (#19698) |
✅ success | success |
1d41aa885 (#19697) |
✅ success | success |
28f927790 (#19685) |
cancelled (superseded) | success |
⇒ the shard was green on the two commits immediately before this one and red first here.
What the annotations say
Failing step #11 Run this shard's tests, and the package pointer names the package:
command (…/packages/client) /opt/hostedtoolcache/node/22.23.2/x64/bin/pnpm run test exited (1)
Process completed with exit code 1.
⇒ packages/client, which consumes @objectstack/metadata-protocol — the package this PR changes.
⛔ What is NOT established, stated so nobody reads more into this than it holds
- ⛔ The failing assertion.
scripts/pm/ci-failure.mjsreturnedVERDICT: UNDETERMINEDfor it: the job log download redirects offapi.github.comand the request did not complete from this container (GET /repos/…/actions/jobs/106702565388/logs). That is an absence of a READING, ⛔ not an absence of evidence — the stdout exists, this seat could not fetch it. - ⛔ That it is this diff's. 「red first appears at commit X」 is a strong pointer, ⛔ not a proof; a first occurrence can still be a flake, and this seat has run the shard exactly zero times.
- ⛔ Any judgement about the change itself. This seat has not read the diff.
⇒ ⛔ Nothing is asked of this seat's lane and nothing is being taken over. Recorded here because main is red now and the fastest reader is whoever owns this change.
Generated by Claude Code
Fixes #19452
Clause-②: no
A stopped or rolled-back bulk batch named a causal row that did not fail, and called the real error 「unknown error」 while that error was sitting in the same array.
The defect
reconcileStoppedBatchandbuildRolledBackBatchResponseboth located the causal row withfindIndex(r => !r.success). That encoded one invariant:!successmeans this row failed, and it carrieserrors[0].PR #19432 broke that invariant deliberately and correctly: a row that MATCHED and was NOT removed now answers
success: falsewith noerrorsentry, because a surviving record is an outcome, not a fault. So the locator could land on that survivor,errors?.[0]?.messagewasundefined, and the message named the wrong index while falling back to 「unknown error」.Reproduced before the fix, verbatim
Taken by neutralising the fix on the final tree:
packages/metadata-protocol/src/protocol.tsrestored to the bloborigin/mainholds (be9dd23ad9c865fbcc74ccf20bcc3f633f23845e), proved on disk by blob hash, then the pins run. Received strings, copied out of the run:deleteMany ['t1'(survives), 'missing'(throws), 't3']NOT_ATTEMPTEDrecord 0 failed — unknown error; the batch stopped there. Set options.continueOnError to process the remaining records.batchData delete, same three rowsNOT_ATTEMPTEDbatchData atomic ['t3', 't1'(survives), 'missing'(throws), 't2']ROLLED_BACKrecord 1 failed — unknown errorNOT_ATTEMPTEDatomic batch aborted by record 1batchData atomic ['t1', 't2'(survives), 't3']ROLLED_BACKrecord 1 failed — unknown errorThe fix
Both builders now call one shared locator,
locateBatchCause, which finds the causal row by its recorded fault: the row'serrors[]entry.Why that discriminator cannot drift back the way the boolean did.
successis the envelope's outcome bit and its false arm is open by construction — it means "this row is not a success", so every new non-success ending widens it for free, which is exactly what happened.errorsis not a second boolean:BatchOperationResultSchema.errorsis documented as "Array of errors if operation failed", and the v17 ADR-0087 migration entry publishesrow.errors?.[0]?.message/row.errors?.[0]?.codeto consumers as that read;ApiError.codefromStandardErrorCodeunionERROR_CODE_LEDGER; an unregistered code failsBatchOperationResultSchema.parse. Giving a non-fault ending anerrors[]entry is therefore a ledger widening inpackages/spec— which is precisely the step BOTH survivor sites declined to take, in writing, and the step that would have to be taken deliberately for this locator to start lying;ApiError.messageis required, so a located cause always has text. The 「unknown error」 fallback is deleted, not merely unreached: the string no longer appears in either message template.The scan runs from the END of the attempted rows, because a run ends AT the row it stops on — every stop is a
breakin a loop'scatch, immediately after that row was pushed. A fault that does not stop the run (theUnknown 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
runAtomicBatchrolls back onfailed > 0and nothing ever threw. There the message names the row that did not succeed —record 1 did not succeed— instead of inventing a failure. That is the only place!successis still read, and it is read for the question that boolean does answer: "which row stopped this batch committing", never "which row failed".Acceptance
1. Located by a fault, not by
!success— above. 2. The negative case is pinned —packages/metadata-protocol/src/protocol.batch-causal-row.test.ts, 8 tests. Its central assertion is an agreement between the message and the rows beside it, read out of the response rather than hard-coded: the message namesrecord N failedfor the N that carries an error, contains that row's own error text verbatim, and never contains the stringunknown error. 3. Both builders — one locator, two call sites; neither can be fixed apart from the other. 4. All three bulk faces:batchData(delete verb)deleteManyDatadeleteManyDatarunAtomicBatch; the atomic delete pin runs throughbatchData, whose loop pushes the identical survivor rowupdateManyDatarunUpdateManyLoophas no producer of a non-success row withouterrors(every push issuccess: trueor atoRowApiErrorrow), so!successand "carries a fault" still coincide there. The pin asserts that reading directly: every non-success row in anupdateManyresponse carries anerrorsentry. It shares the two builders, so the attribution moves with them, which the same test also checksAblation
Neutralised on the final tree (merge commit
56e5fd90c), mutation proved on disk by blob hash2204cd2 -> be9dd23, restored, restore proved: blob back to2204cd2,git diff HEADzero bytes, whole-treegit status --porcelainzero lines.Files outside the declared surface, declared rather than quietly widened
The dispatch scoped this to
packages/metadata-protocol/src/. Two files outside it are in the diff, both mechanical and both demanded by the repo's own gates for the in-surface change:.changeset/19452-batch-causal-row-located-by-fault.md— required by the post-task checklist and bycheck:empty-changeset. Measured rather than assumed:@objectstack/metadata-protocolis not private, itsfiles[]shipsdist, and the changed symbol is in the built output (locateBatchCausepresent indist/index.js; a nonsense control string returns zero from the same grep).patch.scripts/engine-double-contract.pinned.json—check:engine-double-contractexited 1 on the new pin file with its own prescription,--writeand commit. The regeneration reports 3 rows added, 0 lost, and every added row names the new test file (3 lines match the file name, 3 lines match.test.ts— the same count, so no other file moved).Verification
dispatch-gates.mjs --commands --repo objectstack-ai/objectstackon the merged tree (56e5fd90c): 69 families, all 69 run, every one exit 0. Reconciled with--rancarrying exit codes:69 derived, 69 run, 0 NOT-MEASURED, 0 UNRUN. Three families first answered exit 3 (PREREQUISITE NOT MET —dual-build-cjs-loads,lean-entry-closure,type-check-debt); each was discharged by building what it named (turbo run buildover all packages, 72/72) and re-running, never by calling it inapplicable.pnpm --filter @objectstack/metadata-protocol test— 2653 passed, 19 skipped, 0 failed.typecheck— exit 0.pnpm lint— the whole-repoeslint . --no-inline-config, exit 0. Run in full, so no narrowing needs declaring.origin/mainmerged before this reading; the gates above were run on the merged tree.Acceptance notes
Noted here, not filed, per this seat's standing rule that it files nothing:
protocol.batch-not-attempted.test.tsasserts the causal index withexpect(message).toContain('1'). The assertion is satisfied by any1anywhere in the string, so it is much weaker than it reads; it happens to be correct today. Test-quality observation, no live defect, and this PR does not touch that file.runBatchDataLoop'sUnknown operation:arm records aVALIDATION_FAILEDrow per record and keeps going even withcontinueOnErrorabsent, because nothing is thrown.BatchOptionsSchema.continueOnErrordeclares the opposite default.batchDatadoes not parse the verb at its own door, so an in-process caller can reach the arm; the effect is a longerresultsarray and nothing else — no write happens and the counters still reconcile. Reported to the PM as a contract-violation candidate with its seam rather than filed here.Generated by Claude Code