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..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'; @@ -318,6 +319,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. + let didReject = false; + const settled = integration.fetch('https://h.com/api/v4').catch(() => { + didReject = true; + }); + await jest.advanceTimersByTimeAsync(100); + await jest.advanceTimersByTimeAsync(200); + await settled; + + expect(didReject).toBe(true); + expect(callCount).toBe(3); // initial + 2 retries + }); }); }); @@ -414,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 19c349af9e..4e7f98c649 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 = (url, options) => fetch(url, options); const retryConfig = this.integrationConfig.retry; if (retryConfig) { @@ -109,35 +107,82 @@ 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 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; + try { + response = await fetchFn(url, options); + } catch (e) { + // The caller aborted — surface that immediately rather than retrying. + if (abortSignal?.aborted) throw e; + // No more attempts left — propagate the network error. + if (attempt++ >= maxRetries) throw e; + await sleep(backoffDelay(attempt), abortSignal); + if (abortSignal?.aborted) throw e; + continue; } - // If this was the last allowed attempt, return response - if (attempt++ >= maxRetries) { - break; + // Successful, non-retryable response: return immediately + if (!retryStatusCodes.includes(response.status)) { + 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 - : Math.min(100 * Math.pow(2, attempt - 1), 10000); // Exponential backoff, cap at 10 seconds + + // No more attempts left — return the last (retryable) response. + if (attempt++ >= maxRetries) { + return response; + } + + // 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; } - - 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,