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") + } +}