From 2ae716faacd26a1372f1c36ee6965087d74871b5 Mon Sep 17 00:00:00 2001 From: Guillaume Lours Date: Tue, 29 Sep 2026 14:54:07 +0200 Subject: [PATCH] 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 --- pkg/compose/down.go | 55 ++++++++++++---------- pkg/compose/down_test.go | 66 ++++++++++++++++++++++++++ pkg/compose/image_pruner.go | 81 ++++++++++++++------------------ pkg/compose/image_pruner_test.go | 19 ++++---- 4 files changed, 142 insertions(+), 79 deletions(-) diff --git a/pkg/compose/down.go b/pkg/compose/down.go index cb3baeddb..51d9329f6 100644 --- a/pkg/compose/down.go +++ b/pkg/compose/down.go @@ -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 } diff --git a/pkg/compose/down_test.go b/pkg/compose/down_test.go index 7fdd1dd2e..fef8e0ac5 100644 --- a/pkg/compose/down_test.go +++ b/pkg/compose/down_test.go @@ -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 diff --git a/pkg/compose/image_pruner.go b/pkg/compose/image_pruner.go index 13629b97e..4243ce2c5 100644 --- a/pkg/compose/image_pruner.go +++ b/pkg/compose/image_pruner.go @@ -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 } diff --git a/pkg/compose/image_pruner_test.go b/pkg/compose/image_pruner_test.go index fff9c3068..a4c5062e8 100644 --- a/pkg/compose/image_pruner_test.go +++ b/pkg/compose/image_pruner_test.go @@ -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") }