From 43b9c921ee023ec57a575cb35fff44036113d3f2 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Sun, 26 Jul 2026 01:29:33 +0000 Subject: [PATCH] fix(orb): invalidate durable CI cache on coalesced fork-PR completions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/queue/processors.ts | 15 ++++- test/unit/ci-completion-fork-resume.test.ts | 74 ++++++++++++++++++++- 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index f4623c4de7..92c805b4e2 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -4572,12 +4572,25 @@ async function maybeReReviewOnCiCompletion( const headSha = ciCompletionHeadSha(eventName, payload); if (isConvergenceRepoAllowed(env, repoFullName)) { // GitHub can emit many empty-pull_requests CI completions for the same fork head SHA. Claim a head-SHA - // window before the fallback resolver so duplicate events do not repeat DB scans or commits/{sha}/pulls calls. + // window before the fallback resolver so duplicate events skip the re-review dispatch and its live + // commits/{sha}/pulls round-trip (the cheap stored-DB invalidation still runs -- see the branch below). if ( populatedPrNumbers.length === 0 && headSha && (await ciHeadShaResolutionCoalesced(env, repoFullName, headSha)) ) { + // The re-review DISPATCH is coalesced away here, but the durable CI-state cache invalidation is NOT + // meant to be -- the loop below invalidates "for EVERY resolved PR, regardless of whether the re-review + // actually fires", and a fork PR must get that same unconditional guarantee same-repo PRs already do + // (a coalesced completion in a burst carries a newer settled CI state that a reader must not miss). + // Resolve via the fast STORED-DB head-SHA lookup only -- no live fork fallback (that's the round-trip the + // coalesce exists to avoid): a durable cache entry exists only for a PR this process already tracks, so an + // untracked fork the DB lookup misses has nothing stale to clear (mirrors maybeInvalidateCiCacheOnLegacyCiEvent). + const openPullRequests = await listOpenPullRequests(env, repoFullName).catch(() => []); + for (const pr of openPullRequests) { + if (pr.headSha !== headSha) continue; + await invalidateCiStateCache(env, repoFullName, pr.number).catch(() => undefined); + } await recordWebhookEvent(env, { deliveryId, eventName, diff --git a/test/unit/ci-completion-fork-resume.test.ts b/test/unit/ci-completion-fork-resume.test.ts index bb95ba08dd..82a149f181 100644 --- a/test/unit/ci-completion-fork-resume.test.ts +++ b/test/unit/ci-completion-fork-resume.test.ts @@ -1,6 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { ciCompletionHeadSha, processJob, resolveCiCompletionPrNumbers } from "../../src/queue/processors"; -import { upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; +import { getPullRequestDetailSyncState, upsertPullRequestDetailSyncState, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { createTestEnv } from "../helpers/d1"; import type { GitHubWebhookPayload, JobMessage } from "../../src/types"; @@ -207,6 +207,78 @@ describe("CI-completion fork PR resume (head-SHA fallback)", () => { expect(webhook?.status).toBe("processed"); }); + it("invalidation: a COALESCED fork completion still invalidates the durable CI-state cache (on BOTH events)", async () => { + const cache = new MemoryTransientCache(); + const env = createTestEnv({ SELFHOST_TRANSIENT_CACHE: cache }); + await seedForkResumeRepo(env, "JSONbored/gittensory", 99, FORK_SHA); + // A second open PR on a DIFFERENT head SHA must be skipped by the invalidation loop's head-SHA filter. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 98, title: "Other fork PR", state: "open", user: { login: "someone-else" }, head: { sha: "feedface0000feedface0000feedface0000feed" }, labels: [], body: "unrelated" }); + // A throwing fetch proves both events resolve/invalidate off the stored DB row, never the live commits/pulls call. + vi.stubGlobal("fetch", async () => { + throw new Error("fetch must not be called: the stored fork PR row matches the head SHA"); + }); + + // Seed a STALE, pre-completion CI aggregate a reader could observe, keyed to the completing head SHA. + const seedStaleCiState = async (pullNumber: number, ciState: "failed" | "passed") => + upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber, status: "complete", ciHeadSha: FORK_SHA, ciState, ciStateFetchedAt: new Date().toISOString() }); + + // Event 1 (NOT coalesced): claims the head-SHA window, invalidates via the re-review loop. + await seedStaleCiState(99, "failed"); + await seedStaleCiState(98, "failed"); + await processJob(env, { + type: "github-webhook", + deliveryId: "fork-invalidate-1", + eventName: "check_suite", + payload: checkSuitePayload({ repo: "JSONbored/gittensory", installationId: 5001, headSha: FORK_SHA, prNumbers: [] }), + }); + expect((await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 99))?.ciState).toBeNull(); + + // A reader repopulates the durable cache with the burst's NEXT, DIFFERING settled CI state before the second event. + await seedStaleCiState(99, "passed"); + + // Event 2 (COALESCED within the same window): the re-review dispatch is suppressed, but the fix guarantees the + // durable-cache invalidation still runs -- otherwise a reader would observe the stale "passed" snapshot until TTL. + await processJob(env, { + type: "github-webhook", + deliveryId: "fork-invalidate-2", + eventName: "check_suite", + payload: checkSuitePayload({ repo: "JSONbored/gittensory", installationId: 5001, headSha: FORK_SHA, prNumbers: [] }), + }); + expect((await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 99))?.ciState).toBeNull(); + // PR 98 (mismatched head SHA) is never touched by the invalidation loop -- its stale state stays put. + expect((await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 98))?.ciState).toBe("failed"); + }); + + it("regression: the COALESCED second fork completion suppresses the duplicate re-review dispatch", async () => { + const cache = new MemoryTransientCache(); + const env = createTestEnv({ SELFHOST_TRANSIENT_CACHE: cache }); + await seedForkResumeRepo(env, "JSONbored/gittensory", 99, FORK_SHA); + vi.stubGlobal("fetch", async () => { + throw new Error("fetch must not be called: the stored fork PR row matches the head SHA"); + }); + + for (const deliveryId of ["fork-dispatch-1", "fork-dispatch-2"]) { + await processJob(env, { + type: "github-webhook", + deliveryId, + eventName: "check_suite", + payload: checkSuitePayload({ repo: "JSONbored/gittensory", installationId: 5001, headSha: FORK_SHA, prNumbers: [] }), + }); + } + + // The fork-resume audit is written on the dispatch path only (AFTER the coalescing early-return), so exactly one + // audit across two same-window events proves the second event's re-review dispatch was coalesced away, not re-run. + const audits = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?") + .bind("github_app.ci_completion_fork_resume") + .first<{ n: number }>(); + expect(audits?.n).toBe(1); + // Both events are still recorded as processed regardless of coalescing. + const processed = await env.DB.prepare("select count(*) as n from webhook_events where status = 'processed' and delivery_id in (?, ?)") + .bind("fork-dispatch-1", "fork-dispatch-2") + .first<{ n: number }>(); + expect(processed?.n).toBe(2); + }); + it("dispatch: a head SHA that matches nothing is a no-op (no fork audit, no throw, webhook recorded)", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 5001);