Skip to content

Commit 869b1e7

Browse files
fix(flows): prevent double-execution of FunctionTools returning None in thread pool
When RunConfig.tool_thread_pool_config is enabled, _call_tool_in_thread_pool used None as a sentinel to distinguish "FunctionTool ran in thread pool" from "non-FunctionTool sync tool, needs async fallback". Because None is also a valid return value from any FunctionTool whose underlying function has no explicit return statement (implicit None), the sentinel check failed and execution fell through to tool.run_async(), invoking the function a second time silently. Replace the None sentinel with a dedicated _SYNC_TOOL_RESULT_UNSET object so that a legitimate None result from a FunctionTool is correctly returned on the first execution, without triggering the async fallback path. Fixes #5284
1 parent abcf14c commit 869b1e7

File tree

2 files changed

+75
-3
lines changed

2 files changed

+75
-3
lines changed

src/google/adk/flows/llm_flows/functions.py

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,13 @@
6868
_TOOL_THREAD_POOLS: dict[int, ThreadPoolExecutor] = {}
6969
_TOOL_THREAD_POOL_LOCK = threading.Lock()
7070

71+
# Sentinel object used to distinguish a FunctionTool that legitimately returns
72+
# None from a non-FunctionTool sync tool that skips thread-pool execution.
73+
# Using None as a sentinel would cause tools whose underlying function has no
74+
# explicit return statement (implicit None) to fall through to the async
75+
# fallback path and execute a second time.
76+
_SYNC_TOOL_RESULT_UNSET = object()
77+
7178

7279
def _is_live_request_queue_annotation(param: inspect.Parameter) -> bool:
7380
"""Check whether a parameter is annotated as LiveRequestQueue.
@@ -159,13 +166,14 @@ def run_sync_tool():
159166
}
160167
return tool.func(**args_to_call)
161168
else:
162-
# For other sync tool types, we can't easily run them in thread pool
163-
return None
169+
# For other sync tool types, we can't easily run them in thread pool.
170+
# Return the sentinel so the caller knows to fall back to run_async.
171+
return _SYNC_TOOL_RESULT_UNSET
164172

165173
result = await loop.run_in_executor(
166174
executor, lambda: ctx.run(run_sync_tool)
167175
)
168-
if result is not None:
176+
if result is not _SYNC_TOOL_RESULT_UNSET:
169177
return result
170178
else:
171179
# For async tools, run them in a new event loop in a background thread.

tests/unittests/flows/llm_flows/test_functions_thread_pool.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,70 @@ def blocking_sleep() -> dict:
277277
event_loop_ticks >= 5
278278
), f'Event loop should have ticked at least 5 times, got {event_loop_ticks}'
279279

280+
@pytest.mark.asyncio
281+
async def test_sync_tool_returning_none_executes_exactly_once(self):
282+
"""Test that a sync FunctionTool returning None is not double-executed.
283+
284+
Regression test for https://github.com/google/adk-python/issues/5284.
285+
A FunctionTool whose underlying function returns None (e.g. side-effect-
286+
only tools with no explicit return statement) must execute exactly once.
287+
Previously the None return was mistaken for the internal sentinel used to
288+
signal 'non-FunctionTool, fall back to run_async', causing a second
289+
invocation via tool.run_async().
290+
"""
291+
call_count = 0
292+
293+
def side_effect_only() -> None:
294+
nonlocal call_count
295+
call_count += 1
296+
# No return statement — implicit None.
297+
298+
tool = FunctionTool(side_effect_only)
299+
model = testing_utils.MockModel.create(responses=[])
300+
agent = Agent(name='test_agent', model=model, tools=[tool])
301+
invocation_context = await testing_utils.create_invocation_context(
302+
agent=agent, user_content=''
303+
)
304+
tool_context = ToolContext(
305+
invocation_context=invocation_context,
306+
function_call_id='test_id',
307+
)
308+
309+
result = await _call_tool_in_thread_pool(tool, {}, tool_context)
310+
311+
assert result is None
312+
assert call_count == 1, (
313+
f'Tool function executed {call_count} time(s); expected exactly 1.'
314+
)
315+
316+
@pytest.mark.asyncio
317+
async def test_sync_tool_returning_explicit_none_executes_exactly_once(self):
318+
"""Test that explicit None return from FunctionTool is not double-executed."""
319+
call_count = 0
320+
321+
def explicit_none_return() -> None:
322+
nonlocal call_count
323+
call_count += 1
324+
return None # Explicit None return.
325+
326+
tool = FunctionTool(explicit_none_return)
327+
model = testing_utils.MockModel.create(responses=[])
328+
agent = Agent(name='test_agent', model=model, tools=[tool])
329+
invocation_context = await testing_utils.create_invocation_context(
330+
agent=agent, user_content=''
331+
)
332+
tool_context = ToolContext(
333+
invocation_context=invocation_context,
334+
function_call_id='test_id',
335+
)
336+
337+
result = await _call_tool_in_thread_pool(tool, {}, tool_context)
338+
339+
assert result is None
340+
assert call_count == 1, (
341+
f'Tool function executed {call_count} time(s); expected exactly 1.'
342+
)
343+
280344
@pytest.mark.asyncio
281345
async def test_sync_tool_exception_propagates(self):
282346
"""Test that exceptions from sync tools propagate correctly."""

0 commit comments

Comments
 (0)