Skip to content

Commit 5b7306c

Browse files
authored
error early when not authenticated (#1149)
### Before you submit your PR Make sure the following is true before submitting your PR: - [ ] I have read the [contributing guidelines](https://github.com/livekit/rust-sdks/blob/main/CONTRIBUTING.md) and validated that this PR will be accepted. - [ ] I have read and followed the principles regarding breaking changes, testing, and code quality. ### PR description Describe the changes in this PR. Explain what the PR is meant to solve and how to reproduce the issue in the first place. ### Breaking changes If this PR introduces breaking changes, list them here and document the rationale for introducing such a change. ### MSRV If the PR modifies the crate's MSRV (Minimum Supported Rust Version), document it here. ### Testing Ideally, unit test the code you add, but ensure you're not repeating existing test cases. Use as many already written scaffolding, utilities as possible; write your own, when needed. If external services, APIs, tokens are required (e.g., running an LK server instance), provide the necessary information. Make sure your tests perform useful, context-aware assertions and do not simply emulate "happy paths". ### Async We want the project to be runtime-agnostic, so please reuse what's already in [livekit-runtime](https://github.com/livekit/rust-sdks/blob/main/livekit-runtime/) and feel free to add anything missing. It's ok to use Tokio directly, when writing unit tests, if necessary. When testing, do not use artificial delays for the state to "catch up"; instead, respect the event flow and subscribe properly using channels or other mechanisms.
1 parent 55ee902 commit 5b7306c

4 files changed

Lines changed: 103 additions & 8 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

livekit/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,3 +59,4 @@ anyhow = "1.0.99"
5959
test-log = "0.2.18"
6060
test-case = "3.3"
6161
serial_test = "3.0"
62+
http = "1.1"

livekit/src/rtc_engine/mod.rs

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,6 +503,14 @@ impl EngineInner {
503503
match try_connect().await {
504504
Ok(res) => return Ok(res),
505505
Err(e) => {
506+
// A validated auth failure (401/403) will not succeed on
507+
// retry with the same token — surface it immediately instead
508+
// of burning the remaining join attempts. Same classification
509+
// as the reconnect loop (see `auth_failure_reason`).
510+
if auth_failure_reason(&e).is_some() {
511+
log::warn!("authentication rejected during connect ({e}); not retrying");
512+
return Err(e);
513+
}
506514
let attempt_i = i + 1;
507515
if i < max_retries {
508516
log::warn!(
@@ -943,6 +951,16 @@ impl EngineInner {
943951
"server requested disconnect during restart".into(),
944952
));
945953
}
954+
if let Some(reason) = auth_failure_reason(&err) {
955+
log::warn!(
956+
"authentication rejected during restart ({err}); not retrying"
957+
);
958+
self.running_handle.write().can_reconnect = false;
959+
self.close(reason).await;
960+
return Err(EngineError::Connection(
961+
"authentication failed during reconnect".into(),
962+
));
963+
}
946964
log::error!("restarting connection failed: {}", err);
947965
}
948966
}
@@ -971,6 +989,16 @@ impl EngineInner {
971989
"server requested disconnect during resume".into(),
972990
));
973991
}
992+
if let Some(reason) = auth_failure_reason(&err) {
993+
log::warn!(
994+
"authentication rejected during resume ({err}); not retrying"
995+
);
996+
self.running_handle.write().can_reconnect = false;
997+
self.close(reason).await;
998+
return Err(EngineError::Connection(
999+
"authentication failed during reconnect".into(),
1000+
));
1001+
}
9741002
log::error!("resuming connection failed: {}", err);
9751003
let mut running_handle = self.running_handle.write();
9761004
running_handle.full_reconnect = true;
@@ -1153,6 +1181,28 @@ fn leave_disconnect_reason(err: &EngineError) -> Option<DisconnectReason> {
11531181
None
11541182
}
11551183

