Skip to content

Commit 658986d

Browse files
committed
feat(speculate): bypass unsettled deps on full passed coverage
## Summary ### Why? A batch's merge used to wait for every dependency to resolve even when its passed builds already covered every way those dependencies could turn out. That made a small change sit behind a slow one for no reason: both the with- and without-dependency trees were validated, so whichever future arrives is already green. ### What? Adds bypassablePath to the speculate controller: decide() merges a head once passed, unbroken, well-formed paths cover every 2^n combination of its unsettled dependencies. Settled dependencies still pin each path to reality through assumptionBroken. Finalize prefers a strict mergeable path and falls back to bypass coverage, superseding siblings either way. Tightened isWellFormed to require dependencies in the head's canonical order, and read the coverage signature positionally off the slice (dropping the per-path map). Order is load-bearing: Base() projects the path positionally, so a permuted path is a different stack, and a reordered path can no longer masquerade as coverage for the canonical combination. The NoBypassWhenCoverageIsIncomplete e2e test asserts ordering (follower lands only after the lead) rather than the racy intermediate speculating state: waiting is a recorded history event, and once the ungated lead lands the seeded path is strictly mergeable — the old assertion raced the queue and flaked on fast runners. ## Test Plan - ./tool/bazel test //submitqueue/orchestrator/controller/speculate:go_default_test - ./tool/bazel test //submitqueue/extension/speculation/... //submitqueue/entity:go_default_test //submitqueue/orchestrator/controller/build:go_default_test - New TestBypassablePath "does not count a reordered path" and TestIsWellFormed "dependencies out of queue order" fail before, pass after. - Four bypass E2E tests pass (BypassedHeadLandsFirst, BypassesMergingDependency, NoBypassWhenCoverageIsIncomplete, BypassesAnUnresolvedDependency); full e2e suite green twice locally.
1 parent 8dc4fbb commit 658986d

16 files changed

Lines changed: 460 additions & 93 deletions

File tree

‎doc/rfc/submitqueue/speculation-generator-best-first.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,7 +447,7 @@ The ordering stays the same. `CandidatePath.RankingScore` contains this logarith
447447
- `Succeeded` fixes an assumption to succeeds.
448448
- `Failed` or `Cancelled` fixes an assumption to fails.
449449
- `Cancelling` remains undecided because cancellation may lose a race with completion.
450-
- `Merging` also remains undecided, because a merge can fail. It is tempting to treat it as committed to landing and skip the scorer call, but that puts a state-specific policy inside the search: whether a path betting against a merging batch is worth funding is a question of price, and price belongs to the scorer. The allocator draws the same line — "no batch state enters this decision" — and the generator holds it too. Nothing is lost by staying open, because a head can never merge ahead of a dependency it took a position on (see [speculation.md](speculation.md)); the cost of an unlikely path is budget, which is the allocator's to ration.
450+
- `Merging` also remains undecided, because a merge can fail. It is tempting to treat it as committed to landing and skip the scorer call, but that puts a state-specific policy inside the search: whether a path betting against a merging batch is worth funding is a question of price, and price belongs to the scorer. The allocator draws the same line — "no batch state enters this decision" — and the generator holds it too. Nothing is lost by staying open: a single passed path still waits for the merge result, while passed paths covering every outcome let the controller bypass the dependency (see [speculation.md](speculation.md)). Funding the unlikely side spends budget, which is the allocator's to ration.
451451
- A fixed assumption stays in the returned path but contributes probability 1 and has no flip.
452452
- A shared dependency is scored once per run.
453453

‎doc/rfc/submitqueue/speculation.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ A merge queue that verifies one change at a time is limited by its slowest build
44

55
Work enters SubmitQueue as **batches** — changes verified and merged together. Two batches **conflict** when they touch the same code, which makes the earlier one a **dependency** of the later. A **path** is one set of assumptions about how a batch's dependencies resolve, and the batch it builds is the path's **head**.
66

