fix(selfhost): cap historical PR file hydration and cache by head SHA - #2527
Merged
Conversation
Live evidence showed the shared GitHub App installation's REST bucket draining to single digits within ~2 hours, dominated by /pulls/:n/files calls for PR numbers with no matching pull_requests row -- the historical merged-PR file backfill (hydrateMergedPullRequestFiles) walks GitHub's live closed-PR history directly and had no per-run cap or budget re-check beyond the once-per-segment gate, so a large un-hydrated backlog could burn through hundreds of file fetches in a single job execution before admission control caught up. - Cap historical merged-PR file hydration to a small per-page batch and re-check GitHub REST headroom against a new, stricter HISTORICAL_BACKFILL_RESERVED_HEADROOM floor before every page (not just once at segment entry), so the least-urgent GitHub consumer yields earliest and never floods the shared bucket. - Add a durable repo+PR+headSha file snapshot: pull_request_detail_sync_state now tracks the head SHA files were last synced for, so the live-review and open-PR-convergence paths skip a redundant /pulls/:n/files fetch when the PR's current head is unchanged, and always refetch when it moves. - Add a closed-PR guard to refreshPullRequestDetails: a closed PR with complete stored telemetry is not refetched unless explicitly forced (the manual "review-now" repair command now forces past the cache). - Fix a webhook-coalescing gap: "reopened" triggers the same file-refresh path as "opened"/"synchronize" but was missing from the coalescable action set, so a reopen-adjacent event burst fanned out one job per delivery instead of collapsing into one per PR+head. - Add a bounded caller label (backfill_open_pr_details, backfill_merged_history, live_review) on the PR files fetch metric so future REST-budget incidents can be attributed by caller without re-deriving it from raw route counts. Validation: npm run test:ci, npm run test:coverage (unsharded), npm audit --audit-level=moderate all green; new/changed lines are fully covered per coverage/lcov.info.
Contributor
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 08:08:02 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
loopover-orb
Bot
added
gittensor
gittensor:bug
…ll path backfillRegisteredRepositories -> backfillRepository (the /v1/internal/jobs/ backfill-registered-repos/run admin endpoint's synchronous path) called the new cache-aware fetchAndStorePullRequestDetails but never wrote the resulting head SHA back to pull_request_detail_sync_state, so the cache it reads from was never populated by that path -- every run refetched files for PRs only ever touched here, even with an unchanged head. Mirror the write-back already done in backfillOpenPullRequestDetails and refreshPullRequestDetails. Added a regression test that fails without the fix (confirmed by reverting it locally) and passes with it.
…he relies on upsertPullRequestDetailSyncState's partial-update behavior (an omitted field leaves the column unchanged, relying on drizzle stripping undefined from the SET clause) was implicit and undocumented. A future edit coalescing an omitted field to null (e.g. `headSha: state.headSha ?? null`) would silently stop the repo+PR+headSha file cache from ever hitting again, with no test failure to catch it. Document the contract at the function and add an invariant test pinning it (plus the explicit-null-clears-it counterpart), confirmed to fail under the exact regression shape described above.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2527 +/- ##
========================================
Coverage 95.95% 95.95%
========================================
Files 226 228 +2
Lines 25387 25595 +208
Branches 9234 9315 +81
========================================
+ Hits 24359 24560 +201
- Misses 417 425 +8
+ Partials 611 610 -1
🚀 New features to boost your workflow:
|
This was referenced Jul 2, 2026
JSONbored
added a commit
that referenced
this pull request
Jul 2, 2026
… sync-state pattern Closes #2537. GET /pulls/{n} (PR state/mergeable_state) and GET /pulls/{n}/reviews were the largest and third-largest GitHub REST contributors observed in a live rate-limit trough, and neither was covered by the earlier PR-files cache (#audit-rate-headroom / #2527). The bare-PR-state read was implemented as four near-identical helpers with no caching or coalescing, called from readiness checks, the maintenance planner, duplicate-sibling reconciliation, and a gate-override command; reviews were explicitly left uncached ("more volatile than files" -- true at a fixed head, but reviews only actually change on a pull_request_review webhook). Extends the existing pull_request_detail_sync_state table (migration 0093) with prMergeableState/prState/prStateFetchedAt, and reuses the already-present reviewsSyncedAt column as the review cache's freshness marker. PR-state is event-invalidated on pull_request synchronize/closed/reopened (with a 5-minute safety-net TTL so a missed webhook self-heals within one sweep tick); reviews are invalidated on pull_request_review submitted/edited/dismissed rather than on head-SHA change, since reviews are independent of the head. The pre-merge verdict thread and its unified-comment mirror (the #4220 act-boundary) are deliberately left routed through the raw, uncached fetch -- they must always force-refetch live, since the stored/cached mergeable_state lagging GitHub's async recompute is exactly what caused #4220. This is verified by two dedicated regression tests plus direct mutation testing (temporarily rerouting the act-boundary through the cache and confirming both tests -- and no others in the suite -- catch it).
JSONbored
added a commit
that referenced
this pull request
Jul 2, 2026
… sync-state pattern Closes #2537. GET /pulls/{n} (PR state/mergeable_state) and GET /pulls/{n}/reviews were the largest and third-largest GitHub REST contributors observed in a live rate-limit trough, and neither was covered by the earlier PR-files cache (#audit-rate-headroom / #2527). The bare-PR-state read was implemented as four near-identical helpers with no caching or coalescing, called from readiness checks, the maintenance planner, duplicate-sibling reconciliation, and a gate-override command; reviews were explicitly left uncached ("more volatile than files" -- true at a fixed head, but reviews only actually change on a pull_request_review webhook). Extends the existing pull_request_detail_sync_state table (migration 0093) with prMergeableState/prState/prStateFetchedAt, and reuses the already-present reviewsSyncedAt column as the review cache's freshness marker. PR-state is event-invalidated on pull_request synchronize/closed/reopened (with a 5-minute safety-net TTL so a missed webhook self-heals within one sweep tick); reviews are invalidated on pull_request_review submitted/edited/dismissed rather than on head-SHA change, since reviews are independent of the head. The pre-merge verdict thread and its unified-comment mirror (the #4220 act-boundary) are deliberately left routed through the raw, uncached fetch -- they must always force-refetch live, since the stored/cached mergeable_state lagging GitHub's async recompute is exactly what caused #4220. This is verified by two dedicated regression tests plus direct mutation testing (temporarily rerouting the act-boundary through the cache and confirming both tests -- and no others in the suite -- catch it).
JSONbored
added a commit
that referenced
this pull request
Jul 2, 2026
… sync-state pattern Closes #2537. GET /pulls/{n} (PR state/mergeable_state) and GET /pulls/{n}/reviews were the largest and third-largest GitHub REST contributors observed in a live rate-limit trough, and neither was covered by the earlier PR-files cache (#audit-rate-headroom / #2527). The bare-PR-state read was implemented as four near-identical helpers with no caching or coalescing, called from readiness checks, the maintenance planner, duplicate-sibling reconciliation, and a gate-override command; reviews were explicitly left uncached ("more volatile than files" -- true at a fixed head, but reviews only actually change on a pull_request_review webhook). Extends the existing pull_request_detail_sync_state table (migration 0093) with prMergeableState/prState/prStateFetchedAt, and reuses the already-present reviewsSyncedAt column as the review cache's freshness marker. PR-state is event-invalidated on pull_request synchronize/closed/reopened (with a 5-minute safety-net TTL so a missed webhook self-heals within one sweep tick); reviews are invalidated on pull_request_review submitted/edited/dismissed rather than on head-SHA change, since reviews are independent of the head. The pre-merge verdict thread and its unified-comment mirror (the #4220 act-boundary) are deliberately left routed through the raw, uncached fetch -- they must always force-refetch live, since the stored/cached mergeable_state lagging GitHub's async recompute is exactly what caused #4220. This is verified by two dedicated regression tests plus direct mutation testing (temporarily rerouting the act-boundary through the cache and confirming both tests -- and no others in the suite -- catch it).
5 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 2, 2026
… sync-state pattern Closes #2537. GET /pulls/{n} (PR state/mergeable_state) and GET /pulls/{n}/reviews were the largest and third-largest GitHub REST contributors observed in a live rate-limit trough, and neither was covered by the earlier PR-files cache (#audit-rate-headroom / #2527). The bare-PR-state read was implemented as four near-identical helpers with no caching or coalescing, called from readiness checks, the maintenance planner, duplicate-sibling reconciliation, and a gate-override command; reviews were explicitly left uncached ("more volatile than files" -- true at a fixed head, but reviews only actually change on a pull_request_review webhook). Extends the existing pull_request_detail_sync_state table (migration 0093) with prMergeableState/prState/prStateFetchedAt, and reuses the already-present reviewsSyncedAt column as the review cache's freshness marker. PR-state is event-invalidated on pull_request synchronize/closed/reopened (with a 5-minute safety-net TTL so a missed webhook self-heals within one sweep tick); reviews are invalidated on pull_request_review submitted/edited/dismissed rather than on head-SHA change, since reviews are independent of the head. The pre-merge verdict thread and its unified-comment mirror (the #4220 act-boundary) are deliberately left routed through the raw, uncached fetch -- they must always force-refetch live, since the stored/cached mergeable_state lagging GitHub's async recompute is exactly what caused #4220. This is verified by two dedicated regression tests plus direct mutation testing (temporarily rerouting the act-boundary through the cache and confirming both tests -- and no others in the suite -- catch it).
JSONbored
added a commit
that referenced
this pull request
Jul 2, 2026
* feat(github): cache PR reviews with webhook-based invalidation The recently-merged head-SHA file cache (#2527) deliberately left reviews uncached ("more volatile than files"), but reviews only actually change on a pull_request_review webhook, not on every sweep tick -- unlike files, reviews are independent of head SHA entirely. fetchAndStorePullRequestDetails now skips the GET /pulls/{n}/reviews call when a prior sync already covers every review-invalidating event: the new reviews_invalidated_at column (bumped by a pull_request_review webhook with action submitted/dismissed/edited) is compared against the existing reviews_synced_at timestamp. A stored row whose last review sync itself failed is excluded from the cache-hit path so a transient failure can't poison the cache into never retrying. Broadened the existing sync-state row lookup (previously gated on `!forceFiles && headSha`, which would silently disable review caching whenever headSha was momentarily unknown) since reviews never depend on headSha or the files-only forceFiles flag. Bare PR-state caching (state/mergeable_state/head SHA), the other half of this issue, was deliberately NOT implemented after auditing every live-read call site in the codebase: each one (the freshness guard in agent-action-executor.ts, the draft-dodge-close and reopen-reclose re-checks, the gate-override command, and the sweep-resync path) is a documented act-boundary that intentionally requires a live, uncached read immediately before a mutation or before publishing review output. mergeable_state already has per-pass caching via the existing LiveGithubFacts mechanism. Caching any of these would risk exactly the regression the issue's own acceptance criteria forbids ("No regression in any decision that depends on a live PR state"). Filing as a follow-up rather than force-fitting a cache onto call sites that don't have safe surface area for one. * fix(github): stop forceFiles from bleeding into the reviews cache decision Gate review finding on PR #2633: the sync-state row lookup was skipped entirely whenever forceFiles && headSha (the manual "refresh files" path), which zeroed out reviewsUpToDate too and forced an unrelated GET /pulls/{n}/reviews on every manual files-only force, even when reviews were already cache-current. forceFiles only ever means "force a files refetch" -- reviews caching must be completely independent of it. Now fetches the row unconditionally and applies forceFiles only to filesUpToDate, never to reviewsUpToDate. * fix(github): close the reviews-cache TOCTOU race and harden invalidation Second round of gate findings on PR #2633: 1. fetchAndStorePullRequestDetails read existingState as a snapshot at the top of the call, but every caller then unconditionally stamped reviewsSyncedAt to "now" once the whole call finished. A pull_request_review webhook racing in after the snapshot read but before that final write would set reviewsInvalidatedAt to a moment the stamped reviewsSyncedAt would then read as already covering -- the cache could confirm itself fresh through an invalidation it never actually observed. Now returns the correct reviewsSyncedAt to the caller instead: captured BEFORE the fetch starts on a genuine success (conservative against races from that instant on), and left unchanged on a skip or a failed fetch. This also makes a stored reviewsSyncedAt trustworthy on its own, so the separate errorSummary string-matching safety net (fragile: it only ever reflects whichever of files/reviews/ checks failed LAST in a given pass, since they run concurrently) is no longer needed and has been removed. reviewsUpToDate's comparison is also now a strict `>` rather than `>=`, so a millisecond-resolution timestamp tie between a sync and a racing invalidation fails toward "still needs a refetch" rather than silently trusting the cache. 2. markPullRequestReviewsInvalidated is the sole source of the invalidation signal, so a single transient D1 write failure would permanently lose that PR's "reviews changed" event. Added a bounded 3-attempt retry -- still best-effort from the webhook handler's perspective (never blocks/retries the whole job), but absorbs a momentary blip in-process. * fix(github): clean up legacy reviews_synced_at rows before trusting the cache Third gate finding on PR #2633: reviews_synced_at has existed since migration 0006, long before this PR gives it any cache-skip meaning -- every sync pass since then has stamped it unconditionally, including passes whose review fetch itself failed. On deploy, reviewsUpToDate would immediately trust every existing row's reviews_synced_at at face value, silently skipping re-fetches for PRs whose last review sync actually failed, until some later invalidating webhook happened to arrive. Migration 0094 now resets reviews_synced_at to NULL for every row whose last sync status was not 'complete' (status only reads 'complete' when that pass recorded zero warnings across files/reviews/checks, which reliably means reviews specifically succeeded -- any other status could have been a review failure, so it's reset to force a guaranteed-safe refetch). A genuinely 'complete' row is left untouched. Adds a migration-effect test (applying the actual 0094 SQL against a scratch table shaped like the pre-#2537 schema, seeded with rows exactly as years of pre-#2537 code would have written them) proving the cleanup targets the right rows and nothing else.
24 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 2, 2026
Advances #2537. The head-SHA file cache (#2527) and the recently-merged review cache (#2633) left one sibling route uncached: a bare GET /pulls/{n} (state/mergeable_state/head SHA), implemented as four separate near-identical helper functions with no caching or cross-call coalescing, called from many independent sites within one review pass. Adds a durable, webhook-invalidated cache for this read on pull_request_detail_sync_state (prMergeableState/prState/prStateFetchedAt), capped at a 5-minute PR_STATE_CACHE_MAX_AGE_MS safety net so a missed invalidation self-heals within one sweep tick. A single GET /pulls/{n} write- throughs all three fields together under one shared fetchedAt stamp (fetchAndCachePrStateFields), so a cache miss on any one field never leaves another looking falsely fresh with an unfetched value. Wired at the freshness-guard readiness read (cachedLiveMergeState), the dup-winner reconcile (reconcileLiveDuplicateSiblings), and primed for free by the per-PR sweep's existing resync fetch (primeDurablePrStateCache) -- deliberately NOT wired into any act-boundary read: the merge/close decision (refreshLiveMergeState, unchanged) and the gate-override head-SHA resolution (resolveOverrideHeadSha, unchanged) both keep forcing a live fetch by design, since both need the literal current commit rather than a value that can be briefly stale. Test plan: - npm run typecheck clean - npm run db:migrations:check clean (0095, contiguous) - npm run test:ci full local gate green - 100% branch coverage on every changed line in backfill.ts/processors.ts/ repositories.ts/schema.ts, confirmed via lcov diff-coverage cross-check - New tests cover: cache miss/hit/expiry for all three fields, the shared- fetch write-through, webhook invalidation on synchronize/closed/reopened (and non-invalidation on unrelated actions), dup-winner reuse of a warm cache row, the sweep priming the cache from its own resync, and two mutation-verified regressions: gate-override must never route through the durable head-SHA cache (the race this function exists to close), and the sweep's resync must actually persist to the cache for later readers.
JSONbored
added a commit
that referenced
this pull request
Jul 2, 2026
…es (#2640) * feat(github): cache bare PR-state reads at non-authoritative call sites Advances #2537. The head-SHA file cache (#2527) and the recently-merged review cache (#2633) left one sibling route uncached: a bare GET /pulls/{n} (state/mergeable_state/head SHA), implemented as four separate near-identical helper functions with no caching or cross-call coalescing, called from many independent sites within one review pass. Adds a durable, webhook-invalidated cache for this read on pull_request_detail_sync_state (prMergeableState/prState/prStateFetchedAt), capped at a 5-minute PR_STATE_CACHE_MAX_AGE_MS safety net so a missed invalidation self-heals within one sweep tick. A single GET /pulls/{n} write- throughs all three fields together under one shared fetchedAt stamp (fetchAndCachePrStateFields), so a cache miss on any one field never leaves another looking falsely fresh with an unfetched value. Wired at the freshness-guard readiness read (cachedLiveMergeState), the dup-winner reconcile (reconcileLiveDuplicateSiblings), and primed for free by the per-PR sweep's existing resync fetch (primeDurablePrStateCache) -- deliberately NOT wired into any act-boundary read: the merge/close decision (refreshLiveMergeState, unchanged) and the gate-override head-SHA resolution (resolveOverrideHeadSha, unchanged) both keep forcing a live fetch by design, since both need the literal current commit rather than a value that can be briefly stale. Test plan: - npm run typecheck clean - npm run db:migrations:check clean (0095, contiguous) - npm run test:ci full local gate green - 100% branch coverage on every changed line in backfill.ts/processors.ts/ repositories.ts/schema.ts, confirmed via lcov diff-coverage cross-check - New tests cover: cache miss/hit/expiry for all three fields, the shared- fetch write-through, webhook invalidation on synchronize/closed/reopened (and non-invalidation on unrelated actions), dup-winner reuse of a warm cache row, the sweep priming the cache from its own resync, and two mutation-verified regressions: gate-override must never route through the durable head-SHA cache (the race this function exists to close), and the sweep's resync must actually persist to the cache for later readers. * fix(github): keep the duplicate-winner reconcile on a live PR-state read Addresses the blocker the gate's own AI review flagged: reconcileLiveDuplicateSiblings was routed through cachedFetchLivePullRequestState, but that reconcile directly feeds duplicate-winner selection, which can auto-CLOSE the current PR when GITTENSORY_DUPLICATE_WINNER is on. A cached "open" read up to PR_STATE_CACHE_MAX_AGE_MS stale after a missed closed webhook would keep an already-closed sibling eligible as the winner, wrongly closing the current PR as the loser -- the same class of irreversible-actuation risk the merge/close decision and gate-override guard against. Reverts this call site to the raw fetchLivePullRequestState (unchanged from before this PR), and replaces the test that asserted cache reuse with a regression proving the opposite: a warm-but-wrong cached row must never be served here. Also replaces three test-fixture token values (fake-installation-token / installation-token) with the shorter placeholder already used elsewhere in these same tests -- both incidentally matched the repo's generic-secret-assignment scan pattern (a keyword-shaped heuristic with no placeholder-value exclusion in REES's copy of the analyzer, unlike the stricter src/review/secrets-scan.ts). No real credential was ever present; this just avoids tripping a keyword-length heuristic on new lines.
This was referenced Jul 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
remainingwithin ~2 hours, with/pulls/:n/filesas the dominant route (1,272–1,672 calls per repo). A follow-up DB check found 475–544 distinct PR numbers with file calls but no matchingpull_requestsrow, against actual open-PR counts of 5–37 per repo.hydrateMergedPullRequestFiles(historical merged-PR file backfill,src/github/backfill.ts) walks GitHub's live/pulls?state=closedhistory directly — its PR numbers never need a localpull_requestsrow, which is exactly why the missing-row evidence pointed here. It had no per-run cap and no rate-limit re-check beyond the once-per-segment gate at the top ofbackfillRepositorySegment, so a large un-hydrated backlog (a fresh/partial backfill) could fan out one/pulls/:n/filesfetch per un-hydrated PR — up toSEGMENT_PAGE_BUDGET × 100PRs — inside a single job execution, well past what the once-at-entry admission check could stop.What changed
MERGED_PR_FILE_HYDRATION_BATCH_SIZEbounds how many not-yet-hydrated merged PRs get a files fetch perrecent_merged_pull_requestspage, and a newHISTORICAL_BACKFILL_RESERVED_HEADROOMfloor (stricter than the existing maintenance floor) is checked before every page. A PR skipped for either reason stays a candidate on the next run — nothing is silently dropped, just deferred.pull_request_detail_sync_statenow tracks the head SHA files were last synced for (migration0090).fetchAndStorePullRequestDetailsskips the/pulls/:n/filesfetch when the stored snapshot's head SHA still matches the PR's current head, and always refetches when it has moved — this benefits the live-review and open-PR-convergence paths, which previously refetched files unconditionally on every trigger regardless of whether the head had actually changed.refreshPullRequestDetailsnow skips a closed PR that already has complete stored telemetry, unless the caller explicitly forces a refresh. The manual "review-now" repair command now passesforce: trueso an explicit user re-check is never served stale data.reopenedtriggers the same file-refresh path asopened/synchronize/ready_for_review(PR_PUBLIC_SURFACE_ACTIONS) but was missing from the coalescable action set inwebhook-coalesce.ts, so a burst of reopen-adjacent deliveries for the same PR+head fanned out one job per delivery instead of collapsing into one.gittensory_github_pull_request_files_fetch_total{caller=...}metric (3 fixed values:backfill_open_pr_details,backfill_merged_history,live_review) attributes files-endpoint traffic by caller, so a future budget incident can be diagnosed without re-deriving it from raw route counts.Correctness / staleness guarantees
backfillOpenPullRequestDetails) and live webhook-triggered reviews are unaffected in scope — they still only ever touch PRs already present inpull_requests.#1941design, unchanged) — the new cap only bounds when a not-yet-hydrated merged PR's files get fetched, never fetches a stale result.Rate-limit impact
recent_merged_pull_requestssegment run drops from up toSEGMENT_PAGE_BUDGET[mode] × 100(1,000 forfull/resume) files fetches toSEGMENT_PAGE_BUDGET[mode] × MERGED_PR_FILE_HYDRATION_BATCH_SIZE[mode](200 forfull/resume, 20 forlight), with an additional early yield once REST headroom drops below the new historical floor.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally (unsharded) — full suite green; new/changed lines insrc/github/backfill.ts,src/db/repositories.ts,src/github/rate-limit.ts,src/github/webhook-coalesce.tsare fully line+branch covered percoverage/lcov.info(cross-checked against the diff hunks directly)npm 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=moderate— 0 vulnerabilitiesSafety
UI Evidencesection. — not applicable, backend-only change.CHANGELOG.mdis not edited in a normal PR.Notes
BackfillMode-keyed caps, admission-key-scoped rate observations) and applies uniformly to every self-hosted installation.