Skip to content

fix(a2a): send long-running calls once from the legacy executor - #7252

Open
chelsealong wants to merge 1 commit into
google:mainfrom
chelsealong:fix/a2a-legacy-long-running-call-duplicate
Open

chelsealong wants to merge 1 commit into
google:mainfrom
chelsealong:fix/a2a-legacy-long-running-call-duplicate

Conversation

@chelsealong

Copy link
Copy Markdown
Contributor

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

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

This is a partial fix for #7247 — see "Additional context" below for what
is and isn't covered.

Problem:

A2aAgentExecutor._handle_request (the legacy, default streaming path used
by to_a2a() + RemoteA2aAgent on a2a-sdk 1.x) streams every ADK event as
a TaskStatusUpdateEvent with its state masked to WORKING
(TaskResultAggregator.process_event), then afterwards republishes the
same content again in the terminal event built from whatever was
captured as task_status_message.

For a tool declared with require_confirmation=True, the
adk_request_confirmation function call is therefore delivered twice with
the same call id: once inline as a WORKING update (surfaced by
RemoteA2aAgent as a thought=True preview), and again as the real
INPUT_REQUIRED/AUTH_REQUIRED terminal event. Both copies get yielded by
Runner.run_async and persisted in the caller's session.

Solution:

Hold long-running call parts back from the intermediate stream — instead of
letting them go out masked as WORKING — and attach them only to the
terminal status event, exactly once. This mirrors what the newer (v2)
executor already does via LongRunningFunctions.process_event, so the
change reuses the same detection helpers (is_long_running_part/
is_euc_part, extracted from _create_status_update_event so both paths
share one implementation) instead of adding new logic.

Testing Plan

Unit Tests:

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

Added test_handle_request_sends_long_running_call_once to
tests/unittests/a2a/executor/test_a2a_agent_executor.py: a real Runner

  • LlmAgent with a require_confirmation=True FunctionTool, run through
    the legacy A2aAgentExecutor, asserting the adk_request_confirmation
    call appears in the enqueued events exactly once and the terminal event's
    state is INPUT_REQUIRED.

Confirmed it fails without the fix (git checkout HEAD~1 -- <source files>,
keeping the new test):

FAILED tests/unittests/a2a/executor/test_a2a_agent_executor.py::TestA2aAgentExecutor::test_handle_request_sends_long_running_call_once - assert 2 == 1
1 failed, 30 deselected, 11 warnings in 2.40s

And passes with the fix restored:

$ pytest tests/unittests/a2a/executor/test_a2a_agent_executor.py -k test_handle_request_sends_long_running_call_once -q
1 passed, 30 deselected, 11 warnings in 4.57s

Full a2a suite:

$ pytest tests/unittests/a2a -q
724 passed, 49 skipped, 44 warnings in 4.86s

Full unit suite (pytest tests/unittests -n auto): 15859 passed, 82
skipped, 26 xfailed, 2 xpassed, 2 failed. The 2 failures
(test_interactions_utils.py::...test_unrecognized_delta_logs_raw_warning_and_drops
and test_litellm.py::...test_function_declaration_to_tool_param_response_schema_exceeds_max_length_budget)
are pre-existing/order-dependent — reproduced on git stash (pre-change
tree) and unrelated to a2a; the litellm one passes cleanly when run in
isolation.

pre-commit run (ruff, isort, pyink, addlicense, ADK compliance checks,
codespell) passes on the changed files.

Manual End-to-End (E2E) Tests:

Not run — would need the three-way to_a2a() server + RemoteA2aAgent
setup from the issue's own repro script, which the unit test above
exercises equivalently through the real Runner/LlmAgent/executor stack
without the network roundtrip.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

Issue #7247 reports two distinct duplications on the legacy executor's
streaming path:

  1. A completed text answer is delivered twice (once as a masked WORKING
    update, once as the final artifact).
  2. A require_confirmation tool call's adk_request_confirmation request
    is delivered twice with the same id (once masked WORKING, once as the
    real INPUT_REQUIRED/AUTH_REQUIRED terminal event).

This PR fixes (2), following the issue's own suggested direction for that
part verbatim ("send the call once, in the terminal
INPUT_REQUIRED/AUTH_REQUIRED status; keep it out of intermediate
updates, as LongRunningFunctions.process_event does in v2"). Long-running
pauses are the more consequential half of the bug — a duplicated call with
a stable id is more likely to confuse a caller resuming the flow than a
duplicated final answer.

(1) is left open. The issue's own writeup frames it as a choice between
several alternatives ("either close with a COMPLETED status and no
repeated parts, or stream agent content as artifact updates, the way v2
does"), which changes the completion contract for every legacy-executor
run, not just the long-running-call path, and touches behavior several
existing tests pin down explicitly
(test_handle_request_with_working_state_publishes_artifact_and_completed
and friends). That's a bigger, more design-sensitive change that deserves
its own PR and maintainer input on which of the alternatives to take,
rather than being bundled into this scoped fix.


This change was written with AI assistance (Claude Code), with the
diagnosis and fix verified by running the repo's real unit tests
(regression test added and shown to fail without the fix / pass with it)
and pre-commit locally, as described above.

A2aAgentExecutor._handle_request streams every ADK event as a masked
WORKING status update, then republishes the last event's content again
in the terminal event. For a require_confirmation tool call, that means
the adk_request_confirmation function call reaches RemoteA2aAgent (and
any other streaming caller) twice: once as a thought=True preview and
once as the real INPUT_REQUIRED/AUTH_REQUIRED pause, with the same call
id both times.

Hold long-running parts back from the intermediate stream and attach
them only to the terminal status event, mirroring how the new (v2)
executor's LongRunningFunctions already handles this.

This addresses the adk_request_confirmation half of google#7247. The
companion issue there (a completed text answer also being duplicated
between the last streamed update and the final artifact) is a separate,
broader change to the non-long-running completion path and is left for
a follow-up.
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.

2 participants