Skip to content

Commit 619cdb3

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 2c9e60c commit 619cdb3

File tree

4 files changed

+85
-275
lines changed

4 files changed

+85
-275
lines changed

CLAUDE.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ This document contains critical information about working with this codebase. Fo
2424
- Coverage: test edge cases and errors
2525
- New features require tests
2626
- Bug fixes require regression tests
27+
- NEVER use `anyio.sleep()` with a fixed duration as a synchronization mechanism. Instead:
28+
- Use `anyio.Event` — set it in the callback/handler, `await event.wait()` in the test
29+
- For stream messages, use `await stream.receive()` instead of `sleep()` + `receive_nowait()`
30+
- Wrap waits in `anyio.fail_after(5)` as a timeout guard
31+
- If a sleep seems unnecessary, just remove it
2732

2833
- For commits fixing bugs or adding features based on user reports add:
2934

src/mcp/server/streamable_http_manager.py

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

6970
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: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,3 +313,74 @@ 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+
"""Idle timeout sets a cancel scope deadline and reaps the session when it fires."""
321+
idle_timeout = 300
322+
app = Server("test-idle-reap")
323+
manager = StreamableHTTPSessionManager(app=app, session_idle_timeout=idle_timeout)
324+
325+
async with manager.run():
326+
sent_messages: list[Message] = []
327+
328+
async def mock_send(message: Message):
329+
sent_messages.append(message)
330+
331+
scope = {
332+
"type": "http",
333+
"method": "POST",
334+
"path": "/mcp",
335+
"headers": [(b"content-type", b"application/json")],
336+
}
337+
338+
async def mock_receive(): # pragma: no cover
339+
return {"type": "http.request", "body": b"", "more_body": False}
340+
341+
before = anyio.current_time()
342+
await manager.handle_request(scope, mock_receive, mock_send)
343+
344+
session_id = None
345+
for msg in sent_messages: # pragma: no branch
346+
if msg["type"] == "http.response.start": # pragma: no branch
347+
for header_name, header_value in msg.get("headers", []): # pragma: no branch
348+
if header_name.decode().lower() == MCP_SESSION_ID_HEADER.lower():
349+
session_id = header_value.decode()
350+
break
351+
if session_id: # pragma: no branch
352+
break
353+
354+
assert session_id is not None, "Session ID not found in response headers"
355+
assert session_id in manager._server_instances
356+
357+
# Verify the idle deadline was set correctly
358+
transport = manager._server_instances[session_id]
359+
assert transport.idle_scope is not None
360+
assert transport.idle_scope.deadline >= before + idle_timeout
361+
362+
# Simulate time passing by expiring the deadline
363+
transport.idle_scope.deadline = anyio.current_time()
364+
365+
with anyio.fail_after(5):
366+
while session_id in manager._server_instances:
367+
await anyio.sleep(0)
368+
369+
assert session_id not in manager._server_instances
370+
371+
# Verify terminate() is idempotent
372+
await transport.terminate()
373+
assert transport.is_terminated
374+
375+
376+
@pytest.mark.parametrize(
377+
"kwargs,match",
378+
[
379+
({"session_idle_timeout": -1}, "positive number"),
380+
({"session_idle_timeout": 0}, "positive number"),
381+
({"session_idle_timeout": 30, "stateless": True}, "not supported in stateless"),
382+
],
383+
)
384+
def test_session_idle_timeout_validation(kwargs: dict[str, Any], match: str):
385+
with pytest.raises(ValueError, match=match):
386+
StreamableHTTPSessionManager(app=Server("test"), **kwargs)

0 commit comments

Comments
 (0)