Skip to content

rtc: close the room when Room.connect is cancelled - #813

Open
ubmids wants to merge 1 commit into
livekit:mainfrom
ubmids:fix/connect-cancellation
Open

ubmids wants to merge 1 commit into
livekit:mainfrom
ubmids:fix/connect-cancellation

Conversation

@ubmids

@ubmids ubmids commented Sep 14, 2026

Copy link
Copy Markdown

Cancelling room.connect() takes the process down with it, as #784 describes. I could
reproduce it exactly as filed, so here is a fix for the Python side of it.

connect() sends ReadyForRoomEventRequest as the last statement in its body, so a
coroutine cancelled anywhere inside it never gets there. The FFI server has already
answered the connect request by then and is waiting for that ready request. After 15s it
gives up:

livekit_ffi::server::room:261 - timed out waiting for ReadyForRoomEventRequest after ConnectCallback (room_handle=3)
FFI Panic:  invalid request: timed out waiting for ReadyForRoomEventRequest after ConnectCallback (room_handle=3)

and _ffi_client.py answers every panic with os.kill(os.getpid(), signal.SIGTERM).

There is no way around it from the caller side. RoomOptions.connect_timeout is
documented as per signal connection attempt rather than a bound on the join, and
disconnect() early returns while isconnected() is false, so it does nothing after a
cancel.

The only cancellation point in connect() is the await on the connect callback,
everything after it is synchronous up to the ready request. So this catches
CancelledError there and hands the room to a task that outlives the cancellation: wait
for the connect callback the server is going to send anyway, answer the ready request,
then disconnect. disconnect() awaits that task, so a caller that cancels and then
disconnects is deterministic.

Measured against livekit-server --dev 1.13.7, macOS arm64, CPython 3.12, cancelling
with a 0.2s wait_for:

before

[  1.04s] connect cancelled by wait_for
[  1.04s] disconnect() returned
[ 16.06s] FFI Panic: timed out waiting for ReadyForRoomEventRequest (room_handle=3)
[ 16.36s] SIGTERM, exit

after: the process survives, and disconnect() takes about 0.25s actually closing the
room.

That second part is the symptom in #804. Reconnecting with the same identity after a
cancelled connect:

first connect retry outcome
before cancelled at 0.2s blocked 15s on wait_pc_connection timed out sigterm at 15.4s
after cancelled at 0.2s connects in 0.07s survives

Two things this deliberately does not do.

#804 asks for the FFI handle and the connect request to be decoupled so an abort can be
sent mid-connect. This does not do that. The connect still runs to completion inside the
FFI and is closed immediately afterwards, which removes the duplicate-identity symptom
but is not the same thing, so I have left that issue open rather than claiming it.

#785 asks for the unconditional SIGTERM on panic to be reconsidered. Left alone, it reads
like a policy call for you rather than something to slip into a bug fix. The reporter of
both frames it the same way.

One limitation worth stating: the cleanup runs as a task, so if the event loop stops
before it gets a turn (cancel, then exit immediately) the FFI can still time out.
Awaiting disconnect() is what makes it deterministic.

Tests are in livekit-rtc/tests/test_connect_cancellation.py. They use the fake-FFI style
from test_audio_stream_room_lifecycle.py so they need no credentials: they assert the
request sequence is connect → ready_for_room_event → disconnect, that a connect error
sends nothing further, and that no queue subscription leaks. Both fail on main. Locally
the rtc suite is 59 passed / 14 skipped (the credentialed ones), ruff 0.15.4 and mypy
clean apart from the pre-existing jupyter.py IPython imports.

Claude helped me write this. I can explain every line of it, and the numbers above are
from runs on my own machine.

Fixes #784
Refs #804

The FFI server has no cancel path for an in-flight connect: it answers the
connect request and then waits for ReadyForRoomEventRequest, which connect()
sends as its last statement. A coroutine cancelled anywhere inside connect()
never reaches it, the server times out after 15s and panics, and the panic
handler sends SIGTERM to the process.

Hand the room to a task that survives the cancellation, answer the pending
ready request and disconnect. disconnect() waits for that task so callers can
close deterministically, and the room no longer stays joined server-side,
which is what evicts a retry using the same identity.

Fixes livekit#784
Refs livekit#804
@CLAassistant

CLAassistant commented Sep 14, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Devin Review

if self._aborted_connect_tasks:
# a cancelled connect may still be closing a room the FFI server opened.
# wait for it so disconnect() leaves nothing behind.
await asyncio.gather(*tuple(self._aborted_connect_tasks), return_exceptions=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Cancelled disconnect kills connect cleanup

When disconnect() is cancelled with an aborted connect pending, its gather cancels the cleanup task. No ready request follows, so the panic handler terminates the process after FFI timeout.

Learn more

The aborted-connect task owns the only subscription waiting for the native connect callback. Cancelling disconnect() cancels its gather, which cancels that task. Its finally block then unsubscribes the queue before the callback arrives. Nothing sends ReadyForRoomEventRequest, and the native timeout invokes the panic handler.

Example: A caller cancels connect(), then wraps disconnect() in a one-second timeout. If the connect callback takes longer, the timeout cancels both disconnect() and _close. Fifteen seconds later, the process receives SIGTERM instead of completing cleanup in the background.

Recommended fix: Shield each task in _aborted_connect_tasks from disconnect() cancellation. This preserves cancellability of disconnect() while allowing _close to send ready and disconnect when the callback arrives.

Suggested change
await asyncio.gather(*tuple(self._aborted_connect_tasks), return_exceptions=True)
await asyncio.gather(
*(
asyncio.shield(task)
for task in tuple(self._aborted_connect_tasks)
),
return_exceptions=True,
)
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Room.connect is not cancellation-safe: cancelling it strands the FFI room-event handshake, and the process is SIGTERMed 15s later

2 participants