Bound outbound HTTP responses and wait time - #625
Conversation
|
Warning Review limit reached
Next review available in: 59 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: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
✨ 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 |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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.
Pull request overview
This PR hardens WFL outbound HTTP (open url) handling by preventing unbounded response buffering and ensuring stalled peers can’t indefinitely occupy an execution, while aligning documentation and adding deterministic local regression coverage.
Changes:
- Stream outbound HTTP responses and enforce
web_server_max_response_sizeon both received bytes and decoded text (including chunked/malformed UTF-8 expansion). - Apply execution-budget cancellation and wall-clock deadlines across connect/headers/body, with a finite per-request timeout inside lifetime-exempt
main loop. - Add deterministic local TCP-peer regression tests and update configuration/runtime documentation to reflect the shared response limit.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/http_outbound_budget_test.rs | Adds deterministic regression tests for outbound response-size limits, UTF-8 expansion, cancellation, and main-loop per-request timeout behavior. |
| src/interpreter/mod.rs | Introduces bounded/streamed outbound HTTP reads, structured outbound HTTP error mapping, and budget/timeout enforcement with cooperative cancellation. |
| src/exec/budget.rs | Clarifies that max_response_bytes applies to both handler responses and outbound open url reads. |
| src/config.rs | Updates config documentation to state web_server_max_response_size applies to both inbound handler responses and outbound reads. |
| Docs/reference/configuration-reference.md | Documents outbound behavior for web_server_max_response_size and how timeout_seconds applies inside/outside main loop. |
| Docs/04-advanced-features/interoperability.md | Documents bounded outbound response streaming and timeout/cancellation semantics. |
| Cargo.toml | Enables reqwest streaming support and adds encoding_rs for incremental decoding. |
| Cargo.lock | Locks new transitive dependencies from reqwest streaming + encoding_rs. |
| fuzz/Cargo.lock | Mirrors dependency lock updates for fuzz workspace. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[allow(dead_code)] | ||
| async fn http_get(&self, url: &str) -> Result<String, String> { | ||
| match self.http_client.get(url).send().await { | ||
| Ok(response) => match response.text().await { | ||
| Ok(text) => Ok(text), | ||
| Err(e) => Err(format!("Failed to read response body: {e}")), | ||
| }, | ||
| Err(e) => Err(format!("Failed to send HTTP GET request: {e}")), | ||
| } | ||
| async fn http_get( |
| #[allow(dead_code)] | ||
| async fn http_post(&self, url: &str, data: &str) -> Result<String, String> { | ||
| match self | ||
| .http_client | ||
| .post(url) | ||
| .body(data.to_string()) | ||
| .send() | ||
| .await | ||
| { | ||
| Ok(response) => match response.text().await { | ||
| Ok(text) => Ok(text), | ||
| Err(e) => Err(format!("Failed to read response body: {e}")), | ||
| }, | ||
| Err(e) => Err(format!("Failed to send HTTP POST request: {e}")), | ||
| } | ||
| async fn http_post( |
| HttpClientError::Timeout { seconds } => RuntimeError::with_kind( | ||
| format!("Outbound HTTP request exceeded timeout ({seconds}s)"), | ||
| line, | ||
| column, | ||
| ErrorKind::Timeout, | ||
| ), |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if written > 0 { | ||
| output.try_reserve_exact(written).map_err(|error| { | ||
| HttpClientError::Request(format!( | ||
| "Failed to allocate bounded HTTP response buffer: {error}" | ||
| )) | ||
| })?; | ||
| let text = std::str::from_utf8(&decoded[..written]) | ||
| .expect("encoding_rs must emit valid UTF-8"); | ||
| output.push_str(text); |
There was a problem hiding this comment.
🟡 Large downloads can grow their text buffer inefficiently, wasting CPU as they get bigger
The response text is grown one small piece at a time by requesting the exact extra room each step (try_reserve_exact(written) at src/interpreter/mod.rs:1431), so the whole accumulated text can be recopied on every step and the work grows quadratically as the response gets larger.
Impact: A large-but-allowed response (up to the 64 MiB ceiling), or a peer that deliberately sends one, can burn far more CPU than the payload size warrants.
Quadratic growth from exact-reserve in the decode loop
In decode_http_chunk (src/interpreter/mod.rs:1413-1447) each decoded slice (≤ 8 KiB) is appended after output.try_reserve_exact(written). Because try_reserve_exact grows capacity to exactly len + written (no amortized headroom) and push_str then fills it completely, the next chunk finds capacity == len and must reallocate again. Repeating this for a response near the 64 MiB ceiling yields thousands of reallocations, each potentially copying the entire buffer (O(n²) total copying).
The initial buffer is also capped at 64 KiB (.min(64 * 1024) at src/interpreter/mod.rs:1354) even when Content-Length is known and within the limit, so the amortization-free path is entered for essentially all large responses. The stated intent (comment at src/interpreter/mod.rs:1409-1412) was to avoid a transient 2x memory spike near the ceiling, but the chosen mechanism trades that for quadratic CPU. Real-world impact varies with the allocator's realloc/mremap behavior, but heap-sized responses reliably copy on every step.
A fix could preallocate min(content_length, max_response_bytes) when the length is known, or use amortized try_reserve for the unknown-length (chunked) path while still enforcing the byte ceiling on each append.
Prompt for agents
In decode_http_chunk (src/interpreter/mod.rs:1413-1447) the response text is appended after calling output.try_reserve_exact(written). Because reserve_exact leaves no amortized headroom and push_str fills capacity exactly, every subsequent chunk forces a reallocation that can copy the entire buffer, giving O(n^2) copying for large responses. Additionally, initial_capacity is capped at 64 KiB (src/interpreter/mod.rs:1349-1354) even when Content-Length is known and within the limit, so the slow path is taken for essentially all large payloads. Consider preallocating min(content_length, max_response_bytes) when Content-Length is known, and/or switching the per-chunk reservation to amortized try_reserve while still enforcing the max_response_bytes ceiling on each append. The goal is bounded memory (never far above the configured limit) without quadratic recopying.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Outbound responses are streamed and decoded into a bounded buffer. The | ||
| `web_server_max_response_size` setting (64 MiB by default) limits the response | ||
| body for `read content` and `read response`, both as received and after text | ||
| decoding. The limit includes chunked responses with no declared length. Outside | ||
| a `main loop`, the connection and body read share the script's remaining | ||
| `timeout_seconds`; inside a lifetime-exempt `main loop`, each request gets a | ||
| fresh timeout of that duration. Cooperative cancellation also interrupts a | ||
| request that is waiting on the remote peer. |
There was a problem hiding this comment.
🟡 Non-trivial behavior change ships without the required Dev Diary entry
This change adds new outbound HTTP streaming/bounding behavior and documents it in the guides (Docs/04-advanced-features/interoperability.md:95-102), but it does not include a Dev Diary entry, which the repository rules require for any non-trivial feature or behavior change.
Impact: The change set violates the repository's mandatory documentation policy, so a required record of this behavior change is missing.
Repo rule requiring a Dev Diary entry
CLAUDE.md and AGENTS.md state under Documentation Development that a non-trivial feature or behavior change must ship "A Dev Diary entry in Dev diary/" in the same change. This PR alters outbound open url semantics (streaming, response ceiling on received and decoded bytes, per-request timeouts, cooperative cancellation) — clearly non-trivial — but the diff adds no file under Dev diary/.
Prompt for agents
The repository rules (CLAUDE.md / AGENTS.md, Documentation Development section) require a Dev Diary entry in the Dev diary/ directory for any non-trivial feature or behavior change. This PR changes outbound HTTP (open url) behavior substantially: streaming responses, enforcing web_server_max_response_size on received and decoded bytes including chunked bodies, applying execution-budget timeouts to connect/headers/body, and cooperative cancellation. Add a dated Dev Diary markdown entry (matching the existing naming convention like 2026-07-16-bounded-outbound-http.md) describing the motivation, behavior change, and validation.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Superseded by #632, which preserves this security fix in the consolidated Rust-source hardening PR. The combined head is mergeable and all required CI checks are green. |
Summary
open urlresponses instead of buffering them without a source-level boundweb_server_max_response_sizeon both received bytes and decoded UTF-8 text, including chunked bodies and malformed-text expansionSecurity impact
This prevents an untrusted or stalled HTTP peer from causing unbounded response buffering or indefinitely occupying a WFL execution. The check runs before appending decoded data beyond the configured ceiling.
Validation
cargo fmt --all -- --checkandgit diff --checkpassedPart of the Rust-source production-readiness work tracked in #610.