1184+
/// Inspect a reconnect-attempt error for a genuine authentication/authorization
1185+
/// failure (HTTP 401/403). Such a failure will not succeed on retry with the
1186+
/// same token, so the reconnect loop should bail out immediately rather than
1187+
/// burning every attempt (and hammering the server) with credentials it already
1188+
/// knows are rejected.
1189+
///
1190+
/// We key on `SignalError::Client(401|403)`, which is produced by the server's
1191+
/// `rtc/validate` probe (see [`super`]'s `SignalInner::validate`) — an
1192+
/// authoritative classification. We deliberately do NOT key on the raw
1193+
/// `WsError::Http` upgrade status, because that can be a fabricated 401 masking a
1194+
/// transient server error (e.g. a 503 from a saturated node), which IS
1195+
/// retryable. A resume that hits a raw 401 simply escalates to a full reconnect,
1196+
/// whose connect path runs `validate()` and surfaces the authoritative status.
1197+
fn auth_failure_reason(err: &EngineError) -> Option<DisconnectReason> {
1198+
if let EngineError::Signal(SignalError::Client(status, _)) = err {
1199+
if matches!(status.as_u16(), 401 | 403) {
1200+
return Some(DisconnectReason::JoinFailure);
1201+
}
1202+
}
1203+
None
1204+
}
1205+
11561206
#[cfg(test)]
11571207
mod tests {
11581208
use super::*;
@@ -1200,4 +1250,47 @@ mod tests {
12001250
);
12011251
}
12021252
}
1253+
1254+
#[test]
1255+
fn auth_failure_reason_flags_validated_401_and_403() {
1256+
// The server's rtc/validate probe surfaces auth failures as Client(4xx).
1257+
for status in [401u16, 403] {
1258+
let err = EngineError::Signal(SignalError::Client(
1259+
http::StatusCode::from_u16(status).unwrap(),
1260+
"invalid token".into(),
1261+
));
1262+
assert_eq!(
1263+
auth_failure_reason(&err),
1264+
Some(DisconnectReason::JoinFailure),
1265+
"Client({status}) must be treated as a non-retryable auth failure"
1266+
);
1267+
}
1268+
}
1269+
1270+
fn auth_failure_reason_ignores_other_client_and_server_errors() {
1271+
let not_auth = [
1272+
// Other client errors are not auth failures.
1273+
EngineError::Signal(SignalError::Client(http::StatusCode::NOT_FOUND, "".into())),
1274+
EngineError::Signal(SignalError::Client(
1275+
http::StatusCode::TOO_MANY_REQUESTS,
1276+
"".into(),
1277+
)),
1278+
// Server errors (e.g. a saturated node) are retryable.
1279+
EngineError::Signal(SignalError::Server(
1280+
http::StatusCode::SERVICE_UNAVAILABLE,
1281+
"".into(),
1282+
)),
1283+
// Generic connectivity/internal errors are retryable.
1284+
EngineError::Connection("network".into()),
1285+
EngineError::Internal("bug".into()),
1286+
EngineError::Signal(SignalError::SendError),
1287+
EngineError::Signal(SignalError::Timeout("waiting".into())),
1288+
];
1289+
for err in &not_auth {
1290+
assert!(
1291+
auth_failure_reason(err).is_none(),
1292+
"{err:?} must NOT be treated as an auth failure"
1293+
);
1294+
}
1295+
}
12031296
}

livekit/src/rtc_engine/reconnect_strategy.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,10 @@ mod tests {
7070
// Monotonic non-decreasing and never above the cap.
7171
let mut prev = Duration::ZERO;
7272
for attempt in 1..=RECONNECT_ATTEMPTS {
73-
let nominal = nominal(attempt);
74-
assert!(nominal >= prev, "backoff must not decrease (attempt {attempt})");
75-
assert!(nominal <= RECONNECT_MAX_DELAY, "backoff must not exceed the cap");
76-
prev = nominal;
73+
let nominal_duration = nominal(attempt);
74+
assert!(nominal_duration >= prev, "backoff must not decrease (attempt {attempt})");
75+
assert!(nominal_duration <= RECONNECT_MAX_DELAY, "backoff must not exceed the cap");
76+
prev = nominal_duration;
7777
}
7878

7979
// Late attempts are pinned to the cap, and large attempt indices don't
@@ -86,12 +86,12 @@ mod tests {
8686
fn backoff_delay_stays_within_nominal_jitter_window() {
8787
// Full jitter: every sample must land within [0, nominal(attempt)].
8888
for attempt in 1..=RECONNECT_ATTEMPTS {
89-
let nominal = nominal(attempt);
89+
let nominal_duration = nominal(attempt);
9090
for _ in 0..1000 {
91-
let delay = delay(attempt);
91+
let delay_duration = delay(attempt);
9292
assert!(
93-
delay <= nominal,
94-
"jittered delay {delay:?} exceeded nominal {nominal:?} (attempt {attempt})"
93+
delay_duration <= nominal_duration,
94+
"jittered delay {delay_duration:?} exceeded nominal {nominal_duration:?} (attempt {attempt})"
9595
);
9696
}
9797
}

0 commit comments

Comments
 (0)