From 1666747a2246ace84b873867b9999f7e863ea23b Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 09:41:39 +0200 Subject: [PATCH 1/3] Keep a check build whose key is unchanged instead of building it again A fresh check asked when only the trunk moved rebuilt every image, though the reuse key said nothing it is built from changed: a controller check took about 12 minutes instead of 5.5. The check now keeps a build kept under its key when a verdict of the build seat names that key and every digest, names it as the build's own check so a walk takes it, and the cleanup keeps it while any check names it (novox/hq issue 703). --- cmd/mesh-builder/main.go | 31 +++--- cmd/mesh-controller/check_builds_test.go | 25 +++++ cmd/mesh-controller/checks.go | 26 ++++- cmd/mesh-controller/release_plan.go | 15 +++ internal/builder/builder.go | 33 ++++++- internal/builder/check_builds.go | 22 ++++- internal/builder/reuse.go | 10 +- internal/builder/reuse_test.go | 120 +++++++++++++++++++++++ internal/link/build.go | 7 ++ internal/link/check_builds_test.go | 30 ++++++ internal/link/queue.go | 31 +++++- 11 files changed, 328 insertions(+), 22 deletions(-) diff --git a/cmd/mesh-builder/main.go b/cmd/mesh-builder/main.go index be469d09..da49ce3b 100644 --- a/cmd/mesh-builder/main.go +++ b/cmd/mesh-builder/main.go @@ -319,7 +319,7 @@ func answer(ctx context.Context, publisher builder.Publisher, on, workspace stri // at build time has no reuse key anyway, so nothing is lost by refusing it here. if isRegistry && len(spec.Builds) > 0 { spec.BuildWith = &builder.CheckBuilding{Publish: publisher, Scratch: registryHere, Held: request.Held, - Seats: request.Seats} + Seats: request.Seats, Named: namedOf(request.Check.Named)} } var v builder.CheckVerdict v, err = builder.Check(building, builder.Command, spec, workspace, registry, forgeFrom(), say) @@ -352,15 +352,7 @@ func answer(ctx context.Context, publisher builder.Publisher, on, workspace stri reuse.Pulls = append(reuse.Pulls, builder.PullRef{Owner: p.Owner, Repo: p.Repo, Number: p.Number}) } // Only what a verdict names is taken (ADR 0350 §2). - for _, b := range request.Reuse.Builds { - named := builder.NamedBuild{Pull: builder.PullRef{Owner: b.Pull.Owner, Repo: b.Pull.Repo, - Number: b.Pull.Number}, Check: b.Check, Module: b.Built.Module, Key: b.Built.Key, - Digests: map[string]string{}} - for _, m := range b.Built.Made { - named.Digests[m.Name] = builder.DigestOf(m.Reference) - } - reuse.Named = append(reuse.Named, named) - } + reuse.Named = namedOf(request.Reuse.Builds) built, err = builder.BuildReusing(building, builder.Labelled(builder.Command, request.ID), publisher, reuse, request.Repository, request.Path, request.Ref, workspace, request.Held, npmrc, forgeFrom(), say, request.Seats) @@ -496,11 +488,28 @@ func triedOf(v builder.CheckVerdict) link.Tried { return t } +// namedOf are the builds the seat's verdicts name, as the builder believes a scratch record by them (novox/hq ADR +// 0350 §2, issue 703): each module's key with every artifact's digest. +func namedOf(builds []link.CheckedBuild) []builder.NamedBuild { + var out []builder.NamedBuild + for _, b := range builds { + named := builder.NamedBuild{Pull: builder.PullRef{Owner: b.Pull.Owner, Repo: b.Pull.Repo, + Number: b.Pull.Number}, Check: b.Check, Module: b.Built.Module, Key: b.Built.Key, + Digests: map[string]string{}} + for _, m := range b.Built.Made { + named.Digests[m.Name] = builder.DigestOf(m.Reference) + } + out = append(out, named) + } + return out +} + // checkBuiltOf is a check's builds for its merge as the outcome carries them (novox/hq ADR 0350 §1). func checkBuiltOf(built []builder.CheckBuilt) []link.CheckBuilt { var out []link.CheckBuilt for _, b := range built { - c := link.CheckBuilt{Module: b.Module, Key: b.Key, NotBuilt: b.NotBuilt, Took: b.Took.Round(time.Second).String()} + c := link.CheckBuilt{Module: b.Module, Key: b.Key, NotBuilt: b.NotBuilt, Took: b.Took.Round(time.Second).String(), + Kept: b.Kept} for _, m := range b.Made { c.Made = append(c.Made, link.MadeArtifact{Name: m.Name, Kind: m.Kind, Reference: m.Reference}) } diff --git a/cmd/mesh-controller/check_builds_test.go b/cmd/mesh-controller/check_builds_test.go index d0f41d6a..c5dc716c 100644 --- a/cmd/mesh-controller/check_builds_test.go +++ b/cmd/mesh-controller/check_builds_test.go @@ -214,3 +214,28 @@ func TestAWalksLeaveNamesTheBuildsItsPullRequestsVerdictsMade(t *testing.T) { t.Fatalf("verdicts that could not be read named %+v", got.Builds) } } + +// A check is handed the builds the seat's verdicts name for the modules it builds, read once, and none when they +// cannot be read (novox/hq issue 703). +func TestACheckIsHandedTheBuildsTheVerdictsNameForItsModules(t *testing.T) { + was := checkedBuildsOfModules + t.Cleanup(func() { checkedBuildsOfModules = was }) + var asked [][]string + named := []link.CheckedBuild{{Pull: link.PullRef{Owner: "novox", Repo: "shop", Number: 7}, Check: "build-1", + Built: link.CheckBuilt{Module: "app", Key: "r1:1"}}} + checkedBuildsOfModules = func(_ context.Context, modules []string) ([]link.CheckedBuild, error) { + asked = append(asked, modules) + return named, nil + } + builds := []link.CheckBuild{{Module: "app"}, {Module: "web", Skip: "the merge deletes it"}} + if got := namedForTheCheck(t.Context(), builds); !reflect.DeepEqual(got, named) || + !reflect.DeepEqual(asked, [][]string{{"app"}}) { + t.Fatalf("the check is handed %+v, read %v", got, asked) + } + checkedBuildsOfModules = func(context.Context, []string) ([]link.CheckedBuild, error) { + return nil, errors.New("no bus") + } + if got := namedForTheCheck(t.Context(), builds); got != nil { + t.Fatalf("verdicts that could not be read named %+v", got) + } +} diff --git a/cmd/mesh-controller/checks.go b/cmd/mesh-controller/checks.go index b6243751..c43ed0e8 100644 --- a/cmd/mesh-controller/checks.go +++ b/cmd/mesh-controller/checks.go @@ -273,6 +273,7 @@ func checkRequestFor(ctx context.Context, open *stores, p link.PullUpdated, scop } } } + builds := checkBuildsOf(p, scope, entries, clone) return link.BuildRequest{ ID: link.NewBuildID(time.Now()), Repository: repository, @@ -282,10 +283,33 @@ func checkRequestFor(ctx context.Context, open *stores, p link.PullUpdated, scop Source: sourceOnSeat(source), Check: &link.CheckRequest{Owner: p.Owner, Repo: p.Repo, Number: p.Number, Base: p.Base, Paths: p.Paths, Beside: beside, Modules: scope.Modules, Dependents: scope.Dependents, New: scope.New, - Manifests: scope.Manifests, Judge: scope.Judge, Builds: checkBuildsOf(p, scope, entries, clone)}, + Manifests: scope.Manifests, Judge: scope.Judge, Builds: builds, Named: namedForTheCheck(ctx, builds)}, }, nil } +// namedForTheCheck are the builds the seat's verdicts name for the modules a check builds, read once for the check +// (novox/hq issue 703): what it may keep instead of building again. None when they cannot be read, and the check +// builds, as before. +func namedForTheCheck(ctx context.Context, builds []link.CheckBuild) []link.CheckedBuild { + var modules []string + for _, b := range builds { + if b.Skip == "" { + modules = append(modules, b.Module) + } + } + if len(modules) == 0 { + return nil + } + named, err := checkedBuildsOfModules(ctx, modules) + if err != nil { + fmt.Printf(" the checks' builds of %d module(s) could not be read, so the check builds them: %v\n", + len(modules), err) + // empty-on-error: said above; a check handed no named build builds every module, as before issue 703 + return nil + } + return named +} + // checkBuildsOf are the modules a merge of the change would build, in the walk's order, as the check builds them // for the walk to take (novox/hq ADR 0350 §1): a module built from the repository checked, at the branch it merges // into, from the merge the check makes; any other — a module standing on one — from its own repository at the diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index f28516fa..6a26dd2c 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -386,6 +386,21 @@ var checkedBuildsOf = func(ctx context.Context, pulls []link.PullRef) ([]link.Ch return link.ReadCheckBuilds(ctx, js, buildSeatHeld(ctx), pulls, time.Now().Add(-builder.CheckBuildsKept)) } +// checkedBuildsOfModules are the builds the build seat's checks of any pull request said they made, or kept, of +// those modules within the week check builds are kept (novox/hq issue 703): what a check may keep instead of +// building. A variable so a test needs no bus. +var checkedBuildsOfModules = func(ctx context.Context, modules []string) ([]link.CheckedBuild, error) { + if len(modules) == 0 { + return nil, nil + } + js, err := aBus() + if err != nil { + return nil, err + } + defer js.Close() + return link.ReadCheckBuildsOf(ctx, js, buildSeatHeld(ctx), modules, time.Now().Add(-builder.CheckBuildsKept)) +} + // reuseWithVerdicts is a walk's leave with the builds its pull requests' verdicts name: the only builds the builder // may take (ADR 0350 §2). When they cannot be read the leave names none, and the walk builds, saying why. func reuseWithVerdicts(ctx context.Context, r *link.ReuseAsk) *link.ReuseAsk { diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 5d203e09..b46118aa 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -374,7 +374,7 @@ func build(ctx context.Context, run Runner, publish Publisher, // only when asked — a check, a dry run, `build --ref` and `rebuild` never are. Its key is taken by the same // steps a build takes, stopped before anything is built; what it takes is copied by digest. if reuse != nil && reuse.keyFirst { - _, key, err := keyFor(ctx, run, publish, manifest, within, workspace, commit, credentials, artifacts, args, + keyed, key, err := keyFor(ctx, run, publish, manifest, within, workspace, commit, credentials, artifacts, args, held, npmrcPath, npmrc, seatBases, src, say) if err == nil && key.Key == "" { err = fmt.Errorf("%w: %s", errNoKey, key.Why) @@ -383,6 +383,11 @@ func build(ctx context.Context, run Runner, publish Publisher, say("reuse", "not built for the merge: %v", err) return Result{Commit: commit, NoKey: err.Error()}, err } + // **A check keeps a build of the same key a verdict names** (novox/hq issue 703): asked again when only + // the trunk moved, it builds nothing the earlier check built of an unchanged source. + if kept := keptCheckBuild(ctx, *reuse, manifest.Module, key.Key, len(artifacts), say); kept != nil { + built, src, reused = kept.Made, keyed, kept + } } else if reuse != nil && reuse.Scratch != nil { taken, keyed, why := takeACheckBuild(ctx, run, publish, *reuse, manifest, within, workspace, commit, credentials, artifacts, args, held, npmrcPath, npmrc, seatBases, src, onTrunk, say) @@ -492,6 +497,32 @@ func takeACheckBuild(ctx context.Context, run Runner, publish Publisher, reuse R return taken, keyed, "" } +// keptCheckBuild is the build a check keeps instead of building (novox/hq issue 703): the one kept under the +// module's key, when a verdict of the build seat names its key and every digest — the record alone is never trusted +// (ADR 0350 §2) — and it holds every artifact the module declares. Nil, and the check builds, otherwise. +func keptCheckBuild(ctx context.Context, reuse Reuse, module, key string, artifacts int, + say func(step, format string, args ...any)) *Reused { + if reuse.Scratch == nil || len(reuse.Named) == 0 { + return nil + } + found, ok, err := reuse.Scratch.Find(ctx, module, key) + switch { + case err != nil: + say("reuse", "the scratch namespace could not be read, so the check builds: %v", err) + return nil + case !ok: + return nil + case unnamed(reuse.Named, module, key, found) != "": + say("reuse", "a build is kept under %s and no verdict names it with its digests: built", twelve(key[3:])) + return nil + case len(found.Made) != artifacts: + return nil + } + kept := &Reused{From: found, Made: found.Made} + say("reuse", "kept, not built: %s", strings.Replace(kept.Words(), "reused from", "the build of", 1)) + return kept +} + // keyFor takes a build's reuse key by the steps a build takes, each artifact stopped before anything is built or // published (ADR 0350 §2): its own copy of the source inputs, so the build's are untouched. func keyFor(ctx context.Context, run Runner, publish Publisher, manifest catalogue.Manifest, diff --git a/internal/builder/check_builds.go b/internal/builder/check_builds.go index 6c9cd37d..5c8e3e50 100644 --- a/internal/builder/check_builds.go +++ b/internal/builder/check_builds.go @@ -37,6 +37,10 @@ type CheckBuilding struct { Held map[string]string Npmrc Npmrc Seats map[string]string + // Named are the builds the build seat's verdicts name for the modules this check builds, of any pull request, + // within the week check builds are kept (novox/hq issue 703): a build kept under a module's key is kept again, + // instead of built, only when one of them names its key and every digest. The record alone is never enough. + Named []NamedBuild } // CheckBuilt is one module a check built for its merge, as its verdict lists it (§1): its key and its artifacts, or @@ -48,6 +52,9 @@ type CheckBuilt struct { // NotBuilt says why it was not: no key, a build that failed, or a module after one of those. NotBuilt string Took time.Duration + // Kept is the check whose build this check kept instead of building, its key unchanged (novox/hq issue 703): + // the verdict names that build, by key and digest, as that check's. Empty when this check built it. + Kept string } // skipsTheBuilds is why a check builds nothing for its merge; empty when it builds. @@ -100,8 +107,8 @@ func buildForTheMerge(ctx context.Context, run Runner, spec CheckSpec, with Chec 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}, - with.Seats) + filepath.Join(workspace, "for-the-merge"), held, with.Npmrc, forge, log, &mirrored, + &Reuse{keyFirst: true, Scratch: with.Scratch, Named: with.Named}, with.Seats) built := CheckBuilt{Module: b.Module, Key: result.ReuseKey, Took: time.Since(began)} switch { case errors.Is(err, errNoKey): @@ -123,6 +130,17 @@ func buildForTheMerge(ctx context.Context, run Runner, spec CheckSpec, with Chec 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 { diff --git a/internal/builder/reuse.go b/internal/builder/reuse.go index 2def775d..1f8e01c1 100644 --- a/internal/builder/reuse.go +++ b/internal/builder/reuse.go @@ -494,11 +494,15 @@ func (r Registry) Discard(ctx context.Context, builds []CheckBuild) error { func (r Registry) letGo(ctx context.Context, checks, all map[string]checkIndex) (int, error) { store := r.store() stays := map[string]bool{} + // recorded are the records under a key another check still names (novox/hq issue 703): a later check that kept + // the build names it as the check's that built it, and the walk finds it by that record. + recorded := map[string]bool{} for tag, c := range all { if _, going := checks[tag]; going { continue } for _, b := range c.Builds { + recorded[b.Module+" "+b.Key+" "+b.Check] = true for _, m := range b.Made { stays[catalogue.Recorded(m.Reference)] = true } @@ -523,8 +527,10 @@ func (r Registry) letGo(ctx context.Context, checks, all map[string]checkIndex) } } // The record under the key, only while it is still this check's: a later check of the same key - // replaced it, and that one is its own check's to let go of. - if kept, found, err := r.Find(ctx, b.Module, b.Key); err == nil && found && kept.Check == c.Check { + // replaced it, and that one is its own check's to let go of. A build a check kept is that build's + // check's record, and goes with the last check naming it (novox/hq issue 703). + if kept, found, err := r.Find(ctx, b.Module, b.Key); err == nil && found && + (kept.Check == c.Check || kept.Check == b.Check) && !recorded[b.Module+" "+b.Key+" "+kept.Check] { if err := store.DeleteTagged(ctx, ScratchPrefix+b.Module, tagOf(b.Key)); err != nil && !errors.Is(err, artifacts.Gone) { ok = false failed = append(failed, fmt.Sprintf("%s's record: %v", b.Module, err)) diff --git a/internal/builder/reuse_test.go b/internal/builder/reuse_test.go index e3e4ec24..9f6ac590 100644 --- a/internal/builder/reuse_test.go +++ b/internal/builder/reuse_test.go @@ -6,6 +6,7 @@ import ( "errors" "net/http" "net/http/httptest" + "reflect" "slices" "strconv" "strings" @@ -625,3 +626,122 @@ func TestABuildTwoChecksShareGoesWithTheLastOfThem(t *testing.T) { t.Fatal("the build stayed after the last check naming it went") } } + +// aCheckAgain is a later check (build-2) of aPull building the module from a tree, with the builds the build seat's +// verdicts name for it (novox/hq issue 703). +func aCheckAgain(t *testing.T, scratch *aScratch, tree string, leave []NamedBuild) (CheckBuilt, *recorded) { + t.Helper() + r, run, workspace := aModuleAt(t, tree) + spec := CheckSpec{ID: "build-2", Owner: aPull.Owner, Repo: aPull.Repo, Number: aPull.Number, + Builds: []CheckBuildSpec{{Module: "meshboard", Repository: "https://forge.invalid/meshboard.git", Path: "mod", + Ref: "main"}}} + built := buildForTheMerge(t.Context(), run, spec, CheckBuilding{Publish: r, Scratch: scratch, Named: leave}, + workspace, "", "", CheckVerdict{Head: "fedcbafedcba", Trunk: "654321654321"}, GitCredential{}, nil, logging(nil)) + if len(built) != 1 || built[0].NotBuilt != "" || built[0].Key == "" { + t.Fatalf("the second check did not answer the module for its merge: %+v", built) + } + return built[0], r +} + +// A check asked again when only the trunk moved, whose key is unchanged, builds nothing: it keeps the build an +// earlier verdict names and names it by key and digest, so the walk can take it (novox/hq issue 703). +func TestASecondCheckWhoseKeyIsUnchangedBuildsNothingAndNamesTheEarlierBuild(t *testing.T) { + withVersion(t, "builder-1") + scratch := newScratch() + first := aCheckBuilt(t, scratch, "tree-one") + + again, r := aCheckAgain(t, scratch, "tree-one", named(first)) + if dockerBuilt(r) || len(r.images) > 0 || len(r.archives) > 0 { + t.Fatalf("the second check built again: ran %v, published %v %v", r.ran, r.images, r.archives) + } + if again.Key != first.Key || again.Kept != "build-1" || !reflect.DeepEqual(again.Made, first.Made) { + t.Fatalf("the second check names %+v, not the first check's build %+v", again, first) + } + // The walk takes it by what the second verdict names. + leave := named(again) + leave[0].Check = again.Kept + w, run, workspace := aModuleAt(t, "tree-one") + got, err := BuildReusing(t.Context(), run, w, Reuse{Scratch: scratch, Pulls: []PullRef{aPull}, Named: leave}, + "https://forge.invalid/meshboard.git", "mod", "main", workspace, nil, Npmrc{}, GitCredential{}, nil) + if err != nil || got.Reused == nil || dockerBuilt(w) { + t.Fatalf("the walk did not take the kept build: %v, why %q", err, got.Why) + } +} + +// A check whose key changed builds, as before. +func TestASecondCheckWhoseKeyChangedBuilds(t *testing.T) { + withVersion(t, "builder-1") + scratch := newScratch() + first := aCheckBuilt(t, scratch, "tree-one") + + again, r := aCheckAgain(t, scratch, "tree-two", named(first)) + if !dockerBuilt(r) || again.Kept != "" || again.Key == first.Key { + t.Fatalf("a check whose key changed did not build: %+v", again) + } +} + +// A scratch record no verdict names is never kept: the store takes writes from anything on its network, so a +// check builds rather than name a planted record (novox/hq issue 703, ADR 0350 §2). +func TestASecondCheckKeepsNoRecordAVerdictDoesNotName(t *testing.T) { + withVersion(t, "builder-1") + scratch := newScratch() + first := aCheckBuilt(t, scratch, "tree-one") + for name, leave := range map[string][]NamedBuild{ + "no verdict": nil, + "another digest": func() []NamedBuild { + n := named(first) + n[0].Digests["server"] = "sha256:" + strings.Repeat("e", 64) + return n + }(), + "another key": func() []NamedBuild { n := named(first); n[0].Key = "r1:" + strings.Repeat("f", 64); return n }(), + } { + t.Run(name, func(t *testing.T) { + again, r := aCheckAgain(t, scratch, "tree-one", leave) + if !dockerBuilt(r) || again.Kept != "" { + t.Fatalf("a record no verdict names was kept: %+v", again) + } + }) + } +} + +// A build a later check kept stays when the check that built it goes: its artifacts and its record, which the later +// check's verdict names (novox/hq issue 703, ADR 0350 §5). +func TestABuildALaterCheckKeptStaysWhenTheCheckThatBuiltItGoes(t *testing.T) { + m := &aTaggingRegistry{aRegistryOfRepositories: newRegistryOfRepositories(), tags: map[string]map[string]string{}} + server := httptest.NewServer(m.serve()) + defer server.Close() + address := strings.TrimPrefix(server.URL, "http://") + registry := Registry{Address: address} + ctx := t.Context() + now := time.Date(2026, 10, 14, 9, 0, 0, 0, time.UTC) + + body := `{"mediaType":"` + mediaManifestOCI + `","layers":[]}` + digest := digestOf([]byte(body)) + m.manifests["scratch/meshboard/server@"+digest] = []byte(body) + key := "r1:" + strings.Repeat("1", 64) + b := CheckBuild{Key: key, Module: "meshboard", Pull: aPull, Check: "build-1", At: now, + Made: []catalogue.Built{{Name: "server", Kind: "image", Reference: address + "/scratch/meshboard/server@" + digest}}} + if err := registry.Keep(ctx, aPull, "build-1", now, []CheckBuild{b}); err != nil { + t.Fatal(err) + } + // Another pull request's check keeps it: its own document names the build, which stays the first check's. + other := PullRef{Owner: "novox", Repo: "meshboard", Number: 8} + if err := registry.Keep(ctx, other, "build-2", now, []CheckBuild{b}); err != nil { + t.Fatal(err) + } + held := func() bool { _, ok := m.manifests["scratch/meshboard/server@"+digest]; return ok } + found := func() bool { _, ok, _ := registry.Find(ctx, "meshboard", key); return ok } + + if n, err := registry.Forget(ctx, aPull); err != nil || n != 1 { + t.Fatalf("forgot %d (%v)", n, err) + } + if !held() || !found() { + t.Fatalf("the build a later check names went with the check that built it: held %v, record %v", held(), found()) + } + if n, err := registry.Forget(ctx, other); err != nil || n != 1 { + t.Fatalf("forgot %d (%v)", n, err) + } + if held() || found() { + t.Fatalf("the build stayed after the last check naming it went: held %v, record %v", held(), found()) + } +} diff --git a/internal/link/build.go b/internal/link/build.go index 73384e9f..cf2a6898 100644 --- a/internal/link/build.go +++ b/internal/link/build.go @@ -164,6 +164,10 @@ type CheckRequest struct { // each, from the merge it judged, into the scratch namespace once it has judged and not failed, for the walk to // take. A build seat from before it builds nothing, and the walk builds as before. Builds []CheckBuild `json:"builds,omitempty"` + // Named are the builds the seat's verdicts name for those modules, of any pull request, within the week check + // builds are kept (novox/hq issue 703): the check keeps a build kept under a module's unchanged key, instead of + // building it again, only when one of these names its key and every digest. A build seat from before it builds. + Named []CheckedBuild `json:"named,omitempty"` // Try marks a branch tried before its pull request (novox/hq issue 686): the repository's own check for a // pushed branch or commit, asked by a builder through the controller's `try` verb. Its outcome is read from // the build's log (StepTried) and said to nobody: no status on the forge, nothing recorded or registered, @@ -212,6 +216,9 @@ type CheckBuilt struct { Made []MadeArtifact `json:"made,omitempty"` NotBuilt string `json:"not-built,omitempty"` Took string `json:"took,omitempty"` + // Kept names the check whose build this check kept instead of building, its key unchanged (novox/hq issue 703): + // a walk takes it as that check's build. Empty when this check built it. + Kept string `json:"kept,omitempty"` } // GroupMember is one other head of a delivery group's composed check. diff --git a/internal/link/check_builds_test.go b/internal/link/check_builds_test.go index 96ccbbf4..13f1100a 100644 --- a/internal/link/check_builds_test.go +++ b/internal/link/check_builds_test.go @@ -37,3 +37,33 @@ func TestNatsTheBuildsAWalkMayTakeAreTheChecksOutcomesOfItsPullRequests(t *testi t.Fatalf("read %+v", got) } } + +// A check reads the builds every pull request's checks made of the modules it builds, and a kept build is named as +// the check's that built it (novox/hq issue 703). +func TestNatsACheckReadsTheBuildsOfItsModulesAndAKeptBuildIsTheBuildersCheck(t *testing.T) { + js := aBus(t) + since := time.Now().Add(-time.Minute) + say := func(r BuildResult) { + body, _ := json.Marshal(r) + if _, err := js.Context().Publish(BuildOutcomeOf(TheBuildMachine), body); err != nil { + t.Fatal(err) + } + } + app := CheckBuilt{Module: "app", Key: "r1:1", Made: []MadeArtifact{{Name: "server", Reference: "s/app/server@sha256:a"}}} + web := CheckBuilt{Module: "web", Key: "r1:2", Made: []MadeArtifact{{Name: "server", Reference: "s/web/server@sha256:b"}}} + say(BuildResult{ID: "build-1", Checked: &CheckRequest{Owner: "novox", Repo: "shop", Number: 7}, + Check: &CheckOutcome{Verdict: "pass", Built: []CheckBuilt{app, web}}}) + kept := app + kept.Kept = "build-1" + say(BuildResult{ID: "build-2", Checked: &CheckRequest{Owner: "novox", Repo: "till", Number: 3}, + Check: &CheckOutcome{Verdict: "pass", Built: []CheckBuilt{kept}}}) + + got, err := ReadCheckBuildsOf(t.Context(), js, TheBuildMachine, []string{"app"}, since) + if err != nil { + t.Fatal(err) + } + if len(got) != 2 || got[0].Check != "build-1" || got[0].Pull.Repo != "till" || got[1].Check != "build-1" || + got[1].Built.Module != "app" { + t.Fatalf("read %+v", got) + } +} diff --git a/internal/link/queue.go b/internal/link/queue.go index 3bbbb8a2..b54e87d5 100644 --- a/internal/link/queue.go +++ b/internal/link/queue.go @@ -375,6 +375,26 @@ func ReadCheckBuilds(ctx context.Context, js *broker.JetStream, seat string, pul for _, p := range pulls { wanted[p] = true } + return readCheckBuilds(ctx, js, seat, since, func(p PullRef, _ CheckBuilt) bool { return wanted[p] }) +} + +// ReadCheckBuildsOf is every build the seat's checks of any pull request said they made, or kept, of those modules +// since a moment (novox/hq issue 703): what a check may keep instead of building. Newest first. +func ReadCheckBuildsOf(ctx context.Context, js *broker.JetStream, seat string, modules []string, since time.Time) ([]CheckedBuild, error) { + if len(modules) == 0 { + return nil, nil + } + wanted := map[string]bool{} + for _, m := range modules { + wanted[m] = true + } + return readCheckBuilds(ctx, js, seat, since, func(_ PullRef, b CheckBuilt) bool { return wanted[b.Module] }) +} + +// readCheckBuilds is every build the seat's checks said they made, or kept, that wanted takes. A kept build is named +// as the check's that built it (novox/hq issue 703): the record under its key is that check's. +func readCheckBuilds(ctx context.Context, js *broker.JetStream, seat string, since time.Time, + wanted func(PullRef, CheckBuilt) bool) ([]CheckedBuild, error) { subject := BuildOutcomeOf(seat) sub, err := js.Context().PullSubscribe(subject, "", nats.BindStream(broker.EventsStream), nats.StartTime(since), nats.AckNone()) @@ -402,12 +422,13 @@ func ReadCheckBuilds(ctx context.Context, js *broker.JetStream, seat string, pul continue } pull := PullRef{Owner: r.Checked.Owner, Repo: r.Checked.Repo, Number: r.Checked.Number} - if !wanted[pull] { - continue - } for _, b := range r.Check.Built { - if b.NotBuilt == "" && b.Key != "" { - out = append(out, CheckedBuild{Pull: pull, Check: r.ID, Built: b}) + if b.NotBuilt == "" && b.Key != "" && wanted(pull, b) { + check := r.ID + if b.Kept != "" { + check = b.Kept + } + out = append(out, CheckedBuild{Pull: pull, Check: check, Built: b}) } } } From 060f2a9ca5fd7e21994c9596d623c788098001ec Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 09:56:23 +0200 Subject: [PATCH 2/3] Build for the merge from a copy its tests never touch, and keep or take only the same head's build The gate and the repository's check mount the workspace read-write, so a build from that tree could hold what the tests wrote under a key that describes the merge. A check now clones the merge outside the workspace before anything of the change runs, and builds from that. A check keeps, and a walk takes, only a build an earlier check of the same pull request at the same head made: another pull request's build, or an earlier head's, is never registered. The walk reads the verdicts once (novox/hq issue 703, review of #328). --- cmd/mesh-controller/batches.go | 2 +- cmd/mesh-controller/check_builds_test.go | 83 +++++++++++++++--------- cmd/mesh-controller/checks.go | 32 ++++----- cmd/mesh-controller/release_plan.go | 83 ++++++++++++++++++------ internal/builder/builder.go | 27 ++++++-- internal/builder/check.go | 51 ++++++++++++++- internal/builder/check_builds.go | 10 +-- internal/builder/check_merge_test.go | 50 ++++++++++++++ internal/builder/reuse_test.go | 35 +++++++++- internal/inventory/plans.go | 3 + internal/link/build.go | 23 +++++-- internal/link/check_builds_test.go | 40 ++++++++---- internal/link/events.go | 11 ++-- internal/link/queue.go | 15 +---- 14 files changed, 355 insertions(+), 110 deletions(-) diff --git a/cmd/mesh-controller/batches.go b/cmd/mesh-controller/batches.go index c7065e80..c7c50ece 100644 --- a/cmd/mesh-controller/batches.go +++ b/cmd/mesh-controller/batches.go @@ -688,7 +688,7 @@ func spelledOf(m inventory.BatchedMerge) string { func namedOf(m inventory.BatchedMerge, repository string) inventory.PlanMerge { k := announcedOf(m) return inventory.PlanMerge{Repository: repository, Commit: m.Commit, Number: k.Number, Title: k.Title, Moves: k.Moves, - Merged: m.Merged, Heard: m.Heard} + Head: k.HeadSHA, Merged: m.Merged, Heard: m.Heard} } // laterMerge says a was merged after b: by the forge's merge time, then by when each was heard. diff --git a/cmd/mesh-controller/check_builds_test.go b/cmd/mesh-controller/check_builds_test.go index c5dc716c..3dc2b516 100644 --- a/cmd/mesh-controller/check_builds_test.go +++ b/cmd/mesh-controller/check_builds_test.go @@ -190,52 +190,75 @@ func TestAReusedCheckBuildIsRegisteredByItsDigestAndGatedAsABuiltOne(t *testing. } } -// A walk's leave names the builds its own pull requests' verdicts made, and none when they cannot be read: the -// builder takes nothing a verdict does not name (novox/hq ADR 0350 §2). +// A walk's leave names the builds its own pull requests' verdicts made at the head each merge merged — never an +// earlier head's — read once per walk, and none when they cannot be read: the builder takes nothing a verdict does +// not name (novox/hq ADR 0350 §2, issue 703). func TestAWalksLeaveNamesTheBuildsItsPullRequestsVerdictsMade(t *testing.T) { was := checkedBuildsOf - t.Cleanup(func() { checkedBuildsOf = was }) - var asked []link.PullRef - named := []link.CheckedBuild{{Pull: link.PullRef{Owner: "novox", Repo: "shop", Number: 7}, Check: "build-1", - Built: link.CheckBuilt{Module: "app", Key: "r1:1"}}} + forgetWalkVerdicts() + t.Cleanup(func() { checkedBuildsOf = was; forgetWalkVerdicts() }) + var asked [][]link.PullRef + pull := link.PullRef{Owner: "novox", Repo: "shop", Number: 7} + atHead := link.CheckedBuild{Pull: pull, Check: "build-2", Head: "bbbbbbbbbbbb", Built: link.CheckBuilt{Module: "app", Key: "r1:2"}} + earlier := link.CheckedBuild{Pull: pull, Check: "build-1", Head: "aaaaaaaaaaaa", Built: link.CheckBuilt{Module: "app", Key: "r1:1"}} checkedBuildsOf = func(_ context.Context, pulls []link.PullRef) ([]link.CheckedBuild, error) { - asked = pulls - return named, nil + asked = append(asked, pulls) + return []link.CheckedBuild{atHead, earlier}, nil } - pulls := []link.PullRef{{Owner: "novox", Repo: "shop", Number: 7}} - if got := reuseWithVerdicts(t.Context(), &link.ReuseAsk{Pulls: pulls}); !reflect.DeepEqual(got.Builds, named) || - !reflect.DeepEqual(asked, pulls) { - t.Fatalf("the leave names %+v, read for %+v", got.Builds, asked) + pulls := []link.PullRef{pull} + heads := map[link.PullRef]string{pull: "bbbbbbbbbbbb0123456789"} + for range 3 { + if got := reuseWithVerdicts(t.Context(), "plan-1", &link.ReuseAsk{Pulls: pulls}, heads); !reflect.DeepEqual(got.Builds, + []link.CheckedBuild{atHead}) { + t.Fatalf("the leave names %+v, not only the merged head's build", got.Builds) + } + } + if !reflect.DeepEqual(asked, [][]link.PullRef{pulls}) { + t.Fatalf("the verdicts were read %d times for one walk: %v", len(asked), asked) + } + // A merge whose head was not said takes nothing. + if got := reuseWithVerdicts(t.Context(), "plan-1", &link.ReuseAsk{Pulls: pulls}, nil); got.Builds != nil { + t.Fatalf("a merge of no said head names %+v", got.Builds) } checkedBuildsOf = func(context.Context, []link.PullRef) ([]link.CheckedBuild, error) { return nil, errors.New("no bus") } - if got := reuseWithVerdicts(t.Context(), &link.ReuseAsk{Pulls: pulls}); got.Builds != nil { + if got := reuseWithVerdicts(t.Context(), "plan-2", &link.ReuseAsk{Pulls: pulls}, heads); got.Builds != nil { t.Fatalf("verdicts that could not be read named %+v", got.Builds) } } -// A check is handed the builds the seat's verdicts name for the modules it builds, read once, and none when they -// cannot be read (novox/hq issue 703). -func TestACheckIsHandedTheBuildsTheVerdictsNameForItsModules(t *testing.T) { - was := checkedBuildsOfModules - t.Cleanup(func() { checkedBuildsOfModules = was }) - var asked [][]string - named := []link.CheckedBuild{{Pull: link.PullRef{Owner: "novox", Repo: "shop", Number: 7}, Check: "build-1", - Built: link.CheckBuilt{Module: "app", Key: "r1:1"}}} - checkedBuildsOfModules = func(_ context.Context, modules []string) ([]link.CheckedBuild, error) { - asked = append(asked, modules) - return named, nil +// A check is handed only the builds earlier checks of its own pull request at its own head made: another pull +// request's build, or an earlier head's, is never kept (novox/hq issue 703). None when they cannot be read. +func TestACheckIsHandedOnlyItsOwnHeadsBuilds(t *testing.T) { + was := checkedBuildsOf + t.Cleanup(func() { checkedBuildsOf = was }) + pull := link.PullRef{Owner: "novox", Repo: "shop", Number: 7} + mine := link.CheckedBuild{Pull: pull, Check: "build-1", Head: "bbbbbbbbbbbb", Built: link.CheckBuilt{Module: "app", Key: "r1:1"}} + earlier := link.CheckedBuild{Pull: pull, Check: "build-0", Head: "aaaaaaaaaaaa", Built: link.CheckBuilt{Module: "app", Key: "r1:1"}} + another := link.CheckedBuild{Pull: link.PullRef{Owner: "novox", Repo: "shop", Number: 8}, Check: "build-9", + Head: "bbbbbbbbbbbb", Built: link.CheckBuilt{Module: "app", Key: "r1:1"}} + var asked [][]link.PullRef + checkedBuildsOf = func(_ context.Context, pulls []link.PullRef) ([]link.CheckedBuild, error) { + asked = append(asked, pulls) + return []link.CheckedBuild{mine, earlier, another}, nil } - builds := []link.CheckBuild{{Module: "app"}, {Module: "web", Skip: "the merge deletes it"}} - if got := namedForTheCheck(t.Context(), builds); !reflect.DeepEqual(got, named) || - !reflect.DeepEqual(asked, [][]string{{"app"}}) { - t.Fatalf("the check is handed %+v, read %v", got, asked) + p := link.PullUpdated{Owner: "novox", Repo: "shop", Number: 7, Commit: "bbbbbbbbbbbb0123456789"} + builds := []link.CheckBuild{{Module: "app"}} + if got := namedForTheCheck(t.Context(), p, builds); !reflect.DeepEqual(got, []link.CheckedBuild{mine}) || + !reflect.DeepEqual(asked, [][]link.PullRef{{pull}}) { + t.Fatalf("the check is handed %+v, read for %v", got, asked) } - checkedBuildsOfModules = func(context.Context, []string) ([]link.CheckedBuild, error) { + checkedBuildsOf = func(context.Context, []link.PullRef) ([]link.CheckedBuild, error) { return nil, errors.New("no bus") } - if got := namedForTheCheck(t.Context(), builds); got != nil { + if got := namedForTheCheck(t.Context(), p, builds); got != nil { t.Fatalf("verdicts that could not be read named %+v", got) } } + +func forgetWalkVerdicts() { + walkVerdicts.Lock() + defer walkVerdicts.Unlock() + walkVerdicts.read = map[string]verdictsRead{} +} diff --git a/cmd/mesh-controller/checks.go b/cmd/mesh-controller/checks.go index c43ed0e8..fbae0304 100644 --- a/cmd/mesh-controller/checks.go +++ b/cmd/mesh-controller/checks.go @@ -283,30 +283,32 @@ func checkRequestFor(ctx context.Context, open *stores, p link.PullUpdated, scop Source: sourceOnSeat(source), Check: &link.CheckRequest{Owner: p.Owner, Repo: p.Repo, Number: p.Number, Base: p.Base, Paths: p.Paths, Beside: beside, Modules: scope.Modules, Dependents: scope.Dependents, New: scope.New, - Manifests: scope.Manifests, Judge: scope.Judge, Builds: builds, Named: namedForTheCheck(ctx, builds)}, + Manifests: scope.Manifests, Judge: scope.Judge, Builds: builds, Named: namedForTheCheck(ctx, p, builds)}, }, nil } -// namedForTheCheck are the builds the seat's verdicts name for the modules a check builds, read once for the check -// (novox/hq issue 703): what it may keep instead of building again. None when they cannot be read, and the check -// builds, as before. -func namedForTheCheck(ctx context.Context, builds []link.CheckBuild) []link.CheckedBuild { - var modules []string - for _, b := range builds { - if b.Skip == "" { - modules = append(modules, b.Module) - } - } - if len(modules) == 0 { +// namedForTheCheck are the builds earlier checks of the same pull request at the same head named, read once for the +// check (novox/hq issue 703): asked again because only the trunk moved, the check keeps what they built of a key +// still the same. Another pull request's build, or one of an earlier head, is never kept. None when they cannot be +// read, and the check builds, as before. +func namedForTheCheck(ctx context.Context, p link.PullUpdated, builds []link.CheckBuild) []link.CheckedBuild { + if len(builds) == 0 || p.Commit == "" { return nil } - named, err := checkedBuildsOfModules(ctx, modules) + pull := link.PullRef{Owner: p.Owner, Repo: p.Repo, Number: p.Number} + read, err := checkedBuildsOf(ctx, []link.PullRef{pull}) if err != nil { - fmt.Printf(" the checks' builds of %d module(s) could not be read, so the check builds them: %v\n", - len(modules), err) + fmt.Printf(" the earlier checks' builds of %s/%s#%d could not be read, so the check builds: %v\n", + p.Owner, p.Repo, p.Number, err) // empty-on-error: said above; a check handed no named build builds every module, as before issue 703 return nil } + var named []link.CheckedBuild + for _, b := range read { + if b.Pull == pull && link.SameHead(b.Head, p.Commit) { + named = append(named, b) + } + } return named } diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 6a26dd2c..8a6d27e1 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -9,6 +9,7 @@ import ( "slices" "sort" "strings" + "sync" "time" "github.com/nats-io/nats.go" @@ -386,30 +387,73 @@ var checkedBuildsOf = func(ctx context.Context, pulls []link.PullRef) ([]link.Ch return link.ReadCheckBuilds(ctx, js, buildSeatHeld(ctx), pulls, time.Now().Add(-builder.CheckBuildsKept)) } -// checkedBuildsOfModules are the builds the build seat's checks of any pull request said they made, or kept, of -// those modules within the week check builds are kept (novox/hq issue 703): what a check may keep instead of -// building. A variable so a test needs no bus. -var checkedBuildsOfModules = func(ctx context.Context, modules []string) ([]link.CheckedBuild, error) { - if len(modules) == 0 { - return nil, nil +// mergedHeadsOf are the head commits the walk's merges merged, by pull request (novox/hq issue 703): a pull request +// merged twice in one walk, or whose head the announcement did not say, has none, and takes no check's build. +func mergedHeadsOf(p *inventory.Plan) map[link.PullRef]string { + heads := map[link.PullRef]string{} + if p.Delivery == nil { + return heads } - js, err := aBus() - if err != nil { - return nil, err + seen := map[link.PullRef]bool{} + for _, m := range p.Delivery.Merges { + pull, ok := pullOf(m) + if !ok { + continue + } + if seen[pull] && heads[pull] != m.Head { + heads[pull] = "" + continue + } + seen[pull] = true + heads[pull] = m.Head } - defer js.Close() - return link.ReadCheckBuildsOf(ctx, js, buildSeatHeld(ctx), modules, time.Now().Add(-builder.CheckBuildsKept)) + return heads +} + +// walkVerdicts are the checks' builds a walk read, by the walk's plan: read once per walk (novox/hq issue 649), not +// once per module it asks for. Kept a day; a walk lasts minutes. +var walkVerdicts = struct { + sync.Mutex + read map[string]verdictsRead +}{read: map[string]verdictsRead{}} + +type verdictsRead struct { + at time.Time + builds []link.CheckedBuild } // reuseWithVerdicts is a walk's leave with the builds its pull requests' verdicts name: the only builds the builder -// may take (ADR 0350 §2). When they cannot be read the leave names none, and the walk builds, saying why. -func reuseWithVerdicts(ctx context.Context, r *link.ReuseAsk) *link.ReuseAsk { - builds, err := checkedBuildsOf(ctx, r.Pulls) - if err != nil { - fmt.Printf(" the checks' builds of %d pull request(s) could not be read, so the walk builds: %v\n", len(r.Pulls), err) - return r +// may take (ADR 0350 §2), and **only those of a check of the head each merge merged** (novox/hq issue 703): a build +// a check made at an earlier head of the same pull request is never registered. Read once per walk. When they +// cannot be read the leave names none, and the walk builds, saying why. +func reuseWithVerdicts(ctx context.Context, walk string, r *link.ReuseAsk, heads map[link.PullRef]string) *link.ReuseAsk { + walkVerdicts.Lock() + read, known := walkVerdicts.read[walk] + walkVerdicts.Unlock() + if !known || walk == "" { + builds, err := checkedBuildsOf(ctx, r.Pulls) + if err != nil { + fmt.Printf(" the checks' builds of %d pull request(s) could not be read, so the walk builds: %v\n", len(r.Pulls), err) + return r + } + read = verdictsRead{at: time.Now(), builds: builds} + if walk != "" { + walkVerdicts.Lock() + for id, w := range walkVerdicts.read { + if time.Since(w.at) > 24*time.Hour { + delete(walkVerdicts.read, id) + } + } + walkVerdicts.read[walk] = read + walkVerdicts.Unlock() + } + } + r.Builds = nil + for _, b := range read.builds { + if link.SameHead(b.Head, heads[b.Pull]) { + r.Builds = append(r.Builds, b) + } } - r.Builds = builds return r } @@ -452,7 +496,8 @@ func askModule(ctx context.Context, p *inventory.Plan, name string, byName map[s } // **The walk takes the check's build of the same key** (novox/hq ADR 0350 §2): its ask says so, with the pull // requests it answers, whose checks name the part of a differing key. - id, err := askABuild(withReuse(ctx, reuseWithVerdicts(ctx, reuseAskOf(p))), source, e.Source.Path, ref) + id, err := askABuild(withReuse(ctx, reuseWithVerdicts(ctx, p.ID, reuseAskOf(p), mergedHeadsOf(p))), source, + e.Source.Path, ref) if err != nil { state.State = "failed" state.Why = err.Error() diff --git a/internal/builder/builder.go b/internal/builder/builder.go index b46118aa..7c6a3ac5 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -162,6 +162,10 @@ type Reuse struct { // keyFirst is a check's build (§1): its key is taken before anything is built, and a module with none is not // built at all — nothing could ever take it. keyFirst bool + // pull and head are the check's own pull request and the head it judged (novox/hq issue 703): it keeps only a + // build an earlier check of the same pull request at the same head made. + pull PullRef + head string } // errNoKey is a check's build of a module whose source does not pin it: not built (ADR 0350 §1). @@ -498,11 +502,22 @@ func takeACheckBuild(ctx context.Context, run Runner, publish Publisher, reuse R } // keptCheckBuild is the build a check keeps instead of building (novox/hq issue 703): the one kept under the -// module's key, when a verdict of the build seat names its key and every digest — the record alone is never trusted -// (ADR 0350 §2) — and it holds every artifact the module declares. Nil, and the check builds, otherwise. +// module's key by an earlier check of the same pull request at the same head — only the trunk moved — when a verdict +// of the build seat names its key and every digest — the record alone is never trusted (ADR 0350 §2) — and it holds +// every artifact the module declares. Another pull request's build, or an earlier head's, is never kept. Nil, and +// the check builds, otherwise. func keptCheckBuild(ctx context.Context, reuse Reuse, module, key string, artifacts int, say func(step, format string, args ...any)) *Reused { - if reuse.Scratch == nil || len(reuse.Named) == 0 { + if reuse.Scratch == nil || reuse.head == "" { + return nil + } + var same []NamedBuild + for _, n := range reuse.Named { + if n.Pull == reuse.pull { + same = append(same, n) + } + } + if len(same) == 0 { return nil } found, ok, err := reuse.Scratch.Find(ctx, module, key) @@ -512,7 +527,11 @@ func keptCheckBuild(ctx context.Context, reuse Reuse, module, key string, artifa return nil case !ok: return nil - case unnamed(reuse.Named, module, key, found) != "": + case found.Pull != reuse.pull || found.Head == "" || twelve(found.Head) != twelve(reuse.head): + say("reuse", "the build kept under %s is %s, not this pull request's at this head: built", twelve(key[3:]), + found.Said()) + return nil + case unnamed(same, module, key, found) != "": say("reuse", "a build is kept under %s and no verdict names it with its digests: built", twelve(key[3:])) return nil case len(found.Made) != artifacts: diff --git a/internal/builder/check.go b/internal/builder/check.go index a2107499..1b2648fe 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -354,6 +354,22 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry v.Head, v.Trunk, v.Merge, v.Beside, v.Members = judged.Head, judged.Trunk, judged.Merge, judged.Beside, judged.Members return v } + // **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 := "", "" + if len(spec.Builds) > 0 && spec.BuildWith != nil && len(spec.Group) == 0 { + at := judged.Merge + if at == "" { + at = spec.Ref + } + building, buildingTree, err = checkoutForTheBuilds(ctx, run, tree, at, workspace) + if err != nil { + return CheckVerdict{}, err + } + defer os.RemoveAll(building) + } // **The base branch's check judges the change** (novox/hq issue 310), never the one the change brings. judging, err := TheCheckThatJudges(ctx, tree, spec.Base) if err != nil { @@ -610,7 +626,7 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry say("check", "%s", why) } else { began := time.Now() - v.Built = buildForTheMerge(ctx, run, spec, *spec.BuildWith, workspace, tree, judged.Merge, judged, forge, + 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)) @@ -623,6 +639,39 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry return named(v), nil } +// 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 +// workspace's objects cannot reach it. +func checkoutForTheBuilds(ctx context.Context, run Runner, tree, at, workspace string) (string, string, error) { + dir, err := os.MkdirTemp("", "mesh-check-builds-") + if err != nil { + return "", "", err + } + if withinDir(workspace, dir) { + os.RemoveAll(dir) + return "", "", fmt.Errorf("the temporary directory %s is inside the workspace %s, which a check's containers "+ + "mount: the merge is not built from where its tests could change it", dir, workspace) + } + commit, err := run(ctx, tree, "git", "rev-parse", "--verify", at+"^{commit}") + if err != nil { + os.RemoveAll(dir) + return "", "", fmt.Errorf("the commit to build for the merge, %s: %w", at, err) + } + commit = strings.TrimSpace(commit) + if _, err := run(ctx, tree, "git", "update-ref", "refs/heads/mesh-check-builds", commit); err != nil { + os.RemoveAll(dir) + return "", "", err + } + cloned := filepath.Join(dir, "merged") + if _, err := run(ctx, dir, "git", "clone", "--quiet", "--no-local", "--no-checkout", "--branch", + "mesh-check-builds", tree, cloned); err != nil { + os.RemoveAll(dir) + return "", "", fmt.Errorf("cannot copy the merge to build it: %w", err) + } + return dir, cloned, nil +} + // builtCount is how many of a check's builds for its merge were built. func builtCount(built []CheckBuilt) int { n := 0 diff --git a/internal/builder/check_builds.go b/internal/builder/check_builds.go index 5c8e3e50..7c0223ca 100644 --- a/internal/builder/check_builds.go +++ b/internal/builder/check_builds.go @@ -37,9 +37,10 @@ type CheckBuilding struct { Held map[string]string Npmrc Npmrc Seats map[string]string - // Named are the builds the build seat's verdicts name for the modules this check builds, of any pull request, - // within the week check builds are kept (novox/hq issue 703): a build kept under a module's key is kept again, - // instead of built, only when one of them names its key and every digest. The record alone is never enough. + // Named are the builds the build seat's verdicts of earlier checks of this pull request at this head name + // (novox/hq issue 703): a build kept under a module's key is kept again, instead of built, only when one of them + // names its key and every digest, and the build is this pull request's at this head. The record alone is never + // enough. Named []NamedBuild } @@ -108,7 +109,8 @@ func buildForTheMerge(ctx context.Context, run Runner, spec CheckSpec, with Chec 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}, with.Seats) + &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): diff --git a/internal/builder/check_merge_test.go b/internal/builder/check_merge_test.go index a35fe888..86ad2da0 100644 --- a/internal/builder/check_merge_test.go +++ b/internal/builder/check_merge_test.go @@ -210,3 +210,53 @@ func TestACheckMergeRunsNoHook(t *testing.T) { t.Fatalf("a hook ran when the check made its merge: %s", said) } } + +// What a check's tests change in the workspace never reaches what it builds for the merge (novox/hq issue 703): the +// builds are made from a copy of the merge, outside the workspace, cloned before anything of the change runs. +func TestWhatTheTestsChangeInTheWorkspaceNeverReachesTheBuild(t *testing.T) { + repo, head, _ := aTrunkAndABranch(t, map[string]string{"main.go": "package main // the merge\n"}, + map[string]string{"trunk-only": "t"}, map[string]string{"head-only": "h"}) + workspace := t.TempDir() + tree := filepath.Join(workspace, "check", "checked") + if out, err := exec.Command("git", "clone", "--quiet", repo, tree).CombinedOutput(); err != nil { + t.Fatalf("%v\n%s", err, out) + } + dir, cloned, err := checkoutForTheBuilds(t.Context(), Command, tree, head, workspace) + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + if withinDir(workspace, cloned) { + t.Fatalf("the builds' checkout %s is inside the workspace %s the tests mount", cloned, workspace) + } + + // The tests rewrite the tree, its branch and every object it holds. + if err := os.WriteFile(filepath.Join(tree, "main.go"), []byte("package main // planted\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := filepath.WalkDir(filepath.Join(tree, ".git", "objects"), func(path string, d os.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + _ = os.Chmod(path, 0o644) + return os.WriteFile(path, []byte("planted"), 0o644) + }); err != nil { + t.Fatal(err) + } + + // The build clones the merge from its own checkout, as buildForTheMerge does, and reads what the merge holds. + build := filepath.Join(t.TempDir(), "source") + if out, err := exec.Command("git", "clone", "--quiet", cloned, build).CombinedOutput(); err != nil { + t.Fatalf("%v\n%s", err, out) + } + if out, err := exec.Command("git", "-C", build, "checkout", "--quiet", head).CombinedOutput(); err != nil { + t.Fatalf("%v\n%s", err, out) + } + body, err := os.ReadFile(filepath.Join(build, "main.go")) + if err != nil { + t.Fatal(err) + } + if string(body) != "package main // the merge\n" { + t.Fatalf("the build read %q: what the tests wrote reached it", body) + } +} diff --git a/internal/builder/reuse_test.go b/internal/builder/reuse_test.go index 9f6ac590..785c2615 100644 --- a/internal/builder/reuse_test.go +++ b/internal/builder/reuse_test.go @@ -630,13 +630,19 @@ func TestABuildTwoChecksShareGoesWithTheLastOfThem(t *testing.T) { // aCheckAgain is a later check (build-2) of aPull building the module from a tree, with the builds the build seat's // verdicts name for it (novox/hq issue 703). func aCheckAgain(t *testing.T, scratch *aScratch, tree string, leave []NamedBuild) (CheckBuilt, *recorded) { + t.Helper() + return aCheckOf(t, scratch, aPull, "abcdefabcdef", tree, leave) +} + +// aCheckOf is a later check (build-2) of a pull request at a head, the trunk moved since build-1. +func aCheckOf(t *testing.T, scratch *aScratch, pull PullRef, head, tree string, leave []NamedBuild) (CheckBuilt, *recorded) { t.Helper() r, run, workspace := aModuleAt(t, tree) - spec := CheckSpec{ID: "build-2", Owner: aPull.Owner, Repo: aPull.Repo, Number: aPull.Number, + spec := CheckSpec{ID: "build-2", Owner: pull.Owner, Repo: pull.Repo, Number: pull.Number, Builds: []CheckBuildSpec{{Module: "meshboard", Repository: "https://forge.invalid/meshboard.git", Path: "mod", Ref: "main"}}} built := buildForTheMerge(t.Context(), run, spec, CheckBuilding{Publish: r, Scratch: scratch, Named: leave}, - workspace, "", "", CheckVerdict{Head: "fedcbafedcba", Trunk: "654321654321"}, GitCredential{}, nil, logging(nil)) + workspace, "", "", CheckVerdict{Head: head, Trunk: "654321654321"}, GitCredential{}, nil, logging(nil)) if len(built) != 1 || built[0].NotBuilt != "" || built[0].Key == "" { t.Fatalf("the second check did not answer the module for its merge: %+v", built) } @@ -745,3 +751,28 @@ func TestABuildALaterCheckKeptStaysWhenTheCheckThatBuiltItGoes(t *testing.T) { t.Fatalf("the build stayed after the last check naming it went: held %v, record %v", held(), found()) } } + +// Only an earlier check of the same pull request at the same head is kept from: another head of it, or another pull +// request whose verdict names the same key and digests, builds (novox/hq issue 703). +func TestACheckKeepsNoBuildOfAnotherHeadOrPullRequest(t *testing.T) { + withVersion(t, "builder-1") + other := PullRef{Owner: "novox", Repo: "meshboard", Number: 8} + for name, c := range map[string]struct { + pull PullRef + head string + }{ + "another head": {aPull, "999999999999"}, + "another pull request": {other, "abcdefabcdef"}, + } { + t.Run(name, func(t *testing.T) { + scratch := newScratch() + first := aCheckBuilt(t, scratch, "tree-one") + leave := named(first) + leave[0].Pull = c.pull // as if its own verdict named it + again, r := aCheckOf(t, scratch, c.pull, c.head, "tree-one", leave) + if !dockerBuilt(r) || again.Kept != "" { + t.Fatalf("a build of %s was kept: %+v", name, again) + } + }) + } +} diff --git a/internal/inventory/plans.go b/internal/inventory/plans.go index c9f712cf..c6f6d323 100644 --- a/internal/inventory/plans.go +++ b/internal/inventory/plans.go @@ -104,6 +104,9 @@ type PlanMerge struct { Number int `json:"number,omitempty"` Title string `json:"title,omitempty"` Moves []string `json:"moves,omitempty"` + // Head is the pull request's head commit the forge merged (novox/hq issue 703): the walk takes only a build a + // check of that head made. Empty where the announcement said none, and then it takes none. + Head string `json:"head,omitempty"` // Merged is when the forge made the merge and Heard when the controller heard it (novox/hq ADR 0282): where // its delivery time starts. Zero for a merge named before they were kept. Merged time.Time `json:"merged,omitzero"` diff --git a/internal/link/build.go b/internal/link/build.go index cf2a6898..1522ddaf 100644 --- a/internal/link/build.go +++ b/internal/link/build.go @@ -114,11 +114,23 @@ type ReuseAsk struct { // CheckedBuild is one build a check's verdict names: the pull request it checked, its ask, and the build. type CheckedBuild struct { - Pull PullRef `json:"pull"` - Check string `json:"check"` + Pull PullRef `json:"pull"` + Check string `json:"check"` + // Head is the pull request's head the check judged, as its verdict says it (twelve characters): a build is + // taken or kept only by a check of the same head (novox/hq issue 703). + Head string `json:"head,omitempty"` Built CheckBuilt `json:"built"` } +// SameHead says two spellings of a commit name the same one: both said, one the other's beginning, at least +// twelve characters of it (a verdict says twelve). +func SameHead(a, b string) bool { + if len(a) > len(b) { + a, b = b, a + } + return len(a) >= 12 && strings.HasPrefix(b, a) +} + // PullRef names a pull request. type PullRef struct { Owner string `json:"owner"` @@ -164,9 +176,10 @@ type CheckRequest struct { // each, from the merge it judged, into the scratch namespace once it has judged and not failed, for the walk to // take. A build seat from before it builds nothing, and the walk builds as before. Builds []CheckBuild `json:"builds,omitempty"` - // Named are the builds the seat's verdicts name for those modules, of any pull request, within the week check - // builds are kept (novox/hq issue 703): the check keeps a build kept under a module's unchanged key, instead of - // building it again, only when one of these names its key and every digest. A build seat from before it builds. + // Named are the builds the seat's verdicts of earlier checks of this pull request at this head name, within the + // week check builds are kept (novox/hq issue 703): asked again because only the trunk moved, the check keeps a + // build kept under a module's unchanged key, instead of building it again, only when one of these names its key + // and every digest. Never another pull request's, nor an earlier head's. A build seat from before it builds. Named []CheckedBuild `json:"named,omitempty"` // Try marks a branch tried before its pull request (novox/hq issue 686): the repository's own check for a // pushed branch or commit, asked by a builder through the controller's `try` verb. Its outcome is read from diff --git a/internal/link/check_builds_test.go b/internal/link/check_builds_test.go index 13f1100a..ca938fe2 100644 --- a/internal/link/check_builds_test.go +++ b/internal/link/check_builds_test.go @@ -38,9 +38,9 @@ func TestNatsTheBuildsAWalkMayTakeAreTheChecksOutcomesOfItsPullRequests(t *testi } } -// A check reads the builds every pull request's checks made of the modules it builds, and a kept build is named as -// the check's that built it (novox/hq issue 703). -func TestNatsACheckReadsTheBuildsOfItsModulesAndAKeptBuildIsTheBuildersCheck(t *testing.T) { +// A build is read with the head its check judged, and a kept build is named as the check's that built it (novox/hq +// issue 703). +func TestNatsAKeptBuildIsTheBuildersCheckAndEveryBuildSaysItsHead(t *testing.T) { js := aBus(t) since := time.Now().Add(-time.Minute) say := func(r BuildResult) { @@ -49,21 +49,39 @@ func TestNatsACheckReadsTheBuildsOfItsModulesAndAKeptBuildIsTheBuildersCheck(t * t.Fatal(err) } } + mine := &CheckRequest{Owner: "novox", Repo: "shop", Number: 7} app := CheckBuilt{Module: "app", Key: "r1:1", Made: []MadeArtifact{{Name: "server", Reference: "s/app/server@sha256:a"}}} - web := CheckBuilt{Module: "web", Key: "r1:2", Made: []MadeArtifact{{Name: "server", Reference: "s/web/server@sha256:b"}}} - say(BuildResult{ID: "build-1", Checked: &CheckRequest{Owner: "novox", Repo: "shop", Number: 7}, - Check: &CheckOutcome{Verdict: "pass", Built: []CheckBuilt{app, web}}}) + say(BuildResult{ID: "build-1", Checked: mine, Check: &CheckOutcome{Verdict: "pass", Head: "aaaaaaaaaaaa", + Built: []CheckBuilt{app}}}) kept := app kept.Kept = "build-1" - say(BuildResult{ID: "build-2", Checked: &CheckRequest{Owner: "novox", Repo: "till", Number: 3}, - Check: &CheckOutcome{Verdict: "pass", Built: []CheckBuilt{kept}}}) + say(BuildResult{ID: "build-2", Checked: mine, Check: &CheckOutcome{Verdict: "pass", Head: "aaaaaaaaaaaa", + Built: []CheckBuilt{kept}}}) - got, err := ReadCheckBuildsOf(t.Context(), js, TheBuildMachine, []string{"app"}, since) + got, err := ReadCheckBuilds(t.Context(), js, TheBuildMachine, []PullRef{{Owner: "novox", Repo: "shop", Number: 7}}, since) if err != nil { t.Fatal(err) } - if len(got) != 2 || got[0].Check != "build-1" || got[0].Pull.Repo != "till" || got[1].Check != "build-1" || - got[1].Built.Module != "app" { + if len(got) != 2 || got[0].Check != "build-1" || got[0].Built.Kept != "build-1" || got[0].Head != "aaaaaaaaaaaa" || + got[1].Check != "build-1" || got[1].Head != "aaaaaaaaaaaa" { t.Fatalf("read %+v", got) } } + +// Two spellings of a head are the same head only when both say at least twelve characters of it. +func TestSameHeadNeedsTwelveCharactersOfBoth(t *testing.T) { + for _, c := range []struct { + a, b string + same bool + }{ + {"aaaaaaaaaaaa", "aaaaaaaaaaaabbbb", true}, + {"aaaaaaaaaaaabbbb", "aaaaaaaaaaaa", true}, + {"aaaaaaaaaaaa", "aaaaaaaaaaab", false}, + {"", "", false}, + {"aaaa", "aaaaaaaaaaaa", false}, + } { + if SameHead(c.a, c.b) != c.same { + t.Errorf("SameHead(%q, %q) is %v", c.a, c.b, !c.same) + } + } +} diff --git a/internal/link/events.go b/internal/link/events.go index 48b7ece6..edbcfbc3 100644 --- a/internal/link/events.go +++ b/internal/link/events.go @@ -172,10 +172,13 @@ type Announcement struct { // which branch, producing which commit. The mesh matches it against every module's recorded // source and builds what moved, bases first. type SourceMoved struct { - Owner string `json:"owner"` - Repo string `json:"repo"` - Base string `json:"base"` - Head string `json:"head"` + Owner string `json:"owner"` + Repo string `json:"repo"` + Base string `json:"base"` + Head string `json:"head"` + // HeadSHA is the head commit the forge merged (novox/hq issue 703): a walk takes only a build a check of that + // head made. Empty from a forge module from before it, and then the walk takes no check's build. + HeadSHA string `json:"head_sha,omitempty"` Commit string `json:"merge_commit_sha"` CloneURL string `json:"clone_url"` HTMLURL string `json:"html_url"` diff --git a/internal/link/queue.go b/internal/link/queue.go index b54e87d5..6b6f5fb1 100644 --- a/internal/link/queue.go +++ b/internal/link/queue.go @@ -378,19 +378,6 @@ func ReadCheckBuilds(ctx context.Context, js *broker.JetStream, seat string, pul return readCheckBuilds(ctx, js, seat, since, func(p PullRef, _ CheckBuilt) bool { return wanted[p] }) } -// ReadCheckBuildsOf is every build the seat's checks of any pull request said they made, or kept, of those modules -// since a moment (novox/hq issue 703): what a check may keep instead of building. Newest first. -func ReadCheckBuildsOf(ctx context.Context, js *broker.JetStream, seat string, modules []string, since time.Time) ([]CheckedBuild, error) { - if len(modules) == 0 { - return nil, nil - } - wanted := map[string]bool{} - for _, m := range modules { - wanted[m] = true - } - return readCheckBuilds(ctx, js, seat, since, func(_ PullRef, b CheckBuilt) bool { return wanted[b.Module] }) -} - // readCheckBuilds is every build the seat's checks said they made, or kept, that wanted takes. A kept build is named // as the check's that built it (novox/hq issue 703): the record under its key is that check's. func readCheckBuilds(ctx context.Context, js *broker.JetStream, seat string, since time.Time, @@ -428,7 +415,7 @@ func readCheckBuilds(ctx context.Context, js *broker.JetStream, seat string, sin if b.Kept != "" { check = b.Kept } - out = append(out, CheckedBuild{Pull: pull, Check: check, Built: b}) + out = append(out, CheckedBuild{Pull: pull, Check: check, Head: r.Check.Head, Built: b}) } } } From c707bf18d8bfb9a3366b009eb8d2394917147754 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 10:05:31 +0200 Subject: [PATCH 3/3] Say which check made a build the walk does not take, and test the merged heads A build of another pull request or of a head the forge did not merge is in no verdict the walk is handed; the walk now names the check that made it, so its "built:" line says which head it was (novox/hq issue 703). --- cmd/mesh-controller/check_builds_test.go | 15 +++++++++++++++ internal/builder/reuse.go | 6 ++++-- internal/builder/reuse_test.go | 3 ++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/cmd/mesh-controller/check_builds_test.go b/cmd/mesh-controller/check_builds_test.go index 3dc2b516..1a093834 100644 --- a/cmd/mesh-controller/check_builds_test.go +++ b/cmd/mesh-controller/check_builds_test.go @@ -262,3 +262,18 @@ func forgetWalkVerdicts() { defer walkVerdicts.Unlock() walkVerdicts.read = map[string]verdictsRead{} } + +// A walk's merged heads are the forge's word for each merge; a merge announced without its head, or a pull request +// merged at two heads in one walk, has none, and its walk takes no check's build (novox/hq issue 703). +func TestAWalksMergedHeadsAreTheForgesWord(t *testing.T) { + merge := func(number int, head string) inventory.PlanMerge { + return inventory.PlanMerge{Repository: "novox/shop", Number: number, Head: head} + } + p := &inventory.Plan{Delivery: &inventory.PlanDelivery{Merges: []inventory.PlanMerge{ + merge(7, "aaaaaaaaaaaa1"), merge(8, ""), merge(9, "bbbbbbbbbbbb"), merge(9, "cccccccccccc")}}} + heads := mergedHeadsOf(p) + shop := func(n int) link.PullRef { return link.PullRef{Owner: "novox", Repo: "shop", Number: n} } + if heads[shop(7)] != "aaaaaaaaaaaa1" || heads[shop(8)] != "" || heads[shop(9)] != "" { + t.Fatalf("merged heads %v", heads) + } +} diff --git a/internal/builder/reuse.go b/internal/builder/reuse.go index 1f8e01c1..5ff2446e 100644 --- a/internal/builder/reuse.go +++ b/internal/builder/reuse.go @@ -616,6 +616,8 @@ func unnamed(named []NamedBuild, module, key string, found CheckBuild) string { return "" } } - return "a build is kept under this key, and no check verdict of the walk's pull requests names it with its " + - "digests: not taken" + // A build of another pull request, or of a head the forge did not merge, is in no verdict the walk is handed + // (novox/hq issue 703): said with the check that made it, whose head names which head it was. + return "a build is kept under this key, by " + found.Said() + ", and no check verdict of the walk's pull requests " + + "at the head the forge merged names it with its digests: not taken" } diff --git a/internal/builder/reuse_test.go b/internal/builder/reuse_test.go index 785c2615..ff5a0451 100644 --- a/internal/builder/reuse_test.go +++ b/internal/builder/reuse_test.go @@ -216,7 +216,8 @@ func TestARecordNoVerdictNamesIsRefusedAndBuilt(t *testing.T) { if got.Reused != nil || len(scratch.took) > 0 || !dockerBuilt(r) { t.Fatalf("a record no verdict names was taken: %+v, copied %v", got.Reused, scratch.took) } - if !strings.Contains(got.Why, "no check verdict of the walk's pull requests names it") { + if !strings.Contains(got.Why, "no check verdict of the walk's pull requests at the head the forge merged names it") || + !strings.Contains(got.Why, "at head abcdefabcdef") { t.Fatalf("the walk says %q", got.Why) } })