fix(speculate): write the merge state before dispatching the batch - #585
Merged
Merged
Conversation
behinddwalls
marked this pull request as ready for review
August 13, 2026 18:41
behinddwalls
force-pushed
the
preetam/speculate-write-before-dispatch
branch
from
August 13, 2026 18:42
ea69c25 to
bcea46e
Compare
## Summary ### Why? `applyOutcome` published a batch to the merge topic before writing `BatchStateMerging`, so a lost compare-and-swap could leave Runway acting on an outcome that was never recorded. That ordering existed to avoid a stall, and the stall is real: nothing re-drives a batch stuck in `Merging`. `Process` self-heals only terminal and `Created` batches, `finalize` walks only heads that are still speculating, and the sole production reader of `BatchStateMerging` is the cancel controller — so a batch written `Merging` whose dispatch never went out would sit there forever. Giving that stall a repair path lets the write come first, which is the ordering the rest of the state machine already wants. ### What? `applyOutcome` is restructured into decide-state → recover → write → dispatch. The `terminal` bool falls out: a second switch mirrors the first and dispatches merge or conclude once the state write has landed. `recoverable` is hoisted above the switch so a cascade-decided *merge* gets a recovery message too, not just a cascade-decided failure. It needs one for the same reason: the write drops the batch out of the speculating set, and `Process`'s self-heal only ever names the trigger batch. `Process` gains a `BatchStateMerging` branch that re-sends the dispatch through the new `dispatchMerge` helper. That keeps the stable `IntentID`, the inverse of `fanout`'s `UniqueID` — for conclude a stable ID would suppress the repair, for merge it is what stops Runway merging the batch twice. One side benefit: a lost state CAS now means the dispatch is never sent at all, narrowing the window where a cancelled batch has a live merge request against it. ## Test Plan ✅ `bazel test //submitqueue/... //platform/...` — 68 tests pass New coverage: the dispatch follows the state write; a lost CAS publishes nothing; a cascade-merged batch gets its recovery signal before the write; `Process` on a `Merging` batch re-dispatches; `dispatchMerge` reuses one message ID per batch. `TestProcess_MergingRunsButDoesNotAct` asserted the old behaviour — that a `Merging` batch publishes nothing — and is replaced by `TestProcess_MergingSelfHeals`.
behinddwalls
force-pushed
the
preetam/speculate-write-before-dispatch
branch
from
August 13, 2026 19:06
bcea46e to
854d9b2
Compare
mnoah1
approved these changes
Aug 13, 2026
manjari25
approved these changes
Aug 13, 2026
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.
Summary
Why?
applyOutcomepublished a batch to the merge topic before writingBatchStateMerging, so a lost compare-and-swap could leave Runway acting on an outcome that was never recorded. That ordering existed to avoid a stall, and the stall is real: nothing re-drives a batch stuck inMerging.Processself-heals only terminal andCreatedbatches,finalizewalks only heads that are still speculating, and the sole production reader ofBatchStateMergingis the cancel controller — so a batch writtenMergingwhose dispatch never went out would sit there forever.Giving that stall a repair path lets the write come first, which is the ordering the rest of the state machine already wants.
What?
applyOutcomeis restructured into decide-state → recover → write → dispatch. Theterminalbool falls out: a second switch mirrors the first and dispatches merge or conclude once the state write has landed.recoverableis hoisted above the switch so a cascade-decided merge gets a recovery message too, not just a cascade-decided failure. It needs one for the same reason: the write drops the batch out of the speculating set, andProcess's self-heal only ever names the trigger batch.Processgains aBatchStateMergingbranch that re-sends the dispatch through the newdispatchMergehelper. That keeps the stableIntentID, the inverse offanout'sUniqueID— for conclude a stable ID would suppress the repair, for merge it is what stops Runway merging the batch twice.One side benefit: a lost state CAS now means the dispatch is never sent at all, narrowing the window where a cancelled batch has a live merge request against it.
Test Plan
✅
bazel test //submitqueue/... //platform/...— 68 tests passNew coverage: the dispatch follows the state write; a lost CAS publishes nothing; a cascade-merged batch gets its recovery signal before the write;
Processon aMergingbatch re-dispatches;dispatchMergereuses one message ID per batch.TestProcess_MergingRunsButDoesNotActasserted the old behaviour — that aMergingbatch publishes nothing — and is replaced byTestProcess_MergingSelfHeals.Stack