ewilliams28/turnstone
1
0
Fork 0
mirror of https://github.com/turnstonelabs/turnstone.git synced 2026-08-12 23:12:23 -06:00
Code Issues Packages Projects Releases Wiki Activity

fix: surface MCP server errors in admin UI instead of silent logging (#100)

Browse Source 
* fix: surface MCP server errors in admin UI instead of silent logging

get_server_status() hardcoded error="" — connection and refresh failures
were logged but never surfaced to the admin panel.

Added _last_error dict to MCPClientManager: set on failure (connect,
refresh, periodic refresh, notification handler), cleared on success,
cleaned up on remove. Read in get_server_status().

Admin UI: error tooltip on list row status span, error text in red
in detail modal per-node list. Schema already had the field.

6 new tests for error tracking lifecycle.

* feat: add turnstone_mcp_server_errors Prometheus gauge

Exposes the count of MCP servers currently in error state via
/metrics for alerting and reliability tracking.

* fix: address copilot review — sanitize error strings, clear on notification success

- Add _set_error() helper: strips newlines, truncates to 256 chars
- All error-setting sites now use _set_error() for consistent sanitization
- Notification handler clears _last_error on successful refresh (fixes
  stale error for push-notification servers that skip _periodic_refresh)
This commit is contained in:
Patrick Buckley
2026-03-15 17:12:13 -07:00
committed by GitHub
parent 0d77d65266
commit f5af4875ba
5 changed files with 103 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/test_mcp_hot_reload.py
+50
View File
@@ -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
# ---------------------------------------------------------------------------
turnstone/console/static/admin.js
+17 -6
View File
@@ -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) {
'<span class="admin-col admin-col-mprompts">' +
promptsVal +
"</span>" +
'<span class="admin-col admin-col-mstatus"><span class="' +
'<span class="admin-col admin-col-mstatus"' +
(firstError ? ' title="' + escapeHtml(firstError) + '"' : "") +
'><span class="' +
dotClass +
'" aria-hidden="true"></span>' +
escapeHtml(statusText) +
@@ -3102,9 +3108,7 @@ function _openMcpDetail(s) {
var dot = ns.connected
? '<span class="mcp-status-dot connected"></span>'
: '<span class="mcp-status-dot error"></span>';
html +=
"<li>" +
dot +
var nodeInfo =
escapeHtml(nodeIds[j]) +
" — " +
(ns.tools || 0) +
@@ -3112,7 +3116,14 @@ function _openMcpDetail(s) {
(ns.resources || 0) +
" resources, " +
(ns.prompts || 0) +
" prompts</li>";
" prompts";
if (ns.error) {
nodeInfo +=
'<br><span style="color:var(--red);font-size:11px">' +
escapeHtml(ns.error) +
"</span>";
}
html += "<li>" + dot + nodeInfo + "</li>";
}
html += "</ul>";
}
turnstone/core/mcp_client.py
+30 -5
View File
@@ -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."""
turnstone/core/metrics.py
+5
View File
@@ -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)
turnstone/server.py
+1
View File
@@ -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,