From b38320d29c5f8c81006c3c85fdc67be2cb02c630 Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Wed, 3 Jun 2026 18:09:25 +0000 Subject: [PATCH] fix(mcp): propagate delete DB errors; drop redundant import and duplicate payload assign Make the per-user env-var delete idempotent with delete_many so a missing row is a no-op, and remove the bare except on the clear endpoint that swallowed real DB failures; a failed delete previously reported success to the dashboard while the row remained in the database, so the next tool call would 412 for credentials the user believed they had cleared. Also use the module-level json instead of a redundant inline import, and drop the no-op duplicate static_headers/env_vars assignment in the create-server UI payload. --- litellm/proxy/_experimental/mcp_server/db.py | 10 +++++++--- .../mcp_management_endpoints.py | 9 ++------- .../mcp_server/test_mcp_env_vars.py | 14 +++++++------- .../test_mcp_management_endpoints.py | 16 ++++++++-------- .../components/mcp_tools/create_mcp_server.tsx | 2 -- 5 files changed, 24 insertions(+), 27 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/db.py b/litellm/proxy/_experimental/mcp_server/db.py index b8179f80434..3465cd19398 100644 --- a/litellm/proxy/_experimental/mcp_server/db.py +++ b/litellm/proxy/_experimental/mcp_server/db.py @@ -1073,7 +1073,11 @@ async def delete_user_env_vars( user_id: str, server_id: str, ) -> None: - """Remove the calling user's env var values for ``server_id``.""" - await prisma_client.db.litellm_mcpuserenvvars.delete( - where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}} + """Remove the calling user's env var values for ``server_id``. + + Uses ``delete_many`` so a missing row is a no-op; real DB errors still + propagate to the caller instead of being silently swallowed. + """ + await prisma_client.db.litellm_mcpuserenvvars.delete_many( + where={"user_id": user_id, "server_id": server_id} ) diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index b075ab19617..61ff14a92fc 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -2172,9 +2172,7 @@ if MCP_AVAILABLE: static_headers = getattr(server, "static_headers", None) or {} if isinstance(static_headers, str): try: - import json as _json - - static_headers = _json.loads(static_headers) or {} + static_headers = json.loads(static_headers) or {} except (ValueError, TypeError): static_headers = {} referenced = collect_env_var_references(strings=static_headers.values()) @@ -2304,10 +2302,7 @@ if MCP_AVAILABLE: detail={"error": f"MCP Server {server_id} not found"}, ) await _authorize_mcp_server_access(prisma_client, user_api_key_dict, server_id) - try: - await delete_user_env_vars(prisma_client, user_id, server_id) - except Exception: - pass # Already deleted / didn't exist + await delete_user_env_vars(prisma_client, user_id, server_id) return _compute_user_env_var_status(server=server, stored_values={}) @router.get( diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py index 0d0c4995ed6..ffed81d7aa9 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py @@ -430,7 +430,7 @@ def _mock_env_vars_prisma(row=None): prisma.db.litellm_mcpuserenvvars.find_unique = AsyncMock(return_value=row) prisma.db.litellm_mcpuserenvvars.find_many = AsyncMock(return_value=[]) prisma.db.litellm_mcpuserenvvars.upsert = AsyncMock() - prisma.db.litellm_mcpuserenvvars.delete = AsyncMock() + prisma.db.litellm_mcpuserenvvars.delete_many = AsyncMock() return prisma @@ -529,16 +529,16 @@ async def test_get_user_env_vars_bulk_empty_ids_short_circuits(): @pytest.mark.asyncio -async def test_delete_user_env_vars_calls_unique_key(): +async def test_delete_user_env_vars_is_idempotent_delete_many(): + """Delete must use ``delete_many`` so a missing row is a no-op rather than + raising RecordNotFound; real DB errors are left to propagate.""" from litellm.proxy._experimental.mcp_server.db import delete_user_env_vars prisma = _mock_env_vars_prisma() await delete_user_env_vars(prisma, "alice", "srv-1") - prisma.db.litellm_mcpuserenvvars.delete.assert_awaited_once() - call = prisma.db.litellm_mcpuserenvvars.delete.call_args - assert call.kwargs["where"] == { - "user_id_server_id": {"user_id": "alice", "server_id": "srv-1"} - } + prisma.db.litellm_mcpuserenvvars.delete_many.assert_awaited_once() + call = prisma.db.litellm_mcpuserenvvars.delete_many.call_args + assert call.kwargs["where"] == {"user_id": "alice", "server_id": "srv-1"} # ── REST exception handling ─────────────────────────────────────────────── diff --git a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py index 8c06b9a8883..be8c72cb136 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py @@ -3614,7 +3614,7 @@ class TestClearMCPUserEnvVars: assert all(not spec.is_set for spec in result.required) @pytest.mark.asyncio - async def test_delete_error_is_swallowed(self): + async def test_delete_db_error_propagates(self): server = _make_env_var_server( env_vars=_ENV_VARS_MIXED, static_headers=_STATIC_HEADERS_MIXED ) @@ -3628,15 +3628,15 @@ class TestClearMCPUserEnvVars: patch.object( mgmt_endpoints, "delete_user_env_vars", - AsyncMock(side_effect=Exception("already gone")), + AsyncMock(side_effect=Exception("db down")), ), ): - # Should not raise even though delete blows up. - result = await mgmt_endpoints.clear_mcp_user_env_vars( - server_id="srv-1", - user_api_key_dict=generate_mock_user_api_key_auth(user_id="alice"), - ) - assert result.missing_count == 2 + # A real DB failure must surface, not be masked as a successful clear. + with pytest.raises(Exception, match="db down"): + await mgmt_endpoints.clear_mcp_user_env_vars( + server_id="srv-1", + user_api_key_dict=generate_mock_user_api_key_auth(user_id="alice"), + ) @pytest.mark.asyncio async def test_missing_user_id_raises_400(self): diff --git a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx index 44520839977..9df3d92a10b 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx @@ -434,8 +434,6 @@ const CreateMCPServer: React.FC = ({ ...(tokenValidation !== null && { token_validation: tokenValidation }), }; - payload.static_headers = staticHeaders; - payload.env_vars = envVars; const includeCredentials = restValues.auth_type && AUTH_TYPES_REQUIRING_CREDENTIALS.includes(restValues.auth_type);