From 2e948527ceaf413e6d2b0ecd90954b66320150d7 Mon Sep 17 00:00:00 2001
From: mateo-berri <277851410+mateo-berri@users.noreply.github.com>
Date: Thu, 4 Jun 2026 16:05:15 +0000
Subject: [PATCH] fix(mcp): make per-user env var cleanup best-effort on server
 delete

The server-row delete is the commit point; a transient failure cleaning the
FK-less per-user env var rows now logs a warning instead of propagating, so a
successful delete is no longer turned into a caller error that triggers a retry
and a 404 for an already-gone server. Also mark the health-check env-var
round-trip test as asyncio so it runs explicitly like its siblings.
---
 litellm/proxy/_experimental/mcp_server/db.py  | 20 ++++++++++++----
 .../mcp_server/test_mcp_env_vars.py           | 23 +++++++++++++++++++
 .../mcp_server/test_mcp_server_manager.py     |  1 +
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/litellm/proxy/_experimental/mcp_server/db.py b/litellm/proxy/_experimental/mcp_server/db.py
index 41072a63659..34093fa7638 100644
--- a/litellm/proxy/_experimental/mcp_server/db.py
+++ b/litellm/proxy/_experimental/mcp_server/db.py
@@ -519,8 +519,10 @@ async def delete_mcp_server(
     """
     Delete the mcp server from the db by server_id
 
-    Also removes any per-user env var rows for the server, which have no FK
-    cascade, so deleting a server never leaves orphaned credential rows behind.
+    The server-row delete is the commit point. Per-user env var rows have no FK
+    cascade, so they are cleaned up afterwards on a best-effort basis: a transient
+    failure there leaves only orphaned rows pointing at a now-missing server and
+    must not turn a successful delete into a caller-visible error.
 
     Returns the deleted mcp server record if it exists, otherwise None
     """
@@ -530,9 +532,17 @@ async def delete_mcp_server(
         },
     )
     if deleted_server is not None:
-        await prisma_client.db.litellm_mcpuserenvvars.delete_many(
-            where={"server_id": server_id}
-        )
+        try:
+            await prisma_client.db.litellm_mcpuserenvvars.delete_many(
+                where={"server_id": server_id}
+            )
+        except Exception as e:
+            verbose_proxy_logger.warning(
+                "MCP server %s deleted but per-user env var cleanup failed; "
+                "orphaned rows can be removed on a later delete: %s",
+                server_id,
+                e,
+            )
     return deleted_server
 
 
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 0bbb052623a..840016d0eed 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
@@ -829,6 +829,29 @@ async def test_delete_mcp_server_skips_env_var_cleanup_when_server_missing():
     prisma.db.litellm_mcpuserenvvars.delete_many.assert_not_awaited()
 
 
+@pytest.mark.asyncio
+async def test_delete_mcp_server_succeeds_when_orphan_cleanup_fails():
+    """The server-row delete is the commit point: a transient failure cleaning
+    the FK-less per-user env var rows must not turn a successful delete into a
+    caller error, otherwise the caller retries and hits a 404 for a server that
+    is already gone."""
+    from unittest.mock import AsyncMock
+
+    from litellm.proxy._experimental.mcp_server.db import delete_mcp_server
+
+    deleted = object()
+    prisma = _mock_env_vars_prisma()
+    prisma.db.litellm_mcpservertable.delete = AsyncMock(return_value=deleted)
+    prisma.db.litellm_mcpuserenvvars.delete_many = AsyncMock(
+        side_effect=Exception("connection pool exhausted")
+    )
+
+    result = await delete_mcp_server(prisma, "srv-1")
+
+    assert result is deleted
+    prisma.db.litellm_mcpuserenvvars.delete_many.assert_awaited_once()
+
+
 # ── DB helpers: global env vars encrypted at rest ─────────────────────────
 
 
diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py
index 2ef1c5b7f11..7391d452097 100644
--- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py
+++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py
@@ -4086,6 +4086,7 @@ class TestRegistryTableConversionPreservesEnvVars:
         table = manager._build_mcp_server_table(self._server_with_env_vars())
         self._assert_env_vars_round_tripped(table)
 
+    @pytest.mark.asyncio
     async def test_health_check_server_preserves_env_vars(self):
         # OAuth2 without client credentials needs a per-user token, so the
         # health check is skipped (no network) and we exercise the table