mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-30 15:10:41 +00:00
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
PeaceMaker-best
|
90d466c917
|
feat(mcp): propagate cache resets across workers (#6126)
* feat(mcp): propagate cache resets across workers Signed-off-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com> * docs: keep MCP reset guidance within instruction budgets --------- Signed-off-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com> Co-authored-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Hyeonsang Cho
|
174c6f7fd3
|
fix(mcp): run lazy MCP discovery once when initialization itself raises (#5836)
* fix(mcp): run lazy MCP discovery once when initialization itself raises get_cached_mcp_tools() wrapped the whole lazy-initialization call in the `except RuntimeError` that was only meant to catch asyncio.get_event_loop() raising when no loop is set. run_until_complete() and Future.result() re-raise the coroutine's own exception, so a RuntimeError from discovery — e.g. the deliberately re-raised McpTaskConfigurationError — took the same fallback and ran asyncio.run(initialize_mcp_tools()) a second time, respawning every stdio server and re-fetching OAuth tokens before giving up. Inside a running loop that fallback fails with "asyncio.run() cannot be called from a running event loop", so the logged traceback hid the real cause. Resolve the loop first, in its own try/except, and drive initialization exactly once: asyncio.run when no loop is set or the current loop is closed, a worker thread when the loop is running, run_until_complete otherwise. Any failure is logged once and returns [] as before. Regression tests pin one discovery call and the real logged cause with and without a running loop, and keep both fallbacks (no loop set, closed loop) covered. * test(mcp): surface worker-thread failures in the no-loop fallback test Thread.join() does not propagate exceptions, so a regression of the asyncio.run fallback failed this test with a bare KeyError: 'result' instead of the real traceback. Capture sys.exc_info() in the worker and re-raise it on the main thread before the assertions. |
||
|
spud
|
8204dcbb20
|
fix(mcp): invalidate cache only for effective MCP config changes (#5750)
* fix(mcp): avoid cache resets for unrelated config changes extensions_config.json carries MCP servers, skills and middleware settings, but the MCP cache treated any whole-file signature change as MCP state change, so toggling a skill retired every cached tool and closed all pooled sessions. Record a snapshot of the effective MCP slice (enabled servers' parsed config plus mcpInterceptors) and retire MCP state only when that slice changes. Hand the exact config revision to tool discovery so a change during discovery cannot publish tools built from a superseded revision, and add a non-initializing refresh entry point so disabling the last enabled server still converges the cache and pool on the next tool assembly. * docs(mcp): align cache guide and restore error diagnostics Review feedback on #5750 flagged three non-blocking issues: the MCP module guide still described the pre-semantic-snapshot invalidation contract; the tools.py sanitization covered every exception in the MCP block, discarding diagnostics for failures that cannot carry resolved credentials; and _is_cache_stale()'s docstring presented it as a pure predicate despite adopting a new file signature. Narrow the type-only logging to ExtensionsConfig.from_file() and restore logger.exception() for tool caching, pool cleanup and plugin filtering, with a regression test asserting the message and traceback survive. Document the effective-MCP-slice comparison, the stable config handoff to discovery, refresh_mcp_cache_if_active() and the _initialized_without_config handling in the module guide, and note the signature-adoption side effect in the docstring. * fix(mcp): unify config normalization and verify signatures Review follow-up on #5750. - Normalize mcpInterceptors (bare string == single-element list; missing key == empty list) and the type/transport alias through one shared module used by the tools cache, the interceptor builder and the durable-task config snapshot, so equivalent spellings no longer retire tools/sessions or raise McpTaskConfigurationError. - Treat a resolved-path switch as a signal to re-read the effective MCP slice instead of an invalidation reason: an identical slice adopts the new path and signature and keeps pooled sessions. - Refuse to publish or adopt a signature whose sha256 digest is missing ((mtime, size, None) from a file that could not be read). - Sync the English and Chinese MCP user docs with the effective-config comparison, and document that the Gateway admin reset stays unconditional. * docs(mcp): note the unverifiable-signature fail-closed branch A present config file whose content cannot be hashed is treated as stale, unlike a deleted file which keeps the last-known-good cache. Record that asymmetry in the module guide. * docs(mcp): scope the no-hash rationale to never-initialized processes The cost-free early return in refresh_mcp_cache_if_active() only covers a process that never published an MCP cache. A process that did publish one (including an empty cache from the zero-server or LangGraph Studio lazy-init path) still pays one stat+sha256 per tool assembly. Correct the rationale in the docstring and the matching comment in get_available_tools(). |
||
|
goloisme
|
683d146a30
|
fix(mcp): MCP cache re-initialization broken by cross-loop asyncio.Lock (#5062)
* Fixes #5060: P1 snapshot config before loading, P2 RLock for sync path P1: Config changes during initialization can permanently cache stale tools. - Snapshot _config_path and _config_signature BEFORE await get_mcp_tools() - Compare AFTER get_mcp_tools() completes using _current_config_state() - If config changed during loading, discard stale result and retry - Prevents publishing old tools with new signature, which would make _is_cache_stale() permanently return False P2: Module-level asyncio.Lock still fails across event loops after real contention. - _init_lock = threading.RLock() for sync path (reentrant, prevents races) - _async_init_lock = asyncio.Lock() for async init serialization - reset_mcp_tools_cache() now acquires _init_lock for serialization Also fixes test P3: removed duplicated test bodies that leaked state between tests. * fix(mcp): make cache initialization cross-loop safe - Replace the module-level asyncio.Lock with thread-safe generation claiming - Snapshot config state before/after MCP loading and discard stale results - Keep reset state changes short and non-blocking for async endpoints - Add regression coverage for contended cross-loop init, config rewrites during load, and reset while init is in flight * fix: release MCP init claim on cancellation Release the in-flight generation claim from a cancellation-safe finally block so cancelling the task that owns initialization does not strand future callers. Add regression coverage for cancelling the owner and then reinitializing successfully. * fix(mcp): retire session pool before cache reset release Prevent a concurrent MCP cache initializer from publishing tool wrappers bound to the session-pool singleton that reset_mcp_tools_cache() is already retiring. Add regression coverage for that interleaving. * fix(mcp): retire session pool on stale cache invalidation * fix(mcp): retire pool on init discard --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Daoyuan Li
|
3ed2e1f1d9
|
fix(config): close out #4124 review follow-ups (shared signature helper, resolve_config_path None contract) (#4275)
* fix(config): close out #4124 review follow-ups Extracts the (mtime, size, sha256) content-signature helper that was duplicated between config/app_config.py and mcp/cache.py into a new config/file_signature.py, and fixes ExtensionsConfig.resolve_config_path() to return None instead of raising FileNotFoundError when an explicit config_path argument or DEER_FLOW_EXTENSIONS_CONFIG_PATH points at a file that has since been deleted -- the exact resolution mode Docker dev/prod uses per AGENTS.md, so the MCP tools-cache staleness check could raise instead of degrading to "not stale". Both were flagged by willem-bd in review on #4124 and explicitly deferred there ("flagging for visibility", "leaving the extraction as the follow-up you suggested"). * fix(config): keep explicit extensions-config paths fail-loud resolve_config_path() previously turned every missing-file case into a clean None, including an explicit config_path argument or DEER_FLOW_EXTENSIONS_CONFIG_PATH (the exact mode Docker dev/prod uses). That silently downgrades a bad Docker mount, typo, or deleted production config to "no extensions" instead of surfacing the misconfiguration, per fancyboi999's review [P1] and willem-bd's follow-up notes on this PR. Restores FileNotFoundError for the two explicit modes (config_path argument, DEER_FLOW_EXTENSIONS_CONFIG_PATH); only the fallback search mode (no explicit path/env var, nothing found in the usual locations) still returns None, since that is the legitimate "extensions were never configured" case. The one caller that needs the old fail-soft behavior -- the MCP tools-cache staleness check, which re-resolves the path on every get_cached_mcp_tools() call -- gets a narrow, local catch instead (deerflow.mcp.cache._resolve_config_path) so a config file going missing mid-run still degrades the cache to "not stale" rather than crashing a hot per-request path. Also hoists the double os.getenv() read in the env-var branch into a local, per willem-bd's nit. Adds resolver-level tests for both explicit-path and env-var raises, a dedicated search-mode None regression test, and updates the existing MCP-cache docstrings to describe the corrected split. |
||
|
Daoyuan Li
|
cdefd4a8aa
|
fix(mcp): invalidate tools cache on config content + path, not just newer mtime (#4124)
* fix(mcp): invalidate tools cache on config content + path, not just newer mtime The MCP tools cache invalidated only on a strict extensions-config mtime `>` comparison and tracked no resolved config path, so `_is_cache_stale()` missed: - content changes with an unchanged mtime (same-second edits; object-store / network mounts that do not bump mtime); - content changes with a backward mtime (git checkout, cp -p / backup restore, tar / rsync preserving timestamps); - a resolved-path switch to a different config file with mtime <= the recorded value (structurally invisible — no path was tracked at all). On multi-worker (uvicorn/gunicorn) or stale-mtime deployments this leaves the LangGraph-embedded runtime and every non-writer worker serving stale MCP tools after `PUT /api/mcp/config`, breaking the module's documented promise that changes made through the Gateway API are reflected in the embedded runtime. Record the resolved config path and a `(mtime, size, sha256)` content signature at initialization and invalidate when the path OR the signature differs (`!=`), mirroring `config/app_config.py::get_app_config()` so the two runtime-editable config files share one content-based staleness signal. The per-call stat was already paid, and the small-file sha256 matches the cost app_config pays per request. The "config missing / not yet initialized" no-op behavior and the cache reset endpoint are preserved. Adds backend/tests/test_mcp_cache.py covering all three failure modes plus unchanged-file and forward-edit sanity cases. Also folds in an incidental backend/AGENTS.md doc-sync: the restart-required field list was missing `scheduler` and `run_ownership`, both present in reload_boundary.py::STARTUP_ONLY_FIELDS. * test(mcp): pin cache staleness contracts raised in review Two review observations on the content-signature cache fix, both raised as non-blocking design questions rather than bugs: - Whether relying on mtime+size alone (skipping the sha256) could ever be "optimized" back in, reopening the narrow same-second / identical-length swap gap the signature was built to close. - Whether the extensions config being deleted entirely after a successful init leaves the cache in a defined state, since current_signature flips to None and _is_cache_stale() returns False. Neither is a behavior change: both were already the intended contract, preserved verbatim from the pre-fix mtime-only code (which also returned False once the file could no longer be stat-ed). Record the reasoning inline and add regression tests that pin each contract so a future change cannot alter either silently: - test_same_mtime_same_size_swap_is_stale: a same-length server-name swap that leaves mtime AND size unchanged (the precise scenario from review, sharper than the existing same-mtime test, which also changes size) is still caught only because the sha256 is computed unconditionally. - test_config_deleted_after_init_is_not_stale: deleting the config file after init keeps the cache serving its last-known-good MCP tools instead of invalidating into an unconfigured state. Both new tests were confirmed to fail against a deliberately reintroduced version of the regression they guard (hash short-circuit / removed None-guard), then confirmed to pass against the real code. |