A late merge is named however its modules read since the cut; a walk builds the commits it carries (hq ADR 0276 review)

cmd/mesh-controller/batches.go

@@ -137,7 +137,23 @@ func (f following) hearMerge(ctx context.Context, m link.SourceMoved, now time.T
 	if err != nil {
 		return notNow(err)
 	}
-	if from, packaging, already := mergeCandidates(m, entries, read); len(from) == 0 && len(packaging) == 0 {
+	merged, err := time.Parse(time.RFC3339Nano, m.MergedAt)
+	if err != nil {
+		merged = now
+	}
+	// **What it touches, history or not** (review of this change): a merge heard after a later merge of its
+	// branch was cut reads as history for every module that cut marked seen, and was dropped from the record
+	// here, never named by the walk that carried it.
+	raw := m
+	raw.MergedAt = ""
+	touches := wouldMove(raw, entries, read)
+	var later []inventory.BatchedMerge
+	if len(touches) > 0 {
+		if later, err = inv.LaterMergesOf(ctx, repository, m.Base, merged.UTC()); err != nil {
+			return notNow(err)
+		}
+	}
+	if from, packaging, already := mergeCandidates(m, entries, read); len(from) == 0 && len(packaging) == 0 && len(later) == 0 {
 		// "Already built from it" and "nothing reads it" are different facts, and reading the first
 		// as the second sends somebody looking for a broken trigger when the mesh is up to date.
 		if already > 0 {
@@ -153,10 +169,9 @@ func (f following) hearMerge(ctx context.Context, m link.SourceMoved, now time.T
 		return notNow(err)
 	}
 	defer release()
-
-	merged, err := time.Parse(time.RFC3339Nano, m.MergedAt)
-	if err != nil {
-		merged = now
+	// Again under the hold: a controller handing over may have kept it since.
+	if _, known, err := inv.MergeOf(ctx, repository, m.Commit); err != nil || known {
+		return notNow(err)
 	}
 	event, err := json.Marshal(m)
 	if err != nil {
@@ -166,7 +181,7 @@ func (f following) hearMerge(ctx context.Context, m link.SourceMoved, now time.T
 		Heard: now, Event: event}
 
 	// **A merge heard after a later merge of its branch** is answered by the walk holding that one.
-	if p, ok, err := answeredByALaterMerge(ctx, inv, heard, m, entries, read); err != nil {
+	if p, ok, err := answeredByALaterMerge(ctx, inv, heard, touches); err != nil {
 		return notNow(err)
 	} else if ok {
 		heard.Plan = p.ID
@@ -190,7 +205,7 @@ func (f following) hearMerge(ctx context.Context, m link.SourceMoved, now time.T
 		return notNow(err)
 	}
 	heard.Plan = batch.ID
-	if _, err := inv.AddMerge(ctx, heard); err != nil {
+	if added, err := inv.AddMerge(ctx, heard); err != nil || !added {
 		return notNow(err)
 	}
 	if err := keepBatch(ctx, inv, &batch, now, ""); err != nil {
@@ -207,12 +222,11 @@ func (f following) hearMerge(ctx context.Context, m link.SourceMoved, now time.T
 // later merge, which contains it. A failed or stopped walk answers nothing more, and a walk folded into
 // another is followed to that one. False when none does: the merge joins the next batch.
 func answeredByALaterMerge(ctx context.Context, inv *inventory.Inventory, heard inventory.BatchedMerge,
-	m link.SourceMoved, entries []inventory.Entry, read map[string][]inventory.ReadRepository) (inventory.Plan, bool, error) {
+	moves []inventory.Entry) (inventory.Plan, bool, error) {
 	later, err := inv.LaterMergesOf(ctx, heard.Repository, heard.Branch, heard.Merged)
 	if err != nil || len(later) == 0 {
 		return inventory.Plan{}, false, err
 	}
-	moves := wouldMove(m, entries, read)
 	for _, l := range later {
 		p, err := followTakeOver(ctx, inv, l.Plan)
 		if err != nil {
@@ -240,8 +254,12 @@ func followTakeOver(ctx context.Context, inv *inventory.Inventory, id string) (i
 	return p, err
 }
 
-// buildsEvery says a walk builds every one of the modules.
+// buildsEvery says a walk builds every one of the modules, and that there is one: a merge that moves nothing
+// is no walk's to name.
 func buildsEvery(p inventory.Plan, modules []inventory.Entry) bool {
+	if len(modules) == 0 {
+		return false
+	}
 	for _, e := range modules {
 		if _, in := p.Modules[e.Manifest.Module]; !in {
 			return false
@@ -381,6 +399,15 @@ func carriedOf(merges []inventory.BatchedMerge) ([]inventory.PlanCommit, []inven
 	return commits, named
 }
 
+// spelledOf is a kept merge's repository as the forge spelled it; empty when its announcement does not say.
+func spelledOf(m inventory.BatchedMerge) string {
+	var e link.SourceMoved
+	if json.Unmarshal(m.Event, &e) == nil && e.Owner != "" {
+		return e.Owner + "/" + e.Repo
+	}
+	return ""
+}
+
 // laterMerge says a was merged after b: by the forge's merge time, then by when each was heard.
 func laterMerge(a, b inventory.BatchedMerge) bool {
 	if !a.Merged.Equal(b.Merged) {
@@ -406,15 +433,21 @@ func nameMerges(ctx context.Context, inv *inventory.Inventory, p *inventory.Plan
 	if err != nil {
 		return err
 	}
-	_, named := carriedOf(merges)
-	// The walk's own commits stand: a merge heard late is carried by the one of its repository.
-	for i := range named {
-		if c := p.CommitOf(named[i].Repository); c != "" && c != named[i].Commit {
-			named[i].Carried = c
-		} else if c == named[i].Commit {
-			named[i].Carried = ""
+	// The walk's own commits stand: a merge heard late is carried by the one of its repository's branch.
+	var named []inventory.PlanMerge
+	for _, m := range merges {
+		n := inventory.PlanMerge{Repository: m.Repository, Commit: m.Commit}
+		if e := spelledOf(m); e != "" {
+			n.Repository = e
 		}
+		if c := p.CommitOn(m.Repository, m.Branch); c != "" && c != m.Commit {
+			n.Carried = c
+		}
+		named = append(named, n)
 	}
+	sort.SliceStable(named, func(i, j int) bool {
+		return strings.ToLower(named[i].Repository) < strings.ToLower(named[j].Repository)
+	})
 	if p.Delivery == nil {
 		p.Delivery = &inventory.PlanDelivery{}
 	}
@@ -493,7 +526,11 @@ func cutBatchesHeld(ctx context.Context, open *stores, now time.Time) error {
 	}
 	if len(alone) > 0 {
 		if len(waiting) > 0 {
-			return nil // the waiting walk is the one open; the search walks after it
+			// The waiting walk is the one open; the search walks after it, and the batch waits behind both.
+			if batch != nil {
+				return keepBatch(ctx, inv, batch, now, waiting[0].ID)
+			}
+			return nil
 		}
 		return walkAlone(ctx, open, alone[0], now)
 	}
@@ -600,6 +637,10 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [
 	var moved []string
 	var members []batchMember
 	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 = ""
 		names, err := movesOfMerge(ctx, inv, m, entries, read)
 		if err != nil {
 			return err
@@ -686,12 +727,17 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [
 }
 
 // combinedMerges is a batch's merges as one merge per repository's branch: the latest, with every file the
-// merges of it changed, removed and said to be a module's — on a linear trunk the latest contains the others.
+// merges of it changed and said to be a module's — on a linear trunk the latest contains the others. A file is
+// removed when the last merge of the batch that changed it removed it. merges are in the order they were made.
 func combinedMerges(merges []inventory.BatchedMerge) []link.SourceMoved {
 	type combined struct {
-		latest inventory.BatchedMerge
-		m      link.SourceMoved
-		said   bool
+		latest  inventory.BatchedMerge
+		m       link.SourceMoved
+		said    bool
+		removed map[string]bool
+		paths   []string
+		dirs    []string
+		cut     bool
 	}
 	by := map[string]*combined{}
 	var keys []string
@@ -703,49 +749,39 @@ func combinedMerges(merges []inventory.BatchedMerge) []link.SourceMoved {
 		k := strings.ToLower(b.Repository) + "\x00" + b.Branch
 		c, ok := by[k]
 		if !ok {
-			c = &combined{latest: b, m: e, said: e.ModuleDirsSaid}
+			c = &combined{latest: b, m: e, said: true, removed: map[string]bool{}}
 			by[k] = c
 			keys = append(keys, k)
-		} else {
-			paths, removed, dirs, truncated := c.m.Paths, c.m.Removed, c.m.ModuleDirs, c.m.PathsTruncated
-			said := c.said && e.ModuleDirsSaid
-			if laterMerge(b, c.latest) {
-				c.latest, c.m = b, e
-			}
-			c.m.Paths = union(paths, e.Paths)
-			c.m.Removed = union(removed, e.Removed)
-			c.m.ModuleDirs = union(dirs, e.ModuleDirs)
-			c.m.PathsTruncated = truncated || e.PathsTruncated
-			c.said = said
+		} else if laterMerge(b, c.latest) {
+			c.latest, c.m = b, e
+		}
+		c.said = c.said && e.ModuleDirsSaid
+		c.cut = c.cut || e.PathsTruncated
+		c.paths, c.dirs = union(c.paths, e.Paths), union(c.dirs, e.ModuleDirs)
+		for _, path := range e.Paths {
+			c.removed[path] = slices.Contains(e.Removed, path)
+		}
+		for _, path := range e.Removed {
+			c.removed[path] = true
 		}
 	}
 	sort.Strings(keys)
 	var out []link.SourceMoved
 	for _, k := range keys {
 		c := by[k]
-		c.m.ModuleDirsSaid = c.said
-		// A file a later merge of the batch brought back is no longer removed.
-		var removed []string
-		for _, r := range c.m.Removed {
-			if !restoredLater(r, c.latest, merges) {
-				removed = append(removed, r)
+		m := c.m
+		m.Paths, m.ModuleDirs, m.PathsTruncated, m.ModuleDirsSaid = c.paths, c.dirs, c.cut, c.said
+		m.Removed = nil
+		for _, path := range c.paths {
+			if c.removed[path] {
+				m.Removed = append(m.Removed, path)
 			}
 		}
-		c.m.Removed = removed
-		out = append(out, c.m)
+		out = append(out, m)
 	}
 	return out
 }
 
-// restoredLater says a file removed by one merge of a batch was changed, not removed, by the latest.
-func restoredLater(path string, latest inventory.BatchedMerge, _ []inventory.BatchedMerge) bool {
-	var e link.SourceMoved
-	if json.Unmarshal(latest.Event, &e) != nil {
-		return false
-	}
-	return slices.Contains(e.Paths, path) && !slices.Contains(e.Removed, path)
-}
-
 // union is a and b without repeats, in order.
 func union(a, b []string) []string {
 	out := append([]string{}, a...)

cmd/mesh-controller/batches_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"bytes"
+	"encoding/json"
 	"io"
 	"os"
 	"slices"
@@ -137,8 +138,8 @@ func TestTwoMergesSecondsApartAreOneWalkAtTheLaterCommit(t *testing.T) {
 	if w.Delivery == nil || !slices.Equal(w.Delivery.Merges, want) {
 		t.Fatalf("the walk answers %+v, want %+v", w.Delivery, want)
 	}
-	if len(*asked) != 2 {
-		t.Fatalf("the walk asked %v", *asked)
+	if len(*asked) != 2 || (*asked)[0][2] != dunst.Commit || (*asked)[1][2] != dunst.Commit {
+		t.Fatalf("the walk did not ask its modules at the commit it carries: %v", *asked)
 	}
 	// Heard again, as the bus may hand it over twice: never a second merge, nor a second walk.
 	hear(t, open, claude, t0.Add(5*time.Minute))
@@ -378,7 +379,7 @@ func TestALateMergeIsAnsweredByTheWalkOfTheLaterOne(t *testing.T) {
 // first; the first delivered answers the older ones, and the search stops.
 func TestAFailedWalkWalksItsEarlierMergesAlone(t *testing.T) {
 	open := windowed(t)
-	asksWithPaths(t)
+	asked := asksWithPaths(t)
 	ctx := t.Context()
 	oldest := catalogueMerge("aaaa0001", "app", t0)
 	middle := catalogueMerge("bbbb0002", "app", t0.Add(10*time.Second))
@@ -404,6 +405,14 @@ func TestAFailedWalkWalksItsEarlierMergesAlone(t *testing.T) {
 		alone.Delivery.Merges[0] != (inventory.PlanMerge{Repository: "novox/mesh-catalog", Commit: middle.Commit}) {
 		t.Fatalf("the newest earlier merge was not walked alone on its own commit: %+v", alone)
 	}
+	if _, in := alone.Modules["app"]; !in || (*asked)[len(*asked)-1] != [3]string{"novox/mesh-catalog", "modules/app",
+		middle.Commit} {
+		t.Fatalf("the merge walked alone does not build app at its own commit: %v, asked %v", alone.Modules, *asked)
+	}
+	// Retried, the failed walk would name merges the search answers now.
+	if _, err := retryPlan(ctx, open, failed.ID); err == nil || !strings.Contains(err.Error(), "walked alone") {
+		t.Fatalf("a failed walk whose merges are searched was retried: %v", err)
+	}
 	alone.State = inventory.PlanDone
 	if err := open.inventory.SavePlan(ctx, &alone); err != nil {
 		t.Fatal(err)
@@ -535,3 +544,83 @@ func TestAWaitingWalkNamesTheMergesItAnswers(t *testing.T) {
 		t.Fatalf("the waiting walk does not name both merges: %+v", got)
 	}
 }
+
+// A merge heard after a later merge of its branch was cut is named by that walk however its modules read since
+// the cut, in a repository of one module (review of this change); a walk that never built what it moves names
+// nothing, and the merge joins the next batch.
+func TestALateMergeOfAOneModuleRepositoryIsNamed(t *testing.T) {
+	open := windowed(t)
+	asksWithPaths(t)
+	ctx := t.Context()
+	c2 := repoMerge("one", "c2", t0.Add(-50*time.Second))
+	hear(t, open, c2, t0)
+	cutAt(t, open, t0.Add(2*time.Minute))
+	ws, _ := walks(t, open)
+	w := ws[0]
+	w.State = inventory.PlanDone
+	if err := open.inventory.SavePlan(ctx, &w); err != nil {
+		t.Fatal(err)
+	}
+	hear(t, open, repoMerge("one", "c1", t0.Add(-60*time.Second)), t0.Add(15*time.Minute))
+	got, _ := open.inventory.PlanByID(ctx, w.ID)
+	if len(got.Delivery.Merges) != 2 || got.Delivery.Merges[0] != (inventory.PlanMerge{Repository: "novox/one",
+		Commit: "c1", Carried: "c2"}) {
+		t.Fatalf("the late merge was not named by the walk that carried it: %+v", got.Delivery.Merges)
+	}
+
+	// A walk of two that built only two: a late merge of one is not its to name.
+	hear(t, open, repoMerge("two", "d2", t0.Add(16*time.Minute)), t0.Add(16*time.Minute))
+	cutAt(t, open, t0.Add(18*time.Minute))
+	ws, _ = walks(t, open)
+	w = ws[0]
+	delete(w.Modules, "two")
+	w.State = inventory.PlanDone
+	if err := open.inventory.SavePlan(ctx, &w); err != nil {
+		t.Fatal(err)
+	}
+	hear(t, open, repoMerge("two", "d1", t0.Add(15*time.Minute)), t0.Add(20*time.Minute))
+	if got, _ := open.inventory.PlanByID(ctx, w.ID); len(got.Delivery.Merges) != 1 {
+		t.Fatalf("a walk that never built what the late merge moves named it: %+v", got.Delivery.Merges)
+	}
+	if _, bs := walks(t, open); len(bs) != 1 || bs[0].Delivery.Merges[0].Commit != "d1" {
+		t.Fatalf("the late merge did not join the next batch: %+v", bs)
+	}
+}
+
+// One walk at a time holds against the delivery's word too: a waiting walk is not let go while another started.
+func TestAWaitingWalkIsNotLetGoBesideAStartedOne(t *testing.T) {
+	open := windowed(t)
+	asksWithPaths(t)
+	ctx := t.Context()
+	hear(t, open, repoMerge("one", "c1", t0), t0)
+	cutAt(t, open, t0.Add(2*time.Minute))
+	started, _ := walks(t, open)
+	waiting := inventory.Plan{ID: "plan-waiting", Repository: "novox/two", Branch: "main", Commit: "d1", Created: t0,
+		State: inventory.PlanBuilding, Tiers: [][]string{{"two"}}, Modules: map[string]*inventory.PlanModule{"two": {}},
+		Delivery: &inventory.PlanDelivery{Awaits: catalogue.DeliverySeat}}
+	if err := open.inventory.SavePlan(ctx, &waiting); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := letGo(ctx, open.inventory, waiting.ID, catalogue.DeliverySeat, "its turn"); err == nil ||
+		!strings.Contains(err.Error(), started[0].ID) {
+		t.Fatalf("a waiting walk was let go beside %s: %v", started[0].ID, err)
+	}
+}
+
+// A file a merge of the batch removed and a later one brought back is not removed; one removed last is.
+func TestARemovedFileIsTheLastMergesWord(t *testing.T) {
+	ev := func(commit string, paths, removed []string) inventory.BatchedMerge {
+		m := link.SourceMoved{Owner: "novox", Repo: "one", Base: "main", Commit: commit, Paths: paths, Removed: removed}
+		b, _ := json.Marshal(m)
+		return inventory.BatchedMerge{Repository: "novox/one", Branch: "main", Commit: commit, Event: b,
+			Merged: t0.Add(time.Duration(len(commit)) * time.Second)}
+	}
+	got := combinedMerges([]inventory.BatchedMerge{
+		ev("a", []string{"x/module.json", "y/module.json"}, []string{"x/module.json", "y/module.json"}),
+		ev("bb", []string{"x/module.json"}, nil),
+		ev("ccc", []string{"z.go"}, nil)})
+	if len(got) != 1 || got[0].Commit != "ccc" || !slices.Equal(got[0].Removed, []string{"y/module.json"}) ||
+		len(got[0].Paths) != 3 {
+		t.Fatalf("combined as %+v", got)
+	}
+}

cmd/mesh-controller/delivery.go

@@ -103,6 +103,16 @@ func letGo(ctx context.Context, inv *inventory.Inventory, id, by, why string) (i
 		return p, fmt.Errorf("%s was let go by %s at %s already", p.ID, p.Delivery.By,
 			p.Delivery.Go.Local().Format("15:04:05"))
 	}
+	// **One walk at a time** (novox/hq ADR 0276): a walk that started is open, so this one waits for its end.
+	open, err := inv.OpenPlans(ctx)
+	if err != nil {
+		return p, err
+	}
+	for _, q := range open {
+		if q.ID != p.ID && q.Release == nil && !q.Waiting() {
+			return p, fmt.Errorf("%s is open (%s): one walk at a time — %s is let go once it ended", q.ID, q.Named(), p.ID)
+		}
+	}
 	now := time.Now().UTC()
 	p.Delivery.Go, p.Delivery.By, p.Delivery.Why = &now, by, why
 	p.Note = "let go by " + by + "; its first tier is asked next"

cmd/mesh-controller/plan_retry.go

@@ -197,11 +197,11 @@ func retryRefusal(p inventory.Plan, plans []inventory.Plan) error {
 	return oneWalkAtATime(p, plans)
 }
 
-// oneWalkAtATime refuses a retry while another walk that started is open (novox/hq ADR 0276): a walk retried
-// beside it would be two walks at once.
+// oneWalkAtATime refuses a retry while another walk is open, started or waiting for its word (novox/hq ADR
+// 0276): a walk retried beside it would be two walks at once.
 func oneWalkAtATime(p inventory.Plan, plans []inventory.Plan) error {
 	for _, q := range plans {
-		if q.ID != p.ID && q.Open() && q.Release == nil && !q.Waiting() {
+		if q.ID != p.ID && q.Open() && q.Release == nil {
 			return fmt.Errorf("%s is open (%s): one walk at a time — retry %s once it ended", q.ID, q.Named(), p.ID)
 		}
 	}
@@ -276,6 +276,18 @@ func retryPlan(ctx context.Context, open *stores, id string) (string, error) {
 		return "", err
 	}
 	plans = append(plans, recent...)
+	// **A failed walk whose earlier merges are walked alone is not retried** (novox/hq ADR 0276): the search for
+	// the merge that brought the failure answers them now, and a retried walk would name them twice.
+	if p.Delivery != nil && len(p.Delivery.Merges) > 0 {
+		kept, err := inv.MergesOf(ctx, p.ID)
+		if err != nil {
+			return "", err
+		}
+		if len(kept) < len(p.Delivery.Merges) {
+			return "", fmt.Errorf("%s's earlier merges are walked alone, to find which one brought its failure: "+
+				"those walks answer them; a newer merge, or `rebuild <module>`, builds again", p.ID)
+		}
+	}
 	if err := retryRefusal(p, plans); err != nil {
 		return "", err
 	}

cmd/mesh-controller/release_plan.go

@@ -330,8 +330,15 @@ func askModule(ctx context.Context, p *inventory.Plan, name string, byName map[s
 	}
 	source := buildSource{Repository: e.Source.Repository, Seat: e.Source.Seat}
 	fmt.Printf("  tier %d: ", p.Tier)
-	// The branch it follows, never a commit a build once named (novox/hq 04-ISSUES/215).
-	id, err := askABuild(ctx, source, e.Source.Path, followedBranch(e.Source.Ref))
+	// The branch it follows, never a commit a build once named (novox/hq 04-ISSUES/215) — but for the commit a
+	// batch's walk carries of the module's own repository and branch (novox/hq ADR 0276): the walk builds what
+	// it answers, not a merge heard after it was cut, and a merge walked alone is built on its own commit.
+	ref := followedBranch(e.Source.Ref)
+	carried := p.CommitOn(e.Source.Repository, ref)
+	if len(p.Commits) > 0 && carried != "" {
+		ref = carried
+	}
+	id, err := askABuild(ctx, source, e.Source.Path, ref)
 	if err != nil {
 		state.State = "failed"
 		state.Why = err.Error()
@@ -341,7 +348,7 @@ func askModule(ctx context.Context, p *inventory.Plan, name string, byName map[s
 	}
 	// The commit of the module's own repository the walk carries (novox/hq ADR 0276): a batch's walk carries
 	// one per repository.
-	commit := p.CommitOf(e.Source.Repository)
+	commit := carried
 	if commit == "" {
 		commit = p.Commit
 	}

internal/inventory/plans.go

@@ -94,6 +94,21 @@ func (p Plan) CommitOf(repository string) string {
 	return ""
 }
 
+// CommitOn is the commit of a repository's branch the walk carries; empty when it carries none of it. A
+// commit kept without its branch matches any branch.
+func (p Plan) CommitOn(repository, branch string) string {
+	for _, c := range p.Commits {
+		if strings.EqualFold(c.Repository, repository) && (branch == "" || c.Branch == "" || c.Branch == branch) {
+			return c.Commit
+		}
+	}
+	if len(p.Commits) == 0 && strings.EqualFold(p.Repository, repository) &&
+		(branch == "" || p.Branch == "" || p.Branch == branch) {
+		return p.Commit
+	}
+	return ""
+}
+
 // Carried is every repository's commit the walk carries: Commits, or its one repository and commit.
 func (p Plan) Carried() []PlanCommit {
 	if len(p.Commits) > 0 {