Repository navigation
Conversation
On Linux, hull's image records come from containerd rather than a store directory, and `hull rmi` there has to resolve a reference against them the way FindImages does on macOS: the reference as written, a digest pin, a bare index digest, then a digest prefix. MatchImages is that matching, taken out of FindImages so it works on records a caller already holds. FindImages lists the store and calls it. Refs: #105 Signed-off-by: Charalampos Mainas <cmainas@nofire.ai>
`hull rmi` on Linux takes the same flags and resolves a reference to the images it removes with the same refusals: no such image, in the store but not for that platform, and an ambiguous digest prefix. The command and that resolution, now pickRemovals, move to rmi.go, with the holder and wording helpers. What reads and deletes from the macOS store stays in rmi_darwin.go, and the command's description, which names the vz rootfs modes, comes from there. rmi.go keeps its darwin tag until the Linux command exists, since the Linux build would otherwise carry code nothing calls. An ambiguous prefix is now judged by the distinct digests it matched rather than the number of records. A store record is keyed by its digest, so the two are the same on macOS. Refs: #105 Signed-off-by: Charalampos Mainas <cmainas@nofire.ai>
8219a11 to
1bc7512
Compare
|
@claude-fleet review |
|
Review task queued. Task: 9cb95002-2bdf-4c92-adb7-51d3d120371f |
There was a problem hiding this comment.
Request changes: one should_fix finding.
Findings:
- should_fix, internal/containerd/remove.go:48 —
ImageManifestsskips the presence check when the name points straight at a manifest, sormi --platformleaves a dangling name instead of removing it
Verified:
--platform linux/arm64matches an entry recorded aslinux/arm64/v8, and matches the way pull does: both useplatforms.OnlyStrict, whoseNormalizestrips thev8variant on both sides — cmd/hull/rmi_linux.go:76, internal/containerd/pull.go:73lookupRefleaves a digest or digest prefix alone and normalizes everything else — cmd/hull/rmi_linux.go:141-151;digest.Algorithm("sha256").Available()is true becausecrypto/sha256is in the binary's dependency graph (go list -deps ./cmd/hull), not because go-digest registers it- The macOS wording is unchanged: the four constants and
rmiDescriptionin cmd/hull/rmi_darwin.go:37-53 are the strings the old cmd/hull/rmi.go carried - The resolution refactor is behavior-preserving:
MatchImages(pkg/store/reference.go:164-197) is the oldFindImagesbody verbatim, andFindImagesnow only lists and delegates — pkg/store/image.go:306-313 - A bare index digest matching several names comes back with
byPrefixfalse and so is refused by name count, not as ambiguity — pkg/store/reference.go:182-189, cmd/hull/rmi_linux.go:212 - The refusals are the same code on both OSes:
pickRemovalsis reached from cmd/hull/rmi_darwin.go:88 and cmd/hull/rmi_linux.go:105
Checks run:
GOOS=linux GOARCH=arm64 go build ./...→ okGOOS=linux GOARCH=arm64 go vet ./cmd/hull/ ./internal/containerd/ ./pkg/store/→ ok (the Linux-only files and their tests compile)go test ./pkg/store/ -run 'TestMatchImages|TestFindImages'→ okgo test ./cmd/hull/ -run 'TestRmi|TestPickRemovals|TestRemoveImages'→ ok- Replaced
slices.Compact(digests)withslices.Clone(digests)at cmd/hull/rmi.go:117 →TestPickRemovalsJudgesAmbiguityByDigestfails on the first assertion ("two names of one digest = 0 match(es) ... is ambiguous"); file restored,git status --porcelainclean
Smaller things:
- cmd/hull/rmi_linux.go:283 drops
DropManifests'deletedName. The output line is the same either way, but a user who just removed the last platform of a name is not told the name went, which the description calls out as the behavior. - The refusal for a running container (cmd/hull/rmi_linux.go:309-313) has no test;
rmi_linux_test.gocovers only the stopped case, andtaskRunningis never exercised with a live task. The description leads with "a running one ... refuses the removal even under--force". - cmd/hull/rmi_linux.go:101 returns
errfromlistCatalogdirectly, discarding thefailurescollected for earlier references. Same shape as the macOS code, so this is only a note:hull rmi a bwith an unknownaand a daemon that fails the second listing reports only the listing error. dropPlatformreads every index in the namespace a second time (cmd/hull/rmi_linux.go:252, afterplatformManifestsalready did it for the matches) andDropManifestsruns a synchronous collection per affected name. Correct, just more daemon round trips than needed.- docs/build.md:116 is 107 characters; the rest of the paragraph wraps at about 76.
Not checked:
- Any test that needs a containerd daemon:
internal/containerd/remove_test.goandcmd/hull/rmi_linux_test.gowere read but not run (no daemon here), so the hand-check numbers in the description (31 blobs to 28, the two-namectr image tagcase) are unverified. - The claim that deleting an empty lease with
leases.SynchronousDeleteruns a collection before returning — read as containerd semantics from the comment at internal/containerd/remove.go:136-141, not observed. - Whether the index's leftover
gc.ref.content.m.<n>label for a dropped manifest is harmless to containerd's collector.
Not checked:
- internal/containerd/remove_test.go and cmd/hull/rmi_linux_test.go were read but not executed: both need a containerd daemon, which this workspace has none
- The description's hand-check against containerd v2.4.2 (content store 31 blobs to 28, the ctr image tag two-name case)
- That deleting an empty lease with leases.SynchronousDelete forces a garbage collection before returning
- Whether a leftover containerd.io/gc.ref.content.m. label on the index, naming a deleted manifest, is harmless to containerd's collector
Task 9cb95002-2bdf-4c92-adb7-51d3d120371f on pmoust-mbp, model claude-opus-5, effort medium.
| // present when its blob is, whatever is missing under it. | ||
| func ImageManifests(ctx context.Context, cs content.Store, img images.Image) ([]Manifest, error) { | ||
| if images.IsManifestType(img.Target.MediaType) { | ||
| return []Manifest{{Desc: img.Target, Platform: configPlatform(ctx, cs, img.Target)}}, nil |
There was a problem hiding this comment.
should_fix: The manifest-type branch of ImageManifests returns img.Target without checking the blob is in the content store, while the index branch below filters on cs.Info (remove.go:63-66). DropManifests uses ImageManifests to decide whether the name has a platform left (remove.go:126-130), so for a name that points straight at a manifest the answer is always yes, even immediately after the blob was deleted.
Failure: hull rmi --platform linux/amd64 example.com/single:1, where single:1 is a single-platform image whose target is a manifest, not an index. dropPlatform matches it, DropManifests deletes the manifest blob, then ImageManifests still reports one manifest, so deletedName is false and the image record survives. containerd is left with a name whose target blob is gone: hull images logs image example.com/single:1: read manifest: ... and lists it with no size or platform. The documented rule, "The name goes with its last platform" (docs/build.md), and the behavior TestRmiPlatformDropsOnePlatform asserts for the index case do not hold here.
Evidence: internal/containerd/remove.go:46-49 (no presence check) against remove.go:59-75 (the index branch does check); internal/containerd/remove.go:126-132 decides the name's fate from that result. TestImageManifests (internal/containerd/remove_test.go) covers the manifest-target case only with the blob present.
Fix: give the manifest-type branch the same cs.Info / errdefs.IsNotFound check the index entries get, and return no manifests when the blob is gone.
The Linux CLI could pull and list images but not remove them. `hull rmi` takes the macOS arguments and flags and prints the same lines, resolving a reference through MatchImages and pickRemovals over the records `hull images` prints. A name is normalized as pull normalizes it first, so `busybox` finds docker.io/library/busybox:latest. A string whose part before the colon is a digest algorithm is left alone, since normalizing it would make it a tag on Docker Hub. containerd keeps one image per name, and the rules follow Docker where that differs from the macOS store, where a digest keys one entry: - A digest or digest pin that several names point at is refused without --force, as `docker rmi` refuses an image ID in several repositories. An ambiguous prefix is judged by distinct digests, so a prefix of one image under two names is that rule's case, not ambiguity. - --platform deletes the platform's manifest blob, as `docker rmi --platform` does, and keeps the name while another platform remains. The manifest's gc.ref labels go with the blob, so the collector takes its config, layers and snapshot. The index keeps a label naming the missing digest, which holds nothing. A manifest is stored once, so every name whose index lists it loses the platform, and that takes --force as well. - Containers in the namespace created from the image hold it as instances do on macOS: a running task refuses even under --force, a stopped container without --force. Nothing creates such a container yet, but ctr and nerdctl can, and `hull run` will. Removal waits for the garbage collector, so the space is free when rmi returns. containerd has no call to run a collection, so DropManifests deletes a fresh lease with SynchronousDelete, which runs one. rmi.go loses its darwin tag now that both builds use it. The usage text and the platform-miss error now come from the per-OS files, as the description does: the macOS wording names the store and its instances, which Linux does not have. Refs: #105 Signed-off-by: Charalampos Mainas <cmainas@nofire.ai>
1bc7512 to
e72248e
Compare
|
@claude-fleet review |
|
Review task queued. Task: 41ce9dd0-8cf6-4173-851d-07b3d8a38bb7 |
There was a problem hiding this comment.
Previous round at 1bc7512: 1 finding.
| Previous finding | Classification | Cited line |
|---|---|---|
internal/containerd/remove.go:48 (should_fix): The manifest-type branch of ImageManifests returns img.Target without checking the blob is in the content store, while the index branch below filters on cs.Info (remove.go:63-66). DropManifests uses ImageManifests to decide whether the name has a platform left (remove.go:126-130), so for a name that points straight at a manifest the answer is always yes, even immediately after the blob was deleted. Failure: hull rmi --platform linux/amd64 example.com/single:1, where single:1 is a single-platform image whose target is a manifest, not an index. dropPlatform matches it, DropManifests deletes the manifest blob, then ImageManifests still reports one manifest, so deletedName is false and the image record survives. containerd is left with a name whose target blob is gone: hull images logs image example.com/single:1: read manifest: ... and lists it with no size or platform. The documented rule, "The name goes with its last platform" (docs/build.md), and the behavior TestRmiPlatformDropsOnePlatform asserts for the index case do not hold here. Evidence: internal/containerd/remove.go:46-49 (no presence check) against remove.go:59-75 (the index branch does check); internal/containerd/remove.go:126-132 decides the name's fate from that result. TestImageManifests (internal/containerd/remove_test.go) covers the manifest-target case only with the blob present. Fix: give the manifest-type branch the same cs.Info / errdefs.IsNotFound check the index entries get, and return no manifests when the blob is gone. |
fixed | internal/containerd/remove.go:48 |
No findings.
Approve: the previous finding is fixed, and I found nothing blocking in the rest of the diff.
Verified the fix by reverting it: with the present check removed from the manifest-type branch of ImageManifests, TestImageManifests fails on the new assertion (remove_test.go:77 — "a name pointing at a missing manifest gave [...], want none"). Restored; tree clean.
Verified:
--platform linux/arm64matches an entry recorded aslinux/arm64/v8, as the docs claim:platforms.OnlyStrictnormalizes both sides (compare.go:245-247,platforms.go:189-194), andpull.go:73uses the same matcher, sormiandpullagree — rmi_linux.go:76.- The macOS code moved without behavior change:
rmi_darwin.go:54-186is the oldrmi.gobody verbatim, and the one semantic change (matchedDigestsnow compacting) is a no-op there because the store keys one record per manifest digest — rmi.go:106-117. MatchImagesis the oldFindImagesbody unchanged, including the bare-index-digest pass — reference.go:163-197 against the deleted block in image.go.- A digest or digest prefix is left unnormalized:
lookupRefreturns early when the part before the first:is a registered digest algorithm, so a registry port (localhost:5000/x) still normalizes — rmi_linux.go:141-151. - The GC wait is real on both paths: whole-name removal uses
images.SynchronousDelete()(remove.go:115), platform drop deletes a freshly created lease withleases.SynchronousDelete(remove.go:158-169). - Every refusal path checks holders before any deletion: rmi_linux.go:218, 271-276.
Checks run:
go build ./...andGOOS=darwin GOARCH=arm64 go build ./...→ ok;GOOS=darwin go vet ./cmd/hull/ ./pkg/store/→ ok (the sharedrmi.golost its//go:build darwin, so this was worth confirming).go test ./pkg/store/ ./cmd/hull/ ./internal/containerd/→ ok.- Reverted the manifest-branch
presentcheck ininternal/containerd/remove.go→TestImageManifestsFAIL; restored,git status --porcelainclean apart from the pre-existingnode_modules.
Smaller things:
- rmi_linux.go:268 — the shared-platform refusal names
matches[0].Ref, andmatchesfollowsImageService().Listorder, so which of the sharing names appears in "the linux/amd64 content of X is shared by names A, B" is not deterministic across runs. Thenameslist is sorted; the lead-in is not. - rmi_linux.go:252-255 — a name whose index blob cannot be read is skipped when building
affected, so if it shares a dropped manifest it is neither counted toward the--forcethreshold nor holder-checked. Narrow and fails toward doing less. - The four Linux-vs-macOS rules are written out twice, in
rmiDescription(rmi_linux.go:51-61) and under "Building on Linux" (docs/build.md), and the gc.ref explanation a third time at remove.go:126-133. They will drift; a pointer from the help text to the doc would cost nothing. - docs/build.md:115 — "All three need a running containerd, which hull does not install or" runs well past the ~75-column wrap the rest of the file uses.
- The PR body's one acknowledged gap (under
--platform, a digest matches only the digesthull imagesshows) is not in docs/build.md, where the rest of the Linux differences are. Worth a line there, since that is where a user looks. - "A running one ... refuses the removal even under
--force" has no test; only the stopped-container case is covered (TestRmiRefusesAnImageAStoppedContainerHolds).
Not checked:
- Every containerd-daemon test skipped here — no daemon in the environment:
TestDropManifestsThroughDaemon,TestImageHoldersThroughDaemon, and all ofrmi_linux_test.go. I read them but did not see them pass; the hand-check in the PR body against containerd v2.4.2 is likewise unverified. - macOS runtime behavior: darwin builds and vets, but no darwin test run and no
make macos.
Not checked:
- All containerd-daemon tests (internal/containerd/remove_test.go daemon cases and the whole of cmd/hull/rmi_linux_test.go) skipped: no containerd daemon in this environment.
- The hand-verification described in the PR body against a containerd v2.4.2 daemon.
- macOS runtime behavior: cross-build and vet only, no darwin test run and no
make macos.
Task 41ce9dd0-8cf6-4173-851d-07b3d8a38bb7 on ci-16gb-fsn1-1, model claude-opus-5, effort medium.
ananos
left a comment
There was a problem hiding this comment.
thanks @cmainas, looks great!
I ran it against containerd 2.3.5 (under rootlesskit) on a Linux box. The full suite passes with the daemon tests on. By hand, a short Hub name, an unknown ref next to a known one, a digest shared by two names, and --platform on a platform two names share all behave as described. The last one is refused, and --force then takes the content store from 65 to 28 blobs and keeps both names.
One suggestion: the "a running container refuses even under --force" path has no test, only the stopped one. unit-linux runs containerd as root on a runner with runc, so starting a sleep task from the image there should be enough to cover it. I could not start one under rootlesskit here (no cgroup delegation over ssh).
Summary
On Linux, hull keeps its images in containerd, and the CLI can pull and
list them but has no way to remove one. This adds
hull rmi. It takes thesame arguments and flags as the macOS rmi,
--force/-fand--platform,and prints the same
Removed <digest> <name>line for each image itremoves.
The resolution is shared with macOS rather than written again. A reference
is matched against the records
hull imagesprints, by the rulesFindImagesapplies to the macOS store: the reference as written, a digestpin, a bare index digest, then a digest prefix. The refusals are the same
code on both OSes too: no such image, in the store but not for that
platform, and an ambiguous prefix. Only reading the images and deleting
them is per OS. Two refactors come first, so the macOS code moves without
changing behavior before the Linux command uses it.
A name is normalized the way
hull pullstores it, sohull rmi busyboxfinds
docker.io/library/busybox:latest. A digest or digest prefix is leftas it is: normalized,
sha256:c408baae42f5would become a tag of arepository named
sha256on Docker Hub.Where Linux differs from macOS
containerd keeps one image per name, while the macOS store keys one entry
by each digest. Where the two models disagree, Linux follows Docker. These
are documented under "Building on Linux" in
docs/build.md:answers every digest it has resolved to, and all of them go.
of one image. That is refused without
--force, asdocker rmirefusesan image ID referenced in several repositories, and
--forceremovesevery name. A prefix is ambiguous only when it matches more than one
distinct digest, as on macOS, so two names of one image are this case
rather than ambiguity.
--platformdeletes that platform's manifest blob and keeps the namewhile another platform remains, as
docker rmi --platformdoes. Thename goes with its last platform. The manifest's
gc.reflabels go withthe blob, so containerd's garbage collector takes its config, layers and
unpacked snapshot. The index keeps a label naming the deleted digest,
which holds nothing. The content store holds a manifest once, so every
name whose index lists it loses the platform as well, and that takes
--force. The platform is matched ashull pullmatches it, solinux/arm64matches an entry recorded aslinux/arm64/v8.image hold it, as instances do on macOS. A running one, with a task that
has not stopped, refuses the removal even under
--force, and a stoppedone refuses it without
--force. A container keeps its own snapshot, sounlike a vz generic-container instance it does not lose its root
filesystem with the image, and the refusal leaves out the vz sentence.
Nothing in hull creates such a container yet, but ctr and nerdctl can,
and
hull runwill.hull rmiwaits for the garbage collector, so the space is free when itreturns. containerd has no call that runs a collection, so after dropping a
platform hull creates a lease and deletes it with
SynchronousDelete,which does. Removing a whole name uses
images.SynchronousDelete.The command's usage text and the error for a platform the name does not
have come from the per-OS files, as the description does. On Linux they
speak of containerd, containers and the names sharing an image's content,
not of a store and its instances. The macOS text is unchanged.
One gap is left: under
--platform, a digest matches only the digesthull imagesshows for a name, not the digests of its other platforms.Related issues
Refs #105. This is the
rmipart of L41 in the Linux parity plan.pruneand
images --jsonare not part of it.Changes
pkg/store:MatchImagesmatches a reference against records a calleralready holds.
FindImageslists the store and calls it.cmd/hull/rmi.go: the command, its flags andpickRemovals, the sharedresolution, now build on both OSes. Their wording comes from per-OS
constants. The macOS store code moves to
rmi_darwin.gounchanged. An ambiguous prefix is now judged by distinctdigests, which on macOS is the same as counting records.
internal/containerd/remove.go:ImageManifests,DeleteImage,DropManifestsandImageHolders.cmd/hull/rmi_linux.go: the Linuxhull rmi, registered inmain_linux.go.remove_test.goandrmi_linux_test.gorun against a containerddaemon. They cover:
rmi_shared_test.gocovers ambiguity by distinct digests on both OSes.reference_test.gocoversMatchImages.docs/build.mdlistshull rmiand how it differs on Linux.docs/cli.mdpoints there from thermirow.Checked by hand against a containerd v2.4.2 daemon, run as an ordinary user,
on
ghcr.io/containerd/busybox:1.36pulled withctr --all-platforms:--platform linux/arm64removed thelinux/arm64/v8manifest. Thecontent store went from 31 blobs to 28 before rmi returned, and the name
kept its other platforms.
ctr image tagwas refused on the shared digest.--forcethen removed both names, and the content store was left empty.Checklist
make testpassesmake macosbuilds hull, vz-runner and hvi, if Go, Swift or Rust code changedI have run the e2e harnesses (test/*.py) for changes touching boot, console, or the run pathdocs/)