Merge pull request 'A delivering group says success on mesh/delivery-group, so its later members can merge (issue 902)' (#329) from fix/902-group-member-merges-while-delivering into main

modules/mesh-delivery/cmd/mesh-delivery/holder.go

@@ -1847,11 +1847,18 @@ func (h *Holder) planGroup(g *Group, members []*Delivery, now time.Time) []func(
 		}
 	}
 	// Its state, said when it changes.
-	if state, why := GroupState(g, members); state != g.Said {
-		g.Said = state
+	state, why := GroupState(g, members)
+	status := groupStatusState(state)
+	if state == "delivering" && !(g.Check != nil && (g.Check.Verdict == "pass" || g.Check.Verdict == "warning") &&
+		sameMembers(g.Check.Commits, commitsOf(members)) && g.Check.OutOfDate == "") {
+		// Success there lets the merge tool merge its members: only for a composed check that passed for these heads.
+		status = "pending"
+	}
+	if state != g.Said || status != g.SaidStatus {
+		g.Said, g.SaidStatus = state, status
 		for _, d := range members {
 			g.Owed = append(g.Owed, Effect{Kind: EffectStatus, Commit: d.Commit, Context: "mesh/delivery-group",
-				State: groupStatusState(state), Line: clip(fmt.Sprintf("group %s %s: %s", g.ID, state, why), 140),
+				State: status, Line: clip(fmt.Sprintf("group %s %s: %s", g.ID, state, why), 140),
 				Since: now, Last: d.Repository + "#" + fmt.Sprint(d.Number) + " " + d.HTMLURL})
 		}
 		g.Owed = append(g.Owed, Effect{Kind: EffectEmit, Line: string(mustJSON(map[string]any{"group": g.ID,
@@ -2626,6 +2633,9 @@ func GroupState(g *Group, members []*Delivery) (string, string) {
 			return "checking", g.Check.OutOfDate
 		}
 		return "checking", "composed again for its members' new heads"
+	case g.Closed && g.Check != nil && g.Check.Verdict != "pass" && g.Check.Verdict != "warning":
+		// Composed again for new heads once it was delivering, and those do not compose: its members wait (issue 902).
+		return "rejected", "composed together: " + g.Check.Summary
 	case g.Closed:
 		return "delivering", fmt.Sprintf("%d of %d delivered", reached, n)
 	case len(g.Cycle) > 0:
@@ -2648,9 +2658,12 @@ func GroupState(g *Group, members []*Delivery) (string, string) {
 // "group <id> stopped: <why>", is clipped at 140 characters, and the member's id must fit in it whole.
 const supersededWhy = "superseded, not carried by a later delivery: "
 
+// groupStatusState is the forge status mesh/delivery-group takes for a group's state. A delivering group passed its composed check for these very heads (GroupState reads checking while it is out of
+// date), so it is a success too: its members after the first merge only once mesh/delivery-group is one at their
+// heads (novox/hq ADR 0382), and a pending one there would hold them for ever (novox/hq issue 902).
 func groupStatusState(s string) string {
 	switch s {
-	case "ready", "delivered":
+	case "ready", "delivering", "delivered":
 		return "success"
 	case "rejected", "failed":
 		return "failure"

modules/mesh-delivery/cmd/mesh-delivery/issue902_test.go

@@ -0,0 +1,67 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// novox/hq issue 902: a group that is delivering passed its composed check for its members' heads, so
+// mesh/delivery-group says success there; pending would keep its later members from ever merging.
+func TestReplay902(t *testing.T) {
+	for state, want := range map[string]string{
+		"ready": "success", "delivering": "success", "delivered": "success",
+		"checking": "pending", "rejected": "failure", "failed": "failure", "stopped": "error",
+	} {
+		if got := groupStatusState(state); got != want {
+			t.Errorf("a group %s says %s on mesh/delivery-group, want %s", state, got, want)
+		}
+	}
+}
+
+// group902 is a two-member group whose first member in its order merged.
+func group902(t *testing.T) (*world, string, string, string, string) {
+	w := newWorld(t)
+	const ctlHead, catHead = "c1c1c1c1c1c1c1c1", "d2d2d2d2d2d2d2d2"
+	ctl, cat := IDOf("novox/mesh-controller", ctlHead), IDOf("novox/mesh-catalog", catHead)
+	w.ctl.order = func(ms []Member) Order { return Order{Order: []string{cat, ctl}} }
+	w.h.PullUpdated(pr("novox/mesh-controller", 1, ctlHead, "feat/x", "internal/a.go"))
+	w.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json"))
+	w.h.Checked(verdict("novox/mesh-controller", 1, ctlHead, "pass"))
+	w.h.Checked(verdict("novox/mesh-catalog", 2, catHead, "pass"))
+	w.settleAll()
+	w.h.Checked(CheckedEvent{Group: "feat/x", ID: "check-1", Verdict: "pass", Summary: "both compose"})
+	w.settleAll()
+	w.h.PullMerged(merged("novox/mesh-catalog", 2, catHead, "m2m2m2m2m2m2"))
+	w.settleAll()
+	return w, ctl, cat, ctlHead, catHead
+}
+
+func TestReplay902TheNextMemberOfADeliveringGroupReadsSuccess(t *testing.T) {
+	w, _, _, ctlHead, _ := group902(t)
+	g := w.h.groups["feat/x"]
+	st, _ := GroupState(g, w.h.membersOf(g))
+	s := w.forge.statuses["novox/mesh-controller@"+ctlHead+" mesh/delivery-group"]
+	t.Logf("state=%s status=%q", st, s)
+	if st != "delivering" || !strings.HasPrefix(s, "success") {
+		t.Fatalf("want delivering/success, got %s %q", st, s)
+	}
+}
+
+func TestReplay902AClosedGroupComposedAgainAndFailingSaysNoSuccess(t *testing.T) {
+	w, ctl, _, _, _ := group902(t)
+	const newHead = "e3e3e3e3e3e3e3e3"
+	_ = ctl
+	w.h.PullUpdated(pr("novox/mesh-controller", 1, newHead, "feat/x", "internal/a.go"))
+	w.h.Checked(verdict("novox/mesh-controller", 1, newHead, "pass"))
+	w.settleAll()
+	t.Logf("checks asked: %v", w.ctl.checks)
+	w.h.Checked(CheckedEvent{Group: "feat/x", ID: "check-2", Verdict: "fail", Summary: "no longer compose"})
+	w.settleAll()
+	g := w.h.groups["feat/x"]
+	st, why := GroupState(g, w.h.membersOf(g))
+	s := w.forge.statuses["novox/mesh-controller@"+newHead+" mesh/delivery-group"]
+	t.Logf("closed=%v check=%+v state=%s (%s) status=%q", g.Closed, g.Check, st, why, s)
+	if strings.HasPrefix(s, "success") {
+		t.Fatalf("a closed group whose composed check failed for its heads says success: %q", s)
+	}
+}

modules/mesh-delivery/cmd/mesh-delivery/model.go

@@ -876,8 +876,11 @@ type Group struct {
 	// Closed is set once a member reached the trunk: a group delivering takes no new member.
 	Closed bool `json:"closed,omitempty"`
 	// Said is the derived state last said, so a change of it is said once.
-	Said string   `json:"said,omitempty"`
-	Owed []Effect `json:"owed,omitempty"`
+	Said string `json:"said,omitempty"`
+	// SaidStatus is the mesh/delivery-group status last written for it: a change of the status a state takes is
+	// written too, though the state did not change (novox/hq issue 902).
+	SaidStatus string   `json:"said_status,omitempty"`
+	Owed       []Effect `json:"owed,omitempty"`
 }
 
 // Pair is one "before" among a group's members: the rule that won, and the rules that ordered it the other