Skip to content

fix(go/ai/exp): suppress agent stream the moment a detach directive lands - #5881

Open
apascal07 wants to merge 1 commit into
mainfrom
ap/go-detach-stream-race
Open

fix(go/ai/exp): suppress agent stream the moment a detach directive lands#5881
apascal07 wants to merge 1 commit into
mainfrom
ap/go-detach-stream-race

Conversation

@apascal07

@apascal07 apascal07 commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

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 its customPatch, artifact, and turnEnd chunks on the wire before suppression engaged. Loaded CI runners hit that window. JS does not have this hole: its input pump flips isDetached before forwarding the input, and every emit site checks it.

Ports that shape to Go. The router gains a detached ingress latch (an atomic.Bool): the intake reader sets it the instant it reads the directive, before releasing the inputs riding it, and Responder.send/sendChunk skip 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 fnDone and detach arms can both be ready (select picks among ready arms at random), and an fnDone win would now resolve a detached invocation as a completed output whose stream had been silently muted. The fnDone arm re-checks the detach signal (detachCh became 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's Promise.race does.

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_SuppressesChunksFromTheDetachedTurn pins 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 -race on main and is 150/150 clean here. Go 1.26.3 in go/: the conformance, detach, intake, and stream-transform suites pass -race -count=10 and GOMAXPROCS=1 -count=5, and the full go test ./... passes all 40 packages.

…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.
@github-actions github-actions Bot added the go label Aug 1, 2026
@apascal07
apascal07 requested review from huangjeff5 and pavelgj August 1, 2026 00:44
@apascal07
apascal07 marked this pull request as ready for review August 1, 2026 00:44

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant