fix(mcp): share one production session across HTTP MCP and WebSocket (from #1169) - #1182
cursor[bot] wants to merge 8 commits into
Conversation
Pull latest contributor tip (77a9915 session ownership / attach locks) and keep the Bugbot medium fix converting ws://|wss:// base URLs for /mcp. Co-authored-by: mugenkyou <sachinskyte@gmail.com> Co-authored-by: burtenshaw <burtenshaw@users.noreply.github.com>
Bugbot high on #1169: openenv/session/close was rejected while /ws was attached, so production close cleared the local id and leaked the server session. Disconnect first; make HTTP session/close authoritative if a socket is still attached. Co-authored-by: mugenkyou <sachinskyte@gmail.com>
Sync production `_close_async` nesting from fork tip 41cb4e5 (disconnect before session/close; keep provider teardown in finally). Server-side authoritative HTTP close from prior commit remains for attach races. Co-authored-by: mugenkyou <sachinskyte@gmail.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
Sync on-repo #1182 with contributor tip 715d5af: HTTP session/close while attached marks closing and destroys on detach (pending_closes), instead of force-destroying under an active WebSocket. Add disconnect- before-close unit coverage. Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
Catch BaseException on production connect so CancelledError after HTTP session create still tears down capacity. Reject WebSocket attach when session is in _session_pending_closes to close the reattach/destroy race. Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Head d7c9a7e0 adds cancel-safe production connect (except BaseException + shielded close) and refuses /ws attach while _session_pending_closes (Bugbot medium). Prefer this over merging leaky #1169 tip 715d5af6; Co-authored-by path for mugenkyou’s shared-session work. Watching CI.
Sent by Cursor Automation: Release
burtenshaw
added
bug
Cursor
| # after session create so capacity is not leaked. | ||
| try: | ||
| await asyncio.shield(self.close()) | ||
| except Exception: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d7c9a7e. Configure here.
| self._update_session_activity(session_id) | ||
| else: | ||
| session_id, session_env = await self._create_session() | ||
| owns_session = True |
There was a problem hiding this comment.
Owned WebSockets skip exclusive attach tracking
Medium Severity
Plain /ws connections create a session but never join _session_websocket_attachments. The new attach guard only rejects IDs already in that set, so a second socket with session_id can share a WebSocket-owned environment. When the owner disconnects, _destroy_session still runs and tears that environment down under the attached client.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d7c9a7e. Configure here.
| query="", | ||
| fragment="", | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Schemeless base URLs break MCP requests
Low Severity
_production_mcp_url now builds the /mcp endpoint from raw _base_url instead of the scheme-normalized _ws_url. A schemeless host such as localhost:8000 is parsed as a URL scheme, so tool and session calls go to an invalid URL while the WebSocket still connects via convert_to_ws_url.
Reviewed by Cursor Bugbot for commit d7c9a7e. Configure here.
There was a problem hiding this comment.
Alignment Review Report
fix(mcp): share one production session across HTTP MCP and WebSocket — production MCP clients now create one HTTP /mcp session and attach the /ws WebSocket to it via ?session_id=, so reset/step/state (WebSocket) and list_tools/call_tool (HTTP MCP) operate on the same server-side environment instance instead of two divergent ones.
Automated Checks
- Lint: PASS for this PR. All four changed files pass
ruff format --check,ruff check, andusort checkstandalone..claude/hooks/lint.shexits non-zero, but only from pre-existing, repo-wide drift unrelated to this diff: the newerruffreformats ~57 Markdown/docstring code fences across the repo, plus the two long-standingusortfiles (tests/envs/test_grid_world.py,tests/envs/test_julia_env.py, documented inAGENTS.md). None of this PR's files appear in the hook's reformat list, and CI lints onlysrc/+tests/(notenvs/). - Debug code: CLEAN for this PR.
check-debug.shonly flagsprint(...)inside docstring examples inmcp_client.py(pre-existing documentation), no breakpoints/pdb/TODOs.
Verification Performed
tests/core/full suite: 626 passed, 2 skipped — no regressions.test_production_mode_routes.py+test_mode_selection.py: 105 passed; MCP session-persistence + the new attach/detach lifecycle tests: 15 passed (fastmcp 3.4.7 on this base — no fastmcp-4.x session regression here).- Traced the new concurrency paths (attach guard, pending-close race, cancel-safe connect, detach-before-close ordering). The design is sound: the pending-close set correctly handles the WS-detach vs
openenv/session/closerace in either interleaving, and_run_mcp_client_operationreuses the connected client (managed_session_id and is_connected()) so HTTP tool calls don't re-open the shared MCP session.
Open RFCs Context
- RFC 003 – MCP Support (In Review, @Darktex, @pankit-eng) governs this area (documents production inference via
POST /mcp). This change is consistent with it, but adds a session-attachment protocol (?session_id=on/ws, plusopenenv/session/create|closepending-close semantics) not spelled out in the RFC. - RFC 002 – Env Spec (In Review): WebSocket step loop — unaffected in spirit.
Tier 1: Fixes Required
None.
Tier 2: Alignment Discussion
Principle conflicts: None — the change reinforces the invariants:
- Dual API boundary is preserved and made consistent:
/wsstill carries orchestration,/mcpcarries agent tools, now over one shared env session instead of two silently-divergent ones. - Agents cannot reset: the
session_idattach is an infrastructure mechanism on/ws; it is not exposed through MCP tools, so agents gain no reset/simulation control. - Client–server separation: no new cross-imports; the client only builds a URL and passes
session_id.
RFC / robustness discussion (non-blocking):
ALIGNMENT FLAG: Production MCP session lifecycle now spans /mcp + /ws
- Principle/RFC at stake: RFC 003 (MCP Support, In Review); INVARIANTS "Dual API boundary" / "Communication patterns" (the HTTP-deprecation note)
- The concern: The HTTP
/mcpsession is now the owner of the shared env and/wsattaches to it. That's a reasonable refinement of today's transitional model, but it deepens the coupling to HTTP/mcpwhile INVARIANTS notes HTTP is slated for eventual WebSocket-only deprecation. Please confirm the attachment protocol matches the intended production MCP direction. - Suggested reviewer: @Darktex, @pankit-eng (RFC 003 authors); also loop in the author of
mcp_environment.py::mcp_session()and the MCP-session-persistence tests (git blame:swappy).
Two concrete, non-blocking robustness notes are left inline (crash-time reclamation of the shared session; a possibly-redundant mcp_session re-entry on the owned path).
Summary
- 0 mechanical issues to fix
- 1 alignment point for human review (production MCP session lifecycle / RFC 003 awareness)
- 0 hard RFC conflicts — the change is consistent with RFC 003; flagged for author awareness
Overall this is a careful, well-tested change (cancel-safe connect, race-safe teardown, thorough new tests). Nothing blocking.
Sent by Cursor Automation: Pre-review
| await websocket.send_text(error_resp.model_dump_json()) | ||
| finally: | ||
| if session_id: | ||
| if attached_session and session_id: |
There was a problem hiding this comment.
Non-blocking robustness note. On an abnormal WebSocket drop (e.g. a crashed production client), this branch detaches (discard) but does not destroy the session — a crashed client never sent openenv/session/close, so the session isn't in _session_pending_closes. The shared, HTTP-owned env then stays alive and holds a slot against MAX_CONCURRENT_ENVS, reclaimable only by the idle reaper, which is a no-op when session_timeout is None (the default ConcurrencyConfig).
This matches the pre-existing persistent-HTTP-/mcp-session behavior, but note the behavior change vs. before this PR: a production /ws drop used to reclaim its own dedicated session immediately; now the attached /ws no longer owns the env, so a crash leaks the slot until server restart unless an idle session_timeout is configured. Worth confirming that's acceptable for the intended deployments (or documenting that production servers should set a session_timeout).
| async with AsyncExitStack() as stack: | ||
| mcp_session_factory = getattr(session_env, "mcp_session", None) | ||
| if callable(mcp_session_factory): | ||
| if owns_session and callable(mcp_session_factory): |
There was a problem hiding this comment.
The owns_session and gate is correct — it avoids re-entering mcp_session on the attach path, since the HTTP session's _create_session() already holds it open for the session's lifetime.
Follow-up (pre-existing, non-blocking): for the owns_session path, _create_session() also enters mcp_session and stores it in _session_stacks[session_id] (since #432), so this handler-level re-entry now opens a second concurrent mcp_session context on the same env. Since _create_session now owns that lifecycle for both owned and attached sessions, this re-entry looks redundant — consider dropping it so owned sessions don't double-enter mcp_session.
2 participants
Summary
Completes contributor #1169 lifecycle work on top of already-merged #1175: production mode creates one HTTP MCP session, attaches
/ws?session_id=…to that same env, and only destroys WebSocket-owned sessions on disconnect. Fixes the dual-session capacity hole left on main after #1175.Fork #1169 is CONFLICTING and we cannot push the fork head (403); this on-repo PR preserves the contributor’s approach with
Co-authored-by: mugenkyou.Type of Change
Alignment Checklist
Before submitting, verify:
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violatedRFC Status
Test Plan
PYTHONPATH=src:envs uv run pytest tests/core/test_mode_selection.py tests/core/test_production_mode_routes.py -q→ 96 passed, 1 skippedsession_idon WS URL; sync/async close session cleanup;/wsattach preserves HTTP session; plain/wsstill destroys its own sessionClaude Code Review
N/A
Note
Medium Risk
Changes concurrent session lifecycle, idle reaping, and close ordering across HTTP and WebSocket; bugs could leak capacity or tear down sessions while still in use, but behavior is heavily tested.
Overview
Production MCP clients now use one server session for HTTP
tools/*and Gym-style/wstraffic, fixing the dual-session capacity leak after separate HTTP and WebSocket lifecycles.On the server,
/wscan attach to an existing HTTP session via?session_id=…(one WebSocket per session). Attached sessions are exempt from idle reaping;openenv/session/closereturns{closed: false, closing: true}until the socket disconnects, then destroys the env. WebSocket-only sessions still tear down on disconnect.MCPToolClientin production mode creates the HTTP session first, connects/wswith thatsession_id(preserving other query params), builds/mcpfrom the stablebase_url, and on close detaches the WebSocket beforeopenenv/session/close. Connect failures and cancellation run shielded cleanup so sessions are not leaked.Reviewed by Cursor Bugbot for commit d7c9a7e. Bugbot is set up for automated code reviews on this repo. Configure here.