diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/group_merge.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/group_merge.go index ade98c17..8b2fd6a2 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/group_merge.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/group_merge.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "sort" "strings" "time" ) @@ -63,8 +64,8 @@ func (h *Holder) notASuccess(m memberRead) (says string, forge bool, err error) } if p.Mergeable != nil && !*p.Mergeable { base := firstOf(p.Base, m.base) - return fmt.Sprintf("the forge says it does not merge into %s now (in conflict with it, or still working that out "+ - "after %s moved)", base, base), true, nil + return fmt.Sprintf("the forge says it does not merge into %s now (in conflict with it, a work in progress, or still "+ + "being worked out after %s moved)", base, base), true, nil } } return "", false, nil @@ -164,62 +165,93 @@ func (h *Holder) groupMergeable(gid, target string, reads []memberRead) bool { // passWaits is why an approved merge is not handed yet because another approved merge's pass is under way into a // repository it goes into; empty when none is (issue 1275, 2026-10-26 13:40: group feat/890-placement merged // mesh-delivery#72 between the members of fix/734-lost-ask, whose mesh-delivery#73 then conflicted). A pass is under way -// from its first member handed until every member is merged, or it stopped (a member refused, or the group held). The -// first merge handed has the repositories; the other is handed once that pass ends, on the event that ends it. This holds -// whether or not mesh/check gates the merge: it waits for a merge of seconds, never for a check (ADR 0551). Under the lock. +// from its first member handed until every member is merged; one half merged stays under way while it is refused or held, +// until it is whole, declined, or its remaining members are gone or asked anew (a new head is a new Approve). The first +// merge handed has the repositories; the other is handed once that pass ends, on the event that ends it. This holds +// whether or not mesh/check gates the merge: it waits for a merge, never for a check (ADR 0551). Under the lock. func (h *Holder) passWaits(d *Delivery) string { c := h.covering(d) if c == nil || c.Approved == nil || c.AskID == "" { return "" } mine := h.unmergedRepos(c) - for _, am := range h.approvedMerges() { - if am.run.AskID == c.AskID || !h.passUnderWay(am.run) { + for _, p := range h.passes() { + if p.run.AskID == c.AskID || !h.passUnderWay(p.run) { continue } - for repo := range h.unmergedRepos(am.run) { + for repo := range h.unmergedRepos(p.run) { if mine[repo] { return fmt.Sprintf("approved; waits for the merge of %s into %s to end, so no other merge lands there between "+ - "its members (issue 1275)", firstOf(am.group, am.owner.ID), repo) + "its members (issue 1275)", p.name, repo) } } } return "" } -// unmergedRepos are the repositories (lowercase) of the deliveries an Approve covers that are not merged. Under the lock. +// pass is an approved merge not merged whole: the run holding its Approve and the name of its group (or pull request). +type pass struct { + run *ReadyRun + name string +} + +// passes are the approved merges not merged whole and not declined, each ask once. Unlike approvedMerges it keeps one whose +// members merged or were refused: a half-merged pass still holds its repositories (issue 1275). Under the lock. +func (h *Holder) passes() []pass { + ids := make([]string, 0, len(h.deliveries)) + for id := range h.deliveries { + ids = append(ids, id) + } + sort.Strings(ids) + seen := map[string]bool{} + var out []pass + for _, id := range ids { + d := h.deliveries[id] + r := d.Ready + if r == nil || r.Approved == nil || r.Declined != nil || r.AskID == "" || len(r.Heads) == 0 || seen[r.AskID] || + d.Base != MergeBranch || h.allMerged(r.Heads) { + continue + } + seen[r.AskID] = true + out = append(out, pass{run: r, name: askGroup(d)}) + } + return out +} + +// unmergedRepos are the repositories (lowercase) of the deliveries an Approve covers that are not merged and may still be +// merged on it: not over (closed, held), at the head approved, and covered by this Approve, not a newer one. Under the lock. func (h *Holder) unmergedRepos(r *ReadyRun) map[string]bool { out := map[string]bool{} - for id := range r.Heads { - if m := h.deliveries[id]; m != nil && m.MergedAs == "" && (m.Ready == nil || m.Ready.Merged == nil) { - out[strings.ToLower(m.Repository)] = true + for id, sha := range r.Heads { + m := h.deliveries[id] + if m == nil || m.MergedAs != "" || (m.Ready != nil && m.Ready.Merged != nil) || m.State.Final() || !sameCommit(m.Commit, sha) { + continue } + if c := h.covering(m); c == nil || c.AskID != r.AskID { + continue // asked anew (its new head, or a new Approve): that ask is the pass now + } + out[strings.ToLower(m.Repository)] = true } return out } // passUnderWay is whether an approved merge's pass has started and not ended: a member is handed to the bridge, or some -// are merged and the rest neither refused nor held. Under the lock. +// are merged and others may still merge on this Approve, refused or held or not (issue 1275, review: a held half-merged +// group that let another group start, then asked again, waited on that group while it waited on it). Under the lock. func (h *Holder) passUnderWay(r *ReadyRun) bool { - merged, stopped := false, false + if len(h.unmergedRepos(r)) == 0 { + return false + } for id := range r.Heads { m := h.deliveries[id] if m == nil { continue } - switch { - case m.MergedAs != "" || (m.Ready != nil && m.Ready.Merged != nil): - merged = true - case m.Ready != nil && m.Ready.merging: + if m.MergedAs != "" || (m.Ready != nil && m.Ready.Merged != nil) || (m.Ready != nil && m.Ready.merging) { return true - case m.Ready != nil && m.Ready.RefusedSince != nil: - stopped = true - } - if g := h.groups[m.Group]; m.Group != "" && g != nil && g.Hold != nil && g.Hold.Kind == "blocked" { - stopped = true } } - return merged && !stopped + return false } // groupLines are the groups that are held or half merged, as urgent stalled lines the gate says as conditions (conditions.go): @@ -323,6 +355,9 @@ func (h *Holder) MergeAgain(id, why string) (string, error) { if !held && !refused { return "", fmt.Errorf("%s was neither refused nor held: %s", id, firstOf(d.readyWords(), "it is merged at its turn")) } + if waits := h.passWaits(d); waits != "" { + return "", fmt.Errorf("%s is not asked again yet: it %s; it is asked at that pass's end", id, strings.TrimPrefix(waits, "approved; ")) + } for hid := range c.Heads { if m := h.deliveries[hid]; m != nil && m.Ready != nil && m.Ready.Merged == nil { m.Ready.Repeats, m.Ready.LastTry, m.Ready.merging = 0, time.Time{}, false diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1275_whole_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1275_whole_test.go index bd76b8e4..8271fdef 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1275_whole_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1275_whole_test.go @@ -257,3 +257,91 @@ func refusedPartWayAskedAgainOnTheOperatorsWord(t *testing.T) { t.Fatalf("a merged delivery is asked again: %v", err) } } + +// approveSecondGroup makes a second group of these members on branch, checked, composed together and approved; the world +// already holds group feat/g. +func approveSecondGroup(t *testing.T, w *world, branch string, ms ...member) { + t.Helper() + for _, m := range ms { + w.h.PullUpdated(pr(m.repo, m.number, m.head, branch, strings.Split(m.paths, ",")...)) + w.h.Checked(judged(m.repo, m.number, m.head, "pass", gTrunk0)) + } + allGreenFor(w, ms...) + w.settleAll() + g := w.h.groups[branch] + if g == nil || g.Check == nil { + t.Fatalf("no composed check was asked for %s: %+v", branch, g) + } + c := CheckedEvent{Group: branch, ID: g.Check.Asked, Verdict: "pass", Summary: "composed"} + for _, m := range ms { + owner, repo, _ := strings.Cut(m.repo, "/") + c.Members = append(c.Members, struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + Commit string `json:"commit"` + Trunk string `json:"trunk,omitempty"` + }{Owner: owner, Repo: repo, Commit: m.head, Trunk: gTrunk0[:12]}) + } + w.h.Checked(c) + w.settleAll() + for _, key := range w.op.open() { + if strings.Contains(key, strings.ReplaceAll(branch, "/", "_")) || strings.Contains(key, branch) { + w.op.answer(key, OptionApprove) + } + } +} + +// A group held half merged keeps its repositories until it is whole or declined: another group sharing one waits, and the +// operator's word that asks the held group again never leaves the two waiting on each other (issue 1275, review round 1: +// a2 waited for group B, b2 waited for group A, for ever). And the operator's word is refused while another pass it would +// wait for is under way, naming it. +func TestAHeldHalfMergedGroupKeepsItsRepositoriesAndNoTwoPassesWaitOnEachOther(t *testing.T) { + groupA := []member{lostAsk[0], lostAsk[2]} // mesh-sdk#92, then mesh-delivery#73 + groupB := []member{lostAsk[1], {wDel, 72, wH72, "modules/mesh-delivery-checker/go.mod"}} + t.Run("half merged and held: the other group waits, and both merge once the held one is asked again", func(t *testing.T) { + w := approvedGroupOf(t, groupA...) + allGreenFor(w, groupA...) + w.forge.refuseRepo = map[string]error{wDel: errors.New(forgeLater)} + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + if len(w.forge.merges) != 2 { + t.Fatalf("the case: mesh-sdk#92 merged, mesh-delivery#73 refused: %v", w.forge.merges) + } + approveSecondGroup(t, w, "feat/890-placement", groupB...) + w.settleAll() + if len(w.forge.merges) != 2 { + t.Fatalf("group feat/890-placement merged while feat/g is half merged into a repository both go into: %v", w.forge.merges) + } + w.forge.refuseRepo = nil + if _, err := w.h.MergeAgain(IDOf(wDel, wH73), "the forge said try again later"); err != nil { + t.Fatal(err) + } + w.settleAll() + want := []string{"mesh-sdk#92", "mesh-delivery#73", "mesh-delivery#73", "mesh-controller#472", "mesh-delivery#72"} + if len(w.forge.merges) != len(want) { + t.Fatalf("merged %v, wanted %v: the held group first, then the other", w.forge.merges, want) + } + for i, m := range want { + if !strings.Contains(w.forge.merges[i], m+"@") { + t.Fatalf("merged %v, wanted %v in that order", w.forge.merges, want) + } + } + }) + t.Run("the operator's word is refused while another pass it would wait for is under way", func(t *testing.T) { + w := approvedGroupOf(t, groupA...) + allGreenFor(w, groupA...) + w.forge.refuseRepo = map[string]error{wSDK: errors.New(forgeLater)} // refused first: nothing of feat/g merged + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + w.merges.manual = true + approveSecondGroup(t, w, "feat/890-placement", groupB...) + w.settleAll() + if got := handedWords(handing(w)); got != "novox/mesh-controller#472" { + t.Fatalf("the other group's pass did not start: handed %q", got) + } + _, err := w.h.MergeAgain(IDOf(wSDK, wH92), "asked again") + if err == nil || !strings.Contains(err.Error(), "feat/890-placement") { + t.Fatalf("asked again while a pass it waits for is under way: %v", err) + } + }) +}