Compare a build with the one the mesh registered, not the newest, which may be one put back (review of #336, hq issue 721)

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 "<module>/<artifact>" 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.

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