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.

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).
 //

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)
+}

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
 }