From fb0b03e06ddee1053ea633cfb0ab22a012997be5 Mon Sep 17 00:00:00 2001 From: Rainier Potgieter Date: Wed, 27 May 2026 00:11:09 +0200 Subject: [PATCH] fix(mcp): respond with 500 on HTTP handler error instead of hanging The HTTP transport's request handler caught handleRequest rejections, wrote to stderr, and never responded. The client hung until its socket timeout and the MCP host reported the server as unresponsive rather than surfacing an error. The .catch now logs (using err.message for Error instances), then sends a structured response: status 500, Content-Type application/json, and a body of {error, detail} when headers have not been sent. If the transport already started a response, it ends the response when it is still writable. The headersSent and writableEnded guards prevent a double-write. The request listener is extracted into createRequestListener(transport) so the error path can be tested with an injected transport. The added integration test imports that factory from source, wires it to a transport whose handleRequest rejects, and asserts the client receives 500 plus a JSON body within a timeout. Before the fix that request hangs and the test fails on the timeout; after, it returns the structured error. The stdio transport is untouched. A malformed JSON-RPC body does not reach this path on the installed SDK (@modelcontextprotocol/sdk 1.29.0 returns its own 400 via Hono before the catch), so the test drives the actual rejection path through the injected transport rather than a malformed body. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/mcp/src/transport/http.ts | 41 +++++++++++++--- packages/mcp/tests/server.test.mjs | 79 ++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 6 deletions(-) diff --git a/packages/mcp/src/transport/http.ts b/packages/mcp/src/transport/http.ts index b01a194..696d545 100644 --- a/packages/mcp/src/transport/http.ts +++ b/packages/mcp/src/transport/http.ts @@ -1,19 +1,48 @@ import { randomUUID } from "node:crypto"; -import { createServer as createHttpServer } from "node:http"; +import { + createServer as createHttpServer, + type IncomingMessage, + type RequestListener, + type ServerResponse, +} from "node:http"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamableHttp.js"; +/** Minimal transport surface the request listener depends on. */ +interface RequestHandlingTransport { + handleRequest(req: IncomingMessage, res: ServerResponse): Promise; +} + +/** + * Build the Node HTTP request listener that drives the MCP transport. + * + * Extracted so the error path can be exercised in isolation. When + * handleRequest rejects, the client must receive a structured response rather + * than a hung connection that only times out at the socket level. + */ +export function createRequestListener(transport: RequestHandlingTransport): RequestListener { + return (req, res) => { + transport.handleRequest(req, res).catch((err: unknown) => { + const message = err instanceof Error ? err.message : String(err); + process.stderr.write(`HTTP handler error: ${message}\n`); + if (!res.headersSent) { + res.statusCode = 500; + res.setHeader("Content-Type", "application/json"); + res.end(JSON.stringify({ error: "Internal MCP server error", detail: message })); + } else if (!res.writableEnded) { + res.end(); + } + }); + }; +} + export async function startHttp(server: McpServer, port: number): Promise { const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID(), }); await server.connect(transport); - const httpServer = createHttpServer((req, res) => { - transport.handleRequest(req, res).catch((err: unknown) => { - process.stderr.write(`HTTP handler error: ${String(err)}\n`); - }); - }); + const httpServer = createHttpServer(createRequestListener(transport)); httpServer.listen(port, () => { process.stderr.write(`@logic-md/mcp listening on http://localhost:${port}\n`); diff --git a/packages/mcp/tests/server.test.mjs b/packages/mcp/tests/server.test.mjs index d92d8cd..de7e401 100644 --- a/packages/mcp/tests/server.test.mjs +++ b/packages/mcp/tests/server.test.mjs @@ -1,10 +1,14 @@ import { strict as assert } from "node:assert"; import { symlinkSync, unlinkSync } from "node:fs"; +import { createServer as createHttpServer, request as httpRequest } from "node:http"; import { join } from "node:path"; import { describe, it } from "node:test"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { createServer } from "../dist/server.js"; +// Imported from source: exercises the real request listener with an injected +// transport. Node strips the TypeScript types on import (>=22.18 default). +import { createRequestListener } from "../src/transport/http.ts"; // Ensure templates directory is resolved relative to this test file so // list_templates works regardless of the cwd when tests are invoked. @@ -370,3 +374,78 @@ describe("MCP server integration", () => { }); }); }); + +// ----------------------------------------------------------------------------- +// Issue #66: HTTP transport must respond on handler error, not hang +// ----------------------------------------------------------------------------- + +/** + * POST a payload and resolve with {status, contentType, body}. Rejects if no + * response arrives within timeoutMs, which is how a hung connection (the bug) + * surfaces as a test failure rather than waiting on a socket-level timeout. + */ +function postWithTimeout(port, payload, timeoutMs) { + return new Promise((resolve, reject) => { + const req = httpRequest( + { + host: "127.0.0.1", + port, + method: "POST", + path: "/", + headers: { + "Content-Type": "application/json", + "Content-Length": Buffer.byteLength(payload), + }, + }, + (res) => { + let data = ""; + res.on("data", (chunk) => { + data += chunk; + }); + res.on("end", () => { + resolve({ + status: res.statusCode, + contentType: res.headers["content-type"], + body: data, + }); + }); + }, + ); + req.setTimeout(timeoutMs, () => { + req.destroy(new Error(`request timed out after ${timeoutMs}ms (hung connection)`)); + }); + req.on("error", reject); + req.write(payload); + req.end(); + }); +} + +describe("HTTP transport error response (issue #66)", () => { + it("responds 500 with a structured JSON body when handleRequest rejects", async () => { + // Stub transport whose handleRequest rejects, simulating the failure + // path. Before the fix the listener only logs to stderr and never ends + // the response, so the client hangs until its socket timeout. + const rejectingTransport = { + async handleRequest() { + throw new Error("simulated handler failure"); + }, + }; + const httpServer = createHttpServer(createRequestListener(rejectingTransport)); + await new Promise((resolve) => httpServer.listen(0, "127.0.0.1", resolve)); + const { port } = httpServer.address(); + + try { + const res = await postWithTimeout(port, '{"jsonrpc":"2.0","method":"x","id":1}', 2000); + assert.strictEqual(res.status, 500, `expected status 500, got ${res.status}`); + assert.ok( + res.contentType?.includes("application/json"), + `expected application/json content-type, got: ${res.contentType}`, + ); + const parsed = JSON.parse(res.body); + assert.ok(parsed.error, `expected an error field in body, got: ${res.body}`); + assert.ok("detail" in parsed, `expected a detail field in body, got: ${res.body}`); + } finally { + await new Promise((resolve) => httpServer.close(resolve)); + } + }); +});