deer-flow/backend/tests/blocking_io/test_mcp_router.py
xiaodu55 6176b057a2
fix(mcp): drain the config read-modify-write across cancellation (#6210)
* fix(mcp): drain the config read-modify-write across cancellation

Route the five MCP config endpoints through the canonical
run_drained_write helper and fold the tools-cache reset into the drained
unit, so a cancelled caller cannot detach from the worker holding the
extensions-config locks or skip the cache invalidation behind a settled
write. Caller-facing HTTPException contracts re-raise unlogged.

Fixes #6209

* fix(mcp): type the drain helper against the worker signature and unlist drain adopters

- _run_drained_mcp_apply now mirrors the canonical helper's [**P, T]
  Callable[P, T] typing so each call site's argument list is checked;
- AGENTS.md stops enumerating the adopted routers, so the line stops
  needing an edit on every new adopter.

---------

Co-authored-by: xiaodu55 <xiaodu55@users.noreply.github.com>
2026-10-03 14:12:02 +08:00

219 lines
8.7 KiB
Python

"""Regression anchor: reading or updating MCP config must not block the event loop.
The GET handler resolves the extensions config path and reads raw JSON. PUT
and PATCH also atomically write and reload it. All of that is blocking
filesystem IO, so the handlers offload the read or whole read-modify-write via
``asyncio.to_thread``. If one regresses back onto the event loop, the strict
Blockbuster gate raises ``BlockingError`` and this test fails.
The admin check is patched to a no-op so the anchor exercises the handler's own
filesystem IO, not the authz layer. Imports sit at module top so any import-time
IO runs at collection, outside the gate.
"""
from __future__ import annotations
import asyncio
import threading
import time
from pathlib import Path
from types import SimpleNamespace
import pytest
from app.gateway.routers import mcp as mcp_router
from app.gateway.routers.mcp import (
McpConfigUpdateRequest,
McpServerConfigResponse,
McpServerStateUpdateRequest,
get_mcp_configuration,
update_mcp_configuration,
update_mcp_server_state,
)
pytestmark = pytest.mark.asyncio
async def test_get_mcp_configuration_does_not_block_or_expand_placeholders(tmp_path: Path, monkeypatch) -> None:
config_path = tmp_path / "extensions_config.json"
placeholder = "$CODEX_PR_5022_BLOCKING_TOKEN"
await asyncio.to_thread(
config_path.write_text,
'{"mcpServers":{"stdio":{"type":"stdio","command":"npx","args":["--token","' + placeholder + '"]}},"skills":{}}',
encoding="utf-8",
)
monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(config_path))
monkeypatch.setenv("CODEX_PR_5022_BLOCKING_TOKEN", "must-not-reach-the-editor")
async def _noop_admin(_request, **_kwargs) -> None:
return None
monkeypatch.setattr(mcp_router, "require_admin_user", _noop_admin)
response = await get_mcp_configuration(request=None)
assert response.mcp_servers["stdio"].args == ["--token", placeholder]
async def test_update_mcp_configuration_does_not_block_event_loop(tmp_path: Path, monkeypatch) -> None:
config_path = tmp_path / "extensions_config.json"
# resolve_config_path() requires the env-pointed file to exist; seed a minimal one.
await asyncio.to_thread(config_path.write_text, '{"mcpServers": {}, "skills": {}}', encoding="utf-8")
monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(config_path))
async def _noop_admin(_request, **_kwargs) -> None:
return None
monkeypatch.setattr(mcp_router, "require_admin_user", _noop_admin)
# An http transport skips the stdio command allowlist check, so the anchor
# stays focused on the filesystem offload rather than command validation.
body = McpConfigUpdateRequest(
mcp_servers={"test-server": McpServerConfigResponse(type="http", url="https://example.test/mcp", description="anchor")},
)
resp = await update_mcp_configuration(request=None, body=body)
assert "test-server" in resp.mcp_servers
# The merged config was actually written to the env-pointed path (offload the
# stat so the assertion itself doesn't trip the gate).
assert await asyncio.to_thread(config_path.exists)
async def test_update_mcp_server_state_does_not_block_event_loop(tmp_path: Path, monkeypatch) -> None:
config_path = tmp_path / "extensions_config.json"
await asyncio.to_thread(
config_path.write_text,
'{"mcpServers":{"remote":{"enabled":false,"transport":"http","url":"https://example.test/mcp"}},"skills":{}}',
encoding="utf-8",
)
monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(config_path))
async def _noop_admin(_request, **_kwargs) -> None:
return None
monkeypatch.setattr(mcp_router, "require_admin_user", _noop_admin)
monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None)
response = await update_mcp_server_state(
request=None,
body=McpServerStateUpdateRequest(server_name="remote", enabled=True),
)
assert response.mcp_servers["remote"].enabled is True
assert response.mcp_servers["remote"].type == "http"
async def test_concurrent_mcp_put_and_patch_updates_are_serialized(tmp_path: Path, monkeypatch) -> None:
"""The write lock keeps the offloaded read-modify-write atomic within the process.
Offloading the RMW to a worker thread dropped the implicit serialization the
single-threaded event loop provided. ``extensions_config_write_lock`` restores
it — and, being shared with the skills router (the other writer of this file),
also serializes against skill toggles: even with several concurrent
mix of ``PUT /api/mcp/config`` and ``PATCH /api/mcp/config`` calls, only one
RMW is inside the critical section at a time. (Without the lock the tracked
max concurrency would exceed 1.)
The tracker is injected *inside* the real worker (via ``reload_extensions_config``,
the last step under the lock) rather than replacing ``_apply_mcp_config_update``,
because the lock now lives in the worker — stubbing the worker out would bypass
the very thing under test.
"""
config_path = tmp_path / "extensions_config.json"
await asyncio.to_thread(
config_path.write_text,
'{"mcpServers":{"s":{"enabled":true,"type":"http","url":"https://example.test/mcp"}},"skills":{}}',
encoding="utf-8",
)
monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(config_path))
async def _noop_admin(_request, **_kwargs) -> None:
return None
monkeypatch.setattr(mcp_router, "require_admin_user", _noop_admin)
monkeypatch.setattr(mcp_router, "_validate_mcp_update_request", lambda _body: None)
state_lock = threading.Lock()
counters = {"active": 0, "max": 0}
def _tracking_reload(*_args, **_kwargs):
# Runs inside the real worker, under extensions_config_write_lock.
with state_lock:
counters["active"] += 1
counters["max"] = max(counters["max"], counters["active"])
time.sleep(0.02) # worker thread (off-loop): hold long enough to expose overlap
with state_lock:
counters["active"] -= 1
return SimpleNamespace(mcp_servers={})
monkeypatch.setattr(mcp_router, "reload_extensions_config", _tracking_reload)
body = McpConfigUpdateRequest(
mcp_servers={"s": McpServerConfigResponse(type="http", url="https://example.test/mcp")},
)
await asyncio.gather(
*[update_mcp_configuration(request=None, body=body) for _ in range(4)],
update_mcp_server_state(
request=None,
body=McpServerStateUpdateRequest(server_name="s", enabled=False),
),
)
assert counters["max"] == 1, f"config updates were not serialized (max concurrency {counters['max']})"
async def test_update_mcp_configuration_drains_write_across_cancellation(tmp_path: Path, monkeypatch) -> None:
"""A cancelled PUT still settles the config write and the tools-cache reset."""
config_path = tmp_path / "extensions_config.json"
await asyncio.to_thread(config_path.write_text, '{"mcpServers": {}, "skills": {}}', encoding="utf-8")
monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(config_path))
async def _noop_admin(_request, **_kwargs) -> None:
return None
monkeypatch.setattr(mcp_router, "require_admin_user", _noop_admin)
started = threading.Event()
release = threading.Event()
original_apply = mcp_router._apply_mcp_config_update
def _blocked_apply(body):
started.set()
assert release.wait(timeout=5)
return original_apply(body)
monkeypatch.setattr(mcp_router, "_apply_mcp_config_update", _blocked_apply)
cache_resets: list[int] = []
monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: cache_resets.append(1))
body = McpConfigUpdateRequest(
mcp_servers={"test-server": McpServerConfigResponse(type="http", url="https://example.test/mcp", description="anchor")},
)
task = asyncio.create_task(update_mcp_configuration(request=None, body=body))
try:
assert await asyncio.to_thread(started.wait, 5)
task.cancel()
await asyncio.sleep(0.05)
task.cancel()
await asyncio.sleep(0.05)
assert not task.done()
release.set()
with pytest.raises(asyncio.CancelledError):
await task
finally:
release.set()
if not task.done():
task.cancel()
await asyncio.gather(task, return_exceptions=True)
# The cancelled caller drained the write: the merged config landed and the
# tools-cache reset published behind it.
written = await asyncio.to_thread(config_path.read_text, encoding="utf-8")
assert "test-server" in written
assert cache_resets == [1]