feat(mcp): refresh tools for active conversations - #4402
Conversation
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
1 similar comment
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
Hi @neubig and @VascoSch92 — thank you both for the work and review around #4367 and #4369. This PR follows the same line of work. #4367 handles add/update/remove reconciliation while an MCP connection remains alive, and #4369 hardens that path. This PR covers a separate deployment boundary: when the MCP server process is replaced, an existing conversation can explicitly reconnect and refresh its tool snapshot. It reuses the reconciliation path introduced in #4367 and preserves the conversation rather than forking or recreating it. CI is green now. When you have a chance, I’d really appreciate your thoughts on whether this is the right lifecycle and API boundary. Thank you! |
|
Hi @neubig — when you have a chance, could you please take a look at this PR? We have a production integration that needs to refresh MCP tool definitions for existing conversations after backend deployments, so this capability would be very helpful to us. All CI checks are passing, and the change is independent of #4369. If the interface and lifecycle look right to you, we’d really appreciate your help reviewing and merging it. Thank you! |
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Hey @Shimada666 just a couple of quick questions, given that this PR goes quite deep in the core code: I wonder if you have seen that MCP 2.0 is now stateless, so all this careful handling of sessions, I think, shouldn’t be necessary anymore?
Also, is this a problem you encounter a lot or sometimes or rarely?
Just trying to understand what we could do here. 🤔
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR adds Conversation.refresh_mcp_tools() for both local and remote conversations, with an Agent Server REST endpoint (POST /{conversation_id}/refresh_mcp_tools). It reconnects each configured MCP client and reuses the existing add/update/remove reconciliation from #4367. The lock migration from _ToolListChangedHandler._refresh_lock to MCPClient._tools_refresh_lock is a good improvement — it ensures the notification-driven refresh and the explicit refresh share the same lock, preventing concurrent refresh attempts on the same client.
The test coverage is thorough: the deployment test (test_refresh_reconnects_after_mcp_deployment) exercises the real scenario (process termination + restart on the same URL) in both stateful and stateless HTTP modes, verifies tool add/remove/update semantics, confirms conversation ID/events/workspace are preserved, and asserts the next LLM completion receives exactly the refreshed tool set.
Risk Assessment
Low–Medium. The change is additive and follows existing patterns. The REST endpoint is additive with correct 404 handling. The run_in_executor wrapper in EventService.refresh_mcp_tools is consistent with the existing load_plugin path. The main concern is error propagation in the refresh loop (see inline comment), which could leave a multi-server conversation partially refreshed.
Findings
1. Error propagation aborts multi-server refresh (Moderate)
In LocalConversation.refresh_mcp_tools(), if one MCP client fails to reconnect (e.g., one server is still down while others are healthy), the exception propagates immediately and remaining clients are skipped. For conversations with multiple MCP servers, one dead server blocks refreshing all others. The close() method already handles individual client failures gracefully with best-effort cleanup. Consider wrapping each client's refresh in a try/except, logging failures, and continuing — or at minimum, documenting that callers should expect partial refresh on failure.
2. _reconnect partial-failure state (Low, non-blocking)
In MCPClient._reconnect, if __aexit__ succeeds but connect() fails (e.g., the new server process hasn't fully started), the client has no active session but _closed remains False. This is handled gracefully by the existing MCPToolExecutor.call_tool reconnection logic (which checks is_connected() and retries), so it's not a bug — just noting that the client remains in a recoverable-but-disconnected state.
No Issues Found
- The unconditional
on_tools_reconciledcall in_reconnect_and_refresh_tools(line 193-194) is correct — it ensures the agent re-processes the full tool snapshot after a session replacement, even if the tool list is identical._refresh_toolsis called without callbacks in the reconnect path, so there's no double-callback risk. fastmcp.Client.__aexit__usesanyio.move_on_after(self._disconnect_timeout)internally, so calling__aexit__on a dead session won't hang.- The
_close_mcp_clienterror-path cleanup correctly removes the client from_mcp_clients. - Thread safety is preserved:
tuple(self._mcp_clients)is used for safe iteration during refresh, and all list mutations occur on sync code paths between runs.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR adds Conversation.refresh_mcp_tools() for local and remote conversations, backed by an Agent Server REST endpoint (POST /{conversation_id}/refresh_mcp_tools). The approach—reconnect each MCP client and reuse the existing add/update/remove reconciliation from #4367—is sound and well-scoped. Tracking MCP clients on the conversation (even when their initial tool list is empty) is a good improvement that ensures later deployments can add tools and cleanup remains complete.
The deployment test (test_refreshes_tools_after_mcp_deployment) is thorough: it exercises process termination + restart in both stateful and stateless HTTP modes, verifies tool add/remove/update semantics, confirms conversation ID/events/workspace are preserved, and asserts the next LLM completion receives exactly the refreshed tool set.
Risk Assessment
Medium. The change is additive and follows existing patterns. The REST endpoint has correct 404 handling and the run_in_executor wrapper in EventService is consistent with load_plugin. However, the reconnect logic in _refresh_or_reconnect_tools has a narrow exception catch (except McpError) that does not cover all dead-session failure modes, which can leave a client permanently stuck after a failed refresh. See the inline findings below for details.
Findings
1. Client gets permanently stuck after a failed refresh (Moderate)
_refresh_or_reconnect_tools only catches McpError to trigger reconnection. But when a dead session is accessed, list_tools() can raise exceptions that are not McpError:
ConnectError(httpx transport error): raised when the session still appears connected but the underlying HTTP connection is dead (e.g., the server was terminated and no replacement is running yet).RuntimeError: raised by thesessionproperty ("Client is not connected...") whenis_connected()isFalse— which is the state the client ends up in after a failedConnectError.
I verified this empirically by killing an MCP server process and calling _refresh_or_reconnect_tools:
# Server killed, new server NOT yet running:
_refresh_tools raised: ConnectError: All connection attempts failed
isinstance McpError? False
# After the ConnectError, is_connected() becomes False.
# New server started, refresh retried:
Refresh with 3rd server raised: RuntimeError: Client is not connected.
isinstance McpError? False
After the initial ConnectError, is_connected() flips to False. Subsequent refresh attempts raise RuntimeError (not McpError), so the except McpError catch never triggers, and the client is permanently stuck — even when the server comes back up, refresh_mcp_tools() cannot recover it.
This is inconsistent with both the notification-driven path and the tool-execution path, which both check is_connected() and reconnect proactively:
_refresh_connected_tools(line 181):if not client.is_connected(): await client.connect()MCPToolExecutor.call_tool(tool.py line 80):if not self.client.is_connected(): ... await self.client.connect()
Suggested fix: Add an is_connected() check at the start of _refresh_or_reconnect_tools (mirroring _refresh_connected_tools), or broaden the catch to include RuntimeError:
if not client.is_connected():
await client._reconnect()
await _refresh_tools(client)
else:
try:
await _refresh_tools(client)
except McpError:
await client._reconnect()
await _refresh_tools(client)The primary use case (kill old → start new → refresh) works correctly because the new server is already running when list_tools() is called, producing a McpError: Session terminated that IS caught. The bug manifests when the refresh is called while the server is briefly unavailable — a realistic operational scenario during rolling deployments.
2. Error propagation aborts multi-server refresh (Low–Moderate)
In LocalConversation.refresh_mcp_tools(), if one MCP client fails to refresh (e.g., ConnectError, RuntimeError, or MCPError after a failed reconnect), the exception propagates immediately and remaining clients in self._mcp_clients are never refreshed. For conversations with multiple MCP servers, one dead server blocks refreshing all others.
The close() method (line 2665) handles individual client failures gracefully with best-effort cleanup. Consider wrapping each client's refresh in a try/except, logging failures, and continuing to the next client so healthy servers still get refreshed — or at minimum, documenting that callers should expect partial refresh on failure.
No Issues Found
- The lock migration from
_ToolListChangedHandler._refresh_locktoMCPClient._tools_refresh_lockis correct — it ensures the notification-driven refresh and the explicit refresh share the same lock. - The unconditional
on_tools_reconciledcall in_refresh_or_reconnect_tools(line 199-200) is correct —_refresh_toolsis called without callbacks in the reconnect path, so there's no double-callback risk. _close_mcp_clientcorrectly removes the client from_mcp_clientsandsync_close()is safe to call (handles exceptions internally).- Thread safety is preserved:
tuple(self._mcp_clients)is used for safe iteration, and the agent's_on_mcp_tools_reconciledusesself._tools_lock. - The
close()method's MCP client cleanup is correct and idempotent.
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
461e038 to
4f8a647
Compare
Co-authored-by: openhands <openhands@all-hands.dev>
|
Thanks @enyst — yes, our production integration uses stateless MCP. I changed the implementation so a healthy client performs a direct This is not frequent per request, but it occurs deterministically when our backend-owned MCP tool schema changes while a long-lived Analysis conversation is still active. We call the explicit action after such a deployment so the existing conversation can use the new schema without being recreated. I also rebased onto the now-merged #4369 and reused its reconciliation path. Please let me know if you would prefer a narrower approach or if there is anything else I should adjust. Thanks again for taking a look! |
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
This comment was posted by an AI agent (OpenHands). |
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR adds Conversation.refresh_mcp_tools() (local + remote) and a matching agent-server REST action to re-fetch MCP tool definitions for an active conversation without forking or recreating it. The design is sound: a shared _tools_refresh_lock serializes the notification-driven and explicit refresh paths; _refresh_or_reconnect_tools reconnects only on a terminated session (McpError / not is_connected()) and otherwise reuses the live session; failures across multiple clients are aggregated into an ExceptionGroup; and MCPClient.sync_close() is idempotent so the new close() client loop plus the existing tool-executor loop don't double-close.
I reviewed the reconnect/reconcile control flow, lock ordering between the asyncio refresh lock and the agent's threading RLock, the _close_runtime_tools to _close_mcp_client rename (no stale references remain), the REST endpoint 404 handling, and the event-service thread-pool delegation. The test matrix (real FastMCP deployments in stateful + stateless modes, recovery after temporary unavailability, initially-empty clients, multi-client failure aggregation, plugin load/close paths) covers the important scenarios.
Findings
No material bugs, security problems, or significant design flaws found.
A couple of minor, non-blocking observations (no change required):
_refresh_or_reconnect_toolscallson_tools_reconciled(client, client.tools)unconditionally, even when the tool snapshot is unchanged, whereas the notification path only fires it when there is an actual add/update/remove. The reconciliation is idempotent so this is harmless, just slightly redundant.MCPClient._reconnectonly catcheshttpx.TransportErrorfrom__aexit__; a non-transport exception would propagate and leave the client disconnected (though_disconnectsuppresses most internal errors, so this is very unlikely in practice). A subsequent refresh would still recover via thenot is_connected()path.
Risk Assessment
Low risk. The change is additive (new refresh_mcp_tools API surface) and the existing notification-driven reconciliation path is preserved. The renamed _runtime_mcp_tools to _runtime_mcp_client refactoring is mechanical and well-covered by tests. The new REST endpoint is guarded by the existing conversation lookup and returns 404 for unknown conversations.
|
Hi @enyst @VascoSch92, gentle ping for another look when you have a chance. The concerns raised in the earlier reviews have been addressed, and the latest automated review found no material issues and assessed the change as low risk. Would either of you be able to review this for approval? Thanks! |
3 participants
HUMAN:
We expose backend-owned MCP tools to OpenHands App Server running in E2B. As these tools may change between releases, existing conversations need a lightweight way to refresh their tool definitions without being forked or recreated.
AGENT:
Why
notifications/tools/list_changedhandles changes while an MCP server remains available, but an existing conversation also needs an explicit refresh after the backend tool schema changes across a deployment. The conversation, history, and workspace must remain intact.Summary
Conversation.refresh_mcp_tools()for local and remote conversations, plus an Agent Server REST action.ExceptionGroup.Issue Number
N/A
How to Test
The real FastMCP deployment test runs in stateful and stateless HTTP modes. It replaces a server process on the same URL, changes tool names and schemas, and verifies that the same conversation receives the new snapshot. It also verifies that stateless refresh performs no new initialization and that a refresh can recover after the server was temporarily unavailable.
Commands and current results:
Video/Screenshots
N/A — SDK and Agent Server API change with no visual surface.
Type
Notes
mainand reuses the reconciliation behavior merged in fix(mcp): close reconciliation gaps left by #4367 #4369.