Skip to content

Commit e28d90b

Browse files
JSONboredJSONbored
andauthored
fix(backfill): stop recording a resume cursor the sampled segment can never consume (#10238)
recent_merged_pull_requests is the only progressiveHistory segment and the only one whose terminal status is 'sampled'. Three independent gates make that status unresumable: complete+hasMore maps to 'sampled' rather than 'running'; canResumePreviousScan accepts only running/partial/waiting_rate_limit, so both a stored nextCursor and an explicitly passed cursor are ignored and startPage falls back to 1; and the automatic resume re-send is scoped to labels/open_issues/ open_pull_requests while the cron only dispatches light/full. So the nextCursor this segment faithfully recorded was written and never read. Confirmed on edge-nl-01: a resume run with an explicit cursor '11' against a segment at next_cursor=11 re-crawled pages 1-10, persisted nothing new, and handed back the same cursor it started with. Takes the second of the two options on #10209: accept the rolling-window design and remove the misleading bookkeeping, rather than making 'sampled' resumable. Nothing consumes the deep history -- every reader goes through listRecentMergedPullRequests, which is ORDER BY merged_at DESC LIMIT 200 -- so option 1 would build machinery to satisfy a claim no caller makes. expectedCount is deliberately kept: 'this window holds N of the M closed PRs GitHub reports' is a true coverage statement. It only misleads when read as progress toward M, which is exactly what the now-absent cursor signals. No behaviour change to the crawl itself: sampled is not a fresh status, so conditionalRequestForSegment already returned undefined for it regardless of the cursor, and the 304 fast path is untouched. Closes #10209 Co-authored-by: JSONbored <aetherealdev@gmail.com>
1 parent ee1b86e commit e28d90b

2 files changed

Lines changed: 75 additions & 0 deletions

File tree

src/github/backfill.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1712,6 +1712,11 @@ async function fetchPagedSegment<T>(
17121712
expectedCount: number | undefined,
17131713
persistPage: (payloads: T[], scanStartedAt: string) => Promise<number>,
17141714
options: {
1715+
/** Marks a segment as a BOUNDED RECENT-WINDOW SAMPLE rather than an exhaustive crawl: when the page budget
1716+
* runs out with more pages upstream, the run settles as `sampled` instead of `running`, and records no
1717+
* continuation cursor because none can be consumed (#10209 — see the fuller note at the `sampled` branch).
1718+
* The name is historical and reads as "walks deeper each run"; it does not. Coverage grows only by
1719+
* accretion as the underlying `sort=updated` window slides. */
17151720
progressiveHistory?: boolean;
17161721
countPersisted?: () => Promise<number>;
17171722
reconcileOnComplete?: (scanStartedAt: string) => Promise<number>;
@@ -1814,6 +1819,25 @@ async function fetchPagedSegment<T>(
18141819
if (status === "complete") {
18151820
if (hasMore && options.progressiveHistory) {
18161821
status = "sampled";
1822+
// #10209: a `sampled` segment is NOT resumable, so a nextCursor recorded here is written and never read.
1823+
// Three independent gates make that so: this branch maps complete+hasMore to `sampled` rather than
1824+
// `running`; `canResumePreviousScan` accepts only running/partial/waiting_rate_limit, so both
1825+
// `previous.nextCursor` and an explicitly passed `cursor` are ignored and startPage falls back to 1; and
1826+
// the automatic resume re-send below is scoped to labels/open_issues/open_pull_requests, with the
1827+
// scheduled cron only ever dispatching light/full. Confirmed live on edge-nl-01: a `resume` run with an
1828+
// explicit cursor: "11" against a segment at next_cursor=11 re-crawled pages 1-10 and persisted nothing
1829+
// new, returning the same nextCursor: "11" it started with.
1830+
//
1831+
// Clearing it makes the stored row describe what this segment actually IS -- a bounded window over the
1832+
// most-recently-updated closed PRs, re-crawled from page 1 every run, whose coverage grows by accretion
1833+
// as the window slides (and is trimmed by the 30-day updated_at retention in src/db/retention.ts). That
1834+
// is a defensible design; recording a continuation position no scheduled or manual path can consume is
1835+
// not, because it invites a reader to conclude the crawl is advancing when it never can.
1836+
//
1837+
// `expectedCount` is deliberately KEPT: "this window holds N of the M closed PRs GitHub reports" is a
1838+
// true and useful coverage statement. It is only misleading when read as progress toward M, which is
1839+
// what the absent cursor now signals.
1840+
nextCursor = undefined;
18171841
} else if (hasMore) {
18181842
status = "running";
18191843
} else {

test/unit/backfill.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest";
22
import { clearInstallationTokenCacheForTest } from "../../src/github/app";
33
import {
44
getInstallationHealth,
5+
getRepoSyncSegment,
56
listCheckSummaries,
67
listContributorRepoStats,
78
listIssues,
@@ -4052,6 +4053,56 @@ describe("GitHub backfill", () => {
40524053

40534054
expect(result).toMatchObject({ status: "sampled", fetchedCount: 10, expectedCount: 2000 });
40544055
expect(await listRecentMergedPullRequests(env, "JSONbored/gittensory")).toHaveLength(10);
4056+
4057+
// REGRESSION (#10209): a `sampled` segment records NO continuation cursor. It is not resumable --
4058+
// canResumePreviousScan accepts only running/partial/waiting_rate_limit -- so a stored nextCursor would be
4059+
// written and never read, describing a continuation no scheduled or manual path can perform. expectedCount
4060+
// is deliberately still recorded: "10 of 2000" is a true coverage statement, just not a progress one.
4061+
expect(result.nextCursor ?? null).toBeNull();
4062+
const stored = await getRepoSyncSegment(env, "JSONbored/gittensory", "recent_merged_pull_requests");
4063+
expect(stored?.status).toBe("sampled");
4064+
expect(stored?.nextCursor ?? null).toBeNull();
4065+
expect(stored?.expectedCount).toBe(2000);
4066+
});
4067+
4068+
it("REGRESSION (#10209): a resume dispatched against a sampled segment restarts at page 1 rather than advancing", async () => {
4069+
// Pins the behaviour the absent cursor now advertises honestly. Verified live on edge-nl-01 before the
4070+
// change: a resume run with an explicit cursor: "11" against next_cursor=11 re-crawled pages 1-10 and
4071+
// persisted nothing new. The segment is a rolling most-recently-updated window, not a deepening crawl.
4072+
const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
4073+
await seedRegisteredRepo(env);
4074+
const pagesRequested: number[] = [];
4075+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
4076+
const url = input.toString();
4077+
if (url === "https://api.github.com/graphql") return githubTotalsResponse({ openIssues: 0, openPullRequests: 0, mergedPullRequests: 2000, closedPullRequests: 0, labels: 0 });
4078+
if (/\/pulls\/\d+\/files/.test(url)) return Response.json([]);
4079+
if (url.includes("/pulls?state=closed")) {
4080+
const page = Number(new URL(url).searchParams.get("page") ?? "1");
4081+
pagesRequested.push(page);
4082+
return Response.json(
4083+
[{ number: page, title: `Merged ${page}`, state: "closed", merged_at: "2026-05-20T00:00:00.000Z", user: { login: "oktofeesh1" }, labels: [], body: "" }],
4084+
{ headers: { link: `<https://api.github.com/repositories/1/pulls?page=${page + 1}>; rel="next"` } },
4085+
);
4086+
}
4087+
return Response.json([]);
4088+
});
4089+
4090+
await backfillRepositorySegment(env, { repoFullName: "JSONbored/gittensory", segment: "recent_merged_pull_requests", mode: "full" });
4091+
pagesRequested.length = 0;
4092+
4093+
const resumed = await backfillRepositorySegment(env, {
4094+
repoFullName: "JSONbored/gittensory",
4095+
segment: "recent_merged_pull_requests",
4096+
mode: "resume",
4097+
cursor: "11",
4098+
});
4099+
4100+
// The explicitly-passed cursor is ignored: the crawl restarts from page 1, so nothing beyond the window
4101+
// is ever reachable and the run settles as `sampled` again with no cursor to hand back.
4102+
expect(pagesRequested[0]).toBe(1);
4103+
expect(pagesRequested).not.toContain(11);
4104+
expect(resumed).toMatchObject({ status: "sampled" });
4105+
expect(resumed.nextCursor ?? null).toBeNull();
40554106
});
40564107

40574108
it("hydrates PR files and reviews through GraphQL when public-token REST detail endpoints are hidden", async () => {

0 commit comments

Comments
 (0)