Repository navigation
purge, prune: name the Docker disk neither command frees - #207
Conversation
Both commands report freeing disk and neither frees the disk most machines are actually holding: on the host devlaunch#160 was decided, a prune had 4.00 GB of stale clones to give back while `docker system df` read 86.5 GB of reclaimable images, 43.18 GB of volumes and 13.88 GB of build cache. A freed figure printed on its own reads as all of it, and was out by an order of magnitude. So both end on one line, from one constant, so the two cannot drift apart. Printed from a wrapper around each command's body rather than at each of their outcomes, which is what makes "a run that finished names the boundary" true of every way either can finish -- including the ones that removed nothing, where somebody whose disk is full is most likely to be reading. `--prune`'s refusal of an unknown option is the one exit that does not, never having looked at a directory. A sentence, not a measurement: no docker process, no image enumeration, no parsing of devpod's image names. Nothing to be slow, and nothing to fail where Docker is absent or stopped. It points at `docker system df` and never at `docker image prune -a`, which is unscoped and would take images devlaunch never built -- the footgun PR #129 took out of `--purge` -- nor at a `docker image rm` for ids `dl` cannot honestly supply, devpod's images carrying no devlaunch or devpod label.
The line is documentation that ships as output, so the README shows it in both commands' captured output and says what it is for, the help says it under `--purge` for both, and the changelog carries the measurement it answers. The `--ls --size` note already drew the same boundary for its own reasons; it now links to the section rather than being a second copy of it.
Reviewer's GuideThis PR introduces a shared "Docker boundary" message that both Sequence diagram for shared Docker boundary in prune and purge commandssequenceDiagram
actor User
participant dl as dl
participant purge_all_data
participant _purge_devlaunch_data
participant prune_command
participant _prune_clone_directories
participant print_docker_boundary
%% Purge flow
User->>dl: dl --purge
dl->>purge_all_data: purge_all_data()
purge_all_data->>_purge_devlaunch_data: _purge_devlaunch_data()
_purge_devlaunch_data-->>purge_all_data: status
purge_all_data->>print_docker_boundary: print_docker_boundary()
print_docker_boundary-->>purge_all_data: prints Docker boundary line
purge_all_data-->>dl: status
%% Prune flow
User->>dl: dl --prune [flags]
dl->>prune_command: prune_command(flags)
prune_command->>_prune_clone_directories: _prune_clone_directories(flags)
_prune_clone_directories-->>prune_command: status
prune_command->>print_docker_boundary: print_docker_boundary()
print_docker_boundary-->>prune_command: prints Docker boundary line
prune_command-->>dl: status
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #207 +/- ##
==========================================
+ Coverage 92.25% 92.28% +0.02%
==========================================
Files 24 24
Lines 3436 3448 +12
==========================================
+ Hits 3170 3182 +12
Misses 266 266
🚀 New features to boost your workflow:
|
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis review of #207 against ticket #160 at merge-base f5f1333. Preflight: all checks green (ci ×5, e2e, prek, gate, codecov patch+project), non-empty diff (5 files, +304/−3), MERGEABLE.
Standards
BLOCKING
- CHANGELOG entry is filed under a released section. The new bullet lands at
CHANGELOG.md:111, i.e. below## [0.0.26] - 2026-08-14(line 95), not under## [Unreleased](line 8). The last three CHANGELOG commits on main —dfd738b,dc75158,461f740— all wrote into[Unreleased](hunks at lines 66, 50, 19), andpyproject.tomlalready readsversion = "0.0.26", so as written the entry tells readers a feature shipped in a release that does not contain it. One-line move to[Unreleased]'s### Added.
Non-blocking
DOCKER_BOUNDARY(devlaunch/dl.py:887) diverges from its two siblings in both name and home:DEVPOD_MISSING_MESSAGE/SSH_MISSING_MESSAGEare*_MESSAGE, live in the constants block at lines 95–118, and each carries a "why these words" comment above it.DOCKER_BOUNDARY_MESSAGE, or a comment, would close the gap. (nit)- Constant + wrapper is two names for one sentence with one reader each — the tests deliberately use literals, so
DOCKER_BOUNDARYis read only byprint_docker_boundary. Mild Middle Man; inlining loses nothing. Theprint_*wrapper itself is squarely in-idiom (print_prune_plan,print_workspaces,print_help), and extracting_purge_devlaunch_data/_prune_clone_directoriesso the line covers every exit is the right shape — the repeated 3-line wrapper body twice is the only structural duplication introduced, and it is cheaper than the alternative. (observation) - Help text (
dl.py:3401) documents the boundary only on--purge, as "Like --prune…", while--prune's own entry — which the reader meets first — says nothing. Also,README.md:414's command-table row for--purgeis unchanged; AGENTS.md asks README to track help output, though the new#### The disk neither command freessection and its links arguably discharge that. (nit) run_purgeintest/unit/test_docker_boundary.pypatchesdevlaunch.dl.update_cache_background, but--purgenever reaches it (wants_startup_cache_refreshreturns True only for_CACHE_READING_COMMANDS, and no purge-path call exists). Inert scaffolding copied fromtest_purge_ownership.py;run_pruneomits it, so the two helpers are asymmetric for no reason. (nit)RecordedProcessesis the fifth subprocess recorder intest/unit/(fourRecordedDevpodclasses), despitetest/fixtures/devpod_mock.pyexisting as a shared home. Per-file recorders are the suite's established practice and this one has a genuinely different job (records all argv), so consistent — just noting the trend. (observation)- The 86.5/43.18/13.88 GB figures now appear in four places (CHANGELOG, README,
dl.pydocstring, test module docstring). Host-measured citations in docstrings are repo idiom; four copies of one number is four things to keep true. (observation)
Test conventions otherwise match well: behavioural class/test names, strings written out rather than imported (with the reason stated), and boundary_line's exactly-one assertion plus the purge/prune equality test are the right drift guards. test_docker_boundary.py, test_purge_ownership.py, test_purge_partial_removal.py: 54 passed.
Spec
Verified against ticket #160 (fetched), the exact diff, and by running the code. Nothing blocks on this axis.
Met
- "Print one line at the end of
--purgeand--prune: devlaunch does not manage Docker images or volumes, the containers these workspaces used may still hold disk, anddocker system dfshows what Docker is holding." — one shared constantDOCKER_BOUNDARY(devlaunch/dl.py:887), printed via wrapperspurge_all_data(:929) andprune_command(:2404). All three clauses present verbatim. - "No docker subprocess — nothing to fail when Docker is absent." — the whole implementation is
print(DOCKER_BOUNDARY); nodockerinvocation anywhere in the diff. Held (vacuously, as the PR declares) bytest_naming_it_starts_no_docker_process. - "No image enumeration, and no parsing of devpod's
<workspace-id>-<5hex>:devpod-<32hex>naming." — no enumeration or parsing added. - "Point at
docker system df… and never atdocker image prune -a" — the printed line names onlydocker system df;test_neither_command_offers_to_delete_an_imagescrubs both commands' output forimage prune,docker image rm,docker rmi,system prune. - "Tested: the boundary line is asserted in both commands' captured output … and README /
--help/ CHANGELOG say the same thing." —test/unit/test_docker_boundary.py(9 tests, pass; wording written as literals, drift test compares the two commands' actual lines) plus one intest_prune_orphaned_clones.py. README,--helpand CHANGELOG all updated.
Findings
- Non-blocking (edge path + doc overclaim). "Print one line at the end of
--purgeand--prune". Both abort paths run:dl --pruneanswerednprints the line;dl --purgeanswerednprintsAborted.and stops — the purge confirmation is inmain()(dl.py:3651-3655), outside the wrapper. Defensible (an aborted purge reported no freed figure), but it is asymmetric with prune's ownn, is not declared in the PR body's "Where it prints" (which lists only the unknown-flag exit as the exception), and two claims are then false as written: thepurge_all_datadocstring's "holds for every way a purge can end" (:925) and README's "That last line ends every purge" (README.md:489). Either print it on abort or narrow both sentences. - Non-blocking.
--prune'sreport_unlocatablefailure exit (dl.py:2417-2419, status 1) also prints the line — a failure report rather than a cleanup. Harmless, and consistent with the docstring's stated scope.
PR body claims checked: "on main, neither command says anything about Docker" — confirmed (no system df in dl.py at the merge base). Test inventory and the "8 of 10 bite" split match the diff.
Verdict
Request changes — one blocking finding:
- Move the CHANGELOG entry from under
## [0.0.26]to## [Unreleased]→### Added(Standards).
Worth folding into the same round because it makes two shipped sentences false: resolve the purge-n asymmetry — either print the boundary line on the aborted-purge path too, or narrow the purge_all_data docstring (":925 holds for every way a purge can end") and README:489 ("That last line ends every purge") to match what the code does (Spec, non-blocking on its own).
All other findings are non-blocking and recorded above. GitHub refuses --request-changes from the PR author's account, so this is posted as a comment; this written verdict governs.
… purge Blocking (Standards): the boundary-line bullet was filed under `## [0.0.26]`, a section pyproject.toml already claims as released, so it told readers the feature shipped in a release that does not contain it. Moved to `## [Unreleased]` -> `### Added`, where dfd738b, dc75158 and 461f740 filed theirs. Spec: `dl --prune` answered `n` ended on the Docker boundary line, `dl --purge` answered `n` printed `Aborted.` and stopped -- the purge confirmation lives in main(), outside the wrapper that prints the line. That made two shipped sentences false: purge_all_data's "holds for every way a purge can end" and README's "That last line ends every purge". Resolved by removing the asymmetry rather than documenting it: the aborted purge now prints the line too, through the existing print_docker_boundary wrapper so there is still one sentence and one literal. The listing above the prompt is a report of what a purge would take, which is where the disk it would not take is worth naming -- somebody who just declined is still looking for the gigabytes. Docstring, README and CHANGELOG now say so, and TestAPurgeAnsweredNoEndsThereToo pins the abort path, including that its line is byte-identical to a completed purge's.
Closes #160
dl --purgeanddl --pruneboth end on one line naming the disk they do not free:What was there before
The decision on #155 reads as if
--prunealready printed this, so the first thing this branch did was check: onmain, neither command says anything about Docker.--pruneends atRemoved N clone director(ies) -- X.and--purgeatRemoved: <cache>or the refusal report. So this adds the line to both rather than extending one to the other, and a single shared constant is what keeps them saying the same thing afterwards — pinned by a test that compares the line the two commands actually printed, rather than each against the constant.Why
The gigabytes a cleanup reports are usually not the ones being looked for. On the host #160 was decided on,
--prunehad 4.00 GB of stale clones to give back whiledocker system dfread 86.5 GB reclaimable images, 43.18 GB volumes, 13.88 GB build cache — an order of magnitude more, sitting behind a command that had just said "Removed". Saying nothing is what makes a freed figure read as all of it.The constraints the ticket is about
docker.docker image rm.docker image prune -a— unscoped, and it would take images devlaunch never built, the exact footgun PR dl --purge deletes only the workspaces devlaunch created #129 removed from--purge. A test scrubs both commands' output forimage prune,docker image rm,docker rmiandsystem prune.Where it prints
From a wrapper around each command's body rather than at each outcome, so "a run that finished names the boundary" holds for every way either can finish — including a purge that found nothing to purge and a prune answered
n, which are where somebody whose disk is full is most likely to be reading. The one exit that does not print it is--prune's refusal of an unknown option, which never got as far as looking at a directory.Tests
test/unit/test_docker_boundary.py(new) holds both commands together, with the wording written out as a literal traceable to the ticket rather than imported from the code:TestWhatAPurgeSaysAboutTheDiskItDidNotFree— names the disk it does not manage / says it may still be held / points at the command that measures itTestWhatAPruneSaysAboutTheDiskItDidNotFree— the same threeTestTheTwoCommandsCannotDriftApart::test_they_name_the_boundary_in_the_same_wordsTestWhatTheBoundaryMayNotDo::test_neither_command_offers_to_delete_an_imageTestWhatTheBoundaryMayNotDo::test_naming_it_starts_no_docker_processtest/unit/test_prune_orphaned_clones.pygainsTestTheDiskThisCommandDoesNotFree::test_a_run_that_removed_clones_still_names_the_docker_disk_it_does_not_manage, because the fixture that removes something real lives there and the removal path is the one a freed figure is read on.All eight assertions were checked to bite: with the print neutered, eight of the ten new tests fail. (The two constraint tests pass vacuously by design — they are there to fail if someone later adds a
docker image rmto the line.)Docs
README shows the line in both commands' captured output and adds "The disk neither command frees" explaining the sentence-not-a-measurement and points-not-offers properties; the
--ls --sizenote that already drew the same boundary now links there instead of being a second copy.--helpsays it under--purge, for both commands. CHANGELOG carries the entry with the measurement it answers.Verification
pixi run cigreen: 1610 passed, pylint 10.00/10, ruff and ty clean. Also run for real against a scratchXDG_CACHE_HOME—dl --prune -y(nothing to prune) anddl --purge -y(foreign workspaces named and left standing) both print the line as their last output.🤖 Generated with Claude Code
Summary by Sourcery
Add a shared Docker disk boundary message printed at the end of both --purge and --prune runs, clarifying the cleanup scope and pointing users to
docker system df.Enhancements:
Tests: