fix: cancel the invocation when a sync run() generator is closed early - #6564
fix: cancel the invocation when a sync run() generator is closed early#6564CTWalk wants to merge 4 commits into
Conversation
|
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. |
52731b0 to
7fabbf1
Compare
a27d8c0 to
55acd42
Compare
|
Rebased onto The conflict was with b55000d ("fix: re-raise agent errors from the synchronous The rebase made this patch smaller. That change's Re-verified on the new base:
The PR description has been updated to match the rebased patch, including replacing two verification claims that no longer applied to this base. |
55acd42 to
5a32cb8
Compare
|
Added a second test, and updated the description to match.
I checked the tests against the plausible wrong fixes rather than only against the bug:
The new test calls Also now stated explicitly in the description: teardown reaches the agent as
|
5a32cb8 to
7c20c34
Compare
Closing the generator returned by the synchronous Runner.run() did not stop the invocation it started. The agent kept running on the background event loop and could append further events to the session after Generator.close() returned. Runner._cleanup_root_task() documents that the root task must be cancelled when the caller stops iterating early, and test_run_async_teardown_on_aclose pins that behavior for the async entrance. The sync wrapper starts run_async() in a background thread but never propagated the foreground generator's close to that task. Hand the background event loop and task to the foreground generator, tell normal queue exhaustion apart from an early exit, and on early exit only cancel the background task -- which unwinds through the existing aclosing(...) and so reuses run_async()'s own _cleanup_root_task() teardown. Treat the resulting CancelledError as expected thread teardown, and join the thread on both paths so close() does not return while the invocation is still alive. Adds test_run_teardown_on_close, the sync counterpart of the existing test_run_async_teardown_on_aclose.
7c20c34 to
100026d
Compare
Component:
core—Runner.run()insrc/google/adk/runners.py.Summary: closing the generator returned by the synchronous
Runner.run()does not stop the invocation behind it, so the agent keeps running and can
append events to the session after
close()has returned. The async twinalready cancels correctly on
aclose(); the sync wrapper never propagatesclose to the background task. This cancels it on early exit only, reusing
run_async()'s existing teardown, and adds the sync counterpart of the testthat pins the async behavior.
Describe the bug
Closing the generator returned by the synchronous
Runner.run()does not stopthe invocation it started. The agent keeps running on the background event loop
and can append further events to the session after
Generator.close()hasreturned.
Runner._cleanup_root_task()documents the intended behavior: when the callerstops iterating early, the root task must be cancelled to avoid a leaked task.
test_run_async_teardown_on_aclosepins that for the async entrance. The syncwrapper starts
run_async()in a background thread but never propagates theforeground generator's close to that task.
I did not find an existing issue for this, so I have followed the bug-template
structure in this description as
CONTRIBUTING.mdsuggests. Happy to open aseparate issue first if you would prefer that.
Steps to reproduce
Keyless — no model or network call.
The agent blocks on a thread event before its second yield, so the ordering is
deterministic: the second event can only be produced after
close()has alreadyreturned.
Observed behavior (before this change)
Expected behavior (after this change)
stream.close()cancels the underlying invocation before any further agent workor session append, matching
run_async().aclose().Root cause
Runner.run()runs_invoke_run_async()in a background event-loop thread andconsumes an event queue in the foreground generator.
Generator.close()raisesGeneratorExitat the foregroundyield; the frame exits without cancellingthe background task and without joining the thread. The background invocation
stays free to run tools, emit events, and mutate session history.
What this change does
Runner.run()is the only function changed insrc/:through a one-item queue;
early exit;
loop.call_soon_threadsafe(task.cancel)— which unwinds through the existingaclosing(...)and so reusesrun_async()'s own_cleanup_root_task()teardown;
close()does not return while theinvocation is still alive.
No public signature, dependency, documentation, or unrelated error behavior
changes. Normal full-consumption runs take the same path as before.
Behavioral note for reviewers:
close()now blocks until teardown completes,which is the same contract
run_async().aclose()already has (it awaits thecancelled root task). An agent that swallows
CancelledErrorand keepsrunning will therefore delay
close(), exactly as it already delaysaclose().I measured both entrances against an agent that holds the signal for 1s: sync
close()blocked 1002.9 ms, asyncaclose()blocked 1002.4 ms.One asymmetry worth naming, since the two paths stop the agent by different
means: teardown reaches the agent as
CancelledErroronrun().close()and asGeneratorExitonrun_async().aclose(). The observable contract is the same —the invocation stops and appends nothing further — and the existing
test_run_async_teardown_on_aclosealready treats the two as one case bycatching
(asyncio.CancelledError, GeneratorExit)together, which the new synctests mirror. Happy to unify the signal type instead if you would prefer that.
I measured the cases that note implies, on the same pin:
The abandoned-stream case is worth calling out: before this change, dropping
the last reference to a partially consumed stream left the invocation running
to completion in a detached thread. After it, the generator's own finalizer
cancels the invocation and returns promptly, so the abandonment path stops
leaking as well.
Testing plan
Unit tests
Added
test_run_teardown_on_closeintests/unittests/test_runners.py, thesync counterpart of the existing
test_run_async_teardown_on_aclose. Itconsumes the first event, closes the stream, and asserts that the agent was
cancelled, did not complete, and appended no later event to the session. The
agent's wait is bounded, so a broken teardown fails the test rather than
hanging it.
A second test,
test_run_close_cancels_agent_parked_without_timers, pins thepart the first one cannot see. Its agent parks on a future and schedules no
timer, so nothing wakes the background event loop on its own. Cancelling the
task without
call_soon_threadsafesets the flag but never wakes the selector,and
close()blocks forever; a polling agent hides this, because its ownsleeps keep waking the loop. The test calls
close()on a helper thread andasserts it returns, so that failure mode fails the test instead of hanging it,
and it releases the agent in a
finallyso the non-daemon runner thread alwaysexits.
As a sanity check that the tests are not vacuous: keeping them and reverting
only
runners.pymakes them fail on the cancellation assertion, so they failon today's
mainand pass with this change. I also checked them against theplausible wrong fixes — dropping the
thread.join(), cancelling withoutcall_soon_threadsafe, and not cancelling at all — and each is caught.Ran locally against
989721746aba65e90f644e51606375699175f709:The runner-adjacent sweep (
-k "runner or run_"acrosstests/unittests)passes as well: 866 passed, with one failure —
test_import_loading.py::test_entry_point_loads_only_allowlisted_packages[runner]— that reproduces identically on unmodified
main, so it is pre-existing andnot caused by this change. Development host was macOS 15.7.4 / CPython 3.11.14.
Scope of what I did not run, so this is not read as a CI claim: the
toxmatrix, other Python versions, and
tests/unittests/evaluationandtests/unittests/optimization, which do not collect in my local environmentfor want of optional dependencies.
Manual E2E (Runner)
Runner setup and agent definition: the reproducer in "Steps to reproduce"
above — an in-memory session service, a deterministic two-event
BaseAgent,and
Runner.run(). Command:Console output before the change:
Console output after the change:
The relevant lines are
cancelledflipping toTrueand'second'disappearing from the persisted session events: the invocation stops at close
instead of running on and appending.
Related, not duplicates
This change is confined to
core; none of the items below overlap with it, andnone of them are the component this PR belongs to.
disappearing across the sync thread boundary, not early-close teardown.
run_async()from outside the agent #4796 requests an external "stop generating" handle forrun_async(). Thischange does not add a new API; it makes the existing generator-close path
behave as documented.
b55000d9("fix: re-raise agent errors from the synchronousRunner.run()")landed upstream after this PR was filed and rewrote the same function. This
branch is rebased onto it, and the two compose rather than conflict: that
change forwards an agent failure to the calling thread, this one cancels the
invocation when the caller stops iterating. The rebase made this patch
smaller — its
except BaseExceptionhandler already absorbs theCancelledErrorraised by teardown, so the explicit suppression this PRoriginally carried is gone.
test_run_reraises_agent_error,test_run_yields_events_before_reraising_agent_error, andtest_run_reports_agent_cancellation_as_runtime_errorall still pass.context-detach error on early
run_async()close, intelemetry/_instrumentation.py. No file, symbol, or behavior is shared withthis PR.
Environment
google-adk2.6.1, editable checkout at989721746aba65e90f644e51606375699175f709deterministic agent and make no model or network call.