fix(worker): handle 5xx errors in http retry logic (#855)

brendan-kellam · web-flow · commit c59cdbaa9eed · 2026-02-04T17:37:03.000-08:00
* fix

* changelog
diff --git 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
 
diff --git 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
@@ -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
@@ -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
@@ -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 <T>(cb: () => Promise<T>) => {
     const start = Date.now();
@@ -72,10 +72,25 @@ export const fetchWithRetry = async <T>(
             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<void>, pollingIntervalMs:
     ): (...args: Parameters<T>) => Promise<void> => {
         return async function (...args: Parameters<T>): Promise<void> {
             if ((target as any).isRunning) return;
-    
+
             (target as any).isRunning = true;
             try {
                 await target(...args);
diff --git 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"
