Skip to content

Commit ece8397

Browse files
authored
fix(api): clean up fetch retry abort listeners (JhaSourav07#1131)
## Description Fixes JhaSourav07#1130 ## Pillar - [x] 🛠️ Other (Bug fix, refactoring, docs) ## What changed `fetchWithRetry` was adding an `abort` listener to the caller-provided `AbortSignal`, but the listener was never removed after the request attempt finished. This PR keeps the abort callback in a named function and removes it in a cleanup path after each fetch attempt settles. The existing cancellation behavior is preserved: aborting the caller signal still aborts the in-flight request. I also added regression tests covering: - listener cleanup after a successful request - caller-triggered abort still cancelling the active request ## Visual Preview N/A — this is an API client cleanup fix with no UI or SVG visual change. ## Checklist before requesting a review: - [x] I have read the `CONTRIBUTING.md` file. - [x] I have tested these changes locally (`npm run test -- lib/github.test.ts`, `npm run test`, `npm run test:coverage`). - [x] I have run `npm run format` and `npm run lint` locally and resolved all errors. - [x] My commits follow the Conventional Commits format. - [x] I have updated `README.md` if I added a new theme or URL parameter. - [x] I have starred the repo. - [x] I have made sure that I have only one commit to merge in this PR. - [x] The SVG output matches the CommitPulse quality standard.
2 parents aef1152 + f17bbdc commit ece8397

2 files changed

Lines changed: 67 additions & 6 deletions

File tree

lib/github.test.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
22
import {
33
fetchGitHubContributions,
4+
fetchWithRetry,
45
fetchUserProfile,
56
fetchUserRepos,
67
getFullDashboardData,
@@ -60,6 +61,54 @@ afterEach(() => {
6061
}
6162
});
6263

64+
describe('fetchWithRetry', () => {
65+
beforeEach(() => {
66+
vi.spyOn(global, 'fetch');
67+
});
68+
69+
afterEach(() => {
70+
vi.restoreAllMocks();
71+
});
72+
73+
it('removes caller abort listeners after a successful request', async () => {
74+
const controller = new AbortController();
75+
const addListenerSpy = vi.spyOn(controller.signal, 'addEventListener');
76+
const removeListenerSpy = vi.spyOn(controller.signal, 'removeEventListener');
77+
78+
vi.mocked(fetch).mockResolvedValue(mockResponse({ ok: true }));
79+
80+
await fetchWithRetry('https://api.github.com/test', { signal: controller.signal }, 0, 1000);
81+
82+
const abortListener = addListenerSpy.mock.calls.find(([event]) => event === 'abort')?.[1];
83+
84+
expect(abortListener).toEqual(expect.any(Function));
85+
expect(addListenerSpy).toHaveBeenCalledWith('abort', abortListener, { once: true });
86+
expect(removeListenerSpy).toHaveBeenCalledWith('abort', abortListener);
87+
});
88+
89+
it('still aborts the in-flight request when the caller signal is aborted', async () => {
90+
const controller = new AbortController();
91+
92+
vi.mocked(fetch).mockImplementation(
93+
(_url: RequestInfo | URL, options?: RequestInit) =>
94+
new Promise<Response>((_resolve, reject) => {
95+
options?.signal?.addEventListener(
96+
'abort',
97+
() => reject(new DOMException('Aborted', 'AbortError')),
98+
{ once: true }
99+
);
100+
})
101+
);
102+
103+
const request = fetchWithRetry('https://api.github.com/test', { signal: controller.signal });
104+
105+
controller.abort();
106+
107+
await expect(request).rejects.toThrow('Aborted');
108+
expect(fetch).toHaveBeenCalledOnce();
109+
});
110+
});
111+
63112
describe('fetchGitHubContributions', () => {
64113
beforeEach(() => {
65114
vi.spyOn(global, 'fetch');

lib/github.ts

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,27 +29,39 @@ export async function fetchWithRetry(
2929

3030
const controller = new AbortController();
3131
const timeoutId = setTimeout(() => controller.abort(), resolvedTimeout);
32+
const abortRequest = () => controller.abort();
3233

3334
if (options.signal) {
34-
options.signal.addEventListener('abort', () => controller.abort(), { once: true });
35+
options.signal.addEventListener('abort', abortRequest, { once: true });
3536
}
3637

3738
let res: Response | null = null;
39+
let fetchError: unknown;
40+
let didThrow = false;
41+
3842
try {
3943
res = await fetch(url, { ...options, signal: controller.signal });
40-
} catch (err) {
44+
} catch (err: unknown) {
45+
fetchError = err;
46+
didThrow = true;
47+
} finally {
4148
clearTimeout(timeoutId);
42-
if (options.signal?.aborted) throw err;
43-
if (err instanceof Error && err.name === 'AbortError') {
49+
options.signal?.removeEventListener('abort', abortRequest);
50+
}
51+
52+
if (didThrow) {
53+
if (options.signal?.aborted) throw fetchError;
54+
if (fetchError instanceof Error && fetchError.name === 'AbortError') {
4455
throw new Error(`GitHub API request timed out after ${resolvedTimeout / 1000}s`);
4556
}
46-
if (attempt >= MAX_RETRIES) throw err;
57+
if (attempt >= MAX_RETRIES) throw fetchError;
4758
const delay = BASE_DELAY_MS * Math.pow(2, attempt);
4859
await new Promise((resolve) => setTimeout(resolve, delay));
4960
return fetchWithRetry(url, options, attempt + 1, timeoutMs);
5061
}
5162

52-
clearTimeout(timeoutId);
63+
if (!res) throw new Error('GitHub API request failed without a response');
64+
5365
const shouldRetry = res.status === 429 || res.status >= 500;
5466
if (!shouldRetry || attempt >= MAX_RETRIES) return res;
5567

0 commit comments

Comments
 (0)