From 71b3aeaf2c742ddce6c5c658f210203c3e5ad5e4 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 12:54:11 +0200 Subject: [PATCH] Keep a group member of the other kind's batch from being overtaken (hq issue 525) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A merge's group member counted as merged when its merge waited in any batch, but kinds never share a batch: the later member got a batch of its own, cut at once, and walked before the earlier member's batch was cut, breaking the group's order (ADR 0276 §5). Only a member kept in a batch of the merge's own kind counts as merged now; one kept in the other kind's keeps the merge waiting, and its batch names that member, so the two are cut in the order their windows close (review of #250). --- cmd/mesh-controller/batches.go | 4 +- cmd/mesh-controller/group_wait.go | 54 ++++++++++++------- cmd/mesh-controller/lone_merge_test.go | 73 ++++++++++++++++++++++++-- 3 files changed, 108 insertions(+), 23 deletions(-) diff --git a/cmd/mesh-controller/batches.go b/cmd/mesh-controller/batches.go index 15cd862b..9f030a6c 100644 --- a/cmd/mesh-controller/batches.go +++ b/cmd/mesh-controller/batches.go @@ -179,7 +179,8 @@ func (f following) hearMerge(ctx context.Context, m link.SourceMoved, now time.T return nil } // **Its delivery group's word, before the plans are held** (ADR 0306 decision 2): a slow answer holds up no walk. - waits, err := groupWait(ctx, inv, m, deliverySeatHeld(entries)) + own := ownPath(touches) + waits, err := groupWait(ctx, inv, m, deliverySeatHeld(entries), own) if err != nil { return notNow(err) } @@ -208,7 +209,6 @@ func (f following) hearMerge(ctx context.Context, m link.SourceMoved, now time.T // **A merge on the controller's own path never shares a batch with one that waits for the delivery's // word** (decided during the build, 2026-10-10): batched together, the catalogue's deliveries would start // with the controller's and skip their turn. Each kind has a batch of its own. - own := ownPath(touches) if p, ok, err := answeredByALaterMerge(ctx, inv, heard, touches, own, deliverySeatHeld(entries)); err != nil { return notNow(err) } else if ok { diff --git a/cmd/mesh-controller/group_wait.go b/cmd/mesh-controller/group_wait.go index dc293819..391a9b28 100644 --- a/cmd/mesh-controller/group_wait.go +++ b/cmd/mesh-controller/group_wait.go @@ -84,14 +84,29 @@ func memberOf(s string) groupMember { } // groupWait says whether a merge heard now waits for the window for its delivery group, and why in words; nothing -// when it names no head, its head names no group, or every other member is merged or closed. A member of another -// repository whose merge this controller already keeps in a batch not yet cut is merged, whatever mesh-delivery -// says yet (it marks a member merged once its walk opened). Asked before the plans are held: a slow answer holds -// up no walk. -func groupWait(ctx context.Context, inv *inventory.Inventory, m link.SourceMoved, held bool) (string, error) { +// when it names no head, its head names no group, or every other member is merged or closed. own is the merge's +// kind (ownPath). +// +// A member of another repository whose merge this controller already keeps in a batch not yet cut of the same +// kind is merged, whatever mesh-delivery says yet (it marks a member merged once its walk opened): the merge joins +// that batch by their head. One kept in a batch of the other kind keeps the merge waiting, whatever mesh-delivery +// says (review of #250): kinds never share a batch, so the merge's batch is a new one, and cut at once it would +// walk before that member's batch is cut and break the group's order (ADR 0276 decision 5). Waiting, the two +// batches are cut in the order their windows close, an own-path one first at one look, as cutBatchesHeld cuts +// every pair of kinds. +// +// Asked before the plans are held: a slow answer holds up no walk. +func groupWait(ctx context.Context, inv *inventory.Inventory, m link.SourceMoved, held, own bool) (string, error) { if m.Head == "" { return "", nil } + same, other, err := keptFromHead(ctx, inv, m.Head, own) + if err != nil { + return "", err + } + if len(other) > 0 { + return "group " + m.Head + " has a member waiting in a batch of the other kind: " + strings.Join(other, ", "), nil + } members, isGroup, err := deliveryGroupOf(ctx, held, m.Head) if err != nil { return fmt.Sprintf("%s did not say whether group %s has a member still to merge (%v): it waits for the "+ @@ -100,13 +115,9 @@ func groupWait(ctx context.Context, inv *inventory.Inventory, m link.SourceMoved if !isGroup { return "", nil } - kept, err := keptFromHead(ctx, inv, m.Head) - if err != nil { - return "", err - } var open []string for _, g := range members { - if strings.EqualFold(g.Repository, m.Owner+"/"+m.Repo) || kept[strings.ToLower(g.Repository)] || + if strings.EqualFold(g.Repository, m.Owner+"/"+m.Repo) || same[strings.ToLower(g.Repository)] || !openPullStates[g.State] { continue } @@ -118,23 +129,30 @@ func groupWait(ctx context.Context, inv *inventory.Inventory, m link.SourceMoved return "group " + m.Head + " has a member still to merge: " + strings.Join(open, ", "), nil } -// keptFromHead are the repositories (lower case) of the merges from a head that batches not yet cut hold. -func keptFromHead(ctx context.Context, inv *inventory.Inventory, head string) (map[string]bool, error) { +// keptFromHead are the merges from a head that batches not yet cut hold: the repositories (lower case) of those in +// a batch of the kind own says, and, in words, each of the other kind with its batch. +func keptFromHead(ctx context.Context, inv *inventory.Inventory, head string, own bool) (map[string]bool, []string, error) { batches, err := inv.Batches(ctx) if err != nil { - return nil, err + return nil, nil, err } - out := map[string]bool{} + same := map[string]bool{} + var other []string for _, b := range batches { merges, err := inv.MergesOf(ctx, b.ID) if err != nil { - return nil, err + return nil, nil, err } for _, k := range merges { - if announcedOf(k).Head == head { - out[strings.ToLower(k.Repository)] = true + if announcedOf(k).Head != head { + continue + } + if b.OwnPath() == own { + same[strings.ToLower(k.Repository)] = true + } else { + other = append(other, fmt.Sprintf("%s@%s in %s", k.Repository, short(k.Commit), b.ID)) } } } - return out, nil + return same, other, nil } diff --git a/cmd/mesh-controller/lone_merge_test.go b/cmd/mesh-controller/lone_merge_test.go index 1a291896..7ab1dbc3 100644 --- a/cmd/mesh-controller/lone_merge_test.go +++ b/cmd/mesh-controller/lone_merge_test.go @@ -167,17 +167,17 @@ func TestAGroupsMergedOrClosedMembersMakeNobodyWait(t *testing.T) { deliveryGroupOf = func(context.Context, bool, string) ([]groupMember, bool, error) { return members, true, nil } t.Cleanup(func() { deliveryGroupOf = was }) m := fromHead(repoMerge("one", "c1", t0), "feat/x") - if why, err := groupWait(ctx, open.inventory, m, true); err != nil || why != "" { + if why, err := groupWait(ctx, open.inventory, m, true, false); err != nil || why != "" { t.Fatalf("the group's other members are merged or closed, yet: %q %v", why, err) } members = append(members, groupMember{Repository: "novox/four", State: "checked"}) - if why, _ := groupWait(ctx, open.inventory, m, true); !strings.Contains(why, "novox/four (checked)") { + if why, _ := groupWait(ctx, open.inventory, m, true, false); !strings.Contains(why, "novox/four (checked)") { t.Fatalf("an open member: %q", why) } // novox/four's merge is kept in a batch already: merged, whatever mesh-delivery says yet. hear(t, open, fromHead(repoMerge("two", "c4", t0), "feat/x"), t0) members[3].Repository = "novox/two" - if why, _ := groupWait(ctx, open.inventory, m, true); why != "" { + if why, _ := groupWait(ctx, open.inventory, m, true, false); why != "" { t.Fatalf("a member this controller keeps the merge of still waits: %q", why) } if got := memberOf("novox/mesh-sdk@d41d255f28ad (held)"); got != (groupMember{Repository: "novox/mesh-sdk", State: "held"}) { @@ -329,3 +329,70 @@ func TestALoneMergeRelatedToAWalkTheCutFoldsJoinsTheBatch(t *testing.T) { t.Fatalf("a lone merge related to the waiting walk: batches %+v", bs) } } + +// A delivery group with a member of each kind (review of #250): a merge on the controller's own path and a +// catalogue merge never share a batch, so the later member cannot join the earlier one's. Kept in a batch of the +// other kind, the earlier member keeps the later one waiting, whatever mesh-delivery says yet — never cut at once +// to walk before it. The two batches are then cut in the order their windows close (cutBatchesHeld: the oldest +// closed batch first, an own-path one first at one look, and the other queued behind a walk that started): the +// earlier member's window closes first, so it walks first, in both orders. +func TestAGroupAcrossBothKindsKeepsItsOrder(t *testing.T) { + for _, ownFirst := range []bool{true, false} { + name := "catalogue member first" + if ownFirst { + name = "controller member first" + } + t.Run(name, func(t *testing.T) { + open := windowed(t) + asksWithPaths(t) + ctx := t.Context() + was := deliveryGroupOf + // mesh-delivery still says both members ready: it marks one merged only once its walk opened. + deliveryGroupOf = func(context.Context, bool, string) ([]groupMember, bool, error) { + return []groupMember{{Repository: "novox/mesh-controller", State: "ready"}, + {Repository: "novox/mesh-catalog", State: "ready"}}, true, nil + } + t.Cleanup(func() { deliveryGroupOf = was }) + own := fromHead(repoMerge("mesh-controller", "k1", t0), "feat/both-kinds") + cat := fromHead(catalogueMerge("cat1aaaaaaaa", "app", t0), "feat/both-kinds") + first, second := cat, own + if ownFirst { + first, second = own, cat + } + second.MergedAt = t0.Add(10 * time.Second).Format(time.RFC3339Nano) + hear(t, open, first, t0) + hear(t, open, second, t0.Add(10*time.Second)) + ws, bs := walks(t, open) + if len(ws) != 0 || len(bs) != 2 { + t.Fatalf("both members heard: %d walk(s), %d batch(es); want two batches, nothing cut", len(ws), len(bs)) + } + later := bs[0] // newest first + if later.Delivery.Batch.CutAtOnce || !strings.Contains(later.Delivery.Batch.Waits, + "has a member waiting in a batch of the other kind: novox/"+first.Repo+"@") { + t.Fatalf("the later member's batch reads %+v; want it waiting for the earlier member", later.Delivery.Batch) + } + // The earlier member's window closes first: its batch alone is cut. + cutAt(t, open, t0.Add(90*time.Second)) + ws, bs = walks(t, open) + if len(ws) != 1 || !ws[0].Open() || ws[0].CommitOf("novox/"+first.Repo) != first.Commit || len(bs) != 1 || + bs[0].ID != later.ID { + t.Fatalf("at the earlier window: walks %+v, batches %+v; want the earlier member alone cut", ws, bs) + } + earlier := ws[0] + // The later member's window closes while the earlier walk runs: it queues behind it. + cutAt(t, open, t0.Add(100*time.Second)) + if _, bs := walks(t, open); len(bs) != 1 || bs[0].State != inventory.PlanQueued || bs[0].Delivery.Batch.Behind != earlier.ID { + t.Fatalf("the later member's batch does not queue behind the earlier walk: %+v", bs) + } + earlier.State = inventory.PlanDone + if err := open.inventory.SavePlan(ctx, &earlier); err != nil { + t.Fatal(err) + } + cutAt(t, open, t0.Add(101*time.Second)) + got, err := open.inventory.PlanByID(ctx, later.ID) + if err != nil || got.Batch() || got.CommitOf("novox/"+second.Repo) != second.Commit { + t.Fatalf("the later member was not walked after the earlier: %s %v", got.State, err) + } + }) + } +}