spec: define non-atomic batch failure behavior - #9
Open
ssilvius wants to merge 3 commits into
Open
Conversation
SPEC.md S6.2 covered only the atomic failure case. The rule a client must actually code against lived at conformance/README.md:51 -- a document S1 gives no normative authority -- and that document invented a permission the spec never granted: 200 with partial results. Three clauses, adjacent because the adjacency is the point: 1. Forbid partial success. A results array MUST match the batch length; a failure returns the error envelope for atomic AND non-atomic. 2. error.statementIndex is REQUIRED for non-atomic batch failures, RECOMMENDED for atomic. Scoped deliberately: both reference servers attach it only on the non-atomic path (worker/DO runBatch), and D1's batch failure surfaces no per-statement index -- an unscoped MUST would make both non-conforming on day one. Correction from smugglr. 3. State that preceding statements REMAIN COMMITTED, with the client MUST NOT in the same block. This sentence existed nowhere. B-4's "fail closed" sitting under B-3's "no rows persisted" reads as nothing-applied, and that false belief corrupts a migration ledger on replay. Verified against both reference servers before writing: on a non-atomic failure each throws and discards the accumulated results, so clause 1 costs them nothing. The conformance doc granted a permission no implementation exercises. Also in scope, beyond the issue's original "spec text only": - conformance/README.md B-3/B-4 rewritten to assert what persists, plus a new B-5 covering that statements AFTER the failure did not execute. Fixing SPEC.md alone would leave the contradicting permission live in the document clients actually read. - examples/reference-client.ts carries appliedCount. The new client MUST is unsatisfiable by a client that discards how far the batch got. undefined means unknown, never zero -- a caller reading zero replays the whole batch. Not touched: S10.1's "honor atomic when not rejected" is issue #3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scope drift, mine. Issue #1 considered a SHOULD-emit on the atomic path and explicitly did NOT file it: "Zero consumers need it, D1 cannot satisfy it, and an unsatisfiable SHOULD is surface for its own sake." I wrote it in from an earlier reflection that predated that reasoning, without re-reading the issue I was implementing. It was also the wrong shape twice over. "SHOULD include it where the underlying engine surfaces it" is a normative keyword qualified by a condition the spec does not define -- which is issue #3's exact defect ("honor atomic when not rejected"), reintroduced inside the fix for #1. S6.2.1 now states the absence positively rather than leaving a gap: nothing persisted on an atomic failure, so there is nothing to locate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third instance of the same defect on this branch, and the one that would have shipped. Clause 3 asserts that statements before the failure persisted and statements after it did not execute. Nothing in SPEC.md made that true. S6.2's results bullet says "in the same order as the request batch" -- that is RESPONSE ordering. S4.2 describes batch as an array and says nothing about how statements are run. So a server that fanned a non-atomic batch out concurrently conformed to every word of the spec and falsified clause 3: a later statement can commit while an earlier one fails, and then statementIndex does not partition the batch at all. The sentence a migration applier acts on was resting on a mechanism the spec never defined -- issue #3's shape, for the third time here. Free, verified the way clause 1 was: both non-atomic runBatch paths are sequential loops that stop at the first failure -- the worker awaits runOne inside `for (let i...)` (index.ts:96-103), the DO calls runOne synchronously in the same shape (index.ts:122-131). Neither pays anything for the MUST. Atomic batches are deliberately exempt: the transaction makes execution order unobservable. Also, reference-client appliedCount: undefined was covering two different situations -- a single-statement request AND a non-atomic batch whose server omitted the REQUIRED index. Only the second is unknown; a failed single statement applied nothing, which is 0. Now `atomic === false ? err.statementIndex : 0`, so undefined means exactly one thing and a caller can branch on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the P0 at #1. A non-atomic batch that failed partway through had no normative behavior anywhere in SPEC.md; the only statement of the rule lived in a document S1 gives no authority, and that document granted a permission the spec never issued.
Acceptance criteria mapping
"S6.2 states the non-atomic failure rule normatively (clause 1)."
New S6.2.1 "Batch failure" at
SPEC.md:141. The old single sentence at the previousSPEC.md:139covered only the atomic case; the replacement opens with the rule for both -- a failure returns the error envelope, not a partialresultsarray. I also added a preceding invariant atSPEC.md:139that aresultsarray MUST match the requestbatchin length, because "return the error envelope" alone does not close the door on a 200 carrying a short array. The length rule is what actually forbids partial success."S7 marks
error.statementIndexREQUIRED for non-atomic batch errors, OPTIONAL otherwise."The bullet at
SPEC.md:170now reads(REQUIRED for non-atomic batch failures, otherwise OPTIONAL, integer). I wrote(CONDITIONAL, ...)first and threw it out before commit: CONDITIONAL is not in S1's RFC-2119 enumeration atSPEC.md:11, so it would have introduced a normative keyword the spec does not define -- the same class of defect as #3, wearing a field table."S6.2 (or S4.2) carries the clause-3 sentence."
Carried at
SPEC.md:150, in smugglr's wording: statements preceding the failing statement have been executed and their effects persist, and clients MUST NOT treat a non-atomic batch error as no-statements-applied. It sits in the same block as clause 1 rather than further down, deliberately -- a reader who meets "the response is the error envelope" without the persistence sentence infers rollback, and that inference is the ledger corruption this issue was filed for. The atomic and non-atomic arms are adjacent bullets (SPEC.md:149-150) so the contrast in what persists is unmissable. The clause also needed a premise it did not have, added atSPEC.md:145: servers MUST execute a non-atomic batch one statement at a time in array order, stopping at the first failure. Without that, clause 3 asserts something the spec never established --SPEC.md:137orders the RESPONSE,SPEC.md:63describesbatchas an array and nothing more, so a server that fanned the batch out concurrently conformed to every word and falsified the clause by committing a later statement while an earlier one failed. Verified free the same way as clause 1 before writing it:cloudflare-worker-to-d1/src/index.ts:96-103awaitsrunOneinside aforloop and throws at the first failure, andcloudflare-durable-object/src/index.ts:127-133is the same shape synchronously. Atomic batches are exempt -- the transaction makes execution order unobservable."
conformance/README.mdB-4 is rewritten to test the spec rule rather than grant a permission; the partial-success permission is removed."Permission gone. B-4 at
conformance/README.md:51now assertserror.statementIndex= 1 and that the first statement's INSERT is present; B-3 at:50asserts the first INSERT is absent. The pair is the point -- the old "recommended behavior is to fail closed" wording, read directly under B-3's "no rows persisted," is what made nothing-applied the natural inference. Added B-5 at:52beyond the criterion: a three-statement non-atomic batch failing at index 1, checking the third statement did not execute. B-4 tests what persisted before the failure; nothing tested that execution stopped, and a server that runs the whole batch and reports the first error passes B-4 while failing B-5."Both reference servers still conform with no code change."
Holds, and I verified it before writing the spec text rather than after, because if either had returned 200-with-partial then clause 1 would make both non-conforming on day one.
examples/cloudflare-worker-to-d1/src/index.tsrunBatchandexamples/cloudflare-durable-object/src/index.tsrunBatchboththrowout of the non-atomic loop and discard the accumulatedoutarray, so neither has ever returned a partial result. Clause 1 codifies shipped behavior; the conformance doc was granting a permission no implementation exercised. Both stampstatementIndexonly on the non-atomic path, which is why clause 2 is scoped there (scoping credit: smugglr, from the applier seat).Beyond the criteria, and why
examples/reference-client.tsgainsappliedCountat line 58. The criterion said "no code change" and meant the servers; the client is a different matter, because S10.2 gains a client MUST and a client that discards how far the batch got cannot satisfy it. The value isatomic === false ? err.statementIndex : 0, soundefinedarises from exactly one condition -- a non-atomic batch whose server omitted the now-REQUIRED index -- and means unknown. My first version hadundefinedcovering a single-statement failure too, which is wrong: nothing applied, that is 0 and fully determined, and a caller branching onundefinedto mean "cannot tell, do not replay" would have got the wrong behavior on the one certain case. Never zero for the unknown case, though: a caller reading zero replays a batch that partly applied, which is the failure mode clause 3 exists to prevent. The amendment is recorded as a comment on #1 rather than by editing its criteria, deliberately -- the pr-write gate validated this mapping against those five criteria, and rewriting them afterward would make the gate record a claim about criteria that no longer exist.SPEC.md:220-221(S10.1 items 6 and 7) andSPEC.md:238(S10.2 item 5) add three conformance items between them, renumbering the items after each. S10 is by construction a restatement of S2-S9, so a new normative obligation absent from the checklist is a gap in the checklist.Not done
Nothing on the atomic path. Commit e86d06c reverts my own overreach: I had written a RECOMMENDED
statementIndexfor atomic failures, carried in from a reflection that predated #1's own reasoning. #1's body records that SHOULD as considered-and-rejected -- zero consumers need it, D1's batch failure surfaces no per-statement index, and an unsatisfiable SHOULD is surface for its own sake. Worse, my qualifier ("where the underlying engine surfaces it") was a normative keyword gated on a condition the spec does not define, which is #3's defect reproduced inside the fix for #1.SPEC.md:152now states the absence positively: nothing persisted, so there is nothing to locate.S10.1's "Honor
atomic: trueon batch requests when not rejected" (SPEC.md:217) is untouched. That is issue #3, and it needs its own decision about whether a server gets a conforming way to decline.No
result | errorunion per statement. Considered in #1 and rejected there; forbid-partial-success is cheaper and matches both reference servers, but only because clauses 2 and 3 ride with it.The three remaining reference-server bugs are not in this branch and are not yet filed as issues: D1
Object.keyscollapsing duplicate join columns (cloudflare-worker-to-d1/src/index.ts:109), the zero-row SELECT returningcolumns: []on the same line, and DOrowsAffectedcomputed as cumulativerowsWrittenminus rows read (cloudflare-durable-object/src/index.ts:117). All were found in the same review; none is about batch failure semantics. The fourth finding from that group,implementations.md:18, is filed as #10 and landing separately in #11.Closes #1