feat: add idle timeout for StreamableHTTP sessions #1994
+315
−7
Conversation
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
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
Comment on lines
+59
to
+66
| session_idle_timeout: Optional idle timeout in seconds for stateful sessions. | ||
| If set, sessions that receive no HTTP requests for this | ||
| duration will be automatically terminated and removed. | ||
| When retry_interval is also configured, ensure the idle | ||
| timeout comfortably exceeds the retry interval to avoid | ||
| reaping sessions during normal SSE polling gaps. | ||
| Default is None (no timeout). A value of 1800 | ||
| (30 minutes) is recommended for most deployments. |
Member
There was a problem hiding this comment.
Fill 120 characters, and 4 padding from the above line, please.
Suggested change
| session_idle_timeout: Optional idle timeout in seconds for stateful sessions. | |
| If set, sessions that receive no HTTP requests for this | |
| duration will be automatically terminated and removed. | |
| When retry_interval is also configured, ensure the idle | |
| timeout comfortably exceeds the retry interval to avoid | |
| reaping sessions during normal SSE polling gaps. | |
| Default is None (no timeout). A value of 1800 | |
| (30 minutes) is recommended for most deployments. | |
| session_idle_timeout: Optional idle timeout in seconds for stateful sessions. | |
| if set, sessions that receive no HTTP requests for this | |
| duration will be automatically terminated and removed. | |
| When retry_interval is also configured, ensure the idle | |
| timeout comfortably exceeds the retry interval to avoid | |
| reaping sessions during normal SSE polling gaps. | |
| Default is None (no timeout). A value of 1800 | |
| (30 minutes) is recommended for most deployments. |
Kludex
reviewed
Feb 10, 2026
Member
There was a problem hiding this comment.
I was hoping for us to not have this issue based folder, and instead have a structured based on the feature set itself/project structure.
A lot of the tests that AI creates seems unnecessary.
Kludex
reviewed
Feb 10, 2026
| await manager.handle_request(_make_scope(), _mock_receive, _make_send(sent)) | ||
| session_id = _extract_session_id(sent) | ||
|
|
||
| await anyio.sleep(0.3) |
Member
There was a problem hiding this comment.
Please review your own test suite. And drop sleeps.
maxisbey
added
the
P2
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.retry_interval(SSE polling) is configured, the effective timeout is at leastretry_interval_seconds * 3to avoid reaping sessions that are simply between polling reconnections.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?
12 tests in
tests/issues/test_1283_idle_session_cleanup.py.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