Conversation
…hing to clean up `down --rmi` always printed "Dangling images ... Removed" even when there was nothing to remove, on every repeat `down` (docker#14219). The dangling-images removal now: stays silent when there's nothing eligible, same as removeNetwork's silent skip; shows a visible error instead of the misleading label when the listing itself fails, without aborting the rest of `down`; and keeps the "Removed" status visible alongside the real daemon error when some images fail to remove for a genuine reason (not a benign already-gone race). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Found during review of docker#14219: `ensureImagesDown` still listed tagged images to prune synchronously and returned the error immediately, before `down` had scheduled the already-built network/volume cleanup ops onto its errgroup — the exact same hazard just fixed for dangling images, on a different code path. The listing is now deferred to when its own op runs, so a failure surfaces as a visible error without blocking sibling ops. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The fix correctly defers dangling- and tagged-image listing into the ops phase so a listing failure can no longer abort sibling cleanup (networks, volumes). The new removeDanglingImagesOp silent-skip logic mirrors removeNetwork, and the removeImages helper in image_pruner.go properly bounds parallelism with eg.SetLimit(s.maxConcurrency). Test coverage is thorough and the logic is sound.
Lower-confidence findings (not posted inline)
- [low]
pkg/compose/down.go:217 — doc comment contradicts implementation: claims listing failure is reported "without the 'Dangling images' label" buterrorEvent(eventID, err.Error())at line 226 useseventID = "Dangling images"as the resource ID, so the label IS attached. The comment should say "under the 'Dangling images' label". (Documentation-only inconsistency; no behavioural impact. Not sent to verifier per policy for low-severity findings.)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ndeloof
left a comment
ndeloof
left a comment
There was a problem hiding this comment.
Reviewed the full diff, ran the pkg/compose suite (green) and the CI-pinned golangci-lint v2.13.2 (0 issues). The core is solid: the #14219 fix does exactly what the issue asks, the sibling hazard caught in ensureImagesDown is real and the signature change (no error return) locks the bug class structurally, and the five new tests pin the event stream with unusual precision.
One design point worth deciding explicitly (inline): tagged and dangling images now behave oppositely on the same daemon condition. A tagged image still in use goes through removeResource -> warning "Resource is still in use", error swallowed, down exits 0. A dangling image still in use is a hard error in removeImages -> the op fails -> down exits 1 (the PR body's manual test). Suggested tolerance inline; if the asymmetry is intended, a comment saying so would keep the next reader from "fixing" it either way.
Two smaller items with suggestions inline: the "Removed" label can currently show when nothing was actually removed, and the tagged-images fan-out is unbounded where removeImages is capped. Plus one note on a duplicated filter.
| if errdefs.IsNotFound(err) { | ||
| // already gone, e.g. removed concurrently by something else | ||
| logrus.Debugf("dangling image %s already removed: %v", img.ID, err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The asymmetry: a tagged image still in use ends in removeResource's IsConflict branch — visible warning, error swallowed, down exits 0 — while a dangling image still in use lands here as a hard error, failing the op and the whole down (exit 1). Same daemon condition, opposite outcomes depending on the tag. Suggest tolerating Conflict the way NotFound is tolerated (skipped, neither removed nor failed), aligning with the tagged path:
| if errdefs.IsNotFound(err) { | |
| // already gone, e.g. removed concurrently by something else | |
| logrus.Debugf("dangling image %s already removed: %v", img.ID, err) | |
| return nil | |
| } | |
| if errdefs.IsNotFound(err) { | |
| // already gone, e.g. removed concurrently by something else | |
| logrus.Debugf("dangling image %s already removed: %v", img.ID, err) | |
| return nil | |
| } | |
| if errdefs.IsConflict(err) { | |
| // still in use: 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) | |
| return nil | |
| } |
If the exit-1 behavior is the intended contract for dangling images, keeping it is fine too — but then a comment stating the asymmetry is deliberate would help.
| _, err = s.removeImages(ctx, eligible) | ||
| if err != nil { | ||
| s.events.On(newEvent(eventID, api.Warning, "Removed", err.Error())) | ||
| return err |
There was a problem hiding this comment.
The doc comment says "some images did get removed — and adds the error as a visible warning alongside it", but nothing guarantees that: on a total failure (every eligible image errors, none NotFound), removed is empty and the label still claims "Removed". removeImages already returns the removed list; using it keeps the label honest:
| _, err = s.removeImages(ctx, eligible) | |
| if err != nil { | |
| s.events.On(newEvent(eventID, api.Warning, "Removed", err.Error())) | |
| return err | |
| removed, err := s.removeImages(ctx, eligible) | |
| if err != nil { | |
| if len(removed) == 0 { | |
| // nothing was actually removed: report the failure without | |
| // claiming "Removed" | |
| s.events.On(errorEvent(eventID, err.Error())) | |
| } else { | |
| s.events.On(newEvent(eventID, api.Warning, "Removed", err.Error())) | |
| } | |
| return err | |
| } |
| return err | ||
| } | ||
|
|
||
| eg, ctx := errgroup.WithContext(ctx) |
There was a problem hiding this comment.
nit: removeImages caps its fan-out at s.maxConcurrency, but this one is unbounded (as the old per-image downOps were, so no regression — just an opportunity to harmonize while the code moves):
| eg, ctx := errgroup.WithContext(ctx) | |
| eg, ctx := errgroup.WithContext(ctx) | |
| eg.SetLimit(s.maxConcurrency) |
| return err | ||
| } | ||
|
|
||
| var eligible []image.Summary |
There was a problem hiding this comment.
nit: this listing + keep filter is now duplicated with removeDanglingImages (image_pruner.go), which does exactly the same dance for the watch path. A tiny shared helper (eligibleDanglingImages(ctx, projectName, keep)) would keep the two from drifting apart.
A dangling image still in use failed the whole op, while a tagged image or volume in the same state only gets a swallowed warning via removeResource — same daemon condition, inconsistent outcome. Dangling images now get the same tolerance. Also stop claiming "Removed" when a removal failure means nothing actually was, bound the tagged-image fan-out the same way the dangling one already is, and share the listing+filter logic between the down and watch paths instead of duplicating it. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since 67105b5ed48a.
Assessment: 🟡 NEEDS ATTENTION
One medium-confidence finding in the newly introduced errdefs.IsConflict branch.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since 67105b5ed48a.
Assessment: 🟡 NEEDS ATTENTION
One CONFIRMED medium-severity logic issue introduced in this incremental diff.
When a dangling-image batch had both a genuine removal error and an image tolerated as still in use, with nothing actually removed, the error path reported only the genuine failure and silently dropped the still-in-use count — the same information loss the "Removed" label was already fixed to avoid, just on its error-reporting sibling branch. Both facts now show up together regardless of which path the batch's outcome takes. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since d9eb9e3.
Assessment: 🟢 APPROVE
The incremental diff introduces a clean refactor of the dangling-image removal path:
image_pruner.go:removeImagesnow returns three separate buckets (removed,stillInUse,alreadyGone) instead of a single removed list, and the mutex is held for the entire error-handling block — safe and simpler than the prior pattern.down.go:removeDanglingImagesOpuses a well-structured switch over all outcome combinations, correctly guarding the Removed event behind both non-zeroremovedand zero-removed/stillInUsechecks. The prior review threads (false Removed on all-conflict paths) appear fully addressed.- Tests: New test cases accurately model each scenario, and the
errdefs.ErrConflict/errdefs.ErrNotFoundpatterns are consistent with the production checks.
No bugs introduced by the new + lines were found.
This branch has not been deployed
3 participants
What I did
docker compose down --rmi ...always printed a misleadingDangling images ... Removedline, even on a repeatdownwith nothing left to clean up.removeNetwork. A failure while checking for dangling images surfaces as a visible error instead of the misleading label, and no longer aborts the rest ofdown. A partial removal failure (a real daemon error, not a benign "already gone" race) keeps the "Removed" status visible alongside the actual error, instead of hiding it.ensureImagesDownalso listed tagged images to prune synchronously and returned early on failure, beforedownhad scheduled the already-built network/volume cleanup ops onto its errgroup. That listing is now deferred the same way, so a failure there can't block sibling cleanup either.Verified with
go test ./pkg/compose/...(full suite green, new coverage for both fixes),golangci-lint run --build-tags "e2e" ./pkg/compose/...(0 issues), and manually against a real Docker daemon: repeatdown --rmiwith nothing to remove stays silent (exit 0); a dangling image pinned by a running container surfaces the real conflict alongside "Removed" (exit 1) while network/containers still get cleaned up; a normaldown --rmion a built tagged image is unchanged.Related issue
fixes #14219
(not mandatory) A picture of a cute animal, if possible in relation to what you did
