diff --git a/src/github/backfill.ts b/src/github/backfill.ts index c0ec63c8c5..70940f0ef4 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -30,6 +30,7 @@ import { upsertPullRequestDetailSyncState, upsertPullRequestFromGitHub, upsertPullRequestReview, + listRecentMergedPullRequests, upsertRecentMergedPullRequest, upsertRepoLabel, upsertRepoSyncSegment, @@ -1238,16 +1239,39 @@ async function backfillRecentMergedSegment( // Hydrate each merged PR's changed files (like the monolithic backfill path) so // recent_merged_pull_requests.changedFiles is populated instead of always empty. const warnings: string[] = []; - await mapWithConcurrency(merged, 8, async (pr) => { - const changedFiles = await fetchPullRequestFiles(env, repo.fullName, pr.number, token, warnings).catch(() => []); - await upsertRecentMergedPullRequest(env, toRecentMergedPullRequest(repo.fullName, pr, changedFiles)); - }); + await hydrateMergedPullRequestFiles(env, repo.fullName, merged, token, warnings, 8); return merged.length; }, { progressiveHistory: true, countPersisted: () => countRecentMergedPullRequests(env, repo.fullName) }, ); } +// A merged PR is immutable, so its changed-file list never changes once stored. Skip the per-PR `/pulls/{n}/files` +// 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) +async function hydrateMergedPullRequestFiles( + env: Env, + repoFullName: string, + merged: GitHubPullRequestPayload[], + token: string | undefined, + warnings: string[], + concurrency: number, +): Promise { + const alreadyHydrated = new Set( + (await listRecentMergedPullRequests(env, repoFullName)) + .filter((record) => record.changedFiles.length > 0) + .map((record) => record.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); + await upsertRecentMergedPullRequest(env, toRecentMergedPullRequest(repoFullName, pr, changedFiles)); + }); +} + function isNotModifiedResponse(result: GitHubJsonConditionalResponse): result is GitHubJsonNotModifiedResponse { return "notModified" in result && result.notModified; } @@ -1691,10 +1715,7 @@ 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 mapWithConcurrency(recentMerged, limits.detailConcurrency, async (pr) => { - const changedFiles = await fetchPullRequestFiles(env, repo.fullName, pr.number, token, warnings).catch(() => []); - await upsertRecentMergedPullRequest(env, toRecentMergedPullRequest(repo.fullName, pr, changedFiles)); - }); + await hydrateMergedPullRequestFiles(env, repo.fullName, recentMerged, token, warnings, limits.detailConcurrency); const detailTargets = normalizedPullRequests.slice(0, limits.pullRequestDetails); const detailWarningStart = warnings.length; diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 2901273e4a..422d42bb2c 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -1023,6 +1023,45 @@ describe("GitHub backfill", () => { ); }); + it("skips the /files fetch for a merged PR whose changed files are already stored (#1941)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + // PR 9 was hydrated with files by a prior sync; a merged PR is immutable, so its files can never change. + await upsertRecentMergedPullRequest(env, { + repoFullName: "JSONbored/gittensory", + number: 9, + title: "Fix webhook", + authorLogin: "oktofeesh1", + mergedAt: "2026-05-22T00:00:00.000Z", + labels: ["bug"], + linkedIssues: [1], + changedFiles: ["src/github/webhook.ts"], + payload: {}, + }); + let fileFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === "https://api.github.com/graphql") return githubTotalsResponse({ openIssues: 0, openPullRequests: 0, mergedPullRequests: 1, closedPullRequests: 1, labels: 0 }); + if (url.includes("/pulls?state=closed")) { + return Response.json([{ number: 9, title: "Fix webhook processing (edited)", state: "closed", merged_at: "2026-05-22T00:00:00.000Z", user: { login: "oktofeesh1" }, labels: [{ name: "bug" }], body: "Fixes #1" }]); + } + if (url.includes("/pulls/9/files")) { + fileFetches += 1; + return Response.json([{ filename: "src/github/webhook.ts", status: "modified", additions: 12, deletions: 3, changes: 15 }]); + } + return new Response("not found", { status: 404 }); + }); + + const result = await backfillRepositorySegment(env, { repoFullName: "JSONbored/gittensory", segment: "recent_merged_pull_requests", mode: "full" }); + + expect(result).toMatchObject({ status: "complete" }); + expect(fileFetches).toBe(0); // already hydrated → the per-PR /files fetch (the N+1) is skipped + // The cheap metadata is still refreshed (title updated) and the stored files are preserved. + expect(await listRecentMergedPullRequests(env, "JSONbored/gittensory")).toEqual( + expect.arrayContaining([expect.objectContaining({ number: 9, title: "Fix webhook processing (edited)", changedFiles: ["src/github/webhook.ts"] })]), + ); + }); + it("preserves previously-hydrated merged PR files when a later upsert has none", async () => { const env = createTestEnv(); await upsertRecentMergedPullRequest(env, {