Skip to content

Commit 701ae4e

Browse files
calvin-codecovcalvin-codecov
authored
feat: Add more observability to getPreSignedUrl errors and prevent retries f… (#335)
1 parent 78a72cb commit 701ae4e

8 files changed

Lines changed: 188 additions & 46 deletions

File tree

‎integration-tests/fixtures/generate-bundle-stats/astro/astro-base.config.mjs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ export default defineConfig({
1616
rollupOptions: {
1717
output: {
1818
format: "esm",
19-
}
20-
}
19+
},
20+
},
2121
},
2222
integrations: [
2323
react(),

‎integration-tests/test-api/tsconfig.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,6 @@
22
"compilerOptions": {
33
"strict": true,
44
"jsx": "react-jsx",
5-
"jsxImportSource": "hono/jsx"
6-
}
5+
"jsxImportSource": "hono/jsx",
6+
},
77
}

‎packages/bundler-plugin-core/src/errors/AuthenticationError.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
export class AuthenticationError extends Error {
2+
constructor(msg: string, options?: ErrorOptions) {
3+
super(msg, options);
4+
}
5+
}

‎packages/bundler-plugin-core/src/utils/Output.ts‎

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { type Client, type Scope, startSpan } from "@sentry/core";
1+
import { type Client, type Scope, startSpan, type Span } from "@sentry/core";
22
import {
33
type Asset,
44
type Chunk,
@@ -8,6 +8,7 @@ import {
88
type ProviderUtilInputs,
99
type UploadOverrides,
1010
} from "../types.ts";
11+
import { AuthenticationError } from "../errors/AuthenticationError.ts";
1112
import { getPreSignedURL } from "./getPreSignedURL.ts";
1213
import { type NormalizedOptions } from "./normalizeOptions.ts";
1314
import { detectProvider } from "./provider.ts";
@@ -249,7 +250,7 @@ class Output {
249250
scope: this.sentryScope,
250251
parentSpan: outputWriteSpan,
251252
},
252-
async (getPreSignedURLSpan) => {
253+
async (getPreSignedURLSpan: Span | undefined) => {
253254
let url = "";
254255
try {
255256
url = await getPreSignedURL({
@@ -264,19 +265,23 @@ class Output {
264265
});
265266
} catch (error) {
266267
if (this.sentryClient && this.sentryScope) {
267-
this.sentryScope.addBreadcrumb({
268-
category: "output.write.getPreSignedURL",
269-
level: "error",
270-
data: { error },
271-
});
272-
// only setting this as info because this could be caused by user error
273-
this.sentryClient.captureMessage(
274-
"Error in getPreSignedURL",
275-
"info",
276-
undefined,
277-
this.sentryScope,
278-
);
279-
await safeFlushTelemetry(this.sentryClient);
268+
// if the user gets an authentication error, something is wrong with their setup
269+
// so we don't want to log it to sentry
270+
if (!(error instanceof AuthenticationError)) {
271+
this.sentryScope.addBreadcrumb({
272+
category: "output.write.getPreSignedURL",
273+
level: "error",
274+
data: { error },
275+
});
276+
// only setting this as info because this could be caused by user error
277+
this.sentryClient.captureMessage(
278+
"Error in getPreSignedURL",
279+
"info",
280+
undefined,
281+
this.sentryScope,
282+
);
283+
await safeFlushTelemetry(this.sentryClient);
284+
}
280285
}
281286

282287
if (emitError) {

‎packages/bundler-plugin-core/src/utils/__tests__/fetchWithRetry.test.ts‎

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ interface SetupArgs {
3535
retryCount?: number;
3636
sendError?: boolean;
3737
failFetch?: boolean;
38+
retryFailureStatus?: number;
3839
}
3940

4041
describe("fetchWithRetry", () => {
@@ -50,6 +51,7 @@ describe("fetchWithRetry", () => {
5051
sendError = false,
5152
failFetch = false,
5253
retryCount = 0,
54+
retryFailureStatus = 503,
5355
}: SetupArgs) {
5456
consoleSpy = vi.spyOn(console, "log").mockImplementation(() => null);
5557

@@ -64,7 +66,9 @@ describe("fetchWithRetry", () => {
6466
}
6567

6668
retryCount -= 1;
67-
return new HttpResponse("not found", { status: 404 });
69+
return new HttpResponse("service unavailable", {
70+
status: retryFailureStatus,
71+
});
6872
}),
6973
);
7074
}
@@ -92,6 +96,7 @@ describe("fetchWithRetry", () => {
9296
setup({
9397
data: { url: "http://example.com" },
9498
retryCount: 2,
99+
retryFailureStatus: 503,
95100
});
96101

97102
const urlPromise = await fetchWithRetry({
@@ -105,12 +110,43 @@ describe("fetchWithRetry", () => {
105110
});
106111
});
107112

113+
describe("when the initial response is a 4xx error", () => {
114+
it("returns the response without retrying", async () => {
115+
let requestCount = 0;
116+
consoleSpy = vi.spyOn(console, "log").mockImplementation(() => null);
117+
118+
server.use(
119+
http.all("http://localhost", () => {
120+
requestCount += 1;
121+
return HttpResponse.json(
122+
{ message: "Bad Request", detail: "example error body" },
123+
{ status: 400 },
124+
);
125+
}),
126+
);
127+
128+
const response = await fetchWithRetry({
129+
url: "http://localhost",
130+
requestData: {},
131+
retryCount: 5,
132+
});
133+
134+
expect(requestCount).toBe(1);
135+
expect(response.status).toBe(400);
136+
await expect(response.json()).resolves.toEqual({
137+
message: "Bad Request",
138+
detail: "example error body",
139+
});
140+
});
141+
});
142+
108143
describe("retry count exceeds limit", () => {
109144
it("returns the response", async () => {
110145
setup({
111146
data: { url: "http://example.com" },
112147
retryCount: 2,
113148
failFetch: true,
149+
retryFailureStatus: 503,
114150
});
115151

116152
const response = await fetchWithRetry({
@@ -119,7 +155,7 @@ describe("fetchWithRetry", () => {
119155
retryCount: 1,
120156
});
121157

122-
expect(response.status).toBe(404);
158+
expect(response.status).toBe(503);
123159
});
124160
});
125161

‎packages/bundler-plugin-core/src/utils/__tests__/getPreSignedURL.test.ts‎

Lines changed: 75 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -302,30 +302,85 @@ describe("getPreSignedURL", () => {
302302
});
303303

304304
describe('http response is not "ok"', () => {
305-
it("throws an error", async () => {
306-
const { consoleSpy } = setup({
307-
data: { url: "http://example.com" },
308-
status: 400,
305+
describe("4xx error", () => {
306+
it("throws an error", async () => {
307+
let requestCount = 0;
308+
const consoleSpy = vi
309+
.spyOn(console, "log")
310+
.mockImplementation(() => null);
311+
312+
server.use(
313+
http.post(
314+
"http://localhost/upload/bundle_analysis/v1",
315+
async ({ request }) => {
316+
requestCount += 1;
317+
const requestBody = await request.json();
318+
requestBodyMock(requestBody);
319+
320+
if (request.headers.get("Authorization")) {
321+
requestTokenMock(request.headers.get("Authorization"));
322+
}
323+
324+
return HttpResponse.json(
325+
{ detail: "Bad Request" },
326+
{ status: 400 },
327+
);
328+
},
329+
),
330+
);
331+
332+
let error;
333+
try {
334+
await getPreSignedURL({
335+
apiUrl: "http://localhost",
336+
uploadToken: "cool-upload-token",
337+
retryCount: 3,
338+
serviceParams: {
339+
commit: "123",
340+
},
341+
});
342+
} catch (e) {
343+
error = e;
344+
}
345+
346+
expect(requestCount).toBe(1);
347+
expect(error).toBeInstanceOf(FailedFetchError);
348+
expect(consoleSpy).toHaveBeenCalledWith(
349+
`[codecov] ${Chalk.red(
350+
"Failed to get pre-signed URL, bad response: Bad Request",
351+
)}`,
352+
);
309353
});
354+
});
310355

311-
let error;
312-
try {
313-
await getPreSignedURL({
314-
apiUrl: "http://localhost",
315-
uploadToken: "cool-upload-token",
316-
retryCount: 0,
317-
serviceParams: {
318-
commit: "123",
319-
},
356+
describe("non-4xx error", () => {
357+
it("throws an error", async () => {
358+
const { consoleSpy } = setup({
359+
data: { url: "http://example.com" },
360+
status: 503,
320361
});
321-
} catch (e) {
322-
error = e;
323-
}
324362

325-
expect(error).toBeInstanceOf(FailedFetchError);
326-
expect(consoleSpy).toHaveBeenCalledWith(
327-
`[codecov] ${Chalk.red('Failed to get pre-signed URL, bad response: "400 - Bad Request"')}`,
328-
);
363+
let error;
364+
try {
365+
await getPreSignedURL({
366+
apiUrl: "http://localhost",
367+
uploadToken: "cool-upload-token",
368+
retryCount: 0,
369+
serviceParams: {
370+
commit: "123",
371+
},
372+
});
373+
} catch (e) {
374+
error = e;
375+
}
376+
377+
expect(error).toBeInstanceOf(FailedFetchError);
378+
expect(consoleSpy).toHaveBeenCalledWith(
379+
`[codecov] ${Chalk.red(
380+
"Failed to get pre-signed URL (server), bad response: 503 - Service Unavailable",
381+
)}`,
382+
);
383+
});
329384
});
330385
});
331386

‎packages/bundler-plugin-core/src/utils/fetchWithRetry.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,13 @@ export const fetchWithRetry = async ({
2626
response = await fetch(url, requestData);
2727

2828
if (!response.ok) {
29+
// 4xx errors are permanent client errors - no need to retry
30+
if (response.status >= 400 && response.status < 500) {
31+
debug(
32+
`${name} received ${response.status} client error, not retrying`,
33+
);
34+
return response;
35+
}
2936
throw new BadResponseError("Response not ok");
3037
}
3138
break;

‎packages/bundler-plugin-core/src/utils/getPreSignedURL.ts‎

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
type Span,
88
} from "@sentry/core";
99
import { z } from "zod";
10+
import { AuthenticationError } from "../errors/AuthenticationError.ts";
1011
import { FailedFetchError } from "../errors/FailedFetchError.ts";
1112
import { UploadLimitReachedError } from "../errors/UploadLimitReachedError.ts";
1213
import { type ProviderServiceParams } from "../types.ts";
@@ -39,6 +40,10 @@ const PreSignedURLSchema = z.object({
3940
url: z.string(),
4041
});
4142

43+
const ApiErrorBodySchema = z.object({
44+
detail: z.string().optional(),
45+
});
46+
4247
const API_ENDPOINT = "/upload/bundle_analysis/v1";
4348

4449
export const getPreSignedURL = async ({
@@ -119,7 +124,7 @@ export const getPreSignedURL = async ({
119124
scope: sentryScope,
120125
parentSpan: sentrySpan,
121126
},
122-
async (getPreSignedURLSpan) => {
127+
async (getPreSignedURLSpan: Span | undefined) => {
123128
let wrappedResponse: Response;
124129
const HTTP_METHOD = "POST";
125130
const URL = `${apiUrl}${API_ENDPOINT}`;
@@ -182,11 +187,40 @@ export const getPreSignedURL = async ({
182187
throw new UploadLimitReachedError("Upload limit reached");
183188
}
184189

185-
if (!response.ok) {
186-
red(
187-
`Failed to get pre-signed URL, bad response: "${response.status} - ${response.statusText}"`,
190+
if (response.status >= 400 && response.status < 500) {
191+
// Attempt to parse a server-provided error message to give users actionable feedback
192+
let serverMessage = "";
193+
try {
194+
const raw: unknown = await response.clone().json();
195+
const parsed = ApiErrorBodySchema.safeParse(raw);
196+
serverMessage = parsed.success ? parsed.data.detail ?? "" : "";
197+
} catch {
198+
// ignore parse failures
199+
}
200+
const responseStatusWithText = `${response.status} - ${response.statusText}`;
201+
const msgDetail = serverMessage || responseStatusWithText;
202+
red(`Failed to get pre-signed URL, bad response: ${msgDetail}`);
203+
if (
204+
serverMessage.toLowerCase().includes("token") ||
205+
response.status === 401 ||
206+
response.status === 403
207+
) {
208+
red(
209+
"Set uploadToken in your Codecov plugin config or enable tokenless uploads for your public repository.",
210+
);
211+
throw new AuthenticationError(
212+
serverMessage || `Authentication failed: ${responseStatusWithText}`,
213+
);
214+
}
215+
throw new FailedFetchError(
216+
`Failed to get pre-signed URL (client), bad response: ${responseStatusWithText}`,
188217
);
189-
throw new FailedFetchError("Failed to get pre-signed URL");
218+
}
219+
220+
if (!response.ok) {
221+
const msg = `Failed to get pre-signed URL (server), bad response: ${response.status} - ${response.statusText}`;
222+
red(msg);
223+
throw new FailedFetchError(msg);
190224
}
191225

192226
let data;

0 commit comments

Comments
 (0)