diff --git a/src/stream.rs b/src/stream.rs index 135dbaa99..676fd11f7 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -203,13 +203,21 @@ impl Stream { pub fn from(stream: TcpStream, stream_addr: SocketAddr) -> Self { Self::Tcp(tcp::FramedStream::from(stream, stream_addr)) } +} - #[inline] - #[cfg(feature = "webrtc")] - pub fn get_webrtc_stream(&self) -> Option { - match self { - Self::WebRTC(s) => Some(s.clone()), - _ => None, - } +/// Owning the stream owns the transport, WebRTC included. +/// +/// A peer connection outlives its handle — the session cache holds a clone, and the handler that +/// evicts it only fires on a terminal ICE state — so it has to be closed explicitly. Doing that +/// at each exit path made it an obligation every `return`, `break` and `?` had to remember, and +/// the long-lived side never did: `server::connection` ends its ~15 exits by dropping the stream. +/// Nothing warned, because a missed close leaks silently and only under WebRTC. +/// +/// `Stream` is not `Clone`, so dropping it really is the end of the transport and there is no +/// second owner to surprise. Explicit `close_webrtc()` calls remain valid — they close sooner +/// than scope end — but they are now an optimization rather than the thing correctness rests on. +impl Drop for Stream { + fn drop(&mut self) { + self.close_webrtc(); } } diff --git a/src/webrtc.rs b/src/webrtc.rs index fdb7297be..a81fc8612 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -1645,6 +1645,31 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc panic!("detached close never evicted the peer connection from SESSIONS"); } + // Owning the Stream owns the peer connection: dropping it must close the pc and evict the + // session, without the owner having to remember to. Every exit path used to carry that + // obligation, and the controlled side never honoured it. + #[tokio::test] + async fn dropping_the_stream_closes_the_peer_connection() { + let offerer = WebRTCStream::new("", false, 20000).await.unwrap(); + let key = format!("offer:{}", offerer.session_key()); + assert!( + SESSIONS.lock().await.contains_key(&key), + "offerer should be cached while live" + ); + + // No explicit close anywhere: the stream simply goes out of scope, as it does on the + // exits that forget. + drop(crate::Stream::WebRTC(offerer)); + + for _ in 0..200 { + if !SESSIONS.lock().await.contains_key(&key) { + return; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + panic!("dropping the stream left the peer connection in SESSIONS"); + } + // A replayed offer asking for a different ICE policy must not be handed the cached peer // connection. The lookup and the insert-time duplicate check read the same key, so the test // fails if either one stops applying `is_reusable_for` — which is how the guard was dead