Skip to content

Fix MCP task cancellation after event loop shutdown - #61466

Closed
sc-starman wants to merge 2 commits into
NousResearch:mainfrom
sc-starman:fix/mcp-closed-loop-cancel
Closed

Fix MCP task cancellation after event loop shutdown#61466
sc-starman wants to merge 2 commits into
NousResearch:mainfrom
sc-starman:fix/mcp-closed-loop-cancel

Conversation

@sc-starman

@sc-starman sc-starman commented Jul 9, 2026

Copy link
Copy Markdown

Summary

This fixes a shutdown/finalization race in MCPServerTask where lifecycle waiter tasks can be cancelled after the MCP event loop has already closed.

asyncio.Task.cancel() schedules cancellation on the task's owning loop. If coroutine finalization runs after the loop is closed, raw task.cancel() can raise RuntimeError("Event loop is closed"), producing noisy Exception ignored in: <coroutine object MCPServerTask.run ...> traces during shutdown.

Fixes #60197.

Related: #60032, #60104.

Changes

  • Add MCPServerTask._cancel_task_if_loop_open().
  • Use it in lifecycle waiter cleanup paths.
  • Use it for timeout/pending-refresh task cancellation during MCP shutdown.
  • Preserve normal cancellation behavior when the task loop is still open.

Verification

  • python3 -m py_compile tools/mcp_tool.py
  • Focused closed-loop check confirms _cancel_task_if_loop_open() returns False instead of raising when a task's owning loop is already closed.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/mcp MCP client and OAuth labels Jul 9, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: LGTM

What the PR Does

Fix MCP task cancellation after event loop shutdown.

Assessment

  • Clean fix for a race condition between task cancellation and event loop shutdown.

Note: PR was previously unreviewed.


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for covering the lifecycle waiter, start, timeout, and refresh-task cancellation paths.

Problems

  • The PR has no regression test for the closed-owning-loop contract. Current coverage in tests/tools/test_mcp_cancelled_error_propagation.py:31-91 exercises cancellation propagation on a live loop, but does not cover the reported Task.cancel() after loop.close() race. The raw failure sites remain on main at tools/mcp_tool.py:1953, :1997, and :2956.

Suggested changes

  • Add a focused tests/tools/ test with a pending task tied to a closed loop that asserts _cancel_task_if_loop_open() returns False without raising, plus an open-loop case that confirms ordinary cancellation is preserved.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 11, 2026
@sc-starman
sc-starman force-pushed the fix/mcp-closed-loop-cancel branch from c657ebd to dd244dc Compare July 11, 2026 13:10
@sc-starman

Copy link
Copy Markdown
Author

Addressed the regression-test request. Added focused coverage for closed-loop cleanup (no cancellation attempt or RuntimeError) and normal live-loop task cancellation. Verified with: .. [100%]
2 passed in 0.32s (4 passed).

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

The shutdown race this PR guards against is now fixed at the root on main via #74139: the MCP loop owner drains pending tasks (bounded, loop-owned) before loop.stop()/loop.close(), and a server parked after exhausted initial connect retries is retained in _servers so shutdown owns and reaps it. With the loop never closing under a suspended task, the Task.cancel()-on-closed-loop path this PR's guard defends can no longer be reached from the reported scenario — and the empirical matrix on #60197 showed catch-at-cancel-site guards leave the parked task leaked (cleanup still never runs, just more quietly).

Closing as superseded by the root-cause fix. Thanks for the defensive-guard work and for adding the closed-loop regression coverage when asked — the loop-aware cancellation analysis here helped confirm the failure chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] RuntimeError: Event loop is closed during /exit (MCPServerTask.shutdown)

5 participants