From 4cccd817a040753f6e216e883660e80144979148 Mon Sep 17 00:00:00 2001 From: "fix-deliv-awg@novox" Date: Fri, 16 Oct 2026 21:36:06 +0200 Subject: [PATCH] =?UTF-8?q?End=20a=20group's=20ask=20and=20Approve=20when?= =?UTF-8?q?=20any=20member=20is=20pushed=20(issue=20948,=20ADR=200398=20?= =?UTF-8?q?=C2=A73)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../cmd/mesh-delivery/approval.go | 52 ++++++++++++++++++- .../cmd/mesh-delivery/approval_test.go | 51 ++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/holder.go | 2 + .../mesh-delivery/cmd/mesh-delivery/ready.go | 3 ++ 4 files changed, 107 insertions(+), 1 deletion(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/approval.go b/modules/mesh-delivery/cmd/mesh-delivery/approval.go index 7989ddd3..41b46e18 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/approval.go +++ b/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) } } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/approval_test.go b/modules/mesh-delivery/cmd/mesh-delivery/approval_test.go index 33926550..aa1454b4 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/approval_test.go +++ b/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) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 2d40a824..42626b6a 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/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 diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go index ca66438b..9ac5f36f 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready.go +++ b/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 }