mirror of
https://github.com/AstrBotDevs/AstrBot.git
synced 2026-09-01 15:32:49 +08:00
fix: exit MCP anyio contexts in the lifecycle task on disable (#9132)
* fix: exit MCP anyio contexts in the lifecycle task on disable #9070 moved MCP shutdown cleanup into the per-server lifecycle task so that the anyio cancel scopes entered in connect_to_server() are exited from the task that entered them. Running the cleanup through asyncio.shield() defeats that: shield wraps the coroutine in a new task, so disabling a server still fails with "Attempted to exit cancel scope in a different task than it was entered in" and leaves the scope state corrupted -- the failure mode behind the CPU spin reported in #9068. Await _terminate_mcp_client() directly instead. The graceful disable path has no cancellation in flight, and on forced shutdown the lifecycle task still runs its finally block in-task, so the shield only served to break task affinity. Closes #9068 * fix: absorb late cancellations during MCP shutdown cleanup A cancellation delivered while the lifecycle task is already running its finally-block cleanup would abort _terminate_mcp_client() halfway, stranding the runtime entry and the MCP transport. Retry the cleanup in the same task and uncancel() the absorbed request instead, so a forced shutdown can no longer skip it. * fix: guard Task.uncancel() for Python 3.10 runtimes Task.uncancel() only exists on Python 3.11+. On 3.10 absorbing the CancelledError is sufficient, so skip the call when unavailable.
This commit is contained in:
@@ -723,8 +723,21 @@ class FunctionToolManager:
|
||||
logger.debug(f"MCP client {name} task was cancelled")
|
||||
raise
|
||||
finally:
|
||||
# Cleanup in the same task that entered the anyio contexts
|
||||
await asyncio.shield(self._terminate_mcp_client(name))
|
||||
# Cleanup in the same task that entered the anyio contexts:
|
||||
# asyncio.shield() would schedule the coroutine as a separate
|
||||
# Task, and anyio cancel scopes cannot exit across tasks (#9068).
|
||||
# Absorb late cancellations so a forced shutdown cannot abort
|
||||
# the cleanup halfway.
|
||||
task = asyncio.current_task()
|
||||
while True:
|
||||
try:
|
||||
await self._terminate_mcp_client(name)
|
||||
break
|
||||
except asyncio.CancelledError:
|
||||
# Task.uncancel() is 3.11+; on 3.10 absorbing the
|
||||
# cancellation is sufficient.
|
||||
if task is not None and hasattr(task, "uncancel"):
|
||||
task.uncancel()
|
||||
|
||||
lifecycle_task = asyncio.create_task(
|
||||
connect_and_lifecycle(), name=f"mcp-client:{name}"
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import asyncio
|
||||
import json
|
||||
|
||||
import pytest
|
||||
@@ -348,6 +349,65 @@ def test_firecrawl_tools_are_registered_as_builtin_tools():
|
||||
assert manager.is_builtin_tool("firecrawl_extract_web_page") is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mcp_shutdown_cleanup_runs_in_lifecycle_task(monkeypatch):
|
||||
"""Disabling an MCP server must clean up in the task that connected.
|
||||
|
||||
anyio cancel scopes entered in connect_to_server() can only be exited
|
||||
from the same task, otherwise the scope state is corrupted and its
|
||||
cancellation loop spins at 100% CPU (#9068).
|
||||
"""
|
||||
manager = FunctionToolManager()
|
||||
seen = {}
|
||||
|
||||
async def fake_connect(self, config, name):
|
||||
seen["connect_task"] = asyncio.current_task()
|
||||
|
||||
async def fake_list_tools(self):
|
||||
self.tools = []
|
||||
|
||||
async def fake_cleanup(self):
|
||||
seen["cleanup_task"] = asyncio.current_task()
|
||||
|
||||
monkeypatch.setattr(ftm.MCPClient, "connect_to_server", fake_connect)
|
||||
monkeypatch.setattr(ftm.MCPClient, "list_tools_and_save", fake_list_tools)
|
||||
monkeypatch.setattr(ftm.MCPClient, "cleanup", fake_cleanup)
|
||||
|
||||
await manager.enable_mcp_server("dummy", {"command": "python"}, timeout=5)
|
||||
await manager.disable_mcp_server("dummy", timeout=5)
|
||||
|
||||
assert seen["cleanup_task"] is seen["connect_task"]
|
||||
assert "dummy" not in manager.mcp_client_dict
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mcp_shutdown_cleanup_survives_late_cancellation(monkeypatch):
|
||||
"""A cancellation arriving mid-cleanup must not abort the cleanup."""
|
||||
manager = FunctionToolManager()
|
||||
cleanup_calls = []
|
||||
|
||||
async def fake_connect(self, config, name):
|
||||
pass
|
||||
|
||||
async def fake_list_tools(self):
|
||||
self.tools = []
|
||||
|
||||
async def fake_cleanup(self):
|
||||
cleanup_calls.append(asyncio.current_task())
|
||||
if len(cleanup_calls) == 1:
|
||||
raise asyncio.CancelledError()
|
||||
|
||||
monkeypatch.setattr(ftm.MCPClient, "connect_to_server", fake_connect)
|
||||
monkeypatch.setattr(ftm.MCPClient, "list_tools_and_save", fake_list_tools)
|
||||
monkeypatch.setattr(ftm.MCPClient, "cleanup", fake_cleanup)
|
||||
|
||||
await manager.enable_mcp_server("dummy", {"command": "python"}, timeout=5)
|
||||
await manager.disable_mcp_server("dummy", timeout=5)
|
||||
|
||||
assert len(cleanup_calls) == 2
|
||||
assert "dummy" not in manager.mcp_client_dict
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_modelscope_sync_enables_only_synced_servers(monkeypatch):
|
||||
class FakeResponse:
|
||||
|
||||
Reference in New Issue
Block a user