fix(integration): correct GitLab fetch mode and retry on network errors
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) <noreply@anthropic.com> Signed-off-by: Fredrik Adelöw <freben@spotify.com>
This commit is contained in:
@@ -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.
|
||||
@@ -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
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -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;
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user