Merge pull request 'A delivery whose walk was folded into a batch follows that batch's walk and ends delivered (issue 1786)' (#426) from fix/1786-follow-taken-over-walk into main

2026-10-21 22:26:33 +00:00
parent a3794ea156 c4176107f2
commit 839be0eb64
5 changed files with 174 additions and 8 deletions
@@ -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() {
@@ -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() }
@@ -1396,10 +1396,31 @@ 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.State.Final() || d.Walk == nil || d.Walk.TakenOverBy == "" {
+		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 {
+			if s.walk.IsBatch() {
+				// A batch is no walk yet (ADR 0276): the walk it is cut into answers the merge. Waited for, never
+				// read as a walk that does not answer it (issue 1786).
+				continue
+			}
 			if c, ok := walkAnswers(s.walk, d); ok {
 				h.linkWalk(d, s.walk, c)
 			} else {
@@ -2181,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
@@ -0,0 +1,126 @@
+package main
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+// novox/hq issue 1786: two merges whose walks the controller folded, before they were cut, into a batch that was
+// still queued (not yet cut into a walk) ended superseded: the batch is no walk that answers a merge, so the follow
+// loop took the walk's taken_over_by off ("the walk that took its own over does not answer it") and the table's
+// supersession then held ("names no walk that took it over"). Delivered by the batch's walk, they showed
+// superseded for good. A batch that took a walk over is waited for; and one already ended so follows its taker.
+
+const (
+	head1786  = "1786178617861786178617861786178617861786"
+	merge1786 = "a1786a1786a1786a1786a1786a1786a1786a17860"
+)
+
+// folded1786 is a delivery whose walk was folded into a batch that is still queued.
+func folded1786(t *testing.T) (*world, string) {
+	t.Helper()
+	w := newWorld(t)
+	id := IDOf("novox/app", head1786)
+	w.h.PullUpdated(pr("novox/app", 7, head1786, "feat/x", "src/a.go"))
+	w.h.Checked(verdict("novox/app", 7, head1786, "pass"))
+	w.h.PullMerged(merged("novox/app", 7, head1786, merge1786))
+	w.h.WalkMoved(w.ctl.put(aWalk("plan-own", "novox/app", merge1786, true)))
+	w.settleAll()
+	if s := w.state(id); s != Delivering {
+		t.Fatalf("setup: %s, want delivering", s)
+	}
+	closes := w.now.Add(time.Minute)
+	batch := Walk{ID: "plan-batch", Repository: "novox/other", Branch: "main", Commit: "ffff", Created: w.now, State: walkQueued,
+		Delivery: &WalkDelivery{Awaits: "mesh-delivery", Batch: &WalkBatch{ClosesAt: &closes},
+			Merges: []WalkMerge{{Repository: "novox/app", Commit: merge1786}}}}
+	w.h.WalkMoved(w.ctl.put(batch))
+	folded := w.ctl.walk("plan-own")
+	folded.State = walkSuperseded
+	folded.Note = "folded into plan-batch before it was cut"
+	folded.Delivery.TakenOverBy = "plan-batch"
+	w.h.WalkMoved(w.ctl.put(folded))
+	w.settleAll()
+	return w, id
+}
+
+func cut1786(w *world, state string) {
+	b := w.ctl.walk("plan-batch")
+	b.State = state
+	b.Delivery.Batch = nil
+	b.Tiers, b.Modules = [][]string{{"app"}}, map[string]*WalkModule{"app": {}}
+	w.h.WalkMoved(w.ctl.put(b))
+	w.settleAll()
+}
+
+func TestAWalkFoldedIntoAQueuedBatchIsNotSupersededAndFollowsTheBatchWhenItIsCut(t *testing.T) {
+	w, id := folded1786(t)
+	if d := w.delivery(id); d.State.Final() || d.Walk.TakenOverBy != "plan-batch" {
+		t.Fatalf("folded into a queued batch it is %s, taken over by %q; wanted it still following plan-batch", d.State, d.Walk.TakenOverBy)
+	}
+	cut1786(w, walkBuilding)
+	cut1786(w, walkDone)
+	if s := w.state(id); s != Delivered {
+		t.Fatalf("the batch's walk is done and it is %s; wanted delivered", s)
+	}
+}
+
+func TestADeliverySupersededWhileItsTakerWasABatchIsHealedWhenTheTakerEndsDone(t *testing.T) {
+	w, id := folded1786(t)
+	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})
+	if err := w.h.Store.PutDelivery(d); err != nil {
+		t.Fatal(err)
+	}
+	cut1786(w, walkBuilding)
+	cut1786(w, walkDone)
+	w.later(time30s)
+	w.settleAll()
+	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)
+	}
+}
+
+func TestASupersededDeliveryWithNoWalkThatTookItOverStaysSuperseded(t *testing.T) {
+	w := newWorld(t)
+	id := IDOf("novox/app", head1786)
+	w.h.PullUpdated(pr("novox/app", 7, head1786, "feat/x", "src/a.go"))
+	w.h.PullUpdated(pr("novox/app", 7, "b"+head1786[1:], "feat/x", "src/a.go"))
+	w.settleAll()
+	if s := w.state(id); s != Superseded {
+		t.Fatalf("a head superseded by a newer one is %s", s)
+	}
+	w.later(time30s)
+	w.settleAll()
+	if s := w.state(id); s != Superseded {
+		t.Fatalf("it moved: %s", s)
+	}
+}
@@ -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)
+}