From d6826df6a54ea22306321efb57b4071e5059c48a Mon Sep 17 00:00:00 2001 From: Calvin Yau Date: Wed, 6 May 2026 14:38:03 -0700 Subject: [PATCH 1/3] feat: Add more observability to getPreSignedUrl and prevent retries for client auth errors --- .../src/errors/AuthenticationError.ts | 5 +++ .../bundler-plugin-core/src/utils/Output.ts | 35 ++++++++++------- .../src/utils/fetchWithRetry.ts | 5 +++ .../src/utils/getPreSignedURL.ts | 39 +++++++++++++++++-- 4 files changed, 65 insertions(+), 19 deletions(-) create mode 100644 packages/bundler-plugin-core/src/errors/AuthenticationError.ts diff --git a/packages/bundler-plugin-core/src/errors/AuthenticationError.ts b/packages/bundler-plugin-core/src/errors/AuthenticationError.ts new file mode 100644 index 00000000..0f9b8a2c --- /dev/null +++ b/packages/bundler-plugin-core/src/errors/AuthenticationError.ts @@ -0,0 +1,5 @@ +export class AuthenticationError extends Error { + constructor(msg: string, options?: ErrorOptions) { + super(msg, options); + } +} diff --git a/packages/bundler-plugin-core/src/utils/Output.ts b/packages/bundler-plugin-core/src/utils/Output.ts index 0bc53d78..e8bce793 100644 --- a/packages/bundler-plugin-core/src/utils/Output.ts +++ b/packages/bundler-plugin-core/src/utils/Output.ts @@ -1,4 +1,4 @@ -import { type Client, type Scope, startSpan } from "@sentry/core"; +import { type Client, type Scope, startSpan, type Span } from "@sentry/core"; import { type Asset, type Chunk, @@ -8,6 +8,7 @@ import { type ProviderUtilInputs, type UploadOverrides, } from "../types.ts"; +import { AuthenticationError } from "../errors/AuthenticationError.ts"; import { getPreSignedURL } from "./getPreSignedURL.ts"; import { type NormalizedOptions } from "./normalizeOptions.ts"; import { detectProvider } from "./provider.ts"; @@ -249,7 +250,7 @@ class Output { scope: this.sentryScope, parentSpan: outputWriteSpan, }, - async (getPreSignedURLSpan) => { + async (getPreSignedURLSpan: Span | undefined) => { let url = ""; try { url = await getPreSignedURL({ @@ -264,19 +265,23 @@ class Output { }); } catch (error) { if (this.sentryClient && this.sentryScope) { - this.sentryScope.addBreadcrumb({ - category: "output.write.getPreSignedURL", - level: "error", - data: { error }, - }); - // only setting this as info because this could be caused by user error - this.sentryClient.captureMessage( - "Error in getPreSignedURL", - "info", - undefined, - this.sentryScope, - ); - await safeFlushTelemetry(this.sentryClient); + // if the user gets an authentication error, something is wrong with their setup + // so we don't want to log it to sentry + if (!(error instanceof AuthenticationError)) { + this.sentryScope.addBreadcrumb({ + category: "output.write.getPreSignedURL", + level: "error", + data: { error }, + }); + // only setting this as info because this could be caused by user error + this.sentryClient.captureMessage( + "Error in getPreSignedURL", + "info", + undefined, + this.sentryScope, + ); + await safeFlushTelemetry(this.sentryClient); + } } if (emitError) { diff --git a/packages/bundler-plugin-core/src/utils/fetchWithRetry.ts b/packages/bundler-plugin-core/src/utils/fetchWithRetry.ts index 6ae81f19..8e2cf686 100644 --- a/packages/bundler-plugin-core/src/utils/fetchWithRetry.ts +++ b/packages/bundler-plugin-core/src/utils/fetchWithRetry.ts @@ -26,6 +26,11 @@ export const fetchWithRetry = async ({ response = await fetch(url, requestData); if (!response.ok) { + // 4xx errors are permanent client errors - no need to retry + if (response.status >= 400 && response.status < 500) { + debug(`${name} received ${response.status} client error, not retrying`); + return response; + } throw new BadResponseError("Response not ok"); } break; diff --git a/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts b/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts index 4c74228e..0fbd8325 100644 --- a/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts +++ b/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts @@ -7,6 +7,7 @@ import { type Span, } from "@sentry/core"; import { z } from "zod"; +import { AuthenticationError } from "../errors/AuthenticationError.ts"; import { FailedFetchError } from "../errors/FailedFetchError.ts"; import { UploadLimitReachedError } from "../errors/UploadLimitReachedError.ts"; import { type ProviderServiceParams } from "../types.ts"; @@ -119,7 +120,7 @@ export const getPreSignedURL = async ({ scope: sentryScope, parentSpan: sentrySpan, }, - async (getPreSignedURLSpan) => { + async (getPreSignedURLSpan: Span | undefined) => { let wrappedResponse: Response; const HTTP_METHOD = "POST"; const URL = `${apiUrl}${API_ENDPOINT}`; @@ -182,11 +183,41 @@ export const getPreSignedURL = async ({ throw new UploadLimitReachedError("Upload limit reached"); } - if (!response.ok) { + if (response.status >= 400 && response.status < 500) { + // Attempt to parse a server-provided error message to give users actionable feedback + let serverMessage = ""; + try { + const errorBody = await response.clone().json(); + serverMessage = errorBody?.message ?? ""; + } catch { + // ignore parse failures + } + const responseStatusWithText = `${response.status} - ${response.statusText}`; + const msgDetail = serverMessage ?? responseStatusWithText; red( - `Failed to get pre-signed URL, bad response: "${response.status} - ${response.statusText}"`, + `Failed to get presigned URL, bad response: ${msgDetail}`, ); - throw new FailedFetchError("Failed to get pre-signed URL"); + if ( + serverMessage.toLowerCase().includes("token") || + response.status === 401 || + response.status === 403 + ) { + red( + "Set uploadToken in your Codecov plugin config or enable tokenless uploads for your public repository.", + ); + throw new AuthenticationError( + serverMessage || `Authentication failed: ${responseStatusWithText}`, + ); + } + throw new FailedFetchError( + `Failed to get presigned URL (client), bad response: ${responseStatusWithText}`, + ); + } + + if (!response.ok) { + const msg = `Failed to get presigned URL (server), bad response: ${response.status} - ${response.statusText}`; + red(msg); + throw new FailedFetchError(msg); } let data; From 466d8501ac607130348779fa80c3ad378cff0668 Mon Sep 17 00:00:00 2001 From: Calvin Yau Date: Wed, 6 May 2026 16:37:45 -0700 Subject: [PATCH 2/3] chore: Run prettier --- .../generate-bundle-stats/astro/astro-base.config.mjs | 4 ++-- integration-tests/test-api/tsconfig.json | 4 ++-- packages/bundler-plugin-core/src/utils/fetchWithRetry.ts | 4 +++- packages/bundler-plugin-core/src/utils/getPreSignedURL.ts | 6 ++---- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/integration-tests/fixtures/generate-bundle-stats/astro/astro-base.config.mjs b/integration-tests/fixtures/generate-bundle-stats/astro/astro-base.config.mjs index d9e32b23..39164f25 100644 --- a/integration-tests/fixtures/generate-bundle-stats/astro/astro-base.config.mjs +++ b/integration-tests/fixtures/generate-bundle-stats/astro/astro-base.config.mjs @@ -16,8 +16,8 @@ export default defineConfig({ rollupOptions: { output: { format: "esm", - } - } + }, + }, }, integrations: [ react(), diff --git a/integration-tests/test-api/tsconfig.json b/integration-tests/test-api/tsconfig.json index 9136c143..c829ef88 100644 --- a/integration-tests/test-api/tsconfig.json +++ b/integration-tests/test-api/tsconfig.json @@ -2,6 +2,6 @@ "compilerOptions": { "strict": true, "jsx": "react-jsx", - "jsxImportSource": "hono/jsx" - } + "jsxImportSource": "hono/jsx", + }, } diff --git a/packages/bundler-plugin-core/src/utils/fetchWithRetry.ts b/packages/bundler-plugin-core/src/utils/fetchWithRetry.ts index 8e2cf686..1c874d49 100644 --- a/packages/bundler-plugin-core/src/utils/fetchWithRetry.ts +++ b/packages/bundler-plugin-core/src/utils/fetchWithRetry.ts @@ -28,7 +28,9 @@ export const fetchWithRetry = async ({ if (!response.ok) { // 4xx errors are permanent client errors - no need to retry if (response.status >= 400 && response.status < 500) { - debug(`${name} received ${response.status} client error, not retrying`); + debug( + `${name} received ${response.status} client error, not retrying`, + ); return response; } throw new BadResponseError("Response not ok"); diff --git a/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts b/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts index 0fbd8325..9768ae5f 100644 --- a/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts +++ b/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts @@ -194,9 +194,7 @@ export const getPreSignedURL = async ({ } const responseStatusWithText = `${response.status} - ${response.statusText}`; const msgDetail = serverMessage ?? responseStatusWithText; - red( - `Failed to get presigned URL, bad response: ${msgDetail}`, - ); + red(`Failed to get presigned URL, bad response: ${msgDetail}`); if ( serverMessage.toLowerCase().includes("token") || response.status === 401 || @@ -213,7 +211,7 @@ export const getPreSignedURL = async ({ `Failed to get presigned URL (client), bad response: ${responseStatusWithText}`, ); } - + if (!response.ok) { const msg = `Failed to get presigned URL (server), bad response: ${response.status} - ${response.statusText}`; red(msg); From 0e9bc91bbe0efe5e9b24c827878ed6a17633192d Mon Sep 17 00:00:00 2001 From: Calvin Yau Date: Thu, 7 May 2026 01:14:35 -0700 Subject: [PATCH 3/3] feat: Use detail instead of message and fix tests --- .../utils/__tests__/fetchWithRetry.test.ts | 40 +++++++- .../utils/__tests__/getPreSignedURL.test.ts | 95 +++++++++++++++---- .../src/utils/getPreSignedURL.ts | 17 ++-- 3 files changed, 124 insertions(+), 28 deletions(-) diff --git a/packages/bundler-plugin-core/src/utils/__tests__/fetchWithRetry.test.ts b/packages/bundler-plugin-core/src/utils/__tests__/fetchWithRetry.test.ts index ef1f28a9..72da5b02 100644 --- a/packages/bundler-plugin-core/src/utils/__tests__/fetchWithRetry.test.ts +++ b/packages/bundler-plugin-core/src/utils/__tests__/fetchWithRetry.test.ts @@ -35,6 +35,7 @@ interface SetupArgs { retryCount?: number; sendError?: boolean; failFetch?: boolean; + retryFailureStatus?: number; } describe("fetchWithRetry", () => { @@ -50,6 +51,7 @@ describe("fetchWithRetry", () => { sendError = false, failFetch = false, retryCount = 0, + retryFailureStatus = 503, }: SetupArgs) { consoleSpy = vi.spyOn(console, "log").mockImplementation(() => null); @@ -64,7 +66,9 @@ describe("fetchWithRetry", () => { } retryCount -= 1; - return new HttpResponse("not found", { status: 404 }); + return new HttpResponse("service unavailable", { + status: retryFailureStatus, + }); }), ); } @@ -92,6 +96,7 @@ describe("fetchWithRetry", () => { setup({ data: { url: "http://example.com" }, retryCount: 2, + retryFailureStatus: 503, }); const urlPromise = await fetchWithRetry({ @@ -105,12 +110,43 @@ describe("fetchWithRetry", () => { }); }); + describe("when the initial response is a 4xx error", () => { + it("returns the response without retrying", async () => { + let requestCount = 0; + consoleSpy = vi.spyOn(console, "log").mockImplementation(() => null); + + server.use( + http.all("http://localhost", () => { + requestCount += 1; + return HttpResponse.json( + { message: "Bad Request", detail: "example error body" }, + { status: 400 }, + ); + }), + ); + + const response = await fetchWithRetry({ + url: "http://localhost", + requestData: {}, + retryCount: 5, + }); + + expect(requestCount).toBe(1); + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + message: "Bad Request", + detail: "example error body", + }); + }); + }); + describe("retry count exceeds limit", () => { it("returns the response", async () => { setup({ data: { url: "http://example.com" }, retryCount: 2, failFetch: true, + retryFailureStatus: 503, }); const response = await fetchWithRetry({ @@ -119,7 +155,7 @@ describe("fetchWithRetry", () => { retryCount: 1, }); - expect(response.status).toBe(404); + expect(response.status).toBe(503); }); }); diff --git a/packages/bundler-plugin-core/src/utils/__tests__/getPreSignedURL.test.ts b/packages/bundler-plugin-core/src/utils/__tests__/getPreSignedURL.test.ts index 5fcc49f1..cc272dfb 100644 --- a/packages/bundler-plugin-core/src/utils/__tests__/getPreSignedURL.test.ts +++ b/packages/bundler-plugin-core/src/utils/__tests__/getPreSignedURL.test.ts @@ -302,30 +302,85 @@ describe("getPreSignedURL", () => { }); describe('http response is not "ok"', () => { - it("throws an error", async () => { - const { consoleSpy } = setup({ - data: { url: "http://example.com" }, - status: 400, + describe("4xx error", () => { + it("throws an error", async () => { + let requestCount = 0; + const consoleSpy = vi + .spyOn(console, "log") + .mockImplementation(() => null); + + server.use( + http.post( + "http://localhost/upload/bundle_analysis/v1", + async ({ request }) => { + requestCount += 1; + const requestBody = await request.json(); + requestBodyMock(requestBody); + + if (request.headers.get("Authorization")) { + requestTokenMock(request.headers.get("Authorization")); + } + + return HttpResponse.json( + { detail: "Bad Request" }, + { status: 400 }, + ); + }, + ), + ); + + let error; + try { + await getPreSignedURL({ + apiUrl: "http://localhost", + uploadToken: "cool-upload-token", + retryCount: 3, + serviceParams: { + commit: "123", + }, + }); + } catch (e) { + error = e; + } + + expect(requestCount).toBe(1); + expect(error).toBeInstanceOf(FailedFetchError); + expect(consoleSpy).toHaveBeenCalledWith( + `[codecov] ${Chalk.red( + "Failed to get pre-signed URL, bad response: Bad Request", + )}`, + ); }); + }); - let error; - try { - await getPreSignedURL({ - apiUrl: "http://localhost", - uploadToken: "cool-upload-token", - retryCount: 0, - serviceParams: { - commit: "123", - }, + describe("non-4xx error", () => { + it("throws an error", async () => { + const { consoleSpy } = setup({ + data: { url: "http://example.com" }, + status: 503, }); - } catch (e) { - error = e; - } - expect(error).toBeInstanceOf(FailedFetchError); - expect(consoleSpy).toHaveBeenCalledWith( - `[codecov] ${Chalk.red('Failed to get pre-signed URL, bad response: "400 - Bad Request"')}`, - ); + let error; + try { + await getPreSignedURL({ + apiUrl: "http://localhost", + uploadToken: "cool-upload-token", + retryCount: 0, + serviceParams: { + commit: "123", + }, + }); + } catch (e) { + error = e; + } + + expect(error).toBeInstanceOf(FailedFetchError); + expect(consoleSpy).toHaveBeenCalledWith( + `[codecov] ${Chalk.red( + "Failed to get pre-signed URL (server), bad response: 503 - Service Unavailable", + )}`, + ); + }); }); }); diff --git a/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts b/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts index 9768ae5f..45b3e731 100644 --- a/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts +++ b/packages/bundler-plugin-core/src/utils/getPreSignedURL.ts @@ -40,6 +40,10 @@ const PreSignedURLSchema = z.object({ url: z.string(), }); +const ApiErrorBodySchema = z.object({ + detail: z.string().optional(), +}); + const API_ENDPOINT = "/upload/bundle_analysis/v1"; export const getPreSignedURL = async ({ @@ -187,14 +191,15 @@ export const getPreSignedURL = async ({ // Attempt to parse a server-provided error message to give users actionable feedback let serverMessage = ""; try { - const errorBody = await response.clone().json(); - serverMessage = errorBody?.message ?? ""; + const raw: unknown = await response.clone().json(); + const parsed = ApiErrorBodySchema.safeParse(raw); + serverMessage = parsed.success ? parsed.data.detail ?? "" : ""; } catch { // ignore parse failures } const responseStatusWithText = `${response.status} - ${response.statusText}`; - const msgDetail = serverMessage ?? responseStatusWithText; - red(`Failed to get presigned URL, bad response: ${msgDetail}`); + const msgDetail = serverMessage || responseStatusWithText; + red(`Failed to get pre-signed URL, bad response: ${msgDetail}`); if ( serverMessage.toLowerCase().includes("token") || response.status === 401 || @@ -208,12 +213,12 @@ export const getPreSignedURL = async ({ ); } throw new FailedFetchError( - `Failed to get presigned URL (client), bad response: ${responseStatusWithText}`, + `Failed to get pre-signed URL (client), bad response: ${responseStatusWithText}`, ); } if (!response.ok) { - const msg = `Failed to get presigned URL (server), bad response: ${response.status} - ${response.statusText}`; + const msg = `Failed to get pre-signed URL (server), bad response: ${response.status} - ${response.statusText}`; red(msg); throw new FailedFetchError(msg); }