diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 6aeac8a8..360746eb 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/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 stopped: ", 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": diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue442_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue442_test.go index 45d8402d..2863aca4 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/issue442_test.go +++ b/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 stopped: ", 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) + } + } +}