diff --git a/src/queue/processors.ts b/src/queue/processors.ts index cd4c56af5..701ddfa91 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -97,6 +97,7 @@ import { upsertRepositoryFromGitHub, getLatestAdvisoryForPullRequest, } from "../db/repositories"; +import { readVerdictStability, recordVerdict, shouldSkipStableVerdict, verdictStabilityKey, writeVerdictStability } from "../review/verdict-stability"; import { withLinkedIssueMaintainerExemption, type LinkedIssueExemptionAuthor } from "../settings/linked-issue-exemption"; import { resolveConfiguredRepoCandidates } from "../review/configured-repo-set"; import { renameRepositoryIdentity } from "../db/repo-identity-rename"; @@ -3968,6 +3969,19 @@ async function runAgentMaintenancePlanAndExecute( { reason: deriveReevaluationReason(deliveryId) }, holdCause, ); + // #10184: fold this verdict into the PR's stability state, at the one place every verdict passes through + // so no caller can bypass it -- the same reasoning persistDecisionRecord itself uses for its + // reevaluation check. Keyed on the head SHA, so a new commit starts clean without an explicit reset. + // Best effort: a failed write means the next pass sees no prior state and evaluates normally. + if (record.headSha) { + const stabilityKey = verdictStabilityKey(repoFullName, pr.number, record.headSha); + const priorStability = await readVerdictStability(env.SELFHOST_TRANSIENT_CACHE, stabilityKey); + await writeVerdictStability( + env.SELFHOST_TRANSIENT_CACHE, + stabilityKey, + recordVerdict(priorStability, { action: record.action, reasonCode: record.reasonCode, holdCause }, Date.now()), + ); + } // #8838: persist the evaluation's own exact inputs beside the record (PRIVATE sibling, migration 0182) // so the replay harness can re-derive this decision bit-exactly. Best-effort, like the record itself; // the no-replay no-op (synthetic content-lane/bridge evaluations) lives inside the helper. Keyed to the @@ -4428,6 +4442,32 @@ export async function reReviewStoredPullRequest( }).catch(() => undefined); throw new PrActuationLockContendedError(repoFullName, pr.number, "public-surface-publish"); } + // #10184: a PR whose answer has not changed does not need asking again yet. metagraphed#8886 produced 56 + // identical `hold | missing_linked_issue` verdicts on ONE head SHA in 47 minutes; four such PRs made 66% of + // all decision records in a two-hour window, and that window exhausted the installation's REST quota. + // + // Placed AFTER the lock claim so the check itself is nearly free (one cache read on a pass that already + // owns the PR) and BEFORE the refresh, so a backed-off pass spends nothing. Returns rather than throws: + // this is not contention, there is no work to retry, and the state is already published and correct. + // + // Fails OPEN in every uncertain case -- no state, unreadable state, no cache -- see verdict-stability.ts. + // The delay is capped, so a stuck PR is still revisited; it just stops being asked 1.2x/minute. + if (pr.headSha) { + const stability = await readVerdictStability(env.SELFHOST_TRANSIENT_CACHE, verdictStabilityKey(repoFullName, pr.number, pr.headSha)); + if (shouldSkipStableVerdict(stability, Date.now())) { + await recordAuditEvent(env, { + eventType: "github_app.review_skipped_stable_verdict", + actor: "loopover", + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + detail: `Verdict unchanged across ${stability?.repeats ?? 0} consecutive evaluations of this commit; backing off instead of re-deriving the same answer.`, + metadata: { deliveryId, repoFullName, repeats: stability?.repeats ?? 0 }, + }).catch(() => undefined); + await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken).catch(() => undefined); + // false = "did not re-review", the same signal this function's other early bail uses. + return false; + } + } // #2537 follow-up (gate-flagged): the durable review cache's only invalidation path is markPullRequestReviewsInvalidated // on a webhook (processors.ts). A "quiet" PR (no new pushes, slop evidence + manifest gate both off, no // pre-merge check paths) never hits any of the three reasons below, so a DROPPED invalidation write could sit diff --git a/src/review/verdict-stability.ts b/src/review/verdict-stability.ts new file mode 100644 index 000000000..e3bd546cc --- /dev/null +++ b/src/review/verdict-stability.ts @@ -0,0 +1,158 @@ +// Backoff for a pull request whose verdict never changes (#10184). +// +// metagraphed#8886 was evaluated 56 times on ONE unchanged commit in 47 minutes -- about 1.2 per minute -- +// producing the identical `hold | missing_linked_issue` every time. It is CONFLICTING so it cannot merge, +// held so it does not close, and its linked issue was closed by a merged rival (#10168), so the hold never +// clears. It is in a state it cannot leave, and nothing throttled re-entry. +// +// Four such PRs produced 66% of all decision records in a two-hour window. That is not a burst: it is a +// steady drip of legitimately distinct deliveries (CI completions, label writes, sibling activity) arriving +// long after any coalescing window. The webhook coalescer (#10127) collapses SIMULTANEOUS events and cannot +// help here. The missing control is a different kind: +// +// not "collapse events that arrive together" +// but "stop asking a question whose answer has not changed" +// +// ── WHAT COUNTS AS "THE SAME ANSWER" ────────────────────────────────────────────────────────────────────── +// The decision fields -- action, reason_code, hold_cause -- and nothing else. Deliberately NOT +// `record_digest`: #8886 has 56 DISTINCT digests for its 56 identical verdicts, because the digest commits to +// per-evaluation data. Using it would make every repeat look novel, which is exactly how this went unnoticed. +// +// ── WHY IT CANNOT STRAND A PR ───────────────────────────────────────────────────────────────────────────── +// The delay is capped, so a stuck PR is still revisited -- just at the sweep's cadence rather than 1.2x/min. +// A new head SHA resets it outright (a new commit is a genuinely new question), and so does any change in the +// verdict itself. Backoff that could grow without bound would trade a spend bug for a liveness bug. + +/** The decision fields that make two verdicts "the same answer". Kept as an explicit shape rather than a + * free-form string so a caller cannot accidentally fingerprint on something incidental. */ +export type VerdictFacts = { + action: string; + reasonCode: string; + /** #9991's recorded hold cause. Null/absent for a verdict that is not a hold. */ + holdCause?: string | null | undefined; +}; + +/** Stability state carried between evaluations for one (repo, pull, head SHA). */ +export type VerdictStabilityState = { + fingerprint: string; + /** How many CONSECUTIVE evaluations produced this same fingerprint, including the first. */ + repeats: number; + /** When the most recent evaluation ran. */ + lastEvaluatedMs: number; +}; + +/** First delay applied once a verdict has repeated enough to be considered stable. */ +export const VERDICT_BACKOFF_BASE_MS = 60_000; + +/** Ceiling on the delay. Chosen so a stuck PR is still revisited on roughly the sweep's own cadence -- the + * point is to stop the 1.2/min drip, not to stop looking. */ +export const VERDICT_BACKOFF_CAP_MS = 15 * 60_000; + +/** Repeats tolerated before backoff engages at all. Two identical verdicts can be an ordinary race (a webhook + * and the sweep landing together); a third means the answer is genuinely settled. Below this the behaviour + * is byte-identical to having no backoff. */ +export const VERDICT_BACKOFF_MIN_REPEATS = 3; + +/** PURE. The fingerprint two evaluations must share to count as the same answer. */ +export function verdictFingerprint(facts: VerdictFacts): string { + return [facts.action, facts.reasonCode, facts.holdCause ?? ""].join("|"); +} + +/** + * PURE. How long to wait before re-evaluating, given how many times this answer has repeated. + * + * Exponential from the base, capped. Returns 0 below the threshold so the common case -- a PR whose verdict + * is still moving -- is completely unaffected. + */ +export function verdictBackoffDelayMs(repeats: number): number { + if (repeats < VERDICT_BACKOFF_MIN_REPEATS) return 0; + const doublings = repeats - VERDICT_BACKOFF_MIN_REPEATS; + // No clamp on the exponent: an earlier version had one, and mutation testing showed removing it changed + // nothing, because `2 ** 1000` is Infinity and `Math.min(Infinity, cap)` is the cap. The cap is the single + // thing keeping this bounded, and it is directly tested -- a second guard that no test can distinguish is + // not defence in depth, it is a claim nobody is checking. + return Math.min(VERDICT_BACKOFF_BASE_MS * 2 ** doublings, VERDICT_BACKOFF_CAP_MS); +} + +/** + * PURE. Fold a fresh verdict into the prior state. + * + * A DIFFERENT fingerprint resets the count to 1 -- the answer moved, so whatever we had learned about its + * stability is void. Callers reset on a new head SHA by keying the state on the head SHA, so a new commit + * never sees the old state at all. + */ +export function recordVerdict(prior: VerdictStabilityState | null, facts: VerdictFacts, nowMs: number): VerdictStabilityState { + const fingerprint = verdictFingerprint(facts); + const repeats = prior !== null && prior.fingerprint === fingerprint ? prior.repeats + 1 : 1; + return { fingerprint, repeats, lastEvaluatedMs: nowMs }; +} + +/** + * PURE. Should this evaluation be skipped because the answer is settled and the backoff has not elapsed? + * + * Fails OPEN in every uncertain case -- no prior state, an unreadable state, or a delay of zero all evaluate + * normally. A backoff that engaged on missing information would silently stop reviewing PRs, which is far + * worse than the churn it is trying to prevent. + */ +export function shouldSkipStableVerdict(prior: VerdictStabilityState | null, nowMs: number): boolean { + if (prior === null) return false; + // HOLDS ONLY. "Same verdict" is not the same as "nothing happened": a pass can take real actions -- + // update-branch, cap accounting, assignment -- and still produce an unchanged verdict, and throttling that + // suppresses actual progress. The force-fresh-rebase test (#9497/#2552) is exactly this shape: three + // identical passes deliberately spend the 24h update-branch cap, and an earlier version of this backoff + // silently swallowed the third. + // + // A `hold` is the one action that means "the gate declined to act", so repeating it genuinely produces + // nothing -- and it is the case this exists for (#8886: 56 identical holds on one commit). Everything else + // keeps its current behaviour exactly. + if (!prior.fingerprint.startsWith("hold|")) return false; + const delay = verdictBackoffDelayMs(prior.repeats); + if (delay <= 0) return false; + return nowMs - prior.lastEvaluatedMs < delay; +} + +// ── PERSISTENCE ─────────────────────────────────────────────────────────────────────────────────────────── +// Same transient-cache idiom as ciPendingDeferStuck (processors.ts): keyed on repo#pr:headSha, so a NEW +// COMMIT never sees the old state -- the reset on a new head is structural rather than a rule someone has to +// remember. Best effort throughout: a cache miss or error yields null, which fails OPEN at every caller. + +/** Cache key. Includes the head SHA so a new commit starts clean. */ +export function verdictStabilityKey(repoFullName: string, prNumber: number, headSha: string): string { + return `verdict-stability:${repoFullName.toLowerCase()}#${prNumber}:${headSha}`; +} + +/** How long a stability record outlives its last write. Comfortably longer than the cap so a slow-drip PR + * keeps accumulating repeats, short enough that an abandoned head SHA does not linger. */ +const VERDICT_STABILITY_TTL_SECONDS = 6 * 3600; + +type TransientCache = { + get(key: string): Promise; + set(key: string, value: string, ttlSeconds: number): Promise; +}; + +/** Read prior stability state. Null on absence, malformed JSON, or any error -- every one of which must let + * the evaluation proceed rather than suppress it. */ +export async function readVerdictStability(cache: TransientCache | undefined, key: string): Promise { + if (!cache) return null; + try { + const raw = await cache.get(key); + if (!raw) return null; + const parsed = JSON.parse(raw) as Partial; + if (typeof parsed.fingerprint !== "string" || typeof parsed.repeats !== "number" || typeof parsed.lastEvaluatedMs !== "number") return null; + if (!Number.isFinite(parsed.repeats) || !Number.isFinite(parsed.lastEvaluatedMs)) return null; + return { fingerprint: parsed.fingerprint, repeats: parsed.repeats, lastEvaluatedMs: parsed.lastEvaluatedMs }; + } catch { + return null; + } +} + +/** Persist stability state. Best effort: a failed write means the next evaluation sees no prior state and + * proceeds normally, which is the safe direction. */ +export async function writeVerdictStability(cache: TransientCache | undefined, key: string, state: VerdictStabilityState): Promise { + if (!cache) return; + try { + await cache.set(key, JSON.stringify(state), VERDICT_STABILITY_TTL_SECONDS); + } catch { + // Telemetry-grade write; never fail the pass carrying it. + } +} diff --git a/test/unit/verdict-stability.test.ts b/test/unit/verdict-stability.test.ts new file mode 100644 index 000000000..22a027669 --- /dev/null +++ b/test/unit/verdict-stability.test.ts @@ -0,0 +1,211 @@ +import { describe, expect, it } from "vitest"; + +import { + recordVerdict, + shouldSkipStableVerdict, + verdictBackoffDelayMs, + verdictFingerprint, + VERDICT_BACKOFF_BASE_MS, + VERDICT_BACKOFF_CAP_MS, + VERDICT_BACKOFF_MIN_REPEATS, + verdictStabilityKey, + readVerdictStability, + writeVerdictStability, + type VerdictStabilityState, +} from "../../src/review/verdict-stability"; + +// #10184: metagraphed#8886 was evaluated 56 times on ONE unchanged commit in 47 minutes, producing the +// identical `hold | missing_linked_issue` every time. Four such PRs made 66% of all decision records in a +// two-hour window, and the same window exhausted the installation's GitHub REST quota. +// +// Every test below is about a way this control could be worse than the problem: stranding a PR it should +// still revisit, engaging on a verdict that is actually moving, or failing closed on missing state. + +const HOLD = { action: "hold", reasonCode: "missing_linked_issue", holdCause: null }; + +describe("verdictFingerprint", () => { + it("is the DECISION fields and nothing else", () => { + expect(verdictFingerprint(HOLD)).toBe("hold|missing_linked_issue|"); + expect(verdictFingerprint({ action: "hold", reasonCode: "success", holdCause: "guardrailHit" })).toBe("hold|success|guardrailHit"); + }); + + it("separates verdicts that differ only by hold cause", () => { + // #9991 exists precisely because "hold / success" conflated seven mechanisms. Two different causes are + // two different answers, and treating them as one would back off through a genuine change. + const a = verdictFingerprint({ action: "hold", reasonCode: "success", holdCause: "guardrailHit" }); + const b = verdictFingerprint({ action: "hold", reasonCode: "success", holdCause: "screenshotEvidenceHold" }); + expect(a).not.toBe(b); + }); + + it("treats null and absent hold cause identically", () => { + expect(verdictFingerprint({ action: "merge", reasonCode: "success" })).toBe( + verdictFingerprint({ action: "merge", reasonCode: "success", holdCause: null }), + ); + }); +}); + +describe("verdictBackoffDelayMs", () => { + it("is ZERO below the threshold — the common path is untouched", () => { + // A PR whose verdict is still moving must behave exactly as it did before this existed. + for (let n = 0; n < VERDICT_BACKOFF_MIN_REPEATS; n += 1) { + expect(verdictBackoffDelayMs(n), `repeats=${n}`).toBe(0); + } + }); + + it("engages at the threshold and grows exponentially", () => { + expect(verdictBackoffDelayMs(VERDICT_BACKOFF_MIN_REPEATS)).toBe(VERDICT_BACKOFF_BASE_MS); + expect(verdictBackoffDelayMs(VERDICT_BACKOFF_MIN_REPEATS + 1)).toBe(VERDICT_BACKOFF_BASE_MS * 2); + expect(verdictBackoffDelayMs(VERDICT_BACKOFF_MIN_REPEATS + 2)).toBe(VERDICT_BACKOFF_BASE_MS * 4); + }); + + it("INVARIANT: never exceeds the cap, however long a PR stays stuck", () => { + // The liveness property. An uncapped backoff would turn a spend bug into a PR nobody ever looks at again. + for (const repeats of [10, 50, 500, 10_000]) { + expect(verdictBackoffDelayMs(repeats), `repeats=${repeats}`).toBeLessThanOrEqual(VERDICT_BACKOFF_CAP_MS); + } + }); + + it("stays finite at an absurd repeat count — the cap, not an exponent clamp, is what bounds this", () => { + // `2 ** 1000` is Infinity and `Math.min(Infinity, cap)` is the cap, so the cap alone is sufficient. An + // earlier version also clamped the exponent; mutation testing showed no test could tell the difference, + // so the clamp was removed rather than kept as an unverifiable second guard. + expect(Number.isFinite(verdictBackoffDelayMs(1000))).toBe(true); + expect(verdictBackoffDelayMs(1000)).toBe(VERDICT_BACKOFF_CAP_MS); + }); + + it("is monotonic — more repeats never means a shorter wait", () => { + let prev = -1; + for (let n = 0; n <= 20; n += 1) { + const d = verdictBackoffDelayMs(n); + expect(d, `repeats=${n}`).toBeGreaterThanOrEqual(prev); + prev = d; + } + }); +}); + +describe("recordVerdict", () => { + it("counts consecutive identical verdicts", () => { + let state: VerdictStabilityState | null = null; + for (let i = 1; i <= 5; i += 1) state = recordVerdict(state, HOLD, i * 1000); + expect(state?.repeats).toBe(5); + }); + + it("REGRESSION: a CHANGED verdict resets the count to 1", () => { + // The answer moved, so everything learned about its stability is void. Without this, a PR that flipped + // between two states would keep backing off as though settled. + let state = recordVerdict(null, HOLD, 1000); + state = recordVerdict(state, HOLD, 2000); + state = recordVerdict(state, { action: "merge", reasonCode: "success" }, 3000); + expect(state.repeats).toBe(1); + expect(state.fingerprint).toBe("merge|success|"); + }); + + it("resets when only the hold cause changes", () => { + let state = recordVerdict(null, { action: "hold", reasonCode: "success", holdCause: "guardrailHit" }, 1000); + state = recordVerdict(state, { action: "hold", reasonCode: "success", holdCause: "advisoryCheckHold" }, 2000); + expect(state.repeats).toBe(1); + }); +}); + +describe("shouldSkipStableVerdict", () => { + const settled = (nowMs: number): VerdictStabilityState => ({ + fingerprint: verdictFingerprint(HOLD), + repeats: VERDICT_BACKOFF_MIN_REPEATS, + lastEvaluatedMs: nowMs, + }); + + it("skips while the backoff window is open", () => { + expect(shouldSkipStableVerdict(settled(0), VERDICT_BACKOFF_BASE_MS - 1)).toBe(true); + }); + + it("evaluates again once the window has elapsed", () => { + expect(shouldSkipStableVerdict(settled(0), VERDICT_BACKOFF_BASE_MS)).toBe(false); + expect(shouldSkipStableVerdict(settled(0), VERDICT_BACKOFF_BASE_MS + 1)).toBe(false); + }); + + it("FAILS OPEN with no prior state", () => { + // Missing state must never suppress a review. Failing closed here would silently stop reviewing PRs -- + // far worse than the churn this prevents. + expect(shouldSkipStableVerdict(null, Date.now())).toBe(false); + }); + + it("FAILS OPEN below the repeat threshold, however recent the last evaluation", () => { + const fresh: VerdictStabilityState = { fingerprint: verdictFingerprint(HOLD), repeats: 1, lastEvaluatedMs: 1000 }; + expect(shouldSkipStableVerdict(fresh, 1001)).toBe(false); + }); + + it("REGRESSION: only a HOLD is ever backed off — a pass that ACTED is never throttled", () => { + // "Same verdict" is not "nothing happened". A pass can take real actions (update-branch, cap accounting, + // assignment) and still produce an unchanged verdict; throttling that suppresses progress. Caught by the + // force-fresh-rebase test (#9497/#2552), where three deliberate identical passes spend the 24h + // update-branch cap and an earlier version of this backoff swallowed the third. + for (const action of ["merge", "close", "update_branch", "approve", "label"]) { + const acted: VerdictStabilityState = { + fingerprint: verdictFingerprint({ action, reasonCode: "success" }), + repeats: 99, + lastEvaluatedMs: 0, + }; + expect(shouldSkipStableVerdict(acted, 1), action).toBe(false); + } + // ...while the hold this exists for still backs off. + const held: VerdictStabilityState = { fingerprint: verdictFingerprint(HOLD), repeats: 99, lastEvaluatedMs: 0 }; + expect(shouldSkipStableVerdict(held, 1)).toBe(true); + }); + + it("INVARIANT: a stuck PR is always revisited within the cap", () => { + // The liveness guarantee stated as a property rather than trusted from the delay function. + const veryStuck: VerdictStabilityState = { fingerprint: verdictFingerprint(HOLD), repeats: 9999, lastEvaluatedMs: 0 }; + expect(shouldSkipStableVerdict(veryStuck, VERDICT_BACKOFF_CAP_MS)).toBe(false); + }); +}); + +describe("persistence (#10184)", () => { + const makeCache = () => { + const store = new Map(); + return { + store, + get: async (k: string) => store.get(k) ?? null, + set: async (k: string, v: string) => void store.set(k, v), + }; + }; + + it("keys on the head SHA, so a new commit starts clean", () => { + // The reset-on-new-commit rule is structural rather than something a caller must remember. + const a = verdictStabilityKey("Acme/Widgets", 7, "aaa"); + const b = verdictStabilityKey("acme/widgets", 7, "bbb"); + expect(a).not.toBe(b); + expect(a).toBe(verdictStabilityKey("acme/widgets", 7, "aaa")); // repo case-insensitive + }); + + it("round-trips state", async () => { + const cache = makeCache(); + const key = verdictStabilityKey("acme/widgets", 7, "aaa"); + const state = recordVerdict(null, HOLD, 1000); + await writeVerdictStability(cache, key, state); + expect(await readVerdictStability(cache, key)).toEqual(state); + }); + + it("FAILS OPEN on absent, malformed, or mistyped state", async () => { + // Every one of these must let the evaluation proceed. Returning a bogus state instead would suppress a + // review on corrupt data -- the one outcome worse than the churn this prevents. + const cache = makeCache(); + expect(await readVerdictStability(cache, "missing")).toBeNull(); + cache.store.set("bad-json", "{not json"); + expect(await readVerdictStability(cache, "bad-json")).toBeNull(); + cache.store.set("wrong-shape", JSON.stringify({ fingerprint: 1, repeats: "x" })); + expect(await readVerdictStability(cache, "wrong-shape")).toBeNull(); + cache.store.set("nan", JSON.stringify({ fingerprint: "f", repeats: Number.NaN, lastEvaluatedMs: 1 })); + expect(await readVerdictStability(cache, "nan")).toBeNull(); + }); + + it("FAILS OPEN with no cache configured at all", async () => { + expect(await readVerdictStability(undefined, "k")).toBeNull(); + await expect(writeVerdictStability(undefined, "k", recordVerdict(null, HOLD, 1))).resolves.toBeUndefined(); + }); + + it("a throwing cache never propagates — telemetry must not fail the pass carrying it", async () => { + const broken = { get: async () => { throw new Error("boom"); }, set: async () => { throw new Error("boom"); } }; + expect(await readVerdictStability(broken, "k")).toBeNull(); + await expect(writeVerdictStability(broken, "k", recordVerdict(null, HOLD, 1))).resolves.toBeUndefined(); + }); +});