diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go index c5f0aeb4..e765003a 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go +++ b/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() { diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go index 61de32c0..d7c47f8e 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go +++ b/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() } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go index 9b6d8135..35599fb2 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -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 diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1786_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1786_test.go new file mode 100644 index 00000000..79a4f60c --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1786_test.go @@ -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) + } +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go index 791d2c26..c2d9b806 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go +++ b/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) +}