refactor(api_client): unify get_bytes behind CalloutClient#492
Conversation
…oxy#454) Route content downloads through CalloutClient instead of a separate reqwest::Client, gaining circuit-breaker and callout-depth protection. The client-wide max_response_bytes caps memory during collection while a per-request max_bytes post-check preserves ResponseTooLarge (413). Closes praxis-proxy#454 Signed-off-by: Sébastien Han <seb@redhat.com>
| client: &CalloutClient, | ||
| request: CalloutRequest, | ||
| ) -> Result<praxis_core::callout::CalloutResponse, ApiClientError> { | ||
| match client.execute(request).await { |
There was a problem hiding this comment.
Issue #454 requires preserving ResponseTooLarge so openai_file_resolve returns 413 for size limits. The per-request max_bytes post-check correctly maps to ResponseTooLarge -> 413 when the body is fully collected under max_response_bytes. When collection hits the client-wide max_response_bytes first, praxis-core read_body fails, execute_callout maps CalloutResult::Rejected to CalloutFailed, and resolve_error_response returns 502. Example: max_body_bytes=64 with a 70 MiB download and absent or understated metadata.bytes collects until the client-wide cap, then returns 502 instead of 413. get_bytes_client_wide_limit_rejects_oversized_chunked_response encodes CalloutFailed for this path rather than ResponseTooLarge. Map client-wide size rejections to ResponseTooLarge (or propagate a distinct size error from execute_callout) before they reach the filter.
The get_bytes change to wrap execute_callout in tokio::time::timeout increased the async future size by ~600 bytes. This pushes sync_state_with_budget and resolve_state_history over clippy's 16 KiB stack-size threshold after merging the file_url feature. Box::pin the internal sync_message_history and sync_persisted_history calls to move those futures to the heap. Signed-off-by: Sébastien Han <seb@redhat.com>
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
Review: refactor(api_client): unify get_bytes behind CalloutClient
Clean, well-executed refactoring that eliminates the separate reqwest::Client and routes all outbound requests through CalloutClient. The unification adds circuit-breaker protection, callout-depth accounting, and max_response_bytes enforcement to content downloads, closing the gap documented in #454.
CI passes (21/21 checks). Test coverage is good: the new get_bytes_client_wide_limit_rejects_oversized_chunked_response test validates the client-wide limit, and the existing tests were correctly updated to use test_client_with_limits with explicit max_response_bytes values.
The Box::pin additions in file_resolve/mod.rs are necessary to keep async future sizes bounded after the callout client change increases the transitive future size of get_bytes.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
One medium behavioral observation noted inline. No blocking issues.
| let response = tokio::time::timeout(self.timeout, execute_callout(&self.client, request)) | ||
| .await | ||
| .map_err(|_elapsed| ApiClientError::CalloutFailed { | ||
| detail: "content download timed out".to_owned(), |
There was a problem hiding this comment.
[Medium] Behavioral change in memory bounding: the old read_bounded_body rejected responses chunk-by-chunk at the per-request max_bytes limit, so peak allocation was bounded to max_bytes. Now, CalloutClient::read_body collects the full response up to max_response_bytes before this post-collection check runs.
In openai_file_resolve, max_response_bytes is set to max_body_bytes (default 64 MiB) while max_content_bytes passed here from fetch_content can be much smaller (derived from the per-file resolution budget). A backend returning a response larger than the per-file budget but within 64 MiB will now be fully collected before rejection, whereas the old code would halt at the per-file limit.
The PR description and doc comments document this tradeoff clearly, so this is noted for the record rather than as a requested change. If per-file budgets are typically orders of magnitude smaller than max_body_bytes, a future optimization could set a tighter max_response_bytes on a per-download basis.
3 participants
Summary
Unifies
get_bytesbehindCalloutClientinstead of a separatereqwest::Client, completing the work unblocked by the praxis-core 0.4.1 bump (#486). Content downloads now share circuit-breaker, callout-depth protection, and bounded reads viaCalloutConfig::max_response_bytes. A per-requestmax_bytespost-collection check preservesResponseTooLarge→ 413 for file-level budgets.content_clientfield andread_bounded_bodyfromApiClientget_bytesthroughCalloutClientwithtokio::time::timeoutwrapping the full operationmax_response_bytesto the filter'smax_body_bytesinopenai_file_resolvemax_response_bytesrejectionCloses #454