GitHub rate-limit amplification fix (t3 #5673 twin) - #1061
Conversation
Two violations of the invariants t3code documented after their own throttling incident, both confirmed against ADE's code. **A failure bought zero quiet.** `cachedGithubSnapshot` published only on success, so a success bought 120s of quiet and a failure bought nothing: once GitHub throttled us, nothing republished `cachedGithubSnapshotAt`, the snapshot stayed stale forever, and every UI-driven read re-armed the fetch. ADE asked *faster* while degraded than while healthy. The poller's backoff never covered those callers. New `githubReadBackoff.ts` holds one keyed ladder (20s -> 40s -> ... capped at 15 min) shared by the whole-repo snapshot and per-branch lookups. Every rung is floored at the window a *success* would have bought — that floor, not the doubling, is what closes the amplification. **Never-pushed lanes cost a GitHub call each.** Per-branch lookups for lanes with no known PR spent one call per snapshot rebuild even when the branch had never left the machine. One `git for-each-ref refs/remotes/` (~20ms, one subprocess regardless of lane count) now filters those out *before* the 12-lookup budget applies. Keyed on remote-tracking refs, not upstream config: `git push` writes the tracking ref without `-u`. A failed probe keeps the lookup — hiding a PR badge is worse than spending a call — and the history sweep is exempt, since GitHub deletes the head branch on merge and those are exactly the PRs that sweep exists to recover. Measured on this machine: 13 of 23 lane worktree branches (57%) are never-pushed, so more than half the targeted-lookup budget was being spent on branches that cannot have a PR. t3 measured 66% in the same state. Also from review: - `automaticRefresh` opt-out plumbed renderer -> preload -> IPC and through the sync remote command service. `force` cannot be read as user intent: focus reconcile, the poller, the GitHub tab's hot-refresh timer, and two LanesPage effects all pass it automatically. Modelled as an opt-out so a transport that drops the field degrades to today's behavior rather than disabling the user's Refresh button. - Only GitHub arms the GitHub ladder; a lane/SQLite hiccup no longer silences GitHub reads or replays a local error as an API failure. - A dedicated auth generation replaces the cache epoch on the failure path — the epoch bumps on webhooks and PR mutations, so a routine invalidation landing mid-fetch used to swallow a real failure. - The renderer's own freshness ref now stamps on failure, so `prs-updated` stops re-firing a forced snapshot on every poll tick. Two named invariants have regression tests: a failure TTL is never shorter than the success TTL, and a lookup that falls back to cached data never reports success upstream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 14 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
bb83103 to
7f8df32
Compare
|
Thanks — this is real, and it's the deliberate trade-off. Recording why it stands rather than being fixed: Scope. The skip runs only on the open-list path, and only over candidates that already exclude any branch with a local PR row ( Recovery is bounded. Any There is no cheap correct fix. Every signal available is local state: The invariant the code follows is "a failed probe keeps the lookup"; a successfully-read-but-stale ref set is genuinely indistinguishable from a fresh one, so extending that to cover it would mean never skipping at all — which is the amplification this PR exists to remove (13 of 23 lane branches on the author's machine have no remote ref). Left as-is and documented at the call site. If the missing-badge window ever shows up in practice, the fix is upstream of this filter — make the lane list's fetch cadence guarantee freshness — not to disable the skip. |
1 participant
Item 1 of three independent hygiene fixes from the 2026-08-09 t3code competitor research (§3.2). Items 2 and 3 follow as separate PRs from this lane.
The bug
Two violations of the invariants t3 documented after their own throttling incident, both verified against ADE's code on current main.
1. A failure bought zero quiet.
cachedGithubSnapshotwas published only in the success path. A success bought 120s (GITHUB_SNAPSHOT_TTL_MS); a failure bought nothing — nothing republishedcachedGithubSnapshotAt, so the snapshot stayed stale forever and every UI-driven read (list reads, lane-visibility refresh, focus reconcile) re-armed the fetch. Once GitHub throttled us, ADE asked faster than when healthy. The poller's exponential backoff never covered those callers, andgithubBackgroundRequestPauseUntilMsis a reserve floor, not a fix.2. Never-pushed lane branches cost a GitHub call each. Per-branch PR lookups for lanes with no known PR (
MAX_TARGETED_LANE_PR_BRANCH_LOOKUPS = 12) ran for branches that had never left the machine, and failures were not negatively cached — one wasted call per snapshot rebuild, per branch.Before / after
for-each-ref, ~20ms, one subprocess regardless of lane countMeasured on this machine's real ADE checkout: 13 of 23 lane worktree branches (57%) have no remote-tracking ref — more than half the 12-slot targeted-lookup budget was being spent on branches that cannot have a PR, starving the lanes that do. t3 measured 66% of their worktrees in the same state.
What changed
New
apps/desktop/src/main/services/prs/githubReadBackoff.ts— one keyed failure ladder (20s → 40s → 80s → … capped at 15 min) shared by the whole-repo snapshot (snapshot:<repo>) and per-branch lookups (branch:<repo>#<branch>). Every rung is floored at the window a success would have bought. That floor, not the doubling, is what closes the amplification, so the first rungs are 120s rather than t3's 20s.The never-pushed skip runs one
git for-each-ref --format=%(refname:lstrip=3) refs/remotes/at the project root (lane worktrees share the common git dir). Deliberately keyed on remote-tracking refs, not upstream config —git pushwrites the tracking ref even without-u. One enumeration rather than t3's pair of--count=1probes: one subprocess regardless of lane count, and nofor-each-refglob built out of a branch name. A failed probe keeps every lookup, and the history sweep is exempt — GitHub deletes the head branch on merge, so the merged PRs that sweep exists to recover are exactly the ones with no remote-tracking ref left.Found during review, also fixed
forcecannot be read as user intent. Focus reconcile, the poller's lane discovery, the GitHub tab's hot-refresh timer, and two LanesPage effects all passforce: trueautomatically. AnautomaticRefreshopt-out is now plumbed renderer → preload → IPC and through the sync remote command service. Modelled as an opt-out on purpose: a transport that drops the field degrades to today's behavior rather than silently disabling the user's Refresh button in a packaged build.lastSnapshotLoadedAtRefwas written only on success, so theprs-updatedguard never tripped while GitHub was down and every poll tick re-fired a forced snapshot plus a hot-refresh timer.Invariants under test
Both of t3's stated invariants have named regression tests:
githubReadBackoff.test.ts :: never lets a failure buy less quiet than a successprService.test.ts :: backs a failed lane branch lookup off without starving healthy branches— the failed lookup returnsnull, not[]; reporting "this branch has no PRs" was the success-shaped answer that defeated the backoff (t3's subtler bug).Verification
/quality: 26 findings across 3 review passes, all applied, gate empty. Two were High and both were defects in my own first fix — the flag not reaching production transports, andforce-as-user-intent./test: 736 tests across 35 affected files pass. Desktop shard 1/8: 1591 pass, 1 pre-existing flake (laneProxyServiceE2E port reachability — fails identically with this branch stashed).validate-docs.mjs: 228 files).runGithelper (structured argv,windowsHide,resolveGitExecutable); no path construction or comparison added.🤖 Generated with Claude Code
Greptile Summary
This PR adds shared failure backoff for GitHub snapshot and per-branch reads, distinguishes automatic forced refreshes from user retries, and reduces targeted lookups for apparently unpublished lane branches.
Confidence Score: 4/5
The PR is not yet safe to merge because the previously reported local-ref filter can still suppress real GitHub PRs created from another checkout.
The targeted lookup path enumerates only local
refs/remotes/state and filters out absent branches without first establishing that those refs are current, leaving the previously reported PR-discovery failure outstanding.Files Needing Attention: apps/desktop/src/main/services/prs/prService.ts
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI participant PR as PR Service participant Git as Local Git participant GH as GitHub UI->>PR: Request snapshot PR->>PR: Check snapshot failure ladder alt Snapshot is eligible for refresh PR->>GH: Fetch repository PR snapshot PR->>Git: Enumerate remote-tracking refs PR->>GH: Fetch eligible lane branches PR-->>UI: Return refreshed snapshot else Snapshot is backed off PR-->>UI: Return cached data or recorded failure endReviews (2): Last reviewed commit: "fix(prs): stop GitHub rate-limit amplifi..." | Re-trigger Greptile