feat(speculation): route builds through the tree via prioritize#365
feat(speculation): route builds through the tree via prioritize#365behinddwalls wants to merge 1 commit into
Conversation
behinddwalls
force-pushed
the
preetam/int/tree-driven-builds
branch
from
966e46a to
2334a19
Compare
The speculate controller previously drove a batch's own build directly (publish to build, CAS to Speculating) while the speculation tree it maintained was pure shadow-mode bookkeeping nobody read. To make the tree the actual source of truth for what gets built, the pipeline needs to flip: speculate hands off to prioritize, and prioritize's admission decisions (Prioritized paths) are what the build stage acts on. This also completes the ownership rule this stack is building toward: the build stage becomes the sole owner of runner interaction for path-level work. Other stages (prioritize's preemptive eviction, speculate's reconcile for dead paths, both landing in a later commit) only ever record a cancel decision as intent (SpeculationPathStatusCancelling) on the tree; nothing but the build stage calls BuildRunner.Cancel. speculate.go: speculateBatch no longer publishes to build. It CASes Created/Scored batches to Speculating, then publishes the queue to prioritize on every pass (after the CAS, so a concurrent prioritize round never observes a batch as not-yet-Speculating with no later wake-up). The legacy tryFinalize step is otherwise unchanged and still only runs once a batch has reached Speculating on this or a prior pass, so merge timing is unchanged for now — tightening it to gate on the tree's own path outcome is deferred to a later commit in this stack. build.go is replaced with its tree-driven form: it loads the batch's speculation tree and, per path, either triggers a build for a Prioritized path with no build yet, or enacts a persisted Cancelling intent. Trigger dedup is on the path->build mapping (SpeculationPathBuildStore), checked before Trigger runs since it is readable up front unlike a Build row. The write order is Trigger -> Build.Create -> mapping.Create -> publish to buildsignal; a crash between Trigger and the mapping write is recovered by redelivery re-running the same path, and a lost mapping-create race defers to the winner (republishing buildsignal for its build) rather than erroring. A Prioritized path whose mapping already resolves to a non-terminal build republishes buildsignal to close the crash window between the original Create and its original publish; a terminal build is a no-op, since reconcile is what will fold that outcome into the path's status. The new Cancelling arm resolves the path's build via the same mapping, calls runner.Cancel if the build is not already terminal, and republishes buildsignal so the poll loop reacts promptly. Every lookup miss (no mapping yet, or a mapping pointing at a missing build row) is tolerated as a no-op rather than an error, leaving the intent for a later reconcile pass to settle. A missing speculation tree, by contrast, is an invariant violation — the tree is created before the batch is ever published to prioritize and is never deleted — so it surfaces as a plain error and dead-letters, failing the batch loudly instead of retrying against broken state. buildsignal.go picks up its RFC-final wording (build's ID doubling as the runner handle) plus a corrected error-classification doc paragraph — classification is per error cause via the wired classifier walk, not the per-call-site policy the old text described — with no behavior change; the test file is byte-identical to before. ✅ `go build ./submitqueue/... ./service/...` ✅ `go vet ./submitqueue/... ./service/...` ✅ `bazel test //submitqueue/... //service/...` — 56/56 targets pass, including new build.go tests for the Cancelling arm (non-terminal enacts + republishes, terminal no-ops, no-mapping/dangling-mapping skip, runner error surfaces with no silent ack). ✅ `make gazelle` / `make fmt` — no diffs beyond the intended source changes. ✅ `make e2e-test` — 2/2 targets pass (stovepipe, submitqueue), confirming parity: prioritize still admits the single chain path and exactly one build runs per batch.
behinddwalls
force-pushed
the
preetam/int/tree-driven-builds
branch
from
2334a19 to
6208e14
Compare
behinddwalls
force-pushed
the
preetam/int/speculate-tree
branch
from
e30249e to
527af81
Compare
| // reached Speculating on a prior pass — merge timing is unchanged from | ||
| // before prioritize/build existed. This tightens in a later commit to | ||
| // gate on the tree's own path outcome instead of dependency states alone. | ||
| if originalState == entity.BatchStateSpeculating { |
There was a problem hiding this comment.
High risk in this PR slice: redelivery can finalize before any build starts. Suppose the first pass persists Speculating and then crashes or errors while publishing prioritize. Redelivery starts with originalState == Speculating, republishes prioritize, and immediately enters tryFinalize. A dependency-free batch can move to Merging before prioritize starts a build; prioritize then skips it because it is no longer Speculating. This is fixed by PR #371, which requires a passed speculation path before finalization.
| return fmt.Errorf("failed to serialize queue ID: %w", err) | ||
| } | ||
|
|
||
| msg := entityqueue.NewMessage(queue, payload, queue, nil) |
There was a problem hiding this comment.
High risk: distinct prioritize rounds share one persistent dedup key. Every wake for queue q uses (topic=prioritize, partition=q, id=q). If an older consumed row has not been garbage-collected, a later batch can publish successfully here without creating a delivery. Its selected paths may then never be reconsidered for capacity. Use a logical event ID that is stable for retrying one round but distinct for later rounds, such as source batch plus version. This remains present through PR #380.
| Version: 1, | ||
| CreatedAt: time.Now().UnixMilli(), | ||
| } | ||
| if err := c.store.GetSpeculationPathBuildStore().Create(ctx, mapping); err != nil { |
There was a problem hiding this comment.
High risk: the mapping race deliberately abandons a live external build. Two deliveries can both observe no mapping and both call runner.Trigger before reaching this create. Only one mapping wins. The loser has already started an external build, but that build is not mapped to the path, polled, cancelled, or counted by the queue-wide budget. This can leak CI work and exceed configured capacity. Persist a dispatch reservation before the external call and pass a stable idempotency key to the runner; cancelling a known race loser is useful defense in depth. This remains present through PR #380.
| return fmt.Errorf("failed to get build %s for path %s of batch %s: %w", pb.BuildID, p.ID, batch.ID, err) | ||
| } | ||
|
|
||
| if b.Status.IsTerminal() { |
There was a problem hiding this comment.
High risk if PR #365 is merged independently: completed builds retain capacity. This branch assumes a later reconcile pass will fold the terminal build into SpeculationPathInfo, but this PR has no production writer that performs that reconciliation. The path can remain Prioritized or Building, so sticky continues counting its slot indefinitely and new work can starve. This structural omission is fixed by PR #371. The later stack still depends on buildsignal observing and persisting the terminal runner status, so the inherited polling dedup issue can still delay that observation.
2 participants
Summary
Why?
The speculate controller previously drove a batch's own build directly (publish to build, CAS to Speculating) while the speculation tree it maintained was pure shadow-mode bookkeeping nobody read. To make the tree the actual source of truth for what gets built, the pipeline needs to flip: speculate hands off to prioritize, and prioritize's admission decisions (Prioritized paths) are what the build stage acts on.
This also completes the ownership rule this stack is building toward: the build stage becomes the sole owner of runner interaction for path-level work. Other stages (prioritize's preemptive eviction, speculate's reconcile for dead paths, both landing in a later commit) only ever record a cancel decision as intent (SpeculationPathStatusCancelling) on the tree; nothing but the build stage calls BuildRunner.Cancel.
What?
speculate.go: speculateBatch no longer publishes to build. It CASes Created/Scored batches to Speculating, then publishes the queue to prioritize on every pass (after the CAS, so a concurrent prioritize round never observes a batch as not-yet-Speculating with no later wake-up). The legacy tryFinalize step is otherwise unchanged and still only runs once a batch has reached Speculating on this or a prior pass, so merge timing is unchanged for now — tightening it to gate on the tree's own path outcome is deferred to a later commit in this stack.
build.go is replaced with its tree-driven form: it loads the batch's speculation tree and, per path, either triggers a build for a Prioritized path with no build yet, or enacts a persisted Cancelling intent. Trigger dedup is on the path->build mapping (SpeculationPathBuildStore), checked before Trigger runs since it is readable up front unlike a Build row. The write order is Trigger -> Build.Create -> mapping.Create -> publish to buildsignal; a crash between Trigger and the mapping write is recovered by redelivery re-running the same path, and a lost mapping-create race defers to the winner (republishing buildsignal for its build) rather than erroring. A Prioritized path whose mapping already resolves to a non-terminal build republishes buildsignal to close the crash window between the original Create and its original publish; a terminal build is a no-op, since reconcile is what will fold that outcome into the path's status.
The new Cancelling arm resolves the path's build via the same mapping, calls runner.Cancel if the build is not already terminal, and republishes buildsignal so the poll loop reacts promptly. Every lookup miss (no mapping yet, or a mapping pointing at a missing build row) is tolerated as a no-op rather than an error, leaving the intent for a later reconcile pass to settle. A missing speculation tree, by contrast, is an invariant violation — the tree is created before the batch is ever published to prioritize and is never deleted — so it surfaces as a plain error and dead-letters, failing the batch loudly instead of retrying against broken state.
buildsignal.go picks up its RFC-final wording (build's ID doubling as the runner handle) plus a corrected error-classification doc paragraph — classification is per error cause via the wired classifier walk, not the per-call-site policy the old text described — with no behavior change; the test file is byte-identical to before.
Test Plan
✅
go build ./submitqueue/... ./service/...✅
go vet ./submitqueue/... ./service/...✅
bazel test //submitqueue/... //service/...— 56/56 targets pass, including new build.go tests for the Cancelling arm (non-terminal enacts + republishes, terminal no-ops, no-mapping/dangling-mapping skip, runner error surfaces with no silent ack).✅
make gazelle/make fmt— no diffs beyond the intended source changes.✅
make e2e-test— 2/2 targets pass (stovepipe, submitqueue), confirming parity: prioritize still admits the single chain path and exactly one build runs per batch.Stack