Fix rate-limit retry and error management for code search and team fetching

shouze · shouze · commit a5855f7da481 · 2026-02-23T10:11:04.000+01:00
- Add formatRetryWait() to produce human-readable wait durations - Add isRateLimitExceeded() to detect GitHub primary rate-limit 403s (x-ratelimit-remaining: 0) in addition to standard 429/503 - Add getRetryDelayMs() consolidating x-ratelimit-reset > Retry-After > backoff - fetchWithRetry(): check threshold on base delay (before jitter) to avoid false-positive long-wait errors; throw descriptive error when wait > 10 s - Add throwApiError() helper in api.ts: detects rate-limit by body message when headers are absent, produces clean error with wait time - Team-repos loop: throw cleanly on rate-limit 403 instead of logging warning - main() in entry point: catch any Error and print message only (no stack trace) Fixes #22
diff --git a/github-code-search.ts b/github-code-search.ts
@@ -245,6 +245,12 @@ async function main(): Promise<void> {
       writeFileSync(2, queryCmd.helpInformation() + "\n");
       process.exit(1);
     }
+    // Any other known Error (e.g. rate-limit exceeded) → print a clean message
+    // to stderr without a stack trace, then exit 1.
+    if (e instanceof Error) {
+      writeFileSync(2, `error: ${e.message}\n`);
+      process.exit(1);
+    }
     throw e;
   }
 }
diff --git a/src/api-utils.test.ts b/src/api-utils.test.ts
@@ -1,5 +1,5 @@
 import { afterEach, beforeEach, describe, expect, it } from "bun:test";
-import { fetchWithRetry, paginatedFetch } from "./api-utils.ts";
+import { fetchWithRetry, formatRetryWait, paginatedFetch } from "./api-utils.ts";
 
 const originalFetch = globalThis.fetch;
 const originalSetTimeout = globalThis.setTimeout;
@@ -130,6 +130,108 @@ describe("fetchWithRetry", () => {
   });
 });
 
