Merge pull request 'Agree a toolchain by strict majority; a node on another version sits out (ADR 0584, issue 1578)' (#154) from fix/toolchain-majority into main

go/buildstep/askrun.go

@@ -25,7 +25,7 @@ type StepFunc func(ctx context.Context, args map[string]any) (any, error)
 // AskSteps are the step tools a build module runs an ask with. Copy is the image module's alone; Digest the Go module's.
 type AskSteps struct {
 	Fetch, Build, Copy, Digest, Test StepFunc
-	// Toolchain is the toolchain the module builds with now; a build ask naming another is refused. Nil checks nothing
+	// Toolchain is the toolchain the module builds with now; a build ask naming another is declined (Declining). Nil checks nothing
 	// (an image or an archive names none).
 	Toolchain func(ctx context.Context) (delivery.ToolchainUsed, error)
 	// Kept looks up a kept verdict by level and digest (the checker's kept bucket, delivery.KeptKey): whether one is
@@ -107,19 +107,32 @@ func (s AskSteps) fetch(ctx context.Context, args map[string]any) (json.RawMessa
 	return r, f, nil
 }
 
+// onToolchain is the toolchain this node builds with when the ask names one and this module reports its own, nil when
+// either is absent. A node on another toolchain than the ask names answers a Declining: it is not on the agreed one and
+// sits out, for a node that is to take the ask (novox/hq ADR 0584 clause 2).
+func (s AskSteps) onToolchain(ctx context.Context, want *delivery.ToolchainUsed) (*delivery.ToolchainUsed, error) {
+	if want == nil || s.Toolchain == nil {
+		return nil, nil
+	}
+	tc, err := s.Toolchain(ctx)
+	if err != nil {
+		return nil, err
+	}
+	if tc.Version != want.Version || tc.Recipe != want.Recipe {
+		return nil, &Declining{Lacks: []string{fmt.Sprintf("the agreed toolchain %s (recipe %s)", want.Version, want.Recipe)},
+			Text: fmt.Sprintf("the ask is for %s (recipe %s) and this node builds with %s (recipe %s): it sits out, and the ask is left to a node on the agreed toolchain",
+				want.Version, want.Recipe, tc.Version, tc.Recipe)}
+	}
+	return &tc, nil
+}
+
 func (s AskSteps) build(ctx context.Context, job delivery.BuildJob) (any, error) {
 	answer := delivery.BuildAnswer{}
-	if job.Toolchain != nil && s.Toolchain != nil {
-		tc, err := s.Toolchain(ctx)
-		if err != nil {
-			return nil, err
-		}
-		if tc.Version != job.Toolchain.Version || tc.Recipe != job.Toolchain.Recipe {
-			return nil, Refuse("the ask is for %s (recipe %s) and this node builds with %s (recipe %s)",
-				job.Toolchain.Version, job.Toolchain.Recipe, tc.Version, tc.Recipe)
-		}
-		answer.Toolchain = &tc
+	tc, err := s.onToolchain(ctx, job.Toolchain)
+	if err != nil {
+		return nil, err
 	}
+	answer.Toolchain = tc
 	var f fetched
 	defer func() { s.release(f.Workspace) }()
 	for _, a := range job.Artifacts {
@@ -181,6 +194,9 @@ func (s AskSteps) build(ctx context.Context, job delivery.BuildJob) (any, error)
 }
 
 func (s AskSteps) check(ctx context.Context, job delivery.CheckJob) (any, error) {
+	if _, err := s.onToolchain(ctx, job.Toolchain); err != nil {
+		return nil, err
+	}
 	r, f, err := s.fetch(ctx, job.Fetch)
 	if err != nil {
 		return nil, err

go/buildstep/askrun_test.go

@@ -103,7 +103,7 @@ build {"artifact":"lib","module":"modules/m","publish":true,"workspace":"/w/1"}`
 	}
 }
 
-// A build ask for another toolchain is refused, so its step ends refused and the asker reads why.
+// A build ask for another toolchain is declined (ADR 0584 clause 2), so the node sits out and the asker puts it again.
 func TestRunAskRefusesAnotherToolchain(t *testing.T) {
 	r := &recorder{fetch: map[string]any{"workspace": "/w"}}
 	s := r.steps()
@@ -112,10 +112,16 @@ func TestRunAskRefusesAnotherToolchain(t *testing.T) {
 	}
 	_, err := RunAsk(context.Background(), s, ask(delivery.BuildJob{Module: "m", Path: ".", Fetch: map[string]any{"repository": "novox/x", "commit": runCommit},
 		Artifacts: []delivery.AskArtifact{{Artifact: "bundle"}}, Toolchain: &delivery.ToolchainUsed{Version: "go1.26", Recipe: "3"}}))
-	var ref *Refusal
-	if !errors.As(err, &ref) || len(r.calls) != 0 {
+	var dec *Declining
+	if !errors.As(err, &dec) || len(dec.Lacks) != 1 || len(r.calls) != 0 {
 		t.Fatalf("err %v, calls %v", err, r.calls)
 	}
+	// A check ask names the toolchain too (ADR 0584), and a node on another declines it before it fetches.
+	_, err = RunAsk(context.Background(), s, ask(delivery.CheckJob{Fetch: map[string]any{"repository": "novox/x", "commit": runCommit}, Levels: []string{delivery.LevelUnit},
+		Toolchain: &delivery.ToolchainUsed{Version: "go1.26", Recipe: "3"}}))
+	if !errors.As(err, &dec) || len(r.calls) != 0 {
+		t.Fatalf("a check ask: err %v, calls %v", err, r.calls)
+	}
 }
 
 // 1887/A3, A5a: a check ask runs the levels of one check after one fetch: it digests the unit level, takes the kept

go/buildstep/pull.go

@@ -538,6 +538,28 @@ func (p *Puller) decline(msg *Pulled, ask delivery.StepAsk, lacks []string) {
 	p.mu.Unlock()
 }
 
+// declineRun ends the step of an ask that began and then found this node should not run it (Declining), as decline does
+// before it begins: refused as declined, the message dropped for its asker to put again, and the worker it came from left
+// alone until the next pull's wait.
+func (p *Puller) declineRun(msg *Pulled, ask delivery.StepAsk, key string, step delivery.Step, d *Declining) {
+	result, _ := json.Marshal(delivery.StepDeclined{Declined: true, Needs: ask.Needs, Lacks: d.Lacks})
+	step.Ended = now().UTC()
+	step.State, step.Reason, step.Result = delivery.StepRefused, d.Text, result
+	if err := writeEnd(p.Put, key, step); err != nil {
+		p.logf("the ask %s cannot be declined on %s, so it is given back: %v", ask.ID, p.Node, err)
+		_ = msg.Nak()
+		return
+	}
+	p.logf("the ask %s is declined on %s: %s", ask.ID, p.Node, d.Text)
+	_ = msg.Term()
+	p.mu.Lock()
+	if p.declined == nil {
+		p.declined = map[string]bool{}
+	}
+	p.declined[msg.Kind] = true
+	p.mu.Unlock()
+}
+
 func withKind(msg *Pulled, kind string) *Pulled {
 	if msg != nil {
 		msg.Kind = kind
@@ -625,6 +647,11 @@ func (p *Puller) run(ctx context.Context, msg *Pulled, ask delivery.StepAsk, key
 	})
 	step = ph.end() // its end keeps the phase it ended in
 	close(done)
+	var dec *Declining
+	if errors.As(err, &dec) && ctx.Err() == nil {
+		p.declineRun(msg, ask, key, step, dec)
+		return
+	}
 	if ctx.Err() != nil {
 		// The process stops: the ask is neither ended nor acknowledged here, so the stream gives it to a node that runs.
 		return

go/buildstep/pull_test.go

@@ -230,6 +230,21 @@ func TestPullDropsWhatIsNoAsk(t *testing.T) {
 		Run: func(ctx context.Context, a delivery.StepAsk) (any, error) { return nil, nil }}
 	go q.Pull(ctx)
 	waitFor(t, "the placed ask acked where its need is met", func() bool { a, _, _ := has.Fates(); return len(a) == 1 })
+	// A run that finds its node is not on the agreed toolchain declines the ask after it began (ADR 0584 clause 2); a node on it builds.
+	sit, sitSteps := NewFakeAsks(), newSteps()
+	toolchain := testAsk(delivery.AskDelivery)
+	_ = sit.PublishAsk(toolchain)
+	s2 := &Puller{Source: sit, Language: delivery.AskGo, Node: "ace", Put: sitSteps.put, Capacity: func() int { return 1 }, PullWait: 10 * time.Millisecond,
+		Run: func(ctx context.Context, a delivery.StepAsk) (any, error) {
+			return nil, &Declining{Text: "the ask is for 26.13.0 and this node builds with 26.12.1", Lacks: []string{"the agreed toolchain 26.13.0 (recipe 3)"}}
+		}}
+	go s2.Pull(ctx)
+	waitFor(t, "the ask termed on the node that sits out", func() bool { _, tm, _ := sit.Fates(); return len(tm) == 1 })
+	sk, _ := delivery.AskStepKey("ace", toolchain.ID)
+	ss, _ := sitSteps.get(sk)
+	if d, ok := delivery.DeclinedOf(ss); !ok || len(d.Lacks) != 1 || !strings.Contains(ss.Reason, "26.12.1") {
+		t.Fatalf("the sitting-out step = %+v", ss)
+	}
 }
 
 // load reads the recorded steps state's raw values: the pause key is not a step, so it is kept apart.

go/buildstep/refusal.go

@@ -16,3 +16,14 @@ func (r *Refusal) Error() string { return r.Text }
 
 // Refuse is a Refusal with its text formatted.
 func Refuse(format string, args ...any) *Refusal { return &Refusal{Text: fmt.Sprintf(format, args...)} }
+
+// Declining is a step a node does not run because another node should: it ends refused as declined (delivery.StepDeclined),
+// the ask's message is dropped, and the asker puts the ask again under a new id (issue 890). A build ask naming a
+// toolchain other than this node's is one (novox/hq ADR 0584 clause 2): a node that is not in the toolchain majority
+// sits out and never fails a build.
+type Declining struct {
+	Text  string
+	Lacks []string // what this node lacks, in words the operator reads
+}
+
+func (d *Declining) Error() string { return d.Text }

go/delivery/buildrun/standing_test.go

@@ -91,8 +91,8 @@ func TestToolchainGate(t *testing.T) {
 		gate, set, said := start(t, true)
 		key, value := gateReport(t, "ace", "v26.12.0", old)
 		gate.Toolchain(NodeBuildModule, key, value, false)
-		if _, none := agreedOf(set); !strings.HasPrefix(none, "builders disagree") {
-			t.Fatalf("before the placement is settled every report counts; got %q", none)
+		if a := delivery.ToolchainAgreement(set.Entries("typescript")); len(a.SittingOut) != 1 || a.SittingOut[0].Node != "ace" { // ADR 0584: the majority is agreed, ace's report counts as the one that sits out
+			t.Fatalf("before the placement is settled every report counts; got %+v", a)
 		}
 		k, v := placing(t, "ace", GoBuildModule) // another build module on ace: no matter
 		gate.Placement(k, v, false, old)
@@ -111,8 +111,8 @@ func TestToolchainGate(t *testing.T) {
 		// Placed on ace with a report written since: it counts, and the builders really disagree.
 		k, v = placing(t, "ace", GoBuildModule, NodeBuildModule)
 		gate.Placement(k, v, false, old.Add(-time.Minute))
-		if _, none := agreedOf(set); !strings.HasPrefix(none, "builders disagree: v26.12.0 recipe f59571cf4be75adf on ace") {
-			t.Fatalf("builders that run different versions still disagree; got %q", none)
+		if a := delivery.ToolchainAgreement(set.Entries("typescript")); a.Toolchain.Version != "v26.12.1" || len(a.SittingOut) != 1 || a.SittingOut[0].Version != "v26.12.0" {
+			t.Fatalf("builders that run different versions: the majority is agreed and ace sits out; got %+v", a)
 		}
 		// Taken off ace again (its key deleted): its report goes again.
 		if !gate.Placement(k, nil, true, placed) {

go/delivery/stepasks.go

@@ -369,6 +369,9 @@ type CheckJob struct {
 	// has to list them. The build step hands them to its test tool as the argument NodeNamesArg, and the build module
 	// gives them to the tests in NodeNamesEnv.
 	NodeNames []string `json:"node-names,omitempty"`
+	// Toolchain is the toolchain the checker agreed for the check's language (novox/hq ADR 0584): a node building with
+	// another declines the ask, so a pass is never recorded as run on a version it did not run on. Nil names none.
+	Toolchain *ToolchainUsed `json:"toolchain,omitempty"`
 }
 
 // NodeNamesArg is the test tool's argument that carries a check's node names, and NodeNamesEnv the environment

go/delivery/toolchains.go

@@ -99,22 +99,47 @@ type NodeToolchain struct {
 	Toolchain
 }
 
-// AgreedToolchain is the one Toolchain2 every node's entry of a language agrees on, or none and why: "toolchain not
-// yet reported" with no entries, and "builders disagree" naming each differing version and recipe with the nodes
-// reporting it when two differ (novox/hq ADR 0405 clause 2). The answer does not depend on the entries' order. The
-// entries must all be of one language.
-func AgreedToolchain(entries []NodeToolchain) (tc Toolchain2, none string) {
+// SittingOut is a node whose toolchain of a language is not the agreed one: it builds nothing of that language until
+// it reports the agreed version (novox/hq ADR 0584 clause 2).
+type SittingOut struct {
+	Node    string
+	Version string
+	Recipe  string
+}
+
+// Agreement is what the nodes' entries of one language come to (novox/hq ADR 0584). Toolchain is the agreed one, set
+// when Wait is empty. Wait is why nothing is agreed: "toolchain not yet reported" with no entries, or "builders
+// disagree" naming each version and recipe with the nodes reporting it, when no version has a strict majority.
+// SittingOut lists, sorted by node, the nodes that report another version than the agreed one.
+type Agreement struct {
+	Toolchain  Toolchain2
+	SittingOut []SittingOut
+	Wait       string
+}
+
+// ToolchainAgreement is the agreed toolchain of a language: the version and recipe a strict majority of the
+// reporting nodes report (novox/hq ADR 0584 clause 1). With no strict majority (two against two) or no entry it
+// gives no toolchain and says why in Wait. The answer does not depend on the entries' order. The entries must all be
+// of one language.
+func ToolchainAgreement(entries []NodeToolchain) Agreement {
 	if len(entries) == 0 {
-		return Toolchain2{}, ToolchainNoReport
+		return Agreement{Wait: ToolchainNoReport}
 	}
 	groups := map[Toolchain2][]string{}
 	for _, e := range entries {
 		k := Toolchain2{Language: e.Language, Version: e.Version, Recipe: e.Recipe}
 		groups[k] = append(groups[k], e.Node)
 	}
-	if len(groups) == 1 {
-		for k := range groups {
-			return k, ""
+	for k, nodes := range groups {
+		if 2*len(nodes) > len(entries) {
+			a := Agreement{Toolchain: k}
+			for _, e := range entries {
+				if e.Version != k.Version || e.Recipe != k.Recipe {
+					a.SittingOut = append(a.SittingOut, SittingOut{Node: e.Node, Version: e.Version, Recipe: e.Recipe})
+				}
+			}
+			sort.Slice(a.SittingOut, func(i, j int) bool { return a.SittingOut[i].Node < a.SittingOut[j].Node })
+			return a
 		}
 	}
 	parts := make([]string, 0, len(groups))
@@ -123,5 +148,12 @@ func AgreedToolchain(entries []NodeToolchain) (tc Toolchain2, none string) {
 		parts = append(parts, fmt.Sprintf("%s recipe %s on %s", k.Version, k.Recipe, strings.Join(nodes, ", ")))
 	}
 	sort.Strings(parts)
-	return Toolchain2{}, "builders disagree: " + strings.Join(parts, "; ")
+	return Agreement{Wait: "builders disagree: " + strings.Join(parts, "; ")}
+}
+
+// AgreedToolchain is ToolchainAgreement's toolchain, or none and why (its Wait): the version a strict majority of the
+// nodes report (novox/hq ADR 0584, which changes ADR 0405 clause 2 and ADR 0477 clause 2).
+func AgreedToolchain(entries []NodeToolchain) (tc Toolchain2, none string) {
+	a := ToolchainAgreement(entries)
+	return a.Toolchain, a.Wait
 }

go/delivery/toolchains_test.go

@@ -55,36 +55,50 @@ func TestToolchainKeyAndValue(t *testing.T) {
 	}
 }
 
-// 1443/AC2
-func TestAgreedToolchain(t *testing.T) {
-	tc, none := AgreedToolchain([]NodeToolchain{tcEntry("ace", "go", "1.26.1", "3"), tcEntry("novox", "go", "1.26.1", "3")})
-	if tc != (Toolchain2{Language: "go", Version: "1.26.1", Recipe: "3"}) || none != "" {
-		t.Errorf("%+v %q", tc, none)
-	}
-}
-
-// 1443/AC3
-func TestAgreedToolchainNoReport(t *testing.T) {
-	tc, none := AgreedToolchain(nil)
-	if tc != (Toolchain2{}) || none != "toolchain not yet reported" {
-		t.Errorf("%+v %q", tc, none)
-	}
-}
-
-// 1443/AC4
-func TestAgreedToolchainDisagree(t *testing.T) {
-	a, b, c := tcEntry("ace", "go", "1.26.1", "3"), tcEntry("novox", "go", "1.26.2", "3"), tcEntry("g14", "go", "1.26.1", "4")
-	_, first := AgreedToolchain([]NodeToolchain{a, b, c})
-	_, second := AgreedToolchain([]NodeToolchain{c, b, a})
-	if first == "" || first != second {
-		t.Errorf("order changed the answer: %q vs %q", first, second)
-	}
-	for _, want := range []string{"builders disagree", "1.26.1", "1.26.2", "ace", "novox", "g14"} {
-		if !strings.Contains(first, want) {
-			t.Errorf("%q does not name %q", first, want)
+// 1443/AC2-4, ADR 0584: the agreed toolchain is the strict majority's; a minority node sits out; a tie waits.
+func TestToolchainAgreement(t *testing.T) {
+	e := tcEntry
+	for _, c := range []struct {
+		name    string
+		in      []NodeToolchain
+		version string
+		out     []SittingOut
+		wait    []string // "" wait means agreed
+	}{
+		{"all agree", []NodeToolchain{e("ace", "go", "1.26.1", "3"), e("novox", "go", "1.26.1", "3")}, "1.26.1", nil, nil},
+		{"three against one", []NodeToolchain{e("ace", "typescript", "26.12.1", "3"), e("g14", "typescript", "26.13.0", "3"), e("novox", "typescript", "26.13.0", "3"), e("shanks", "typescript", "26.13.0", "3")},
+			"26.13.0", []SittingOut{{"ace", "26.12.1", "3"}}, nil},
+		{"two against two", []NodeToolchain{e("ace", "go", "1.26.1", "3"), e("novox", "go", "1.26.2", "3"), e("g14", "go", "1.26.1", "3"), e("shanks", "go", "1.26.2", "3")},
+			"", nil, []string{"builders disagree", "1.26.1 recipe 3 on ace, g14", "1.26.2 recipe 3 on novox, shanks"}},
+		{"one node alone", []NodeToolchain{e("ace", "go", "1.26.1", "3")}, "1.26.1", nil, nil},
+		{"no report", nil, "", nil, []string{ToolchainNoReport}},
+	} {
+		a := ToolchainAgreement(c.in)
+		rev := make([]NodeToolchain, len(c.in))
+		for i, x := range c.in {
+			rev[len(c.in)-1-i] = x
+		}
+		if b := ToolchainAgreement(rev); b.Wait != a.Wait || b.Toolchain != a.Toolchain {
+			t.Errorf("%s: order changed the answer: %+v vs %+v", c.name, a, b)
+		}
+		if a.Toolchain.Version != c.version || len(a.SittingOut) != len(c.out) {
+			t.Errorf("%s: %+v", c.name, a)
+		}
+		for i := range c.out {
+			if a.SittingOut[i] != c.out[i] {
+				t.Errorf("%s: sitting out %+v, want %+v", c.name, a.SittingOut, c.out)
+			}
+		}
+		if (len(c.wait) == 0) != (a.Wait == "") {
+			t.Errorf("%s: wait %q", c.name, a.Wait)
+		}
+		for _, w := range c.wait {
+			if !strings.Contains(a.Wait, w) {
+				t.Errorf("%s: %q does not name %q", c.name, a.Wait, w)
+			}
+		}
+		if tc, none := AgreedToolchain(c.in); tc != a.Toolchain || none != a.Wait {
+			t.Errorf("%s: AgreedToolchain %+v %q", c.name, tc, none)
 		}
 	}
-	if tc, _ := AgreedToolchain([]NodeToolchain{a, b}); tc != (Toolchain2{}) {
-		t.Errorf("a disagreement gave %+v", tc)
-	}
 }