Skip to content

Commit 853ebe0

Browse files
authored
fix(queue): stop the verdict backoff from eating a manual retrigger or an operator force (#10229)
#10204 placed the skip at the entry of the publish-and-maintain pass, past two things it must not run past. Readiness fires options.onReachedReadiness -- which charges regatePullRequest's bounded repair budget -- and then consumes the ONE-SHOT panel-retrigger marker (#7626). A guard sitting after both meant a backed-off pass had already spent a user's "Re-run LoopOver review" click with nothing left to re-trigger it, and had charged a repair attempt for work it never did. The guard now sits between the readiness gate and onReachedReadiness. It must not move EARLIER than readiness either: readiness legitimately defers a pass, and the screenshot-table recapture chain (#10061) depends on those deferrals to bound its retry budget -- a pre-readiness guard truncated it from 5 attempts to 3, caught by that test. options.force is now honoured. An operator's manual re-gate passes force: true and was being silently suppressed; a poll tick (previewPollAttempt) likewise. Backoff exists to stop the machine re-asking itself a settled question and must never suppress a pass a human asked for. The guard is extracted so both publish-and-maintain sites can share it, and its !headSha half is documented as a TSC-enforced early-out rather than a safety guard -- mutation testing confirms no runtime test can distinguish its absence, and an unverifiable guard is a claim, not a safeguard (verdict-stability.ts's own removed exponent clamp made the same point). #10204 shipped this wiring with NO test; test/unit/verdict-stability-wire.test.ts is the first, and pins each defect above as a regression. The webhook path is deliberately still unguarded. Adding it there truncates #10061's recapture budget, and the fix is not another exemption but moving the skip to the verdict-derivation choke point the record half already uses -- scoped in #10227. Closes #10222
1 parent ff28627 commit 853ebe0

2 files changed

Lines changed: 220 additions & 26 deletions

File tree

src/queue/processors.ts

Lines changed: 66 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -4369,6 +4369,27 @@ export async function reReviewStoredPullRequest(
43694369
))
43704370
)
43714371
return false;
4372+
// #10222: back off AFTER the readiness gate, but BEFORE onReachedReadiness and the retrigger consumption
4373+
// just below. #10204 placed it after the actuation-lock claim, past both -- so a backed-off pass had already
4374+
// consumed the user's one-shot "Re-run LoopOver review" marker and charged regatePullRequest's repair budget
4375+
// for work it never did. It must not move EARLIER than readiness either: readiness legitimately defers a
4376+
// pass (rebase fired, CI still running), and the screenshot-table gate's bounded recapture chain (#10061)
4377+
// depends on those deferrals continuing to happen, so a pre-readiness guard silently truncates that retry
4378+
// budget. Between the two is the only correct place.
4379+
//
4380+
// `force` is an operator's manual re-gate and `previewPollAttempt` is a visual poll's own next tick -- both
4381+
// are explicit requests for THIS pass, and backoff must never suppress a pass a human or a bounded retry
4382+
// chain asked for.
4383+
if (
4384+
await stableVerdictBackoffEngaged(env, {
4385+
repoFullName,
4386+
prNumber,
4387+
headSha: pr.headSha,
4388+
deliveryId,
4389+
explicitlyRequested: options.force === true || previewPollAttempt !== undefined,
4390+
})
4391+
)
4392+
return false;
43724393
// Fire BEFORE any further (throwable) work below -- this is the one instant readiness is confirmed, so a
43734394
// caller learns it even if this call goes on to THROW instead of returning (see the JSDoc above).
43744395
options.onReachedReadiness?.();
@@ -4448,32 +4469,6 @@ export async function reReviewStoredPullRequest(
44484469
}).catch(() => undefined);
44494470
throw new PrActuationLockContendedError(repoFullName, pr.number, "public-surface-publish");
44504471
}
4451-
// #10184: a PR whose answer has not changed does not need asking again yet. metagraphed#8886 produced 56
4452-
// identical `hold | missing_linked_issue` verdicts on ONE head SHA in 47 minutes; four such PRs made 66% of
4453-
// all decision records in a two-hour window, and that window exhausted the installation's REST quota.
4454-
//
4455-
// Placed AFTER the lock claim so the check itself is nearly free (one cache read on a pass that already
4456-
// owns the PR) and BEFORE the refresh, so a backed-off pass spends nothing. Returns rather than throws:
4457-
// this is not contention, there is no work to retry, and the state is already published and correct.
4458-
//
4459-
// Fails OPEN in every uncertain case -- no state, unreadable state, no cache -- see verdict-stability.ts.
4460-
// The delay is capped, so a stuck PR is still revisited; it just stops being asked 1.2x/minute.
4461-
if (pr.headSha) {
4462-
const stability = await readVerdictStability(env.SELFHOST_TRANSIENT_CACHE, verdictStabilityKey(repoFullName, pr.number, pr.headSha));
4463-
if (shouldSkipStableVerdict(stability, Date.now())) {
4464-
await recordAuditEvent(env, {
4465-
eventType: "github_app.review_skipped_stable_verdict",
4466-
actor: "loopover",
4467-
targetKey: `${repoFullName}#${pr.number}`,
4468-
outcome: "completed",
4469-
detail: `Verdict unchanged across ${stability?.repeats ?? 0} consecutive evaluations of this commit; backing off instead of re-deriving the same answer.`,
4470-
metadata: { deliveryId, repoFullName, repeats: stability?.repeats ?? 0 },
4471-
}).catch(() => undefined);
4472-
await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken).catch(() => undefined);
4473-
// false = "did not re-review", the same signal this function's other early bail uses.
4474-
return false;
4475-
}
4476-
}
44774472
// #2537 follow-up (gate-flagged): the durable review cache's only invalidation path is markPullRequestReviewsInvalidated
44784473
// on a webhook (processors.ts). A "quiet" PR (no new pushes, slop evidence + manifest gate both off, no
44794474
// pre-merge check paths) never hits any of the three reasons below, so a DROPPED invalidation write could sit
@@ -5142,6 +5137,51 @@ async function consumePendingPrPanelRetrigger(
51425137
* varies by call (#selfhost-ci-deferral-staleness). A missing cache / cache hiccup degrades to `false` (never
51435138
* force-finalize → keeps the safe old defer rather than acting early).
51445139
*/
5140+
/**
5141+
* #10222: should this publish-and-maintain pass back off, because this PR's verdict has not changed (#10184)?
5142+
*
5143+
* Shared by BOTH sites that run the unit -- `reReviewStoredPullRequest` (sweep / CI completion) and
5144+
* `handlePullRequestWebhookEvent` (the `pull_request` webhook). #10204 guarded only the first, which left the
5145+
* DOMINANT source unthrottled: over 24h on the Orb, 293 of 344 repeat evaluations carried
5146+
* `upstream_state_change` -- `deriveReevaluationReason`'s mapping for a RAW GitHub delivery, i.e. the webhook
5147+
* path. A label write the engine itself caused arrives there, not on the sweep.
5148+
*
5149+
* Call this BEFORE the readiness gate at either site. Readiness fires `onReachedReadiness` (which charges
5150+
* regatePullRequest's repair budget) and consumes the one-shot panel-retrigger marker, and a pass that backs
5151+
* off after those has silently eaten a user's "Re-run LoopOver review" click with nothing left to re-trigger
5152+
* it. Before the lock claim, too: there is no lock to take or release on a pass that is not going to run.
5153+
*
5154+
* `explicitlyRequested` is the escape hatch and the reason this takes a flag at all -- backoff exists to stop
5155+
* the machine asking itself the same question, and must NEVER suppress a pass a human asked for.
5156+
*
5157+
* Fails OPEN everywhere: no head SHA, no state, unreadable state, no cache, or a throwing read all return
5158+
* false and evaluate normally (see verdict-stability.ts). The delay is capped, so even a stuck PR is still
5159+
* revisited -- it just stops being asked 1.2x/minute.
5160+
*/
5161+
async function stableVerdictBackoffEngaged(
5162+
env: Env,
5163+
args: { repoFullName: string; prNumber: number; headSha: string | null | undefined; deliveryId: string; explicitlyRequested: boolean },
5164+
): Promise<boolean> {
5165+
const { repoFullName, prNumber, headSha, deliveryId, explicitlyRequested } = args;
5166+
// The `!headSha` half is enforced by TSC, not by a test: verdictStabilityKey takes a `string`, so removing
5167+
// it does not compile. Mutation testing confirms no RUNTIME test can distinguish its absence -- with no head
5168+
// SHA the lookup would miss and shouldSkipStableVerdict would return false anyway -- so it is an early-out
5169+
// that saves a pointless cache round-trip, not a safety guard. Recorded here so nobody later mistakes it for
5170+
// one (same reasoning as verdict-stability.ts's removed exponent clamp).
5171+
if (explicitlyRequested || !headSha) return false;
5172+
const stability = await readVerdictStability(env.SELFHOST_TRANSIENT_CACHE, verdictStabilityKey(repoFullName, prNumber, headSha)).catch(() => null);
5173+
if (!shouldSkipStableVerdict(stability, Date.now())) return false;
5174+
await recordAuditEvent(env, {
5175+
eventType: "github_app.review_skipped_stable_verdict",
5176+
actor: "loopover",
5177+
targetKey: `${repoFullName}#${prNumber}`,
5178+
outcome: "completed",
5179+
detail: `Verdict unchanged across ${stability?.repeats ?? 0} consecutive evaluations of this commit; backing off instead of re-deriving the same answer.`,
5180+
metadata: { deliveryId, repoFullName, repeats: stability?.repeats ?? 0 },
5181+
}).catch(() => undefined);
5182+
return true;
5183+
}
5184+
51455185
async function ciPendingDeferStuck(
51465186
env: Env,
51475187
repoFullName: string,
Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
import { afterEach, describe, expect, it, vi } from "vitest";
2+
import { listAuditEventsByType, upsertInstallation, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories";
3+
import { reReviewStoredPullRequest } from "../../src/queue/processors";
4+
import { verdictStabilityKey, writeVerdictStability } from "../../src/review/verdict-stability";
5+
import { normalizeRegistryPayload } from "../../src/registry/normalize";
6+
import { persistRegistrySnapshot } from "../../src/registry/sync";
7+
import { asCloudEnv, createTestEnv } from "../helpers/d1";
8+
import { generatePrivateKeyPem } from "../helpers/github-app-key";
9+
10+
// #10222: the wiring #10204 shipped without a test. The backoff logic itself is covered by
11+
// verdict-stability.test.ts; what was never covered is WHERE the guard sits, and #10204 put it after the
12+
// readiness gate -- which fires onReachedReadiness and consumes the one-shot panel-retrigger marker. These
13+
// tests pin the three properties that placement got wrong.
14+
15+
const REPO = "JSONbored/gittensory";
16+
const HEAD = "sha-settled";
17+
18+
async function seed(env: ReturnType<typeof createTestEnv>) {
19+
await persistRegistrySnapshot(
20+
asCloudEnv(env),
21+
normalizeRegistryPayload({ [REPO]: { emission_share: 0.01, issue_discovery_share: 0 } }, { kind: "raw-github", url: "https://example.test" }, "2026-05-23T00:00:00.000Z"),
22+
);
23+
await upsertInstallation(env, {
24+
action: "created",
25+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "selected", permissions: {}, events: [] },
26+
});
27+
await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: REPO, private: false, owner: { login: "JSONbored" } }, 123);
28+
await upsertRepositorySettings(env, { repoFullName: REPO, autoLabelEnabled: false, autonomy: { label: "auto" } });
29+
await upsertPullRequestFromGitHub(env, REPO, {
30+
number: 77, title: "settled", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR",
31+
head: { sha: HEAD }, base: { ref: "main" }, labels: [], body: "Closes #1", created_at: "2026-07-31T09:00:00Z",
32+
} as never);
33+
}
34+
35+
/** A verdict that has repeated enough to be settled, evaluated a moment ago -- so the backoff is engaged. */
36+
async function seedSettledVerdict(env: ReturnType<typeof createTestEnv>) {
37+
await writeVerdictStability(env.SELFHOST_TRANSIENT_CACHE, verdictStabilityKey(REPO, 77, HEAD), {
38+
fingerprint: "hold|missing_linked_issue|",
39+
repeats: 8,
40+
lastEvaluatedMs: Date.now(),
41+
});
42+
}
43+
44+
function stubGitHub(onReadiness: () => void) {
45+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
46+
const url = input.toString();
47+
if (url.includes("/access_tokens")) return Response.json({ token: "t" });
48+
// A files read means the pass got PAST the guard into the publish unit. Readiness itself deliberately
49+
// runs before the guard (#10061), so it is not the probe.
50+
if (url.includes("/files")) onReadiness();
51+
if (url.endsWith("/pulls/77")) return Response.json({ number: 77, title: "settled", state: "open", user: { login: "contributor" }, head: { sha: HEAD }, labels: [], body: "Closes #1", mergeable_state: "clean" });
52+
if (url.includes("/check-runs")) return Response.json({ total_count: 1, check_runs: [{ name: "t", status: "completed", conclusion: "success", app: { slug: "github-actions" } }] });
53+
if (url.includes("/status")) return Response.json({ state: "success", statuses: [] });
54+
if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } });
55+
return Response.json({});
56+
});
57+
}
58+
59+
async function skippedEvents(env: ReturnType<typeof createTestEnv>) {
60+
return listAuditEventsByType(env, "github_app.review_skipped_stable_verdict", "2000-01-01T00:00:00Z");
61+
}
62+
63+
/** The same key processors.ts's pendingPrPanelRetriggerKey builds -- written directly because the marker
64+
* writer is module-private to processors.ts. */
65+
const RETRIGGER_KEY = `pr-panel-retrigger-pending:${REPO.toLowerCase()}#77:${HEAD}`;
66+
67+
describe("verdict-stability backoff wiring (#10222)", () => {
68+
afterEach(() => {
69+
vi.unstubAllGlobals();
70+
});
71+
72+
it("backs off a settled verdict, and records why", async () => {
73+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
74+
await seed(env);
75+
await seedSettledVerdict(env);
76+
let reviewRan = false;
77+
stubGitHub(() => { reviewRan = true; });
78+
79+
expect(await reReviewStoredPullRequest(env, "d1", 123, REPO, 77)).toBe(false);
80+
expect(reviewRan, "a backed-off pass must not reach the publish unit").toBe(false);
81+
expect(await skippedEvents(env)).toHaveLength(1);
82+
});
83+
84+
it("REGRESSION: an explicit force is NEVER backed off", async () => {
85+
// #10204's guard ignored options.force, so an operator's manual re-gate could be silently suppressed.
86+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
87+
await seed(env);
88+
await seedSettledVerdict(env);
89+
let reviewRan = false;
90+
stubGitHub(() => { reviewRan = true; });
91+
92+
await reReviewStoredPullRequest(env, "d2", 123, REPO, 77, undefined, { force: true });
93+
expect(reviewRan, "a forced pass must proceed into the publish unit").toBe(true);
94+
expect(await skippedEvents(env)).toHaveLength(0);
95+
});
96+
97+
it("REGRESSION: a visual-preview poll tick is NEVER backed off", async () => {
98+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
99+
await seed(env);
100+
await seedSettledVerdict(env);
101+
let reviewRan = false;
102+
stubGitHub(() => { reviewRan = true; });
103+
104+
await reReviewStoredPullRequest(env, "d3", 123, REPO, 77, 2);
105+
expect(reviewRan).toBe(true);
106+
expect(await skippedEvents(env)).toHaveLength(0);
107+
});
108+
109+
it("REGRESSION: a backed-off pass does not eat the one-shot panel-retrigger marker (#7626)", async () => {
110+
// The failure #10204's placement caused: readiness consumed the marker, THEN the guard returned, so the
111+
// user's "Re-run LoopOver review" click vanished with nothing left to re-trigger it.
112+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
113+
await seed(env);
114+
await seedSettledVerdict(env);
115+
await env.SELFHOST_TRANSIENT_CACHE?.set(RETRIGGER_KEY, "1", 3600);
116+
stubGitHub(() => undefined);
117+
118+
expect(await reReviewStoredPullRequest(env, "d4", 123, REPO, 77)).toBe(false);
119+
120+
// The marker must still be there for a later pass to consume.
121+
const stillPending = await env.SELFHOST_TRANSIENT_CACHE?.get(RETRIGGER_KEY);
122+
expect(stillPending, "the retrigger marker must survive a backed-off pass").toBeTruthy();
123+
});
124+
125+
it("never backs off a PR with no head SHA -- there is no key to have settled under", async () => {
126+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
127+
await seed(env);
128+
await seedSettledVerdict(env);
129+
await upsertPullRequestFromGitHub(env, REPO, {
130+
number: 78, title: "no head", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR",
131+
base: { ref: "main" }, labels: [], body: "Closes #1", created_at: "2026-07-31T09:00:00Z",
132+
} as never);
133+
stubGitHub(() => undefined);
134+
135+
await reReviewStoredPullRequest(env, "d6", 123, REPO, 78);
136+
expect(await skippedEvents(env)).toHaveLength(0);
137+
});
138+
139+
it("does not back off a verdict that has not settled yet", async () => {
140+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
141+
await seed(env);
142+
await writeVerdictStability(env.SELFHOST_TRANSIENT_CACHE, verdictStabilityKey(REPO, 77, HEAD), {
143+
fingerprint: "hold|missing_linked_issue|",
144+
repeats: 1,
145+
lastEvaluatedMs: Date.now(),
146+
});
147+
let reviewRan = false;
148+
stubGitHub(() => { reviewRan = true; });
149+
150+
await reReviewStoredPullRequest(env, "d5", 123, REPO, 77);
151+
expect(reviewRan).toBe(true);
152+
expect(await skippedEvents(env)).toHaveLength(0);
153+
});
154+
});

0 commit comments

Comments
 (0)