From bd35b1c06c40cabe6f40795bca5deb647b6f1543 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 03:23:50 +0200 Subject: [PATCH] Judge a packaging module's news by plans that built it, over every plan since its build (hq ADR 0267, review) A plan that closed without building a module hid a missed merge from it; a window of recent plans let an old closure back in once the plan that overtook it slid out; a merge the forge gave no time was acted on again every pass. A plan answering the merge's own commit is now a look at it, and clocks a little apart do not make a merge history. --- cmd/mesh-controller/missed_merges.go | 5 +++ cmd/mesh-controller/planner_rules_test.go | 20 ++++++++- cmd/mesh-controller/upgrades.go | 52 +++++++++++++++++++---- internal/inventory/builds.go | 3 ++ internal/inventory/plans.go | 5 +++ 5 files changed, 75 insertions(+), 10 deletions(-) diff --git a/cmd/mesh-controller/missed_merges.go b/cmd/mesh-controller/missed_merges.go index 6428a2a7..a5d3f7fa 100644 --- a/cmd/mesh-controller/missed_merges.go +++ b/cmd/mesh-controller/missed_merges.go @@ -102,6 +102,11 @@ func catchUpOnMerges(ctx context.Context, now time.Time, announced merges, return err } for _, a := range all { + // A merge the forge said no time of is dated by its announcement, so a packaging module's look can + // make it history once acted on, and the catch-up does not act on it again every pass (ADR 0267). + if a.SourceMoved.MergedAt == "" && !a.At.IsZero() { + a.SourceMoved.MergedAt = a.At.UTC().Format(time.RFC3339) + } if now.Sub(a.At) < mergeGrace { continue } diff --git a/cmd/mesh-controller/planner_rules_test.go b/cmd/mesh-controller/planner_rules_test.go index 8f591813..f47cded2 100644 --- a/cmd/mesh-controller/planner_rules_test.go +++ b/cmd/mesh-controller/planner_rules_test.go @@ -403,11 +403,29 @@ func TestAMissedMergeMovingOnlyAPackagingModuleIsActedOnOnce(t *testing.T) { if got := wouldMove(m, entries, planningView(read, nil)); len(got) != 1 || got[0].Manifest.Module != "route-proxy" { t.Fatalf("a missed merge of the proxy's program would move %v", got) } - acted := []inventory.Plan{{State: inventory.PlanBuilding, Created: merged.Add(time.Minute), + // Acted on at once: the plan answering this very merge is a look, however close the clocks. + atOnce := []inventory.Plan{{State: inventory.PlanBuilding, Commit: "c1", Created: merged.Add(2 * time.Second), + Modules: map[string]*inventory.PlanModule{"route-proxy": {State: "building"}}}} + if got := wouldMove(m, entries, planningView(read, atOnce)); len(got) != 0 { + t.Fatalf("a merge whose own plan holds the proxy would move %v again", got) + } + acted := []inventory.Plan{{State: inventory.PlanBuilding, Created: merged.Add(2 * time.Minute), Modules: map[string]*inventory.PlanModule{"route-proxy": {State: "building"}}}} if got := wouldMove(m, entries, planningView(read, acted)); len(got) != 0 { t.Fatalf("a merge acted on for the proxy would move %v again", got) } + // A plan that closed without building it looked at nothing: the merge is still news for it. + closed := []inventory.Plan{{State: "failed", Created: merged.Add(2 * time.Minute), + Modules: map[string]*inventory.PlanModule{"route-proxy": {State: "waiting"}}}} + if got := wouldMove(m, entries, planningView(read, closed)); len(got) != 1 { + t.Fatalf("a plan that never built the proxy hid the merge from it: %v", got) + } + // A look just before the merge, on clocks a little apart, is no look after it. + skewed := []inventory.Plan{{State: inventory.PlanBuilding, Created: merged.Add(30 * time.Second), + Modules: map[string]*inventory.PlanModule{"route-proxy": {State: "building"}}}} + if got := wouldMove(m, entries, planningView(read, skewed)); len(got) != 1 { + t.Fatalf("a look within the clocks' margin made the merge history: %v", got) + } } // sharedRepositoryEdges is what dependenciesOf derives for the catalogue of the test above, sorted as it diff --git a/cmd/mesh-controller/upgrades.go b/cmd/mesh-controller/upgrades.go index 9a41fe48..dc089f96 100644 --- a/cmd/mesh-controller/upgrades.go +++ b/cmd/mesh-controller/upgrades.go @@ -569,7 +569,13 @@ func mergeCandidates(m link.SourceMoved, entries []inventory.Entry, // build or plan of it after the merge already read the repository with the merge in it. Per // module, since a merge that moved only the module built from the repository says nothing // about the ones packaging it. - if isHistory(m.MergedAt, lookedOf(read[e.Manifest.Module])) { + // Judged with a margin for the forge's clock running behind the store's: too late a look + // rebuilds once more, too early one would miss the merge. + if lookedAtCommit(read[e.Manifest.Module], m.Commit) { + continue + } + if looked := lookedOf(read[e.Manifest.Module]); !looked.IsZero() && + isHistory(m.MergedAt, looked.Add(-historyMargin)) { continue } packaging = append(packaging, e) @@ -578,6 +584,19 @@ func mergeCandidates(m link.SourceMoved, entries []inventory.Entry, return from, packaging, already } +// lookedAtCommit is whether a plan that built a module, or is building it, answered this merge commit. +func lookedAtCommit(read []inventory.ReadRepository, commit string) bool { + for _, r := range read { + if slices.Contains(r.LookedAt, commit) { + return true + } + } + return false +} + +// historyMargin is how far a packaging module's last look is taken back before a merge is history for it. +const historyMargin = time.Minute + // lookedOf is when a module packaging another repository was last looked at, as readForPlanning says. func lookedOf(read []inventory.ReadRepository) time.Time { var at time.Time @@ -761,8 +780,9 @@ func staleIn(read map[string][]inventory.ReadRepository, plans []inventory.Plan) } // lookedAt is when a merge was last acted on for a module that packages another repository's source: its -// newest build, or the newest plan that held it, whichever is later. A build asked after a merge clones that -// repository with the merge in it, so an older merge is history for it. +// newest build, or the newest plan that built it or is still building it, whichever is later. A build asked +// after a merge clones that repository with the merge in it, so an older merge is history for it; a plan +// that closed without building it looked at nothing. func lookedAt(name string, read []inventory.ReadRepository, plans []inventory.Plan) time.Time { var at time.Time for _, r := range read { @@ -771,7 +791,8 @@ func lookedAt(name string, read []inventory.ReadRepository, plans []inventory.Pl } } for _, p := range plans { - if _, in := p.Modules[name]; in && p.Created.After(at) { + s, in := p.Modules[name] + if in && (p.Open() || (s != nil && s.State == "built")) && p.Created.After(at) { at = p.Created } } @@ -787,22 +808,35 @@ func readForPlanning(ctx context.Context, inv *inventory.Inventory) (map[string] if err != nil { return nil, err } - plans, err := inv.RecentPlans(ctx, planLookBack) + // Every plan since the oldest build whose source is recorded: one made after a module's build can have + // overtaken it, however long ago, so no window of recent plans would do. + var oldest time.Time + for _, rs := range read { + for _, r := range rs { + if !r.Built.IsZero() && (oldest.IsZero() || r.Built.Before(oldest)) { + oldest = r.Built + } + } + } + plans, err := inv.PlansSince(ctx, oldest) if err != nil { return nil, err } return planningView(read, plans), nil } -// planLookBack is how many recent plans judge whether a recorded build source is overtaken. -const planLookBack = 500 - // planningView is readForPlanning over what was read, so a test can hand it records. func planningView(read map[string][]inventory.ReadRepository, plans []inventory.Plan) map[string][]inventory.ReadRepository { stale := staleIn(read, plans) out := make(map[string][]inventory.ReadRepository, len(read)) for name, rs := range read { looked := lookedAt(name, rs, plans) + var commits []string + for _, p := range plans { + if st, in := p.Modules[name]; in && p.Commit != "" && (p.Open() || (st != nil && st.State == "built")) { + commits = append(commits, p.Commit) + } + } var kept []inventory.ReadRepository for _, r := range rs { if stale[name] { @@ -811,7 +845,7 @@ func planningView(read map[string][]inventory.ReadRepository, plans []inventory. } r.Paths = nil } - r.Looked = looked + r.Looked, r.LookedAt = looked, commits kept = append(kept, r) } out[name] = kept diff --git a/internal/inventory/builds.go b/internal/inventory/builds.go index 0550f195..73f6020a 100644 --- a/internal/inventory/builds.go +++ b/internal/inventory/builds.go @@ -95,6 +95,9 @@ type ReadRepository struct { // any outcome was: what the planner judges a build source's age, and a merge's news, by. Never stored. Built time.Time `json:"-"` Looked time.Time `json:"-"` + // LookedAt are the merge commits a plan that built the module, or is building it, answered: a merge + // of one of them is history for the module whatever the clocks say. Never stored. + LookedAt []string `json:"-"` } // BuildSource is the build source a build read in one repository (novox/hq ADR 0267): Repository and Ref diff --git a/internal/inventory/plans.go b/internal/inventory/plans.go index 59f832b8..526bdd37 100644 --- a/internal/inventory/plans.go +++ b/internal/inventory/plans.go @@ -312,6 +312,11 @@ func (i *Inventory) OpenPlans(ctx context.Context) ([]Plan, error) { return i.plans(ctx, `where state in ('building', 'rolling') order by created`) } +// PlansSince is every plan made after a moment, and every plan still being worked, oldest first. +func (i *Inventory) PlansSince(ctx context.Context, since time.Time) ([]Plan, error) { + return i.plans(ctx, `where created > $1 or state in ('building', 'rolling') order by created`, since) +} + // RecentPlans is the last few plans, newest first, open or not — what the overview shows. func (i *Inventory) RecentPlans(ctx context.Context, limit int) ([]Plan, error) { return i.plans(ctx, fmt.Sprintf(`order by created desc limit %d`, limit))