Skip to content

Commit 84157bb

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 96f363b commit 84157bb

File tree

4 files changed

+78
-275
lines changed

4 files changed

+78
-275
lines changed

CLAUDE.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,13 @@ This document contains critical information about working with this codebase. Fo
3131
- IMPORTANT: Before pushing, verify 100% branch coverage on changed files by running
3232
`uv run --frozen pytest -x` (coverage is configured in `pyproject.toml` with `fail_under = 100`
3333
and `branch = true`). If any branch is uncovered, add a test for it before pushing.
34+
- NEVER use `anyio.sleep()` with a fixed duration as a synchronization mechanism. Instead:
35+
- Poll the condition you're waiting for: `while condition: await anyio.sleep(0)` inside `anyio.fail_after(5)`
36+
- `anyio.sleep(0)` is a zero-delay scheduler yield, not a real sleep
37+
- For cancel scope deadlines, set `scope.deadline = anyio.current_time()` to trigger immediately
38+
- For stream messages, use `await stream.receive()` instead of `sleep()` + `receive_nowait()`
39+
- For callbacks/notifications, use `anyio.Event` — set it in the callback, await it in the test
40+
- If a sleep seems unnecessary, just remove it
3441

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

src/mcp/server/streamable_http_manager.py

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,15 @@ 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
61+
sessions. If set, sessions that receive no HTTP
62+
requests for this duration will be automatically
63+
terminated and removed. When retry_interval is
64+
also configured, ensure the idle timeout
65+
comfortably exceeds the retry interval to avoid
66+
reaping sessions during normal SSE polling gaps.
67+
Default is None (no timeout). A value of 1800
68+
(30 minutes) is recommended for most deployments.
6869
"""
6970

7071
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
@@ -333,3 +333,65 @@ async def handle_list_tools(ctx: ServerRequestContext, params: PaginatedRequestP
333333
Client(streamable_http_client(f"http://{host}/mcp", http_client=http_client)) as client,
334334
):
335335
await client.list_tools()
336+
337+
338+
@pytest.mark.anyio
339+
async def test_idle_session_is_reaped():
340+
"""Session should be removed from _server_instances after idle timeout."""
341+
app = Server("test-idle-reap")
342+
manager = StreamableHTTPSessionManager(app=app, session_idle_timeout=300)
343+
344+
async with manager.run():
345+
sent_messages: list[Message] = []
346+
347+
async def mock_send(message: Message):
348+
sent_messages.append(message)
349+
350+
scope = {
351+
"type": "http",
352+
"method": "POST",
353+
"path": "/mcp",
354+
"headers": [(b"content-type", b"application/json")],
355+
}
356+
357+
async def mock_receive(): # pragma: no cover
358+
return {"type": "http.request", "body": b"", "more_body": False}
359+
360+
await manager.handle_request(scope, mock_receive, mock_send)
361+
362+
session_id = None
363+
for msg in sent_messages: # pragma: no branch
364+
if msg["type"] == "http.response.start": # pragma: no branch
365+
for header_name, header_value in msg.get("headers", []): # pragma: no branch
366+
if header_name.decode().lower() == MCP_SESSION_ID_HEADER.lower():
367+
session_id = header_value.decode()
368+
break
369+
if session_id: # pragma: no branch
370+
break
371+
372+
assert session_id is not None, "Session ID not found in response headers"
373+
assert session_id in manager._server_instances
374+
375+
# Force the idle deadline to expire now
376+
transport = manager._server_instances[session_id]
377+
assert transport.idle_scope is not None
378+
transport.idle_scope.deadline = anyio.current_time()
379+
380+
with anyio.fail_after(5):
381+
while session_id in manager._server_instances:
382+
await anyio.sleep(0)
383+
384+
assert session_id not in manager._server_instances
385+
386+
387+
@pytest.mark.parametrize(
388+
"kwargs,match",
389+
[
390+
({"session_idle_timeout": -1}, "positive number"),
391+
({"session_idle_timeout": 0}, "positive number"),
392+
({"session_idle_timeout": 30, "stateless": True}, "not supported in stateless"),
393+
],
394+
)
395+
def test_session_idle_timeout_validation(kwargs: dict[str, Any], match: str):
396+
with pytest.raises(ValueError, match=match):
397+
StreamableHTTPSessionManager(app=Server("test"), **kwargs)

0 commit comments

Comments
 (0)