mesh-delivery: say where GroupState and groupTurns still differ, keep the stopped line whole

Review of #197: a carried superseded member is reached in GroupState but
still stops the members after it in groupTurns (hq issue 445); cite ADR
0239 decision 4, which GroupState extends; shorten the stopped reason so
the member's id survives the forge's 140-character status line. Tests
pin one forge ask per pair and CoveredBy/ContainedIn across a read-back.

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

@@ -1417,8 +1417,10 @@ func (h *Holder) groupTurns(g *Group, members []*Delivery) []func() {
 		}
 		switch {
 		case d.State == Failed || d.State == Stopped || d.State == Superseded:
-			// A superseded member is an end here as in GroupState, which reads the group stopped unless a later
-			// delivered walk carried it (novox/hq issue 442).
+			// A superseded member is an end here, and it stops the members after it even when a later delivered
+			// walk carried it. GroupState counts such a member as reached (novox/hq issue 442), so for a carried
+			// member the two do not agree: the members after it are stopped all the same, and the group then reads
+			// stopped for them. That gap is novox/hq issue 445, not closed here.
 			for _, later := range ordered[i+1:] {
 				if a := h.stopInGroup(byID[later.ID], fmt.Sprintf("its group %s stopped: %s %s", g.ID, d.ID, d.State)); a != nil {
 					asks = append(asks, a)
@@ -1488,13 +1490,17 @@ func (h *Holder) stopInGroup(d *Delivery, why string) func() {
 	}
 }
 
-// GroupState is a group's state, derived from its members and its composed check — never set apart.
+// GroupState is a group's state, derived from its members and its composed check — never set apart (novox/hq
+// ADR 0239 decision 4, which lists checking, rejected, ready, delivering, delivered, failed and stopped).
 //
-// A superseded member is an end, as groupTurns has always read it: it stops the members after it. It counts as
-// reached when a later delivered walk on its trunk carried every module it moves (CoveredBy, ADR 0270's covered
-// test); a closed group whose members all ended is then delivered, and otherwise stopped, naming the member. So
-// GroupState and groupTurns agree, and a group with nothing in progress never reads delivering (novox/hq issue
-// 442: five such groups read "delivering, 1 of 2 delivered" for days).
+// It extends that list for a superseded member, which ADR 0239 does not name (novox/hq issue 442: five closed
+// groups read "delivering, 1 of 2 delivered" for days with nothing in progress). A superseded member is an end.
+// It counts as reached when a later delivered walk on its trunk carried every module it moves (CoveredBy, ADR
+// 0270's covered test); a closed group whose members all ended is then delivered, and otherwise stopped, naming
+// the member. A group with nothing in progress never reads delivering.
+//
+// groupTurns does not yet agree for a carried member: it stops the members after any superseded one, carried or
+// not, and a stopped member makes the group read stopped here. That gap is novox/hq issue 445.
 func GroupState(g *Group, members []*Delivery) (string, string) {
 	count := map[State]int{}
 	reached, ended := 0, 0
@@ -1520,8 +1526,7 @@ func GroupState(g *Group, members []*Delivery) (string, string) {
 	case n > 0 && reached == n:
 		return "delivered", "every member is delivered"
 	case g.Closed && n > 0 && ended == n:
-		return "stopped", fmt.Sprintf("a member was superseded and no later delivered merge is known to carry it: %s",
-			strings.Join(uncovered, ", "))
+		return "stopped", supersededWhy + strings.Join(uncovered, ", ")
 	case g.Closed:
 		return "delivering", fmt.Sprintf("%d of %d delivered", reached, n)
 	case len(g.Cycle) > 0:
@@ -1537,6 +1542,10 @@ func GroupState(g *Group, members []*Delivery) (string, string) {
 	return "checking", fmt.Sprintf("%d of %d member(s) ready", count[Ready], n)
 }
 
+// supersededWhy begins why a group whose members all ended is stopped. It is kept short: the forge's status line,
+// "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: "
+
 func groupStatusState(s string) string {
 	switch s {
 	case "ready", "delivered":

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

@@ -74,12 +74,12 @@ func TestAGroupWhoseMembersEndedEnds(t *testing.T) {
 			[]*Delivery{superseded(cat, "13894744a701fe59", "9d4bbbd2bd271049", checks, "gitea", "mesh-delivery"),
 				delivered(ctl, "1fdff00794e95470", "85d664438f87f885", checks),
 				later(cat, "68aeee2c00000000", "mesh-delivery")},
-			group(checks, ctlID, catID), true, "stopped", "superseded"},
+			group(checks, ctlID, catID), true, "stopped", "not carried by a later delivery: "},
 		{"superseded last, the forge says the later merge does not contain it",
 			[]*Delivery{superseded(cat, "13894744a701fe59", "9d4bbbd2bd271049", checks, "gitea", "mesh-delivery"),
 				delivered(ctl, "1fdff00794e95470", "85d664438f87f885", checks),
 				later(cat, "68aeee2c00000000", "gitea", "mesh-delivery")},
-			group(checks, ctlID, catID), false, "stopped", "superseded"},
+			group(checks, ctlID, catID), false, "stopped", "not carried by a later delivery: "},
 		{"superseded first, a later delivered walk carried every module it moves",
 			[]*Delivery{superseded(ctl, "8adb7f1a051941e9", "a5f53ef9ebdf3650", assign, "build-agent", "mesh-controller", "route-proxy"),
 				delivered(lab, "33e63ee5b47a7552", "77777777aaaaaaaa", assign),
@@ -88,7 +88,7 @@ func TestAGroupWhoseMembersEndedEnds(t *testing.T) {
 		{"superseded first, nothing later delivered",
 			[]*Delivery{superseded(ctl, "8adb7f1a051941e9", "a5f53ef9ebdf3650", assign, "build-agent", "mesh-controller", "route-proxy"),
 				delivered(lab, "33e63ee5b47a7552", "77777777aaaaaaaa", assign)},
-			group(assign, asCtlID, asLabID), true, "stopped", "superseded"},
+			group(assign, asCtlID, asLabID), true, "stopped", "not carried by a later delivery: "},
 		{"still in progress: a member delivering beside a superseded one",
 			func() []*Delivery {
 				d := delivered(ctl, "1fdff00794e95470", "85d664438f87f885", checks)
@@ -120,6 +120,48 @@ func TestAGroupWhoseMembersEndedEnds(t *testing.T) {
 			if state != c.want || !strings.Contains(why, c.whySays) {
 				t.Fatalf("the group reads %s (%s), want %s saying %q", state, why, c.want, c.whySays)
 			}
+			// The forge is asked once per pair, however many ticks follow.
+			for range 3 {
+				w.later(2 * time.Minute)
+				w.settleAll()
+			}
+			w.forge.mu.Lock()
+			asked := map[string]int{}
+			for _, a := range w.forge.containsAsks {
+				asked[a]++
+			}
+			w.forge.mu.Unlock()
+			for pair, n := range asked {
+				if n != 1 {
+					t.Fatalf("the forge was asked %d times whether %s", n, pair)
+				}
+			}
+			// What was learned survives a read-back, and the group reads the same with no new ask.
+			sup := w.delivery(c.kept[0].ID)
+			again := newWorldOver(t, store, newFakeController(), forge, w.now.Add(time.Hour))
+			if err := again.h.Reconcile(); err != nil {
+				t.Fatal(err)
+			}
+			again.settleAll()
+			back := again.delivery(c.kept[0].ID)
+			if back.CoveredBy != sup.CoveredBy || len(back.ContainedIn) != len(sup.ContainedIn) {
+				t.Fatalf("read back, covered by %q and contained in %v; was %q and %v", back.CoveredBy, back.ContainedIn,
+					sup.CoveredBy, sup.ContainedIn)
+			}
+			for k, v := range sup.ContainedIn {
+				if back.ContainedIn[k] != v {
+					t.Fatalf("read back, contained in %s is %v, was %v", k, back.ContainedIn[k], v)
+				}
+			}
+			if s2, _ := GroupState(again.h.groups[c.group.ID], again.h.membersOf(again.h.groups[c.group.ID])); s2 != state {
+				t.Fatalf("read back, the group reads %s, was %s", s2, state)
+			}
+			forge.mu.Lock()
+			n := len(forge.containsAsks)
+			forge.mu.Unlock()
+			if want := len(asked); c.want != "delivering" && n != want {
+				t.Fatalf("read back, the forge was asked again: %d asks, was %d", n, want)
+			}
 			// Each member keeps its own end: the group's label changes, no delivery is moved by hand.
 			for _, d := range c.kept {
 				if got := w.state(d.ID); got != d.State {
@@ -138,3 +180,22 @@ func TestAGroupWhoseMembersEndedEnds(t *testing.T) {
 		})
 	}
 }
+
+// The forge's mesh/delivery-group line, "group <id> stopped: <why>", is clipped at 140 characters: for each of
+// the five live groups of novox/hq issue 442, the superseded member's id fits in it whole.
+func TestAStoppedGroupsLineNamesItsSupersededMemberWhole(t *testing.T) {
+	for group, member := range map[string]string{
+		"feat/delivery-checks-verb":                 "novox/mesh-catalog@13894744a701",
+		"feat/machine-network-health":               "novox/mesh-controller@b8bbf9c79fd8",
+		"fix/373-one-message-per-condition":         "novox/mesh-catalog@8dfc19c9f7c8",
+		"fix/assign-says-why-a-module-is-not-there": "novox/mesh-controller@8adb7f1a0519",
+		"fix/needs-you-from-the-console":            "novox/mesh-controller@2e6a9b1efc05",
+	} {
+		g := &Group{ID: group, Members: []string{member, "novox/hq@39585e3bb2ea"}, Closed: true}
+		state, why := GroupState(g, []*Delivery{{ID: member, State: Superseded}, {ID: "novox/hq@39585e3bb2ea", State: Delivered}})
+		line := clip("group "+group+" "+state+": "+why, 140)
+		if state != "stopped" || !strings.HasSuffix(line, member) {
+			t.Fatalf("group %s: the line %q (%d characters) does not end in %s", group, line, len(line), member)
+		}
+	}
+}