Close the transport a rejected connect_to_server opened

[Kushal9889]

Fixes #3490

connect_to_server opens the transport before the server's components are validated, so a server rejected for a duplicate name leaves its connection running with no way to reach it.

Motivation and Context

_establish_session opens the transport, runs initialize, stores the session's stack in self._session_exit_stacks[session], and enters it into self._exit_stack. _aggregate_components then raises MCPError when a component name collides with one already in the group — and self._sessions[session] = component_names is on the line after that raise.

So the connection stays live while being unreachable: group.sessions reads self._sessions, which the rejected server never reached, and disconnect_from_server needs the ClientSession object that connect_to_server raised instead of returning. It was released only when the whole group tore down — one live child process (stdio) or initialized session (streamable HTTP) per rejection.

This is the case the docs treat as ordinary: docs/client/session-groups.md says two servers you don't control "will collide eventually", and tells the reader to run exactly this and see the MCPError. The same page says the error is "raised before anything from the second server is registered", which held for the three component dicts but not for the connection opened to read them.

The fix closes the transport in connect_to_server, which is the only caller that owns one. connect_with_session is deliberately untouched: the caller owns that session and still holds it, and per the docs "the group never closes a session it didn't open". Putting the cleanup in _aggregate_components instead would close caller-owned sessions and break that contract.

It mirrors the existing precedent in this same file — _establish_session already does except Exception: → aclose() → raise for a failure during setup. This extends the same handling to a failure during aggregation.

How Has This Been Tested?

test_client_session_group_connect_to_server_duplicate_closes_transport fails on main (assert closed → AssertionError) and passes with the change. The test registers the session's exit stack the way the real _establish_session does, so it observes the leak rather than a mock call.

The existing test_client_session_group_connect_to_server_duplicate_tool_raises_error mocks _establish_session and so never registers a stack — that is why it passes either way, and it now covers the pop(...) is None branch.

Locally against 9972c21a:

• pytest -n auto — 5968 passed, 10 skipped (Windows-only), 1 xfailed
• coverage report — 100.00%, 0 missed statements, 0 partial branches
• strict-no-cover — clean, no pragma added
• ruff format --check, ruff check, pyright — clean on both touched files
• uv lock --check, README snippet check — clean; no dependency or lockfile changes

Also checked by hand with two real stdio servers both exposing a search tool: before the change each rejected connect_to_server left one more live child process behind (1, 2, 3 …) while group.sessions stayed at 1; after it, none.

Breaking Changes

None. The MCPError and its message are unchanged, and callers that already catch it see the same exception — the connection behind it is simply closed now. Nothing that was reachable before becomes unreachable.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I am assigned to the linked issue (or it is labeled help wanted, or I'm a maintainer)
• I have disclosed any AI assistance and can explain the change in my own words
• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

I used AI assistance to narrow this down and to build the reproduction; I ran it myself and can walk through the code path.

I reported #3490 and would like to fix it; I am not assigned yet, so the intake gate will close this until a maintainer decides. Opening it now so the fix is on the table rather than to jump the queue — happy for it to sit closed, and I will push any changes as new commits rather than force-pushing so it can reopen cleanly.

Not a duplicate of #3384. That one is the KeyError from del self._session_exit_stacks[session] in the empty-server branch, reached through connect_with_session. This is the duplicate-name branch reached through connect_to_server, which raises MCPError by design — the defect was what stayed running afterwards. I read the three PRs opened for #3384 (#3386, #3419, #3428); each removes only that one block, so none of them changes this path and this does not conflict with whichever you take.

Four things a reviewer might reasonably ask, answered up front:

• Can this ever close a session the caller owns? No. _session_exit_stacks has exactly one writer in the whole source tree — _establish_session, on a ClientSession it constructed itself — so the pop can only ever find a transport the group opened. connect_with_session is byte-identical to main.
• Why except Exception and not BaseException? Cancellation is deliberately excluded. anyio.get_cancelled_exc_class() is CancelledError, a BaseException; closing the stack under an active cancellation would need a shielded scope, which is a larger change. This matches the existing cleanup in _establish_session.
• The closed stack stays registered in _exit_stack — is that a double close? It is a no-op. AsyncExitStack has no unregister API, and re-closing a drained stack iterates an empty callback deque. disconnect_from_server has popped-and-closed this same way since it was written.
• Behaviour change for callers who catch MCPError and retry: the exception now arrives after transport teardown rather than before it, so there is bounded extra latency on stdio, and a rejected streamable-HTTP connection now terminates its session instead of leaving it registered. Both are the point of the change rather than side effects, but worth naming.

One nearby thing I deliberately did not touch: _aggregate_components un-tracks a component-less server's stack without closing it (the del self._session_exit_stacks[session] in its empty-server branch, further down the same call path rather than adjacent to this hunk). That is a sibling of this leak and is reachable from disconnect_from_server, and on the connect_with_session path it is the KeyError reported in #3384. It cannot interact with this fix — an empty component set makes every collision check vacuous, so the two paths are mutually exclusive — and expanding this diff into it would collide with whichever #3384 PR you take. Happy to follow up on it separately if useful.

No documentation change: docs/client/session-groups.md already states the intended behaviour ("raised before anything from the second server is registered"). This makes the code match it.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and you aren't currently assigned to #3490.

If a maintainer assigns you to #3490, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take.

You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way.

CONTRIBUTING.md has the full reasoning, but in short:

• We're a small team with very little capacity to review community PRs right now.
• Many recent PRs are AI-generated with little human review, and reviewing one carefully still costs a maintainer as much time as it ever did. A well-described issue is usually more useful to us than the code.

Maintainers: reopen, remove missing-issue-link, or add bypass-issue-check to override.