fix(server): settle in-flight JSON-mode requests on transport close - #2584
Draft
axits-lab wants to merge 3 commits into
Draft
fix(server): settle in-flight JSON-mode requests on transport close#2584axits-lab wants to merge 3 commits into
axits-lab wants to merge 3 commits into
Conversation
`close()` runs every stream mapping's `cleanup`. JSON response mode's `cleanup` only deleted the map entry, so the `Promise<Response>` returned by `handleRequest()` was never settled and a POST whose handler was still running hung until the client gave up. Resolve it with a 503 JSON-RPC error instead. The success path is unaffected: `send()` resolves the response before calling `cleanup()`, and re-resolving an already-settled promise is a no-op, so this only fires when nothing was sent. Note the companion half of modelcontextprotocol#2559 — the `_streamMapping` leak on completed JSON-mode POSTs — no longer reproduces on main; `send()` has called `stream.cleanup()` since modelcontextprotocol#2286. Verified: `_streamMapping.size` is 0 after a completed JSON-mode POST. Refs modelcontextprotocol#2559 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 1472942 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
The handler now signals that it has parked via a promise the test awaits, instead of the test polling a boolean on a 5ms tick. close() still has to land while the handler is parked — landing earlier means the POST is not yet validated and the test would pass for the wrong reason — but that no longer depends on how fast the runner is. The single remaining timer is the hang detector, which only fires on a regression: with the fix the response is already resolved by the time close() returns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Promise.race left the 2s timeout pending whenever the request won, so a passing run handed a live timer to the rest of the suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #2559.
One half reproduces, one no longer does
The hang — real, fixed here.
close()runs every stream mapping'scleanup. JSON response mode's cleanup only deleted the map entry:so the
Promise<Response>returned byhandleRequest()was never settled and a POST whose handler was still running hung indefinitely. Probed by parking a tool handler, waiting until it had genuinely started, then closing:It now resolves with a
503JSON-RPC error:The success path is untouched —
send()resolves before it callscleanup(), and re-resolving a settled promise is a no-op, so this only fires when nothing was sent.The leak — no longer reproduces on
main. The issue notessend()"never runs the mapping'scleanup", but it has since #2286 (merged 2026-06-26);stream.cleanup()is called on both branches atstreamableHttp.ts:1229/:1232. Measured directly:So I've left that half alone rather than write a no-op change. Shout if you're seeing it on a path I haven't hit.
Test
Added to the existing JSON-response-mode block. It waits until the handler is genuinely parked before closing — closing earlier lands before the POST is even validated and the test proves nothing (my first attempt did exactly that and passed for the wrong reason).
Fails without the src change:
Verification
469/469tests pass in@modelcontextprotocol/server;99/99in@modelcontextprotocol/node(which wraps this transport)pnpm lintandpnpm typecheckcleanChangeset included.
On your note that this applies to
v1.xtoo — happy to put up the equivalent there, and I asked on the issue whether you'd prefer that as its own PR or a backport after this lands.