From add807f034b12b47b734461a24fa903908150e20 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 22:22:02 +0200 Subject: [PATCH] Say no cycle for a packages edge in a plan's last tier A packages edge orders nothing, so a module and what packages its source share a tier by rule; hasCycle counted the edge and the merge handler said "the last tier depends on itself" of plans with no cycle. Skip the kind as tiersOf does. The planner tests' cycle rows and property now pass. --- cmd/mesh-controller/planner_rules_test.go | 17 ++++++----------- cmd/mesh-controller/release_plan.go | 7 ++++++- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/cmd/mesh-controller/planner_rules_test.go b/cmd/mesh-controller/planner_rules_test.go index f6c0c5cf..1622f336 100644 --- a/cmd/mesh-controller/planner_rules_test.go +++ b/cmd/mesh-controller/planner_rules_test.go @@ -218,14 +218,9 @@ func TestAPlanIsWhatTheChangeTouchedAndWhatIsBuiltOnIt(t *testing.T) { if u := strings.Join(r.Unread, ","); u != c.unread { t.Errorf("%s: unread %q, wanted %q", c.what, u, c.unread) } - // BUG found by this test on main (8170fc5): hasCycle reads any edge between two modules of the last - // tier as a cycle — a packages edge too, which puts both there by rule — so the merge handler logs - // "the last tier depends on itself … built together, in no order" for a plan with no cycle (the - // packages rows and the mixed diamond fail here). ADR 0162: a cycle is one last tier, *and said*; - // one said where there is none is the rule broken. The expectation stays. + // A packages edge in the last tier is no cycle; hasCycle said one on main at 8170fc5. if hasCycle(r.Plan.Tiers, c.edges) != c.cycle { - t.Errorf("%s: a cycle said %v, wanted %v (%v) [BUG: hasCycle counts a packages edge]", - c.what, !c.cycle, c.cycle, r.Plan.Tiers) + t.Errorf("%s: a cycle said %v, wanted %v (%v)", c.what, !c.cycle, c.cycle, r.Plan.Tiers) } } } @@ -310,7 +305,7 @@ var sharedRepositoryEdges = []inventory.Edge{ // packages — never along built-by or worker-of; // - every stands-on, declared, built-by and worker-of edge with both ends in the plan has the module // depended on in an earlier tier; -// - no cycle is said (this one fails on main: see the BUG note below). +// - no cycle is said. // // Seeded, so a failure is replayed by its seed and case. func TestAPlanIsTheTouchedModulesAndWhatIsReachableAlongTheWideningEdges(t *testing.T) { @@ -413,10 +408,10 @@ func TestAPlanIsTheTouchedModulesAndWhatIsReachableAlongTheWideningEdges(t *test } } } - // BUG found by this property on main (8170fc5); see the table's cycle check. Said once, after the - // other invariants have run over every case, so it hides none of them. + // Said once, after the other invariants have run over every case, so it hides none of them (hasCycle + // counted a packages edge on main at 8170fc5: 19 of these 500 cases). if falseCycles > 0 { - t.Errorf("a cycle said of a graph with none in %d of 500 cases [BUG: hasCycle counts a packages edge]; "+ + t.Errorf("a cycle said of a graph with none in %d of 500 cases; "+ "the first:\n%s", falseCycles, firstFalseCycle) } } diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index c240d478..8688705b 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -157,7 +157,9 @@ func reachableFrom(moved []string, edges []inventory.Edge) []string { return out } -// hasCycle says whether the tiers' last tier holds modules that still depend on each other. +// hasCycle says whether the tiers' last tier holds modules that still depend on each other. A packages +// edge orders nothing (tiersOf), so a module and what packages its source share a tier by rule: that is +// no cycle, and saying one was is a false report in every such plan's log. func hasCycle(tiers [][]string, edges []inventory.Edge) bool { if len(tiers) == 0 { return false @@ -167,6 +169,9 @@ func hasCycle(tiers [][]string, edges []inventory.Edge) bool { last[m] = true } for _, e := range edges { + if e.Kind == inventory.EdgePackages { + continue + } if last[e.From] && last[e.To] { return true }