Skip to content

Commit 01b5a98

Browse files
committed
fix(speculate): wait for every assumption to settle before merging
## Summary ### Why? A head could merge on an assumption that had not come true. `mergeablePath` required every dependency a path assumed would *succeed* to have actually merged, but imposed no wait at all on one it assumed would *fail*. The doc comment stated the reasoning: *"A dependency assumed to fail imposes no wait: the path is broken the moment that dependency succeeds, so a still-live path has already been vindicated on it."* That holds only if the transition were instantaneous. It is not — a dependency spends time in `speculating`, and then in `merging`, having neither succeeded nor failed. `assumptionBroken` only fires on a terminal state, so throughout that window the path is neither broken nor vindicated. It is unsettled, and the gate read unsettled as permission. A path that assumed a dependency would fail was built *without* that dependency's changes. Landing the head while that dependency is still live, and watching it land too, puts a combination on the trunk that no build ever validated — the one thing the queue exists to prevent. It needs no textual conflict to break the trunk, because the two changes were never built together. Measured against the real predicates before fixing, with a passed `fails(D)` path and only D's state varying: | D's state | `mergeablePath` | correct? | | -- | -- | -- | | `speculating` | true | no | | `merging` | true | no | | `cancelling` | true | no | | `failed` | true | yes — the assumption came true | | `succeeded` | false | yes — `assumptionBroken` catches it | `decide` returned `merge` in all three of the wrong rows. ### What? The rule is now symmetric: a path may merge once every dependency it took a position on has finished the way it assumed. `succeeds` needs `Succeeded`; `fails` needs `Failed` or `Cancelled`; `ignored` is not a position, so it still imposes no wait and conflict relaxation is untouched. `allAssumedSucceedingMerged` becomes `allAssumptionsSettled`, since it no longer looks only at succeeding dependencies. Note this is not the "bypass large diff" early merge the RFC describes. That reads a passed path for *every* combination of the dependencies, and is not implemented on the controller side — nothing enumerates combinations. A single path betting the right way was never a sound approximation of it, and `speculation.md` now says so rather than describing behaviour the code does not have. One liveness consequence, recorded on CODEM-428 rather than fixed here: a dependency stuck in `Cancelling` now stalls its dependents too, not just itself. `finalizeCancellations` converges `Cancelling → Cancelled` on any subsequent run, so this only bites when no further run is triggered — which is that issue's edge-triggering gap. ## Test Plan - ✅ `make test` — 96/96 pass - ✅ `make lint`, `make check-gazelle`, `make check-tidy` `TestMergeablePath` gains the cases the old rule got wrong: a fails assumption waits out `speculating`, `merging` and `cancelling`, and merges on `Failed` or `Cancelled`. An assumed-succeeding dependency waits out its merge, and an ignored one still imposes no wait. ## Issue Fixes https://linear.app/uber/issue/CODEM-429
1 parent 9228d86 commit 01b5a98

3 files changed

Lines changed: 64 additions & 19 deletions

File tree

doc/rfc/submitqueue/speculation.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -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), *fails* (built without), or *ignored*. Once a path's build has passed and every dependency it assumes *succeeds* has merged, the speculate controller moves the head to Merging and hands it to Runway — it waits only on the dependencies it was built on top of, not the head's full dependency list. 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 the predecessors it assumes succeed, so a chain merges one at a time.
57+
- **Merge (strict).** Each path carries an assumption about every dependency — *succeeds* (built on top of), *fails* (built without), or *ignored*. Once a path's build has passed and every dependency it took a position on 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. It waits only on the dependencies it took a position on, never on the head's full dependency list: an *ignored* dependency is not a position, so its outcome gates nothing. A dependency that is merely *merging* has not finished — 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 the predecessors it took a position on, so a chain merges one at a time.
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

@@ -72,6 +72,8 @@ If a batch's passed builds cover *every* way its dependencies could resolve, the
7272

7373
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.
7474

75+
**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.
76+
7577
### Cancellation
7678

7779
Cancellation is best-effort: a batch marked *cancelling* may still merge if a merge wins the race, so terminal states prevail. A cancel sets the intent; a later run drives it terminal.

submitqueue/orchestrator/controller/speculate/outcome.go

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -63,17 +63,22 @@ func decide(head entity.Batch, set entity.SpeculationPathSet, snap snapshot) out
6363
return outcomeWait
6464
}
6565

