Merge pull request 'mesh-delivery: cover a held merge whose module a backlog walk carried to every machine (hq issue 351)' (#163) from fix/a-release-carried-module-counts-as-sent into main

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)
+	}
+}

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

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"`