fix(down): spare dangling images of orphaned services, like their tagged image

`down --rmi` removed every dangling image for the project unconditionally, while its tagged-image counterpart (ImagesToPrune) already skips a service no longer in the project unless --remove-orphans is set. `down --rmi local` on a project with an orphaned service therefore kept the tagged image but deleted its dangling ones - an inconsistent result for one command. Extract the "list a project's dangling images, remove what a keep predicate doesn't spare, tolerate individual failures" logic - which down.go and watch.go's pruneDanglingImagesOnRebuild had each partially and divergently implemented - into one shared composeService.removeDanglingImages, parallelized via errgroup like the rest of the codebase's bulk removals. down.go's keep now mirrors ImagesToPrune's own RemoveOrphans/GetService check; watch.go keeps its existing built-IDs check, behavior unchanged. Signed-off-by: hiroto.toyoda <[email protected]>
2026-10-12 02:13:08 +00:00 · 2026-09-03 16:00:29 +02:00
parent a896529193
commit a966bad33a
5 changed files with 272 additions and 21 deletions
@@ -25,6 +25,7 @@ import (
 	"github.com/compose-spec/compose-go/v2/types"
 	"github.com/containerd/errdefs"
 	containerType "github.com/moby/moby/api/types/container"
+	"github.com/moby/moby/api/types/image"
 	"github.com/moby/moby/client"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sync/errgroup"
@@ -180,6 +181,25 @@ func (s *composeService) ensureImagesDown(ctx context.Context, project *types.Pr
 			})
 		})
 	}
+
+	if pruneOpts.Mode != ImagePruneNone {
+		// mirrors ImagesToPrune's own orphan check: a dangling image from a
+		// service no longer in the project must be spared unless
+		// RemoveOrphans is set, same as that service's tagged image is.
+		keep := func(img image.Summary) bool {
+			if options.RemoveOrphans {
+				return false
+			}
+			_, err := project.GetService(img.Labels[api.ServiceLabel])
+			return err != nil
+		}
+		ops = append(ops, func() error {
+			return s.removeResource("Dangling images", func() error {
+				_, err := s.removeDanglingImages(ctx, project.Name, keep)
+				return err
+			})
+		})
+	}
 	return ops, nil
 }

@@ -330,6 +330,10 @@ func TestDownRemoveImages(t *testing.T) {
 		},
 	}}, nil).AnyTimes()

+	api.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{
+		Filters: projectFilter(strings.ToLower(testProject)).Add("dangling", "true"),
+	}).Return(client.ImageListResult{}, nil).AnyTimes()
+
 	imagesToBeInspected := map[string]bool{
 		"testproject-local-anonymous":     true,
 		"local-named-image":               true,
@@ -420,6 +424,10 @@ func TestDownRemoveImages_NoLabel(t *testing.T) {
 		Filters: projectFilter(strings.ToLower(testProject)).Add("dangling", "false"),
 	}).Return(client.ImageListResult{}, nil)

+	api.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{
+		Filters: projectFilter(strings.ToLower(testProject)).Add("dangling", "true"),
+	}).Return(client.ImageListResult{}, nil)
+
 	api.EXPECT().ImageInspect(gomock.Any(), "testproject-service1", gomock.Any()).Return(client.ImageInspectResult{}, nil)
 	api.EXPECT().ContainerStop(gomock.Any(), "123", client.ContainerStopOptions{}).Return(client.ContainerStopResult{}, nil)
 	api.EXPECT().ContainerRemove(gomock.Any(), "123", client.ContainerRemoveOptions{Force: true}).Return(client.ContainerRemoveResult{}, nil)
@@ -459,6 +467,129 @@ func prepareMocks(mockCtrl *gomock.Controller) (*mocks.MockAPIClient, *mocks.Moc
 	return api, cli
 }

