fix(mcp): drain parked tasks before closing loop

[seppegadeyne]

Summary

Fixes the MCP shutdown race that can print a noisy traceback like:

Exception ignored in: <coroutine object MCPServerTask.run ...>
RuntimeError: Event loop is closed

This is the same failure mode discussed in #60032, but instead of only catching RuntimeError at individual Task.cancel() call sites, this drains pending MCP-loop tasks before the shared MCP event loop is stopped and closed.

What changed

• Run the final pending-task drain on the owning MCP event-loop thread.
• Cancel parked/reconnect waiters while the loop is still alive so their finally cleanup gets a cancellation cycle.
• Keep cancellation bounded with an internal drain timeout and a bounded cross-thread wait.
• Keep drain and loop.stop() in one loop-owned sequence, so an outer wait timeout cannot queue stop() ahead of a drain that is still waiting for the loop to resume.
• Log scheduling failures, drain timeouts, drain errors, join timeouts, and close failures at warning level instead of silently swallowing them at debug level.
• Add direct, blocked-loop, and end-to-end regressions through shutdown_mcp_servers().

Why this is better than #60032's narrow catch

#60032 suppresses the symptom by catching RuntimeError around specific child-task cancellation sites. This PR fixes the lifecycle issue one layer higher: the MCP loop owner now gives pending parked tasks a final cancellation cycle before stopping and closing the loop, so their cleanup does not get deferred to coroutine GC after the loop is already closed.

The defensive exception handler for unrelated transport finalizer noise remains unchanged.

Relationship to #72054

#72054 and this PR are complementary:

• fix(mcp): reap orphaned parked server task in _connect_server #72054 reaps an MCPServerTask in _connect_server() when server.start() fails, preventing the known orphan at its source.
• This PR makes the MCP event-loop owner drain any tasks that are still pending before closing the loop, providing the final lifecycle backstop for parked/reconnect tasks and other future task sources.

The two branches merge without code conflicts. Their combined result was tested against current main; the full MCP test set passed.

The broader parked-task ownership/revival issue remains tracked in #60197 and is intentionally outside this PR's scope.

Tests

scripts/run_tests.sh tests/tools/test_mcp_stability.py tests/tools/test_mcp_tool.py tests/tools/test_mcp_cancelled_error_propagation.py -q
# 249 passed

scripts/run_tests.sh tests/tools/test_mcp*.py -q
# passed

# Current main + this PR + #72054
scripts/run_tests.sh tests/tools/test_mcp*.py -q
# 751 passed

python -m ruff check tools/mcp_tool.py tests/tools/test_mcp_stability.py tests/tools/test_mcp_tool.py
# All checks passed

git diff --check upstream/main...HEAD
# clean

A local full-suite run reached the end of the 45k-test matrix but could not produce a clean aggregate result because this workstation's /tmp quota and unrelated existing flaky tests interfered. The affected MCP suites above pass; the refreshed GitHub CI run is the clean-environment full-suite check.

[alt-glitch]

This was generated by AI during triage.

Competing with #60032 for the same MCP shutdown race (RuntimeError: Event loop is closed). #60032 catches RuntimeError at individual Task.cancel() sites; this PR drains pending parked tasks at the loop owner before loop.close() (a level higher). Same goal, different mechanism — maintainer to pick. Relates to merged MCP-lifecycle work #53599 / #59331.

[teknium1]

Thanks for the focused loop-owner cleanup. The reported race remains on current main: parked waiter cleanup calls t.cancel() in tools/mcp_tool.py:2159-2164, while shutdown_mcp_servers() continues to _stop_mcp_loop() after its bounded wait (tools/mcp_tool.py:5663-5667) and that owner closes the loop (tools/mcp_tool.py:5822-5826). The proposed bounded drain addresses that lifecycle boundary without adding configuration or tool surface.

Suggested changes

• Add an end-to-end regression through shutdown_mcp_servers() for a parked MCPServerTask. The new direct _stop_mcp_loop() test establishes the drain invariant, but the reported production route also involves server ownership and the bounded shutdown scheduling path (tools/mcp_tool.py:5624-5667).

Automated hermes-sweeper review.

[seppegadeyne]

Implemented the requested end-to-end regression through shutdown_mcp_servers().

The new test registers a real parked MCPServerTask, exercises the bounded shutdown scheduling path, and verifies that the parked waiter and scheduled shutdown coroutine are drained before the shared loop closes.

Verification:

• On current main, the test fails with the reported RuntimeError: Event loop is closed during parked waiter cleanup.
• On this branch, the focused regression passes.
• scripts/run_tests.sh tests/tools/test_mcp_stability.py tests/tools/test_mcp_tool.py tests/tools/test_mcp_cancelled_error_propagation.py -q: 240 passed.
• scripts/run_tests.sh tests/tools/test_mcp*.py -q: 602 passed.
• Ruff and git diff --check pass.

