Skip to content

Conversation

@jhiatt-verkada
Copy link
Contributor

  • Ensure default delay applies when constructing config via ClientConfig::default()
  • Respect TLS configuration parameters

serde_with = "3"
time = "0.3"
tokio = { workspace = true, features = ["full"] }
tokio-tungstenite = { version = "0.24", features = ["rustls-tls-webpki-roots"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would probably be better if we could continue to use this crate via web-transport's re-export, so we can go that route if preferred. We need to enable this feature to enable rustls support. The tokio-tungstenite crate only exposes that via two higher-level features, either rustls-tls-native-roots or rustls-tls-webpki-roots, so you have to pick one or the other. I chose this one because the webpki crates are already in the dependency tree elsewhere so I thought that was the lesser of two evils.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we can set feature flags when using re-exports unfortunately. You'd have to add these features to web_transport_ws and proxy them through that way, which is probably better because then you won't run into versioning issues.

@jhiatt-verkada jhiatt-verkada force-pushed the fix-websocket-fallback branch 2 times, most recently from 3ced7b9 to 1917075 Compare January 14, 2026 01:28
The clap::arg default value only applies when parsing args.
This version re-exports the tokio-tungstenite traits needed to modify
TLS behaviors.
Previously it did not respect flags controlling TLS behavior.
@jhiatt-verkada jhiatt-verkada marked this pull request as ready for review January 14, 2026 01:31
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 14, 2026

Walkthrough

The pull request updates the web-transport-ws dependency from version 0.2 to 0.2.2 across the workspace and adds the rustls-tls-webpki-roots feature flag to the dependency in the moq-native crate. The ClientWebSocket struct changes its Default trait handling, replacing the derived implementation with an explicit one that sets a default delay of 200 milliseconds. WebSocket connection logic is refactored to conditionally handle TLS based on URL schemes: HTTP maps to unencrypted WebSocket, HTTPS/MOQL/MOQT maps to encrypted WebSocket, while WS/WSS schemes are preserved. Connection calls transition from connect_async_with_config to connect_async_tls_with_config with optional TLS connector parameters.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix bugs with WebSocket fallback' directly corresponds to the main changes: adding proper default delay handling and respecting TLS configuration for WebSocket fallback paths.
Description check ✅ Passed The description accurately reflects the two key objectives: ensuring default delay applies in ClientConfig::default() and respecting TLS configuration parameters, both of which are evident in the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3aca608 and 84fb6b3.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • Cargo.toml
  • rs/moq-native/Cargo.toml
  • rs/moq-native/src/client.rs
🧰 Additional context used
📓 Path-based instructions (1)
rs/**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

rs/**/*.rs: Use cargo for Rust-specific operations
Rust tests should be integrated within source files rather than in separate test files

Files:

  • rs/moq-native/src/client.rs
🧠 Learnings (2)
📚 Learning: 2025-12-20T13:14:11.182Z
Learnt from: Frando
Repo: moq-dev/moq PR: 794
File: rs/moq-native/src/iroh/client.rs:40-42
Timestamp: 2025-12-20T13:14:11.182Z
Learning: In rs/moq-native/src/iroh/server.rs, the Server::close method uses `&mut self` instead of `&self` (unlike Client::close) because the Server struct contains `FuturesUnordered<BoxFuture<...>>` which is Send but not Sync, and using `&self` would require Self: Sync. This is documented in the code and is an acceptable trade-off.

Applied to files:

  • rs/moq-native/src/client.rs
📚 Learning: 2026-01-11T03:47:52.766Z
Learnt from: CR
Repo: moq-dev/moq PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T03:47:52.766Z
Learning: Applies to rs/**/*.rs : Use `cargo` for Rust-specific operations

Applied to files:

  • rs/moq-native/src/client.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Check
🔇 Additional comments (5)
rs/moq-native/Cargo.toml (1)

53-53: Feature addition aligns with PR objectives.

The rustls-tls-webpki-roots feature enables the rustls-based TLS connector needed for the WebSocket fallback path. This is required to respect the tls-disable-verify flag.

Note from past discussion: proxying these features through web_transport_ws would avoid potential version conflicts. This approach works for now, but consider upstreaming feature proxies if versioning issues arise.

Cargo.toml (1)

27-27: LGTM!

Patch version bump to 0.2.2 provides the re-exported tokio-tungstenite traits needed for the TLS connector functionality in the WebSocket fallback path.

rs/moq-native/src/client.rs (3)

43-66: Correct fix for programmatic default.

The derived Default would set delay to None, while the clap default_value = "200ms" only applies during CLI argument parsing. This explicit implementation ensures consistency: both ClientConfig::default() and CLI usage get the same 200ms delay.


354-366: URL scheme handling is correct.

The mapping logic properly handles:

  • httpws:// (unencrypted)
  • https/moql/moqtwss:// (encrypted)
  • ws/wss preserved as-is

This aligns with the QUIC path's scheme handling while correctly determining TLS requirements.


370-390: TLS configuration correctly respected for WebSocket fallback.

The WebSocket path properly uses self.tls, which respects the --tls-disable-verify flag configured during client initialization (set via the NoCertificateVerification verifier when the flag is enabled). The Arc<rustls::ClientConfig> type passed to tokio_tungstenite::Connector::Rustls is correct and ensures consistent TLS behavior between QUIC and WebSocket transports.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kixelated kixelated merged commit 80cb5e6 into moq-dev:main Jan 14, 2026
1 check passed
@moq-bot moq-bot bot mentioned this pull request Jan 13, 2026
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.

2 participants