diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 8cc6e8f7..213e3b99 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/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 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: " +// 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" diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue902_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue902_test.go new file mode 100644 index 00000000..f2d8a43c --- /dev/null +++ b/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) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/model.go b/modules/mesh-delivery/cmd/mesh-delivery/model.go index 2d2a0c67..432e079d 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/model.go +++ b/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