From 55936c1cd7d37bffe0eb77b6c3ae493b3368fd75 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 5 Nov 2026 18:05:24 +0100 Subject: [PATCH] Try a delivery group: members at exact commits, judged by the code of a group's check, every failure named (issue 1921) A try names several repositories, each the exact commit asked. A whole try (not narrowed to a level or tests) runs the group check's own path: the manifest validator, the module graph and the composition on every placed node, reading the indexer's candidate of the try's group. A try with no such candidate is an error that says so, never a pass. A try's candidate is no check: the checker leaves it alone and owes the forge nothing. Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../cmd/mesh-delivery-checker-tools/main.go | 52 +++-- .../mesh-delivery-checker-tools/main_test.go | 28 +++ .../mesh-delivery-checker/integration_test.go | 27 ++- .../cmd/mesh-delivery-checker/judge.go | 7 +- .../cmd/mesh-delivery-checker/main.go | 5 +- .../mesh-delivery-checker/try_candidates.go | 72 +++++++ .../internal/check/examiner_test.go | 10 + .../internal/check/graph_cycle.go | 2 +- .../internal/check/judge.go | 68 ++++++- .../internal/check/manifests_judge.go | 2 +- .../internal/check/read.go | 2 + .../internal/check/try.go | 189 +++++++++++++++--- .../internal/check/try_test.go | 142 +++++++++++-- 13 files changed, 528 insertions(+), 78 deletions(-) create mode 100644 modules/mesh-delivery-checker/cmd/mesh-delivery-checker/try_candidates.go diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker-tools/main.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker-tools/main.go index fd39fe86..d570a958 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker-tools/main.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker-tools/main.go @@ -103,15 +103,25 @@ func TryTools(tr Trying) []stdio.Tool { str := func(args map[string]any, k string) string { s, _ := args[k].(string); return s } return []stdio.Tool{ {Name: check.TryTool, - Description: "Try a pushed head before its pull request is opened: the checker judges it as it judges a pull request's head, merged onto its trunk's head, " + - "its unit and integration levels through the build modules, and owes the forge nothing. Answers the try's id at once; " + check.TryReadTool + " reads " + - "where it stands. A try of the same head that waits or runs is the answer to a second ask; a new try is refused while 4 tries run on a build node or 8 wait for one, mesh-wide; tries let in while waiting may later all run at once, and each node's build capacity bounds what runs on it. A repository " + - "the checker checks no level of (one whose trunk the gate does not name) is refused. A try whose ask no build node took within the step " + - "bound ends as error; its ask stays in the build seat's stream and may still run later, judged by nobody (issue 734).", - Input: map[string]any{"type": "object", "required": []string{"repository", "head"}, "properties": map[string]any{ - "repository": map[string]any{"type": "string", "description": "/"}, - "head": map[string]any{"type": "string", "description": "the full commit to try, 40 or 64 hex digits"}, - "trunk": map[string]any{"type": "string", "description": "the trunk commit to merge it onto, or the trunk's branch for its head now; the trunk's head when absent"}, + Description: "Try a delivery group, or one pushed head, before its pull requests are opened (try a group, group try, try several repositories): " + + "name the members, each a repository and the exact commit asked (never the branch's head now, so a push after the ask changes nothing of what is tried), " + + "or one repository and its head for a try of one member. The checker judges it by the code a group's pull-request check runs, so the two cannot drift: " + + "each member merged onto its trunk's head, the unit and the integration level of every member through the build modules, the manifest validator over " + + "the members' manifests, the module graph, and the composition on every placed node. Every failure is named (each member, level, test, manifest and node), not " + + "the first. A try owes the forge nothing (no status, no Approve asked) and writes no verdict of a pull request; what its passing levels prove is kept, so a pull request's " + + "check of the same heads on the same trunk runs only what the try did not pass. A try narrowed to a level or to some tests judges only those, no manifest and no composition, and keeps nothing of tests. " + + "Answers the try's id at once; " + check.TryReadTool + " reads where it stands, " + check.TryEndTool + " ends it by hand. A try of the same commits that waits or runs is the answer to a second ask " + + "(whatever the order of the members); a new try is refused while 4 tries run on a build node or 8 wait for one, mesh-wide. A repository the checker checks no level of (one whose trunk the gate does not name) " + + "is refused. A try whose ask no build node took within the step bound ends as error; its ask stays in the build seat's stream and may still run later, judged by nobody (issue 734).", + Input: map[string]any{"type": "object", "properties": map[string]any{ + "members": map[string]any{"type": "array", "description": "the group: one repository at most once, each at the exact commit asked and the trunk commit it is merged onto", + "items": map[string]any{"type": "object", "required": []string{"repository", "head"}, "properties": map[string]any{ + "repository": map[string]any{"type": "string", "description": "/"}, + "head": map[string]any{"type": "string", "description": "the full commit to try, 40 or 64 hex digits"}, + "trunk": map[string]any{"type": "string", "description": "the trunk commit to merge it onto, or the trunk's branch for its head now; the trunk's head when absent"}}}}, + "repository": map[string]any{"type": "string", "description": "/, for a try of one member (instead of members)"}, + "head": map[string]any{"type": "string", "description": "the full commit to try, 40 or 64 hex digits (with repository)"}, + "trunk": map[string]any{"type": "string", "description": "the trunk commit to merge it onto, or the trunk's branch for its head now; the trunk's head when absent (with repository)"}, "tests": map[string]any{"type": "string", "description": "a go test -run pattern: only those tests run, and nothing is kept"}, "level": map[string]any{"type": "string", "enum": []string{"unit", "integration"}, "description": "only this level; both when absent"}}}, Run: func(args map[string]any) (any, error) { @@ -126,16 +136,32 @@ func TryTools(tr Trying) []stdio.Tool { if tr.Now != nil { now = tr.Now } + var members []check.TryAskMember + if raw, ok := args["members"]; ok { + list, isList := raw.([]any) + if !isList { + return nil, fmt.Errorf("members is a list of {repository, head, trunk}") + } + for _, item := range list { + m, isMap := item.(map[string]any) + if !isMap { + return nil, fmt.Errorf("a member is {repository, head, trunk}") + } + members = append(members, check.TryAskMember{Repository: str(m, "repository"), Head: str(m, "head"), Trunk: str(m, "trunk")}) + } + } id, err := check.AskTry(tr.Tries, trunks, check.TryAsk{Repository: str(args, "repository"), Head: str(args, "head"), - Trunk: str(args, "trunk"), Tests: str(args, "tests"), Level: str(args, "level")}, now()) + Trunk: str(args, "trunk"), Tests: str(args, "tests"), Level: str(args, "level"), Members: members}, now()) if err != nil { return nil, err } return map[string]any{"id": id}, nil }}, {Name: check.TryReadTool, - Description: "Where a try stands, as stored: queued, running, or its end (pass, fail, or error when it could not run), with what was said of it, " + - "the head and the trunk commit it was merged onto, the time it took and what failed first; while it runs, each ask it put as soon as it is put, " + + Description: "Where a try (of a group or of one head) stands, read whole, as stored: queued, running, or its end (pass, fail, or error when it could not run, or " + + "when its manifests and composition could not be judged), with what was said of it, every member's commit and the trunk commit it was merged onto, the time it took, " + + "every failure one line each (members, levels, tests, manifests, the composition per node), the levels' tests run and kept, the composition on every placed node, " + + "the notes of the check (manifests judged, module graph, kept verdicts); while it runs, each ask it put as soon as it is put, " + "with the node that took it, its state and the phase its step says (fetch and merge, a level's digest, lint, tests) and since when. Read-only.", Input: map[string]any{"type": "object", "required": []string{"id"}, "properties": map[string]any{ "id": map[string]any{"type": "string", "description": "the try's id, try-"}}}, @@ -146,7 +172,7 @@ func TryTools(tr Trying) []stdio.Tool { return check.ReadTry(tr.Tries, str(args, "id")) }}, {Name: check.TryEndTool, - Description: "End a try that waits or runs, as error, by hand: say why (kept in what it says). Its place among the 4 is free at once, " + + Description: "End a try of a group (or of one head) that waits or runs, as error, by hand: say why (kept in what it says). Its place among the 4 is free at once, " + "the checker stops waiting for its asks, and the next " + check.TryTool + " of its head asks anew with a new id. For a try whose ask " + "was lost (no step runs for it in checker_status); the checker ends one by itself when the build seat gives its ask up, or no node " + "takes it within the step bound. A try that ended already is refused.", diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker-tools/main_test.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker-tools/main_test.go index 779922bc..8f460c3e 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker-tools/main_test.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker-tools/main_test.go @@ -106,6 +106,34 @@ func TestTheTryToolsAskAndReadATry(t *testing.T) { // ADR 0501 §5, the agreed i if _, err := verbs[0].Run(map[string]any{"repository": "novox/hq", "head": strings.Repeat("1", 40)}); err == nil { t.Error("the verb tried a repository nothing checks") } + // A group try (issue 1921): the members, each an exact commit; members and one repository together are refused; the + // description says what it does in the words a search for "try a group" finds. + later := TryTools(Trying{Tries: store, Ender: store, Now: func() time.Time { return time.Unix(2, 5) }, + Trunks: func() (map[string]string, error) { + return map[string]string{"novox/mesh-delivery": strings.Repeat("a", 40)}, nil + }}) + g, err := later[0].Run(map[string]any{"members": []any{map[string]any{"repository": "novox/mesh-delivery", "head": strings.Repeat("2", 40)}}}) + if err != nil || g.(map[string]any)["id"] == id { + t.Errorf("a group try = %v, %v, want a try of its own", g, err) + } + if _, err := tools[0].Run(map[string]any{"members": []any{map[string]any{"repository": "novox/mesh-delivery", "head": strings.Repeat("2", 40)}}, + "repository": "novox/mesh-delivery", "head": strings.Repeat("1", 40)}); err == nil { + t.Error("members and a repository were both taken") + } + if _, err := tools[0].Run(map[string]any{"members": "novox/mesh-delivery"}); err == nil { + t.Error("members that are no list were taken") + } + for _, tl := range tools { + for _, words := range []string{"group"} { + if !strings.Contains(strings.ToLower(tl.Description), words) { + t.Errorf("%s says nothing of a %s: %q", tl.Name, words, tl.Description) + } + } + } + if d := strings.ToLower(tools[0].Description); !strings.Contains(d, "try a group") || !strings.Contains(d, "exact commit") || !strings.Contains(d, "every failure") || + !strings.Contains(d, "composition on every placed node") || !strings.Contains(d, "owes the forge nothing") || !strings.Contains(d, "kept") { + t.Errorf("the try's description does not say what a group try does: %q", d) + } // Issue 734: a try whose ask was lost is ended by hand with why, and its head is asked anew. ended, err := tools[2].Run(map[string]any{"id": id, "why": "its ask was lost"}) if err != nil || ended.(map[string]any)["state"] != "error" { diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/integration_test.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/integration_test.go index 823deb6c..9eeb0c8f 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/integration_test.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/integration_test.go @@ -862,9 +862,11 @@ func (s triesStore) Create(key string, value []byte) error { // TestATryIsJudgedAndItsEndWrittenWithNoStatusOwed: a try written as checker_try writes it is taken by the running checker, // judged through the build modules as a pull request's head merged onto its trunk, and ended pass in its key, with no -// verdict and no status owed (ADR 0501 §5). +// verdict and no status owed (ADR 0501 §5). A whole try waits for the indexer's candidate of its group, which it reads on the +// key's watch, and writes the merge previews the indexer makes it from (issue 1921). func TestATryIsJudgedAndItsEndWrittenWithNoStatusOwed(t *testing.T) { b := startBed(t) + asked := b.engine(t, "n1") // a whole try is composed on every placed node, as a group's check is b.start(t, nil) store := triesStore{b.buckets[check.TriesBucket], b.ctx} id, err := check.AskTry(store, map[string]string{repo: trunk}, check.TryAsk{Repository: repo, Head: head1}, time.Now()) @@ -872,19 +874,36 @@ func TestATryIsJudgedAndItsEndWrittenWithNoStatusOwed(t *testing.T) { t.Fatal(err) } var last check.Try + // The indexer's candidate of the try's group (here written by the test), at the heads of its members. + raw0, _ := store.Get(id) + first, _ := check.DecodeTry(raw0) + cand := delivery.Candidate{V: delivery.Version, Group: id, HeadsHash: check.TryCase(id, first, nil).HeadsHash, + Pulls: []delivery.CandidatePull{{Head: delivery.Head{Repository: repo, Head: head1}, Modules: []string{}}}} + cand.FPSet = delivery.FingerprintSet(cand.Pulls) + data, err := delivery.EncodeCandidate(cand) + if err != nil { + t.Fatal(err) + } + ckey, _ := delivery.CandidatesKey(id) + if _, err := b.buckets[delivery.CandidatesBucket].Create(b.ctx, ckey, data); err != nil { + t.Fatal(err) + } for deadline := time.Now().Add(20 * time.Second); time.Now().Before(deadline); time.Sleep(50 * time.Millisecond) { raw, _ := store.Get(id) if last, err = check.DecodeTry(raw); err == nil && last.Over() { break } } - if last.State != check.TryPass || last.Trunk != trunk || last.Took == "" { - t.Fatalf("try = %+v, want passed on the trunk", last) + if last.State != check.TryPass || last.Trunk != trunk || last.Took == "" || last.Composition == nil { + t.Fatalf("try = %+v, want passed on the trunk with its composition made", last) + } + if got := asked(); len(got) != 1 || !strings.HasPrefix(got[0].Candidate, id+".") { + t.Errorf("the placed node's engine was asked %+v, want the try's candidate %s.", got, id) } if b.fake.count("mesh-build-go.build_go_fetch") == 0 { t.Errorf("the try fetched nothing") } - for _, name := range []string{delivery.CheckerOwedBucket, delivery.CheckerVerdictsBucket, delivery.PreviewsBucket} { + for _, name := range []string{delivery.CheckerOwedBucket, delivery.CheckerVerdictsBucket} { if keys, _ := b.buckets[name].Keys(b.ctx); len(keys) != 0 { t.Errorf("a try wrote %s: %v", name, keys) } diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/judge.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/judge.go index d826ebee..72cd4811 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/judge.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/judge.go @@ -29,7 +29,10 @@ type judgeDeps struct { trunks *check.Trunks kept check.KV previews check.KV - repos *repoModules + // candidates answers the indexer's candidate of a try's group (try_candidates.go); nil leaves a whole try with no + // manifest or composition judged, said. + candidates check.TryCandidates + repos *repoModules // published is the indexer's packages state as it is followed (packages.go). published *publishedPackages forge check.Forge @@ -157,7 +160,7 @@ func newJudge(d judgeDeps) check.Judge { nodes = d.nodes } ex := &check.Examiner{Nodes: nodes, Compositions: d.compositions, Asks: d.asks, Toolchains: d.tools, - Kept: check.Kept{Store: d.kept}, Previews: d.previews, Modules: d.repos, Packages: d.repos, Published: d.published, Forge: d.forge, Runs: d.repos, Builds: d.builds, BuildEnds: d.ends, BuildWait: d.buildWait, Announce: d.announce, Trunks: d.trunks, Manifests: d.repos, Read: d.repos, Predefined: d.predefined, Logf: d.logf} + Kept: check.Kept{Store: d.kept}, Previews: d.previews, Candidates: d.candidates, Modules: d.repos, Packages: d.repos, Published: d.published, Forge: d.forge, Runs: d.repos, Builds: d.builds, BuildEnds: d.ends, BuildWait: d.buildWait, Announce: d.announce, Trunks: d.trunks, Manifests: d.repos, Read: d.repos, Predefined: d.predefined, Logf: d.logf} if d.nodes != nil && d.builds != nil && d.repos != nil { ex.Serving = runningServes{have: d.nodes, builds: d.builds, read: d.repos, logf: d.logf} } diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go index 1cc2b977..fc2222e1 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go @@ -216,7 +216,7 @@ func run(ctx context.Context, getenv func(string) string, logOut io.Writer) erro ch.Runs = repos // the node-engine a check reads beside at its running commit (ADR 0548 §6) ch.Judge = newJudge(judgeDeps{ asks: buildrun.Asker{Publisher: asks, Waiter: waiter, Bound: stepBound}, tools: tools, trunks: trunks, - kept: kvStore{buckets[delivery.CheckerKeptBucket]}, previews: kvStore{buckets[delivery.PreviewsBucket]}, + kept: kvStore{buckets[delivery.CheckerKeptBucket]}, previews: kvStore{buckets[delivery.PreviewsBucket]}, candidates: tryCandidates{buckets[delivery.CandidatesBucket]}, repos: repos, published: published, builds: builds, ends: ends, buildWait: set.buildWait, forge: forgeReads{call: caller.Call}, logf: logger.Printf, announce: announce, compositions: compositions, nodes: openNodeNames(ctx, js, logger.Printf), @@ -333,6 +333,9 @@ func run(ctx context.Context, getenv func(string) string, logOut io.Writer) erro }) }() each := func(e jetstream.KeyValueEntry) { + if check.IsTryGroup(e.Key()) { + return // a group try's candidate: its try judges it, and the forge is owed nothing (issue 1921) + } if opOf(e) { ch.Candidate(ctx, e.Key(), e.Value()) } else { diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/try_candidates.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/try_candidates.go new file mode 100644 index 00000000..2eeb9a08 --- /dev/null +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/try_candidates.go @@ -0,0 +1,72 @@ +package main + +import ( + "context" + "fmt" + "time" + + "github.com/nats-io/nats.go/jetstream" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// candidateWaitsOnPreview is the None2 of a module the indexer wrote from a pull message alone, before it had the merge +// preview of the head (the indexer's NoneNoPreview): its fingerprint comes with the preview, so a candidate that still says +// it is not yet the candidate of the heads previewed. It moves to mesh-sdk's delivery package with the next change there. +const candidateWaitsOnPreview = "no merge preview is computed" + +// tryCandidates answers the indexer's candidate of a try's group (novox/hq ADR 0593 §7, issue 1921): the candidate the +// engines compose and the manifest validator judges, which the indexer makes of the try's previews. It waits for it on the +// key's own watch, with no timer between reads, and gives up at the bound. +type tryCandidates struct{ kv jetstream.KeyValue } + +func (t tryCandidates) TryCandidate(ctx context.Context, group, heads string, bound time.Duration) (delivery.Candidate, error) { + key, err := delivery.CandidatesKey(group) + if err != nil { + return delivery.Candidate{}, err + } + if bound <= 0 { + bound = time.Minute + } + waiting, cancel := context.WithTimeout(ctx, bound) + defer cancel() + w, err := t.kv.Watch(waiting, key) // the value now, then each change + if err != nil { + return delivery.Candidate{}, fmt.Errorf("the indexer's candidate %s cannot be watched: %w", key, err) + } + defer func() { _ = w.Stop() }() + last := "the indexer holds none" + for { + select { + case <-waiting.Done(): + return delivery.Candidate{}, fmt.Errorf("the indexer wrote no candidate of %s at heads %s within %s (%s)", group, heads, bound, last) + case e := <-w.Updates(): + if e == nil || e.Operation() != jetstream.KeyValuePut { + continue + } + c, err := delivery.DecodeCandidate(e.Value()) + switch { + case err != nil: + last = "what it holds is not a candidate: " + err.Error() + case c.HeadsHash != heads: + last = "it holds heads " + c.HeadsHash + case waitsOnPreview(c): + last = "it has not yet the merge previews' fingerprints" + default: + return c, nil + } + } + } +} + +// waitsOnPreview says a module of the candidate has no fingerprint yet because its preview was not read. +func waitsOnPreview(c delivery.Candidate) bool { + for _, p := range c.Pulls { + for _, m := range p.Fps { + if m.None2 == candidateWaitsOnPreview { + return true + } + } + } + return false +} diff --git a/modules/mesh-delivery-checker/internal/check/examiner_test.go b/modules/mesh-delivery-checker/internal/check/examiner_test.go index 5cf259ff..07a468f7 100644 --- a/modules/mesh-delivery-checker/internal/check/examiner_test.go +++ b/modules/mesh-delivery-checker/internal/check/examiner_test.go @@ -1040,8 +1040,14 @@ func TestTheIndexersJudgeJudgesTheChangesManifests(t *testing.T) { // trunk's, in a second ask (ADR 0633 clause 6, 1585/T22, T23) indexer string ownJudge bool + // try judges the same change as a whole group try, whose manifests come from the indexer's candidate of the try's + // group (issue 1921): the same judge, the same verdicts. + try bool }{ {name: "accepted"}, + {name: "accepted in a group try", try: true}, + {name: "refused in a group try", try: true, refused: "web has no id", fails: "the indexer's judge at mesh-delivery's trunk " + strings.Repeat("d", 40) + ` refuses the manifest of web: web has no id`}, + {name: "a cycle the group try makes", try: true, cycle: "provisions", fails: "the change's module graph has the cycle mail -> web -> mail"}, {name: "the indexer accepted by the trunk's manifest validator", indexer: "ok"}, {name: "the indexer held by the trunk's manifest validator", indexer: "held", fails: "the indexer web, the holder of the seat mesh-catalog, would never run: the trunk's manifest validator at mesh-delivery's trunk " + strings.Repeat("d", 40) + " does not accept its manifest: the manifest validator holds it, with no verdict yet"}, {name: "the indexer with a field the trunk's manifest validator does not know", indexer: "aside", fails: "the manifest validator does not know its fields new_field, so it waits"}, @@ -1120,6 +1126,10 @@ func TestTheIndexersJudgeJudgesTheChangesManifests(t *testing.T) { p.Modules = []string{"web"} p.Fps = map[string]delivery.CandidateModule{"web": {FP2: "src2:" + strings.Repeat("f", 64), Manifest: web}} cs := caseOf(t, p) + if row.try { + cs = TryCase("try-1", Try{Members: []TryMember{{Repository: repoA, Head: head1, Trunk: trunkA}}}, nil) + b.ex.Candidates = &fakeTryCandidates{cand: delivery.Candidate{FPSet: "fps", Pulls: []delivery.CandidatePull{p}}} + } v := b.judge(t, cs).Verdict if row.indexer == "ok" && !slices.ContainsFunc(cs.notes, func(n string) bool { return strings.Contains(n, "the new indexer's manifest of web is accepted by the trunk's manifest validator") diff --git a/modules/mesh-delivery-checker/internal/check/graph_cycle.go b/modules/mesh-delivery-checker/internal/check/graph_cycle.go index 56766476..b3209485 100644 --- a/modules/mesh-delivery-checker/internal/check/graph_cycle.go +++ b/modules/mesh-delivery-checker/internal/check/graph_cycle.go @@ -30,7 +30,7 @@ type ManifestReader interface { // refuseCycle fails the check for a cycle of the change's module graph, naming it; nil when the case touches no manifest, // when nothing reads manifests, or when there is none. A manifest that cannot be read fails the check, said, never passes it. func (st *state) refuseCycle(ctx context.Context) *delivery.Verdict { - if st.j.Manifests == nil || st.j.Read == nil || st.c.Try != nil { + if st.j.Manifests == nil || st.j.Read == nil || st.narrowTry() { return nil } trunk := st.j.Manifests.ManifestDigests() diff --git a/modules/mesh-delivery-checker/internal/check/judge.go b/modules/mesh-delivery-checker/internal/check/judge.go index 3480dca7..267f40c0 100644 --- a/modules/mesh-delivery-checker/internal/check/judge.go +++ b/modules/mesh-delivery-checker/internal/check/judge.go @@ -139,8 +139,18 @@ type Examiner struct { Nodes interface { Names(ctx context.Context) ([]string, error) } - Now func() time.Time - Logf func(format string, args ...any) + // Candidates answers the indexer's candidate of a try's group once it wrote it from the try's previews, the manifests + // and fingerprints a group try's manifest validator and composition judge (issue 1921); nil leaves a whole try with no + // manifest or composition judged, said. + Candidates TryCandidates + Now func() time.Time + Logf func(format string, args ...any) +} + +// TryCandidates answers the candidate the indexer holds for a try's group at its heads hash, waiting for it within the +// bound; an error says it did not come (and why). +type TryCandidates interface { + TryCandidate(ctx context.Context, group, heads string, bound time.Duration) (delivery.Candidate, error) } // CompositionReader makes a check's composition verdict; an error is only a case that cannot be composed at all. @@ -246,6 +256,46 @@ type state struct { // unpreviewed are the members (pullKey) with no preview of a merge: a head that does not merge, or a fetch that did not // answer. The indexer holds no manifest of them, so the assigner's composition would only wait: it is not awaited. unpreviewed []string + // noCandidate says why a whole try has no candidate of its group from the indexer, so its manifests were not judged and + // it was not composed; empty when it has one, and for a pull request's check. + noCandidate string +} + +// narrowTry says the case is a try narrowed to a level or to some tests: it judges no manifest, graph or composition. +func (st *state) narrowTry() bool { return st.c.Try != nil && !st.c.Try.Whole() } + +// takeTryCandidate gives a whole try's pulls the fingerprints and manifests the indexer's candidate of its group holds +// (the group's pull-request check has them from its candidate, CaseOf), once the indexer wrote it from the try's previews. +// A try with no such candidate is said so, in a note and in its composition, and is no pass (Tryer.run). +func (st *state) takeTryCandidate(ctx context.Context) { + if st.c.Try == nil || !st.c.Try.Whole() { + return + } + no := func(why string) { + st.noCandidate = "the group's manifests and composition were not judged: " + why + st.c.note(st.noCandidate) + } + if st.j.Candidates == nil { + no("the checker reads no candidate of the indexer's") + return + } + if len(st.unpreviewed) > 0 { + no("no preview of a merge of " + strings.Join(st.unpreviewed, ", ") + " (its head does not merge or its fetch did not answer)") + return + } + cand, err := st.j.Candidates.TryCandidate(ctx, st.c.Group, st.c.HeadsHash, st.j.buildWait()) + if err != nil { + no(err.Error()) + return + } + for i, p := range st.c.Pulls { + for _, cp := range cand.Pulls { + if cp.Repository == p.Repository && cp.Head.Head == p.Head { + st.c.Pulls[i].Modules, st.c.Pulls[i].Fps = cp.Modules, cp.Fps + } + } + } + st.c.FPSet = cand.FPSet } // Judge runs the case (it is the Judge of a Checker). A failure of the change, of a merge or of a step is a verdict; an error is only a case that cannot @@ -256,6 +306,7 @@ func (j *Examiner) Judge(ctx context.Context, c *Case) (Outcome, error) { // Every member and level runs to its end, and every manifest the change touches is judged by the indexer's judge (ADR // 0578 §2), also after a failure: the verdict names every failure, not only the first (the operator, 2026-11-02). tests := st.judgeAll(ctx) + st.takeTryCandidate(ctx) if v := settled(joinFailures(tests, st.judgeManifests(ctx), st.refuseCycle(ctx))); v != nil { return st.finish(ctx, *v), nil } @@ -282,7 +333,14 @@ func (st *state) composed(ctx context.Context, v *delivery.Verdict) { st.c.note("the composition is kept: no fingerprint of a module it composed changed") return } - if st.j.Compositions == nil || st.c.Try != nil { + if st.narrowTry() { + return + } + if st.noCandidate != "" { + v.Composition = &delivery.Composition{State: delivery.CompositionNotRun, Words: st.noCandidate} + return + } + if st.j.Compositions == nil { return } if len(st.unpreviewed) > 0 { @@ -433,7 +491,7 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { // Every build module's fetch merged (each is in merged once): the preview is written, also when the member's tests // failed, so the indexer holds its manifests and the assigner can compose the change (review of 650a1d0). switch { - case st.c.Try != nil: // a try owes the indexer no preview: no pull request holds its head + case st.narrowTry(): // a try of a level or some tests owes the indexer no preview: it judges no manifest case len(merged) == len(modules): st.j.writePreview(ctx, p, merged) default: @@ -562,7 +620,7 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l return fail(step.Node, fmt.Sprintf("the fetch of %s#%d answered no workspace: %s", p.Repository, p.Number, answer.Fetch)) } if len(t.fetched.Conflicts) > 0 { - if st.c.Try == nil { + if !st.narrowTry() { st.j.writePreview(ctx, p, append(*merged, t.fetched)) } return fail(step.Node, fmt.Sprintf("%s#%d %s%s", p.Repository, p.Number, ConflictWords, strings.Join(t.fetched.Conflicts, ", "))) diff --git a/modules/mesh-delivery-checker/internal/check/manifests_judge.go b/modules/mesh-delivery-checker/internal/check/manifests_judge.go index c34e8308..878fb438 100644 --- a/modules/mesh-delivery-checker/internal/check/manifests_judge.go +++ b/modules/mesh-delivery-checker/internal/check/manifests_judge.go @@ -97,7 +97,7 @@ func judgeRequest(trunk map[string]string, pulls []Pull) (req composer.Request, // refuses. A case that touches no manifest, or an examiner that reads no trunk manifests, judges nothing; a judge that // cannot be asked or does not answer fails the check, said, never passes it. func (st *state) judgeManifests(ctx context.Context) *delivery.Verdict { - if st.j.Manifests == nil || st.c.Try != nil { + if st.j.Manifests == nil || st.narrowTry() { return nil } req, touched := judgeRequest(st.j.Manifests.ManifestDigests(), st.c.Pulls) diff --git a/modules/mesh-delivery-checker/internal/check/read.go b/modules/mesh-delivery-checker/internal/check/read.go index 1fc0efbe..af42b94c 100644 --- a/modules/mesh-delivery-checker/internal/check/read.go +++ b/modules/mesh-delivery-checker/internal/check/read.go @@ -164,6 +164,7 @@ func (r Reader) Pull(repository string, number int) (VerdictRead, error) { if err != nil { return VerdictRead{}, err } + keys = slices.DeleteFunc(keys, IsTryGroup) sort.Strings(keys) keys, cut := capKeys(keys, "") // The candidates are read in batches, each in parallel and taken in key order, so the read stops at the batch that holds it. @@ -444,6 +445,7 @@ func (r Reader) Status() (Status, error) { if err != nil { return st, err } + keys = slices.DeleteFunc(keys, IsTryGroup) // a try's candidate is judged by its try, never as a check sort.Strings(keys) total := len(keys) keys, cut := capKeys(keys, "") diff --git a/modules/mesh-delivery-checker/internal/check/try.go b/modules/mesh-delivery-checker/internal/check/try.go index 990cf686..c34b8c7a 100644 --- a/modules/mesh-delivery-checker/internal/check/try.go +++ b/modules/mesh-delivery-checker/internal/check/try.go @@ -62,13 +62,17 @@ var ( // Try is the value of a tries key. type Try struct { - V int `json:"v"` - ID string `json:"id"` + V int `json:"v"` + ID string `json:"id"` + // Repository, Head, Trunk and Branch are the first member's (the try's headline, and all a try of one repository + // names); Members are every member of the try, by repository: each an exact commit merged onto a trunk commit (a + // group try, issue 1921). A try written before members read as one member. Repository string `json:"repository"` // Head is the commit tried; Trunk the trunk commit it is merged onto, and Branch the trunk's branch when it was asked by name. - Head string `json:"head"` - Trunk string `json:"trunk"` - Branch string `json:"branch,omitempty"` + Head string `json:"head"` + Trunk string `json:"trunk"` + Branch string `json:"branch,omitempty"` + Members []TryMember `json:"members,omitempty"` // Tests is a go test -run pattern: only those tests run, and nothing is kept. Level is unit or integration: only that // level runs. Empty is every test of both levels. Tests string `json:"tests,omitempty"` @@ -83,6 +87,11 @@ type Try struct { Failed []string `json:"failed,omitempty"` Levels map[string]delivery.LevelResult `json:"levels,omitempty"` Machines []string `json:"machines,omitempty"` + // Composition is what the composition on every placed node came to (a whole try, not one narrowed to a level or to + // some tests), and Notes the words the check noted on its way (the manifests judged, the module graph, the kept + // verdicts taken): the same as a group's pull-request check reports. + Composition *delivery.Composition `json:"composition,omitempty"` + Notes []string `json:"notes,omitempty"` // Asks are the ids of the asks the try put on the build seats' streams, each ending on the step key . of its // build module: the delivery view counts those steps as a try's, not a candidate's check (build_usage). Asks []string `json:"asks,omitempty"` @@ -94,6 +103,23 @@ type Try struct { Ended time.Time `json:"ended,omitzero"` } +// TryMember is one repository of a try: the exact commit asked, never the branch's head now, and the trunk commit it is +// merged onto. Branch is the trunk's branch when the trunk was asked by name. +type TryMember struct { + Repository string `json:"repository"` + Head string `json:"head"` + Trunk string `json:"trunk"` + Branch string `json:"branch,omitempty"` +} + +// MembersOf are the try's members: its Members, or the one its headline names. +func (t Try) MembersOf() []TryMember { + if len(t.Members) > 0 { + return t.Members + } + return []TryMember{{Repository: t.Repository, Head: t.Head, Trunk: t.Trunk, Branch: t.Branch}} +} + // TryAsked is a TryStep's state before a build node took its ask: it waits in the seat's stream. const TryAsked = "asked" @@ -195,6 +221,7 @@ func DecodeTry(raw []byte) (Try, error) { if !tryKeyOK.MatchString(t.ID) || t.Repository == "" || t.Head == "" || t.Trunk == "" { return Try{}, errors.New("not a try: it names no id, repository, head or trunk") } + t.Members = t.MembersOf() return t, nil } @@ -206,6 +233,14 @@ func encodeTry(t Try) ([]byte, error) { // TryAsk is what checker_try is given. type TryAsk struct { Repository, Head, Trunk, Tests, Level string + // Members are the repositories of a group try, each an exact commit and, optionally, its trunk (a commit, or the + // trunk's branch for its head now); the single Repository, Head and Trunk above are one member. Both at once are refused. + Members []TryAskMember +} + +// TryAskMember is one repository of a group try as it is asked. +type TryAskMember struct { + Repository, Head, Trunk string } // TryStore is the tries state as the tools see it. @@ -221,21 +256,16 @@ func NotChecked(repository string) error { return fmt.Errorf("no level of %s is checked by the checker", repository) } -// AskTry writes a new try, queued, and answers its id: a try of the same head that waits or runs is the answer to a second -// ask, and a new try is refused while MaxTries tries run on a build node or MaxTriesWaiting wait for one. trunks are the -// gate's trunk heads of every repository (source.main): a repository with none is no repository the checker checks. A trunk asked by name is its trunk head now. -func AskTry(store TryStore, trunks map[string]string, a TryAsk, now time.Time) (string, error) { +// tryMember checks one member as it is asked and resolves its trunk: the gate's trunk head of its repository, a commit +// asked, or the trunk's branch (its head now). A repository with no trunk head is no repository the checker checks. +func tryMember(trunks map[string]string, a TryAskMember) (TryMember, error) { repo := strings.ToLower(strings.TrimSuffix(strings.TrimSpace(a.Repository), ".git")) head := strings.ToLower(strings.TrimSpace(a.Head)) switch { case !tryRepoOK.MatchString(repo) || strings.Contains(repo, ".."): - return "", fmt.Errorf("%q is not a repository, /", a.Repository) + return TryMember{}, fmt.Errorf("%q is not a repository, /", a.Repository) case !tryHeadOK.MatchString(head): - return "", fmt.Errorf("the head %q is not a full commit, 40 or 64 hex digits", a.Head) - case a.Level != "" && a.Level != delivery.LevelUnit && a.Level != delivery.LevelIntegration: - return "", fmt.Errorf("the level %q is neither unit nor integration", a.Level) - case a.Tests != "" && !tryTestsOK.MatchString(a.Tests): - return "", fmt.Errorf("the tests %q are not a go test -run pattern", a.Tests) + return TryMember{}, fmt.Errorf("the head %q of %s is not a full commit, 40 or 64 hex digits", a.Head, repo) } trunkHead := "" for r, h := range trunks { @@ -244,20 +274,67 @@ func AskTry(store TryStore, trunks map[string]string, a TryAsk, now time.Time) ( } } if trunkHead == "" { - return "", NotChecked(repo) + return TryMember{}, NotChecked(repo) } - t := Try{Repository: repo, Head: head, Tests: a.Tests, Level: a.Level, State: TryQueued, Asked: now.UTC()} + m := TryMember{Repository: repo, Head: head} switch trunk := strings.TrimSpace(a.Trunk); { case trunk == "": - t.Trunk = trunkHead + m.Trunk = trunkHead case tryHeadOK.MatchString(strings.ToLower(trunk)): - t.Trunk = strings.ToLower(trunk) + m.Trunk = strings.ToLower(trunk) case tryTrunkOK.MatchString(trunk) && !strings.Contains(trunk, ".."): // The checker judges pull requests into the trunk alone (ADR 0484 clause 11): a branch named is the trunk's head now. - t.Trunk, t.Branch = trunkHead, trunk + m.Trunk, m.Branch = trunkHead, trunk default: - return "", fmt.Errorf("the trunk %q is neither a full commit nor a branch", a.Trunk) + return TryMember{}, fmt.Errorf("the trunk %q of %s is neither a full commit nor a branch", a.Trunk, repo) } + return m, nil +} + +// sameMembers says whether two tries ask the same commits onto the same trunk commits. +func sameMembers(a, b []TryMember) bool { + return slices.EqualFunc(a, b, func(x, y TryMember) bool { + return x.Repository == y.Repository && x.Head == y.Head && x.Trunk == y.Trunk + }) +} + +// AskTry writes a new try, queued, and answers its id: a try of the same commits that waits or runs is the answer to a second +// ask, and a new try is refused while MaxTries tries run on a build node or MaxTriesWaiting wait for one. A try is a group of +// one or more members (issue 1921), each an exact commit, never the branch's head now: a push after the ask changes nothing +// of what is tried. trunks are the gate's trunk heads of every repository (source.main): a repository with none is no +// repository the checker checks. A trunk asked by name is its trunk head now. +func AskTry(store TryStore, trunks map[string]string, a TryAsk, now time.Time) (string, error) { + asked := a.Members + if a.Repository != "" || a.Head != "" || a.Trunk != "" { + if len(asked) > 0 { + return "", errors.New("name the members, or one repository with its head, not both") + } + asked = []TryAskMember{{Repository: a.Repository, Head: a.Head, Trunk: a.Trunk}} + } + if len(asked) == 0 { + return "", errors.New("name what to try: members (repository and head each), or a repository and its head") + } + switch { + case a.Level != "" && a.Level != delivery.LevelUnit && a.Level != delivery.LevelIntegration: + return "", fmt.Errorf("the level %q is neither unit nor integration", a.Level) + case a.Tests != "" && !tryTestsOK.MatchString(a.Tests): + return "", fmt.Errorf("the tests %q are not a go test -run pattern", a.Tests) + } + var members []TryMember + for _, m := range asked { + tm, err := tryMember(trunks, m) + if err != nil { + return "", err + } + if slices.ContainsFunc(members, func(o TryMember) bool { return o.Repository == tm.Repository }) { + return "", fmt.Errorf("%s is named twice: a try holds one commit of a repository", tm.Repository) + } + members = append(members, tm) + } + slices.SortFunc(members, func(x, y TryMember) int { return strings.Compare(x.Repository, y.Repository) }) + first := members[0] + t := Try{Repository: first.Repository, Head: first.Head, Trunk: first.Trunk, Branch: first.Branch, Members: members, + Tests: a.Tests, Level: a.Level, State: TryQueued, Asked: now.UTC()} keys, err := store.Keys() if err != nil { return "", fmt.Errorf("the tries cannot be listed: %w", err) @@ -291,10 +368,10 @@ func AskTry(store TryStore, trunks map[string]string, a TryAsk, now time.Time) ( if !live[i] { continue } - if cur.Repository == t.Repository && cur.Head == t.Head && cur.Trunk == t.Trunk && cur.Tests == t.Tests && cur.Level == t.Level { + if sameMembers(cur.MembersOf(), t.Members) && cur.Tests == t.Tests && cur.Level == t.Level { return cur.ID, nil } - said := fmt.Sprintf("%s (%s@%.12s, %s)", cur.ID, cur.Repository, cur.Head, cur.State) + said := fmt.Sprintf("%s (%s, %s)", cur.ID, sayMembers(cur.MembersOf()), cur.State) if cur.HoldsANode() { holding = append(holding, said) } else { @@ -379,12 +456,50 @@ func EndTry(store TryEnder, id, why string, now time.Time) (Try, error) { return t, nil } +// IsTryGroup says a candidates key is the group of a try (the indexer makes the candidate of a group try's previews, for the +// manifest validator and the composition): the try judges it, so the checker's check of candidates leaves it alone, writes +// no verdict and owes the forge no status for it. +func IsTryGroup(key string) bool { return tryKeyOK.MatchString(key) } + // TryOptions narrow what a try runs; a pull request's check has none. type TryOptions struct { Level string Run string } +// Whole says the try runs everything a group's pull-request check runs (both levels of every member, the manifests judged by +// the manifest validator, the module graph, the composition on every placed node): it is narrowed to neither a level nor +// some tests. Such a try judges by the same code as the check, so the two cannot drift (issue 1921). +func (o *TryOptions) Whole() bool { return o != nil && o.Level == "" && o.Run == "" } + +// TryCase is the case a try is judged as: one pull of the group per member at its exact commit, with no pull request number +// (no ref: the commit is fetched as it is), the group named by the try's key, and the heads hash its candidate is keyed by. +func TryCase(key string, t Try, watch func(string, *delivery.Step)) *Case { + cs := &Case{Key: key, Group: key, Watch: watch, Trunk: map[string]string{}, Queued: t.Asked, Progress: &Progress{}, + Try: &TryOptions{Level: t.Level, Run: t.Tests}} + var heads []delivery.Head + for _, m := range t.MembersOf() { + cs.Pulls = append(cs.Pulls, Pull{Repository: m.Repository, Head: m.Head, Trunk: m.Trunk}) + cs.Trunk[m.Repository] = m.Trunk + heads = append(heads, delivery.Head{Repository: m.Repository, Head: m.Head}) + } + cs.HeadsHash, _ = delivery.HeadsHash(heads) + cs.Heads = cs.HeadsHash + return cs +} + +// compositionMade says whether a composition was made; notMadeWords what it says when it was not. +func compositionMade(c *delivery.Composition) bool { + return c != nil && c.State != delivery.CompositionNotRun +} + +func notMadeWords(c *delivery.Composition) string { + if c == nil || c.Words == "" { + return "no reason was given" + } + return c.Words +} + // Tryer runs the tries: it takes a queued try, judges it through the same Judge as a pull request's check (a check ask, ADR // 0525 §1), and writes its end. It holds no timer: a try starts when its key is written. type Tryer struct { @@ -502,12 +617,7 @@ func (tr *Tryer) run(ctx context.Context, key string) { return } progress := &tryWriter{tr: tr, ctx: ctx, key: key, t: t, rev: rev} - cs := &Case{Key: key, Group: key, Watch: progress.heard, - Pulls: []Pull{{Repository: t.Repository, Head: t.Head, Trunk: t.Trunk}}, - Trunk: map[string]string{t.Repository: t.Trunk}, - Queued: t.Asked, - Progress: &Progress{}, - Try: &TryOptions{Level: t.Level, Run: t.Tests}} + cs := TryCase(key, t, progress.heard) judging := ctx if tr.NotTaken > 0 { judging = buildrun.NotTakenBound(ctx, tr.NotTaken) @@ -521,8 +631,13 @@ func (tr *Tryer) run(ctx context.Context, key string) { t.Ended = tr.now().UTC() t.Took = t.Ended.Sub(t.Started).Round(time.Second).String() t.Levels, t.Machines, t.Asks = v.Levels, v.Machines, cs.Asks() + t.Composition = v.Composition t.Said = Report(v, cs, cs.Pulls[0], out.Builds, out.Slowest, out.Judged) - for _, n := range cs.Notes() { + if len(cs.Pulls) > 1 { + t.Said = sayMembers(t.MembersOf()) + ": " + t.Said + } + t.Notes = cs.Notes() + for _, n := range t.Notes { t.Said += " | " + n } switch { @@ -532,6 +647,11 @@ func (tr *Tryer) run(ctx context.Context, key string) { if IsLostAsk(v) || IsNotTaken(v) { t.Failed = []string{v.Failed.Words} } + case v.State == delivery.VerdictPass && cs.Try.Whole() && !compositionMade(v.Composition): + // A whole try that judged the tests and not the manifests and the composition has not tried what its group's check + // judges: it is no pass, and it says why (failure must be loud). It is an error, not the change's failure. + t.State = TryError + t.Failed = []string{"the composition was not made: " + notMadeWords(v.Composition)} case v.State == delivery.VerdictPass: t.State = TryPass default: @@ -579,6 +699,15 @@ func judgeWithin(ctx context.Context, j Judge, cs *Case, bound time.Duration) (O return out, true } +// sayMembers names a try's members, each repository at its short commit. +func sayMembers(ms []TryMember) string { + var out []string + for _, m := range ms { + out = append(out, fmt.Sprintf("%s@%.12s", m.Repository, m.Head)) + } + return strings.Join(out, " + ") +} + // orNone joins the tries said, or says none. func orNone(said []string) string { if len(said) == 0 { diff --git a/modules/mesh-delivery-checker/internal/check/try_test.go b/modules/mesh-delivery-checker/internal/check/try_test.go index be2ea659..1f11b00e 100644 --- a/modules/mesh-delivery-checker/internal/check/try_test.go +++ b/modules/mesh-delivery-checker/internal/check/try_test.go @@ -3,6 +3,7 @@ package check import ( "context" "encoding/json" + "errors" "fmt" "strings" "testing" @@ -70,6 +71,22 @@ func TestATryIsAskedQueuedOncePerHeadAndAtMostFourAtOnce(t *testing.T) { // ADR } // Issue 1890: tries on a build node (a step of theirs running, or none named yet) and tries waiting for one (queued, or // their asks not taken) are bounded apart, MaxTries and MaxTriesWaiting; the refusal names both counts. + // A group try is the same try whatever the order of its members; the commits asked are the commits tried. + group := TryAsk{Members: []TryAskMember{{Repository: repoSDK, Head: head2}, {Repository: repoA, Head: head1}}} + gid, err := AskTry(store, tryTrunks, group, at.Add(time.Second)) + if err != nil || gid == id { + t.Fatalf("the group try = %q, %v, want a try of its own", gid, err) + } + group.Members[0], group.Members[1] = group.Members[1], group.Members[0] + if again, err := AskTry(store, tryTrunks, group, at.Add(time.Minute)); err != nil || again != gid { + t.Errorf("the group asked in another order answered %q, %v, want %q", again, err, gid) + } + if g := readTry(t, kv, gid); len(g.Members) != 2 || g.Members[0].Repository != repoA || g.Members[0].Head != head1 || g.Members[1].Trunk != trunkS { + t.Errorf("group try = %+v, want its members by repository at the commits asked", g.Members) + } + if !IsTryGroup(gid) || IsTryGroup("feat/x") || IsTryGroup("fix/1921-try-1") { + t.Errorf("which candidates keys are a try's group: %s is, a branch is not", gid) + } onNode := func(i int) Try { return Try{ID: fmt.Sprintf("try-%d", i), Repository: repoA, Head: fmt.Sprintf("%040x", i), Trunk: trunkA, State: TryRunning, Steps: []TryStep{{Ask: "a", State: delivery.StepRunning}}} @@ -112,59 +129,113 @@ func TestATryOfARepositoryWithNoCheckedLevelIsRefused(t *testing.T) { // the agr } } for _, bad := range []TryAsk{{Repository: repoA, Head: "abc"}, {Repository: repoA, Head: head1, Level: "e2e"}, - {Repository: repoA, Head: head1, Tests: "a b"}, {Repository: "x", Head: head1}, {Repository: repoA, Head: head1, Trunk: "-x"}} { + {Repository: repoA, Head: head1, Tests: "a b"}, {Repository: "x", Head: head1}, {Repository: repoA, Head: head1, Trunk: "-x"}, + // A group try (issue 1921): members or one repository, never both or none; one commit per repository; every member checked. + {}, + {Repository: repoA, Head: head1, Members: []TryAskMember{{Repository: repoSDK, Head: head2}}}, + {Members: []TryAskMember{{Repository: repoA, Head: head1}, {Repository: repoA, Head: head2}}}, + {Members: []TryAskMember{{Repository: repoA, Head: head1}, {Repository: repoSDK, Head: "main"}}}, + {Members: []TryAskMember{{Repository: repoA, Head: head1}, {Repository: "novox/no-trunk", Head: head2}}}} { if _, err := AskTry(store, tryTrunks, bad, time.Now()); err == nil { t.Errorf("%+v was taken", bad) } } } -func TestATryIsJudgedAsAPullRequestsHeadAndOwesTheForgeNothing(t *testing.T) { // ADR 0501 §5 +// fakeTryCandidates answers the indexer's candidate of a try's group from memory, recording the keys asked. +type fakeTryCandidates struct { + cand delivery.Candidate + err error + asked []string +} + +func (f *fakeTryCandidates) TryCandidate(_ context.Context, group, heads string, _ time.Duration) (delivery.Candidate, error) { + f.asked = append(f.asked, group+"."+heads) + return f.cand, f.err +} + +// wholeBench makes the bench judge a whole try as the checker does: the composition on every placed node and the +// indexer's candidate of the try's group. +func wholeBench(b *bench, cand delivery.Candidate) (*fakeCompositions, *fakeTryCandidates) { + comps := &fakeCompositions{v: delivery.CompositionVerdict{State: delivery.CompositionPass, SDK: "v0.1.14-x", Machines: []string{"g14", "shanks"}, Took: 40}} + cands := &fakeTryCandidates{cand: cand} + b.ex.Compositions, b.ex.Candidates = comps, cands + return comps, cands +} + +// A group try (issue 1921): several repositories each at the exact commit asked, judged by the code of a group's pull-request +// check; the two cannot drift, and a later check of the same heads keeps what the try passed. +func TestATryIsJudgedAsAGroupsCheckAndOwesTheForgeNothing(t *testing.T) { // ADR 0501 §5, issue 1921 ctx := context.Background() kv := newMemKV() - id, err := AskTry(memTries{kv}, tryTrunks, TryAsk{Repository: repoA, Head: head1, Trunk: trunkA}, time.Now()) + id, err := AskTry(memTries{kv}, tryTrunks, TryAsk{Members: []TryAskMember{{Repository: repoSDK, Head: head2, Trunk: trunkS}, {Repository: repoA, Head: head1, Trunk: trunkA}}}, time.Now()) if err != nil { t.Fatal(err) } b := newBench(t) + comps, cands := wholeBench(b, delivery.Candidate{FPSet: "fps-of-the-try"}) written := &writtenTries{memKV: kv} tr := &Tryer{Tries: written, Judge: b.ex} raw, _, _, _ := kv.Get(ctx, id) tr.Put(ctx, id, raw) tr.Wait() got := readTry(t, kv, id) - if got.State != TryPass || got.Took == "" || got.Head != head1 || got.Trunk != trunkA || got.Said == "" || got.Levels[delivery.LevelUnit].Run != 2 { + if got.State != TryPass || got.Took == "" || len(got.Members) != 2 || got.Members[0].Repository != repoA || got.Members[1].Head != head2 || + got.Said == "" || got.Levels[delivery.LevelUnit].Run+got.Levels[delivery.LevelUnit].Kept != 4 { // the bench gives both members the same digests t.Fatalf("try = %+v", got) } - // Issue 1890: while it runs, the try names its ask as soon as it is put, and then where its step stands; the end - // keeps each ask's node and state. - if len(written.running) < 3 || len(written.running[1].Asks) != 1 || written.running[1].Steps[0].State != TryAsked || - written.running[2].Steps[0].Node != "shanks" { - t.Errorf("the running try's writes = %+v, want its ask named while it ran, then its node", written.running) + if got.Composition == nil || got.Composition.State != delivery.CompositionPass || len(got.Machines) == 0 { + t.Errorf("the try's composition = %+v, want the composition on every placed node, as a check's", got.Composition) } - if len(got.Steps) != 1 || got.Steps[0].Node != "shanks" || got.Steps[0].State != delivery.StepPassed { - t.Errorf("the ended try's steps = %+v", got.Steps) + // The same key the indexer's candidate of the group has: the try's id and the heads of its members. + if len(comps.asked) != 1 || comps.asked[0] != id || len(cands.asked) != 1 || !strings.HasPrefix(cands.asked[0], id+".") { + t.Errorf("composed %v, candidate asked %v, want the try's key %s", comps.asked, cands.asked, id) } - // The try names the asks it put, so the delivery view counts their steps as a try's (build_usage). - if len(got.Asks) != 1 { - t.Errorf("asks = %v, want the one check ask the try put", got.Asks) - } else if _, err := delivery.AskStepKey("shanks", got.Asks[0]); err != nil { - t.Errorf("the try names %q, which is no ask id: %v", got.Asks[0], err) + // Issue 1890: while it runs, the try names its asks as soon as they are put, and then where each step stands. + if len(written.running) < 3 || len(written.running[1].Asks) != 1 || written.running[1].Steps[0].State != TryAsked { + t.Errorf("the running try's writes = %+v, want its ask named while it ran", written.running) } + if len(got.Steps) != 2 || got.Steps[0].Node != "shanks" || got.Steps[0].State != delivery.StepPassed || len(got.Asks) != 2 { + t.Errorf("the ended try's steps = %+v, asks %v", got.Steps, got.Asks) + } + for _, a := range got.Asks { // the delivery view counts their steps as a try's (build_usage) + if _, err := delivery.AskStepKey("shanks", a); err != nil { + t.Errorf("the try names %q, which is no ask id: %v", a, err) + } + } + // Each member is fetched at the exact commit asked, merged onto its own trunk, with no pull request's ref. fetch := b.steps.tool("fetch") - if len(fetch) != 1 || fetch[0].args["commit"] != head1 || fetch[0].args["base"] != trunkA || fetch[0].args["ref"] != "" { - t.Errorf("fetch = %+v, want the head merged onto the trunk, with no pull request's ref", fetch) + if len(fetch) != 2 { + t.Fatalf("fetch = %+v, want one per member", fetch) } - if keys, _ := b.prev.Keys(ctx); len(keys) != 0 { - t.Errorf("a try wrote the previews %v", keys) + for _, f := range fetch { + want := map[string][2]string{repoA: {head1, trunkA}, repoSDK: {head2, trunkS}}[f.args["repository"].(string)] + if f.args["commit"] != want[0] || f.args["base"] != want[1] || f.args["ref"] != "" { + t.Errorf("fetch = %+v, want the exact commit %s merged onto %s", f.args, want[0], want[1]) + } + } + // A whole try writes the merge previews the indexer makes its candidate from; it writes no status and asks no Approve. + if keys, _ := b.prev.Keys(ctx); len(keys) != 2 { + t.Errorf("a whole try wrote the previews %v, want one per member", keys) } // An ended try is not run again. raw, _, _, _ = kv.Get(ctx, id) tr.Put(ctx, id, raw) tr.Wait() - if n := len(b.steps.tool("fetch")); n != 1 { + if n := len(b.steps.tool("fetch")); n != 2 { t.Errorf("an ended try was fetched again: %d fetches", n) } + // The kept verdicts: what the try passed is kept under the digests of the merged trees, so the pull-request check of + // the same head on the same trunk runs none of it again (and a push of other code runs only what changed). + before := len(b.steps.tool("test")) + out := b.judge(t, caseOf(t, pullOf(repoA, 7, head1))) + if out.Verdict.State != delivery.VerdictPass || out.Verdict.Levels[delivery.LevelUnit].Kept == 0 || out.Verdict.Levels[delivery.LevelUnit].Run != 0 || + out.Verdict.Levels[delivery.LevelIntegration].Run != 0 { + t.Errorf("the check of the tried head = %+v, want everything the try passed kept and nothing run", out.Verdict.Levels) + } + if after := len(b.steps.tool("test")); after != before { + t.Errorf("the check of the tried head ran %d test steps, want none: the try's passes are kept", after-before) + } } func TestATryOfOneLevelAndSomeTestsRunsThemAloneAndKeepsNothing(t *testing.T) { @@ -208,6 +279,34 @@ func TestAFailingTryNamesWhatFailedAndAStepThatCannotRunIsAnError(t *testing.T) !strings.Contains(got.Failed[1], "integration: fixture/failing/bad.TestThreeIsFour") { // both levels ran (the operator, 2026-11-02) t.Errorf("try = %+v", got) } + // A whole try that could not judge the manifests and the composition is no pass; a failed composition is named beside + // the failed tests (the group's check does as much), and a candidate of other heads is not the try's. + for _, row := range []struct { + name string + cand *fakeTryCandidates + comp delivery.CompositionVerdict + state, said string + }{ + {name: "the indexer wrote no candidate", cand: &fakeTryCandidates{err: errors.New("the indexer wrote no candidate within 5m0s")}, state: TryError, said: "the indexer wrote no candidate within 5m0s"}, + {name: "a composition refused on a node", cand: &fakeTryCandidates{}, comp: delivery.CompositionVerdict{State: delivery.CompositionFail, Machine: "g14", Words: "not composed on g14: web: needs a state mail does not keep"}, + state: TryFail, said: "not composed on g14: web: needs a state mail does not keep"}, + } { + kv := newMemKV() + b := newBench(t) + comps, _ := wholeBench(b, delivery.Candidate{}) + b.ex.Candidates, comps.v = row.cand, row.comp + if row.comp.State == "" { + comps.v.State = delivery.CompositionPass + } + id, _ := AskTry(memTries{kv}, tryTrunks, TryAsk{Repository: repoA, Head: head1}, time.Now()) + tr := &Tryer{Tries: kv, Judge: b.ex} + raw, _, _, _ := kv.Get(ctx, id) + tr.Put(ctx, id, raw) + tr.Wait() + if got := readTry(t, kv, id); got.State != row.state || !strings.Contains(strings.Join(got.Failed, " | ")+got.Said, row.said) { + t.Errorf("%s: try = %s, failed %v, said %q, want %s naming %q", row.name, got.State, got.Failed, got.Said, row.state, row.said) + } + } // No build node is no error since R4g: the try's ask waits in the check stream until a node pulls it (ADR 0525 §4). } @@ -224,6 +323,7 @@ func TestATryLeftRunningIsTakenOver(t *testing.T) { t.Fatal(err) } b := newBench(t) + wholeBench(b, delivery.Candidate{}) tr := &Tryer{Tries: kv, Judge: b.ex} tr.Reconsider(ctx) tr.Wait()