Skip to content

Commit bd273c1

Browse files
authored
fix(queue): retry a transient GitHub mergeable_state read before deferring to the next sweep (#8560)
Live incident: metagraphed#8037 (and others) sat unmerged for ~6 minutes after being approved, gate-passing, and fully-autonomous-merge-configured (autonomy.merge: auto, requireApprovals: 0). GitHub computes mergeable_state ASYNCHRONOUSLY after a push/review and can return "unknown" (still computing) even on the forced live re-fetch taken moments after posting the approving review -- the disposition correctly refused to merge into an unconfirmed state, but simply deferred to the next scheduled regate sweep (~6 minutes later) instead of retrying. That multi-minute window is exactly where an overlapping sibling PR can land first and base-conflict the original PR out from under it. refreshLiveMergeState now retries a short, bounded number of times (2) SPECIFICALLY on "unknown" -- "dirty"/"blocked"/"behind" are real, stable, non-computing states and are never retried -- before falling through to the same defer-to-sweep behavior as before. Mirrors the identical GitHub-lag pattern already used for diff/files computation (fetchAndStorePullRequestFilesForReview). Test-only delay override via the same vitest-setup.ts suite default this session's other perf fixes use, so the retry's own logic is exercised without adding real wall-clock time to the suite.
1 parent 0c19927 commit bd273c1

3 files changed

Lines changed: 89 additions & 1 deletion

File tree

src/queue/ci-resolution.ts

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,43 @@ export function cachedLiveMergeState(
339339
return next;
340340
}
341341

342+
// #merge-race (observed live: metagraphed#8037 and others -- an approved, gate-passing, fully-autonomous-merge
343+
// PR sat unmerged for ~6 minutes, its own merge window, long enough for an overlapping sibling PR to land first
344+
// and base-conflict it out from under it): fetchLivePullRequestMergeState's own doc comment already documents
345+
// that GitHub computes mergeable_state ASYNCHRONOUSLY and can return "unknown" (still computing) even on a
346+
// forced live re-fetch taken moments after posting the approving review -- previously the disposition simply
347+
// accepted that single read and deferred to the next scheduled regate sweep (several minutes later) to catch
348+
// the now-resolved state. Retry a short, bounded number of times SPECIFICALLY on "unknown" (the one transient
349+
// value -- "dirty"/"blocked"/"behind" are real, stable, non-computing states that must never be retried) before
350+
// falling through to the same defer-to-sweep behavior as before. Mirrors the identical "GitHub hasn't finished
351+
// computing X yet" pattern already used for the diff/files lag (backfill.ts's
352+
// fetchAndStorePullRequestFilesForReview / REVIEW_FILES_EMPTY_RETRY_DELAY_MS).
353+
const MERGE_STATE_UNKNOWN_MAX_RETRIES = 2;
354+
let mergeStateUnknownRetryDelayMsOverride: number | null = null;
355+
/** Test-only override (#test-hotspots convention) -- production default (2s) is untouched; a test that wants
356+
* to exercise the retry loop's own logic sets this near-zero via test/helpers/vitest-setup.ts. */
357+
export function setMergeStateUnknownRetryDelayMsForTest(value: number | null): void {
358+
mergeStateUnknownRetryDelayMsOverride = value;
359+
}
360+
function mergeStateUnknownRetryDelayMs(): number {
361+
return mergeStateUnknownRetryDelayMsOverride ?? 2_000;
362+
}
363+
const sleepForMergeStateRetry = (ms: number): Promise<void> => new Promise((resolve) => setTimeout(resolve, ms));
364+
365+
async function fetchLivePullRequestMergeStateWithUnknownRetry(
366+
env: Env,
367+
repoFullName: string,
368+
prNumber: number,
369+
token: string | undefined,
370+
admissionKey?: GitHubRateLimitAdmissionKey,
371+
): Promise<string | undefined> {
372+
for (let attempt = 0; ; attempt += 1) {
373+
const state = await fetchLivePullRequestMergeState(env, repoFullName, prNumber, token, admissionKey);
374+
if (state !== "unknown" || attempt >= MERGE_STATE_UNKNOWN_MAX_RETRIES) return state;
375+
await sleepForMergeStateRetry(mergeStateUnknownRetryDelayMs());
376+
}
377+
}
378+
342379
// #4220 contradiction: the stored pr.mergeableState lags GitHub's async recompute, so a base-conflicting PR could
343380
// read clean here (safe to merge) while the disposition reads the live dirty and auto-CLOSES it. This ALWAYS
344381
// force-refetches live from GitHub and MUST NEVER be routed through the durable pull_request_detail_sync_state
@@ -356,7 +393,7 @@ export function refreshLiveMergeState(
356393
const next = evictLiveFactOnReject(
357394
facts.mergeStates,
358395
key,
359-
fetchLivePullRequestMergeState(env, repoFullName, prNumber, token, admissionKey),
396+
fetchLivePullRequestMergeStateWithUnknownRetry(env, repoFullName, prNumber, token, admissionKey),
360397
);
361398
facts.mergeStates.set(key, next);
362399
facts.forcedMergeStateKeys.add(key);

test/helpers/vitest-setup.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
// behavior (attempt count, precedence) still exercises the identical code path, just without the sleep.
99
import { setReviewFilesEmptyRetryDelayMsForTest } from "../../src/github/backfill";
1010
import { setGithubRateLimitRetrySleepCapMsForTest } from "../../src/github/client";
11+
import { setMergeStateUnknownRetryDelayMsForTest } from "../../src/queue/ci-resolution";
1112

1213
setReviewFilesEmptyRetryDelayMsForTest(0);
1314
setGithubRateLimitRetrySleepCapMsForTest(0);
15+
setMergeStateUnknownRetryDelayMsForTest(0);

test/unit/ci-resolution.test.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ import {
44
cachedLiveCiAggregate,
55
cachedRequiredStatusContexts,
66
observeRequiredContextsLookup,
7+
refreshLiveMergeState,
78
REQUIRED_CONTEXTS_UNRESOLVED_METRIC,
9+
setMergeStateUnknownRetryDelayMsForTest,
810
} from "../../src/queue/ci-resolution";
911
import type { LiveGithubFacts } from "../../src/queue/processors";
1012
import { counterValue, resetMetrics } from "../../src/selfhost/metrics";
@@ -20,6 +22,53 @@ function emptyFacts(): LiveGithubFacts {
2022
};
2123
}
2224

25+
describe("refreshLiveMergeState retries a transient \"unknown\" read (#merge-race)", () => {
26+
afterEach(() => {
27+
vi.restoreAllMocks();
28+
setMergeStateUnknownRetryDelayMsForTest(0);
29+
});
30+
31+
it("REGRESSION: retries once and resolves to \"clean\" within the SAME pass when GitHub's first read is still computing", async () => {
32+
// metagraphed#8037 (live incident): an approved, gate-passing PR sat unmerged for ~6 minutes because the
33+
// one live mergeable_state read taken right after posting the review came back "unknown" (GitHub still
34+
// computing) and the disposition deferred to the next scheduled sweep. This proves the retry converts that
35+
// into an immediate same-pass resolution instead.
36+
const fetchSpy = vi
37+
.spyOn(backfillModule, "fetchLivePullRequestMergeState")
38+
.mockResolvedValueOnce("unknown")
39+
.mockResolvedValueOnce("clean");
40+
const facts = emptyFacts();
41+
const result = await refreshLiveMergeState(createTestEnv(), "owner/repo", facts, 7, "tok");
42+
expect(result).toBe("clean");
43+
expect(fetchSpy).toHaveBeenCalledTimes(2);
44+
});
45+
46+
it("gives up after the retry cap and still returns \"unknown\" (falls through to the next scheduled sweep, unchanged prior behavior)", async () => {
47+
const fetchSpy = vi.spyOn(backfillModule, "fetchLivePullRequestMergeState").mockResolvedValue("unknown");
48+
const facts = emptyFacts();
49+
const result = await refreshLiveMergeState(createTestEnv(), "owner/repo", facts, 7, "tok");
50+
expect(result).toBe("unknown");
51+
// 1 original + MAX_RETRIES(2) = 3 total attempts, never unbounded.
52+
expect(fetchSpy).toHaveBeenCalledTimes(3);
53+
});
54+
55+
it("does not retry a real, stable non-clean state (dirty) — only the transient 'still computing' value", async () => {
56+
const fetchSpy = vi.spyOn(backfillModule, "fetchLivePullRequestMergeState").mockResolvedValueOnce("dirty");
57+
const facts = emptyFacts();
58+
const result = await refreshLiveMergeState(createTestEnv(), "owner/repo", facts, 7, "tok");
59+
expect(result).toBe("dirty");
60+
expect(fetchSpy).toHaveBeenCalledTimes(1);
61+
});
62+
63+
it("does not retry an outright fetch failure (undefined) — best-effort, same as a stable state", async () => {
64+
const fetchSpy = vi.spyOn(backfillModule, "fetchLivePullRequestMergeState").mockResolvedValueOnce(undefined);
65+
const facts = emptyFacts();
66+
const result = await refreshLiveMergeState(createTestEnv(), "owner/repo", facts, 7, "tok");
67+
expect(result).toBeUndefined();
68+
expect(fetchSpy).toHaveBeenCalledTimes(1);
69+
});
70+
});
71+
2372
describe("cachedLiveCiAggregate request-scoped memoization (#4498)", () => {
2473
afterEach(() => {
2574
vi.restoreAllMocks();

0 commit comments

Comments
 (0)