fix(backfill): stop recording a resume cursor the sampled segment can never consume - #10238
Conversation
… never consume 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
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-31 14:17:06 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10238 +/- ##
==========================================
- Coverage 92.25% 91.38% -0.87%
==========================================
Files 938 938
Lines 114659 114660 +1
Branches 27680 27680
==========================================
- Hits 105774 104781 -993
- Misses 7580 8769 +1189
+ Partials 1305 1110 -195
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
1 participant
Summary
recent_merged_pull_requestsrecorded anextCursorthat nothing could ever read.It is the only segment using
progressiveHistory, and the only one whose terminal status issampled. Three independent gates make that status unresumable:complete && hasMoremaps tosampled, neverrunning— so the status that would mean "more pages pending" is never produced for this segment.canResumePreviousScanaccepts onlyrunning/partial/waiting_rate_limit.sampledis absent, so bothprevious.nextCursorand an explicitly passedcursorare ignored andstartPagefalls back to 1.mode: "resume"re-send is scoped tolabels/open_issues/open_pull_requests, and the scheduled cron only dispatcheslightorfull.Confirmed on edge-nl-01 before the change — a
resumerun with an explicitcursor: "11"against a segment sitting atnext_cursor=11:fetchedCountunchanged,nextCursorstill11. It re-crawled pages 1–10 and persisted nothing new.Which option this takes, and why
#10209 offered two. This is option 2: accept the rolling-window design and remove the misleading bookkeeping, rather than making
sampledresumable.Option 1 would build machinery to satisfy a claim no caller makes. Every consumer reads through
listRecentMergedPullRequests, which isORDER BY merged_at DESC LIMIT 200— nothing asks for the deep history, so nothing would benefit from being able to walk it. The stored cursor's only effect today is to invite a reader to conclude the crawl is advancing when it structurally cannot.What the segment actually is, now stated in the code: a bounded window over the most-recently-updated closed PRs, re-crawled from page 1 each run, whose coverage grows by accretion as the
sort=updatedwindow slides, and is trimmed by the 30-dayupdated_atretention insrc/db/retention.ts. That is a defensible design. Recording a continuation position for it is not.expectedCountis deliberately kept. "This window holds N of the M closed PRs GitHub reports" is a true and useful 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
sampledis not a fresh status (isFreshSegmentStatusiscomplete/not_modifiedonly), soconditionalRequestForSegmentalready returnedundefinedfor these rows regardless of the cursor. The 304 conditional-request fast path is untouched, and no page is fetched or skipped differently.Closes #10209
Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changednpm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateDetail:
nextCursor = undefined); everything else is comment. Verified against lcov line-by-line: that line has 3 hits, and no changed line is uncovered.db:migrations:check,db:schema-drift:check,selfhost:env-reference:check,docs:drift-check,coverage-boltons:check,dead-exports:check,manifest:drift-check.npm auditreports only pre-existing advisories transitive underrelease-please; this PR changes no dependencies.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails.The
/v1/internal/jobs/backfill-repo-segment/runresponse dropsnextCursorfor this segment, which is the point — it previously reported a continuation that did not exist. No other field changes shape.UI Evidence
Not applicable — no visible UI, frontend, docs, or extension change.
Notes
The defect is latent, not user-visible, which is why #10209 was filed separately from #10203 rather than folded into it. It bounds how much history the table can ever hold and makes the cursor/
expectedCountbookkeeping misleading to anyone reasoning about coverage — including the 2026-07-10 → 2026-07-23 hole #10193 opened, which no supported path can backfill. This PR does not change what is reachable; it stops the stored row claiming otherwise. Widening the window, if that is ever wanted, is a separate decision with a real GitHub-cost tradeoff.