Repository navigation
fix(client): close the SSE transport when its stream ends for good - #3140
arjunprabhulal wants to merge 1 commit into
Conversation
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.
🦋 Changeset detectedLatest commit: 1938f8e 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".
| if (this._endpoint !== undefined && this._eventSource?.readyState === EventSource.CLOSED) { | ||
| void this.close(); |
There was a problem hiding this comment.
If an application closes the client in onerror and starts reconnecting in onclose, this check executes after the error callback has already called close(). Because SSEClientTransport.close() leaves _eventSource in CLOSED and fires onclose unconditionally, the helper closes the old transport again and runs Protocol._onclose() twice; the second call clears _transport for the replacement connection started by the first callback. This also happens for transient errors that become CLOSED only because the application closed them. Guard against an already-closed transport, or make close() idempotent.
Useful? React with 👍 / 👎.
claude
Bot
added
the
v2
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. 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.sse.ts, the test fails:Error: Test timed out in 15000ms.With it, the test passes.@modelcontextprotocol/clientsuite passes: 45 test files, 1079 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.AI assistance: Claude Code was used for this change.