fix(mcp): make streamable-http session mode configurable via MCP_STATELESS_HTTP - #42814
Conversation
…disconnects Stateless mode tears down a request's transport as soon as that single HTTP round trip finishes, while the tool call it started keeps running as a background task. Tools that send more than one progress notification per call (generate_chart, get_chart_data, etc.) hit a ClosedResourceError on the second notification, crashing that session and disconnecting other concurrent clients on the same worker -- this is the direct cause of the mid-session MCP disconnects seen in production. Stateful sessions keep the transport alive for the session's lifetime, so this no longer races. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dosubot
Bot
added
infra
|
Bito Review Skipped - Source Branch Not Found |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42814 +/- ##
==========================================
- Coverage 65.59% 65.49% -0.10%
==========================================
Files 2819 2822 +3
Lines 160166 160923 +757
Branches 36569 36682 +113
==========================================
+ Hits 105059 105396 +337
- Misses 53059 53435 +376
- Partials 2048 2092 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Config plumbing is correct. Both read sites — run_server() (server.py:1038) and the CLI entrypoint (__main__.py:175) — resolve via flask_app.config.get("MCP_STATELESS_HTTP", MCP_STATELESS_HTTP), passing the module constant itself as the .get() fallback rather than a re-hardcoded literal. That's the single-source form: flipping the constant in mcp_config.py moves every reader with it, so there's no drifting second default. The value comes from Flask config (a real bool), not an env var, so there's no bool("False") string-parsing hazard.
Default preserves production behavior: MCP_STATELESS_HTTP = True matches the pre-PR hardcoded stateless_http=True at all three call sites (the two here plus the multi-pod http_app path), so upgrades are behavior-preserving and stateful mode (False) is strictly opt-in. Both readers actually feed the resolved value into mcp.run/http_app — not a defined-but-unread no-op.
Isolation is unaffected: the default is unchanged and user identity is resolved per HTTP request from the JWT context; the stateful path only keeps the streamable-HTTP transport alive per Mcp-Session-Id (with the documented session-affinity requirement) and does not share DB session or identity across tool calls, so no cross-request leak is introduced.
Two nits, neither blocking:
-
Test doesn't pin the new wiring.
test_mcp_e2e_smoke.pyhardcodesstateless_http=Falsedirectly in_real_asgi_clientand its docstring notes it does so "rather than readingMCP_STATELESS_HTTP's True default." So it never exercises theflask_app.config.get(...)resolution inrun_server()/__main__, never asserts the True default, and covers only the False mode — reverting the production.get()wiring would leave this test green. A flipped default or broken resolution wouldn't be caught by this change. -
MCP_STATELESS_HTTPisn't inget_mcp_config()'sdefaultsallow-list (mcp_config.py:704) the wayMCP_RBAC_ENABLED/MCP_DEBUGare. It still works — the reads fall back to the module constant andconfig.update()won't clobber an operator'ssuperset_config.pyoverride — but it's inconsistent with the module's other operator-facing flags.
Add MCP_STATELESS_HTTP to get_mcp_config()'s defaults allow-list so an operator override in superset_config.py is actually surfaced via flask_app.config, consistent with MCP_DEBUG/MCP_RBAC_ENABLED. Add tests pinning run_server()'s True default and its False override, since the existing e2e smoke test hardcodes stateless_http=False and never exercises that resolution. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code Review Agent Run #af1089Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
Adds
MCP_STATELESS_HTTP(default True, matching current behavior) to make FastMCP's streamable-HTTP session mode configurable instead of hardcoded. In stateless mode, each request's transport is torn down as soon as that single HTTP round trip finishes while the tool call keeps running in the background; if a client gives up on a still-running call and the tool tries to send another progress notification, it hits the now-closed transport and crashes the session, disconnecting other concurrent clients on the same worker. Setting this to False keeps the session (and its transport) alive for the session's lifetime, fixing the crash — confirmed via a live A/B repro where the identical client-disconnect trigger crashed under True and completed cleanly under False.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION