From a9e57349a38882ebabd7bfcb17c7b07516aa52b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Mon, 9 Mar 2026 02:20:13 +0100 Subject: [PATCH 1/7] Fix rate-limit errors aborting multi-page searches instead of waiting and retrying MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- github-code-search.ts | 7 ++- src/api-utils.test.ts | 101 ++++++++++++++++++++++++++++++++++++++++++ src/api-utils.ts | 42 +++++++++++++----- src/api.ts | 22 ++++++--- 4 files changed, 154 insertions(+), 18 deletions(-) diff --git a/github-code-search.ts b/github-code-search.ts index 889fc72..e9fd6fe 100644 --- a/github-code-search.ts +++ b/github-code-search.ts @@ -18,6 +18,7 @@ import { resolve } from "node:path"; import pc from "picocolors"; import { aggregate, normaliseExtractRef, normaliseRepo } from "./src/aggregate.ts"; import { fetchAllResults, fetchRepoTeams } from "./src/api.ts"; +import { formatRetryWait } from "./src/api-utils.ts"; import { buildOutput } from "./src/output.ts"; import { groupByTeamPrefix, flattenTeamSections } from "./src/group.ts"; import { checkForUpdate } from "./src/upgrade.ts"; @@ -223,7 +224,11 @@ async function searchAction( /** True when running in non-interactive / CI mode */ const isCI = process.env.CI === "true" || opts.interactive === false; - const rawMatches = await fetchAllResults(query, org, GITHUB_TOKEN!); + const rawMatches = await fetchAllResults(query, org, GITHUB_TOKEN!, (waitMs) => { + process.stderr.write( + `\n ${pc.yellow("Rate limited")} — waiting ${formatRetryWait(waitMs)}, resuming automatically…\n `, + ); + }); let groups = aggregate(rawMatches, excludedRepos, excludedExtractRefs, includeArchived); // ─── Team-prefix grouping ───────────────────────────────────────────────── diff --git a/src/api-utils.test.ts b/src/api-utils.test.ts index 248c4b8..6f8f2f9 100644 --- a/src/api-utils.test.ts +++ b/src/api-utils.test.ts @@ -218,6 +218,47 @@ describe("fetchWithRetry – 403 rate-limit handling", () => { expect(calls).toBe(1); }); + it("retries on 403 secondary rate-limit (Retry-After header) within threshold", async () => { + let calls = 0; + globalThis.fetch = (async () => { + calls++; + if (calls === 1) { + return new Response("secondary rate limited", { + status: 403, + headers: { "Retry-After": "1" }, // 1 s → within 10 s threshold + }); + } + return new Response("ok", { status: 200 }); + }) as typeof fetch; + + const res = await fetchWithRetry("https://example.com", {}, 3); + expect(res.status).toBe(200); + expect(calls).toBe(2); + }); + + it("calls onRateLimit for 403 secondary rate-limit (Retry-After) exceeding threshold", async () => { + let calls = 0; + const callbackArgs: number[] = []; + globalThis.fetch = (async () => { + calls++; + if (calls === 1) { + return new Response("secondary rate limited", { + status: 403, + headers: { "Retry-After": "300" }, // 5 minutes → exceeds threshold + }); + } + return new Response("ok", { status: 200 }); + }) as typeof fetch; + + const res = await fetchWithRetry("https://example.com", {}, 3, (ms) => { + callbackArgs.push(ms); + }); + expect(res.status).toBe(200); + expect(calls).toBe(2); + expect(callbackArgs).toHaveLength(1); + expect(callbackArgs[0]).toBeGreaterThan(10_000); + }); + it("throws with a human-readable error when Retry-After header exceeds threshold", async () => { globalThis.fetch = (async () => { return new Response("rate limited", { @@ -230,6 +271,66 @@ describe("fetchWithRetry – 403 rate-limit handling", () => { /GitHub API rate limit exceeded\. Please retry in \d+ minute/, ); }); + + it("calls onRateLimit callback and retries instead of throwing when wait exceeds threshold", async () => { + let calls = 0; + const callbackArgs: number[] = []; + // reset = now + 5 minutes → exceeds the 10 s threshold + const resetTimestamp = Math.ceil((Date.now() + 300_000) / 1_000); + globalThis.fetch = (async () => { + calls++; + if (calls === 1) { + return new Response("rate limited", { + status: 403, + headers: { + "x-ratelimit-remaining": "0", + "x-ratelimit-reset": String(resetTimestamp), + }, + }); + } + return new Response("ok", { status: 200 }); + }) as typeof fetch; + + const onRateLimit = (waitMs: number) => { + callbackArgs.push(waitMs); + }; + const res = await fetchWithRetry("https://example.com", {}, 3, onRateLimit); + expect(res.status).toBe(200); + expect(calls).toBe(2); + expect(callbackArgs).toHaveLength(1); + expect(callbackArgs[0]).toBeGreaterThan(10_000); + }); + + it("does not count the onRateLimit wait as a retry attempt", async () => { + let calls = 0; + const resetTimestamp = Math.ceil((Date.now() + 300_000) / 1_000); + // First 2 calls: rate-limited (long wait); 3rd call: success + globalThis.fetch = (async () => { + calls++; + if (calls < 3) { + return new Response("rate limited", { + status: 403, + headers: { + "x-ratelimit-remaining": "0", + "x-ratelimit-reset": String(resetTimestamp), + }, + }); + } + return new Response("ok", { status: 200 }); + }) as typeof fetch; + + const callbackCount = { n: 0 }; + const res = await fetchWithRetry( + "https://example.com", + {}, + 1, // maxRetries = 1 — would throw on attempt 2 if rate-limit burns retries + () => { + callbackCount.n++; + }, + ); + expect(res.status).toBe(200); + expect(callbackCount.n).toBe(2); + }); }); // ─── paginatedFetch ─────────────────────────────────────────────────────────── diff --git a/src/api-utils.ts b/src/api-utils.ts index 34e47db..aadc212 100644 --- a/src/api-utils.ts +++ b/src/api-utils.ts @@ -30,11 +30,17 @@ export function formatRetryWait(ms: number): string { } /** - * Returns true when the response is a GitHub primary rate-limit 403 - * (x-ratelimit-remaining is "0"). + * Returns true when the response is a GitHub primary or secondary rate-limit 403. + * + * Primary rate limit: 403 + x-ratelimit-remaining: 0 + * Secondary rate limit: 403 + Retry-After header + * (see https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api) */ function isRateLimitExceeded(res: Response): boolean { - return res.status === 403 && res.headers.get("x-ratelimit-remaining") === "0"; + if (res.status !== 403) return false; + return ( + res.headers.get("x-ratelimit-remaining") === "0" || res.headers.get("Retry-After") !== null + ); } /** @@ -43,12 +49,15 @@ function isRateLimitExceeded(res: Response): boolean { * exponential back-off. */ function getRetryDelayMs(res: Response, attempt: number): number { - // x-ratelimit-reset: Unix timestamp (seconds) when the quota refills + // x-ratelimit-reset: Unix timestamp (seconds) when the quota refills. + // Add a 1 s buffer to guard against clock skew between client and GitHub + // servers — without it the first retry can arrive fractionally early and + // immediately hit the same limit again. const resetHeader = res.headers.get("x-ratelimit-reset"); if (resetHeader !== null) { const resetTime = parseInt(resetHeader, 10); if (Number.isFinite(resetTime)) { - return Math.max(0, resetTime * 1_000 - Date.now()); + return Math.max(0, resetTime * 1_000 - Date.now() + 1_000); } } // Retry-After: seconds to wait (used by 429 / secondary rate limits) @@ -72,13 +81,17 @@ function getRetryDelayMs(res: Response, attempt: number): number { * After `maxRetries` exhausted the last response is returned — callers must * still check `res.ok`. * - * When the computed wait exceeds MAX_AUTO_RETRY_WAIT_MS the function throws a - * descriptive error so the user isn't silently blocked for minutes. + * When the computed wait exceeds MAX_AUTO_RETRY_WAIT_MS and no `onRateLimit` + * callback is provided, the function throws a descriptive error so the user + * isn't silently blocked for minutes. When a callback is provided it is called + * with the wait duration in milliseconds, the function sleeps for that duration, + * then retries — the caller is responsible for surfacing the wait to the user. */ export async function fetchWithRetry( url: string, options: RequestInit, maxRetries = 3, + onRateLimit?: (waitMs: number) => void, ): Promise { let attempt = 0; while (true) { @@ -96,11 +109,18 @@ export async function fetchWithRetry( const baseDelayMs = getRetryDelayMs(res, attempt); if (baseDelayMs > MAX_AUTO_RETRY_WAIT_MS) { - // Cancel the response body before throwing to allow connection reuse + // Cancel the response body before waiting/throwing to allow connection reuse await res.body?.cancel(); - throw new Error( - `GitHub API rate limit exceeded. Please retry in ${formatRetryWait(baseDelayMs)}.`, - ); + if (!onRateLimit) { + throw new Error( + `GitHub API rate limit exceeded. Please retry in ${formatRetryWait(baseDelayMs)}.`, + ); + } + // Fix: inform caller and wait for the full reset duration without counting + // this as a retry attempt (it is an API-imposed mandatory pause). — see issue #102 + onRateLimit(baseDelayMs); + await new Promise((r) => setTimeout(r, baseDelayMs)); + continue; // do NOT increment attempt } // Add ±10 % jitter to avoid thundering-herd on concurrent retries diff --git a/src/api.ts b/src/api.ts index 1cb7a70..77d8514 100644 --- a/src/api.ts +++ b/src/api.ts @@ -128,6 +128,7 @@ export async function searchCode( org: string, token: string, page = 1, + onRateLimit?: (waitMs: number) => void, ): Promise<{ items: RawCodeItem[]; total: number }> { const params = new URLSearchParams({ // @see https://docs.github.com/en/rest/search/search?apiVersion=2022-11-28#constructing-a-search-query @@ -136,9 +137,12 @@ export async function searchCode( page: String(page), }); // @see https://docs.github.com/en/rest/search/search?apiVersion=2022-11-28#search-code - const res = await fetchWithRetry(`https://api.github.com/search/code?${params}`, { - headers: githubHeaders(token), - }); + const res = await fetchWithRetry( + `https://api.github.com/search/code?${params}`, + { headers: githubHeaders(token) }, + 3, + onRateLimit, + ); if (!res.ok) await throwApiError(res); const data = (await res.json()) as SearchCodeResponse; return { items: data.items ?? [], total: data.total_count ?? 0 }; @@ -191,16 +195,22 @@ export async function fetchAllResults( query: string, org: string, token: string, + onRateLimit?: (waitMs: number) => void, ): Promise { // Write the initial progress line (no newline — will be overwritten by \r). process.stderr.write(pc.dim(" Fetching results from GitHub…")); let totalPages = 0; // GitHub code search is capped at 1000 results; paginatedFetch stops naturally - // when a page returns fewer than 100 items (or the API returns an empty page - // after the cap is reached). + // when a page returns fewer than 100 items. When total_count is an exact + // multiple of 100 (e.g. 1000 results), paginatedFetch would request page 11 + // which GitHub rejects with 422 "Cannot access beyond the first 1000 results". + // Fix: guard against that by returning [] as soon as page > totalPages. — see issue #102 const allItems = await paginatedFetch( async (page) => { - const { items, total } = await searchCode(query, org, token, page); + // Short-circuit: GitHub caps at 1 000 results (10 pages max). + // Once we know totalPages, skip any page that would exceed the cap. + if (totalPages > 0 && page > totalPages) return []; + const { items, total } = await searchCode(query, org, token, page, onRateLimit); // On the first page, compute the expected number of pages so the bar // can show realistic progress. GitHub caps at 1 000 results (10 pages). if (page === 1) { From 81b0e47432d83e48d91b573af1be39ec02cb8291 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Mon, 9 Mar 2026 02:46:12 +0100 Subject: [PATCH 2/7] Fix rate limit callback not propagated to fetchRepoTeams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- github-code-search.ts | 23 +++++++++++----- src/api-utils.test.ts | 28 +++++++++++++++++-- src/api-utils.ts | 62 ++++++++++++++++++++++++++++++++----------- src/api.test.ts | 25 +++++++++++++++++ src/api.ts | 12 ++++++--- 5 files changed, 124 insertions(+), 26 deletions(-) diff --git a/github-code-search.ts b/github-code-search.ts index e9fd6fe..3790ddc 100644 --- a/github-code-search.ts +++ b/github-code-search.ts @@ -224,11 +224,22 @@ async function searchAction( /** True when running in non-interactive / CI mode */ const isCI = process.env.CI === "true" || opts.interactive === false; - const rawMatches = await fetchAllResults(query, org, GITHUB_TOKEN!, (waitMs) => { - process.stderr.write( - `\n ${pc.yellow("Rate limited")} — waiting ${formatRetryWait(waitMs)}, resuming automatically…\n `, - ); - }); + /** Shared rate-limit handler used for both the code search and the team fetch. */ + const onRateLimit = async (waitMs: number) => { + const totalSeconds = Math.ceil(waitMs / 1_000); + // Start on a fresh line so the countdown doesn't overwrite the progress bar + process.stderr.write("\n"); + for (let s = totalSeconds; s > 0; s--) { + process.stderr.write( + `\r ${pc.yellow("Rate limited")} — resuming in ${formatRetryWait(s * 1_000)}\u2026${" ".repeat(10)}`, + ); + await new Promise((r) => setTimeout(r, 1_000)); + } + // Leave cursor at line start; the next \r progress update will overwrite cleanly + process.stderr.write(`\r ${pc.dim("Rate limited")} — resuming\u2026${" ".repeat(40)}`); + }; + + const rawMatches = await fetchAllResults(query, org, GITHUB_TOKEN!, onRateLimit); let groups = aggregate(rawMatches, excludedRepos, excludedExtractRefs, includeArchived); // ─── Team-prefix grouping ───────────────────────────────────────────────── @@ -238,7 +249,7 @@ async function searchAction( .map((p) => p.trim()) .filter(Boolean); if (prefixes.length > 0) { - const teamMap = await fetchRepoTeams(org, GITHUB_TOKEN!, prefixes, opts.cache); + const teamMap = await fetchRepoTeams(org, GITHUB_TOKEN!, prefixes, opts.cache, onRateLimit); // Attach team lists to each group for (const g of groups) { g.teams = teamMap.get(g.repoFullName) ?? []; diff --git a/src/api-utils.test.ts b/src/api-utils.test.ts index 6f8f2f9..211a6fd 100644 --- a/src/api-utils.test.ts +++ b/src/api-utils.test.ts @@ -304,7 +304,8 @@ describe("fetchWithRetry – 403 rate-limit handling", () => { it("does not count the onRateLimit wait as a retry attempt", async () => { let calls = 0; const resetTimestamp = Math.ceil((Date.now() + 300_000) / 1_000); - // First 2 calls: rate-limited (long wait); 3rd call: success + // First 2 calls: rate-limited (long wait); 3rd call: success. + // maxRetries = 5 so neither longWaitAttempts (2) nor attempt (0) hits the cap. globalThis.fetch = (async () => { calls++; if (calls < 3) { @@ -323,7 +324,7 @@ describe("fetchWithRetry – 403 rate-limit handling", () => { const res = await fetchWithRetry( "https://example.com", {}, - 1, // maxRetries = 1 — would throw on attempt 2 if rate-limit burns retries + 5, // maxRetries = 5 — long waits do not burn regular retry attempts () => { callbackCount.n++; }, @@ -331,6 +332,29 @@ describe("fetchWithRetry – 403 rate-limit handling", () => { expect(res.status).toBe(200); expect(callbackCount.n).toBe(2); }); + + it("throws after maxRetries long-wait attempts even when onRateLimit is provided", async () => { + const resetTimestamp = Math.ceil((Date.now() + 300_000) / 1_000); + let callbackCalls = 0; + // Always rate-limited — the loop must not run forever + globalThis.fetch = (async () => + new Response("rate limited", { + status: 403, + headers: { + "x-ratelimit-remaining": "0", + "x-ratelimit-reset": String(resetTimestamp), + }, + })) as typeof fetch; + + await expect( + fetchWithRetry("https://example.com", {}, 2, () => { + callbackCalls++; + }), + ).rejects.toThrow(/GitHub API rate limit exceeded/); + // Called twice (longWaitAttempts 0 and 1 → both < maxRetries=2), + // then throws on the 3rd hit (longWaitAttempts=2 >= maxRetries=2) + expect(callbackCalls).toBe(2); + }); }); // ─── paginatedFetch ─────────────────────────────────────────────────────────── diff --git a/src/api-utils.ts b/src/api-utils.ts index aadc212..cd713bf 100644 --- a/src/api-utils.ts +++ b/src/api-utils.ts @@ -71,29 +71,58 @@ function getRetryDelayMs(res: Response, attempt: number): number { return Math.min(BASE_RETRY_DELAY_MS * 2 ** attempt, MAX_RETRY_DELAY_MS); } +/** + * Sleep for `ms` milliseconds. If `signal` is provided and aborts before the + * timer fires, the promise rejects with the signal's abort reason. This makes + * the inter-retry delay abort-aware, consistent with the `fetch` call itself. + */ +function abortableDelay(ms: number, signal?: AbortSignal): Promise { + return new Promise((resolve, reject) => { + if (signal?.aborted) { + reject(signal.reason ?? new DOMException("Aborted", "AbortError")); + return; + } + const id = setTimeout(resolve, ms); + signal?.addEventListener( + "abort", + () => { + clearTimeout(id); + reject(signal.reason ?? new DOMException("Aborted", "AbortError")); + }, + { once: true }, + ); + }); +} + /** * Performs a `fetch` with automatic retry on 429 (rate-limited), 503 - * (server unavailable) and 403 primary rate-limit responses, using - * exponential backoff with optional `Retry-After` / `x-ratelimit-reset` - * header support. + * (server unavailable) and 403 rate-limit responses (both primary and + * secondary), using exponential backoff with optional `Retry-After` / + * `x-ratelimit-reset` header support. * * Non-retryable responses (including successful ones) are returned immediately. * After `maxRetries` exhausted the last response is returned — callers must * still check `res.ok`. * - * When the computed wait exceeds MAX_AUTO_RETRY_WAIT_MS and no `onRateLimit` - * callback is provided, the function throws a descriptive error so the user - * isn't silently blocked for minutes. When a callback is provided it is called - * with the wait duration in milliseconds, the function sleeps for that duration, - * then retries — the caller is responsible for surfacing the wait to the user. + * When the computed wait exceeds MAX_AUTO_RETRY_WAIT_MS: + * - Without `onRateLimit`: throws a descriptive error immediately. + * - With `onRateLimit`: **awaits** the callback, which is responsible for the + * full sleep (so it can show a countdown, honour an AbortSignal, etc.). + * These long waits do **not** count against `maxRetries`; a separate + * `longWaitAttempts` counter (also capped at `maxRetries`) prevents infinite + * loops when the rate-limit never clears. + * + * Short retries (wait ≤ MAX_AUTO_RETRY_WAIT_MS) honour `options.signal` — + * aborting during the delay rejects the promise with the signal's abort reason. */ export async function fetchWithRetry( url: string, options: RequestInit, maxRetries = 3, - onRateLimit?: (waitMs: number) => void, + onRateLimit?: (waitMs: number) => void | Promise, ): Promise { let attempt = 0; + let longWaitAttempts = 0; while (true) { const res = await fetch(url, options); @@ -111,15 +140,17 @@ export async function fetchWithRetry( if (baseDelayMs > MAX_AUTO_RETRY_WAIT_MS) { // Cancel the response body before waiting/throwing to allow connection reuse await res.body?.cancel(); - if (!onRateLimit) { + if (!onRateLimit || longWaitAttempts >= maxRetries) { throw new Error( `GitHub API rate limit exceeded. Please retry in ${formatRetryWait(baseDelayMs)}.`, ); } - // Fix: inform caller and wait for the full reset duration without counting - // this as a retry attempt (it is an API-imposed mandatory pause). — see issue #102 - onRateLimit(baseDelayMs); - await new Promise((r) => setTimeout(r, baseDelayMs)); + // Fix: await the callback — it owns the sleep so it can show a countdown + // and honour an AbortSignal. Long waits do NOT count against maxRetries; + // longWaitAttempts (capped at maxRetries) guards against infinite loops. + // — see issue #102 + longWaitAttempts++; + await onRateLimit(baseDelayMs); continue; // do NOT increment attempt } @@ -127,7 +158,8 @@ export async function fetchWithRetry( const delayMs = baseDelayMs * (0.9 + Math.random() * 0.2); // Cancel the response body to allow the connection to be reused await res.body?.cancel(); - await new Promise((r) => setTimeout(r, delayMs)); + // Honour the caller's AbortSignal during the short wait + await abortableDelay(delayMs, options.signal as AbortSignal | undefined); attempt++; } } diff --git a/src/api.test.ts b/src/api.test.ts index d0bc333..b0a9bb0 100644 --- a/src/api.test.ts +++ b/src/api.test.ts @@ -287,6 +287,31 @@ describe("fetchAllResults", () => { expect(results[0].textMatches[0].matches[0].line).toBe(1); }); + it("does not request page 11 when total_count is exactly 1000 (10 full pages)", async () => { + // Regression guard for the 422 "Cannot access beyond the first 1000 results" + // error that occurred when total_count was an exact multiple of 100. + let searchCallCount = 0; + globalThis.fetch = (async (url: string | URL | Request) => { + const urlStr = url.toString(); + if (urlStr.includes("raw.githubusercontent.com")) { + return new Response("", { status: 404 }); + } + searchCallCount++; + return new Response( + JSON.stringify({ + items: Array.from({ length: 100 }, (_, i) => makeFetchItem(i)), + total_count: 1000, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + }) as typeof fetch; + + const results = await fetchAllResults("q", "org", "tok"); + expect(results).toHaveLength(1000); + // Exactly 10 pages fetched — page 11 must never be requested + expect(searchCallCount).toBe(10); + }); + it("falls back when raw content fetch returns non-ok status", async () => { const fakeItem = { path: "src/mod.ts", diff --git a/src/api.ts b/src/api.ts index 77d8514..ed944c9 100644 --- a/src/api.ts +++ b/src/api.ts @@ -308,6 +308,7 @@ export async function fetchRepoTeams( token: string, prefixes: string[], useCache = true, + onRateLimit?: (waitMs: number) => void | Promise, ): Promise> { // ── Cache lookup ──────────────────────────────────────────────────────────── // The team list is quasi-static; cache it for 24 h to avoid dozens of API @@ -333,9 +334,12 @@ export async function fetchRepoTeams( let teamsPage = 1; while (true) { const params = new URLSearchParams({ per_page: "100", page: String(teamsPage) }); - const res = await fetchWithRetry(`https://api.github.com/orgs/${org}/teams?${params}`, { - headers: githubHeaders(token, "application/vnd.github+json"), - }); + const res = await fetchWithRetry( + `https://api.github.com/orgs/${org}/teams?${params}`, + { headers: githubHeaders(token, "application/vnd.github+json") }, + 3, + onRateLimit, + ); if (!res.ok) await throwApiError(res, "list teams"); const teams = (await res.json()) as RawTeam[]; for (const t of teams) { @@ -368,6 +372,8 @@ export async function fetchRepoTeams( const res = await fetchWithRetry( `https://api.github.com/orgs/${org}/teams/${slug}/repos?${params}`, { headers: githubHeaders(token, "application/vnd.github+json") }, + 3, + onRateLimit, ); if (!res.ok) { // 404 is expected for nested/secret teams — skip silently. From c3da59b74271ba18a0986a4bd77aedf9bd118891 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Mon, 9 Mar 2026 02:49:35 +0100 Subject: [PATCH 3/7] Fix incomplete URL hostname check in tests 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). --- src/api.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/api.test.ts b/src/api.test.ts index b0a9bb0..83ae3bb 100644 --- a/src/api.test.ts +++ b/src/api.test.ts @@ -210,7 +210,7 @@ describe("fetchAllResults", () => { let searchPage = 0; globalThis.fetch = (async (url: string | URL | Request) => { const urlStr = url.toString(); - if (urlStr.includes("raw.githubusercontent.com")) { + if (new URL(urlStr).hostname === "raw.githubusercontent.com") { return new Response("", { status: 404 }); } searchPage++; @@ -245,7 +245,7 @@ describe("fetchAllResults", () => { }; globalThis.fetch = (async (url: string | URL | Request) => { const urlStr = url.toString(); - if (urlStr.includes("raw.githubusercontent.com")) { + if (new URL(urlStr).hostname === "raw.githubusercontent.com") { return new Response(fileContent, { status: 200 }); } return new Response(JSON.stringify({ items: [fakeItem], total_count: 1 }), { @@ -273,7 +273,7 @@ describe("fetchAllResults", () => { }; globalThis.fetch = (async (url: string | URL | Request) => { const urlStr = url.toString(); - if (urlStr.includes("raw.githubusercontent.com")) { + if (new URL(urlStr).hostname === "raw.githubusercontent.com") { throw new Error("network error"); } return new Response(JSON.stringify({ items: [fakeItem], total_count: 1 }), { @@ -293,7 +293,7 @@ describe("fetchAllResults", () => { let searchCallCount = 0; globalThis.fetch = (async (url: string | URL | Request) => { const urlStr = url.toString(); - if (urlStr.includes("raw.githubusercontent.com")) { + if (new URL(urlStr).hostname === "raw.githubusercontent.com") { return new Response("", { status: 404 }); } searchCallCount++; @@ -326,7 +326,7 @@ describe("fetchAllResults", () => { }; globalThis.fetch = (async (url: string | URL | Request) => { const urlStr = url.toString(); - if (urlStr.includes("raw.githubusercontent.com")) { + if (new URL(urlStr).hostname === "raw.githubusercontent.com") { return new Response("Not Found", { status: 404 }); } return new Response(JSON.stringify({ items: [fakeItem], total_count: 1 }), { From 470f3c06541682287ea40c1e6aa61c65bc60c6e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Mon, 9 Mar 2026 02:58:00 +0100 Subject: [PATCH 4/7] Address review: tighten onRateLimit contract and fix listener leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) to Promise 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 return-type contract --- github-code-search.ts | 36 +++++++++++++++++++++++++----------- src/api-utils.test.ts | 6 +++--- src/api-utils.ts | 28 ++++++++++++++++------------ src/api.ts | 6 +++--- 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/github-code-search.ts b/github-code-search.ts index 3790ddc..15ea833 100644 --- a/github-code-search.ts +++ b/github-code-search.ts @@ -224,19 +224,33 @@ async function searchAction( /** True when running in non-interactive / CI mode */ const isCI = process.env.CI === "true" || opts.interactive === false; + // Shared promise for concurrent rate-limit hits (e.g. from Promise.all in + // fetchRepoTeams). If a countdown is already running, new callers attach to + // the same promise so only one countdown displays at a time. + let activeCooldown: Promise | null = null; + /** Shared rate-limit handler used for both the code search and the team fetch. */ - const onRateLimit = async (waitMs: number) => { - const totalSeconds = Math.ceil(waitMs / 1_000); - // Start on a fresh line so the countdown doesn't overwrite the progress bar - process.stderr.write("\n"); - for (let s = totalSeconds; s > 0; s--) { - process.stderr.write( - `\r ${pc.yellow("Rate limited")} — resuming in ${formatRetryWait(s * 1_000)}\u2026${" ".repeat(10)}`, - ); - await new Promise((r) => setTimeout(r, 1_000)); + const onRateLimit = (waitMs: number): Promise => { + if (activeCooldown !== null) { + // Piggyback on the in-flight countdown — no need for a second one. + return activeCooldown; } - // Leave cursor at line start; the next \r progress update will overwrite cleanly - process.stderr.write(`\r ${pc.dim("Rate limited")} — resuming\u2026${" ".repeat(40)}`); + activeCooldown = (async () => { + const totalSeconds = Math.ceil(waitMs / 1_000); + // Start on a fresh line so the countdown doesn't overwrite the progress bar + process.stderr.write("\n"); + for (let s = totalSeconds; s > 0; s--) { + process.stderr.write( + `\r ${pc.yellow("Rate limited")} — resuming in ${formatRetryWait(s * 1_000)}\u2026${" ".repeat(10)}`, + ); + await new Promise((r) => setTimeout(r, 1_000)); + } + // Leave cursor at line start; the next \r progress update will overwrite cleanly + process.stderr.write(`\r ${pc.dim("Rate limited")} — resuming\u2026${" ".repeat(40)}`); + })().finally(() => { + activeCooldown = null; + }); + return activeCooldown; }; const rawMatches = await fetchAllResults(query, org, GITHUB_TOKEN!, onRateLimit); diff --git a/src/api-utils.test.ts b/src/api-utils.test.ts index 211a6fd..7240cd2 100644 --- a/src/api-utils.test.ts +++ b/src/api-utils.test.ts @@ -291,7 +291,7 @@ describe("fetchWithRetry – 403 rate-limit handling", () => { return new Response("ok", { status: 200 }); }) as typeof fetch; - const onRateLimit = (waitMs: number) => { + const onRateLimit = async (waitMs: number) => { callbackArgs.push(waitMs); }; const res = await fetchWithRetry("https://example.com", {}, 3, onRateLimit); @@ -325,7 +325,7 @@ describe("fetchWithRetry – 403 rate-limit handling", () => { "https://example.com", {}, 5, // maxRetries = 5 — long waits do not burn regular retry attempts - () => { + async () => { callbackCount.n++; }, ); @@ -347,7 +347,7 @@ describe("fetchWithRetry – 403 rate-limit handling", () => { })) as typeof fetch; await expect( - fetchWithRetry("https://example.com", {}, 2, () => { + fetchWithRetry("https://example.com", {}, 2, async () => { callbackCalls++; }), ).rejects.toThrow(/GitHub API rate limit exceeded/); diff --git a/src/api-utils.ts b/src/api-utils.ts index cd713bf..466659d 100644 --- a/src/api-utils.ts +++ b/src/api-utils.ts @@ -82,15 +82,17 @@ function abortableDelay(ms: number, signal?: AbortSignal): Promise { reject(signal.reason ?? new DOMException("Aborted", "AbortError")); return; } - const id = setTimeout(resolve, ms); - signal?.addEventListener( - "abort", - () => { - clearTimeout(id); - reject(signal.reason ?? new DOMException("Aborted", "AbortError")); - }, - { once: true }, - ); + // Extract the handler so it can be removed when the timer fires, preventing + // listener accumulation across many retries. + const onAbort = () => { + clearTimeout(id); + reject(signal!.reason ?? new DOMException("Aborted", "AbortError")); + }; + const id = setTimeout(() => { + signal?.removeEventListener("abort", onAbort); + resolve(); + }, ms); + signal?.addEventListener("abort", onAbort, { once: true }); }); } @@ -106,8 +108,10 @@ function abortableDelay(ms: number, signal?: AbortSignal): Promise { * * When the computed wait exceeds MAX_AUTO_RETRY_WAIT_MS: * - Without `onRateLimit`: throws a descriptive error immediately. - * - With `onRateLimit`: **awaits** the callback, which is responsible for the - * full sleep (so it can show a countdown, honour an AbortSignal, etc.). + * - With `onRateLimit`: **awaits** the callback, which **must** sleep for (at + * least) `waitMs` milliseconds — e.g. by showing a countdown timer. A + * callback that returns immediately without sleeping will cause the loop to + * retry at once and exhaust `longWaitAttempts` very quickly. * These long waits do **not** count against `maxRetries`; a separate * `longWaitAttempts` counter (also capped at `maxRetries`) prevents infinite * loops when the rate-limit never clears. @@ -119,7 +123,7 @@ export async function fetchWithRetry( url: string, options: RequestInit, maxRetries = 3, - onRateLimit?: (waitMs: number) => void | Promise, + onRateLimit?: (waitMs: number) => Promise, ): Promise { let attempt = 0; let longWaitAttempts = 0; diff --git a/src/api.ts b/src/api.ts index ed944c9..3ee670b 100644 --- a/src/api.ts +++ b/src/api.ts @@ -128,7 +128,7 @@ export async function searchCode( org: string, token: string, page = 1, - onRateLimit?: (waitMs: number) => void, + onRateLimit?: (waitMs: number) => Promise, ): Promise<{ items: RawCodeItem[]; total: number }> { const params = new URLSearchParams({ // @see https://docs.github.com/en/rest/search/search?apiVersion=2022-11-28#constructing-a-search-query @@ -195,7 +195,7 @@ export async function fetchAllResults( query: string, org: string, token: string, - onRateLimit?: (waitMs: number) => void, + onRateLimit?: (waitMs: number) => Promise, ): Promise { // Write the initial progress line (no newline — will be overwritten by \r). process.stderr.write(pc.dim(" Fetching results from GitHub…")); @@ -308,7 +308,7 @@ export async function fetchRepoTeams( token: string, prefixes: string[], useCache = true, - onRateLimit?: (waitMs: number) => void | Promise, + onRateLimit?: (waitMs: number) => Promise, ): Promise> { // ── Cache lookup ──────────────────────────────────────────────────────────── // The team list is quasi-static; cache it for 24 h to avoid dozens of API From 4be5743f262acd904bbe3d196927ed609cc4abe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Mon, 9 Mar 2026 03:12:15 +0100 Subject: [PATCH 5/7] Fix misleading error message for 503 long backoff and type safety - 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 contract --- src/api-utils.test.ts | 2 +- src/api-utils.ts | 14 ++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/api-utils.test.ts b/src/api-utils.test.ts index 7240cd2..a94d833 100644 --- a/src/api-utils.test.ts +++ b/src/api-utils.test.ts @@ -250,7 +250,7 @@ describe("fetchWithRetry – 403 rate-limit handling", () => { return new Response("ok", { status: 200 }); }) as typeof fetch; - const res = await fetchWithRetry("https://example.com", {}, 3, (ms) => { + const res = await fetchWithRetry("https://example.com", {}, 3, async (ms) => { callbackArgs.push(ms); }); expect(res.status).toBe(200); diff --git a/src/api-utils.ts b/src/api-utils.ts index 466659d..b3e5cc6 100644 --- a/src/api-utils.ts +++ b/src/api-utils.ts @@ -37,6 +37,9 @@ export function formatRetryWait(ms: number): string { * (see https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api) */ function isRateLimitExceeded(res: Response): boolean { + // 429 Too Many Requests is always a rate-limit response. + if (res.status === 429) return true; + // 403 is a GitHub-specific rate-limit when accompanied by the right headers. if (res.status !== 403) return false; return ( res.headers.get("x-ratelimit-remaining") === "0" || res.headers.get("Retry-After") !== null @@ -145,9 +148,12 @@ export async function fetchWithRetry( // Cancel the response body before waiting/throwing to allow connection reuse await res.body?.cancel(); if (!onRateLimit || longWaitAttempts >= maxRetries) { - throw new Error( - `GitHub API rate limit exceeded. Please retry in ${formatRetryWait(baseDelayMs)}.`, - ); + // Produce an accurate message: 403/429 are rate-limit errors; 503 is a + // server backoff that should not be labelled "rate limit exceeded". + const messagePrefix = isRateLimitExceeded(res) + ? "GitHub API rate limit exceeded." + : "GitHub API requested a long backoff before retrying."; + throw new Error(`${messagePrefix} Please retry in ${formatRetryWait(baseDelayMs)}.`); } // Fix: await the callback — it owns the sleep so it can show a countdown // and honour an AbortSignal. Long waits do NOT count against maxRetries; @@ -163,7 +169,7 @@ export async function fetchWithRetry( // Cancel the response body to allow the connection to be reused await res.body?.cancel(); // Honour the caller's AbortSignal during the short wait - await abortableDelay(delayMs, options.signal as AbortSignal | undefined); + await abortableDelay(delayMs, options.signal ?? undefined); attempt++; } } From f8c26a4c068605dcaf5432af4c7340a7c5ec66ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Mon, 9 Mar 2026 03:18:15 +0100 Subject: [PATCH 6/7] Fix abortableDelay race condition and cooldown duration extension 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 --- github-code-search.ts | 16 ++++++++++++---- src/api-utils.ts | 11 ++++++++++- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/github-code-search.ts b/github-code-search.ts index 15ea833..d3d6998 100644 --- a/github-code-search.ts +++ b/github-code-search.ts @@ -225,16 +225,23 @@ async function searchAction( const isCI = process.env.CI === "true" || opts.interactive === false; // Shared promise for concurrent rate-limit hits (e.g. from Promise.all in - // fetchRepoTeams). If a countdown is already running, new callers attach to - // the same promise so only one countdown displays at a time. + // 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 + // (different reset timestamps), a fresh countdown replaces the old one so + // no caller retries before its required deadline. let activeCooldown: Promise | null = null; + let cooldownUntil = 0; /** Shared rate-limit handler used for both the code search and the team fetch. */ const onRateLimit = (waitMs: number): Promise => { - if (activeCooldown !== null) { - // Piggyback on the in-flight countdown — no need for a second one. + const desiredEnd = Date.now() + waitMs; + if (activeCooldown !== null && cooldownUntil >= desiredEnd) { + // Existing countdown covers the required wait — piggyback on it. return activeCooldown; } + // No countdown running, or the new wait extends beyond the current one. + // Start a fresh countdown for the longer duration. + cooldownUntil = desiredEnd; activeCooldown = (async () => { const totalSeconds = Math.ceil(waitMs / 1_000); // Start on a fresh line so the countdown doesn't overwrite the progress bar @@ -249,6 +256,7 @@ async function searchAction( process.stderr.write(`\r ${pc.dim("Rate limited")} — resuming\u2026${" ".repeat(40)}`); })().finally(() => { activeCooldown = null; + cooldownUntil = 0; }); return activeCooldown; }; diff --git a/src/api-utils.ts b/src/api-utils.ts index b3e5cc6..dff0d2f 100644 --- a/src/api-utils.ts +++ b/src/api-utils.ts @@ -91,11 +91,20 @@ function abortableDelay(ms: number, signal?: AbortSignal): Promise { clearTimeout(id); reject(signal!.reason ?? new DOMException("Aborted", "AbortError")); }; + // Attach the listener BEFORE starting the timer so an abort that fires + // between the initial check and addEventListener is not missed. + signal?.addEventListener("abort", onAbort, { once: true }); + // Re-check after attaching to close the race window: if the signal was + // aborted in between, clean up and reject synchronously. + if (signal?.aborted) { + signal.removeEventListener("abort", onAbort); + reject(signal.reason ?? new DOMException("Aborted", "AbortError")); + return; + } const id = setTimeout(() => { signal?.removeEventListener("abort", onAbort); resolve(); }, ms); - signal?.addEventListener("abort", onAbort, { once: true }); }); } From 512795f23d496e0ecacd0d823c8b3df0757f9177 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Mon, 9 Mar 2026 03:29:23 +0100 Subject: [PATCH 7/7] Address review: fix JSDoc for isRateLimitExceeded and single-loop cooldown --- github-code-search.ts | 21 ++++++++++++--------- src/api-utils.ts | 5 +++-- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/github-code-search.ts b/github-code-search.ts index d3d6998..ac26503 100644 --- a/github-code-search.ts +++ b/github-code-search.ts @@ -227,28 +227,31 @@ async function searchAction( // Shared promise for concurrent rate-limit hits (e.g. from Promise.all in // 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 - // (different reset timestamps), a fresh countdown replaces the old one so - // no caller retries before its required deadline. + // 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. let activeCooldown: Promise | null = null; let cooldownUntil = 0; /** Shared rate-limit handler used for both the code search and the team fetch. */ const onRateLimit = (waitMs: number): Promise => { const desiredEnd = Date.now() + waitMs; - if (activeCooldown !== null && cooldownUntil >= desiredEnd) { - // Existing countdown covers the required wait — piggyback on it. + if (activeCooldown !== null) { + // A loop is already running. Extend the deadline if the new wait is longer; + // piggyback in either case — a single loop covers all concurrent callers. + if (desiredEnd > cooldownUntil) cooldownUntil = desiredEnd; return activeCooldown; } - // No countdown running, or the new wait extends beyond the current one. - // Start a fresh countdown for the longer duration. + // No countdown running — start a fresh one. cooldownUntil = desiredEnd; activeCooldown = (async () => { - const totalSeconds = Math.ceil(waitMs / 1_000); // Start on a fresh line so the countdown doesn't overwrite the progress bar process.stderr.write("\n"); - for (let s = totalSeconds; s > 0; s--) { + while (true) { + const remaining = cooldownUntil - Date.now(); + if (remaining <= 0) break; process.stderr.write( - `\r ${pc.yellow("Rate limited")} — resuming in ${formatRetryWait(s * 1_000)}\u2026${" ".repeat(10)}`, + `\r ${pc.yellow("Rate limited")} — resuming in ${formatRetryWait(remaining)}\u2026${" ".repeat(10)}`, ); await new Promise((r) => setTimeout(r, 1_000)); } diff --git a/src/api-utils.ts b/src/api-utils.ts index dff0d2f..ab96cf6 100644 --- a/src/api-utils.ts +++ b/src/api-utils.ts @@ -30,9 +30,10 @@ export function formatRetryWait(ms: number): string { } /** - * Returns true when the response is a GitHub primary or secondary rate-limit 403. + * Returns true when the response is a GitHub rate-limit response. * - * Primary rate limit: 403 + x-ratelimit-remaining: 0 + * Standard rate limit: 429 Too Many Requests + * Primary rate limit: 403 + x-ratelimit-remaining: 0 * Secondary rate limit: 403 + Retry-After header * (see https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api) */