Make /compact non-blocking with spinner feedback#1687
Conversation
There was a problem hiding this comment.
Review Summary
Found 3 potential issues related to goroutine management and context handling in the async compaction implementation:
- Goroutine leak risk: The outer goroutine in
CompactSessiondoesn't monitor context cancellation in its event loop - Session pointer race: The session pointer is accessed in a goroutine without synchronization
- Context lifecycle: The context created for compaction doesn't integrate with app lifecycle
The async design is sound, but these issues could lead to resource leaks if compaction is cancelled or called repeatedly.
| return | ||
| } | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
Goroutine leak: outer goroutine doesn't respect context cancellation
The outer goroutine launched at line 725 runs a for event := range events loop without checking ctx.Done(). If the context is cancelled while waiting for events, the goroutine will remain blocked until the inner goroutine closes the channel.
This is problematic because:
- If
Summarize()is slow or hangs, the outer goroutine leaks - Multiple calls to
CompactSessioncould accumulate orphaned goroutines - Other methods in this file (e.g.,
throttleEventsat line 850) use a properselectstatement
Suggested fix:
for {
select {
case event, ok := <-events:
if !ok { return }
a.sendEvent(ctx, event)
case <-ctx.Done():
return
}
}This pattern ensures the goroutine exits promptly when the context is cancelled.
| a.runtime.Summarize(context.Background(), a.session, additionalPrompt, events) | ||
| close(events) | ||
| go func() { | ||
| defer close(events) |
There was a problem hiding this comment.
Potential race condition: session pointer accessed without synchronization
a.session is captured and passed to a.runtime.Summarize() inside a goroutine without any synchronization. While the nil check at line 722 happens before launching the goroutine, concurrent calls to ReplaceSession() or NewSession() could replace a.session while the goroutine is still using it.
Although Go's GC keeps the old session alive while referenced, this creates a race where the goroutine operates on a stale session that's no longer considered "current" by the app.
Suggested fix:
sess := a.session
if sess == nil {
return
}
go func() {
events := make(chan runtime.Event, 100)
go func() {
defer close(events)
a.runtime.Summarize(ctx, sess, additionalPrompt, events)
}()
// ... rest of code
}()Capturing the session pointer in a local variable before the goroutine prevents the race.
|
|
||
| p.app.CompactSession(additionalPrompt) | ||
| var ctx context.Context | ||
| ctx, p.msgCancel = context.WithCancel(context.Background()) |
There was a problem hiding this comment.
Context lifecycle management issue
The context created with context.Background() at line 1161 is isolated from the app lifecycle. This means:
- If the app is shutting down, this context won't be cancelled automatically
- If
CompactSessionis called multiple times before the first completes,p.msgCancelgets overwritten, potentially orphaning the previous context
While this mirrors the pattern in processMessage, best practice would be to derive from a parent context representing the app lifecycle.
Consider:
- If the app has a root context, use
context.WithCancel(appCtx)instead ofcontext.Background() - Or ensure there's a mechanism to cancel all outstanding operations during shutdown
The immediate risk is moderate since compaction isn't called frequently, but it's worth addressing for proper resource management.
Run session compaction asynchronously in a background goroutine instead of blocking the TUI render thread. Show the working spinner and pending response indicator while the LLM generates the summary, and clear them when the compaction completes. The compaction context is cancellable via Esc, matching the behavior of regular agent streams. Fixes docker#1678 Assisted-By: cagent
Run session compaction asynchronously in a background goroutine instead of blocking the TUI render thread. Show the working spinner and pending response indicator while the LLM generates the summary, and clear them when the compaction completes. The compaction context is cancellable via Esc, matching the behavior of regular agent streams.
Fixes #1678
Assisted-By: cagent