fix: improve tool status updates and fix test flake8 errors #1641

[jonpspri]

Summary

• Rename toggle_tool_status to set_tool_status for clarity and consistency
• Add row-level locking with with_for_update to prevent race conditions during concurrent status updates
• Wrap status updates in explicit transaction with db.begin() for better atomicity
• Fix multiple flake8 errors in test files (E402, E262, F841, F401, F541, F811, E266)

This is the first PR addressing Issue #1641 - more PRs will follow.

Addresses #1641 (more to come)

[Copilot]

Pull request overview

This PR is the first in a series addressing race conditions under high concurrency (Issue #1641). It focuses specifically on improving the tool status update mechanism by renaming toggle_tool_status to set_tool_status, adding row-level locking, implementing explicit transactions, and fixing various flake8 linting errors in test files.

Key changes:

• Renamed toggle_tool_status to set_tool_status for clearer semantics
• Added row-level locking using with_for_update to prevent race conditions during concurrent status updates
• Wrapped status updates in explicit transaction with db.begin() for better atomicity
• Fixed multiple flake8 errors (E402, E262, F841, F401, F541, F811, E266) across test files

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
mcpgateway/services/tool_service.py	Renamed method to set_tool_status, added row-level locking with with_for_update, and wrapped status updates in db.begin() transaction
mcpgateway/services/gateway_service.py	Updated calls from toggle_tool_status to set_tool_status and updated comments
mcpgateway/main.py	Updated REST API endpoint to call set_tool_status
mcpgateway/admin.py	Updated admin endpoint to call set_tool_status and updated docstring examples
tests/unit/mcpgateway/test_main.py	Updated test mocks and patches to use set_tool_status, removed duplicate test sections, fixed unused variables and imports
tests/unit/mcpgateway/test_admin.py	Updated test mocks to use set_tool_status, fixed formatting and removed unused variables
tests/unit/mcpgateway/services/test_tool_service.py	Updated tests to mock db.begin() and db.is_modified(), renamed test methods, removed unused imports and variables
tests/unit/mcpgateway/services/test_gateway_service.py	Updated test mocks to use set_tool_status and updated comments
tests/loadtest/locustfile.py	Renamed load test method from toggle_tool_status to set_tool_status
.pre-commit-config.yaml	Added loadtest directory to test naming convention exclusions

Comments suppressed due to low confidence (1)

tests/unit/mcpgateway/test_main.py:1330

• This comment appears to contain commented-out code.

# class TestA2AAgentEndpoints:
#     """Test A2A agent API endpoints."""
#
#     @patch("mcpgateway.main.a2a_service.list_agents")
#     def test_list_a2a_agents(self, mock_list, test_client, auth_headers):
#        """Test listing A2A agents."""
#        mock_list.return_value = []
#        response = test_client.get("/a2a", headers=auth_headers)
#        assert response.status_code == 200
#        mock_list.assert_called_once()
#
#    @patch("mcpgateway.main.a2a_service.get_agent")
#    def test_get_a2a_agent(self, mock_get, test_client, auth_headers):
#        """Test getting specific A2A agent."""
#        mock_agent = {
#            "id": "test-id",
#            "name": "test-agent",
#            "description": "Test agent",
#            "endpoint_url": "https://api.example.com",
#            "agent_type": "generic",
#            "enabled": True,
#            "metrics": MOCK_METRICS,
#        }
#        mock_get.return_value = mock_agent
#
#        response = test_client.get("/a2a/test-id", headers=auth_headers)
#        assert response.status_code == 200
#        mock_get.assert_called_once()
#
#    @patch("mcpgateway.main.a2a_service.register_agent")
#    @patch("mcpgateway.main.MetadataCapture.extract_creation_metadata")
#    def test_create_a2a_agent(self, mock_metadata, mock_register, test_client, auth_headers):
#        """Test creating A2A agent."""
#        mock_metadata.return_value = {
#            "created_by": "test_user",
#            "created_from_ip": "127.0.0.1",
#            "created_via": "api",
#            "created_user_agent": "test",
#            "import_batch_id": None,
#            "federation_source": None,
#        }
#        mock_register.return_value = {"id": "new-id", "name": "new-agent"}
#
#        agent_data = {
#            "name": "new-agent",
#            "endpoint_url": "https://api.example.com/agent",
#            "agent_type": "custom",
#            "description": "New test agent",
#        }
#
#        response = test_client.post("/a2a", json=agent_data, headers=auth_headers)
#        assert response.status_code == 201
#        mock_register.assert_called_once()
#
#    @patch("mcpgateway.main.a2a_service.update_agent")
#    @patch("mcpgateway.main.MetadataCapture.extract_modification_metadata")
#    def test_update_a2a_agent(self, mock_metadata, mock_update, test_client, auth_headers):
#        """Test updating A2A agent."""
#        mock_metadata.return_value = {
#            "modified_by": "test_user",
#            "modified_from_ip": "127.0.0.1",
#            "modified_via": "api",
#            "modified_user_agent": "test",
#        }
#        mock_update.return_value = {"id": "test-id", "name": "updated-agent"}
#
#        update_data = {"description": "Updated description"}
#
#        response = test_client.put("/a2a/test-id", json=update_data, headers=auth_headers)
#        assert response.status_code == 200
#        mock_update.assert_called_once()
#
#    @patch("mcpgateway.main.a2a_service.toggle_agent_status")
#    def test_toggle_a2a_agent_status(self, mock_toggle, test_client, auth_headers):
#        """Test toggling A2A agent status."""
#        mock_toggle.return_value = {"id": "test-id", "enabled": False}
#
#        response = test_client.post("/a2a/test-id/toggle?activate=false", headers=auth_headers)
#        assert response.status_code == 200
#        mock_toggle.assert_called_once()
#
#    @patch("mcpgateway.main.a2a_service.delete_agent")
#    def test_delete_a2a_agent(self, mock_delete, test_client, auth_headers):
#        """Test deleting A2A agent."""
#        mock_delete.return_value = None
#
#        response = test_client.delete("/a2a/test-id", headers=auth_headers)
#        assert response.status_code == 200
#        mock_delete.assert_called_once()
#
#    @patch("mcpgateway.main.a2a_service.invoke_agent")
#    def test_invoke_a2a_agent(self, mock_invoke, test_client, auth_headers):
#        """Test invoking A2A agent."""
#        mock_invoke.return_value = {"response": "Agent response", "status": "success"}
#
#        response = test_client.post(
#            "/a2a/test-agent/invoke",
#            json={"parameters": {"query": "test"}, "interaction_type": "query"},
#            headers=auth_headers
#        )
#        assert response.status_code == 200
#        mock_invoke.assert_called_once()
#


# ----------------------------------------------------- #
# Middleware & Security Tests                           #
# ----------------------------------------------------- #

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The db.get() method may not support the with_for_update parameter in all SQLAlchemy versions. The recommended pattern throughout this codebase (and in the linked issue) is to use db.execute(select(DbTool).where(DbTool.id == tool_id).with_for_update(...)).scalar_one_or_none(). This ensures compatibility and follows the established convention.

Additionally, the issue recommends skip_locked=True rather than key_share=True. The skip_locked=True option causes blocked transactions to immediately skip locked rows rather than wait, which is better for high-concurrency scenarios where you want to avoid request pileups.

[Copilot]

The logic for detecting if the tool was modified is potentially incorrect. The code sets tool.enabled = activate and tool.reachable = reachable unconditionally (lines 2059-2060), then checks if the tool was modified with db.is_modified(tool). However, if activate equals the current tool.enabled value and reachable equals the current tool.reachable value, SQLAlchemy may not mark the object as modified since the values didn't actually change. Consider explicitly checking if the values changed before assigning: if tool.enabled != activate or tool.reachable != reachable: before making the assignments.

[Copilot]

Verify that the mocking of db.get() with with_for_update parameter works correctly. If the actual implementation is changed to use db.execute(select(...).with_for_update(...)) as recommended, these test mocks will need to be updated accordingly to mock db.execute() instead of db.get().

[crivetimihai]

Check with @rakdutta please you might be working on the same issue

[rakdutta]

@jonpspri @crivetimihai
PR #1504 already addresses the issue of renaming toggle_tool_status to set_tool_status for clearer semantics.
I have already started working on issue #1641 for all services.

[jonpspri]

Redundant to other work.