From b651fb75b9b77181ccc3dd8916b4f30550db6244 Mon Sep 17 00:00:00 2001 From: Guillaume Lours Date: Fri, 31 Jul 2026 11:38:11 +0200 Subject: [PATCH 1/2] fix: detect externally stopped and removed containers in up monitor Since 2.39.3 the monitor relies solely on die events to stop tracking containers, but a container stopped while in restart backoff emits only stop and destroy events, so an attached `up` hangs forever after an external `stop`/`down`. Handle destroy as terminal, and on stop inspect the container to distinguish a definitive stop from the transient one emitted during a ContainerRestart, which keeps watch sync+restart (#13161) working. Fixes #13985 Signed-off-by: Guillaume Lours --- pkg/compose/monitor.go | 68 ++++++++++++-- pkg/compose/monitor_test.go | 178 ++++++++++++++++++++++++++++++++++++ 2 files changed, 237 insertions(+), 9 deletions(-) diff --git a/pkg/compose/monitor.go b/pkg/compose/monitor.go index 58bb9341cf..33d912f205 100644 --- a/pkg/compose/monitor.go +++ b/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]) diff --git a/pkg/compose/monitor_test.go b/pkg/compose/monitor_test.go index 54b9be8922..53456919bf 100644 --- a/pkg/compose/monitor_test.go +++ b/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) +} From c9cadaffcf2c9e346e7c5e4d38590cd3012fd362 Mon Sep 17 00:00:00 2001 From: Guillaume Lours Date: Thu, 10 Sep 2026 19:41:06 +0200 Subject: [PATCH 2/2] test(e2e): add attached up regression test for #13985 Unit tests alone didn't cover the exact failure: an attached up hanging when the project is stopped/removed externally while a service sits in restart backoff. The new test reproduces that sequence end to end, so a future regression on this path breaks loudly instead of slipping through as a passing unit suite. A RestartCount guard narrows the race with the backoff expiring, so a missed window fails loudly instead of passing silently through the pre-existing die-based path. Signed-off-by: Guillaume Lours --- .../compose.yaml | 5 ++ pkg/e2e/up_test.go | 57 +++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 pkg/e2e/testdata/TestUpAttachedTerminatesOnExternalStop/compose.yaml diff --git a/pkg/e2e/testdata/TestUpAttachedTerminatesOnExternalStop/compose.yaml b/pkg/e2e/testdata/TestUpAttachedTerminatesOnExternalStop/compose.yaml new file mode 100644 index 0000000000..137e6f173f --- /dev/null +++ b/pkg/e2e/testdata/TestUpAttachedTerminatesOnExternalStop/compose.yaml @@ -0,0 +1,5 @@ +services: + app: + image: alpine + restart: unless-stopped + command: sh -c "exit 1" diff --git a/pkg/e2e/up_test.go b/pkg/e2e/up_test.go index 01c11b7b93..744a95a41a 100644 --- a/pkg/e2e/up_test.go +++ b/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