diff --git a/src/backend/base/langflow/api/utils/mcp/default_servers.py b/src/backend/base/langflow/api/utils/mcp/default_servers.py index ab393cd250..0111938567 100644 --- a/src/backend/base/langflow/api/utils/mcp/default_servers.py +++ b/src/backend/base/langflow/api/utils/mcp/default_servers.py @@ -1,5 +1,7 @@ """Orchestrator that auto-installs the default MCP servers (registry-driven) for every user.""" +import platform + from fastapi import HTTPException from lfx.log.logger import logger from lfx.services.deps import get_settings_service @@ -17,6 +19,35 @@ from langflow.services.database.models.user.model import User from langflow.services.deps import get_service from langflow.services.schema import ServiceType +# Metadata fields the spec authors. We compare these (and only these) when +# deciding whether a persisted auto-configured entry has drifted from the +# canonical spec — extra metadata keys (added by future API extensions or +# user tooling) must not trigger spurious reconciliations. +_SPEC_OWNED_METADATA_KEYS: tuple[str, ...] = ( + "description", + "auto_configured", + "langflow_internal", + "startup_timeout_seconds", +) + + +def _is_persisted_payload_in_sync(persisted: dict, canonical: dict) -> bool: + """Return True iff the persisted MCP entry already matches the canonical spec. + + Compares only the fields the spec controls (command, args, env, and the + spec-owned metadata keys). Extra fields on either side are ignored so we + don't thrash on derived / user-added properties. + """ + if persisted.get("command") != canonical["command"]: + return False + if list(persisted.get("args") or []) != list(canonical["args"]): + return False + if dict(persisted.get("env") or {}) != dict(canonical["env"]): + return False + persisted_meta = persisted.get("metadata") or {} + canonical_meta = canonical["metadata"] + return all(persisted_meta.get(key) == canonical_meta.get(key) for key in _SPEC_OWNED_METADATA_KEYS) + def _build_server_payload(spec: DefaultMcpServerSpec) -> dict: """Build the persisted payload, re-validating via MCPServerConfig. @@ -70,15 +101,41 @@ async def auto_configure_default_mcp_servers(session: AsyncSession) -> None: payload = _build_server_payload(spec) for user in users: try: - existing = await get_server_list(user, session, storage_service, settings_service) - if server_name in existing.get("mcpServers", {}): - await logger.adebug( - "default_mcp_server_skipped", + existing_servers = (await get_server_list(user, session, storage_service, settings_service)).get( + "mcpServers", {} + ) + persisted = existing_servers.get(server_name) + if persisted is not None: + persisted_meta = persisted.get("metadata") or {} + if not persisted_meta.get("auto_configured"): + # User-owned entry (no `auto_configured` flag): never overwrite. + await logger.adebug( + "default_mcp_server_skipped", + user_id=str(user.id), + server_name=server_name, + reason="user_owned", + ) + continue + if _is_persisted_payload_in_sync(persisted, payload): + await logger.adebug( + "default_mcp_server_skipped", + user_id=str(user.id), + server_name=server_name, + reason="already_in_sync", + ) + continue + # Spec drifted (e.g. user upgraded across a commit that changed the + # canonical payload). Reconcile by overwriting with the new spec — + # `auto_configured: True` was our marker that the entry is ours. + # Platform context lets ops correlate reconciliations against the + # reported OS when the prior version's payload is too short for + # that OS (notably first-run npx timeouts on Windows). + await logger.ainfo( + "default_mcp_server_reconciled", user_id=str(user.id), server_name=server_name, - reason="already_exists", + platform=platform.system(), ) - continue await update_server( server_name=server_name, server_config=payload, diff --git a/src/backend/tests/unit/api/utils/mcp/test_default_servers_orchestrator.py b/src/backend/tests/unit/api/utils/mcp/test_default_servers_orchestrator.py index 1b09454050..3871e4ae0c 100644 --- a/src/backend/tests/unit/api/utils/mcp/test_default_servers_orchestrator.py +++ b/src/backend/tests/unit/api/utils/mcp/test_default_servers_orchestrator.py @@ -142,6 +142,99 @@ class TestIdempotency: patched_orchestrator_deps.update_server.assert_not_called() +class TestReconcileAutoConfiguredEntries: + """Reconcile auto-configured entries when the canonical spec drifts. + + When the canonical spec evolves (e.g. a new `metadata.startup_timeout_seconds` + field is added), users who installed Langflow on an earlier build keep a + stale persisted payload — the orchestrator's old skip-if-exists behavior + would lock them out of fixes forever (notably: the 60s opt-in needed to + survive the first `npx -y @wonderwhy-er/desktop-commander@latest` download + on Windows + slow networks). + + Reconciliation is gated on `metadata.auto_configured == True`: only entries + that we wrote can be overwritten. Anything missing the flag (e.g. the user + swapped in their own server) is preserved unconditionally. + """ + + async def test_should_reconcile_auto_configured_entry_when_persisted_payload_diverges_from_spec( + self, fake_session_with_users, patched_orchestrator_deps + ): + """Stale entry from a pre-`startup_timeout_seconds` build must be updated.""" + patched_orchestrator_deps.get_server_list.return_value = { + "mcpServers": { + "shell-execution": { + "command": "npx", + "args": ["-y", "@wonderwhy-er/desktop-commander@latest"], + "env": {}, + "metadata": { + "description": ( + "Cross-platform shell execution + filesystem control (wonderwhy-er/desktop-commander)." + ), + "auto_configured": True, + "langflow_internal": True, + # NOTE: no startup_timeout_seconds — this is the stale field. + }, + } + } + } + + await auto_configure_default_mcp_servers(fake_session_with_users) + + patched_orchestrator_deps.update_server.assert_awaited_once() + cfg = patched_orchestrator_deps.update_server.await_args.kwargs["server_config"] + assert cfg["metadata"].get("startup_timeout_seconds") == 60 + + async def test_should_not_overwrite_entry_lacking_auto_configured_flag( + self, fake_session_with_users, patched_orchestrator_deps + ): + """User-owned entry (no `auto_configured` flag) must be preserved.""" + patched_orchestrator_deps.get_server_list.return_value = { + "mcpServers": { + "shell-execution": { + "command": "npx", + "args": ["-y", "@user/custom-fork"], + "env": {}, + "metadata": {"description": "user-owned override"}, + } + } + } + + await auto_configure_default_mcp_servers(fake_session_with_users) + + patched_orchestrator_deps.update_server.assert_not_called() + + async def test_should_skip_when_auto_configured_entry_payload_already_matches_spec( + self, fake_session_with_users, patched_orchestrator_deps + ): + """No-op when the persisted payload already matches the canonical spec. + + Avoids needless disk writes and noisy logs on every Langflow startup. + """ + from langflow.api.utils.mcp.default_servers_specs import DEFAULT_MCP_SERVERS + + spec = DEFAULT_MCP_SERVERS["shell-execution"] + patched_orchestrator_deps.get_server_list.return_value = { + "mcpServers": { + "shell-execution": { + "command": spec.config.command, + "args": list(spec.config.args), + "env": dict(spec.config.env), + "metadata": { + "description": spec.description, + "auto_configured": True, + "langflow_internal": True, + "startup_timeout_seconds": spec.startup_timeout_seconds, + }, + } + } + } + + await auto_configure_default_mcp_servers(fake_session_with_users) + + patched_orchestrator_deps.update_server.assert_not_called() + + class TestRegistryRevalidation: """Defense-in-depth on the registry.