diff --git a/migrations/0090_pull_request_detail_sync_head_sha.sql b/migrations/0090_pull_request_detail_sync_head_sha.sql new file mode 100644 index 0000000000..66695fbd95 --- /dev/null +++ b/migrations/0090_pull_request_detail_sync_head_sha.sql @@ -0,0 +1,2 @@ +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN head_sha TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index a53dc2b73a..dbf347b183 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -1098,6 +1098,12 @@ export async function getRepoQueueTrendSnapshot(env: Env, repoFullName: string): return row ? toRepoQueueTrendSnapshotRecord(row) : null; } +// PARTIAL-UPDATE CONTRACT: an omitted (`undefined`) field on `state` leaves that column UNCHANGED on conflict — +// drizzle's `onConflictDoUpdate` strips `undefined` entries from the generated SQL `SET` clause rather than +// writing NULL. Every "running" pre-fetch stamp (backfill.ts) relies on this to touch only `status` without +// clearing the PREVIOUS `headSha`/`*SyncedAt` row — including the repo+PR+headSha file cache +// (#audit-rate-headroom), which would silently stop hitting if a future edit here coalesced an omitted field to +// `null` (e.g. `headSha: state.headSha ?? null`). Pass `null` explicitly to actually clear a column. export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequestDetailSyncStateRecord): Promise { const db = getDb(env.DB); await db @@ -1107,6 +1113,7 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ repoFullName: state.repoFullName, pullNumber: state.pullNumber, status: state.status, + headSha: state.headSha, filesSyncedAt: state.filesSyncedAt, reviewsSyncedAt: state.reviewsSyncedAt, checksSyncedAt: state.checksSyncedAt, @@ -1118,6 +1125,7 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ target: [pullRequestDetailSyncState.repoFullName, pullRequestDetailSyncState.pullNumber], set: { status: state.status, + headSha: state.headSha, filesSyncedAt: state.filesSyncedAt, reviewsSyncedAt: state.reviewsSyncedAt, checksSyncedAt: state.checksSyncedAt, @@ -1128,6 +1136,16 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ }); } +export async function getPullRequestDetailSyncState(env: Env, fullName: string, pullNumber: number): Promise { + const db = getDb(env.DB); + const [row] = await db + .select() + .from(pullRequestDetailSyncState) + .where(and(eq(pullRequestDetailSyncState.repoFullName, fullName), eq(pullRequestDetailSyncState.pullNumber, pullNumber))) + .limit(1); + return row ? toPullRequestDetailSyncStateRecord(row) : null; +} + export async function listPullRequestDetailSyncStates(env: Env, fullName: string): Promise { const db = getDb(env.DB); const rows = await db.select().from(pullRequestDetailSyncState).where(eq(pullRequestDetailSyncState.repoFullName, fullName)).limit(2000); @@ -4147,6 +4165,7 @@ function toPullRequestDetailSyncStateRecord(row: typeof pullRequestDetailSyncSta repoFullName: row.repoFullName, pullNumber: row.pullNumber, status: parsePullRequestDetailSyncStatus(row.status), + headSha: row.headSha, filesSyncedAt: row.filesSyncedAt, reviewsSyncedAt: row.reviewsSyncedAt, checksSyncedAt: row.checksSyncedAt, diff --git a/src/db/schema.ts b/src/db/schema.ts index a89c006f5d..6321999f9e 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -215,6 +215,9 @@ export const pullRequestDetailSyncState = sqliteTable( repoFullName: text("repo_full_name").notNull(), pullNumber: integer("pull_number").notNull(), status: text("status").notNull().default("never_synced"), + // The head SHA the FILES were last synced for (not the review/checks SHA) — lets a caller skip a + // `/pulls/{n}/files` refetch when the PR's current head still matches what is already stored (#audit-rate-headroom). + headSha: text("head_sha"), filesSyncedAt: text("files_synced_at"), reviewsSyncedAt: text("reviews_synced_at"), checksSyncedAt: text("checks_synced_at"), diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 668f38148f..5beb5dbf3e 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -8,6 +8,7 @@ import { deletePullRequestFiles, countRepoLabels, getInstallation, + getPullRequestDetailSyncState, listRepoGithubTotalsSnapshotHistory, getRepoSyncSegment, getRepoSyncState, @@ -67,7 +68,7 @@ import { GITTENSORY_LEGACY_GATE_CHECK_NAME, } from "../review/check-names"; import { buildReviewThreadBlocker, type ReviewThreadBlocker } from "../review/review-thread-findings"; -import { delayUntil, shouldWaitForGitHubRateLimit } from "./rate-limit"; +import { delayUntil, HISTORICAL_BACKFILL_RESERVED_HEADROOM, shouldWaitForGitHubRateLimit } from "./rate-limit"; import { githubRateLimitAdmissionKeyForPublicToken, githubRateLimitAdmissionKeyForToken, @@ -76,6 +77,7 @@ import { type GitHubRateLimitAdmissionKey, } from "./client"; import { fetchCachedGitHubGraphQl } from "./graphql-cache"; +import { incr } from "../selfhost/metrics"; type GitHubLabelPayload = { name: string; color?: string; @@ -316,6 +318,14 @@ const FRESH_SYNC_MS = 6 * 60 * 60 * 1000; const ERROR_BACKOFF_MS = 60 * 60 * 1000; const SEGMENT_PAGE_BUDGET: Record = { light: 2, full: 10, resume: 10 }; const PR_DETAIL_BATCH_SIZE: Record = { light: 12, full: 40, resume: 40 }; +// Caps how many NOT-yet-hydrated merged PRs get a `/pulls/{n}/files` fetch per `recent_merged_pull_requests` +// page (independent of SEGMENT_PAGE_BUDGET, which only caps LIST pages). Without this a repo with a large +// un-hydrated merged-PR backlog can fan out one files fetch per PR across up to SEGMENT_PAGE_BUDGET * 100 PRs +// in a single job execution, draining the shared installation bucket before the once-per-segment rate check +// runs again (#audit-rate-headroom). Any PR left un-hydrated this run stays a candidate on the next page/run. +const MERGED_PR_FILE_HYDRATION_BATCH_SIZE: Record = { light: 10, full: 20, resume: 20 }; +const PULL_REQUEST_FILES_FETCH_METRIC = "gittensory_github_pull_request_files_fetch_total"; +type PullRequestFilesFetchCaller = "backfill_open_pr_details" | "backfill_merged_history" | "live_review"; const CURRENT_OPEN_SCAN_MARKER = "gittensory-current-open-scan-v1"; const FRESH_TOTALS_SNAPSHOT_MS = 10 * 60 * 1000; const TOTALS_SNAPSHOT_LOOKBACK = 8; @@ -603,13 +613,14 @@ 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); + await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey, "backfill_open_pr_details"); const syncedAt = nowIso(); const newWarnings = warnings.slice(before); await upsertPullRequestDetailSyncState(env, { repoFullName: repo.fullName, pullNumber: pr.number, status: newWarnings.length > 0 ? "partial" : "complete", + headSha: pr.headSha, filesSyncedAt: syncedAt, reviewsSyncedAt: syncedAt, checksSyncedAt: syncedAt, @@ -659,22 +670,33 @@ export async function refreshPullRequestDetails( env: Env, repoFullName: string, pullNumber: number, + options: { force?: boolean } = {}, ): Promise<{ ok: true; repoFullName: string; pullNumber: number; status: PullRequestDetailSyncStateRecord["status"]; warnings: string[] }> { const [repo, pr] = await Promise.all([getRepository(env, repoFullName), getPullRequest(env, repoFullName, pullNumber)]); if (!repo || !pr) { return { ok: true, repoFullName, pullNumber, status: "partial", warnings: ["Repository or pull request was not found."] }; } + // Closed/missing PR guard (#audit-rate-headroom): a CLOSED PR that already has a complete detail sync has all + // the outcome/telemetry it will ever need — GitHub's data for it is final. Skip the files/reviews/checks + // refetch unless the caller explicitly forces one (e.g. the manual "review-now" repair command). + if (!options.force && pr.state !== "open") { + const existingState = await getPullRequestDetailSyncState(env, repoFullName, pullNumber); + if (existingState?.status === "complete") { + return { ok: true, repoFullName, pullNumber, status: existingState.status, warnings: [] }; + } + } const token = await tokenForRepo(env, repo); const admissionKey = repoAdmissionKeyForToken(env, repo, token); const warnings: string[] = []; await upsertPullRequestDetailSyncState(env, { repoFullName, pullNumber, status: "running" }); - await fetchAndStorePullRequestDetails(env, repoFullName, pr, token, warnings, admissionKey); + 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, { repoFullName, pullNumber, status, + headSha: pr.headSha, filesSyncedAt: syncedAt, reviewsSyncedAt: syncedAt, checksSyncedAt: syncedAt, @@ -1276,7 +1298,7 @@ async function backfillRecentMergedSegment( // recent_merged_pull_requests.changedFiles is populated instead of always empty. const warnings: string[] = []; const admissionKey = repoAdmissionKeyForToken(env, repo, token); - await hydrateMergedPullRequestFiles(env, repo.fullName, merged, token, warnings, 8, admissionKey); + await hydrateMergedPullRequestFiles(env, repo.fullName, merged, token, warnings, 8, mode, admissionKey); return merged.length; }, { progressiveHistory: true, countPersisted: () => countRecentMergedPullRequests(env, repo.fullName) }, @@ -1287,6 +1309,13 @@ async function backfillRecentMergedSegment( // fetch — the N+1 REST fan-out that dominated this segment's GitHub cost — for any merged PR ALREADY hydrated, // re-upserting only the cheap metadata (the upsert preserves the stored files when passed an empty list). One // `listRecentMergedPullRequests` read per batch replaces up to one `/files` fetch per merged PR. (#1941) +// +// This is scheduled, historical work: none of it is needed for a CURRENT review, so it is both hard-capped +// (MERGED_PR_FILE_HYDRATION_BATCH_SIZE, independent of the page's own size) and budget-gated at the earliest, +// most conservative floor (HISTORICAL_BACKFILL_RESERVED_HEADROOM) — re-checked on every page, not just once at +// segment entry, so a large un-hydrated backlog can never flood the shared bucket in one job execution +// (#audit-rate-headroom). A PR skipped for either reason is upserted with cheap metadata only (empty +// changedFiles, preserved by the upsert if already hydrated) and stays a candidate on the next run. async function hydrateMergedPullRequestFiles( env: Env, repoFullName: string, @@ -1294,6 +1323,7 @@ async function hydrateMergedPullRequestFiles( token: string | undefined, warnings: string[], concurrency: number, + mode: BackfillMode, admissionKey?: GitHubRateLimitAdmissionKey, ): Promise { const alreadyHydrated = new Set( @@ -1301,11 +1331,15 @@ async function hydrateMergedPullRequestFiles( .filter((record) => record.changedFiles.length > 0) .map((record) => record.number), ); + const pending = merged.filter((pr) => !alreadyHydrated.has(pr.number)); + const resetAt = pending.length > 0 ? await shouldWaitForGitHubRateLimit(env, HISTORICAL_BACKFILL_RESERVED_HEADROOM) : undefined; + if (resetAt) warnings.push(`Historical merged PR file hydration deferred for ${pending.length} pull request(s): GitHub REST budget below the historical-backfill headroom floor (retry after ${resetAt}).`); + const budgeted = resetAt ? new Set() : new Set(pending.slice(0, MERGED_PR_FILE_HYDRATION_BATCH_SIZE[mode]).map((pr) => pr.number)); await mapWithConcurrency(merged, concurrency, async (pr) => { // fetchPullRequestFiles never throws — it returns [] (and records a warning) on any fetch failure. - const changedFiles = alreadyHydrated.has(pr.number) - ? [] - : await fetchPullRequestFiles(env, repoFullName, pr.number, token, warnings, admissionKey); + const changedFiles = budgeted.has(pr.number) + ? await fetchPullRequestFiles(env, repoFullName, pr.number, token, warnings, admissionKey, "backfill_merged_history") + : []; await upsertRecentMergedPullRequest(env, toRecentMergedPullRequest(repoFullName, pr, changedFiles)); }); } @@ -1767,12 +1801,30 @@ async function backfillRepository(env: Env, repo: RepositoryRecord, limits: Back const normalizedPullRequests = await mapWithConcurrency(pullRequests, 16, async (pr) => upsertPullRequestFromGitHub(env, repo.fullName, pr, { seenOpenAt: startedAt })); const mergedFileWarningStart = warnings.length; - await hydrateMergedPullRequestFiles(env, repo.fullName, recentMerged, token, warnings, limits.detailConcurrency, admissionKey); + await hydrateMergedPullRequestFiles(env, repo.fullName, recentMerged, token, warnings, limits.detailConcurrency, mode, admissionKey); const detailTargets = normalizedPullRequests.slice(0, limits.pullRequestDetails); const detailWarningStart = warnings.length; await mapWithConcurrency(detailTargets, limits.detailConcurrency, async (pr) => { - await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey); + const before = warnings.length; + 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 + // for PRs only ever touched by this monolithic backfill path. + const syncedAt = nowIso(); + const newWarnings = warnings.slice(before); + await upsertPullRequestDetailSyncState(env, { + repoFullName: repo.fullName, + pullNumber: pr.number, + status: newWarnings.length > 0 ? "partial" : "complete", + headSha: pr.headSha, + filesSyncedAt: syncedAt, + reviewsSyncedAt: syncedAt, + checksSyncedAt: syncedAt, + lastSyncedAt: syncedAt, + errorSummary: newWarnings.at(-1), + }); }); const fileWarnings = warnings.slice(mergedFileWarningStart).filter((warning) => /File sync failed/i.test(warning)); const reviewWarnings = warnings.slice(detailWarningStart).filter((warning) => /Review sync failed/i.test(warning)); @@ -1923,17 +1975,25 @@ async function fetchAndStorePullRequestDetails( pr: PullRequestRecord, token: string | undefined, warnings: string[], - admissionKey?: GitHubRateLimitAdmissionKey, + admissionKey: GitHubRateLimitAdmissionKey | undefined, + caller: PullRequestFilesFetchCaller, + options: { forceFiles?: boolean | undefined } = {}, ): Promise { + // 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; + const filesUpToDate = Boolean(existingState?.headSha) && existingState?.headSha === pr.headSha && Boolean(existingState?.filesSyncedAt); const warningStart = warnings.length; const [files, reviews, checks] = await Promise.all([ - fetchPullRequestFiles(env, repoFullName, pr.number, token, warnings, admissionKey), + filesUpToDate ? Promise.resolve([]) : fetchPullRequestFiles(env, repoFullName, pr.number, token, warnings, admissionKey, caller), 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}:`)); - if (!fileSyncFailed) { + if (!filesUpToDate && !fileSyncFailed) { await deletePullRequestFiles(env, repoFullName, pr.number); for (const file of files) { await upsertPullRequestFile(env, { @@ -2009,8 +2069,10 @@ async function fetchPullRequestFiles( pullNumber: number, token: string | undefined, warnings: string[], - admissionKey?: GitHubRateLimitAdmissionKey, + admissionKey: GitHubRateLimitAdmissionKey | undefined, + caller: PullRequestFilesFetchCaller, ): Promise { + incr(PULL_REQUEST_FILES_FETCH_METRIC, { caller }); const files = await githubPaginatedList(env, repoFullName, `/pulls/${pullNumber}/files`, token, admissionKey); if (files) return files; const fallback = token ? await fetchPullRequestDetailsFromGraphQl(env, repoFullName, pullNumber, token, admissionKey).catch(() => undefined) : undefined; @@ -2055,7 +2117,7 @@ export async function fetchAndStorePullRequestFilesForReview( admissionKey?: GitHubRateLimitAdmissionKey, ): Promise { const warnings: string[] = []; - const files = await fetchPullRequestFiles(env, repoFullName, pullNumber, token, warnings, admissionKey).catch(() => [] as GitHubFilePayload[]); + const files = await fetchPullRequestFiles(env, repoFullName, pullNumber, token, warnings, admissionKey, "live_review").catch(() => [] as GitHubFilePayload[]); if (files.length === 0) return []; const records = files.map((file) => toPullRequestFileRecordFromGitHub(repoFullName, pullNumber, file)); // Persist so the AI review, grounding, gate, check-run, and unified-comment reads in THIS run (and any later diff --git a/src/github/rate-limit.ts b/src/github/rate-limit.ts index b97fc14d21..feee4b7c30 100644 --- a/src/github/rate-limit.ts +++ b/src/github/rate-limit.ts @@ -4,12 +4,16 @@ import { listLatestGitHubRateLimitObservations } from "../db/repositories"; // from draining the budget real webhook traffic needs, maintenance yields while there is still headroom: // - backfill yields at LOW_REST_RATE_LIMIT_REMAINING; // - the re-gate sweep + its per-PR jobs yield EARLIER, at MAINTENANCE_RESERVED_HEADROOM, reserving the budget -// between the two floors for webhooks. +// between the two floors for webhooks; +// - historical/scheduled hydration that isn't needed for any CURRENT PR (e.g. backfilling file lists for old +// merged pull requests) yields EARLIEST, at HISTORICAL_BACKFILL_RESERVED_HEADROOM — it is the least urgent +// GitHub REST consumer, so it must never be the reason a live review or an open-PR convergence pass stalls. // Self-host queues also use the latest persisted observation for admission control, so a known-exhausted bucket // delays webhook jobs before they start and avoids burning the first live delivery just to discover the limit. // (#audit-rate-headroom) export const LOW_REST_RATE_LIMIT_REMAINING = 75; export const MAINTENANCE_RESERVED_HEADROOM = 150; +export const HISTORICAL_BACKFILL_RESERVED_HEADROOM = 300; /** The REST rate-limit reset time to wait until when the latest recorded REST budget is at/below `floor`, or * undefined when there is headroom, no usable observation, or the reset is already in the past. Reads the latest diff --git a/src/github/webhook-coalesce.ts b/src/github/webhook-coalesce.ts index d6ec0ddd08..e703bc94af 100644 --- a/src/github/webhook-coalesce.ts +++ b/src/github/webhook-coalesce.ts @@ -1,7 +1,11 @@ import type { GitHubWebhookPayload } from "../types"; +// Kept in sync with PR_PUBLIC_SURFACE_ACTIONS (src/queue/processors.ts) minus "closed" (merge/close has its own +// non-coalesced handling): every action that can trigger a file refresh for the SAME PR+head should collapse a +// burst into one job, not one job per delivery (#audit-rate-headroom). const COALESCABLE_PULL_REQUEST_ACTIONS = new Set([ "opened", + "reopened", "synchronize", "edited", "ready_for_review", diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 00c0eb4d5d..03da2ad78e 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -7518,13 +7518,15 @@ async function maybeProcessPrPanelRetrigger( }); // A manual re-run is a re-evaluation surface — the user clicks it AFTER the PR changed — so the slop and // manifest-policy gates must see the PR's current files, not whatever is cached. Mirror the webhook path - // (#866/#925): refresh before publishing so the re-published Gate check reflects the latest file set. + // (#866/#925): refresh before publishing so the re-published Gate check reflects the latest file set. This is + // the explicit manual repair/debug trigger (#audit-rate-headroom), so force a fresh fetch past the head-SHA + // snapshot cache — the user asked for a re-check even if nothing detectably changed. if ( shouldCollectSlopEvidence(settings) || settings.manifestPolicyGateMode !== "off" || (await shouldRefreshFilesForPreMergeChecks(env, repoFullName)) ) { - await refreshPullRequestDetails(env, repoFullName, pr.number); + await refreshPullRequestDetails(env, repoFullName, pr.number, { force: true }); } const liveFacts = createLiveGithubFacts(); if ( diff --git a/src/types.ts b/src/types.ts index 62c5f4685f..63bc03cf3e 100644 --- a/src/types.ts +++ b/src/types.ts @@ -820,6 +820,7 @@ export type PullRequestDetailSyncStateRecord = { repoFullName: string; pullNumber: number; status: "never_synced" | "running" | "complete" | "partial" | "waiting_rate_limit" | "error"; + headSha?: string | null | undefined; filesSyncedAt?: string | null | undefined; reviewsSyncedAt?: string | null | undefined; checksSyncedAt?: string | null | undefined; diff --git a/test/unit/backfill-file-hydration-scoping.test.ts b/test/unit/backfill-file-hydration-scoping.test.ts new file mode 100644 index 0000000000..22ed3e12fa --- /dev/null +++ b/test/unit/backfill-file-hydration-scoping.test.ts @@ -0,0 +1,412 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + getPullRequestDetailSyncState, + listPullRequestFiles, + listRecentMergedPullRequests, + recordGitHubRateLimitObservation, + upsertPullRequestDetailSyncState, + upsertPullRequestFile, + upsertPullRequestFromGitHub, +} from "../../src/db/repositories"; +import { backfillOpenPullRequestDetails, backfillRegisteredRepositories, backfillRepositorySegment, refreshPullRequestDetails } from "../../src/github/backfill"; +import { clearGitHubResponseCacheForTest } from "../../src/github/client"; +import { renderMetrics, 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 file hydration scoping (#audit-rate-headroom)", () => { + 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("does not fetch PR files for a pull request missing from pull_requests", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + const urls = stubFetchTracking(() => Response.json([])); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 404); + + expect(result).toMatchObject({ status: "partial", warnings: ["Repository or pull request was not found."] }); + expect(urls.some((url) => url.includes("/files"))).toBe(false); + }); + + it("does not re-fetch files for a closed PR that already has complete stored telemetry, but does when forced", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 20, + title: "Closed PR", + state: "closed", + user: { login: "oktofeesh1" }, + head: { sha: "closed-sha" }, + labels: [], + body: "", + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 20, + status: "complete", + headSha: "closed-sha", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => (url.includes("/files") ? new Response("must not be called", { status: 500 }) : Response.json([]))); + + const unforced = await refreshPullRequestDetails(env, "JSONbored/gittensory", 20); + expect(unforced).toMatchObject({ status: "complete", warnings: [] }); + expect(urls.some((url) => url.includes("/files"))).toBe(false); + + urls.length = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + urls.push(url); + return url.includes("/pulls/20/files") ? Response.json([{ filename: "src/final.ts", status: "modified", additions: 1, deletions: 0, changes: 1 }]) : Response.json([]); + }); + const forced = await refreshPullRequestDetails(env, "JSONbored/gittensory", 20, { force: true }); + expect(forced).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/20/files"))).toBe(true); + }); + + it("still fetches a closed PR without a complete sync state (no stored telemetry yet to rely on)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 21, + title: "Closed PR, never synced", + state: "closed", + user: { login: "oktofeesh1" }, + head: { sha: "closed-sha-2" }, + labels: [], + body: "", + }); + const urls = stubFetchTracking(() => Response.json([])); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 21); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/21/files"))).toBe(true); + }); + + it("fetches PR files for a current open PR when no repo+PR+head snapshot exists", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 30, + title: "Open PR, never synced", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-1" }, + labels: [], + body: "", + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/30/files") ? Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1 }]) : Response.json([]), + ); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 30); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/30/files"))).toBe(true); + expect(await listPullRequestFiles(env, "JSONbored/gittensory", 30)).toEqual([expect.objectContaining({ path: "src/a.ts" })]); + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 30)).toMatchObject({ headSha: "head-1", status: "complete" }); + }); + + it("reuses the repo+PR+head file snapshot without calling GitHub when the head is unchanged", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 31, + title: "Open PR, already synced", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-1" }, + labels: [], + body: "", + }); + await upsertPullRequestFile(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 31, + path: "src/cached.ts", + status: "modified", + additions: 2, + deletions: 1, + changes: 3, + payload: { filename: "src/cached.ts" }, + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 31, + status: "complete", + headSha: "head-1", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => (url.includes("/files") ? new Response("must not be called", { status: 500 }) : Response.json([]))); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 31); + + expect(result).toMatchObject({ status: "complete", warnings: [] }); + expect(urls.some((url) => url.includes("/pulls/31/files"))).toBe(false); + expect(await listPullRequestFiles(env, "JSONbored/gittensory", 31)).toEqual([expect.objectContaining({ path: "src/cached.ts", changes: 3 })]); + }); + + it("fetches fresh files and does not reuse the previous snapshot when the head SHA changes", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 32, + title: "Open PR, new push", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-2" }, + labels: [], + body: "", + }); + await upsertPullRequestFile(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 32, + path: "src/old.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: { filename: "src/old.ts" }, + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 32, + status: "complete", + headSha: "head-1", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/32/files") ? Response.json([{ filename: "src/new.ts", status: "added", additions: 5, deletions: 0, changes: 5 }]) : Response.json([]), + ); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 32); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/32/files"))).toBe(true); + expect(await listPullRequestFiles(env, "JSONbored/gittensory", 32)).toEqual([expect.objectContaining({ path: "src/new.ts" })]); + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 32)).toMatchObject({ headSha: "head-2" }); + }); + + it("defers historical merged-PR file hydration when the REST budget is below the historical-backfill floor, while cheap metadata still syncs", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + // Below HISTORICAL_BACKFILL_RESERVED_HEADROOM (300) but above MAINTENANCE_RESERVED_HEADROOM (150) and + // LOW_REST_RATE_LIMIT_REMAINING (75) — healthy enough for the segment's own entry check and for current-PR + // convergence, but not for the least-urgent historical hydration path. + await recordGitHubRateLimitObservation(env, { + repoFullName: "JSONbored/gittensory", + resource: "rest", + path: "/pulls", + statusCode: 200, + remaining: 200, + resetAt: "2999-01-01T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => { + if (url.includes("/pulls?state=closed")) { + return Response.json([ + { number: 501, title: "Merged 501", state: "closed", merged_at: "2026-05-20T00:00:00.000Z", user: { login: "a" }, labels: [], body: "" }, + { number: 502, title: "Merged 502", state: "closed", merged_at: "2026-05-20T00:00:00.000Z", user: { login: "a" }, labels: [], body: "" }, + ]); + } + return Response.json([]); + }); + + const result = await backfillRepositorySegment(env, { repoFullName: "JSONbored/gittensory", segment: "recent_merged_pull_requests", mode: "full" }); + + // Not throttled at the segment's own entry check (default LOW_REST_RATE_LIMIT_REMAINING floor) — only the + // historical file hydration inside it yields, at the stricter HISTORICAL_BACKFILL_RESERVED_HEADROOM floor. + expect(result.status).not.toBe("waiting_rate_limit"); + expect(urls.some((url) => url.includes("/files"))).toBe(false); + const stored = await listRecentMergedPullRequests(env, "JSONbored/gittensory"); + expect(stored.map((row) => row.number).sort()).toEqual([501, 502]); + expect(stored.every((row) => row.changedFiles.length === 0)).toBe(true); + }); + + it("still allows current open PR convergence within its small batch cap at the same reduced budget", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await recordGitHubRateLimitObservation(env, { + repoFullName: "JSONbored/gittensory", + resource: "rest", + path: "/pulls", + statusCode: 200, + remaining: 200, + resetAt: "2999-01-01T00:00:00.000Z", + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 40, + title: "Open PR under reduced budget", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-40" }, + labels: [], + body: "", + }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/40/files") ? Response.json([{ filename: "src/x.ts", status: "modified", additions: 1, deletions: 0, changes: 1 }]) : Response.json([]))); + + const result = await backfillOpenPullRequestDetails(env, { repoFullName: "JSONbored/gittensory", mode: "light", cursor: 0 }); + + expect(result.status).toBe("complete"); + expect(urls.some((url) => url.includes("/pulls/40/files"))).toBe(true); + }); + + it("caps historical merged-PR file hydration per page even when the REST budget is healthy", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + const mergedPage = Array.from({ length: 25 }, (_, index) => ({ + number: 600 + index, + title: `Merged ${600 + index}`, + state: "closed" as const, + merged_at: "2026-05-20T00:00:00.000Z", + user: { login: "a" }, + labels: [], + body: "", + })); + const fetchedFileNumbers: number[] = []; + stubFetchTracking((url) => { + if (url.includes("/pulls?state=closed")) return Response.json(mergedPage); + const match = /\/pulls\/(\d+)\/files/.exec(url); + if (match) { + fetchedFileNumbers.push(Number(match[1])); + return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1 }]); + } + return Response.json([]); + }); + + await backfillRepositorySegment(env, { repoFullName: "JSONbored/gittensory", segment: "recent_merged_pull_requests", mode: "light" }); + + // MERGED_PR_FILE_HYDRATION_BATCH_SIZE.light === 10, far under the 25 un-hydrated candidates in this page. + expect(fetchedFileNumbers.length).toBe(10); + const stored = await listRecentMergedPullRequests(env, "JSONbored/gittensory"); + expect(stored).toHaveLength(25); + expect(stored.filter((row) => row.changedFiles.length > 0)).toHaveLength(10); + }); + + it("records bounded caller labels on the PR files fetch metric for backfill, historical, and live-review callers", async () => { + resetMetrics(); + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 50, + title: "Open PR (backfill caller)", + state: "open", + user: { login: "a" }, + head: { sha: "head-50" }, + labels: [], + body: "", + }); + stubFetchTracking((url) => { + if (url.includes("/pulls?state=closed")) return Response.json([{ number: 700, title: "Merged", state: "closed", merged_at: "2026-05-20T00:00:00.000Z", user: { login: "a" }, labels: [], body: "" }]); + return Response.json([]); + }); + + // #50 is the only open PR at this point, so this call attributes exactly one fetch to "backfill_open_pr_details". + await backfillOpenPullRequestDetails(env, { repoFullName: "JSONbored/gittensory", mode: "light", cursor: 0 }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 51, + title: "Open PR (live review caller)", + state: "open", + user: { login: "a" }, + head: { sha: "head-51" }, + labels: [], + body: "", + }); + await refreshPullRequestDetails(env, "JSONbored/gittensory", 51); + await backfillRepositorySegment(env, { repoFullName: "JSONbored/gittensory", segment: "recent_merged_pull_requests", mode: "light" }); + + const metrics = await renderMetrics(); + expect(metrics).toContain('gittensory_github_pull_request_files_fetch_total{caller="backfill_open_pr_details"} 1'); + expect(metrics).toContain('gittensory_github_pull_request_files_fetch_total{caller="live_review"} 1'); + expect(metrics).toContain('gittensory_github_pull_request_files_fetch_total{caller="backfill_merged_history"} 1'); + // Bounded: only the 3 known caller values appear, never a per-PR-number label. + const callerLines = metrics.split("\n").filter((line) => line.startsWith("gittensory_github_pull_request_files_fetch_total{")); + expect(callerLines).toHaveLength(3); + }); + + it("populates the head-SHA snapshot from the monolithic backfillRegisteredRepositories path so a later run reuses it (regression: the /run admin endpoint's PR-detail loop must WRITE the cache it reads)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + let filesCallCount = 0; + stubFetchTracking((url) => { + if (url.endsWith("/repos/JSONbored/gittensory")) return Response.json({ name: "gittensory", full_name: "JSONbored/gittensory", default_branch: "main", owner: { login: "JSONbored" } }); + if (url.includes("/pulls?state=open")) { + return Response.json([{ number: 60, title: "Open PR", state: "open", user: { login: "a" }, head: { sha: "head-60" }, labels: [], body: "" }]); + } + if (url.includes("/pulls/60/files")) { + filesCallCount += 1; + return Response.json([{ filename: "src/x.ts", status: "modified", additions: 1, deletions: 0, changes: 1 }]); + } + return Response.json([]); + }); + + // First run: cold cache, must fetch and then WRITE the snapshot marker. + await backfillRegisteredRepositories(env, { repoFullName: "JSONbored/gittensory", limits: { issues: 10, pullRequests: 10, recentMergedPullRequests: 10, pullRequestDetails: 10 } }); + expect(filesCallCount).toBe(1); + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 60)).toMatchObject({ headSha: "head-60", status: "complete" }); + + // Second run, same head: the cache this path itself just wrote must be reused, not re-fetched. + await backfillRegisteredRepositories(env, { repoFullName: "JSONbored/gittensory", force: true, limits: { issues: 10, pullRequests: 10, recentMergedPullRequests: 10, pullRequestDetails: 10 } }); + expect(filesCallCount).toBe(1); + }); + + describe("upsertPullRequestDetailSyncState partial-update contract", () => { + it("preserves an existing headSha (and other fields) when a later upsert omits them, as every 'running' pre-fetch stamp does", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 90, + status: "complete", + headSha: "sha-a", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + errorSummary: "prior warning", + }); + + // The "running" pre-fetch stamp every backfill path sends touches ONLY status — headSha/filesSyncedAt/ + // errorSummary are omitted (undefined), not explicitly cleared. The file cache depends on this NOT + // wiping the row it is about to read. + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 90, status: "running" }); + + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 90)).toMatchObject({ + status: "running", + headSha: "sha-a", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + errorSummary: "prior warning", + }); + }); + + it("clears headSha when a caller explicitly passes null, distinguishing 'omitted' from 'cleared'", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 91, status: "complete", headSha: "sha-b" }); + + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 91, status: "complete", headSha: null }); + + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 91)).toMatchObject({ headSha: null }); + }); + }); +}); diff --git a/test/unit/github-webhook-coalesce.test.ts b/test/unit/github-webhook-coalesce.test.ts index 04d8935d39..1704f3cc81 100644 --- a/test/unit/github-webhook-coalesce.test.ts +++ b/test/unit/github-webhook-coalesce.test.ts @@ -51,6 +51,21 @@ describe("githubWebhookCoalesceKey", () => { } }); + it("coalesces a burst of reopened + synchronize + ready_for_review events for the same PR head into one key (regression for #audit-rate-headroom)", () => { + // "reopened" triggers the same file-refresh path as "opened"/"synchronize" (PR_PUBLIC_SURFACE_ACTIONS in + // src/queue/processors.ts) but was missing from the coalescable set — a burst of reopen-adjacent events for + // the same PR+head fanned out one `/pulls/{n}/files` fetch per delivery instead of coalescing into one job. + const burstKeys = (["reopened", "synchronize", "ready_for_review"] as const).map((action) => + githubWebhookCoalesceKey("pull_request", { + action, + repository: { full_name: "JSONbored/Gittensory" }, + pull_request: { number: 100, head: { sha: "BEEF123" } }, + } as GitHubWebhookPayload), + ); + expect(new Set(burstKeys).size).toBe(1); + expect(burstKeys[0]).toBe("github-webhook:pr-refresh:jsonbored/gittensory#100@beef123"); + }); + it("returns null for malformed or non-coalescible webhook shapes", () => { expect(githubWebhookCoalesceKey("issues", { action: "closed", repository: { full_name: "JSONbored/Gittensory" } } as never)).toBeNull(); expect(githubWebhookCoalesceKey("check_suite", { action: "requested", repository: { full_name: "JSONbored/Gittensory" } } as never)).toBeNull();