fix: clean up server_tool_association before tool deletion (#4263)

* fix: clean up server_tool_association before tool deletion

delete_tool() uses a Core SQL DELETE which bypasses ORM cascades.
The server_tool_association FK to tools.id has no ondelete cascade,
so deleting a tool that is associated with a virtual server (e.g. an
A2A agent tool) raises sqlite3.IntegrityError.

Explicitly delete server_tool_association rows for the tool before
the tool row itself, matching the pattern already used in
gateway_service.

Closes #4261

Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>

* fix(tests): update tool delete test to expect association cleanup execute call

delete_tool now calls db.execute twice: once for server_tool_association
cleanup and once for the tool DELETE. The test assertion was stale after
commit 5be84b4 added the association cleanup step.

Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>

* docs(test): correct stale 'DELETE ... RETURNING' docstring in concurrency test

test_concurrent_tool_delete_operations described an 'atomic
DELETE ... RETURNING' implementation that no longer reflects the production
code. delete_tool currently uses get_for_update(nowait=True) to acquire a
row-level lock, then a Core-SQL DELETE with a rowcount check. Update the
docstring so it accurately describes the two paths a losing concurrent
caller can take (lock-conflict 409 or not-found 404).

Signed-off-by: Jonathan Springer <jps@s390x.com>

---------

Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
Signed-off-by: Jonathan Springer <jps@s390x.com>
Co-authored-by: Jonathan Springer <jps@s390x.com>

Author: madhav165

.secrets.baseline

@@ -3,7 +3,7 @@
     "files": "(?x)( package-lock\\.json$ |Cargo\\.lock$ |uv\\.lock$ |go\\.sum$ |mcpgateway/sri_hashes\\.json$ )|^.secrets.baseline$",
     "lines": null
   },
-  "generated_at": "2026-04-26T18:36:21Z",
+  "generated_at": "2026-04-26T22:08:03Z",
   "plugins_used": [
     {
       "name": "AWSKeyDetector"
@@ -8584,63 +8584,63 @@
         "hashed_secret": "7b1552c7c7ffb8bd70b5666e5997c8e017630aab",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 1981,
+        "line_number": 1984,
         "type": "Base64 High Entropy String",
         "verified_result": null
       },
       {
         "hashed_secret": "9fb7fe1217aed442b04c0f5e43b5d5a7d3287097",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 3318,
+        "line_number": 3321,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "72cb70dbbafe97e5ea13ad88acd65d08389439b0",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 3951,
+        "line_number": 3954,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "ee977806d7286510da8b9a7492ba58e2484c0ecc",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 7307,
+        "line_number": 7310,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "f2e7745f43b0ef0e2c2faf61d6c6a28be2965750",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 7799,
+        "line_number": 7802,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "4a249743d4d2241bd2ae085b4fe654d089488295",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 9146,
+        "line_number": 9149,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "0c8d051d3c7eada5d31b53d9936fce6bcc232ae2",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 9288,
+        "line_number": 9291,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "f2b14f68eb995facb3a1c35287b778d5bd785511",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 9770,
+        "line_number": 9773,
         "type": "Secret Keyword",
         "verified_result": null
       }
@@ -8674,71 +8674,71 @@
         "hashed_secret": "0857abc2d90c5d9821229fc0a880f1c38ffb0e04",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 4066,
+        "line_number": 4125,
         "type": "Hex High Entropy String",
         "verified_result": null
       },
       {
         "hashed_secret": "99834bc4eff3f1e1c1e4692d2476b593b501d045",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 6993,
+        "line_number": 7052,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "f2b14f68eb995facb3a1c35287b778d5bd785511",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 7011,
+        "line_number": 7070,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "b8cec023ad982a1355abb9e7c7700e42d7e6fac3",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 7035,
+        "line_number": 7094,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "0614eb27c6e0d2f4ed9a1cce2a5bcbbbc17aa556",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 7099,
+        "line_number": 7158,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "a38fea79a043f0c9a62f851659d459dc3b716ea9",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 7110,
+        "line_number": 7169,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "a50da1fb101595ac5158f2c9394a65a84061b56c",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 7151,
+        "line_number": 7210,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "ed3e0017cb8e4b06a59af1a441f62cbe58d2ef59",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 7157,
+        "line_number": 7216,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "9bdc408babb4c5740aa9ee42e65b9308bd01f5f9",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 8351,
+        "line_number": 8410,
         "type": "Secret Keyword",
         "verified_result": null
       }

