fix(#6702): observe GitHub rate-limit state and name it in exhausted-retry errors - #6705
Conversation
PR Summary by QodoObserve GitHub rate limits and classify exhausted retries
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
|
🤖 Review · Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
204f501 to
58137f4
Compare
|
🤖 Finished Review · ✅ Success · Started 9:48 PM UTC · Completed 10:08 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.38 |
|
Risk Assessment: moderate (2/5) DetailsMedium-sized additive change (357 lines, 8 files) adding rate-limit observability. No protected paths, no security-sensitive files, no CI/dependency changes. Forge core files are high-churn hotspots but changes are purely additive instrumentation. Well-scoped step 1 of a 2-step bug fix with clear issue linkage. Re-review anchoring: signals unchanged from prior assessment. Previous runRisk Assessment: moderate (2/5) DetailsMedium-sized additive change (332 lines, 8 files) adding rate-limit observability to the GitHub client and behaviour-test harness. No protected paths, no security-sensitive files, no CI/dependency changes. The forge core files are high-churn hotspots with many recent authors and fix commits, elevating git-history risk. However, the change is purely additive instrumentation (step 1 of 2) with no behavioral modification, well-scoped to the linked issue, and includes tests. Overall moderate risk. |
ReviewFindingsLow
Previous runReviewFindingsLow
Labels: PR modifies the forge client rate-limit handling and e2e test infrastructure |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
58137f4 to
1d5edd0
Compare
|
🤖 Review · Commit: |
…retry errors Two behaviour-test runs on different pool orgs went blind ~10 minutes into the suite: every listing behind the harness waits failed on 403 until the wait timed out (#6697, #6702). Diagnosing which limit fired and who spent the budget was impossible from the logs because the GitHub client never read X-RateLimit-Remaining/Reset, and the error it returns after exhausting retries — "403 retryable error after 5 attempts" — does not even satisfy its own IsRateLimitError. Record the X-RateLimit-* headers of every response on the LiveClient and expose them through forge.RateLimitReporter; prefix the exhausted-retry error for 429 and retryable 403 with "rate limit:" and append the last observed budget, so IsRateLimitError matches it and callers that only see the error can tell a rate limit from a generic 403 (the org-pool acquisition already backs off on IsRateLimitError and now gets to). The behaviour suite's ensurer logs the budget before every allocation, which samples the drain across the whole run. This is the instrumentation step of #6702; the fix (conditional requests, a remaining-budget circuit breaker, or lower parallelism) is chosen from the numbers this produces. Refs #6702 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
1d5edd0 to
1479dbe
Compare
|
🤖 Finished Review · ✅ Success · Started 12:12 AM UTC · Completed 12:31 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.15 |
|
For the core reviewer: this is ready and auto-merge is armed — your approval is the only remaining gate (CODEOWNERS; I can't approve my own PR). What to look at, in order of substance:
The samples already paid for themselves on this PR's own behaviour run: |
|
🤖 Finished Retro · ✅ Success · Started 7:22 AM UTC · Completed 7:37 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.67 |
Retro: PR #6705 — Rate-limit instrumentationWorkflow: Human-authored PR by waynesun09 adding rate-limit observability to the GitHub client (refs #6702). No code, triage, or fix agent was involved — only the fullsend review agent. Timeline: PR opened 2026-08-27. qodo-code-review caught a real concurrency bug (error messages reported a stale client-wide rate-limit observation instead of the failing response's own headers) on the initial commit. The author fixed it within ~2.5 hours and addressed all review findings across two follow-up pushes. The fullsend review agent completed two reviews ($7.38 on commit 58137f4, $8.15 on final commit 1479dbe), approving both times with valid low-severity findings (import ordering, edge-case handling, scope-creep acknowledgment, comment style). Two additional review runs were cancelled on intermediate commits. Human reviewer rh-hemartin approved 2026-08-31; merged same day. Review quality: Appropriate. All review agent findings were valid and acted upon. The agent never saw the original concurrency bug because its first run was cancelled before completing — by the time it reviewed (commit 58137f4), the fix was already in place. The second completed review cost $8.15 but surfaced only one new low-severity finding beyond what the first review had already flagged. Evidence for existing issues:
Proposals filed
|
The harness-wait poll loop (and its diagnostics) re-request the same workflow-runs/jobs/artifacts URLs every few seconds, from up to a dozen concurrent scenarios sharing one installation token. #6705's instrumentation measured that traffic draining the primary quota ~235 req/min, exhausting it ~20 minutes into a suite run. GitHub does not count a 304 response against the primary rate-limit budget (verified against the live API: repeated If-None-Match requests left X-RateLimit-Remaining unchanged, only the initial uncached GET consumed one unit). This adds a small conditional-GET cache to LiveClient (etagCache, opt-in per path via getCached) and wires it into the five GET endpoints the harness-wait poll loop and its diagnostics use: ListWorkflowRuns, ListRecentWorkflowRuns, ListWorkflowRunJobs, ListWorkflowRunArtifacts, and ListRepositoryArtifacts. Unchanged results between polls now cost nothing; a status change still forces a full re-fetch, since GitHub issues a new ETag whenever the underlying data changes. do() grows a variadic requestHeader option so getConditional can set If-None-Match without touching its other 28 call sites. The cache is capped (etagCacheLimit) since a long suite run touches many distinct run/job/artifact URLs. Deliberately out of scope: a remaining-budget circuit breaker (the issue's second candidate). If post-merge behaviour runs still show #6698's 403 diagnostics after this lands, that's the next step, sized with real data instead of a guessed threshold. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
2 participants
Summary
Instrumentation step for the first of the two real causes behind the #6647 behaviour-test failures (diagnosed on #6697): the pool-org installation token's API budget is exhausted ~10 minutes into the suite, after which the harness waits poll blind. Two runs on different orgs (halfsend-10 and halfsend-06) showed the same onset; the second already had #6669's backoff.
Which limit fires (primary hourly vs secondary) and who spends the budget cannot be told from today's logs: the client never reads
X-RateLimit-*, and its exhausted-retry error (403 retryable error after 5 attempts …) does not satisfy its ownIsRateLimitError. This PR makes the numbers visible so the fix (conditional requests / circuit breaker / parallelism) is chosen from evidence rather than guessed — which is how #6697 went wrong the first time.Related Issue
Refs #6702 (left open for the fix step)
Changes
forge.RateLimit+forge.RateLimitReporter: the last observed primary-quota state (X-RateLimit-Limit/Remaining/Reset/Resource), with aString()that renders absent fields asunknownrather than plausible zeros.LiveClientrecords the headers of every response and implements the reporter.rate limit: retryable error after N attempts on … [remaining=0/5000 reset=… resource=core], soIsRateLimitErrormatches it. The budget in the brackets is parsed from the response that failed; when that response carries noX-RateLimit-*(secondary-limit shape), the message says so and names the last observation with its age — a stale value is never presented as the failing response's budget. 5xx exhaustion is unchanged.pkg/e2etest): with mint, tokens are minted per org so the limit is per org — a rate-limited org is now skipped for the next one instead of aborting the round (the round still records the limit, so if every org turns out limited the existing between-round back-off applies); the shared-PAT path is unchanged. Without this, making the exhausted-retry 403 recognisable would have activated a stale "limits are per user" assumption.[driver] rate limit after allocating|deallocating <org/repo>: remaining=…on every allocation and release (not just the first ensure per repo, which theensuredcache short-circuits), so the samples span the whole run.Testing
go vet,gofmt;go test -race ./internal/forge/... ./pkg/behaviourtest/drivers/install/ ./pkg/e2etest/TestDo_ObservesRateLimitHeaders,TestDo_ResponseWithoutRateLimitHeadersKeepsPreviousObservation,TestDo_RateLimitExhaustedErrorSelfIdentifies,TestDo_RateLimitExhaustedWithoutHeadersNamesLastObservation(secondary-limit shape),TestDo_ServerErrorExhaustedIsNotARateLimit,TestRateLimitString_UnknownFields,TestComposedDriver_SamplesRateLimitOnAllocateAndDeallocateFollow-ups (not here)
forge.IsTransientstill excludes exhausted rate limits, sosteps/cleanup.gowill not retry them; now that the error self-identifies, it could consultIsRateLimitError.Checklist
!for breaking changes)