refactor(client): centralize managed target accounting #95

2026-08-14T18:36:25Z

Lochnair commented

2026-08-14 18:36:25 +00:00

(Migrated from github.com)

Summary

Narrow locality work inside the existing TargetRuntime. No new abstraction, no file moved, no lifecycle machinery.

Target-local accounting was dispersed across the managed driver:

the four event counter increments were written inline in publish_client_events, each spelled self.targets[index].counters.<field> = self.targets[index].counters.<field>.saturating_add(1);
the authoritative packets_sent was copied out of the underlying client at fifteen separate call sites, all spelled identically;
ManagedTargetOutcome was constructed twice from the same six counter fields — once in a free target_outcome function, once inline in finish_target.

Now:

TargetCounters::observe(&ClientEvent) owns per-event accounting and matches ClientEvent exhaustively, so a new event variant has to be classified deliberately rather than silently ignored by a wildcard.
TargetRuntime::sync_packets_sent(&AsyncClient) (and record_packets_sent for the send path, which already has the count in hand) own adopting the authoritative sent count.
TargetRuntime::outcome(end_reason, cleanup_failure) owns outcome construction; the free function and the inline copy are gone.
No counter field is read or written outside TargetRuntime/TargetCounters any more.

Behavior

No behavior change. Every existing managed test passes unchanged, including the outcome-counter tests.
replies_received semantics are preserved exactly — on-time unique echo replies only, with a late unique reply counted by late — and are now documented on both the private counter and the public ManagedTargetOutcome field, which previously had no documentation at all.
ManagedTargetOutcome's doc now states explicitly that these counters are durable, in contrast to the lossy ManagedEvent presentation stream.

Scope

Explicitly not changed:

No TimeoutScheduler was extracted.
No callback, effect, or lifecycle interface was introduced.
TargetState was not redesigned.
Durable counters were not replaced by irtt-stats, and public outcome semantics are unchanged.
No file was split, and TargetRuntime was not moved.

Verification

Run from the task worktree, all successful:

cargo fmt --all -- --check
cargo check --workspace --all-targets --all-features --locked
cargo test --workspace --all-targets --all-features --locked
cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
RUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-deps --locked
cargo check -p irtt-cli --no-default-features --features client --all-targets --locked
cargo check -p irtt-cli --no-default-features --features tui --all-targets --locked
cargo check -p irtt-cli --no-default-features --features server --all-targets --locked
git diff --check

Two focused tests cover the new accounting seam: every event kind accounted exactly once with a late unique reply counted as late rather than received, and events with no durable target-local count leaving the counters untouched.

## Summary Narrow locality work inside the existing `TargetRuntime`. No new abstraction, no file moved, no lifecycle machinery. Target-local accounting was dispersed across the managed driver: - the four event counter increments were written inline in `publish_client_events`, each spelled `self.targets[index].counters.<field> = self.targets[index].counters.<field>.saturating_add(1)`; - the authoritative `packets_sent` was copied out of the underlying client at **fifteen** separate call sites, all spelled identically; - `ManagedTargetOutcome` was constructed twice from the same six counter fields — once in a free `target_outcome` function, once inline in `finish_target`. Now: - `TargetCounters::observe(&ClientEvent)` owns per-event accounting and matches `ClientEvent` exhaustively, so a new event variant has to be classified deliberately rather than silently ignored by a wildcard. - `TargetRuntime::sync_packets_sent(&AsyncClient)` (and `record_packets_sent` for the send path, which already has the count in hand) own adopting the authoritative sent count. - `TargetRuntime::outcome(end_reason, cleanup_failure)` owns outcome construction; the free function and the inline copy are gone. - No counter field is read or written outside `TargetRuntime`/`TargetCounters` any more. ## Behavior - No behavior change. Every existing managed test passes unchanged, including the outcome-counter tests. - `replies_received` semantics are preserved exactly — on-time unique echo replies only, with a late unique reply counted by `late` — and are now documented on both the private counter and the public `ManagedTargetOutcome` field, which previously had no documentation at all. - `ManagedTargetOutcome`'s doc now states explicitly that these counters are durable, in contrast to the lossy `ManagedEvent` presentation stream. ## Scope Explicitly not changed: - No `TimeoutScheduler` was extracted. - No callback, effect, or lifecycle interface was introduced. - `TargetState` was not redesigned. - Durable counters were not replaced by `irtt-stats`, and public outcome semantics are unchanged. - No file was split, and `TargetRuntime` was not moved. ## Verification Run from the task worktree, all successful: - `cargo fmt --all -- --check` - `cargo check --workspace --all-targets --all-features --locked` - `cargo test --workspace --all-targets --all-features --locked` - `cargo clippy --workspace --all-targets --all-features --locked -- -D warnings` - `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-deps --locked` - `cargo check -p irtt-cli --no-default-features --features client --all-targets --locked` - `cargo check -p irtt-cli --no-default-features --features tui --all-targets --locked` - `cargo check -p irtt-cli --no-default-features --features server --all-targets --locked` - `git diff --check` Two focused tests cover the new accounting seam: every event kind accounted exactly once with a late unique reply counted as late rather than received, and events with no durable target-local count leaving the counters untouched.

👍 1

chatgpt-codex-connector[bot] (Migrated from github.com) reviewed

2026-08-14 18:39:32 +00:00

chatgpt-codex-connector[bot] (Migrated from github.com) left a comment

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5506220f67

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

### 💡 Codex Review Here are some automated review suggestions for this pull request. **Reviewed commit:** `5506220f67` <details> <summary>ℹ️ About Codex in GitHub</summary> [Your team has set up Codex to review pull requests in this repo](https://chatgpt.com/codex/cloud/settings/general). Reviews are triggered when you - Open a pull request for review - Mark a draft as ready - Comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". </details>

crates/irtt-client/src/managed/model.rs

						
				@@ -198,16 +198,32 @@ pub enum ManagedEndReason {

				}

chatgpt-codex-connector[bot] (Migrated from github.com) commented

2026-08-14 18:39:32 +00:00

Correct the timeout-only description of late replies

When replies arrive out of order but before either probe times out—for example, sequence 1 followed by sequence 0—process_echo_reply emits LateReply for sequence 0 because it is behind highest_received_seq. Consequently this outcome increments late and not replies_received despite arriving within its timeout, so the new public documentation incorrectly defines both counters in timeout-only terms and may lead callers to interpret reordered replies as timed-out probes.

Useful? React with 👍 / 👎.

**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Correct the timeout-only description of late replies** When replies arrive out of order but before either probe times out—for example, sequence 1 followed by sequence 0—`process_echo_reply` emits `LateReply` for sequence 0 because it is behind `highest_received_seq`. Consequently this outcome increments `late` and not `replies_received` despite arriving within its timeout, so the new public documentation incorrectly defines both counters in timeout-only terms and may lead callers to interpret reordered replies as timed-out probes. Useful? React with 👍 / 👎.

crates/irtt-client/src/managed/tests.rs

chatgpt-codex-connector[bot] (Migrated from github.com) commented

2026-08-14 18:39:32 +00:00

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 👍 / 👎.

**![P1 Badge](https://img.shields.io/badge/P1-orange?style=flat) 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 (Migrated from github.com) reviewed

2026-08-14 18:49:43 +00:00

crates/irtt-client/src/managed/tests.rs

Lochnair (Migrated from github.com) commented

2026-08-14 18:49:43 +00:00

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)_

Lochnair (Migrated from github.com) reviewed

2026-08-14 18:49:44 +00:00

crates/irtt-client/src/managed/model.rs

						
				@@ -198,16 +198,32 @@ pub enum ManagedEndReason {

				}

Lochnair (Migrated from github.com) commented

2026-08-14 18:49:44 +00:00

Correct — a reply behind highest_received_seq is classified late by process_echo_reply while still pending, so both counters are now documented in terms of order and timeout rather than timeout alone.

🤖 Addressed by Claude Code

Correct — a reply behind `highest_received_seq` is classified late by `process_echo_reply` while still pending, so both counters are now documented in terms of order and timeout rather than timeout alone. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Lochnair-Labs/irtt-rs#95