From afed887217914b332888fdb9c3b971a64330d269 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 20:04:13 +0200 Subject: [PATCH 1/2] Build a check's builds for its merge where the host's container engine sees them (hq issue 713) The check built in a temporary directory of the build holder's container, which the host's engine mounted empty: every check build failed and every walk built again. --- internal/builder/check.go | 45 +++++++---- internal/builder/check_builds.go | 6 +- internal/builder/replay713_test.go | 125 +++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 16 deletions(-) create mode 100644 internal/builder/replay713_test.go diff --git a/internal/builder/check.go b/internal/builder/check.go index 0e10a85f..ee320858 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -406,13 +406,16 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry // **What the merge would move is built from a checkout nothing of the check touches** (novox/hq issue 703): the // gate and the repository's own check run in containers that mount the workspace read-write, so they can change // its tree, its objects and its caches. The builds are made from a copy of the merge's objects, cloned before - // anything of the change runs, outside the workspace — so what is kept under a key is what the key describes. - building, buildingTree := "", "" + // anything of the change runs, outside the workspace — so what is kept under a key is what the key describes. Each + // build's working copy is cloned from it into the workspace only after the check has ended (forTheMerge): the + // host's container engine sees no other directory of the holder's (novox/hq issue 713). + buildingTree := "" if len(spec.Builds) > 0 && spec.BuildWith != nil && len(spec.Group) == 0 { at := judged.Merge if at == "" { at = spec.Ref } + var building string building, buildingTree, err = checkoutForTheBuilds(ctx, run, tree, at, workspace) if err != nil { return CheckVerdict{}, err @@ -750,23 +753,35 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry // **What the merge would move, built once, here** (novox/hq ADR 0350 §1): after the judging, from the merge it // judged, into the scratch namespace — so the walk of the merge commit takes it rather than building again. if len(spec.Builds) > 0 && spec.BuildWith != nil && len(spec.Group) == 0 { - if why := skipsTheBuilds(v); why != "" { - say("check", "%s", why) - } else { - began := time.Now() - v.Built = buildForTheMerge(ctx, run, spec, *spec.BuildWith, building, buildingTree, judged.Merge, judged, forge, - func(step, message string) { - if log != nil { - log(step, redact.redact(message)) - } - }, say) - say("check", "built for the merge: %d of %d module(s) (%s)", builtCount(v.Built), len(v.Built), - time.Since(began).Round(time.Second)) - } + v.Built = forTheMerge(ctx, run, spec, v, workspace, buildingTree, judged, forge, func(step, message string) { + if log != nil { + log(step, redact.redact(message)) + } + }, say) } return named(v), nil } +// forTheMerge builds what the merge moves once the check has judged it, from merged — the copy of the merge +// checkoutForTheBuilds made outside the workspace — **in the builder's own workspace** (novox/hq issue 713). A build's +// containers are run by the host's container engine, which mounts a path as the host names it: the build seat's +// holder runs in a container that shares only its workspace with the host, at the same path, so a working copy in +// its own temporary directory reaches the toolchain as an empty directory ("go.mod file not found"), every check +// build failed, and every walk built again. The copy stays outside the workspace, so nothing the tests ran reaches it; +// the working copy cloned from it is made only now, after every container of the check has ended. +func forTheMerge(ctx context.Context, run Runner, spec CheckSpec, verdict CheckVerdict, workspace, merged string, + judged CheckVerdict, forge GitCredential, log Log, say func(step, format string, args ...any)) []CheckBuilt { + if why := skipsTheBuilds(verdict); why != "" { + say("check", "%s", why) + return nil + } + began := time.Now() + built := buildForTheMerge(ctx, run, spec, *spec.BuildWith, workspace, merged, judged.Merge, judged, forge, log, say) + say("check", "built for the merge: %d of %d module(s) (%s)", builtCount(built), len(built), + time.Since(began).Round(time.Second)) + return built +} + // checkoutForTheBuilds is a clone of the commit a check builds for its merge, made before anything of the check runs, // outside the workspace its containers mount (novox/hq issue 703): the directory to remove, and the clone in it. // Cloned over git's own transport, never by linking the objects: what the check's containers later write into the diff --git a/internal/builder/check_builds.go b/internal/builder/check_builds.go index 7c0223ca..0540d0e2 100644 --- a/internal/builder/check_builds.go +++ b/internal/builder/check_builds.go @@ -70,7 +70,9 @@ func skipsTheBuilds(v CheckVerdict) string { // buildForTheMerge builds each module the plan moves, tier by tier, into the scratch namespace, and keeps what it // built under its keys. A module standing after one not built is not built either: what it stands on in the walk -// is not what it would stand on here. tree is the check's clone and merge the merge commit it made there. +// is not what it would stand on here. tree is the check's clone and merge the merge commit it made there; workspace +// is where each build's working copy is made, a directory the host's container engine sees at the same path — the +// builder's own workspace, never a temporary directory of its container (novox/hq issue 713). func buildForTheMerge(ctx context.Context, run Runner, spec CheckSpec, with CheckBuilding, workspace, tree, merge string, judged CheckVerdict, forge GitCredential, log Log, say func(step, format string, args ...any)) []CheckBuilt { if len(spec.Builds) == 0 || with.Publish == nil || with.Scratch == nil { @@ -80,6 +82,8 @@ func buildForTheMerge(ctx context.Context, run Runner, spec CheckSpec, with Chec if ref == "" { ref = spec.Ref } + // What an earlier check left, ended before it could remove it, is not built on. + _ = os.RemoveAll(filepath.Join(workspace, "for-the-merge")) held := map[string]string{} for k, v := range with.Held { held[k] = v diff --git a/internal/builder/replay713_test.go b/internal/builder/replay713_test.go new file mode 100644 index 00000000..214ae2a2 --- /dev/null +++ b/internal/builder/replay713_test.go @@ -0,0 +1,125 @@ +package builder + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" +) + +// novox/hq issue 713: after ADR 0350 the walk of novox/mesh-controller#331 built mesh-controller, build-agent and +// route-proxy again, each "built: no check of novox/mesh-controller#331 built it". The check had built none: its +// build log said "built for the merge: 0 of 3 module(s)", mesh-controller's go bundle failing with "go.mod file not +// found". The build seat's holder runs in a container sharing only its workspace with the host, and a build's +// toolchain container is run by the host's container engine, which mounts a path as the host names it. The check +// built in a temporary directory of the holder's own (issue 703's copy), which the engine mounted as an empty one. + +// aHostsEngine runs as the host's container engine does for a holder in a container: a mount of a path outside the +// workspace the holder shares with the host is an empty directory, and a Go toolchain finds no go.mod in it. +type aHostsEngine struct { + onTrunk + shared string +} + +func (h aHostsEngine) run(ctx context.Context, dir, name string, args ...string) (string, error) { + if name == "docker" && len(args) > 0 && args[0] == "run" { + for i, a := range args { + if a != "--volume" || i+1 >= len(args) { + continue + } + from, _, _ := strings.Cut(args[i+1], ":") + if !withinDir(h.shared, from) { + return "", errors.New("go: go.mod file not found in current directory or any parent directory") + } + } + } + return h.onTrunk.run(ctx, dir, name, args...) +} + +// aGoModule is a module of one Go bundle built from a repository's root. +func aGoModule(module string) map[string]string { + files := aSharedRepository("one", "1") + files[ManifestName] = `{"module":"` + module + `","version":"1","build":{"artifacts":[ + {"name":"server","kind":"bundle","language":"go","system":"arch","from":"cmd/ctl","binary":"ctl"}]}, + "resources":[{"id":"server","type":"process","name":"` + module + `","artifact":"server","run":["./ctl"]}]}` + return files +} + +// A controller change whose walk moves mesh-controller, build-agent and route-proxy (#331's) has its check build all +// three, in the workspace the host's engine sees, and the walk of its merge takes each from the check by its key. +func TestReplay713TheWalkOfAControllerChangeTakesAllThreeFromItsCheck(t *testing.T) { + withVersion(t, "builder-1") + pull := PullRef{Owner: "novox", Repo: "mesh-controller", Number: 331} + held := map[string]string{"mesh-tools-go/build": "registry.invalid/mesh-tools-go/build@sha256:" + strings.Repeat("b", 64)} + repositories := map[string]string{"build-agent": "https://forge.invalid/mesh-catalog-build-agent.git", + "route-proxy": "https://forge.invalid/mesh-catalog-route-proxy.git"} + checked := func(shared string) (*recorded, aHostsEngine) { + r := &recorded{tree: "tree-of-the-merge", contents: aGoModule("mesh-controller"), + secondary: map[string]map[string]string{}} + for module, repository := range repositories { + r.secondary[repository] = aGoModule(module) + } + return r, aHostsEngine{onTrunk: onTrunk{recorded: r}, shared: shared} + } + spec := CheckSpec{ID: "build-331", Owner: pull.Owner, Repo: pull.Repo, Number: pull.Number, + Builds: []CheckBuildSpec{{Module: "mesh-controller", Merged: true, Tier: 0}, + {Module: "build-agent", Repository: repositories["build-agent"], Ref: "main", Tier: 1}, + {Module: "route-proxy", Repository: repositories["route-proxy"], Ref: "main", Tier: 2}}} + judged := CheckVerdict{Head: "98e856874913", Trunk: "c975dbf7b053", Merge: "2d22279b1d07"} + passed := CheckVerdict{Gate: &Layer{Verdict: "pass"}, Repo: &Layer{Verdict: "pass"}} + + // As it ran: the builds made in the holder's own temporary directory, which the host's engine does not see. + shared, outside := t.TempDir(), t.TempDir() + merged := filepath.Join(outside, "merged") + r, engine := checked(shared) + asItRan := buildForTheMerge(t.Context(), engine.run, spec, CheckBuilding{Publish: r, Scratch: newScratch(), Held: held}, + outside, merged, judged.Merge, judged, GitCredential{}, nil, logging(nil)) + if builtCount(asItRan) != 0 { + t.Fatalf("the engine of this test sees the holder's temporary directory: %+v", asItRan) + } + + // The check as it builds now: every module its merge moves, kept under its key. + scratch := newScratch() + spec.BuildWith = &CheckBuilding{Publish: r, Scratch: scratch, Held: held} + built := forTheMerge(t.Context(), engine.run, spec, passed, shared, merged, judged, GitCredential{}, nil, + logging(nil)) + if len(built) != 3 || builtCount(built) != 3 { + t.Fatalf("the check built %d of %d module(s) its merge moves: %+v", builtCount(built), len(built), built) + } + var names []NamedBuild + for _, b := range built { + n := NamedBuild{Pull: pull, Check: spec.ID, Module: b.Module, Key: b.Key, Digests: map[string]string{}} + for _, m := range b.Made { + d := m.Digest + if d == "" { + d = DigestOf(m.Reference) + } + n.Digests[m.Name] = d + } + names = append(names, n) + } + + // The walk of the merge commit, tier by tier: each module taken from the check, none built. + walked := map[string]string{"mesh-controller": "https://forge.invalid/mesh-controller.git"} + for module, repository := range repositories { + walked[module] = repository + } + for _, module := range []string{"mesh-controller", "build-agent", "route-proxy"} { + w, walk := checked(shared) + w.secondary[walked["mesh-controller"]] = aGoModule("mesh-controller") + got, err := BuildReusing(t.Context(), walk.run, w, Reuse{Scratch: scratch, Pulls: []PullRef{pull}, Named: names}, + walked[module], "", "main", t.TempDir(), held, Npmrc{}, GitCredential{}, nil) + if err != nil { + t.Fatalf("%s: %v", module, err) + } + if got.Reused == nil || got.Why != "" { + t.Fatalf("the walk built %s again: %s", module, got.Why) + } + for _, line := range w.ran { + if strings.HasPrefix(line, "docker run") && strings.Contains(line, "go build") { + t.Fatalf("the walk compiled %s: %s", module, line) + } + } + } +} -- 2.54.0 From bbee20adf9b9120f5ca1a0a9817467242bd41c02 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 20:20:49 +0200 Subject: [PATCH 2/2] Build a merge's modules of a tier at once, and beside the check's tests where the judge cache allows (hq issue 716) A check built its images one after another, after merge-check.sh. With a judge cache the builds run during the tests, out of their reach; a failed check lets go of them. --- cmd/mesh-builder/main.go | 6 + internal/builder/check.go | 43 +++- internal/builder/check_builds.go | 321 +++++++++++++++++++++-------- internal/builder/replay716_test.go | 158 ++++++++++++++ 4 files changed, 440 insertions(+), 88 deletions(-) create mode 100644 internal/builder/replay716_test.go diff --git a/cmd/mesh-builder/main.go b/cmd/mesh-builder/main.go index 4ad26abc..6fce8127 100644 --- a/cmd/mesh-builder/main.go +++ b/cmd/mesh-builder/main.go @@ -26,6 +26,7 @@ import ( "os/signal" "sort" "strings" + "sync" "syscall" "time" @@ -272,7 +273,12 @@ func answer(ctx context.Context, publisher builder.Publisher, on, workspace stri // role's own events under the build's id (novox/hq ADR 0157) — so whoever asked, and anybody // watching, reads the same lines this container's log holds, live, and after the fact from the // stream. Said first, before anything runs, so a build that hangs is one that visibly started. + // One line at a time: a check's builds for its merge say their lines while its tests say theirs (novox/hq + // issue 716). + var saying sync.Mutex say := func(step, message string) { + saying.Lock() + defer saying.Unlock() fmt.Fprintf(os.Stderr, " [%s] %s\n", step, message) work.Say(step, message) if mine != nil && step != "run" && step != "output" { diff --git a/internal/builder/check.go b/internal/builder/check.go index ee320858..cac6b0ad 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -15,6 +15,7 @@ import ( "regexp" "sort" "strings" + "sync" "time" "github.com/novox/mesh-controller/internal/artifacts" @@ -300,7 +301,17 @@ func ScriptToolchain(script []byte) string { // Check runs one merge check. An error is that it could not run; the verdict is then "error". func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry string, forge GitCredential, log Log) (CheckVerdict, error) { - // Everything a check says goes to the build's log, which the bus keeps: said redacted (novox/hq issue 462). + // Everything a check says goes to the build's log, which the bus keeps: said redacted (novox/hq issue 462). A line + // at a time: its builds for the merge say theirs while its tests run (novox/hq issue 716). + if log != nil { + var saying sync.Mutex + inner := log + log = func(step, message string) { + saying.Lock() + defer saying.Unlock() + inner(step, message) + } + } redact := redactorFor(forge) say := logging(func(step, message string) { if log != nil { @@ -532,6 +543,25 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry // Every clone is made: the credential is gone before anything of the check runs (novox/hq issue 462). forget() + // **What the merge moves is built beside the tests** (novox/hq issue 716) when the build agent has a judge cache to + // build it in, where no container of the check reaches; without one, after the check has judged. + logged := func(step, message string) { + if log != nil { + log(step, redact.redact(message)) + } + } + var beside *besideTheTests + if buildingTree != "" { + place, err := buildsBesideTheTests(spec.JudgeCache, workspace, spec.ID) + if err != nil { + return CheckVerdict{}, err + } + if place != "" { + beside = startBesideTheTests(ctx, run, spec, place, buildingTree, judged, forge, logged, say) + defer beside.abandon() + } + } + // The facts, and the versions they say the mesh runs. if registry == "" { return CheckVerdict{}, errors.New("no artifact store to read the facts snapshot from") @@ -752,12 +782,11 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry // **What the merge would move, built once, here** (novox/hq ADR 0350 §1): after the judging, from the merge it // judged, into the scratch namespace — so the walk of the merge commit takes it rather than building again. - if len(spec.Builds) > 0 && spec.BuildWith != nil && len(spec.Group) == 0 { - v.Built = forTheMerge(ctx, run, spec, v, workspace, buildingTree, judged, forge, func(step, message string) { - if log != nil { - log(step, redact.redact(message)) - } - }, say) + switch { + case beside != nil: + v.Built = beside.settle(ctx, v) + case len(spec.Builds) > 0 && spec.BuildWith != nil && len(spec.Group) == 0: + v.Built = forTheMerge(ctx, run, spec, v, workspace, buildingTree, judged, forge, logged, say) } return named(v), nil } diff --git a/internal/builder/check_builds.go b/internal/builder/check_builds.go index 0540d0e2..4a1be47a 100644 --- a/internal/builder/check_builds.go +++ b/internal/builder/check_builds.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "sync" "time" "github.com/novox/mesh-controller/internal/catalogue" @@ -68,108 +69,266 @@ func skipsTheBuilds(v CheckVerdict) string { return "" } -// buildForTheMerge builds each module the plan moves, tier by tier, into the scratch namespace, and keeps what it -// built under its keys. A module standing after one not built is not built either: what it stands on in the walk -// is not what it would stand on here. tree is the check's clone and merge the merge commit it made there; workspace -// is where each build's working copy is made, a directory the host's container engine sees at the same path — the -// builder's own workspace, never a temporary directory of its container (novox/hq issue 713). +// buildForTheMerge builds each module the plan moves into the scratch namespace (makeForTheMerge), and keeps what it +// built under its keys (keepForTheMerge): the order of a check that builds after it has judged. func buildForTheMerge(ctx context.Context, run Runner, spec CheckSpec, with CheckBuilding, workspace, tree, merge string, judged CheckVerdict, forge GitCredential, log Log, say func(step, format string, args ...any)) []CheckBuilt { + out, kept := makeForTheMerge(ctx, run, spec, with, workspace, tree, merge, judged, forge, log, say) + return keepForTheMerge(ctx, spec, with, out, kept, say) +} + +// makeForTheMerge builds each module the plan moves, tier by tier, into the scratch namespace — **the modules of a +// tier at the same time** (novox/hq issue 716): none of them stands on another, and a tier waits only for the ones +// before it, whose builds it is handed. A module standing after one not built is not built either: what it stands on +// in the walk is not what it would stand on here. tree is the check's copy of the merge and merge the merge commit it +// made there; workspace is where each build's working copy is made, one directory per module, which the host's +// container engine sees at the same path (novox/hq issue 713). Answers what each module came to, in the plan's order, +// and the builds to keep — nothing is kept here. +func makeForTheMerge(ctx context.Context, run Runner, spec CheckSpec, with CheckBuilding, workspace, tree, merge string, + judged CheckVerdict, forge GitCredential, log Log, say func(step, format string, args ...any)) ([]CheckBuilt, []CheckBuild) { if len(spec.Builds) == 0 || with.Publish == nil || with.Scratch == nil { - return nil + return nil, nil + } + // What an earlier check left, ended before it could remove it, is not built on. + place := filepath.Join(workspace, "for-the-merge") + _ = os.RemoveAll(place) + defer os.RemoveAll(place) + // One log, said to by builds running at once: a line at a time. + var saying sync.Mutex + if log != nil { + inner := log + log = func(step, message string) { + saying.Lock() + defer saying.Unlock() + inner(step, message) + } + } + inner := say + say = func(step, format string, args ...any) { + saying.Lock() + defer saying.Unlock() + inner(step, format, args...) } ref := merge if ref == "" { ref = spec.Ref } - // What an earlier check left, ended before it could remove it, is not built on. - _ = os.RemoveAll(filepath.Join(workspace, "for-the-merge")) held := map[string]string{} for k, v := range with.Held { held[k] = v } - var out []CheckBuilt + out := make([]CheckBuilt, len(spec.Builds)) + results := make([]Result, len(spec.Builds)) var kept []CheckBuild stopAt, stopWhy := -1, "" - for _, b := range spec.Builds { - began := time.Now() - if stopAt >= 0 && b.Tier > stopAt { - out = append(out, CheckBuilt{Module: b.Module, NotBuilt: stopWhy}) - continue + for start := 0; start < len(spec.Builds); { + tier := spec.Builds[start].Tier + end := start + for end < len(spec.Builds) && spec.Builds[end].Tier == tier { + end++ } - if b.Skip != "" { - say("build", "%s not built for the merge: %s", b.Module, b.Skip) - out = append(out, CheckBuilt{Module: b.Module, NotBuilt: b.Skip}) - if stopAt < 0 || b.Tier < stopAt { - stopAt, stopWhy = b.Tier, "it stands after "+b.Module+", which was not built for the merge" + // What this tier's builds are handed: the builds of the tiers before it, read alone while they run. + handed := map[string]string{} + for k, v := range held { + handed[k] = v + } + var building sync.WaitGroup + for i := start; i < end; i++ { + b := spec.Builds[i] + if stopAt >= 0 && b.Tier > stopAt { + out[i] = CheckBuilt{Module: b.Module, NotBuilt: stopWhy} + continue } - continue - } - repository, at := b.Repository, b.Ref - if b.Merged { - repository, at = tree, ref - } - say("build", "for the merge: %s (tier %d)", b.Module, b.Tier) - var mirrored []string - result, err := build(ctx, run, scratchPublisher{to: with.Publish}, repository, b.Path, at, - filepath.Join(workspace, "for-the-merge"), held, with.Npmrc, forge, log, &mirrored, - &Reuse{keyFirst: true, Scratch: with.Scratch, Named: with.Named, - pull: PullRef{Owner: spec.Owner, Repo: spec.Repo, Number: spec.Number}, head: judged.Head}, with.Seats) - built := CheckBuilt{Module: b.Module, Key: result.ReuseKey, Took: time.Since(began)} - switch { - case errors.Is(err, errNoKey): - built.NotBuilt = err.Error() - case err != nil: - built.NotBuilt = "its build failed: " + err.Error() - case result.ReuseKey == "": - built.NotBuilt = "it has no reuse key: " + result.NoKey - } - if built.NotBuilt != "" { - say("build", "%s not built for the merge: %s", b.Module, built.NotBuilt) - out = append(out, built) - if stopAt < 0 || b.Tier < stopAt { - stopAt, stopWhy = b.Tier, "it stands after "+b.Module+", which was not built for the merge" + if b.Skip != "" { + say("build", "%s not built for the merge: %s", b.Module, b.Skip) + out[i] = CheckBuilt{Module: b.Module, NotBuilt: b.Skip} + continue } - continue - } - built.Made = result.Built - for _, m := range result.Built { - held[b.Module+"/"+m.Name] = m.Reference - } - if result.Reused != nil { - // **Kept, not built** (novox/hq issue 703): the earlier check's build, named as that check's, so the - // walk takes it as the verdict names it; this check's document names it too, so it stays while this - // check does (ADR 0350 §5). - built.Kept = result.Reused.From.Check - say("build", "%s kept for the merge under %s: %s", b.Module, twelve(result.ReuseKey[3:]), - result.Reused.From.Said()) - out = append(out, built) - kept = append(kept, result.Reused.From) - continue - } - say("build", "%s built for the merge under %s (%s)", b.Module, twelve(result.ReuseKey[3:]), built.Took.Round(time.Second)) - out = append(out, built) - if len(result.Built) > 0 { - kept = append(kept, CheckBuild{Key: result.ReuseKey, Module: b.Module, Parts: result.ReuseParts, - Made: result.Built, Pull: PullRef{Owner: spec.Owner, Repo: spec.Repo, Number: spec.Number}, - Head: judged.Head, Trunk: judged.Trunk, Check: spec.ID, At: time.Now().UTC()}) - } - } - _ = os.RemoveAll(filepath.Join(workspace, "for-the-merge")) - if len(kept) > 0 { - if err := with.Scratch.Keep(ctx, PullRef{Owner: spec.Owner, Repo: spec.Repo, Number: spec.Number}, spec.ID, - time.Now().UTC(), kept); err != nil { - // Built and not findable: the walk builds, and says no check built it. Said, never the check's verdict. - say("build", "the builds for the merge could not be kept, and the walk will build: %v", err) - if err := with.Scratch.Discard(ctx, kept); err != nil { - say("build", "and what they made could not all be let go of: %v", err) + repository, at := b.Repository, b.Ref + if b.Merged { + repository, at = tree, ref } - for i := range out { - if out[i].NotBuilt == "" { - out[i].NotBuilt = fmt.Sprintf("built, and could not be kept: %v", err) + name := b.Module + if !safeName.MatchString(name) { + name = fmt.Sprintf("module-%d", i) + } + building.Add(1) + go func(i int, b CheckBuildSpec, repository, at, dir string) { + defer building.Done() + began := time.Now() + say("build", "for the merge: %s (tier %d)", b.Module, b.Tier) + var mirrored []string + result, err := build(ctx, run, scratchPublisher{to: with.Publish}, repository, b.Path, at, dir, handed, + with.Npmrc, forge, log, &mirrored, &Reuse{keyFirst: true, Scratch: with.Scratch, Named: with.Named, + pull: PullRef{Owner: spec.Owner, Repo: spec.Repo, Number: spec.Number}, head: judged.Head}, + with.Seats) + built := CheckBuilt{Module: b.Module, Key: result.ReuseKey, Took: time.Since(began)} + switch { + case errors.Is(err, errNoKey): + built.NotBuilt = err.Error() + case err != nil: + built.NotBuilt = "its build failed: " + err.Error() + case result.ReuseKey == "": + built.NotBuilt = "it has no reuse key: " + result.NoKey } + out[i], results[i] = built, result + }(i, b, repository, at, filepath.Join(place, name)) + } + building.Wait() + for i := start; i < end; i++ { + b, built, result := spec.Builds[i], &out[i], results[i] + if built.NotBuilt != "" { + if built.Key != "" || built.Took > 0 { + say("build", "%s not built for the merge: %s", b.Module, built.NotBuilt) + } + if stopAt < 0 || b.Tier < stopAt { + stopAt, stopWhy = b.Tier, "it stands after "+b.Module+", which was not built for the merge" + } + continue + } + built.Made = result.Built + for _, m := range result.Built { + held[b.Module+"/"+m.Name] = m.Reference + } + if result.Reused != nil { + // **Kept, not built** (novox/hq issue 703): the earlier check's build, named as that check's, so the + // walk takes it as the verdict names it; this check's document names it too, so it stays while this + // check does (ADR 0350 §5). + built.Kept = result.Reused.From.Check + say("build", "%s kept for the merge under %s: %s", b.Module, twelve(result.ReuseKey[3:]), + result.Reused.From.Said()) + kept = append(kept, result.Reused.From) + continue + } + say("build", "%s built for the merge under %s (%s)", b.Module, twelve(result.ReuseKey[3:]), + built.Took.Round(time.Second)) + if len(result.Built) > 0 { + kept = append(kept, CheckBuild{Key: result.ReuseKey, Module: b.Module, Parts: result.ReuseParts, + Made: result.Built, Pull: PullRef{Owner: spec.Owner, Repo: spec.Repo, Number: spec.Number}, + Head: judged.Head, Trunk: judged.Trunk, Check: spec.ID, At: time.Now().UTC()}) + } + } + start = end + } + return out, kept +} + +// keepForTheMerge keeps a check's builds under their keys, its own document naming them, so the walk of its merge +// finds them; what cannot be kept is let go of, said, and each module says it — never the check's verdict. +func keepForTheMerge(ctx context.Context, spec CheckSpec, with CheckBuilding, out []CheckBuilt, kept []CheckBuild, + say func(step, format string, args ...any)) []CheckBuilt { + if len(kept) == 0 { + return out + } + if err := with.Scratch.Keep(ctx, PullRef{Owner: spec.Owner, Repo: spec.Repo, Number: spec.Number}, spec.ID, + time.Now().UTC(), kept); err != nil { + // Built and not findable: the walk builds, and says no check built it. Said, never the check's verdict. + say("build", "the builds for the merge could not be kept, and the walk will build: %v", err) + if err := with.Scratch.Discard(ctx, kept); err != nil { + say("build", "and what they made could not all be let go of: %v", err) + } + for i := range out { + if out[i].NotBuilt == "" { + out[i].NotBuilt = fmt.Sprintf("built, and could not be kept: %v", err) } } } return out } + +// discardForTheMerge lets go of builds a check made beside its tests when the check did not pass (novox/hq issue 716): +// a change that fails is not merged, and nothing it built is kept. A build another check names stays (Discard). +func discardForTheMerge(ctx context.Context, with CheckBuilding, kept []CheckBuild, why string, + say func(step, format string, args ...any)) { + say("check", "%s: the %d build(s) made beside it are let go of", why, len(kept)) + if len(kept) == 0 { + return + } + if err := with.Scratch.Discard(ctx, kept); err != nil { + say("build", "what they made could not all be let go of, and goes with the week's sweep: %v", err) + } +} + +// besideTheTests are a check's builds for its merge running while its gate and its own check run (novox/hq issue +// 716): started once every clone is made, waited for when the check has judged, kept when it passed and let go of +// when it did not. +type besideTheTests struct { + spec CheckSpec + with CheckBuilding + cancel context.CancelFunc + done chan struct{} + place string + out []CheckBuilt + kept []CheckBuild + began time.Time + settled bool + say func(step, format string, args ...any) +} + +// buildsBesideTheTests is where a check's builds run beside its tests: a directory made for this check under the judge +// cache's homes/ (ADR 0358 §6), on the host path the build agent declares — so the host's container engine mounts what +// the agent sees (issue 713) — and never mounted by a check's container, so nothing a pull request's script runs can +// change a build while it is made (issue 703). Empty, and the builds wait for the check, without a judge cache. +func buildsBesideTheTests(cache, workspace, id string) (string, error) { + if cache == "" { + return "", nil + } + if withinDir(workspace, cache) { + return "", fmt.Errorf("the judge cache %s is inside the workspace %s, which a check's containers mount: "+ + "nothing is built for the merge where a pull request could reach it", cache, workspace) + } + if !safeName.MatchString(id) { + return "", fmt.Errorf("the check's id %q cannot name its place in the judge cache", id) + } + place := filepath.Join(cache, "homes", id+"-builds") + if err := os.RemoveAll(place); err != nil { + return "", err + } + if err := os.MkdirAll(place, 0o700); err != nil { + return "", err + } + return place, nil +} + +// startBesideTheTests starts the builds for the merge in place, from merged, the check's copy of the merge. +func startBesideTheTests(ctx context.Context, run Runner, spec CheckSpec, place, merged string, judged CheckVerdict, + forge GitCredential, log Log, say func(step, format string, args ...any)) *besideTheTests { + building, cancel := context.WithCancel(ctx) + b := &besideTheTests{spec: spec, with: *spec.BuildWith, cancel: cancel, done: make(chan struct{}), place: place, + began: time.Now(), say: say} + say("check", "building what the merge moves (%d module(s)) beside its tests", len(spec.Builds)) + go func() { + defer close(b.done) + b.out, b.kept = makeForTheMerge(building, run, spec, b.with, place, merged, judged.Merge, judged, forge, log, say) + }() + return b +} + +// settle waits for the builds and keeps them when the check passed; a check that did not pass lets go of them, and +// its verdict names none. +func (b *besideTheTests) settle(ctx context.Context, verdict CheckVerdict) []CheckBuilt { + <-b.done + b.settled = true + defer os.RemoveAll(b.place) + if why := skipsTheBuilds(verdict); why != "" { + discardForTheMerge(ctx, b.with, b.kept, why, b.say) + return nil + } + built := keepForTheMerge(ctx, b.spec, b.with, b.out, b.kept, b.say) + b.say("check", "built for the merge: %d of %d module(s), beside the tests (%s)", builtCount(built), len(built), + time.Since(b.began).Round(time.Second)) + return built +} + +// abandon ends builds a check that ended without a verdict leaves, and lets go of what they made. +func (b *besideTheTests) abandon() { + if b.settled { + return + } + b.cancel() + <-b.done + defer os.RemoveAll(b.place) + letGo, done := context.WithTimeout(context.Background(), time.Minute) + defer done() + discardForTheMerge(letGo, b.with, b.kept, "the check ended without a verdict", b.say) +} diff --git a/internal/builder/replay716_test.go b/internal/builder/replay716_test.go new file mode 100644 index 00000000..306c3e92 --- /dev/null +++ b/internal/builder/replay716_test.go @@ -0,0 +1,158 @@ +package builder + +import ( + "context" + "errors" + "path/filepath" + "strings" + "sync" + "testing" + "time" +) + +// novox/hq issue 716: a check built the images its merge moves one after another, and only after its merge-check.sh +// had passed — 3 to 6 minutes on a check of about 5. The modules of a tier stand on none of each other and are built +// at the same time; with a judge cache, beside the tests; a check that does not pass lets go of them. + +// atOnce is a fake engine whose compiles wait until every one of a tier's has started: builds one after another never +// get there, and the first one's compile says so. +type atOnce struct { + mu sync.Mutex + r *recorded + want int + started int + all chan struct{} + shared string +} + +func (a *atOnce) run(ctx context.Context, dir, name string, args ...string) (string, error) { + if name == "docker" && len(args) > 0 && args[0] == "run" && strings.Contains(strings.Join(args, " "), "go build") { + a.mu.Lock() + a.started++ + if a.started == a.want { + close(a.all) + } + a.mu.Unlock() + select { + case <-a.all: + case <-time.After(5 * time.Second): + return "", errors.New("built alone: no other module of its tier was being built") + } + } + a.mu.Lock() + defer a.mu.Unlock() + return onTrunk{recorded: a.r}.run(ctx, dir, name, args...) +} + +func (a *atOnce) PublishImage(ctx context.Context, localTag, repository string) (string, error) { + a.mu.Lock() + defer a.mu.Unlock() + return a.r.PublishImage(ctx, localTag, repository) +} + +func (a *atOnce) PublishArchive(ctx context.Context, repository string, body []byte, digest string) (string, error) { + a.mu.Lock() + defer a.mu.Unlock() + return a.r.PublishArchive(ctx, repository, body, digest) +} + +// aLockedScratch is the scratch namespace in memory, said to by builds at once. +type aLockedScratch struct { + mu sync.Mutex + *aScratch + discarded []CheckBuild +} + +func (s *aLockedScratch) Keep(ctx context.Context, p PullRef, c string, at time.Time, b []CheckBuild) error { + s.mu.Lock() + defer s.mu.Unlock() + return s.aScratch.Keep(ctx, p, c, at, b) +} + +func (s *aLockedScratch) Find(ctx context.Context, module, key string) (CheckBuild, bool, error) { + s.mu.Lock() + defer s.mu.Unlock() + return s.aScratch.Find(ctx, module, key) +} + +func (s *aLockedScratch) Discard(_ context.Context, b []CheckBuild) error { + s.mu.Lock() + defer s.mu.Unlock() + s.discarded = append(s.discarded, b...) + return nil +} + +// threeAtOnce are three Go modules in one tier, each from its own repository. +func threeAtOnce(t *testing.T) (*atOnce, CheckSpec, map[string]string) { + t.Helper() + r := &recorded{tree: "tree-of-the-merge", contents: aGoModule("mesh-controller"), + secondary: map[string]map[string]string{}} + spec := CheckSpec{ID: "build-716", Owner: "novox", Repo: "mesh-controller", Number: 716, + Builds: []CheckBuildSpec{{Module: "mesh-controller", Merged: true, Tier: 0}}} + for _, module := range []string{"build-agent", "route-proxy"} { + repository := "https://forge.invalid/" + module + ".git" + r.secondary[repository] = aGoModule(module) + spec.Builds = append(spec.Builds, CheckBuildSpec{Module: module, Repository: repository, Ref: "main", Tier: 0}) + } + held := map[string]string{"mesh-tools-go/build": "registry.invalid/mesh-tools-go/build@sha256:" + strings.Repeat("b", 64)} + return &atOnce{r: r, want: 3, all: make(chan struct{})}, spec, held +} + +// The modules of one tier are built at the same time, each in a working copy of its own. +func TestReplay716TheModulesOfATierAreBuiltAtTheSameTime(t *testing.T) { + withVersion(t, "builder-1") + engine, spec, held := threeAtOnce(t) + scratch := &aLockedScratch{aScratch: newScratch()} + built := buildForTheMerge(t.Context(), engine.run, spec, CheckBuilding{Publish: engine, Scratch: scratch, Held: held}, + t.TempDir(), filepath.Join(t.TempDir(), "merged"), "2d22279b1d07", + CheckVerdict{Head: "98e856874913", Trunk: "c975dbf7b053"}, GitCredential{}, nil, logging(nil)) + if builtCount(built) != 3 { + t.Fatalf("the check built %d of 3 module(s) of one tier: %+v", builtCount(built), built) + } + if len(scratch.builds) != 3 { + t.Fatalf("the check kept %d build(s), not 3", len(scratch.builds)) + } +} + +// With a judge cache, the builds run while the tests run, where no container of the check reaches; a check that passed +// keeps them, and one that failed lets go of every one and names none. +func TestReplay716TheBuildsRunBesideTheTestsAndAFailedCheckLetsGoOfThem(t *testing.T) { + withVersion(t, "builder-1") + workspace := t.TempDir() + if _, err := buildsBesideTheTests(filepath.Join(workspace, "judge-cache"), workspace, "build-716"); err == nil { + t.Fatal("a judge cache inside the workspace was built in, where a pull request's script reaches") + } + for _, passed := range []bool{true, false} { + cache := t.TempDir() + place, err := buildsBesideTheTests(cache, workspace, "build-716") + if err != nil || !withinDir(cache, place) || withinDir(workspace, place) { + t.Fatalf("the builds' place %q (%v) is not the judge cache's, outside the workspace", place, err) + } + engine, spec, held := threeAtOnce(t) + scratch := &aLockedScratch{aScratch: newScratch()} + spec.BuildWith = &CheckBuilding{Publish: engine, Scratch: scratch, Held: held} + judged := CheckVerdict{Head: "98e856874913", Trunk: "c975dbf7b053", Merge: "2d22279b1d07"} + beside := startBesideTheTests(t.Context(), engine.run, spec, place, filepath.Join(t.TempDir(), "merged"), + judged, GitCredential{}, nil, logging(nil)) + // The tests: the builds are under way before the check has a verdict. + select { + case <-engine.all: + case <-time.After(5 * time.Second): + t.Fatal("nothing was built while the tests ran") + } + verdict := CheckVerdict{Gate: &Layer{Verdict: "pass"}, Repo: &Layer{Verdict: "pass"}} + if !passed { + verdict.Repo.Verdict = "fail" + } + built := beside.settle(t.Context(), verdict) + beside.abandon() + switch { + case passed && (builtCount(built) != 3 || len(scratch.builds) != 3 || len(scratch.discarded) != 0): + t.Fatalf("a check that passed: built %+v, kept %d, let go of %d", built, len(scratch.builds), + len(scratch.discarded)) + case !passed && (built != nil || len(scratch.builds) != 0 || len(scratch.discarded) != 3): + t.Fatalf("a check that failed: named %+v, kept %d, let go of %d", built, len(scratch.builds), + len(scratch.discarded)) + } + } +} -- 2.54.0