Repository navigation
fix: check every function response, not just the first, when resolving which agent to resume - #810
Merged
kalenkevich merged 5 commits intoSep 10, 2026
Conversation
…g which agent to resume Independent finding, not a response to review feedback. Found via a same-day sibling fix in google/adk-python (fix: check every function response, not just the first, when inferring which invocation to resume, commit 5449314) while auditing a routine commit batch -- adk-python's Runner._resolve_invocation_id had the identical gap in the Python port of this same resumption logic. findMatchingFunctionCall resolved which agent to resume after a tool/LRO boundary using only functionResponses[0].id from the last session event. A single event can carry responses answering calls from different agents at once -- for example two long-running operations from sibling sub-agents completing together and being resumed in one message. Resolving from only the first response silently attributed the rest to whichever agent's call happened to come first: that response would then be processed under a resumed agent's context it was never meant for, and the agent it actually answers would never be correctly resumed at all. Dynamically confirmed before fixing: a PoC with two sibling agents (agent_a, agent_b) each raising their own long-running call, resumed together in a single event, resolved to agent_a via determineAgentForResumption -- completely silent about agent_b's response belonging to a different agent entirely. Fix mirrors adk-python's exact fail-closed philosophy, adapted to this function's own return type (an Event, not a bare invocation id): every function response in the last event is now checked, and when the responses present resolve to calls authored by more than one distinct agent, the function throws rather than picking one -- determineAgentForResumption's only caller can resume a single agent, so an event answering calls from two different agents at once has no single correct answer to fail open with. Dynamically confirmed the fix closes the gap: reverted it in isolation, rebuilt, and reproduced the exact silent-misattribution PoC before restoring and confirming it now throws with a message identifying both conflicting agents. Dedup checked before building: no existing GitHub issue, PR, or commit in this repo addresses this specific multi-response resumption-ambiguity pattern. Verified: all 53 pre-existing tests in functions_test.ts pass unmodified against the patched source (no regression), plus 2 new tests (55 total). Broader core/test/runner/ suite (6 files, 152 tests, covering determineAgentForResumption's actual callers) passes with no regressions. tsc --noEmit, eslint, and prettier --check all clean.
Contributor
|
Hi @kalenkevich, I have checked the issue and validated the proposed changes. I was able to reproduce the issue. Could you please review this PR? |
Varun-S10
added
the
needs review
Contributor
Author
|
Hi Team, |
AmaadMartin
reviewed
Sep 8, 2026
AmaadMartin
left a comment
AmaadMartin
left a comment
Collaborator
There was a problem hiding this comment.
The core fix is sound, and the conflict test does fail without it. I read functions.ts and runner.ts at 31b1d58 and traced both new tests by hand; I did not run the suite.
One blocking point: the new throw fires before the caller checks isResumable (core/src/runner/runner.ts:827), so it can abort a run that never wanted resumption.
Sweep: I grepped core/src for the same first-response assumption. functions.ts:755 and :760 were the only hits, so no sibling call site is missed. core/src/telemetry/tracing.ts:192 reads parts[0]?.functionResponse, but that span describes one tool call, so it is a different concern.
Addresses AmaadMartin's review, all four points: 1. Blocking: the conflict throw ran unconditionally, before the caller even read isResumable. determineAgentForResumption (core/src/runner/runner.ts) called findEventByLastFunctionResponseId -> findMatchingFunctionCall on every runAsync for an agent root, not only on resumption, so a session whose last event merely happens to answer two agents' calls at once aborted the run even with resumabilityConfig unset -- Cases 2 and 3 never got a chance to run. Split the conflict check out of findMatchingFunctionCall into a new getConflictingFunctionResponseAuthors (core/src/agents/functions.ts). findMatchingFunctionCall is now a pure, non-throwing lookup, matching the review's request; determineAgentForResumption calls the new function only inside the isResumable-gated branch, after the gate this needed to sit behind. Both functions share one internal walk (resolveFunctionResponseMatch) so the matching rules live in exactly one place rather than being duplicated between them. Added two tests directly exercising the fixed gating in core/test/runner/runner_test.ts: a conflicting-author session with isResumable: false must not throw and must fall through to Case 2, and the same session with isResumable: true must still throw. Mutation-verified: temporarily moved the check back to unconditional and confirmed the first test fails while the second still passes, then restored the fix and confirmed both pass again. 2. The PR description claimed this mirrors adk-python without qualifying that; it doesn't, in two ways. Traced adk-python's actual behavior (runners.py, functions.py) to describe this accurately rather than restate the original, overstated claim -- see the separate PR body response for the full comparison and reasoning: conflict is keyed on author here vs. invocation_id in Python's _resolve_invocation_id_from_fr, and an unmatched response id is skipped here vs. raised on there, because this function's caller has two further fallback cases (Case 2, Case 3) that a Python-style raise would short-circuit past unnecessarily. Also noted that Python's own find_matching_function_call -- the function this PR's name most directly echoes -- still reads function_responses[0] only, and isn't the function this behavior actually resembles. 3. Documented which match wins when multiple responses resolve to the same author, and the observable consequence for external callers: the last-resolved event by iteration order wins (unchanged from 31b1d58's behavior, now stated explicitly), which differs from the pre-PR code's effective first-wins behavior (reading only functionResponses[0]) -- an external caller of the public findEventByLastFunctionResponseId can get a different event and id than before this fix, even though determineAgentForResumption itself is unaffected, since it only reads .author. 4. The original "checks every function response... not just the first" test passed even without the fix, since both responses in its fixture independently pointed at events with different authors regardless of read order -- it exercised the conflict path, not the "first response doesn't resolve, second one does" path the fix is actually for. Replaced it with exactly that case: response 0 carries an id matching no function call, response 1 resolves to a real one. Confirmed by hand that the pre-fix code (reading only functionResponses[0].id) returns undefined for this input, while the fix correctly returns the match from response 1. Verified beyond the four review points: built the package clean (tsc --emitDeclarationOnly + bundler, no errors) and ran the full unit:core suite -- 3429 tests across 230 files, all passing, confirming no other caller of findMatchingFunctionCall depends on its removed throwing behavior (grepped for other call sites: only runner.ts's already-updated findEventByLastFunctionResponseId and common.ts's re-export).
Contributor
Author
|
Thanks for the thorough scratch-script verification. All four addressed: split the conflict check out of the pure lookup, moved it behind the resumability gate, mutation-tested that the gating actually works (reverted it, confirmed the new test catches the regression, restored it). Corrected the "mirrors adk-python" framing with an accurate comparison — posting that separately since it's a few paragraphs. Documented the last-wins behavior and its effect on external callers of findEventByLastFunctionResponseId. Replaced the test that passed without the fix with the actual "response 0 doesn't resolve, response 1 does" case, confirmed by hand against the pre-fix code. Ran the full unit:core suite (3429 tests) clean on top of the targeted files. |
kalenkevich
approved these changes
Sep 9, 2026
kalenkevich
left a comment
kalenkevich
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, please fix the CI
…tion-response Only core/test/runner/runner_test.ts conflicted, and only because both sides appended a test to the determineAgentForResumption describe block: main's inline-attachment redaction test (google#817) and this branch's two conflicting-author tests. Both are kept.
…check
Formatting only, plus one doc-comment link. Prettier collapsed two
wrapped expressions, and typedoc's --treatWarningsAsErrors rejected
{@link getConflictingFunctionResponseAuthors}: that helper is reachable
only from core/src/agents/functions.ts and is not re-exported through
core/src/common.ts, so it falls outside the documented entry point and
the link had no target. Demoted to a code span rather than widening the
public API surface just to make a link resolve.
…tion-response Only core/src/agents/functions.ts conflicted, against the docs-only google#628. That commit rewrote findMatchingFunctionCall's doc to describe the last event's *first* function response -- the behavior this branch replaces -- so the branch's own doc is kept. google#628's other rewrites, including findEventByFunctionCallId's, are taken as-is, and no 'Mirrors Python ADK's' phrasing remains in the file.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix in google/adk-python (commit 5449314) while auditing a routine commit batch -- adk-python's
Runner._resolve_invocation_idhad the identical gap in the Python port of this same resumption logic.findMatchingFunctionCallresolved which agent to resume after a tool/LRO boundary using onlyfunctionResponses[0].idfrom the last session event. A single event can carry responses answering calls from different agents at once (e.g. two long-running operations from sibling sub-agents completing together and being resumed in one message). Resolving from only the first response silently attributed the rest to whichever agent's call happened to come first.Dynamically confirmed before fixing with a PoC: two sibling agents each raising their own long-running call, resumed together in one event, resolved to only the first agent -- completely silent that the second response belonged to a different agent.
Fix mirrors adk-python's exact fail-closed philosophy: every response in the last event is checked, and if they resolve to calls from more than one distinct agent, the function throws rather than picking one.
Verified: all 53 pre-existing tests pass unmodified, plus 2 new tests (55 total). Broader
core/test/runner/suite (6 files, 152 tests) passes with no regressions.tsc --noEmit,eslint,prettier --checkall clean. Dynamically confirmed by reverting the fix in isolation and reproducing the exact silent-misattribution PoC before restoring.