From 6f7f07635e0297256d48ad9e528c716d55eeaea3 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 23:15:38 +0200 Subject: [PATCH] Cover a failed or stopped delivery once a later finished walk contains it (hq issue 747) A group member whose walk failed in an outage stayed failed after a later walk of its repository, containing its merge, was done; its group then stopped every later walk of the other repository, and the catalogue could deliver nothing. A failed or stopped delivery now reads covered when a done walk of its repository, made after its merge, whose commit contains it (the forge's word, as issue 362's containment), built every module it moves. A group counts it as delivered, and a member the group stopped for it no longer holds a walk back: the group stops a walk only for a member whose change is still undelivered. A stop covered under issue 480 now likewise makes its group read reached, not stopped. While the forge is asked, the walk is held back rather than stopped, for at most ten minutes from the first ask: a forge that keeps erroring or cannot say then lets the walk be asked to leave the stopped merge out, as before. The hold is said by waits, stalled and stalled walks. The pass reads kept walks each tick, so today's stuck deliveries clear after a restart. --- .../cmd/mesh-delivery/answers.go | 10 + .../cmd/mesh-delivery/effects.go | 5 +- .../mesh-delivery/cmd/mesh-delivery/holder.go | 243 ++++++++++++++++- .../cmd/mesh-delivery/issue747_test.go | 258 ++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/model.go | 83 +++++- .../mesh-delivery/cmd/mesh-delivery/verbs.go | 4 + 6 files changed, 585 insertions(+), 18 deletions(-) create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/issue747_test.go diff --git a/modules/mesh-delivery/cmd/mesh-delivery/answers.go b/modules/mesh-delivery/cmd/mesh-delivery/answers.go index 3323a5c5..e3c65cb3 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/answers.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/answers.go @@ -298,6 +298,9 @@ func (h *Holder) orphanLines(now time.Time) []StalledLine { Says: fmt.Sprintf("the walk %s of %s at %s waits for this owner's word, and no delivery that has not ended "+ "follows it or is answered by it: `mesh-delivery.show` for the deliveries of %s, and `plans go %s --why …` "+ "starts it by hand", w.ID, w.Repository, shortOf(w.Commit, 8), w.Repository, w.ID)}) + if hold := h.coverHeld(id); hold != "" { + out[len(out)-1].Says += "; it is held back meanwhile: " + hold + } } return out } @@ -388,6 +391,9 @@ func (h *Holder) words() []func() { if stopped, _ := h.carriesStopped(walk); len(stopped) > 0 { h.waitsWhy[walk] = "it carries " + strings.Join(stopped, ", ") + ", whose delivery was stopped: it is " + "asked to leave that merge out first" + if hold := h.coverHeld(walk); hold != "" { + h.waitsWhy[walk] = hold + } h.Logf("[mesh-delivery] the walk %s is not let go: it carries %s, whose delivery was stopped", walk, strings.Join(stopped, ", ")) continue @@ -479,6 +485,10 @@ func (h *Holder) WalksWaiting() []StalledLine { switch { case len(held) > 0: out = append(out, held...) + case h.coverHeld(id) != "": + out = append(out, StalledLine{ID: IDOf(w.Repository, w.Commit), State: Published, Walk: id, For: waited, + Bound: coverAskBound.String(), H2: "none: it is asked to leave the merge out past the bound", + Says: "held back: " + h.coverHeld(id)}) case h.waitsWhy[id] != "": out = append(out, StalledLine{ID: IDOf(w.Repository, w.Commit), State: Published, Walk: id, For: waited, Bound: "0s", H2: "none: it goes at its turn", Says: "not let go yet: " + h.waitsWhy[id]}) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/effects.go b/modules/mesh-delivery/cmd/mesh-delivery/effects.go index e7db65f2..b7a2115f 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/effects.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/effects.go @@ -150,6 +150,9 @@ func statusOf(d *Delivery) (string, string) { case Delivered: return "success", "delivered" case Failed: + if d.coveredStop() { + return "success", clip(d.coveredStopSaid(), 140) // its change reached the machines (novox/hq issue 747) + } return "failure", clip("failed: "+why, 140) case Superseded: return "warning", clip("superseded: "+why, 140) @@ -178,7 +181,7 @@ 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) + fmt.Fprintf(&b, "**Covered**: %s\n\n", d.coveredStopSaid()) } 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 da430dc2..383e2d67 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -67,6 +67,13 @@ type Holder struct { // leaveRefused is, by walk, what the controller answered when it refused both to leave a stopped merge out and // to stop the walk whole. leaveRefused map[string]string + // coverAsked are the deliveries that failed or were stopped whose cover by a later finished walk waits for the + // forge's word on containment, worked out each pass (novox/hq issue 747): a walk carrying a merge stopped for one of + // them is held back meanwhile, never asked to leave it out before the answer. + coverAsked map[string]bool + // coverAskedSince is, by delivery, when the forge was first asked about its cover and has not answered: past + // coverAskBound the walk is no longer held back for it, and is asked to leave the merge out (review of issue 747). + coverAskedSince map[string]time.Time // tried counts the owed effects Flush tried since this owner started, by how each ended (novox/hq issue 407). tried owedTries // marks are the open pull requests read with a ready mark, by owner/repo#number (novox/hq issue 722, ready.go). @@ -1112,6 +1119,48 @@ func (h *Holder) plan() []func() { // A published delivery whose walk ended done before its word is read the same way (novox/hq issue 474): what // that walk did not build, a later delivered merge may have; and when the cover test says none did, past the // grace, it is stopped saying so — never left waiting for a start that cannot come. + // A delivery that failed or was stopped is read for a later finished walk of its repository that delivered its + // change after all: a walk that failed in an outage and was resumed, or the walk of a later merge that contains it + // (novox/hq issue 747). Read each tick, from what the walks and the kept deliveries say, so one stuck before this + // was built clears by itself after a restart. + h.coverAsked = map[string]bool{} + if h.coverAskedSince == nil { + h.coverAskedSince = map[string]time.Time{} + } + pending := map[string]bool{} + for _, d := range h.deliveries { + if d.State != Failed && d.State != Stopped || d.coveredStop() { + continue + } + by, ask := h.walkCover(d) + if len(ask) > 0 && by == "" { + pending[d.ID] = true + since, asked := h.coverAskedSince[d.ID] + if !asked { + since = now + h.coverAskedSince[d.ID] = now + } + // Held back only within the bound: a forge that keeps erroring never holds a walk for good. + h.coverAsked[d.ID] = now.Sub(since) < coverAskBound + } + if by != "" { + d.DeliveredBy = by + h.tellCovered(d, now) + continue + } + for _, in := range ask { + if a := h.askContains(d, in, now); a != nil { + asks = append(asks, a) + } + } + } + + for id := range h.coverAskedSince { + if !pending[id] { + delete(h.coverAskedSince, id) + } + } + for _, d := range h.deliveries { ended := d.endedUnstarted() if d.coveredStop() && !d.CoverTold { @@ -1273,7 +1322,7 @@ func (h *Holder) laterOnItsWay(d *Delivery) string { // 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())) + line := oneLine(fmt.Sprintf("%s delivery %s: %s, %s", now.UTC().Format(time.RFC3339), d.ID, d.State, 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}) @@ -1283,7 +1332,7 @@ func (h *Holder) tellCovered(d *Delivery, now time.Time) { 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.Logf("[mesh-delivery] %s: %s, %s", d.ID, d.State, d.coveredStopSaid()) h.keep(d) } @@ -1294,6 +1343,104 @@ func strandedUncovered(d *Delivery) bool { return d.State == Stopped && d.Stranded != "" && d.CoveredBy == "" } +// walkCover says which later finished walk delivered the change of a delivery that failed or was stopped, or +// nothing, and which walk commits the forge must still be asked about (novox/hq issue 747). A walk covers it when it +// is of its repository and trunk, done, made after its merge, its commit containing its merge — the merge itself, or +// a later one the forge says contains it — and it built every module the delivery's plan moves, each it did not +// build being one the delivery's own walk said unchanged (issue 611). This is the containment "one walk answers every +// merge it contains" uses (issue 362, ADR 0276), read after the fact: a failed walk resumed to done answers its own +// merge, and a later merge's walk answers every merge it contains. A walk that only said a module unchanged for its +// own merges says nothing of this one's, so it covers no module that way. Walks are read from the controller's records +// this owner holds and from every delivery's kept walk, so a walk long ended still counts after a restart. +func (h *Holder) walkCover(d *Delivery) (string, []string) { + if d.State != Failed && d.State != Stopped || d.MergedAs == "" || d.Plan == nil || len(d.Plan.Moved) == 0 { + return "", nil + } + var ask []string + for _, c := range h.doneWalksOf(d.Repository) { + w := c.walk + if !sameBranch(w.Branch, d.Base) || !w.Created.IsZero() && w.Created.Before(d.MergedAt.Add(-time.Minute)) { + continue + } + var missing []string + for _, m := range d.Plan.Moved { + unchanged := false + if d.Walk != nil { + _, unchanged = d.Walk.Unchanged[m] + } + if !containsString(w.Moved, m) && !unchanged { + missing = append(missing, m) + } + } + if len(missing) > 0 { + continue + } + if !sameCommit(c.commit, d.MergedAs) { + yes, known := d.ContainedIn[c.commit] + if !known && !containsString(ask, c.commit) { + ask = append(ask, c.commit) + } + if !yes { + continue + } + } + return fmt.Sprintf("%s (%s at %s into %s, done, contains its merge %s)", w.ID, d.Repository, shortOf(c.commit, 8), + orNone(d.Base), shortOf(d.MergedAs, 8)), nil + } + return "", ask +} + +// doneWalk is a finished walk and the commit of its repository it carried. +type doneWalk struct { + walk *WalkSeen + commit string +} + +// doneWalksOf are the finished walks of a repository this owner knows, oldest first: the controller's records it +// holds, and each delivery's kept walk with the commit of its repository that walk carried for it. +func (h *Holder) doneWalksOf(repository string) []doneWalk { + byID := map[string]doneWalk{} + for id, s := range h.walks { + w := s.walk + if w.Release != nil || w.IsBatch() || w.State != walkDone || !strings.EqualFold(w.Repository, repository) { + continue + } + byID[id] = doneWalk{walk: w.Seen(), commit: w.Commit} + } + for _, o := range h.deliveries { + if o.Walk == nil || o.Walk.State != walkDone || !strings.EqualFold(o.Repository, repository) { + continue + } + if _, known := byID[o.Walk.ID]; known { + continue + } + commit := o.AnsweredBy + if commit == "" && o.MergedAs != "" && sameCommit(o.Walk.Commit, o.MergedAs) { + commit = o.MergedAs + } + if commit == "" { + continue + } + byID[o.Walk.ID] = doneWalk{walk: o.Walk, commit: commit} + } + out := make([]doneWalk, 0, len(byID)) + for _, c := range byID { + out = append(out, c) + } + sort.Slice(out, func(i, j int) bool { + if !out[i].walk.Created.Equal(out[j].walk.Created) { + return out[i].walk.Created.Before(out[j].walk.Created) + } + return out[i].walk.ID < out[j].walk.ID + }) + return out +} + +// stopLapsed is whether a member its group stopped for another is stopped for nothing any more (see stopLapsed). +func (h *Holder) stopLapsed(d *Delivery) bool { + return stopLapsed(d, func(id string) *Delivery { return h.deliveries[id] }) +} + // unwalkedPast is whether a merged delivery has gone without a walk past the bound one opens within, read // again after it: what holds it for a person, unless something already ends it. func (h *Holder) unwalkedPast(d *Delivery, now time.Time) bool { @@ -1736,7 +1883,8 @@ func (h *Holder) turnsOf(g *Group, members []*Delivery, waits map[string]carryWa // A superseded member a later delivered walk carried counts as reached, as in GroupState: a group counts a // covered member as delivered, so the member after it gets its turn (ADR 0270 decision 2, novox/hq // issues 442 and 445). - if d.State.Reached() || d.State == Superseded && d.CoveredBy != "" { + // A stop or failure a later walk delivered since counts as reached too (novox/hq issues 480 and 747). + if memberReached(d) { continue } if sharing != "" && (d.Walk == nil || d.Walk.ID != sharing) { @@ -1761,7 +1909,7 @@ func (h *Holder) turnsOf(g *Group, members []*Delivery, waits map[string]carryWa } if w.why != "" { for _, later := range ordered[i+1:] { - if a := h.stopInGroup(byID[later.ID], fmt.Sprintf("its group %s stopped: %s superseded, and %s", g.ID, + if a := h.stopInGroup(byID[later.ID], d.ID, fmt.Sprintf("its group %s stopped: %s superseded, and %s", g.ID, d.ID, w.why)); a != nil { asks = append(asks, a) } @@ -1788,9 +1936,13 @@ func (h *Holder) turnsOf(g *Group, members []*Delivery, waits map[string]carryWa continue } return asks + case h.stopLapsed(d): + // Stopped for a member whose change reached the machines since: it stops nothing more. Its own change is + // delivered by a later walk of its repository that contains it, which then covers it (novox/hq issue 747). + continue case d.State == Failed || d.State == Stopped: for _, later := range ordered[i+1:] { - if a := h.stopInGroup(byID[later.ID], fmt.Sprintf("its group %s stopped: %s %s", g.ID, d.ID, d.State)); a != nil { + if a := h.stopInGroup(byID[later.ID], d.ID, fmt.Sprintf("its group %s stopped: %s %s", g.ID, d.ID, d.State)); a != nil { asks = append(asks, a) } } @@ -2028,7 +2180,7 @@ func mayCarry(c, d *Delivery, module string) bool { // controller ends the walk and walks the other merges again without it, and without each later merge of its // repository, which contains it. Every member a pass stops on one walk is gathered first, and the walk is asked // once, by the first member's ask, after the pass: a member is never walked again because its stop came second. -func (h *Holder) stopInGroup(d *Delivery, why string) func() { +func (h *Holder) stopInGroup(d *Delivery, cause, why string) func() { if d == nil || d.State.Final() { return nil } @@ -2037,6 +2189,7 @@ func (h *Holder) stopInGroup(d *Delivery, why string) func() { h.refuse(err) return nil } + d.StoppedFor = cause // the member it was stopped for: once that one is delivered, the stop lapses (novox/hq issue 747) h.moved(d, []Transition{t}) // Its open walk; or, its walk stopped for another merge, the batch or walk that walks its merge again (review // of #221): a merge walked again and then stopped by its group is left out there too. @@ -2118,6 +2271,12 @@ func (h *Holder) carriesStopped(walk string) ([]string, string) { if last := d.Transitions[len(d.Transitions)-1]; last.Event != EvStop || last.By == "" { continue } + // A stop whose change a later walk delivered since, or one its group made for a member delivered since, + // holds nothing back: the group stops a walk only for a member whose change is still undelivered (novox/hq + // issue 747). + if d.coveredStop() || h.stopLapsed(d) { + continue + } // By the merges it names alone: its own commit may be another's merge it no longer answers. if !slices.ContainsFunc(s.walk.Delivery.Merges, func(m WalkMerge) bool { return strings.EqualFold(m.Repository, d.Repository) && sameCommit(m.Commit, d.MergedAs) @@ -2135,6 +2294,51 @@ func (h *Holder) carriesStopped(walk string) ([]string, string) { return out, why } +// coverAwaited is whether a stopped merge a walk carries, given as `@`, waits for the forge's +// word on whether a later finished walk delivered it or the member its group stopped it for (novox/hq issue 747). +// It says since when the forge was first asked, the earliest of them. Held only within coverAskBound of the first +// ask: past it the forge's silence or error lets the walk be asked to leave the merge out. Under the lock. +func (h *Holder) coverAwaited(merges []string) (time.Time, bool) { + var since time.Time + awaited := false + for _, d := range h.deliveries { + if d.State != Stopped || d.MergedAs == "" || !containsString(merges, d.Repository+"@"+d.MergedAs) { + continue + } + for _, id := range []string{d.ID, d.stoppedFor()} { + if id == "" || !h.coverAsked[id] { + continue + } + if at := h.coverAskedSince[id]; !awaited || at.Before(since) { + since = at + } + awaited = true + } + } + return since, awaited +} + +// coverHeld says why a walk is held back while the forge is asked whether a later finished walk delivered a stopped +// merge it carries (novox/hq issue 747), or nothing when it is not. Said by `waits`, `stalled` and `stalled walks`, so +// a forge that cannot answer is loud. Under the lock. +func (h *Holder) coverHeld(walk string) string { + without, _ := h.carriesStopped(walk) + if len(without) == 0 { + return "" + } + since, awaited := h.coverAwaited(without) + if !awaited { + return "" + } + return fmt.Sprintf("it carries %s, whose delivery was stopped, and the forge is asked since %s whether a later "+ + "finished walk delivered it; it is asked to leave the merge out if no answer comes by %s (novox/hq issue 747)", + strings.Join(without, ", "), since.Local().Format("15:04:05"), since.Add(coverAskBound).Local().Format("15:04:05")) +} + +// coverAskBound is how long a walk carrying a stopped merge is held back while the forge is asked whether a later +// finished walk delivered it (novox/hq issue 747); past it, the walk is asked to leave the merge out. +const coverAskBound = 10 * time.Minute + // lastWhyOf is the reason of a delivery's last transition. func lastWhyOf(d *Delivery) string { if len(d.Transitions) == 0 { @@ -2170,6 +2374,9 @@ func (h *Holder) leaveStopped() []func() { if without, _ := h.carriesStopped(id); len(without) > 0 { said := fmt.Sprintf("its walk %s is not let go: it carries %s, whose delivery was stopped, and is asked to "+ "leave it out (novox/hq ADR 0299)", id, strings.Join(without, ", ")) + if hold := h.coverHeld(id); hold != "" { + said = "its walk " + id + " is held back: " + hold + } if r := h.leaveRefused[id]; r != "" { said += "; the controller refused: " + r } @@ -2193,6 +2400,9 @@ func (h *Holder) leaveStopped() []func() { delete(h.leaveRefused, id) continue } + if _, awaited := h.coverAwaited(without); awaited { + continue // held back, not stopped, until the forge says whether a later walk delivered it (issue 747) + } // Once per record of it: the record that answers an ask is newer. Asked again after five minutes, should // the ask have gone nowhere. if a, asked := h.leaveAsked[id]; asked && seen.walk.Revision <= a.revision && now.Sub(a.at) < 5*time.Minute { @@ -2314,18 +2524,28 @@ func againWords(a StopAnswer) string { func GroupState(g *Group, members []*Delivery) (string, string) { count := map[State]int{} reached, ended := 0, 0 - var uncovered []string + var uncovered, lapsed []string + byID := map[string]*Delivery{} + for _, d := range members { + byID[d.ID] = d + } for _, d := range members { - count[d.State]++ if d.State.Final() { ended++ } switch { - case d.State.Reached(), d.State == Superseded && d.CoveredBy != "": + case memberReached(d): + // Reached, whatever its final state: a failure or stop a later walk delivered since is no failure of the + // group (novox/hq issue 747). reached++ + continue + case stopLapsed(d, func(id string) *Delivery { return byID[id] }): + lapsed = append(lapsed, d.ID) + continue case d.State == Superseded: uncovered = append(uncovered, d.ID) } + count[d.State]++ } n := len(members) switch { @@ -2335,6 +2555,11 @@ func GroupState(g *Group, members []*Delivery) (string, string) { return "stopped", "a member was stopped" case n > 0 && reached == n: return "delivered", "every member is delivered" + case g.Closed && len(lapsed) > 0 && reached+len(lapsed) == n: + // Stopped for a member delivered since: its own change waits for a later walk of its repository, which then + // covers it (novox/hq issue 747). + return "delivering", fmt.Sprintf("%d of %d delivered; %s, stopped for a member delivered since, waits for a "+ + "later walk of its repository", reached, n, strings.Join(lapsed, ", ")) case g.Closed && n > 0 && ended == n: return "stopped", supersededWhy + strings.Join(uncovered, ", ") case g.Closed && g.Check != nil && (g.Check.OutOfDate != "" || g.Check.Verdict == "" || diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue747_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue747_test.go new file mode 100644 index 00000000..7646ec14 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue747_test.go @@ -0,0 +1,258 @@ +package main + +import ( + "strings" + "testing" + "time" +) + +// novox/hq issue 747: in the group fix/a-module-moves-only-when-its-program-changes, the controller's member #336 +// (merged as 5d22d7df) failed at 22:32 during an outage of the forge's tools. The walk of the controller's next merge, +// #339 at 60cc9ec3, which contains 5d22d7df, failed with it and was resumed to done by a rebuild. mesh-delivery still +// held #336 failed, so its group stopped every later walk of the catalogue that carried the catalogue member's merge +// 2edef5a0: the catalogue could deliver nothing. A failed or stopped delivery is covered once a later finished walk of +// its repository that contains its merge built its modules, and a group stops a walk only for a member whose change is +// still undelivered. +// +// The shape is the live state of 2026-10-14 23:07, as mesh-delivery.show gave it, read back after a restart: the +// controller holds none of the walks of the night any more, only the walk of the catalogue's next merge. + +const ( + g747 = "fix/a-module-moves-only-when-its-program-changes" + ctl747, cat747 = "novox/mesh-controller", "novox/mesh-catalog" + head336, merge336 = "b34055f580d4e2a4", "5d22d7df86c73452" + head339, merge339 = "e4acab32d6a20ade", "60cc9ec363dfa7c6" + head301, merge301 = "d94764f00168471f", "2edef5a00ae05605" + merge307 = "0f76b6e2aaaaaaaa" +) + +var ( + at747 = time.Date(2026, 10, 14, 20, 21, 6, 0, time.UTC) + id336, id339, id301 = IDOf(ctl747, head336), IDOf(ctl747, head339), IDOf(cat747, head301) + unchangedRouteProxy747 = map[string]string{"route-proxy": "its build source holds none of the changed files"} +) + +// night747 is the stuck state, with the forge saying, or not, that 60cc9ec3 contains 5d22d7df. +func night747(t *testing.T, contained bool) *world { return night747With(t, contained, nil) } + +// night747With is night747 with #339's walk having built only the modules given, saying the others unchanged. +func night747With(t *testing.T, contained bool, built []string) *world { + t.Helper() + return night747Over(t, func(f *fakeForge) { f.contains[merge336+" in "+merge339] = contained }, built) +} + +// night747Over is night747With with the forge set as given: one that is not told the pair cannot say. +func night747Over(t *testing.T, set func(*fakeForge), built []string) *world { + t.Helper() + at := at747 + failedAt := at.Add(11 * time.Minute) + store := newMemStore() + moved339, unchanged339 := []string{"build-agent", "mesh-controller"}, unchangedRouteProxy747 + if built != nil { + moved339, unchanged339 = built, map[string]string{} + for _, m := range []string{"build-agent", "mesh-controller", "route-proxy"} { + if !containsString(built, m) { + unchanged339[m] = "its build source holds none of the files #339 changed" + } + } + } + m336 := &Delivery{ID: id336, Repository: ctl747, Commit: head336, Number: 336, Base: "main", Branch: g747, Group: g747, + State: Failed, Since: failedAt, Created: at.Add(-time.Hour), MergedAs: merge336, MergedAt: at, + AnsweredBy: merge336, Check: &Verdict{Gate: "pass"}, + Plan: planMoving(ctl747, "build-agent", "mesh-controller", "route-proxy"), + Walk: &WalkSeen{ID: "plan-336", Commit: merge336, State: walkFailed, Branch: "main", MovedSaid: true, + Note: "mesh-controller failed to build in tier 0", Created: at.Add(7 * time.Minute), Tiers: 2, + Unchanged: unchangedRouteProxy747}, + Transitions: []Transition{{At: failedAt, From: Delivering, To: Failed, Event: EvFailed, + Why: "its walk failed, carrying its own commit"}}} + m339 := &Delivery{ID: id339, Repository: ctl747, Commit: head339, Number: 339, Base: "main", Branch: "fix/726", + State: Failed, Since: failedAt, Created: at.Add(-10 * time.Minute), MergedAs: merge339, MergedAt: at.Add(7 * time.Minute), + AnsweredBy: merge339, Check: &Verdict{Gate: "pass"}, Plan: planMoving(ctl747, "mesh-controller"), + // Its walk failed with #336's, and a rebuild resumed it to done: the record it keeps says so. + Walk: &WalkSeen{ID: "plan-339", Commit: merge339, State: walkDone, Branch: "main", MovedSaid: true, + Moved: moved339, Unchanged: unchanged339, + Created: at.Add(11 * time.Minute), Tiers: 2, Tier: 2}, + Transitions: []Transition{{At: failedAt, From: Delivering, To: Failed, Event: EvFailed, + Why: "its walk failed, carrying its own commit"}}} + m301 := &Delivery{ID: id301, Repository: cat747, Commit: head301, Number: 301, Base: "main", Branch: g747, Group: g747, + State: Stopped, Since: failedAt, Created: at.Add(-2 * time.Hour), MergedAs: merge301, MergedAt: at.Add(7 * time.Second), + Check: &Verdict{Gate: "pass"}, Plan: planMoving(cat747, "build-agent", "route-proxy"), + Transitions: []Transition{{At: failedAt, From: Ready, To: Stopped, Event: EvStop, By: byOwner, + Why: "its group " + g747 + " stopped: " + id336 + " failed"}}} + for _, d := range []*Delivery{m336, m339, m301} { + if err := store.PutDelivery(d); err != nil { + t.Fatal(err) + } + } + g := &Group{ID: g747, Members: []string{id301, id336}, Closed: true, Order: []string{id336, id301}, Said: "failed", + OrderOf: commitsOf([]*Delivery{m301, m336}), Check: &GroupCheck{Asked: "check-1", At: at.Add(-time.Minute), + Verdict: "pass", Summary: "both compose", Commits: commitsOf([]*Delivery{m301, m336})}} + if err := store.PutGroup(g); err != nil { + t.Fatal(err) + } + ctlr := newFakeController() + // The catalogue's next merge: its walk carries 2edef5a0, merged before it and never walked, and waits for its word. + ctlr.put(Walk{ID: "plan-cat", Repository: cat747, Branch: "main", Commit: merge307, Created: at.Add(46 * time.Minute), + State: walkBuilding, Tiers: [][]string{{"gitea"}}, Modules: map[string]*WalkModule{"gitea": {}}, + Delivery: &WalkDelivery{Awaits: "mesh-delivery", Merges: []WalkMerge{ + {Repository: cat747, Commit: merge301, Carried: merge307}, {Repository: cat747, Commit: merge307}}}}) + ctlr.mergedAt[merge301], ctlr.mergedAt[merge307] = m301.MergedAt, at.Add(46*time.Minute) + forge := newFakeForge() + set(forge) + w := newWorldOver(t, store, ctlr, forge, at.Add(47*time.Minute)) + if err := w.h.Reconcile(); err != nil { + t.Fatal(err) + } + w.settleAll() + return w +} + +func groupState747(w *world) (string, string) { + g := w.h.groups[g747] + return GroupState(g, w.h.membersOf(g)) +} + +// **A failed member is covered by a later finished walk that contains it**: #339's walk, done at 60cc9ec3, which the +// forge says contains 5d22d7df, built build-agent and mesh-controller, and #336's own walk said route-proxy unchanged. +// #336 stays failed, reads covered by that walk on its pull request, and its group no longer reads failed. #339, whose +// own walk was resumed to done, is covered by it too. +func TestAFailedMemberIsCoveredByALaterDoneWalkThatContainsIt(t *testing.T) { + w := night747(t, true) + d := w.delivery(id336) + if state, desc := statusOf(d); d.State != Failed || state != "success" || !strings.Contains(desc, "covered after its failure") || + !strings.Contains(desc, "plan-339") { + t.Fatalf("#336's status reads %s %q", state, desc) + } + if s := statusOn(w, ctl747, head336); !strings.HasPrefix(s, "success") { + t.Fatalf("the forge was told %q of #336", s) + } + if state, desc := statusOf(w.delivery(id339)); state != "success" || !strings.Contains(desc, "plan-339") { + t.Fatalf("#339 reads %s %q; want it covered by its own walk, resumed to done", state, desc) + } + if state, why := groupState747(w); state == "failed" || state == "stopped" { + t.Fatalf("the group reads %s: %s", state, why) + } + // Told once, and kept: a restart tells nothing again. + notes := len(w.forge.notesOn(merge336)) + again := newWorldOver(t, w.store, w.ctl, w.forge, w.now.Add(time.Minute)) + again.settleAll() + if state, _ := statusOf(again.delivery(id336)); len(w.forge.notesOn(merge336)) != notes || state != "success" { + t.Fatalf("a restart told it again or forgot it: %d notes, was %d", len(w.forge.notesOn(merge336)), notes) + } +} + +// **The group then no longer stops the other repository's walk**: the catalogue member was stopped for #336 alone, +// so the walk of the catalogue's next merge, which carries its merge, is neither stopped nor held back, and the +// group waits for that walk to deliver the catalogue member's change. +func TestACoveredMemberNoLongerStopsTheOtherRepositorysWalk(t *testing.T) { + w := night747(t, true) + if len(w.ctl.withouts) > 0 || containsString(w.ctl.stops, "plan-cat") { + t.Fatalf("the catalogue's walk was stopped: without %v, stops %v", w.ctl.withouts, w.ctl.stops) + } + if stopped, _ := w.h.carriesStopped("plan-cat"); len(stopped) > 0 { + t.Fatalf("the catalogue's walk is held back for %v", stopped) + } + if state, why := groupState747(w); state != "delivering" || !strings.Contains(why, id301) { + t.Fatalf("the group reads %s: %s; want delivering, the catalogue member waiting for a later walk", state, why) + } + // Once a later walk of the catalogue that contains its merge built its modules, it is covered, and the group is + // delivered. + w.forge.contains[merge301+" in "+merge307] = true + r := w.ctl.walk("plan-cat") + sent := w.now + r.Modules = map[string]*WalkModule{} + for _, m := range []string{"build-agent", "route-proxy", "gitea"} { + r.Modules[m] = &WalkModule{State: "built", SentAt: &sent} + } + r.State = walkDone + w.h.WalkMoved(w.ctl.put(r)) + w.later(time.Minute) + w.settleAll() + if state, desc := statusOf(w.delivery(id301)); state != "success" || !strings.Contains(desc, "plan-cat") { + t.Fatalf("the catalogue member reads %s %q", state, desc) + } + if state, why := groupState747(w); state != "delivered" { + t.Fatalf("the group reads %s: %s", state, why) + } +} + +// **A failed member not contained still stops it**: the forge says 60cc9ec3 does not contain 5d22d7df, so #336's change +// never reached the machines; it stays failed and uncovered, its group failed, and the catalogue's walk that carries +// the stopped member's merge is asked to leave it out. +func TestAFailedMemberNotContainedStillStopsTheOtherRepositorysWalk(t *testing.T) { + w := night747(t, false) + d := w.delivery(id336) + if state, desc := statusOf(d); d.State != Failed || state != "failure" { + t.Fatalf("#336 is %s and reads %s %q", d.State, state, desc) + } + if state, _ := groupState747(w); state != "failed" { + t.Fatalf("the group reads %s", state) + } + if len(w.ctl.withouts) == 0 || !strings.HasPrefix(w.ctl.withouts[0], "plan-cat without "+cat747+"@"+merge301) { + t.Fatalf("the catalogue's walk was not asked to leave the stopped merge out: %v", w.ctl.withouts) + } +} + +// **A later walk that only said a module unchanged covers nothing of an earlier merge that moves it**: #339's walk +// built mesh-controller alone and said build-agent unchanged for #339's own files, which says nothing of #336's; only +// #336's own walk says that of its change (route-proxy). #336 stays failed and its group still stops the catalogue. +func TestALaterWalksUnchangedCoversNoModuleOfAnEarlierMerge(t *testing.T) { + w := night747With(t, true, []string{"mesh-controller"}) + if state, desc := statusOf(w.delivery(id336)); state != "failure" { + t.Fatalf("#336 reads %s %q, though no walk built build-agent for it", state, desc) + } + if state, _ := groupState747(w); state != "failed" { + t.Fatalf("the group reads %s", state) + } + if len(w.ctl.withouts) == 0 { + t.Fatal("the catalogue's walk was not asked to leave the stopped merge out") + } +} + +// **A forge that cannot say holds the walk only within the bound** (review of issue 747): while the forge errors on +// whether 60cc9ec3 contains 5d22d7df, the catalogue's walk is held back, loudly, neither stopped nor asked to leave the +// merge out, and #336 still reads failed. Past the bound the forge's silence holds nothing back any more: the walk +// is asked to leave the stopped merge out, as with no later walk at all. +func TestAForgeThatCannotSayHoldsTheWalkOnlyWithinTheBound(t *testing.T) { + for _, c := range []struct { + name string + set func(*fakeForge) + }{ + {"erroring", func(f *fakeForge) { f.containsDown = true }}, + {"not answering for the pair", func(*fakeForge) {}}, + } { + t.Run(c.name, func(t *testing.T) { + w := night747Over(t, c.set, nil) + if len(w.ctl.withouts) > 0 || len(w.ctl.stops) > 0 { + t.Fatalf("within the bound the walk was not held: without %v, stops %v", w.ctl.withouts, w.ctl.stops) + } + if state, desc := statusOf(w.delivery(id336)); state != "failure" { + t.Fatalf("#336 reads %s %q while the forge cannot say", state, desc) + } + said := "" + for _, l := range w.h.WalksWaiting() { + if l.Walk == "plan-cat" { + said += l.Says + } + } + w.later(3 * time.Minute) // past the grace an unheld walk is given, so `stalled` lists it + w.settleAll() + for _, l := range w.h.Stalled() { + if l.Walk == "plan-cat" { + said += " | " + l.Says + } + } + if strings.Count(said, "the forge is asked since") != 2 { + t.Fatalf("the hold is not said by `stalled walks` and `stalled`: %q", said) + } + if len(w.ctl.withouts) > 0 || len(w.ctl.stops) > 0 { + t.Fatalf("within the bound the walk was not held: without %v, stops %v", w.ctl.withouts, w.ctl.stops) + } + w.later(coverAskBound + time.Minute) + w.settleAll() + if len(w.ctl.withouts) == 0 || !strings.HasPrefix(w.ctl.withouts[0], "plan-cat without "+cat747+"@"+merge301) { + t.Fatalf("past the bound the walk was not asked to leave the stopped merge out: %v", w.ctl.withouts) + } + }) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/model.go b/modules/mesh-delivery/cmd/mesh-delivery/model.go index ec3e2e1d..2d2a0c67 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/model.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/model.go @@ -83,6 +83,14 @@ type Delivery struct { // 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"` + // DeliveredBy says, for a delivery that failed or was stopped, the later finished walk of its repository whose + // commit contains its merge and which built every module it moves — each it did not build, its own walk said + // unchanged: its change reached the machines after all (novox/hq issue 747). Its state stays final, as a covered + // stop's does (issue 480), and it reads covered: its group counts it as delivered. + DeliveredBy string `json:"delivered_by,omitempty"` + // StoppedFor is, for a member its group stopped, the member whose end stopped it (novox/hq issue 747): once that + // member's change reached the machines, the stop has lapsed, and holds no walk of its repository back. + StoppedFor string `json:"stopped_for,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. @@ -303,18 +311,77 @@ 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. +// coveredStop is whether a delivery that ended stopped or failed was covered since, and reads covered. A stop +// because its walk ended done before its word without building its modules (novox/hq issue 474) is covered when 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; issue 480). Any stop or failure is covered when a later finished walk +// of its repository whose commit contains its merge built its modules (DeliveredBy, novox/hq issue 747). Its state +// stays final, as a covered superseded member stays superseded (issue 442), and it reads covered, by that merge or +// walk. A person's stop with a cover found only while it was held keeps its error. func (d *Delivery) coveredStop() bool { - return d.State == Stopped && d.Stranded != "" && d.CoveredBy != "" + switch d.State { + case Stopped: + return d.Stranded != "" && d.CoveredBy != "" || d.DeliveredBy != "" + case Failed: + return d.DeliveredBy != "" + } + return false } -// coveredStopSaid is how a covered stop reads: on the forge's status, in its view and note, and in `show`. +// coveredStopSaid is how a covered stop or failure 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 + ended := "its stop" + if d.State == Failed { + ended = "its failure" + } + if d.Stranded != "" && d.CoveredBy != "" { + return "covered after " + ended + ": a later merge that contains it was delivered: " + d.CoveredBy + } + return "covered after " + ended + ": a later walk that contains it was delivered: " + d.DeliveredBy +} + +// memberReached is whether a group counts a member as delivered: its change reached the machines, or had nothing to +// reach them with — delivered, history or covered, a superseded member a later delivered walk carried (issue 442), +// or a stop or failure covered since (issues 480 and 747). +func memberReached(d *Delivery) bool { + return d.State.Reached() || d.State == Superseded && d.CoveredBy != "" || d.coveredStop() +} + +// stoppedFor is the member whose end stopped this one in its group: kept since novox/hq issue 747, and read from the +// stop's reason for a stop made before ("its group stopped: …"). +func (d *Delivery) stoppedFor() string { + if d.StoppedFor != "" || d.State != Stopped || d.Group == "" { + return d.StoppedFor + } + prefix := "its group " + d.Group + " stopped: " + for i := len(d.Transitions) - 1; i >= 0; i-- { + t := d.Transitions[i] + if t.Event != EvStop { + continue + } + if t.By != byOwner || !strings.HasPrefix(t.Why, prefix) { + return "" + } + member, _, _ := strings.Cut(strings.TrimPrefix(t.Why, prefix), " ") + return strings.TrimRight(member, ",;:") + } + return "" +} + +// stopLapsed is whether a member its group stopped for another member is stopped for nothing any more: that member's +// change reached the machines since — delivered, or its failure covered by a later walk (novox/hq issue 747). Its own +// change is still to deliver, by a later walk of its repository that contains it; its stop holds none back. find +// gives a delivery by its id. +func stopLapsed(d *Delivery, find func(string) *Delivery) bool { + if d.State != Stopped || d.coveredStop() { + return false + } + by := d.stoppedFor() + if by == "" || by == d.ID { + return false + } + o := find(by) + return o != nil && memberReached(o) } func (d *Delivery) shownOnItsPullRequest() bool { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go index b95aacd2..b2dfed42 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go @@ -72,6 +72,10 @@ func (h *Holder) waitsFor(d *Delivery) string { return "its walk: " + clip(d.Walk.Note, 120) } case Stopped: + if h.stopLapsed(d) { + return "a later walk of its repository that contains its merge: its group stopped it for " + d.stoppedFor() + + ", whose change was delivered since (novox/hq issue 747)" + } 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)" -- 2.54.0