fix(mcp): eliminate duplicate config loading during startup (#69)
* fix(mcp): eliminate duplicate config loading during startup load_mcp_config() was called twice on every startup — once in _mcp_config_signature() to compute the cache key, and again inside load_mcp_tools(). This caused warnings to appear twice. Merge the two calls into _load_mcp_config_once() which returns both the signature and the parsed config, then pass the config through to load_mcp_tools() via a new optional parameter. * test(mcp): fix existing cache tests and add coverage for single-load guarantee - Update fake_load_mcp_tools to accept optional config kwarg - Add test_load_mcp_config_called_once_per_cache_miss: verifies load_mcp_config is called exactly once per cache miss (the bug) - Add test_cached_config_passed_to_load_mcp_tools: verifies the pre-loaded config dict is forwarded to load_mcp_tools --------- Co-authored-by: Xi Zhang <106144707+X-iZhang@users.noreply.github.com>
This commit is contained in:
@@ -89,18 +89,18 @@ def _ensure_chat_model():
|
||||
# =============================================================================
|
||||
|
||||
|
||||
def _mcp_config_signature() -> str:
|
||||
"""Return a stable signature for the effective MCP config."""
|
||||
def _load_mcp_config_once() -> tuple[str, dict]:
|
||||
"""Load MCP config and return ``(signature, config)``."""
|
||||
from .mcp.client import load_mcp_config
|
||||
|
||||
cfg = load_mcp_config()
|
||||
if not cfg:
|
||||
return ""
|
||||
return "", {}
|
||||
try:
|
||||
return json.dumps(cfg, sort_keys=True, ensure_ascii=True)
|
||||
sig = json.dumps(cfg, sort_keys=True, ensure_ascii=True)
|
||||
except TypeError:
|
||||
# Fallback for non-JSON-serializable values (should be rare)
|
||||
return repr(cfg)
|
||||
sig = repr(cfg)
|
||||
return sig, cfg
|
||||
|
||||
|
||||
def _load_mcp_tools_cached() -> dict[str, list]:
|
||||
@@ -109,7 +109,7 @@ def _load_mcp_tools_cached() -> dict[str, list]:
|
||||
|
||||
from .mcp import load_mcp_tools
|
||||
|
||||
cfg_key = _mcp_config_signature()
|
||||
cfg_key, cfg = _load_mcp_config_once()
|
||||
if not cfg_key:
|
||||
_MCP_TOOLS_CACHE_KEY = ""
|
||||
_MCP_TOOLS_CACHE_VALUE = {}
|
||||
@@ -118,7 +118,7 @@ def _load_mcp_tools_cached() -> dict[str, list]:
|
||||
if _MCP_TOOLS_CACHE_KEY == cfg_key and _MCP_TOOLS_CACHE_VALUE is not None:
|
||||
return {k: list(v) for k, v in _MCP_TOOLS_CACHE_VALUE.items()}
|
||||
|
||||
loaded = load_mcp_tools()
|
||||
loaded = load_mcp_tools(config=cfg)
|
||||
_MCP_TOOLS_CACHE_KEY = cfg_key
|
||||
_MCP_TOOLS_CACHE_VALUE = {k: list(v) for k, v in loaded.items()}
|
||||
return {k: list(v) for k, v in loaded.items()}
|
||||
|
||||
@@ -660,12 +660,17 @@ async def _load_tools(config: dict[str, Any]) -> dict[str, list]:
|
||||
return server_tools
|
||||
|
||||
|
||||
async def aload_mcp_tools() -> dict[str, list]:
|
||||
async def aload_mcp_tools(config: dict[str, Any] | None = None) -> dict[str, list]:
|
||||
"""Async version of :func:`load_mcp_tools`.
|
||||
|
||||
Prefer this when already inside an async context (e.g. Jupyter, async CLI).
|
||||
|
||||
Args:
|
||||
config: Optional pre-loaded MCP config dict. When ``None``,
|
||||
loads from ``~/.config/evoscientist/mcp.yaml``.
|
||||
"""
|
||||
config = load_mcp_config()
|
||||
if config is None:
|
||||
config = load_mcp_config()
|
||||
if not config:
|
||||
return {}
|
||||
try:
|
||||
@@ -676,7 +681,7 @@ async def aload_mcp_tools() -> dict[str, list]:
|
||||
return _route_tools(config, server_tools)
|
||||
|
||||
|
||||
def load_mcp_tools() -> dict[str, list]:
|
||||
def load_mcp_tools(config: dict[str, Any] | None = None) -> dict[str, list]:
|
||||
"""Load MCP tools and return them grouped by target agent.
|
||||
|
||||
This is the main synchronous entry point. It:
|
||||
@@ -685,12 +690,19 @@ def load_mcp_tools() -> dict[str, list]:
|
||||
3. Filters tools per server allowlist
|
||||
4. Routes tools to target agents
|
||||
|
||||
Args:
|
||||
config: Optional pre-loaded MCP config dict. When ``None``,
|
||||
loads from ``~/.config/evoscientist/mcp.yaml``. Passing a
|
||||
pre-loaded config avoids duplicate env-var interpolation
|
||||
warnings when the caller has already loaded the config.
|
||||
|
||||
Returns:
|
||||
Dict mapping agent name -> list of LangChain ``BaseTool`` objects.
|
||||
Key ``"main"`` = main agent. Other keys = subagent names.
|
||||
Returns empty dict if no MCP servers are configured.
|
||||
"""
|
||||
config = load_mcp_config()
|
||||
if config is None:
|
||||
config = load_mcp_config()
|
||||
if not config:
|
||||
return {}
|
||||
|
||||
|
||||
@@ -23,7 +23,7 @@ class TestMcpToolCaching:
|
||||
lambda: {"srv": {"transport": "stdio", "command": "demo"}},
|
||||
)
|
||||
|
||||
def fake_load_mcp_tools():
|
||||
def fake_load_mcp_tools(config=None):
|
||||
calls["load"] += 1
|
||||
return {"main": [tool]}
|
||||
|
||||
@@ -44,7 +44,7 @@ class TestMcpToolCaching:
|
||||
def fake_load_config():
|
||||
return state["cfg"]
|
||||
|
||||
def fake_load_mcp_tools():
|
||||
def fake_load_mcp_tools(config=None):
|
||||
calls["load"] += 1
|
||||
return {"main": [f"tool-v{calls['load']}"]}
|
||||
|
||||
@@ -57,3 +57,39 @@ class TestMcpToolCaching:
|
||||
|
||||
assert calls["load"] == 2
|
||||
assert first != second
|
||||
|
||||
def test_load_mcp_config_called_once_per_cache_miss(self, monkeypatch):
|
||||
"""load_mcp_config should be called exactly once per cache miss,
|
||||
not twice (once for the signature and once inside load_mcp_tools)."""
|
||||
calls = {"config": 0}
|
||||
|
||||
def counting_load_config():
|
||||
calls["config"] += 1
|
||||
return {"srv": {"transport": "stdio", "command": "demo"}}
|
||||
|
||||
monkeypatch.setattr(
|
||||
"EvoScientist.mcp.client.load_mcp_config", counting_load_config
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"EvoScientist.mcp.load_mcp_tools", lambda config=None: {"main": []}
|
||||
)
|
||||
|
||||
agent_module._load_mcp_tools_cached()
|
||||
assert calls["config"] == 1
|
||||
|
||||
def test_cached_config_passed_to_load_mcp_tools(self, monkeypatch):
|
||||
"""load_mcp_tools should receive the pre-loaded config dict."""
|
||||
received = {}
|
||||
|
||||
def fake_load_config():
|
||||
return {"srv": {"transport": "stdio", "command": "demo"}}
|
||||
|
||||
def fake_load_mcp_tools(config=None):
|
||||
received["config"] = config
|
||||
return {"main": []}
|
||||
|
||||
monkeypatch.setattr("EvoScientist.mcp.client.load_mcp_config", fake_load_config)
|
||||
monkeypatch.setattr("EvoScientist.mcp.load_mcp_tools", fake_load_mcp_tools)
|
||||
|
||||
agent_module._load_mcp_tools_cached()
|
||||
assert received["config"] == {"srv": {"transport": "stdio", "command": "demo"}}
|
||||
|
||||
Reference in New Issue
Block a user