fix(client): propagate HTTP transport exceptions to caller#2124
Open
gspeter-max wants to merge 3 commits intomodelcontextprotocol:mainfrom
Open
fix(client): propagate HTTP transport exceptions to caller#2124gspeter-max wants to merge 3 commits intomodelcontextprotocol:mainfrom
gspeter-max wants to merge 3 commits intomodelcontextprotocol:mainfrom
Conversation
ROOT CAUSE: In SSE and StreamableHTTP transports, when HTTP errors occur or other exceptions are raised in post_writer, exceptions are caught and logged but never sent to read_stream_writer, causing the caller to hang indefinitely waiting for a response that will never arrive. CHANGES: - sse.py: Send exceptions from post_writer to read_stream_writer - streamable_http.py: Send exceptions from post_writer to read_stream_writer IMPACT: This prevents callers from hanging when HTTP errors occur in the transport layer, ensuring exceptions are properly propagated to the caller for handling. FILES MODIFIED: - src/mcp/client/sse.py - src/mcp/client/streamable_http.py Reported-by: gspeter Github-Issue: modelcontextprotocol#2110
Author
CI Failure NoteThe failing test Evidence:
Files changed in this PR:
Requesting a re-run of the failed job would be appreciated. |
ROOT CAUSE: Flaky test failures on Windows Python 3.11 CI were caused by arbitrary timeout-based waiting (sleep 0.5s + 0.3s) instead of waiting for the actual condition (child process writing to file). CHANGES: - Added _wait_for_file_to_exist() helper with condition-based polling - Replaced arbitrary sleep calls with condition-based waiting in: - test_basic_child_process_cleanup - test_nested_process_tree - test_early_parent_exit IMPACT: Eliminates race conditions in process startup timing. Tests now wait for the actual condition (file exists and is non-empty) rather than guessing how long process startup takes. FILES MODIFIED: - tests/client/test_stdio.py
Author
Update: Fixed the Flaky TestI have also fixed the flaky test that was failing on Windows Python 3.11. Root Cause: Fix:
Verification:
|
ROOT CAUSE: CI coverage check failed because timeout error paths in _wait_for_file_to_exist() were not covered (99.96% instead of required 100%). CHANGES: - Added test_wait_for_file_to_exist_timeout() to test nonexistent file case - Added test_wait_for_file_to_exist_empty_file() to test empty file case IMPACT: Brings test_stdio.py coverage back to 100%, satisfying CI requirements. These error paths are now tested with realistic timeout scenarios. FILES MODIFIED: - tests/client/test_stdio.py
Author
|
@maxisbey @Kludex @felixweinberger This PR fixes issue #2110. Would appreciate your review when you have a chance. Summary of Changes:
All tests pass locally and CI is running. Thank you! |
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
2 participants
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.
Summary
Fixes #2110 - HTTP transport swallows non-2xx status codes causing client to hang
When an MCP server returns non-2xx HTTP status codes (401/403/404/5xx), the Streamable HTTP and SSE transports catch exceptions in
post_writerbut only log them without forwarding them through the read stream. This causes callers to hang indefinitely waiting for a response that will never arrive.Root Cause
In both
sse.pyandstreamable_http.py, thepost_writerfunction catches exceptions but never sends them toread_stream_writer, breaking the exception propagation pattern that works correctly in other transports (stdio, websocket).Changes
post_writerto send exceptions toread_stream_writerpost_writerto send exceptions toread_stream_writerThis aligns with the pattern used in working transports:
stdio.pyline 156:await read_stream_writer.send(exc)websocket.pyline 59:await read_stream_writer.send(exc)Testing