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.

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,

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)

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

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

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

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 {