Repository navigation
fix(rtc): make Room.connect cancellation-safe to avoid FFI timeout panic - #880
NabiBukhsh-AI wants to merge 1 commit into
Conversation
a0bb864 to
a483762
Compare
9cdd009 to
95764c1
Compare
| # while guaranteeing that when ConnectCallback arrives the FFI handshake is | ||
| # completed and cleanly disconnected (Issues #784, #804). | ||
| queue_unsubscribed = True | ||
| self._loop.create_task( |
There was a problem hiding this comment.
should we hold the reference of cleanup_task ? like
self._cleanup_tasks: set[asyncio.Task] = set() # in init
...
t = self._loop.create_task(self._cleanup_cancelled_connect(...))
self._cleanup_tasks.add(t)
t.add_done_callback(self._cleanup_tasks.discard)
otherwise GC can just drop the task and the original bug could come back non-deterministically
There was a problem hiding this comment.
Good call! Added self._cleanup_tasks: set[asyncio.Task[None]] = set() in f110eef, tracking all active cleanup tasks and awaiting �syncio.shield(asyncio.gather(*self._cleanup_tasks)) in connect() and disconnect() to prevent garbage collection and eliminate race conditions.
| self._remote_participants.clear() | ||
| self._local_participant = None | ||
| self._info = proto_room.RoomInfo() | ||
| if self._first_sid_future.done(): |
There was a problem hiding this comment.
wonder if it is better to do
def _fail_sid_waiters(self, err: BaseException) -> None:
fut = self._first_sid_future
if not fut.done(): # required: avoids InvalidStateError on the common path
fut.set_exception(err)
fut.exception() # required: suppresses never-retrieved noise
self._first_sid_future = asyncio.Future[str]()
and call this in both except asyncio.CancelledError (Path A) and except BaseException
There was a problem hiding this comment.
Implemented in f110eef: added _fail_sid_waiters exactly as suggested and invoked in both CancelledError and BaseException teardowns, resetting _first_sid_future cleanly with regression verification in est_room_connect_cancellation_fails_sid_waiters.
|
Hi @xianshijing-lk, All requested updates and synchronization improvements are in place:
|
5d5bd9e to
bf0fc1e
Compare
2b9dfa1 to
51c0225
Compare
51c0225 to
2b6f463
Compare
2b6f463 to
4dcf3cf
Compare
4ca77f0 to
813f768
Compare
ab44139 to
d298b99
Compare
# Conflicts: # livekit-rtc/tests/test_ffi_handle_lifecycle.py
d298b99 to
04dcda8
Compare
|
👋 This PR has no changeset. If it should be released, add a markdown file under ---
"livekit-rtc": patch
---
Short description of the change, used in the release notes.
If this change doesn't need a release (docs, CI, tests), you can ignore this comment. |
|
Hi @xianshijing-lk @cloudwebrtc, Following up on the earlier review feedback:
All 44 CI checks and Devin review have completed with |
Summary
Cancelling
Room.connect()whileConnectRequestis in flight previously stranded the FFI room-event handshake becauseReadyForRoomEventRequestwas never dispatched. When the 15-secondROOM_EVENT_READY_TIMEOUTexpired in native Rust FFI, it raised an FFI panic that caused Python'sFfiClientto kill the process withSIGTERM(#784). Furthermore, the native connection remained open on the LiveKit server in the background, causing subsequent reconnect attempts using the same token to disconnect due toduplicated identity(#804).This PR makes
Room.connect()cancellation-safe and ensures that cancellation or initialization exceptions reliably complete the FFI handshake, cleanly disconnect the session, and dispose of the native room handle. Additionally, reconnects and disconnects coordinate with any in-flight background cleanup so immediate retry attempts never collide with an unreleased native session.Root Cause Analysis
Room.connect(),cb = await queue.wait_for(...)is the only await point during the connection handshake.ConnectCallback,asyncio.CancelledErrorabortsconnect().ConnectCallback, then waits up to 15 seconds forReadyForRoomEventRequestfrom the Python SDK.ReadyForRoomEventRequestwas never sent. When the 15-second timer expired, Rust FFI reported a panic, causing_ffi_client.pyto terminate the process withos.kill(os.getpid(), signal.SIGTERM).Key Changes
Room.connect(), catchasyncio.CancelledErroronqueue.wait_for().self._loop.create_task(self._cleanup_cancelled_connect(...))and re-raiseCancelledErrorimmediately so caller timeouts and cancellations are unblocked promptly.ReadyForRoomEventRequestwith the allocated handle, satisfying the FFI 15-second timer and preventing the SIGTERM panic.DisconnectRequestto cleanly tear down the participant session on the server, avoiding duplicate identity errors on reconnect.FfiHandle, and unsubscribeself._ffi_queue.self._ffi_queueand finish cleanly.self._cleanup_taskonRoom.connect()and indisconnect(), checkif self._cleanup_task is not None and not self._cleanup_task.done(): await asyncio.shield(self._cleanup_task).Roominstance from colliding with a session whose native teardown is still in flight.ConnectCallbackhas arrived but beforeReadyForRoomEventRequestis sent, run the cleanup sequence to prevent stranding the FFI room, and clear room state so subsequent retry attempts start fresh.await self._taskindisconnect()withif self._task is not None:.Tests Added
Added unit test suite in
livekit-rtc/tests/test_room_connect_cancellation.py:test_room_connect_cancelled_during_handshake_cleans_up: Verifies that cancelling an in-flightconnect()sendsready_for_room_event, triggersdisconnect, disposes of the handle, and raisesCancelledError.test_room_connect_cancelled_when_connect_fails: Verifies that cancelling when the connection fails cleanly unsubscribes the queue without dispatching disconnect requests.test_room_connect_wait_for_timeout_cancellation: Verifiesasyncio.wait_for(room.connect(...), timeout=...)timeout cancellation cleans up properly.test_room_connect_setup_failure_cleans_up: Verifies that an exception during room setup beforeready_for_room_eventcompletes the handshake and closes the room session.test_room_connect_repeated_cancellation_during_cleanup: Verifies that repeated cancellations while waiting for cleanup callbacks do not interrupt the teardown sequence.test_room_connect_subsequent_connect_waits_for_cleanup: Verifies that retryingconnect()on a room with an in-flight background cleanup waits for the previous teardown to complete before issuing the new connection request.Fixes #784
Fixes #804