End a group's ask and Approve when any member is pushed (issue 948, ADR 0398 §3)

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

@@ -229,11 +229,58 @@ func (h *Holder) covering(d *Delivery) *ReadyRun {
 	return nil
 }
 
+func headsEqual(a, b map[string]string) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for k, v := range a {
+		if b[k] != v {
+			return false
+		}
+	}
+	return true
+}
+
+// headsHold is whether an ask's heads are still the heads of d's group: every member now at the head asked, and every
+// head asked that is no member now a member already merged.
+func (h *Holder) headsHold(d *Delivery, c *ReadyRun) bool {
+	now := map[string]bool{}
+	for _, m := range h.askMembers(d) {
+		now[m.ID] = true
+		if c.Heads[m.ID] != m.Commit {
+			return false
+		}
+	}
+	for id := range c.Heads {
+		if !now[id] {
+			if o := h.deliveries[id]; o == nil || o.MergedAs == "" {
+				return false
+			}
+		}
+	}
+	return true
+}
+
+// endStaleAsks ends every ask whose heads are no longer its group's: a push to any member, marked or not, ends the
+// approval (ADR 0398 §3). Under the lock; the cancels run with the tick's asks.
+func (h *Holder) endStaleAsks() {
+	for _, d := range h.deliveries {
+		r := d.Ready
+		if r == nil || r.Merged != nil || d.MergedAs != "" || (r.AskID == "" && r.Approved == nil) || len(r.Heads) == 0 {
+			continue
+		}
+		if !h.headsHold(d, r) {
+			h.cancels = append(h.cancels, h.dropAsk(r.AskID, "the heads it covers are no longer the group's heads"))
+		}
+	}
+}
+
 // approval is what a delivery that may merge waits on for the operator: empty once its head is approved, and the ask
 // to make when none is made yet. Under the lock.
 func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) {
 	r := d.Ready
-	if c := h.covering(d); c != nil {
+	c := h.covering(d)
+	if c != nil {
 		if c.Approved != nil {
 			r.Approved, r.Warrant, r.AskID, r.Ask, r.Heads = c.Approved, c.Warrant, c.AskID, c.Ask, c.Heads
 			return "", nil
@@ -289,6 +336,9 @@ func (h *Holder) dropAsk(id, why string) func() {
 	for _, d := range h.deliveries {
 		if d.Ready != nil && d.Ready.AskID == id {
 			d.Ready.AskID, d.Ready.Ask, d.Ready.Heads = "", nil, nil
+			if d.Ready.Merged == nil {
+				d.Ready.Approved, d.Ready.Warrant = nil, ""
+			}
 			h.keep(d)
 		}
 	}

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

@@ -200,3 +200,54 @@ func TestAGroupIsAskedOnceForAllItsHeadsAndMergedInOrderOnOneApprove(t *testing.
 		t.Fatalf("the next member was not merged on the same Approve: merges %v, asks %d", w.forge.merges, w.op.count())
 	}
 }
+
+// A push to a group member without a mark ends the group's ask (ADR 0398 §3, issue 948; review round 1): the Approve
+// that follows would be for heads the group no longer has.
+func TestAPushToAnUnmarkedGroupMemberEndsTheGroupsAskAndApprove(t *testing.T) {
+	w := newWorld(t)
+	w.op.auto = false
+	const ctlHead, catHead, catPushed = "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1", "d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2", "e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3"
+	ctl, cat := IDOf("novox/mesh-controller", ctlHead), IDOf("novox/mesh-catalog", catHead)
+	w.ctl.order = func(ms []Member) Order {
+		var o []string
+		for _, m := range ms {
+			if m.Repository == "novox/mesh-controller" {
+				o = append([]string{m.ID}, o...)
+			} else {
+				o = append(o, m.ID)
+			}
+		}
+		return Order{Order: o}
+	}
+	_, _ = ctl, cat
+	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(judgedAt("novox/mesh-controller", 1, ctlHead, "pass", "t1t1t1t1t1t1"))
+	w.h.Checked(judgedAt("novox/mesh-catalog", 2, catHead, "pass", "t3t3t3t3t3t3"))
+	w.forge.open = []OpenPull{markedPull("novox/mesh-controller", 1, ctlHead, ctlHead),
+		markedPull("novox/mesh-catalog", 2, catHead, catHead)}
+	w.h.SeeOpenPulls()
+	w.settleAll()
+	w.h.Checked(CheckedEvent{Group: "feat/x", ID: "check-1", Verdict: "pass", Summary: "both compose"})
+	w.settleAll()
+	first := w.op.open()
+	if len(first) != 1 {
+		t.Fatalf("asked %v", first)
+	}
+	// The catalog member is pushed; its new head has no mark yet.
+	w.h.PullUpdated(pr("novox/mesh-catalog", 2, catPushed, "feat/x", "modules/app/module.json"))
+	w.forge.open = []OpenPull{markedPull("novox/mesh-controller", 1, ctlHead, ctlHead),
+		{Owner: "novox", Repo: "mesh-catalog", Number: 2, Base: "main", HeadSHA: catPushed}}
+	w.h.SeeOpenPulls()
+	w.settleAll()
+	if len(w.op.cancelled) != 1 || w.op.cancelled[0] != first[0] {
+		t.Fatalf("the group's ask was not cancelled by the push: %v", w.op.cancelled)
+	}
+	w.op.answer(first[0], OptionApprove) // a late answer to the cancelled ask
+	w.h.Checked(judgedAt("novox/mesh-catalog", 2, catPushed, "pass", "t3t3t3t3t3t3"))
+	w.h.Checked(CheckedEvent{Group: "feat/x", ID: "check-2", Verdict: "pass", Summary: "both compose again"})
+	w.settleAll()
+	if len(w.forge.merges) != 0 {
+		t.Fatalf("merged on an Approve of heads the group no longer has: %v", w.forge.merges)
+	}
+}

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

@@ -29,6 +29,8 @@ type Holder struct {
 	Logf       func(format string, a ...any)
 	// Operator is the operator channel, asked for the Approve of a merge into main (approval.go).
 	Operator Operator
+	// cancels are asks to take back, gathered under the lock and run with the other asks of the tick.
+	cancels []func()
 
 	mu         sync.Mutex
 	deliveries map[string]*Delivery

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

@@ -161,6 +161,7 @@ func sameFull(mark, head string) bool {
 // The asking happens outside the lock.
 func (h *Holder) readyAsks(now time.Time) []func() {
 	var asks []func()
+	h.endStaleAsks()
 	for k, m := range h.marks {
 		p, mark := m.pull, *m.pull.Ready
 		repository := strings.ToLower(p.Owner + "/" + p.Repo)
@@ -262,6 +263,8 @@ func (h *Holder) readyAsks(now time.Time) []func() {
 		h.keep(d)
 		asks = append(asks, h.askMerge(d.ID, p, mark.Head))
 	}
+	asks = append(h.cancels, asks...)
+	h.cancels = nil
 	return asks
 }