From 6d7d1c4f72ccc021a86ca0f525517f867642e6fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Wed, 20 May 2026 16:28:32 +0200 Subject: [PATCH 1/4] fix(api-client): fix retry failing and add a timeout of 30s --- packages/api-client/package.json | 6 +- packages/api-client/src/fetch.test.ts | 100 ++++++++++++++++++++++++++ packages/api-client/src/fetch.ts | 67 +++++++++++++++++ packages/api-client/src/index.ts | 30 +------- pnpm-lock.yaml | 3 + 5 files changed, 177 insertions(+), 29 deletions(-) create mode 100644 packages/api-client/src/fetch.test.ts create mode 100644 packages/api-client/src/fetch.ts diff --git a/packages/api-client/package.json b/packages/api-client/package.json index 453e48b9..b8462be9 100644 --- a/packages/api-client/package.json +++ b/packages/api-client/package.json @@ -40,13 +40,15 @@ "build": "tsdown", "check-types": "tsc", "check-format": "prettier --check --ignore-unknown --ignore-path=../../.gitignore --ignore-path=../../.prettierignore .", - "lint": "eslint ." + "lint": "eslint .", + "test": "vitest" }, "devDependencies": { "@types/debug": "^4.1.13", "@types/node": "catalog:", "openapi-typescript": "^7.13.0", - "p-retry": "^8.0.0" + "p-retry": "^8.0.0", + "vitest": "catalog:" }, "dependencies": { "debug": "^4.4.3", diff --git a/packages/api-client/src/fetch.test.ts b/packages/api-client/src/fetch.test.ts new file mode 100644 index 00000000..ebcde292 --- /dev/null +++ b/packages/api-client/src/fetch.test.ts @@ -0,0 +1,100 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { APIError, apiFetch } from "./fetch"; + +describe("apiFetch", () => { + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it("retries server errors with a fresh request and replays the body", async () => { + const cloneSpy = vi + .spyOn(Request.prototype, "clone") + .mockImplementation(() => { + throw new TypeError("unusable"); + }); + const bodies: string[] = []; + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const request = input instanceof Request ? input : new Request(input); + bodies.push(await request.text()); + + return new Response("{}", { + status: bodies.length === 1 ? 500 : 200, + }); + }); + const body = JSON.stringify({ commit: "abc123" }); + const request = new Request("https://api.argos-ci.test/builds", { + body, + headers: { + "content-type": "application/json", + }, + method: "POST", + }); + + const response = await apiFetch(request, { + fetch: fetchMock as unknown as typeof fetch, + minTimeout: 0, + }); + + expect(response.status).toBe(200); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(bodies).toEqual([body, body]); + expect(cloneSpy).not.toHaveBeenCalled(); + }); + + it("does not retry client errors", async () => { + const fetchMock = vi.fn(async () => new Response("{}", { status: 400 })); + + const response = await apiFetch( + new Request("https://api.argos-ci.test/builds"), + { + fetch: fetchMock as unknown as typeof fetch, + minTimeout: 0, + }, + ); + + expect(response.status).toBe(400); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("throws APIError after server error retries are exhausted", async () => { + const fetchMock = vi.fn(async () => new Response("{}", { status: 503 })); + + const promise = apiFetch(new Request("https://api.argos-ci.test/builds"), { + fetch: fetchMock as unknown as typeof fetch, + minTimeout: 0, + retries: 1, + }); + + await expect(promise).rejects.toThrow(APIError); + await expect(promise).rejects.toThrow("Internal Server Error (503)"); + + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("aborts the request after the configured timeout", async () => { + const fetchMock = vi.fn( + async (input: RequestInfo | URL) => + new Promise((_resolve, reject) => { + const request = input instanceof Request ? input : new Request(input); + request.signal.addEventListener( + "abort", + () => reject(request.signal.reason), + { once: true }, + ); + }), + ); + + await expect( + apiFetch(new Request("https://api.argos-ci.test/builds"), { + fetch: fetchMock as unknown as typeof fetch, + minTimeout: 0, + retries: 0, + timeout: 1, + }), + ).rejects.toMatchObject({ + name: "TimeoutError", + }); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/api-client/src/fetch.ts b/packages/api-client/src/fetch.ts new file mode 100644 index 00000000..3bb97a0c --- /dev/null +++ b/packages/api-client/src/fetch.ts @@ -0,0 +1,67 @@ +import pRetry from "p-retry"; +import { debug } from "./debug"; + +const DEFAULT_TIMEOUT = 30_000; + +export class APIError extends Error { + constructor(message: string) { + super(message); + } +} + +interface APIFetchOptions { + fetch?: typeof fetch; + minTimeout?: number; + retries?: number; + timeout?: number; +} + +async function createRequestFactory(request: Request, timeout: number) { + // Snapshot the body once so retries do not clone/tee the original Request. + const body = request.body ? await request.arrayBuffer() : undefined; + const headers = new Headers(request.headers); + + return () => + new Request(request.url, { + body, + cache: request.cache, + credentials: request.credentials, + headers, + integrity: request.integrity, + keepalive: request.keepalive, + method: request.method, + mode: request.mode, + redirect: request.redirect, + referrer: request.referrer, + referrerPolicy: request.referrerPolicy, + signal: AbortSignal.timeout(timeout), + }); +} + +export async function apiFetch(input: Request, options: APIFetchOptions = {}) { + const fetchImpl = options.fetch ?? fetch; + const createRequest = await createRequestFactory( + input, + options.timeout ?? DEFAULT_TIMEOUT, + ); + + return pRetry( + async () => { + const response = await fetchImpl(createRequest()); + if (response.status >= 500) { + throw new APIError(`Internal Server Error (${response.status})`); + } + return response; + }, + { + minTimeout: options.minTimeout, + retries: options.retries ?? 3, + onFailedAttempt: (context) => { + debug("API request failed", context.error.message); + if (context.retriesLeft > 0) { + debug(`Retrying API request... (${context.retriesLeft} left)`); + } + }, + }, + ); +} diff --git a/packages/api-client/src/index.ts b/packages/api-client/src/index.ts index 698e2681..fd22f300 100644 --- a/packages/api-client/src/index.ts +++ b/packages/api-client/src/index.ts @@ -1,9 +1,10 @@ import createFetchClient from "openapi-fetch"; -import pRetry from "p-retry"; import { debug } from "./debug"; +import { apiFetch, APIError } from "./fetch"; import type { paths, components } from "./schema"; export * as ArgosAPISchema from "./schema"; +export { APIError } from "./fetch"; export type ArgosAPIClient = ReturnType; @@ -20,35 +21,10 @@ export function createClient(options?: { headers: { Authorization: authToken ? `Bearer ${authToken}` : undefined, }, - fetch: (input) => { - return pRetry( - async () => { - const response = await fetch(input.clone()); - if (response.status >= 500) { - throw new APIError("Internal Server Error"); - } - return response; - }, - { - retries: 3, - onFailedAttempt: (context) => { - debug("API request failed", context.error.message); - if (context.retriesLeft > 0) { - debug(`Retrying API request... (${context.retriesLeft} left)`); - } - }, - }, - ); - }, + fetch: apiFetch, }); } -export class APIError extends Error { - constructor(message: string) { - super(message); - } -} - /** * Handle API errors. */ diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d86b37f0..c59c5a0c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -89,6 +89,9 @@ importers: p-retry: specifier: ^8.0.0 version: 8.0.0 + vitest: + specifier: 'catalog:' + version: 4.1.5(@types/node@20.19.40)(@vitest/browser-playwright@4.1.5)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(msw@2.14.5(@types/node@20.19.40)(typescript@6.0.3))(vite@7.3.3(@types/node@20.19.40)(jiti@2.7.0)(tsx@4.21.0)(yaml@2.8.4)) packages/browser: devDependencies: From f6696ad7685f38f7365066fd2019f10bc0c53958 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Wed, 20 May 2026 16:51:40 +0200 Subject: [PATCH 2/4] chore: handle signal --- packages/api-client/src/fetch.test.ts | 60 +++++++++++++++++++++++++++ packages/api-client/src/fetch.ts | 5 ++- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/packages/api-client/src/fetch.test.ts b/packages/api-client/src/fetch.test.ts index ebcde292..90fe6e85 100644 --- a/packages/api-client/src/fetch.test.ts +++ b/packages/api-client/src/fetch.test.ts @@ -77,6 +77,10 @@ describe("apiFetch", () => { async (input: RequestInfo | URL) => new Promise((_resolve, reject) => { const request = input instanceof Request ? input : new Request(input); + if (request.signal.aborted) { + reject(request.signal.reason); + return; + } request.signal.addEventListener( "abort", () => reject(request.signal.reason), @@ -97,4 +101,60 @@ describe("apiFetch", () => { }); expect(fetchMock).toHaveBeenCalledTimes(1); }); + + it("preserves caller cancellation signal", async () => { + const controller = new AbortController(); + const reason = new Error("cancelled by caller"); + const fetchMock = vi.fn( + async (input: RequestInfo | URL) => + new Promise((_resolve, reject) => { + const request = input instanceof Request ? input : new Request(input); + if (request.signal.aborted) { + reject(request.signal.reason); + return; + } + request.signal.addEventListener( + "abort", + () => reject(request.signal.reason), + { once: true }, + ); + }), + ); + + const promise = apiFetch( + new Request("https://api.argos-ci.test/builds", { + signal: controller.signal, + }), + { + fetch: fetchMock as unknown as typeof fetch, + minTimeout: 0, + }, + ); + const rejection = promise.catch((error: unknown) => error); + + controller.abort(reason); + + await expect(rejection).resolves.toBe(reason); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("does not call fetch when the caller signal is already aborted", async () => { + const controller = new AbortController(); + const reason = new Error("already cancelled"); + const fetchMock = vi.fn(async () => new Response("{}")); + + controller.abort(reason); + + await expect( + apiFetch( + new Request("https://api.argos-ci.test/builds", { + signal: controller.signal, + }), + { + fetch: fetchMock as unknown as typeof fetch, + }, + ), + ).rejects.toBe(reason); + expect(fetchMock).not.toHaveBeenCalled(); + }); }); diff --git a/packages/api-client/src/fetch.ts b/packages/api-client/src/fetch.ts index 3bb97a0c..de5a3572 100644 --- a/packages/api-client/src/fetch.ts +++ b/packages/api-client/src/fetch.ts @@ -34,11 +34,13 @@ async function createRequestFactory(request: Request, timeout: number) { redirect: request.redirect, referrer: request.referrer, referrerPolicy: request.referrerPolicy, - signal: AbortSignal.timeout(timeout), + signal: AbortSignal.any([request.signal, AbortSignal.timeout(timeout)]), }); } export async function apiFetch(input: Request, options: APIFetchOptions = {}) { + input.signal.throwIfAborted(); + const fetchImpl = options.fetch ?? fetch; const createRequest = await createRequestFactory( input, @@ -56,6 +58,7 @@ export async function apiFetch(input: Request, options: APIFetchOptions = {}) { { minTimeout: options.minTimeout, retries: options.retries ?? 3, + shouldRetry: () => !input.signal.aborted, onFailedAttempt: (context) => { debug("API request failed", context.error.message); if (context.retriesLeft > 0) { From 599ba95b5491cba596f37209fbb3b8270b8ebdfa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Wed, 20 May 2026 16:53:30 +0200 Subject: [PATCH 3/4] fix(api-client): fix p-retry missing from deps --- packages/api-client/package.json | 4 ++-- pnpm-lock.yaml | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/api-client/package.json b/packages/api-client/package.json index b8462be9..f29a7f66 100644 --- a/packages/api-client/package.json +++ b/packages/api-client/package.json @@ -47,11 +47,11 @@ "@types/debug": "^4.1.13", "@types/node": "catalog:", "openapi-typescript": "^7.13.0", - "p-retry": "^8.0.0", "vitest": "catalog:" }, "dependencies": { "debug": "^4.4.3", - "openapi-fetch": "^0.17.0" + "openapi-fetch": "^0.17.0", + "p-retry": "^8.0.0" } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c59c5a0c..644e0657 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -76,6 +76,9 @@ importers: openapi-fetch: specifier: ^0.17.0 version: 0.17.0 + p-retry: + specifier: ^8.0.0 + version: 8.0.0 devDependencies: '@types/debug': specifier: ^4.1.13 @@ -86,9 +89,6 @@ importers: openapi-typescript: specifier: ^7.13.0 version: 7.13.0(typescript@6.0.3) - p-retry: - specifier: ^8.0.0 - version: 8.0.0 vitest: specifier: 'catalog:' version: 4.1.5(@types/node@20.19.40)(@vitest/browser-playwright@4.1.5)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(msw@2.14.5(@types/node@20.19.40)(typescript@6.0.3))(vite@7.3.3(@types/node@20.19.40)(jiti@2.7.0)(tsx@4.21.0)(yaml@2.8.4)) From 0aa566fe86f0f6af3a29f7e52935039b26820556 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Wed, 20 May 2026 16:57:27 +0200 Subject: [PATCH 4/4] test: disable tokenless test --- packages/cli/e2e/upload-tokenless.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/cli/e2e/upload-tokenless.test.js b/packages/cli/e2e/upload-tokenless.test.js index 22728dd3..f069bab5 100644 --- a/packages/cli/e2e/upload-tokenless.test.js +++ b/packages/cli/e2e/upload-tokenless.test.js @@ -4,7 +4,9 @@ import { run } from "./utils.js"; // No ARGOS_TOKEN — authentication is handled via the GitHub Actions // tokenless exchange flow. -test( +// It is skipped because it only works on PR, enable it if you have to test tokenless. +// eslint-disable-next-line vitest/no-disabled-tests +test.skip( "upload returns a full build URL using tokenless authentication", { tags: ["tokenless"], timeout: 20_000 }, () => {