Make MCP connections concurrency-safe and extend thinking config#107
Merged
Conversation
Contributor
Reviewer's GuideMakes MCP client connection handling idempotent and concurrency-safe, adds structured thinking configuration support to the OpenAI adapter, enables local search in docs, performs minor cleanup of deprecated comments/config, and bumps the project version. Sequence diagram for concurrent MCPClient connection handlingsequenceDiagram
actor Caller1
actor Caller2
participant MCPClient
participant Client
Caller1->>MCPClient: _connect(update_tools)
activate MCPClient
MCPClient->>MCPClient: _connect_lock.acquire()
MCPClient->>MCPClient: _clean_waitter()
MCPClient->>Client: Client(server_script)
MCPClient->>Client: __aenter__()
Client-->>MCPClient: client ready
MCPClient->>MCPClient: list_tools()
MCPClient->>MCPClient: _cast_tool_to_amrita()
MCPClient->>MCPClient: _connect_lock.release()
deactivate MCPClient
Caller2->>MCPClient: _connect(update_tools)
activate MCPClient
MCPClient->>MCPClient: _connect_lock.acquire()
alt mcp_client already connected
MCPClient-->>Caller2: return
end
MCPClient->>MCPClient: _connect_lock.release()
deactivate MCPClient
Caller1->>MCPClient: _close()
activate MCPClient
MCPClient->>MCPClient: _connect_lock.acquire()
MCPClient->>Client: __aexit__(None, None, None)
Client-->>MCPClient: closed
MCPClient->>MCPClient: mcp_client = None
MCPClient->>MCPClient: _connect_lock.release()
deactivate MCPClient
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Member
Author
|
@sourcery-ai title |
Deploying amritacore with
|
| Latest commit: |
86f3af7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ea216461.amritacore.pages.dev |
| Branch Preview URL: | https://fix.amritacore.pages.dev |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
MCPClient._connect, callingawait self._clean_waitter()inside the_connect_lockcan deadlock because_clean_waitterawaits a_closetask that now also acquires_connect_lock; consider moving_clean_waitteroutside theasync with _connect_lockor adjusting_closeso it doesn't contend for the same lock when awaited via_close_waitter.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MCPClient._connect`, calling `await self._clean_waitter()` inside the `_connect_lock` can deadlock because `_clean_waitter` awaits a `_close` task that now also acquires `_connect_lock`; consider moving `_clean_waitter` outside the `async with _connect_lock` or adjusting `_close` so it doesn't contend for the same lock when awaited via `_close_waitter`.
## Individual Comments
### Comment 1
<location path="src/amrita_core/tools/mcp.py" line_range="109-113" />
<code_context>
await self._close()
async def _connect(self, update_tools: bool = False):
- """Connect to MCP Server
+ """Connect to MCP Server (idempotent, safe for concurrent calls)
Args:
update_tools (bool, optional): whether to update the tool list. Defaults to False.
"""
- await self._clean_waitter()
- if self.mcp_client is not None:
- raise RuntimeError("MCP Server is already connected!")
-
</code_context>
<issue_to_address>
**issue (bug_risk):** Update-tools path is ignored if a connection already exists, which may surprise callers.
Previously, `_connect` raised if already connected, so callers wouldn’t expect tools to remain stale. Now, if another coroutine connects first, a later call with `update_tools=True` returns early and `self.tools` is never refreshed. If reloading tools on an open connection is expected, consider honoring `update_tools` even when `self.mcp_client` is set (e.g., refresh tools under the lock or separate connect vs. tool-refresh logic). If not, clarify (via docs or assertions) that `update_tools` only applies on the initial connection to avoid subtle staleness issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Make MCP client connections concurrency-safe and enhance OpenAI adapter thinking configuration while updating docs and versioning.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: