From 6327f297ea30cf72925151905c3c16e0ddfcb739 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 02:57:18 +0200 Subject: [PATCH] Cover a held merge only when the forge says a later delivered merge contains it (review of #150) Merge order on a protected trunk was the whole proof; the review asked for ancestry. The gitea module gains gitea_contains, a read-only compare of two commits (total_commits 0 is contained), and mesh-delivery asks it once per pair, off the lock like its plan ask, keeping the answer on the delivery. A forge that cannot say leaves the merge held, not covered. Merge order stays the cheap first filter. A later delivery now carries a module only when its own check passed (so a merge held without a passing check is closed only by one that passed), its walk is of its own merge commit on the same trunk, and its steps say the module reached every machine running it, not one. The words say what is known: a later merge that contains it was delivered. --- modules/gitea/checks.ts | 17 +++ modules/gitea/client.ts | 6 + modules/gitea/test/checks.test.ts | 17 +++ modules/gitea/tools/index.ts | 20 +++- .../cmd/mesh-delivery/covered_test.go | 91 +++++++++++++-- .../cmd/mesh-delivery/effects.go | 2 +- .../cmd/mesh-delivery/fakes_test.go | 21 +++- .../mesh-delivery/cmd/mesh-delivery/holder.go | 106 ++++++++++++++---- .../cmd/mesh-delivery/manifest_test.go | 2 +- .../mesh-delivery/cmd/mesh-delivery/model.go | 25 +++-- .../mesh-delivery/cmd/mesh-delivery/ports.go | 20 ++++ .../mesh-delivery/cmd/mesh-delivery/table.go | 18 +-- modules/mesh-delivery/module.json | 3 +- 13 files changed, 292 insertions(+), 56 deletions(-) diff --git a/modules/gitea/checks.ts b/modules/gitea/checks.ts index 672ec03b..9208fcc7 100644 --- a/modules/gitea/checks.ts +++ b/modules/gitea/checks.ts @@ -81,3 +81,20 @@ export function mergeableUnder(branch: string, rule: BranchProtection | null | u blocking.map((b) => `${b.context} (${b.state})`).join(", "); return { branch, protected: !!rule, required, blocking, mergeable: blocking.length === 0, says }; } + +/** A commit, by its sha or the start of one: what `gitea_contains` takes, so a branch name or an expression + * never reaches the compare's path. */ +export function isCommit(ref: string): boolean { + return /^[0-9a-f]{7,40}$/i.test(ref); +} + +/** Whether a commit is contained in another — an ancestor of it, or it (novox/hq ADR 0270) — from the forge's + * compare of `in...commit`: the commits on `commit` that `in` lacks. None is contained. An answer that does not + * say how many is no answer. */ +export function containedFrom(compare: unknown): { contains: boolean; ahead: number } { + const total = (compare as Record | null)?.total_commits; + if (typeof total !== "number" || !Number.isInteger(total) || total < 0) { + throw new Error("the forge's compare said no total_commits"); + } + return { contains: total === 0, ahead: total }; +} diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index fe8e4234..10577d13 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -355,6 +355,12 @@ export class GiteaClient { return { sha: String(raw?.sha ?? ""), state: String(raw?.state ?? ""), statuses: Array.isArray(raw?.statuses) ? raw.statuses : [] }; } + /** The forge's compare of two commits, `base...head`: the commits on head that base lacks, and how many + * (novox/hq ADR 0270). Reads only. */ + async compare(owner: string, repo: string, base: string, head: string): Promise { + return this.request(`/repos/${owner}/${repo}/compare/${encodeURIComponent(base)}...${encodeURIComponent(head)}`); + } + /** Replace a comment's body: the delivery's view, kept current in place (novox/hq ADR 0239). */ async editComment(owner: string, repo: string, id: number, body: string): Promise<{ id: number; html_url: string }> { const c = await this.request(`/repos/${owner}/${repo}/issues/comments/${id}`, { method: "PATCH", body: JSON.stringify({ body }) }); diff --git a/modules/gitea/test/checks.test.ts b/modules/gitea/test/checks.test.ts index b29062bb..a6dee70a 100644 --- a/modules/gitea/test/checks.test.ts +++ b/modules/gitea/test/checks.test.ts @@ -46,3 +46,20 @@ test("no rule requires nothing; a rule that cannot be read says nothing, never y assert.equal(unread.mergeable, null); assert.match(unread.says, /could not be read/); }); + +// novox/hq ADR 0270: a held delivery is covered only when a later delivered merge contains it. +test("a commit is contained in another when the compare finds nothing on it the other lacks", async () => { + const { containedFrom, isCommit } = await import("../checks.ts"); + for (const [compare, contains, ahead] of [ + [{ total_commits: 0, commits: [] }, true, 0], + [{ total_commits: 3, commits: [{}, {}, {}] }, false, 3], + ] as const) { + assert.deepEqual(containedFrom(compare), { contains, ahead }); + } + for (const bad of [{}, { total_commits: "0" }, { total_commits: -1 }, null]) { + assert.throws(() => containedFrom(bad), /total_commits/); + } + assert.equal(isCommit("6aabe8e"), true); + assert.equal(isCommit("90516fc0a1b2c3d4e5f60718293a4b5c6d7e8f90"), true); + for (const bad of ["main", "6aabe8e...90516fc", "../x", "6aabe8", ""]) assert.equal(isCommit(bad), false, bad); +}); diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index f18281d8..33b70d41 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -12,7 +12,7 @@ import { registerModuleTools, type ToolDefinition } from "@novox/mesh-sdk/tools" import { emit } from "@novox/mesh-sdk/events"; import { GiteaClient, type BranchProtection } from "../client.js"; import { appendNote, deliveryStatus, run, viewBody, viewComment } from "../delivery.js"; -import { mergeableUnder, newestByContext } from "../checks.js"; +import { containedFrom, isCommit, mergeableUnder, newestByContext } from "../checks.js"; /** The forge's container, where its repositories and git are: the note is written there (novox/hq ADR 0239). */ const forgeContainer = process.env.MESH_GITEA_CONTAINER || "gitea"; @@ -492,6 +492,24 @@ export function getGiteaTools(gitea: GiteaClient): ToolDefinition[] { merge: mergeableUnder(base, rule, statuses, unreadable) }; }, }, + { + name: "gitea_contains", + description: "Whether a commit is contained in another — an ancestor of it, or the same — from the forge's compare of the two: what mesh-delivery reads before it says a held merge is covered by a later one (novox/hq ADR 0270). Commits only, by sha or the start of one. Reads only.", + input: { + owner: { type: "string", description: "the repository owner" }, + repo: { type: "string", description: "the repository name" }, + commit: { type: "string", description: "the commit asked about" }, + in: { type: "string", description: "the commit it may be contained in" }, + }, + run: async (args) => { + const owner = String(args.owner ?? "").trim(), repo = String(args.repo ?? "").trim(); + const commit = String(args.commit ?? "").trim(), within = String(args.in ?? "").trim(); + if (!owner || !repo) throw new Error("name the repository: owner and repo"); + if (!isCommit(commit) || !isCommit(within)) throw new Error("commit and in are commits: a sha, or the start of one"); + const { contains, ahead } = containedFrom(await gitea.compare(owner, repo, within, commit)); + return { commit, in: within, contains, ahead }; + }, + }, { name: "gitea_commit_status", description: "Set one of the mesh's statuses on a commit (mesh/delivery, mesh/delivery-group): pending, success, error, failure or warning, a short description, and the page it links to. The merge check's statuses (mesh/merge-gate, mesh/repo-check) take pending only — a recheck putting them back until the fresh verdict sets them.", diff --git a/modules/mesh-delivery/cmd/mesh-delivery/covered_test.go b/modules/mesh-delivery/cmd/mesh-delivery/covered_test.go index 62686249..8ec081e4 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/covered_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/covered_test.go @@ -133,19 +133,33 @@ func TestAMergeALaterDeliveredWalkCarriedIsCovered(t *testing.T) { modules []string // Whether this one was held for a person before the later merge was delivered. heldFirst bool - want State + // What the forge says of the later merge containing this one: yes, no, or away until asked again. + forge string + want State }{ {"carried every module, delivered after this one was held", "main", 35, walkDone, - []string{"build-agent", "mesh-controller"}, true, Covered}, + []string{"build-agent", "mesh-controller"}, true, "yes", Covered}, {"carried every module, delivered before this one was due to be held", "main", 35, walkDone, - []string{"build-agent", "mesh-controller", "route-proxy"}, false, Covered}, - {"carried one of its two modules", "main", 35, walkDone, []string{"mesh-controller"}, true, Held}, - {"merged before this one", "main", 25, walkDone, []string{"build-agent", "mesh-controller"}, true, Held}, - {"merged into another branch", "release", 35, walkDone, []string{"build-agent", "mesh-controller"}, true, Held}, - {"its walk failed", "main", 35, walkFailed, []string{"build-agent", "mesh-controller"}, true, Held}, + []string{"build-agent", "mesh-controller", "route-proxy"}, false, "yes", Covered}, + {"carried one of its two modules", "main", 35, walkDone, []string{"mesh-controller"}, true, "yes", Held}, + {"merged before this one", "main", 25, walkDone, []string{"build-agent", "mesh-controller"}, true, "yes", Held}, + {"merged into another branch", "release", 35, walkDone, []string{"build-agent", "mesh-controller"}, true, "yes", Held}, + {"its walk failed", "main", 35, walkFailed, []string{"build-agent", "mesh-controller"}, true, "yes", Held}, + {"the forge says the later merge does not contain it", "main", 35, walkDone, + []string{"build-agent", "mesh-controller"}, true, "no", Held}, + {"the forge could not say at first, then said it does", "main", 35, walkDone, + []string{"build-agent", "mesh-controller"}, true, "away", Covered}, } { t.Run(c.name, func(t *testing.T) { w := newWorld(t) + pair := merge + " in nnnnnnnnnnnn8888" + switch c.forge { + case "yes", "away": + w.forge.contains[pair] = true + case "no": + w.forge.contains[pair] = false + } + w.forge.containsDown = c.forge == "away" p := pr(ctl, 1, head, "feat/one", "cmd/a.go") w.h.PullUpdated(p) w.h.Checked(verdictMoving(ctl, 1, head, "mesh-controller", "build-agent")) @@ -172,13 +186,25 @@ func TestAMergeALaterDeliveredWalkCarriedIsCovered(t *testing.T) { t.Fatal(err) } w.settleAll() + if c.forge == "away" { + // A forge that cannot say is "not yet covered": held, and asked again a minute on. + if s := w.state(IDOf(ctl, head)); s != Held { + t.Fatalf("with the forge away it is %s", s) + } + w.forge.mu.Lock() + w.forge.containsDown = false + w.forge.mu.Unlock() + w.later(2 * time.Minute) + w.settleAll() + } d := w.delivery(IDOf(ctl, head)) if d.State != c.want { t.Fatalf("it is %s (%s), want %s", d.State, d.HeldWhy, c.want) } s := statusOn(w, ctl, head) if c.want == Covered { - if !strings.HasPrefix(s, "success covered") || !strings.Contains(s, "nnnnnnnn") { + if !strings.HasPrefix(s, "success covered: a later merge that contains it was delivered") || + !strings.Contains(s, "nnnnnnnn") { t.Fatalf("its pull request's mesh/delivery says %q", s) } last := d.Transitions[len(d.Transitions)-1] @@ -189,6 +215,22 @@ func TestAMergeALaterDeliveredWalkCarriedIsCovered(t *testing.T) { if strings.HasPrefix(s, "error") { t.Fatalf("its pull request's mesh/delivery is an error: %q", s) } + // Asked once for the pair, however many ticks read it — once more only after the forge was away. + asked := 0 + for _, a := range w.forge.containsAsks { + if a == pair { + asked++ + } + } + candidate := c.base == "main" && c.at > 30 && c.state == walkDone && len(c.modules) >= 2 + switch { + case !candidate && asked != 0: + t.Fatalf("the forge was asked whether %s for a merge order and walk that cannot cover it", pair) + case candidate && c.forge != "away" && asked != 1: + t.Fatalf("the forge was asked %d time(s) whether %s, want once", asked, pair) + case candidate && c.forge == "away" && asked < 2: + t.Fatalf("the forge, away, was asked %d time(s) whether %s: never again", asked, pair) + } }) } } @@ -212,6 +254,7 @@ func TestACoveredGroupMemberLetsTheNextOneGo(t *testing.T) { w.h.PullMerged(mergedAt(pl, "m2m2m2m2m2m2", t0.Add(31*time.Minute))) w.h.WalkMoved(w.ctl.put(aWalk("plan-lab", lab, "m2m2m2m2m2m2", true))) w.settleAll() + w.forge.contains["m1m1m1m1m1m1 in m3m3m3m3m3m3"] = true deliverLater(w, ctl, 3, "main", "e3e3e3e3e3e3e3e3", "m3m3m3m3m3m3", t0.Add(35*time.Minute), walkDone, "mesh-controller") w.later(20 * time.Minute) if err := w.h.Reconcile(); err != nil { @@ -241,7 +284,7 @@ func TestHeldDeliveriesReadBackCloseByThemselvesOnce(t *testing.T) { coverer := func(repo, module string) *Delivery { return &Delivery{ID: IDOf(repo, "c0c0c0c0c0c0c0c0"), Repository: repo, Commit: "c0c0c0c0c0c0c0c0", Number: 90, Base: "main", State: Delivered, Since: merged2.Add(time.Hour), Created: merged2.Add(-time.Hour), - MergedAs: "f0f0f0f0f0f0f0f0", MergedAt: merged2, Plan: planMoving(repo, module), + MergedAs: "f0f0f0f0f0f0f0f0", MergedAt: merged2, Plan: planMoving(repo, module), Check: &Verdict{Gate: "pass"}, Walk: &WalkSeen{ID: "plan-90", Commit: "f0f0f0f0f0f0f0f0", State: walkDone}, Steps: []Step{{Machine: "anchor", Module: module, State: StepPassed}, {Machine: "the rest running it", Module: module, State: StepSent}}} } @@ -252,6 +295,12 @@ func TestHeldDeliveriesReadBackCloseByThemselvesOnce(t *testing.T) { } const unwalkedWhy = "merged, and the controller opened no walk for it within 10m0s — nothing it holds follows that branch, " + "or the merge was not heard: a person decides" + withCoverer := func(d *Delivery, change func(c *Delivery)) []*Delivery { + c := coverer(d.Repository, d.Plan.Moved[0]) + change(c) + return []*Delivery{d, c} + } + const uncheckedWhy = "merged without a passing check: only a person decides that it goes on" for _, c := range []struct { name string kept []*Delivery @@ -266,6 +315,24 @@ func TestHeldDeliveriesReadBackCloseByThemselvesOnce(t *testing.T) { nil, Delivered}, {"no walk opened, a later delivered walk carried its module", []*Delivery{held(ctl, planMoving(ctl, "mesh-controller"), unwalkedWhy), coverer(ctl, "mesh-controller")}, nil, Covered}, + {"merged without a passing check, a later delivered walk whose own check passed carried its module", + withCoverer(held(ctl, planMoving(ctl, "mesh-controller"), uncheckedWhy), func(*Delivery) {}), nil, Covered}, + {"merged without a passing check, the later delivery's own check did not pass", + withCoverer(held(ctl, planMoving(ctl, "mesh-controller"), uncheckedWhy), func(c *Delivery) { + c.Check = &Verdict{Gate: "fail"} + }), nil, Held}, + {"no walk opened, the later delivery's walk was of another commit", + withCoverer(held(ctl, planMoving(ctl, "mesh-controller"), unwalkedWhy), func(c *Delivery) { + c.Walk.Commit = "e0e0e0e0e0e0e0e0" + }), nil, Held}, + {"no walk opened, the later walk sent its module to its first machine only", + withCoverer(held(ctl, planMoving(ctl, "mesh-controller"), unwalkedWhy), func(c *Delivery) { + c.Steps = c.Steps[:1] + }), nil, Held}, + {"no walk opened, the later walk's branch is another", + withCoverer(held(ctl, planMoving(ctl, "mesh-controller"), unwalkedWhy), func(c *Delivery) { + c.Walk.Branch = "release" + }), nil, Held}, {"no walk opened, a later delivered walk carried another module", []*Delivery{held(ctl, planMoving(ctl, "mesh-controller"), unwalkedWhy), coverer(ctl, "route-proxy")}, nil, Held}, {"held for its group, its own walk superseded, a later delivered walk carried its module", @@ -298,7 +365,9 @@ func TestHeldDeliveriesReadBackCloseByThemselvesOnce(t *testing.T) { } } now := merged2.Add(30 * time.Hour) - w := newWorldOver(t, store, newFakeController(), newFakeForge(), now) + forge := newFakeForge() + forge.contains[merge+" in f0f0f0f0f0f0f0f0"] = true + w := newWorldOver(t, store, newFakeController(), forge, now) if err := w.h.Reconcile(); err != nil { t.Fatal(err) } @@ -312,7 +381,7 @@ func TestHeldDeliveriesReadBackCloseByThemselvesOnce(t *testing.T) { t.Fatalf("its pull request's mesh/delivery says %q", s) } // A restart over what it kept: nothing moves again. - again := newWorldOver(t, store, newFakeController(), newFakeForge(), now.Add(time.Hour)) + again := newWorldOver(t, store, newFakeController(), forge, now.Add(time.Hour)) if err := again.h.Reconcile(); err != nil { t.Fatal(err) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/effects.go b/modules/mesh-delivery/cmd/mesh-delivery/effects.go index 83fb8c84..84780839 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/effects.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/effects.go @@ -108,7 +108,7 @@ func statusOf(d *Delivery) (string, string) { case Retired: return "success", "history: merged before this owner delivered anything, never walked" case Covered: - return "success", clip("covered: "+d.CoveredBy, 140) + return "success", clip("covered: a later merge that contains it was delivered: "+d.CoveredBy, 140) } return "pending", string(d.State) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go index 1ecacccf..26a02a39 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go @@ -235,11 +235,15 @@ type fakeForge struct { required []string protectionDown bool asked []string + // contains answers gitea_contains by " in "; a pair it does not hold is a forge that cannot say. + contains map[string]bool + containsDown bool + containsAsks []string } func newFakeForge() *fakeForge { return &fakeForge{notes: map[string][]string{}, views: map[string]string{}, statuses: map[string]string{}, - pulls: map[string]ForgePull{}} + pulls: map[string]ForgePull{}, contains: map[string]bool{}} } func (f *fakeForge) Note(owner, repo, commit, line string) error { @@ -347,6 +351,21 @@ func (f *fakeForge) Statuses(owner, repo string, number int, commit, base string return out, nil } +// Contains answers what the fake was told of two commits. +func (f *fakeForge) Contains(owner, repo, commit, in string) (bool, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.containsAsks = append(f.containsAsks, commit+" in "+in) + if f.containsDown || f.down { + return false, errors.New("the forge is away") + } + yes, said := f.contains[commit+" in "+in] + if !said { + return false, fmt.Errorf("the forge has no commit %s or %s", commit, in) + } + return yes, nil +} + func (f *fakeForge) notesOn(commit string) []string { f.mu.Lock() defer f.mu.Unlock() diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index a5114dce..060c3c2a 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -764,7 +764,13 @@ func (h *Holder) plan() []func() { continue } if d.CoveredBy == "" { - d.CoveredBy = h.coverOf(d) + var ask []string + d.CoveredBy, ask = h.coverOf(d) + for _, in := range ask { + if a := h.askContains(d, in, now); a != nil { + asks = append(asks, a) + } + } } if ts := settle(d, Facts{Now: now}); len(ts) > 0 { h.moved(d, ts) @@ -819,26 +825,42 @@ func (h *Holder) unwalkedPast(d *Delivery, now time.Time) bool { now.Sub(d.MergedAt) > unwalked && h.reconciled.After(d.MergedAt.Add(unwalked)) } -// coverOf says which later merges cover a merged delivery, or nothing (novox/hq issue 319): for every module -// its plan moves, a delivered delivery of the same repository and trunk, merged after it — so its merge commit -// contains this one's, the trunk being protected and only ever merged into — whose finished walk sent that -// module. Read from this owner's own records: the forge's merge times and the walks' records. -func (h *Holder) coverOf(d *Delivery) string { +// coverOf says which later merges cover a merged delivery, or nothing, and which later merge commits the forge +// must still be asked about (novox/hq ADR 0270). A module is covered by a later delivery that carries it (see +// carries) and whose merge commit the forge said contains this one's merge. Merge order is the cheap filter; +// ancestry decides. The forge is asked only once every module has a candidate, once per pair, and its answer +// is kept on the delivery: an answer not known yet is "not yet covered". +func (h *Holder) coverOf(d *Delivery) (string, []string) { if d.MergedAs == "" || d.MergedAt.IsZero() || d.Base == "" || d.Plan == nil || len(d.Plan.Moved) == 0 { - return "" + return "", nil + } + candidates := map[string][]*Delivery{} + for _, module := range d.Plan.Moved { + for _, c := range h.deliveries { + if carries(c, d, module) { + candidates[module] = append(candidates[module], c) + } + } + if len(candidates[module]) == 0 { + return "", nil // nothing later carried it: nothing to ask + } } carried := map[string][]string{} var by []*Delivery + var ask []string for _, module := range d.Plan.Moved { var first *Delivery - for _, c := range h.deliveries { - if carries(c, d, module) && (first == nil || c.MergedAt.Before(first.MergedAt) || - c.MergedAt.Equal(first.MergedAt) && c.ID < first.ID) { + for _, c := range candidates[module] { + yes, known := d.ContainedIn[c.MergedAs] + if !known && !containsString(ask, c.MergedAs) { + ask = append(ask, c.MergedAs) + } + if yes && (first == nil || c.MergedAt.Before(first.MergedAt) || c.MergedAt.Equal(first.MergedAt) && c.ID < first.ID) { first = c } } if first == nil { - return "" + return "", ask } if carried[first.ID] == nil { by = append(by, first) @@ -850,29 +872,67 @@ func (h *Holder) coverOf(d *Delivery) string { for _, c := range by { mods := carried[c.ID] sort.Strings(mods) - parts = append(parts, fmt.Sprintf("delivered with %s (merged as %s, walk %s): %s", c.ID, shortOf(c.MergedAs, 8), + parts = append(parts, fmt.Sprintf("%s (merged as %s into %s, walk %s): %s", c.ID, shortOf(c.MergedAs, 8), d.Base, c.Walk.ID, strings.Join(mods, ", "))) } - return fmt.Sprintf("%s — merged into %s after %s, which it therefore contains", strings.Join(parts, "; "), d.Base, - shortOf(d.MergedAs, 8)) + return strings.Join(parts, "; "), nil } -// carries is whether c delivered a module after d, from a commit that contains d's merge. +// carries is whether c delivered a module after d: of the same repository, merged later into the same trunk, +// delivered on its own passing check — a merge held without one is covered only by a later merge that passed +// its own — by its own walk, of its merge commit on that trunk, which sent the module to every machine running +// it. Whether c's merge commit contains d's is the forge's to say (coverOf). func carries(c, d *Delivery, module string) bool { - if c.ID == d.ID || c.State != Delivered || !strings.EqualFold(c.Repository, d.Repository) || + if c.ID == d.ID || c.State != Delivered || !c.Check.Passes() || !strings.EqualFold(c.Repository, d.Repository) || !strings.EqualFold(c.Base, d.Base) || c.MergedAs == "" || sameCommit(c.MergedAs, d.MergedAs) || - !c.MergedAt.After(d.MergedAt) || c.Walk == nil || c.Walk.State != walkDone { + !c.MergedAt.After(d.MergedAt) || c.Walk == nil || c.Walk.State != walkDone || !sameCommit(c.Walk.Commit, c.MergedAs) || + c.Walk.Branch != "" && !strings.EqualFold(c.Walk.Branch, d.Base) { return false } - if containsString(c.Walk.Sent, module) { - return true - } + everywhere := false for _, s := range c.Steps { - if s.Module == module && (s.State == StepPassed || s.State == StepSent) { - return true + if s.Module != module { + continue + } + switch { + case s.State == StepFailed || s.State == StepRolledBack: + return false + case s.State == StepSent && (s.Machine == stepEveryMachine || s.Machine == stepTheRest): + everywhere = true } } - return false + return everywhere +} + +// askContains is the ask of the forge whether a later merge commit contains a delivery's merge: at most once a +// minute while the forge cannot say, and never again once it said — its answer is kept on the delivery. +func (h *Holder) askContains(d *Delivery, in string, now time.Time) func() { + key := d.ID + " in " + in + if tried, was := h.askTried[key]; was && now.Sub(tried) < time.Minute { + return nil + } + h.askTried[key] = now + id, owner, repo, merged := d.ID, d.Owner(), d.Repo(), d.MergedAs + return func() { + yes, err := h.Forge.Contains(owner, repo, merged, in) + h.mu.Lock() + defer h.mu.Unlock() + if err != nil { + h.Logf("[mesh-delivery] %s: whether %s contains its merge cannot be read yet (%v): not yet covered", id, + shortOf(in, 12), err) + return + } + delete(h.askTried, key) + d := h.deliveries[id] + if d == nil { + return + } + if d.ContainedIn == nil { + d.ContainedIn = map[string]bool{} + } + d.ContainedIn[in] = yes + h.keep(d) + } } // heldForGroup is whether a member is held only because its group's composed check had not passed. diff --git a/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go b/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go index 05989ba2..43a2c526 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go @@ -65,7 +65,7 @@ func TestItClaimsTheDeliverySeatAndFollowsWhatItHandles(t *testing.T) { want = append(want, "seat:"+ControllerSeat+"."+v) } want = append(want, "gitea.gitea_note_append", "gitea.gitea_delivery_view", "gitea.gitea_commit_status", - "gitea.gitea_commit_statuses") + "gitea.gitea_commit_statuses", "gitea.gitea_contains") if !reflect.DeepEqual(m.Invokes, want) { t.Fatalf("invokes %v, wanted %v", m.Invokes, want) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/model.go b/modules/mesh-delivery/cmd/mesh-delivery/model.go index fa98d32f..ef31ee77 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/model.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/model.go @@ -65,6 +65,9 @@ type Delivery struct { // (novox/hq issue 319); Lifted why what held it for a person no longer holds (issue 316). CoveredBy string `json:"covered_by,omitempty"` Lifted string `json:"lifted,omitempty"` + // ContainedIn is the forge's answer, by a later merge commit, to whether that commit contains this one's + // merge (gitea_contains): asked once per pair, and kept (ADR 0270). + ContainedIn map[string]bool `json:"contained_in,omitempty"` // Walk is the controller's walk of its trunk commit, as last read; Steps the machines it reached. Walk *WalkSeen `json:"walk,omitempty"` Steps []Step `json:"steps,omitempty"` @@ -343,8 +346,8 @@ type WalkSeen struct { Tier int `json:"tier"` Tiers int `json:"tiers"` Updated time.Time `json:"updated"` - // Sent are the modules the walk sent to a machine, as its record says. - Sent []string `json:"sent,omitempty"` + // Branch is the branch the walk's commit was merged into; empty in a record kept before it was read. + Branch string `json:"branch,omitempty"` } // Seen is a walk as a delivery keeps it. @@ -357,12 +360,7 @@ func (w Walk) Seen() *WalkSeen { s.GoBy = d.By s.StoppedBy = d.Stopped } - for name, m := range w.Modules { - if m != nil && (m.SentAt != nil || m.FirstAt != nil) { - s.Sent = append(s.Sent, name) - } - } - sort.Strings(s.Sent) + s.Branch = w.Branch return s } @@ -379,6 +377,13 @@ func (s *WalkSeen) LetGoByAPerson() bool { return s != nil && strings.HasPrefix( // Open is whether the controller still works it. func (w Walk) Open() bool { return w.State == walkBuilding || w.State == walkRolling } +// The steps that say a module reached every machine running it: sent to all at once, or to the rest after its +// first machines passed. +const ( + stepEveryMachine = "every machine running it" + stepTheRest = "the rest running it" +) + // StepsOf are the machine steps a walk's record says, for one delivery: every module it moved, on its first // machines from its gate, and the rest once sent. func StepsOf(w Walk) []Step { @@ -392,7 +397,7 @@ func StepsOf(w Walk) []Step { m := w.Modules[name] if m == nil || m.FirstAt == nil { if m != nil && m.SentAt != nil && m.FirstAt == nil && m.Why == "" { - out = append(out, Step{Machine: "every machine running it", Module: name, Build: m.Build, + out = append(out, Step{Machine: stepEveryMachine, Module: name, Build: m.Build, State: StepSent, Since: *m.SentAt}) } continue @@ -419,7 +424,7 @@ func StepsOf(w Walk) []Step { out = append(out, Step{Machine: machine, Module: name, Build: m.Build, State: state, Since: since, Why: why}) } if m.SentAt != nil && state == StepPassed { - out = append(out, Step{Machine: "the rest running it", Module: name, Build: m.Build, State: StepSent, + out = append(out, Step{Machine: stepTheRest, Module: name, Build: m.Build, State: StepSent, Since: *m.SentAt}) } } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ports.go b/modules/mesh-delivery/cmd/mesh-delivery/ports.go index 8b775503..be2bb752 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ports.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ports.go @@ -76,6 +76,8 @@ type Forge interface { // Statuses reads a commit's statuses — a pull request's head's, given its number — and whether the branch // it merges into would let it merge. Reads only. Statuses(owner, repo string, number int, commit, base string) (*ForgeChecks, error) + // Contains is whether a commit is contained in another — an ancestor of it, or it. Reads only. + Contains(owner, repo, commit, in string) (bool, error) } // ForgeChecks is what the forge says of a commit's checks (the gitea module's gitea_commit_statuses). @@ -337,6 +339,24 @@ func (f toolForge) Statuses(owner, repo string, number int, commit, base string) return &c, nil } +func (f toolForge) Contains(owner, repo, commit, in string) (bool, error) { + raw, err := f.ask("gitea.gitea_contains", map[string]any{"owner": owner, "repo": repo, "commit": commit, "in": in}) + if err != nil { + return false, err + } + answer, output, ok := answerOf(raw) + if !ok { + return false, fmt.Errorf("the forge refused gitea_contains: %s", lastLine(output)) + } + var a struct { + Contains *bool `json:"contains"` + } + if err := json.Unmarshal(answer, &a); err != nil || a.Contains == nil { + return false, fmt.Errorf("the forge's gitea_contains is not readable: %s", clip(string(answer), 200)) + } + return *a.Contains, nil +} + // kvStore is the module's own state: `deliveries` and `groups`, one key each. type kvStore struct{} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/table.go b/modules/mesh-delivery/cmd/mesh-delivery/table.go index df43b3e3..e554cabf 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/table.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/table.go @@ -34,10 +34,11 @@ const ( // replayed from the bus when it first held the seat. It reached the machines before this owner existed, // and nothing is delivered, released or put back for it (novox/hq issue 313). Retired State = "retired" - // Covered is a merge whose change reached the machines through a later one: every module it moves was - // sent by a finished walk of a commit merged after it into the same trunk, which therefore contains it. It - // is not walked itself, and it is no failure: its pull request says covered, as a success (novox/hq issue - // 319). + // Covered is a merge a later one that contains it delivered: every module it moves was sent to every machine + // running it by the finished walk of a later merge into the same trunk, whose own check passed and which the + // forge says contains this one. It is not walked itself, and it is no failure: its pull request says covered, + // as a success (novox/hq ADR 0270). A revert merged in between is still possible: covered says what was + // delivered, not what runs. Covered State = "covered" ) @@ -182,8 +183,9 @@ var Table = []Row{ }, Says: func(d *Delivery) string { return "merged, and nothing for a walk to move: " + d.Plan.Summary }}, {From: []State{Ready, Held}, Event: EvCovered, To: Covered, - Guard: "merged, no walk of its own open, and every module it moves sent by a finished walk of a commit merged " + - "after it into the same trunk — which contains it, the trunk being protected: its change already runs", + Guard: "merged, no walk of its own open, and every module it moves sent to every machine running it by the " + + "finished walk of a later merge into the same trunk, whose own check passed and which the forge says contains " + + "it: a later merge that contains it was delivered", Holds: func(d *Delivery, f Facts) error { switch { case d.MergedAs == "" || d.CoveredBy == "": @@ -193,7 +195,9 @@ var Table = []Row{ } return nil }, - Says: func(d *Delivery) string { return "covered: " + d.CoveredBy }}, + Says: func(d *Delivery) string { + return "covered: a later merge that contains it was delivered: " + d.CoveredBy + }}, {From: []State{Proposed, Checked, Rejected}, Event: EvMergedUnchecked, To: Held, Guard: "merged without a passing check — a verdict known with it is taken first: only a person decides that it goes on", Holds: func(d *Delivery, f Facts) error { return unless(d.MergedAs != "", "not merged") }}, diff --git a/modules/mesh-delivery/module.json b/modules/mesh-delivery/module.json index 570e766f..a165b473 100644 --- a/modules/mesh-delivery/module.json +++ b/modules/mesh-delivery/module.json @@ -43,7 +43,8 @@ "gitea.gitea_note_append", "gitea.gitea_delivery_view", "gitea.gitea_commit_status", - "gitea.gitea_commit_statuses" + "gitea.gitea_commit_statuses", + "gitea.gitea_contains" ], "state": [ "deliveries",