Conversation
- 1-byte-header fragmentation past the 64KB SCTP cap; empty-message and clean-EOF handling - is_relayed() via selected candidate-pair stats for the direct/relayed flag - IdPk.dtls_fingerprint + rendezvous webrtc SDP/IceCandidate proto fields - fix pc leaks: Weak capture breaks the state-handler Arc self-cycle; close pc on new() error paths Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Cargo.toml: record why webrtc is pinned to 0.13 — >=0.14 pulls sdp 0.10 / webrtc-util 0.12 using usize::is_multiple_of (needs rustc >=1.87), while rustdesk CI builds with Rust 1.75 (sciter i128 ABI pin) - module-level upgrade checklist in src/webrtc.rs listing the version-coupled webrtc-rs internals this transport relies on (SCTP write backpressure, 64KB message cap, detach() semantics, handler-capture leak cycle, Disconnected transience, stats-based is_relayed), all verified against webrtc 0.13 / webrtc-data 0.11 / webrtc-sctp 0.12 - send_bytes: document the bounded-backpressure mechanism (128 KiB PendingQueue semaphore + cwnd/rwnd cap) and that it is NOT cancel-safe Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds WebRTC signaling fields, ICE candidate exchange, DTLS fingerprint access, connection-state handling, explicit cleanup, relay detection, and bounded fragmented messaging. It also pins the WebRTC dependency, exposes the maximum frame length, and adds log throttling with size-based log rotation. ChangesWebRTC transport integration
Log throttling utility
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WebRTCStream
participant PeerConnection
participant ICECandidateChannel
participant RendezvousMessage
WebRTCStream->>PeerConnection: create offer or answer
PeerConnection-->>WebRTCStream: return SDP
PeerConnection-->>ICECandidateChannel: emit local ICE candidate
ICECandidateChannel->>RendezvousMessage: serialize IceCandidate
RendezvousMessage->>WebRTCStream: deliver remote candidate
WebRTCStream->>PeerConnection: add remote ICE candidate
PeerConnection-->>WebRTCStream: publish connection state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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.
🧹 Nitpick comments (2)
src/webrtc.rs (2)
579-586: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding the ICE gathering wait in
get_local_endpoint.
gathering_complete_promise()followed byrecv().awaithas no timeout. If a configured STUN or TURN server never answers and gathering does not finish, this public method blocks its caller indefinitely. The trickle variant avoids the wait, butget_local_endpointremains the one-shot entry point.
ms_timeoutis already passed intonew()and stored assend_timeout. Reusing it, or accepting an explicit bound here, would make the one-shot path self-limiting instead of relying on each caller to wrap the call. The newtest_webrtc_loopback_gathered_endpointstest wraps the flow in a 40-second timeout, which shows the wait is currently unbounded by the transport itself.♻️ Proposed bound on the gathering wait
pub async fn get_local_endpoint(&self) -> ResultType<String> { // Preserve the original one-shot endpoint contract: callers that only exchange this SDP // do not have a separate path for `take_local_ice_rx`, so their endpoint must contain the // gathered host/srflx/relay candidates. let mut gather_complete = self.pc.gathering_complete_promise().await; - let _gathering_channel_closed = gather_complete.recv().await; + // Bound the wait: an unreachable STUN/TURN server must not block the caller forever. + // On timeout fall through and return whatever candidates were gathered so far. + if self.send_timeout > 0 { + let _ = timeout( + Duration::from_millis(self.send_timeout), + gather_complete.recv(), + ) + .await; + } else { + let _gathering_channel_closed = gather_complete.recv().await; + } self.get_local_endpoint_trickle().await }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/webrtc.rs` around lines 579 - 586, Bound the ICE gathering wait in get_local_endpoint by applying the existing send_timeout (or another explicit timeout) to the gathering_complete_promise receive operation. Preserve the one-shot behavior of returning get_local_endpoint_trickle after gathering completes, while ensuring the method returns an error instead of blocking indefinitely when gathering exceeds the bound.
1191-1218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose both streams in
connect_loopbackfailure paths, and note the shared global ICE config.Two points on this helper.
First, every
unwrap()here abandons an openRTCPeerConnectionwhen it fails. A failure inanswererconstruction or inwait_connectedleaves the earlier peer connection in the globalSESSIONSmap for the rest of the test binary. That turns one failed test into a resource leak that can affect later tests in the same process.Second,
test_webrtc_ice_urlmutates the process-wideice-serversoption throughconfig::Config::set_option. Cargo runs these tests as threads in one process, so a loopback test can callget_ice_servers()while that option holdsstun://example.com,turn://example.com. Host-candidate connectivity still succeeds andis_relayed()still returnsSome(false), so the assertions hold. Gathering does get slower against the unreachable servers, which erodes the 40-second budget. Consider pinningice-serversto an empty value at the start of each loopback test, or serializing the config-mutating test behind a shared mutex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/webrtc.rs` around lines 1191 - 1218, Update connect_loopback to ensure both WebRTCStream instances are explicitly closed on every failure path, including errors during answerer creation, ICE setup, and either wait_connected call, instead of allowing unwrap failures to abandon peer connections. Also isolate the process-wide ice-servers configuration used by test_webrtc_ice_url from loopback tests by pinning it to an empty value at each loopback test start or serializing access with a shared mutex.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/webrtc.rs`:
- Around line 579-586: Bound the ICE gathering wait in get_local_endpoint by
applying the existing send_timeout (or another explicit timeout) to the
gathering_complete_promise receive operation. Preserve the one-shot behavior of
returning get_local_endpoint_trickle after gathering completes, while ensuring
the method returns an error instead of blocking indefinitely when gathering
exceeds the bound.
- Around line 1191-1218: Update connect_loopback to ensure both WebRTCStream
instances are explicitly closed on every failure path, including errors during
answerer creation, ICE setup, and either wait_connected call, instead of
allowing unwrap failures to abandon peer connections. Also isolate the
process-wide ice-servers configuration used by test_webrtc_ice_url from loopback
tests by pinning it to an empty value at each loopback test start or serializing
access with a shared mutex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae82a5aa-c3d7-427a-9411-a9d7e50d695f
📒 Files selected for processing (6)
Cargo.tomlprotos/message.protoprotos/rendezvous.protosrc/bytes_codec.rssrc/stream.rssrc/webrtc.rs
There was a problem hiding this comment.
Pull request overview
This PR significantly expands the WebRTC transport implementation, adding trickle ICE candidate streaming, DTLS fingerprint support for identity binding, explicit connection lifecycle management, and more robust send/receive behavior (including large/fragmented and empty messages).
Changes:
- Implement trickle ICE candidate production/consumption and add relay (TURN) detection via stats.
- Add DTLS fingerprint plumbing and surface it through
Streamfor secure handshake binding. - Rework WebRTC data-channel I/O with fragmentation/reassembly, send backpressure gating, timeouts, and explicit
close()cleanup.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/webrtc.rs | Major WebRTC transport refactor: trickle ICE, relay detection, DTLS fingerprint accessors, explicit close, fragmentation/reassembly, send gating + timeout semantics, and new tests. |
| src/stream.rs | Adds WebRTC-aware helpers for transport identification, explicit WebRTC close, relay status, and DTLS fingerprint retrieval. |
| src/bytes_codec.rs | Exposes MAX_FRAME_LENGTH constant and reuses it in encoding/tests to align frame-size limits across transports. |
| protos/rendezvous.proto | Adds SDP offer/answer fields and introduces an IceCandidate message for trickle ICE signaling. |
| protos/message.proto | Extends IdPk with a signed DTLS fingerprint for binding WebRTC DTLS identity to the authenticated peer. |
| Cargo.toml | Pins webrtc crate to 0.13.0 (and dev-dep) to preserve MSRV compatibility, with upgrade guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| acc.extend_from_slice(&scratch[1..n]); | ||
| // Match TCP's maximum frame size while preventing an unbounded FRAG_MORE stream from | ||
| // exhausting memory. | ||
| if acc.len() > MAX_FRAME_LENGTH { | ||
| acc.clear(); |
| if self.relay_only { | ||
| return Some(true); | ||
| } | ||
| let dtls = self.pc.sctp().transport(); | ||
| dtls.ice_transport().get_selected_candidate_pair().await?; | ||
|
|
Debug output is written to the log file, so a log site that fires per received message lets whoever is sending decide how much a machine writes to disk. Dropping the line instead would hide real faults, so collapse it: one line per interval carrying the count of everything suppressed since the last one, with the first occurrence after a quiet period always reported so an isolated fault is not delayed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/log_throttle.rs`:
- Around line 14-20: The Rustdoc example for LogThrottle currently cannot
compile because its referenced types are unresolved. Update the example to
import or qualify Duration, LogThrottle, and hbb_common as needed, then remove
the ignore marker so rustdoc can validate it; use a plain text block instead
only if it is not intended as executable documentation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b94b937-7e53-4a8e-be42-1b45d37318ff
📒 Files selected for processing (2)
src/lib.rssrc/log_throttle.rs
| /// ```ignore | ||
| /// static DROPPED_ICE: LogThrottle = LogThrottle::new(Duration::from_secs(60)); | ||
| /// | ||
| /// if let Some(n) = DROPPED_ICE.due() { | ||
| /// log::debug!("dropped {n} ICE candidate(s) with no route"); | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify the crate name and inspect the documented example.
fd -a '^Cargo\.toml$' -x sh -c 'echo "--- $1"; rg -n "^(name|rust-version|edition)\s*=" "$1"' sh {} \;
sed -n '1,24p' src/log_throttle.rsRepository: rustdesk/hbb_common
Length of output: 1116
🌐 Web query:
Rustdoc code-block languages ignore no_compile_check # no_run compile_fail
💡 Result:
There is no official code-block attribute named no_compile_check in Rustdoc [1][2][3]. If you are looking for ways to control how Rustdoc handles code blocks, the following standard attributes are available: 1. ignore: Tells Rustdoc to ignore the code block entirely when running tests [2][3]. While the code will not be tested (neither compiled nor run), it will still be syntax-highlighted as Rust code if you specify rust [4]. 2. no_run: Compiles the code but does not execute it [2][3]. This is useful for code that might be unsafe, requires network access, or demonstrates behaviors that cannot be run in a standard test environment [2]. 3. compile_fail: Asserts that the code should fail to compile [2][3]. If the code compiles successfully, the test will fail [2]. If you intended to write a code block that should not be interpreted as Rust code at all (and thus not tested), you should use the text language identifier instead of rust [2]. For example: text This block is treated as text and will not be tested by rustdoc. There have been discussions within the Rust community regarding a potential no_compile attribute to distinguish between "ignored" (but still compiled) and "do not compile" states, but this has not been implemented as a standard code-block attribute [5][6].
Citations:
- 1: https://doc.rust-lang.org/rustdoc/documentation-tests.html
- 2: https://doc.rust-lang.org/stable/rustdoc/write-documentation/documentation-tests.html
- 3: https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html
- 4: https://stackoverflow.com/questions/42348075/how-to-put-a-line-into-the-documentation-which-is-ignored-for-doc-tests
- 5: add
no_compiledoctest attribute rust-lang/rust#96573 - 6: Flag for stop interpreting code blocks as doctests rust-lang/rust#63193
🌐 Web query:
doc test ignored rust code still compiled not linted
💡 Result:
In Rust documentation tests, annotating a code block with the ignore attribute (e.g., ignore) instructs the test runner to skip execution [1][6]. However, this does not mean the code is completely ignored by the toolchain: 1. Compilation and Linting: By design, the ignore attribute does not prevent the code from being parsed or linted by rustdoc [1][2][4]. Because rustdoc must still parse the block to identify it as a code example, it will still trigger certain lints, such as rustdoc::invalid_rust_codeblocks, if the content contains invalid syntax [9][10]. The block is not treated as a standard, runnable test, but it is still subject to the initial processing phases of the documentation test pipeline [2][4]. 2. Behavior with --ignored: It is a common point of confusion that code blocks marked with ignore are still technically included in the test suite [2][4]. When running cargo test, they are reported as ignored [3][7]. If you explicitly run the test suite with the --ignored flag (e.g., cargo test --doc -- --ignored), the runner may attempt to compile and execute them, depending on the current version of the toolchain and the specific configuration [2][4][7]. 3. Alternatives: Because ignore is often used for non-compilable snippets (like pseudocode or documentation that should not be tested), users frequently request a way to truly "disable" a block [2][4]. Official recommendations for such cases include: - Using the text language tag instead of rust (e.g., text) if you do not want the code to be treated as Rust code at all [1][2]. - Using hidden lines (#) to make the snippet compilable while showing only the relevant parts to the reader [1][3]. - If you need to ensure code does not compile, use the compile_fail attribute, though this still subjects the code to the compilation process [1][2].
Citations:
- 1: https://doc.rust-lang.org/stable/rustdoc/write-documentation/documentation-tests.html
- 2: https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html
- 3: https://doc.rust-lang.org/rust-by-example/testing/doc_testing.html
Make the Rustdoc example compilable or use a plain text block.
The ignored example still shows unresolved Duration and LogThrottle, so a caller who copies it into another module cannot compile it. Add Duration, LogThrottle, and hbb_common:: as needed, then drop ignore; or use text if the example is only illustrative.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/log_throttle.rs` around lines 14 - 20, The Rustdoc example for
LogThrottle currently cannot compile because its referenced types are
unresolved. Update the example to import or qualify Duration, LogThrottle, and
hbb_common as needed, then remove the ignore marker so rustdoc can validate it;
use a plain text block instead only if it is not intended as executable
documentation.
…oning Rotating on age alone let a single day's file grow without limit, so whoever can drive a hot log site decided how much disk this uses and no amount of per-site throttling could bound it. Add a size criterion, which covers every call site at once — including ones no throttle was added to. LogThrottle: recover the guard on a poisoned lock rather than returning None. Poisoning only means another thread panicked while holding it; the guarded data is two counters that are still usable, and going silent for the rest of the process is worse than a stale count. AGENTS.md permits handling lock poisoning directly, and it forbids swallowing the error. Keep map_or over clippy's is_none_or: that was stabilized in Rust 1.82 and CI pins 1.75. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
…docs `next()` read every non-FRAG_END header as "more fragments", so a peer whose framing had diverged was only caught by the MAX_FRAME_LENGTH cap — and a FRAG_MORE carrying no payload was never caught at all: it adds nothing to the accumulator, so the cap never trips and the loop spins for as long as the peer keeps writing, with no error and no teardown. Decide the header's meaning in one match, so a future header kind cannot be handled in one place and missed in the other. Neither case is reachable from send_bytes_inner, which emits FRAG_MORE only for a full MAX_FRAGMENT_PAYLOAD chunk. Release the accumulator on the error paths rather than truncating it: at the cap that is ~1 GiB still referenced through the SESSIONS clone. Doc corrections, all of them overclaims in the previous pass: - the cancel-safety entry held only for the successful read path. read_data_channel does await after dequeuing on its ErrShortBuffer and DCEP branches, and next() awaits pc.close() on its error paths — where RTCPeerConnection::close latches is_closed before its first await, so a cancelled close silently turns every later close into a no-op and leaves the pc in SESSIONS. - recv_state: cancellation drops the guard mid-message, so it is the single-reader assumption, not the mutex, that ultimately keeps two readers from splicing into one accumulator. - is_relayed: stream.rs promised None before pair selection while webrtc.rs documented Some(true) under Relay policy; align both. - get_local_endpoint: examples/webrtc.rs calls it too, not only the tests. - PunchHole.reserved 11: named the wrong writer — PunchHole is written by the rendezvous server, not by peers. Reserve the name as well as the tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`requester_id = 11` was added and removed in the same rebase batch, never reached main, and never reached hbbs — whose vendored copy of this file still stops at field 9. So nothing has ever written or read tag 11, and reserving it guards a wire format that does not exist. It was also inconsistent with what this branch already does: `IceCandidate` retyped tag 2 from `string to_id` to `bytes socket_addr` in place, which is only sound because none of this proto has shipped. Same premise, so tag 11 is free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
use_ws() folds into force_relay because a ws tunnel kills classic TCP/UDP punching — but ICE opens its own sockets and does not care how signaling reaches the server. Without a signal, the controlled side must treat every force_relay offer as Relay-only ICE (answer gated on TURN), which locks WebSocket deployments out of direct WebRTC entirely. webrtc_all_ice marks an offer that gathered every candidate type: the controller's force_relay covers only classic punching, not ICE policy. Absent/false keeps today's semantics on every skew combination (old controller, old server dropping the field, old controlled side). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
… field Reverts the webrtc_all_ice proto field (64b54ab) in favor of an `ice_policy: "all"` key inside the webrtc:// envelope JSON, next to the RTCSessionDescription fields. Same information, better carrier: - it is a property of the offer itself, so it rides with the offer; - the rendezvous server never has to know: the envelope is an opaque, length-bounded string to hbbs, so no forwarding code and no vendored proto copies to keep in sync; - serde ignores unknown JSON keys when parsing RTCSessionDescription, so every skew combination degrades exactly like the proto field did: absence - not an error - is the old Relay-only reading. endpoint_declares_all_ice() is the receiving side: parse failure, foreign scheme or missing key all read as "not declared". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
Same pattern as enable-udp-punch / enable-ipv6-punch: empty value reads as on against the public server and off against a private one (the injection lives in rustdesk's get_local_option), so self-hosted deployments opt in once their server / TURN is ready. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
The type alone still needs a static plus an `if let` at every use, which is why the codebase kept hand-rolling equivalents. The macro declares the static for itself, so adding a bounded site is one line, and it appends the multiplicity only when there is one to report - an isolated event logs exactly as it would unthrottled. Count semantics stay inclusive (the reported number is the total this line stands for, first occurrence = 1), so a reader needs no arithmetic; the type's docs now point at the macro and say when to reach past it. Also rustfmt the module and webrtc.rs, which had drifted (no CI gate enforces it on this branch). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
…a channel Four review findings on the receive path, all verified against the vendored webrtc-rs rather than inferred: - next() awaited pc.close() on every error/EOF path while every consumer polls next() inside a select! against a 1s timer. close() latches is_closed before its first await and fires the state handler last, so losing that race left a pc no later close() could retry, a SESSIONS entry only that handler evicts, and a state_notify that never reaches Closed. close_detached() hands the teardown to the runtime; being a non-async fn, its callers have no await point to be cancelled at. The send path's timeout arm passes its logical-message permit along, so the exclusion it relies on now outlives the caller too. - the reassembly cap was checked after extend_from_slice, so the peak was the cap plus a fragment, and BytesMut's reallocate-and-copy growth held old and new buffers at once. Check before appending, and stop borrowing bytes_codec's ~1 GiB MAX_FRAME_LENGTH: that bound is only affordable for TCP because its length prefix rejects an oversize frame before buffering any of it, while this framing can only discover the overrun by accumulating it - and the answerer runs before any password check. MAX_RECV_MESSAGE (64 MiB) bounds both directions. - the EOF path claimed an empty message could never be confused with a reset. It can: webrtc-data maps the StringEmpty/BinaryEmpty PPIDs to n == 0 and dc.read() discards the flag that separates them. Both mean the same thing to us, so the handling stands - the comment and the log line now say what actually happened. - on_data_channel bound whatever the remote opened, however it opened it. Reassembly spans messages, so it is sound only on an ordered, fully-reliable channel, and webrtc-rs derives those parameters verbatim from the remote's DCEP OPEN; extra channels additionally split teardown from the channel carrying traffic and re-arm Open over a latched Closed. Refuse both. Also release the accumulator on the EOF and read-error exits, which were the only paths that left a partial message reachable through the SESSIONS clone. Regression tests for the detached teardown and the bind-once guard, both mutation-checked; the test comments state what is and is not covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
… by volume Continues the review pass. Eight findings, each verified against the vendored webrtc-rs (0.13 / -data 0.11 / -ice 0.13) rather than inferred: - a mid-message dc.write() failure returned the error and left the pc open, so the peer kept an unterminated FRAG_MORE prefix and appended the next message to it - undetectable, since this framing carries no length or sequence number, and callers wrap sends in allow_err!. Close the stream when fragments are already on the wire. - one deadline covered the connect-wait, the gate queue and every write. A slow ICE/DTLS completion therefore ate the budget and the write timed out into a pc.close() an RTT from working; and the gate arm closed the pc from a task that never held the permit, aborting a healthy sender's fragment sequence - the exact corruption the permit exists to prevent. Connection setup gets its own budget, and only the arm holding the permit tears down. - a SESSIONS hit returned a pc built for the first caller, so a replayed offer could get an All-policy connection where the mediator had just computed Relay-only, with is_relayed() answering from the cached handle. It could also hand back a stream the state handler had already closed (it closes before it evicts). Reject both; peer_verified stays shared, being a fact about the certificate the entry is keyed by. - the ICE-candidate sender lives in the on_ice_candidate handler and close() clears no handler, so the receiver never closed and the forwarder loop this API asks callers to write parked on recv() holding a stream clone - one leaked task plus one leaked pc per connection. The terminal-state handler now drops that closure. Regression test included (fails, by 20s timeout, with the drop removed). - has_turn_server() accepted the RFC 7065 spelling `turn:host:port`, which url makes cannot-be-a-base: host_str() is None, so the server became a hostless "turn::3478" that still passed the scheme check. force_relay then built a Relay-only pc that could only time out. Parse host and port out of the path (IPv6 literals included), and make the gate require a host. - is_relayed() read the stats report's `nominated` flag, which webrtc-ice sets per checklist entry and never clears, so after a pair switch several entries carry it and HashMap order picked the answer. Read the selected pair instead - which also drops a redundant stats round trip. - the ICE-server parsing tests rewrote the process-global, on-disk `ice-servers` option while sibling loopback tests were building peer connections from it. Split parsing out of get_ice_servers so they can test it directly; the suite now passes in parallel. - log retention is a file count, so pairing it with the new 16 MiB size criterion let a flooder rotate away every file predating its own activity. Keep enough files that ~31 days survives even at full size. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
Summary by CodeRabbit
New Features
Bug Fixes