Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 29 additions & 8 deletions src/github/backfill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
upsertPullRequestDetailSyncState,
upsertPullRequestFromGitHub,
upsertPullRequestReview,
listRecentMergedPullRequests,
upsertRecentMergedPullRequest,
upsertRepoLabel,
upsertRepoSyncSegment,
Expand Down Expand Up @@ -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<void> {
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<T>(result: GitHubJsonConditionalResponse<T>): result is GitHubJsonNotModifiedResponse {
return "notModified" in result && result.notModified;
}
Expand Down Expand Up @@ -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;
Expand Down
39 changes: 39 additions & 0 deletions test/unit/backfill.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down
Loading