From 0fac057fa8ead74c39339f16705c4ba7d61fcb43 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 12 Jun 2026 09:29:10 +0300 Subject: [PATCH] fix(runtime): serve indexed tools when StateView per-server cache is empty Approving (unquarantining) a server triggers a disconnect/reconnect cycle. The supervisor clears the per-server StateView tool list on the connection-down event and only repopulates it asynchronously from background discovery, so in the field the StateView can hold zero tools even though the durable search index already indexed the server's tools. handleGetServerTools / GetServerTools serve straight from that volatile StateView snapshot, so the Tools tab showed "Tools: 0 / No tools available" for a connected server with 15 indexed tools. Add a defensive fallback in Runtime.GetServerTools: when StateView reports no tools for a known server, serve the server's tools from the authoritative search index (normalizing names to the bare tool name to match the approval-record / enrichment convention). A populated StateView is still preferred (freshest source) and a genuinely empty server still reports empty. Related MCP-2083 --- .../runtime/get_server_tools_fallback_test.go | 119 ++++++++++++++++++ internal/runtime/runtime.go | 43 +++++++ 2 files changed, 162 insertions(+) create mode 100644 internal/runtime/get_server_tools_fallback_test.go diff --git a/internal/runtime/get_server_tools_fallback_test.go b/internal/runtime/get_server_tools_fallback_test.go new file mode 100644 index 000000000..3e996f9ba --- /dev/null +++ b/internal/runtime/get_server_tools_fallback_test.go @@ -0,0 +1,119 @@ +package runtime + +import ( + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime/stateview" +) + +// seedStateViewServer registers a server in the supervisor StateView with the +// given tool set, simulating a connected server whose StateView per-server tool +// cache holds exactly `tools` (possibly empty). +func seedStateViewServer(t *testing.T, rt *Runtime, name string, connected bool, tools []stateview.ToolInfo) { + t.Helper() + sv := rt.supervisor.StateView() + if sv == nil { + t.Fatal("StateView not available on test runtime") + } + sv.UpdateServer(name, func(status *stateview.ServerStatus) { + status.Connected = connected + status.State = "ready" + status.Tools = tools + status.ToolCount = len(tools) + }) +} + +// TestGetServerTools_FallsBackToIndexWhenStateViewEmpty reproduces MCP-2083: +// after approving (unquarantining) a server, the per-server StateView tool list +// is transiently cleared by the disconnect/reconnect cycle, but the durable +// search index still holds the server's tools. GetServerTools must never serve +// an empty list when the index has tools for a known server. +func TestGetServerTools_FallsBackToIndexWhenStateViewEmpty(t *testing.T) { + rt := newTestRuntime(t) + + const server = "com.googleapis.sqladmin/mcp" + // Index holds 2 tools (analogue of the 15 indexed after approval). + if err := rt.indexManager.BatchIndexTools([]*config.ToolMetadata{ + {ServerName: server, Name: "list_instances", Description: "List instances", ParamsJSON: `{"type":"object"}`}, + {ServerName: server, Name: "get_instance", Description: "Get instance"}, + }); err != nil { + t.Fatalf("failed to seed index: %v", err) + } + + // StateView has the server present and connected, but with an empty tool list + // (the bug state right after the unquarantine reconnect). + seedStateViewServer(t, rt, server, true, nil) + + tools, err := rt.GetServerTools(server) + if err != nil { + t.Fatalf("GetServerTools returned error: %v", err) + } + if len(tools) != 2 { + t.Fatalf("expected fallback to index (2 tools), got %d: %#v", len(tools), tools) + } + + got := map[string]bool{} + for _, tool := range tools { + name, _ := tool["name"].(string) + got[name] = true + if sn, _ := tool["server_name"].(string); sn != server { + t.Fatalf("expected server_name %q, got %q", server, sn) + } + } + for _, want := range []string{"list_instances", "get_instance"} { + if !got[want] { + t.Fatalf("missing tool %q in fallback result: %#v", want, got) + } + } +} + +// TestGetServerTools_PrefersStateViewWhenPopulated guards against the fallback +// overriding a populated StateView: when StateView already has tools, the index +// is not consulted (StateView is the freshest source). +func TestGetServerTools_PrefersStateViewWhenPopulated(t *testing.T) { + rt := newTestRuntime(t) + + const server = "srv" + if err := rt.indexManager.BatchIndexTools([]*config.ToolMetadata{ + {ServerName: server, Name: "stale_indexed_tool", Description: "stale"}, + }); err != nil { + t.Fatalf("failed to seed index: %v", err) + } + + seedStateViewServer(t, rt, server, true, []stateview.ToolInfo{ + {Name: "live_a", Description: "A"}, + {Name: "live_b", Description: "B"}, + }) + + tools, err := rt.GetServerTools(server) + if err != nil { + t.Fatalf("GetServerTools returned error: %v", err) + } + if len(tools) != 2 { + t.Fatalf("expected StateView tools (2), got %d: %#v", len(tools), tools) + } + for _, tool := range tools { + if name, _ := tool["name"].(string); name == "stale_indexed_tool" { + t.Fatalf("fallback incorrectly overrode a populated StateView with stale index data") + } + } +} + +// TestGetServerTools_EmptyStateViewAndIndexReturnsEmpty ensures a genuinely +// empty server (no StateView tools, no index tools) still reports empty — the +// fallback must not invent tools. +func TestGetServerTools_EmptyStateViewAndIndexReturnsEmpty(t *testing.T) { + rt := newTestRuntime(t) + + const server = "empty-srv" + seedStateViewServer(t, rt, server, true, nil) + + tools, err := rt.GetServerTools(server) + if err != nil { + t.Fatalf("GetServerTools returned error: %v", err) + } + if len(tools) != 0 { + t.Fatalf("expected empty result for a server with no tools, got %d: %#v", len(tools), tools) + } +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index e769cfe90..340bd20f5 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -2111,6 +2111,49 @@ func (r *Runtime) GetServerTools(serverName string) ([]map[string]interface{}, e tools = append(tools, toolMap) } + // Defensive fallback (MCP-2083): the per-server StateView tool list is volatile + // derived state — it is cleared on disconnect (supervisor clears Tools on a + // connection-down event) and only repopulated asynchronously by background + // discovery. Approving a quarantined server triggers a disconnect/reconnect + // cycle, and in the field this can leave StateView holding zero tools even + // though the durable search index already indexed the server's tools. Serving + // that empty snapshot makes the Tools tab show "No tools available" for a + // connected server that demonstrably has tools. When StateView reports no tools + // for a server, fall back to the authoritative search index so we never serve + // an empty list when indexed tools exist. + if len(tools) == 0 && r.indexManager != nil { + if indexed, err := r.indexManager.GetToolsByServer(serverName); err == nil && len(indexed) > 0 { + tools = make([]map[string]interface{}, 0, len(indexed)) + for _, tool := range indexed { + // Index stores the full tool name; normalize to the bare tool name + // (no "server:" prefix) to match the StateView/approval-record + // convention used by the enrichment layer. + name := tool.Name + if idx := strings.Index(name, ":"); idx != -1 { + name = name[idx+1:] + } + toolMap := map[string]interface{}{ + "name": name, + "description": tool.Description, + "server_name": serverName, + } + if tool.ParamsJSON != "" { + var inputSchema map[string]interface{} + if err := json.Unmarshal([]byte(tool.ParamsJSON), &inputSchema); err == nil { + toolMap["inputSchema"] = inputSchema + } + } + if tool.Annotations != nil { + toolMap["annotations"] = tool.Annotations + } + tools = append(tools, toolMap) + } + r.logger.Debug("GetServerTools: StateView empty, served tools from search index fallback", + zap.String("server", serverName), + zap.Int("tool_count", len(tools))) + } + } + return tools, nil }