fix(native): surface terminal auth connect errors#1649
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds terminal authentication error handling by introducing a ConnectError enum (Unauthorized, Forbidden) with status-code mapping, wiring it into moq-native, classifying transport-layer failures (noq, quinn, quiche, websocket), centralizing QUIC-vs-WebSocket racing to short-circuit on auth failures, making reconnect loops terminate on auth errors, and exposing Unauthorized/Forbidden on FFI surfaces (libmoq/moq-ffi) with distinct return codes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
rs/moq-native/tests/broadcast.rs (1)
786-857: ⚡ Quick winAdd a fallback-path auth regression test.
Both new auth tests dial
ws://, so they only exercise the pure WebSocket connector. The regression called out in this PR was losingUnauthorizedonce another transport also failed, and these cases won't catch that. Please add a sibling case that hitshttp://localhost:{port}(with only the WebSocket TCP listener bound) and assertsclient.connect(...)/client.reconnect(...)still surfaceConnectError::Unauthorizedwhile QUIC fails in parallel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-native/tests/broadcast.rs` around lines 786 - 857, Duplicate each failing-websocket test as a sibling that uses an http:// URL to exercise the fallback path: add two new tests (e.g., websocket_unauthorized_handshake_is_explicit_http_fallback and reconnect_stops_on_websocket_unauthorized_http_fallback) that copy the existing test bodies but parse url::Url from format!("http://{addr}") instead of "ws://{addr}", call client.connect(url) / client.reconnect(url) respectively, and assert the same expect_connect_err/ assert_connect_error(moq_native::ConnectError::Unauthorized) results; keep the same TCP listener that only serves an unauthorized HTTP response so QUIC (other transports) will fail in parallel and the fallback path is exercised.rs/moq-native/src/connect.rs (1)
26-29: ⚡ Quick winCall
ConnectError::is_auth()when classifying terminal auth failures.
downcast_ref::<ConnectError>().is_some()makes every futureConnectErrorvariant terminal because the enum is already#[non_exhaustive]. The first non-auth variant added here would silently stop reconnect retries.Suggested change
pub(crate) fn is_auth_error(err: &anyhow::Error) -> bool { - err.downcast_ref::<ConnectError>().is_some() + err.downcast_ref::<ConnectError>() + .is_some_and(ConnectError::is_auth) || matches!(err.downcast_ref::<moq_net::Error>(), Some(moq_net::Error::Unauthorized)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-native/src/connect.rs` around lines 26 - 29, The current is_auth_error function treats any ConnectError as terminal by using downcast_ref::<ConnectError>().is_some(); change it to downcast_ref::<ConnectError>().map_or(false, |e| e.is_auth()) so only auth variants are considered terminal, and keep the existing check for moq_net::Error::Unauthorized (matches!(err.downcast_ref::<moq_net::Error>(), Some(moq_net::Error::Unauthorized))). Update the function to call ConnectError::is_auth() on the downcasted value instead of just checking presence.rs/moq-ffi/src/session.rs (1)
54-69: ⚡ Quick winAdd test coverage for the
moq_net::Error::Unauthorizedmapping.The test suite covers
moq_native::ConnectErrormappings but does not test themoq_net::Error::Unauthorizedpath handled at lines 47-49. For parity withrs/libmoq/src/session.rs(which includes this test case in context snippet 2), add:assert!(matches!( map_connect_error(moq_net::Error::Unauthorized.into()), MoqError::Unauthorized ));This ensures both FFI layers maintain consistent test coverage for all auth error paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-ffi/src/session.rs` around lines 54 - 69, Add a test assertion to cover the moq_net authentication error mapping: in the tests module in session.rs (inside the maps_native_auth_connect_errors test), call map_connect_error with moq_net::Error::Unauthorized converted into the unified error (moq_net::Error::Unauthorized.into()) and assert it matches MoqError::Unauthorized; place this alongside the existing moq_native assertions so both FFI layers (map_connect_error, MoqError) have parity in coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-native/src/client.rs`:
- Around line 659-666: In the async test function
race_transport_connect_stops_on_quic_auth_error, call tokio::time::pause() at
the start of the test body (before any tokio::time::sleep is awaited) so the
sleep-based branch uses simulated time instead of real wall-clock time; locate
the test by the function name race_transport_connect_stops_on_quic_auth_error
and insert the single call to tokio::time::pause() as the first statement in the
async block to eliminate real-time delays in CI.
---
Nitpick comments:
In `@rs/moq-ffi/src/session.rs`:
- Around line 54-69: Add a test assertion to cover the moq_net authentication
error mapping: in the tests module in session.rs (inside the
maps_native_auth_connect_errors test), call map_connect_error with
moq_net::Error::Unauthorized converted into the unified error
(moq_net::Error::Unauthorized.into()) and assert it matches
MoqError::Unauthorized; place this alongside the existing moq_native assertions
so both FFI layers (map_connect_error, MoqError) have parity in coverage.
In `@rs/moq-native/src/connect.rs`:
- Around line 26-29: The current is_auth_error function treats any ConnectError
as terminal by using downcast_ref::<ConnectError>().is_some(); change it to
downcast_ref::<ConnectError>().map_or(false, |e| e.is_auth()) so only auth
variants are considered terminal, and keep the existing check for
moq_net::Error::Unauthorized (matches!(err.downcast_ref::<moq_net::Error>(),
Some(moq_net::Error::Unauthorized))). Update the function to call
ConnectError::is_auth() on the downcasted value instead of just checking
presence.
In `@rs/moq-native/tests/broadcast.rs`:
- Around line 786-857: Duplicate each failing-websocket test as a sibling that
uses an http:// URL to exercise the fallback path: add two new tests (e.g.,
websocket_unauthorized_handshake_is_explicit_http_fallback and
reconnect_stops_on_websocket_unauthorized_http_fallback) that copy the existing
test bodies but parse url::Url from format!("http://{addr}") instead of
"ws://{addr}", call client.connect(url) / client.reconnect(url) respectively,
and assert the same expect_connect_err/
assert_connect_error(moq_native::ConnectError::Unauthorized) results; keep the
same TCP listener that only serves an unauthorized HTTP response so QUIC (other
transports) will fail in parallel and the fallback path is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 738c30cd-df3f-4036-b948-6d86ec568081
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
rs/libmoq/src/error.rsrs/libmoq/src/session.rsrs/moq-ffi/Cargo.tomlrs/moq-ffi/src/error.rsrs/moq-ffi/src/session.rsrs/moq-native/Cargo.tomlrs/moq-native/src/client.rsrs/moq-native/src/connect.rsrs/moq-native/src/lib.rsrs/moq-native/src/noq.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rsrs/moq-native/src/reconnect.rsrs/moq-native/src/websocket.rsrs/moq-native/tests/broadcast.rs
Preserve unauthorized and forbidden connection failures as typed errors through native reconnect, libmoq, and UniFFI so clients can distinguish auth failures from retryable transport failures.
79a261b to
ecbf13e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/libmoq/src/session.rs`:
- Around line 113-118: The map_connect_error function currently maps non-auth
connect failures to Error::Native(err); change it so only
moq_native::ConnectError::Unauthorized and ::Forbidden are special-cased and all
other connect errors fall back to the generic connect error variant (i.e.,
return the same Error::Connect-style variant used by the FFI surface) rather
than Error::Native; update the match in map_connect_error accordingly so
behavior matches rs/moq-ffi/src/session.rs:34-39 and only 401/403 are handled
specially.
In `@rs/moq-native/src/error.rs`:
- Around line 77-90: The connect_error() matcher currently ignores the
TransportRace variant, causing nested Unauthorized/Forbidden to be lost; update
connect_error() (in the Error impl) to handle the TransportRace branch by
checking both children (the TransportRace left/right fields) and returning the
first Some(crate::ConnectError) found (e.g., call connect_error() on each child
and propagate a non-None result), or alternatively ensure the TransportRace
construction never wraps auth failures — but the simpler fix is to add a
TransportRace arm that recurses into both children and returns any propagated
connect error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8b47cf4-13dc-4d90-a1b6-0a3301928a76
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
rs/libmoq/src/error.rsrs/libmoq/src/session.rsrs/moq-ffi/src/error.rsrs/moq-ffi/src/session.rsrs/moq-native/Cargo.tomlrs/moq-native/src/client.rsrs/moq-native/src/connect.rsrs/moq-native/src/error.rsrs/moq-native/src/lib.rsrs/moq-native/src/noq.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rsrs/moq-native/src/reconnect.rsrs/moq-native/src/websocket.rsrs/moq-native/tests/broadcast.rs
✅ Files skipped from review due to trivial changes (2)
- rs/moq-native/src/connect.rs
- rs/moq-ffi/src/error.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- rs/moq-native/Cargo.toml
- rs/moq-native/tests/broadcast.rs
- rs/moq-native/src/websocket.rs
- rs/libmoq/src/error.rs
- rs/moq-native/src/client.rs
- rs/moq-native/src/noq.rs
Keep libmoq connect failures on the generic connect error path while still mapping 401 and 403 explicitly. Propagate nested connect errors out of TransportRace so auth failures are not lost when both transports fail.
Appending the new auth codes instead of inserting them. Native stays -32 so existing C consumers keyed on that value don't silently misread. Unauthorized and Forbidden take the next free codes (-33, -34). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note, this disable automatic retries on auth failures, which I guess is the right thing to do? |
I would say so? Given the token doesn't change I wouldn't expect the relay to somehow start accepting it. |
2 participants
Preserve unauthorized and forbidden connection failures as typed errors through native reconnect, libmoq, and UniFFI so clients can distinguish auth failures from retryable transport failures.
Previously an auth error would not be recognized since the client would fallback to connecting to e.g. websocket. If both transports failed we were left with generic error that completely stripped the unauthorized information.