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 b431eef3..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,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 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 index a7feadde..79a4f60c 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1786_test.go +++ b/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) } } 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) +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table.go index cd5479e4..b8ed6d08 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table.go +++ b/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) -} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table_test.go index 42db6d43..573e4935 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table_test.go +++ b/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.