fix: send error on SSE disconnect without resumption token #1838
Conversation
When SSE stream disconnects before receiving any events with IDs, the client would hang forever waiting for a response. This adds an else branch to send a JSONRPCError to the session layer when reconnection is not possible. Fixes modelcontextprotocol#1811
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #1811 where SSE stream disconnections without receiving a resumption token would cause the client to hang indefinitely. The fix adds an else branch to send a JSONRPCError to the session layer when no event ID has been received before disconnection, preventing deadlock.
Key Changes:
- Added error handling for SSE disconnections without resumption tokens in
_handle_sse_response() - Sends a JSONRPCError with code -32000 (CONNECTION_CLOSED) to notify the session layer
- Includes comprehensive test coverage for the new error path
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/mcp/client/streamable_http.py | Adds else branch in _handle_sse_response() to send JSONRPCError when SSE stream disconnects without receiving an event ID, preventing indefinite hangs |
| tests/shared/test_streamable_http.py | Adds regression test that verifies error is sent to session layer when SSE stream disconnects before receiving resumption token |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/mcp/client/streamable_http.py
Outdated
| jsonrpc="2.0", | ||
| id=request_id, | ||
| error=ErrorData( | ||
| code=-32000, |
There was a problem hiding this comment.
The error code -32000 should use the CONNECTION_CLOSED constant defined in mcp.types instead of a magic number. The constant is already imported and available. Using the constant improves maintainability and makes the error code's meaning clearer.
| code=-32000, | |
| code=CONNECTION_CLOSED, |
tests/shared/test_streamable_http.py
Outdated
| jsonrpc="2.0", | ||
| id=request_id, | ||
| error=ErrorData( | ||
| code=-32000, |
There was a problem hiding this comment.
The error code -32000 should use the CONNECTION_CLOSED constant defined in mcp.types instead of a magic number. Using the constant improves test maintainability and ensures consistency with the production code.
tests/shared/test_streamable_http.py
Outdated
| assert isinstance(received, SessionMessage) | ||
| assert isinstance(received.message.root, JSONRPCError) | ||
| assert received.message.root.id == "test-request-123" | ||
| assert received.message.root.error.code == -32000 |
There was a problem hiding this comment.
The error code assertion should use the CONNECTION_CLOSED constant from mcp.types instead of the magic number -32000. This would make the test more maintainable and ensure consistency with the constant definition.
|
As there's no linked issue or description I've set this PR to draft for now. |
Rewrite test to actually call _handle_sse_response() instead of duplicating the implementation logic inline. This ensures lines 440-451 in streamable_http.py are properly covered. The test mocks EventSource to yield no events, simulating a stream that closes immediately without sending any resumption tokens. Github-Issue: modelcontextprotocol#1811
The else branch on the isinstance check is unreachable in practice since POST SSE responses always contain JSONRPCRequest messages. Github-Issue: modelcontextprotocol#1811
Yea my bad, my setup crashed. lol |
2 participants
Summary
Fixes #1811
When an SSE stream disconnects before the server sends any events with IDs (no resumption token), the client now sends a JSONRPCError to the session layer instead of hanging indefinitely.
Changes
_handle_sse_responsefor streams that close without sending resumption tokensCONNECTION_CLOSED(-32000) error code with descriptive messageTest Plan
test_sse_disconnect_without_resumption_token_sends_errorunit test