fix(mcp): single-session production /ws+/mcp (from #1169)

[cursor]

Summary

On-repo resolution of fork #1169 (mugenkyou) against current main after #1175.

Fork push/conflict resolution is 403 for the release bot, and #1169 is genuinely CONFLICTING with #1175’s dual-session connect. This PR lands the contributor’s intended fix: one shared server session for production mode.

Changes

1. Client (mcp_client.py) — create HTTP MCP session first, then connect /ws?session_id=… so Gym + tools share one env (mugenkyou).
2. Server (http_server.py) — /ws accepts session_id to attach; only destroys the session on disconnect when the WebSocket created it.
3. Tests — single-session connect, sync/async close cleanup, attach-vs-create disconnect behavior.
4. Keeps fix(mcp): production JSON-RPC routing with sync-safe teardown (from #1169) #1175’s sync-safe _close_async HTTP session teardown.

Credit

Co-authored-by mugenkyou (from #1169).

Please close #1169 as superseded after this merges (bot cannot close forks).

  

---

Note

Medium Risk
Changes session lifecycle and production connect ordering; incorrect attach/destroy logic could leak sessions or drop shared env state, but behavior is covered by new integration tests.

Overview
Production MCP clients now use one server-side environment session for both HTTP /mcp tool calls and Gym-style /ws reset/step/state, instead of opening separate sessions on connect.

On MCPToolClient, connect order changes: create the HTTP session via openenv/session/create, append session_id to the WebSocket URL, then open /ws. On the server, /ws accepts optional ?session_id= to attach to an existing session; disconnect only calls _destroy_session when the WebSocket created the session (owns_session), so tearing down /ws does not kill the HTTP MCP session.

Tests assert single-session connect (WS URL includes session_id), sync/async close still hits openenv/session/close, and attach-vs-create disconnect behavior on HTTPEnvServer.

^{Reviewed by Cursor Bugbot for commit 58b5500. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 58b5500. Configure here.}

[cursor]

Failed WS connect leaks MCP session

High Severity

Production connect now creates the HTTP MCP session first, then temporarily appends ?session_id= onto _ws_url before super()._connect_async(). If that WebSocket connect fails, the parent path calls close() while _ws_url still has the query string. _production_mcp_url() only strips a trailing /ws, so the session-close POST goes to a malformed URL, the error is swallowed, and _production_session_id is cleared. The outer cleanup then skips close, leaving the server session allocated. With default max_concurrent_envs=1 and no idle timeout, that leaked session can block the environment until restart.

Additional Locations (2)

• src/openenv/core/mcp_client.py#L167-L173
• src/openenv/core/mcp_client.py#L380-L391

 

^{Reviewed by Cursor Bugbot for commit 58b5500. Configure here.}

[cursor]

Alignment Review Report

Scope: fix(mcp): single-session production /ws+/mcp (from #1169) — 3 files (+230/-69): src/openenv/core/mcp_client.py, src/openenv/core/env_server/http_server.py, tests/core/test_mode_selection.py. On-repo adoption of @mugenkyou's single-session approach, built on merged #1175.

Automated Checks

• Lint: PASS for the files this PR changes. ruff format --check reports the 3 touched files as already formatted; ruff check src/ tests/ → All checks passed; usort check src/ tests/ flags only the two pre-existing files (tests/envs/test_grid_world.py, tests/envs/test_julia_env.py, documented in AGENTS.md) — neither is touched here. (A repo-wide ruff format --check shows unrelated envs/** candidates due to local ruff-version drift; none in this PR.)
• Debug code: CLEAN. No print/breakpoint/pdb/TODO added on changed lines (check-debug.sh hits are all pre-existing CLI files).
• Tests (ran locally, bonus): PASS. tests/core/test_mode_selection.py = 43/43 incl. all new cases; full tests/core/ = 617 passed, 2 skipped. The MCP session-persistence suite still passes, so shared-session behavior is intact.

Open RFCs Context

In Review: 000, 001, 002, 003 (MCP Support — most relevant), 005, 008. Draft: 010, 011. RFC 003 (authors @Darktex, @pankit-eng) governs the dual /step (Gym) + /mcp (agent tools) interfaces and, under “Scenario 3: I already have my own MCP client,” lists “No session management” as a current /mcp limitation to be added “when we implement standard Streamable HTTP transport.”

Tier 1: Fixes Required

None. No lint/debug/type/import/syntax/security issues in the diff. Correctness points verified:

• Connect ordering is safe: _start_provider_if_needed() is idempotent (calling it before super()._connect_async(), which calls it again, is fine); _ws_url is temporarily suffixed with session_id and restored in a finally, so /mcp URL derivation elsewhere stays correct, and connect-failure cleanup calls close() (covered by a test).
• The shared env’s mcp_session() being entered by both the HTTP _create_session stack and the /ws attach path is safe — mcp_session() is explicitly reentrant (FastMCP Client refcounts; transport closes only on the outermost exit). Teardown ordering is also safe: env.close() only nulls references, and the /ws handler captured the client object before entry.
• owns_session correctly scopes teardown so attaching /ws never destroys an HTTP-owned session (covered by the two new server-side tests).

Tier 2: Alignment Discussion

Principle / Invariant Conflicts

The Dual API boundary invariant is preserved — this PR does not expose reset/step/state via MCP; the agent channel (/mcp tools) is unchanged and /ws remains the orchestration channel. One point worth a maintainer’s confirmation:

ALIGNMENT FLAG: /ws?session_id= lets any caller attach Gym control to an existing session

• Principle/Invariant at stake: INVARIANTS.md → Security / Agent isolation (“The WebSocket interface for reset/step is for orchestration only”) and “Dual API boundary.”
• The concern: Previously /ws always minted a fresh, isolated session, so reaching /ws never granted control over a “real” session. Now a caller that reaches /ws and knows a valid session_id can attach and drive reset/step/state on the shared env that also serves /mcp tool calls; nothing binds a session to its creator. Under the intended topology this is fine (the orchestration client mints and holds the UUID session_id; the trained policy only sees /mcp tools and never the id). Please confirm the threat model — that session_ids are never exposed to the agent/policy and /ws stays unreachable by agent code.
• Suggested reviewer: @Darktex (author of the dual-API-boundary / agent-isolation invariants).

RFC Conflicts

ALIGNMENT FLAG: Session management over /mcp diverges from RFC 003’s stated plan

• RFC at stake: RFC 003 (MCP Support, In Review).
• The concern: RFC 003 currently documents “No session management” on /mcp, to be added “when we implement standard Streamable HTTP transport.” This PR builds on OpenEnv-custom session management over /mcp (openenv/session/create + openenv/session/close, introduced in merged #1175) and now shares one such session across /ws and /mcp via ?session_id=. That’s a reasonable, working design, but it implements session management ahead of / differently from the RFC’s Streamable-HTTP path. Recommend updating RFC 003 to describe the now-implemented openenv/session/* lifecycle (or reconciling the two) so the In-Review design matches reality.
• Suggested reviewers: @Darktex, @pankit-eng (RFC 003 authors); cc swappy (git-blame author of mcp_session() and the session-persistence tests) for the session-lifecycle behavior.

Summary

• 0 mechanical issues to fix (lint clean, no debug code, tests green).
• 1 alignment point for human review (/ws?session_id= attach vs. agent-isolation invariant — confirm threat model).
• 1 RFC conflict to discuss (RFC 003 “No session management” vs. the implemented openenv/session/* + shared-session design).

Overall a focused, well-tested fix that correctly makes production /ws + /mcp share one env session; the flags above are non-blocking design/documentation discussion points, not defects.

  

_{Sent by Cursor Automation: Pre-review}

[cursor]

Tier 2 (agent-isolation invariant) — non-blocking. /ws now attaches to an existing session whenever ?session_id= is supplied, so a caller that reaches /ws and knows a valid session_id can drive reset/step/state on the shared env (previously /ws always minted a fresh, isolated session). Nothing binds a session to its creator. Under the intended topology this is fine — the orchestration client holds the UUID id and the trained policy only sees /mcp tools — but per INVARIANTS.md (“the WebSocket interface for reset/step is for orchestration only”) please confirm session_ids are never exposed to the agent and /ws stays unreachable by agent code. cc @Darktex. (See review summary.)

[cursor]

Tier 2 (RFC 003) — non-blocking. Creating the HTTP MCP session here and then attaching /ws to it makes production /ws+/mcp share one server-side session via the custom openenv/session/create|close methods (from #1175). RFC 003 Scenario 3 still documents “No session management” on /mcp, to be added “when we implement standard Streamable HTTP transport.” Worth updating RFC 003 to describe the implemented openenv/session/* lifecycle (or reconciling the two). cc @Darktex, @pankit-eng; cc swappy (mcp_session / session-persistence author). (See review summary.)

[cursor]

Blocking assessment for exact head 58b55000 after independent ASGI/network probes. The earlier optimistic review missed three concrete lifecycle failures:

1. HTTP close can destroy an attached session. While /ws?session_id=… was active, an HTTP openenv/session/close returned closed: true; that same WebSocket then successfully executed another state request against its retained, already-cleaned environment reference. Track attachments and reject close, or explicitly terminate/invalidate the attached socket before destroying the environment.
2. Concurrent attachment is unrestricted. Two WebSockets using the same session ID were both accepted and both served state, so they can concurrently mutate one environment. Check-and-reserve the attachment atomically under _session_lock, reject a second attachment, and release the reservation in finally.
3. Connect/cancellation cleanup still leaks. Bugbot's failed-WebSocket-connect finding is valid: cleanup runs while _ws_url still contains the query string, builds a malformed /mcp URL, swallows the failure, and forgets the server session. CancelledError also bypasses except Exception; cancellation during connect or session close can leave the HTTP session/client/socket/provider alive.

Required before merge: stable MCP URL construction, cancel-safe connect/close teardown, attachment ownership/exclusion under the session lock, and real regressions for failed/cancelled connect, close while attached, and simultaneous attachments. RFC 003 and the agent/infrastructure capability boundary still require owner/security review.

I rechecked the audit's separate double-mcp_session() concern and am not treating it as a blocker: FastMCP's nested contexts are reference-counted, and a real tools/list call still succeeded after WebSocket detach.

  

_{Sent by Cursor Automation: Release}