fix: make StorOps actually work on macOS -- packaging, memory, and APFS duplicate counting - #24
Merged
Merged
Conversation
… 3.9 Two defects that only ever surfaced in an installed StorOps, and were therefore invisible to every test: CI installs with `pip install -e .`, which leaves the repo-root rules/ directory reachable via core/rules.py's development-layout candidate. The wheel shipped no rule files at all. `[tool.setuptools.package-data]` declared `storops = ["rules/*.yaml"]`, but package-data patterns can only match files already inside a package directory and rules/ lives at the repo root -- so the pattern matched nothing, and every command in an installed storops died with "could not locate the rules/ directory". rules/ stays at the root (SKILL.md and README.md point at it by that path, and rules/README.md documents it as where to add rules); it is now mapped in as the `storops.rules` package instead, which needs an explicit `packages` list because `packages.find` cannot discover a package outside `where`. test_packaging.py keeps that list from going stale. requires-python demanded 3.11 for no reason: a stock macOS ships 3.9 as `python3`, RHEL 9 ships 3.9, and the whole suite passes unmodified there. StorOps is most useful on a machine nobody has set a modern toolchain up on yet, so that was a hard stop exactly where it hurt most. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured on a real Mac (macOS 27, APFS, no gdu, so the du backend is what
runs), `storops scan ~` over a 157GB/2.44M-file home directory:
before after
wall clock 104.5s 64.0s
peak RSS 2,613 MB 17.6 MB
du output rows 2,442,932 ~75
os.path.isdir() ~2.44M 0
and `storops scan /`: 164.4s -> 98.5s, with /System dropping from 95.0G
to 72.9G as the duplicate counting went away. Top-15 ordering is
unchanged throughout; sizes match to within the churn of live caches
being written during the runs.
Three separate causes.
BSD du's -a and -d depth are mutually exclusive (`usage: du [-a | -s | -d
depth]`), so the BSD branch always ran `-a` at unbounded depth -- even
for scan/inspect, which only ever want directories one level down. Both
flavors report directories only when `-a` is omitted AND accept a native
depth limit, so the fix is simply not to ask for file-level rows when the
caller did not want them. That alone is the 2.44M rows -> ~75.
The whole listing was captured with subprocess.run(capture_output=True)
and then splitlines()'d, holding hundreds of MB of text plus the list
built from it. Now streamed line by line through Popen. StorOps is most
likely to be run on a machine that is already out of disk and under
memory pressure, so that was exactly the wrong trade.
`/Users` and `/System/Volumes/Data/Users` are literally the same
directory -- same st_dev, same st_ino, neither one a symlink -- because
`/` is the sealed System volume with firmlinks projecting the writable
Data volume into it. `du /` therefore walks the user's entire home twice,
and `du -x` cannot help: a volume group reports one st_dev throughout, so
there is no device boundary to stop at. Darwin's fcntl(F_GETPATH) is the
only thing that reports a firmlink's real identity (os.path.realpath does
not resolve them); a pre-pass uses it to find all 18 firmlink entry
points under `/` in 0.07s and prunes them before du is handed them. The
Data volume's contribution drops from du's 309G to its actual 3.5G.
Sizing depth-1 children as independent `du -s` processes is what makes
the pruning possible and also removes the single-threaded-du bottleneck:
99.9s -> 66.0s at 8 workers (16 gave nothing back, the walk being bound
by filesystem metadata I/O). Known trade-off, documented at the call
site: N separate du runs cannot dedupe a hardlink the way one run can.
SKILL.md gains a rule about macOS totals: firmlinks are now pruned, but
APFS block sharing (clones, Time Machine local snapshots) remains and no
per-path size can attribute shared blocks to one path, so the agent must
not present per-directory sizes as something that should add up to the
volume's used space.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sizing one probe path means walking its whole subtree, so a cleanup plan is almost entirely time spent waiting on filesystem metadata -- and all of it was serialized. Measured on a real Mac: 6.32s for nine probes, down to 2.80s. Opt-in per backend rather than assumed. The Windows native backend resets per-call state on `self` (its warnings list) and already splits its own walk across threads internally, so sizing several paths at once through it would both clobber that state and oversubscribe the disk. DuBackend and GduBackend declare `path_size_is_concurrent`; anything that does not keeps the sequential path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
On a Mac used for development the largest reclaimable directories were all classified unknown/critical, which meant StorOps could surface tens of GB in a scan and had nothing to say about any of it. Identification of typical macOS paths goes from 7/18 to 19/20 in a spot check. Added: Xcode DerivedData, DeviceSupport and Archives; CoreSimulator Devices and Caches; the Homebrew cache; ~/Library/Logs. Plus Gradle and Cargo, which were missing from the rule base on every platform. Two of these are deliberately never offered for deletion despite sitting beside things that are. Xcode Archives are shipping artifacts and the only way to symbolicate crash reports from a released build, so they are deletable: false. CoreSimulator Devices are classified high risk -- they are identified, but `cleanup plan` will not propose them, because deleting those directories by hand leaves CoreSimulator's own device index dangling; `xcrun simctl delete unavailable` is the right tool and the rule's notes say so. Also corrects rules/README.md and macos.yaml's header, both of which still claimed the ai-models/applications/caches rule files were Windows-token-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every defect fixed in this branch was invisible to CI, and each one had the same shape: the job did not exercise the configuration real users run. The macOS job installs gdu, so BSD `du` -- the backend on any Mac that has not gone out of its way to install gdu -- was never under test. That is where macOS' own quirks live. A second macOS job now asserts gdu is absent and that get_scan_backend() really did pick Du. Only 3.11 was tested, while the declared floor is now 3.9 (a stock macOS `python3`) and 3.13 is what a new machine gets. Both are in the matrix; the middle of the range is left to those two to bracket. `pip install -e .` leaves the repo-root rules/ directory reachable, so no job could tell whether the built distribution actually shipped the rule files. It did not. A third job builds the wheel, installs it into a venv where the source tree cannot be reached, and runs real commands through the installed console script. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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 is
An audit of StorOps' runtime behaviour on macOS, and the fixes for what it
turned up. Short version: it did not meet expectations, and one defect made
an installed StorOps completely non-functional on every platform.
All numbers below are measured on a real Mac (macOS 27, APFS, no gdu on
PATH -- so the
dubackend is what runs, which is the default on any Macthat has not gone out of its way to install gdu).
scan ~(157 GB / 2.44M files)scan ~peak RSSscan ~du output rows /isdir()callsscan /scan /reported size of/Systemcleanup planpip install storopsTop-15 ordering is unchanged throughout; individual sizes match to within
the churn of live caches being written during the runs.
The five root causes
1.
rules/*.yamlwere not in the wheel.package-datapatterns canonly match files already inside a package directory, and
rules/lives atthe repo root -- so
storops = ["rules/*.yaml"]matched nothing and aninstalled StorOps died with "could not locate the rules/ directory" on
every command. Invisible to CI because
pip install -e .leaves therepo-root
rules/reachable throughcore/rules.py's development-layoutcandidate.
rules/stays where it is (SKILL.md, README.md andrules/README.md all point at that path); it is mapped in as the
storops.rulespackage instead.2.
requires-python = ">=3.11"while a stock macOS ships 3.9 aspython3and RHEL 9 ships 3.9. The whole suite passes unmodified on 3.9,so this was a hard stop exactly where StorOps is most useful -- a machine
nobody has set a modern toolchain up on. Floor lowered to 3.9.
3. BSD
du's-aand-d depthare mutually exclusive(
usage: du [-a | -s | -d depth]), so the BSD branch always ran-aatunbounded depth -- even for
scan/inspect, which only ever wantdirectories one level down. 2.44M output rows and an
os.path.isdir()perrow, to produce 15. Both flavors report directories only when
-aisomitted and accept a native depth limit, so the fix is to stop asking for
file-level rows the caller never wanted. The listing was also captured
whole via
capture_output=Trueand thensplitlines()'d; it is streamednow. StorOps is most likely to run on a machine that is already out of disk
and under memory pressure, so holding 300 MB of text was the wrong trade.
4. APFS firmlinks made
scan /wrong./Usersand/System/Volumes/Data/Usersare literally the same directory -- samest_dev, samest_ino, neither a symlink -- because/is the sealedSystem volume with firmlinks projecting the writable Data volume into it.
du /walks the user's entire home twice.du -xcannot help: a volumegroup reports one
st_devthroughout, so there is no device boundary tostop at. Darwin's
fcntl(F_GETPATH)is the only thing that reports afirmlink's real identity (
os.path.realpathdoes not resolve them); apre-pass uses it to find all 18 firmlink entry points under
/in 0.07 sand prunes them before
duis handed them. The Data volume's contributiondrops from du's 309 G to its actual 3.5 G.
5. CI never ran the configuration real users have. The macOS job
installs gdu, so BSD
duwas never under test -- which is how all of theabove shipped. Now: a 3.9/3.13 matrix, a second macOS job that asserts gdu
is absent and that
get_scan_backend()really pickedDu, and a job thatbuilds the wheel and runs commands from a venv where the source tree is
unreachable.
Design notes for the reviewer
Depth-1 children are sized as independent
du -sprocesses. This iswhat makes alias pruning possible at all (
duhas no way to be told "youalready counted that directory under another name"), and it also removes
the single-threaded-
dubottleneck: 99.9 s → 66.0 s at 8 workers. 16workers gave nothing back (70.7 s) -- the walk is bound by filesystem
metadata I/O, not CPU. Falls back to a single
du -d 1past 512 children,where process spawns stop paying for themselves.
Known trade-off, documented at the call site: N separate
durunscannot dedupe a hardlinked file the way one run can. Checked against
du -d 1on a real home directory, top-15 ordering was identical and everysize matched within live-cache churn. This is why it is a depth-1-only path
rather than the default everywhere.
Accepted limitation:
searchstill walks at arbitrary depth throughplain
du, so on a macOS root it can still double-count. Pruning coversscan/inspect.SKILL.mdgains a rule telling the agent not to present macOSper-directory sizes as something that should add up to the volume's used
space: firmlinks are pruned now, but APFS block sharing (file clones, Time
Machine local snapshots) remains, and no per-path size can attribute shared
blocks to one path.
Two rules are deliberately never offered for deletion despite sitting
next to things that are. Xcode Archives are shipping artifacts and the only
way to symbolicate crash reports from a released build (
deletable: false).CoreSimulator Devices are classified high risk, so
cleanup planidentifiesbut never proposes them -- deleting those directories by hand leaves
CoreSimulator's own device index dangling, and
xcrun simctl delete unavailableis the right tool.Not in this PR
Six rules (
macos-user-ssh,macos-user-documents-desktop, their Linux andWindows counterparts, and
downloads-folder) setcategory: unknowntoforce a CHECK recommendation.
recommended_action()branches on thatcategory first and returns "No identification rule matched this path", so
asking "is it safe to delete ~/.ssh?" reports that nothing matched and hides
the rule's own consequence text ("can lock the user out of every host it has
keys for"). The outcome is still safe -- nothing gets deleted -- but the
explanation the agent relays is wrong, and SKILL.md tells the agent to relay
Category and Consequence verbatim. Fixing it means touching
risk.py, whosedocstring says the safety model has "no room for creative changes", so it is
left for a focused change of its own.
Verification
229 tests pass on Python 3.9, 3.12 and 3.13. The wheel installs into a clean
3.9 venv and
identify/scan/cleanup planall work end to end throughthe installed console script.
🤖 Generated with Claude Code