fix(server): prevent send-on-closed-channel panic in session title generation - #3832
fix(server): prevent send-on-closed-channel panic in session title generation#3832Piyush0049 wants to merge 4 commits into
Conversation
9065123 to
89df77d
Compare
|
Same here @Piyush0049 Please provide more details about the issue you are trying to solve. Is it something which happens often? What are the symptoms? Right now the PR doesn't provide enough context to understand if you are fixing a bug or if these are just code reviews by an LLM which could easily hallucinate bugs. Thanks |
|
@aheritier This fixes a runtime panic in Root Cause
FixI synchronized the background All unit tests in pkg/server pass cleanly This guarantees the channel isn't closed while it's still being written to, preventing the panic. |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The concurrency fix is correctly implemented. The sync.WaitGroup coordinates the async generateTitle goroutine with the close(streamChan) defer, eliminating the send-on-closed-channel panic. Key points verified:
wg.Go()availability: Go 1.26.5 (per go.mod) includessync.WaitGroup.Go(introduced in Go 1.25) — compiles cleanly.- Defer ordering:
wg.Wait()is called inline (not deferred), so it blocks the goroutine before any of the LIFO defers (streaming.Unlock,cancel,close(streamChan)) fire — correct. return→break: The change correctly allows execution to fall through towg.Wait()on context cancellation, preserving the early-exit semantics while ensuring the WaitGroup is honoured.- All return paths covered: Whether the stream completes normally or context is cancelled,
wg.Wait()is reached before the deferredclose(streamChan). - One drafter hypothesis (stream producer goroutine-leak on
break) was investigated and dismissed: thebreakhas identical effect to the previousreturn— both abandon the range loop at the same point — so no new risk is introduced.
Sayt-0
left a comment
There was a problem hiding this comment.
The fix addresses a real panic vector, confirmed by tracing the contexts involved:
| Path | Behaviour on main |
|---|---|
| Run finishes before title generation | close(streamChan) runs; generateTitle receives the request ctx (not streamCtx) and the deferred cancel() only cancels streamCtx, so the ctx.Done() fallback in the emit select never fires and the send hits a closed channel (deterministic panic) |
| Client disconnect mid-run | request ctx cancelled, both select cases ready, the send on the closed channel can still be chosen (probabilistic panic) |
The WaitGroup coordination itself is correct:
wg.Wait()runs inline, before the LIFO defers (Unlock,cancel,close(streamChan)).- The
returntobreakchange makeswg.Wait()reachable on cancellation. - The SSE consumer (
runAgent) keeps draining until the channel closes, so no deadlock is introduced. - Side benefit: on normal completion the
SessionTitleevent is now guaranteed to be delivered instead of being lost or panicking.
Two changes requested before merge:
- A regression test reproducing the panic (details inline at
wg.Wait()). - A short note on the new teardown trade-off in the
DeleteSessionpath (details inline atwg.Go).
Description
This PR fixes a critical runtime concurrency bug in
SessionManager.RunSessionwhere asynchronous session title generation could attempt to send events on a closed channel, causing the server process to crash.Root Cause
When a session turn required title generation,
sm.generateTitlewas spawned asynchronously as a background goroutine writing title events tostreamChan. If the conversation stream (RunStream) terminated early due to cancellation, completion, or error, the function returned immediately and triggereddefer close(streamChan). When the background title goroutine subsequently attempted to emitruntime.SessionTitletostreamChan, sending on the closed channel caused a runtime panic.Key Changes
sync.WaitGrouparound the asynchronousgenerateTitlegoroutine and placedwg.Wait()before the return paths inRunSession. This guarantees thatclose(streamChan)cannot execute until title generation completes or safely aborts on context cancellation.returnwithbreakinside theRunStreamconsumer loop when encountering a context error. This ensures execution always falls through towg.Wait(), guaranteeing background synchronization even during early client disconnects or stream aborts.Verification
task lintandgolangci-lint runcleanly with zero issues reported.generateTitleunblocks immediately viaselectonctx.Done(), preventing hangs or deadlocks.