From f0e65b9738f02695261ded2cdddab255f1057ab5 Mon Sep 17 00:00:00 2001 From: Wanjohi Date: Wed, 2 Sep 2026 13:34:00 +0300 Subject: [PATCH] fix(nesdoctor): bloat was measured against the median, and could grade a bad line A Found by running the published one-liner, which is the only reason it was found: `up=34Mbps rtt=188ms rttload=181ms bloat=+0ms grade=A` on a connection that measured +115 ms and grade F three hours earlier. The idle baseline was the median of twelve handshakes to one anycast address. On the development connection those twelve came back **bimodal**: [56, 56, 57, 59, 60, 176, 177, 179, 179, 179, 182, 368] min 56 p50 177 max 368 spread 312 ms on an *idle* link Two points of presence answering. The median therefore lands wherever the split happens to fall, and when it lands high the loaded median comes in *below* it, the difference goes negative, `.max(0.0)` clamps it to zero, and the headline number reports grade A. That is the one error direction that cannot be tolerated here. A tool whose whole pitch is a number nobody else shows you has no business saying "your line is fine" about a line that is not. Bloat is now measured against the **minimum**. Queueing is delay above the floor the path can achieve, so the floor is the baseline -- which is also how every bufferbloat test does it. Twenty samples rather than twelve. The distance verdict deliberately keeps the **median**, because it asks a different question. Bloat asks how much queueing is added, so its baseline is the best case. HOST-READY-LOCAL asks what a player will actually see, so it takes the typical case: on a link that is bimodal between 56 ms and 180 ms, the floor would call it near when half of all connections are not. Both are now reported, and the gap between them is itself the finding -- a floor of 55 ms against a typical of 180 ms says the route is the problem, which no single number could have said. Verified on the same connection: floor 55, typical 180, loaded 95, **+39 ms, grade C**, verdict HOST-NET. Defensible, and no longer flattering. Version to 0.1.1. Submissions carry it, so any row with `v=0.1.0` has a grade that cannot be trusted. --- Cargo.lock | 2 +- apps/nesdoctor/Cargo.toml | 2 +- apps/nesdoctor/README.md | 14 ++++++++++---- apps/nesdoctor/src/main.rs | 3 ++- apps/nesdoctor/src/net.rs | 34 ++++++++++++++++++++++++++++++---- apps/nesdoctor/src/report.rs | 21 ++++++++++++++++++--- 6 files changed, 62 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4d2180f9..9cb552db 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2462,7 +2462,7 @@ dependencies = [ [[package]] name = "nesdoctor" -version = "0.1.0" +version = "0.1.1" dependencies = [ "anyhow", "clap", diff --git a/apps/nesdoctor/Cargo.toml b/apps/nesdoctor/Cargo.toml index df0fdacc..aaa0ff92 100644 --- a/apps/nesdoctor/Cargo.toml +++ b/apps/nesdoctor/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "nesdoctor" -version = "0.1.0" +version = "0.1.1" edition.workspace = true license.workspace = true repository.workspace = true diff --git a/apps/nesdoctor/README.md b/apps/nesdoctor/README.md index 7a256391..6a9b0ed5 100644 --- a/apps/nesdoctor/README.md +++ b/apps/nesdoctor/README.md @@ -62,12 +62,18 @@ their connection adds when it is busy**, and for anything interactive that is th figure that decides it: ``` - upstream 28 Mbps - latency, idle 179 ms - latency, loaded 198 ms - added under load +19 ms grade B + upstream 39 Mbps + latency, idle floor 55 ms + latency, idle typ. 180 ms + latency, loaded 95 ms + added under load +39 ms grade C ``` +Two idle figures because they answer different questions. **Floor** is the best +the path can do, and queueing is everything above it — so that is what the +bloat number is measured against. **Typical** is what a connection actually +gets, and where the two differ this much, the route itself is the problem. + A 500 Mbps uplink that queues for 300 ms under load cannot carry a game. A 25 Mbps one with `fq_codel` or CAKE can. If your grade is C or F it is almost always a router setting rather than a line you need to upgrade. diff --git a/apps/nesdoctor/src/main.rs b/apps/nesdoctor/src/main.rs index f4cfaecd..2a31cec7 100644 --- a/apps/nesdoctor/src/main.rs +++ b/apps/nesdoctor/src/main.rs @@ -353,7 +353,8 @@ fn print_net(n: &net::NetReport) { .unwrap_or_else(|| "—".into()) }; println!(" upstream {}", f(n.upstream_mbps, " Mbps")); - println!(" latency, idle {}", f(n.idle_rtt_ms, " ms")); + println!(" latency, idle floor {}", f(n.idle_rtt_ms, " ms")); + println!(" latency, idle typ. {}", f(n.idle_rtt_p50_ms, " ms")); if n.idle_rtt_ms.is_some_and(|r| r > 60.0) { println!( " \x1b[2m That is the round trip to the *nearest* major network, so it is a\x1b[0m" diff --git a/apps/nesdoctor/src/net.rs b/apps/nesdoctor/src/net.rs index 0b11f064..dffc7511 100644 --- a/apps/nesdoctor/src/net.rs +++ b/apps/nesdoctor/src/net.rs @@ -55,7 +55,12 @@ const CHUNK: usize = 1 << 20; // 1 MiB per write #[derive(Debug, Serialize)] pub struct NetReport { + /// The **floor** of the idle distribution, which is the propagation delay + /// the path is capable of. Baseline for [`NetReport::bloat_ms`]. pub idle_rtt_ms: Option, + /// The **typical** idle round trip. Reported separately because the two + /// answer different questions and, on a badly routed link, differ wildly. + pub idle_rtt_p50_ms: Option, pub loaded_rtt_ms: Option, pub loaded_rtt_p95_ms: Option, /// Loaded minus idle: the queue, in milliseconds. @@ -77,6 +82,7 @@ impl NetReport { fn unavailable(note: &str) -> Self { Self { idle_rtt_ms: None, + idle_rtt_p50_ms: None, loaded_rtt_ms: None, loaded_rtt_p95_ms: None, bloat_ms: None, @@ -116,8 +122,14 @@ pub fn run() -> NetReport { }; // --- idle baseline --------------------------------------------------- - let idle = sample_rtt(addr, 12, Duration::from_millis(120)); - let Some(idle_p50) = percentile(&idle, 0.50) else { + // + // Twenty samples, not twelve. Measured on a Nairobi connection + // 2026-09-02, twelve idle handshakes to one anycast address came back + // **bimodal** -- `[56, 56, 57, 59, 60, 176, 177, 179, 179, 179, 182, 368]`, + // two different points of presence answering, 312 ms of spread on an *idle* + // link. + let idle = sample_rtt(addr, 20, Duration::from_millis(120)); + let Some(idle_min) = percentile(&idle, 0.0) else { return NetReport::unavailable( "no TCP handshake completed to 1.1.1.1:443 — a firewall may block it, so the \ latency half could not run", @@ -160,7 +172,19 @@ pub fn run() -> NetReport { let loaded_p50 = percentile(&loaded, 0.50); let loaded_p95 = percentile(&loaded, 0.95); - let bloat = loaded_p50.map(|l| (l - idle_p50).max(0.0)); + let idle_p50 = percentile(&idle, 0.50); + + // Bloat is measured against the **minimum**, not the median. + // + // Queueing is delay *above the floor the path can do*, so the floor is the + // baseline; that is also how every bufferbloat test does it. Using the + // median was a real bug and it failed in the dangerous direction: on the + // bimodal link above, the idle median landed at 188 ms while the loaded + // median came back 181 ms, so the difference went negative, clamped to + // zero, and reported **grade A on a connection that measures +115 ms and + // grade F**. A tool whose headline number can say "fine" about a line that + // is not fine has no business being trusted with the rest. + let bloat = loaded_p50.map(|l| (l - idle_min).max(0.0)); let note = match (&upstream_mbps, &bloat) { (None, _) => "upstream could not be measured (the upload sink was unreachable), so the \ @@ -175,7 +199,8 @@ pub fn run() -> NetReport { }; NetReport { - idle_rtt_ms: Some(idle_p50), + idle_rtt_ms: Some(idle_min), + idle_rtt_p50_ms: idle_p50, loaded_rtt_ms: loaded_p50, loaded_rtt_p95_ms: loaded_p95, bloat_ms: bloat, @@ -204,6 +229,7 @@ fn sample_rtt(addr: SocketAddr, n: usize, gap: Duration) -> Vec { out } +/// `p` of 0.0 gives the minimum, which is what the bloat baseline uses. fn percentile(v: &[f64], p: f64) -> Option { if v.is_empty() { return None; diff --git a/apps/nesdoctor/src/report.rs b/apps/nesdoctor/src/report.rs index 1f645f3d..9012bd7b 100644 --- a/apps/nesdoctor/src/report.rs +++ b/apps/nesdoctor/src/report.rs @@ -119,7 +119,17 @@ pub fn verdict(sys: &SysInfo, host: &HostReport, net: &NetReport) -> Verdict { (Some(up), Some(bloat)) => { if up < MIN_UP_MBPS || bloat > MAX_BLOAT_MS { Verdict::HostBlockedByNetwork - } else if net.idle_rtt_ms.is_some_and(|r| r > FAR_RTT_MS) { + // Deliberately the *median* and not the floor. Bloat asks "how much + // queueing is added", so its baseline is the best the path can do. + // This asks "what will a player actually see", so it takes the + // typical case -- and on a link whose idle latency is bimodal + // between 56 ms and 180 ms, the floor would call it near when half + // of all connections are not. + } else if net + .idle_rtt_p50_ms + .or(net.idle_rtt_ms) + .is_some_and(|r| r > FAR_RTT_MS) + { Verdict::HostReadyLocalOnly } else { Verdict::HostReady @@ -185,8 +195,10 @@ pub fn summary_line(f_: &Full) -> String { match (net.upstream_mbps, net.bloat_ms, net.grade) { (Some(up), Some(b), Some(g)) => f.push(format!( - "up={up:.0}Mbps rtt={}ms bloat=+{b:.0}ms grade={g}", - net.idle_rtt_ms.map_or("?".into(), |r| format!("{r:.0}")) + "up={up:.0}Mbps rtt={}/{}ms bloat=+{b:.0}ms grade={g}", + net.idle_rtt_ms.map_or("?".into(), |r| format!("{r:.0}")), + net.idle_rtt_p50_ms + .map_or("?".into(), |r| format!("{r:.0}")) )), _ => f.push("net=unmeasured".into()), } @@ -442,6 +454,9 @@ pub fn submit_url(base: &str, f_: &Full) -> String { if let Some(v) = net.idle_rtt_ms { put("rtt", format!("{v:.0}")); } + if let Some(v) = net.idle_rtt_p50_ms { + put("rttidle50", format!("{v:.0}")); + } if let Some(v) = net.loaded_rtt_ms { put("rttload", format!("{v:.0}")); }