Fix rate-limit errors aborting multi-page searches instead of waiting and retrying#103
Merged
Fix rate-limit errors aborting multi-page searches instead of waiting and retrying#103
Conversation
… and retrying Three root causes addressed: 1. fetchWithRetry threw immediately when x-ratelimit-reset indicated a wait longer than MAX_AUTO_RETRY_WAIT_MS (10 s), aborting the entire paginatedFetch loop and losing all results fetched so far. Add an optional onRateLimit callback to fetchWithRetry: when provided, the function calls the callback with the wait duration (ms), sleeps for that duration without counting it as a retry attempt, then resumes. Callers without a callback retain the existing throw behaviour. 2. Secondary rate limits (403 + Retry-After, no x-ratelimit-remaining header) were not detected by isRateLimitExceeded, so they bypassed fetchWithRetry's retry logic and surfaced as unhandled API errors. Extend isRateLimitExceeded to treat any 403 with a Retry-After header as a (secondary) rate limit. 3. When total_count is an exact multiple of 100 (e.g. 1000 results / 10 pages), paginatedFetch would attempt page 11 after the last full page, which GitHub rejects with 422 "Cannot access beyond the first 1000 results". Guard fetchAllResults with an early return [] when page > totalPages. Thread the onRateLimit callback through searchCode → fetchAllResults → the entry point, where it writes a yellow message to stderr so the user sees the pause. Also add 1 s clock-skew buffer to x-ratelimit-reset computation. Fixes #102
|
Coverage after merging fix/auto-retry-on-rate-limit into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
Improves the CLI’s resilience during multi-page GitHub code searches by handling both primary and secondary rate limits more gracefully and preventing pagination past GitHub’s 1000-result cap.
Changes:
- Extend rate-limit detection to include secondary rate limits (
403 + Retry-After) and add a small clock-skew buffer forx-ratelimit-reset. - Add an optional
onRateLimitcallback pathway to allow long waits to be surfaced to users and automatically resumed (without throwing). - Prevent requesting an extra page when
total_countis an exact multiple of 100 (e.g., 1000 results → avoid page 11 / 422).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/api-utils.ts | Enhances rate-limit detection/delay calculation and adds callback-based long-wait handling to fetchWithRetry. |
| src/api.ts | Threads onRateLimit through search/pagination and guards against page overflow beyond computed total pages. |
| src/api-utils.test.ts | Adds tests covering secondary rate-limit handling and the onRateLimit long-wait path. |
| github-code-search.ts | Plumbs an onRateLimit callback from the CLI to show a user-visible “waiting…” message. |
When --group-by-team-prefix was used, fetchRepoTeams called fetchWithRetry without an onRateLimit callback, causing an immediate throw instead of waiting and retrying. - Thread onRateLimit through fetchRepoTeams (list teams + repos pages) - Extract the rate-limit countdown into a named const onRateLimit so it is reused by both fetchAllResults and fetchRepoTeams - Fix broken test (maxRetries=1 → 5 with new longWaitAttempts cap) - Add test: throws after maxRetries long-wait attempts - Add test: page-11 guard for total_count=1000 (10 full pages)
|
Coverage after merging fix/auto-retry-on-rate-limit into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Replace urlStr.includes("raw.githubusercontent.com") with strict
hostname equality check new URL(urlStr).hostname === "raw.githubusercontent.com"
to prevent false positives from attacker-controlled URLs containing
the substring (e.g. evil.com/raw.githubusercontent.com/...).
Reported by GitHub Advanced Security code scanning (issue #18).
|
Coverage after merging fix/auto-retry-on-rate-limit into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- abortableDelay: extract abort handler to named var so it is removed via removeEventListener when the timer fires, preventing listener accumulation across many retries - fetchWithRetry / searchCode / fetchAllResults / fetchRepoTeams: narrow onRateLimit type from (void | Promise<void>) to Promise<void> and document that the callback MUST sleep for at least waitMs ms; a log-only callback would exhaust longWaitAttempts immediately - github-code-search.ts: add activeCooldown deduplication — concurrent Promise.all callers in fetchRepoTeams that hit a rate limit together now share the same countdown promise instead of interleaving stderr - Tests: make all onRateLimit callbacks explicitly async so they satisfy the new Promise<void> return-type contract
|
Coverage after merging fix/auto-retry-on-rate-limit into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- isRateLimitExceeded: include 429 (was only catching 403), so the 'rate limit exceeded' message correctly covers all HTTP rate-limit responses while 503 long-backoff gets its own message - Long-wait error message differentiates 429/403 rate limit from 503 server backoff: 'GitHub API requested a long backoff before retrying.' - options.signal: replace unsafe cast (as AbortSignal | undefined) with nullish coalescing (?? undefined) to preserve null-safety - Test callback: updated to async to match Promise<void> contract
|
Coverage after merging fix/auto-retry-on-rate-limit into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
abortableDelay (api-utils.ts):
- Attach the abort listener BEFORE starting the timer so an abort
that fires between the initial signal.aborted check and addEventListener
is not missed (was a critical race condition)
- Re-check signal.aborted immediately after addEventListener; if aborted
in the race window, remove the listener and reject synchronously
activeCooldown (github-code-search.ts):
- Track cooldownUntil timestamp alongside activeCooldown
- Piggyback only when cooldownUntil >= desiredEnd (current countdown
covers the required wait); otherwise start a fresh countdown for the
longer duration so no caller retries before its required deadline
- Reset cooldownUntil = 0 in .finally() alongside activeCooldown
|
Coverage after merging fix/auto-retry-on-rate-limit into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
Author
|
Addressed in commit
|
|
Coverage after merging fix/auto-retry-on-rate-limit into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+228
to
+232
| // fetchRepoTeams). If a countdown is already running and covers the required | ||
| // wait, new callers piggyback on it. If a new caller needs a *longer* wait | ||
| // A single shared countdown loop: if a new caller needs a longer wait while | ||
| // the loop is already running, we extend cooldownUntil and the loop picks up | ||
| // the new deadline on its next tick — no second loop is ever started. |
There was a problem hiding this comment.
The new rate-limit handler comment block has a duplicated/incomplete sentence ("If a new caller needs a longer wait" followed by another sentence starting the same thought). Please rewrite this comment to be a single coherent explanation of the shared cooldown logic.
Suggested change
| // fetchRepoTeams). If a countdown is already running and covers the required | |
| // wait, new callers piggyback on it. If a new caller needs a *longer* wait | |
| // A single shared countdown loop: if a new caller needs a longer wait while | |
| // the loop is already running, we extend cooldownUntil and the loop picks up | |
| // the new deadline on its next tick — no second loop is ever started. | |
| // fetchRepoTeams). If a countdown is already running and already covers the | |
| // required wait, new callers piggyback on it. If a new caller needs a longer | |
| // wait while the loop is already running, we extend cooldownUntil and the | |
| // countdown loop picks up the new deadline on its next tick — no second loop | |
| // is ever started. |
Comment on lines
+247
to
+257
| activeCooldown = (async () => { | ||
| // Start on a fresh line so the countdown doesn't overwrite the progress bar | ||
| process.stderr.write("\n"); | ||
| while (true) { | ||
| const remaining = cooldownUntil - Date.now(); | ||
| if (remaining <= 0) break; | ||
| process.stderr.write( | ||
| `\r ${pc.yellow("Rate limited")} — resuming in ${formatRetryWait(remaining)}\u2026${" ".repeat(10)}`, | ||
| ); | ||
| await new Promise((r) => setTimeout(r, 1_000)); | ||
| } |
There was a problem hiding this comment.
onRateLimit prints an updating countdown every second using \r. When stderr is not a TTY (common in CI/log capture), these updates typically won’t overwrite a single line and can spam logs for long waits (e.g. minutes). Consider detecting process.stderr.isTTY and, when false, printing a single message (or a much lower-frequency update) and sleeping without per-second redraws.
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
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.
Root cause
Three distinct issues combined to produce the observed failure:
1. Long rate-limit waits threw immediately (primary fix)
fetchWithRetryinsrc/api-utils.tscorrectly detected rate-limit responses and computed the reset delay fromx-ratelimit-reset. However, when that delay exceededMAX_AUTO_RETRY_WAIT_MS(10 s), it threw immediately instead of waiting. ThepaginatedFetchloop propagated the exception, aborting the search at ~90 % completion.2. Secondary rate limits were not detected
GitHub secondary rate limits return
403 + Retry-After(withoutx-ratelimit-remaining: 0).isRateLimitExceededdid not recognise this pattern, so the response bypassed the retry logic and surfaced as an unhandled API error viathrowApiError.3. 422 error when total results = 1 000 (exact multiple of 100)
When the result set hit exactly 1 000 items (10 full pages),
paginatedFetchattempted page 11 after the last full page. GitHub rejects this with422 Cannot access beyond the first 1000 results.Steps to reproduce (before the fix)
Changes
src/api-utils.tsisRateLimitExceeded: now also matches403 + Retry-After(secondary rate limit).getRetryDelayMs: adds 1 s clock-skew buffer to thex-ratelimit-resetcomputation.fetchWithRetry: new optional 4th parameteronRateLimit?: (waitMs: number) => void. When provided and the wait exceedsMAX_AUTO_RETRY_WAIT_MS, the callback is invoked with the wait duration, the function sleeps for that duration without counting it as a retry attempt, then resumes. Without a callback, the existing throw behaviour is preserved.src/api.tssearchCode: accepts and forwardsonRateLimittofetchWithRetry.fetchAllResults: accepts and forwardsonRateLimit; adds earlyreturn []guard whenpage > totalPagesto prevent the page 11 / 422 error.github-code-search.tsonRateLimitcallback tofetchAllResultsthat writes a yellow inline message to stderr:src/api-utils.test.ts403 + Retry-Afterwithin threshold (retry),403 + Retry-Afterexceeding threshold with callback (no throw),onRateLimitwaits not counted as retry attempts, secondary rate-limit 403 callback path.Steps to verify (after the fix)
Closes #102