diff --git a/src/adapters/_node/send.ts b/src/adapters/_node/send.ts index 36445c55..68773145 100644 --- a/src/adapters/_node/send.ts +++ b/src/adapters/_node/send.ts @@ -56,8 +56,45 @@ function handleSendError(nodeRes: NodeServerResponse, error: unknown, silent?: b if (!silent) { console.error("[srvx] Failed to send response:", error); } + failResponse(nodeRes); +} + +/** + * Answers an error that escaped the fetch handler with a bare 500. + * + * Bun and Deno both back their handler with a runtime-level catch that answers + * 500 and keeps serving; node:http has no equivalent, so an escaping error + * becomes a process-level `uncaughtException`/`unhandledRejection` (fatal for + * an unguarded process) and leaves the client socket hanging until it times + * out. Catching here keeps the default path consistent across runtimes. + * + * Mostly reached when no `error` option is set, since `errorPlugin` otherwise + * handles the error as middleware first — but it also backstops an `error` + * handler that throws itself. + * + * @internal + */ +export function sendErrorResponse( + nodeRes: NodeServerResponse, + error: unknown, + silent?: boolean, +): void { + // Mirrors the Bun/Deno default of logging the cause server-side; the client + // response stays detail-free. + if (!silent) { + console.error("[srvx] Unhandled error in fetch handler:", error); + } + failResponse(nodeRes); +} + +function failResponse(nodeRes: NodeServerResponse): void { + if (nodeRes.writableEnded) { + // Response already complete (e.g. the handler wrote directly to + // `req.runtime.node.res` and then failed) — nothing left to answer with. + return; + } if (nodeRes.headersSent) { - // Response already committed — the only recovery is to tear down the socket. + // Status line already committed — the only recovery is to tear down the socket. nodeRes.destroy(); } else { nodeRes.statusCode = 500; diff --git a/src/adapters/node.ts b/src/adapters/node.ts index 5fc980c2..99de7337 100644 --- a/src/adapters/node.ts +++ b/src/adapters/node.ts @@ -1,4 +1,4 @@ -import { sendNodeResponseDetached } from "./_node/send.ts"; +import { sendErrorResponse, sendNodeResponseDetached } from "./_node/send.ts"; import { NodeRequest } from "./_node/request.ts"; import { fmtURL, @@ -82,12 +82,22 @@ class NodeServer implements Server { trustProxy: this.options.trustProxy, }); request.waitUntil = this.#wait?.waitUntil; - const res = fetchHandler(request); + let res: Response | Promise; + try { + res = fetchHandler(request); + } catch (error) { + // Sync throw with no `error` option: answer 500 instead of letting it + // escape as an `uncaughtException` (see sendErrorResponse). + return sendErrorResponse(nodeRes, error, this.options.silent); + } // node:http ignores the listener's return value — use the detached // variant to skip the per-response end-tracking Promise. return res instanceof Promise - ? res.then((resolvedRes) => - sendNodeResponseDetached(nodeRes, resolvedRes, this.options.silent), + ? res.then( + (resolvedRes) => sendNodeResponseDetached(nodeRes, resolvedRes, this.options.silent), + // Rejection handler (not `.catch`) so send failures, which + // `sendNodeResponseDetached` already answers, aren't handled twice. + (error) => sendErrorResponse(nodeRes, error, this.options.silent), ) : sendNodeResponseDetached(nodeRes, res, this.options.silent); }; diff --git a/test/_error-tests.ts b/test/_error-tests.ts index d321b43e..9fe10d2d 100644 --- a/test/_error-tests.ts +++ b/test/_error-tests.ts @@ -10,10 +10,10 @@ const testDir = fileURLToPath(new URL(".", import.meta.url)); * option) under the given runtime and assert that an unhandled handler throw * does not take the process down and the client still gets a response. * - * Deno and Bun both surface an uncaught handler error as `500` and keep - * serving. Node's in-process behavior differs (the throw escapes as an - * `uncaughtException` that would crash the process) and is documented - * separately in `node-error-paths.test.ts`. + * All three runtimes answer an uncaught handler error with a `500` and keep + * serving -- Deno and Bun via their runtime-level catch, Node via the adapter + * (#244). Only a spawned process can prove the "does not crash" half; the + * in-process assertions for Node live in `node-error-paths.test.ts`. */ export function addExecUnhandledThrowTests(cmd: string): void { let childProc: ExecaRes; diff --git a/test/node-error-paths.test.ts b/test/node-error-paths.test.ts index 6cd47b5d..f198bded 100644 --- a/test/node-error-paths.test.ts +++ b/test/node-error-paths.test.ts @@ -1,18 +1,16 @@ import { afterEach, describe, expect, test } from "vitest"; import { serve } from "../src/adapters/node.ts"; +import { addExecUnhandledThrowTests } from "./_error-tests.ts"; -// F9 (error paths): with no `error` option the `errorPlugin` no-ops and handler -// exceptions propagate to the runtime. This documents the *actual* Node -// behavior (see `test/_error-tests.ts` for the Deno/Bun counterparts, which -// answer 500 and stay alive). +// F9 (error paths): with no `error` option the `errorPlugin` no-ops, so the +// adapter itself is the last line of defense for a handler that fails. It +// answers a bare 500 and keeps serving, matching the Bun/Deno runtimes (see +// `test/_error-tests.ts` for those counterparts). // -// Node currently lets an unhandled throw escape the request handler as a -// process-level `uncaughtException`/`unhandledRejection` and sends no response -// on that connection -- i.e. an unguarded server would crash. We install a -// temporary handler to capture it (preventing the vitest worker from dying) and -// assert the observed behavior + that the server keeps serving afterwards. -// Fixing the divergence (making Node answer 500 like Deno/Bun) belongs to the -// Node adapter scope, not this edge-adapter batch. +// Before #244 the failure escaped the request listener as a process-level +// `uncaughtException`/`unhandledRejection` -- fatal for an unguarded process -- +// and left the client socket hanging until it timed out. Each test captures +// process-level errors to assert none escape. describe("node adapter unhandled errors (F9)", () => { let restore: (() => void) | undefined; @@ -21,11 +19,22 @@ describe("node adapter unhandled errors (F9)", () => { restore = undefined; }); + /** Captures (and swallows) process-level errors raised during the test. */ + function captureProcessErrors(event: "uncaughtException" | "unhandledRejection"): unknown[] { + const captured: unknown[] = []; + const onError = (error: unknown) => captured.push(error); + process.prependListener(event, onError); + restore = () => process.removeListener(event, onError); + return captured; + } + async function withServer( handler: (req: Request) => Response | Promise, fn: (url: string) => Promise, ) { - const server = serve({ hostname: "localhost", port: 0, fetch: handler }); + // `silent` also gates the adapter's error log: these throws are intentional + // and would only add noise to the vitest output. + const server = serve({ hostname: "localhost", port: 0, silent: true, fetch: handler }); await server.ready(); try { await fn(server.url!); @@ -34,39 +43,52 @@ describe("node adapter unhandled errors (F9)", () => { } } - test("sync throw escapes as uncaughtException and does not silently succeed", async () => { - const captured: unknown[] = []; - const onUncaught = (err: unknown) => captured.push(err); - process.prependListener("uncaughtException", onUncaught); - restore = () => process.removeListener("uncaughtException", onUncaught); - - await withServer( - (req) => { - if (new URL(req.url).pathname === "/throw") { - throw new Error("unhandled sync error"); - } - return new Response("ok"); + for (const [name, event, fail] of [ + [ + "sync throw", + "uncaughtException", + () => { + throw new Error("unhandled sync error"); }, - async (url) => { - // The throwing request gets no proper response: Node leaves the socket - // hanging (and surfaces the throw as an uncaughtException), so the fetch - // never completes -- bound it with a short timeout. Either way it must - // NOT be a silent 2xx. - const status = await fetch(url + "throw", { signal: AbortSignal.timeout(1000) }) - .then((r) => r.status) - .catch(() => undefined); - // Give the event loop a tick for the uncaughtException to fire. - await new Promise((r) => setTimeout(r, 50)); + ], + [ + "async rejection", + "unhandledRejection", + () => Promise.reject(new Error("unhandled async error")), + ], + ] as const) { + test(`${name} answers 500 without escaping as ${event}`, async () => { + const captured = captureProcessErrors(event); - const uncaught = captured.some((e) => (e as Error)?.message === "unhandled sync error"); - expect(uncaught || (status !== undefined && status >= 500)).toBe(true); + await withServer( + (req) => (new URL(req.url).pathname === "/throw" ? fail() : new Response("ok")), + async (url) => { + // Bounded: a regression leaves the socket hanging rather than failing + // fast, so without a timeout this would stall for ~1.5s and report as + // a fetch failure rather than a 500 mismatch. + const res = await fetch(url + "throw", { signal: AbortSignal.timeout(1000) }); + expect(res.status).toBe(500); + // Bare 500: no error details leak to the client. + expect(await res.text()).toBe(""); - // The server itself survives (we swallowed the exception): a normal - // request still succeeds. - const ok = await fetch(url); - expect(ok.status).toBe(200); - expect(await ok.text()).toBe("ok"); - }, - ); - }); + // Give the event loop a tick for a process-level error to surface. + await new Promise((r) => setTimeout(r, 50)); + expect(captured).toEqual([]); + + // The server keeps serving: a normal request still succeeds. + const ok = await fetch(url); + expect(ok.status).toBe(200); + expect(await ok.text()).toBe("ok"); + }, + ); + }); + } +}); + +// The in-process tests above run inside vitest, which installs its own +// process-level error handlers -- so they can prove no error escapes, but not +// that an *unguarded* process survives one. Spawn the shared fixture to check +// that, against the same assertions Deno and Bun are held to. +describe("node (unhandled errors)", () => { + addExecUnhandledThrowTests("node ./_error-fixture.ts"); });