7-
On every queue update the **speculate controller** reruns from scratch: it reads the current state, applies the incoming signals, asks a pluggable **Speculator** which paths are worth building within the CI budget, and persists only those. Everything else is recomputed next time, never stored. Merging stays strict: a batch merges only after its dependencies resolve and a matching build has passed.
7+
On every queue update the **speculate controller** reruns from scratch: it reads the current state, applies the incoming signals, asks a pluggable **Speculator** which paths are worth building within the CI budget, and persists only those. Everything else is recomputed next time, never stored. A batch normally merges after its dependencies resolve and a matching build has passed; complete passed coverage of every unresolved outcome lets it bypass those dependencies.
88

99
## The speculation run
1010

@@ -54,7 +54,7 @@ Every write is a compare-and-swap: a writer that loses re-reads on a later run.
5454

5555
Verdicts are controller-owned facts: the Speculator can neither compute nor veto them.
5656

57-
- **Merge (strict).** Each path carries an assumption about every dependency — *succeeds* (built on top of) or *fails* (built without). Once a path's build has passed and every dependency has finished the way the path assumed — one assumed *succeeds* has merged, one assumed *fails* has failed or been cancelled — the speculate controller moves the head to Merging and hands it to Runway. A dependency that is merely *merging* has not finished, because a merge can fail, so it is still waited on. If that hand-off is lost, the next run re-sends it. The same run sets the head's remaining in-flight paths *cancelling*: once one path has passed the others cannot help, and they hold CI slots until they stop. The mergesignal controller records Runway's terminal result: success marks the head Succeeded, while failure marks it Failed. The result publishes a single dirty signal — no per-dependent fan-out — and the next run refutes paths whose assumption disagrees with the result: *fails* assumptions after success, *succeeds* assumptions after failure. The hand-off is idempotent, so Runway reports success without another merge when the change is already present. Down a chain, each head waits for its predecessors to settle, so a chain merges one at a time.
57+
- **Merge.** Each path carries an assumption about every dependency — *succeeds* (built on top of) or *fails* (built without). Normally, once a path's build has passed and every dependency has finished the way the path assumed — one assumed *succeeds* has merged, one assumed *fails* has failed or been cancelled — the speculate controller moves the head to Merging and hands it to Runway. A dependency that is merely *merging* has not finished, because a merge can fail, so a single matching path still waits for the answer. Complete passed coverage is the exception described in Bypass large diff: it lets a head merge before those answers arrive. If the hand-off is lost, the next run re-sends it. The same run sets the head's remaining in-flight paths *cancelling*: once the head can merge they cannot help, and they hold CI slots until they stop. The mergesignal controller records Runway's terminal result: success marks the head Succeeded, while failure marks it Failed. The result publishes a single dirty signal — no per-dependent fan-out — and the next run refutes paths whose assumption disagrees with the result: *fails* assumptions after success, *succeeds* assumptions after failure. The hand-off is idempotent, so Runway reports success without another merge when the change is already present. A chain ordinarily merges one at a time, but a fully covered head can bypass its unsettled predecessors.
5858
- **Failure (no viable path).** A batch fails when every possible future has a failed build — no path can pass, so it can never merge.
5959
- **Cancel.** A cancelled batch is driven terminal: its in-flight paths are set *cancelling*, then the batch is marked Cancelled once they stop (see Cancellation).
6060

