fix(mcp): catch RuntimeError when canceling tasks during event loop close - #64114
fix(mcp): catch RuntimeError when canceling tasks during event loop close#64114NorethSea wants to merge 2 commits into
Conversation
…lose
When /quit stops the MCP event loop via _stop_mcp_loop(),
MCPServerTask coroutines parked in _wait_for_lifecycle_event or
_wait_for_reconnect_or_shutdown may reach t.cancel() after the loop
is closed. Task.cancel() internally calls loop.call_soon(), which
raises RuntimeError('Event loop is closed') on a dead loop.
The existing _mcp_loop_exception_handler suppresses this error when
it arrives via the loop's exception handler, but not when the
coroutine throws it directly and the exception surfaces to GC as
'Exception ignored in: <coroutine object MCPServerTask.run>'.
Catch RuntimeError around t.cancel() in both finally blocks so the
race is silently handled.
Fixes: NousResearch#60197
Duplicate of #60032 — same two sites ( |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Changes
Fixes event loop shutdown race in tools/mcp_tool.py. When the event loop is already closed (RuntimeError), the code now catches it instead of propagating. Two similar finally-block fixes in _wait_for_lifecycle_event and _wait_for_reconnect_or_shutdown.
Assessment
- Correctness: Fixes a benign shutdown race where RuntimeError is raised when awaiting a cancelled task whose event loop has already closed. Now correctly catches and ignores. The logic is equivalent — either we awaited successfully or caught the error.
- Safety: No security implications. No behavior change for正常运行 paths.
- Code quality: Clean, targeted fix. Comments explain the benign shutdown race. No debug artifacts.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the cancellation race. Current main still has the two unguarded t.cancel() calls targeted here at tools/mcp_tool.py:2115-2121 and tools/mcp_tool.py:2159-2165, so the report is grounded.
Problems
- The equivalent pending-task cleanup in
_wait_for_lazy_reconnect()remains unguarded attools/mcp_tool.py:3118-3124. - The lifecycle race remains at the loop owner:
shutdown_mcp_servers()proceeds to_stop_mcp_loop()after its bounded wait (tools/mcp_tool.py:5661-5667), and that function closes the loop (tools/mcp_tool.py:5822-5826). Guarding two leaf cancellations suppresses only part of the class. - This diff adds no regression for the parked-server shutdown route.
Suggested changes
- Prefer a bounded pending-task drain before loop close, or consistently cover all equivalent waiter cleanup paths.
- Add an end-to-end regression through
shutdown_mcp_servers()with a parkedMCPServerTask.
Automated hermes-sweeper review.
| await t | ||
| except (asyncio.CancelledError, Exception): | ||
| t.cancel() | ||
| except RuntimeError: |
There was a problem hiding this comment.
This guard is needed here, but the same cancel()-then-await cleanup remains in _wait_for_lazy_reconnect() on current main (tools/mcp_tool.py:3118-3124). Please cover that equivalent waiter too if this approach is retained.
There was a problem hiding this comment.
Thanks for catching the equivalent cleanup path. I pushed c6bead7, which adds the same Event loop is closed guard to _wait_for_lazy_reconnect() and a bounded pending-task drain before _stop_mcp_loop() closes the loop. It also adds regression coverage for the waiter cleanup and shutdown_mcp_servers() with a parked MCPServerTask. tests/tools/test_mcp_tool.py passes all 214 tests, and the related MCP tests pass all 24 tests.
|
Rebase-on-behalf delivery — this PR's branch was rebased onto current main ( Rebased branch: Conflict resolution (3 files):
To adopt (author): py_compile verified; test-coverage superset verified. This unblocks the mcp_tool extraction window (hunk gate). |
What does this PR do?
Fixes a benign but noisy
RuntimeError: Event loop is closedtraceback that appears on stderr when using/quitto exit a conversation with active MCP servers.Root cause:
_stop_mcp_loop()closes the MCP event loop, butMCPServerTaskcoroutines parked in_wait_for_lifecycle_event/_wait_for_reconnect_or_shutdownmay reacht.cancel()inside theirfinallyblocks after the loop is already dead.Task.cancel()internally callsloop.call_soon(), which raisesRuntimeError("Event loop is closed").The existing
_mcp_loop_exception_handlersuppresses this error only when routed through the loop's exception handler — not when the coroutine throws it directly, leaving anException ignored in:message from Python's GC.Fix: Catch
RuntimeErroraroundt.cancel()in bothfinallyblocks. The loop-closed case is a harmless race — no connection or state is lost.Related Issue
Fixes #60197
Type of Change
Changes Made
tools/mcp_tool.py— wrapt.cancel()withtry/except RuntimeErrorin_wait_for_lifecycle_eventand_wait_for_reconnect_or_shutdownHow to Test
/quitException ignored in: <coroutine object MCPServerTask.run...>withRuntimeError: Event loop is closed; after the fix: clean exitChecklist
Code
pytest tests/ -qand all tests passDocumentation & Housekeeping