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() {