fix: clean up old docker images after app updates - #2196
Open
ryanwaits wants to merge 1 commit into
Open
Conversation
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.
Problem
umbrelOS never reclaims old Docker images after app updates, so
/var/lib/dockergrows unbounded. #2140 reports 94 GB of overlay2 with 66 GB reclaimable, and the community threads linked there go back to 2021.The cleanup actually exists (
app.tsrunsdocker rmi <old refs>after every update) but it silently misses:repo:tagrefs the pull re-points the tag first, so the rmi targets the new in-use image, fails, and the emptycatch {}hides it. The old image is left behind as an untagged<none>. Repro'd in umbrel-dev with a local registry.--rmi allonly covers images the current compose references (verified, a leaked image survives uninstall). Old app-proxy/tor images accumulate the same way after version bumps.This looks like regression rather than policy. The 0.5.x era cleaned up old images several times over (
docker image prunein 2020, scoped to Umbrel-only in #138, per-app rmi in 2022, cfc5a9b "Restore image cleanup logic from previous update" in 2023), and nothing in umbreld uses an old image after an update. Log rotation (solution 3 in the issue) was already fixed via #2169, so this is purely about images.Fixes #2140
Fix
Two parts. Both only ever touch images umbrelOS itself manages, never a global
docker image prune, keeping the constraint from #138 that user images on the same daemon are untouchable.1. Make update cleanup correct (
app.ts): resolve the old compose refs to image ids before the pull, so a re-pointed tag can't dodge removal. After the update, remove the old refs first (leaves any user-created tags on the same image alone), then remove now-untagged old ids. Anything still used by the updated app or another installed app is skipped, since apps share images and the samerepo:tageven exists with different digests across official apps. Failures are logged instead of swallowed, and cleanup can never fail an otherwise successful update.2. Reclaim already-leaked images (
apps.ts+utilities/docker-images.ts): a conservative sweep on boot (after apps are up, skipped in dev mode). An image is only removed if it was pulled from a registry and has no tags (the state leaked images are in, user-built images have neither), its repo belongs to an installed app or the system containers (read from the legacy-compat compose files at runtime so they can't drift), no container in any state uses it, it isn't what any installed app's compose currently resolves to, and it's still untagged at removal time. If any installed app's compose can't be read the sweep aborts entirely.Two deliberate details worth flagging:
docker image ls --allbecause Docker CLI 29 hides untagged images from plain listings (the sweep would silently no-op at the next Docker bump), and the sweep won't touch orphans of repos no installed app references (e.g. leftovers of an app uninstalled after a failed update). That's the "only images umbrel manages" rule, happy to change the dial there.Testing
Full
apps.integration.test.tssuite passing viatest:umbrel-dev(37 tests), including three new ones:update() removes the old image when a mutable reference is re-pointed— the Docker images accumulate indefinitely after app updates — /var/lib/docker grows unbounded #2140 leak mechanism, asserts the old image is removed and the new one survivesupdate() applies a new app version from the app store— a real version-bump update through the git-server fixture, fills in the existing update test's// TODO: Check this actually worked(createTestUmbreld now exposes its git server to make this possible)cleanOrphanedImages() removes orphaned app images and nothing else— creates a leaked-style orphan of a managed repo plus three must-survive images (an orphan of an unmanaged repo, a tagged image, a locally built image), asserts only the managed orphan is removedAlso verified by hand in umbrel-dev before writing the fix: the mutable-tag leak (exact rmi conflict captured), the digest-pinned happy path not leaking, and a leaked image surviving uninstall.
Caveat
Tested in umbrel-dev (Docker/macOS), not on real umbrelOS hardware. Failed updates still skip the in-update cleanup (the old ref stays tagged, so the next successful update or the boot sweep reclaims it), the leak just can't compound silently anymore.