From f1775281e27bb62b3e2445daf18e26c69cf5d40a Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Mon, 19 Oct 2026 21:45:48 +0000 Subject: [PATCH] Refuse a retry of a failed plan that a newer done plan of its branch has passed (issue 1328, ADR 0451) --- cmd/mesh-controller/plan_failed.go | 13 +++++- cmd/mesh-controller/plan_failed_test.go | 61 +++++++++++++++++++++++++ cmd/mesh-controller/plan_retry.go | 30 ++++++++++++ cmd/mesh-controller/queue_test.go | 9 ++-- 4 files changed, 108 insertions(+), 5 deletions(-) diff --git a/cmd/mesh-controller/plan_failed.go b/cmd/mesh-controller/plan_failed.go index bd82b38c..2e3011ec 100644 --- a/cmd/mesh-controller/plan_failed.go +++ b/cmd/mesh-controller/plan_failed.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "regexp" "sort" @@ -30,6 +31,9 @@ type failedPlanFacts struct { since time.Time // refusal is why `plans retry` would not run it, empty when it would. refusal string + // passed is set when a newer plan of its repository and branch is done and the retry is refused for that + // (novox/hq ADR 0451): the condition asks nothing. + passed bool } // machines are the machines the failed modules run on, sorted, each once. @@ -135,11 +139,14 @@ func planFailedWords(p failedPlanFacts) (headline, explanation, needs string, ac } else { explanation = fmt.Sprintf("The delivery of %s stopped before it finished.", what) } - if p.refusal == "" { + switch { + case p.passed: + explanation += " A newer delivery has passed it, so a retry would send older code. Nothing is needed from you." + case p.refusal == "": explanation += " Nothing is tried again until you say so." needs = "try the failed builds again " + FromMeshMCPServer actions = []conditions.Action{retryAction(p.id)} - } else { + default: explanation += " " + conditions.Capital(plainRefusal(p.refusal)) + "." needs = "look at why it failed; a retry would not run now." } @@ -213,6 +220,8 @@ func failedFactsOf(p inventory.Plan, plans []inventory.Plan, onPath map[string]s } if err := retryRefusal(p, plans, onPath); err != nil { f.refusal = err.Error() + var passed passedBy + f.passed = errors.As(err, &passed) } return f } diff --git a/cmd/mesh-controller/plan_failed_test.go b/cmd/mesh-controller/plan_failed_test.go index b26542b9..535ec2e7 100644 --- a/cmd/mesh-controller/plan_failed_test.go +++ b/cmd/mesh-controller/plan_failed_test.go @@ -190,3 +190,64 @@ func TestNoClockRetriesAFailedPlan(t *testing.T) { t.Fatalf("a plan failed beyond the window is still raised: %+v", got) } } + +// A later plan of the same repository and branch, in a state, made after the failed one. +func laterPlan(id, branch, state string, after inventory.Plan) inventory.Plan { + q := aFailedPlan(id, func(p *inventory.Plan) { + p.State, p.Branch, p.Created = state, branch, after.Created.Add(time.Hour) + }) + return q +} + +// 1328/AC1: a later done plan of the same repository and branch makes `plans retry` refuse, naming the later plan. +func TestARetryIsRefusedWhenALaterPlanOfTheBranchIsDone(t *testing.T) { + failed := aFailedPlan("plan-1", func(p *inventory.Plan) { p.Branch, p.Created = "main", time.Date(2026, 10, 19, 12, 0, 0, 0, time.UTC) }) + later := laterPlan("plan-2", "main", inventory.PlanDone, failed) + err := retryRefusal(failed, []inventory.Plan{failed, later}, map[string]string{}) + if err == nil || !strings.Contains(err.Error(), "plan-2") || !strings.Contains(err.Error(), "passed") { + t.Fatalf("the retry was not refused, naming the later plan: %v", err) + } +} + +// 1328/AC2: with the later plan still open the refusal is today's; a later done plan of another branch refuses nothing. +func TestARetryIsRefusedAsBeforeWhileTheLaterPlanIsOpenAndNotForAnotherBranch(t *testing.T) { + failed := aFailedPlan("plan-1", func(p *inventory.Plan) { p.Branch, p.Created = "main", time.Date(2026, 10, 19, 12, 0, 0, 0, time.UTC) }) + open := laterPlan("plan-2", "main", inventory.PlanBuilding, failed) + err := retryRefusal(failed, []inventory.Plan{failed, open}, map[string]string{}) + if err == nil || !strings.Contains(err.Error(), "supersedes it") { + t.Fatalf("with the later plan open the refusal is not today's: %v", err) + } + other := laterPlan("plan-3", "feature/x", inventory.PlanDone, failed) + if err := retryRefusal(failed, []inventory.Plan{failed, other}, map[string]string{}); err != nil { + t.Fatalf("a later done plan of another branch refused the retry: %v", err) + } + earlier := laterPlan("plan-4", "main", inventory.PlanDone, failed) + earlier.Created = failed.Created.Add(-time.Hour) + if err := retryRefusal(failed, []inventory.Plan{failed, earlier}, map[string]string{}); err != nil { + t.Fatalf("an earlier done plan refused the retry: %v", err) + } +} + +// 1328/AC3: such a plan's plan-failed condition has no action and says it was passed by a newer delivery. +func TestAPassedFailedPlanHasNoActionAndSaysItWasPassed(t *testing.T) { + now := time.Date(2026, 10, 19, 18, 30, 0, 0, time.UTC) + failed := aFailedPlan("plan-1", func(p *inventory.Plan) { p.Branch, p.Created = "main", now.Add(-5*time.Hour) }) + later := laterPlan("plan-2", "main", inventory.PlanDone, failed) + f := calm(now) + f.failed = []failedPlanFacts{failedFactsFor(failed, later)} + got := watchPlansFailed(f) + if len(got) != 1 || len(got[0].Actions) != 0 { + t.Fatalf("%+v", got) + } + o := got[0] + if !strings.Contains(o.Explanation, "newer delivery") || !strings.Contains(o.Explanation, "passed") { + t.Errorf("the words do not say it was passed by a newer delivery: %q", o.Explanation) + } + if strings.Contains(o.Explanation, "details say why") { + t.Errorf("the generic fallback was said: %q", o.Explanation) + } + words := conditions.Words{Headline: o.Headline, Explanation: o.Explanation, Resolved: o.Resolved, Needs: o.Needs} + if why, ok := conditions.PlainWords(words, "g14", "shanks"); !ok { + t.Errorf("not plain: %s\n%+v", why, words) + } +} diff --git a/cmd/mesh-controller/plan_retry.go b/cmd/mesh-controller/plan_retry.go index aa9adf7b..e3b79760 100644 --- a/cmd/mesh-controller/plan_retry.go +++ b/cmd/mesh-controller/plan_retry.go @@ -154,6 +154,31 @@ func newerOpenPlan(p inventory.Plan, plans []inventory.Plan) (inventory.Plan, bo return inventory.Plan{}, false } +// newerDonePlan is a done plan of the same repository and branch made after this one: the newer commit it delivered +// runs now (novox/hq ADR 0451). +func newerDonePlan(p inventory.Plan, plans []inventory.Plan) (inventory.Plan, bool) { + var found inventory.Plan + var ok bool + for _, q := range plans { + if q.ID == p.ID || q.State != inventory.PlanDone || !strings.EqualFold(q.Repository, p.Repository) || + (p.Branch != "" && q.Branch != "" && p.Branch != q.Branch) || !q.Created.After(p.Created) { + continue + } + if !ok || q.Created.After(found.Created) { + found, ok = q, true + } + } + return found, ok +} + +// passedBy is the refusal of a retry of a plan that a newer done plan of its repository and branch has passed. +type passedBy struct{ plan, by, repository, commit string } + +func (e passedBy) Error() string { + return fmt.Sprintf("%s was passed by %s, a newer plan of %s (at %s) that is done: retrying it would build and "+ + "send the older commit over the newer one that runs", e.plan, e.by, e.repository, short(e.commit)) +} + // retryRefusal is why a plan cannot be retried, or nothing. func retryRefusal(p inventory.Plan, plans []inventory.Plan, onPath map[string]string) error { switch { @@ -165,6 +190,11 @@ func retryRefusal(p inventory.Plan, plans []inventory.Plan, onPath map[string]st return fmt.Errorf("%s is still %s; nothing in it failed to retry — `rebuild ` asks one module again", p.ID, p.State) } + // **A newer delivery of the branch has passed it** (novox/hq ADR 0451): retrying would build and send the older + // commit over the newer one that runs. + if q, found := newerDonePlan(p, plans); found { + return passedBy{plan: p.ID, by: q.ID, repository: q.Repository, commit: q.Commit} + } if len(failedIn(p)) > 0 { if q, found := newerOpenPlan(p, plans); found { return fmt.Errorf("%s supersedes it: a newer merge of %s (%s at %s) is open, and retrying %s would build "+ diff --git a/cmd/mesh-controller/queue_test.go b/cmd/mesh-controller/queue_test.go index fe1a4674..82834cec 100644 --- a/cmd/mesh-controller/queue_test.go +++ b/cmd/mesh-controller/queue_test.go @@ -79,11 +79,14 @@ func TestRetryRefusesWhatItCannotResume(t *testing.T) { if err := retryRefusal(stopped, nil, ownPathOf(nil)); err == nil || !strings.Contains(err.Error(), "nothing in tier 0") { t.Errorf("a plan with nothing failed to build was retried: %v", err) } - // A failed or done plan does not supersede it. - if err := retryRefusal(failed, []inventory.Plan{plan("plan-4", inventory.PlanFailed, at.Add(time.Hour)), - plan("plan-5", inventory.PlanDone, at.Add(time.Hour))}, ownPathOf(nil)); err != nil { + // A later failed plan does not supersede it; a later done one passes it (novox/hq ADR 0451). + if err := retryRefusal(failed, []inventory.Plan{plan("plan-4", inventory.PlanFailed, at.Add(time.Hour))}, ownPathOf(nil)); err != nil { t.Errorf("refused for a plan that does not supersede it: %v", err) } + if err := retryRefusal(failed, []inventory.Plan{plan("plan-5", inventory.PlanDone, at.Add(time.Hour))}, ownPathOf(nil)); err == nil || + !strings.Contains(err.Error(), "plan-5") { + t.Errorf("a plan a later done plan passed was retried: %v", err) + } // Another branch's open walk, or an older one, does not supersede it, and is one walk open: one at a time // (novox/hq ADR 0276). other := plan("plan-3", inventory.PlanBuilding, at.Add(time.Hour))