Skip to content

Add SRTLA extension support for connection info and capabilities negotiation#9

Closed
datagutt wants to merge 6 commits into
mainfrom
feat/protocol-extensions
Closed

Add SRTLA extension support for connection info and capabilities negotiation#9
datagutt wants to merge 6 commits into
mainfrom
feat/protocol-extensions

Conversation

@datagutt

@datagutt datagutt commented Dec 3, 2025

Copy link
Copy Markdown
Member
  • Introduced extensions module and updated lib.rs and main.rs to include it.
  • Enhanced SrtlaConnection struct to track negotiated extension capabilities.
  • Implemented methods for sending connection info telemetry and extension HELLO packets.
  • Updated housekeeping logic to periodically send connection info if extensions are negotiated.
  • Adjusted tests to validate changes in packet parsing and handling.

Summary by CodeRabbit

  • New Features

    • Added SRTLA protocol extensions with automatic capability negotiation and per-connection telemetry; telemetry sent immediately on availability and periodically (~5s).
  • Documentation

    • New extensions docs covering negotiation, telemetry formats, compatibility, security, debugging, and extension workflow.
  • Tests

    • Unit tests for extension messages, capability checks, and connection-info round-trip parsing.

✏️ Tip: You can customize this high-level summary in your review settings.

… capabilities negotiation

- Introduced `extensions` module and updated `lib.rs` and `main.rs` to include it.
- Enhanced `SrtlaConnection` struct to track negotiated extension capabilities.
- Implemented methods for sending connection info telemetry and extension HELLO packets.
- Updated housekeeping logic to periodically send connection info if extensions are negotiated.
- Adjusted tests to validate changes in packet parsing and handling.
@datagutt datagutt changed the title feat(connection): add SRTLA extension support for connection info and capabilities negotiation Add SRTLA extension support for connection info and capabilities negotiation Dec 3, 2025
@coderabbitai

coderabbitai Bot commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Adds an IRLSERVER SRTLA extensions subsystem: a new extensions module with HELLO/ACK negotiation and CONN_INFO telemetry packet formats, connection-state fields and methods for negotiation/telemetry, housekeeping logic to periodically emit telemetry, documentation, and unit tests.

Changes

Cohort / File(s) Summary
Documentation & Feature Description
README.md, docs/EXTENSIONS.md
New EXTENSIONS.md and README entry describing non-standard IRLSERVER SRTLA extensions, HELLO/ACK negotiation, CONN_INFO telemetry packet format, capability semantics, implementation notes, debugging, security, and compatibility guidance.
Extension Module
src/extensions.rs
New module defining constants, packet layouts, creator/parsers, ExtensionCapabilities and ConnectionInfoData structs, capability helpers, validators, and unit tests for HELLO/ACK and CONN_INFO round-trips and validations.
Public Exports / Module Tree
src/lib.rs, src/main.rs
Exposes new extensions module (pub mod extensions) and declares mod extensions in main for compilation.
Connection State & Negotiation
src/connection/mod.rs
Added fields last_conn_info_sent, extensions_negotiated, receiver_capabilities; initialized them; added send_extension_hello, send_connection_info, and has_extension; send HELLO after registration and handle SRTLA_EXT_ACK to set negotiation state.
Telemetry Scheduling
src/sender/housekeeping.rs
Added CONN_INFO_INTERVAL_SECS = 5 and logic in housekeeping to periodically send CONN_INFO when the extension is negotiated and the connection is connected; logs on failures.
Test Helpers & Tests
src/test_helpers.rs, src/tests/protocol_tests.rs
Test helpers updated to initialize new fields; protocol test minor assertion refactor; new unit tests added in src/extensions.rs.

Sequence Diagram(s)

sequenceDiagram
    participant Conn as SrtlaConnection
    participant Peer as Remote Peer
    participant HK as Housekeeping

    Conn->>Peer: Reg3 (registration)
    Peer-->>Conn: Reg3 ACK
    Note over Conn: on Reg3 success -> send_extension_hello()
    Conn->>Peer: EXT_HELLO (0x9FF0) version=0x0001 caps=CAP_CONN_INFO
    Peer-->>Conn: EXT_ACK (0x9FF1) version, capabilities
    Note over Conn: set extensions_negotiated=true, store receiver_capabilities

    loop every 5s
        HK->>Conn: trigger housekeeping
        alt extensions_negotiated && connected && interval elapsed
            Conn->>Peer: CONN_INFO (0x9F00) conn_id, window, in_flight, rtt, naks, bitrate
            Peer-->>Conn: (optional) process telemetry / ignore if unsupported
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Focus review on src/extensions.rs (packet layout, endianness, lengths, parsing edge cases).
  • Verify integration in src/connection/mod.rs (when HELLO is sent, proper error handling, ACK parsing).
  • Check housekeeping timing/gating in src/sender/housekeeping.rs.
  • Review unit tests for coverage and correctness.

