Skip to content

fix(tui): restore terminal on Ctrl-C when bubbletea shutdown stalls#2842

Merged
dgageot merged 4 commits into
docker:mainfrom
dgageot:board/99354826b22eb556
May 21, 2026
Merged

fix(tui): restore terminal on Ctrl-C when bubbletea shutdown stalls#2842
dgageot merged 4 commits into
docker:mainfrom
dgageot:board/99354826b22eb556

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented May 20, 2026

Problem

When the user hits Ctrl-C in the TUI and confirms the exit, the terminal is sometimes left in a broken state — raw mode still on, alt-screen still active, mouse tracking on, cursor hidden, etc.

Root cause

The shutdown safety net in appModel.cleanupAll was implemented as:

go func() {
    time.Sleep(shutdownTimeout)
    exitFunc(0)
}()

It exists because bubbletea's renderer can deadlock during shutdown when stdout is wedged (the final flush after tea.Quit re-acquires the renderer mutex still held by a blocked previous flush — TestExitDeadlock_BlockedStdout proves it). But:

  • It always sleeps the full timeout, even on a clean shutdown — leaks a goroutine until the process exits.
  • os.Exit(0) runs no defers, so the terminal stays in raw mode + alt-screen + mouse tracking + hidden cursor + kitty keyboard flags + …

Fix

Race program.Wait() against the deadline so a clean shutdown skips the force-exit path entirely:

select {
case <-done: // Wait() returned, bubbletea cleaned up normally
case <-time.After(timeout):
    go func() { _ = program.ReleaseTerminal() }()
    exit(0)
}

Program.ReleaseTerminal() is bubbletea's own terminal-restore primitive (out of raw mode, alt-screen exit, cursor on, mouse off, etc.) and is independent from the regular shutdown path. It can itself deadlock on the wedged renderer mutex, so it's run in a fire-and-forget goroutine — best-effort terminal restore, then exit no matter what.

Side fix: shutdownTimeout and exitFunc are package globals that tests mutate; they're now snapshotted synchronously inside cleanupAll so the safety-net goroutine can't race with t.Cleanup restoring the originals.

Tests

New tests in pkg/tui/tui_exit_test.go, all passing under -race:

  • TestCleanupAll_SpawnsSafetyNet — stuck program → exitFunc fires.
  • TestCleanupAll_GracefulShutdownSkipsExit — clean shutdown → exitFunc does NOT fire.
  • TestCleanupAll_NilProgramIsSafe — no program → no safety-net spawned.
  • TestCleanupAll_WedgedStdoutFiresExit — realistic regression test: bubbletea is stuck on a wedged stdout, Wait() never returns, ReleaseTerminal() would itself deadlock — exitFunc must still fire. Verified to fail under the previous inline ReleaseTerminal call.

Validation

  • task lint
  • task test
  • go test ./pkg/tui -race

@dgageot dgageot requested a review from a team as a code owner May 20, 2026 15:15
docker-agent

This comment was marked as outdated.

docker-agent

This comment was marked as outdated.

dgageot added 4 commits May 20, 2026 18:46
…out/exitFunc

This fixes two issues found during code review:

1. Safety-net deadlock: program.ReleaseTerminal() was called inline in the
   timeout branch but acquires the renderer mutex held by a wedged
   stdout-write goroutine. Now calls ReleaseTerminal in its own goroutine
   without waiting, allowing the safety net to proceed and force-exit.
   Adds TestCleanupAll_WedgedStdoutFiresExit regression test.

2. Data-race in package globals: shutdownTimeout and exitFunc were read
   inside the safety-net goroutine while tests mutate/restore them,
   causing a data race. Now snapshots them into locals synchronously
   in cleanupAll before spawning the safety-net goroutine.
   Updates TestCleanupAll_GracefulShutdownSkipsExit to use p.Send(syncMsg{})
   instead of time.Sleep for proper synchronization.
cleanupAll is invoked from several message handlers (ExitSessionMsg,
ExitConfirmedMsg, ExitAfterFirstResponseMsg) and could be called more
than once on the same model. Each call snapshotted exitFunc and armed
its own deadline, so multiple safety nets could race to call exit(0)
- harmless in production where exit is os.Exit, fatal in tests where
exit is a channel close.

Nil out m.program after capturing it so subsequent calls are no-ops.

Also send triggerQuitMsg in TestCleanupAll_WedgedStdoutFiresExit so it
actually drives the renderer into the final-flush deadlock path it
claims to test, and add TestCleanupAll_MultipleCallsFireExitOnce as a
regression test for the multi-call guard.
@dgageot dgageot force-pushed the board/99354826b22eb556 branch from 31e3d56 to a184f1e Compare May 20, 2026 16:51
@dgageot dgageot merged commit 32ceee5 into docker:main May 21, 2026
5 checks passed
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.

3 participants