From a213182d9dd8229e0ead94d04f4f01be54fd4836 Mon Sep 17 00:00:00 2001 From: Vigneshraj Sekar Babu Date: Mon, 21 Sep 2026 11:47:48 -0700 Subject: [PATCH 1/3] fix(github): retry transient failures on cached GET requests A dropped keep-alive socket surfaces as an octokit RequestError with status 500, which cacheRequest reported as a hard failure with no retry. Callers that read repository config through it then could not tell a network blip from a missing config file: a pull request opened with autoDeploy enabled was treated as a non-autoDeploy repo, so its environment never received the deploy label, and nothing later re-read the config to correct it. Retry idempotent GET requests twice, at 5s and 15s. Writes are excluded so a retried POST cannot duplicate a deployment, and the budget stays under the job queue's 30s lock so a retry cannot outlive a worker restart. --- .../lib/github/__tests__/cacheRequest.test.ts | 59 +++++++++++++++++++ src/server/lib/github/cacheRequest.ts | 12 +++- 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/src/server/lib/github/__tests__/cacheRequest.test.ts b/src/server/lib/github/__tests__/cacheRequest.test.ts index 247834c8..bb70a5de 100644 --- a/src/server/lib/github/__tests__/cacheRequest.test.ts +++ b/src/server/lib/github/__tests__/cacheRequest.test.ts @@ -44,6 +44,7 @@ describe('cacheRequest', () => { let logger: { debug: jest.Mock; info: jest.Mock; + warn: jest.Mock; error: jest.Mock; }; @@ -54,6 +55,7 @@ describe('cacheRequest', () => { logger = { debug: jest.fn(), info: jest.fn(), + warn: jest.fn(), error: jest.fn(), }; (redisClient.getRedis as jest.Mock).mockReturnValue(cache); @@ -328,4 +330,61 @@ describe('cacheRequest', () => { expect(cache.hset).toHaveBeenCalledTimes(1); expect(cache.expire).not.toHaveBeenCalled(); }); + + describe('transient failures', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('retries a GET that fails with a server error and returns the eventual response', async () => { + const endpoint = 'GET /repos/acme/widget/git/trees/abc123'; + const response = { status: 200, headers: {}, data: { tree: [] } }; + request + .mockRejectedValueOnce(Object.assign(new Error('other side closed'), { status: 500 })) + .mockResolvedValueOnce(response); + + const pending = cacheRequest(endpoint, {}, { cache }); + await jest.advanceTimersByTimeAsync(20_000); + const result = await pending; + + expect(result).toBe(response); + expect(request).toHaveBeenCalledTimes(2); + expect(logger.warn).toHaveBeenCalledWith('GitHub: cache request retrying'); + expect(getLogger).toHaveBeenCalledWith({ endpoint, attempt: 1, status: 500 }); + expect(cache.hset).toHaveBeenCalledTimes(1); + }); + + it('does not retry a non-GET endpoint, so a write is never duplicated', async () => { + const endpoint = 'POST /repos/acme/widget/deployments'; + request.mockRejectedValue(Object.assign(new Error('other side closed'), { status: 500 })); + + const assertion = expect(cacheRequest(endpoint, { data: { ref: 'main' } }, { cache })).rejects.toMatchObject({ + message: 'GitHub API request failed', + }); + await jest.advanceTimersByTimeAsync(20_000); + await assertion; + + expect(request).toHaveBeenCalledTimes(1); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it('gives up once the retry budget is exhausted', async () => { + const endpoint = 'GET /repos/acme/widget'; + request.mockRejectedValue(Object.assign(new Error('other side closed'), { status: 500 })); + + const assertion = expect(cacheRequest(endpoint, {}, { cache })).rejects.toMatchObject({ + message: 'GitHub API request failed', + }); + await jest.advanceTimersByTimeAsync(20_000); + await assertion; + + expect(request).toHaveBeenCalledTimes(3); + expect(logger.warn).toHaveBeenCalledTimes(2); + expect(logger.error).toHaveBeenCalledWith('GitHub: cache request failed'); + }); + }); }); diff --git a/src/server/lib/github/cacheRequest.ts b/src/server/lib/github/cacheRequest.ts index 733678fe..5f7abfb1 100644 --- a/src/server/lib/github/cacheRequest.ts +++ b/src/server/lib/github/cacheRequest.ts @@ -22,10 +22,12 @@ import { CacheRequestData } from 'server/lib/github/types'; import { redisClient } from 'server/lib/dependencies'; +const TRANSIENT_RETRY_DELAYS_MS = [5_000, 15_000]; + export async function cacheRequest( endpoint: string, requestData = {} as CacheRequestData, - { cache = redisClient.getRedis(), ignoreCache = false } = {} + { cache = redisClient.getRedis(), ignoreCache = false, attempt = 1 } = {} ) { const cacheKey = `github:req_cache:${endpoint}`; let cached; @@ -73,11 +75,17 @@ export async function cacheRequest( getLogger({ endpoint, cacheHit: true }).debug('GitHub: cache request hit'); return { data, cacheHit: true }; } catch (error) { - return cacheRequest(endpoint, requestData, { cache, ignoreCache: true }); + return cacheRequest(endpoint, requestData, { cache, ignoreCache: true, attempt }); } } else if (error?.status === 404) { getLogger().info(`GitHub: cache request not found endpoint=${endpoint}`); throw new Error('Resource not found'); + } else if (endpoint.startsWith('GET ') && error?.status >= 500 && attempt <= TRANSIENT_RETRY_DELAYS_MS.length) { + // Octokit reports a dropped keep-alive socket as status 500, indistinguishable from a real 5xx. + // Delays are spaced to miss the same dead socket, and sum to under the queue's 30s job lock. + getLogger({ endpoint, attempt, status: error.status }).warn('GitHub: cache request retrying'); + await new Promise((resolve) => setTimeout(resolve, TRANSIENT_RETRY_DELAYS_MS[attempt - 1])); + return cacheRequest(endpoint, requestData, { cache, ignoreCache, attempt: attempt + 1 }); } else { const errorHeaders = error?.response?.headers || error?.headers; getLogger({ From 35d6b6b97b33b0904ee3bcb99d02efdaaf35da9f Mon Sep 17 00:00:00 2001 From: Vigneshraj Sekar Babu Date: Mon, 21 Sep 2026 11:53:19 -0700 Subject: [PATCH 2/3] test(github): cover a 304 refetch that hits a transient failure The cached-body refetch and the transient retry are two separate recursive paths through cacheRequest. This pins the attempt budget being preserved across the first and consumed only by the second. Also corrects the retry comment: the delays are bounded by a worker's shutdown grace, not by the job lock, which is renewed while the process runs. --- .../lib/github/__tests__/cacheRequest.test.ts | 18 ++++++++++++++++++ src/server/lib/github/cacheRequest.ts | 3 ++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/server/lib/github/__tests__/cacheRequest.test.ts b/src/server/lib/github/__tests__/cacheRequest.test.ts index bb70a5de..0c07d3a3 100644 --- a/src/server/lib/github/__tests__/cacheRequest.test.ts +++ b/src/server/lib/github/__tests__/cacheRequest.test.ts @@ -372,6 +372,24 @@ describe('cacheRequest', () => { expect(logger.warn).not.toHaveBeenCalled(); }); + it('keeps the retry budget intact when a 304 refetch is what hits the server error', async () => { + const endpoint = 'GET /repos/acme/widget'; + const response = { status: 200, headers: {}, data: { id: 17 } }; + cache.hgetall.mockResolvedValue({ etag: '"stale-etag"', lastModified: '', data: 'not-json' }); + request + .mockRejectedValueOnce(Object.assign(new Error('Not Modified'), { status: 304 })) + .mockRejectedValueOnce(Object.assign(new Error('other side closed'), { status: 500 })) + .mockRejectedValueOnce(Object.assign(new Error('other side closed'), { status: 500 })) + .mockResolvedValueOnce(response); + + const pending = cacheRequest(endpoint, {}, { cache }); + await jest.advanceTimersByTimeAsync(20_000); + + expect(await pending).toBe(response); + expect(request).toHaveBeenCalledTimes(4); + expect(logger.warn).toHaveBeenCalledTimes(2); + }); + it('gives up once the retry budget is exhausted', async () => { const endpoint = 'GET /repos/acme/widget'; request.mockRejectedValue(Object.assign(new Error('other side closed'), { status: 500 })); diff --git a/src/server/lib/github/cacheRequest.ts b/src/server/lib/github/cacheRequest.ts index 5f7abfb1..3eabbebd 100644 --- a/src/server/lib/github/cacheRequest.ts +++ b/src/server/lib/github/cacheRequest.ts @@ -82,7 +82,8 @@ export async function cacheRequest( throw new Error('Resource not found'); } else if (endpoint.startsWith('GET ') && error?.status >= 500 && attempt <= TRANSIENT_RETRY_DELAYS_MS.length) { // Octokit reports a dropped keep-alive socket as status 500, indistinguishable from a real 5xx. - // Delays are spaced to miss the same dead socket, and sum to under the queue's 30s job lock. + // Delays are spaced to miss the same dead socket, and kept short so a sleeping retry does + // not outlive a worker's shutdown grace and leave its job to stall recovery. getLogger({ endpoint, attempt, status: error.status }).warn('GitHub: cache request retrying'); await new Promise((resolve) => setTimeout(resolve, TRANSIENT_RETRY_DELAYS_MS[attempt - 1])); return cacheRequest(endpoint, requestData, { cache, ignoreCache, attempt: attempt + 1 }); From 03cd8dfc62390e838ba1252a93c58c929e640a3f Mon Sep 17 00:00:00 2001 From: Vigneshraj Sekar Babu Date: Mon, 21 Sep 2026 12:04:11 -0700 Subject: [PATCH 3/3] refactor(github): retry a transient GET once, on a single delay Replaces the two-entry delay schedule and its attempt counter with one constant and a boolean. The retried request is the same idempotent GET, so the only thing the counter bought was a second wait, and a 10s gap already clears the dropped socket that motivated the retry. A failure that outlives it is an outage rather than a blip, and wants a different fix. --- .../lib/github/__tests__/cacheRequest.test.ts | 23 +++++++++---------- src/server/lib/github/cacheRequest.ts | 18 +++++++-------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/server/lib/github/__tests__/cacheRequest.test.ts b/src/server/lib/github/__tests__/cacheRequest.test.ts index 0c07d3a3..63636007 100644 --- a/src/server/lib/github/__tests__/cacheRequest.test.ts +++ b/src/server/lib/github/__tests__/cacheRequest.test.ts @@ -348,13 +348,13 @@ describe('cacheRequest', () => { .mockResolvedValueOnce(response); const pending = cacheRequest(endpoint, {}, { cache }); - await jest.advanceTimersByTimeAsync(20_000); + await jest.advanceTimersByTimeAsync(10_000); const result = await pending; expect(result).toBe(response); expect(request).toHaveBeenCalledTimes(2); expect(logger.warn).toHaveBeenCalledWith('GitHub: cache request retrying'); - expect(getLogger).toHaveBeenCalledWith({ endpoint, attempt: 1, status: 500 }); + expect(getLogger).toHaveBeenCalledWith({ endpoint, status: 500 }); expect(cache.hset).toHaveBeenCalledTimes(1); }); @@ -365,43 +365,42 @@ describe('cacheRequest', () => { const assertion = expect(cacheRequest(endpoint, { data: { ref: 'main' } }, { cache })).rejects.toMatchObject({ message: 'GitHub API request failed', }); - await jest.advanceTimersByTimeAsync(20_000); + await jest.advanceTimersByTimeAsync(10_000); await assertion; expect(request).toHaveBeenCalledTimes(1); expect(logger.warn).not.toHaveBeenCalled(); }); - it('keeps the retry budget intact when a 304 refetch is what hits the server error', async () => { + it('still retries when a 304 refetch is what hits the server error', async () => { const endpoint = 'GET /repos/acme/widget'; const response = { status: 200, headers: {}, data: { id: 17 } }; cache.hgetall.mockResolvedValue({ etag: '"stale-etag"', lastModified: '', data: 'not-json' }); request .mockRejectedValueOnce(Object.assign(new Error('Not Modified'), { status: 304 })) .mockRejectedValueOnce(Object.assign(new Error('other side closed'), { status: 500 })) - .mockRejectedValueOnce(Object.assign(new Error('other side closed'), { status: 500 })) .mockResolvedValueOnce(response); const pending = cacheRequest(endpoint, {}, { cache }); - await jest.advanceTimersByTimeAsync(20_000); + await jest.advanceTimersByTimeAsync(10_000); expect(await pending).toBe(response); - expect(request).toHaveBeenCalledTimes(4); - expect(logger.warn).toHaveBeenCalledTimes(2); + expect(request).toHaveBeenCalledTimes(3); + expect(logger.warn).toHaveBeenCalledTimes(1); }); - it('gives up once the retry budget is exhausted', async () => { + it('retries only once, then gives up', async () => { const endpoint = 'GET /repos/acme/widget'; request.mockRejectedValue(Object.assign(new Error('other side closed'), { status: 500 })); const assertion = expect(cacheRequest(endpoint, {}, { cache })).rejects.toMatchObject({ message: 'GitHub API request failed', }); - await jest.advanceTimersByTimeAsync(20_000); + await jest.advanceTimersByTimeAsync(10_000); await assertion; - expect(request).toHaveBeenCalledTimes(3); - expect(logger.warn).toHaveBeenCalledTimes(2); + expect(request).toHaveBeenCalledTimes(2); + expect(logger.warn).toHaveBeenCalledTimes(1); expect(logger.error).toHaveBeenCalledWith('GitHub: cache request failed'); }); }); diff --git a/src/server/lib/github/cacheRequest.ts b/src/server/lib/github/cacheRequest.ts index 3eabbebd..f96940f6 100644 --- a/src/server/lib/github/cacheRequest.ts +++ b/src/server/lib/github/cacheRequest.ts @@ -22,12 +22,12 @@ import { CacheRequestData } from 'server/lib/github/types'; import { redisClient } from 'server/lib/dependencies'; -const TRANSIENT_RETRY_DELAYS_MS = [5_000, 15_000]; +const TRANSIENT_RETRY_DELAY_MS = 10_000; export async function cacheRequest( endpoint: string, requestData = {} as CacheRequestData, - { cache = redisClient.getRedis(), ignoreCache = false, attempt = 1 } = {} + { cache = redisClient.getRedis(), ignoreCache = false, retried = false } = {} ) { const cacheKey = `github:req_cache:${endpoint}`; let cached; @@ -75,18 +75,18 @@ export async function cacheRequest( getLogger({ endpoint, cacheHit: true }).debug('GitHub: cache request hit'); return { data, cacheHit: true }; } catch (error) { - return cacheRequest(endpoint, requestData, { cache, ignoreCache: true, attempt }); + return cacheRequest(endpoint, requestData, { cache, ignoreCache: true, retried }); } } else if (error?.status === 404) { getLogger().info(`GitHub: cache request not found endpoint=${endpoint}`); throw new Error('Resource not found'); - } else if (endpoint.startsWith('GET ') && error?.status >= 500 && attempt <= TRANSIENT_RETRY_DELAYS_MS.length) { + } else if (!retried && endpoint.startsWith('GET ') && error?.status >= 500) { // Octokit reports a dropped keep-alive socket as status 500, indistinguishable from a real 5xx. - // Delays are spaced to miss the same dead socket, and kept short so a sleeping retry does - // not outlive a worker's shutdown grace and leave its job to stall recovery. - getLogger({ endpoint, attempt, status: error.status }).warn('GitHub: cache request retrying'); - await new Promise((resolve) => setTimeout(resolve, TRANSIENT_RETRY_DELAYS_MS[attempt - 1])); - return cacheRequest(endpoint, requestData, { cache, ignoreCache, attempt: attempt + 1 }); + // The wait is long enough to miss the same dead socket, and short enough that a sleeping + // retry does not outlive a worker's shutdown grace and leave its job to stall recovery. + getLogger({ endpoint, status: error.status }).warn('GitHub: cache request retrying'); + await new Promise((resolve) => setTimeout(resolve, TRANSIENT_RETRY_DELAY_MS)); + return cacheRequest(endpoint, requestData, { cache, ignoreCache, retried: true }); } else { const errorHeaders = error?.response?.headers || error?.headers; getLogger({