Repository navigation
fix(client): close the SSE transport when its stream ends for good - #3140
arjunprabhulal wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 581a4f8 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1938f8e622
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
claude
Bot
added
the
v2
After start() resolved, the EventSource onerror handler only reported the error. When the stream dropped and the reconnect failed with a non-200 status, EventSource stopped retrying but the transport never called close(), so onclose did not fire and pending requests waited for their timeout. Close the transport once the EventSource is CLOSED, unless an onerror handler already closed it, so onclose fires once.
1938f8e to
248faeb
Compare
Motivation and Context
When an
SSEClientTransportloses its stream after connecting and the reconnect fails for good, the transport stays open with nothing behind it. For example, the server restarts and the reconnect GET returns 404 or 401. In-flight requests wait for their timeout, 60 seconds by default, instead of failing, andonclosenever fires, so the application is never told it needs to reconnect.Steps to reproduce:
ClientoverSSEClientTransport.tools/list, and have the server end the SSE response before it answers.EventSourcereconnect GET with 404.Expected: the pending
tools/listrejects withConnectionClosedandonclosefires.Observed:
EventSourcestops reconnecting (readyStateisCLOSED), but the request stays pending until it times out, andoncloseis never called.Once
start()has resolved, the_eventSource.onerrorhandler inpackages/client/src/client/sse.tscallsreject()(a no-op by then) andonerror, and returns. It never checks whetherEventSourcehas given up, soclose()is never called.The handler now calls
close()when the endpoint is known andEventSourceisCLOSED. It does this on both terminal error paths, soonclosefires andProtocolrejects pending requests withConnectionClosed. It skips the close when anonerrorhandler has already closed the transport, soonclosefires once and a replacement connection started fromoncloseis not torn down. Errors beforestart()resolves, transient errors whileEventSourceis still reconnecting, and theonUnauthorizedretry path behave as before.How Has This Been Tested?
packages/client/test/client/sseStreamEnd.test.tsruns a real HTTP server. The server ends the SSE stream in the middle oftools/listand answers the reconnect with 404. The test asserts the request rejects withConnectionClosedandonclosefires.onerrorand assertsonclosefires exactly once. Without the already closed check it fires twice (expected 2 to be 1).sse.ts, the first test fails:Error: Test timed out in 15000ms.With it, both tests pass.@modelcontextprotocol/clientsuite passes: 45 test files, 1080 tests.pnpm build:all,pnpm typecheck:allandpnpm lint:allpass, andprettier --checkpasses on the changed files.Breaking Changes
None. A transport whose stream can no longer be reopened now closes instead of staying open with no stream.
Types of changes
Checklist
Additional context
v1.xhas the same handler (src/client/sse.ts:194), and most SSE users are still on v1. I can open the same fix againstv1.xif you'd like it there too.