test(remoteagent): name the awaited event instead of panicking on a closed channel - #1400
Open
harshitwandhare wants to merge 3 commits into
Open
test(remoteagent): name the awaited event instead of panicking on a closed channel#1400harshitwandhare wants to merge 3 commits into
harshitwandhare wants to merge 3 commits into
Conversation
…losed channel The A2A cleanup-propagation tests took the first status update with a bare receive: taskID := (<-statusUpdateEventChan).TaskInfo().TaskID The producing goroutine defers close on that channel and returns early when SendStreamingMessage fails, after recording the error with t.Errorf. The receive then yields the zero value of a2a.Event, which is a nil interface, and TaskInfo dereferences it. A streaming error therefore surfaced as a nil-pointer panic that killed the test binary and buried the error that had just been recorded. Add testutil.AwaitValue, the one-value counterpart to AwaitN. It treats a closed channel as a failure rather than a receive, since callers use the value, and it reports which value never arrived. Use it for the three ad-hoc receives in these two tests. That also brings the v2 variant in line with the compat one, which already guarded remoteTaskIDChan with its own select, and replaces that ad-hoc 1s bound with the shared deadline so a loaded machine cannot trip it. Also correct the call-site comments and document the deadline: AwaitN arms one timer for all n receives, so it is a budget for the whole call, not per wait as the comments claimed. This does not fix the flake in google#1298. It makes a failure name the event it was waiting for instead of panicking or hanging anonymously, which is step 1 of what that issue asks for. Part of google#1298.
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.
Link to Issue or Description of Change
Not
Closes: this does not fix that flake. See the end of the Solution section.Problem:
TestA2ACleanupPropagationandTestCompat_A2ACleanupPropagationtake the first status update with a bare receive:The goroutine feeding that channel does
defer close(statusUpdateEventChan)and returns early whenSendStreamingMessagefails, after recording the error witht.Errorf. The receive on the closed channel then yields the zero value ofa2a.Event, which is a nil interface, andTaskInfo()dereferences it.So a streaming error surfaces as a nil-pointer panic that kills the test binary and buries the error that was just recorded. Forcing that error path on
mainshows both halves:Two smaller things in the same two tests:
remoteTaskID := <-remoteTaskIDChanin the v2 variant is unguarded, so if server B is never reached the test hangs there with nothing to say. The compat variant already guards the same receive with its ownselectand a 1s bound, so this is an asymmetry rather than a decision.The call-site comments describe the deadline as "per-wait".
AwaitNarms one timer before the loop, so it is a budget for allnreceives together.Solution:
Add
testutil.AwaitValue, the one-value counterpart toAwaitN. A closed channel is a failure there rather than a receive, because the caller uses the value, and the failure names which value never arrived.Use it for the three ad-hoc receives across the two tests. That brings the v2 variant in line with compat, and replaces compat's ad-hoc 1s bound with the shared deadline. The 1s is reachable on a loaded machine, which is the same class of problem #1298 is about; by that point in the test
remoteCleanupCalledChanhas already delivered both values, so server B has been reached and the ID is already buffered.Same forced error path with this change:
The recorded error survives and the wait is named.
This does not fix the flake in #1298. I could not reproduce that on this machine, and I said so on the issue. What it changes is the failure report: a wait that does not complete names the event it was waiting for instead of panicking or hanging anonymously, which is step 1 of what @baptmont asked for there. The
AwaitNdeadline knob from step 2 is a separate change and is not in this PR.Testing Plan
Windows, Go 1.26.6,
-racevia mingw-w64 GCC 16.2.0.Unit Tests:
Targeted:
Full module:
None of the failures is caused by this change, and the set has been shrinking as unrelated
fixes land.
internaldropped out because #1394 merged.The two that remain are both Windows-only and I have now confirmed that rather than assumed
it: on Linux,
go test -race -shuffle=on -count=25under CPU load passes both.internal/configurable/conformance/replayplugin:filepath.Relcannot relate aC:tempdir to a clone on
D:. Needs the repo andTMPDIRon different volumes, so it is closerto a local-setup edge than a repo bug.
internal/telemetry/functionaltest:TestTelemetrySchema_Workflowspan ordering.A third,
internal/llminternalTestRunLiveNoGoroutineLeak/sender_error_after_connection_loss_does_not_leak, showed up inone full run and not the two since, with no change to that package, and passes
-count=3inisolation. Shuffle-order dependent rather than related to anything here.
I attributed these by re-running the same command on a clean
mainwith the change stashedand getting the identical set.
Other gates:
The "added or updated unit tests" box is ticked on the reading that this PR is a test change. No new test function asserts on
AwaitValueitself: the behaviour it fixes is a failure-reporting path, so asserting on it means asserting that at.Fatalffired, which needs a faketesting.T. The before/after output above is the evidence instead. Happy to add atestutiltest with a stub if you would rather have one.Manual End-to-End (E2E) Tests:
Not applicable, this is test-only and makes no production change. The before/after outputs above were produced by temporarily forcing the
SendStreamingMessageerror path in the v2 test, running it, and reverting; that edit is not part of the diff.Checklist
Additional context
AwaitValuereturning the zero value aftert.Fatalfis unreachable in practice, sinceFatalfcallsruntime.Goexit, but the compiler needs the return.