From 746ca4cf58a8a8616a93ee30943029b8e9878f6b Mon Sep 17 00:00:00 2001 From: jasonwitty Date: Wed, 19 Aug 2026 16:52:31 -0700 Subject: [PATCH] fix(review): restore Agent Update Required flow, command field, axis alignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from Jason's hands-on verification of the branch: 1. Old-agent messaging regression (this branch): a detail-request timeout went through the loud poison/reconnect flow, burying the ProcessDetails modal's 'Agent Update Required' message under a connection-error modal. Old agents IGNORE unknown messages (no late reply, no desync), so the optional per-PID endpoints now use quiet_reconnect(): swap the stream silently (still safe against merely-slow agents) and let the modal show its message. Only a failed reconnect surfaces loudly. Verified against a real v1.40.0 agent: message shows, session stays healthy. 2. Draw starvation (this branch): an agent that never answers get_metrics put the loop in fetch->timeout->poison->restart cycles that never reached the draw call — permanently blank TUI. The iteration now paints before fetching, and a second consecutive metrics timeout trips a circuit breaker: persistent 'Agent is not responding' error, recovery left to the manual/30s retry paths. Verified against a 0.9-era agent. 3. Command & Details pane blank (pre-existing on master): the minimal- refresh optimization dropped cmd/exe/cwd from the detail endpoint's ProcessRefreshKind, so process.cmd() had nothing to return. Restored with UpdateKind::OnlyIfNotSet — immutable values, read once per PID. Regression test added; journal E2E re-verified (100 entries render). 4. Scatter-plot axis misalignment: Y labels used a fixed 4-char field from the era when CPU times were 1000x too small; honest millisecond values (e.g. 136114) blew through it. Labels now right-align to the widest value per frame and X labels/titles share the dynamic padding. Co-Authored-By: Claude Fable 5 --- socktop/src/app.rs | 72 ++++++++++++++++++++++---- socktop/src/ui/modal_process.rs | 46 ++++++++++------ socktop_agent/src/metrics.rs | 9 +++- socktop_agent/tests/process_details.rs | 16 ++++++ 4 files changed, 115 insertions(+), 28 deletions(-) diff --git a/socktop/src/app.rs b/socktop/src/app.rs index 989390d..51588f6 100644 --- a/socktop/src/app.rs +++ b/socktop/src/app.rs @@ -94,6 +94,12 @@ pub struct App { last_net_totals: Option<(u64, u64, Instant)>, // Agent-side sample timestamp of the previous snapshot (1.51+ agents). last_net_sampled_at_ms: Option, + + // Consecutive metrics-request timeouts. One timeout gets a silent stream + // refresh; a second in a row means the agent accepts connections but + // never answers, and deserves a persistent error instead of an invisible + // reconnect loop that starves the UI. + consecutive_request_timeouts: u32, rx_hist: VecDeque, tx_hist: VecDeque, rx_peak: u64, @@ -195,6 +201,7 @@ impl App { per_core_hist: PerCoreHistory::new(60), last_net_totals: None, last_net_sampled_at_ms: None, + consecutive_request_timeouts: 0, rx_hist: VecDeque::with_capacity(600), tx_hist: VecDeque::with_capacity(600), rx_peak: 0, @@ -374,6 +381,30 @@ impl App { self.retry_connection().await; } + /// Replace the connection WITHOUT any modal or state churn. + /// + /// For timeouts on the optional per-process endpoints: an old agent + /// ignores those messages entirely (no late reply, so no desync), but a + /// merely-slow agent would desync the stream — indistinguishable at + /// timeout time, so we still swap to a fresh stream, silently. The + /// ProcessDetails modal keeps showing its "Agent Update Required" + /// message instead of being buried under a connection-error modal. + /// Only a failed reconnect (connection genuinely dead) surfaces loudly. + async fn quiet_reconnect(&mut self) { + let tls_ca_ref = self.tls_ca.as_deref(); + match self + .try_connect(&self.ws_url, tls_ca_ref, self.verify_hostname) + .await + { + Ok(ws) => { + self.replacement_connection = Some(ws); + } + Err(e) => { + self.show_connection_error(format!("Reconnect failed: {e}")); + } + } + } + /// Mark connection as successful and dismiss any error modals pub fn mark_connected(&mut self) { if self.connection_state != ConnectionState::Connected { @@ -1053,6 +1084,13 @@ impl App { break; } + // Paint the current state BEFORE fetching: a request can stall for + // the full 5s timeout, and an iteration that ends in a poisoned- + // stream restart never reaches the draw at the bottom — without + // this, an agent that never answers left the screen permanently + // blank. (ratatui diffs make an unchanged repaint nearly free.) + terminal.draw(|f| self.draw(f))?; + // Fetch and update. Skipped while disconnected — the retry paths // (manual 'r' or the 30s auto-retry) own recovery, and hammering a // dead socket with 5s-timeout requests would stall the loop. The @@ -1061,10 +1099,22 @@ impl App { if self.connection_state == ConnectionState::Connected { match timeout(REQUEST_TIMEOUT, ws.request(AgentRequest::Metrics)).await { Err(_) => { - self.poison_connection("Metrics request timed out").await; + self.consecutive_request_timeouts += 1; + if self.consecutive_request_timeouts >= 2 { + // The agent accepts connections but never answers + // (wrong protocol era, or wedged): reconnecting + // can't help, so surface a persistent error and + // leave recovery to the manual/auto retry paths. + self.show_connection_error( + "Agent is not responding to requests".to_string(), + ); + } else { + self.poison_connection("Metrics request timed out").await; + } } Ok(Ok(AgentResponse::Metrics(m))) => { self.mark_connected(); // Mark as connected on successful request + self.consecutive_request_timeouts = 0; self.update_with_metrics(m); // Only poll processes every 2s @@ -1209,14 +1259,14 @@ impl App { self.process_details_unsupported = true; } Err(_) => { - // No reply at all: mark unsupported AND - // reconnect — a late reply would desync - // every later request on this stream. + // No reply at all: old agents IGNORE + // this message, so show the "Agent + // Update Required" state and refresh + // the stream quietly (a merely-slow + // agent's late reply would otherwise + // desync it). self.process_details_unsupported = true; - self.poison_connection( - "Process details request timed out", - ) - .await; + self.quiet_reconnect().await; } Ok(Ok(_)) => { // Wrong response type @@ -1244,9 +1294,9 @@ impl App { self.journal_entries = Some(journal); } Err(_) => { - // No reply: poison the stream (see above). - self.poison_connection("Journal request timed out") - .await; + // No reply: same quiet stream refresh + // as the details endpoint above. + self.quiet_reconnect().await; } Ok(Err(_)) | Ok(Ok(_)) => { // Endpoint unsupported or wrong type; diff --git a/socktop/src/ui/modal_process.rs b/socktop/src/ui/modal_process.rs index 9b9ace7..49c75aa 100644 --- a/socktop/src/ui/modal_process.rs +++ b/socktop/src/ui/modal_process.rs @@ -624,16 +624,29 @@ impl ModalManager { // labels + axis title + (top) Y-axis title + legend + spacing. let mut lines: Vec = Vec::with_capacity(plot_height + 6); - // Y-axis labels and plot content - let mut row_buf = String::with_capacity(plot_width); - for y in 0..plot_height { - let y_value = params.max_system * (1.0 - (y as f64 / (plot_height - 1).max(1) as f64)); - // 4-char fixed-width label so the axis doesn't shift as digits change. - let y_label = if y_value >= 100.0 { - format!("{y_value:>4.0}") + // Format a CPU-time value: whole ms once past 100, one decimal below. + let fmt_ms = |v: f64| { + if v >= 100.0 { + format!("{v:.0}") } else { - format!("{y_value:>4.1}") - }; + format!("{v:.1}") + } + }; + + // Y-axis labels, right-aligned to the widest value this frame so the + // axis stays a straight line. The old fixed 4-char field predates the + // CPU-time unit fix; honest millisecond values (e.g. 136114) blew + // through it and skewed the whole axis. + let y_values: Vec = (0..plot_height) + .map(|y| { + fmt_ms(params.max_system * (1.0 - (y as f64 / (plot_height - 1).max(1) as f64))) + }) + .collect(); + let y_label_w = y_values.iter().map(|s| s.len()).max().unwrap_or(4).max(4); + + let mut row_buf = String::with_capacity(plot_width); + for (y, y_value) in y_values.iter().enumerate() { + let y_label = format!("{y_value:>y_label_w$}"); // Build the row's char slice into a reusable String buffer. row_buf.clear(); @@ -650,8 +663,8 @@ impl ModalManager { ])); } - // Add X-axis - let x_axis_padding = " ".to_string(); // Match Y-axis label width + // Add X-axis (padding = Y label width + the space before the bar) + let x_axis_padding = " ".repeat(y_label_w + 1); let x_axis_line = "─".repeat(plot_width + 1); lines.push(Line::from(vec![ Span::styled(x_axis_padding, Style::default()), @@ -659,13 +672,14 @@ impl ModalManager { ])); // Add X-axis labels - let x_label_start = "0.0".to_string(); - let x_label_mid = format!("{:.1}", params.max_user / 2.0); - let x_label_end = format!("{:.1}", params.max_user); + let x_label_start = fmt_ms(0.0); + let x_label_mid = fmt_ms(params.max_user / 2.0); + let x_label_end = fmt_ms(params.max_user); let spacing = plot_width / 3; let x_labels = format!( - " {}{}{}{}{}", + "{}{}{}{}{}{}", + " ".repeat(y_label_w + 1), x_label_start, " ".repeat(spacing.saturating_sub(x_label_start.len())), x_label_mid, @@ -677,7 +691,7 @@ impl ModalManager { // Add axis titles with better visibility lines.push(Line::from(vec![Span::styled( - " User CPU Time (ms) →", + format!("{}User CPU Time (ms) →", " ".repeat(y_label_w + 1)), Style::default() .fg(Color::Yellow) .add_modifier(Modifier::BOLD), diff --git a/socktop_agent/src/metrics.rs b/socktop_agent/src/metrics.rs index 002e714..83a1532 100644 --- a/socktop_agent/src/metrics.rs +++ b/socktop_agent/src/metrics.rs @@ -1161,10 +1161,17 @@ pub async fn collect_process_metrics( system.refresh_processes_specifics( ProcessesToUpdate::Some(&[sysinfo::Pid::from_u32(pid)]), false, + // cmd/exe/cwd feed the modal's Command & Details pane. They're + // immutable per process, so OnlyIfNotSet reads them once per PID and + // serves the cache afterwards — the "minimal refresh" optimization + // had dropped them entirely, leaving the pane blank. ProcessRefreshKind::nothing() .with_memory() .with_cpu() - .with_disk_usage(), + .with_disk_usage() + .with_cmd(sysinfo::UpdateKind::OnlyIfNotSet) + .with_exe(sysinfo::UpdateKind::OnlyIfNotSet) + .with_cwd(sysinfo::UpdateKind::OnlyIfNotSet), ); let process = system diff --git a/socktop_agent/tests/process_details.rs b/socktop_agent/tests/process_details.rs index b73d5d6..7ffe0e7 100644 --- a/socktop_agent/tests/process_details.rs +++ b/socktop_agent/tests/process_details.rs @@ -87,3 +87,19 @@ async fn test_collect_journal_entries_invalid_pid() { } } } + +/// The Command & Details pane went blank when the minimal-refresh +/// optimization dropped cmd from the detail endpoint's refresh kind. +#[tokio::test] +async fn test_process_metrics_include_command() { + let state = AppState::new(); + let pid = std::process::id(); + let resp = collect_process_metrics(pid, &state) + .await + .expect("collect self"); + assert!( + !resp.process.command.is_empty(), + "command should not be empty for self (cmdline is always readable)" + ); + println!("command = {}", resp.process.command); +}