From 5855c41f9d868c730b0ae33f7abf4ac412a5016b Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 03:00:23 +0200 Subject: [PATCH 1/2] mesh-delivery: say a ready or rejected delivery whose check has not answered on the forge (issue 441) Ready and rejected have no bound, and the watch for unchecked heads skipped every head with a delivery, so a lost or lagging status write left a pull request unable to merge with nothing said. Such a head is now said on issue 438's rule: an unanswered line past the table's bound for a check asked and not answered. --- .../cmd/mesh-delivery/issue441_test.go | 175 ++++++++++++++++++ .../cmd/mesh-delivery/unchecked.go | 46 ++++- 2 files changed, 214 insertions(+), 7 deletions(-) create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/issue441_test.go diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue441_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue441_test.go new file mode 100644 index 00000000..ac53f0c8 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue441_test.go @@ -0,0 +1,175 @@ +package main + +import ( + "slices" + "strings" + "testing" + "time" +) + +// A delivery held in ready or rejected whose required merge check reads pending or missing on the forge +// (novox/hq issue 441). The pull requests below are mesh-controller #215's head, ready, and mesh-lab #76's, rejected, +// as gitea_open_pulls said them on 2026-10-11, with only the one status changed that the case is about: a status +// write that lagged or was lost (issue 423), or a check asked again after the verdict. + +const readyHeadSHA = "d2e9dc61590f950b74ae2e93282c354646705188" + +// readyRepoCheckPending is #215 with mesh/repo-check pending since its delivery was ready. +const readyRepoCheckPending = `{"pulls": [{ + "base": "main", "head_sha": "d2e9dc61590f950b74ae2e93282c354646705188", + "html_url": "http://127.0.0.1:20000/novox/mesh-controller/pulls/215", "number": 215, + "owner": "novox", "repo": "mesh-controller", + "required": ["mesh/merge-gate", "mesh/repo-check"], + "statuses": ["mesh/merge-gate", "mesh/repo-check", "mesh/delivery"], + "checks": { + "mesh/delivery": {"state": "success", "description": "ready: it delivers once merged", "updated_at": "2026-10-11T00:42:06Z"}, + "mesh/merge-gate": {"state": "success", "description": "pass: builds build-agent → ace, g14, novox, shanks; no bus step; every machine composes with the change as it did without (4 of 4 compose)", "updated_at": "2026-10-11T00:42:01Z"}, + "mesh/repo-check": {"state": "pending", "description": "the repository's own check runs", "updated_at": "2026-10-11T00:42:01Z"} + } +}]}` + +// readyRepoCheckMissing is #215 whose mesh/repo-check was never written. +const readyRepoCheckMissing = `{"pulls": [{ + "base": "main", "head_sha": "d2e9dc61590f950b74ae2e93282c354646705188", "number": 215, + "owner": "novox", "repo": "mesh-controller", + "required": ["mesh/merge-gate", "mesh/repo-check"], + "statuses": ["mesh/merge-gate", "mesh/delivery"], + "checks": { + "mesh/delivery": {"state": "success", "description": "ready: it delivers once merged", "updated_at": "2026-10-11T00:42:06Z"}, + "mesh/merge-gate": {"state": "success", "description": "pass", "updated_at": "2026-10-11T00:42:01Z"} + } +}]}` + +// readyAnswered is #215 as the forge said it: every required check answered. +const readyAnswered = `{"pulls": [{ + "base": "main", "head_sha": "d2e9dc61590f950b74ae2e93282c354646705188", "number": 215, + "owner": "novox", "repo": "mesh-controller", + "required": ["mesh/merge-gate", "mesh/repo-check"], + "statuses": ["mesh/merge-gate", "mesh/repo-check", "mesh/delivery"], + "checks": { + "mesh/merge-gate": {"state": "success", "description": "pass", "updated_at": "2026-10-11T00:42:01Z"}, + "mesh/repo-check": {"state": "success", "description": "pass: its merge-check.sh passed", "updated_at": "2026-10-11T00:42:01Z"} + } +}]}` + +// heldIn is a delivery of #215's head in ready (a passing verdict) or rejected (a failing one). +func heldIn(t *testing.T, w *world, gate string, want State) string { + t.Helper() + w.h.PullUpdated(pr("novox/mesh-controller", 215, readyHeadSHA, "fix/215")) + w.h.Checked(verdict("novox/mesh-controller", 215, readyHeadSHA, gate)) + id := IDOf("novox/mesh-controller", readyHeadSHA) + if s := w.state(id); s != want { + t.Fatalf("a %s verdict left it %s", gate, s) + } + return id +} + +// **A delivery in ready whose required check reads pending on the forge is said** (novox/hq issue 441) on the same +// rule as a head with no delivery (issue 438): past the table's bound for a check asked and not answered, since this +// owner first saw it so, as an unanswered line naming the check and since when. Before, any head with a delivery was +// skipped, and ready has no bound of its own, so it waited for ever — unable to merge — with nothing said. +func TestADeliveryInReadyWhoseCheckIsPendingIsSaid(t *testing.T) { + w := newWorldOver(t, newMemStore(), newFakeController(), newFakeForge(), time.Date(2026, 10, 11, 0, 43, 0, 0, time.UTC)) + id := heldIn(t, w, "pass", Ready) + w.forge.open = openPullsOf(t, readyRepoCheckPending) + w.h.SeeOpenPulls() + w.later(Bounds[Proposed].For - time.Minute) + w.h.SeeOpenPulls() + if lines := w.h.Stalled(); len(lines) != 0 { + t.Fatalf("a ready delivery still within the bound is said: %+v", lines) + } + w.later(2 * time.Minute) + lines := w.h.Stalled() + if len(lines) != 1 { + t.Fatalf("a ready delivery whose required check is pending past the bound is not said: %+v", lines) + } + l := lines[0] + if l.ID != id || l.Number != 215 || l.State != Unanswered || l.Bound != Bounds[Proposed].For.String() || l.For != "1h1m0s" { + t.Fatalf("the line is not this delivery's, past the table's bound: %+v", l) + } + if want := []string{"mesh/repo-check pending since 2026-10-11T00:42:01Z"}; !slices.Equal(l.Waiting, want) { + t.Fatalf("the line's checks waited on are %q, not %q", l.Waiting, want) + } + for _, want := range []string{"novox/mesh-controller#215", "ready", "mesh/repo-check pending since 2026-10-11T00:42:01Z", "recheck"} { + if !strings.Contains(l.Says, want) { + t.Fatalf("the line does not say %q: %s", want, l.Says) + } + } + if strings.Contains(l.Says, "No delivery of it is held") { + t.Fatalf("the line says no delivery is held of a head whose delivery is ready: %s", l.Says) + } + + // The forge's status catches up: the check answered, and the line is gone. + w.forge.open = openPullsOf(t, readyAnswered) + w.h.SeeOpenPulls() + if lines := w.h.Stalled(); len(lines) != 0 { + t.Fatalf("a ready delivery whose check answered is still said: %+v", lines) + } +} + +// **A delivery in rejected whose required check was never set is said** too: a lost status write leaves the pull +// request with no verdict to show, and nothing here asks again. It was announced — a delivery exists — so it is +// unanswered, never unannounced, though its head has no merge check at all. +func TestADeliveryInRejectedWhoseCheckIsMissingIsSaid(t *testing.T) { + w := newWorldOver(t, newMemStore(), newFakeController(), newFakeForge(), time.Date(2026, 10, 11, 0, 43, 0, 0, time.UTC)) + id := heldIn(t, w, "fail", Rejected) + w.forge.open = openPullsOf(t, strings.Replace(readyRepoCheckMissing, + `"mesh/merge-gate": {"state": "success", "description": "pass", "updated_at": "2026-10-11T00:42:01Z"}`, + `"mesh/merge-gate": {"state": "failure", "description": "fail", "updated_at": "2026-10-11T00:42:01Z"}`, 1)) + w.h.SeeOpenPulls() + w.later(Bounds[Proposed].For + time.Minute) + w.h.SeeOpenPulls() + lines := w.h.Stalled() + if len(lines) != 1 || lines[0].ID != id || lines[0].State != Unanswered || + !slices.Equal(lines[0].Waiting, []string{"mesh/repo-check never set"}) || !strings.Contains(lines[0].Says, "rejected") { + t.Fatalf("a rejected delivery whose required check was never set is not said: %+v", lines) + } + + // No merge check on its head at all — both writes lost — is still a delivery's: unanswered, never unannounced. + w2 := newWorldOver(t, newMemStore(), newFakeController(), newFakeForge(), time.Date(2026, 10, 11, 0, 43, 0, 0, time.UTC)) + heldIn(t, w2, "fail", Rejected) + w2.forge.open = openPullsOf(t, `{"pulls": [{"base": "main", "head_sha": "`+readyHeadSHA+`", "number": 215, + "owner": "novox", "repo": "mesh-controller", "required": ["mesh/merge-gate", "mesh/repo-check"], + "statuses": ["mesh/delivery"], "checks": {"mesh/delivery": {"state": "failure", "description": "rejected"}}}]}`) + w2.h.SeeOpenPulls() + if lines := w2.h.Stalled(); len(lines) != 0 { + t.Fatalf("a delivery's head without a check is said at once, before the table's bound: %+v", lines) + } + w2.later(Bounds[Proposed].For + time.Minute) + w2.h.SeeOpenPulls() + lines = w2.h.Stalled() + if len(lines) != 1 || lines[0].State != Unanswered || + !slices.Equal(lines[0].Waiting, []string{"mesh/merge-gate never set", "mesh/repo-check never set"}) { + t.Fatalf("a rejected delivery whose head has no merge check is not said as unanswered: %+v", lines) + } +} + +// **A delivery in a state with a bound of its own is not said here**: a proposed one waits on its check under +// Bounds[Proposed] and H2's re-ask, and a second line for it would say the same thing twice. +func TestAProposedDeliveryIsSaidByItsOwnBoundOnly(t *testing.T) { + w := newWorldOver(t, newMemStore(), newFakeController(), newFakeForge(), time.Date(2026, 10, 11, 0, 43, 0, 0, time.UTC)) + w.h.PullUpdated(pr("novox/mesh-controller", 215, readyHeadSHA, "fix/215")) + id := IDOf("novox/mesh-controller", readyHeadSHA) + w.forge.open = openPullsOf(t, readyRepoCheckPending) + w.h.SeeOpenPulls() + w.later(3 * Bounds[Proposed].For) + w.h.SeeOpenPulls() + for _, l := range w.h.Stalled() { + if l.State == Unanswered || l.State == Unannounced { + t.Fatalf("a proposed delivery %s is said as a head without a delivery: %+v", id, l) + } + } +} + +// **A ready delivery whose required checks answered is not said**, as the forge said every one on 2026-10-11. +func TestAReadyDeliveryWhoseChecksAnsweredIsNotSaid(t *testing.T) { + w := newWorldOver(t, newMemStore(), newFakeController(), newFakeForge(), time.Date(2026, 10, 11, 0, 43, 0, 0, time.UTC)) + heldIn(t, w, "pass", Ready) + w.forge.open = openPullsOf(t, readyAnswered) + w.h.SeeOpenPulls() + w.later(3 * Bounds[Proposed].For) + w.h.SeeOpenPulls() + if lines := w.h.Stalled(); len(lines) != 0 { + t.Fatalf("a ready delivery whose checks answered is said: %+v", lines) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go b/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go index 78acea05..2d5069c1 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go @@ -26,6 +26,14 @@ import ( // controller lets one check run (its checkTimeout, 45 minutes), so a slow check or a status read a moment behind // (issue 423) is never said. Such a head was announced — the controller set its status — so its line is not in the // state Unannounced but Unanswered, with words of its own. +// +// A head whose delivery is held in ready or rejected is said by the same rule (novox/hq issue 441). Those states have +// no bound of their own — the verdict is known here — but the forge's status is what lets the pull request merge or +// shows why not, and a status write that lagged or was lost (issue 423), or a check asked again after the verdict, +// leaves a required check pending or never set there: the pull request waits on it, and nothing here asks again. So +// such a head is kept here like one without a delivery, and said on the same bound, as Unanswered — it was announced, +// so never Unannounced, even with no merge check on its head at all. A delivery in any other state is not: proposed +// and checked wait under bounds of their own, and the rest are past the merge, their pull request no longer open. // Unannounced is a pull request's head the forge never announced: not a state of the table — no delivery exists — // only what a stalled line says. @@ -78,6 +86,15 @@ type unchecked struct { // asked again from one still waiting on the same ask. pending map[string]string first time.Time + // held is the state of the delivery of this head, ready or rejected, when one is held here (novox/hq issue + // 441); empty for a head with no delivery. + held State +} + +// saidByTheForge is whether a delivery's head is said here when its required check has not answered on the forge: +// one in ready or rejected, whose state has no bound of its own (novox/hq issue 441). No delivery at all is too. +func saidByTheForge(d *Delivery) bool { + return d == nil || d.State == Ready || d.State == Rejected } // unanswered are the merge checks the base requires that the head has no verdict of, each as it reads — "never @@ -156,13 +173,18 @@ func (h *Holder) SeeOpenPulls() { continue } id := IDOf(p.Owner+"/"+p.Repo, p.HeadSHA) - if h.deliveries[id] != nil { - continue // announced: a delivery with its own bound + d := h.deliveries[id] + if !saidByTheForge(d) { + continue // a delivery with its own bound, or past its merge + } + var held State + if d != nil { + held = d.State } // The clock runs from when this owner first saw the head waiting, and starts again only on a newer ask: a // check that started, or was asked again — its pending status set anew — is given its whole bound from then. // A check that answers does not start it again, so a head is said on the bound from its first sight. - u := unchecked{pull: p, waiting: waiting, pending: pending, first: now} + u := unchecked{pull: p, waiting: waiting, pending: pending, first: now, held: held} if was, ok := h.unchecked[id]; ok && !askedAgain(was.pending, pending) { u.first = was.first } else { @@ -174,10 +196,11 @@ func (h *Holder) SeeOpenPulls() { h.unchecked = seen } -// started is whether the head has any merge check: its check started, rather than was never asked. +// started is whether the head's check started, rather than was never asked: it has any merge check, or a delivery +// of it is held here, which the forge announced and the controller checked (novox/hq issue 441). func (u unchecked) started() bool { _, _, started := u.pull.unanswered() - return started + return started || u.held != "" } // bound is how long the head may wait as it does: UncheckedAfter for a check never asked, the table's bound for a @@ -201,8 +224,9 @@ func (u unchecked) what() string { func (h *Holder) uncheckedLines(now time.Time) []StalledLine { var out []StalledLine for id, u := range h.unchecked { - if h.deliveries[id] != nil || now.Sub(u.first) < u.bound() { - continue + d := h.deliveries[id] + if !saidByTheForge(d) || (d != nil && d.State != u.held) || now.Sub(u.first) < u.bound() { + continue // moved on since the forge was last read: the next reading says it anew } h2 := "none: the forge never announced it, so there is no delivery to close — the operator's" says := fmt.Sprintf("%s/%s#%d is open on %s, which requires the merge check, and its head has had no merge "+ @@ -217,6 +241,14 @@ func (h *Holder) uncheckedLines(now time.Time) []StalledLine { strings.Join(u.waiting, ", ")) h2 = "none: no delivery of it is held here, so there is none to close — the operator's" } + if u.held != "" { + // A delivery is held, its verdict known, and the forge shows another (novox/hq issue 441). + says = fmt.Sprintf("%s/%s#%d is open on %s, which requires the merge check, and its delivery is %s here, "+ + "but on the forge its check has not answered: %s. The pull request waits on that status and nothing "+ + "asks it again. A recheck of the delivery asks its check again, as does a new commit on its branch", + u.pull.Owner, u.pull.Repo, u.pull.Number, u.pull.Base, u.held, strings.Join(u.waiting, ", ")) + h2 = "none: its delivery is " + string(u.held) + ", a state the operator moves — the operator's" + } out = append(out, StalledLine{ID: id, Number: u.pull.Number, State: state, For: now.Sub(u.first).Round(time.Second).String(), Bound: u.bound().String(), H2: h2, Says: says, Waiting: waiting}) From 9bd8903ed23e0b66353f716817ca18604dd88b02 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 03:03:43 +0200 Subject: [PATCH 2/2] mesh-delivery: test that an unanswered line clears when its delivery moves on or its pull request closes (issue 441) The review asked for the clearing paths to be pinned: a recheck or a merge clears the line before the forge is read again, and a pull request gone from the open list is no longer said. --- .../cmd/mesh-delivery/issue441_test.go | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue441_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue441_test.go index ac53f0c8..1b6f0691 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/issue441_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue441_test.go @@ -173,3 +173,79 @@ func TestAReadyDeliveryWhoseChecksAnsweredIsNotSaid(t *testing.T) { t.Fatalf("a ready delivery whose checks answered is said: %+v", lines) } } + +// saidReady is a ready delivery of #215's head whose mesh/repo-check reads pending on the forge, said past the bound. +func saidReady(t *testing.T) (*world, string) { + t.Helper() + w := newWorldOver(t, newMemStore(), newFakeController(), newFakeForge(), time.Date(2026, 10, 11, 0, 43, 0, 0, time.UTC)) + id := heldIn(t, w, "pass", Ready) + w.forge.open = openPullsOf(t, readyRepoCheckPending) + w.h.SeeOpenPulls() + w.later(Bounds[Proposed].For + time.Minute) + if lines := w.h.Stalled(); len(lines) != 1 || lines[0].ID != id || lines[0].State != Unanswered { + t.Fatalf("a ready delivery whose check is pending past the bound is not said: %+v", lines) + } + return w, id +} + +// unansweredOf are the unanswered lines among what `stalled` says. +func unansweredOf(w *world) []StalledLine { + var out []StalledLine + for _, l := range w.h.Stalled() { + if l.State == Unanswered { + out = append(out, l) + } + } + return out +} + +// **A said line clears when its delivery's state changes** (novox/hq issue 441): a recheck proposes it again, under +// the proposed state's own bound, and a merge publishes it, past the pull request. Both clear the line at once — +// before the forge is read again, which may lag — and stay cleared once it is read. +func TestASaidLineClearsWhenItsDeliveryMovesOn(t *testing.T) { + t.Run("a recheck proposes it", func(t *testing.T) { + w, id := saidReady(t) + if _, err := w.h.Recheck(id, "its repo-check status was lost", "a person"); err != nil { + t.Fatal(err) + } + if s := w.state(id); s != Proposed { + t.Fatalf("a recheck left it %s", s) + } + if lines := unansweredOf(w); len(lines) != 0 { + t.Fatalf("a rechecked delivery, proposed again, is still said as unanswered: %+v", lines) + } + w.h.SeeOpenPulls() + if lines := unansweredOf(w); len(lines) != 0 { + t.Fatalf("a rechecked delivery is said again once the forge is read: %+v", lines) + } + }) + t.Run("a merge publishes it", func(t *testing.T) { + w, id := saidReady(t) + w.h.WalkMoved(w.ctl.put(aWalk("plan-1", "novox/mesh-controller", merge, true))) + w.h.PullMerged(merged("novox/mesh-controller", 215, readyHeadSHA, merge)) + if s := w.state(id); s != Published { + t.Fatalf("a merge left it %s", s) + } + if lines := unansweredOf(w); len(lines) != 0 { + t.Fatalf("a published delivery is still said as unanswered: %+v", lines) + } + w.h.SeeOpenPulls() // the forge's open list lags the merge: the head is still on it + if lines := unansweredOf(w); len(lines) != 0 { + t.Fatalf("a published delivery still on a lagging open list is said: %+v", lines) + } + }) +} + +// **A said line clears when its pull request leaves the open list** — closed, its event to this owner lost, so the +// delivery is still ready: the forge no longer lists it, and nothing is said of it. +func TestASaidLineClearsWhenItsPullRequestIsNoLongerOpen(t *testing.T) { + w, id := saidReady(t) + w.forge.open = nil + w.h.SeeOpenPulls() + if s := w.state(id); s != Ready { + t.Fatalf("the delivery moved to %s with no event: the case is not the one meant", s) + } + if lines := unansweredOf(w); len(lines) != 0 { + t.Fatalf("a pull request no longer open is still said: %+v", lines) + } +}