Merge pull request #34117 from backstage/freben/gitlab-integration-fetch-fixes

fix(integration): correct GitLab fetch mode and retry on network errors

.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.

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(

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,