From 5bef8a8dcac5afddfe29137ff53bfcab9d4dc088 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Mon, 5 Oct 2026 12:24:26 +0000 Subject: [PATCH] fix: promote a size that rounds up to a full unit in two more formatters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rocm_core::format_bytes` printed sizes just below a unit boundary as "1024.0 KiB": it promoted on the raw value (`>= 1024`) but printed with `{:.1}`, which rounds 1023.95 and up to 1024.0. Two other formatters carry the same shape: - `format_bytes_for_user` in the `rocm` binary, behind the `download: approved up to …` line: 1_048_525..=1_048_575 bytes printed "1024.0 KB" and 1_073_689_396..=1_073_741_823 printed "1024.0 MB". - `mib` and `mib_pair` in the dashboard, for VRAM: 1_048_525..=1_048_575 MiB printed "1024.0 GiB". Both now promote while the value as printed would reach 1024, comparing the rounded tenths exactly as the rocm-core fix does. `mib` and `mib_pair` share one helper, so the pair's unit can no longer drift from the single value's. Unit labels and the "bytes" wording are unchanged. Each fix carries pinned boundary examples and a two-edge property test (never a unit the size has outgrown; never one it has not reached), with proptest added as a dev-dependency of both crates at the version and features rocm-core already uses. Signed-off-by: Roman Inflianskas --- Cargo.lock | 2 + apps/rocm/Cargo.toml | 9 + apps/rocm/proptest-regressions/main.txt | 7 + apps/rocm/src/main.rs | 134 +++++++++++-- crates/rocm-dash-tui/Cargo.toml | 9 + .../proptest-regressions/ui/format.txt | 8 + crates/rocm-dash-tui/src/ui/format.rs | 188 +++++++++++++++--- 7 files changed, 320 insertions(+), 37 deletions(-) create mode 100644 apps/rocm/proptest-regressions/main.txt create mode 100644 crates/rocm-dash-tui/proptest-regressions/ui/format.txt diff --git a/Cargo.lock b/Cargo.lock index 9d5380e6d..175183d8f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3664,6 +3664,7 @@ dependencies = [ "flate2", "keyring-core", "libc", + "proptest", "rocm-core", "rocm-dash-core", "rocm-dash-daemon", @@ -3777,6 +3778,7 @@ dependencies = [ "futures", "http", "libc", + "proptest", "ratatui", "reqwest 0.13.4", "rig-core", diff --git a/apps/rocm/Cargo.toml b/apps/rocm/Cargo.toml index 3d4990869..7e6684067 100644 --- a/apps/rocm/Cargo.toml +++ b/apps/rocm/Cargo.toml @@ -58,3 +58,12 @@ apple-native-keyring-store = { version = "1.0", features = ["keychain"] } [target.'cfg(any(target_os = "linux", target_os = "freebsd", target_os = "openbsd"))'.dependencies] zbus-secret-service-keyring-store = { version = "1.0", features = ["rt-tokio-crypto-rust"] } + +[dev-dependencies] +# Property-based tests for the byte-size formatter behind the `download: +# approved up to …` line. "A size is printed in its own unit" has to hold at +# every unit boundary, and the narrow band where `{:.1}` rounding reaches 1024.0 +# is exactly the input a hand-picked example misses. Same version and features +# as rocm-core's, so no new crate or feature enters the lockfile. +# Test-only, so it adds nothing to any shipped binary. +proptest = { version = "1", default-features = false, features = ["std"] } diff --git a/apps/rocm/proptest-regressions/main.txt b/apps/rocm/proptest-regressions/main.txt new file mode 100644 index 000000000..02e28f303 --- /dev/null +++ b/apps/rocm/proptest-regressions/main.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 4f22a6b41e30f0e449b891bb36a3f8806e75d89ce2fda068a5cc0fe7ae194d84 # shrinks to bytes = 1048525 diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 450bd5d66..ef5b8bbdd 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -19306,19 +19306,23 @@ fn audit_event_plain_summary(event: &AuditEventRecord) -> &'static str { } fn format_bytes_for_user(bytes: u64) -> String { - const KB: f64 = 1024.0; - const MB: f64 = 1024.0 * KB; - const GB: f64 = 1024.0 * MB; - let bytes = bytes as f64; - if bytes >= GB { - format!("{:.1} GB", bytes / GB) - } else if bytes >= MB { - format!("{:.1} MB", bytes / MB) - } else if bytes >= KB { - format!("{:.1} KB", bytes / KB) - } else { - format!("{} bytes", bytes as u64) + const UNITS: [&str; 3] = ["KB", "MB", "GB"]; + // Below 1 KB the count is printed whole, so no rounding can disagree with + // the comparison. + if bytes < 1024 { + return format!("{bytes} bytes"); + } + let mut value = bytes as f64 / 1024.0; + let mut unit = 0; + // Promote while the value AS PRINTED would reach 1024, not merely while the + // raw value does: 1_048_575 bytes is 1023.999… KB, which `{:.1}` renders as + // "1024.0 KB". Comparing the rounded tenths, as `rocm_core::format_bytes` + // does, keeps every size in the unit it belongs to. + while unit + 1 < UNITS.len() && (value * 10.0).round() >= 10_240.0 { + value /= 1024.0; + unit += 1; } + format!("{value:.1} {}", UNITS[unit]) } const fn watcher_mode_plain_label(mode: WatcherMode) -> &'static str { @@ -26253,6 +26257,107 @@ model recipes assert_eq!(format_bytes(1_073_741_823), "1.0 GiB"); } + /// Just below a unit boundary the value rounds up to a full 1024 of the + /// SMALLER unit, which has to be reported as 1.0 of the larger one — the + /// same defect `rocm_core::format_bytes` had. Each pair is the last input + /// that still belongs to the smaller unit and the first that `{:.1}` rounds + /// up to 1024.0 of it; the second used to print "1024.0 KB" / "1024.0 MB". + #[test] + fn format_bytes_for_user_promotes_a_value_that_rounds_up_to_a_full_unit() { + assert_eq!(format_bytes_for_user(1023), "1023 bytes"); + assert_eq!(format_bytes_for_user(1024), "1.0 KB"); + assert_eq!(format_bytes_for_user(1_048_524), "1023.9 KB"); + assert_eq!(format_bytes_for_user(1_048_525), "1.0 MB"); + assert_eq!(format_bytes_for_user(1_048_575), "1.0 MB"); + assert_eq!(format_bytes_for_user(1_048_576), "1.0 MB"); + assert_eq!(format_bytes_for_user(1_073_689_395), "1023.9 MB"); + assert_eq!(format_bytes_for_user(1_073_689_396), "1.0 GB"); + assert_eq!(format_bytes_for_user(1_073_741_823), "1.0 GB"); + // GB is the top unit: nothing to promote to, so 1024 GB stays in it. + assert_eq!(format_bytes_for_user(1_099_511_627_776), "1024.0 GB"); + } + + /// The units `format_bytes_for_user` prints, smallest first. + const USER_BYTE_UNITS: [&str; 4] = ["bytes", "KB", "MB", "GB"]; + + /// Byte counts that actually visit the unit boundaries. A uniform `u64` + /// almost always lands far above the top unit, so on its own it never + /// samples the band where `{:.1}` rounding reaches 1024.0. The other arms + /// draw uniformly within one unit's range, and from a window just below + /// each rounded boundary (KB→MB, MB→GB) that scales with the boundary, as + /// the band does — see `rocm_core::disk_space`'s generator for the full + /// reasoning. + fn user_byte_count_strategy() -> impl proptest::strategy::Strategy { + use proptest::prelude::*; + prop_oneof![ + any::(), + (0u32..=3).prop_flat_map(|exponent| { + let low = if exponent == 0 { + 0 + } else { + 1024u64.pow(exponent) + }; + low..1024u64.pow(exponent + 1) + }), + (2u32..=3).prop_flat_map(|exponent| { + let boundary = 1024u64.pow(exponent); + (boundary - boundary / 16384)..=(boundary + 1) + }), + ] + } + + proptest::proptest! { + /// A size is rendered in the unit it belongs to, which has two edges. + /// + /// Upper: below the top unit, the printed mantissa is under 1024.0 — + /// otherwise the size is shown in a unit it has outgrown. + /// + /// Lower: above `bytes`, the printed mantissa is at least 1.0, and the + /// next smaller unit would have printed 1024.0 or more — otherwise the + /// size was promoted before it reached a whole unit. + /// + /// Both edges compare the mantissa as printed, in tenths: that is the + /// quantity a reader sees, so it is the one the scaling has to decide on. + #[test] + fn format_bytes_for_user_renders_a_size_in_its_own_unit( + bytes in user_byte_count_strategy(), + ) { + let rendered = format_bytes_for_user(bytes); + let (value, unit) = rendered + .split_once(' ') + .expect("rendered size is ` `"); + let value: f64 = value.parse().expect("numeric part parses"); + let tenths = (value * 10.0).round(); + let exponent = USER_BYTE_UNITS + .iter() + .position(|name| *name == unit) + .expect("rendered unit is one of the known units"); + if exponent + 1 < USER_BYTE_UNITS.len() { + proptest::prop_assert!( + tenths < 10_240.0, + "{bytes} rendered as {rendered}, which should have been \ + promoted to the next unit", + ); + } + if exponent > 0 { + proptest::prop_assert!( + tenths >= 10.0, + "{bytes} rendered as {rendered}, which was promoted before \ + it reached a whole unit", + ); + // Dividing by a power of two is exact, so this is the value the + // smaller unit would have printed, not an approximation of it. + let smaller = (1..exponent).fold(bytes as f64, |value, _| value / 1024.0); + proptest::prop_assert!( + (smaller * 10.0).round() >= 10_240.0, + "{bytes} rendered as {rendered}, but still fits the smaller \ + unit as {smaller:.1} {}", + USER_BYTE_UNITS[exponent - 1], + ); + } + } + } + #[test] fn setup_reset_requires_approval() { let action = @@ -36790,7 +36895,10 @@ ID_LIKE="suse opensuse" arguments: serde_json::json!({ "artifact_ref": "tiny/model#gguf", "allow_artifact_download": true, - "artifact_max_bytes": 1_048_576 + // One byte under 1 MiB: `{:.1}` rounds it up to a full + // 1024 KB, so the approved cap must read as "1.0 MB", not + // "1024.0 KB". + "artifact_max_bytes": 1_048_575 }), reviewed_at_unix_ms: None, }, diff --git a/crates/rocm-dash-tui/Cargo.toml b/crates/rocm-dash-tui/Cargo.toml index 569a0bdcc..a01eb88d3 100644 --- a/crates/rocm-dash-tui/Cargo.toml +++ b/crates/rocm-dash-tui/Cargo.toml @@ -57,3 +57,12 @@ reqwest = { version = "0.13", default-features = false, features = ["json", "rus # dependency — the production signal path goes through `tokio::signal`. [target.'cfg(unix)'.dev-dependencies] libc.workspace = true + +[dev-dependencies] +# Property-based tests for the MiB/GiB/TiB formatters in `ui::format`. "A size +# is printed in its own unit" has to hold at every unit boundary, and the narrow +# band where `{:.1}` rounding reaches 1024.0 is exactly the input a hand-picked +# example misses. Same version and features as rocm-core's, so no new crate or +# feature enters the lockfile. Test-only, so it adds nothing to any shipped +# binary. +proptest = { version = "1", default-features = false, features = ["std"] } diff --git a/crates/rocm-dash-tui/proptest-regressions/ui/format.txt b/crates/rocm-dash-tui/proptest-regressions/ui/format.txt new file mode 100644 index 000000000..5a57d5fc3 --- /dev/null +++ b/crates/rocm-dash-tui/proptest-regressions/ui/format.txt @@ -0,0 +1,8 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc b601b47bd4715f8484144d384962e1c755af100ba78b64c039143a1031b66619 # shrinks to value = 1048525 +cc 83d6dea8ddced7d1b7df3d6eb575ce828033551d8f540667d632c4c89e8937e5 # shrinks to total = 1048525, used = 0 diff --git a/crates/rocm-dash-tui/src/ui/format.rs b/crates/rocm-dash-tui/src/ui/format.rs index caec9d688..0a47b6cf0 100644 --- a/crates/rocm-dash-tui/src/ui/format.rs +++ b/crates/rocm-dash-tui/src/ui/format.rs @@ -44,37 +44,50 @@ pub fn display_or_placeholder(v: &str, placeholder: &'static str) -> String { } } +/// The unit a mebibyte count is shown in once it reaches 1024 MiB, as the +/// divisor that converts MiB into it and its label. `None` below 1024 MiB, +/// which is printed as a whole number of MiB. +/// +/// Promotes while the value AS PRINTED would reach 1024, not merely while the +/// raw value does: 1_048_575 MiB is 1023.999… GiB, which `{:.1}` renders as +/// "1024.0 GiB". Comparing the rounded tenths, as `rocm_core::format_bytes` +/// does, keeps every size in the unit it belongs to. +fn promoted_mib_unit(value: u64) -> Option<(f64, &'static str)> { + const UNITS: [&str; 2] = ["GiB", "TiB"]; + if value < 1024 { + return None; + } + let mut divisor = 1024.0; + let mut unit = 0; + while unit + 1 < UNITS.len() && (value as f64 / divisor * 10.0).round() >= 10_240.0 { + divisor *= 1024.0; + unit += 1; + } + Some((divisor, UNITS[unit])) +} + /// Format a byte count that's already in mebibytes (e.g. amd-smi `vram_used_mb`). -/// Promotes to GiB at 1024, TiB at 1024², with one decimal. +/// +/// Promotes to GiB at 1024, TiB at 1024², with one decimal — and as soon as the +/// printed value would read 1024.0, so it never shows "1024.0 GiB". pub fn mib(value: u64) -> String { - if value >= 1024 * 1024 { - format!("{:.1} TiB", value as f64 / (1024.0 * 1024.0)) - } else if value >= 1024 { - format!("{:.1} GiB", value as f64 / 1024.0) - } else { - format!("{value} MiB") + match promoted_mib_unit(value) { + Some((divisor, unit)) => format!("{:.1} {unit}", value as f64 / divisor), + None => format!("{value} MiB"), } } /// Pair of (used_mib, total_mib) → "used / total" with promotion. Both promoted -/// to the same unit (driven by total) so they compare visually. +/// to the same unit (driven by total, exactly as [`mib`] would pick it) so they +/// compare visually. pub fn mib_pair(used: u64, total: u64) -> String { - if total >= 1024 * 1024 { - let scale = 1024.0 * 1024.0; - format!( - "{:.1} / {:.1} TiB", - used as f64 / scale, - total as f64 / scale - ) - } else if total >= 1024 { - let scale = 1024.0; - format!( - "{:.1} / {:.1} GiB", - used as f64 / scale, - total as f64 / scale - ) - } else { - format!("{used} / {total} MiB") + match promoted_mib_unit(total) { + Some((divisor, unit)) => format!( + "{:.1} / {:.1} {unit}", + used as f64 / divisor, + total as f64 / divisor + ), + None => format!("{used} / {total} MiB"), } } @@ -352,6 +365,133 @@ mod tests { assert_eq!(mib_pair(1024, 1024 * 1024), "0.0 / 1.0 TiB"); } + /// Just below 1 TiB the value rounds up to a full 1024 GiB, which has to be + /// reported as 1.0 TiB — the same defect `rocm_core::format_bytes` had. + /// 1_048_524 MiB is the last input that still belongs to GiB; 1_048_525 is + /// the first that `{:.1}` rounds up to 1024.0 of it and used to print + /// "1024.0 GiB". + #[test] + fn mib_promotes_a_value_that_rounds_up_to_a_full_unit() { + assert_eq!(mib(1023), "1023 MiB"); + assert_eq!(mib(1_048_524), "1023.9 GiB"); + assert_eq!(mib(1_048_525), "1.0 TiB"); + assert_eq!(mib(1_048_575), "1.0 TiB"); + } + + /// `mib_pair` picks the unit from the total, so the total is the value that + /// must not print as "1024.0 GiB". + #[test] + fn mib_pair_promotes_a_total_that_rounds_up_to_a_full_unit() { + assert_eq!(mib_pair(512, 1_048_524), "0.5 / 1023.9 GiB"); + assert_eq!(mib_pair(0, 1_048_525), "0.0 / 1.0 TiB"); + assert_eq!(mib_pair(1_048_575, 1_048_575), "1.0 / 1.0 TiB"); + } + + // ── Properties ───────────────────────────────────────────────── + + /// The units `mib` and `mib_pair` print, smallest first. + const MIB_UNITS: [&str; 3] = ["MiB", "GiB", "TiB"]; + + /// Check that `value`, printed as `printed` in `unit`, is in the unit it + /// belongs to. Two edges, both on the mantissa as printed, in tenths: + /// + /// Upper: below the top unit, the mantissa is under 1024.0 — otherwise the + /// size is shown in a unit it has outgrown. + /// + /// Lower: above `MiB`, the mantissa is at least 1.0, and the next smaller + /// unit would have printed 1024.0 or more — otherwise the size was + /// promoted before it reached a whole unit. + fn own_unit_violation(value_mib: u64, printed: f64, unit: &str) -> Option { + let tenths = (printed * 10.0).round(); + let exponent = MIB_UNITS + .iter() + .position(|name| *name == unit) + .expect("rendered unit is one of the known units"); + if exponent + 1 < MIB_UNITS.len() && tenths >= 10_240.0 { + return Some(format!( + "{value_mib} MiB printed as {printed:.1} {unit}, which should have \ + been promoted to the next unit" + )); + } + if exponent > 0 { + if tenths < 10.0 { + return Some(format!( + "{value_mib} MiB printed as {printed:.1} {unit}, which was \ + promoted before it reached a whole unit" + )); + } + // Dividing by a power of two is exact, so this is the value the + // smaller unit would have printed, not an approximation of it. + let smaller = (1..exponent).fold(value_mib as f64, |value, _| value / 1024.0); + if (smaller * 10.0).round() < 10_240.0 { + return Some(format!( + "{value_mib} MiB printed as {printed:.1} {unit}, but still fits \ + the smaller unit as {smaller:.1} {}", + MIB_UNITS[exponent - 1] + )); + } + } + None + } + + /// MiB counts that actually visit the unit boundaries. A uniform `u64` + /// almost always lands far above the top unit, so on its own it never + /// samples the band where `{:.1}` rounding reaches 1024.0. The other arms + /// draw uniformly within one unit's range, and from a window just below the + /// GiB→TiB boundary that scales with it, as the band does — see + /// `rocm_core::disk_space`'s generator for the full reasoning. The + /// MiB→GiB boundary has no band: MiB prints a whole number. + fn mib_count_strategy() -> impl proptest::strategy::Strategy { + use proptest::prelude::*; + const TIB_BOUNDARY: u64 = 1024 * 1024; + prop_oneof![ + any::(), + (0u32..=2).prop_flat_map(|exponent| { + let low = if exponent == 0 { + 0 + } else { + 1024u64.pow(exponent) + }; + low..1024u64.pow(exponent + 1) + }), + (TIB_BOUNDARY - TIB_BOUNDARY / 16384)..=(TIB_BOUNDARY + 1), + ] + } + + /// Split `" "` into its parts. + fn split_rendered(rendered: &str) -> (f64, &str) { + let (value, unit) = rendered + .split_once(' ') + .expect("rendered size is ` `"); + (value.parse().expect("numeric part parses"), unit) + } + + proptest::proptest! { + #[test] + fn mib_renders_a_size_in_its_own_unit(value in mib_count_strategy()) { + let rendered = mib(value); + let (printed, unit) = split_rendered(&rendered); + let violation = own_unit_violation(value, printed, unit); + proptest::prop_assert!(violation.is_none(), "{}", violation.unwrap_or_default()); + } + + /// The pair takes its unit from the total, so the total obeys the same + /// contract as `mib` does on its own; the used half just shares it. + #[test] + fn mib_pair_renders_the_total_in_its_own_unit( + total in mib_count_strategy(), + used in mib_count_strategy(), + ) { + let rendered = mib_pair(used, total); + let (_used, total_part) = rendered + .split_once(" / ") + .expect("rendered pair is ` / `"); + let (printed, unit) = split_rendered(total_part); + let violation = own_unit_violation(total, printed, unit); + proptest::prop_assert!(violation.is_none(), "{}", violation.unwrap_or_default()); + } + } + #[test] fn pct_uses_two_decimals_for_tiny_values() { assert_eq!(pct(0.0), "0.0%");