@@ -74,9 +74,9 @@ Example of the payoff either way: `H` conflicts with `B1` and weak `B2`. Relax `
7474

7575
If a batch's passed builds cover *every* way its dependencies could resolve, the outcome is the same either way — so it can merge now, ahead of them. Classic case: a small change stuck behind a slow one is built both with and without it; both pass, and it merges immediately.
7676

77-
The default Speculator covers the whole space only when doing so is cheap enough, and funds the extra candidates within the build budget. The controller merges early only when a passed path exists for every combination of the dependencies — it reads that straight off the path records. If any combination is missing or unbuilt, the head waits normally.
77+
The controller checks coverage over only the dependencies that have not settled yet. Settled dependencies pin each surviving path to the outcome that actually happened; for every combination of the remaining dependencies, the path set must contain a passed, unbroken path with that combination of assumptions. If any combination is missing, unbuilt, failed, or contradicted by a settled dependency, the head waits normally. The check only observes paths the Speculator already funded — it does not fund the exponential path space itself or alter the queue's build budget.
7878

79-
**Not yet implemented on the controller side.** `decide`/`mergeablePath` gate on a single passed path whose assumptions have all been settled by the dependency's actual state; nothing enumerates the combinations. The distinction matters: a *single* passed path that assumed a dependency would fail is not complete coverage, and merging on it while that dependency is still live would put a combination on the trunk that no build validated. Coverage is what makes early merge sound — one path betting the right way is not.
79+
Coverage makes the bypass sound because whichever way the dependencies later resolve, a passed build already validated the resulting set of changes. The build order and merge order differ: a path assuming dependency `D` succeeds validates `D` then head `H`, while bypass lands `H` before `D`. SubmitQueue treats those orders as content-equivalent. Runway still performs the real merge, so if the reordered changes conflict textually, the older dependency can fail after the newer head has bypassed it; this is an accepted cost of landing the fully covered head early rather than a licence to put unmergeable content on the target.
8080

8181
### Cancellation
8282

‎submitqueue/entity/request_log.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,11 @@ const (
5858
RequestStatusBatched RequestStatus = "batched"
5959

6060
// RequestStatusSpeculating indicates that the batch containing the request is in speculation:
61-
// planning, building, or waiting for its dependencies to settle. None of those leaves it able to land.
61+
// planning, building, or waiting until either its dependencies settle or passed paths cover every possible outcome.
6262
RequestStatusSpeculating RequestStatus = "speculating"
6363

6464
// RequestStatusSpeculated indicates that the batch containing the request has finished speculating:
65-
// a build passed on a path whose assumptions all held, and the batch has been cleared to merge.
65+
// either a passed path's assumptions all held, or passed paths cover every outcome of its unsettled dependencies.
6666
RequestStatusSpeculated RequestStatus = "speculated"
6767

6868
// RequestStatusLanding indicates that the request is actively being landed (e.g., source control operation is in progress to push the change to the target branch).

‎submitqueue/extension/speculation/speculator/standard/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ The `standard` `Speculator` funds the queue's most promising speculation paths f
44

55
Each run it considers candidate paths in descending order of their probability of being the future that actually happens, and proposes builds down that ranking. Paths already pending or building keep the slot they hold rather than restarting; paths whose builds already finished are skipped for as long as their records remain in the supplied path sets, so a finished path can be proposed again — for a retry, say — once retention drops it; new builds fill whatever budget remains.
66

7-
When the budget runs out, everything below the cut waits for a later run. That is safe because a batch's verdict never depends on what was funded — only on how its dependencies resolve and which builds pass.
7+
When the budget runs out, everything below the cut waits for a later run. That is safe because the propose-side cannot invent a batch verdict: the speculate controller still decides merge from the persisted paths, including complete coverage of unsettled dependencies.
88

99
Both halves are swappable. The ranking is the `Generator`'s: the default `bestfirst` scores each path by the probability that all its assumptions hold. The budget policy is the `Allocator`'s: the default `sticky` fills only free slots and never preempts, where a preempting allocator would cancel a low-value in-flight path to fund a better one.
1010

‎submitqueue/orchestrator/controller/speculate/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test")
33
go_library(
44
name = "go_default_library",
55
srcs = [
6+
"bypass.go",
67
"check.go",
78
"dispatch.go",
89
"doc.go",
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package speculate
16+
17+
import "github.com/uber/submitqueue/submitqueue/entity"
18+
19+
// bypassablePath returns a passed path once passed builds cover every possible
20+
// outcome of the head's unsettled dependencies. Settled dependencies stay
21+
// pinned to reality through assumptionBroken.
22+
//
23+
// A path's combination is read positionally — the i-th assumption belongs to
24+
// the head's i-th dependency — which isWellFormed's order check is what
25+
// licenses: two paths with the same assumptions in different orders are
26+
// different stacks, not the same outcome.
27+
func bypassablePath(head entity.Batch, set entity.SpeculationPathSet, snap snapshot) (entity.SpeculationPathEntry, bool) {
28+
unsettled := unsettledDependencyIndices(head, snap)
29+
if len(unsettled) == 0 {
30+
return entity.SpeculationPathEntry{}, false
31+
}
32+
33+
required := 1
34+
for range unsettled {
35+
if required > len(set.Paths)/2 {
36+
return entity.SpeculationPathEntry{}, false
37+
}
38+
required *= 2
39+
}
40+
41+
seen := make(map[string]struct{}, required)
42+
var winner entity.SpeculationPathEntry
43+
for _, entry := range set.Paths {
44+
if entry.Status != entity.SpeculationPathStatusPassed ||
45+
assumptionBroken(entry.Path, snap) ||
46+
!isWellFormed(entry.Path, head) {
47+
continue
48+
}
49+
50+
signature := make([]byte, len(unsettled))
51+
for i, depIndex := range unsettled {
52+
if entry.Path.Dependencies[depIndex].Assumption == entity.DependencyAssumptionFails {
53+
signature[i] = 'f'
54+
} else {
55+
signature[i] = 's'
56+
}
57+
}
58+
key := string(signature)
59+
if _, exists := seen[key]; exists {
60+
continue
61+
}
62+
seen[key] = struct{}{}
63+
if len(seen) == 1 {
64+
winner = entry
65+
}
66+
}
67+
68+
return winner, len(seen) == required
69+
}
70+
71+
func unsettledDependencyIndices(head entity.Batch, snap snapshot) []int {
72+
var indices []int
73+
for i, depID := range head.Dependencies {
74+
if !snap.batchState(depID).IsTerminal() {
75+
indices = append(indices, i)
76+
}
77+
}
78+
return indices
79+
}

‎submitqueue/orchestrator/controller/speculate/check.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ const (
2929
rejectHeadNotSpeculating rejection = "head_not_speculating"
3030
// rejectMalformedPath is a path whose assumptions do not line up with its
3131
// head's dependency list: one missing or extra, a duplicate, a wrong head,
32-
// or a made-up assumption value.
32+
// an assumption out of queue order, or a made-up assumption value.
3333
rejectMalformedPath rejection = "malformed_path"
3434
// rejectBrokenAssumption is a path with an assumption a finished
3535
// dependency has already proven wrong.
@@ -122,7 +122,13 @@ func rejectionReason(proposal entity.Speculation, snap snapshot) (rejection, boo
122122

123123
// isWellFormed reports whether a path is a proper guess about its head:
124124
// exactly one assumption for each of the head's dependencies, no more and no
125-
// fewer, and every assumption a real value.
125+
// fewer, in the head's dependency order, and every assumption a real value.
126+
//
127+
// Order is load-bearing: Base() projects the path positionally, so a path
128+
// whose dependencies are permuted describes a stack the build runner applied
129+
// in a different order — a different tree, which no verdict may count as the
130+
// combination its assumptions name. With the length check and position-wise
131+
// equality, a missing, extra, or duplicate dependency is also impossible.
126132
//
127133
// A malformed path is not merely suboptimal, it is unmergeable — the merge
128134
// preconditions are read off the path's assumptions (see mergeablePath), so a
@@ -135,16 +141,10 @@ func isWellFormed(path entity.SpeculationPath, head entity.Batch) bool {
135141
return false
136142
}
137143

138-
required := make(map[string]struct{}, len(head.Dependencies))
139-
for _, dep := range head.Dependencies {
140-
required[dep] = struct{}{}
141-
}
142-
143-
for _, dep := range path.Dependencies {
144-
if _, ok := required[dep.Batch]; !ok {
144+
for i, dep := range path.Dependencies {
145+
if dep.Batch != head.Dependencies[i] {
145146
return false
146147
}
147-
delete(required, dep.Batch)
148148

149149
switch dep.Assumption {
150150
case entity.DependencyAssumptionSucceeds,
@@ -154,7 +154,7 @@ func isWellFormed(path entity.SpeculationPath, head entity.Batch) bool {
154154
}
155155
}
156156

157-
return len(required) == 0
157+
return true
158158
}
159159

160160
// findPath returns the entry for a path ID in the set.

‎submitqueue/orchestrator/controller/speculate/check_test.go‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,18 @@ func TestFilterProposals_Rejects(t *testing.T) {
102102
snap: checkSnapshot(entity.BatchStateSpeculating),
103103
want: rejectMalformedPath,
104104
},
105+
{
106+
name: "path with dependencies out of queue order",
107+
proposal: entity.Speculation{
108+
Path: entity.SpeculationPath{Head: head, Dependencies: []entity.PathDependency{
109+
{Batch: dep2, Assumption: entity.DependencyAssumptionFails},
110+
{Batch: dep1, Assumption: entity.DependencyAssumptionSucceeds},
111+
}},
112+
Action: entity.PathActionBuild,
113+
},
114+
snap: checkSnapshot(entity.BatchStateSpeculating),
115+
want: rejectMalformedPath,
116+
},
105117
{
106118
name: "cancel on a path that is not stored",
107119
proposal: entity.Speculation{Path: valid, Action: entity.PathActionCancel},
@@ -208,12 +220,12 @@ func TestIsWellFormed(t *testing.T) {
208220
want: true,
209221
},
210222
{
211-
name: "order does not matter",
223+
name: "dependencies out of queue order",
212224
path: entity.SpeculationPath{Head: head, Dependencies: []entity.PathDependency{
213225
{Batch: dep2, Assumption: entity.DependencyAssumptionSucceeds},
214226
{Batch: dep1, Assumption: entity.DependencyAssumptionFails},
215227
}},
216-
want: true,
228+
want: false,
217229
},
218230
{
219231
name: "missing a dependency",

‎submitqueue/orchestrator/controller/speculate/doc.go‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@
2424
// speculation everything is serial: C waits for B, B waits for A. Speculation
2525
// builds a batch against a guess about how its dependencies turn out. When
2626
// the guess holds, the batch merges the moment the guessed-on dependencies
27-
// land — it never waits for a build of its own to start afterwards.
27+
// land. If passed paths cover every possible outcome, the batch can merge
28+
// before those dependencies settle.
2829
//
2930
// # Paths
3031
//
@@ -46,6 +47,8 @@
4647
//
4748
// Fund both and every future is covered:
4849
//
50+
// - While A is still unresolved, both P1 and P2 passing lets B bypass A and
51+
// merge immediately: either possible future has already been validated.
4952
// - A succeeds and P1 passed: B merges the moment A lands. P2's guess
5053
// ("A fails") is broken — it can no longer come true — so its build is
5154
// cancelled to free the slot.
@@ -75,9 +78,8 @@
7578
// building, and every pending, building, and cancelling path holds its slot
7679
// until its build stops. A path is broken once a dependency's actual result
7780
// proves one of its assumptions wrong: its guess can no longer come true, so
78-
// its build is cancelled to free the slot. A path is superseded when a
79-
// sibling path of the same head passes — that sibling will carry the head out
80-
// of the queue, so the others are cancelled too.
81+
// its build is cancelled to free the slot. A path is superseded when its head
82+
// becomes mergeable, so any still-running siblings are cancelled too.
8183
//
8284
// Cancelling is intent, not fact: the build keeps its slot until CI actually
8385
// stops it, and only an observation of that stop (or proof nothing was ever
@@ -89,8 +91,8 @@
8991
//
9092
// # The life of a batch, as seen from here
9193
//
92-
// Created ──admit──► Speculating ──┬── merge ──► Merging (merge stage takes over)
93-
// └── fail ───► Failed
94+
// Created ──admit──► Speculating ──┬── merge or bypass ──► Merging
95+
// └── fail ─────────────► Failed
9496
// user cancel (cancel stage):
9597
// ... ──► Cancelling ── every path stopped ──► Cancelled
9698
//

0 commit comments

Comments
 (0)