From 38c87a7e47145dc5a53726d48c01ce4fd153f771 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 21:30:09 +0200 Subject: [PATCH] Compare a build with the one the mesh registered, not the newest, which may be one put back (review of #336, hq issue 721) --- cmd/mesh-builder/main.go | 2 +- cmd/mesh-controller/build.go | 28 +++++++++ cmd/mesh-controller/checks.go | 2 +- cmd/mesh-controller/same_program.go | 2 +- internal/builder/check_builds.go | 6 +- internal/inventory/builds.go | 46 ++++++++++++++ internal/inventory/registered_held_test.go | 72 ++++++++++++++++++++++ internal/link/build.go | 5 ++ testdata/store-tests | 1 + 9 files changed, 160 insertions(+), 4 deletions(-) create mode 100644 internal/inventory/registered_held_test.go diff --git a/cmd/mesh-builder/main.go b/cmd/mesh-builder/main.go index 67bdb002..12b6f527 100644 --- a/cmd/mesh-builder/main.go +++ b/cmd/mesh-builder/main.go @@ -329,7 +329,7 @@ func answer(ctx context.Context, publisher builder.Publisher, on, workspace stri // at build time has no reuse key anyway, so nothing is lost by refusing it here. if isRegistry && len(spec.Builds) > 0 { spec.BuildWith = &builder.CheckBuilding{Publish: publisher, Scratch: registryHere, Held: request.Held, - Seats: request.Seats, Named: namedOf(request.Check.Named)} + Running: request.Check.Running, Seats: request.Seats, Named: namedOf(request.Check.Named)} } var v builder.CheckVerdict v, err = builder.Check(building, builder.Command, spec, workspace, registry, forgeFrom(), say) diff --git a/cmd/mesh-controller/build.go b/cmd/mesh-controller/build.go index d7eb9c2d..9331dec0 100644 --- a/cmd/mesh-controller/build.go +++ b/cmd/mesh-controller/build.go @@ -899,6 +899,34 @@ type answers struct { // // Routed through the artifact store as the network reaches it now (novox/hq 04-ISSUES/102): a // base is recorded by digest and path, and a build machine needs something it can pull. +// runningBy is, keyed "/", each artifact of the build the mesh has registered for its module, as a +// builder reaches the artifact store (novox/hq issue 721): what a check compares its builds with. Empty when it cannot +// be read, said, and then nothing is said unchanged. +func runningBy(ctx context.Context) map[string]string { + open, err := openStores(ctx) + if err != nil { + fmt.Fprintf(os.Stderr, "could not read what this mesh runs, so the check compares nothing: %v\n", err) + // empty-on-error: said above; a check given nothing to compare with says no module unchanged, and all move + return nil + } + defer open.Close() + running, err := open.inventory.RegisteredHeld(ctx) + if err != nil { + fmt.Fprintf(os.Stderr, "could not read what this mesh runs, so the check compares nothing: %v\n", err) + // empty-on-error: said above; a check given nothing to compare with says no module unchanged, and all move + return nil + } + address, err := whereABuilderReachesTheStore(ctx, open.inventory) + if err != nil || address == "" { + return running + } + routed := make(map[string]string, len(running)) + for key, reference := range running { + routed[key] = catalogue.Rerouted(reference, address) + } + return routed +} + func heldBy(ctx context.Context) map[string]string { open, err := openStores(ctx) if err != nil { diff --git a/cmd/mesh-controller/checks.go b/cmd/mesh-controller/checks.go index 1c7eefda..c23ff441 100644 --- a/cmd/mesh-controller/checks.go +++ b/cmd/mesh-controller/checks.go @@ -269,7 +269,7 @@ func checkRequestFor(ctx context.Context, open *stores, p link.PullUpdated, scop Check: &link.CheckRequest{Owner: p.Owner, Repo: p.Repo, Number: p.Number, Base: p.Base, Paths: p.Paths, Beside: beside, Modules: scope.Modules, Dependents: scope.Dependents, New: scope.New, Manifests: scope.Manifests, Judge: scope.Judge, Builds: builds, Named: namedForTheCheck(ctx, p, builds), - Release: release}, + Release: release, Running: runningBy(ctx)}, }, nil } diff --git a/cmd/mesh-controller/same_program.go b/cmd/mesh-controller/same_program.go index 20645e74..20b7598d 100644 --- a/cmd/mesh-controller/same_program.go +++ b/cmd/mesh-controller/same_program.go @@ -107,7 +107,7 @@ func leaveSameAsRunning(ctx context.Context, inv *inventory.Inventory, plan *inv // empty-on-error: said above; nothing left out of the walk is the safe answer, every module moves as before return nil } - held, err := inv.Held(ctx) + held, err := inv.RegisteredHeld(ctx) if err != nil { fmt.Printf(" what the mesh holds could not be read, so every module moves: %v\n", err) // empty-on-error: said above; nothing left out of the walk is the safe answer, every module moves as before diff --git a/internal/builder/check_builds.go b/internal/builder/check_builds.go index a848a69c..fb10d051 100644 --- a/internal/builder/check_builds.go +++ b/internal/builder/check_builds.go @@ -36,6 +36,10 @@ type CheckBuilding struct { Publish Publisher Scratch Scratch Held map[string]string + // Running is each artifact of the build the mesh has registered for its module, keyed "/" + // (novox/hq issue 721): what a build for the merge is compared with — never Held's newest build, which may be one + // its gate failed and the mesh put back. + Running map[string]string Npmrc Npmrc Seats map[string]string // Named are the builds the build seat's verdicts of earlier checks of this pull request at this head name @@ -179,7 +183,7 @@ func makeForTheMerge(ctx context.Context, run Runner, spec CheckSpec, with Check if built.NotBuilt == "" { // **Compared with what runs** (novox/hq issue 721): a failed comparison says nothing, and the // module moves as before. - same, against, err := sameAsHeld(ctx, run, dir, b.Module, result.Built, with.Held) + same, against, err := sameAsHeld(ctx, run, dir, b.Module, result.Built, with.Running) switch { case err != nil: say("build", "%s could not be compared with the build the mesh holds, so it moves: %v", b.Module, err) diff --git a/internal/inventory/builds.go b/internal/inventory/builds.go index c85d9f42..8a11e13c 100644 --- a/internal/inventory/builds.go +++ b/internal/inventory/builds.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "sort" + "strings" "time" "github.com/jackc/pgx/v5" @@ -301,6 +302,51 @@ func (i *Inventory) Held(ctx context.Context) (map[string]string, error) { return held, nil } +// RegisteredHeld is Held narrowed to the build each module has registered (novox/hq issue 721): an artifact is kept +// only when its reference's digest is in the manifest the module table holds for it, which is what its machines are +// sent. After a gate failed and the mesh put the earlier build back (RestoreModule), the newest successful build is +// not what runs, and its artifacts are left out here, so nothing is compared with them. +func (i *Inventory) RegisteredHeld(ctx context.Context) (map[string]string, error) { + held, err := i.Held(ctx) + if err != nil { + return nil, err + } + rows, err := i.store.Pool().Query(ctx, `select name, coalesce(manifest::text, '') from module`) + if err != nil { + return nil, err + } + defer rows.Close() + registered := map[string]string{} + for rows.Next() { + var name, manifest string + if err := rows.Scan(&name, &manifest); err != nil { + return nil, err + } + registered[name] = manifest + } + if err := rows.Err(); err != nil { + return nil, err + } + return registeredOnly(held, registered), nil +} + +// registeredOnly keeps of held each "/" whose reference's digest the module's registered manifest +// names. +func registeredOnly(held, registered map[string]string) map[string]string { + out := map[string]string{} + for key, ref := range held { + module, _, ok := strings.Cut(key, "/") + at := strings.LastIndex(ref, "@") + if !ok || at < 0 || len(ref) <= at+1 { + continue + } + if strings.Contains(registered[module], ref[at+1:]) { + out[key] = ref + } + } + return out +} + // BuiltAgainst is what each module's newest successful build stood on, as recorded — the build // edges (ADR 0009). A module whose last build recorded no bases is absent, which is also what a // module standing on nothing looks like: an edge the mesh has not derived is not an edge. diff --git a/internal/inventory/registered_held_test.go b/internal/inventory/registered_held_test.go new file mode 100644 index 00000000..ecb86043 --- /dev/null +++ b/internal/inventory/registered_held_test.go @@ -0,0 +1,72 @@ +package inventory + +import ( + "encoding/json" + "testing" + "time" + + "github.com/novox/mesh-controller/internal/catalogue" +) + +// TestWhatRunsIsTheRegisteredBuildNotANewerOnePutBack is novox/hq issue 721's review: the newest build of a module +// failed its gate and the mesh put the build before it back. Held names the newest; what runs, and what a merge's build +// is compared with, is the build put back — so a merge whose bytes equal the failed build is never said unchanged. +func TestWhatRunsIsTheRegisteredBuildNotANewerOnePutBack(t *testing.T) { + inv := fresh(t) + ctx := t.Context() + manifestOf := func(image string) catalogue.Manifest { + return catalogue.Manifest{Module: "one", Version: "1", Resources: []map[string]any{ + {"id": "app", "type": "container", "name": "one", "image": image}}} + } + record := func(id, image string, asked time.Time) Build { + t.Helper() + raw, _ := json.Marshal(manifestOf(image)) + b := Build{ID: id, Module: "one", Commit: id + "aaaaaaaa", Repository: "novox/one", Manifest: raw, + Asked: asked, At: asked, Made: []Artifact{{Name: "one", Kind: "image", Reference: image}}} + if err := inv.RecordBuild(ctx, b); err != nil { + t.Fatal(err) + } + if err := inv.RegisterModule(ctx, manifestOf(image), Source{Repository: "novox/one", BuiltFrom: b.Commit, + Asked: asked}); err != nil { + t.Fatal(err) + } + return b + } + x := "store/one@sha256:" + "1111111111111111111111111111111111111111111111111111111111111111" + y := "store/one@sha256:" + "2222222222222222222222222222222222222222222222222222222222222222" + before := record("x", x, time.Now().Add(-2*time.Hour)) + failed := record("y", y, time.Now().Add(-time.Minute)) + if err := inv.RecordGate(ctx, GateVerdict{Build: failed.ID, Module: "one", Commit: failed.Commit, + Machines: []string{"anchor"}, Verdict: GateFailed}); err != nil { + t.Fatal(err) + } + if err := inv.RestoreModule(ctx, before); err != nil { + t.Fatal(err) + } + held, err := inv.Held(ctx) + if err != nil { + t.Fatal(err) + } + running, err := inv.RegisteredHeld(ctx) + if err != nil { + t.Fatal(err) + } + if running["one/one"] == y { + t.Fatalf("what runs is read as the build whose gate failed (%s); Held says %s", y, held["one/one"]) + } + if got := running["one/one"]; got != "" && got != x { + t.Fatalf("what runs is read as %s, want the build put back %s or nothing", got, x) + } +} + +// TestOnlyArtifactsTheRegisteredManifestNamesAreRunning: registeredOnly keeps a held artifact only when its digest is +// in its module's registered manifest. +func TestOnlyArtifactsTheRegisteredManifestNamesAreRunning(t *testing.T) { + held := map[string]string{"one/app": "store/one@sha256:aaaa", "two/app": "store/two@sha256:bbbb", "three/app": "nodigest"} + registered := map[string]string{"one": `{"resources":[{"image":"store/one@sha256:aaaa"}]}`, + "two": `{"resources":[{"image":"store/two@sha256:cccc"}]}`, "three": "nodigest"} + got := registeredOnly(held, registered) + if len(got) != 1 || got["one/app"] == "" { + t.Fatalf("running reads %v, want one/app alone", got) + } +} diff --git a/internal/link/build.go b/internal/link/build.go index d822326f..e881889a 100644 --- a/internal/link/build.go +++ b/internal/link/build.go @@ -182,6 +182,11 @@ type CheckRequest struct { // each, from the merge it judged, into the scratch namespace once it has judged and not failed, for the walk to // take. A build seat from before it builds nothing, and the walk builds as before. Builds []CheckBuild `json:"builds,omitempty"` + // Running is, keyed "/", each artifact of the build the mesh has registered for its module — the + // build its machines are sent (novox/hq issue 721): what a build for the merge is compared with, never the newest + // build, which may be one its gate failed and the mesh put back. A module absent is compared with nothing, and + // moves. A controller from before it sends none, and nothing is said unchanged. + Running map[string]string `json:"running,omitempty"` // Named are the builds the seat's verdicts of earlier checks of this pull request at this head name, within the // week check builds are kept (novox/hq issue 703): asked again because only the trunk moved, the check keeps a // build kept under a module's unchanged key, instead of building it again, only when one of these names its key diff --git a/testdata/store-tests b/testdata/store-tests index 3eac39ce..8d09e44a 100644 --- a/testdata/store-tests +++ b/testdata/store-tests @@ -73,3 +73,4 @@ ./cmd/mesh-controller TestANoMoveTierWaitsForNoNewReport ./cmd/mesh-controller TestOneProviderDownAndThreeConsumersFailingAreOneCondition ./cmd/mesh-controller TestAConsumerFailingBesideAHealthyProviderIsItsOwn +./internal/inventory TestWhatRunsIsTheRegisteredBuildNotANewerOnePutBack