fix(mcp): HTTP transport responds with 500 on handler error instead of hanging#79
Conversation
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) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe HTTP transport previously logged rejections to stderr but never sent responses to clients, leaving them hanging. This change extracts request handling into a reusable ChangesHTTP transport error response handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
The MCP HTTP transport's request handler caught
handleRequestrejections, wrote to stderr, and never responded. The client hung until its socket-level timeout, and the MCP host (Claude Desktop, Cursor, etc.) reported the server as unresponsive rather than surfacing an actual error. The stdio transport does not share this path and is untouched.Fix
In
packages/mcp/src/transport/http.ts, the.catchnow:err.messagewhenerris anError),!res.headersSent: setsstatusCode = 500,Content-Type: application/json, and ends with{ error, detail },!res.writableEnded: ends the response.The
headersSent/writableEndedguards prevent a double-write when the transport already started a response.The request listener is extracted into a named
createRequestListener(transport)factory (behaviour otherwise unchanged) so the error path can be exercised in isolation.Test (TDD, RED first)
Added an integration test in
packages/mcp/tests/server.test.mjsthat importscreateRequestListenerfrom source, wires it to a stub transport whosehandleRequestrejects, starts a realhttp.Server, POSTs a body, and asserts the client receives status500plus a JSON{ error, detail }body within a 2s timeout.Observed RED before the fix:
GREEN after the fix (mcp suite: 19 pass, 0 fail).
Why the test injects a rejecting transport rather than POSTing a malformed body
The issue suggested POSTing a malformed JSON-RPC body. Empirically, on the installed SDK (
@modelcontextprotocol/sdk1.29.0)handleRequestdelegates to Hono's request listener, which returns its own structured400for malformed JSON, badAccept, missing session, and truncated bodies. None of those reach the.catch, so a malformed-body test passes before and after the fix and proves nothing. The test therefore drives the real rejection path through an injected transport, which is genuinely RED before the change.Test imports from TypeScript source
The test imports the factory from
../src/transport/http.ts. Node 22.18+ strips TypeScript types on import by default (confirmed on 22.22.2), so this needs no build-config or test-runner change.tsup.config.tswas not touched.Verification (node v22.22.2)
npm run build:coreand fullnpm run build: successnpm test(root vitest): 473 passed, 0 failednode --test: 19 pass, 0 fail (includes the new bug(mcp): HTTP transport logs but never responds to client on handler error #66 test)npm run typecheck(root): cleannpm run lint: both changed files clean (repo baseline of 5 errors / 22 warnings is in untouched files)node spec/fixtures/run-fixtures.mjs: 29 passed, 0 failedNote: the mcp package's standalone
tsc --noEmitreports 44 errors (missing node globals), but this is pre-existing onmain(identical count with and without this change) and stems from the mcp package's tsconfig not resolving@types/node; it is unrelated to this fix and not among the project gates.Closes #66
Summary by cubic
Fixes the MCP HTTP transport so handler failures return a 500 JSON error instead of hanging, letting clients see a clear error and hosts avoid marking the server as unresponsive.
Bug Fixes
handleRequestrejects.res.headersSentandres.writableEnded; log the error to stderr.Refactors
createRequestListener(transport)to isolate and test the error path; behavior unchanged and stdio transport untouched.Written for commit 2f7606e. Summary will update on new commits. Review in cubic
Summary by CodeRabbit