From 4e14ef9835e007e28e02d2387d1e84f486cb851c Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 16:48:52 +0200 Subject: [PATCH] mesh-delivery: cover a held merge whose module a backlog walk carried to every machine (hq issue 351) A build the controller held back for a person reaches its machines by a backlog walk on the operator's word, which credits the delivery that published the build one step per machine and never the walk's own "every machine running it". Its last record keeps no gate, only the machines it is done on, so the credited steps stayed judging for ever, and `covered` read only the aggregate steps, so the held merge had no end but a person's wrong act (the catalogue's #125, held 28 hours after hq ADR 0270 went live). A finished backlog walk now passes the steps it carried on the machines it is done on; `carries` counts a module as sent everywhere when a passed step stands on every machine the held delivery's plan sends it to; and a walk that carried a step still judging is asked for by its id, since one that ended before the holder's restart may be older than the ended walks the controller lists back. Three tests fail before and pass after. --- .../cmd/mesh-delivery/covered_test.go | 194 ++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/holder.go | 96 ++++++++- .../mesh-delivery/cmd/mesh-delivery/model.go | 15 ++ 3 files changed, 301 insertions(+), 4 deletions(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/covered_test.go b/modules/mesh-delivery/cmd/mesh-delivery/covered_test.go index 8ec081e4..ef43241b 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/covered_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/covered_test.go @@ -474,3 +474,197 @@ func TestAMemberHeldForItsComposedCheckIsLiftedWhenItPasses(t *testing.T) { }) } } + +// **A module a release carried to every machine the held delivery sends it to covers it** (novox/hq issue +// 351). A build the controller held back for a person and released on their word reaches each machine by a +// backlog walk, which credits the delivery that published the build one step per machine — never the walk's +// own "every machine running it" or "the rest running it". The held delivery's plan says which machines +// receive the module: a passed step on each of them is the module sent everywhere. A machine still judging, +// one that failed, or a plan that names no machines leaves it held, as before. +func TestAModuleAReleaseCarriedToEveryMachineCovers(t *testing.T) { + const cat = "novox/mesh-catalog" + merged1, merged2 := t0, t0.Add(2*time.Hour) + laptop, desktop := "laptop", "desktop" + plan := func(machines ...string) *DeliveryPlan { + p := planMoving(cat, "openrazer") + for _, m := range machines { + p.Machines = append(p.Machines, MachinePlan{Machine: m, Receives: []string{"openrazer"}}) + } + return p + } + held := func(p *DeliveryPlan) *Delivery { + why := "its group feat/j's composed check did not pass for the heads that merged; a person decides that it goes on" + return &Delivery{ID: IDOf(cat, head), Repository: cat, Commit: head, Number: 7, Base: "main", State: Held, + Since: merged1.Add(10 * time.Minute), Created: merged1.Add(-time.Hour), MergedAs: merge, MergedAt: merged1, + HeldWhy: why, Plan: p, Walk: &WalkSeen{ID: "plan-7", Commit: merge, State: walkSuperseded, Waits: true, WaitedFor: "mesh-delivery"}, + Transitions: []Transition{{At: merged1, From: Published, To: Held, Event: EvHold, Why: why}}} + } + step := func(machine string, state StepState) Step { + return Step{Machine: machine, Module: "openrazer", Build: "build-1", State: state, Why: "carried by release-1"} + } + coverer := func(steps ...Step) *Delivery { + return &Delivery{ID: IDOf(cat, "c0c0c0c0c0c0c0c0"), Repository: cat, Commit: "c0c0c0c0c0c0c0c0", Number: 90, + Base: "main", State: Delivered, Since: merged2.Add(time.Hour), Created: merged2.Add(-time.Hour), + MergedAs: "f0f0f0f0f0f0f0f0", MergedAt: merged2, Plan: planMoving(cat, "power"), Check: &Verdict{Gate: "pass"}, + Walk: &WalkSeen{ID: "plan-90", Commit: "f0f0f0f0f0f0f0f0", State: walkDone}, + Steps: append([]Step{{Machine: "anchor", Module: "power", State: StepPassed}, + {Machine: "the rest running it", Module: "power", State: StepSent}}, steps...)} + } + for _, c := range []struct { + name string + held *Delivery + by *Delivery + want State + }{ + {"passed on every machine its plan sends it to", held(plan(laptop, desktop)), + coverer(step(laptop, StepPassed), step(desktop, StepPassed)), Covered}, + {"still judging on one of them", held(plan(laptop, desktop)), + coverer(step(laptop, StepPassed), step(desktop, StepJudging)), Held}, + {"failed on one of them", held(plan(laptop, desktop)), + coverer(step(laptop, StepPassed), step(desktop, StepFailed)), Held}, + {"never sent to one of them", held(plan(laptop, desktop)), coverer(step(laptop, StepPassed)), Held}, + {"its plan names no machines", held(plan()), coverer(step(laptop, StepPassed), step(desktop, StepPassed)), Held}, + } { + t.Run(c.name, func(t *testing.T) { + store := newMemStore() + for _, d := range []*Delivery{c.held, c.by} { + if err := store.PutDelivery(d); err != nil { + t.Fatal(err) + } + } + forge := newFakeForge() + forge.contains[merge+" in f0f0f0f0f0f0f0f0"] = true + w := newWorldOver(t, store, newFakeController(), forge, merged2.Add(30*time.Hour)) + if err := w.h.Reconcile(); err != nil { + t.Fatal(err) + } + w.settleAll() + d := w.delivery(c.held.ID) + if d.State != c.want { + t.Fatalf("it is %s (%s), want %s", d.State, d.HeldWhy, c.want) + } + if c.want == Covered { + if s := statusOn(w, cat, head); !strings.HasPrefix(s, "success covered: a later merge that contains it was delivered") { + t.Fatalf("its pull request's mesh/delivery says %q", s) + } + if last := d.Transitions[len(d.Transitions)-1]; !strings.Contains(last.Why, "openrazer") { + t.Fatalf("its transition does not name the module: %q", last.Why) + } + } + }) + } +} + +// **A finished release moves the steps it carried to passed** (novox/hq issue 351): a backlog walk's gate +// credits a step per machine while it judges, and its last record keeps no gate — only which machines it is +// done on. A step credited to it, on a machine it is done on, is passed; a step it failed on, one on a machine +// it is not done on, and a step another walk carried are left as they are. +func TestAFinishedReleaseMovesTheStepsItCarriedToPassed(t *testing.T) { + const cat = "novox/mesh-catalog" + carried := func(machine string, state StepState, by string) Step { + return Step{Machine: machine, Module: "openrazer", Build: "build-1", State: state, Why: "carried by " + by} + } + for _, c := range []struct { + name string + steps []Step + done []string + want map[string]StepState + }{ + {"done on both", []Step{carried("laptop", StepJudging, "release-1"), carried("desktop", StepJudging, "release-1")}, + []string{"laptop", "desktop"}, map[string]StepState{"laptop": StepPassed, "desktop": StepPassed}}, + {"done on one, the other still to come", []Step{carried("laptop", StepJudging, "release-1"), carried("desktop", StepJudging, "release-1")}, + []string{"laptop"}, map[string]StepState{"laptop": StepPassed, "desktop": StepJudging}}, + {"a step it failed on stays failed", []Step{carried("laptop", StepFailed, "release-1")}, + []string{"laptop"}, map[string]StepState{"laptop": StepFailed}}, + {"a step another walk carried is not its", []Step{carried("laptop", StepJudging, "release-2")}, + []string{"laptop"}, map[string]StepState{"laptop": StepJudging}}, + } { + t.Run(c.name, func(t *testing.T) { + d := &Delivery{ID: IDOf(cat, "c0c0c0c0c0c0c0c0"), Repository: cat, Commit: "c0c0c0c0c0c0c0c0", Number: 90, + Base: "main", State: Delivered, Since: t0.Add(time.Hour), Created: t0, MergedAs: "f0f0f0f0f0f0f0f0", MergedAt: t0, + Plan: planMoving(cat, "power"), Check: &Verdict{Gate: "pass"}, + Walk: &WalkSeen{ID: "plan-90", Commit: "f0f0f0f0f0f0f0f0", State: walkDone}, Steps: c.steps} + store := newMemStore() + if err := store.PutDelivery(d); err != nil { + t.Fatal(err) + } + w := newWorldOver(t, store, newFakeController(), newFakeForge(), t0.Add(2*time.Hour)) + w.h.WalkMoved(Walk{ID: "release-1", Repository: "the builds waiting for a gate", State: walkDone, + Tiers: [][]string{{"openrazer"}}, Modules: map[string]*WalkModule{}, + Release: &WalkRelease{Order: []string{"laptop", "desktop"}, Next: len(c.done), Done: c.done}}) + got := map[string]StepState{} + for _, s := range w.delivery(d.ID).Steps { + got[s.Machine] = s.State + } + for machine, want := range c.want { + if got[machine] != want { + t.Fatalf("openrazer on %s is %s, want %s (steps %v)", machine, got[machine], want, got) + } + } + }) + } +} + +// The day's case whole (novox/hq issue 351): a member held for its group's check, its own walk taken over by +// later merges; the one later walk that carried its module failed its gate; a later merge's walk was delivered +// without the module; the controller then held the module's build back for a person, who released it, and the +// release reached both machines. The held delivery is covered by that later merge once the release is done — +// read by its id, since a walk that ended before the holder's restart may be older than the ended walks the +// controller lists back. +func TestAHeldDeliveryWhoseModuleAReleaseCarriedIsCoveredOnceTheReleaseIsDone(t *testing.T) { + const cat = "novox/mesh-catalog" + merged1, merged2 := t0, t0.Add(2*time.Hour) + plan := planMoving(cat, "openrazer") + plan.Machines = []MachinePlan{{Machine: "laptop", Receives: []string{"openrazer"}}, {Machine: "desktop", Receives: []string{"openrazer"}}} + why := "its group feat/j's composed check did not pass for the heads that merged; a person decides that it goes on" + held := &Delivery{ID: IDOf(cat, head), Repository: cat, Commit: head, Number: 7, Base: "main", State: Held, + Since: merged1.Add(10 * time.Minute), Created: merged1.Add(-time.Hour), MergedAs: merge, MergedAt: merged1, + HeldWhy: why, Plan: plan, Walk: &WalkSeen{ID: "plan-7", Commit: merge, State: walkSuperseded, Waits: true, WaitedFor: "mesh-delivery"}, + Transitions: []Transition{{At: merged1, From: Published, To: Held, Event: EvHold, Why: why}}} + later := &Delivery{ID: IDOf(cat, "c0c0c0c0c0c0c0c0"), Repository: cat, Commit: "c0c0c0c0c0c0c0c0", Number: 90, + Base: "main", State: Delivered, Since: merged2.Add(time.Hour), Created: merged2.Add(-time.Hour), + MergedAs: "f0f0f0f0f0f0f0f0", MergedAt: merged2, Plan: planMoving(cat, "power"), Check: &Verdict{Gate: "pass"}, + Walk: &WalkSeen{ID: "plan-90", Commit: "f0f0f0f0f0f0f0f0", State: walkDone}, + Steps: []Step{{Machine: "anchor", Module: "power", State: StepPassed}, {Machine: "the rest running it", Module: "power", State: StepSent}}} + store := newMemStore() + for _, d := range []*Delivery{held, later} { + if err := store.PutDelivery(d); err != nil { + t.Fatal(err) + } + } + forge := newFakeForge() + forge.contains[merge+" in f0f0f0f0f0f0f0f0"] = true + w := newWorldOver(t, store, newFakeController(), forge, merged2.Add(3*time.Hour)) + if err := w.h.Reconcile(); err != nil { + t.Fatal(err) + } + w.settleAll() + if s := w.state(held.ID); s != Held { + t.Fatalf("before any release it is %s", s) + } + // The release judges the build on each machine in turn: its gate names what it carried. + release := Walk{ID: "release-1", Repository: "the builds waiting for a gate", State: walkRolling, + Tiers: [][]string{{"openrazer"}}, Modules: map[string]*WalkModule{}, + Release: &WalkRelease{Order: []string{"laptop", "desktop"}, Gate: &WalkGate{Machines: []string{"laptop"}, + Carried: []WalkCarry{{Module: "openrazer", Node: "laptop", To: "f0f0f0f0f0f0f0f0", Build: "build-1"}, + {Module: "openrazer", Node: "desktop", To: "f0f0f0f0f0f0f0f0", Build: "build-1"}}}}} + w.h.WalkMoved(release) + w.settleAll() + if s := w.state(held.ID); s != Held { + t.Fatalf("while the release judges it is %s", s) + } + // Done on both: its last record keeps no gate. It ended while this holder was down, and is older than the + // ended walks the controller lists back: the holder asks for it by its id, since a step it carried still judges. + release.State, release.Release.Gate, release.Release.Done, release.Release.Next = walkDone, nil, []string{"laptop", "desktop"}, 2 + w.ctl.put(release) + w.later(time.Hour) // a walk a step still judges is asked for once an hour; at a start, at once + w.settleAll() + d := w.delivery(held.ID) + if d.State != Covered { + t.Fatalf("once the release is done it is %s (%s), want covered", d.State, d.HeldWhy) + } + if last := d.Transitions[len(d.Transitions)-1]; !strings.Contains(last.Why, IDOf(cat, "c0c0c0c0c0c0c0c0")) || + !strings.Contains(last.Why, "openrazer") { + t.Fatalf("its transition does not name what covered it: %q", last.Why) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 0ed12ca6..5cbe008c 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -40,7 +40,9 @@ type Holder struct { unreached string // askTried is when a check this owner asks was last tried and could not be asked: tried again a minute on. askTried map[string]time.Time - refusals []string + // walkAsked is when a walk that carried a step still judging was last asked for by its id: once an hour. + walkAsked map[string]time.Time + refusals []string // unchecked are the open pull requests' heads seen without a merge check (unchecked.go). unchecked map[string]unchecked } @@ -82,6 +84,7 @@ func (h *Holder) init() { h.dirtyGroup = map[string]bool{} h.unmatched = map[string]unmatched{} h.askTried = map[string]time.Time{} + h.walkAsked = map[string]time.Time{} h.unchecked = map[string]unchecked{} h.started = h.Now() } @@ -646,6 +649,58 @@ func (h *Holder) creditBacklog(w Walk) { if w.Release.Gate != nil { h.creditCarried(w.Release.Gate, w.ID, "") } + if w.State == walkDone { + h.creditReleased(w) + } +} + +// creditReleased passes the steps a finished backlog walk carried: its last record keeps no gate, only the +// machines it is done on, and a release ends done only when its gate passed on each of them (novox/hq issue +// 351). A step it failed on, and a step another walk carried, are left as they are. +func (h *Holder) creditReleased(w Walk) { + why := carriedBy + w.ID + for _, d := range h.deliveries { + moved := false + for _, s := range d.Steps { + if s.Why != why || !containsString(w.Release.Done, s.Machine) || s.State != StepSent && s.State != StepJudging { + continue + } + s.State, s.Since = StepPassed, h.Now() + if h.step(d, s) { + moved = true + } + } + if moved { + h.keep(d) + } + } +} + +// carriedBy begins the why of a step another walk's send carried, followed by that walk's id. +const carriedBy = "carried by " + +// askWalk is the ask of one walk's record by its id, once an hour while a step it carried still judges; +// what it answers is taken as said. +func (h *Holder) askWalk(id string, now time.Time) func() { + if asked, was := h.walkAsked[id]; was && now.Sub(asked) < time.Hour { + return nil + } + h.walkAsked[id] = now + return func() { + _, walks, err := h.Controller.Walks(id) + h.mu.Lock() + defer h.mu.Unlock() + if err != nil { + h.Logf("[mesh-delivery] the walk %s, which carried a step still judging, cannot be read yet: %v", id, err) + h.walkAsked[id] = h.Now().Add(time.Minute - time.Hour) // asked again a minute on + return + } + for _, w := range walks { + if w.ID == id { + h.walkMoved(w) + } + } + } } // creditCarried gives the delivery that published each carried build its machine step. @@ -665,7 +720,7 @@ func (h *Holder) creditCarried(g *WalkGate, walk, except string) { continue } if h.step(d, Step{Machine: c.Node, Module: c.Module, Build: c.Build, State: state, Since: h.Now(), - Why: "carried by " + walk}) { + Why: carriedBy + walk}) { h.keep(d) } } @@ -781,6 +836,20 @@ func (h *Holder) plan() []func() { } } + // A step a walk carried and left judging: that walk's record is asked for by its id. A walk that ended + // while this holder was down may be older than the ended walks the controller lists back at its start, + // and its end is what passes the step (novox/hq issue 351). + for _, d := range h.deliveries { + for _, s := range d.Steps { + if s.State != StepSent && s.State != StepJudging || !strings.HasPrefix(s.Why, carriedBy) { + continue + } + if a := h.askWalk(strings.TrimPrefix(s.Why, carriedBy), now); a != nil { + asks = append(asks, a) + } + } + } + // Merged, and no walk opened for it after the walks were read again: never silent — held, saying so. for _, d := range h.deliveries { if h.unwalkedPast(d, now) { @@ -884,7 +953,11 @@ func (h *Holder) coverOf(d *Delivery) (string, []string) { // 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). +// it. Its machine steps say so in one of two ways: the walk's own record, which sends to every machine at once +// or to the rest after its first machines passed; or, for a build the controller held back and a person +// released, one step per machine credited by the backlog walk that carried it — passed on every machine d's +// plan sends the module to (novox/hq issue 351). 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 || !c.Check.Passes() || !strings.EqualFold(c.Repository, d.Repository) || !strings.EqualFold(c.Base, d.Base) || c.MergedAs == "" || sameCommit(c.MergedAs, d.MergedAs) || @@ -893,6 +966,7 @@ func carries(c, d *Delivery, module string) bool { return false } everywhere := false + passed := map[string]bool{} for _, s := range c.Steps { if s.Module != module { continue @@ -902,9 +976,23 @@ func carries(c, d *Delivery, module string) bool { return false case s.State == StepSent && (s.Machine == stepEveryMachine || s.Machine == stepTheRest): everywhere = true + case s.State == StepPassed: + passed[s.Machine] = true } } - return everywhere + if everywhere { + return true + } + machines := d.Plan.receiving(module) + if len(machines) == 0 { + return false + } + for _, m := range machines { + if !passed[m] { + return false + } + } + return true } // askContains is the ask of the forge whether a later merge commit contains a delivery's merge: at most once a diff --git a/modules/mesh-delivery/cmd/mesh-delivery/model.go b/modules/mesh-delivery/cmd/mesh-delivery/model.go index ef31ee77..17004128 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/model.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/model.go @@ -144,6 +144,21 @@ type DeliveryPlan struct { Summary string `json:"summary"` } +// receiving are the machines the plan sends a module to, in the plan's order; none when the plan names no +// machines, as a plan kept before they were said may not. +func (p *DeliveryPlan) receiving(module string) []string { + if p == nil { + return nil + } + var out []string + for _, m := range p.Machines { + if containsString(m.Receives, module) { + out = append(out, m.Machine) + } + } + return out +} + // MachinePlan is one machine's part of a plan. type MachinePlan struct { Machine string `json:"machine"`