feat(install): check free disk space before SDK downloads and extraction - #171
feat(install): check free disk space before SDK downloads and extraction#171rominf wants to merge 2 commits into
Conversation
SDK installs pulled multi-GB tarballs and extracted them without ever checking free space, so a nearly-full disk surfaced as a raw low-level write failure partway through the install. Add `rocm_core::disk_space`, built on the already-vendored `sysinfo` crate, which resolves free space on the filesystem that will actually hold a path (walking up to the nearest existing ancestor, then matching the longest mount point) rather than on the current directory. Wire it into the two paths that move large files: * `install_tarball_runtime` preflights the tarball with a HEAD probe. A `Content-Length` shortfall for the download is an exact requirement and hard-fails upfront with required vs available. The extraction requirement is only an estimate (conservative 4x compressed-size multiplier; TheRock publishes no uncompressed size) so it merely warns — a false refusal blocking a valid install is worse than a late failure. The archive size is added to the extraction estimate when cache and install root share a filesystem. * `download_file_to_path` preflights with `Content-Length` where the server sends one. Write failures caused by a full disk now map `ErrorKind::StorageFull` to a clear message naming the path and the remaining free space, instead of the raw OS error. Also promote `sysinfo` to a workspace dependency so rocm-core and rocm-dash-collectors stay on one version. No new package enters the dependency graph, so THIRD_PARTY_NOTICES.txt is unchanged. Closes #159 Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
volen-silo
left a comment
There was a problem hiding this comment.
Good problem to solve and the module is cleanly built — select_mount split out as a pure function is the right call, and the arithmetic is genuinely careful (saturating_* throughout, verified against u64::MAX inputs).
But the safety argument reasons about the wrong side of the comparison. "Content-Length is exact, so hard-failing is safe" is true of the required side. The available side is not exact — available_space_for_path can silently return a different filesystem's free space, and the hard fail then refuses a valid install with a confident, specific, wrong number. "Unknown space never blocks" is structurally true (I verified there is exactly one bail! in the module, at disk_space.rs:191, reachable only from Insufficient) — but it guards the harmless failure mode, not the dangerous one.
Blocking
1. select_mount silently attributes another filesystem's free space when the real mount isn't in sysinfo's list
crates/rocm-core/src/disk_space.rs:96-102 picks the longest mount point that is a prefix of the path. If the path's actual mount is missing from sysinfo::Disks::list(), the filter doesn't fail — it falls through to the nearest listed ancestor, virtually always /, and reports that filesystem's free space as the target's. SpaceCheck::Unknown only happens when nothing matches, which on Linux essentially never occurs because / is always listed.
sysinfo 0.34.2 excludes real, writable mounts by default. From the vendored source, src/unix/linux/disk.rs:395-397:
"tmpfs" => !cfg!(feature = "linux-tmpfs"),
// calling statvfs on a mounted CIFS or NFS may hang, when they are mounted with option: hard
"cifs" | "nfs" | "nfs4" => !cfg!(feature = "linux-netdevs"),Its default feature set is ["component", "disk", "network", "system", "user"] — neither linux-tmpfs nor linux-netdevs. Root Cargo.toml:48 declares sysinfo = "0.34" with no features, and crates/rocm-core/Cargo.toml:26 takes it via workspace = true. So NFS, NFS4, CIFS and tmpfs mounts are invisible to this code.
Demonstrated with a scratch binary replicating nearest_existing_ancestor + select_mount verbatim against real sysinfo 0.34.2:
/dev/shm/rocm-sdk.tar.gz -> Some(("/", 966154645504)) # code says 966 GB
df /dev/shm -> tmpfs, 67108864 # reality: 64 MiB
Wrong filesystem, off by ~15,000×, no signal anything is wrong.
The scenario that matters for this audience: an enterprise or HPC workstation with $HOME on NFS — very common for ROCm SDK users on shared infrastructure — and a conventional root partition with 3 GB free. ~/.cache/rocm/ has terabytes. sysinfo omits the NFS mount, select_mount falls back to /, and rocm install sdk --format tarball gives:
Error: not enough free disk space to download the SDK tarball: need about 5.3 GiB
but only 3.1 GiB is available on the filesystem holding
/home/user/.cache/rocm/therock/therock-dist-....tar.gz. Free up 2.2 GiB and retry.
df -h ~/.cache shows 2 TB free. The install is now impossible — there's no bypass flag, and the only remedy the message suggests is freeing space on a filesystem the download won't touch. Same shape where /tmp is tmpfs (Fedora, Arch defaults) with TMPDIR/ROCM_CLI_CACHE_DIR pointing into it.
Suggested fix: before letting Insufficient escalate to a hard failure, verify the selected mount actually owns the path — compare MetadataExt::dev() of the resolved ancestor against the mount point's, and on mismatch return None so it degrades to Unknown and fails open. Enabling sysinfo's linux-netdevs and linux-tmpfs features would cover the common cases, but the dev() cross-check is the load-bearing part: it turns "confidently wrong" back into "unknown", which the existing design already handles correctly.
2. The feature is a silent no-op on Windows
crates/rocm-core/src/disk_space.rs:83-87 — nearest_existing_ancestor returns candidate.canonicalize(). On Windows that's a verbatim path (\\?\C:\Users\...), whose first component parses as Prefix::VerbatimDisk(b'C'). sysinfo reports mount points as C:\ → Prefix::Disk(b'C'). Path::starts_with compares component-wise including the prefix, and Prefix derives PartialEq — two different variants are never equal regardless of payload. So starts_with fails at the first component for every mount, select_mount returns None, and every Windows path yields Unknown.
Net: on a supported platform per AGENTS.md §6, no preflight ever runs. Fails open, so not a false refusal — but the PR ships a feature that does nothing on half its supported platforms while claiming to close #159.
windows-build-and-test passing doesn't contradict this: every select_mount test (disk_space.rs:257-292) uses synthetic Unix-style mount tables, and nothing asserts mount_for_path returns Some for a real path. The gap is invisible to the suite by construction.
Two related Windows issues in the same area: Component::Normal compares case-sensitively, so a volume mounted at C:\Data won't match a path C:\data\...; and mapped network drives are filtered out by sysinfo (src/windows/disk.rs:285-288 keeps only DRIVE_FIXED/DRIVE_REMOVABLE).
This is cheap for a Windows reviewer to settle directly: a temporary dbg!(mount_for_path(Path::new("C:\\Users"))) and see whether it's Some or None. I confirmed the Prefix inequality from Rust 1.96.0 std source but could not execute on Windows.
3. The flat 256 MiB margin makes a general-purpose helper hard-fail small downloads
crates/rocm-core/src/lib.rs:139-143 applies ensure_space_for(..., with_margin(content_length)) inside download_file_to_path, and disk_space.rs:69-71 adds a flat SPACE_MARGIN_BYTES (256 MiB, :28) to every requirement regardless of payload.
download_file_to_path isn't an SDK-tarball function. Its production callers are crates/rocm-core/src/uv.rs:129 (the uv binary, ~15-35 MB, on the default --format wheel path), apps/rocm/src/comfyui.rs:1352, and engines/lemonade/src/lib.rs:1245.
On a machine with 200 MB free, downloading the 20 MB uv binary now hard-fails: "need about 276.0 MiB but only 190.7 MiB is available". The 20 MB it actually needs fits fine. Reachable in small VMs, CI containers, constrained scratch volumes. Suggest a proportional margin with a floor — max(bytes / 20, 32 MiB) — or take the margin as a parameter so the SDK path can ask for 256 MiB without a 20 MB helper download inheriting it.
4. The one operation that only warns is also the one with no ENOSPC message
apps/rocm/src/therock.rs:2141-2147 deliberately downgrades the extraction check to a warning, on the grounds that a late failure is acceptable because the message will be clear. But extract_tarball (therock.rs:2265-2274) shells out to tar via run_command, and map_write_error (disk_space.rs:220-231) only handles std::io::ErrorKind::StorageFull from direct Rust I/O — a subprocess exit code never reaches it.
So when the warning is right and extraction runs out of space, the user gets exactly what #159 asked to eliminate:
extract TheRock tarball artifact: tar: ...: No space left on device
Either map tar's ENOSPC stderr, or say in the PR that extraction still surfaces the raw error.
5. The default install format gets no preflight at all
apps/rocm/src/main.rs:503: #[arg(long, default_value = "wheel")]. grep -n "disk_space::" apps/rocm/src/therock.rs hits only download_file (:2093), preflight_tarball_space (:2129-2144), and write_file_atomically (:2256) — all tarball-path. install_wheel_runtime, the default path and the larger download once PyTorch/torchvision/torchaudio come through uv pip install, has zero preflight and zero ENOSPC mapping. Either extend coverage or state the gap and downgrade to "Refs #159".
Non-blocking
- The hard fail trusts an unauthenticated
Content-Lengthwith no ceiling.therock.rs:2106-2115→:2129-2133. A HEAD response is never cross-checked against the subsequent GET. A CDN or proxy returning an inflated length blocks the install outright even though the GET would deliver the correct smaller body. A bogus 2^63 header yields "need about 8.0 EiB" and a refusal. - Tests don't cover the false-refusal paths, and two pass vacuously.
disk_space.rs:337-341(zero_requirement_never_fails_on_a_real_path) — withrequired = 0,available >= 0always holds, so it never exercises theUnknownbranch; nothing would fail if "unknown never blocks" regressed for a nonzero requirement.disk_space.rs:366-374puts every assertion insideif let Some(warning), so aNonereturn passes asserting nothing. Root cause of both, and of #1 slipping through:check_space_for_pathcalls the realavailable_space_for_pathdirectly (:137), so the policy layer — the part that decides to hard-fail — can't be tested against a synthetic mount table. Threading a mount-table parameter through would make all three testable. therock.rs:3248-3253makes a real network connect to127.0.0.1:1. Fast here (~0.04 s) because the port refuses, but a host that blackholes will sit for ureq's connect timeout — and ureq 2.12.1 documents.timeout_connect()(default 30 s) as taking precedence over.timeout(), soTHEROCK_HEAD_PROBE_TIMEOUT_SECS = 10(therock.rs:36) isn't an airtight ceiling.- Output-ordering wart:
therock.rs:2146callsprogress_line(warning), an unconditionalprintln!, while every other line ininstall_tarball_runtimeaccumulates into a localoutput: String(:1034-1049) printed once by the caller. The warning lands before the block it belongs to, and inapply_runtime_update's dry-run branch appears unindented and detached. - Redundant work:
therock.rs:2093-2097re-runs a hard checkpreflight_tarball_spacealready did on the same path moments earlier. And one preflight performs four independentDisks::new_with_refreshed_list()sweeps, each a full re-enumeration that also re-reads/proc/diskstatsand/sys/block/*/queue/rotational— neither used here. 3.6-33 ms measured, so not a perf problem, butnew_with_refreshed_list_specifics(DiskRefreshKind::nothing().with_storage(true))called once would be cheaper and would fix themax_by_key-returns-last tie-break nondeterminism on duplicate mount points. - Retrying a cached download under-estimates. No cache-hit check in
install_tarball_runtimeordownload_file, so a re-run always re-downloads;write_file_atomicallywrites a sibling temp then renames, so true peak is archive + existing cached copy + margin while the preflight asks only archive + margin. Under-asks, so late failure, not a false refusal.engines/lemonade/src/lib.rs:976-979already implements the cache-hit pattern. - The body says the
proc_lifecyclefailures are "fixed by #169", but #169 is an open unmerged PR — reads as landed. Also says 15 unit tests; the diff adds 17.
Verified clean
available_space()usesf_bavail, notf_bfree— the safe direction.sysinfo-0.34.2/src/unix/linux/disk.rs:205-229. Confirmed byte-exact againststat -fhere:/reports 975,423,778,816 (bavail·bsize 975,420,985,344) versus bfree·bsize 1,030,413,340,672 — ~55 GB apart. Root-reserved blocks correctly excluded.Path::starts_withis component-wise, so the/datavs/databasetrap doesn't apply — verified empirically (/database/fileagainst[("/",…),("/data",…)]correctly picks/;/mnt/dataagainst[("/mnt/d",…)]returnsNone). Nothing guards against a future refactor to string comparison, though.- Arithmetic is overflow-safe —
estimated_extracted_size(u64::MAX) == u64::MAX,with_margin(u64::MAX) == u64::MAX, both tested. nearest_existing_ancestoredge cases are sound — relative paths resolve against cwd; a deleted cwd makescurrent_dir()fail →Unknown→ fails open; a..tail can't smuggle a mismatched filesystem pastselect_mount; a mode-000 directory stops the walk at that directory rather than skipping past it.- The dependency claim is correct. The
Cargo.lockdiff is genuinely one line (+ "sysinfo",underrocm-core) — no new[[package]], no version change, no transitive additions.sysinfo 0.34.2is already atTHIRD_PARTY_NOTICES.txt:10873;xtask/src/tpn.rs:100-109runscargo about generate --workspaceandabout.tomlhas no member allowlist, so scope is the whole workspace andcrates/rocm-dash-collectorsalready held the dep. Nothing new enters the covered graph. - Merging with #165 conflicts loudly, not silently.
git merge-tree --write-treeagainst #165's live head8c1ec0cgives a hard content conflict in bothapps/rocm/src/therock.rsandcrates/rocm-core/src/lib.rs— so no clean-merge-but-broken hazard. Resolution still needs care: #165 replaces the exact lines this PR edits with a call tostream_to_path_atomically, and.map_err(|e| map_write_error(e, destination))won't typecheck against itsanyhow::Errorreturn, so the mapping has to move onto the realio::copy— and whoever resolves must pass&partial, notdestination, or the message namessdk.tar.gzwhen the file that failed issdk.tar.gz.part-<pid>-<ts>. The preflight target is fine either way since the.partfile is a same-directory sibling. Worth landing #165 first — rebasing this onto the.partflow is a same-author fixup; the reverse hands an unfamiliar feature to #165's author. - Locally:
cargo fmt --all -- --checkclean;cargo clippy --locked --workspace --all-targets -- -D warningsclean;cargo test -p rocm-core --lib disk_space13/13;cargo test --workspace --all-targets --no-fail-fastgreen everywhere except the two knownproc_lifecycleWSL2 failures. Leak scan clean (one hit, a pre-existing public endpoint in unchanged context). 21/21 CI checks green.
Not verified: Windows runtime behaviour (no host; mingw link fails, no wine) — see the cheap check suggested under #2; cross-filesystem symlink resolution (no writable second filesystem in this sandbox); and cargo xtask tpn --check (cargo-about not installed — the licensing conclusion rests on reading tpn.rs, about.toml, the notices file and the lock diff, plus CI's passing notices job).
The preflight picked the longest mount point that prefixed the target path. When the path's real mount was absent from the platform's mount list the filter did not fail — it fell through to the nearest listed ancestor, in practice the root filesystem, and reported that filesystem's free space as the target's. sysinfo omits tmpfs by default and skips NFS and CIFS unless opted in, so a cache directory on a network home or a tmpfs /tmp was reported with a completely unrelated number. The download check hard-fails, so this refused valid installs citing a filesystem the download would never touch, with no bypass. Cross-check the resolved path's device ID against the selected mount point's and report the space as unknown on a mismatch, which the design already treats as "never block". Enable sysinfo's linux-tmpfs feature so tmpfs mounts are measured rather than merely detected as unknown; linux-netdevs stays off because statvfs on a hard-mounted share can block indefinitely. Also in the same check: - Strip Windows verbatim path prefixes before matching. canonicalize yields \\?\C:\..., whose Prefix variant never equals a mount point's C:\, so every Windows path resolved to no mount and the preflight was a silent no-op there. Compare case-insensitively on Windows too. - Make the safety margin proportional (5% of the payload, floor 32 MiB) instead of a flat 256 MiB, which turned a 20 MiB uv download into a 276 MiB requirement and refused it on small volumes. - Map tar's out-of-space stderr to the same plain-language message as direct writes, so the advisory extraction check no longer leaves the raw error as the outcome when it is right. - Ignore an implausible Content-Length rather than refusing on it; the header is unauthenticated and never cross-checked against the body. - Bound the HEAD probe's connect phase, which otherwise defaults to 30s and outlives the intended ceiling on a host that blackholes. - Return the extraction warning instead of printing it, so it appears in the install report rather than ahead of it. Thread the resolved free-space figure through the policy layer so the refusal paths are testable against synthetic values, and replace the two tests that passed vacuously. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Summary
Check free disk space before starting a multi-gigabyte download or extraction, and report an out-of-space failure in plain language instead of a raw OS error.
Root cause
Nothing in the codebase looked at free space, and nothing handled the resulting failure. A user with a nearly-full disk got a low-level write error partway through an install rather than an upfront statement of what was needed. Verified by searching
apps/,crates/,engines/, andxtask/foravailable_space,free_space,statvfs,ENOSPC,StorageFull,disk_space,fs2, and prose variants — zero hits, and nothing mappedErrorKind::StorageFullto a user-facing message.What this adds
A new
crates/rocm-core/src/disk_space.rs:mount_for_path/available_space_for_pathresolve the filesystem that will actually hold a path — walking up to the nearest existing ancestor, so a not-yet-created destination still resolves correctly, then picking the longest matching mount point.ensure_space_for(hard fail) andwarn_if_low_space(advisory).map_write_errorfor direct writes, andsubprocess_full_disk_errorfor a helper process such astar, whose out-of-space failure arrives as stderr text rather than anio::Error.The SDK tarball install path gains a preflight before download and extraction, plus a best-effort
Content-Lengthprobe.Technical decisions
Free space is only reported when the filesystem is positively identified. Longest-prefix mount matching is only correct if every mount is listed, and it is not:
sysinfoomits tmpfs by default and skips NFS/CIFS unless opted in. When the real mount is missing the prefix filter does not fail — it falls through to the nearest listed ancestor, in practice/, and reports an unrelated filesystem's free space. Since the download check hard-fails, that refuses a valid install citing a filesystem the download never touches. A device-ID cross-check between the resolved path and the selected mount point rejects that case, reporting the space as unknown instead.Unknown space never blocks. If the filesystem cannot be identified, or the
HEADprobe fails or omitsContent-Length, the check passes silently. This is what makes the check above safe: it converts "confidently wrong" into "no opinion".linux-tmpfson,linux-netdevsoff. Enabling tmpfs enumeration measures a cache or install root under a tmpfs/tmp(the Fedora and Arch default) rather than merely declining to guess. Network filesystems stay off:statvfson a hard-mounted share can block indefinitely, which is not an acceptable cost for a preflight. NFS and CIFS paths therefore report unknown and fail open.Hard-fail on the download, warn on the extraction.
Content-Lengthis exact, so refusing up front saves a doomed multi-gigabyte transfer. The extracted size is only an estimate, so a shortfall there is a warning. Because that warning can be right,tar's out-of-space stderr is mapped to the same plain-language message.An implausible
Content-Lengthis ignored rather than acted on. The header is unauthenticated and is never cross-checked against the body the GET delivers, so an inflated value would refuse an install that would succeed. Past a ceiling the preflight is skipped;download_filestill checks the real buffered body length before writing.The margin is proportional, not flat.
max(payload / 20, 32 MiB).download_file_to_pathis a general-purpose helper — its other callers fetch theuvbinary and similar — and a flat 256 MiB margin turned a 20 MiB download into a 276 MiB requirement, refusing it on small volumes. At SDK-tarball scale the proportional part lands in the same range as the old constant.Extracted size is estimated at 4x compressed. No manifest or index field carries the uncompressed size. Observed gzip ratios on these tarballs run about 2-3x, so 4x is a deliberate conservative upper bound. Documented on the constant.
Windows
canonicalizereturns verbatim paths (\\?\C:\...) whosePrefixvariant never equals a mount point'sC:\, so component-wisestarts_withfailed for every mount and the preflight was a silent no-op on Windows. Verbatim prefixes are now stripped before matching, and comparison is case-insensitive there since volumes are.I have no Windows host, so this is reasoned from the std source and covered by unit tests, not observed at runtime. The prefix-stripping is pure string handling and is exercised on every platform; the end-to-end mount selection against a
C:\-style table is a#[cfg(windows)]test that runs in CI but not locally. Mapped network drives remain invisible on Windows for the same reason as NFS on Linux, and fail open.Non-goals
--format wheelpath gets no preflight — Free-space preflight does not cover the default--format wheelinstall path #191.uv pip installresolves the wheel set at run time, so there is no cheap upfront size signal comparable to a tarball'sContent-Length; it needs its own approach. Because the default path is uncovered this is Refs [Issue]: No free-space check before multi-GB downloads and extractions #159, not a fix for it.run_commanddiscards subprocess stderr on Windows, sotar's out-of-space text is not available to map there.Dependencies
sysinfowas promoted to[workspace.dependencies]with thelinux-tmpfsfeature, androcm-corenow uses it. No new package enters the graph — theCargo.lockdiff is a single edge — soTHIRD_PARTY_NOTICES.txtandabout.tomlare unchanged (sysinfo 0.34.2is already listed). I verified this by inspecting the lock diff rather than by runningcargo xtask tpn --check, since cargo-about was not installed here; CI's notices check confirms.Tests
Unit tests covering byte formatting, the proportional margin at both
uvand SDK scale, saturating arithmetic, mount selection (longest prefix, no match, siblings, verbatim Windows prefixes), the device-ID rejection of a mount that does not own the path, ancestor walking, message content,StorageFullmapping versus passthrough,tarENOSPC recognition versus passthrough, and a preflight that skips when theHEADprobe is unreachable.The refusal policy is driven through an internal seam that takes the resolved free-space figure, so both the refusal and the fail-open paths are asserted against synthetic values rather than whatever the host happens to have free.
Verification
cargo fmt --all -- --check,cargo clippy --locked --workspace --all-targets -- -D warnings, andcargo test --workspace --all-targets --no-fail-fastall pass locally, except twoproc_lifecyclefailures that are pre-existing on this WSL2 host and unrelated to this change — tracked in #168, with #169 open to address them.The mis-attribution bug was reproduced before fixing: a path under a tmpfs
/dev/shmreported the root filesystem's free space (hundreds of GB against a real 64 MiB). After the change that path reports 64 MiB, and a path on a filesystem that remains unlisted reports unknown and does not block.Refs #159