From 6c616838a5190c971e536083faf14ed5c04c2406 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 02:56:48 +0200 Subject: [PATCH 1/3] Move a module on a merge only when its build source holds a changed file (hq ADR 0267, issue 363) Every merge to the controller's repository planned the controller, the build seat's holder and the route proxy in three gated tiers, whatever it changed (issue 338). The planner now maps a merge's files onto the build source each module's newest trunk build said: a README moves nothing, the controller's command the controller alone, the proxy's program the proxy alone. A module with none said, or one an open plan has yet to build, is read whole as before. Sharing a repository draws no packages edge any more, and one recorded before neither widens nor orders a plan. --- cmd/mesh-controller/build.go | 4 + cmd/mesh-controller/check_here.go | 4 +- cmd/mesh-controller/facts.go | 9 + cmd/mesh-controller/merge_gate.go | 13 +- cmd/mesh-controller/missed_merges.go | 2 +- cmd/mesh-controller/order_test.go | 4 +- cmd/mesh-controller/planner_records_test.go | 62 +++-- cmd/mesh-controller/planner_rules_test.go | 216 ++++++++++++++---- cmd/mesh-controller/release_plan.go | 7 +- cmd/mesh-controller/release_plan_test.go | 13 +- cmd/mesh-controller/upgrades.go | 146 ++++++++++-- cmd/mesh-controller/worker_order_test.go | 8 +- internal/facts/facts.go | 11 + internal/inventory/builds.go | 87 ++++++- internal/inventory/builds_test.go | 45 ++++ internal/inventory/dependencies.go | 38 ++- internal/inventory/dependencies_test.go | 5 +- .../0088-a-build-says-its-build-source.sql | 9 + 18 files changed, 543 insertions(+), 140 deletions(-) create mode 100644 internal/inventory/migrations/0088-a-build-says-its-build-source.sql diff --git a/cmd/mesh-controller/build.go b/cmd/mesh-controller/build.go index 6f1c0cd5..5b772e91 100644 --- a/cmd/mesh-controller/build.go +++ b/cmd/mesh-controller/build.go @@ -166,6 +166,10 @@ func buildFrom(result link.BuildResult) inventory.Build { for _, r := range result.Read { kept.Read = append(kept.Read, inventory.ReadRepository{Repository: r.Repository, Ref: r.Ref}) } + // What it was made from, as files (novox/hq ADR 0267): the planner maps the next merge onto it. + for _, s := range result.Sources { + kept.Sources = append(kept.Sources, inventory.BuildSource{Repository: s.Repository, Ref: s.Ref, Paths: s.Paths}) + } var announced []inventory.Artifact for _, made := range result.Made { announced = append(announced, inventory.Artifact{ diff --git a/cmd/mesh-controller/check_here.go b/cmd/mesh-controller/check_here.go index 58645c71..201e1dec 100644 --- a/cmd/mesh-controller/check_here.go +++ b/cmd/mesh-controller/check_here.go @@ -82,7 +82,9 @@ func checkHereCommand(ctx context.Context, args []string) error { if _, err := git("fetch", "--quiet", "origin", *base); err != nil { return fmt.Errorf("cannot fetch %s to say what the change touches: %w", *base, err) } - changedText, err := git("diff", "--name-only", "origin/"+*base+"...HEAD") + // Without rename detection, so a file moved out of a build source is said under its old name as well: + // its going is a change to the build that held it (novox/hq ADR 0267). + changedText, err := git("diff", "--name-only", "--no-renames", "origin/"+*base+"...HEAD") if err != nil { return err } diff --git a/cmd/mesh-controller/facts.go b/cmd/mesh-controller/facts.go index 01ffd437..2d0cda96 100644 --- a/cmd/mesh-controller/facts.go +++ b/cmd/mesh-controller/facts.go @@ -411,7 +411,16 @@ func gatherFacts(ctx context.Context, open *stores, busVersion string) (snapshot Commit: e.Source.BuiltFrom, Provided: e.Provided, RollOut: current[e.Manifest.Module].RollOut, Manifest: raw} for _, r := range read[e.Manifest.Module] { + // The module's own build source is said apart: a gate that predates it would read an own + // entry among Reads as a context of its own repository. + if r.Own { + mod.Sources = append(mod.Sources, snapshot.BuildSource{Own: true, Paths: r.Paths}) + continue + } mod.Reads = append(mod.Reads, snapshot.RepositoryName(r.Repository)) + if len(r.Paths) > 0 { + mod.Sources = append(mod.Sources, snapshot.BuildSource{Repository: snapshot.RepositoryName(r.Repository), Paths: r.Paths}) + } } f.Modules = append(f.Modules, mod) if e.Provided || e.Source.Repository == "" { diff --git a/cmd/mesh-controller/merge_gate.go b/cmd/mesh-controller/merge_gate.go index dd925642..ddcfd609 100644 --- a/cmd/mesh-controller/merge_gate.go +++ b/cmd/mesh-controller/merge_gate.go @@ -1204,7 +1204,18 @@ func graphOfFacts(f snapshot.Facts) ([]inventory.Entry, map[string][]inventory.R entries = append(entries, inventory.Entry{Manifest: manifest, Provided: mod.Provided, Source: inventory.Source{Repository: mod.Repository, Path: mod.Path, BuiltFrom: mod.Commit}}) for _, r := range mod.Reads { - read[mod.Name] = append(read[mod.Name], inventory.ReadRepository{Repository: r}) + entry := inventory.ReadRepository{Repository: r} + for _, s := range mod.Sources { + if !s.Own && s.Repository == r && len(s.Paths) > 0 { + entry.Paths = s.Paths + } + } + read[mod.Name] = append(read[mod.Name], entry) + } + for _, s := range mod.Sources { + if s.Own && len(s.Paths) > 0 { + read[mod.Name] = append(read[mod.Name], inventory.ReadRepository{Own: true, Paths: s.Paths}) + } } } var edges []inventory.Edge diff --git a/cmd/mesh-controller/missed_merges.go b/cmd/mesh-controller/missed_merges.go index d68bfebf..fb43871f 100644 --- a/cmd/mesh-controller/missed_merges.go +++ b/cmd/mesh-controller/missed_merges.go @@ -48,7 +48,7 @@ func catchingUpOnMerges(ctx context.Context, open *stores, announced merges) { if err != nil { return nil, nil, err } - read, err := open.inventory.ReadRepositories(ctx) + read, err := readForAMerge(ctx, open.inventory) return entries, read, err } failing := "" diff --git a/cmd/mesh-controller/order_test.go b/cmd/mesh-controller/order_test.go index 7c6a901c..f362c8df 100644 --- a/cmd/mesh-controller/order_test.go +++ b/cmd/mesh-controller/order_test.go @@ -157,7 +157,7 @@ func TestAMergeRebuildsTheModulesItChanged(t *testing.T) { {"a file directly among the modules", merge([]string{"modules/README.md"}, false), ""}, {"a root file beside a module's", merge([]string{"merge-check.sh", "modules/keycloak/x.ts"}, false), "keycloak"}, } { - if got := named(whatTheMergeTouched(candidates, known, c.m)); got != c.want { + if got := named(whatTheMergeTouched(candidates, known, c.m, nil)); got != c.want { t.Errorf("%s: rebuilt %q, wanted %q", c.what, got, c.want) } } @@ -285,7 +285,7 @@ func TestAChangeInsideAModuleIsThatModulesHeldOrNot(t *testing.T) { {"an old announcer saying nothing", merge(showcase, nil, false), ""}, {"a manifest the merge removed, said or not", merge([]string{"modules/gone/module.json", "modules/gone/x.ts"}, nil, true), ""}, } { - if got := named(whatTheMergeTouched(candidates, known, c.m)); got != c.want { + if got := named(whatTheMergeTouched(candidates, known, c.m, nil)); got != c.want { t.Errorf("%s: rebuilt %q, wanted %q", c.what, got, c.want) } } diff --git a/cmd/mesh-controller/planner_records_test.go b/cmd/mesh-controller/planner_records_test.go index bb636f13..6d3e261c 100644 --- a/cmd/mesh-controller/planner_records_test.go +++ b/cmd/mesh-controller/planner_records_test.go @@ -15,16 +15,16 @@ import ( // inventory.Dependencies (dependenciesOf over the records), and the merge planned by reachOfMerge — the // path a real merge takes, short of the bus. // -// **The repository rows are CURRENT BEHAVIOUR, documented — not the rule the operator states** -// (novox/hq issue 338, and the decision pending on it): a build that read a repository gives its module a -// packages edge to every module built from that repository, and mergeCandidates moves it on any merge to -// that repository, whatever the files. So a change to C alone, or to a README, moves the module that -// packages C's repository. ADR 0238 §3 records exactly that today ("a repository a recipe names"); the -// expectations marked 338 change with that decision. -func TestASharedRepositoryIsPlannedFromTheRecordsAsItIsToday(t *testing.T) { +// **A shared repository moves only what a change's files are in the build source of** (novox/hq ADR 0267, +// issues 338 and 363): each build records the build source it said, and a merge is mapped onto those of the +// newest builds. A build that said none (P below, as every build before ADR 0267) is read as before: P moves +// on any merge to the repository it packages, though through no edge. The rows tagged 338 held the opposite +// until ADR 0267 was built. +func TestASharedRepositoryIsPlannedFromTheRecordedBuildSources(t *testing.T) { inv := inventory.ForTest(t) ctx := t.Context() asked := time.Now().Add(-time.Hour) + sourcesOf := map[string][]inventory.BuildSource{} register := func(m catalogue.Manifest, repository, path string, against []string, read []inventory.ReadRepository) { t.Helper() if err := inv.RegisterModule(ctx, m, inventory.Source{Repository: repository, Seat: "git", Path: path, @@ -32,7 +32,8 @@ func TestASharedRepositoryIsPlannedFromTheRecordsAsItIsToday(t *testing.T) { t.Fatal(err) } if err := inv.RecordBuild(ctx, inventory.Build{ID: "build-" + m.Module, Repository: repository, Ref: "main", - Module: m.Module, Commit: "old", On: "builder", Path: path, Against: against, Read: read, Asked: asked}); err != nil { + Module: m.Module, Commit: "old", On: "builder", Path: path, Against: against, Read: read, Asked: asked, + Sources: sourcesOf[m.Module]}); err != nil { t.Fatal(err) } } @@ -40,7 +41,16 @@ func TestASharedRepositoryIsPlannedFromTheRecordsAsItIsToday(t *testing.T) { agent := catalogue.Manifest{Module: "build-agent", Version: "1", Claims: []catalogue.Claim{{Name: "node-build-agent", Scope: catalogue.ScopeNode}}} - // The shape of issue 338. + // The shape of issue 338, each build saying its build source (ADR 0267). + gomod := []string{"go.mod", "go.sum"} + sourcesOf["mesh-controller"] = []inventory.BuildSource{{Paths: append([]string{"module.json", "cmd/mesh-controller/", + "internal/conditions/", "internal/broker/"}, gomod...)}} + sourcesOf["build-agent"] = []inventory.BuildSource{ + {Paths: []string{"modules/build-agent/Dockerfile", "modules/build-agent/module.json"}}, + {Repository: "novox/mesh-controller", Ref: "main", Paths: append([]string{"cmd/mesh-builder/", "internal/broker/"}, gomod...)}} + sourcesOf["route-proxy"] = []inventory.BuildSource{ + {Paths: []string{"modules/route-proxy/Dockerfile", "modules/route-proxy/module.json"}}, + {Repository: "novox/mesh-controller", Ref: "main", Paths: append([]string{"examples/route-proxy/", "internal/broker/"}, gomod...)}} register(catalogue.Manifest{Module: "mesh-controller", Version: "1"}, "novox/mesh-controller", "", nil, nil) register(agent, "novox/mesh-catalog", "modules/build-agent", nil, controllerRead) register(catalogue.Manifest{Module: "route-proxy", Version: "1"}, "novox/mesh-catalog", "modules/route-proxy", nil, controllerRead) @@ -81,13 +91,15 @@ func TestASharedRepositoryIsPlannedFromTheRecordsAsItIsToday(t *testing.T) { if !reflect.DeepEqual(shared, sharedRepositoryEdges) { t.Errorf("derived %v\nthe hand-written rows use %v", shared, sharedRepositoryEdges) } - // Each kind derived from its record: built against (stands-on), build.on (declared), read (packages). + // Each kind derived from its record: built against (stands-on), build.on (declared); a read draws none. + for _, e := range edges { + if e.Kind == inventory.EdgePackages { + t.Errorf("a packages edge was drawn (ADR 0267 rule 4): %v", e) + } + } for _, want := range []inventory.Edge{ dep("d", inventory.EdgeStandsOn, "a"), dep("e", inventory.EdgeDeclared, "b"), - dep("p", inventory.EdgePackages, "a"), - dep("p", inventory.EdgePackages, "b"), - dep("p", inventory.EdgePackages, "c"), dep("d", inventory.EdgeBuiltBy, "build-agent"), } { found := false @@ -105,15 +117,23 @@ func TestASharedRepositoryIsPlannedFromTheRecordsAsItIsToday(t *testing.T) { want string issue338 bool }{ - {"A and B changed, C untouched: D after A, E after B; P packages their repository", "one", + {"A and B changed, C untouched: D after A, E after B; P, which said no build source, reads all", "one", []string{"modules/a/x.go", "modules/b/x.go"}, "a,b,p | d,e", false}, - {"C alone: C, and P, which packages C's repository", "one", - []string{"modules/c/x.go"}, "c,p", true}, - {"a README of the repository P packages: P moves, nothing built from it does", "one", - []string{"README.md"}, "p", true}, + {"C alone: C, and P, read whole as before; P after nothing", "one", + []string{"modules/c/x.go"}, "c,p", false}, + {"a README of the repository P packages: P, read whole as before", "one", + []string{"README.md"}, "p", false}, {"the dependent's repository: D alone", "two", []string{"d/main.go"}, "d", false}, - {"a README of the controller's repository: all three, three tiers", "mesh-controller", - []string{"README.md"}, "mesh-controller | build-agent | route-proxy", true}, + {"a README of the controller's repository: no module", "mesh-controller", + []string{"README.md"}, "", true}, + {"the controller's own command: the controller alone", "mesh-controller", + []string{"cmd/mesh-controller/main.go"}, "mesh-controller", true}, + {"a package only the controller builds from: the controller alone", "mesh-controller", + []string{"internal/conditions/condition.go"}, "mesh-controller", true}, + {"the route proxy's program: the route proxy alone", "mesh-controller", + []string{"examples/route-proxy/main.go"}, "route-proxy", true}, + {"a package all three build from: all three", "mesh-controller", + []string{"internal/broker/broker.go"}, "mesh-controller | build-agent | route-proxy", false}, {"the route proxy's directory in the catalogue: it alone", "mesh-catalog", []string{"modules/route-proxy/module.json"}, "route-proxy", false}, {"the build agent's directory: it alone, nothing it builds", "mesh-catalog", @@ -123,7 +143,7 @@ func TestASharedRepositoryIsPlannedFromTheRecordsAsItIsToday(t *testing.T) { if got != c.want { tag := "" if c.issue338 { - tag = " (current behaviour, issue 338)" + tag = " (issue 338, flipped by ADR 0267)" } t.Errorf("%s: planned %q, wanted %q%s", c.what, got, c.want, tag) } diff --git a/cmd/mesh-controller/planner_rules_test.go b/cmd/mesh-controller/planner_rules_test.go index 6b8dd23d..1aab2260 100644 --- a/cmd/mesh-controller/planner_rules_test.go +++ b/cmd/mesh-controller/planner_rules_test.go @@ -1,12 +1,14 @@ package main import ( + "encoding/json" "fmt" "math/rand/v2" "sort" "strings" "testing" + snapshot "github.com/novox/mesh-controller/internal/facts" "github.com/novox/mesh-controller/internal/inventory" "github.com/novox/mesh-controller/internal/link" ) @@ -21,7 +23,7 @@ import ( // kind widens the plan orders the tiers // stands-on yes yes, after its base is built // declared yes yes, after its base is built -// packages yes no, the same tier (a code dependency) +// packages no no — retired by novox/hq ADR 0267; one recorded before is read and ignored // built-by no yes, after the build machine — except for what the build machine stands // on, and for the controller whose worker it binds // worker-of no yes, the build seat's holder after the controller (hq issue 206) @@ -123,9 +125,9 @@ func TestAPlanIsWhatTheChangeTouchedAndWhatIsBuiltOnIt(t *testing.T) { {what: "transitive: F on D on A, A changed", edges: []inventory.Edge{dep("f", standsOn, "d"), dep("d", standsOn, "a")}, repo: "one", paths: []string{"modules/a/x"}, want: "a | d | f"}, - {what: "transitive across kinds: F declared on D, D packages A", + {what: "transitive across kinds stops at a packages edge: F declared on D, D packages A (ADR 0267)", edges: []inventory.Edge{dep("f", declared, "d"), dep("d", packages, "a")}, - repo: "one", paths: []string{"modules/a/x"}, want: "a,d | f"}, + repo: "one", paths: []string{"modules/a/x"}, want: "a"}, // Each kind alone: X depends on A, A changed (widening), then both changed (ordering). {what: "stands-on (built against A's artifact) widens", edges: []inventory.Edge{dep("x", standsOn, "a")}, @@ -136,8 +138,8 @@ func TestAPlanIsWhatTheChangeTouchedAndWhatIsBuiltOnIt(t *testing.T) { repo: "one", paths: []string{"modules/a/x"}, want: "a | x"}, {what: "declared orders", edges: []inventory.Edge{dep("x", declared, "a")}, repo: "one", paths: []string{"modules/a/x", "modules/x/y"}, want: "a | x"}, - {what: "packages widens, into the same tier", edges: []inventory.Edge{dep("x", packages, "a")}, - repo: "one", paths: []string{"modules/a/x"}, want: "a,x"}, + {what: "packages, recorded before ADR 0267, widens nothing", edges: []inventory.Edge{dep("x", packages, "a")}, + repo: "one", paths: []string{"modules/a/x"}, want: "a"}, {what: "packages does not order", edges: []inventory.Edge{dep("x", packages, "a")}, repo: "one", paths: []string{"modules/a/x", "modules/x/y"}, want: "a,x"}, {what: "built-by never widens", edges: []inventory.Edge{dep("x", builtBy, "a")}, @@ -188,7 +190,7 @@ func TestAPlanIsWhatTheChangeTouchedAndWhatIsBuiltOnIt(t *testing.T) { repo: "one", paths: []string{"modules/z/x"}, want: "z | a,b | d"}, {what: "a diamond of mixed kinds orders on the ordering side only", edges: []inventory.Edge{dep("d", standsOn, "a"), dep("d", packages, "b")}, - repo: "one", paths: []string{"modules/b/x"}, want: "b,d"}, + repo: "one", paths: []string{"modules/b/x"}, want: "b"}, // A cycle the catalogue should never produce: what remains is one last tier, and said. {what: "a cycle is one last tier, not lost", edges: []inventory.Edge{dep("a", standsOn, "b"), dep("b", standsOn, "a"), @@ -225,22 +227,16 @@ func TestAPlanIsWhatTheChangeTouchedAndWhatIsBuiltOnIt(t *testing.T) { } } -// **CURRENT BEHAVIOUR, documented — not the rule the operator states.** novox/hq issue 338 (a module -// built from a shared repository moves on every merge to it) and the decision pending on it would change -// every row here. Today: +// **A shared repository moves only what a change's files are in the build source of** (novox/hq ADR 0267, +// issue 338, issue 363). The controller is built from its repository's root as a Go bundle; the route proxy +// and the build seat's holder build images whose context is that repository and which name the package they +// compile. Each newest trunk build said its build source — the import closure of its program — and a merge +// is mapped onto those. The rows tagged 338 held the opposite until ADR 0267 was built: every merge to the +// controller's repository planned all three, in three tiers. // -// - mesh-controller is built from its repository's root, so every file of that repository touches it; -// - route-proxy and build-agent package the whole of that repository (a build context), so the build -// record's `read` makes them move on any merge to it, whatever the files, and dependenciesOf gives -// each a packages edge to every module built from it; -// - built-by (route-proxy on build-agent) and worker-of (build-agent on the controller) make it three -// tiers. -// -// These follow ADR 0238 §3 as written ("the whole repository for a module built from its root, and a -// repository a recipe names"), so they are not failures; when the decision on issue 338 lands, these -// expectations change with it. The edges are the ones dependenciesOf derives from this catalogue — held -// to that by TestASharedRepositoryIsPlannedFromTheRecordsAsItIsToday, which derives them from the store. -func TestASharedRepositoryMovesWhatPackagesItAsItDoesToday(t *testing.T) { +// The build sources are this repository's own programs as GoBuildSource reads them (held to that by +// TestThisRepositorysProgramsHaveBuildSourcesOfTheirOwn in internal/builder), cut to what the rows need. +func TestASharedRepositoryMovesOnlyWhatItsBuildSourceHolds(t *testing.T) { const catalogueRepo = "http://forge.internal:20000/novox/mesh-catalog.git" const controllerRepo = "http://forge.internal:20000/novox/mesh-controller.git" entries := []inventory.Entry{ @@ -249,7 +245,26 @@ func TestASharedRepositoryMovesWhatPackagesItAsItDoesToday(t *testing.T) { fromRepo("route-proxy", catalogueRepo, "modules/route-proxy"), fromRepo("gitea", catalogueRepo, "modules/gitea"), } + gomod := []string{"go.mod", "go.sum", "vendor/modules.txt"} + with := func(paths ...string) []string { return append(append([]string{}, gomod...), paths...) } read := map[string][]inventory.ReadRepository{ + "mesh-controller": {{Own: true, Paths: with("module.json", "cmd/mesh-controller/", "internal/conditions/", + "internal/broker/", "internal/builder/", "internal/inventory/", "internal/inventory/migrations/**", + "vendor/github.com/nats-io/nats.go/")}}, + "build-agent": { + {Repository: "novox/mesh-controller", Ref: "main", Paths: with("cmd/mesh-builder/", "internal/broker/", + "internal/builder/", "internal/inventory/", "internal/inventory/migrations/**", "vendor/github.com/nats-io/nats.go/")}, + {Own: true, Paths: []string{"modules/build-agent/Dockerfile", "modules/build-agent/module.json"}}, + }, + "route-proxy": { + {Repository: "novox/mesh-controller", Ref: "main", Paths: with("examples/route-proxy/", "internal/broker/", + "vendor/github.com/nats-io/nats.go/")}, + {Own: true, Paths: []string{"modules/route-proxy/Dockerfile", "modules/route-proxy/module.json"}}, + }, + } + // With no build source said — before each module's first trunk build under ADR 0267, or while an + // earlier merge's build of it is pending — a module is read as before. + unsaid := map[string][]inventory.ReadRepository{ "build-agent": {{Repository: "novox/mesh-controller", Ref: "main"}}, "route-proxy": {{Repository: "novox/mesh-controller", Ref: "main"}}, } @@ -257,51 +272,122 @@ func TestASharedRepositoryMovesWhatPackagesItAsItDoesToday(t *testing.T) { for _, c := range []struct { what, repo string paths []string + read map[string][]inventory.ReadRepository want string + unread string }{ - // The live three-tier plan of 2026-10-08 (issue 338), in the worker-of order (issue 206) that - // TestAMergeIsPlannedInTiersAlongTheThreeKindsOfDependency's controller case holds too. - {"a README of the controller's repository moves all three, in three tiers", "mesh-controller", - []string{"README.md"}, "mesh-controller | build-agent | route-proxy"}, - {"the controller's own code: the same", "mesh-controller", - []string{"cmd/mesh-controller/main.go"}, "mesh-controller | build-agent | route-proxy"}, - {"the route proxy's program alone: the same, the controller with it", "mesh-controller", - []string{"examples/route-proxy/main.go"}, "mesh-controller | build-agent | route-proxy"}, - // In the catalogue, where they live, the rule is path-precise. - {"the route proxy's directory in the catalogue: it alone", "mesh-catalog", - []string{"modules/route-proxy/module.json"}, "route-proxy"}, - {"the build agent's directory: it alone, nothing it builds", "mesh-catalog", - []string{"modules/build-agent/module.json"}, "build-agent"}, + // The operator's acceptance: a merge of the controller's own code plans the controller alone. + {"the controller's own command: the controller alone (338)", "mesh-controller", + []string{"cmd/mesh-controller/main.go"}, read, "mesh-controller", ""}, + {"a package only the controller builds from: the controller alone (338)", "mesh-controller", + []string{"internal/conditions/condition.go", "internal/conditions/bus.go"}, read, "mesh-controller", ""}, + {"a test beside the controller's command: nothing is built from it", "mesh-controller", + []string{"cmd/mesh-controller/main_test.go"}, read, "", "cmd/mesh-controller/main_test.go"}, + {"a README of the controller's repository: no module (338)", "mesh-controller", + []string{"README.md"}, read, "", "README.md"}, + {"the route proxy's program alone: the route proxy alone (338)", "mesh-controller", + []string{"examples/route-proxy/main.go"}, read, "route-proxy", ""}, + {"the build seat's program alone: its holder alone", "mesh-controller", + []string{"cmd/mesh-builder/main.go"}, read, "build-agent", ""}, + {"a package the build seat's program and the controller build from: both", "mesh-controller", + []string{"internal/builder/builder.go"}, read, "mesh-controller | build-agent", ""}, + {"a migration the controller and the build seat's program embed: both", "mesh-controller", + []string{"internal/inventory/migrations/0088-a-build-says-its-build-source.sql"}, read, + "mesh-controller | build-agent", ""}, + // A package all three build from: all three; the build seat's holder after the controller whose worker + // it binds (worker-of, issue 206), the proxy after the holder that builds it (built-by). + {"a package all three build from: all three", "mesh-controller", + []string{"internal/broker/broker.go"}, read, "mesh-controller | build-agent | route-proxy", ""}, + {"a vendored package all three build from: all three", "mesh-controller", + []string{"vendor/github.com/nats-io/nats.go/nats.go"}, read, "mesh-controller | build-agent | route-proxy", ""}, + {"go.sum: all three", "mesh-controller", + []string{"go.sum"}, read, "mesh-controller | build-agent | route-proxy", ""}, + {"a file added to the proxy's package: the proxy", "mesh-controller", + []string{"examples/route-proxy/new.go"}, read, "route-proxy", ""}, + // Before any build source is said: as before. + {"no build source said: a README moves all three, as before", "mesh-controller", + []string{"README.md"}, unsaid, "mesh-controller | build-agent | route-proxy", ""}, + // In the catalogue, where they live, each by its own build source. + {"the route proxy's recipe: it alone", "mesh-catalog", + []string{"modules/route-proxy/Dockerfile"}, read, "route-proxy", ""}, + {"the route proxy's manifest: it alone", "mesh-catalog", + []string{"modules/route-proxy/module.json"}, read, "route-proxy", ""}, + {"the route proxy's README: nothing", "mesh-catalog", + []string{"modules/route-proxy/README.md"}, read, "", "modules/route-proxy/README.md"}, + {"the build agent's manifest: it alone, nothing it builds", "mesh-catalog", + []string{"modules/build-agent/module.json"}, read, "build-agent", ""}, {"another module of the catalogue: neither", "mesh-catalog", - []string{"modules/gitea/index.ts"}, "gitea"}, + []string{"modules/gitea/index.ts"}, read, "gitea", ""}, } { - r, got := planMerge(t, c.repo, c.paths, entries, read, edges) + r, got := planMerge(t, c.repo, c.paths, entries, c.read, edges) if got != c.want { - t.Errorf("%s: planned %q, wanted %q (as today; issue 338)", c.what, got, c.want) + t.Errorf("%s: planned %q, wanted %q", c.what, got, c.want) } - if c.repo == "mesh-controller" && strings.Join(r.Unread, ",") != "" { - t.Errorf("%s: a root-built module reads every file, and %v were said unread", c.what, r.Unread) + if u := strings.Join(r.Unread, ","); u != c.unread { + t.Errorf("%s: unread %q, wanted %q", c.what, u, c.unread) } } + // Files not all said: everything the repository builds, as before. + m := link.SourceMoved{Owner: "novox", Repo: "mesh-controller", Base: "main", Commit: "head", + Paths: []string{"README.md"}, PathsTruncated: true} + if got := tiered(reachOfMerge(m, entries, read, edges).Plan.Tiers); got != "mesh-controller | build-agent | route-proxy" { + t.Errorf("a merge whose files were not all said: planned %q, wanted all three", got) + } +} + +// **A module an open plan has yet to build is read whole** (novox/hq ADR 0267): its build source is about to +// change. A merge that added an import to the route proxy is planned; before that build lands, a merge +// changing only the package newly imported must still move the proxy — the build source its last build +// said does not hold it yet. +func TestAModuleAPlanHasYetToBuildIsReadWhole(t *testing.T) { + read := map[string][]inventory.ReadRepository{ + "route-proxy": { + {Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"examples/route-proxy/", "go.mod"}}, + {Own: true, Paths: []string{"modules/route-proxy/module.json"}}, + }, + "mesh-controller": {{Own: true, Paths: []string{"module.json", "cmd/mesh-controller/"}}}, + } + open := []inventory.Plan{{State: inventory.PlanBuilding, Modules: map[string]*inventory.PlanModule{ + "route-proxy": {State: "building"}, "gitea": {State: "built"}}}} + pending := pendingIn(open) + if !pending["route-proxy"] || pending["gitea"] { + t.Fatalf("pending %v", pending) + } + m := link.SourceMoved{Owner: "novox", Repo: "mesh-controller", Base: "main", Paths: []string{"internal/newly/imported.go"}} + if readsFrom(read["route-proxy"], m) { + t.Fatal("the said build source holds the new package: the fixture is wrong") + } + whole := unnarrowed(read, pending) + if !readsFrom(whole["route-proxy"], m) { + t.Error("a module a plan has yet to build was read through the build source it is about to replace") + } + if ownSource(whole["route-proxy"]) != nil { + t.Error("a pending module kept its own build source") + } + if ownSource(whole["mesh-controller"]) == nil { + t.Error("a module no plan is building lost its build source") + } + // A closed plan holds nothing back. + if len(pendingIn([]inventory.Plan{{State: inventory.PlanDone, Modules: open[0].Modules}})) != 0 { + t.Error("a plan that is done held a module back") + } } // sharedRepositoryEdges is what dependenciesOf derives for the catalogue of the test above, sorted as it -// sorts them. +// sorts them: no packages edge (novox/hq ADR 0267 rule 4). var sharedRepositoryEdges = []inventory.Edge{ - dep("build-agent", inventory.EdgePackages, "mesh-controller"), dep("build-agent", inventory.EdgeWorkerOf, "mesh-controller"), dep("gitea", inventory.EdgeBuiltBy, "build-agent"), dep("mesh-controller", inventory.EdgeBuiltBy, "build-agent"), dep("route-proxy", inventory.EdgeBuiltBy, "build-agent"), - dep("route-proxy", inventory.EdgePackages, "mesh-controller"), } // **The planner's invariant, over random catalogues.** For any catalogue whose dependencies form no cycle // and any set of changed files in one repository: // // - the plan is exactly the modules of that repository whose directory holds a changed file (every file, -// for a module built from the root), and everything reachable from them along stands-on, declared and -// packages — never along built-by or worker-of; +// for a module built from the root), and everything reachable from them along stands-on and declared — +// never along packages (novox/hq ADR 0267), 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. @@ -310,7 +396,7 @@ var sharedRepositoryEdges = []inventory.Edge{ func TestAPlanIsTheTouchedModulesAndWhatIsReachableAlongTheWideningEdges(t *testing.T) { kinds := []string{inventory.EdgeStandsOn, inventory.EdgeDeclared, inventory.EdgePackages, inventory.EdgeBuiltBy, inventory.EdgeWorkerOf} - widens := map[string]bool{inventory.EdgeStandsOn: true, inventory.EdgeDeclared: true, inventory.EdgePackages: true} + widens := map[string]bool{inventory.EdgeStandsOn: true, inventory.EdgeDeclared: true} orders := map[string]bool{inventory.EdgeStandsOn: true, inventory.EdgeDeclared: true, inventory.EdgeBuiltBy: true, inventory.EdgeWorkerOf: true} // Directory names drawn from one pool, so two repositories hold directories of the same name, and one @@ -445,3 +531,43 @@ func describe(entries []inventory.Entry) []string { } return out } + +// **The merge gate reads the build sources the snapshot carries** (novox/hq ADR 0267): the gate's plan of a +// change is the merge handler's, so a snapshot taken by a controller that records build sources narrows the +// gate's plan as it narrows the merge's; one without them reads every module as before. +func TestTheGatePlansFromTheBuildSourcesTheSnapshotCarries(t *testing.T) { + manifest := func(name string) json.RawMessage { return json.RawMessage(`{"module":"` + name + `","version":"1"}`) } + facts := snapshot.Facts{Modules: []snapshot.Module{ + {Name: "mesh-controller", Repository: "novox/mesh-controller", Manifest: manifest("mesh-controller"), + Sources: []snapshot.BuildSource{{Own: true, Paths: []string{"module.json", "cmd/mesh-controller/", "internal/broker/"}}}}, + {Name: "route-proxy", Repository: "novox/mesh-catalog", Path: "modules/route-proxy", Manifest: manifest("route-proxy"), + Reads: []string{"novox/mesh-controller"}, + Sources: []snapshot.BuildSource{{Repository: "novox/mesh-controller", Paths: []string{"examples/route-proxy/", "internal/broker/"}}, + {Own: true, Paths: []string{"modules/route-proxy/module.json"}}}}, + }} + for paths, want := range map[string]string{ + "cmd/mesh-controller/main.go": "mesh-controller", + "examples/route-proxy/main.go": "route-proxy", + "internal/broker/broker.go": "mesh-controller,route-proxy", + "README.md": "", + } { + r, err := reachOfChange(facts, "novox/mesh-controller", []string{paths}, "") + if err != nil { + t.Fatal(err) + } + if got := tiered(r.Plan.Tiers); got != want { + t.Errorf("%s: the gate planned %q, wanted %q", paths, got, want) + } + } + // A snapshot without build sources: as before. + for i := range facts.Modules { + facts.Modules[i].Sources = nil + } + r, err := reachOfChange(facts, "novox/mesh-controller", []string{"README.md"}, "") + if err != nil { + t.Fatal(err) + } + if got := tiered(r.Plan.Tiers); got != "mesh-controller,route-proxy" { + t.Errorf("a snapshot without build sources: the gate planned %q for a README", got) + } +} diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 31c90340..039d5a22 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -138,9 +138,10 @@ func reachableFrom(moved []string, edges []inventory.Edge) []string { grew = false for _, e := range edges { // Built-by and worker-of order a plan; neither widens it. A new build machine changes - // nothing it builds, and a new controller changes nothing about the holder it orders — - // what packages the controller's source is already a code edge. - if e.Kind == inventory.EdgeBuiltBy || e.Kind == inventory.EdgeWorkerOf { + // nothing it builds, and a new controller changes nothing about the holder it orders. A + // packages edge, read from a record made before novox/hq ADR 0267, widens nothing either: + // a shared file moves each module whose build source holds it, directly. + if e.Kind == inventory.EdgeBuiltBy || e.Kind == inventory.EdgeWorkerOf || e.Kind == inventory.EdgePackages { continue } if in[e.To] && !in[e.From] { diff --git a/cmd/mesh-controller/release_plan_test.go b/cmd/mesh-controller/release_plan_test.go index 118a495b..d26bcf57 100644 --- a/cmd/mesh-controller/release_plan_test.go +++ b/cmd/mesh-controller/release_plan_test.go @@ -59,17 +59,18 @@ func TestAMergeIsPlannedInTiersAlongTheThreeKindsOfDependency(t *testing.T) { t.Fatalf("no cycle here: %v", tiers) } - // The controller alone moved: the proxy with it, nothing else. - small := reachableFrom([]string{"mesh-controller"}, edges) - if len(small) != 3 { - t.Fatalf("a controller merge rebuilds the controller and what packages it: %v", small) + // The controller alone moved: the controller alone — what packages its repository moves only when the + // change is in its own build source (novox/hq ADR 0267), so a packages edge widens nothing. + if alone := reachableFrom([]string{"mesh-controller"}, edges); len(alone) != 1 { + t.Fatalf("a controller merge rebuilds the controller alone: %v", alone) } - // The builder and the proxy package the controller's source, which orders nothing. The builder holds + // A change to a package all three build from moves all three. The builder holds // the build seat, whose worker the controller defines, so it follows the controller (worker-of, // novox/hq issue 206), and the controller's built-by edge to it yields: the controller is built by the // build machine that is running. The proxy is built by the new builder: the controller, the builder, // the proxy — the live plan of every controller merge. (This read "the builder, then the controller // and the proxy together" before issue 206, and the fixture had no worker-of edge.) + small := reachableFrom([]string{"mesh-controller", "builder", "route-proxy"}, edges) smallTiers := tiersOf(small, edges) if got := tiered(smallTiers); got != "mesh-controller | builder | route-proxy" { t.Fatalf("the controller, then the builder, then the proxy: %v", smallTiers) @@ -243,7 +244,7 @@ func TestAChangeToTheBuildAgentRebuildsTheBuildAgentAlone(t *testing.T) { } { m := link.SourceMoved{Owner: "novox", Repo: "mesh-catalog", Base: "main", Commit: "abc", Paths: paths, ModuleDirs: []string{"modules/build-agent"}, ModuleDirsSaid: true} - touched := whatTheMergeTouched(entries, entries, m) + touched := whatTheMergeTouched(entries, entries, m, nil) if len(touched) != 1 || touched[0].Manifest.Module != "build-agent" { t.Fatalf("%v touched %v", paths, touched) } diff --git a/cmd/mesh-controller/upgrades.go b/cmd/mesh-controller/upgrades.go index d0c2215c..3288559e 100644 --- a/cmd/mesh-controller/upgrades.go +++ b/cmd/mesh-controller/upgrades.go @@ -12,6 +12,7 @@ import ( "sync" "time" + "github.com/novox/mesh-controller/internal/builder" "github.com/novox/mesh-controller/internal/catalogue" "github.com/novox/mesh-controller/internal/inventory" "github.com/novox/mesh-controller/internal/link" @@ -313,7 +314,7 @@ func (f following) SourceMoved(ctx context.Context, m link.SourceMoved) error { if err != nil { return notNow(err) } - read, err := inv.ReadRepositories(ctx) + read, err := readForAMerge(ctx, inv) if err != nil { return notNow(err) } @@ -360,7 +361,7 @@ func (f following) SourceMoved(ctx context.Context, m link.SourceMoved) error { e.Manifest.Module, m.Owner, m.Repo, e.Source.Ref, m.Base) } } - touched, added, _ := touchedBy(from, entries, m) + touched, added, _ := touchedBy(from, entries, m, read) // **A module the merge deleted is not built** (novox/hq ADR 0236): its manifest is gone, so the build // seat finds nothing saying what it is, and the plan failed on it (`has no module.json at …`) with // every other module of its tier left unsent. It is forgotten where nothing holds it, said otherwise. @@ -587,7 +588,7 @@ func mergeCandidates(m link.SourceMoved, entries []inventory.Entry, func wouldMove(m link.SourceMoved, entries []inventory.Entry, read map[string][]inventory.ReadRepository) []inventory.Entry { from, _, _ := mergeCandidates(m, entries, read) - touched, _ := splitDeleted(whatTheMergeTouched(from, entries, m), m) + touched, _ := splitDeleted(whatTheMergeTouched(from, entries, m, read), m) return touched } @@ -672,18 +673,112 @@ func sameRepository(repository string, m link.SourceMoved) bool { (m.CloneURL != "" && repo == strings.ToLower(strings.TrimSuffix(m.CloneURL, ".git"))) } -// readsFrom is whether a module's build read the repository a merge names: the second repository its -// recipe packages source from. Its ref must be the branch that moved, or unset — the same rule a -// module's own source follows. +// readsFrom is whether a merge changed what a module's build read in another repository: the second +// repository its recipe packages source from. Its ref must be the branch that moved, or unset — the same +// rule a module's own source follows. +// +// **Only a changed file in what the build read there** (novox/hq ADR 0267 rule 2): where the module's +// newest trunk build said its build source in that repository, a merge touching none of it is no change +// to the module (issue 338: every merge to the controller's repository moved the route proxy and the +// build seat's holder). Where it said none, or the merge's files are not all said, the whole repository is +// read, as before. func readsFrom(read []inventory.ReadRepository, m link.SourceMoved) bool { for _, r := range read { - if sameRepository(r.Repository, m) && (r.Ref == "" || r.Ref == m.Base) { + if r.Own || !sameRepository(r.Repository, m) || (r.Ref != "" && r.Ref != m.Base) { + continue + } + if len(r.Paths) == 0 || len(m.Paths) == 0 || m.PathsTruncated { return true } + for _, p := range m.Paths { + if builder.SourceHolds(r.Paths, p) { + return true + } + } } return false } +// ownSource is the build source a module's newest trunk build said it read in its own repository; nil +// when it said none, and the module's own directory — or, built from the root, its whole repository — is +// its build source, as before (novox/hq ADR 0267). +func ownSource(read []inventory.ReadRepository) []string { + for _, r := range read { + if r.Own && len(r.Paths) > 0 { + return r.Paths + } + } + return nil +} + +// readsFile is whether a module built from the merged repository reads one of its changed files: in its +// build source where its newest trunk build said one, else anywhere in its directory, or anywhere at all +// for a module built from the repository's root. +func readsFile(e inventory.Entry, read []inventory.ReadRepository, p string) bool { + if own := ownSource(read); own != nil { + return builder.SourceHolds(own, p) + } + return strings.Trim(e.Source.Path, "/") == "" || inside(p, e.Source.Path) +} + +// pendingIn is the modules an open plan has yet to build (novox/hq ADR 0267): what such a module was built +// from is about to change, so the build source its last build said is not the one a merge now meets — an +// earlier merge that added an import to it would otherwise hide a change to what that import names. Each +// is read whole, as before, and planned again with the merge; the newer plan supersedes the older. +func pendingIn(plans []inventory.Plan) map[string]bool { + pending := map[string]bool{} + for _, p := range plans { + if !p.Open() { + continue + } + for name, s := range p.Modules { + if s == nil || (s.State != "built" && s.State != planDeleted) { + pending[name] = true + } + } + } + return pending +} + +// readForAMerge is what each module's build read, as a merge acting now maps its files onto: the build +// sources the newest trunk builds said, but for the modules an open plan has yet to build (pendingIn). +func readForAMerge(ctx context.Context, inv *inventory.Inventory) (map[string][]inventory.ReadRepository, error) { + read, err := inv.ReadRepositories(ctx) + if err != nil { + return nil, err + } + open, err := inv.OpenPlans(ctx) + if err != nil { + return nil, err + } + return unnarrowed(read, pendingIn(open)), nil +} + +// unnarrowed is what modules read, with the build sources of the pending ones set aside: each of them is +// read whole. +func unnarrowed(read map[string][]inventory.ReadRepository, pending map[string]bool) map[string][]inventory.ReadRepository { + if len(pending) == 0 { + return read + } + out := make(map[string][]inventory.ReadRepository, len(read)) + for name, rs := range read { + if !pending[name] { + out[name] = rs + continue + } + var whole []inventory.ReadRepository + for _, r := range rs { + if r.Own { + continue + } + r.Paths = nil + whole = append(whole, r) + } + out[name] = whole + } + return out +} + // lastLookAt is the most recent look at this repository by anything built from it. func lastLookAt(entries []inventory.Entry, m link.SourceMoved) time.Time { var newest time.Time @@ -698,8 +793,9 @@ func lastLookAt(entries []inventory.Entry, m link.SourceMoved) time.Time { // whatTheMergeTouched narrows the modules built from a repository to the ones the merge changed: **a // changed file touches exactly the modules whose build reads it** (novox/hq issue 280, ADR 0238). It is // touchedBy's first answer; touchedBy is the one place the mesh maps a changed file onto its modules. -func whatTheMergeTouched(candidates, known []inventory.Entry, m link.SourceMoved) []inventory.Entry { - touched, _, _ := touchedBy(candidates, known, m) +func whatTheMergeTouched(candidates, known []inventory.Entry, m link.SourceMoved, + read map[string][]inventory.ReadRepository) []inventory.Entry { + touched, _, _ := touchedBy(candidates, known, m, read) return touched } @@ -707,8 +803,11 @@ func whatTheMergeTouched(candidates, known []inventory.Entry, m link.SourceMoved // merge handler, the release planner's what-if, the merge gate and a pull request's check alike (novox/hq // ADR 0238), so planning and gating cannot disagree about what a change touches. // -// **A changed file touches exactly the modules whose build reads it.** What a build reads is the module's -// own directory — the builder clones the repository and builds within that directory alone: the manifest, +// **A changed file touches exactly the modules whose build reads it.** What a build reads is its build +// source, where the module's newest trunk build said one (novox/hq ADR 0267): a Go program's import closure, +// an archive's directory, a recipe, its manifest — so a README at the root of a repository whose module is +// built from its root, or another program's package beside it, touches nothing. Where none was said, it is +// the module's own directory — the builder clones the repository and builds within that directory alone: the manifest, // the recipes, the bundles' sources, the Docker context — or the whole repository for a module built from // its root. A second repository a recipe packages (an artifact's `context`) is read too; that is the build // record's `read`, answered by readsFrom in mergeCandidates. So a changed file inside a module's directory @@ -728,7 +827,8 @@ func whatTheMergeTouched(candidates, known []inventory.Entry, m link.SourceMoved // // Nothing said about the files, or not all of them said, is still everything: what is not known cannot // be narrowed. -func touchedBy(candidates, known []inventory.Entry, m link.SourceMoved) (touched []inventory.Entry, added, unread []string) { +func touchedBy(candidates, known []inventory.Entry, m link.SourceMoved, + read map[string][]inventory.ReadRepository) (touched []inventory.Entry, added, unread []string) { knownDirs := map[string]bool{} for _, e := range known { if !e.Provided && sameRepository(e.Source.Repository, m) { @@ -763,19 +863,27 @@ func touchedBy(candidates, known []inventory.Entry, m link.SourceMoved) (touched return candidates, added, nil } for _, e := range candidates { - if strings.Trim(e.Source.Path, "/") == "" || anyInside(m.Paths, e.Source.Path) { - touched = append(touched, e) + for _, p := range m.Paths { + if readsFile(e, read[e.Manifest.Module], p) { + touched = append(touched, e) + break + } } } for _, p := range m.Paths { - read := newDir["."] + isRead := newDir["."] for _, e := range candidates { - read = read || strings.Trim(e.Source.Path, "/") == "" || inside(p, e.Source.Path) + isRead = isRead || readsFile(e, read[e.Manifest.Module], p) } for d := range newDir { - read = read || inside(p, d) + isRead = isRead || inside(p, d) } - if !read { + // A file a module packages from this repository is read too, by that module's build. + for _, e := range known { + isRead = isRead || readsFrom(read[e.Manifest.Module], link.SourceMoved{Owner: m.Owner, Repo: m.Repo, + Base: m.Base, CloneURL: m.CloneURL, Paths: []string{p}}) + } + if !isRead { unread = append(unread, p) } } @@ -834,7 +942,7 @@ func (r mergeReach) Dependents() []string { func reachOfMerge(m link.SourceMoved, entries []inventory.Entry, read map[string][]inventory.ReadRepository, edges []inventory.Edge) mergeReach { from, packaging, already := mergeCandidates(m, entries, read) - touched, added, unread := touchedBy(from, entries, m) + touched, added, unread := touchedBy(from, entries, m, read) kept, deleted := splitDeleted(touched, m) r := mergeReach{Touched: kept, Deleted: deleted, Packaging: packaging, Already: already, Added: added, Unread: unread} var building []string diff --git a/cmd/mesh-controller/worker_order_test.go b/cmd/mesh-controller/worker_order_test.go index f5b43f20..c633ca55 100644 --- a/cmd/mesh-controller/worker_order_test.go +++ b/cmd/mesh-controller/worker_order_test.go @@ -19,10 +19,12 @@ func TestTheBuildSeatsHolderFollowsTheControllerThatDefinesItsWorker(t *testing. {From: "route-proxy", To: "mesh-controller", Kind: inventory.EdgePackages}, {From: "route-proxy", To: "build-agent", Kind: inventory.EdgeBuiltBy}, } - set := reachableFrom([]string{"mesh-controller"}, edges) - if len(set) != 3 { - t.Fatalf("the controller, what packages it, and nothing more: %v", set) + // A packages edge recorded before novox/hq ADR 0267 widens nothing: the controller moved alone moves + // alone, and a change to a package all three build from moves all three, each by its own build source. + if alone := reachableFrom([]string{"mesh-controller"}, edges); len(alone) != 1 { + t.Fatalf("the controller alone, whatever packages its repository: %v", alone) } + set := reachableFrom([]string{"mesh-controller", "build-agent", "route-proxy"}, edges) tiers := tiersOf(set, edges) pos := map[string]int{} for i, tier := range tiers { diff --git a/internal/facts/facts.go b/internal/facts/facts.go index e010916d..be55f445 100644 --- a/internal/facts/facts.go +++ b/internal/facts/facts.go @@ -226,10 +226,21 @@ type Module struct { RollOut bool `json:"roll-out,omitempty"` // Reads are the other repositories its build read source from. Reads []string `json:"reads,omitempty"` + // Sources are the build source its newest build said it read, per repository (novox/hq ADR 0267): a + // context of Reads by name with its paths, or the module's own repository (Own). Absent, the module's + // build reads every file of each of Reads and of its own directory, as before. + Sources []BuildSource `json:"sources,omitempty"` // Manifest is the module as the mesh holds it: artifacts resolved to the builds it runs. Manifest json.RawMessage `json:"manifest"` } +// BuildSource is the build source a module's build read in one repository (novox/hq ADR 0267). +type BuildSource struct { + Repository string `json:"repository,omitempty"` + Own bool `json:"own,omitempty"` + Paths []string `json:"paths"` +} + // Edge is one build dependency: From is built standing on To. type Edge struct { From string `json:"from"` diff --git a/internal/inventory/builds.go b/internal/inventory/builds.go index 08a48235..178c32d8 100644 --- a/internal/inventory/builds.go +++ b/internal/inventory/builds.go @@ -45,6 +45,9 @@ type Build struct { // Read is every repository this build read source from besides the module's own (novox/hq // 04-ISSUES/131), at the ref it read. Read []ReadRepository + // Sources are what the build was made from, as files, per repository (novox/hq ADR 0267): said by the + // builder for a build of a trunk commit; nil where it said none. + Sources []BuildSource // SourceFingerprint is what the build was made from, hashed, as its builder said it (novox/hq // issue 280); empty from a builder that predates it, or where the source does not pin the build. SourceFingerprint string @@ -75,9 +78,27 @@ func (b Build) AskedOrAt() time.Time { const newestRequestFirst = `coalesce(asked, at) desc, at desc` // ReadRepository is a repository a build read source from besides the module's own. +// +// Paths and Own are never stored in a build's `built_contexts` (a controller that predates them would read +// an own entry as a context, and every module of a repository as packaging every other): ReadRepositories +// lays them over what it reads, from the build's build sources (novox/hq ADR 0267). type ReadRepository struct { Repository string `json:"repository"` Ref string `json:"ref,omitempty"` + // Paths are the build source the build read in this repository (builder.SourceHolds): a changed file + // outside them is no change to the module. Empty is the whole repository, as before. + Paths []string `json:"paths,omitempty"` + // Own is the module's own repository, whose Paths narrow what its own directory — or, for a module + // built from its repository's root, the whole repository — would otherwise be. + Own bool `json:"own,omitempty"` +} + +// BuildSource is the build source a build read in one repository (novox/hq ADR 0267): Repository and Ref +// a context's, empty for the module's own. +type BuildSource struct { + Repository string `json:"repository,omitempty"` + Ref string `json:"ref,omitempty"` + Paths []string `json:"paths"` } // Artifact is one thing a build published. @@ -108,6 +129,14 @@ func (i *Inventory) RecordBuild(ctx context.Context, b Build) error { if err != nil { return err } + var sources any + if len(b.Sources) > 0 { + raw, err := json.Marshal(b.Sources) + if err != nil { + return err + } + sources = raw + } var module *string if b.Module != "" { module = &b.Module @@ -118,11 +147,12 @@ func (i *Inventory) RecordBuild(ctx context.Context, b Build) error { } _, err = i.store.Pool().Exec(ctx, `insert into build (id, repository, ref, module, commit_hash, built_on, failed, made, - source_path, manifest, built_against, built_contexts, asked, source_fingerprint) - values ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) + source_path, manifest, built_against, built_contexts, asked, source_fingerprint, + build_sources) + values ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) on conflict (id) do nothing`, b.ID, b.Repository, b.Ref, module, b.Commit, b.On, b.Failed, made, - b.Path, manifestOrNil(b.Manifest), against, read, asked, b.SourceFingerprint) + b.Path, manifestOrNil(b.Manifest), against, read, asked, b.SourceFingerprint, sources) if err != nil { return err } @@ -302,9 +332,14 @@ func (i *Inventory) BuiltAgainst(ctx context.Context) (map[string][]string, erro // The mirror of BuiltAgainst, and derived the same way and for the same reason: a merge into a // repository a module only packages is a change to that module, and the manifest the mesh keeps // carries nothing that would say so (novox/hq 04-ISSUES/131). +// +// **With the build source that build said** (novox/hq ADR 0267): a context's entry carries the paths the +// build read there, and the module's own repository an entry marked Own with its paths. A build that said +// none — off the trunk, from a builder that predates it, or of a source not known — leaves its module read +// as before: every file of a context, and its own directory or root. func (i *Inventory) ReadRepositories(ctx context.Context) (map[string][]ReadRepository, error) { rows, err := i.store.Pool().Query(ctx, - `select distinct on (module) module, built_contexts + `select distinct on (module) module, built_contexts, build_sources from build where module is not null and module <> '' and failed = '' order by module, `+newestRequestFirst) @@ -316,24 +351,52 @@ func (i *Inventory) ReadRepositories(ctx context.Context) (map[string][]ReadRepo read := map[string][]ReadRepository{} for rows.Next() { var module string - var raw []byte - if err := rows.Scan(&module, &raw); err != nil { + var raw, rawSources []byte + if err := rows.Scan(&module, &raw, &rawSources); err != nil { return nil, err } - if len(raw) == 0 { - continue - } var of []ReadRepository - if err := json.Unmarshal(raw, &of); err != nil { - continue + if len(raw) > 0 { + if err := json.Unmarshal(raw, &of); err != nil { + of = nil + } } - if len(of) > 0 { + var sources []BuildSource + if len(rawSources) > 0 { + if err := json.Unmarshal(rawSources, &sources); err != nil { + sources = nil + } + } + if of = WithBuildSources(of, sources); len(of) > 0 { read[module] = of } } return read, rows.Err() } +// WithBuildSources lays a build's build sources over the repositories it read: a context's paths on its +// entry, and the module's own as an entry of its own. A context the build read that its sources do not +// name stays whole; a source naming a context the build did not say it read is dropped, since nothing +// moves a module through a repository it is not recorded as reading. +func WithBuildSources(read []ReadRepository, sources []BuildSource) []ReadRepository { + out := make([]ReadRepository, 0, len(read)+1) + for _, r := range read { + r.Paths, r.Own = nil, false + for _, s := range sources { + if s.Repository != "" && s.Repository == r.Repository && s.Ref == r.Ref && len(s.Paths) > 0 { + r.Paths = append([]string(nil), s.Paths...) + } + } + out = append(out, r) + } + for _, s := range sources { + if s.Repository == "" && len(s.Paths) > 0 { + out = append(out, ReadRepository{Ref: s.Ref, Paths: append([]string(nil), s.Paths...), Own: true}) + } + } + return out +} + // manifestOrNil keeps the difference between "declared nothing" and "predates this being kept". // // A build recorded before the mesh kept manifests has no manifest, and that is not the same as one diff --git a/internal/inventory/builds_test.go b/internal/inventory/builds_test.go index a2513335..fda48b9e 100644 --- a/internal/inventory/builds_test.go +++ b/internal/inventory/builds_test.go @@ -2,6 +2,7 @@ package inventory import ( "context" + "reflect" "strings" "testing" ) @@ -169,3 +170,47 @@ func TestWhatABuildReadComesBackForTheNewestBuildOfEachModule(t *testing.T) { t.Fatal("a failed build's reading was kept as what that module reads") } } + +// **What a build said it was made from comes back laid over what it read** (novox/hq ADR 0267): a context's +// paths on its entry, the module's own as an entry marked Own — and never in the stored `built_contexts`, +// where a controller that predates them would read an own entry as a context of its own repository. +func TestABuildsBuildSourceComesBackOverWhatItRead(t *testing.T) { + inv := fresh(t) + ctx := context.Background() + said := aBuild("said", "route-proxy", "") + said.Read = []ReadRepository{{Repository: "novox/mesh-controller", Ref: "main"}, {Repository: "novox/other"}} + said.Sources = []BuildSource{ + {Paths: []string{"modules/route-proxy/Dockerfile", "modules/route-proxy/module.json"}}, + {Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"examples/route-proxy/", "go.mod"}}, + {Repository: "novox/unread", Paths: []string{"x"}}, + } + unsaid := aBuild("unsaid", "build-agent", "") + unsaid.Read = []ReadRepository{{Repository: "novox/mesh-controller", Ref: "main"}} + for _, b := range []Build{said, unsaid} { + if err := inv.RecordBuild(ctx, b); err != nil { + t.Fatal(err) + } + } + read, err := inv.ReadRepositories(ctx) + if err != nil { + t.Fatal(err) + } + want := []ReadRepository{ + {Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"examples/route-proxy/", "go.mod"}}, + {Repository: "novox/other"}, + {Paths: []string{"modules/route-proxy/Dockerfile", "modules/route-proxy/module.json"}, Own: true}, + } + if !reflect.DeepEqual(read["route-proxy"], want) { + t.Fatalf("read back %+v\nwanted %+v", read["route-proxy"], want) + } + if got := read["build-agent"]; len(got) != 1 || got[0].Paths != nil || got[0].Own { + t.Fatalf("a build that said no build source reads as %+v", got) + } + var stored string + if err := inv.store.Pool().QueryRow(ctx, `select built_contexts::text from build where id = 'said'`).Scan(&stored); err != nil { + t.Fatal(err) + } + if strings.Contains(stored, "paths") || strings.Contains(stored, "own") { + t.Fatalf("the build source was stored among what the build read: %s", stored) + } +} diff --git a/internal/inventory/dependencies.go b/internal/inventory/dependencies.go index 7384ceca..26d07385 100644 --- a/internal/inventory/dependencies.go +++ b/internal/inventory/dependencies.go @@ -13,7 +13,10 @@ import ( const ( // EdgeStandsOn: the module's artifact is built on the other's. EdgeStandsOn = "stands-on" - // EdgePackages: the module's build reads the other's repository. + // EdgePackages: the module's build read the other's repository. **No longer drawn** (novox/hq ADR 0267 + // rule 4): sharing a repository is no dependency, and a changed file moves every module whose build + // source holds it, directly. Kept so an edge recorded or snapshotted before is read, and then neither + // widens nor orders a plan. EdgePackages = "packages" // EdgeBuiltBy: the module is built by the holder of the build-machine seat. EdgeBuiltBy = "built-by" @@ -42,10 +45,10 @@ type Edge struct { // nothing else computes an edge (novox/hq ADR 0162) — the merge handler, `build --on` and the // overview all read this. // -// Four sources, one relation: a manifest's `build.on`; the artifacts the latest build was made -// against (an `artifact-store:///…` reference is an edge to that module); the repositories -// the latest build read (an edge to the module whose source that is); and the build machine, which -// every source-built module is built by. +// Three sources, one relation: a manifest's `build.on`; the artifacts the latest build was made +// against (an `artifact-store:///…` reference is an edge to that module); and the build machine, +// which every source-built module is built by. The repositories a build read are no edge (novox/hq ADR +// 0267 rule 4). func (i *Inventory) Dependencies(ctx context.Context) ([]Edge, error) { entries, err := i.Catalogued(ctx) if err != nil { @@ -55,24 +58,19 @@ func (i *Inventory) Dependencies(ctx context.Context) ([]Edge, error) { if err != nil { return nil, err } - read, err := i.ReadRepositories(ctx) - if err != nil { - return nil, err - } - return dependenciesOf(entries, against, read), nil + return dependenciesOf(entries, against, nil), nil } // dependenciesOf is Dependencies over what was read, so a test can hand it a catalogue. -func dependenciesOf(entries []Entry, against map[string][]string, read map[string][]ReadRepository) []Edge { +// +// `read` draws no edge (novox/hq ADR 0267 rule 4): what a build read moves it through its build source, in +// the planner, never through the relation. +func dependenciesOf(entries []Entry, against map[string][]string, _ map[string][]ReadRepository) []Edge { known := map[string]bool{} - byRepository := map[string][]string{} var builders []string for _, e := range entries { name := e.Manifest.Module known[name] = true - if r := repositoryKey(e.Source.Repository); r != "" { - byRepository[r] = append(byRepository[r], name) - } if e.Manifest.ClaimsSeat("node-build-agent") || e.Manifest.ClaimsSeat("mesh-build-machine") { builders = append(builders, name) } @@ -125,11 +123,6 @@ func dependenciesOf(entries []Entry, against map[string][]string, read map[strin } } } - for _, r := range read[name] { - for _, other := range byRepository[repositoryKey(r.Repository)] { - add(name, other, EdgePackages) - } - } if e.Source.Repository != "" { for _, b := range builders { add(name, b, EdgeBuiltBy) @@ -154,8 +147,3 @@ func dependenciesOf(entries []Entry, against map[string][]string, read map[strin }) return out } - -// repositoryKey is a repository as compared: lower-cased, without a trailing `.git`. -func repositoryKey(repository string) string { - return strings.ToLower(strings.TrimSuffix(strings.TrimSpace(repository), ".git")) -} diff --git a/internal/inventory/dependencies_test.go b/internal/inventory/dependencies_test.go index cda50102..39286193 100644 --- a/internal/inventory/dependencies_test.go +++ b/internal/inventory/dependencies_test.go @@ -44,7 +44,6 @@ func TestDependenciesAreOneRelationWithTheirKinds(t *testing.T) { {"shop", "mesh-tools", EdgeStandsOn}, {"builder", "mesh-tools", EdgeStandsOn}, {"shop-plugin", "shop", EdgeDeclared}, - {"route-proxy", "mesh-controller", EdgePackages}, {"shop", "builder", EdgeBuiltBy}, {"mesh-controller", "builder", EdgeBuiltBy}, {"mesh-tools", "builder", EdgeBuiltBy}, @@ -53,6 +52,10 @@ func TestDependenciesAreOneRelationWithTheirKinds(t *testing.T) { t.Errorf("missing %+v in %+v", want, got) } } + // Sharing a repository is no dependency (novox/hq ADR 0267 rule 4): what a build read draws no edge. + if has("route-proxy", "mesh-controller", EdgePackages) { + t.Error("a packages edge was drawn from what a build read") + } if has("builder", "builder", EdgeBuiltBy) { t.Error("the builder is not built by itself") } diff --git a/internal/inventory/migrations/0088-a-build-says-its-build-source.sql b/internal/inventory/migrations/0088-a-build-says-its-build-source.sql new file mode 100644 index 00000000..319ae571 --- /dev/null +++ b/internal/inventory/migrations/0088-a-build-says-its-build-source.sql @@ -0,0 +1,9 @@ +-- A build says what it was made from, as files (novox/hq ADR 0267, issue 363). +-- +-- A merge to a repository moved every module whose build read it, whatever the files: the controller, +-- built from its repository's root, and the two images whose context is that repository (issue 338). +-- A build of a trunk commit now says its build source per repository — a Go program's import closure, +-- an archive's directory, an image's recipe — and the planner maps the next merge's changed files onto +-- the build source of the module's newest build. Null for a build that said none (one off the trunk, +-- one from a builder that predates this, one whose source is not known): its module is read as before. +alter table build add column build_sources jsonb; From 150f038ff8c4f1eb1fc014a061b692aef815a51a Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 03:16:37 +0200 Subject: [PATCH 2/3] Read a module whole while a plan has overtaken its build source, and plan every view alike (hq ADR 0267, review) A failed build, or a plan closed before reaching a module, left the closure its last good build said, and a fix-forward to a newly imported package would have moved nothing. A missed merge moving only a module that packages the repository was never caught up, and an older merge read as history for it through a look that was not its own. The gate, a pull request's check, the what-if and a delivery's order now read the same view the merge handler does. --- cmd/mesh-controller/checks.go | 2 +- cmd/mesh-controller/delivery.go | 2 +- cmd/mesh-controller/facts.go | 2 +- cmd/mesh-controller/missed_merges.go | 2 +- cmd/mesh-controller/planner_rules_test.go | 85 ++++++++---- cmd/mesh-controller/release_plan.go | 2 +- cmd/mesh-controller/upgrades.go | 151 +++++++++++++--------- internal/inventory/builds.go | 48 ++++++- internal/inventory/builds_test.go | 33 +++++ 9 files changed, 235 insertions(+), 92 deletions(-) diff --git a/cmd/mesh-controller/checks.go b/cmd/mesh-controller/checks.go index 478fceab..2fdfbddd 100644 --- a/cmd/mesh-controller/checks.go +++ b/cmd/mesh-controller/checks.go @@ -143,7 +143,7 @@ func (f following) PullUpdated(ctx context.Context, p link.PullUpdated) error { if err != nil { return err } - read, err := inv.ReadRepositories(ctx) + read, err := readForPlanning(ctx, inv) if err != nil { return err } diff --git a/cmd/mesh-controller/delivery.go b/cmd/mesh-controller/delivery.go index 4bffcb9c..b9bd0161 100644 --- a/cmd/mesh-controller/delivery.go +++ b/cmd/mesh-controller/delivery.go @@ -604,7 +604,7 @@ func theGraph(ctx context.Context, inv *inventory.Inventory) ([]inventory.Entry, if err != nil { return nil, nil, nil, err } - read, err := inv.ReadRepositories(ctx) + read, err := readForPlanning(ctx, inv) if err != nil { return nil, nil, nil, err } diff --git a/cmd/mesh-controller/facts.go b/cmd/mesh-controller/facts.go index 2d0cda96..95a99bc8 100644 --- a/cmd/mesh-controller/facts.go +++ b/cmd/mesh-controller/facts.go @@ -205,7 +205,7 @@ func gatherFacts(ctx context.Context, open *stores, busVersion string) (snapshot if err != nil { return snapshot.Facts{}, err } - read, err := inv.ReadRepositories(ctx) + read, err := readForPlanning(ctx, inv) if err != nil { return snapshot.Facts{}, err } diff --git a/cmd/mesh-controller/missed_merges.go b/cmd/mesh-controller/missed_merges.go index fb43871f..6428a2a7 100644 --- a/cmd/mesh-controller/missed_merges.go +++ b/cmd/mesh-controller/missed_merges.go @@ -48,7 +48,7 @@ func catchingUpOnMerges(ctx context.Context, open *stores, announced merges) { if err != nil { return nil, nil, err } - read, err := readForAMerge(ctx, open.inventory) + read, err := readForPlanning(ctx, open.inventory) return entries, read, err } failing := "" diff --git a/cmd/mesh-controller/planner_rules_test.go b/cmd/mesh-controller/planner_rules_test.go index 1aab2260..8f591813 100644 --- a/cmd/mesh-controller/planner_rules_test.go +++ b/cmd/mesh-controller/planner_rules_test.go @@ -7,6 +7,7 @@ import ( "sort" "strings" "testing" + "time" snapshot "github.com/novox/mesh-controller/internal/facts" "github.com/novox/mesh-controller/internal/inventory" @@ -335,41 +336,77 @@ func TestASharedRepositoryMovesOnlyWhatItsBuildSourceHolds(t *testing.T) { } } -// **A module an open plan has yet to build is read whole** (novox/hq ADR 0267): its build source is about to -// change. A merge that added an import to the route proxy is planned; before that build lands, a merge -// changing only the package newly imported must still move the proxy — the build source its last build -// said does not hold it yet. -func TestAModuleAPlanHasYetToBuildIsReadWhole(t *testing.T) { +// **A module whose recorded build source a plan has overtaken is read whole** (novox/hq ADR 0267): a merge +// that added an import to the route proxy is planned; before a build of it works — still building, failed, +// or its plan closed before reaching it — a merge changing only the newly imported package must still move +// the proxy, since the build source its last build said does not hold that package. +func TestAModuleAPlanOvertookIsReadWhole(t *testing.T) { + built := time.Date(2026, 10, 10, 1, 0, 0, 0, time.UTC) read := map[string][]inventory.ReadRepository{ "route-proxy": { - {Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"examples/route-proxy/", "go.mod"}}, - {Own: true, Paths: []string{"modules/route-proxy/module.json"}}, + {Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"examples/route-proxy/", "go.mod"}, Built: built}, + {Own: true, Paths: []string{"modules/route-proxy/module.json"}, Built: built}, }, - "mesh-controller": {{Own: true, Paths: []string{"module.json", "cmd/mesh-controller/"}}}, - } - open := []inventory.Plan{{State: inventory.PlanBuilding, Modules: map[string]*inventory.PlanModule{ - "route-proxy": {State: "building"}, "gitea": {State: "built"}}}} - pending := pendingIn(open) - if !pending["route-proxy"] || pending["gitea"] { - t.Fatalf("pending %v", pending) + "mesh-controller": {{Own: true, Paths: []string{"module.json", "cmd/mesh-controller/"}, Built: built}}, } m := link.SourceMoved{Owner: "novox", Repo: "mesh-controller", Base: "main", Paths: []string{"internal/newly/imported.go"}} if readsFrom(read["route-proxy"], m) { t.Fatal("the said build source holds the new package: the fixture is wrong") } - whole := unnarrowed(read, pending) - if !readsFrom(whole["route-proxy"], m) { - t.Error("a module a plan has yet to build was read through the build source it is about to replace") + proxy := func(state string) map[string]*inventory.PlanModule { + return map[string]*inventory.PlanModule{"route-proxy": {State: state}, "gitea": {State: "built"}} } - if ownSource(whole["route-proxy"]) != nil { - t.Error("a pending module kept its own build source") + for _, c := range []struct { + what string + plans []inventory.Plan + whole bool + }{ + {"no plan", nil, false}, + {"a plan still building it", []inventory.Plan{{State: inventory.PlanBuilding, Created: built.Add(-time.Hour), + Modules: proxy("building")}}, true}, + {"a plan made after its build that failed before building it", []inventory.Plan{{State: "failed", + Created: built.Add(time.Minute), Modules: proxy("waiting")}}, true}, + {"a plan made after its build that built it", []inventory.Plan{{State: inventory.PlanDone, + Created: built.Add(time.Minute), Modules: proxy("built")}}, false}, + {"a plan closed before its build", []inventory.Plan{{State: "failed", Created: built.Add(-time.Hour), + Modules: proxy("waiting")}}, false}, + } { + view := planningView(read, c.plans) + if readsFrom(view["route-proxy"], m) != c.whole { + t.Errorf("%s: read whole %v, wanted %v", c.what, !c.whole, c.whole) + } + if (ownSource(view["route-proxy"]) == nil) != c.whole { + t.Errorf("%s: its own build source kept %v", c.what, ownSource(view["route-proxy"]) != nil) + } + if ownSource(view["mesh-controller"]) == nil { + t.Errorf("%s: a module no plan holds lost its build source", c.what) + } } - if ownSource(whole["mesh-controller"]) == nil { - t.Error("a module no plan is building lost its build source") +} + +// **A missed merge that moves only a module packaging the repository is acted on** (novox/hq ADR 0267, +// issue 266): the catch-up asks wouldMove, which counts it; once a plan or build of it is made after the +// merge, the merge is history for it, and the catch-up leaves it. +func TestAMissedMergeMovingOnlyAPackagingModuleIsActedOnOnce(t *testing.T) { + merged := time.Date(2026, 10, 10, 1, 0, 0, 0, time.UTC) + entries := []inventory.Entry{ + fromRepo("mesh-controller", "http://forge.internal:20000/novox/mesh-controller.git", ""), + fromRepo("route-proxy", "http://forge.internal:20000/novox/mesh-catalog.git", "modules/route-proxy"), } - // A closed plan holds nothing back. - if len(pendingIn([]inventory.Plan{{State: inventory.PlanDone, Modules: open[0].Modules}})) != 0 { - t.Error("a plan that is done held a module back") + read := map[string][]inventory.ReadRepository{ + "mesh-controller": {{Own: true, Paths: []string{"module.json", "cmd/mesh-controller/"}, Built: merged.Add(-time.Hour)}}, + "route-proxy": {{Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"examples/route-proxy/"}, + Built: merged.Add(-time.Hour), Looked: merged.Add(-time.Hour)}}, + } + m := link.SourceMoved{Owner: "novox", Repo: "mesh-controller", Base: "main", Commit: "c1", + MergedAt: merged.Format(time.RFC3339), Paths: []string{"examples/route-proxy/main.go"}} + 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), + 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) } } diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 039d5a22..05fc4773 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -1562,7 +1562,7 @@ func planWhatIf(ctx context.Context, inv *inventory.Inventory, repository string if err != nil { return err } - read, err := inv.ReadRepositories(ctx) + read, err := readForPlanning(ctx, inv) if err != nil { return err } diff --git a/cmd/mesh-controller/upgrades.go b/cmd/mesh-controller/upgrades.go index 3288559e..9a41fe48 100644 --- a/cmd/mesh-controller/upgrades.go +++ b/cmd/mesh-controller/upgrades.go @@ -314,7 +314,7 @@ func (f following) SourceMoved(ctx context.Context, m link.SourceMoved) error { if err != nil { return notNow(err) } - read, err := readForAMerge(ctx, inv) + read, err := readForPlanning(ctx, inv) if err != nil { return notNow(err) } @@ -347,12 +347,6 @@ func (f following) SourceMoved(ctx context.Context, m link.SourceMoved) error { m.Owner, m.Repo, m.Base, m.Commit) return nil } - // The same judgement for the packaging kind, against the newest look at that repository by - // anything built from it: they keep no record of it themselves, and a replayed old merge should - // not rebuild them either. - if isHistory(m.MergedAt, lastLookAt(entries, m)) { - packaging = nil - } // Said, never silent (novox/hq 04-ISSUES/215): a module built from this repository that follows // another branch is not part of this merge, and whoever is waiting for its change should read why. for _, e := range entries { @@ -571,25 +565,43 @@ func mergeCandidates(m link.SourceMoved, entries []inventory.Entry, } from = append(from, e) case readsFrom(read[e.Manifest.Module], m): + // **A merge older than the module's last look is history for it** (novox/hq ADR 0267): a + // 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])) { + continue + } packaging = append(packaging, e) } } return from, packaging, already } -// wouldMove is the modules built from the merged repository that acting on this merge would mark as -// moved and rebuild — SourceMoved's judgement, made without acting (novox/hq issue 266). Empty for a -// merge already acted on: acting marks each of them as looked at, so the merge then reads as history. +// 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 + for _, r := range read { + if r.Looked.After(at) { + at = r.Looked + } + } + return at +} + +// wouldMove is the modules acting on this merge would move and rebuild — SourceMoved's judgement, made +// without acting (novox/hq issue 266). Empty for a merge already acted on: acting marks each module built +// from the repository as looked at, so the merge then reads as history for it. // -// **Only the modules built from it, never the ones that merely package source from it.** Acting -// records nothing about those, so a merge acted on would go on reading as unacted for them, and be -// acted on again on every look. A merge that moves both is caught by the first kind, and acting on it -// rebuilds the second as well. +// **The ones packaging source from it too** (novox/hq ADR 0267): with a module moved only by the files of +// its build source, a merge can move a packaging module and nothing built from the repository, and a missed +// one of those was never acted on. A packaging module's look is its newest build or plan (lookedAt), so a +// merge acted on for it reads as history once its plan is made. func wouldMove(m link.SourceMoved, entries []inventory.Entry, read map[string][]inventory.ReadRepository) []inventory.Entry { - from, _, _ := mergeCandidates(m, entries, read) + from, packaging, _ := mergeCandidates(m, entries, read) touched, _ := splitDeleted(whatTheMergeTouched(from, entries, m, read), m) - return touched + return append(touched, packaging...) } // splitDeleted parts the modules a merge touched into those it changed and those whose manifest it @@ -721,75 +733,92 @@ func readsFile(e inventory.Entry, read []inventory.ReadRepository, p string) boo return strings.Trim(e.Source.Path, "/") == "" || inside(p, e.Source.Path) } -// pendingIn is the modules an open plan has yet to build (novox/hq ADR 0267): what such a module was built -// from is about to change, so the build source its last build said is not the one a merge now meets — an -// earlier merge that added an import to it would otherwise hide a change to what that import names. Each -// is read whole, as before, and planned again with the merge; the newer plan supersedes the older. -func pendingIn(plans []inventory.Plan) map[string]bool { - pending := map[string]bool{} - for _, p := range plans { - if !p.Open() { - continue +// staleIn is the modules whose recorded build source a plan has overtaken (novox/hq ADR 0267): a plan still +// working that has yet to build one, or a plan made after that build which never built it — failed, stopped +// or superseded. What such a module is built from is changing, or changed without a build to say so: a merge +// that added an import to it, and a later one changing only what that import names, would otherwise move +// nothing. Each is read whole, as before, until a build of it works again. +func staleIn(read map[string][]inventory.ReadRepository, plans []inventory.Plan) map[string]bool { + stale := map[string]bool{} + for name, rs := range read { + var since time.Time + for _, r := range rs { + if r.Built.After(since) { + since = r.Built + } } - for name, s := range p.Modules { - if s == nil || (s.State != "built" && s.State != planDeleted) { - pending[name] = true + for _, p := range plans { + s, in := p.Modules[name] + if !in || (s != nil && (s.State == "built" || s.State == planDeleted)) { + continue + } + if p.Open() || p.Created.After(since) { + stale[name] = true } } } - return pending + return stale } -// readForAMerge is what each module's build read, as a merge acting now maps its files onto: the build -// sources the newest trunk builds said, but for the modules an open plan has yet to build (pendingIn). -func readForAMerge(ctx context.Context, inv *inventory.Inventory) (map[string][]inventory.ReadRepository, error) { +// 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. +func lookedAt(name string, read []inventory.ReadRepository, plans []inventory.Plan) time.Time { + var at time.Time + for _, r := range read { + if r.Looked.After(at) { + at = r.Looked + } + } + for _, p := range plans { + if _, in := p.Modules[name]; in && p.Created.After(at) { + at = p.Created + } + } + return at +} + +// readForPlanning is what each module's build read, as the planner maps a change onto it — for a merge +// acting now, the merge gate, a pull request's check, a delivery's order and the what-if alike, so planning +// and gating cannot disagree (novox/hq ADR 0238): the build sources the newest trunk builds said, but for +// the modules a plan has overtaken (staleIn), and with when each was last looked at (lookedAt). +func readForPlanning(ctx context.Context, inv *inventory.Inventory) (map[string][]inventory.ReadRepository, error) { read, err := inv.ReadRepositories(ctx) if err != nil { return nil, err } - open, err := inv.OpenPlans(ctx) + plans, err := inv.RecentPlans(ctx, planLookBack) if err != nil { return nil, err } - return unnarrowed(read, pendingIn(open)), nil + return planningView(read, plans), nil } -// unnarrowed is what modules read, with the build sources of the pending ones set aside: each of them is -// read whole. -func unnarrowed(read map[string][]inventory.ReadRepository, pending map[string]bool) map[string][]inventory.ReadRepository { - if len(pending) == 0 { - return read - } +// 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 { - if !pending[name] { - out[name] = rs - continue - } - var whole []inventory.ReadRepository + looked := lookedAt(name, rs, plans) + var kept []inventory.ReadRepository for _, r := range rs { - if r.Own { - continue + if stale[name] { + if r.Own { + continue + } + r.Paths = nil } - r.Paths = nil - whole = append(whole, r) + r.Looked = looked + kept = append(kept, r) } - out[name] = whole + out[name] = kept } return out } -// lastLookAt is the most recent look at this repository by anything built from it. -func lastLookAt(entries []inventory.Entry, m link.SourceMoved) time.Time { - var newest time.Time - for _, e := range entries { - if sameRepository(e.Source.Repository, m) && e.Source.Seen.After(newest) { - newest = e.Source.Seen - } - } - return newest -} - // whatTheMergeTouched narrows the modules built from a repository to the ones the merge changed: **a // changed file touches exactly the modules whose build reads it** (novox/hq issue 280, ADR 0238). It is // touchedBy's first answer; touchedBy is the one place the mesh maps a changed file onto its modules. diff --git a/internal/inventory/builds.go b/internal/inventory/builds.go index 178c32d8..0550f195 100644 --- a/internal/inventory/builds.go +++ b/internal/inventory/builds.go @@ -91,6 +91,10 @@ type ReadRepository struct { // Own is the module's own repository, whose Paths narrow what its own directory — or, for a module // built from its repository's root, the whole repository — would otherwise be. Own bool `json:"own,omitempty"` + // Built is when the build these were read from was asked, and Looked when the module's newest build of + // 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:"-"` } // BuildSource is the build source a build read in one repository (novox/hq ADR 0267): Repository and Ref @@ -338,8 +342,41 @@ func (i *Inventory) BuiltAgainst(ctx context.Context) (map[string][]string, erro // none — off the trunk, from a builder that predates it, or of a source not known — leaves its module read // as before: every file of a context, and its own directory or root. func (i *Inventory) ReadRepositories(ctx context.Context) (map[string][]ReadRepository, error) { + // The newest build of each module whatever its outcome: a failed one newer than the newest that + // worked leaves that one's build source stale — the merge it was asked for may have changed the closure + // (novox/hq ADR 0267), so its module is read whole until a build works again. + newest := map[string]struct { + at time.Time + failed bool + }{} + tried, err := i.store.Pool().Query(ctx, + `select distinct on (module) module, coalesce(asked, at), failed <> '' + from build + where module is not null and module <> '' + order by module, `+newestRequestFirst) + if err != nil { + return nil, err + } + for tried.Next() { + var module string + var at time.Time + var failed bool + if err := tried.Scan(&module, &at, &failed); err != nil { + tried.Close() + return nil, err + } + newest[module] = struct { + at time.Time + failed bool + }{at, failed} + } + tried.Close() + if err := tried.Err(); err != nil { + return nil, err + } + rows, err := i.store.Pool().Query(ctx, - `select distinct on (module) module, built_contexts, build_sources + `select distinct on (module) module, built_contexts, build_sources, coalesce(asked, at) from build where module is not null and module <> '' and failed = '' order by module, `+newestRequestFirst) @@ -352,7 +389,8 @@ func (i *Inventory) ReadRepositories(ctx context.Context) (map[string][]ReadRepo for rows.Next() { var module string var raw, rawSources []byte - if err := rows.Scan(&module, &raw, &rawSources); err != nil { + var built time.Time + if err := rows.Scan(&module, &raw, &rawSources, &built); err != nil { return nil, err } var of []ReadRepository @@ -367,7 +405,13 @@ func (i *Inventory) ReadRepositories(ctx context.Context) (map[string][]ReadRepo sources = nil } } + if n, known := newest[module]; known && n.failed && n.at.After(built) { + sources = nil + } if of = WithBuildSources(of, sources); len(of) > 0 { + for k := range of { + of[k].Built, of[k].Looked = built, newest[module].at + } read[module] = of } } diff --git a/internal/inventory/builds_test.go b/internal/inventory/builds_test.go index fda48b9e..f2e0fe9b 100644 --- a/internal/inventory/builds_test.go +++ b/internal/inventory/builds_test.go @@ -5,6 +5,7 @@ import ( "reflect" "strings" "testing" + "time" ) // A build result was answered to whoever asked and kept nowhere, so "when did this last build", @@ -200,6 +201,12 @@ func TestABuildsBuildSourceComesBackOverWhatItRead(t *testing.T) { {Repository: "novox/other"}, {Paths: []string{"modules/route-proxy/Dockerfile", "modules/route-proxy/module.json"}, Own: true}, } + for k := range read["route-proxy"] { + if read["route-proxy"][k].Built.IsZero() { + t.Errorf("no build time on %+v", read["route-proxy"][k]) + } + read["route-proxy"][k].Built, read["route-proxy"][k].Looked = time.Time{}, time.Time{} + } if !reflect.DeepEqual(read["route-proxy"], want) { t.Fatalf("read back %+v\nwanted %+v", read["route-proxy"], want) } @@ -214,3 +221,29 @@ func TestABuildsBuildSourceComesBackOverWhatItRead(t *testing.T) { t.Fatalf("the build source was stored among what the build read: %s", stored) } } + +// A build newer than the newest that worked, and failed, leaves that one's build source stale: its module is +// read whole until a build works again (novox/hq ADR 0267). +func TestAFailedNewerBuildLeavesTheBuildSourceStale(t *testing.T) { + inv := fresh(t) + ctx := context.Background() + worked := aBuild("worked", "route-proxy", "") + worked.Asked = time.Now().Add(-time.Hour) + worked.Read = []ReadRepository{{Repository: "novox/mesh-controller", Ref: "main"}} + worked.Sources = []BuildSource{{Paths: []string{"modules/route-proxy/module.json"}}, + {Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"examples/route-proxy/"}}} + failed := aBuild("failed", "route-proxy", "compile error") + failed.Asked = time.Now() + for _, b := range []Build{worked, failed} { + if err := inv.RecordBuild(ctx, b); err != nil { + t.Fatal(err) + } + } + read, err := inv.ReadRepositories(ctx) + if err != nil { + t.Fatal(err) + } + if got := read["route-proxy"]; len(got) != 1 || got[0].Paths != nil || got[0].Own || !got[0].Looked.After(got[0].Built) { + t.Fatalf("after a newer failed build the proxy reads as %+v", got) + } +} From bd35b1c06c40cabe6f40795bca5deb647b6f1543 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 03:23:50 +0200 Subject: [PATCH 3/3] 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))