Skip to content

fix(ffi): hold the room weakly from its own RPC method handlers - #1437

Open
LautaroPetaccio wants to merge 1 commit into
livekit:mainfrom
LautaroPetaccio:fix/ffi-rpc-handler-weak-room
Open

LautaroPetaccio wants to merge 1 commit into
livekit:mainfrom
LautaroPetaccio:fix/ffi-rpc-handler-weak-room

Conversation

@LautaroPetaccio

@LautaroPetaccio LautaroPetaccio commented Sep 18, 2026

Copy link
Copy Markdown

The problem

FfiParticipant::register_rpc_method cloned Arc<RoomInner> into the handler it registers on the SDK:

let room: Arc<RoomInner> = self.room.clone();
local.register_rpc_method(method.clone(), move |data| { ... room ... });

That handler is stored on the room's own RPC server. The ownership chain is Arc<RoomInner>RoomArc<RoomSession>rpc_server.handlers (livekit-rpc/src/server.rs:50) → the closure → back to Arc<RoomInner>.

Nothing unregisters the method during teardown:

  • RoomSession::close fails pending client RPCs but never touches rpc_server.handlers.
  • FfiRoom::close does not touch it either.
  • FfiServer::dispose only clears the handle map, which drops one Arc out of a self-sustaining cycle and releases nothing.

So a single RegisterRpcMethodRequest makes the room outlive dispose(), keeping the engine, its peer connections and the PeerConnectionFactory resident for the rest of the process — precisely what the dispose() doc comment says it exists to prevent. Repeated connect/dispose cycles leak a full room each time. Only an explicit UnregisterRpcMethod per method broke the cycle.

The fix

Capture a Weak and upgrade per invocation, as AGENTS.md requires for a callback stored by the object it captures:

A callback stored by an object must not capture a strong clone of that same object.

An invocation arriving once the room is gone now fails cleanly with an application error rather than resurrecting it.

Verification

Adds __lk-e2e-test to livekit-ffi, mirroring the feature in livekit, and a test under it that drives the real FFI path: connect a room, complete the room-event ready handshake, register a method, dispose(), then assert the room's internals are released.

  • Before: fails — "the registered RPC handler retained the room after dispose".
  • After: passes.

Since making the capture weak is the only change, that strong capture was by definition the retaining reference.

All 17 livekit-ffi tests pass, including the existing dispose_cleans_up_resources. The default build (feature off) compiles, and cargo check --features __lk-e2e-test --tests at the workspace root — how CI invokes the suite — resolves correctly with the new feature.

devin-ai-integration[bot]

This comment was marked as resolved.

@LautaroPetaccio
LautaroPetaccio force-pushed the fix/ffi-rpc-handler-weak-room branch from e109955 to 8fc6711 Compare September 18, 2026 14:26
devin-ai-integration[bot]

This comment was marked as resolved.

`register_rpc_method` cloned `Arc<RoomInner>` into the handler it registers on the
SDK. That handler is stored on the room's own RPC server — `RoomInner` owns the
`livekit::Room`, which owns the `RoomSession`, which owns the handler map — so the
capture closed a cycle back onto the object that transitively stores it.

Nothing unregisters the method during teardown. `RoomSession::close` fails pending
client RPCs but never touches `rpc_server.handlers`, and `FfiRoom::close` does not
either, so the cycle survives both. `FfiServer::dispose` only clears the handle map,
which drops one `Arc` out of a self-sustaining cycle and releases nothing: the room,
the engine, its peer connections and the WebRTC runtime stay resident for the rest of
the process. That defeats what `dispose()` is documented to do, and repeated
connect/dispose cycles leak a full room each time. Only an explicit
`UnregisterRpcMethod` per method broke it.

Capture a `Weak` and upgrade per invocation, as AGENTS.md requires for a callback
stored by the object it captures. An invocation that arrives once the room is gone now
fails cleanly instead of resurrecting it.

A weak capture alone only breaks the idle cycle, though. Each accepted invocation
upgrades it to a strong `Arc<RoomInner>`, stores its responder in the room and parks on
the matching receiver with no timeout. Disposal removes the client's handles, so the
response can never arrive and the handler would stay pending forever, keeping the room
alive exactly as the idle cycle did. `FfiRoom::close` now drains
`rpc_method_invocation_waiters` and fails each one, after the room is closed so the SDK
is no longer delivering invocations that could repopulate the map.

Adds a `__lk-e2e-test` feature mirroring the one in `livekit`, and two tests under it.
The first registers a method, disposes, and asserts the room is released. The second
has a second participant invoke that method, waits for the invocation to reach the
handler, never answers it, then disposes and asserts the same. Each fails without its
respective change. `serial_test` is added because `FFI_SERVER` is a process-wide
singleton that both tests configure and dispose.

The drain also marks the room closed under the same lock, and
`store_rpc_method_invocation_waiter` refuses entries once that is set. Incoming RPCs run on
detached tasks that `RoomSession::close` does not join, so one can reach the handler and
upgrade the room after the drain has run; a waiter stored then could never be answered,
because the client's handles are gone by that point, and the handler would hold the room
open indefinitely. Such an invocation now fails immediately instead.
@LautaroPetaccio
LautaroPetaccio force-pushed the fix/ffi-rpc-handler-weak-room branch from 8cad232 to ee2cecf Compare September 18, 2026 16:21
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.

1 participant