From 9d766b675d0f81d93d58914fbe5f2a7e5065b6a3 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 02:16:34 +0200 Subject: [PATCH] mesh-delivery: say a head whose merge check started and never answered (issue 438) The watch for unchecked heads counted any merge-check status as checked, pending included, so a head left with a pending mesh/merge-gate, or never given mesh/repo-check, waited unable to merge with nothing saying so. gitea_open_pulls now gives each head status with its state and time, and the watch says a head whose required check stays pending or unset past the delivery table's bound for a check asked and not answered. --- modules/gitea/README.md | 3 + modules/gitea/client.ts | 28 +++- modules/gitea/test/announcing.test.ts | 6 +- modules/gitea/tools/index.ts | 2 +- .../cmd/mesh-delivery/issue438_test.go | 147 ++++++++++++++++++ .../cmd/mesh-delivery/unchecked.go | 134 +++++++++++++--- 6 files changed, 292 insertions(+), 28 deletions(-) create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/issue438_test.go diff --git a/modules/gitea/README.md b/modules/gitea/README.md index bf775099..08c51f0f 100644 --- a/modules/gitea/README.md +++ b/modules/gitea/README.md @@ -18,6 +18,9 @@ Every new head of an open pull request is announced as `pull.updated`, once, and `mesh-delivery` module asks it every five minutes, and a head on a base requiring `mesh/merge-gate` or `mesh/repo-check` with neither for ten minutes since it first saw it so is a delivery held past its bound (`delivery..stalled`, state `unannounced`), which the controller raises like any other (issue 347). + Each of the head's statuses comes with its state and when it was set, so a head whose check started and never + answered — a required check pending, or never set beside one that is — is said the same way once that has lasted + the delivery table's bound for a check asked and not answered, an hour (issue 438). - **How to see it work.** A pull request opened more than a minute after its branch was pushed — this fix's own was one, and the announcer it fixes never saw it — has `mesh/merge-gate` pending within a pass of the next sweep at the latest, and `mesh-controller.conditions` names none for it. diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index eac04eb6..eb3879b1 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -81,6 +81,16 @@ export interface OpenPull { required: string[]; /** The contexts the head has a status of, in any state. */ statuses: string[]; + /** The head's statuses by context, the newest of each, with its state (novox/hq issue 438): what tells a check + * that started and never answered — pending — from one that answered. */ + checks: Record; +} + +/** One status on a head, the newest of its context: its state, what it said, and when it was set. */ +export interface HeadCheck { + state: string; + description?: string; + updated_at?: string; } /** A commit status, as the forge keeps it: what a pull request shows beside its head commit. */ @@ -323,9 +333,10 @@ export class GiteaClient { if (pull.state !== "open" || !pull.head_sha) continue; if (!rules.has(full)) rules.set(full, await this.branchProtections(found.owner, found.name).catch(() => undefined)); const rule = rules.get(full)?.find((p) => (p.rule_name ?? p.branch_name) === (pull.base || "main")); - const statuses = Object.keys(await this.commitStatuses(found.owner, found.name, pull.head_sha)); + const checks = await this.headChecks(found.owner, found.name, pull.head_sha); out.push({ owner: found.owner, repo: found.name, number: pull.number, base: pull.base ?? "", head_sha: pull.head_sha, - html_url: pull.html_url, required: rule?.enable_status_check ? (rule.status_check_contexts ?? []) : [], statuses }); + html_url: pull.html_url, required: rule?.enable_status_check ? (rule.status_check_contexts ?? []) : [], + statuses: Object.keys(checks), checks }); } return out; } @@ -441,6 +452,19 @@ export class GiteaClient { return out; } + /** A head's statuses by context, the newest of each, with state, description and time (novox/hq issue 438): + * commitStatuses' read, keeping what a watch needs to say a check pending and since when. */ + async headChecks(owner: string, repo: string, sha: string): Promise> { + const raw = await this.request(`${repoPath(owner, repo)}/commits/${commitSegment(sha)}/status`); + const out: Record = {}; + for (const s of raw?.statuses ?? []) { + if (!s?.context || out[s.context]) continue; + out[s.context] = { state: String(s.status ?? s.state ?? ""), description: String(s.description ?? ""), + updated_at: String(s.updated_at ?? s.created_at ?? "") }; + } + return out; + } + /** A commit's statuses whole, as the forge combines them for a ref (a sha, or the start of one): the commit * it resolved to and every status with its description, link, times and setter (novox/hq ADR 0239). */ async combinedStatus(owner: string, repo: string, ref: string): Promise<{ sha: string; state: string; statuses: unknown[] }> { diff --git a/modules/gitea/test/announcing.test.ts b/modules/gitea/test/announcing.test.ts index a1299d7e..19ab6f8a 100644 --- a/modules/gitea/test/announcing.test.ts +++ b/modules/gitea/test/announcing.test.ts @@ -54,7 +54,8 @@ test("every open pull request is read with what its base requires and what its h return send([{ rule_name: "main", enable_status_check: true, status_check_contexts: ["mesh/merge-gate", "mesh/repo-check"] }]); } if (url.includes("/commits/aaa1111/status")) return send({ statuses: [] }); - if (url.includes("/commits/bbb2222/status")) return send({ statuses: [{ context: "mesh/merge-gate", status: "pending" }] }); + if (url.includes("/commits/bbb2222/status")) return send({ statuses: [{ context: "mesh/merge-gate", status: "pending", + description: "the mesh is mapping this head onto its module graph", updated_at: "2026-10-11T00:11:39Z" }] }); res.statusCode = 404; res.end("{}"); }); @@ -66,6 +67,9 @@ test("every open pull request is read with what its base requires and what its h ["novox", "hq", 243, "main", "aaa1111", ["mesh/repo-check"], []], ["novox", "mesh-sdk", 14, "main", "bbb2222", ["mesh/merge-gate", "mesh/repo-check"], ["mesh/merge-gate"]], ]); + // Each status with its state and time (novox/hq issue 438): a check pending is told from one that answered. + assert.deepEqual(open.map((p) => p.checks), [{}, { "mesh/merge-gate": { state: "pending", + description: "the mesh is mapping this head onto its module graph", updated_at: "2026-10-11T00:11:39Z" } }]); } finally { server.close(); } diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 3f0638af..ce5985a3 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -617,7 +617,7 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { }, { name: "gitea_open_pulls", - description: "Every open pull request of every repository, from the forge itself: its head, its base, the statuses the base's protection requires and the ones its head has, in any state. What mesh-delivery reads to say a pull request the mesh was never asked to check (novox/hq issue 347). Reads only.", + description: "Every open pull request of every repository, from the forge itself: its head, its base, the statuses the base's protection requires and the ones its head has, each with its state, description and when it was set. What mesh-delivery reads to say a pull request the mesh was never asked to check (novox/hq issue 347), or whose check started and never answered (issue 438). Reads only.", input: {}, run: async () => ({ pulls: await gitea.openPulls() }), }, diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue438_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue438_test.go new file mode 100644 index 00000000..7b78e952 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue438_test.go @@ -0,0 +1,147 @@ +package main + +import ( + "encoding/json" + "strings" + "testing" + "time" +) + +// openPullsOf reads gitea_open_pulls' answer as the forge module gives it, so the tests read the real shape: the +// pull requests below are mesh-controller #212's and #211's heads and mesh-catalog #194's, as the forge said them on +// 2026-10-11 (novox/hq issue 438), with only their numbers and shas as they were. +func openPullsOf(t *testing.T, answer string) []OpenPull { + t.Helper() + var open struct { + Pulls []OpenPull `json:"pulls"` + } + if err := json.Unmarshal([]byte(answer), &open); err != nil { + t.Fatal(err) + } + return open.Pulls +} + +// A head the mesh's check started on and never answered: mesh/merge-gate pending since the controller began mapping +// it, mesh/repo-check never set. Its base requires both. +const startedNeverAnswered = `{"pulls": [{ + "base": "main", "head_sha": "c11222026a3b26a0cc4b277119e1ea059b82a36b", + "html_url": "http://127.0.0.1:20000/novox/mesh-controller/pulls/212", "number": 212, + "owner": "novox", "repo": "mesh-controller", + "required": ["mesh/merge-gate", "mesh/repo-check"], + "statuses": ["mesh/merge-gate", "mesh/delivery"], + "checks": { + "mesh/delivery": {"state": "pending", "description": "checking", "updated_at": "2026-10-11T00:11:46Z"}, + "mesh/merge-gate": {"state": "pending", "description": "the mesh is mapping this head onto its module graph", + "updated_at": "2026-10-11T00:11:39Z"} + } +}]}` + +// **A head whose merge check started and never answered is said** (novox/hq issue 438): pending, or missing a check +// its base requires, for as long as the delivery table gives a check asked and not answered (Bounds[Proposed]) since +// this owner first saw it so, it is a stalled line naming each check that did not answer and since when. Before, +// any merge-check status counted as checked, pending included, and such a head was never said. +func TestAHeadWhoseCheckStartedAndNeverAnsweredIsSaid(t *testing.T) { + w := newWorldOver(t, newMemStore(), newFakeController(), newFakeForge(), time.Date(2026, 10, 11, 0, 12, 5, 0, time.UTC)) + w.forge.open = openPullsOf(t, startedNeverAnswered) + w.h.SeeOpenPulls() + w.later(Bounds[Proposed].For - time.Minute) + w.h.SeeOpenPulls() + if lines := w.h.Stalled(); len(lines) != 0 { + t.Fatalf("a head still within the bound of a check asked and not answered is said: %+v", lines) + } + w.later(2 * time.Minute) + lines := w.h.Stalled() + if len(lines) != 1 { + t.Fatalf("a head whose check started and never answered is not said: %+v", lines) + } + l := lines[0] + if l.ID != IDOf("novox/mesh-controller", "c11222026a3b26a0cc4b277119e1ea059b82a36b") || l.Number != 212 || + l.State != Unannounced || l.Bound != Bounds[Proposed].For.String() || l.For != "1h1m0s" { + t.Fatalf("the line is not this head's, past the table's bound: %+v", l) + } + for _, want := range []string{"mesh/merge-gate", "pending since 2026-10-11T00:11:39Z", "mesh/repo-check", "never set", + "never answered"} { + if !strings.Contains(l.Says, want) { + t.Fatalf("the line does not say %q: %s", want, l.Says) + } + } + if strings.Contains(l.Says, "mesh/delivery") { + t.Fatalf("the line names a status that is not the merge check: %s", l.Says) + } + + // Announced at last: a delivery of its own, with its own bound — no longer this line. + w.h.PullUpdated(pr("novox/mesh-controller", 212, "c11222026a3b26a0cc4b277119e1ea059b82a36b", "fix/402")) + w.h.SeeOpenPulls() + for _, l := range w.h.Stalled() { + if l.State == Unannounced { + t.Fatalf("an announced head is still said unannounced: %+v", l) + } + } +} + +// A check asked again — its pending status set anew, as a recheck or a new announcement does — starts the bound again: +// the head is said only once that ask too has gone unanswered past it. +func TestACheckAskedAgainStartsItsBoundAgain(t *testing.T) { + w := newWorldOver(t, newMemStore(), newFakeController(), newFakeForge(), time.Date(2026, 10, 11, 0, 12, 5, 0, time.UTC)) + w.forge.open = openPullsOf(t, startedNeverAnswered) + w.h.SeeOpenPulls() + w.later(50 * time.Minute) + again := openPullsOf(t, strings.Replace(startedNeverAnswered, "2026-10-11T00:11:39Z", "2026-10-11T01:02:00Z", 1)) + w.forge.open = again + w.h.SeeOpenPulls() + w.later(20 * time.Minute) + w.h.SeeOpenPulls() + if lines := w.h.Stalled(); len(lines) != 0 { + t.Fatalf("a check asked again twenty minutes ago is said on the first ask's time: %+v", lines) + } + w.later(41 * time.Minute) + if lines := w.h.Stalled(); len(lines) != 1 || !strings.Contains(lines[0].Says, "pending since 2026-10-11T01:02:00Z") { + t.Fatalf("a check asked again and not answered past the bound is not said: %+v", lines) + } +} + +// **A head whose required checks all answered is not said**, whatever they answered: a failure is a verdict, and the +// pull request says it itself. Nor is one whose only pending check is one its base does not require: hq requires +// mesh/repo-check alone. +func TestAHeadWhoseRequiredChecksAnsweredIsNotSaid(t *testing.T) { + w := newWorldOver(t, newMemStore(), newFakeController(), newFakeForge(), time.Date(2026, 10, 11, 0, 12, 5, 0, time.UTC)) + w.forge.open = openPullsOf(t, `{"pulls": [{ + "base": "main", "head_sha": "99df6b999bad77c51dfdd13470cf57b1f38e8c24", "number": 194, "owner": "novox", "repo": "mesh-catalog", + "required": ["mesh/merge-gate", "mesh/repo-check"], + "statuses": ["mesh/merge-gate", "mesh/repo-check", "mesh/delivery"], + "checks": { + "mesh/delivery": {"state": "failure", "description": "rejected", "updated_at": "2026-10-11T00:05:37Z"}, + "mesh/merge-gate": {"state": "success", "description": "pass: builds mounts → shanks; no bus step; every machine composes with the change as it did without (4 of 4 compose)", "updated_at": "2026-10-11T00:05:28Z"}, + "mesh/repo-check": {"state": "failure", "description": "fail: its merge-check.sh failed", "updated_at": "2026-10-11T00:05:28Z"} + } +}, { + "base": "main", "head_sha": "11b719fd5cdec43970e513a10a44a558042599e6", "number": 308, "owner": "novox", "repo": "hq", + "required": ["mesh/repo-check"], + "statuses": ["mesh/merge-gate", "mesh/repo-check", "mesh/delivery"], + "checks": { + "mesh/merge-gate": {"state": "pending", "description": "the mesh is mapping this head onto its module graph", "updated_at": "2026-10-11T00:01:00Z"}, + "mesh/repo-check": {"state": "success", "description": "pass: its merge-check.sh passed", "updated_at": "2026-10-11T00:02:00Z"} + } +}]}`) + w.h.SeeOpenPulls() + w.later(3 * Bounds[Proposed].For) + w.h.SeeOpenPulls() + if lines := w.h.Stalled(); len(lines) != 0 { + t.Fatalf("a head whose required checks answered is said: %+v", lines) + } +} + +// A forge module that says only which contexts a head has, not their states — the gitea module before issue 438 — +// is read as before: a head with any merge check is not said, and nothing is guessed of what it does not say. +func TestAForgeThatSaysNoStatesIsReadAsBefore(t *testing.T) { + w := newWorld(t) + w.forge.open = openPullsOf(t, `{"pulls": [{"base": "main", "head_sha": "c11222026a3b26a0cc4b277119e1ea059b82a36b", + "number": 212, "owner": "novox", "repo": "mesh-controller", "required": ["mesh/merge-gate", "mesh/repo-check"], + "statuses": ["mesh/merge-gate", "mesh/delivery"]}]}`) + w.h.SeeOpenPulls() + w.later(3 * Bounds[Proposed].For) + w.h.SeeOpenPulls() + if lines := w.h.Stalled(); len(lines) != 0 { + t.Fatalf("a head is said on states the forge did not give: %+v", lines) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go b/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go index e8c67ed7..9f1ea3ba 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go @@ -3,6 +3,7 @@ package main import ( "fmt" "slices" + "strings" "time" ) @@ -15,6 +16,15 @@ import ( // since this owner first saw it so is said as a delivery held: a `stalled` line in the state Unannounced, which the // controller raises as `delivery..stalled` (D14) like any other delivery past its bound. The delivery domain's own // word, so no second condition and no second judge: the id is the one its delivery will have once announced. +// +// A head whose check started and never answered is said the same way (novox/hq issue 438). Counting any merge-check +// status as checked let a head that got only a pending mesh/merge-gate, or never got mesh/repo-check, wait unable to +// merge with nothing saying so. So a head is checked only when every merge check its base requires has answered — +// success, failure, error or warning, a failure being a verdict the pull request shows itself — and one with a +// required check pending or never set is said once that has lasted the delivery table's bound for a check asked and +// not answered (Bounds[Proposed]): an announced head would be said after that bound too, and it is longer than the +// 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. // 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. @@ -29,7 +39,8 @@ const UncheckedAfter = 10 * time.Minute var mergeChecks = []string{"mesh/merge-gate", "mesh/repo-check"} // OpenPull is an open pull request as the forge says it (the gitea module's gitea_open_pulls): its head, its base, -// the contexts the base's protection requires, and the contexts its head has a status of, in any state. +// the contexts the base's protection requires, the contexts its head has a status of, in any state, and each of +// those statuses with its state. type OpenPull struct { Owner string `json:"owner"` Repo string `json:"repo"` @@ -39,24 +50,64 @@ type OpenPull struct { HTMLURL string `json:"html_url,omitempty"` Required []string `json:"required"` Statuses []string `json:"statuses"` + // Checks are the head's statuses by context, the newest of each (novox/hq issue 438). A forge module older + // than that says none: the head is then read by Statuses alone, as before, and a pending check is not told + // from an answered one. + Checks map[string]HeadCheck `json:"checks,omitempty"` } -// unchecked is a head seen without a merge check, and when this owner first saw it so — its own time, since the -// forge's update time moves on every comment. +// HeadCheck is one status on a head as the forge keeps it: pending, success, failure, error or warning. +type HeadCheck struct { + State string `json:"state"` + Description string `json:"description,omitempty"` + UpdatedAt string `json:"updated_at,omitempty"` +} + +// unchecked is a head seen without an answered merge check, what it lacks, and when this owner first saw it so — +// its own time, since the forge's update time moves on every comment. type unchecked struct { - pull OpenPull - first time.Time + pull OpenPull + waiting []string + first time.Time } -// wantsACheck is whether the base requires the mesh's merge check and the head has none of it. -func (p OpenPull) wantsACheck() bool { - required := slices.ContainsFunc(mergeChecks, func(c string) bool { return slices.Contains(p.Required, c) }) - checked := slices.ContainsFunc(mergeChecks, func(c string) bool { return slices.Contains(p.Statuses, c) }) - return required && !checked && p.HeadSHA != "" +// unanswered are the merge checks the base requires that the head has no verdict of, each as it reads — "never +// set", or pending and since when — and whether the head has any merge check at all: its check started. A head on +// a base that requires none, or without a head, has nothing unanswered. +func (p OpenPull) unanswered() (waiting []string, started bool) { + if p.HeadSHA == "" { + return nil, false + } + started = slices.ContainsFunc(mergeChecks, func(c string) bool { + return slices.Contains(p.Statuses, c) || p.Checks[c].State != "" + }) + for _, c := range mergeChecks { + if !slices.Contains(p.Required, c) { + continue + } + got, ok := p.Checks[c] + switch { + case p.Checks == nil && started: + // A forge that says no states: a head with any merge check counts as checked, as before issue 438, + // since nothing here says whether the one it has answered. + case !ok || got.State == "": + waiting = append(waiting, c+" never set") + case got.State == "pending": + waiting = append(waiting, c+" pending since "+orUnknown(got.UpdatedAt)) + } + } + return waiting, started } -// SeeOpenPulls asks the forge for every open pull request and keeps the heads that want a check and have none. -// A forge that cannot be asked changes nothing: what was seen stands, and nothing is guessed. +func orUnknown(at string) string { + if at == "" { + return "a time the forge did not say" + } + return at +} + +// SeeOpenPulls asks the forge for every open pull request and keeps the heads with a required merge check not +// answered. A forge that cannot be asked changes nothing: what was seen stands, and nothing is guessed. func (h *Holder) SeeOpenPulls() { open, err := h.Forge.OpenPulls() if err != nil { @@ -68,38 +119,73 @@ func (h *Holder) SeeOpenPulls() { now := h.Now() seen := map[string]unchecked{} for _, p := range open { - if !p.wantsACheck() { + waiting, _ := p.unanswered() + if len(waiting) == 0 { continue } id := IDOf(p.Owner+"/"+p.Repo, p.HeadSHA) if h.deliveries[id] != nil { continue // announced: a delivery with its own bound } + // The clock runs from when this owner first saw the head waiting as it waits now: a check that started, or + // was asked again — its pending status set anew — is given its whole bound from then. first := now - if was, ok := h.unchecked[id]; ok { + if was, ok := h.unchecked[id]; ok && slices.Equal(was.waiting, waiting) { first = was.first } else { - h.Logf("[mesh-delivery] %s/%s#%d's head %s has no merge check; said if it has none in %s", p.Owner, p.Repo, - p.Number, short(p.HeadSHA), UncheckedAfter) + u := unchecked{pull: p, waiting: waiting} + h.Logf("[mesh-delivery] %s/%s#%d's head %s %s; said if so in %s", p.Owner, p.Repo, p.Number, + short(p.HeadSHA), u.what(), u.bound()) } - seen[id] = unchecked{pull: p, first: first} + seen[id] = unchecked{pull: p, waiting: waiting, first: first} } h.unchecked = seen } -// uncheckedLines are the heads unchecked past UncheckedAfter, as stalled lines; under the lock. +// started is whether the head has any merge check: its check started, rather than was never asked. +func (u unchecked) started() bool { + _, started := u.pull.unanswered() + return started +} + +// bound is how long the head may wait as it does: UncheckedAfter for a check never asked, the table's bound for a +// check asked and not answered for one that started (novox/hq issue 438). +func (u unchecked) bound() time.Duration { + if u.started() { + return Bounds[Proposed].For + } + return UncheckedAfter +} + +// what is what the head lacks, in words. +func (u unchecked) what() string { + if u.started() { + return "has a merge check that has not answered: " + strings.Join(u.waiting, ", ") + } + return "has no merge check" +} + +// uncheckedLines are the heads unchecked past their bound, as stalled lines; under the lock. 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) < UncheckedAfter { + if h.deliveries[id] != nil || now.Sub(u.first) < u.bound() { continue } + 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 "+ + "check: the mesh was never asked to check it. A new commit on its branch announces it", + u.pull.Owner, u.pull.Repo, u.pull.Number, u.pull.Base) + if u.started() { + says = fmt.Sprintf("%s/%s#%d is open on %s, which requires the merge check, and its head's check started "+ + "and never answered: %s. No delivery of it is held here, so nothing asks it again. A new commit on "+ + "its branch announces it and asks its check", u.pull.Owner, u.pull.Repo, u.pull.Number, u.pull.Base, + strings.Join(u.waiting, ", ")) + h2 = "none: no delivery of it is held here, so there is none to close — the operator's" + } out = append(out, StalledLine{ID: id, Number: u.pull.Number, State: Unannounced, For: now.Sub(u.first).Round(time.Second).String(), - Bound: UncheckedAfter.String(), - 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 "+ - "check: the mesh was never asked to check it. A new commit on its branch announces it", - u.pull.Owner, u.pull.Repo, u.pull.Number, u.pull.Base)}) + Bound: u.bound().String(), + H2: h2, Says: says}) } return out }