From f4eaba5d70b5bb14b1eac5ff17540e1e9965d427 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Fri, 2 Oct 2026 18:53:24 +0700 Subject: [PATCH] fix(tui): surface incomplete statistics and strengthen review coverage --- crates/irtt-app/src/cmd/tui/run.rs | 57 +++++++- crates/irtt-app/tests/cli.rs | 135 ++++++++++++++++++ .../irtt-server/tests/protocol_contracts.rs | 9 ++ crates/irtt-server/tests/runtime.rs | 60 +++++--- 4 files changed, 239 insertions(+), 22 deletions(-) diff --git a/crates/irtt-app/src/cmd/tui/run.rs b/crates/irtt-app/src/cmd/tui/run.rs index afca9d1..4cbe24b 100644 --- a/crates/irtt-app/src/cmd/tui/run.rs +++ b/crates/irtt-app/src/cmd/tui/run.rs @@ -87,12 +87,18 @@ pub fn run_tui( ) { drop(handle.stop()); } + let previous_dropped = dropped_events; let drain_state = drain_tui_events( &mut events, &mut state, &mut terminal_targets, &mut dropped_events, ); + if dropped_events != previous_dropped { + // Data loss changes the meaning of displayed statistics, even while + // ordinary measurement refreshes are paused. + render_if_due(&mut terminal, &state, &mut next_render, true)?; + } match drain_state { ManagedDrainState::Empty => { thread::sleep(managed_tui_wait_duration(&next_render, state.paused)); @@ -178,11 +184,16 @@ fn drain_tui_events( terminal_targets: &mut HashSet, dropped_events: &mut u64, ) -> ManagedDrainState { - drain_managed_events(events, dropped_events, |event| { + let previous_dropped = *dropped_events; + let drained = drain_managed_events(events, dropped_events, |event| { process_tui_event(event, state, terminal_targets); Ok::<(), std::convert::Infallible>(()) }) - .expect("processing TUI managed events is infallible") + .expect("processing TUI managed events is infallible"); + if *dropped_events != previous_dropped { + state.mark_dropped_managed_events(*dropped_events); + } + drained } fn drain_final_tui_events( @@ -301,3 +312,45 @@ fn managed_tui_wait_duration(next_render: &Instant, paused: bool) -> Duration { }; render_wait.min(TUI_WAIT_SLICE) } + +#[cfg(test)] +mod tests { + use super::*; + use ratatui::{backend::TestBackend, Terminal}; + + #[test] + fn subscription_lag_keeps_incomplete_statistics_visible_while_paused() { + for paused in [false, true] { + // The production subscription is a broadcast receiver. Overflow the + // real channel rather than synthesizing a lag result or UI state. + let (sender, mut events) = tokio::sync::broadcast::channel(1); + sender.send(ManagedEvent::Started).unwrap(); + sender.send(ManagedEvent::Started).unwrap(); + let mut state = TuiState::default(); + if paused { + state.toggle_pause(); + } + let mut dropped = 0; + let mut terminal_targets = HashSet::new(); + drain_tui_events(&mut events, &mut state, &mut terminal_targets, &mut dropped); + assert_eq!(dropped, 1); + let mut terminal = Terminal::new(TestBackend::new(200, 40)).unwrap(); + for _ in 0..2 { + // Empty drains must not clear the warning on later redraws. + drain_tui_events(&mut events, &mut state, &mut terminal_targets, &mut dropped); + terminal + .draw(|frame| super::super::ui::draw_dashboard(frame, &state)) + .unwrap(); + let text = terminal + .backend() + .buffer() + .content() + .iter() + .map(|cell| cell.symbol()) + .collect::(); + assert!(text.contains("incomplete:dropped=1")); + assert_eq!(text.contains("display paused"), paused); + } + } + } +} diff --git a/crates/irtt-app/tests/cli.rs b/crates/irtt-app/tests/cli.rs index c648fa6..a2c3939 100644 --- a/crates/irtt-app/tests/cli.rs +++ b/crates/irtt-app/tests/cli.rs @@ -121,3 +121,138 @@ fn client_arguments_export_a_real_client_run() { .iter() .any(|row| row.contains("\"event\":\"session_closed\""))); } + +#[cfg(any(feature = "client", feature = "tui"))] +#[test] +fn malformed_target_diagnostics_do_not_expose_authentication_keys() { + let secret = "malformed-target-secret-92817"; + for applet in [ + #[cfg(feature = "client")] + "client", + #[cfg(feature = "tui")] + "tui", + ] { + let output = Command::new(env!("CARGO_BIN_EXE_irtt-rs")) + .args([applet, &format!(r"edge=127.0.0.1:9@hmac={secret}\q")]) + .output() + .unwrap(); + assert!(!output.status.success()); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + !stderr.trim().is_empty(), + "malformed target must report a diagnostic" + ); + assert!(!stderr.contains(secret), "target authentication key leaked"); + assert!(!String::from_utf8_lossy(&output.stdout).contains(secret)); + } +} + +#[cfg(feature = "server")] +#[test] +fn server_arguments_resolve_binds_and_map_policy() { + use clap::{CommandFactory, FromArgMatches, Parser}; + use irtt_app::cmd::server::ServerArgs; + use irtt_server::TimestampAllowance; + use std::time::Duration; + // Isolate zero-argument defaults from the test runner's environment without + // mutating process-global environment in this parallel test process. + let defaults = ServerArgs::from_arg_matches( + &ServerArgs::command() + .mut_args(|arg| arg.env(None::<&str>)) + .try_get_matches_from(["irtt-server"]) + .unwrap(), + ) + .unwrap(); + assert_eq!( + defaults.resolve_binds(), + vec![ + "[::]:2112".parse().unwrap(), + "0.0.0.0:2112".parse().unwrap() + ] + ); + assert_eq!( + defaults.server_config(), + irtt_server::ServerConfig::default() + ); + let args = ServerArgs::try_parse_from([ + "irtt-server", + "--bind", + "127.0.0.1:2113", + "--bind", + "[::1]:2114", + "--hmac", + "server-cli-test", + "--max-sessions", + "3", + "--max-packet-length", + "96", + "--min-interval", + "25ms", + "--burst", + "2", + "--idle-timeout", + "8s", + "--max-duration", + "1m", + "--timestamp-allowance", + "single", + "--no-dscp", + ]) + .unwrap(); + assert_eq!( + args.resolve_binds(), + vec![ + "127.0.0.1:2113".parse().unwrap(), + "[::1]:2114".parse().unwrap() + ] + ); + let config = args.server_config(); + assert_eq!(config.hmac_key(), Some(b"server-cli-test".as_slice())); + assert_eq!(config.max_sessions(), 3); + assert_eq!(config.max_packet_length(), 96); + assert_eq!(config.min_send_interval(), Duration::from_millis(25)); + assert_eq!(config.burst_allowance(), 2); + assert_eq!(config.idle_timeout(), Duration::from_secs(8)); + assert_eq!(config.max_test_duration(), Some(Duration::from_secs(60))); + assert_eq!(config.timestamp_allowance(), TimestampAllowance::Single); + assert!(!config.dscp_allowed()); +} + +#[cfg(feature = "server")] +#[test] +fn server_command_line_binds_replace_the_environment_bind() { + use std::net::UdpSocket; + // Occupied ports give bounded, observable startup failures, without starting + // a daemon or needing a process/signal harness merely to inspect arguments. + let environment = UdpSocket::bind("127.0.0.1:0").unwrap(); + let explicit = UdpSocket::bind("127.0.0.1:0").unwrap(); + let environment_addr = environment.local_addr().unwrap().to_string(); + let explicit_addr = explicit.local_addr().unwrap().to_string(); + for use_explicit in [false, true] { + let mut command = Command::new(env!("CARGO_BIN_EXE_irtt-server")); + command + .env_clear() + .env("IRTT_SERVER_BIND", &environment_addr) + .env("IRTT_SERVER_MAX_SESSIONS", "invalid") + .args(["--max-sessions", "3"]); + if use_explicit { + command.args(["--bind", "127.0.0.1:0", "--bind", &explicit_addr]); + } + let output = command.output().unwrap(); + assert!(!output.status.success()); + let stderr = String::from_utf8(output.stderr).unwrap(); + let expected = if use_explicit { + &explicit_addr + } else { + &environment_addr + }; + assert!( + stderr.contains(expected), + "startup must attempt the selected bind: {stderr}" + ); + assert!( + !stderr.contains("listening on"), + "partial startup must not announce listeners" + ); + } +} diff --git a/crates/irtt-server/tests/protocol_contracts.rs b/crates/irtt-server/tests/protocol_contracts.rs index 7c894a4..761efb0 100644 --- a/crates/irtt-server/tests/protocol_contracts.rs +++ b/crates/irtt-server/tests/protocol_contracts.rs @@ -355,3 +355,12 @@ fn duration_close_waits_for_rate_allowance_after_the_deadline() { assert_eq!(closed.recv_window, Some(3)); assert_eq!(server.session_count(), 0); } + +#[test] +fn default_server_resource_limits_remain_bounded() { + let config = ServerConfig::default(); + // Upper resource budgets, not an exact default or a comparison against + // the same constants used by the implementation. Smaller bounds are fine. + assert!((1..=1024).contains(&config.max_sessions())); + assert!((1..=65_507).contains(&config.max_packet_length())); +} diff --git a/crates/irtt-server/tests/runtime.rs b/crates/irtt-server/tests/runtime.rs index 047f3e9..9b3af99 100644 --- a/crates/irtt-server/tests/runtime.rs +++ b/crates/irtt-server/tests/runtime.rs @@ -158,8 +158,8 @@ async fn wildcard_construction_is_refused_without_reply_source_selection() { // A current-thread runtime cannot receive while this test holds its thread. // Queue real packets before delaying the server: no synthetic ancillary data or -// transport hooks. A short queue delay selects kernel wall time; a delay beyond -// the documented one-second sanity bound falls back to userspace. Monotonic and +// transport hooks. Observed queue timing determines whether kernel wall time +// is plausible under the one-second bound, including scheduler overshoot. Monotonic and // midpoint timestamps must always remain at the later userspace observation. #[cfg(target_os = "linux")] #[tokio::test(flavor = "current_thread")] @@ -174,10 +174,10 @@ async fn queued_echoes_select_kernel_receive_wall_only_when_plausible() { ) .unwrap() }; - for (placement, delay, kernel_expected) in [ - (StampAt::Both, Duration::from_millis(100), true), - (StampAt::Both, Duration::from_millis(1100), false), - (StampAt::Midpoint, Duration::from_millis(100), false), + for (placement, delay) in [ + (StampAt::Both, Duration::from_millis(100)), + (StampAt::Both, Duration::from_millis(1100)), + (StampAt::Midpoint, Duration::from_millis(100)), ] { let mut server = Server::bind("127.0.0.1:0".parse().unwrap(), unthrottled()) .await @@ -238,23 +238,43 @@ async fn queued_echoes_select_kernel_receive_wall_only_when_plausible() { assert_eq!(reply.recv_count, Some(1)); assert_eq!(reply.recv_window, Some(1)); let timestamps = reply.timestamps; - if kernel_expected { + if placement == StampAt::Both { let receive = timestamps.recv_wall.unwrap(); - assert!( - receive >= before_send && receive <= queued_by, - "receive wall must identify the kernel arrival before the queue delay" - ); - let wall_elapsed = timestamps.send_wall.unwrap() - receive; - let mono_elapsed = timestamps.send_mono.unwrap() - timestamps.recv_mono.unwrap(); - assert!( - wall_elapsed - mono_elapsed >= 50_000_000, - "queue delay must not move the monotonic endpoint" - ); + let send = timestamps.send_wall.unwrap(); + let kernel_endpoint = (before_send..=queued_by).contains(&receive); + let userspace_endpoint = (resumed_at..=send).contains(&receive); + // Arrival lies in [before_send, queued_by], and core observation + // lies in [resumed_at, send]. Only demand one selection where these + // observed bounds put the whole lag interval on one side of 1 s. + // Scheduler overshoot can legitimately change the expected path. + if send - before_send <= 1_000_000_000 { + assert!( + kernel_endpoint, + "plausible arrival must select kernel wall time" + ); + } else if resumed_at - queued_by > 1_000_000_000 { + assert!( + userspace_endpoint, + "stale arrival must fall back to userspace" + ); + } else { + assert!( + kernel_endpoint || userspace_endpoint, + "boundary observation must identify a real endpoint" + ); + } + if kernel_endpoint { + let wall_elapsed = send - receive; + let mono_elapsed = timestamps.send_mono.unwrap() - timestamps.recv_mono.unwrap(); + assert!( + wall_elapsed - mono_elapsed >= 50_000_000, + "queue delay must not move the monotonic endpoint" + ); + } } else { - let receive = timestamps.recv_wall.or(timestamps.midpoint_wall).unwrap(); assert!( - receive >= resumed_at, - "fallback and midpoint must use userspace time" + timestamps.midpoint_wall.unwrap() >= resumed_at, + "midpoint must use userspace time" ); } stop_tx.send(()).unwrap();