fix(runners): dispatch run_after_run_callback in _run_node_async (#5282)#5323
Open
fix(runners): dispatch run_after_run_callback in _run_node_async (#5282)#5323
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. |
…gle#5282) Runner._run_node_async had a TODO at runners.py:427 for node runtime plugin lifecycle, and never dispatched plugin_manager.run_after_run_callback. Because both Workflow(BaseNode) and (post a3 refactor) LlmAgent roots funnel through _run_node_async, any BasePlugin subclass that overrides after_run_callback silently no-op'd — breaking the canonical pattern for memory persistence, metrics emission, and post-run audit hooks. This wires one dispatch call after _consume_event_queue drains, mirroring the legacy path in _exec_with_plugin at runners.py:1230 exactly: outside the finally block, so semantics match legacy (fires on natural drain only, skipped on error in the loop, skipped on caller-break via GeneratorExit). Also: - Narrows the TODO at runners.py:427 from "tracing and plugin lifecycle" to "tracing" since plugin lifecycle is now wired. - Incidental pyink reformat at runners.py:1451: pre-existing 82-char line on v2 HEAD, unrelated to google#5282 but required by the pyink CI gate. Companion to test-only PR google#5301 (which established the regression anchor). This PR ships the fix plus flips the anchor by replacing the WorkaroundRunner scaffolding with a direct positive assertion. Alternatives considered and rejected: - Wrap in finally to fire on error/break — changes legacy contract. - Lift dispatch to run_async call sites — requires plumbing ic out of _run_node_async, bigger blast radius. - Extract a shared _with_plugin_lifecycle context manager — right long-term shape, but refactors the legacy path too and expands scope. Fixes google#5282
723283f to
073da99
Compare
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
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.
Summary
Fixes #5282 by wiring
plugin_manager.run_after_run_callbackintoRunner._run_node_async, closing therunners.py:427TODO for node runtime plugin lifecycle.Companion to test-only PR #5301 (which established the xfail regression anchor on stock 2.0.0a3). This PR ships the source fix plus flips the anchor by replacing the
WorkaroundRunnerscaffolding with a direct positive assertion.The fix (3 lines)
Placement mirrors the legacy dispatch at
runners.py:1230exactly: outside thefinallyblock, so semantics match legacy —_exec_with_pluginfinally)GeneratorExitatyieldjumps tofinally)Full semantic equivalence. Single insertion point covers both
LlmAgentandBaseNoderoot types becauserun_asyncBranches A (line 807) and B (line 838) both funnel into_run_node_async. This also removes the last pre-condition to collapsing the two branches per the TODO atrunners.py:836-837.Also in this PR
runners.py:427from "tracing and plugin lifecycle" to "tracing" — plugin lifecycle is now wired, tracing remains out of scope for this fix.runners.py:1451— pre-existing 82-char line onv2HEAD, unrelated to Custom memory persistence silently no-ops on Workflow (BaseNode) roots in 2.x — no working post-run hook #5282, but required by the pyink CI gate since the workflow runspyink --checkon the whole changed file. Keeping this out of a separate PR to avoid churn.tests/unittests/runners/test_issue_5282.py— replaces the xfail-anchored version from test: regression tests for #5282 (after_run_callback on BaseNode roots) #5301 with two positive assertions:test_workflow_root_dispatches_pre_run_and_event_hooks(baseline)test_workflow_root_dispatches_after_run_callback(regression anchor, no xfail)Alternatives considered and rejected
finallyblock — would fireafter_run_callbackon error and caller-break, changing the legacy contract. Plugin authors wrote handlers expecting "successful completion" semantics; flipping to "always fires" is a breaking change.run_asynccall sites (lines 825, 841) —icis created inside_run_node_asyncat line 446, so this would require plumbingicout of the generator. Expands blast radius._with_plugin_lifecycle(ic)shared context manager — right long-term shape, but refactors the legacy_exec_with_plugintoo (which has early-exit logic insidebefore_run_callbackthat's awkward to extract). Out of scope for a fix PR.Test plan
pytest tests/unittests/runners/test_issue_5282.py -v— 2 passedpytest tests/unittests/runners/(full subdir) — 69 passed, 1 skipped, 5 xfailed (all pre-existing), 0 new failuresisort --check— cleanpyink --check --config pyproject.toml— cleanpyink.yml,isort.yml,python-unit-tests.yml,mypy.yml,mypy-new-errors.ymlLinks
run_livepath)on_agent_error,on_run_error) to ADK Framework #4774 (motivates whyafter_run_callbackis load-bearing for enterprise telemetry)🤖 Generated with Claude Code