From b039495985d9b175d4db6eaa60be3ce4f67c9f34 Mon Sep 17 00:00:00 2001 From: tzzs Date: Sun, 20 Sep 2026 20:01:40 +0800 Subject: [PATCH 1/5] fix(packaging): ship rules/*.yaml in the wheel and lower the floor to 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 --- README.md | 8 +++-- README.zh-CN.md | 7 ++-- pyproject.toml | 34 +++++++++++++++--- tests/unit/test_packaging.py | 69 ++++++++++++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 10 deletions(-) create mode 100644 tests/unit/test_packaging.py diff --git a/README.md b/README.md index 9ca8ea2..60c3b84 100644 --- a/README.md +++ b/README.md @@ -51,8 +51,10 @@ coverage. ## Requirements -- **Python 3.11+** — the only implementation (`src/storops/`); `python3`/ - `python` needs to be on `PATH`. No `pip install` is required for the +- **Python 3.9+** — the only implementation (`src/storops/`); `python3`/ + `python` needs to be on `PATH`. 3.9 is the floor deliberately: it is what + a stock macOS ships as `python3`, and StorOps is most useful on a machine + nobody has set a modern toolchain up on yet. No `pip install` is required for the common "cloned into a skills directory" install path — `python -m storops` works straight out of the checkout. - **Windows**: NTFS volumes. [WizTree](https://diskanalyzer.com/) is @@ -79,7 +81,7 @@ StorOps is a plain agent skill: a directory with a `SKILL.md` at its root, discovered by name and description rather than invoked as a slash command. No build step and no `pip install` required — the agent reads `SKILL.md` to decide when to use the skill, then invokes `python -m storops ` -directly. The only runtime requirements are Python 3.11+ and, on Windows, +directly. The only runtime requirements are Python 3.9+ and, on Windows, WizTree — see [Requirements](#requirements) above. ### Ask your agent to install it (recommended) diff --git a/README.zh-CN.md b/README.zh-CN.md index 42f223c..9c0948b 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -45,8 +45,9 @@ Windows token;只有关键系统路径规则(`rules/windows.yaml`/`linux.yaml`/ ## 环境要求 -- **Python 3.11+**——唯一的实现(`src/storops/`);`python3`/`python` - 需要在 `PATH` 上。最常见的"克隆进 skills 目录"安装方式不需要 +- **Python 3.9+**——唯一的实现(`src/storops/`);`python3`/`python` + 需要在 `PATH` 上。下限定在 3.9 是刻意的:macOS 自带的 `python3` 就是 + 3.9,而 StorOps 最该派上用场的,恰恰是还没配好现代工具链的机器。最常见的"克隆进 skills 目录"安装方式不需要 `pip install`——从 checkout 目录直接运行 `python -m storops` 即可。 - **Windows**:NTFS 卷。[WizTree](https://diskanalyzer.com/) 是可选的—— StorOps 自带的原生扫描(`os.scandir`,以工作队列在整棵扫描树内并行; @@ -67,7 +68,7 @@ Windows token;只有关键系统路径规则(`rules/windows.yaml`/`linux.yaml`/ StorOps 是一个标准的 agent skill:一个根目录带有 `SKILL.md` 的目录,agent 依据 其 name/description 自动发现并调用,而非以 slash command 的形式手动触发。无需 构建步骤,也无需 `pip install`——agent 会读取 `SKILL.md` 来判断何时使用该 -skill,然后直接调用 `python -m storops `。运行时依赖是 Python 3.11+, +skill,然后直接调用 `python -m storops `。运行时依赖是 Python 3.9+, 以及在 Windows 上的 WizTree,详见上方[环境要求](#环境要求)。 ### 直接让 Agent 帮你安装(推荐) diff --git a/pyproject.toml b/pyproject.toml index 6fdbbd6..892f031 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,11 +7,18 @@ name = "storops" version = "2.0.0a0" description = "Storage Operations for AI Agents -- cross-platform disk/storage diagnosis, cleanup, and migration CLI." readme = "README.md" -requires-python = ">=3.11" +# 3.9 is the floor because that is what a stock macOS ships as +# `python3` (Xcode CLT) and what RHEL 9 ships -- StorOps is most useful on +# a machine the user has NOT already set up a modern toolchain on, so +# demanding 3.11 there turned `pip install storops` into a hard stop for +# no reason: the whole test suite passes unmodified on 3.9, and CI now +# pins the floor so it stays that way. +requires-python = ">=3.9" license = { text = "MIT" } authors = [{ name = "tzzs" }] classifiers = [ "Programming Language :: Python :: 3", + "Programming Language :: Python :: 3.9", "Operating System :: OS Independent", "Environment :: Console", ] @@ -23,11 +30,30 @@ dev = ["pytest>=8"] [project.scripts] storops = "storops.cli:main" -[tool.setuptools.packages.find] -where = ["src"] +# `rules/` deliberately lives at the repo root, not under src/storops/: it +# is agent-facing content that SKILL.md/README.md point at by that path, +# and rules/README.md documents it as the place to add rules. Mapping it +# in as the `storops.rules` package is what gets those YAML files into the +# wheel -- without this, `pip install storops` produced a package whose +# every command died with "could not locate the rules/ directory", since +# package-data patterns can only ever match files that are already inside +# a package directory. An explicit `packages` list is required because +# `packages.find` cannot discover a package that lives outside `where`; +# tests/unit/test_packaging.py keeps the list from drifting. +[tool.setuptools] +package-dir = { "" = "src", "storops.rules" = "rules" } +packages = [ + "storops", + "storops.core", + "storops.output", + "storops.platform", + "storops.platform.backends", + "storops.platform.windows", + "storops.rules", +] [tool.setuptools.package-data] -storops = ["rules/*.yaml"] +"storops.rules" = ["*.yaml"] [tool.pytest.ini_options] testpaths = ["tests"] diff --git a/tests/unit/test_packaging.py b/tests/unit/test_packaging.py new file mode 100644 index 0000000..5cc27ca --- /dev/null +++ b/tests/unit/test_packaging.py @@ -0,0 +1,69 @@ +"""Guards on pyproject.toml's distribution metadata. + +These exist because the packaging bugs they cover are invisible to every +other test in this suite: CI installs the project with `pip install -e .`, +which leaves the repo-root `rules/` directory reachable via +core/rules.py's development-layout candidate. A real `pip install storops` +does not -- and shipped a wheel with no rule files at all, so every single +command failed with "could not locate the rules/ directory". +""" +from __future__ import annotations + +import sys +from pathlib import Path + +if sys.version_info >= (3, 11): + import tomllib +else: # pragma: no cover - 3.9/3.10 runners + import pytest + + tomllib = pytest.importorskip("tomli", reason="needs tomllib (3.11+) or tomli") + +REPO_ROOT = Path(__file__).resolve().parents[2] +SRC = REPO_ROOT / "src" + + +def _pyproject() -> dict: + with (REPO_ROOT / "pyproject.toml").open("rb") as handle: + return tomllib.load(handle) + + +def _declared_packages() -> list[str]: + return _pyproject()["tool"]["setuptools"]["packages"] + + +def test_rules_directory_is_mapped_into_the_wheel(): + setuptools_config = _pyproject()["tool"]["setuptools"] + assert setuptools_config["package-dir"]["storops.rules"] == "rules" + assert "storops.rules" in setuptools_config["packages"] + assert setuptools_config["package-data"]["storops.rules"] == ["*.yaml"] + + +def test_every_rule_file_the_engine_loads_is_shipped(): + from storops.core.rules import _RULE_FILE_ORDER + + for filename in _RULE_FILE_ORDER: + assert (REPO_ROOT / "rules" / filename).is_file(), filename + + +def test_declared_packages_cover_every_package_under_src(): + """`packages.find` had to be replaced by an explicit list to map + `storops.rules` in from outside `src/` (see pyproject.toml's comment). + That trades discovery for a list that can silently go stale, so a new + subpackage that nobody remembers to add here -- and would therefore be + missing from the wheel -- fails right here instead. + """ + on_disk = { + ".".join(path.parent.relative_to(SRC).parts) + for path in SRC.rglob("__init__.py") + } + missing = on_disk - set(_declared_packages()) + assert not missing, f"not listed in pyproject.toml [tool.setuptools].packages: {sorted(missing)}" + + +def test_requires_python_floor_matches_the_running_interpreter_support(): + """StorOps must stay installable on a stock macOS `python3` (3.9) and + RHEL 9 (3.9); raising this floor is a deliberate decision, not a + drive-by. + """ + assert _pyproject()["project"]["requires-python"] == ">=3.9" From 15776681d990bdb2b98da1b2a0c5b9a1e43fab53 Mon Sep 17 00:00:00 2001 From: tzzs Date: Sun, 20 Sep 2026 20:01:58 +0800 Subject: [PATCH 2/5] perf(du): fix the macOS scan path -- 2.6GB peak RSS and duplicate totals 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 --- SKILL.md | 15 +- src/storops/platform/backends/du.py | 487 ++++++++++++++++++++++++---- tests/unit/test_du_backend.py | 115 ++++++- tests/unit/test_du_depth_one.py | 192 +++++++++++ 4 files changed, 724 insertions(+), 85 deletions(-) create mode 100644 tests/unit/test_du_depth_one.py diff --git a/SKILL.md b/SKILL.md index 756cd6b..d8b4ba6 100644 --- a/SKILL.md +++ b/SKILL.md @@ -79,11 +79,24 @@ commands ad hoc; the user should not need to know command names. (e.g. "by the way, installing gdu would make these scans noticeably faster") -- don't repeat it on every single command, and don't mention it at all on Windows or when it's `null`. +14. On macOS, a scan's per-directory sizes will not add up to the volume's + used space, and you must not present them as if they should. Two + separate reasons: StorOps already prunes APFS firmlinks (`/Users` and + `/System/Volumes/Data/Users` are literally the same directory, and an + unpruned `du /` counts the user's whole home twice), but APFS *block + sharing* -- file clones, and Time Machine local snapshots -- remains, + and no per-path size can attribute shared blocks to one path. Report + the ranking and the individual sizes, which are sound; do not compute + "everything else" by subtracting the total from the drive's used + figure, and do not tell the user their disk is lying to them. ## Workflow: "why is my drive full?" 1. `storops scan C:\` (or the drive the user mentioned) for top-level - consumers and free space. + consumers and free space. On macOS scan `/` -- not `/System/Volumes/Data`: + StorOps prunes the Data volume's firmlinked duplicates when it is handed + the real root, and scanning the Data volume directly reports the same + content under its uglier internal paths. 2. To see what's inside several large entries at once, prefer one `storops search --folders --max-depth 2` (bump to `3` if two levels isn't enough) over `storops inspect`-ing each one individually. `inspect` diff --git a/src/storops/platform/backends/du.py b/src/storops/platform/backends/du.py index f4a17ec..1dda3de 100644 --- a/src/storops/platform/backends/du.py +++ b/src/storops/platform/backends/du.py @@ -13,13 +13,26 @@ from __future__ import annotations import os +import platform as _platform import subprocess +from collections import deque +from concurrent.futures import ThreadPoolExecutor from fnmatch import fnmatch +from typing import Iterator from storops.core.errors import InvalidPathError, PermissionDeniedError from storops.core.models import Entry, ScanWarning from storops.core.paths import resolve_path +try: + import fcntl +except ImportError: # pragma: no cover - Windows has no fcntl + # This module is POSIX-only at runtime (platform/base.py never selects + # it on Windows), but the test suite imports it everywhere in order to + # collect its skip markers -- a hard import here would turn every + # Windows CI run into a collection error rather than a set of skips. + fcntl = None # type: ignore[assignment] + _DU_FALLBACK_ADVICE = ( "Install gdu for noticeably faster scans on large directory trees: " "https://github.com/dundee/gdu#installation" @@ -30,11 +43,148 @@ def _split_segments(path: str) -> list[str]: return [p for p in path.replace("\\", "/").split("/") if p] +# --- Depth-1 child sizing, with volume-alias pruning ------------------------ + +# Beyond this many immediate children, sizing each one with its own `du -s` +# costs more in process spawns than the parallelism buys back, so +# top_entries() falls back to a single `du -d 1` over the whole target. +_MAX_PARALLEL_CHILDREN = 512 + +# Measured sweet spot for concurrent `du -s` processes on a real macOS home +# directory: 8 workers took 99.9s down to 66.0s, 16 gave nothing back (70.7s) +# -- the walk is bound by filesystem metadata I/O, not by CPU. +_DU_CONCURRENCY = 8 + +# macOS' firmlinked directories all live under this one mount, so the alias +# pre-pass only ever walks toward and inside it. Everything big gets pruned +# at its entry point (a firmlink is recognized before it is descended into), +# which is what keeps the pre-pass to well under a second on a full disk. +_DARWIN_DATA_VOLUME = "/System/Volumes" + +# Guards against a pathological tree, not tuning knobs. +_ALIAS_SCAN_MAX_DEPTH = 12 +_ALIAS_SCAN_BUDGET = 50_000 + +# Darwin fcntl(2) command: write the file descriptor's canonical path into +# the supplied buffer. Stable since OS X 10.5 (sys/fcntl.h), and the only +# thing that reports a firmlink's real identity -- os.path.realpath() does +# not resolve them, and neither st_dev (identical across a volume group) +# nor a symlink check can see them at all. +_F_GETPATH = 50 +_F_GETPATH_BUFSIZE = 1024 + + +def _canonical_path(path: str) -> str | None: + """The kernel's own canonical path for `path`, or None if it cannot be + determined (no permission, not Darwin, path vanished mid-scan). + """ + if fcntl is None: + return None + try: + fd = os.open(path, os.O_RDONLY | getattr(os, "O_DIRECTORY", 0)) + except OSError: + return None + try: + raw = fcntl.fcntl(fd, _F_GETPATH, b"\0" * _F_GETPATH_BUFSIZE) + except (OSError, ValueError): + return None + finally: + os.close(fd) + return raw.split(b"\0", 1)[0].decode(errors="replace") or None + + +def _alias_scan_is_warranted(target: str) -> bool: + """True where the filesystem is known to expose the same directory + under more than one path below `target`. + + This is macOS' APFS volume-group topology, and only it: `/` is the + sealed, read-only System volume, and the writable Data volume is + mounted at /System/Volumes/Data and *also* projected into `/` by + firmlinks. So `/Users` and `/System/Volumes/Data/Users` are literally + the same directory -- same st_dev, same st_ino, neither one a symlink + -- and a plain `du /` walks the user's entire home twice and reports + a total to match. `du -x` cannot help: a volume group reports one + st_dev throughout, so there is no device boundary to stop at. + + Restricted to the volume roots rather than run on every scan: a scan + of e.g. a home directory has nothing for the pre-pass to find. + """ + if _platform.system() != "Darwin": + return False + head = target.rstrip("/") or "/" + return head == "/" or os.path.dirname(head) in ("/Volumes", _DARWIN_DATA_VOLUME) + + +def _alias_pre_pass_descends_into(path: str) -> bool: + """Keep the pre-pass on the one branch that can contain firmlinks: + /System/Volumes and everything under it (plus the ancestors needed to + reach it). Without this the walk would wander into the user's home + looking for aliases that, by construction, are never there. + """ + head = path.rstrip("/") or "/" + # "/" is its own separator, so the usual head + "/" would build "//". + prefix = head if head == "/" else head + "/" + return ( + head == _DARWIN_DATA_VOLUME + or head.startswith(_DARWIN_DATA_VOLUME + "/") + or _DARWIN_DATA_VOLUME.startswith(prefix) + ) + + +def _alias_paths(target: str) -> set[str]: + """Directories under `target` that the kernel reports as living at a + different canonical path -- i.e. a second route to content already + counted under that canonical path, which `du` would otherwise walk and + add in twice. + + An alias is never descended into, so the expensive subtrees + (/System/Volumes/Data/Users and friends) are recognized and dropped at + their entry point rather than walked. + """ + aliases: set[str] = set() + frontier = deque([(target.rstrip("/") or "/", 0)]) + budget = _ALIAS_SCAN_BUDGET + + while frontier: + current, depth = frontier.popleft() + if depth >= _ALIAS_SCAN_MAX_DEPTH: + continue + try: + with os.scandir(current) as it: + for entry in it: + if budget <= 0: + return aliases + try: + if not entry.is_dir(follow_symlinks=False): + continue + except OSError: + continue + budget -= 1 + canonical = _canonical_path(entry.path) + if canonical is not None and canonical != entry.path: + aliases.add(entry.path) + continue + if _alias_pre_pass_descends_into(entry.path): + frontier.append((entry.path, depth + 1)) + except OSError: + continue + + return aliases + + class DuBackend: """ScanBackend implementation shelling out to the system `du`.""" name = "Du" + # core/cleanup.py sizes every probe path it found; each one is an + # independent `du -s` subprocess here, so they are safe to run + # concurrently (no shared mutable state on this instance -- _flavor is + # write-once-with-the-same-value, and take_warnings() is a constant). + # Backends that keep per-call state (e.g. the Windows native backend's + # self._warnings) must NOT set this. + path_size_is_concurrent = True + def __init__(self) -> None: self._flavor: str | None = None # "gnu" | "bsd", cached per instance @@ -50,6 +200,78 @@ def _du_flavor(self) -> str: self._flavor = "bsd" return self._flavor + def _du_args(self, target: str, *, dirs_only: bool, max_depth: int) -> list[str]: + """Build the `du` command line for this flavor. + + `dirs_only` (the caller wants directories and no file-level rows -- + i.e. every scan()/inspect() call, since both go through + top_entries(include_files=False)) is the case worth special-casing: + without `-a`, BOTH flavors report directories only AND accept a + native depth limit, so du prints tens of rows instead of millions + and StorOps never has to classify or even look at a single file. + On a real macOS home directory that is the difference between + 2.4M output rows / ~2.6GB peak RSS and ~75 rows / a few MB, for + byte-identical results. + + With `-a` the two flavors diverge. GNU takes `-a` and + `--max-depth` together. BSD's are mutually exclusive + (`usage: du [-a | -s | -d depth]`) -- combining them always fails + with a usage error (exit 64), and `-d depth` alone reports only + directory totals, never individual files (confirmed against a real + macOS runner), so a caller that needs files at a limited depth + gets `-a` at unbounded depth here and the depth filter in scan() + does the real depth-limiting instead. + """ + if self._du_flavor() == "gnu": + # -b = --apparent-size --block-size=1: logical/apparent size in + # bytes, comparable to WizTree's "Size" column (not its "Allocated"). + args = ["du", "-b"] + if not dirs_only: + args.append("-a") + if max_depth > 0: + args.append(f"--max-depth={max_depth}") + else: + # BSD/macOS du has no portable apparent-size-in-bytes flag; + # report 1024-byte blocks (-k) and scale in scan(). This is + # disk-usage, not apparent size, on this flavor -- a known, + # documented approximation. + args = ["du", "-k"] + if dirs_only: + if max_depth > 0: + args.extend(["-d", str(max_depth)]) + else: + args.append("-a") + args.extend(["--", target]) + return args + + @staticmethod + def _du_lines(args: list[str]) -> Iterator[str]: + """Stream `du`'s stdout line by line. + + Streaming rather than subprocess.run(capture_output=True) is the + point: a `du -a` over a large home directory emits hundreds of MB + of text, and capturing it whole (plus the list splitlines() builds + from it) was measured at ~2.6GB peak RSS for a scan that returns + 15 rows. StorOps is most likely to be run on a machine that is + already out of disk and under memory pressure, so holding the + whole listing in memory is exactly the wrong trade. + + stderr is discarded, matching Du.psm1's `2>$null`: a + permission-denied subtree during `du` is silently skipped rather + than surfaced as a structured warning -- see take_warnings(). + """ + proc = subprocess.Popen( + args, stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, text=True + ) + try: + assert proc.stdout is not None + for line in proc.stdout: + yield line + finally: + if proc.stdout is not None: + proc.stdout.close() + proc.wait() + def scan( self, path: str, @@ -69,89 +291,67 @@ def scan( raise InvalidPathError(f"StorOps: '{target}' does not exist.") flavor = self._du_flavor() - if flavor == "gnu": - # -b = --apparent-size --block-size=1: logical/apparent size in - # bytes, comparable to WizTree's "Size" column (not its "Allocated"). - args: list[str] = ["du", "-a", "-b"] - if max_depth > 0: - args.append(f"--max-depth={max_depth}") - else: - # BSD/macOS du has no portable apparent-size-in-bytes flag; - # report 1024-byte blocks (-k) and scale below. This is - # disk-usage, not apparent size, on this flavor -- a known, - # documented approximation. - # - # BSD du's -a, -s, and -d depth are mutually exclusive - # (`usage: du [-a | -s | -d depth]`) -- combining -a with -d - # always fails with a usage error (exit 64), which used to be - # misreported below as "permission denied". Unlike GNU, BSD's - # -d depth alone reports only directory totals, never - # individual files (confirmed against a real macOS runner -- - # dropping -a silently loses every file-level entry, which - # broke callers that need files at a limited depth, e.g. - # top_entries/path_size). There is no single BSD du invocation - # that gives both file-level entries and a native depth limit, - # so on this flavor we always run -a (every file, unbounded - # depth) and let the depth filter below -- originally just a - # belt-and-braces guard for GNU -- do the real depth-limiting - # for BSD instead. - args = ["du", "-k", "-a"] - args.extend(["--", target]) + dirs_only = export_folders and not export_files + args = self._du_args(target, dirs_only=dirs_only, max_depth=max_depth) - # stderr is discarded, matching Du.psm1's `2>$null`: a - # permission-denied subtree during `du` is silently skipped rather - # than surfaced as a structured warning. This is an acceptable, - # documented v1 limitation -- see take_warnings() below. - proc = subprocess.run(args, capture_output=True, text=True) - raw = proc.stdout - if proc.returncode != 0 and not raw: - raise PermissionDeniedError( - f"StorOps: du exited with code {proc.returncode} scanning '{target}' " - "(permission denied on a subtree? re-run the whole command under sudo " - "-- StorOps never self-elevates)." - ) + # Depth is limited natively except on BSD-with-`-a` (see _du_args); + # computing a row's depth means splitting its whole path into + # segments, which is real per-row work at scale, so it is skipped + # whenever du already did the limiting. + filter_depth = max_depth if (max_depth > 0 and not dirs_only and flavor != "gnu") else 0 + root_segments = len(_split_segments(target)) if filter_depth else 0 + scale = 1 if flavor == "gnu" else 1024 - root_segments = len(_split_segments(target)) + # In `-a` mode du emits a directory only AFTER everything inside + # it, so by the time a directory's own row arrives it has already + # been recorded here as some child's parent -- that makes the + # os.path.isdir() stat below unnecessary for every non-empty + # directory. (An *empty* directory is never any row's parent and + # is indistinguishable from a file in du's output, so those still + # get stat'ed; they are a rounding error next to the file rows.) + known_dirs: set[str] = set() entries: list[Entry] = [] + saw_any_row = False - for line in raw.splitlines(): - if not line: - continue - parts = line.split("\t", 1) - if len(parts) < 2: + for line in self._du_lines(args): + size_text, tab, entry_path = line.partition("\t") + if not tab: continue + saw_any_row = True + entry_path = entry_path.rstrip("\n") + + if not dirs_only: + parent = entry_path.rpartition("/")[0] + if parent: + known_dirs.add(parent) - size = int(parts[0]) - if flavor != "gnu": - size *= 1024 - entry_path = parts[1] if entry_path == target: continue - # Belt-and-braces: du was already asked to stop at max_depth, - # this just guards against any flavor quirk that returns deeper - # rows. Computing a row's depth is itself real per-row work - # (splitting the whole path into segments) that matters at - # scale, so it's skipped entirely for the common max_depth=0 - # (unlimited) case, where the check below is a no-op anyway. - if max_depth != 0: + if filter_depth: depth = len(_split_segments(entry_path)) - root_segments - if depth > max_depth: + if depth > filter_depth: continue - is_folder = os.path.isdir(entry_path) - if (is_folder and not export_folders) or ((not is_folder) and not export_files): - continue - - # Likewise: basename() is only ever needed for these two - # filters, so skip it when neither was given. + # Cheapest discriminators first: the name filters are pure + # string work, while is_folder can still cost a stat(). if name_filter or name_exclude: - name = os.path.basename(entry_path) + name = entry_path.rpartition("/")[2] if name_filter and not fnmatch(name, name_filter): continue if name_exclude and fnmatch(name, name_exclude): continue + if dirs_only: + is_folder = True + elif entry_path in known_dirs: + is_folder = True + else: + is_folder = os.path.isdir(entry_path) + if (is_folder and not export_folders) or ((not is_folder) and not export_files): + continue + + size = int(size_text) * scale entries.append( Entry( full_name=entry_path, @@ -164,6 +364,145 @@ def scan( ) ) + if not saw_any_row: + raise PermissionDeniedError( + f"StorOps: du produced no output scanning '{target}' " + "(permission denied on a subtree? re-run the whole command under sudo " + "-- StorOps never self-elevates)." + ) + + return entries + + def _entry_size(self, dir_entry: os.DirEntry) -> int: + """Size of one already-scandir'd file, in the same units this + flavor's `du` reports: apparent bytes for GNU `-b`, allocated + bytes (st_blocks is always 512-byte units, regardless of -k) for + BSD. Mixing the two would make a directory listing's file rows + incomparable with its folder rows. + """ + try: + stat_result = dir_entry.stat(follow_symlinks=False) + except OSError: + return 0 + if self._du_flavor() == "gnu": + return stat_result.st_size + return stat_result.st_blocks * 512 + + def _total_size(self, path: str, aliases: set[str]) -> int: + """`du -s path`, except where a pruned alias lives inside `path`: + then sum `path`'s non-alias children instead, so `du` is never + handed the duplicate subtree in the first place. Correcting the + total by subtracting the alias afterwards would give the same + number but none of the speed -- du would still have walked it. + + The recursion only expands along branches that actually contain an + alias (nine directories under `/` on macOS, all within four levels), + so in practice this is one `du -s` per child plus a shallow detour. + """ + prefix = path.rstrip("/") + "/" + if not any(alias.startswith(prefix) for alias in aliases): + sized = self.path_size(path) + return sized.size_bytes if sized else 0 + + total = 0 + try: + with os.scandir(path) as it: + for child in it: + if child.path in aliases: + continue + try: + is_dir = child.is_dir(follow_symlinks=False) + except OSError: + continue + total += self._total_size(child.path, aliases) if is_dir else self._entry_size(child) + except OSError: + return 0 + return total + + def _depth_one_entries(self, target: str, *, include_files: bool) -> list[Entry] | None: + """Size each of `target`'s immediate children independently, or + None to tell top_entries() to fall back to a single `du` run. + + Two things this buys over `du -d 1 `, which is otherwise + exactly equivalent: + + * Concurrency. Each child is its own `du -s` process, so the walk + is no longer serialized through one single-threaded `du`; + measured 99.9s -> 66s on a real macOS home directory. + * A place to prune volume aliases. `du` has no way to be told "you + have already counted that directory under another name", and on + macOS it has to be told -- see _alias_scan_is_warranted(). + + Falls back (returns None) when the target is not a directory or has + so many children that per-child process spawns would cost more than + the concurrency returns. + + Known trade-off: one `du` run counts a hardlinked file once no + matter how many of its links it meets, and N independent runs + cannot. Two top-level children sharing hardlinked content therefore + each count it in full here. Checked against `du -d 1` on a real + macOS home directory, the top-15 ordering was identical and every + size matched to within the churn of the live caches being written + during the runs -- cross-child hardlinks are rare enough in + practice to be worth the ~1.5x, and are the reason this is a + depth-1-only path rather than the default everywhere. + """ + if not os.path.isdir(target): + return None + try: + with os.scandir(target) as it: + children = list(it) + except OSError: + return None + + directories: list[str] = [] + files: list[Entry] = [] + for child in children: + try: + is_dir = child.is_dir(follow_symlinks=False) + except OSError: + continue + if is_dir: + directories.append(child.path) + elif include_files: + size = self._entry_size(child) + files.append( + Entry( + full_name=child.path, + is_folder=False, + size_bytes=size, + allocated_bytes=size, + modified=None, + file_count=None, + folder_count=None, + ) + ) + + if len(directories) > _MAX_PARALLEL_CHILDREN: + return None + + aliases = _alias_paths(target) if _alias_scan_is_warranted(target) else set() + directories = [d for d in directories if d not in aliases] + + sizes: list[int] = [] + if directories: + workers = min(_DU_CONCURRENCY, len(directories)) + with ThreadPoolExecutor(max_workers=workers) as pool: + sizes = list(pool.map(lambda d: self._total_size(d, aliases), directories)) + + entries = files + for directory, size in zip(directories, sizes): + entries.append( + Entry( + full_name=directory, + is_folder=True, + size_bytes=size, + allocated_bytes=size, + modified=None, + file_count=None, + folder_count=None, + ) + ) return entries def top_entries( @@ -175,13 +514,17 @@ def top_entries( admin: bool = False, include_files: bool = False, ) -> list[Entry]: - entries = self.scan( - path, - export_folders=True, - export_files=include_files, - max_depth=max_depth, - admin=admin, - ) + entries: list[Entry] | None = None + if max_depth == 1: + entries = self._depth_one_entries(resolve_path(path), include_files=include_files) + if entries is None: + entries = self.scan( + path, + export_folders=True, + export_files=include_files, + max_depth=max_depth, + admin=admin, + ) entries.sort(key=lambda e: e.size_bytes, reverse=True) return entries[:top] diff --git a/tests/unit/test_du_backend.py b/tests/unit/test_du_backend.py index f6b4cab..dda73fa 100644 --- a/tests/unit/test_du_backend.py +++ b/tests/unit/test_du_backend.py @@ -9,6 +9,7 @@ """ from __future__ import annotations +import io import subprocess import sys @@ -207,6 +208,27 @@ def fake_run(args, **kwargs): assert backend._du_flavor() == "bsd" +class _FakePopen: + """Minimal stand-in for subprocess.Popen as scan() uses it: an + iterable text-mode stdout plus wait()/close().""" + + def __init__(self, stdout: str): + self.stdout = io.StringIO(stdout) + self.returncode = 0 + + def wait(self): + return self.returncode + + +def _fake_popen_returning(stdout, *, recorder=None): + def factory(args, **kwargs): + if recorder is not None: + recorder.append(args) + return _FakePopen(stdout) + + return factory + + def test_scan_parses_bsd_style_kilobyte_output(monkeypatch, tmp_path): """Force the BSD parsing branch (size in 1024-byte blocks, scaled up) regardless of the real `du` installed on the test runner, using real @@ -216,28 +238,97 @@ def test_scan_parses_bsd_style_kilobyte_output(monkeypatch, tmp_path): backend = DuBackend() backend._flavor = "bsd" # skip flavor probing - def fake_run(args, **kwargs): - assert "-k" in args - # BSD `du -a -k` output: \t - stdout = f"4\t{tmp_path / 'sub'}\n8\t{tmp_path}\n" - return _FakeCompleted(returncode=0, stdout=stdout) - - monkeypatch.setattr(subprocess, "run", fake_run) + args_seen: list[list[str]] = [] + # BSD `du -a -k` output: \t + stdout = f"4\t{tmp_path / 'sub'}\n8\t{tmp_path}\n" + monkeypatch.setattr(subprocess, "Popen", _fake_popen_returning(stdout, recorder=args_seen)) entries = backend.scan(str(tmp_path), export_folders=True, export_files=True, max_depth=1) + assert "-k" in args_seen[0] assert len(entries) == 1 assert entries[0].full_name == str(tmp_path / "sub") assert entries[0].size_bytes == 4 * 1024 # scaled from 1024-byte blocks -def test_scan_raises_permission_denied_when_du_fails_with_no_output(monkeypatch, tmp_path): +def test_scan_raises_permission_denied_when_du_produces_no_output(monkeypatch, tmp_path): backend = DuBackend() backend._flavor = "gnu" - def fake_run(args, **kwargs): - return _FakeCompleted(returncode=1, stdout="", stderr="du: Permission denied") - - monkeypatch.setattr(subprocess, "run", fake_run) + monkeypatch.setattr(subprocess, "Popen", _fake_popen_returning("")) with pytest.raises(PermissionDeniedError): backend.scan(str(tmp_path)) + + +# --- macOS/BSD flag selection (the expensive `-a` is opt-in) ---------------- + + +def _args_for(monkeypatch, backend, tmp_path, **scan_kwargs): + args_seen: list[list[str]] = [] + stdout = f"8\t{tmp_path}\n" + monkeypatch.setattr(subprocess, "Popen", _fake_popen_returning(stdout, recorder=args_seen)) + backend.scan(str(tmp_path), **scan_kwargs) + return args_seen[0] + + +def test_bsd_directory_only_scan_skips_dash_a_and_limits_depth_natively(monkeypatch, tmp_path): + """The scan/inspect hot path (top_entries(include_files=False)) must + not ask BSD du for file-level rows: `-a` there is unbounded-depth by + necessity (BSD's -a and -d are mutually exclusive), which on a real + macOS home directory meant millions of rows and ~2.6GB peak RSS to + produce ~15 rows of output. + """ + backend = DuBackend() + backend._flavor = "bsd" + + args = _args_for( + monkeypatch, backend, tmp_path, export_folders=True, export_files=False, max_depth=1 + ) + + assert "-a" not in args + assert args[args.index("-d") + 1] == "1" + + +def test_bsd_file_level_scan_still_uses_dash_a_without_depth_flag(monkeypatch, tmp_path): + backend = DuBackend() + backend._flavor = "bsd" + + args = _args_for( + monkeypatch, backend, tmp_path, export_folders=True, export_files=True, max_depth=2 + ) + + assert "-a" in args + assert "-d" not in args # mutually exclusive with -a on BSD; filtered in Python + + +def test_gnu_directory_only_scan_skips_dash_a(monkeypatch, tmp_path): + backend = DuBackend() + backend._flavor = "gnu" + + args = _args_for( + monkeypatch, backend, tmp_path, export_folders=True, export_files=False, max_depth=1 + ) + + assert "-a" not in args + assert "--max-depth=1" in args + + +def test_non_empty_directories_are_classified_without_stat(monkeypatch, tmp_path): + """In `-a` mode du emits a directory after its contents, so a + directory is already known to be one by the time its own row arrives + -- no os.path.isdir() call needed. Proven by pointing the backend at + paths that do not exist on disk: isdir() would report False for all + of them. + """ + backend = DuBackend() + backend._flavor = "gnu" + ghost = tmp_path / "ghost" + stdout = f"10\t{ghost / 'inner' / 'f.bin'}\n20\t{ghost / 'inner'}\n30\t{ghost}\n40\t{tmp_path}\n" + monkeypatch.setattr(subprocess, "Popen", _fake_popen_returning(stdout)) + + entries = backend.scan(str(tmp_path), export_folders=True, export_files=True) + + by_path = {e.full_name: e.is_folder for e in entries} + assert by_path[str(ghost)] is True + assert by_path[str(ghost / "inner")] is True + assert by_path[str(ghost / "inner" / "f.bin")] is False diff --git a/tests/unit/test_du_depth_one.py b/tests/unit/test_du_depth_one.py new file mode 100644 index 0000000..7ceee0c --- /dev/null +++ b/tests/unit/test_du_depth_one.py @@ -0,0 +1,192 @@ +"""Unit tests for DuBackend's depth-1 fast path and its volume-alias +pruning (src/storops/platform/backends/du.py). + +The macOS topology these exist for: `/` is the sealed System volume, the +writable Data volume is mounted at /System/Volumes/Data, and firmlinks +project the Data volume's directories into `/`. `/Users` and +`/System/Volumes/Data/Users` are therefore the same directory -- same +st_dev, same st_ino, neither a symlink -- so `du /` walks the user's home +twice. Measured on a real Mac: `storops scan /` reported /System at 95.0G +before pruning and 72.9G after, with the Data volume's own contribution +dropping from du's 309G to its actual 3.5G. +""" +from __future__ import annotations + +import sys + +import pytest + +pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only backend") + +from storops.platform.backends import du as du_module +from storops.platform.backends.du import ( + DuBackend, + _alias_pre_pass_descends_into, + _alias_scan_is_warranted, +) + + +# --- Where the pre-pass runs at all ---------------------------------------- + + +@pytest.mark.parametrize( + "target,expected", + [ + ("/", True), + ("/System/Volumes/Data", True), + ("/Volumes/External", True), + ("/Users/someone", False), + ("/Users/someone/Library/Caches", False), + ("/tmp", False), + ], +) +def test_alias_scan_only_runs_at_volume_roots_on_darwin(monkeypatch, target, expected): + monkeypatch.setattr(du_module._platform, "system", lambda: "Darwin") + assert _alias_scan_is_warranted(target) is expected + + +def test_alias_scan_never_runs_off_darwin(monkeypatch): + for system in ("Linux", "Windows"): + monkeypatch.setattr(du_module._platform, "system", lambda: system) + assert _alias_scan_is_warranted("/") is False + + +@pytest.mark.parametrize( + "path,expected", + [ + ("/", True), # ancestor of /System/Volumes -- must be walked through + ("/System", True), + ("/System/Volumes", True), # the boundary itself, not just below it + ("/System/Volumes/Data", True), + ("/System/Volumes/Data/System/Library", True), + ("/Users", False), # by construction never holds a firmlink + ("/System/Applications", False), + ], +) +def test_pre_pass_stays_on_the_branch_that_can_hold_firmlinks(path, expected): + assert _alias_pre_pass_descends_into(path) is expected + + +# --- Pruning arithmetic ----------------------------------------------------- + + +class _RecordingBackend(DuBackend): + """DuBackend whose `du -s` is replaced by a directory-size lookup, so + the pruning logic can be tested without depending on real firmlinks + (which only exist on a real macOS volume group).""" + + def __init__(self, sizes: dict[str, int]): + super().__init__() + self._sizes = sizes + self.sized: list[str] = [] + + def path_size(self, path, *, admin=False): + from storops.core.models import Entry + + self.sized.append(path) + size = self._sizes.get(path, 0) + return Entry( + full_name=path, is_folder=True, size_bytes=size, allocated_bytes=size, + modified=None, file_count=None, folder_count=None, + ) + + +def test_total_size_without_aliases_is_a_single_du_call(tmp_path): + branch = tmp_path / "branch" + branch.mkdir() + backend = _RecordingBackend({str(branch): 4096}) + + assert backend._total_size(str(branch), set()) == 4096 + assert backend.sized == [str(branch)] + + +def test_total_size_never_hands_du_a_pruned_alias(tmp_path): + """The whole point of pruning rather than subtracting afterwards: the + duplicate subtree must never be walked, or the correction costs the + same time it was meant to save.""" + branch = tmp_path / "branch" + keep = branch / "keep" + alias = branch / "alias" + keep.mkdir(parents=True) + alias.mkdir() + backend = _RecordingBackend({str(keep): 1000, str(alias): 999_999}) + + total = backend._total_size(str(branch), {str(alias)}) + + assert total == 1000 + assert str(alias) not in backend.sized + + +def test_total_size_recurses_only_along_branches_holding_an_alias(tmp_path): + branch = tmp_path / "branch" + clean = branch / "clean" + dirty = branch / "dirty" + alias = dirty / "alias" + clean.mkdir(parents=True) + alias.mkdir(parents=True) + backend = _RecordingBackend({str(clean): 10, str(dirty): 20, str(alias): 30}) + + total = backend._total_size(str(branch), {str(alias)}) + + # `clean` was sized in one `du -s` without being descended into; only + # `dirty` (which contains the alias) was opened up. + assert str(clean) in backend.sized + assert str(dirty) not in backend.sized + assert total == 10 + + +# --- Fast path vs. the plain `du` path must agree --------------------------- + + +def _tree(tmp_path): + (tmp_path / "big").mkdir() + (tmp_path / "big" / "a.bin").write_bytes(b"x" * 20_000) + (tmp_path / "small").mkdir() + (tmp_path / "small" / "b.bin").write_bytes(b"y" * 1_000) + (tmp_path / "loose.bin").write_bytes(b"z" * 5_000) + return tmp_path + + +def test_depth_one_fast_path_matches_the_du_scan_it_replaces(tmp_path): + root = _tree(tmp_path) + backend = DuBackend() + + fast = backend.top_entries(str(root), top=10, max_depth=1, include_files=False) + slow = backend.scan(str(root), export_folders=True, export_files=False, max_depth=1) + slow.sort(key=lambda e: e.size_bytes, reverse=True) + + assert [e.full_name for e in fast] == [e.full_name for e in slow] + assert all(e.is_folder for e in fast) + + +def test_depth_one_fast_path_includes_files_when_asked(tmp_path): + root = _tree(tmp_path) + backend = DuBackend() + + entries = backend.top_entries(str(root), top=10, max_depth=1, include_files=True) + + by_path = {e.full_name: e for e in entries} + assert by_path[str(root / "loose.bin")].is_folder is False + assert by_path[str(root / "big")].is_folder is True + + +def test_wide_directories_fall_back_to_a_single_du_run(tmp_path, monkeypatch): + """One `du -s` per child stops paying for itself once the process + spawns outnumber the work; past the threshold top_entries() must go + back to a single `du` over the whole target.""" + for index in range(5): + (tmp_path / f"d{index}").mkdir() + monkeypatch.setattr(du_module, "_MAX_PARALLEL_CHILDREN", 2) + backend = DuBackend() + + assert backend._depth_one_entries(str(tmp_path), include_files=False) is None + # ...and top_entries still returns the same answer via the fallback. + assert len(backend.top_entries(str(tmp_path), top=10, max_depth=1)) == 5 + + +def test_depth_one_fast_path_declines_a_non_directory(tmp_path): + target = tmp_path / "f.bin" + target.write_bytes(b"x" * 10) + backend = DuBackend() + + assert backend._depth_one_entries(str(target), include_files=True) is None From ceabe159ca7ea85c3b10fd0fbd1906b2cc7e2370 Mon Sep 17 00:00:00 2001 From: tzzs Date: Sun, 20 Sep 2026 20:02:12 +0800 Subject: [PATCH 3/5] perf(cleanup): size a plan's probe paths concurrently 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 --- src/storops/core/cleanup.py | 35 ++++++++-- src/storops/platform/backends/gdu.py | 6 ++ tests/unit/test_cleanup_probe_concurrency.py | 72 ++++++++++++++++++++ 3 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 tests/unit/test_cleanup_probe_concurrency.py diff --git a/src/storops/core/cleanup.py b/src/storops/core/cleanup.py index aeb1733..2b12843 100644 --- a/src/storops/core/cleanup.py +++ b/src/storops/core/cleanup.py @@ -13,6 +13,7 @@ import json import os import shutil +from concurrent.futures import ThreadPoolExecutor from datetime import datetime, timezone from pathlib import Path @@ -44,6 +45,32 @@ def _probe_path(pattern: str) -> str | None: return rules.expand_pattern_tokens(stripped) +# Sizing one probe path means walking its whole subtree, so a plan over a +# handful of caches is almost entirely time spent waiting on filesystem +# metadata -- measured at 6.3s for nine probes on a real Mac, all of it +# serialized for no reason. Kept modest: past this the backends' own +# per-call subprocesses start competing for the same disk. +_PROBE_CONCURRENCY = 8 + + +def _sized_probes(backend, probe_paths: list[str], *, admin: bool) -> list: + """backend.path_size() for each path, concurrently where the backend + says that is safe. + + 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. + Backends that are safe declare `path_size_is_concurrent = True`. + """ + if len(probe_paths) < 2 or not getattr(backend, "path_size_is_concurrent", False): + return [backend.path_size(p, admin=admin) for p in probe_paths] + + workers = min(_PROBE_CONCURRENCY, len(probe_paths)) + with ThreadPoolExecutor(max_workers=workers) as pool: + return list(pool.map(lambda p: backend.path_size(p, admin=admin), probe_paths)) + + def plan(max_risk: str = "low", *, out_file: str | None = None, admin: bool = False) -> CleanupPlan: backend = platform_pkg.get_scan_backend() deletable_rules = [r for r in rules.load_rules() if r.deletable] @@ -60,11 +87,11 @@ def plan(max_risk: str = "low", *, out_file: str | None = None, admin: bool = Fa # double-counted it when keyed on the raw expansion. probes.setdefault(resolve_path(probe), rule) + existing = [(p, r) for p, r in probes.items() if os.path.exists(p)] + sizes = _sized_probes(backend, [p for p, _ in existing], admin=admin) + items: list[CleanupItem] = [] - for probe_path, rule in probes.items(): - if not os.path.exists(probe_path): - continue - sized = backend.path_size(probe_path, admin=admin) + for (probe_path, rule), sized in zip(existing, sizes): if not sized or sized.size_bytes <= 0: continue diff --git a/src/storops/platform/backends/gdu.py b/src/storops/platform/backends/gdu.py index ec69b5f..e7fc39d 100644 --- a/src/storops/platform/backends/gdu.py +++ b/src/storops/platform/backends/gdu.py @@ -222,6 +222,12 @@ class GduBackend: name = "Gdu" + # Each path_size() call is its own `gdu` subprocess over its own + # target, with no state kept on this instance between calls -- see + # core/cleanup.py's _sized_probes(), which sizes every probe path + # concurrently when a backend declares this. + path_size_is_concurrent = True + def scan( self, path: str, diff --git a/tests/unit/test_cleanup_probe_concurrency.py b/tests/unit/test_cleanup_probe_concurrency.py new file mode 100644 index 0000000..39e8850 --- /dev/null +++ b/tests/unit/test_cleanup_probe_concurrency.py @@ -0,0 +1,72 @@ +"""Unit tests for core/cleanup.py's _sized_probes(). + +Every probe path in a cleanup plan is sized by walking its whole subtree, +so a plan is almost entirely time spent waiting on filesystem metadata -- +measured at 6.3s for nine probes on a real Mac, down to 2.8s once they run +concurrently. Concurrency is opt-in per backend because the Windows native +backend keeps per-call state on `self` and already parallelizes its own +walk internally. +""" +from __future__ import annotations + +import threading + +from storops.core.cleanup import _sized_probes +from storops.core.models import Entry + + +class _Backend: + def __init__(self, *, concurrent: bool): + if concurrent: + self.path_size_is_concurrent = True + self.calls: list[str] = [] + self.max_in_flight = 0 + self._in_flight = 0 + self._lock = threading.Lock() + self._gate = threading.Barrier(2, timeout=5) + + def path_size(self, path, *, admin=False): + with self._lock: + self.calls.append(path) + self._in_flight += 1 + self.max_in_flight = max(self.max_in_flight, self._in_flight) + try: + # Blocks until a second call joins -- which can only ever + # happen if the caller really is running them concurrently. + try: + self._gate.wait() + except threading.BrokenBarrierError: + pass + finally: + with self._lock: + self._in_flight -= 1 + return Entry(full_name=path, is_folder=True, size_bytes=1, allocated_bytes=1) + + +def test_probes_are_sized_concurrently_when_the_backend_allows_it(): + backend = _Backend(concurrent=True) + + results = _sized_probes(backend, ["/a", "/b"], admin=False) + + assert [r.full_name for r in results] == ["/a", "/b"] # order preserved + assert backend.max_in_flight == 2 + + +def test_probes_stay_sequential_for_a_backend_that_does_not_opt_in(): + backend = _Backend(concurrent=False) + backend._gate.abort() # a sequential caller would otherwise deadlock + + results = _sized_probes(backend, ["/a", "/b"], admin=False) + + assert [r.full_name for r in results] == ["/a", "/b"] + assert backend.max_in_flight == 1 + + +def test_a_single_probe_never_spins_up_a_pool(): + backend = _Backend(concurrent=True) + backend._gate.abort() + + results = _sized_probes(backend, ["/only"], admin=False) + + assert [r.full_name for r in results] == ["/only"] + assert backend.max_in_flight == 1 From 1551692b56d3cfb25ae3b3736232f9bc345b5555 Mon Sep 17 00:00:00 2001 From: tzzs Date: Sun, 20 Sep 2026 20:02:12 +0800 Subject: [PATCH 4/5] feat(rules): identify the macOS developer toolchain's reclaimable caches 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 --- rules/README.md | 6 +- rules/applications.yaml | 164 ++++++++++++++++++++++++++++++++++++++++ rules/caches.yaml | 20 +++++ rules/macos.yaml | 11 ++- 4 files changed, 192 insertions(+), 9 deletions(-) diff --git a/rules/README.md b/rules/README.md index 4ba925d..a85da8c 100644 --- a/rules/README.md +++ b/rules/README.md @@ -8,9 +8,9 @@ path is from its name alone (see [`docs/DESIGN.md`](../docs/DESIGN.md) §3.3). | File | Covers | |---|---| -| `ai-models.yaml` | AI/ML model weights and inference-tool caches (LM Studio, Ollama, Hugging Face, ComfyUI/Stable Diffusion, PyTorch/CUDA) -- `path_patterns` are currently Windows-token-only | -| `applications.yaml` | Dev tooling (npm, pnpm, pip, uv, conda, Git, VS Code, JetBrains, Visual Studio, Docker, WSL) and general consumer apps (Steam, Chrome, Edge, Discord, Adobe) -- `path_patterns` are currently Windows-token-only | -| `caches.yaml` | Generic OS/browser/temp caches not owned by one specific application above -- `path_patterns` are currently Windows-token-only | +| `ai-models.yaml` | AI/ML model weights and inference-tool caches (LM Studio, Ollama, Hugging Face, ComfyUI/Stable Diffusion, PyTorch/CUDA) | +| `applications.yaml` | Dev tooling (npm, pnpm, pip, uv, conda, Git, VS Code, JetBrains, Visual Studio, Docker, WSL, Gradle, Cargo, Homebrew, the Xcode/CoreSimulator toolchain) and general consumer apps (Steam, Chrome, Edge, Discord, Adobe) | +| `caches.yaml` | Generic OS/browser/temp caches not owned by one specific application above | | `windows.yaml` | Windows system paths StorOps must never classify as safe to touch | | `linux.yaml` | Linux system paths StorOps must never classify as safe to touch | | `macos.yaml` | macOS system paths StorOps must never classify as safe to touch | diff --git a/rules/applications.yaml b/rules/applications.yaml index 3fbae5f..9d7901f 100644 --- a/rules/applications.yaml +++ b/rules/applications.yaml @@ -357,3 +357,167 @@ notes: > No Linux pattern here by design -- the Adobe Creative Cloud desktop suite does not ship a Linux build, so there is no real path to match. + +# --- Apple developer toolchain (macOS) -------------------------------------- +# The largest reclaimable consumers on a Mac used for development, and all +# previously "unknown"/critical -- which meant StorOps could see tens of GB +# in a scan and had nothing to say about any of it. + +- id: xcode-derived-data + application: Xcode + category: ide-cache + path_patterns: + - "%HOME%/Library/Developer/Xcode/DerivedData/*" + confidence: 0.95 + owner: user + purpose: > + Per-project build intermediates, module caches, and indexes that Xcode + regenerates from the project source on the next build. + deletable: true + migratable: false + cleanup_risk: low + cleanup_consequence: > + The next build of each affected project is a full rebuild (slower + once); nothing that is not reproducible from source is lost. + notes: > + Routinely the single largest directory under ~/Library/Developer on a + machine with more than a couple of Xcode projects. + +- id: xcode-device-support + application: Xcode + category: ide-cache + path_patterns: + - "%HOME%/Library/Developer/Xcode/iOS DeviceSupport/*" + - "%HOME%/Library/Developer/Xcode/watchOS DeviceSupport/*" + - "%HOME%/Library/Developer/Xcode/tvOS DeviceSupport/*" + confidence: 0.95 + owner: user + purpose: > + Debug symbols copied off each physical device the first time it is + attached, kept per OS build -- so one entry accumulates per iOS version + ever connected, including long-obsolete ones. + deletable: true + migratable: false + cleanup_risk: medium + cleanup_consequence: > + Re-copied off the device (a few minutes) the next time that exact OS + build is attached for debugging; symbolication of already-captured + crash logs from that build is lost until then. + +- id: xcode-archives + application: Xcode + category: build-output + path_patterns: + - "%HOME%/Library/Developer/Xcode/Archives/*" + confidence: 0.95 + owner: user + purpose: > + Archived builds with their dSYMs -- what App Store submissions were cut + from, and the only way to symbolicate crash reports from a shipped build. + deletable: false + migratable: true + migration_method: manual + migration_config_hint: > + Move the Archives directory to external storage by hand; Xcode's + Organizer reads whatever is present at this path. + cleanup_risk: high + cleanup_consequence: > + Not reproducible: deleting an archive permanently loses the ability to + symbolicate crash reports from the build it came from. + notes: > + Deliberately never offered for deletion even though it sits beside + DerivedData and looks similar -- these are shipping artifacts. + +- id: coresimulator-caches + application: Xcode Simulator + category: ide-cache + path_patterns: + - "%HOME%/Library/Developer/CoreSimulator/Caches/*" + confidence: 0.95 + owner: user + purpose: Downloaded simulator runtime disk images and their extraction scratch space. + deletable: true + migratable: false + cleanup_risk: medium + cleanup_consequence: > + Simulator runtimes are re-downloaded from Apple (several GB each) the + next time one is needed. + +- id: coresimulator-devices + application: Xcode Simulator + category: application-data + path_patterns: + - "%HOME%/Library/Developer/CoreSimulator/Devices/*" + confidence: 0.95 + owner: user + purpose: > + One full disk image per created simulator device, including every app + installed into it and its data. + deletable: true + migratable: false + cleanup_risk: high + cleanup_consequence: > + Deleting these directories by hand leaves CoreSimulator's own device + index pointing at devices that no longer exist. + notes: > + Reclaim this through Xcode itself -- `xcrun simctl delete unavailable` + removes devices for uninstalled runtimes and keeps the index + consistent. Classified high on purpose so `cleanup plan` identifies it + but never proposes deleting it. + +- id: homebrew-cache + application: Homebrew + category: package-manager-cache + path_patterns: + - "%CACHES%/Homebrew/*" + - "%HOME%/Library/Caches/Homebrew/*" + - "%XDG_CACHE_HOME%/Homebrew/*" + confidence: 0.95 + owner: user + purpose: Downloaded bottles, source tarballs, and their partial downloads. + deletable: true + migratable: false + cleanup_risk: low + cleanup_consequence: > + Re-downloaded on the next install or upgrade of the affected formula; + installed software is unaffected. + notes: "`brew cleanup -s` does the same thing through Homebrew itself." + +# --- Cross-platform build caches missing from the rule base entirely -------- + +- id: gradle-cache + application: Gradle + category: package-manager-cache + path_patterns: + - "%HOME%/.gradle/caches/*" + - "%USERPROFILE%\\.gradle\\caches\\*" + confidence: 0.9 + owner: user + purpose: Resolved dependency jars, build-script compilation output, and transformed artifacts. + deletable: true + migratable: true + migration_method: app-config + migration_config_hint: > + Set GRADLE_USER_HOME to the new location (Gradle puts caches/ beneath it). + cleanup_risk: medium + cleanup_consequence: > + Every dependency is re-downloaded and every build script recompiled on + the next build; offline builds stop working until then. + +- id: cargo-registry + application: Cargo (Rust) + category: package-manager-cache + path_patterns: + - "%HOME%/.cargo/registry/*" + - "%USERPROFILE%\\.cargo\\registry\\*" + confidence: 0.9 + owner: user + purpose: Downloaded crate sources and the registry index. + deletable: true + migratable: true + migration_method: app-config + migration_config_hint: "Set CARGO_HOME to the new location." + cleanup_risk: medium + cleanup_consequence: > + Every dependency is re-downloaded from crates.io on the next build; + offline builds stop working until then. diff --git a/rules/caches.yaml b/rules/caches.yaml index 0b170ea..2ab7b41 100644 --- a/rules/caches.yaml +++ b/rules/caches.yaml @@ -142,3 +142,23 @@ this as a large-consumer candidate for the user to review, never auto-classifies individual files as safe to delete. notes: Report largest/oldest files here for the user's own review rather than proposing bulk deletion. + +- id: macos-user-logs + application: macOS + category: os-temp + path_patterns: + - "%HOME%/Library/Logs/*" + confidence: 0.85 + owner: user + purpose: > + Per-user application and crash logs, written by apps and by macOS' + diagnostic reporting. + deletable: true + migratable: false + cleanup_risk: low + cleanup_consequence: > + Only historical diagnostics are lost; nothing here is needed for any + application to run. + notes: > + The system-wide counterpart under /Library/Logs is covered by + macos.yaml's critical /Library rule and is never offered. diff --git a/rules/macos.yaml b/rules/macos.yaml index ad684d4..0c5b343 100644 --- a/rules/macos.yaml +++ b/rules/macos.yaml @@ -8,12 +8,11 @@ # migratable: false. StorOps must never offer these for cleanup or migration, # regardless of what the user asks for. # -# NOTE: this is a starter set covering the same *kind* of paths windows.yaml -# covers (OS itself, installed apps, user profile/library, user documents) -- -# it does not yet attempt parity with ai-models.yaml/applications.yaml/ -# caches.yaml, whose path_patterns are still Windows-token-only. Extending -# those to macOS equivalents (LM Studio, Ollama, Docker Desktop, npm/pip -# caches under %CACHES%/%XDG_CACHE_HOME%, etc.) is separate follow-up work. +# NOTE: this file covers only the macOS paths StorOps must never classify as +# safe to touch. The macOS *reclaimable* paths live where their application +# does: applications.yaml carries the Xcode/CoreSimulator/Homebrew rules and +# the %CACHES%/%APP_SUPPORT% patterns for the cross-platform tools, and +# caches.yaml carries ~/Library/Logs. - id: macos-system application: macOS From 81242fe27de450f1fac13a895f41fe83a98dc28e Mon Sep 17 00:00:00 2001 From: tzzs Date: Sun, 20 Sep 2026 20:02:22 +0800 Subject: [PATCH 5/5] ci: cover 3.9/3.13, the du backend on macOS, and the built wheel 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 --- .github/workflows/test.yml | 69 +++++++++++++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a9ca557..a97c00c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -12,13 +12,17 @@ jobs: fail-fast: false matrix: os: [ubuntu-latest, macos-latest, windows-latest] + # 3.9 is the declared floor (a stock macOS `python3` and RHEL 9 both + # ship it) and is tested so it stays real; 3.13 catches deprecations + # early. The middle of the range is left to those two to bracket. + python-version: ["3.9", "3.13"] runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 with: - python-version: "3.11" + python-version: ${{ matrix.python-version }} - name: Install gdu (Linux/macOS only -- the preferred scan backend, du is the fallback) if: runner.os != 'Windows' @@ -34,3 +38,66 @@ jobs: - name: Run unit + integration tests run: pytest tests/unit tests/integration -q + + # The backend a real Mac actually uses. gdu is a `brew install` away, so + # the job above never exercises BSD `du` -- which is where macOS' own + # quirks live (its -a and -d are mutually exclusive, so an unguarded + # file-level scan is unbounded in depth; a `storops scan ~` cost ~2.6GB + # peak RSS before that was caught). Without this job those regressions + # are invisible on every runner. + test-macos-without-gdu: + runs-on: macos-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: "3.13" + + - name: Confirm gdu is absent, so the du backend is the one under test + run: | + ! command -v gdu || { echo "gdu unexpectedly on PATH"; exit 1; } + + - name: Install storops (editable) + dev dependencies + run: pip install -e ".[dev]" + + - name: Run unit + integration tests + run: pytest tests/unit tests/integration -q + + - name: The du backend really is what got selected + run: | + python - <<'PY' + from storops import platform + backend = platform.get_scan_backend() + assert backend.name == "Du", backend.name + print("scan backend:", backend.name) + PY + + # `pip install -e .` leaves the repo-root rules/ directory reachable, so it + # cannot tell whether the built distribution actually ships the rule files. + # It did not: every command in an installed storops failed with "could not + # locate the rules/ directory" until this was caught. + test-installed-wheel: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: "3.9" + + - name: Build the wheel + run: | + pip install build + python -m build --wheel + + - name: Install it somewhere the source tree cannot be reached + run: | + python -m venv /tmp/wheel-venv + /tmp/wheel-venv/bin/pip install dist/*.whl + + - name: The installed package can identify a path (i.e. it found its rules) + run: | + cd /tmp + /tmp/wheel-venv/bin/storops identify /tmp --json + /tmp/wheel-venv/bin/storops scan /tmp --json > /dev/null