fix(client): propagate SSE POST errors to caller instead of hanging (#2110) - #2483
fix(client): propagate SSE POST errors to caller instead of hanging (#2110)#2483fede-kamel wants to merge 1 commit into
Conversation
Sorry, something went wrong.
|
Hi @Kludex @felixweinberger — friendly ping for review. This fixes #2110 (HTTP transport swallowing errors → client hangs). The fix is small (~50 LOC of behavior change), and there's a substantial test suite to back it: 10 substantive tests including a negative control where I reverted the fix and confirmed all 10 hang against unpatched main. CI green, no conflicts. cc @chriscasola since you reported the original issue — happy to get any input. Would really appreciate getting a reviewer assigned when there's bandwidth. Thanks! |
Sorry, something went wrong.
|
Hey @maxisbey — could I get your eyes on this? You filed #2110 originally; this PR propagates the swallowed httpx errors (the root cause flagged in the issue) and preserves the HTTP status in ErrorData.data. Just rebased onto current main (clean, no conflicts with #2560's SSEError import refactor) and re-ran the full tests/shared/test_sse.py + tests/shared/test_streamable_http.py suite locally — 99 passed, including the network-error regression guard. ~50 LOC of behavior change with 10 substantive tests + negative control. Happy to backport to v1.x if useful — saw the recent [v1.x] activity. Closes #2110. |
Sorry, something went wrong.
|
Hi @maxisbey, @Kludex, @felixweinberger — would love your eyes on this when you have a moment 🙏 This PR takes a fresh swing at #2110 (HTTP transport swallowing non-2xx status codes and hanging the client). Current state:
Happy to iterate on anything — feedback very welcome, and thank you all for your work on this SDK! |
Sorry, something went wrong.
In `sse_client`, the message-POST coroutine `_send_message` called
`response.raise_for_status()` with no handler. When the server returned a
non-2xx (401/403/404/5xx) or the POST hit a network error, the exception
propagated into the `post_writer` task group and was swallowed by its
`except Exception: logger.exception("Error in post_writer")`. The failure
was never delivered through the read stream, so a caller blocked on
`read_stream.receive()` (e.g. `ClientSession.initialize()`) hung forever.
Catch `httpx.HTTPError` inside `_send_message` and forward it to
`read_stream_writer`, the same pattern stdio.py and websocket.py already
use, and that `streamable_http.py` uses for its >= 400 responses. The
caller now receives the error promptly instead of deadlocking.
Adds an in-process regression test (matching the modelcontextprotocol#2765 harness) whose
message POST returns 503; it asserts the caller receives the
`HTTPStatusError` via the read stream within a bounded timeout, and fails
(times out) against the unpatched client.
Refs modelcontextprotocol#2110
|
You've opened a duplicated pull request, please search opened PRs before creating new ones. Duplicated from #2122. |
Sorry, something went wrong.
Summary
Fixes the remaining client-side hang from #2110, in sse_client.
_send_message (the message-POST coroutine) called response.raise_for_status() with no handler. On a non-2xx (401/403/404/5xx) or a network error, the exception propagated into the post_writer task group and was swallowed by its except Exception: logger.exception("Error in post_writer"). The failure never reached the read stream, so a caller blocked on read_stream.receive() — e.g. ClientSession.initialize() — hung forever.
The fix catches httpx.HTTPError inside _send_message and forwards it to read_stream_writer, the pattern stdio.py and websocket.py already use, and that streamable_http.py uses for its >= 400 responses. The caller now receives the error promptly.
Scope note
This PR was originally broader. Since it was opened, main independently landed the equivalent fix for streamable_http_client (the status_code >= 400 branch in _handle_post_request now sends a JSONRPCError to the read stream with data={"http_status": ...}). That half is therefore dropped here — this is rebased on current main and scoped to the one remaining swallow path: sse_client. It's also distinct from #2340, which addresses the sse_reader/connection path rather than the message-POST path.
Verification
Closes #2110.