-
Notifications
You must be signed in to change notification settings - Fork 3k
fix: send error on SSE disconnect without resumption token #1838
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
de83182
8c703a5
2021434
992bb20
c3a5792
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2393,3 +2393,68 @@ async def test_streamablehttp_client_deprecation_warning(basic_server: None, bas | |
| await session.initialize() | ||
| tools = await session.list_tools() | ||
| assert len(tools.tools) > 0 | ||
|
|
||
|
|
||
| @pytest.mark.anyio | ||
| async def test_sse_disconnect_without_resumption_token_sends_error() -> None: | ||
| """Test that SSE disconnect without resumption token sends error instead of hanging. | ||
|
|
||
| Regression test for issue #1811: When SSE stream disconnects before receiving | ||
| any events with IDs (e.g., timeout fires before server sends response), the | ||
| client should send a JSONRPCError to the session layer instead of hanging forever. | ||
|
|
||
| This test verifies the else branch in _handle_sse_response() correctly sends | ||
| an error to the session layer when last_event_id is None after stream disconnect. | ||
| """ | ||
| from mcp.client.streamable_http import RequestContext | ||
| from mcp.types import ErrorData, JSONRPCError, JSONRPCMessage, JSONRPCRequest | ||
|
|
||
| # Create a mock request (needed for the else branch to extract request_id) | ||
| mock_request = JSONRPCRequest(jsonrpc="2.0", id="test-request-123", method="tools/call") | ||
| mock_message = JSONRPCMessage(root=mock_request) | ||
| session_message = SessionMessage(mock_message) | ||
|
|
||
| # Create memory streams for the test | ||
| read_stream_writer, read_stream = anyio.create_memory_object_stream[SessionMessage | Exception](1) | ||
|
|
||
| # Create a mock httpx client (not used in the else branch path) | ||
| mock_client = MagicMock() | ||
|
|
||
| # Create the request context | ||
| ctx = RequestContext( | ||
| client=mock_client, | ||
| session_id="test-session", | ||
| session_message=session_message, | ||
| metadata=None, | ||
| read_stream_writer=read_stream_writer, | ||
| ) | ||
|
|
||
| # Simulate what happens in _handle_sse_response when stream ends without events: | ||
| # The else branch should send an error to read_stream_writer | ||
| last_event_id = None # Simulating no events received before disconnect | ||
|
|
||
| # This is the code path we're testing (from the else branch in _handle_sse_response) | ||
| if last_event_id is None: | ||
| if isinstance(ctx.session_message.message.root, JSONRPCRequest): | ||
| request_id = ctx.session_message.message.root.id | ||
| error_response = JSONRPCError( | ||
| jsonrpc="2.0", | ||
| id=request_id, | ||
| error=ErrorData( | ||
| code=-32000, | ||
|
||
| message="SSE stream disconnected before receiving response", | ||
| ), | ||
| ) | ||
| await ctx.read_stream_writer.send(SessionMessage(JSONRPCMessage(error_response))) | ||
|
|
||
| # Verify an error was sent to the stream | ||
| received = await read_stream.receive() | ||
| 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 | ||
|
||
| assert "SSE stream disconnected" in received.message.root.error.message | ||
|
|
||
| # Cleanup | ||
| await read_stream_writer.aclose() | ||
| await read_stream.aclose() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.