The branch was also rebased onto current main.

[shady2k]

I'm closing my #66143 as a duplicate of this one — you got here first and this branch is further along. Handing over the one thing from mine that might be worth folding in.

The drain can silently no-op when the join times out

The drain lands after thread.join(timeout=5):

loop.call_soon_threadsafe(loop.stop)
if thread is not None:
    thread.join(timeout=5)
_drain_pending_mcp_loop_tasks(loop)   # <-- here

In the normal path that's fine: the loop has stopped, the thread has exited, and run_until_complete() can restart the loop from the calling thread.

But when the join times out, the loop is still running, so run_until_complete() trips _check_running() and raises RuntimeError: This event loop is already running. The except BaseException catches it and logs at debug, so the drain returns having done nothing — cancellation stays scheduled-but-unthrown, which is the exact failure this PR exists to fix. The asyncio.wait(...) coroutine is also constructed before the raise, so it leaks un-awaited.

Repro, using your _drain_pending_mcp_loop_tasks verbatim, with a callback that blocks the loop past the join timeout:

>>> join timed out, thread alive = True
>>> loop.is_running() = True
DEBUG Error draining pending MCP loop tasks: This event loop is already running
RuntimeWarning: coroutine 'wait' was never awaited
>>> parked waiter cleanup ran = False

To be fair on scope: a loop wedged that badly can't be fully drained by anyone — my version times out against it too. The difference is that this fails silently, at debug, in precisely the scenario where pending tasks are most likely to have been left behind.

Two ways to close it:

1. Drain before loop.stop(), scheduled onto the loop's own thread with run_coroutine_threadsafe → cancel() → gather(). That's the asyncio.runners pattern: no cross-thread run_until_complete, and no dependence on join timing. It's what fix(mcp): drain pending tasks before closing the MCP loop #66143 did — feel free to lift it, it's 20 lines.
2. Keep the placement, but bail early on loop.is_running() and log the skipped drain at warning rather than letting it vanish into debug.

Either is fine by me; (1) is what I'd pick.

Unrelated, for context

The ownership bug behind these parked tasks is separate from the traceback and still open. start() raises _error without cancelling self._task, so _discover_and_register_server never reaches _servers[name] = server, and shutdown_mcp_servers() iterates only _servers — so nothing ever sets that task's _shutdown_event, and every rediscovery constructs another one. Writeup here: #60197 (comment)

This PR reaps those tasks correctly at exit, which is the right fix for the traceback. The accumulation itself needs a behaviour change with a design fork I'd want maintainer input on first, so #60197 should stay open regardless of this landing.

[seppegadeyne]

Thanks @shady2k — I folded in the loop-owned drain direction from #66143 and cherry-picked the relevant commit so your authorship is preserved.

The shutdown sequence now schedules a single loop-owned coroutine that:

1. cancels and boundedly waits for pending MCP-loop tasks;
2. logs cancellation-resistant tasks at warning level;
3. only then schedules loop.stop() from the owning thread.

I also tightened the outer-timeout path beyond the first revision. It no longer cancels the drain future and separately queues loop.stop(), because that could let stop() overtake a drain waiting behind a blocked callback. A real blocked-loop regression now exercises run_coroutine_threadsafe -> timeout -> loop resumes; the old ordering reproduced Task was destroyed but it is pending! and an un-awaited _drain_mcp_loop_tasks warning, while the updated sequence drains the parked task before stopping and closing the loop.

Verification after the final change:

• focused shutdown path: 242 passed;
• complete MCP suite: 631 passed;
• full suite: 42,125 passed, with the same 3 unrelated failures reproduced on clean origin/main;
• Ruff and git diff --check: clean.

The separate ownership/accumulation behavior remains outside this PR and stays tracked in #60197.

[seppegadeyne]

Rebased this PR onto current main (f228e145b) and revalidated the MCP lifecycle changes. I also tested it together with #72054: the patches merge cleanly and all 751 MCP tests pass in the combined tree. The fixes are complementary — #72054 reaps the known failed-start task at the source, while this PR drains any tasks still pending before the MCP loop owner closes the loop. Focused tests, the standalone MCP suite, Ruff, and git diff --check pass on this branch. Fresh GitHub CI is now running on the rebased head.

[kshitijk4poor]

Merged via #74139 with your commits cherry-picked and authorship preserved (rebase merge) — the loop-owned drain landed exactly as you wrote it, including the drain-then-stop sequencing and the blocked-loop regression. shady2k's drain commit from #66143 also survived with authorship intact. Thanks for driving this through the review rounds!