From 62a6759d513d5a0647f85dfc6a45ebb3aed96145 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 10 Aug 2026 11:23:44 -0700 Subject: [PATCH] refactor(speculation)!: drop the ignored dependency assumption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? `DependencyAssumptionIgnored` was vocabulary the system could not produce. It expressed conflict relaxation — "this path makes no claim about this dependency" — but nothing ever set it: `bestfirst` only ever writes *succeeds* or *fails*, and no other Speculator exists. Outside the entity definition its only non-test appearance was `isWellFormed` accepting it as a legal value. Dead vocabulary is not free. Every consumer that reads an assumption had to decide what a third value meant, and each one answered by omission — `assumptionBroken`, `allAssumedSucceedingMerged` and `Base` all silently skip it, in three separate switches with no case tying them together. That is the kind of thing that stays correct only as long as nobody adds a fourth reader. It also cost real reasoning: while reviewing the CODEM-424 fix, `ignored` was the state that made a merged head's stored path impossible for the generator to regenerate, and working out whether that was reachable took a probe to answer, for a value nothing emits. ### What? The constant is gone, along with `isWellFormed`'s acceptance of it and every doc comment describing it. An assumption is now total: *succeeds* or *fails*, one per dependency. Test fixtures used `ignored` heavily as a "don't care" filler for the second dependency. On a dependency that has not finished, *fails* is indistinguishable from it — neither breaks the path nor gates the merge — so those became *fails*. The fixtures that could not be substituted mechanically are the ones where every dependency is resolved: exactly one assumption pair is unbroken there, so tests needing two live paths now share that shape and separate the entries by ID, which the existing helpers already supported. `speculation.md`'s relaxation section is rewritten rather than deleted, so the design intent survives the removal. It now records that relaxation is unimplemented, and that when it is built it belongs in the controller as a trim of the dependency list handed to the Speculator — keeping the decision beside the other correctness decisions, and keeping a path a total function over the dependencies it was given. It also records the question that design still owes an answer to: a path built against a trimmed list no longer lines up with a head whose list grew back, and `isWellFormed` rejects it, so the trim has to be stable for a head's lifetime or recorded with the path. **Breaking:** any persisted path carrying `ignored` fails `isWellFormed` after this and is dropped as malformed. Nothing writes the value, so no such record should exist. ## Test Plan - ✅ `make test` — 96/96 pass - ✅ `make lint`, `make check-gazelle`, `make check-tidy` The removal is covered by what the fixtures no longer need: `TestAssumptionBroken` and `TestMergeablePath` lose their ignored-specific rows, and `TestIsWellFormed` now rejects the value along with every other non-assumption. ## Issue Groundwork for https://linear.app/uber/issue/CODEM-424 and https://linear.app/uber/issue/CODEM-429, both of which had to reason about a value nothing produces. --- doc/rfc/submitqueue/speculation.md | 14 ++++-- submitqueue/entity/speculation.go | 9 ++-- .../generator/bestfirst/bestfirst_test.go | 3 +- .../orchestrator/controller/build/build.go | 4 +- .../controller/build/build_test.go | 6 +-- .../controller/speculate/check.go | 3 +- .../controller/speculate/check_test.go | 4 +- .../orchestrator/controller/speculate/doc.go | 8 ++-- .../controller/speculate/outcome.go | 6 +-- .../controller/speculate/outcome_test.go | 48 +++++++++---------- .../controller/speculate/run_test.go | 12 ++--- .../controller/speculate/snapshot.go | 3 +- .../controller/speculate/snapshot_test.go | 7 +-- 13 files changed, 59 insertions(+), 68 deletions(-) diff --git a/doc/rfc/submitqueue/speculation.md b/doc/rfc/submitqueue/speculation.md index a0a744d9a..f80f1fdb4 100644 --- a/doc/rfc/submitqueue/speculation.md +++ b/doc/rfc/submitqueue/speculation.md @@ -54,17 +54,21 @@ Every write is a compare-and-swap: a writer that loses re-reads on a later run. Verdicts are controller-owned facts: the Speculator can neither compute nor veto them. -- **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. +- **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 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. - **Failure (no viable path).** A batch fails when every possible future has a failed build — no path can pass, so it can never merge. - **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). ### Conflict relaxation -Conflict analysis is conservative — it flags any *possible* conflict — so heads carry dependencies that rarely matter and over-serialize. Relaxation lets the Speculator **ignore** the weakest: the path marks that dependency *ignored*, and its outcome neither gates the merge nor refutes the path. Which to ignore is a per-run Speculator policy. +Conflict analysis is conservative — it flags any *possible* conflict — so heads carry dependencies that rarely matter and over-serialize. Relaxation is the intended answer: drop the weakest dependencies so a head does not wait on them. -That marking lives on the path, so the path stays self-describing: finalization needs no external relaxed set (relaxing is what shrinks the space of outcomes a head's paths range over). +**Not implemented.** An earlier design expressed it per path, with a third assumption value — *ignored* — meaning "this path makes no claim about this dependency". Nothing ever produced one, and the value has been removed rather than left as vocabulary the system could not create. -Example: `H` conflicts with `B1` and weak `B2`. Ignore `B2`, and `H` merges once `B1` merges and its build passes — even if `B2` later merges. Without it, `H` waits on both. +When relaxation is built, it belongs in the **controller**, as a trim of the dependency list before the snapshot is handed over: the Speculator then sees a head whose dependencies are exactly the ones that count, and a path stays a total function over them — one assumption per dependency, each *succeeds* or *fails*, with no third state to reason about. That keeps the decision where the other correctness decisions live, since dropping a dependency is a judgement about what may land untested, not about which candidate is most promising. It also keeps every consumer honest by construction: a merge gate, a refutation check, or a generator cannot forget to special-case a value that does not exist. + +The open question that design has to answer is what a stored path means once the trim changes between runs — a path built against a trimmed list no longer lines up with a head whose list has grown back, and `isWellFormed` rejects it. The per-path marker made that case self-describing; a trim does not, so the trim has to be either stable for a head's lifetime or recorded alongside the path. + +Example of the payoff either way: `H` conflicts with `B1` and weak `B2`. Relax `B2`, and `H` merges once `B1` merges and its build passes — even if `B2` later merges. Without it, `H` waits on both. ### Bypass large diff @@ -108,7 +112,7 @@ The default Speculator is composed from two swappable interfaces — a **Generat Signatures live in code and are not copied here, so they cannot drift. This section says what each contract is for and where to read it. -**Entities** — [`submitqueue/entity/speculation.go`](../../../submitqueue/entity/speculation.go). A `SpeculationPath` is a head batch plus one `PathDependency` per dependency in queue order, each carrying a `DependencyAssumption`: *succeeds*, *fails*, or *ignored*. A `SpeculationPathEntry` is the stored record of one chosen path, keyed by a hash of its content, plus its status and attempt number; it holds no build reference — the execution record has that, keyed by (path ID, attempt) — and no ranking score, which means nothing outside the run that produced it. A `SpeculationPathSet` is one head's chosen paths, live and recently finished, under a single version for compare-and-swap. Every logical path is self-describing, but a store may encode the common head and ordered dependency IDs once per set and keep each path's assumptions positionally — two bits per dependency, or a base-3 code that stays a small integer. +**Entities** — [`submitqueue/entity/speculation.go`](../../../submitqueue/entity/speculation.go). A `SpeculationPath` is a head batch plus one `PathDependency` per dependency in queue order, each carrying a `DependencyAssumption`: *succeeds* or *fails*. A `SpeculationPathEntry` is the stored record of one chosen path, keyed by a hash of its content, plus its status and attempt number; it holds no build reference — the execution record has that, keyed by (path ID, attempt) — and no ranking score, which means nothing outside the run that produced it. A `SpeculationPathSet` is one head's chosen paths, live and recently finished, under a single version for compare-and-swap. Every logical path is self-describing, but a store may encode the common head and ordered dependency IDs once per set and keep each path's assumptions positionally — one bit per dependency. **Speculator** — [`submitqueue/extension/speculation/speculator`](../../../submitqueue/extension/speculation/speculator/README.md). `Speculate` takes one queue snapshot (the batches and their path sets) and returns the build and cancel actions it proposes; a path it wants left alone has no entry. Actions must target Speculating heads. Verdicts stay controller-owned, so there is no merge or fail action. diff --git a/submitqueue/entity/speculation.go b/submitqueue/entity/speculation.go index bd0ce6399..1d199c902 100644 --- a/submitqueue/entity/speculation.go +++ b/submitqueue/entity/speculation.go @@ -34,9 +34,6 @@ const ( // whether it fails or is cancelled. The head is built without it, and the // path is refuted if it succeeds after all. DependencyAssumptionFails DependencyAssumption = "fails" - // DependencyAssumptionIgnored means the path makes no assumption about this - // dependency. Its outcome neither gates the merge nor refutes the path. - DependencyAssumptionIgnored DependencyAssumption = "ignored" ) // PathDependency is one dependency of a path's head, with what the path assumes @@ -84,9 +81,9 @@ func (p SpeculationPath) ID() string { // dependency order. // // It is a projection of the path rather than a decision about it — a -// dependency the path assumes will fail is by definition built without, and -// an ignored one is not built on either — so every caller that needs the base -// derives it here rather than re-reading the assumptions itself. +// dependency the path assumes will fail is by definition built without — so +// every caller that needs the base derives it here rather than re-reading the +// assumptions itself. func (p SpeculationPath) Base() []string { var deps []string for _, dep := range p.Dependencies { diff --git a/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go b/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go index 5cc324163..923d03a8a 100644 --- a/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go +++ b/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go @@ -895,14 +895,13 @@ func TestBestFirst_ReturnedPathsAreIndependent(t *testing.T) { for i := range first.Path.Dependencies { first.Path.Dependencies[i].Batch = "clobbered" - first.Path.Dependencies[i].Assumption = entity.DependencyAssumptionIgnored + first.Path.Dependencies[i].Assumption = entity.DependencyAssumption("clobbered") } rest := drainAll(t, iter) require.NotEmpty(t, rest) for _, c := range rest { assert.NotContains(t, assumptionKey(c.Path), "clobbered") - assert.NotContains(t, assumptionKey(c.Path), string(entity.DependencyAssumptionIgnored)) } assert.NotEqual(t, before, assumptionKey(first.Path), "the test mutated what it was handed") } diff --git a/submitqueue/orchestrator/controller/build/build.go b/submitqueue/orchestrator/controller/build/build.go index ce770ad7e..bc0686737 100644 --- a/submitqueue/orchestrator/controller/build/build.go +++ b/submitqueue/orchestrator/controller/build/build.go @@ -192,8 +192,8 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // // The base is the path's own — the dependencies it assumes will succeed, in its // order. This is the behavioral heart of speculation: dependencies the path -// assumes will fail, and ones it ignores, are absent from the base, which is -// what lets the head be verified before they resolve. +// assumes will fail are absent from the base, which is what lets the head be +// verified before they resolve. // // The write order is Trigger, then the Build record, then the link, then the // signal — each write makes the previous one reachable. The Build record gives diff --git a/submitqueue/orchestrator/controller/build/build_test.go b/submitqueue/orchestrator/controller/build/build_test.go index 5ade579d2..dcb9a69cb 100644 --- a/submitqueue/orchestrator/controller/build/build_test.go +++ b/submitqueue/orchestrator/controller/build/build_test.go @@ -74,15 +74,15 @@ func headBatch(state entity.BatchState) entity.Batch { } } -// pathEntry builds one path-set entry for the head, assuming depA succeeds, -// depB fails, and depC is ignored — so only depA belongs in the build base. +// pathEntry builds one path-set entry for the head, assuming depA succeeds +// while depB and depC fail — so only depA belongs in the build base. func pathEntry(status entity.SpeculationPathStatus, attempt int) entity.SpeculationPathEntry { path := entity.SpeculationPath{ Head: headID, Dependencies: []entity.PathDependency{ {Batch: depA, Assumption: entity.DependencyAssumptionSucceeds}, {Batch: depB, Assumption: entity.DependencyAssumptionFails}, - {Batch: depC, Assumption: entity.DependencyAssumptionIgnored}, + {Batch: depC, Assumption: entity.DependencyAssumptionFails}, }, } return entity.SpeculationPathEntry{ diff --git a/submitqueue/orchestrator/controller/speculate/check.go b/submitqueue/orchestrator/controller/speculate/check.go index eb140d37f..f82cd8a21 100644 --- a/submitqueue/orchestrator/controller/speculate/check.go +++ b/submitqueue/orchestrator/controller/speculate/check.go @@ -148,8 +148,7 @@ func isWellFormed(path entity.SpeculationPath, head entity.Batch) bool { switch dep.Assumption { case entity.DependencyAssumptionSucceeds, - entity.DependencyAssumptionFails, - entity.DependencyAssumptionIgnored: + entity.DependencyAssumptionFails: default: return false } diff --git a/submitqueue/orchestrator/controller/speculate/check_test.go b/submitqueue/orchestrator/controller/speculate/check_test.go index e092a3b69..b095f9274 100644 --- a/submitqueue/orchestrator/controller/speculate/check_test.go +++ b/submitqueue/orchestrator/controller/speculate/check_test.go @@ -144,7 +144,7 @@ func TestFilterProposals_Rejects(t *testing.T) { // A path a resolved dependency has already ruled out must not be funded, even // if the Speculator proposes it. func TestFilterProposals_RejectsBrokenPath(t *testing.T) { - path := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionIgnored) + path := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionFails) snap := checkSnapshot(entity.BatchStateSpeculating) snap.batches[dep1] = entity.Batch{ID: dep1, State: entity.BatchStateFailed} @@ -211,7 +211,7 @@ func TestIsWellFormed(t *testing.T) { name: "order does not matter", path: entity.SpeculationPath{Head: head, Dependencies: []entity.PathDependency{ {Batch: dep2, Assumption: entity.DependencyAssumptionSucceeds}, - {Batch: dep1, Assumption: entity.DependencyAssumptionIgnored}, + {Batch: dep1, Assumption: entity.DependencyAssumptionFails}, }}, want: true, }, diff --git a/submitqueue/orchestrator/controller/speculate/doc.go b/submitqueue/orchestrator/controller/speculate/doc.go index bed664f34..109269bfc 100644 --- a/submitqueue/orchestrator/controller/speculate/doc.go +++ b/submitqueue/orchestrator/controller/speculate/doc.go @@ -29,10 +29,10 @@ // # Paths // // The batch being speculated on is the head. One complete guess about it is a -// path: one assumption per dependency, each "succeeds", "fails", or "ignored" -// (no claim either way). A path's ID hashes the head and its assumptions, so -// a path *is* its guess; building the same guess again is a new attempt of -// the same path, and (path ID, attempt) names the resulting build. +// path: one assumption per dependency, each "succeeds" or "fails". A path's ID +// hashes the head and its assumptions, so a path *is* its guess; building the +// same guess again is a new attempt of the same path, and (path ID, attempt) +// names the resulting build. // // # A worked example // diff --git a/submitqueue/orchestrator/controller/speculate/outcome.go b/submitqueue/orchestrator/controller/speculate/outcome.go index 16932ad17..9a102816e 100644 --- a/submitqueue/orchestrator/controller/speculate/outcome.go +++ b/submitqueue/orchestrator/controller/speculate/outcome.go @@ -65,12 +65,12 @@ func decide(head entity.Batch, set entity.SpeculationPathSet, snap snapshot) out // // This is what makes speculation pay. The head waits only on the dependencies // the passed build was stacked on — not on its full dependency list — so a -// batch built without a slow neighbour, or with that neighbour relaxed to -// ignored, merges as soon as the ones it actually built on have landed. +// batch built without a slow neighbour merges as soon as the ones it actually +// built on have landed. // // 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. An ignored dependency imposes no wait by definition. +// vindicated on it. func mergeablePath(set entity.SpeculationPathSet, snap snapshot) (entity.SpeculationPathEntry, bool) { for _, entry := range set.Paths { if entry.Status != entity.SpeculationPathStatusPassed { diff --git a/submitqueue/orchestrator/controller/speculate/outcome_test.go b/submitqueue/orchestrator/controller/speculate/outcome_test.go index 97801ee5e..9f5231d3b 100644 --- a/submitqueue/orchestrator/controller/speculate/outcome_test.go +++ b/submitqueue/orchestrator/controller/speculate/outcome_test.go @@ -37,7 +37,6 @@ func TestMergeablePath(t *testing.T) { const ( succeeds = entity.DependencyAssumptionSucceeds fails = entity.DependencyAssumptionFails - ignored = entity.DependencyAssumptionIgnored ) tests := []struct { @@ -49,25 +48,19 @@ func TestMergeablePath(t *testing.T) { }{ { name: "waits for an assumed-succeeding dependency to merge", - assumption: [2]entity.DependencyAssumption{succeeds, ignored}, + assumption: [2]entity.DependencyAssumption{succeeds, fails}, dep1State: entity.BatchStateSpeculating, want: false, }, { name: "merges once it has", - assumption: [2]entity.DependencyAssumption{succeeds, ignored}, + assumption: [2]entity.DependencyAssumption{succeeds, fails}, dep1State: entity.BatchStateSucceeded, want: true, }, { name: "an assumed-failing dependency imposes no wait", - assumption: [2]entity.DependencyAssumption{fails, ignored}, - dep1State: entity.BatchStateSpeculating, - want: true, - }, - { - name: "an ignored dependency imposes no wait", - assumption: [2]entity.DependencyAssumption{ignored, ignored}, + assumption: [2]entity.DependencyAssumption{fails, fails}, dep1State: entity.BatchStateSpeculating, want: true, }, @@ -104,7 +97,7 @@ func TestMergeablePath_IgnoresUnpassedPaths(t *testing.T) { } { t.Run(string(status), func(t *testing.T) { set := setOf(entryFor( - pathOver(entity.DependencyAssumptionIgnored, entity.DependencyAssumptionIgnored), status)) + pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionSucceeds), status)) _, ok := mergeablePath(set, snapWith(entity.BatchStateSucceeded, entity.BatchStateSucceeded)) assert.False(t, ok) }) @@ -114,7 +107,7 @@ func TestMergeablePath_IgnoresUnpassedPaths(t *testing.T) { // A passed build whose assumptions reality has since contradicted is not a // licence to merge — it verified a world that did not happen. func TestMergeablePath_ExcludesBrokenPassedPath(t *testing.T) { - set := setOf(passedPath(entity.DependencyAssumptionFails, entity.DependencyAssumptionIgnored)) + set := setOf(passedPath(entity.DependencyAssumptionFails, entity.DependencyAssumptionFails)) // The path was built without dep1, but dep1 landed after all. _, ok := mergeablePath(set, snapWith(entity.BatchStateSucceeded, entity.BatchStateSpeculating)) @@ -123,8 +116,10 @@ func TestMergeablePath_ExcludesBrokenPassedPath(t *testing.T) { func TestHasNoViableFuture(t *testing.T) { headBatch := entity.Batch{ID: head, Dependencies: []string{dep1, dep2}} + // With every dependency resolved exactly one assumption pair is unbroken, + // so every live path here has that shape and they differ by ID. failed := entryFor( - pathOver(entity.DependencyAssumptionIgnored, entity.DependencyAssumptionIgnored), + pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionSucceeds), entity.SpeculationPathStatusFailed) t.Run("waits while a dependency is unresolved", func(t *testing.T) { @@ -140,7 +135,7 @@ func TestHasNoViableFuture(t *testing.T) { t.Run("does not fail while a path is still running", func(t *testing.T) { running := entryFor( - pathOver(entity.DependencyAssumptionIgnored, entity.DependencyAssumptionIgnored), + pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionSucceeds), entity.SpeculationPathStatusBuilding) running.ID = "still-running" snap := snapWith(entity.BatchStateSucceeded, entity.BatchStateSucceeded) @@ -157,7 +152,7 @@ func TestHasNoViableFuture(t *testing.T) { // This path assumed dep1 would fail; it succeeded, so the failed build // tells us nothing about a future that can still happen. brokenFail := entryFor( - pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionIgnored), + pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionSucceeds), entity.SpeculationPathStatusFailed) snap := snapWith(entity.BatchStateSucceeded, entity.BatchStateSucceeded) assert.False(t, hasNoViableFuture(headBatch, setOf(brokenFail), snap)) @@ -169,11 +164,14 @@ func TestDecide(t *testing.T) { allResolved := snapWith(entity.BatchStateSucceeded, entity.BatchStateSucceeded) passed := passedPath(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionSucceeds) + // Only one assumption pair is unbroken here, so the live failed path has + // the same shape as the passed one and the ID separates them in a set. failed := entryFor( - pathOver(entity.DependencyAssumptionIgnored, entity.DependencyAssumptionIgnored), + pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionSucceeds), entity.SpeculationPathStatusFailed) + failed.ID = "failed-attempt" building := entryFor( - pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionIgnored), + pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionFails), entity.SpeculationPathStatusBuilding) assert.Equal(t, outcomeMerge, decide(headBatch, setOf(passed), allResolved)) @@ -187,12 +185,12 @@ func TestDecide(t *testing.T) { // Once a path has passed, its siblings cannot help the head but are still // holding CI slots the rest of the queue could use. func TestSupersede(t *testing.T) { - winner := passedPath(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionIgnored) + winner := passedPath(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionSucceeds) sibling := entryFor( - pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionIgnored), + pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionSucceeds), entity.SpeculationPathStatusBuilding) finished := entryFor( - pathOver(entity.DependencyAssumptionIgnored, entity.DependencyAssumptionIgnored), + pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionFails), entity.SpeculationPathStatusFailed) set := setOf(winner, sibling, finished) @@ -207,11 +205,11 @@ func TestSupersede(t *testing.T) { } func TestAllPathsStopped(t *testing.T) { - running := entryFor(pathOver(entity.DependencyAssumptionIgnored, entity.DependencyAssumptionIgnored), + running := entryFor(pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionSucceeds), entity.SpeculationPathStatusBuilding) - cancelling := entryFor(pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionIgnored), + cancelling := entryFor(pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionFails), entity.SpeculationPathStatusCancelling) - done := entryFor(pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionIgnored), + done := entryFor(pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionFails), entity.SpeculationPathStatusCancelled) assert.True(t, allPathsStopped(setOf(done))) @@ -222,9 +220,9 @@ func TestAllPathsStopped(t *testing.T) { } func TestCancelAllPaths(t *testing.T) { - running := entryFor(pathOver(entity.DependencyAssumptionIgnored, entity.DependencyAssumptionIgnored), + running := entryFor(pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionSucceeds), entity.SpeculationPathStatusBuilding) - done := entryFor(pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionIgnored), + done := entryFor(pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionFails), entity.SpeculationPathStatusPassed) set := setOf(running, done) diff --git a/submitqueue/orchestrator/controller/speculate/run_test.go b/submitqueue/orchestrator/controller/speculate/run_test.go index 150c30f91..8e27f4fc2 100644 --- a/submitqueue/orchestrator/controller/speculate/run_test.go +++ b/submitqueue/orchestrator/controller/speculate/run_test.go @@ -290,7 +290,7 @@ func TestRun_PassesSnapshotToSpeculator(t *testing.T) { // without the Speculator being consulted. func TestRun_CancelsBrokenPath(t *testing.T) { ctrl := gomock.NewController(t) - broken := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionIgnored) + broken := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionFails) spec := &scriptedSpeculator{} h := newRunHarness(t, ctrl, spec, []entity.Batch{speculatingHead()}) @@ -385,7 +385,7 @@ func TestRun_RedispatchesPendingPath(t *testing.T) { // run, and the rest of the queue is unaffected. func TestRun_LostCASIsNotAnError(t *testing.T) { ctrl := gomock.NewController(t) - broken := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionIgnored) + broken := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionFails) spec := &scriptedSpeculator{} h := newRunHarness(t, ctrl, spec, []entity.Batch{speculatingHead()}) @@ -534,7 +534,7 @@ func TestRun_DoesNotReReadFinishedPaths(t *testing.T) { // throw away. func TestRun_BrokenPathsAreVisibleToTheSpeculator(t *testing.T) { ctrl := gomock.NewController(t) - broken := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionIgnored) + broken := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionFails) spec := &scriptedSpeculator{} h := newRunHarness(t, ctrl, spec, []entity.Batch{speculatingHead()}) @@ -562,8 +562,8 @@ func TestRun_BrokenPathsAreVisibleToTheSpeculator(t *testing.T) { // may still be occupying CI, and only the signal that sees them stop can call // it done. func TestCancelBrokenPathsInSet(t *testing.T) { - broken := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionIgnored) - intact := pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionIgnored) + broken := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionFails) + intact := pathOver(entity.DependencyAssumptionFails, entity.DependencyAssumptionFails) set := entity.SpeculationPathSet{ Head: head, @@ -585,7 +585,7 @@ func TestCancelBrokenPathsInSet(t *testing.T) { // A path whose build already finished is left alone: a recorded outcome is not // something a later run gets to revise. func TestCancelBrokenPathsInSet_LeavesFinishedPaths(t *testing.T) { - broken := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionIgnored) + broken := pathOver(entity.DependencyAssumptionSucceeds, entity.DependencyAssumptionFails) snap := snapWith(entity.BatchStateFailed, entity.BatchStateSpeculating) for _, status := range []entity.SpeculationPathStatus{ diff --git a/submitqueue/orchestrator/controller/speculate/snapshot.go b/submitqueue/orchestrator/controller/speculate/snapshot.go index 03d738541..2e43b3892 100644 --- a/submitqueue/orchestrator/controller/speculate/snapshot.go +++ b/submitqueue/orchestrator/controller/speculate/snapshot.go @@ -91,8 +91,7 @@ func (s snapshot) batchState(id string) entity.BatchState { // assumptionBroken reports whether a finished dependency has already proven // one of the path's assumptions wrong: a dependency the path assumed would // succeed ended some other way, or one it assumed would fail succeeded. A -// dependency still in flight proves nothing either way, and an ignored one -// never does — the path made no claim about it. +// dependency still in flight proves nothing either way. func assumptionBroken(path entity.SpeculationPath, snap snapshot) bool { for _, dep := range path.Dependencies { state := snap.batchState(dep.Batch) diff --git a/submitqueue/orchestrator/controller/speculate/snapshot_test.go b/submitqueue/orchestrator/controller/speculate/snapshot_test.go index a8c9d68fd..0099fa477 100644 --- a/submitqueue/orchestrator/controller/speculate/snapshot_test.go +++ b/submitqueue/orchestrator/controller/speculate/snapshot_test.go @@ -59,7 +59,6 @@ func TestAssumptionBroken(t *testing.T) { const ( succeeds = entity.DependencyAssumptionSucceeds fails = entity.DependencyAssumptionFails - ignored = entity.DependencyAssumptionIgnored ) tests := []struct { @@ -77,15 +76,11 @@ func TestAssumptionBroken(t *testing.T) { {"fails holds when it fails", fails, failed, false}, {"fails holds when it is cancelled", fails, cancelled, false}, {"fails broken when it succeeds", fails, succeeded, true}, - - {"ignored survives success", ignored, succeeded, false}, - {"ignored survives failure", ignored, failed, false}, - {"ignored survives cancellation", ignored, cancelled, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - path := pathOver(tt.assumption, entity.DependencyAssumptionIgnored) + path := pathOver(tt.assumption, entity.DependencyAssumptionFails) assert.Equal(t, tt.want, assumptionBroken(path, snapWith(tt.depState, entity.BatchStateSpeculating))) }) }