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)

2026-10-18 12:25:53 +02:00
parent 1760b94824
commit fc9ddd9185
3 changed files with 151 additions and 61 deletions
@@ -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) {
@@ -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)
+	}
+}
@@ -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)
 	}
 }