From 1ac75ad59a40d6dd1d18e84f9350e5552fbcf0e1 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 22 Oct 2026 03:53:29 +0200 Subject: [PATCH] fix(bonfire): claim the live view slot before building the program (IN-15) --- modules/bonfire/live.go | 20 ++++++---- modules/bonfire/live_test.go | 71 ++++++++++++++++++++++++++++++++++++ scripts/check-phase19.8.sh | 2 + 3 files changed, 85 insertions(+), 8 deletions(-) diff --git a/modules/bonfire/live.go b/modules/bonfire/live.go index f9939773..43567434 100644 --- a/modules/bonfire/live.go +++ b/modules/bonfire/live.go @@ -341,9 +341,10 @@ var errLivePanic = errors.New("bonfire: live view work panicked") // runLive runs model as a bubbletea program on out while work runs on the // calling goroutine, with c as the owner of the view. It first claims the -// single live slot under liveMu; when another view holds it, runLive -// returns started false without running work, and the caller prints plain -// lines instead. work gets a send function for progress messages. When +// single live slot under liveMu and only then builds the program; when +// another view holds the slot, runLive returns started false without +// building a program or running work, and the caller prints plain lines +// instead. work gets a send function for progress messages. When // work returns or panics, the view is unregistered (so PrintAbove stops // sending to it), told to finish and waited for, all under liveMu; it // never receives lines after that, and a panic goes on after the cleanup. @@ -351,6 +352,14 @@ func runLive(ctx context.Context, out io.Writer, c *console, model tea.Model, wo if ctx == nil { ctx = context.Background() } + // Claim the slot before building the program: a widget that loses it + // builds none, so it registers no child context on ctx and opens no + // TEA_TRACE file. NewProgram takes no bonfire lock and prints nothing. + liveMu.Lock() + if activeView != nil { + liveMu.Unlock() + return false, nil + } prog := tea.NewProgram(model, tea.WithInput(nil), tea.WithOutput(out), @@ -359,11 +368,6 @@ func runLive(ctx context.Context, out io.Writer, c *console, model tea.Model, wo tea.WithColorProfile(c.profile()), ) view := &liveView{owner: c, prog: prog} - liveMu.Lock() - if activeView != nil { - liveMu.Unlock() - return false, nil - } activeView = view liveMu.Unlock() diff --git a/modules/bonfire/live_test.go b/modules/bonfire/live_test.go index 43a796ab..0badba00 100644 --- a/modules/bonfire/live_test.go +++ b/modules/bonfire/live_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "os" "regexp" "strings" "sync" @@ -584,3 +585,73 @@ func TestProcessViewStripsControl(t *testing.T) { }) } } + +// countingCtx is a context that never ends and counts the children that +// context.WithCancel registers on it through AfterFunc and has not stopped. +type countingCtx struct { + mu sync.Mutex + outstanding int + done chan struct{} +} + +func (c *countingCtx) Deadline() (time.Time, bool) { return time.Time{}, false } +func (c *countingCtx) Done() <-chan struct{} { return c.done } +func (c *countingCtx) Err() error { return nil } +func (c *countingCtx) Value(any) any { return nil } + +func (c *countingCtx) AfterFunc(func()) (stop func() bool) { + c.mu.Lock() + c.outstanding++ + c.mu.Unlock() + var once sync.Once + return func() bool { + stopped := false + once.Do(func() { + c.mu.Lock() + c.outstanding-- + c.mu.Unlock() + stopped = true + }) + return stopped + } +} + +func (c *countingCtx) registered() int { + c.mu.Lock() + defer c.mu.Unlock() + return c.outstanding +} + +// TestNestedProcessBuildsNoProgram (IN-15): a Process nested in a running +// view loses the live slot before any program is built, so it registers no +// child context on the caller's context and opens no TEA_TRACE file; its +// items print as plain lines. +func TestNestedProcessBuildsNoProgram(t *testing.T) { + out := &lockedBuffer{} + c := newConsole(strings.NewReader(""), out, io.Discard, noEnv, boolPtr(true), boolPtr(true)) + trace := t.TempDir() + "/trace.log" + ctx := &countingCtx{done: make(chan struct{})} + calls := 0 + err := c.Spinner("outer", func() error { + t.Setenv("TEA_TRACE", trace) + return Process(ctx, c, "Nested", []string{"x"}, func(context.Context, string) error { + calls++ + return nil + }) + }) + if err != nil { + t.Fatal(err) + } + if calls != 1 { + t.Errorf("fn ran %d times, want 1", calls) + } + if _, serr := os.Stat(trace); serr == nil { + t.Error("the nested Process built a program: the TEA_TRACE file exists") + } + if n := ctx.registered(); n != 0 { + t.Errorf("%d child contexts still registered on the caller's context, want 0", n) + } + if !strings.Contains(ansi.Strip(out.String()), symCheck+" x") { + t.Errorf("the item's success line is missing:\n%s", ansi.Strip(out.String())) + } +} diff --git a/scripts/check-phase19.8.sh b/scripts/check-phase19.8.sh index aba3eae8..d98ebfd1 100755 --- a/scripts/check-phase19.8.sh +++ b/scripts/check-phase19.8.sh @@ -129,6 +129,8 @@ BONFIRE_TESTS=( TestLiveViewRestoresTerminalOnSignal TestWithExitCode TestWrongDirectoryMasksSecrets TestSanitize TestRenderErrorStripsControl TestProcessViewStripsControl TestTableStripsControl + # quick 261022-4z8 (re-review fixes): IN-15, WR-09. + TestNestedProcessBuildsNoProgram ) DOCSITE_TESTS=( # Plan 04: the docs checker reads past the global options.