feat: add idle timeout for StreamableHTTP sessions#1994
Open
felixweinberger wants to merge 5 commits intov1.xfrom
Open
feat: add idle timeout for StreamableHTTP sessions#1994felixweinberger wants to merge 5 commits intov1.xfrom
felixweinberger wants to merge 5 commits intov1.xfrom
Conversation
Member
|
Can't the task manage itself? I don't think we want the overhead of a background task that checks other tasks. This seems something the specification should mention. |
felixweinberger
force-pushed
the
fweinberger/idle-timeout-termination
branch
from
debfd7d to
73f4cf2
Compare
felixweinberger
force-pushed
the
fweinberger/idle-timeout-termination
branch
7 times, most recently
from
f8b58ad to
3b38262
Compare
Add a session_idle_timeout parameter to StreamableHTTPSessionManager that enables automatic cleanup of idle sessions, fixing the memory leak described in #1283. Uses a CancelScope with a deadline inside each session's run_server task. When the deadline passes, app.run() is cancelled and the session cleans up. Incoming requests push the deadline forward. No background tasks needed — each session manages its own lifecycle. - terminate() on the transport is now idempotent - Effective timeout accounts for retry_interval - Default is None (no timeout, fully backwards compatible) Github-Issue: #1283
felixweinberger
force-pushed
the
fweinberger/idle-timeout-termination
branch
from
3b38262 to
2c87708
Compare
- Remove unrelated blank line between logger.info and try block - Only create CancelScope when session_idle_timeout is set, keeping the no-timeout path identical to the original code - Add docstring to _effective_idle_timeout explaining the 3x retry_interval multiplier rationale
felixweinberger
force-pushed
the
fweinberger/idle-timeout-termination
branch
from
3bdc230 to
7345bd7
Compare
The helper silently clamped the user's configured timeout to retry_interval * 3, which is surprising. Users should set a timeout that suits their deployment. Updated the docstring to note that the timeout should comfortably exceed retry_interval when both are set.
Contributor
Author
|
@Kludex another attempt, no external managers or anything this time. |
This was referenced Feb 9, 2026
Contributor
Author
|
Same thing for |
Kludex
reviewed
Feb 10, 2026
Kludex
reviewed
Feb 10, 2026
Kludex
reviewed
Feb 10, 2026
maxisbey
added
the
P2
felixweinberger
force-pushed
the
fweinberger/idle-timeout-termination
branch
from
6141a20 to
3469185
Compare
felixweinberger
commented
Feb 16, 2026
felixweinberger
force-pushed
the
fweinberger/idle-timeout-termination
branch
4 times, most recently
from
0aa0e6c to
8b91fb3
Compare
Kludex
reviewed
Feb 16, 2026
Kludex
reviewed
Feb 16, 2026
felixweinberger
force-pushed
the
fweinberger/idle-timeout-termination
branch
from
8b91fb3 to
619cdb3
Compare
Kludex
reviewed
Feb 16, 2026
- 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
felixweinberger
force-pushed
the
fweinberger/idle-timeout-termination
branch
from
619cdb3 to
d5686df
Compare
Kludex
reviewed
Feb 16, 2026
Kludex
reviewed
Feb 16, 2026
Comment on lines
+239
to
+241
| # Push back idle deadline on activity | ||
| if transport.idle_scope is not None and self.session_idle_timeout is not None: | ||
| transport.idle_scope.deadline = anyio.current_time() + self.session_idle_timeout |
Member
There was a problem hiding this comment.
What happens if the response takes more than the deadline?
Contributor
Author
There was a problem hiding this comment.
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 ClosedResourceError and 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.
Kludex
reviewed
Feb 16, 2026
| if idle_scope.cancelled_caught: | ||
| session_id = http_transport.mcp_session_id | ||
| logger.info(f"Session {session_id} idle timeout") | ||
| if session_id is not None: # pragma: no branch |
Member
There was a problem hiding this comment.
Can the session_id be None here?
Contributor
Author
There was a problem hiding this comment.
No - Line 253 already does
assert http_transport.mcp_session_id is not None
But because the type is str | None we have to do it again here - I replaced it with the same assert now though.
felixweinberger
force-pushed
the
fweinberger/idle-timeout-termination
branch
6 times, most recently
from
77f7e81 to
1292484
Compare
felixweinberger
commented
Feb 16, 2026
| 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) |
Contributor
Author
There was a problem hiding this comment.
After some back and forth trying out event based stuff, given we're implementing timeout behavior here, this actually seems like the right way to test this - a case where a anyio.sleep(0.1) is justified?
- Fix docstring indentation: use 4-space continuation indent filled to 120 cols consistently across all Args parameters - Change stateless+idle_timeout error from ValueError to RuntimeError - Use logger.exception() instead of logger.error() with exc_info - Remove unnecessary None guard on session_id in idle timeout cleanup - Replace while+sleep(0) polling with anyio.Event in test Github-Issue: #1283
felixweinberger
force-pushed
the
fweinberger/idle-timeout-termination
branch
from
1292484 to
4191aa6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a
session_idle_timeoutparameter toStreamableHTTPSessionManagerthat enables automatic cleanup of idle sessions, fixing the memory leak described in #1283.Motivation and Context
Sessions created via
StreamableHTTPSessionManagerpersist indefinitely in_server_instanceseven after clients disconnect without sending DELETE. Over time this leaks memory. This was reported in #1283 and originally addressed in #1159 by @hopeful0 — this PR reworks that approach.Key design decisions:
CancelScopewith a deadline inside each session'srun_servertask. When the deadline passes,app.run()is cancelled and the session cleans up. No background tasks needed — each session manages its own lifecycle.terminate()on the transport is now idempotent (early return if already terminated).None(no timeout, fully backwards compatible). Docstring recommends 1800s (30 min) for most deployments.How Has This Been Tested?
Tests added to
tests/server/test_streamable_http_manager.py: idle session reaping (deterministic, no sleeps), terminate idempotency, and parameterized input validation. All existing tests pass unchanged.Breaking Changes
None.
session_idle_timeoutdefaults toNone(no timeout).Types of changes
Checklist
Additional context
Based on the approach from PR #1159 by @hopeful0, reworked to use cancel scope deadlines instead of transport-level timeout logic.
Closes #1283