From e5b6609b9c1b5e0debc267f352566cf40eca0fc0 Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Wed, 19 Aug 2026 15:50:05 -0400 Subject: [PATCH 01/10] fix(tui): make the dashboard immune to stray console output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dashboard rendered to `io::stderr()` — the same descriptor used by `eprintln!`, tracing's default writer, inherited plugin child stderr, and every noisy C library in the process. Sharing that descriptor is why a stray write lands *on top of* the dashboard and never heals: ratatui diffs against its own idea of the screen, so damaged cells already match the buffer it believes is displayed. Converting individual call sites cannot close this. `plugin/runtime.rs` hands spawned plugins `Stdio::inherit()`, the staged llama.cpp runtime is C, and third-party crates print whatever they like. So the fix is at the descriptor layer: - Render to the controlling terminal (`/dev/tty`, `CONOUT$`) instead of fd 2, giving the dashboard a channel nothing else holds. This is why `less`, `fzf`, and `vim` open the tty directly. - With that in place, redirect fd 1 and fd 2 into the dashboard while it owns the screen. A reader thread turns each line into an `OutputEvent`, so stray output becomes a dashboard row instead of screen damage. The original descriptors are restored on exit and on the panic path. - Wire `R` to a one-shot physical clear plus diff invalidation. The status bar has advertised `R Refresh` since the dashboard shipped with nothing behind it; it is the repair for damage capture cannot intercept, such as another process writing straight to the tty. Also converts the three `skippy-server` KV-tier `eprintln!` calls and the embedded-runtime-tracing `eprintln!` to `tracing`, and adds the `skippy_server=warn` directive without which `EnvFilter::from_default_env` (which defaults to ERROR) drops those warnings before the writer sees them — converting them alone would have silenced the diagnostics rather than routed them. `mesh-llm-tui` keeps `#![forbid(unsafe_code)]`: the descriptor plumbing uses `std::io::pipe` and rustix's safe `dup2`/`fcntl` wrappers. Refs #1340 Co-Authored-By: Claude Opus 5 --- Cargo.lock | 2 + .../src/runtime/tracing_writer.rs | 12 +- crates/mesh-llm-tui/Cargo.toml | 5 + .../src/output/console_capture.rs | 288 ++++++++++++++++++ crates/mesh-llm-tui/src/output/dashboard.rs | 10 + crates/mesh-llm-tui/src/output/formatting.rs | 46 ++- crates/mesh-llm-tui/src/output/mod.rs | 2 + .../mesh-llm-tui/src/output/rendering/mod.rs | 5 +- .../mesh-llm-tui/src/output/rendering/tui.rs | 50 ++- crates/mesh-llm-tui/src/output/state.rs | 4 + .../mesh-llm-tui/src/output/terminal_out.rs | 83 +++++ crates/mesh-llm-tui/src/output/tests/mod.rs | 6 +- .../src/output/tests/rendering.rs | 150 +++++++++ crates/skippy-server/Cargo.toml | 1 + 14 files changed, 647 insertions(+), 17 deletions(-) create mode 100644 crates/mesh-llm-tui/src/output/console_capture.rs create mode 100644 crates/mesh-llm-tui/src/output/terminal_out.rs diff --git a/Cargo.lock b/Cargo.lock index abfb73a633..0afbd45f76 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4296,6 +4296,7 @@ dependencies = [ "crossterm 0.28.1", "mesh-llm-events", "ratatui", + "rustix 1.1.4", "serde_json", "tokio", "tracing", @@ -7561,6 +7562,7 @@ dependencies = [ "tokio-stream", "tonic", "tower", + "tracing", "uuid", ] diff --git a/crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs b/crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs index 670de6a287..85e9ab04e3 100644 --- a/crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs +++ b/crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs @@ -301,6 +301,11 @@ pub(super) fn runtime_tracing_subscriber() .with_env_filter( tracing_subscriber::EnvFilter::from_default_env() .add_directive("mesh_inference=info".parse()?) + // Without this, `EnvFilter::from_default_env()` defaults to + // ERROR and every `skippy_server` warning is dropped before it + // reaches the writer — the diagnostics would be silently gone + // rather than routed to the dashboard. + .add_directive("skippy_server=warn".parse()?) .add_directive("nostr_relay_pool=off".parse()?) .add_directive("nostr_sdk=warn".parse()?) .add_directive("noq_proto::connection=warn".parse()?), @@ -318,8 +323,11 @@ pub(super) fn init_runtime_tracing() -> Result<()> { pub(super) fn init_embedded_runtime_tracing() -> Result<()> { let subscriber = runtime_tracing_subscriber()?; if let Err(err) = tracing::subscriber::set_global_default(subscriber) { - eprintln!( - "mesh-llm embedded runtime using existing tracing subscriber; could not install mesh-llm subscriber: {err}" + // The host already had a subscriber, so this warning has somewhere to + // go. Writing it raw would put it straight onto the dashboard frame. + tracing::warn!( + error = %err, + "mesh-llm embedded runtime using existing tracing subscriber" ); } Ok(()) diff --git a/crates/mesh-llm-tui/Cargo.toml b/crates/mesh-llm-tui/Cargo.toml index 7dbf90ced8..d07d485986 100644 --- a/crates/mesh-llm-tui/Cargo.toml +++ b/crates/mesh-llm-tui/Cargo.toml @@ -24,3 +24,8 @@ ratatui = "0.30" serde_json.workspace = true tokio = { version = "1", features = ["macros", "rt", "sync", "time"] } tracing = "0.1" + +[target.'cfg(unix)'.dependencies] +# Safe wrappers for the dup/dup2/pipe plumbing behind console capture. Used in +# preference to raw libc so this crate keeps `#![forbid(unsafe_code)]`. +rustix = { version = "1", features = ["stdio"] } diff --git a/crates/mesh-llm-tui/src/output/console_capture.rs b/crates/mesh-llm-tui/src/output/console_capture.rs new file mode 100644 index 0000000000..b523778880 --- /dev/null +++ b/crates/mesh-llm-tui/src/output/console_capture.rs @@ -0,0 +1,288 @@ +//! Redirects the process's stdout and stderr into the dashboard while it owns +//! the screen. +//! +//! Converting individual `println!`/`eprintln!` call sites cannot close this +//! hole, because the writers are not all ours: `plugin/runtime.rs` hands +//! spawned plugins `Stdio::inherit()`, the staged llama.cpp runtime is C, and +//! third-party crates print whatever they like. All of them write to fd 1 or +//! fd 2, so that is where the interception belongs. +//! +//! While installed, fd 1 and fd 2 point at a pipe. A reader thread turns each +//! line into an `OutputEvent`, so stray output shows up as a dashboard event +//! instead of painting over the frame. On restore the original descriptors are +//! put back and any still-buffered lines are written to the real stderr so +//! nothing is silently swallowed. +//! +//! This is only safe because the dashboard renders to the controlling terminal +//! (see [`super::terminal_out`]) rather than to fd 2; installing capture while +//! the dashboard still rendered to stderr would redirect the dashboard into its +//! own pipe. + +#[cfg(unix)] +pub(in crate::output) use unix::ConsoleCapture; + +#[cfg(not(unix))] +pub(in crate::output) use fallback::ConsoleCapture; + +#[cfg(unix)] +mod unix { + use mesh_llm_events::{OutputEvent, emit_event}; + use rustix::io::fcntl_dupfd_cloexec; + use rustix::stdio::{dup2_stderr, dup2_stdout, stderr, stdout}; + use std::fs::File; + use std::io::{self, BufRead, BufReader, PipeReader, Write}; + use std::os::fd::OwnedFd; + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + + /// Lines longer than this are split rather than buffered without bound, so + /// a child spewing bytes with no newline cannot grow the reader's buffer + /// until the process dies. + const MAX_CAPTURED_LINE_BYTES: usize = 8 * 1024; + + pub(in crate::output) struct ConsoleCapture { + saved_stdout: OwnedFd, + saved_stderr: OwnedFd, + active: Arc, + } + + impl ConsoleCapture { + /// Point fd 1 and fd 2 at a pipe drained into the dashboard. + pub(in crate::output) fn install() -> io::Result { + // Anything already buffered belongs on the real terminal, not in + // the pipe we are about to install. + let _ = io::stdout().flush(); + let _ = io::stderr().flush(); + + // Both pipe ends are close-on-exec. `dup2` clears that flag on the + // descriptor it installs, so children still inherit the redirected + // fd 1/2 while the read end stays private to this process. + let (read_fd, write_fd) = io::pipe()?; + + let saved_stdout = fcntl_dupfd_cloexec(stdout(), 0)?; + let saved_stderr = fcntl_dupfd_cloexec(stderr(), 0)?; + // The reader keeps its own handle on the real stderr so it can + // still deliver lines after the dashboard goes away. + let reader_stderr = fcntl_dupfd_cloexec(stderr(), 0)?; + + dup2_stdout(&write_fd)?; + dup2_stderr(&write_fd)?; + drop(write_fd); + + let active = Arc::new(AtomicBool::new(true)); + spawn_reader(read_fd, reader_stderr, Arc::clone(&active)); + + Ok(Self { + saved_stdout, + saved_stderr, + active, + }) + } + + /// Put the original descriptors back. Safe to call more than once. + pub(in crate::output) fn restore(&mut self) -> io::Result<()> { + if !self.active.swap(false, Ordering::Release) { + return Ok(()); + } + let _ = io::stdout().flush(); + let _ = io::stderr().flush(); + dup2_stdout(&self.saved_stdout)?; + dup2_stderr(&self.saved_stderr)?; + Ok(()) + } + } + + impl Drop for ConsoleCapture { + fn drop(&mut self) { + let _ = self.restore(); + } + } + + /// The reader is deliberately detached rather than joined. A plugin child + /// that inherited the write end keeps the pipe open past restore, so a join + /// could block shutdown indefinitely; the thread instead exits on EOF + /// whenever that arrives, and dies with the process at worst. + fn spawn_reader(read_fd: PipeReader, original_stderr: OwnedFd, active: Arc) { + let _ = std::thread::Builder::new() + .name("mesh-console-capture".to_string()) + .spawn(move || { + let mut passthrough = File::from(original_stderr); + let mut reader = BufReader::new(read_fd); + let mut line = Vec::new(); + loop { + line.clear(); + match read_bounded_line(&mut reader, &mut line) { + Ok(0) | Err(_) => break, + Ok(_) => {} + } + let text = String::from_utf8_lossy(&line) + .trim_end_matches(['\r', '\n']) + .to_string(); + if text.trim().is_empty() { + continue; + } + if active.load(Ordering::Acquire) { + // Failure here means the dashboard sink is gone; fall + // back to the real terminal rather than dropping it. + if emit_event(dashboard_event(text.clone())).is_ok() { + continue; + } + } + let _ = writeln!(passthrough, "{text}"); + } + }); + } + + /// Captured output has no level of its own. Anything that looks like a + /// complaint is surfaced as a warning so it is not lost among info rows; + /// the `stdout` context tells the reader it was intercepted rather than + /// emitted through the normal event path. + pub(super) fn dashboard_event(message: String) -> OutputEvent { + let lowered = message.to_ascii_lowercase(); + let looks_like_a_problem = ["error", "warn", "failed", "panic"] + .iter() + .any(|needle| lowered.contains(needle)); + let context = Some("stdout".to_string()); + if looks_like_a_problem { + OutputEvent::Warning { message, context } + } else { + OutputEvent::Info { message, context } + } + } + + /// Like `read_until(b'\n')`, but refuses to grow past + /// `MAX_CAPTURED_LINE_BYTES` so unterminated output cannot exhaust memory. + pub(super) fn read_bounded_line( + reader: &mut R, + line: &mut Vec, + ) -> io::Result { + let mut total = 0usize; + loop { + let (consumed, done) = { + let available = match reader.fill_buf() { + Ok(buffer) => buffer, + Err(ref err) if err.kind() == io::ErrorKind::Interrupted => continue, + Err(err) => return Err(err), + }; + if available.is_empty() { + return Ok(total); + } + let (chunk, done) = match available.iter().position(|byte| *byte == b'\n') { + Some(index) => (&available[..=index], true), + None => (available, false), + }; + let take = chunk + .len() + .min(MAX_CAPTURED_LINE_BYTES.saturating_sub(total)); + line.extend_from_slice(&chunk[..take]); + total += take; + (chunk.len(), done) + }; + reader.consume(consumed); + if done || total >= MAX_CAPTURED_LINE_BYTES { + return Ok(total.max(1)); + } + } + } +} + +#[cfg(not(unix))] +mod fallback { + use std::io; + + /// Descriptor-level capture is POSIX-specific. On other platforms the + /// dashboard still renders to the controlling terminal, and stray output + /// remains repairable with the `R` key. + pub(in crate::output) struct ConsoleCapture; + + impl ConsoleCapture { + pub(in crate::output) fn install() -> io::Result { + Err(io::Error::new( + io::ErrorKind::Unsupported, + "console capture requires a POSIX platform", + )) + } + + pub(in crate::output) fn restore(&mut self) -> io::Result<()> { + Ok(()) + } + } +} + +#[cfg(all(test, unix))] +mod tests { + use super::unix::{dashboard_event, read_bounded_line}; + use mesh_llm_events::OutputEvent; + use std::io::BufReader; + + fn read_all_lines(input: &[u8]) -> Vec { + let mut reader = BufReader::new(input); + let mut lines = Vec::new(); + loop { + let mut line = Vec::new(); + match read_bounded_line(&mut reader, &mut line) { + Ok(0) | Err(_) => break, + Ok(_) => lines.push(String::from_utf8_lossy(&line).into_owned()), + } + } + lines + } + + #[test] + fn captured_output_splits_on_newlines() { + assert_eq!( + read_all_lines(b"first\nsecond\nthird\n"), + vec!["first\n", "second\n", "third\n"] + ); + } + + #[test] + fn captured_output_keeps_a_trailing_unterminated_line() { + assert_eq!( + read_all_lines(b"done\nno trailing newline"), + vec!["done\n", "no trailing newline"] + ); + } + + #[test] + fn captured_output_without_newlines_cannot_grow_without_bound() { + // A child spewing bytes with no newline must not be buffered forever. + let flood = vec![b'x'; 64 * 1024]; + + let lines = read_all_lines(&flood); + + assert!( + lines.iter().all(|line| line.len() <= 8 * 1024), + "an unterminated flood must be split, not accumulated" + ); + assert_eq!( + lines.iter().map(String::len).sum::(), + flood.len(), + "splitting must not drop bytes" + ); + } + + #[test] + fn captured_output_is_classified_by_what_it_says() { + assert!(matches!( + dashboard_event("llama_model_loader: loaded meta data".to_string()), + OutputEvent::Info { .. } + )); + assert!(matches!( + dashboard_event("ggml_cuda_init: failed to initialise".to_string()), + OutputEvent::Warning { .. } + )); + } + + #[test] + fn captured_output_is_labelled_as_intercepted() { + let OutputEvent::Info { context, .. } = dashboard_event("plain line".to_string()) else { + panic!("expected an info event"); + }; + assert_eq!( + context.as_deref(), + Some("stdout"), + "the dashboard should show that this line was intercepted, not emitted" + ); + } +} diff --git a/crates/mesh-llm-tui/src/output/dashboard.rs b/crates/mesh-llm-tui/src/output/dashboard.rs index 5ff49248b2..4e3ffe80d2 100644 --- a/crates/mesh-llm-tui/src/output/dashboard.rs +++ b/crates/mesh-llm-tui/src/output/dashboard.rs @@ -39,6 +39,7 @@ pub(super) enum DashboardAction { selected_row: Option, }, Resize(DashboardLayoutState), + RequestFullRepaint, } impl DashboardState { @@ -241,6 +242,7 @@ impl DashboardState { pub(in crate::output) fn reduce(&mut self, action: DashboardAction) { match action { + DashboardAction::RequestFullRepaint => self.pending_full_repaint = true, DashboardAction::OutputEvent(event) => self.apply_output_event(&event), DashboardAction::SnapshotUpdated(snapshot) => self.apply_snapshot(&snapshot), DashboardAction::FocusNextPanel => { @@ -1641,6 +1643,14 @@ impl DashboardState { self.reduce(DashboardAction::ToggleEventsFollow); Some(TuiControlFlow::Continue) } + // The status bar has advertised `R Refresh` since the dashboard + // shipped without anything behind it. This is the repair path for + // corruption that capture cannot intercept — anything written to + // the tty by a process that did not inherit our descriptors. + TuiEvent::Key(TuiKeyEvent::Char('r')) if !self.events_filter.editing => { + self.reduce(DashboardAction::RequestFullRepaint); + Some(TuiControlFlow::Continue) + } _ => None, } } diff --git a/crates/mesh-llm-tui/src/output/formatting.rs b/crates/mesh-llm-tui/src/output/formatting.rs index b9e3cc2927..fc962e62a7 100644 --- a/crates/mesh-llm-tui/src/output/formatting.rs +++ b/crates/mesh-llm-tui/src/output/formatting.rs @@ -1,11 +1,14 @@ +use super::console_capture::ConsoleCapture; use super::logging_projection::{projected_json_fields, projected_message, projected_pretty_text}; +use super::terminal_out::TerminalOut; use super::{ ConsoleSessionMode, DashboardAction, DashboardLayoutState, DashboardSnapshot, DashboardSnapshotProvider, DashboardState, LogFormat, ModelProgressStatus, OutputEvent, OutputSink, OutputSinkFuture, PRETTY_TUI_JOIN_TOKEN_PANEL_HEIGHT, PRETTY_TUI_MIN_DASHBOARD_WIDTH, PRETTY_TUI_REDRAW_INTERVAL, PRETTY_TUI_SNAPSHOT_INTERVAL, RuntimeStatus, TuiControlFlow, TuiEvent, TuiTerminal, draw_tui_dashboard_with_terminal, - format_invite_mesh_label, render_dashboard_text, strip_leading_severity_icon, + format_invite_mesh_label, render_dashboard_text, repair_tui_terminal, + strip_leading_severity_icon, }; use chrono::{SecondsFormat, Utc}; use crossterm::{ @@ -629,6 +632,10 @@ pub struct InteractiveDashboardFormatter { pub(in crate::output) tui_entered: Arc, pub(in crate::output) panic_restored: Arc, pub(in crate::output) dirty: bool, + /// Holds fd 1/2 redirected into the dashboard for as long as it owns the + /// screen. `None` when capture could not be installed, in which case stray + /// output still lands on the frame and `R` remains the repair. + pub(in crate::output) console_capture: Option, } impl InteractiveDashboardFormatter { @@ -652,7 +659,17 @@ impl InteractiveDashboardFormatter { self.panic_restored.load(Ordering::Acquire) } + /// Restore the real stdout/stderr. Idempotent, and safe on the panic path. + pub(super) fn release_console_capture(&mut self) { + if let Some(mut capture) = self.console_capture.take() { + let _ = capture.restore(); + } + } + pub(super) fn mark_panic_restored(&mut self) { + // A panic message is about to be written to stderr; it must not go + // into the capture pipe. + self.release_console_capture(); self.terminal = None; self.terminal_active = false; self.dirty = false; @@ -706,12 +723,22 @@ impl InteractiveDashboardFormatter { if self.terminal_active { return Ok(()); } + let out = TerminalOut::open(); + // Capture is only installed when the dashboard has a descriptor of its + // own. Redirecting stderr while still rendering to it would send every + // frame into the capture pipe. + let capture_is_safe = out.is_private(); write_tui_enter()?; self.mark_terminal_escape_written(); - let backend = CrosstermBackend::new(io::stderr()); + let backend = CrosstermBackend::new(out); let mut terminal = Terminal::new(backend).map_err(io::Error::other)?; terminal.hide_cursor().map_err(io::Error::other)?; self.terminal = Some(terminal); + if capture_is_safe { + // A failure here is not fatal: the dashboard works without capture, + // it is just no longer immune to stray writes. + self.console_capture = ConsoleCapture::install().ok(); + } Ok(()) } @@ -728,6 +755,10 @@ impl InteractiveDashboardFormatter { if !self.terminal_active { return Ok(()); } + // Put fd 1/2 back before tearing the screen down, so anything written + // during shutdown reaches the real terminal instead of a pipe whose + // reader is about to stop projecting into a dashboard that is gone. + self.release_console_capture(); if let Some(mut terminal) = self.terminal.take() { terminal.show_cursor().map_err(io::Error::other)?; } @@ -755,6 +786,9 @@ impl InteractiveDashboardFormatter { let terminal = self.terminal.as_mut().ok_or_else(|| { io::Error::other("pretty TUI terminal missing while terminal mode is active") })?; + if std::mem::take(&mut self.state.pending_full_repaint) { + repair_tui_terminal(terminal)?; + } draw_tui_dashboard_with_terminal(terminal, &self.state)?; self.dirty = false; Ok(true) @@ -1517,13 +1551,13 @@ pub(super) fn dashboard_layout_for_terminal_size(columns: u16, rows: u16) -> Das } pub(super) fn write_tui_enter() -> io::Result<()> { - let mut stderr = io::stderr().lock(); - write_tui_enter_to_writer(&mut stderr) + // Must be the same destination the frames go to. Entering the alternate + // screen on one descriptor and leaving it on another strands the terminal. + write_tui_enter_to_writer(&mut TerminalOut::open()) } pub(super) fn write_tui_exit() -> io::Result<()> { - let mut stderr = io::stderr().lock(); - write_tui_exit_to_writer(&mut stderr) + write_tui_exit_to_writer(&mut TerminalOut::open()) } #[cfg(test)] diff --git a/crates/mesh-llm-tui/src/output/mod.rs b/crates/mesh-llm-tui/src/output/mod.rs index 3b69dd9f19..85e96cef5b 100644 --- a/crates/mesh-llm-tui/src/output/mod.rs +++ b/crates/mesh-llm-tui/src/output/mod.rs @@ -15,12 +15,14 @@ use tokio::time::Duration; mod fatal; pub use fatal::{emit_fatal_error, emit_fatal_panic}; +mod console_capture; mod dashboard; mod formatting; mod logging_projection; mod merging; pub(in crate::output) mod rendering; mod state; +mod terminal_out; #[cfg(test)] mod tests; diff --git a/crates/mesh-llm-tui/src/output/rendering/mod.rs b/crates/mesh-llm-tui/src/output/rendering/mod.rs index bb4fac2a37..7a887649a2 100644 --- a/crates/mesh-llm-tui/src/output/rendering/mod.rs +++ b/crates/mesh-llm-tui/src/output/rendering/mod.rs @@ -1,5 +1,6 @@ #[cfg(test)] use super::PRETTY_TUI_LIST_HIGHLIGHT_SYMBOL_WIDTH; +use super::terminal_out::TerminalOut; use super::{ DashboardEventsFilterState, DashboardPanel, DashboardState, MeshEventState, PRETTY_TUI_EVENT_LEVEL_WIDTH, PRETTY_TUI_EVENTS_COLUMN_PERCENT, @@ -25,7 +26,7 @@ use ratatui::{ }; #[cfg(test)] use std::fmt::Write as _; -use std::io; + use tokio::time::Duration; mod events; @@ -160,7 +161,7 @@ pub(super) enum TuiEventRow<'a> { Padding, } -pub(in crate::output) type TuiTerminal = Terminal>; +pub(in crate::output) type TuiTerminal = Terminal>; pub(in crate::output) fn dashboard_status_line( state: &DashboardState, width: u16, diff --git a/crates/mesh-llm-tui/src/output/rendering/tui.rs b/crates/mesh-llm-tui/src/output/rendering/tui.rs index 8e97d8ab7c..0e9cfbb96d 100644 --- a/crates/mesh-llm-tui/src/output/rendering/tui.rs +++ b/crates/mesh-llm-tui/src/output/rendering/tui.rs @@ -5,20 +5,62 @@ use super::{ render_model_progress_loader, render_models_panel, render_process_table, render_processes_panel, render_requests_panel, render_tui_logo, tui_layout, tui_theme, }; -use std::io; +use ratatui::backend::Backend; +use std::{fmt::Display, io}; pub(in crate::output) fn draw_tui_dashboard_with_terminal( terminal: &mut TuiTerminal, state: &DashboardState, ) -> io::Result<()> { - terminal.hide_cursor().map_err(io::Error::other)?; + draw_tui_dashboard_with_backend(terminal, state) +} + +pub(in crate::output) fn draw_tui_dashboard_with_backend( + terminal: &mut ratatui::Terminal, + state: &DashboardState, +) -> io::Result<()> +where + B: Backend, + B::Error: Display, +{ + terminal + .hide_cursor() + .map_err(|error| io::Error::other(error.to_string()))?; terminal .set_cursor_position((0, 0)) - .map_err(io::Error::other)?; + .map_err(|error| io::Error::other(error.to_string()))?; terminal .draw(|frame| render_tui_frame(frame, state)) .map(|_| ()) - .map_err(io::Error::other) + .map_err(|error| io::Error::other(error.to_string())) +} + +/// Repair a physically desynchronized screen. +/// +/// Ratatui diffs against its own idea of the screen, so once something else has +/// written to the terminal the damaged cells are never redrawn — they match the +/// buffer ratatui believes is displayed. Erasing the real screen and discarding +/// both internal buffers forces the next draw to emit every cell. +/// +/// This runs only when the operator asks for it with `R`. Doing it on every +/// frame also works, but repaints the whole screen from blank ~2.6 times a +/// second on an idle dashboard, which reads as a black blink. +pub(in crate::output) fn repair_tui_terminal( + terminal: &mut ratatui::Terminal, +) -> io::Result<()> +where + B: Backend, + B::Error: Display, +{ + terminal + .backend_mut() + .clear() + .map_err(|error| io::Error::other(error.to_string()))?; + // `swap_buffers` resets the inactive buffer before toggling, so two swaps + // reset both buffers and leave the original active index in place. + terminal.swap_buffers(); + terminal.swap_buffers(); + Ok(()) } pub(in crate::output) fn render_tui_frame(frame: &mut Frame, state: &DashboardState) { diff --git a/crates/mesh-llm-tui/src/output/state.rs b/crates/mesh-llm-tui/src/output/state.rs index 26f7ec2f2d..e804c8348b 100644 --- a/crates/mesh-llm-tui/src/output/state.rs +++ b/crates/mesh-llm-tui/src/output/state.rs @@ -627,6 +627,9 @@ pub struct DashboardState { pub(super) startup_milestones: BTreeSet, pub(super) startup_lifecycle: StartupLifecycleState, pub(super) shutdown_in_progress: bool, + /// Set by the `R` key. Consumed by the next render, which repairs the + /// physical screen before drawing. Costs nothing until it is pressed. + pub(super) pending_full_repaint: bool, } impl Default for DashboardState { @@ -670,6 +673,7 @@ impl Default for DashboardState { startup_milestones: BTreeSet::new(), startup_lifecycle: StartupLifecycleState::default(), shutdown_in_progress: false, + pending_full_repaint: false, }; state.apply_layout(panel_layout); state diff --git a/crates/mesh-llm-tui/src/output/terminal_out.rs b/crates/mesh-llm-tui/src/output/terminal_out.rs new file mode 100644 index 0000000000..86609d112a --- /dev/null +++ b/crates/mesh-llm-tui/src/output/terminal_out.rs @@ -0,0 +1,83 @@ +//! The writer the dashboard paints through. +//! +//! The dashboard used to render straight to `io::stderr()`, which is also +//! where `eprintln!`, `tracing`'s default writer, inherited child stderr, and +//! most noisy C libraries write. Sharing that descriptor is what made stray +//! output land *on top of* the dashboard instead of in it, and it is why no +//! amount of converting individual call sites could ever close the hole. +//! +//! Rendering to the controlling terminal instead gives the dashboard a channel +//! nothing else holds a descriptor for, which in turn frees fd 1 and fd 2 to be +//! captured (see [`super::console_capture`]) without fighting over the screen. +//! This is the same reason `less`, `fzf`, and `vim` open the tty directly +//! rather than trusting their standard descriptors. + +use std::fs::{File, OpenOptions}; +use std::io::{self, Write}; + +/// Where terminal control sequences and dashboard frames are written. +/// +/// Falls back to stderr when there is no controlling terminal to open — a +/// piped or redirected session never reaches the dashboard path anyway, but +/// the fallback keeps enter/exit sequences working for the fallback console. +pub(in crate::output) enum TerminalOut { + Tty(File), + Stderr(io::Stderr), +} + +impl TerminalOut { + pub(in crate::output) fn open() -> Self { + match open_controlling_terminal() { + Some(file) => Self::Tty(file), + None => Self::Stderr(io::stderr()), + } + } + + /// True when the dashboard owns a descriptor that is independent of fd 1 + /// and fd 2. Console capture is only safe to install in that case, + /// otherwise redirecting stderr would redirect the dashboard itself. + pub(in crate::output) fn is_private(&self) -> bool { + matches!(self, Self::Tty(_)) + } +} + +#[cfg(unix)] +fn open_controlling_terminal() -> Option { + OpenOptions::new() + .read(true) + .write(true) + .open("/dev/tty") + .ok() +} + +#[cfg(windows)] +fn open_controlling_terminal() -> Option { + // `CONOUT$` is the Windows analogue: it resolves to the active console + // screen buffer regardless of how the standard handles were redirected. + OpenOptions::new() + .read(true) + .write(true) + .open("CONOUT$") + .ok() +} + +#[cfg(not(any(unix, windows)))] +fn open_controlling_terminal() -> Option { + None +} + +impl Write for TerminalOut { + fn write(&mut self, buf: &[u8]) -> io::Result { + match self { + Self::Tty(file) => file.write(buf), + Self::Stderr(stderr) => stderr.write(buf), + } + } + + fn flush(&mut self) -> io::Result<()> { + match self { + Self::Tty(file) => file.flush(), + Self::Stderr(stderr) => stderr.flush(), + } + } +} diff --git a/crates/mesh-llm-tui/src/output/tests/mod.rs b/crates/mesh-llm-tui/src/output/tests/mod.rs index 04ec8cde27..ff647166b0 100644 --- a/crates/mesh-llm-tui/src/output/tests/mod.rs +++ b/crates/mesh-llm-tui/src/output/tests/mod.rs @@ -2,8 +2,8 @@ use super::*; use crate::output::formatting::*; use crate::output::rendering::*; use ratatui::{ - Terminal, - backend::TestBackend, + Terminal, TerminalOptions, Viewport, + backend::{Backend, CrosstermBackend, TestBackend}, buffer::Buffer, layout::{Constraint, Direction, Layout, Rect}, style::Modifier, @@ -13,7 +13,7 @@ use serde_json::Value; use std::{ io::Write as _, sync::{ - Arc, + Arc, Mutex, atomic::{AtomicBool, Ordering}, }, }; diff --git a/crates/mesh-llm-tui/src/output/tests/rendering.rs b/crates/mesh-llm-tui/src/output/tests/rendering.rs index 2dec60ac17..6c923f8935 100644 --- a/crates/mesh-llm-tui/src/output/tests/rendering.rs +++ b/crates/mesh-llm-tui/src/output/tests/rendering.rs @@ -1689,3 +1689,153 @@ pub(super) fn planned_rows_transition_from_not_ready_to_ready_events() { ); assert_eq!(state.loaded_model_rows[0].status, RuntimeStatus::Ready); } + +/// A writer that keeps the bytes a `CrosstermBackend` emits so a test can +/// assert on the escape sequences that actually reach the terminal. +#[derive(Clone, Default)] +struct RecordingWriter { + bytes: Arc>>, +} + +impl std::io::Write for RecordingWriter { + fn write(&mut self, bytes: &[u8]) -> std::io::Result { + self.bytes + .lock() + .expect("recording writer lock should not be poisoned") + .extend_from_slice(bytes); + Ok(bytes.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +fn recording_terminal() -> ( + Terminal>, + Arc>>, +) { + let bytes = Arc::new(Mutex::new(Vec::new())); + let backend = CrosstermBackend::new(RecordingWriter { + bytes: Arc::clone(&bytes), + }); + // A fixed viewport keeps the backend from querying a terminal that does not + // exist under the test harness. + let terminal = Terminal::with_options( + backend, + TerminalOptions { + viewport: Viewport::Fixed(Rect::new(0, 0, 100, 32)), + }, + ) + .expect("recording backend should initialize"); + (terminal, bytes) +} + +fn recorded_contains_full_screen_clear(bytes: &Arc>>) -> bool { + bytes + .lock() + .expect("recording writer lock should not be poisoned") + .windows(b"\x1b[2J".len()) + .any(|window| window == b"\x1b[2J") +} + +#[test] +pub(super) fn tui_refresh_key_requests_a_full_repaint() { + let mut state = DashboardState::default(); + assert!( + !state.pending_full_repaint, + "a fresh dashboard should not be asking for a repair" + ); + + let control = state.apply_tui_event(TuiEvent::Key(TuiKeyEvent::Char('r'))); + + assert!(matches!(control, TuiControlFlow::Continue)); + assert!( + state.pending_full_repaint, + "R is advertised in the status bar and must request a physical repair" + ); +} + +#[test] +pub(super) fn tui_refresh_key_types_into_the_filter_instead_of_repainting() { + let mut state = DashboardState::default(); + state.reduce(DashboardAction::StartEventsFilterEdit); + + state.apply_tui_event(TuiEvent::Key(TuiKeyEvent::Char('r'))); + + assert!( + !state.pending_full_repaint, + "R while editing the filter is filter text, not a repair request" + ); + assert_eq!(state.events_filter.query, "r"); +} + +#[test] +pub(super) fn tui_repair_redraws_cells_an_out_of_band_write_corrupted() { + let state = DashboardState::default(); + let mut terminal = + Terminal::new(TestBackend::new(100, 32)).expect("test backend should initialize"); + draw_tui_dashboard_with_backend(&mut terminal, &state) + .expect("initial dashboard draw should succeed"); + + let area = terminal.backend().buffer().area; + let (x, y) = (0..area.width) + .flat_map(|x| (0..area.height).map(move |y| (x, y))) + .find(|&(x, y)| terminal.backend().buffer()[(x, y)].symbol() != " ") + .expect("dashboard should paint at least one cell"); + let expected_symbol = terminal.backend().buffer()[(x, y)].symbol().to_string(); + + // Exactly what a stray write to the terminal does: the physical screen + // changes while ratatui's idea of it does not. + let stale_cell = ratatui::buffer::Cell::new("X"); + terminal + .backend_mut() + .draw(std::iter::once((x, y, &stale_cell))) + .expect("out-of-band physical write should succeed"); + + // An ordinary redraw cannot heal it: the cell already matches the buffer + // ratatui believes is on screen, so it is diffed away. + draw_tui_dashboard_with_backend(&mut terminal, &state).expect("redraw should succeed"); + assert_eq!( + terminal.backend().buffer()[(x, y)].symbol(), + "X", + "a plain redraw is expected to leave out-of-band damage in place" + ); + + repair_tui_terminal(&mut terminal).expect("repair should succeed"); + draw_tui_dashboard_with_backend(&mut terminal, &state) + .expect("redraw after repair should succeed"); + + assert_eq!( + terminal.backend().buffer()[(x, y)].symbol(), + expected_symbol, + "repair must invalidate ratatui's diff so the next draw repaints every cell" + ); +} + +#[test] +pub(super) fn tui_steady_state_draws_emit_no_full_screen_clear() { + let (mut terminal, bytes) = recording_terminal(); + let state = DashboardState::default(); + + for _ in 0..8 { + draw_tui_dashboard_with_backend(&mut terminal, &state).expect("draw should succeed"); + } + + assert!( + !recorded_contains_full_screen_clear(&bytes), + "clearing the screen on every frame repaints from blank and reads as a black blink" + ); +} + +#[test] +pub(super) fn tui_repair_emits_a_physical_full_screen_clear() { + let (mut terminal, bytes) = recording_terminal(); + + repair_tui_terminal(&mut terminal).expect("repair should succeed"); + + assert!( + recorded_contains_full_screen_clear(&bytes), + "a logical ratatui Clear widget cannot fix backend desync; the repair must erase the real screen" + ); +} diff --git a/crates/skippy-server/Cargo.toml b/crates/skippy-server/Cargo.toml index fb879633b8..9d3cc7c293 100644 --- a/crates/skippy-server/Cargo.toml +++ b/crates/skippy-server/Cargo.toml @@ -44,6 +44,7 @@ libc = "0.2" tokio = { version = "1", features = ["macros", "net", "rt-multi-thread", "sync"] } tokio-stream = "0.1" tonic = "0.14" +tracing = "0.1" uuid = { version = "1", features = ["v4"] } [dev-dependencies] From b58083a23a49ac12645b6be6d7a59780082794e5 Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Wed, 19 Aug 2026 15:54:13 -0400 Subject: [PATCH 02/10] fix(tui): drop process-table columns that do not fit instead of crushing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MODEL/PROCESSES column was laid out with `Constraint::Fill(1)` while its cell text was truncated to a separately computed width that had a minimum of 8. The two never had to agree, and at narrow panel widths they did not: the text was fitted to 8 characters and then rendered into whatever `Fill(1)` had left over, which at a 100-column terminal was a single character. The table showed a `M` header over an `l` cell. Raising the dashboard's minimum width does not fix this — the column is still one character at 100 columns and still truncated at 120 — it only hides the narrow cases while costing every 80-column user the dashboard entirely. Columns are now solved explicitly and rendered with exact `Length` constraints, so the layout is what the text was fitted to. When the panel cannot afford every column it surrenders them from the right (STATE, then PORT) rather than crushing the column that identifies the row. Truncating a header to `STA` is not an improvement over dropping it. Measured across 80/100/120/160/200 columns: every width now renders whole, legible columns, and the model name stays recognizable at all of them. Refs #1340 Co-Authored-By: Claude Opus 5 --- .../src/output/rendering/processes.rs | 189 +++++++++++------- .../src/output/tests/rendering.rs | 64 ++++++ 2 files changed, 181 insertions(+), 72 deletions(-) diff --git a/crates/mesh-llm-tui/src/output/rendering/processes.rs b/crates/mesh-llm-tui/src/output/rendering/processes.rs index 0712cc5dca..3f19eb72a7 100644 --- a/crates/mesh-llm-tui/src/output/rendering/processes.rs +++ b/crates/mesh-llm-tui/src/output/rendering/processes.rs @@ -62,8 +62,9 @@ pub(in crate::output) fn render_process_table( return; } - let [model_width, pid_width, port_width, status_width] = + let widths = llama_process_column_widths_for_rows(inner_area.width, &state.llama_process_rows); + let [model_width, pid_width, port_width, status_width] = widths; let available_rows = usize::from(inner_area.height.saturating_sub(1)); let rows = state .llama_process_rows @@ -74,18 +75,21 @@ pub(in crate::output) fn render_process_table( .map(|(_, row)| { let model = llama_process_model_metadata(row, &state.loaded_model_rows); let model_name = model.map(|model| model.name.as_str()).unwrap_or(&row.name); - Row::new(vec![ - Cell::from(truncate_with_ellipsis( - model_name_without_variant_suffix(model_name), - model_width, - )), - Cell::from(truncate_with_ellipsis( - &format_dashboard_pid((row.pid != 0).then_some(row.pid)), - pid_width, - )), - Cell::from(truncate_with_ellipsis(&row.port.to_string(), port_width)), - process_status_cell(&row.status, status_width), - ]) + Row::new(present_columns( + widths, + [ + Cell::from(truncate_with_ellipsis( + model_name_without_variant_suffix(model_name), + model_width, + )), + Cell::from(truncate_with_ellipsis( + &format_dashboard_pid((row.pid != 0).then_some(row.pid)), + pid_width, + )), + Cell::from(truncate_with_ellipsis(&row.port.to_string(), port_width)), + process_status_cell(&row.status, status_width), + ], + )) }) .collect::>(); let selected_local_index = view @@ -93,25 +97,20 @@ pub(in crate::output) fn render_process_table( .map(|selected| selected.saturating_sub(view.scroll_offset)); let mut table_state = TableState::default(); table_state.select(selected_local_index); - let table = Table::new( - rows, - [ - Constraint::Fill(1), - Constraint::Length(u16::try_from(pid_width).unwrap_or(u16::MAX)), - Constraint::Length(u16::try_from(port_width).unwrap_or(u16::MAX)), - Constraint::Length(u16::try_from(status_width).unwrap_or(u16::MAX)), - ], - ) - .header(process_table_header_row([ - "MODEL".to_string(), - "PID".to_string(), - "PORT".to_string(), - right_align_text("STATE", status_width), - ])) - .column_spacing(1) - .highlight_symbol(if is_focused { "› " } else { " " }) - .highlight_spacing(HighlightSpacing::Always) - .row_highlight_style(process_table_highlight_style(is_focused)); + let table = Table::new(rows, process_table_constraints(widths)) + .header(process_table_header_row(present_columns( + widths, + [ + "MODEL".to_string(), + "PID".to_string(), + "PORT".to_string(), + right_align_text("STATE", status_width), + ], + ))) + .column_spacing(1) + .highlight_symbol(if is_focused { "› " } else { " " }) + .highlight_spacing(HighlightSpacing::Always) + .row_highlight_style(process_table_highlight_style(is_focused)); frame.render_stateful_widget(table, inner_area, &mut table_state); } DashboardPanel::Webserver => { @@ -124,8 +123,9 @@ pub(in crate::output) fn render_process_table( return; } - let [label_width, pid_width, port_width, status_width] = + let widths = webserver_process_column_widths_for_rows(inner_area.width, &state.webserver_rows); + let [label_width, pid_width, port_width, status_width] = widths; let available_rows = usize::from(inner_area.height.saturating_sub(1)); let rows = state .webserver_rows @@ -134,18 +134,21 @@ pub(in crate::output) fn render_process_table( .skip(view.scroll_offset) .take(available_rows) .map(|(_, row)| { - Row::new(vec![ - Cell::from(truncate_with_ellipsis(&row.label, label_width)), - Cell::from(truncate_with_ellipsis( - &format_dashboard_pid(row.pid), - pid_width, - )), - Cell::from(truncate_with_ellipsis( - &format_dashboard_port(row.port), - port_width, - )), - process_status_cell(&row.status, status_width), - ]) + Row::new(present_columns( + widths, + [ + Cell::from(truncate_with_ellipsis(&row.label, label_width)), + Cell::from(truncate_with_ellipsis( + &format_dashboard_pid(row.pid), + pid_width, + )), + Cell::from(truncate_with_ellipsis( + &format_dashboard_port(row.port), + port_width, + )), + process_status_cell(&row.status, status_width), + ], + )) }) .collect::>(); let selected_local_index = view @@ -153,25 +156,20 @@ pub(in crate::output) fn render_process_table( .map(|selected| selected.saturating_sub(view.scroll_offset)); let mut table_state = TableState::default(); table_state.select(selected_local_index); - let table = Table::new( - rows, - [ - Constraint::Fill(1), - Constraint::Length(u16::try_from(pid_width).unwrap_or(u16::MAX)), - Constraint::Length(u16::try_from(port_width).unwrap_or(u16::MAX)), - Constraint::Length(u16::try_from(status_width).unwrap_or(u16::MAX)), - ], - ) - .header(process_table_header_row([ - PRETTY_TUI_WEBSERVER_PROCESS_HEADER_LABEL.to_string(), - "PID".to_string(), - "PORT".to_string(), - right_align_text("STATE", status_width), - ])) - .column_spacing(1) - .highlight_symbol(if is_focused { "› " } else { " " }) - .highlight_spacing(HighlightSpacing::Always) - .row_highlight_style(process_table_highlight_style(is_focused)); + let table = Table::new(rows, process_table_constraints(widths)) + .header(process_table_header_row(present_columns( + widths, + [ + PRETTY_TUI_WEBSERVER_PROCESS_HEADER_LABEL.to_string(), + "PID".to_string(), + "PORT".to_string(), + right_align_text("STATE", status_width), + ], + ))) + .column_spacing(1) + .highlight_symbol(if is_focused { "› " } else { " " }) + .highlight_spacing(HighlightSpacing::Always) + .row_highlight_style(process_table_highlight_style(is_focused)); frame.render_stateful_widget(table, inner_area, &mut table_state); } _ => {} @@ -245,8 +243,8 @@ pub(in crate::output) fn process_table_highlight_style(is_focused: bool) -> Styl } } -pub(in crate::output) fn process_table_header_row( - labels: [String; N], +pub(in crate::output) fn process_table_header_row>( + labels: L, ) -> Row<'static> { let theme = tui_theme(); Row::new(labels.into_iter().map(|label| { @@ -363,18 +361,65 @@ pub(in crate::output) fn webserver_process_column_widths_for_rows( ) } +/// Column widths for a process table, in render order. A width of `0` means +/// the column does not fit and is not rendered at all. +/// +/// Columns are surrendered from the right — STATE first, then PORT — because +/// the text column identifies the row. Crushing it instead produces the +/// one-character `MODEL` column that made narrow dashboards unreadable, and +/// truncating a header to `STA` is not an improvement over dropping it. pub(in crate::output) fn process_column_widths( body_width: u16, min_text_width: usize, pid_width: usize, status_width: usize, ) -> [usize; 4] { - let port_width = 5usize; - let reserved_width = pid_width + port_width + status_width + 3 + 2; - let text_width = usize::from(body_width) - .saturating_sub(reserved_width) - .max(min_text_width); - [text_width, pid_width, port_width, status_width] + const PORT_WIDTH: usize = 5; + // Ratatui always reserves room for the row highlight symbol. + const HIGHLIGHT_WIDTH: usize = 2; + + let available = usize::from(body_width).saturating_sub(HIGHLIGHT_WIDTH); + for (port_width, status_width) in [(PORT_WIDTH, status_width), (PORT_WIDTH, 0), (0, 0)] { + let rendered_columns = 2 + usize::from(port_width > 0) + usize::from(status_width > 0); + let fixed_width = + pid_width + port_width + status_width + rendered_columns.saturating_sub(1); + if available >= fixed_width + min_text_width { + return [available - fixed_width, pid_width, port_width, status_width]; + } + } + + // Narrower than even text + PID. Keep both and let the text truncate; + // there is no useful table left to render otherwise. + [ + available.saturating_sub(pid_width + 1).max(1), + pid_width, + 0, + 0, + ] +} + +/// Constraints for the columns that survived [`process_column_widths`]. +/// +/// These are exact `Length`s rather than a `Fill`, so what ratatui lays out is +/// what the cell contents were truncated to. The two disagreeing was the +/// original defect: the text was fitted to a floored width while the column got +/// whatever `Fill(1)` had left, which could be a single character. +pub(in crate::output) fn process_table_constraints(widths: [usize; 4]) -> Vec { + widths + .into_iter() + .filter(|width| *width > 0) + .map(|width| Constraint::Length(u16::try_from(width).unwrap_or(u16::MAX))) + .collect() +} + +/// Drop the labels/cells whose column was not rendered. +pub(in crate::output) fn present_columns(widths: [usize; 4], values: [T; 4]) -> Vec { + values + .into_iter() + .zip(widths) + .filter(|(_, width)| *width > 0) + .map(|(value, _)| value) + .collect() } pub(in crate::output) fn process_pid_width(pids: I) -> usize diff --git a/crates/mesh-llm-tui/src/output/tests/rendering.rs b/crates/mesh-llm-tui/src/output/tests/rendering.rs index 6c923f8935..5fc962c3ab 100644 --- a/crates/mesh-llm-tui/src/output/tests/rendering.rs +++ b/crates/mesh-llm-tui/src/output/tests/rendering.rs @@ -1839,3 +1839,67 @@ pub(super) fn tui_repair_emits_a_physical_full_screen_clear() { "a logical ratatui Clear widget cannot fix backend desync; the repair must erase the real screen" ); } + +#[test] +pub(super) fn process_columns_keep_every_column_when_there_is_room() { + let [text, pid, port, status] = process_column_widths(80, 8, 5, 8); + + assert!(text >= 8, "the identifying column must never be crushed"); + assert_eq!((pid, port, status), (5, 5, 8)); +} + +#[test] +pub(super) fn process_columns_drop_state_before_crushing_the_model_name() { + // The width at which all four columns stop fitting. + let [text, _pid, port, status] = process_column_widths(26, 8, 5, 8); + + assert_eq!(status, 0, "STATE is the first column to be surrendered"); + assert_eq!(port, 5, "PORT still fits once STATE is gone"); + assert!( + text >= 8, + "the model name keeps its minimum instead of collapsing: got {text}" + ); +} + +#[test] +pub(super) fn process_columns_drop_port_next_and_still_keep_the_name() { + let [text, pid, port, status] = process_column_widths(18, 8, 5, 8); + + assert_eq!((port, status), (0, 0)); + assert_eq!(pid, 5, "PID is the last column to go"); + assert!(text >= 8, "the model name keeps its minimum: got {text}"); +} + +#[test] +pub(super) fn process_table_renders_only_the_columns_that_fit() { + let widths = [9usize, 5, 0, 0]; + + assert_eq!(process_table_constraints(widths).len(), 2); + assert_eq!( + present_columns(widths, ["MODEL", "PID", "PORT", "STATE"]), + vec!["MODEL", "PID"] + ); +} + +#[test] +pub(super) fn tui_narrow_dashboard_keeps_the_model_name_readable() { + let mut state = DashboardState::default(); + state.reduce(DashboardAction::Resize(dashboard_layout_for_terminal_size( + 100, 32, + ))); + state.reduce(DashboardAction::SnapshotUpdated(snapshot_fixture(2, 30))); + + let rendered = render_tui_frame_snapshot(&state, 100, 32); + + // Regression: the MODEL column used to be laid out with `Fill(1)`, which + // ignored the floor the cell text was truncated to and collapsed to a + // single character at this width. + assert!( + rendered.contains("MODEL"), + "the model header must not be truncated away:\n{rendered}" + ); + assert!( + rendered.contains("llama-ser"), + "the model name must stay recognizable at 100 columns:\n{rendered}" + ); +} From 1decd6b562c9d9cfc952ae8e95b2a1cbdd376c0e Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Wed, 19 Aug 2026 16:06:43 -0400 Subject: [PATCH 03/10] fix(tui): surface partial captured lines and strip control bytes Live PTY testing found two gaps in the capture reader that the unit tests could not: a stray write with no trailing newline was intercepted correctly (the frame stayed clean) but then sat in the reader's buffer forever, and raw escape bytes were forwarded into the dashboard verbatim. Both matter. `print!` without a newline and `\r` progress counters are ordinary output, and holding them until the next newline means the operator never sees them. Rendering an unfiltered escape sequence into a dashboard cell would move the cursor and corrupt the very frame capture exists to protect. The reader now polls with a 150 ms idle timeout and flushes whatever partial line is pending, treats `\r` as a line end so progress counters surface, and strips control characters before the text reaches a cell. Refs #1340 Co-Authored-By: Claude Opus 5 --- crates/mesh-llm-tui/Cargo.toml | 2 +- .../src/output/console_capture.rs | 205 ++++++++++-------- 2 files changed, 119 insertions(+), 88 deletions(-) diff --git a/crates/mesh-llm-tui/Cargo.toml b/crates/mesh-llm-tui/Cargo.toml index d07d485986..436efb4c4d 100644 --- a/crates/mesh-llm-tui/Cargo.toml +++ b/crates/mesh-llm-tui/Cargo.toml @@ -28,4 +28,4 @@ tracing = "0.1" [target.'cfg(unix)'.dependencies] # Safe wrappers for the dup/dup2/pipe plumbing behind console capture. Used in # preference to raw libc so this crate keeps `#![forbid(unsafe_code)]`. -rustix = { version = "1", features = ["stdio"] } +rustix = { version = "1", features = ["event", "stdio"] } diff --git a/crates/mesh-llm-tui/src/output/console_capture.rs b/crates/mesh-llm-tui/src/output/console_capture.rs index b523778880..40e10af5d7 100644 --- a/crates/mesh-llm-tui/src/output/console_capture.rs +++ b/crates/mesh-llm-tui/src/output/console_capture.rs @@ -27,13 +27,18 @@ pub(in crate::output) use fallback::ConsoleCapture; #[cfg(unix)] mod unix { use mesh_llm_events::{OutputEvent, emit_event}; + use rustix::event::{PollFd, PollFlags, poll}; use rustix::io::fcntl_dupfd_cloexec; use rustix::stdio::{dup2_stderr, dup2_stdout, stderr, stdout}; use std::fs::File; - use std::io::{self, BufRead, BufReader, PipeReader, Write}; + use std::io::{self, PipeReader, Read, Write}; use std::os::fd::OwnedFd; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; + use std::time::Duration; + + /// How long a partial line may sit unflushed before it is shown anyway. + const IDLE_FLUSH: Duration = Duration::from_millis(150); /// Lines longer than this are split rather than buffered without bound, so /// a child spewing bytes with no newline cannot grow the reader's buffer @@ -102,37 +107,103 @@ mod unix { /// that inherited the write end keeps the pipe open past restore, so a join /// could block shutdown indefinitely; the thread instead exits on EOF /// whenever that arrives, and dies with the process at worst. - fn spawn_reader(read_fd: PipeReader, original_stderr: OwnedFd, active: Arc) { + fn spawn_reader(mut read_fd: PipeReader, original_stderr: OwnedFd, active: Arc) { let _ = std::thread::Builder::new() .name("mesh-console-capture".to_string()) .spawn(move || { let mut passthrough = File::from(original_stderr); - let mut reader = BufReader::new(read_fd); - let mut line = Vec::new(); - loop { - line.clear(); - match read_bounded_line(&mut reader, &mut line) { - Ok(0) | Err(_) => break, - Ok(_) => {} - } - let text = String::from_utf8_lossy(&line) - .trim_end_matches(['\r', '\n']) - .to_string(); - if text.trim().is_empty() { + let mut pending = Vec::new(); + let mut buffer = [0u8; 4096]; + // Wait for data, but not forever: a writer that emitted a + // partial line and then went quiet (`print!`, a `\r` progress + // counter) must still be shown rather than sit in this buffer + // until the next newline arrives. + while let Ok(readable) = wait_for_input(&read_fd, IDLE_FLUSH) { + if !readable { + for line in take_pending_lines(&mut pending, true) { + deliver(line, &active, &mut passthrough); + } continue; } - if active.load(Ordering::Acquire) { - // Failure here means the dashboard sink is gone; fall - // back to the real terminal rather than dropping it. - if emit_event(dashboard_event(text.clone())).is_ok() { - continue; - } + match read_fd.read(&mut buffer) { + Ok(0) | Err(_) => break, + Ok(count) => pending.extend_from_slice(&buffer[..count]), } - let _ = writeln!(passthrough, "{text}"); + // A writer with no line breaks at all must not grow this + // buffer without bound. + let force = pending.len() >= MAX_CAPTURED_LINE_BYTES; + for line in take_pending_lines(&mut pending, force) { + deliver(line, &active, &mut passthrough); + } + } + for line in take_pending_lines(&mut pending, true) { + deliver(line, &active, &mut passthrough); } }); } + fn deliver(text: String, active: &AtomicBool, passthrough: &mut File) { + if text.trim().is_empty() { + return; + } + if active.load(Ordering::Acquire) { + // Failure here means the dashboard sink is gone; fall back to the + // real terminal rather than dropping the line. + if emit_event(dashboard_event(text.clone())).is_ok() { + return; + } + } + let _ = writeln!(passthrough, "{text}"); + } + + /// Split `pending` into displayable lines. + /// + /// `\r` terminates a line as well as `\n` so carriage-return progress + /// counters surface instead of accumulating. When `flush_remainder` is set + /// the trailing partial line is taken too. + pub(super) fn take_pending_lines(pending: &mut Vec, flush_remainder: bool) -> Vec { + let mut lines = Vec::new(); + while let Some(index) = pending + .iter() + .position(|byte| *byte == b'\n' || *byte == b'\r') + { + let mut line: Vec = pending.drain(..=index).collect(); + line.pop(); + lines.push(decode(&line)); + } + if flush_remainder && !pending.is_empty() { + let remainder = std::mem::take(pending); + lines.push(decode(&remainder)); + } + lines + } + + fn decode(line: &[u8]) -> String { + // Captured bytes are arbitrary. A raw escape sequence rendered into a + // dashboard cell would move the cursor and corrupt the very frame this + // exists to protect, so control characters are stripped here. + String::from_utf8_lossy(line) + .chars() + .filter(|character| !character.is_control()) + .collect::() + .trim_end() + .to_string() + } + + /// Block until the pipe has data or `timeout` elapses. `Ok(false)` is a + /// timeout, `Err` means the descriptor is unusable and the reader stops. + fn wait_for_input(read_fd: &PipeReader, timeout: Duration) -> io::Result { + let mut fds = [PollFd::new(read_fd, PollFlags::IN)]; + loop { + match poll(&mut fds, Some(&timeout.try_into().unwrap_or_default())) { + Ok(0) => return Ok(false), + Ok(_) => return Ok(true), + Err(rustix::io::Errno::INTR) => continue, + Err(err) => return Err(err.into()), + } + } + } + /// Captured output has no level of its own. Anything that looks like a /// complaint is surfaced as a warning so it is not lost among info rows; /// the `stdout` context tells the reader it was intercepted rather than @@ -149,41 +220,6 @@ mod unix { OutputEvent::Info { message, context } } } - - /// Like `read_until(b'\n')`, but refuses to grow past - /// `MAX_CAPTURED_LINE_BYTES` so unterminated output cannot exhaust memory. - pub(super) fn read_bounded_line( - reader: &mut R, - line: &mut Vec, - ) -> io::Result { - let mut total = 0usize; - loop { - let (consumed, done) = { - let available = match reader.fill_buf() { - Ok(buffer) => buffer, - Err(ref err) if err.kind() == io::ErrorKind::Interrupted => continue, - Err(err) => return Err(err), - }; - if available.is_empty() { - return Ok(total); - } - let (chunk, done) = match available.iter().position(|byte| *byte == b'\n') { - Some(index) => (&available[..=index], true), - None => (available, false), - }; - let take = chunk - .len() - .min(MAX_CAPTURED_LINE_BYTES.saturating_sub(total)); - line.extend_from_slice(&chunk[..take]); - total += take; - (chunk.len(), done) - }; - reader.consume(consumed); - if done || total >= MAX_CAPTURED_LINE_BYTES { - return Ok(total.max(1)); - } - } - } } #[cfg(not(unix))] @@ -211,54 +247,49 @@ mod fallback { #[cfg(all(test, unix))] mod tests { - use super::unix::{dashboard_event, read_bounded_line}; + use super::unix::{dashboard_event, take_pending_lines}; use mesh_llm_events::OutputEvent; - use std::io::BufReader; - fn read_all_lines(input: &[u8]) -> Vec { - let mut reader = BufReader::new(input); - let mut lines = Vec::new(); - loop { - let mut line = Vec::new(); - match read_bounded_line(&mut reader, &mut line) { - Ok(0) | Err(_) => break, - Ok(_) => lines.push(String::from_utf8_lossy(&line).into_owned()), - } - } - lines + fn split(input: &[u8], flush_remainder: bool) -> Vec { + let mut pending = input.to_vec(); + take_pending_lines(&mut pending, flush_remainder) } #[test] fn captured_output_splits_on_newlines() { assert_eq!( - read_all_lines(b"first\nsecond\nthird\n"), - vec!["first\n", "second\n", "third\n"] + split(b"first\nsecond\nthird\n", false), + vec!["first", "second", "third"] ); } #[test] - fn captured_output_keeps_a_trailing_unterminated_line() { + fn captured_output_holds_a_partial_line_until_it_is_flushed() { + // Mid-write: the rest of the line may still be coming. + assert_eq!(split(b"done\npartial", false), vec!["done"]); + // The writer went quiet, so show it rather than wait forever. This is + // the `print!`-with-no-newline case, which otherwise never surfaces. + assert_eq!(split(b"done\npartial", true), vec!["done", "partial"]); + } + + #[test] + fn captured_output_treats_carriage_returns_as_line_ends() { + // llama.cpp-style progress counters overwrite one line with `\r`. assert_eq!( - read_all_lines(b"done\nno trailing newline"), - vec!["done\n", "no trailing newline"] + split(b"loading 10%\rloading 20%\r", false), + vec!["loading 10%", "loading 20%"] ); } #[test] - fn captured_output_without_newlines_cannot_grow_without_bound() { - // A child spewing bytes with no newline must not be buffered forever. - let flood = vec![b'x'; 64 * 1024]; - - let lines = read_all_lines(&flood); - - assert!( - lines.iter().all(|line| line.len() <= 8 * 1024), - "an unterminated flood must be split, not accumulated" - ); + fn captured_output_strips_control_characters() { + // The whole point is to keep stray bytes off the frame. An escape + // sequence rendered into a dashboard cell would move the cursor and + // corrupt the frame this exists to protect. assert_eq!( - lines.iter().map(String::len).sum::(), - flood.len(), - "splitting must not drop bytes" + split(b"\x1b[10;5HXXXX\n", false), + vec!["[10;5HXXXX"], + "escape bytes must not survive into a rendered cell" ); } From b2252dd611a3e2bab3df0099be90254b7104fcb7 Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Wed, 19 Aug 2026 16:08:50 -0400 Subject: [PATCH 04/10] docs: record the terminal dashboard integrity invariants Refs #1340 Co-Authored-By: Claude Opus 5 --- AGENTS.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index d36b270368..463de67281 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -475,6 +475,33 @@ The patched llama.cpp staged runtime has its own ABI version, tracked in `skippy For changes in `crates/mesh-llm-ui/`, use components and compose interfaces consistently with shadcn/ui patterns. Prefer extending existing primitives in `src/components/ui/` over ad-hoc markup. +### Terminal dashboard integrity + +The dashboard renders to the **controlling terminal** (`/dev/tty`, `CONOUT$`), +never to fd 1 or fd 2, and while it owns the screen those two descriptors are +redirected into it (`crates/mesh-llm-tui/src/output/console_capture.rs`). Stray +`println!`/`eprintln!`, inherited child stderr, and native llama.cpp output +therefore arrive as dashboard events instead of painting over the frame. + +Consequences worth knowing before changing this area: + +- Do not point the TUI backend, or the enter/exit escape writers, at + `io::stderr()`. Capture would then redirect the dashboard into its own pipe. +- Anything restoring the terminal (panic hooks, emergency writers) must release + the capture *before* writing, or the message goes into the pipe. +- Capture cannot intercept a write to the tty by a process that did not inherit + our descriptors. `R` is the repair for that case: a physical clear plus a + ratatui diff invalidation. A logical `Clear` widget does not fix backend + desynchronization. +- Do not perform that clear on every draw. It repaints from blank and reads as + a black blink; measured on an idle dashboard it was ~2.6 full repaints/s. + +Prefer `OutputEvent` or `tracing` in runtime code regardless — captured lines +have no level and are shown with a `stdout` context. When adding a `tracing` +target that must reach the dashboard, add a directive for it in +`runtime_tracing_subscriber`: `EnvFilter::from_default_env()` defaults to ERROR, +so an unlisted target's `warn!` is dropped before the writer sees it. + ## Testing Read `docs/design/TESTING.md` before running tests. It has all test scenarios, remote deploy instructions, and cleanup commands. From 5a7965eb34a9e5a7d0d37c0702d5fe15bde42f13 Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Wed, 19 Aug 2026 16:17:33 -0400 Subject: [PATCH 05/10] fix(xtask): drop the four converted eprintln! sites from the ratchet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quality / CI contracts and consistency failed on this branch: the console-print ratchet still approved four eprintln! occurrences that this PR converted to tracing — crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs:321 and crates/skippy-server/src/kv_integration/config.rs:110/120/152 — so the checker reported them as "approved occurrence is missing or was replaced". Regenerated with `cargo run -p xtask -- repo-consistency no-console-print --regen`. The diff is deletions only: 1044 legacy hits across 115 files becomes 1040 across 113. No entry was added and no line number moved, so the ratchet strictly tightened. Refs #1340 Co-authored-by: Nick DiZazzo Signed-off-by: Nick DiZazzo --- tools/xtask/data/console_print_allowlist.json | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tools/xtask/data/console_print_allowlist.json b/tools/xtask/data/console_print_allowlist.json index f1b4f01594..946f013cc3 100644 --- a/tools/xtask/data/console_print_allowlist.json +++ b/tools/xtask/data/console_print_allowlist.json @@ -1901,12 +1901,6 @@ "macro_name": "eprintln!" } ], - "crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs": [ - { - "line": 321, - "macro_name": "eprintln!" - } - ], "crates/mesh-llm-identity/src/keychain.rs": [ { "line": 236, From 2921d73187ddd92c36bda03c5d7f0d162a04e1c4 Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Wed, 19 Aug 2026 16:21:40 -0400 Subject: [PATCH 06/10] fix(tui): refuse to install console capture without a live reader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `spawn_reader` discarded the `thread::Builder::spawn` result, so a failed spawn still left fd 1 and fd 2 pointing at a pipe with nothing draining it. The failure mode is the worst kind: everything works until the 64 KiB pipe buffer fills, and then every write to stdout or stderr — in this process and in every child that inherited those descriptors — blocks forever, while the dashboard keeps painting as if nothing is wrong. The spawn result is now propagated and `install` puts the saved descriptors back before returning the error. Capture is optional at the call site (`enter_terminal` treats a failure as "no capture"), so the dashboard still comes up — just without interception. Also stop dropping tabs from captured lines. `char::is_control` counts `\t`, and llama.cpp's loader lines are tab-separated, so stripping it ran two columns together; it degrades to a space instead. Raised by CodeRabbit on #1382. Refs #1340 Co-authored-by: Nick DiZazzo Signed-off-by: Nick DiZazzo --- .../src/output/console_capture.rs | 39 ++++++++++++++++--- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/crates/mesh-llm-tui/src/output/console_capture.rs b/crates/mesh-llm-tui/src/output/console_capture.rs index 40e10af5d7..8c074fecd5 100644 --- a/crates/mesh-llm-tui/src/output/console_capture.rs +++ b/crates/mesh-llm-tui/src/output/console_capture.rs @@ -75,7 +75,14 @@ mod unix { drop(write_fd); let active = Arc::new(AtomicBool::new(true)); - spawn_reader(read_fd, reader_stderr, Arc::clone(&active)); + if let Err(error) = spawn_reader(read_fd, reader_stderr, Arc::clone(&active)) { + // With no reader, nothing drains the pipe: the process would + // block forever on the write that fills the pipe buffer. Put + // the real descriptors back before giving up. + let _ = dup2_stdout(&saved_stdout); + let _ = dup2_stderr(&saved_stderr); + return Err(error); + } Ok(Self { saved_stdout, @@ -107,8 +114,12 @@ mod unix { /// that inherited the write end keeps the pipe open past restore, so a join /// could block shutdown indefinitely; the thread instead exits on EOF /// whenever that arrives, and dies with the process at worst. - fn spawn_reader(mut read_fd: PipeReader, original_stderr: OwnedFd, active: Arc) { - let _ = std::thread::Builder::new() + fn spawn_reader( + mut read_fd: PipeReader, + original_stderr: OwnedFd, + active: Arc, + ) -> io::Result<()> { + std::thread::Builder::new() .name("mesh-console-capture".to_string()) .spawn(move || { let mut passthrough = File::from(original_stderr); @@ -139,7 +150,8 @@ mod unix { for line in take_pending_lines(&mut pending, true) { deliver(line, &active, &mut passthrough); } - }); + }) + .map(|_| ()) } fn deliver(text: String, active: &AtomicBool, passthrough: &mut File) { @@ -184,7 +196,14 @@ mod unix { // exists to protect, so control characters are stripped here. String::from_utf8_lossy(line) .chars() - .filter(|character| !character.is_control()) + .filter_map(|character| match character { + // A tab is alignment, not damage — llama.cpp's loader lines are + // full of them. Dropping it would run two columns together, so + // it degrades to a space instead. + '\t' => Some(' '), + character if character.is_control() => None, + character => Some(character), + }) .collect::() .trim_end() .to_string() @@ -293,6 +312,16 @@ mod tests { ); } + #[test] + fn captured_output_keeps_tabs_as_spaces() { + // llama.cpp separates its loader columns with tabs. Stripping them as + // control characters would run the columns together. + assert_eq!( + split(b"llm_load_print_meta: n_ctx\t= 4096\n", false), + vec!["llm_load_print_meta: n_ctx = 4096"] + ); + } + #[test] fn captured_output_is_classified_by_what_it_says() { assert!(matches!( From b0ac772eb52eec7a9e9de66bb636097819d6d64c Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Wed, 19 Aug 2026 16:21:48 -0400 Subject: [PATCH 07/10] fix(tui): make the R repair match the key the status bar prints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways the repair could fail to repair. The handler matched only `Char('r')`, but the status bar reads `R Refresh` and Shift+R arrives as `Char('R')` — pressing the advertised key did nothing. It now accepts either, with the existing guard that keeps both as filter text while the events filter is being edited. `render_if_dirty` also cleared `pending_full_repaint` with `mem::take` before the fallible repair ran. If the erase failed, the request was gone and the next dirty render was an ordinary diff against a screen ratatui still believed was intact — so the damage survived a key press the operator had already made. The flag is now cleared only after `repair_tui_terminal` succeeds, and the propagated error leaves `dirty` set so the next render retries. Raised by CodeRabbit on #1382. Refs #1340 Co-authored-by: Nick DiZazzo Signed-off-by: Nick DiZazzo --- crates/mesh-llm-tui/src/output/dashboard.rs | 2 +- crates/mesh-llm-tui/src/output/formatting.rs | 7 ++++++- crates/mesh-llm-tui/src/output/tests/rendering.rs | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/crates/mesh-llm-tui/src/output/dashboard.rs b/crates/mesh-llm-tui/src/output/dashboard.rs index 4e3ffe80d2..2881876eed 100644 --- a/crates/mesh-llm-tui/src/output/dashboard.rs +++ b/crates/mesh-llm-tui/src/output/dashboard.rs @@ -1647,7 +1647,7 @@ impl DashboardState { // shipped without anything behind it. This is the repair path for // corruption that capture cannot intercept — anything written to // the tty by a process that did not inherit our descriptors. - TuiEvent::Key(TuiKeyEvent::Char('r')) if !self.events_filter.editing => { + TuiEvent::Key(TuiKeyEvent::Char('r' | 'R')) if !self.events_filter.editing => { self.reduce(DashboardAction::RequestFullRepaint); Some(TuiControlFlow::Continue) } diff --git a/crates/mesh-llm-tui/src/output/formatting.rs b/crates/mesh-llm-tui/src/output/formatting.rs index fc962e62a7..abf2d36c8c 100644 --- a/crates/mesh-llm-tui/src/output/formatting.rs +++ b/crates/mesh-llm-tui/src/output/formatting.rs @@ -786,8 +786,13 @@ impl InteractiveDashboardFormatter { let terminal = self.terminal.as_mut().ok_or_else(|| { io::Error::other("pretty TUI terminal missing while terminal mode is active") })?; - if std::mem::take(&mut self.state.pending_full_repaint) { + if self.state.pending_full_repaint { + // Cleared only once the repair actually happened: if the erase + // fails, the next draw would be an ordinary diff against a screen + // ratatui still believes is intact, and the damage would survive + // the key press the operator already made. repair_tui_terminal(terminal)?; + self.state.pending_full_repaint = false; } draw_tui_dashboard_with_terminal(terminal, &self.state)?; self.dirty = false; diff --git a/crates/mesh-llm-tui/src/output/tests/rendering.rs b/crates/mesh-llm-tui/src/output/tests/rendering.rs index 5fc962c3ab..8bfcd6b8c0 100644 --- a/crates/mesh-llm-tui/src/output/tests/rendering.rs +++ b/crates/mesh-llm-tui/src/output/tests/rendering.rs @@ -1756,6 +1756,20 @@ pub(super) fn tui_refresh_key_requests_a_full_repaint() { ); } +#[test] +pub(super) fn tui_refresh_key_accepts_the_uppercase_it_advertises() { + // The status bar reads `R Refresh`, and Shift+R arrives as `Char('R')`. + let mut state = DashboardState::default(); + + let control = state.apply_tui_event(TuiEvent::Key(TuiKeyEvent::Char('R'))); + + assert!(matches!(control, TuiControlFlow::Continue)); + assert!( + state.pending_full_repaint, + "the key the status bar prints must be the key that works" + ); +} + #[test] pub(super) fn tui_refresh_key_types_into_the_filter_instead_of_repainting() { let mut state = DashboardState::default(); From b8eed67a692d2296d28c646ef2596cc589b62e5a Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Thu, 20 Aug 2026 09:05:57 -0400 Subject: [PATCH 08/10] fix(tui): roll back failed terminal setup Restore terminal escape and descriptor state when setup or capture restoration fails, keep retries possible, buffer terminal frames, and remove the Skippy tracing remnants superseded by main's OutputEvent routing. --- Cargo.lock | 1 - .../src/runtime/tracing_writer.rs | 5 - .../src/output/console_capture.rs | 14 ++- crates/mesh-llm-tui/src/output/formatting.rs | 46 ++++++--- .../src/output/rendering/processes.rs | 93 +++++++++++-------- .../mesh-llm-tui/src/output/terminal_out.rs | 10 +- .../src/output/tests/formatting.rs | 24 +++++ crates/mesh-llm-tui/src/output/tests/mod.rs | 2 +- crates/skippy-server/Cargo.toml | 1 - 9 files changed, 128 insertions(+), 68 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0afbd45f76..9fbe432f60 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7562,7 +7562,6 @@ dependencies = [ "tokio-stream", "tonic", "tower", - "tracing", "uuid", ] diff --git a/crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs b/crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs index 85e9ab04e3..f82ee103d0 100644 --- a/crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs +++ b/crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs @@ -301,11 +301,6 @@ pub(super) fn runtime_tracing_subscriber() .with_env_filter( tracing_subscriber::EnvFilter::from_default_env() .add_directive("mesh_inference=info".parse()?) - // Without this, `EnvFilter::from_default_env()` defaults to - // ERROR and every `skippy_server` warning is dropped before it - // reaches the writer — the diagnostics would be silently gone - // rather than routed to the dashboard. - .add_directive("skippy_server=warn".parse()?) .add_directive("nostr_relay_pool=off".parse()?) .add_directive("nostr_sdk=warn".parse()?) .add_directive("noq_proto::connection=warn".parse()?), diff --git a/crates/mesh-llm-tui/src/output/console_capture.rs b/crates/mesh-llm-tui/src/output/console_capture.rs index 8c074fecd5..430b147312 100644 --- a/crates/mesh-llm-tui/src/output/console_capture.rs +++ b/crates/mesh-llm-tui/src/output/console_capture.rs @@ -71,7 +71,12 @@ mod unix { let reader_stderr = fcntl_dupfd_cloexec(stderr(), 0)?; dup2_stdout(&write_fd)?; - dup2_stderr(&write_fd)?; + if let Err(error) = dup2_stderr(&write_fd) { + // stdout was already redirected. Restore it before the pipe + // handles drop so a partial install cannot strand fd 1. + let _ = dup2_stdout(&saved_stdout); + return Err(error.into()); + } drop(write_fd); let active = Arc::new(AtomicBool::new(true)); @@ -93,13 +98,15 @@ mod unix { /// Put the original descriptors back. Safe to call more than once. pub(in crate::output) fn restore(&mut self) -> io::Result<()> { - if !self.active.swap(false, Ordering::Release) { + if !self.active.load(Ordering::Acquire) { return Ok(()); } let _ = io::stdout().flush(); let _ = io::stderr().flush(); dup2_stdout(&self.saved_stdout)?; dup2_stderr(&self.saved_stderr)?; + // Keep restoration retryable until both descriptors are back. + self.active.store(false, Ordering::Release); Ok(()) } } @@ -213,8 +220,9 @@ mod unix { /// timeout, `Err` means the descriptor is unusable and the reader stops. fn wait_for_input(read_fd: &PipeReader, timeout: Duration) -> io::Result { let mut fds = [PollFd::new(read_fd, PollFlags::IN)]; + let timeout = timeout.try_into().map_err(io::Error::other)?; loop { - match poll(&mut fds, Some(&timeout.try_into().unwrap_or_default())) { + match poll(&mut fds, Some(&timeout)) { Ok(0) => return Ok(false), Ok(_) => return Ok(true), Err(rustix::io::Errno::INTR) => continue, diff --git a/crates/mesh-llm-tui/src/output/formatting.rs b/crates/mesh-llm-tui/src/output/formatting.rs index abf2d36c8c..bd3e2bc5c6 100644 --- a/crates/mesh-llm-tui/src/output/formatting.rs +++ b/crates/mesh-llm-tui/src/output/formatting.rs @@ -723,16 +723,28 @@ impl InteractiveDashboardFormatter { if self.terminal_active { return Ok(()); } - let out = TerminalOut::open(); + let mut out = TerminalOut::open(); // Capture is only installed when the dashboard has a descriptor of its // own. Redirecting stderr while still rendering to it would send every // frame into the capture pipe. let capture_is_safe = out.is_private(); - write_tui_enter()?; + write_tui_enter_to_writer(&mut out)?; self.mark_terminal_escape_written(); let backend = CrosstermBackend::new(out); - let mut terminal = Terminal::new(backend).map_err(io::Error::other)?; - terminal.hide_cursor().map_err(io::Error::other)?; + let mut terminal = match Terminal::new(backend).map_err(io::Error::other) { + Ok(terminal) => terminal, + Err(error) => { + self.rollback_terminal_enter(); + return Err(error); + } + }; + if let Err(error) = terminal.hide_cursor().map_err(io::Error::other) { + // Drop the buffered backend before writing the rollback sequence; + // otherwise its final flush could hide the cursor again afterward. + drop(terminal); + self.rollback_terminal_enter(); + return Err(error); + } self.terminal = Some(terminal); if capture_is_safe { // A failure here is not fatal: the dashboard works without capture, @@ -751,6 +763,22 @@ impl InteractiveDashboardFormatter { self.dirty = true; } + fn rollback_terminal_enter(&mut self) { + self.rollback_terminal_enter_with(write_tui_exit); + } + + pub(super) fn rollback_terminal_enter_with(&mut self, exit: impl FnOnce() -> io::Result<()>) { + // The setup error remains the useful result, but teardown is still + // attempted. Reset the bookkeeping even if that best-effort write + // fails so a later enter can retry instead of observing a phantom TUI. + let _ = exit(); + self.release_console_capture(); + self.terminal = None; + self.terminal_active = false; + self.dirty = false; + self.tui_entered.store(false, Ordering::Release); + } + pub(super) fn exit_terminal(&mut self) -> io::Result<()> { if !self.terminal_active { return Ok(()); @@ -760,7 +788,9 @@ impl InteractiveDashboardFormatter { // reader is about to stop projecting into a dashboard that is gone. self.release_console_capture(); if let Some(mut terminal) = self.terminal.take() { - terminal.show_cursor().map_err(io::Error::other)?; + // `write_tui_exit` also shows the cursor, so a backend-specific + // show failure must not prevent the alternate-screen teardown. + let _ = terminal.show_cursor(); } self.terminal_active = false; self.dirty = false; @@ -1555,12 +1585,6 @@ pub(super) fn dashboard_layout_for_terminal_size(columns: u16, rows: u16) -> Das ) } -pub(super) fn write_tui_enter() -> io::Result<()> { - // Must be the same destination the frames go to. Entering the alternate - // screen on one descriptor and leaving it on another strands the terminal. - write_tui_enter_to_writer(&mut TerminalOut::open()) -} - pub(super) fn write_tui_exit() -> io::Result<()> { write_tui_exit_to_writer(&mut TerminalOut::open()) } diff --git a/crates/mesh-llm-tui/src/output/rendering/processes.rs b/crates/mesh-llm-tui/src/output/rendering/processes.rs index 3f19eb72a7..a1c9c3e475 100644 --- a/crates/mesh-llm-tui/src/output/rendering/processes.rs +++ b/crates/mesh-llm-tui/src/output/rendering/processes.rs @@ -1,5 +1,5 @@ use super::super::{ - DashboardEndpointRow, DashboardModelRow, DashboardProcessRow, + DashboardEndpointRow, DashboardModelRow, DashboardPanelViewState, DashboardProcessRow, PRETTY_TUI_WEBSERVER_PROCESS_HEADER_LABEL, llama_process_model_name, model_name_without_variant_suffix, model_names_match, }; @@ -92,26 +92,20 @@ pub(in crate::output) fn render_process_table( )) }) .collect::>(); - let selected_local_index = view - .selected_row - .map(|selected| selected.saturating_sub(view.scroll_offset)); - let mut table_state = TableState::default(); - table_state.select(selected_local_index); - let table = Table::new(rows, process_table_constraints(widths)) - .header(process_table_header_row(present_columns( - widths, - [ - "MODEL".to_string(), - "PID".to_string(), - "PORT".to_string(), - right_align_text("STATE", status_width), - ], - ))) - .column_spacing(1) - .highlight_symbol(if is_focused { "› " } else { " " }) - .highlight_spacing(HighlightSpacing::Always) - .row_highlight_style(process_table_highlight_style(is_focused)); - frame.render_stateful_widget(table, inner_area, &mut table_state); + render_populated_process_table( + frame, + inner_area, + view, + widths, + [ + "MODEL".to_string(), + "PID".to_string(), + "PORT".to_string(), + right_align_text("STATE", status_width), + ], + rows, + is_focused, + ); } DashboardPanel::Webserver => { if state.webserver_rows.is_empty() { @@ -151,31 +145,48 @@ pub(in crate::output) fn render_process_table( )) }) .collect::>(); - let selected_local_index = view - .selected_row - .map(|selected| selected.saturating_sub(view.scroll_offset)); - let mut table_state = TableState::default(); - table_state.select(selected_local_index); - let table = Table::new(rows, process_table_constraints(widths)) - .header(process_table_header_row(present_columns( - widths, - [ - PRETTY_TUI_WEBSERVER_PROCESS_HEADER_LABEL.to_string(), - "PID".to_string(), - "PORT".to_string(), - right_align_text("STATE", status_width), - ], - ))) - .column_spacing(1) - .highlight_symbol(if is_focused { "› " } else { " " }) - .highlight_spacing(HighlightSpacing::Always) - .row_highlight_style(process_table_highlight_style(is_focused)); - frame.render_stateful_widget(table, inner_area, &mut table_state); + render_populated_process_table( + frame, + inner_area, + view, + widths, + [ + PRETTY_TUI_WEBSERVER_PROCESS_HEADER_LABEL.to_string(), + "PID".to_string(), + "PORT".to_string(), + right_align_text("STATE", status_width), + ], + rows, + is_focused, + ); } _ => {} } } +fn render_populated_process_table( + frame: &mut Frame, + inner_area: Rect, + view: DashboardPanelViewState, + widths: [usize; 4], + labels: [String; 4], + rows: Vec>, + is_focused: bool, +) { + let selected_local_index = view + .selected_row + .map(|selected| selected.saturating_sub(view.scroll_offset)); + let mut table_state = TableState::default(); + table_state.select(selected_local_index); + let table = Table::new(rows, process_table_constraints(widths)) + .header(process_table_header_row(present_columns(widths, labels))) + .column_spacing(1) + .highlight_symbol(if is_focused { "› " } else { " " }) + .highlight_spacing(HighlightSpacing::Always) + .row_highlight_style(process_table_highlight_style(is_focused)); + frame.render_stateful_widget(table, inner_area, &mut table_state); +} + pub(in crate::output) fn combine_panel_rect(title_area: Rect, body_area: Rect) -> Rect { Rect { x: title_area.x, diff --git a/crates/mesh-llm-tui/src/output/terminal_out.rs b/crates/mesh-llm-tui/src/output/terminal_out.rs index 86609d112a..a8d6474acc 100644 --- a/crates/mesh-llm-tui/src/output/terminal_out.rs +++ b/crates/mesh-llm-tui/src/output/terminal_out.rs @@ -13,7 +13,7 @@ //! rather than trusting their standard descriptors. use std::fs::{File, OpenOptions}; -use std::io::{self, Write}; +use std::io::{self, BufWriter, Write}; /// Where terminal control sequences and dashboard frames are written. /// @@ -21,15 +21,15 @@ use std::io::{self, Write}; /// piped or redirected session never reaches the dashboard path anyway, but /// the fallback keeps enter/exit sequences working for the fallback console. pub(in crate::output) enum TerminalOut { - Tty(File), - Stderr(io::Stderr), + Tty(BufWriter), + Stderr(BufWriter), } impl TerminalOut { pub(in crate::output) fn open() -> Self { match open_controlling_terminal() { - Some(file) => Self::Tty(file), - None => Self::Stderr(io::stderr()), + Some(file) => Self::Tty(BufWriter::new(file)), + None => Self::Stderr(BufWriter::new(io::stderr())), } } diff --git a/crates/mesh-llm-tui/src/output/tests/formatting.rs b/crates/mesh-llm-tui/src/output/tests/formatting.rs index db57f5010c..341ebeed7b 100644 --- a/crates/mesh-llm-tui/src/output/tests/formatting.rs +++ b/crates/mesh-llm-tui/src/output/tests/formatting.rs @@ -1330,6 +1330,30 @@ pub(super) fn tui_terminal_setup_marks_cleanup_required_after_enter_escape() { assert!(formatter.terminal.is_none()); } +#[test] +pub(super) fn tui_terminal_setup_rollback_resets_state_even_when_exit_fails() { + let mut formatter = InteractiveDashboardFormatter::default(); + formatter.mark_terminal_escape_written(); + let mut exit_attempted = false; + + formatter.rollback_terminal_enter_with(|| { + exit_attempted = true; + Err(io::Error::other("simulated terminal exit failure")) + }); + + assert!(exit_attempted); + assert!(formatter.terminal.is_none()); + assert!(!formatter.terminal_active); + assert!(!formatter.tui_entered()); + assert!(!formatter.dirty); + + formatter.mark_terminal_escape_written(); + assert!( + formatter.terminal_active && formatter.tui_entered(), + "a later terminal setup must be able to retry normally" + ); +} + #[test] pub(super) fn tui_panic_restore_flag_tracks_terminal_entry() { let mut formatter = InteractiveDashboardFormatter::default(); diff --git a/crates/mesh-llm-tui/src/output/tests/mod.rs b/crates/mesh-llm-tui/src/output/tests/mod.rs index ff647166b0..45a23c0381 100644 --- a/crates/mesh-llm-tui/src/output/tests/mod.rs +++ b/crates/mesh-llm-tui/src/output/tests/mod.rs @@ -11,7 +11,7 @@ use ratatui::{ }; use serde_json::Value; use std::{ - io::Write as _, + io::{self, Write as _}, sync::{ Arc, Mutex, atomic::{AtomicBool, Ordering}, diff --git a/crates/skippy-server/Cargo.toml b/crates/skippy-server/Cargo.toml index 9d3cc7c293..fb879633b8 100644 --- a/crates/skippy-server/Cargo.toml +++ b/crates/skippy-server/Cargo.toml @@ -44,7 +44,6 @@ libc = "0.2" tokio = { version = "1", features = ["macros", "net", "rt-multi-thread", "sync"] } tokio-stream = "0.1" tonic = "0.14" -tracing = "0.1" uuid = { version = "1", features = ["v4"] } [dev-dependencies] From 4d960c843d431d6829d7d9bbe17730a0dc2ad655 Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Thu, 20 Aug 2026 09:45:25 -0400 Subject: [PATCH 09/10] fix(tui): keep capture reentry isolated --- .../src/output/console_capture.rs | 141 ++++++++++++++---- .../src/output/rendering/processes.rs | 16 +- .../src/output/tests/rendering.rs | 17 +++ 3 files changed, 135 insertions(+), 39 deletions(-) diff --git a/crates/mesh-llm-tui/src/output/console_capture.rs b/crates/mesh-llm-tui/src/output/console_capture.rs index 430b147312..92cc912d1f 100644 --- a/crates/mesh-llm-tui/src/output/console_capture.rs +++ b/crates/mesh-llm-tui/src/output/console_capture.rs @@ -11,7 +11,9 @@ //! line into an `OutputEvent`, so stray output shows up as a dashboard event //! instead of painting over the frame. On restore the original descriptors are //! put back and any still-buffered lines are written to the real stderr so -//! nothing is silently swallowed. +//! nothing is silently swallowed. If a child keeps an old pipe open across a +//! later dashboard session, that stale reader follows the current capture +//! state and routes its output into the new dashboard. //! //! This is only safe because the dashboard renders to the controlling terminal //! (see [`super::terminal_out`]) rather than to fd 2; installing capture while @@ -26,15 +28,14 @@ pub(in crate::output) use fallback::ConsoleCapture; #[cfg(unix)] mod unix { - use mesh_llm_events::{OutputEvent, emit_event}; + use mesh_llm_events::OutputEvent; use rustix::event::{PollFd, PollFlags, poll}; use rustix::io::fcntl_dupfd_cloexec; use rustix::stdio::{dup2_stderr, dup2_stdout, stderr, stdout}; use std::fs::File; use std::io::{self, PipeReader, Read, Write}; use std::os::fd::OwnedFd; - use std::sync::Arc; - use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::atomic::{AtomicUsize, Ordering}; use std::time::Duration; /// How long a partial line may sit unflushed before it is shown anyway. @@ -45,10 +46,14 @@ mod unix { /// until the process dies. const MAX_CAPTURED_LINE_BYTES: usize = 8 * 1024; + /// Readers can outlive the dashboard session that spawned them when a + /// child inherits a pipe write end, so delivery follows global activity. + static ACTIVE_CAPTURES: AtomicUsize = AtomicUsize::new(0); + pub(in crate::output) struct ConsoleCapture { saved_stdout: OwnedFd, saved_stderr: OwnedFd, - active: Arc, + active: bool, } impl ConsoleCapture { @@ -70,35 +75,35 @@ mod unix { // still deliver lines after the dashboard goes away. let reader_stderr = fcntl_dupfd_cloexec(stderr(), 0)?; - dup2_stdout(&write_fd)?; + // Start the reader before redirecting either descriptor. Once the + // capture is registered, stale readers from earlier children also + // route into the active dashboard. + spawn_reader(read_fd, reader_stderr)?; + ACTIVE_CAPTURES.fetch_add(1, Ordering::AcqRel); + + if let Err(error) = dup2_stdout(&write_fd) { + unregister_capture(); + return Err(error.into()); + } if let Err(error) = dup2_stderr(&write_fd) { // stdout was already redirected. Restore it before the pipe // handles drop so a partial install cannot strand fd 1. let _ = dup2_stdout(&saved_stdout); + unregister_capture(); return Err(error.into()); } drop(write_fd); - let active = Arc::new(AtomicBool::new(true)); - if let Err(error) = spawn_reader(read_fd, reader_stderr, Arc::clone(&active)) { - // With no reader, nothing drains the pipe: the process would - // block forever on the write that fills the pipe buffer. Put - // the real descriptors back before giving up. - let _ = dup2_stdout(&saved_stdout); - let _ = dup2_stderr(&saved_stderr); - return Err(error); - } - Ok(Self { saved_stdout, saved_stderr, - active, + active: true, }) } /// Put the original descriptors back. Safe to call more than once. pub(in crate::output) fn restore(&mut self) -> io::Result<()> { - if !self.active.load(Ordering::Acquire) { + if !self.active { return Ok(()); } let _ = io::stdout().flush(); @@ -106,7 +111,8 @@ mod unix { dup2_stdout(&self.saved_stdout)?; dup2_stderr(&self.saved_stderr)?; // Keep restoration retryable until both descriptors are back. - self.active.store(false, Ordering::Release); + self.active = false; + unregister_capture(); Ok(()) } } @@ -121,11 +127,7 @@ mod unix { /// that inherited the write end keeps the pipe open past restore, so a join /// could block shutdown indefinitely; the thread instead exits on EOF /// whenever that arrives, and dies with the process at worst. - fn spawn_reader( - mut read_fd: PipeReader, - original_stderr: OwnedFd, - active: Arc, - ) -> io::Result<()> { + fn spawn_reader(mut read_fd: PipeReader, original_stderr: OwnedFd) -> io::Result<()> { std::thread::Builder::new() .name("mesh-console-capture".to_string()) .spawn(move || { @@ -139,7 +141,7 @@ mod unix { while let Ok(readable) = wait_for_input(&read_fd, IDLE_FLUSH) { if !readable { for line in take_pending_lines(&mut pending, true) { - deliver(line, &active, &mut passthrough); + deliver(line, &mut passthrough); } continue; } @@ -151,24 +153,52 @@ mod unix { // buffer without bound. let force = pending.len() >= MAX_CAPTURED_LINE_BYTES; for line in take_pending_lines(&mut pending, force) { - deliver(line, &active, &mut passthrough); + deliver(line, &mut passthrough); } } for line in take_pending_lines(&mut pending, true) { - deliver(line, &active, &mut passthrough); + deliver(line, &mut passthrough); } }) .map(|_| ()) } - fn deliver(text: String, active: &AtomicBool, passthrough: &mut File) { + fn unregister_capture() { + let result = ACTIVE_CAPTURES.fetch_update(Ordering::AcqRel, Ordering::Acquire, |count| { + count.checked_sub(1) + }); + debug_assert!(result.is_ok(), "capture activity count underflowed"); + } + + fn dashboard_capture_active(active_captures: &AtomicUsize) -> bool { + active_captures.load(Ordering::Acquire) > 0 + } + + fn deliver(text: String, passthrough: &mut File) { + deliver_with( + text, + &ACTIVE_CAPTURES, + passthrough, + mesh_llm_events::emit_event, + ); + } + + pub(super) fn deliver_with( + text: String, + active_captures: &AtomicUsize, + passthrough: &mut W, + emit: E, + ) where + W: Write, + E: FnOnce(OutputEvent) -> io::Result<()>, + { if text.trim().is_empty() { return; } - if active.load(Ordering::Acquire) { + if dashboard_capture_active(active_captures) { // Failure here means the dashboard sink is gone; fall back to the // real terminal rather than dropping the line. - if emit_event(dashboard_event(text.clone())).is_ok() { + if emit(dashboard_event(text.clone())).is_ok() { return; } } @@ -274,8 +304,11 @@ mod fallback { #[cfg(all(test, unix))] mod tests { - use super::unix::{dashboard_event, take_pending_lines}; + use super::unix::{dashboard_event, deliver_with, take_pending_lines}; use mesh_llm_events::OutputEvent; + use std::io::{Read, Write}; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::mpsc; fn split(input: &[u8], flush_remainder: bool) -> Vec { let mut pending = input.to_vec(); @@ -353,4 +386,50 @@ mod tests { "the dashboard should show that this line was intercepted, not emitted" ); } + + #[test] + fn stale_reader_routes_child_output_to_reentered_dashboard() { + let active_captures = AtomicUsize::new(1); + let (mut read_fd, write_fd) = std::io::pipe().expect("pipe should open"); + let (child_ready_tx, child_ready_rx) = mpsc::channel(); + let (write_tx, write_rx) = mpsc::channel(); + let child = std::thread::spawn(move || { + let mut retained_write_fd = write_fd; + child_ready_tx + .send(()) + .expect("parent should wait for retained pipe"); + write_rx.recv().expect("parent should release child writer"); + retained_write_fd + .write_all(b"late child output\n") + .expect("child output should write"); + }); + + child_ready_rx + .recv() + .expect("child should retain the first pipe write end"); + active_captures.fetch_sub(1, Ordering::AcqRel); + active_captures.fetch_add(1, Ordering::AcqRel); + write_tx.send(()).expect("child should still be waiting"); + + let mut captured = String::new(); + read_fd + .read_to_string(&mut captured) + .expect("reader should receive late child output"); + child.join().expect("child writer should exit"); + + let mut emitted = Vec::new(); + let mut passthrough = Vec::new(); + deliver_with( + captured.trim_end().to_string(), + &active_captures, + &mut passthrough, + |event| { + emitted.push(event); + Ok(()) + }, + ); + + assert_eq!(emitted.len(), 1, "late output belongs in the new dashboard"); + assert!(passthrough.is_empty(), "it must not bypass the dashboard"); + } } diff --git a/crates/mesh-llm-tui/src/output/rendering/processes.rs b/crates/mesh-llm-tui/src/output/rendering/processes.rs index a1c9c3e475..8cf5e86664 100644 --- a/crates/mesh-llm-tui/src/output/rendering/processes.rs +++ b/crates/mesh-llm-tui/src/output/rendering/processes.rs @@ -399,14 +399,14 @@ pub(in crate::output) fn process_column_widths( } } - // Narrower than even text + PID. Keep both and let the text truncate; - // there is no useful table left to render otherwise. - [ - available.saturating_sub(pid_width + 1).max(1), - pid_width, - 0, - 0, - ] + // Narrower than even one text cell + spacing + PID. Preserve the table's + // width invariant by surrendering PID as well. + if available < pid_width.saturating_add(2) { + return [available, 0, 0, 0]; + } + + // Keep text + PID and let the text truncate. + [available - pid_width - 1, pid_width, 0, 0] } /// Constraints for the columns that survived [`process_column_widths`]. diff --git a/crates/mesh-llm-tui/src/output/tests/rendering.rs b/crates/mesh-llm-tui/src/output/tests/rendering.rs index 8bfcd6b8c0..7a55715d2f 100644 --- a/crates/mesh-llm-tui/src/output/tests/rendering.rs +++ b/crates/mesh-llm-tui/src/output/tests/rendering.rs @@ -1884,6 +1884,23 @@ pub(super) fn process_columns_drop_port_next_and_still_keep_the_name() { assert!(text >= 8, "the model name keeps its minimum: got {text}"); } +#[test] +pub(super) fn process_columns_drop_pid_before_exceeding_the_table_width() { + let pid_width = 5; + + // body width includes the two cells reserved for the highlight symbol. + assert_eq!( + process_column_widths((pid_width + 3) as u16, 8, pid_width, 8), + [pid_width + 1, 0, 0, 0], + "available == pid width + 1 cannot also fit text and spacing" + ); + assert_eq!( + process_column_widths((pid_width + 4) as u16, 8, pid_width, 8), + [1, pid_width, 0, 0], + "available == pid width + 2 exactly fits text, spacing, and PID" + ); +} + #[test] pub(super) fn process_table_renders_only_the_columns_that_fit() { let widths = [9usize, 5, 0, 0]; From f86539c09f63d742b7be4d9757596c87d639272f Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Thu, 20 Aug 2026 09:52:30 -0400 Subject: [PATCH 10/10] fix(tui): publish capture before reader startup --- .../src/output/console_capture.rs | 85 +++++++++++++++++-- 1 file changed, 77 insertions(+), 8 deletions(-) diff --git a/crates/mesh-llm-tui/src/output/console_capture.rs b/crates/mesh-llm-tui/src/output/console_capture.rs index 92cc912d1f..17f7080ccc 100644 --- a/crates/mesh-llm-tui/src/output/console_capture.rs +++ b/crates/mesh-llm-tui/src/output/console_capture.rs @@ -78,18 +78,17 @@ mod unix { // Start the reader before redirecting either descriptor. Once the // capture is registered, stale readers from earlier children also // route into the active dashboard. - spawn_reader(read_fd, reader_stderr)?; - ACTIVE_CAPTURES.fetch_add(1, Ordering::AcqRel); + register_capture_reader(&ACTIVE_CAPTURES, || spawn_reader(read_fd, reader_stderr))?; if let Err(error) = dup2_stdout(&write_fd) { - unregister_capture(); + unregister_capture(&ACTIVE_CAPTURES); return Err(error.into()); } if let Err(error) = dup2_stderr(&write_fd) { // stdout was already redirected. Restore it before the pipe // handles drop so a partial install cannot strand fd 1. let _ = dup2_stdout(&saved_stdout); - unregister_capture(); + unregister_capture(&ACTIVE_CAPTURES); return Err(error.into()); } drop(write_fd); @@ -112,7 +111,7 @@ mod unix { dup2_stderr(&self.saved_stderr)?; // Keep restoration retryable until both descriptors are back. self.active = false; - unregister_capture(); + unregister_capture(&ACTIVE_CAPTURES); Ok(()) } } @@ -163,8 +162,20 @@ mod unix { .map(|_| ()) } - fn unregister_capture() { - let result = ACTIVE_CAPTURES.fetch_update(Ordering::AcqRel, Ordering::Acquire, |count| { + pub(super) fn register_capture_reader( + active_captures: &AtomicUsize, + spawn_reader: impl FnOnce() -> io::Result<()>, + ) -> io::Result<()> { + active_captures.fetch_add(1, Ordering::AcqRel); + if let Err(error) = spawn_reader() { + unregister_capture(active_captures); + return Err(error); + } + Ok(()) + } + + fn unregister_capture(active_captures: &AtomicUsize) { + let result = active_captures.fetch_update(Ordering::AcqRel, Ordering::Acquire, |count| { count.checked_sub(1) }); debug_assert!(result.is_ok(), "capture activity count underflowed"); @@ -304,9 +315,10 @@ mod fallback { #[cfg(all(test, unix))] mod tests { - use super::unix::{dashboard_event, deliver_with, take_pending_lines}; + use super::unix::{dashboard_event, deliver_with, register_capture_reader, take_pending_lines}; use mesh_llm_events::OutputEvent; use std::io::{Read, Write}; + use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::mpsc; @@ -432,4 +444,61 @@ mod tests { assert_eq!(emitted.len(), 1, "late output belongs in the new dashboard"); assert!(passthrough.is_empty(), "it must not bypass the dashboard"); } + + #[test] + fn stale_reader_routes_output_during_reader_registration() { + let active_captures = Arc::new(AtomicUsize::new(0)); + let (deliver_tx, deliver_rx) = mpsc::channel(); + let (delivered_tx, delivered_rx) = mpsc::channel(); + let reader_activity = Arc::clone(&active_captures); + let stale_reader = std::thread::spawn(move || { + deliver_rx + .recv() + .expect("registration should release reader"); + let mut emitted = Vec::new(); + let mut passthrough = Vec::new(); + deliver_with( + "concurrent stale output".to_string(), + &reader_activity, + &mut passthrough, + |event| { + emitted.push(event); + Ok(()) + }, + ); + delivered_tx + .send(()) + .expect("registration should await delivery"); + (emitted, passthrough) + }); + + register_capture_reader(&active_captures, || { + deliver_tx.send(()).expect("stale reader should be waiting"); + delivered_rx + .recv() + .expect("stale reader should deliver during registration"); + Ok(()) + }) + .expect("reader registration should succeed"); + + let (emitted, passthrough) = stale_reader.join().expect("stale reader should exit"); + assert_eq!( + emitted.len(), + 1, + "concurrent output belongs in the dashboard" + ); + assert!(passthrough.is_empty(), "it must not bypass the dashboard"); + } + + #[test] + fn failed_reader_registration_rolls_back_capture_activity() { + let active_captures = AtomicUsize::new(0); + + let result = register_capture_reader(&active_captures, || { + Err(std::io::Error::other("reader spawn failed")) + }); + + assert!(result.is_err()); + assert_eq!(active_captures.load(Ordering::Acquire), 0); + } }