diff --git a/doc/rfc/submitqueue/speculation.md b/doc/rfc/submitqueue/speculation.md index a0a744d9..f80f1fdb 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 bd0ce639..1d199c90 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 5cc32416..923d03a8 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 ce770ad7..bc068673 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 5ade579d..dcb9a69c 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 eb140d37..f82cd8a2 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 e092a3b6..b095f927 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 bed664f3..109269bf 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 16932ad1..9a102816 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 97801ee5..9f5231d3 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 150c30f9..8e27f4fc 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 03d73854..2e43b389 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 a8c9d68f..0099fa47 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))) }) }