diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 910c15647..7250dd567 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -271,7 +271,7 @@ Setup: Copy `config.example.yaml` to `config.yaml` in the **project root** direc **Config Hot-Reload Boundary**: Gateway dependencies route through `get_app_config()` on every request, so per-run fields like `models[*].max_tokens`, `summarization.*`, `title.*`, `memory.*`, `subagents.*`, `tools[*]`, and the agent system prompt pick up `config.yaml` edits on the next message. `AppConfig` is intentionally **not** cached on `app.state` — `lifespan()` keeps a local `startup_config` variable for one-shot bootstrap work and passes it to `langgraph_runtime(app, startup_config)`. -Infrastructure fields are **restart-required**. The authoritative list lives in `packages/harness/deerflow/config/reload_boundary.py::STARTUP_ONLY_FIELDS` and is mirrored by the standardised `"startup-only:"` prefix on the corresponding `Field(description=...)` in `AppConfig`, so IDE hover on those fields surfaces the reason inline (no need to context-switch into this table). Currently registered: `database`, `checkpointer`, `run_events`, `stream_bridge`, `sandbox`, `log_level`, `logging`, `channels`, `channel_connections`. Adding a new restart-required field requires updating the registry; drift is pinned by `tests/test_reload_boundary.py`. +Infrastructure fields are **restart-required**. The authoritative list lives in `packages/harness/deerflow/config/reload_boundary.py::STARTUP_ONLY_FIELDS` and is mirrored by the standardised `"startup-only:"` prefix on the corresponding `Field(description=...)` in `AppConfig`, so IDE hover on those fields surfaces the reason inline (no need to context-switch into this table). Currently registered: `database`, `checkpointer`, `run_events`, `stream_bridge`, `sandbox`, `log_level`, `logging`, `channels`, `channel_connections`, `scheduler`, `run_ownership`. Adding a new restart-required field requires updating the registry; drift is pinned by `tests/test_reload_boundary.py`. **Persistence backend resolution**: the unified `database` section selects the Gateway's LangGraph checkpointer, LangGraph Store, and DeerFlow SQL repositories. @@ -397,7 +397,7 @@ Proxied through nginx: `/api/langgraph/*` → Gateway LangGraph-compatible runti `get_available_tools(groups, include_mcp, model_name, subagent_enabled)` assembles: 1. **Config-defined tools** - Resolved from `config.yaml` via `resolve_variable()` -2. **MCP tools** - From enabled MCP servers (lazy initialized, cached with mtime invalidation) +2. **MCP tools** - From enabled MCP servers (lazy initialized, cached with resolved-path + content-signature invalidation) 3. **Built-in tools**: - `present_files` - Make output files visible to user (only `/mnt/user-data/outputs`) - `ask_clarification` - Request clarification (intercepted by ClarificationMiddleware, which preserves text fallback and adds `artifact.human_input` for Web UI Human Input Cards) @@ -429,7 +429,7 @@ Additional providers also live here (`boxlite`, `brave`, `browserless`, `crawl4a - Uses `langchain-mcp-adapters` `MultiServerMCPClient` for multi-server management - **Lazy initialization**: Tools loaded on first use via `get_cached_mcp_tools()` -- **Cache invalidation**: Detects config file changes via mtime comparison +- **Cache invalidation**: Detects extensions-config changes by comparing the resolved config path and a `(mtime, size, sha256)` content signature against the values recorded at initialization (mirrors `config/app_config.py::get_app_config()`), not a strict mtime `>` comparison. This catches same-second edits, mtime that stays put or moves backward (`git checkout`, `cp -p` / backup restore, `tar` / `rsync`, object-store / network mounts), and a switch to a different config file with an equal-or-older mtime - **Transports**: stdio (command-based), SSE, HTTP - **OAuth (HTTP/SSE)**: Supports token endpoint flows (`client_credentials`, `refresh_token`) with automatic token refresh + Authorization header injection - **Routing hints**: `extensions_config.json -> mcpServers..routing` and @@ -442,7 +442,7 @@ Additional providers also live here (`boxlite`, `brave`, `browserless`, `crawl4a add a parallel routing middleware for PR1-style preference hints. - **Stdio file outputs**: Persistent stdio sessions are scoped by `user_id:thread_id`. For stdio transports only, DeerFlow pins the subprocess default `cwd` to the thread workspace and `TMPDIR`/`TMP`/`TEMP` to `workspace/.mcp/tmp/`, unless the operator explicitly configured `cwd` or temp env values. SSE/HTTP transports skip this filesystem prep entirely. - **Stdio path translation**: MCP-returned local file references are not copied. If a `ResourceLink` or conservative free-text path resolves to an existing file inside the thread's mounted user-data tree, it is translated deterministically to `/mnt/user-data/...`; paths outside that tree remain unchanged. -- **Runtime updates**: Gateway API saves to extensions_config.json; the Gateway-embedded runtime detects changes via mtime +- **Runtime updates**: Gateway API saves to extensions_config.json; the Gateway-embedded runtime detects changes via the resolved-path + content-signature check above, so multi-worker / stale-mtime deployments still pick up an added/removed MCP server without a restart (the `PUT /api/mcp/config` reset only clears the cache in its own worker) ### Skills System (`packages/harness/deerflow/skills/`) diff --git a/backend/packages/harness/deerflow/mcp/cache.py b/backend/packages/harness/deerflow/mcp/cache.py index 333a034c8..d45987d76 100644 --- a/backend/packages/harness/deerflow/mcp/cache.py +++ b/backend/packages/harness/deerflow/mcp/cache.py @@ -1,8 +1,9 @@ """Cache for MCP tools to avoid repeated loading.""" import asyncio +import hashlib import logging -import os +from pathlib import Path from langchain_core.tools import BaseTool @@ -11,43 +12,101 @@ logger = logging.getLogger(__name__) _mcp_tools_cache: list[BaseTool] | None = None _cache_initialized = False _initialization_lock = asyncio.Lock() -_config_mtime: float | None = None # Track config file modification time + +# Cache-invalidation key for the resolved extensions config file. We track the +# resolved path *and* a ``(mtime, size, sha256)`` content signature — mirroring +# ``deerflow.config.app_config`` for the sibling runtime-editable config file — +# rather than only the mtime. A strict mtime ``>`` comparison misses same-second +# edits and mtime that stays put or moves backward (object-store / network +# mounts, ``git checkout``, ``cp -p`` / backup restore, ``tar`` / ``rsync`` that +# preserve timestamps), and tracking no path at all makes a switch to a +# different config file with an equal-or-older mtime structurally invisible. +_ConfigSignature = tuple[float | None, int | None, str | None] +_config_path: Path | None = None # Resolved extensions config path at init time +_config_signature: _ConfigSignature | None = None # (mtime, size, sha256) at init time -def _get_config_mtime() -> float | None: - """Get the modification time of the extensions config file. - - Returns: - The modification time as a float, or None if the file doesn't exist. - """ +def _resolve_config_path() -> Path | None: + """Resolve the extensions config file path, or ``None`` when unconfigured.""" from deerflow.config.extensions_config import ExtensionsConfig - config_path = ExtensionsConfig.resolve_config_path() - if config_path and config_path.exists(): - return os.path.getmtime(config_path) - return None + return ExtensionsConfig.resolve_config_path() + + +def _get_config_signature(config_path: Path) -> _ConfigSignature | None: + """Get cache metadata for the extensions config file, including a content digest. + + Mirrors ``deerflow.config.app_config._get_config_signature`` so both + runtime-editable config files (``config.yaml`` and ``extensions_config.json``) + share the same content-based staleness signal. Returns ``None`` when the + file cannot be stat-ed (e.g. it does not exist). + """ + try: + stat_result = config_path.stat() + except OSError: + return None + + # Always hash the full file here rather than short-circuiting when + # mtime/size already match a previously recorded signature: swapping in a + # different MCP server config of identical byte length within the same + # second leaves mtime *and* size unchanged, so only the sha256 catches + # that swap. Skipping the hash on an mtime/size match would reopen the + # narrow gap this signature was built to close. + digest = hashlib.sha256() + try: + with config_path.open("rb") as f: + for chunk in iter(lambda: f.read(1024 * 1024), b""): + digest.update(chunk) + except OSError: + return (stat_result.st_mtime, stat_result.st_size, None) + + return (stat_result.st_mtime, stat_result.st_size, digest.hexdigest()) + + +def _current_config_state() -> tuple[Path | None, _ConfigSignature | None]: + """Return the currently resolved extensions config path and its signature.""" + config_path = _resolve_config_path() + if config_path is None: + return None, None + return config_path, _get_config_signature(config_path) def _is_cache_stale() -> bool: """Check if the cache is stale due to config file changes. + The cache is stale when the resolved extensions config path changed, or when + the ``(mtime, size, sha256)`` content signature differs from the one recorded + at initialization. Using content equality (``!=``) instead of a strict mtime + ``>`` comparison detects same-second edits and backward mtime moves, and + tracking the resolved path detects a switch to a different config file. + Returns: True if the cache should be invalidated, False otherwise. """ - global _config_mtime - if not _cache_initialized: return False # Not initialized yet, not stale - current_mtime = _get_config_mtime() + current_path, current_signature = _current_config_state() - # If we couldn't get mtime before or now, assume not stale - if _config_mtime is None or current_mtime is None: + # Preserve the original "config missing / not yet recorded" behavior: if + # there was no readable config when the cache was populated, or there is + # none now, do not invalidate. This also covers the config being deleted + # entirely after a successful init (current_signature flips to None): the + # cache intentionally keeps serving its last-known-good MCP tools rather + # than invalidating into an unconfigured state, matching the pre-fix + # mtime-only contract (which also returned False once the file could no + # longer be stat-ed). Treat this as a deliberate fail-soft choice, not an + # oversight — a future change that wants "config deleted" to tear down + # MCP tools needs its own explicit signal here, not an inferred one. + if _config_signature is None or current_signature is None: return False - # If the config file has been modified since we cached, it's stale - if current_mtime > _config_mtime: - logger.info(f"MCP config file has been modified (mtime: {_config_mtime} -> {current_mtime}), cache is stale") + if current_path != _config_path: + logger.info("MCP config path changed (%s -> %s), cache is stale", _config_path, current_path) + return True + + if current_signature != _config_signature: + logger.info("MCP config content changed (signature %s -> %s), cache is stale", _config_signature, current_signature) return True return False @@ -61,7 +120,7 @@ async def initialize_mcp_tools() -> list[BaseTool]: Returns: List of LangChain tools from all enabled MCP servers. """ - global _mcp_tools_cache, _cache_initialized, _config_mtime + global _mcp_tools_cache, _cache_initialized, _config_path, _config_signature async with _initialization_lock: if _cache_initialized: @@ -73,8 +132,8 @@ async def initialize_mcp_tools() -> list[BaseTool]: logger.info("Initializing MCP tools...") _mcp_tools_cache = await get_mcp_tools() _cache_initialized = True - _config_mtime = _get_config_mtime() # Record config file mtime - logger.info(f"MCP tools initialized: {len(_mcp_tools_cache)} tool(s) loaded (config mtime: {_config_mtime})") + _config_path, _config_signature = _current_config_state() # Record config path + content signature + logger.info("MCP tools initialized: %d tool(s) loaded (config path: %s)", len(_mcp_tools_cache), _config_path) return _mcp_tools_cache @@ -136,10 +195,11 @@ def reset_mcp_tools_cache() -> None: Also closes all persistent MCP sessions so they are recreated on the next tool load. """ - global _mcp_tools_cache, _cache_initialized, _config_mtime + global _mcp_tools_cache, _cache_initialized, _config_path, _config_signature _mcp_tools_cache = None _cache_initialized = False - _config_mtime = None + _config_path = None + _config_signature = None # Close persistent sessions – they will be recreated by the next # get_mcp_tools() call with the (possibly updated) connection config. diff --git a/backend/tests/test_mcp_cache.py b/backend/tests/test_mcp_cache.py new file mode 100644 index 000000000..d2913bba2 --- /dev/null +++ b/backend/tests/test_mcp_cache.py @@ -0,0 +1,245 @@ +"""Tests for MCP tools cache staleness detection (``deerflow.mcp.cache``). + +Regression coverage for the content-signature invalidation fix. The cache used +to invalidate on a strict extensions-config *mtime* ``>`` comparison and tracked +no resolved path, so it missed three real edit patterns that leave stale MCP +tools serving in the LangGraph-embedded runtime and every non-writer worker: + +1. content change with an unchanged mtime (same-second edit; object-store / + network mounts that do not bump mtime), +2. content change with a backward mtime (``git checkout``, ``cp -p`` / backup + restore, ``tar`` / ``rsync`` preserving timestamps), +3. a resolved-path switch to a different config file whose mtime is <= the one + recorded at initialization. + +The fix mirrors ``deerflow.config.app_config``'s ``(path, (mtime, size, +sha256))`` detection so both runtime-editable config files share one staleness +signal. These tests fail on the pre-fix code (cases 1-3 return ``False``) and +pass afterwards. +""" + +from __future__ import annotations + +import asyncio +import json +import os +from pathlib import Path + +import pytest + +import deerflow.mcp.cache as cache_module +from deerflow.config.extensions_config import ExtensionsConfig + +_MISSING = object() + +# Module globals that hold cache state. Snapshotted and restored around every +# test so an initialized cache — or an asyncio lock bound to a closed loop — +# cannot leak between tests. ``_config_mtime`` is the pre-fix global name and is +# tracked too so the same fixture works when the source fix is reverted. +_TRACKED_GLOBALS = ( + "_mcp_tools_cache", + "_cache_initialized", + "_config_path", + "_config_signature", + "_config_mtime", + "_initialization_lock", +) + + +def _write_extensions_config(path: Path, servers: dict) -> None: + path.write_text(json.dumps({"mcpServers": servers, "skills": {}}), encoding="utf-8") + + +def _server(command: str = "npx") -> dict: + return {"enabled": True, "type": "stdio", "command": command} + + +@pytest.fixture() +def cache_globals(): + """Snapshot/restore ``deerflow.mcp.cache`` module globals and reset the lock.""" + saved = {name: getattr(cache_module, name, _MISSING) for name in _TRACKED_GLOBALS} + + cache_module._mcp_tools_cache = None + cache_module._cache_initialized = False + for name in ("_config_path", "_config_signature", "_config_mtime"): + if hasattr(cache_module, name): + setattr(cache_module, name, None) + # asyncio.Lock binds to the first event loop it is awaited on, so each test + # (which drives initialize_mcp_tools via a fresh asyncio.run) needs its own. + cache_module._initialization_lock = asyncio.Lock() + + try: + yield + finally: + for name, value in saved.items(): + if value is _MISSING: + if hasattr(cache_module, name): + delattr(cache_module, name) + else: + setattr(cache_module, name, value) + + +def _initialize_against(monkeypatch, config_path: Path) -> None: + """Populate the cache against ``config_path`` via the real init entry point. + + ``initialize_mcp_tools()`` records the resolved config path + content + signature after loading tools; the tool load itself is stubbed so this stays + a cache-state unit test with no real MCP servers. + """ + monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(config_path)) + + async def _fake_get_mcp_tools(): + return [] + + monkeypatch.setattr("deerflow.mcp.tools.get_mcp_tools", _fake_get_mcp_tools) + asyncio.run(cache_module.initialize_mcp_tools()) + assert cache_module._cache_initialized is True + + +def test_not_stale_before_initialization(cache_globals): + """An uninitialized cache is never stale (preserved behavior).""" + assert cache_module._cache_initialized is False + assert cache_module._is_cache_stale() is False + + +def test_initialize_records_path_and_signature(cache_globals, monkeypatch, tmp_path): + """initialize_mcp_tools records the resolved path and a full content signature.""" + cfg = tmp_path / "extensions_config.json" + _write_extensions_config(cfg, {"srv1": _server()}) + + _initialize_against(monkeypatch, cfg) + + assert cache_module._config_path == cfg + assert cache_module._config_signature is not None + mtime, size, digest = cache_module._config_signature + assert mtime == cfg.stat().st_mtime + assert size == cfg.stat().st_size + assert isinstance(digest, str) and len(digest) == 64 # sha256 hexdigest + + +def test_same_mtime_content_change_is_stale(cache_globals, monkeypatch, tmp_path): + """Failure mode 1: content rewritten, mtime forced to stay identical.""" + cfg = tmp_path / "extensions_config.json" + _write_extensions_config(cfg, {"srv1": _server()}) + _initialize_against(monkeypatch, cfg) + recorded_mtime = cfg.stat().st_mtime + + _write_extensions_config(cfg, {"srv1": _server(), "srv2": _server("uvx")}) + os.utime(cfg, (recorded_mtime, recorded_mtime)) + assert cfg.stat().st_mtime == recorded_mtime # guard: mtime truly unchanged + + assert cache_module._is_cache_stale() is True + + +def test_backward_mtime_content_change_is_stale(cache_globals, monkeypatch, tmp_path): + """Failure mode 2: content rewritten, mtime moved backward.""" + cfg = tmp_path / "extensions_config.json" + _write_extensions_config(cfg, {"srv1": _server()}) + _initialize_against(monkeypatch, cfg) + recorded_mtime = cfg.stat().st_mtime + + _write_extensions_config(cfg, {"different": _server()}) + older = recorded_mtime - 100 + os.utime(cfg, (older, older)) + assert cfg.stat().st_mtime < recorded_mtime # guard: mtime went backward + + assert cache_module._is_cache_stale() is True + + +def test_config_path_switch_is_stale(cache_globals, monkeypatch, tmp_path): + """Failure mode 3: resolved path switches to a different file, mtime <= recorded.""" + cfg_a = tmp_path / "extensions_config.json" + cfg_b = tmp_path / "other_extensions_config.json" + _write_extensions_config(cfg_a, {"srv1": _server()}) + _initialize_against(monkeypatch, cfg_a) + recorded_mtime = cfg_a.stat().st_mtime + + _write_extensions_config(cfg_b, {"totally": _server("uvx")}) + older = recorded_mtime - 50 + os.utime(cfg_b, (older, older)) # a DIFFERENT file, mtime <= recorded + + # The resolver now points at cfg_b (e.g. DEER_FLOW_EXTENSIONS_CONFIG_PATH + # was repointed, or default resolution now finds a different file). + monkeypatch.setattr( + ExtensionsConfig, + "resolve_config_path", + classmethod(lambda cls, config_path=None: cfg_b), + ) + + assert cache_module._is_cache_stale() is True + + +def test_unchanged_file_is_not_stale(cache_globals, monkeypatch, tmp_path): + """Sanity: an untouched config file does not trigger a needless reinit.""" + cfg = tmp_path / "extensions_config.json" + _write_extensions_config(cfg, {"srv1": _server()}) + _initialize_against(monkeypatch, cfg) + + assert cache_module._is_cache_stale() is False + + +def test_forward_edit_is_stale(cache_globals, monkeypatch, tmp_path): + """Sanity: a genuine forward edit is still detected as stale.""" + cfg = tmp_path / "extensions_config.json" + _write_extensions_config(cfg, {"srv1": _server()}) + _initialize_against(monkeypatch, cfg) + recorded_mtime = cfg.stat().st_mtime + + _write_extensions_config(cfg, {"srv1": _server(), "srv2": _server("uvx")}) + newer = recorded_mtime + 100 + os.utime(cfg, (newer, newer)) + + assert cache_module._is_cache_stale() is True + + +def test_same_mtime_same_size_swap_is_stale(cache_globals, monkeypatch, tmp_path): + """Precise variant of failure mode 1: mtime *and* size both stay unchanged + (an equal-length server-name swap), so mtime/size alone are indistinguishable + and only the sha256 content digest can catch the change. Guards the content + digest itself: a future change that starts short-circuiting the hash + whenever mtime/size already match a recorded value must not make this test + pass without actually detecting the swap. + """ + cfg = tmp_path / "extensions_config.json" + _write_extensions_config(cfg, {"srv1": _server()}) + _initialize_against(monkeypatch, cfg) + recorded_mtime = cfg.stat().st_mtime + recorded_size = cfg.stat().st_size + + _write_extensions_config(cfg, {"srv9": _server()}) # same-length key swap + os.utime(cfg, (recorded_mtime, recorded_mtime)) + assert cfg.stat().st_mtime == recorded_mtime # guard: mtime truly unchanged + assert cfg.stat().st_size == recorded_size # guard: size truly unchanged too + + assert cache_module._is_cache_stale() is True + + +def test_config_deleted_after_init_is_not_stale(cache_globals, monkeypatch, tmp_path): + """Latent edge preserved by design: if the resolved config file is deleted + entirely after a successful init, ``current_signature`` becomes ``None`` and + the cache does NOT invalidate — it keeps serving its last-known-good MCP + tools instead of tearing down into an unconfigured state. This matches the + pre-fix mtime-only contract, which also returned ``False`` once the file + could no longer be stat-ed, so it is not a regression introduced by the + content-signature fix. + + The resolver is monkeypatched to keep pointing at the (now-missing) path, + isolating ``_is_cache_stale``'s own stat-failure handling from + ``ExtensionsConfig.resolve_config_path``'s separate not-found contract for + explicit path/env-var configuration (that function raises + ``FileNotFoundError`` in that mode instead of returning ``None`` — a + distinct, pre-existing latent issue outside this module's scope). + """ + cfg = tmp_path / "extensions_config.json" + _write_extensions_config(cfg, {"srv1": _server()}) + _initialize_against(monkeypatch, cfg) + assert cache_module._config_signature is not None # guard: had a real signature + + cfg.unlink() # the config file is deleted entirely, not just edited + monkeypatch.setattr( + ExtensionsConfig, + "resolve_config_path", + classmethod(lambda cls, config_path=None: cfg), + ) + + assert cache_module._is_cache_stale() is False