refactor(speculation): drop snapshot validation from bestfirst (#524)

## Summary

### Why?

`Generate` re-validated the queue snapshot on every run: seven checks
covering empty and duplicate batch IDs, empty, duplicate, and self
dependencies, dependencies missing from the snapshot, and unknown
dependency states — plus a nil-scorer panic in `New`. Those are
properties of a correctly assembled snapshot, established wherever the
snapshot is built, so checking them again on the hot path of every run
spreads one contract across two places and buys nothing a well-formed
snapshot could ever trip. They were also the bulk of `Generate`'s
branching.

The scorer range check was different in kind: a score arrives from an
injected extension, so no earlier stage can vet it. But sinking the
whole run on one bad number is the wrong response when a default will
do.

### What?

Snapshot validation is gone. The `Generator` interface doc and the
best-first RFC now state the well-formedness rules as a caller
precondition — a generator may assume them, and a malformed snapshot
yields undefined candidates rather than an error. Nothing consumes the
generator yet, so whoever assembles the snapshot will own the check.

One behavior worth flagging for reviewers: a head that repeats a
dependency was previously rejected and now produces paths that assume
the same batch both succeeds and fails. That is undefined input under
the new contract, but it fails quietly rather than loudly.

A score outside `[0, 1]`, or `NaN`, is now replaced with a default of
0.95 rather than rejected. The default is optimistic on purpose: a
dependency nobody could estimate keeps its head's preferred path near
the front instead of being buried. Both comparisons in `asProbability`
are false for `NaN`, so it needs no separate case.

`Generate` is now three linear steps — index, `speculatingHeads`,
`score` — and seeding the global heap appends and heapifies once.
Previously `heap.Init` ran on an empty slice and each head was pushed
individually, costing a sift and an interface boxing per head; building
the slice first is linear. `slices.Sorted(maps.Keys(...))` replaces the
collect-then-sort loop, and `slices.Clone` replaces three `make`+`copy`
pairs.

Net 143 lines removed, 80 added.

## Test Plan

✅ `go test ./submitqueue/extension/speculation/...`
✅ `go vet ./submitqueue/extension/speculation/...`
✅ `gofmt -l submitqueue/extension/speculation/` — clean

The malformed-snapshot table test is removed. The score-range test now
asserts the default is applied and both of the head's paths still come
out, ranked as the default and its complement, rather than asserting an
error.

Author: behinddwalls

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

@@ -6,7 +6,7 @@ The default generator returns the most likely complete build path across the who
 
 The code has one setup method, `Generate`, and one repeated method, `Next`:
 
-1. `Generate` validates the queue snapshot, then records a preferred assumption and the cost of flipping to its opposite for each unresolved direct dependency. It totals the best score for each eligible head; resolved dependencies remain fixed facts.
+1. `Generate` records a preferred assumption and the cost of flipping to its opposite for each unresolved direct dependency. It totals the best score for each eligible head; resolved dependencies remain fixed facts.
 2. `Generate` pushes one lightweight best-path candidate per head into a global heap. Each head also owns a stream that enumerates its remaining paths on demand; at this point the stream has done no work beyond that total.
 3. `Next` removes the highest-ranked candidate from the global heap, advances only that head's stream, inserts the head's next candidate, and constructs and returns the complete path that was removed.
 
@@ -45,11 +45,11 @@ The scorer estimates:
 
 The batch being built is written before its assumptions. For example, `C [A succeeds, B fails]` means “build C assuming A succeeds and B fails.” The code calls C the path's **head**.
 
-## The snapshot is a strict contract
+## The snapshot is a caller precondition
 
-`Generate` receives the queue's live batches as a snapshot and validates it before doing any other work. Every batch a head's direct dependencies reference must be present with a readable state, batch IDs must be unique and non-empty, and no head may repeat a dependency or depend on itself. A snapshot that breaks any of these is malformed input, and `Generate` returns an error instead of a stream.
+`Generate` receives the queue's live batches as a snapshot and takes it as given. A well-formed snapshot carries unique, non-empty batch IDs, includes every batch a head's direct dependencies reference, and gives no head an empty, duplicate, or self dependency. Those are preconditions the caller owns, established where the snapshot is assembled. The generator does not re-check them: it is on the hot path of every run, the checks it could make are the ones an assembled-correctly snapshot can never fail, and paying for them here only spreads the same contract across two places. A malformed snapshot yields undefined candidates rather than an error.
 
-In particular, a missing dependency is never guessed about. Every unresolved dependency is scored by the injected scorer, and any defaulting for a batch that is hard to score belongs to the scorer implementation — which knows what information it does and does not have — not to the generator.
+A score that is not a probability is the one bad input the generator absorbs, because it arrives from the injected scorer rather than from the caller and there is no earlier point that could catch it. A score outside `[0, 1]`, or `NaN`, is replaced with a default of 0.95 — optimistic on purpose, so a dependency nobody could estimate keeps its head's preferred path near the front instead of burying it or failing the whole run on one number. Any deliberate defaulting still belongs to the scorer implementation, which knows what information it does and does not have; this is only the floor under it.
 
 ## Step 1: `Generate` prepares each head
 
@@ -420,8 +420,7 @@ A and D tie at 1.0, so batch ID puts A first. Other exact ties prefer fewer flip
 
 `Generate` must eagerly:
 
-- validate the snapshot;
-- score every unique unresolved direct dependency needed by an eligible head;
+- score every unique unresolved direct dependency needed by an eligible head, substituting the default for any score that is not a probability;
 - choose each unresolved dependency's preferred assumption and calculate its `flipCost`; and
 - total the best score for every head.
 

submitqueue/extension/speculation/generator/bestfirst/bestfirst.go

@@ -25,8 +25,8 @@ import (
 	"cmp"
 	"container/heap"
 	"context"
-	"errors"
 	"fmt"
+	"maps"
 	"math"
 	"slices"
 
@@ -47,72 +47,64 @@ var _ generator.Generator = (*bestFirst)(nil)
 // unresolved dependency assumption holds. The scorer is called at most once
 // per unresolved dependency batch in each Generate call.
 func New(s scorer.Scorer) generator.Generator {
-	if s == nil {
-		panic("bestfirst.New: scorer must not be nil")
-	}
 	return &bestFirst{scorer: s}
 }
 
-// Generate validates the queue snapshot, scores the unresolved dependencies of
-// Speculating heads, and opens a lazy global best-first iterator.
+// Generate scores the unresolved dependencies of the snapshot's Speculating
+// heads and opens a lazy global best-first iterator. The snapshot is taken as
+// given: it is the caller's to keep well formed, and nothing here re-checks it.
 func (g *bestFirst) Generate(ctx context.Context, batches []entity.Batch) (generator.Iterator, error) {
 	if err := ctx.Err(); err != nil {
 		return nil, err
 	}
 
 	batchByID := make(map[string]entity.Batch, len(batches))
 	for _, batch := range batches {
-		if batch.ID == "" {
-			return nil, errors.New("batch has an empty ID")
-		}
-		if _, exists := batchByID[batch.ID]; exists {
-			return nil, fmt.Errorf("duplicate batch ID %q", batch.ID)
-		}
 		batchByID[batch.ID] = batch
 	}
+	heads, unresolvedIDs := speculatingHeads(batches, batchByID)
+
+	probabilityByID, err := g.score(ctx, unresolvedIDs, batchByID)
+	if err != nil {
+		return nil, err
+	}
+
+	// Seeding the heap by appending and then heapifying once is linear, where
+	// pushing head by head would cost a sift per head.
+	it := &candidateIterator{candidates: make(candidateHeap, 0, len(heads))}
+	for _, head := range heads {
+		stream := newPathStream(head, batchByID, probabilityByID)
+		it.candidates = append(it.candidates, candidateItem{
+			stream: stream,
+			score:  stream.bestScore,
+		})
+	}
+	heap.Init(&it.candidates)
+	return it, nil
+}
 
-	heads := make([]entity.Batch, 0)
-	unresolvedDependencyIDs := make(map[string]struct{})
+// speculatingHeads picks out the batches worth proposing work on and the
+// distinct dependencies of theirs still awaiting an outcome. The dependency IDs
+// come back sorted, so scoring order does not vary with map iteration.
+func speculatingHeads(batches []entity.Batch, batchByID map[string]entity.Batch) (heads []entity.Batch, unresolvedIDs []string) {
+	unresolved := make(map[string]struct{})
 	for _, batch := range batches {
 		if batch.State != entity.BatchStateSpeculating {
 			continue
 		}
 		heads = append(heads, batch)
-
-		seen := make(map[string]struct{}, len(batch.Dependencies))
 		for _, dependencyID := range batch.Dependencies {
-			if dependencyID == "" {
-				return nil, fmt.Errorf("head %q has an empty dependency ID", batch.ID)
-			}
-			if dependencyID == batch.ID {
-				return nil, fmt.Errorf("head %q depends on itself", batch.ID)
-			}
-			if _, duplicate := seen[dependencyID]; duplicate {
-				return nil, fmt.Errorf("head %q repeats dependency %q", batch.ID, dependencyID)
-			}
-			seen[dependencyID] = struct{}{}
-
-			dependency, exists := batchByID[dependencyID]
-			if !exists {
-				return nil, fmt.Errorf("head %q references dependency %q missing from the snapshot", batch.ID, dependencyID)
-			}
-			if dependency.State == entity.BatchStateUnknown {
-				return nil, fmt.Errorf("dependency %q has an unknown state", dependencyID)
-			}
-			if _, resolved := resolvedAssumption(dependency.State); !resolved {
-				unresolvedDependencyIDs[dependencyID] = struct{}{}
+			if _, resolved := resolvedAssumption(batchByID[dependencyID].State); !resolved {
+				unresolved[dependencyID] = struct{}{}
 			}
 		}
 	}
+	return heads, slices.Sorted(maps.Keys(unresolved))
+}
 
-	// Score each unique unresolved dependency once, in a stable order. A score
-	// outside [0, 1] is rejected here because everything downstream treats it
-	// as a probability, and a bad value would corrupt the ordering silently.
-	ids := make([]string, 0, len(unresolvedDependencyIDs))
-	for id := range unresolvedDependencyIDs {
-		ids = append(ids, id)
-	}
-	slices.Sort(ids)
+// score asks the scorer for each unresolved dependency exactly once, however
+// many heads wait on it.
+func (g *bestFirst) score(ctx context.Context, ids []string, batchByID map[string]entity.Batch) (map[string]float64, error) {
 	probabilityByID := make(map[string]float64, len(ids))
 	for _, id := range ids {
 		if err := ctx.Err(); err != nil {
@@ -122,22 +114,25 @@ func (g *bestFirst) Generate(ctx context.Context, batches []entity.Batch) (gener
 		if err != nil {
 			return nil, fmt.Errorf("score dependency %q: %w", id, err)
 		}
-		if math.IsNaN(probability) || probability < 0 || probability > 1 {
-			return nil, fmt.Errorf("scorer returned %v for batch %q: want a probability in [0, 1]", probability, id)
-		}
-		probabilityByID[id] = probability
+		probabilityByID[id] = asProbability(probability)
 	}
+	return probabilityByID, nil
+}
 
-	it := &candidateIterator{}
-	heap.Init(&it.candidates)
-	for _, head := range heads {
-		stream := newPathStream(head, batchByID, probabilityByID)
-		heap.Push(&it.candidates, candidateItem{
-			stream: stream,
-			score:  stream.bestScore,
-		})
+// defaultProbability stands in for a score that is not a probability. It is
+// optimistic on purpose: a dependency nobody could estimate is treated as very
+// likely to succeed, which keeps its head's preferred path near the front
+// rather than burying it or dropping the queue's whole snapshot on one bad
+// number.
+const defaultProbability = 0.95
+
+// asProbability keeps a usable score and substitutes the default for anything
+// else. The comparisons are both false for NaN, so NaN takes the default too.
+func asProbability(score float64) float64 {
+	if score >= 0 && score <= 1 {
+		return score
 	}
-	return it, nil
+	return defaultProbability
 }
 
 // resolvedAssumption converts a terminal dependency outcome into the only
@@ -282,8 +277,7 @@ func (s *pathStream) scoreFor(flipped []int) float64 {
 // build constructs the path taking the given flips. The returned path owns its
 // dependencies.
 func (s *pathStream) build(flipped []int) entity.SpeculationPath {
-	dependencies := make([]entity.PathDependency, len(s.base))
-	copy(dependencies, s.base)
+	dependencies := slices.Clone(s.base)
 	for _, i := range flipped {
 		at := s.variables[i].dependencyIndex
 		dependencies[at].Assumption = opposite(dependencies[at].Assumption)
@@ -301,15 +295,11 @@ func opposite(assumption entity.DependencyAssumption) entity.DependencyAssumptio
 }
 
 func appendCopy(values []int, value int) []int {
-	result := make([]int, len(values)+1)
-	copy(result, values)
-	result[len(values)] = value
-	return result
+	return append(slices.Clone(values), value)
 }
 
 func replaceLastCopy(values []int, value int) []int {
-	result := make([]int, len(values))
-	copy(result, values)
+	result := slices.Clone(values)
 	result[len(result)-1] = value
 	return result
 }

submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go

@@ -392,71 +392,6 @@ func TestBestFirst_PropagatesScorerError(t *testing.T) {
 	assert.Nil(t, iter)
 }
 
-func TestBestFirst_RejectsMalformedSnapshots(t *testing.T) {
-	// The snapshot contract: every batch a head's direct dependencies reference
-	// is present with a readable state, IDs are unique and non-empty, and no
-	// head repeats a dependency or depends on itself. Anything else is
-	// malformed input. In particular a missing dependency is never guessed
-	// about — the scorer, not the generator, owns any defaulting for batches
-	// that are hard to score.
-	tests := []struct {
-		name    string
-		batches []entity.Batch
-	}{
-		{
-			name:    "empty batch ID",
-			batches: []entity.Batch{{State: entity.BatchStateSpeculating}},
-		},
-		{
-			name: "duplicate batch ID",
-			batches: []entity.Batch{
-				{ID: "q/A", State: entity.BatchStateCreated},
-				{ID: "q/A", State: entity.BatchStateSpeculating},
-			},
-		},
-		{
-			name: "empty dependency ID",
-			batches: []entity.Batch{
-				{ID: "q/H", State: entity.BatchStateSpeculating, Dependencies: []string{""}},
-			},
-		},
-		{
-			name: "self dependency",
-			batches: []entity.Batch{
-				{ID: "q/H", State: entity.BatchStateSpeculating, Dependencies: []string{"q/H"}},
-			},
-		},
-		{
-			name: "duplicate dependency",
-			batches: []entity.Batch{
-				{ID: "q/A", State: entity.BatchStateCreated},
-				{ID: "q/H", State: entity.BatchStateSpeculating, Dependencies: []string{"q/A", "q/A"}},
-			},
-		},
-		{
-			name: "missing dependency",
-			batches: []entity.Batch{
-				{ID: "q/H", State: entity.BatchStateSpeculating, Dependencies: []string{"q/ghost"}},
-			},
-		},
-		{
-			name: "unknown dependency state",
-			batches: []entity.Batch{
-				{ID: "q/A", State: entity.BatchStateUnknown},
-				{ID: "q/H", State: entity.BatchStateSpeculating, Dependencies: []string{"q/A"}},
-			},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			iter, err := New(scored(nil)).Generate(context.Background(), tt.batches)
-			require.Error(t, err)
-			assert.Nil(t, iter)
-		})
-	}
-}
-
 func TestBestFirst_GeneratesOnlyWhatIsPulled(t *testing.T) {
 	// 12 unresolved dependencies is an outcome space of 4096 paths.
 	const deps, space = 12, 1 << 12
@@ -841,12 +776,14 @@ func TestBestFirst_HonorsCancelledContext(t *testing.T) {
 	})
 }
 
-func TestBestFirst_RejectsScoreOutsideUnitInterval(t *testing.T) {
+func TestBestFirst_DefaultsScoreOutsideUnitInterval(t *testing.T) {
 	// Everything downstream treats a score as a probability: its log is the
-	// ranking key and its complement is the other side's probability. An
-	// out-of-range value would not fail loudly, it would quietly produce a
+	// ranking key and its complement is the other side's probability. Carrying
+	// an out-of-range value would not fail loudly, it would quietly produce a
 	// positive log, an inverted ordering, or a NaN that makes every comparison
-	// false — so it is rejected at the source instead.
+	// false. The scorer is an injected extension and nothing earlier can vet
+	// what it returns, so a value that is not a probability is replaced with an
+	// optimistic default here rather than sinking the whole run.
 	tests := []struct {
 		name  string
 		score float64
@@ -855,6 +792,7 @@ func TestBestFirst_RejectsScoreOutsideUnitInterval(t *testing.T) {
 		{name: "below zero", score: -0.1},
 		{name: "not a number", score: math.NaN()},
 		{name: "positive infinity", score: math.Inf(1)},
+		{name: "negative infinity", score: math.Inf(-1)},
 	}
 
 	batches := []entity.Batch{
@@ -865,8 +803,16 @@ func TestBestFirst_RejectsScoreOutsideUnitInterval(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			iter, err := New(constScorer{tt.score}).Generate(context.Background(), batches)
-			require.Error(t, err)
-			assert.Nil(t, iter)
+			require.NoError(t, err)
+			cands := drainAll(t, iter)
+
+			// The dependency is scored at the default, so the head still yields
+			// both of its paths, ranked as that default and its complement.
+			require.Len(t, cands, 2)
+			assert.Equal(t, entity.DependencyAssumptionSucceeds, assumptionFor(cands[0].Path, "q/A"))
+			assert.InDelta(t, math.Log(defaultProbability), cands[0].RankingScore, 1e-9)
+			assert.Equal(t, entity.DependencyAssumptionFails, assumptionFor(cands[1].Path, "q/A"))
+			assert.InDelta(t, math.Log(1-defaultProbability), cands[1].RankingScore, 1e-9)
 		})
 	}
 }

submitqueue/extension/speculation/generator/generator.go

@@ -40,10 +40,11 @@ type Generator interface {
 	// queue that has moved on is a new snapshot and a new Generate, which is how
 	// batches are revised anyway — they are replaced, not edited in place.
 	//
-	// The snapshot must include every batch a head's direct dependencies
-	// reference. A snapshot that breaks that — or carries empty or duplicate
-	// batch IDs, or a head with an empty, duplicate, or self dependency — is
-	// malformed input and aborts with an error rather than a stream.
+	// A well-formed snapshot carries unique, non-empty batch IDs, includes every
+	// batch a head's direct dependencies reference, and gives no head an empty,
+	// duplicate, or self dependency. That is a precondition the caller owns: a
+	// generator may assume it and is not required to detect a breach, so a
+	// malformed snapshot yields undefined candidates rather than an error.
 	Generate(ctx context.Context, batches []entity.Batch) (Iterator, error)
 }