From c0bc4b6a835e3274c5494abf632a1bcea70f65ec Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:39:26 +0200 Subject: [PATCH] mesh-delivery: end a shared walk only with the last delivery it carries, so a stop stops only its own (hq issue 459) Since the merge window one walk carries every merge of its batch, and the controller ends a walk whole: stopping one group member, or one delivery on a person's word, ended the rollout of unrelated deliveries on the same walk. A stop now ends the walk only when no live delivery rides it; otherwise the walk goes on and the stopped delivery says so on its commit. --- .../mesh-delivery/cmd/mesh-delivery/holder.go | 60 +++++++++++++- .../cmd/mesh-delivery/issue459_test.go | 80 +++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/verbs.go | 13 ++- 3 files changed, 147 insertions(+), 6 deletions(-) create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 3130671b..5b9f338e 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -1807,7 +1807,7 @@ func mayCarry(c, d *Delivery, module string) bool { c.Plan != nil && slices.Contains(c.Plan.Moved, module) } -// stopInGroup stops a member after one that failed, and its walk if it has one open. +// stopInGroup stops a member after one that failed, and its walk if it has one open and carries nothing else live. func (h *Holder) stopInGroup(d *Delivery, why string) func() { if d == nil || d.State.Final() { return nil @@ -1821,14 +1821,68 @@ func (h *Holder) stopInGroup(d *Delivery, why string) func() { if d.Walk == nil || d.Walk.State == walkDone || d.Walk.State == walkFailed || d.Walk.State == walkSuperseded { return nil } - walk := d.Walk.ID + walk, id := d.Walk.ID, d.ID return func() { - if err := h.Controller.Stop(walk, why, byOwner); err != nil { + if _, _, err := h.endWalk(walk, id, why, byOwner); err != nil { h.Logf("[mesh-delivery] the walk %s could not be stopped: %v", walk, err) } } } +// walkRiders are the live deliveries a walk carries, as "id (state)" in id order: those that follow it, and those +// its record answers and that follow it now. Under the lock. +func (h *Holder) walkRiders(walk string) []string { + s, known := h.walks[walk] + var out []string + for _, d := range h.deliveries { + if d.State.Final() { + continue + } + on := d.Walk != nil && d.Walk.ID == walk + if !on && known && follows(d, s.walk) { + _, on = walkAnswers(s.walk, d) + } + if on { + out = append(out, fmt.Sprintf("%s (%s)", d.ID, d.State)) + } + } + sort.Strings(out) + return out +} + +// endWalk asks the controller to end a walk on a stopped delivery's word, unless the walk still carries a live +// delivery. Since the merge window (ADR 0276) one walk carries every merge of its batch, across repositories and +// groups, and the controller ends a walk whole: ending it for one stopped delivery ended the rollout of every other +// delivery it carries, for a reason not theirs (novox/hq issue 459). While one rides it, the walk goes on — what +// the stopped delivery's walk had asked still builds and is sent with it — and the stopped delivery says so on its +// commit, naming what it goes on for. The last delivery stopped on it ends it. Not under the lock. +func (h *Holder) endWalk(walk, stopped, why, by string) (ended bool, goesOn []string, err error) { + h.mu.Lock() + goesOn = h.walkRiders(walk) + if len(goesOn) > 0 { + line := fmt.Sprintf("%s delivery %s is stopped, and its walk %s goes on: it also carries %s, which were not "+ + "stopped; it is ended once the last delivery it carries is (novox/hq issue 459)", + h.Now().UTC().Format(time.RFC3339), stopped, walk, strings.Join(goesOn, ", ")) + if d := h.deliveries[stopped]; d != nil { + now := h.Now() + d.Owed = append(d.Owed, Effect{Kind: EffectNote, Commit: d.Commit, Line: oneLine(line), Since: now}) + if d.MergedAs != "" && !sameCommit(d.MergedAs, d.Commit) { + d.Owed = append(d.Owed, Effect{Kind: EffectNote, Commit: d.MergedAs, Line: oneLine(line), Since: now}) + } + h.keep(d) + } + h.Logf("[mesh-delivery] the walk %s is not ended for %s: it goes on for %s (novox/hq issue 459)", walk, stopped, + strings.Join(goesOn, ", ")) + h.mu.Unlock() + return false, goesOn, nil + } + h.mu.Unlock() + if err := h.Controller.Stop(walk, why, by); err != nil { + return false, nil, err + } + return true, nil, nil +} + // GroupState is a group's state, derived from its members and its composed check — never set apart (novox/hq // ADR 0239 decision 4, which lists checking, rejected, ready, delivering, delivered, failed and stopped). // diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go new file mode 100644 index 00000000..b0368b5e --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go @@ -0,0 +1,80 @@ +package main + +import ( + "strings" + "testing" +) + +// novox/hq issue 459: since the merge window (ADR 0276) one walk carries every merge of its batch, across +// repositories. Stopping a group member asked the controller to end that member's walk, and with it the rollout of +// every other delivery the walk carries, though none of them is in the group or was stopped. The shape is +// foldShape (issue 445's review of mesh-catalog #203): group member B and C, a delivery of no group, on one walk, +// plan-2. A stop now ends only the stopped delivery; the walk is ended only once no live delivery rides it, and +// while it goes on the stopped delivery says so, naming what it goes on for. + +// B is stopped by its group (the forge says C does not contain A's merge): plan-2 is not ended, C takes its turn +// on it, and B says its walk goes on for C. +func TestAGroupStopLeavesAWalkThatCarriesAnotherDelivery(t *testing.T) { + s := foldShape() + s.forge = map[string]bool{mergedA + " in " + mergedC: false} + w := s.open(t) + stopped445(t, w, "does not contain its merge") + if containsString(w.ctl.stops, "plan-2") { + t.Fatalf("plan-2 was ended %v, and with it C, which is in no group and was not stopped", w.ctl.stops) + } + if d := w.delivery(cID); d.State != Delivering || !containsString(w.ctl.delivers, "plan-2") { + t.Fatalf("C is %s, let go %v; want it delivering on plan-2", d.State, w.ctl.delivers) + } + goesOn459(t, w, headB, cID) + // The walk delivers what it carries: C is delivered, and B stays stopped. + end445(w, "plan-2", walkDone) + if b, c := w.state(bID), w.state(cID); b != Stopped || c != Delivered { + t.Fatalf("once plan-2 is done, B is %s and C is %s; want B stopped and C delivered", b, c) + } +} + +// A person stops B while B and C deliver on plan-2: the walk goes on for C, and the answer says so. Then the person +// stops C too: nothing live rides plan-2 any more, and it is ended. +func TestAPersonsStopEndsTheWalkOnlyWithItsLastDelivery(t *testing.T) { + s := foldShape() + s.forge = map[string]bool{mergedA + " in " + mergedC: true} + w := s.open(t) + letGo445(t, w, "plan-2") + said, err := w.h.Stop(bID, "not wanted after all", "jochen") + if err != nil { + t.Fatal(err) + } + w.settleAll() + if w.state(bID) != Stopped { + t.Fatalf("B is %s", w.state(bID)) + } + if containsString(w.ctl.stops, "plan-2") { + t.Fatalf("plan-2 was ended %v while it carries C", w.ctl.stops) + } + if s := w.state(cID); s != Delivering { + t.Fatalf("C is %s, want delivering", s) + } + if !strings.Contains(said, "goes on") || !strings.Contains(said, cID) { + t.Fatalf("the stop said %q; want it to say plan-2 goes on for C", said) + } + goesOn459(t, w, headB, cID) + + if _, err := w.h.Stop(cID, "this one too", "jochen"); err != nil { + t.Fatal(err) + } + w.settleAll() + if !containsString(w.ctl.stops, "plan-2") { + t.Fatalf("plan-2 was not ended once every delivery it carries was stopped: %v", w.ctl.stops) + } +} + +// goesOn459 is whether the stopped delivery at head said on its commit that its walk goes on, naming other. +func goesOn459(t *testing.T, w *world, head, other string) { + t.Helper() + for _, l := range w.forge.notesOn(head) { + if strings.Contains(l, "plan-2 goes on") && strings.Contains(l, other) { + return + } + } + t.Fatalf("the stopped delivery's notes %q do not say plan-2 goes on for %s", w.forge.notesOn(head), other) +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go index fbee6df1..a3151466 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go @@ -346,8 +346,8 @@ func (h *Holder) Release(id, why, by string) (string, error) { return fmt.Sprintf("%s is delivering on %s's word; its walk %s is let go", id, by, walk), nil } -// Stop is the `stop` verb: a delivery stopped, its walk ended through the controller, and in a group every -// member after it stopped too. +// Stop is the `stop` verb: a delivery stopped, its walk ended through the controller once it carries no other live +// delivery (novox/hq issue 459), and in a group every member after it stopped too. func (h *Holder) Stop(id, why, by string) (string, error) { h.mu.Lock() d := h.deliveries[id] @@ -378,10 +378,17 @@ func (h *Holder) Stop(id, why, by string) (string, error) { if walk == "" { return fmt.Sprintf("%s is stopped, on %s's word", id, by), nil } - if err := h.Controller.Stop(walk, why, by); err != nil { + // The walk is ended only when it carries no other live delivery: since the merge window it may carry merges of + // other repositories and groups, which a stop of this one does not stop (novox/hq issue 459). + ended, goesOn, err := h.endWalk(walk, id, why, by) + if err != nil { return "", fmt.Errorf("%s is stopped, and its walk %s could not be ended yet: %v — `plans stop %s --why …` "+ "ends it by hand", id, walk, err, walk) } + if !ended { + return fmt.Sprintf("%s is stopped, on %s's word; its walk %s goes on for %s, which it also carries and which "+ + "were not stopped", id, by, walk, strings.Join(goesOn, ", ")), nil + } return fmt.Sprintf("%s is stopped, on %s's word; its walk %s is ended", id, by, walk), nil }