diff --git a/cmd/display/tty.go b/cmd/display/tty.go index d858db88f..a277da856 100644 --- a/cmd/display/tty.go +++ b/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) diff --git a/cmd/display/tty_test.go b/cmd/display/tty_test.go index 0945ee8e6..07e606649 100644 --- a/cmd/display/tty_test.go +++ b/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) { diff --git a/pkg/compose/run.go b/pkg/compose/run.go index 3b3308493..1f233fe32 100644 --- a/pkg/compose/run.go +++ b/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) }