Bugfix: Server-initiated room disconnect shutdown fix#205
Bugfix: Server-initiated room disconnect shutdown fix#205alan-george-lk wants to merge 10 commits into
Conversation
alan-george-lk
changed the title
Co-authored-by: Cursor <cursoragent@cursor.com>
alan-george-lk
requested review from
stephen-derosa and
xianshijing-lk
as code owners
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
lgtm, assuming you address those comments.
| const bool has_room_state = connection_state_ != ConnectionState::Disconnected || listener_id_ != 0 || | ||
| room_handle_ || local_participant_ || !remote_participants_.empty(); | ||
| if (teardown_started_ || !has_room_state) { | ||
| return false; |
There was a problem hiding this comment.
should we return true if nothing to do or it is being torndown ?
There was a problem hiding this comment.
I'm open to changing this, right now our public API for disconnect() states:
@returns trueif the graceful disconnect succeeds;falseif the room was already disconnected (no-op) or the graceful disconnect fails.
Which this matches, false == noop. I get how that could be confusing, but I think the idea is true is when the disconnect actually happened
There was a problem hiding this comment.
That is fair. We can keep the current behavior to avoid breaking changes.
…gfix-room-shutdown
Align the shared cleanup helper with LocalParticipant/FfiClient naming, keep disconnect() returning false for no-ops per its documented contract, and cover Room reuse after server-initiated shutdown. Co-authored-by: Cursor <cursoragent@cursor.com>
alan-george-lk
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (connection_state_ != ConnectionState::Disconnected) { | ||
| throw std::runtime_error("already connected"); | ||
| } | ||
| shutdown_started_ = false; |
There was a problem hiding this comment.
🔴 Room becomes permanently stuck after concurrent disconnect during connect, leaking FFI handles
The shutdown latch is reset only at the start of the connection attempt (shutdown_started_ = false at src/room.cpp:105) but is never re-checked or re-cleared when the connection succeeds and state is published atomically at line 174–181, so a concurrent disconnect() that sets the latch mid-connect leaves the room in an unrecoverable state where it appears connected but can never be torn down.
Impact: FFI resource handles (room, participants) are permanently leaked because the destructor's disconnect() call is also a no-op.
Race window and mechanism
The race window is between line 107 (lock release after setting Reconnecting) and line 174 (lock acquire to publish final state). During this window, connect() performs network I/O (connectAsync().get()) which can take hundreds of milliseconds.
If another thread calls disconnect() during this window:
shutdown()acquireslock_, seesshutdown_started_ == falseandconnection_state_ == Reconnecting(has_room_state is true)- Sets
shutdown_started_ = true, captureslistener_to_remove, setsconnection_state_ = Disconnected - Removes the listener via
FfiClient::instance().removeListener() - Returns true
Then connect() continues:
5. FFI connect succeeds
6. At line 174, acquires lock and publishes connection_state_ = Connected, room_handle_, local_participant_, etc.
7. Does NOT reset shutdown_started_ (still true)
8. Calls readyForRoomEvent() — but listener is gone, so events are lost
Now the room is stuck:
connection_state_ == Connected,room_handle_is set,local_participant_is setshutdown_started_ == true- No listener registered
- Any call to
disconnect()→shutdown()seesshutdown_started_ == trueand returns false immediately ~Room()callsdisconnect()which also returns false- FFI handles are never released
In the old code (before this PR), disconnect() checked connection_state_ == Disconnected to bail out. After connect() overwrote it with Connected, a subsequent disconnect() would see Connected and proceed normally, cleaning up handles. The introduction of shutdown_started_ as a one-way latch creates this new failure mode.
Prompt for agents
The bug is that shutdown_started_ is reset only at line 105 (beginning of connect()) but not when connect() successfully publishes state at lines 174-181. If disconnect()/shutdown() runs between these two points and sets shutdown_started_ = true, connect() overwrites connection_state_ with Connected but leaves shutdown_started_ = true, making the room permanently stuck.
Possible fixes:
1. In the atomic state publish block at line 174-181, also reset shutdown_started_ = false. This ensures that a successful connect always leaves the room in a clean state.
2. Alternatively, check shutdown_started_ at line 174 before publishing state, and if it's true, abort the connect (treat it as a failure, clean up the new state, return false).
3. Option 2 is arguably better because if disconnect() was called during connect(), the user's intent was to disconnect, so connect() should fail rather than silently succeed in a broken state.
The relevant code is in Room::connect() in src/room.cpp. The state publish block is around lines 174-181 (the 'Publish all state atomically under lock' section).
Was this helpful? React with 👍 or 👎 to provide feedback.
|
lgtm, thanks |
| case proto::RoomEvent::kDisconnected: { | ||
| // If disconnect() was driven from our side, it already flipped state | ||
| // to Disconnected and fired the delegate; skip the duplicate here. | ||
| bool already_disconnected = false; | ||
| { | ||
| const std::scoped_lock<std::mutex> guard(lock_); | ||
| already_disconnected = (connection_state_ == ConnectionState::Disconnected); | ||
| connection_state_ = ConnectionState::Disconnected; | ||
| } | ||
| if (already_disconnected) { | ||
| break; | ||
| } | ||
| DisconnectedEvent ev; | ||
| ev.reason = toDisconnectReason(re.disconnected().reason()); | ||
| if (delegate_snapshot) { | ||
| delegate_snapshot->onDisconnected(*this, ev); | ||
| } | ||
| const auto reason = toDisconnectReason(re.disconnected().reason()); | ||
| (void)shutdown(false, reason, true); | ||
| break; |
There was a problem hiding this comment.
🔴 Server-initiated disconnect silently suppresses the end-of-stream callback to the application
The listener is removed during server-disconnect cleanup (shutdown(false, reason, true) at src/room.cpp:1199) before the Rust FFI sends the follow-up end-of-stream event, so the onRoomEos delegate callback is never delivered.
Impact: Applications relying on the end-of-stream callback for final cleanup after a server-initiated disconnect will silently miss it.
Mechanism: kDisconnected removes the listener before kEos arrives
In the old code, kDisconnected (at the old src/room.cpp:1170–1186) only set connection_state_ to Disconnected and fired onDisconnected. The FFI listener remained registered, so the subsequent kEos event from Rust would still be dispatched to Room::onEvent(), which performed full state cleanup and called delegate_snapshot->onRoomEos().
In the new code, kDisconnected calls shutdown(false, reason, true) which:
- Moves out all room state (
src/room.cpp:239–248) - Removes the FFI listener via
FfiClient::instance().removeListener()(src/room.cpp:290) - Fires
onDisconnected(src/room.cpp:311)
Because the listener is now removed, when Rust later sends the kEos event, FfiClient::pushEvent finds no matching listener slot for this Room. The kEos case at src/room.cpp:1223–1230 is never reached, and onRoomEos is never called.
The old event sequence for server disconnect was: kDisconnected → onDisconnected → kEos → cleanup + onRoomEos.
The new sequence is: kDisconnected → cleanup + onDisconnected → (kEos silently dropped).
Prompt for agents
The kDisconnected handler at src/room.cpp:1197-1200 calls shutdown() which removes the FFI listener. This prevents the subsequent kEos event from Rust from being dispatched to this Room, so onRoomEos is never called after a server-initiated disconnect.
Possible approaches:
1. Do not remove the FFI listener during shutdown when called from kDisconnected. Instead, let kEos arrive and do the listener removal there (similar to the old code's split between kDisconnected and kEos).
2. Keep the current shutdown() behavior but have the kDisconnected handler explicitly fire onRoomEos after shutdown() returns, since the delegate_snapshot is still valid.
3. Split shutdown into two phases: one that does state cleanup without removing the listener (for kDisconnected), and one that also removes the listener (for kEos and explicit disconnect).
The key constraint is that the Rust FFI sends kDisconnected followed by kEos, and both should result in their respective delegate callbacks being fired to maintain backward compatibility with the public RoomDelegate API.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const std::string delete_command = std::string("lk --dev room delete --yes ") + room_name; | ||
| ASSERT_EQ(std::system(delete_command.c_str()), 0) << "failed to delete local test room"; |
There was a problem hiding this comment.
🟨 Unsanitized environment variable concatenated into shell command
The integration test constructs a shell command by directly concatenating the LIVEKIT_ROOM environment variable into a std::system() call (src/tests/integration/test_room.cpp:287-288). If the environment variable contained shell metacharacters (e.g., ; rm -rf /), arbitrary commands could execute. In practice, this variable is set by the test helper script to a hardcoded safe value, and this code only runs in test/CI environments.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
shutdown()helper, which by proxy cleans up event codeBefore fix, when server deleted the room:
Above observed on Mac, but crashed on Linux (user-reported). No longer after the fix.
Testing
lk --dev room deleteemitFfiEventinto standalone header helperffi_utils.hfor use across two tests now