From cf4879eaa16431093391dd86df0b21f517b15957 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 28 Oct 2026 20:59:46 +0100 Subject: [PATCH] Ask the Approve when the check's verdict arrives, not after an hour (issue 1733) --- .../cmd/mesh-delivery-gate/approval.go | 3 - .../cmd/mesh-delivery-gate/holder.go | 7 +-- .../cmd/mesh-delivery-gate/onestatus.go | 5 -- .../cmd/mesh-delivery-gate/ready.go | 57 ++----------------- .../cmd/mesh-delivery-gate/ready_test.go | 35 ++++++------ 5 files changed, 23 insertions(+), 84 deletions(-) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go index e57a47b0..1e08b31c 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go @@ -411,9 +411,6 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) { } } check := h.checkAt(members) - if !h.checkGates() { - check = unansweredWords(check) // asked with no verdict only past its bound (verdictWaits) - } ask := composeMergeAsk(sdk.AskingID(id, asking), d, group, members, heads, check, now) taken := []*Delivery{} var reads []memberRead diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go index b2337024..79af7cda 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -79,10 +79,7 @@ type Holder struct { unchecked map[string]unchecked // checker are the checker's verdicts by the hash of the heads they judged, as its verdicts bucket holds them // (onestatus.go, issue 1302); runningSince when this owner first saw each running (ADR 0548 §2, turns.go). - checker map[string]checkerVerdict - // verdictGone is when the checker's verdict at a heads hash was taken away (its key deleted): the Approve's wait for a - // verdict counts its bound from then, as from a check that starts again (verdictWaits). - verdictGone map[string]time.Time + checker map[string]checkerVerdict runningSince map[string]time.Time // unmergeableSince is when the forge first said a group's member does not merge now, by group and member (issue 1275, // group_merge.go): within forgeMergeGrace that is the forge still working out mergeability, asked again shortly. @@ -146,7 +143,7 @@ func (h *Holder) init() { h.lostRead = map[string]time.Time{} h.unchecked = map[string]unchecked{} h.checker = map[string]checkerVerdict{} - h.runningSince, h.turnsHeld, h.verdictGone = map[string]time.Time{}, map[string]sdk.Turn{}, map[string]time.Time{} + h.runningSince, h.turnsHeld = map[string]time.Time{}, map[string]sdk.Turn{} h.reach = map[string]sdk.Reach{} h.started = h.Now() } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus.go index e53a7c0f..6715fdc7 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus.go @@ -82,13 +82,9 @@ func (h *Holder) CheckerVerdictChanged(key string, value []byte) { if h.runningSince == nil { h.runningSince = map[string]time.Time{} } - if h.verdictGone == nil { - h.verdictGone = map[string]time.Time{} - } if len(value) == 0 || string(value) == "null" { delete(h.checker, hash) delete(h.runningSince, hash) - h.verdictGone[hash] = h.Now() h.Poke() return } @@ -104,7 +100,6 @@ func (h *Holder) CheckerVerdictChanged(key string, value []byte) { h.runningSince[v.Heads] = h.Now() // a check starts: a merge it holds says since when (ADR 0548 §2) } else if v.State != checkerRunning { delete(h.runningSince, v.Heads) - delete(h.verdictGone, v.Heads) } h.checker[v.Heads] = v h.Poke() diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go index 0f038c3f..94ffc5b8 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -453,67 +453,20 @@ func (h *Holder) ungatedWaits(d *Delivery) string { return "" } -// askCheckBound is how long an Approve waits for the check's verdict at its heads while the check gates no merge: past it, -// the operator is asked anyway, and the ask says the check has not answered. The checker's own step bound is 30 minutes -// and a check runs several steps; an hour covers a whole check. -const askCheckBound = time.Hour - // waitsCheckBeforeAsking is how the wait for the check's verdict begins, as `show` and the delivery's wait say it. const waitsCheckBeforeAsking = "waits its check before asking the operator's Approve: " // verdictWaits is why an Approve is not asked yet while the check gates no merge: the check at the heads the ask would cover // has no verdict (the operator, 2026-10-27 23:54: "if you don't want me to approve it, don't send the approval request"). -// A pass or a failure is asked (a failure in its words: the operator may still merge it), and so is a check that has not -// answered within askCheckBound of the heads becoming ready, which the ask says. For a group the check is the one of all its -// heads together. Under the lock. +// A pass or a failure is asked (a failure in its words: the operator may still merge it), and nothing else: no timer stands +// in for the verdict, because the checker turns a check it lost into a failure it says aloud (issue 1733, the operator, +// 2026-10-28: "one hour is absurd"). For a group the check is the one of all its heads together. Under the lock. func (h *Holder) verdictWaits(d *Delivery) string { - members := h.askMembers(d) - c := h.checkAt(members) + c := h.checkAt(h.askMembers(d)) if c.Key == checkSaidPass || c.Key == checkSaidFail { return "" } - until := h.waitSince(d, members).Add(askCheckBound) - if !h.Now().Before(until) { - return "" - } - h.lookAt(until, "an Approve that waits its check, asked anyway at its bound") - return waitsCheckBeforeAsking + c.Words + "; asked anyway at " + clockSaid(until) + " if it has not answered" -} - -// waitSince is when the wait for a verdict began, the moment the bound of verdictWaits counts from: the latest of the -// heads becoming ready (a member with no ready run yet, such as one re-pushed or settled ready by an event this look has -// not reached, became ready now), the check at those heads starting again, and its verdict being taken away. Under the lock. -func (h *Holder) waitSince(d *Delivery, members []*Delivery) time.Time { - now := h.Now() - since := time.Time{} - later := func(t time.Time) { - if t.After(since) { - since = t - } - } - for _, m := range append([]*Delivery{d}, members...) { - if m.Ready == nil { - return now - } - later(m.Ready.Seen) - } - if hash, err := sdk.HeadsHash(headsOf(members)); err == nil { - later(h.runningSince[hash]) - later(h.verdictGone[hash]) - } - if since.IsZero() || since.After(now) { - return now - } - return since -} - -// unansweredWords is the check's words in an ask made past askCheckBound with no verdict: it says the check has not answered. -func unansweredWords(c checkSaid) checkSaid { - if c.Key == checkSaidPass || c.Key == checkSaidFail { - return c - } - c.Words += ", and has given none in " + strconv.Itoa(int(askCheckBound.Minutes())) + " minutes, so the operator is asked anyway" - return c + return waitsCheckBeforeAsking + c.Words } // waitsForReview is how a hold for a review begins, as the delivery's state and its mesh/delivery status say it. diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go index a02db03b..c8354af9 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go @@ -71,8 +71,8 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { // 1733: the Approve's ask says the check's state at the heads it names, read from the checker's verdict there: "passed // its check" only when it passed. While the check gates no merge, the ask waits for the check's verdict (the operator, // 2026-10-27 23:54: "if you don't want me to approve it, don't send the approval request"): a pass or a failure is asked, - // the failure in its words; with no verdict nothing is asked and the delivery says it waits its check, until the bound, - // past which it is asked anyway, saying so, and asked again in new words once the check passes. The verdict is the + // the failure in its words; with no verdict nothing is asked and the delivery says it waits its check, until the verdict + // comes, however long (issue 1733: no timer stands in for it). The verdict is the // checker's verdicts bucket value as captured on the bus (mesh-delivery-checker, `.`). for _, row := range []struct { name, state, failed, then string @@ -84,21 +84,19 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { {name: "gated, passed", gated: true, state: "pass", asks: 1, want: []string{"app#7 Fix it passed its check. Approve merges it into main at " + readyHead[:12] + ";"}, not: []string{"before its check ends", "no verdict yet"}}, - {name: "ungated, no verdict", asks: 0, want: []string{waitsCheckBeforeAsking + "its check has no verdict yet; asked anyway at 15:00 CEST if"}}, + {name: "ungated, no verdict", asks: 0, want: []string{waitsCheckBeforeAsking + "its check has no verdict yet"}, not: []string{"anyway"}}, {name: "ungated, running", state: "running", asks: 0, want: []string{waitsCheckBeforeAsking + "its check has no verdict yet: it is running"}}, - {name: "ungated, running past its bound", state: "running", after: askCheckBound, asks: 1, - want: []string{"its check has no verdict yet: it is running", "has given none in 60 minutes, so the operator is asked anyway", - "Approve merges it into main at " + readyHead[:12] + " now, before its check ends"}, not: []string{"passed"}}, - {name: "ungated, past its bound, then passed", state: "running", after: askCheckBound, then: "pass", asks: 2, + {name: "ungated, running for hours, still not asked", state: "running", after: 5 * time.Hour, asks: 0, + want: []string{waitsCheckBeforeAsking + "its check has no verdict yet: it is running"}, not: []string{"anyway"}}, + {name: "ungated, running, then passed", state: "running", after: 2 * time.Hour, then: "pass", asks: 1, want: []string{"passed its check. Approve merges it into main at " + readyHead[:12] + ";"}, not: []string{"before its check ends", "anyway"}}, {name: "ungated, passed", state: "pass", asks: 1, want: []string{"passed its check. Approve merges it into main at " + readyHead[:12] + ";"}, not: []string{"before its check ends"}}, - // A pass that turns back into a check with no verdict (a check started again, its verdict taken away) past the bound - // withdraws the ask, and the bound counts again from that moment: nothing is asked at once as unanswered. - {name: "ungated, passed, running again past its bound", state: "pass", after: askCheckBound, then: "running", asks: 1, open: -1, - want: []string{waitsCheckBeforeAsking + "its check has no verdict yet: it is running (since 15:00 CEST); asked anyway at 16:00 CEST"}}, - {name: "ungated, passed, its verdict gone past its bound", state: "pass", after: askCheckBound, then: "gone", asks: 1, open: -1, - want: []string{waitsCheckBeforeAsking + "its check has no verdict yet; asked anyway at 16:00 CEST"}}, + // A pass that turns back into a check with no verdict (a check started again, its verdict taken away) withdraws the ask. + {name: "ungated, passed, running again", state: "pass", then: "running", asks: 1, open: -1, + want: []string{waitsCheckBeforeAsking + "its check has no verdict yet: it is running"}, not: []string{"anyway"}}, + {name: "ungated, passed, its verdict gone", state: "pass", then: "gone", asks: 1, open: -1, + want: []string{waitsCheckBeforeAsking + "its check has no verdict yet"}, not: []string{"anyway"}}, {name: "ungated, failed", state: "fail", failed: "TestThing: wanted 1, got 2", asks: 1, want: []string{"its check failed: TestThing: wanted 1, got 2", "although its check failed"}, not: []string{"passed", "anyway"}}, } { @@ -369,8 +367,8 @@ func TestAGroupMergesEveryMemberInOnePassOnlyOnceComposedAndApproved(t *testing. if e := u.op.ask(u.op.open()[0]).Explanation; !strings.Contains(e, "2 pull requests, passed its check. Approve merges all of them into main in one pass, at the heads named;") { t.Fatalf("ungated, the group's ask does not say its check passed:\n%s", e) } - // A member re-pushed past the bound after its sibling became ready: the bound counts from the new head, so nothing is - // asked as unanswered at once (review round 1). + // A member re-pushed hours after its sibling became ready: with no verdict at the new heads nothing is asked, however + // long the sibling has waited. r := newWorld(t) ungated(r) r.op.auto = false @@ -378,17 +376,16 @@ func TestAGroupMergesEveryMemberInOnePassOnlyOnceComposedAndApproved(t *testing. r.h.PullUpdated(pr("novox/mesh-controller", 1, ctlHead, "feat/x", "internal/a.go")) r.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json")) r.settleAll() - r.later(askCheckBound + time.Minute) + r.later(5 * time.Hour) r.h.PullUpdated(pr("novox/mesh-controller", 1, ctlHead2, "feat/x", "internal/a.go")) - // Made ready by a handler, before the look gives it a run: no verdict and no status at the new heads, so only the rule - // that a member with no run counts as ready now keeps the older sibling's time from asking it as unanswered. + // Made ready by a handler, before the look gives it a run: no verdict and no status at the new heads. r.h.mu.Lock() r.h.ungate(r.h.deliveries[IDOf("novox/mesh-controller", ctlHead2)], r.h.Now()) r.h.mu.Unlock() r.settleAll() if d := r.delivery(IDOf("novox/mesh-catalog", catHead)); r.op.count() != 0 || d.Ready == nil || !strings.Contains(d.Ready.Waits, waitsCheckBeforeAsking) { - t.Fatalf("a member re-pushed past the bound: asked %d times, wait %+v; wanted none asked and the wait said", r.op.count(), d.Ready) + t.Fatalf("a member re-pushed hours later: asked %d times, wait %+v; wanted none asked and the wait said", r.op.count(), d.Ready) } u.op.answer(u.op.open()[0], OptionApprove)