mcpgateway/services/tool_service.py

@@ -3249,6 +3249,11 @@ async def delete_tool(self, db: Session, tool_id: str, user_email: Optional[str]
                     delete_metrics_in_batches(db, ToolMetric, ToolMetric.tool_id, tool_id)
                     delete_metrics_in_batches(db, ToolMetricsHourly, ToolMetricsHourly.tool_id, tool_id)
 
+            # Clean up server_tool_association rows referencing this tool.
+            # The association table FK has no ondelete cascade, so rows must
+            # be removed explicitly before the tool row can be deleted.
+            db.execute(delete(server_tool_association).where(server_tool_association.c.tool_id == tool_id))
+
             # Use DELETE with rowcount check for database-agnostic atomic delete
             stmt = delete(DbTool).where(DbTool.id == tool_id)
             result = db.execute(stmt)

tests/integration/test_concurrency_row_locking.py

@@ -524,11 +524,12 @@ async def update_description():
 @pytest.mark.asyncio
 @SKIP_IF_NOT_POSTGRES
 async def test_concurrent_tool_delete_operations(client: AsyncClient):
-    """Test concurrent delete operations with atomic DELETE ... RETURNING.
+    """Test that concurrent deletes of the same tool don't corrupt state.
 
-    With the atomic DELETE ... RETURNING implementation, exactly one delete should
-    succeed. All other concurrent deletes will find no row to delete and should
-    return an error in the redirect URL.
+    delete_tool uses a Core-SQL DELETE with a rowcount check: exactly one
+    caller sees rowcount == 1 (success) while the others see rowcount == 0
+    and raise ToolNotFoundError, which the admin endpoint surfaces as an
+    error in the redirect URL.
 
     Note: The admin endpoint always returns 303 redirects, so we check the
     redirect URL for error messages to distinguish success from failure.

tests/unit/mcpgateway/services/test_resource_ownership.py

@@ -285,8 +285,8 @@ async def test_delete_tool_owner_success(self, tool_service, mock_db_session):
 
             await tool_service.delete_tool(mock_db_session, "tool-1", user_email="owner@example.com")
 
-            # Verify execute was called for DELETE ... RETURNING
-            mock_db_session.execute.assert_called_once()
+            # Verify execute was called for association cleanup + tool DELETE
+            assert mock_db_session.execute.call_count == 2
             mock_db_session.commit.assert_called_once()
 
     @pytest.mark.asyncio

tests/unit/mcpgateway/services/test_tool_service.py

@@ -1530,8 +1530,8 @@ async def test_delete_tool(self, tool_service, mock_tool, test_db):
 
         # Verify DB operations
         test_db.get.assert_called_once_with(DbTool, 1)
-        # Verify execute was called for DELETE ... RETURNING
-        test_db.execute.assert_called_once()
+        # Verify execute was called for server_tool_association cleanup + DELETE
+        assert test_db.execute.call_count == 2
         test_db.commit.assert_called_once()
 
         # Verify notification
@@ -1544,19 +1544,22 @@ async def test_delete_tool_purge_metrics(self, tool_service, mock_tool, test_db)
         test_db.commit = Mock()
         test_db.rollback = Mock()
 
-        # Mock execute results: batch deletes return rowcount=0 to stop loop, final DELETE returns rowcount=1
+        # Mock execute results: batch deletes return rowcount=0 to stop loop,
+        # association cleanup returns a result, final DELETE returns rowcount=1
         batch_result = Mock()
         batch_result.rowcount = 0  # No rows to delete (stops the batch loop)
+        assoc_result = Mock()
+        assoc_result.rowcount = 0  # No server_tool_association rows
         delete_result = Mock()
         delete_result.rowcount = 1  # Final DELETE succeeded
-        test_db.execute = Mock(side_effect=[batch_result, batch_result, delete_result])
+        test_db.execute = Mock(side_effect=[batch_result, batch_result, assoc_result, delete_result])
 
         tool_service._notify_tool_deleted = AsyncMock()
 
         await tool_service.delete_tool(test_db, 1, purge_metrics=True)
 
-        # Verify execute was called: 1 for ToolMetric + 1 for ToolMetricsHourly + 1 for DELETE = 3
-        assert test_db.execute.call_count == 3
+        # Verify execute was called: 1 for ToolMetric + 1 for ToolMetricsHourly + 1 for association cleanup + 1 for DELETE = 4
+        assert test_db.execute.call_count == 4
         test_db.commit.assert_called_once()
 
     @pytest.mark.asyncio

tests/unit/mcpgateway/services/test_tool_service_coverage.py

@@ -3979,6 +3979,65 @@ async def test_delete_with_purge_metrics(self, tool_service):
         assert mock_delete.call_count == 2  # ToolMetric + ToolMetricsHourly
 
 
+class TestDeleteToolServerAssociationCleanup:
+    """Tests for delete_tool cleaning up server_tool_association rows (issue #4261)."""
+
+    @pytest.fixture
+    def tool_service(self):
+        service = ToolService()
+        service._http_client = AsyncMock()
+        service._event_service = AsyncMock()
+        return service
+
+    @pytest.mark.asyncio
+    async def test_delete_tool_cleans_server_tool_association(self, tool_service):
+        """delete_tool must remove server_tool_association rows before deleting the tool."""
+        db = MagicMock()
+        mock_tool = MagicMock(spec=DbTool)
+        mock_tool.id = "tool-1"
+        mock_tool.name = "a2a_test_agent"
+        mock_tool.url = "http://example.com"
+        mock_tool.tags = ["a2a", "agent"]
+        mock_tool.team_id = None
+        mock_tool.gateway_id = None
+
+        db.get.return_value = mock_tool
+
+        # Track execute calls to verify ordering
+        execute_calls = []
+        assoc_result = MagicMock()
+        assoc_result.rowcount = 1  # One association row removed
+        delete_result = MagicMock()
+        delete_result.rowcount = 1
+
+        def track_execute(stmt):
+            execute_calls.append(str(stmt))
+            # First call is association cleanup, second is tool DELETE
+            if len(execute_calls) == 1:
+                return assoc_result
+            return delete_result
+
+        db.execute.side_effect = track_execute
+
+        mock_admin_cache = MagicMock()
+        mock_admin_cache.invalidate_tags = AsyncMock()
+        mock_metrics_cache = MagicMock()
+
+        with (
+            patch("mcpgateway.services.tool_service._get_registry_cache") as mock_rc,
+            patch("mcpgateway.services.tool_service._get_tool_lookup_cache") as mock_tlc,
+            patch("mcpgateway.cache.admin_stats_cache.admin_stats_cache", mock_admin_cache),
+            patch("mcpgateway.cache.metrics_cache.metrics_cache", mock_metrics_cache),
+        ):
+            mock_rc.return_value = AsyncMock()
+            mock_tlc.return_value = AsyncMock()
+            await tool_service.delete_tool(db, "tool-1")
+
+        # Must have 2 execute calls: association cleanup then tool DELETE
+        assert db.execute.call_count == 2
+        db.commit.assert_called()
+
+
 # ============================================================================
 # convert_tool_to_read with metrics
 # ============================================================================
@@ -6576,7 +6635,7 @@ async def fake_get(*a, **kw):
             # Positional fallback: the `success` argument is the 3rd positional
             # after (tool_id, start_time, success) in the recorder signature.
             recorded_success = metrics_record.call_args.args[2]
-        assert recorded_success is False, f"Expected metrics success=False for REST isError=true response, got {recorded_success}. " "This regression would silently inflate tool success rates."
+        assert recorded_success is False, f"Expected metrics success=False for REST isError=true response, got {recorded_success}. This regression would silently inflate tool success rates."
 
     @pytest.mark.asyncio
     async def test_mcp_non_direct_proxy_iserror_records_success_false_in_metrics(self, tool_service):
@@ -6688,7 +6747,7 @@ async def __aexit__(self, *exc):
         assert metrics_record.called, "record_tool_metric was not invoked"
         recorded_success = metrics_record.call_args.kwargs.get("success")
         assert recorded_success is False, (
-            f"Expected metrics success=False for MCP non-direct-proxy isError=true response, got {recorded_success}. " "This would silently inflate federated-tool success rates."
+            f"Expected metrics success=False for MCP non-direct-proxy isError=true response, got {recorded_success}. This would silently inflate federated-tool success rates."
         )