From b77340f3ea04c76f647ca8c6216a5a8c088a62c9 Mon Sep 17 00:00:00 2001 From: Brendan Kellam Date: Wed, 12 Aug 2026 19:37:15 -0700 Subject: [PATCH 1/3] fix(worker): classify GitHub rate limit retries --- packages/backend/src/errors.test.ts | 2 +- packages/backend/src/errors.ts | 5 +- packages/backend/src/utils.test.ts | 98 +++++++++++++++++++++++++++-- packages/backend/src/utils.ts | 57 +++++++++++++---- 4 files changed, 142 insertions(+), 20 deletions(-) diff --git a/packages/backend/src/errors.test.ts b/packages/backend/src/errors.test.ts index 55bfc4e73..c527de12b 100644 --- a/packages/backend/src/errors.test.ts +++ b/packages/backend/src/errors.test.ts @@ -284,7 +284,7 @@ describe('isGitHubRateLimitError', () => { test('recognizes a secondary rate limit response with retry-after', () => { const error = createRequestError('Forbidden', 403, { - 'retry-after': '60', + 'Retry-After': '60', }); expect(isGitHubRateLimitError(error)).toBe(true); diff --git a/packages/backend/src/errors.ts b/packages/backend/src/errors.ts index 50045c7db..a6e93db3b 100644 --- a/packages/backend/src/errors.ts +++ b/packages/backend/src/errors.ts @@ -68,10 +68,9 @@ export const isGitHubRateLimitError = (err: unknown): boolean => { return true; } - const responseHeaders = (err as { response?: { headers?: Record } }).response?.headers; const message = (err as { message?: unknown }).message; - return responseHeaders?.['x-ratelimit-remaining'] === '0' - || responseHeaders?.['retry-after'] !== undefined + return getErrorHeader(err, 'x-ratelimit-remaining') === '0' + || getErrorHeader(err, 'retry-after') !== undefined || (typeof message === 'string' && /rate limit/i.test(message)); }; diff --git a/packages/backend/src/utils.test.ts b/packages/backend/src/utils.test.ts index b8d2e9753..012f15b29 100644 --- a/packages/backend/src/utils.test.ts +++ b/packages/backend/src/utils.test.ts @@ -163,12 +163,28 @@ describe('fetchWithRetry', () => { const result = await resultPromise; expect(result).toBe('success'); expect(fetchFn).toHaveBeenCalledTimes(2); - expect(logger.warn).toHaveBeenCalled(); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('Rate limit exceeded for test'), + { + httpStatus: 429, + responseHeaders: {}, + }, + ); }); - test('retries on 403 (Forbidden) and succeeds', async () => { + test('retries on an unrelated 403 without classifying it as rate limited', async () => { const logger = createMockLogger(); - const error = { status: 403, message: 'Forbidden' }; + const resetTime = Math.floor((Date.now() + 60_000) / 1000); + const error = { + status: 403, + message: 'Resource not accessible', + response: { + headers: { + 'x-ratelimit-remaining': '4999', + 'x-ratelimit-reset': String(resetTime), + }, + }, + }; const fetchFn = vi.fn() .mockRejectedValueOnce(error) .mockResolvedValueOnce('success'); @@ -180,6 +196,16 @@ describe('fetchWithRetry', () => { const result = await resultPromise; expect(result).toBe('success'); expect(fetchFn).toHaveBeenCalledTimes(2); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('Request failed for test with status 403'), + { + httpStatus: 403, + responseHeaders: { + 'x-ratelimit-remaining': '4999', + 'x-ratelimit-reset': String(resetTime), + }, + }, + ); }); test('retries on 503 (Service Unavailable) and succeeds', async () => { @@ -196,6 +222,13 @@ describe('fetchWithRetry', () => { const result = await resultPromise; expect(result).toBe('success'); expect(fetchFn).toHaveBeenCalledTimes(2); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('Request failed for test with status 503'), + { + httpStatus: 503, + responseHeaders: {}, + }, + ); }); test('retries on 500 (Internal Server Error) and succeeds', async () => { @@ -253,7 +286,7 @@ describe('fetchWithRetry', () => { expect(result).toBe('success'); }); - test('respects x-ratelimit-reset header for Octokit errors', async () => { + test('respects x-ratelimit-reset when the primary rate limit is exhausted', async () => { const logger = createMockLogger(); const now = Date.now(); const resetTime = Math.floor((now + 5000) / 1000); // 5 seconds from now @@ -261,7 +294,12 @@ describe('fetchWithRetry', () => { const error = new RequestError('Rate limit exceeded', 429, { response: { headers: { + 'x-ratelimit-limit': '5000', + 'x-ratelimit-remaining': '0', + 'x-ratelimit-used': '5000', + 'x-ratelimit-resource': 'core', 'x-ratelimit-reset': String(resetTime), + 'x-github-request-id': 'ABC1:DEF2:1234:5678', }, status: 429, url: 'https://api.github.com/test', @@ -286,6 +324,52 @@ describe('fetchWithRetry', () => { const result = await resultPromise; expect(result).toBe('success'); expect(fetchFn).toHaveBeenCalledTimes(2); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('Rate limit exceeded for test'), + { + httpStatus: 429, + responseHeaders: { + 'x-ratelimit-limit': '5000', + 'x-ratelimit-remaining': '0', + 'x-ratelimit-used': '5000', + 'x-ratelimit-resource': 'core', + 'x-ratelimit-reset': String(resetTime), + 'x-github-request-id': 'ABC1:DEF2:1234:5678', + }, + }, + ); + }); + + test('respects retry-after for secondary rate limits', async () => { + const logger = createMockLogger(); + const error = new RequestError('You have exceeded a secondary rate limit.', 403, { + response: { + headers: { + 'retry-after': '30', + 'x-ratelimit-remaining': '42', + }, + status: 403, + url: 'https://api.github.com/test', + data: {}, + }, + request: { + method: 'GET', + url: 'https://api.github.com/test', + headers: {}, + }, + }); + const fetchFn = vi.fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce('success'); + + const resultPromise = fetchWithRetry(fetchFn, 'test', logger); + + await vi.advanceTimersByTimeAsync(29_999); + expect(fetchFn).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(1); + + await expect(resultPromise).resolves.toBe('success'); + expect(fetchFn).toHaveBeenCalledTimes(2); }); test('respects custom maxAttempts parameter', async () => { @@ -317,7 +401,11 @@ describe('fetchWithRetry', () => { expect(logger.warn).toHaveBeenCalledTimes(1); expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining('test-identifier') + expect.stringContaining('test-identifier'), + { + httpStatus: 429, + responseHeaders: {}, + }, ); }); }); diff --git a/packages/backend/src/utils.ts b/packages/backend/src/utils.ts index ba028fb20..2b92b5cd0 100644 --- a/packages/backend/src/utils.ts +++ b/packages/backend/src/utils.ts @@ -7,7 +7,7 @@ import { GithubConnectionConfig, GitlabConnectionConfig, GiteaConnectionConfig, import { GithubAppManager } from "./ee/githubAppManager.js"; import { hasEntitlement } from "./entitlements.js"; import { StatusCodes } from "http-status-codes"; -import { isOctokitRequestError } from "./github.js"; +import { getErrorHeader, getErrorStatus, isGitHubRateLimitError } from "./errors.js"; export const measure = async (cb: () => Promise) => { const start = Date.now(); @@ -80,26 +80,61 @@ export const fetchWithRetry = async ( Sentry.captureException(e); attempts++; + const status = getErrorStatus(e); + const isRateLimitError = isGitHubRateLimitError(e); + const isServerError = status !== null && status >= 500 && status < 600; if ( ( - (e.status >= 500 && e.status < 600) || - e.status === StatusCodes.FORBIDDEN || - e.status === StatusCodes.TOO_MANY_REQUESTS + isServerError || + status === StatusCodes.FORBIDDEN || + status === StatusCodes.TOO_MANY_REQUESTS ) && attempts < maxAttempts ) { + const now = Date.now(); + const retryAfter = getErrorHeader(e, 'retry-after'); + const rateLimitRemaining = getErrorHeader(e, 'x-ratelimit-remaining'); + const rateLimitReset = getErrorHeader(e, 'x-ratelimit-reset'); const resetDateMs = (() => { - // First, try to see if we have a reset date specified in the response headers - if (isOctokitRequestError(e) && e.response?.headers['x-ratelimit-reset']) { - return parseInt(e.response.headers['x-ratelimit-reset']) * 1000; + if (isRateLimitError && retryAfter) { + const retryAfterSeconds = Number(retryAfter); + if (Number.isFinite(retryAfterSeconds) && retryAfterSeconds >= 0) { + return now + retryAfterSeconds * 1000; + } + } + + if (isRateLimitError && rateLimitRemaining === '0' && rateLimitReset) { + const resetTimeSeconds = Number(rateLimitReset); + if (Number.isFinite(resetTimeSeconds)) { + return resetTimeSeconds * 1000; + } } - // Default to a exponential backoff approach + // Default to an exponential backoff approach. const defaultWaitTime = 3000 * Math.pow(2, attempts - 1); - return Date.now() + defaultWaitTime; + return now + defaultWaitTime; })(); - const waitTime = Math.max(0, resetDateMs - Date.now()); - logger.warn(`Rate limit exceeded for ${identifier}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...`); + const waitTime = Math.max(0, resetDateMs - now); + const responseHeaders = Object.fromEntries([ + 'x-ratelimit-limit', + 'x-ratelimit-remaining', + 'x-ratelimit-used', + 'x-ratelimit-resource', + 'x-ratelimit-reset', + 'retry-after', + 'x-github-request-id', + ].flatMap((header) => { + const value = getErrorHeader(e, header); + return value === undefined ? [] : [[header, value]]; + })); + const message = isRateLimitError + ? `Rate limit exceeded for ${identifier}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...` + : `Request failed for ${identifier} with status ${status}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...`; + + logger.warn(message, { + httpStatus: status, + responseHeaders, + }); await new Promise(resolve => setTimeout(resolve, waitTime)); continue; From eb47d31e94c0d0d6797ba7e7b1652336c16364db Mon Sep 17 00:00:00 2001 From: Brendan Kellam Date: Wed, 12 Aug 2026 19:37:58 -0700 Subject: [PATCH 2/3] chore: update changelog for #1576 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66a9ab3ad..9ed6bc1a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Upgraded `@sentry/*` to `^10.70.0`, fixing memory leaks where spans retained request data indefinitely. [#1572](https://github.com/sourcebot-dev/sourcebot/pull/1572) - Fixed code search result links occasionally getting stuck during navigation and restored Cmd/Ctrl-click to open matches in preview. [#1574](https://github.com/sourcebot-dev/sourcebot/pull/1574) - Fixed a server-side memory leak where a single shared react-query cache retained state from every server render; the cache is now created per-request. [#1575](https://github.com/sourcebot-dev/sourcebot/pull/1575) +- Fixed GitHub retry handling to distinguish rate limits from other errors and include rate-limit diagnostics in logs. [#1576](https://github.com/sourcebot-dev/sourcebot/pull/1576) ## [5.1.6] - 2026-08-10 From 8130f6b31aa1be5c86a8b28df082b9da238c1d10 Mon Sep 17 00:00:00 2001 From: Brendan Kellam Date: Wed, 12 Aug 2026 20:02:35 -0700 Subject: [PATCH 3/3] fix(worker): simplify retry warning diagnostics --- CHANGELOG.md | 2 +- packages/backend/src/errors.test.ts | 2 +- packages/backend/src/errors.ts | 5 +- packages/backend/src/utils.test.ts | 98 ++--------------------------- packages/backend/src/utils.ts | 57 ++++------------- 5 files changed, 21 insertions(+), 143 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ed6bc1a1..2674ebba7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Upgraded `@sentry/*` to `^10.70.0`, fixing memory leaks where spans retained request data indefinitely. [#1572](https://github.com/sourcebot-dev/sourcebot/pull/1572) - Fixed code search result links occasionally getting stuck during navigation and restored Cmd/Ctrl-click to open matches in preview. [#1574](https://github.com/sourcebot-dev/sourcebot/pull/1574) - Fixed a server-side memory leak where a single shared react-query cache retained state from every server render; the cache is now created per-request. [#1575](https://github.com/sourcebot-dev/sourcebot/pull/1575) -- Fixed GitHub retry handling to distinguish rate limits from other errors and include rate-limit diagnostics in logs. [#1576](https://github.com/sourcebot-dev/sourcebot/pull/1576) +- Fixed code host retry warnings to include the HTTP response status. [#1576](https://github.com/sourcebot-dev/sourcebot/pull/1576) ## [5.1.6] - 2026-08-10 diff --git a/packages/backend/src/errors.test.ts b/packages/backend/src/errors.test.ts index c527de12b..55bfc4e73 100644 --- a/packages/backend/src/errors.test.ts +++ b/packages/backend/src/errors.test.ts @@ -284,7 +284,7 @@ describe('isGitHubRateLimitError', () => { test('recognizes a secondary rate limit response with retry-after', () => { const error = createRequestError('Forbidden', 403, { - 'Retry-After': '60', + 'retry-after': '60', }); expect(isGitHubRateLimitError(error)).toBe(true); diff --git a/packages/backend/src/errors.ts b/packages/backend/src/errors.ts index a6e93db3b..50045c7db 100644 --- a/packages/backend/src/errors.ts +++ b/packages/backend/src/errors.ts @@ -68,9 +68,10 @@ export const isGitHubRateLimitError = (err: unknown): boolean => { return true; } + const responseHeaders = (err as { response?: { headers?: Record } }).response?.headers; const message = (err as { message?: unknown }).message; - return getErrorHeader(err, 'x-ratelimit-remaining') === '0' - || getErrorHeader(err, 'retry-after') !== undefined + return responseHeaders?.['x-ratelimit-remaining'] === '0' + || responseHeaders?.['retry-after'] !== undefined || (typeof message === 'string' && /rate limit/i.test(message)); }; diff --git a/packages/backend/src/utils.test.ts b/packages/backend/src/utils.test.ts index 012f15b29..7977c4b60 100644 --- a/packages/backend/src/utils.test.ts +++ b/packages/backend/src/utils.test.ts @@ -163,28 +163,12 @@ describe('fetchWithRetry', () => { const result = await resultPromise; expect(result).toBe('success'); expect(fetchFn).toHaveBeenCalledTimes(2); - expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining('Rate limit exceeded for test'), - { - httpStatus: 429, - responseHeaders: {}, - }, - ); + expect(logger.warn).toHaveBeenCalled(); }); - test('retries on an unrelated 403 without classifying it as rate limited', async () => { + test('retries on 403 (Forbidden) and succeeds', async () => { const logger = createMockLogger(); - const resetTime = Math.floor((Date.now() + 60_000) / 1000); - const error = { - status: 403, - message: 'Resource not accessible', - response: { - headers: { - 'x-ratelimit-remaining': '4999', - 'x-ratelimit-reset': String(resetTime), - }, - }, - }; + const error = { status: 403, message: 'Forbidden' }; const fetchFn = vi.fn() .mockRejectedValueOnce(error) .mockResolvedValueOnce('success'); @@ -196,16 +180,6 @@ describe('fetchWithRetry', () => { const result = await resultPromise; expect(result).toBe('success'); expect(fetchFn).toHaveBeenCalledTimes(2); - expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining('Request failed for test with status 403'), - { - httpStatus: 403, - responseHeaders: { - 'x-ratelimit-remaining': '4999', - 'x-ratelimit-reset': String(resetTime), - }, - }, - ); }); test('retries on 503 (Service Unavailable) and succeeds', async () => { @@ -222,13 +196,6 @@ describe('fetchWithRetry', () => { const result = await resultPromise; expect(result).toBe('success'); expect(fetchFn).toHaveBeenCalledTimes(2); - expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining('Request failed for test with status 503'), - { - httpStatus: 503, - responseHeaders: {}, - }, - ); }); test('retries on 500 (Internal Server Error) and succeeds', async () => { @@ -286,7 +253,7 @@ describe('fetchWithRetry', () => { expect(result).toBe('success'); }); - test('respects x-ratelimit-reset when the primary rate limit is exhausted', async () => { + test('respects x-ratelimit-reset header for Octokit errors', async () => { const logger = createMockLogger(); const now = Date.now(); const resetTime = Math.floor((now + 5000) / 1000); // 5 seconds from now @@ -294,12 +261,7 @@ describe('fetchWithRetry', () => { const error = new RequestError('Rate limit exceeded', 429, { response: { headers: { - 'x-ratelimit-limit': '5000', - 'x-ratelimit-remaining': '0', - 'x-ratelimit-used': '5000', - 'x-ratelimit-resource': 'core', 'x-ratelimit-reset': String(resetTime), - 'x-github-request-id': 'ABC1:DEF2:1234:5678', }, status: 429, url: 'https://api.github.com/test', @@ -324,52 +286,6 @@ describe('fetchWithRetry', () => { const result = await resultPromise; expect(result).toBe('success'); expect(fetchFn).toHaveBeenCalledTimes(2); - expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining('Rate limit exceeded for test'), - { - httpStatus: 429, - responseHeaders: { - 'x-ratelimit-limit': '5000', - 'x-ratelimit-remaining': '0', - 'x-ratelimit-used': '5000', - 'x-ratelimit-resource': 'core', - 'x-ratelimit-reset': String(resetTime), - 'x-github-request-id': 'ABC1:DEF2:1234:5678', - }, - }, - ); - }); - - test('respects retry-after for secondary rate limits', async () => { - const logger = createMockLogger(); - const error = new RequestError('You have exceeded a secondary rate limit.', 403, { - response: { - headers: { - 'retry-after': '30', - 'x-ratelimit-remaining': '42', - }, - status: 403, - url: 'https://api.github.com/test', - data: {}, - }, - request: { - method: 'GET', - url: 'https://api.github.com/test', - headers: {}, - }, - }); - const fetchFn = vi.fn() - .mockRejectedValueOnce(error) - .mockResolvedValueOnce('success'); - - const resultPromise = fetchWithRetry(fetchFn, 'test', logger); - - await vi.advanceTimersByTimeAsync(29_999); - expect(fetchFn).toHaveBeenCalledTimes(1); - await vi.advanceTimersByTimeAsync(1); - - await expect(resultPromise).resolves.toBe('success'); - expect(fetchFn).toHaveBeenCalledTimes(2); }); test('respects custom maxAttempts parameter', async () => { @@ -401,11 +317,7 @@ describe('fetchWithRetry', () => { expect(logger.warn).toHaveBeenCalledTimes(1); expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining('test-identifier'), - { - httpStatus: 429, - responseHeaders: {}, - }, + expect.stringContaining('test-identifier with status 429') ); }); }); diff --git a/packages/backend/src/utils.ts b/packages/backend/src/utils.ts index 2b92b5cd0..7b999ecc9 100644 --- a/packages/backend/src/utils.ts +++ b/packages/backend/src/utils.ts @@ -7,7 +7,7 @@ import { GithubConnectionConfig, GitlabConnectionConfig, GiteaConnectionConfig, import { GithubAppManager } from "./ee/githubAppManager.js"; import { hasEntitlement } from "./entitlements.js"; import { StatusCodes } from "http-status-codes"; -import { getErrorHeader, getErrorStatus, isGitHubRateLimitError } from "./errors.js"; +import { isOctokitRequestError } from "./github.js"; export const measure = async (cb: () => Promise) => { const start = Date.now(); @@ -80,61 +80,26 @@ export const fetchWithRetry = async ( Sentry.captureException(e); attempts++; - const status = getErrorStatus(e); - const isRateLimitError = isGitHubRateLimitError(e); - const isServerError = status !== null && status >= 500 && status < 600; if ( ( - isServerError || - status === StatusCodes.FORBIDDEN || - status === StatusCodes.TOO_MANY_REQUESTS + (e.status >= 500 && e.status < 600) || + e.status === StatusCodes.FORBIDDEN || + e.status === StatusCodes.TOO_MANY_REQUESTS ) && attempts < maxAttempts ) { - const now = Date.now(); - const retryAfter = getErrorHeader(e, 'retry-after'); - const rateLimitRemaining = getErrorHeader(e, 'x-ratelimit-remaining'); - const rateLimitReset = getErrorHeader(e, 'x-ratelimit-reset'); const resetDateMs = (() => { - if (isRateLimitError && retryAfter) { - const retryAfterSeconds = Number(retryAfter); - if (Number.isFinite(retryAfterSeconds) && retryAfterSeconds >= 0) { - return now + retryAfterSeconds * 1000; - } - } - - if (isRateLimitError && rateLimitRemaining === '0' && rateLimitReset) { - const resetTimeSeconds = Number(rateLimitReset); - if (Number.isFinite(resetTimeSeconds)) { - return resetTimeSeconds * 1000; - } + // First, try to see if we have a reset date specified in the response headers + if (isOctokitRequestError(e) && e.response?.headers['x-ratelimit-reset']) { + return parseInt(e.response.headers['x-ratelimit-reset']) * 1000; } - // Default to an exponential backoff approach. + // Default to a exponential backoff approach const defaultWaitTime = 3000 * Math.pow(2, attempts - 1); - return now + defaultWaitTime; + return Date.now() + defaultWaitTime; })(); - const waitTime = Math.max(0, resetDateMs - now); - const responseHeaders = Object.fromEntries([ - 'x-ratelimit-limit', - 'x-ratelimit-remaining', - 'x-ratelimit-used', - 'x-ratelimit-resource', - 'x-ratelimit-reset', - 'retry-after', - 'x-github-request-id', - ].flatMap((header) => { - const value = getErrorHeader(e, header); - return value === undefined ? [] : [[header, value]]; - })); - const message = isRateLimitError - ? `Rate limit exceeded for ${identifier}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...` - : `Request failed for ${identifier} with status ${status}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...`; - - logger.warn(message, { - httpStatus: status, - responseHeaders, - }); + const waitTime = Math.max(0, resetDateMs - Date.now()); + logger.warn(`Request failed for ${identifier} with status ${e.status}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...`); await new Promise(resolve => setTimeout(resolve, waitTime)); continue;