diff --git a/Cargo.lock b/Cargo.lock index 8b8bf4d6f..531d21b17 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1269,7 +1269,9 @@ dependencies = [ "axum", "cucumber", "e2e-report", + "futures", "portable-pty", + "rand 0.9.4", "reqwest 0.13.4", "serde", "serde_json", diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index 308cf6eb3..9e2d392bc 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -30,13 +30,48 @@ const MIN_PROGRESS_REPAINT_INTERVAL: Duration = Duration::from_millis(100); /// a stalled transfer still visibly animates instead of looking hung. const IDLE_TICK_INTERVAL: Duration = Duration::from_millis(200); +/// Assumed terminal width when `crossterm::terminal::size()` fails (e.g. +/// stderr is a TTY but not one `ioctl(TIOCGWINSZ)` can query). The +/// conventional default columns most real terminals start at, so a repaint +/// still truncates to a single row instead of growing unbounded. +const FALLBACK_WIDTH: u16 = 80; + +/// The spinner's current text: either a plain label, or a label paired with +/// a progress suffix that [`assemble_status_line`] must always keep intact. +/// Folding both into one type — rather than two independently-mutated +/// fields a caller could update out of sync — makes "a plain message never +/// carries a stale byte-count suffix" a structural invariant instead of a +/// convention every setter has to remember to uphold. +enum SpinnerText { + Plain(String), + Progress { label: String, suffix: String }, +} + +impl SpinnerText { + fn label(&self) -> &str { + match self { + Self::Plain(label) | Self::Progress { label, .. } => label, + } + } + + /// The byte-count/percentage tail of a progress label (e.g. + /// `" 1.5 MiB / 19.1 MiB (8%)"`), kept apart from the label so + /// [`assemble_status_line`] can always keep it intact — see its comment. + fn suffix(&self) -> Option<&str> { + match self { + Self::Plain(_) => None, + Self::Progress { suffix, .. } => Some(suffix), + } + } +} + /// 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, + text: SpinnerText, active: bool, last_progress_paint: Option, max_progress_bytes: u64, @@ -47,7 +82,7 @@ impl Spinner { Self { enabled: std::io::stderr().is_terminal(), idx: 0, - label: label.into(), + text: SpinnerText::Plain(label.into()), active: false, last_progress_paint: None, max_progress_bytes: 0, @@ -56,7 +91,7 @@ impl Spinner { /// 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.text = SpinnerText::Plain(label.into()); self.render_current(); } @@ -90,7 +125,10 @@ impl Spinner { } self.last_progress_paint = Some(now); self.idx = self.idx.wrapping_add(1); - self.label = format_download_progress(prefix, bytes, total); + self.text = SpinnerText::Progress { + label: prefix.to_owned(), + suffix: format_progress_suffix(bytes, total), + }; self.render_current(); } @@ -99,15 +137,25 @@ 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); - } + // 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. When the size can't be + // queried, fall back to the same conventional 80-column width the + // e2e PTY harness and most real terminals default to, so this path + // still truncates instead of emitting an unbounded line — it's rare + // (an unusual stderr, not merely "not a TTY", which `enabled` already + // filters out above). The fallback *value* is covered by assembling + // at `FALLBACK_WIDTH` directly; the `size()` error branch itself is + // not exercised by any test. + let cols = crossterm::terminal::size().map_or(FALLBACK_WIDTH, |(cols, _)| cols); + let line = assemble_status_line( + frame, + self.text.label(), + self.text.suffix(), + cols.saturating_sub(1) as usize, + ); let mut err = std::io::stderr(); let _ = err.queue(MoveToColumn(0)); let _ = err.queue(Clear(ClearType::CurrentLine)); @@ -155,6 +203,60 @@ fn truncate_to_width(line: &str, max_width: usize) -> String { truncated } +/// Assembles `"{frame} {label}{suffix}"` within `max_width` columns. +/// +/// When `suffix` is present (a download's byte-count/percentage tail) and +/// the full line would overflow, truncates `label` — the operation's file +/// name, already printed in full elsewhere in the command's output — rather +/// than the assembled line as a whole, so `suffix` always survives intact. +/// Without truncating this way, `label`'s growth alone (e.g. `"0 B"` growing +/// into `"1.5 MiB"`) can push a line that fit at 0% past the terminal width, +/// and a blind tail-truncation would silently drop the percentage for the +/// rest of the transfer. +/// +/// If `frame`, the mandatory separator space, and `suffix` together already +/// meet or exceed `max_width` (an extremely narrow terminal, a suffix wider +/// than the terminal, or the exact boundary where there'd be zero columns +/// left for the label), there is no +/// longer room to keep `suffix` intact with a label alongside it either — +/// falls back to truncating `"{frame}{suffix}"` as a whole (no literal +/// space; `suffix` already carries its own leading space), same as the +/// no-suffix case below, so the result never exceeds `max_width` regardless +/// of how narrow it is. +fn assemble_status_line( + frame: &str, + label: &str, + suffix: Option<&str>, + max_width: usize, +) -> String { + let Some(suffix) = suffix else { + return truncate_to_width(&format!("{frame} {label}"), max_width); + }; + debug_assert!( + suffix.starts_with(' '), + "assemble_status_line's narrow-terminal fallback below assumes `suffix` \ + already carries its own leading space (true of every current caller via \ + `format_progress_suffix`); a space-less suffix would glue straight onto \ + `frame` with no gap: {suffix:?}" + ); + let reserved = frame.width() + 1 + suffix.width(); + // `>=`, not `>`: at the exact boundary (`reserved == max_width`) the + // label_budget branch below would still take the label path, but with a + // budget of exactly 0 — truncating the label to nothing while the + // explicit space before it and `suffix`'s own leading space both remain, + // doubling up the gap. Routing the exact-fit case through this fallback + // too keeps that boundary case's single space consistent with every + // narrower width's. + if reserved >= max_width { + // No literal space here: `suffix` (from `format_progress_suffix`) + // already carries its own leading space, matching the spacing the + // label_budget branch below produces between `frame` and `suffix`. + return truncate_to_width(&format!("{frame}{suffix}"), max_width); + } + let label_budget = max_width - reserved; + format!("{frame} {}{suffix}", truncate_to_width(label, label_budget)) +} + /// 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 @@ -247,10 +349,11 @@ impl Drop for AnimatedSpinner { } } -/// 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 { +/// The trailing `" / (%)"` (or `" "` when the +/// total is unknown) portion of a progress label, kept separate from the +/// operation prefix so [`assemble_status_line`] can always keep it +/// visible — see its comment. +fn format_progress_suffix(bytes: u64, total: Option) -> String { match total { Some(total) if total > 0 => { // Floor rather than round: a multi-gigabyte transfer sitting at @@ -265,12 +368,12 @@ pub(crate) fn format_download_progress(prefix: &str, bytes: u64, total: Option format!("{prefix} {}", rocm_core::format_bytes(bytes)), + _ => format!(" {}", rocm_core::format_bytes(bytes)), } } @@ -279,27 +382,30 @@ mod tests { use super::*; #[test] - fn format_download_progress_shows_bytes_and_percent_when_total_is_known() { + fn format_progress_suffix_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%)" + format_progress_suffix(gib, Some(4 * gib)), + " 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); + fn format_progress_suffix_omits_total_when_unknown() { + let rendered = format_progress_suffix(883_147_264, None); assert!( !rendered.contains('/') && !rendered.contains('%'), "no total means no fraction or percentage: {rendered}" ); - assert!(rendered.starts_with("Downloading… ")); + assert!( + rendered.starts_with(' '), + "format_progress_suffix's None-total branch must keep its own leading space: {rendered}" + ); } #[test] - fn format_download_progress_clamps_percent_at_100_when_bytes_exceeds_total() { - let rendered = format_download_progress("Downloading…", 105, Some(100)); + fn format_progress_suffix_clamps_percent_at_100_when_bytes_exceeds_total() { + let rendered = format_progress_suffix(105, Some(100)); assert!( rendered.contains("(100%)"), "a server sending a few bytes past its declared length must not report over 100%: {rendered}" @@ -307,8 +413,8 @@ mod tests { } #[test] - fn format_download_progress_does_not_round_up_to_100_before_completion() { - let rendered = format_download_progress("Downloading…", 995, Some(1000)); + fn format_progress_suffix_does_not_round_up_to_100_before_completion() { + let rendered = format_progress_suffix(995, Some(1000)); assert!( rendered.contains("(99%)"), "99.5% must floor to 99%, not round up to a premature 100%: {rendered}" @@ -316,11 +422,11 @@ mod tests { } #[test] - fn format_download_progress_does_not_round_up_to_100_for_huge_totals() { + fn format_progress_suffix_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)); + let rendered = format_progress_suffix(u64::MAX - 1, Some(u64::MAX)); assert!( !rendered.contains("(100%)"), "a single outstanding byte out of u64::MAX must not show as complete: {rendered}" @@ -331,16 +437,125 @@ mod tests { 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")); + assert!(spinner.text.suffix().unwrap().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)); + let suffix = spinner.text.suffix().unwrap(); + assert!( + suffix.contains("900"), + "progress must not regress after a retry: {suffix}" + ); + } + + #[test] + fn set_label_clears_a_stale_progress_suffix() { + // A caller that moves on to a plain (non-byte-progress) message must + // not have a previous transfer's byte count still glued to it — + // `render_current` would otherwise render an unrelated message with a + // stale suffix appended. + let mut spinner = Spinner::new("Downloading…"); + spinner.set_progress("Downloading…", 900, Some(1000)); + assert!(spinner.text.suffix().is_some()); + spinner.set_label("Checking AMD GPU access…"); assert!( - spinner.label.contains("900"), - "progress must not regress after a retry: {}", - spinner.label + spinner.text.suffix().is_none(), + "set_label must clear any progress suffix left over from a prior set_progress call" + ); + } + + #[test] + fn assemble_status_line_keeps_the_progress_suffix_intact_when_the_label_would_overflow() { + // Regression test: an early version truncated the whole assembled + // line from the tail, which — once the byte count grew past a couple + // of characters — cut off the "(NN%)" suffix entirely on an ordinary + // 80-column terminal, silently hiding the download's percentage for + // the rest of the transfer. Truncation must eat the (already + // fully-shown-elsewhere) file name instead. + let label = "Downloading therock-dist-linux-gfx120X-all-7.10.0.tar.gz…"; + let suffix = format_progress_suffix(1_608_192, Some(20_003_341)); + let line = assemble_status_line("⠋", label, Some(&suffix), 79); + assert!( + line.contains(&suffix), + "the progress suffix must survive truncation intact: {line:?}" + ); + assert!( + line.width() <= 79, + "the assembled line must still respect the terminal width: {line:?} (width {})", + line.width() + ); + } + + #[test] + fn assemble_status_line_produces_exact_output_on_the_ordinary_label_fits_path() { + // Regression test: the tests around this one only assert + // `contains`/`width <=` on the ordinary (non-boundary, non-fallback) + // `label_budget` branch, so a mutation dropping the separator space + // between `frame` and `label`, or shrinking `label_budget` by one, + // would still pass every other test in this module. A label whose + // width exactly fills its budget makes both mutations visible: the + // former glues `frame` and `label` together, and the latter forces + // an otherwise-unwarranted truncation. + let suffix = format_progress_suffix(883_147_264, None); + let label = "exact"; + let max_width = "⠋".width() + 1 + suffix.width() + label.width(); + let line = assemble_status_line("⠋", label, Some(&suffix), max_width); + assert_eq!(line, format!("⠋ {label}{suffix}")); + } + + #[test] + fn assemble_status_line_never_exceeds_max_width_when_suffix_alone_overflows() { + // Regression test: when the terminal is narrower than `frame + " " + + // suffix` alone, the label truncates to "" and an earlier version + // fell back to printing the untruncated suffix anyway, silently + // exceeding `max_width` — the same bug class this module exists to + // eliminate, just past the point where the suffix can stay intact. + let suffix = format_progress_suffix(1_608_192, Some(20_003_341)); + assert!(suffix.width() > 10, "test needs an overlong suffix"); + let line = assemble_status_line("⠋", "Downloading a file…", Some(&suffix), 10); + assert!( + line.width() <= 10, + "the assembled line must never exceed max_width, even when the \ + suffix alone doesn't fit: {line:?} (width {})", + line.width() + ); + } + + #[test] + fn assemble_status_line_fallback_does_not_double_the_space_before_suffix() { + // Regression test: `format_progress_suffix` already returns a string + // with its own leading space (e.g. " 883.1 MiB"). The narrow-terminal + // fallback used to insert another literal space before it, wasting a + // column of already-scarce width on a doubled-up gap. + let suffix = format_progress_suffix(883_147_264, None); + let max_width = 1 + suffix.width(); + let line = assemble_status_line("⠋", "irrelevant label", Some(&suffix), max_width); + assert_eq!( + line, + format!("⠋{suffix}"), + "the suffix's own leading space must not be doubled up: {line:?}" + ); + } + + #[test] + fn assemble_status_line_does_not_double_the_space_at_the_exact_fit_boundary() { + // Regression test: at `reserved == max_width` exactly (frame + the + // mandatory space + suffix fills the width with zero columns left for + // any label), an earlier version still took the label_budget branch + // with a budget of 0, truncating the label to nothing while leaving + // both the branch's own literal space *and* the suffix's leading + // space in the output — one column narrower and the fallback branch + // produced a single space instead. The exact-fit case must match its + // narrower neighbor, not double up. + let suffix = format_progress_suffix(883_147_264, None); + let max_width = "⠋".width() + 1 + suffix.width(); + let line = assemble_status_line("⠋", "irrelevant label", Some(&suffix), max_width); + assert_eq!( + line, + format!("⠋{suffix}"), + "the exact-fit boundary must not double the space before suffix: {line:?}" ); } diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 2f45da87f..0e5c954a3 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -31,6 +31,18 @@ const COMFYUI_SOURCE_ARCHIVE_NAME: &str = "ComfyUI-master.tar.gz"; const COMFYUI_DEFAULT_HOST: &str = "127.0.0.1"; const COMFYUI_DEFAULT_PORT: u16 = 8188; +/// The ComfyUI source archive URL, overridable only in `e2e-test-hooks` +/// builds so a fixture server can exercise the real download path. +#[cfg(feature = "e2e-test-hooks")] +fn comfyui_source_archive_url() -> String { + std::env::var("ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE") + .unwrap_or_else(|_| COMFYUI_SOURCE_ARCHIVE_URL.to_owned()) +} +#[cfg(not(feature = "e2e-test-hooks"))] +fn comfyui_source_archive_url() -> String { + COMFYUI_SOURCE_ARCHIVE_URL.to_owned() +} + #[derive(Debug, Clone, Eq, PartialEq)] pub(crate) struct ComfyUiInstallOptions { pub runtime_id: Option, @@ -324,7 +336,7 @@ pub(crate) fn install( fs::remove_dir_all(&source_path) .with_context(|| format!("failed to remove {}", source_path.display()))?; } - if source_path.exists() { + let source_url = if source_path.exists() { println!("Using existing ComfyUI source folder..."); let _ = io::stdout().flush(); writeln!( @@ -332,11 +344,12 @@ pub(crate) fn install( "Using existing ComfyUI folder at {}.", source_path.display() )?; + reused_source_url(paths) } else { println!("Downloading ComfyUI source..."); let _ = io::stdout().flush(); - download_and_extract_source(&app_root, &source_path, &mut log)?; - } + download_and_extract_source(&app_root, &source_path, &mut log)? + }; fs::create_dir_all(&models_folder) .with_context(|| format!("failed to create {}", models_folder.display()))?; @@ -384,7 +397,7 @@ pub(crate) fn install( runtime_version: runtime.manifest.version.clone(), runtime_root: runtime.manifest.install_root.clone(), python_executable: runtime.python.clone(), - source_url: COMFYUI_SOURCE_ARCHIVE_URL.to_owned(), + source_url, source_path: source_path.clone(), requirements_path, pip_cache_dir: None, @@ -980,6 +993,21 @@ fn load_manifest(paths: &AppPaths) -> Result> { .with_context(|| format!("failed to parse {}", path.display())) } +/// Reuse whatever URL the manifest already on disk recorded, rather than the +/// current `comfyui_source_archive_url()` — that folder was produced by +/// *some* prior install, which may have run under a different +/// source-archive override than this one. Falls back to the current URL if +/// there's no prior manifest that can be read (e.g. it was deleted out from +/// under an otherwise-intact source folder, or is unreadable/unparseable) — +/// this path must not abort on a broken manifest, since it's otherwise the +/// one command that recovers from one. +fn reused_source_url(paths: &AppPaths) -> String { + load_manifest(paths) + .ok() + .flatten() + .map_or_else(comfyui_source_archive_url, |manifest| manifest.source_url) +} + fn save_manifest(paths: &AppPaths, manifest: &ComfyUiManifest) -> Result<()> { let path = manifest_path(paths); fs::create_dir_all( @@ -1378,11 +1406,16 @@ fn same_path_text(left: &Path, right: &Path) -> bool { runtime_paths_equivalent(left, right) } +/// Downloads (if not already cached) and extracts the ComfyUI source +/// archive, returning the source URL it resolved — so the caller can record +/// it on the install manifest without re-resolving +/// [`comfyui_source_archive_url`] a second time. fn download_and_extract_source( app_root: &Path, source_path: &Path, log: &mut fs::File, -) -> Result<()> { +) -> Result { + let source_url = comfyui_source_archive_url(); let archive_path = app_root.join("downloads").join(COMFYUI_SOURCE_ARCHIVE_NAME); fs::create_dir_all( archive_path @@ -1396,16 +1429,12 @@ fn download_and_extract_source( archive_path.display() )?; } else { - writeln!(log, "Downloading {COMFYUI_SOURCE_ARCHIVE_URL}.")?; + writeln!(log, "Downloading {source_url}.")?; let download_label = "Fetching ComfyUI source archive…"; let spinner = AnimatedSpinner::start(download_label); - let download_result = download_file( - COMFYUI_SOURCE_ARCHIVE_URL, - &archive_path, - &mut |bytes, total| { - spinner.set_progress(download_label, bytes, total); - }, - ); + let download_result = download_file(&source_url, &archive_path, &mut |bytes, total| { + spinner.set_progress(download_label, bytes, total); + }); drop(spinner); download_result?; } @@ -1437,7 +1466,7 @@ fn download_and_extract_source( })?; fs::remove_dir_all(&extract_root).ok(); writeln!(log, "Installed source at {}.", source_path.display())?; - Ok(()) + Ok(source_url) } fn first_child_dir(root: &Path) -> Result { @@ -2087,7 +2116,7 @@ mod tests { runtime_version: runtime.version.clone(), runtime_root: runtime.install_root.clone(), python_executable: paths.data_dir.join("runtimes").join("python.exe"), - source_url: COMFYUI_SOURCE_ARCHIVE_URL.to_owned(), + source_url: comfyui_source_archive_url(), source_path: source_path(&paths), requirements_path: source_path(&paths).join("requirements.txt"), pip_cache_dir: None, @@ -2119,6 +2148,62 @@ mod tests { Ok(()) } + fn test_manifest_with_source_url(paths: &AppPaths, source_url: &str) -> ComfyUiManifest { + ComfyUiManifest { + app_id: APP_ID.to_owned(), + runtime_key: "test-runtime".to_owned(), + runtime_id: "test-runtime-id".to_owned(), + runtime_version: "1.0.0".to_owned(), + runtime_root: paths.data_dir.join("runtimes").join("test-runtime"), + python_executable: paths.data_dir.join("runtimes").join("python.exe"), + source_url: source_url.to_owned(), + source_path: source_path(paths), + requirements_path: source_path(paths).join("requirements.txt"), + pip_cache_dir: None, + log_path: app_root(paths).join("logs").join("install-100.log"), + torch_version: None, + torch_cuda_available: false, + installed_at_unix_ms: 100, + } + } + + #[test] + fn reused_source_url_falls_back_to_current_url_when_no_manifest_on_disk() { + let paths = test_paths("comfyui-reused-url-no-manifest"); + + let url = reused_source_url(&paths); + + assert_eq!(url, comfyui_source_archive_url()); + } + + #[test] + fn reused_source_url_returns_recorded_url_from_valid_manifest() -> Result<()> { + let paths = test_paths("comfyui-reused-url-valid-manifest"); + let recorded_url = "https://example.invalid/prior-comfyui-source.tar.gz"; + save_manifest(&paths, &test_manifest_with_source_url(&paths, recorded_url))?; + + let url = reused_source_url(&paths); + + assert_eq!(url, recorded_url); + Ok(()) + } + + #[test] + fn reused_source_url_falls_back_to_current_url_on_unparseable_manifest() -> Result<()> { + // Mutation-sensitive: fails the instant `reused_source_url` reverts + // from `.ok().flatten()` to propagating `load_manifest`'s error, + // which is the exact regression this PR shipped and then fixed. + let paths = test_paths("comfyui-reused-url-corrupt-manifest"); + let path = manifest_path(&paths); + fs::create_dir_all(path.parent().expect("manifest path has a parent"))?; + fs::write(&path, b"not valid json")?; + + let url = reused_source_url(&paths); + + assert_eq!(url, comfyui_source_archive_url()); + Ok(()) + } + #[test] fn install_dry_run_uses_selected_runtime_folder() -> Result<()> { let paths = test_paths("comfyui-selected-runtime-folder"); @@ -2174,7 +2259,7 @@ mod tests { runtime_version: "7.13.0a20260511".to_owned(), runtime_root: paths.data_dir.join("runtimes").join("runtime"), python_executable: paths.data_dir.join("runtimes").join("python.exe"), - source_url: COMFYUI_SOURCE_ARCHIVE_URL.to_owned(), + source_url: comfyui_source_archive_url(), source_path: source_path(&paths), requirements_path: source_path(&paths).join("requirements.txt"), pip_cache_dir: None, diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 281094944..c9163d669 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -9519,6 +9519,23 @@ fn ensure_libnuma_for_torch(approved: bool) { ); } +/// Whether an E2E scenario has asked to skip the torch runtime dependency +/// checks entirely. These checks run a real system package-manager install +/// (`apt-get` or equivalent) whenever a dependency happens to be missing on +/// the host, which is slow, network-dependent, and mutates host state — none +/// of which a PTY scenario testing an unrelated concern (e.g. the download +/// spinner) should depend on. Only active under `e2e-test-hooks`; production +/// builds always run the real check. +#[cfg(feature = "e2e-test-hooks")] +fn torch_runtime_dep_checks_disabled() -> bool { + std::env::var_os("ROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS").is_some() +} + +#[cfg(not(feature = "e2e-test-hooks"))] +const fn torch_runtime_dep_checks_disabled() -> bool { + false +} + /// Shared control flow behind [`ensure_libatomic_for_torch`] and /// [`ensure_libnuma_for_torch`]: detect the dependency, print the distro-aware /// plan, and (when approved or auto-installable) run it via @@ -9528,6 +9545,9 @@ fn ensure_torch_runtime_dep(approved: bool, dep: &TorchRuntimeDep) { if cfg!(windows) { return; } + if torch_runtime_dep_checks_disabled() { + return; + } if (dep.present)() { return; } diff --git a/docs/release-trust.md b/docs/release-trust.md index 9edb6ddca..3fa23afce 100644 --- a/docs/release-trust.md +++ b/docs/release-trust.md @@ -228,6 +228,22 @@ Set the gate and the specific base variable together, and only in tests or deliberate manual QA against a fixture server. The `therock-next` E2E scenarios use exactly this pairing to exercise next-layout dispatch hermetically. +## ComfyUI Source Archive Override + +rocm-cli hardcodes the URL it downloads the ComfyUI source archive from. It +can be overridden, for fixture-server testing only, and only in builds +compiled with the `e2e-test-hooks` Cargo feature: + +```text +ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE +``` + +Unlike the TheRock base overrides above, this needs no separate "allow" gate: +the override *logic* does not exist at all in a build without +`e2e-test-hooks` — the accessor compiled in that configuration ignores the +environment entirely and unconditionally returns the hardcoded default URL, so +a stray environment variable can never redirect a production install. + ## Remaining Owner Step The repo still needs a real project-owned public signing key and matching diff --git a/tests/e2e-cucumber/Cargo.toml b/tests/e2e-cucumber/Cargo.toml index 88e74611d..89e1e6541 100644 --- a/tests/e2e-cucumber/Cargo.toml +++ b/tests/e2e-cucumber/Cargo.toml @@ -26,11 +26,16 @@ path = "src/bin/fake-tailscale.rs" axum.workspace = true cucumber = { version = "0.23", features = ["output-json", "output-junit"] } e2e-report = { path = "../../crates/e2e-report" } +# Builds the paced download fixture's chunked response body (`stream::unfold`). +futures = "0.3" # Drive the interactive dash TUI black-box: spawn the real `rocm` binary under a # pseudo-terminal (`portable-pty`, cross-platform openpty/ConPTY) and parse the # emitted terminal stream into the current on-screen grid (`vt100`). This is the # only way to exercise the crossterm raw-mode event loop a piped `Command` can't. portable-pty = "0.9" +# Deterministic filler bytes for the paced-download fixtures (see +# `src/paced_download.rs`) — a seeded `StdRng` rather than a hand-rolled PRNG. +rand.workspace = true reqwest = { version = "0.13", features = ["json"] } serde.workspace = true serde_json.workspace = true diff --git a/tests/e2e-cucumber/features/comfyui.feature b/tests/e2e-cucumber/features/comfyui.feature index 8dcbbdec3..db02e0dec 100644 --- a/tests/e2e-cucumber/features/comfyui.feature +++ b/tests/e2e-cucumber/features/comfyui.feature @@ -61,3 +61,28 @@ Feature: ComfyUI install reports progress and makes failures actionable And the refusal names the --runtime-id flag And the refusal names rocm runtimes activate And the refusal lists both runtime keys + + # `download_and_extract_source` reports its download the same way TheRock's + # tarball install does (`cli_progress::AnimatedSpinner`), but its extraction + # is an in-process `GzDecoder`/`tar` unpack with no separate progress phase — + # unlike TheRock's subprocess `tar -xf`, it never renders its own frame. This + # proves the download half end to end: a real `rocm` binary, under a real + # PTY, fetching from a server paced slowly enough to observe an intermediate + # progress frame, and confirms the spinner line is gone once the process + # exits. See `download_progress_pty.feature` for the TheRock counterpart. + # @serial: same reasoning as `download_progress_pty.feature`'s + # `download-progress-01` — this scenario's intermediate progress frame + # depends on real wall-clock pacing that CPU contention from up to 63 + # concurrently-running scenarios can starve away entirely. + # Note: "Fetching ComfyUI source archive…" is short enough to never + # truncate at 80 columns, so this scenario does not exercise the + # label-truncation fix in `cli_progress::assemble_status_line` — the + # tarball scenario in `download_progress_pty.feature` is the regression + # test for that. + @id:comfyui-source-download-shows-live-progress @requires-os:linux @serial + Scenario: comfyui-04 - The source-archive download spinner renders progress and clears on completion + Given a paced ComfyUI source archive fixture + When the user installs ComfyUI under a real terminal + Then the terminal shows an intermediate ComfyUI download progress frame + And the ComfyUI install exits cleanly + And the final terminal screen shows no ComfyUI download spinner line diff --git a/tests/e2e-cucumber/features/download_progress_pty.feature b/tests/e2e-cucumber/features/download_progress_pty.feature new file mode 100644 index 000000000..bab70dcc3 --- /dev/null +++ b/tests/e2e-cucumber/features/download_progress_pty.feature @@ -0,0 +1,25 @@ +Feature: Download-progress spinner under a real terminal + + # `cli_progress::AnimatedSpinner` only has in-process unit coverage today — + # it never runs under a spawned subprocess, so a regression that broke its + # TTY detection, throttling, or line-clearing on `Drop` could ship + # unnoticed. This proves it end to end: a real `rocm` binary, under a real + # PTY, downloading from a server paced slowly enough to observe an + # intermediate progress frame, and confirms the spinner line is gone once + # the process exits. + + # @serial: this scenario's progress frames depend on real wall-clock pacing + # between paced HTTP chunks and the PTY's polling cadence. Running alongside + # up to 63 other scenarios (the mock lane's default concurrency) starves it + # of CPU at unpredictable moments, letting the whole paced transfer (or the + # `tar` extraction) complete between polls with no intermediate frame ever + # observed — reproduced locally by running the full suite, never by running + # this scenario alone. Serial execution removes that contention. + @id:download-progress-linux-tarball-install-shows-live-progress @requires-os:linux @serial + Scenario: download-progress-01 - The tarball download spinner renders progress and clears on completion + Given a paced canonical release tarball fixture + When the user installs the tarball SDK for family gfx120X-all under a real terminal + Then the terminal shows an intermediate download progress frame + And the terminal shows the archive being extracted + And the tarball install exits cleanly + And the final terminal screen shows neither spinner line diff --git a/tests/e2e-cucumber/src/expectation.rs b/tests/e2e-cucumber/src/expectation.rs index bf382a487..145d1e7be 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -36,6 +36,13 @@ const NIGHTLY_TAG: &str = "nightly"; const LIFECYCLE_TAG: &str = "lifecycle"; const MERGE_QUEUE_TAG: &str = "merge-queue"; +// `@serial` deliberately has no entry here, and `from_tags` below silently +// ignores it like any other unrecognized tag: it isn't an expectation- +// resolution concern, it's a cucumber-rs *runner* concern, consumed directly +// by its default `Runner::Basic::which_scenario` (unmodified by this crate) to +// force a scenario to run without any concurrent sibling. A feature file's +// `@serial` tag works whether or not it's listed here. + /// The resolved expectation for one scenario on one host. #[derive(Debug, Clone, PartialEq, Eq)] pub enum Expectation { diff --git a/tests/e2e-cucumber/src/lib.rs b/tests/e2e-cucumber/src/lib.rs index d56451520..f82bde9f6 100644 --- a/tests/e2e-cucumber/src/lib.rs +++ b/tests/e2e-cucumber/src/lib.rs @@ -10,6 +10,7 @@ pub mod loopback_http; pub mod mock_server; pub mod model_id; pub mod monotonic_clock; +pub mod paced_download; pub mod panic_capture; pub mod reader_failure; pub mod send_until; diff --git a/tests/e2e-cucumber/src/paced_download.rs b/tests/e2e-cucumber/src/paced_download.rs new file mode 100644 index 000000000..7e5b4cc7e --- /dev/null +++ b/tests/e2e-cucumber/src/paced_download.rs @@ -0,0 +1,299 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Loopback HTTP server that serves one named file in delayed chunks. +//! +//! [`crate::loopback_http::LoopbackServer`] answers every request from +//! `ServeDir` as fast as the OS can read the file, which never gives a PTY +//! test harness a chance to observe an intermediate download-progress frame +//! from `cli_progress::AnimatedSpinner` — the whole transfer completes within +//! a single poll of the emulated screen. This server keeps `ServeDir` as the +//! fallback for every other path, but answers one specific file itself, with +//! an accurate `Content-Length` header and the body written as fixed-size +//! chunks separated by a fixed delay. `rocm-core`'s download client reads +//! `Content-Length` to compute the progress percentage and reads the body in +//! an ordinary streaming loop, so pacing here needs nothing special on the +//! client side — it behaves exactly as if a slow network served the file. + +use std::path::Path; +use std::sync::Arc; +use std::time::Duration; + +use axum::Router; +use axum::body::{Body, Bytes}; +use axum::http::header; +use axum::response::{IntoResponse, Response}; +use axum::routing::get; +use futures::stream; +use rand::{RngCore, SeedableRng}; +use tower_http::services::ServeDir; + +use crate::http_server::{self, ServerHandle}; + +/// A loopback HTTP server that serves one named file in paced chunks. +/// +/// Falls back to serving `root` normally (via `ServeDir`) for every other +/// path. Shuts down on drop, like [`crate::loopback_http::LoopbackServer`]. +#[derive(Debug)] +pub struct PacedDownloadServer { + server: ServerHandle, +} + +impl PacedDownloadServer { + /// Bind an ephemeral loopback port and serve `root` (via `ServeDir`) + /// until dropped, except for `GET /`, which streams + /// `contents` in `chunk_size`-byte pieces with `delay` between each. + /// + /// Blocks until the port is bound, matching `LoopbackServer::start`, so + /// [`Self::base_url`] is immediately usable. + pub fn start( + root: &Path, + paced_file: &str, + contents: Vec, + chunk_size: usize, + delay: Duration, + ) -> Self { + let contents = Arc::new(contents); + let route = format!("/{paced_file}"); + let app = Router::new() + .route( + &route, + get(move || std::future::ready(paced_response(contents, chunk_size, delay))), + ) + .fallback_service(ServeDir::new(root)); + Self { + server: http_server::spawn_on_own_thread(app), + } + } + + /// The served root, without a trailing slash — see + /// [`crate::loopback_http::LoopbackServer::base_url`]. + pub fn base_url(&self) -> String { + self.server.base_url() + } +} + +/// High-entropy filler bytes for a paced-fixture payload. +/// +/// A naive multiplicative-hash sequence looked pseudo-random but gzip still +/// compressed it by over 99%, collapsing a paced transfer into a single +/// unpaced chunk. A fixed seed keeps the fixture (and therefore the archive's +/// compressed size) deterministic across runs — though `rand` doesn't +/// guarantee `StdRng`'s algorithm is stable across crate versions, so a +/// future `rand` bump could change these bytes (and the compressed size) +/// even with the seed unchanged. Shared by both the TheRock tarball and +/// ComfyUI source-archive fixtures, which each need enough incompressible +/// bytes to stream in more than one paced chunk. +pub fn deterministic_payload(len: usize) -> Vec { + let mut rng = rand::rngs::StdRng::seed_from_u64(0x9E37_79B9_7F4A_7C15); + let mut buf = vec![0u8; len]; + rng.fill_bytes(&mut buf); + buf +} + +/// Builds a real gzip tarball and returns its bytes. +/// +/// Packages the single top-level directory `build_dir.join(dir_name)` into +/// `build_dir.join(archive_name)`. Shared by the TheRock tarball and ComfyUI +/// source-archive fixtures, which each need a genuine archive for their +/// installer's real `tar` extraction to unpack once the paced download +/// completes. +/// +/// Runs the actual `tar` invocation on a blocking-pool thread +/// (`spawn_blocking`) rather than the calling task's worker thread: building +/// a multi-megabyte archive is not instant, and shelling out synchronously +/// from an async `given` step would otherwise tie up a tokio worker thread +/// for the duration. +pub async fn build_gzip_tarball(build_dir: &Path, archive_name: &str, dir_name: &str) -> Vec { + let build_dir = build_dir.to_path_buf(); + let archive_name = archive_name.to_owned(); + let dir_name = dir_name.to_owned(); + tokio::task::spawn_blocking(move || { + let archive_path = build_dir.join(&archive_name); + let status = std::process::Command::new("tar") + .arg("-czf") + .arg(&archive_path) + .arg("-C") + .arg(&build_dir) + .arg(&dir_name) + .status(); + match status { + Ok(status) if status.success() => {} + Ok(status) => panic!("tar exited with {status} while building {archive_name}"), + Err(error) => panic!("tar is required to build {archive_name}: {error}"), + } + std::fs::read(&archive_path) + .unwrap_or_else(|error| panic!("failed to read built archive {archive_name}: {error}")) + }) + .await + .unwrap_or_else(|error| panic!("build_gzip_tarball blocking task panicked: {error}")) +} + +/// Whether `screen` shows a genuine in-transfer download progress frame: a +/// percentage strictly between 0% and 100%. +/// +/// `download_file_streaming_with_progress` reports once, unthrottled, before +/// the transfer starts (an immediate "(0%)" frame) and once per chunk after — +/// so a caller that only excluded "(100%)" would pass on that very first +/// callback even if pacing never let a real in-transfer frame render. +/// Requiring a percentage strictly between 0 and 100 proves an actual +/// mid-transfer frame was observed. Shared by every paced-download PTY +/// scenario so this heuristic can't drift between per-scenario copies. +pub fn is_intermediate_download_progress_frame(screen: &str) -> bool { + screen.contains("%)") && !screen.contains("(0%)") && !screen.contains("(100%)") +} + +/// Stream `contents` as an HTTP response with an explicit `Content-Length`, +/// in `chunk_size`-byte pieces, sleeping `delay` before every chunk after the +/// first. +fn paced_response(contents: Arc>, chunk_size: usize, delay: Duration) -> Response { + // Fail fast on a fixture bug in debug builds, but degrade to 1 byte per + // chunk rather than panic (dividing the whole transfer into single-byte + // chunks is slow, not wrong) if this ever runs in a release test binary. + debug_assert!(chunk_size > 0, "chunk_size must be at least 1 byte"); + let total_len = contents.len(); + let chunk_size = chunk_size.max(1); + let body = Body::from_stream(stream::unfold(0_usize, move |offset| { + let contents = Arc::clone(&contents); + async move { + if offset >= contents.len() { + return None; + } + if offset > 0 { + tokio::time::sleep(delay).await; + } + let end = (offset + chunk_size).min(contents.len()); + let chunk = Bytes::copy_from_slice(&contents[offset..end]); + Some((Ok::<_, std::io::Error>(chunk), end)) + } + })); + ([(header::CONTENT_LENGTH, total_len.to_string())], body).into_response() +} + +#[cfg(test)] +mod tests { + use std::time::Instant; + + use super::*; + + /// GET `path` from `server`, resolved against its root URL. + async fn get(server: &PacedDownloadServer, path: &str) -> reqwest::Response { + let url = server + .server + .url() + .join(path) + .unwrap_or_else(|e| panic!("{path} is not a valid relative URL: {e}")); + reqwest::get(url) + .await + .unwrap_or_else(|e| panic!("request for {path} failed: {e}")) + } + + #[tokio::test] + async fn serves_the_paced_file_byte_for_byte() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let contents: Vec = (0..10_000).map(|i| (i % 251) as u8).collect(); + let server = PacedDownloadServer::start( + dir.path(), + "archive.tar.gz", + contents.clone(), + 2_000, + Duration::from_millis(1), + ); + + let response = get(&server, "archive.tar.gz").await; + assert!(response.status().is_success()); + assert_eq!( + response + .headers() + .get(reqwest::header::CONTENT_LENGTH) + .and_then(|v| v.to_str().ok()), + Some("10000"), + "Content-Length must report the exact total so the client can compute a percentage" + ); + assert_eq!( + response.bytes().await.expect("no body").as_ref(), + &contents[..] + ); + } + + #[tokio::test] + async fn serves_a_multi_segment_paced_route_byte_for_byte() { + // Both real callers register a multi-segment paced path + // (`archive/comfyui-source.tar.gz`, `tarball/current/`), not the + // single-segment paths the other tests here use — cover that route + // shape directly so a future axum/tower_http routing regression can't + // break both real E2E scenarios while these unit tests keep passing. + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let contents: Vec = (0..5_000).map(|i| (i % 251) as u8).collect(); + let server = PacedDownloadServer::start( + dir.path(), + "tarball/current/archive.tar.gz", + contents.clone(), + 1_000, + Duration::from_millis(1), + ); + + let response = get(&server, "tarball/current/archive.tar.gz").await; + assert!(response.status().is_success()); + assert_eq!( + response.bytes().await.expect("no body").as_ref(), + &contents[..] + ); + } + + #[tokio::test] + async fn pacing_delays_the_response_by_roughly_one_delay_per_chunk_boundary() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + // 3 chunks of 10 bytes: 2 chunk boundaries after the first, so the + // full transfer should take at least 2 delays. + let contents = vec![0_u8; 30]; + let delay = Duration::from_millis(50); + let server = PacedDownloadServer::start(dir.path(), "paced.bin", contents, 10, delay); + + let started = Instant::now(); + let response = get(&server, "paced.bin").await; + let _ = response.bytes().await.expect("no body"); + let elapsed = started.elapsed(); + + assert!( + elapsed >= delay * 2, + "expected the paced response to take at least {:?}, took {elapsed:?} — \ + pacing did not actually delay the chunks", + delay * 2 + ); + } + + #[tokio::test] + async fn falls_back_to_serving_other_files_from_root() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + std::fs::write(dir.path().join("index.html"), b"") + .expect("failed to write fallback file"); + let server = PacedDownloadServer::start( + dir.path(), + "archive.tar.gz", + vec![1, 2, 3], + 1, + Duration::ZERO, + ); + + let response = get(&server, "index.html").await; + assert!(response.status().is_success()); + assert_eq!(response.text().await.expect("no body"), ""); + } + + #[tokio::test] + async fn missing_file_is_a_404() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let server = PacedDownloadServer::start( + dir.path(), + "archive.tar.gz", + vec![1, 2, 3], + 1, + Duration::ZERO, + ); + + let response = get(&server, "absent.zip").await; + assert_eq!(response.status(), reqwest::StatusCode::NOT_FOUND); + } +} diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 57d2e001c..96503f241 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -14,6 +14,7 @@ use cucumber::{World as _, WriterExt as _}; use e2e_cucumber::cli_failure_report; use e2e_cucumber::loopback_http::LoopbackServer; use e2e_cucumber::mock_server::{MockServer, ServiceRecordOptions, write_service_record_with}; +use e2e_cucumber::paced_download::PacedDownloadServer; use tempfile::TempDir; mod e2e { @@ -49,6 +50,10 @@ pub struct E2eWorld { /// Loopback file server used by artifact-prefetch scenarios. Kept on the /// World so it remains alive while the real `rocmd` subprocess downloads. pub artifact_server: Option, + /// Paced download server used by the download-progress-spinner PTY + /// scenario. Kept on the World so it remains alive while the real `rocm` + /// subprocess downloads. + pub paced_download_server: Option, /// Cache-marker destination discovered from `rocmd`'s own JSON report. pub artifact_marker_path: Option, pub endpoint: Option, @@ -220,6 +225,7 @@ impl Default for E2eWorld { Self { mock: None, artifact_server: None, + paced_download_server: None, artifact_marker_path: None, endpoint: None, model_name: None, @@ -542,6 +548,7 @@ impl Drop for E2eWorld { mock.stop(); } self.artifact_server.take(); + self.paced_download_server.take(); // A scenario that ran `rocm serve --managed` left a DETACHED supervisor + // engine process (vLLM / llama-server) that outlives this harness — the // TempDir drop below removes the on-disk record but never kills those diff --git a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs index 6f6dae113..f510256a1 100644 --- a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs @@ -34,14 +34,31 @@ //! (`crates/rocm-dash-tui/src/app/mod.rs`) asserts that envelope is collapsed //! out of the chat. //! +//! `comfyui-04` covers the source-archive download spinner under a real PTY, +//! mirroring `download_progress_pty.feature`'s tarball scenario but for +//! `download_and_extract_source`'s in-process `GzDecoder`/`tar` unpack, which +//! has no separate extraction phase (unlike TheRock's subprocess `tar -xf`, it +//! never renders its own "Extracting…" frame — only the download spinner +//! line matters here). It plants a ready wheel runtime, points +//! `ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE` at a paced loopback server, +//! and gives the fixture's `requirements.txt` only torch-stack entries so +//! `install()`'s dependency filter empties out and skips the `uv` block +//! entirely — this scenario is about the download spinner, not the +//! dependency install already covered above. +//! //! Black-box throughout: the planted registry manifests are plain JSON matching //! the CLI's on-disk schema, not typed imports from the product crates. use std::path::{Path, PathBuf}; +use std::time::Duration; use cucumber::{given, then, when}; +use e2e_cucumber::paced_download::{ + PacedDownloadServer, build_gzip_tarball, deterministic_payload, +}; use crate::E2eWorld; +use crate::e2e::tui_driver::TuiSession; const RUNTIME_KEY: &str = "e2e-comfyui-runtime"; @@ -89,6 +106,24 @@ fn write_shim(path: &Path, body: &str) { } } +/// Writes a fake runtime Python that answers both forms `probe_comfyui`'s +/// post-install GPU check shells out to: `-c