From 9fad90ee890a2097a6dd49bc28d4e08af56c5a90 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 23 Oct 2026 00:52:46 +0200 Subject: [PATCH] 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. --- .../mesh-delivery-builder/integration_test.go | 11 +-- .../cmd/mesh-delivery-builder/main.go | 40 ++++++----- .../cmd/mesh-delivery-builder/says.go | 68 ++++++++++++++++--- .../cmd/mesh-delivery-builder/says_test.go | 18 +++-- .../internal/build/r4e_test.go | 18 +++++ .../internal/build/run.go | 6 ++ 6 files changed, 118 insertions(+), 43 deletions(-) diff --git a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/integration_test.go b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/integration_test.go index da5896d..182885b 100644 --- a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/integration_test.go +++ b/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) { diff --git a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/main.go b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/main.go index b51db07..822b325 100644 --- a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/main.go +++ b/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) }() diff --git a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says.go b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says.go index 8ec0e9c..8ff0302 100644 --- a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says.go +++ b/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 } diff --git a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says_test.go b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says_test.go index 314d005..f219959 100644 --- a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says_test.go +++ b/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 { diff --git a/modules/mesh-delivery-builder/internal/build/r4e_test.go b/modules/mesh-delivery-builder/internal/build/r4e_test.go index 1c53597..d3d851c 100644 --- a/modules/mesh-delivery-builder/internal/build/r4e_test.go +++ b/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) + } +} diff --git a/modules/mesh-delivery-builder/internal/build/run.go b/modules/mesh-delivery-builder/internal/build/run.go index 9c6f23e..3021dd0 100644 --- a/modules/mesh-delivery-builder/internal/build/run.go +++ b/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) + } } }