From 89c30c44bee2bf6c1b7224e0b913bee3a5bd9d6c Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 27 Oct 2026 23:59:09 +0100 Subject: [PATCH 1/3] mesh-delivery-gate: while the check gates no merge, ask the Approve only once the check has a verdict (issue 1733) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The operator, 2026-10-27 23:54: "if you don't want me to approve it, don't send the approval request". The gate asked the Approve the moment a pull request opened, before its check answered, and five changes were approved unchecked. Now the ask waits for the checker's verdict at the heads it covers (a group's verdict of all its heads together; a placement change's verdict ends only once its nodes answered, ADR 0559 §5). A pass or a failure is asked, the failure in its words, since check-gates-merge false lets the operator merge it anyway. A check with no verdict 60 minutes after the heads became ready is asked anyway, saying so, and asked again in new words when its verdict comes. Quick fix (only the ask's wording) against thorough (the one place that decides when to ask, ungatedWaits): thorough, which is one place here. --- .../cmd/mesh-delivery-gate/approval.go | 3 + .../cmd/mesh-delivery-gate/helpers_test.go | 16 +++- .../cmd/mesh-delivery-gate/ready.go | 53 +++++++++++ .../cmd/mesh-delivery-gate/ready_test.go | 94 ++++++++++++------- modules/mesh-delivery-gate/module.json | 2 +- 5 files changed, 133 insertions(+), 35 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 1e08b31c..e57a47b0 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go @@ -411,6 +411,9 @@ 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/helpers_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go index 1bec05c3..2001a2a0 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go @@ -1,6 +1,11 @@ package main -import "time" +import ( + "encoding/json" + "time" + + sdk "git.novox.be/novox/mesh-sdk/go/delivery" +) var t0 = time.Date(2026, 10, 6, 12, 0, 0, 0, time.UTC) @@ -27,3 +32,12 @@ func statusOn(w *world, repo, commit string) string { defer w.forge.mu.Unlock() return w.forge.statuses[repo+"@"+commit+" mesh/delivery"] } + +// checkerSays puts the checker's verdict at these heads together into its verdicts bucket, as captured on the bus +// (mesh-delivery-checker, key `.`). +func checkerSays(w *world, group, state, failed string, heads ...sdk.Head) { + hash, _ := sdk.HeadsHash(heads) + value, _ := json.Marshal(map[string]any{"v": 1, "group": group, "heads": hash, "state": state, + "failed": map[string]string{"words": failed}}) + w.h.CheckerVerdictChanged(group+"."+hash, value) +} 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 aee84748..fdc97fd3 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -446,9 +446,62 @@ func (h *Holder) ungatedWaits(d *Delivery) string { } } } + if h.covering(d) == nil { + return h.verdictWaits(d) + } 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; for a placement change it ends only once every node it places answered (ADR 0559 §5). Under the lock. +func (h *Holder) verdictWaits(d *Delivery) string { + members := h.askMembers(d) + c := h.checkAt(members) + if c.Key == checkSaidPass || c.Key == checkSaidFail { + return "" + } + until := h.readySince(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 " + until.UTC().Format("15:04") + " UTC if it has not answered" +} + +// readySince is when the latest of the heads an ask would cover became ready: the bound of verdictWaits counts from it. +func (h *Holder) readySince(d *Delivery, members []*Delivery) time.Time { + since := h.Now() + if d.Ready != nil { + since = d.Ready.Seen + } + for _, m := range members { + if m.Ready != nil && m.Ready.Seen.After(since) { + since = m.Ready.Seen + } + } + 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 +} + // waitsForReview is how a hold for a review begins, as the delivery's state and its mesh/delivery status say it. const waitsForReview = "waits for a review at " 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 6e139b14..0249ec4c 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 @@ -1,7 +1,6 @@ package main import ( - "encoding/json" "errors" "sort" "strings" @@ -70,27 +69,32 @@ 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 is made before any verdict, says that Approve - // merges before the check ends, and is asked again in new words once the check passes or fails; a running check is no - // new word. + // 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 + // checker's verdicts bucket value as captured on the bus (mesh-delivery-checker, `.`). for _, row := range []struct { - name, state, failed string - gated bool - asks int - want, not []string + name, state, failed, then string + gated bool + after time.Duration + asks int + want, not []string }{ {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: 1, - want: []string{"its check has no verdict yet", "Approve merges it into main at " + readyHead[:12] + " now, before its check ends"}, - not: []string{"passed"}}, - {name: "ungated, running", state: "running", asks: 1, - want: []string{"its check has no verdict yet", "before its check ends"}, not: []string{"passed"}}, - {name: "ungated, then passed", state: "pass", asks: 2, + {name: "ungated, no verdict", asks: 0, want: []string{waitsCheckBeforeAsking + "its check has no verdict yet; asked anyway at "}}, + {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, + 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"}}, - {name: "ungated, then failed", state: "fail", failed: "TestThing: wanted 1, got 2", asks: 2, - want: []string{"its check failed: TestThing: wanted 1, got 2", "although its check failed"}, not: []string{"passed"}}, + {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"}}, } { t.Run(row.name, func(t *testing.T) { v := newWorld(t) @@ -101,23 +105,35 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") p.Title = "Fix it" v.h.PullUpdated(p) - put := func() { - hash, _ := sdk.HeadsHash([]sdk.Head{headOf("novox/app", 7, readyHead)}) - value, _ := json.Marshal(map[string]any{"v": 1, "group": "g", "heads": hash, "state": row.state, - "failed": map[string]string{"words": row.failed}}) - v.h.CheckerVerdictChanged("g."+hash, value) - } + head := headOf("novox/app", 7, readyHead) if row.gated { v.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) } v.settleAll() if !row.gated && row.state != "" { - put() + checkerSays(v, "g", row.state, row.failed, head) + v.settleAll() + } + if row.after > 0 { + v.later(row.after) + v.settleAll() + } + if row.then != "" { + checkerSays(v, "g", row.then, row.failed, head) v.settleAll() } open := v.op.open() - if len(open) != 1 || v.op.count() != row.asks { - t.Fatalf("open asks %v, asked %d times; wanted one open, asked %d times", open, v.op.count(), row.asks) + if v.op.count() != row.asks || len(open) != min(row.asks, 1) { + t.Fatalf("open asks %v, asked %d times; wanted asked %d times", open, v.op.count(), row.asks) + } + if row.asks == 0 { + d := v.delivery(IDOf("novox/app", readyHead)) + for _, w := range row.want { + if d.Ready == nil || !strings.Contains(d.Ready.Waits, w) { + t.Fatalf("the delivery's wait does not say %q: %+v", w, d.Ready) + } + } + return } e := v.op.ask(open[0]).Explanation for _, w := range row.want { @@ -146,8 +162,8 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { u.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) u.h.PullUpdated(pr("novox/app", 8, other, "feat/y", "src/b.go")) u.settleAll() - if u.op.count() != 2 || u.state(IDOf("novox/app", readyHead)) != Ready { - t.Fatalf("ungated, two reviewed pull requests with no verdict: %d asks, %s", u.op.count(), u.state(IDOf("novox/app", readyHead))) + if u.op.count() != 0 || u.state(IDOf("novox/app", readyHead)) != Ready { + t.Fatalf("ungated, two reviewed pull requests with no verdict: %d asks, %s; wanted ready and not asked", u.op.count(), u.state(IDOf("novox/app", readyHead))) } if gates, _ := u.h.Status("listening")["check-gates-merge"].(bool); gates { t.Fatalf("delivery_status does not say the check gates no merge: %v", u.h.Status("listening")) @@ -157,8 +173,12 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { t.Fatalf("show does not say the check gates no merge: %v", shown) } u.h.Checked(judgedAt("novox/app", 8, other, "fail", "t1t1t1t1t1t1")) + checkerSays(u, "feat/y", "fail", "TestB", headOf("novox/app", 8, other)) u.h.PullMerged(merged("novox/app", 7, readyHead, "m7m7m7m7m7m7")) u.settleAll() + if u.op.count() != 1 { + t.Fatalf("ungated, a failed verdict is asked once (the check gates no merge): %d asks", u.op.count()) + } d = u.delivery(IDOf("novox/app", other)) if d.State != Ready || d.Check == nil || d.Check.Passes() { t.Fatalf("ungated, after a failed verdict and a trunk's move: %s, verdict %+v", d.State, d.Check) @@ -323,13 +343,21 @@ func TestAGroupMergesEveryMemberInOnePassOnlyOnceComposedAndApproved(t *testing. u.h.PullUpdated(pr("novox/mesh-controller", 1, ctlHead, "feat/x", "internal/a.go")) u.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json")) u.settleAll() - if u.op.count() != 1 { - t.Fatalf("ungated, the group is asked %d times; wanted once", u.op.count()) + group := []sdk.Head{headOf("novox/mesh-controller", 1, ctlHead), headOf("novox/mesh-catalog", 2, catHead)} + checkerSays(u, "feat/x", "running", "", group...) + u.settleAll() + // The group's check is the one of its heads together: while it runs, nothing is asked and each member says it waits. + if d := u.delivery(IDOf("novox/mesh-catalog", catHead)); u.op.count() != 0 || d.Ready == nil || + !strings.Contains(d.Ready.Waits, waitsCheckBeforeAsking+"its check has no verdict yet: it is running") { + t.Fatalf("ungated, the group is asked %d times while its check runs; wanted none, and the wait said: %+v", u.op.count(), d.Ready) } - // 1733: the group's ask claims no pass it has not had. - if e := u.op.ask(u.op.open()[0]).Explanation; strings.Contains(e, "passed") || - !strings.Contains(e, "2 pull requests: its check has no verdict yet. Approve merges all of them into main in one pass, at the heads named, now, before its check ends;") { - t.Fatalf("ungated, the group's ask does not say its check has no verdict yet:\n%s", e) + checkerSays(u, "feat/x", "pass", "", group...) + u.settleAll() + if u.op.count() != 1 { + t.Fatalf("ungated, the group is asked %d times once its check passed; wanted once", u.op.count()) + } + 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) } u.op.answer(u.op.open()[0], OptionApprove) u.settleAll() diff --git a/modules/mesh-delivery-gate/module.json b/modules/mesh-delivery-gate/module.json index 06e16ec7..ef5d1b80 100644 --- a/modules/mesh-delivery-gate/module.json +++ b/modules/mesh-delivery-gate/module.json @@ -99,7 +99,7 @@ }, "check-gates-merge": { "type": "boolean", - "description": "Whether mesh/check gates the merge. True (the default): the operator's Approve is asked, and a merge handed, only on a passing verdict judged against the trunk's head, and a trunk's move puts a verdict out of date (ADR 0447, ADR 0505 §4). False (the operator, 2026-10-25 22:56: \"run the check locally only, but not as a gate, it loses us too much time\"; developers run the unit and integration levels locally before a pull request): a pull request reviewed at its head is ready and its Approve asked at once, with no verdict waited on; no merge is refused for a check judged on an older trunk; a verdict that comes is shown on the pull request and the delivery, never waited on; a group still merges all its members in one pass. Set it false together with the gitea module's check-gates-merge, which lets its merge tool merge an approved head without a passing mesh/check.", + "description": "Whether mesh/check gates the merge. True (the default): the operator's Approve is asked, and a merge handed, only on a passing verdict judged against the trunk's head, and a trunk's move puts a verdict out of date (ADR 0447, ADR 0505 §4). False (the operator, 2026-10-25 22:56: \"run the check locally only, but not as a gate, it loses us too much time\"; developers run the unit and integration levels locally before a pull request): a pull request reviewed at its head is ready, and its Approve is asked once the check at its heads (a group: of all its heads together) has a verdict, a pass or a failure said in the ask, or after 60 minutes with none, said so (the operator, 2026-10-27 23:54: \"if you don't want me to approve it, don't send the approval request\"); an approved merge waits for no verdict; no merge is refused for a check judged on an older trunk; a later verdict is shown on the pull request and the delivery, and asks a pending Approve again in its words; a group still merges all its members in one pass. Set it false together with the gitea module's check-gates-merge, which lets its merge tool merge an approved head without a passing mesh/check.", "trusted": false, "default": true } From dea390ac97b80dac14da4477df37c321ffb9ffa1 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 28 Oct 2026 00:05:36 +0100 Subject: [PATCH 2/3] mesh-delivery-gate: the Approve's verdict bound counts from the latest head, restarts with its check, and speaks the operator's time (issue 1733, review round 1) A group member made ready in an event handler has no ready run when its sibling is looked at; the bound counted from the older sibling alone, so a re-push past 60 minutes asked at once as unanswered. Such a member now counts as ready now. The bound also counts from a check starting again at the same heads, or its verdict being taken away, so a pass that turns back into running never asks at once as unanswered. Times in the wait and the ask are the operator's zone (Europe/Brussels, as the messenger says them), and a placement change whose pass holds no composition says that no node answered its placement. --- .../cmd/mesh-delivery-gate/helpers_test.go | 16 ++++- .../cmd/mesh-delivery-gate/holder.go | 7 ++- .../cmd/mesh-delivery-gate/onestatus.go | 44 +++++++++++++- .../cmd/mesh-delivery-gate/ready.go | 35 +++++++---- .../cmd/mesh-delivery-gate/ready_test.go | 60 +++++++++++++++---- 5 files changed, 134 insertions(+), 28 deletions(-) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go index 2001a2a0..28ad0e54 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go @@ -36,8 +36,20 @@ func statusOn(w *world, repo, commit string) string { // checkerSays puts the checker's verdict at these heads together into its verdicts bucket, as captured on the bus // (mesh-delivery-checker, key `.`). func checkerSays(w *world, group, state, failed string, heads ...sdk.Head) { + checkerSaysWith(w, group, state, failed, nil, heads...) +} + +// checkerSaysWith is checkerSays with more of the verdict's fields (its composition); the state "gone" deletes the key. +func checkerSaysWith(w *world, group, state, failed string, more map[string]any, heads ...sdk.Head) { hash, _ := sdk.HeadsHash(heads) - value, _ := json.Marshal(map[string]any{"v": 1, "group": group, "heads": hash, "state": state, - "failed": map[string]string{"words": failed}}) + if state == "gone" { + w.h.CheckerVerdictChanged(group+"."+hash, nil) + return + } + v := map[string]any{"v": 1, "group": group, "heads": hash, "state": state, "failed": map[string]string{"words": failed}} + for k, x := range more { + v[k] = x + } + value, _ := json.Marshal(v) w.h.CheckerVerdictChanged(group+"."+hash, value) } 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 9d690822..823c89d9 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -77,7 +77,10 @@ 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 + 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 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. @@ -141,7 +144,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 = map[string]time.Time{}, map[string]sdk.Turn{} + h.runningSince, h.turnsHeld, h.verdictGone = map[string]time.Time{}, map[string]sdk.Turn{}, map[string]time.Time{} 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 76386e98..90a06d57 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus.go @@ -8,6 +8,7 @@ import ( "sort" "strings" "time" + _ "time/tzdata" // the operator's zone is known whatever this machine has installed sdk "git.novox.be/novox/mesh-sdk/go/delivery" ) @@ -57,6 +58,12 @@ type checkerVerdict struct { Machine string `json:"machine,omitempty"` Words string `json:"words,omitempty"` } `json:"failed,omitzero"` + // Composition is what the check of the machines' composition made of it (mesh-sdk delivery.Composition): its state + // not-run says the composition, and with it a placement change's answers from its nodes, was not judged. + Composition *struct { + State string `json:"state"` + Words string `json:"words,omitempty"` + } `json:"composition,omitempty"` } // CheckerVerdictChanged takes a key of the checker's verdicts bucket (`.`) put or deleted (an empty @@ -75,9 +82,13 @@ 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 } @@ -93,6 +104,7 @@ 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() @@ -329,11 +341,11 @@ func (h *Holder) checkAt(members []*Delivery) checkSaid { if ok { switch v.State { case checkerPass: - return checkSaid{Key: checkSaidPass, Words: "passed its check"} + return checkSaid{Key: checkSaidPass, Words: "passed its check" + notComposed(v, members)} case checkerRunning: words := "its check has no verdict yet: it is running" if since, held := h.runningSince[hash]; held && !since.IsZero() { - words += " (since " + since.UTC().Format("15:04") + " UTC)" + words += " (since " + clockSaid(since) + ")" } return checkSaid{Key: checkSaidRunning, Words: words} default: @@ -371,3 +383,31 @@ func (h *Holder) checkAt(members []*Delivery) checkSaid { } return checkSaid{Key: checkSaidNone, Words: "its check has no verdict yet"} } + +// notComposed is what a passing verdict adds when its composition was not judged (mesh-sdk delivery.CompositionNotRun): a +// placement change's pass then holds no node's answer to its placement (ADR 0559 §5), which the ask says. Empty otherwise. +func notComposed(v checkerVerdict, members []*Delivery) string { + if v.Composition == nil || v.Composition.State != sdk.CompositionNotRun { + return "" + } + why := firstOf(oneLine(v.Composition.Words), "it was not run") + for _, m := range members { + for _, p := range m.Paths { + if strings.HasPrefix(p, "placement/") && strings.HasSuffix(p, ".json") { + return ", but its composition was not judged (" + why + "), so no node answered its placement" + } + } + } + return ", but its composition was not judged (" + why + ")" +} + +// operatorZone is the zone the operator reads times in, as the messenger says them (its time-zone setting). +var operatorZone = func() *time.Location { + if loc, err := time.LoadLocation("Europe/Brussels"); err == nil { + return loc + } + return time.UTC +}() + +// clockSaid is a time of day as the operator reads it: in their zone, named ("00:35 CET"). +func clockSaid(t time.Time) string { return t.In(operatorZone).Format("15:04 MST") } 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 fdc97fd3..c3ecbb17 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -471,25 +471,38 @@ func (h *Holder) verdictWaits(d *Delivery) string { if c.Key == checkSaidPass || c.Key == checkSaidFail { return "" } - until := h.readySince(d, members).Add(askCheckBound) + 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 " + until.UTC().Format("15:04") + " UTC if it has not answered" + return waitsCheckBeforeAsking + c.Words + "; asked anyway at " + clockSaid(until) + " if it has not answered" } -// readySince is when the latest of the heads an ask would cover became ready: the bound of verdictWaits counts from it. -func (h *Holder) readySince(d *Delivery, members []*Delivery) time.Time { - since := h.Now() - if d.Ready != nil { - since = d.Ready.Seen - } - for _, m := range members { - if m.Ready != nil && m.Ready.Seen.After(since) { - since = m.Ready.Seen +// 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 } 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 0249ec4c..03ef6b9e 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 @@ -75,16 +75,17 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { // past which it is asked anyway, saying so, and asked again in new words once the check passes. 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 - gated bool - after time.Duration - asks int - want, not []string + name, state, failed, then, path string + gated bool + notComposed bool + after time.Duration + asks, open int + want, not []string }{ {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 "}}, + {name: "ungated, no verdict", asks: 0, want: []string{waitsCheckBeforeAsking + "its check has no verdict yet; asked anyway at 15:00 CEST if"}}, {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", @@ -93,6 +94,15 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { 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 placement change's pass with its composition not judged holds no node's answer, and the ask says so (ADR 0559 §5). + {name: "ungated, placement passed uncomposed", state: "pass", path: "placement/node-a.json", notComposed: true, asks: 1, + want: []string{"passed its check, but its composition was not judged (the assigner is not running), so no node answered its placement"}}, {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"}}, } { @@ -102,7 +112,7 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { ungated(v) } v.op.auto = false - p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") + p := pr("novox/app", 7, readyHead, "feat/x", firstOf(row.path, "src/a.go")) p.Title = "Fix it" v.h.PullUpdated(p) head := headOf("novox/app", 7, readyHead) @@ -111,7 +121,11 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { } v.settleAll() if !row.gated && row.state != "" { - checkerSays(v, "g", row.state, row.failed, head) + var more map[string]any + if row.notComposed { + more = map[string]any{"composition": map[string]string{"state": "not-run", "words": "the assigner is not running"}} + } + checkerSaysWith(v, "g", row.state, row.failed, more, head) v.settleAll() } if row.after > 0 { @@ -123,10 +137,14 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { v.settleAll() } open := v.op.open() - if v.op.count() != row.asks || len(open) != min(row.asks, 1) { - t.Fatalf("open asks %v, asked %d times; wanted asked %d times", open, v.op.count(), row.asks) + wantOpen := min(row.asks, 1) + if row.open < 0 { + wantOpen = 0 } - if row.asks == 0 { + if v.op.count() != row.asks || len(open) != wantOpen { + t.Fatalf("open asks %v, asked %d times; wanted asked %d times, %d open", open, v.op.count(), row.asks, wantOpen) + } + if wantOpen == 0 { d := v.delivery(IDOf("novox/app", readyHead)) for _, w := range row.want { if d.Ready == nil || !strings.Contains(d.Ready.Waits, w) { @@ -359,6 +377,26 @@ 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). + r := newWorld(t) + ungated(r) + r.op.auto = false + const ctlHead2 = "c3c3c3c3c3c3c3c3c3c3c3c3c3c3c3c3c3c3c3c3" + 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.h.PullUpdated(pr("novox/mesh-controller", 1, ctlHead2, "feat/x", "internal/a.go")) + // Ready in its handler, with no run yet, while the check of the group's new heads together runs. + r.h.Checked(judgedAt("novox/mesh-controller", 1, ctlHead2, "pass", "t1t1t1t1t1t1")) + checkerSays(r, "feat/x", "running", "", headOf("novox/mesh-controller", 1, ctlHead2), headOf("novox/mesh-catalog", 2, catHead)) + 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) + } + u.op.answer(u.op.open()[0], OptionApprove) u.settleAll() if len(u.forge.merges) != 2 { From 354d62340d4d3d907d72146fc77f6fa49f12ef5e Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 28 Oct 2026 00:07:39 +0100 Subject: [PATCH 3/3] mesh-delivery-gate: the group re-push row holds no verdict at the new heads, so only the missing-run rule keeps it unasked (issue 1733, review round 2) --- .../cmd/mesh-delivery-gate/ready_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 03ef6b9e..4f85d309 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 @@ -388,9 +388,11 @@ func TestAGroupMergesEveryMemberInOnePassOnlyOnceComposedAndApproved(t *testing. r.settleAll() r.later(askCheckBound + time.Minute) r.h.PullUpdated(pr("novox/mesh-controller", 1, ctlHead2, "feat/x", "internal/a.go")) - // Ready in its handler, with no run yet, while the check of the group's new heads together runs. - r.h.Checked(judgedAt("novox/mesh-controller", 1, ctlHead2, "pass", "t1t1t1t1t1t1")) - checkerSays(r, "feat/x", "running", "", headOf("novox/mesh-controller", 1, ctlHead2), headOf("novox/mesh-catalog", 2, catHead)) + // 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. + 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) {