Repository navigation
fix(node): cancel the response stream when the client disconnects - #3151
Open
SwapnilSkr wants to merge 1 commit into
Open
SwapnilSkr wants to merge 1 commit into
SwapnilSkr wants to merge 1 commit into
Conversation
`toNodeHandler` read the response body with `for await`, so it only saw a client disconnect when the stream produced its next chunk. On an idle GET stream that held the session's only GET slot until the next keep-alive, and reconnects got 409 until the client gave up. Read through a reader and cancel it on abort. Racing `next()` against the abort does not work here: `return()` on the async iterator waits for the pending `next()`, so an idle stream would still not be cancelled.
🦋 Changeset detectedLatest commit: 6ca7b5b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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: |
JosephDoUrden
left a comment
Contributor
There was a problem hiding this comment.
pulled 6ca7b5b, 0 behind main. @modelcontextprotocol/node 5 files, 106 passed. new test fails with main's toNodeHandler.ts (1 failed, 14 passed). real socket on node 22.23.2, idle stream, client drops after first chunk: main cancel calls 0, this branch 1, within 2ms of destroy. plain Response('ok') still 200.
no findings. lgtm.
This branch has not been deployed
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
Fixes #3148.
toNodeHandlerread the response body withfor await, so it only noticed a client disconnect when the stream produced its next chunk. An idle session GET stream produces nothing until the next keep-alive, up to 15s by default, and its slot stays taken until then. During that time every reconnect gets 409. With default options the client retries twice in about 2.5s and gives up, so one dropped connection, such as a proxy idle timeout, ends notifications for the rest of the session.NodeStreamableHTTPServerTransportalready releases the slot in about 2ms in the same test.What changed
packages/middleware/node/src/toNodeHandler.ts: read the body through a reader and cancel it when the abort signal fires. The transport'scancelhandler then frees the GET slot straight away. The existing backpressure handling is unchanged.packages/middleware/node/test/toNodeHandler.test.ts: add a test that closes the response while the stream is idle and checks thatcancelruns.@modelcontextprotocol/node).No new API or options.
Racing
iterator.next()against the abort, as suggested in the issue, is not enough. It exits the loop, butreturn()on the async iterator waits for the pendingnext(), so the stream is not cancelled and the 409 stays. The new test fails with that version.Behaviour to review
All response streams now cancel at disconnect rather than at their next chunk, including POST SSE streams. The existing
hosting:http:disconnect-not-cancelscenario still passes.How Has This Been Tested?
mainand passes with this change.@modelcontextprotocol/nodetests: 106 passed.pnpm build:all,pnpm typecheck:allandpnpm lint:allpass.pnpm test:allpasses except forprotocol:timeout:max-totalintest/e2e/scenarios/protocol.test.ts, which is flaky and not related to this change. Run on its own, it failed 1 of 10 runs with this change and 1 of 10 without it.Tested locally on Node 25.9.0, macOS.
Breaking Changes
None.
Types of changes