feat(orchestrator)!: finalize batches from their speculation paths - #507
feat(orchestrator)!: finalize batches from their speculation paths#507behinddwalls wants to merge 1 commit into
Conversation
dfb687a to
2b97a64
Compare
2b97a64 to
93573ae
Compare
93573ae to
1492c63
Compare
1492c63 to
23b4741
Compare
23b4741 to
88bb6b3
Compare
88bb6b3 to
d9beb4d
Compare
d9beb4d to
42b8e81
Compare
42b8e81 to
26ae2d9
Compare
7fd59cb to
8ed1570
Compare
8ed1570 to
7266fb6
Compare
7266fb6 to
7ca8a7e
Compare
7ca8a7e to
29c08c8
Compare
25e0321 to
98b83ea
Compare
98b83ea to
5c7c642
Compare
5c7c642 to
b374994
Compare
b374994 to
ca00864
Compare
ca00864 to
f767dc3
Compare
f767dc3 to
eb3fc58
Compare
432bd24 to
054694a
Compare
054694a to
326f20d
Compare
| newVersion := batch.Version + 1 | ||
| updated := batch | ||
| updated.State = state | ||
| if err := store.GetBatchStore().Update(ctx, updated, batch.Version, newVersion); err != nil { |
There was a problem hiding this comment.
corebatch.Transition also updates BatchStateStore - do we need that here as well? Should we be doing this via corebatch.Transition?
There was a problem hiding this comment.
Yes on both counts — good catch, fixed in d2378c5.
The record write is needed, and skipping it is worse than a missing side effect. ListByStates lists each state bucket, hydrates every candidate ID, and classifies by the batch's own State, so a stale record never misreports a batch — but records are only ever added here, never moved. A raw CAS leaves the batch filed under the bucket it just left, so the outcome is right while the bucket grows forever: every later run of that queue lists the whole history and does a point read per entry. Unbounded table growth plus read amplification on every run.
Both batch state writes in this controller now go through corebatch.Transition — applyOutcome here, and admit one function over, which had the same shape. That also picks up Transition's ordering guarantee (Put the new record before Delete-ing the old, so the batch is never unfiled) and its ErrVersionMismatch semantics, which I kept mapped to the existing outcome_cas_lost lost-race branch. As a bonus, admit no longer mutates batch.State before the write, so a failed CAS can't leave the in-memory copy ahead of the row.
The harness previously stubbed the record store with a blanket AnyTimes(), which is why nothing caught this. It now records the writes, and the merging-outcome test asserts the head is filed under merging and dropped from speculating; I checked that assertion fails against the old raw CAS.
## Summary ### Why? Speculation was building the right things and merging on the wrong rule. A head still waited for *every* dependency to succeed before it could merge — the same rule as before any of this — so a batch built without a slow neighbour sat behind that neighbour anyway. The paths were being earned and then ignored. This is the commit that collects them. ### What? A head merges once one of its **passed** paths has had every dependency it *assumed would succeed* actually merge; assumed-failing and ignored dependencies impose no wait. That is the whole speedup: the head waits on what its passing build was actually stacked on, not on its full dependency list. A head fails only when no funded path has a future left, and the winner's siblings are superseded — cancelled to free their slots. Outcomes are reached in `finalize`, before the Speculator is asked, and committed one generation at a time, so a cascade (A fails → B's last path breaks → B fails → maybe C too) resolves in a single run without ever enacting a dependent of an outcome whose own write lost its compare-and-swap. A batch decided by a cascade gets a recovery signal before it turns terminal, since no retry of the triggering message would ever revisit it. Cancellation joins the run: a cancelling batch is just another batch the run walks — its live paths are marked cancelling, the poll loop stops their builds, and whichever later run sees them all stopped drives the batch to Cancelled. A cancelling path with no build link is cancelled immediately; there is deliberately no reservation state and no staleness bound, so a crashed dispatch can never keep a batch out of its terminal state. The legacy per-batch finalizer is deleted; `speculate.go` keeps only admission and message routing, and the package doc gains the batch lifecycle, a worked example, and the finalize step. Batch state writes go through `corebatch.Transition` rather than a raw CAS, so the queue's membership record moves with the state. A raw CAS leaves the batch filed under the bucket it just left, and because records are only ever added, every later run of that queue goes on listing and hydrating it. This covers both the outcome write added here and `admit`, which had the same shape. ## Test Plan ✅ `bazel test //submitqueue/orchestrator/...` — the merge rule is table-driven per assumption; failure, cascade commit ordering, and CAS-loss isolation are covered, and cancellation end to end: never-dispatched, live-build, no-paths, and lost-race cases. ✅ `make fmt`, `make gazelle` # Conflicts: # submitqueue/orchestrator/controller/speculate/BUILD.bazel # submitqueue/orchestrator/controller/speculate/speculate_test.go # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto ec05b712 # Last command done (1 command done): # pick 4462c89 # feat(orchestrator)!: finalize batches from their speculation paths # No commands remaining. # You are currently rebasing branch 'preetam/speculation-finalization' on 'ec05b712'. # # Changes to be committed: # modified: submitqueue/orchestrator/controller/cancel/cancel.go # modified: submitqueue/orchestrator/controller/speculate/BUILD.bazel # modified: submitqueue/orchestrator/controller/speculate/check.go # modified: submitqueue/orchestrator/controller/speculate/dispatch.go # modified: submitqueue/orchestrator/controller/speculate/doc.go # new file: submitqueue/orchestrator/controller/speculate/finalize.go # new file: submitqueue/orchestrator/controller/speculate/outcome.go # new file: submitqueue/orchestrator/controller/speculate/outcome_test.go # modified: submitqueue/orchestrator/controller/speculate/run.go # modified: submitqueue/orchestrator/controller/speculate/run_test.go # modified: submitqueue/orchestrator/controller/speculate/snapshot.go # modified: submitqueue/orchestrator/controller/speculate/speculate.go # modified: submitqueue/orchestrator/controller/speculate/speculate_test.go # # Conflicts: # submitqueue/orchestrator/controller/speculate/speculate.go # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto 9a490fe # Last command done (1 command done): # pick ec12146f # feat(orchestrator)!: finalize batches from their speculation paths # No commands remaining. # You are currently rebasing branch 'preetam/speculation-finalization' on '9a490fe4'. # # Changes to be committed: # modified: submitqueue/orchestrator/controller/speculate/BUILD.bazel # modified: submitqueue/orchestrator/controller/speculate/check.go # modified: submitqueue/orchestrator/controller/speculate/dispatch.go # modified: submitqueue/orchestrator/controller/speculate/doc.go # new file: submitqueue/orchestrator/controller/speculate/finalize.go # new file: submitqueue/orchestrator/controller/speculate/outcome.go # new file: submitqueue/orchestrator/controller/speculate/outcome_test.go # modified: submitqueue/orchestrator/controller/speculate/run.go # modified: submitqueue/orchestrator/controller/speculate/run_test.go # modified: submitqueue/orchestrator/controller/speculate/snapshot.go # modified: submitqueue/orchestrator/controller/speculate/speculate.go # modified: submitqueue/orchestrator/controller/speculate/speculate_test.go # # Conflicts: # submitqueue/orchestrator/controller/speculate/dispatch.go # submitqueue/orchestrator/controller/speculate/run.go # submitqueue/orchestrator/controller/speculate/run_test.go # submitqueue/orchestrator/controller/speculate/speculate.go # submitqueue/orchestrator/controller/speculate/speculate_test.go # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto e6ea890a # Last command done (1 command done): # pick 25e0321 # feat(orchestrator)!: finalize batches from their speculation paths # No commands remaining. # You are currently rebasing branch 'preetam/speculation-finalization' on 'e6ea890a'. # # Changes to be committed: # modified: submitqueue/orchestrator/controller/speculate/BUILD.bazel # modified: submitqueue/orchestrator/controller/speculate/check.go # modified: submitqueue/orchestrator/controller/speculate/dispatch.go # modified: submitqueue/orchestrator/controller/speculate/doc.go # new file: submitqueue/orchestrator/controller/speculate/finalize.go # new file: submitqueue/orchestrator/controller/speculate/outcome.go # new file: submitqueue/orchestrator/controller/speculate/outcome_test.go # modified: submitqueue/orchestrator/controller/speculate/run.go # modified: submitqueue/orchestrator/controller/speculate/run_test.go # modified: submitqueue/orchestrator/controller/speculate/snapshot.go # modified: submitqueue/orchestrator/controller/speculate/speculate.go # modified: submitqueue/orchestrator/controller/speculate/speculate_test.go # # Conflicts: # submitqueue/orchestrator/controller/speculate/run_test.go # submitqueue/orchestrator/controller/speculate/speculate.go # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto 643005d # Last command done (1 command done): # pick 432bd24 # feat(orchestrator)!: finalize batches from their speculation paths # No commands remaining. # You are currently rebasing branch 'preetam/speculation-finalization' on '643005d1'. # # Changes to be committed: # modified: submitqueue/orchestrator/controller/speculate/BUILD.bazel # modified: submitqueue/orchestrator/controller/speculate/check.go # modified: submitqueue/orchestrator/controller/speculate/dispatch.go # modified: submitqueue/orchestrator/controller/speculate/doc.go # new file: submitqueue/orchestrator/controller/speculate/finalize.go # new file: submitqueue/orchestrator/controller/speculate/outcome.go # new file: submitqueue/orchestrator/controller/speculate/outcome_test.go # modified: submitqueue/orchestrator/controller/speculate/run.go # modified: submitqueue/orchestrator/controller/speculate/run_test.go # modified: submitqueue/orchestrator/controller/speculate/snapshot.go # modified: submitqueue/orchestrator/controller/speculate/speculate.go # modified: submitqueue/orchestrator/controller/speculate/speculate_test.go #
326f20d to
d2378c5
Compare
Summary
Why?
Speculation was building the right things and merging on the wrong rule. A head still waited for every dependency to succeed before it could merge — the same rule as before any of this — so a batch built without a slow neighbour sat behind that neighbour anyway. The paths were being earned and then ignored. This is the commit that collects them.
What?
A head merges once one of its passed paths has had every dependency it assumed would succeed actually merge; assumed-failing and ignored dependencies impose no wait. That is the whole speedup: the head waits on what its passing build was actually stacked on, not on its full dependency list. A head fails only when no funded path has a future left, and the winner's siblings are superseded — cancelled to free their slots.
Outcomes are reached in
finalize, before the Speculator is asked, and committed one generation at a time, so a cascade (A fails → B's last path breaks → B fails → maybe C too) resolves in a single run without ever enacting a dependent of an outcome whose own write lost its compare-and-swap. A batch decided by a cascade gets a recovery signal before it turns terminal, since no retry of the triggering message would ever revisit it.Cancellation joins the run: a cancelling batch is just another batch the run walks — its live paths are marked cancelling, the poll loop stops their builds, and whichever later run sees them all stopped drives the batch to Cancelled. A cancelling path with no build link is cancelled immediately; there is deliberately no reservation state and no staleness bound, so a crashed dispatch can never keep a batch out of its terminal state.
The legacy per-batch finalizer is deleted;
speculate.gokeeps only admission and message routing, and the package doc gains the batch lifecycle, a worked example, and the finalize step.Batch state writes go through
corebatch.Transitionrather than a raw CAS, so the queue's membership record moves with the state. A raw CAS leaves the batch filed under the bucket it just left, and because records are only ever added, every later run of that queue goes on listing and hydrating it. This covers both the outcome write added here andadmit, which had the same shape.Test Plan
✅
bazel test //submitqueue/orchestrator/...— the merge rule is table-driven per assumption; failure, cascade commit ordering, and CAS-loss isolation are covered, and cancellation end to end: never-dispatched, live-build, no-paths, and lost-race cases.✅
make fmt,make gazelle