fix(speculation): overlay queue predictor factors

A queue predictor block now revises named factors instead of replacing the whole map, so defaults like pathFailed stay in force. Best-first tests rank pathFailed, cancelling, and merging through the evidence predictor rather than a stub.

Author: behinddwalls

service/submitqueue/orchestrator/server/config.go

@@ -16,6 +16,7 @@ package main
 
 import (
 	"fmt"
+	"maps"
 	"math"
 	"os"
 	"time"
@@ -249,7 +250,7 @@ type bucketConfig struct {
 }
 
 // speculatorConfig tunes how much CI a queue's speculation may occupy. It has no
-// `type`: there is one speculator, composed from the queue's scorer, and what
+// `type`: there is one speculator, composed from the queue's predictor, and what
 // varies between queues is what it is allowed to spend.
 type speculatorConfig struct {
 	// BuildBudget caps how many builds this queue may have occupying CI at once,
@@ -264,8 +265,9 @@ type speculatorConfig struct {
 type predictorConfig struct {
 	Type string `yaml:"type"`
 	// Factors revise the scorer's price, one per piece of evidence and keyed by
-	// evidence name. An omitted factor is neutral, so an omitted block ranks on
-	// the scorer's price alone.
+	// evidence name. An omitted key keeps the inherited value, or 1 if neither
+	// defaults nor the queue named it. An omitted predictor block inherits the
+	// whole default, so every factor stays 1 until someone sets one.
 	Factors map[string]float64 `yaml:"factors"`
 }
 
@@ -394,11 +396,30 @@ func (c profilesConfig) resolve(q namedQueueProfileConfig) queueProfileConfig {
 		profile.Speculator = *q.Speculator
 	}
 	if q.Predictor != nil {
-		profile.Predictor = *q.Predictor
+		profile.Predictor = overlayPredictor(profile.Predictor, *q.Predictor)
 	}
 	return profile
 }
 
+// overlayPredictor keeps default factors the queue did not name. A present
+// predictor block is otherwise a normal extension override: type replaces when
+// set, and named factor keys win.
+func overlayPredictor(base, override predictorConfig) predictorConfig {
+	if override.Type != "" {
+		base.Type = override.Type
+	}
+	if len(override.Factors) == 0 {
+		return base
+	}
+	merged := maps.Clone(base.Factors)
+	if merged == nil {
+		merged = make(map[string]float64, len(override.Factors))
+	}
+	maps.Copy(merged, override.Factors)
+	base.Factors = merged
+	return base
+}
+
 func (p *queueProfileConfig) normalizeAndValidate(where string) error {
 	if err := p.ChangeProvider.normalizeAndValidate(where); err != nil {
 		return err

service/submitqueue/orchestrator/server/config_test.go

@@ -713,3 +713,18 @@ func TestLoadProfilesConfig_ReadsPredictorFactors(t *testing.T) {
 	assert.Equal(t, 12.0, factors.Merging)
 	assert.Equal(t, 0.1, factors.Cancelling)
 }
+
+func TestLoadProfilesConfig_QueuePredictorFactorsOverlayDefaults(t *testing.T) {
+	cfg, err := loadProfilesConfig(writeProfiles(t,
+		"defaults:\n  predictor:\n    factors: {pathPassed: 10, pathFailed: 0.3, merging: 12, cancelling: 0.1}\nqueues:\n  - name: q\n    predictor:\n      factors: {pathPassed: 4}\n"))
+	require.NoError(t, err)
+
+	factors := factorsFrom(cfg.resolve(cfg.Queues[0]).Predictor)
+	assert.Equal(t, 4.0, factors.PathPassed)
+	assert.Equal(t, 0.3, factors.PathFailed)
+	assert.Equal(t, 12.0, factors.Merging)
+	assert.Equal(t, 0.1, factors.Cancelling)
+
+	defaults := factorsFrom(cfg.Defaults.Predictor)
+	assert.Equal(t, 10.0, defaults.PathPassed)
+}

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

@@ -461,38 +461,42 @@ type flatScorer struct{}
 
 func (flatScorer) Score(context.Context, entity.Batch) (float64, error) { return 0.5, nil }
 
-// PathPassed on a green all-succeed build is the join the generator exists to
-// consume: same scorer price, different evidence, different rank.
-func TestBestFirst_EvidencePathPassedRanksTheGreenDependencyFirst(t *testing.T) {
-	batches := []entity.Batch{
-		{ID: "q/built", State: entity.BatchStateSpeculating},
-		{ID: "q/fresh", State: entity.BatchStateSpeculating},
-		{ID: "q/H", State: entity.BatchStateSpeculating, Dependencies: []string{"q/built", "q/fresh"}},
-	}
-	built := entity.SpeculationPathSet{
+func evidencePredictor(t *testing.T, factors evidence.Factors) predictor.Predictor {
+	t.Helper()
+	pred, err := evidence.New(predictor.Config{QueueName: "q"}, flatScorer{}, factors, tally.NoopScope)
+	require.NoError(t, err)
+	return pred
+}
+
+func allSucceedSet(head string, status entity.SpeculationPathStatus) entity.SpeculationPathSet {
+	return entity.SpeculationPathSet{
 		Queue: "q",
-		Head:  "q/built",
+		Head:  head,
 		Paths: []entity.SpeculationPathEntry{{
 			ID:     "p1",
-			Status: entity.SpeculationPathStatusPassed,
+			Status: status,
 			Path: entity.SpeculationPath{
-				Head: "q/built",
+				Head: head,
 				Dependencies: []entity.PathDependency{{
 					Batch:      "q/dep0",
 					Assumption: entity.DependencyAssumptionSucceeds,
 				}},
 			},
 		}},
 	}
-	pred, err := evidence.New(
-		predictor.Config{QueueName: "q"},
-		flatScorer{},
-		evidence.Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1},
-		tally.NoopScope,
-	)
-	require.NoError(t, err)
+}
 
-	iter, err := New(pred).Generate(context.Background(), batches, []entity.SpeculationPathSet{built})
+// PathPassed on a green all-succeed build is the join the generator exists to
+// consume: same scorer price, different evidence, different rank.
+func TestBestFirst_EvidencePathPassedRanksTheGreenDependencyFirst(t *testing.T) {
+	batches := []entity.Batch{
+		{ID: "q/built", State: entity.BatchStateSpeculating},
+		{ID: "q/fresh", State: entity.BatchStateSpeculating},
+		{ID: "q/H", State: entity.BatchStateSpeculating, Dependencies: []string{"q/built", "q/fresh"}},
+	}
+	pred := evidencePredictor(t, evidence.Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1})
+
+	iter, err := New(pred).Generate(context.Background(), batches, []entity.SpeculationPathSet{allSucceedSet("q/built", entity.SpeculationPathStatusPassed)})
 	require.NoError(t, err)
 	cands := forHead(drainAll(t, iter), "q/H")
 	require.NotEmpty(t, cands)
@@ -511,25 +515,56 @@ func TestBestFirst_EvidencePathPassedRanksTheGreenDependencyFirst(t *testing.T)
 	assert.Greater(t, cands[0].RankingScore, failScore)
 }
 
+func TestBestFirst_EvidencePathFailedPrefersTheFailedSide(t *testing.T) {
+	batches := []entity.Batch{
+		{ID: "q/failed", State: entity.BatchStateSpeculating},
+		{ID: "q/H", State: entity.BatchStateSpeculating, Dependencies: []string{"q/failed"}},
+	}
+	pred := evidencePredictor(t, evidence.Factors{PathPassed: 1, PathFailed: 0.25, Merging: 1, Cancelling: 1})
+
+	iter, err := New(pred).Generate(context.Background(), batches, []entity.SpeculationPathSet{allSucceedSet("q/failed", entity.SpeculationPathStatusFailed)})
+	require.NoError(t, err)
+	cands := forHead(drainAll(t, iter), "q/H")
+	require.Len(t, cands, 2)
+	assert.Equal(t, entity.DependencyAssumptionFails, assumptionFor(cands[0].Path, "q/failed"))
+	assert.Equal(t, entity.DependencyAssumptionSucceeds, assumptionFor(cands[1].Path, "q/failed"))
+	assert.Greater(t, cands[0].RankingScore, cands[1].RankingScore)
+}
+
+func TestBestFirst_EvidenceCancellingPrefersTheFailedSide(t *testing.T) {
+	batches := []entity.Batch{
+		{ID: "q/stopping", State: entity.BatchStateCancelling},
+		{ID: "q/H", State: entity.BatchStateSpeculating, Dependencies: []string{"q/stopping"}},
+	}
+	pred := evidencePredictor(t, evidence.Factors{PathPassed: 1, PathFailed: 1, Merging: 1, Cancelling: 0.25})
+
+	iter, err := New(pred).Generate(context.Background(), batches, nil)
+	require.NoError(t, err)
+	cands := drainAll(t, iter)
+	require.Len(t, cands, 2)
+	assert.Equal(t, entity.DependencyAssumptionFails, assumptionFor(cands[0].Path, "q/stopping"))
+	assert.Equal(t, entity.DependencyAssumptionSucceeds, assumptionFor(cands[1].Path, "q/stopping"))
+	assert.Greater(t, cands[0].RankingScore, cands[1].RankingScore)
+}
+
 // A merging dependency is still in progress — the merge can fail — so it stays
-// an open question here like any other. Whether a path betting against it is
-// worth funding is a matter of price, which is the scorer's to say, not a
-// state the search hard-codes.
+// an open question here like any other. How much it is worth is a predictor
+// price, not a fact the search hard-codes.
 func TestBestFirst_MergingDependencyStaysOpen(t *testing.T) {
 	batches := []entity.Batch{
 		{ID: "q/landing", State: entity.BatchStateMerging},
 		{ID: "q/H", State: entity.BatchStateSpeculating, Dependencies: []string{"q/landing"}},
 	}
-	sc := newCountingPredictor(map[string]float64{"q/landing": 0.9})
+	pred := evidencePredictor(t, evidence.Factors{PathPassed: 1, PathFailed: 1, Merging: 19, Cancelling: 1})
 
-	iter, err := New(sc).Generate(context.Background(), batches, nil)
+	iter, err := New(pred).Generate(context.Background(), batches, nil)
 	require.NoError(t, err)
 	cands := drainAll(t, iter)
 
-	assert.Equal(t, 1, sc.calls["q/landing"], "a merging dependency is priced like any other")
 	require.Len(t, cands, 2, "both sides of a merge that has not landed yet")
 	assert.Equal(t, entity.DependencyAssumptionSucceeds, assumptionFor(cands[0].Path, "q/landing"))
 	assert.Equal(t, entity.DependencyAssumptionFails, assumptionFor(cands[1].Path, "q/landing"))
+	assert.Greater(t, cands[0].RankingScore, cands[1].RankingScore)
 }
 
 func TestBestFirst_GeneratesOnlyWhatIsPulled(t *testing.T) {