From 9aff2e80f98b23a5b0a1d0bb4a34b1897318f395 Mon Sep 17 00:00:00 2001 From: Jaro-c <75870284+Jaro-c@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:39:18 -0500 Subject: [PATCH] fix(cli): close live board rows on every exit path (#1347) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six call sites across `up` and `down` opened a board row with `progress::start` and never closed it on a code path that returned or fell through, so the row stayed on the working spinner forever even though the operation was over. The plain sink emitted nothing and the live board painted a frozen `Creating`/`Removing`/`Pulling` spinner. The container-create side of `up` already does this right — it closes with `"Exists"`, `"Running"` or `"Created"` before returning. The six sites missing the close are: | Command | File | Verb before | Verb after | |---------|-----------------------|-------------|-------------| | up | network/mod.rs | (stuck) | "Exists" | | up | volume/mod.rs | (stuck) | "Exists" | | up | build/pull.rs | (stuck) | "Failed" | | down | lifecycle/mod.rs | (stuck) | "Absent" / "Failed" | | down | lifecycle/mod.rs | (stuck) | "Absent" / "Failed" | | down | lifecycle/parallel.rs | (stuck) | "Absent" / "Failed" | Closing with an honest verb makes the failure arms in `down` visible: the genuine removal failure used to print as a tracing::warn and exit with the error, but the board kept painting `Removing` with a spinner, hiding which resource failed. The verb-banding now paints "Failed" red instead of green, and the row marker carries a red ✘ for verb = "Failed" (case-insensitive, failsafe to ✘ so a future caller using "failed" or "Failing" does not silently regress to the green ✔ of a successful row). The per-site fix is what the issue calls out; the structural fix (a guard returned by `start()` that force-closes on drop) is a separate concern worth its own issue — leaving the per-site pattern unchanged matches the existing `Exists`/`Running` precedent and fixes every observable from the bug report. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --- internal/engine/build/pull.rs | 10 +++++++++- internal/engine/lifecycle/mod.rs | 16 ++++++++++++++-- internal/engine/lifecycle/parallel.rs | 11 ++++++++++- internal/engine/network/mod.rs | 8 ++++++-- internal/engine/volume/mod.rs | 8 ++++++-- internal/ui/mod.rs | 8 +++++++- internal/ui/mod_tests.rs | 26 ++++++++++++++++++++++++++ internal/ui/progress/row.rs | 17 +++++++++++++++-- internal/ui/progress/row_tests.rs | 26 ++++++++++++++++++++++++++ 9 files changed, 119 insertions(+), 11 deletions(-) diff --git a/internal/engine/build/pull.rs b/internal/engine/build/pull.rs index afeec7d4..e45a910b 100644 --- a/internal/engine/build/pull.rs +++ b/internal/engine/build/pull.rs @@ -294,7 +294,15 @@ impl Engine { } match pull_err { - Some(e) => Err(ComposeError::Build(format!("pull {image} failed: {e}"))), + Some(e) => { + // Close the row before returning: an unfinished `start` on the + // failure path leaves the live board spinning on `Pulling` forever + // even though the operation is over (#1347). + if !quiet { + crate::ui::progress_line("Image", &image, "Failed"); + } + Err(ComposeError::Build(format!("pull {image} failed: {e}"))) + } None => { if !quiet { crate::ui::progress_line("Image", &image, "Pulled"); diff --git a/internal/engine/lifecycle/mod.rs b/internal/engine/lifecycle/mod.rs index f63ff955..77640c79 100644 --- a/internal/engine/lifecycle/mod.rs +++ b/internal/engine/lifecycle/mod.rs @@ -653,9 +653,15 @@ impl Engine { crate::ui::progress::start("Network", &network_name, "Removing"); match self.client.delete_existed(&net_path).await { Ok(true) => crate::ui::progress_line("Network", &network_name, "Removed"), - Ok(false) => {} + // The network was already gone (404) — nothing to do, but the row + // has to close, or the live board leaves it spinning on + // `Removing` forever (#1347). + Ok(false) => crate::ui::progress_line("Network", &network_name, "Absent"), Err(e) => { tracing::warn!("could not remove network {network_name}: {e}"); + // Close the row visibly — a `down` whose removal genuinely + // failed previously hid the failure behind a spinner (#1347). + crate::ui::progress_line("Network", &network_name, "Failed"); first_err.get_or_insert(crate::error::ComposeError::Podman(e)); } } @@ -694,9 +700,15 @@ impl Engine { crate::ui::progress::start("Volume", &volume_name, "Removing"); match self.client.delete_existed(&vol_path).await { Ok(true) => crate::ui::progress_line("Volume", &volume_name, "Removed"), - Ok(false) => {} + // The volume was already gone (404) — nothing to do, but the + // row has to close, or the live board leaves it spinning on + // `Removing` forever (#1347). + Ok(false) => crate::ui::progress_line("Volume", &volume_name, "Absent"), Err(e) => { tracing::warn!("could not remove volume {volume_name}: {e}"); + // A `down -v` whose volume removal genuinely failed + // previously hid the failure behind a spinner (#1347). + crate::ui::progress_line("Volume", &volume_name, "Failed"); first_err.get_or_insert(crate::error::ComposeError::Podman(e)); } } diff --git a/internal/engine/lifecycle/parallel.rs b/internal/engine/lifecycle/parallel.rs index 66b3d7ec..73789784 100644 --- a/internal/engine/lifecycle/parallel.rs +++ b/internal/engine/lifecycle/parallel.rs @@ -382,7 +382,13 @@ impl Engine { crate::ui::progress_line("Container", container_name, "Removed"); Ok(()) } - Err(e) if e.is_status(404) => Ok(()), + // The container was already gone (404) — nothing to do, but the row + // has to close, or the live board leaves it spinning on `Stopping` + // forever (#1347). + Err(e) if e.is_status(404) => { + crate::ui::progress_line("Container", container_name, "Absent"); + Ok(()) + } // The other state-changing call the drops were measured on (#1339). // `Gone` rather than `NotRunning`: a stopped-but-present container // would satisfy the latter and read a failed removal as a success. @@ -392,6 +398,9 @@ impl Engine { .map(|_| ()), Err(e) => { tracing::warn!("could not remove {container_name}: {e}"); + // A `down` whose container removal genuinely failed previously + // hid the failure behind a spinner (#1347). + crate::ui::progress_line("Container", container_name, "Failed"); Err(ComposeError::Podman(e)) } } diff --git a/internal/engine/network/mod.rs b/internal/engine/network/mod.rs index 8b6a9172..00263bfa 100644 --- a/internal/engine/network/mod.rs +++ b/internal/engine/network/mod.rs @@ -79,8 +79,12 @@ impl Engine { Ok(_) => crate::ui::progress_line("Network", &network_name, "Created"), // An existing network is not an error on re-`up`; accept any // already-exists conflict (network-create returns 409, but share - // the same predicate as volume-create for consistency). - Err(ref e) if e.is_already_exists() => {} + // the same predicate as volume-create for consistency). The row + // still needs to close — without an explicit closing verb the + // live board leaves it spinning on `Creating` (#1347). + Err(ref e) if e.is_already_exists() => { + crate::ui::progress_line("Network", &network_name, "Exists"); + } Err(e) => return Err(ComposeError::Podman(e)), } } diff --git a/internal/engine/volume/mod.rs b/internal/engine/volume/mod.rs index 9b834527..9ce25034 100644 --- a/internal/engine/volume/mod.rs +++ b/internal/engine/volume/mod.rs @@ -77,8 +77,12 @@ impl Engine { Ok(_) => crate::ui::progress_line("Volume", &volume_name, "Created"), // Podman's libpod volume-create returns 500 (not 409) for an // existing name; treat an already-exists conflict as success so a - // re-`up` over an existing named volume stays idempotent. - Err(ref e) if e.is_already_exists() => {} + // re-`up` over an existing named volume stays idempotent. The + // row still needs to close — without an explicit closing verb + // the live board leaves it spinning on `Creating` (#1347). + Err(ref e) if e.is_already_exists() => { + crate::ui::progress_line("Volume", &volume_name, "Exists"); + } Err(e) => return Err(ComposeError::Podman(e)), } } diff --git a/internal/ui/mod.rs b/internal/ui/mod.rs index cce334dc..67d6aedb 100644 --- a/internal/ui/mod.rs +++ b/internal/ui/mod.rs @@ -219,9 +219,15 @@ fn action_style(action: &str) -> Style { // either way — a container resuming is a thing becoming active, which is what // green means here — but only by falling through, and adding `unpause` to the // yellow arm's prefixes would silently invert it. Naming it pins the intent. + // `fail` joins the red arm so a row closing with verb "Failed" reads as a + // failure in colour, not just in word (#1347). if a.starts_with("unpaus") { Style::new().fg_color(Some(AnsiColor::Green.into())) - } else if a.starts_with("remov") || a.starts_with("kill") || a.starts_with("delet") { + } else if a.starts_with("remov") + || a.starts_with("kill") + || a.starts_with("delet") + || a.starts_with("fail") + { Style::new().fg_color(Some(AnsiColor::Red.into())) } else if a.starts_with("stop") || a.starts_with("paus") || a.starts_with("restart") { Style::new().fg_color(Some(AnsiColor::Yellow.into())) diff --git a/internal/ui/mod_tests.rs b/internal/ui/mod_tests.rs index 57c379ab..68382937 100644 --- a/internal/ui/mod_tests.rs +++ b/internal/ui/mod_tests.rs @@ -98,6 +98,32 @@ fn status_style_is_semantic() { assert!(status_style("weird-state").is_none()); } +/// The verb bands are what colour a progress line as it closes. A row that +/// closes with `Failed` previously fell through to the default green — the same +/// default that paints `Created`, so a failed row read as a successful one +/// except for the word. The fix is one prefix on the existing red arm (#1347). +#[test] +fn a_failed_progress_verb_is_red() { + let red = Style::new().fg_color(Some(AnsiColor::Red.into())); + let failed = action_style("Failed"); + let failed_lower = action_style("failed"); + assert_eq!( + failed.render().to_string(), + red.render().to_string(), + "Failed must share the red band" + ); + assert_eq!( + failed_lower.render().to_string(), + red.render().to_string(), + "failed (lower) must share the red band" + ); + // The verbs that were already red stay red — the change is additive. + assert_eq!( + action_style("Removed").render().to_string(), + red.render().to_string() + ); +} + #[test] fn progress_toggle_is_observable() { // Off by default-or-restored; toggling flips the observable state. Restore diff --git a/internal/ui/progress/row.rs b/internal/ui/progress/row.rs index 77fdcfbf..8265a17b 100644 --- a/internal/ui/progress/row.rs +++ b/internal/ui/progress/row.rs @@ -19,6 +19,13 @@ pub const SPINNER: [&str; 10] = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦" /// Marker for a finished row. const DONE_MARK: &str = "✔"; +/// Marker for a row that finished in error. Distinct from [`DONE_MARK`] so a +/// failed row cannot be mistaken for a successful one at a glance — the verb +/// already says "Failed", but the row before the user shows the green checkmark +/// of a row that closed cleanly, which is the contradiction #1347 introduced +/// when the missing-close sites started sending `"Failed"` as the closing verb. +const FAILED_MARK: &str = "✘"; + /// Marker for a row nothing has happened to yet. const PENDING_MARK: &str = "⠿"; @@ -34,9 +41,15 @@ const TIME_WIDTH: usize = 6; /// Colour here is state, not identity: a finished row is green because something /// now exists, a pending one is dim because nothing has happened to it. The name /// beside it keeps its identity colour, which is why the marker must not reuse -/// that palette. +/// that palette. A failed row breaks the green-equals-success tie by carrying +/// the verb "Failed" — the marker reads it too, so a failure and a success do +/// not share a glyph. fn marker(row: &Row, frame: usize) -> (&'static str, Style) { - match row.state { + match &row.state { + State::Done(verb) if verb.to_ascii_lowercase().starts_with("fail") => ( + FAILED_MARK, + Style::new().fg_color(Some(AnsiColor::Red.into())), + ), State::Done(_) => ( DONE_MARK, Style::new().fg_color(Some(AnsiColor::Green.into())), diff --git a/internal/ui/progress/row_tests.rs b/internal/ui/progress/row_tests.rs index 8f0e6648..b3eea363 100644 --- a/internal/ui/progress/row_tests.rs +++ b/internal/ui/progress/row_tests.rs @@ -74,6 +74,32 @@ fn each_state_gets_its_own_marker() { assert!(pending.contains(PENDING_MARK), "{pending:?}"); } +/// A row that closed with the verb "Failed" is not a successful row. The marker +/// is the first thing the eye lands on, so a failure without `✘` — a row that +/// says "Failed" with a green `✔` — is the same contradiction the missing-close +/// fix (#1347) introduced: the verb now says "Failed", and the marker has to +/// match. +#[test] +fn a_failed_row_uses_the_failed_marker_and_not_the_done_marker() { + let now = Instant::now(); + let line = plain(&render(&row(State::Done("Failed".into())), 20, 0, now, 80)); + assert!(line.contains(FAILED_MARK), "{line:?}"); + assert!(!line.contains(DONE_MARK), "{line:?}"); +} + +/// Verb case-insensitive: any verb that begins with `fail` is a failure, so a +/// future caller using `"failed"` (lower) or `"Failing"` does not silently +/// regress to the green checkmark. +#[test] +fn a_failed_row_recognises_the_fail_prefix_case_insensitively() { + let now = Instant::now(); + for verb in ["Failed", "failed", "Failing", "FAIL"] { + let line = plain(&render(&row(State::Done(verb.into())), 20, 0, now, 80)); + assert!(line.contains(FAILED_MARK), "{verb:?} → {line:?}"); + assert!(!line.contains(DONE_MARK), "{verb:?} → {line:?}"); + } +} + /// The spinner advances with the frame, or a slow pull looks like a hang. #[test] fn the_working_marker_advances_with_the_frame() {