Conversation
…estamp - Removed last_keepalive_ms field and replaced it with last_sent to track the last time any data was sent. - Added fields for bitrate tracking: bytes_sent_total, bytes_sent_window, last_rate_update_ms, and current_bitrate_bps. - Implemented methods to update and calculate bitrate based on sent data, matching the Android C implementation. - Updated connection status logging to include total bitrate across all connections. - Adjusted tests to reflect changes in connection behavior regarding keepalive and bitrate tracking.
- Introduced new structs: RttTracker, CongestionControl, BitrateTracker, and ReconnectionState to encapsulate related state management. - Replaced individual fields in SrtlaConnection with these new structs for better organization and clarity. - Updated methods to utilize the new structures for tracking RTT measurements, congestion control, and bitrate calculations. - Adjusted tests and helper functions to accommodate the new structure and ensure proper functionality. - Enhanced logging to reflect changes in connection state and metrics.
…tructure Split the 1383-line sender.rs into a sender/ directory with 6 focused modules for better maintainability and code organization: - sequence.rs (76 lines): Sequence tracking data structures and cleanup - uplink.rs (120 lines): Uplink reader tasks and packet queue management - selection.rs (135 lines): Connection selection algorithm and quality scoring - housekeeping.rs (140 lines): Keepalive, timeout, and cleanup tasks - packet_handler.rs (346 lines): SRT packet handling and forwarding - mod.rs (639 lines): Main event loop, public API, and utilities This follows the pattern established in the connection/ directory and makes it easier to understand and modify specific functionality. All 115 tests pass.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughSplit large monolithic connection and sender modules into focused submodules (connection/{bitrate,congestion,incoming,reconnection,rtt,socket} and sender/{uplink,packet_handler,selection,sequence,housekeeping}), added protocol parsers/builders, moved alloc/global startup helpers, bumped tokio and tracing-subscriber, added mimalloc, and updated tests to the new nested connection API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Uplink Reader
participant S as Sender Loop
participant P as PacketHandler
participant Sel as Selection
participant C as SrtlaConnection
participant H as Housekeeping
rect rgba(232,246,255,0.9)
U->>S: emit UplinkPacket(conn_id, bytes)
S->>P: dispatch packet
P->>Sel: select_connection_idx(conns, toggles...)
Sel-->>P: chosen_idx
P->>C: send_data_with_tracking(data, seq)
C-->>P: ack / state updates (rtt/congestion/bitrate)
P->>S: update sequence tracking
end
rect rgba(255,244,230,0.9)
Note right of H: periodic maintenance
S->>H: handle_housekeeping()
H->>C: perform_window_recovery()/send_keepalive()/calculate_bitrate()
C-->>H: metrics/state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/registration.rs (1)
54-58: Reuse a single RNG instance innew
Line [55] and Line [57] each acquire a fresh thread RNG. Grabbing it once keeps the intent clear and avoids redundant calls.- let mut id = [0u8; SRTLA_ID_LEN]; - rand::rng().fill_bytes(&mut id); - let mut probe_id = [0u8; SRTLA_ID_LEN]; - rand::rng().fill_bytes(&mut probe_id); + let mut rng = rand::rng(); + let mut id = [0u8; SRTLA_ID_LEN]; + rng.fill_bytes(&mut id); + let mut probe_id = [0u8; SRTLA_ID_LEN]; + rng.fill_bytes(&mut probe_id);src/protocol.rs (1)
106-116: Avoid the extra Vec allocation increate_ack_packet.
SmallVec::from_vec(vec![...])always spins up a heap-backedVecbefore moving it into the smallvec, so we lose the inline-capacity benefit on every call. You can build the buffer in-place withresizeinstead.Apply this diff:
- let mut pkt = SmallVec::from_vec(vec![0u8; 4 + 4 * acks.len()]); + let mut pkt = SmallVec::<u8, 64>::new(); + pkt.resize(4 + 4 * acks.len(), 0);src/test_helpers.rs (1)
38-47: Capture the timestamp once when seedingReconnectionState.Calling
now_ms()twice can yield slightly different values; taking it once keepsconnection_established_msandstartup_grace_deadline_msin sync and avoids double syscalls.Apply this diff:
- SrtlaConnection { + let now = now_ms(); + + SrtlaConnection { @@ - reconnection: ReconnectionState { - connection_established_ms: now_ms(), - startup_grace_deadline_ms: now_ms(), - ..Default::default() - }, + reconnection: ReconnectionState { + connection_established_ms: now, + startup_grace_deadline_ms: now, + ..Default::default() + }, @@ - let conn = SrtlaConnection { + let now = now_ms(); + let conn = SrtlaConnection { @@ - reconnection: ReconnectionState { - connection_established_ms: now_ms(), - startup_grace_deadline_ms: now_ms(), - ..Default::default() - }, + reconnection: ReconnectionState { + connection_established_ms: now, + startup_grace_deadline_ms: now, + ..Default::default() + },Also applies to: 75-83
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.toml(2 hunks)src/connection.rs(0 hunks)src/connection/bitrate.rs(1 hunks)src/connection/congestion.rs(1 hunks)src/connection/incoming.rs(1 hunks)src/connection/mod.rs(1 hunks)src/connection/reconnection.rs(1 hunks)src/connection/rtt.rs(1 hunks)src/connection/socket.rs(1 hunks)src/lib.rs(1 hunks)src/main.rs(1 hunks)src/protocol.rs(3 hunks)src/registration.rs(1 hunks)src/sender.rs(0 hunks)src/sender/housekeeping.rs(1 hunks)src/sender/mod.rs(1 hunks)src/sender/packet_handler.rs(1 hunks)src/sender/selection.rs(1 hunks)src/sender/sequence.rs(1 hunks)src/sender/uplink.rs(1 hunks)src/test_helpers.rs(3 hunks)src/tests/connection_tests.rs(17 hunks)src/tests/integration_tests.rs(0 hunks)src/tests/sender_tests.rs(4 hunks)src/toggles.rs(0 hunks)
💤 Files with no reviewable changes (4)
- src/tests/integration_tests.rs
- src/toggles.rs
- src/sender.rs
- src/connection.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: All Rust code must pass cargo fmt --all -- --check
All Rust code must pass clippy with -D warnings (no warnings allowed)
Use anyhow::Result for error propagation
Group imports as std → external → crate with module-level granularity (imports_granularity = "Module", group_imports = "StdExternalCrate")
Follow naming conventions: constants SCREAMING_SNAKE_CASE
Follow naming conventions: structs PascalCase
Follow naming conventions: functions snake_case
Follow naming conventions: modules snake_case (file/module names)
Use tracing macros (debug!, info!, warn!, etc.) for logging
Use Tokio for async runtime (net, time, io, signal) and async operations
Keep code self-documenting; add doc comments for public API when necessary
Files:
src/main.rssrc/sender/selection.rssrc/registration.rssrc/connection/congestion.rssrc/sender/sequence.rssrc/sender/housekeeping.rssrc/connection/incoming.rssrc/connection/rtt.rssrc/connection/reconnection.rssrc/sender/mod.rssrc/sender/uplink.rssrc/sender/packet_handler.rssrc/tests/sender_tests.rssrc/connection/bitrate.rssrc/test_helpers.rssrc/connection/socket.rssrc/connection/mod.rssrc/lib.rssrc/tests/connection_tests.rssrc/protocol.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Expose internal fields for testing only behind #[cfg(feature = "test-internals")]
Files:
src/main.rssrc/sender/selection.rssrc/registration.rssrc/connection/congestion.rssrc/sender/sequence.rssrc/sender/housekeeping.rssrc/connection/incoming.rssrc/connection/rtt.rssrc/connection/reconnection.rssrc/sender/mod.rssrc/sender/uplink.rssrc/sender/packet_handler.rssrc/tests/sender_tests.rssrc/connection/bitrate.rssrc/test_helpers.rssrc/connection/socket.rssrc/connection/mod.rssrc/lib.rssrc/tests/connection_tests.rssrc/protocol.rs
src/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Place integration, end-to-end, and protocol tests under src/tests/
Files:
src/tests/sender_tests.rssrc/tests/connection_tests.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Set Rust edition to 2024 and target MSRV 1.87 (requires nightly toolchain)
Maintain build profiles: dev, release-debug (thin LTO), release-lto (full LTO, stripped)
Files:
Cargo.toml
🧠 Learnings (6)
📚 Learning: 2025-10-15T14:24:40.523Z
Learnt from: CR
Repo: irlserver/srtla_send PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T14:24:40.523Z
Learning: Applies to src/tests/**/*.rs : Place integration, end-to-end, and protocol tests under src/tests/
Applied to files:
src/tests/sender_tests.rssrc/tests/connection_tests.rs
📚 Learning: 2025-10-15T14:24:40.523Z
Learnt from: CR
Repo: irlserver/srtla_send PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T14:24:40.523Z
Learning: Applies to src/**/*.rs : Expose internal fields for testing only behind #[cfg(feature = "test-internals")]
Applied to files:
src/test_helpers.rssrc/lib.rssrc/tests/connection_tests.rs
📚 Learning: 2025-10-15T14:24:40.523Z
Learnt from: CR
Repo: irlserver/srtla_send PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T14:24:40.523Z
Learning: Applies to **/*.rs : Use Tokio for async runtime (net, time, io, signal) and async operations
Applied to files:
Cargo.toml
📚 Learning: 2025-10-15T14:24:40.523Z
Learnt from: CR
Repo: irlserver/srtla_send PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T14:24:40.523Z
Learning: Applies to Cargo.toml : Set Rust edition to 2024 and target MSRV 1.87 (requires nightly toolchain)
Applied to files:
Cargo.toml
📚 Learning: 2025-10-15T14:24:40.523Z
Learnt from: CR
Repo: irlserver/srtla_send PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T14:24:40.523Z
Learning: Applies to rustfmt.toml : Use the project's rustfmt configuration with unstable features enabled
Applied to files:
Cargo.toml
📚 Learning: 2025-10-05T18:50:56.382Z
Learnt from: datagutt
Repo: irlserver/srtla_send PR: 3
File: src/connection.rs:29-32
Timestamp: 2025-10-05T18:50:56.382Z
Learning: In Rust projects using smallvec version 2.0 alpha (2.0.0-alpha.*), the API uses const generics with the syntax SmallVec<T, N> (e.g., SmallVec<Vec<u8>, 4>, SmallVec<u32, 4>), not the array form SmallVec<[T; N]> used in stable 1.x versions.
Applied to files:
src/protocol.rs
🧬 Code graph analysis (14)
src/sender/selection.rs (2)
src/utils.rs (1)
now_ms(7-12)src/connection/mod.rs (2)
total_nak_count(509-511)nak_burst_count(513-515)
src/connection/congestion.rs (2)
src/utils.rs (1)
now_ms(7-12)src/connection/mod.rs (4)
handle_nak(360-377)nak_burst_count(513-515)perform_window_recovery(474-477)time_since_last_nak_ms(505-507)
src/sender/sequence.rs (1)
src/utils.rs (1)
now_ms(7-12)
src/sender/housekeeping.rs (3)
src/sender/sequence.rs (1)
cleanup_expired_sequence_tracking(20-76)src/sender/uplink.rs (1)
restart_reader_for(96-113)src/utils.rs (1)
now_ms(7-12)
src/connection/rtt.rs (3)
src/protocol.rs (1)
extract_keepalive_timestamp(90-102)src/utils.rs (1)
now_ms(7-12)src/connection/mod.rs (1)
connection_established_ms(517-519)
src/connection/reconnection.rs (2)
src/utils.rs (1)
now_ms(7-12)src/connection/mod.rs (1)
should_attempt_reconnect(521-523)
src/sender/mod.rs (8)
src/sender/housekeeping.rs (1)
handle_housekeeping(17-140)src/sender/packet_handler.rs (3)
drain_packet_queue(152-177)handle_srt_packet(180-285)handle_uplink_packet(103-149)src/sender/selection.rs (2)
calculate_quality_multiplier(7-46)select_connection_idx(48-135)src/sender/uplink.rs (2)
create_uplink_channel(115-120)sync_readers(68-94)src/utils.rs (1)
now_ms(7-12)src/registration.rs (3)
new(53-71)has_connected(432-434)active_connections(428-430)src/toggles.rs (1)
new(25-31)src/connection/mod.rs (6)
connect_from_ip(99-132)current_bitrate_mbps(539-541)get_smooth_rtt_ms(442-444)get_fast_rtt_ms(446-448)get_rtt_jitter_ms(450-452)is_rtt_stable(438-440)
src/sender/uplink.rs (1)
src/sender/mod.rs (1)
std(92-92)
src/sender/packet_handler.rs (3)
src/sender/selection.rs (1)
select_connection_idx(48-135)src/utils.rs (1)
now_ms(7-12)src/protocol.rs (1)
get_srt_sequence_number(54-64)
src/tests/sender_tests.rs (2)
src/utils.rs (1)
now_ms(7-12)src/sender/selection.rs (1)
calculate_quality_multiplier(7-46)
src/connection/bitrate.rs (1)
src/utils.rs (1)
now_ms(7-12)
src/test_helpers.rs (1)
src/utils.rs (1)
now_ms(7-12)
src/connection/mod.rs (8)
src/sender/mod.rs (2)
std(92-92)None(93-93)src/connection/socket.rs (2)
bind_from_ip(7-39)resolve_remote(41-48)src/utils.rs (1)
now_ms(7-12)src/connection/bitrate.rs (1)
default(13-20)src/connection/rtt.rs (1)
default(22-35)src/protocol.rs (6)
create_keepalive_packet(80-88)create_reg2_packet(73-78)get_packet_type(46-51)parse_srt_ack(120-128)parse_srt_nak(131-160)parse_srtla_ack(163-182)src/connection/congestion.rs (3)
handle_nak(34-90)perform_window_recovery(181-255)time_since_last_nak_ms(257-263)src/connection/reconnection.rs (1)
should_attempt_reconnect(17-46)
src/tests/connection_tests.rs (3)
src/utils.rs (1)
now_ms(7-12)src/connection/mod.rs (3)
nak_burst_count(513-515)needs_keepalive(459-472)needs_rtt_measurement(454-457)src/test_helpers.rs (1)
create_test_connection(16-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test (macOS)
- GitHub Check: Test
- GitHub Check: Test (Windows)
🔇 Additional comments (2)
src/sender/packet_handler.rs (1)
252-259: Fix exploration timer: passingInstant::now()breaks the modulo window
select_connection_idxexpects a long-lived reference instant and calls.elapsed()on it to decide when to explore. Because we pass a freshly-createdInstant::now()every time,elapsed()is always ~0 ms, so whenever exploration is enabled we always fall into the exploration branch and select the second-best connection. That regresses routing quality as soon as the toggle is flipped on. Please thread a stable epoch (e.g. recordlet selection_epoch = Instant::now();at startup and pass it through) or rework the selector to use an absolute monotonic source such asnow_ms()so the modulo window advances correctly.commentaryraise_major_issuesrc/sender/mod.rs (1)
92-114: Avoid blocking the runtime onstd::sync::mpsc::recv()This async task is awaiting
instant_rx.recv()from astd::sync::mpscchannel.recv()blocks the thread, so on a single-threaded runtime (or under load) it will park the executor and starve every other future. Please move to a non-blocking primitive—e.g. replace this channel withtokio::sync::mpsc::unbounded_channel(and adjust the call sites to use the async sender/receiver) so the loop canawaitwithout blocking:- let (instant_tx, instant_rx) = std::sync::mpsc::channel::<SmallVec<u8, 64>>(); + let (instant_tx, mut instant_rx) = tokio::sync::mpsc::unbounded_channel::<SmallVec<u8, 64>>(); ... - tokio::spawn(async move { - while let Ok(ack_packet) = instant_rx.recv() { + tokio::spawn(async move { + while let Some(ack_packet) = instant_rx.recv().await { ... - let _ = local_listener_clone.send_to(&ack_packet, client).await; + if let Err(e) = local_listener_clone.send_to(&ack_packet, client).await { + warn!("instant ACK send failed: {e}"); + }This keeps the entire sender loop non-blocking and maintains executor health.commentaryraise_major_issue
There was a problem hiding this comment.
Pull Request Overview
This PR implements significant refactoring with enhanced logging and mimalloc integration. The main changes include:
- Replaced
last_keepalive_msfield withlast_sent(Instant-based) to track all data sends, not just keepalives - Added comprehensive bitrate tracking matching the Android C implementation
- Refactored connection state into organized sub-structs (RttTracker, CongestionControl, BitrateTracker, ReconnectionState)
- Split large sender.rs and connection.rs files into modular components
- Integrated mimalloc as the global allocator on non-Windows platforms
- Updated deprecated
rand::thread_rng()torand::rng()
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Added mimalloc dependency and updated tokio/tracing-subscriber versions |
| Cargo.lock | Dependency version updates including mimalloc |
| src/main.rs | Added mimalloc global allocator for non-Windows |
| src/lib.rs | Added mimalloc global allocator for tests on non-Windows |
| src/protocol.rs | Removed module-level dead_code allow, added granular annotations |
| src/registration.rs | Updated deprecated rand API usage |
| src/toggles.rs | Removed dead_code annotation from DynamicToggles::new() |
| src/connection/mod.rs | Main connection logic with refactored field organization |
| src/connection/bitrate.rs | New bitrate tracking implementation |
| src/connection/congestion.rs | Extracted congestion control logic |
| src/connection/rtt.rs | Extracted RTT measurement logic |
| src/connection/reconnection.rs | Extracted reconnection state logic |
| src/connection/socket.rs | Extracted socket utilities |
| src/connection/incoming.rs | Extracted incoming packet struct |
| src/sender/mod.rs | Main sender loop with bitrate logging |
| src/sender/housekeeping.rs | Extracted housekeeping logic with bitrate calculation calls |
| src/sender/packet_handler.rs | Extracted packet handling logic |
| src/sender/selection.rs | Extracted connection selection logic |
| src/sender/sequence.rs | Extracted sequence tracking logic |
| src/sender/uplink.rs | Extracted uplink reader management |
| src/test_helpers.rs | Updated test helpers for new field structure |
| src/tests/*.rs | Updated tests to use new field paths (e.g., conn.congestion.nak_count) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…connection - Introduced a reset method in BitrateTracker to clear all tracking state for fresh measurements. - Updated SrtlaConnection to call the reset method on the bitrate tracker after a reconnection, ensuring accurate bitrate calculations moving forward.
… selection - Added a static `STARTUP_INSTANT` for consistent timing calculations based on program startup. - Introduced `elapsed_ms` function to retrieve elapsed time since startup. - Refactored `select_connection_idx` to use `elapsed_ms` instead of passing `Instant::now()`, improving code clarity and consistency. - Updated tests to reflect changes in connection selection logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/connection/mod.rs (1)
543-584: Bitrate reset concern resolved; consider refactoring manual state resets.The bitrate tracker is now correctly reset at line 578, addressing the previous review concern about stale measurement windows after reconnect.
However, the method manually resets numerous congestion and RTT fields (lines 556-574) instead of delegating to helper methods like
self.congestion.reset()orself.rtt.reset(). This approach is error-prone—new fields added to those submodules might be missed during reconnect, leading to subtle bugs.Consider adding
reset()methods toCongestionControlandRttTracker(similar toBitrateTracker::reset()used at line 578) to encapsulate state-clearing logic:// In src/connection/congestion.rs impl CongestionControl { pub fn reset(&mut self) { self.nak_count = 0; self.last_nak_time_ms = 0; self.nak_burst_count = 0; self.nak_burst_start_time_ms = 0; self.last_window_increase_ms = 0; self.consecutive_acks_without_nak = 0; self.fast_recovery_mode = false; self.fast_recovery_start_ms = 0; } } // In src/connection/rtt.rs impl RttTracker { pub fn reset(&mut self) { self.last_rtt_measurement_ms = 0; self.smooth_rtt_ms = 0.0; self.fast_rtt_ms = 0.0; self.rtt_jitter_ms = 0.0; self.prev_rtt_ms = 0.0; self.rtt_avg_delta_ms = 0.0; self.rtt_min_ms = 200.0; self.estimated_rtt_ms = 0.0; self.last_keepalive_sent_ms = 0; self.waiting_for_keepalive_response = false; } }Then simplify
reconnect():- self.congestion.nak_count = 0; - self.congestion.last_nak_time_ms = 0; - self.congestion.nak_burst_count = 0; - self.congestion.nak_burst_start_time_ms = 0; - self.congestion.last_window_increase_ms = 0; - self.congestion.consecutive_acks_without_nak = 0; - self.congestion.fast_recovery_mode = false; - self.rtt.last_rtt_measurement_ms = 0; - self.rtt.smooth_rtt_ms = 0.0; - self.rtt.fast_rtt_ms = 0.0; - self.rtt.rtt_jitter_ms = 0.0; - self.rtt.prev_rtt_ms = 0.0; - self.rtt.rtt_avg_delta_ms = 0.0; - self.rtt.rtt_min_ms = 200.0; - self.rtt.estimated_rtt_ms = 0.0; - self.rtt.last_keepalive_sent_ms = 0; - self.rtt.waiting_for_keepalive_response = false; + self.congestion.reset(); + self.rtt.reset(); self.packet_send_times_ms = [0; PKT_LOG_SIZE]; - self.congestion.fast_recovery_start_ms = 0; self.reconnection.last_reconnect_attempt_ms = now_ms(); self.reconnection.reconnect_failure_count = 0; - // Reset bitrate tracker to start fresh measurement window after reconnect self.bitrate.reset();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/connection/bitrate.rs(1 hunks)src/connection/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: All Rust code must pass cargo fmt --all -- --check
All Rust code must pass clippy with -D warnings (no warnings allowed)
Use anyhow::Result for error propagation
Group imports as std → external → crate with module-level granularity (imports_granularity = "Module", group_imports = "StdExternalCrate")
Follow naming conventions: constants SCREAMING_SNAKE_CASE
Follow naming conventions: structs PascalCase
Follow naming conventions: functions snake_case
Follow naming conventions: modules snake_case (file/module names)
Use tracing macros (debug!, info!, warn!, etc.) for logging
Use Tokio for async runtime (net, time, io, signal) and async operations
Keep code self-documenting; add doc comments for public API when necessary
Files:
src/connection/bitrate.rssrc/connection/mod.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Expose internal fields for testing only behind #[cfg(feature = "test-internals")]
Files:
src/connection/bitrate.rssrc/connection/mod.rs
🧬 Code graph analysis (2)
src/connection/bitrate.rs (1)
src/utils.rs (1)
now_ms(7-12)
src/connection/mod.rs (7)
src/connection/socket.rs (2)
bind_from_ip(7-39)resolve_remote(41-48)src/utils.rs (1)
now_ms(7-12)src/connection/bitrate.rs (1)
default(13-20)src/connection/rtt.rs (1)
default(22-35)src/protocol.rs (6)
create_keepalive_packet(80-88)create_reg2_packet(73-78)get_packet_type(46-51)parse_srt_ack(120-128)parse_srt_nak(131-160)parse_srtla_ack(163-182)src/connection/congestion.rs (3)
handle_nak(34-90)perform_window_recovery(181-255)time_since_last_nak_ms(257-263)src/connection/reconnection.rs (1)
should_attempt_reconnect(17-46)
🔇 Additional comments (4)
src/connection/bitrate.rs (1)
1-63: LGTM! Well-structured bitrate tracker with robust overflow protection.The implementation correctly tracks bitrate over a 2-second window with appropriate safeguards:
- Saturating arithmetic prevents overflow (lines 35, 43, 46, 50)
- Division-by-zero protection via the
time_diff_ms > 0check (line 48)- Clean separation of concerns with dedicated methods
- Proper use of
now_ms()for timestamp initialization and updatesThe public fields align with the test-internals pattern used elsewhere in the codebase.
src/connection/mod.rs (3)
1-132: Clean modular structure with proper initialization.The connection module is well-organized with dedicated submodules for bitrate, congestion, RTT, and reconnection tracking. The constructor properly initializes all state, including the startup grace period for reconnection backoff.
The use of feature gates for test-internals is consistent with the coding guidelines and provides appropriate visibility control.
147-157: Proper integration of bitrate tracking.The method correctly updates bitrate statistics via
update_on_send()and trackslast_sentfor keepalive decisions, aligning with the PR objectives to replacelast_keepalive_mswithlast_sentfor broader activity tracking.
459-472: Correct keepalive logic matches PR objectives.The implementation properly checks
last_sent(line 468) rather than a separatelast_keepalive_msfield, preventing unnecessary keepalives during active transmission. This aligns with the C implementation behavior described in the PR summary.
- Implemented reset methods in CongestionControl and RttTracker to clear all state variables, facilitating a clean slate during reconnections. - Updated SrtlaConnection to utilize these reset methods, ensuring accurate state management across reconnections.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/sender/packet_handler.rs (2)
162-175: Consider limiting batch size for better async runtime behavior.The current implementation drains all available packets in a tight loop using
try_recv(). In high-throughput scenarios, this could monopolize the async runtime. Consider adding a batch size limit and yielding periodically to allow other tasks to progress.Example approach:
pub async fn drain_packet_queue( packet_rx: &mut UnboundedReceiver<UplinkPacket>, connections: &mut [SrtlaConnection], reg: &mut SrtlaRegistrationManager, instant_tx: &std::sync::mpsc::Sender<SmallVec<u8, 64>>, last_client_addr: Option<SocketAddr>, local_listener: &UdpSocket, seq_to_conn: &mut HashMap<u32, SequenceTrackingEntry>, seq_order: &mut VecDeque<u32>, toggles: &DynamicToggles, ) { + const MAX_BATCH_SIZE: usize = 100; + let mut processed = 0; while let Ok(packet) = packet_rx.try_recv() { handle_uplink_packet( packet, connections, reg, instant_tx, last_client_addr, local_listener, seq_to_conn, seq_order, toggles, ) .await; + processed += 1; + if processed >= MAX_BATCH_SIZE { + break; + } } }
198-220: Consider simplifying pre-registration connection selection.The nested conditionals for selecting a connection during pre-registration are complex and could be refactored for clarity. The logic is correct but difficult to follow.
Consider extracting the selection logic into a helper function:
fn select_pre_registration_connection( connections: &[SrtlaConnection], last_selected_idx: Option<usize>, ) -> Option<usize> { // Try last selected if still valid if let Some(idx) = last_selected_idx { if let Some(conn) = connections.get(idx) { if conn.connected && !conn.is_timed_out() { return Some(idx); } } } // Find any non-timed-out connection connections .iter() .enumerate() .find(|(_, c)| !c.is_timed_out()) .map(|(i, _)| i) }Then use it in the main function:
-let sel_idx = if let Some(idx) = last_selected_idx { - if let Some(conn) = connections.get(*idx) { - if conn.connected && !conn.is_timed_out() { - Some(*idx) - } else { - None - } - } else { - None - } -} else { - connections - .iter() - .enumerate() - .find(|(_, c)| !c.is_timed_out()) - .map(|(i, _)| i) - .or_else(|| { - connections - .first() - .and_then(|c| if !c.is_timed_out() { Some(0) } else { None }) - }) -}; +let sel_idx = select_pre_registration_connection(connections, *last_selected_idx);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/sender/packet_handler.rs(1 hunks)src/sender/selection.rs(1 hunks)src/tests/sender_tests.rs(7 hunks)src/utils.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sender/selection.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: All Rust code must pass cargo fmt --all -- --check
All Rust code must pass clippy with -D warnings (no warnings allowed)
Use anyhow::Result for error propagation
Group imports as std → external → crate with module-level granularity (imports_granularity = "Module", group_imports = "StdExternalCrate")
Follow naming conventions: constants SCREAMING_SNAKE_CASE
Follow naming conventions: structs PascalCase
Follow naming conventions: functions snake_case
Follow naming conventions: modules snake_case (file/module names)
Use tracing macros (debug!, info!, warn!, etc.) for logging
Use Tokio for async runtime (net, time, io, signal) and async operations
Keep code self-documenting; add doc comments for public API when necessary
Files:
src/utils.rssrc/sender/packet_handler.rssrc/tests/sender_tests.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Expose internal fields for testing only behind #[cfg(feature = "test-internals")]
Files:
src/utils.rssrc/sender/packet_handler.rssrc/tests/sender_tests.rs
src/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Place integration, end-to-end, and protocol tests under src/tests/
Files:
src/tests/sender_tests.rs
🧬 Code graph analysis (2)
src/sender/packet_handler.rs (3)
src/sender/selection.rs (1)
select_connection_idx(47-134)src/utils.rs (1)
now_ms(15-20)src/protocol.rs (1)
get_srt_sequence_number(54-64)
src/tests/sender_tests.rs (2)
src/sender/selection.rs (2)
select_connection_idx(47-134)calculate_quality_multiplier(6-45)src/utils.rs (1)
now_ms(15-20)
🔇 Additional comments (6)
src/tests/sender_tests.rs (1)
1-346: Tests updated correctly to reflect refactored connection structure.The test file has been systematically updated to use the new nested field structure (
congestion.*,reconnection.*,rtt.*) and the updatedselect_connection_idxsignature. All test assertions are consistent with the new API surface, and the test logic remains sound.src/sender/packet_handler.rs (4)
71-90: NAK handling fallback logic is sound.The NAK attribution logic first attempts to use the sequence tracking map with timestamp expiration checks, then falls back to iterating all connections if no valid entry is found. This defensive approach ensures NAKs are attributed even when sequence tracking data is stale or missing.
101-148: Uplink packet handling is resilient.The function gracefully handles errors by logging warnings without propagating them, ensuring that a single failed packet doesn't disrupt the overall system. The logic correctly identifies the connection by
conn_idand processes the packet with appropriate event handling.
285-344: Connection forwarding logic is well-structured.The function properly handles:
- Bounds checking before accessing connections
- Informative logging for connection switches
- Error recovery by marking failed connections
- Sequence tracking with proper capacity management (FIFO eviction)
- Consistent client address updates
The implementation is defensive and maintains system invariants correctly.
75-75: No issues found. Theis_expiredmethod is correctly implemented.The
SequenceTrackingEntrystruct (src/sender/sequence.rs:9) has a properly implementedis_expiredmethod (line 15) that safely handles timestamp expiration usingsaturating_subto prevent underflow. The method signaturepub fn is_expired(&self, current_time_ms: u64) -> boolcorrectly matches the call at line 75 in packet_handler.rs.src/utils.rs (1)
11-26: Monotonic timing helper looks solidThe
LazyLock-backedSTARTUP_INSTANTpluselapsed_ms()give a clean, runtime-friendly monotonic reference without extra synchronization. Nothing to change here.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…istration phase - Introduced a new function `select_pre_registration_connection` to streamline the selection of connections during the pre-registration phase. - The function prioritizes reusing the last selected connection if valid, followed by any non-timed-out connection, improving clarity and maintainability of the code. - Updated the `handle_srt_packet` function to utilize the new selection logic, enhancing the overall packet handling process.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/connection/rtt.rs (1)
112-132: LGTM! Keepalive response handling is correct.The RTT validation (≤10 seconds), estimate update, and state management are properly implemented.
🧹 Nitpick comments (2)
src/connection/congestion.rs (1)
20-31: Consider restricting field visibility to pub(crate).All fields in
CongestionControlare currentlypub, exposing internal congestion state directly. Since the struct provides methods likehandle_nak()andperform_window_recovery()to manage this state, consider restricting field visibility topub(crate)unless external modules genuinely need direct field access. This encapsulation prevents accidental state corruption and clarifies the intended API surface.Apply this diff if direct field access is not required externally:
#[derive(Debug, Clone, Default)] pub struct CongestionControl { - pub nak_count: i32, - pub last_nak_time_ms: u64, - pub last_window_increase_ms: u64, - pub consecutive_acks_without_nak: i32, - pub fast_recovery_mode: bool, - pub fast_recovery_start_ms: u64, - pub nak_burst_count: i32, - pub nak_burst_start_time_ms: u64, + pub(crate) nak_count: i32, + pub(crate) last_nak_time_ms: u64, + pub(crate) last_window_increase_ms: u64, + pub(crate) consecutive_acks_without_nak: i32, + pub(crate) fast_recovery_mode: bool, + pub(crate) fast_recovery_start_ms: u64, + pub(crate) nak_burst_count: i32, + pub(crate) nak_burst_start_time_ms: u64, }src/connection/rtt.rs (1)
6-19: Consider restricting field visibility to pub(crate).All fields in
RttTrackerare currentlypub, exposing internal RTT state directly. Since the struct provides methods likeupdate_estimate()andhandle_keepalive_response()to manage this state, consider restricting field visibility topub(crate)unless external modules genuinely need direct field access.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/connection/congestion.rs(1 hunks)src/connection/mod.rs(1 hunks)src/connection/rtt.rs(1 hunks)src/sender/packet_handler.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: All Rust code must pass cargo fmt --all -- --check
All Rust code must pass clippy with -D warnings (no warnings allowed)
Use anyhow::Result for error propagation
Group imports as std → external → crate with module-level granularity (imports_granularity = "Module", group_imports = "StdExternalCrate")
Follow naming conventions: constants SCREAMING_SNAKE_CASE
Follow naming conventions: structs PascalCase
Follow naming conventions: functions snake_case
Follow naming conventions: modules snake_case (file/module names)
Use tracing macros (debug!, info!, warn!, etc.) for logging
Use Tokio for async runtime (net, time, io, signal) and async operations
Keep code self-documenting; add doc comments for public API when necessary
Files:
src/connection/congestion.rssrc/connection/rtt.rssrc/connection/mod.rssrc/sender/packet_handler.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Expose internal fields for testing only behind #[cfg(feature = "test-internals")]
Files:
src/connection/congestion.rssrc/connection/rtt.rssrc/connection/mod.rssrc/sender/packet_handler.rs
🧬 Code graph analysis (4)
src/connection/congestion.rs (2)
src/utils.rs (1)
now_ms(15-20)src/connection/mod.rs (4)
handle_nak(360-377)nak_burst_count(513-515)perform_window_recovery(474-477)time_since_last_nak_ms(505-507)
src/connection/rtt.rs (3)
src/protocol.rs (1)
extract_keepalive_timestamp(90-102)src/utils.rs (1)
now_ms(15-20)src/connection/mod.rs (1)
connection_established_ms(517-519)
src/connection/mod.rs (9)
src/sender/mod.rs (2)
std(92-92)None(93-93)src/connection/socket.rs (2)
bind_from_ip(7-39)resolve_remote(41-48)src/utils.rs (1)
now_ms(15-20)src/registration.rs (2)
new(53-71)default(47-49)src/connection/rtt.rs (1)
default(22-35)src/connection/bitrate.rs (1)
default(13-20)src/protocol.rs (6)
create_keepalive_packet(80-88)create_reg2_packet(73-78)get_packet_type(46-51)parse_srt_ack(120-128)parse_srt_nak(131-160)parse_srtla_ack(163-182)src/connection/congestion.rs (3)
handle_nak(47-103)perform_window_recovery(194-268)time_since_last_nak_ms(270-276)src/connection/reconnection.rs (1)
should_attempt_reconnect(17-46)
src/sender/packet_handler.rs (3)
src/sender/selection.rs (1)
select_connection_idx(47-134)src/utils.rs (1)
now_ms(15-20)src/protocol.rs (1)
get_srt_sequence_number(54-64)
🔇 Additional comments (23)
src/connection/congestion.rs (3)
47-103: LGTM! NAK handling logic is sound.The NAK burst tracking, window reduction, and fast recovery mode activation are correctly implemented with appropriate safeguards (saturating arithmetic, minimum window bounds).
105-124: LGTM! Classic ACK handling matches C implementation.The window increase logic correctly mirrors the C version with the
WINDOW_INCR - 1adjustment and proper upper bound capping.
126-192: LGTM! Enhanced ACK handling implements conservative recovery.The utilization-based throttling, consecutive ACK requirements, and rate-limited window increases provide robust congestion control. The floating-point threshold comparisons are appropriate for this use case.
src/connection/rtt.rs (2)
54-101: LGTM! RTT estimation uses appropriate asymmetric smoothing.The dual smoothing paths (smooth_rtt_ms for general trends, fast_rtt_ms for spike detection), delta tracking, and jitter calculation are well-designed. The stability check for minimum RTT updates is a good refinement.
134-147: LGTM! RTT measurement deferral logic is appropriate.The decision to defer RTT probing until after REG3 completion prevents measurement spam during the handshake phase.
src/sender/packet_handler.rs (6)
20-99: LGTM! Connection event processing is well-structured.The function correctly handles ACKs, SRTLA ACKs, and NAKs with appropriate sequence tracking and fallback logic. The NAK handling (lines 71-90) properly attempts sequence-based lookup before falling back to iterating all connections.
101-148: LGTM! Uplink packet handling is correct.The connection lookup, packet processing, and error handling are properly implemented. The atomic load with
Relaxedordering is appropriate for a configuration flag.
150-176: LGTM! Packet queue draining is correctly implemented.The
try_recvloop pattern efficiently drains all buffered packets without blocking.
178-203: LGTM! Pre-registration connection selection is well-designed.The priority-based selection (last used → any available) with clear documentation makes the logic easy to understand and maintain.
205-289: LGTM! SRT packet handling correctly branches on registration state.The pre-registration and post-registration paths are properly separated, and the toggle loading with classic mode overrides is correctly implemented.
291-350: LGTM! Connection forwarding with proper error recovery.The send error handling (lines 324-330) correctly marks the connection for recovery rather than propagating the error, allowing the system to continue with other connections. The sequence tracking cleanup (lines 332-336) prevents unbounded memory growth.
src/connection/mod.rs (12)
30-96: LGTM! Feature-gated struct definition is correctly implemented.The extensive use of
#[cfg(feature = "test-internals")]properly exposes internal fields for testing while keeping them private in production builds. This aligns with the coding guidelines for exposing test internals.
98-132: LGTM! Connection constructor is well-structured.The socket setup, state initialization, and startup grace configuration are correctly implemented. The atomic counter for connection IDs is appropriate.
134-144: LGTM! Score calculation with proper safeguards.The division-by-zero prevention and overflow protection via
saturating_addmake this robust.
191-222: LGTM! Non-blocking packet drain is correctly implemented.The
try_recvloop with error handling properly drains all available packets without blocking.
237-312: LGTM! Packet processing pipeline is well-organized.The registration state machine integration, packet type dispatch, and dual forwarding (instant + batched) are correctly implemented.
314-327: LGTM! Packet registration with circular buffer is correctly implemented.The wraparound handling and debug logging provide good visibility into buffer behavior.
329-358: LGTM! SRT ACK handling with RTT extraction is correct.The packet log search, in-flight count recalculation, and opportunistic RTT measurement are properly implemented.
360-377: LGTM! NAK handling delegates to congestion control.The packet log search and delegation pattern is clean and correct.
379-414: LGTM! SRTLA ACK handling with mode-specific delegation.The specific-sequence ACK processing correctly branches between classic and enhanced congestion control modes.
416-436: LGTM! Global SRTLA ACK window increase matches C implementation.The conditional +1 window increase for active connections is correctly implemented.
459-472: LGTM! Keepalive logic correctly tracks all sends.The use of
last_sent(instead of only tracking keepalives) prevents unnecessary keepalive spam during active transmission, as intended.
490-503: LGTM! Connection recovery reset is comprehensive.The state reset correctly prepares the connection for recovery by clearing all runtime state while preserving the socket and identity.
Probe packets were incorrectly setting waiting_for_keepalive_response = true, but probe responses never cleared this flag. This permanently blocked all future keepalive RTT measurements on connections that participated in probing. Probes use independent tracking in registration.rs, so they don't need the keepalive RTT infrastructure.
…zation Updated the SrtlaConnection initialization to set last_received to None instead of the current time, ensuring a clearer state management during connection setup.
Updated the congestion control logic to introduce progressive recovery rates based on the time since the last NAK. The recovery rates are now categorized as follows: 25% for recent NAKs (<5 seconds), 50% for 5-7 seconds, 100% for 7-10 seconds, and 200% for 10+ seconds. Additionally, added tests to validate the new recovery rates and their behavior under fast recovery mode, ensuring accurate window adjustments based on timing constraints.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/connection/congestion.rs(1 hunks)src/connection/mod.rs(1 hunks)src/tests/connection_tests.rs(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/connection/congestion.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: All Rust code must pass cargo fmt --all -- --check
All Rust code must pass clippy with -D warnings (no warnings allowed)
Use anyhow::Result for error propagation
Group imports as std → external → crate with module-level granularity (imports_granularity = "Module", group_imports = "StdExternalCrate")
Follow naming conventions: constants SCREAMING_SNAKE_CASE
Follow naming conventions: structs PascalCase
Follow naming conventions: functions snake_case
Follow naming conventions: modules snake_case (file/module names)
Use tracing macros (debug!, info!, warn!, etc.) for logging
Use Tokio for async runtime (net, time, io, signal) and async operations
Keep code self-documenting; add doc comments for public API when necessary
Files:
src/tests/connection_tests.rssrc/connection/mod.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Expose internal fields for testing only behind #[cfg(feature = "test-internals")]
Files:
src/tests/connection_tests.rssrc/connection/mod.rs
src/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Place integration, end-to-end, and protocol tests under src/tests/
Files:
src/tests/connection_tests.rs
🧠 Learnings (2)
📚 Learning: 2025-10-15T14:24:40.523Z
Learnt from: CR
Repo: irlserver/srtla_send PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T14:24:40.523Z
Learning: Applies to src/tests/**/*.rs : Place integration, end-to-end, and protocol tests under src/tests/
Applied to files:
src/tests/connection_tests.rs
📚 Learning: 2025-10-15T14:24:40.523Z
Learnt from: CR
Repo: irlserver/srtla_send PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T14:24:40.523Z
Learning: Applies to src/**/*.rs : Expose internal fields for testing only behind #[cfg(feature = "test-internals")]
Applied to files:
src/tests/connection_tests.rs
🧬 Code graph analysis (2)
src/tests/connection_tests.rs (4)
src/utils.rs (1)
now_ms(15-20)src/connection/mod.rs (2)
needs_keepalive(457-470)needs_rtt_measurement(452-455)src/test_helpers.rs (1)
create_test_connection(16-48)src/connection/congestion.rs (1)
time_since_last_nak_ms(270-276)
src/connection/mod.rs (9)
src/sender/mod.rs (2)
std(92-92)None(93-93)src/connection/socket.rs (2)
bind_from_ip(7-39)resolve_remote(41-48)src/utils.rs (1)
now_ms(15-20)src/registration.rs (2)
new(53-71)default(47-49)src/connection/rtt.rs (1)
default(22-35)src/connection/bitrate.rs (1)
default(13-20)src/protocol.rs (6)
create_keepalive_packet(80-88)create_reg2_packet(73-78)get_packet_type(46-51)parse_srt_ack(120-128)parse_srt_nak(131-160)parse_srtla_ack(163-182)src/connection/congestion.rs (3)
handle_nak(47-103)perform_window_recovery(194-268)time_since_last_nak_ms(270-276)src/connection/reconnection.rs (1)
should_attempt_reconnect(17-46)
🪛 GitHub Actions: CI
src/tests/connection_tests.rs
[error] 691-696: cargo fmt --all -- --check failed. Code formatting issues detected in tests: the diff shows an assertion formatting change. Run 'cargo fmt --all' (or commit formatted changes) to fix.
🪛 GitHub Check: Test
src/tests/connection_tests.rs
[warning] 691-691:
Diff in /home/runner/work/srtla_send/srtla_send/src/tests/connection_tests.rs
🔇 Additional comments (4)
src/tests/connection_tests.rs (2)
564-726: Excellent test coverage for progressive recovery behavior.The three new test functions comprehensively validate:
- Progressive recovery rates (25%/50%/100%/200%) based on time since last NAK
- Fast recovery mode with 2x multiplier
- Timing constraints for both normal (2000ms + 1000ms increment) and fast mode (500ms + 300ms increment)
This provides strong regression protection for the new window recovery logic.
3-5: Clean refactoring to nested API structure.The test updates consistently reflect the new modular organization:
- Congestion state now under
conn.congestion.*- RTT tracking under
conn.rtt.*- Reconnection state under
conn.reconnection.*- Timing model migrated from
last_keepalive_mstolast_sent: Option<Instant>All accesses follow the new API surface properly.
Also applies to: 73-74, 94-94, 96-96, 358-358, 362-362, 375-375, 379-380, 396-397
src/connection/mod.rs (2)
1-27: Well-structured modular refactoring.The new module organization cleanly separates concerns:
- Bitrate, congestion, RTT, reconnection state into dedicated submodules
- Proper conditional compilation for test-internals feature gating
- Clean re-exports of public types
The initialization in
connect_from_ipproperly sets up all nested structures with appropriate defaults.Also applies to: 30-96, 99-132
541-570: Previous issues successfully resolved in reconnect logic.All three concerns from past reviews have been addressed:
- Line 559:
bitrate.reset()properly clears stale measurement state- Line 549:
last_received = Nonematches initial connection and mark_for_recovery patterns- Probe RTT tracking (line 181-187) no longer blocks regular keepalive measurements
The reset sequence is now complete and consistent.
- Remove unused now_instant variable in send_keepalive function - Add instant_to_elapsed_ms utility for consistent timeout comparisons - Standardize test timing to use tokio::time::Instant - Clarify separation between Instant (relative) and now_ms (absolute) usage - Apply rustfmt formatting to test files
Summary by CodeRabbit
New Features
Refactor
Removed
Performance
Chores
Tests