From 8123ca565492d9b2b9bd244b4b09e21dcab98cc7 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 02:20:37 +0200 Subject: [PATCH 1/3] Test that a branch is tried before its pull request (hq issue 686) A builder's store tests first ran in the pull request's merge check, a review round late. These fail on main: a try asks the repository's own check alone, its outcome sets no status and registers nothing, an unknown ref is refused before anything is asked, and the answer carries the failing lines. --- cmd/mesh-builder/tried_test.go | 31 ++++++ cmd/mesh-controller/try_test.go | 162 ++++++++++++++++++++++++++++++++ internal/builder/check_test.go | 12 +++ 3 files changed, 205 insertions(+) create mode 100644 cmd/mesh-builder/tried_test.go create mode 100644 cmd/mesh-controller/try_test.go diff --git a/cmd/mesh-builder/tried_test.go b/cmd/mesh-builder/tried_test.go new file mode 100644 index 00000000..06aaf706 --- /dev/null +++ b/cmd/mesh-builder/tried_test.go @@ -0,0 +1,31 @@ +package main + +import ( + "testing" + "time" + + "github.com/novox/mesh-controller/internal/builder" +) + +// A try's outcome, as the build seat says it in the build's log (novox/hq issue 686): the repository's own +// check's verdict, how long it took, and each line naming what failed. +func TestATrysOutcomeNamesWhatFailed(t *testing.T) { + tried := triedOf(builder.CheckVerdict{Verdict: "pass", Summary: "the change touches no module", Took: 303 * time.Second, + Head: "c0ffee", Trunk: "ca3f43c", Gate: &builder.Layer{Verdict: "pass"}, + Repo: &builder.Layer{Verdict: "fail", Summary: "its merge-check.sh failed: TestX", + Failed: "--- FAIL: TestX (0.01s)\n x_test.go:12: got 1, want 2"}}) + if tried.Verdict != "fail" || tried.Took != "5m3s" || len(tried.Failed) != 2 || tried.Trunk != "ca3f43c" || + tried.Summary != "its merge-check.sh failed: TestX" { + t.Fatalf("a failed try: %+v", tried) + } + // A head that does not merge fails both layers: the check's own word. + conflict := triedOf(builder.CheckVerdict{Verdict: "fail", Summary: "does not merge", Gate: &builder.Layer{Verdict: "fail"}, + Repo: &builder.Layer{Verdict: "fail", Summary: "does not merge"}}) + if conflict.Verdict != "fail" || conflict.Summary != "does not merge" { + t.Fatalf("a try that does not merge: %+v", conflict) + } + // No verdict is an error, never a pass. + if none := triedOf(builder.CheckVerdict{}); none.Verdict != "error" { + t.Fatalf("a try with no verdict: %+v", none) + } +} diff --git a/cmd/mesh-controller/try_test.go b/cmd/mesh-controller/try_test.go new file mode 100644 index 00000000..01bc05c9 --- /dev/null +++ b/cmd/mesh-controller/try_test.go @@ -0,0 +1,162 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "reflect" + "strings" + "testing" + + "github.com/novox/mesh-controller/internal/catalogue" + "github.com/novox/mesh-controller/internal/link" +) + +// 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). +func TestATrysOutcomeSetsNoStatusAndRegistersNothing(t *testing.T) { + said := &saidOnTheBus{} + checkEvents = said + defer func() { checkEvents = nil }() + for name, result := range map[string]link.BuildResult{ + "a try": {ID: "build-1", Ref: "c0ffee", Checked: &link.CheckRequest{Owner: "novox", Repo: "a", Try: true}, + Check: &link.CheckOutcome{Verdict: "pass", RepoCheck: &link.CheckLayer{Verdict: "fail", Summary: "failed"}}}, + "a try that could not run": {ID: "build-2", Checked: &link.CheckRequest{Owner: "novox", Repo: "a", Try: true}, + Failed: "cannot clone"}, + // A build seat from before the mark echoes the ask without it: a check of no pull request and no + // group is still nobody's status. + "a try echoed by an older build seat": {ID: "build-3", Checked: &link.CheckRequest{Owner: "novox", Repo: "a"}, + Check: &link.CheckOutcome{Verdict: "pass"}}, + } { + if err := (builds{}).Built(context.Background(), result); err != nil { + t.Fatalf("%s: %v", name, err) + } + } + if len(said.events) != 0 { + t.Fatalf("a try's outcome was said for the forge: %v", said.events) + } + // A pull request's check is still said. + _ = (builds{}).Built(context.Background(), link.BuildResult{ID: "build-4", Checked: &link.CheckRequest{ + Owner: "novox", Repo: "a", Number: 7}, Check: &link.CheckOutcome{Verdict: "pass"}}) + if len(said.events) != 1 { + t.Fatalf("a pull request's check was said %d time(s)", len(said.events)) + } +} + +// saidOnTheBus keeps the seat events said, and nothing else. +type saidOnTheBus struct { + link.Bus + events []string +} + +func (s *saidOnTheBus) PublishSeatEvent(_ context.Context, seat, event string, _ []byte) error { + s.events = append(s.events, seat+" "+event) + return nil +} + +// A ref the forge does not hold is refused before anything is asked, naming it. +func TestATryOfAnUnknownRefIsRefusedLoudly(t *testing.T) { + was := forgeCommit + defer func() { forgeCommit = was }() + forgeCommit = func(_ context.Context, owner, repo, ref string) (string, string, error) { + return "", "", errors.New("404 object does not exist [id: " + ref + "]") + } + err := tryCommand(context.Background(), []string{"novox/mesh-controller", "fix/no-such-branch"}) + if err == nil { + t.Fatal("a try of a ref the forge does not hold was asked") + } + for _, want := range []string{"fix/no-such-branch", "novox/mesh-controller", "nothing was asked", "404"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal does not say %q: %v", want, err) + } + } + for _, args := range [][]string{{"mesh-controller", "main"}, {"novox/mesh-controller"}, {"novox/a", "--upload-pack=x"}, + {"novox/a", "main", "--tests", "x; rm -rf /"}, {"novox/a", "main", "--id", "build-1"}} { + if err := tryCommand(context.Background(), args); err == nil { + t.Errorf("try %v was taken", args) + } + } +} + +// 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) + } + // 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) + } + if a := tryAnswerOf("build-1", nil); a.State != "waiting" { + t.Fatalf("a try nothing was said of: %+v", a) + } + // 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") { + t.Fatalf("a try that could not run: %+v", a) + } +} + +// The verb runs its command, and is declared with the shell habit it replaces (novox/hq ADR 0245). +func TestTheTryVerbRunsItsCommand(t *testing.T) { + for _, c := range []struct { + args map[string]any + want []string + }{ + {map[string]any{"repository": "novox/a", "ref": "fix/x"}, []string{"try", "novox/a", "fix/x"}}, + {map[string]any{"repository": "novox/a", "ref": "fix/x", "base": "main", "tests": "^TestX$", "store-only": "true"}, + []string{"try", "novox/a", "fix/x", "--base", "main", "--tests", "^TestX$", "--store-only"}}, + {map[string]any{"id": "build-1"}, []string{"try", "--id", "build-1"}}, + } { + got, err := argvFor("try", c.args) + if err != nil || !reflect.DeepEqual(got, c.want) { + t.Errorf("try %v: %v %v, want %v", c.args, got, err, c.want) + } + } + if _, err := argvFor("try", map[string]any{"repository": "novox/a"}); err == nil { + t.Error("a try of no ref was taken") + } + for _, v := range catalogue.ControllerVerbs { + if v.Name == "try" { + if len(v.Replaces) == 0 { + t.Error("try names no shell habit it replaces") + } + return + } + } + t.Error("try is not a verb of the controller seat") +} diff --git a/internal/builder/check_test.go b/internal/builder/check_test.go index 92583f5a..1eb5890a 100644 --- a/internal/builder/check_test.go +++ b/internal/builder/check_test.go @@ -590,3 +590,15 @@ func TestARepositoryInTwoLanguagesIsCheckedInBoth(t *testing.T) { t.Errorf("a part in a toolchain the mesh does not hold answered %+v — never a pass", layer) } } + +// A branch tried before its pull request may narrow what the repository's script runs (novox/hq issue 686); a +// pull request's check is handed nothing, and runs whole. +func TestOnlyATrysSelectionReachesTheScript(t *testing.T) { + if env := selection(CheckSpec{Number: 7}); len(env) != 0 { + t.Fatalf("a pull request's check was handed a selection: %v", env) + } + env := selection(CheckSpec{Tests: "^TestX$", StoreOnly: true}) + if len(env) != 2 || env[0] != "MESH_CHECK_TESTS=^TestX$" || env[1] != "MESH_CHECK_STORE_ONLY=1" { + t.Fatalf("a try's selection: %v", env) + } +} From 65508d9df9f45093bc5f3f91e337dbf0801a6f94 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 02:20:37 +0200 Subject: [PATCH 2/3] Try a pushed branch on the build seat before its pull request (hq issue 686) Store tests skip without a store and no agent may start one, so each failure cost a 15-30 minute review round. The try verb asks the build seat for the repository's merge-check.sh on the ref merged into the trunk, as a check in the build queue, and reads its outcome from the build's log: no status on the forge, nothing registered, nothing built for a merge. --- cmd/mesh-builder/main.go | 26 ++- cmd/mesh-controller/main.go | 9 + cmd/mesh-controller/seatverbs.go | 28 +++ cmd/mesh-controller/try.go | 309 +++++++++++++++++++++++++++++++ internal/builder/check.go | 23 +++ internal/catalogue/verbs.go | 19 ++ internal/link/build.go | 24 +++ merge-check.sh | 23 ++- module.json | 1 + 9 files changed, 457 insertions(+), 5 deletions(-) create mode 100644 cmd/mesh-controller/try.go diff --git a/cmd/mesh-builder/main.go b/cmd/mesh-builder/main.go index 7bf565be..be469d09 100644 --- a/cmd/mesh-builder/main.go +++ b/cmd/mesh-builder/main.go @@ -332,6 +332,12 @@ func answer(ctx context.Context, publisher builder.Publisher, on, workspace stri result.Check.Members = append(result.Check.Members, link.CheckedMember{Owner: m.Owner, Repo: m.Repo, Commit: m.Head, Trunk: m.Trunk}) } + // A branch tried before its pull request reads its outcome from this log, whole (novox/hq issue 686). + if request.Check.Try { + if line, err := json.Marshal(triedOf(v)); err == nil { + say(link.StepTried, string(line)) + } + } } } else if err == nil { // The package-registry credential is a build input, so it is resolved before the clone: a @@ -457,7 +463,8 @@ func checkSpecOf(request link.BuildRequest) builder.CheckSpec { spec := builder.CheckSpec{ID: request.ID, Repository: request.Repository, Ref: request.Ref, Owner: c.Owner, Repo: c.Repo, Number: c.Number, Paths: c.Paths, Beside: map[string]builder.Beside{}, Modules: c.Modules, New: c.New, Manifests: c.Manifests, Judge: c.Judge, Base: c.Base, - Toolchain: builder.ToolchainOf(request.Held), Toolchains: builder.ToolchainsOf(request.Held)} + Toolchain: builder.ToolchainOf(request.Held), Toolchains: builder.ToolchainsOf(request.Held), + Tests: c.Tests, StoreOnly: c.StoreOnly} for dir, b := range c.Beside { spec.Beside[dir] = builder.Beside{Repository: b.Repository, Ref: b.Ref} } @@ -472,6 +479,23 @@ func checkSpecOf(request link.BuildRequest) builder.CheckSpec { return spec } +// triedOf is a try's outcome as its log line says it (novox/hq issue 686): the repository's own check's verdict, +// or the check's where that did not run — a head that does not merge — and the lines naming what failed. +func triedOf(v builder.CheckVerdict) link.Tried { + t := link.Tried{Verdict: v.Verdict, Summary: v.Summary, Took: v.Took.Round(time.Second).String(), + Head: v.Head, Trunk: v.Trunk} + if v.Repo != nil && (v.Gate == nil || v.Gate.Verdict == "pass") { + t.Verdict, t.Summary = v.Repo.Verdict, v.Repo.Summary + } + if v.Repo != nil && v.Repo.Failed != "" { + t.Failed = strings.Split(v.Repo.Failed, "\n") + } + if t.Verdict == "" { + t.Verdict = "error" + } + return t +} + // 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 diff --git a/cmd/mesh-controller/main.go b/cmd/mesh-controller/main.go index 1e656622..41d7319d 100644 --- a/cmd/mesh-controller/main.go +++ b/cmd/mesh-controller/main.go @@ -106,6 +106,8 @@ func run() error { return killCommand(ctx, args[1:]) case "forget-checks": return forgetChecksCommand(ctx, args[1:]) + case "try": + return tryCommand(ctx, args[1:]) case "pause", "resume": return pauseCommand(ctx, args[0], args[1:]) case "collection": @@ -381,6 +383,13 @@ func (b builds) Built(ctx context.Context, result link.BuildResult) error { // **A merge check builds nothing** (novox/hq to-be 45 §9): its verdict is said, and nothing of it is // recorded or registered. if result.Check != nil || result.Checked != nil { + if isTry(result) { + // A branch tried before its pull request (novox/hq issue 686): its asker reads the outcome from the + // build's log. Never said as `checked`, so the forge's holder sets no status. + fmt.Printf("%s: a try of %s at %.8s, read with `try --id`: said to nobody, nothing taken in\n", result.ID, + result.Repository, result.Ref) + return nil + } checked(ctx, result) return nil } diff --git a/cmd/mesh-controller/seatverbs.go b/cmd/mesh-controller/seatverbs.go index 3cc62d1a..c0c222e3 100644 --- a/cmd/mesh-controller/seatverbs.go +++ b/cmd/mesh-controller/seatverbs.go @@ -457,6 +457,28 @@ func (a *verbArguments) commandLine() ([]string, error) { return nil, err } return []string{"forget-checks", str("owner"), str("repo"), str("number")}, nil + case "try": + // A branch tried before its pull request (novox/hq issue 686): asked, or read by the id it answered. + if id := str("id"); id != "" { + return []string{"try", "--id", id}, nil + } + if a.given["repository"] == "" && a.given["ref"] == "" { + // Neither shape named: the command says its usage, which names both, as `build` does. + return []string{"try"}, nil + } + if err := need("repository", "ref"); err != nil { + return nil, err + } + argv := []string{"try", str("repository"), str("ref")} + for _, flag := range []string{"base", "tests"} { + if v := str(flag); v != "" { + argv = append(argv, "--"+flag, v) + } + } + if on("store-only") { + argv = append(argv, "--store-only") + } + return argv, nil case "cancel", "kill": if err := need("id"); err != nil { return nil, err @@ -1018,6 +1040,8 @@ var jsonVerbs = map[string]bool{"status": true, "seats": true, "plan": true, "co "delivery": true, // The build queue, each ask with its position among the waiting (novox/hq issue 455). "queue": true, + // A branch tried before its pull request (novox/hq issue 686). + "try": true, // A plan followed (novox/hq issue 229): `plans wait` answers JSON; its other forms say text, and so answer no data. "plans": true} @@ -1308,6 +1332,10 @@ func seatToolHandlers() (map[string]link.ToolHandler, []string, error) { link.AnswersWithin(ctx, bound+planWaitStart) } } + if verb == "try" && len(argv) > 1 && argv[1] == "--id" { + // Reading a try holds its answer for the verdict, up to its bound (novox/hq issue 686, 229). + link.AnswersWithin(ctx, tryWait+planWaitStart) + } if answer, read := readHere(ctx, argv); read { return answer, nil } diff --git a/cmd/mesh-controller/try.go b/cmd/mesh-controller/try.go new file mode 100644 index 00000000..9f586b5e --- /dev/null +++ b/cmd/mesh-controller/try.go @@ -0,0 +1,309 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "flag" + "fmt" + "net/url" + "os" + "regexp" + "strings" + "time" + + "github.com/nats-io/nats.go" + + "github.com/novox/mesh-controller/internal/broker" + "github.com/novox/mesh-controller/internal/link" +) + +// A branch is tried before its pull request (novox/hq issue 686). +// +// 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: +// +// - **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. +// +// 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. + +// 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 ( + tryRef = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._/-]*$`) + tryTests = regexp.MustCompile(`^[A-Za-z0-9_^$|().*+?/\[\]-]+$`) +) + +// tryAnswer is what `try` answers, asking or reading. +type tryAnswer struct { + ID string `json:"id"` + // State is asked (just now), waiting (nothing said of it yet: in the build queue), running, or the verdict: + // pass, warning (the repository declares no merge-check.sh), fail, or error (it could not run). + State string `json:"state"` + Said string `json:"said"` + Repository string `json:"repository,omitempty"` + Ref string `json:"ref,omitempty"` + Commit string `json:"commit,omitempty"` + Base string `json:"base,omitempty"` + Trunk string `json:"trunk,omitempty"` + On string `json:"on,omitempty"` + Took string `json:"took,omitempty"` + Failed []string `json:"failed,omitempty"` + // Last is the last thing the build seat said of one still running. + Last string `json:"last,omitempty"` +} + +// isTry says a check's outcome is a try's: marked so, or — echoed by a build seat from before the mark — a +// check of no pull request and no group, which is nobody's status either. +func isTry(result link.BuildResult) bool { + c := result.Checked + 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 +} + +// forgeCommit asks the forge, through its module's tools on the bus, which commit a branch or commit of a +// repository is, and the repository's default branch. A variable so a test needs no forge. +var forgeCommit = func(ctx context.Context, owner, repo, ref string) (commit, trunk string, err error) { + js, err := aBus() + if err != nil { + return "", "", err + } + defer js.Close() + bus := link.OverNATS{Conn: js.Conn()} + ask := func(path string, into any) error { + raw, _ := json.Marshal(map[string]any{"path": path}) + answer, err := link.Ask(ctx, bus, "gitea", "gitea_api", raw, 30*time.Second) + if err != nil { + return err + } + if answer.Error != "" { + return errors.New(answer.Error) + } + return json.Unmarshal(answer.Result, into) + } + var commits struct { + Result []struct { + SHA string `json:"sha"` + } `json:"result"` + } + if err := ask("/repos/"+owner+"/"+repo+"/commits?limit=1&stat=false&verification=false&files=false&sha="+ + url.QueryEscape(ref), &commits); err != nil { + return "", "", err + } + if len(commits.Result) == 0 || commits.Result[0].SHA == "" { + return "", "", errors.New("the forge named no commit for it") + } + var found struct { + Result struct { + DefaultBranch string `json:"default_branch"` + } `json:"result"` + } + if err := ask("/repos/"+owner+"/"+repo, &found); err != nil { + return "", "", err + } + return commits.Result[0].SHA, found.Result.DefaultBranch, nil +} + +// tryCommand is `try / [--base ] [--tests ] [--store-only]`, and +// `try --id ` to read what came of one. +func tryCommand(ctx context.Context, args []string) error { + set := flag.NewFlagSet("try", flag.ContinueOnError) + base := set.String("base", "", "the branch it is merged into (the repository's default branch when none)") + tests := set.String("tests", "", "only the tests this `go test -run` pattern names") + storeOnly := set.Bool("store-only", false, "only the repository's store tests") + id := set.String("id", "", "a try's id: what came of it") + positionals, err := parseAround(set, args) + if err != nil { + return err + } + usage := errors.New("try / [--base ] [--tests ] " + + "[--store-only], or try --id ") + if *id != "" { + if len(positionals) != 0 { + return usage + } + return tryRead(ctx, *id) + } + if len(positionals) != 2 { + return usage + } + owner, repo, isPath := strings.Cut(positionals[0], "/") + ref := positionals[1] + switch { + case !isPath || !tryRef.MatchString(owner) || !tryRef.MatchString(repo) || strings.Contains(repo, "/"): + return fmt.Errorf("%q is not a repository on the forge, owner/name; nothing was asked", positionals[0]) + case !tryRef.MatchString(ref): + return fmt.Errorf("%q is not a branch or a commit; nothing was asked", ref) + case *base != "" && !tryRef.MatchString(*base): + return fmt.Errorf("%q is not a branch; nothing was asked", *base) + case *tests != "" && !tryTests.MatchString(*tests): + return fmt.Errorf("%q is not a `go test -run` pattern of test names; nothing was asked", *tests) + } + // **An unknown ref is refused here, loudly** — not sent to a machine to fail at its clone minutes later. + commit, trunk, err := forgeCommit(ctx, owner, repo, ref) + if err != nil { + return fmt.Errorf("the forge holds no branch or commit %s in %s/%s — is it pushed? — so nothing was asked: %w", + ref, owner, repo, err) + } + if *base != "" { + trunk = *base + } + if trunk == "" { + return fmt.Errorf("the forge named no default branch for %s/%s, and no --base says what %s is merged into; "+ + "nothing was asked", owner, repo, ref) + } + + open, err := openStores(ctx) + if err != nil { + return err + } + defer open.Close() + entries, read, edges, err := theGraph(ctx, open.inventory) + if err != nil { + return err + } + p := link.PullUpdated{Owner: owner, Repo: repo, Base: trunk, Commit: commit} + // Whose repository it is, as a pull request's check reckons it: only the mesh's is tried, the code the + // build seat runs for every pull request already. + mesh := pullScope(p, entries, read, edges) + if !mesh.Mesh { + 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 + } + request = asATry(request, *tests, *storeOnly) + seat := buildSeatHeld(ctx) + 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)}) +} + +// 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 + } + a := tryAnswerOf(id, lines) + if (a.State != "waiting" && a.State != "running") || !time.Now().Add(2*time.Second).Before(until) { + return printTry(a) + } + if err := sleepOrDone(ctx, 2*time.Second); err != nil { + return err + } + } +} + +// 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 { + fmt.Fprintln(os.Stderr, " "+l) + } + out, err := json.MarshalIndent(a, "", " ") + if err != nil { + return err + } + fmt.Println(string(out)) + return nil +} + +// buildLines is everything the build seat said of one build, as the bus keeps it, in order. +func buildLines(ctx context.Context, id string) ([]link.BuildLine, error) { + js, err := aBus() + if err != nil { + return nil, fmt.Errorf("cannot reach the bus to read a build's log: %w", err) + } + defer js.Close() + subject := link.BuildLogOf("*", id) + sub, err := js.Context().PullSubscribe(subject, "", + nats.BindStream(broker.EventsStream), nats.DeliverAll(), nats.AckNone()) + if err != nil { + return nil, fmt.Errorf("cannot read %s from the bus: %w", subject, err) + } + defer func() { _ = sub.Unsubscribe() }() + var lines []link.BuildLine + for { + batch, err := sub.Fetch(200, nats.MaxWait(2*time.Second)) + if err != nil && !errors.Is(err, nats.ErrTimeout) && !errors.Is(err, context.DeadlineExceeded) { + return nil, fmt.Errorf("reading a build's log: %w", err) + } + for _, msg := range batch { + var line link.BuildLine + if json.Unmarshal(msg.Data, &line) == nil { + lines = append(lines, line) + } + } + if len(batch) < 200 { + return lines, nil + } + } +} diff --git a/internal/builder/check.go b/internal/builder/check.go index bf0ce5bc..a2107499 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -103,6 +103,23 @@ type CheckSpec struct { // for the walk to take. None for a group's composed check, whose members' own checks build. Builds []CheckBuildSpec BuildWith *CheckBuilding + // Tests and StoreOnly narrow what the repository's own script runs, for a branch tried before its pull + // request (novox/hq issue 686): handed to the script as EnvTests and EnvStoreOnly, which it may read. + Tests string + StoreOnly bool +} + +// selection is what a check hands the repository's script to narrow what it runs (novox/hq issue 686): nothing +// for a pull request's check, which always runs whole. +func selection(spec CheckSpec) []string { + var env []string + if spec.Tests != "" { + env = append(env, EnvTests+"="+spec.Tests) + } + if spec.StoreOnly { + env = append(env, EnvStoreOnly+"=1") + } + return env } // GroupHead is one other head of a delivery group's composed check. @@ -193,6 +210,11 @@ const ( // EnvGroup is a delivery group's other heads, as JSON, for the gate (novox/hq ADR 0239); empty for a // pull request checked alone. EnvGroup = "MESH_CHECK_GROUP" + // EnvTests is a `go test -run` pattern and EnvStoreOnly is 1 when a branch tried before its pull request + // asks for only those tests, or only the store tests (novox/hq issue 686). Never set for a pull request's + // check. A script that reads neither runs whole. + EnvTests = "MESH_CHECK_TESTS" + EnvStoreOnly = "MESH_CHECK_STORE_ONLY" ) // CheckTimeout bounds one check; a check that runs past it is an error, not a pass. @@ -515,6 +537,7 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry EnvModules + "=" + strings.Join(append(append([]string{}, spec.Modules...), spec.New...), ","), EnvGroup + "=" + groupFile, "GOCACHE=" + filepath.Join(workspace, "go-cache"), "GOMODCACHE=" + filepath.Join(workspace, "go-modules")} + env = append(env, selection(spec)...) v := CheckVerdict{} modules := append(append([]string{}, spec.Modules...), prefixed("new:", spec.New)...) diff --git a/internal/catalogue/verbs.go b/internal/catalogue/verbs.go index 673f62f4..669b7758 100644 --- a/internal/catalogue/verbs.go +++ b/internal/catalogue/verbs.go @@ -362,6 +362,25 @@ var ControllerVerbs = []Verb{ "register": "\"true\" to register what it builds", "older": "\"true\", with register: even though a newer build of the module is registered", }, []string{"id"}, "register", "older")}, + // novox/hq issue 686: a builder ran a branch's store tests for the first time in its pull request's merge check. + {Name: "try", Description: "Try a pushed branch or commit before its pull request (novox/hq issue 686): the build " + + "seat merges it into the repository's trunk and runs the repository's own merge-check.sh on the merge, in the " + + "toolchain containers and against the throwaway store and bus a pull request's merge check gets. Answers at " + + "once with an id; asked again with that id it waits up to 20 s and answers pass, fail or error, how long it " + + "took and the lines naming what failed — or that it is still waiting or running, to ask again. Sets no " + + "status on the forge, registers and builds nothing, runs no merge gate, counts as a check in the build queue " + + "(after deliveries) and is bounded as one. Only for a repository of the mesh; the ref must be on the forge.", + Input: schema(map[string]string{ + "repository": "the repository on the forge, owner/name; with ref", + "ref": "the branch or commit to try, as pushed to the forge", + "base": "the branch it is merged into (the repository's default branch when none is named)", + "tests": "only the tests this `go test -run` pattern names, where the repository's merge-check.sh reads " + + "MESH_CHECK_TESTS; a script that does not runs whole", + "store-only": "\"true\": only the repository's store tests, where its merge-check.sh reads " + + "MESH_CHECK_STORE_ONLY", + "id": "instead of a repository: the id a try answered, to read what came of it", + }, nil, "store-only"), + Replaces: []string{"sh merge-check.sh", "docker run postgres"}}, // novox/hq ADR 0350 §5: asked by the delivery's owner when the forge says a pull request closed unmerged. {Name: "forget-checks", Description: "Let go of every build a pull request's merge checks made for its merge in " + "the artifact store's scratch namespace (novox/hq ADR 0350 §5): asked when it closed unmerged; a merged one's go " + diff --git a/internal/link/build.go b/internal/link/build.go index 6eb16588..73384e9f 100644 --- a/internal/link/build.go +++ b/internal/link/build.go @@ -164,6 +164,30 @@ 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"` + // 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, + // nothing built for a merge. Tests narrows what the repository's script runs to the tests a pattern names, + // and StoreOnly to its store tests; a script that reads neither runs whole. + Try bool `json:"try,omitempty"` + Tests string `json:"tests,omitempty"` + StoreOnly bool `json:"store-only,omitempty"` +} + +// StepTried is the step a build seat says a try's outcome under, in the build's log: one line, a Tried as JSON +// (novox/hq issue 686). +const StepTried = "tried" + +// Tried is what came of a branch tried before its pull request (novox/hq issue 686): the verdict of the +// repository's own check for the ref merged into the trunk, how long it took, and the lines that name what +// failed, picked from the whole of its output (issue 460). +type Tried struct { + Verdict string `json:"verdict"` + Summary string `json:"summary"` + Took string `json:"took"` + Head string `json:"head,omitempty"` + Trunk string `json:"trunk,omitempty"` + Failed []string `json:"failed,omitempty"` } // CheckBuild is one module a check builds for its merge: from the merge it made when Merged — a module built from diff --git a/merge-check.sh b/merge-check.sh index e1e85687..36de3711 100755 --- a/merge-check.sh +++ b/merge-check.sh @@ -54,11 +54,26 @@ if [ -n "$unformatted" ]; then fi CGO_ENABLED=0 go vet ./... -if command -v gcc >/dev/null 2>&1; then - CGO_ENABLED=1 go test -race -count=1 -timeout 30m ./... +suite() { + if command -v gcc >/dev/null 2>&1; then + CGO_ENABLED=1 go test -race -count=1 -timeout 30m "$@" + else + echo "NOT RACE-CHECKED: the toolchain holds no C compiler; the suite runs without the race detector" + CGO_ENABLED=0 go test -count=1 -timeout 30m "$@" + fi +} +# **A branch tried before its pull request may ask for a part** (novox/hq issue 686): the controller's `try` verb +# hands MESH_CHECK_TESTS, a `go test -run` pattern, or MESH_CHECK_STORE_ONLY=1. A pull request's merge check is +# handed neither and always runs whole; a part that passes is said as a part, never as the check. +if [ -n "${MESH_CHECK_TESTS:-}" ]; then + echo "TRIED IN PART: only the tests matching $MESH_CHECK_TESTS, in every package" + suite -run "$MESH_CHECK_TESTS" ./... + exit 0 +fi +if [ "${MESH_CHECK_STORE_ONLY:-}" = 1 ]; then + echo "TRIED IN PART: only the store tests listed in testdata/store-tests" else - echo "NOT RACE-CHECKED: the toolchain holds no C compiler; the suite runs without the race detector" - CGO_ENABLED=0 go test -count=1 -timeout 30m ./... + suite ./... fi # **The store tests listed ran** (novox/hq issue 459): a test that needs the store skips without one, and a skip diff --git a/module.json b/module.json index 1b84a8c4..538f5cca 100644 --- a/module.json +++ b/module.json @@ -77,6 +77,7 @@ "replay", "kill", "forget-checks", + "try", "pause", "resume", "hand-act", From 3ce5a0239bb1bc7acf2afd9db545a2c6a0d6ab4b Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 02:29:28 +0200 Subject: [PATCH 3/3] Keep a try off the forge and bound how many run (hq issue 686) Review of #314: a try's start named its commit, which the forge's holder matches to an open pull request's head and marks pending. Its start now names no repository or commit. One try per commit and four in all, a try never ages ahead of deliveries or pull request checks, and its id is checked before it is a bus subject. --- cmd/mesh-controller/try.go | 44 ++++++++++++++++++++++++++ cmd/mesh-controller/try_test.go | 32 +++++++++++++++++++ internal/catalogue/verbs.go | 7 +++-- internal/link/builds.go | 19 ++++++++++++ internal/link/builds_nats.go | 5 +-- internal/link/queue.go | 3 ++ internal/link/queue_order.go | 11 +++++-- internal/link/try_test.go | 55 +++++++++++++++++++++++++++++++++ 8 files changed, 168 insertions(+), 8 deletions(-) create mode 100644 internal/link/try_test.go diff --git a/cmd/mesh-controller/try.go b/cmd/mesh-controller/try.go index 9f586b5e..a120385c 100644 --- a/cmd/mesh-controller/try.go +++ b/cmd/mesh-controller/try.go @@ -87,6 +87,26 @@ func asATry(request link.BuildRequest, tests string, storeOnly bool) link.BuildR return request } +// maxTries is how many tries may wait or run at once, mesh-wide: each holds a build machine for minutes. +const maxTries = 4 + +// 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 + } + n++ + c := a.Request.Check + if same == "" && strings.EqualFold(c.Owner, owner) && strings.EqualFold(c.Repo, repo) && a.Request.Ref == commit { + same = a.ID + } + } + return same, n < maxTries +} + // forgeCommit asks the forge, through its module's tools on the bus, which commit a branch or commit of a // repository is, and the repository's default branch. A variable so a test needs no forge. var forgeCommit = func(ctx context.Context, owner, repo, ref string) (commit, trunk string, err error) { @@ -148,6 +168,10 @@ 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) + } return tryRead(ctx, *id) } if len(positionals) != 2 { @@ -202,6 +226,26 @@ func tryCommand(ctx context.Context, args []string) error { } 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 + } + queue, err := link.ReadQueue(ctx, js, seat) + js.Close() + if err != nil { + return 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 diff --git a/cmd/mesh-controller/try_test.go b/cmd/mesh-controller/try_test.go index 01bc05c9..5f72bcda 100644 --- a/cmd/mesh-controller/try_test.go +++ b/cmd/mesh-controller/try_test.go @@ -131,6 +131,38 @@ func TestATrysAnswerCarriesTheFailingLines(t *testing.T) { } } +// 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"} { + 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) + } + } +} + // The verb runs its command, and is declared with the shell habit it replaces (novox/hq ADR 0245). func TestTheTryVerbRunsItsCommand(t *testing.T) { for _, c := range []struct { diff --git a/internal/catalogue/verbs.go b/internal/catalogue/verbs.go index 669b7758..a186e558 100644 --- a/internal/catalogue/verbs.go +++ b/internal/catalogue/verbs.go @@ -369,11 +369,14 @@ var ControllerVerbs = []Verb{ "once with an id; asked again with that id it waits up to 20 s and answers pass, fail or error, how long it " + "took and the lines naming what failed — or that it is still waiting or running, to ask again. Sets no " + "status on the forge, registers and builds nothing, runs no merge gate, counts as a check in the build queue " + - "(after deliveries) and is bounded as one. Only for a repository of the mesh; the ref must be on the forge.", + "(after deliveries and after every pull request's check, however long it waits) and is bounded as one. One " + + "try of a commit at a time — a second ask answers the first's id — and at most four waiting or running in " + + "all: a fifth is refused. Only for a repository of the mesh; the ref must be on the forge.", Input: schema(map[string]string{ "repository": "the repository on the forge, owner/name; with ref", "ref": "the branch or commit to try, as pushed to the forge", - "base": "the branch it is merged into (the repository's default branch when none is named)", + "base": "the branch it is merged into (the repository's default branch when none is named). The " + + "merge-check.sh that runs is that branch's, never the ref's own: the same trust as a pull request's base", "tests": "only the tests this `go test -run` pattern names, where the repository's merge-check.sh reads " + "MESH_CHECK_TESTS; a script that does not runs whole", "store-only": "\"true\": only the repository's store tests, where its merge-check.sh reads " + diff --git a/internal/link/builds.go b/internal/link/builds.go index 7fe20541..f372d056 100644 --- a/internal/link/builds.go +++ b/internal/link/builds.go @@ -96,8 +96,27 @@ type BuildStart struct { // an earlier taker left unfinished — most often a build agent replaced mid-build — and started // again from the beginning (novox/hq issue 594). 0 from a machine that predates it. Delivery int `json:"delivery,omitempty"` + // Try marks the start of a branch tried before its pull request (novox/hq issue 686), which names no + // repository and no ref (startOf). + Try bool `json:"try,omitempty"` } +// startOf is what a build machine says of the ask it took. **A try's start names no repository and no commit** +// (novox/hq issue 686): the forge's holder writes mesh/merge-gate pending on an open pull request whose head is +// the commit a start names, and a try of that head is no check of the pull request — nothing on the forge moves +// for a try. Its id, machine and time are all the queue reads of a start. +func startOf(r BuildRequest, on string, delivery int, at time.Time) BuildStart { + s := BuildStart{ID: r.ID, Repository: r.Repository, Path: r.Path, Ref: r.Ref, On: on, + At: at.UTC().Format(time.RFC3339Nano), Delivery: delivery} + if IsTry(r) { + s.Repository, s.Path, s.Ref, s.Try = "", "", "", true + } + return s +} + +// IsTry says an ask is a branch tried before its pull request (novox/hq issue 686). +func IsTry(r BuildRequest) bool { return r.Check != nil && r.Check.Try } + // Restarted is how a build handed out again says so (novox/hq issue 594), in its log and in its check's // report, so a second run never reads as the first. Neutral on why: a holder replaced mid-build, a paused // holder handing an ask back as it arrived, and a bus that held its acknowledgements past the wait all diff --git a/internal/link/builds_nats.go b/internal/link/builds_nats.go index c7d0c67e..5ec3b9c1 100644 --- a/internal/link/builds_nats.go +++ b/internal/link/builds_nats.go @@ -401,10 +401,7 @@ func (b *natsBuild) Done() error { return b.msg.Ack() } // Began publishes that this machine has taken the build, into the stream like the outcome, so a // reader that asks afterwards sees when it started as well as how it ended. func (b *natsBuild) Began(ctx context.Context) error { - body, err := json.Marshal(BuildStart{ - ID: b.request.ID, Repository: b.request.Repository, Path: b.request.Path, - Ref: b.request.Ref, On: b.on, At: time.Now().UTC().Format(time.RFC3339Nano), Delivery: b.delivery, - }) + body, err := json.Marshal(startOf(b.request, b.on, b.delivery, time.Now())) if err != nil { return err } diff --git a/internal/link/queue.go b/internal/link/queue.go index 50865ef8..3bbbb8a2 100644 --- a/internal/link/queue.go +++ b/internal/link/queue.go @@ -427,6 +427,9 @@ func ReadCheckBuilds(ctx context.Context, js *broker.JetStream, seat string, pul // safeKey is an id that can be a key, and a subject token, as it is: a build id, never a pattern. var safeKey = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) +// SafeBuildID says an id can be a subject token as it is: a build's id, never a pattern. +func SafeBuildID(id string) bool { return safeKey.MatchString(id) } + // Cancelled is what the controller writes for an ask it cancelled. type Cancelled struct { At string `json:"at"` diff --git a/internal/link/queue_order.go b/internal/link/queue_order.go index 449618f9..90e68f50 100644 --- a/internal/link/queue_order.go +++ b/internal/link/queue_order.go @@ -26,6 +26,8 @@ import ( // - a check that has waited CheckWaitsAtMost goes before every delivery asked after it, so no check // waits for ever behind a stream of deliveries; // - within a kind, first come, first served, by when it was asked; +// - a branch tried before its pull request goes after everything else, however long it has waited: it is +// never promoted ahead of a delivery, and never ahead of a pull request's check (novox/hq issue 686); // - an ask whose kind cannot be told counts as a check: a delivery taken late breaks a budget, a check // taken late breaks nothing. // @@ -64,8 +66,12 @@ func (a QueuedAsk) askedOrStored() time.Time { // QueueOrder is the order the waiting asks are to be handed out in, now: the deliveries and every check // that has waited CheckWaitsAtMost, by when each was asked, then the other checks, by when each was asked. func QueueOrder(waiting []QueuedAsk, now time.Time) []QueuedAsk { - var first, then []QueuedAsk + var first, then, tries []QueuedAsk for _, a := range waiting { + if IsTry(a.Request) { + tries = append(tries, a) + continue + } if KindOf(a.Request) == KindCheck && now.Sub(a.askedOrStored()) < CheckWaitsAtMost { then = append(then, a) continue @@ -82,7 +88,8 @@ func QueueOrder(waiting []QueuedAsk, now time.Time) []QueuedAsk { } byAsking(first) byAsking(then) - return append(first, then...) + byAsking(tries) + return append(append(first, then...), tries...) } // toMove is the asks to move to the tail, in this order, for the queue to stand as order says: everything diff --git a/internal/link/try_test.go b/internal/link/try_test.go new file mode 100644 index 00000000..f434dc18 --- /dev/null +++ b/internal/link/try_test.go @@ -0,0 +1,55 @@ +package link + +import ( + "encoding/json" + "reflect" + "strings" + "testing" + "time" +) + +// The start of a try names no repository and no commit (novox/hq issue 686): the forge's holder writes +// mesh/merge-gate pending on the open pull request whose head a start names, and nothing on the forge moves +// for a try. A pull request's check still names both. +func TestATrysStartNamesNoCommitTheForgeCouldMatch(t *testing.T) { + sha := strings.Repeat("c0ffee", 7)[:40] + at := time.Date(2026, 10, 14, 0, 0, 0, 0, time.UTC) + try := BuildRequest{ID: "build-1", Repository: "https://forge/novox/a.git", Ref: sha, + Check: &CheckRequest{Owner: "novox", Repo: "a", Try: true}} + body, _ := json.Marshal(startOf(try, "ace", 1, at)) + if strings.Contains(string(body), sha) || strings.Contains(string(body), "novox/a") { + t.Fatalf("a try's start names what the forge matches a pull request by: %s", body) + } + if s := startOf(try, "ace", 2, at); s.ID != "build-1" || s.On != "ace" || !s.Try || s.Delivery != 2 || s.At == "" { + t.Fatalf("a try's start lost what the queue reads: %+v", s) + } + pull := try + pull.Check = &CheckRequest{Owner: "novox", Repo: "a", Number: 7} + if s := startOf(pull, "ace", 1, at); s.Ref != sha || s.Repository == "" || s.Try { + t.Fatalf("a pull request's check's start: %+v", s) + } +} + +// A try stays behind every delivery and every pull request's check, however long it has waited: the ten +// minutes that put a check ahead of later deliveries never promote a try (novox/hq issue 686). +func TestATryIsNeverHandedOutBeforeADeliveryOrAPullRequestsCheck(t *testing.T) { + now := time.Date(2026, 10, 12, 12, 0, 0, 0, time.UTC) + try := func(seq uint64, id string, asked time.Time) QueuedAsk { + a := anAsk(seq, id, asked, true) + a.Request.Check.Try = true + return a + } + waiting := []QueuedAsk{ + try(1, "try-old", now.Add(-40*time.Minute)), + try(2, "try-young", now.Add(-time.Minute)), + anAsk(3, "check-young", now.Add(-2*time.Minute), true), + anAsk(4, "delivery", now.Add(-time.Minute), false), + } + want := []string{"delivery", "check-young", "try-old", "try-young"} + if got := idsOf(QueueOrder(waiting, now)); !reflect.DeepEqual(got, want) { + t.Fatalf("handed out as %v, want %v", got, want) + } + if !SafeBuildID("build-20261014T000000-1a2b") || SafeBuildID("x.>") || SafeBuildID("*") || SafeBuildID("") { + t.Fatal("an id that is a pattern passes for a build's id") + } +}