From 4b8b1c047cfa471e8a73b645b8eddfb3f89384d6 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 23 Oct 2026 00:48:28 +0200 Subject: [PATCH 1/2] Say in the builder's own bucket of instances that it writes references (issue 1886) The controller delivers from the builder's keys only while a running builder says its done keys name references; a builder from before R4e says nothing, so R4e's own delivery is asked of build-agent instead of waiting for ever. The say is put once started and taken away when stopped. --- .../mesh-delivery-builder/integration_test.go | 28 ++++++++++++- .../cmd/mesh-delivery-builder/main.go | 21 ++++++++++ .../cmd/mesh-delivery-builder/says.go | 33 +++++++++++++++ .../cmd/mesh-delivery-builder/says_test.go | 42 +++++++++++++++++++ .../mesh-delivery-builder/manifest_test.go | 2 +- modules/mesh-delivery-builder/module.json | 3 +- 6 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says.go create mode 100644 modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says_test.go 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 5ae38b7..da5896d 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 @@ -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,23 @@ func TestAManifestFallingBackToAPredefinedKeyIsBuilt(t *testing.T) { t.Errorf("build = %+v, want done: the manifest is read", v) } } + +// 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) { + b := startBed(t) + 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 started 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) + } +} 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 40b6e29..b51db07 100644 --- a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/main.go +++ b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/main.go @@ -270,6 +270,27 @@ 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 new file mode 100644 index 0000000..8ec0e9c --- /dev/null +++ b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says.go @@ -0,0 +1,33 @@ +package main + +import ( + "context" + "fmt" + + "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. +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 +} + +// 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) + } + return func(ctx context.Context) error { return s.Delete(ctx, node) }, 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 new file mode 100644 index 0000000..314d005 --- /dev/null +++ b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/says_test.go @@ -0,0 +1,42 @@ +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 started builder says, under its node, that it writes references; stopped, it says nothing any more. +func TestTheBuilderSaysItWritesReferencesWhileItRuns(t *testing.T) { + s := saysOf{} + unsay, err := say(t.Context(), s, "shanks") + if err != nil { + t.Fatal(err) + } + 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) + } + if err := unsay(t.Context()); err != nil { + t.Fatal(err) + } + if _, ok := s["shanks"]; ok { + t.Fatalf("the stopped builder still says: %v", s) + } +} diff --git a/modules/mesh-delivery-builder/manifest_test.go b/modules/mesh-delivery-builder/manifest_test.go index 3fd061d..6f13144 100644 --- a/modules/mesh-delivery-builder/manifest_test.go +++ b/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", diff --git a/modules/mesh-delivery-builder/module.json b/modules/mesh-delivery-builder/module.json index 460ae22..f18e708 100644 --- a/modules/mesh-delivery-builder/module.json +++ b/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", From 9fad90ee890a2097a6dd49bc28d4e08af56c5a90 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 23 Oct 2026 00:52:46 +0200 Subject: [PATCH 2/2] 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) + } } }