feat: add idle timeout for StreamableHTTP sessions#1994
Open
felixweinberger wants to merge 1 commit intov1.xfrom
Open
feat: add idle timeout for StreamableHTTP sessions#1994felixweinberger wants to merge 1 commit intov1.xfrom
felixweinberger wants to merge 1 commit intov1.xfrom
Conversation
Add session_idle_timeout parameter to StreamableHTTPSessionManager that enables automatic cleanup of idle sessions, fixing the memory leak described in issue #1283. Key design decisions: - Idle timeout logic lives in the session manager, not the transport. The manager already owns the session lifecycle, so this is the natural place for reaping. The transport stays unaware of timeout logic. - A background reaper task scans _server_instances periodically and terminates sessions that have been idle longer than the threshold. - When retry_interval (SSE polling) is configured, the effective timeout is at least retry_interval_seconds * 3 to avoid reaping sessions that are simply between polling reconnections. - terminate() on the transport is now idempotent (early return if already terminated), making it safe to call from both the reaper and explicit DELETE requests. Based on the approach from PR #1159 by @hopeful0, reworked to: - Move idle tracking to the session manager instead of the transport - Remove __aenter__/__aexit__ context manager and condition variables - Account for retry_interval in the idle threshold - Add comprehensive tests Github-Issue: #1283 Reported-by: hopeful0
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
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:
_server_instances, crash cleanup), so this is the natural place for reaping. The transport stays unaware of timeout logic._server_instancesperiodically and terminates sessions that have been idle longer than the threshold.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), making it safe to call from both the reaper and explicit DELETE requests.None(no timeout, fully backwards compatible). Docstring recommends 1800s (30 min) for most deployments.How Has This Been Tested?
6 new tests in
tests/issues/test_1283_idle_session_cleanup.py:terminate()idempotencyretry_intervalNoneAll 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 move idle tracking to the session manager, remove
__aenter__/__aexit__and condition variables from the transport, and account forretry_intervalin the idle threshold.Closes #1283