fix: clean up SSE session on client disconnect#2200
fix: clean up SSE session on client disconnect#2200maxisbey merged 5 commits intomodelcontextprotocol:mainfrom
Conversation
Remove session from _read_stream_writers when a client disconnects from the SSE endpoint. Previously, stale session entries accumulated indefinitely, causing issues after server reloads where old session IDs would still be found in the dict (but with closed streams). Fixes modelcontextprotocol#423 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Please provide evidence this fixes the original issue, I'd be keen to see both a reproduction and a unit test if possible |
Add test_sse_session_cleanup_on_disconnect that verifies: - After a client disconnects, the stale session is removed from _read_stream_writers - POST requests to the disconnected session return 404 (not 202) This provides evidence that the fix for modelcontextprotocol#423 works correctly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I've added a regression test (test_sse_session_cleanup_on_disconnect in tests/shared/test_sse.py) that:
The test uses a retry loop with a 5-second timeout to account for the async disconnect processing, and runs alongside the existing 26 SSE tests (all passing). |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…l#1227) The fix addresses stale session entries in _read_stream_writers after disconnect (202 + ClosedResourceError symptom from modelcontextprotocol#1227), not the EventSource auto-reconnect scenario from modelcontextprotocol#423 which is a client-side concern already mitigated by modelcontextprotocol#822 and modelcontextprotocol#1478.
|
Thanks for this @Varun6578 — the fix is correct and the test is solid (fails on main with A few housekeeping notes for the record: This actually fixes #1227, not #423. I've retargeted the PR accordingly. #1227 is the exact production symptom this addresses: That issue was closed as not-planned because nobody provided a repro — your test is that repro. Why this isn't #423: #423's reproduction is a full uvicorn restart. After a process restart, Spec alignment: The legacy SSE spec (2024-11-05) is silent on dead-session responses, but Streamable HTTP (2025-03-26 onwards) says "the server MAY terminate the session at any time, after which it MUST respond to requests containing that session ID with HTTP 404 Not Found." The server already returns 404 for never-existed session IDs; this makes disconnected sessions hit the same path. Re: #2044 — that's a byte-identical diff without a test; will close as superseded. Pushed one small commit to update the test docstring reference. |
On Python 3.11 with coverage 7.10.7 (lowest-direct), plain statements immediately after async with sse_client(...) exits are not traced reliably due to cancellation in __aexit__ interacting with 3.11's zero-cost exception bytecode. Entering a new async with block does generate a line event, so removing the intermediate assert and indexing the list directly inside the httpx block sidesteps the tracer gap without needing a pragma.
Note
EDIT from @maxisbey: Retargeting to
Fixes #1227— see #2200 (comment) for details. The fix is correct and useful, but #423 is a different bug (client-side EventSource auto-reconnect without re-initialize) already mitigated by #822 and #1478. The "server reload" claim below is inaccurate — a uvicorn restart gives a fresh empty dict; this leak only occurs within a single process lifetime.Summary
Fixes #1227
After a client disconnects from the SSE endpoint, the session entry was never removed from
_read_stream_writers. This caused stale sessions to accumulate,and after a server reload old session IDs could still be found in the dict (with closed streams),leading to confusing behavior.Changes
src/mcp/server/sse.py:self._read_stream_writers.pop(session_id, None)in the disconnect handler (response_wrapper) to clean up the session entry when a client disconnectsThis is a one-line fix. The streams were already being closed on disconnect; this ensures the session dict entry is also removed so that subsequent POST requests to the old session ID correctly receive a 404 response.