Repository navigation
[v1.x] Resolve a failed SSE message POST instead of hanging the caller - #3785
Mart22052006 wants to merge 1 commit into
Conversation
The SSE client's post_writer called raise_for_status() on every message POST and swallowed the error, so a request whose POST failed (a 429 from a rate limiter, a 401, a refused connection) was never resolved: its caller waited for its own timeout, or forever without one. The write loop died with it, so every later message was dropped. A failed POST now resolves its request with a JSON-RPC error, with the outcomes modelcontextprotocol#3278 gives on main: a non-2xx status gives INTERNAL_ERROR, or INVALID_REQUEST "Session terminated" for a 404 while the endpoint carries a session id; an error before any response gives CONNECTION_CLOSED. A failed notification is only logged, and the write loop keeps going. Unlike on main, the error is delivered from a task of its own: v1.x ClientSession answers server requests inline in its receive loop, so a reply can be waiting on the write stream just as the error needs the receive loop, and delivering it from the write loop deadlocks. A test covers that interleaving. Github-Issue: modelcontextprotocol#2110 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and you aren't currently assigned to #2110. If a maintainer assigns you to #2110, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take. You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way. CONTRIBUTING.md has the full reasoning, but in short:
Maintainers: reopen, remove |
Fixes #2110 (the SSE part, on v1.x; #3278 covers main)
Backports the SSE half of #3278 to v1.x: a failed message POST now resolves its request with a JSON-RPC error instead of leaving the caller hanging.
Motivation and Context
sse_client'spost_writercalledraise_for_status()on every message POST and swallowed the error in a catch-all. A request whose POST failed (a 429 from a rate limiter, a 401, a refused connection) was never resolved: its caller waited for its own timeout, or forever without one. The write loop died with it, so every later message was dropped. We hit this in production: a 429 from a rate limiter in front of an MCP server left an agent's tool call waiting 90 minutes for an unrelated outer timeout.How Has This Been Tested?
10 new tests in
tests/shared/test_sse.py, all of which fail on unpatched v1.x (7 hang, 3 end with a closed stream or the wrong error):The full suite passes with 100% coverage, along with pyright and ruff. The SSE tests also pass on 3.10 with lowest-direct deps and on 3.14.
Breaking Changes
None. A caller that used to hang now gets an
McpError, and later messages keep flowing.Types of changes
Checklist
help wanted, or I'm a maintainer): asked on HTTP transport swallows non-2xx status codes causing client to hang #2110Additional context
The outcomes match #3278 on main: a non-2xx status gives
INTERNAL_ERROR"Server returned an error response", orINVALID_REQUEST"Session terminated" for a 404 while the endpoint carries a session id; an error before any response givesCONNECTION_CLOSED"Failed to send message: …".One v1.x-specific difference: the error is delivered from a task of its own. v1.x
ClientSessionanswers server requests inline in its receive loop, so a reply can be waiting on the write stream just as the error needs the receive loop, and delivering it from the write loop deadlocks.AI assistance: written with Claude Code, as the commit's
Co-Authored-Bytrailer notes.