From e3003c348fcba5b86955b9f514950db3d76ff759 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Sat, 15 Aug 2026 01:03:35 +0700 Subject: [PATCH 1/2] refactor(client): make managed failure classification exhaustive classify_client_error now matches every ClientError variant explicitly, so a new variant fails to compile here until its real classification is chosen. NoTokioRuntime classifies as InvalidState and DatagramLengthMismatch as Socket; ManagedTargetFailureKind::Other survives only as a deliberate category with no default path into it. ManagedTargetFailurePhase and ManagedTargetFailureKind gain controlled Display impls, and the CLI client/TUI failure diagnostics use them instead of leaking Debug enum spelling into user-facing text. --- crates/irtt-cli/src/cmd/client/run.rs | 8 +- crates/irtt-cli/src/cmd/tui/ui.rs | 14 +- crates/irtt-client/src/managed/model.rs | 270 +++++++++++++++++++++++- 3 files changed, 279 insertions(+), 13 deletions(-) diff --git a/crates/irtt-cli/src/cmd/client/run.rs b/crates/irtt-cli/src/cmd/client/run.rs index 60f5386..3c78a8d 100644 --- a/crates/irtt-cli/src/cmd/client/run.rs +++ b/crates/irtt-cli/src/cmd/client/run.rs @@ -277,13 +277,13 @@ fn target_failure_messages(target: &ManagedTargetOutcome) -> Vec { let mut messages = Vec::with_capacity(2); if let ManagedTargetEndReason::Failed(failure) = &target.end_reason { messages.push(format!( - "irtt-rs: target {} failed ({:?} {:?}): {}", + "irtt-rs: target {} failed ({} {}): {}", target.target.id, failure.phase, failure.kind, failure.message )); } if let Some(failure) = &target.cleanup_failure { messages.push(format!( - "irtt-rs: target {} cleanup failed ({:?} {:?}): {}", + "irtt-rs: target {} cleanup failed ({} {}): {}", target.target.id, failure.phase, failure.kind, failure.message )); } @@ -480,8 +480,8 @@ mod tests { assert_eq!( target_failure_messages(&target), [ - "irtt-rs: target edge failed (Receiving Protocol): primary failure", - "irtt-rs: target edge cleanup failed (Closing Socket): cleanup failure", + "irtt-rs: target edge failed (receiving protocol): primary failure", + "irtt-rs: target edge cleanup failed (closing socket): cleanup failure", ] ); } diff --git a/crates/irtt-cli/src/cmd/tui/ui.rs b/crates/irtt-cli/src/cmd/tui/ui.rs index d468bac..b1742e8 100644 --- a/crates/irtt-cli/src/cmd/tui/ui.rs +++ b/crates/irtt-cli/src/cmd/tui/ui.rs @@ -194,7 +194,7 @@ impl TuiState { (TargetStatus::Replaced, "target replaced".to_owned(), None) } ManagedTargetEndReason::Failed(failure) => { - let message = format!("{:?}: {}", failure.kind, failure.message); + let message = format!("{}: {}", failure.kind, failure.message); ( TargetStatus::Failed, format!("target failed: {message}"), @@ -205,7 +205,7 @@ impl TuiState { let cleanup_warning = outcome.cleanup_failure.as_ref().map(|failure| { format!( - "cleanup failed ({:?} {:?}): {}", + "cleanup failed ({} {}): {}", failure.phase, failure.kind, failure.message ) }); @@ -2430,7 +2430,7 @@ mod tests { assert!(state.targets[1] .last_warning .as_deref() - .is_some_and(|warning| warning.contains("Timeout"))); + .is_some_and(|warning| warning.contains("timeout"))); } #[test] @@ -2474,11 +2474,11 @@ mod tests { assert!(state.targets[0] .last_warning .as_deref() - .is_some_and(|warning| warning.contains("cleanup failed (Closing Socket)"))); + .is_some_and(|warning| warning.contains("cleanup failed (closing socket)"))); assert!(state .recent_events .iter() - .any(|event| event.contains("cleanup failed (Closing Socket)"))); + .any(|event| event.contains("cleanup failed (closing socket)"))); } #[test] @@ -2509,11 +2509,11 @@ mod tests { assert!(state .recent_events .iter() - .any(|event| event.contains("target failed: Protocol: primary failure"))); + .any(|event| event.contains("target failed: protocol: primary failure"))); assert!(state .recent_events .iter() - .any(|event| event.contains("cleanup failed (Closing Socket): cleanup failure"))); + .any(|event| event.contains("cleanup failed (closing socket): cleanup failure"))); } #[test] diff --git a/crates/irtt-client/src/managed/model.rs b/crates/irtt-client/src/managed/model.rs index fac057f..57c7cd8 100644 --- a/crates/irtt-client/src/managed/model.rs +++ b/crates/irtt-client/src/managed/model.rs @@ -1,4 +1,4 @@ -use std::{net::SocketAddr, sync::Arc, time::Duration}; +use std::{fmt, net::SocketAddr, sync::Arc, time::Duration}; use thiserror::Error; @@ -235,6 +235,20 @@ pub enum ManagedTargetFailurePhase { Closing, } +impl fmt::Display for ManagedTargetFailurePhase { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let text = match self { + Self::Connecting => "connecting", + Self::Opening => "opening", + Self::Sending => "sending", + Self::Receiving => "receiving", + Self::Timing => "timing", + Self::Closing => "closing", + }; + f.write_str(text) + } +} + /// Stable category for a target-local failure. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum ManagedTargetFailureKind { @@ -246,9 +260,31 @@ pub enum ManagedTargetFailureKind { Configuration, ResourceExhausted, InvalidState, + /// A failure that deliberately has no more specific category. + /// + /// This is a classification decision, not a fallback: every current + /// [`ClientError`] variant is classified explicitly, so nothing reaches + /// this category by default. Other, } +impl fmt::Display for ManagedTargetFailureKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let text = match self { + Self::Resolve => "resolve", + Self::Socket => "socket", + Self::SocketOption => "socket option", + Self::Protocol => "protocol", + Self::Timeout => "timeout", + Self::Configuration => "configuration", + Self::ResourceExhausted => "resource exhausted", + Self::InvalidState => "invalid state", + Self::Other => "other", + }; + f.write_str(text) + } +} + /// Durable target-local failure details. #[derive(Clone, Debug, Eq, PartialEq)] pub struct ManagedTargetFailure { @@ -350,6 +386,12 @@ pub enum ManagedDriverFailure { Internal { message: Arc }, } +/// Map a [`ClientError`] onto its durable managed failure classification. +/// +/// The match is deliberately exhaustive over every [`ClientError`] variant. A +/// new variant must fail to compile here until its real classification is +/// chosen, rather than silently degrading to +/// [`ManagedTargetFailureKind::Other`]. pub(crate) fn classify_client_error( phase: ManagedTargetFailurePhase, error: &ClientError, @@ -375,13 +417,15 @@ pub(crate) fn classify_client_error( | ClientError::CounterOverflow { .. } | ClientError::DurationOverflow | ClientError::PendingLimitExceeded { .. } => ManagedTargetFailureKind::ResourceExhausted, + ClientError::DatagramLengthMismatch { .. } => ManagedTargetFailureKind::Socket, + #[cfg(feature = "tokio")] + ClientError::NoTokioRuntime => ManagedTargetFailureKind::InvalidState, ClientError::NotOpen | ClientError::AlreadyOpen | ClientError::AlreadyCompleted | ClientError::AlreadyClosed | ClientError::StalePreparedProbe { .. } | ClientError::PendingSequenceCollision { .. } => ManagedTargetFailureKind::InvalidState, - _ => ManagedTargetFailureKind::Other, }; ManagedTargetFailure { phase, @@ -389,3 +433,225 @@ pub(crate) fn classify_client_error( message: Arc::from(error.to_string()), } } + +#[cfg(test)] +mod tests { + use std::{collections::TryReserveError, io}; + + use super::*; + + fn io_error() -> io::Error { + io::Error::other("boom") + } + + fn try_reserve_error() -> TryReserveError { + Vec::::new().try_reserve(usize::MAX).unwrap_err() + } + + fn classification_cases() -> Vec<(ClientError, ManagedTargetFailureKind)> { + vec![ + ( + ClientError::Resolve { + addr: "example.invalid:2112".to_owned(), + }, + ManagedTargetFailureKind::Resolve, + ), + ( + ClientError::Socket(io_error()), + ManagedTargetFailureKind::Socket, + ), + ( + ClientError::DatagramLengthMismatch { + expected: 64, + actual: 32, + }, + ManagedTargetFailureKind::Socket, + ), + ( + ClientError::SocketOption { + operation: "set read timeout", + remote: "127.0.0.1:2112".parse().unwrap(), + source: io_error(), + }, + ManagedTargetFailureKind::SocketOption, + ), + ( + ClientError::ReadTimeoutRestore { source: io_error() }, + ManagedTargetFailureKind::SocketOption, + ), + ( + ClientError::Protocol(irtt_proto::ProtoError::BadMagic), + ManagedTargetFailureKind::Protocol, + ), + ( + ClientError::ProtocolVersionMismatch { + requested: 1, + received: 2, + }, + ManagedTargetFailureKind::Protocol, + ), + (ClientError::ZeroToken, ManagedTargetFailureKind::Protocol), + ( + ClientError::UnexpectedNoTestReply, + ManagedTargetFailureKind::Protocol, + ), + ( + ClientError::NonZeroNoTestToken { token: 7 }, + ManagedTargetFailureKind::Protocol, + ), + ( + ClientError::ServerRejected, + ManagedTargetFailureKind::Protocol, + ), + ( + ClientError::NegotiationRejected { + reason: "duration".to_owned(), + }, + ManagedTargetFailureKind::Protocol, + ), + (ClientError::OpenTimeout, ManagedTargetFailureKind::Timeout), + ( + ClientError::InvalidConfig { + reason: "interval".to_owned(), + }, + ManagedTargetFailureKind::Configuration, + ), + ( + ClientError::OpenTimeoutTooSmall { + timeout: Duration::from_millis(1), + minimum: Duration::from_millis(10), + }, + ManagedTargetFailureKind::Configuration, + ), + ( + ClientError::NoOpenTimeouts, + ManagedTargetFailureKind::Configuration, + ), + ( + ClientError::AllocationFailed { + operation: "pending probes", + source: try_reserve_error(), + }, + ManagedTargetFailureKind::ResourceExhausted, + ), + ( + ClientError::CounterOverflow { + counter: "packets_sent", + }, + ManagedTargetFailureKind::ResourceExhausted, + ), + ( + ClientError::DurationOverflow, + ManagedTargetFailureKind::ResourceExhausted, + ), + ( + ClientError::PendingLimitExceeded { limit: 8 }, + ManagedTargetFailureKind::ResourceExhausted, + ), + ( + ClientError::NoTokioRuntime, + ManagedTargetFailureKind::InvalidState, + ), + (ClientError::NotOpen, ManagedTargetFailureKind::InvalidState), + ( + ClientError::AlreadyOpen, + ManagedTargetFailureKind::InvalidState, + ), + ( + ClientError::AlreadyCompleted, + ManagedTargetFailureKind::InvalidState, + ), + ( + ClientError::AlreadyClosed, + ManagedTargetFailureKind::InvalidState, + ), + ( + ClientError::StalePreparedProbe { + prepared_seq: 3, + next_wire_seq: 4, + }, + ManagedTargetFailureKind::InvalidState, + ), + ( + ClientError::PendingSequenceCollision { seq: 9 }, + ManagedTargetFailureKind::InvalidState, + ), + ] + } + + #[test] + fn every_client_error_classifies_to_its_documented_kind() { + for (error, expected) in classification_cases() { + let failure = classify_client_error(ManagedTargetFailurePhase::Opening, &error); + assert_eq!( + failure.kind, expected, + "unexpected classification for {error:?}" + ); + assert_eq!(failure.phase, ManagedTargetFailurePhase::Opening); + assert_eq!(&*failure.message, error.to_string()); + } + } + + #[test] + fn classification_covers_every_failure_kind_except_other() { + let observed: Vec<_> = classification_cases() + .into_iter() + .map(|(error, _)| { + classify_client_error(ManagedTargetFailurePhase::Sending, &error).kind + }) + .collect(); + for kind in [ + ManagedTargetFailureKind::Resolve, + ManagedTargetFailureKind::Socket, + ManagedTargetFailureKind::SocketOption, + ManagedTargetFailureKind::Protocol, + ManagedTargetFailureKind::Timeout, + ManagedTargetFailureKind::Configuration, + ManagedTargetFailureKind::ResourceExhausted, + ManagedTargetFailureKind::InvalidState, + ] { + assert!(observed.contains(&kind), "no case classifies as {kind}"); + } + assert!( + !observed.contains(&ManagedTargetFailureKind::Other), + "Other must remain a deliberate category, not a fallback" + ); + } + + #[test] + fn classification_preserves_the_reported_phase() { + for phase in [ + ManagedTargetFailurePhase::Connecting, + ManagedTargetFailurePhase::Opening, + ManagedTargetFailurePhase::Sending, + ManagedTargetFailurePhase::Receiving, + ManagedTargetFailurePhase::Timing, + ManagedTargetFailurePhase::Closing, + ] { + let failure = classify_client_error(phase, &ClientError::OpenTimeout); + assert_eq!(failure.phase, phase); + } + } + + #[test] + fn phase_and_kind_display_without_debug_spelling() { + assert_eq!( + ManagedTargetFailurePhase::Connecting.to_string(), + "connecting" + ); + assert_eq!(ManagedTargetFailurePhase::Closing.to_string(), "closing"); + assert_eq!( + ManagedTargetFailureKind::SocketOption.to_string(), + "socket option" + ); + assert_eq!( + ManagedTargetFailureKind::ResourceExhausted.to_string(), + "resource exhausted" + ); + assert_eq!( + ManagedTargetFailureKind::InvalidState.to_string(), + "invalid state" + ); + assert_eq!(ManagedTargetFailureKind::Other.to_string(), "other"); + } +} From 665f8393336e9a0c552f70f9480b7279d7994648 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Sat, 15 Aug 2026 01:49:22 +0700 Subject: [PATCH 2/2] test(client): drop the coverage-only classification enumeration The removed test only checked that every failure kind appeared somewhere in the same hand-maintained case table the mapping test already walks. It could not prove exhaustiveness or the absence of a fallback, which the compiler enforces, and it pinned the incidental fact that each category currently has an input. --- crates/irtt-client/src/managed/model.rs | 26 ------------------------- 1 file changed, 26 deletions(-) diff --git a/crates/irtt-client/src/managed/model.rs b/crates/irtt-client/src/managed/model.rs index 57c7cd8..7bb92d3 100644 --- a/crates/irtt-client/src/managed/model.rs +++ b/crates/irtt-client/src/managed/model.rs @@ -592,32 +592,6 @@ mod tests { } } - #[test] - fn classification_covers_every_failure_kind_except_other() { - let observed: Vec<_> = classification_cases() - .into_iter() - .map(|(error, _)| { - classify_client_error(ManagedTargetFailurePhase::Sending, &error).kind - }) - .collect(); - for kind in [ - ManagedTargetFailureKind::Resolve, - ManagedTargetFailureKind::Socket, - ManagedTargetFailureKind::SocketOption, - ManagedTargetFailureKind::Protocol, - ManagedTargetFailureKind::Timeout, - ManagedTargetFailureKind::Configuration, - ManagedTargetFailureKind::ResourceExhausted, - ManagedTargetFailureKind::InvalidState, - ] { - assert!(observed.contains(&kind), "no case classifies as {kind}"); - } - assert!( - !observed.contains(&ManagedTargetFailureKind::Other), - "Other must remain a deliberate category, not a fallback" - ); - } - #[test] fn classification_preserves_the_reported_phase() { for phase in [