Possibly related PRs

  • Test suite #1 — Related changes to test-internals visibility patterns and test helper adjustments that overlap with this PR's visibility/init updates.
  • Refactoring, enhanced logging and mimalloc #7 — Overlaps in connection-state additions and telemetry/housekeeping behavior; likely intersects with send/telemetry changes.

Poem

🐰 I stitched a HELLO with a tiny cap,
ACK bounced back on a neat little map,
Every five heartbeats I whisper conn-info,
Packets hop by with a soft, steady tempo,
If you don't understand, I'll tiptoe and let go. 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: adding SRTLA extension support for connection info and capabilities negotiation, which is demonstrated across the new extensions module, connection enhancements, and housekeeping updates.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/protocol-extensions

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
src/tests/protocol_tests.rs (1)

213-217: Test change is fine; could be simplified

The explicit empty_slice: &[u32] = &[] is correct and keeps the type unambiguous. If you prefer slightly simpler tests, you could also write:

assert!(parsed_empty.is_empty());

Functionally equivalent, so this is purely stylistic.

docs/EXTENSIONS.md (1)

19-43: Add languages to fenced code blocks to satisfy markdownlint

The extension docs are clear and align with the implementation, but markdownlint is flagging unlabeled fenced blocks (e.g., handshake diagram, packet layouts, metric examples, log snippets).

Consider adding explicit languages, for example:

  • Handshake and metric examples:
-```
+```text
 Sender                          Receiver
 ...
  • Rust snippets:
-```rust
+```rust
 // existing Rust examples
  • Log output snippets:
-```
+```text
 DEBUG srtla_send: Connection 1: Sent extension HELLO ...

This should clear the MD040 warnings without changing content.

Also applies to: 48-56, 103-114, 133-138, 305-309

src/connection/mod.rs (3)

22-28: Extension capability fields and wiring look correct; consider reset on reconnect as an optional safeguard

  • Importing SRTLA_EXT_ACK and SRTLA_EXT_CAP_CONN_INFO and adding extensions_negotiated/receiver_capabilities to SrtlaConnection is consistent with the documented negotiation model.
  • Visibility is correctly guarded with #[cfg(feature = "test-internals")], matching the project’s testing guidelines.
  • connect_from_ip initializes both fields to a known baseline (false / 0), which is good.

One optional improvement: neither reconnect() nor mark_for_recovery() currently reset extensions_negotiated/receiver_capabilities. In practice the receiver’s capabilities are unlikely to change, so this is safe, but if you want the negotiation state to strictly reflect the current socket’s handshake you could clear it on reconnect:

