From fc9ddd91856f1846b6136e8d152d30b09eb21641 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 12:25:53 +0200 Subject: [PATCH] mesh-delivery adopts a standing verdict only when every required check stands, at every look, and counts an ask with no id as none (issue 1115) --- .../mesh-delivery/cmd/mesh-delivery/holder.go | 159 +++++++++++------- .../cmd/mesh-delivery/timers_test.go | 51 ++++++ .../mesh-delivery/unchecked_events_test.go | 2 +- 3 files changed, 151 insertions(+), 61 deletions(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 5647fdf7..d02478a7 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -1316,10 +1316,8 @@ func (h *Holder) plan() []func() { // Proposed, with no verdict and no check asked: its check is asked at once (issue 1115), or the verdict that // stands on its head at the forge is adopted. - for _, d := range h.deliveries { - if a := h.askCheck(d, now); a != nil { - asks = append(asks, a) - } + if a := h.askChecks(now); a != nil { + asks = append(asks, a) } // Merged with no walk yet: tied to the newest walk heard that answers it — by name, by its own commit, or by @@ -1848,74 +1846,115 @@ func heldForGroup(d *Delivery, g *Group) bool { return d.State == Held && strings.HasPrefix(d.HeldWhy, fmt.Sprintf("its group %s's composed check", g.ID)) } -// askCheck is the ask of a proposed delivery's own check, at once: no verdict, no check this owner asked. Nil -// otherwise — once asked, a late verdict is the bound's and H2's to speak for. Before asking, the forge's statuses -// on the head are read: a passing verdict that already stands there for this pull request is adopted, and the -// delivery goes on to ready with no new check (issue 1115). -func (h *Holder) askCheck(d *Delivery, now time.Time) func() { - if d.State != Proposed || d.Check != nil || d.CheckAsk != nil || d.MergedAs != "" || d.Number == 0 { +// standingVerdict is the passing verdict an open pull request's head holds at the forge, when every merge check and +// every context its base requires stands as success or warning; nil otherwise, a missing context included (issue 1115). +func standingVerdict(p OpenPull, now time.Time) *Verdict { + if p.Checks == nil { return nil } - if tried, was := h.askTried[d.ID]; was && now.Sub(tried) < time.Minute { - h.lookAt(tried.Add(time.Minute), "a proposed delivery's check that could not be asked") + contexts := slices.Clone(mergeChecks) + for _, c := range p.Required { + if !slices.Contains(contexts, c) { + contexts = append(contexts, c) + } + } + for _, c := range contexts { + if st := p.Checks[c].State; st != "success" && st != "warning" { + return nil + } + } + return &Verdict{At: now, Gate: verdictOfState(p.Checks["mesh/merge-gate"].State), + Summary: "the head's mesh/merge-gate, as the forge holds it", Repo: verdictOfState(p.Checks["mesh/repo-check"].State)} +} + +// askedCheck is whether a proposed delivery's own check stands asked: a kept ask with no id is none. +func askedCheck(d *Delivery) bool { return d.CheckAsk != nil && d.CheckAsk.ID != "" } + +// askChecks are the asks of proposed deliveries with no verdict, at each look, at once (issue 1115): the forge's open +// pull requests are read once, a passing verdict standing on a head is adopted (the delivery goes on to ready with no +// new check), and a head with no check asked is asked its check. Under the lock; the asks run outside it. +func (h *Holder) askChecks(now time.Time) func() { + type want struct { + id, owner, repo, head string + number int + member Member + ask bool + why string + } + var wants []want + for _, d := range h.deliveries { + if d.State != Proposed || d.Check != nil || d.MergedAs != "" || d.Number == 0 { + continue + } + w := want{id: d.ID, head: d.Commit, number: d.Number, member: MemberOf(d)} + w.owner, w.repo, _ = strings.Cut(d.Repository, "/") + if !askedCheck(d) { + if tried, was := h.askTried[d.ID]; was && now.Sub(tried) < time.Minute { + h.lookAt(tried.Add(time.Minute), "a proposed delivery's check that could not be asked") + } else { + w.ask = true + h.askTried[d.ID] = now + h.lookAt(now.Add(time.Minute), "a proposed delivery's check that could not be asked") + w.why = fmt.Sprintf("proposed for %s with no verdict and no check asked", now.Sub(d.Since).Round(time.Second)) + } + } + wants = append(wants, w) + } + if len(wants) == 0 { return nil } - h.askTried[d.ID] = now - h.lookAt(now.Add(time.Minute), "a proposed delivery's check that could not be asked") - id, m := d.ID, MemberOf(d) - owner, repo, number, head := strings.Split(d.Repository, "/")[0], d.Repository[strings.Index(d.Repository, "/")+1:], d.Number, d.Commit - why := fmt.Sprintf("proposed for %s with no verdict and no check asked", now.Sub(d.Since).Round(time.Second)) return func() { - var standing *Verdict - if open, err := h.Forge.OpenPulls(); err == nil { - for _, p := range open { - if p.Number != number || !strings.EqualFold(p.Owner, owner) || !strings.EqualFold(p.Repo, repo) || - !sameCommit(p.HeadSHA, head) { - continue - } - states := map[string]string{} - for c, hc := range p.Checks { - states[c] = hc.State - } - if v := decidedOnHead(states, h.Now()); v != nil && v.Passes() { - standing = v + open, err := h.Forge.OpenPulls() + for _, w := range wants { + var standing *Verdict + if err == nil { + for _, p := range open { + if p.Number == w.number && strings.EqualFold(p.Owner, w.owner) && strings.EqualFold(p.Repo, w.repo) && + sameCommit(p.HeadSHA, w.head) { + standing = standingVerdict(p, h.Now()) + } } } - } - if standing != nil { - h.mu.Lock() - defer h.mu.Unlock() - d := h.deliveries[id] - if d == nil || d.State != Proposed || d.Check != nil { - return + if standing != nil { + h.mu.Lock() + if d := h.deliveries[w.id]; d != nil && d.State == Proposed && d.Check == nil { + delete(h.askTried, w.id) + d.Check = standing + h.Logf("[mesh-delivery] %s: a passing verdict already stands on its head at the forge; adopted, no check asked", w.id) + h.settleAndKeep(d) + } + h.mu.Unlock() + continue + } + if w.ask { + h.askOwnCheck(w.id, w.member, w.why) } - delete(h.askTried, id) - d.Check = standing - h.Logf("[mesh-delivery] %s: a passing verdict already stands on its head at the forge; adopted, no check asked", id) - h.settleAndKeep(d) - return } - asked, err := h.Controller.Check("", []Member{m}) - h.mu.Lock() - defer h.mu.Unlock() - if err != nil { - h.Logf("[mesh-delivery] %s's check cannot be asked yet: %v", id, err) - return - } - delete(h.askTried, id) - d := h.deliveries[id] - if d == nil || d.State != Proposed || d.Check != nil { - return - } - d.CheckAsk = &CheckAsk{ID: asked, At: h.Now(), By: byOwner, Why: why} - h.Logf("[mesh-delivery] %s: its check asked of the controller as %s — %s", id, asked, why) - d.Owed = append(d.Owed, Effect{Kind: EffectNote, Commit: d.Commit, Since: h.Now(), - Line: oneLine(fmt.Sprintf("%s delivery %s: its check asked by %s as %s: %s", h.Now().UTC().Format(time.RFC3339), - id, byOwner, asked, why))}) - h.keep(d) } } +// askOwnCheck asks the controller a proposed delivery's own check and keeps the ask. +func (h *Holder) askOwnCheck(id string, m Member, why string) { + asked, err := h.Controller.Check("", []Member{m}) + h.mu.Lock() + defer h.mu.Unlock() + if err != nil { + h.Logf("[mesh-delivery] %s's check cannot be asked yet: %v", id, err) + return + } + delete(h.askTried, id) + d := h.deliveries[id] + if d == nil || d.State != Proposed || d.Check != nil || askedCheck(d) { + return + } + d.CheckAsk = &CheckAsk{ID: asked, At: h.Now(), By: byOwner, Why: why} + h.Logf("[mesh-delivery] %s: its check asked of the controller as %s — %s", id, asked, why) + d.Owed = append(d.Owed, Effect{Kind: EffectNote, Commit: d.Commit, Since: h.Now(), + Line: oneLine(fmt.Sprintf("%s delivery %s: its check asked by %s as %s: %s", h.Now().UTC().Format(time.RFC3339), + id, byOwner, asked, why))}) + h.keep(d) +} + // adopt makes a delivery of a walk no pull request is known for: running already — at the switch, or on the // controller's own path — it is delivering; waiting for a word, it waits for a person. func (h *Holder) adopt(w Walk) { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go b/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go index 90ca696f..4d378110 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go @@ -141,3 +141,54 @@ func TestAnAskTheControllerCouldNotAnswerIsAskedAgainAfterAWhileByTheTimerAlone( t.Fatalf("the look wanted at %s for %q; wanted %s for the plan that could not be asked", at, step, want) } } + +func TestAStandingGateWithNoRepoCheckIsNotAdoptedAndTheCheckIsAsked(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + id := IDOf("novox/app", readyHead) + op := openPull("novox/app", 7, readyHead) + op.Required = []string{"mesh/merge-gate", "mesh/repo-check"} + op.Checks = map[string]HeadCheck{"mesh/merge-gate": {State: "success"}} + w.forge.open = []OpenPull{op} + w.settleAll() + if d := w.delivery(id); d.Check != nil || d.State != Proposed { + t.Fatalf("a gate with no repo-check was adopted: %+v (%s)", d.Check, d.State) + } + if len(w.ctl.checks) != 1 { + t.Fatalf("its check was not asked: %v", w.ctl.checks) + } +} + +func TestAKeptAskWithNoIdIsNoAskAndAStandingVerdictIsAdopted(t *testing.T) { + w := newWorld(t) + w.op.auto = false + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + id := IDOf("novox/app", readyHead) + w.h.mu.Lock() + w.h.deliveries[id].CheckAsk = &CheckAsk{At: w.now, By: byOwner} + w.h.mu.Unlock() + op := openPull("novox/app", 7, readyHead) + op.Required = []string{"mesh/merge-gate", "mesh/repo-check"} + op.Checks = map[string]HeadCheck{"mesh/merge-gate": {State: "success"}, "mesh/repo-check": {State: "success"}} + w.forge.open = []OpenPull{op} + w.settleAll() + if d := w.delivery(id); d.State != Ready || d.Check == nil { + t.Fatalf("the standing verdict was not adopted over an ask with no id: %s", d.State) + } + if len(w.ctl.checks) != 0 || w.op.count() != 1 { + t.Fatalf("checks %v, Approves asked %d; wanted none and one", w.ctl.checks, w.op.count()) + } +} + +func TestAKeptAskWithNoIdAndNoStandingVerdictIsAskedAgainAtOnce(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + id := IDOf("novox/app", readyHead) + w.h.mu.Lock() + w.h.deliveries[id].CheckAsk = &CheckAsk{At: w.now, By: byOwner} + w.h.mu.Unlock() + w.settleAll() + if d := w.delivery(id); d.CheckAsk == nil || d.CheckAsk.ID == "" || len(w.ctl.checks) != 1 { + t.Fatalf("not asked again: %+v %v", d.CheckAsk, w.ctl.checks) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go b/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go index ab8a2d64..b2999e7c 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go @@ -60,7 +60,7 @@ func TestANewlyAnnouncedHeadReadsTheForgeAndAKnownOneDoesNot(t *testing.T) { t.Fatalf("a new head read the forge %d times, wanted twice", w.forge.openCalls-before) } hear(t, w, "gitea.pull.updated", pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - if w.forge.openCalls != before+2 { + if w.forge.openCalls != before+3 { // the look of a delivery awaiting its verdict reads its standing one t.Fatalf("the same head announced again read the forge again (%d reads)", w.forge.openCalls-before) } }