feat: add extended KEEPALIVE with embedded connection info #10
Conversation
Add backwards-compatible extended KEEPALIVE packet format that embeds connection telemetry data directly in keepalive packets, eliminating the need for separate extension negotiation (EXT_HELLO/EXT_ACK) and connection info packets (CONN_INFO). Extended KEEPALIVE format (42 bytes): - Bytes 0-9: Standard KEEPALIVE (type + timestamp) - Bytes 10-11: Magic number (0xC01F) for format detection - Bytes 12-13: Version (0x0001) - Bytes 14-41: Connection info (conn_id, window, in_flight, rtt_us, nak_count, bitrate) Backwards compatibility: - Old receivers read first 10 bytes and ignore the rest - New receivers detect magic number and parse connection info - Combined magic + version check provides ~1 in 4 billion false positive rate Benefits over previous extension system: - No negotiation handshake required - Telemetry available immediately after connection - Simpler implementation (eliminates extension negotiation module) - Fewer packet types (one instead of three) - More reliable (no dependency on EXT_ACK response) BREAKING CHANGE: Senders now always send 42-byte extended keepalives instead of 10-byte standard keepalives. This is backwards compatible with existing receivers but changes the wire format.
WalkthroughAdds a backwards-compatible 42-byte extended KEEPALIVE packet for SRTLA with per-connection telemetry; documents the format and parsing rules; implements encoding/decoding and unit tests in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Conn as Connection (sender)
participant Proto as Protocol (encoder)
participant Net as Network
participant Recv as Receiver
participant Parser as Protocol (extractor)
Conn->>Proto: build ConnectionInfo (conn_id, window, in_flight, rtt_us, nak_count, bitrate)
Proto-->>Conn: 42-byte extended KEEPALIVE packet
Conn->>Net: send KEEPALIVE packet
Net->>Recv: deliver packet bytes
Recv->>Parser: call extract_keepalive_conn_info(buf)
alt magic/version valid & length == 42
Parser-->>Recv: ConnectionInfo parsed
else not extended or older sender
Parser-->>Recv: None (fall back to extract_keepalive_timestamp)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/EXTENDED_KEEPALIVE.md (1)
20-25: Add language specifiers to code blocks for linting compliance.The packet layout diagrams at lines 20, 29, and 235 lack language specifiers. Use
textorplaintextto satisfy markdown linters.-``` +```text Offset | Size | Field -------|------|------------------ 0-1 | 2 | Packet type (0x9000) 2-9 | 8 | Timestamp (u64 ms, big-endian)Apply the same fix to the other two occurrences (lines 29 and 235). </blockquote></details> <details> <summary>src/protocol.rs (1)</summary><blockquote> `317-401`: **Test suite provides good coverage.** The tests verify the key scenarios: roundtrip encoding/decoding, backwards compatibility with standard keepalives, and rejection of packets with wrong magic or version. Consider adding a test case with negative `window` and `in_flight` values to verify signed integer serialization roundtrips correctly: ```rust #[test] fn test_extended_keepalive_negative_values() { let info = ConnectionInfo { conn_id: 1, window: -5000, in_flight: -10, rtt_us: 50_000, nak_count: 0, bitrate_bytes_per_sec: 500_000, }; let pkt = create_keepalive_packet_ext(info); let extracted = extract_keepalive_conn_info(&pkt).unwrap(); assert_eq!(extracted, info); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/EXTENDED_KEEPALIVE.md(1 hunks)src/connection/mod.rs(1 hunks)src/protocol.rs(5 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/mod.rssrc/protocol.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Expose internal fields for testing only behind #[cfg(feature = "test-internals")]
Files:
src/connection/mod.rssrc/protocol.rs
🧬 Code graph analysis (1)
src/connection/mod.rs (1)
src/protocol.rs (1)
create_keepalive_packet_ext(124-143)
🪛 markdownlint-cli2 (0.18.1)
docs/EXTENDED_KEEPALIVE.md
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
235-235: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: Test (Windows)
- GitHub Check: Test
🔇 Additional comments (6)
src/connection/mod.rs (1)
160-169: Extended keepalive construction looks correct.The
ConnectionInfopopulation correctly maps connection state to the protocol fields. The RTT conversion (ms × 1000 → μs) and bitrate conversion (bps ÷ 8 → bytes/sec) are accurate.Minor observation:
conn_idis truncated fromu64tou32on line 162. SinceNEXT_CONN_IDstarts at 1 and increments atomically, this is safe in practice (you'd need 4+ billion connections to wrap). If this sender is intended for very long-running or high-churn deployments, consider either:
- Changing
SrtlaConnection::conn_idtou32, or- Documenting the truncation behavior
Otherwise, this is fine as-is.
docs/EXTENDED_KEEPALIVE.md (1)
1-271: Documentation is comprehensive and accurate.The packet format, field definitions, and compatibility behavior are well-documented and align with the implementation. The security considerations section appropriately notes the lack of authentication and recommends rate-limiting.
src/protocol.rs (4)
45-48: Constants are well-defined and follow naming conventions.The magic number
0xc01f("C01F" mnemonic for "Connection Info") is a reasonable choice. Version0x0001allows for future extensibility as documented.
96-105:ConnectionInfostruct is well-designed.The struct is appropriately sized (28 bytes), derives
Copyfor efficient passing, and usesEq(valid since all fields are integral). The public fields align with the documented packet format.
124-143: Packet creation is correct and well-documented.The function correctly serializes all fields in big-endian format at the documented offsets. The use of
copy_from_slicewithto_be_bytes()is idiomatic and efficient.
167-205: Extraction function has robust validation.The validation sequence (length → type → magic → version) correctly prevents out-of-bounds access and gracefully handles standard keepalives or corrupted packets. The parsing logic mirrors the creation function exactly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
receiver (1)
1-1: External symlink-style target likely to be fragile (../srtla).Line 1 makes
receiverlook like a Git symlink pointing outside this repo (to../srtla). Anyone cloning this repo standalone will end up with a broken link unless they also have a siblingsrtladirectory arranged exactly the same way. This can easily break builds, tooling, or editor navigation that expectreceiverto be self-contained.Consider one of:
- If this should link to code inside this repo, adjust the target to a path within the repo.
- If this is meant to pull in another project, prefer a Git submodule/subtree or documented dependency instead of a relative symlink escaping the repo root.
Can you confirm that this external path is intentional and works in your intended deployment/CI environments?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
receiver(1 hunks)src/connection/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/connection/mod.rs
⏰ 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). (2)
- GitHub Check: Test (Windows)
- GitHub Check: Test
Add backwards-compatible extended KEEPALIVE packet format that embeds connection telemetry data directly in keepalive packets, eliminating the need for separate extension negotiation (EXT_HELLO/EXT_ACK) and connection info packets (CONN_INFO).
Extended KEEPALIVE format (42 bytes):
Backwards compatibility:
Benefits over previous extension system:
BREAKING CHANGE: Senders now always send 42-byte extended keepalives instead of 10-byte standard keepalives. This is backwards compatible with existing receivers but changes the wire format.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.