diff --git a/modules/mesh-delivery/cmd/mesh-delivery/answers.go b/modules/mesh-delivery/cmd/mesh-delivery/answers.go index 13467d97..2dd8e594 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/answers.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/answers.go @@ -382,6 +382,13 @@ func (h *Holder) words() []func() { continue } ids, why := append([]string(nil), want.ids...), strings.Join(want.whys, "; ") + // **Never the word for a walk that carries a stopped change** (ADR 0299, review of #221): it is asked to + // leave the merge out first (leaveStopped). + if stopped, _ := h.carriesStopped(walk); len(stopped) > 0 { + h.Logf("[mesh-delivery] the walk %s is not let go: it carries %s, whose delivery was stopped", walk, + strings.Join(stopped, ", ")) + continue + } if h.asking[walk] { continue // its word is being asked } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go index a491df78..cc42cb60 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go @@ -96,8 +96,12 @@ type fakeController struct { stops []string // withouts are the stops that left merges out (novox/hq ADR 0299): " without ,…". withouts []string - plans int - down bool + // refuseWithout refuses every stop with merges to leave out, as a controller before ADR 0299 does; mergedAt are + // the merges' times, by commit, for what a later merge contains. + refuseWithout bool + mergedAt map[string]time.Time + plans int + down bool // onCheck runs as a check is asked, before it is answered: what the forge held at that moment. onCheck func() // planned is what Plan answers instead of "builds app", when set. @@ -105,7 +109,7 @@ type fakeController struct { } func newFakeController() *fakeController { - return &fakeController{walks: map[string]Walk{}, held: true} + return &fakeController{walks: map[string]Walk{}, held: true, mergedAt: map[string]time.Time{}} } func (c *fakeController) Plan(repository, base, head string, paths, dirs, removed []string) (*DeliveryPlan, error) { @@ -179,7 +183,10 @@ func (c *fakeController) Stop(walk, why, by string) error { return errors.New("down") } c.stops = append(c.stops, walk) - if w, known := c.walks[walk]; known { + if w, known := c.walks[walk]; known && !w.Open() { + // As the controller: only a walk that is building or rolling is stopped (`delivery-stop`). + return fmt.Errorf("%s is already %s", walk, w.State) + } else if known { w.State = walkFailed if w.Delivery == nil { w.Delivery = &WalkDelivery{} @@ -192,8 +199,10 @@ func (c *fakeController) Stop(walk, why, by string) error { } // StopWithout ends a walk without the merges named, as the controller does (novox/hq ADR 0299): each merge named, -// and each merge of its repository whose commit the walk carried for it, is left out; the others are walked again -// by a walk of their own, "-again", waiting for its word. +// and each later merge of its repository by merge time (mergedAt), is left out; the others are put in a batch, +// "-again", assembling until a test cuts it. Given a batch, the merges left out go to a stopped record, +// "-left", and the batch goes on with the others. refuseWithout refuses every such stop, as an older +// controller does. func (c *fakeController) StopWithout(walk, why, by string, without []string) (StopAnswer, error) { c.mu.Lock() defer c.mu.Unlock() @@ -201,46 +210,64 @@ func (c *fakeController) StopWithout(walk, why, by string, without []string) (St return StopAnswer{}, errors.New("down") } c.withouts = append(c.withouts, walk+" without "+strings.Join(without, ",")) + if c.refuseWithout { + return StopAnswer{}, errors.New(`delivery-stop takes no argument "without"`) + } w, known := c.walks[walk] - if !known || w.State == walkFailed || w.State == walkDone { - return StopAnswer{}, fmt.Errorf("%s is no open walk", walk) + if !known || !(w.Open() || w.IsBatch()) { + return StopAnswer{}, fmt.Errorf("%s is no open walk or batch", walk) } wd := WalkDelivery{} if w.Delivery != nil { wd = *w.Delivery } named := func(m WalkMerge) bool { return containsString(without, m.Repository+"@"+m.Commit) } - a := StopAnswer{Stopped: walk} - var again []WalkMerge - for _, m := range wd.Merges { - if named(m) { - a.LeftOut = append(a.LeftOut, WalkLeftOut{Repository: m.Repository, Commit: m.Commit}) - } - } - for _, m := range wd.Merges { - if named(m) { - continue - } - contains := "" + contains := func(m WalkMerge) string { for _, s := range wd.Merges { - if named(s) && s.Repository == m.Repository && s.Carried == m.Commit { - contains = s.Commit + if named(s) && s.Repository == m.Repository && c.mergedAt[m.Commit].After(c.mergedAt[s.Commit]) { + return s.Commit } } - if contains != "" { - a.LeftOut = append(a.LeftOut, WalkLeftOut{Repository: m.Repository, Commit: m.Commit, Contains: contains}) - continue + return "" + } + a := StopAnswer{Stopped: walk} + var again, left []WalkMerge + for _, m := range wd.Merges { + switch { + case named(m): + a.LeftOut = append(a.LeftOut, WalkLeftOut{Repository: m.Repository, Commit: m.Commit}) + left = append(left, m) + case contains(m) != "": + a.LeftOut = append(a.LeftOut, WalkLeftOut{Repository: m.Repository, Commit: m.Commit, Contains: contains(m)}) + left = append(left, m) + default: + m.Carried = "" + again = append(again, m) + a.WalkedAgain = append(a.WalkedAgain, m.Repository+"@"+m.Commit) } - m.Carried = "" - again = append(again, m) - a.WalkedAgain = append(a.WalkedAgain, m.Repository+"@"+m.Commit) + } + if len(a.LeftOut) == 0 { + return StopAnswer{}, fmt.Errorf("%s answers no merge %s", walk, strings.Join(without, ", ")) + } + if w.IsBatch() { + rid := walk + "-left" + c.walks[rid] = Walk{ID: rid, Repository: left[0].Repository, Branch: "main", Commit: left[0].Commit, + Created: w.Created, State: walkFailed, Revision: 1, Modules: map[string]*WalkModule{}, + Delivery: &WalkDelivery{Stopped: by, StoppedWhy: why, Merges: left, LeftOut: a.LeftOut, WalkedAgainBy: walk}} + wd.Merges = again + w.Delivery = &wd + w.Repository, w.Commit = again[0].Repository, again[0].Commit + w.Revision++ + c.walks[walk] = w + a.Stopped, a.WalkedAgainBy = rid, walk + return a, nil } if len(again) > 0 { id := walk + "-again" + closes := w.Created.Add(time.Minute) c.walks[id] = Walk{ID: id, Repository: again[0].Repository, Branch: "main", Commit: again[0].Commit, - Created: w.Created.Add(time.Minute), State: walkBuilding, Tiers: [][]string{{"app"}}, - Modules: map[string]*WalkModule{"app": {}}, Revision: 1, - Delivery: &WalkDelivery{Awaits: "mesh-delivery", Merges: again}} + Created: w.Created.Add(time.Minute), State: walkAssembling, Revision: 1, + Modules: map[string]*WalkModule{}, Delivery: &WalkDelivery{Merges: again, Batch: &WalkBatch{ClosesAt: &closes}}} wd.WalkedAgainBy, a.WalkedAgainBy = id, id } wd.Stopped, wd.StoppedWhy, wd.LeftOut = by, why, a.LeftOut @@ -251,6 +278,19 @@ func (c *fakeController) StopWithout(walk, why, by string, without []string) (St c.walks[walk] = w return a, nil } + +// cut makes a batch the walk under the same id, waiting for its word, as the controller cuts it. +func (c *fakeController) cut(id string) Walk { + c.mu.Lock() + w := c.walks[id] + c.mu.Unlock() + wd := *w.Delivery + wd.Batch, wd.Awaits = nil, "mesh-delivery" + w.Delivery = &wd + w.State, w.Tiers, w.Modules = walkBuilding, [][]string{{"app"}}, map[string]*WalkModule{"app": {}} + return c.put(w) +} + func (c *fakeController) Walks(walk string) (bool, []Walk, error) { c.mu.Lock() defer c.mu.Unlock() diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index d66cdac6..841a5c10 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -59,6 +59,8 @@ type Holder struct { asking map[string]bool // leaving are, by walk, the merges a stop gathered to leave out of it, asked once (novox/hq ADR 0299). leaving map[string][]string + // leaveAsked is when a walk was last asked to leave out the stopped merges it carries (leaveStopped). + leaveAsked map[string]leaveAsk } // unmatched is a walk no delivery is known for yet: its merge not heard, or merged before this owner held. @@ -832,6 +834,9 @@ func (h *Holder) Tick() { for _, ask := range h.plan() { ask() } + for _, ask := range h.leaveStopped() { + ask() + } } // plan works out, under the lock, what to ask; each ask applies its own answer. @@ -1835,10 +1840,19 @@ func (h *Holder) stopInGroup(d *Delivery, why string) func() { return nil } h.moved(d, []Transition{t}) - if d.Walk == nil || d.Walk.State == walkDone || d.Walk.State == walkFailed || d.Walk.State == walkSuperseded { + // Its open walk; or, its walk stopped for another merge, the batch or walk that walks its merge again (review + // of #221): a merge walked again and then stopped by its group is left out there too. + var walk string + switch { + case d.Walk == nil: + return nil + case d.Walk.State == walkBuilding || d.Walk.State == walkRolling: + walk = d.Walk.ID + case d.walkedAgain(): + walk = d.Walk.WalkedAgainBy + default: return nil } - walk := d.Walk.ID if !h.leave(walk, d) { return nil // the first member left out of this walk asks for every one } @@ -1850,15 +1864,114 @@ func (h *Holder) stopInGroup(d *Delivery, why string) func() { } return } - a, err := h.Controller.StopWithout(walk, why, byOwner, without) - if err != nil { - h.Logf("[mesh-delivery] the walk %s could not be stopped without %s: %v", walk, strings.Join(without, ", "), err) - return - } - h.Logf("[mesh-delivery] the walk %s is stopped without %s; %s", walk, strings.Join(without, ", "), againWords(a)) + h.Logf("[mesh-delivery] %s", h.stopLeaving(walk, why, byOwner, without)) } } +// stopLeaving asks the controller to stop a walk or batch without merges (ADR 0299) and says what came of it. A +// refusal — an older controller takes no `without`, or the leave-out could not be made — falls back to stopping the +// whole walk: the stopped change is held back, and every delivery on it ends stopped with the reason (review of +// #221). Outside the lock. +func (h *Holder) stopLeaving(walk, why, by string, without []string) string { + h.mu.Lock() + h.askedToLeave(walk) + h.mu.Unlock() + a, err := h.Controller.StopWithout(walk, why, by, without) + if err == nil { + return fmt.Sprintf("the walk %s is stopped without %s; %s", walk, strings.Join(without, ", "), againWords(a)) + } + refused := err + if err := h.Controller.Stop(walk, fmt.Sprintf("%s, and it could not be left out alone (%v)", why, refused), by); err != nil { + return fmt.Sprintf("the walk %s could not be stopped without %s (%v), nor whole (%v): `plans stop %s --why …` "+ + "ends it by hand", walk, strings.Join(without, ", "), refused, err, walk) + } + return fmt.Sprintf("the walk %s could not be stopped without %s (%v), so it was stopped whole: every delivery on it "+ + "ends stopped", walk, strings.Join(without, ", "), refused) +} + +// carriesStopped are the merges a walk or batch carries whose deliveries were stopped, as `@`, +// and why the first was: a record that names a stopped delivery's merge and has not left it out would deliver a +// stopped change (review of #221). Under the lock. +func (h *Holder) carriesStopped(walk string) ([]string, string) { + s, known := h.walks[walk] + if !known || !(s.walk.Open() || s.walk.IsBatch()) || s.walk.Delivery == nil || len(s.walk.Delivery.Merges) == 0 { + return nil, "" + } + var out []string + why := "" + ids := make([]string, 0, len(h.deliveries)) + for id := range h.deliveries { + ids = append(ids, id) + } + slices.Sort(ids) + for _, id := range ids { + d := h.deliveries[id] + if d.State != Stopped || d.MergedAs == "" { + continue + } + // By the merges it names alone: its own commit may be another's merge it no longer answers. + if !slices.ContainsFunc(s.walk.Delivery.Merges, func(m WalkMerge) bool { + return strings.EqualFold(m.Repository, d.Repository) && sameCommit(m.Commit, d.MergedAs) + }) { + continue + } + if _, out := s.walk.leftOut(d.Repository, d.MergedAs); out { + continue + } + out = append(out, d.Repository+"@"+d.MergedAs) + if why == "" { + why = fmt.Sprintf("%s was stopped: %s", d.ID, lastWhyOf(d)) + } + } + return out, why +} + +// lastWhyOf is the reason of a delivery's last transition. +func lastWhyOf(d *Delivery) string { + if len(d.Transitions) == 0 { + return "" + } + return d.Transitions[len(d.Transitions)-1].Why +} + +// leaveAsk is when a walk was last asked to leave merges out, and at which revision of its record. +type leaveAsk struct { + at time.Time + revision int64 +} + +// askedToLeave notes an ask to leave merges out of a walk, at its record's revision: under the lock. +func (h *Holder) askedToLeave(walk string) { + if h.leaveAsked == nil { + h.leaveAsked = map[string]leaveAsk{} + } + h.leaveAsked[walk] = leaveAsk{at: h.Now(), revision: h.walks[walk].walk.Revision} +} + +// leaveStopped asks, for every open walk or batch that carries a stopped delivery's merge, that the merge be left +// out (ADR 0299, review of #221): a merge walked again and then stopped, or stopped while its batch assembled, never +// ships. Asked once per record of it while it is carried; such a walk is never given its word (words). +func (h *Holder) leaveStopped() []func() { + h.mu.Lock() + defer h.mu.Unlock() + now := h.Now() + var asks []func() + for id, seen := range h.walks { + without, why := h.carriesStopped(id) + if len(without) == 0 { + continue + } + // Once per record of it: the record that answers an ask is newer. Asked again after five minutes, should + // the ask have gone nowhere. + if a, asked := h.leaveAsked[id]; asked && seen.walk.Revision <= a.revision && now.Sub(a.at) < 5*time.Minute { + continue + } + walk := id + asks = append(asks, func() { h.Logf("[mesh-delivery] %s", h.stopLeaving(walk, why, byOwner, without)) }) + } + return asks +} + // leave gathers a delivery's merge to leave out of a walk (ADR 0299), for a caller holding the lock; true for the // first of that walk, whose ask asks for all. func (h *Holder) leave(walk string, d *Delivery) bool { @@ -1913,7 +2026,9 @@ func (h *Holder) othersOn(walk string, without []string) []string { return keys } -// againWords is what a stop that left merges out did, in a person's words. +// againWords is what a stop that left merges out did, in a person's words: the walk that walks the others again, +// what it left out (the controller's word on what contains a stopped merge), what stays on the machines, and what +// will still deliver a stopped change (ADR 0299). func againWords(a StopAnswer) string { var left []string for _, o := range a.LeftOut { @@ -1923,14 +2038,31 @@ func againWords(a StopAnswer) string { } left = append(left, s) } + var out string switch { case a.WalkedAgainBy != "": - return fmt.Sprintf("%s walks its other merges again: %s; left out: %s", a.WalkedAgainBy, + out = fmt.Sprintf("%s walks its other merges again: %s; left out: %s", a.WalkedAgainBy, strings.Join(a.WalkedAgain, ", "), orNone(strings.Join(left, ", "))) case len(left) > 0: - return "left out: " + strings.Join(left, ", ") + "; nothing else to walk" + out = "left out: " + strings.Join(left, ", ") + "; nothing else to walk" + default: + out = "nothing else to walk" } - return "the controller ended it whole: it named no walk that walks its other merges again" + if len(a.Withheld) > 0 { + out += "; its walk withholds " + strings.Join(a.Withheld, ", ") + } + if len(a.Kept) > 0 { + var kept []string + for _, k := range a.Kept { + kept = append(kept, k.Module+" on "+strings.Join(k.Machines, ", ")) + } + out += "; what the stopped walk already sent stays on the machines: " + strings.Join(kept, "; ") + } + if len(a.StillCarriedBy) > 0 { + out += "; still to deliver the stopped change when walked, a revert's to hold back: " + + strings.Join(a.StillCarriedBy, ", ") + } + return out } // GroupState is a group's state, derived from its members and its composed check — never set apart (novox/hq diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go index 98dc5e62..e70b3671 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue459_test.go @@ -77,6 +77,12 @@ func (s *shape459) heard(walks ...string) { s.settleAll() } +// cutAgain is the controller cutting the batch that walks a stopped walk's merges again, as plan-moved says it. +func (s *shape459) cutAgain(id string) { + s.h.WalkMoved(s.ctl.cut(id)) + s.settleAll() +} + func (s *shape459) done(walk string) { r := s.ctl.walk(walk) sent := s.now @@ -113,6 +119,10 @@ func TestAGroupStopWalksTheOtherDeliveriesAgainWithoutIt(t *testing.T) { t.Fatalf("the controller was asked %v; want %q", s.ctl.withouts, want) } s.heard("plan-2", "plan-2-again") + if containsString(s.ctl.delivers, "plan-2-again") { + t.Fatal("a batch not yet cut was given its word") + } + s.cutAgain("plan-2-again") z := s.delivery(s.ids["novox/z"]) if z.State.Final() || z.Walk == nil || z.Walk.ID != "plan-2-again" || !containsString(s.ctl.delivers, "plan-2-again") { t.Fatalf("z is %s on %+v, let go %v; want it following plan-2-again, let go", z.State, z.Walk, s.ctl.delivers) @@ -152,6 +162,7 @@ func TestAGroupStopEndsAMergeThatContainsTheStoppedOne(t *testing.T) { s.retarget("novox/b2", "novox/b") s.failFirst() s.heard("plan-2", "plan-2-again") + s.cutAgain("plan-2-again") b2 := s.delivery(s.ids["novox/b2"]) if b2.State != Stopped || !strings.Contains(lastWhy(b2), shortOf(s.merges["novox/b"], 8)) || !strings.Contains(lastWhy(b2), "contains") { @@ -170,6 +181,9 @@ func (s *shape459) retarget(later, of string) { d := s.h.deliveries[s.ids[later]] d.Repository, d.MergedAt = of, d.MergedAt.Add(time.Minute) s.h.mu.Unlock() + s.ctl.mu.Lock() + s.ctl.mergedAt[s.merges[of]], s.ctl.mergedAt[s.merges[later]] = d.MergedAt.Add(-time.Minute), d.MergedAt + s.ctl.mu.Unlock() two := s.ctl.walk("plan-2") merges := []WalkMerge{} for _, m := range two.Delivery.Merges { @@ -240,6 +254,7 @@ func TestAPersonsStopAgainWalksTheOthersAgain(t *testing.T) { t.Fatalf("z is %s once plan-2 was stopped and walked again; want published, waiting for that walk", z.State) } s.heard("plan-2-again") + s.cutAgain("plan-2-again") if z := s.delivery(s.ids["novox/z"]); z.Walk == nil || z.Walk.ID != "plan-2-again" || z.State != Delivering { t.Fatalf("z is %s on %+v; want it delivering on plan-2-again", z.State, z.Walk) } @@ -263,3 +278,106 @@ func TestAPersonsStopOfTheWalkStopsEveryDelivery(t *testing.T) { t.Fatalf("z is %s (%q); want it stopped, naming y's stop", z.State, lastWhy(z)) } } + +// The reviewer's case (review of #221): group [a, b]; y and z ride plan-2 with b. y is stopped `way: again`, and the +// batch that walks plan-2's others again carries b and z. Then a fails: b is stopped by its group, and its merge is +// left out of that batch — never shipped by it — and z alone is walked and delivered. +func TestAMergeWalkedAgainAndThenStoppedByItsGroupNeverShips(t *testing.T) { + s := open459(t, []string{"novox/a", "novox/b"}, []string{"novox/y", "novox/z"}) + if _, err := s.h.StopWay(s.ids["novox/y"], "not wanted after all", "jochen", "again"); err != nil { + t.Fatal(err) + } + s.heard("plan-2", "plan-2-again") + s.failFirst() + if b := s.state(s.ids["novox/b"]); b != Stopped { + t.Fatalf("b is %s", b) + } + if want := "plan-2-again without novox/b@" + s.merges["novox/b"]; !containsString(s.ctl.withouts, want) { + t.Fatalf("the controller was asked %v; want %q", s.ctl.withouts, want) + } + s.heard("plan-2-again", "plan-2-again-left") + s.cutAgain("plan-2-again") + again := s.ctl.walk("plan-2-again") + for _, m := range again.Delivery.Merges { + if m.Repository == "novox/b" { + t.Fatalf("the walk %s still carries b's stopped merge: %+v", again.ID, again.Delivery.Merges) + } + } + if !containsString(s.ctl.delivers, "plan-2-again") { + t.Fatalf("z's walk was not let go: %v", s.ctl.delivers) + } + s.done("plan-2-again") + if z := s.state(s.ids["novox/z"]); z != Delivered { + t.Fatalf("z is %s", z) + } +} + +// Whatever brought it there, a walk that carries a stopped delivery's merge is asked to leave it out, and is never +// given its word while it carries it. +func TestAWalkCarryingAStoppedMergeIsAskedToLeaveItOutAndNeverLetGo(t *testing.T) { + s := shared459(t) + if _, err := s.h.StopWay(s.ids["novox/y"], "not wanted after all", "jochen", "again"); err != nil { + t.Fatal(err) + } + // A walk the controller made that carries y's merge anyway, waiting for its word, with z on it. + x := aWalk("plan-x", "novox/z", s.merges["novox/z"], true) + x.Created = x.Created.Add(10 * time.Minute) + x.Delivery.Merges = []WalkMerge{{Repository: "novox/y", Commit: s.merges["novox/y"]}, + {Repository: "novox/z", Commit: s.merges["novox/z"]}} + s.ctl.refuseWithout = true // it cannot be left out: the walk is stopped whole + s.h.WalkMoved(s.ctl.put(x)) + s.settleAll() + if want := "plan-x without novox/y@" + s.merges["novox/y"]; !containsString(s.ctl.withouts, want) { + t.Fatalf("the controller was asked %v; want %q", s.ctl.withouts, want) + } + if containsString(s.ctl.delivers, "plan-x") { + t.Fatalf("a walk carrying a stopped merge was let go: %v", s.ctl.delivers) + } + if !containsString(s.ctl.stops, "plan-x") { + t.Fatalf("refused the leave-out, the walk was not stopped whole: %v", s.ctl.stops) + } +} + +// A group stop the controller refuses to make without the merge (an older controller) stops the whole walk: the +// stopped change is held back, and the delivery riding with it ends stopped, saying why. +func TestARefusedLeaveOutStopsTheWholeWalk(t *testing.T) { + s := open459(t, []string{"novox/a", "novox/b"}, []string{"novox/z"}) + s.ctl.refuseWithout = true + s.failFirst() + if len(s.ctl.withouts) != 1 || !containsString(s.ctl.stops, "plan-2") { + t.Fatalf("asked %v, ended whole %v; want the leave-out tried, then plan-2 ended whole", s.ctl.withouts, s.ctl.stops) + } + s.heard("plan-2") + if z := s.delivery(s.ids["novox/z"]); z.State != Stopped || !strings.Contains(lastWhy(z), "could not be left out") { + t.Fatalf("z is %s (%q); want it stopped, saying the leave-out was refused", z.State, lastWhy(z)) + } +} + +// A person's stop the controller cannot make leaves the delivery as it was, and says how to go on. +func TestAPersonsStopThatCannotBeMadeStopsNothing(t *testing.T) { + s := shared459(t) + s.ctl.refuseWithout = true + _, err := s.h.StopWay(s.ids["novox/y"], "not wanted after all", "jochen", "again") + if err == nil || !strings.Contains(err.Error(), "is not stopped") || !strings.Contains(err.Error(), "plans stop") { + t.Fatalf("the refused stop said %v", err) + } + if st := s.state(s.ids["novox/y"]); st != Delivering { + t.Fatalf("y is %s; want it delivering, its stop not made", st) + } +} + +// What a stop that left merges out says: the walk that walks the others, what it left out and what each contains, +// as the controller says it, what stays on which machines, and what will still deliver the stopped change. +func TestAStopSaysWhatStaysAndWhatStillCarriesIt(t *testing.T) { + said := againWords(StopAnswer{Stopped: "plan-2", WalkedAgainBy: "plan-3", WalkedAgain: []string{"novox/z@zz"}, + LeftOut: []WalkLeftOut{{Repository: "novox/b", Commit: "b1b1b1b1b1b1"}, + {Repository: "novox/b", Commit: "b2b2b2b2b2b2", Contains: "b1b1b1b1b1b1"}}, + Kept: []WalkKept{{Module: "lab", Machines: []string{"anchor"}}}, + StillCarriedBy: []string{"novox/b@b3b3 (in plan-4)"}}) + for _, want := range []string{"plan-3", "novox/z@zz", "b2b2b2b2b2b2 (contains b1b1b1b1b1b1)", "lab on anchor", + "stays on the machines", "novox/b@b3b3 (in plan-4)", "revert"} { + if !strings.Contains(said, want) { + t.Fatalf("the stop said %q; want %q in it", said, want) + } + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ports.go b/modules/mesh-delivery/cmd/mesh-delivery/ports.go index 072220ce..8675075e 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ports.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ports.go @@ -280,6 +280,18 @@ type StopAnswer struct { WalkedAgainBy string `json:"walked_again_by,omitempty"` WalkedAgain []string `json:"walked_again,omitempty"` LeftOut []WalkLeftOut `json:"left_out,omitempty"` + // Kept are the modules the stopped walk sent that nothing walks again, on the machines named; Withheld the + // modules the walk that walks the others withholds; StillCarriedBy the merges elsewhere that will deliver a + // stopped change when walked — a later merge of its repository, or one moving its modules (ADR 0299). + Kept []WalkKept `json:"kept,omitempty"` + Withheld []string `json:"withheld,omitempty"` + StillCarriedBy []string `json:"still_carried_by,omitempty"` +} + +// WalkKept is a module a stopped walk left running its build, and where. +type WalkKept struct { + Module string `json:"module"` + Machines []string `json:"machines"` } func (c seatController) StopWithout(walk, why, by string, without []string) (StopAnswer, error) { @@ -290,9 +302,8 @@ func (c seatController) StopWithout(walk, why, by string, without []string) (Sto } var a StopAnswer if json.Unmarshal(raw, &a) != nil || a.Stopped == "" { - // A controller from before ADR 0299 ends the walk whole and says it as text: its record names no walk - // that walks the others again, and every delivery on it ends stopped, as it reads. - return StopAnswer{Stopped: walk}, nil + return StopAnswer{}, fmt.Errorf("the controller's delivery-stop did not say what it left out: %s", + lastLine(string(raw))) } return a, nil } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go index 14fe3209..cb389a14 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go @@ -326,10 +326,19 @@ func (h *Holder) Release(id, why, by string) (string, error) { if d.Walk != nil && d.Walk.Waits { walk = d.Walk.ID } + var stopped []string + if walk != "" { + stopped, _ = h.carriesStopped(walk) + } h.mu.Unlock() if walk == "" { return fmt.Sprintf("%s is delivering, on %s's word", id, by), nil } + if len(stopped) > 0 { + // Never the word for a walk that carries a stopped change (ADR 0299): asked to leave it out first. + return "", fmt.Errorf("%s is delivering on %s's word, and its walk %s is not let go yet: it carries %s, whose "+ + "delivery was stopped, and is asked to leave it out first", id, by, walk, strings.Join(stopped, ", ")) + } if err := h.Controller.Deliver(walk, "released by "+by+": "+why); err != nil { return "", fmt.Errorf("%s is delivering on %s's word, and its walk %s could not be let go yet (%v): the next "+ "tick asks again", id, by, walk, err) @@ -373,30 +382,66 @@ func (h *Holder) StopWay(id, why, by, way string) (string, error) { h.mu.Unlock() return "", fmt.Errorf("no delivery %q", id) } + // Its guard is read on a copy: the delivery is stopped only once its walk is (review of #221). + trial := *d + if _, err := Apply(&trial, EvStop, true, Facts{Now: h.Now(), Why: why, By: by}); err != nil { + h.mu.Unlock() + return "", err + } var walk string if d.Walk != nil && (d.Walk.State == walkBuilding || d.Walk.State == walkRolling) { walk = d.Walk.ID } + merge := d.Repository + "@" + d.MergedAs var others []string - if walk != "" && !d.State.Final() { - others = h.othersOn(walk, []string{d.Repository + "@" + d.MergedAs}) + if walk != "" { + others = h.othersOn(walk, []string{merge}) } if len(others) > 0 && way == "" { said := h.takenDown(d, walk, others) h.mu.Unlock() return "", errors.New(said) } + h.mu.Unlock() + + var did string + switch { + case walk == "": + case len(others) == 0 || way == wayWalk: + whole := why + if len(others) > 0 { + whole = fmt.Sprintf("%s was stopped, and its whole walk with it: %s", id, why) + } + if err := h.Controller.Stop(walk, whole, by); err != nil { + return "", fmt.Errorf("%s is not stopped: its walk %s could not be ended yet: %v — `plans stop %s --why …` "+ + "ends it by hand", id, walk, err, walk) + } + did = fmt.Sprintf("; its walk %s is ended", walk) + if len(others) > 0 { + did = fmt.Sprintf("; its walk %s is ended whole, and every delivery on it with it: %s", walk, + strings.Join(others, ", ")) + } + default: + a, err := h.Controller.StopWithout(walk, why, by, []string{merge}) + if err != nil { + return "", fmt.Errorf("%s is not stopped: its walk %s could not be ended without it: %v — `way: %s` stops "+ + "the whole walk, and `plans stop %s --why …` ends it by hand", id, walk, err, wayWalk, walk) + } + did = fmt.Sprintf("; its walk %s is ended without it, and %s", walk, againWords(a)) + } + + h.mu.Lock() + d = h.deliveries[id] + if d == nil || d.State.Final() { + h.mu.Unlock() + return fmt.Sprintf("%s ended meanwhile%s", id, did), nil + } t, err := Apply(d, EvStop, true, Facts{Now: h.Now(), Why: why, By: by}) if err != nil { h.mu.Unlock() return "", err } h.moved(d, []Transition{t}) - if walk != "" { - // Gathered before the group's turns, so a member after it on the same walk adds to this ask, never asks one - // of its own that would walk this merge again. - h.leave(walk, d) - } var asks []func() if g := h.groups[d.Group]; g != nil && g.Closed { asks = h.groupTurns(g, h.membersOf(g)) @@ -407,31 +452,7 @@ func (h *Holder) StopWay(id, why, by, way string) (string, error) { for _, a := range asks { a() } - if walk == "" { - return fmt.Sprintf("%s is stopped, on %s's word", id, by), nil - } - without, shared := h.leaving459(walk) - if !shared || way == wayWalk { - whole := why - if shared { - whole = fmt.Sprintf("%s was stopped, and its whole walk with it: %s", id, why) - } - if err := h.Controller.Stop(walk, whole, by); err != nil { - return "", fmt.Errorf("%s is stopped, and its walk %s could not be ended yet: %v — `plans stop %s --why …` "+ - "ends it by hand", id, walk, err, walk) - } - if shared { - return fmt.Sprintf("%s is stopped, on %s's word; its walk %s is ended whole, and every delivery on it with "+ - "it: %s", id, by, walk, strings.Join(others, ", ")), nil - } - return fmt.Sprintf("%s is stopped, on %s's word; its walk %s is ended", id, by, walk), nil - } - a, err := h.Controller.StopWithout(walk, why, by, without) - if err != nil { - return "", fmt.Errorf("%s is stopped, and its walk %s could not be ended without it yet: %v", id, walk, err) - } - return fmt.Sprintf("%s is stopped, on %s's word; its walk %s is ended without it, and %s", id, by, walk, - againWords(a)), nil + return fmt.Sprintf("%s is stopped, on %s's word%s", id, by, did), nil } // takenDown is what ending a delivery's shared walk would take down, and the two ways: for a caller holding the @@ -451,7 +472,8 @@ func (h *Holder) takenDown(d *Delivery, walk string, others []string) string { if len(contain) > 0 { s += fmt.Sprintf(" These contain its merge and end with it either way: %s.", strings.Join(contain, ", ")) } - return s + fmt.Sprintf(" Choose a way: `way: %s` ends the walk and walks the others again without this merge; "+ + return s + fmt.Sprintf(" What the walk already sent stays where it was sent; the stop's answer names it."+ + " Choose a way: `way: %s` ends the walk and walks the others again without this merge; "+ "`way: %s` stops the whole walk and every delivery on it.", wayAgain, wayWalk) }