@@ pub async fn reconnect(&mut self) -> Result<()> {
-        self.connected = false;
-        self.last_received = None;
+        self.connected = false;
+        self.last_received = None;
@@
-        self.bitrate.reset();
+        self.bitrate.reset();
+
+        // Clear extension negotiation state; a fresh REG3/EXT_HELLO/EXT_ACK cycle will re‑establish it
+        self.extensions_negotiated = false;
+        self.receiver_capabilities = 0;

Not required for correctness (extensions gracefully degrade if the receiver stops responding), but it makes the state machine more explicit.

Also applies to: 31-106, 120-143


186-247: New extension methods are coherent; watch for telemetry field ranges

The three new methods are conceptually solid:

  • send_connection_info builds a 0x9F00 CONN_INFO packet from existing connection state and logs a detailed debug line.
  • send_extension_hello announces SRTLA_EXT_CAP_CONN_INFO as the currently supported capability.
  • has_extension correctly checks that negotiation completed and the requested capability bit is set.

Two minor points to keep in mind:

  1. Casting to u32 in telemetry:
(self.congestion.nak_count as u32, self.bitrate.current_bitrate_bps as u32)

If nak_count can ever become negative or current_bitrate_bps can exceed u32::MAX, these casts will truncate. That’s unlikely in practice and only affects monitoring, but if you want to be defensive you could clamp:

let nak_count = self.congestion.nak_count.max(0) as u32;
let bitrate = self.bitrate.current_bitrate_bps.min(u32::MAX as f64) as u32;
  1. Shared use of last_sent:

You already use last_sent consistently across send_data_with_tracking, send_keepalive, send_srtla_packet, and these extension methods, which keeps idle/keepalive logic coherent. Once you introduce a dedicated telemetry timestamp (see housekeeping comment), this method is the right place to update that as well.

Overall, the extension API shape is good and matches the docs.


334-345: Extension HELLO on REG3 and EXT_ACK handling integrate cleanly

  • Sending send_extension_hello() on RegistrationEvent::Reg3 is the right trigger point: it ensures registration succeeded before starting extension negotiation.
  • Errors from send_extension_hello() are logged at debug! and don’t affect the core connection logic, so extensions degrade gracefully if something goes wrong.
  • Handling SRTLA_EXT_ACK in process_packet_internal via parse_extension_packet and updating extensions_negotiated/receiver_capabilities with an info! log gives clear visibility into negotiated capabilities.

One small enhancement you might consider later is logging or tracing when parse_extension_packet(data) returns None for an EXT_ACK packet (e.g., bad version/length), to ease debugging misbehaving receivers, but that’s not required for correctness.

The negotiation flow as wired here is sound.

Also applies to: 384-394

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 212b2b8 and cc981cb.

📒 Files selected for processing (9)
  • README.md (1 hunks)
  • docs/EXTENSIONS.md (1 hunks)
  • src/connection/mod.rs (6 hunks)
  • src/extensions.rs (1 hunks)
  • src/lib.rs (1 hunks)
  • src/main.rs (1 hunks)
  • src/sender/housekeeping.rs (2 hunks)
  • src/test_helpers.rs (2 hunks)
  • src/tests/protocol_tests.rs (1 hunks)
🧰 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/lib.rs
  • src/main.rs
  • src/tests/protocol_tests.rs
  • src/sender/housekeeping.rs
  • src/connection/mod.rs
  • src/test_helpers.rs
  • src/extensions.rs
src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Expose internal fields for testing only behind #[cfg(feature = "test-internals")]

Files:

  • src/lib.rs
  • src/main.rs
  • src/tests/protocol_tests.rs
  • src/sender/housekeeping.rs
  • src/connection/mod.rs
  • src/test_helpers.rs
  • src/extensions.rs
src/tests/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Place integration, end-to-end, and protocol tests under src/tests/

Files:

  • src/tests/protocol_tests.rs
🧠 Learnings (3)
📚 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 : Group imports as std → external → crate with module-level granularity (imports_granularity = "Module", group_imports = "StdExternalCrate")

Applied to files:

  • src/lib.rs
  • src/main.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/tests/**/*.rs : Place integration, end-to-end, and protocol tests under src/tests/

Applied to files:

  • src/tests/protocol_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/protocol_tests.rs
  • src/test_helpers.rs
🧬 Code graph analysis (1)
src/extensions.rs (2)
src/protocol.rs (1)
  • get_packet_type (46-51)
src/connection/mod.rs (1)
  • has_extension (245-247)
🪛 markdownlint-cli2 (0.18.1)
docs/EXTENSIONS.md

19-19: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


48-48: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


103-103: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


133-133: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


305-305: 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). (3)
  • GitHub Check: Test (macOS)
  • GitHub Check: Test
  • GitHub Check: Test (Windows)
🔇 Additional comments (12)
README.md (1)

30-30: README wiring for protocol extensions looks good

The new bullet cleanly surfaces extensions and correctly links to docs/EXTENSIONS.md. No issues from a tooling or UX standpoint.

src/lib.rs (1)

14-14: Publicly exposing extensions module is appropriate

Re‑exporting extensions at the crate root aligns with the documented use srtla_send::extensions::... examples and keeps the API surface coherent.

src/main.rs (1)

10-12: Binary module wiring for extensions is consistent

Adding mod extensions; matches the existing module layout (connection, protocol, etc.) and keeps the binary in sync with the library layout.

src/test_helpers.rs (1)

48-50: Test helpers correctly initialize new extension fields

Initializing extensions_negotiated to false and receiver_capabilities to 0 in both helpers keeps test connections in a clean pre‑negotiation state and in sync with the struct definition.

Also applies to: 87-89

src/extensions.rs (8)

1-22: LGTM! Clear documentation of the extension protocol.

The module documentation clearly explains the negotiation flow and range allocation strategy, making it easy for maintainers to understand the protocol design.


23-23: LGTM! Import follows guidelines.

