fix(server): retire in-flight request state when the streamable HTTP transport closes - #2600
Open
rxits wants to merge 1 commit into
Open
Conversation
…transport closes close() cleared _streamMapping and _requestResponseMap but never cleared _requestToStreamMapping, so request ids stayed resolvable on a closed transport. A send() suspended in eventStore.storeEvent() across a close() resumed against the retired correlation and recorded a response that could never be delivered, leaking both the response and its correlation. close() now clears the correlations, and send() re-checks the correlation after the storeEvent() await for the same reason the stream is already re-read there. close() also settles JSON-response-mode POSTs parked waiting on their responses, answering 404 Session not found instead of leaving the HTTP request hanging until the platform's own timeout fires. Both paths also reach NodeStreamableHTTPServerTransport, which wraps this transport.
🦋 Changeset detectedLatest commit: 9ba775a 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: |
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.
Motivation and Context
WebStandardStreamableHTTPServerTransport.close()clears_streamMappingand_requestResponseMap, but never clears_requestToStreamMapping. Request ids therefore stay resolvable on a closed transport, which leaks state on two paths.1. A
send()suspended acrossclose()records an undeliverable response.send()resolvesstreamIdfrom_requestToStreamMappingbefore awaitingeventStore.storeEvent(). Ifclose()lands during that await, the resumedsend()still holds a stalestreamIdand falls through to_requestResponseMap.set(requestId, message). For a batched POST the sibling response was already dropped byclose(), soallResponsesReadyisfalseforever, the cleanup loop never runs, and the response plus both correlations leak for the lifetime of the process. The closed transport also keeps accepting further responses for those ids and writing them to the event store.2. JSON-response mode hangs the HTTP request.
In
enableJsonResponsemodehandleRequest()returns a promise parked inresolveJsonuntil every response is ready.close()runs that stream'scleanup(), which only deletes the map entry — it never resolves. The POST promise never settles, so the request hangs until the platform's own timeout fires.Both paths also reach
NodeStreamableHTTPServerTransport, which is a thin wrapper around this transport.How Has This Been Tested?
Three tests in
packages/server/test/server/streamableHttp.test.ts, each verified to fail before the change and pass after:send()parked instoreEvent()acrossclose()now rejects and leaves both maps emptyclose()settles as404 Session not foundinstead of hangingFull monorepo suite passes (
pnpm test:all), plustypecheckandprettier.Breaking Changes
None.
close()andsend()keep their signatures. Asend()that previously resolved afterclose()while recording an undeliverable response now rejects with the module's existingNo connection established for request ID: …error — the same error asend()issued afterclose()already produced, so the mid-flight case just becomes consistent with it.Types of changes
Checklist
Additional context
send()'s new post-await correlation re-check mirrors the re-read ofstreamthat already sits directly above it, and for the same reason: state can change across that await.