From 6d436f671bd12d3104cfdcd1e73e07e73e6b8753 Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Bajpai Date: Sun, 9 Aug 2026 02:44:50 +0530 Subject: [PATCH] fix(api): honor Retry-After and stop retrying non-rate-limit 403s fetchWithRetry ignored GitHub's Retry-After header on rate-limit responses, burning attempts with a fixed backoff on 429s and waiting for the full X-RateLimit-Reset window on 403s even when the header suggested a shorter wait. Non-rate-limit 403s (e.g. insufficient scopes) were also retried, delaying the real error. Rate-limit waits now prefer Retry-After, fall back to the reset window for exhausted limits, and non-rate-limit 403s fail fast. --- README.md | 1 - src/lib/api/github.test.ts | 27 ++++++++++ src/lib/api/github.ts | 101 +++++++++++++++++++++---------------- 3 files changed, 84 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 07c81d5..adfb2c6 100644 --- a/README.md +++ b/README.md @@ -65,7 +65,6 @@ These instructions have been tested on a clean machine to ensure a reliable setu > **Note:** If the repository already includes `.env.example`, you only need to copy it to `.env`. Creating a new `.env.example` is only necessary if the file is missing. - 5. **Start the development server:** ```bash bun dev diff --git a/src/lib/api/github.test.ts b/src/lib/api/github.test.ts index 1dd3c88..aa01ca7 100644 --- a/src/lib/api/github.test.ts +++ b/src/lib/api/github.test.ts @@ -111,6 +111,33 @@ describe('GitHub API Client', () => { expect(comments).toEqual([]); }); + it('honors the Retry-After header on 429 instead of exponential backoff', async () => { + const delays: number[] = []; + vi.spyOn(globalThis, 'setTimeout').mockImplementation((fn: any, ms?: number) => { + delays.push(ms ?? 0); + fn(); + return 0 as any; + }); + + mockFetch + .mockResolvedValueOnce(createMockResponse(429, {}, { 'Retry-After': '60' })) + .mockResolvedValueOnce(createMockResponse(200, [])); + + const comments = await fetchIssueComments('owner', 'repo', 42); + expect(mockFetch).toHaveBeenCalledTimes(2); + expect(delays[0]).toBe(60000); + expect(comments).toEqual([]); + }); + + it('does not retry 403 errors that are not rate-limit exhaustion', async () => { + mockFetch.mockResolvedValue(createMockResponse(403, {}, { 'X-RateLimit-Remaining': '4999' })); + + await expect(fetchIssueComments('owner', 'repo', 42)).rejects.toThrow( + 'GitHub API error: 403 Error', + ); + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + it('throws error immediately on non-retryable error (e.g. 500 Internal Server Error)', async () => { mockFetch.mockResolvedValue(createMockResponse(500, { message: 'Crash' })); diff --git a/src/lib/api/github.ts b/src/lib/api/github.ts index 3816c83..f09549a 100644 --- a/src/lib/api/github.ts +++ b/src/lib/api/github.ts @@ -67,6 +67,18 @@ function updateRateLimit(response: Response) { } } +// GitHub sends a Retry-After header (in seconds) on 403/429 rate-limit +// responses. Prefer it over guessing, when present. +function getRetryAfter(response: Response): number | null { + const raw = response.headers.get('Retry-After'); + if (!raw) { + return null; + } + + const seconds = parseInt(raw, 10); + return Number.isFinite(seconds) && seconds > 0 ? seconds : null; +} + export function getRateLimitInfo(): RateLimitInfo { return { ...rateLimitInfo }; } @@ -86,18 +98,25 @@ async function fetchWithRetry( updateRateLimit(response); - if (response.status === 403 && rateLimitInfo.remaining <= 0) { - const resetTime = rateLimitInfo.reset * 1000 - Date.now(); - const waitTime = Math.max(resetTime, 1000); - - await new Promise((r) => setTimeout(r, waitTime)); - continue; - } - - if (response.status === 429) { + if (response.status === 403 || response.status === 429) { + const retryAfter = getRetryAfter(response); const backoff = Math.pow(2, i) * 1000; - await new Promise((r) => setTimeout(r, backoff)); + let waitTime: number; + + if (retryAfter !== null) { + waitTime = retryAfter * 1000; + } else if (response.status === 403 && rateLimitInfo.remaining <= 0) { + const resetTime = rateLimitInfo.reset * 1000 - Date.now(); + waitTime = Math.max(resetTime, 1000); + } else if (response.status === 429) { + waitTime = backoff; + } else { + // 403 that is not a rate-limit exhaustion — retrying won't help + throw new Error(`GitHub API error: ${response.status} ${response.statusText}`); + } + + await new Promise((r) => setTimeout(r, waitTime)); continue; } @@ -137,21 +156,19 @@ export async function searchIssues( const issues = data.items .filter((item) => !item.pull_request) - .map( - (item): Issue => ({ - number: item.number, - title: item.title, - body: item.body, - user: item.user, - labels: item.labels, - assignees: item.assignees, - comments_count: item.comments, - created_at: item.created_at, - updated_at: item.updated_at, - html_url: item.html_url, - state: item.state, - }), - ); + .map((item): Issue => ({ + number: item.number, + title: item.title, + body: item.body, + user: item.user, + labels: item.labels, + assignees: item.assignees, + comments_count: item.comments, + created_at: item.created_at, + updated_at: item.updated_at, + html_url: item.html_url, + state: item.state, + })); allIssues.push(...issues); @@ -190,15 +207,13 @@ export async function fetchIssueComments( const data = await response.json(); - return data.map( - (c: Record): Comment => ({ - id: c.id as number, - user: c.user as Comment['user'], - body: c.body as string, - created_at: c.created_at as string, - updated_at: c.updated_at as string, - }), - ); + return data.map((c: Record): Comment => ({ + id: c.id as number, + user: c.user as Comment['user'], + body: c.body as string, + created_at: c.created_at as string, + updated_at: c.updated_at as string, + })); } export async function fetchIssueTimeline( @@ -217,16 +232,14 @@ export async function fetchIssueTimeline( const data: Record[] = await response.json(); - return data.map( - (e): TimelineEvent => ({ - event: e.event as string, - created_at: e.created_at as string, - actor: e.actor as TimelineEvent['actor'], - source: e.source as TimelineEvent['source'], - commit_id: e.commit_id as string | undefined, - label: e.label as TimelineEvent['label'], - }), - ); + return data.map((e): TimelineEvent => ({ + event: e.event as string, + created_at: e.created_at as string, + actor: e.actor as TimelineEvent['actor'], + source: e.source as TimelineEvent['source'], + commit_id: e.commit_id as string | undefined, + label: e.label as TimelineEvent['label'], + })); } catch { return []; }