feat(speculation): cancel batches through the tree via prioritize#379
Open
behinddwalls wants to merge 3 commits into
Open
feat(speculation): cancel batches through the tree via prioritize#379behinddwalls wants to merge 3 commits into
behinddwalls wants to merge 3 commits into
Conversation
cancelBatch's build-cancellation step predates the speculation tree: it flipped a single Build entity keyed directly by batch.ID, with no build runner in the loop, because there was no external runner yet and no tree to consult. That model is now out of step with the rest of the stack — builds are found through the tree's per-path build mapping (a batch can have more than one in-flight build across its paths), and the build stage now owns real runner interaction for path-level work. Batch-level cancellation needs the same runner access, for the same reason speculate needed a build runner in the first place, but it earns a deliberate exception to "only the build stage calls the runner": build.go's buildsignal handler short-circuits halted batches, so it never gets a chance to enact a batch-terminal cancel itself. Something has to call Cancel for the batch-wide case, and cancelBatch is that something. cancelBatch now runs cancelBuilds followed by cancelTree instead of the old single-build cancelBuild. cancelBuilds walks the batch's speculation tree, resolves each path's build via the path->build mapping (mirroring reconcile's two-hop lookup and its handling of a missing tree, a path with no mapping yet, or a mapping pointing at a since-vanished build row), and for every still-running build calls the queue's build runner's Cancel before flipping it locally to BuildStatusCancelled — resolving the runner lazily so a batch with nothing in flight never touches the factory. cancelTree then marks every remaining non-terminal path Cancelled under the tree's version lock, so a stray reconcile pass on redelivery sees a tree already consistent with the batch's terminal state. The Controller gains a buildRunners buildrunner.Factory dependency, threaded through NewController and wired in service/submitqueue/orchestrator/server/main.go with the same brf factory already used by the build and buildsignal controllers. This is the only place in the speculate package that talks to a build runner: applySelection and reconcile still only ever record cancel intent (SpeculationPathStatusCancelling) for a single dead or deselected path, leaving enactment to the build stage. Doc comments across the package are updated to describe the tree-driven algorithm end to end (speculateBatch's nine-step sequence, cancelBatch's five-step order of operations) and to call out this file's one runner-calling exception explicitly, so the ownership rule doesn't read as absolute where it isn't. `go build ./...` and `go vet ./submitqueue/orchestrator/controller/speculate/...` — clean. `go test ./submitqueue/orchestrator/controller/speculate/...` — new table-driven CancellingFlow test covers two non-terminal builds cancelled and their tree paths marked Cancelled, an already-terminal build left untouched, a missing tree, a path with no mapping yet, and a mapping pointing at a missing build row; CancellingTerminalCASVersionMismatch is updated for the new double tree-read shape. `go test ./submitqueue/... ./service/...` — all packages pass. `make gazelle && make fmt` — regenerated BUILD.bazel deps for the new buildrunner imports, no other changes. `bazel test //submitqueue/... //service/...` — 56/56 tests pass. `make e2e-test` — stovepipe and submitqueue e2e suites both pass. This is the last commit in the speculate/build/prioritize re-slice stack; `git diff preetam/int/speculate-rework preetam/int/cancel-via-tree` is empty except for the deliberate D1-D6 deltas verified separately.
Review on the previous slice pushed back on cancelBatch calling the build runner directly: decisions belong in the tree, prioritize is the one channel from tree state to build messages, and the build stage is the sole runner caller. This commit removes the exception instead of documenting it. speculate loses its buildrunner.Factory entirely. cancelBatch becomes a convergent sweep: each pass resolves every non-terminal path through the path->build mapping and either settles it to Cancelled (no build, dangling mapping, or terminal build) or captures intent (Cancelling) while its build is still in flight. While anything is pending the batch stays in BatchStateCancelling and the pass publishes the queue to prioritize; only a pass that finds nothing pending CASes the batch terminal, fans out dependents, and publishes conclude. A terminal-transition guard re-checks Cancelled paths on would-be-final passes and pulls any that still hides a live build (a pre-build Cancel racing the build stage's trigger) back to Cancelling, so the batch never settles over a running build — parity with the old cancelBuilds sweep, which cancelled by mapping regardless of path status. Three stages change to make the loop live end to end: prioritize now loads Cancelling batches' trees (routing only — their paths are never candidates, and rogue decisions naming them are skipped as illegal), the build stage cancels every in-flight build of a Cancelling batch regardless of the path's recorded status — so a build message racing the tree sweep still stops paths that read Building — and never triggers builds for it (its wholesale skip is now terminal-only), and buildsignal keeps polling and re-publishing speculate for Cancelling batches so the wind-down is recorded and the sweep re-woken — previously it silent-acked them, which would deadlock the new flow. The same review suggested optimistic merge finalization (a base published for merge counts as landed for a dependent's merge readiness). This commit does not adopt it: the mysql subscriber keeps delivering later offsets of a partition while an earlier one sits in nack backoff (platform/extension/messagequeue/mysql/subscriber.go), so a speculate-side relaxation alone could let a dependent's merge overtake its still-Merging base and land changes validated on a base that never landed. The next slice in the stack adopts the optimism properly, paired with the merge-stage confirmation gate it requires. Note for the stack: the cancel-flow CAS-race retry classification (34204dcd on preetam/int/cancel-retryable-cas) is not an ancestor of this branch; without it a benign version race on the terminal CAS still dead-letters, so that fix should land first. Metric renames: skipped_halted -> skipped_terminal (build, buildsignal), no_speculating_batches -> no_batches (prioritize). Tests: speculate's harness drops the runner mock; CancellingFlow is rewritten around the two-phase convergence (pending passes publish prioritize only, settled passes run the terminal sequence, the orphaned-build guard re-opens a poisoned Cancelled path); the merge gate pins that a Merging base does NOT merge; build gains a Cancelling-batch test proving every in-flight build (including a still-Building path) is cancelled and nothing triggers; buildsignal splits its halted test into terminal-only skip plus Cancelling-keeps-polling; prioritize gains a routed-not-ranked test including a rogue decision on a Cancelling batch's path. go build ./... && go vet — clean. go test ./submitqueue/... ./service/... ./platform/... — all pass. bazel test //submitqueue/... //service/... — 56/56. make e2e-test — stovepipe and submitqueue suites pass. make lint / check-tidy / check-gazelle — clean.
Review asked for two wording fixes on the build stage's batch-teardown branch: the note about a Cancelled path hiding a live build now also credits speculation-level cancels (a selector or prioritizer decision landing before the build was stamped onto the tree), not just the tree-sweep race; and every comment the branch introduced now says build / build system instead of CI, including the one pre-existing entity doc that still said CI/CD.
behinddwalls
changed the title
This was referenced Jul 16, 2026
albertywu
reviewed
Jul 16, 2026
| } | ||
|
|
||
| newVersion := tree.Version + 1 | ||
| if err := c.store.GetSpeculationTreeStore().Update(ctx, batch.ID, tree.Version, newVersion, paths); err != nil { |
Contributor
There was a problem hiding this comment.
High risk: a normal cancel-tree CAS race can force Cancelling to Failed. A stale speculate or prioritize worker can advance the tree after this pass reads it. This conditional update then returns wrapped storage.ErrVersionMismatch. Production does not classify that sentinel as retryable, so the cancellation message can enter DLQ; DLQ reconciliation changes the non-terminal cancelling batch to Failed, potentially before a live build is stopped. Reload and recompute on expected optimistic conflicts rather than dead-lettering them. This remains present through PR #380.
| // system, costing extra point reads only on would-be-final passes. | ||
| if pending == 0 { | ||
| for i, p := range paths { | ||
| if resolved[i] || p.Status != entity.SpeculationPathStatusCancelled { |
Contributor
There was a problem hiding this comment.
High risk: this terminal guard skips the exact path a stale build worker can race. A build worker can pass its early batch-state check, while this sweep observes no mapping and resolves the path to Cancelled. Because resolved[i] is true, the guard skips it. The stale worker can then trigger and create the mapping before the batch's terminal CAS, leaving a live build under a terminal batch. The trigger-before-marker race is inherited, but this PR's new guard does not close it. A durable pre-trigger reservation or fence is needed. This remains present through PR #380.
| var candidates []entity.SpeculationPathInfo | ||
| for _, tree := range trees { | ||
| for batchID, tree := range trees { | ||
| if cancelling[batchID] { |
Contributor
There was a problem hiding this comment.
High risk: cancelling work stops counting against capacity before the external build stops. Skipping the whole batch removes its Building or Cancelling paths from occupancy immediately. With a limit of 1, batch A can still be stopping in the runner while batch B is promoted and triggered, producing two active external builds. The omission existed on this PR's base, but it remains unsafe under the new asynchronous wind-down protocol. Separate eligibility from occupancy and count a slot until the runner reports a terminal state. This remains present through PR #380.
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
2 participants
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?
cancelBatch's build-cancellation step predates the speculation tree: it flipped a single Build entity keyed directly by batch.ID, with no build runner in the loop and no tree consulted. Builds are now found through the tree's per-path build mapping — a batch can hold more than one in-flight build across its paths — so batch-level cancellation has to work through the tree like every other stage.
What?
Cancellation becomes a convergent sweep through the tree and the existing stage seams. Each cancelBatch pass resolves every non-terminal path through the path->build mapping and either settles it to Cancelled (no build, dangling mapping, or terminal build) or captures intent (Cancelling) while its build is still in flight. While anything is pending the batch stays BatchStateCancelling and the pass publishes the queue to prioritize; only a pass that finds nothing pending CASes the batch terminal, fans out dependents, and publishes conclude. A terminal-transition guard re-checks Cancelled paths on would-be-final passes and pulls any that still hides a live build (a pre-build Cancel racing the build stage's trigger) back to Cancelling, so the batch never settles over a running build.
Three stages close the loop: prioritize loads Cancelling batches' trees for routing only (their paths are never candidates, and rogue decisions naming them are skipped as illegal), the build stage cancels every in-flight build of a Cancelling batch regardless of the path's recorded status and never triggers builds for it, and buildsignal keeps polling and re-publishing speculate for Cancelling batches so the wind-down is recorded and the sweep re-woken.
The branch keeps its review history as commits: the first commit cancels through the tree with a direct build-runner call in speculate (a documented exception to "only the build stage calls the runner"), the second removes that exception per review — decisions live in the tree, prioritize is the one channel from tree state to build messages, the build stage stays the sole runner caller — and the third fixes comment wording (build system, not CI; credit speculation-level cancels alongside the tree-sweep race).
Note for the stack: the cancel-flow CAS-race retry classification (preetam/int/cancel-retryable-cas) is not an ancestor of this branch; without it a benign version race on the terminal CAS still dead-letters, so that fix should land first. Metric renames: skipped_halted -> skipped_terminal (build, buildsignal), no_speculating_batches -> no_batches (prioritize).
Test Plan
go build ./...andgo vetclean;go test ./submitqueue/... ./service/... ./platform/...all passbazel test //submitqueue/... //service/...— 56/56make e2e-test— stovepipe and submitqueue suites passmake lint/make check-tidy/make check-gazellecleanStack