The import correctly uses crate-level granularity as per the coding guidelines.


29-51: LGTM! Constants follow naming conventions.

The constants are well-documented with clear packet format descriptions. The #[allow(dead_code)] on SRTLA_EXT_ACK_LEN is acceptable as it maintains API completeness for future use.


57-62: LGTM! Capability flag design is extensible.

The bitmask approach with commented examples for future extensions demonstrates good forward-thinking design.


68-84: LGTM! Connection info packet format is well-documented.

The detailed packet format documentation makes it clear how the 32-byte structure is organized.


90-108: LGTM! Data structures follow Rust conventions.

The derive traits are appropriate for each struct, with Copy correctly limited to the smaller ExtensionCapabilities struct.


114-179: LGTM! Extension negotiation functions are well-implemented.

The functions correctly handle packet creation, parsing, and validation. The use of Option for fallible parsing is idiomatic Rust. The doc comment example for create_extension_hello is particularly helpful.


279-349: LGTM! Comprehensive test coverage.

The tests cover creation, parsing, validation, and round-trip scenarios for all packet types. The range validation test (lines 343-348) is particularly valuable for maintaining protocol compliance.

Note: If the conn_id parameter type is changed to u32 as suggested in the previous comment, the test at line 320 will continue to work correctly since the literal 42 fits in u32.

Comment thread src/extensions.rs
Comment thread src/sender/housekeeping.rs
datagutt and others added 3 commits December 3, 2025 16:23
…rval

CONN_INFO telemetry was incorrectly gated by last_sent which updates on
every packet send, preventing periodic telemetry from firing at the
intended 5-second interval. Added dedicated last_conn_info_sent field to
track CONN_INFO sends separately, ensuring telemetry fires reliably.
…_INFO packets

- Added version validation policy to ensure compatibility and prevent protocol confusion.
- Updated EXT_HELLO to log errors and ignore packets with mismatched versions.
- Updated CONN_INFO to log warnings and ignore packets with mismatched versions.
- Renamed bitrate field to clarify it represents bytes per second.
- Adjusted related parsing and packet creation functions to reflect the new naming convention.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
docs/EXTENSIONS.md (4)

19-43: Add language specifier to fenced code block for accessibility and tooling.

Static analysis indicates this code block lacks a language specifier. For ASCII diagrams and protocol flows, use text or plaintext.

-```
+```text
 Sender                          Receiver

48-55: Add language specifiers to table-format code blocks.

These code blocks representing packet format tables should have a language specifier (e.g., text) for consistency and linting compliance.

-```
+```text
 Offset | Size | Field
-```
+```text
 Offset | Size | Field                      | Type

Also applies to: 103-114


133-138: Add language specifier for example metrics block.

-```
+```text
 Connection 1: window=25000, in_flight=3, rtt=120ms, naks=5,  bitrate=2.5MB/s

335-339: Add language specifier for log output example.

-```
+```text
 DEBUG srtla_send: Connection 1: Sent extension HELLO (capabilities=0x00000001)
src/connection/mod.rs (1)

211-212: Minor: Reuse single timestamp for consistency.

Two separate Instant::now() calls create slightly different timestamps. Use a single timestamp for both fields.

     self.socket.send(&pkt).await?;
-    self.last_sent = Some(Instant::now());
-    self.last_conn_info_sent = Some(Instant::now());
+    let now = Instant::now();
+    self.last_sent = Some(now);
+    self.last_conn_info_sent = Some(now);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cc981cb and 3499dd3.

