cloudflare / cloudflare/pingora

pingora-proxy retries non-idempotent requests to h2 upstreams on InvalidH2 (unconditional retry; h1 path uses ReusedOnly)

Open
#979 0 comments 0 reactions 1 assignee Claimed by @andrewhavck View on GitHub
Accepted bug
Dominant language
Rust
Stars
27.4k
Forks
1.7k
Avg merge
6h 22m
Merged PRs (30d)
3

Description

## Summary

`pingora-proxy` retries a request to an HTTP/2 upstream **unconditionally** when
the response cannot be read because the origin sent an invalid HTTP/2 response
(`InvalidH2`, a library-generated GOAWAY with `PROTOCOL_ERROR`). This includes
non-idempotent methods (POST/PUT/PATCH) whose request body was already fully
written to the origin and may already have been processed there, so a single
client request can be executed twice at the origin.

This conflicts with RFC 9110 §9.2.2 (a proxy MUST NOT automatically retry a
non-idempotent request that may have been processed) and with this repository's
own guidance in `docs/user_guide/failover.md` ("POST ... are not safe to retry
if the requests have already been sent").

The HTTP/1.1 upstream path already implements the safe pattern: response-read
errors get `RetryType::ReusedOnly` (`pingora-core/src/protocols/http/v1/client.rs`),
i.e. the request is only retried when the connection came from the pool and the
request provably was not sent. The h2 `InvalidH2` path is the only one that
retries a certainly-sent request, and it does so with no method check and no
operator-visible switch.

## Affected code

`pingora-core/src/protocols/http/v2/client.rs`, `handle_read_header_error`
(still present on `main` as of 2026-08-26; reproduced on a build from
`0046038`, h2 crate 0.4.18):

```rust
} else if e.is_go_away() && e.is_library() && (e.reason() == Some(h2::Reason::PROTOCOL_ERROR)) {
// remote send invalid H2 responses
let mut err = Error::because(InvalidH2, "while reading h2 header", e);
err.retry = true.into(); // <-- unconditional: no method check, no sent-ness check
err
```

The error is raised **while reading the response headers**, i.e. after the
request was fully written to the origin. `pingora-proxy/src/lib.rs:348-356`
then treats `InvalidH2` as a signal to downgrade the peer to h1
(`prefer_h1`, gated only on the peer's ALPN allowing h1) and the upstream
retry loop (`max_retries`, default 16) replays the request.

## Repro (self-contained, all local)

Proxy — cargo project with path dependencies on this repo
(`pingora-core` with the `rustls` feature):

```rust
use async_trait::async_trait;
use pingora_core::protocols::tls::ALPN;
use pingora_core::server::Server;
use pingora_core::upstreams::peer::HttpPeer;
use pingora_error::Result;
use pingora_proxy::{http_proxy_service, ProxyHttp, Session};

struct Cpk;

#[async_trait]
impl ProxyHttp for Cpk {
type CTX = ();
fn new_ctx(&self) -> Self::CTX { () }

async fn upstream_peer(&self, _s: &mut Session, _c: &mut Self::CTX) -> Result> {
let mut peer = Box::new(HttpPeer::new("127.0.0.1:9443", true, "localhost".to_string()));
peer.options.verify_cert = false;
peer.options.verify_hostname = false;
peer.options.alpn = ALPN::H2H1; // documented h2-upstream usage; h1 fallback allowed
Ok(peer)
}
}

fn main() {
let mut server = Server::new(None).unwrap(); // default configuration: max_retries = 16
server.bootstrap();
let mut svc = http_proxy_service(&server.configuration, Cpk);
svc.add_tcp("0.0.0.0:9080");
server.add_service(svc);
server.run_forever();
}
```

Origin — a ~80-line TLS server on 127.0.0.1:9443 with ALPN `["h2","http/1.1"]`
that speaks raw HTTP/2 frames. On the first h2 connection it reads the client
preface + SETTINGS, reads the request HEADERS frame, then sends a DATA frame
on an idle stream (stream 3). This is an invalid h2 response — the case the
comment at `pingora-proxy/src/lib.rs:347` already anticipates ("origin h2 is
not production ready") — and it makes the client-side h2 library raise the
library-generated GOAWAY `PROTOCOL_ERROR` that hits the `InvalidH2` arm above.
On any h1 connection the same origin just logs the request and answers 200.

Driver:

```
curl -X POST http://127.0.0.1:9080/order -d "item=1&qty=1"
```

Observed origin log (verbatim):

```
conn=1 proto=h2 event=request_headers stream=1
conn=1 proto=h2 event=sent_invalid_data_frame stream=3
conn=2 proto=h1 event=request method=POST path=/order alpn=http/1.1 body_len=12
```

The single POST reaches the origin **twice** — once over h2, once replayed
over h1 after the downgrade. The client receives a transparent 200. A control
GET issued after the downgrade arrives exactly once (h1, `ReusedOnly`
behaviour on that path), confirming the defect is specific to the h2 error
path.

## Why this matters

For any application fronted by `pingora-proxy` with an HTTP/2 upstream, one
client request can be executed twice at the origin whenever the origin's h2
response is invalid: duplicate orders, payments, votes, notifications,
inventory deductions, coupon redemptions, etc. The trigger does not require
operator action and cannot be disabled — the retry flag is hardcoded inside
`handle_read_header_error`, and `failover.md` documents the safety rule only
for user hooks (`e.set_retry(true)`), so an operator who follows the
documentation and never retries POSTs still gets the library's replay.

## Related observations (for consideration, not part of the bug claim)

- The remote `GOAWAY(NO_ERROR)` arm in the same function only errors streams
with `stream id > last_stream_id`; per RFC 9113 §8.7 those streams were not
processed, so that retry is semantically safe for execution purposes. It
still re-sends non-idempotent bodies, so a method guard there would be
consistent with `failover.md`.
- The `HTTP_1_1_REQUIRED` (`H2Downgrade`) arm is sent by the server instead
of processing the request, so it is retry-safe in practice; same optional
consideration applies.

## Suggested fix

- Mark the `InvalidH2` retry as `RetryType::ReusedOnly`, mirroring the h1
path; or
- gate the retry on an idempotent-method check (`RequestHeader.method`),
with an explicit opt-in for operators who know their origin — consistent
with nginx's default of not retrying non-idempotent methods
(`proxy_next_upstream` / `non_idempotent`).

## Environment

pingora 0.8.0 built from `main` at `0046038` (2026-08-23), h2 crate 0.4.18,
rustls backend, single machine. All tests ran locally against a self-hosted
proxy and a fake origin — no third-party systems, no production traffic.

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.