diff --git a/tests/test_mcp_hot_reload.py b/tests/test_mcp_hot_reload.py index e26167b9..c75ced37 100644 --- a/tests/test_mcp_hot_reload.py +++ b/tests/test_mcp_hot_reload.py @@ -235,6 +235,56 @@ class TestGetAllServerStatus: assert statuses["down"]["tools"] == 0 +# --------------------------------------------------------------------------- +# Error tracking (_last_error) +# --------------------------------------------------------------------------- + + +class TestErrorTracking: + def test_get_server_status_returns_error(self) -> None: + """Error stored in _last_error flows through get_server_status.""" + mgr = MCPClientManager({"test": {"command": "echo"}}) + mgr._last_error["test"] = "Connection refused" + status = mgr.get_server_status("test") + assert status["error"] == "Connection refused" + assert status["connected"] is False + + def test_no_error_by_default(self) -> None: + """Default error is empty string.""" + mgr = MCPClientManager({"test": {"command": "echo"}}) + status = mgr.get_server_status("test") + assert status["error"] == "" + + def test_error_cleared_after_pop(self) -> None: + """Clearing _last_error makes get_server_status return empty.""" + mgr = MCPClientManager({"test": {"command": "echo"}}) + mgr._last_error["test"] = "Connection refused" + mgr._last_error.pop("test", None) + status = mgr.get_server_status("test") + assert status["error"] == "" + + def test_error_cleared_on_remove(self) -> None: + """remove_server_sync cleans up _last_error entry.""" + mgr = MCPClientManager({"test": {"command": "echo"}}) + mgr._last_error["test"] = "Connection refused" + mgr.remove_server_sync("test") + assert "test" not in mgr._last_error + + def test_all_server_status_includes_errors(self) -> None: + """get_all_server_status propagates per-server errors.""" + mgr = MCPClientManager({"alpha": {}, "bravo": {}}) + mgr._last_error["alpha"] = "Timeout" + statuses = mgr.get_all_server_status() + assert statuses["alpha"]["error"] == "Timeout" + assert statuses["bravo"]["error"] == "" + + def test_error_does_not_leak_across_servers(self) -> None: + """Error on one server does not affect another.""" + mgr = MCPClientManager({"a": {}, "b": {}}) + mgr._last_error["a"] = "Failed" + assert mgr.get_server_status("b")["error"] == "" + + # --------------------------------------------------------------------------- # reconcile_sync # --------------------------------------------------------------------------- diff --git a/turnstone/console/static/admin.js b/turnstone/console/static/admin.js index 56404cb9..46365436 100644 --- a/turnstone/console/static/admin.js +++ b/turnstone/console/static/admin.js @@ -2681,6 +2681,7 @@ function _renderMcpServers(items) { var nodeIds = Object.keys(statusEntries); var anyConnected = false; var anyError = false; + var firstError = ""; var totalTools = 0, totalRes = 0, totalPrompts = 0; @@ -2692,7 +2693,10 @@ function _renderMcpServers(items) { totalRes += ns.resources || 0; totalPrompts += ns.prompts || 0; } - if (ns.error) anyError = true; + if (ns.error) { + anyError = true; + if (!firstError) firstError = ns.error; + } } var dotClass = "mcp-status-dot disabled"; @@ -2769,7 +2773,9 @@ function _renderMcpServers(items) { '' + promptsVal + "" + - '' + escapeHtml(statusText) + @@ -3102,9 +3108,7 @@ function _openMcpDetail(s) { var dot = ns.connected ? '' : ''; - html += - "
  • " + - dot + + var nodeInfo = escapeHtml(nodeIds[j]) + " — " + (ns.tools || 0) + @@ -3112,7 +3116,14 @@ function _openMcpDetail(s) { (ns.resources || 0) + " resources, " + (ns.prompts || 0) + - " prompts
  • "; + " prompts"; + if (ns.error) { + nodeInfo += + '
    ' + + escapeHtml(ns.error) + + ""; + } + html += "
  • " + dot + nodeInfo + "
  • "; } html += ""; } diff --git a/turnstone/core/mcp_client.py b/turnstone/core/mcp_client.py index 96ec6abb..8c71b950 100644 --- a/turnstone/core/mcp_client.py +++ b/turnstone/core/mcp_client.py @@ -109,6 +109,9 @@ class MCPClientManager: # Config-file servers loaded at startup are NOT in this set and # will never be removed by reconcile_sync. self._db_managed: set[str] = set() + # Per-server last-error tracking (set on failure, cleared on success) + self._last_error: dict[str, str] = {} + self._MAX_ERROR_LEN = 256 # Per-server tool storage for surgical refresh self._per_server_tools: dict[str, list[dict[str, Any]]] = {} @@ -171,8 +174,9 @@ class MCPClientManager: for name, cfg in self._server_configs.items(): try: await self._connect_one(name, cfg) - except Exception: + except Exception as exc: log.warning("Failed to connect MCP server '%s'", name, exc_info=True) + self._set_error(name, f"{type(exc).__name__}: {exc}") self._connected.set() @@ -244,8 +248,10 @@ class MCPClientManager: elif isinstance(root, mcp_types.PromptListChangedNotification): log.info("Received prompts/list_changed from '%s'", name) await self._refresh_server_prompts(name) - except Exception: + self._last_error.pop(name, None) + except Exception as exc: log.warning("Refresh after notification failed for '%s'", name, exc_info=True) + self._set_error(name, f"Refresh failed: {exc}") try: session = await stack.enter_async_context( @@ -372,6 +378,9 @@ class MCPClientManager: except Exception: log.warning("Prompt sync after connect failed for '%s'", name, exc_info=True) + # Connection succeeded — clear any previous error + self._last_error.pop(name, None) + # -- tool refresh -------------------------------------------------------- def _rebuild_tools(self) -> None: @@ -428,6 +437,7 @@ class MCPClientManager: added, removed = await self._refresh_server_tools(name) await self._refresh_server_resources(name) await self._refresh_server_prompts(name) + self._last_error.pop(name, None) return added, removed async def _refresh_all( @@ -456,8 +466,9 @@ class MCPClientManager: continue added, removed = await self._refresh_server(name) results[name] = (added, removed) - except Exception: + except Exception as exc: log.warning("Refresh failed for MCP server '%s'", name, exc_info=True) + self._set_error(name, f"Refresh failed: {exc}") results[name] = ([], []) # Final sync to clean up templates from servers that are no longer connected @@ -497,8 +508,10 @@ class MCPClientManager: await self._refresh_server_resources(name) if not self._supports_prompt_list_changed.get(name, False): await self._refresh_server_prompts(name) - except Exception: + self._last_error.pop(name, None) + except Exception as exc: log.warning("Periodic refresh failed for '%s'", name, exc_info=True) + self._set_error(name, f"Periodic refresh failed: {exc}") await asyncio.sleep(self._refresh_interval) # -- resource refresh ---------------------------------------------------- @@ -958,6 +971,7 @@ class MCPClientManager: self._supports_resource_list_changed.pop(name, None) self._supports_prompts.pop(name, None) self._supports_prompt_list_changed.pop(name, None) + self._last_error.pop(name, None) # Rebuild merged state (serialized with notification handlers) self._rebuild_tools() self._rebuild_resources() @@ -979,6 +993,7 @@ class MCPClientManager: self._supports_resource_list_changed.pop(name, None) self._supports_prompts.pop(name, None) self._supports_prompt_list_changed.pop(name, None) + self._last_error.pop(name, None) self._rebuild_tools() self._rebuild_resources() self._rebuild_prompts() @@ -992,6 +1007,11 @@ class MCPClientManager: log.info("Removed MCP server '%s'", name) return was_connected + def _set_error(self, name: str, msg: str) -> None: + """Store a sanitized error string for a server.""" + clean = msg.replace("\n", " ").replace("\r", "") + self._last_error[name] = clean[: self._MAX_ERROR_LEN] + def get_server_status(self, name: str) -> dict[str, Any]: """Return live status for a single server, including config details.""" connected = name in self._sessions @@ -1002,7 +1022,7 @@ class MCPClientManager: "tools": len(self._per_server_tools.get(name, [])) if connected else 0, "resources": len(self._per_server_resources.get(name, [])) if connected else 0, "prompts": len(self._per_server_prompts.get(name, [])) if connected else 0, - "error": "", + "error": self._last_error.get(name, ""), "transport": transport, "command": cfg.get("command", "") if transport == "stdio" else "", "url": cfg.get("url", "") if transport != "stdio" else "", @@ -1123,6 +1143,11 @@ class MCPClientManager: def server_count(self) -> int: return len(self._sessions) + @property + def error_count(self) -> int: + """Number of servers currently in error state.""" + return len(self._last_error) + @property def server_names(self) -> list[str]: """Return configured server names.""" diff --git a/turnstone/core/metrics.py b/turnstone/core/metrics.py index 7d697471..6175e313 100644 --- a/turnstone/core/metrics.py +++ b/turnstone/core/metrics.py @@ -402,6 +402,11 @@ class MetricsCollector: "Number of MCP prompts available", mcp_info.get("prompts", 0), ) + gauge( + "turnstone_mcp_server_errors", + "Number of MCP servers currently in error state", + mcp_info.get("errors", 0), + ) lines.append("") # trailing newline return "\n".join(lines) diff --git a/turnstone/server.py b/turnstone/server.py index acfd28b3..c200ff31 100644 --- a/turnstone/server.py +++ b/turnstone/server.py @@ -1006,6 +1006,7 @@ async def metrics_endpoint(request: Request) -> Response: "servers": mc.server_count, "resources": mc.resource_count, "prompts": mc.prompt_count, + "errors": mc.error_count, } content = _metrics.generate_text( workstream_states=states,