Repository navigation
Report per-workspace disk size in dl --ls --size - #165
Conversation
Adds the measurement primitive the disk work on map #139 needs before anything can be called "reduced": what deleting a workspace's clone would actually free. The number is exclusive bytes, not apparent size. A repo is cloned once into a bare cache and every workspace hardlinks its git objects out of that one copy, so an apparent size bills each workspace for the whole shared pool -- reporting away the saving the sharing exists for. Measured on a bare plus two clones of this repo, `du` says each workspace's .git costs 2,539,520 B where 258,048 B is what removing it frees. The choice is pinned by test: swapping in an apparent size fails the hardlink tests and nothing else. Opt-in behind --size, because the walk is O(files) with no ceiling while plain --ls is one devpod round-trip and no filesystem work at all. Measured on a real cache, a workspace clone is 27.6-32.2 ms warm. A measurement that could not be completed is never reported as a number: the result is a two-armed sum type, and a walk that hit an unreadable directory reports a floor the caller cannot mistake for a total. devlaunch/disk_usage.py is deliberately free of devpod, git and metadata so #159's --prune imports it rather than growing its own walk. Closes #153
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideImplements opt-in per-workspace disk usage reporting for Sequence diagram for dl --ls --json --size disk usage reportingsequenceDiagram
actor User
participant dl_cli as dl_cli
participant disk_usage as disk_usage
User->>dl_cli: _run_cli(["--ls", "--json", "--size"])
dl_cli->>dl_cli: workspaces_as_json(with_size=True)
loop per workspace
dl_cli->>dl_cli: _disk_report(clone_path)
alt clone_path is None
dl_cli-->>dl_cli: return None (disk=null)
else clone_path exists
dl_cli->>disk_usage: exclusive_usage(clone_path)
disk_usage-->>dl_cli: DiskUsage (Measured|PartlyUnreadable)
dl_cli->>disk_usage: usage_as_json(usage)
disk_usage-->>dl_cli: {exclusiveBytes}|{atLeastBytes, unreadable}
dl_cli-->>dl_cli: row["disk"] = json object
end
end
dl_cli-->>User: JSON list with optional disk field
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #165 +/- ##
==========================================
+ Coverage 93.03% 93.49% +0.45%
==========================================
Files 20 21 +1
Lines 2354 2505 +151
==========================================
+ Hits 2190 2342 +152
+ Misses 164 163 -1
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
The SIZE column was sized from its cells only, so a listing whose sizes are all "-" -- every workspace opened from a path devlaunch did not clone -- printed a heading wider than the column it heads and shifted every LAST USED after it. Pinned by a test that fails without the fix. The refusal arms of the walk are now exercised rather than argued for: a tree behind an untraversable parent, entries a directory will name but not stat, and a file handed in where a directory was expected. The exhaustiveness sentinel joins its sibling on coverage's exclude list, for the reason already recorded there -- ty rejects any reachable call to it, so in a build that type-checks the line cannot run.
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis review of dcf157d against merge-base f03158b (git diff f03158b...HEAD), spec = ticket #153. Preflight: 3 workflow runs exist on the head SHA (prek success, CI success, Auto-publish skipped) and every check reports pass, including ci (py310). The axes ran as independent subagents in a fresh worktree and their findings are reported below unmerged and unreranked — where they overlap, they arrived there separately.
Posted as --comment: GitHub refuses --request-changes on a same-account PR. The written verdict at the bottom is the gate, not the GitHub review state.
Standards
- blocking —
test/test_devpod_spawn_counts.py:165(test_ls_with_sizes_spawns_nothing_extra): the guard is hollow. Thespawnsfixture lists one workspace whose source is/cache/myws, which is not under the real_get_cache_dir(), so_measurable_clonereturnsNone, the cell prints-, andexclusive_usageis never called. I wrappeddevlaunch.disk_usage.exclusive_usagewith a counter: 0 calls. I then injected the exact regression the test claims to catch (adevpod dusubprocess per measured workspace) and it stayed green under all three selections, including the full suite. This file is the repo's primary round-trip guard; the fix is a fixture workspace whoselocalFoldersits under the patched cache dir. - non-blocking —
devlaunch/dl.py:489vsdevlaunch/dl.py:1210: "which directory may we measure" is answered twice by two mechanisms — JSON viaclone_mgr.storage.get_worktree_by_workspace_id(...).local_path, table via_measurable_clone→source.path. Duplicated Code / Divergent Change, and they already disagree: with a workspace under the cache but no storage record, JSON reportsdisk: nullwhile the table reports2.0 MiB(verified, output below)._measurable_clone's docstring claim that "this is where that boundary is kept" is true for only one of the two outputs; #159 adding a case must edit both, and no test covers the mismatch. - non-blocking —
devlaunch/disk_usage.py:80: the module is clean of devpod/git coupling (confirmed by reading — imports areos,stat,dataclasses,pathlib,typingonly), but_unhandled_usageis private and there is no exported accessor for "the bytes, whatever arm". A downstream--prunegets the sum type without the exhaustiveness helper, so it will hand-roll anelse. - nit —
devlaunch/dl.py:1642:_size_cellreturns""for "column suppressed" and"-"for "not measurable" — one string doing two jobs, next door to a module that built a proper sum type for exactly this. - Docs (README "How much disk a workspace costs",
--help) state both hazards plainly — non-summing and sibling-deletion. No finding.
Evidence
Verified by execution (pixi run -e py310, review worktree):
pytest test/unit/test_disk_usage.py -q→20 passed in 0.34spytest test/test_devpod_spawn_counts.py test/test_workspace_state.py -q→52 passed in 3.52spytest(full, py310) →1191 passed, 23 deselected in 54.00s- Probe plugin, single test:
PROBE exclusive_usage calls: []; full file: same,24 passed - Regression plugin (subprocess injected into size walk): single test
1 passed; full file24 passed; full suite1191 passed, 23 deselected in 43.22s - Divergence scratch test:
JSON devlaunch= True disk= None/ table row... 2.0 MiB 2026-08-08 11:43:27 pylint→10.00/10, exit 0;ruff check .→All checks passed!;ty→All checks passed!git diff <base>...HEAD | grep '^+.*/home/'→ only README's elided/home/…/sample output; no hardcoded user path.
Not run / not verified: full pixi run ci (ran its test, pylint, ruff-lint, ty components individually, not format/coverage); e2e suite; ./dev.sh / dl-next (no real devpod exercised); the README's quoted timings (39–46 ms, 224/707 ms) and byte figures — not reproduced.
Spec
- non-blocking — shipped docs cite a walk cost the PR never measured. Spec: "if it costs more than ~100ms per workspace, make it opt-in, e.g.
dl --ls --size, and say so."README.md:528says "one workspace clone costs 39–46 ms to walk with a warm page cache" (repeated atdevlaunch/dl.py:1601andtest/test_workspace_state.py:210). The PR body's only measurement says "27.6–32.2 ms per real workspace clone". Same subject, same stated conditions, different number; 39–46 appears in no measurement anywhere. My own run of the shipped walk on a 9,124-entry tree was 17.6–21.0 ms. Doesn't change the conclusion (all under 100 ms) but it is a documented number contradicting its own evidence. - non-blocking — table and JSON disagree on which workspaces are measurable. Spec: "JSON field name and human rendering consistent with the existing table." JSON gates on a storage record (
_disk_report(clone_path),dl.py:517) and walksrecord.local_path; the table gates on source-under-cache (_measurable_clone,dl.py:1210) and walkssource.path. Verified by execution: for a clone under the cache with no metadata record, JSON emits"devlaunch": true, "disk": nullwhile the table prints2.0 MiB. README states "A workspacedldid not create reads-(nullin JSON)" — sonullhere means something the README doesn't cover, and #159's--prunereading JSON would see "not mine" for a tree the table sizes. - nit —
README.md:522attributes the 2.48 GiB figure to "a size that added up file lengths"; that number came fromdu, which counts blocks, not lengths. The code usesst_blocks(disk_usage.py:93) and only the module docstring says so. - nit — a file vanishing mid-walk (
FileNotFoundErrorfromentry.stat) is filed asunreadable, so a race downgrades a total to a floor, while the same error at the root isMeasured(0).
Nothing blocking on this axis. Constructive modeling holds: Measured/PartlyUnreadable is genuinely total, no path builds Measured from a partial walk, and absent / null / value are three distinct JSON states.
Opt-in deviation: correct call. The ticket's ~100 ms is a floor for requiring opt-in, not a ceiling forbidding it; it even names --ls --size as the form. O(files) with no ceiling, plus #155's ruling that --ls isn't where extra questions go, justifies it. README says it is opt-in but not that measured cost was under the ticket's trigger — the PR body says so; the docs should.
Evidence
Verified by execution (review worktree, py310 env):
- Hardlink experiment, own bare + two
git clones in scratch.statconfirms shared pack inodes (8270119, nlink 7). Per-treedu -s --block-size=1, separate invocations: bare3133440,ws1/.git3203072,ws2/.git3203072. Shipped code:Measured(exclusive_bytes=401408)/471040/471040.du -scover all three in one invocation:4075520. Direction of the PR's claim reproduced (per-treeduover-bills a workspace ~6.8x here) and so is the honest cost — exclusive sums to 1,343,488 against 4,075,520 actually held. - Mutation check, full suite (
pytest test/ -q -p no:randomlyon agit archivecopy withlinks_here >= linksremoved):1 failed, 1190 passed, 23 deselected in 60.48s, failing exactlyTestSharingIsNotBilledTwice::test_a_file_hardlinked_from_outside_is_not_billed_to_the_tree—assert 5246976 < (2 * 1048576). The pin is CI-effective, not isolation-only; baseline 1191 confirmed. - Selection
test/unit/test_disk_usage.py test/test_workspace_state.py test/test_devpod_spawn_counts.py:72 passed in 9.07s. - Divergence in finding 2: scratch pytest with an empty record store; verbatim table row
r-feature-aaa local … 2.0 MiB 2026-08-08 11:43:27against JSONdevlaunch= True disk= None. - Timing, warm, 3 runs after a warm-up, worktree root (9,124 entries):
20.7ms 21.0ms 20.3ms;.pixi(8,888 entries):18.8ms 17.6ms 17.9ms. - Cold-cache disclosure is present in the PR body: "no cold-cache measurement was taken (dropping the page cache needs root on this host), which is stated rather than estimated."
Not run / not verified: the PR's own host numbers (2.48 GiB / 1,824,649,216 B, 8,956 entries / 45.5 ms, 114,611-entry 224 ms / 707 ms) — different machine and cache, not reproducible there; pixi run -e py310 ci not run by this axis; no cold-cache measurement taken.
Verdict
Request changes.
The design decision at the centre of this PR is sound and, unusually, verified: the Spec axis rebuilt the hardlink experiment from scratch with real git and reproduced the direction and the honest cost of exclusive-vs-apparent independently, and it confirmed by full-suite mutation that the exclusivity pin is CI-effective rather than isolation-only. Constructive modeling holds. The opt-in deviation from the ticket's literal ~100 ms trigger is judged correct — the threshold is a floor for requiring opt-in, not a ceiling forbidding it.
One blocking finding:
test_ls_with_sizes_spawns_nothing_extradoes not guard anything (test/test_devpod_spawn_counts.py:165). The fixture's workspace source (/cache/myws) is not under the patched_get_cache_dir(), so_measurable_clonereturnsNoneandexclusive_usageis called 0 times under that test. The Standards axis injected the exact regression the test claims to catch — a subprocess per measured workspace on the size path — and the suite stayed green under single-test, whole-file, and full-suite selection. The PR body advertises this case as the pin for "--ls --sizeadds no subprocess at all"; it currently pins a code path that never executes. Fix: give the fixture a workspace whoselocalFoldersits under the patched cache dir, and confirm the guard goes red on the injected regression before it goes green.
Two non-blocking findings are worth folding in while the branch is open, both raised independently by each axis:
- Table and JSON gate measurability by two different mechanisms and already disagree — a clone under the cache with no storage record renders
2.0 MiBin the table and"disk": nullin JSON. That is a documented-behaviour mismatch (nullis README-defined as "a workspacedldid not create") and it is exactly the surface #159's--prunewill read. README.md:528states 39–46 ms where the PR's only measurement says 27.6–32.2 ms, and a third-party run measured 17.6–21.0 ms. On a ticket whose entire product is numbers, a shipped doc figure that matches no measurement should be corrected or sourced.
Neither of those blocks on its own; the hollow guard does.
The spawn-count pin for `--ls --size` was hollow: its fixture workspace sat outside the patched cache directory, so nothing was measurable, the walk never ran, and a `devpod du` per workspace passed it unnoticed. The fixture now puts the clone under the cache with a payload in it and asserts the rendered size before the spawn counts, so a `-` in that cell fails the test rather than emptying it. "May we measure this workspace" was answered twice, by the source directory for the table and by the metadata record for the JSON, and the two already disagreed: a clone under the cache with no record printed a size and reported `disk: null`. Both now ask _measurable_clone, which is the predicate `--purge` deletes by, and a test pins the agreement. A file that vanishes between being named and being weighed is worth nothing rather than unknown -- a live cache repacks and writes, and only a door that will not open should make the answer a floor. known_bytes exports "the bytes, whichever arm", so a caller ordering usages does not hand-roll an else. The walk timings are re-measured and the conditions named; the figures that matched no measurement are gone, and no cold-cache number is quoted because none was taken.
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Re-verdict after the retry. Two-axis review of 6e7e213 against merge-base f03158b (git diff f03158b...HEAD, +869/−34, 7 files), spec = ticket #153. I am a fresh reviewer; I wrote neither the code nor the previous review.
Preflight confirmed independently: 3 workflow runs exist on the head SHA (CI success, prek success, Auto-publish skipped), MERGEABLE/CLEAN, and every check passes including codecov/patch and ci (py310).
The blocking finding is CLOSED. Re-derived by me, not accepted on the builder's word — see the first evidence block.
Posted as --comment: GitHub refuses --approve on a same-account PR. The written verdict at the bottom is the gate, not the GitHub review state.
What I re-derived vs. what I accepted as previously covered
Re-derived from scratch: every mutation result below (12 distinct mutations, all under full-suite selection); the exclusive-vs-apparent design crux, rebuilt with my own git clone from a bare; the agreement between _measurable_clone, --purge and devlaunch: true, read out of the source; the disappearance of the 39–46 ms / 45.5 ms / 707 ms / 224 ms figures; the absence of hardcoded /home/<user>.
Accepted as previously covered, not re-derived: the opt-in deviation from the ticket's literal ~100 ms trigger (upheld — the threshold is a floor for requiring opt-in, not a ceiling forbidding it, and the ticket names --ls --size itself), and the constructive-modeling crux (Measured/PartlyUnreadable genuinely total, no path builds Measured from a partial walk, absent / null / value three distinct JSON states). I re-read both and found nothing to reopen.
⚠️ Reviewer-side disclosure: worktree contamination, and what was done about it
I gave both axis subagents the same worktree and both mutated source in it concurrently. The Standards axis caught this via ps, found a mutation in its tree that it had not written, and traced a false green of its own to the other axis's in-flight edit. It redid its results in an isolated git archive tree. This is my error, not the builder's, and it means some axis-reported mutation results were produced under contaminated conditions.
Remediation: after both agents finished I asserted the worktree byte-identical to 6e7e213 (git show 6e7e213:<file> compared to disk) and re-ran the four load-bearing mutations myself, serially, with a pristine-check before each and a mutation-still-present check after each. Those four re-derived results are the ones the verdict rests on. Where an axis result was not re-derived, it is marked as such below and carries no weight in the verdict.
Standards
- non-blocking —
known_bytes()is Speculative Generality.devlaunch/disk_usage.py:194. No production caller; the only references are intest/unit/test_disk_usage.py. It is exported for #159, which is not in this PR. On the misuse question: the only thing stopping a consumer printing this number and turning a floor into a total is 6 lines of docstring plus a test-class preamble — prose, not type — and a caller who wants "the bytes" reaches for the function literally named "the bytes". Under principle 2, land it in #159 alongside the ordering caller that makes it real and testable end-to-end. The exhaustiveness argument for the function living in this module is sound; it just needs something to call it. - non-blocking — one explanation copied five times, one measurement set three times. The exclusive-vs-
durationale lives atdisk_usage.py:9-31,README.md:492-509,test/unit/test_disk_usage.py:3-14,dl.py:519-533anddl.py:2160-2171; the figures in three of those, the timings in three more. Shotgun Surgery: re-measure on new hardware and six places drift. One canonical home (the module docstring) with the rest pointing at it. - non-blocking — a documented term of the sum is unpinned.
disk_usage.py:159, whose behaviour:106-108explicitly claims ("Directories are always this tree's… their own blocks count"). Replacing it withtotal += 0leaves the full suite green —test_a_tree_of_known_content_reports_what_it_holds's4*MIB <= got < 5*MIBwindow is a whole MiB wide, so nothing notices. Given this PR's history of a hollow guard, worth one tight assertion. - nit —
sizes/zip(strict=True)is machinery without a job.dl.py:1622-1634.sizesis constructed by the two preceding lines, sostrict=Trueguards an invariant that cannot fail; andwith_size: boolandcache_dir: Optional[Path]are two representations of one fact. One comprehension says it. On the_size_cellitem proper:print_workspaceshas not grown a second job — column layout was always its job andsized()makes one column optional cleanly. Only the list-then-zip step is surplus. - nit —
_size_cell/_disk_reportcontinuedl.py's accretion (1010 statements). Both are pure rendering adapters overdisk_usage._measurable_clonegenuinely belongs indl.py; these two need not. Pre-existing Divergent Change, not this PR's to fix. - Upheld, no finding — the measurability root cause. Gating on
is_devlaunch_clonerather than the metadata record is the right root cause, not a consistency patch. - Clean on the repo rules. No scope creep (
README.md:546-547rules Docker images/volumes out in two sentences; no code touches them). No hardcoded/home/<user>— README's samples use the elided/home/…/. The new_unhandled_usage\(coverage exclusion (pyproject.toml:165) is legitimate: it mirrors the adjacent_unhandled_source\(under the samety-proves-unreachable justification, anddisk_usage.pyis 83/83 otherwise.
Spec
- blocking (as labelled by this axis) — un-verified figures shipped un-hedged, in the present tense. Spec: "report the workspace's exclusive bytes, or clearly label the number as apparent size — decide during build, document the choice in --help and README." The docs justify that choice with figures the author states are unreproducible.
README.md:505-507— "On a real cache, three workspaces of one repo read as 2.48 GiB that way, against the 1.70 GiBdusays the disk actually holds…";disk_usage.py:16-18— "Measured on a real cache: one ROS workspace reads 755.0 MiB… against 488.8 MiB exclusive". The relabelling ("not reproducible… recorded here as history, not re-verified") exists only in the PR body, which does not ship;488.8 MiBis also the sample value in README's example table (README.md:493). Meanwhile the live replacement the builder already measured —du -s353,230,848 B vs 68,050,944 B exclusive, one 270 MB pack with two links — appears nowhere in any.pyor.md(grep, exit 1). Same paragraph, two figures: the timings carry OS/fs/cache/run-count/load, the exclusive-vs-apparent figure carries nothing. - non-blocking — the measurability predicate is correct; one note for #159.
_measurable_clone(dl.py:1221-1240) gates onis_devlaunch_clone, andworkspaces_as_jsoncomputesdevlaunchfrom the same call (dl.py:489), so"devlaunch": true⟺disk != nullby construction, and it is exactly what--purgedeletes by (purge_all_data→workspace_ownership→is_devlaunch_clone). Theisinstance(source, LocalFolder)conjunct is redundant but harmless (it is needed for type narrowing). Note for #159:is_devlaunch_clonecomparesPurePathlexically with noresolve()— precisely the symlinked-cache trap #159 calls "a total-loss bug". Benign here (a symlinked cache merely understates to-); it must not be reused as-is forReferenced. - non-blocking — the removed-clone edge is documented but unpinned.
disk_usage.py:76-78asserts that a directory that is not there isMeasured(0), "the same answer workspace_state gives for a clone that has already been removed by hand" — so a devlaunch workspace whose clone was deleted by hand reads0 B/{"exclusiveBytes": 0}, not-/null. Making_size_cell/_disk_reportreturn-/Nonefor a non-existent clone leaves the full suite green (re-derived by me, below). Nothing at thedl.pyboundary holds "measured zero" apart from "not measurable", and #159 classifies exactly those directories.0 Bis the right answer; it deserves a pin. - non-blocking — the vanishing-entry downgrade: the argument holds. The two situations are distinguished at the right seam —
FileNotFoundError(the bytes no longer exist and free nothing) versus any otherOSError(the bytes exist behind a door). ENOENT cannot mask EACCES, so no uncertainty is laundered. A rename mid-walk can under-count, never over-count, which is the safe direction for a--prunethat must not over-claim reclaimable bytes. - nit — timings supported, upper bound not conservative. Entry counts reproduce exactly (
~/.pixi= 114,817). This axis's warm five-run range on~/.pixiwas 240.5–274.5 ms against README's quoted 232–239 ms. README hedges ("one machine's numbers… yours will differ"), so this is a nit. - nit — three names for one concept. Spec: "JSON field name and human rendering consistent with the existing table." Flag
--size, columnSIZE, JSON keydisk, valuesexclusiveBytes/atLeastBytes. Defensible (diskis an object, not a size) but not literally consistent. - nit — unknown flags after
--lsare silently ignored (dl.py:2313-2318):dl --ls --sizprints the plain table with no complaint. Pre-existing pattern.
Acceptance criteria met. "a workspace tree of known content reports the expected bytes" → test_a_tree_of_known_content_reports_what_it_holds. "JSON schema gains the field" → disk. "hardlinked pack files are not double-counted" → pinned and mutation-verified. "document the choice in --help and README" → dl.py:2160-2171 + README §"How much disk a workspace costs" (presence satisfied; content disputed by finding 1). "Docker named volumes and image sizes are explicitly out" → zero docker references in the devlaunch/ diff. "must not slow --ls unacceptably" → opt-in plus the spawn-count pin.
Evidence
Re-derived by me, serially, after both subagents finished
Selection for every line in this block: pixi run -e py310 pytest test/ -q (full suite) in a worktree at 6e7e213. Before each mutation, devlaunch/dl.py and devlaunch/disk_usage.py were compared byte-for-byte against git show 6e7e213:<file> and the check printed PRISTINE; after each run, git diff --stat confirmed the mutation was still present.
Baseline: 1197 passed, 23 deselected in 86.09s
The blocking finding — one devpod du per measured workspace injected into _size_cell:
E AssertionError: assert [['list', '--.../repos/myws']] == [['list', '--output', 'json']]
E Left contains one more item: ['du', '/tmp/pytest-of-ags/pytest-413/test_ls_with_sizes_spawns_noth0/devlaunch/repos/myws']
FAILED test/test_devpod_spawn_counts.py::TestHotCommandSpawnCounts::test_ls_with_sizes_spawns_nothing_extra
1 failed, 1196 passed, 23 deselected in 73.80s
The guard now bites the exact regression that sailed past it before. Closed.
Divergence pin — JSON measurability reverted to the metadata record (_disk_report(clone_path)):
E TypeError: 'NoneType' object is not subscriptable
FAILED test/test_workspace_state.py::TestReportingWhatAWorkspaceCostsOnDisk::test_the_two_renderings_measure_the_same_workspaces
1 failed, 1196 passed, 23 deselected in 64.80s
Exclusivity pin — links_here >= links → plain per-tree dedup (an apparent size):
E assert 5246976 < (2 * 1048576)
FAILED test/unit/test_disk_usage.py::TestSharingIsNotBilledTwice::test_a_file_hardlinked_from_outside_is_not_billed_to_the_tree
1 failed, 1196 passed, 23 deselected in 50.68s
Spec finding 3 — _size_cell/_disk_report return -/None for a clone that is not on disk:
1197 passed, 23 deselected in 56.67s
Green: the documented Measured(0) answer for a hand-removed clone is unguarded at the dl.py boundary.
Also run by me, before the subagents were spawned (same worktree, no other writer)
Selection for all eight: pixi run -e py310 pytest test/ -q (full suite); baseline 1197 passed, 23 deselected in 36.17s; tree confirmed clean before and after. Four of these eight are the ones re-derived above and reproduced identically. The other four: vanishing entry re-filed as unreadable → 2 failed, 1195 passed (both TestWhatVanishesMidWalk cases); disk always emitted → 1 failed (test_the_listing_says_nothing_about_disk_unless_asked); size_width heading term removed → 1 failed (test_the_size_column_lines_up_with_its_heading, assert 103 == 106); floor rendered as exclusiveBytes → 2 failed (both floor tests); follow_symlinks=True → 1 failed (test_a_symlink_is_worth_its_own_size_not_its_targets). 12 of 12 mutations bite. I found no hollow pin.
The design crux, rebuilt independently
My own bare + two real git clones (not the builder's cache, not the previous reviewer's). One pack inode 30153421, nlink 3, shared by all three trees:
du -s per tree: bare 1,953,792 ws2 3,600,384 ws3 3,600,384
du -sc all three: 5,492,736
exclusive_usage: bare 122,880 ws2 1,769,472 ws3 1,769,472 (sum 3,661,824)
Per-tree du over-bills a workspace 2.03x and the bare 15.9x; the 1,830,912 B pack is billed to nobody, and exclusive sums to less than the disk holds — both stated consequences reproduced. uptime at the time: load average: 3.35, 2.70, 3.44 (machine under load from sibling agents).
Verified by execution (axis subagents)
- Standards, isolated
git archivetree, full suite: baseline1197 passed; defeating the ownership predicate →2 failed, 1195 passed;disk_usage.py:159→total += 0→1197 passed(the unpinned term, finding 3). - Standards, shared worktree:
pixi run -e py310 ciexit 0 —1197 passed, 23 deselected in 35.09s,devlaunch/disk_usage.py 83 0 100%,TOTAL 2460 164 93%.pixi run -e py310 lintexit 0 — ruffAll checks passed!,tyAll checks passed!, pylint10.00/10. - Spec, shared worktree, full suite: root
FileNotFoundError→PartlyUnreadable→1 failed;describe_usagefloor without≥→2 failed. Timings taken with the shipped walk, warm, five runs after a warm-up,load average: 4.24, 2.77, 3.49:~/.pixientries=114,817 → 274.5, 273.7, 261.0, 243.8, 240.5 ms; repo tree entries=9,479 → 18.8, 17.9, 17.4, 18.4, 18.4 ms. - Grep (reported as grep, not as execution):
39–46,27.6,45.5,224 ms,707 msappear in no.pyor.md; the live353,230,848/68,050,944figures appear in none either (exit 1); no hardcoded/home/<user>in the diff's added lines.
Not run / not verified
./dev.sh/dl-next --ls --sizeagainst a real devpod and a real cache — not run by anyone. Still the builder's own stated gap.- The
kinisi_rosrow (8,309 entries, 24.2–27.7 ms) — not run; no such cache exists on this machine. Its entry count is unverified. The other two timing rows reproduced. - The
2.48 GiB/1.70 GiB/755.0 MiB/488.8 MiBfigures — not verified and not verifiable; the cache they came from has been emptied. That is the substance of Spec finding 1. - Cold-cache timings — not taken (dropping the page cache needs root here), matching the PR's own disclosure.
- Two Spec-axis mutation results were produced in the contaminated window and are not re-derived; they support only nits and carry no weight in the verdict.
- Interaction with #147's
fresh_clone_managerfixture: not investigated beyond noting both touch per-test cache state. No semantic conflict found; none looked for in depth. PR isMERGEABLE/CLEAN.
Verdict
Approve.
The blocking finding is closed. test_ls_with_sizes_spawns_nothing_extra now guards what it advertises: I injected the exact regression that previously sailed past it under every selection — one devpod du per measured workspace — and it failed, under full-suite selection, in a tree verified byte-identical to 6e7e213 immediately beforehand. The fix is the right one: the fixture's clone sits under the patched cache dir with a real payload, and the rendered 2.0 MiB is asserted before the spawn counts, so a - in that cell fails the test rather than silently emptying it. I did not take the builder's hollowness re-audit on trust — I ran 12 mutations of my own choosing across the walk, the renderings, the JSON gating, the column layout and the ownership predicate, all under full-suite selection, and all 12 bite. I found no hollow pin.
Rulings on the three judgement items:
- The measurability predicate is right, not merely consistent. "Source under the cache" is the correct definition of "devlaunch owns this" because it is the same predicate
--purgedestroys by and the same call that producesdevlaunch: true— so"devlaunch": true⟺disk != nullholds by construction rather than by two functions agreeing. Gating on the metadata record was the genuine root cause of the divergence: the record can be absent from a cache that is very much on disk, and it is append-mostly, so it drifts from disk in both directions. #159 inherits a boundary that already matches deletion. One caveat handed forward: the containment test is lexical with noresolve(), which is benign here but must not be reused as-is for #159'sReferenced. - The vanishing-file downgrade holds. A vanished file and an unreadable one are different, and the code now distinguishes them at exactly the right seam: ENOENT means the bytes are gone and free nothing; any other
OSErrormeans the bytes exist behind a door and the answer is a floor. ENOENT cannot mask EACCES, so no uncertainty is laundered — and a mid-walk rename can only under-count, which is the safe direction for a consumer deciding what to reclaim. Making≥the routine answer on a live cache would have degraded the one signalPartlyUnreadableexists to carry. - The retained relabelled figure should go the way of the other two. Retaining it un-hedged in shipped docs is not acceptable. The hedge the builder wrote lives only in the PR body, which no user reads, while
README.md:505-507anddisk_usage.py:16-18state the number in the present tense beside timings that carry full conditions — and488.8 MiBis the sample value in README's own example table. The builder already measured a live, reproducible replacement (du -s353,230,848 B vs 68,050,944 B exclusive on one clone, one 270 MB pack with two links) and shipped it nowhere. This is the one thing that should not reachmainas it stands, and the fix is to move a number that is already written.
I considered blocking on that, and the Spec axis did label it blocking. I am not blocking, for three reasons, and I am recording the override rather than quietly reranking: the claim the figure supports is now independently verified three separate times (my own real-git rebuild at 2.03x/15.9x, the previous review's at ~6.8x, the builder's live 5.2x), so no reader is misled about the decision itself; the previous review graded a strictly worse instance — a figure matching no measurement anywhere — as non-blocking, and blocking a milder one now would be reviewer ratcheting; and the code that merges is correct and, for the first time on this PR, genuinely pinned. Treat it as a pre-merge docs tidy — replace the four stale figures with the live measurement, or hedge them in-doc as the PR body already does — rather than a re-review cycle.
Worth folding in while the branch is open, none of them blocking: known_bytes() has no production caller and its only defence against a consumer printing a floor as a total is a docstring, so it belongs in #159 with the caller that makes it real; the Measured(0) answer for a hand-removed clone is documented but unpinned (a mutation removing it leaves the full suite green) and #159 classifies exactly those directories; disk_usage.py:159's directory-blocks term is likewise unpinned behind a MiB-wide assertion window; and one explanation now exists in five places with the figures in three, which is where the next re-measurement will drift.
The exclusive-vs-apparent case was argued in the shipped docs with four numbers from a cache that has since been emptied -- unverifiable now, and stated in the present tense beside timings that carry their full conditions, so nothing told a reader which was which. One of them was also the sample value in README's example table. The hedge existed only in the pull request body, which nobody installs. They are replaced by the measurement actually taken on this branch, with its conditions: one real clone against `du`, the bare it hardlinks out of, and the two together. It makes the same point with a stronger one -- a 5.2x over-bill traced to a single named pack file with two links -- and the non-summing consequence is now readable off the same four rows instead of asserted. The explanation had accumulated in five places and the figures in three, which is where the next re-measurement drifts. The module docstring is now the one home for why-and-how-much; the rendering adapters and the test modules point at it, and the timings live once, in README. Docs only: no executable line changed.
Four review items on the #171 fix, none of them in the fix itself. `dl --ls --json` gated `unsaved` on the metadata record while `disk` gated on the clone directory, so a clone under the cache with no record printed `devlaunch: true`, a measured `disk`, and `unsaved: null` -- and `null` is documented as "no clone of dl's here". #171's own sentinel, one layer out, and the divergence PR #165 already closed for `disk`. `unsaved`, `path` and `checkedOut` now fall back to `_measurable_clone`, so `unsaved: null` and `devlaunch: false` are the same set by construction. `--work-tree` was load-bearing and unguarded: dropping it left the suite green, but `core.worktree` pointing elsewhere makes `git status --porcelain` print nothing at rc 0 on a clone holding an untracked file. Test added that goes red without the flag. `read_clone` asked `Path.is_dir()`, which cannot express "I was not allowed to look" and answers differently per Python: <=3.12 raised PermissionError out of the listing, 3.13+ returns False and reads as nothing-to-lose. The errno is read directly now. Two docstrings claimed more than was measured: `_git` said all five broken shapes answered about the ancestor under plain `cwd=` (four did; a truncated gitfile is a hard error in git and was never part of the bug), and `CloneState` asserted an invariant its own new test disproves.
1 participant
Closes #153.
dl --ls --sizeanddl --ls --json --sizereport what deleting each workspace's clone would free. This is the measurement primitive the disk work on map #139 needs before anything can be called "reduced", and #159dl --pruneimports it rather than growing its own walk, exactly as #155 required.The decision: exclusive bytes, not apparent size
#154 settled that
.bareis the repo's object cache and every workspace hardlinks out of it. An apparent size (du -sper workspace) bills each workspace for the whole shared pool, so it reports away the saving the sharing exists for — and it gets worse as the design gets better, since #154 extends the same hardlinking to LFS.Measured on one machine — Ubuntu 24.04, ext4, 32 cores, warm page cache, five
runs after a warm-up, machine otherwise busy — with the shipped code, on a real
clone of
kinisi_rosmade by git from the bare cache this branch's own toolingmaintains:
du -s --block-size=1on that same clone says 353,230,848 B; the shippednumber says 68,050,944 B (64.9 MiB) is what deleting it would free. The gap
is one 270 MB pack file with two links — one in the clone, one in the bare — so
removing either frees neither.
du -scover the clone and the bare togethersays the disk really holds 353,882,112 B, and 273 MB of that is the pack
neither tree can free on its own.
Those four rows are what the shipped docs now argue from, in
README.mdand inthe
disk_usagemodule docstring, with the conditions attached. The figures theearlier revision shipped — 2.48 GiB / 1.70 GiB / 755.0 MiB / 488.8 MiB, from a
cache since emptied of its workspace clones — are gone from every shipped
file, including the sample table where
488.8 MiBwas the example value. Theywere unverifiable rather than merely unverified, and a hedge that lives only in
a PR body does not ship.
The claim they supported is independently verified three times over regardless:
this measurement's 5.2x, the first review's rebuilt-from-scratch ~6.8x, and the
second review's own real-
gitrebuild at 2.03x per workspace and 15.9x on thebare.
The honest cost of the choice, stated in the docs rather than hidden: exclusive bytes do not sum to total disk. A pool shared by three workspaces is billed to none of them, because deleting any one frees none of it — it becomes the last holder's the moment it is the last holder. And a workspace's size can change when a sibling is deleted. Both are properties of shared storage; the alternative is a total that over-counts — 5.2x on the clone measured above, 46% on the whole cache in the historical figures.
Documented in
--helpand in a new README section, per the ticket.Opt-in, and why
Measured above: 17–28 ms for a ~9,000-entry clone, warm page cache — under the ticket's ~100 ms trigger for these trees. It is opt-in anyway, and the reasoning is measured rather than assumed:
postCreateCommandrunspixi install) is most of the file count, and a 114,817-entry tree measured 232–239 ms warm on the same machine;dl --lsis today one devpod round-trip and zero filesystem work, pinned intest_devpod_spawn_counts.py, and it stays that way;--lsis not where a third unrelated question goes.--ls --sizeadds no subprocess at all — sizes come fromlstat, neverdu, never docker. That is pinned by a case intest_devpod_spawn_counts.pywhose workspace is genuinely measurable, and the pin is mutation-checked below.No zero for "could not tell"
Principle 3, taken literally.
exclusive_usagereturns a two-armed sum type:Measured(exclusive_bytes)orPartlyUnreadable(at_least_bytes, unreadable). A container writes into its clone as its own user, so a directory this process cannot read is the ordinary case here — and it reports≥2.0 MiBin the table and{"atLeastBytes": …, "unreadable": 1}in JSON, never a total. "The directory is not there" isMeasured(0), which is a real answer and matches whatworkspace_state.read_clonealready says about a clone removed by hand.JSON shape:
diskis absent unless--sizewas passed (so "nobody asked" cannot read as "nothing on disk"),nullfor a workspace devlaunch did not create (the same wayrepoandbranchalready say "not mine"), and otherwise an object with exactly one ofexclusiveBytes/atLeastBytes. A caller cannot arrive at a floor by ignoring a field.Only devlaunch's own clones are walked —
dl ./pathopens somebody's project directory and walking it would be both unbounded and none of devlaunch's business.Out of scope, held
Docker named volumes (#88) and image sizes: untouched, per the map's ruling and #155's. The README says so in one line and points at
docker system df; the--purge/--pruneboundary line itself is #160's.What the review changed
The spawn-count guard was hollow and now bites. Its fixture workspace's
source sat outside the patched cache directory, so
_measurable_clonereturnedNone, the cell printed
-, and the walk never ran — adevpod duper workspacepassed it under every selection. The fixture now puts the clone under the
patched cache with a payload in it, and the rendered size is asserted before the
spawn counts, so a
-in that cell fails the test instead of emptying it.Mutation-checked, full-suite selection (
pixi run -e py310 pytest test/ -q),same tree, injection then revert:
The table and the JSON now measure the same workspaces. "May we measure
this" was answered twice — by the source directory for the table, by the
metadata record for the JSON — and the two already disagreed: a clone under the
cache with no record printed a size and reported
"disk": null, which theREADME defines as "not dl's to measure". Both now ask
_measurable_clone, thesame predicate
--purgedeletes by, and a record is no longer part of thequestion: it can be missing from a cache that is very much on disk. Pinned by
test_the_two_renderings_measure_the_same_workspaces; reverting the fix failsit (
TypeError: 'NoneType' object is not subscriptable) and nothing else.The 39–46 ms figure is gone. It matched no measurement — not this PR's, not
the reviewer's. Everything the docs now quote is in the table above, with the
machine and conditions named, and no cold-cache number is quoted because none
was taken (dropping the page cache needs root here). The README also no longer
attributes a
dufigure to "file lengths":ducounts blocks, which is whatthe code measures.
Also folded in:
known_bytesexports "the bytes, whichever arm" so #159 canorder usages without hand-rolling an
else; a file or directory that vanishesbetween being named and being weighed is now worth nothing rather than unknown,
matching what the root of a walk already answers for a directory that is not
there; and
_size_cellno longer means both "column suppressed" and "notmeasurable" with one string.
Tests
New coverage:
test/unit/test_disk_usage.py(25 cases) — 100% ofdevlaunch/disk_usage.py; the--ls/--ls --jsoncases intest/test_workspace_state.py; the spawn-count pin.The exclusive-vs-apparent choice is still mutation-checked. Replacing the
exclusivity condition with plain per-tree dedup — an apparent size — under
full-suite selection (
pixi run -e py310 pytest test/ -q):Local run,
pixi run -e py310 ciend to end as a single task (the docs tidyadds one rendering case, so 1198):
Timing numbers above are warm page cache on one machine, named with them; no cold-cache measurement was taken, because dropping the page cache needs root on that machine.
🤖 Generated with Claude Code