Merge pull request #8 from minpeter/fix/webfetch-body-disposal

fix(webfetch): handle bodies without dump

Author: code-yeongyu

src/webfetch/fetcher.test.ts

@@ -0,0 +1,58 @@
+import { createServer, type Server } from "node:http";
+import { afterEach, describe, expect, it } from "vitest";
+
+import { fetchUrl } from "./fetcher.js";
+
+const servers: Server[] = [];
+
+async function createFixtureServer(): Promise<string> {
+	const server = createServer((request, response) => {
+		if (request.url === "/start") {
+			response.writeHead(302, {
+				location: "/final",
+				"content-type": "text/plain; charset=utf-8",
+			});
+			response.end("redirecting");
+			return;
+		}
+
+		response.writeHead(200, { "content-type": "text/plain; charset=utf-8" });
+		response.end("ready");
+	});
+	await new Promise<void>((resolve) => server.listen(0, "127.0.0.1", resolve));
+	const address = server.address();
+	if (typeof address !== "object" || address === null) {
+		throw new Error("Expected TCP server address");
+	}
+	servers.push(server);
+	return `http://127.0.0.1:${address.port}`;
+}
+
+afterEach(async () => {
+	await Promise.all(
+		servers.splice(0).map(
+			(server) =>
+				new Promise<void>((resolve, reject) => {
+					server.close((error) => (error ? reject(error) : resolve()));
+				}),
+		),
+	);
+});
+
+describe("fetchUrl", () => {
+	it("discards a response body without dump", async () => {
+		// given
+		const baseUrl = await createFixtureServer();
+
+		// when
+		const result = await fetchUrl({
+			url: `${baseUrl}/start`,
+			format: "text",
+			timeoutSeconds: 1,
+		});
+
+		// then
+		expect(new TextDecoder().decode(result.body)).toBe("ready");
+		expect(result.url).toBe(`${baseUrl}/final`);
+	});
+});

src/webfetch/fetcher.ts

@@ -47,7 +47,8 @@ interface HttpResponse {
 
 interface ResponseBodyStream extends AsyncIterable<unknown> {
 	destroy(error?: Error): void;
-	dump(options?: { limit: number; signal?: AbortSignal }): Promise<void>;
+	dump?(options?: { limit: number; signal?: AbortSignal }): Promise<void>;
+	once(event: "error", listener: (error: Error) => void): unknown;
 }
 
 export async function fetchUrl(options: FetchOptions): Promise<FetchResult> {
@@ -209,17 +210,28 @@ function getHeader(headers: IncomingHttpHeaders, name: string): string {
 }
 
 async function discardBody(body: ResponseBodyStream): Promise<void> {
+	if (typeof body.dump !== "function") {
+		destroyDiscardedBody(body);
+		return;
+	}
+
 	try {
 		await body.dump({ limit: 1024 });
 	} catch (error) {
 		if (error instanceof Error) {
-			body.destroy(error);
+			destroyDiscardedBody(body);
 			return;
 		}
 		throw error;
 	}
 }
 
+function destroyDiscardedBody(body: ResponseBodyStream): void {
+	// Deliberate teardown can emit an error after the body no longer has a consumer.
+	body.once("error", () => undefined);
+	body.destroy();
+}
+
 async function readResponseBody(response: HttpResponse, signal: AbortSignal): Promise<Uint8Array> {
 	const chunks: Uint8Array[] = [];
 	let total = 0;

test/webfetch-explicit-article.test.ts

@@ -1,15 +1,12 @@
-import { beforeEach, describe, expect, it, vi } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 
-const readability = vi.hoisted(() => ({
-	parse: vi.fn(() => {
-		throw new Error("Readability should not run for explicit article matches");
-	}),
-}));
+let readabilityParseCalls = 0;
 
 vi.mock("@mozilla/readability", () => ({
 	Readability: class {
 		parse(): unknown {
-			return readability.parse();
+			readabilityParseCalls += 1;
+			return null;
 		}
 	},
 }));
@@ -29,17 +26,13 @@ function explicitArticleHtml(): string {
 }
 
 describe("webfetch explicit article extraction", () => {
-	beforeEach(() => {
-		readability.parse.mockClear();
-	});
-
 	it("#given an explicit article container #when converting markdown #then skips Readability fallback parsing", () => {
 		// given / when
 		const markdown = htmlToMarkdown(explicitArticleHtml(), "https://example.test/post");
 
 		// then
 		expect(markdown).toContain("# Explicit Article");
 		expect(markdown).toContain("Explicit article body");
-		expect(readability.parse).not.toHaveBeenCalled();
+		expect(readabilityParseCalls).toBe(0);
 	});
 });

test/webfetch.test.ts

@@ -563,6 +563,33 @@ describe("webfetch", () => {
 		expect(headerValue(challengeHeaders, "sec-ch-ua-platform")).toBe('"Windows"');
 	});
 
+	it("#given one redirect #when fetching #then returns the final response body", async () => {
+		// given
+		const visitedPaths: string[] = [];
+		const server = await createFixtureServer((request, response) => {
+			visitedPaths.push(request.url ?? "");
+			if (request.url === "/start") {
+				response.writeHead(302, {
+					location: "/final",
+					"content-type": "text/plain; charset=utf-8",
+				});
+				response.end("redirecting");
+				return;
+			}
+
+			response.writeHead(200, { "content-type": "text/html; charset=utf-8" });
+			response.end("<html><body><h1>Redirect Complete</h1><p>Final page.</p></body></html>");
+		});
+
+		// when
+		const result = await executeWebfetch({ url: `${server.baseUrl}/start`, format: "markdown" });
+
+		// then
+		expect(textContent(result)).toContain("# Redirect Complete");
+		expect(textContent(result)).toContain("Final page.");
+		expect(visitedPaths).toEqual(["/start", "/final"]);
+	});
+
 	it("#given too many redirects #when fetching #then returns the final redirect response body", async () => {
 		// given
 		const server = await createFixtureServer((_request, response) => {