Merge pull request #23063 from BerriAI/litellm_fix_mcp_health_check_trigger

[Fix] UI - MCP Servers: stop health checks triggering on server deletion

ui/litellm-dashboard/src/app/(dashboard)/hooks/mcpServers/useMCPServerHealth.test.ts

@@ -38,7 +38,7 @@ describe("useMCPServerHealth", () => {
     vi.clearAllMocks();
   });
 
-  it("should fetch health status for given server IDs", async () => {
+  it("should fetch health status for all servers", async () => {
     const mockHealthStatuses = [
       { server_id: "server-1", status: "healthy" },
       { server_id: "server-2", status: "unhealthy" },
@@ -46,27 +46,6 @@ describe("useMCPServerHealth", () => {
 
     vi.mocked(networking.fetchMCPServerHealth).mockResolvedValue(mockHealthStatuses);
 
-    const { result } = renderHook(() => useMCPServerHealth(["server-1", "server-2"]), {
-      wrapper,
-    });
-
-    await waitFor(() => {
-      expect(result.current.isSuccess).toBe(true);
-    });
-
-    expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("test-token-123", ["server-1", "server-2"]);
-    expect(result.current.data).toEqual(mockHealthStatuses);
-  });
-
-  it("should fetch health status for all servers when no server IDs provided", async () => {
-    const mockHealthStatuses = [
-      { server_id: "server-1", status: "healthy" },
-      { server_id: "server-2", status: "healthy" },
-      { server_id: "server-3", status: "unhealthy" },
-    ];
-
-    vi.mocked(networking.fetchMCPServerHealth).mockResolvedValue(mockHealthStatuses);
-
     const { result } = renderHook(() => useMCPServerHealth(), {
       wrapper,
     });
@@ -75,30 +54,15 @@ describe("useMCPServerHealth", () => {
       expect(result.current.isSuccess).toBe(true);
     });
 
-    expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("test-token-123", undefined);
+    expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("test-token-123");
     expect(result.current.data).toEqual(mockHealthStatuses);
   });
 
-  it("should handle empty server list", async () => {
-    vi.mocked(networking.fetchMCPServerHealth).mockResolvedValue([]);
-
-    const { result } = renderHook(() => useMCPServerHealth([]), {
-      wrapper,
-    });
-
-    await waitFor(() => {
-      expect(result.current.isSuccess).toBe(true);
-    });
-
-    expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("test-token-123", []);
-    expect(result.current.data).toEqual([]);
-  });
-
   it("should handle errors when fetching health status", async () => {
     const mockError = new Error("Failed to fetch health status");
     vi.mocked(networking.fetchMCPServerHealth).mockRejectedValue(mockError);
 
-    const { result } = renderHook(() => useMCPServerHealth(["server-1"]), {
+    const { result } = renderHook(() => useMCPServerHealth(), {
       wrapper,
     });
 
@@ -116,7 +80,7 @@ describe("useMCPServerHealth", () => {
       accessToken: null,
     } as any);
 
-    const { result } = renderHook(() => useMCPServerHealth(["server-1"]), {
+    const { result } = renderHook(() => useMCPServerHealth(), {
       wrapper,
     });
 
@@ -124,4 +88,18 @@ describe("useMCPServerHealth", () => {
     expect(result.current.status).toBe("pending");
     expect(networking.fetchMCPServerHealth).not.toHaveBeenCalled();
   });
+
+  it("should use a stable query key that does not include server IDs", () => {
+    // Regression test: deleting a server used to pass a changing serverIds array into the
+    // hook, which was embedded in the query key. React Query would see a new key and fire
+    // a health check for every remaining server.
+    //
+    // The fix: the hook takes no serverIds parameter and uses a constant query key, so
+    // deleting (or adding) a server never causes an extra health check request.
+    //
+    // We verify the contract here by confirming the hook accepts no arguments.
+    // The stable-key behaviour is further exercised by mcp_servers.test.tsx.
+    const hookLength = useMCPServerHealth.length;
+    expect(hookLength).toBe(0);
+  });
 });

ui/litellm-dashboard/src/app/(dashboard)/hooks/mcpServers/useMCPServerHealth.ts

@@ -10,11 +10,11 @@ interface MCPServerHealth {
   status: string;
 }
 
-export const useMCPServerHealth = (serverIds?: string[]) => {
+export const useMCPServerHealth = () => {
   const { accessToken } = useAuthorized();
   return useQuery<MCPServerHealth[]>({
-    queryKey: [...mcpServerHealthKeys.lists(), { serverIds }],
-    queryFn: async () => await fetchMCPServerHealth(accessToken!, serverIds),
+    queryKey: mcpServerHealthKeys.lists(),
+    queryFn: async () => await fetchMCPServerHealth(accessToken!),
     enabled: !!accessToken,
     // Refetch health status every 30 seconds to keep it up to date
     refetchInterval: 30000,

ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.test.tsx

@@ -185,9 +185,10 @@ describe("MCPServers", () => {
       expect(getByText("MCP Servers")).toBeInTheDocument();
     });
 
-    // Verify the health check API was called with server IDs
+    // Verify the health check API was called (without a server ID filter — the hook always
+    // fetches health for all servers so the query key stays stable)
     await waitFor(() => {
-      expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("123", ["server-1", "server-2"]);
+      expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("123");
     });
   });
 
@@ -348,4 +349,86 @@ describe("MCPServers", () => {
     // Team B server should not be visible
     expect(screen.queryByText("Team B Server")).not.toBeInTheDocument();
   });
+
+  it("should not trigger an extra health check when the server list changes after deletion", async () => {
+    // Regression test: previously useMCPServerHealth received serverIds derived from the
+    // server list. Deleting a server changed serverIds, which changed the React Query key,
+    // which caused a new health check request for every remaining server.
+    //
+    // Fix: useMCPServerHealth uses a stable, argument-free query key. The component
+    // re-rendering with a shorter server list must NOT produce a second health fetch.
+    const twoServers = [
+      {
+        server_id: "server-1",
+        server_name: "Test Server 1",
+        alias: "test-server-1",
+        url: "https://example.com/mcp",
+        transport: "http",
+        auth_type: "none",
+        created_at: "2024-01-01T00:00:00Z",
+        created_by: "user-1",
+        updated_at: "2024-01-01T00:00:00Z",
+        updated_by: "user-1",
+        teams: [],
+        mcp_access_groups: [],
+      },
+      {
+        server_id: "server-2",
+        server_name: "Test Server 2",
+        alias: "test-server-2",
+        url: "https://example2.com/mcp",
+        transport: "sse",
+        auth_type: "api_key",
+        created_at: "2024-01-02T00:00:00Z",
+        created_by: "user-2",
+        updated_at: "2024-01-02T00:00:00Z",
+        updated_by: "user-2",
+        teams: [],
+        mcp_access_groups: [],
+      },
+    ];
+    const oneServer = twoServers.slice(0, 1);
+
+    // First call returns two servers; second (after deletion) returns one
+    vi.mocked(networking.fetchMCPServers)
+      .mockResolvedValueOnce(twoServers)
+      .mockResolvedValueOnce(oneServer);
+    vi.mocked(networking.fetchMCPServerHealth).mockResolvedValue([
+      { server_id: "server-1", status: "healthy" },
+      { server_id: "server-2", status: "healthy" },
+    ]);
+
+    // Use a shared queryClient with a non-zero gcTime so cached health data survives
+    // the re-render triggered by the server list refresh
+    const queryClient = new QueryClient({
+      defaultOptions: { queries: { retry: false, gcTime: 60_000 } },
+    });
+
+    const { rerender } = render(
+      <QueryClientProvider client={queryClient}>
+        <MCPServers {...defaultProps} />
+      </QueryClientProvider>,
+    );
+
+    // Wait for the initial health fetch to complete
+    await waitFor(() => {
+      expect(networking.fetchMCPServerHealth).toHaveBeenCalledTimes(1);
+    });
+
+    // Simulate what happens after a server is deleted: the server list query is
+    // refetched (returns oneServer), causing the component to re-render with the
+    // shorter list.
+    await act(async () => {
+      await queryClient.invalidateQueries({ queryKey: ["mcpServers"] });
+    });
+
+    rerender(
+      <QueryClientProvider client={queryClient}>
+        <MCPServers {...defaultProps} />
+      </QueryClientProvider>,
+    );
+
+    // The server list refresh must NOT trigger a second health check
+    expect(networking.fetchMCPServerHealth).toHaveBeenCalledTimes(1);
+  });
 });

ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.tsx

@@ -29,8 +29,7 @@ const MCPServers: React.FC<MCPServerProps> = ({ accessToken, userRole, userID })
   const { data: mcpServers, isLoading: isLoadingServers, refetch } = useMCPServers();
 
   // Fetch health status for all servers
-  const serverIds = useMemo(() => mcpServers?.map((server) => server.server_id), [mcpServers]);
-  const { data: healthStatuses, isLoading: isLoadingHealth } = useMCPServerHealth(serverIds);
+  const { data: healthStatuses, isLoading: isLoadingHealth } = useMCPServerHealth();
 
   // Merge health status data into servers
   const serversWithHealth = useMemo(() => {