fix(proxy): box forward errors for clippy on Rust 1.98 - #417
Conversation
There was a problem hiding this comment.
Code Review
This pull request wraps ForwardError and Response in Box within Result types across src/codex_endpoint.rs, src/http_tuning.rs, and src/proxy/failover.rs to address a clippy lint for large error types. The review feedback recommends boxing the large response field inside the ForwardError struct itself rather than boxing the entire struct. This alternative approach resolves the lint at its source, eliminating the need for widespread signature changes and boilerplate code.
Greptile SummaryThis PR boxes large HTTP and forwarding error values to satisfy Rust 1.98's
Confidence Score: 5/5The PR appears safe to merge, with no actionable behavioral, security, or compatibility defects identified. The changes consistently add heap indirection around existing error values, and every affected caller preserves the original status, response body, logging, and protocol-specific error handling.
|
| Filename | Overview |
|---|---|
| src/codex_endpoint.rs | Boxes forwarding errors while preserving Codex authentication, body-limit handling, logging, and OpenAI error shaping. |
| src/http_tuning.rs | Boxes body-read error responses and updates focused tests to consume the boxed response. |
| src/proxy/failover.rs | Propagates boxed forwarding errors through routing, authentication, failover, and token-count paths without changing their decisions or responses. |
Reviews (1): Last reviewed commit: "fix(http): box read_body error responses..." | Re-trigger Greptile
There was a problem hiding this comment.
No issues found across 3 files
Architecture diagram
sequenceDiagram
participant Client as Client
participant Proxy as Proxy Service (Failover/Codex)
participant Tuning as HTTP Tuning
participant Router as Routing Engine
participant Upstream as Upstream Provider (e.g. OpenAI)
Note over Client,Upstream: Request Inbound & Validation Flow
Client->>Proxy: POST Request (Body, Headers)
Proxy->>Tuning: content_length_exceeds(limit)
opt NEW: Length Exceeded
Tuning-->>Proxy: NEW: Err(Box<ForwardError>)
end
Proxy->>Tuning: read_body(body, limit)
alt Body Processing
Tuning->>Tuning: to_bytes(body)
Tuning-->>Proxy: Ok(Bytes)
else NEW: Body Error / Too Large
Tuning-->>Proxy: NEW: Err(Box<Response>)
end
Proxy->>Router: resolve_request_chain_value(json)
alt Routing Success
Router-->>Proxy: routes list + model
else Routing Error
Router-->>Proxy: NEW: Err(Box<ForwardError>)
end
Note over Proxy,Upstream: Forwarding & Failover Loop
loop For each route in chain
Proxy->>Upstream: Forward request to provider
alt Upstream Success (200 OK)
Upstream-->>Proxy: Response
Proxy-->>Client: Result<(StatusCode, Response)>
else Upstream Failure (Retriable)
Upstream-->>Proxy: 429 / 5xx Error
Proxy->>Proxy: Attempt next route
else NEW: Final Failure Path
Upstream-->>Proxy: Final Error
Proxy->>Proxy: NEW: Box::new(ForwardError)
Proxy-->>Client: NEW: Err(Box<ForwardError>)
end
end
Note over Proxy,Upstream: Token Counting Flow (Special Case)
opt count_tokens requested
Proxy->>Upstream: count_tokens_response()
alt NEW: Token Count Error
Upstream-->>Proxy: Error
Proxy-->>Client: NEW: Err(Box<ForwardError>)
end
end
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merging this PR will improve performance by 12.33%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | arc_stored_continuation_retrieve[1048576] |
275.8 ns | 245.6 ns | +12.33% |
| ⚡ | arc_stored_continuation_retrieve[307200] |
275.8 ns | 245.6 ns | +12.33% |
| ⚡ | arc_stored_continuation_retrieve[3145728] |
275.8 ns | 245.6 ns | +12.33% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing r-uben:fix/forward-error-clippy (84fe2da) with main (ecf5664)
|
Thanks for catching this and for the fix, @r-uben — the CI breakage was real and you diagnosed it correctly. Unfortunately this one got overtaken: while it was open, #418 landed the same repair from the other direction and is already on pub(crate) struct ForwardError {
message: String,
response: Box<axum::response::Response>,
}That mirrors the existing
Really appreciate you unblocking this; hope to see you in the next one. 🙏 |
|
No argument here — #418 is the better fix. Boxing inside the struct keeps every |
Summary
ForwardErroracross proxy failover, token-count, and Codex forwarding paths soclippy::result_large_errpasses on Rust 1.98 CI.read_bodyerror responses for the same lint inhttp_tuning.rs.Unblocks CI on PRs #370, #410, #411, #414, and #415.
Test plan
cargo fmt --all --checkcargo clippy --all-targets --all-features -- -D warnings(Rust 1.98)cargo test --all-features --workspaceMade with Cursor
Summary by cubic
Boxes
ForwardErrorandResponsein proxy failover, Codex forwarding, andhttp_tuningto satisfyclippy::result_large_erron Rust 1.98 and restore CI. Previously these functions returned large error types by value; now they return boxed errors. HTTP behavior and metrics remain unchanged.proxy/failover.rs(forward,count_tokens_response),codex_endpoint.rs(forward), andhttp_tuning::read_bodynow return boxed errors and that call sites deref only in tests and response mapping.Box<ForwardError>andBox<Response>(tests updated to deref when asserting).Written for commit 84fe2da. Summary will update on new commits.