cloudflare / cloudflare/pingora

HTTP/1.1 upstream keep-alive lost when following 3xx via `response_filter` error + outer retry loop

Open
#866 1 comment 0 reactions 0 assignees View on GitHub
bug
Dominant language
Rust
Stars
27.4k
Forks
1.7k
Avg merge
6h 22m
Merged PRs (30d)
3

Description

## Describe the bug

When user code implements “redirect follow” for an HTTP/1.1 upstream by returning `Err` from `ProxyHttp::response_filter` after seeing a 3xx (e.g. with `Location`), and relies on the outer `process_request` retry loop (`while retries < max_retries` → `proxy_to_upstream`) with `error.retry == true`, the upstream **keep-alive connection is not returned to the pool** for reuse.

A 3xx with a body is not a transport failure; correct H1 behavior is to fully consume the redirect response on the same TCP connection, then issue the follow-up request on that connection when host/port matches (or after resolving a relative `Location` on the same origin). Today, erroring out of `response_filter` causes `proxy_1to1`’s `try_join!` to fail, yielding `client_reuse = false`, so `release_http_session` is not called for the H1 session and the next retry opens a **new** upstream connection. Under high RPS this hurts latency and pool efficiency.

Relevant code: `pingora-proxy/src/proxy_h1.rs` (`h1_response_filter` → `response_filter`, `proxy_1to1` / `try_join!`), `pingora-proxy/src/lib.rs` (`proxy_to_upstream` and the `client_reuse` branch that calls `Connector::release_http_session`).

## Pingora info

Please include the following information about your environment:

**Pingora version**: e.g. `0.8.x` or commit hash `________________`
**Rust version**: output of `rustc --version` / `cargo --version`
**Operating system version**: e.g. Ubuntu 22.04, Debian 12.4, macOS 15.x

## Steps to reproduce

1. Run an origin (or mock) that responds to `GET /a` with `302 Found`, `Location: /b`, and optionally a small body; `GET /b` returns `200 OK`.
2. Implement a `ProxyHttp` that in `response_filter` detects the 3xx + `Location`, returns `Err` with retry enabled (per your usual pattern) so `process_request` runs `proxy_to_upstream` again on the next iteration.
3. Observe upstream connection behavior (e.g. new TCP connect per logical client request, pool miss, or tracing in the connector).

**Example (conceptual)** — adapt to your crate:

```rust
// In response_filter: on 3xx with Location, trigger outer retry
if upstream_response.status.is_redirection() {
// Build error with retry = true; user may also mutate session for next try
return Err(/* Error with retry enabled */);
}
```

## Expected results

For the same logical downstream request, when policy allows following the redirect on the same origin: the 3xx response body (if any) is fully read on the **same** H1 connection, then the follow-up request is sent on that connection; the connection remains eligible for pooling via `release_http_session` when still healthy.

Alternatively, the framework offers a supported first-class path (API or internal loop) for “same-connection redirect follow” without treating it as a fatal proxy error that drops `client_reuse`.

## Observed results

After `response_filter` returns `Err`, the H1 path does not complete with `client_reuse == true` for that session, so the upstream session is not released to the pool for keep-alive reuse. The retry iteration typically uses a new upstream connection.

## Additional context

- **Scope**: HTTP/1.1 to origin only; HTTP/2 upstream is out of scope for this report unless you expand it.
- **Security**: Cross-origin redirects should remain under explicit user policy; this issue is about connection reuse semantics, not blind following.
- **RFC-ish constraint**: Do not reuse the connection if the 3xx body was not fully consumed (unless the connection is closing anyway).
- **Workarounds**: Any approach that avoids `Err` from `response_filter` for this path, or avoids the outer retry for an “internal” redirect, may behave differently; document any workaround you use.
- **Optional**: Link to a PR that adds docs (e.g. `ProxyHttp::response_filter` rustdoc) or a full fix (internal `proxy_1to1` loop / new API).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.