Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 3.8k
feat: add idle timeout for StreamableHTTP sessions#1994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
2c877087345bd72c9e60cd5686df4191aa6File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -313,3 +313,80 @@ async def mock_receive(): | ||
| assert error_data["id"] == "server-error" | ||
| assert error_data["error"]["code"] == INVALID_REQUEST | ||
| assert error_data["error"]["message"] == "Session not found" | ||
| @pytest.mark.anyio | ||
| async def test_idle_session_is_reaped(): | ||
| """After idle timeout fires, the session returns 404.""" | ||
| app = Server("test-idle-reap") | ||
| manager = StreamableHTTPSessionManager(app=app, session_idle_timeout=0.05) | ||
| async with manager.run(): | ||
| sent_messages: list[Message] = [] | ||
| async def mock_send(message: Message): | ||
| sent_messages.append(message) | ||
| scope = { | ||
| "type": "http", | ||
| "method": "POST", | ||
| "path": "/mcp", | ||
| "headers": [(b"content-type", b"application/json")], | ||
| } | ||
| async def mock_receive(): # pragma: no cover | ||
| return {"type": "http.request", "body": b"", "more_body": False} | ||
| await manager.handle_request(scope, mock_receive, mock_send) | ||
| session_id = None | ||
| for msg in sent_messages: # pragma: no branch | ||
| if msg["type"] == "http.response.start": # pragma: no branch | ||
| for header_name, header_value in msg.get("headers", []): # pragma: no branch | ||
| if header_name.decode().lower() == MCP_SESSION_ID_HEADER.lower(): | ||
| session_id = header_value.decode() | ||
| break | ||
| if session_id: # pragma: no branch | ||
| break | ||
| assert session_id is not None, "Session ID not found in response headers" | ||
| # Wait for the 50ms idle timeout to fire and cleanup to complete | ||
| await anyio.sleep(0.1) | ||
| ||
| # Verify via public API: old session ID now returns 404 | ||
| response_messages: list[Message] = [] | ||
| async def capture_send(message: Message): | ||
| response_messages.append(message) | ||
| scope_with_session = { | ||
| "type": "http", | ||
| "method": "POST", | ||
| "path": "/mcp", | ||
| "headers": [ | ||
| (b"content-type", b"application/json"), | ||
| (b"mcp-session-id", session_id.encode()), | ||
| ], | ||
| } | ||
| await manager.handle_request(scope_with_session, mock_receive, capture_send) | ||
| response_start = next( | ||
| (msg for msg in response_messages if msg["type"] == "http.response.start"), | ||
| None, | ||
| ) | ||
| assert response_start is not None | ||
| assert response_start["status"] == 404 | ||
| def test_session_idle_timeout_rejects_non_positive(): | ||
| with pytest.raises(ValueError, match="positive number"): | ||
| StreamableHTTPSessionManager(app=Server("test"), session_idle_timeout=-1) | ||
| with pytest.raises(ValueError, match="positive number"): | ||
| StreamableHTTPSessionManager(app=Server("test"), session_idle_timeout=0) | ||
| def test_session_idle_timeout_rejects_stateless(): | ||
| with pytest.raises(RuntimeError, match="not supported in stateless"): | ||
| StreamableHTTPSessionManager(app=Server("test"), session_idle_timeout=30, stateless=True) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the response takes more than the deadline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good question - there's no logic to prevent the cleanup from happening, so the response wouldn't make it back before the Transport gets closed. It'd get a
ClosedResourceErrorand the response would be lost. I'd probably argue that's something for the server dev to calibrate for their use case if they have very long running requests? Default stays no timeout anyway.