66-
// mergeablePath returns a passed path whose merge preconditions are met:
67-
// every dependency it assumed would succeed has actually merged.
66+
// mergeablePath returns a passed path whose merge preconditions are met: every
67+
// guess it made about a dependency has been borne out by that dependency's
68+
// actual state.
6869
//
6970
// This is what makes speculation pay. The head waits only on the dependencies
70-
// the passed build was stacked on — not on its full dependency list — so a
71-
// batch built without a slow neighbour, or with that neighbour relaxed to
72-
// ignored, merges as soon as the ones it actually built on have landed.
71+
// its passed build actually took a position on — so a batch built with a slow
72+
// neighbour relaxed to ignored merges without waiting for that neighbour at
73+
// all.
7374
//
74-
// A dependency assumed to fail imposes no wait: the path is broken the
75-
// moment that dependency succeeds, so a still-live path has already been
76-
// vindicated on it. An ignored dependency imposes no wait by definition.
75+
// A guess that has not been settled yet is not a licence to merge, whichever
76+
// way it points. A path that assumed a dependency would fail was built without
77+
// that dependency's changes, so landing it while the dependency is still live
78+
// puts a combination on the trunk that no build ever validated — which is the
79+
// one thing the queue exists to prevent. The dependency merging is not enough
80+
// either: a merge can fail, so "on its way in" is still an open question, and
81+
// the head waits for the answer.
7782
func mergeablePath(set entity.SpeculationPathSet, snap snapshot) (entity.SpeculationPathEntry, bool) {
7883
for _, entry := range set.Paths {
7984
if entry.Status != entity.SpeculationPathStatusPassed {
@@ -82,22 +87,30 @@ func mergeablePath(set entity.SpeculationPathSet, snap snapshot) (entity.Specula
8287
if assumptionBroken(entry.Path, snap) {
8388
continue
8489
}
85-
if allAssumedSucceedingMerged(entry.Path, snap) {
90+
if allAssumptionsSettled(entry.Path, snap) {
8691
return entry, true
8792
}
8893
}
8994
return entity.SpeculationPathEntry{}, false
9095
}
9196

92-
// allAssumedSucceedingMerged reports whether every dependency the path
93-
// assumed would succeed has reached Succeeded.
94-
func allAssumedSucceedingMerged(path entity.SpeculationPath, snap snapshot) bool {
97+
// allAssumptionsSettled reports whether every dependency the path took a
98+
// position on has finished the way the path assumed: one it assumed would
99+
// succeed has reached Succeeded, and one it assumed would fail has finished
100+
// some other way. An ignored dependency is not a position — the path made no
101+
// claim about it — so it never imposes a wait.
102+
func allAssumptionsSettled(path entity.SpeculationPath, snap snapshot) bool {
95103
for _, dep := range path.Dependencies {
96-
if dep.Assumption != entity.DependencyAssumptionSucceeds {
97-
continue
98-
}
99-
if snap.batchState(dep.Batch) != entity.BatchStateSucceeded {
100-
return false
104+
state := snap.batchState(dep.Batch)
105+
switch dep.Assumption {
106+
case entity.DependencyAssumptionSucceeds:
107+
if state != entity.BatchStateSucceeded {
108+
return false
109+
}
110+
case entity.DependencyAssumptionFails:
111+
if state != entity.BatchStateFailed && state != entity.BatchStateCancelled {
112+
return false
113+
}
101114
}
102115
}
103116
return true

submitqueue/orchestrator/controller/speculate/outcome_test.go

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,33 @@ func TestMergeablePath(t *testing.T) {
6060
want: true,
6161
},
6262
{
63-
name: "an assumed-failing dependency imposes no wait",
63+
name: "waits for an assumed-failing dependency to actually fail",
6464
assumption: [2]entity.DependencyAssumption{fails, ignored},
6565
dep1State: entity.BatchStateSpeculating,
66+
want: false,
67+
},
68+
{
69+
name: "still waits while that dependency is merging",
70+
assumption: [2]entity.DependencyAssumption{fails, ignored},
71+
dep1State: entity.BatchStateMerging,
72+
want: false,
73+
},
74+
{
75+
name: "still waits while that dependency is cancelling",
76+
assumption: [2]entity.DependencyAssumption{fails, ignored},
77+
dep1State: entity.BatchStateCancelling,
78+
want: false,
79+
},
80+
{
81+
name: "merges once it has failed",
82+
assumption: [2]entity.DependencyAssumption{fails, ignored},
83+
dep1State: entity.BatchStateFailed,
84+
want: true,
85+
},
86+
{
87+
name: "merges once it has been cancelled",
88+
assumption: [2]entity.DependencyAssumption{fails, ignored},
89+
dep1State: entity.BatchStateCancelled,
6690
want: true,
6791
},
6892
{
@@ -71,6 +95,12 @@ func TestMergeablePath(t *testing.T) {
7195
dep1State: entity.BatchStateSpeculating,
7296
want: true,
7397
},
98+
{
99+
name: "an assumed-succeeding dependency waits out its merge",
100+
assumption: [2]entity.DependencyAssumption{succeeds, ignored},
101+
dep1State: entity.BatchStateMerging,
102+
want: false,
103+
},
74104
{
75105
name: "one unmerged dependency is enough to wait",
76106
assumption: [2]entity.DependencyAssumption{succeeds, succeeds},

0 commit comments

Comments
 (0)