From 6e6d1f3e47f4e12afc6dc6b8e69401743af16e9f Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 20:32:15 +0200 Subject: [PATCH 1/3] Merge a group whole or not at all: re-read every required status first, stop at the first refusal, say a held or half merged group urgent (issue 1275, 1276) An approved group was merged member by member, each on its own, so a refusal of the first did not stop the second. A group now merges in one pass in its order, after every member's required statuses are read again at its head, and a held or half merged group stays raised. A refusal naming the member's own repository check as missing asks that check (issue 1276). --- .../cmd/mesh-delivery/fakes_test.go | 18 +- .../cmd/mesh-delivery/group_merge.go | 220 ++++++++++++++++++ .../cmd/mesh-delivery/group_test.go | 15 +- .../mesh-delivery/cmd/mesh-delivery/holder.go | 5 +- .../cmd/mesh-delivery/issue1275_test.go | 159 +++++++++++++ .../mesh-delivery/cmd/mesh-delivery/model.go | 12 + .../mesh-delivery/cmd/mesh-delivery/ready.go | 129 ++++++---- .../mesh-delivery/cmd/mesh-delivery/verbs.go | 3 + 8 files changed, 497 insertions(+), 64 deletions(-) create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/group_merge.go create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go index c2f355bc..e04f0196 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go @@ -504,17 +504,17 @@ func (f *fakeForge) Statuses(owner, repo string, number int, commit, base string f.asked = append(f.asked, fmt.Sprintf("%s/%s#%d@%s base=%s", owner, repo, number, commit, base)) out := &ForgeChecks{} if number > 0 { - p, ok := f.pulls[fmt.Sprintf("%s/%s#%d", owner, repo, number)] - if !ok { + if p, ok := f.pulls[fmt.Sprintf("%s/%s#%d", owner, repo, number)]; ok { + out.Pull = &p + if commit == "" { + commit = p.HeadSHA + } + if base == "" { + base = p.Base + } + } else if commit == "" { return nil, fmt.Errorf("no pull request %d", number) } - out.Pull = &p - if commit == "" { - commit = p.HeadSHA - } - if base == "" { - base = p.Base - } } for key, said := range f.statuses { at, context, _ := strings.Cut(key, " ") diff --git a/modules/mesh-delivery/cmd/mesh-delivery/group_merge.go b/modules/mesh-delivery/cmd/mesh-delivery/group_merge.go new file mode 100644 index 00000000..6ebc8b31 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/group_merge.go @@ -0,0 +1,220 @@ +package main + +import ( + "fmt" + "strings" + "time" +) + +// A group is merged whole or not at all, in its order (issue 1275, novox/hq ADR 0418). +// +// - Before the operator is asked, and again before any member is merged, every member's required statuses are read at +// its head on the forge; one that is not a success holds the group, and nothing is merged. +// - The members are merged one after the other in the group's order, and the pass stops at the first refusal: no later +// member is merged. +// - A group held or half merged is said as a stalled line, urgent, until every member is merged or a person decides. + +// memberRead is what a status read of one member needs, taken under the lock. +type memberRead struct { + id, owner, repo, base, head string + number int +} + +// readOf is a delivery as a status read needs it, at the head the Approve or the ask covers. +func readOf(d *Delivery, head string) memberRead { + if head == "" { + head = d.Commit + } + return memberRead{id: d.ID, owner: d.Owner(), repo: d.Repo(), base: d.Base, head: head, number: d.Number} +} + +// notAsSuccess reads a member's required statuses at its head and says, in the forge's words, which are not a success; +// empty when all are. ownOnly is whether only the member's own repository check is missing or pending. An error is a +// read that could not be made: the member then counts as not mergeable. +func (h *Holder) notASuccess(m memberRead) (says string, ownOnly bool, err error) { + fc, err := h.Forge.Statuses(m.owner, m.repo, m.number, m.head, m.base) + if err != nil { + return "", false, err + } + if fc.Merge.Mergeable == nil { + return "", false, fmt.Errorf("the forge could not say whether %s may merge: %s", m.id, fc.Merge.Says) + } + if *fc.Merge.Mergeable { + return "", false, nil + } + var parts []string + ownOnly = len(fc.Merge.Blocking) > 0 + for _, b := range fc.Merge.Blocking { + parts = append(parts, fmt.Sprintf("%s (%s)", b.Context, b.State)) + if b.Context != ctxRepo || (b.State != "pending" && b.State != "missing") { + ownOnly = false + } + } + return fmt.Sprintf("%s requires %s; not a success: %s", fc.Merge.Branch, strings.Join(fc.Merge.Required, ", "), + strings.Join(parts, ", ")), ownOnly, nil +} + +// unmergedLive are a group's members not yet merged and not over, in the group's order. Under the lock. +func (h *Holder) unmergedLive(g *Group) []*Delivery { + var out []*Delivery + for _, d := range h.membersOf(g) { + if d.MergedAs == "" && !d.State.Final() && (d.Ready == nil || d.Ready.Merged == nil) { + out = append(out, d) + } + } + return out +} + +// groupPass is the one pass that merges a group's members, made by the first of them in the group's order that may be +// merged; nil when it is not d's turn, or a member is not approved at its head. Under the lock; the pass runs outside it. +func (h *Holder) groupPass(g *Group, d *Delivery, now time.Time) func() { + order := h.unmergedLive(g) + if len(order) == 0 || order[0] != d { + return nil + } + var pass []passMember + for _, m := range order { + c := h.covering(m) + if m.Ready == nil || m.Ready.Declined != nil || c == nil || c.Approved == nil || c.Heads[m.ID] != m.Commit || h.readyWaits(m) != "" { + return nil // the others are not approved at their heads yet: nothing is merged before they are + } + pass = append(pass, passMember{read: readOf(m, c.Heads[m.ID]), heads: c.Heads}) + } + for _, m := range order { + m.Ready.Tries++ + m.Ready.LastTry, m.Ready.merging = now, true + h.keep(m) + } + gid := g.ID + return func() { h.mergeGroupPass(gid, pass) } +} + +// passMember is one member of a pass as it was taken under the lock. +type passMember struct { + read memberRead + heads map[string]string +} + +// mergeGroupPass reads every member's required statuses, and only when all are a success merges the members in order, +// stopping at the first the merge tool refuses. +func (h *Holder) mergeGroupPass(gid string, pass []passMember) { + release := func(from int, retry bool) { + for _, p := range pass[from:] { + if d := h.deliveries[p.read.id]; d != nil && d.Ready != nil { + d.Ready.merging = false + if retry { + d.Ready.LastTry = time.Time{} + } + } + } + } + for _, p := range pass { + says, ownOnly, err := h.notASuccess(p.read) + if err == nil && says == "" { + continue + } + h.mu.Lock() + if err != nil { + h.Logf("[mesh-delivery] group %s: the required statuses of %s cannot be read, so nothing is merged yet: %v", gid, p.read.id, err) + release(0, true) + h.again("a group's merge whose required statuses could not be read") + h.mu.Unlock() + return + } + var own func() + if g := h.groups[gid]; g != nil { + if g.Hold == nil || g.Hold.Kind != "blocked" || g.Hold.Says != says || g.Hold.Member != p.read.id { + h.Logf("[mesh-delivery] group %s is approved and NOT merged: %s at %s: %s", gid, p.read.id, shortOf(p.read.head, 12), says) + } + since := time.Time{} + if g.Hold != nil && g.Hold.Kind == "blocked" { + since = g.Hold.Since + } + if since.IsZero() { + since = h.Now() + } + g.Hold = &GroupHold{Kind: "blocked", Member: p.read.id, Says: says, Since: since} + h.keepGroup(g) + } + if d := h.deliveries[p.read.id]; ownOnly && d != nil && d.Ready != nil { + own = h.askOwnAfterRefusal(d) // its own repository check is what is missing: asked, once for the head + } + release(0, true) + h.again("a group's merge held for a required status") + h.mu.Unlock() + if own != nil { + own() + } + return + } + h.mu.Lock() + if g := h.groups[gid]; g != nil && g.Hold != nil { + g.Hold = nil + h.keepGroup(g) + } + h.mu.Unlock() + for i, p := range pass { + if h.mergeOne(p.read.id, p.heads) { + continue + } + h.mu.Lock() + release(i+1, false) + if g := h.groups[gid]; g != nil { + h.Logf("[mesh-delivery] group %s: the merge of %s was refused, so none of the members after it is merged", gid, p.read.id) + h.keepGroup(g) + } + h.mu.Unlock() + return + } +} + +// groupLines are the groups that are held or half merged, as urgent stalled lines the controller raises as conditions: +// one for each, on its first member not merged, until every member is merged or a person decides (issue 1275). A member +// the merge tool refused is said by readyLines, with its group; this adds a group held for a required status, and a +// group half merged with no refusal. Under the lock. +func (h *Holder) groupLines(now time.Time) []StalledLine { + var out []StalledLine + for _, g := range h.groups { + live := h.unmergedLive(g) + if len(live) == 0 { + continue + } + first, refused, running, merged := live[0], false, false, 0 + for _, d := range live { + if d.Ready != nil && (d.Ready.RefusedSince != nil || d.Ready.Failed != "" || d.Ready.Refused != "") { + refused = true + } + if d.Ready != nil && d.Ready.merging { + running = true + } + } + for _, d := range h.membersOf(g) { + if d.MergedAs != "" || (d.Ready != nil && d.Ready.Merged != nil) { + merged++ + } + } + switch { + case g.Hold != nil && g.Hold.Kind == "blocked" && !running: + out = append(out, StalledLine{ID: g.Hold.Member, Number: numberOf(h.deliveries[g.Hold.Member]), State: first.State, + For: now.Sub(g.Hold.Since).Round(time.Second).String(), Bound: "0s", Severity: "urgent", + H2: "none: the required status turns success, which asks the merge again — or the operator's decision", + Says: fmt.Sprintf("group %s is approved and nothing is merged (merged %d of %d): %s waits on a required status "+ + "at its head: %s. Needed: that status turns success (its check asked again), or the operator decides.", + g.ID, merged, len(h.membersOf(g)), g.Hold.Member, g.Hold.Says)}) + case merged > 0 && !refused && !running: + out = append(out, StalledLine{ID: first.ID, Number: first.Number, State: first.State, + For: now.Sub(g.Updated).Round(time.Second).String(), Bound: "0s", Severity: "urgent", + H2: "none: every member merged, or the operator decides", + Says: fmt.Sprintf("group %s is half merged (%d of %d members merged): %s is not merged. Needed: it merges "+ + "(its Approve and checks hold at its head), or the operator decides.", g.ID, merged, len(h.membersOf(g)), first.ID)}) + } + } + return out +} + +func numberOf(d *Delivery) int { + if d == nil { + return 0 + } + return d.Number +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go index c55936fd..952c40f9 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go @@ -89,25 +89,26 @@ func TestAnApprovedGroupWhoseTrunkMovedIsCheckedAgainAndMergedOnTheSameApprove(t } } -// ADR 0418 §4: a member the forge refuses is merged again on its fresh check, while the other merged in the same pass. -func TestAMemberTheForgeRefusesIsMergedAgainWhileTheOtherMergedInTheSamePass(t *testing.T) { +// ADR 0418 §4, issue 1275/AC3: a member the forge refuses stops the pass (the members after it are not asked), and is +// merged again on its fresh check, the others after it in the same pass. +func TestAMemberTheForgeRefusesStopsThePassAndIsMergedAgainOnItsFreshCheck(t *testing.T) { w := approvedGroup(t) w.forge.refuse = errAskedAgain w.op.answer(w.op.open()[0], OptionApprove) w.settleAll() - if len(w.forge.merges) != 2 { - t.Fatalf("both members are asked in the same pass: %v", w.forge.merges) + if len(w.forge.merges) != 1 { + t.Fatalf("the pass stops at the first refusal: %v", w.forge.merges) } w.forge.refuse = nil w.later(2 * time.Minute) w.settleAll() - if len(w.forge.merges) != 2 { + if len(w.forge.merges) != 1 { t.Fatalf("a refused member is asked again with no event, on a timer or at each look: %v", w.forge.merges) } w.h.Checked(CheckedEvent{Group: "feat/g", ID: w.h.groups["feat/g"].Check.Asked, Verdict: "pass", Summary: "composed again"}) w.settleAll() - if len(w.forge.merges) != 4 { - t.Fatalf("a refused member is asked again on the next composed verdict: %v", w.forge.merges) + if len(w.forge.merges) != 3 { + t.Fatalf("a refused member is asked again on the next composed verdict, then the next: %v", w.forge.merges) } if d := w.delivery(IDOf(gCat, gHk)); d.Ready.Merged == nil { t.Fatalf("not merged: %+v", d.Ready) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 41aaf4aa..362436c6 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -2670,11 +2670,12 @@ func refusedAfter(d *Delivery, at time.Time) bool { } // ownStatusPending is whether the merge tool refused a merge because a status of the member's own merge check is -// pending while its gate is not: the forge's "not a success: mesh/repo-check (pending)" (gitea merge.ts, mergeableUnder). +// pending or missing (issue 1276) while its gate is not: the forge's "not a success: mesh/repo-check (pending)" (gitea merge.ts, mergeableUnder). // A refusal that also names mesh/merge-gate is the group's composed check's to answer, as is a verdict out of date. func ownStatusPending(answer string) bool { _, listed, ok := strings.Cut(answer, "not a success:") - return ok && strings.Contains(listed, "mesh/repo-check (pending)") && !strings.Contains(listed, "mesh/merge-gate") + return ok && (strings.Contains(listed, "mesh/repo-check (pending)") || strings.Contains(listed, "mesh/repo-check (missing)")) && + !strings.Contains(listed, "mesh/merge-gate") } // askOwnAfterRefusal asks a member's own check once for its head, after the merge tool refused it for a pending status of diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go new file mode 100644 index 00000000..a5b0ef12 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go @@ -0,0 +1,159 @@ +package main + +import ( + "errors" + "strings" + "testing" + "time" +) + +const time30s = 31 * time.Second + +// Issue 1275 (the 2026-10-19 18:05 group): a group is merged whole or not at all, in its order, and a group that is held or +// half merged is said loudly. Issue 1276/AC1: a refusal naming the member's own repository check as missing asks that check. + +const ( + gate, repoCheck = "mesh/merge-gate", "mesh/repo-check" +) + +// statusOf sets a member head's two merge checks on the forge, and makes both required. +func setChecks(w *world, repository, sha, gateState, repoState string) { + w.forge.required = []string{gate, repoCheck} + for ctx, state := range map[string]string{gate: gateState, repoCheck: repoState} { + key := repository + "@" + sha + " " + ctx + if state == "" { + delete(w.forge.statuses, key) // no status at all: the forge says it is missing + continue + } + w.forge.statuses[key] = state + " said → x" + } +} + +func allGreen(w *world) { + setChecks(w, gCat, gHk, "success", "success") + setChecks(w, gCtl, gHc, "success", "success") +} + +func refusedWords(repository string) error { + return errors.New("the forge refused gitea_merge_pull_request: " + repository + " is not merged: main requires " + + "mesh/merge-gate, mesh/repo-check; not a success: mesh/repo-check (missing). A pull request merges once its checks succeed") +} + +func urgentLines(w *world) []StalledLine { + var out []StalledLine + for _, l := range w.h.Stalled() { + if l.Severity == "urgent" { + out = append(out, l) + } + } + return out +} + +// 1275/AC2: an approved group whose member has a required status that is not a success at its head when the merge starts +// is merged not at all, though the first member in order is fine; the group says which member and which status. +func TestAnApprovedGroupWithAMemberWhoseRequiredStatusIsNotASuccessMergesNothing(t *testing.T) { + w := approvedGroup(t) + allGreen(w) + w.op.answer(w.op.open()[0], OptionApprove) + setChecks(w, gCtl, gHc, "success", "") // the second member's repository check went missing after the ask + w.settleAll() + if len(w.forge.merges) != 0 { + t.Fatalf("merged with a required status missing: %v", w.forge.merges) + } + lines := urgentLines(w) + if len(lines) != 1 || !strings.Contains(lines[0].Says, "feat/g") || !strings.Contains(lines[0].Says, IDOf(gCtl, gHc)) || + !strings.Contains(lines[0].Says, "mesh/repo-check (missing)") || !strings.Contains(lines[0].Says, "Needed") { + t.Fatalf("the held group is not said as one urgent line naming group, member, status and need: %+v", lines) + } + // The status turns success: the whole group merges. + setChecks(w, gCtl, gHc, "success", "success") + w.h.CommitStatus(ForgeStatusEvent{Owner: "novox", Repo: "mesh-controller", SHA: gHc, Context: repoCheck, State: "success"}) + w.later(time30s) + w.settleAll() + if len(w.forge.merges) != 2 || len(urgentLines(w)) != 0 { + t.Fatalf("merges %v, lines %+v; wanted both merged and no line", w.forge.merges, urgentLines(w)) + } +} + +// 1275/AC2 with 1276/AC1: a member whose own repository check is the missing status has that check asked, once. +func TestAHeldGroupWhoseMemberLacksItsOwnRepositoryCheckAsksThatCheck(t *testing.T) { + w := approvedGroup(t) + allGreen(w) + w.op.answer(w.op.open()[0], OptionApprove) + setChecks(w, gCtl, gHc, "success", "") + asked := len(w.ctl.checks) + w.settleAll() + w.later(time30s) + w.settleAll() + own, composed := ownChecks(w, asked) + if len(composed) != 0 || len(own) != 1 || own[0] != ":"+IDOf(gCtl, gHc) { + t.Fatalf("own checks %v, composed %v; wanted the member's own check once", own, composed) + } +} + +// 1275/AC3: the merge tool refuses the first member in order; the second is not merged, and the group is said urgent, +// naming the group, the refused member and the forge's words. +func TestARefusedFirstMemberStopsTheGroupAndIsSaidUrgent(t *testing.T) { + w := approvedGroup(t) + allGreen(w) + w.forge.refuseRepo = map[string]error{gCat: refusedWords("novox/mesh-catalog#2")} + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + if len(w.forge.merges) != 1 || !strings.Contains(w.forge.merges[0], "mesh-catalog#2") { + t.Fatalf("wanted only the first member asked: %v", w.forge.merges) + } + lines := urgentLines(w) + if len(lines) != 1 || lines[0].ID != IDOf(gCat, gHk) || !strings.Contains(lines[0].Says, "feat/g") || + !strings.Contains(lines[0].Says, "mesh/repo-check (missing)") || !strings.Contains(lines[0].Says, "Needed") { + t.Fatalf("not said as one urgent line: %+v", lines) + } +} + +// 1275/AC4: a group half merged (the second member refused after the first merged) stays raised, pass after pass, until +// the second is merged; then it goes. +func TestAHalfMergedGroupStaysRaisedUntilEveryMemberIsMerged(t *testing.T) { + w := approvedGroup(t) + allGreen(w) + w.forge.refuseRepo = map[string]error{gCtl: refusedWords("novox/mesh-controller#1")} + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + w.h.PullMerged(merged(gCat, 2, gHk, gT1)) + for range 3 { + w.later(time30s) + w.settleAll() + lines := urgentLines(w) + if len(lines) != 1 || lines[0].ID != IDOf(gCtl, gHc) || !strings.Contains(lines[0].Says, "feat/g") { + t.Fatalf("the half merged group is not raised: %+v", lines) + } + } + // A half merged group with no refusal on record (the pass died between members) is raised too. + d := w.delivery(IDOf(gCtl, gHc)) + d.Ready.RefusedSince = nil + if lines := urgentLines(w); len(lines) != 1 || !strings.Contains(lines[0].Says, "half merged") { + t.Fatalf("a half merged group without a refusal is not raised: %+v", lines) + } + w.forge.refuseRepo = nil + setChecks(w, gCtl, gHc, "success", "success") + w.h.PullMerged(merged(gCtl, 1, gHc, gT1)) + w.settleAll() + if lines := urgentLines(w); len(lines) != 0 { + t.Fatalf("the group is whole and still raised: %+v", lines) + } +} + +// 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) { + w := approvedGroup(t) + allGreen(w) + w.forge.refuseRepo = map[string]error{gCtl: refusedWords("novox/mesh-controller#1")} + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + w.h.PullMerged(merged(gCat, 2, gHk, gT1)) + asked := len(w.ctl.checks) + w.settleAll() + own, composed := ownChecks(w, asked) + if len(composed) != 0 || len(own) != 1 || own[0] != ":"+IDOf(gCtl, gHc) { + t.Fatalf("own checks %v, composed checks %v; wanted the member's own check once and no composed check", own, composed) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/model.go b/modules/mesh-delivery/cmd/mesh-delivery/model.go index a81bc2bf..863d832a 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/model.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/model.go @@ -968,12 +968,24 @@ type Group struct { 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"` + // Hold is why the group's merge or its Approve's ask is held back by a member's required status that is not a + // success (issue 1275); nil when nothing is. + Hold *GroupHold `json:"hold,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"` } +// GroupHold is a group held back from its Approve or its merge because a member's required status is not a success. +type GroupHold struct { + // Kind is "unasked" (the Approve is not asked) or "blocked" (the approved merge is not started: nothing is merged). + Kind string `json:"kind"` + Member string `json:"member"` + Says string `json:"says"` + Since time.Time `json:"since"` +} + // GroupCheck is a group's composed check. type GroupCheck struct { Asked string `json:"asked"` diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go index 8417ba16..88fa960c 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready.go @@ -202,6 +202,13 @@ func (h *Holder) readyAsks(now time.Time) []func() { if !r.LastTry.IsZero() { continue // refused: asked again at the next event that decides whether it may merge (mergeAgainNow), with no timer } + if g := h.groups[d.Group]; d.Group != "" && g != nil { + // A group is merged in one pass, in its order, and stops at the first refusal (issue 1275). + if a := h.groupPass(g, d, now); a != nil { + asks = append(asks, a) + } + continue + } r.Tries++ r.LastTry, r.merging = now, true h.keep(d) @@ -330,48 +337,52 @@ func (h *Holder) groupTurnToMerge(d *Delivery) string { // the heads the Approve covers, each `owner/repo@` (novox/hq ADR 0421 §9), and refuses a pull request whose // head differs from the one approved. func (h *Holder) askMerge(id string, heads map[string]string) func() { - return func() { - h.mu.Lock() - d := h.deliveries[id] - if d == nil || d.Ready == nil { - h.mu.Unlock() - return - } - owner, repo, number, head := d.Owner(), d.Repo(), d.Number, d.Commit - if sha := heads[id]; sha != "" { - head = sha - } + return func() { h.mergeOne(id, heads) } +} + +// mergeOne asks the merge tool for one approved head and keeps its answer; whether it merged. +func (h *Holder) mergeOne(id string, heads map[string]string) bool { + h.mu.Lock() + d := h.deliveries[id] + if d == nil || d.Ready == nil { h.mu.Unlock() - err := h.Forge.Merge(owner, repo, number, head, approvedHeads(heads)) - h.mu.Lock() - defer h.mu.Unlock() - d = h.deliveries[id] - if d == nil || d.Ready == nil { - return - } - r, now := d.Ready, h.Now() - r.merging = false - if err == nil { - // Issue 1025: the announcement is owed, kept with the delivery, so a death right after the merge - // still says it once. - d.Owed = append(d.Owed, Effect{Kind: EffectEmit, Event: "merged", Line: string(mustJSON(mergedEvent(d, head))), Since: now}) - r.Merged, r.Answer, r.RefusedSince = &now, "merged", nil - h.Logf("[mesh-delivery] %s merged on the operator's Approve at %s, its check passed against its trunk's head", id, - shortOf(head, 12)) - h.keep(d) - return - } - answer := oneLine(err.Error()) - if r.RefusedSince == nil { - r.RefusedSince = &now - } - if answer != r.Answer { - h.Logf("[mesh-delivery] %s is approved, and the merge tool did not merge it (asked again at the next verdict, Approve or composed check): %s", id, - answer) - } - r.Answer = answer - h.keep(d) + return false } + owner, repo, number, head := d.Owner(), d.Repo(), d.Number, d.Commit + if sha := heads[id]; sha != "" { + head = sha + } + h.mu.Unlock() + err := h.Forge.Merge(owner, repo, number, head, approvedHeads(heads)) + h.mu.Lock() + defer h.mu.Unlock() + d = h.deliveries[id] + if d == nil || d.Ready == nil { + return false + } + r, now := d.Ready, h.Now() + r.merging = false + if err == nil { + // Issue 1025: the announcement is owed, kept with the delivery, so a death right after the merge + // still says it once. + d.Owed = append(d.Owed, Effect{Kind: EffectEmit, Event: "merged", Line: string(mustJSON(mergedEvent(d, head))), Since: now}) + r.Merged, r.Answer, r.RefusedSince = &now, "merged", nil + h.Logf("[mesh-delivery] %s merged on the operator's Approve at %s, its check passed against its trunk's head", id, + shortOf(head, 12)) + h.keep(d) + return true + } + answer := oneLine(err.Error()) + if r.RefusedSince == nil { + r.RefusedSince = &now + } + if answer != r.Answer { + h.Logf("[mesh-delivery] %s is approved, and the merge tool did not merge it (asked again at the next verdict, Approve or composed check): %s", id, + answer) + } + r.Answer = answer + h.keep(d) + return false } // approvedHeads are the heads an Approve covers as the merge tool is handed them: `owner/repo@`, sorted. @@ -420,17 +431,19 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { // again after each composed check. Said after the merge window, with the group and the tool's last answer. out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(*r.RefusedSince).Round(time.Second).String(), Bound: groupRefusedFor.String(), - H2: "none: the group's composed check is asked again after each refusal, and the merge each minute — the operator's", - Says: fmt.Sprintf("%s#%d is approved at %s with its group %s closed (a sibling merged), and the merge tool has "+ - "refused it since %s: %s", d.Repository, d.Number, shortOf(r.Head, 12), d.Group, - r.RefusedSince.UTC().Format(time.RFC3339), r.Answer)}) + H2: "none: the group's composed check is asked again after each refusal, and the merge each minute — the operator's", + Severity: "urgent", + Says: fmt.Sprintf("group %s is half merged: %s#%d is approved at %s and the merge tool has refused it since %s: %s. "+ + "Needed: the refusal's cause is met (the merge is asked again at each verdict, status or Approve), or the operator decides.", + d.Group, d.Repository, d.Number, shortOf(r.Head, 12), r.RefusedSince.UTC().Format(time.RFC3339), r.Answer)}) case r.RefusedSince != nil && now.Sub(*r.RefusedSince) >= readyRefusedFor: out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(*r.RefusedSince).Round(time.Second).String(), Bound: readyRefusedFor.String(), H2: "none: the merge is asked again on a verdict, an Approve, a group's composed check or a merge check's " + "status turning success — the operator's", - Says: fmt.Sprintf("%s#%d is approved at %s, and the merge tool has refused it since %s: %s", d.Repository, - d.Number, shortOf(r.Head, 12), r.RefusedSince.UTC().Format(time.RFC3339), r.Answer)}) + Says: fmt.Sprintf("%s%s#%d is approved at %s, and the merge tool has refused it since %s: %s%s", heldInGroup(d), + d.Repository, d.Number, shortOf(r.Head, 12), r.RefusedSince.UTC().Format(time.RFC3339), r.Answer, neededOf(d)), + Severity: severityOf(d)}) } } return out @@ -452,3 +465,27 @@ func mergedEvent(d *Delivery, head string) map[string]any { } return ev } + +// heldInGroup is the words that put a refused member in its group's pass: none for a lone delivery. +func heldInGroup(d *Delivery) string { + if d.Group == "" { + return "" + } + return "group " + d.Group + " is held, and no member after this one is merged: " +} + +// neededOf is what a refused member needs, for a member of a group (issue 1275). +func neededOf(d *Delivery) string { + if d.Group == "" { + return "" + } + return ". Needed: the refusal's cause is met (the merge is asked again at each verdict, status or Approve), or the operator decides." +} + +// severityOf is "urgent" for a member of a group (a refused merge is the group held), empty for a lone delivery. +func severityOf(d *Delivery) string { + if d.Group == "" { + return "" + } + return "urgent" +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go index 06c4d7db..3a7dbce7 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go @@ -226,6 +226,8 @@ type StalledLine struct { Bound string `json:"bound"` H2 string `json:"h2"` Says string `json:"says"` + // Severity is "urgent" for a group that is held or half merged (issue 1275); empty otherwise. + Severity string `json:"severity,omitempty"` } // Stalled is the `stalled` verb: what the controller's self-check raises as `delivery..stalled`. @@ -269,6 +271,7 @@ func (h *Holder) Stalled() []StalledLine { out = append(out, h.withChecks(h.uncheckedLines(now))...) out = append(out, h.orphanLines(now)...) out = append(out, h.readyLines(now)...) + out = append(out, h.groupLines(now)...) sort.Slice(out, func(i, j int) bool { return out[i].ID < out[j].ID }) return out } From 524e3405e5175bbb8ade583f8ec02f1bc71435e5 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 20:32:22 +0200 Subject: [PATCH 2/3] 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) { From 301cd2287f5fa66cc0823e258be2d080687ada34 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 20:38:48 +0200 Subject: [PATCH 3/3] Answer gitea_commit_statuses in the bus test's runtime, and say a held, refused member once (issue 1275, review round 1) --- .../cmd/mesh-delivery/bus_test.go | 11 ++++++++ .../cmd/mesh-delivery/group_merge.go | 11 +++++++- .../cmd/mesh-delivery/issue1275_test.go | 27 +++++++++++++++++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go b/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go index a9562208..363da871 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go @@ -306,6 +306,17 @@ func (r *runtime) answer(m rpcMessage) { return toolAnswer(map[string]any{"asked": "check-1"}), nil case "gitea.gitea_open_pulls": return toolAnswer(map[string]any{"pulls": pulls}), nil + case "gitea.gitea_commit_statuses": + // Green: both merge checks a success at the head asked, as the gitea module answers. + sha, _ := a.Body["sha"].(string) + if sha == "" { + sha = "0000000" + } + return toolAnswer(map[string]any{"commit": sha, "statuses": []any{ + map[string]any{"context": "mesh/merge-gate", "state": "success"}, + map[string]any{"context": "mesh/repo-check", "state": "success"}}, + "merge": map[string]any{"branch": "main", "protected": true, "required": []string{"mesh/merge-gate", "mesh/repo-check"}, + "blocking": []any{}, "mergeable": true, "says": "main requires mesh/merge-gate, mesh/repo-check"}}), nil case "gitea.gitea_note_append", "gitea.gitea_delivery_view", "gitea.gitea_commit_status", "gitea.gitea_merge_pull_request", "gitea.gitea_add_comment": return toolAnswer(map[string]any{}), nil diff --git a/modules/mesh-delivery/cmd/mesh-delivery/group_merge.go b/modules/mesh-delivery/cmd/mesh-delivery/group_merge.go index 6ebc8b31..049d8179 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/group_merge.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/group_merge.go @@ -193,8 +193,10 @@ func (h *Holder) groupLines(now time.Time) []StalledLine { merged++ } } + heldOn := h.deliveries[holdMember(g)] switch { - case g.Hold != nil && g.Hold.Kind == "blocked" && !running: + // A held member the merge tool refused is said by readyLines already: one line for an id, not two. + case g.Hold != nil && g.Hold.Kind == "blocked" && !running && !(heldOn != nil && heldOn.Ready != nil && heldOn.Ready.RefusedSince != nil): out = append(out, StalledLine{ID: g.Hold.Member, Number: numberOf(h.deliveries[g.Hold.Member]), State: first.State, For: now.Sub(g.Hold.Since).Round(time.Second).String(), Bound: "0s", Severity: "urgent", H2: "none: the required status turns success, which asks the merge again — or the operator's decision", @@ -218,3 +220,10 @@ func numberOf(d *Delivery) int { } return d.Number } + +func holdMember(g *Group) string { + if g.Hold == nil { + return "" + } + return g.Hold.Member +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go index 315b5337..c7b08cb0 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue1275_test.go @@ -221,3 +221,30 @@ func TestAClosedGroupsMemberRefusedForItsOwnMissingStatusAsksItsOwnCheck(t *test t.Fatalf("own checks %v, composed checks %v; wanted the member's own check once and no composed check", own, composed) } } + +// 1275/AC3 (review round 1): a member that is both refused and held for a required status is said by one stalled line, +// not two with the same id. +func TestAMemberBothRefusedAndHeldForAStatusIsOneStalledLine(t *testing.T) { + w := approvedGroup(t) + allGreen(w) + w.forge.refuseRepo = map[string]error{gCat: refusedWords("novox/mesh-catalog#2")} + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + setChecks(w, gCat, gHk, "success", "") // now its status is missing too; the next pass is held + w.h.CommitStatus(ForgeStatusEvent{Owner: "novox", Repo: "mesh-catalog", SHA: gHk, Context: repoCheck, State: "success"}) + w.h.mergeAgainNow(w.delivery(IDOf(gCat, gHk))) + w.later(time30s) + w.settleAll() + if g := w.h.groups["feat/g"]; g.Hold == nil { + t.Fatalf("the group was not held for the missing status") + } + n := 0 + for _, l := range w.h.Stalled() { + if l.ID == IDOf(gCat, gHk) { + n++ + } + } + if n != 1 { + t.Fatalf("%d stalled lines for one member: %+v", n, w.h.Stalled()) + } +}