fix(bonfire): kill a live view on a signal only after it has started (IN-12, quick-261028-rqd)

modules/bonfire/README.md

@@ -27,7 +27,7 @@ Use cases:
 - Usage strings and argument-count checks derived from the declared arguments (`<name>` for required, `[name]` for optional).
 - Scalar flags, bare flags (`bonfire.Flag.Bare`, so `--force` alone stores `true`, read as a switch with `bonfire.FlagBool`, which also accepts `--force=1` and `--force=false`) and ordered repeatable flags (`bonfire.Flag.Repeatable`, read back through `bonfire.Input.Flags`).
 - Styled status lines: `bonfire.Output.Info`, `bonfire.Output.Success`, `bonfire.Output.Warning` and `bonfire.Output.Error` (the last one writes to the error stream).
-- Widgets on the charm stack, ported from SSU's terminal UI: box-drawn tables (`bonfire.Output.Table`, lipgloss), a spinner around a function (`bonfire.Output.Spinner`), a gradient progress bar (`bonfire.Output.Progress`, whose `bonfire.Progress` also implements `bonfire.ItemProgress`) and a multi-row process view for bulk loops (`bonfire.Process`), each a bubbletea program that keeps stdin and signals to the command: Ctrl+C or SIGTERM during a view restores the cursor and the terminal, then stops the command as before (or reaches its own signal handler). Log lines, and output the command prints through the console that started the view (and its `bonfire.Output.Error` lines when stderr is a terminal too), appear above a running view (`bonfire.PrintAbove`); other consoles (a `bonfire.Call` buffer, a console on a file) keep writing to their own writers. When the context given to `bonfire.Process` is cancelled, the view ends with its last frame and later lines print plainly below it. Table cells, the item names and errors of `bonfire.Process` (in the process view and in its plain lines), the process view's operation label, and failure lines are stripped of control characters (`bonfire.Sanitize`).
+- Widgets on the charm stack, ported from SSU's terminal UI: box-drawn tables (`bonfire.Output.Table`, lipgloss), a spinner around a function (`bonfire.Output.Spinner`), a gradient progress bar (`bonfire.Output.Progress`, whose `bonfire.Progress` also implements `bonfire.ItemProgress`) and a multi-row process view for bulk loops (`bonfire.Process`), each a bubbletea program that keeps stdin and signals to the command: Ctrl+C or SIGTERM during a view restores the cursor and the terminal (also when it arrives while the view is still starting), then stops the command as before (or reaches its own signal handler). On Windows, where a process cannot signal itself, the view exits with 128 plus the signal number after restoring the terminal, so a command's own handler may not run. Log lines, and output the command prints through the console that started the view (and its `bonfire.Output.Error` lines when stderr is a terminal too), appear above a running view (`bonfire.PrintAbove`); other consoles (a `bonfire.Call` buffer, a console on a file) keep writing to their own writers. When the context given to `bonfire.Process` is cancelled, the view ends with its last frame and later lines print plainly below it. Table cells, the item names and errors of `bonfire.Process` (in the process view and in its plain lines), the process view's operation label, and failure lines are stripped of control characters (`bonfire.Sanitize`).
 - Prompts: `bonfire.Output.Ask`, `bonfire.Output.Confirm` and `bonfire.Output.Secret` use huh fields on an interactive terminal, and `bonfire.Output.Choice` an arrow-key selector with a confirmation screen. Off a terminal they read lines from stdin as before (a piped password still works), return their defaults when input ends, and `bonfire.Output.Confirm` returns its default without asking. A cancelled prompt returns `bonfire.ErrAborted`. `bonfire.ConfirmProceed` asks before a destructive step only on an interactive terminal.
 - Degradation: every widget follows the terminal and the global options, and the `bonfire.Output` interface is unchanged for plugins:
 

modules/bonfire/live.go

