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

[CorsenAI]

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):

} 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):

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<Box<HttpPeer>> {
        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.