Add top-level Fastify error handler that logs full stack on 5xx (JavaScriptSolidServer#312)

melvincarvalho · melvincarvalho · commit 5b34d721bf4d · 2026-04-23T04:50:49.000+02:00
Without this, 500 responses carried only the 91-byte Fastify default body and nothing hit stdout/pm2 — the JavaScriptSolidServer#309 investigation had to infer the exception from the response-body size. Now every 5xx logs err.stack plus method/url/hostname via request.log.error. 4xx errors are unaffected. Response body shape is preserved (reply.send(err) delegates to Fastify's default serialization), so no existing consumer sees a behavior change.
diff --git a/src/server.js b/src/server.js
@@ -21,6 +21,7 @@ import { dbPlugin } from './db/index.js';
 import { webrtcPlugin } from './webrtc/index.js';
 import { tunnelPlugin } from './tunnel/index.js';
 import { terminalPlugin } from './terminal/index.js';
+import { registerErrorHandler } from './utils/error-handler.js';
 
 const __dirname = dirname(fileURLToPath(import.meta.url));
 
@@ -154,6 +155,7 @@ export function createServer(options = {}) {
   }
 
   const fastify = Fastify(fastifyOptions);
+  registerErrorHandler(fastify);
 
   // Add raw body parser for all content types
   fastify.addContentTypeParser('*', { parseAs: 'buffer' }, (req, body, done) => {
diff --git a/src/utils/error-handler.js b/src/utils/error-handler.js
@@ -0,0 +1,24 @@
+/**
+ * Top-level Fastify error handler that logs the full stack for any 5xx.
+ *
+ * Without this, 500 responses carry only Fastify's default 91-byte body and
+ * leave no trace in logs — production debugging has to infer the exception
+ * from the response body alone (see #309 investigation for why that's bad).
+ *
+ * 4xx errors carry their own statusCode and don't need stack logging — they
+ * are expected client errors with self-explanatory messages.
+ */
+export function registerErrorHandler(fastify) {
+  fastify.setErrorHandler(function (err, request, reply) {
+    const statusCode = err.statusCode ?? 500;
+    if (statusCode >= 500) {
+      request.log.error({
+        err,
+        method: request.method,
+        url: request.url,
+        hostname: request.hostname
+      }, 'Unhandled 5xx error');
+    }
+    reply.send(err);
+  });
+}
diff --git a/test/error-handler.test.js b/test/error-handler.test.js
@@ -0,0 +1,83 @@
+/**
+ * Tests for the top-level error handler (#312).
+ *
+ * Verifies that:
+ *  1. Unhandled exceptions from route handlers produce a 500 whose body
+ *     matches Fastify's default shape — the existing 91-byte response
+ *     format must not regress (that's the baseline consumers rely on).
+ *  2. The error's stack is actually logged (with method/url/hostname
+ *     context), so production debugging has a real trace.
+ *  3. 4xx errors are NOT stack-logged (they're expected client errors).
+ *
+ * Uses a minimal Fastify instance so the test exercises only the handler,
+ * not the full server's auth/routing stack.
+ */
+
+import { describe, it, before, after } from 'node:test';
+import assert from 'node:assert';
+import Fastify from 'fastify';
+import { registerErrorHandler } from '../src/utils/error-handler.js';
+
+class LogCapture {
+  constructor() { this.lines = []; }
+  write(chunk) {
+    try { this.lines.push(JSON.parse(chunk)); } catch { /* non-JSON */ }
+    return true;
+  }
+  errorLines() {
+    return this.lines.filter((l) => l.level >= 50);
+  }
+  clear() { this.lines.length = 0; }
+}
+
+describe('registerErrorHandler (#312)', () => {
+  let app;
+  const capture = new LogCapture();
+
+  before(async () => {
+    app = Fastify({
+      logger: { level: 'error', stream: capture },
+      disableRequestLogging: true
+    });
+    registerErrorHandler(app);
+    app.get('/throw500', async () => { throw new Error('synthetic 5xx for test'); });
+    app.get('/throw400', async () => {
+      const err = new Error('bad client input');
+      err.statusCode = 400;
+      throw err;
+    });
+  });
+
+  after(async () => { await app.close(); });
+
+  it('500 response body matches Fastify default shape (no regression)', async () => {
+    capture.clear();
+    const res = await app.inject({ method: 'GET', url: '/throw500' });
+    assert.strictEqual(res.statusCode, 500);
+    assert.deepStrictEqual(res.json(), {
+      statusCode: 500,
+      error: 'Internal Server Error',
+      message: 'synthetic 5xx for test'
+    });
+  });
+
+  it('500 logs include the stack and request context', async () => {
+    capture.clear();
+    await app.inject({ method: 'GET', url: '/throw500', headers: { host: 'example.test' } });
+    const line = capture.errorLines().find((l) => l.msg === 'Unhandled 5xx error');
+    assert.ok(line, `expected 'Unhandled 5xx error' log line, got: ${JSON.stringify(capture.errorLines())}`);
+    assert.strictEqual(line.method, 'GET');
+    assert.strictEqual(line.url, '/throw500');
+    assert.strictEqual(line.hostname, 'example.test');
+    assert.ok(line.err && line.err.stack, 'expected err.stack in log');
+    assert.ok(line.err.stack.includes('synthetic 5xx'), 'stack should identify the error');
+  });
+
+  it('4xx errors do not trigger the 5xx stack log', async () => {
+    capture.clear();
+    const res = await app.inject({ method: 'GET', url: '/throw400' });
+    assert.strictEqual(res.statusCode, 400);
+    const unhandled = capture.errorLines().filter((l) => l.msg === 'Unhandled 5xx error');
+    assert.strictEqual(unhandled.length, 0, '4xx must not produce a 5xx stack log');
+  });
+});
