fix(mcp): require SDK >=1.26.0 — one MCP request 500s the entire site on self-hosted - #1238
fix(mcp): require SDK >=1.26.0 — one MCP request 500s the entire site on self-hosted#1238fawadafr wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe ChangesMCP request lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HTTPClient
participant MCPHandler
participant MCPTransport
participant McpServer
HTTPClient->>MCPHandler: Send MCP initialize request
MCPHandler->>MCPTransport: Dispatch request
MCPTransport->>McpServer: Handle MCP request
MCPHandler->>MCPTransport: Close transport
MCPHandler->>McpServer: Close server
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/api/mcp.ts`:
- Around line 1431-1467: Update parseBody to accumulate incoming chunks as raw
Buffers instead of appending them to a string, while preserving the existing
byte-limit tracking and rejection behavior. In the req "end" handler,
concatenate the buffered chunks and decode the complete byte sequence as UTF-8
before attempting JSON.parse, ensuring split multi-byte characters remain
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c45fa330-415d-43b9-a199-413584687cc7
📒 Files selected for processing (2)
src/__tests__/api/mcp-global-response.test.tssrc/pages/api/mcp.ts
| async function parseBody(req: NextApiRequest): Promise<unknown> { | ||
| const MAX_BODY_SIZE = 1024 * 1024; // 1MB | ||
| return new Promise((resolve, reject) => { | ||
| let body = ""; | ||
| let bytesReceived = 0; | ||
| let settled = false; | ||
| const settle = (action: () => void) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| action(); | ||
| }; | ||
|
|
||
| req.on("data", (chunk: Buffer | string) => { | ||
| bytesReceived += Buffer.isBuffer(chunk) ? chunk.length : Buffer.byteLength(chunk); | ||
| body += chunk; | ||
| if (bytesReceived > MAX_BODY_SIZE) { | ||
| req.destroy(); | ||
| reject(new PayloadTooLargeError()); | ||
| settle(() => reject(new PayloadTooLargeError())); | ||
| return; | ||
| } | ||
| }); | ||
| req.on("end", () => { | ||
| try { | ||
| resolve(JSON.parse(body)); | ||
| } catch { | ||
| resolve(body); | ||
| } | ||
| settle(() => { | ||
| try { | ||
| resolve(JSON.parse(body)); | ||
| } catch { | ||
| resolve(body); | ||
| } | ||
| }); | ||
| }); | ||
| req.on("error", reject); | ||
| req.on("error", (err) => settle(() => reject(err))); | ||
| // A client that disconnects mid-upload emits `close` without `end` or `error`. | ||
| // Without this the promise never settles and the McpServer/transport it is | ||
| // holding are retained for the lifetime of the process. | ||
| req.on("close", () => settle(() => reject(new ClientDisconnectedError()))); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Multi-byte UTF-8 chunks can corrupt the parsed body.
body += chunk at Line 1445 implicitly calls chunk.toString() on each Buffer with default utf8 decoding. If a multi-byte UTF-8 character splits across two TCP chunks, each chunk decodes independently and the character corrupts (replacement bytes on each side of the split). For save_prompt/save_skill requests with non-ASCII title/content, this can produce a body that fails JSON.parse or silently stores corrupted text, depending on where the split lands.
Accumulate raw Buffers and decode once with Buffer.concat(...).toString("utf8") (or a StringDecoder) after the stream ends, so decoding always happens on the complete byte sequence.
🐛 Proposed fix to decode after concatenation
async function parseBody(req: NextApiRequest): Promise<unknown> {
const MAX_BODY_SIZE = 1024 * 1024; // 1MB
return new Promise((resolve, reject) => {
- let body = "";
+ const chunks: Buffer[] = [];
let bytesReceived = 0;
let settled = false;
const settle = (action: () => void) => {
if (settled) return;
settled = true;
action();
};
req.on("data", (chunk: Buffer | string) => {
- bytesReceived += Buffer.isBuffer(chunk) ? chunk.length : Buffer.byteLength(chunk);
- body += chunk;
+ const buf = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk);
+ bytesReceived += buf.length;
+ chunks.push(buf);
if (bytesReceived > MAX_BODY_SIZE) {
req.destroy();
settle(() => reject(new PayloadTooLargeError()));
return;
}
});
req.on("end", () => {
settle(() => {
+ const body = Buffer.concat(chunks).toString("utf8");
try {
resolve(JSON.parse(body));
} catch {
resolve(body);
}
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function parseBody(req: NextApiRequest): Promise<unknown> { | |
| const MAX_BODY_SIZE = 1024 * 1024; // 1MB | |
| return new Promise((resolve, reject) => { | |
| let body = ""; | |
| let bytesReceived = 0; | |
| let settled = false; | |
| const settle = (action: () => void) => { | |
| if (settled) return; | |
| settled = true; | |
| action(); | |
| }; | |
| req.on("data", (chunk: Buffer | string) => { | |
| bytesReceived += Buffer.isBuffer(chunk) ? chunk.length : Buffer.byteLength(chunk); | |
| body += chunk; | |
| if (bytesReceived > MAX_BODY_SIZE) { | |
| req.destroy(); | |
| reject(new PayloadTooLargeError()); | |
| settle(() => reject(new PayloadTooLargeError())); | |
| return; | |
| } | |
| }); | |
| req.on("end", () => { | |
| try { | |
| resolve(JSON.parse(body)); | |
| } catch { | |
| resolve(body); | |
| } | |
| settle(() => { | |
| try { | |
| resolve(JSON.parse(body)); | |
| } catch { | |
| resolve(body); | |
| } | |
| }); | |
| }); | |
| req.on("error", reject); | |
| req.on("error", (err) => settle(() => reject(err))); | |
| // A client that disconnects mid-upload emits `close` without `end` or `error`. | |
| // Without this the promise never settles and the McpServer/transport it is | |
| // holding are retained for the lifetime of the process. | |
| req.on("close", () => settle(() => reject(new ClientDisconnectedError()))); | |
| }); | |
| } | |
| async function parseBody(req: NextApiRequest): Promise<unknown> { | |
| const MAX_BODY_SIZE = 1024 * 1024; // 1MB | |
| return new Promise((resolve, reject) => { | |
| const chunks: Buffer[] = []; | |
| let bytesReceived = 0; | |
| let settled = false; | |
| const settle = (action: () => void) => { | |
| if (settled) return; | |
| settled = true; | |
| action(); | |
| }; | |
| req.on("data", (chunk: Buffer | string) => { | |
| const buf = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); | |
| bytesReceived += buf.length; | |
| chunks.push(buf); | |
| if (bytesReceived > MAX_BODY_SIZE) { | |
| req.destroy(); | |
| settle(() => reject(new PayloadTooLargeError())); | |
| return; | |
| } | |
| }); | |
| req.on("end", () => { | |
| settle(() => { | |
| const body = Buffer.concat(chunks).toString("utf8"); | |
| try { | |
| resolve(JSON.parse(body)); | |
| } catch { | |
| resolve(body); | |
| } | |
| }); | |
| }); | |
| req.on("error", (err) => settle(() => reject(err))); | |
| // A client that disconnects mid-upload emits `close` without `end` or `error`. | |
| // Without this the promise never settles and the McpServer/transport it | |
| // is holding are retained for the lifetime of the process. | |
| req.on("close", () => settle(() => reject(new ClientDisconnectedError()))); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/api/mcp.ts` around lines 1431 - 1467, Update parseBody to
accumulate incoming chunks as raw Buffers instead of appending them to a string,
while preserving the existing byte-limit tracking and rejection behavior. In the
req "end" handler, concatenate the buffered chunks and decode the complete byte
sequence as UTF-8 before attempting JSON.parse, ensuring split multi-byte
characters remain intact.
On any long-lived Node deployment (`next start`, Docker, Cloud Run, Fly,
a VPS) a single POST /api/mcp permanently breaks every route. After one
request, /, /login and /api/health all return 500 with:
TypeError: Expected an instance of Response to be returned
Only a process restart clears it. Vercel and other serverless hosts are
unaffected, which is why this is invisible on the hosted site.
Cause: @hono/node-server replaces the Node globals inside
getRequestListener() unless passed `overrideGlobalObjects: false`:
if (options.overrideGlobalObjects !== false && global.Request !== Request) {
Object.defineProperty(global, "Request", { value: Request })
Object.defineProperty(global, "Response", { value: Response })
}
@modelcontextprotocol/sdk 1.25.x calls it with no options, from the
transport constructor and from every handleRequest. NextResponse
subclasses the *original* Response captured at module load, so once the
global is swapped, src/proxy.ts fails Next's `response instanceof
Response` check in next/dist/esm/server/web/adapter.js and throws for
every request matching the proxy matcher.
The SDK fixed this in 1.26.0 by passing `overrideGlobalObjects: false`
(upstream issue #1369). package.json declared ^1.24.3 -- a version with
no hono dependency at all -- so the caret range silently resolved into
the broken 1.25.x window and the lockfile pinned it there. Note that
hono itself is NOT fixed: @hono/node-server@2.0.12 still overrides
global Response, so the SDK floor is the actual fix.
Raising the floor to ^1.26.0 and refreshing the lockfile resolves it.
Verified: with the SDK bumped and no application changes, the added
regression test passes; on 1.25.1 it fails.
Also fixes two pre-existing leaks in the same handler that are
independent of the SDK version:
1. res.on("close") was registered *after* awaiting handleRequest, by
which point the response may already have closed, so the listener
never fired and the McpServer + transport were never released. It is
now registered before dispatch, with an idempotent teardown() in
finally that also covers the early returns -- rate-limit 429s
previously leaked unconditionally.
2. parseBody could never settle. A client disconnecting mid-upload emits
close without end or error, pinning the server/transport for the life
of the process. It now settles once, on whichever arrives first.
Adds src/__tests__/api/mcp-global-response.test.ts, which drives a real
initialize through the handler over a real socket and asserts
globalThis.Response is unchanged -- guarding against the dependency
regressing back into the broken window.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
510d254 to
8fa770b
Compare
fawadafr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/__tests__/api/mcp-global-response.test.ts (1)
83-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
anycasts with a typed adapter.Lines 85-86 disable the explicit-
anyrule and remove compile-time checks at the boundary between Node HTTP objects and the Next API handler. Define a typed adapter for the fields consumed by the handler, and keep any unavoidable assertion narrow and explicit.As per coding guidelines, prefer explicit types over
anyand do not weaken TypeScript types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/api/mcp-global-response.test.ts` around lines 83 - 86, Replace the any casts in the http.createServer callback around the imported handler with a typed adapter matching only the Node request/response fields consumed by the Next API handler. Remove the eslint suppression, keep any unavoidable compatibility assertion narrow and explicit, and preserve the existing handler invocation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/api/mcp-global-response.test.ts`:
- Around line 92-99: Update the test setup around the MCP initialize test to
capture the original globalThis.Response and globalThis.Request in
beforeEach-scoped variables, then restore both constructors in afterEach within
a finally block while still closing the server. Remove the test-local captures
in the individual test and ensure cleanup runs even if server.close or
restoration-related logic encounters an error.
- Around line 105-122: In the MCP initialize request test, assert that the fetch
response is successful via res.ok immediately after the fetch and before
draining the body or checking global constructors. Keep the existing
response-draining behavior and subsequent global-constructor assertions
unchanged.
---
Nitpick comments:
In `@src/__tests__/api/mcp-global-response.test.ts`:
- Around line 83-86: Replace the any casts in the http.createServer callback
around the imported handler with a typed adapter matching only the Node
request/response fields consumed by the Next API handler. Remove the eslint
suppression, keep any unavoidable compatibility assertion narrow and explicit,
and preserve the existing handler invocation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea0a6b83-4f5e-4a46-82ae-5279881743a0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/__tests__/api/mcp-global-response.test.tssrc/pages/api/mcp.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/api/mcp.ts
| afterEach(async () => { | ||
| await new Promise<void>((resolve) => server.close(() => resolve())); | ||
| }); | ||
|
|
||
| it("leaves globalThis.Response identical after a real MCP initialize request", async () => { | ||
| const OriginalResponse = globalThis.Response; | ||
| const OriginalRequest = globalThis.Request; | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore global constructors during teardown.
OriginalResponse and OriginalRequest are scoped to the test, so afterEach cannot restore them. If the regression occurs, the mutated globals can remain in the Vitest worker and break later tests. Capture the originals in beforeEach and restore both globals in afterEach inside finally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/api/mcp-global-response.test.ts` around lines 92 - 99, Update
the test setup around the MCP initialize test to capture the original
globalThis.Response and globalThis.Request in beforeEach-scoped variables, then
restore both constructors in afterEach within a finally block while still
closing the server. Remove the test-local captures in the individual test and
ensure cleanup runs even if server.close or restoration-related logic encounters
an error.
| const res = await fetch(url, { | ||
| method: "POST", | ||
| headers: { | ||
| "content-type": "application/json", | ||
| accept: "application/json, text/event-stream", | ||
| }, | ||
| body: JSON.stringify({ | ||
| jsonrpc: "2.0", | ||
| id: 1, | ||
| method: "initialize", | ||
| params: { | ||
| protocolVersion: "2025-03-26", | ||
| capabilities: {}, | ||
| clientInfo: { name: "regression-test", version: "1.0.0" }, | ||
| }, | ||
| }), | ||
| }); | ||
| await res.text(); // drain so the request fully completes |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert that the MCP request succeeds.
await res.text() only drains the response. It does not prove that the request was accepted or reached MCP dispatch. If the handler returns an authentication or validation error, the globals remain unchanged and this regression test passes without testing the target path. Assert res.ok before checking the global constructors.
Proposed fix
- await res.text(); // drain so the request fully completes
+ await res.text(); // drain so the request fully completes
+ expect(res.ok).toBe(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const res = await fetch(url, { | |
| method: "POST", | |
| headers: { | |
| "content-type": "application/json", | |
| accept: "application/json, text/event-stream", | |
| }, | |
| body: JSON.stringify({ | |
| jsonrpc: "2.0", | |
| id: 1, | |
| method: "initialize", | |
| params: { | |
| protocolVersion: "2025-03-26", | |
| capabilities: {}, | |
| clientInfo: { name: "regression-test", version: "1.0.0" }, | |
| }, | |
| }), | |
| }); | |
| await res.text(); // drain so the request fully completes | |
| const res = await fetch(url, { | |
| method: "POST", | |
| headers: { | |
| "content-type": "application/json", | |
| accept: "application/json, text/event-stream", | |
| }, | |
| body: JSON.stringify({ | |
| jsonrpc: "2.0", | |
| id: 1, | |
| method: "initialize", | |
| params: { | |
| protocolVersion: "2025-03-26", | |
| capabilities: {}, | |
| clientInfo: { name: "regression-test", version: "1.0.0" }, | |
| }, | |
| }), | |
| }); | |
| await res.text(); // drain so the request fully completes | |
| expect(res.ok).toBe(true); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/api/mcp-global-response.test.ts` around lines 105 - 122, In the
MCP initialize request test, assert that the fetch response is successful via
res.ok immediately after the fetch and before draining the body or checking
global constructors. Keep the existing response-draining behavior and subsequent
global-constructor assertions unchanged.
daltino
left a comment
daltino
left a comment
There was a problem hiding this comment.
The root cause diagnosis here is sharp — @hono/node-server mutating globalThis.Response without overrideGlobalObjects: false is exactly the kind of subtle runtime pollution that's nearly impossible to catch without knowing where to look, and the regression test in mcp-global-response.test.ts is a genuinely good addition since it documents why the version pin exists so future maintainers don't blindly downgrade it. The settle guard pattern in parseBody is a nice cleanup too — previously req.destroy() followed by additional data events could have triggered multiple reject calls in the original Promise. One thing worth confirming: does @hono/node-server 2.0.x dropping Node <20 support affect any CI or deployment targets? The engine bump from >=18.14.1 to >=20 is a breaking constraint for anyone still on Node 18, which is still LTS until April 2025.
2 participants
On any long-lived Node deployment —
next start, Docker (ghcr.io/f/prompts.chat), Cloud Run, Fly, a VPS — a singlePOST /api/mcppermanently breaks every route. After one request,/,/loginand/api/healthall return 500:Only a process restart clears it. Vercel and other serverless hosts are unaffected, which is why this is invisible on the hosted site.
No credentials are needed to trigger it — the handler builds the transport before any auth check.
Cause
@hono/node-serverreplaces the Node globals insidegetRequestListener()unless passedoverrideGlobalObjects: false:@modelcontextprotocol/sdk1.25.x calls it with no options — from the transport constructor and from everyhandleRequest.NextResponsesubclasses the originalResponsecaptured at module load, so once the global is swapped,src/proxy.tsfails Next's check innext/dist/esm/server/web/adapter.js:…for every request matching the proxy matcher — i.e. the entire site. Serverless is immune because middleware runs in a separate isolate, so a Node-global mutation cannot cross realms.
Why this happened with no code change
overrideGlobalObjects: falsepackage.jsondeclared^1.24.3— a version with no hono dependency — so the caret range silently resolved into the broken 1.25.x window, and the lockfile pinned it at 1.25.1.Worth noting: hono itself is not fixed.
@hono/node-server@2.0.12still contains theObject.defineProperty(global, "Response", …)call; the referenced hono PR only removed thefetchoverride. So raising the SDK floor is the actual fix, not bumping hono.The fix
Raise the floor to
^1.26.0and refresh the lockfile (resolves to 1.30.0).Verified: with only the SDK bumped and no application code changes, the added regression test passes; on 1.25.1 it fails.
I checked #1994 (stateless transport regression) before recommending the bump — it only affects reused transport instances, and this handler constructs a fresh transport per request, so it does not apply.
Two other leaks fixed in the same handler
Both are independent of SDK version and present today:
res.on("close")was registered afterawait transport.handleRequest(...), by which point the response may already have closed — so the listener often never fired and theMcpServer+ transport were never released. Now registered before dispatch, with an idempotentteardown()infinallythat also covers the earlyreturns (rate-limit 429s previously leaked unconditionally).parseBodycould never settle. A client disconnecting mid-upload emitsclosewithoutend/error, so the promise stayed pending and pinned the server/transport for the life of the process. It now settles once, on whichever ofend/error/closearrives first.Test
src/__tests__/api/mcp-global-response.test.tsdrives a realinitializethrough the handler over a real socket and assertsglobalThis.Responseis unchanged — guarding against the dependency drifting back into the broken window. 307 API tests pass, lint clean.Found while migrating a self-hosted instance from Vercel to Cloud Run, where it caused a full outage.
🤖 Generated with Claude Code
Summary
RequestandResponseobjects around MCP transport construction and request handling.@modelcontextprotocol/sdkto^1.26.0.initializerequest and verifies that global constructors remain unchanged.Testing
src/__tests__/api/mcp-global-response.test.ts.