Skip to content

fix(review): TUI sink must never backpressure the orchestrator - #1677

Merged
peyton-alt merged 5 commits into
mainfrom
feat/review-nonblocking-tui-sink
Jul 14, 2026
Merged

fix(review): TUI sink must never backpressure the orchestrator#1677
peyton-alt merged 5 commits into
mainfrom
feat/review-nonblocking-tui-sink

Conversation

@peyton-alt

@peyton-alt peyton-alt commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

https://entire.io/gh/entireio/cli/trails/797

During a live full-crew dogfood run (2026-07-07), the parent entire review process wedged mid-run: the TUI stopped rendering (elapsed frozen at 11m58s, spinner ticks dead), an armed --timeout 20m never surfaced, no judge ran, and the process had to be killed externally 15 minutes later.

Mechanism (proven)

Program.Send is an unbuffered blocking send, and TUISink called it directly from the orchestrator's serial dispatch goroutine. Any stall in the Bubble Tea Update/render pipeline therefore freezes, in order: sink dispatch → the bounded fanIn drain loop → the per-agent forwarding goroutines → the stdout parsers → reviewer-timeout event handling. Ticks are self-re-arming and commands run in independent goroutines, so a frozen elapsed counter is itself proof the Update loop was blocked. (The stall's own trigger was not reproduced across three targeted shim experiments — single-agent linger, multi-agent linger + burst, pre-fix tailer backlog — all of which enforced timeouts exactly; this PR closes the amplification path that turns any display stall into a full orchestrator freeze.)

Change

  • TUISink gains a bounded internal queue (4096) drained by a pump goroutine — the only goroutine allowed to block on Send.
  • AgentEvent never blocks: overflow is dropped and counted (droppedCount), zero drops in any healthy run.
  • Control messages (RunFinished, FinalPhase*, PostRunComplete) use a bounded wait; PostRunComplete keeps its existing Kill fallback. A wedged TUI now degrades to lost frames and a stale footer instead of a hung run with an unenforced timeout.
  • FIFO order preserved through the single queue; teaRunner interface extracted so tests can inject a deterministically wedged program.

Verification

  • New tests: AgentEvent completes 3× the queue cap against a wedged program without blocking; control messages return within their bounded wait; a recording runner pins FIFO delivery. All -race clean.
  • Full unit suite green (7,693), lint clean.

Investigation record and incident evidence: session notes from the #1666 live dogfood runs.

🤖 Generated with Claude Code


Note

Medium Risk
Changes the interactive review TUI event path and drops display events under extreme backlog; orchestration and timeouts should keep working but the UI may lag or miss frames when wedged.

Overview
Prevents a stalled Bubble Tea TUI from freezing the entire review run by stopping direct blocking Program.Send calls from the orchestrator's serial dispatch path.

TUISink now enqueues tea.Msg on a 4096-cap channel drained by a dedicated pump goroutine (the only place that may block on Send). AgentEvent uses a non-blocking enqueue and drops + counts overflow via droppedCount instead of backpressuring fan-in, parsers, or timeout handling. Control messages (RunFinished, final-phase updates, PostRunComplete) go through enqueueControl with a bounded wait; PostRunComplete keeps its Kill fallback without the extra goroutine around Send.

A teaRunner interface and newTUISinkWithProgram enable tests with wedged/recording fakes. New tests assert non-blocking AgentEvent under a wedged loop, FIFO delivery when healthy, and bounded RunFinished when the queue is jammed.

Reviewed by Cursor Bugbot for commit a91d234. Configure here.

During a live full-crew dogfood run (2026-07-07, run 6), the parent
entire process wedged mid-run: the TUI stopped rendering (elapsed froze
at 11m58s, spinner ticks dead), the armed --timeout 20m never surfaced,
no judge ran, and the process had to be killed externally 15 minutes
later. The mechanism is structural: Program.Send is an unbuffered
BLOCKING send, and TUISink called it directly from the orchestrator's
serial dispatch goroutine — so any stall in the Bubble Tea Update/render
pipeline freezes sink dispatch, the bounded fanIn drain loop, the
forwarding goroutines, the stdout parsers, and reviewer-timeout event
handling with it. (The stall's own trigger was not reproduced in three
targeted shim experiments; this closes the amplification path that
turned a display stall into a full orchestrator freeze.)

