From 2d8ad2e66a8f652dc9f90cd82a071cae8772767a Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 28 Oct 2026 21:04:55 +0100 Subject: [PATCH] Checker: no candidate is left without a verdict (issue 1733) Once the gate's one-hour ask bound goes (mesh-catalog fix/1733-no-hour), the gate asks the operator's Approve only on a verdict, so every path that left a candidate without one now writes one, from an event and with no timer: - a group with a member whose repository's trunk head is not known (a pull request the checker cannot claim) is not judged in part: its verdict fails naming that repository, and every status of the group says so (a pass of the others would have let it merge with that member never checked); - the verdict of heads that cannot be judged replaces whatever the key held, such as a running verdict left by a check that can no longer run; - a verdict key deleted is judged again on its delete (the checker watches its verdicts), and a check running when its key is deleted writes it anew. Tests: three rows at the end of the no-trunk test; counts unchanged. --- .../cmd/mesh-delivery-checker/main.go | 7 +- .../internal/check/checker.go | 99 +++++++++++++------ .../internal/check/checker_test.go | 57 +++++++++-- .../internal/check/read.go | 2 +- .../internal/check/read_test.go | 1 + 5 files changed, 122 insertions(+), 44 deletions(-) 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 a485138..5f3018c 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go @@ -338,15 +338,18 @@ func run(ctx context.Context, getenv func(string) string, logOut io.Writer) erro } }) }() - // The checker's own verdicts, updates only: one checker_ask_again wrote asks its check again (Checker.VerdictWritten). + // The checker's own verdicts, updates only, in one watch: one checker_ask_again wrote asks its check again + // (Checker.VerdictWritten); a deleted key of heads a candidate holds is judged again (Checker.VerdictDeleted, issue 1733). vw, err := buckets[delivery.CheckerVerdictsBucket].WatchAll(ctx, jetstream.UpdatesOnly()) if err != nil { - return fmt.Errorf("the checker's verdicts cannot be watched, so no check can be asked again by hand: %w", err) + return fmt.Errorf("the checker's verdicts cannot be watched, so no check can be asked again by hand and a deleted verdict would never be written again: %w", err) } go func() { errs <- follow(ctx, vw, func(e jetstream.KeyValueEntry) { if opOf(e) { ch.VerdictWritten(ctx, e.Key(), e.Value()) + } else { + ch.VerdictDeleted(ctx, e.Key()) } }) }() diff --git a/modules/mesh-delivery-checker/internal/check/checker.go b/modules/mesh-delivery-checker/internal/check/checker.go index da952d3..2456ab9 100644 --- a/modules/mesh-delivery-checker/internal/check/checker.go +++ b/modules/mesh-delivery-checker/internal/check/checker.go @@ -250,6 +250,35 @@ func (ch *Checker) Candidate(ctx context.Context, key string, value []byte) { ch.consider(ctx, key, c) } +// VerdictDeleted takes the delete of a verdict key in (the checker's watch of its verdicts, issue 1733): the gate asks the +// operator's Approve only on a verdict, so heads a candidate holds are never left without one. A check of them running in +// this process writes its verdict anew at its end; otherwise the candidate is considered at once and judged again (or said +// again to be unjudgeable). A key no candidate holds is said and left. +func (ch *Checker) VerdictDeleted(ctx context.Context, key string) { + ch.init() + ch.mu.Lock() + if ch.inflight[key] { + ch.mu.Unlock() + ch.logf("%s: deleted while its check runs; its end writes it anew", key) + return + } + var group string + var cand delivery.Candidate + for k, c := range ch.known { + if vk, err := delivery.CheckerVerdictKey(c.Group, c.HeadsHash); err == nil && vk == key { + group, cand = k, c + break + } + } + ch.mu.Unlock() + if group == "" { + ch.logf("%s: deleted, and no candidate holds these heads, so nothing is judged", key) + return + } + ch.logf("%s: deleted; its heads are judged again", key) + ch.consider(ctx, group, cand) +} + // Forget takes a candidate out (its key was deleted): a check of its heads still running is ended, as no candidate holds // them any more. func (ch *Checker) Forget(key string) { @@ -298,12 +327,16 @@ func (ch *Checker) heads() map[string]string { func (ch *Checker) consider(ctx context.Context, key string, c delivery.Candidate) { heads := ch.heads() - cs := CaseOf(c, heads) if left := leftOf(c, heads); len(left) > 0 { - ch.untrunked(ctx, c, left, cs == nil) + // A pull request the checker cannot claim (its repository's trunk head is not known) leaves its whole group + // unjudged, said in a failure verdict (issue 1422, issue 1733): the gate asks the Approve only on a verdict, and a + // pass of the other members would let the group merge with that one never checked. + ch.untrunked(ctx, c, left) + return } + cs := CaseOf(c, heads) if cs == nil { - return // nothing to judge: no trunk known, which untrunked said + return // no pull request at all: nothing to judge } cs.Queued = ch.now() ch.mu.Lock() @@ -362,14 +395,16 @@ func (ch *Checker) consider(ctx context.Context, key string, c delivery.Candidat } // untrunked says, loudly, that pull requests of a candidate cannot be judged because the gate's source.main names no trunk -// head of their repository (issue 1422): each gets the status error with UntrunkedWords, and when none of the candidate is -// judged (whole) its verdict key holds a verdict that says so, which checker_verdicts and checker_status show. It is logged -// once per head: when a status or the verdict is written, never at a look that finds them already said. A verdict of these -// heads that says anything else is left. Once the trunk head is known the verdict is stale (it names no trunk) and the heads -// are judged whole (prepare, claim). -func (ch *Checker) untrunked(ctx context.Context, c delivery.Candidate, left []Pull, whole bool) { +// head of their repository (issue 1422): its verdict key holds a failure that says so, whatever it held before (a running +// verdict left by a check that can no longer run, or a verdict judged on a trunk that is no longer named: issue 1733), which +// checker_verdicts and checker_status show; and every pull request of the candidate gets the status error with +// UntrunkedWords, the members whose trunk is known too, as their group is not judged. It is logged once per head: when a +// status or the verdict is written, never at a look that finds them already said. Once the trunk head is known the verdict +// is stale (it names no trunk) and the heads are judged whole (prepare, claim). +func (ch *Checker) untrunked(ctx context.Context, c delivery.Candidate, left []Pull) { key, err := delivery.CheckerVerdictKey(c.Group, c.HeadsHash) if err != nil { + ch.logf("candidates of %s: no verdict key for heads %q, so it cannot be said that they cannot be judged: %v", c.Group, c.HeadsHash, err) return } repos := make([]string, 0, len(left)) @@ -377,27 +412,22 @@ func (ch *Checker) untrunked(ctx context.Context, c delivery.Candidate, left []P repos = append(repos, p.Repository) } words := UntrunkedWordsOf(repos) - said := false - if whole { - v := delivery.Verdict{Group: c.Group, Heads: GateHeads(c), Trunk: map[string]string{}, State: delivery.VerdictFail, - Failed: delivery.FailedFirst{Machine: "checker", Words: words}} - data, err := delivery.EncodeVerdict(v) - if err != nil { - ch.logf("%s: %s, and the verdict cannot say so: %v", key, words, err) - return - } - wrote, err := putOnce(ctx, ch.Verdicts, key, data, func(cur []byte) bool { - held, err := delivery.DecodeVerdict(cur) - // The same words stand; a verdict that says anything else (a check that ran, one running) is not this one's to replace. - return err != nil || !IsUntrunked(held) || held.Failed.Words == words - }) - if err != nil { - ch.logf("%s: %s, and the verdict cannot say so: %v", key, words, err) - } - said = said || wrote + v := delivery.Verdict{Group: c.Group, Heads: GateHeads(c), Trunk: map[string]string{}, State: delivery.VerdictFail, + Failed: delivery.FailedFirst{Machine: "checker", Words: words}} + data, err := delivery.EncodeVerdict(v) + if err != nil { + ch.logf("%s: %s, and the verdict cannot say so: %v", key, words, err) + return } - for _, p := range left { - okey, err := delivery.CheckerOwedKey(p.Repository, p.Number, p.Head) + said, err := putOnce(ctx, ch.Verdicts, key, data, func(cur []byte) bool { + held, err := delivery.DecodeVerdict(cur) + return err == nil && IsUntrunked(held) && held.Failed.Words == words // the same words stand; anything else is replaced + }) + if err != nil { + ch.logf("%s: %s, and the verdict cannot say so: %v", key, words, err) + } + for _, cp := range c.Pulls { + okey, err := delivery.CheckerOwedKey(cp.Repository, cp.Number, cp.Head.Head) if err != nil { ch.logf("%s: %v", key, err) continue @@ -430,7 +460,7 @@ func (ch *Checker) untrunked(ctx context.Context, c delivery.Candidate, left []P for _, p := range left { names = append(names, fmt.Sprintf("%s#%d@%s", p.Repository, p.Number, short12(p.Head))) } - ch.logf("%s: %s %s; the status says so (error), and they are judged once the trunk head is known", key, strings.Join(names, ", "), words) + ch.logf("%s: %s %s; the verdict and every status of its group say so (error), and it is judged once the trunk head is known", key, strings.Join(names, ", "), words) } } @@ -782,8 +812,13 @@ func (ch *Checker) try(ctx, run context.Context, key string, cs *Case) { } } if _, err := ch.Verdicts.Update(ctx, cs.Key, data, cl.rev); err != nil { - ch.logf("%s: the verdict could not be written: %v", cs.Key, err) - return + if _, _, found, gerr := ch.Verdicts.Get(ctx, cs.Key); gerr == nil && !found { + _, err = ch.Verdicts.Create(ctx, cs.Key, data) // its key was deleted while it ran (issue 1733): written anew + } + if err != nil { + ch.logf("%s: the verdict could not be written: %v", cs.Key, err) + return + } } if IsNodesFailure(v) { ch.mu.Lock() diff --git a/modules/mesh-delivery-checker/internal/check/checker_test.go b/modules/mesh-delivery-checker/internal/check/checker_test.go index dd89894..888cf41 100644 --- a/modules/mesh-delivery-checker/internal/check/checker_test.go +++ b/modules/mesh-delivery-checker/internal/check/checker_test.go @@ -293,15 +293,54 @@ func TestARepositoryWithNoTrunkCannotBeJudgedLoudlyUntilItsTrunkIsKnown(t *testi t.Errorf("%s#4 owes %q once its trunk is known, want success", unknown, s) } - // A group with one member of a repository whose trunk is not known: the others are judged, its status says why it is not. - group := candidateOf(t, "feat/z", pullOf(repoA, 9, head2), pullOf("novox/other", 2, head2)) - r.ch.Candidate(ctx, "feat/z", group) - r.ch.Wait() - if s := owedState(t, r, repoA, 9, head2); s != "success" { - t.Errorf("%s#9 owes %q, want success", repoA, s) - } - if s := owedState(t, r, "novox/other", 2, head2); s != "error" { - t.Errorf("novox/other#2 owes %q, want error", s) + // Issue 1733: the gate asks the Approve only on a verdict, so no candidate is left without one. A group with a member + // whose trunk is not known is not judged in part: its verdict fails naming that repository, and every status says so; a + // running verdict left for heads that cannot be judged is replaced; a verdict key deleted is judged again on its delete. + for _, row := range []struct { + name string + pulls []delivery.CandidatePull + running bool // a running verdict is left under the key first + deleted bool // the key is deleted after its verdict + judged int + want string // the verdict's state, and for a failure the repository it names + }{ + {name: "a member the checker cannot claim", pulls: []delivery.CandidatePull{pullOf(repoA, 9, head2), pullOf("novox/other", 2, head2)}, want: "novox/other"}, + {name: "a running verdict left for heads that cannot be judged", pulls: []delivery.CandidatePull{pullOf(unknown, 5, head2)}, running: true, want: unknown}, + {name: "a verdict key deleted", pulls: []delivery.CandidatePull{pullOf(repoA, 5, head2)}, deleted: true, judged: 2, want: delivery.VerdictPass}, + } { + t.Run(row.name, func(t *testing.T) { + j := passJudge() + r := newRig(t, j) + cand := candidateOf(t, "feat/z", row.pulls...) + key := keyOf(t, "feat/z", cand) + if row.running { + group, heads, _ := delivery.ParseCheckerVerdictKey(key) + raw, _ := delivery.EncodeVerdict(delivery.Verdict{Group: group, Heads: heads, State: delivery.VerdictRunning}) + _, _ = r.verdicts.Create(ctx, key, raw) + } + r.ch.Candidate(ctx, "feat%2Fz", cand) + r.ch.Wait() + if row.deleted { + _ = r.verdicts.Delete(ctx, key) + r.ch.VerdictDeleted(ctx, key) // the watch of the verdicts hands the delete over + r.ch.Wait() + } + v := r.verdict(t, key) + if row.want == delivery.VerdictPass { + if v.State != delivery.VerdictPass || j.count() != row.judged { + t.Errorf("verdict %s after %d judgings, want a pass after %d", v.State, j.count(), row.judged) + } + return + } + if !IsUntrunked(v) || !strings.Contains(v.Failed.Words, row.want) || j.count() != 0 { + t.Errorf("verdict %+v after %d judgings, want a failure naming %s and none judged", v, j.count(), row.want) + } + for _, p := range row.pulls { + if s := owedState(t, r, p.Repository, p.Number, p.Head.Head); s != "error" { + t.Errorf("%s#%d owes %q, want error", p.Repository, p.Number, s) + } + } + }) } } diff --git a/modules/mesh-delivery-checker/internal/check/read.go b/modules/mesh-delivery-checker/internal/check/read.go index 250b6b9..4d22745 100644 --- a/modules/mesh-delivery-checker/internal/check/read.go +++ b/modules/mesh-delivery-checker/internal/check/read.go @@ -466,7 +466,7 @@ func (r Reader) Status() (Status, error) { continue } c, raw := row.c, row.raw - if heads != nil && CaseOf(c, heads) == nil { + if heads != nil && len(leftOf(c, heads)) > 0 { // Nothing of it can be judged now, whatever its verdict says (issue 1422): never counted as waiting. var repos []string for _, p := range leftOf(c, heads) { diff --git a/modules/mesh-delivery-checker/internal/check/read_test.go b/modules/mesh-delivery-checker/internal/check/read_test.go index 6f1b3f9..91d1348 100644 --- a/modules/mesh-delivery-checker/internal/check/read_test.go +++ b/modules/mesh-delivery-checker/internal/check/read_test.go @@ -36,6 +36,7 @@ func (f fakeState) read() StateRead { func readerOf(t *testing.T) (Reader, string) { t.Helper() r := newRig(t, passJudge()) + r.setTrunks(t, map[string]string{repoA: trunkA, repoSDK: trunkS, hqRepo: trunkA2}) cand := candidateOf(t, "feat/x", pullOf(repoA, 7, head1), pullOf(hqRepo, 3, head2)) r.ch.Candidate(context.Background(), "feat%2Fx", cand) r.ch.Wait()