Skip to content

Make /compact non-blocking with spinner feedback #1687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dgageot merged 1 commit into from
Feb 11, 2026
Merged

Make /compact non-blocking with spinner feedback #1687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions pkg/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,15 +717,25 @@ func (a *App) ShouldExitAfterFirstResponse() bool {
return a.exitAfterFirstResponse
}

func (a *App) CompactSession(additionalPrompt string) {
if a.session != nil {
func (a *App) CompactSession(ctx context.Context, additionalPrompt string) {
sess := a.session
if sess == nil {
return
}

go func() {
Copy link
Copy Markdown

@docker-agent docker-agent Bot Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CompactSession could accumulate orphaned goroutines
  • Other methods in this file (e.g., throttleEvents at line 850) use a proper select statement

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.

events := make(chan runtime.Event, 100)
a.runtime.Summarize(context.Background(), a.session, additionalPrompt, events)
close(events)
go func() {
defer close(events)
Copy link
Copy Markdown

@docker-agent docker-agent Bot Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

a.runtime.Summarize(ctx, sess, additionalPrompt, events)
}()
for event := range events {
a.events <- event
if ctx.Err() != nil {
return
}
a.sendEvent(ctx, event)
}
}
}()
}

func (a *App) PlainTextTranscript() string {
Expand Down
13 changes: 10 additions & 3 deletions pkg/tui/page/chat/chat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1155,11 +1155,18 @@ func (p *chatPage) processMessage(msg msgtypes.SendMsg) tea.Cmd {
// CompactSession generates a summary and compacts the session history
func (p *chatPage) CompactSession(additionalPrompt string) tea.Cmd {
// Cancel any active stream without showing cancellation message
p.cancelStream(false)
cancelCmd := p.cancelStream(false)

p.app.CompactSession(additionalPrompt)
var ctx context.Context
ctx, p.msgCancel = context.WithCancel(context.Background())
Copy link
Copy Markdown

@docker-agent docker-agent Bot Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context lifecycle management issue

The context created with context.Background() at line 1161 is isolated from the app lifecycle. This means:

  1. If the app is shutting down, this context won't be cancelled automatically
  2. If CompactSession is called multiple times before the first completes, p.msgCancel gets 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 of context.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.

p.app.CompactSession(ctx, additionalPrompt)

return p.messages.ScrollToBottom()
return tea.Batch(
cancelCmd,
p.setWorking(true),
p.setPendingResponse(true),
p.messages.ScrollToBottom(),
)
}

func (p *chatPage) Cleanup() {
Expand Down
7 changes: 6 additions & 1 deletion pkg/tui/page/chat/runtime_events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,12 @@ func (p *chatPage) handleRuntimeEvent(msg tea.Msg) (bool, tea.Cmd) {

case *runtime.SessionCompactionEvent:
if msg.Status == "completed" {
return true, notification.SuccessCmd("Session compacted successfully.")
return true, tea.Batch(
p.setWorking(false),
p.setPendingResponse(false),
notification.SuccessCmd("Session compacted successfully."),
p.messages.ScrollToBottom(),
)
}
return true, nil

Expand Down