Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical request-capacity and cancellation issues, plus a moderate pipelining timeout issue, block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves proxy-client forwarding with concurrent HTTP/2 handling, HTTP/1.1 reconnects, deadlines, cancellation, and configurable limits.
Changes:
- Adds configurable in-flight request limits and timeouts.
- Implements request-body cancellation and response idle tracking.
- Adds concurrency, reconnect, timeout tests, CLI options, and documentation.
File summaries
| File | Summary |
|---|---|
src/main.rs |
Adds client configuration flags. |
src/lib.rs |
Integrates concurrency and reconnect logic. Critical: queued or timed-out requests can retain permits; oversized semaphore limits can panic. |
src/http_version.rs |
Supports the new request body abstraction. |
src/client_request/upload.rs |
Provides cancellable upload handling. Critical: canceled uploads can retain semaphore permits. |
src/client_request/tests.rs |
Tests concurrency, cancellation, timeouts, and streaming. |
src/client_request/response_idle.rs |
Tracks response write activity. Moderate: shared response state can mishandle pipelined responses. |
src/client_request/mod.rs |
Implements forwarding, deadlines, and capacity tracking. Critical: dropped bodies or canceled uploads can retain request slots. |
README.md |
Documents the new configuration options. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| _ = tokio::time::sleep_until(deadline), if !uploaded => { | ||
| // Headers may already have reached the caller, so a 504 can no | ||
| // longer replace them. Cancel the upload instead. | ||
| drop(upload_guard); | ||
| return ForwardResult { reconnect: http1 || sender.is_closed(), sender }; |
There was a problem hiding this comment.
I have a fix for this in a separate branch, but it ended up being quite a bit of extra code for something debatably quite edge-casey. Im not sure if i want to merge it here.
There was a problem hiding this comment.
Ok in the end i decided this was worth fixing, even though it adds quite an extra maintenance burden. The fix involves adding a custom h2 adapter, meaning we do quite some extra work that was previously done by hyper.
| requests_tx | ||
| .send(PendingRequest { | ||
| request: req, | ||
| response_tx, | ||
| deadline, | ||
| permit, | ||
| }) |
| impl ResponseActivity { | ||
| /// Starts idle tracking and wraps the response body to observe completion. | ||
| pub(crate) fn track(&self, response: ProxyResponse) -> ProxyResponse { | ||
| self.0.send_replace(Some(ActiveResponse { | ||
| last_write: Instant::now(), | ||
| body_finished: false, | ||
| })); | ||
| response.map(|inner| { | ||
| let mut body = IdleBody { | ||
| inner, | ||
| activity: Some(self.clone()), | ||
| }; | ||
| if body.inner.is_end_stream() { | ||
| body.finish(); | ||
| } | ||
| body.boxed() | ||
| }) |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved correctness and configuration-validation issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (4)
PendingRequestis sent intorequests_tx, a source-side timeout only drops…upload_guardonly removes theIncomingfromUploadState; the…
Resolved since last review (3)
| if let Some(connection) = headers.remove(http::header::CONNECTION) | ||
| && let Ok(names) = connection.to_str() | ||
| { | ||
| for name in names.split(',') { | ||
| headers.remove(name.trim()); | ||
| } | ||
| } |
This improves concurrency of proxy-client to proxy-server connections.
On HTTP2 (default) requests run concurrently. proxy-server to target service connections still use HTTP1.1 - since we dont use TLS for these and therefore no protocol negotiation. This means we end up with multiple connections between proxy-server and target (usually the same host), multiplexed over a single attested connection over the wire.
On HTTP1.1, requests remain serialized but this PR adds deadlines, cancellation and reconnects.
Limits are configurable: 64 in-flight requests, 60-second request deadlines, and 60-second response idle timeouts, through CLI flags and
ProxyClientOptions.Closes #118