fix(mcp): drain pending tasks before closing the MCP loop

[shady2k]

Resolves the RuntimeError: Event loop is closed traceback from #60197 / #66113. Root-cause writeup: #60197 (comment)

The problem

_stop_mcp_loop() stops and closes the background loop without reaping the tasks still on it:

loop.call_soon_threadsafe(loop.stop)
thread.join(timeout=5)
loop.close()

A task left suspended there isn't gone — the GC finalizes its coroutine later, which resumes it to run cleanup against a loop that no longer exists. For a server parked in _wait_for_reconnect_or_shutdown, that cleanup is t.cancel() → call_soon() → RuntimeError. Because it happens in a finalizer, Python can only print "Exception ignored in".

How a task ends up unowned: run() deliberately stays alive after exhausting _MAX_INITIAL_CONNECT_RETRIES so a later refresh can revive it, but start() then raises _error without cancelling self._task, so _discover_and_register_server never reaches _servers[name] = server. shutdown_mcp_servers() iterates only _servers, so nothing ever sets that task's _shutdown_event.

The fix

Drain the loop the way asyncio.run() does before closing it — cancel the remaining tasks and gather them while the loop is still open, so each runs its own finally.

Cancel-without-gather would not work: Task.cancel() only schedules the throw.

This is deliberately not a try/except around the raise site. #64960 took that route and the reporter of #66113 observed the error "shifted but never fully disappeared" — silencing the exception leaves the cleanup unrun. This makes the cleanup actually run.

Scope — please read before merging

This fixes the traceback, not the leak. Until final shutdown the parked task is still unowned: every rediscovery constructs another one (:5106 and :5224 classify servers purely by absence from _servers), and each self-probes every _PARKED_RETRY_INTERVAL. Long-lived gateways still accumulate them; this change just reaps them correctly at the end.

The ownership fix is a separate, behaviour-changing patch with a design fork I'd like maintainer input on first — I laid the options out in the issue comment linked above. I used "Addresses" rather than "Fixes" on purpose, so #60197 stays open for that follow-up.

Testing

tests/tools/test_mcp_stability.py::TestMCPLoopDrainOnStop — puts a task on the MCP loop whose finally cancels a helper (the operation that needs a live loop), then calls _stop_mcp_loop().

Verified it's a real regression test:

• without the fix: task.done() = False, cleanup never ran, and Python prints Task was destroyed but it is pending!
• with the fix: task done, cleanup ran, no error

Full MCP suite (pytest tests/tools/ -k mcp): 663 passed with this change vs 662 passed on main — the same 5 pre-existing failures in test_mcp_structured_content.py appear on both (they pass in isolation; looks like unrelated cross-test pollution).

🤖 Generated with Claude Code

[alt-glitch]

This was generated by AI during triage.

Duplicate of #60104 — both PRs drain pending MCP-loop tasks before loop closure to let parked-task cleanup run on a live event loop. #60104 is the earlier, broader implementation.

[tonydwb]

Code Review Summary

Verdict: Approved

PR #66143 — fix(mcp): drain pending tasks before closing the MCP loop

Assessment

• Correctness: Regression test for GC-triggered when pending tasks are cancelled after loop close. The test simulates a parked task that does + in its block — exactly the pattern that triggers the bug. Test logic is sound.
• Scope: Adds 72 lines of focused regression test only; no production code changes.
• Testing: Dedicated class with clear docstring linking to #60197.
• No issues found: Clean fix-PR profile.

---

Reviewed by Hermes Agent

[tonydwb]

Code Review Summary

Verdict: Approved

PR #66143 fix(mcp): drain pending tasks before closing the MCP loop

Assessment

• Correctness: Regression test for GC-triggered RuntimeError when pending tasks are cancelled after loop close. The test simulates a parked task that does cancel() + call_soon() in its finally block — exactly the pattern that triggers bug #60197. Test logic is sound.
• Scope: Adds 72 lines of focused regression test only; no production code changes.
• Testing: Dedicated TestMCPLoopDrainOnStop class with clear docstring linking to #60197.
• No issues found

---

Reviewed by Hermes Agent

[shady2k]

Closing as a duplicate of #60104 — the triage call was right.

Same fix (drain pending tasks while the loop is still open, before loop.close()), same function, and #60104 predates this by ten days, has review from @teknium1, and already added the end-to-end regression through shutdown_mcp_servers() that this PR doesn't have. No reason to keep a competing PR open against it.

The one distinction that was worth carrying over — the drain silently no-opping when thread.join() times out — I've left there instead: #60104 (comment)

#60197 stays open on its own merits: the task-ownership leak behind these parked tasks is a separate behaviour change and isn't addressed by either PR.