TUISink now enqueues onto a bounded internal queue drained by a pump
goroutine — the only goroutine allowed to block on Send. Display events
(AgentEvent) never block: overflow beyond the 4096-message cap is
dropped and counted. Rare control messages (run summary, phase
transitions, quit) use a bounded wait, and PostRunComplete keeps its
Kill fallback, so a wedged TUI degrades to lost frames and a stale
footer instead of a hung run. Order is preserved through the single
queue for a healthy program.

Tests inject a deterministically wedged teaRunner: AgentEvent must
complete 3x the queue cap without blocking, control messages must
return within their bounded wait, and a recording runner pins FIFO
delivery.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 01KX0HNDFDWYN2F02BKXA5AJ6A
@peyton-alt
peyton-alt requested a review from a team as a code owner July 8, 2026 09:42
Copilot AI review requested due to automatic review settings July 8, 2026 09:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens entire review’s interactive Bubble Tea TUI sink so UI stalls cannot backpressure (and thereby wedge) the orchestrator event pipeline, preserving timeout/judge execution even if the TUI freezes.

Changes:

  • Introduces a bounded internal message queue in TUISink, with a dedicated pump goroutine as the only caller allowed to block on tea.Program.Send.
  • Makes AgentEvent non-blocking by dropping overflow events and tracking a dropped-message count.
  • Adds tests covering non-blocking behavior against a deterministically wedged program, FIFO delivery for a healthy program, and bounded behavior for control messages.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cmd/entire/cli/review/tui_sink.go Adds bounded queue + pump goroutine to prevent TUI backpressure from freezing orchestration; introduces control-message enqueue with bounded wait.
cmd/entire/cli/review/tui_sink_test.go Adds wedge-hardening tests using injected fake teaRunner implementations (wedged + recording).
Comments suppressed due to low confidence (1)

cmd/entire/cli/review/tui_sink.go:269

  • The PostRunComplete doc comment says it "waits for the Bubble Tea program to finish", but the implementation is a bounded best-effort wait: after two grace windows it returns even if s.done is still not closed. This mismatch can confuse callers/reviewers about the shutdown guarantees.

Consider updating the comment to reflect the bounded wait + Kill fallback behavior.

// PostRunComplete exits the TUI and waits for the Bubble Tea program to finish.
// Call after post-run sinks have produced any buffered output.
func (s *TUISink) PostRunComplete() {

Comment thread cmd/entire/cli/review/tui_sink.go
peyton-alt and others added 4 commits July 8, 2026 11:59
…indings)

The live full-crew review of this branch (approve-path run, 3/3
reviewers succeeded through the hardened sink) returned three findings:

- Wait() blocked unconditionally on done, so the deferred teardown wait
  could still hang forever if Bubble Tea never returns from Run even
  after Kill — the wedge class survived at command exit. Wait now
  escalates: one grace period, Kill, one more grace, then abandons the
  goroutine. This also fixes the pre-existing early-error-path hang
  where nothing ever quit a healthy TUI before the deferred Wait.
- pumpDone was created and closed but never joined; Wait now joins the
  pump whenever the program actually exited.
- The dropped counter was invisible in production; PostRunComplete now
  debug-logs a non-zero count — the first diagnostic a future wedge
  investigation needs.

Pinned by a stubbornProgram fake whose Run never returns even after
Kill: Wait must return within the bounded escalation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 01KX0JNDR51T3R0G488FSB63K7
Review asked whether the pump can Send after the Bubble Tea program
exits (select picking a queued msg after done closes). It can — and
it's a no-op by construction: tea.Program.Send is a context-guarded
select and the msgs channel is never closed, so a post-exit Send
returns immediately rather than panicking or blocking. Recorded at the
pump so the next reader doesn't re-derive it from the tea source.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 01KX0Z9J512Y4QA9MHEGY042RT
@peyton-alt
peyton-alt merged commit 372bc14 into main Jul 14, 2026
10 checks passed
@peyton-alt
peyton-alt deleted the feat/review-nonblocking-tui-sink branch July 14, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants