From 7743bd0d4b34c43f7a1dc36d720458a83a946703 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 17:53:45 +0200 Subject: [PATCH] Delete the walk's reuse of a check's build: a walk takes keys and asks build-agent only for the builder's own module (issue 1851, A8) --- cmd/mesh-controller/build.go | 1 - cmd/mesh-controller/check_builds.go | 2 +- .../check_builds_integration_test.go | 17 +--- cmd/mesh-controller/check_builds_test.go | 44 ---------- cmd/mesh-controller/release_plan.go | 88 ++----------------- cmd/mesh-controller/same_program.go | 4 +- 6 files changed, 14 insertions(+), 142 deletions(-) diff --git a/cmd/mesh-controller/build.go b/cmd/mesh-controller/build.go index f78f79f1..326dcf12 100644 --- a/cmd/mesh-controller/build.go +++ b/cmd/mesh-controller/build.go @@ -511,7 +511,6 @@ func buildOneAsked(ctx context.Context, source buildSource, path, ref string, wa Seats: seatBases(ctx), DryRun: dryRun, } - request.Reuse = reuseAsked(ctx, dryRun) fmt.Printf("asked for %s", source) if source.Seat != "" { fmt.Printf(" (%s)", repository) diff --git a/cmd/mesh-controller/check_builds.go b/cmd/mesh-controller/check_builds.go index 7cf571de..b1f78938 100644 --- a/cmd/mesh-controller/check_builds.go +++ b/cmd/mesh-controller/check_builds.go @@ -53,7 +53,7 @@ func forgetChecksOfWalk(ctx context.Context, inv *inventory.Inventory, p invento if p.WalkSaid != inventory.WalkEnded || p.Delivery == nil { return } - for _, pull := range reuseAskOf(&p).Pulls { + for _, pull := range pullsOfWalk(&p) { said, err := forgetChecks(ctx, inv, builder.PullRef{Owner: pull.Owner, Repo: pull.Repo, Number: pull.Number}) if err != nil { fmt.Fprintf(os.Stderr, "%s: its check builds stay until the week's sweep: %v\n", p.ID, err) diff --git a/cmd/mesh-controller/check_builds_integration_test.go b/cmd/mesh-controller/check_builds_integration_test.go index 36a78eb6..1e21463c 100644 --- a/cmd/mesh-controller/check_builds_integration_test.go +++ b/cmd/mesh-controller/check_builds_integration_test.go @@ -14,23 +14,12 @@ import ( "github.com/novox/mesh-controller/internal/link" ) -func TestOnlyAWalksAskTakesAChecksBuildAndADryRunNever(t *testing.T) { +func TestAWalkNamesThePullRequestsOfItsMergesOnce(t *testing.T) { p := &inventory.Plan{Delivery: &inventory.PlanDelivery{Merges: []inventory.PlanMerge{ {Repository: "novox/shop", Number: 7}, {Repository: "http://forge.invalid/novox/web.git", Number: 3}, {Repository: "novox/shop", Number: 7}, {Repository: "novox/shop"}}}} - asked := reuseAskOf(p) - if want := []link.PullRef{{Owner: "novox", Repo: "shop", Number: 7}, {Owner: "novox", Repo: "web", Number: 3}}; !reflect.DeepEqual(asked.Pulls, want) { - t.Fatalf("the walk names %+v", asked.Pulls) - } - walk := withReuse(t.Context(), asked) - if reuseAsked(walk, false) != asked { - t.Fatal("a walk's ask does not carry its leave") - } - if reuseAsked(walk, true) != nil { - t.Fatal("a dry run carries a leave to take a check's build") - } - if reuseAsked(t.Context(), false) != nil { - t.Fatal("an ask that is not a walk's carries a leave") + if want := []link.PullRef{{Owner: "novox", Repo: "shop", Number: 7}, {Owner: "novox", Repo: "web", Number: 3}}; !reflect.DeepEqual(pullsOfWalk(p), want) { + t.Fatalf("the walk names %+v", pullsOfWalk(p)) } } diff --git a/cmd/mesh-controller/check_builds_test.go b/cmd/mesh-controller/check_builds_test.go index 8c4d0ccf..1e670644 100644 --- a/cmd/mesh-controller/check_builds_test.go +++ b/cmd/mesh-controller/check_builds_test.go @@ -104,44 +104,6 @@ func TestAWalksBuildSaysWhetherItReusedOrBuilt(t *testing.T) { // build as it judges one the walk built. A scratch reference is refused. Needs the store (MESH_TEST_POSTGRES), and // is named in testdata/store-tests. -// 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 - 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 = append(asked, pulls) - return []link.CheckedBuild{atHead, earlier}, nil - } - 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(), "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 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) { @@ -171,12 +133,6 @@ func TestACheckIsHandedOnlyItsOwnHeadsBuilds(t *testing.T) { } } -func forgetWalkVerdicts() { - walkVerdicts.Lock() - 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) { diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index d8ec6a75..57e43e81 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -415,35 +415,13 @@ func init() { } } -// reuseKey carries a walk's leave to take a check's build from askModule to the ask it makes (novox/hq ADR 0350 §2). -type reuseKey struct{} - -// withReuse is ctx carrying a walk's leave to take a check's build. -func withReuse(ctx context.Context, r *link.ReuseAsk) context.Context { - return context.WithValue(ctx, reuseKey{}, r) -} - -// reuseIn is the leave ctx carries; nil for every ask but a walk's. -func reuseIn(ctx context.Context) *link.ReuseAsk { - r, _ := ctx.Value(reuseKey{}).(*link.ReuseAsk) - return r -} - -// reuseAsked is the leave an ask carries: a walk's, and never a dry run's — a dry run registers nothing, so it -// takes nothing either (novox/hq ADR 0350 §2, §6). -func reuseAsked(ctx context.Context, dryRun bool) *link.ReuseAsk { - if dryRun { +// pullsOfWalk are the pull requests of the merges a walk answers, each once: what its end lets go of (forgetChecksOfWalk) +// and what a check of one was (same_program.go). +func pullsOfWalk(p *inventory.Plan) []link.PullRef { + if p.Delivery == nil { return nil } - return reuseIn(ctx) -} - -// reuseAskOf is a walk's leave to take a check's build: the pull requests of the merges it answers. -func reuseAskOf(p *inventory.Plan) *link.ReuseAsk { - r := &link.ReuseAsk{} - if p.Delivery == nil { - return r - } + var out []link.PullRef seen := map[link.PullRef]bool{} for _, m := range p.Delivery.Merges { pull, ok := pullOf(m) @@ -452,10 +430,10 @@ func reuseAskOf(p *inventory.Plan) *link.ReuseAsk { } if !seen[pull] { seen[pull] = true - r.Pulls = append(r.Pulls, pull) + out = append(out, pull) } } - return r + return out } // checkedBuildsOf are the builds the build seat's checks of those pull requests said they made for their merges, @@ -496,53 +474,6 @@ func mergedHeadsOf(p *inventory.Plan) map[link.PullRef]string { 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), 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) - } - } - return r -} - // pullOf is a merge's pull request: owner and repository, the last two parts of however the repository is spelled // (owner/repo, a path on the git seat, a URL), and its number. False when the merge names none. func pullOf(m inventory.PlanMerge) (link.PullRef, bool) { @@ -587,13 +518,10 @@ func askModule(ctx context.Context, p *inventory.Plan, name string, byName map[s if p.Delivery != nil && p.Delivery.Alone && carried != "" { ref = carried } - // **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. // **The plan's key is there before its first build is** (ADR 0483 clause 2): a check taken between the ask and the plan's save // would hold a place for its whole run. sayBuilding(ctx, p.ID, now) - id, err := askABuild(withReuse(ctx, reuseWithVerdicts(ctx, p.ID, reuseAskOf(p), mergedHeadsOf(p))), source, - e.Source.Path, ref) + id, err := askABuild(ctx, source, e.Source.Path, ref) if err != nil { // **A bus that does not answer for a moment is not a failed build** (issue 1334): the module stays unasked and the // tier's next look asks it again. A fault of the module's own fails the plan at once, as does a bus silent too long. diff --git a/cmd/mesh-controller/same_program.go b/cmd/mesh-controller/same_program.go index 6c79c26b..fa1260d2 100644 --- a/cmd/mesh-controller/same_program.go +++ b/cmd/mesh-controller/same_program.go @@ -30,7 +30,7 @@ func sameAsRunning(plan *inventory.Plan, builds []link.CheckedBuild, held map[st return nil } } - pulls := reuseAskOf(plan).Pulls + pulls := pullsOfWalk(plan) heads := mergedHeadsOf(plan) out := map[string]string{} for name := range plan.Modules { @@ -97,7 +97,7 @@ func leaveSameAsRunning(ctx context.Context, inv *inventory.Inventory, plan *inv if plan.Delivery == nil || len(plan.Modules) == 0 { return nil } - pulls := reuseAskOf(plan).Pulls + pulls := pullsOfWalk(plan) if len(pulls) == 0 { return nil }