Skip to content

fix(flows): pair the latest function response with the nearest call - #7271

Open
chelsealong wants to merge 2 commits into
google:mainfrom
chelsealong:fix-7269-rearrange-latest-response-reused-call-id
Open

chelsealong wants to merge 2 commits into
google:mainfrom
chelsealong:fix-7269-rearrange-latest-response-reused-call-id

Conversation

@chelsealong

Copy link
Copy Markdown
Contributor

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

Problem:

rearrange_events_for_latest_function_response (in
google/adk/flows/llm_flows/tools/_rearranger.py) walks the event history
backward looking for the function-call event that matches the latest
function-response's id. When it finds a match it records the index — but the
inner for function_call in function_calls: ... break only breaks the loop
over calls within one event; the outer loop over event indices keeps
walking all the way back to the start of history.

If a function-call id is reused across turns (some model providers, e.g.
Gemini forwarded through LiteLLM, do this), the outer loop overwrites the
match with an earlier, already-answered call carrying the same id. Every
event between that stale call and the latest response — including
intervening user turns — is then silently dropped, and the response gets
merged onto the wrong call. This is the same bug class already fixed for the
sibling function rearrange_events_for_async_function_responses_in_history
in deee6d2c47 (closing #6761): pair a response with the nearest
preceding call sharing its id, not the oldest one.

Solution:

Track whether the inner loop found a match and break the outer loop too, so
the search stops at the first (nearest) preceding call event with a matching
id, mirroring the fix already applied to the sibling rearranger function.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Added test_rearrange_latest_response_reused_call_id_pairs_with_nearest_call
in tests/unittests/flows/llm_flows/tools/test_rearranger.py, reproducing
the issue's repro scenario: a lookup call/response in turn 1, then a
update call reusing the same id in turn 2 with a placeholder response (HITL
pause) followed by the real response (HITL resume).

Verified the test fails without the fix (checked out the pre-fix source with
git checkout HEAD~1 -- src/google/adk/flows/llm_flows/tools/_rearranger.py
before this fix was committed):

$ pytest tests/unittests/flows/llm_flows/tools/test_rearranger.py::test_rearrange_latest_response_reused_call_id_pairs_with_nearest_call -q
...
AssertionError: assert [Event(...)] == [Event(...)]
  At index 1 diff: ... name='update' ... vs ... name='lookup' ...
  Right contains 2 more items, first extra item: Event(... 'q2' ...)
1 failed in 2.61s

With the fix applied:

$ pytest tests/unittests/flows/llm_flows/tools/test_rearranger.py tests/unittests/flows/llm_flows/test_context_shims.py -q
........................                                                 [100%]
24 passed in 2.07s

Full unit suite (pytest tests/unittests -n auto): 16043 passed, 82 skipped, 26 xfailed, 2 xpassed plus one pre-existing, unrelated failure
(test_concurrent_prepare_tables_no_race_condition, a SQLite
prepare_tables concurrency test) that reproduces identically on unmodified
upstream/main before this change, confirming it is unrelated.

pre-commit run --files src/google/adk/flows/llm_flows/tools/_rearranger.py tests/unittests/flows/llm_flows/tools/test_rearranger.py: all hooks passed.

Manual End-to-End (E2E) Tests:

Not applicable — this is a pure function fix covered by unit tests; the
minimal repro in the issue is what the added test encodes.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective.
  • New and existing unit tests pass locally with my changes.

Additional context

This PR was prepared with AI assistance (Claude Code), including the root
cause analysis, patch, test, and verification steps described above.

rearrange_events_for_latest_function_response walked backward looking
for the call matching the latest response's id, but never stopped once
it found one. When a function-call id is reused across turns (some
model providers reuse ids), the search kept walking past the nearest
match and settled on the oldest call sharing that id instead, dropping
every event in between and merging the response onto the wrong call.

Stop at the first (nearest) match, matching the fix already applied to
the sibling function rearrange_events_for_async_function_responses_in_history
for the same class of bug (deee6d2, closing google#6761).

Fixes google#7269
@mkbctrl

mkbctrl commented Sep 24, 2026

Copy link
Copy Markdown

Thanks for this — I ran the branch against a few more histories (both ids of a batch reused, one reused id beside a fresh one, an id reused three times, id-less calls) and it pairs with the nearest call every time, with distinct-id histories unchanged. One coverage gap: on main this bug has a second face. When the reused call was paused beside another call, the walk reaches the older event and the subset check raises ValueError instead of truncating; the added test covers the truncation face only, so a batch case would pin the other (fails on main with that ValueError, passes here):

def test_rearrange_latest_response_reused_id_in_parallel_batch_pairs_with_nearest_call():
  call2 = Event(author="test_agent", content=types.Content(role="model", parts=[
      types.Part(function_call=types.FunctionCall(id="call_1", name="update", args={})),
      types.Part(function_call=types.FunctionCall(id="call_2", name="list", args={}))]))
  paused = Event(author="user", content=types.Content(role="user", parts=[
      types.Part(function_response=types.FunctionResponse(id="call_1", name="update", response={"placeholder": True})),
      types.Part(function_response=types.FunctionResponse(id="call_2", name="list", response={"rows": 3}))]))
  events = [_call_event("call_1", "lookup"), _resp_event("call_1", "lookup", "looked up"), call2, paused, _resp_event("call_1", "update", {"applied": True})]
  result = rearrange_events_for_latest_function_response(events)
  assert result[:3] == events[:3]
  assert [(r.id, r.response) for r in result[-1].get_function_responses()] == [("call_1", {"applied": True}), ("call_2", {"rows": 3})]

Two more that change with this diff and are worth pinning: id=None calls now pair with the nearest id-less call rather than the oldest, and a latest event answering a batch whose partner was answered in its own earlier event no longer raises. One limit, shared with the sibling fix and not a regression: if a newer call reuses a pending call's id and is answered before the pending one resumes, id-only matching hands the resumed result to the newer call; the response name could break that tie if you ever want to go further.

…atch

Adds the batch-call regression test from review: on unfixed code the
outer-loop walk continues past the nearest matching call event to an
older one that doesn't carry every id in the batch, and the subset
check raises ValueError instead of truncating.
@chelsealong

Copy link
Copy Markdown
Contributor Author

Thanks for the extra coverage — added test_rearrange_latest_response_reused_id_in_parallel_batch_pairs_with_nearest_call (matches the repro you gave), pinning the second face: a reused id paused beside another call in a batch. Confirmed it fails with the ValueError you described on the pre-fix source and passes with the fix; full unit suite still green aside from the pre-existing unrelated test_concurrent_prepare_tables_no_race_condition. Pushed as 98888a8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_rearrange_events_for_latest_function_response attributes a reused function-call id to the oldest matching call, silently truncating history

3 participants