Skip to content

Commit 2bb5522

Browse files
fix(orb): invalidate durable CI cache on coalesced fork-PR completions (#8737)
maybeReReviewOnCiCompletion returns early for fork PRs (empty pull_requests[]) once a completion event in a burst is head-SHA coalesced, before ever reaching the invalidation loop whose own comment promises it runs "for EVERY resolved PR, regardless of whether the re-review below actually fires". Only the first completion per 60s window invalidated the durable CI-state cache, so a reader could observe a stale pre-completion aggregate for up to the cache TTL — for fork PRs only (same-repo PRs invalidate before their coalesce check). Invalidate the durable cache in the coalesced branch too, resolving via the fast stored-DB head-SHA lookup only (no live fork fallback — the round-trip the coalesce exists to avoid; an untracked fork the DB misses has no cache entry to clear, mirroring maybeInvalidateCiCacheOnLegacyCiEvent). The coalescing itself is unchanged: it still suppresses the duplicate re-review dispatch. Closes #8684 Co-authored-by: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Co-authored-by: loopover-orb[bot] <296761690+loopover-orb[bot]@users.noreply.github.com>
1 parent 36d48b5 commit 2bb5522

2 files changed

Lines changed: 87 additions & 2 deletions

File tree

src/queue/processors.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4572,12 +4572,25 @@ async function maybeReReviewOnCiCompletion(
45724572
const headSha = ciCompletionHeadSha(eventName, payload);
45734573
if (isConvergenceRepoAllowed(env, repoFullName)) {
45744574
// GitHub can emit many empty-pull_requests CI completions for the same fork head SHA. Claim a head-SHA
4575-
// window before the fallback resolver so duplicate events do not repeat DB scans or commits/{sha}/pulls calls.
4575+
// window before the fallback resolver so duplicate events skip the re-review dispatch and its live
4576+
// commits/{sha}/pulls round-trip (the cheap stored-DB invalidation still runs -- see the branch below).
45764577
if (
45774578
populatedPrNumbers.length === 0 &&
45784579
headSha &&
45794580
(await ciHeadShaResolutionCoalesced(env, repoFullName, headSha))
45804581
) {
4582+
// The re-review DISPATCH is coalesced away here, but the durable CI-state cache invalidation is NOT
4583+
// meant to be -- the loop below invalidates "for EVERY resolved PR, regardless of whether the re-review
4584+
// actually fires", and a fork PR must get that same unconditional guarantee same-repo PRs already do
4585+
// (a coalesced completion in a burst carries a newer settled CI state that a reader must not miss).
4586+
// Resolve via the fast STORED-DB head-SHA lookup only -- no live fork fallback (that's the round-trip the
4587+
// coalesce exists to avoid): a durable cache entry exists only for a PR this process already tracks, so an
4588+
// untracked fork the DB lookup misses has nothing stale to clear (mirrors maybeInvalidateCiCacheOnLegacyCiEvent).
4589+
const openPullRequests = await listOpenPullRequests(env, repoFullName).catch(() => []);
4590+
for (const pr of openPullRequests) {
4591+
if (pr.headSha !== headSha) continue;
4592+
await invalidateCiStateCache(env, repoFullName, pr.number).catch(() => undefined);
4593+
}
45814594
await recordWebhookEvent(env, {
45824595
deliveryId,
45834596
eventName,

test/unit/ci-completion-fork-resume.test.ts

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
22
import { ciCompletionHeadSha, processJob, resolveCiCompletionPrNumbers } from "../../src/queue/processors";
3-
import { upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories";
3+
import { getPullRequestDetailSyncState, upsertPullRequestDetailSyncState, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories";
44
import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader";
55
import { createTestEnv } from "../helpers/d1";
66
import type { GitHubWebhookPayload, JobMessage } from "../../src/types";
@@ -207,6 +207,78 @@ describe("CI-completion fork PR resume (head-SHA fallback)", () => {
207207
expect(webhook?.status).toBe("processed");
208208
});
209209

210+
it("invalidation: a COALESCED fork completion still invalidates the durable CI-state cache (on BOTH events)", async () => {
211+
const cache = new MemoryTransientCache();
212+
const env = createTestEnv({ SELFHOST_TRANSIENT_CACHE: cache });
213+
await seedForkResumeRepo(env, "JSONbored/gittensory", 99, FORK_SHA);
214+
// A second open PR on a DIFFERENT head SHA must be skipped by the invalidation loop's head-SHA filter.
215+
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 98, title: "Other fork PR", state: "open", user: { login: "someone-else" }, head: { sha: "feedface0000feedface0000feedface0000feed" }, labels: [], body: "unrelated" });
216+
// A throwing fetch proves both events resolve/invalidate off the stored DB row, never the live commits/pulls call.
217+
vi.stubGlobal("fetch", async () => {
218+
throw new Error("fetch must not be called: the stored fork PR row matches the head SHA");
219+
});
220+
221+
// Seed a STALE, pre-completion CI aggregate a reader could observe, keyed to the completing head SHA.
222+
const seedStaleCiState = async (pullNumber: number, ciState: "failed" | "passed") =>
223+
upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber, status: "complete", ciHeadSha: FORK_SHA, ciState, ciStateFetchedAt: new Date().toISOString() });
224+
225+
// Event 1 (NOT coalesced): claims the head-SHA window, invalidates via the re-review loop.
226+
await seedStaleCiState(99, "failed");
227+
await seedStaleCiState(98, "failed");
228+
await processJob(env, {
229+
type: "github-webhook",
230+
deliveryId: "fork-invalidate-1",
231+
eventName: "check_suite",
232+
payload: checkSuitePayload({ repo: "JSONbored/gittensory", installationId: 5001, headSha: FORK_SHA, prNumbers: [] }),
233+
});
234+
expect((await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 99))?.ciState).toBeNull();
235+
236+
// A reader repopulates the durable cache with the burst's NEXT, DIFFERING settled CI state before the second event.
237+
await seedStaleCiState(99, "passed");
238+
239+
// Event 2 (COALESCED within the same window): the re-review dispatch is suppressed, but the fix guarantees the
240+
// durable-cache invalidation still runs -- otherwise a reader would observe the stale "passed" snapshot until TTL.
241+
await processJob(env, {
242+
type: "github-webhook",
243+
deliveryId: "fork-invalidate-2",
244+
eventName: "check_suite",
245+
payload: checkSuitePayload({ repo: "JSONbored/gittensory", installationId: 5001, headSha: FORK_SHA, prNumbers: [] }),
246+
});
247+
expect((await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 99))?.ciState).toBeNull();
248+
// PR 98 (mismatched head SHA) is never touched by the invalidation loop -- its stale state stays put.
249+
expect((await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 98))?.ciState).toBe("failed");
250+
});
251+
252+
it("regression: the COALESCED second fork completion suppresses the duplicate re-review dispatch", async () => {
253+
const cache = new MemoryTransientCache();
254+
const env = createTestEnv({ SELFHOST_TRANSIENT_CACHE: cache });
255+
await seedForkResumeRepo(env, "JSONbored/gittensory", 99, FORK_SHA);
256+
vi.stubGlobal("fetch", async () => {
257+
throw new Error("fetch must not be called: the stored fork PR row matches the head SHA");
258+
});
259+
260+
for (const deliveryId of ["fork-dispatch-1", "fork-dispatch-2"]) {
261+
await processJob(env, {
262+
type: "github-webhook",
263+
deliveryId,
264+
eventName: "check_suite",
265+
payload: checkSuitePayload({ repo: "JSONbored/gittensory", installationId: 5001, headSha: FORK_SHA, prNumbers: [] }),
266+
});
267+
}
268+
269+
// The fork-resume audit is written on the dispatch path only (AFTER the coalescing early-return), so exactly one
270+
// audit across two same-window events proves the second event's re-review dispatch was coalesced away, not re-run.
271+
const audits = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?")
272+
.bind("github_app.ci_completion_fork_resume")
273+
.first<{ n: number }>();
274+
expect(audits?.n).toBe(1);
275+
// Both events are still recorded as processed regardless of coalescing.
276+
const processed = await env.DB.prepare("select count(*) as n from webhook_events where status = 'processed' and delivery_id in (?, ?)")
277+
.bind("fork-dispatch-1", "fork-dispatch-2")
278+
.first<{ n: number }>();
279+
expect(processed?.n).toBe(2);
280+
});
281+
210282
it("dispatch: a head SHA that matches nothing is a no-op (no fork audit, no throw, webhook recorded)", async () => {
211283
const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
212284
await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 5001);

0 commit comments

Comments
 (0)