From 4684efb0ab20971f836a8790fba10652373d967e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nikola=20Milojevi=C4=87?= Date: Wed, 16 Sep 2026 13:35:29 +0200 Subject: [PATCH 1/3] fix: retain shell errors and offer debug recovery Preserve failed command status and stderr after the terminal resumes. Offer the existing debug workflow for the original container when the runtime reports a missing shell, with image selection and guardrail checks. Closes #606 --- docs/debugging.md | 8 +++ docs/features.md | 3 + docs/keys.md | 8 +++ src/app/actions.rs | 67 +++++++++++++++++- src/app/command_failure.rs | 81 +++++++++++++++++++++ src/app/input.rs | 4 ++ src/app/mod.rs | 12 ++++ src/app/mouse.rs | 3 + src/app/overlays.rs | 8 ++- src/app/tests.rs | 139 +++++++++++++++++++++++++++++++++++++ src/main.rs | 13 +--- src/terminal.rs | 49 +++++++++++-- src/terminal/tests.rs | 15 +++- src/ui.rs | 21 +++++- 14 files changed, 409 insertions(+), 22 deletions(-) create mode 100644 src/app/command_failure.rs diff --git a/docs/debugging.md b/docs/debugging.md index e362628e..d5caa954 100644 --- a/docs/debugging.md +++ b/docs/debugging.md @@ -118,6 +118,14 @@ For history that outlives the pod, use [VictoriaLogs](providers.md#log-provider- ## Debug containers and pods +When a pod shell fails, sofka keeps the exit status and the last 16 KiB of +standard error in a dialog until you dismiss it. If the runtime reports a +missing `sh`, press `d` to open the debug image prompt for the same target. +The image is shown before creation. No debug container is created until you +accept the prompt and any required guardrail confirmation. Canceling returns +to the original error. Read-only mode and the `debug` guardrail still apply. +Permission errors and connection failures do not trigger this recovery offer. + `:debug` on a **pod** attaches a temporary ephemeral debug container with `kubectl debug`. sofka prompts for the image (prefilled from `[debug]`). An empty `command` starts an interactive shell (bash if the image has it, else sh), like diff --git a/docs/features.md b/docs/features.md index 064bd30a..70db7de6 100644 --- a/docs/features.md +++ b/docs/features.md @@ -535,6 +535,9 @@ concurrent drains, and full kubectl drain parity are outside this feature. - **PVC explore** (`x` on a PVC, or `:pvc-explore`) - a two-pane browser over a volume's contents, with `s` for a shell inside it. See [PVC explore](#pvc-explore). +- **Shell failure recovery** keeps command errors visible until dismissed. + A missing shell offers the built-in debug image prompt for the same target, + without plugins. Creation requires explicit acceptance and obeys guardrails. - **Ephemeral debug containers** and **node debug pods** (`:debug`). See [Debug containers and pods](debugging.md#debug-containers-and-pods). - **Logs** (`l`) - combined logs for marked pods, per-container on a pod, or aggregated across all matching diff --git a/docs/keys.md b/docs/keys.md index f1aff561..0bb74e03 100644 --- a/docs/keys.md +++ b/docs/keys.md @@ -326,3 +326,11 @@ See [Create a plugin package](plugin-authoring.md). `PgUp` and `PgDn` scroll text that does not fit in the popup. The action keys stay visible. In an input popup, typing moves the view to the cursor. + +## Command failures + +A failed interactive command opens an error dialog. `esc` or `enter` dismisses +it; `PgUp` and `PgDn` scroll the output. If a pod shell failed because `sh` is +missing, `d` opens the debug image prompt for the same pod and container. +Accept the image to start the debug container, subject to the configured +guardrails. Canceling the prompt returns to the original error. diff --git a/src/app/actions.rs b/src/app/actions.rs index f6b15525..a1803f1b 100644 --- a/src/app/actions.rs +++ b/src/app/actions.rs @@ -754,6 +754,28 @@ impl App { if self.deny_readonly() { return; } + let container = container.or_else(|| { + let obj = self.store.get(&format!("{ns}/{pod}"))?; + let containers = obj.data.pointer("/spec/containers")?.as_array()?; + let default = obj + .metadata + .annotations + .as_ref() + .and_then(|a| a.get("kubectl.kubernetes.io/default-container")); + default + .filter(|name| { + containers + .iter() + .any(|c| c["name"].as_str() == Some(name.as_str())) + }) + .cloned() + .or_else(|| containers.first()?.get("name")?.as_str().map(str::to_owned)) + }); + self.shell_target = Some(ShellTarget { + ns: ns.clone(), + pod: pod.clone(), + container: container.clone(), + }); self.note_action("shell", format!("{pod} in {ns}")); let mut argv = self.kubectl_base(); argv.extend(["exec".into(), "-it".into(), "-n".into(), ns, pod]); @@ -788,8 +810,18 @@ impl App { }; let name = obj.metadata.name.clone().unwrap_or_default(); let ns = obj.metadata.namespace.clone().unwrap_or_default(); - // A debug container is a mutation of the pod — let guardrails gate it, - // with no default confirmation (like shell). + self.request_debug_target(ns, name, target); + } + + pub(super) fn request_debug_target( + &mut self, + ns: String, + name: String, + target: Option, + ) { + if self.deny_readonly() { + return; + } let targets = [(name.clone(), ns.clone())]; if self .guard("debug", "pods", &targets, ConfirmLevel::None) @@ -810,6 +842,36 @@ impl App { self.mode = Mode::Prompt; } + pub(super) fn confirm_debug( + &mut self, + ns: String, + pod: String, + target: Option, + image: String, + ) { + if self.deny_readonly() { + self.retain_recovery_error(); + return; + } + let targets = [(pod.clone(), ns.clone())]; + let Some(level) = self.guard("debug", "pods", &targets, ConfirmLevel::None) else { + self.retain_recovery_error(); + return; + }; + let label = format!("Start debug container in {ns}/{pod} with image {image}?"); + self.begin_guarded( + ConfirmAction::Debug { + ns, + pod: pod.clone(), + target, + image, + }, + label, + level, + pod, + ); + } + /// Launch `kubectl debug` for the ephemeral container: suspends the TUI and /// shells out interactively, exactly like exec/attach. The ephemeral /// container persists on the pod (Kubernetes can't remove it) until the pod @@ -821,6 +883,7 @@ impl App { target: Option, image: String, ) { + self.command_failure = None; let tgt = target .as_deref() .map(|c| format!(" --target {c}")) diff --git a/src/app/command_failure.rs b/src/app/command_failure.rs new file mode 100644 index 00000000..954b71b0 --- /dev/null +++ b/src/app/command_failure.rs @@ -0,0 +1,81 @@ +use super::*; + +#[derive(Clone)] +pub struct ShellTarget { + pub ns: String, + pub pod: String, + pub container: Option, +} + +pub struct CommandFailure { + pub message: String, + pub target: Option, +} + +pub(crate) fn missing_shell(message: &str) -> bool { + message.lines().any(|line| { + let line = line.to_ascii_lowercase(); + (line.contains("exec: \"sh\"") || line.contains("exec: \"/bin/sh\"")) + && (line.contains("executable file not found") + || line.contains("no such file or directory")) + }) +} + +impl App { + pub fn handle_command_result( + &mut self, + target: Option, + result: std::io::Result<()>, + ) { + match result { + Ok(()) => { + self.command_failure = None; + self.flash = "Command completed.".into(); + self.flash_err = false; + } + Err(error) => { + let message = crate::ui::strip_ansi_if_present(&error.to_string()).into_owned(); + let target = target.filter(|_| missing_shell(&message)); + self.command_failure = Some(CommandFailure { message, target }); + self.popup_scroll = 0; + self.flash = "Command failed.".into(); + self.flash_err = true; + } + } + } + + pub fn command_failure_visible(&self) -> bool { + self.command_failure.is_some() && !matches!(self.mode, Mode::Prompt | Mode::Confirm) + } + + pub(super) fn retain_recovery_error(&mut self) { + if let Some(failure) = &mut self.command_failure { + failure.message.push_str(&format!("\n\n{}", self.flash)); + } + } + + pub(super) fn key_command_failure(&mut self, key: KeyEvent) { + match key.code { + KeyCode::Esc | KeyCode::Enter => self.command_failure = None, + KeyCode::PageUp | KeyCode::Up => { + self.popup_scroll = self.popup_scroll.saturating_sub(self.popup_viewport.max(1)); + } + KeyCode::PageDown | KeyCode::Down => { + self.popup_scroll = self + .popup_scroll + .saturating_add(self.popup_viewport.max(1)) + .min(self.popup_max_scroll); + } + KeyCode::Char('d') => { + let target = self.command_failure.as_ref().and_then(|f| f.target.clone()); + if let Some(target) = target { + self.request_debug_target(target.ns, target.pod, target.container); + if self.mode != Mode::Prompt { + self.retain_recovery_error(); + } + } + } + _ => {} + } + } +} diff --git a/src/app/input.rs b/src/app/input.rs index ae3da65c..51b5a2fa 100644 --- a/src/app/input.rs +++ b/src/app/input.rs @@ -17,6 +17,10 @@ impl App { // ----- key handling -------------------------------------------------- pub fn handle_key(&mut self, key: KeyEvent) -> Result<()> { + if self.command_failure_visible() { + self.key_command_failure(key); + return Ok(()); + } let action = self.keymap.action(self.key_scope(), &key); if action == Some(Action::PluginActivity) { self.toggle_plugin_activity(); diff --git a/src/app/mod.rs b/src/app/mod.rs index 23a091a8..51551ec6 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -373,6 +373,12 @@ enum ConfirmAction { }, /// Delete the node debugger pods sofka launched this session (`:debug-clean`). CleanupDebuggers, + Debug { + ns: String, + pod: String, + target: Option, + image: String, + }, /// Create a temporary pod that mounts a PVC nothing else mounts, so it can /// be browsed or shelled into. PvcHelper { @@ -2260,6 +2266,8 @@ pub struct App { event_task: Option>, pub pending: Option, + pub shell_target: Option, + pub command_failure: Option, /// Mode to return to when leaving a transient view (logs/detail/diff). return_mode: Mode, /// Row key (ns/name) selected when a transient view was opened, restored on @@ -2546,6 +2554,8 @@ impl App { event_gen: 0, event_task: None, pending: None, + shell_target: None, + command_failure: None, return_mode: Mode::Table, return_selection: None, should_quit: false, @@ -2625,6 +2635,8 @@ impl App { } mod actions; +mod command_failure; +pub use command_failure::{CommandFailure, ShellTarget}; mod adjacent; mod argocd; mod authz; diff --git a/src/app/mouse.rs b/src/app/mouse.rs index 346aa4fc..57d3e606 100644 --- a/src/app/mouse.rs +++ b/src/app/mouse.rs @@ -69,6 +69,9 @@ impl App { /// logs, documents, pickers) without a second navigation code path; /// clicks are table-specific (select a row, sort by a header). pub fn handle_mouse(&mut self, m: MouseEvent) -> Result<()> { + if self.command_failure_visible() { + return Ok(()); + } if self.plugin_activity_visible() { let code = match m.kind { MouseEventKind::ScrollUp => KeyCode::Up, diff --git a/src/app/overlays.rs b/src/app/overlays.rs index 22eb3b59..b7575bc9 100644 --- a/src/app/overlays.rs +++ b/src/app/overlays.rs @@ -146,6 +146,12 @@ impl App { } => { self.do_node_debug(node, image, namespace, profile); } + ConfirmAction::Debug { + ns, + pod, + target, + image, + } => self.do_debug(ns, pod, target, image), ConfirmAction::CleanupDebuggers => { self.do_cleanup_debuggers(); } @@ -341,7 +347,7 @@ impl App { if input.is_empty() { self.flash_warn("no debug image given"); } else { - self.do_debug(ns, pod, target, input); + self.confirm_debug(ns, pod, target, input); } } // The transfer prompts chain: source path first, then the diff --git a/src/app/tests.rs b/src/app/tests.rs index 467dec1c..6174741b 100644 --- a/src/app/tests.rs +++ b/src/app/tests.rs @@ -32959,3 +32959,142 @@ async fn native_describe_key_preserves_secret_token_exception() { app.handle_key(press(KeyCode::Esc)).unwrap(); } } + +fn complete_failed_shell(app: &mut App, error: &str) { + assert!(matches!(app.pending.take(), Some(Suspend::Shell(_)))); + let target = app.shell_target.take(); + app.handle_command_result(target, Err(std::io::Error::other(error.to_owned()))); + app.after_suspend(); +} + +const MISSING_SHELL: &str = + "OCI runtime exec failed: exec: \"sh\": executable file not found in $PATH"; + +#[tokio::test] +async fn missing_shell_keeps_error_and_offers_debug_for_original_container() { + let (mut app, _rx) = app_with_pod(); + apply( + &mut app, + json!({"apiVersion":"v1", "kind":"Pod", + "metadata":{"name":"a", "namespace":"default", "annotations":{ + "kubectl.kubernetes.io/default-container":"worker"}}, + "spec":{"containers":[{"name":"web"},{"name":"worker"}]}}), + ); + app.handle_key(press(KeyCode::Char('s'))).unwrap(); + let Some(Suspend::Shell(argv)) = &app.pending else { + panic!("no shell"); + }; + assert!(argv.windows(2).any(|v| v == ["-c", "worker"])); + complete_failed_shell(&mut app, MISSING_SHELL); + app.expire_flash(); + let mut terminal = ratatui::Terminal::new(ratatui::backend::TestBackend::new(110, 35)).unwrap(); + terminal.draw(|f| crate::ui::draw(f, &mut app)).unwrap(); + let screen = terminal + .backend() + .buffer() + .content + .chunks(110) + .map(|row| row.iter().map(|cell| cell.symbol()).collect::()) + .collect::>() + .join("\n"); + assert!(screen.contains("Start debug container"), "{screen}"); + assert!(app.command_failure_visible()); + app.handle_key(press(KeyCode::Char('x'))).unwrap(); + assert!(app.command_failure_visible()); + app.handle_key(press(KeyCode::Char('d'))).unwrap(); + assert_eq!(app.mode, Mode::Prompt); + assert_eq!(app.prompt_input, app.debug.image); + assert!(app.pending.is_none()); + app.handle_key(press(KeyCode::Esc)).unwrap(); + assert!(app.command_failure_visible()); + assert!( + app.command_failure + .as_ref() + .unwrap() + .message + .contains(MISSING_SHELL) + ); + app.handle_key(press(KeyCode::Char('d'))).unwrap(); + app.handle_key(press(KeyCode::Enter)).unwrap(); + let Some(Suspend::Shell(argv)) = app.pending.take() else { + panic!("no debug command"); + }; + assert!(argv.iter().any(|a| a == "--target=worker")); + assert!(argv.windows(2).any(|v| v == ["default", "a"])); + assert!(app.command_failure.is_none()); +} + +#[tokio::test] +async fn shell_failure_recovery_retains_errors_when_readonly_or_guardrail_blocks() { + for readonly in [true, false] { + let (mut app, _rx) = app_with_pod(); + app.handle_key(press(KeyCode::Char('s'))).unwrap(); + complete_failed_shell(&mut app, MISSING_SHELL); + if readonly { + app.readonly = true; + } else { + app.guardrails = vec![crate::config::Guardrail { + actions: vec!["debug".into()], + deny: true, + ..Default::default() + }]; + } + app.handle_key(press(KeyCode::Char('d'))).unwrap(); + assert!(app.command_failure_visible()); + let message = &app.command_failure.as_ref().unwrap().message; + assert!(message.contains(MISSING_SHELL)); + assert!( + message.contains(if readonly { "read-only" } else { "guardrail" }), + "{message}" + ); + assert!(app.pending.is_none()); + app.handle_key(press(KeyCode::Esc)).unwrap(); + assert!(app.command_failure.is_none()); + } +} + +#[tokio::test] +async fn shell_debug_recovery_obeys_typed_confirmation() { + let (mut app, _rx) = app_with_pod(); + app.guardrails = vec![crate::config::Guardrail { + actions: vec!["debug".into()], + confirmation: Some("type-resource-name".into()), + ..Default::default() + }]; + app.handle_key(press(KeyCode::Char('s'))).unwrap(); + complete_failed_shell(&mut app, MISSING_SHELL); + app.handle_key(press(KeyCode::Char('d'))).unwrap(); + app.handle_key(press(KeyCode::Enter)).unwrap(); + assert_eq!(app.mode, Mode::Prompt); + assert!(app.pending.is_none()); + app.handle_key(press(KeyCode::Char('a'))).unwrap(); + app.handle_key(press(KeyCode::Enter)).unwrap(); + assert!(app.pending.is_some()); +} + +#[tokio::test] +async fn other_shell_failures_do_not_offer_debug_and_success_has_no_dialog() { + for error in [ + "Error from server (Forbidden): pods/exec is forbidden", + "connection refused", + "exit status 127", + "sh: ls: not found", + ] { + let (mut app, _rx) = app_with_pod(); + app.handle_key(press(KeyCode::Char('s'))).unwrap(); + complete_failed_shell(&mut app, error); + assert!(app.command_failure_visible()); + assert!(app.command_failure.as_ref().unwrap().target.is_none()); + app.handle_key(press(KeyCode::Char('d'))).unwrap(); + assert!(app.pending.is_none()); + app.handle_key(press(KeyCode::Enter)).unwrap(); + assert!(app.command_failure.is_none()); + } + let (mut app, _rx) = app_with_pod(); + app.handle_key(press(KeyCode::Char('s'))).unwrap(); + app.pending.take(); + let target = app.shell_target.take(); + app.handle_command_result(target, Ok(())); + assert!(!app.flash_err); + assert!(app.command_failure.is_none()); +} diff --git a/src/main.rs b/src/main.rs index cb99ef74..5d803e71 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1010,16 +1010,9 @@ fn dispatch( /// for it). fn take_suspend(terminal: &mut ratatui::DefaultTerminal, app: &mut App, captured: bool) { if let Some(app::Suspend::Shell(argv)) = app.pending.take() { - match terminal::suspend_and_run(terminal, &argv, captured) { - Ok(()) => { - app.flash = format!("ran: {}", argv.join(" ")); - app.flash_err = false; - } - Err(error) => { - app.flash = format!("cannot run command: {error}"); - app.flash_err = true; - } - } + let target = app.shell_target.take(); + let result = terminal::suspend_and_run(terminal, &argv, captured); + app.handle_command_result(target, result); app.after_suspend(); terminal_title::set(app.terminal_title().as_deref()); } diff --git a/src/terminal.rs b/src/terminal.rs index c5931a56..9f9e27a1 100644 --- a/src/terminal.rs +++ b/src/terminal.rs @@ -1,4 +1,5 @@ -use std::io; +use std::io::{self, Read, Write}; +use std::process::{Command, Stdio}; use crossterm::event::{DisableMouseCapture, EnableMouseCapture}; use crossterm::terminal::{ @@ -23,9 +24,7 @@ pub fn suspend_and_run( } let _ = disable_raw_mode(); let _ = crossterm::execute!(io::stdout(), LeaveAlternateScreen, crossterm::cursor::Show); - let result = std::process::Command::new(&argv[0]) - .args(&argv[1..]) - .status(); + let result = run_command(argv); // Set the modes directly. ratatui::init would install another panic hook. let _ = enable_raw_mode(); let _ = crossterm::execute!(io::stdout(), EnterAlternateScreen); @@ -33,7 +32,47 @@ pub fn suspend_and_run( let _ = crossterm::execute!(io::stdout(), EnableMouseCapture); } let _ = terminal.clear(); - result.map(|_| ()) + result +} + +const ERROR_LIMIT: usize = 16 * 1024; + +fn run_command(argv: &[String]) -> io::Result<()> { + let mut child = Command::new(&argv[0]) + .args(&argv[1..]) + .stderr(Stdio::piped()) + .spawn()?; + let mut stderr = child.stderr.take().expect("piped stderr"); + let reader = std::thread::spawn(move || { + let mut tail = Vec::new(); + let mut buffer = [0; 4096]; + loop { + match stderr.read(&mut buffer) { + Ok(0) => break, + Ok(n) => { + let _ = io::stderr().write_all(&buffer[..n]); + tail.extend_from_slice(&buffer[..n]); + if tail.len() > ERROR_LIMIT { + tail.drain(..tail.len() - ERROR_LIMIT); + } + } + Err(error) if error.kind() == io::ErrorKind::Interrupted => continue, + Err(_) => break, + } + } + tail + }); + let status = child.wait(); + let stderr = reader.join().unwrap_or_default(); + let status = status?; + if status.success() { + Ok(()) + } else { + Err(io::Error::other(format!( + "Command failed ({status}).\n{}", + String::from_utf8_lossy(&stderr).trim() + ))) + } } #[cfg(unix)] diff --git a/src/terminal/tests.rs b/src/terminal/tests.rs index 8f6602f5..96e49e0a 100644 --- a/src/terminal/tests.rs +++ b/src/terminal/tests.rs @@ -150,7 +150,7 @@ impl Drop for Session { #[test] fn terminal_plugin_restores_tui_after_exit_interrupt_quit_and_spawn_error() { - for case in ["exit", "interrupt", "quit", "missing"] { + for case in ["exit", "failure", "interrupt", "quit", "missing"] { let mut session = Session::start(case); for _ in 0..2 { session.expect("TUI_READY"); @@ -202,10 +202,14 @@ fn terminal_plugin_command() { } println!("PLUGIN_READY"); io::stdout().flush().unwrap(); - if case == "exit" { + if matches!(case.as_str(), "exit" | "failure") { let mut answer = String::new(); io::stdin().read_line(&mut answer).unwrap(); assert_eq!(answer.trim_end(), "done"); + if case == "failure" { + eprintln!("exec: \"sh\": executable file not found in $PATH"); + std::process::exit(7); + } } else { std::thread::sleep(Duration::from_secs(30)); panic!("terminal interrupt did not stop the command"); @@ -247,7 +251,12 @@ async fn terminal_plugin_child() { panic!("plugin did not queue a terminal command: {}", app.flash); }; let result = suspend_and_run(&mut terminal, &argv, captured); - assert_eq!(result.is_err(), case == "missing"); + assert_eq!(result.is_err(), case != "exit"); + if case == "failure" { + let error = result.unwrap_err().to_string(); + assert!(error.contains("executable file not found"), "{error}"); + assert!(error.contains("7"), "{error}"); + } app.after_suspend(); assert!(crossterm::terminal::is_raw_mode_enabled().unwrap()); for (signal, previous) in [libc::SIGINT, libc::SIGQUIT].into_iter().zip(&before) { diff --git a/src/ui.rs b/src/ui.rs index a2e32671..c7748f6f 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -114,6 +114,9 @@ pub fn resize( pub fn draw(frame: &mut Frame, app: &mut App) { draw_base(frame, app); draw_plugin_activity(frame, app); + if app.command_failure_visible() { + draw_text_popup(frame, app, false); + } } fn draw_plugin_activity(frame: &mut Frame, app: &mut App) { @@ -2802,6 +2805,10 @@ fn build_help(app: &App, width: usize) -> (Vec>, String) { ":journal · :audit", "session-local log of the mutating actions you've taken", )); + lines.push(bind( + "Command error", + "esc dismiss · PgUp/PgDn scroll · d debug if shell is missing", + )); lines.push(bind( ":debug", "pod: ephemeral debug container · node: privileged debug pod", @@ -4210,7 +4217,19 @@ fn draw_text_popup(frame: &mut Frame, app: &mut App, input: bool) { centered_rect_with_min(50, 20, 56, 7, bounds) }; let color = if input { theme::peach() } else { theme::red() }; - let (label, scope, title, hint) = if input { + let (label, scope, title, hint) = if app.command_failure_visible() { + let failure = app.command_failure.as_ref().unwrap(); + ( + &failure.message, + "confirm", + " Command failed ", + if failure.target.is_some() { + "d Start debug container · esc/enter dismiss · PgUp/PgDn scroll".into() + } else { + "esc/enter dismiss · PgUp/PgDn scroll".into() + }, + ) + } else if input { ( &app.prompt_label, "prompt", From 4760b3c3f5a7bc6eb8b630f23fa31335381018f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nikola=20Milojevi=C4=87?= Date: Wed, 16 Sep 2026 13:45:55 +0200 Subject: [PATCH 2/3] fix: preserve recovery errors and bound terminal output capture Keep the original missing-shell error if debug creation fails. Stop reading stderr shortly after the command exits so a descendant that retains the pipe cannot block terminal restoration. --- src/app/actions.rs | 1 - src/app/command_failure.rs | 18 +++++++++- src/app/tests.rs | 10 ++++++ src/terminal.rs | 70 ++++++++++++++++++++++++++------------ src/terminal/tests.rs | 21 ++++++++++-- 5 files changed, 94 insertions(+), 26 deletions(-) diff --git a/src/app/actions.rs b/src/app/actions.rs index a1803f1b..fbe9eca0 100644 --- a/src/app/actions.rs +++ b/src/app/actions.rs @@ -883,7 +883,6 @@ impl App { target: Option, image: String, ) { - self.command_failure = None; let tgt = target .as_deref() .map(|c| format!(" --target {c}")) diff --git a/src/app/command_failure.rs b/src/app/command_failure.rs index 954b71b0..2a14ef70 100644 --- a/src/app/command_failure.rs +++ b/src/app/command_failure.rs @@ -9,6 +9,7 @@ pub struct ShellTarget { pub struct CommandFailure { pub message: String, + original_message: String, pub target: Option, } @@ -36,7 +37,22 @@ impl App { Err(error) => { let message = crate::ui::strip_ansi_if_present(&error.to_string()).into_owned(); let target = target.filter(|_| missing_shell(&message)); - self.command_failure = Some(CommandFailure { message, target }); + let (message, original_message, target) = match self.command_failure.take() { + Some(previous) => ( + format!( + "{}\n\nRecovery failed:\n{message}", + previous.original_message + ), + previous.original_message, + previous.target, + ), + None => (message.clone(), message, target), + }; + self.command_failure = Some(CommandFailure { + message, + original_message, + target, + }); self.popup_scroll = 0; self.flash = "Command failed.".into(); self.flash_err = true; diff --git a/src/app/tests.rs b/src/app/tests.rs index 6174741b..25c2813f 100644 --- a/src/app/tests.rs +++ b/src/app/tests.rs @@ -33021,6 +33021,16 @@ async fn missing_shell_keeps_error_and_offers_debug_for_original_container() { }; assert!(argv.iter().any(|a| a == "--target=worker")); assert!(argv.windows(2).any(|v| v == ["default", "a"])); + app.handle_command_result( + None, + Err(std::io::Error::other("debug containers are forbidden")), + ); + let message = &app.command_failure.as_ref().unwrap().message; + assert!(message.contains(MISSING_SHELL) && message.contains("debug containers are forbidden")); + app.handle_key(press(KeyCode::Char('d'))).unwrap(); + app.handle_key(press(KeyCode::Enter)).unwrap(); + app.pending.take(); + app.handle_command_result(None, Ok(())); assert!(app.command_failure.is_none()); } diff --git a/src/terminal.rs b/src/terminal.rs index 9f9e27a1..ae144ca4 100644 --- a/src/terminal.rs +++ b/src/terminal.rs @@ -1,5 +1,5 @@ -use std::io::{self, Read, Write}; -use std::process::{Command, Stdio}; +use std::io::{self, Write}; +use std::process::Stdio; use crossterm::event::{DisableMouseCapture, EnableMouseCapture}; use crossterm::terminal::{ @@ -38,39 +38,65 @@ pub fn suspend_and_run( const ERROR_LIMIT: usize = 16 * 1024; fn run_command(argv: &[String]) -> io::Result<()> { - let mut child = Command::new(&argv[0]) + std::thread::scope(|scope| { + scope + .spawn(|| { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build()? + .block_on(run_command_async(argv)) + }) + .join() + .map_err(|_| io::Error::other("Command runner failed."))? + }) +} + +async fn run_command_async(argv: &[String]) -> io::Result<()> { + use tokio::io::AsyncReadExt; + let mut child = tokio::process::Command::new(&argv[0]) .args(&argv[1..]) + .stdin(Stdio::inherit()) + .stdout(Stdio::inherit()) .stderr(Stdio::piped()) .spawn()?; let mut stderr = child.stderr.take().expect("piped stderr"); - let reader = std::thread::spawn(move || { - let mut tail = Vec::new(); - let mut buffer = [0; 4096]; - loop { - match stderr.read(&mut buffer) { - Ok(0) => break, - Ok(n) => { - let _ = io::stderr().write_all(&buffer[..n]); - tail.extend_from_slice(&buffer[..n]); - if tail.len() > ERROR_LIMIT { - tail.drain(..tail.len() - ERROR_LIMIT); + let mut tail = Vec::new(); + let mut buffer = [0; 4096]; + let mut status = None; + let mut closed = false; + let mut deadline = tokio::time::Instant::now(); + loop { + tokio::select! { + result = child.wait(), if status.is_none() => { + status = Some(result?); + // A descendant can retain stderr after the command exits. + deadline = tokio::time::Instant::now() + std::time::Duration::from_millis(100); + } + result = stderr.read(&mut buffer), if !closed => { + match result { + Ok(0) => closed = true, + Ok(n) => { + let _ = io::stderr().write_all(&buffer[..n]); + tail.extend_from_slice(&buffer[..n]); + if tail.len() > ERROR_LIMIT { tail.drain(..tail.len() - ERROR_LIMIT); } } + Err(error) if error.kind() == io::ErrorKind::Interrupted => {}, + Err(_) => closed = true, } - Err(error) if error.kind() == io::ErrorKind::Interrupted => continue, - Err(_) => break, } + _ = tokio::time::sleep_until(deadline), if status.is_some() => break, } - tail - }); - let status = child.wait(); - let stderr = reader.join().unwrap_or_default(); - let status = status?; + if status.is_some() && closed { + break; + } + } + let status = status.expect("command has exited"); if status.success() { Ok(()) } else { Err(io::Error::other(format!( "Command failed ({status}).\n{}", - String::from_utf8_lossy(&stderr).trim() + String::from_utf8_lossy(&tail).trim() ))) } } diff --git a/src/terminal/tests.rs b/src/terminal/tests.rs index 96e49e0a..9b7811a2 100644 --- a/src/terminal/tests.rs +++ b/src/terminal/tests.rs @@ -150,7 +150,14 @@ impl Drop for Session { #[test] fn terminal_plugin_restores_tui_after_exit_interrupt_quit_and_spawn_error() { - for case in ["exit", "failure", "interrupt", "quit", "missing"] { + for case in [ + "exit", + "failure", + "descendant", + "interrupt", + "quit", + "missing", + ] { let mut session = Session::start(case); for _ in 0..2 { session.expect("TUI_READY"); @@ -202,10 +209,20 @@ fn terminal_plugin_command() { } println!("PLUGIN_READY"); io::stdout().flush().unwrap(); - if matches!(case.as_str(), "exit" | "failure") { + if matches!(case.as_str(), "exit" | "failure" | "descendant") { let mut answer = String::new(); io::stdin().read_line(&mut answer).unwrap(); assert_eq!(answer.trim_end(), "done"); + if case == "descendant" { + let _child = Command::new("sleep") + .arg("30") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .spawn() + .unwrap(); + eprintln!("descendant still has stderr open"); + std::process::exit(7); + } if case == "failure" { eprintln!("exec: \"sh\": executable file not found in $PATH"); std::process::exit(7); From 91986cfe7cf77ecd1277de162494cbeeddd90588 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nikola=20Milojevi=C4=87?= Date: Wed, 16 Sep 2026 14:05:24 +0200 Subject: [PATCH 3/3] fix: drain ready errors and bind recovery to its command Read available stderr before the idle deadline, and limit output from descendants that keep writing. Carry the original failure with each recovery command so an unrelated command cannot reuse its error or pod target. --- docs/debugging.md | 5 ++- src/app/actions.rs | 20 ++++++++--- src/app/command_failure.rs | 7 ++-- src/app/mod.rs | 6 ++++ src/app/overlays.rs | 12 +++++-- src/app/tests.rs | 59 ++++++++++++++++++++++++++++--- src/main.rs | 8 +++-- src/terminal.rs | 71 ++++++++++++++++++++++++++++---------- src/terminal/tests.rs | 49 ++++++++++++++++++++++++++ 9 files changed, 203 insertions(+), 34 deletions(-) diff --git a/docs/debugging.md b/docs/debugging.md index d5caa954..63c5a7bf 100644 --- a/docs/debugging.md +++ b/docs/debugging.md @@ -119,7 +119,10 @@ For history that outlives the pod, use [VictoriaLogs](providers.md#log-provider- ## Debug containers and pods When a pod shell fails, sofka keeps the exit status and the last 16 KiB of -standard error in a dialog until you dismiss it. If the runtime reports a +captured standard error in a dialog until you dismiss it. After the command +exits, sofka reads available error output before applying a 100 ms deadline to +an idle pipe. It reads at most 1 MiB after exit so a descendant that keeps +writing cannot block the interface. If the runtime reports a missing `sh`, press `d` to open the debug image prompt for the same target. The image is shown before creation. No debug container is created until you accept the prompt and any required guardrail confirmation. Canceling returns diff --git a/src/app/actions.rs b/src/app/actions.rs index fbe9eca0..984f6448 100644 --- a/src/app/actions.rs +++ b/src/app/actions.rs @@ -810,7 +810,7 @@ impl App { }; let name = obj.metadata.name.clone().unwrap_or_default(); let ns = obj.metadata.namespace.clone().unwrap_or_default(); - self.request_debug_target(ns, name, target); + self.request_debug_target(ns, name, target, None); } pub(super) fn request_debug_target( @@ -818,6 +818,7 @@ impl App { ns: String, name: String, target: Option, + recovery: Option>, ) { if self.deny_readonly() { return; @@ -838,6 +839,7 @@ impl App { ns, pod: name, target, + recovery, }); self.mode = Mode::Prompt; } @@ -848,14 +850,19 @@ impl App { pod: String, target: Option, image: String, + recovery: Option>, ) { if self.deny_readonly() { - self.retain_recovery_error(); + if recovery.is_some() { + self.retain_recovery_error(); + } return; } let targets = [(pod.clone(), ns.clone())]; let Some(level) = self.guard("debug", "pods", &targets, ConfirmLevel::None) else { - self.retain_recovery_error(); + if recovery.is_some() { + self.retain_recovery_error(); + } return; }; let label = format!("Start debug container in {ns}/{pod} with image {image}?"); @@ -865,6 +872,7 @@ impl App { pod: pod.clone(), target, image, + recovery, }, label, level, @@ -882,6 +890,7 @@ impl App { pod: String, target: Option, image: String, + recovery: Option>, ) { let tgt = target .as_deref() @@ -908,7 +917,10 @@ impl App { argv.push("--".into()); argv.extend(self.debug.command.clone()); } - self.pending = Some(Suspend::Shell(argv)); + self.pending = Some(match recovery { + Some(failure) => Suspend::Recovery { argv, failure }, + None => Suspend::Shell(argv), + }); } /// `:debug` on a node — preview and confirm the host access a node debug diff --git a/src/app/command_failure.rs b/src/app/command_failure.rs index 2a14ef70..17a50496 100644 --- a/src/app/command_failure.rs +++ b/src/app/command_failure.rs @@ -7,6 +7,7 @@ pub struct ShellTarget { pub container: Option, } +#[derive(Clone)] pub struct CommandFailure { pub message: String, original_message: String, @@ -27,6 +28,7 @@ impl App { &mut self, target: Option, result: std::io::Result<()>, + recovery: Option>, ) { match result { Ok(()) => { @@ -37,7 +39,7 @@ impl App { Err(error) => { let message = crate::ui::strip_ansi_if_present(&error.to_string()).into_owned(); let target = target.filter(|_| missing_shell(&message)); - let (message, original_message, target) = match self.command_failure.take() { + let (message, original_message, target) = match recovery { Some(previous) => ( format!( "{}\n\nRecovery failed:\n{message}", @@ -85,7 +87,8 @@ impl App { KeyCode::Char('d') => { let target = self.command_failure.as_ref().and_then(|f| f.target.clone()); if let Some(target) = target { - self.request_debug_target(target.ns, target.pod, target.container); + let recovery = self.command_failure.clone().map(Box::new); + self.request_debug_target(target.ns, target.pod, target.container, recovery); if self.mode != Mode::Prompt { self.retain_recovery_error(); } diff --git a/src/app/mod.rs b/src/app/mod.rs index 51551ec6..97b26651 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -239,6 +239,10 @@ pub enum Mode { /// command (exec, edit, port-forward), then resume. pub enum Suspend { Shell(Vec), + Recovery { + argv: Vec, + failure: Box, + }, } /// A `kubectl port-forward` running in the background (not `Suspend::Shell` @@ -378,6 +382,7 @@ enum ConfirmAction { pod: String, target: Option, image: String, + recovery: Option>, }, /// Create a temporary pod that mounts a PVC nothing else mounts, so it can /// be browsed or shelled into. @@ -489,6 +494,7 @@ enum PromptKind { ns: String, pod: String, target: Option, + recovery: Option>, }, /// File-transfer path prompts (`t` on a pod), asked in two steps: the /// source path first (`src` is `None`), then the destination with the diff --git a/src/app/overlays.rs b/src/app/overlays.rs index b7575bc9..803d5cc9 100644 --- a/src/app/overlays.rs +++ b/src/app/overlays.rs @@ -151,7 +151,8 @@ impl App { pod, target, image, - } => self.do_debug(ns, pod, target, image), + recovery, + } => self.do_debug(ns, pod, target, image, recovery), ConfirmAction::CleanupDebuggers => { self.do_cleanup_debuggers(); } @@ -343,11 +344,16 @@ impl App { self.do_set_image(ns, name, plural, container, input); } } - Some(PromptKind::Debug { ns, pod, target }) => { + Some(PromptKind::Debug { + ns, + pod, + target, + recovery, + }) => { if input.is_empty() { self.flash_warn("no debug image given"); } else { - self.confirm_debug(ns, pod, target, input); + self.confirm_debug(ns, pod, target, input, recovery); } } // The transfer prompts chain: source path first, then the diff --git a/src/app/tests.rs b/src/app/tests.rs index 25c2813f..46608d2c 100644 --- a/src/app/tests.rs +++ b/src/app/tests.rs @@ -12136,6 +12136,7 @@ async fn debug_from_container_picker_pins_target() { "a".into(), Some("app".into()), "busybox:latest".into(), + None, ); let Some(Suspend::Shell(argv)) = app.pending.take() else { panic!("expected a debug shell"); @@ -32963,7 +32964,7 @@ async fn native_describe_key_preserves_secret_token_exception() { fn complete_failed_shell(app: &mut App, error: &str) { assert!(matches!(app.pending.take(), Some(Suspend::Shell(_)))); let target = app.shell_target.take(); - app.handle_command_result(target, Err(std::io::Error::other(error.to_owned()))); + app.handle_command_result(target, Err(std::io::Error::other(error.to_owned())), None); app.after_suspend(); } @@ -33016,7 +33017,7 @@ async fn missing_shell_keeps_error_and_offers_debug_for_original_container() { ); app.handle_key(press(KeyCode::Char('d'))).unwrap(); app.handle_key(press(KeyCode::Enter)).unwrap(); - let Some(Suspend::Shell(argv)) = app.pending.take() else { + let Some(Suspend::Recovery { argv, failure }) = app.pending.take() else { panic!("no debug command"); }; assert!(argv.iter().any(|a| a == "--target=worker")); @@ -33024,13 +33025,14 @@ async fn missing_shell_keeps_error_and_offers_debug_for_original_container() { app.handle_command_result( None, Err(std::io::Error::other("debug containers are forbidden")), + Some(failure), ); let message = &app.command_failure.as_ref().unwrap().message; assert!(message.contains(MISSING_SHELL) && message.contains("debug containers are forbidden")); app.handle_key(press(KeyCode::Char('d'))).unwrap(); app.handle_key(press(KeyCode::Enter)).unwrap(); app.pending.take(); - app.handle_command_result(None, Ok(())); + app.handle_command_result(None, Ok(()), None); assert!(app.command_failure.is_none()); } @@ -33104,7 +33106,56 @@ async fn other_shell_failures_do_not_offer_debug_and_success_has_no_dialog() { app.handle_key(press(KeyCode::Char('s'))).unwrap(); app.pending.take(); let target = app.shell_target.take(); - app.handle_command_result(target, Ok(())); + app.handle_command_result(target, Ok(()), None); assert!(!app.flash_err); assert!(app.command_failure.is_none()); } + +#[tokio::test] +async fn unrelated_command_failure_replaces_pending_shell_recovery() { + let (mut app, _rx) = app_with_pod(); + app.handle_key(press(KeyCode::Char('s'))).unwrap(); + complete_failed_shell(&mut app, MISSING_SHELL); + app.handle_key(press(KeyCode::Char('d'))).unwrap(); + assert_eq!(app.mode, Mode::Prompt); + assert!(app.command_failure.is_some()); + + app.handle_command_result( + None, + Err(std::io::Error::other("unrelated command failed")), + None, + ); + app.handle_key(press(KeyCode::Esc)).unwrap(); + let failure = app.command_failure.as_ref().unwrap(); + assert_eq!(failure.message, "unrelated command failed"); + assert!(failure.target.is_none()); + app.handle_key(press(KeyCode::Char('d'))).unwrap(); + assert!(app.pending.is_none()); + assert!(app.command_failure_visible()); +} + +#[tokio::test] +async fn unrelated_missing_shell_uses_its_own_target() { + let (mut app, _rx) = app_with_pod(); + app.handle_key(press(KeyCode::Char('s'))).unwrap(); + complete_failed_shell(&mut app, MISSING_SHELL); + app.handle_key(press(KeyCode::Char('d'))).unwrap(); + app.handle_command_result( + Some(ShellTarget { + ns: "other".into(), + pod: "new-pod".into(), + container: Some("worker".into()), + }), + Err(std::io::Error::other(MISSING_SHELL)), + None, + ); + app.handle_key(press(KeyCode::Esc)).unwrap(); + app.handle_key(press(KeyCode::Char('d'))).unwrap(); + app.handle_key(press(KeyCode::Enter)).unwrap(); + let Some(Suspend::Recovery { argv, failure }) = app.pending.take() else { + panic!("no recovery command"); + }; + assert!(argv.windows(2).any(|v| v == ["other", "new-pod"])); + assert!(argv.iter().any(|a| a == "--target=worker")); + assert_eq!(failure.target.as_ref().unwrap().pod, "new-pod"); +} diff --git a/src/main.rs b/src/main.rs index 5d803e71..1b4bd6ed 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1009,10 +1009,14 @@ fn dispatch( /// its shell is requested from a message, not from the keystroke that asked /// for it). fn take_suspend(terminal: &mut ratatui::DefaultTerminal, app: &mut App, captured: bool) { - if let Some(app::Suspend::Shell(argv)) = app.pending.take() { + if let Some(command) = app.pending.take() { + let (argv, recovery) = match command { + app::Suspend::Shell(argv) => (argv, None), + app::Suspend::Recovery { argv, failure } => (argv, Some(failure)), + }; let target = app.shell_target.take(); let result = terminal::suspend_and_run(terminal, &argv, captured); - app.handle_command_result(target, result); + app.handle_command_result(target, result, recovery); app.after_suspend(); terminal_title::set(app.terminal_title().as_deref()); } diff --git a/src/terminal.rs b/src/terminal.rs index ae144ca4..813e646f 100644 --- a/src/terminal.rs +++ b/src/terminal.rs @@ -62,35 +62,30 @@ async fn run_command_async(argv: &[String]) -> io::Result<()> { let mut stderr = child.stderr.take().expect("piped stderr"); let mut tail = Vec::new(); let mut buffer = [0; 4096]; - let mut status = None; let mut closed = false; - let mut deadline = tokio::time::Instant::now(); - loop { + let status = loop { tokio::select! { - result = child.wait(), if status.is_none() => { - status = Some(result?); - // A descendant can retain stderr after the command exits. - deadline = tokio::time::Instant::now() + std::time::Duration::from_millis(100); - } + biased; + result = child.wait() => break result?, result = stderr.read(&mut buffer), if !closed => { match result { Ok(0) => closed = true, - Ok(n) => { - let _ = io::stderr().write_all(&buffer[..n]); - tail.extend_from_slice(&buffer[..n]); - if tail.len() > ERROR_LIMIT { tail.drain(..tail.len() - ERROR_LIMIT); } - } + Ok(n) => record_stderr(&mut tail, &mut io::stderr(), &buffer[..n]), Err(error) if error.kind() == io::ErrorKind::Interrupted => {}, Err(_) => closed = true, } } - _ = tokio::time::sleep_until(deadline), if status.is_some() => break, - } - if status.is_some() && closed { - break; } + }; + if !closed { + drain_stderr( + &mut stderr, + &mut tail, + &mut io::stderr(), + tokio::time::Instant::now() + std::time::Duration::from_millis(100), + ) + .await; } - let status = status.expect("command has exited"); if status.success() { Ok(()) } else { @@ -101,6 +96,46 @@ async fn run_command_async(argv: &[String]) -> io::Result<()> { } } +fn record_stderr(tail: &mut Vec, output: &mut impl Write, bytes: &[u8]) { + let _ = output.write_all(bytes); + tail.extend_from_slice(bytes); + if tail.len() > ERROR_LIMIT { + tail.drain(..tail.len() - ERROR_LIMIT); + } +} + +async fn drain_stderr( + stderr: &mut (impl tokio::io::AsyncRead + Unpin), + tail: &mut Vec, + output: &mut impl Write, + deadline: tokio::time::Instant, +) { + use tokio::io::AsyncReadExt; + let mut buffer = [0; 4096]; + // Also bound descendants that keep writing after the command exits. + let mut remaining: usize = 1024 * 1024; + while remaining > 0 { + let capacity = remaining.min(buffer.len()); + tokio::select! { + biased; + // Read available bytes before an expired timer. Cooperative task + // budgets must not make a ready pipe appear empty during this drain. + result = tokio::task::unconstrained(stderr.read(&mut buffer[..capacity])) => { + match result { + Ok(0) => break, + Ok(n) => { + record_stderr(tail, output, &buffer[..n]); + remaining -= n; + } + Err(error) if error.kind() == io::ErrorKind::Interrupted => {}, + Err(_) => break, + } + } + _ = tokio::time::sleep_until(deadline) => break, + } + } +} + #[cfg(unix)] struct SignalGuard { saved: Vec<(libc::c_int, libc::sigaction)>, diff --git a/src/terminal/tests.rs b/src/terminal/tests.rs index 9b7811a2..3d1bd362 100644 --- a/src/terminal/tests.rs +++ b/src/terminal/tests.rs @@ -308,3 +308,52 @@ async fn terminal_plugin_child() { ratatui::restore(); println!("ALL_DONE"); } + +#[tokio::test] +async fn stderr_drain_keeps_ready_tail_after_deadline() { + let final_error = b"exec: \"sh\": executable file not found in $PATH\n"; + let mut bytes = vec![b'x'; ERROR_LIMIT * 4]; + bytes.extend_from_slice(final_error); + let mut reader = bytes.as_slice(); + let mut tail = Vec::new(); + let mut output = Vec::new(); + drain_stderr( + &mut reader, + &mut tail, + &mut output, + tokio::time::Instant::now(), + ) + .await; + assert_eq!(output, bytes); + assert_eq!(tail.len(), ERROR_LIMIT); + assert!(tail.ends_with(final_error)); +} + +#[tokio::test] +async fn stderr_drain_stops_for_idle_or_continuously_writing_descendants() { + let (mut reader, _held_open) = tokio::io::duplex(4096); + let mut tail = Vec::new(); + tokio::time::timeout( + Duration::from_secs(1), + drain_stderr( + &mut reader, + &mut tail, + &mut io::sink(), + tokio::time::Instant::now(), + ), + ) + .await + .unwrap(); + assert!(tail.is_empty()); + let mut reader = tokio::io::repeat(b'x'); + let mut output = Vec::new(); + drain_stderr( + &mut reader, + &mut tail, + &mut output, + tokio::time::Instant::now(), + ) + .await; + assert_eq!(output.len(), 1024 * 1024); + assert_eq!(tail.len(), ERROR_LIMIT); +}