fix(realtime): record and envelope non-WebSocket requests to /v1/realtime - #887
Conversation
…time A non-WebSocket request (plain GET, malformed upgrade headers, or a connection that cannot upgrade) was refused by axum's WebSocketUpgrade extractor with a bare response: no access log, no usage event, no error envelope — the same silent pre-dispatch class #863/#880/#884 collected for body, path-param, and multipart rejections. The handler now takes the upgrade as a Result and maps a rejection into its normal error arm (access log + usage event + envelope) through a new ProxyError::WebSocketUpgradeRequired that carries the upgrade layer's own status classification — 400 for bad/missing upgrade headers, 426 Upgrade Required for a connection that cannot upgrade — so the refusal keeps the status the protocol expects instead of being flattened to 400. The wire error.type is `websocket_upgrade_required`, in the DP's stable taxonomy. Fixes #885.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
…ection detail Independent review findings: - The error arm emitted the access log and usage event but never record_request — #885 named the missing metrics explicitly, and a refusal storm would be invisible in the request-rate series while filling the logs. The arm now counts every refusal with the same unresolved labels reject_before_dispatch uses (also closing the pre-existing gap for prepare() failures). - "405 unreachable" was wrong: axum's get() also serves HEAD, so a HEAD request reaches the extractor's method check. The handler now extracts the real Method for the access log instead of hardcoding GET, and a test pins HEAD → 405 + Allow. - The per-variant rejection reason (which upgrade header was wrong) was being discarded; the variant now carries axum's body_text so callers and operators keep the diagnostic the bare rejection used to show. - RFC 9110 headers on the gateway-owned response: Upgrade: websocket on 426 (§15.5.22), Allow: GET, HEAD on 405 (§15.5.6) — both pinned.
…ared emit #887 landed a new `record_request` call for non-WebSocket requests to /v1/realtime while this branch was open — exactly the drift the shared chokepoint exists to prevent. Route it through `request_metrics::record` so the refusal reaches the detailed proxy families like every other pre-dispatch rejection.
1 participant
What
A non-WebSocket request to
/v1/realtime— a plain GET, malformed upgrade headers, a connection that cannot upgrade, or a HEAD — was refused by axum'sWebSocketUpgradeextractor with a bare response: no access log, no metrics, no usage event, no error envelope. The same silent pre-dispatch class #863 (body), #880/#884 (path params, multipart) collected; deferred out of #884 because the status semantics didn't fit the existing error mapping.The handler now takes the upgrade as a
Resultand maps a rejection into the endpoint's normal error arm — the same access log + usage event + envelope path every other/v1/realtimerefusal (bad key, unknown model, quota) already takes — via a newProxyError::WebSocketUpgradeRequired { status, detail }:426 Upgrade Requiredfor a connection that cannot upgrade, and 405 for HEAD (axum'sget()also serves HEAD, so the extractor's method check is reachable — the access log now records the real method instead of a hardcoded GET);record_requestwith the same unresolved labelsreject_before_dispatchuses — a refusal storm shows up in the request-rate series, not just the logs (this also closes the pre-existing gap whereprepare()failures weren't counted);Upgrade: websocketon 426 (§15.5.22),Allow: GET, HEADon 405 (§15.5.6);error.typeiswebsocket_upgrade_required, joining the DP's stable taxonomy; theOkpath is byte-identical for real WebSocket clients and extractor order is unchanged.Tests
realtime.rs: a plain GET answers 400 + the envelope, is emitted as a usage event (inbound_protocol="realtime", emptyapi_key_id— auth never ran — and the?model=echo) and is counted in the metrics scrape; correct WS headers over a non-upgradable connection keep axum's 426 plus theUpgradeheader; HEAD keeps its 405 plus theAllowheader and is recorded. Existing handshake/relay/subprotocol-auth tests pin that real WS clients are untouched (9/9).realtime-ws-e2e.test.ts): real binary — a plain HTTP GET answers the envelope withwebsocket_upgrade_required; the existing WS handshake and credential-rejection cases still pass.No LiteLLM baseline applies: gateway-internal rejection telemetry with no equivalent surface.
Fixes #885.