From 7dcf422deaca733eb1c72185bd3d605c86048a42 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Thu, 27 Aug 2026 14:30:21 +0200 Subject: [PATCH] chore(compose): orphan semantics stated where up and down diverge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A container is orphaned when it is a one-off (compose run) that FINISHED its task, or when it carries the project labels but its service is not defined by the compose model — the typical leftover after the compose file was edited. A still-RUNNING one-off is somebody's live session: 'up --remove-orphans' cleans up leftovers and must never kill it. 'down --remove-orphans' is the explicit stop-the-application action, so it DOES take running one-offs down — through the per-service removal loop, not the orphan branch. That asymmetry was implemented but stated nowhere: isOrphaned's comment only mentioned the model-absent half, down.go removed running one-offs as an unexplained side effect of including one-offs in the listing, and the observed-state collection silently dropped running one-offs with no hint it was deliberate. No behavior change — the semantics are now written at all three sites and pinned by tests: the predicate matrix, the observed-state classification (a running one-off is neither a service replica nor an orphan), and down's single stop+remove path for a running one-off of a declared service. Closes item C.4 of #14074. Signed-off-by: Nicolas De Loof --- pkg/compose/containers.go | 22 ++++++--- pkg/compose/containers_test.go | 71 ++++++++++++++++++++++++++++++ pkg/compose/down.go | 8 ++++ pkg/compose/down_test.go | 14 ++++++ pkg/compose/observed_state.go | 10 +++-- pkg/compose/observed_state_test.go | 18 +++++++- 6 files changed, 132 insertions(+), 11 deletions(-) create mode 100644 pkg/compose/containers_test.go diff --git a/pkg/compose/containers.go b/pkg/compose/containers.go index 74cc51c74e..c6ce6474fb 100644 --- a/pkg/compose/containers.go +++ b/pkg/compose/containers.go @@ -142,18 +142,26 @@ func isService(services ...string) containerPredicate { } } -// isOrphaned is a predicate to select containers without a matching service definition in compose project +// isOrphaned selects the containers `--remove-orphans` cleans up — and that +// `up` warns about otherwise. A container is orphaned when: +// +// - it is a one-off (`compose run`) that FINISHED its task (exited/dead): +// one-offs are ephemeral by design, a terminated one is a leftover. A +// still-RUNNING one-off is somebody's live session and is deliberately +// NOT an orphan: `up --remove-orphans` must never kill it. `down` does +// stop running one-offs, but on purpose and through the per-service +// removal path — down stops the application, and a running one-off is +// part of what goes down (see down.go); +// - or it carries this project's labels but its service is not defined by +// the compose model (neither enabled nor disabled) — the typical leftover +// after the compose file was edited and a service removed or renamed. func isOrphaned(project *types.Project) containerPredicate { services := append(project.ServiceNames(), project.DisabledServiceNames()...) return func(c container.Summary) bool { - // One-off container - v, ok := c.Labels[api.OneoffLabel] - if ok && v == "True" { + if v, ok := c.Labels[api.OneoffLabel]; ok && v == "True" { return c.State == container.StateExited || c.State == container.StateDead } - // Service that is not defined in the compose model - service := c.Labels[api.ServiceLabel] - return !slices.Contains(services, service) + return !slices.Contains(services, c.Labels[api.ServiceLabel]) } } diff --git a/pkg/compose/containers_test.go b/pkg/compose/containers_test.go new file mode 100644 index 0000000000..ae85cb373b --- /dev/null +++ b/pkg/compose/containers_test.go @@ -0,0 +1,71 @@ +/* + Copyright 2020 Docker Compose CLI authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package compose + +import ( + "testing" + + "github.com/compose-spec/compose-go/v2/types" + "github.com/moby/moby/api/types/container" + "gotest.tools/v3/assert" + + "github.com/docker/compose/v5/pkg/api" +) + +// TestIsOrphaned pins the orphan definition: a container is orphaned when it +// is a one-off that FINISHED its task (a still-running `compose run` is a live +// session `up --remove-orphans` must never kill), or when it carries the +// project labels but its service is not defined by the compose model (enabled +// or disabled) — the typical leftover after the compose file was edited. +func TestIsOrphaned(t *testing.T) { + project := &types.Project{ + Name: "p", + Services: types.Services{ + "web": {Name: "web"}, + }, + DisabledServices: types.Services{ + "debug": {Name: "debug"}, + }, + } + ctr := func(service, oneOff string, state container.ContainerState) container.Summary { + labels := map[string]string{api.ServiceLabel: service, api.ProjectLabel: "p"} + if oneOff != "" { + labels[api.OneoffLabel] = oneOff + } + return container.Summary{Labels: labels, State: state} + } + pred := isOrphaned(project) + + for _, tc := range []struct { + name string + c container.Summary + want bool + }{ + {"service replica running", ctr("web", "False", container.StateRunning), false}, + {"service replica exited", ctr("web", "False", container.StateExited), false}, + {"disabled-profile service", ctr("debug", "False", container.StateExited), false}, + {"service removed from the model", ctr("old", "False", container.StateRunning), true}, + {"one-off of a declared service, exited", ctr("web", "True", container.StateExited), true}, + {"one-off of a declared service, RUNNING: a live session, not an orphan", ctr("web", "True", container.StateRunning), false}, + {"one-off of a removed service, dead", ctr("old", "True", container.StateDead), true}, + {"one-off of a removed service, RUNNING: still a live session", ctr("old", "True", container.StateRunning), false}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, pred(tc.c), tc.want) + }) + } +} diff --git a/pkg/compose/down.go b/pkg/compose/down.go index e327b09145..87da25ff58 100644 --- a/pkg/compose/down.go +++ b/pkg/compose/down.go @@ -46,6 +46,14 @@ func (s *composeService) down(ctx context.Context, projectName string, options a include := oneOffExclude if options.RemoveOrphans { + // down stops the application: one-off containers — RUNNING ones + // included — are part of what goes down. Those attached to a declared + // service are stopped/removed by the per-service loop below (they + // match isService); the orphan branch at the end catches the + // remainder (finished one-offs and model-absent services — see + // isOrphaned). This is deliberately broader than `up + // --remove-orphans`, which only cleans up FINISHED one-offs and never + // kills a live `compose run` session. include = oneOffInclude } containers, err := s.getContainers(ctx, projectName, include, true) diff --git a/pkg/compose/down_test.go b/pkg/compose/down_test.go index eea13b9d23..6c4e446399 100644 --- a/pkg/compose/down_test.go +++ b/pkg/compose/down_test.go @@ -199,6 +199,7 @@ func TestDownRemoveOrphans(t *testing.T) { testContainer("service1", "123", false), testContainer("service2", "789", false), testContainer("service_orphan", "321", true), + runningOneOff("service1", "654"), }, }, nil) api.EXPECT().VolumeList( @@ -221,10 +222,16 @@ func TestDownRemoveOrphans(t *testing.T) { api.EXPECT().ContainerStop(gomock.Any(), "123", stopOptions).Return(client.ContainerStopResult{}, nil) api.EXPECT().ContainerStop(gomock.Any(), "789", stopOptions).Return(client.ContainerStopResult{}, nil) api.EXPECT().ContainerStop(gomock.Any(), "321", stopOptions).Return(client.ContainerStopResult{}, nil) + // The RUNNING one-off of a declared service goes down too — down stops the + // application — via the per-service removal loop (it matches isService; + // isOrphaned deliberately excludes running one-offs so `up` never kills a + // live session). Exactly one stop+remove. + api.EXPECT().ContainerStop(gomock.Any(), "654", stopOptions).Return(client.ContainerStopResult{}, nil) api.EXPECT().ContainerRemove(gomock.Any(), "123", client.ContainerRemoveOptions{Force: true}).Return(client.ContainerRemoveResult{}, nil) api.EXPECT().ContainerRemove(gomock.Any(), "789", client.ContainerRemoveOptions{Force: true}).Return(client.ContainerRemoveResult{}, nil) api.EXPECT().ContainerRemove(gomock.Any(), "321", client.ContainerRemoveOptions{Force: true}).Return(client.ContainerRemoveResult{}, nil) + api.EXPECT().ContainerRemove(gomock.Any(), "654", client.ContainerRemoveOptions{Force: true}).Return(client.ContainerRemoveResult{}, nil) api.EXPECT().NetworkList(gomock.Any(), client.NetworkListOptions{ Filters: projectFilter(strings.ToLower(testProject)).Add("label", networkFilter("default")), @@ -536,3 +543,10 @@ func TestDownHookContainerRemovalFailureIsNonFatal(t *testing.T) { err = tested.Down(t.Context(), strings.ToLower(testProject), compose.DownOptions{}) assert.NilError(t, err) } + +// runningOneOff builds a RUNNING `compose run` container of the given service. +func runningOneOff(service, id string) container.Summary { + c := testContainer(service, id, true) + c.State = container.StateRunning + return c +} diff --git a/pkg/compose/observed_state.go b/pkg/compose/observed_state.go index cfef7f3718..25d0e3da6c 100644 --- a/pkg/compose/observed_state.go +++ b/pkg/compose/observed_state.go @@ -151,9 +151,9 @@ func (s *composeService) collectObservedState(ctx context.Context, project *type } // --- Containers --- - // One-off (run) containers are included in the listing on purpose: exited - // one-offs are classified as orphans below, so `--remove-orphans` can - // clean them up. + // One-off (run) containers are included in the listing on purpose: + // FINISHED ones are classified as orphans below (see isOrphaned), so `up` + // can warn about them and `--remove-orphans` can clean them up. raw, err := s.getContainers(ctx, project.Name, oneOffInclude, true) if err != nil { return nil, err @@ -175,6 +175,10 @@ func (s *composeService) collectObservedState(ctx context.Context, project *type } else if isOrphaned(project)(ctr) { state.Orphans = append(state.Orphans, toObservedContainer(ctr)) } + // else: a still-RUNNING one-off. Deliberately absent from the + // observed state: it is somebody's live `compose run` session — up + // neither reconciles it, nor warns about it, nor removes it (only + // `down` stops running one-offs). } // --- Networks --- diff --git a/pkg/compose/observed_state_test.go b/pkg/compose/observed_state_test.go index 6005566485..cc24868de4 100644 --- a/pkg/compose/observed_state_test.go +++ b/pkg/compose/observed_state_test.go @@ -146,6 +146,20 @@ func TestCollectObservedState(t *testing.T) { api.OneoffLabel: "False", }, }, + { + // RUNNING one-off: somebody's live `compose run` session — + // deliberately dropped from the observed state (neither + // reconciled as a service replica, nor listed as orphan, so + // `up --remove-orphans` never kills it). + ID: "c4", + Names: []string{"/myproject-web-run-1"}, + State: container.StateRunning, + Labels: map[string]string{ + api.ServiceLabel: "web", + api.ProjectLabel: "myproject", + api.OneoffLabel: "True", + }, + }, }, }, nil) @@ -188,7 +202,9 @@ func TestCollectObservedState(t *testing.T) { assert.Equal(t, len(state.Containers["db"]), 1) assert.Equal(t, state.Containers["db"][0].ID, "c2") - // Orphan container (service "old" not in project) + // Orphans: only the model-absent service "old". The running one-off c4 is + // absent everywhere — not in the "web" bucket (asserted above: 1 replica), + // not an orphan: up leaves live `compose run` sessions alone. assert.Equal(t, len(state.Orphans), 1) assert.Equal(t, state.Orphans[0].ID, "c3")