From f76da2ce9c7233c77dde5c00f8a9c534b00fe23b Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 18:05:49 +0200 Subject: [PATCH] =?UTF-8?q?Ask=20the=20checker=20for=20a=20try=20of=20a=20?= =?UTF-8?q?branch=20head,=20not=20build-agent,=20and=20read=20its=20word?= =?UTF-8?q?=20in=20today's=20answer=20(issue=201851,=20A11,=20ADR=200501?= =?UTF-8?q?=20=C2=A75)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- cmd/mesh-controller/try.go | 205 +++++++++----------- cmd/mesh-controller/try_integration_test.go | 39 ++++ cmd/mesh-controller/try_test.go | 97 +++------ 3 files changed, 159 insertions(+), 182 deletions(-) diff --git a/cmd/mesh-controller/try.go b/cmd/mesh-controller/try.go index a120385c..d306975e 100644 --- a/cmd/mesh-controller/try.go +++ b/cmd/mesh-controller/try.go @@ -18,35 +18,31 @@ import ( "github.com/novox/mesh-controller/internal/link" ) -// A branch is tried before its pull request (novox/hq issue 686). +// A branch is tried before its pull request (novox/hq issue 686), by the checker (novox/hq ADR 0501 §5; issue 1851). // -// A repository's store tests skip without a store, and no agent may start a database on its machine. So a -// builder pushed, a reviewer read, and only the pull request's merge check ran those tests for the first time: -// each failure there cost a round of fifteen to thirty minutes (mesh-controller #283, #310, #312). +// A repository's store tests skip without a store, and no agent may start a database on its machine. So a builder +// pushed, a reviewer read, and only the pull request's merge check ran those tests for the first time: each failure +// there cost a round of fifteen to thirty minutes (mesh-controller #283, #310, #312). // -// `try / ` asks the build seat for the repository's own check of a pushed branch or -// commit, as the merge check runs it (internal/builder/check.go): the ref merged into the trunk's head, the -// trunk's merge-check.sh, each part in its toolchain's container, against a throwaway store and bus. It is a -// check and nothing more: +// `try / ` asks the checker (mesh-delivery-checker's `checker_try`) to judge the commit the ref +// names merged onto the trunk, as it judges a pull request's head: the build modules' fetch, digest and test steps, +// the unit level and then the integration level, each level's kept verdicts taken. From R4e build-agent is asked no +// try. It is a check and nothing more: // -// - **no pull request, so no status**: its outcome is never said as `checked`, which is what the forge's -// holder sets statuses from (isTry, in Built); -// - **nothing registered, nothing built**: it names no module of the graph, so the merge gate does not run and -// no build is made for a merge (novox/hq ADR 0350's scratch builds are a pull request's check's alone); -// - **a check in the build queue**: after deliveries (link.KindOf, novox/hq issue 558), and bounded by the -// check's own bound (checkTimeout); -// - **the code a pull request's check would run anyway**: only a repository of the mesh, whose every pull -// request the build seat already checks, in containers that hold no runtime socket. +// - **no pull request, so no status**: the checker owes the forge nothing for a try; +// - **nothing registered, nothing built for delivery**: a try writes no verdict of a group and no preview; +// - **bounded**: one try of a head at a time — a second ask answers the first's id — and at most four open, the +// checker's own rule; a repository the checker judges no level of (hq) is refused by the checker, and said. // -// A check outlasts a seat verb's one answer, so the ask answers its id at once and `try --id ` reads what -// came of it from the build's log, where the build seat says it whole (link.StepTried): holding its answer up -// to tryWait for a verdict, like `plans wait` (novox/hq issue 229). Nothing is kept beside the log. +// A check outlasts a seat verb's one answer, so the ask answers its id at once and `try --id ` reads what came of +// it from the checker (`checker_try_read`), holding its answer up to tryWait for a verdict, like `plans wait`. // tryWait is how long reading a try holds its answer for the verdict. A variable so a test need not wait. var tryWait = 20 * time.Second // A ref as the forge is asked for it — a branch or a commit, never an option — and a `go test -run` pattern. var ( + tryID = regexp.MustCompile(`^try-[0-9]+$`) tryRef = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._/-]*$`) tryTests = regexp.MustCompile(`^[A-Za-z0-9_^$|().*+?/\[\]-]+$`) ) @@ -77,34 +73,70 @@ func isTry(result link.BuildResult) bool { return c != nil && (c.Try || (c.Number == 0 && c.Group == "")) } -// asATry makes a check's ask a try's: the repository's own script alone, for no pull request. -func asATry(request link.BuildRequest, tests string, storeOnly bool) link.BuildRequest { - c := *request.Check - c.Number, c.Try, c.Tests, c.StoreOnly = 0, true, tests, storeOnly - c.Paths, c.Modules, c.New, c.Dependents, c.Manifests, c.Builds = nil, nil, nil, nil, nil, nil - c.Judge, c.Plan, c.Group, c.Members = "", nil, "", nil - request.Check = &c - return request +// checkerModule is the module whose tools judge a try (mesh-delivery-checker). +const checkerModule = "mesh-delivery-checker" + +// checkerTry is the checker's answer to a try's ask: the try's id. +type checkerTry struct { + ID string `json:"id"` } -// maxTries is how many tries may wait or run at once, mesh-wide: each holds a build machine for minutes. -const maxTries = 4 +// checkerTryRead is the checker's word on a try (`checker_try_read`): its state queued, running, pass, fail or error +// (could not run), what it said, the head and trunk judged, how long it took, what failed, and the machines it ran on. +type checkerTryRead struct { + ID string `json:"id"` + Repository string `json:"repository"` + Head string `json:"head"` + Trunk string `json:"trunk"` + Branch string `json:"branch,omitempty"` + State string `json:"state"` + Said string `json:"said"` + Took string `json:"took,omitempty"` + Failed []string `json:"failed,omitempty"` + Machines []string `json:"machines,omitempty"` +} -// triesIn reads the build queue for a new try of one commit: the id of a try of that commit already waiting or -// running — the answer to a second ask — and whether there is room for another. -func triesIn(q link.Queue, owner, repo, commit string) (same string, room bool) { - n := 0 - for _, a := range q.Asks { - if !link.IsTry(a.Request) || (a.State != link.AskWaiting && a.State != link.AskInFlight) { - continue +// askChecker asks one of the checker's tools and reads its result into into. A variable so a test needs no checker. +var askChecker = func(ctx context.Context, tool string, args map[string]any, into any) error { + js, err := aBus() + if err != nil { + return err + } + defer js.Close() + raw, _ := json.Marshal(args) + answer, err := link.Ask(ctx, link.OverNATS{Conn: js.Conn()}, checkerModule, tool, raw, 30*time.Second) + if err != nil { + return fmt.Errorf("%s.%s: %w", checkerModule, tool, err) + } + if answer.Error != "" { + return fmt.Errorf("%s.%s: %s", checkerModule, tool, answer.Error) + } + var wrapped struct { + Result json.RawMessage `json:"result"` + } + if err := json.Unmarshal(answer.Result, &wrapped); err == nil && len(wrapped.Result) > 0 { + return json.Unmarshal(wrapped.Result, into) + } + return json.Unmarshal(answer.Result, into) +} + +// tryOfChecker is the checker's word on a try as `try` answers it, today's answer unchanged (novox/hq issue 686): queued +// is waiting, and the machines it ran on are where. +func tryOfChecker(r checkerTryRead) tryAnswer { + a := tryAnswer{ID: r.ID, State: r.State, Said: r.Said, Repository: r.Repository, Commit: r.Head, Trunk: r.Trunk, + Took: r.Took, Failed: r.Failed, On: strings.Join(r.Machines, ", "), Base: r.Branch} + switch r.State { + case "queued": + a.State = "waiting" + if a.Said == "" { + a.Said = "it waits for the checker; ask again" } - n++ - c := a.Request.Check - if same == "" && strings.EqualFold(c.Owner, owner) && strings.EqualFold(c.Repo, repo) && a.Request.Ref == commit { - same = a.ID + case "running": + if a.Said == "" { + a.Said = "still running; ask again" } } - return same, n < maxTries + return a } // forgeCommit asks the forge, through its module's tools on the bus, which commit a branch or commit of a @@ -168,9 +200,9 @@ func tryCommand(ctx context.Context, args []string) error { if len(positionals) != 0 { return usage } - // Before it is a token of a bus subject: an id, never a pattern. - if !link.SafeBuildID(*id) { - return fmt.Errorf("%q is not a try's id", *id) + // An id, never a pattern: the checker's `try-`. + if !tryID.MatchString(*id) { + return fmt.Errorf("%q is not a try's id, try-", *id) } return tryRead(ctx, *id) } @@ -220,56 +252,33 @@ func tryCommand(ctx context.Context, args []string) error { return fmt.Errorf("%s/%s is no repository of the mesh — no module is built from it and its owner is not "+ "the core's — and only the mesh's are tried on the build seat; nothing was asked", owner, repo) } - request, err := checkRequestFor(ctx, open, p, checkScope{Mesh: true, From: mesh.From}, entries) - if err != nil { - return err + // **Asked of the checker, never of build-agent** (ADR 0501 §5): the head merged onto the trunk, as a pull request's. + askArgs := map[string]any{"repository": strings.ToLower(owner + "/" + repo), "head": commit, "trunk": trunk} + if *tests != "" { + askArgs["tests"] = *tests } - request = asATry(request, *tests, *storeOnly) - seat := buildSeatHeld(ctx) - // **Bounded in number** (novox/hq issue 686): one try of a commit at a time, and maxTries in all. - js, err := dialTheBus() - if err != nil { - return err + if *storeOnly { + askArgs["level"] = "integration" } - queue, err := link.ReadQueue(ctx, js, seat) - js.Close() - if err != nil { - return err + var asked checkerTry + if err := askChecker(ctx, "checker_try", askArgs, &asked); err != nil { + return fmt.Errorf("the checker did not take a try of %s/%s at %.12s; nothing was asked: %w", owner, repo, commit, err) } - same, room := triesIn(queue, owner, repo, commit) - if same != "" { - return printTry(tryAnswer{ID: same, State: "asked", Repository: owner + "/" + repo, Ref: ref, Commit: commit, - On: seat, Said: fmt.Sprintf("a try of %s/%s at %.12s already waits or runs, as %s: nothing new was asked. "+ - "Read it with try id=%s", owner, repo, commit, same, same)}) - } - if !room { - return fmt.Errorf("%d tries already wait or run on the build seat, the most at once (`queue` lists them); "+ - "nothing was asked — read one of yours, or ask again when one has ended", maxTries) - } - ask, err := askOverOn(seat) - if err != nil { - return err - } - defer ask.Close() - if err := ask.Ask(ctx, request); err != nil { - return err - } - return printTry(tryAnswer{ID: request.ID, State: "asked", Repository: owner + "/" + repo, Ref: ref, Commit: commit, - Base: trunk, On: seat, Said: fmt.Sprintf("asked %s to try %s/%s at %.12s (%s) merged into %s: its own "+ - "merge-check.sh, as a check in the build queue, bounded at %s; no status is set and nothing is registered. "+ - "Read it with try id=%s — a full run of this kind takes minutes", seat, owner, repo, commit, ref, trunk, - checkTimeout, request.ID)}) + return printTry(tryAnswer{ID: asked.ID, State: "asked", Repository: owner + "/" + repo, Ref: ref, Commit: commit, + Base: trunk, On: checkerModule, Said: fmt.Sprintf("asked %s to try %s/%s at %.12s (%s) merged into %s: the head "+ + "judged as a pull request's, unit level then integration level; no status is set and nothing is registered. "+ + "Read it with try id=%s", checkerModule, owner, repo, commit, ref, trunk, asked.ID)}) } // tryRead answers what came of a try, holding the answer up to tryWait for its verdict. func tryRead(ctx context.Context, id string) error { until := time.Now().Add(tryWait) for { - lines, err := buildLines(ctx, id) - if err != nil { - return err + var r checkerTryRead + if err := askChecker(ctx, "checker_try_read", map[string]any{"id": id}, &r); err != nil { + return printTry(tryAnswer{ID: id, State: "error", Said: "the checker's word on it cannot be read: " + err.Error()}) } - a := tryAnswerOf(id, lines) + a := tryOfChecker(r) if (a.State != "waiting" && a.State != "running") || !time.Now().Add(2*time.Second).Before(until) { return printTry(a) } @@ -279,34 +288,6 @@ func tryRead(ctx context.Context, id string) error { } } -// tryAnswerOf reads a try's answer from what the build seat said of it: its outcome under StepTried; `failed` -// for one that could not run — an error, never a pass; else how far it is. -func tryAnswerOf(id string, lines []link.BuildLine) tryAnswer { - a := tryAnswer{ID: id, State: "waiting", Said: "nothing is said of " + id + " yet: it waits in the build queue " + - "(`queue` says where), or no try has that id; ask again"} - for _, l := range lines { - switch l.Step { - case link.StepTried: - var t link.Tried - if err := json.Unmarshal([]byte(l.Message), &t); err != nil || t.Verdict == "" { - return tryAnswer{ID: id, State: "error", Said: "the build seat's word on it cannot be read: " + l.Message} - } - return tryAnswer{ID: id, State: t.Verdict, Said: t.Summary, Took: t.Took, Commit: t.Head, Trunk: t.Trunk, - Failed: t.Failed} - case "failed": - return tryAnswer{ID: id, State: "error", Said: "it could not run: " + l.Message} - case "checked": - // A check that is no try, or a build seat from before the verb: its verdict is not a try's. - return tryAnswer{ID: id, State: "error", Said: "the build seat checked it and said no try's outcome — " + - "it predates the verb (novox/hq issue 686), or " + id + " is no try: " + l.Message + "; `builds` with " + - "the id says every line"} - } - a.State, a.Last = "running", l.Message - a.Said = "still running; ask again. `builds` with the id says every line so far" - } - return a -} - func printTry(a tryAnswer) error { fmt.Fprintln(os.Stderr, a.Said) for _, l := range a.Failed { diff --git a/cmd/mesh-controller/try_integration_test.go b/cmd/mesh-controller/try_integration_test.go index c3b930e8..5885470c 100644 --- a/cmd/mesh-controller/try_integration_test.go +++ b/cmd/mesh-controller/try_integration_test.go @@ -4,8 +4,13 @@ package main import ( "context" + "encoding/json" + "reflect" + "strings" "testing" + "git.novox.be/novox/mesh-sdk/go/catalogue" + "github.com/novox/mesh-controller/internal/inventory" "github.com/novox/mesh-controller/internal/link" ) @@ -37,3 +42,37 @@ func TestATrysOutcomeSetsNoStatusAndRegistersNothing(t *testing.T) { t.Fatalf("a pull request's check was said %d time(s)", len(said.events)) } } + +// ADR 0501 §5 (issue 1851): a try is asked of the checker, the head merged onto the trunk, never of build-agent: +// --store-only is the integration level, --tests passes on, the ask answers the checker's id. +func TestATryIsAskedOfTheChecker(t *testing.T) { + open := aMesh(t) + ctx := t.Context() + noBuildAsked(t) + if err := open.inventory.RegisterModule(ctx, catalogue.Manifest{Module: "sudo", Version: "1"}, + inventory.Source{Repository: "novox/mesh-catalog", Seat: "git", Path: "modules/sudo", Ref: "main", + BuiltFrom: "c0", Head: "c0"}); err != nil { + t.Fatal(err) + } + wasForge, wasChecker := forgeCommit, askChecker + forgeCommit = func(context.Context, string, string, string) (string, string, error) { + return strings.Repeat("c", 40), "main", nil + } + var asked []map[string]any + askChecker = func(_ context.Context, tool string, args map[string]any, into any) error { + if tool != "checker_try" { + t.Errorf("asked %s", tool) + } + asked = append(asked, args) + return json.Unmarshal([]byte(`{"id":"try-1"}`), into) + } + t.Cleanup(func() { forgeCommit, askChecker = wasForge, wasChecker }) + if err := tryCommand(ctx, []string{"novox/Mesh-Catalog", "fix/x", "--store-only", "--tests", "^TestX$"}); err != nil { + t.Fatal(err) + } + want := map[string]any{"repository": "novox/mesh-catalog", "head": strings.Repeat("c", 40), "trunk": "main", + "level": "integration", "tests": "^TestX$"} + if len(asked) != 1 || !reflect.DeepEqual(asked[0], want) { + t.Fatalf("the checker was asked %v, want %v", asked, want) + } +} diff --git a/cmd/mesh-controller/try_test.go b/cmd/mesh-controller/try_test.go index 49ef75a8..e696945b 100644 --- a/cmd/mesh-controller/try_test.go +++ b/cmd/mesh-controller/try_test.go @@ -15,29 +15,6 @@ import ( // A branch is tried before its pull request (novox/hq issue 686): the repository's own check asked for a pushed // branch or commit, with no pull request. -// What a try asks is a check — after deliveries in the build queue, bounded as one — of the repository's own -// script alone: no module of the graph, so no gate; nothing built for a merge, so no reusable image. -func TestATryAsksACheckOfTheRepositorysOwnScriptAlone(t *testing.T) { - request := link.BuildRequest{ID: "build-1", Ref: "c0ffee", Check: &link.CheckRequest{Owner: "novox", Repo: "a", - Number: 7, Base: "main", Modules: []string{"a"}, New: []string{"b"}, Manifests: []string{"module.json"}, - Dependents: []string{"c"}, Builds: []link.CheckBuild{{Module: "a"}}, Judge: link.JudgeSelf, - Plan: &link.ChangePlan{}}} - tried := asATry(request, "^TestStore", true) - c := tried.Check - if !c.Try || c.Number != 0 || c.Tests != "^TestStore" || !c.StoreOnly { - t.Fatalf("not marked a try, or it kept a pull request's number: %+v", c) - } - if len(c.Modules)+len(c.New)+len(c.Manifests)+len(c.Dependents)+len(c.Builds) != 0 || c.Judge != "" || c.Plan != nil { - t.Fatalf("a try runs the gate or builds for a merge: %+v", c) - } - if c.Base != "main" || c.Owner != "novox" || tried.Ref != "c0ffee" { - t.Fatalf("a try lost what it merges into what: %+v", tried) - } - if link.KindOf(tried) != link.KindCheck { - t.Fatal("a try does not count as a check in the build queue") - } -} - // A try's outcome is heard and left there: no `checked` is said, so the forge's holder sets no status, and // nothing is recorded or registered (the handler holds no store here, and would fail reaching for one). @@ -76,58 +53,38 @@ func TestATryOfAnUnknownRefIsRefusedLoudly(t *testing.T) { } } -// The answer of a try carries the lines that name what failed (novox/hq issue 460's shape) and how long it took. -func TestATrysAnswerCarriesTheFailingLines(t *testing.T) { - tried, _ := json.Marshal(link.Tried{Verdict: "fail", Summary: "its merge-check.sh failed: TestX", Took: "5m3s", - Head: "c0ffee", Trunk: "ca3f43c", Failed: []string{"--- FAIL: TestX (0.01s)", " x_test.go:12: got 1, want 2", - "FAIL\tgithub.com/novox/a/cmd\t3.1s"}}) - lines := []link.BuildLine{{Seq: 1, Step: "check", Message: "novox/a#0 at c0ffee"}, - {Seq: 2, Step: "output", Message: "ok github.com/novox/a/internal 1.2s"}, - {Seq: 3, Step: link.StepTried, Message: string(tried)}, {Seq: 4, Step: "checked", Message: "pass: x"}} - a := tryAnswerOf("build-1", lines) - if a.State != "fail" || a.Took != "5m3s" || len(a.Failed) != 3 || a.Failed[0] != "--- FAIL: TestX (0.01s)" || - a.Trunk != "ca3f43c" || a.ID != "build-1" { - t.Fatalf("the answer lost the verdict, the time or the failing lines: %+v", a) +// The answer of a try is the checker's word, in today's shape (ADR 0501 §5; issue 1851): its verdict, the failing +// line, how long it took and where; queued is waiting; one that could not run is an error, never a pass. +func TestATrysAnswerIsTheCheckersWord(t *testing.T) { + var asked []string + was := askChecker + askChecker = func(_ context.Context, tool string, args map[string]any, into any) error { + asked = append(asked, tool) + raw, _ := json.Marshal(checkerTryRead{ID: "try-1", Repository: "novox/a", Head: "c0ffee", Trunk: "ca3f43c", + State: "fail", Said: "the unit level failed: TestX", Took: "5m3s", + Failed: []string{"unit: a/cmd.TestX: got 1, want 2"}, Machines: []string{"ace"}}) + return json.Unmarshal(raw, into) } - // Not answered yet: said as running, with how far it is — never as a pass. - if a := tryAnswerOf("build-1", lines[:2]); a.State != "running" || a.Last == "" { - t.Fatalf("a try still running: %+v", a) + t.Cleanup(func() { askChecker = was }) + if err := tryRead(t.Context(), "try-1"); err != nil { + t.Fatal(err) } - if a := tryAnswerOf("build-1", nil); a.State != "waiting" { - t.Fatalf("a try nothing was said of: %+v", a) + if len(asked) != 1 || asked[0] != "checker_try_read" { + t.Fatalf("a try is read from %v, not the checker", asked) } - // One that could not run is an error with the build seat's word, never a pass. - if a := tryAnswerOf("build-1", append(lines[:1:1], link.BuildLine{Seq: 2, Step: "failed", - Message: "cannot clone"})); a.State != "error" || !strings.Contains(a.Said, "cannot clone") { + a := tryOfChecker(checkerTryRead{ID: "try-1", State: "fail", Took: "5m3s", Head: "c0ffee", Trunk: "ca3f43c", + Failed: []string{"unit: a/cmd.TestX: got 1, want 2"}, Machines: []string{"ace"}}) + if a.State != "fail" || a.Took != "5m3s" || len(a.Failed) != 1 || a.Commit != "c0ffee" || a.Trunk != "ca3f43c" || a.On != "ace" { + t.Fatalf("the answer lost the verdict, the time, the failing line or where: %+v", a) + } + if a := tryOfChecker(checkerTryRead{ID: "try-2", State: "queued"}); a.State != "waiting" || a.Said == "" { + t.Fatalf("a try the checker has not started: %+v", a) + } + if a := tryOfChecker(checkerTryRead{ID: "try-3", State: "error", Said: "no node holds the go build module"}); a.State != "error" { t.Fatalf("a try that could not run: %+v", a) } -} - -// One try of a commit at a time, and four in all (novox/hq issue 686): a second ask is answered the first's id, -// a fifth is refused; a try that ended, and a pull request's check, count for nothing. -func TestTriesAreBoundedInNumber(t *testing.T) { - ask := func(id, repo, ref, state string, try bool) link.QueuedAsk { - return link.QueuedAsk{ID: id, State: state, Request: link.BuildRequest{ID: id, Ref: ref, - Check: &link.CheckRequest{Owner: "novox", Repo: repo, Try: try}}} - } - q := link.Queue{Asks: []link.QueuedAsk{ - ask("t1", "a", "c1", link.AskInFlight, true), ask("t2", "a", "c2", link.AskWaiting, true), - ask("t3", "b", "c1", link.AskDead, true), ask("p1", "a", "c9", link.AskWaiting, false)}} - if same, room := triesIn(q, "novox", "a", "c2"); same != "t2" || !room { - t.Fatalf("a second try of a commit: %q %v", same, room) - } - if same, room := triesIn(q, "novox", "b", "c1"); same != "" || !room { - t.Fatalf("a dead try, or another repository's, was taken for this one: %q %v", same, room) - } - if same, _ := triesIn(q, "novox", "a", "c9"); same != "" { - t.Fatalf("a pull request's check was taken for a try: %q", same) - } - q.Asks = append(q.Asks, ask("t4", "c", "c3", link.AskWaiting, true), ask("t5", "d", "c4", link.AskWaiting, true)) - if _, room := triesIn(q, "novox", "e", "c5"); room { - t.Fatal("a fifth try was let in") - } - // An id is checked before it is a token of a bus subject. - for _, id := range []string{"x.>", "*", "a b"} { + // An id is checked before it is asked of anybody. + for _, id := range []string{"x.>", "*", "a b", "build-1"} { if err := tryCommand(context.Background(), []string{"--id", id}); err == nil || !strings.Contains(err.Error(), "not a try's id") { t.Errorf("try --id %q: %v", id, err)