From 58f4147c7141575ed34d4a21992ce3d2e4ecf51b Mon Sep 17 00:00:00 2001 From: bkellam Date: Wed, 4 Feb 2026 17:25:59 -0800 Subject: [PATCH 1/2] fix --- packages/backend/package.json | 1 + packages/backend/src/github.ts | 15 ++ packages/backend/src/utils.test.ts | 228 ++++++++++++++++++++++++++++- packages/backend/src/utils.ts | 29 +++- yarn.lock | 1 + 5 files changed, 265 insertions(+), 9 deletions(-) diff --git a/packages/backend/package.json b/packages/backend/package.json index d0e7158f9..018f5e182 100644 --- a/packages/backend/package.json +++ b/packages/backend/package.json @@ -46,6 +46,7 @@ "gitea-js": "^1.22.0", "glob": "^11.1.0", "groupmq": "^1.0.0", + "http-status-codes": "^2.3.0", "ioredis": "^5.4.2", "lowdb": "^7.0.1", "micromatch": "^4.0.8", diff --git a/packages/backend/src/github.ts b/packages/backend/src/github.ts index 4ef4e880c..878c7ee68 100644 --- a/packages/backend/src/github.ts +++ b/packages/backend/src/github.ts @@ -1,4 +1,5 @@ import { Octokit } from "@octokit/rest"; +import { RequestError } from "@octokit/request-error"; import * as Sentry from "@sentry/node"; import { getTokenFromConfig } from "@sourcebot/shared"; import { createLogger } from "@sourcebot/shared"; @@ -49,6 +50,20 @@ export const supportsOAuthScopeIntrospection = (tokenType: GitHubTokenType): boo return SCOPE_INTROSPECTABLE_TOKEN_TYPES.includes(tokenType); }; +/** + * Type guard to check if an error is an Octokit RequestError. + */ +export const isOctokitRequestError = (error: unknown): error is RequestError => { + return ( + error !== null && + typeof error === 'object' && + 'status' in error && + typeof error.status === 'number' && + 'name' in error && + error.name === 'HttpError' + ); +}; + // Limit concurrent GitHub requests to avoid hitting rate limits and overwhelming installations. const MAX_CONCURRENT_GITHUB_QUERIES = 5; const githubQueryLimit = pLimit(MAX_CONCURRENT_GITHUB_QUERIES); diff --git a/packages/backend/src/utils.test.ts b/packages/backend/src/utils.test.ts index c24cf6f59..524c354f4 100644 --- a/packages/backend/src/utils.test.ts +++ b/packages/backend/src/utils.test.ts @@ -1,6 +1,19 @@ -import { expect, test } from 'vitest'; -import { arraysEqualShallow } from './utils'; +import { expect, test, describe, vi, beforeEach, afterEach } from 'vitest'; +import { arraysEqualShallow, fetchWithRetry } from './utils'; import { isRemotePath } from '@sourcebot/shared'; +import { Logger } from 'winston'; +import { RequestError } from '@octokit/request-error'; + +vi.mock('@sentry/node', () => ({ + captureException: vi.fn(), +})); + +const createMockLogger = (): Logger => ({ + warn: vi.fn(), + error: vi.fn(), + info: vi.fn(), + debug: vi.fn(), +} as unknown as Logger); test('should return true for identical arrays', () => { expect(arraysEqualShallow([1, 2, 3], [1, 2, 3])).toBe(true); @@ -69,3 +82,214 @@ test('isRemotePath should return false for non HTTP paths', () => { expect(isRemotePath('')).toBe(false); expect(isRemotePath(' ')).toBe(false); }); + +describe('fetchWithRetry', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + test('returns result on successful fetch', async () => { + const logger = createMockLogger(); + const fetchFn = vi.fn().mockResolvedValue('success'); + + const result = await fetchWithRetry(fetchFn, 'test', logger); + + expect(result).toBe('success'); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + test('throws immediately for non-retryable errors (e.g., 404)', async () => { + const logger = createMockLogger(); + const error = { status: 404, message: 'Not Found' }; + const fetchFn = vi.fn().mockRejectedValue(error); + + await expect(fetchWithRetry(fetchFn, 'test', logger)).rejects.toEqual(error); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + test('throws immediately for non-retryable errors (e.g., 401)', async () => { + const logger = createMockLogger(); + const error = { status: 401, message: 'Unauthorized' }; + const fetchFn = vi.fn().mockRejectedValue(error); + + await expect(fetchWithRetry(fetchFn, 'test', logger)).rejects.toEqual(error); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + test('retries on 429 (Too Many Requests) and succeeds', async () => { + const logger = createMockLogger(); + const error = { status: 429, message: 'Too Many Requests' }; + const fetchFn = vi.fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce('success'); + + const resultPromise = fetchWithRetry(fetchFn, 'test', logger); + + // Advance timer to trigger retry + await vi.advanceTimersByTimeAsync(3000); + + const result = await resultPromise; + expect(result).toBe('success'); + expect(fetchFn).toHaveBeenCalledTimes(2); + expect(logger.warn).toHaveBeenCalled(); + }); + + test('retries on 403 (Forbidden) and succeeds', async () => { + const logger = createMockLogger(); + const error = { status: 403, message: 'Forbidden' }; + const fetchFn = vi.fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce('success'); + + const resultPromise = fetchWithRetry(fetchFn, 'test', logger); + + await vi.advanceTimersByTimeAsync(3000); + + const result = await resultPromise; + expect(result).toBe('success'); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + test('retries on 503 (Service Unavailable) and succeeds', async () => { + const logger = createMockLogger(); + const error = { status: 503, message: 'Service Unavailable' }; + const fetchFn = vi.fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce('success'); + + const resultPromise = fetchWithRetry(fetchFn, 'test', logger); + + await vi.advanceTimersByTimeAsync(3000); + + const result = await resultPromise; + expect(result).toBe('success'); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + test('retries on 500 (Internal Server Error) and succeeds', async () => { + const logger = createMockLogger(); + const error = { status: 500, message: 'Internal Server Error' }; + const fetchFn = vi.fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce('success'); + + const resultPromise = fetchWithRetry(fetchFn, 'test', logger); + + await vi.advanceTimersByTimeAsync(3000); + + const result = await resultPromise; + expect(result).toBe('success'); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + test('throws after max attempts exceeded', async () => { + const logger = createMockLogger(); + const error = { status: 429, message: 'Too Many Requests' }; + const fetchFn = vi.fn().mockRejectedValue(error); + + const resultPromise = fetchWithRetry(fetchFn, 'test', logger, 3); + // Prevent unhandled rejection warning while we advance timers + resultPromise.catch(() => {}); + + // Advance through all retry attempts + await vi.advanceTimersByTimeAsync(3000); // 1st retry + await vi.advanceTimersByTimeAsync(6000); // 2nd retry + + await expect(resultPromise).rejects.toEqual(error); + expect(fetchFn).toHaveBeenCalledTimes(3); + }); + + test('uses exponential backoff for wait times', async () => { + const logger = createMockLogger(); + const error = { status: 429, message: 'Too Many Requests' }; + const fetchFn = vi.fn() + .mockRejectedValueOnce(error) + .mockRejectedValueOnce(error) + .mockResolvedValueOnce('success'); + + const resultPromise = fetchWithRetry(fetchFn, 'test', logger, 4); + + // First retry: 3000 * 2^0 = 3000ms + await vi.advanceTimersByTimeAsync(3000); + expect(fetchFn).toHaveBeenCalledTimes(2); + + // Second retry: 3000 * 2^1 = 6000ms + await vi.advanceTimersByTimeAsync(6000); + expect(fetchFn).toHaveBeenCalledTimes(3); + + const result = await resultPromise; + expect(result).toBe('success'); + }); + + 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 + + const error = new RequestError('Rate limit exceeded', 429, { + response: { + headers: { + 'x-ratelimit-reset': String(resetTime), + }, + status: 429, + 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); + + // Should wait approximately 5000ms based on the reset header + await vi.advanceTimersByTimeAsync(5000); + + const result = await resultPromise; + expect(result).toBe('success'); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + test('respects custom maxAttempts parameter', async () => { + const logger = createMockLogger(); + const error = { status: 503, message: 'Service Unavailable' }; + const fetchFn = vi.fn().mockRejectedValue(error); + + const resultPromise = fetchWithRetry(fetchFn, 'test', logger, 2); + // Prevent unhandled rejection warning while we advance timers + resultPromise.catch(() => {}); + + await vi.advanceTimersByTimeAsync(3000); + + await expect(resultPromise).rejects.toEqual(error); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + test('logs warning on each retry attempt', async () => { + const logger = createMockLogger(); + const error = { status: 429, message: 'Too Many Requests' }; + const fetchFn = vi.fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce('success'); + + const resultPromise = fetchWithRetry(fetchFn, 'test-identifier', logger); + + await vi.advanceTimersByTimeAsync(3000); + await resultPromise; + + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('test-identifier') + ); + }); +}); diff --git a/packages/backend/src/utils.ts b/packages/backend/src/utils.ts index 163641420..5bdbe6e10 100644 --- a/packages/backend/src/utils.ts +++ b/packages/backend/src/utils.ts @@ -1,13 +1,13 @@ import { Logger } from "winston"; import { RepoAuthCredentials, RepoWithConnections } from "./types.js"; import path from 'path'; -import { Repo } from "@sourcebot/db"; import { getTokenFromConfig } from "@sourcebot/shared"; import * as Sentry from "@sentry/node"; import { GithubConnectionConfig, GitlabConnectionConfig, GiteaConnectionConfig, BitbucketConnectionConfig, AzureDevOpsConnectionConfig } from '@sourcebot/schemas/v3/connection.type'; import { GithubAppManager } from "./ee/githubAppManager.js"; import { hasEntitlement } from "@sourcebot/shared"; -import { REPOS_CACHE_DIR } from "./constants.js"; +import { StatusCodes } from "http-status-codes"; +import { isOctokitRequestError } from "./github.js"; export const measure = async (cb: () => Promise) => { const start = Date.now(); @@ -72,10 +72,25 @@ export const fetchWithRetry = async ( Sentry.captureException(e); attempts++; - if ((e.status === 403 || e.status === 429 || e.status === 443) && attempts < maxAttempts) { - const computedWaitTime = 3000 * Math.pow(2, attempts - 1); - const resetTime = e.response?.headers?.['x-ratelimit-reset'] ? parseInt(e.response.headers['x-ratelimit-reset']) * 1000 : Date.now() + computedWaitTime; - const waitTime = resetTime - Date.now(); + if ( + ( + (e.status >= 500 && e.status < 600) || + e.status === StatusCodes.FORBIDDEN || + e.status === StatusCodes.TOO_MANY_REQUESTS + ) && attempts < maxAttempts + ) { + 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; + } + + // Default to a exponential backoff approach + const defaultWaitTime = 3000 * Math.pow(2, attempts - 1); + return Date.now() + defaultWaitTime; + })(); + + const waitTime = Math.max(0, resetDateMs - Date.now()); logger.warn(`Rate limit exceeded for ${identifier}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...`); await new Promise(resolve => setTimeout(resolve, waitTime)); @@ -258,7 +273,7 @@ export const setIntervalAsync = (target: () => Promise, pollingIntervalMs: ): (...args: Parameters) => Promise => { return async function (...args: Parameters): Promise { if ((target as any).isRunning) return; - + (target as any).isRunning = true; try { await target(...args); diff --git a/yarn.lock b/yarn.lock index 3bf7a2bf0..05dd12e18 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8125,6 +8125,7 @@ __metadata: gitea-js: "npm:^1.22.0" glob: "npm:^11.1.0" groupmq: "npm:^1.0.0" + http-status-codes: "npm:^2.3.0" ioredis: "npm:^5.4.2" json-schema-to-typescript: "npm:^15.0.4" lowdb: "npm:^7.0.1" From db235d8d2847ce7660ad28c3a4f557f814352a49 Mon Sep 17 00:00:00 2001 From: bkellam Date: Wed, 4 Feb 2026 17:27:55 -0800 Subject: [PATCH 2/2] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64091e31e..b92647248 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed issue where the branch filter in the repos detail page would not return any results. [#851](https://github.com/sourcebot-dev/sourcebot/pull/851) +- Fixed issue where 5xx http errors would not be retried. [#855](https://github.com/sourcebot-dev/sourcebot/pull/855) ## [4.10.25] - 2026-02-04