fix: drain runtime streams before releasing turn ownership - #4353
Merged
Merged
Conversation
dgageot
enabled auto-merge
September 18, 2026 12:03
dgageot
force-pushed
the
fix/drain-runtime-streams-pr
branch
from
September 18, 2026 12:33
246d424 to
b75129f
Compare
trungutt
previously approved these changes
Sep 18, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Sep 18, 2026
Collaborator
|
👋 This PR has merge conflicts with the base branch. Please rebase or merge the latest base branch and resolve them. I've moved it to draft and added |
Assisted-By: Claude Signed-off-by: David Gageot <david.gageot@docker.com>
Cancel abandoned runs and drain their event channels before returning. Keep API streaming locks and ACP turn tokens held until teardown finishes; apply the same lifecycle contract to A2A, CLI, and synchronous runtime wrappers. Regression tests cover cancellation, handler errors, and follow-up turns. Signed-off-by: David Gageot <david.gageot@docker.com> Assisted-By: Claude (Anthropic)
dgageot
force-pushed
the
fix/drain-runtime-streams-pr
branch
from
September 18, 2026 13:21
b75129f to
db5746f
Compare
trungutt
approved these changes
Sep 18, 2026
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.
A code audit identified that several call sites consumed a
RunStreampartially and then returned, leaving the stream abandoned. This matters because the server's streaming lock and the ACP turn token are held untilRunSessionandrunAgentreturn — if the caller exits before the stream is fully drained, the lock or token is released while the producer goroutine may still be running, breaking the intended ordering guarantee.The fix cancels the child context to signal the producer to stop, then drains and discards any remaining items from the stream before returning. This preserves the normal close-then-unlock sequence: the streaming lock in
session_manager.goand the ACP turn token inagent.goare only released after the stream has actually closed. The per-turn CLI child context is kept alive until the stream is drained so that follow-up prompts remain possible during teardown. In the A2A adapter, an early consumer exit now also stops the detached producer goroutine rather than letting it run to completion unobserved.RunStreamownership semantics are documented on the type itself. A new lint cop (drain_run_stream_before_release) detectsreturn,break, andgotothat abandon a locally consumed stream; it was deliberately run against the unfixed codebase first and found exactly seven violations acrosspkg/server,pkg/acp,pkg/a2a,pkg/cli, andpkg/runtime. After the fixes it reports zero. The cop uses real package types with a localCFG, handles comma-ok closure patterns, and supports deferred empty-range drains; it does not attempt to prove cancellation ordering or track cross-function transfers — the regression tests cover those behavioral properties instead.Regression tests use
synctestand channel-driven synchronization to give deterministic before/after coverage: they prove the pre-fix code fails (stream not drained, lock released early) and the fixed code passes, including tails that exceed channel capacity, handler errors, iterator exit, and CLI follow-up sequences.golang.org/x/toolsis promoted from indirect to direct ingo.modto support the cop; no version was bumped.