refactor(gateway): route realtime API through RouterTrait - #690
Conversation
github-actions
Bot
added
model-gateway
|
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:
📝 WalkthroughWalkthroughAdds OpenAI-compatible realtime REST and WebSocket routing, validation/types for realtime session requests, WorkerRegistry helpers for selecting external realtime workers, realtime metrics labels, router/server wiring for realtime flows, and RealtimeRegistry lifecycle integration. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant Router as OpenAIRouter
participant Registry as WorkerRegistry
participant Worker as ExternalWorker
rect rgba(100,150,200,0.5)
Client->>Server: realtime REST or WS request
Server->>Router: route_realtime_rest(...) or route_realtime_ws(req)
Router->>Registry: find_best_external_worker(model_id)
Registry->>Registry: filter external, healthy workers
Registry->>Registry: check supports_model + circuit breaker (can_execute)
Registry-->>Router: selected ExternalWorker or None
alt worker found
Router->>Worker: proxy request / upgrade websocket
Worker-->>Router: response / stream
Router-->>Client: response / websocket stream
else no worker
Router-->>Client: error (no worker / model not found)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Realtime API by integrating its WebSocket and REST endpoints into the existing Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
This comment was marked as resolved.
This comment was marked as resolved.
mergify
Bot
added
the
needs-rebase
7a781e5 to
4ef0fc8
Compare
mergify
Bot
removed
the
needs-rebase
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes the routing logic for the realtime API by integrating it with the RouterTrait. However, critical security vulnerabilities related to credential leakage in route_realtime_rest and route_realtime_ws have been identified. These are cross-cutting concerns that, per repository guidelines, should ideally be addressed in a dedicated pull request to ensure comprehensive design and review. Additionally, I've identified areas for improvement in error handling: providing more accurate error messages when all workers for a model are unhealthy, and refining request query parsing to return more specific error details to the client. Addressing these points will significantly improve the security, consistency, and robustness of the gateway.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a781e52cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/core/worker_registry.rs`:
- Around line 667-670: The helper any_external_worker_supports_model currently
restricts the scan to healthy-only workers (calls get_workers_filtered(...,
Some(RuntimeType::External), true)), which causes models that exist only on
unhealthy external workers to be treated as missing; update the call in
any_external_worker_supports_model to include unhealthy workers as well by
passing healthy_only = false (i.e., replace the final argument true with false)
so the check returns true if any external worker (healthy or not) supports the
model.
In `@model_gateway/src/routers/openai/router.rs`:
- Around line 1327-1344: The realtime WebSocket upgrade does not hold a
WorkerLoadGuard for the lifetime of the proxied session, so active WS sessions
aren't reflected in load selection and outcomes aren't recorded; create a
WorkerLoadGuard (from WorkerRegistry/WorkerLoadGuard) immediately after
selecting the worker and before calling super::realtime::proxy::run_ws_proxy,
keep the guard alive for the duration of the ws.on_upgrade async block (e.g.,
bind it to a local variable so it isn't dropped) and ensure you call the guard's
outcome recording method (or otherwise record success/failure) depending on
whether run_ws_proxy returns Ok or Err; reference session_id, cancel_token,
upstream_ws_url, auth_str, registry.clone(), and the run_ws_proxy call to locate
where to instantiate and record the WorkerLoadGuard.
- Around line 1163-1187: The code records success/duration from the upstream
resp before awaiting proxy_response(resp).await, so body-read failures (which
make proxy_response return 502) still count as successes; change the order and
logic: first call let response =
super::realtime::rest::proxy_response(resp).await (handling any Err case as an
upstream failure), then determine success from the proxy_response result/status,
and only after that call worker.record_outcome(success) and the appropriate
Metrics::record_router_duration or Metrics::record_router_error; update
references to resp/status to use the proxy_response outcome so circuit breaker
and realtime metrics reflect body-read failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ac05a67-7463-4368-8ac2-3f80f7e9776f
📒 Files selected for processing (7)
model_gateway/src/core/worker_registry.rsmodel_gateway/src/observability/metrics.rsmodel_gateway/src/routers/mod.rsmodel_gateway/src/routers/openai/realtime/rest.rsmodel_gateway/src/routers/openai/realtime/ws.rsmodel_gateway/src/routers/openai/router.rsmodel_gateway/src/server.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7659f82c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .route("/v1/realtime", get(realtime_ws::ws_handler)) | ||
| .route("/v1/realtime/sessions", post(realtime_rest::create_session)) | ||
| // Realtime endpoints (same middleware as other protected routes) | ||
| .route("/v1/realtime", get(v1_realtime_ws)) |
There was a problem hiding this comment.
Placing /v1/realtime under protected_routes subjects websocket upgrades to middleware::wasm_middleware, whose OnResponse path reconstructs a new response from status/headers/body without preserving response extensions (model_gateway/src/middleware.rs OnResponse reconstruction), so the upgrade metadata from WebSocketUpgrade::on_upgrade is dropped. In deployments with enable_wasm=true and any OnResponse module configured, realtime websocket handshakes can no longer complete even though the route returns 101.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Separated a ws route. for now.
There was a problem hiding this comment.
cc @slin1237
I wonder if we should fix this in wasm instead. I won't do it in this PR as it is kinda out of scope.
There was a problem hiding this comment.
Already addressed in commit c9fad02 — the WebSocket route is excluded from WASM middleware.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/openai/router.rs`:
- Around line 1093-1111: The code that extracts the model (the let model = if
endpoint == "/v1/realtime/client_secrets" { ... } else { ... } block) misses the
nested path used by the realtime transcription sessions endpoint, causing valid
requests to hit the error::bad_request branch; update the model extraction to
special-case "/v1/realtime/transcription_sessions" and read from
body.pointer("/input_audio_transcription/model").and_then(|v| v.as_str()) (or
similar) so that the variable model gets populated for that endpoint before the
match and backstop error::bad_request remains intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94527e80-c646-48be-a5c5-87209195030a
📒 Files selected for processing (1)
model_gateway/src/routers/openai/router.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19da07aa3c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
model_gateway/src/routers/openai/router.rs (3)
1224-1237:⚠️ Potential issue | 🔴 CriticalDon't ignore the transcription model when picking a worker.
The typed request already carries a model under
body.audio.input.transcription.model, but this path hardcodes"realtime-transcription". Requests that specify a real transcription model can miss every matching worker and fail as routing errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/openai/router.rs` around lines 1224 - 1237, The routing currently hardcodes the model key "realtime-transcription" in route_realtime_transcription_session when calling forward_realtime_rest, which ignores any model specified on the request at body.audio.input.transcription.model and can cause mismatched worker selection; update route_realtime_transcription_session to read body.audio.input.transcription.model (if present and non-empty) and pass that model string to forward_realtime_rest (falling back to "realtime-transcription" only when absent), keeping the same endpoint "/v1/realtime/transcription_sessions" and metrics_labels::ENDPOINT_REALTIME_TRANSCRIPTION.
348-372:⚠️ Potential issue | 🟠 MajorStill recording realtime REST success before proxying the body.
proxy_response(resp).awaitcan still turn a 2xx upstream into a 502 when reading the body fails, butsuccessis captured first and fed intorecord_outcome()/ duration metrics. Those failures will still look healthy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/openai/router.rs` around lines 348 - 372, The code records success/duration based on resp.status() before proxying, but proxy_response(resp).await can fail/convert a 2xx into a 502; move the proxying step before recording outcomes so the final response status (or error) is used: call proxy_response(resp).await first, then derive success from the proxied response (or from the error), call worker.record_outcome(success), and only then invoke Metrics::record_router_duration or Metrics::record_router_error (using start.elapsed() for duration) so metrics reflect the actual proxied result; reference proxy_response, worker.record_outcome, Metrics::record_router_duration, and Metrics::record_router_error to locate and update the logic.
1277-1293:⚠️ Potential issue | 🟠 MajorKeep worker/load tracking alive for the whole realtime WebSocket.
Once the
101response is returned, the selected worker is no longer covered by aWorkerLoadGuard, and therun_ws_proxy()result never feeds back into worker outcomes or router duration metrics. Active sessions stay invisible to load-based selection, and upstream WebSocket failures don't affect health tracking.Also applies to: 1361-1378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/openai/router.rs` around lines 1277 - 1293, The selected worker's load guard currently drops as soon as the 101 response is returned, so wrap and hold the WorkerLoadGuard for the full lifetime of the realtime WebSocket: after obtaining the worker via select_worker_for_model(&model, auth_header.as_ref()).await (the Ok(w) branch), keep the returned WorkerLoadGuard in a local variable that stays in scope, then call run_ws_proxy(...) while that guard is alive (do not return the 101 response immediately). When run_ws_proxy returns, map its Result into the existing worker outcome and router duration metrics (the same Metrics::record_router_error / success code paths used elsewhere) so upstream WebSocket failures and session duration are recorded; ensure run_ws_proxy receives any needed references and the guard is not moved out of scope prematurely so active sessions remain visible to load-based selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/protocols/src/realtime_session.rs`:
- Around line 41-48: The validate_session_create_request function currently only
checks req.model.is_none() and allows empty or whitespace-only strings; update
this validation (and the analogous check around lines 478-485) to reject blank
models by trimming and verifying non-empty content (e.g., treat model as invalid
if model.map(|m| m.trim().is_empty()).unwrap_or(true)); return
ValidationError::new("model is required") when the trimmed model is empty or
missing so empty/whitespace-only models produce a 400 instead of routing errors.
---
Duplicate comments:
In `@model_gateway/src/routers/openai/router.rs`:
- Around line 1224-1237: The routing currently hardcodes the model key
"realtime-transcription" in route_realtime_transcription_session when calling
forward_realtime_rest, which ignores any model specified on the request at
body.audio.input.transcription.model and can cause mismatched worker selection;
update route_realtime_transcription_session to read
body.audio.input.transcription.model (if present and non-empty) and pass that
model string to forward_realtime_rest (falling back to "realtime-transcription"
only when absent), keeping the same endpoint
"/v1/realtime/transcription_sessions" and
metrics_labels::ENDPOINT_REALTIME_TRANSCRIPTION.
- Around line 348-372: The code records success/duration based on resp.status()
before proxying, but proxy_response(resp).await can fail/convert a 2xx into a
502; move the proxying step before recording outcomes so the final response
status (or error) is used: call proxy_response(resp).await first, then derive
success from the proxied response (or from the error), call
worker.record_outcome(success), and only then invoke
Metrics::record_router_duration or Metrics::record_router_error (using
start.elapsed() for duration) so metrics reflect the actual proxied result;
reference proxy_response, worker.record_outcome,
Metrics::record_router_duration, and Metrics::record_router_error to locate and
update the logic.
- Around line 1277-1293: The selected worker's load guard currently drops as
soon as the 101 response is returned, so wrap and hold the WorkerLoadGuard for
the full lifetime of the realtime WebSocket: after obtaining the worker via
select_worker_for_model(&model, auth_header.as_ref()).await (the Ok(w) branch),
keep the returned WorkerLoadGuard in a local variable that stays in scope, then
call run_ws_proxy(...) while that guard is alive (do not return the 101 response
immediately). When run_ws_proxy returns, map its Result into the existing worker
outcome and router duration metrics (the same Metrics::record_router_error /
success code paths used elsewhere) so upstream WebSocket failures and session
duration are recorded; ensure run_ws_proxy receives any needed references and
the guard is not moved out of scope prematurely so active sessions remain
visible to load-based selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9cd8a146-a5bf-4f5f-b05d-bb6a7fece40a
📒 Files selected for processing (5)
crates/protocols/src/realtime_session.rsmodel_gateway/src/routers/mod.rsmodel_gateway/src/routers/openai/router.rsmodel_gateway/src/routers/router_manager.rsmodel_gateway/src/server.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8eb6ee6456
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
model_gateway/src/routers/openai/router.rs (1)
1357-1374:⚠️ Potential issue | 🟠 MajorWorker load and outcome tracking missing for WebSocket sessions.
After
ws.on_upgrade()returns the 101 response, noWorkerLoadGuardspans the WebSocket session lifetime. This means:
- Load blindness: Active realtime sessions are invisible to
find_best_external_worker(), potentially causing unbalanced routing to already-busy workers.- Circuit breaker gaps: Neither success nor failure of the upstream WebSocket connection is recorded via
worker.record_outcome(), so repeated WS failures won't trip the circuit breaker.The session is registered with
realtime_registryfor lifecycle management, but worker load accounting requires a separate mechanism.🔧 Suggested approach
Create a
WorkerLoadGuardbefore upgrade and pass it into theon_upgradeclosure to keep it alive for the session duration. Record outcome based onrun_ws_proxyresult:+ let load_guard = WorkerLoadGuard::new(worker.clone(), Some(&parts.headers)); + ws.on_upgrade(move |socket: WebSocket| async move { - if let Err(e) = run_ws_proxy( + let result = run_ws_proxy( socket, &upstream_ws_url, &auth_str, registry.clone(), session_id.clone(), cancel_token, ) - .await - { - tracing::error!(session_id, error = %e, "Realtime WebSocket proxy error"); - } + .await; + + // Record outcome for circuit breaker + let success = result.is_ok(); + load_guard.worker().record_outcome(success); + drop(load_guard); // Explicit drop for clarity + + if let Err(e) = result { + tracing::error!(session_id, error = %e, "Realtime WebSocket proxy error"); + } // Cleanup: remove session on disconnect registry.remove_session(&session_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/openai/router.rs` around lines 1357 - 1374, The WebSocket path creates a realtime session but never holds a WorkerLoadGuard for the session lifetime or records worker outcomes; before calling ws.on_upgrade(...) create/acquire a WorkerLoadGuard (the same kind used by find_best_external_worker) and move it into the on_upgrade closure so it lives for the entire session, then after awaiting run_ws_proxy(...) call worker.record_outcome(success_or_failure) based on the Result from run_ws_proxy to update the circuit breaker; also ensure registry.remove_session(&session_id) remains after the upgrade and that the guard is dropped only when the closure exits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@model_gateway/src/routers/openai/router.rs`:
- Around line 1357-1374: The WebSocket path creates a realtime session but never
holds a WorkerLoadGuard for the session lifetime or records worker outcomes;
before calling ws.on_upgrade(...) create/acquire a WorkerLoadGuard (the same
kind used by find_best_external_worker) and move it into the on_upgrade closure
so it lives for the entire session, then after awaiting run_ws_proxy(...) call
worker.record_outcome(success_or_failure) based on the Result from run_ws_proxy
to update the circuit breaker; also ensure registry.remove_session(&session_id)
remains after the upgrade and that the guard is dropped only when the closure
exits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9241807a-6f63-40c0-b408-55d06cb3cdf2
📒 Files selected for processing (2)
model_gateway/src/routers/openai/router.rsmodel_gateway/src/server.rs
… realtime metrics labels Add find_best_external_worker() and any_external_worker_supports_model() as pure data query methods on WorkerRegistry. These provide reusable worker selection logic without HTTP dependencies in the core module. Add metrics label constants for realtime endpoints (sessions, client_secrets, transcription) and WebSocket connection type. Signed-off-by: Chang Su <chang.s.su@oracle.com>
…endpoints Realtime API handlers (WebSocket, REST) previously bypassed RouterTrait entirely, accessing worker_registry directly. This caused realtime traffic to miss lazy model refresh (spurious 503s), metrics, and WASM middleware. Add route_realtime_rest() and route_realtime_ws() to RouterTrait with default not-implemented stubs. Implement both in OpenAIRouter using the existing select_worker_for_model flow with lazy refresh, metrics recording, and circuit breaker integration. Move realtime routes from a separate route group into protected_routes in server.rs so they share the same middleware stack (auth, concurrency, WASM) as all other endpoints. Thin handlers in server.rs delegate to state.router.route_realtime_*() following the standard pattern. Strip rest.rs and ws.rs down to shared helpers (proxy_response, build_upstream_ws_url) now that handler logic lives in the router. Signed-off-by: Chang Su <chang.s.su@oracle.com> # Conflicts: # model_gateway/src/routers/mod.rs
Replace inline super::realtime::* paths with top-level crate:: imports to satisfy clippy absolute_paths lint and improve readability. Signed-off-by: Chang Su <chang.s.su@oracle.com>
…dpoints Replace untyped Json<Value> with ValidatedJson<TypedStruct> for realtime REST handlers, matching the pattern used by all other endpoints (chat, messages, responses, etc.). - Add RealtimeClientSecretCreateRequest wrapper type with nested session - Add Validate + Normalizable impls with model-required validation for RealtimeSessionCreateRequest and RealtimeClientSecretCreateRequest - Split route_realtime_rest into three typed RouterTrait methods: route_realtime_session, route_realtime_client_secret, route_realtime_transcription_session - Extract shared forwarding logic into forward_realtime_rest helper - Add RouterManager delegation for all realtime methods (fixes 501 fallthrough when state.router is RouterManager) Signed-off-by: Chang Su <chang.s.su@oracle.com>
WASM OnResponse reconstructs the response from status/headers/body, dropping response extensions that carry the WebSocket upgrade future. Move the /v1/realtime WS route to a separate route group with auth and concurrency middleware but without WASM, preventing broken handshakes when enable_wasm=true. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Return the actual Axum rejection response instead of a hardcoded error message, providing more accurate detail on whether the query string was missing or failed to deserialize. Signed-off-by: Chang Su <chang.s.su@oracle.com>
8eb6ee6 to
99f6a2e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99f6a2e1e1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/protocols/src/realtime_session.rs (1)
41-48:⚠️ Potential issue | 🟡 MinorReject blank realtime models during validation.
These schema hooks still only check
is_none(), so""and whitespace-onlymodelvalues passValidatedJsonand fail later as routing errors instead of a 400.🛠️ Suggested fix
fn validate_session_create_request( req: &RealtimeSessionCreateRequest, ) -> Result<(), ValidationError> { - if req.model.is_none() { + let missing_model = req + .model + .as_deref() + .map(str::trim) + .filter(|model| !model.is_empty()) + .is_none(); + if missing_model { return Err(ValidationError::new("model is required")); } Ok(()) } @@ fn validate_client_secret_create_request( req: &RealtimeClientSecretCreateRequest, ) -> Result<(), ValidationError> { - if req.session.model.is_none() { + let missing_model = req + .session + .model + .as_deref() + .map(str::trim) + .filter(|model| !model.is_empty()) + .is_none(); + if missing_model { return Err(ValidationError::new("session.model is required")); } Ok(()) }Also applies to: 478-485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/protocols/src/realtime_session.rs` around lines 41 - 48, The validate_session_create_request function currently only checks req.model.is_none(), allowing empty or whitespace-only strings; update it to treat empty or all-whitespace model values as invalid by checking the string content (e.g., by calling trim() or equivalent on req.model.as_ref()) and return Err(ValidationError::new("model is required")) when the model is missing or blank; apply the same change to the other validation hook noted (the duplicate at the other validate function location around the 478-485 area) so both reject empty/whitespace-only model strings.model_gateway/src/routers/router_manager.rs (1)
755-819:⚠️ Potential issue | 🟠 MajorUse model-aware router selection for realtime delegation.
In IGW, these four methods still call
select_router_for_request(..., None), so realtime traffic is dispatched by the generic default/PD heuristic instead of the session/query model. That can still send/v1/realtime*to routers that only inherit the 501 stub. Passbody.model/body.session.modelinto selection, parse the WSmodelquery before dispatch, and route transcription sessions to the OpenAI router explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/router_manager.rs` around lines 755 - 819, The four realtime handlers (route_realtime_session, route_realtime_client_secret, route_realtime_transcription_session, route_realtime_ws) are calling select_router_for_request(..., None) and thus ignore the request model; update each to supply the appropriate model when selecting a router: for route_realtime_session and route_realtime_client_secret pass body.model (or body.session.model if present) into select_router_for_request, for route_realtime_transcription_session parse the model from body.session.model and explicitly route transcription sessions to the OpenAI router (i.e., choose the OpenAI router identifier instead of the generic heuristic), and for route_realtime_ws extract/parse the model from the WS request's query string before calling select_router_for_request; keep the same NOT_FOUND fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/core/worker_registry.rs`:
- Around line 666-670: Update the docstring for
any_external_worker_supports_model to state explicitly that it only scans
healthy external workers (it calls get_workers_filtered with healthy_only =
true), so callers understand this helper excludes unhealthy workers and that 503
is reserved for healthy-but-circuit-open cases while unhealthy workers may lead
to 404; reference the function name any_external_worker_supports_model and its
call to get_workers_filtered(RuntimeType::External, healthy_only = true) so
maintainers can see the intended behavior.
---
Duplicate comments:
In `@crates/protocols/src/realtime_session.rs`:
- Around line 41-48: The validate_session_create_request function currently only
checks req.model.is_none(), allowing empty or whitespace-only strings; update it
to treat empty or all-whitespace model values as invalid by checking the string
content (e.g., by calling trim() or equivalent on req.model.as_ref()) and return
Err(ValidationError::new("model is required")) when the model is missing or
blank; apply the same change to the other validation hook noted (the duplicate
at the other validate function location around the 478-485 area) so both reject
empty/whitespace-only model strings.
In `@model_gateway/src/routers/router_manager.rs`:
- Around line 755-819: The four realtime handlers (route_realtime_session,
route_realtime_client_secret, route_realtime_transcription_session,
route_realtime_ws) are calling select_router_for_request(..., None) and thus
ignore the request model; update each to supply the appropriate model when
selecting a router: for route_realtime_session and route_realtime_client_secret
pass body.model (or body.session.model if present) into
select_router_for_request, for route_realtime_transcription_session parse the
model from body.session.model and explicitly route transcription sessions to the
OpenAI router (i.e., choose the OpenAI router identifier instead of the generic
heuristic), and for route_realtime_ws extract/parse the model from the WS
request's query string before calling select_router_for_request; keep the same
NOT_FOUND fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54f1669d-bcc6-4437-aa1b-4339236f23e5
📒 Files selected for processing (9)
crates/protocols/src/realtime_session.rsmodel_gateway/src/core/worker_registry.rsmodel_gateway/src/observability/metrics.rsmodel_gateway/src/routers/mod.rsmodel_gateway/src/routers/openai/realtime/rest.rsmodel_gateway/src/routers/openai/realtime/ws.rsmodel_gateway/src/routers/openai/router.rsmodel_gateway/src/routers/router_manager.rsmodel_gateway/src/server.rs
find_best_external_worker and any_external_worker_supports_model are only called from OpenAIRouter. Move them back as private methods to keep WorkerRegistry free of caller-specific logic. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Rename find_best_external_worker -> find_best_worker_for_model and any_external_worker_supports_model -> any_worker_supports_model to match the original names from sgl-project/sglang#15611. Place them right before select_worker_for_model where they originally lived. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Move forward_realtime_rest and route_realtime_ws bodies into free pub(crate) functions in their respective submodule files, reducing router.rs by ~250 lines while keeping realtime code colocated with its helpers. The trait impls in router.rs become thin delegators that handle worker selection then forward to the extracted functions. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Empty and whitespace-only model strings now fail validation with a 400 instead of passing through and failing later as routing errors. Signed-off-by: Chang Su <chang.s.su@oracle.com>
…ion request The OpenAI transcription session API has top-level model, language, and prompt fields that were missing from RealtimeTranscriptionSessionCreateRequest. This caused the router to use a hardcoded synthetic model id instead of the client-specified model. Now uses the actual model for worker routing and validates it is present. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Move WebSocket query param extraction and model validation to server.rs, matching the pattern used by REST endpoints (ValidatedJson in server.rs). The route_realtime_ws trait method now takes a model parameter. RouterManager passes the model to select_router_for_request for all realtime endpoints (REST and WS), ensuring correct router selection in IGW deployments with multiple routers. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Move proxy_response and build_upstream_ws_url to the bottom of their respective files, matching their original positions for easier diff comparison. Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42b1672110
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…t#690) Signed-off-by: Sydney Firmin <sydney.firmin@oracle.com>
2 participants
Description
Problem
PR #637 introduced realtime API support (WebSocket + REST) but bypassed
RouterTraitentirely — handlers accessedstate.context.worker_registrydirectly instead of going throughstate.router. This meant realtime traffic lacked:Solution
Route realtime API through
RouterTraitfollowing the same pattern asv1/chat/completions,v1/messages, and other endpoints.Changes
WorkerRegistry: Addfind_best_external_worker()andany_external_worker_supports_model()as pure data query methods (no HTTP dependencies in core)RouterTrait: Addroute_realtime_rest()androute_realtime_ws()with default not-implemented stubsOpenAIRouter: Implement both methods using existingselect_worker_for_modelflow with lazy refresh, metrics recording (record_router_request,record_router_duration,record_router_error), and circuit breaker integrationserver.rs: Replace direct realtime handler imports with thin delegates tostate.router.route_realtime_*(). Merge realtime routes intoprotected_routesso they share auth, concurrency, and WASM middlewarerest.rs/ws.rs: Strip down to shared helpers (proxy_response,build_upstream_ws_url) now that handler logic lives in the routermetrics.rs: AddENDPOINT_REALTIME_*andCONNECTION_WEBSOCKETlabel constantsTest Plan
OpenAIRouter::select_worker_for_modellazy refresh logic is exercised by realtime the same way as chat/completionsChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Enhancements
Refactor