Skip to content

Commit 8130f6b

Browse files
brendan-kellambrendan-kellam
committed
fix(worker): simplify retry warning diagnostics
1 parent eb47d31 commit 8130f6b

5 files changed

Lines changed: 21 additions & 143 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1717
- Upgraded `@sentry/*` to `^10.70.0`, fixing memory leaks where spans retained request data indefinitely. [#1572](https://github.com/sourcebot-dev/sourcebot/pull/1572)
1818
- 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)
1919
- 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)
20-
- 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)
20+
- Fixed code host retry warnings to include the HTTP response status. [#1576](https://github.com/sourcebot-dev/sourcebot/pull/1576)
2121

2222
## [5.1.6] - 2026-08-10
2323

‎packages/backend/src/errors.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ describe('isGitHubRateLimitError', () => {
284284

285285
test('recognizes a secondary rate limit response with retry-after', () => {
286286
const error = createRequestError('Forbidden', 403, {
287-
'Retry-After': '60',
287+
'retry-after': '60',
288288
});
289289

290290
expect(isGitHubRateLimitError(error)).toBe(true);

‎packages/backend/src/errors.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,10 @@ export const isGitHubRateLimitError = (err: unknown): boolean => {
6868
return true;
6969
}
7070

71+
const responseHeaders = (err as { response?: { headers?: Record<string, string | undefined> } }).response?.headers;
7172
const message = (err as { message?: unknown }).message;
7273

73-
return getErrorHeader(err, 'x-ratelimit-remaining') === '0'
74-
|| getErrorHeader(err, 'retry-after') !== undefined
74+
return responseHeaders?.['x-ratelimit-remaining'] === '0'
75+
|| responseHeaders?.['retry-after'] !== undefined
7576
|| (typeof message === 'string' && /rate limit/i.test(message));
7677
};

‎packages/backend/src/utils.test.ts‎

Lines changed: 5 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -163,28 +163,12 @@ describe('fetchWithRetry', () => {
163163
const result = await resultPromise;
164164
expect(result).toBe('success');
165165
expect(fetchFn).toHaveBeenCalledTimes(2);
166-
expect(logger.warn).toHaveBeenCalledWith(
167-
expect.stringContaining('Rate limit exceeded for test'),
168-
{
169-
httpStatus: 429,
170-
responseHeaders: {},
171-
},
172-
);
166+
expect(logger.warn).toHaveBeenCalled();
173167
});
174168

175-
test('retries on an unrelated 403 without classifying it as rate limited', async () => {
169+
test('retries on 403 (Forbidden) and succeeds', async () => {
176170
const logger = createMockLogger();
177-
const resetTime = Math.floor((Date.now() + 60_000) / 1000);
178-
const error = {
179-
status: 403,
180-
message: 'Resource not accessible',
181-
response: {
182-
headers: {
183-
'x-ratelimit-remaining': '4999',
184-
'x-ratelimit-reset': String(resetTime),
185-
},
186-
},
187-
};
171+
const error = { status: 403, message: 'Forbidden' };
188172
const fetchFn = vi.fn()
189173
.mockRejectedValueOnce(error)
190174
.mockResolvedValueOnce('success');
@@ -196,16 +180,6 @@ describe('fetchWithRetry', () => {
196180
const result = await resultPromise;
197181
expect(result).toBe('success');
198182
expect(fetchFn).toHaveBeenCalledTimes(2);
199-
expect(logger.warn).toHaveBeenCalledWith(
200-
expect.stringContaining('Request failed for test with status 403'),
201-
{
202-
httpStatus: 403,
203-
responseHeaders: {
204-
'x-ratelimit-remaining': '4999',
205-
'x-ratelimit-reset': String(resetTime),
206-
},
207-
},
208-
);
209183
});
210184

211185
test('retries on 503 (Service Unavailable) and succeeds', async () => {
@@ -222,13 +196,6 @@ describe('fetchWithRetry', () => {
222196
const result = await resultPromise;
223197
expect(result).toBe('success');
224198
expect(fetchFn).toHaveBeenCalledTimes(2);
225-
expect(logger.warn).toHaveBeenCalledWith(
226-
expect.stringContaining('Request failed for test with status 503'),
227-
{
228-
httpStatus: 503,
229-
responseHeaders: {},
230-
},
231-
);
232199
});
233200

234201
test('retries on 500 (Internal Server Error) and succeeds', async () => {
@@ -286,20 +253,15 @@ describe('fetchWithRetry', () => {
286253
expect(result).toBe('success');
287254
});
288255

289-
test('respects x-ratelimit-reset when the primary rate limit is exhausted', async () => {
256+
test('respects x-ratelimit-reset header for Octokit errors', async () => {
290257
const logger = createMockLogger();
291258
const now = Date.now();
292259
const resetTime = Math.floor((now + 5000) / 1000); // 5 seconds from now
293260

294261
const error = new RequestError('Rate limit exceeded', 429, {
295262
response: {
296263
headers: {
297-
'x-ratelimit-limit': '5000',
298-
'x-ratelimit-remaining': '0',
299-
'x-ratelimit-used': '5000',
300-
'x-ratelimit-resource': 'core',
301264
'x-ratelimit-reset': String(resetTime),
302-
'x-github-request-id': 'ABC1:DEF2:1234:5678',
303265
},
304266
status: 429,
305267
url: 'https://api.github.com/test',
@@ -324,52 +286,6 @@ describe('fetchWithRetry', () => {
324286
const result = await resultPromise;
325287
expect(result).toBe('success');
326288
expect(fetchFn).toHaveBeenCalledTimes(2);
327-
expect(logger.warn).toHaveBeenCalledWith(
328-
expect.stringContaining('Rate limit exceeded for test'),
329-
{
330-
httpStatus: 429,
331-
responseHeaders: {
332-
'x-ratelimit-limit': '5000',
333-
'x-ratelimit-remaining': '0',
334-
'x-ratelimit-used': '5000',
335-
'x-ratelimit-resource': 'core',
336-
'x-ratelimit-reset': String(resetTime),
337-
'x-github-request-id': 'ABC1:DEF2:1234:5678',
338-
},
339-
},
340-
);
341-
});
342-
343-
test('respects retry-after for secondary rate limits', async () => {
344-
const logger = createMockLogger();
345-
const error = new RequestError('You have exceeded a secondary rate limit.', 403, {
346-
response: {
347-
headers: {
348-
'retry-after': '30',
349-
'x-ratelimit-remaining': '42',
350-
},
351-
status: 403,
352-
url: 'https://api.github.com/test',
353-
data: {},
354-
},
355-
request: {
356-
method: 'GET',
357-
url: 'https://api.github.com/test',
358-
headers: {},
359-
},
360-
});
361-
const fetchFn = vi.fn()
362-
.mockRejectedValueOnce(error)
363-
.mockResolvedValueOnce('success');
364-
365-
const resultPromise = fetchWithRetry(fetchFn, 'test', logger);
366-
367-
await vi.advanceTimersByTimeAsync(29_999);
368-
expect(fetchFn).toHaveBeenCalledTimes(1);
369-
await vi.advanceTimersByTimeAsync(1);
370-
371-
await expect(resultPromise).resolves.toBe('success');
372-
expect(fetchFn).toHaveBeenCalledTimes(2);
373289
});
374290

375291
test('respects custom maxAttempts parameter', async () => {
@@ -401,11 +317,7 @@ describe('fetchWithRetry', () => {
401317

402318
expect(logger.warn).toHaveBeenCalledTimes(1);
403319
expect(logger.warn).toHaveBeenCalledWith(
404-
expect.stringContaining('test-identifier'),
405-
{
406-
httpStatus: 429,
407-
responseHeaders: {},
408-
},
320+
expect.stringContaining('test-identifier with status 429')
409321
);
410322
});
411323
});

‎packages/backend/src/utils.ts‎

Lines changed: 11 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { GithubConnectionConfig, GitlabConnectionConfig, GiteaConnectionConfig,
77
import { GithubAppManager } from "./ee/githubAppManager.js";
88
import { hasEntitlement } from "./entitlements.js";
99
import { StatusCodes } from "http-status-codes";
10-
import { getErrorHeader, getErrorStatus, isGitHubRateLimitError } from "./errors.js";
10+
import { isOctokitRequestError } from "./github.js";
1111

1212
export const measure = async <T>(cb: () => Promise<T>) => {
1313
const start = Date.now();
@@ -80,61 +80,26 @@ export const fetchWithRetry = async <T>(
8080
Sentry.captureException(e);
8181

8282
attempts++;
83-
const status = getErrorStatus(e);
84-
const isRateLimitError = isGitHubRateLimitError(e);
85-
const isServerError = status !== null && status >= 500 && status < 600;
8683
if (
8784
(
88-
isServerError ||
89-
status === StatusCodes.FORBIDDEN ||
90-
status === StatusCodes.TOO_MANY_REQUESTS
85+
(e.status >= 500 && e.status < 600) ||
86+
e.status === StatusCodes.FORBIDDEN ||
87+
e.status === StatusCodes.TOO_MANY_REQUESTS
9188
) && attempts < maxAttempts
9289
) {
93-
const now = Date.now();
94-
const retryAfter = getErrorHeader(e, 'retry-after');
95-
const rateLimitRemaining = getErrorHeader(e, 'x-ratelimit-remaining');
96-
const rateLimitReset = getErrorHeader(e, 'x-ratelimit-reset');
9790
const resetDateMs = (() => {
98-
if (isRateLimitError && retryAfter) {
99-
const retryAfterSeconds = Number(retryAfter);
100-
if (Number.isFinite(retryAfterSeconds) && retryAfterSeconds >= 0) {
101-
return now + retryAfterSeconds * 1000;
102-
}
103-
}
104-
105-
if (isRateLimitError && rateLimitRemaining === '0' && rateLimitReset) {
106-
const resetTimeSeconds = Number(rateLimitReset);
107-
if (Number.isFinite(resetTimeSeconds)) {
108-
return resetTimeSeconds * 1000;
109-
}
91+
// First, try to see if we have a reset date specified in the response headers
92+
if (isOctokitRequestError(e) && e.response?.headers['x-ratelimit-reset']) {
93+
return parseInt(e.response.headers['x-ratelimit-reset']) * 1000;
11094
}
11195

112-
// Default to an exponential backoff approach.
96+
// Default to a exponential backoff approach
11397
const defaultWaitTime = 3000 * Math.pow(2, attempts - 1);
114-
return now + defaultWaitTime;
98+
return Date.now() + defaultWaitTime;
11599
})();
116100

117-
const waitTime = Math.max(0, resetDateMs - now);
118-
const responseHeaders = Object.fromEntries([
119-
'x-ratelimit-limit',
120-
'x-ratelimit-remaining',
121-
'x-ratelimit-used',
122-
'x-ratelimit-resource',
123-
'x-ratelimit-reset',
124-
'retry-after',
125-
'x-github-request-id',
126-
].flatMap((header) => {
127-
const value = getErrorHeader(e, header);
128-
return value === undefined ? [] : [[header, value]];
129-
}));
130-
const message = isRateLimitError
131-
? `Rate limit exceeded for ${identifier}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...`
132-
: `Request failed for ${identifier} with status ${status}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...`;
133-
134-
logger.warn(message, {
135-
httpStatus: status,
136-
responseHeaders,
137-
});
101+
const waitTime = Math.max(0, resetDateMs - Date.now());
102+
logger.warn(`Request failed for ${identifier} with status ${e.status}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...`);
138103

139104
await new Promise(resolve => setTimeout(resolve, waitTime));
140105
continue;

0 commit comments

Comments
 (0)