fix: don't clobber attached runtime's cancel in RunSession/recallSession#3525
Merged
Conversation
…ssion/recallSession Commit 064a464 made both RunSession and recallSession unconditionally overwrite the cancel stored on an attached runtime with the per-stream cancel, so DeleteSession was aborting the in-flight stream instead of cancelling the attach context. That broke the graceful-delete contract (DELETE ?wait=true must block until the stream ends naturally) and made TestAttachedServer_DeleteWithWaitBlocksUntilStreamStops flaky under -race. Guard both assignments with a done==nil check so only server-owned runtimes track the per-stream cancel. Assisted-By: Claude
aheritier
added
area/sessions
Sayt-0
approved these changes
Jul 8, 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
3 participants
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.
go test -race ./pkg/server/madeTestAttachedServer_DeleteWithWaitBlocksUntilStreamStopsfail roughly 25% of runs with "DELETE returned before the stream goroutine exited". The race was introduced by commit 064a464 ("fix: wake idle sessions on tool recall"), which unconditionally overwroteruntimeSession.cancelinSessionManager.RunSessionandrt.cancelinrecallSessionwith the per-stream cancel.DeleteSessionthen aborted the in-flight stream instead of cancelling the attach context, soWaitStoppedreturned as soon as the aborted stream unwound — breaking the graceful-delete contract established in e73a2c0 (DELETE /api/sessions/:id?wait=truemust block until the stream ends naturally).The fix guards both cancel assignments with
done == nil. Thedonechannel is only set byAttachRuntime, so server-owned sessions continue to get the abort-on-delete behaviour from 064a464, while attached (TUI-owned) sessions keep their attach-lifetime cancel intact. The previously flaky test now passes 50/50 under-race, and a fullgo test -race ./...reports zero data races.