fix: detect externally stopped and removed containers in up monitor

pkg/compose/monitor.go

@@ -159,6 +159,13 @@ func (c *monitor) Start(ctx context.Context) error {
 				c.onContainerStart(event, ctr, containers, restarting)
 			case events.ActionRestart:
 				c.onContainerRestart(event, ctr)
+			case events.ActionStop:
+				err := c.onContainerStop(ctx, ctr, containers, restarting)
+				if err != nil {
+					return err
+				}
+			case events.ActionDestroy:
+				c.onContainerDestroy(ctr, containers, restarting)
 			case events.ActionDie:
 				err := c.onContainerDie(ctx, event, ctr, containers, restarting)
 				if err != nil {
@@ -238,17 +245,11 @@ func (c *monitor) onContainerRestart(event events.Message, ctr *api.ContainerSum
 
 func (c *monitor) onContainerDie(ctx context.Context, event events.Message, ctr *api.ContainerSummary, containers, restarting utils.Set[string]) error {
 	logrus.Debugf("container %s exited with code %d", ctr.Name, ctr.ExitCode)
-	inspect, err := c.apiClient.ContainerInspect(ctx, event.Actor.ID, client.ContainerInspectOptions{})
-	if errdefs.IsNotFound(err) {
-		// Source is already removed
-	} else if err != nil {
+	willRestart, err := c.isRestarting(ctx, ctr.ID)
+	if err != nil {
 		return err
 	}
-
-	if inspect.Container.State != nil && (inspect.Container.State.Restarting || inspect.Container.State.Running) {
-		// State.Restarting is set by engine when container is configured to restart on exit
-		// on ContainerRestart it doesn't (see https://github.com/moby/moby/issues/45538)
-		// container state still is reported as "running"
+	if willRestart {
 		logrus.Debugf("container %s is restarting", ctr.Name)
 		restarting.Add(ctr.ID)
 		c.notify(newContainerEvent(event.TimeNano, ctr, api.ContainerEventExited, func(e *api.ContainerEvent) {
@@ -262,6 +263,55 @@ func (c *monitor) onContainerDie(ctx context.Context, event events.Message, ctr
 	return nil
 }
 
+// onContainerStop handles a stop event with no following start: the container won't
+// come back, either because it has no restart policy, or because an external
+// `stop`/`down` canceled the restart loop of a container in backoff
+// (https://github.com/docker/compose/issues/13985). The event alone can't tell us:
+// during a ContainerRestart (watch sync+restart, https://github.com/docker/compose/issues/13161)
+// the engine also emits `stop` before `start`.
+func (c *monitor) onContainerStop(ctx context.Context, ctr *api.ContainerSummary, containers, restarting utils.Set[string]) error {
+	willRestart, err := c.isRestarting(ctx, ctr.ID)
+	if err != nil {
+		return err
+	}
+	if willRestart {
+		logrus.Debugf("container %s stopped, restart in progress", ctr.Name)
+		restarting.Add(ctr.ID)
+	} else {
+		// definitive stop: the exit was already reported to listeners by the
+		// preceding die event, just stop tracking the container
+		logrus.Debugf("container %s stopped", ctr.Name)
+		restarting.Remove(ctr.ID)
+		containers.Remove(ctr.ID)
+	}
+	return nil
+}
+
+// onContainerDestroy handles a container removed by an external `docker compose down`:
+// terminal state, there is nothing left to inspect.
+func (c *monitor) onContainerDestroy(ctr *api.ContainerSummary, containers, restarting utils.Set[string]) {
+	logrus.Debugf("container %s destroyed", ctr.Name)
+	restarting.Remove(ctr.ID)
+	containers.Remove(ctr.ID)
+}
+
+// isRestarting tells whether a container which just stopped is expected to come back.
+// State.Restarting is set by the engine when the container is configured to restart on
+// exit, but not on a ContainerRestart, where state still is reported as "running"
+// (see https://github.com/moby/moby/issues/45538). A container already removed won't
+// come back.
+func (c *monitor) isRestarting(ctx context.Context, containerID string) (bool, error) {
+	inspect, err := c.apiClient.ContainerInspect(ctx, containerID, client.ContainerInspectOptions{})
+	if errdefs.IsNotFound(err) {
+		return false, nil
+	}
+	if err != nil {
+		return false, err
+	}
+	state := inspect.Container.State
+	return state != nil && (state.Restarting || state.Running), nil
+}
+
 func newContainerEvent(timeNano int64, ctr *api.ContainerSummary, eventType int, opts ...func(e *api.ContainerEvent)) api.ContainerEvent {
 	name := ctr.Name
 	defaultName := getDefaultContainerName(ctr.Project, ctr.Labels[api.ServiceLabel], ctr.Labels[api.ContainerNumberLabel])

pkg/compose/monitor_test.go

@@ -19,13 +19,17 @@ package compose
 import (
 	"context"
 	"errors"
+	"strconv"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/containerd/errdefs"
 	"github.com/google/go-cmp/cmp"
 	"github.com/moby/moby/api/types/container"
 	"github.com/moby/moby/api/types/events"
 	"github.com/moby/moby/client"
+	"go.uber.org/goleak"
 	"go.uber.org/mock/gomock"
 	"gotest.tools/v3/assert"
 
@@ -222,3 +226,177 @@ func TestMonitorStartBadExitCode(t *testing.T) {
 	err := m.Start(t.Context())
 	assert.ErrorContains(t, err, "not-a-number")
 }
+
+// monitorEvent builds an engine event for container "123"/service1, with the
+// Actor.Attributes shape reported by the engine: compose labels plus the
+// container name.
+func monitorEvent(action events.Action) events.Message {
+	attrs := containerLabels("service1", false)
+	attrs["name"] = "testproject-service1-1"
+	return events.Message{
+		Type:   events.ContainerEventType,
+		Action: action,
+		Actor:  events.Actor{ID: "123", Attributes: attrs},
+	}
+}
+
+// monitorDieEvent builds a die event, which the engine reports with an exit code.
+func monitorDieEvent(exitCode int) events.Message {
+	event := monitorEvent(events.ActionDie)
+	event.Actor.Attributes["exitCode"] = strconv.Itoa(exitCode)
+	return event
+}
+
+// newMonitorTestFixture wires a monitor against a mocked API client, with the
+// goroutine-leak guard and the standard initial ContainerList expectation.
+func newMonitorTestFixture(t *testing.T) (*monitor, *mocks.MockAPIClient) {
+	t.Helper()
+	ignoreExisting := goleak.IgnoreCurrent()
+	t.Cleanup(func() {
+		goleak.VerifyNone(t, ignoreExisting)
+	})
+	mockCtrl := gomock.NewController(t)
+	t.Cleanup(mockCtrl.Finish)
+	apiMock := mocks.NewMockAPIClient(mockCtrl)
+
+	apiMock.EXPECT().ContainerList(gomock.Any(), gomock.Any()).
+		Return(client.ContainerListResult{Items: []container.Summary{testContainer("service1", "123", false)}}, nil)
+
+	m := newMonitor(apiMock, strings.ToLower(testProject))
+	return m, apiMock
+}
+
+// expectEvents makes the mocked engine deliver the given events, in order.
+func expectEvents(apiMock *mocks.MockAPIClient, msgs ...events.Message) {
+	ch := make(chan events.Message, len(msgs))
+	for _, msg := range msgs {
+		ch <- msg
+	}
+	apiMock.EXPECT().Events(gomock.Any(), gomock.Any()).
+		Return(client.EventsResult{Messages: ch, Err: make(chan error)})
+}
+
+// expectInspects makes successive inspections of container "123" report the
+// given states, in order.
+func expectInspects(apiMock *mocks.MockAPIClient, states ...container.State) {
+	calls := make([]any, 0, len(states))
+	for _, state := range states {
+		calls = append(calls, apiMock.EXPECT().
+			ContainerInspect(gomock.Any(), "123", client.ContainerInspectOptions{}).
+			Return(client.ContainerInspectResult{Container: container.InspectResponse{State: &state}}, nil))
+	}
+	gomock.InOrder(calls...)
+}
+
+// runMonitor starts the monitor under test in a goroutine and waits (with a
+// timeout) for it to return, reporting the events it published. It fails the
+// test if the monitor doesn't stop on its own, which is how an un-fixed
+// monitor.Start reacts to stop/destroy events it doesn't know how to process:
+// the tracked containers set never empties, so the loop blocks forever on the
+// events channel.
+func runMonitor(t *testing.T, m *monitor) ([]api.ContainerEvent, error) {
+	t.Helper()
+	var got []api.ContainerEvent
+	m.withListener(func(e api.ContainerEvent) {
+		got = append(got, e)
+	})
+
+	done := make(chan error, 1)
+	go func() {
+		done <- m.Start(t.Context())
+	}()
+	select {
+	case err := <-done:
+		return got, err
+	case <-time.After(10 * time.Second):
+		t.Fatal("monitor did not stop")
+		return nil, nil
+	}
+}
+
+// TestMonitorExitsOnDestroy pins the expectation that a destroy event (e.g. a
+// container removed by `docker rm` or `docker compose rm` outside of a
+// tracked lifecycle transition) drops the container from the tracked set
+// without requiring any inspection, so the monitor loop terminates.
+func TestMonitorExitsOnDestroy(t *testing.T) {
+	m, apiMock := newMonitorTestFixture(t)
+	expectEvents(apiMock, monitorEvent(events.ActionDestroy))
+
+	got, err := runMonitor(t, m)
+	assert.NilError(t, err)
+	assert.Equal(t, len(got), 0)
+}
+
+// TestMonitorExitsWhenRestartingContainerStopped is the #13985 repro: a
+// container configured to restart on failure dies (engine reports it as
+// still "restarting"), then is explicitly stopped (e.g. `docker stop`)
+// before the restart happens. The monitor must inspect on stop, observe the
+// container is no longer restarting/running, and evict it so the loop
+// terminates instead of waiting forever for a start event that never comes.
+func TestMonitorExitsWhenRestartingContainerStopped(t *testing.T) {
+	m, apiMock := newMonitorTestFixture(t)
+	expectEvents(apiMock, monitorDieEvent(1), monitorEvent(events.ActionStop))
+	expectInspects(apiMock,
+		// on die: waiting for the restart policy to kick in
+		container.State{Status: container.StateRestarting, Restarting: true, ExitCode: 1},
+		// on stop: the restart loop got canceled
+		container.State{Status: container.StateExited, ExitCode: 1},
+	)
+
+	got, err := runMonitor(t, m)
+	assert.NilError(t, err)
+	assert.Equal(t, len(got), 1)
+	assert.Equal(t, got[0].Type, api.ContainerEventExited)
+	assert.Equal(t, got[0].Restarting, true)
+	assert.Equal(t, got[0].ExitCode, 1)
+}
+
+// TestMonitorKeepsRunningOnRestart is the #13161 guard: a container that
+// dies and is restarted by the engine (watch/sync workflows trigger this via
+// `docker restart`) must not be evicted by an intervening stop event that is
+// merely part of the moby#45538 restart sequence (State reports
+// Running=true while mid-ContainerRestart). The monitor must keep tracking
+// it and still observe the subsequent start.
+func TestMonitorKeepsRunningOnRestart(t *testing.T) {
+	m, apiMock := newMonitorTestFixture(t)
+	expectEvents(apiMock,
+		monitorDieEvent(0),
+		monitorEvent(events.ActionStop),
+		monitorEvent(events.ActionStart),
+		monitorDieEvent(1),
+	)
+	expectInspects(apiMock,
+		// on die then on stop: mid-ContainerRestart, so still reported as running
+		container.State{Status: container.StateRunning, Running: true},
+		container.State{Status: container.StateRunning, Running: true},
+		// on the final die: really gone
+		container.State{Status: container.StateExited, ExitCode: 1},
+	)
+
+	got, err := runMonitor(t, m)
+	assert.NilError(t, err)
+	assert.Equal(t, len(got), 3)
+	assert.Equal(t, got[0].Type, api.ContainerEventExited)
+	assert.Equal(t, got[0].Restarting, true)
+	assert.Equal(t, got[0].ExitCode, 0)
+	assert.Equal(t, got[1].Type, api.ContainerEventStarted)
+	assert.Equal(t, got[1].Restarting, true)
+	assert.Equal(t, got[2].Type, api.ContainerEventExited)
+	assert.Equal(t, got[2].Restarting, false)
+	assert.Equal(t, got[2].ExitCode, 1)
+}
+
+// TestMonitorStopInspectNotFound covers a stop event racing a container's
+// removal: the inspect on stop returns NotFound, which must be tolerated
+// (not treated as a fatal error) and the container evicted so the monitor
+// terminates.
+func TestMonitorStopInspectNotFound(t *testing.T) {
+	m, apiMock := newMonitorTestFixture(t)
+	expectEvents(apiMock, monitorEvent(events.ActionStop))
+	apiMock.EXPECT().ContainerInspect(gomock.Any(), "123", client.ContainerInspectOptions{}).
+		Return(client.ContainerInspectResult{}, errdefs.ErrNotFound.WithMessage("no such container: 123"))
+
+	got, err := runMonitor(t, m)
+	assert.NilError(t, err)
+	assert.Equal(t, len(got), 0)
+}

pkg/e2e/testdata/TestUpAttachedTerminatesOnExternalStop/compose.yaml

@@ -0,0 +1,5 @@
+services:
+  app:
+    image: alpine
+    restart: unless-stopped
+    command: sh -c "exit 1"

pkg/e2e/up_test.go

@@ -22,12 +22,14 @@ import (
 	"context"
 	"errors"
 	"os/exec"
+	"path/filepath"
 	"strings"
 	"syscall"
 	"testing"
 	"time"
 
 	"gotest.tools/v3/assert"
+	"gotest.tools/v3/icmd"
 
 	"github.com/docker/compose/v5/pkg/utils"
 )
@@ -167,6 +169,61 @@ func TestUpImageID(t *testing.T) {
 			ComposeCmd("up"))
 }
 
+// TestUpAttachedTerminatesOnExternalStop is the #13985 repro: since 2.39.3 an
+// attached `up` never returns when the project is stopped and removed by
+// another process while a service configured with a restart policy sits in
+// its restart backoff — such a container only emits stop/destroy, never the
+// die event the monitor used to rely on exclusively to detect termination.
+func TestUpAttachedTerminatesOnExternalStop(t *testing.T) {
+	s := NewScenario(t, "an attached up must return once an external stop/down cancels a service's restart backoff")
+
+	var out utils.SafeBuffer
+	ctx, cancel := context.WithTimeout(t.Context(), 60*time.Second)
+	t.Cleanup(cancel)
+	cmd, err := StartWithNewGroupID(ctx,
+		s.CLI().NewDockerComposeCmd(t, "-f", filepath.Join(s.Dir(), "compose.yaml"), "--project-name", s.Project(), "up"),
+		&out, &out)
+	assert.NilError(t, err)
+
+	upDone := make(chan error, 1)
+	go func() {
+		upDone <- cmd.Wait()
+	}()
+
+	// wait until the container is in restart backoff (no process running,
+	// State.Restarting=true): the die event from its failed attempt already
+	// fired, and won't fire again until the backoff expires
+	var restartCount string
+	s.CLI().WaitForCmdResult(t,
+		s.CLI().NewDockerCmd(t, "inspect", s.Project()+"-app-1", "-f", "{{.State.Restarting}} {{.RestartCount}}"),
+		func(res *icmd.Result) bool {
+			restarting, count, ok := strings.Cut(strings.TrimSpace(res.Stdout()), " ")
+			restartCount = count
+			return ok && restarting == "true"
+		},
+		30*time.Second, 250*time.Millisecond)
+
+	// narrow (can't fully close) the race with the backoff expiring: if the
+	// container already restarted by here, the die event handles
+	// termination the same way it always has, and the #13985 fix (stop
+	// landing with no process running) never gets exercised
+	res := s.CLI().RunDockerCmd(t, "inspect", s.Project()+"-app-1", "-f", "{{.RestartCount}}")
+	assert.Equal(t, strings.TrimSpace(res.Stdout()), restartCount, "container restarted again before the external stop could land in its backoff window; rerun")
+
+	// stop while still in backoff is the #13985 regression; down is then
+	// plain teardown — the container is already untracked by the time it
+	// runs, so it does not exercise onContainerDestroy (covered separately
+	// by TestMonitorExitsOnDestroy)
+	s.CLI().RunDockerComposeCmd(t, "--project-name", s.Project(), "stop")
+	s.CLI().RunDockerComposeCmd(t, "--project-name", s.Project(), "down")
+
+	err = <-upDone
+	if ctx.Err() != nil {
+		t.Fatalf("up did not terminate after the project was stopped and removed externally (see #13985)\n%s", out.String())
+	}
+	assert.NilError(t, err, out.String())
+}
+
 func TestUpStopWithLogsMixed(t *testing.T) {
 	// service2 pings forever so the abort always interrupts it: with a bounded
 	// ping, on a fast machine it can exit on its own before the abort reaches