From 786267f30a53d11ba0da83d9918080d224dfae1a Mon Sep 17 00:00:00 2001 From: Wanjohi Date: Wed, 2 Sep 2026 16:10:05 +0300 Subject: [PATCH] fix(nesdoctor): report storage properly, and stop being blind on Windows A submission from a team machine with four drives and 22 TiB reported `disk=8880`, and the field was not wrong so much as meaningless: it was the free space on the single largest mount, with no capacity anywhere and no total. A content store is sized against capacity. Storage now reports four things, because they answer different questions and one number could not: diskfree total free across every real filesystem disksize total capacity diskmax the largest single filesystem, which is the real ceiling for any one store -- a dataset cannot be spread across drives disks how many there are The ambiguous `disk` key is gone rather than silently redefined, so old rows stay readable as what they were. `Get-PSDrive` reports Free *and* Used and we were reading only Free, hence no capacity on Windows at all. Pseudo-filesystems are now excluded by *type* rather than by mount path. Path filtering missed `/tmp` on a tmpfs, whose free space is RAM -- so 7 GiB of memory was being added to a storage total, which is exactly the sort of number a capacity plan gets built on. ## The real finding, which was not about disks "We are working blind on Windows" is correct, and both Windows bugs this tool has had prove it: a virtual display adapter reported as the GPU, and a URL truncated at its first `&`. Both were in code that only runs on Windows, both were found by a person reading the results channel, and neither could have been found here -- the development machine is Linux and `xdg-open` never sees a shell. Two things about that, and the first is the one that generalises. `OPENERS` is now a const with a test asserting the property that actually matters: **never hand a URL to anything that will re-parse it.** No `cmd`, no `sh`, no `powershell`, no `start` builtin, and no argument that looks like it wants the URL interpolated into it. Unlike the bug, that is checkable on every platform in a millisecond. Verified by reintroducing `cmd /C start "" ` and confirming the test fails with the right message, then reverting. And CI already runs a real Windows machine and a real macOS one -- we simply were not looking at them. Each smoke-tested target now prints its full report and JSON into a collapsed log group. Deliberately not `set -e`: this step is for looking, and a probe that misbehaves on a runner must not fail a release. It turns "working blind" into "looking at it once per release", which would have shown the Parsec adapter problem the first time a Windows binary was ever built. Version to 0.2.2. --- .github/workflows/release-nesdoctor.yml | 27 ++++++ Cargo.lock | 2 +- apps/nesdoctor/Cargo.toml | 2 +- apps/nesdoctor/src/main.rs | 25 +++++- apps/nesdoctor/src/report.rs | 111 +++++++++++++++++++++--- apps/nesdoctor/src/sys.rs | 79 +++++++++++++---- 6 files changed, 214 insertions(+), 32 deletions(-) diff --git a/.github/workflows/release-nesdoctor.yml b/.github/workflows/release-nesdoctor.yml index 42d366ef..9bb12c4f 100644 --- a/.github/workflows/release-nesdoctor.yml +++ b/.github/workflows/release-nesdoctor.yml @@ -104,6 +104,33 @@ jobs: fi test -s "$RUNNER_TEMP/nd.json" + # Eyes on the platforms the developer machine is not. + # + # Every Windows bug this tool has had was found by a person reading the + # results channel: a virtual display adapter reported as the GPU, and a + # URL truncated at its first `&`. Both were in code that only runs on + # Windows, and the development machine is Linux -- so nobody had ever + # seen what these probes return on the platform most of the audience + # uses. + # + # CI already runs a real Windows machine and a real macOS one. Printing + # the full report from each is nearly free and turns "we are working + # blind" into "we are looking at it once per release". + - name: Show what the probes actually return here + if: matrix.smoke + shell: bash + run: | + set -uo pipefail + BIN="target/${{ matrix.target }}/release/${{ matrix.bin }}" + echo "::group::${{ matrix.target }} — full report" + # Not `set -e`: this step is for looking, and a probe that fails on a + # runner must not fail the release. + "$BIN" --no-net --no-steam --json "$RUNNER_TEMP/probe.json" < /dev/null || true + echo "::endgroup::" + echo "::group::${{ matrix.target }} — JSON" + cat "$RUNNER_TEMP/probe.json" 2>/dev/null || echo "(no json written)" + echo "::endgroup::" + - name: Package shell: bash run: | diff --git a/Cargo.lock b/Cargo.lock index 203ef211..ff020078 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2462,7 +2462,7 @@ dependencies = [ [[package]] name = "nesdoctor" -version = "0.2.1" +version = "0.2.2" dependencies = [ "anyhow", "clap", diff --git a/apps/nesdoctor/Cargo.toml b/apps/nesdoctor/Cargo.toml index 98e0986d..3965a575 100644 --- a/apps/nesdoctor/Cargo.toml +++ b/apps/nesdoctor/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "nesdoctor" -version = "0.2.1" +version = "0.2.2" edition.workspace = true license.workspace = true repository.workspace = true diff --git a/apps/nesdoctor/src/main.rs b/apps/nesdoctor/src/main.rs index 886bc31e..e4c7db57 100644 --- a/apps/nesdoctor/src/main.rs +++ b/apps/nesdoctor/src/main.rs @@ -389,14 +389,33 @@ fn print_sys(s: &sys::SysInfo) { .unwrap_or_default() ); } - for d in s.disks.iter().take(3) { + for d in s.disks.iter().take(5) { println!( - " {} · {} · {:.0} GiB free", + " {} · {} · {:.0} GiB free{}", d.mount, d.fs.clone().unwrap_or_else(|| "?".into()), - d.free_gib + d.free_gib, + d.size_gib + .map(|s| format!(" of {s:.0} GiB")) + .unwrap_or_default() ); } + if s.disks.len() > 1 { + let free: f64 = s.disks.iter().map(|d| d.free_gib).sum(); + let size: f64 = s.disks.iter().filter_map(|d| d.size_gib).sum(); + println!( + " {} filesystems · {free:.0} GiB free{}", + s.disks.len(), + if size > 0.0 { + format!(" of {size:.0} GiB total") + } else { + String::new() + } + ); + } + if s.disks.len() > 5 { + println!(" \x1b[2m(showing the five largest)\x1b[0m"); + } match (s.powered_hours_per_day, s.powered_span_days) { (Some(h), Some(days)) => println!( " powered {h:.1} h/day, averaged over {days:.0} days of boot history\n \ diff --git a/apps/nesdoctor/src/report.rs b/apps/nesdoctor/src/report.rs index 9daba80e..53416b4c 100644 --- a/apps/nesdoctor/src/report.rs +++ b/apps/nesdoctor/src/report.rs @@ -204,8 +204,19 @@ pub fn summary_line(f_: &Full) -> String { _ => f.push("net=unmeasured".into()), } - if let Some(d) = sys.disks.first() { - f.push(format!("disk={:.0}G", d.free_gib)); + // Total free, and the largest single filesystem, because one dataset + // cannot span drives. + let free_total: f64 = sys.disks.iter().map(|d| d.free_gib).sum(); + if free_total > 0.0 { + let max = sys.disks.first().map_or(0.0, |d| d.free_gib); + f.push(if sys.disks.len() > 1 { + format!( + "disk={free_total:.0}G free/{max:.0}G largest x{}", + sys.disks.len() + ) + } else { + format!("disk={free_total:.0}G") + }); } if let (Some(h), Some(days)) = (sys.powered_hours_per_day, sys.powered_span_days) { f.push(format!("powered={h:.0}h/d over {days:.0}d")); @@ -485,8 +496,25 @@ pub fn submit_url(base: &str, f_: &Full) -> String { put("edge", r.clone()); } + // Four fields rather than one, because `disk=8880` was ambiguous and wrong + // for its purpose. A submission from a four-drive, 22 TiB machine reported + // the free space on its single largest mount and nothing else. + // + // Both the totals and the largest single mount matter, and they answer + // different questions: a content store is sized against total capacity, + // but one dataset cannot be spread across drives, so the largest single + // filesystem is the real ceiling for any one store. The old ambiguous + // `disk` key is gone rather than silently redefined. + let free_total: f64 = sys.disks.iter().map(|d| d.free_gib).sum(); + let size_total: f64 = sys.disks.iter().filter_map(|d| d.size_gib).sum(); + if free_total > 0.0 { + put("diskfree", format!("{free_total:.0}")); + } + if size_total > 0.0 { + put("disksize", format!("{size_total:.0}")); + } if let Some(d) = sys.disks.first() { - put("disk", format!("{:.0}", d.free_gib)); + put("diskmax", format!("{:.0}", d.free_gib)); if let Some(fs) = &d.fs { put("diskfs", fs.clone()); } @@ -638,6 +666,17 @@ pub fn submit_contents(steam: &SteamReport, answers: &Answers) -> Vec<&'static s v } +/// Every program we will hand a URL to, in order. +/// +/// A `const` rather than a local, so the tests at the bottom of this file can +/// assert the property that actually matters about this list. +const OPENERS: [(&str, &[&str]); 4] = [ + ("xdg-open", &[]), + ("open", &[]), // macOS + ("rundll32", &["url.dll,FileProtocolHandler"]), // Windows + ("wslview", &[]), // WSL, where xdg-open is often absent +]; + /// Hand a URL to whatever the desktop uses to open links. pub fn open_in_browser(url: &str) -> bool { use std::process::{Command, Stdio}; @@ -660,13 +699,7 @@ pub fn open_in_browser(url: &str) -> bool { // protocol handler without any command interpreter in the path, so nothing // re-parses it. `explorer.exe` also works and returns a non-zero exit // status even on success, which would make the caller think it failed. - let attempts: [(&str, &[&str]); 4] = [ - ("xdg-open", &[]), - ("open", &[]), // macOS - ("rundll32", &["url.dll,FileProtocolHandler"]), // Windows - ("wslview", &[]), // WSL, where xdg-open is often absent - ]; - for (cmd, args) in attempts { + for (cmd, args) in OPENERS { if Command::new(cmd) .args(args) .arg(url) @@ -681,3 +714,61 @@ pub fn open_in_browser(url: &str) -> bool { } false } + +#[cfg(test)] +mod tests { + use super::OPENERS; + + /// The regression test for the worst bug this program has had. + /// + /// `cmd /C start "" ` destroyed every Windows submission for a day. + /// `cmd.exe` re-parses its own command line and treats `&` as a command + /// separator, so the URL was cut at its first one — immediately after + /// `v=` — and the browser opened a link carrying a version number and + /// nothing else, behind a thank-you page. + /// + /// It was found by a person reading the results channel, because the + /// developer machine is Linux and `xdg-open` never sees a shell. The + /// property that prevents the whole class is **never hand a URL to + /// anything that will re-parse it** — and unlike the bug, that is + /// checkable on every platform, in a millisecond, forever. + #[test] + fn no_opener_goes_through_a_command_interpreter() { + const INTERPRETERS: [&str; 8] = [ + "cmd", + "cmd.exe", + "sh", + "bash", + "zsh", + "powershell", + "powershell.exe", + "pwsh", + ]; + for (cmd, args) in OPENERS { + assert!( + !INTERPRETERS.contains(&cmd), + "{cmd} re-parses its arguments; a URL containing & will be truncated" + ); + // `start` exists only as a cmd builtin, so seeing it means a shell + // is involved even when the program name looks innocent. + assert!( + !args.contains(&"start"), + "{cmd} {args:?} looks like a shell invocation" + ); + } + } + + /// The URL is always passed as its own argument, never interpolated into + /// one — the other half of the same property. + #[test] + fn openers_take_fixed_arguments_only() { + for (_, args) in OPENERS { + for a in args { + assert!( + !a.contains("{}") && !a.contains('&') && !a.contains('?'), + "argument {a:?} looks like it wants the URL interpolated into it" + ); + } + } + } +} diff --git a/apps/nesdoctor/src/sys.rs b/apps/nesdoctor/src/sys.rs index 24548603..4fd42b85 100644 --- a/apps/nesdoctor/src/sys.rs +++ b/apps/nesdoctor/src/sys.rs @@ -63,6 +63,14 @@ pub struct Disk { /// *separate devices*) cannot be answered at all. pub source: Option, pub free_gib: f64, + /// Total capacity, not just what is free. + /// + /// Added after a submission from a machine with four drives and 22 TiB + /// reported `disk=8880` -- the free space on the single largest mount. A + /// content store is sized against capacity, and reporting only the largest + /// mount's free space understates a multi-drive machine by however many + /// drives it has. + pub size_gib: Option, } pub fn probe() -> SysInfo { @@ -348,38 +356,75 @@ fn disks() -> Vec { let Ok(avail_kb) = f[3].parse::() else { continue; }; + let size_kb = f[1].parse::().ok(); let source = f[0].to_string(); let mount = f[5..].join(" "); - // Pseudo-filesystems are noise, and tmpfs free space is RAM. - if ["/dev", "/sys", "/proc", "/run", "/boot", "/snap"] - .iter() - .any(|p| mount.starts_with(p)) + let fs = fs_type(&mount); + + // Filter by filesystem type, not by mount path. Filtering paths + // missed `/tmp` on a tmpfs, whose "free space" is RAM -- so a + // 7 GiB tmpfs was being added to a storage total, which is exactly + // the sort of number a capacity plan would then be built on. + const PSEUDO: [&str; 9] = [ + "tmpfs", + "ramfs", + "devtmpfs", + "devfs", + "squashfs", + "overlay", + "efivarfs", + "fuse.portal", + "iso9660", + ]; + if fs.as_deref().is_some_and(|f| PSEUDO.contains(&f)) { + continue; + } + // Paths still worth skipping regardless of what they are mounted as. + if [ + "/dev", + "/sys", + "/proc", + "/run", + "/boot", + "/snap", + "/var/lib/docker", + ] + .iter() + .any(|p| mount.starts_with(p)) { continue; } out.push(Disk { - fs: fs_type(&mount), + fs, mount, source: Some(source), free_gib: avail_kb / 1048576.0, + size_gib: size_kb.map(|k| k / 1048576.0), }); } } #[cfg(windows)] - if let Some(txt) = - ps("Get-PSDrive -PSProvider FileSystem | ForEach-Object { \"$($_.Name)|$($_.Free)\" }") - { + // Free *and* Used, so capacity is Free + Used. `Get-PSDrive` reports both + // and we were reading only Free. + if let Some(txt) = ps( + r#"Get-PSDrive -PSProvider FileSystem | ForEach-Object { "$($_.Name)|$($_.Free)|$($_.Used)" }"#, + ) { for line in txt.lines() { - if let Some((name, free)) = line.split_once('|') { - if let Ok(b) = free.trim().parse::() { - out.push(Disk { - mount: format!("{}:", name.trim()), - fs: None, - source: None, - free_gib: b / 1073741824.0, - }); - } + let f: Vec<&str> = line.split('|').collect(); + if f.len() < 2 { + continue; } + let Ok(free) = f[1].trim().parse::() else { + continue; + }; + let used = f.get(2).and_then(|u| u.trim().parse::().ok()); + out.push(Disk { + mount: format!("{}:", f[0].trim()), + fs: None, + source: None, + free_gib: free / 1073741824.0, + size_gib: used.map(|u| (free + u) / 1073741824.0), + }); } } out.sort_by(|a, b| b.free_gib.total_cmp(&a.free_gib));