Repository navigation
livekit-rpc: give a departed recipient's reply time to land - #1520
Merged
Merged
Conversation
Purging a call's pending ack and response the instant its recipient disconnects assumed that "participant left" implies "no reply is coming". It does not: the departure is announced on the signalling connection while the ack and response travel on the data channel, and nothing orders the two against each other. `lk.sip.EndCall` produces that order routinely, because the SIP participant leaves the room as soon as it has ended the call it was asked to end. The reply then arrived to find its entry already gone, was discarded with "Response received for unexpected RPC request", and the caller was handed RECIPIENT_DISCONNECTED for a call that had in fact succeeded. Callers that retry went on to address a participant no longer in the room and spent the full 7s ack timeout before failing with CONNECTION_TIMEOUT. A disconnect now shortens the wait instead of ending it. The pending entries stay registered and each affected caller is signalled over a per-leg oneshot; it then waits up to DISCONNECT_GRACE_PERIOD (2s), never past its own timeout, for a reply already on the wire. Per-leg signals mean a disconnect seen while waiting on the ack is still remembered at the response. A recipient that really did leave without replying still fails fast rather than running out the 15s response timeout, which is what the purge was added for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Changeset ✓This PR includes a changeset covering all affected packages:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Devin Review found 2 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
`DISCONNECT_GRACE_PERIOD_DURATION` says at the use site that the value is a `Duration`, matching how the other time-valued constants in this crate read. The two doc comments that cite it are reflowed to stay inside the 100 column limit; rustfmt does not reflow comments, so that has to be done by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… leg A call waits on its ack and then on its response, and both waits need to know that the recipient has left. Two separate `oneshot`s meant the disconnect had to be delivered twice and `Pending` had to carry an `Option` so each could be taken exactly once. One `watch` channel per call replaces them. The value latches, so a disconnect seen while waiting on the ack is still readable when the call moves on to the response — which is what the second `oneshot` existed to arrange — and signalling it more than once is harmless, so nothing has to be taken. `oneshot::Sender` is not `Clone`, hence the `Arc` around the sender: that is what the ack and response entries each hold a handle to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`forget` does not say what is being forgotten. `clear_in_flight_rpc` names the state it drops, which is what the call sites are actually doing when they give up on a call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The generic local `fn notify` existed only because the two maps hold different `Pending` payloads. Now that signalling a call is a single `send`, writing the scan out per map is shorter than the helper that abstracted it and reads without a jump. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eping What these two tests are about is that a reply which lands after the disconnect still resolves the caller. The gap between the two is not part of that claim: the `biased` select checks the reply first, so the reply wins whether or not the waiting task has observed the disconnect yet, and the length of the sleep could never change the outcome. Dropping the sleeps also drops the reason these tests ran on virtual time. Neither one ever waits out the grace period, so neither needs `start_paused`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`test_disconnect_grace_period_does_not_wait_out_response_timeout` set up the same call as `test_participant_disconnect_fails_pending_call` — ack, then disconnect — and asserted the same two things: `RECIPIENT_DISCONNECTED`, well before the response timeout could explain it. The surviving test already makes that claim against a 30s response timeout with a 5s bound, so the grace period cannot have turned into a second response timeout without failing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment explained a feature whose name already says what it is for, and it named only one of the tests that now rely on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
livekit-capture needs the patch bump alongside the other packages in this release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing was ever carried over the `watch` channel: `send(true)` existed only so that the caller could observe that something had happened. A closed `mpsc` channel says the same thing without a value to inspect, and says it permanently — `recv` keeps returning `None` once every sender is gone, which is the latching the response leg depends on. `mpsc::Sender` is `Clone`, so the ack and response entries hold a sender each rather than two handles to one `Arc`, and `handle_participant_disconnected` drops them instead of sending. Holding them in an `Option` is what lets a dropped sender coexist with a pending entry that deliberately stays registered for the grace period. Both entries must be dropped for the channel to close, which is why the scan covers each map. A call that stops being tracked for any other reason closes the channel too. That is harmless: the `biased` select reads the reply first, and every such path has either resolved the reply or already returned to the caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1egoman
reviewed
Oct 13, 2026
Comment on lines
161
to
+182
| // Register channels BEFORE sending the request to avoid race condition | ||
| // where the response arrives before we've registered the handlers | ||
| let (participant_disconnected_tx, mut participant_disconnected_rx) = mpsc::channel(1); | ||
| { | ||
| let mut pending_acks = self.pending_acks.lock(); | ||
| let mut pending_responses = self.pending_responses.lock(); | ||
| let destination = ParticipantIdentity(data.destination_identity.clone()); | ||
| pending_acks.insert( | ||
| id.clone(), | ||
| Pending { destination_identity: destination.clone(), tx: ack_tx }, | ||
| Pending { | ||
| destination_identity: destination.clone(), | ||
| tx: ack_tx, | ||
| participant_disconnected_tx: Some(participant_disconnected_tx.clone()), | ||
| }, | ||
| ); | ||
| pending_responses.insert( | ||
| id.clone(), | ||
| Pending { | ||
| destination_identity: destination, | ||
| tx: response_tx, | ||
| participant_disconnected_tx: Some(participant_disconnected_tx), | ||
| }, |
Contributor
There was a problem hiding this comment.
Note to reviewers: I'm not 100% sure this is the best way to handle this. I'm effectively using this mpsc::channel as a oneshot channel where I can trigger it from multiple locations (avalue in pending_acks, and a value in pending_responses). There may be a better way to do this though.
xianshijing-lk
commented
Oct 17, 2026
xianshijing-lk
left a comment
xianshijing-lk
left a comment
Contributor
Author
There was a problem hiding this comment.
should we land this ?
Contributor
|
@xianshijing-lk I'm waiting for an approving review first. I'll resurface it to the team again in slack. |
cloudwebrtc
approved these changes
Oct 19, 2026
Contributor
Author
|
Landed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a regression introduced by #1376: an RPC call whose reply arrives just after its recipient leaves the room is failed, even though the call succeeded.
#1376 added
RpcClientManager::handle_participant_disconnected, which removes a call's pending ack and response the moment its recipient disconnects. That was a real fix — before it, a recipient leaving mid-call stranded the caller for the full 15s response timeout and reportedResponseTimeoutinstead of the disconnect that actually ended the call. But it assumed "participant left" implies "no reply is coming", and that is not true: the departure is announced on the signalling connection (signal_response::Message::Update→SessionEvent::ParticipantUpdate), while the ack and response travel on the data channel. Nothing orders the two against each other.lk.sip.EndCallproduces the losing order routinely, because the SIP participant leaves the room as soon as it has ended the call it was just asked to end. The reply then arrives to find its entry already gone, is discarded withResponse received for unexpected RPC request, and the caller getsRECIPIENT_DISCONNECTED(1503) for a call that in fact completed. A caller that retries goes on to address a participant no longer in the room, nothing acks, and it spends the fullmax_round_trip_latency— 7s by default — before failing withCONNECTION_TIMEOUT.Reported impact
Surfaced by a customer upgrading
livekit-agents1.7.1 → 1.8.5, whose E2E suite went from a historical <1% failure rate onlk.sip.EndCallto 13 of ~44 calls, each costing ~11s of extra session time (2s before the retry + the 7s ack timeout). The version boundary matches this change exactly:rust-sdkssubmodulelivekit==1.1.182d9f01ab(Release #1327)livekit==1.1.205a656c4b(Release #1423)livekit==1.1.205a656c4bwhich is consistent with their report of 260 clean scenarios after downgrading to 1.8.3. (Note for anyone triaging the same report: the break is 1.8.4, not 1.8.5 — both pin the same
livekit-rtc.)The
EndCallitself always landed; nothing left calls up. The damage was a false error plus the wasted retry.Change
A disconnect now shortens the wait instead of ending it.
handle_participant_disconnectedno longer removes the pending entries — dropping them is what turns an in-flight reply into an "unexpected RPC request". It signals each affected caller over a per-legoneshot, and the caller then waits up toDISCONNECT_GRACE_PERIOD(2s), never past its own timeout, for a reply already on the wire. Per-leg signals mean a disconnect seen while waiting on the ack is still remembered once the call moves on to the response.A recipient that really did leave without replying still fails fast with
RECIPIENT_DISCONNECTEDrather than running out the 15s response timeout, so #1376's improvement is preserved.fail_all_pending(room close) still fails immediately — the data channel is gone, nothing more can arrive.Tests
livekit-rpc/src/tests.rs— three new tests on virtual time, driving the exact event ordering that caused the bug:test_response_after_disconnect_still_resolves_caller— the repro: ack, disconnect, then the response 50ms later (matching the ~45ms gap seen in the field). Fails onmainwithRpcError { code: 1503, message: "Recipient disconnected" }— the reported symptom — and passes here.test_ack_after_disconnect_still_resolves_caller— same race on the ack leg. Also fails onmain.test_disconnect_grace_period_does_not_wait_out_response_timeout— the grace period is a reprieve, not a second response timeout.Reverting only
client.rstomainand keeping the tests gives 25 passed / 2 failed; with the fix, 27 passed. The existingtest_participant_disconnect_fails_pending_callstill passes and now also asserts the call ends well inside its 30s response timeout.livekit/tests/rpc_test.rs—test_rpc_recipient_leaving_without_replying_fails_promptlycovers the same contract end-to-end against a live server: a handler that never replies, the recipient closes its room, and the caller must fail withRECIPIENT_DISCONNECTEDinside 15s despite a 60s response timeout. This guards the fast-fail behaviour from being reverted along with the race fix.Worth being explicit about the split: the race itself cannot be forced end-to-end — it is a sub-50ms window owned by two independent transports, and a client-side room close tears down the data channel, so the reply would be genuinely lost rather than merely late. The deterministic coverage therefore lives in the unit tests, at the seam where the event ordering is actually decided. The e2e test is compiled but not run locally (it needs a server); CI runs it under
__lk-e2e-test.Follow-up, not in this PR
perform_rpcto an identity that is not in the room does not fail fast: it sends into the void and waits the full 7s ack timeout before returningCONNECTION_TIMEOUT.RpcTransportalready exposesremote_identities(), so this could returnRECIPIENT_NOT_FOUND(1401) immediately. Left out because it changes an API-visible error code on an existing path.🤖 Generated with Claude Code