diff --git a/modules/mesh-delivery/cmd/mesh-delivery/effects.go b/modules/mesh-delivery/cmd/mesh-delivery/effects.go index 02d6694b..35d9d3fa 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/effects.go +++ b/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 = "" 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) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 554ee6a1..edcf42e6 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/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). diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue480_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue480_test.go new file mode 100644 index 00000000..b999c478 --- /dev/null +++ b/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()) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/model.go b/modules/mesh-delivery/cmd/mesh-delivery/model.go index 74315c8b..8f8a5047 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/model.go +++ b/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 } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go index 4148b6e9..fa80497b 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go +++ b/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}