diff --git a/Cargo.lock b/Cargo.lock index e09e6ee..1d68e8c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1355,7 +1355,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2334,7 +2334,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2724,7 +2724,7 @@ dependencies = [ "once_cell", "socket2", "tracing", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2736,7 +2736,7 @@ dependencies = [ "cfg_aliases", "libc", "socket2", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -3013,7 +3013,7 @@ dependencies = [ [[package]] name = "rtc" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "bytes", "hex", @@ -3047,7 +3047,7 @@ dependencies = [ [[package]] name = "rtc-datachannel" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "bytes", "log", @@ -3059,7 +3059,7 @@ dependencies = [ [[package]] name = "rtc-dtls" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "aes", "bytecheck", @@ -3091,7 +3091,7 @@ dependencies = [ [[package]] name = "rtc-ice" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "bytes", "crc", @@ -3109,7 +3109,7 @@ dependencies = [ [[package]] name = "rtc-interceptor" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "log", "rand", @@ -3123,7 +3123,7 @@ dependencies = [ [[package]] name = "rtc-interceptor-derive" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "proc-macro2", "quote", @@ -3133,7 +3133,7 @@ dependencies = [ [[package]] name = "rtc-mdns" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "bytes", "log", @@ -3145,7 +3145,7 @@ dependencies = [ [[package]] name = "rtc-media" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "byteorder", "bytes", @@ -3158,7 +3158,7 @@ dependencies = [ [[package]] name = "rtc-rtcp" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "bytes", "rtc-shared", @@ -3167,7 +3167,7 @@ dependencies = [ [[package]] name = "rtc-rtp" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "bytes", "memchr", @@ -3179,7 +3179,7 @@ dependencies = [ [[package]] name = "rtc-sctp" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "bytes", "crc32c", @@ -3194,7 +3194,7 @@ dependencies = [ [[package]] name = "rtc-sdp" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "rand", "rtc-shared", @@ -3204,7 +3204,7 @@ dependencies = [ [[package]] name = "rtc-shared" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "aes", "aes-gcm", @@ -3225,7 +3225,7 @@ dependencies = [ [[package]] name = "rtc-srtp" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "aes", "byteorder", @@ -3243,7 +3243,7 @@ dependencies = [ [[package]] name = "rtc-stun" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "base64", "bytes", @@ -3261,7 +3261,7 @@ dependencies = [ [[package]] name = "rtc-turn" version = "0.20.0-rc.4" -source = "git+https://github.com/webrtc-rs/rtc.git?rev=a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2#a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" +source = "git+https://github.com/lann/rtc.git?rev=353a84a550a8201d4f639d3d61800fd67821e9ac#353a84a550a8201d4f639d3d61800fd67821e9ac" dependencies = [ "bytes", "log", @@ -3316,7 +3316,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -3744,7 +3744,7 @@ dependencies = [ "getrandom 0.4.3", "once_cell", "rustix", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -4895,7 +4895,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 8e1d61e..35c6b12 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,9 +50,12 @@ rtc = "0.20.0-rc.4" # that is not yet in any published release: connectivity checks from a # server-reflexive candidate are sent from its base rather than the NAT-mapped # address, so drivers that demultiplex outbound transmits by local address no -# longer drop them (TODO item E4's netns-lab finding). Using `[patch]` rather +# longer drop them (TODO item E4's netns-lab finding). The pinned rev is +# upstream a10cd2c plus one cherry-pick from https://github.com/lann/rtc/pull/1 +# (sctp: don't discard received-but-undelivered data on incoming stream reset — +# TODO item E18's receive-side data loss). Using `[patch]` rather # than a direct git dependency is what lets the fix reach `webrtc`'s own copy # of `rtc`. Drop the patch and return to a plain crates.io version once a -# release including the fix ships (see TODO item E6). +# release including the fixes ships (see TODO item E6). [patch.crates-io] -rtc = { git = "https://github.com/webrtc-rs/rtc.git", rev = "a10cd2c18f7c4e646e83f6f00b792ef38ac3cdb2" } +rtc = { git = "https://github.com/lann/rtc.git", rev = "353a84a550a8201d4f639d3d61800fd67821e9ac" } diff --git a/TODO.md b/TODO.md index 001e8d1..da72897 100644 --- a/TODO.md +++ b/TODO.md @@ -61,13 +61,17 @@ concrete extension of the existing machinery: ### E6. Unwind the `rtc` git pin once upstream ships a release -The `rtc` dependency is pinned to an upstream `master` commit (`Cargo.toml` -`[patch.crates-io]`) because the srflx source-address fix +The `rtc` dependency is pinned to a fork commit (`Cargo.toml` +`[patch.crates-io]`, `lann/rtc`) carrying two fixes not yet in any published +release: the srflx source-address fix ([`webrtc-rs/rtc#136`](https://github.com/webrtc-rs/rtc/pull/136), merged upstream; the netns lab's `stun-srflx` scenario passes with zero dropped -srflx-sourced transmits with it, vs ~100 drops without) is not yet in any -published release. Drop the patch and return to a plain crates.io version -once a release including it ships. +srflx-sourced transmits with it, vs ~100 drops without) and the +receive-side stream-reset data-loss fix +([`lann/rtc#1`](https://github.com/lann/rtc/pull/1), see E18 — an incoming +stream reset discarded received-but-undelivered messages). Drop the patch +and return to a plain crates.io version once a release including both +ships. ### E12. node-webrtc receive-side reordering: file the write-ups upstream @@ -183,26 +187,46 @@ copy onto the pinned version via npm `overrides` — keep those blocks until driver-loop errors, so `patch-driver-loop-errors.mjs` remains warranted, as does the upstream report of the swallowing. -### E18. Undiagnosed data loss: messages sent before close by the libwebrtc reference offerer sometimes never arrive - -`channel-close-flush` fails on roughly a third of `reference-x-wasmtime` -and `reference-x-jco-node` interop runs with `answerer: receive: closed`: -the answerer observes the channel closed before it has received every -payload the reference offerer sent immediately before closing. Messages -that were handed to `send` are never delivered — this is data loss, not a -test artifact. It reproduces with two unrelated answerer stacks -(webrtc-rs via the wasmtime host, and `node-datachannel` via jco-node), -so the common factor is the libwebrtc reference offerer (or the -offerer-side close path in `conformance/adapters/reference`); note the -W3C spec leaves it to the transport layer whether buffered messages are -sent or discarded on close, and libwebrtc may be discarding. Undiagnosed -beyond that. This is pre-existing on `main` (the pair's code is identical -there; CI has simply been passing by chance) and is distinct from E16 -(answerer-side TSFN loss in `node-datachannel`) — though it may account -for some failures previously attributed to E16. Diagnose (packet capture -or libwebrtc logging on the reference peer would settle which side drops -them) and fix or work around; do not paper over it with a manifest -expected-fail, since the test passes most runs. +### E18. `channel-close-flush` data loss on reference-peer pairs: diagnosed, one receiver-side bug remains + +`channel-close-flush` failed intermittently (roughly a third of runs, +locally and in CI) on interop pairs involving the libwebrtc reference +peer. Diagnosis: the reference peer was blameless — its immediate +close-after-send (spec-legal; libwebrtc flushes queued data before the +SCTP stream reset, per RFC 8831 §6.7) coalesces the tail DATA chunks and +the reset RECONFIG into one network burst, and three distinct +*receiver-side* bugs mishandled that burst: + +1. **`rtc-sctp` discarded received-but-undelivered messages on an incoming + stream reset** (`reference-x-wasmtime`, `answerer: receive: closed`): + `reset_streams_if_any` → `unregister_stream` removed the stream and its + reassembly queue with reassembled-but-unread messages still inside, + before the driver polled the pending `Readable` events. Fixed by + [`lann/rtc#1`](https://github.com/lann/rtc/pull/1) (defer teardown + until the consumer drains the queue), carried in this repo's `rtc` pin + (see E6); upstream to `webrtc-rs/rtc`. +2. **The reference peer's `wait_open` treated an already-closing incoming + channel as an error** (`wasmtime-x-reference`, `answerer: channel + closed before open`): a remote-announced channel that reads + `closing`/`closed` was necessarily open first, and its messages are + already queued — the whole open→send→close sequence can land inside one + 25ms poll interval. Fixed in `conformance/adapters/reference`. +3. **The `webrtc` 0.20 driver delivers a data-channel close ahead of + already-received messages** (still failing, both `after 0 of 4` and + `after 3 of 4` shapes): the driver loop runs `poll_events()` before + `poll_reads()` each iteration, and the core surfaces stream-close via + the event queue but messages via the read queue — so a close arriving + in the same input batch as data overtakes it into the per-channel event + stream. A naive swap is unsafe (a message could race ahead of its + channel's `OnOpen`, whose handling creates the event channel); the fix + is to drain the pending read-queue messages for a channel before + forwarding its close. Fix upstream in `webrtc-rs/webrtc` + (`src/peer_connection/driver.rs`). + +`reference-x-jco-node` failures with the same trigger are E16 (the +`node-datachannel` TSFN close/message race), not a fourth bug. Do not +paper over the remaining failure with a manifest expected-fail: it is +data loss, and the test passes most runs. ## F. Conformance suite diff --git a/conformance/adapters/reference/src/main.rs b/conformance/adapters/reference/src/main.rs index 5066f9a..a0f9e8a 100644 --- a/conformance/adapters/reference/src/main.rs +++ b/conformance/adapters/reference/src/main.rs @@ -340,15 +340,26 @@ async fn wait_connected(wiring: &mut Wiring) -> Result<()> { } } -/// Wait until a locally created channel reports open, consuming (and -/// buffering back) nothing: open is observed via the channel's state. -async fn wait_open(channel: &DataChannel) -> Result<()> { +/// Wait until a channel reports open, consuming (and buffering back) +/// nothing: open is observed via the channel's state. +/// +/// A remote-announced channel that already reads `closing`/`closed` was +/// necessarily open first (announcement implies the open transition), and its +/// messages are already queued in the wiring — so for `remote` channels that +/// state counts as opened rather than an error. A close-after-send peer can +/// land the entire open→send→close sequence inside one poll interval. A +/// locally created channel has no such guarantee: closed-before-open there is +/// a failed open. +async fn wait_open(channel: &DataChannel, remote: bool) -> Result<()> { // Poll the state: the open transition may have raced callback // registration, and state reads are cheap and race-free. for _ in 0..1200 { match channel.state() { DataChannelState::Open => return Ok(()), DataChannelState::Closing | DataChannelState::Closed => { + if remote { + return Ok(()); + } anyhow::bail!("channel closed before open") } DataChannelState::Connecting => { @@ -400,7 +411,7 @@ async fn handshake( }; wait_connected(wiring).await?; - wait_open(&channel).await?; + wait_open(&channel, cli.role != "offerer").await?; Ok((channel, inbound)) } @@ -561,10 +572,13 @@ async fn exchange( } } else { // Every payload must arrive despite the peer's immediate - // close, after which the close itself is observed. + // close, after which the close itself is observed. A failure + // names how many messages had already arrived. let mut received = Vec::with_capacity(count as usize); for _ in 0..count { - let (_binary, data) = receive(inbound).await?; + let (_binary, data) = receive(inbound).await.map_err(|e| { + anyhow!("{e} (after {} of {count} messages)", received.len()) + })?; received.push(data); } verify_all(&received, count, false)?; diff --git a/conformance/guest/src/lib.rs b/conformance/guest/src/lib.rs index 1e32383..5035d66 100644 --- a/conformance/guest/src/lib.rs +++ b/conformance/guest/src/lib.rs @@ -518,11 +518,16 @@ async fn send_sequence(dc: &DataChannel, count: u32, size: u32) -> Result<(), St Ok(()) } -/// Receive `count` messages, returning their raw bytes. +/// Receive `count` messages, returning their raw bytes. A failure names how +/// many messages had already arrived, so a partial delivery (data loss on the +/// wire) is distinguishable from a receive path that never yielded at all. async fn recv_sequence(dc: &DataChannel, count: u32) -> Result>, String> { let mut out = Vec::with_capacity(count as usize); for _ in 0..count { - match receive(dc).await? { + match receive(dc) + .await + .map_err(|e| format!("{e} (after {} of {count} messages)", out.len()))? + { Message::Binary(bytes) => out.push(bytes), Message::String(text) => out.push(text.into_bytes()), }