From bf7c7f7dad47dd7fc80a1378bb31bc4cc9cb4a5e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 14:26:51 -0700 Subject: [PATCH 1/4] feat(github): cache PR reviews with webhook-based invalidation The recently-merged head-SHA file cache (#2527) deliberately left reviews uncached ("more volatile than files"), but reviews only actually change on a pull_request_review webhook, not on every sweep tick -- unlike files, reviews are independent of head SHA entirely. fetchAndStorePullRequestDetails now skips the GET /pulls/{n}/reviews call when a prior sync already covers every review-invalidating event: the new reviews_invalidated_at column (bumped by a pull_request_review webhook with action submitted/dismissed/edited) is compared against the existing reviews_synced_at timestamp. A stored row whose last review sync itself failed is excluded from the cache-hit path so a transient failure can't poison the cache into never retrying. Broadened the existing sync-state row lookup (previously gated on `!forceFiles && headSha`, which would silently disable review caching whenever headSha was momentarily unknown) since reviews never depend on headSha or the files-only forceFiles flag. Bare PR-state caching (state/mergeable_state/head SHA), the other half of this issue, was deliberately NOT implemented after auditing every live-read call site in the codebase: each one (the freshness guard in agent-action-executor.ts, the draft-dodge-close and reopen-reclose re-checks, the gate-override command, and the sweep-resync path) is a documented act-boundary that intentionally requires a live, uncached read immediately before a mutation or before publishing review output. mergeable_state already has per-pass caching via the existing LiveGithubFacts mechanism. Caching any of these would risk exactly the regression the issue's own acceptance criteria forbids ("No regression in any decision that depends on a live PR state"). Filing as a follow-up rather than force-fitting a cache onto call sites that don't have safe surface area for one. --- .../0094_pull_request_reviews_invalidated.sql | 6 + src/db/repositories.ts | 30 ++ src/db/schema.ts | 6 + src/github/backfill.ts | 35 +- src/queue/processors.ts | 24 ++ src/types.ts | 1 + .../backfill-reviews-cache-scoping.test.ts | 343 ++++++++++++++++++ test/unit/queue.test.ts | 95 +++++ 8 files changed, 537 insertions(+), 3 deletions(-) create mode 100644 migrations/0094_pull_request_reviews_invalidated.sql create mode 100644 test/unit/backfill-reviews-cache-scoping.test.ts diff --git a/migrations/0094_pull_request_reviews_invalidated.sql b/migrations/0094_pull_request_reviews_invalidated.sql new file mode 100644 index 0000000000..9666394ec0 --- /dev/null +++ b/migrations/0094_pull_request_reviews_invalidated.sql @@ -0,0 +1,6 @@ +-- PR reviews cache invalidation marker (#2537): bumped by a `pull_request_review` webhook +-- (submitted/dismissed/edited) to signal the cached `pull_request_reviews` rows are stale. NULL (the +-- default) means no invalidating event has been recorded, so a subsequent fetchAndStorePullRequestDetails +-- pass can skip the `GET /pulls/{n}/reviews` call when reviews_synced_at already covers it -- byte-identical +-- behavior for every existing row. +ALTER TABLE pull_request_detail_sync_state ADD COLUMN reviews_invalidated_at TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index c54c3fe6b8..e0ad6fc5a9 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -1152,6 +1152,7 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ headSha: state.headSha, filesSyncedAt: state.filesSyncedAt, reviewsSyncedAt: state.reviewsSyncedAt, + reviewsInvalidatedAt: state.reviewsInvalidatedAt, checksSyncedAt: state.checksSyncedAt, lastSyncedAt: state.lastSyncedAt, errorSummary: state.errorSummary, @@ -1164,6 +1165,7 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ headSha: state.headSha, filesSyncedAt: state.filesSyncedAt, reviewsSyncedAt: state.reviewsSyncedAt, + reviewsInvalidatedAt: state.reviewsInvalidatedAt, checksSyncedAt: state.checksSyncedAt, lastSyncedAt: state.lastSyncedAt, errorSummary: state.errorSummary, @@ -1172,6 +1174,33 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ }); } +/** Reviews-cache invalidation stamp (#2537): a pure single-field bump of `reviewsInvalidatedAt`, leaving + * every other column (`headSha`/`filesSyncedAt`/`reviewsSyncedAt`/`checksSyncedAt`/...) untouched when the + * row already exists — mirrors the narrow single-field touches on `pull_requests` (markPullRequestApproved, + * markPullRequestRegated). Creates the row (all other columns default/NULL) if this repo+PR has never been + * synced yet, so an early review webhook is not silently dropped. A plain D1 write, independent of headSha. */ +export async function markPullRequestReviewsInvalidated(env: Env, repoFullName: string, pullNumber: number): Promise { + const db = getDb(env.DB); + const now = nowIso(); + await db + .insert(pullRequestDetailSyncState) + .values({ + id: `${repoFullName}#${pullNumber}`, + repoFullName, + pullNumber, + status: "never_synced", + reviewsInvalidatedAt: now, + updatedAt: now, + }) + .onConflictDoUpdate({ + target: [pullRequestDetailSyncState.repoFullName, pullRequestDetailSyncState.pullNumber], + set: { + reviewsInvalidatedAt: now, + updatedAt: now, + }, + }); +} + export async function getPullRequestDetailSyncState(env: Env, fullName: string, pullNumber: number): Promise { const db = getDb(env.DB); const [row] = await db @@ -4226,6 +4255,7 @@ function toPullRequestDetailSyncStateRecord(row: typeof pullRequestDetailSyncSta headSha: row.headSha, filesSyncedAt: row.filesSyncedAt, reviewsSyncedAt: row.reviewsSyncedAt, + reviewsInvalidatedAt: row.reviewsInvalidatedAt, checksSyncedAt: row.checksSyncedAt, lastSyncedAt: row.lastSyncedAt, errorSummary: row.errorSummary, diff --git a/src/db/schema.ts b/src/db/schema.ts index 4dca4013e4..31f9f9d372 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -231,6 +231,12 @@ export const pullRequestDetailSyncState = sqliteTable( headSha: text("head_sha"), filesSyncedAt: text("files_synced_at"), reviewsSyncedAt: text("reviews_synced_at"), + // Bumped by a `pull_request_review` webhook (submitted/dismissed/edited) to signal the cached reviews are + // stale. NULL, or a value <= reviewsSyncedAt, means the last sync already covers every invalidating event, + // so fetchAndStorePullRequestDetails can skip the `GET /pulls/{n}/reviews` call. Reviews are independent of + // headSha (a new commit alone does not invalidate existing review state; only an actual review-webhook event + // does) -- unlike the files cache, which is why this uses its own timestamp column instead of headSha matching. + reviewsInvalidatedAt: text("reviews_invalidated_at"), checksSyncedAt: text("checks_synced_at"), lastSyncedAt: text("last_synced_at"), errorSummary: text("error_summary"), diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 4158a2e0e7..5e06884e12 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -1982,13 +1982,42 @@ async function fetchAndStorePullRequestDetails( // Durable repo+PR+headSha file snapshot (#audit-rate-headroom): a bare URL cache is insufficient because // `/pulls/{n}/files` has the SAME url across different heads. Reuse the stored `pull_request_files` rows // instead of refetching when the last successful files sync already covered the PR's CURRENT head SHA — - // only files are cached here; reviews/checks are more volatile at a fixed head and still refresh every call. - const existingState = !options.forceFiles && pr.headSha ? await getPullRequestDetailSyncState(env, repoFullName, pr.number) : null; + // only files are cached here; checks are more volatile at a fixed head and still refresh every call. + // + // The lookup below is gated by `!options.forceFiles || !pr.headSha` rather than by the files-only + // `!options.forceFiles && pr.headSha` condition the ORIGINAL (files-only) cache used. Reviews caching + // (#2537) reuses this SAME row (rather than adding a second DB read) but does not depend on `headSha` + // at all and is never controlled by `forceFiles` (that flag only ever forces a FILES re-fetch — see its + // name and its only caller, refreshPullRequestDetails's manual "force" option). If the lookup stayed + // gated on `pr.headSha` being present, a PR with a momentarily-empty head SHA would silently lose review + // caching too, even though reviews never needed a head SHA to begin with. So: skip the read only in the + // one case where NEITHER cache can use it (forceFiles is set AND headSha is present, i.e. the files-only + // force path) — every other combination still fetches the row so reviewsUpToDate can be computed. + const existingState = options.forceFiles && pr.headSha ? null : await getPullRequestDetailSyncState(env, repoFullName, pr.number); const filesUpToDate = Boolean(existingState?.headSha) && existingState?.headSha === pr.headSha && Boolean(existingState?.filesSyncedAt); + // Reviews cache (#2537): independent of headSha — a new commit alone does not invalidate existing review + // state, only an actual `pull_request_review` webhook (submitted/dismissed/edited) does, via + // markPullRequestReviewsInvalidated. Up to date when a prior sync recorded reviewsSyncedAt and either no + // invalidation has been recorded since, or the invalidation predates that sync. + // + // Every caller of this function stamps reviewsSyncedAt UNCONDITIONALLY once fetchAndStorePullRequestDetails + // returns, even when the reviews fetch itself failed (there is no per-segment success timestamp, only the + // aggregate PR-level errorSummary/status). Trusting a bare reviewsSyncedAt presence alone would let a + // transient review-fetch failure poison the cache forever, so this also excludes the case where the row's + // OWN errorSummary is the review-sync failure fetchPullRequestReviews just recorded for THIS PR (the same + // `Review sync failed for #` message every caller already greps for — see the /Review sync failed/i + // filters in backfillRepositorySegment). A files/checks-only failure still leaves reviews cached, matching + // the reviews-are-independent-of-files intent; only a review-specific failure forces a retry. + const reviewsSyncedAt = existingState?.reviewsSyncedAt; + const reviewsFetchPreviouslyFailed = Boolean(existingState?.errorSummary?.startsWith(`Review sync failed for #${pr.number}:`)); + const reviewsUpToDate = + Boolean(reviewsSyncedAt) && + !reviewsFetchPreviouslyFailed && + (!existingState?.reviewsInvalidatedAt || (reviewsSyncedAt ?? "") >= existingState.reviewsInvalidatedAt); const warningStart = warnings.length; const [files, reviews, checks] = await Promise.all([ filesUpToDate ? Promise.resolve([]) : fetchPullRequestFiles(env, repoFullName, pr.number, token, warnings, admissionKey, caller), - fetchPullRequestReviews(env, repoFullName, pr.number, token, warnings, admissionKey), + reviewsUpToDate ? Promise.resolve([]) : fetchPullRequestReviews(env, repoFullName, pr.number, token, warnings, admissionKey), fetchPullRequestChecks(env, repoFullName, pr, token, warnings, admissionKey), ]); const fileSyncFailed = warnings.slice(warningStart).some((warning) => warning.startsWith(`File sync failed for #${pr.number}:`)); diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ffbb345d69..d051075233 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -43,6 +43,7 @@ import { getCachedAiReview, putCachedAiReview, markPullRequestsRegated, + markPullRequestReviewsInvalidated, markPullRequestSurfacePublished, getLatestRegatedAt, claimRegateFanoutSlot, @@ -4220,6 +4221,29 @@ async function processGitHubWebhook( }), ); }); + // Reviews-cache invalidation (#2537): a `pull_request_review` webhook (submitted/dismissed/edited) is + // the ONLY event that can change the set of reviews GitHub reports for this PR, so it is the sole signal + // fetchAndStorePullRequestDetails's reviewsUpToDate check needs to know the cached reviews are stale. + // Independent of, and does not gate, any downstream processing below — best-effort like the outcome/ + // reversal recording above, so a transient D1 failure here never blocks the webhook. + if ( + eventName === "pull_request_review" && + (payload.action === "submitted" || payload.action === "dismissed" || payload.action === "edited") + ) { + await markPullRequestReviewsInvalidated(env, repoFullName, payloadPullRequest.number).catch((error) => { + /* v8 ignore next -- best-effort: cache-invalidation stamping never blocks the webhook. */ + console.warn( + JSON.stringify({ + level: "warn", + event: "pull_request_reviews_invalidate_failed", + deliveryId, + repository: repoFullName, + pullNumber: payloadPullRequest.number, + error: errorMessage(error), + }), + ); + }); + } const pr = await upsertPullRequestFromGitHub( env, repoFullName, diff --git a/src/types.ts b/src/types.ts index b8b6e57b70..aa0fb8870c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -870,6 +870,7 @@ export type PullRequestDetailSyncStateRecord = { headSha?: string | null | undefined; filesSyncedAt?: string | null | undefined; reviewsSyncedAt?: string | null | undefined; + reviewsInvalidatedAt?: string | null | undefined; checksSyncedAt?: string | null | undefined; lastSyncedAt?: string | null | undefined; errorSummary?: string | null | undefined; diff --git a/test/unit/backfill-reviews-cache-scoping.test.ts b/test/unit/backfill-reviews-cache-scoping.test.ts new file mode 100644 index 0000000000..3ba8640901 --- /dev/null +++ b/test/unit/backfill-reviews-cache-scoping.test.ts @@ -0,0 +1,343 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + getPullRequestDetailSyncState, + listPullRequestReviews, + markPullRequestReviewsInvalidated, + upsertPullRequestDetailSyncState, + upsertPullRequestFromGitHub, + upsertPullRequestReview, +} from "../../src/db/repositories"; +import { refreshPullRequestDetails } from "../../src/github/backfill"; +import { clearGitHubResponseCacheForTest } from "../../src/github/client"; +import { resetMetrics } from "../../src/selfhost/metrics"; +import { normalizeRegistryPayload } from "../../src/registry/normalize"; +import { persistRegistrySnapshot } from "../../src/registry/sync"; +import { createTestEnv } from "../helpers/d1"; + +describe("GitHub PR reviews cache scoping (#2537)", () => { + afterEach(() => { + clearGitHubResponseCacheForTest(); + resetMetrics(); + vi.unstubAllGlobals(); + }); + + async function seedRegisteredRepo(env: Env) { + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0, trusted_label_pipeline: true, label_multipliers: {} } }, + { kind: "raw-github", url: "https://example.test/master_repositories.json" }, + "2026-05-23T00:00:00.000Z", + ), + ); + } + + function stubFetchTracking(handler: (url: string, init?: RequestInit) => Response | Promise): string[] { + const urls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + urls.push(url); + return handler(url, init); + }); + return urls; + } + + it("fetches and stores reviews on first sync when no sync-state row exists (cache miss)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 60, + title: "Open PR, never synced", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-60" }, + labels: [], + body: "", + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/60/reviews") + ? Response.json([{ id: 1, user: { login: "maintainer" }, state: "APPROVED", author_association: "OWNER", submitted_at: "2026-05-20T00:00:00.000Z" }]) + : Response.json([]), + ); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 60); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/60/reviews"))).toBe(true); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 60)).toEqual([expect.objectContaining({ reviewerLogin: "maintainer", state: "APPROVED" })]); + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 60)).toMatchObject({ status: "complete" }); + }); + + it("does not re-fetch reviews when reviewsSyncedAt is set and no invalidation has been recorded (cache hit), and leaves stored rows untouched", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 61, + title: "Open PR, reviews already synced", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-61" }, + labels: [], + body: "", + }); + await upsertPullRequestReview(env, { + id: "JSONbored/gittensory#61#1", + repoFullName: "JSONbored/gittensory", + pullNumber: 61, + reviewerLogin: "maintainer", + state: "APPROVED", + authorAssociation: "OWNER", + submittedAt: "2026-05-19T00:00:00.000Z", + payload: { id: 1 }, + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 61, + status: "complete", + headSha: "head-61", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => (url.includes("/reviews") ? new Response("must not be called", { status: 500 }) : Response.json([]))); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 61); + + expect(result).toMatchObject({ status: "complete", warnings: [] }); + expect(urls.some((url) => url.includes("/pulls/61/reviews"))).toBe(false); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 61)).toEqual([expect.objectContaining({ reviewerLogin: "maintainer", state: "APPROVED" })]); + }); + + it("does not re-fetch reviews when reviewsInvalidatedAt predates reviewsSyncedAt (stale invalidation, still a cache hit)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 62, + title: "Open PR, invalidation predates sync", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-62" }, + labels: [], + body: "", + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 62, + status: "complete", + headSha: "head-62", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + reviewsInvalidatedAt: "2026-05-19T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => (url.includes("/reviews") ? new Response("must not be called", { status: 500 }) : Response.json([]))); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 62); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/62/reviews"))).toBe(false); + }); + + it("re-fetches reviews on the next sync after markPullRequestReviewsInvalidated bumps reviewsInvalidatedAt past reviewsSyncedAt (cache invalidation)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 63, + title: "Open PR, invalidated after sync", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-63" }, + labels: [], + body: "", + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 63, + status: "complete", + headSha: "head-63", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + + await markPullRequestReviewsInvalidated(env, "JSONbored/gittensory", 63); + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 63)).toMatchObject({ + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + status: "complete", + headSha: "head-63", + }); + + const urls = stubFetchTracking((url) => + url.includes("/pulls/63/reviews") + ? Response.json([{ id: 2, user: { login: "second-reviewer" }, state: "CHANGES_REQUESTED", author_association: "NONE", submitted_at: "2026-05-21T00:00:00.000Z" }]) + : Response.json([]), + ); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 63); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/63/reviews"))).toBe(true); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 63)).toEqual([expect.objectContaining({ reviewerLogin: "second-reviewer", state: "CHANGES_REQUESTED" })]); + }); + + it("REGRESSION: a prior FAILED review fetch does not poison the cache — the next sync retries reviews even though reviewsSyncedAt is already set", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 65, + title: "Open PR, review fetch failed last time", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-65" }, + labels: [], + body: "", + }); + // Simulates the state left behind by a run whose review fetch failed: reviewsSyncedAt IS stamped (every + // caller stamps it unconditionally), but errorSummary records the review-specific failure. + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 65, + status: "partial", + headSha: "head-65", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + errorSummary: "Review sync failed for #65: GitHub REST and GraphQL detail fetches failed.", + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/65/reviews") + ? Response.json([{ id: 3, user: { login: "late-reviewer" }, state: "APPROVED", author_association: "NONE", submitted_at: "2026-05-22T00:00:00.000Z" }]) + : Response.json([]), + ); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 65); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/65/reviews"))).toBe(true); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 65)).toEqual([expect.objectContaining({ reviewerLogin: "late-reviewer" })]); + }); + + it("does not treat a FILES-only failure as a reason to re-fetch reviews (only a review-specific failure forces a retry)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 66, + title: "Open PR, prior FILES failure only", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-66" }, + labels: [], + body: "", + }); + await upsertPullRequestReview(env, { + id: "JSONbored/gittensory#66#1", + repoFullName: "JSONbored/gittensory", + pullNumber: 66, + reviewerLogin: "maintainer", + state: "APPROVED", + authorAssociation: "OWNER", + submittedAt: "2026-05-19T00:00:00.000Z", + payload: { id: 1 }, + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 66, + status: "partial", + // headSha intentionally omitted/mismatched so files remain "not up to date" too — the point of this + // test is only that the FILES failure text in errorSummary must not be mistaken for a reviews failure. + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + errorSummary: "File sync failed for #66: GitHub REST and GraphQL detail fetches failed.", + }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/66/reviews") ? new Response("must not be called", { status: 500 }) : Response.json([]))); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 66); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/66/reviews"))).toBe(false); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 66)).toEqual([expect.objectContaining({ reviewerLogin: "maintainer" })]); + }); + + it("REGRESSION: a head SHA change alone does not invalidate cached reviews (reviews are independent of the head, unlike files)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 64, + title: "Open PR, new commit pushed", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-new" }, + labels: [], + body: "", + }); + await upsertPullRequestReview(env, { + id: "JSONbored/gittensory#64#1", + repoFullName: "JSONbored/gittensory", + pullNumber: 64, + reviewerLogin: "maintainer", + state: "APPROVED", + authorAssociation: "OWNER", + submittedAt: "2026-05-19T00:00:00.000Z", + payload: { id: 1 }, + }); + // Sync state was stamped for a DIFFERENT (older) head SHA — files caching would treat this as stale, but + // reviews caching must not, since reviews.reviewsSyncedAt has no head-SHA gate at all. + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 64, + status: "complete", + headSha: "head-old", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/64/files") + ? Response.json([{ filename: "src/new.ts", status: "added", additions: 3, deletions: 0, changes: 3 }]) + : url.includes("/reviews") + ? new Response("must not be called", { status: 500 }) + : Response.json([]), + ); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 64); + + expect(result).toMatchObject({ status: "complete" }); + // Files WERE refetched (head changed)... + expect(urls.some((url) => url.includes("/pulls/64/files"))).toBe(true); + // ...but reviews were NOT — the core distinction from the files cache. + expect(urls.some((url) => url.includes("/pulls/64/reviews"))).toBe(false); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 64)).toEqual([expect.objectContaining({ reviewerLogin: "maintainer", state: "APPROVED" })]); + }); + + describe("markPullRequestReviewsInvalidated", () => { + it("creates a sync-state row if none exists yet", async () => { + const env = createTestEnv(); + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 70)).toBeNull(); + + await markPullRequestReviewsInvalidated(env, "JSONbored/gittensory", 70); + + const state = await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 70); + expect(state).not.toBeNull(); + expect(state?.reviewsInvalidatedAt).toBeTruthy(); + }); + + it("updates ONLY reviewsInvalidatedAt when a row already exists, leaving filesSyncedAt/reviewsSyncedAt/checksSyncedAt/headSha unchanged", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 71, + status: "complete", + headSha: "sha-preserved", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + checksSyncedAt: "2026-05-20T00:00:00.000Z", + errorSummary: "prior warning", + }); + + await markPullRequestReviewsInvalidated(env, "JSONbored/gittensory", 71); + + const state = await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 71); + expect(state).toMatchObject({ + status: "complete", + headSha: "sha-preserved", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + checksSyncedAt: "2026-05-20T00:00:00.000Z", + errorSummary: "prior warning", + }); + expect(state?.reviewsInvalidatedAt).toBeTruthy(); + expect(state?.reviewsInvalidatedAt).not.toBe("2026-05-20T00:00:00.000Z"); + }); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index c5ccf7b9d2..6b29f55f83 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -18,6 +18,7 @@ import { getInstallation, getLatestUpstreamRulesetSnapshot, getPullRequest, + getPullRequestDetailSyncState, getRepository, listUpstreamDriftReports, listInstallationHealth, @@ -12413,6 +12414,100 @@ describe("queue processors", () => { expect(permissionCalls).toEqual([]); }); + it.each(["submitted", "dismissed", "edited"] as const)( + "bumps reviewsInvalidatedAt for the right repo+PR on a pull_request_review '%s' webhook (#2537)", + async (action) => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + JOBS: { async send() {} } as unknown as Queue, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + return new Response("not found", { status: 404 }); + }); + // Seed an existing sync-state row so the assertion can confirm ONLY reviewsInvalidatedAt moved. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 42, + title: "Add feature", + state: "open", + user: { login: "contributor" }, + head: { sha: "sha-42" }, + labels: [], + body: "", + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: `review-invalidate-${action}`, + eventName: "pull_request_review", + payload: { + action, + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 42, + title: "Add feature", + state: "open", + user: { login: "contributor", type: "User" }, + html_url: "https://github.com/JSONbored/gittensory/pull/42", + }, + review: { + state: action === "dismissed" ? "DISMISSED" : "APPROVED", + user: { login: "maintainer", type: "User" }, + submitted_at: "2026-05-28T12:00:00.000Z", + html_url: "https://github.com/JSONbored/gittensory/pull/42#pullrequestreview-1", + }, + sender: { login: "maintainer", type: "User" }, + }, + }); + + const state = await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 42); + expect(state?.reviewsInvalidatedAt).toBeTruthy(); + }, + ); + + it("does not bump reviewsInvalidatedAt for a pull_request_review action outside submitted/dismissed/edited", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + JOBS: { async send() {} } as unknown as Queue, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "review-invalidate-unsupported-action", + eventName: "pull_request_review", + payload: { + // "submitted" | "dismissed" | "edited" are the only invalidating actions; GitHub also emits others + // (e.g. review comments carry their own event) that must NOT stamp the cache marker. + action: "unrecognized_action" as unknown as "submitted", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 43, + title: "Add feature", + state: "open", + user: { login: "contributor", type: "User" }, + html_url: "https://github.com/JSONbored/gittensory/pull/43", + }, + review: { + state: "APPROVED", + user: { login: "maintainer", type: "User" }, + submitted_at: "2026-05-28T12:00:00.000Z", + html_url: "https://github.com/JSONbored/gittensory/pull/43#pullrequestreview-1", + }, + sender: { login: "maintainer", type: "User" }, + }, + }); + + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 43)).toBeNull(); + }); + it("notifies issue-watchers when a new grabbable maintainer-created issue opens (#699 path B)", async () => { const enqueued: Array<{ type: string; event?: { eventType: string; recipientLogin: string; pullNumber: number } }> = []; const env = createTestEnv({ JOBS: { async send(message: { type: string }) { enqueued.push(message); } } as unknown as Queue }); From 8dbb0e5cd9d0b0687518c5ad2ad3e8335f2fa2ee Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 14:40:19 -0700 Subject: [PATCH 2/4] fix(github): stop forceFiles from bleeding into the reviews cache decision Gate review finding on PR #2633: the sync-state row lookup was skipped entirely whenever forceFiles && headSha (the manual "refresh files" path), which zeroed out reviewsUpToDate too and forced an unrelated GET /pulls/{n}/reviews on every manual files-only force, even when reviews were already cache-current. forceFiles only ever means "force a files refetch" -- reviews caching must be completely independent of it. Now fetches the row unconditionally and applies forceFiles only to filesUpToDate, never to reviewsUpToDate. --- src/github/backfill.ts | 20 ++++--- .../backfill-reviews-cache-scoping.test.ts | 52 +++++++++++++++++++ 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 5e06884e12..64f1473c71 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -1984,17 +1984,15 @@ async function fetchAndStorePullRequestDetails( // instead of refetching when the last successful files sync already covered the PR's CURRENT head SHA — // only files are cached here; checks are more volatile at a fixed head and still refresh every call. // - // The lookup below is gated by `!options.forceFiles || !pr.headSha` rather than by the files-only - // `!options.forceFiles && pr.headSha` condition the ORIGINAL (files-only) cache used. Reviews caching - // (#2537) reuses this SAME row (rather than adding a second DB read) but does not depend on `headSha` - // at all and is never controlled by `forceFiles` (that flag only ever forces a FILES re-fetch — see its - // name and its only caller, refreshPullRequestDetails's manual "force" option). If the lookup stayed - // gated on `pr.headSha` being present, a PR with a momentarily-empty head SHA would silently lose review - // caching too, even though reviews never needed a head SHA to begin with. So: skip the read only in the - // one case where NEITHER cache can use it (forceFiles is set AND headSha is present, i.e. the files-only - // force path) — every other combination still fetches the row so reviewsUpToDate can be computed. - const existingState = options.forceFiles && pr.headSha ? null : await getPullRequestDetailSyncState(env, repoFullName, pr.number); - const filesUpToDate = Boolean(existingState?.headSha) && existingState?.headSha === pr.headSha && Boolean(existingState?.filesSyncedAt); + // The row is now fetched UNCONDITIONALLY (the original files-only cache gated this on `!options.forceFiles`, + // skipping the read entirely on a forced refresh) because reviews caching (#2537) reuses this SAME row and + // does not depend on `headSha` or `forceFiles` at all -- `forceFiles` only ever forces a FILES re-fetch (see + // its name and its only caller, refreshPullRequestDetails's manual "force" option), so a caller asking to + // force-refresh files must not ALSO force an unrelated reviews refetch (gate review finding: the previous + // version skipped the row entirely on `forceFiles && headSha`, which zeroed out `reviewsUpToDate` too). + // `forceFiles` is applied ONLY to `filesUpToDate` below, never to `reviewsUpToDate`. + const existingState = await getPullRequestDetailSyncState(env, repoFullName, pr.number); + const filesUpToDate = !options.forceFiles && Boolean(existingState?.headSha) && existingState?.headSha === pr.headSha && Boolean(existingState?.filesSyncedAt); // Reviews cache (#2537): independent of headSha — a new commit alone does not invalidate existing review // state, only an actual `pull_request_review` webhook (submitted/dismissed/edited) does, via // markPullRequestReviewsInvalidated. Up to date when a prior sync recorded reviewsSyncedAt and either no diff --git a/test/unit/backfill-reviews-cache-scoping.test.ts b/test/unit/backfill-reviews-cache-scoping.test.ts index 3ba8640901..ecfee55a9d 100644 --- a/test/unit/backfill-reviews-cache-scoping.test.ts +++ b/test/unit/backfill-reviews-cache-scoping.test.ts @@ -300,6 +300,58 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { expect(await listPullRequestReviews(env, "JSONbored/gittensory", 64)).toEqual([expect.objectContaining({ reviewerLogin: "maintainer", state: "APPROVED" })]); }); + it("REGRESSION (gate finding): a manual force-files refresh does not also force an unrelated reviews refetch", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 67, + title: "Open PR, manual force-files refresh", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-67" }, + labels: [], + body: "", + }); + await upsertPullRequestReview(env, { + id: "JSONbored/gittensory#67#1", + repoFullName: "JSONbored/gittensory", + pullNumber: 67, + reviewerLogin: "maintainer", + state: "APPROVED", + authorAssociation: "OWNER", + submittedAt: "2026-05-19T00:00:00.000Z", + payload: { id: 1 }, + }); + // Same head SHA + a fresh reviewsSyncedAt — reviews ARE cache-current; `force: true` must only re-fetch + // files (its own documented purpose), never reviews (an earlier version of this cache accidentally + // skipped the whole sync-state row lookup whenever `forceFiles && headSha`, which zeroed out + // `reviewsUpToDate` too and forced an unrelated reviews refetch on every manual files-only force). + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 67, + status: "complete", + headSha: "head-67", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/67/files") + ? Response.json([{ filename: "src/refreshed.ts", status: "modified", additions: 1, deletions: 1, changes: 2 }]) + : url.includes("/reviews") + ? new Response("must not be called", { status: 500 }) + : Response.json([]), + ); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 67, { force: true }); + + expect(result).toMatchObject({ status: "complete" }); + // Files WERE refetched (force: true)... + expect(urls.some((url) => url.includes("/pulls/67/files"))).toBe(true); + // ...but reviews were NOT — forceFiles must never bleed into the (unrelated) reviews cache decision. + expect(urls.some((url) => url.includes("/pulls/67/reviews"))).toBe(false); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 67)).toEqual([expect.objectContaining({ reviewerLogin: "maintainer", state: "APPROVED" })]); + }); + describe("markPullRequestReviewsInvalidated", () => { it("creates a sync-state row if none exists yet", async () => { const env = createTestEnv(); From 23f10f6a827c5dcb77c7479b5055887ecff5d09b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 14:57:02 -0700 Subject: [PATCH 3/4] fix(github): close the reviews-cache TOCTOU race and harden invalidation Second round of gate findings on PR #2633: 1. fetchAndStorePullRequestDetails read existingState as a snapshot at the top of the call, but every caller then unconditionally stamped reviewsSyncedAt to "now" once the whole call finished. A pull_request_review webhook racing in after the snapshot read but before that final write would set reviewsInvalidatedAt to a moment the stamped reviewsSyncedAt would then read as already covering -- the cache could confirm itself fresh through an invalidation it never actually observed. Now returns the correct reviewsSyncedAt to the caller instead: captured BEFORE the fetch starts on a genuine success (conservative against races from that instant on), and left unchanged on a skip or a failed fetch. This also makes a stored reviewsSyncedAt trustworthy on its own, so the separate errorSummary string-matching safety net (fragile: it only ever reflects whichever of files/reviews/ checks failed LAST in a given pass, since they run concurrently) is no longer needed and has been removed. reviewsUpToDate's comparison is also now a strict `>` rather than `>=`, so a millisecond-resolution timestamp tie between a sync and a racing invalidation fails toward "still needs a refetch" rather than silently trusting the cache. 2. markPullRequestReviewsInvalidated is the sole source of the invalidation signal, so a single transient D1 write failure would permanently lose that PR's "reviews changed" event. Added a bounded 3-attempt retry -- still best-effort from the webhook handler's perspective (never blocks/retries the whole job), but absorbs a momentary blip in-process. --- src/db/repositories.ts | 53 ++++--- src/github/backfill.ts | 52 ++++--- .../backfill-reviews-cache-scoping.test.ts | 139 ++++++++++++++++-- 3 files changed, 188 insertions(+), 56 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index e0ad6fc5a9..6126a0b420 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -1178,27 +1178,44 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ * every other column (`headSha`/`filesSyncedAt`/`reviewsSyncedAt`/`checksSyncedAt`/...) untouched when the * row already exists — mirrors the narrow single-field touches on `pull_requests` (markPullRequestApproved, * markPullRequestRegated). Creates the row (all other columns default/NULL) if this repo+PR has never been - * synced yet, so an early review webhook is not silently dropped. A plain D1 write, independent of headSha. */ + * synced yet, so an early review webhook is not silently dropped. + * + * Unlike its siblings above (advisory/reporting markers with other fallback signals), this write is the SOLE + * source of the reviews-cache invalidation signal (#2537 gate finding) — a single failed attempt loses that + * PR's specific "reviews changed" event permanently, with nothing to naturally re-trigger it until some LATER + * invalidation happens to succeed. The caller already treats this as best-effort (never blocks the webhook), + * so a short bounded retry absorbs a transient D1 blip in-process rather than needing a durable retry queue + * for what is still, even after this, a best-effort write. */ export async function markPullRequestReviewsInvalidated(env: Env, repoFullName: string, pullNumber: number): Promise { const db = getDb(env.DB); const now = nowIso(); - await db - .insert(pullRequestDetailSyncState) - .values({ - id: `${repoFullName}#${pullNumber}`, - repoFullName, - pullNumber, - status: "never_synced", - reviewsInvalidatedAt: now, - updatedAt: now, - }) - .onConflictDoUpdate({ - target: [pullRequestDetailSyncState.repoFullName, pullRequestDetailSyncState.pullNumber], - set: { - reviewsInvalidatedAt: now, - updatedAt: now, - }, - }); + const MAX_ATTEMPTS = 3; + let lastError: unknown; + for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt += 1) { + try { + await db + .insert(pullRequestDetailSyncState) + .values({ + id: `${repoFullName}#${pullNumber}`, + repoFullName, + pullNumber, + status: "never_synced", + reviewsInvalidatedAt: now, + updatedAt: now, + }) + .onConflictDoUpdate({ + target: [pullRequestDetailSyncState.repoFullName, pullRequestDetailSyncState.pullNumber], + set: { + reviewsInvalidatedAt: now, + updatedAt: now, + }, + }); + return; + } catch (error) { + lastError = error; + } + } + throw lastError; } export async function getPullRequestDetailSyncState(env: Env, fullName: string, pullNumber: number): Promise { diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 64f1473c71..07cc777742 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -613,7 +613,7 @@ export async function backfillOpenPullRequestDetails( await mapWithConcurrency(batch, 2, async (pr) => { await upsertPullRequestDetailSyncState(env, { repoFullName: repo.fullName, pullNumber: pr.number, status: "running" }); const before = warnings.length; - await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey, "backfill_open_pr_details"); + const { reviewsSyncedAt } = await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey, "backfill_open_pr_details"); const syncedAt = nowIso(); const newWarnings = warnings.slice(before); await upsertPullRequestDetailSyncState(env, { @@ -622,7 +622,7 @@ export async function backfillOpenPullRequestDetails( status: newWarnings.length > 0 ? "partial" : "complete", headSha: pr.headSha, filesSyncedAt: syncedAt, - reviewsSyncedAt: syncedAt, + reviewsSyncedAt, checksSyncedAt: syncedAt, lastSyncedAt: syncedAt, errorSummary: newWarnings.at(-1), @@ -689,7 +689,7 @@ export async function refreshPullRequestDetails( const admissionKey = repoAdmissionKeyForToken(env, repo, token); const warnings: string[] = []; await upsertPullRequestDetailSyncState(env, { repoFullName, pullNumber, status: "running" }); - await fetchAndStorePullRequestDetails(env, repoFullName, pr, token, warnings, admissionKey, "live_review", { forceFiles: options.force }); + const { reviewsSyncedAt } = await fetchAndStorePullRequestDetails(env, repoFullName, pr, token, warnings, admissionKey, "live_review", { forceFiles: options.force }); const syncedAt = nowIso(); const status: PullRequestDetailSyncStateRecord["status"] = warnings.length > 0 ? "partial" : "complete"; await upsertPullRequestDetailSyncState(env, { @@ -698,7 +698,7 @@ export async function refreshPullRequestDetails( status, headSha: pr.headSha, filesSyncedAt: syncedAt, - reviewsSyncedAt: syncedAt, + reviewsSyncedAt, checksSyncedAt: syncedAt, lastSyncedAt: syncedAt, errorSummary: warnings.at(-1), @@ -1807,7 +1807,7 @@ async function backfillRepository(env: Env, repo: RepositoryRecord, limits: Back const detailWarningStart = warnings.length; await mapWithConcurrency(detailTargets, limits.detailConcurrency, async (pr) => { const before = warnings.length; - await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey, "backfill_open_pr_details"); + const { reviewsSyncedAt } = await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey, "backfill_open_pr_details"); // Persist the repo+PR+headSha snapshot marker (#audit-rate-headroom) so a later call through ANY // cache-aware path (open-PR convergence, live review) can skip refetching this PR's files while its // head is unchanged — without this write, fetchAndStorePullRequestDetails's cache check always misses @@ -1820,7 +1820,7 @@ async function backfillRepository(env: Env, repo: RepositoryRecord, limits: Back status: newWarnings.length > 0 ? "partial" : "complete", headSha: pr.headSha, filesSyncedAt: syncedAt, - reviewsSyncedAt: syncedAt, + reviewsSyncedAt, checksSyncedAt: syncedAt, lastSyncedAt: syncedAt, errorSummary: newWarnings.at(-1), @@ -1978,7 +1978,7 @@ async function fetchAndStorePullRequestDetails( admissionKey: GitHubRateLimitAdmissionKey | undefined, caller: PullRequestFilesFetchCaller, options: { forceFiles?: boolean | undefined } = {}, -): Promise { +): Promise<{ reviewsSyncedAt: string | null | undefined }> { // Durable repo+PR+headSha file snapshot (#audit-rate-headroom): a bare URL cache is insufficient because // `/pulls/{n}/files` has the SAME url across different heads. Reuse the stored `pull_request_files` rows // instead of refetching when the last successful files sync already covered the PR's CURRENT head SHA — @@ -1996,22 +1996,22 @@ async function fetchAndStorePullRequestDetails( // Reviews cache (#2537): independent of headSha — a new commit alone does not invalidate existing review // state, only an actual `pull_request_review` webhook (submitted/dismissed/edited) does, via // markPullRequestReviewsInvalidated. Up to date when a prior sync recorded reviewsSyncedAt and either no - // invalidation has been recorded since, or the invalidation predates that sync. - // - // Every caller of this function stamps reviewsSyncedAt UNCONDITIONALLY once fetchAndStorePullRequestDetails - // returns, even when the reviews fetch itself failed (there is no per-segment success timestamp, only the - // aggregate PR-level errorSummary/status). Trusting a bare reviewsSyncedAt presence alone would let a - // transient review-fetch failure poison the cache forever, so this also excludes the case where the row's - // OWN errorSummary is the review-sync failure fetchPullRequestReviews just recorded for THIS PR (the same - // `Review sync failed for #` message every caller already greps for — see the /Review sync failed/i - // filters in backfillRepositorySegment). A files/checks-only failure still leaves reviews cached, matching - // the reviews-are-independent-of-files intent; only a review-specific failure forces a retry. - const reviewsSyncedAt = existingState?.reviewsSyncedAt; - const reviewsFetchPreviouslyFailed = Boolean(existingState?.errorSummary?.startsWith(`Review sync failed for #${pr.number}:`)); + // invalidation has been recorded since, or the invalidation predates that sync. STRICTLY greater-than (not + // >=): millisecond-resolution ISO timestamps can tie when a sync and a racing invalidation land in the same + // millisecond, and sub-millisecond ordering is unknowable from the stored strings — a tie must fail toward + // "still needs a refetch," never toward silently trusting a possibly-stale cache. + const reviewsSyncedAtBefore = existingState?.reviewsSyncedAt; const reviewsUpToDate = - Boolean(reviewsSyncedAt) && - !reviewsFetchPreviouslyFailed && - (!existingState?.reviewsInvalidatedAt || (reviewsSyncedAt ?? "") >= existingState.reviewsInvalidatedAt); + Boolean(reviewsSyncedAtBefore) && + (!existingState?.reviewsInvalidatedAt || (reviewsSyncedAtBefore ?? "") > existingState.reviewsInvalidatedAt); + // Gate review finding (TOCTOU race): `existingState` above is a snapshot read at the TOP of this call. If a + // `pull_request_review` webhook races in AFTER that read but BEFORE this function returns, an unconditional + // "stamp reviewsSyncedAt to now" on the CALLER's side (the old design) would advance the timestamp PAST that + // webhook's invalidation without ever having fetched the reviews it invalidated -- the cache would then + // permanently believe it's fresh through an event it never actually observed. Captured HERE, before the + // fetch even starts, so it's safe: any invalidation racing in from this instant onward still leaves + // `reviewsInvalidatedAt` newer than whatever we return below, forcing a correct refetch on the NEXT pass. + const reviewFetchStartedAt = nowIso(); const warningStart = warnings.length; const [files, reviews, checks] = await Promise.all([ filesUpToDate ? Promise.resolve([]) : fetchPullRequestFiles(env, repoFullName, pr.number, token, warnings, admissionKey, caller), @@ -2019,6 +2019,13 @@ async function fetchAndStorePullRequestDetails( fetchPullRequestChecks(env, repoFullName, pr, token, warnings, admissionKey), ]); const fileSyncFailed = warnings.slice(warningStart).some((warning) => warning.startsWith(`File sync failed for #${pr.number}:`)); + // reviewsSyncedAt only ever ADVANCES on a genuine success in THIS call -- never on a cache-hit skip, and + // never on a failed fetch attempt. This is what makes a stored reviewsSyncedAt a trustworthy "last confirmed + // successful sync" marker on its own (no separate errorSummary string-matching needed: a failed or skipped + // pass simply preserves whatever was already known, which -- being unchanged -- correctly keeps comparing as + // stale against reviewsInvalidatedAt on the next pass until a real fetch actually succeeds). + const reviewSyncFailedThisCall = !reviewsUpToDate && warnings.slice(warningStart).some((warning) => warning.startsWith(`Review sync failed for #${pr.number}:`)); + const reviewsSyncedAtResult = !reviewsUpToDate && !reviewSyncFailedThisCall ? reviewFetchStartedAt : reviewsSyncedAtBefore; if (!filesUpToDate && !fileSyncFailed) { await deletePullRequestFiles(env, repoFullName, pr.number); @@ -2063,6 +2070,7 @@ async function fetchAndStorePullRequestDetails( payload: check as unknown as Record, }); } + return { reviewsSyncedAt: reviewsSyncedAtResult }; } // GitHub caps list endpoints at 100 items/page, so a single `per_page=100` fetch silently truncates a diff --git a/test/unit/backfill-reviews-cache-scoping.test.ts b/test/unit/backfill-reviews-cache-scoping.test.ts index ecfee55a9d..22a30adcc5 100644 --- a/test/unit/backfill-reviews-cache-scoping.test.ts +++ b/test/unit/backfill-reviews-cache-scoping.test.ts @@ -175,39 +175,104 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { expect(await listPullRequestReviews(env, "JSONbored/gittensory", 63)).toEqual([expect.objectContaining({ reviewerLogin: "second-reviewer", state: "CHANGES_REQUESTED" })]); }); - it("REGRESSION: a prior FAILED review fetch does not poison the cache — the next sync retries reviews even though reviewsSyncedAt is already set", async () => { + it("REGRESSION (gate finding): a FAILED review fetch never advances reviewsSyncedAt, so the next sync retries instead of trusting a false cache hit", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); await seedRegisteredRepo(env); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 65, - title: "Open PR, review fetch failed last time", + title: "Open PR, review fetch fails on first sync", state: "open", user: { login: "oktofeesh1" }, head: { sha: "head-65" }, labels: [], body: "", }); - // Simulates the state left behind by a run whose review fetch failed: reviewsSyncedAt IS stamped (every - // caller stamps it unconditionally), but errorSummary records the review-specific failure. - await upsertPullRequestDetailSyncState(env, { - repoFullName: "JSONbored/gittensory", - pullNumber: 65, - status: "partial", - headSha: "head-65", - reviewsSyncedAt: "2026-05-20T00:00:00.000Z", - errorSummary: "Review sync failed for #65: GitHub REST and GraphQL detail fetches failed.", - }); - const urls = stubFetchTracking((url) => + // First pass: reviews REST + GraphQL fallback both fail (mirrors backfill.test.ts's "review failure, 503" + // stub — any unstubbed URL, including the GraphQL fallback, falls through to a 404). + const firstPassUrls = stubFetchTracking((url) => (url.includes("/pulls/65/reviews") ? new Response("review failure", { status: 503 }) : Response.json([]))); + + const firstResult = await refreshPullRequestDetails(env, "JSONbored/gittensory", 65); + + expect(firstResult.status).toBe("partial"); + expect(firstPassUrls.some((url) => url.includes("/pulls/65/reviews"))).toBe(true); + // The FAILED attempt must NOT advance reviewsSyncedAt — a stored value here (as the pre-fix code produced, + // stamping it unconditionally regardless of success) would let the next pass wrongly treat the failed + // fetch as a valid cache hit and never retry. + expect((await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 65))?.reviewsSyncedAt).toBeFalsy(); + + // Second pass: reviews now succeed — since reviewsSyncedAt is still unset, this MUST be treated as a cache + // miss and genuinely refetched (not skipped). + const secondPassUrls = stubFetchTracking((url) => url.includes("/pulls/65/reviews") ? Response.json([{ id: 3, user: { login: "late-reviewer" }, state: "APPROVED", author_association: "NONE", submitted_at: "2026-05-22T00:00:00.000Z" }]) : Response.json([]), ); - const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 65); + const secondResult = await refreshPullRequestDetails(env, "JSONbored/gittensory", 65); - expect(result).toMatchObject({ status: "complete" }); - expect(urls.some((url) => url.includes("/pulls/65/reviews"))).toBe(true); + expect(secondResult.status).toBe("complete"); + expect(secondPassUrls.some((url) => url.includes("/pulls/65/reviews"))).toBe(true); expect(await listPullRequestReviews(env, "JSONbored/gittensory", 65)).toEqual([expect.objectContaining({ reviewerLogin: "late-reviewer" })]); + // The now-successful sync DOES advance reviewsSyncedAt. + expect((await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 65))?.reviewsSyncedAt).toBeTruthy(); + }); + + it("REGRESSION (gate finding, TOCTOU race): a pull_request_review webhook racing in DURING a sync still forces a retry on the next pass", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 68, + title: "Open PR, invalidation races in mid-sync", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-68" }, + labels: [], + body: "", + }); + // No existing sync state — first-ever sync, so a real reviews fetch happens. The /reviews handler itself + // calls markPullRequestReviewsInvalidated mid-flight, simulating a `pull_request_review` webhook landing + // AFTER fetchAndStorePullRequestDetails already read `existingState` but BEFORE it (and the caller's final + // write) complete — exactly the race the gate flagged: a naive "stamp reviewsSyncedAt to now, once the + // whole call finishes" would land AFTER this invalidation and wrongly look like it already covers it. + let racingInvalidationDone = false; + const urls = stubFetchTracking(async (url) => { + if (url.includes("/pulls/68/reviews")) { + await markPullRequestReviewsInvalidated(env, "JSONbored/gittensory", 68); + racingInvalidationDone = true; + return Response.json([{ id: 1, user: { login: "reviewer" }, state: "APPROVED", author_association: "NONE", submitted_at: "2026-05-22T00:00:00.000Z" }]); + } + return Response.json([]); + }); + + await refreshPullRequestDetails(env, "JSONbored/gittensory", 68); + + expect(racingInvalidationDone).toBe(true); + expect(urls.some((url) => url.includes("/pulls/68/reviews"))).toBe(true); + const stateAfterRace = await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 68); + expect(stateAfterRace?.reviewsSyncedAt).toBeTruthy(); + expect(stateAfterRace?.reviewsInvalidatedAt).toBeTruthy(); + // The stored reviewsSyncedAt was captured BEFORE the fetch started (and therefore no later than the + // race). Millisecond-resolution timestamps can tie in a fast test run, so allow equality here — the + // production `reviewsUpToDate` check uses a STRICT `>` specifically so a tie still forces a refetch. + expect(stateAfterRace!.reviewsSyncedAt! <= stateAfterRace!.reviewsInvalidatedAt!).toBe(true); + + // A follow-up pass must therefore still see this as stale and genuinely refetch — not trust the sync that + // raced against (and missed) the invalidating event. + const followUpUrls = stubFetchTracking((url) => + url.includes("/pulls/68/reviews") + ? Response.json([ + { id: 1, user: { login: "reviewer" }, state: "APPROVED", author_association: "NONE", submitted_at: "2026-05-22T00:00:00.000Z" }, + { id: 2, user: { login: "second-reviewer" }, state: "CHANGES_REQUESTED", author_association: "NONE", submitted_at: "2026-05-23T00:00:00.000Z" }, + ]) + : Response.json([]), + ); + + await refreshPullRequestDetails(env, "JSONbored/gittensory", 68); + + expect(followUpUrls.some((url) => url.includes("/pulls/68/reviews"))).toBe(true); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 68)).toEqual( + expect.arrayContaining([expect.objectContaining({ reviewerLogin: "second-reviewer", state: "CHANGES_REQUESTED" })]), + ); }); it("does not treat a FILES-only failure as a reason to re-fetch reviews (only a review-specific failure forces a retry)", async () => { @@ -391,5 +456,47 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { expect(state?.reviewsInvalidatedAt).toBeTruthy(); expect(state?.reviewsInvalidatedAt).not.toBe("2026-05-20T00:00:00.000Z"); }); + + it("REGRESSION (gate finding): retries a transient D1 write failure instead of losing the sole invalidation signal", async () => { + const env = createTestEnv(); + const realPrepare = env.DB.prepare.bind(env.DB); + let calls = 0; + // Fail the first 2 attempts (a transient blip), succeed on the 3rd (within MAX_ATTEMPTS). + vi.spyOn(env.DB, "prepare").mockImplementation((sql: string) => { + calls += 1; + if (calls <= 2) { + return { + bind: () => ({ + run: () => Promise.reject(new Error("d1 transient error")), + all: () => Promise.reject(new Error("d1 transient error")), + first: () => Promise.reject(new Error("d1 transient error")), + }), + } as unknown as ReturnType; + } + return realPrepare(sql); + }); + + await markPullRequestReviewsInvalidated(env, "JSONbored/gittensory", 72); + + expect(calls).toBeGreaterThan(2); + const state = await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 72); + expect(state?.reviewsInvalidatedAt).toBeTruthy(); + }); + + it("REGRESSION (gate finding): still throws (bounded, not infinite) once every retry attempt fails", async () => { + const env = createTestEnv(); + vi.spyOn(env.DB, "prepare").mockImplementation( + () => + ({ + bind: () => ({ + run: () => Promise.reject(new Error("d1 permanently down")), + all: () => Promise.reject(new Error("d1 permanently down")), + first: () => Promise.reject(new Error("d1 permanently down")), + }), + }) as unknown as ReturnType, + ); + + await expect(markPullRequestReviewsInvalidated(env, "JSONbored/gittensory", 73)).rejects.toThrow(); + }); }); }); From 5042e609fd92bb05ab43c624a23e74f945d108e4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 15:15:13 -0700 Subject: [PATCH 4/4] fix(github): clean up legacy reviews_synced_at rows before trusting the cache Third gate finding on PR #2633: reviews_synced_at has existed since migration 0006, long before this PR gives it any cache-skip meaning -- every sync pass since then has stamped it unconditionally, including passes whose review fetch itself failed. On deploy, reviewsUpToDate would immediately trust every existing row's reviews_synced_at at face value, silently skipping re-fetches for PRs whose last review sync actually failed, until some later invalidating webhook happened to arrive. Migration 0094 now resets reviews_synced_at to NULL for every row whose last sync status was not 'complete' (status only reads 'complete' when that pass recorded zero warnings across files/reviews/checks, which reliably means reviews specifically succeeded -- any other status could have been a review failure, so it's reset to force a guaranteed-safe refetch). A genuinely 'complete' row is left untouched. Adds a migration-effect test (applying the actual 0094 SQL against a scratch table shaped like the pre-#2537 schema, seeded with rows exactly as years of pre-#2537 code would have written them) proving the cleanup targets the right rows and nothing else. --- .../0094_pull_request_reviews_invalidated.sql | 14 +++- .../backfill-reviews-cache-scoping.test.ts | 67 +++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/migrations/0094_pull_request_reviews_invalidated.sql b/migrations/0094_pull_request_reviews_invalidated.sql index 9666394ec0..12673eebfb 100644 --- a/migrations/0094_pull_request_reviews_invalidated.sql +++ b/migrations/0094_pull_request_reviews_invalidated.sql @@ -1,6 +1,16 @@ -- PR reviews cache invalidation marker (#2537): bumped by a `pull_request_review` webhook -- (submitted/dismissed/edited) to signal the cached `pull_request_reviews` rows are stale. NULL (the -- default) means no invalidating event has been recorded, so a subsequent fetchAndStorePullRequestDetails --- pass can skip the `GET /pulls/{n}/reviews` call when reviews_synced_at already covers it -- byte-identical --- behavior for every existing row. +-- pass can skip the `GET /pulls/{n}/reviews` call when reviews_synced_at already covers it. ALTER TABLE pull_request_detail_sync_state ADD COLUMN reviews_invalidated_at TEXT; + +-- One-time cleanup (gate review finding): reviews_synced_at has existed since migration 0006, long before +-- today it gains any cache-skip meaning -- every sync pass since then has stamped it UNCONDITIONALLY, +-- including passes whose review fetch itself failed (there was no per-segment success timestamp before this +-- PR). A `status != 'complete'` row is exactly the set where SOME segment failed on its last sync (`status` +-- only reads 'complete' when that pass recorded zero warnings across files/reviews/checks, which is a +-- reliable historical guarantee reviews specifically succeeded); for every other status the failure could +-- have been reviews, so trusting a stale reviews_synced_at there the moment this cache-skip logic goes live +-- would silently skip re-fetching reviews that were never actually captured. Reset ONLY the ambiguous rows; +-- a genuinely 'complete' row's reviews_synced_at is trustworthy as-is and is left untouched. +UPDATE pull_request_detail_sync_state SET reviews_synced_at = NULL WHERE status != 'complete' AND reviews_synced_at IS NOT NULL; diff --git a/test/unit/backfill-reviews-cache-scoping.test.ts b/test/unit/backfill-reviews-cache-scoping.test.ts index 22a30adcc5..067ec195e1 100644 --- a/test/unit/backfill-reviews-cache-scoping.test.ts +++ b/test/unit/backfill-reviews-cache-scoping.test.ts @@ -1,3 +1,7 @@ +import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { DatabaseSync } from "node:sqlite"; import { afterEach, describe, expect, it, vi } from "vitest"; import { getPullRequestDetailSyncState, @@ -12,6 +16,8 @@ import { clearGitHubResponseCacheForTest } from "../../src/github/client"; import { resetMetrics } from "../../src/selfhost/metrics"; import { normalizeRegistryPayload } from "../../src/registry/normalize"; import { persistRegistrySnapshot } from "../../src/registry/sync"; +import { createD1Adapter, nodeSqliteDriver } from "../../src/selfhost/d1-adapter"; +import { runSelfHostMigrations } from "../../src/selfhost/migrate"; import { createTestEnv } from "../helpers/d1"; describe("GitHub PR reviews cache scoping (#2537)", () => { @@ -499,4 +505,65 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { await expect(markPullRequestReviewsInvalidated(env, "JSONbored/gittensory", 73)).rejects.toThrow(); }); }); + + describe("migration 0094 legacy-row cleanup (gate review finding)", () => { + it("clears reviews_synced_at ONLY for rows whose last sync was not 'complete', leaving genuinely-complete rows untouched", async () => { + // Applies the REAL migration 0094 SQL (read straight off disk, not a hand-copied duplicate) against a + // scratch table shaped like the pre-#2537 schema (reviews_synced_at has existed since migration 0006, + // long before it gained any cache-skip meaning), seeded with rows exactly as years of pre-#2537 code + // would have unconditionally stamped reviews_synced_at regardless of whether that sync actually + // succeeded -- this is what a real production database looks like on the day this migration runs. + const dir = mkdtempSync(join(tmpdir(), "gtmig-reviews-")); + writeFileSync( + join(dir, "0001_base.sql"), + `CREATE TABLE pull_request_detail_sync_state ( + id TEXT PRIMARY KEY, + repo_full_name TEXT NOT NULL, + pull_number INTEGER NOT NULL, + status TEXT NOT NULL DEFAULT 'never_synced', + files_synced_at TEXT, + reviews_synced_at TEXT, + checks_synced_at TEXT, + last_synced_at TEXT, + error_summary TEXT, + updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP + );`, + ); + const db = createD1Adapter(nodeSqliteDriver(new DatabaseSync(":memory:") as never)); + await runSelfHostMigrations(db, dir); + + await db + .prepare( + "insert into pull_request_detail_sync_state (id, repo_full_name, pull_number, status, reviews_synced_at, error_summary, updated_at) values (?, ?, ?, ?, ?, ?, ?)", + ) + .bind("owner/repo#1", "owner/repo", 1, "complete", "2026-05-20T00:00:00.000Z", null, "2026-05-20T00:00:00.000Z") + .run(); + await db + .prepare( + "insert into pull_request_detail_sync_state (id, repo_full_name, pull_number, status, reviews_synced_at, error_summary, updated_at) values (?, ?, ?, ?, ?, ?, ?)", + ) + .bind("owner/repo#2", "owner/repo", 2, "partial", "2026-05-20T00:00:00.000Z", "Review sync failed for #2: GitHub REST and GraphQL detail fetches failed.", "2026-05-20T00:00:00.000Z") + .run(); + await db + .prepare( + "insert into pull_request_detail_sync_state (id, repo_full_name, pull_number, status, reviews_synced_at, error_summary, updated_at) values (?, ?, ?, ?, ?, ?, ?)", + ) + .bind("owner/repo#3", "owner/repo", 3, "partial", "2026-05-20T00:00:00.000Z", "File sync failed for #3: GitHub REST and GraphQL detail fetches failed.", "2026-05-20T00:00:00.000Z") + .run(); + + writeFileSync(join(dir, "0002_reviews_invalidated.sql"), readFileSync("migrations/0094_pull_request_reviews_invalidated.sql", "utf8")); + await runSelfHostMigrations(db, dir); + + const rows = (await db.prepare("select id, status, reviews_synced_at from pull_request_detail_sync_state order by pull_number").all()).results as Array<{ + id: string; + status: string; + reviews_synced_at: string | null; + }>; + expect(rows).toEqual([ + { id: "owner/repo#1", status: "complete", reviews_synced_at: "2026-05-20T00:00:00.000Z" }, // untouched + { id: "owner/repo#2", status: "partial", reviews_synced_at: null }, // reset — reviews specifically failed + { id: "owner/repo#3", status: "partial", reviews_synced_at: null }, // reset — ambiguous (files failed, reviews unverifiable) + ]); + }); + }); });