Skip to content

Cache MCP tools to avoid calling multiple times when streaming #3236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −0
Open

Cache MCP tools to avoid calling multiple times when streaming #3236

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/google/adk/tools/mcp_tool/mcp_toolset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ def __init__(
self._auth_scheme = auth_scheme
self._auth_credential = auth_credential
self._require_confirmation = require_confirmation
self._loaded_mcp_tools: List[BaseTool] | None = None

@retry_on_closed_resource
async def get_tools(
Expand All @@ -170,6 +171,9 @@ async def get_tools(
Returns:
List[BaseTool]: A list of tools available under the specified context.
"""
if self._loaded_mcp_tools is not None:
return self._loaded_mcp_tools
Comment on lines +174 to +175
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This caching implementation has a critical correctness issue and a potential race condition.

  • Correctness Bug (Not Context-Aware): The list of tools returned by get_tools depends on the readonly_context argument, which influences session headers and tool filtering. This implementation caches the tools from the first call and returns this cached list for all subsequent calls, regardless of their context. This will lead to incorrect behavior if get_tools is called with different contexts.

  • Race Condition: If multiple coroutines call get_tools() concurrently while the cache is empty, they may all bypass the cache check and proceed to fetch tools from the server simultaneously. This negates the performance benefit of caching.

Due to the correctness bug, I recommend rethinking this caching strategy. An incorrect cache can be more harmful than no cache.

Recommendation:
A more robust solution would involve a context-aware cache. For example, you could cache the result of the expensive session.list_tools() call (which depends on headers) and then re-apply the context-specific filtering (_is_tool_selected) on every get_tools call.

If you are certain that a McpToolset instance will only ever be used with a single, unchanging context, please add a prominent comment or docstring to clarify this assumption. Even in that case, the race condition should be fixed using asyncio.Lock to ensure tools are fetched only once.


headers = (
self._header_provider(readonly_context)
if self._header_provider and readonly_context
Expand All @@ -195,6 +199,8 @@ async def get_tools(

if self._is_tool_selected(mcp_tool, readonly_context):
tools.append(mcp_tool)

self._loaded_mcp_tools = tools
return tools

async def close(self) -> None:
Expand Down