Close peer connections before awaiting signal teardown - #1335
Conversation
`SessionInner::close` released the peer connections only after two awaits that can block indefinitely, so cancelling `close()` -- for example by wrapping it in a timeout -- dropped the future before the transports were ever closed, leaving their ICE UDP sockets bound for the lifetime of the process. Nothing else releases them, so long-lived clients eventually exhausted their file descriptors and could not recover. `PeerTransport::close` is synchronous, so closing before the first await means a cancelled teardown can no longer leak. The signalling socket is unaffected and the Leave still goes out.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // Both awaits below are unbounded, and `PeerTransport::close` is synchronous, so | ||
| // closing here — before the future can suspend — is what stops a cancelled | ||
| // `close()` leaving the ICE sockets bound for the process's lifetime. The | ||
| // signalling socket is unaffected, so the Leave below still goes out. | ||
| self.publisher_pc.close(); | ||
| if let Some(ref sub_pc) = self.subscriber_pc { | ||
| sub_pc.close(); | ||
| } |
There was a problem hiding this comment.
🔴 Cancelling a room shutdown still leaves the media connections open
The media transports are still shut down (self.publisher_pc.close() at livekit/src/rtc_engine/rtc_session.rs:1995) only after the shutdown has already waited on four background workers, so abandoning a shutdown early still leaves them open.
Impact: A caller who bounds room teardown with a short timeout still leaks the room's network sockets for the lifetime of the process, and the new test that asserts otherwise fails.
Why the reorder does not reach the first suspension point
Room::close → RoomSession::close (livekit/src/room/mod.rs:1175-1183) → RtcEngine::close → EngineInner::close (livekit/src/rtc_engine/mod.rs:1052-1065) → RtcSession::close (livekit/src/rtc_engine/rtc_session.rs:797-811). RtcSession::close awaits handle.rtc_task, handle.signal_task, handle.dc_task and handle.dt_sender_task before calling self.inner.close(reason).await, which is where the new publisher_pc.close() lives. Those JoinHandle awaits return Pending on the first poll (the tasks have only just been signalled via close_tx, and on the default current-thread test runtime they cannot even run while the close future is being polled), and the workers themselves contain unbounded awaits.
Consequently the future's first suspension happens at handle.rtc_task.await, still with both peer connections open. livekit/tests/room_test.rs:114-122 cancels at exactly that first suspension (timeout(Duration::ZERO, room.close())) and then asserts PeerConnectionState::Closed, which cannot hold under this call chain.
To make cancellation actually safe, the transport close needs to happen before the task joins in RtcSession::close (or at the very top of the teardown chain), not merely before the signal-client awaits in SessionInner::close.
Prompt for agents
The PR moves publisher/subscriber PeerConnection close to the top of SessionInner::close so that a cancelled close() (e.g. wrapped in a timeout) still releases ICE sockets. However, SessionInner::close is only reached after RtcSession::close (livekit/src/rtc_engine/rtc_session.rs, around lines 797-811) awaits four JoinHandles (rtc_task, signal_task, dc_task, dt_sender_task), and EngineInner::close/RoomSession::close sit above that. Those joins are the real first suspension points of the teardown future, so a cancelled close still leaves the transports open — and the new test test_cancelled_close_still_closes_peer_connections in livekit/tests/room_test.rs, which cancels at the first suspension, should fail. Consider closing the transports synchronously before the task joins (e.g. at the start of RtcSession::close, keeping SessionInner::close idempotent) or otherwise ensuring the synchronous PeerTransport::close runs before any await in the whole teardown chain, and re-verify the test.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Before you submit your PR
Make sure the following is true before submitting your PR:
PR description
Fixes #1334
SessionInner::closeclosed the peer connections after two awaits that can block indefinitely:PeerConnection::closeis the only thing that releases a room's ICE UDP sockets, so a caller boundingclose()with a timeout — a natural response to an unbounded API — dropped the future before it ran, and the sockets stayed bound for the process's lifetime. Nothing else releases them:engine_taskandroom_taskownArcs of the inner state and exit only onclose_rx, so an un-closed room keeps the graph alive and thePeerConnectiondestructor never runs.Both awaits really can park.
SignalStream::sendwaits on the writer task, which doesconn.send(data).awaitwith no timeout, so a half-open socket blocks it until TCP gives up.SignalStream::closethen joins that same task, and theInternalMessage::Closeit queues sits behind the stuck message in a capacity-8 channel. Every timeout insignal_clientis on the connect path.This moves the peer-connection close above both awaits.
PeerTransport::closeis synchronous, so it has already run by the time the future first suspends. The signalling socket is unaffected, so theLeavestill goes out.Measured on aarch64 with a bare connect/close loop, counting
/proc/self/fdsocket inodes against/proc/self/net/udp{,6}:close()cancelledclose()awaitedRoomdropped, never closedRoom-dropped is deliberately untouched: that needs the spawned tasks stopped so the graph can drop, which is a larger change.client-sdk-jsalready orders it this way —RTCEngine.close()closes the peer connections before the signal client.Breaking changes
None. Statement reorder inside a private
async fn; no public API or behaviour change.MSRV
Unchanged.
Testing
Adds
test_cancelled_close_still_closes_peer_connectionstolivekit/tests/room_test.rs, using the existingcommon::test_roomsharness.It cancels
close()at its earliest suspension point (timeout(Duration::ZERO, ..)— tokio polls the inner future once before the elapsed timer fires) and asserts the publisher transport isClosed. That only holds if the transports were closed before the future suspended, so moving the close back below an await fails the test. It also asserts the publisher wasConnectedfirst, so it cannot pass vacuously.Reading transport state needed a test-only accessor, following the existing
#[cfg(feature = "__lk-e2e-test")]pattern onRoom. Room-level state was not usable — a room can reportDisconnectedwhile its transport is still open, which is the bug.Reverting only the reorder, keeping the test:
With it restored, all four
room_testcases pass. Run withcargo test -p livekit --features __lk-e2e-test,rustls-tls-webpki-roots --test room_test; a TLS feature is needed only because this ran against awss://endpoint rather than a local--devserver.I also ran the rest of the suite, each file serially, with and without the change. Nothing regressed:
dynacast_test3/0 both,rpc_test6/0 both,data_track_test11 passed/4 failed both,reconnection_test4/1 both (same test),peer_connection_signaling_test20/7 before vs 23/4 after, plus the 76 lib unit tests green. The pre-existing failures look environmental rather than real —test_published_stateasserts a 20 ms tolerance that a cloud RTT cannot meet, and thenode_failure/resume_*cases want a server they can fault-inject. I did not have a locallivekit-server --devto confirm that, so I am reporting the comparison rather than claiming a green suite.Async
No new runtime dependencies; the production change introduces no
.await. The test usestokio::time::timeoutdirectly and contains no artificial delay — it polls once and asserts.