refactor(client): centralize managed target accounting #95

Merged
Lochnair merged 2 commits from refactor/managed-target-locality into main 2026-08-15 03:06:18 +00:00
3 changed files with 16 additions and 114 deletions
Showing only changes of commit dd68608d56 - Show all commits

No files matched your search

+9 -5
View File
2
@@ -209,15 +209,19 @@ pub struct ManagedTargetOutcome {
pub end_reason: ManagedTargetEndReason,
/// Probes the underlying client reported as sent.
pub packets_sent: u64,
/// On-time unique echo replies.
/// Unique echo replies that arrived in order and before their probe timed
/// out.
///
/// A unique reply that arrived after its probe timed out is counted by
/// `late` instead, so this is the count of replies that arrived in time.
/// A unique reply that arrives behind an already-received sequence, or
/// after its own probe timed out, is counted by `late` instead.
pub replies_received: u64,
/// Replies for a sequence that had already been answered.
pub duplicates: u64,
/// Replies that arrived after their probe timed out, whether or not the
/// client could still match them to retained send state.
/// Late replies: those arriving behind an already-received sequence
/// (reordering) or after their own probe timed out.
///
/// This counts every late reply, whether or not the client could still
/// match it to retained send state.
pub late: u64,
/// Warning events observed for this target.
pub warning_events: u64,
+7 -6
View File
@@ -394,15 +394,16 @@ impl TargetState {
}
}
#[derive(Debug, Default, Eq, PartialEq)]
#[derive(Default)]
struct TargetCounters {
packets_sent: u64,
/// On-time unique echo replies.
/// Unique echo replies that arrived in order and before their probe timed
/// out.
///
/// A late unique reply is counted by `late` and deliberately not here, so
/// `replies_received` stays the count of replies that arrived within their
/// probe timeout. These counters are durable, unlike the lossy managed
/// presentation events.
/// A reply that arrives behind an already-received sequence, or after its
/// own probe timed out, is counted by `late` and deliberately not here.
/// These counters are durable, unlike the lossy managed presentation
/// events.
replies_received: u64,
duplicates: u64,
late: u64,
-103
View File
@@ -2954,106 +2954,3 @@ fn terminal_subscription_is_closed() {
Err(ManagedSubscribeError::Closed)
chatgpt-codex-connector[bot] commented 2026-08-14 18:39:32 +00:00 (Migrated from github.com)
Review

P1 Badge Remove implementation-only counter tests

These tests directly instantiate the private TargetCounters, call its private observe helper, and mirror its match-arm results, even though outcome-level tests such as overdue_queued_reply_is_lost_then_late and post_deadline_receive_sweep_drains_queued_packets_and_is_bounded already verify the durable behavior through ManagedTargetOutcome. This couples a harmless private refactor to persistent test rewrites; remove these tests or replace them with normal-interface coverage only if an externally meaningful case remains unprotected.

AGENTS.md reference: AGENTS.md:L93-L103

Useful? React with 👍 / 👎.

**<sub><sub>![P1 Badge](https://img.shields.io/badge/P1-orange?style=flat)</sub></sub> Remove implementation-only counter tests** These tests directly instantiate the private `TargetCounters`, call its private `observe` helper, and mirror its match-arm results, even though outcome-level tests such as `overdue_queued_reply_is_lost_then_late` and `post_deadline_receive_sweep_drains_queued_packets_and_is_bounded` already verify the durable behavior through `ManagedTargetOutcome`. This couples a harmless private refactor to persistent test rewrites; remove these tests or replace them with normal-interface coverage only if an externally meaningful case remains unprotected. AGENTS.md reference: [AGENTS.md:L93-L103](https://github.com/Lochnair/irtt-rs/blob/5506220f67af4385a9cdab50d5c720cff610d11e/AGENTS.md#L93-L103) Useful? React with 👍 / 👎.
Lochnair commented 2026-08-14 18:49:43 +00:00 (Migrated from github.com)
Review

Agreed — removed both tests and the helper; confirmed overdue_queued_reply_is_lost_then_late already asserts replies_received == 0 and late == 1 on the outcome, so this was duplicate coverage through private internals.

🤖 Addressed by Claude Code

Agreed — removed both tests and the helper; confirmed `overdue_queued_reply_is_lost_then_late` already asserts `replies_received == 0` and `late == 1` on the outcome, so this was duplicate coverage through private internals. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
));
}
/// Build the client events target accounting distinguishes.
fn accounting_events() -> Vec<ClientEvent> {
let remote: std::net::SocketAddr = "127.0.0.1:2112".parse().unwrap();
let at = ClientTimestamp {
mono: Instant::now(),
wall: SystemTime::UNIX_EPOCH,
};
let rtt = crate::RttSample {
raw: Duration::from_millis(1),
adjusted: None,
effective: crate::SignedDuration::from_nanos(1_000_000),
};
vec![
ClientEvent::EchoSent {
seq: 0,
remote,
scheduled_at: at.mono,
sent_at: at,
bytes: 64,
send_call: Duration::ZERO,
timer_error: Duration::ZERO,
},
ClientEvent::EchoReply {
seq: 0,
remote,
sent_at: at,
received_at: at,
rtt,
server_timing: None,
one_way: None,
received_stats: None,
bytes: 64,
packet_meta: crate::PacketMeta::default(),
},
ClientEvent::DuplicateReply {
seq: 0,
remote,
received_at: at,
bytes: 64,
},
ClientEvent::LateReply {
seq: 1,
highest_seen: 2,
remote,
sent_at: Some(at),
received_at: at,
rtt: Some(rtt),
server_timing: None,
one_way: None,
received_stats: None,
bytes: 64,
packet_meta: crate::PacketMeta::default(),
},
ClientEvent::EchoLoss {
seq: 2,
sent_at: at,
timeout_at: at.mono,
},
ClientEvent::Warning {
kind: crate::WarningKind::WrongToken,
message: "wrong token".to_owned(),
at,
},
]
}
#[test]
fn target_counters_account_for_each_observed_event_once() {
let mut counters = TargetCounters::default();
for event in accounting_events() {
counters.observe(&event);
}
// A late unique reply is counted as late, not as a received reply, and a
// probe send is not accounted here because the client owns packets_sent.
assert_eq!(counters.packets_sent, 0);
assert_eq!(counters.replies_received, 1);
assert_eq!(counters.duplicates, 1);
assert_eq!(counters.late, 1);
assert_eq!(counters.warning_events, 1);
}
#[test]
fn target_counters_ignore_events_without_a_durable_count() {
let ignored: Vec<ClientEvent> = accounting_events()
.into_iter()
.filter(|event| {
matches!(
event,
ClientEvent::EchoSent { .. } | ClientEvent::EchoLoss { .. }
)
})
.collect();
assert_eq!(ignored.len(), 2);
let mut counters = TargetCounters::default();
for event in ignored {
counters.observe(&event);
}
assert_eq!(counters, TargetCounters::default());
}