fix(server): tear down the request stream when an event store write rejects - #2609
Draft
rxits wants to merge 2 commits into
Draft
fix(server): tear down the request stream when an event store write rejects#2609rxits wants to merge 2 commits into
rxits wants to merge 2 commits into
Conversation
…ejects send() awaits the user-supplied eventStore.storeEvent() before writing a response to the per-request SSE stream. A rejection propagated straight out of send(), skipping every teardown below it: the stream mapping and the request correlation stayed in their maps, the keep-alive timer stayed armed, and the HTTP response body was never closed. The client waited on a stream that would never carry its response and the server held the request forever. The write is now wrapped so a rejection retires the request and closes its stream before rethrowing. send() still rejects, so callers continue to see the failure.
🦋 Changeset detectedLatest commit: 996b709 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: |
for...of iterates a Map directly; the array copy was unnecessary and tripped unicorn/no-useless-spread. Deleting entries during Map iteration is well-defined, and the entries removed here are the ones the loop intends to retire.
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
1 participant
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
send()awaits the user-suppliedeventStore.storeEvent()before writing a response to the per-request SSE stream. Thatawaitis unguarded, so a rejected write propagates straight out ofsend()and skips every teardown below it:_streamMappingsurvives, so the stream is never closed_requestToStreamMappingsurvives, so the request id is never retired: keepalivecomment framesThe client is left holding a stream that will never carry its response while the server keeps the request alive indefinitely. Every failed write leaks another one. An event store is user-supplied infrastructure — a transient database or network failure is an expected condition, not a programming error.
This follows the same principle as the existing shutdown-path handling: teardown must not be skippable by a throw from a user-supplied callback (#1735, #1763).
Changes
send()wraps thestoreEvent()call. On rejection it closes the request's stream via the existingcleanup()and retires every correlation and recorded response for that stream, then rethrows.send()still rejects with the original error, so callers continue to see the failure — they just no longer leave a leaked stream behind.How Has This Been Tested?
New test in
packages/server/test/server/streamableHttp.test.tsuses an event store that always rejects, assertssend()rejects with the store's error, asserts all three internal maps are empty, and asserts the SSE body is terminated rather than held open by the keep-alive timer. Verified to fail before the change and pass after.Full server suite passes (469 tests), plus
typecheckandprettier.Breaking Changes
None.
send()rejects with the same error it did before.Types of changes
Checklist
Additional context
Also reaches
NodeStreamableHTTPServerTransport, which wraps this transport.