From 32633778e78406be186fe99a3b3ef74ef7d2d34d Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 7 Sep 2026 09:05:18 +0000 Subject: [PATCH 1/9] feat(rocm): show live progress for multi-gigabyte downloads rocm_core::download_file_streaming gains a progress-callback sibling reporting cumulative bytes (and total, when known) after every chunk, resume-aware so a retried attempt reports its true starting offset instead of 0. Extract the serve command's TTY-gated stderr spinner into a shared apps/rocm::cli_progress module and give it a throttled, byte-aware repaint mode with a high-water-mark clamp so a retry never visibly regresses the displayed progress. Wire the SDK tarball install path (therock.rs) and the ComfyUI source archive download (comfyui.rs) to drive that spinner, so an interactive terminal shows a live "X / Y (Z%)" status line while non-interactive output is unaffected. Signed-off-by: Jussi Elo --- apps/rocm/src/cli_progress.rs | 175 +++++++++++++++++++++++++++++++++ apps/rocm/src/comfyui.rs | 98 ++++++++++++++++-- apps/rocm/src/main.rs | 3 +- apps/rocm/src/serve_summary.rs | 63 ------------ apps/rocm/src/therock.rs | 109 ++++++++++++++++++-- crates/rocm-core/src/lib.rs | 130 +++++++++++++++++++++++- 6 files changed, 497 insertions(+), 81 deletions(-) create mode 100644 apps/rocm/src/cli_progress.rs diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs new file mode 100644 index 000000000..48d7b1838 --- /dev/null +++ b/apps/rocm/src/cli_progress.rs @@ -0,0 +1,175 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Shared TTY-gated status indicator for long-running CLI operations +//! (starting a server, downloading a large artifact). Written to stderr only, +//! so piped/redirected output — and stdout, which callers may still be +//! printing a final summary to — never sees control characters. + +use std::io::{IsTerminal, Write}; +use std::time::{Duration, Instant}; + +use crossterm::QueueableCommand; +use crossterm::cursor::MoveToColumn; +use crossterm::terminal::{Clear, ClearType}; + +/// Braille spinner frames (matching the dashboard's visual language). +const SPINNER_FRAMES: &[&str] = &["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; + +/// A byte-progress repaint fires at most this often. A 64 KiB read cadence on +/// a fast local link would otherwise flood the terminal with far more +/// repaints per second than a human can perceive. +const MIN_PROGRESS_REPAINT_INTERVAL: Duration = Duration::from_millis(100); + +/// A carriage-return status indicator written to stderr. Disabled (a no-op) when +/// stderr is not a TTY, so piped/redirected output never receives control +/// characters. Keeps stdout clean for whatever the caller prints afterward. +pub(crate) struct Spinner { + enabled: bool, + idx: usize, + label: String, + active: bool, + last_progress_paint: Option, + max_progress_bytes: u64, +} + +impl Spinner { + pub(crate) fn new(label: impl Into) -> Self { + Self { + enabled: std::io::stderr().is_terminal(), + idx: 0, + label: label.into(), + active: false, + last_progress_paint: None, + max_progress_bytes: 0, + } + } + + /// Change the message shown next to the spinner (e.g. "Running smoke test…"). + pub(crate) fn set_label(&mut self, label: impl Into) { + self.label = label.into(); + self.render_current(); + } + + /// Advance to the next animation frame and repaint. + pub(crate) fn tick(&mut self) { + self.idx = self.idx.wrapping_add(1); + self.render_current(); + } + + /// Repaint with a byte-progress label. Throttled to at most one repaint + /// per [`MIN_PROGRESS_REPAINT_INTERVAL`], except the final chunk (`bytes + /// >= total`) always repaints, so the last frame shown is never stale. + /// + /// `bytes` is clamped to a high-water mark: a retried transfer that + /// restarts from zero (or resumes from an earlier offset than what was + /// already shown) never visibly regresses the displayed count. + pub(crate) fn set_progress(&mut self, prefix: &str, bytes: u64, total: Option) { + let bytes = bytes.max(self.max_progress_bytes); + self.max_progress_bytes = bytes; + let is_final = total.is_some_and(|total| bytes >= total); + let now = Instant::now(); + if !is_final + && let Some(last) = self.last_progress_paint + && now.duration_since(last) < MIN_PROGRESS_REPAINT_INTERVAL + { + return; + } + self.last_progress_paint = Some(now); + self.idx = self.idx.wrapping_add(1); + self.label = format_download_progress(prefix, bytes, total); + self.render_current(); + } + + fn render_current(&mut self) { + if !self.enabled { + return; + } + let frame = SPINNER_FRAMES[self.idx % SPINNER_FRAMES.len()]; + let mut err = std::io::stderr(); + let _ = err.queue(MoveToColumn(0)); + let _ = err.queue(Clear(ClearType::CurrentLine)); + let _ = write!(err, "{frame} {}", self.label); + let _ = err.flush(); + self.active = true; + } + + /// Erase the spinner line so whatever prints next starts on a clean line. + pub(crate) fn clear(&mut self) { + if self.enabled && self.active { + let mut err = std::io::stderr(); + let _ = err.queue(MoveToColumn(0)); + let _ = err.queue(Clear(ClearType::CurrentLine)); + let _ = err.flush(); + self.active = false; + } + } +} + +/// e.g. `"Downloading SDK tarball… 842.1 MiB / 3.2 GiB (26%)"`, or +/// `"Downloading SDK tarball… 842.1 MiB"` when the total is unknown (the +/// server never reported a `Content-Length`). +pub(crate) fn format_download_progress(prefix: &str, bytes: u64, total: Option) -> String { + match total { + Some(total) if total > 0 => { + let pct = ((bytes.min(total) as f64 / total as f64) * 100.0).round() as u64; + format!( + "{prefix} {} / {} ({pct}%)", + rocm_core::format_bytes(bytes), + rocm_core::format_bytes(total) + ) + } + _ => format!("{prefix} {}", rocm_core::format_bytes(bytes)), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn format_download_progress_shows_bytes_and_percent_when_total_is_known() { + let gib = 1024 * 1024 * 1024; + assert_eq!( + format_download_progress("Downloading…", gib, Some(4 * gib)), + "Downloading… 1.0 GiB / 4.0 GiB (25%)" + ); + } + + #[test] + fn format_download_progress_omits_total_when_unknown() { + let rendered = format_download_progress("Downloading…", 883_147_264, None); + assert!( + !rendered.contains('/') && !rendered.contains('%'), + "no total means no fraction or percentage: {rendered}" + ); + assert!(rendered.starts_with("Downloading… ")); + } + + #[test] + fn format_download_progress_clamps_percent_at_100_when_bytes_exceeds_total() { + let rendered = format_download_progress("Downloading…", 105, Some(100)); + assert!( + rendered.contains("(100%)"), + "a server sending a few bytes past its declared length must not report over 100%: {rendered}" + ); + } + + #[test] + fn set_progress_never_displays_fewer_bytes_than_already_shown() { + let mut spinner = Spinner::new("Downloading…"); + spinner.set_progress("Downloading…", 900, Some(1000)); + assert!(spinner.label.contains("900")); + // A retried transfer restarts its own byte count from a lower offset. + // Force this repaint past the throttle (via a small `total` that the + // clamped byte count already exceeds) to prove the clamp itself, not + // just that the repaint was skipped. + spinner.set_progress("Downloading…", 100, Some(500)); + assert!( + spinner.label.contains("900"), + "progress must not regress after a retry: {}", + spinner.label + ); + } +} diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 5ae412265..7acce1cf2 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -2,14 +2,15 @@ // // SPDX-License-Identifier: MIT +use crate::cli_progress::Spinner; use crate::{format_structured_tool_call, runtime_usability_status, therock}; use anyhow::{Context, Result, bail}; use flate2::read::GzDecoder; use rocm_core::{ - AppPaths, RocmCliConfig, download_file_to_path, ensure_uv_binary, format_http_base_url, - runtime_is_linux, runtime_is_windows, runtime_path_for_windows_child, runtime_path_list_join, - runtime_path_list_split, runtime_paths_equivalent, unix_time_millis, uv_command_env, - uv_pip_install_base, + AppPaths, RocmCliConfig, download_file_to_path_with_progress, ensure_uv_binary, + format_http_base_url, runtime_is_linux, runtime_is_windows, runtime_path_for_windows_child, + runtime_path_list_join, runtime_path_list_split, runtime_paths_equivalent, unix_time_millis, + uv_command_env, uv_pip_install_base, }; use serde::{Deserialize, Serialize}; use std::ffi::OsString; @@ -1296,7 +1297,17 @@ fn download_and_extract_source( )?; } else { writeln!(log, "Downloading {COMFYUI_SOURCE_ARCHIVE_URL}.")?; - download_file(COMFYUI_SOURCE_ARCHIVE_URL, &archive_path)?; + let mut spinner = Spinner::new("Fetching ComfyUI source archive…"); + spinner.tick(); + let download_result = download_file( + COMFYUI_SOURCE_ARCHIVE_URL, + &archive_path, + &mut |bytes, total| { + spinner.set_progress("Fetching ComfyUI source archive…", bytes, total); + }, + ); + spinner.clear(); + download_result?; } let extract_root = app_root .join("extract") @@ -1357,8 +1368,12 @@ fn copy_dir_all(from: &Path, to: &Path) -> Result<()> { Ok(()) } -fn download_file(url: &str, destination: &Path) -> Result<()> { - download_file_to_path(url, destination, Duration::from_mins(2)) +fn download_file( + url: &str, + destination: &Path, + on_progress: &mut dyn FnMut(u64, Option), +) -> Result<()> { + download_file_to_path_with_progress(url, destination, Duration::from_mins(2), on_progress) } fn filtered_requirement_specs(requirements_path: &Path) -> Result> { @@ -2306,4 +2321,73 @@ mod tests { cache_dir: root.join("cache"), } } + + #[test] + fn download_file_reports_cumulative_progress_to_its_caller() -> Result<()> { + // A multi-chunk body (the streaming downloader reads in 64 KiB + // chunks) so a single callback firing wouldn't already satisfy the + // "monotonically increasing" assertion below. + let body: Vec = (0..200_000_u32).map(|i| (i % 256) as u8).collect(); + + let listener = TcpListener::bind("127.0.0.1:0")?; + let port = listener.local_addr()?.port(); + let served = body.clone(); + let server = thread::spawn(move || -> Result<()> { + let (mut stream, _) = listener.accept()?; + stream.set_read_timeout(Some(Duration::from_secs(5))).ok(); + let mut request = Vec::new(); + let mut buffer = [0_u8; 1024]; + loop { + let read = stream.read(&mut buffer)?; + if read == 0 { + break; + } + request.extend_from_slice(&buffer[..read]); + if request.windows(4).any(|window| window == b"\r\n\r\n") { + break; + } + } + write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Type: application/octet-stream\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + served.len() + )?; + stream.write_all(&served)?; + stream.flush()?; + Ok(()) + }); + + let url = format!("http://127.0.0.1:{port}/archive.tar.gz"); + let root = std::env::temp_dir().join(format!( + "rocm-cli-comfyui-download-progress-{}", + unix_time_millis() + )); + fs::create_dir_all(&root)?; + let destination = root.join("archive.tar.gz"); + + let mut calls: Vec<(u64, Option)> = Vec::new(); + download_file(&url, &destination, &mut |bytes, total| { + calls.push((bytes, total)); + })?; + assert_eq!(fs::read(&destination)?, body); + + server.join().expect("localhost server thread panicked")?; + let _ = fs::remove_dir_all(&root); + + let total = Some(body.len() as u64); + assert!( + calls.len() >= 2, + "expected at least a pre-transfer and a final callback: {calls:?}" + ); + assert!( + calls.windows(2).all(|pair| pair[0].0 <= pair[1].0), + "byte counts must never regress: {calls:?}" + ); + assert_eq!( + calls.last(), + Some(&(body.len() as u64, total)), + "the last callback must report the complete transfer: {calls:?}" + ); + Ok(()) + } } diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 75a75ffc3..35d2d7e0e 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -4,6 +4,7 @@ mod automations; mod bootstrap; +mod cli_progress; mod comfyui; mod dash; mod dash_seam; @@ -5150,7 +5151,7 @@ fn serve(args: ServeArgs) -> Result<()> { if background { let mut spinner = - serve_summary::Spinner::new(format!("Starting {model} on {selected_engine}…")); + cli_progress::Spinner::new(format!("Starting {model} on {selected_engine}…")); spinner.tick(); let report = start_managed_service( &selected_engine, diff --git a/apps/rocm/src/serve_summary.rs b/apps/rocm/src/serve_summary.rs index f12e8279f..c4bfa0213 100644 --- a/apps/rocm/src/serve_summary.rs +++ b/apps/rocm/src/serve_summary.rs @@ -18,12 +18,8 @@ //! exact, approximated, or `n/a`). use std::fmt::Write as _; -use std::io::{IsTerminal, Write}; use std::time::{Duration, Instant}; -use crossterm::QueueableCommand; -use crossterm::cursor::MoveToColumn; -use crossterm::terminal::{Clear, ClearType}; use rocm_core::AppPaths; use crate::providers::{self, ChatMessage, ChatRequest, ProviderStreamEvent}; @@ -33,8 +29,6 @@ use crate::providers::{self, ChatMessage, ChatRequest, ProviderStreamEvent}; const SMOKE_PROMPT: &str = "Reply with a short one-sentence greeting."; /// Cap the smoke-test generation so a slow or verbose model cannot stall startup. const SMOKE_MAX_TOKENS: u32 = 32; -/// Braille spinner frames (matching the dashboard's visual language). -const SPINNER_FRAMES: &[&str] = &["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; /// Best-effort metrics measured against a freshly-started server. Every field is /// optional: any probe failure leaves it `None` and the summary renders `n/a`. @@ -256,63 +250,6 @@ fn compute_metrics( SmokeMetrics { ttft, gen_tps } } -/// A carriage-return status indicator written to stderr. Disabled (a no-op) when -/// stderr is not a TTY, so piped/redirected output never receives control -/// characters. Keeps stdout clean for the summary table. -pub(crate) struct Spinner { - enabled: bool, - idx: usize, - label: String, - active: bool, -} - -impl Spinner { - pub(crate) fn new(label: impl Into) -> Self { - Self { - enabled: std::io::stderr().is_terminal(), - idx: 0, - label: label.into(), - active: false, - } - } - - /// Change the message shown next to the spinner (e.g. "Running smoke test…"). - pub(crate) fn set_label(&mut self, label: impl Into) { - self.label = label.into(); - self.render_current(); - } - - /// Advance to the next animation frame and repaint. - pub(crate) fn tick(&mut self) { - self.idx = self.idx.wrapping_add(1); - self.render_current(); - } - - fn render_current(&mut self) { - if !self.enabled { - return; - } - let frame = SPINNER_FRAMES[self.idx % SPINNER_FRAMES.len()]; - let mut err = std::io::stderr(); - let _ = err.queue(MoveToColumn(0)); - let _ = err.queue(Clear(ClearType::CurrentLine)); - let _ = write!(err, "{frame} {}", self.label); - let _ = err.flush(); - self.active = true; - } - - /// Erase the spinner line so the summary table starts on a clean line. - pub(crate) fn clear(&mut self) { - if self.enabled && self.active { - let mut err = std::io::stderr(); - let _ = err.queue(MoveToColumn(0)); - let _ = err.queue(Clear(ClearType::CurrentLine)); - let _ = err.flush(); - self.active = false; - } - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 6d34ba253..6df94728a 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -1095,7 +1095,14 @@ fn install_tarball_runtime( let _ = writeln!(output, " {warning}"); } - download_file(&artifact.url, &cache_path)?; + let download_label = format!("Downloading {}…", artifact.file_name); + let mut spinner = crate::cli_progress::Spinner::new(download_label.clone()); + spinner.tick(); + let download_result = download_file(&artifact.url, &cache_path, &mut |bytes, total| { + spinner.set_progress(&download_label, bytes, total); + }); + spinner.clear(); + download_result?; extract_tarball_and_discard_archive(&cache_path, &install_root)?; let manifest = InstalledRuntimeManifest { @@ -2157,23 +2164,27 @@ fn http_header_value(headers: &str, name: &str) -> Option { value } -/// Fetch an artifact to `destination`. +/// Fetch an artifact to `destination`, reporting cumulative bytes written and +/// (when known) the total size to `on_progress` as the transfer proceeds. /// /// Streams rather than buffers: SDK tarballs are single-digit gigabytes, and /// holding one in memory to write it out again costs that much RAM on top of /// the same amount of disk. The primitive also handles the free-space /// preflight, retry with resume, and the length cross-check that catches a /// transfer the server ended early. -fn download_file(url: &str, destination: &Path) -> Result<()> { +fn download_file( + url: &str, + destination: &Path, + on_progress: &mut dyn FnMut(u64, Option), +) -> Result<()> { let parent = destination .parent() .context("download destination has no parent directory")?; fs::create_dir_all(parent)?; - rocm_core::download_file_streaming(&rocm_core::DownloadRequest::new( - url, - destination, - THEROCK_DOWNLOAD_TIMEOUT, - )) + rocm_core::download_file_streaming_with_progress( + &rocm_core::DownloadRequest::new(url, destination, THEROCK_DOWNLOAD_TIMEOUT), + on_progress, + ) .with_context(|| format!("failed to fetch {url}"))?; Ok(()) } @@ -5281,7 +5292,7 @@ echo Python 3.12.10 fs::create_dir_all(&temp)?; let destination = temp.join("artifact.bin"); - download_file(&url, &destination)?; + download_file(&url, &destination, &mut |_, _| {})?; assert_eq!(fs::read(&destination)?, body); let response = http_get(&url, &[], Some(5))?; @@ -5293,6 +5304,86 @@ echo Python 3.12.10 Ok(()) } + #[test] + fn download_file_reports_cumulative_progress_to_its_caller() -> Result<()> { + use std::net::TcpListener; + use std::thread; + + // A multi-chunk body (`download_file_streaming` reads in 64 KiB + // chunks) so a single callback firing wouldn't already satisfy the + // "monotonically increasing" assertion below. + let body: Vec = (0..200_000_u32).map(|i| (i % 256) as u8).collect(); + + let listener = TcpListener::bind("127.0.0.1:0")?; + let port = listener.local_addr()?.port(); + let served = body.clone(); + let server = thread::spawn(move || -> Result<()> { + let (mut stream, _) = listener.accept()?; + stream.set_read_timeout(Some(Duration::from_secs(5))).ok(); + let mut request = Vec::new(); + let mut buffer = [0_u8; 1024]; + loop { + let read = stream.read(&mut buffer)?; + if read == 0 { + break; + } + request.extend_from_slice(&buffer[..read]); + if request.windows(4).any(|window| window == b"\r\n\r\n") { + break; + } + } + write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Type: application/octet-stream\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + served.len() + )?; + stream.write_all(&served)?; + stream.flush()?; + Ok(()) + }); + + let url = format!("http://127.0.0.1:{port}/artifact.bin"); + + let temp = workspace_test_artifact_dir().join(format!( + "download-progress-{}-{}", + std::process::id(), + unix_time_millis() + )); + fs::create_dir_all(&temp)?; + let destination = temp.join("artifact.bin"); + + let mut calls: Vec<(u64, Option)> = Vec::new(); + download_file(&url, &destination, &mut |bytes, total| { + calls.push((bytes, total)); + })?; + assert_eq!(fs::read(&destination)?, body); + + server.join().expect("localhost server thread panicked")?; + let _ = fs::remove_dir_all(&temp); + + let total = Some(body.len() as u64); + assert!( + calls.len() >= 2, + "expected at least a pre-transfer and a final callback: {calls:?}" + ); + assert!( + calls.windows(2).all(|pair| pair[0].0 <= pair[1].0), + "byte counts must never regress: {calls:?}" + ); + assert_eq!( + calls.last(), + Some(&(body.len() as u64, total)), + "the last callback must report the complete transfer: {calls:?}" + ); + assert!( + calls + .iter() + .all(|&(_, reported_total)| reported_total == total), + "the reported total must stay consistent across callbacks: {calls:?}" + ); + Ok(()) + } + #[test] fn update_report_policy_mentions_bounded_startup_check() -> Result<()> { let (root, paths) = test_paths("update-report-policy"); diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index ec50c0724..21ace86d5 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -242,6 +242,20 @@ pub struct DownloadOutcome { /// matching length proves nothing about the bytes — do not read a successful /// return as "the artifact is genuine" unless a digest was supplied. pub fn download_file_streaming(request: &DownloadRequest<'_>) -> Result { + download_file_streaming_with_progress(request, &mut |_written, _total| {}) +} + +/// As [`download_file_streaming`], but reports progress via `on_progress`. +/// +/// `on_progress` is called with the cumulative bytes written and, when +/// known, the total size — once before the transfer starts (already +/// resume-aware, so a resumed attempt reports its true starting offset +/// rather than 0) and once after every chunk is written to disk. Callers +/// that don't need progress should use [`download_file_streaming`] instead. +pub fn download_file_streaming_with_progress( + request: &DownloadRequest<'_>, + on_progress: &mut dyn FnMut(u64, Option), +) -> Result { if let Some(parent) = request.destination.parent() && !parent.as_os_str().is_empty() { @@ -257,7 +271,7 @@ pub fn download_file_streaming(request: &DownloadRequest<'_>) -> Result break outcome, Err(error) => { let retryable = error.retryable && attempt < DOWNLOAD_MAX_ATTEMPTS; @@ -318,6 +332,7 @@ const fn status_is_retryable(status: u16) -> bool { fn download_attempt( request: &DownloadRequest<'_>, partial_path: &Path, + on_progress: &mut dyn FnMut(u64, Option), ) -> Result { // Resume from whatever a previous attempt already wrote. A missing file is // simply a fresh start. @@ -417,6 +432,8 @@ fn download_attempt( })? }; + on_progress(written, total_len); + let mut reader = response.into_reader(); let mut buffer = vec![0_u8; DOWNLOAD_CHUNK_BYTES]; loop { @@ -454,6 +471,7 @@ fn download_attempt( if let Err(error) = file.write_all(&buffer[..read]) { return Err(permanent(disk_space::map_write_error(error, partial_path))); } + on_progress(written, total_len); } if let Err(error) = file.sync_all() { return Err(permanent(disk_space::map_write_error(error, partial_path))); @@ -522,6 +540,21 @@ pub fn download_file_to_path(url: &str, destination: &Path, timeout: Duration) - Ok(()) } +/// As [`download_file_to_path`], but reports progress via `on_progress`. See +/// [`download_file_streaming_with_progress`] for callback semantics. +pub fn download_file_to_path_with_progress( + url: &str, + destination: &Path, + timeout: Duration, + on_progress: &mut dyn FnMut(u64, Option), +) -> Result<()> { + download_file_streaming_with_progress( + &DownloadRequest::new(url, destination, timeout), + on_progress, + )?; + Ok(()) +} + pub fn http_get_text(endpoint_url: &str, path: &str, timeout: Duration) -> Result { http_get_text_with_auth(endpoint_url, path, None, timeout) } @@ -8032,6 +8065,101 @@ mod tests { Ok(()) } + #[test] + fn download_with_progress_reports_cumulative_bytes_across_chunks() -> Result<()> { + let body = download_body(); + let (port, server) = spawn_download_server(body.clone(), vec![DownloadReply::Complete])?; + let dir = download_scratch("progress"); + let destination = dir.join("artifact.bin"); + + let mut calls: Vec<(u64, Option)> = Vec::new(); + let outcome = download_file_streaming_with_progress( + &DownloadRequest::new( + &format!("http://127.0.0.1:{port}/artifact.bin"), + &destination, + Duration::from_secs(10), + ), + &mut |written, total| calls.push((written, total)), + )?; + + server.join().expect("server thread")?; + fs::remove_dir_all(&dir).ok(); + + let total = Some(body.len() as u64); + assert_eq!( + calls.first(), + Some(&(0, total)), + "the first call must fire before any bytes are read, already knowing the total: {calls:?}" + ); + assert_eq!( + calls.last(), + Some(&(body.len() as u64, total)), + "the last call must report the complete byte count: {calls:?}" + ); + assert!( + calls.windows(2).all(|pair| pair[0].0 <= pair[1].0), + "cumulative bytes must never go backwards: {calls:?}" + ); + assert!( + calls + .iter() + .all(|&(_, reported_total)| reported_total == total), + "the total must stay constant across an attempt: {calls:?}" + ); + assert_eq!(outcome.bytes_written, body.len() as u64); + Ok(()) + } + + #[test] + fn download_with_progress_stays_continuous_across_a_resumed_attempt() -> Result<()> { + let body = download_body(); + let (port, server) = spawn_download_server( + body.clone(), + vec![ + DownloadReply::Truncated { sent: 5000 }, + DownloadReply::Resume, + ], + )?; + let dir = download_scratch("progress-resume"); + let destination = dir.join("artifact.bin"); + + let mut calls: Vec<(u64, Option)> = Vec::new(); + let outcome = download_file_streaming_with_progress( + &DownloadRequest::new( + &format!("http://127.0.0.1:{port}/artifact.bin"), + &destination, + Duration::from_secs(10), + ), + &mut |written, total| calls.push((written, total)), + )?; + + server.join().expect("server thread")?; + fs::remove_dir_all(&dir).ok(); + + let total = Some(body.len() as u64); + assert!( + calls.windows(2).all(|pair| pair[0].0 <= pair[1].0), + "byte counts must never regress across a retried attempt, e.g. reset to 0: {calls:?}" + ); + assert!( + calls.iter().any(|&(written, _)| written == 5000), + "the resumed attempt must report the byte count already on disk before reading more: {calls:?}" + ); + assert!( + calls + .iter() + .all(|&(_, reported_total)| reported_total == total), + "the total size must stay stable across the retry: {calls:?}" + ); + assert_eq!( + calls.last(), + Some(&(body.len() as u64, total)), + "the last call must report the complete byte count: {calls:?}" + ); + assert_eq!(outcome.bytes_written, body.len() as u64); + Ok(()) + } + #[test] fn download_interrupted_beyond_recovery_leaves_no_destination_file() -> Result<()> { // Every attempt ends early, so the download never completes and the From cbb856ef583cb3c181fce3125dcbd62a600abefb Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 7 Sep 2026 09:27:38 +0000 Subject: [PATCH 2/9] fix(cli): address Copilot review findings on download progress - format_download_progress rounded 99.5% up to a misleading "100%" while bytes were still outstanding; floor instead, reserving 100% for bytes >= total. - download_attempt's on_progress callback reported None whenever the server omitted Content-Length, even when the caller had already supplied expected_len (the same value already used for preflight disk-space and size checks). Fall back to it so progress reporting doesn't lose a total that's already known. Signed-off-by: Jussi Elo --- apps/rocm/src/cli_progress.rs | 18 +++++++++++++++++- crates/rocm-core/src/lib.rs | 12 ++++++++---- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index 48d7b1838..13be206c6 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -113,7 +113,14 @@ impl Spinner { pub(crate) fn format_download_progress(prefix: &str, bytes: u64, total: Option) -> String { match total { Some(total) if total > 0 => { - let pct = ((bytes.min(total) as f64 / total as f64) * 100.0).round() as u64; + // Floor rather than round: a multi-gigabyte transfer sitting at + // 99.5% must not be shown as "complete" while bytes are still + // outstanding. 100% is reserved for `bytes >= total`. + let pct = if bytes >= total { + 100 + } else { + ((bytes as f64 / total as f64) * 100.0).floor() as u64 + }; format!( "{prefix} {} / {} ({pct}%)", rocm_core::format_bytes(bytes), @@ -156,6 +163,15 @@ mod tests { ); } + #[test] + fn format_download_progress_does_not_round_up_to_100_before_completion() { + let rendered = format_download_progress("Downloading…", 995, Some(1000)); + assert!( + rendered.contains("(99%)"), + "99.5% must floor to 99%, not round up to a premature 100%: {rendered}" + ); + } + #[test] fn set_progress_never_displays_fewer_bytes_than_already_shown() { let mut spinner = Spinner::new("Downloading…"); diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index 21ace86d5..c14721f1d 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -388,7 +388,11 @@ fn download_attempt( let remaining_len = header_u64(&response, "Content-Length"); let total_len = remaining_len.map(|len| len.saturating_add(if resuming { resume_from } else { 0 })); - if let Some(total) = total_len.or(request.expected_len) { + // Fall back to the caller-supplied expected length when the server omits + // `Content-Length`, so progress reporting doesn't lose a total that's + // already known and already used for the preflight checks below. + let reported_total = total_len.or(request.expected_len); + if let Some(total) = reported_total { if let Some(max_bytes) = request.max_bytes && total > max_bytes { @@ -432,7 +436,7 @@ fn download_attempt( })? }; - on_progress(written, total_len); + on_progress(written, reported_total); let mut reader = response.into_reader(); let mut buffer = vec![0_u8; DOWNLOAD_CHUNK_BYTES]; @@ -446,7 +450,7 @@ fn download_attempt( // the user, so report the shortfall either way rather than a bare // transport error. Err(error) => { - let reason = total_len.or(request.expected_len).map_or_else( + let reason = reported_total.map_or_else( || format!("failed while downloading {}", request.url), |expected| { format!( @@ -471,7 +475,7 @@ fn download_attempt( if let Err(error) = file.write_all(&buffer[..read]) { return Err(permanent(disk_space::map_write_error(error, partial_path))); } - on_progress(written, total_len); + on_progress(written, reported_total); } if let Err(error) = file.sync_all() { return Err(permanent(disk_space::map_write_error(error, partial_path))); From 35d2082b2f76d82262e337c7f70a7c5e00c1eac9 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 8 Sep 2026 06:17:29 +0000 Subject: [PATCH 3/9] fix(core): keep download progress monotonic across restarted attempts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit download_file_streaming_with_progress's doc comment claimed a resumed attempt always reports its true starting offset instead of 0, but that only holds for a confirmed 206 continuation. A restart that discards the partial file (the server ignored Range, or resumed at the wrong offset) truncates written back to 0 internally, and on_progress saw that raw value — a real regression the doc comment promised wouldn't happen. This was invisible in practice only because both current callers (therock.rs, comfyui.rs) apply their own high-water-mark clamp in Spinner. Any caller of the primitive without that clamp would show progress jumping backward. Wrap on_progress in download_file_streaming_with_progress with a high-water mark so the guarantee holds in rocm-core itself, for every caller, not just ones that add their own UI-side clamp. download_attempt keeps counting from 0 on a from-scratch restart internally; that's now explicitly documented as an implementation detail the wrapper absorbs. Adds a test exercising the restart-from-scratch path (a wrong-offset resume discarding the partial file) and asserting the callback sequence never decreases. Signed-off-by: Jussi Elo --- crates/rocm-core/src/lib.rs | 90 ++++++++++++++++++++++++++++++++++--- 1 file changed, 85 insertions(+), 5 deletions(-) diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index c14721f1d..b2bbf2b69 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -248,10 +248,15 @@ pub fn download_file_streaming(request: &DownloadRequest<'_>) -> Result, on_progress: &mut dyn FnMut(u64, Option), @@ -270,8 +275,18 @@ pub fn download_file_streaming_with_progress( let _ = fs::remove_file(&partial_path); let mut backoff = Backoff::default(); let mut attempt = 1; + // `download_attempt` reports whatever it has on disk for *this* attempt, + // which resets to 0 on a from-scratch restart even though earlier + // attempts already progressed further. Clamp to a high-water mark here + // so every caller — not just ones that happen to add their own UI-side + // clamp — sees a byte count that never goes backwards. + let mut high_water = 0_u64; + let mut monotonic_progress = move |written: u64, total: Option| { + high_water = high_water.max(written); + on_progress(high_water, total); + }; let outcome = loop { - match download_attempt(request, &partial_path, on_progress) { + match download_attempt(request, &partial_path, &mut monotonic_progress) { Ok(outcome) => break outcome, Err(error) => { let retryable = error.retryable && attempt < DOWNLOAD_MAX_ATTEMPTS; @@ -329,6 +344,15 @@ const fn status_is_retryable(status: u16) -> bool { status == 408 || status == 429 || status >= 500 } +/// A single attempt at the transfer. `written` — and so what this reports +/// through `on_progress` — reflects only what this attempt itself has put on +/// disk: a confirmed `206` continuation starts counting from the resumed +/// offset, but a restart (the server ignored `Range`, or resumed at the +/// wrong offset and had its partial file discarded) truncates the file and +/// starts counting from 0 again, even if a previous attempt already reported +/// further along. That's fine — [`download_file_streaming_with_progress`] +/// wraps `on_progress` with a high-water mark so callers never observe the +/// drop; this function does not need to care. fn download_attempt( request: &DownloadRequest<'_>, partial_path: &Path, @@ -8164,6 +8188,62 @@ mod tests { Ok(()) } + #[test] + fn download_with_progress_stays_monotonic_after_a_discarded_restart() -> Result<()> { + // The second reply resumes at the wrong offset, so its partial file is + // discarded and the third attempt restarts from scratch — internally + // reporting 0 bytes written again even though the first attempt had + // already reached 5000. The caller must never see that drop. + let body = download_body(); + let (port, server) = spawn_download_server( + body.clone(), + vec![ + DownloadReply::Truncated { sent: 5000 }, + DownloadReply::ResumeAtWrongOffset { start: 8000 }, + DownloadReply::Complete, + ], + )?; + let dir = download_scratch("progress-wrong-offset"); + let destination = dir.join("artifact.bin"); + + let mut calls: Vec<(u64, Option)> = Vec::new(); + let outcome = download_file_streaming_with_progress( + &DownloadRequest::new( + &format!("http://127.0.0.1:{port}/artifact.bin"), + &destination, + Duration::from_secs(10), + ), + &mut |written, total| calls.push((written, total)), + )?; + + let requests = server.join().expect("server thread")?; + fs::remove_dir_all(&dir).ok(); + + let total = Some(body.len() as u64); + assert_eq!( + requests.len(), + 3, + "the wrong-offset reply must be discarded and retried, not accepted" + ); + assert!( + calls.windows(2).all(|pair| pair[0].0 <= pair[1].0), + "cumulative bytes must never go backwards, even across a discarded \ + partial file and a from-scratch restart: {calls:?}" + ); + assert!( + calls.iter().any(|&(written, _)| written == 5000), + "the truncated first attempt's progress must not be lost once the \ + restart reports 0 internally: {calls:?}" + ); + assert_eq!( + calls.last(), + Some(&(body.len() as u64, total)), + "the last call must report the complete byte count: {calls:?}" + ); + assert_eq!(outcome.bytes_written, body.len() as u64); + Ok(()) + } + #[test] fn download_interrupted_beyond_recovery_leaves_no_destination_file() -> Result<()> { // Every attempt ends early, so the download never completes and the From 2ca7cff7944619d73d09194ad7c23240bef3904e Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 8 Sep 2026 06:17:38 +0000 Subject: [PATCH 4/9] fix(cli): tidy up progress-spinner doc comment and label reuse cli_progress.rs: Spinner::set_progress's doc comment described the final-chunk always-repaints exception but not that the very first call also always repaints, since last_progress_paint starts unset. Note both explicitly. comfyui.rs: bind the progress label to a variable once instead of repeating the literal, matching therock.rs's existing pattern. Signed-off-by: Jussi Elo --- apps/rocm/src/cli_progress.rs | 6 ++++-- apps/rocm/src/comfyui.rs | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index 13be206c6..e407a5096 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -59,8 +59,10 @@ impl Spinner { } /// Repaint with a byte-progress label. Throttled to at most one repaint - /// per [`MIN_PROGRESS_REPAINT_INTERVAL`], except the final chunk (`bytes - /// >= total`) always repaints, so the last frame shown is never stale. + /// per [`MIN_PROGRESS_REPAINT_INTERVAL`], except the very first call + /// (`last_progress_paint` starts unset) and the final chunk (`bytes >= + /// total`) always repaint, so the first and last frames shown are never + /// stale. /// /// `bytes` is clamped to a high-water mark: a retried transfer that /// restarts from zero (or resumes from an earlier offset than what was diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 7acce1cf2..0f0aa6175 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -1297,13 +1297,14 @@ fn download_and_extract_source( )?; } else { writeln!(log, "Downloading {COMFYUI_SOURCE_ARCHIVE_URL}.")?; - let mut spinner = Spinner::new("Fetching ComfyUI source archive…"); + let download_label = "Fetching ComfyUI source archive…"; + let mut spinner = Spinner::new(download_label); spinner.tick(); let download_result = download_file( COMFYUI_SOURCE_ARCHIVE_URL, &archive_path, &mut |bytes, total| { - spinner.set_progress("Fetching ComfyUI source archive…", bytes, total); + spinner.set_progress(download_label, bytes, total); }, ); spinner.clear(); From af1c3cedca1ec0938821ab462f89927e1bf5cec9 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 9 Sep 2026 05:40:41 +0000 Subject: [PATCH 5/9] fix(cli): keep the download spinner animating and single-line Two UX gaps in the download progress spinner, found while reviewing the progress-indication feature: - The serve spinner keeps animating throughout its wait because its poll loop ticks every iteration regardless of readiness. The download spinner only ticked once before the transfer started and otherwise relied entirely on on_progress firing from data arrival, so it froze solid during a stall (slow handshake, a mid-transfer network hiccup) with no indication the process hadn't hung. AnimatedSpinner fixes this with a background thread that ticks on a fixed interval independent of progress callbacks. - render_current never checked terminal width. Download labels (with byte counts and percentages) are long enough to wrap on a narrow terminal, and Clear(CurrentLine) can only erase the row the cursor ends up on after wrapping, leaving stale fragments behind on every repaint. truncate_to_width keeps every repaint confined to one row. Signed-off-by: Jussi Elo --- apps/rocm/src/cli_progress.rs | 122 +++++++++++++++++++++++++++++++++- apps/rocm/src/comfyui.rs | 7 +- apps/rocm/src/therock.rs | 5 +- 3 files changed, 126 insertions(+), 8 deletions(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index e407a5096..14453e795 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -8,6 +8,9 @@ //! printing a final summary to — never sees control characters. use std::io::{IsTerminal, Write}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; +use std::thread::{self, JoinHandle}; use std::time::{Duration, Instant}; use crossterm::QueueableCommand; @@ -22,6 +25,10 @@ const SPINNER_FRAMES: &[&str] = &["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦ /// repaints per second than a human can perceive. const MIN_PROGRESS_REPAINT_INTERVAL: Duration = Duration::from_millis(100); +/// How often [`AnimatedSpinner`]'s background thread repaints while idle, so +/// a stalled transfer still visibly animates instead of looking hung. +const IDLE_TICK_INTERVAL: Duration = Duration::from_millis(200); + /// A carriage-return status indicator written to stderr. Disabled (a no-op) when /// stderr is not a TTY, so piped/redirected output never receives control /// characters. Keeps stdout clean for whatever the caller prints afterward. @@ -89,10 +96,19 @@ impl Spinner { return; } let frame = SPINNER_FRAMES[self.idx % SPINNER_FRAMES.len()]; + let mut line = format!("{frame} {}", self.label); + if let Ok((cols, _)) = crossterm::terminal::size() { + // A line that fits exactly at `cols` still wraps on some terminals + // once the cursor lands in the last column, and `Clear::CurrentLine` + // on the next repaint can only erase the row the cursor ends up on + // — not a wrapped-over first row. Leaving one column of slack keeps + // every repaint confined to a single row. + line = truncate_to_width(&line, cols.saturating_sub(1) as usize); + } let mut err = std::io::stderr(); let _ = err.queue(MoveToColumn(0)); let _ = err.queue(Clear(ClearType::CurrentLine)); - let _ = write!(err, "{frame} {}", self.label); + let _ = write!(err, "{line}"); let _ = err.flush(); self.active = true; } @@ -109,6 +125,78 @@ impl Spinner { } } +/// Truncates `line` (by character count) to fit within `max_width` columns, +/// appending an ellipsis when it doesn't already fit, so a repaint can never +/// wrap to a second terminal row. +fn truncate_to_width(line: &str, max_width: usize) -> String { + if max_width == 0 { + return String::new(); + } + if line.chars().count() <= max_width { + return line.to_owned(); + } + let keep = max_width.saturating_sub(1); + let mut truncated: String = line.chars().take(keep).collect(); + truncated.push('…'); + truncated +} + +/// A [`Spinner`] kept animating by a background thread, for callers whose +/// progress signal can go quiet for long stretches — a stalled download's +/// `on_progress` callback only fires when bytes actually arrive, unlike +/// `serve`'s HTTP-polling wait loop, which already ticks on every iteration +/// regardless of readiness. Clears the line and stops the thread on drop. +pub(crate) struct AnimatedSpinner { + inner: Arc>, + stop: Arc, + ticker: Option>, +} + +impl AnimatedSpinner { + pub(crate) fn start(label: impl Into) -> Self { + Self::start_with_interval(label, IDLE_TICK_INTERVAL) + } + + fn start_with_interval(label: impl Into, interval: Duration) -> Self { + let inner = Arc::new(Mutex::new(Spinner::new(label))); + inner.lock().unwrap().tick(); + let stop = Arc::new(AtomicBool::new(false)); + let ticker = { + let inner = Arc::clone(&inner); + let stop = Arc::clone(&stop); + thread::spawn(move || { + while !stop.load(Ordering::Relaxed) { + thread::sleep(interval); + if stop.load(Ordering::Relaxed) { + break; + } + inner.lock().unwrap().tick(); + } + }) + }; + Self { + inner, + stop, + ticker: Some(ticker), + } + } + + /// Repaint with a byte-progress label. See [`Spinner::set_progress`]. + pub(crate) fn set_progress(&self, prefix: &str, bytes: u64, total: Option) { + self.inner.lock().unwrap().set_progress(prefix, bytes, total); + } +} + +impl Drop for AnimatedSpinner { + fn drop(&mut self) { + self.stop.store(true, Ordering::Relaxed); + if let Some(ticker) = self.ticker.take() { + let _ = ticker.join(); + } + self.inner.lock().unwrap().clear(); + } +} + /// e.g. `"Downloading SDK tarball… 842.1 MiB / 3.2 GiB (26%)"`, or /// `"Downloading SDK tarball… 842.1 MiB"` when the total is unknown (the /// server never reported a `Content-Length`). @@ -190,4 +278,36 @@ mod tests { spinner.label ); } + + #[test] + fn truncate_to_width_leaves_short_lines_untouched() { + assert_eq!(truncate_to_width("⠋ short", 40), "⠋ short"); + assert_eq!(truncate_to_width("⠋ exact", 7), "⠋ exact"); + } + + #[test] + fn truncate_to_width_ellipsizes_overlong_lines() { + let truncated = truncate_to_width("⠋ a very long download progress line", 10); + assert_eq!(truncated.chars().count(), 10); + assert!( + truncated.ends_with('…'), + "overlong line must end with an ellipsis marker: {truncated}" + ); + } + + #[test] + fn truncate_to_width_handles_zero_width() { + assert_eq!(truncate_to_width("anything", 0), ""); + } + + #[test] + fn animated_spinner_keeps_ticking_without_progress_calls() { + let spinner = AnimatedSpinner::start_with_interval("Downloading…", Duration::from_millis(5)); + thread::sleep(Duration::from_millis(60)); + let idx = spinner.inner.lock().unwrap().idx; + assert!( + idx >= 3, + "the background ticker must keep advancing frames on its own: idx={idx}" + ); + } } diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 0f0aa6175..2cc257438 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -2,7 +2,7 @@ // // SPDX-License-Identifier: MIT -use crate::cli_progress::Spinner; +use crate::cli_progress::AnimatedSpinner; use crate::{format_structured_tool_call, runtime_usability_status, therock}; use anyhow::{Context, Result, bail}; use flate2::read::GzDecoder; @@ -1298,8 +1298,7 @@ fn download_and_extract_source( } else { writeln!(log, "Downloading {COMFYUI_SOURCE_ARCHIVE_URL}.")?; let download_label = "Fetching ComfyUI source archive…"; - let mut spinner = Spinner::new(download_label); - spinner.tick(); + let spinner = AnimatedSpinner::start(download_label); let download_result = download_file( COMFYUI_SOURCE_ARCHIVE_URL, &archive_path, @@ -1307,7 +1306,7 @@ fn download_and_extract_source( spinner.set_progress(download_label, bytes, total); }, ); - spinner.clear(); + drop(spinner); download_result?; } let extract_root = app_root diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 6df94728a..7efc4392e 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -1096,12 +1096,11 @@ fn install_tarball_runtime( } let download_label = format!("Downloading {}…", artifact.file_name); - let mut spinner = crate::cli_progress::Spinner::new(download_label.clone()); - spinner.tick(); + let spinner = crate::cli_progress::AnimatedSpinner::start(download_label.clone()); let download_result = download_file(&artifact.url, &cache_path, &mut |bytes, total| { spinner.set_progress(&download_label, bytes, total); }); - spinner.clear(); + drop(spinner); download_result?; extract_tarball_and_discard_archive(&cache_path, &install_root)?; From b9acb53f4237006ca50fdfac43299e6700b2dfcd Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 9 Sep 2026 06:31:15 +0000 Subject: [PATCH 6/9] fix(cli): truncate by display width, use exact percent arithmetic Addresses Copilot review comments on PR #347: - truncate_to_width measured char count, not terminal columns; a wide (e.g. CJK) glyph in a label could still overflow the row and leave stale spinner fragments. Truncate by Unicode display width instead. - format_download_progress computed the percentage via an f64 ratio, which loses precision for u64 values near u64::MAX and could report 100% while a byte was still outstanding. Use exact u128 arithmetic. Signed-off-by: Jussi Elo --- Cargo.lock | 1 + apps/rocm/Cargo.toml | 1 + apps/rocm/src/cli_progress.rs | 58 +++++++++++++++++++++++++++++------ 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 473b8eb7b..0685bfa54 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3585,6 +3585,7 @@ dependencies = [ "tracing", "tracing-appender", "tracing-subscriber", + "unicode-width", "ureq", "windows-native-keyring-store", "windows-sys 0.61.2", diff --git a/apps/rocm/Cargo.toml b/apps/rocm/Cargo.toml index 944e83e8b..d26c7c8e3 100644 --- a/apps/rocm/Cargo.toml +++ b/apps/rocm/Cargo.toml @@ -42,6 +42,7 @@ tar = "0.4" tracing = "0.1" tracing-appender = "0.2" tracing-subscriber = { version = "0.3", features = ["env-filter"] } +unicode-width = "0.2" ureq = { version = "2.12", features = ["native-certs"] } [target.'cfg(unix)'.dependencies] diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index 14453e795..6adffd0ad 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -16,6 +16,7 @@ use std::time::{Duration, Instant}; use crossterm::QueueableCommand; use crossterm::cursor::MoveToColumn; use crossterm::terminal::{Clear, ClearType}; +use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; /// Braille spinner frames (matching the dashboard's visual language). const SPINNER_FRAMES: &[&str] = &["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; @@ -125,18 +126,29 @@ impl Spinner { } } -/// Truncates `line` (by character count) to fit within `max_width` columns, -/// appending an ellipsis when it doesn't already fit, so a repaint can never -/// wrap to a second terminal row. +/// Truncates `line` (by Unicode display width, not character count — a wide +/// CJK glyph occupies two terminal columns) to fit within `max_width` +/// columns, appending an ellipsis when it doesn't already fit, so a repaint +/// can never wrap to a second terminal row. fn truncate_to_width(line: &str, max_width: usize) -> String { if max_width == 0 { return String::new(); } - if line.chars().count() <= max_width { + if line.width() <= max_width { return line.to_owned(); } - let keep = max_width.saturating_sub(1); - let mut truncated: String = line.chars().take(keep).collect(); + let ellipsis_width = '…'.width().unwrap_or(1); + let keep_width = max_width.saturating_sub(ellipsis_width); + let mut truncated = String::new(); + let mut used_width = 0; + for ch in line.chars() { + let ch_width = ch.width().unwrap_or(0); + if used_width + ch_width > keep_width { + break; + } + truncated.push(ch); + used_width += ch_width; + } truncated.push('…'); truncated } @@ -205,11 +217,14 @@ pub(crate) fn format_download_progress(prefix: &str, bytes: u64, total: Option 0 => { // Floor rather than round: a multi-gigabyte transfer sitting at // 99.5% must not be shown as "complete" while bytes are still - // outstanding. 100% is reserved for `bytes >= total`. + // outstanding. 100% is reserved for `bytes >= total`. Integer + // arithmetic in u128 (rather than an f64 ratio) avoids adjacent + // huge u64 values collapsing to the same float and reporting + // 100% early. let pct = if bytes >= total { 100 } else { - ((bytes as f64 / total as f64) * 100.0).floor() as u64 + ((u128::from(bytes) * 100) / u128::from(total)) as u64 }; format!( "{prefix} {} / {} ({pct}%)", @@ -262,6 +277,18 @@ mod tests { ); } + #[test] + fn format_download_progress_does_not_round_up_to_100_for_huge_totals() { + // An f64 ratio can't distinguish adjacent values this close to + // u64::MAX — it collapses to 1.0 and would misreport 100% while a + // byte is still outstanding. Integer arithmetic must not. + let rendered = format_download_progress("Downloading…", u64::MAX - 1, Some(u64::MAX)); + assert!( + !rendered.contains("(100%)"), + "a single outstanding byte out of u64::MAX must not show as complete: {rendered}" + ); + } + #[test] fn set_progress_never_displays_fewer_bytes_than_already_shown() { let mut spinner = Spinner::new("Downloading…"); @@ -288,7 +315,7 @@ mod tests { #[test] fn truncate_to_width_ellipsizes_overlong_lines() { let truncated = truncate_to_width("⠋ a very long download progress line", 10); - assert_eq!(truncated.chars().count(), 10); + assert_eq!(truncated.width(), 10); assert!( truncated.ends_with('…'), "overlong line must end with an ellipsis marker: {truncated}" @@ -300,6 +327,19 @@ mod tests { assert_eq!(truncate_to_width("anything", 0), ""); } + #[test] + fn truncate_to_width_accounts_for_wide_characters() { + // Each 下 occupies two terminal columns, so a naive char-count + // truncation would keep too many of them and still overflow the row. + let truncated = truncate_to_width("下载中下载中下载中", 10); + assert!( + truncated.width() <= 10, + "display width must respect max_width even with wide glyphs: {truncated} ({})", + truncated.width() + ); + assert!(truncated.ends_with('…')); + } + #[test] fn animated_spinner_keeps_ticking_without_progress_calls() { let spinner = AnimatedSpinner::start_with_interval("Downloading…", Duration::from_millis(5)); From 856afbd94b0893bd70b34cab26d5e79b0f565e45 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 9 Sep 2026 06:40:56 +0000 Subject: [PATCH 7/9] fix(cli): apply cargo fmt to cli_progress.rs Signed-off-by: Jussi Elo --- apps/rocm/src/cli_progress.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index 6adffd0ad..b4c7562d2 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -195,7 +195,10 @@ impl AnimatedSpinner { /// Repaint with a byte-progress label. See [`Spinner::set_progress`]. pub(crate) fn set_progress(&self, prefix: &str, bytes: u64, total: Option) { - self.inner.lock().unwrap().set_progress(prefix, bytes, total); + self.inner + .lock() + .unwrap() + .set_progress(prefix, bytes, total); } } @@ -342,7 +345,8 @@ mod tests { #[test] fn animated_spinner_keeps_ticking_without_progress_calls() { - let spinner = AnimatedSpinner::start_with_interval("Downloading…", Duration::from_millis(5)); + let spinner = + AnimatedSpinner::start_with_interval("Downloading…", Duration::from_millis(5)); thread::sleep(Duration::from_millis(60)); let idx = spinner.inner.lock().unwrap().idx; assert!( From d8d8f9dd9fd7ff480a381d4b2266b21b85273d47 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 9 Sep 2026 12:37:44 +0000 Subject: [PATCH 8/9] fix(cli): address non-blocking review feedback on the download spinner Clarify that the progress reporter's monotonic guarantee covers the byte count, not total (which can change across a retry); document that a from-scratch restart holds the count flat until it catches up. Rename a resume test to match what it actually guards, and note the sibling test that exercises the high-water-mark clamp. Narrow the spinner's "never stale" doc to the known-total case. Skip spawning the animated spinner's ticker thread when stderr isn't a TTY, since every repaint it would trigger is a no-op there. Signed-off-by: Jussi Elo --- apps/rocm/src/cli_progress.rs | 54 +++++++++++++++++++++++++++++------ crates/rocm-core/src/lib.rs | 20 ++++++++++--- 2 files changed, 62 insertions(+), 12 deletions(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index b4c7562d2..90e5d3877 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -68,9 +68,11 @@ impl Spinner { /// Repaint with a byte-progress label. Throttled to at most one repaint /// per [`MIN_PROGRESS_REPAINT_INTERVAL`], except the very first call - /// (`last_progress_paint` starts unset) and the final chunk (`bytes >= - /// total`) always repaint, so the first and last frames shown are never - /// stale. + /// (`last_progress_paint` starts unset) always repaints, as does the + /// final chunk when the total size is known (`bytes >= total`). With an + /// unknown total there is no final-chunk signal to detect, so the true + /// last frame is subject to the same throttle as any other and may be + /// swallowed. /// /// `bytes` is clamped to a high-water mark: a retried transfer that /// restarts from zero (or resumes from an earlier offset than what was @@ -170,13 +172,34 @@ impl AnimatedSpinner { } fn start_with_interval(label: impl Into, interval: Duration) -> Self { + Self::start_with_interval_impl(label, interval, false) + } + + /// Like [`Self::start_with_interval`], but always spawns the ticker + /// thread even when stderr isn't a TTY. Test-only: production callers go + /// through `start`/`start_with_interval`, which skip the thread when + /// nobody can see its repaints, but a test running with stderr piped + /// still needs the thread to verify the ticker mechanism itself. + #[cfg(test)] + fn start_with_interval_forced(label: impl Into, interval: Duration) -> Self { + Self::start_with_interval_impl(label, interval, true) + } + + fn start_with_interval_impl( + label: impl Into, + interval: Duration, + force_ticker: bool, + ) -> Self { let inner = Arc::new(Mutex::new(Spinner::new(label))); inner.lock().unwrap().tick(); let stop = Arc::new(AtomicBool::new(false)); - let ticker = { + // A ticker thread only exists to keep repainting an already-visible + // spinner; when stderr isn't a TTY every repaint it would trigger is + // a no-op, so skip holding an OS thread open for the whole download. + let ticker = if force_ticker || inner.lock().unwrap().enabled { let inner = Arc::clone(&inner); let stop = Arc::clone(&stop); - thread::spawn(move || { + Some(thread::spawn(move || { while !stop.load(Ordering::Relaxed) { thread::sleep(interval); if stop.load(Ordering::Relaxed) { @@ -184,12 +207,14 @@ impl AnimatedSpinner { } inner.lock().unwrap().tick(); } - }) + })) + } else { + None }; Self { inner, stop, - ticker: Some(ticker), + ticker, } } @@ -346,7 +371,7 @@ mod tests { #[test] fn animated_spinner_keeps_ticking_without_progress_calls() { let spinner = - AnimatedSpinner::start_with_interval("Downloading…", Duration::from_millis(5)); + AnimatedSpinner::start_with_interval_forced("Downloading…", Duration::from_millis(5)); thread::sleep(Duration::from_millis(60)); let idx = spinner.inner.lock().unwrap().idx; assert!( @@ -354,4 +379,17 @@ mod tests { "the background ticker must keep advancing frames on its own: idx={idx}" ); } + + #[test] + fn animated_spinner_skips_the_ticker_thread_when_disabled() { + // Test processes don't have a TTY on stderr, so `start_with_interval` + // (unlike `start_with_interval_forced`) must see `enabled == false` + // here and skip spawning the thread entirely. + let spinner = + AnimatedSpinner::start_with_interval("Downloading…", Duration::from_millis(5)); + assert!( + spinner.ticker.is_none(), + "no ticker thread should be spawned when stderr isn't a TTY" + ); + } } diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index 87ed9fb7e..44e385c00 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -249,14 +249,21 @@ pub fn download_file_streaming(request: &DownloadRequest<'_>) -> Result, on_progress: &mut dyn FnMut(u64, Option), @@ -8189,7 +8196,12 @@ mod tests { } #[test] - fn download_with_progress_stays_continuous_across_a_resumed_attempt() -> Result<()> { + // This guards the resume path re-seeding `written` from the partial file + // already on disk (see `written = std::io::copy(...)` above), not the + // high-water-mark clamp itself — it would pass unchanged with the clamp + // removed entirely. `download_with_progress_stays_monotonic_after_a_discarded_restart` + // below is the one that actually exercises the clamp. + fn download_with_progress_reports_the_resumed_offset_before_reading_more() -> Result<()> { let body = download_body(); let (port, server) = spawn_download_server( body.clone(), From 7565e19d10eddae64c07b3052edd92161e0def8e Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 9 Sep 2026 12:51:50 +0000 Subject: [PATCH 9/9] fix(rocm): make AnimatedSpinner ticker tests independent of stderr TTY The disabled-ticker test relied on the test process's real stderr not being a TTY, so it only passed because CI happens to redirect stderr. Run from an interactive terminal it would flip to failing. Force Spinner::enabled explicitly via a test-only constructor instead of probing the environment. Signed-off-by: Jussi Elo --- apps/rocm/src/cli_progress.rs | 52 +++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index 90e5d3877..308cf6eb3 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -172,31 +172,41 @@ impl AnimatedSpinner { } fn start_with_interval(label: impl Into, interval: Duration) -> Self { - Self::start_with_interval_impl(label, interval, false) + Self::start_with_interval_impl(label, interval, None) } - /// Like [`Self::start_with_interval`], but always spawns the ticker - /// thread even when stderr isn't a TTY. Test-only: production callers go - /// through `start`/`start_with_interval`, which skip the thread when - /// nobody can see its repaints, but a test running with stderr piped - /// still needs the thread to verify the ticker mechanism itself. + /// Like [`Self::start_with_interval`], but overrides whether the spinner + /// is treated as enabled instead of probing stderr. Test-only: whether + /// the ticker thread spawns depends on `Spinner::enabled`, which + /// `Spinner::new` derives from the real `stderr().is_terminal()` — a + /// property of however the test happens to be run, not of the behavior + /// under test. Forcing it here keeps these tests deterministic whether + /// `cargo test` is launched from an interactive terminal or not. #[cfg(test)] - fn start_with_interval_forced(label: impl Into, interval: Duration) -> Self { - Self::start_with_interval_impl(label, interval, true) + fn start_with_interval_enabled( + label: impl Into, + interval: Duration, + enabled: bool, + ) -> Self { + Self::start_with_interval_impl(label, interval, Some(enabled)) } fn start_with_interval_impl( label: impl Into, interval: Duration, - force_ticker: bool, + enabled_override: Option, ) -> Self { - let inner = Arc::new(Mutex::new(Spinner::new(label))); + let mut spinner = Spinner::new(label); + if let Some(enabled) = enabled_override { + spinner.enabled = enabled; + } + let inner = Arc::new(Mutex::new(spinner)); inner.lock().unwrap().tick(); let stop = Arc::new(AtomicBool::new(false)); // A ticker thread only exists to keep repainting an already-visible // spinner; when stderr isn't a TTY every repaint it would trigger is // a no-op, so skip holding an OS thread open for the whole download. - let ticker = if force_ticker || inner.lock().unwrap().enabled { + let ticker = if inner.lock().unwrap().enabled { let inner = Arc::clone(&inner); let stop = Arc::clone(&stop); Some(thread::spawn(move || { @@ -370,8 +380,11 @@ mod tests { #[test] fn animated_spinner_keeps_ticking_without_progress_calls() { - let spinner = - AnimatedSpinner::start_with_interval_forced("Downloading…", Duration::from_millis(5)); + let spinner = AnimatedSpinner::start_with_interval_enabled( + "Downloading…", + Duration::from_millis(5), + true, + ); thread::sleep(Duration::from_millis(60)); let idx = spinner.inner.lock().unwrap().idx; assert!( @@ -382,11 +395,14 @@ mod tests { #[test] fn animated_spinner_skips_the_ticker_thread_when_disabled() { - // Test processes don't have a TTY on stderr, so `start_with_interval` - // (unlike `start_with_interval_forced`) must see `enabled == false` - // here and skip spawning the thread entirely. - let spinner = - AnimatedSpinner::start_with_interval("Downloading…", Duration::from_millis(5)); + // Force `enabled = false` explicitly rather than relying on stderr + // not being a TTY in the test process, so this stays deterministic + // whether `cargo test` runs piped (CI) or from an interactive shell. + let spinner = AnimatedSpinner::start_with_interval_enabled( + "Downloading…", + Duration::from_millis(5), + false, + ); assert!( spinner.ticker.is_none(), "no ticker thread should be spawned when stderr isn't a TTY"