test(remotagent) deflake a2a cleanup propagation#1131
Open
wolo-lab wants to merge 3 commits into
Open
Conversation
2636074 to
f99c4cb
Compare
Under CPU contention / low parallelism the test hit two problems: - The detached CancelTask goroutine was never joined (cancelResultChan was created but never read), so a slow RPC could still be in flight at teardown, fail on the cancelled t.Context(), and call t.Errorf on a finished test, which panics. Both detached goroutines are now joined via a WaitGroup and t.Cleanup. - The parent was cancelled right after the first status event, before the remote agent had received any subagent event. In that window the parent does not know the subagent's server-assigned task ID, so cancellation cannot propagate and subagent cleanup never fires. The test now waits until the subagent's own output round-trips to the client before cancelling. The single fixed 5s deadline shared across all cleanup waits is also replaced with a per-wait, contention-tolerant bound. Test-only change.
f99c4cb to
dccbf00
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
TestA2ACleanupPropagation(agent/remoteagent/v2/a2a_e2e_test.go)tafter the test completes.Test-only change; no production code is modified.
Background / root cause
The test exercised two failure modes:
Log in goroutine after test has completed. TheCancelTaskgoroutine was never joined (
cancelResultChanwas created but never read),so under load it could still be mid-RPC when the test returned, fail on the
cancelled
t.Context(), and callt.Errorfon a finished test.remote cleanup was not called. This was not merely a too-tighttimeout. Via goroutine dumps + tracing I confirmed the test cancelled the
parent task right after the first status event — i.e. before the remote
agent had received any event from the subagent. In that window the remote
agent doesn't yet know the subagent's (server-assigned) task ID, so
cleanupRemoteTaskreturns early (lastEvent == nil) and cannot propagatethe cancellation. Under
GOMAXPROCS=1/ CPU contention this race was lostdeterministically, so the subagent task kept running and its cleanup never
fired.
What changed
sync.WaitGroup+t.Cleanup(wg.Wait)join both detached goroutines before the test tears down (fixes the panic in all exit paths).time.After(5s)shared across all cleanup-channel reads with a per-wait, contention-tolerant helperawaitN.Note on scope (best-effort propagation)
adk-go's remote-task cancellation propagation (
cleanupRemoteTask) is anadditive, best-effort feature; adk-python's remote A2A agent does not cancel
remote tasks at all. Cancelling in the window before the remote agent has
received its first subagent event genuinely cannot propagate (the remote task
ID is unknown to the caller). This PR fixes the flaky test to respect that
contract; making propagation robust in that early-cancel window would be a
separate, behaviour-changing effort (client-assigned task IDs and/or a2a-go
support) and is intentionally out of scope here.