Skip to content

fix: non-progressive ask leaks the provider stream when the output consumer dies early#832

Closed
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-0e3f7b3f
Closed

fix: non-progressive ask leaks the provider stream when the output consumer dies early#832
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-0e3f7b3f

Conversation

@sam-saffron-jarvis

Copy link
Copy Markdown
Contributor

What changed

  • ran the non-progressive ask streaming path under a child context.WithCancel
  • passed that child context to engine.Stream, adapter.ProcessStream, and the foreground stream consumers
  • on consumer-side failures (emitSessionStarted, JSON write failures, renderer failures, plain-text write failures), cancel the child context before returning and wait for the producer goroutine to finish
  • added a regression test that simulates a consumer write failure and verifies cancellation unblocks the producer and reaches stream.Close()

Why this is high-value

This fixes a real hot-path reliability leak in ask streaming. Before this change, if the output consumer died early (for example ask --json | head, a broken pipe, or a renderer failure), runAsk could return immediately while the provider stream goroutine stayed blocked behind the undrained event pipeline. That could leave an in-flight LLM request open until process exit, wasting provider time/tokens and sometimes hanging shell pipelines longer than necessary.

With the new cancellation/join behavior, consumer failures now actively tear down the provider stream and wait for shutdown to complete deterministically.

Validation

  • gofmt -w cmd/ask.go cmd/ask_test.go
  • go build ./...
  • go test ./...
  • added TestNonProgressiveConsumerWriteErrorCancellationUnblocksProducer
  • git diff --check

@SamSaffron

Copy link
Copy Markdown
Owner

Closing as requested: the high-confidence fixes have been applied or superseded in the current working tree.

@SamSaffron SamSaffron closed this Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants