feat(rtc_engine): report the reconnect reason to the server on resume - #1338
feat(rtc_engine): report the reconnect reason to the server on resume#1338xianshijing-lk wants to merge 2 commits into
Conversation
Resumes sent no reason at all, so the server could not attribute why Rust
clients reconnect. `SignalInner::restart` passed `None` for the parameter, and
the v0 signalling path had no `reconnect_reason` query parameter to pass it to in
the first place -- only the v1 JoinRequest protobuf carried one. The server reads
this to classify reconnects (`rtcservice.go`: `r.FormValue("reconnect_reason")`),
and client-sdk-js has always sent it, so Rust sessions were simply missing from
that breakdown.
Record the cause where it is known -- signal link closed unexpectedly,
publisher PeerConnection failed, subscriber PeerConnection failed -- carry it on
the engine handle for the episode, and report it on every resume attempt of that
episode. Later failures within an episode do not overwrite it, since they are
consequences of the first.
Kept deliberately separate from the existing `reconnect_reason: DisconnectReason`
on the handle: that one is the reason the engine *closes* with if recovery fails,
a different enum for a different purpose.
A server-initiated `Leave` reports `RR_UNKNOWN` -- the server initiated it and
already knows why. The `ForceTcp`/`ForceTls` simulate scenarios synthesize a local
`Leave{Reconnect}` and so also report `RR_UNKNOWN` rather than
`RR_SWITCH_CANDIDATE`; threading a reason through that path is not worth the
plumbing for a test-only scenario.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Changeset ✓This PR includes a changeset covering all affected packages:
|
The parameter is already an `i32` (the caller casts the enum), so `to_string` yields the numeric value the server's `strconv.Atoi` expects -- but the previous assertion restated that as `(reason as i32).to_string()` rather than asserting it, so it could not distinguish the number from the enum name. Pin "3" literally on both paths, and rename the binding at the point of use so its type is obvious without following the signature. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
It's the numeric value stringified — The reason it's already correct: Worth noting it couldn't have produced the name even by accident: prost doesn't implement Three changes so this isn't ambiguous to the next reader:
// The server parses this with `strconv.Atoi`, so it must be the enum's numeric value
// stringified ("3"), never its name ("RR_SUBSCRIBER_FAILED").
assert_eq!(reason as i32, 3);
assert!(v0_params.contains(&("reconnect_reason".to_string(), "3".to_string())), ...);That last one is the substantive fix. The previous assertion compared against |
| pub async fn restart( | ||
| &self, | ||
| reason: proto::ReconnectReason, | ||
| ) -> SignalResult<proto::ReconnectResponse> { |
There was a problem hiding this comment.
🟡 A published library function changes its call signature without being flagged as a breaking change
A function that outside code can call is given an extra required argument (SignalClient::restart at livekit-api/src/signal_client/mod.rs:307-310) while the change is described as having no public-facing impact, so anyone already calling it will find their code no longer works after upgrading.
Impact: External users of the API crate hit an unexpected build break on a patch release.
Public reachability of the changed signature and the repository rule it touches
livekit-api/src/lib.rs:27 exposes pub mod signal_client (behind the signal-client feature), and SignalClient/restart are both pub, so restart() is part of livekit-api's public API surface — not pub(crate) as the PR description states. AGENTS.md requires that breaking public API changes be avoided unless necessary and that the author be explicit about them; the changeset (.changeset/report_reconnect_reason_on_resume.md) marks livekit-api as a patch. Either keep the old zero-argument entry point (delegating with ReconnectReason::RrUnknown) and add a reason-taking variant, or state the break explicitly and bump accordingly.
Prompt for agents
livekit_api::signal_client::SignalClient::restart is part of livekit-api's public API (livekit-api/src/lib.rs:27 exports `pub mod signal_client`), yet this PR adds a required `reason: proto::ReconnectReason` parameter and the PR/changeset describe the release as a patch with no public API change. Per AGENTS.md, breaking public API changes should be avoided unless necessary and must be called out explicitly. Consider preserving a backwards-compatible entry point (e.g. keep `restart()` delegating with `ReconnectReason::RrUnknown` and add `restart_with_reason(reason)`), or explicitly document the break and adjust the changeset version bump.
Was this helpful? React with 👍 or 👎 to provide feedback.
Before you submit your PR
PR description
A resume never told the server why it was reconnecting, so Rust sessions are missing from the server-side reconnect-reason breakdown — every resume looks like
RR_UNKNOWN.There were two independent gaps:
SignalInner::restart()passedNonefor thereconnect_reasonparameter, even though the parameter, theReconnectReasonproto enum, and the plumbing all already existed.reconnect=1andsid; the reason is carried inside the JoinRequest protobuf on the v1 path, so even a supplied reason would have been dropped for every v0 client.The server reads this to classify reconnects (
rtcservice.go:reconnectReason, _ := strconv.Atoi(r.FormValue("reconnect_reason"))), and client-sdk-js has always sent it (SignalClient.ts:1219).What it now reports
The cause is recorded where it is actually known, and carried on the engine handle for the whole reconnect episode:
RR_SIGNAL_DISCONNECTEDRR_PUBLISHER_FAILEDRR_SUBSCRIBER_FAILEDLeaveRR_UNKNOWN— the server initiated it and already knows whyLater failures inside an episode do not overwrite the first, since they are consequences of it.
This is kept deliberately separate from the existing
reconnect_reason: DisconnectReasonon the handle. That field is the reason the engine closes with if recovery fails — a different enum for a different purpose — so the new one is namedreported_reconnect_reasonto make the destination unambiguous.Why this is worth having
It is the prerequisite for answering an operational question we currently cannot: how often do resumes fail and escalate to a full reconnect, and for which cause? That determines whether further work on resume recovery latency is justified at all. Right now the data does not exist for Rust clients.
Breaking changes
None to the public API. Three internal signatures gain a parameter (
SignalClient::restart,RtcSession::restart,EngineInner::reconnection_needed) andSessionEvent::Closegains a field; all arepub(crate)-facing or withinrtc_engine.Behaviourally the only change is an extra query parameter (v0) / populated protobuf field (v1) on resume.
RR_UNKNOWNis0, which is what the server already inferred from its absence, so a server that ignores the value is unaffected.MSRV
Unchanged.
Testing
cargo test -p livekit-api --no-default-features --features "signal-client-tokio,access-token" --lib— 44 passed.cargo test -p livekit --lib— 76 passed.Note
signal_clientis not inlivekit-api's default feature set, so its tests need that feature selection to run at all.Two tests, both on
get_livekit_url, which is where the value either reaches the wire or does not:resume_reports_the_reconnect_reason_on_both_signalling_paths— asserts a v0 resume carriesreconnect_reasonas a query parameter and a v1 resume carries it inside the decoded JoinRequest. Covering both matters because the two paths serialize it completely differently, and only one of them was ever wired up. I verified this fails against the unfixed v0 path, with the assertion printing the full parameter list so the omission is obvious.initial_connect_sends_no_reconnect_reason— an initial connect is not a reconnect and must not claim a cause.Not covered by unit tests: the mapping from each trigger to its enum value, which needs a live server to exercise (the triggers are a signal close and PeerConnection state transitions). The existing reconnect integration tests in
livekit/tests/cover those code paths; they just don't assert on the reported value.Async
No new runtime dependencies and no new tasks or timers. The reason is a
Copyenum read under the existingrunning_handlelock at the start of each resume attempt, so it adds no synchronization. The new tests are synchronous.