+// ─── formatRetryWait ──────────────────────────────────────────────────────────
+
+describe("formatRetryWait", () => {
+  it("formats seconds when duration < 60 s", () => {
+    expect(formatRetryWait(5_000)).toBe("5 seconds");
+  });
+
+  it("uses singular 'second' for exactly 1 s", () => {
+    expect(formatRetryWait(1_000)).toBe("1 second");
+  });
+
+  it("rounds up sub-second remainders", () => {
+    expect(formatRetryWait(1_500)).toBe("2 seconds");
+  });
+
+  it("formats whole minutes without a seconds clause", () => {
+    expect(formatRetryWait(120_000)).toBe("2 minutes");
+  });
+
+  it("uses singular 'minute' for exactly 1 min", () => {
+    expect(formatRetryWait(60_000)).toBe("1 minute");
+  });
+
+  it("formats minutes and seconds when there is a remainder", () => {
+    expect(formatRetryWait(90_000)).toBe("1 minute and 30 seconds");
+  });
+
+  it("formats large values correctly", () => {
+    expect(formatRetryWait(3_600_000)).toBe("60 minutes");
+  });
+});
+
+// ─── fetchWithRetry – rate-limit (403) handling ───────────────────────────────
+
+describe("fetchWithRetry – 403 rate-limit handling", () => {
+  it("retries on 403 rate-limit when wait is within the auto-retry threshold", async () => {
+    let calls = 0;
+    // reset = now + 1 second → well within the 10 s threshold
+    const resetTimestamp = Math.ceil((Date.now() + 1_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 res = await fetchWithRetry("https://example.com", {}, 3);
+    expect(res.status).toBe(200);
+    expect(calls).toBe(2);
+  });
+
+  it("throws a human-readable error when the rate-limit reset is far in the future", async () => {
+    // reset = now + 5 minutes → exceeds the 10 s auto-retry threshold
+    const resetTimestamp = Math.ceil((Date.now() + 300_000) / 1_000);
+    globalThis.fetch = (async () => {
+      return 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", {}, 3)).rejects.toThrow(
+      /GitHub API rate limit exceeded\. Please retry in \d+ minute/,
+    );
+  });
+
+  it("does NOT treat a plain 403 (non-rate-limit) as retryable", async () => {
+    let calls = 0;
+    globalThis.fetch = (async () => {
+      calls++;
+      return new Response("forbidden", { status: 403 });
+    }) as typeof fetch;
+
+    const res = await fetchWithRetry("https://example.com", {}, 3);
+    expect(res.status).toBe(403);
+    expect(calls).toBe(1);
+  });
+
+  it("throws with a human-readable error when Retry-After header exceeds threshold", async () => {
+    globalThis.fetch = (async () => {
+      return new Response("rate limited", {
+        status: 429,
+        headers: { "Retry-After": "300" }, // 5 minutes
+      });
+    }) as typeof fetch;
+
+    await expect(fetchWithRetry("https://example.com", {}, 3)).rejects.toThrow(
+      /GitHub API rate limit exceeded\. Please retry in \d+ minute/,
+    );
+  });
+});
+
 // ─── paginatedFetch ───────────────────────────────────────────────────────────
 
 describe("paginatedFetch", () => {
diff --git a/src/api-utils.ts b/src/api-utils.ts
@@ -6,15 +6,74 @@
 const RETRYABLE_STATUSES = new Set([429, 503]);
 const BASE_RETRY_DELAY_MS = 1_000;
 const MAX_RETRY_DELAY_MS = 60_000;
+// Above this threshold the user is told to wait manually rather than blocking.
+const MAX_AUTO_RETRY_WAIT_MS = 10_000; // 10 seconds
 
 /**
- * Performs a `fetch` with automatic retry on 429 (rate-limited) and 503
- * (server unavailable), using exponential backoff with optional `Retry-After`
+ * Format a millisecond duration as a human-readable "retry in …" string.
+ * Values ≥ 60 s are expressed in minutes (and seconds when non-zero).
+ *
+ * @example formatRetryWait(90_000) → "1 minute and 30 seconds"
+ * @example formatRetryWait(3_600_000) → "60 minutes"
+ * @example formatRetryWait(5_000) → "5 seconds"
+ */
+export function formatRetryWait(ms: number): string {
+  const totalSeconds = Math.ceil(ms / 1_000);
+  if (totalSeconds >= 60) {
+    const mins = Math.floor(totalSeconds / 60);
+    const secs = totalSeconds % 60;
+    const minStr = `${mins} minute${mins !== 1 ? "s" : ""}`;
+    if (secs === 0) return minStr;
+    return `${minStr} and ${secs} second${secs !== 1 ? "s" : ""}`;
+  }
+  return `${totalSeconds} second${totalSeconds !== 1 ? "s" : ""}`;
+}
+
+/**
+ * Returns true when the response is a GitHub primary rate-limit 403
+ * (x-ratelimit-remaining is "0").
+ */
+function isRateLimitExceeded(res: Response): boolean {
+  return res.status === 403 && res.headers.get("x-ratelimit-remaining") === "0";
+}
+
+/**
+ * Compute the delay in milliseconds before the next retry attempt.
+ * Prefers x-ratelimit-reset (Unix timestamp) > Retry-After header >
+ * exponential back-off.
+ */
+function getRetryDelayMs(res: Response, attempt: number): number {
+  // x-ratelimit-reset: Unix timestamp (seconds) when the quota refills
+  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());
+    }
+  }
+  // Retry-After: seconds to wait (used by 429 / secondary rate limits)
+  const retryAfterHeader = res.headers.get("Retry-After");
+  if (retryAfterHeader !== null) {
+    const seconds = parseInt(retryAfterHeader, 10);
+    if (Number.isFinite(seconds) && seconds > 0) {
+      return seconds * 1_000;
+    }
+  }
+  return Math.min(BASE_RETRY_DELAY_MS * 2 ** attempt, MAX_RETRY_DELAY_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.
  *
  * 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.
  */
 export async function fetchWithRetry(
   url: string,
@@ -24,22 +83,28 @@ export async function fetchWithRetry(
   let attempt = 0;
   while (true) {
     const res = await fetch(url, options);
-    if (!RETRYABLE_STATUSES.has(res.status) || attempt >= maxRetries) {
+
+    // Fix: handle GitHub primary rate-limit (403 + x-ratelimit-remaining: 0)
+    // in addition to the standard 429/503 retryable statuses — see issue #22
+    const retryable = RETRYABLE_STATUSES.has(res.status) || isRateLimitExceeded(res);
+    if (!retryable || attempt >= maxRetries) {
       return res;
     }
-    const retryAfterHeader = res.headers.get("Retry-After");
-    let delayMs: number;
-    if (retryAfterHeader !== null) {
-      const seconds = parseInt(retryAfterHeader, 10);
-      delayMs =
-        Number.isFinite(seconds) && seconds > 0
-          ? seconds * 1_000
-          : Math.min(BASE_RETRY_DELAY_MS * 2 ** attempt, MAX_RETRY_DELAY_MS);
-    } else {
-      delayMs = Math.min(BASE_RETRY_DELAY_MS * 2 ** attempt, MAX_RETRY_DELAY_MS);
+
+    // Compute the base delay (no jitter yet) so the threshold check and the
+    // error message both reflect the real wait time reported by the API.
+    const baseDelayMs = getRetryDelayMs(res, attempt);
+
+    if (baseDelayMs > MAX_AUTO_RETRY_WAIT_MS) {
+      // Cancel the response body before throwing to allow connection reuse
+      await res.body?.cancel();
+      throw new Error(
+        `GitHub API rate limit exceeded. Please retry in ${formatRetryWait(baseDelayMs)}.`,
+      );
     }
+
     // Add ±10 % jitter to avoid thundering-herd on concurrent retries
-    delayMs = delayMs * (0.9 + Math.random() * 0.2);
+    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));
diff --git a/src/api.ts b/src/api.ts
@@ -1,6 +1,6 @@
 import pc from "picocolors";
 import type { CodeMatch } from "./types.ts";
-import { fetchWithRetry, paginatedFetch } from "./api-utils.ts";
+import { fetchWithRetry, formatRetryWait, paginatedFetch } from "./api-utils.ts";
 import { getCacheKey, readCache, writeCache } from "./cache.ts";
 
 // ─── Raw GitHub API types (internal) ─────────────────────────────────────────
@@ -38,6 +38,40 @@ interface RawRepo {
 
 // ─── API client ───────────────────────────────────────────────────────────────
 
+/**
+ * Read a GitHub API error response and throw a human-readable Error.
+ * When the response body signals a rate-limit condition the message includes
+ * the wait time derived from the x-ratelimit-reset header.
+ */
+async function throwApiError(res: Response, context?: string): Promise<never> {
+  let apiMsg = "";
+  let resetHeader: string | null = null;
+  try {
+    const body = (await res.json()) as { message?: string };
+    apiMsg = body.message ?? "";
+    resetHeader = res.headers.get("x-ratelimit-reset");
+  } catch {
+    // Ignore JSON parse errors; fall through to generic message
+  }
+
+  // Fix: detect rate-limit by body message when headers are absent — see issue #22
+  if (
+    res.status === 403 &&
+    (res.headers.get("x-ratelimit-remaining") === "0" ||
+      apiMsg.toLowerCase().includes("rate limit"))
+  ) {
+    let wait = "";
+    if (resetHeader !== null) {
+      const resetMs = parseInt(resetHeader, 10) * 1_000 - Date.now();
+      if (resetMs > 0) wait = ` Please retry in ${formatRetryWait(resetMs)}.`;
+    }
+    throw new Error(`GitHub API rate limit exceeded.${wait}`);
+  }
+
+  const ctx = context ? ` (${context})` : "";
+  throw new Error(`GitHub API error ${res.status}${ctx}: ${apiMsg || "(no message)"}`);
+}
+
 /**
  * Build common GitHub API request headers.
  */
@@ -104,10 +138,7 @@ export async function searchCode(
   const res = await fetchWithRetry(`https://api.github.com/search/code?${params}`, {
     headers: githubHeaders(token),
   });
-  if (!res.ok) {
-    const body = await res.text();
-    throw new Error(`GitHub API error ${res.status}: ${body}`);
-  }
+  if (!res.ok) await throwApiError(res);
   const data = (await res.json()) as SearchCodeResponse;
   return { items: data.items ?? [], total: data.total_count ?? 0 };
 }
@@ -222,10 +253,7 @@ export async function fetchRepoTeams(
       const res = await fetchWithRetry(`https://api.github.com/orgs/${org}/teams?${params}`, {
         headers: githubHeaders(token, "application/vnd.github+json"),
       });
-      if (!res.ok) {
-        const body = await res.text();
-        throw new Error(`GitHub API error ${res.status} (list teams): ${body}`);
-      }
+      if (!res.ok) await throwApiError(res, "list teams");
       const teams = (await res.json()) as RawTeam[];
       for (const t of teams) {
         if (lowerPrefixes.some((p) => t.slug.toLowerCase().startsWith(p))) {
@@ -260,24 +288,36 @@ export async function fetchRepoTeams(
         );
         if (!res.ok) {
           // 404 is expected for nested/secret teams — skip silently.
-          // Other errors are unexpected: read the body, log a warning, and stop.
-          if (res.status !== 404) {
-            let bodyText = "";
-            try {
-              bodyText = (await res.text()).trim();
-            } catch {
-              // Ignore errors while reading the body; we still want to log something.
-            }
-            if (bodyText.length > 200) {
-              bodyText = bodyText.slice(0, 200) + "…";
+          if (res.status === 404) {
+            break;
+          }
+          // Rate-limit 403: throw a clean error like every other rate-limit hit.
+          const bodyJson = await res
+            .json()
+            .catch(() => ({}) as { message?: string });
+          const apiMsg: string = (bodyJson as { message?: string }).message ?? "";
+          if (
+            res.status === 403 &&
+            (res.headers.get("x-ratelimit-remaining") === "0" ||
+              apiMsg.toLowerCase().includes("rate limit"))
+          ) {
+            const resetHeader = res.headers.get("x-ratelimit-reset");
+            let wait = "";
+            if (resetHeader !== null) {
+              const resetMs = parseInt(resetHeader, 10) * 1_000 - Date.now();
+              if (resetMs > 0) wait = ` Please retry in ${formatRetryWait(resetMs)}.`;
             }
-            const message = bodyText ? `; body: ${bodyText}` : "";
-            process.stderr.write(
-              pc.dim(
-                `Warning: could not fetch repos for team "${slug}" (HTTP ${res.status}${message})\n`,
-              ),
-            );
+            throw new Error(`GitHub API rate limit exceeded.${wait}`);
           }
+          // Other errors are unexpected: log a warning and skip this team.
+          let bodyText = apiMsg || JSON.stringify(bodyJson);
+          if (bodyText.length > 200) bodyText = bodyText.slice(0, 200) + "…";
+          const message = bodyText ? `; body: ${bodyText}` : "";
+          process.stderr.write(
+            pc.dim(
+              `Warning: could not fetch repos for team "${slug}" (HTTP ${res.status}${message})\n`,
+            ),
+          );
           break;
         }
         const repos = (await res.json()) as RawRepo[];

Original file line number	Diff line number	Diff line change
`@@ -245,6 +245,12 @@ async function main(): Promise<void> {`
`245`	`245`	`writeFileSync(2, queryCmd.helpInformation() + "\n");`
`246`	`246`	`process.exit(1);`
`247`	`247`	`}`
	`248`	`+ // Any other known Error (e.g. rate-limit exceeded) → print a clean message`
	`249`	`+ // to stderr without a stack trace, then exit 1.`
	`250`	`+ if (e instanceof Error) {`
	`251`	+ writeFileSync(2, `error: ${e.message}\n`);
	`252`	`+ process.exit(1);`
	`253`	`+ }`
`248`	`254`	`throw e;`
`249`	`255`	`}`
`250`	`256`	`}`