+// TestEnsureImagesDown_ReportsDanglingImagesAsOneGroupedEvent guards that
+// dangling-image removal is reported as a single grouped event regardless
+// of count, instead of one row per meaningless raw image ID.
+func TestEnsureImagesDown_ReportsDanglingImagesAsOneGroupedEvent(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	apiClient, cli := prepareMocks(mockCtrl)
+	rec := &capturingEvents{}
+	svcIface, err := NewComposeService(cli, WithEventProcessor(rec))
+	assert.NilError(t, err)
+	svc := svcIface.(*composeService)
+
+	project := &types.Project{Name: "prj"}
+	apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{
+		Filters: projectFilter("prj").Add("dangling", "false"),
+	}).Return(client.ImageListResult{}, nil)
+	apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{
+		Filters: projectFilter("prj").Add("dangling", "true"),
+	}).Return(client.ImageListResult{Items: []image.Summary{
+		{ID: "sha256:aaa"},
+		{ID: "sha256:bbb"},
+	}}, nil)
+	apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:aaa", client.ImageRemoveOptions{}).
+		Return(client.ImageRemoveResult{}, nil)
+	apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:bbb", client.ImageRemoveOptions{}).
+		Return(client.ImageRemoveResult{}, errdefs.ErrNotFound.WithMessage("already removed"))
+
+	// RemoveOrphans:true bypasses the per-service keep check so this test
+	// stays focused on the single-grouped-event behavior.
+	ops, err := svc.ensureImagesDown(t.Context(), project, compose.DownOptions{Images: "local", RemoveOrphans: true})
+	assert.NilError(t, err)
+	for _, op := range ops {
+		assert.NilError(t, op())
+	}
+
+	events := make([]string, len(rec.resources))
+	for i, e := range rec.resources {
+		events[i] = e.ID + ": " + e.Text
+	}
+	assert.DeepEqual(t, events, []string{
+		"Dangling images: Removing",
+		"Dangling images: Removed",
+	})
+}
+
+// TestEnsureImagesDown_SparesDanglingImagesOfOrphanedServices guards a bug
+// caught in review: ImagesToPrune already spares a service's tagged image
+// when the service is no longer in the project and RemoveOrphans isn't set;
+// its dangling images must be spared the same way, or `down --rmi` leaves
+// an inconsistent result (tagged image kept, dangling image gone).
+func TestEnsureImagesDown_SparesDanglingImagesOfOrphanedServices(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	apiClient, cli := prepareMocks(mockCtrl)
+	tested, err := NewComposeService(cli)
+	assert.NilError(t, err)
+	svc := tested.(*composeService)
+
+	project := &types.Project{
+		Name: "prj",
+		Services: types.Services{
+			"web": {Name: "web", Image: "web-image"},
+		},
+	}
+	apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{
+		Filters: projectFilter("prj").Add("dangling", "false"),
+	}).Return(client.ImageListResult{}, nil)
+	apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{
+		Filters: projectFilter("prj").Add("dangling", "true"),
+	}).Return(client.ImageListResult{Items: []image.Summary{
+		{ID: "sha256:web-dangling", Labels: types.Labels{compose.ServiceLabel: "web"}},
+		{ID: "sha256:orphan-dangling", Labels: types.Labels{compose.ServiceLabel: "orphan"}},
+	}}, nil)
+	// only the known service's dangling image may be removed; a call for
+	// the orphaned one is an unexpected call and fails the test
+	apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:web-dangling", client.ImageRemoveOptions{}).
+		Return(client.ImageRemoveResult{}, nil)
+
+	ops, err := svc.ensureImagesDown(t.Context(), project, compose.DownOptions{Images: "local"})
+	assert.NilError(t, err)
+	for _, op := range ops {
+		assert.NilError(t, op())
+	}
+}
+
+// TestEnsureImagesDown_RemoveOrphansAlsoTakesDanglingImages guards that
+// --remove-orphans overrides the spare-orphans behavior for dangling
+// images too, matching what it already does for tagged images.
+func TestEnsureImagesDown_RemoveOrphansAlsoTakesDanglingImages(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	apiClient, cli := prepareMocks(mockCtrl)
+	tested, err := NewComposeService(cli)
+	assert.NilError(t, err)
+	svc := tested.(*composeService)
+
+	project := &types.Project{
+		Name: "prj",
+		Services: types.Services{
+			"web": {Name: "web", Image: "web-image"},
+		},
+	}
+	apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{
+		Filters: projectFilter("prj").Add("dangling", "false"),
+	}).Return(client.ImageListResult{}, nil)
+	apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{
+		Filters: projectFilter("prj").Add("dangling", "true"),
+	}).Return(client.ImageListResult{Items: []image.Summary{
+		{ID: "sha256:orphan-dangling", Labels: types.Labels{compose.ServiceLabel: "orphan"}},
+	}}, nil)
+	apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:orphan-dangling", client.ImageRemoveOptions{}).
+		Return(client.ImageRemoveResult{}, nil)
+
+	ops, err := svc.ensureImagesDown(t.Context(), project, compose.DownOptions{Images: "local", RemoveOrphans: true})
+	assert.NilError(t, err)
+	for _, op := range ops {
+		assert.NilError(t, op())
+	}
+}
+
 // TestDownRemovesRetainedPreStartHookContainers verifies that compose down finds and
 // removes pre_start hook containers that were retained after a failed hook run.
 // These containers lack ConfigHashLabel so the normal getContainers path never sees them.
