From 6b112d3fe86dafb6d6f86ac83357f67d8d75abf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 4 May 2026 10:43:49 +0200 Subject: [PATCH 1/6] fix(integration): correct GitLab fetch mode and retry on network errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes a misplaced `mode: 'same-origin'` option that would have rejected cross-origin requests when the integration is used from a browser, and extends the retry wrapper so transient network errors are retried using the configured `maxRetries`. Caller-initiated aborts still propagate immediately. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Fredrik Adelöw --- .changeset/gitlab-integration-fetch-fixes.md | 8 +++ .../src/gitlab/GitLabIntegration.test.ts | 63 +++++++++++++++++++ .../src/gitlab/GitLabIntegration.ts | 36 ++++++----- 3 files changed, 93 insertions(+), 14 deletions(-) create mode 100644 .changeset/gitlab-integration-fetch-fixes.md diff --git a/.changeset/gitlab-integration-fetch-fixes.md b/.changeset/gitlab-integration-fetch-fixes.md new file mode 100644 index 0000000000..a00fa4d6fa --- /dev/null +++ b/.changeset/gitlab-integration-fetch-fixes.md @@ -0,0 +1,8 @@ +--- +'@backstage/integration': patch +--- + +Fixed two issues in the GitLab integration's fetch behavior: + +- The internal fetch wrapper was passing `mode: 'same-origin'` on every request. This had no practical effect server-side, but would have caused cross-origin requests to be rejected when the integration is used from a browser. Requests now use the default fetch mode and work correctly in both browser and Node environments. +- When retries are configured, transient network errors (such as dropped connections or DNS hiccups) are now retried using the same `maxRetries` and exponential delay as retryable HTTP status codes. Previously, a thrown fetch error would propagate immediately on the first failure regardless of the retry configuration. Caller-initiated aborts continue to surface immediately without being retried. diff --git a/packages/integration/src/gitlab/GitLabIntegration.test.ts b/packages/integration/src/gitlab/GitLabIntegration.test.ts index 7132849a7c..0d817a60c1 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.test.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.test.ts @@ -318,6 +318,69 @@ describe('GitLabIntegration', () => { expect(response.status).toBe(200); expect(callCount).toBe(3); }); + + it('retries on transient network errors and returns once recovered', async () => { + let callCount = 0; + worker.use( + rest.get('https://h.com/api/v4', (_req, res, ctx) => { + callCount += 1; + if (callCount === 1) { + return res.networkError('boom'); + } + return res(ctx.status(200), ctx.json({})); + }), + ); + + const integration = new GitLabIntegration({ + host: 'h.com', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', + retry: { + maxRetries: 3, + retryStatusCodes: [429], + }, + }); + + const responsePromise = integration.fetch('https://h.com/api/v4'); + await jest.advanceTimersByTimeAsync(100); + const response = await responsePromise; + + expect(response.status).toBe(200); + expect(callCount).toBe(2); + }); + + it('surfaces the error after exhausting retries on persistent network errors', async () => { + let callCount = 0; + worker.use( + rest.get('https://h.com/api/v4', (_req, res) => { + callCount += 1; + return res.networkError('boom'); + }), + ); + + const integration = new GitLabIntegration({ + host: 'h.com', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', + retry: { + maxRetries: 2, + retryStatusCodes: [429], + }, + }); + + // Attach the catch handler before advancing timers so the in-flight + // rejections don't surface as unhandled. + const caught = integration + .fetch('https://h.com/api/v4') + .catch((e: unknown) => e); + await jest.advanceTimersByTimeAsync(100); + await jest.advanceTimersByTimeAsync(200); + const error = await caught; + + expect(error).toBeTruthy(); + expect(String(error)).toMatch(/fetch/i); + expect(callCount).toBe(3); // initial + 2 retries + }); }); }); diff --git a/packages/integration/src/gitlab/GitLabIntegration.ts b/packages/integration/src/gitlab/GitLabIntegration.ts index 19c349af9e..9b326f3dea 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.ts @@ -75,9 +75,7 @@ export class GitLabIntegration implements ScmIntegration { } private createFetchStrategy(): FetchFunction { - let fetchFn: FetchFunction = async (url, options) => { - return fetch(url, { ...options, mode: 'same-origin' }); - }; + let fetchFn: FetchFunction = fetch; const retryConfig = this.integrationConfig.retry; if (retryConfig) { @@ -111,29 +109,39 @@ export class GitLabIntegration implements ScmIntegration { return async (url, options) => { const abortSignal = options?.signal; - let response: Response; let attempt = 0; for (;;) { - response = await fetchFn(url, options); - // If response is not retryable, return immediately - if (!retryStatusCodes.includes(response.status)) { - break; + let response: Response | undefined; + let error: unknown; + try { + response = await fetchFn(url, options); + } catch (e) { + // The caller aborted — surface that immediately rather than retrying. + if (abortSignal?.aborted) { + throw e; + } + error = e; } - // If this was the last allowed attempt, return response - if (attempt++ >= maxRetries) { - break; + // Successful, non-retryable response: return immediately + if (response && !retryStatusCodes.includes(response.status)) { + return response; } + + // Out of attempts: surface the response or rethrow the captured error + if (attempt++ >= maxRetries) { + if (error) throw error; + return response!; + } + // Determine delay from Retry-After header if present, otherwise exponential backoff - const retryAfter = response.headers.get('Retry-After'); + const retryAfter = response?.headers.get('Retry-After'); const delay = retryAfter ? parseInt(retryAfter, 10) * 1000 : Math.min(100 * Math.pow(2, attempt - 1), 10000); // Exponential backoff, cap at 10 seconds await sleep(delay, abortSignal); } - - return response; }; } } From 90d3968e6ce0978099cfa4772179b8c1a4d31764 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 4 May 2026 11:01:41 +0200 Subject: [PATCH 2/6] address review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cancel discarded response bodies before retrying so the underlying connection can be returned to the pool instead of being held open until the response is garbage collected. - Stop asserting on the rejected error message in the network-error retry test; track rejection via a flag so the test isn't tied to fetch/MSW error strings. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Fredrik Adelöw --- .../integration/src/gitlab/GitLabIntegration.test.ts | 12 ++++++------ packages/integration/src/gitlab/GitLabIntegration.ts | 4 ++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/integration/src/gitlab/GitLabIntegration.test.ts b/packages/integration/src/gitlab/GitLabIntegration.test.ts index 0d817a60c1..dcd2965cb3 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.test.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.test.ts @@ -370,15 +370,15 @@ describe('GitLabIntegration', () => { // Attach the catch handler before advancing timers so the in-flight // rejections don't surface as unhandled. - const caught = integration - .fetch('https://h.com/api/v4') - .catch((e: unknown) => e); + let didReject = false; + const settled = integration.fetch('https://h.com/api/v4').catch(() => { + didReject = true; + }); await jest.advanceTimersByTimeAsync(100); await jest.advanceTimersByTimeAsync(200); - const error = await caught; + await settled; - expect(error).toBeTruthy(); - expect(String(error)).toMatch(/fetch/i); + expect(didReject).toBe(true); expect(callCount).toBe(3); // initial + 2 retries }); }); diff --git a/packages/integration/src/gitlab/GitLabIntegration.ts b/packages/integration/src/gitlab/GitLabIntegration.ts index 9b326f3dea..d76fd0e0cb 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.ts @@ -140,6 +140,10 @@ export class GitLabIntegration implements ScmIntegration { ? parseInt(retryAfter, 10) * 1000 : Math.min(100 * Math.pow(2, attempt - 1), 10000); // Exponential backoff, cap at 10 seconds + // Release the underlying connection so it can be reused, since we're + // about to discard this response in favor of a retry. + await response?.body?.cancel().catch(() => {}); + await sleep(delay, abortSignal); } }; From d7f994308ec3bea5a6b0b95f3594037fb48624ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 4 May 2026 11:17:51 +0200 Subject: [PATCH 3/6] drop non-null assertion in retry loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flip the order of the response/error check so TypeScript narrows the types itself rather than relying on `response!` to assert what the code already guarantees by construction. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Fredrik Adelöw --- packages/integration/src/gitlab/GitLabIntegration.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/integration/src/gitlab/GitLabIntegration.ts b/packages/integration/src/gitlab/GitLabIntegration.ts index d76fd0e0cb..31b9bb1578 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.ts @@ -130,8 +130,8 @@ export class GitLabIntegration implements ScmIntegration { // Out of attempts: surface the response or rethrow the captured error if (attempt++ >= maxRetries) { - if (error) throw error; - return response!; + if (response) return response; + throw error; } // Determine delay from Retry-After header if present, otherwise exponential backoff From 1500fe31b431aa62aa9d69712cbb8c58fe11c414 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 4 May 2026 11:28:29 +0200 Subject: [PATCH 4/6] restructure retry loop to remove parallel response/error variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each catch path now decides for itself whether to rethrow or sleep and continue, so the loop body has a definite Response after the try/catch and there is no shared `error: unknown` slot that could in principle be thrown unset. Pulls the exponential delay into a tiny local helper to share between the two retry paths. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Fredrik Adelöw --- .../src/gitlab/GitLabIntegration.ts | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/packages/integration/src/gitlab/GitLabIntegration.ts b/packages/integration/src/gitlab/GitLabIntegration.ts index 31b9bb1578..f917ac92d9 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.ts @@ -107,42 +107,45 @@ export class GitLabIntegration implements ScmIntegration { return fetchFn; } + // Exponential backoff, cap at 10 seconds + const backoffDelay = (a: number) => + Math.min(100 * Math.pow(2, a - 1), 10000); + return async (url, options) => { const abortSignal = options?.signal; let attempt = 0; for (;;) { - let response: Response | undefined; - let error: unknown; + let response: Response; try { response = await fetchFn(url, options); } catch (e) { // The caller aborted — surface that immediately rather than retrying. - if (abortSignal?.aborted) { - throw e; - } - error = e; + if (abortSignal?.aborted) throw e; + // No more attempts left — propagate the network error. + if (attempt++ >= maxRetries) throw e; + await sleep(backoffDelay(attempt), abortSignal); + continue; } // Successful, non-retryable response: return immediately - if (response && !retryStatusCodes.includes(response.status)) { + if (!retryStatusCodes.includes(response.status)) { return response; } - // Out of attempts: surface the response or rethrow the captured error + // No more attempts left — return the last (retryable) response. if (attempt++ >= maxRetries) { - if (response) return response; - throw error; + return response; } // Determine delay from Retry-After header if present, otherwise exponential backoff - const retryAfter = response?.headers.get('Retry-After'); + const retryAfter = response.headers.get('Retry-After'); const delay = retryAfter ? parseInt(retryAfter, 10) * 1000 - : Math.min(100 * Math.pow(2, attempt - 1), 10000); // Exponential backoff, cap at 10 seconds + : backoffDelay(attempt); // Release the underlying connection so it can be reused, since we're // about to discard this response in favor of a retry. - await response?.body?.cancel().catch(() => {}); + await response.body?.cancel().catch(() => {}); await sleep(delay, abortSignal); } From dd78c9cffa4a1b2327ff4bc0a151a0de37e035c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 4 May 2026 14:01:43 +0200 Subject: [PATCH 5/6] defer fetch lookup so test interceptors apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Capturing globalThis.fetch as a value at construction time meant the GitLabUrlReader tests bypassed the MSW fetch interceptor and hit the real gitlab.com (returning 401). Wrapping the call resolves fetch at invocation time so the patched fetch is used. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Fredrik Adelöw --- packages/integration/src/gitlab/GitLabIntegration.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/integration/src/gitlab/GitLabIntegration.ts b/packages/integration/src/gitlab/GitLabIntegration.ts index f917ac92d9..4f6985e344 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.ts @@ -75,7 +75,7 @@ export class GitLabIntegration implements ScmIntegration { } private createFetchStrategy(): FetchFunction { - let fetchFn: FetchFunction = fetch; + let fetchFn: FetchFunction = (url, options) => fetch(url, options); const retryConfig = this.integrationConfig.retry; if (retryConfig) { From da5ba4eecdbfb4e49d054f13cebebbf6d1c18e0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 4 May 2026 14:27:58 +0200 Subject: [PATCH 6/6] Handle non-numeric Retry-After header values gracefully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validate the parsed Retry-After value is a finite positive number before using it as a delay, falling back to exponential backoff otherwise. This prevents a near-zero delay tight retry loop when the header contains an HTTP-date or unexpected value. Signed-off-by: Fredrik Adelöw Co-authored-by: Cursor Signed-off-by: Fredrik Adelöw Co-authored-by: Cursor Signed-off-by: Fredrik Adelöw Co-authored-by: Cursor --- .../src/gitlab/GitLabIntegration.test.ts | 34 ++++++++++++++++ .../src/gitlab/GitLabIntegration.ts | 40 ++++++++++++++++--- 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/packages/integration/src/gitlab/GitLabIntegration.test.ts b/packages/integration/src/gitlab/GitLabIntegration.test.ts index dcd2965cb3..156ea0ef3b 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.test.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.test.ts @@ -19,6 +19,7 @@ import { rest } from 'msw'; import { ConfigReader } from '@backstage/config'; import { GitLabIntegration, + parseRetryAfterMs, replaceGitLabUrlType, sleep, } from './GitLabIntegration'; @@ -477,6 +478,39 @@ describe('sleep', () => { }); }); +describe('parseRetryAfterMs', () => { + it('parses delay-seconds', () => { + expect(parseRetryAfterMs('120', 5000)).toBe(120_000); + expect(parseRetryAfterMs('1', 5000)).toBe(1000); + }); + + it('parses an HTTP-date and computes a delta', () => { + const futureDate = new Date(Date.now() + 30_000).toUTCString(); + const result = parseRetryAfterMs(futureDate, 5000); + expect(result).toBeGreaterThan(29_000); + expect(result).toBeLessThanOrEqual(30_000); + }); + + it('returns 0 for an HTTP-date in the past', () => { + const pastDate = new Date(Date.now() - 10_000).toUTCString(); + expect(parseRetryAfterMs(pastDate, 5000)).toBe(0); + }); + + it('returns fallback for null or unparseable values', () => { + expect(parseRetryAfterMs(null, 5000)).toBe(5000); + expect(parseRetryAfterMs('', 5000)).toBe(5000); + expect(parseRetryAfterMs('not-a-date-or-number', 5000)).toBe(5000); + }); + + it('treats zero seconds as retry-immediately', () => { + expect(parseRetryAfterMs('0', 5000)).toBe(0); + }); + + it('returns fallback for negative seconds', () => { + expect(parseRetryAfterMs('-5', 5000)).toBe(5000); + }); +}); + describe('replaceGitLabUrlType', () => { it('should replace with expected type', () => { expect( diff --git a/packages/integration/src/gitlab/GitLabIntegration.ts b/packages/integration/src/gitlab/GitLabIntegration.ts index 4f6985e344..4e7f98c649 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.ts @@ -124,6 +124,7 @@ export class GitLabIntegration implements ScmIntegration { // No more attempts left — propagate the network error. if (attempt++ >= maxRetries) throw e; await sleep(backoffDelay(attempt), abortSignal); + if (abortSignal?.aborted) throw e; continue; } @@ -137,22 +138,51 @@ export class GitLabIntegration implements ScmIntegration { return response; } - // Determine delay from Retry-After header if present, otherwise exponential backoff - const retryAfter = response.headers.get('Retry-After'); - const delay = retryAfter - ? parseInt(retryAfter, 10) * 1000 - : backoffDelay(attempt); + // Retry-After is either delay-seconds or an HTTP-date (RFC 9110 §10.2.3). + const delay = parseRetryAfterMs( + response.headers.get('Retry-After'), + backoffDelay(attempt), + ); // Release the underlying connection so it can be reused, since we're // about to discard this response in favor of a retry. await response.body?.cancel().catch(() => {}); await sleep(delay, abortSignal); + if (abortSignal?.aborted) return response; } }; } } +/** @internal */ +export function parseRetryAfterMs( + headerValue: string | null, + fallbackMs: number, +): number { + if (!headerValue) { + return fallbackMs; + } + + // delay-seconds per RFC 9110 is 1*DIGIT + if (/^\d+$/.test(headerValue)) { + return Number(headerValue) * 1000; + } + + // HTTP-dates (IMF-fixdate) always contain a comma, e.g. + // "Sun, 06 Nov 1994 08:49:37 GMT" — use that as a prerequisite + // to avoid Date.parse interpreting random strings as dates. + if (headerValue.includes(',')) { + const dateMs = Date.parse(headerValue); + if (Number.isFinite(dateMs)) { + const deltaMs = dateMs - Date.now(); + return deltaMs > 0 ? deltaMs : 0; + } + } + + return fallbackMs; +} + /** @internal */ export async function sleep( durationMs: number,