fix(http): enforce streaming timeouts outside reqwest - #1858
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:
📝 WalkthroughWalkthroughAdds a configurable stream idle timeout, a shared ChangesStream Idle Timeout Feature
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated streaming_client alongside the standard HTTP client in AppContext and the various routers (Anthropic, HTTP, OpenAI) to handle streaming requests without a total request timeout, preventing premature timeouts during long-lived streams. Feedback suggests exposing a dedicated streaming_client builder method on AppContextBuilder to allow callers to configure and customize the streaming client independently of the standard client.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
e353ffc to
536a941
Compare
5717ea2 to
27353c3
Compare
b7ee09a to
eb93e0d
Compare
eb93e0d to
9210c8a
Compare
|
Hi @jshanson7, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@model_gateway/src/routers/anthropic/worker.rs`:
- Around line 240-257: Reuse the existing record_error_metrics helper in
handle_error_response instead of duplicating the duration and error metric calls
inline. Update the non-streaming error path in the anthropic worker to pass the
same model_id, start_time, and error_type into record_error_metrics, keeping the
metric labels and behavior unchanged while removing the repeated block.
In `@model_gateway/src/routers/http/router.rs`:
- Around line 663-692: The streaming total-timeout branch in the `send_result`
handling path is missing `Metrics::record_worker_error`, unlike the `reqwest`
error handling later in `router.rs`. Update the `is_stream` timeout arm around
`StreamDeadline::until_total(...)` to record a worker error before returning the
504 response, using the same `model_id`, `endpoint`, and status-derived error
classification as the existing `worker.record_outcome` and
`Metrics::record_router_error` calls.
In `@model_gateway/src/routers/openai/chat.rs`:
- Around line 188-204: In `chat.rs`, the `is_streaming` branch in
`StreamDeadline::new()`/`until_total(req.send())` currently returns a 504 from
`error::gateway_timeout`, which is treated as retryable by
`is_retryable_status()`. Update this timeout path so streaming total-timeout
responses are marked non-retryable or use a distinct non-retryable status, and
keep the change localized to the `resp` handling in `openai::chat` so repeated
attempts do not reconsume the full request timeout.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ae371554-17cf-4825-bed7-856f9eedf979
📒 Files selected for processing (23)
docs/concepts/reliability/rate-limiting.mddocs/reference/configuration.mdmodel_gateway/src/app_context.rsmodel_gateway/src/config/builder.rsmodel_gateway/src/config/types.rsmodel_gateway/src/config/validation.rsmodel_gateway/src/main.rsmodel_gateway/src/routers/anthropic/context.rsmodel_gateway/src/routers/anthropic/non_streaming.rsmodel_gateway/src/routers/anthropic/router.rsmodel_gateway/src/routers/anthropic/sse.rsmodel_gateway/src/routers/anthropic/streaming.rsmodel_gateway/src/routers/anthropic/worker.rsmodel_gateway/src/routers/common/mod.rsmodel_gateway/src/routers/common/stream_timeout.rsmodel_gateway/src/routers/http/pd_router.rsmodel_gateway/src/routers/http/router.rsmodel_gateway/src/routers/openai/chat.rsmodel_gateway/src/routers/openai/context.rsmodel_gateway/src/routers/openai/responses/streaming.rsmodel_gateway/src/routers/openai/router.rsmodel_gateway/src/service_discovery.rsmodel_gateway/src/workflow/steps/local/drain_workers.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9210c8a560
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9210c8a to
8739949
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/app_context.rs (1)
430-502: 🚀 Performance & Scalability | 🔵 TrivialDual clients double the idle-connection ceiling per worker.
Each
build_worker_http_clientcall setspool_max_idle_per_host(500), so a single worker can now accumulate up to 1000 idle connections combined (500 forclient+ 500 forstreaming_client) instead of 500. Likely fine given today's deployment sizes, but worth keeping in mind for connection/FD budgeting as worker counts grow.🤖 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 `@model_gateway/src/app_context.rs` around lines 430 - 502, `build_worker_http_client` is creating two separate reqwest clients (`client` and `streaming_client`) that each allow 500 idle connections per host, effectively doubling the per-worker idle-connection cap. Reduce the combined idle pool budget by lowering `pool_max_idle_per_host` for these clients or otherwise centralize the limit so the total across both clients stays at the intended ceiling. Use the `client` and `streaming_client` setup in this section to keep the per-worker FD/connection budget bounded.
🤖 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 `@model_gateway/src/main.rs`:
- Around line 426-429: Add a two-path plumbing test for stream_idle_timeout_secs
in main.rs to match the existing *_flows_into_both_configs coverage used by
engine_metrics, multimodal_tensor_transport, runtime_worker_threads, and
health_check_port. Create a test that builds CLI args with
--stream-idle-timeout-secs, calls cli_args_from and then to_router_config (and
the server-config path if the other tests do both), and asserts the value is
preserved so the flag is verified end-to-end through the config conversion flow.
In `@model_gateway/src/routers/anthropic/streaming.rs`:
- Around line 110-141: The stream relay logic in the anthropic streaming path is
duplicated between the main response handling and the SSE error handling.
Extract the repeated `stream_deadline.next(...)` / `tx.send(...)` / break-loop
behavior from the `tokio::spawn` block into a shared helper used by both call
sites, keeping the existing timeout and error translation behavior intact. Make
the helper reusable for both the success path and SSE error path so
`Body::from_stream` and the error-event sender rely on the same relay
implementation.
In `@model_gateway/src/routers/common/stream_timeout.rs`:
- Around line 45-50: The sse_error_event method is hand-building a JSON payload
by interpolating self.message(timeout) directly into a string, which is fragile
if the message ever contains characters that need escaping. Update
sse_error_event in StreamTimeoutKind to construct the SSE error payload with
serde_json::json! or a typed serializable struct and serialize it with
serde_json::to_string, then wrap that serialized JSON in the Bytes response.
---
Outside diff comments:
In `@model_gateway/src/app_context.rs`:
- Around line 430-502: `build_worker_http_client` is creating two separate
reqwest clients (`client` and `streaming_client`) that each allow 500 idle
connections per host, effectively doubling the per-worker idle-connection cap.
Reduce the combined idle pool budget by lowering `pool_max_idle_per_host` for
these clients or otherwise centralize the limit so the total across both clients
stays at the intended ceiling. Use the `client` and `streaming_client` setup in
this section to keep the per-worker FD/connection budget bounded.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a439a475-9de2-4acc-9f03-e6a3724e31fe
📒 Files selected for processing (23)
docs/concepts/reliability/rate-limiting.mddocs/reference/configuration.mdmodel_gateway/src/app_context.rsmodel_gateway/src/config/builder.rsmodel_gateway/src/config/types.rsmodel_gateway/src/config/validation.rsmodel_gateway/src/main.rsmodel_gateway/src/routers/anthropic/context.rsmodel_gateway/src/routers/anthropic/non_streaming.rsmodel_gateway/src/routers/anthropic/router.rsmodel_gateway/src/routers/anthropic/sse.rsmodel_gateway/src/routers/anthropic/streaming.rsmodel_gateway/src/routers/anthropic/worker.rsmodel_gateway/src/routers/common/mod.rsmodel_gateway/src/routers/common/stream_timeout.rsmodel_gateway/src/routers/http/pd_router.rsmodel_gateway/src/routers/http/router.rsmodel_gateway/src/routers/openai/chat.rsmodel_gateway/src/routers/openai/context.rsmodel_gateway/src/routers/openai/responses/streaming.rsmodel_gateway/src/routers/openai/router.rsmodel_gateway/src/service_discovery.rsmodel_gateway/src/workflow/steps/local/drain_workers.rs
💤 Files with no reviewable changes (1)
- model_gateway/src/workflow/steps/local/drain_workers.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 873994913e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8739949 to
1f64f27
Compare
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
6bcd1d4 to
fdf505c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdf505c4b5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 490d962d8a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaafcf19d3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5eedf093f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eff2867217
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c20cf45004
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 298681bc2d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef76e470c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95670114fd
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a465d16a47
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cd2950a71
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1271a373b7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05f664a5af
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jeff Hanson <jshanson7@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05ccf91a26
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut terminal_seen = false; | ||
|
|
||
| loop { | ||
| let chunk_result = match stream_deadline.next(&mut upstream_stream).await { |
There was a problem hiding this comment.
Recheck the deadline while forwarding Responses events
When an MCP Responses worker yields a large chunk just before the total deadline, this is the last deadline check before the inner next_block() loop parses and forwards every buffered event. Because those sends use the unbounded channel without another elapsed-time check, the gateway can continue emitting ordinary events after request_timeout_secs; if the same chunk contains a terminal event, it can even record the stream as successful without ever observing the expired deadline. Check the shared deadline before forwarding each decoded event, reserving post-deadline sends for the timeout event.
Useful? React with 👍 / 👎.
| if is_terminal { | ||
| break 'stream_loop; |
There was a problem hiding this comment.
Preserve the trailing Responses done marker
When a normal Responses upstream sends response.completed followed by data: [DONE]—the ordering used by the repository's mock worker—this branch exits the outer relay immediately after forwarding response.completed, so the subsequent done marker is dropped. Clients that rely on [DONE] to finalize a Responses stream receive an unexpected EOF instead; continue consuming through [DONE] or synthesize the marker when treating response.completed as terminal.
Useful? React with 👍 / 👎.
| let mut done_observer = sse::SseTerminalObserver::done(); | ||
| let mut terminal_seen = false; | ||
| loop { | ||
| let chunk = match stream_deadline.next(&mut s).await { |
There was a problem hiding this comment.
Bound queued chat output by the total deadline
When the upstream chat worker is faster than a slow downstream client, this deadline only governs reads from the upstream stream; each chunk is then placed on an unbounded channel. The relay can therefore consume through [DONE], record success, and exit before the client receives the queued data, after which the HTTP body may keep draining that queue well past request_timeout_secs while memory grows with the backlog. Use a bounded channel and deadline-aware sends so downstream delivery remains part of the total stream cap.
Useful? React with 👍 / 👎.
Problem
Streaming routes currently use a reqwest client configured with
request_timeout_secs. Reqwest applies that timeout while reading the response body, so a long generation that hits the cap gets cut off as an incomplete chunked read instead of a controlled SMG timeout.Fix
Use a separate worker HTTP client for streaming requests without reqwest's total body timeout, and enforce streaming deadlines in SMG instead:
request_timeout_secsremains the total wall-clock cap, from upstream send through body relay.stream_idle_timeout_secscaps gaps between streamed chunks, defaulting to 300s.Non-streaming requests still use the existing timeout-bearing client. The idle timeout is exposed as
--stream-idle-timeout-secsfor CLI users.Test Plan
Summary by CodeRabbit
New Features
--stream-idle-timeout-secs(default300) to configure the maximum idle gap between streamed chunks, including wiring into server and router behavior.Bug Fixes
Documentation
Tests