Merge pull request 'mesh-delivery: a stop a later merge covered reads covered, and stays in deliveries while stalled lists it (hq issue 480)' (#242) from fix/480-a-covered-stop-reads-covered into main

modules/mesh-delivery/cmd/mesh-delivery/effects.go

@@ -104,6 +104,9 @@ func statusOf(d *Delivery) (string, string) {
 	case Superseded:
 		return "warning", clip("superseded: "+why, 140)
 	case Stopped:
+		if d.coveredStop() {
+			return "success", clip(d.coveredStopSaid(), 140) // its change reached the machines (novox/hq issue 480)
+		}
 		return "error", clip("stopped: "+why, 140)
 	case Retired:
 		return "success", "history: merged before this owner delivered anything, never walked"
@@ -124,6 +127,9 @@ const viewMarker = "<!-- mesh-delivery:view -->"
 func ViewBody(d *Delivery, g *Group) string {
 	var b strings.Builder
 	fmt.Fprintf(&b, "%s\n**Delivery** `%s` — **%s** since %s\n\n", viewMarker, d.ID, d.State, d.Since.UTC().Format(time.RFC3339))
+	if d.coveredStop() {
+		fmt.Fprintf(&b, "**Covered** after its stop: a later merge that contains it was delivered: %s\n\n", d.CoveredBy)
+	}
 	if d.HeldWhy != "" && d.State == Held {
 		fmt.Fprintf(&b, "Held for a person: %s — `mesh-delivery.release` with why lets it go on.\n\n", d.HeldWhy)
 	}

modules/mesh-delivery/cmd/mesh-delivery/holder.go

@@ -953,12 +953,16 @@ func (h *Holder) plan() []func() {
 	// grace, it is stopped saying so — never left waiting for a start that cannot come.
 	for _, d := range h.deliveries {
 		ended := d.endedUnstarted()
+		if d.coveredStop() && !d.CoverTold {
+			h.tellCovered(d, now) // covered before this holder told it, or before a restart (novox/hq issue 480)
+			continue
+		}
 		if strandedUncovered(d) {
 			// Stopped and final, but still read for a later merge that delivered its modules: until one did,
 			// `stalled` keeps listing it (review of novox/hq issue 474).
 			var ask []string
 			if d.CoveredBy, ask = h.coverOf(d); d.CoveredBy != "" {
-				h.keep(d)
+				h.tellCovered(d, now)
 			}
 			for _, in := range ask {
 				if a := h.askContains(d, in, now); a != nil {
@@ -1103,6 +1107,25 @@ func (h *Holder) laterOnItsWay(d *Delivery) string {
 	return ""
 }
 
+// tellCovered tells the forge that a stop is covered since, once: its status says covered and by which merge, its view
+// says so, and its note keeps the line on its head and its merge commit (novox/hq issue 480). No transition: the stop
+// stays final, and the bus's transition event is not said again.
+func (h *Holder) tellCovered(d *Delivery, now time.Time) {
+	d.CoverTold = true
+	line := oneLine(fmt.Sprintf("%s delivery %s: stopped, %s", now.UTC().Format(time.RFC3339), d.ID, d.coveredStopSaid()))
+	d.Owed = append(d.Owed, Effect{Kind: EffectNote, Commit: d.Commit, Line: line, Since: now})
+	if d.MergedAs != "" && !sameCommit(d.MergedAs, d.Commit) {
+		d.Owed = append(d.Owed, Effect{Kind: EffectNote, Commit: d.MergedAs, Line: line, Since: now})
+	}
+	if d.shownOnItsPullRequest() {
+		d.owe(Effect{Kind: EffectView, Since: now})
+		state, desc := statusOf(d)
+		d.owe(Effect{Kind: EffectStatus, Commit: d.Commit, Context: "mesh/delivery", State: state, Line: desc, Since: now})
+	}
+	h.Logf("[mesh-delivery] %s: stopped, %s", d.ID, d.coveredStopSaid())
+	h.keep(d)
+}
+
 // strandedUncovered is whether a delivery stopped because its ended walk delivered nothing is still uncovered: a
 // merged change that never reached the machines, which `stalled` keeps listing until a later delivered merge
 // carries its modules (review of novox/hq issue 474).

modules/mesh-delivery/cmd/mesh-delivery/issue480_test.go

@@ -0,0 +1,134 @@
+package main
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+// novox/hq issue 480: a delivery stopped because its walk ended done before its word without building its modules
+// (issue 474) is still read for a later merge that delivers them, and kept CoveredBy once one did. Its state stays
+// stopped — a final state never moves, as for a covered superseded member (issue 442) — but the forge's status, its
+// view, its note and `show` went on saying "stopped" for a change that reached every machine. Once covered they say
+// covered, and by which merge. And while `stalled` lists such a stop, `deliveries` lists it too, past a day.
+
+// strandedNight is theNight with #215 stopped, its walk past the grace with nothing later carrying build-agent.
+func strandedNight(t *testing.T) (*world, string) {
+	w := theNight(t)
+	id := IDOf(ctl474, head215)
+	w.later(strandedAfter + 5*time.Minute)
+	w.settleAll()
+	if d := w.delivery(id); d.State != Stopped || d.Stranded == "" {
+		t.Fatalf("#215 is %s, stranded %q", d.State, d.Stranded)
+	}
+	return w, id
+}
+
+func TestAStopALaterMergeCoversReadsCoveredByThatMerge(t *testing.T) {
+	w, id := strandedNight(t)
+	if got := statusOn(w, ctl474, head215); !strings.HasPrefix(got, "error stopped: ") {
+		t.Fatalf("stopped, its status says %q", got)
+	}
+	w.forge.contains[merge215+" in "+merge219] = true
+	by := deliverLater(w, ctl474, 219, "main", head219, merge219, w.now, walkDone, "build-agent")
+	w.later(2 * time.Minute)
+	w.settleAll()
+	d := w.delivery(id)
+	if d.State != Stopped || !strings.Contains(d.CoveredBy, by) {
+		t.Fatalf("after #219 it is %s, covered by %q", d.State, d.CoveredBy)
+	}
+	got := statusOn(w, ctl474, head215)
+	if !strings.HasPrefix(got, "success covered") || !strings.Contains(got, by) {
+		t.Fatalf("covered by %s, its status still says %q", by, got)
+	}
+	shown, err := w.h.Show(id)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if reads, _ := shown.(map[string]any)["reads"].(string); !strings.HasPrefix(reads, "covered") || !strings.Contains(reads, by) {
+		t.Fatalf("show reads %q", reads)
+	}
+	view := w.forge.views[ctl474+"#215"]
+	if !strings.Contains(view, "Covered") || !strings.Contains(view, by) {
+		t.Fatalf("its view does not say it was covered by %s:\n%s", by, view)
+	}
+	noted := false
+	for _, l := range w.forge.notesOn(merge215) {
+		noted = noted || strings.Contains(l, "covered") && strings.Contains(l, by)
+	}
+	if !noted {
+		t.Fatalf("its merge's note does not say it was covered: %v", w.forge.notesOn(merge215))
+	}
+	// Told once: further ticks owe nothing more.
+	w.later(time.Hour)
+	w.settleAll()
+	if d := w.delivery(id); len(d.Owed) != 0 || !d.CoverTold {
+		t.Fatalf("still owes %+v, told %v", d.Owed, d.CoverTold)
+	}
+}
+
+// A stop covered before this was built (CoveredBy kept, nothing told) is told after a restart, from what was kept.
+func TestACoveredStopKeptBeforeIsToldAfterARestart(t *testing.T) {
+	w, id := strandedNight(t)
+	w.h.mu.Lock()
+	d := w.h.deliveries[id]
+	d.CoveredBy = IDOf(ctl474, head219) + " (merged as " + merge219[:8] + ")"
+	if err := w.store.PutDelivery(d); err != nil {
+		t.Fatal(err)
+	}
+	w.h.mu.Unlock()
+	again := newWorldOver(t, w.store, w.ctl, w.forge, w.now.Add(time.Hour))
+	again.settleAll()
+	if got := statusOn(again, ctl474, head215); !strings.HasPrefix(got, "success covered") ||
+		!strings.Contains(got, IDOf(ctl474, head219)) {
+		t.Fatalf("after a restart its status says %q", got)
+	}
+	if d := again.delivery(id); d.State != Stopped || len(d.Transitions) != len(w.delivery(id).Transitions) {
+		t.Fatalf("a restart moved it: %s", d.State)
+	}
+}
+
+// Only a stop for a walk that never built its modules reads covered: a person's stop, or one the cover test has
+// not answered, keeps its error.
+func TestOnlyACoveredStrandedStopReadsCovered(t *testing.T) {
+	for _, d := range []*Delivery{
+		{State: Stopped, CoveredBy: "x"},            // a person's stop, with a cover found while it was held
+		{State: Stopped, Stranded: "never reached"}, // stranded, not covered
+	} {
+		if state, _ := statusOf(d); state != "error" {
+			t.Fatalf("%+v reads %s", d, state)
+		}
+		if d.coveredStop() {
+			t.Fatalf("%+v reads as a covered stop", d)
+		}
+	}
+}
+
+func TestAStopStalledListsStaysInDeliveries(t *testing.T) {
+	w, id := strandedNight(t)
+	w.later(30 * time.Hour)
+	w.settleAll()
+	listed := func() *Line {
+		for _, l := range w.h.Deliveries("", "", "", false) {
+			if l.ID == id {
+				return &l
+			}
+		}
+		return nil
+	}
+	if len(w.h.Stalled()) != 1 {
+		t.Fatalf("stalled lists %+v", w.h.Stalled())
+	}
+	l := listed()
+	if l == nil || !l.Stalled || !strings.Contains(l.Waits, "build-agent") {
+		t.Fatalf("listed by stalled, deliveries shows %+v", l)
+	}
+	// Covered, it ends like any final delivery: a day after its stop.
+	w.forge.contains[merge215+" in "+merge219] = true
+	deliverLater(w, ctl474, 219, "main", head219, merge219, w.now, walkDone, "build-agent")
+	w.later(2 * time.Minute)
+	w.settleAll()
+	if len(w.h.Stalled()) != 0 || listed() != nil {
+		t.Fatalf("covered, stalled lists %+v and deliveries %+v", w.h.Stalled(), listed())
+	}
+}

modules/mesh-delivery/cmd/mesh-delivery/main.go

@@ -192,7 +192,7 @@ func tools(h *Holder, l *listening) []stdio.Tool {
 	seat := "mesh-delivery."
 	return []stdio.Tool{
 		{Name: seat + "deliveries",
-			Description: "Every delivery not final, and those that ended in the last day, one line each: its id " +
+			Description: "Every delivery not final, those that ended in the last day, and any stop `stalled` still lists, one line each: its id " +
 				"(owner/repository@commit), its state, its group, what it waits for and since when. Narrowed by state, " +
 				"repository or group.",
 			Input: map[string]any{"type": "object", "properties": map[string]any{"state": str("one state"),

modules/mesh-delivery/cmd/mesh-delivery/model.go

@@ -68,6 +68,9 @@ type Delivery struct {
 	// Stranded says why a merge whose walk ended done before its word, without moving every module it moves, is
 	// stopped: nothing later delivered them either (novox/hq issue 474). Set once the cover test says no.
 	Stranded string `json:"stranded,omitempty"`
+	// CoverTold is whether the forge was told that such a stop was covered since: its status, its view and its note
+	// (novox/hq issue 480). Kept, so a restart tells it once, and a stop covered before this was built is told too.
+	CoverTold bool `json:"cover_told,omitempty"`
 	// TurnAwaits says, for a group member whose turn waits for a later merge to carry a superseded member before it,
 	// which merges and until when; TurnAwaitsUntil is that moment, which bounds the wait (ADR 0276 decision 9,
 	// novox/hq issue 445). `show` shows both, and `stalled` says them past that moment.
@@ -248,6 +251,20 @@ func (d *Delivery) FromForgesWord() bool {
 
 // shownOnItsPullRequest is whether its view and status are kept on its pull request: not for history, whose
 // pull request the forge may no longer hold — or holds under its number another one.
+// coveredStop is whether a delivery stopped because its walk ended done before its word without building its modules
+// (novox/hq issue 474) was covered since: a later merge into its trunk that contains it was delivered, carrying every
+// module it moves to every machine running it (ADR 0270's cover test, kept as CoveredBy). Its state stays stopped —
+// final, as a covered superseded member stays superseded (issue 442) — and it reads covered, by that merge (novox/hq
+// issue 480). A stop for any other reason never reads covered.
+func (d *Delivery) coveredStop() bool {
+	return d.State == Stopped && d.Stranded != "" && d.CoveredBy != ""
+}
+
+// coveredStopSaid is how a covered stop reads: on the forge's status, in its view and note, and in `show`.
+func (d *Delivery) coveredStopSaid() string {
+	return "covered after its stop: a later merge that contains it was delivered: " + d.CoveredBy
+}
+
 func (d *Delivery) shownOnItsPullRequest() bool {
 	return d.Number > 0 && d.History == "" && d.State != Retired
 }

modules/mesh-delivery/cmd/mesh-delivery/verbs.go

@@ -71,6 +71,11 @@ func (h *Holder) waitsFor(d *Delivery) string {
 		if d.Walk != nil {
 			return "its walk: " + clip(d.Walk.Note, 120)
 		}
+	case Stopped:
+		if strandedUncovered(d) {
+			return "a later delivered merge to carry " + strings.Join(d.notMovedBy(), ", ") + ": it is stopped, and " +
+				"listed by stalled until one does (novox/hq issue 480)"
+		}
 	}
 	return ""
 }
@@ -86,7 +91,8 @@ func (h *Holder) Deliveries(state, repository, group string, all bool) []Line {
 			group != "" && d.Group != group {
 			continue
 		}
-		if d.State.Final() && !all && now.Sub(d.Since) > 24*time.Hour {
+		// A final delivery `stalled` still lists stays, however old: the two never disagree (novox/hq issue 480).
+		if d.State.Final() && !all && now.Sub(d.Since) > 24*time.Hour && !strandedUncovered(d) {
 			continue
 		}
 		l := Line{ID: d.ID, State: d.State, Since: d.Since.UTC().Format(time.RFC3339), For: now.Sub(d.Since).Round(time.Second).String(),
@@ -97,7 +103,7 @@ func (h *Holder) Deliveries(state, repository, group string, all bool) []Line {
 		if d.Walk != nil {
 			l.Walk = d.Walk.ID
 		}
-		l.Stalled = pastBound(d, now)
+		l.Stalled = pastBound(d, now) || strandedUncovered(d)
 		out = append(out, l)
 	}
 	sort.Slice(out, func(i, j int) bool {
@@ -115,6 +121,9 @@ func (h *Holder) Show(id string) (any, error) {
 	defer h.mu.Unlock()
 	if d := h.deliveries[id]; d != nil {
 		out := map[string]any{"delivery": d, "waits": h.waitsFor(d)}
+		if d.coveredStop() {
+			out["reads"] = d.coveredStopSaid() // stopped, and its change reached the machines (novox/hq issue 480)
+		}
 		if g := h.groups[d.Group]; g != nil {
 			state, why := GroupState(g, h.membersOf(g))
 			out["group"] = map[string]any{"group": g, "state": state, "why": why}