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.
2026-10-28 21:50:03 +01:00
parent 1f6f4eda32
commit 2d8ad2e66a
5 changed files with 122 additions and 44 deletions
@@ -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())
 			}
 		})
 	}()
@@ -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()
@@ -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)
+				}
+			}
+		})
 	}
 }

@@ -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) {
@@ -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()