@@ -27,6 +27,7 @@ import (
 	"github.com/distribution/reference"
 	"github.com/moby/moby/api/types/image"
 	"github.com/moby/moby/client"
+	"github.com/sirupsen/logrus"
 	"golang.org/x/sync/errgroup"

 	"github.com/docker/compose/v5/pkg/api"
@@ -150,12 +151,6 @@ func (p *ImagePruner) namedImages(ctx context.Context) ([]string, error) {
 // created from the project + service name.
 func (p *ImagePruner) labeledLocalImages(ctx context.Context) ([]image.Summary, error) {
 	res, err := p.client.ImageList(ctx, client.ImageListOptions{
-		// TODO(milas): we should really clean up the dangling images as
-		// well (historically we have NOT); need to refactor this to handle
-		// it gracefully without producing confusing CLI output, i.e. we
-		// do not want to print out a bunch of untagged/dangling image IDs,
-		// they should be grouped into a logical operation for the relevant
-		// service
 		Filters: projectFilter(p.project.Name).Add("dangling", "false"),
 	})
 	if err != nil {
@@ -164,6 +159,40 @@ func (p *ImagePruner) labeledLocalImages(ctx context.Context) ([]image.Summary,
 	return res.Items, nil
 }

+// removeDanglingImages removes a project's dangling images not spared by
+// keep, in parallel, tolerating individual failures so one bad image
+// doesn't abort the rest. Shared by down --rmi and watch --prune, which
+// differ only in what keep spares.
+func (s *composeService) removeDanglingImages(ctx context.Context, projectName string, keep func(image.Summary) bool) ([]string, error) {
+	res, err := s.apiClient().ImageList(ctx, client.ImageListOptions{
+		Filters: projectFilter(projectName).Add("dangling", "true"),
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	var mu sync.Mutex
+	var removed []string
+	eg, ctx := errgroup.WithContext(ctx)
+	for _, img := range res.Items {
+		if keep(img) {
+			continue
+		}
+		eg.Go(func() error {
+			if _, err := s.apiClient().ImageRemove(ctx, img.ID, client.ImageRemoveOptions{}); err != nil {
+				logrus.Debugf("failed to remove dangling image %s: %v", img.ID, err)
+				return nil
+			}
+			mu.Lock()
+			defer mu.Unlock()
+			removed = append(removed, img.ID)
+			return nil
+		})
+	}
+	_ = eg.Wait() // errgroup is only used for fan-out here; goroutines never return an error
+	return removed, nil
+}
+
 // unlabeledLocalImages are images that match the implicit naming convention
 // for locally-built images but did not get labeled, presumably because they
 // were produced by an older version of Compose.
@@ -0,0 +1,79 @@
+/*
+   Copyright 2026 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/containerd/errdefs"
+	"github.com/moby/moby/api/types/image"
+	"github.com/moby/moby/client"
+	"go.uber.org/mock/gomock"
+	"gotest.tools/v3/assert"
+)
+
+// TestRemoveDanglingImages_FiltersKeepsAndToleratesFailures guards the
+// shared helper used by both `down --rmi` and `watch --prune`: images the
+// keep predicate spares must never be removed, and one failed removal must
+// not stop the others or propagate as an error.
+func TestRemoveDanglingImages_FiltersKeepsAndToleratesFailures(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	apiClient, cli := prepareMocks(mockCtrl)
+	svcIface, err := NewComposeService(cli)
+	assert.NilError(t, err)
+	svc := svcIface.(*composeService)
+
+	apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{
+		Filters: projectFilter("prj").Add("dangling", "true"),
+	}).Return(client.ImageListResult{Items: []image.Summary{
+		{ID: "sha256:keep"},
+		{ID: "sha256:removed"},
+		{ID: "sha256:fails"},
+	}}, nil)
+	apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:removed", client.ImageRemoveOptions{}).
+		Return(client.ImageRemoveResult{}, nil)
+	apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:fails", client.ImageRemoveOptions{}).
+		Return(client.ImageRemoveResult{}, errdefs.ErrNotFound.WithMessage("already removed"))
+	// no expectation for "sha256:keep" — a call to ImageRemove for it fails the test
+
+	keep := func(img image.Summary) bool { return img.ID == "sha256:keep" }
+	removed, err := svc.removeDanglingImages(t.Context(), "prj", keep)
+	assert.NilError(t, err)
+	assert.DeepEqual(t, removed, []string{"sha256:removed"})
+}
+
+// TestRemoveDanglingImages_NoneFound guards that an empty dangling-image
+// list is a no-op: no removal calls, no error.
+func TestRemoveDanglingImages_NoneFound(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	apiClient, cli := prepareMocks(mockCtrl)
+	svcIface, err := NewComposeService(cli)
+	assert.NilError(t, err)
+	svc := svcIface.(*composeService)
+
+	apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{
+		Filters: projectFilter("prj").Add("dangling", "true"),
+	}).Return(client.ImageListResult{}, nil)
+
+	removed, err := svc.removeDanglingImages(t.Context(), "prj", func(image.Summary) bool { return false })
+	assert.NilError(t, err)
+	assert.Equal(t, len(removed), 0)
+}
@@ -36,6 +36,7 @@ import (
 	"github.com/go-viper/mapstructure/v2"
 	"github.com/moby/buildkit/util/progress/progressui"
 	"github.com/moby/moby/api/types/container"
+	"github.com/moby/moby/api/types/image"
 	"github.com/moby/moby/client"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sync/errgroup"
@@ -744,27 +745,18 @@ func writeWatchSyncMessage(log api.LogConsumer, serviceName string, pathMappings
 }

 func (s *composeService) pruneDanglingImagesOnRebuild(ctx context.Context, projectName string, imageNameToIdMap map[string]string) {
-	images, err := s.apiClient().ImageList(ctx, client.ImageListOptions{
-		Filters: projectFilter(projectName).Add("dangling", "true"),
-	})
-	if err != nil {
-		logrus.Debugf("Failed to list images: %v", err)
-		return
-	}
-
 	// imageNameToIdMap is keyed by image name; the freshly built images to
 	// spare are its VALUES (image IDs), matched against the dangling IDs
 	builtIDs := make(map[string]struct{}, len(imageNameToIdMap))
 	for _, id := range imageNameToIdMap {
 		builtIDs[id] = struct{}{}
 	}
-	for _, img := range images.Items {
-		if _, ok := builtIDs[img.ID]; !ok {
-			_, err := s.apiClient().ImageRemove(ctx, img.ID, client.ImageRemoveOptions{})
-			if err != nil {
-				logrus.Debugf("Failed to remove image %s: %v", img.ID, err)
-			}
-		}
+	keep := func(img image.Summary) bool {
+		_, ok := builtIDs[img.ID]
+		return ok
+	}
+	if _, err := s.removeDanglingImages(ctx, projectName, keep); err != nil {
+		logrus.Debugf("Failed to list images: %v", err)
 	}
 }