From e1499d41961c2052932eb7f5581061bcd10febc7 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 13:39:21 +0200 Subject: [PATCH] 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 | 138 ++++++++++++++++++---------- cmd/mesh-controller/batches_test.go | 95 ++++++++++++++++++- cmd/mesh-controller/delivery.go | 10 ++ cmd/mesh-controller/plan_retry.go | 18 +++- cmd/mesh-controller/release_plan.go | 13 ++- internal/inventory/plans.go | 15 +++ 6 files changed, 229 insertions(+), 60 deletions(-) diff --git a/cmd/mesh-controller/batches.go b/cmd/mesh-controller/batches.go index de4c8f9b..243c5718 100644 --- a/cmd/mesh-controller/batches.go +++ b/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...) diff --git a/cmd/mesh-controller/batches_test.go b/cmd/mesh-controller/batches_test.go index 40bed716..26bf3f63 100644 --- a/cmd/mesh-controller/batches_test.go +++ b/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) + } +} diff --git a/cmd/mesh-controller/delivery.go b/cmd/mesh-controller/delivery.go index bf11ebd3..e4a017fb 100644 --- a/cmd/mesh-controller/delivery.go +++ b/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" diff --git a/cmd/mesh-controller/plan_retry.go b/cmd/mesh-controller/plan_retry.go index 3d14a556..a8377bc1 100644 --- a/cmd/mesh-controller/plan_retry.go +++ b/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 `, builds again", p.ID) + } + } if err := retryRefusal(p, plans); err != nil { return "", err } diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 20cc6af5..989d3449 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/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 } diff --git a/internal/inventory/plans.go b/internal/inventory/plans.go index 865ce77b..e43186d5 100644 --- a/internal/inventory/plans.go +++ b/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 {