fix(review): bound TUISink.Wait, join the pump, surface drops (crew findings)

62abe4b→main·

peyton-alt·1w ago·2 files·+97 added/-3 removed

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

Sessions

01KX0JNDR51T3R0G488FSB63K7View transcript

Changes

2

• cmd/entire/cli/review

• Mtui_sink.go+32/-3

• Mtui_sink_test.go+65

8 unmodified lines

9
10
11
12
13
14
15
16
17
18
19
20
21
22
143 unmodified lines

166
167
168
167
168
169
170
171
172
173
174
175
176
177
178
179
1 unmodified line

181
182
183
176
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
111 unmodified lines

312
313
314
315
316
317
318
319
320
321
322
323

8 unmodified lines

import (
    "context"
    "io"
    "log/slog"
    "sync"
    "time"

tea "charm.land/bubbletea/v2"
    "golang.org/x/term"

"github.com/entireio/cli/cmd/entire/cli/logging"
    reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types"
)

143 unmodified lines

// Wait blocks until the Bubble Tea program exits. Safe to call after Start.
// If Start was never called, Wait returns immediately.
// Wait blocks until the Bubble Tea program exits, with a bounded escalation
// so teardown can never hang: in the normal flow PostRunComplete has already
// quit the program and Wait returns immediately; otherwise (early-error
// return paths, or a wedged loop that survived Kill) Wait gives the program
// one grace period, Kills it, gives it one more, and then abandons the
// goroutine — a stuck display must not hold command exit hostage. Joins the
// pump goroutine whenever the program actually exited. Safe to call after
// Start; if Start was never called, returns immediately.
func (s *TUISink) Wait() {
    s.mu.Lock()
    started := s.started

if !started {
        return
    }
    <-s.done
    select {
    case <-s.done:
        <-s.pumpDone
        return
    case <-time.After(tuiPostRunCompleteGrace):
    }
    s.program.Kill()
    select {
    case <-s.done:
        <-s.pumpDone
    case <-time.After(tuiPostRunCompleteGrace):
        // Bubble Tea never returned from Run despite Kill. Abandon the
        // program and pump goroutines rather than hanging teardown.
    }
}

// AgentEvent (Sink interface): translate ev into a tea.Msg and enqueue it for
111 unmodified lines

case <-time.After(tuiPostRunCompleteGrace):
        s.program.Kill()
    }

// Surface silent loss: a healthy run never drops. A non-zero count means
// the TUI loop stalled or lagged badly enough to jam the queue — exactly
// the diagnostic a future wedge investigation needs first.
if n := s.droppedCount(); n > 0 {
    logging.Debug(context.Background(), "tui sink dropped messages under backpressure",
        slog.Int("dropped", n))
}
}

Mcmd/entire/cli/review/tui_sink.go+32/-3

400 unmodified lines

401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468

400 unmodified lines

t.Fatal("RunFinished blocked past its bounded wait on a wedged TUI")
    }
}
// stubbornProgram is a teaRunner whose Run NEVER returns, even after Kill —
// modeling a Bubble Tea teardown stuck restoring a blocked terminal. Send
// unblocks on Kill so the pump can drain, but done never closes.
type stubbornProgram struct {
    killed chan struct{}
    block  chan struct{}
}

func newStubbornProgram() *stubbornProgram {
    return &stubbornProgram{killed: make(chan struct{}), block: make(chan struct{})}
}

func (p *stubbornProgram) Run() (tea.Model, error) {
    <-p.block       // never closed — Run never returns
    return nil, nil //nolint:nilnil // unreachable; mirrors tea.Program.Run's shape
}

func (p *stubbornProgram) Send(tea.Msg) { <-p.killed }

func (p *stubbornProgram) Kill() {
    select {
    case <-p.killed:
    default:
        close(p.killed)
    }
}

// TestTUISink_WaitIsBoundedWhenProgramNeverExits pins the teardown guarantee:
// `defer tuiSink.Wait()` must not hang the command forever when the Bubble
// Tea program never returns from Run, even after Kill. Wait escalates
// (grace → Kill → grace) and then abandons the goroutine.
func TestTUISink_WaitIsBoundedWhenProgramNeverExits(t *testing.T) {
    t.Parallel()
    prog := newStubbornProgram()
    sink := newTUISinkWithProgram(prog)
    sink.Start()

finished := make(chan struct{})
    go func() {
        sink.Wait()
        close(finished)
    }()
    select {
    case <-finished:
    case <-time.After(2*tuiPostRunCompleteGrace + 3*time.Second):
        t.Fatal("Wait hung on a program that never exits — teardown wedge")
    }
}

// TestTUISink_WaitJoinsPump pins that a normal Wait joins the pump goroutine
// (no leak between done closing and the pump observing it).
func TestTUISink_WaitJoinsPump(t *testing.T) {
    t.Parallel()
    prog := newRecordingProgram()
    sink := newTUISinkWithProgram(prog)
    sink.Start()
    prog.Kill()
    sink.Wait()
    select {
    case <-sink.pumpDone:
    case <-time.After(2 * time.Second):
        t.Fatal("Wait returned before the pump goroutine exited")
    }
}