refactor(client): make managed failure classification exhaustive #90

Merged
Lochnair merged 2 commits from refactor/exhaustive-client-failures into main 2026-08-15 02:49:37 +00:00
3 changed files with 279 additions and 13 deletions
Showing only changes of commit e3003c348f - Show all commits

No files matched your search

+4 -4
View File
@@ -277,13 +277,13 @@ fn target_failure_messages(target: &ManagedTargetOutcome) -> Vec<String> {
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",
]
);
}
+7 -7
View File
@@ -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]
+268 -2
View File
@@ -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<str> },
}
/// 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 { .. }
chatgpt-codex-connector[bot] commented 2026-08-14 18:08:44 +00:00 (Migrated from github.com)
Review

P2 Badge Remove the coverage-only category enumeration

This test only verifies that every failure-kind bucket appears in the same hand-maintained case table already exercised by the preceding test; it cannot establish exhaustiveness or absence of a fallback, which the production match enforces at compile time. It instead pins the incidental fact that each category currently has an input, so a valid future regrouping would require unrelated test churn. Repository guidance explicitly discourages tests that exercise helpers solely for coverage or duplicate stronger coverage.

AGENTS.md reference: AGENTS.md:L93-L101

Useful? React with 👍 / 👎.

**<sub><sub>![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)</sub></sub> Remove the coverage-only category enumeration** This test only verifies that every failure-kind bucket appears in the same hand-maintained case table already exercised by the preceding test; it cannot establish exhaustiveness or absence of a fallback, which the production match enforces at compile time. It instead pins the incidental fact that each category currently has an input, so a valid future regrouping would require unrelated test churn. Repository guidance explicitly discourages tests that exercise helpers solely for coverage or duplicate stronger coverage. AGENTS.md reference: [AGENTS.md:L93-L101](https://github.com/Lochnair/irtt-rs/blob/e3003c348fcba5b86955b9f514950db3d76ff759/AGENTS.md#L93-L101) Useful? React with 👍 / 👎.
Lochnair commented 2026-08-14 18:49:40 +00:00 (Migrated from github.com)
Review

Agreed — removed the test entirely; the mapping table plus compile-time exhaustiveness already carry that weight without pinning which categories happen to have inputs.

🤖 Addressed by Claude Code

Agreed — removed the test entirely; the mapping table plus compile-time exhaustiveness already carry that weight without pinning which categories happen to have inputs. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
| 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::<u8>::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");
}
}