fix(github): detect primary rate limits returned as 403 - #2776
Conversation
PR Summary by QodoFix GitHub primary rate limit detection (403) and reduce org-pool thrashing
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Site previewPreview: https://12c91f97-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · ❌ Terminated · Started 11:48 AM UTC · Ended 12:02 PM UTC |
|
🤖 Finished Review · ❌ Failure · Started 11:48 AM UTC · Completed 12:02 PM UTC |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. Rate-limit chain misdetected
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| func IsRateLimitError(err error) bool { | ||
| var apiErr *APIError | ||
| if !errors.As(err, &apiErr) { | ||
| return false | ||
| } | ||
| if apiErr.StatusCode == http.StatusTooManyRequests { | ||
| return true | ||
| } | ||
| if apiErr.StatusCode == http.StatusForbidden { | ||
| lower := strings.ToLower(apiErr.Message) | ||
| return strings.Contains(lower, "rate limit") | ||
| } | ||
| return false |
There was a problem hiding this comment.
1. Rate-limit chain misdetected 🐞 Bug ☼ Reliability
IsRateLimitError only treats 403s as rate limits when APIError.Message contains "rate limit", but LiveClient.do() returns a synthesized 403 APIError after retry exhaustion whose Message is "retryable error after ..." (no "rate limit"). As a result, acquireOrg may not break out on persistent rate limits and will run additional full retry cycles against more orgs, wasting time and quota.
Agent Prompt
## Issue description
`IsRateLimitError()` only matches 403 rate limits by substring search on `APIError.Message` ("rate limit"). However, when `LiveClient.do()` exhausts retries for a retryable 403, it returns an `APIError` with a synthesized message ("retryable error after ..."), which does not include "rate limit". That makes `gh.IsRateLimitError(err)` return false for the very rate-limit scenario it is intended to catch, so `acquireOrg()` may continue iterating other orgs and trigger additional expensive retry cycles.
## Issue Context
- `do()` decides retryability via `isRetryable()` based on status + headers/body.
- On the last attempt, `do()` returns `&APIError{StatusCode: resp.StatusCode, Message: msg}` where `msg` is a generated string.
- The PR’s new early-break in `acquireOrg()` depends on `IsRateLimitError(err)` being true.
## Fix Focus Areas
- internal/forge/github/github.go[105-117]
### Suggested implementation directions (pick one)
1) Expand `IsRateLimitError()` to also treat `403` APIErrors whose message starts with/contains "retryable error after" as rate limit (since `do()` only synthesizes that message for retryable responses).
OR
2) Preserve rate-limit classification in the synthesized error returned from `do()` (e.g., prefix the synthesized `Message` with "rate limit" for retryable 403/429, or wrap with a dedicated sentinel error), so `IsRateLimitError()` stays accurate without relying on brittle string matching.
Add/adjust a unit test to cover the `do()` retry-exhaustion error shape (403 + synthesized message) and confirm `IsRateLimitError()` returns true for it.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsMedium
Labels: Bug fix for rate-limit detection in the GitHub forge client with e2e test coverage changes. Previous runReviewFindingsHigh
Medium
Low
Labels: Bug fix for rate-limit detection in the GitHub forge client, with e2e test changes. |
GitHub sometimes returns primary rate limits as HTTP 403 with body text "API rate limit exceeded" instead of the expected 429. The isRetryable() check only matched "secondary rate limit" in 403 bodies, so these primary-as-403 errors passed straight through without any retry or backoff. Additionally, acquireOrg() had no rate-limit awareness — when one org hit a rate limit, it would immediately try all remaining orgs in the pool, burning through the same per-user quota. This prevented the rate limit from ever recovering. Two fixes: - Broaden isRetryable() to match "rate limit" (not just "secondary rate limit") in 403 response bodies, so do() applies the 60s+ backoff before returning to callers. - Add IsRateLimitError() helper and use it in acquireOrg() to break out of the inner org loop on rate-limit errors, since trying more orgs under the same user quota is pointless. Signed-off-by: Ryan Bean <rbean@redhat.com> Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
61a8f04 to
d27f367
Compare
|
🤖 Review · |
|
🤖 Review · |
The rebase brought in the envConfig refactor from main, which changed acquireOrg's signature. Update the rate-limit test to call acquireOrgWithClient instead. Also add the IsRateLimitError break to acquireOrgFromClient so the test-facing code path has the same rate-limit awareness as the production acquireOrg. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
c5cbc52 to
db969dd
Compare
|
🤖 Review · ❌ Terminated · Started 6:50 PM UTC · Ended 7:05 PM UTC |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 6:50 PM UTC · Completed 7:05 PM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — 4 findings (MEDIUM)
Agents: 6 (2x Claude, 2x Claude, 1x Gemini, 1x Codex) | Models: Claude, Gemini, Codex
The HIGH finding (
do()retry-exhaustion loses rate-limit signal forIsRateLimitError()) is already covered by an existing review comment — skipped.
See inline comments for MEDIUM findings.
Assisted-by: Claude (review), Gemini (review), Codex (review)
| return false, nil | ||
| } | ||
| if strings.Contains(strings.ToLower(string(data)), "secondary rate limit") { | ||
| if strings.Contains(strings.ToLower(string(data)), "rate limit") { |
There was a problem hiding this comment.
MEDIUM — Broadened "rate limit" match could be more precise (3-agent consensus: Claude, Gemini)
Changing from "secondary rate limit" to "rate limit" correctly catches primary-as-403, but is broader than needed. No known GitHub 403 messages trigger false positives today, but a more targeted disjunction would be more defensive:
lower := strings.ToLower(string(data))
if strings.Contains(lower, "rate limit exceeded") || strings.Contains(lower, "secondary rate limit") {
return true, nil
}This covers both known patterns ("API rate limit exceeded for user ID ..." and "You have exceeded a secondary rate limit") with less ambiguity. Acceptable as-is if the team prefers resilience to unknown variants.
Assisted-by: Claude (review), Gemini (review)
| for _, org := range shuffled { | ||
| acquired, err := tryCreateLock(ctx, client, org, runID, logf) | ||
| if err != nil { | ||
| if gh.IsRateLimitError(err) { |
There was a problem hiding this comment.
MEDIUM — Missing error log before rate-limit check (3-agent consensus: Claude)
This is the only one of the 4 identical IsRateLimitError + break blocks that does NOT log the error before checking. The other 3 sites (lines 90, 148, 194) all have:
logf("[org-pool] Error trying %s: %v", org, err)Without it, errors in this code path are silently swallowed — a rate-limit break logs "skipping remaining" but never shows the actual error.
if err != nil {
+ logf("[org-pool] Error trying %s: %v", org, err)
if gh.IsRateLimitError(err) {Assisted-by: Claude (review)
| // Secondary rate limits may include a Retry-After header, or may only be | ||
| // identifiable by the response body containing "secondary rate limit". | ||
| // GitHub uses 429 for primary rate limits and 403 for both primary and | ||
| // secondary rate limits. Rate-limit 403s may include a Retry-After header, |
There was a problem hiding this comment.
MEDIUM — Stale secondaryRateLimitBackoff naming and comments (5-agent consensus: Claude, Gemini, Codex)
The updated comment here correctly reflects that 403 covers both primary and secondary rate limits. However, the variable secondaryRateLimitBackoff (line 231) and its doc comment still say "secondary rate limits", and the retryDelay inline comment (line 248) says "For secondary rate limits (403)". These are now inaccurate since isRetryable() routes all 403 rate limits through that backoff.
Suggest renaming secondaryRateLimitBackoff → rateLimitBackoff403 and updating the downstream comments to match:
// rateLimitBackoff is the minimum backoff for rate-limit 403s
// (primary or secondary) when no Retry-After header is present.
var rateLimitBackoff = 60 * time.SecondAssisted-by: Claude (review), Gemini (review), Codex (review)
| if gh.IsRateLimitError(err) { | ||
| logf("[org-pool] Hit rate limit, skipping remaining orgs this round") | ||
| break | ||
| } |
There was a problem hiding this comment.
MEDIUM — Misleading "All orgs are locked" log after rate-limit break (2-agent consensus: Claude)
When this first-pass loop breaks due to a rate limit, execution falls through to line 126 which logs "All %d orgs are locked, polling with timeout %s". The orgs aren't locked — the loop exited due to a rate limit. This makes incident diagnosis harder from logs.
Suggest tracking whether the break was rate-limit-induced:
hitRateLimit := false
for _, org := range shuffled {
// ...
if gh.IsRateLimitError(err) {
logf("[org-pool] Hit rate limit, skipping remaining orgs this round")
hitRateLimit = true
break
}
}
if hitRateLimit {
logf("[org-pool] Rate limited, polling with timeout %s", timeout)
} else {
logf("[org-pool] All %d orgs are locked, polling with timeout %s", len(pool), timeout)
}Assisted-by: Claude (review)
|
🤖 Finished Retro · ✅ Success · Started 7:18 PM UTC · Completed 7:27 PM UTC |
Retro: PR #2776 — fix(github): detect primary rate limits returned as 403Overall: A well-scoped human-authored bug fix that went through 4 review agent runs (1 failure, 2 cancelled, 1 success) before merging. Two improvement opportunities identified. Timeline
Skipped proposals (covered by existing issues)
Proposals filed
|
3 participants
Summary
isRetryable()to match"rate limit"in 403 response bodies (not just"secondary rate limit"), sodo()applies the 60s+ backoff on primary rate limits that GitHub returns as 403 instead of 429.IsRateLimitError()helper to detect rate-limit errors through wrapped error chains.IsRateLimitError()inacquireOrg()to break out of the inner org loop on rate-limit errors — trying more orgs under the same per-user quota just burns API calls and prevents recovery.Context
CI failure on #2766 — the e2e test hit a primary rate limit returned as HTTP 403 with body
"API rate limit exceeded for user ID ...". BecauseisRetryable()only checked for"secondary rate limit",do()never retried. ThenacquireOrg()blasted through all 12 orgs in rapid succession (~20ms apart), each hitting the same user-level limit, repeating every 30 seconds for 10 minutes. The rapid-fire attempts prevented the rate limit from ever recovering.Independent of #2764 (retry count + jitter) — that PR improves retry behavior for errors that
do()does retry; this PR fixes the case wheredo()wasn't retrying at all.Test plan
TestIsRetryable_PrimaryRateLimitAs403— verifies 403 with "API rate limit exceeded" is retryableTestIsRetryable_403NotRateLimit— verifies non-rate-limit 403s are still non-retryableTestIsRateLimitError— verifies detection through wrapped errors, 429, primary-as-403, secondary-as-403TestAcquireOrg_RateLimitSkipsRemainingOrgs— verifies only 1 org is attempted before breaking out on rate limit🤖 Generated with Claude Code