Fix: Process function_response from separate request by searching full history Fixes #3531 #3553
Conversation
…ync function_response Fixes google#3531
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @miguelabdonsh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical regression that prevented the correct processing of asynchronous function responses in the system. By extending the search scope for corresponding function calls to the entire session history, the change ensures that Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
adk-bot
added
the
core
|
Response from ADK Triaging Agent Hello @miguelabdonsh, thank you for your contribution! Before we can merge this PR, you'll need to sign a Contributor License Agreement (CLA). You can find more information in our contribution guidelines. Once the CLA is signed, we can proceed with the review. Thanks! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a regression where a function_response arriving in a separate request could not find its corresponding function_call. The fix correctly expands the search to the full session history. The addition of a new unit test to validate this asynchronous scenario is a great improvement. However, I've identified a critical issue in the implementation where an index from the full history is incorrectly used on a filtered list of events, which could lead to errors. Please see my detailed comment.
| search_events = all_events if all_events else events | ||
| function_call_event_idx = -1 | ||
| # look for corresponding function call event reversely | ||
| for idx in range(len(events) - 2, -1, -1): | ||
| event = events[idx] | ||
| for idx in range(len(search_events) - 2, -1, -1): | ||
| event = search_events[idx] | ||
| function_calls = event.get_function_calls() | ||
| if function_calls: | ||
| for function_call in function_calls: |
There was a problem hiding this comment.
There appears to be a critical issue with how the index from this search is used. The function_call_event_idx is calculated by iterating over search_events (which can be the full history all_events), but this index is later used to slice and access elements in events (the filtered list).
Since events is a processed and filtered version of search_events, their indices are not interchangeable. This will likely lead to an IndexError or incorrect logic when the lists differ in length or content.
To resolve this, after finding the matching event at idx in search_events, you should find the corresponding index of that same event within the events list (for example, by matching event.id). This new, correct index should then be used for all subsequent operations on events.
ryanaiagent
added
the
request clarification
|
Hi @miguelabdonsh , Thank you for your work on this pull request. We appreciate the effort you've invested. |
matching Replace identity comparison (is) with attribute-based comparison using invocation_id and timestamp. This ensures correct event matching even when events are deep-copied during processing. The previous identity comparison would fail after model_copy(deep=True) transformations. Updated test to verify behavior with cloned events.
Hi @ryanaiagent, the issue was already fixed in commit 74b034f, we now search for the event in search_events, then map it to the correct index in the filtered events list. I've also improved the matching to use invocation_id + timestamp instead of identity comparison, making it robust against deep-copied events. |
contents.py
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a ValueError that occurred when a function_response from a separate request couldn't find its corresponding function_call. The fix correctly modifies _rearrange_events_for_latest_function_response to search the full session history, not just the events from the current request. The changes are logical, and the addition of a new unit test to validate the fix for asynchronous scenarios is a great improvement. My review includes one suggestion to make the event matching logic more robust.
| if ( | ||
| event.invocation_id == function_call_event.invocation_id | ||
| and event.timestamp == function_call_event.timestamp | ||
| ): |
There was a problem hiding this comment.
For identifying an event, it's more robust to use the event.id field, which is a unique UUID. Comparing invocation_id and timestamp can be brittle for the following reasons:
invocation_idis not unique to a single event; multiple events can share one.timestampis a float, and direct equality comparison (==) can be unreliable due to floating-point precision issues.
Using event.id provides a more reliable way to match the exact event.
if event.id == function_call_event.id:
There was a problem hiding this comment.
This is consistent with how event uniqueness is managed in other parts of the ADK. Can you implement this suggestion.
3 participants
Problem
When sending a function_response via a separate
/run_sserequest (e.g., asyncjob completion callback),
_rearrange_events_for_latest_function_responseraised:
ValueError: No function call event found for function responses ids
The function only searched events in the current request context
(
filtered_events), not the full session history. When a function_responsearrived in a separate request, it couldn't find the matching function_call from
the previous request.
Regression from ADK 1.17.0 → 1.18.0.
Solution
Modified
_rearrange_events_for_latest_function_responseto search the fullsession history (
rewind_filtered_events) instead of only current requestevents.
Changes in
src/google/adk/flows/llm_flows/contents.py:- Added
all_eventsparameter (line 128)- Search in
all_eventsif provided, otherwise fall back toevents(line165-169)
- Pass rewind_filtered_events when calling the function (line 440-441)
Testing
✅ All 30 existing tests in
test_contents*.pypass✅ New test
test_function_response_in_separate_request.pyverifies fix✅ Test simulates async scenario: function_call in early session history,
function_response arrives later