From 3d7f9a3e669f42d9883d7c72c93d60b9a18e13bf Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 20:27:55 +0200 Subject: [PATCH] mesh-delivery: say a shared-module leave-out truly, hold back only what was stopped by an act, and say a held walk where it is read (review of #221) A merge left out for sharing a module read as containing the stopped merge; the safety net also held back merges stranded by their walk, which nobody stopped; a held walk said why only in the log; and a stop of a merge in a batch did not ask the batch. --- .../cmd/mesh-delivery/fakes_test.go | 42 ++++++++- .../mesh-delivery/cmd/mesh-delivery/holder.go | 46 +++++++++- .../cmd/mesh-delivery/issue459_test.go | 92 +++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/model.go | 30 ++++-- .../mesh-delivery/cmd/mesh-delivery/table.go | 6 ++ .../mesh-delivery/cmd/mesh-delivery/verbs.go | 42 +++++++++ 6 files changed, 244 insertions(+), 14 deletions(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go index cc42cb60..4c1b703d 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go @@ -100,8 +100,12 @@ type fakeController struct { // the merges' times, by commit, for what a later merge contains. refuseWithout bool mergedAt map[string]time.Time - plans int - down bool + // refuseStop refuses every whole stop too; moves are the modules each merge moves, by commit, for what a left-out + // merge shares (ADR 0299). + refuseStop bool + moves map[string][]string + plans int + down bool // onCheck runs as a check is asked, before it is answered: what the forge held at that moment. onCheck func() // planned is what Plan answers instead of "builds app", when set. @@ -109,7 +113,8 @@ type fakeController struct { } func newFakeController() *fakeController { - return &fakeController{walks: map[string]Walk{}, held: true, mergedAt: map[string]time.Time{}} + return &fakeController{walks: map[string]Walk{}, held: true, mergedAt: map[string]time.Time{}, + moves: map[string][]string{}} } func (c *fakeController) Plan(repository, base, head string, paths, dirs, removed []string) (*DeliveryPlan, error) { @@ -183,6 +188,9 @@ func (c *fakeController) Stop(walk, why, by string) error { return errors.New("down") } c.stops = append(c.stops, walk) + if c.refuseStop { + return errors.New("the controller refuses") + } if w, known := c.walks[walk]; known && !w.Open() { // As the controller: only a walk that is building or rolling is stopped (`delivery-stop`). return fmt.Errorf("%s is already %s", walk, w.State) @@ -230,13 +238,33 @@ func (c *fakeController) StopWithout(walk, why, by string, without []string) (St } return "" } + shares := func(m WalkMerge) (string, []string) { + for _, s := range wd.Merges { + if named(s) { + var both []string + for _, x := range c.moves[m.Commit] { + if containsString(c.moves[s.Commit], x) { + both = append(both, x) + } + } + if len(both) > 0 { + return s.Commit, both + } + } + } + return "", nil + } a := StopAnswer{Stopped: walk} var again, left []WalkMerge for _, m := range wd.Merges { + s, both := shares(m) switch { case named(m): a.LeftOut = append(a.LeftOut, WalkLeftOut{Repository: m.Repository, Commit: m.Commit}) left = append(left, m) + case len(both) > 0: + a.LeftOut = append(a.LeftOut, WalkLeftOut{Repository: m.Repository, Commit: m.Commit, Contains: s, Shares: both}) + left = append(left, m) case contains(m) != "": a.LeftOut = append(a.LeftOut, WalkLeftOut{Repository: m.Repository, Commit: m.Commit, Contains: contains(m)}) left = append(left, m) @@ -249,6 +277,14 @@ func (c *fakeController) StopWithout(walk, why, by string, without []string) (St if len(a.LeftOut) == 0 { return StopAnswer{}, fmt.Errorf("%s answers no merge %s", walk, strings.Join(without, ", ")) } + if w.IsBatch() && len(again) == 0 { + // Every merge left out: the batch itself is stopped, as the controller does. + wd.Stopped, wd.StoppedWhy, wd.LeftOut, wd.Batch = by, why, a.LeftOut, nil + w.Delivery, w.State = &wd, walkFailed + w.Revision++ + c.walks[walk] = w + return a, nil + } if w.IsBatch() { rid := walk + "-left" c.walks[rid] = Walk{ID: rid, Repository: left[0].Repository, Branch: "main", Commit: left[0].Commit, diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 841a5c10..23d1185d 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -61,6 +61,9 @@ type Holder struct { leaving map[string][]string // leaveAsked is when a walk was last asked to leave out the stopped merges it carries (leaveStopped). leaveAsked map[string]leaveAsk + // 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 } // unmatched is a walk no delivery is known for yet: its merge not heard, or merged before this owner held. @@ -650,7 +653,7 @@ func (h *Holder) linkWalk(d *Delivery, w Walk, carried string) { // containing a stopped one, or is walked again by the batch the record names. if w.Delivery != nil && (w.Delivery.WalkedAgainBy != "" || len(w.Delivery.LeftOut) > 0) { if o, out := w.leftOut(d.Repository, d.MergedAs); out { - d.Walk.Contains = o.Contains + d.Walk.Contains, d.Walk.Shares = o.Contains, o.Shares } else { d.Walk.WalkedAgainBy = w.Delivery.WalkedAgainBy } @@ -1882,6 +1885,12 @@ func (h *Holder) stopLeaving(walk, why, by string, without []string) string { } refused := err if err := h.Controller.Stop(walk, fmt.Sprintf("%s, and it could not be left out alone (%v)", why, refused), by); err != nil { + h.mu.Lock() + if h.leaveRefused == nil { + h.leaveRefused = map[string]string{} + } + h.leaveRefused[walk] = fmt.Sprintf("leaving it out (%v), and stopping the walk whole (%v)", refused, err) + h.mu.Unlock() return fmt.Sprintf("the walk %s could not be stopped without %s (%v), nor whole (%v): `plans stop %s --why …` "+ "ends it by hand", walk, strings.Join(without, ", "), refused, err, walk) } @@ -1906,7 +1915,12 @@ func (h *Holder) carriesStopped(walk string) ([]string, string) { slices.Sort(ids) for _, id := range ids { d := h.deliveries[id] - if d.State != Stopped || d.MergedAs == "" { + // Stopped by a person or by its group, the acts that hold a change back: never one ended by what its walk + // did (the observed rows, which name nobody), such as a merge stranded after its walk (issue 474). + if d.State != Stopped || d.MergedAs == "" || len(d.Transitions) == 0 { + continue + } + if last := d.Transitions[len(d.Transitions)-1]; last.Event != EvStop || last.By == "" { continue } // By the merges it names alone: its own commit may be another's merge it no longer answers. @@ -1956,9 +1970,32 @@ func (h *Holder) leaveStopped() []func() { defer h.mu.Unlock() now := h.Now() var asks []func() + held := map[string]string{} + for id := range h.walks { + 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 r := h.leaveRefused[id]; r != "" { + said += "; the controller refused: " + r + } + held[id] = said + } + } + // What the waiting deliveries say: `waits`, and `stalled` past their bound. + for _, d := range h.deliveries { + said := "" + if d.Walk != nil && !d.State.Final() { + said = held[d.Walk.ID] + } + if d.HeldBack != said { + d.HeldBack = said + h.keep(d) + } + } for id, seen := range h.walks { without, why := h.carriesStopped(id) if len(without) == 0 { + delete(h.leaveRefused, id) continue } // Once per record of it: the record that answers an ask is newer. Asked again after five minutes, should @@ -2033,7 +2070,10 @@ func againWords(a StopAnswer) string { var left []string for _, o := range a.LeftOut { s := o.Repository + "@" + shortOf(o.Commit, 12) - if o.Contains != "" { + switch { + case len(o.Shares) > 0: + s += " (moves " + strings.Join(o.Shares, ", ") + ", as " + shortOf(o.Contains, 12) + " does)" + case o.Contains != "": s += " (contains " + shortOf(o.Contains, 12) + ")" } left = append(left, s) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go index e70b3671..20cdfe89 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go @@ -381,3 +381,95 @@ func TestAStopSaysWhatStaysAndWhatStillCarriesIt(t *testing.T) { } } } + +// A merge left out for moving a module the stopped merge moves, from another repository, ends stopped saying so — +// never that it contains the stopped merge (review of #221). +func TestALeftOutMergeSharingAModuleSaysSoTruly(t *testing.T) { + s := open459(t, []string{"novox/a", "novox/b"}, []string{"novox/z"}) + s.ctl.mu.Lock() + s.ctl.moves[s.merges["novox/b"]], s.ctl.moves[s.merges["novox/z"]] = []string{"lab"}, []string{"lab"} + s.ctl.mu.Unlock() + s.failFirst() + s.heard("plan-2") + z := s.delivery(s.ids["novox/z"]) + why := lastWhy(z) + if z.State != Stopped || !strings.Contains(why, "moves lab, as the stopped merge "+shortOf(s.merges["novox/b"], 12)) || + strings.Contains(why, "contains the stopped merge") { + t.Fatalf("z is %s (%q); want it stopped, saying it moves lab as b's merge does", z.State, why) + } +} + +// A person's first answer names the deliveries that would be left out for moving a module this merge moves, and says +// the controller may leave out more. +func TestAPersonsFirstAnswerNamesWhatSharesItsModules(t *testing.T) { + s := shared459(t) + _, err := s.h.StopWay(s.ids["novox/y"], "x", "jochen", "") + if err == nil || !strings.Contains(err.Error(), "for moving a module this merge moves: "+s.ids["novox/z"]+" (moves app)") || + !strings.Contains(err.Error(), "may leave out more") { + t.Fatalf("the first answer said %v", err) + } +} + +// The safety net holds back only what a person or a group stopped: a delivery ended by what its walk did (the +// observed rows, which name nobody) is not asked out of a walk that carries its merge. +func TestTheSafetyNetLeavesAStrandedStopAlone(t *testing.T) { + s := shared459(t) + s.h.mu.Lock() + y := s.h.deliveries[s.ids["novox/y"]] + y.State = Stopped + y.Transitions = append(y.Transitions, Transition{At: s.now, From: Published, To: Stopped, Event: EvStop, + Why: "its walk ended done before its word"}) + s.h.mu.Unlock() + s.settleAll() + if len(s.ctl.withouts) != 0 { + t.Fatalf("a stop nobody asked for was held back: %v", s.ctl.withouts) + } +} + +// A walk the safety net holds — its leave-out and its whole stop both refused — says why on the deliveries waiting on +// it, in `waits`, and in `stalled` past their bound. +func TestAHeldWalkIsSaidOnItsDeliveriesAndStalls(t *testing.T) { + s := shared459(t) + if _, err := s.h.StopWay(s.ids["novox/y"], "not wanted after all", "jochen", "again"); err != nil { + t.Fatal(err) + } + s.ctl.refuseWithout, s.ctl.refuseStop = true, true + x := aWalk("plan-x", "novox/z", s.merges["novox/z"], true) + x.Created = x.Created.Add(10 * time.Minute) + x.Delivery.Merges = []WalkMerge{{Repository: "novox/y", Commit: s.merges["novox/y"]}, + {Repository: "novox/z", Commit: s.merges["novox/z"]}} + s.h.WalkMoved(s.ctl.put(x)) + s.settleAll() + z := s.delivery(s.ids["novox/z"]) + if !strings.Contains(z.HeldBack, "plan-x") || !strings.Contains(z.HeldBack, "refused") { + t.Fatalf("z says it is held back by %q; want plan-x and the refusal", z.HeldBack) + } + if w := s.h.waitsFor(z); w != z.HeldBack { + t.Fatalf("z waits for %q", w) + } + s.later(Bounds[Published].For + time.Minute) + s.settleAll() + for _, l := range s.h.Stalled() { + if l.ID == z.ID && strings.Contains(l.Says, "plan-x") { + return + } + } + t.Fatalf("z is not listed stalled naming plan-x: %+v", s.h.Stalled()) +} + +// A person's stop of a delivery whose merge waits in a batch asks that batch now to leave it out, and says so. +func TestAPersonsStopOfAMergeInABatchAsksTheBatch(t *testing.T) { + s := shared459(t) + if _, err := s.h.StopWay(s.ids["novox/y"], "not wanted after all", "jochen", "again"); err != nil { + t.Fatal(err) + } + s.heard("plan-2", "plan-2-again") + said, err := s.h.StopWay(s.ids["novox/z"], "this one too", "jochen", "") + if err != nil { + t.Fatal(err) + } + if want := "plan-2-again without novox/z@" + s.merges["novox/z"]; !containsString(s.ctl.withouts, want) || + !strings.Contains(said, "batch plan-2-again") { + t.Fatalf("said %q, asked %v; want the batch asked now, and said", said, s.ctl.withouts) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/model.go b/modules/mesh-delivery/cmd/mesh-delivery/model.go index 0fb09377..74315c8b 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/model.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/model.go @@ -71,7 +71,10 @@ type Delivery struct { // 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. - TurnAwaits string `json:"turn_awaits,omitempty"` + TurnAwaits string `json:"turn_awaits,omitempty"` + // HeldBack is why the walk it waits on is not let go: it carries a merge whose delivery was stopped, and is + // asked to leave it out (novox/hq ADR 0299). Said by `waits`, and by `stalled` past its bound. + HeldBack string `json:"held_back,omitempty"` TurnAwaitsUntil *time.Time `json:"turn_awaits_until,omitempty"` // ContainedIn is the forge's answer, by a later merge commit, to whether that commit contains this one's // merge (gitea_contains): asked once per pair, and kept (ADR 0270). @@ -409,6 +412,9 @@ type WalkLeftOut struct { Repository string `json:"repository"` Commit string `json:"commit"` Contains string `json:"contains,omitempty"` + // Shares are the modules it moves that a stopped merge moves too, when that is why it was left out: Contains + // then names that stopped merge, which it does not contain. + Shares []string `json:"shares,omitempty"` } // leftOut is the merge of a repository a stop left out, when it did. @@ -492,12 +498,15 @@ type WalkSeen struct { // StoppedWhy is why it was stopped; WalkedAgainBy, on a walk stopped without some merges, the batch that walks // this delivery's merge again; Contains, when this delivery's merge was left out for containing a stopped // merge, that merge (novox/hq ADR 0299). - StoppedWhy string `json:"stopped_why,omitempty"` - WalkedAgainBy string `json:"walked_again_by,omitempty"` - Contains string `json:"contains,omitempty"` - Tier int `json:"tier"` - Tiers int `json:"tiers"` - Updated time.Time `json:"updated"` + StoppedWhy string `json:"stopped_why,omitempty"` + WalkedAgainBy string `json:"walked_again_by,omitempty"` + Contains string `json:"contains,omitempty"` + // Shares are, when this delivery's merge was left out for moving modules a stopped merge moves, those modules: + // built at the branch, which holds the stopped merge, they would deliver it (ADR 0299). + Shares []string `json:"shares,omitempty"` + Tier int `json:"tier"` + Tiers int `json:"tiers"` + Updated time.Time `json:"updated"` // Branch is the branch the walk's commit was merged into; empty in a record kept before it was read. Branch string `json:"branch,omitempty"` // Created is when the controller made the walk: a delivery follows the newest walk that answers it. @@ -558,7 +567,12 @@ func (d *Delivery) stoppedWords() string { if d.Walk.StoppedWhy != "" { s += ": " + d.Walk.StoppedWhy } - if d.Walk.Contains != "" { + switch { + case len(d.Walk.Shares) > 0: + s += fmt.Sprintf("; its merge moves %s, as the stopped merge %s does: built at the branch, which holds that "+ + "merge, it would deliver it, so it was left out with it (novox/hq ADR 0299)", strings.Join(d.Walk.Shares, ", "), + shortOf(d.Walk.Contains, 12)) + case d.Walk.Contains != "": s += fmt.Sprintf("; its merge contains the stopped merge %s, so it cannot be walked without delivering it, "+ "and was left out with it (novox/hq ADR 0299)", shortOf(d.Walk.Contains, 12)) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/table.go b/modules/mesh-delivery/cmd/mesh-delivery/table.go index 707bd563..5203ee17 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/table.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/table.go @@ -568,6 +568,12 @@ func boundOf(d *Delivery) (Bound, bool) { if d.waitsForItsOwnWalk() { return rewalkBound, true } + if (d.State == Published || d.State == Delivering) && d.HeldBack != "" { + // Its walk carries a stopped merge and is not let go (ADR 0299): said with why, past the bound of its state. + b := Bounds[d.State] + b.For, b.Says = Bounds[Published].For, d.HeldBack // a walk not let go waits for a word, whatever its state + return b, true + } if d.State == Published && d.TurnAwaits != "" && d.TurnAwaitsUntil != nil { // Its turn waits for a later merge to carry a member before it: bounded by that carrier's own bound, and // said with it, never the generic "waits for its turn or its word" (novox/hq issue 445). diff --git a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go index cb389a14..9771e281 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go @@ -29,6 +29,9 @@ type Line struct { // waitsFor is what a delivery waits for, in words. func (h *Holder) waitsFor(d *Delivery) string { + if d.HeldBack != "" && !d.State.Final() { + return d.HeldBack + } switch d.State { case Proposed: return "its check" @@ -393,6 +396,11 @@ func (h *Holder) StopWay(id, why, by, way string) (string, error) { walk = d.Walk.ID } merge := d.Repository + "@" + d.MergedAs + // A merge that waits in a batch not yet cut: the batch is asked to leave it out once it is stopped (ADR 0299). + var batch string + if b, batched := h.batchOf(d); batched && walk == "" { + batch = b.ID + } var others []string if walk != "" { others = h.othersOn(walk, []string{merge}) @@ -452,6 +460,11 @@ func (h *Holder) StopWay(id, why, by, way string) (string, error) { for _, a := range asks { a() } + if batch != "" { + // Asked now, not at the next tick; and the batch's walk is never let go while it carries the merge. + did = fmt.Sprintf("; its merge waits in the batch %s, which was asked now to leave it out and is never let go "+ + "while it carries it: %s", batch, h.stopLeaving(batch, why, by, []string{merge})) + } return fmt.Sprintf("%s is stopped, on %s's word%s", id, by, did), nil } @@ -472,11 +485,40 @@ func (h *Holder) takenDown(d *Delivery, walk string, others []string) string { if len(contain) > 0 { s += fmt.Sprintf(" These contain its merge and end with it either way: %s.", strings.Join(contain, ", ")) } + // A module is built at its branch, which holds this merge: a delivery moving a module this one moves would + // deliver it, and the controller leaves it out too (ADR 0299). Its plan is what is known here; the controller + // decides, and its answer names each. + var share []string + for _, o := range ride { + if od := h.deliveries[o]; od != nil && od.Plan != nil && d.Plan != nil { + if both := sharedModules(od.Plan.Moved, d.Plan.Moved); len(both) > 0 { + share = append(share, fmt.Sprintf("%s (moves %s)", o, strings.Join(both, ", "))) + } + } + } + if len(share) > 0 { + s += fmt.Sprintf(" With `way: %s` these are left out too, for moving a module this merge moves: %s.", wayAgain, + strings.Join(share, ", ")) + } + s += fmt.Sprintf(" With `way: %s` the controller may leave out more for sharing a module, and its answer names each.", + wayAgain) return s + fmt.Sprintf(" What the walk already sent stays where it was sent; the stop's answer names it."+ " Choose a way: `way: %s` ends the walk and walks the others again without this merge; "+ "`way: %s` stops the whole walk and every delivery on it.", wayAgain, wayWalk) } +// sharedModules are the modules in both lists, sorted. +func sharedModules(a, b []string) []string { + var out []string + for _, m := range a { + if containsString(b, m) && !containsString(out, m) { + out = append(out, m) + } + } + sort.Strings(out) + return out +} + // containsMerge is whether o is a later merge of d's repository and branch, which contains d's merge on a linear // trunk: merged after it, or the merge the walk carries for it. func containsMerge(o, d *Delivery) bool {