A half-merged group keeps its repositories until whole or declined, and the operator's word does not ask it again while another pass it would wait for is under way (issue 1275, review round 1)

A held half-merged group did not count as under way, so another group sharing a repository could start; asked again with delivery_merge_again, each then waited on the other. Passes are now read from every approved, undeclined Approve not merged whole (approvedMerges leaves out merged and refused members), and delivery_merge_again is refused while passWaits names a pass, naming it. The forge's mergeable is also false for a work in progress: said so.
2026-10-26 15:23:01 +01:00
parent 07ec186031
commit 82bd6f49ff
2 changed files with 148 additions and 25 deletions
@@ -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
@@ -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)
+		}
+	})
+}