Merge pull request 'Say which modules a walk left unchanged, so a merge that moved nothing is not read as stranded (hq issue 611)' (#295) from fix/611-an-unchanged-build-is-delivered into main

cmd/mesh-controller/batches.go

@@ -939,11 +939,16 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [
 	commits, named := carriedOf(merges)
 	var moved []string
 	var members []batchMember
+	unchanged := map[string]string{}
 	for _, m := range combinedMerges(merges) {
 		// **News, whatever was seen since** (review of this change): each merge was judged when it was heard, and
 		// a cut, a fold or a failed walk since marks its modules seen — so a merge walked alone after a failed
 		// walk, or folded, would read as history and move nothing. When it was made is not asked again here.
 		m.MergedAt = ""
+		// **Unchanged, said** (novox/hq issue 611): read before the merge is acted on, from the same records.
+		for n, why := range unchangedOfMerge(m, entries, read) {
+			unchanged[n] = why
+		}
 		names, err := movesOfMerge(ctx, inv, m, entries, read)
 		if err != nil {
 			return err
@@ -995,12 +1000,16 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [
 	plan.Delivery.Merges = named
 	plan.Delivery.Alone = batch.Delivery != nil && batch.Delivery.Alone
 	plan.Delivery.Withheld = withheld
+	plan.Delivery.Unchanged = unchangedLeft(unchanged, plan.Modules)
 	// **Its moments and its class** (novox/hq ADR 0282 decision 6): measured, never acted on.
 	plan.Times = walkTimesAtCut(*batch, plan, entries, now)
 	if len(moved) == 0 {
 		plan.State = inventory.PlanDone
 		plan.Tiers = [][]string{}
 		plan.Note = "moved nothing the mesh holds: nothing to walk"
+		if len(plan.Delivery.Unchanged) > 0 {
+			plan.Note = "moved nothing the mesh holds: nothing to walk; " + unchangedSaid(plan.Delivery.Unchanged)
+		}
 		if err := inv.SavePlan(ctx, &plan); err != nil {
 			return err
 		}
@@ -1045,6 +1054,31 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [
 	return nil
 }
 
+// unchangedSaid names the modules a walk says matched what runs, for its note.
+func unchangedSaid(unchanged map[string]string) string {
+	names := make([]string, 0, len(unchanged))
+	for n := range unchanged {
+		names = append(names, n)
+	}
+	sort.Strings(names)
+	return "unchanged, the build the mesh holds is theirs: " + strings.Join(names, ", ")
+}
+
+// unchangedLeft is what a walk says matched what runs: every module a merge left unchanged that the walk does not
+// build anyway, as another merge's module or a dependent (novox/hq issue 611). Nil when none is.
+func unchangedLeft(unchanged map[string]string, builds map[string]*inventory.PlanModule) map[string]string {
+	out := map[string]string{}
+	for n, why := range unchanged {
+		if _, built := builds[n]; !built {
+			out[n] = why
+		}
+	}
+	if len(out) == 0 {
+		return nil
+	}
+	return out
+}
+
 // withhold takes from a plan every module named that its merges do not move, and the tiers left empty: the
 // modules taken.
 func withhold(plan *inventory.Plan, names, moved []string) []string {

cmd/mesh-controller/issue611_test.go

@@ -0,0 +1,145 @@
+package main
+
+import (
+	"reflect"
+	"sort"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/novox/mesh-controller/internal/inventory"
+	"github.com/novox/mesh-controller/internal/link"
+)
+
+// novox/hq issue 611: mesh-controller #293 changed one test file. Its delivery plan, made while another walk built
+// build-agent and route-proxy (issue 606), read them whole and named them; at the cut their recorded build sources
+// held none of its files, so its walk moved nothing and its record named no module. mesh-delivery could not tell
+// "matched what runs" from "never built" and stopped the delivery, asking the operator about a change already on
+// every machine. The walk now says, per module, that its build matched what runs (delivery.unchanged, ADR 0267 §5).
+
+// The shape of #293: a test file of the controller's repository. The controller, built from the root, and the two
+// modules packaging the repository each recorded a build source holding none of it: each is said unchanged, with
+// why; a module of another repository is not named.
+func TestAMergeSaysTheModulesWhoseBuildSourceItLeftUnchanged(t *testing.T) {
+	const catalogueRepo = "http://forge.internal:20000/novox/mesh-catalog.git"
+	const controllerRepo = "http://forge.internal:20000/novox/mesh-controller.git"
+	entries := []inventory.Entry{
+		fromRepo("mesh-controller", controllerRepo, ""),
+		fromRepo("build-agent", catalogueRepo, "modules/build-agent"),
+		fromRepo("route-proxy", catalogueRepo, "modules/route-proxy"),
+		fromRepo("gitea", catalogueRepo, "modules/gitea"),
+	}
+	read := map[string][]inventory.ReadRepository{
+		"mesh-controller": {{Own: true, Paths: []string{"go.mod", "module.json", "cmd/mesh-controller/", "internal/builder/"}}},
+		"build-agent": {
+			{Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"go.mod", "cmd/mesh-builder/", "internal/builder/"}},
+			{Own: true, Paths: []string{"modules/build-agent/Dockerfile", "modules/build-agent/module.json"}},
+		},
+		"route-proxy": {
+			{Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"go.mod", "examples/route-proxy/"}},
+			{Own: true, Paths: []string{"modules/route-proxy/Dockerfile", "modules/route-proxy/module.json"}},
+		},
+	}
+	m := link.SourceMoved{Owner: "novox", Repo: "mesh-controller", Base: "main", Commit: "8f1aec0",
+		Paths: []string{"internal/builder/replay363_test.go"}}
+	if moved := reachOfMerge(m, entries, read, sharedRepositoryEdges).Moved(); len(moved) != 0 {
+		t.Fatalf("the fixture moves %v: a test file is in no build source", moved)
+	}
+	got := unchangedOfMerge(m, entries, read)
+	var names []string
+	for n, why := range got {
+		names = append(names, n)
+		if !strings.Contains(why, "holds none of") || !strings.Contains(why, "internal/builder/replay363_test.go") ||
+			!strings.Contains(why, "the build the mesh holds is its build") {
+			t.Errorf("%s is said unchanged as %q", n, why)
+		}
+	}
+	if want := []string{"build-agent", "mesh-controller", "route-proxy"}; !reflect.DeepEqual(sortedOf(names), want) {
+		t.Fatalf("said unchanged: %v; want %v", sortedOf(names), want)
+	}
+
+	// A module whose build source holds a changed file is moved, never said unchanged.
+	touched := m
+	touched.Paths = []string{"internal/builder/builder.go"}
+	if got := unchangedOfMerge(touched, entries, read); got["mesh-controller"] != "" || got["build-agent"] != "" {
+		t.Errorf("a module the merge moves is said unchanged: %v", got)
+	}
+	if got := unchangedOfMerge(touched, entries, read); got["route-proxy"] == "" {
+		t.Errorf("the route proxy, whose build source holds none of it, is not said unchanged: %v", got)
+	}
+	// With no build source recorded, the module is read whole and moved: nothing says it unchanged.
+	unsaid := map[string][]inventory.ReadRepository{"build-agent": {{Repository: "novox/mesh-controller", Ref: "main"}}}
+	if got := unchangedOfMerge(m, entries, unsaid); len(got) != 0 {
+		t.Errorf("modules with no recorded build source are said unchanged: %v", got)
+	}
+	// Files not all said: every module is moved, none said unchanged.
+	truncated := m
+	truncated.PathsTruncated = true
+	if got := unchangedOfMerge(truncated, entries, read); len(got) != 0 {
+		t.Errorf("a merge whose files were not all said says %v unchanged", got)
+	}
+}
+
+// A module the walk builds anyway — another merge's, or a dependent — is not said unchanged: the modules map keeps
+// meaning what the walk builds, and the two never name one module.
+func TestAWalkSaysUnchangedOnlyWhatItDoesNotBuild(t *testing.T) {
+	unchanged := map[string]string{"build-agent": "why a", "route-proxy": "why b"}
+	got := unchangedLeft(unchanged, map[string]*inventory.PlanModule{"build-agent": {}})
+	if !reflect.DeepEqual(got, map[string]string{"route-proxy": "why b"}) {
+		t.Fatalf("left %v", got)
+	}
+	if got := unchangedLeft(map[string]string{"x": "y"}, map[string]*inventory.PlanModule{"x": {}}); got != nil {
+		t.Fatalf("left %v; want none", got)
+	}
+}
+
+// Against the store, the path a real merge takes: a merge whose module recorded a build source that holds none
+// of its files is cut into a walk done with nothing to walk, whose record names the module unchanged and why.
+func TestAWalkThatMovedNothingSaysWhichModulesMatchedWhatRuns(t *testing.T) {
+	open := windowed(t)
+	endsItsLine(t)
+	asksRecorded(t)
+	ctx := t.Context()
+	inv := open.inventory
+	if err := inv.RecordBuild(ctx, inventory.Build{ID: "build-one", Repository: "novox/one", Ref: "main",
+		Module: "one", Commit: "c0", On: "builder", Asked: t0.Add(-time.Hour),
+		Sources: []inventory.BuildSource{{Paths: []string{"main.go", "go.mod"}}}}); err != nil {
+		t.Fatal(err)
+	}
+	readme := repoMerge("one", "one1aaaaaaaa", t0)
+	readme.Paths = []string{"README.md"}
+	hear(t, open, readme, t0.Add(time.Second))
+	cutAt(t, open, t0.Add(2*time.Minute))
+	ws, _ := walks(t, open)
+	if len(ws) != 1 {
+		t.Fatalf("walks: %+v", ws)
+	}
+	w := ws[0]
+	if w.State != inventory.PlanDone || len(w.Tiers) != 0 || len(w.Modules) != 0 {
+		t.Fatalf("the walk reads %s, tiers %v, modules %v; want done with nothing to walk", w.State, w.Tiers, w.Modules)
+	}
+	if w.Delivery == nil || !strings.Contains(w.Delivery.Unchanged["one"], "holds none of the 1 changed file(s)") {
+		t.Fatalf("the walk does not say one matched what runs: %+v", w.Delivery)
+	}
+	if !strings.Contains(w.Note, "unchanged") || !strings.Contains(w.Note, "one") {
+		t.Errorf("the walk's note does not name it: %q", w.Note)
+	}
+
+	// A merge that changes the build source is walked, and says nothing unchanged.
+	code := repoMerge("one", "one2bbbbbbbb", t0.Add(3*time.Minute))
+	hear(t, open, code, t0.Add(3*time.Minute+time.Second))
+	cutAt(t, open, t0.Add(6*time.Minute))
+	ws, _ = walks(t, open)
+	if len(ws) != 2 {
+		t.Fatalf("walks: %+v", ws)
+	}
+	if _, built := ws[0].Modules["one"]; !built || ws[0].Delivery == nil || len(ws[0].Delivery.Unchanged) != 0 {
+		t.Fatalf("the walk of a change to main.go reads modules %v, delivery %+v", ws[0].Modules, ws[0].Delivery)
+	}
+}
+
+func sortedOf(names []string) []string {
+	out := append([]string{}, names...)
+	sort.Strings(out)
+	return out
+}

cmd/mesh-controller/upgrades.go

@@ -692,9 +692,73 @@ func readsFile(e inventory.Entry, read []inventory.ReadRepository, p string) boo
 	if own := ownSource(read); own != nil {
 		return builder.SourceHolds(own, p)
 	}
+	return readsDirectory(e, p)
+}
+
+// readsDirectory is whether a changed file is in a module's directory, or anywhere for a module built from its
+// repository's root: the reading of a module with no recorded build source.
+func readsDirectory(e inventory.Entry, p string) bool {
 	return strings.Trim(e.Source.Path, "/") == "" || inside(p, e.Source.Path)
 }
 
+// unchangedOfMerge are the modules a merge changed files for under a reading without recorded build sources, but
+// whose recorded build source holds none of them, each with why (novox/hq issue 611): not built, because the build
+// the mesh holds is theirs (ADR 0267 §5). That reading — a delivery plan made while another walk built the module
+// (issue 606) — would have moved them; the walk says they matched what runs rather than leaving them unnamed.
+// A module built from the repository whose directory, or whole repository when built from its root, holds a
+// changed file, and a module packaging the repository's branch, are each read so. A merge whose changed files
+// were not all said moves every one of them: none is said here. Without acting.
+func unchangedOfMerge(m link.SourceMoved, entries []inventory.Entry,
+	read map[string][]inventory.ReadRepository) map[string]string {
+	if len(m.Paths) == 0 || m.PathsTruncated {
+		return nil
+	}
+	from, _, _ := mergeCandidates(m, entries, read)
+	touched, _, _ := touchedBy(from, entries, m, read)
+	out := map[string]string{}
+	for _, e := range from {
+		name := e.Manifest.Module
+		if ownSource(read[name]) == nil || slices.ContainsFunc(touched, func(t inventory.Entry) bool {
+			return t.Manifest.Module == name
+		}) {
+			continue
+		}
+		var in []string
+		for _, p := range m.Paths {
+			if readsDirectory(e, p) {
+				in = append(in, p)
+			}
+		}
+		if len(in) > 0 {
+			out[name] = fmt.Sprintf("its build source holds none of the %d changed file(s) in its directory (e.g. %s): "+
+				"the build the mesh holds is its build", len(in), in[0])
+		}
+	}
+	for _, e := range entries {
+		name := e.Manifest.Module
+		if sourceIs(e.Source, m) || !packagesSaid(read[name], m) || readsFrom(read[name], m) {
+			continue
+		}
+		out[name] = fmt.Sprintf("its build source in %s/%s holds none of the %d changed file(s) (e.g. %s): "+
+			"the build the mesh holds is its build", m.Owner, m.Repo, len(m.Paths), m.Paths[0])
+	}
+	if len(out) == 0 {
+		return nil
+	}
+	return out
+}
+
+// packagesSaid is whether a module's newest trunk build said the build source it read in a merge's repository and
+// branch, as a context beside its own.
+func packagesSaid(read []inventory.ReadRepository, m link.SourceMoved) bool {
+	for _, r := range read {
+		if !r.Own && sameRepository(r.Repository, m) && (r.Ref == "" || r.Ref == m.Base) && len(r.Paths) > 0 {
+			return true
+		}
+	}
+	return false
+}
+
 // staleIn is the modules whose recorded build source a plan has overtaken (novox/hq ADR 0267): a plan still
 // working that has yet to build one, or a plan made after that build which never built it — failed, stopped
 // or superseded. What such a module is built from is changing, or changed without a build to say so: a merge

internal/inventory/plans.go

@@ -220,6 +220,11 @@ type PlanDelivery struct {
 	// Withheld are, on a walk cut from a batch a stop walked merges again in, the modules it did not build: a
 	// stopped merge moves them, and built only as dependents at the branch they would deliver it (ADR 0299).
 	Withheld []string `json:"withheld,omitempty"`
+	// Unchanged are the modules a merge it answers changed files in the directory of, whose recorded build source
+	// holds none of them, each with why (novox/hq issue 611, ADR 0267 §5): not built, because the build the mesh
+	// holds is their build. A delivery plan that read them whole named them; this says they matched what runs,
+	// rather than that the walk left them unbuilt. Modules keeps meaning what the walk builds.
+	Unchanged map[string]string `json:"unchanged,omitempty"`
 	// Batch is the window of a batch; nil once it is cut into a walk.
 	Batch *PlanBatch `json:"batch,omitempty"`
 	// Alone says the walk walks one merge on its own commit, after a failed walk carried it in a later one:

testdata/store-tests

@@ -44,3 +44,4 @@
 ./cmd/mesh-controller TestReplay349
 ./cmd/mesh-controller TestReplay348AFaultFromBeforeTheSendFailsNoGate
 ./cmd/mesh-controller TestReplay239AModuleNameIsNotTakenOverByAnotherRepository
+./cmd/mesh-controller TestAWalkThatMovedNothingSaysWhichModulesMatchedWhatRuns