fix(mcp): drop cleaned-up servers from active_servers when keeping failed servers

[AAliKKhan]

Summary

MCPServerManager.cleanup_all() (and therefore async with exit) leaves cleaned-up servers listed in active_servers whenever drop_failed_servers=False. This completes the post-cleanup contract established by #4586 for the non-default flag path.

Problem

Repro on current main:

manager = MCPServerManager([server], drop_failed_servers=False)
await manager.connect_all()   # active_servers == [server], _connected_servers == {server}
await manager.cleanup_all()   # _connected_servers == set()
print(manager.active_servers) # still [server] - a disconnected server advertised as active

Callers following the documented pattern (agent.mcp_servers = manager.active_servers, including after cleanup within a lifespan) observe stale state: every server remains "active" even though the manager no longer owns any live connection.

Root cause

_cleanup_all() refreshes active_servers via _refresh_active_servers() in its guaranteed finally. With drop_failed_servers=True that derives the list from the connected set (emptying it), but the else branch returns all servers unconditionally - so the disconnect performed by _cleanup_server() (which always discards from _connected_servers) is never reflected.

Solution

Derive the post-cleanup active list from _connected_servers directly in _cleanup_all()'s finally. Behavior is unchanged for drop_failed_servers=True; for False, that flag continues to govern only connect-phase failures (the _connect_all() failure path is untouched), while servers whose lifecycle the manager has torn down are no longer advertised as active.

This matches #4586's stated intent ("consistently derives the active list from the canonical connected-server set") and the class docstring's guarantee that active_servers reflects successfully connected servers after connection attempts and cleanup.

Test plan

• New regression test: with drop_failed_servers=False, cleanup_all() empties active_servers and _connected_servers
• Full MCP suite passes (463 passed, 24 skipped) including all existing drop_failed_servers=False coverage
• Broader suite, ruff format/check, mypy src/agents, and pyright all clean

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ccf5738718

ℹ️ 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".

[chatgpt-codex-connector]

Update the public flag contract to match cleanup behavior

When drop_failed_servers=False, this new cleanup branch makes active_servers empty, but the public MCPServerManager docstring still promises that active_servers “will still include all servers” when the flag is false. That documentation is now misleading for callers inspecting the class contract after cleanup_all(); qualify the flag description as connect-phase behavior and document the post-cleanup result.

Useful? React with 👍 / 👎.

[seratch]

Thanks for the follow-up. I confirmed the state difference after cleanup, but this is not an uncovered part of #4586. That PR was scoped to the default drop_failed_servers=True path and deliberately retained the released drop_failed_servers=False behavior, under which active_servers includes all configured servers. The current public docstring states the same contract.

Changing only the cleanup path would make the flag phase-dependent and duplicate active-list derivation outside _refresh_active_servers, without a concrete user scenario showing why the documented behavior is insufficient. The default drop_failed_servers=True already provides the connected-only lifecycle view, while all_servers, failed_servers, and errors expose the configured and failure views separately.

I am going to close this PR. If there is a concrete use case that requires failed servers to remain visible after connection attempts but cleaned servers to disappear after cleanup, we can revisit that contract explicitly.