Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
marvin-context-protocol
Bot
added
bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20dc98fccd
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
MCPConfigTransport kept the backend transports of its latest connection in a transport-wide list. Client.new() is a shallow copy, so a clone shared the transport: closing either client (even one that never connected) closed whatever list `_build_composite()` last wrote, tearing down the other client's live backends and making its next call fail with "Connection closed". This is the symmetric case of PrefectHQ#5618. Give each Client.new() clone an independent MCPConfigTransport over the same config, so connection-scoped backend transports belong to one client. A connected client's close still tears down its own backends, and the connection's exit stack continues to own normal teardown.
Cloning a config client by rebuilding MCPConfigTransport from its raw config recreated a single-server backend transport, and with it the OAuth provider and its in-memory token store. Tokens acquired by the source client were lost, so a fresh authorization flow could start on every proxy request that calls new(). It also flattened public MCPConfigTransport subclasses to the base class. Copy the transport instead: sharing config and the eagerly-built backend keeps OAuth and the concrete class, while fresh _transports, _resolved_legacy_only, and _has_connected state keep the clone disconnected and close() scoped to the connection the calling client opened. close() is now a no-op for a client that never connected, which also protects the shared single-server backend.
A single boolean marking "this transport attempted a connect" is not enough: single-server clones share the eagerly-built backend, so a clone that once connected (or merely entered a connect that failed or was cancelled) could still close the source client's live stdio session. Track ownership per backend instead. Participation is recorded only after a connect succeeds and is sticky so a client can still release its keep-alive backend after the context exits. A shared live-connection counter shows when another client is using a backend; each instance also tracks its own holds so direct teardown (the cancelled-connect fallback) can release a backend when no one else is using it. close() now requires participation and no other live user. Multi-server connections reset ownership per build; their exit stack already released the previous connection. Also document the single-server sharing semantics in Client.new() and use getattr for OAuth's bind-time token_storage_adapter in the test so the repository-wide isolated ty check passes. Tests cover the four delta-review cases: connected clone close after its context exits, reconnect-then-close, cancelled connect, and failed connect.
create_proxy(config) builds a fresh client per request through Client.new(), and a guard tool's multi-round-trip may span those clients. The sealing policy lives on MCPConfigTransport precisely so later clones can validate tokens minted by earlier ones; rebuilding the transport (an earlier revision) broke tests/server/test_mrtr_guards.py with "Invalid or expired requestState". copy_for_new_client() shallow-copies the transport and deliberately leaves _request_state_security shared. Add a regression test pinning that identity so a future ownership change cannot silently mint a new key.
langchain's MCPAdapter holds an MCPConfigTransport while its operations connect through Client.new() clones, then reads transport.legacy_only after the clone connected. Before clones got per-instance state both shared one object, so the source observed the resolution. The shallow-copy revision kept that state per instance and left the held transport reading False, failing the langchain protocol-era integration test. Record the copy source on each clone and mirror every era resolution (legacy, modern, and the disconnect reset) up the copy chain. Clones keep their own flag for their own reads; ownership and teardown state remain per-instance. Add a regression test with a mixed-era multi-server config that connects via a clone and asserts the source transport's legacy_only flips True while connected and back to False on disconnect.
BlueX888
force-pushed
the
fix/mcpconfig-close-clone-teardown
branch
from
October 26, 2026 13:45
733ce3e to
89e5010
Compare
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
Author
Contributor
Author
|
The Windows / Python 3.14 job failed in test_encrypted_oauth_reused_by_fresh_client[http-None] on a ResourceWarning (unclosed socket attributed to pydantic/json_schema.py), not an assertion about the client behavior. The same test passes locally on macOS/Python 3.12 (2 passed), and it predates this branch. It looks like a GC-timing flake in the warnings-catch assertion; a rerun of that job would confirm. I cannot rerun jobs without write access. |
Contributor
Author
|
Pushed f324332: the encrypted OAuth reuse test now keeps and closes the three clients it creates instead of letting them reach GC inside the warnings-catch block. That should remove the Windows/Python 3.14 ResourceWarning flake. tests/cli/test_client_commands.py: 130 passed. |
Collaborator
|
Closing: Client.new() shares the transport by design, and a fix for #5824 should be a small guard in close(), not an ownership model. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5824
Description
Client.new()is a shallow copy, so a clone shared the original'sMCPConfigTransportinstance. That transport kept the backend transports of its most recent connection in a transport-wide_transportslist which_build_composite()overwrote on every connect. CallingClient.close()on either client — even one that never connected — therefore closed whatever list was last written, tearing down the other client's live backends; its next call failed withToolError: Error calling tool '...': Connection closed. This is the symmetric case of #5618/#5596: just as a clone connecting must not close the original's backends, an unconnected client closing must not either.This change:
MCPConfigTransport.copy_for_new_client()and uses it fromClient.new(). The shallow copy preserves configuration, the concrete transport subclass, the eagerly-built single-server backend (including anauth: "oauth"provider and its token store), and_request_state_security— required so MRTR request-state tokens minted by one per-request client validate in the next clientcreate_proxy(config)builds.connect_sessionrecords participation only after a successful connect and decrements on context exit;close()releases a participated backend only when no other client's live connection uses it (shared active-user count vs this instance's local holds), while never-connected, failed, or cancelled clients release nothing. This covers connected-close-after-exit, reconnect-then-close, cancelled connect, and failed connect._era_sourceand mirror_resolved_legacy_onlyup the copy chain, matching the pre-clone shared-object behavior that callers such as langchain'sMCPAdapterrely on, without sharing teardown state.Contribution type
Bug fix verification
Test paths:
tests/test_mcp_config.pyandtests/client/client/test_transport.py(regression selection: both close directions, connected-close ownership, single-server isolation, the four ownership cases — clone close after disconnect, stale clone close, cancelled clone connect, failed clone connect — OAuth/store preservation, subclass preservation, and request-state sharing).Before the fix, with two real stdio servers (
tests/client/minimal_stdio_server.py, offline):The four ownership cases failed against earlier revisions of this fix with
AssertionError: assert [<FastMCPTransport ...>] == []; the MRTR proxy suite failed on a rebuild-from-config revision withInvalid or expired requestState.After the fix, both clients keep using the same backend PIDs and all 13 selected tests pass:
Validation
uv run --frozen pytest -q tests/client/client/test_transport.py tests/test_mcp_config.py-> 76 passed, 11 skippeduv run --frozen pytest -q tests/server/test_mrtr_guards.py-> 47 passed (includesTestMultiServerConfigEraMirroring)uv run --frozen pytest -q tests/server/providers/proxy tests/client/client/test_client.py tests/client/client/test_session.py tests/client/client/test_mode_negotiation.py-> 314 passeduv run --frozen pytest -q tests/client-> 671 passed, 9 skipped, 1 xfailed; 2 pre-existing unrelated Windows log-encoding failures (TestLogFileUnicodeDecodeError ... byte 0xa8), reproduced on clean adf90b6uv run --isolated ty check-> All checks passeduv run --frozen ruff checkandruff format --checkon the changed files -> cleanAI assistance
Tool(s) used: Claude Code
How you used it: investigated from a verified reproduction, drafted the ownership model and regression tests, and revised it in response to review.
Checklist
uv run prek run --all-filesand all checks pass