Ask a group's Approve only when every member may merge, and withdraw the ask when a required status turns (issue 1275)

An Approve given hours after its ask must not rest on a check made at the ask: a required status
that turns not a success while the ask waits takes the ask back, and its late answer merges nothing.

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

@@ -318,24 +318,74 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) {
 	id := fmt.Sprintf("merge-%d-%s-%d", d.Number, shortOf(d.Commit, 12), r.Asks)
 	ask := composeMergeAsk(id, d, group, members, heads, now)
 	taken := []*Delivery{}
+	var reads []memberRead
 	for _, m := range members {
 		h.runOf(m, now)
 		m.Ready.AskID, m.Ready.Ask, m.Ready.Heads, m.Ready.AskedAt, m.Ready.Asks = id, &ask, heads, now, max(m.Ready.Asks, r.Asks)
 		h.keep(m)
 		taken = append(taken, m)
+		reads = append(reads, readOf(m, m.Commit))
 	}
 	h.Logf("[mesh-delivery] %s passed its checks: the operator is asked to Approve its merge into main (%s, ask %d)",
 		d.ID, id, r.Asks)
+	gid := ""
+	if group != "" {
+		gid = d.Group
+	}
 	return "the operator's Approve is asked (" + id + ")", func() {
-		if err := h.Operator.Ask(ask); err != nil {
-			h.mu.Lock()
-			defer h.mu.Unlock()
+		// A group is not asked while a member's required status is not a success at its head (issue 1275): the group
+		// says which, and is asked once every member may merge.
+		var holdMember, holdSays string
+		var readErr error
+		for _, m := range reads {
+			if gid == "" {
+				break
+			}
+			says, _, err := h.notASuccess(m)
+			if err != nil {
+				readErr = err
+				break
+			}
+			if says != "" {
+				holdMember, holdSays = m.id, says
+				break
+			}
+		}
+		undo := func() {
 			for _, m := range taken {
 				if m.Ready != nil && m.Ready.AskID == id {
 					m.Ready.AskID, m.Ready.Ask, m.Ready.Heads = "", nil, nil
 					h.keep(m)
 				}
 			}
+		}
+		if readErr != nil || holdSays != "" {
+			h.mu.Lock()
+			defer h.mu.Unlock()
+			undo()
+			if g := h.groups[gid]; g != nil && holdSays != "" {
+				if g.Hold == nil || g.Hold.Kind != "unasked" || g.Hold.Says != holdSays || g.Hold.Member != holdMember {
+					h.Logf("[mesh-delivery] group %s is not asked for its Approve: %s waits on a required status: %s", gid, holdMember, holdSays)
+					g.Hold = &GroupHold{Kind: "unasked", Member: holdMember, Says: holdSays, Since: h.Now()}
+					h.keepGroup(g)
+				}
+			}
+			if readErr != nil {
+				h.Logf("[mesh-delivery] group %s: the required statuses cannot be read, so the operator is not asked yet: %v", gid, readErr)
+			}
+			h.again("a group's Approve held for a required status")
+			return
+		}
+		if g := func() *Group { h.mu.Lock(); defer h.mu.Unlock(); return h.groups[gid] }(); g != nil && g.Hold != nil && g.Hold.Kind == "unasked" {
+			h.mu.Lock()
+			g.Hold = nil
+			h.keepGroup(g)
+			h.mu.Unlock()
+		}
+		if err := h.Operator.Ask(ask); err != nil {
+			h.mu.Lock()
+			defer h.mu.Unlock()
+			undo()
 			h.again("the operator's Approve, which could not be asked")
 			h.Logf("[mesh-delivery] the operator could not be asked about %s (%v); asked again in %s", d.ID, err, askAgainAfter)
 		}

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

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"fmt"
 	"slices"
 	"strings"
 
@@ -63,11 +64,12 @@ func (h *Holder) CommitStatus(e ForgeStatusEvent) {
 		return
 	}
 	h.wantForge()
-	if e.State != "success" {
-		return
-	}
 	h.mu.Lock()
 	defer h.mu.Unlock()
+	if e.State != "success" {
+		h.withdrawForStatus(e)
+		return
+	}
 	repository := strings.ToLower(e.Owner + "/" + e.Repo)
 	for _, d := range h.deliveries {
 		if !strings.EqualFold(d.Repository, repository) || d.Ready == nil || d.Ready.Merged != nil || d.MergedAs != "" ||
@@ -83,3 +85,26 @@ func (h *Holder) CommitStatus(e ForgeStatusEvent) {
 		}
 	}
 }
+
+// withdrawForStatus takes back the ask of an Approve nobody answered yet when a merge check of a head it covers is no
+// longer a success (issue 1275, AC5): the answer to a withdrawn ask merges nothing, and the operator is asked again once
+// every member may merge. Under the lock.
+func (h *Holder) withdrawForStatus(e ForgeStatusEvent) {
+	repository := strings.ToLower(e.Owner + "/" + e.Repo)
+	done := map[string]bool{}
+	for _, d := range h.deliveries {
+		r := d.Ready
+		if !strings.EqualFold(d.Repository, repository) || r == nil || r.AskID == "" || r.Approved != nil || r.Declined != nil || done[r.AskID] {
+			continue
+		}
+		head := d.Commit
+		if sha := r.Heads[d.ID]; sha != "" {
+			head = sha
+		}
+		if sameCommit(head, e.SHA) {
+			done[r.AskID] = true
+			h.cancels = append(h.cancels, h.withdrawAsk(r.AskID, fmt.Sprintf("%s of %s is %s at its head %s", e.Context, d.ID, e.State, shortOf(head, 12))))
+			h.Poke()
+		}
+	}
+}

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

@@ -141,6 +141,70 @@ func TestAHalfMergedGroupStaysRaisedUntilEveryMemberIsMerged(t *testing.T) {
 	}
 }
 
+// 1275/AC1: a group with a member whose required status is not a success is not asked its Approve; the group says which
+// member and status; it is asked once every member may merge.
+func TestAGroupIsNotAskedWhileAMembersRequiredStatusIsNotASuccess(t *testing.T) {
+	w := newWorld(t)
+	w.op.auto = false
+	w.h.PullUpdated(pr(gCtl, 1, gHc, "feat/g", "internal/a.go"))
+	w.h.PullUpdated(pr(gCat, 2, gHk, "feat/g", "modules/app/module.json"))
+	w.h.Checked(judged(gCtl, 1, gHc, "pass", gTrunk0))
+	w.h.Checked(judged(gCat, 2, gHk, "pass", gTrunk0))
+	w.settleAll()
+	g := w.h.groups["feat/g"]
+	setChecks(w, gCat, gHk, "success", "success") // the composed verdict set every member's gate
+	setChecks(w, gCtl, gHc, "success", "pending")
+	w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed",
+		Members: []struct {
+			Owner  string `json:"owner"`
+			Repo   string `json:"repo"`
+			Commit string `json:"commit"`
+			Trunk  string `json:"trunk,omitempty"`
+		}{{Owner: "novox", Repo: "mesh-controller", Commit: gHc, Trunk: gTrunk0[:12]},
+			{Owner: "novox", Repo: "mesh-catalog", Commit: gHk, Trunk: gTrunk0[:12]}}})
+	w.settleAll()
+	if w.op.count() != 0 {
+		t.Fatalf("the operator was asked with a required status pending: %d asks", w.op.count())
+	}
+	if g.Hold == nil || g.Hold.Kind != "unasked" || g.Hold.Member != IDOf(gCtl, gHc) || !strings.Contains(g.Hold.Says, "mesh/repo-check (pending)") {
+		t.Fatalf("the group does not say which member and status it waits for: %+v", g.Hold)
+	}
+	setChecks(w, gCtl, gHc, "success", "success")
+	w.later(time30s)
+	w.settleAll()
+	if w.op.count() != 1 || g.Hold != nil {
+		t.Fatalf("asks %d, hold %+v; wanted the group asked once its statuses are green", w.op.count(), g.Hold)
+	}
+}
+
+// 1275/AC5: a member's required status turning not-success while the ask waits withdraws the ask at the router; the
+// answer to the withdrawn ask merges nothing; the group is asked again once every member may merge.
+func TestAStatusThatTurnsWhileTheAskWaitsWithdrawsItAndItsAnswerMergesNothing(t *testing.T) {
+	w := approvedGroup(t)
+	allGreen(w)
+	ask := w.op.open()[0]
+	w.h.CommitStatus(ForgeStatusEvent{Owner: "novox", Repo: "mesh-catalog", SHA: gHk, Context: repoCheck, State: "pending"})
+	setChecks(w, gCat, gHk, "success", "pending")
+	w.settleAll()
+	if len(w.op.cancelled) != 1 || w.op.cancelled[0] != ask {
+		t.Fatalf("the ask was not withdrawn at the router: %v", w.op.cancelled)
+	}
+	w.op.answer(ask, OptionApprove) // an answer that arrives late
+	w.settleAll()
+	if len(w.forge.merges) != 0 {
+		t.Fatalf("the answer to a withdrawn ask merged: %v", w.forge.merges)
+	}
+	if w.op.count() != 1 {
+		t.Fatalf("asked again while a status was still pending: %d", w.op.count())
+	}
+	setChecks(w, gCat, gHk, "success", "success")
+	w.later(time30s)
+	w.settleAll()
+	if w.op.count() != 2 {
+		t.Fatalf("not asked again once every member may merge: %d", w.op.count())
+	}
+}
+
 // 1276/AC1: given a closed group whose member's merge was refused with "not a success: mesh/repo-check (missing)", then
 // the member's own check is asked, not the composed check.
 func TestAClosedGroupsMemberRefusedForItsOwnMissingStatusAsksItsOwnCheck(t *testing.T) {