From 1b9a2bc3021f09691a9fc577598b9d075e2b9c25 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 6 Nov 2026 22:32:51 +0100 Subject: [PATCH] The running checker's asking records facts and returns no verdict (issue 1921) Every asking function only records what it met; the one verdict is the rules' of the facts, so nothing in the running checker judges beside the checking program. Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../internal/check/every_failure_test.go | 16 ++-- .../internal/check/judge.go | 89 ++++++++++--------- 2 files changed, 52 insertions(+), 53 deletions(-) diff --git a/modules/mesh-delivery-checker/internal/check/every_failure_test.go b/modules/mesh-delivery-checker/internal/check/every_failure_test.go index a845e029..a25dac1b 100644 --- a/modules/mesh-delivery-checker/internal/check/every_failure_test.go +++ b/modules/mesh-delivery-checker/internal/check/every_failure_test.go @@ -4,7 +4,6 @@ import ( "encoding/json" "fmt" "mesh-delivery-checker/rules" - "reflect" "strings" "testing" @@ -24,17 +23,16 @@ func failingResult(t *testing.T, tests int, failed ...Failure) json.RawMessage { func judgeLevel(t *testing.T, tests ...json.RawMessage) (*delivery.Verdict, *state) { t.Helper() st := &state{j: &Examiner{}, c: &Case{}, levels: rules.NewTally()} - v, _ := st.level(t.Context(), &target{pull: Pull{Repository: "novox/x"}}, delivery.LevelAnswer{Level: delivery.LevelIntegration, Tests: tests}) - v = rules.Settled(rules.JoinFailures(v)) - // 1921 (B0, D1a): the facts the running checker gathered, judged by the rules, give the same failures and counts. + st.level(t.Context(), &target{pull: Pull{Repository: "novox/x"}}, delivery.LevelAnswer{Level: delivery.LevelIntegration, Tests: tests}) + // 1921 (B0, D1a): the running checker only gathers the level as a fact; the rules judge it, and count what it kept. judged := rules.Judge(rules.Facts{V: rules.FactsVersion, Steps: st.facts}) - switch { - case v == nil && judged.State != delivery.VerdictPass, v != nil && (judged.Failed != v.Failed || !reflect.DeepEqual(judged.Failures, v.Failures)): - t.Errorf("the rules judge the facts %+v, the running checker %+v", judged, v) - case judged.Levels[delivery.LevelIntegration] != (delivery.LevelResult{Run: st.levels[delivery.LevelIntegration].Run}): + if judged.Levels[delivery.LevelIntegration] != (delivery.LevelResult{Run: st.levels[delivery.LevelIntegration].Run}) { t.Errorf("the rules count %+v, the running checker %+v", judged.Levels, *st.levels[delivery.LevelIntegration]) } - return v, st + if judged.State == delivery.VerdictPass { + return nil, st + } + return &judged, st } // Issue 1921: two failing tests in two packages are two entries of the verdict's Failures and two lines of a try's diff --git a/modules/mesh-delivery-checker/internal/check/judge.go b/modules/mesh-delivery-checker/internal/check/judge.go index 5faa8b33..78856a58 100644 --- a/modules/mesh-delivery-checker/internal/check/judge.go +++ b/modules/mesh-delivery-checker/internal/check/judge.go @@ -47,9 +47,6 @@ const ( judgedByCandidate = "judged by the candidate's mesh-build-go at " ) -// verdictPtr is a verdict of failure a step of the check ends with. -type verdictPtr = *delivery.Verdict - // Asks puts a check's ask on its build seat's stream and waits for its step (buildrun.Asker; ADR 0525 §1, §6): the // checker chooses no node, starts no step and holds none; whichever build node pulls the ask runs the check. type Asks interface { @@ -381,9 +378,8 @@ func (st *state) outcome(v delivery.Verdict) Outcome { // fail records a failure the asking met as a fact of the check, where it is met, and answers it as a verdict of failure // at a machine: the verdict is the judging rules' of the facts (rules.Judge), so a failure only returned is no failure // (milestone 47/B0). -func (st *state) fail(machine, words string) *delivery.Verdict { +func (st *state) fail(machine, words string) { st.facts = append(st.facts, rules.Step{Failed: &delivery.FailedFirst{Machine: machine, Words: words}}) - return fail(machine, words) } // factsOf are the check's facts as met: what the checking program judges. @@ -391,12 +387,6 @@ func (st *state) factsOf() rules.Facts { return rules.Facts{V: rules.FactsVersion, Steps: st.facts, CloneMS: st.clone.Milliseconds(), Composition: st.composition, Notes: st.c.Notes()} } -// fail is a verdict of failure at a machine. -func fail(machine, words string) *delivery.Verdict { - v := failure(machine, words) - return &v -} - // levels are the levels a case's asks run: both, or the one a try names. func (st *state) levelsOf() []string { if st.c.Try != nil && st.c.Try.Level != "" { @@ -409,7 +399,7 @@ func (st *state) levelsOf() []string { // judges each answer. A head that does not merge, a pin that is not allowed or a level that fails is a failure of that // member; every other member is still judged, and the verdict names every failure (the operator, 2026-11-02), unjoined // to its first lines (settled does that). Only a check whose context ended stops. -func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { +func (st *state) judgeAll(ctx context.Context) { // The mesh-sdk member first: whether its merge changes the TypeScript package's paths decides whether the other members' // Node fetches take the group's package (ADR 0500 §5). pulls := slices.Clone(st.c.Pulls) @@ -423,11 +413,11 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { return 0 }) var groupPackage map[string]any - var failures []*delivery.Verdict + met := len(st.facts) for _, p := range pulls { if ctx.Err() != nil { - if len(failures) == 0 { // an ask that ended with the context said it already - failures = append(failures, st.fail(delivery.CheckerModule, ctx.Err().Error())) + if !st.failedSince(met) { // an ask that ended with the context said it already + st.fail(delivery.CheckerModule, ctx.Err().Error()) } break } @@ -446,7 +436,7 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { if bm == buildrun.GoBuildModule { beside, err := st.j.besideFor(ctx, st.c, p) if err != nil { - failures = append(failures, st.fail(forgeMachine(err, bm), err.Error()+unheardWords(err))) + st.fail(forgeMachine(err, bm), err.Error()+unheardWords(err)) continue } if len(beside) > 0 { @@ -460,9 +450,7 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { } t.fetchArgs = args st.targets = append(st.targets, t) - if v := st.ask(ctx, t, &merged); v != nil { - failures = append(failures, v) - } + st.ask(ctx, t, &merged) } // 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). @@ -478,7 +466,16 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { groupPackage = map[string]any{"repository": p.Repository, "commit": p.Head, "ref": refOf(p.Number)} } } - return rules.JoinFailures(failures...) +} + +// failedSince says a failure was recorded among the facts from the nth on. +func (st *state) failedSince(n int) bool { + for _, f := range st.facts[n:] { + if f.Failed != nil { + return true + } + } + return false } // DefaultDeclineBound is an Examiner's DeclineBound when it names none: longer than a check runs, so a node with the @@ -489,14 +486,14 @@ const DefaultDeclineBound = 30 * time.Minute // each level's digest, kept verdicts and tests. When the answer leaves packages unmet, // their tests needing what that node lacks (issue 890), the passes are kept and the levels from the first such one are // asked again of a node that has it (placed). -func (st *state) ask(ctx context.Context, t *target, merged *[]Fetched) *delivery.Verdict { - return st.askNeeding(ctx, t, merged, st.levelsOf(), nil, nil) +func (st *state) ask(ctx context.Context, t *target, merged *[]Fetched) { + st.askNeeding(ctx, t, merged, st.levelsOf(), nil, nil) } // askNeeding is ask with the levels to run, what the node must have and the packages it tests: none for the first ask // of a target; for the placed one the unmet packages' needs and those packages alone, so no package the first ask judged // is run or counted again (ADR 0552 §4). -func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, levels, needs, packages []string) *delivery.Verdict { +func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, levels, needs, packages []string) { p := t.pull placed := len(needs) > 0 job := &delivery.CheckJob{Fetch: t.fetchArgs, Levels: levels, Kept: true, Packages: packages, Toolchain: t.agreed} @@ -506,7 +503,8 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l err = fmt.Errorf("no node record is on the bus") } if err != nil { - return st.fail(t.module, fmt.Sprintf("the check of %s#%d is not asked: the nodes' names the names check needs cannot be read: %v", p.Repository, p.Number, err)) + st.fail(t.module, fmt.Sprintf("the check of %s#%d is not asked: the nodes' names the names check needs cannot be read: %v", p.Repository, p.Number, err)) + return } job.NodeNames = names } @@ -535,7 +533,8 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l st.c.Progress.Set(what, "") st.c.Asked(a.ID) if err := st.j.Asks.Put(ctx, a); err != nil { - return st.fail(t.module, fmt.Sprintf("the check of %s#%d could not be asked: %v", p.Repository, p.Number, err)) + st.fail(t.module, fmt.Sprintf("the check of %s#%d could not be asked: %v", p.Repository, p.Number, err)) + return } waiting := ctx if watch := st.c.Watch; watch != nil { @@ -547,9 +546,11 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l step, err = st.j.Asks.Wait(waiting, a) if err != nil { if ctx.Err() != nil { - return st.fail(t.module, ctx.Err().Error()) + st.fail(t.module, ctx.Err().Error()) + return } - return st.fail(t.module, fmt.Sprintf("the check of %s#%d: %v", p.Repository, p.Number, err)) + st.fail(t.module, fmt.Sprintf("the check of %s#%d: %v", p.Repository, p.Number, err)) + return } d, declined := delivery.DeclinedOf(step) if !declined { @@ -570,13 +571,13 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l if lacks == "" { lacks = strings.Join(d.Lacks, ", ") // a node off the agreed toolchain declined it (ADR 0584) } - return st.fail(step.Node, fmt.Sprintf("the check of %s#%d needs a node with %s, and none took it in %s: every node that pulled it lacks it (last %s)", + st.fail(step.Node, fmt.Sprintf("the check of %s#%d needs a node with %s, and none took it in %s: every node that pulled it lacks it (last %s)", p.Repository, p.Number, lacks, bound, step.Node)) + return } st.j.logf("%s#%d: the ask %s was declined on %s, which lacks %s; asked again", p.Repository, p.Number, a.ID, step.Node, strings.Join(d.Lacks, ", ")) a.ID, a.At = buildstep.NewID(), now.UTC() } - var failures []*delivery.Verdict // The fetch and the digests are what of the ask's time its levels' running did not take (Took.Clone). took := time.Duration(0) if !step.Started.IsZero() && step.Ended.After(step.Started) { @@ -586,20 +587,24 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l st.facts = append(st.facts, rules.Step{Repository: p.Repository, Number: p.Number, Module: t.module, Node: step.Node}) st.c.Progress.Set(fmt.Sprintf("judge the check of %s#%d", p.Repository, p.Number), step.Node) if step.State != delivery.StepPassed { - return st.fail(step.Node, fmt.Sprintf("the check of %s#%d %s: %s", p.Repository, p.Number, step.State, step.Reason)) + st.fail(step.Node, fmt.Sprintf("the check of %s#%d %s: %s", p.Repository, p.Number, step.State, step.Reason)) + return } var answer delivery.CheckAnswer if err := json.Unmarshal(step.Result, &answer); err != nil { - return st.fail(step.Node, fmt.Sprintf("the check of %s#%d answered what is not a check: %s", p.Repository, p.Number, step.Result)) + st.fail(step.Node, fmt.Sprintf("the check of %s#%d answered what is not a check: %s", p.Repository, p.Number, step.Result)) + return } if err := json.Unmarshal(answer.Fetch, &t.fetched); err != nil || t.fetched.Workspace == "" { - return st.fail(step.Node, fmt.Sprintf("the fetch of %s#%d answered no workspace: %s", p.Repository, p.Number, answer.Fetch)) + st.fail(step.Node, fmt.Sprintf("the fetch of %s#%d answered no workspace: %s", p.Repository, p.Number, answer.Fetch)) + return } if len(t.fetched.Conflicts) > 0 { if !st.narrowTry() { st.j.writePreview(ctx, p, append(*merged, t.fetched)) } - return st.fail(step.Node, fmt.Sprintf("%s#%d %s%s", p.Repository, p.Number, ConflictWords, strings.Join(t.fetched.Conflicts, ", "))) + st.fail(step.Node, fmt.Sprintf("%s#%d %s%s", p.Repository, p.Number, ConflictWords, strings.Join(t.fetched.Conflicts, ", "))) + return } t.changed = t.fetched.Changed if !changesBuildGo(p.Repository, t.changed) { @@ -614,16 +619,13 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l if placed { if len(la.Unmet) > 0 { st.clone += max(0, took-time.Duration(ran*float64(time.Second))) - return st.fail(step.Node, fmt.Sprintf("the %s level of %s#%d left %s untested on %s, which was asked for a node with %s", + st.fail(step.Node, fmt.Sprintf("the %s level of %s#%d left %s untested on %s, which was asked for a node with %s", la.Level, p.Repository, p.Number, unmetNames(la.Unmet), step.Node, strings.Join(needs, ", "))) + return } la.Kept = nil // it names its packages and takes no other's kept verdict; this is only a guard against an older module } - v, elapsed := st.level(ctx, t, la) - ran += elapsed - if v != nil { - failures = append(failures, v) // the next level was run too (buildstep): its failures are named as well - } + ran += st.level(ctx, t, la) // the next level was run too (buildstep): its failures are named as well } st.clone += max(0, took-time.Duration(ran*float64(time.Second))) if unmet := delivery.UnmetNeeds(answer.Levels); len(unmet) > 0 && !placed { @@ -643,9 +645,8 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l } } st.j.logf("%s#%d: %s on %s left untested; asked of a node with %s", p.Repository, p.Number, unmetOf(answer.Levels), step.Node, strings.Join(unmet, ", ")) - failures = append(failures, st.askNeeding(ctx, t, merged, again, unmet, packages)) + st.askNeeding(ctx, t, merged, again, unmet, packages) } - return rules.JoinFailures(failures...) } // unmetNames names the unmet packages and their needs. @@ -672,14 +673,14 @@ func unmetOf(levels []delivery.LevelAnswer) string { // of the packages that passed, the kept ones taken are counted, and every test result is read and counted, also after // one that did not pass, so the verdict names every failed test of the level (issue 1921). It answers the level's // running time (its tests' and setup's). -func (st *state) level(ctx context.Context, t *target, la delivery.LevelAnswer) (*delivery.Verdict, float64) { +func (st *state) level(ctx context.Context, t *target, la delivery.LevelAnswer) float64 { if t.runner != "" && t.module == buildrun.GoBuildModule && la.Digest != nil { t.judgedBy(la.Digest.JudgedBy) // the installed module judged it: the runner is not used again } if t.runner != "" && la.Judged != "" { t.judgedBy(la.Judged) // the candidate judged the level: nothing is kept (ADR 0535 §4) } - v, elapsed, results := st.levels.Level(t.pull.Repository, t.pull.Number, t.module, t.node, la) + _, elapsed, results := st.levels.Level(t.pull.Repository, t.pull.Number, t.module, t.node, la) st.facts = append(st.facts, rules.Step{Repository: t.pull.Repository, Number: t.pull.Number, Module: t.module, Node: t.node, Level: &la}) var digests map[string]string if t.module == buildrun.GoBuildModule && la.Digest != nil { @@ -696,7 +697,7 @@ func (st *state) level(ctx context.Context, t *target, la delivery.LevelAnswer) } } } - return v, elapsed + return elapsed } // changesPackage says whether the merge of a pull request changes a file inside the named paths of one of its repository's