Skip to content

fix: progressive ask can deadlock when the UI/output consumer exits early#793

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

fix: progressive ask can deadlock when the UI/output consumer exits early#793
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-c1e4d11a

Conversation

@sam-saffron-jarvis

Copy link
Copy Markdown
Contributor

What changed

  • Made askProgressiveBridge cancellation-aware by adding a stop signal and routing all event delivery through a helper that stops blocking when the consumer has exited.
  • Updated progressive ask orchestration in cmd/ask.go to call bridge.Stop() before waiting on the run goroutine if JSON streaming or the renderer returns a write/render error.
  • Added a regression test that simulates a JSON writer failure and verifies the producer side exits instead of blocking behind a full bridge buffer.

Why this is high-value

Progressive ask is one of the main streaming paths in term-llm, including JSON output and rich rendering. Before this change, if the output consumer returned early (for example due to a broken pipe or renderer failure), the producer goroutine could keep sending into the bridge until the buffer filled and then block forever. The caller then waited on <-runCh, so the whole command could hang even though stdout/the renderer was already gone.

This fix preserves normal behavior but makes the bridge shut down cleanly in the failure path, preventing a user-visible deadlock in automation and piped usage.

Validation

  • gofmt -w cmd/ask_progressive.go cmd/ask.go cmd/ask_progressive_test.go
  • go build ./...
  • go test ./...
  • Added TestAskProgressiveBridge_StopUnblocksProducerAfterConsumerWriteError

@SamSaffron

Copy link
Copy Markdown
Owner

Closing; fix will be applied directly to main.

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