diff --git a/src/queue/ci-resolution.ts b/src/queue/ci-resolution.ts index a4ee9d765..4eaf03837 100644 --- a/src/queue/ci-resolution.ts +++ b/src/queue/ci-resolution.ts @@ -339,6 +339,43 @@ export function cachedLiveMergeState( return next; } +// #merge-race (observed live: metagraphed#8037 and others -- an approved, gate-passing, fully-autonomous-merge +// PR sat unmerged for ~6 minutes, its own merge window, long enough for an overlapping sibling PR to land first +// and base-conflict it out from under it): fetchLivePullRequestMergeState's own doc comment already documents +// that GitHub computes mergeable_state ASYNCHRONOUSLY and can return "unknown" (still computing) even on a +// forced live re-fetch taken moments after posting the approving review -- previously the disposition simply +// accepted that single read and deferred to the next scheduled regate sweep (several minutes later) to catch +// the now-resolved state. Retry a short, bounded number of times SPECIFICALLY on "unknown" (the one transient +// value -- "dirty"/"blocked"/"behind" are real, stable, non-computing states that must never be retried) before +// falling through to the same defer-to-sweep behavior as before. Mirrors the identical "GitHub hasn't finished +// computing X yet" pattern already used for the diff/files lag (backfill.ts's +// fetchAndStorePullRequestFilesForReview / REVIEW_FILES_EMPTY_RETRY_DELAY_MS). +const MERGE_STATE_UNKNOWN_MAX_RETRIES = 2; +let mergeStateUnknownRetryDelayMsOverride: number | null = null; +/** Test-only override (#test-hotspots convention) -- production default (2s) is untouched; a test that wants + * to exercise the retry loop's own logic sets this near-zero via test/helpers/vitest-setup.ts. */ +export function setMergeStateUnknownRetryDelayMsForTest(value: number | null): void { + mergeStateUnknownRetryDelayMsOverride = value; +} +function mergeStateUnknownRetryDelayMs(): number { + return mergeStateUnknownRetryDelayMsOverride ?? 2_000; +} +const sleepForMergeStateRetry = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +async function fetchLivePullRequestMergeStateWithUnknownRetry( + env: Env, + repoFullName: string, + prNumber: number, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + for (let attempt = 0; ; attempt += 1) { + const state = await fetchLivePullRequestMergeState(env, repoFullName, prNumber, token, admissionKey); + if (state !== "unknown" || attempt >= MERGE_STATE_UNKNOWN_MAX_RETRIES) return state; + await sleepForMergeStateRetry(mergeStateUnknownRetryDelayMs()); + } +} + // #4220 contradiction: the stored pr.mergeableState lags GitHub's async recompute, so a base-conflicting PR could // read clean here (safe to merge) while the disposition reads the live dirty and auto-CLOSES it. This ALWAYS // 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( const next = evictLiveFactOnReject( facts.mergeStates, key, - fetchLivePullRequestMergeState(env, repoFullName, prNumber, token, admissionKey), + fetchLivePullRequestMergeStateWithUnknownRetry(env, repoFullName, prNumber, token, admissionKey), ); facts.mergeStates.set(key, next); facts.forcedMergeStateKeys.add(key); diff --git a/test/helpers/vitest-setup.ts b/test/helpers/vitest-setup.ts index 766732677..1223a0989 100644 --- a/test/helpers/vitest-setup.ts +++ b/test/helpers/vitest-setup.ts @@ -8,6 +8,8 @@ // behavior (attempt count, precedence) still exercises the identical code path, just without the sleep. import { setReviewFilesEmptyRetryDelayMsForTest } from "../../src/github/backfill"; import { setGithubRateLimitRetrySleepCapMsForTest } from "../../src/github/client"; +import { setMergeStateUnknownRetryDelayMsForTest } from "../../src/queue/ci-resolution"; setReviewFilesEmptyRetryDelayMsForTest(0); setGithubRateLimitRetrySleepCapMsForTest(0); +setMergeStateUnknownRetryDelayMsForTest(0); diff --git a/test/unit/ci-resolution.test.ts b/test/unit/ci-resolution.test.ts index 27231f6a1..9dece4218 100644 --- a/test/unit/ci-resolution.test.ts +++ b/test/unit/ci-resolution.test.ts @@ -4,7 +4,9 @@ import { cachedLiveCiAggregate, cachedRequiredStatusContexts, observeRequiredContextsLookup, + refreshLiveMergeState, REQUIRED_CONTEXTS_UNRESOLVED_METRIC, + setMergeStateUnknownRetryDelayMsForTest, } from "../../src/queue/ci-resolution"; import type { LiveGithubFacts } from "../../src/queue/processors"; import { counterValue, resetMetrics } from "../../src/selfhost/metrics"; @@ -20,6 +22,53 @@ function emptyFacts(): LiveGithubFacts { }; } +describe("refreshLiveMergeState retries a transient \"unknown\" read (#merge-race)", () => { + afterEach(() => { + vi.restoreAllMocks(); + setMergeStateUnknownRetryDelayMsForTest(0); + }); + + it("REGRESSION: retries once and resolves to \"clean\" within the SAME pass when GitHub's first read is still computing", async () => { + // metagraphed#8037 (live incident): an approved, gate-passing PR sat unmerged for ~6 minutes because the + // one live mergeable_state read taken right after posting the review came back "unknown" (GitHub still + // computing) and the disposition deferred to the next scheduled sweep. This proves the retry converts that + // into an immediate same-pass resolution instead. + const fetchSpy = vi + .spyOn(backfillModule, "fetchLivePullRequestMergeState") + .mockResolvedValueOnce("unknown") + .mockResolvedValueOnce("clean"); + const facts = emptyFacts(); + const result = await refreshLiveMergeState(createTestEnv(), "owner/repo", facts, 7, "tok"); + expect(result).toBe("clean"); + expect(fetchSpy).toHaveBeenCalledTimes(2); + }); + + it("gives up after the retry cap and still returns \"unknown\" (falls through to the next scheduled sweep, unchanged prior behavior)", async () => { + const fetchSpy = vi.spyOn(backfillModule, "fetchLivePullRequestMergeState").mockResolvedValue("unknown"); + const facts = emptyFacts(); + const result = await refreshLiveMergeState(createTestEnv(), "owner/repo", facts, 7, "tok"); + expect(result).toBe("unknown"); + // 1 original + MAX_RETRIES(2) = 3 total attempts, never unbounded. + expect(fetchSpy).toHaveBeenCalledTimes(3); + }); + + it("does not retry a real, stable non-clean state (dirty) — only the transient 'still computing' value", async () => { + const fetchSpy = vi.spyOn(backfillModule, "fetchLivePullRequestMergeState").mockResolvedValueOnce("dirty"); + const facts = emptyFacts(); + const result = await refreshLiveMergeState(createTestEnv(), "owner/repo", facts, 7, "tok"); + expect(result).toBe("dirty"); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("does not retry an outright fetch failure (undefined) — best-effort, same as a stable state", async () => { + const fetchSpy = vi.spyOn(backfillModule, "fetchLivePullRequestMergeState").mockResolvedValueOnce(undefined); + const facts = emptyFacts(); + const result = await refreshLiveMergeState(createTestEnv(), "owner/repo", facts, 7, "tok"); + expect(result).toBeUndefined(); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); +}); + describe("cachedLiveCiAggregate request-scoped memoization (#4498)", () => { afterEach(() => { vi.restoreAllMocks();