Refactor http and websocket implementations into its own crate#1229
Refactor http and websocket implementations into its own crate#1229jhugman wants to merge 7 commits into
Conversation
Introduce `livekit-net`: a self-contained crate that owns the signalling
transport seam.
- `PlatformTransport` / `PlatformConnection` async traits (host- or
Rust-implementable), value types (`Header`, `HttpResponse`,
`TransportError`, `PlatformConnectResult`), and a process-global
`set_transport`/`transport` registry.
- `foreign` feature exports the traits to Dart/Swift/Kotlin via
`#[uniffi::export(with_foreign)]` (uniffi 0.31, pure proc-macro).
`connect` returns a `PlatformConnectResult` record and takes
`timeout_ms: u64` — the shapes that actually cross uniffi-dart codegen.
- Native backend (`native-tokio`/`native-async`/`native-dispatcher`):
tungstenite + reqwest/isahc + proxy CONNECT + TLS + runtime pass-through,
relocated verbatim from livekit-api's signalling stack. WS-handshake HTTP
errors are preserved as `TransportError::Http { status }`.
Also make the workspace runtime flavor explicit: `livekit-runtime` is now
`default-features = false` at the workspace root, and each consumer
(libwebrtc, livekit, livekit-datatrack) selects `tokio` explicitly, so the
one-runtime guard is satisfied and non-tokio flavors can be selected per
crate.
Make `signal_client` blind to the transport backend: it resolves
`livekit_net::transport()` and drives the WebSocket via
`PlatformConnection`, and the two pre-WS HTTP GETs (validate, region
discovery) via `PlatformTransport::http_get`. Deletes the local
tungstenite/proxy/TLS stack and the `WsError` coupling.
- `signal_stream` runs its read/write tasks over `Arc<dyn PlatformConnection>`;
WS control frames (ping/pong, close, reset) are the transport's concern.
- `validate`/`fetch_from_endpoint` go through `http_get`; `TransportError`
maps to `SignalError` via `map_transport_err` (handshake HTTP status is
preserved, so the v1->v0 404 fallback and 403 handling still fire).
- Feature decomposition: a blind `signal-client` feature (livekit-net, no
backend) with thin `signal-client-{tokio,async,dispatcher}` wrappers that
add `livekit-net/native-*`; TLS delegates to livekit-net; signalling
WS/HTTP/TLS deps removed. `signal_client` has zero backend cfg branches.
- Tests: a shared, once-registered deterministic mock transport
(`test_transport.rs`) exercises the connect/validate/region paths.
Add a `foreign` feature that pairs the blind `livekit-api/signal-client` with `livekit-net/foreign`, letting a host-provided transport drive signalling. `livekit-net` becomes an optional dep (pulled only by `foreign`), so default (tokio) builds are unaffected. TLS feature comments updated to reflect that TLS now delegates through livekit-net.
Activate the foreign backend in the cdylib (livekit-net foreign + tokio, livekit-api blind signal-client) and export `set_platform_transport` so a Dart/Swift/Kotlin host can register its own `PlatformTransport` across the UniFFI boundary. Depend on livekit-api directly with default-features off to drop the services (reqwest) stack from the cdylib.
Run the new livekit-net / signal-client / livekit-uniffi test suites in CI (they need no dev server), and note in builds.yml that livekit-uniffi must stay on the tokio runtime while it shares --workspace with the desktop livekit build.
Replace the lossy "everything → Timeout" transport-error mapping with dedicated variants and future-proof the enum: - `TransportError::Connection`/`Other` → `SignalError::Connection(String)` - `TransportError::Closed` → `SignalError::Closed` - (`Timeout`/`Http` unchanged) `SignalError` is now `#[non_exhaustive]`, so future variant additions are non-breaking. Reconnect behavior is unchanged: the engine escalates every non-`LeaveRequest` signal error to a full reconnect, so the new variants follow the same path `Timeout` did.
…th knope Mark livekit-api and livekit as breaking (major) and livekit-uniffi as a feature (minor); on these 0.x crates knope bumps the minor (livekit-api 0.5.1 -> 0.6.0, livekit 0.7.45 -> 0.8.0). Register the new livekit-net crate as a knope package so its version and the workspace dependency entry are managed at release time; it ships at its initial 0.1.0.
jhugman
requested review from
1egoman,
cloudwebrtc,
ladvoc and
pblazej
as code owners
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let _ = response_chn | ||
| .send(Err(SignalError::Timeout(err.to_string()))); |
There was a problem hiding this comment.
🟡 Send failures on the WebSocket are misreported as timeouts, misleading error handling and diagnostics
A transport send failure is wrapped as a timeout error (SignalError::Timeout(err.to_string()) at livekit-api/src/signal_client/signal_stream.rs:108) instead of a connection or send error, so any caller inspecting the error variant sees "timeout" for what is actually a broken-pipe or closed-connection failure.
Impact: Logs and error-handling branches that distinguish timeouts from connection failures will misclassify the root cause, complicating debugging.
Mechanism: write_task maps all PlatformConnection::send errors to Timeout
When conn.send(data).await returns a TransportError (which could be Connection, Closed, or Other), the write_task at livekit-api/src/signal_client/signal_stream.rs:106-108 wraps it as SignalError::Timeout(err.to_string()). The old code used SignalError::WsError(err) which at least preserved the original error kind. The correct mapping would be SignalError::Connection(err.to_string()) or using super::map_transport_err(err) which already exists and correctly dispatches each TransportError variant to the appropriate SignalError.
| let _ = response_chn | |
| .send(Err(SignalError::Timeout(err.to_string()))); | |
| let _ = response_chn | |
| .send(Err(super::map_transport_err(err))); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| let status = | ||
| http::StatusCode::from_u16(res.status).unwrap_or(http::StatusCode::OK); |
There was a problem hiding this comment.
🟡 Invalid HTTP status codes from transport silently pass validation, hiding real errors
An unrecognised status code from the transport is defaulted to HTTP 200 OK (unwrap_or(http::StatusCode::OK) at livekit-api/src/signal_client/mod.rs:473), so the validate check silently succeeds instead of flagging the anomaly.
Impact: A foreign transport returning a non-standard status (e.g. 0 or 999) would cause the validation step to report success, hiding the actual connection problem from the caller.
Mechanism: StatusCode::from_u16 fallback to OK skips both error branches
In SignalInner::validate at livekit-api/src/signal_client/mod.rs:472-473, http::StatusCode::from_u16(res.status) returns Err for values outside 100–999. The fallback unwrap_or(http::StatusCode::OK) means neither is_client_error() nor is_server_error() is true, so the function returns Ok(()). By contrast, the sibling function map_transport_err at livekit-api/src/signal_client/mod.rs:945-946 defaults to BAD_GATEWAY (502), which at least triggers the server-error path. The validate function should use a similar fallback (e.g. BAD_GATEWAY) or return an explicit error for unrecognised status codes.
| let status = | |
| http::StatusCode::from_u16(res.status).unwrap_or(http::StatusCode::OK); | |
| let status = | |
| http::StatusCode::from_u16(res.status).unwrap_or(http::StatusCode::BAD_GATEWAY); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if matches!(&err, SignalError::Client(code, _) if code.as_u16() != 403) { | ||
| log::error!("unexpected signal error: {}", err.to_string()); |
There was a problem hiding this comment.
🔍 Region fallback logging now only covers 4xx errors, silently skipping 5xx
The old code at livekit-api/src/signal_client/mod.rs:215 matched SignalError::WsError(WsError::Http(e)) which covered ALL HTTP status codes from the WS upgrade (including 5xx). The new code matches only SignalError::Client(code, _) which is 4xx only. This means a 503 from a saturated node during WS upgrade (now SignalError::Server) or a SignalError::Connection from a network failure will no longer trigger the "unexpected signal error" log line. The actual fallback-to-region-URLs behavior is unaffected — it runs regardless of this log. But the logging gap could make it harder to diagnose production issues where the primary connection fails with a 5xx before the region fallback kicks in.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let transport = livekit_net::transport() | ||
| .ok_or_else(|| SignalError::Timeout("no platform transport registered".into()))?; |
There was a problem hiding this comment.
🔍 SignalError::Timeout is used as a catch-all for non-timeout conditions in multiple places
Beyond the write_task bug already reported, SignalError::Timeout is used for semantically non-timeout conditions: "no platform transport registered" at livekit-api/src/signal_client/signal_stream.rs:56 and livekit-api/src/signal_client/mod.rs:464, and "connection closed before message received" at livekit-api/src/signal_client/mod.rs:981 and :1014. The first is a configuration error (no transport), and the second is a connection closure. These would be better served by SignalError::Connection or a new variant. The engine's auth_failure_reason at livekit/src/rtc_engine/mod.rs:1198 doesn't key on Timeout, so the misclassification doesn't affect auth-failure detection, but it could confuse monitoring/alerting systems that categorize errors by variant.
Was this helpful? React with 👍 or 👎 to provide feedback.
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
I think I could use some guidance on how to best review this change - what areas are most important to focus on? Surfacing one thing that suck out to me now though.
| livekit-api: major | ||
| livekit: major |
There was a problem hiding this comment.
question: Should this be a major version bump? For a user of the rust crates directly (ie, not via livekit-ffi / livekit-uniffi), what about this change is breaking?
UPDATE: talked to lukas about this, I forgot that knope has some unintuitive behavior where major when under version 1.0.0 actually means minor. So, I think this is ok.
3 participants
This PR adds a
livekit-netcrate which is for HTTP and WebSocket traffic. i.e. signalling shaped tasks.SDKs that want to implement their own networking can call the
setPlatformTransportfunction with their own foreign trait implementation.It needs a Parallel Change:
Still to come:
Before you submit your PR
Make sure the following is true before submitting your PR:
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 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.