e2e: PTY-observed download/extraction spinner coverage, fix truncated progress display

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<String>,
@@ -324,19 +336,20 @@ 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!(
             log,
             "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<Option<ComfyUiManifest>> {
         .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<String> {
+    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<PathBuf> {
@@ -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,

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;
     }

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

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

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

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

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 {

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;