Say that the builder writes references only once each build module answered one (issue 1886, ADR 0522 review)

The references come from the build modules' own pushes, so a new builder beside
an old build module writes none; saying it at start would send the controller
to keys that fail not stored.

modules/mesh-delivery-builder/cmd/mesh-delivery-builder/integration_test.go

@@ -720,10 +720,13 @@ func TestAManifestFallingBackToAPredefinedKeyIsBuilt(t *testing.T) {
 	}
 }
 
-// 1886/AC5: a builder started on n1 says under n1, in its bucket of instances, that it writes references, and takes the
-// say away when it stops: the controller delivers from its keys only while it holds.
-func TestTheStartedBuilderSaysItWritesReferencesAndStoppedSaysNothing(t *testing.T) {
+// 1886/AC5: a builder on n1 whose Go, Node and image build modules each answered a reference says under n1, in its bucket
+// of instances, that it writes references, and takes the say away when it stops: the controller delivers from its keys
+// only while it holds.
+func TestABuilderWhoseBuildModulesAnsweredReferencesSaysItAndStoppedSaysNothing(t *testing.T) {
 	b := startBed(t)
+	b.module(t, "netcheck", "catalogue-a67f0196-netcheck.module.json", "modules/netcheck")              // a Go and a TypeScript bundle
+	b.moduleCommit(t, "imaged", "catalogue-a67f0196-imaged.module.json", "modules/imaged", nil, commit) // an image, for the trunk
 	b.start(t, "n1", nil)
 	var said string
 	for deadline := time.Now().Add(20 * time.Second); time.Now().Before(deadline) && said == ""; time.Sleep(50 * time.Millisecond) {
@@ -732,7 +735,7 @@ func TestTheStartedBuilderSaysItWritesReferencesAndStoppedSaysNothing(t *testing
 		}
 	}
 	if !strings.Contains(said, `"writes":["references"]`) {
-		t.Fatalf("the started builder said %q", said)
+		t.Fatalf("the builder said %q", said)
 	}
 	b.stop()
 	if _, err := b.instances.Get(b.ctx, "n1"); !errors.Is(err, jetstream.ErrKeyNotFound) {

modules/mesh-delivery-builder/cmd/mesh-delivery-builder/main.go

@@ -178,6 +178,25 @@ func run(ctx context.Context, getenv func(string) string, logOut io.Writer) erro
 		Manifests:  manifestReader{manifests},
 		Logf:       logger.Printf,
 	}
+	// **Said once every build module answered a reference, taken away when stopped** (issue 1886, ADR 0522): the
+	// controller delivers from this builder's keys only while an instance says it writes references. A bucket not there
+	// yet (the manifest's state not made) is said, and the controller keeps asking build-agent.
+	switch kv, err := js.KeyValue(ctx, instancesBucket); {
+	case errors.Is(err, jetstream.ErrBucketNotFound):
+		logger.Printf("the bucket %s does not exist yet: the controller is not told that this builder writes references until it starts again", instancesBucket)
+	case err != nil:
+		return fmt.Errorf("the bucket %s: %w", instancesBucket, err)
+	default:
+		says := newSayer(ctx, kv, cred.Node, []string{buildrun.GoBuildModule, buildrun.NodeBuildModule, build.ImageBuildModule}, logger.Printf)
+		b.Answered = says.Answered
+		defer func() {
+			stopping, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second)
+			defer cancel()
+			if err := says.unsay(stopping); err != nil {
+				logger.Printf("%v", err)
+			}
+		}()
+	}
 	// The toolchains first, so a build that starts from the first modules value finds the nodes.
 	watch := newToolchainWatch(ctx, js, tools, b.NodesChanged, logger)
 	if err := watch.start(); err != nil {
@@ -270,27 +289,6 @@ func run(ctx context.Context, getenv func(string) string, logOut io.Writer) erro
 		return err
 	}
 	logger.Printf("started on %s", cred.Node)
-	// **Said once started, taken away when stopped** (issue 1886): the controller delivers from this builder's keys only
-	// while an instance says it writes references. A bucket not there yet (the manifest's state not made) is said, and
-	// the controller keeps asking build-agent.
-	switch kv, err := js.KeyValue(ctx, instancesBucket); {
-	case errors.Is(err, jetstream.ErrBucketNotFound):
-		logger.Printf("the bucket %s does not exist yet: the controller is not told that this builder writes references until it starts again", instancesBucket)
-	case err != nil:
-		return fmt.Errorf("the bucket %s: %w", instancesBucket, err)
-	default:
-		unsay, err := say(ctx, kv, cred.Node)
-		if err != nil {
-			return err
-		}
-		defer func() {
-			stopping, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second)
-			defer cancel()
-			if err := unsay(stopping); err != nil {
-				logger.Printf("the say in %s could not be taken away: %v", instancesBucket, err)
-			}
-		}()
-	}
 	errs := make(chan error, 2)
 	go func() { errs <- follow(ctx, w, each) }()
 	go func() { errs <- follow(ctx, cw, eachCandidate) }()

modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says.go

@@ -3,15 +3,18 @@ package main
 import (
 	"context"
 	"fmt"
+	"sync"
 
 	"github.com/nats-io/nats.go/jetstream"
 )
 
-// What the builder says it writes (novox/hq issue 1886): each running instance keeps a key, named by its node, in its
-// own bucket of instances (`mesh-delivery-builder_instances`, the manifest's state `instances`), saying that every done
-// key it writes names where its artifacts are (ADR 0498 §1). The controller delivers from the builder's keys only while
-// an instance says so, and asks build-agent as before R4e otherwise; a builder from before R4e keeps no such key. The
-// bucket is read by the controller alone, so its name and shape are not in mesh-sdk.
+// What the builder says it writes (novox/hq issue 1886, ADR 0522): each running instance keeps a key, named by its node,
+// in its own bucket of instances (`mesh-delivery-builder_instances`, the manifest's state `instances`), saying that every
+// done key it writes names where its artifacts are (ADR 0498 §1). The references come from the build modules' own pushes,
+// so an instance says it only once a step of every build module it calls answered one since it started: a builder beside
+// a build module from before R4e says nothing. The controller delivers from the builder's keys only while an instance
+// says so, and asks build-agent as before R4e otherwise. The bucket is read by the controller alone, so its name and shape
+// are not in mesh-sdk.
 const (
 	instancesBucket  = "mesh-delivery-builder_instances"
 	writesReferences = `{"v":1,"writes":["references"]}`
@@ -23,11 +26,54 @@ type sayStore interface {
 	Delete(ctx context.Context, key string, opts ...jetstream.KVDeleteOpt) error
 }
 
-// say puts this instance's key once it has started, and returns what takes it away again when it stops: a builder
-// replaced by one that does not write references leaves no say behind it.
-func say(ctx context.Context, s sayStore, node string) (unsay func(context.Context) error, err error) {
-	if _, err := s.Put(ctx, node, []byte(writesReferences)); err != nil {
-		return nil, fmt.Errorf("saying in %s that this builder writes references: %w", instancesBucket, err)
+// sayer puts this instance's say once every build module in need answered a reference, and takes it away at the stop.
+type sayer struct {
+	ctx   context.Context
+	store sayStore
+	node  string
+	need  []string
+	logf  func(string, ...any)
+
+	mu       sync.Mutex
+	answered map[string]bool
+	said     bool
+}
+
+func newSayer(ctx context.Context, store sayStore, node string, need []string, logf func(string, ...any)) *sayer {
+	return &sayer{ctx: ctx, store: store, node: node, need: need, logf: logf, answered: map[string]bool{}}
+}
+
+// Answered is told the build module whose step answered a reference (build.Builder.Answered).
+func (s *sayer) Answered(module string) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	s.answered[module] = true
+	if s.said {
+		return
 	}
-	return func(ctx context.Context) error { return s.Delete(ctx, node) }, nil
+	for _, m := range s.need {
+		if !s.answered[m] {
+			return
+		}
+	}
+	if _, err := s.store.Put(s.ctx, s.node, []byte(writesReferences)); err != nil {
+		s.logf("saying in %s that this builder writes references: %v; said again at the next reference", instancesBucket, err)
+		return
+	}
+	s.said = true
+	s.logf("every build module answered a reference: said in %s that this builder writes references", instancesBucket)
+}
+
+// unsay takes the say away, if it was made: a builder replaced by one that does not write references leaves none.
+func (s *sayer) unsay(ctx context.Context) error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	if !s.said {
+		return nil
+	}
+	if err := s.store.Delete(ctx, s.node); err != nil {
+		return fmt.Errorf("taking away the say in %s: %w", instancesBucket, err)
+	}
+	s.said = false
+	return nil
 }

modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says_test.go

@@ -20,20 +20,24 @@ func (s saysOf) Delete(_ context.Context, key string, _ ...jetstream.KVDeleteOpt
 	return nil
 }
 
-// 1886/AC5: a started builder says, under its node, that it writes references; stopped, it says nothing any more.
-func TestTheBuilderSaysItWritesReferencesWhileItRuns(t *testing.T) {
+// 1886/AC5: a builder says, under its node, that it writes references only once every build module it calls answered
+// one; stopped, it says nothing any more.
+func TestTheBuilderSaysItWritesReferencesOnceEveryBuildModuleAnsweredOne(t *testing.T) {
 	s := saysOf{}
-	unsay, err := say(t.Context(), s, "shanks")
-	if err != nil {
-		t.Fatal(err)
+	say := newSayer(t.Context(), s, "shanks", []string{"mesh-build-go", "mesh-build-node", "mesh-build-image"}, t.Logf)
+	say.Answered("mesh-build-go")
+	say.Answered("mesh-build-node")
+	if v, ok := s["shanks"]; ok {
+		t.Fatalf("said before the image build module answered a reference: %q", v)
 	}
+	say.Answered("mesh-build-image")
 	var v struct {
 		Writes []string `json:"writes"`
 	}
 	if err := json.Unmarshal([]byte(s["shanks"]), &v); err != nil || len(v.Writes) != 1 || v.Writes[0] != "references" {
-		t.Fatalf("the started builder's say: %q %v", s["shanks"], err)
+		t.Fatalf("the builder's say: %q %v", s["shanks"], err)
 	}
-	if err := unsay(t.Context()); err != nil {
+	if err := say.unsay(t.Context()); err != nil {
 		t.Fatal(err)
 	}
 	if _, ok := s["shanks"]; ok {

modules/mesh-delivery-builder/internal/build/r4e_test.go

@@ -383,3 +383,21 @@ func TestACandidatesFetchNamesTheNamedPathsTheIndexerGaveIt(t *testing.T) { // A
 		t.Errorf("fetch request = %+v, %v; want the candidate's base, the module's directory and its named paths", req, err)
 	}
 }
+
+// 1886/AC5: the builder tells who answered a reference: the build module whose step named one, and none for a step that
+// answered a digest alone (a build module from before R4e).
+func TestTheBuilderTellsWhichBuildModuleAnsweredAReference(t *testing.T) {
+	r := newRig(t, goBundle)
+	var answered []string
+	r.b.Answered = func(module string) { answered = append(answered, module) }
+	r.bm.noRef["ace/tool"] = true
+	r.consider(t, "modules/m")
+	if len(answered) != 0 {
+		t.Fatalf("a step that answered no reference was told as one: %v", answered)
+	}
+	delete(r.bm.noRef, "ace/tool")
+	r.consider(t, "modules/m")
+	if len(answered) != 1 || answered[0] != "mesh-build-go" {
+		t.Fatalf("the build module that answered a reference: %v, want mesh-build-go", answered)
+	}
+}

modules/mesh-delivery-builder/internal/build/run.go

@@ -45,6 +45,9 @@ type Builder struct {
 	// Limit is how many builds run at once; four when 0.
 	Limit int
 	Logf  func(format string, args ...any)
+	// Answered is told the build module whose step answered a reference (issue 1886): what the builder says it writes
+	// rests on it. Nil tells nobody.
+	Answered func(buildModule string)
 
 	once     sync.Once
 	sem      chan struct{}
@@ -749,6 +752,9 @@ func (b *Builder) runLane(ctx context.Context, l *lane, repository string, m del
 		if built.Reference != "" && (stored(t.Kind) || !candidate) {
 			l.references[t.Artifact] = built.Reference
 		}
+		if built.Reference != "" && b.Answered != nil {
+			b.Answered(l.module)
+		}
 	}
 }