📒 Files selected for processing (5)
  • docs/EXTENSIONS.md (1 hunks)
  • src/connection/mod.rs (7 hunks)
  • src/extensions.rs (1 hunks)
  • src/sender/housekeeping.rs (2 hunks)
  • src/test_helpers.rs (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/test_helpers.rs
🧰 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/sender/housekeeping.rs
  • src/connection/mod.rs
  • src/extensions.rs
src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Expose internal fields for testing only behind #[cfg(feature = "test-internals")]

Files:

  • src/sender/housekeeping.rs
  • src/connection/mod.rs
  • src/extensions.rs
🧬 Code graph analysis (2)
src/connection/mod.rs (1)
src/extensions.rs (4)
  • create_connection_info_packet (198-233)
  • create_extension_hello (129-145)
  • has_extension (177-179)
  • parse_extension_packet (155-172)
src/extensions.rs (2)
src/protocol.rs (1)
  • get_packet_type (46-51)
src/connection/mod.rs (1)
  • has_extension (251-253)
🪛 markdownlint-cli2 (0.18.1)
docs/EXTENSIONS.md

19-19: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


48-48: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


103-103: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


133-133: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


335-335: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (16)
src/sender/housekeeping.rs (2)

16-21: LGTM! Well-documented constant with appropriate interval.

The 5-second interval is reasonable for telemetry, and the comment clearly explains the purpose and compatibility notes regarding standard receivers.


101-115: Past review concern addressed; telemetry timing is now correct.

The dedicated last_conn_info_sent timestamp (seeded after REG3 in src/connection/mod.rs line 342) ensures CONN_INFO packets are sent independently of keepalives and data sends, matching the documented "~5 seconds after connection" behavior.

src/connection/mod.rs (5)

80-83: LGTM! Proper feature-gated field addition.

The last_conn_info_sent field follows the established pattern for test-internals visibility gating and is correctly initialized to None in the constructor.


101-109: Extension state fields properly initialized.

The extension tracking fields follow established patterns and are correctly initialized to sensible defaults (no extensions negotiated, zero capabilities).

Also applies to: 146-147


228-248: LGTM! Clean extension HELLO implementation.

The method correctly advertises supported capabilities and follows the established send pattern with proper timestamp tracking.


250-253: LGTM! Correct capability check.

Properly validates both that negotiation completed and that the specific capability is present in the receiver's response.


339-345: LGTM! Proper extension negotiation initiation after registration.

Seeding last_conn_info_sent ensures the documented "~5 seconds after connection" timing for first telemetry packet, and debug-level logging for HELLO failures is appropriate since extensions are optional.

src/extensions.rs (9)

1-24: Well-documented module header with clear scope.

The module documentation clearly states these are irlserver-specific extensions and not standard SRTLA, which is important for maintainability and user understanding.


39-84: Constants correctly defined and documented.

Packet types are within the documented 0x9F00-0x9FFF range, and the packet format documentation in comments matches EXTENSIONS.md.


90-108: Data structures correctly model protocol packets.

Fields match the documented packet formats, and #[allow(dead_code)] is appropriate since ConnectionInfoData is primarily for receiver-side parsing.


129-145: LGTM! Clean packet creation with proper encoding.

Big-endian byte ordering matches protocol specification, and the doc example provides clear usage guidance.


155-172: LGTM! Safe parsing with proper validation.

Length and packet type validation prevents malformed input, and returning Option allows callers to handle failures gracefully.


198-233: Past review concern addressed; implementation correct.

The conn_id parameter is now u32 as per protocol specification, and all fields are correctly encoded in big-endian format matching the documented packet layout.


239-266: LGTM! Robust parsing implementation.

Proper validation and field extraction with correct byte offsets matching the packet format specification.


271-273: LGTM! Correct type identification.

Strict length and type checks provide reliable packet identification for the current protocol version.


279-349: Good test coverage for extension functionality.

Tests cover creation, parsing, roundtrip verification, and range validation. The test suite provides solid validation of the core extension protocol implementation.

Comment thread docs/EXTENSIONS.md Outdated
Comment thread src/connection/mod.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/connection/mod.rs (1)

80-109: Reset extension negotiation state on recovery/reconnect to avoid stale capabilities.

extensions_negotiated and receiver_capabilities are initialized in connect_from_ip (Lines 124-148) but never reset in mark_for_recovery (Lines 610-625) or reconnect (Lines 665-693). After a disconnect/reconnect cycle, the connection may:

  • Keep extensions_negotiated == true and non‑zero receiver_capabilities from a previous session.
  • Immediately treat extensions as negotiated for a new session even if the new receiver never sends EXT_ACK (or has different capabilities).

That conflicts with the documented behavior that extensions are only enabled when the receiver actually ACKs and should be disabled when no EXT_ACK is received.

Consider explicitly clearing extension state (and optionally the telemetry timer) when the connection is torn down or re‑established:

@@ pub fn mark_for_recovery(&mut self) {
-        self.reconnection.startup_grace_deadline_ms = 0;
+        self.reconnection.startup_grace_deadline_ms = 0;
+
+        // Reset IRLSERVER extension state; a future registration must renegotiate.
+        self.extensions_negotiated = false;
+        self.receiver_capabilities = 0;
+        self.last_conn_info_sent = None;
@@ pub async fn reconnect(&mut self) -> Result<()> {
-        self.bitrate.reset();
+        self.bitrate.reset();
@@
-        self.reconnection.reset_startup_grace();
+        self.reconnection.reset_startup_grace();
+
+        // Reset IRLSERVER extension state for the new registration cycle.
+        self.extensions_negotiated = false;
+        self.receiver_capabilities = 0;
+        self.last_conn_info_sent = None;

This ensures each registration/extension negotiation starts from a clean slate.

Also applies to: 138-148, 610-625, 665-693

🧹 Nitpick comments (2)
docs/EXTENSIONS.md (1)

19-43: Add explicit languages to fenced code blocks to satisfy markdownlint (MD040).

Static analysis indicates several fenced blocks without a language (handshake diagram, packet-format tables, example metrics, and log output). To fix MD040 and improve rendering, consider labeling them as plain text:

-```
+```text
 Sender                          Receiver
 ...
-```
+```

-```
+```text
 Offset | Size | Field
 ...
-```
+```

-```
+```text
 Connection 1: window=25000, in_flight=3, rtt=120ms, naks=5,  bitrate=2.5MB/s
 ...
-```
+```

-```
+```text
 DEBUG srtla_send: Connection 1: Sent extension HELLO (capabilities=0x00000001)
 ...
-```
+```

Apply similarly to each non‑Rust/non‑bash fenced block.

Also applies to: 48-56, 103-114, 133-138, 353-357

src/connection/mod.rs (1)

250-253: Optional: reuse shared has_extension helper to avoid duplicated bitmask logic.

SrtlaConnection::has_extension re‑implements the same bitmask check as crate::extensions::has_extension. To keep capability checks consistent and centralized, you could delegate:

-    pub fn has_extension(&self, capability_flag: u32) -> bool {
-        self.extensions_negotiated && (self.receiver_capabilities & capability_flag) != 0
-    }
+    pub fn has_extension(&self, capability_flag: u32) -> bool {
+        self.extensions_negotiated
+            && crate::extensions::has_extension(self.receiver_capabilities, capability_flag)
+    }

Purely a readability/DRY improvement; behavior is unchanged.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3499dd3 and 3deef54.

📒 Files selected for processing (2)
  • docs/EXTENSIONS.md (1 hunks)
  • src/connection/mod.rs (7 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.rs
src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Expose internal fields for testing only behind #[cfg(feature = "test-internals")]

Files:

  • src/connection/mod.rs
🧬 Code graph analysis (1)
src/connection/mod.rs (1)
src/extensions.rs (4)
  • create_connection_info_packet (198-233)
  • create_extension_hello (129-145)
  • has_extension (177-179)
  • parse_extension_packet (155-172)
🪛 GitHub Check: Test
src/connection/mod.rs

[warning] 392-392:
Diff in /home/runner/work/srtla_send/srtla_send/src/connection/mod.rs


[warning] 392-392:
Diff in /home/runner/work/srtla_send/srtla_send/src/connection/mod.rs

🪛 markdownlint-cli2 (0.18.1)
docs/EXTENSIONS.md

19-19: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


48-48: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


103-103: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


133-133: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


353-353: 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). (1)
  • GitHub Check: Test (Windows)
🔇 Additional comments (2)
src/connection/mod.rs (2)

191-226: Connection info telemetry implementation looks correct and consistent with docs.

send_connection_info builds the 0x9F00 packet via create_connection_info_packet, sends it on the UDP socket, updates last_sent and last_conn_info_sent, and logs a concise summary. Field ordering and units (RTT in μs, bitrate in bytes/sec) match the documented CONN_INFO format, and using a helper in crate::extensions keeps the wire layout centralized.


228-248: Extension HELLO send and EXT_ACK handling (incl. version validation) look solid.

  • After Reg3, seeding last_conn_info_sent and invoking send_extension_hello once per successful registration aligns with the documented handshake.
  • send_extension_hello correctly advertises SRTLA_EXT_CAP_CONN_INFO and logs the advertised capability bitmask.
  • The SRTLA_EXT_ACK branch now parses the extension packet, validates caps.version against SRTLA_EXT_VERSION, logs a warning and ignores mismatched versions, and only then marks extensions_negotiated and stores receiver_capabilities, with an info log on success.

This matches the documented version‑validation policy and keeps negotiation state updates guarded by a valid ACK.

Also applies to: 340-345, 392-409

@datagutt
datagutt marked this pull request as draft December 3, 2025 19:12
@datagutt datagutt closed this Dec 6, 2025
@datagutt
datagutt deleted the feat/protocol-extensions branch January 26, 2026 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant