Skip to content

Conversation

@davidzhao
Copy link
Member

when server sends an explicit LeaveRequest, we would still end up logging

livekit::rtc_engine:453:livekit::rtc_engine - received session close: "signal client closed: \"stream closed\"" UnknownReason Resume

that's due to the signal client being closed after an explicit Leave is sent

when server sends an explicit LeaveRequest, we would still end up logging

```
livekit::rtc_engine:453:livekit::rtc_engine - received session close: "signal client closed: \"stream closed\"" UnknownReason Resume
```

that's due to the signal client being closed after an explicit Leave is sent
Copy link
Contributor

@ladvoc ladvoc left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@ladvoc
Copy link
Contributor

ladvoc commented Oct 6, 2025

Actually, I just realized this change might have broken SimulateScenario::SignalReconnect—it looks like the E2E DC reliability test is hanging now immediately after the signal reconnect is requested.

@davidzhao
Copy link
Member Author

oh that's not good. how does the test tear down signal connection?

@ladvoc
Copy link
Contributor

ladvoc commented Oct 6, 2025

The test calls room.simulate_scenario(SimulateScenario::SignalReconnect).await on both the sending and receiving rooms (after a short delay) to ensure all packets are received despite the connection lapse. With this change, it appears the connection is never reestablished.

@ladvoc ladvoc self-requested a review October 6, 2025 01:06
match action {
proto::leave_request::Action::Resume
| proto::leave_request::Action::Reconnect => {
let running_handle = self.running_handle.read();
Copy link
Contributor

@ladvoc ladvoc Oct 6, 2025

Choose a reason for hiding this comment

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

It appears reading from the RwLock on this line leads to a deadlock—I will have to dig deeper to determine why this is the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see the issue, the read lock wasn't released in time. fixed in 701962c

Copy link
Contributor

@ladvoc ladvoc left a comment

Choose a reason for hiding this comment

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

See comment

@davidzhao davidzhao merged commit 5bce43f into main Oct 6, 2025
19 checks passed
@davidzhao davidzhao deleted the avoid-loggin branch October 6, 2025 06:08
@github-actions github-actions bot mentioned this pull request Oct 5, 2025
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.

3 participants