Skip to content

fix: drain runtime streams before releasing turn ownership - #4353

Merged
dgageot merged 2 commits into
mainfrom
fix/drain-runtime-streams-pr
Sep 18, 2026
Merged

dgageot merged 2 commits into
mainfrom
fix/drain-runtime-streams-pr

Conversation

@dgageot

@dgageot dgageot commented Sep 17, 2026

Copy link
Copy Markdown
Member

A code audit identified that several call sites consumed a RunStream partially and then returned, leaving the stream abandoned. This matters because the server's streaming lock and the ACP turn token are held until RunSession and runAgent return — 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.go and the ACP turn token in agent.go are 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.

RunStream ownership semantics are documented on the type itself. A new lint cop (drain_run_stream_before_release) detects return, break, and goto that abandon a locally consumed stream; it was deliberately run against the unfixed codebase first and found exactly seven violations across pkg/server, pkg/acp, pkg/a2a, pkg/cli, and pkg/runtime. After the fixes it reports zero. The cop uses real package types with a local CFG, 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 synctest and 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/tools is promoted from indirect to direct in go.mod to support the cop; no version was bumped.

@dgageot
dgageot requested a review from a team as a code owner September 17, 2026 18:31
@aheritier aheritier added area/core Core agent runtime, session management area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection kind/fix PR fixes a bug (maps to fix:). Use on PRs only. labels Sep 17, 2026
@dgageot
dgageot enabled auto-merge September 18, 2026 12:03
@dgageot
dgageot force-pushed the fix/drain-runtime-streams-pr branch from 246d424 to b75129f Compare September 18, 2026 12:33
trungutt
trungutt previously approved these changes Sep 18, 2026
@dgageot
dgageot added this pull request to the merge queue Sep 18, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Sep 18, 2026
@aheritier aheritier added the status/needs-rebase PR has merge conflicts or is out of date with main label Sep 18, 2026
@aheritier

Copy link
Copy Markdown
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 status/needs-rebase; it'll be picked back up automatically once the conflicts are cleared.

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
dgageot force-pushed the fix/drain-runtime-streams-pr branch from b75129f to db5746f Compare September 18, 2026 13:21
@dgageot dgageot removed the status/needs-rebase PR has merge conflicts or is out of date with main label Sep 18, 2026
@dgageot
dgageot added this pull request to the merge queue Sep 18, 2026
Merged via the queue into main with commit 6a30656 Sep 18, 2026
15 checks passed
@dgageot
dgageot deleted the fix/drain-runtime-streams-pr branch September 18, 2026 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core Core agent runtime, session management area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection kind/fix PR fixes a bug (maps to fix:). Use on PRs only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants