fix: stop one tagged-image failure from starving the rest

removeTaggedImagesOp shared an errgroup.WithContext-derived ctx across every image removal goroutine. A genuine failure on one image canceled that ctx, so every image still queued behind it failed with "context canceled" instead of ever being attempted -- a regression from the independent per-image ops it replaced. Also extracts the Conflict/NotFound tolerance rule and the orphaned-image check into single shared helpers instead of leaving two independently maintained copies to drift apart, and drops a return bucket nothing outside its own test consumed. Signed-off-by: Guillaume Lours <[email protected]>
2026-10-12 02:13:08 +00:00 · 2026-09-29 16:19:33 +02:00
parent b3595868f6
commit 2ae716faac
4 changed files with 142 additions and 79 deletions
@@ -162,15 +162,11 @@ func (s *composeService) ensureImagesDown(ctx context.Context, project *types.Pr
 			return s.removeTaggedImagesOp(ctx, project, pruneOpts)
 		})

-		// 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.
+		// 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 (imageBelongsToKnownService is ImagesToPrune's own check).
 		keep := func(img image.Summary) bool {
-			if options.RemoveOrphans {
-				return false
-			}
-			_, err := project.GetService(img.Labels[api.ServiceLabel])
-			return err != nil
+			return !imageBelongsToKnownService(project, options.RemoveOrphans, img)
 		}
 		projectName := project.Name
 		ops = append(ops, func() error {
@@ -194,7 +190,7 @@ func (s *composeService) removeTaggedImagesOp(ctx context.Context, project *type
 		return err
 	}

-	eg, ctx := errgroup.WithContext(ctx)
+	var eg errgroup.Group
 	eg.SetLimit(s.maxConcurrency)
 	for i := range images {
 		img := images[i]
@@ -223,13 +219,11 @@ func (s *composeService) removeTaggedImagesOp(ctx context.Context, project *type
 // A removal failure keeps the "Removed" label only if at least one image
 // actually got removed — with the error visible alongside it, not
 // replacing it — otherwise it's reported as a plain error. removeImages
-// also tolerates images that are already gone or still in use without
-// that being an error, so a nil error here doesn't mean every image was
-// actually removed either: stillInUse (a real, not-yet-achieved goal) is
-// checked separately from alreadyGone (as good as removed), the same
-// distinction removeResource already makes per tagged image, so this
-// aggregate report doesn't collapse the two like removeImages' own return
-// values would if only `removed` were consulted.
+// also tolerates images that are already gone or still in use without that
+// being an error, so a nil error here doesn't mean every image was actually
+// removed either: stillInUse (a real, not-yet-achieved goal) is checked
+// separately, the same distinction removeResource already makes per tagged
+// image (an already-gone image simply isn't counted in either bucket).
 func (s *composeService) removeDanglingImagesOp(ctx context.Context, projectName string, keep func(image.Summary) bool) error {
 	eventID := "Dangling images"
 	eligible, err := s.eligibleDanglingImages(ctx, projectName, keep)
@@ -242,12 +236,16 @@ func (s *composeService) removeDanglingImagesOp(ctx context.Context, projectName
 	}

 	s.events.On(removingEvent(eventID))
-	removed, stillInUse, _, err := s.removeImages(ctx, eligible)
+	removed, stillInUse, err := s.removeImages(ctx, eligible)
+	var stillInUseNote string
+	if len(stillInUse) > 0 {
+		stillInUseNote = fmt.Sprintf("%d image(s) still in use", len(stillInUse))
+	}
 	switch {
 	case err != nil:
 		details := err.Error()
-		if len(stillInUse) > 0 {
-			details = fmt.Sprintf("%s; %d image(s) still in use", details, len(stillInUse))
+		if stillInUseNote != "" {
+			details = fmt.Sprintf("%s; %s", details, stillInUseNote)
 		}
 		if len(removed) == 0 {
 			s.events.On(errorEvent(eventID, details))
@@ -255,10 +253,9 @@ func (s *composeService) removeDanglingImagesOp(ctx context.Context, projectName
 			s.events.On(newEvent(eventID, api.Warning, "Removed", details))
 		}
 	case len(stillInUse) > 0 && len(removed) == 0:
-		s.events.On(newEvent(eventID, api.Warning, "Resource is still in use"))
+		s.events.On(newEvent(eventID, api.Warning, "Resource is still in use", stillInUseNote))
 	case len(stillInUse) > 0:
-		s.events.On(newEvent(eventID, api.Warning, "Removed",
-			fmt.Sprintf("%d image(s) still in use", len(stillInUse))))
+		s.events.On(newEvent(eventID, api.Warning, "Removed", stillInUseNote))
 	case len(removed) == 0:
 		// every eligible image was already gone by the time we got to it:
 		// we already emitted "Removing", so this needs a terminal event
@@ -361,6 +358,15 @@ func (s *composeService) removeVolume(ctx context.Context, id string) error {
 	})
 }

+// classifyRemovalError reports whether a resource-removal error is a benign
+// "still in use" (Conflict) or "already gone" (NotFound) condition, as
+// opposed to a genuine failure that must be surfaced. Shared by
+// removeResource (single resource) and removeImages (a batch), so the two
+// can't independently drift on what counts as tolerable.
+func classifyRemovalError(err error) (stillInUse, alreadyGone bool) {
+	return errdefs.IsConflict(err), errdefs.IsNotFound(err)
+}
+
 // removeResource emits a "Removing" progress event, calls op, then emits the appropriate
 // completion event based on the error: nil→Removed, conflict→still-in-use warning, not-found→gone warning.
 func (s *composeService) removeResource(eventID string, op func() error) error {
@@ -370,11 +376,12 @@ func (s *composeService) removeResource(eventID string, op func() error) error {
 		s.events.On(newEvent(eventID, api.Done, "Removed"))
 		return nil
 	}
-	if errdefs.IsConflict(err) {
+	stillInUse, alreadyGone := classifyRemovalError(err)
+	if stillInUse {
 		s.events.On(newEvent(eventID, api.Warning, "Resource is still in use"))
 		return nil
 	}
-	if errdefs.IsNotFound(err) {
+	if alreadyGone {
 		s.events.On(newEvent(eventID, api.Done, "Warning: No resource found to remove"))
 		return nil
 	}
@@ -17,6 +17,7 @@
 package compose

 import (
+	"context"
 	"errors"
 	"fmt"
 	"os"
@@ -510,6 +511,70 @@ func TestEnsureImagesDown_TaggedImageListingFailureStaysVisibleWithoutAbortingDo
 	assert.Equal(t, rec.resources[0].Details, listErr.Error())
 }

+// liveCtx matches a context.Context argument only if it hasn't been
+// canceled, to catch a shared errgroup ctx getting canceled by a sibling
+// goroutine's failure and silently masking later calls that would run fine
+// with an unmocked API client (which does check ctx and would fail them
+// with "context canceled" instead of ever reaching the daemon).
+type liveCtx struct{}
+
+func (liveCtx) Matches(x any) bool {
+	ctx, ok := x.(context.Context)
+	return ok && ctx.Err() == nil
+}
+
+func (liveCtx) String() string {
+	return "is a non-canceled context"
+}
+
+// TestRemoveTaggedImagesOp_ContinuesAfterOneImageFails guards a regression
+// caught in review: removeTaggedImagesOp used to share its errgroup's
+// derived ctx with every image removal via errgroup.WithContext, so a real
+// failure on one image canceled that ctx and made every image still queued
+// behind it fail with "context canceled" instead of actually being
+// attempted, unlike the independent per-image ops it replaced.
+func TestRemoveTaggedImagesOp_ContinuesAfterOneImageFails(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	apiClient, cli := prepareMocks(mockCtrl)
+	rec := &capturingEvents{}
+	svcIface, err := NewComposeService(cli, WithEventProcessor(rec), WithMaxConcurrency(1))
+	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{Items: []image.Summary{
+		{ID: "sha256:aaa", RepoTags: []string{"repo/a:latest"}},
+		{ID: "sha256:bbb", RepoTags: []string{"repo/b:latest"}},
+		{ID: "sha256:ccc", RepoTags: []string{"repo/c:latest"}},
+	}}, nil)
+	daemonErr := errors.New("i/o timeout")
+	apiClient.EXPECT().ImageRemove(gomock.Any(), "repo/a:latest", client.ImageRemoveOptions{}).
+		Return(client.ImageRemoveResult{}, daemonErr)
+	apiClient.EXPECT().ImageRemove(liveCtx{}, "repo/b:latest", client.ImageRemoveOptions{}).
+		Return(client.ImageRemoveResult{}, nil)
+	apiClient.EXPECT().ImageRemove(liveCtx{}, "repo/c:latest", client.ImageRemoveOptions{}).
+		Return(client.ImageRemoveResult{}, nil)
+
+	opErr := svc.removeTaggedImagesOp(t.Context(), project, ImagePruneOptions{Mode: ImagePruneLocal, RemoveOrphans: true})
+	assert.ErrorContains(t, opErr, daemonErr.Error())
+
+	events := make([]string, len(rec.resources))
+	for i, e := range rec.resources {
+		events[i] = e.ID + ": " + e.Text
+	}
+	assert.DeepEqual(t, events, []string{
+		"Image repo/a:latest: Removing",
+		"Image repo/b:latest: Removing",
+		"Image repo/b:latest: Removed",
+		"Image repo/c:latest: Removing",
+		"Image repo/c:latest: Removed",
+	})
+}
+
 // 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.
@@ -860,6 +925,7 @@ func TestEnsureImagesDown_AllEligibleStillInUseDoesNotClaimRemoved(t *testing.T)
 	assert.Equal(t, rec.resources[1].ID, "Dangling images")
 	assert.Equal(t, rec.resources[1].Status, compose.Warning)
 	assert.Equal(t, rec.resources[1].Text, "Resource is still in use")
+	assert.Equal(t, rec.resources[1].Details, "1 image(s) still in use")
 }

 // TestEnsureImagesDown_MixedRemovedAndStillInUseKeepsBothVisible guards a
@@ -72,6 +72,21 @@ func NewImagePruner(imageClient client.ImageAPIClient, project *types.Project) *
 	}
 }

+// imageBelongsToKnownService reports whether img should be pruned: either
+// removeOrphans is set (indiscriminately prune all project images even if
+// they're not referenced by the current Compose state, e.g. the service was
+// removed from YAML), or the image belongs to a service still declared in
+// project. Shared by ImagesToPrune (tagged images) and down's own
+// dangling-image keep check, so the two can't drift on what counts as
+// orphaned — the image-side analog of containers.go's isOrphaned.
+func imageBelongsToKnownService(project *types.Project, removeOrphans bool, img image.Summary) bool {
+	if removeOrphans {
+		return true
+	}
+	_, err := project.GetService(img.Labels[api.ServiceLabel])
+	return err == nil
+}
+
 // ImagesToPrune returns the set of images that should be removed.
 func (p *ImagePruner) ImagesToPrune(ctx context.Context, opts ImagePruneOptions) ([]string, error) {
 	if opts.Mode == ImagePruneNone {
@@ -101,20 +116,7 @@ func (p *ImagePruner) ImagesToPrune(ctx context.Context, opts ImagePruneOptions)
 			continue
 		}

-		var shouldPrune bool
-		if opts.RemoveOrphans {
-			// indiscriminately prune all project images even if they're not
-			// referenced by the current Compose state (e.g. the service was
-			// removed from YAML)
-			shouldPrune = true
-		} else {
-			// only prune the image if it belongs to a known service for the project.
-			if _, err := p.project.GetService(img.Labels[api.ServiceLabel]); err == nil {
-				shouldPrune = true
-			}
-		}
-
-		if shouldPrune {
+		if imageBelongsToKnownService(p.project, opts.RemoveOrphans, img) {
 			images = append(images, img.RepoTags[0])
 		}
 	}
@@ -160,32 +162,19 @@ func (p *ImagePruner) labeledLocalImages(ctx context.Context) ([]image.Summary,
 	return res.Items, nil
 }

-// danglingImages lists a project's dangling images, with no removal or
-// filtering — split out so a caller can decide whether there's anything to
-// do before committing to a removal attempt, without listing twice.
-func (s *composeService) danglingImages(ctx context.Context, projectName string) ([]image.Summary, error) {
-	res, err := s.apiClient().ImageList(ctx, client.ImageListOptions{
-		Filters: projectFilter(projectName).Add("dangling", "true"),
-	})
-	if err != nil {
-		return nil, err
-	}
-	return res.Items, nil
-}
-
 // removeImages removes the given images in parallel, tolerating individual
 // failures so one bad image doesn't abort the rest. An image already gone
 // (a benign race with something else removing it concurrently) or still in
 // use is never joined into err — same tolerance removeResource already
 // gives a tagged image in either situation (its own distinct, correctly
-// worded event, not an error) — but the two are returned in separate
-// buckets, not folded into removed: "already gone" reached the goal same
-// as an actual removal, "still in use" did not, and a caller aggregating
-// several images needs to tell those apart to report the batch honestly
-// (see removeDanglingImagesOp). Any other removal error is joined into err
-// so callers reporting failures to the user keep the actual daemon error
-// instead of just an image ID.
-func (s *composeService) removeImages(ctx context.Context, images []image.Summary) (removed, stillInUse, alreadyGone []string, err error) {
+// worded event, not an error). "Still in use" is returned in its own bucket,
+// not folded into removed, since a caller aggregating several images (see
+// removeDanglingImagesOp) needs that distinction to report the batch
+// honestly; an already-gone image reached the goal the same as an actual
+// removal, so it's simply not counted against either bucket. Any other
+// removal error is joined into err so callers reporting failures to the
+// user keep the actual daemon error instead of just an image ID.
+func (s *composeService) removeImages(ctx context.Context, images []image.Summary) (removed, stillInUse []string, err error) {
 	var mu sync.Mutex
 	var errs []error
 	eg, ctx := errgroup.WithContext(ctx)
@@ -195,14 +184,14 @@ func (s *composeService) removeImages(ctx context.Context, images []image.Summar
 			if _, err := s.apiClient().ImageRemove(ctx, img.ID, client.ImageRemoveOptions{}); err != nil {
 				mu.Lock()
 				defer mu.Unlock()
+				imgStillInUse, alreadyGone := classifyRemovalError(err)
 				switch {
-				case errdefs.IsNotFound(err):
-					// already gone, e.g. removed concurrently by something else
+				case alreadyGone:
+					// e.g. removed concurrently by something else
 					logrus.Debugf("dangling image %s already removed: %v", img.ID, err)
-					alreadyGone = append(alreadyGone, img.ID)
-				case errdefs.IsConflict(err):
-					// still in use: the same benign skip the tagged-image
-					// path reports via removeResource's conflict branch
+				case imgStillInUse:
+					// the same benign skip the tagged-image path reports via
+					// removeResource's conflict branch
 					logrus.Debugf("dangling image %s still in use: %v", img.ID, err)
 					stillInUse = append(stillInUse, img.ID)
 				default:
@@ -217,7 +206,7 @@ func (s *composeService) removeImages(ctx context.Context, images []image.Summar
 		})
 	}
 	_ = eg.Wait() // errgroup is only used for fan-out here; goroutines never return an error
-	return removed, stillInUse, alreadyGone, errors.Join(errs...)
+	return removed, stillInUse, errors.Join(errs...)
 }

 // eligibleDanglingImages lists a project's dangling images and filters out
@@ -225,13 +214,15 @@ func (s *composeService) removeImages(ctx context.Context, images []image.Summar
 // itself, to decide whether there's anything to report) and watch --prune,
 // so the two don't drift apart.
 func (s *composeService) eligibleDanglingImages(ctx context.Context, projectName string, keep func(image.Summary) bool) ([]image.Summary, error) {
-	images, err := s.danglingImages(ctx, projectName)
+	res, err := s.apiClient().ImageList(ctx, client.ImageListOptions{
+		Filters: projectFilter(projectName).Add("dangling", "true"),
+	})
 	if err != nil {
 		return nil, err
 	}

 	var eligible []image.Summary
-	for _, img := range images {
+	for _, img := range res.Items {
 		if !keep(img) {
 			eligible = append(eligible, img)
 		}
@@ -247,7 +238,7 @@ func (s *composeService) removeDanglingImages(ctx context.Context, projectName s
 	if err != nil {
 		return nil, err
 	}
-	removed, _, _, err := s.removeImages(ctx, eligible)
+	removed, _, err := s.removeImages(ctx, eligible)
 	return removed, err
 }

@@ -70,14 +70,14 @@ func TestRemoveDanglingImages_FiltersKeepsAndToleratesFailures(t *testing.T) {
 	assert.ErrorContains(t, err, "permission denied")
 }

-// TestRemoveImages_BucketsAlreadyGoneSeparatelyFromStillInUse guards a
-// regression caught by review: an image already gone (goal achieved) and
-// an image still in use (goal NOT achieved) must land in distinct return
-// buckets, not be folded together — a caller aggregating several images
-// (removeDanglingImagesOp) needs the distinction to report the batch
-// honestly instead of collapsing both into "removed" or into one generic
-// tolerance.
-func TestRemoveImages_BucketsAlreadyGoneSeparatelyFromStillInUse(t *testing.T) {
+// TestRemoveImages_BucketsStillInUseSeparatelyFromRemoved guards a
+// regression caught by review: an image still in use (goal NOT achieved)
+// must land in its own return bucket, not be folded into removed — a caller
+// aggregating several images (removeDanglingImagesOp) needs the distinction
+// to report the batch honestly. An already-gone image reaches the goal like
+// an actual removal and a genuine failure is joined into err, so neither is
+// exercised here beyond not corrupting the removed/stillInUse buckets.
+func TestRemoveImages_BucketsStillInUseSeparatelyFromRemoved(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()

@@ -101,10 +101,9 @@ func TestRemoveImages_BucketsAlreadyGoneSeparatelyFromStillInUse(t *testing.T) {
 	apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:fails", client.ImageRemoveOptions{}).
 		Return(client.ImageRemoveResult{}, errdefs.ErrPermissionDenied.WithMessage("permission denied"))

-	removed, stillInUse, alreadyGone, err := svc.removeImages(t.Context(), images)
+	removed, stillInUse, err := svc.removeImages(t.Context(), images)
 	assert.DeepEqual(t, removed, []string{"sha256:removed"})
 	assert.DeepEqual(t, stillInUse, []string{"sha256:in-use"})
-	assert.DeepEqual(t, alreadyGone, []string{"sha256:already-gone"})
 	assert.ErrorContains(t, err, "sha256:fails")
 	assert.ErrorContains(t, err, "permission denied")
 }