@@ -366,12 +366,13 @@ func runLive(ctx context.Context, out io.Writer, c *console, model tea.Model, wo
 	// builds none, so it registers no child context on ctx and opens no
 	// TEA_TRACE file. NewProgram takes no bonfire lock and prints nothing.
 	finished := make(chan struct{})
+	ready := make(chan struct{})
 	liveMu.Lock()
 	if activeView != nil {
 		liveMu.Unlock()
 		return false, nil
 	}
-	prog := tea.NewProgram(model,
+	prog := tea.NewProgram(startedModel{Model: model, once: &sync.Once{}, ready: ready},
 		tea.WithInput(nil),
 		tea.WithOutput(out),
 		tea.WithoutSignalHandler(),
@@ -386,7 +387,7 @@ func runLive(ctx context.Context, out io.Writer, c *console, model tea.Model, wo
 		_, _ = prog.Run()
 	}()
 	stopWatch := watchContext(ctx, view, finished)
-	stopSignals := watchSignals(view, finished)
+	stopSignals := watchSignals(view, ready, finished)
 
 	completed := false
 	defer func() {
@@ -447,16 +448,37 @@ func watchContext(ctx context.Context, view *liveView, finished <-chan struct{})
 	return func() { <-exited }
 }
 
+// startedModel wraps a live view's model so the signal relay knows when
+// the program has started: bubbletea v2.1.0 calls Init only after it has
+// started the renderer, so once ready is closed a Kill restores the cursor
+// and the terminal.
+type startedModel struct {
+	tea.Model
+	once  *sync.Once
+	ready chan struct{}
+}
+
+func (m startedModel) Init() tea.Cmd {
+	m.once.Do(func() { close(m.ready) })
+	return m.Model.Init()
+}
+
+func (m startedModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
+	inner, cmd := m.Model.Update(msg)
+	m.Model = inner
+	return m, cmd
+}
+
 // watchSignals relays SIGINT and SIGTERM (those not ignored) while view
 // runs. The views run without bubbletea's own signal handling, so without
 // this a Ctrl+C would end the process with the cursor still hidden. On a
-// signal the program is killed (bubbletea then restores the cursor and the
-// terminal), the view is unregistered and the signal is raised again on
-// the process with the relay stopped, so its default action, or the
-// command's own handler, still applies. The returned function stops the
-// relay when the view ends normally; a signal that arrived meanwhile is
-// raised again too.
-func watchSignals(view *liveView, finished <-chan struct{}) (stop func()) {
+// signal the program is killed once it has started (ready is closed) or
+// its Run has ended (bubbletea then restores the cursor and the terminal),
+// the view is unregistered and the signal is raised again on the process
+// with the relay stopped, so its default action, or the command's own
+// handler, still applies. The returned function stops the relay when the
+// view ends normally; a signal that arrived meanwhile is raised again too.
+func watchSignals(view *liveView, ready, finished <-chan struct{}) (stop func()) {
 	var relay []os.Signal
 	for _, sig := range []os.Signal{os.Interrupt, syscall.SIGTERM} {
 		if !signal.Ignored(sig) {
@@ -472,19 +494,14 @@ func watchSignals(view *liveView, finished <-chan struct{}) (stop func()) {
 	done := make(chan struct{})
 	go func() {
 		defer close(done)
-		select {
-		case sig := <-sigs:
-			view.prog.Kill()
-			<-finished
+		relaySignals(sigs, quit, ready, finished, view.prog.Kill, func() {
 			liveMu.Lock()
 			if activeView == view {
 				activeView = nil
 			}
 			liveMu.Unlock()
 			signal.Stop(sigs)
-			raise(sig)
-		case <-quit:
-		}
+		}, raise)
 	}()
 	return func() {
 		close(quit)
@@ -498,8 +515,35 @@ func watchSignals(view *liveView, finished <-chan struct{}) (stop func()) {
 	}
 }
 
+// relaySignals is the relay of watchSignals. On a signal from sigs it
+// waits until the program has started (ready) or its Run has ended
+// (finished), kills it, waits for finished, unregisters the view and the
+// relay (unregister) and raises the signal. Killing before the program
+// has started would leave the cursor hidden: bubbletea restores the
+// terminal only when its renderer is running. quit ends the relay without
+// a signal.
+func relaySignals(sigs <-chan os.Signal, quit, ready, finished <-chan struct{}, kill, unregister func(), raise func(os.Signal)) {
+	select {
+	case sig := <-sigs:
+		select {
+		case <-ready:
+		case <-finished:
+		}
+		kill()
+		<-finished
+		unregister()
+		raise(sig)
+	case <-quit:
+	}
+}
+
 // raise sends sig to the own process; when that fails, it exits with 128
-// plus the signal number, as a shell reports a death by that signal.
+// plus the signal number, as a shell reports a death by that signal. That
+// is the path on platforms where a process cannot signal itself (Windows):
+// the relay exits after restoring the terminal, so a command's own signal
+// handler may not run. Go cannot tell whether another handler is
+// registered, and returning without exiting would swallow Ctrl+C for
+// commands that have none.
 func raise(sig os.Signal) {
 	p, err := os.FindProcess(os.Getpid())
 	if err == nil {

modules/bonfire/live_test.go

@@ -769,3 +769,86 @@ func TestErrorPrintsAboveView(t *testing.T) {
 		}
 	}
 }
+
+// TestSignalRelayWaitsForStart (IN-12): the signal relay kills the program
+// only once it has started (its Init closed ready) or its Run has ended,
+// so a signal during the view's start-up still restores the cursor; then
+// it waits for the program, unregisters and raises the signal again.
+func TestSignalRelayWaitsForStart(t *testing.T) {
+	type relayRun struct {
+		sigs                  chan os.Signal
+		quit, ready, finished chan struct{}
+		killed, unregistered  chan struct{}
+		raised                chan os.Signal
+		done                  chan struct{}
+	}
+	start := func() *relayRun {
+		r := &relayRun{
+			sigs: make(chan os.Signal, 1), quit: make(chan struct{}),
+			ready: make(chan struct{}), finished: make(chan struct{}),
+			killed: make(chan struct{}, 1), unregistered: make(chan struct{}, 1),
+			raised: make(chan os.Signal, 2), done: make(chan struct{}),
+		}
+		go func() {
+			defer close(r.done)
+			relaySignals(r.sigs, r.quit, r.ready, r.finished,
+				func() { r.killed <- struct{}{} },
+				func() { r.unregistered <- struct{}{} },
+				func(sig os.Signal) { r.raised <- sig })
+		}()
+		return r
+	}
+	wait := func(t *testing.T, ch <-chan struct{}, what string) {
+		t.Helper()
+		select {
+		case <-ch:
+		case <-time.After(5 * time.Second):
+			t.Fatalf("%s did not happen", what)
+		}
+	}
+
+	t.Run("signal before start", func(t *testing.T) {
+		r := start()
+		r.sigs <- os.Interrupt
+		select {
+		case <-r.killed:
+			t.Fatal("killed before the program started")
+		case <-time.After(100 * time.Millisecond):
+		}
+		close(r.ready)
+		wait(t, r.killed, "kill after ready")
+		select {
+		case sig := <-r.raised:
+			t.Fatalf("raised %v before the program finished", sig)
+		case <-time.After(50 * time.Millisecond):
+		}
+		close(r.finished)
+		wait(t, r.done, "relay end")
+		wait(t, r.unregistered, "unregister")
+		if len(r.raised) != 1 {
+			t.Fatalf("raised %d signals, want 1", len(r.raised))
+		}
+		if sig := <-r.raised; sig != os.Interrupt {
+			t.Errorf("raised %v, want %v", sig, os.Interrupt)
+		}
+	})
+
+	t.Run("program ended without starting", func(t *testing.T) {
+		r := start()
+		r.sigs <- os.Interrupt
+		close(r.finished)
+		wait(t, r.done, "relay end")
+		if len(r.raised) != 1 {
+			t.Fatalf("raised %d signals, want 1", len(r.raised))
+		}
+	})
+
+	t.Run("quit", func(t *testing.T) {
+		r := start()
+		close(r.quit)
+		wait(t, r.done, "relay end")
+		if len(r.killed) != 0 || len(r.raised) != 0 {
+			t.Errorf("quit killed %d times and raised %d signals", len(r.killed), len(r.raised))
+		}
+	})
+}

scripts/check-phase19.8.sh

@@ -134,8 +134,8 @@ BONFIRE_TESTS=(
 	# quick 261028-rqd (info findings): IN-03, IN-06, IN-02, IN-04.
 	TestPluginErrorWordedUnknownCommand TestExecuteKeepsRootHelp
 	TestErrorBlockUsesParsedGlobals TestFlagBool
-	# quick 261028-rqd: IN-11, IN-10.
-	TestPrintAboveBlankLine TestErrorPrintsAboveView
+	# quick 261028-rqd: IN-11, IN-10, IN-12.
+	TestPrintAboveBlankLine TestErrorPrintsAboveView TestSignalRelayWaitsForStart
 )
 DOCSITE_TESTS=(
 	# Plan 04: the docs checker reads past the global options.