fix(delete): don't report an image reclaim that didn't happen (backend#1861) - #495
Merged
Merged
Conversation
…d#1861) `tracebloc delete`'s image reclaim printed "Reclaimed tracebloc's downloaded images." whether or not it removed anything. On every install except a Windows GPU host it removes nothing, so an operator offboarding a machine was told disk was freed when the step was a no-op. The reported cause was that the scope constant names the wrong registry: our chart images ship from Docker Hub, not ghcr. Verified against the producers, the scope turns out to be right and the explanation around it wrong. `docker images` reads the HOST daemon. Every chart image, the ingestor Job and the training images are pulled by containerd INSIDE the k3d node — nothing in the installer does a host `docker pull` or `k3d image import` for them — so the host daemon never lists them, and `k3d cluster delete` (step 3, before this one) reclaims them with the node container. The one tracebloc image that does reach the host daemon is the GPU node image the Windows GPU installer pulls, which `ghcr.io/tracebloc/*` matches. Widening the pattern would also be inert or harmful: `docker.io/tracebloc/*` matches nothing (the daemon stores Hub images under their short name), and `tracebloc/*` would reach a developer's locally built images. So the pattern stays; the comment, the test and the success message are fixed. - imageReference: replace the wrong premise with the host-daemon-vs-node- containerd reason, why widening is wrong in both directions, and the mirror/air-gapped gap it cannot cover. - PruneImages returns the number of references removed. A failed `rmi` returns 0 — nothing is claimed reclaimed when the removal didn't succeed. - delete.go says "No tracebloc images left to reclaim." on 0, and reports the count otherwise. The preview line is unchanged: the offboard does remove the machine's images, via the cluster teardown. - Tests: fixtures are names a producer actually publishes, replacing ghcr.io/tracebloc/jobs-manager:1.9.5, which no producer has ever written — that fixture is why the scope went unexamined. New TestImageReferenceScope asserts the scope against producer facts in both directions instead of asserting the constant back at itself. - RFC-0001 §7.10: correct which step reclaims the chart images.
saadqbal
approved these changes
Aug 13, 2026
saadqbal
left a comment
Collaborator
There was a problem hiding this comment.
Nice, careful PR 👍 The reported cause was chased down to the real one — host daemon vs node containerd — and the fix lands where it belongs: reporting, not the scope constant. Reclaimed N only fires when rmi actually succeeded ((0,err) on failure, (0,nil) on no-op), the no-op path tells the truth, and the new tests cover all three. Good call pinning the scope to producer names in both directions instead of asserting the constant back at itself.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
tracebloc delete's image reclaim printed "Reclaimed tracebloc's downloaded images." whether or not it removed anything. On every install except a Windows GPU host it removes nothing, so an operator offboarding a machine was told disk was freed when the step was a total no-op.The reported cause was wrong — the pattern is right, the story around it wasn't
The finding was that the scope constant names the wrong registry:
imageReference = "ghcr.io/tracebloc/*", while the chart's tracebloc images ship from Docker Hub. The registry facts check out, but the conclusion doesn't.docker imagesreads the host daemon, and the chart's images are never in it:tracebloc/jobs-manager,pods-monitor,resource-monitor,mysql-client), the ingestor Job, and the spawned training images are pulled by containerd inside the k3d node. Nothing in the installer does a hostdocker pullor ak3d image importfor them, sodocker imageson the host never lists them.k3d cluster delete— step 3 of the offboard, which runs before this one and takes the node container's image store with it.docker pull ghcr.io/tracebloc/k3s-cuda:<k3s>-cuda-<cuda-base>, and a localdocker buildunder the same name as a fallback).ghcr.io/tracebloc/*matches it.So the pattern is narrowly correct. The comment, the test, and the success message are what were wrong, and this PR fixes those rather than the constant.
Why widening the pattern would be a mistake — blast radius
Both wider forms were checked against a real daemon holding four Hub
tracebloc/*images:docker.io/tracebloc/*would be inert — a second no-op. The daemon stores Hub images under their short name, so onlytracebloc/*matches them.tracebloc/*is the only form that matches, and all four images above are locally built dev images the installer never pulled. Widening would maketracebloc deletesilently destroy a developer's own builds — real harm, and zero gain, since the host daemon holds no chart image to reclaim in the first place.Nothing is newly removed by this PR. The scope is byte-for-byte unchanged; only reporting, docs and tests change.
Changes
imageReferencecomment — replaces the wrong premise ("the ghcr namespace the installer pulls tracebloc images from") with the host-daemon-vs-node-containerd reason, why widening is wrong in both directions, and the one gap it genuinely cannot cover: a mirror / air-gapped install re-homes the node image onto the operator's own registry host, which a literal pattern can't name.PruneImagesreturns(int, error)— how many references it removed, so the caller can tell a real reclaim from a no-op. A failedrmireturns0: nothing is claimed reclaimed when the removal that would have reclaimed it didn't succeed.delete.go—No tracebloc images left to reclaim.on 0,Reclaimed N tracebloc image(s).otherwise. The failure hint is unchanged. The preview line ("tracebloc's downloaded images") is also unchanged and stays accurate: the offboard does remove the machine's images — via the cluster teardown, which is now stated in RFC-0001 §7.10.ghcr.io/tracebloc/jobs-manager:1.9.5, a name no producer has ever written (jobs-manageris a Hub image and its tag is a channel, not a semver). That fixture is precisely why this survived: it asserted the constant back at itself. Fixtures are now names a producer really publishes, and the newTestImageReferenceScopepins the scope to producer facts in both directions — it must cover the published ghcr node image, and it must not reach Docker Hub'stracebloc/*.Evidence
make check(vet + full tests + fmt + file budget + style) andmake lint(errcheck, ineffassign, misspell, staticcheck-checks all) are green;gofmt -l ./internal/is clean. The golden string catalog was regenerated and its diff is exactly the two changed strings.Mutation testing — each part of the fix deliberately broken, test reddens
1. Widen the pattern to Docker Hub's
tracebloc/*(the harmful widening):2. Apply the originally-proposed
docker.io/tracebloc/*(the inert widening):3. Restore the always-claim-success message (
else if n == 0→else if false):4. Claim images reclaimed even when
rmifailed (return 0, err→return len(refs), err):All four restored; suite green.
Confirmed on a real machine, not just reasoned about
The ticket flagged "I did not run
tracebloc deleteon a real installed machine" as the one thing worth doing before fixing. The repo's own Offboard teardown (k3d) e2e does exactly that: it builds the real binary and runstracebloc delete --yes --forceblack-box against a throwaway k3d cluster with a real Helm release installed. Its output on this PR:So on a real Linux host, with a real cluster and a real release, the reclaim matches nothing — the no-op is confirmed end to end, and the offboard now says so instead of claiming it reclaimed disk.
Not covered
imageReferencerather than fixed, since the CLI can't name the operator's registry host from a literal pattern.Closes tracebloc/backend#1861
Test plan:
make check+make lint, plus the four mutations above.Note
Low Risk
Offboard UX and test/doc changes only; image reclaim scope is unchanged and failures remain best-effort.
Overview
tracebloc deleteno longer always prints that disk was reclaimed.PruneImagesnow returns(int, error)so the CLI can distinguish a real host-daemon cleanup from the usual no-op after cluster teardown.User output follows the count:
No tracebloc images left to reclaim.when nothing matched,Reclaimed N tracebloc image(s).when images were removed, with unchanged guidance on failure. Theghcr.io/tracebloc/*filter is unchanged; docs and comments now explain that chart images live in the k3d node’s containerd (reclaimed byk3d cluster delete) while the host step only targets images like the Windows GPU node image—and why widening to Docker Hub’stracebloc/*would be unsafe.Tests use real published image names and add
TestImageReferenceScopeplusTestDelete_ImageReclaim_ReportsWhatItDid; RFC-0001 §7.10 and golden strings are updated.Reviewed by Cursor Bugbot for commit b829c47. Bugbot is set up for automated code reviews on this repo. Configure here.