Expire idle Streamable HTTP sessions by default and cap concurrent sessions - #3395
Expire idle Streamable HTTP sessions by default and cap concurrent sessions#3395maxisbey wants to merge 5 commits into
Conversation
📚 Documentation preview
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also examined two candidate issues and ruled them out: holding _session_creation_lock while serving the entire opening request (including reading the POST body) serializes concurrent session opens, but that scope is pre-existing behavior, not widened by this PR; and the new docs/migration.md section is a deliberate record of the changed idle-timeout/max-sessions defaults rather than a stray addition.
Extended reasoning...
The inline findings cover the cancellation-window leak around task_group.start(run_server) (src/mcp/server/streamable_http_manager.py:354), the idle-expiry race in src/mcp/server/streamable_http.py:490, and the stateless-mode missing try/finally around terminate(). Separately, I checked whether serving the opening request inside _session_creation_lock (the _send_and_report_status call at src/mcp/server/streamable_http_manager.py:362, which awaits the request body) was a regression — the old code already called handle_request inside the same lock, so this PR does not widen that critical section, and the docs/migration.md addition documents this PR's own default changes, which is the kind of entry a behavior-changing PR legitimately adds. Neither warranted a posted finding; recording them here so a human reviewer knows they were looked at.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/mcp/server/streamable_http_manager.py— pre-existing:_handle_stateless_requeststill runshandle_requestthenterminate()with no try/finally, so when the ASGI task is cancelled (client disconnects mid-request — the exact trigger the PR handles for stateful opens)terminate()is skipped andrun_stateless_serverblocks forever inserve_connectionon a read stream onlyterminate()closes, leaking one task+transport per aborted request in the manager's task group until shutdown, exactly as the base does. The PR added this hardening only to the stateful path (try/finally + shieldedterminate(), lines 360-370). Mirror it here: wrap lines 256-259 in try/finally and callterminate()underanyio.CancelScope(shield=True).Extended reasoning...
Path: stateless mode,
_handle_stateless_request(src/mcp/server/streamable_http_manager.py:253-259). It startsrun_stateless_serverin the manager's long-livedself._task_group; that task sits inserve_connectionreading the transport's read stream, which is closed only byhttp_transport.terminate()(streamable_http.py:839-865) —connect()'s own finally can't help because it runs only afterserve_connectionreturns (circular). Trigger: the client disconnects while its POST is being served and the ASGI server cancels the request task (the PR's own testtest_opening_request_that_is_cancelled_leaves_no_sessionmodels exactly this for the stateful path).await http_transport.handle_request(scope, receive, send)at line 256 raises CancelledError, so line 259'sterminate()never runs; the read stream stays open andrun_stateless_serverblocks forever. No safeguard applies: the new idle timeout is not wired in stateless mode (transport is built withoutidle_timeoutat lines 211-216, soidle_scopeis None), andmax_sessionsonly gates the stateful branch — so leakVerification: pre-existing — The mechanism is real, and the diff leaves the stateless path with exactly the gap it just closed on the stateful path. Code path (src/mcp/server/streamable_http_manager.py:253-259, byte-identical to base commit 6705402 —
git diffhas no hunk in_handle_stateless_request): ```python await self._task_group.start(run_stateless_server) # Handle the HTTP request and return the respo
The session manager kept a session's registry entry after the client
ended it with DELETE (the per-session task's cleanup skipped terminated
transports), and a request without a session ID that was refused
(anything but a valid initialize: wrong Accept, malformed JSON, a
non-initialize message, GET/DELETE) still left a registered transport
with a running server task behind it.
Now the manager drops the entry as soon as the transport is terminated,
the per-session task forgets the session and terminates its transport
however the loop ended, and a provisional session whose opening request
was answered with an error is discarded before the request returns. A
follow-up request on a deleted session is answered by the manager
("Session not found", 404) rather than by the dead transport.
`session_idle_timeout` was opt-in (default None), so at stock settings a stateful session that its client never deleted stayed registered, with its server task and streams, until the process exited. The docstring already recommended 1800 seconds; make that the default (DEFAULT_SESSION_IDLE_TIMEOUT) so sessions nobody is using are reclaimed after 30 minutes. `None` keeps the previous behaviour. "Idle" is now measured from the moment the session's last in-flight request completes rather than from the arrival of the last request: the transport takes an `idle_timeout` and owns the countdown, holding it while any request (an open GET stream included) is being served and restarting it when the last one finishes. A connected client, or a call that runs longer than the timeout, therefore never loses its session; a client that goes quiet with no stream open gets 404 on its next request and initializes again, as the spec describes. The timeout is simply unused in stateless mode, which keeps no sessions, so constructing a stateless manager with a timeout no longer raises.
c60175b to
30c3971
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/server/streamable_http.py">
<violation number="1" location="src/mcp/server/streamable_http.py:206">
P2: When `idle_timeout` is `NaN`, the constructor accepts it and the idle scope receives a `NaN` deadline, so session expiration becomes undefined instead of failing fast. Reject non-finite timeout values alongside non-positive values.</violation>
<violation number="2" location="src/mcp/server/streamable_http.py:484">
P2: There is a race window between the idle deadline firing (idle_scope.cancel_called becomes True) and run_server's finally actually calling terminate(). A request that arrives in that window increments _requests_in_flight and sets `self.idle_scope.deadline = math.inf`, but anyio's deadline setter is a no-op once cancel_called is set, so the session cannot be revived. Since `_terminated` is still False, the request is dispatched into a session whose loop is already unwinding, and `writer.send()` blocks until terminate() eventually runs — producing a 500 (JSON mode) or a silently-ended SSE stream instead of the intended clean 404 for an expired session. Guard handle_request against `idle_scope.cancel_called` (or check `_terminated`/loop status) before dispatching, or have this in-flight bump made atomic with the cancellation check.</violation>
</file>
<file name="tests/server/test_streamable_http_manager.py">
<violation number="1" location="tests/server/test_streamable_http_manager.py:638">
P3: These refusal/cancel/failure tests verify only that the session registry empties (`_server_instances == {}`, `_session_owners == {}`), but the PR's claim is that no transport is left behind either. The manager registers the provisional transport and starts its `run_server` task before serving the request, then forgets+terminates it in a finally; if `terminate()` were dropped, the background task would keep running while the dicts are already empty, and all three tests would still pass. Capture the created transport(s) (as `test_stateless_requests_memory_cleanup` does) and assert `transport.is_terminated` after the refused/cancelled/failed open.</violation>
</file>
<file name="src/mcp/server/streamable_http_manager.py">
<violation number="1" location="src/mcp/server/streamable_http_manager.py:349">
P2: During manager shutdown, this `terminate()` await runs inside the cancelled task-group scope and can be interrupted before closing the transport streams. Shield the transport cleanup so session shutdown actually completes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… on the server factories Add `max_sessions` (DEFAULT_MAX_SESSIONS = 10_000, `None` for no limit) to StreamableHTTPSessionManager: while that many stateful sessions are open, a request that would open another is answered 503 with a JSON-RPC error body and nothing is allocated; existing sessions are untouched and room frees up as they end or expire. This matches the Ruby SDK's defaults (the C# SDK uses the same 10 000 figure). `session_idle_timeout` and `max_sessions` are accepted by `Server.streamable_http_app()`, `MCPServer.streamable_http_app()`, `run_streamable_http_async()` and `run(transport="streamable-http")`, the same way `max_request_body_size` is, so applications can tune or disable them without reaching into `session_manager` after the fact. Docs: run/index.md options list, run/legacy-clients.md session cost, troubleshooting.md.
30c3971 to
ae2daca
Compare
There was a problem hiding this comment.
Beyond the inline comment, I also looked at whether holding _session_creation_lock across the opening request's entire handle_request (src/mcp/server/streamable_http_manager.py:292-370) lets one slow initialize stall all new-session creation, now including the 503 capacity check — the lock already spanned handle_request before this change, so it is pre-existing behavior rather than something this PR introduces.
Extended reasoning...
This run confirmed one inline finding (the 503 "Too many open sessions" response reusing _error_response's hardcoded INVALID_REQUEST JSON-RPC code) and separately investigated whether the session-creation lock being held across the whole opening request — including the awaited handle_request at src/mcp/server/streamable_http_manager.py:362 — is a serialization bottleneck introduced by this PR. Comparing against the base commit shows the pre-PR code held the same lock across the opening request's handle_request as well, so the behavior predates this change and was ruled out as a finding rather than silently dropped. Earlier review threads from prior runs (idle-expiry race, registration-before-start() cleanup, migration.md scope) remain visible on the PR; the migration.md edits are gone from the current revision, and the remaining threads have author replies whose substance should be judged in those threads rather than restated here.
Review follow-ups. The session manager now discards a session -- forgets it and terminates its transport, shielded from cancellation -- through one helper, used when the client deletes the session, when the session task ends, and when the request that opens the session is refused, fails or is cancelled; that bracket now starts when the session is registered, so a session whose task cannot be started is discarded as well. A stateless transport is terminated even when its request is cancelled, so the per-request task always ends. A transport whose idle period has run out answers as terminated instead of dispatching into a message loop that is shutting down. Idle timeouts must be finite as well as positive. Tests assert that discarded sessions' transports are terminated, cover the two new cleanup paths and the expired-transport answer, and the idle-hold tests check the suspended countdown directly and arm the short timeout only once the holding request is in flight instead of racing a timer.
There was a problem hiding this comment.
Beyond the inline finding, I also examined whether the new max_sessions cap creates a session-slot exhaustion vector (cheap unauthenticated initialize POSTs filling all 10 000 slots so legitimate clients get 503) and ruled it out as a regression: sessions were previously unbounded, so the cap plus the idle timeout bounds a pre-existing exposure rather than introducing a new one, and both knobs are tunable/disable-able.
Extended reasoning...
One confirmed finding (the manager-wide session-creation lock held while serving the entire opening request) is posted inline, so approval is not on the table. This note records the one additional concern investigated and ruled out this run: the max_sessions cap as a globally exhaustible resource. Filling the cap is cheap in principle, but before this PR the same traffic grew the session table without bound (a strictly worse outcome), slots are reclaimed as sessions end or idle out (default 30 minutes), and deployments can raise the cap or pass None — so the cap is a mitigation of existing exposure, not a new denial-of-service vulnerability introduced by this change. This is informational for the human reviewer, not a completeness guarantee; the prior runs' inline comments on the idle-expiry race, ghost-session registration, and the 503 error code stand on their own and are not restated here.
…ock for admission only Review follow-ups. The 503 for a request that would exceed `max_sessions` now carries JSON-RPC code -32603 like the subscription limit does, instead of -32600, which is what an unknown session ID answers with; a client that reconnects on the latter should not treat a full server the same way. The session-creation lock only needs to make the limit check and the registration atomic, so it is now held for that admission step alone and the opening request (whose body the request-size limit has already buffered) is served outside it. The new-session path is split into `_admit_session` and `_serve_opening_request` accordingly. A test pins that a client which never finishes sending its opening request does not hold up another client's session.
There was a problem hiding this comment.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/mcp/server/streamable_http.py— pre-existing: A GET with aLast-Event-IDheader on a transport without an event store sends no ASGI response at all:_handle_get_requestunconditionally routes the header to_replay_events, which returns immediately whenself._event_storeis None (line 936-937,# pragma: no cover) without sending anything, so the ASGI server raises "ASGI callable returned without sending a response" and the client sees a bare 500 — one stray header against any default (no-resumability) server triggers it, same as on the base branch. The new_send_and_report_statuseven has to special-case this no-response shape (status=None) for opening requests, but an existing-session GET still hits the raw hole. Fix: when no event store is configured, answer the request with a 4xx JSON-RPC error…Extended reasoning...
Path: client sends GET /mcp with
Accept: text/event-stream, a validMcp-Session-Id, and anyLast-Event-IDheader to a server run withoutevent_store(the default). Manager existing-session branch (streamable_http_manager.py:282) -> transport.handle_request -> _handle_request -> _handle_get_request (streamable_http.py:728): accept check passes, _validate_request_headers passes (session id matches), then line 754if last_event_id := request.headers.get(LAST_EVENT_ID_HEADER): await self._replay_events(...); return. _replay_events line 935-937:event_store = self._event_store; if not event_store: return # pragma: no cover— returns without ever calling send(), so nohttp.response.startis emitted. Uvicorn/Hypercorn then raise RuntimeError("ASGI callable returned without sending a response.") and emit a 500 with a server-side exception traceback; on the stateless path (mcp_session_id None, _validate_session returns True unconditionally) the same single header reaches the same hole. Safeguards don't help: the security middleware and accept/session validation all pass for…Verification: pre-existing — src/mcp/server/streamable_http.py:754-756 unconditionally does
if last_event_id := request.headers.get(LAST_EVENT_ID_HEADER): await self._replay_events(last_event_id, request, send); return, and_replay_eventsat lines 935-937 doesif not event_store: return # pragma: no coverwithout sending any ASGI response; no caller sends a fallback (_handle_requestline 522-523 just…
1 participant
Fixes #2455, fixes #3228, fixes #3300
Stateful Streamable HTTP sessions now have a lifecycle the server owns: a session is forgotten as soon as it ends, sessions with nothing in flight expire after
session_idle_timeout(default 30 minutes), and a manager holds at mostmax_sessions(default 10 000) at a time. Both settings are accepted bystreamable_http_app(),run_streamable_http_async()andrun(transport="streamable-http"), next tomax_request_body_size.Motivation and Context
session_idle_timeouthas existed onStreamableHTTPSessionManagersince 1.27 but defaulted toNoneand wasn't reachable fromMCPServerorServer.streamable_http_app()(#2455), so at stock settings a session whose client never sentDELETEstayed registered, with its server task and streams, until the process exited. The docstring already recommended 1800 seconds; this makes it the default, which is also what the Ruby SDK ships (the C# SDK measures idleness the same in-flight-aware way, with a longer window and a 10 000-session limit).Three related bookkeeping fixes ride along because the default only makes sense with them:
DELETEnow removes the session's entry immediately (the per-session cleanup used to skip terminated transports, Cleanly terminated streamable-HTTP sessions are never deregistered: the DELETE path skips its own cleanup #3300), and the per-session task forgets and terminates the session however its loop ended.initialize: wrongAccept, malformed JSON, a non-initialize message, GET/DELETE) no longer leaves a registered session and task behind (Rejected streamable-HTTP requests leave live sessions behind: the session is registered before the request is validated #3228).max_sessionslimits how many stateful sessions one manager holds at a time: while that many are open, a request that would open another gets503with a JSON-RPC error body (code-32603, as for the subscription limit); existing sessions are unaffected and room frees up as they end or expire. Nothing is evicted to make room. The session-creation lock is held only for that check and the registration; the opening request itself is served outside it.Stateless mode gets the matching cleanup: a per-request transport is terminated even when its request is cancelled (client went away), so the per-request task always ends.
How Has This Been Tested?
New tests in
tests/server/test_streamable_http_manager.py,tests/shared/test_streamable_http.py,tests/server/test_streamable_http_router.pyandtests/docs_src/: a deleted session is forgotten and its ID answers 404 "Session not found"; each refused opening-request shape leaves no session (and its transport is terminated), and neither does an opening request whose handler raises, is cancelled, or whose session task cannot start; a cancelled stateless request still terminates its transport; a transport whose idle period has run out answers 404; the defaults; atools/callparked past the timeout, an open GET stream, and a request completing under an open GET stream all keep the session, after which it expires and answers 404; sessionmax_sessions + 1gets 503 and a slot frees as soon as a session is deleted; non-positive or non-finite values are rejected; the factories forward both settings; the transport can be constructed outside an event loop and creates its idle scope onconnect(). The interaction test forDELETEasserts the new contract. Full suite (100 % coverage, strict-no-cover), pyright and ruff pass locally; also exercised end to end againststreamable_http_app()under uvicorn with the SDK client and raw HTTP.Breaking Changes
No signature loses anything and every new parameter has a default (keyword-only on the factories), but two defaults are now active where previously there was no limit:
session_idle_timeoutdefaults to1800instead ofNone. A stateful session with no request in flight for 30 minutes (no open GET stream, no running call) is terminated; the client's next request gets404and it has to initialize again, as the spec describes. Clients that keep the GET stream open (the SDK clients do) or have a call running are unaffected. Passsession_idle_timeout=Nonefor the previous behaviour.max_sessionsdefaults to10_000. Deployments that expect more than 10 000 concurrent stateful sessions in one process should raise it or passNone.Smaller observable differences: constructing a stateless manager with a timeout no longer raises (the value is unused there);
session_idle_timeoutmust be finite (None, notmath.inf, means never); a request on a deleted session is answered by the manager (404, "Session not found") rather than by the terminated transport;StreamableHTTPServerTransportaccepts an optionalidle_timeoutand createsidle_scopeitself when it is set.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
docs/run/index.md,docs/run/legacy-clients.mdand theSession not foundentry indocs/troubleshooting.mddescribe the defaults and how to turn them off. Stateless mode and the 2026-07-28 request path keep no sessions and are unaffected. Thanks to @shaun0927 (#2457) and @sainikhiljuluri (#3229) for the earlier PRs in this area, which this supersedes.AI Disclaimer