Merge pull request 'The builder says it writes references only once its build modules proved it (issue 1886, ADR 0522)' (#23) from fix/1886-builder-says-references into main

Reviewed-on: #23

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

@@ -7,6 +7,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -155,6 +156,8 @@ type bed struct {
 	tools      map[string]jetstream.KeyValue
 	presence   jetstream.KeyValue
 	have       jetstream.KeyValue
+	instances  jetstream.KeyValue
+	stop       func() // stops the builder started last
 	fake       *fakeBuildModules
 	srvURL     string
 	dir        string
@@ -227,6 +230,7 @@ func startBed(t *testing.T) *bed {
 	b.builds = mk(delivery.BuilderBuildsBucket, delivery.BuilderBuildsHistory)
 	presence := mk(delivery.PresenceBucket, delivery.PresenceHistory)
 	b.presence, b.have = presence, mk(delivery.HaveBucket, delivery.HaveHistory)
+	b.instances = mk(instancesBucket, 1)
 	for _, node := range []string{"n1", "n2"} {
 		b.setOnline(t, node, true)
 		for _, module := range []string{"mesh-build-go", "mesh-build-node"} {
@@ -344,7 +348,9 @@ func (b *bed) start(t *testing.T, node string, env map[string]string) {
 	runCtx, stop := context.WithCancel(b.ctx)
 	done := make(chan error, 1)
 	go func() { done <- run(runCtx, func(k string) string { return e[k] }, io.Discard) }()
-	t.Cleanup(func() { stop(); <-done })
+	var once sync.Once
+	b.stop = func() { once.Do(func() { stop(); <-done }) }
+	t.Cleanup(b.stop)
 }
 
 // await reads the build key until pred holds of it, within 20 s.
@@ -713,3 +719,26 @@ func TestAManifestFallingBackToAPredefinedKeyIsBuilt(t *testing.T) {
 		t.Errorf("build = %+v, want done: the manifest is read", v)
 	}
 }
+
+// 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) {
+		if e, err := b.instances.Get(b.ctx, "n1"); err == nil {
+			said = string(e.Value())
+		}
+	}
+	if !strings.Contains(said, `"writes":["references"]`) {
+		t.Fatalf("the builder said %q", said)
+	}
+	b.stop()
+	if _, err := b.instances.Get(b.ctx, "n1"); !errors.Is(err, jetstream.ErrKeyNotFound) {
+		t.Fatalf("the stopped builder still says: %v", err)
+	}
+}

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 {

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

@@ -0,0 +1,79 @@
+package main
+
+import (
+	"context"
+	"fmt"
+	"sync"
+
+	"github.com/nats-io/nats.go/jetstream"
+)
+
+// 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"]}`
+)
+
+// sayStore is the bucket of instances as the builder writes it.
+type sayStore interface {
+	Put(ctx context.Context, key string, value []byte) (uint64, error)
+	Delete(ctx context.Context, key string, opts ...jetstream.KVDeleteOpt) error
+}
+
+// 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
+	}
+	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

@@ -0,0 +1,46 @@
+package main
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+
+	"github.com/nats-io/nats.go/jetstream"
+)
+
+type saysOf map[string]string
+
+func (s saysOf) Put(_ context.Context, key string, value []byte) (uint64, error) {
+	s[key] = string(value)
+	return 1, nil
+}
+
+func (s saysOf) Delete(_ context.Context, key string, _ ...jetstream.KVDeleteOpt) error {
+	delete(s, key)
+	return nil
+}
+
+// 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{}
+	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 builder's say: %q %v", s["shanks"], err)
+	}
+	if err := say.unsay(t.Context()); err != nil {
+		t.Fatal(err)
+	}
+	if _, ok := s["shanks"]; ok {
+		t.Fatalf("the stopped builder still says: %v", s)
+	}
+}

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)
+		}
 	}
 }
 

modules/mesh-delivery-builder/manifest_test.go

@@ -83,7 +83,7 @@ func TestManifestDeclarations(t *testing.T) { // 1607/AC1
 	if m.Module != "mesh-delivery-builder" {
 		t.Errorf("module = %q", m.Module)
 	}
-	if want := []state{{Name: "builds", History: 5}}; !reflect.DeepEqual(m.State, want) { // delivery.BuilderBuildsHistory
+	if want := []state{{Name: "builds", History: 5}, {Name: "instances", History: 1}}; !reflect.DeepEqual(m.State, want) { // delivery.BuilderBuildsHistory
 		t.Errorf("state = %+v, want %+v", m.State, want)
 	}
 	if want := []string{"mesh-build-go.steps", "mesh-build-go.toolchains", "mesh-build-image.steps", "mesh-build-node.steps", "mesh-build-node.toolchains",

modules/mesh-delivery-builder/module.json

@@ -7,7 +7,8 @@
     "mesh-build-node.toolchain.reported"
   ],
   "state": [
-    { "name": "builds", "history": 5 }
+    { "name": "builds", "history": 5 },
+    { "name": "instances", "history": 1 }
   ],
   "reads": [
     "mesh-build-go.steps",