fix(display): harden termWriter lifecycle against review findings on #14119

Transposes the applicable findings from the review of the ttyWriter fix
(#14119) to the rewritten renderer:

- Done waits for the refresh goroutine to exit (outside the lock), so
  nothing can repaint after Done returns and output printed right after
  an operation cannot be garbled by a stray frame
- a nested Start retires the previous cycle instead of leaking its
  refresh goroutine until the parent context is cancelled
- build suspension no longer leaks across cycles: Start resets it and
  starts a fresh block below whatever buildkit wrote
- drop the dead strings.ToLower in startDependencies: the unexported
  start only reads projectName when no Project is passed

The remaining findings are structural to this rewrite: Done is a single
critical section, and per-cycle context cancellation replaces the
doneSignal handshake.

Signed-off-by: Nicolas De Loof <[email protected]>

cmd/display/tty.go

@@ -69,11 +69,12 @@ type termWriter struct {
 	detached bool
 	dryRun   bool
 
-	tree      taskTree
-	scr       screen
-	operation string
-	suspended bool
-	stopTicks context.CancelFunc
+	tree        taskTree
+	scr         screen
+	operation   string
+	suspended   bool
+	stopTicks   context.CancelFunc
+	ticksExited chan struct{} // closed by the refresh goroutine when it returns
 
 	// injected for tests
 	size func() (width, height int)
@@ -94,16 +95,33 @@ func termSize() (int, int) {
 func (w *termWriter) Start(ctx context.Context, operation string) {
 	w.mu.Lock()
 	defer w.mu.Unlock()
+	// A Start before the matching Done (nested brackets are a misuse, but
+	// Full is public API): retire the previous cycle so its refresh
+	// goroutine doesn't outlive it.
+	if w.stopTicks != nil {
+		w.stopTicks()
+		w.stopTicks = nil
+	}
+	// Build suspension is scoped to a cycle: a new operation must not stay
+	// silent because the previous one ended mid-build, and buildkit wrote
+	// below the last frame, so the new cycle starts a fresh block.
+	if w.suspended {
+		w.suspended = false
+		w.scr.reset()
+	}
 	w.operation = operation
 	// The refresh goroutine is bound to a derived context: parent
 	// cancellation (Ctrl-C) and Done both stop it by cancelling, which can
 	// never block — there is no channel handshake to miss.
 	tickCtx, cancel := context.WithCancel(ctx)
+	exited := make(chan struct{})
 	w.stopTicks = cancel
-	go w.refresh(tickCtx)
+	w.ticksExited = exited
+	go w.refresh(tickCtx, exited)
 }
 
-func (w *termWriter) refresh(ctx context.Context) {
+func (w *termWriter) refresh(ctx context.Context, exited chan<- struct{}) {
+	defer close(exited)
 	ticker := time.NewTicker(tickInterval)
 	defer ticker.Stop()
 	for {
@@ -120,15 +138,25 @@ func (w *termWriter) refresh(ctx context.Context) {
 
 func (w *termWriter) Done(string, bool) {
 	w.mu.Lock()
-	defer w.mu.Unlock()
 	if w.stopTicks != nil {
 		w.stopTicks()
 		w.stopTicks = nil
 	}
+	exited := w.ticksExited
+	w.ticksExited = nil
 	w.repaint() // leave the final state on screen
 	w.operation = ""
 	// The tree and the screen survive: a follow-up operation on the same
 	// writer (`up` chains create and start) extends the same block.
+	w.mu.Unlock()
+
+	// Wait for the refresh goroutine outside the lock (it may be blocked on
+	// it, about to no-op since operation is cleared): once Done returns,
+	// nothing repaints anymore, so output printed right after an operation
+	// (a prompt, container logs) cannot be garbled by a stray frame.
+	if exited != nil {
+		<-exited
+	}
 }
 
 func (w *termWriter) On(events ...api.Resource) {
@@ -140,7 +168,10 @@ func (w *termWriter) On(events ...api.Resource) {
 			continue
 		}
 		if w.operation != "start" && (e.Text == api.StatusStarted || e.Text == api.StatusStarting) && !w.detached {
-			// skip those events to avoid mixing with container logs
+			// Deliberate: attached run/up stream container logs on this same
+			// terminal, and painting Starting/Started rows here would repaint
+			// over the first log lines of the container that just started.
+			// Outside an explicit `start` operation those events are dropped.
 			continue
 		}
 		w.handle(e)

cmd/display/tty_test.go

@@ -26,6 +26,7 @@ import (
 	"unicode/utf8"
 
 	"github.com/mattn/go-runewidth"
+	"go.uber.org/goleak"
 	"gotest.tools/v3/assert"
 
 	"github.com/docker/compose/v5/pkg/api"
@@ -167,6 +168,59 @@ func TestTerm_DoneReturnsAfterContextCancel(t *testing.T) {
 	}
 }
 
+// Done is reachable through the public api.EventProcessor interface, so a
+// caller isn't guaranteed to have called Start first: it must be a no-op,
+// not a nil dereference.
+func TestTerm_DoneBeforeStartDoesNotPanic(t *testing.T) {
+	w, _, _ := newTermWriter(80, 24)
+	w.Done("op", false)
+}
+
+// Once Done returns, nothing repaints anymore: the refresh goroutine must
+// not survive the bracket, or a stray frame could garble whatever the caller
+// prints next (a prompt, container logs). goleak needs no settling sleep
+// precisely because Done waits for the goroutine to exit.
+func TestTerm_NoRenderGoroutineSurvivesDone(t *testing.T) {
+	w, _, _ := newTermWriter(80, 24)
+	w.Start(t.Context(), "pull")
+	w.On(api.Resource{ID: "Image foo", Text: "Pulling", Status: api.Working})
+	w.Done("pull", true)
+	goleak.VerifyNone(t)
+}
+
+// A Start before the matching Done (nested brackets are a misuse, but Full
+// is public API) must retire the previous cycle instead of leaking its
+// refresh goroutine until the parent context is cancelled.
+func TestTerm_NestedStartRetiresPreviousCycle(t *testing.T) {
+	w, _, _ := newTermWriter(80, 24)
+	ctx := t.Context()
+	w.Start(ctx, "outer")
+	w.Start(ctx, "inner")
+	w.Done("inner", false)
+	w.Done("outer", false)
+	goleak.VerifyNone(t)
+}
+
+// Build suspension is scoped to a cycle: an operation ending mid-build (a
+// failed build aborts the bracket with suspended still set) must not leave
+// the next operation's progress permanently silent.
+func TestTerm_StartResetsBuildSuspension(t *testing.T) {
+	w, buf, _ := newTermWriter(80, 24)
+	w.Start(t.Context(), "build")
+	w.On(api.Resource{ID: "Image app", Text: api.StatusBuilding, Status: api.Working})
+	w.Done("build", false)
+
+	w.Start(t.Context(), "create")
+	w.On(api.Resource{ID: "container app-1", Text: "Creating", Status: api.Working})
+	w.mu.Lock()
+	w.repaint()
+	w.mu.Unlock()
+	w.Done("create", true)
+
+	frame := stripAnsi(buf.String())
+	assert.Assert(t, strings.Contains(frame, "container app-1"), "expected the new cycle to paint, got: %q", frame)
+}
+
 // The spinner animation must be a function of time, not of how often the
 // layout runs.
 func TestTerm_SpinnerIsTimeBased(t *testing.T) {

pkg/compose/run.go

@@ -23,7 +23,6 @@ import (
 	"os"
 	"os/signal"
 	"slices"
-	"strings"
 
 	"github.com/compose-spec/compose-go/v2/types"
 	"github.com/docker/cli/cli"
@@ -295,7 +294,7 @@ func (s *composeService) startDependencies(ctx context.Context, project *types.P
 	}
 
 	if len(project.Services) > 0 {
-		return s.start(ctx, strings.ToLower(project.Name), api.StartOptions{
+		return s.start(ctx, project.Name, api.StartOptions{
 			Project: project,
 		}, nil)
 	}