Studio: persistent stdio MCP sessions so server state survives across tool calls - #7080
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces persistent stdio sessions for MCP servers to preserve server-side state across tool calls, managing connected clients on dedicated event-loop threads with idle reaping and cancellation support. The review feedback suggests two key improvements: first, avoiding head-of-line blocking by releasing the global lock during slow network operations like session connection and closure; second, making the session teardown path more robust by defensively accessing attributes to handle partially-constructed objects.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 024467b5ce
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
NilayYadav
changed the title
NilayYadav
changed the title
… tool calls call_tool_sync spawned a fresh stdio subprocess per tool call (keep_alive=False) and tore it down when the call returned, so any stateful MCP server lost its state between calls: with @playwright/mcp, browser_navigate opened the page in one subprocess and browser_take_screenshot ran in a brand-new one, screenshotting about:blank. Keep one connected client per (command, env) on a dedicated event-loop thread and reuse it across calls: - idle sessions are reaped after 5 minutes (in-flight calls excluded) and everything closes at exit, preserving the old design's no-orphans property - a dead subprocess is detected via is_connected() and retried once on a fresh session; tool-level errors leave the session alone - cancel and timeout semantics are unchanged, and a timed-out call does not tear the session down - updating a server's endpoint/env/enabled state or deleting it closes its live session - HTTP/SSE servers stay one-shot per call
024467b to
a1b67e6
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b873136a92
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0edede3129
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb04379390
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…re close, scope closes to url+env
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05588eeb09
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…efore caching, keep env secrets out of generation keys
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc16dfdbcc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a6767219d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a29e973f42
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…nect and call, hash urls in generation keys
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b2154cae4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| session = _checkout_stdio_session(key) | ||
| if session is not None: | ||
| return session |
There was a problem hiding this comment.
When a request has already read the old MCP server row and then an update/delete changes that row before close_stdio_sessions() has removed the cached entry, this cached-session fast path returns without calling config_check. In that race the tool can still run on a disabled, deleted, or repointed stdio server; fresh connects are rechecked later, but cache hits bypass that protection. Check the current row before returning a cached session, or make the close/remove step happen before stale callers can borrow it.
Useful? React with 👍 / 👎.
| for key in expired: | ||
| _discard_stdio_key_lock(key) | ||
| for session in sessions: | ||
| logger.info("Closing idle stdio MCP session: %s", session.url) |
There was a problem hiding this comment.
When a stdio MCP command embeds credentials in argv (for example npx server --token ...), the idle reaper writes the full command to INFO logs every time that session ages out. The same code already treats command URLs as potentially secret when hashing generation keys, so this log can persist deleted API keys or bearer tokens outside the MCP config; log a server id/digest or a redacted command instead.
Useful? React with 👍 / 👎.
…oping - Evict a stdio session on any transport-level (non-ToolError) call failure and do not replay it, so a mid-call subprocess crash can no longer poison the scope. Never gate liveness on Client.is_connected() (it only reports that a session object exists, not that the subprocess is alive); add a version-adaptive dead-transport probe that works on fastmcp 3.0.2 and newer. - Re-check closed/defunct/config and transport liveness after acquiring the call lock, and retire a session before releasing the lock, so a queued same-scope caller never reuses a session that another caller's timeout already retired. - Force a ProactorEventLoop on Windows so the stdio transport can always spawn subprocesses regardless of the active event-loop policy. - Scope stdio sessions per conversation: require thread_id to persist, and tag the fields so a session_id and a thread_id with the same value cannot collide. A session_id alone is project-wide, so it now falls back to a safe one-shot session instead of sharing browser/DB/REPL state across conversations. - Forward thread_id on the Anthropic Messages path. - Treat timeout=None as unlimited on connect and the key lock (was capped at 60s). - Bound the session cache (default 32, override via UNSLOTH_STUDIO_MAX_STDIO_MCP_SESSIONS) with LRU eviction of idle sessions. - Run config_check on cache hits, and log a redacted exe#digest label instead of the raw command so credentials in argv never reach the logs.
for more information, see https://pre-commit.ci
|
Pushed a commit that hardens the persistent stdio session lifecycle. The persistence design is the right call, this just closes the recovery, concurrency, and isolation gaps around it. Crash recovery (main one) Queued-borrower race Windows event loop Scope isolation Smaller items
Verification
No schema change, no migration, and no new dependency, so existing installs update safely, and all new parameters are optional so an older frontend just falls back to one-shot. |
|
Ran this fix end to end in the actual Studio chat UI with Playwright, before and after the PR, on GPU-less GitHub CI runners. A stateful stdio MCP
The raw tool-result cards are the oracle here (a small model can paraphrase the number in its prose). Before the PR every stdio tool call spawns and tears down a fresh subprocess, so a stateful stdio server (browser, DB, REPL) loses all state between calls. After the PR, calls that share a conversation reuse one persistent process, and a new conversation still gets a fresh isolated one. Both turns in a conversation carry one stable Cross-platform backend tests, the session suite plus a real-subprocess persistence and mid-call-crash test, pass fail-hard on ubuntu-latest, macos-14, and windows-latest, including the Windows ProactorEventLoop path and the |
… for HTTP servers Two fixes from review of the persistent stdio session lifecycle: - Re-enforce the session cap when a session goes idle. A concurrent burst of distinct-scope calls can overshoot the cap while every cached session is busy (insert-time eviction only reclaims idle sessions), and the overshoot used to persist until the 5-minute idle reaper. _release_stdio_session now trims the idle overshoot back within the cap, without ever evicting an in-flight call. - close_stdio_sessions() now no-ops for a specific non-stdio (HTTP/SSE) url. Those transports are never cached as stdio sessions, so calling it on every HTTP server update or delete used to accrue an unbounded close-generation entry. Both are covered by regression tests that fail before the change and pass after.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38dbf785a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ephemeral = not scope | ||
| if ephemeral: | ||
| scope = f"request-{uuid.uuid4().hex}" | ||
| key = _session_key(url, headers, scope) |
There was a problem hiding this comment.
When thread_id/scope is absent, this creates a unique scope but still sends the call through _get_stdio_session, which publishes the session into _stdio_sessions and runs LRU eviction before the tool executes. If the cache is full of idle persistent chat sessions, a one-shot request can evict an unrelated chat's browser/REPL session and then immediately drop its own ephemeral session in finally, so the other chat loses the state this feature is meant to preserve. Use a private uncached session for ephemeral calls, or at least skip LRU eviction/insertion for them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
One-shot calls (no thread_id) already run under a unique per-request scope (request-) and are dropped immediately after the single call, so they never share a process or state with a conversation. Holding one cache slot for the duration of that single call is intentional; stdio MCP is single-user loopback, so cache contention is not a concern.
| session.connect( | ||
| None if deadline is None else max(0.0, deadline - time.monotonic()), | ||
| cancel_event, | ||
| ) |
There was a problem hiding this comment.
This starts connecting the new stdio session before any cache-cap check, and connecting sessions are not counted in _stdio_sessions. A burst of first calls with distinct thread_id values can therefore spawn far more than _STDIO_MAX_SESSIONS subprocesses/loop threads while they are still starting; if those calls remain in flight, the later LRU eviction also skips them, so the configured cap does not protect against the resource-exhaustion case it was added for. Reserve/queue against the cap before creating and connecting the session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The cap is a soft cap by design: eviction only reclaims idle sessions and never kills an in-flight connect, so a burst can transiently overshoot rather than drop a live call. The overshoot is reclaimed as sessions go idle via the release-time trim, covered by test_stdio_cache_trims_overshoot_after_burst.
|
Did another pass over the stdio session lifecycle. Pushed two fixes in d745b79 and intentionally left the rest, with reasoning below. Fixed
Both have regression tests that fail before the change and pass after. Considered and left as is
Cross-platform backend tests remain green on ubuntu-latest, macos-14, and windows-latest. |
The edit dialog resends url, headers, and use_oauth unchanged whenever a server is saved, so gating the tool-cache invalidation and stdio session close on field presence dropped the persistent process on a plain rename or any no-op edit. Gate on a real value change against the stored row so only a genuine endpoint, auth, or enable change closes the session. Regression tests: a rename that resends unchanged url/headers/oauth keeps the session; a real command change still closes it.
for more information, see https://pre-commit.ci
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Collapse a few verbose comments to fewer lines with the wording preserved, and drop one that restated the clear_oauth_tokens_async docstring. Comments only; no code change.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
call_tool_syncspawned a fresh stdio subprocess for every MCP tool call (StdioTransport(keep_alive=False)) and tore it down when the call returned so any stateful stdio server lost its state between calls.With
@playwright/mcpthis means:browser_navigatespawns a server, opens the page, returns > subprocess andbrowser destroyed
browser_take_screenshotspawns a brand-new server > fresh browser atabout:blank> screenshots an empty pageThere's no error, the tool returns a perfectly valid result for a blank page, so it just looks like the feature is broken. The same applies to any server holding live state (DB sessions, REPLs, terminal servers). Only stateless servers (filesystem, fetch) worked.
Fix
Keep one connected client per (command, env, chat session) on a dedicated event-loop thread and reuse it across tool calls.
atexithook closes everything at shutdown.session_id, so one conversation's browser/DB state never leaks into another.is_connected()and the call retried once on a fresh session; tool-level errors leave the session up.npxstartup for one server doesn't block calls to any other./cancelinterrupts during connect and during the call;timeout=None(no limit) stays unlimited; a timed-out call does not tear the session down.