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..b431eef3 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -1396,17 +1396,24 @@ 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 == "" || d.State.Final() && !d.supersededByItsWalk() { 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 { // 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) - d.Walk.TakenOverBy = "" + if !d.State.Final() { + d.Walk.TakenOverBy = "" + } } h.settleAndKeep(d) continue 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..a7feadde --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1786_test.go @@ -0,0 +1,99 @@ +package main + +import ( + "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 TestADeliverySupersededWhileItsTakerWasABatchFollowsTheTakerOnceItEnds(t *testing.T) { + w, id := folded1786(t) + d := w.delivery(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() + if s := w.state(id); s != Delivered { + t.Fatalf("superseded by the fault and its taker done, it is %s; wanted delivered", s) + } +} + +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/table.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table.go index b8ed6d08..cd5479e4 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/table.go @@ -86,6 +86,8 @@ 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) ) @@ -395,6 +397,23 @@ 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)", @@ -733,3 +752,14 @@ 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 573e4935..42db6d43 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,6 +95,13 @@ 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 } @@ -104,12 +111,13 @@ 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) != 1 { + if d.State != r.To || got.From != from || got.To != r.To || len(d.Transitions) != kept+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. @@ -135,7 +143,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} + EvHistory, EvRetire, EvCovered, EvLifted, EvRewalk, EvTrunkMoved, EvBackupFailed, EvFollowed, EvFollowedFailed} for _, from := range append([]State{None}, AllStates...) { for _, ev := range events { for _, act := range []bool{false, true} { @@ -162,7 +170,8 @@ func TestTheTableKeepsItsRules(t *testing.T) { for _, r := range Table { for _, from := range r.From { // Nothing leaves a final state. - if from.Final() { + // 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 { 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.