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.
This commit is contained in:
mateo-berri 2026-06-03 18:09:25 +00:00
parent 9764f54f4d
commit b38320d29c
No known key found for this signature in database
5 changed files with 24 additions and 27 deletions

View file

@ -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}
)

View file

@ -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(

View file

@ -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 ───────────────────────────────────────────────

View file

@ -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):

View file

@ -434,8 +434,6 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
...(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);