fix(review): restore Agent Update Required flow, command field, axis alignment
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 <noreply@anthropic.com>
This commit is contained in:
+61
-11
@@ -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<u64>,
|
||||
|
||||
// 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<u64>,
|
||||
tx_hist: VecDeque<u64>,
|
||||
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;
|
||||
|
||||
@@ -624,16 +624,29 @@ impl ModalManager {
|
||||
// labels + axis title + (top) Y-axis title + legend + spacing.
|
||||
let mut lines: Vec<Line> = 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<String> = (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),
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user