[format-errors] Await async handlePrettyErrorRequest in fetch handler's try/catch (cloudflare#12756)

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: Dario Piotrowicz <dario@cloudflare.com>

Author: 3 people

.changeset/fix-format-errors-missing-await.md

@@ -0,0 +1,7 @@
+---
+"@cloudflare/format-errors": patch
+---
+
+Fix error formatting to reliably return fallback responses on failure
+
+Previously, if something went wrong while formatting a pretty error page, the failure could go unhandled, resulting in no response being returned to the user. Now, errors during formatting are properly caught, ensuring users always receive a 500 JSON fallback response.

packages/format-errors/package.json

@@ -7,7 +7,8 @@
 		"check:lint": "eslint . --max-warnings=0 --cache",
 		"check:type": "tsc",
 		"deploy": "wrangler deploy",
-		"start": "wrangler dev"
+		"start": "wrangler dev",
+		"test:ci": "vitest run"
 	},
 	"devDependencies": {
 		"@cloudflare/eslint-config-shared": "workspace:*",
@@ -17,6 +18,7 @@
 		"promjs": "^0.4.2",
 		"toucan-js": "4.0.0",
 		"tsconfig": "*",
+		"vitest": "catalog:default",
 		"wrangler": "workspace:*",
 		"zod": "^3.22.3"
 	},

packages/format-errors/src/__tests__/index.test.ts

@@ -0,0 +1,107 @@
+import { describe, expect, it, vi } from "vitest";
+import type { Payload } from "../index";
+
+// Mock external dependencies that can't be resolved by Vite in test env
+vi.mock("promjs", () => {
+	const mockCounter = { inc: vi.fn() };
+	const mockRegistry = {
+		create: vi.fn(() => mockCounter),
+		metrics: vi.fn(() => ""),
+	};
+	return { default: () => mockRegistry };
+});
+
+vi.mock("toucan-js", () => {
+	return {
+		Toucan: vi.fn().mockImplementation(() => ({
+			captureException: vi.fn(),
+		})),
+	};
+});
+
+describe("handlePrettyErrorRequest", () => {
+	it("should propagate async rejections to callers", async () => {
+		// Mock Youch to throw asynchronously
+		vi.doMock("../Youch", () => {
+			return {
+				default: vi.fn().mockImplementation(() => ({
+					addLink: vi.fn(),
+					toHTML: vi.fn().mockRejectedValue(new Error("Youch async error")),
+				})),
+			};
+		});
+
+		// handlePrettyErrorRequest is async — if it's not awaited, async
+		// rejections (e.g. from Youch.toHTML()) won't be caught by a
+		// surrounding try/catch, leading to unhandled promise rejections.
+		// This test verifies that the function properly rejects so that
+		// callers who `await` it can catch the error.
+		const { handlePrettyErrorRequest } = await import("../index");
+
+		const payload: Payload = {
+			url: "https://example.com",
+			method: "GET",
+			headers: { "content-type": "text/html" },
+			error: {
+				message: "Test error",
+				name: "Error",
+				stack: "Error: Test error\n    at test:1:1",
+			},
+		};
+
+		await expect(handlePrettyErrorRequest(payload)).rejects.toThrow(
+			"Youch async error"
+		);
+	});
+});
+
+describe("reviveError", () => {
+	it("should revive a plain Error", async () => {
+		const { reviveError } = await import("../index");
+		const error = reviveError({
+			message: "test",
+			name: "Error",
+			stack: "Error: test\n    at foo:1:1",
+		});
+		expect(error).toBeInstanceOf(Error);
+		expect(error.message).toBe("test");
+		expect(error.name).toBe("Error");
+		expect(error.stack).toBe("Error: test\n    at foo:1:1");
+	});
+
+	it("should revive a TypeError", async () => {
+		const { reviveError } = await import("../index");
+		const error = reviveError({
+			message: "x is not a function",
+			name: "TypeError",
+		});
+		expect(error).toBeInstanceOf(TypeError);
+		expect(error.message).toBe("x is not a function");
+	});
+
+	it("should revive an error with a cause", async () => {
+		const { reviveError } = await import("../index");
+		const error = reviveError({
+			message: "outer",
+			name: "Error",
+			cause: {
+				message: "inner",
+				name: "RangeError",
+			},
+		});
+		expect(error).toBeInstanceOf(Error);
+		expect(error.message).toBe("outer");
+		expect(error.cause).toBeInstanceOf(RangeError);
+		expect((error.cause as Error).message).toBe("inner");
+	});
+
+	it("should fall back to Error for unknown error names", async () => {
+		const { reviveError } = await import("../index");
+		const error = reviveError({
+			message: "custom",
+			name: "CustomError",
+		});
+		expect(error).toBeInstanceOf(Error);
+		expect(error.name).toBe("CustomError");
+	});
+});

packages/format-errors/src/index.ts

@@ -167,7 +167,7 @@ export default {
 		}
 
 		try {
-			return handlePrettyErrorRequest(payload);
+			return await handlePrettyErrorRequest(payload);
 		} catch (e) {
 			sentry.captureException(e);
 			const errorCounter = registry.create(

packages/format-errors/vitest.config.mts

@@ -0,0 +1,23 @@
+import path from "node:path";
+import { fileURLToPath } from "node:url";
+import { defineProject, mergeConfig } from "vitest/config";
+import configShared from "../../vitest.shared";
+
+const __dirname = path.dirname(fileURLToPath(import.meta.url));
+
+export default mergeConfig(
+	configShared,
+	defineProject({
+		resolve: {
+			alias: {
+				// promjs has a broken package.json (main points to lib/index.js
+				// which doesn't exist in the installed package). Alias it to the
+				// actual entry point so Vite can resolve it in tests.
+				promjs: path.resolve(__dirname, "node_modules/promjs/index.js"),
+			},
+		},
+		test: {
+			include: ["src/__tests__/**/*.{test,spec}.{ts,js}"],
+		},
+	})
+);