From 524e3405e5175bbb8ade583f8ec02f1bc71435e5 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 20:32:22 +0200 Subject: [PATCH] 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. --- .../cmd/mesh-delivery/approval.go | 56 +++++++++++++++- .../mesh-delivery/cmd/mesh-delivery/events.go | 31 ++++++++- .../cmd/mesh-delivery/issue1275_test.go | 64 +++++++++++++++++++ 3 files changed, 145 insertions(+), 6 deletions(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/approval.go b/modules/mesh-delivery/cmd/mesh-delivery/approval.go index 524bdcaa..104f270a 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/approval.go +++ b/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) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/events.go b/modules/mesh-delivery/cmd/mesh-delivery/events.go index 9de213c3..ac92d25d 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/events.go +++ b/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() + } + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go index a5b0ef12..315b5337 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go +++ b/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) {