fix(mcp): reconnect stale server entries in register_mcp_servers (#37768)

[Tranquil-Flow]

What does this PR do?

register_mcp_servers() skips servers that already exist in the _servers dict. After a transport disconnect, MCP servers remain in _servers with session=None. The stale entry blocks reconnection, so tool handlers permanently return "not connected" until the agent restarts.

This PR changes the skip condition to allow reconnection when session is None (stale entry) while still skipping servers with an active session.

Note: An existing PR (#37772) also addresses this issue. This PR differs by:

• Guarding the reconnect path with an explicit session is None check rather than unconditionally reconnecting all known servers
• Including a negative test that verifies healthy servers are NOT unnecessarily reconnected
• Providing 8 focused tests covering reconnect, healthy-skip, disabled-skip, empty input, MCP-unavailable, new-server connection, already-connected, and log summary

Related Issue

Fixes #37768

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• tools/mcp_tool.py — Changed the server-skip condition in register_mcp_servers(): now allows reconnection when getattr(_servers[k], "session", None) is None (stale entry), while still honoring enabled: false and skipping servers with active sessions
• tests/tools/test_mcp_tool.py — Added 2 new tests (test_reconnects_stale_server_with_null_session, test_skips_healthy_server_with_active_session) and updated existing test_skips_already_connected_servers to use a mock session object

How to Test

1. Run python3 -m pytest tests/tools/test_mcp_tool.py::TestRegisterMcpServers -v -o 'addopts='
2. Verify stale server (session=None) is reconnected
3. Verify healthy server (active session) is NOT reconnected
4. Verify disabled servers are still skipped
5. Verify new servers are still connected

All 8 tests pass in TestRegisterMcpServers. Fail-without-fix verified: reverting the condition change in tools/mcp_tool.py causes test_reconnects_stale_server_with_null_session to fail.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS 15 (arm64, Python 3.11)

Documentation & Housekeeping

• N/A

[DavidMetcalfe]

Thanks for the fix — the root cause analysis is correct and the test coverage is good.

Two concerns before this is ready to merge:

1. TOCTOU race on session attribute

The session is None check at line ~4080 runs inside _lock, but MCPServerTask.session is mutated by the background task without holding _lock (see _run_http line ~2180: self.session = session). There's a window where:

• register_mcp_servers sees session is None → proceeds to reconnect
• The old background task reconnects simultaneously → sets session = new_session
• Two _discover_and_register_server calls race for the same server name

Mitigation: take a snapshot of the session under _lock and re-check before launching the connection, or cancel the old task first (which also addresses point 2).

2. Old background task not cancelled

When a stale entry is detected, the old MCPServerTask._task may still be alive (parked waiting for _reconnect_event, or still in its reconnect loop). The new connection is launched without cancelling the old task, so both could run concurrently.

Suggested addition before the new_servers dict comprehension in tools/mcp_tool.py:

# Cancel lingering tasks for stale entries
for k in list(_servers.keys()):
    srv = _servers[k]
    if getattr(srv, "session", None) is None:
        old_task = getattr(srv, "_task", None)
        if old_task and not old_task.done():
            old_task.cancel()
        del _servers[k]

This cleans up the stale entry before the skip-condition check, so the TOCTOU window is eliminated and resources are released.

3. Minor: test for mixed-state scenario

The current tests cover stale-only and healthy-only. A test with both stale and healthy servers in the same config would verify that only the stale one is retried.

[Tranquil-Flow]

Thanks for the thorough review! All three concerns are valid. Addressed as follows:

1. TOCTOU race + 2. Old task not cancelled

These share a root cause: a stale MCPServerTask entry in _servers with session=None may still have a live background _task parked in the reconnect backoff loop. register_mcp_servers() previously included the server name in new_servers and launched a fresh _discover_and_register_server, but never cancelled the old task.

The old task could then reconnect independently (setting self.session = new_session from its own _run_http/_run_stdio), racing with the new discovery call — both setting _servers[name], with the old task orphaned and leaking an asyncio Task.

Fix (in tools/mcp_tool.py:register_mcp_servers): Before building new_servers, iterate _servers under _lock and cancel any non-done _task for stale entries, then del _servers[k]. This eliminates the TOCTOU window and releases resources:

for k in list(_servers.keys()):
    srv = _servers[k]
    if getattr(srv, "session", None) is None:
        old_task = getattr(srv, "_task", None)
        if old_task is not None and not old_task.done():
            old_task.cancel()
        del _servers[k]

Task.cancel() is thread-safe as of Python 3.9. The old task will raise CancelledError the next time it awaits (e.g., waking from asyncio.sleep(backoff) in the reconnect loop) and exit cleanly via the existing except asyncio.CancelledError handler at line 1847.

3. Mixed-state test

Added test_mixed_stale_and_healthy_servers — populates _servers with both a healthy server (active session) and a stale server (session=None), then verifies only the stale one triggers _discover_and_register_server.

All 9 tests in TestRegisterMcpServers pass, plus the full 199-test MCP suite.

[teknium1]

Closing with credit — your version of the register-gate fix (cancel the old task before rediscovery) was the safest of the pre-park proposals. Post-#59222 the run task never dies (parks + self-probes), so tearing it down would fight the recovery design; PR #59331 (merged) instead wakes the parked task in place at session startup. Credited alongside #37772 (earliest) in the salvage. Thanks!