fix(go/ai/exp): suppress agent stream the moment a detach directive lands - #5881
Open
apascal07 wants to merge 1 commit into
Open
fix(go/ai/exp): suppress agent stream the moment a detach directive lands#5881apascal07 wants to merge 1 commit into
apascal07 wants to merge 1 commit into
Conversation
…ands A detached run must stream no chunks, but wire suppression lived in the runtime's detach handler, one goroutine hop and one store write after the intake released the detach's inputs to the runner. A fast background turn could emit customPatch/artifact/turnEnd chunks before the handler got there, which is the conformance flake on loaded CI runners. The JS runtime does not have this hole: its input pump flips isDetached before forwarding the input. Mirror that shape: the router gains a detached ingress latch (atomic.Bool), the intake reader sets it the instant it reads the directive (before releasing the inputs riding it), and Responder.send and sendChunk skip the wire forward once it is set, so a detached run's chunks never enter the pipe. Side effects still apply at Send time. The latch store happens-before the input handoff, so every chunk of the detached turn observes it. The run select also re-checks the detach signal when fn completion wins the race (select picks ready arms at random), so a detach that raced a fast turn still resolves as detached instead of as a completed output with a silently muted stream.
Contributor
There was a problem hiding this comment.
Code Review
This pull request ensures that a detached agent run suppresses streaming chunks to the client immediately upon receiving a detach directive, preventing chunks from leaking onto the wire. This is achieved by introducing an ingress latch on the chunk router and responder, triggered immediately by the intake reader when a detach directive is observed. Additionally, the runtime's main loop now explicitly prioritizes the detach signal over function completion to resolve race conditions, and capability checks have been consolidated into handleDetach. A new test has been added to verify this behavior. There are no review comments, so I have no feedback to provide.
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.
Fixes the flaky conformance case
detached run emits no customPatch chunks(failing run). Wire suppression for a detached run lived in the runtime's detach handler, one goroutine hop and one store write after the intake had already released the detach's inputs to the runner, so a fast background turn could put itscustomPatch, artifact, andturnEndchunks on the wire before suppression engaged. Loaded CI runners hit that window. JS does not have this hole: its input pump flipsisDetachedbefore forwarding the input, and every emit site checks it.Ports that shape to Go. The router gains a
detachedingress latch (anatomic.Bool): the intake reader sets it the instant it reads the directive, before releasing the inputs riding it, andResponder.send/sendChunkskip the wire forward once it is set, so a detached run's chunks never enter the pipe. In-process side effects (artifacts, custom-state mutations) still apply at Send time. The latch store happens-before the input handoff to the runner, which is what makes every chunk of the detached turn observe it; only the reader sits ahead of that handoff, which is why the latch cannot live in the detach handler. The router's stop/drain machinery is untouched and keeps owning shutdown safety.Moving suppression earlier exposed a second race: the run select's
fnDoneand detach arms can both be ready (select picks among ready arms at random), and anfnDonewin would now resolve a detached invocation as a completed output whose stream had been silently muted. ThefnDonearm re-checks the detach signal (detachChbecame a closed latch, so observers do not consume it) and re-buffers the settled result for the finalizer, settling the race at directive-read time the way JS'sPromise.racedoes.Two deliberate edges: a rejected detach (store without detach support) now also mutes the stream from directive read, since the invocation resolves as failed either way (JS leaves the stream running there), and the symmetric disconnect-vs-detach coin flip on the
clientCtx.Done()arm predates this change and is left alone, since a disconnect has no wire to protect.Verification
TestAgent_Detach_SuppressesChunksFromTheDetachedTurnpins the leak deterministically: a gated store parks the detach path at its first write while the turn emits, exactly the interleaving that leaked; it fails 10/10 with the reader-side latch removed. The previously flaky conformance case reproduced at ~2/150 under-raceon main and is 150/150 clean here. Go 1.26.3 ingo/: the conformance, detach, intake, and stream-transform suites pass-race -count=10andGOMAXPROCS=1 -count=5, and the fullgo test ./...passes all 40 packages.