github-mirrors/netris-nestri
1
0
Fork 0
mirror of https://github.com/nestriness/nestri.git synced 2026-09-19 09:15:19 +03:00
Code Issues Packages Projects Releases Wiki Activity

fix(nesdoctor): report storage properly, and stop being blind on Windows

Browse Source 
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 "" <url>`
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.
This commit is contained in:
Wanjohi
2026-09-02 16:10:05 +03:00
parent a84886861c
commit 786267f30a
6 changed files with 214 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
.github/workflows/release-nesdoctor.yml vendored
View File

@@ -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: |

2
Cargo.lock generated
View File

@@ -2462,7 +2462,7 @@ dependencies = [
[[package]]
name = "nesdoctor"
version = "0.2.1"
version = "0.2.2"
dependencies = [
"anyhow",
"clap",

2
apps/nesdoctor/Cargo.toml
View File

@@ -1,6 +1,6 @@
[package]
name = "nesdoctor"
version = "0.2.1"
version = "0.2.2"
edition.workspace = true
license.workspace = true
repository.workspace = true

25
apps/nesdoctor/src/main.rs
View File

@@ -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 \

111
apps/nesdoctor/src/report.rs
View File

@@ -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 "" <url>` 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"
);
}
}
}
}

79
apps/nesdoctor/src/sys.rs
View File

@@ -63,6 +63,14 @@ pub struct Disk {
/// *separate devices*) cannot be answered at all.
pub source: Option<String>,
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<f64>,
}
pub fn probe() -> SysInfo {
@@ -348,38 +356,75 @@ fn disks() -> Vec<Disk> {
let Ok(avail_kb) = f[3].parse::<f64>() else {
continue;
};
let size_kb = f[1].parse::<f64>().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::<f64>() {
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::<f64>() else {
continue;
};
let used = f.get(2).and_then(|u| u.trim().parse::<f64>().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));