Fix rate-limit errors aborting multi-page searches instead of waiting and retrying

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,47 @@ 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!);
+  // 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
+  // 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<void> | null = null;
+  let cooldownUntil = 0;
+
+  /** Shared rate-limit handler used for both the code search and the team fetch. */
+  const onRateLimit = (waitMs: number): Promise<void> => {
+    const desiredEnd = Date.now() + waitMs;
+    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 — start a fresh one.
+    cooldownUntil = desiredEnd;
+    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));
+      }
+      // 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;
+      cooldownUntil = 0;
+    });
+    return activeCooldown;
+  };
+
+  const rawMatches = await fetchAllResults(query, org, GITHUB_TOKEN!, onRateLimit);
   let groups = aggregate(rawMatches, excludedRepos, excludedExtractRefs, includeArchived);
 
   // ─── Team-prefix grouping ─────────────────────────────────────────────────
@@ -233,7 +274,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) ?? [];

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, async (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,90 @@ 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 = async (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.
+    // maxRetries = 5 so neither longWaitAttempts (2) nor attempt (0) hits the cap.
+    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",
+      {},
+      5, // maxRetries = 5 — long waits do not burn regular retry attempts
+      async () => {
+        callbackCount.n++;
+      },
+    );
+    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, async () => {
+        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 ───────────────────────────────────────────────────────────

src/api-utils.ts

@@ -30,11 +30,21 @@ 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 rate-limit response.
+ *
+ * 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)
  */
 function isRateLimitExceeded(res: Response): boolean {
-  return res.status === 403 && res.headers.get("x-ratelimit-remaining") === "0";
+  // 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
+  );
 }
 
 /**
@@ -43,12 +53,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)
@@ -62,25 +75,71 @@ 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<void> {
+  return new Promise((resolve, reject) => {
+    if (signal?.aborted) {
+      reject(signal.reason ?? new DOMException("Aborted", "AbortError"));
+      return;
+    }
+    // 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"));
+    };
+    // 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);
+  });
+}
+
 /**
  * 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 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:
+ * - Without `onRateLimit`: throws a descriptive error immediately.
+ * - 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.
+ *
+ * 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) => Promise<void>,
 ): Promise<Response> {
   let attempt = 0;
+  let longWaitAttempts = 0;
   while (true) {
     const res = await fetch(url, options);
 
@@ -96,18 +155,31 @@ 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 || longWaitAttempts >= maxRetries) {
+        // 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;
+      // longWaitAttempts (capped at maxRetries) guards against infinite loops.
+      // — see issue #102
+      longWaitAttempts++;
+      await onRateLimit(baseDelayMs);
+      continue; // do NOT increment attempt
     }
 
     // Add ±10 % jitter to avoid thundering-herd on concurrent retries
     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 ?? undefined);
     attempt++;
   }
 }