Skip to content

Commit d093cab

Browse files
Address review feedback: docstring formatting, test reorganization
- Reformat session_idle_timeout docstring to use 4-space continuation indent and fill to 120 chars - Move idle timeout tests from tests/issues/ into tests/server/test_streamable_http_manager.py alongside existing session manager tests - Remove redundant/unnecessary tests (6 dropped, 5 kept) - Replace sleep-based test assertions with event-based polling using anyio.fail_after for deterministic behavior Github-Issue: #1283
1 parent 84ac697 commit d093cab

File tree

4 files changed

+74
-275
lines changed

4 files changed

+74
-275
lines changed

CLAUDE.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,13 @@ This document contains critical information about working with this codebase. Fo
2828
- Bug fixes require regression tests
2929
- IMPORTANT: The `tests/client/test_client.py` is the most well designed test file. Follow its patterns.
3030
- IMPORTANT: Be minimal, and focus on E2E tests: Use the `mcp.client.Client` whenever possible.
31+
- NEVER use `anyio.sleep()` with a fixed duration as a synchronization mechanism. Instead:
32+
- Poll the condition you're waiting for: `while condition: await anyio.sleep(0)` inside `anyio.fail_after(5)`
33+
- `anyio.sleep(0)` is a zero-delay scheduler yield, not a real sleep
34+
- For cancel scope deadlines, set `scope.deadline = anyio.current_time()` to trigger immediately
35+
- For stream messages, use `await stream.receive()` instead of `sleep()` + `receive_nowait()`
36+
- For callbacks/notifications, use `anyio.Event` — set it in the callback, await it in the test
37+
- If a sleep seems unnecessary, just remove it
3138

3239
Test files mirror the source tree: `src/mcp/client/streamable_http.py``tests/client/test_streamable_http.py`
3340
Add tests to the existing file for that module.

src/mcp/server/streamable_http_manager.py

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,11 @@ class StreamableHTTPSessionManager:
5757
security_settings: Optional transport security settings.
5858
retry_interval: Retry interval in milliseconds to suggest to clients in SSE
5959
retry field. Used for SSE polling behavior.
60-
session_idle_timeout: Optional idle timeout in seconds for stateful sessions.
61-
If set, sessions that receive no HTTP requests for this
62-
duration will be automatically terminated and removed.
63-
When retry_interval is also configured, ensure the idle
64-
timeout comfortably exceeds the retry interval to avoid
65-
reaping sessions during normal SSE polling gaps.
66-
Default is None (no timeout). A value of 1800
67-
(30 minutes) is recommended for most deployments.
60+
session_idle_timeout: Optional idle timeout in seconds for stateful sessions. If set, sessions that receive
61+
no HTTP requests for this duration will be automatically terminated and removed. When retry_interval is
62+
also configured, ensure the idle timeout comfortably exceeds the retry interval to avoid reaping sessions
63+
during normal SSE polling gaps. Default is None (no timeout). A value of 1800 (30 minutes) is recommended
64+
for most deployments.
6865
"""
6966

7067
def __init__(

tests/issues/test_1283_idle_session_cleanup.py

Lines changed: 0 additions & 267 deletions
This file was deleted.

tests/server/test_streamable_http_manager.py

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,3 +313,65 @@ async def mock_receive():
313313
assert error_data["id"] == "server-error"
314314
assert error_data["error"]["code"] == INVALID_REQUEST
315315
assert error_data["error"]["message"] == "Session not found"
316+
317+
318+
@pytest.mark.anyio
319+
async def test_idle_session_is_reaped():
320+
"""Session should be removed from _server_instances after idle timeout."""
321+
app = Server("test-idle-reap")
322+
manager = StreamableHTTPSessionManager(app=app, session_idle_timeout=300)
323+
324+
async with manager.run():
325+
sent_messages: list[Message] = []
326+
327+
async def mock_send(message: Message):
328+
sent_messages.append(message)
329+
330+
scope = {
331+
"type": "http",
332+
"method": "POST",
333+
"path": "/mcp",
334+
"headers": [(b"content-type", b"application/json")],
335+
}
336+
337+
async def mock_receive(): # pragma: no cover
338+
return {"type": "http.request", "body": b"", "more_body": False}
339+
340+
await manager.handle_request(scope, mock_receive, mock_send)
341+
342+
session_id = None
343+
for msg in sent_messages: # pragma: no branch
344+
if msg["type"] == "http.response.start": # pragma: no branch
345+
for header_name, header_value in msg.get("headers", []): # pragma: no branch
346+
if header_name.decode().lower() == MCP_SESSION_ID_HEADER.lower():
347+
session_id = header_value.decode()
348+
break
349+
if session_id: # pragma: no branch
350+
break
351+
352+
assert session_id is not None, "Session ID not found in response headers"
353+
assert session_id in manager._server_instances
354+
355+
# Force the idle deadline to expire now
356+
transport = manager._server_instances[session_id]
357+
assert transport.idle_scope is not None
358+
transport.idle_scope.deadline = anyio.current_time()
359+
360+
with anyio.fail_after(5):
361+
while session_id in manager._server_instances:
362+
await anyio.sleep(0)
363+
364+
assert session_id not in manager._server_instances
365+
366+
367+
@pytest.mark.parametrize(
368+
"kwargs,match",
369+
[
370+
({"session_idle_timeout": -1}, "positive number"),
371+
({"session_idle_timeout": 0}, "positive number"),
372+
({"session_idle_timeout": 30, "stateless": True}, "not supported in stateless"),
373+
],
374+
)
375+
def test_session_idle_timeout_validation(kwargs: dict[str, Any], match: str):
376+
with pytest.raises(ValueError, match=match):
377+
StreamableHTTPSessionManager(app=Server("test"), **kwargs)

0 commit comments

Comments
 (0)