fix(node): answer 500 for unhandled handler errors

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;

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<Response>;
+      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);
     };

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;

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<Response>,
     fn: (url: string) => Promise<void>,
   ) {
-    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");
 });