Keep a delivery superseded by its walk final, and heal it as delivered by the walk that took its walk over (issue 1786)

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go

@@ -154,6 +154,9 @@ func statusOf(d *Delivery) (string, string) {
 		}
 		return "failure", clip("failed: "+why, 140)
 	case Superseded:
+		if d.coveredStop() {
+			return "success", clip(d.coveredStopSaid(), 140) // its walk's taker delivered it (issue 1786)
+		}
 		return "warning", clip("superseded: "+why, 140)
 	case Stopped:
 		if d.coveredStop() {

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go

@@ -601,10 +601,11 @@ type world struct {
 	source *fakeSource
 	owed   *fakeOwed
 	// asked is the key of the ask a test answered.
-	asked string
-	now   time.Time
-	said  []map[string]any
-	mu    sync.Mutex
+	asked  string
+	now    time.Time
+	logbuf strings.Builder
+	said   []map[string]any
+	mu     sync.Mutex
 }
 
 func newWorld(t *testing.T) *world {
@@ -615,7 +616,7 @@ func newWorldOver(t *testing.T, store *memStore, ctl *fakeController, forge *fak
 	w := &world{t: t, store: store, ctl: ctl, forge: forge, now: now}
 	w.h = &Holder{Store: store, Controller: ctl, Forge: forge,
 		Now:  func() time.Time { w.mu.Lock(); defer w.mu.Unlock(); return w.now },
-		Logf: func(format string, a ...any) { t.Logf(format, a...) },
+		Logf: func(format string, a ...any) { t.Logf(format, a...); fmt.Fprintf(&w.logbuf, format+"\n", a...) },
 		Emit: func(event string, body any) error {
 			w.mu.Lock()
 			defer w.mu.Unlock()
@@ -719,3 +720,5 @@ func (f *fakeForge) Comment(owner, repo string, number int, body string) error {
 	f.calls = append(f.calls, "comment "+body)
 	return nil
 }
+
+func (w *world) logs() string { return w.logbuf.String() }

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go

@@ -1396,7 +1396,23 @@ func (h *Holder) plan() []func() {
 	// Following a walk that was taken over: the walk that took it over, read as soon as it is heard of, asked for
 	// by its id when it is not.
 	for _, d := range h.deliveries {
-		if d.Walk == nil || d.Walk.TakenOverBy == "" || d.State.Final() && !d.supersededByItsWalk() {
+		if d.Walk == nil || d.Walk.TakenOverBy == "" {
+			continue
+		}
+		if d.State.Final() {
+			// Ended superseded by its walk before the walk that took it over was heard to answer (issue 1786): the
+			// state stays final, as a covered superseded member's does (issue 442), and the end of that walk is
+			// kept as DeliveredBy, which the group and the views read. Never logged, never changed otherwise.
+			if d.supersededByItsWalk() && d.DeliveredBy == "" {
+				if s, known := h.walks[d.Walk.TakenOverBy]; known {
+					if _, ok := walkAnswers(s.walk, d); ok && s.walk.State == walkDone {
+						d.DeliveredBy = fmt.Sprintf("the walk %s, which took its walk %s over, ended done", s.walk.ID, d.Walk.ID)
+						h.tellCovered(d, now)
+					}
+				} else if a := h.askWalk(d.Walk.TakenOverBy, "which took a delivery's walk over", now); a != nil {
+					asks = append(asks, a)
+				}
+			}
 			continue
 		}
 		if s, known := h.walks[d.Walk.TakenOverBy]; known {
@@ -1411,9 +1427,7 @@ func (h *Holder) plan() []func() {
 				// The walk that took its own over does not answer it: superseded, as before ADR 0276.
 				h.Logf("[mesh-delivery] %s: the walk %s, which took %s over, does not answer its merge", d.ID,
 					d.Walk.TakenOverBy, d.Walk.ID)
-				if !d.State.Final() {
-					d.Walk.TakenOverBy = ""
-				}
+				d.Walk.TakenOverBy = ""
 			}
 			h.settleAndKeep(d)
 			continue
@@ -2188,7 +2202,7 @@ func (h *Holder) planGroup(g *Group, members []*Delivery, now time.Time) []func(
 	// moves — and its group counts it as reached (novox/hq issue 442).
 	if g.Closed {
 		for _, d := range members {
-			if d.State != Superseded || d.CoveredBy != "" {
+			if d.State != Superseded || d.CoveredBy != "" || d.DeliveredBy != "" {
 				continue
 			}
 			var ask []string

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1786_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"strings"
 	"testing"
 	"time"
 )
@@ -64,9 +65,9 @@ func TestAWalkFoldedIntoAQueuedBatchIsNotSupersededAndFollowsTheBatchWhenItIsCut
 	}
 }
 
-func TestADeliverySupersededWhileItsTakerWasABatchFollowsTheTakerOnceItEnds(t *testing.T) {
+func TestADeliverySupersededWhileItsTakerWasABatchIsHealedWhenTheTakerEndsDone(t *testing.T) {
 	w, id := folded1786(t)
-	d := w.delivery(id)
+	d := w.h.deliveries[id]
 	// The state kept by the fault: superseded, its walk record naming the walk that took it over.
 	d.State = Superseded
 	d.Transitions = append(d.Transitions, Transition{At: w.now, From: Delivering, To: Superseded, Event: EvSuperseded})
@@ -77,8 +78,34 @@ func TestADeliverySupersededWhileItsTakerWasABatchFollowsTheTakerOnceItEnds(t *t
 	cut1786(w, walkDone)
 	w.later(time30s)
 	w.settleAll()
-	if s := w.state(id); s != Delivered {
-		t.Fatalf("superseded by the fault and its taker done, it is %s; wanted delivered", s)
+	d = w.delivery(id)
+	// A final state stays final (ADR 0239 §5): the taker's end is kept as DeliveredBy, as issue 442 and 747 do.
+	if d.State != Superseded || d.DeliveredBy == "" || !memberReached(d) || !d.coveredStop() {
+		t.Fatalf("superseded by the fault and its taker done, it is %s, delivered by %q, reached %v; wanted superseded, delivered by the taker, reached",
+			d.State, d.DeliveredBy, memberReached(d))
+	}
+	if st, _ := statusOf(d); st != "success" {
+		t.Fatalf("its status is %s; wanted success", st)
+	}
+}
+
+func TestAFinalDeliveryWhoseTakerDoesNotAnswerItIsNeitherHealedNorLoggedEveryPass(t *testing.T) {
+	w, id := folded1786(t)
+	d := w.h.deliveries[id]
+	d.State = Superseded
+	d.Transitions = append(d.Transitions, Transition{At: w.now, From: Delivering, To: Superseded, Event: EvSuperseded})
+	other := aWalk("plan-batch", "novox/other", "ffff", false)
+	other.State = walkDone
+	w.h.WalkMoved(w.ctl.put(other))
+	w.settleAll()
+	before := w.logs()
+	w.later(time30s)
+	w.settleAll()
+	if d := w.delivery(id); d.DeliveredBy != "" || d.State != Superseded {
+		t.Fatalf("healed by a walk that does not answer it: %s %q", d.State, d.DeliveredBy)
+	}
+	if n := strings.Count(w.logs(), "does not answer its merge") - strings.Count(before, "does not answer its merge"); n != 0 {
+		t.Fatalf("logged %d times on a pass over a final record", n)
 	}
 }
 

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go

@@ -351,7 +351,7 @@ func (d *Delivery) coveredStop() bool {
 	switch d.State {
 	case Stopped:
 		return d.Stranded != "" && d.CoveredBy != "" || d.DeliveredBy != ""
-	case Failed:
+	case Failed, Superseded:
 		return d.DeliveredBy != ""
 	}
 	return false
@@ -397,6 +397,9 @@ func (d *Delivery) strandedEndedSaid() string {
 
 // 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 {
+	if d.State == Superseded {
+		return "delivered after its walk was superseded: " + d.DeliveredBy
+	}
 	ended := "its stop"
 	if d.State == Failed {
 		ended = "its failure"
@@ -1071,3 +1074,13 @@ func mustJSON(v any) json.RawMessage {
 	raw, _ := json.Marshal(v)
 	return raw
 }
+
+// supersededByItsWalk is whether a delivery ended superseded because its walk was: its last move was a supersession
+// from published or delivering, not a newer head of its pull request's (issue 1786).
+func (d *Delivery) supersededByItsWalk() bool {
+	if d.State != Superseded || len(d.Transitions) == 0 {
+		return false
+	}
+	t := d.Transitions[len(d.Transitions)-1]
+	return t.Event == EvSuperseded && (t.From == Published || t.From == Delivering)
+}

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table.go

@@ -86,8 +86,6 @@ const (
 	EvLifted          Event = "lifted"           // what held it for a person is no longer so: its group's composed check passed, or a walk now answers its merge
 	EvRewalk          Event = "rewalk"           // the walk that carried a later commit for it failed, or was stopped for another merge (ADR 0299): it waits for the walk that walks it again
 	EvBackupFailed    Event = "backup failed"    // a backup before the send failed or stayed silent: nothing of its plan was sent (novox/hq ADR 0404)
-	EvFollowed        Event = "followed"         // its walk was superseded by a walk that took it over, which has ended: it follows that walk's end (issue 1786)
-	EvFollowedFailed  Event = "followed failed"  // the same, and that walk failed
 	EvTrunkMoved      Event = "trunk moved"      // another merge moved its trunk, and its verdict names the trunk commit before (novox/hq ADR 0323)
 )
 
@@ -397,23 +395,6 @@ var Table = []Row{
 		Says: func(d *Delivery) string {
 			return "covered: a later merge that contains it was delivered: " + d.Carrier
 		}},
-	{From: []State{Superseded}, Event: EvFollowed, To: Delivered,
-		Guard: "it was ended superseded by its walk, which a walk took over (issue 1786), and the walk it follows now is done, carrying its own commit",
-		Holds: func(d *Delivery, f Facts) error {
-			if !d.supersededByItsWalk() {
-				return errors.New("it was not ended by a walk superseded")
-			}
-			return unless(d.Walk != nil && d.Walk.State == walkDone && !d.carriedLater(), "the walk it follows is not done, or carried a later commit for it")
-		}},
-	{From: []State{Superseded}, Event: EvFollowedFailed, To: Failed,
-		Guard: "it was ended superseded by its walk, which a walk took over (issue 1786), and the walk it follows now failed, carrying its own commit",
-		Holds: func(d *Delivery, f Facts) error {
-			if !d.supersededByItsWalk() {
-				return errors.New("it was not ended by a walk superseded")
-			}
-			return unless(d.Walk != nil && d.Walk.State == walkFailed && d.Walk.StoppedBy == "" && !d.carriedLater(),
-				"the walk it follows did not fail, or carried a later commit for it")
-		}},
 	{From: []State{Delivering}, Event: EvStop, To: Stopped,
 		Guard: "its walk was stopped through this owner, and walks its merge again nowhere: left out, or the walk ended " +
 			"whole (novox/hq ADR 0299)",
@@ -752,14 +733,3 @@ func TableText() map[string]any {
 	}
 	return map[string]any{"transitions": rows, "bounds": bounds, "machine-steps": steps}
 }
-
-// supersededByItsWalk is whether a delivery ended superseded because its walk was: its last move was a supersession
-// from published or delivering, not a newer head of its pull request's (issue 1786). Such a delivery follows the walk
-// that took its walk over, once that is heard, whatever it was ended as before.
-func (d *Delivery) supersededByItsWalk() bool {
-	if d.State != Superseded || d.Walk == nil || len(d.Transitions) == 0 {
-		return false
-	}
-	t := d.Transitions[len(d.Transitions)-1]
-	return t.Event == EvSuperseded && (t.From == Published || t.From == Delivering)
-}

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table_test.go

@@ -95,13 +95,6 @@ func aDeliveryFor(r Row) (*Delivery, Facts) {
 		d.Walk = &WalkSeen{ID: "plan-1", State: walkFailed}
 	case EvSuperseded:
 		d.Walk = &WalkSeen{ID: "plan-1", State: walkSuperseded}
-	case EvFollowed, EvFollowedFailed:
-		// Ended superseded by its walk, which a walk took over (issue 1786); that walk has ended now.
-		d.Transitions = []Transition{{From: Delivering, To: Superseded, Event: EvSuperseded}}
-		d.Walk = &WalkSeen{ID: "plan-2", State: walkDone}
-		if r.Event == EvFollowedFailed {
-			d.Walk.State = walkFailed
-		}
 	}
 	return d, f
 }
@@ -111,13 +104,12 @@ func TestEveryRowOfTheTableIsTakenWhenItsGuardHoldsAndRefusedWhenNot(t *testing.
 		for _, from := range r.From {
 			d, f := aDeliveryFor(r)
 			d.State = from
-			kept := len(d.Transitions)
 			got, err := Apply(d, r.Event, r.Act, f)
 			if err != nil {
 				t.Errorf("%s —%s→ %s was refused with its guard holding: %v", orNothing(from), r.Event, r.To, err)
 				continue
 			}
-			if d.State != r.To || got.From != from || got.To != r.To || len(d.Transitions) != kept+1 {
+			if d.State != r.To || got.From != from || got.To != r.To || len(d.Transitions) != 1 {
 				t.Errorf("%s —%s→ left it %s, transition %+v", orNothing(from), r.Event, d.State, got)
 			}
 			// The guard not holding: a delivery with no facts.
@@ -143,7 +135,7 @@ func TestEveryRowOfTheTableIsTakenWhenItsGuardHoldsAndRefusedWhenNot(t *testing.
 func TestEveryPairTheTableDoesNotHoldIsRefusedByName(t *testing.T) {
 	events := []Event{EvAnnounced, EvAppeared, EvAdopted, EvChecked, EvAccepted, EvRefused, EvRecheck, EvReask, EvNewHead,
 		EvClosed, EvMerged, EvMergedUnchecked, EvGo, EvHold, EvRelease, EvDone, EvFailed, EvSuperseded, EvStop,
-		EvHistory, EvRetire, EvCovered, EvLifted, EvRewalk, EvTrunkMoved, EvBackupFailed, EvFollowed, EvFollowedFailed}
+		EvHistory, EvRetire, EvCovered, EvLifted, EvRewalk, EvTrunkMoved, EvBackupFailed}
 	for _, from := range append([]State{None}, AllStates...) {
 		for _, ev := range events {
 			for _, act := range []bool{false, true} {
@@ -170,8 +162,7 @@ func TestTheTableKeepsItsRules(t *testing.T) {
 	for _, r := range Table {
 		for _, from := range r.From {
 			// Nothing leaves a final state.
-			// The one exception: a delivery superseded by its walk follows the walk that took it over (issue 1786).
-			if from.Final() && r.Event != EvFollowed && r.Event != EvFollowedFailed {
+			if from.Final() {
 				t.Errorf("%s is final and the table leaves it by %s", from, r.Event)
 			}
 			// Nothing goes back before the trunk once a delivery is on it.