From d4066a7068e33b84025e8feb65ccee01afc82793 Mon Sep 17 00:00:00 2001 From: mwfj Date: Sun, 10 May 2026 20:21:04 +0800 Subject: [PATCH 01/14] Support http2 upstream phase2 --- .github/workflows/weekly-valgrind.yml | 1 + docs/http2_upstream.md | 2 + include/upstream/proxy_transaction.h | 50 + include/upstream/upstream_h2_connection.h | 7 +- include/upstream/upstream_h2_stream.h | 14 + include/upstream/upstream_response_sink.h | 17 + server/proxy_transaction.cc | 235 ++- server/upstream_h2_connection.cc | 132 +- test/h2_upstream_test.h | 1579 +++++++++++++++++++++ 9 files changed, 2010 insertions(+), 27 deletions(-) diff --git a/.github/workflows/weekly-valgrind.yml b/.github/workflows/weekly-valgrind.yml index 70359e0d..0b79066e 100644 --- a/.github/workflows/weekly-valgrind.yml +++ b/.github/workflows/weekly-valgrind.yml @@ -71,6 +71,7 @@ jobs: cli \ route \ upstream \ + h2_upstream \ proxy \ rate_limit \ circuit_breaker \ diff --git a/docs/http2_upstream.md b/docs/http2_upstream.md index bda0348a..d6ef2f90 100644 --- a/docs/http2_upstream.md +++ b/docs/http2_upstream.md @@ -138,3 +138,5 @@ The defaults are conservative and work for most deployments. Tune only if you ob - **No mid-stream SETTINGS update** — reloads apply to NEW connections only. Existing sessions keep their construction-time settings. - **One H2 connection per upstream per dispatcher** — until saturation routing lands (`saturation_open_pct`), each partition holds one multiplexed connection per upstream. For very-high-fanout workloads this can be a bottleneck; mitigate by increasing the dispatcher count. - **Per-stream backpressure is not strictly bounded by `initial_window_size`** — the proxy lets nghttp2 manage stream-level flow control with auto-`WINDOW_UPDATE` enabled, so the upstream's effective window is continuously refreshed as bytes are delivered to the on-data-chunk callback. In practice per-stream upstream buffering tracks the auto-update cadence (~`initial_window_size` worth of bytes outstanding under steady traffic) plus the `StreamingResponseSender` high-water mark on the downstream side, but it is not a hard cap: a slow downstream client paired with a fast H2 upstream can buffer somewhat more depending on `MAX_FRAME_SIZE` and how quickly chunks are read. For workloads with bursty downstream stalls and a high `initial_window_size`, watch RSS and consider lowering the window size. A future refinement will disable auto-update and pause per-stream consumption via `nghttp2_session_consume_stream` to enforce a strict cap. +- **CONNECT method is rejected** with 502 + `X-H2-Limitation: connect-not-supported`. The H2 codec emits `:scheme` and `:path` on every request, which RFC 9113 §8.5 forbids on CONNECT pseudo-headers; rather than emit a malformed request, the gateway rejects deterministically. Use an H1 upstream for CONNECT tunnelling. +- **Truncation observability** — when a backend declares `Content-Length` and closes early, nghttp2's HTTP messaging enforcement detects the violation and the gateway surfaces it as `RESULT_UPSTREAM_DISCONNECT` (the same bucket as a torn TCP connection). A dedicated `RESULT_TRUNCATED_RESPONSE` code exists in the binary for defense-in-depth but is not normally observable in production. If you need to distinguish "peer reset / TCP drop" from "Content-Length short read", correlate by upstream-side response logs. diff --git a/include/upstream/proxy_transaction.h b/include/upstream/proxy_transaction.h index 2245511e..a8a9b0ed 100644 --- a/include/upstream/proxy_transaction.h +++ b/include/upstream/proxy_transaction.h @@ -53,6 +53,19 @@ class ProxyTransaction // X-Retry-Budget-Exhausted so operators can tell the two 503s apart // from circuit-open rejects. static constexpr int RESULT_RETRY_BUDGET_EXHAUSTED = -8; + // Upstream response did not match its declared length. Two cases: + // - Content-Length declared, peer delivered fewer bytes before clean + // close, or more bytes than declared. + // - Response classified NO_BODY (status 204/304 or HEAD method) but + // peer sent body bytes anyway. + // Terminal — partial body has already been streamed downstream so retry + // would double-deliver bytes. Maps to 502 BadGateway in MakeErrorResponse. + static constexpr int RESULT_TRUNCATED_RESPONSE = -10; + // CONNECT method on an H2 upstream. RFC 9113 §8.5 forbids :scheme and + // :path on CONNECT pseudo-headers, but our H2 codec always emits both; + // serving CONNECT here would emit a malformed request. Terminal — + // deterministic policy reject (502 BadGateway + X-H2-Limitation header). + static constexpr int RESULT_H2_METHOD_NOT_SUPPORTED = -11; // Constructor copies all needed fields from client_request (method, path, // query, headers, body, params, dispatcher_index, client_ip, client_tls, @@ -137,8 +150,17 @@ class ProxyTransaction const std::vector>& trailers) override; void OnComplete() override; void OnError(int error_code, const std::string& message) override; + void OnRequestSubmitted() override; private: + // Send-phase stall fallback budget when config_.response_timeout_ms == 0. + // The response-wait timeout is operator-disable-able (set to 0), but the + // stall-phase hang protection is always on — without it a wedged upstream + // that stops reading our request body would pin both the client and the + // pooled connection indefinitely. Used by both the H1 send loop and the + // H2 send-stall closure. + static constexpr int SEND_STALL_FALLBACK_MS = 30000; // 30s + // State machine states enum class State { INIT, // Created, not yet started @@ -290,6 +312,34 @@ class ProxyTransaction int32_t h2_stream_id_ = -1; bool h2_path_ = false; + // True iff the inbound request carried `te: trailers` (RFC 7230 §4.3 + // / RFC 9113 §8.2.2 — required by gRPC clients to negotiate trailer + // support). Captured at construction BEFORE HeaderRewriter strips + // all te values per RFC 7230 hop-by-hop rules. The H2 outbound nv + // build re-emits a synthetic `te: trailers` based on this flag; H1 + // path is unchanged (rewriter strips, no re-emit). + bool client_te_trailers_ = false; + + // H2 send-stall generation counter. Bounds the time spent in + // SENDING_REQUEST waiting for END_STREAM to flush — without this, a + // wedged peer that stops reading our DATA frames would pin the H2 + // stream until the peer's PING timeout (or forever, if PING is + // disabled). Armed BEFORE SubmitRequest so the synchronous + // on_frame_send_callback path (bodyless requests where nghttp2 + // inline-flushes HEADERS+END_STREAM) can kill it via generation + // bump. Cleanup also bumps to invalidate any in-flight closure. + uint64_t h2_send_stall_generation_ = 0; + + // H2 response-timeout arm-once flag. Coordinates the response timer + // arming between OnHeaders and OnRequestSubmitted: whichever fires + // first arms ArmResponseTimeout and sets this flag, and the other + // skips re-arming. Required because the existing H1 OnHeaders path + // calls ClearResponseTimeout (semantic doesn't apply to H2's + // two-deadline model) and DispatchH2 cannot arm response-timeout + // upfront without leaking the budget into the body-write phase. + // Reset by Cleanup so retry attempts arm fresh. + bool h2_response_timeout_armed_ = false; + // H2 response timeout uses a dispatcher-scheduled task instead of a // transport-level deadline: the transport is shared across every // stream on the multiplexed session, so SetDeadline would tear down diff --git a/include/upstream/upstream_h2_connection.h b/include/upstream/upstream_h2_connection.h index e299ad2a..89bc1619 100644 --- a/include/upstream/upstream_h2_connection.h +++ b/include/upstream/upstream_h2_connection.h @@ -90,6 +90,10 @@ class UpstreamH2Connection { // Submit an outbound HTTP request as a new H2 stream. Returns the // nghttp2 stream_id on success (>= 1), or -1 on submit failure or // when this connection is no longer usable. + // + // `client_te_trailers`: if true, the nv-array build appends a + // synthetic `te: trailers` entry after the rewriter's strip pass. + // Defaulted false to keep existing test callers compiling unchanged. int32_t SubmitRequest( const std::string& method, const std::string& scheme, @@ -97,7 +101,8 @@ class UpstreamH2Connection { const std::string& path, const std::map& headers, const std::string& body, - UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseSink* sink); + UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseSink* sink, + bool client_te_trailers = false); // Cancel an in-flight stream. Submits RST_STREAM with NGHTTP2_CANCEL // and flushes; defers the flush when called from inside HandleBytes diff --git a/include/upstream/upstream_h2_stream.h b/include/upstream/upstream_h2_stream.h index 02904ac1..f15a2e08 100644 --- a/include/upstream/upstream_h2_stream.h +++ b/include/upstream/upstream_h2_stream.h @@ -45,6 +45,20 @@ struct UpstreamH2Stream { // read_callback. Empty for bodyless requests. std::unique_ptr body_source; + // Set at submit time so frame callbacks can detect HEAD-on-NO_BODY + // (RFC 9110 §9.3.2) without reaching back into the codec or proxy + // transaction. Populated between make_shared and the streams_ insert + // in UpstreamH2Connection::SubmitRequest — race-free because frame + // callbacks for the new stream cannot fire until peer bytes arrive + // via HandleBytes. + std::string request_method; + + // Cumulative response body bytes delivered to the sink (or rejected + // synchronously by Step 1.5 NO_BODY/CL validation). Validated against + // response_head.expected_length on each chunk and at clean close to + // catch peers that lie about Content-Length. + int64_t body_bytes_received = 0; + UpstreamH2Stream() = default; UpstreamH2Stream(const UpstreamH2Stream&) = delete; UpstreamH2Stream& operator=(const UpstreamH2Stream&) = delete; diff --git a/include/upstream/upstream_response_sink.h b/include/upstream/upstream_response_sink.h index 3e2c76d4..c9e679f8 100644 --- a/include/upstream/upstream_response_sink.h +++ b/include/upstream/upstream_response_sink.h @@ -19,6 +19,23 @@ class UpstreamResponseSink { const std::vector>&) {} virtual void OnComplete() = 0; virtual void OnError(int error_code, const std::string& message) = 0; + + // Fired when the upstream stream's request side is fully sent + // (END_STREAM flag observed on either DATA or HEADERS — bodyless + // requests trigger END_STREAM on HEADERS). Sinks use this to swap + // a per-stream send-stall deadline for a response-completion + // deadline. + // + // H2 path only. The H1 path infers send completion from socket + // buffer drain and does not call this. Default no-op so non-H2 + // sinks can ignore. + // + // Invocation timing: may fire SYNCHRONOUSLY from within + // UpstreamH2Connection::SubmitRequest for bodyless requests where + // nghttp2 inline-flushes the HEADERS+END_STREAM frame. Sinks MUST + // be safe to receive this callback before SubmitRequest returns + // control to the caller. + virtual void OnRequestSubmitted() {} }; } // namespace UPSTREAM_CALLBACKS_NAMESPACE diff --git a/server/proxy_transaction.cc b/server/proxy_transaction.cc index ef13a655..05144a98 100644 --- a/server/proxy_transaction.cc +++ b/server/proxy_transaction.cc @@ -246,6 +246,46 @@ ProxyTransaction::ProxyTransaction( stream_sender_(std::move(stream_sender)) { stream_sender_.ConfigureWatermarks(config_.relay_buffer_limit_bytes); + + // Capture client `te: trailers` BEFORE HeaderRewriter::RewriteRequest + // strips all te values per RFC 7230 §4.3 hop-by-hop rules. gRPC clients + // send `te: trailers` to negotiate trailer support; the H2 outbound nv + // build re-emits the token from this flag (RFC 9113 §8.2.2). Inline + // tokenizer instead of std::tolower because the latter is locale- + // dependent (Turkish locale lowercases 'I' to 'ı', producing non-ASCII + // bytes). client_headers_ keys are guaranteed lowercase by HttpParser. + auto te_it = client_headers_.find("te"); + if (te_it != client_headers_.end()) { + std::string buf; + buf.reserve(te_it->second.size()); + for (char c : te_it->second) { + if (c >= 'A' && c <= 'Z') c = static_cast(c | 0x20); + buf.push_back(c); + } + size_t pos = 0; + while (pos < buf.size()) { + size_t comma = buf.find(',', pos); + size_t end = (comma == std::string::npos) ? buf.size() : comma; + size_t token_start = pos; + size_t token_end = end; + // Trim ASCII whitespace per RFC 7230 OWS (SP and HTAB only). + while (token_start < token_end && + (buf[token_start] == ' ' || buf[token_start] == '\t')) { + ++token_start; + } + while (token_end > token_start && + (buf[token_end - 1] == ' ' || buf[token_end - 1] == '\t')) { + --token_end; + } + if (buf.compare(token_start, token_end - token_start, + "trailers") == 0) { + client_te_trailers_ = true; + break; + } + pos = (comma == std::string::npos) ? buf.size() : comma + 1; + } + } + logging::Get()->debug("ProxyTransaction created client_fd={} service={} " "{} {}", client_fd_, service_name_, method_, path_); } @@ -801,6 +841,24 @@ void ProxyTransaction::DispatchH1() { } void ProxyTransaction::DispatchH2() { + // Primary CONNECT-rejection gate. RFC 9113 §8.5 forbids :scheme and + // :path on CONNECT pseudo-headers, but our H2 codec always emits + // both — serving CONNECT through this path would emit a malformed + // request. Reject deterministically with 502 + X-H2-Limitation + // header. The neutral breaker release is run BEFORE OnError so the + // admission slot doesn't leak when OnError's pre-routing hook (which + // is idempotent) double-handles the same code. + if (method_ == "CONNECT") { + logging::Get()->warn( + "H2 upstream rejecting CONNECT: not supported in this gateway " + "fd={} service={}", + client_fd_, service_name_); + ReleaseBreakerAdmissionNeutral(); + OnError(RESULT_H2_METHOD_NOT_SUPPORTED, + "CONNECT not supported on H2 upstream"); + return; + } + PoolPartition* partition = nullptr; if (upstream_manager_ && dispatcher_index_ >= 0) { partition = upstream_manager_->GetPoolPartition( @@ -873,10 +931,60 @@ void ProxyTransaction::DispatchH2() { path_with_query.append(query_); } + // Initialize H2 state BEFORE h2->SubmitRequest. SubmitRequest may + // synchronously fire on_frame_send_callback (via FlushSend) for + // bodyless requests where nghttp2 inline-flushes HEADERS+END_STREAM, + // dispatching OnRequestSubmitted INSIDE the call. If we deferred + // h2_path_/state_/generation init to after the call, the + // OnRequestSubmitted override's `if (!h2_path_) return;` guard + // would silently drop the kill of the send-stall closure → spurious + // OnError(UPSTREAM_DISCONNECT) ~30s later. + // + // Send-stall budget mirrors the H1 zero-disable semantic in the + // H1 send loop above: response_timeout_ms == 0 opts out of the + // response-wait timer but the stall protection stays on, falling + // back to SEND_STALL_FALLBACK_MS. + h2_path_ = true; + h2_conn_weak_ = h2; + state_ = State::SENDING_REQUEST; + h2_response_timeout_armed_ = false; + + const int stall_budget_ms = config_.response_timeout_ms > 0 + ? config_.response_timeout_ms + : SEND_STALL_FALLBACK_MS; + const uint64_t send_stall_gen = ++h2_send_stall_generation_; + if (dispatcher_) { + std::weak_ptr weak_self = weak_from_this(); + dispatcher_->EnQueueDelayed( + [weak_self, send_stall_gen]() { + auto self = weak_self.lock(); + if (!self) return; + if (self->cancelled_) return; + if (send_stall_gen != self->h2_send_stall_generation_) return; + self->OnError(RESULT_UPSTREAM_DISCONNECT, + "H2 send-stall (END_STREAM not flushed)"); + }, + std::chrono::milliseconds(stall_budget_ms)); + } + + // Synchronous on_frame_send (bodyless path) may run inline here, + // bumping h2_send_stall_generation_ → kills the closure above. int32_t stream_id = h2->SubmitRequest( method_, scheme, authority, path_with_query, - rewritten_headers_, request_body_, this); + rewritten_headers_, request_body_, this, client_te_trailers_); if (stream_id < 0) { + // Submit failed. Roll back H2 bookkeeping; state_ stays + // untouched — AttemptCheckout (called by MaybeRetry's + // deferred-retry timer and immediate-retry branch) resets + // state_ before the next attempt. Bump BOTH generations: + // send-stall closure was just queued; response-timeout + // closure may have been queued synchronously by an inline + // on_frame_send fire for a bodyless HEADERS+END_STREAM. + ++h2_send_stall_generation_; + ++h2_response_timeout_generation_; + h2_path_ = false; + h2_response_timeout_armed_ = false; + h2_conn_weak_.reset(); logging::Get()->warn( "ProxyTransaction H2 submit failed client_fd={} service={} " "attempt={}", client_fd_, service_name_, attempt_); @@ -884,11 +992,7 @@ void ProxyTransaction::DispatchH2() { return; } - h2_path_ = true; h2_stream_id_ = stream_id; - h2_conn_weak_ = h2; - state_ = State::AWAITING_RESPONSE; - ArmResponseTimeout(); } void ProxyTransaction::OnCheckoutError(int error_code) { @@ -1046,13 +1150,13 @@ void ProxyTransaction::SendUpstreamRequest() { // deadline never trips. // // The stall budget uses response_timeout_ms when configured, else - // a hardcoded fallback. Unlike the response-wait phase, the stall - // phase is ALWAYS protected — the refresh-on-progress callback - // prevents false positives on large uploads making steady progress, - // so using a fallback here doesn't penalize any legitimate traffic. - // Config "disabled" (response_timeout_ms == 0) opts out of the - // response-wait timeout, NOT the hang protection. - static constexpr int SEND_STALL_FALLBACK_MS = 30000; // 30s + // the class-level SEND_STALL_FALLBACK_MS fallback. Unlike the + // response-wait phase, the stall phase is ALWAYS protected — the + // refresh-on-progress callback prevents false positives on large + // uploads making steady progress, so using a fallback here doesn't + // penalize any legitimate traffic. Config "disabled" + // (response_timeout_ms == 0) opts out of the response-wait timeout, + // NOT the hang protection. const int stall_budget_ms = config_.response_timeout_ms > 0 ? config_.response_timeout_ms : SEND_STALL_FALLBACK_MS; @@ -1215,18 +1319,43 @@ bool ProxyTransaction::OnHeaders( if (!head.keep_alive) { poison_connection_ = true; } - if (state_ == State::SENDING_REQUEST) { - // Early response: the request write is no longer the active phase. If - // the upstream later finishes flushing the request bytes, that callback - // must not re-arm the response-header timer or move us back into the - // pre-headers state machine. - state_ = State::AWAITING_RESPONSE; - poison_connection_ = true; - } - // T1 is complete once the response head arrives. Body-phase timing uses - // the dedicated T2/T3 stream timers. - ClearResponseTimeout(); + if (h2_path_) { + // H2 final-headers branch. The send-stall closure stays armed + // until OnRequestSubmitted (or Cleanup) bumps the generation — + // an early peer-final-headers (e.g. 413) before our END_STREAM + // is sent does not cancel the stall protection. Response timer + // is armed at the FIRST of {OnHeaders, OnRequestSubmitted} via + // the arm-once flag; whichever fires later sees armed=true and + // skips the redundant arm. + // + // No poison_connection_=true here on the early-final-headers + // path: H1 poisons because its transport-sharing model + // contaminates subsequent keep-alive use, but H2 multiplexes + // streams over a single transport — an early 413 on one stream + // is not a fatal upstream signal and should not block sibling + // stream reuse. + if (state_ == State::SENDING_REQUEST) { + state_ = State::AWAITING_RESPONSE; + } + if (!h2_response_timeout_armed_) { + ArmResponseTimeout(); + h2_response_timeout_armed_ = true; + } + } else { + // H1 path: existing behavior preserved verbatim. + if (state_ == State::SENDING_REQUEST) { + // Early response: the request write is no longer the active + // phase. If the upstream later finishes flushing the request + // bytes, that callback must not re-arm the response-header + // timer or move us back into the pre-headers state machine. + state_ = State::AWAITING_RESPONSE; + poison_connection_ = true; + } + // T1 is complete once the response head arrives. Body-phase + // timing uses the dedicated T2/T3 stream timers. + ClearResponseTimeout(); + } if (head.status_code >= HttpStatus::INTERNAL_SERVER_ERROR && head.status_code < 600) { @@ -1451,6 +1580,15 @@ void ProxyTransaction::OnError(int result_code, const std::string& log_message) { if (cancelled_ || IsKilledForShutdown()) return; + // Centralized neutral breaker release for deterministic policy + // rejects. Idempotent — ReleaseBreakerAdmissionNeutral is a no-op + // when no admission is held. Runs BEFORE the H2 retryable- + // disconnect routing below so RESULT_H2_METHOD_NOT_SUPPORTED + // doesn't leak into MaybeRetry. + if (result_code == RESULT_H2_METHOD_NOT_SUPPORTED) { + ReleaseBreakerAdmissionNeutral(); + } + // H2 transport-level failures arrive here through sink->OnError — // unlike H1, which detects transport failure inside OnUpstreamData // and calls MaybeRetry(UPSTREAM_DISCONNECT) directly before the sink @@ -1477,6 +1615,35 @@ void ProxyTransaction::OnError(int result_code, DeliverTerminalError(result_code, log_message); } +void ProxyTransaction::OnRequestSubmitted() { + if (cancelled_ || IsKilledForShutdown()) return; + if (!h2_path_) return; // H1 infers send completion from socket drain + + // Unconditional send-stall kill — the in-flight closure no-ops on + // generation mismatch when its delayed task eventually fires. Both + // the normal path (END_STREAM flushed) and the early-headers path + // (peer responded before our END_STREAM, but END_STREAM eventually + // went out) clear send-stall via this generation bump. + ++h2_send_stall_generation_; + + // Transition state if not already advanced by an early-final- + // headers OnHeaders. Send-stall stayed armed across that early + // arrival; clearing it here is idempotent and safe. + if (state_ == State::SENDING_REQUEST) { + state_ = State::AWAITING_RESPONSE; + } + + // Arm response-timeout if OnHeaders hasn't already armed it. + // Whichever of {OnHeaders, OnRequestSubmitted} fires first arms + // the timer; the other sees armed=true and skips the redundant + // arm. Coordinated via h2_response_timeout_armed_ to avoid + // double-arming and the resulting deadline shift. + if (!h2_response_timeout_armed_) { + ArmResponseTimeout(); + h2_response_timeout_armed_ = true; + } +} + void ProxyTransaction::DeliverTerminalError(int result_code, const std::string& log_message) { if (cancelled_ || IsKilledForShutdown()) return; @@ -1918,6 +2085,10 @@ void ProxyTransaction::Cleanup() { } h2_stream_id_ = -1; h2_conn_weak_.reset(); + // Bump send-stall generation BEFORE the h2_path_ flip so any + // in-flight send-stall closure no-ops on its eventual fire. + // Same pattern as h2_response_timeout_generation_ below. + ++h2_send_stall_generation_; // ClearResponseTimeout MUST run while h2_path_ is still true: // its H2 branch keys on h2_path_ to bump // h2_response_timeout_generation_, which invalidates any queued @@ -1926,6 +2097,11 @@ void ProxyTransaction::Cleanup() { // possibly mid-retry on the H1 path or already destructed) and // produce a spurious RESPONSE_TIMEOUT against the wrong attempt. ClearResponseTimeout(); + // Reset the arm-once flag so a subsequent retry attempt that + // lands back on H2 arms response-timeout fresh (otherwise the + // first OnHeaders/OnRequestSubmitted would skip ArmResponseTimeout + // because the flag is left over from the prior attempt). + h2_response_timeout_armed_ = false; // Reset h2_path_ so a subsequent retry attempt that lands on H1 // (e.g. ALPN renegotiated, or the H2 connection died and prefer=auto's // next probe selects http/1.1) goes through the H1 lease-release @@ -2667,10 +2843,21 @@ HttpResponse ProxyTransaction::MakeErrorResponse(int result_code) { resp.Header("Connection", "close"); return resp; } + if (result_code == RESULT_H2_METHOD_NOT_SUPPORTED) { + // RFC 9113 §8.5: CONNECT pseudo-headers forbid :scheme and :path, + // but our H2 codec always emits both. Surface the limitation in + // a dedicated header so operators can detect the rejection + // without parsing the body. Self-identifying response analogous + // to X-Circuit-Breaker / X-Retry-Budget-Exhausted. + HttpResponse resp = HttpResponse::BadGateway(); + resp.Header("X-H2-Limitation", "connect-not-supported"); + return resp; + } if (result_code == RESULT_CHECKOUT_FAILED || result_code == RESULT_SEND_FAILED || result_code == RESULT_PARSE_ERROR || - result_code == RESULT_UPSTREAM_DISCONNECT) { + result_code == RESULT_UPSTREAM_DISCONNECT || + result_code == RESULT_TRUNCATED_RESPONSE) { return HttpResponse::BadGateway(); } return HttpResponse::InternalError(); diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index 66324387..05ee6ef4 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -208,10 +208,70 @@ int OnDataChunkRecvCallback(nghttp2_session* /*session*/, uint8_t /*flags*/, auto* self = static_cast(user_data); auto* stream = self->GetStream(stream_id); if (!stream || !stream->sink) return 0; + + // Defense-in-depth pre-dispatch validation. In production this code + // is unreachable: nghttp2's HTTP messaging enforcement (default-on) + // intercepts NO_BODY and Content-Length violations before this + // callback fires, routing them through OnStreamClose with a + // non-NO_ERROR code. These checks remain active as a backstop for + // future code paths that opt out of enforcement (e.g. + // nghttp2_option_set_no_http_messaging for streaming protocols). + // RST through ResetStream (not nghttp2_submit_rst_stream directly) so + // the in_receive_data_ guard defers the inline FlushSend; the + // post-receive flush in HandleBytes picks up the queued frame. + using Framing = UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead::Framing; + if (stream->response_head.framing == Framing::NO_BODY && len > 0) { + stream->sink->OnError( + ProxyTransaction::RESULT_TRUNCATED_RESPONSE, + "body bytes on NO_BODY response"); + self->ResetStream(stream_id); + return 0; + } + if (stream->response_head.framing == Framing::CONTENT_LENGTH && + stream->response_head.expected_length >= 0 && + static_cast(len) > + stream->response_head.expected_length - + stream->body_bytes_received) { + stream->sink->OnError( + ProxyTransaction::RESULT_TRUNCATED_RESPONSE, + "body exceeds Content-Length"); + self->ResetStream(stream_id); + return 0; + } + + stream->body_bytes_received += static_cast(len); stream->sink->OnBodyChunk(reinterpret_cast(data), len); return 0; } +// Fired when nghttp2 actually puts a frame on the wire (via +// FlushSend → nghttp2_session_mem_send2). For our purposes we care +// about request-side END_STREAM only — that flag rides on either the +// final HEADERS (bodyless requests) or the final DATA frame (bodied +// requests). The single check covers both shapes. +// +// May fire SYNCHRONOUSLY inside SubmitRequest when nghttp2 inline- +// flushes a bodyless HEADERS+END_STREAM frame; the sink contract +// (OnRequestSubmitted docstring on UpstreamResponseSink) requires +// callers to be safe under that ordering. ProxyTransaction handles +// the synchronous case by initializing all H2 state BEFORE invoking +// h2->SubmitRequest. +int OnFrameSendCallback(nghttp2_session* /*session*/, + const nghttp2_frame* frame, void* user_data) +{ + if (!frame) return 0; + if ((frame->hd.type != NGHTTP2_HEADERS && + frame->hd.type != NGHTTP2_DATA) || + !(frame->hd.flags & NGHTTP2_FLAG_END_STREAM)) { + return 0; + } + auto* self = static_cast(user_data); + auto* stream = self->GetStream(frame->hd.stream_id); + if (!stream || !stream->sink) return 0; + stream->sink->OnRequestSubmitted(); + return 0; +} + // RAII guard for the receive-reentrancy flag. Constructed on each // HandleBytes call; restored on scope exit so the flag tracks the // recursion edge cleanly even when nghttp2 callbacks throw. @@ -282,6 +342,8 @@ bool UpstreamH2Connection::Init() { nghttp2_session_callbacks_set_on_begin_headers_callback(cbs, &OnBeginHeadersCallback); nghttp2_session_callbacks_set_on_data_chunk_recv_callback( cbs, &OnDataChunkRecvCallback); + nghttp2_session_callbacks_set_on_frame_send_callback( + cbs, &OnFrameSendCallback); int rv = nghttp2_session_client_new(&session_, cbs, this); nghttp2_session_callbacks_del(cbs); @@ -478,8 +540,35 @@ void UpstreamH2Connection::OnStreamClose(int32_t stream_id, auto stream = it->second; streams_.erase(it); if (stream && stream->sink) { + using Framing = UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead::Framing; if (error_code == NGHTTP2_NO_ERROR) { - stream->sink->OnComplete(); + // Content-Length short-read: peer ended the stream cleanly + // but delivered fewer bytes than declared. Surface a + // truncation error in place of the OnComplete dispatch so + // downstream consumers see RESULT_TRUNCATED_RESPONSE rather + // than a successful response with a short body. + // + // In production this branch is also a defense-in-depth + // backstop: nghttp2's HTTP messaging enforcement + // (default-on) intercepts CL/NO_BODY violations and + // delivers them via the non-NO_ERROR fan-out above + // (OnDataChunkRecvCallback's Step 1.5 is a parallel + // backstop for the same enforcement-disabled future). + // The active value of THIS branch is the silent-short- + // close case where the peer respects framing on the wire + // but lies about Content-Length — neither nghttp2 nor + // Step 1.5 can detect that until the clean END_STREAM + // arrives short. + if (stream->response_head.framing == Framing::CONTENT_LENGTH && + stream->response_head.expected_length >= 0 && + stream->body_bytes_received < + stream->response_head.expected_length) { + stream->sink->OnError( + ProxyTransaction::RESULT_TRUNCATED_RESPONSE, + "Content-Length short read"); + } else { + stream->sink->OnComplete(); + } } else if (error_code == NGHTTP2_HTTP_1_1_REQUIRED) { // RFC 9113 §13 (error code 0xd): peer indicates THIS // request must be retried over HTTP/1.1. Retrying on H2 @@ -542,6 +631,14 @@ void UpstreamH2Connection::OnHeadersComplete(int32_t stream_id, using Framing = UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead::Framing; if (end_stream) { stream->response_head.framing = Framing::NO_BODY; + } else if (stream->response_head.status_code == 204 || + stream->response_head.status_code == 304 || + stream->request_method == "HEAD") { + // RFC 9110 §15.4 / §15.4.5 / §9.3.2: 204 / 304 / HEAD responses + // MUST NOT carry a body. Classify as NO_BODY so Step 1.5 in + // OnDataChunkRecvCallback rejects any subsequent body bytes from + // a misbehaving peer with RESULT_TRUNCATED_RESPONSE. + stream->response_head.framing = Framing::NO_BODY; } else { // Scan accumulated headers for content-length. Cap at the H1 // codec's MAX_RESPONSE_BODY_SIZE to defend against malicious or @@ -687,10 +784,32 @@ int32_t UpstreamH2Connection::SubmitRequest( const std::string& path, const std::map& headers, const std::string& body, - UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseSink* sink) + UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseSink* sink, + bool client_te_trailers) { if (!IsUsable()) return -1; + // Secondary CONNECT-rejection gate. The primary gate in + // ProxyTransaction::DispatchH2 catches CONNECT before it reaches the + // codec, but a future code path bypassing DispatchH2 (or a unit test + // exercising this method directly) would otherwise emit a malformed + // H2 request. nghttp2 is left untouched: no stream allocation, no + // submit, no frame queue. Sink receives the deterministic policy + // reject so the caller's MaybeRetry sees a terminal code. + if (method == "CONNECT") { + const std::string host = transport_ ? transport_->upstream_host() + : std::string("?"); + logging::Get()->warn( + "UpstreamH2Connection: rejecting CONNECT secondary gate " + "(primary gate bypassed?) host={}", host); + if (sink) { + sink->OnError( + ProxyTransaction::RESULT_H2_METHOD_NOT_SUPPORTED, + "CONNECT not supported on H2 upstream"); + } + return -1; + } + // Build lowercased header-name backing store FIRST so the nghttp2_nv // pointers we build next stay valid for the synchronous submit call. std::vector lower_names; @@ -743,8 +862,17 @@ int32_t UpstreamH2Connection::SubmitRequest( kv.second.data(), kv.second.size()); } + // Re-emit te: trailers after the rewriter's strip pass. RFC 9113 + // §8.2.2 permits exactly this token; gRPC clients require it for + // trailer support negotiation. Static literals have program-lifetime + // storage, safe to reference for the synchronous submit call. + if (client_te_trailers) { + push_nv("te", 2, "trailers", 8); + } + auto stream = std::make_shared(); stream->sink = sink; + stream->request_method = method; nghttp2_data_provider2 provider = {}; nghttp2_data_provider2* data_prd = nullptr; diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index 98290d9d..c0cef74f 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -2828,6 +2828,1551 @@ void TestC6ResetStreamSinkDetachSurvivesDtor() { } } +// --------------------------------------------------------------------------- +// TestN-series — H2 upstream negative / correctness tests covering: +// - truncation detection (CL short / overflow) and NO_BODY rejection +// - CONNECT method rejection (primary at DispatchH2 + secondary in +// UpstreamH2Connection::SubmitRequest) +// - te:trailers capture-before-strip and outbound re-emit +// - send-stall + response-timeout handoff via OnRequestSubmitted +// - sink invariants on natural close (no spurious RST_STREAM) +// --------------------------------------------------------------------------- + +// Helper: build an UpstreamH2Connection with null transport and a null-safe +// nghttp2 session. +static auto MakeH2Conn() { + auto cfg = std::make_shared(); + cfg->enabled = true; + cfg->max_concurrent_streams_pref = 10; + cfg->ping_idle_sec = 0; + cfg->ping_timeout_sec = 0; + cfg->goaway_drain_timeout_sec = 0; + return cfg; +} + +// Helper: build a DATA frame with a raw payload (no padding). +static std::vector BuildDataFrame(int32_t stream_id, + const uint8_t* data, size_t len, + bool end_stream) +{ + std::vector frame; + frame.reserve(9 + len); + frame.push_back(static_cast((len >> 16) & 0xff)); + frame.push_back(static_cast((len >> 8) & 0xff)); + frame.push_back(static_cast(len & 0xff)); + frame.push_back(NGHTTP2_DATA); + frame.push_back(end_stream ? NGHTTP2_FLAG_END_STREAM : 0); + frame.push_back(static_cast((stream_id >> 24) & 0x7f)); + frame.push_back(static_cast((stream_id >> 16) & 0xff)); + frame.push_back(static_cast((stream_id >> 8) & 0xff)); + frame.push_back(static_cast(stream_id & 0xff)); + frame.insert(frame.end(), data, data + len); + return frame; +} + +// Helper: build a RST_STREAM frame. +static std::vector BuildRstStreamFrame(int32_t stream_id, + uint32_t error_code) +{ + std::vector frame; + frame.reserve(13); + // length = 4 + frame.push_back(0); frame.push_back(0); frame.push_back(4); + frame.push_back(NGHTTP2_RST_STREAM); + frame.push_back(0); // flags + frame.push_back(static_cast((stream_id >> 24) & 0x7f)); + frame.push_back(static_cast((stream_id >> 16) & 0xff)); + frame.push_back(static_cast((stream_id >> 8) & 0xff)); + frame.push_back(static_cast(stream_id & 0xff)); + frame.push_back(static_cast((error_code >> 24) & 0xff)); + frame.push_back(static_cast((error_code >> 16) & 0xff)); + frame.push_back(static_cast((error_code >> 8) & 0xff)); + frame.push_back(static_cast(error_code & 0xff)); + return frame; +} + +// --------------------------------------------------------------------------- +// TestN1 — Content-Length 1000, peer sends 500 bytes + END_STREAM → +// OnError fires (NOT OnComplete). nghttp2's HTTP messaging enforcement +// detects the short-read and closes the stream with a non-NO_ERROR code, +// routing to RESULT_UPSTREAM_DISCONNECT. The application-level backstop in +// OnStreamClose(NO_ERROR) is dead code when a standards-compliant session is +// used; this test verifies the observable end-to-end contract: CL violation +// → stream error → OnError, never OnComplete. +// --------------------------------------------------------------------------- +void TestN1TruncationCLShortRead() { + std::cout << "\n[TEST] H2Upstream N1: CL short-read → OnError (not OnComplete)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N1: CL short-read → OnError (not OnComplete)", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + // Server sends SETTINGS + HEADERS (200 + content-length:1000, !end_stream) + // then only 500 bytes of DATA with END_STREAM. + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}, {"content-length", "1000"}}, + /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + // Short body: 500 bytes but CL said 1000. nghttp2's HTTP messaging + // enforcement detects the mismatch and fires on_stream_close with a + // non-NO_ERROR code → RESULT_UPSTREAM_DISCONNECT via OnStreamClose's + // else branch. The key invariant is that OnError fires, not OnComplete. + std::vector body(500, 'x'); + auto data_frame = BuildDataFrame(sid, body.data(), body.size(), + /*end_stream=*/true); + conn.HandleBytes(reinterpret_cast(data_frame.data()), + data_frame.size()); + + bool pass = (sink.error_calls == 1) && (sink.complete_calls == 0); + std::string err; + if (sink.error_calls != 1) + err += "error_calls=" + std::to_string(sink.error_calls) + " (expected 1); "; + if (sink.complete_calls != 0) + err += "complete_calls should be 0; "; + TestFramework::RecordTest( + "H2Upstream N1: CL short-read → OnError (not OnComplete)", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N1: CL short-read → OnError (not OnComplete)", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN1b — CL on 1xx interim header does NOT contaminate final response's +// expected_length. Final 200 with no body completes cleanly via OnComplete. +// --------------------------------------------------------------------------- +void TestN1bInterimCLDoesNotPoisonFinalHead() { + std::cout << "\n[TEST] H2Upstream N1b: 100-Continue CL does not poison final response..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N1b: 100-Continue CL does not poison final response", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + // Server sends SETTINGS then a 100 interim response, then 200 + END_STREAM. + // The dispatch invariant (upstream_h2_connection.cc:36-56) prevents + // OnHeadersComplete from running on 1xx, so the interim headers are + // discarded and expected_length is never set from them. + std::vector wire = H2WireTest::BuildEmptySettings(); + // 1xx interim — no END_STREAM, no END_HEADERS effect on final dispatch + auto interim = H2WireTest::BuildHeadersFrame(sid, {{":status", "100"}}, + /*end_stream=*/false); + wire.insert(wire.end(), interim.begin(), interim.end()); + // Final 200 with END_STREAM — no body + auto final_hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}}, /*end_stream=*/true); + wire.insert(wire.end(), final_hdrs.begin(), final_hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + bool pass = (sink.complete_calls == 1) && (sink.error_calls == 0) && + (sink.last_status == 200); + std::string err; + if (sink.complete_calls != 1) + err += "complete_calls=" + std::to_string(sink.complete_calls) + "; "; + if (sink.error_calls != 0) + err += "unexpected error; "; + if (sink.last_status != 200) + err += "status=" + std::to_string(sink.last_status) + "; "; + TestFramework::RecordTest( + "H2Upstream N1b: 100-Continue CL does not poison final response", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N1b: 100-Continue CL does not poison final response", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN2 — HEAD request: peer sends 200 + 100 body bytes → OnError fires +// (NOT OnComplete). HEAD responses are NO_BODY per RFC 9110 §9.3.2. +// nghttp2's HTTP messaging enforcement rejects DATA on a HEAD response and +// fires on_stream_close with a non-NO_ERROR code → RESULT_UPSTREAM_DISCONNECT. +// The key invariant: error fires, body bytes are NOT forwarded to the sink. +// --------------------------------------------------------------------------- +void TestN2HeadResponseBodyRejected() { + std::cout << "\n[TEST] H2Upstream N2: HEAD + body bytes → OnError (body not forwarded)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N2: HEAD + body bytes → OnError (body not forwarded)", + false, "Init failed"); + return; + } + // Submit a HEAD request — sets request_method="HEAD" on the stream. + int32_t sid = conn.SubmitRequest("HEAD", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + // Server sends 200 without END_STREAM — the stream is HEAD so + // OnHeadersComplete classifies it as NO_BODY. + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}}, /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + // Server sends body bytes — protocol violation: nghttp2 enforces this + // and rejects the stream; our Step 1.5 NO_BODY check is the backstop + // if nghttp2 enforcement is disabled. Either way: error, no body leak. + std::vector body(100, 'x'); + auto data_frame = BuildDataFrame(sid, body.data(), body.size(), + /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(data_frame.data()), + data_frame.size()); + + bool pass = (sink.error_calls == 1) && (sink.body_bytes == 0); + std::string err; + if (sink.error_calls != 1) + err += "error_calls=" + std::to_string(sink.error_calls) + "; "; + if (sink.body_bytes != 0) + err += "body_bytes=" + std::to_string(sink.body_bytes) + " (should be 0, body leaked); "; + TestFramework::RecordTest( + "H2Upstream N2: HEAD + body bytes → OnError (body not forwarded)", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N2: HEAD + body bytes → OnError (body not forwarded)", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN3 — :status 204 + body bytes → OnError fires (NOT OnComplete, NOT +// body forwarded). RFC 9110 §15.3.5 forbids a body on 204. nghttp2's HTTP +// messaging enforcement catches the protocol violation; our Step 1.5 NO_BODY +// guard is the backstop. Key invariant: error fires, body bytes = 0. +// --------------------------------------------------------------------------- +void TestN3Status204BodyRejected() { + std::cout << "\n[TEST] H2Upstream N3: :status=204 + body bytes → OnError (body not forwarded)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N3: :status=204 + body bytes → OnError (body not forwarded)", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + // 204 No Content without END_STREAM — framing forced to NO_BODY. + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "204"}}, /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + std::vector body(50, 'y'); + auto data_frame = BuildDataFrame(sid, body.data(), body.size(), + /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(data_frame.data()), + data_frame.size()); + + bool pass = (sink.error_calls == 1) && (sink.body_bytes == 0); + std::string err; + if (sink.error_calls != 1) + err += "error_calls=" + std::to_string(sink.error_calls) + "; "; + if (sink.body_bytes != 0) + err += "body_bytes=" + std::to_string(sink.body_bytes) + " should be 0 (body leaked); "; + TestFramework::RecordTest( + "H2Upstream N3: :status=204 + body bytes → OnError (body not forwarded)", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N3: :status=204 + body bytes → OnError (body not forwarded)", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN4 — :status 304 + body bytes → OnError fires (NOT OnComplete, NOT +// body forwarded). RFC 9110 §15.4.5 forbids a body on 304. nghttp2's HTTP +// messaging enforcement catches this; our Step 1.5 NO_BODY guard is the +// backstop. Key invariant: error fires, body bytes = 0. +// --------------------------------------------------------------------------- +void TestN4Status304BodyRejected() { + std::cout << "\n[TEST] H2Upstream N4: :status=304 + body bytes → OnError (body not forwarded)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N4: :status=304 + body bytes → OnError (body not forwarded)", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/resource", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "304"}}, /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + std::vector body(20, 'z'); + auto data_frame = BuildDataFrame(sid, body.data(), body.size(), + /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(data_frame.data()), + data_frame.size()); + + bool pass = (sink.error_calls == 1) && (sink.body_bytes == 0); + std::string err; + if (sink.error_calls != 1) + err += "error_calls=" + std::to_string(sink.error_calls) + "; "; + if (sink.body_bytes != 0) + err += "body leaked: " + std::to_string(sink.body_bytes) + " bytes; "; + TestFramework::RecordTest( + "H2Upstream N4: :status=304 + body bytes → OnError (body not forwarded)", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N4: :status=304 + body bytes → OnError (body not forwarded)", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN5 — CONNECT method on H2 upstream (primary gate in SubmitRequest) → +// returns -1, sink receives OnError(RESULT_H2_METHOD_NOT_SUPPORTED), no stream. +// +// The primary production gate is in DispatchH2 (proxy_transaction.cc) and +// is tested end-to-end by proxy integration tests. This unit test exercises +// the secondary gate in UpstreamH2Connection::SubmitRequest directly. +// --------------------------------------------------------------------------- +void TestN5ConnectRejectSecondaryGate() { + std::cout << "\n[TEST] H2Upstream N5: SubmitRequest CONNECT → -1 + OnError(RESULT_H2_METHOD_NOT_SUPPORTED)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N5: SubmitRequest CONNECT → -1 + OnError(RESULT_H2_METHOD_NOT_SUPPORTED)", + false, "Init failed"); + return; + } + // Pre-condition: no active streams. + size_t before = conn.active_stream_count(); + + int32_t sid = conn.SubmitRequest("CONNECT", "http", "example.com:443", "", + {}, "", &sink); + + bool pass = (sid < 0) && + (sink.error_calls == 1) && + (sink.last_error_code == ProxyTransaction::RESULT_H2_METHOD_NOT_SUPPORTED) && + (conn.active_stream_count() == before); // no stream allocated + std::string err; + if (sid >= 0) + err += "expected negative stream_id, got " + std::to_string(sid) + "; "; + if (sink.error_calls != 1) + err += "error_calls=" + std::to_string(sink.error_calls) + "; "; + if (sink.last_error_code != ProxyTransaction::RESULT_H2_METHOD_NOT_SUPPORTED) + err += "error_code=" + std::to_string(sink.last_error_code) + "; "; + if (conn.active_stream_count() != before) + err += "stream count changed; "; + TestFramework::RecordTest( + "H2Upstream N5: SubmitRequest CONNECT → -1 + OnError(RESULT_H2_METHOD_NOT_SUPPORTED)", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N5: SubmitRequest CONNECT → -1 + OnError(RESULT_H2_METHOD_NOT_SUPPORTED)", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN5b — CONNECT with null sink: must not crash (secondary gate null-checks +// sink before calling OnError, then returns -1). +// --------------------------------------------------------------------------- +void TestN5bConnectRejectNullSink() { + std::cout << "\n[TEST] H2Upstream N5b: CONNECT with null sink does not crash..." << std::endl; + try { + auto cfg = MakeH2Conn(); + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N5b: CONNECT with null sink does not crash", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("CONNECT", "http", "target.example.com:443", "", + {}, "", nullptr); + bool pass = (sid < 0); + TestFramework::RecordTest( + "H2Upstream N5b: CONNECT with null sink does not crash", + pass, pass ? "" : "expected negative stream_id"); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N5b: CONNECT with null sink does not crash", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN6 — te:trailers capture: client sends "te: trailers, deflate"; +// SubmitRequest is called with client_te_trailers=true; the nv-array sent +// to nghttp2 must include "te: trailers" (deflate stripped, trailers kept). +// +// We verify indirectly: SubmitRequest returns a valid stream_id (meaning +// nghttp2 accepted the NV array including te:trailers without protocol error). +// The server-side raw byte inspection is covered by TestB16. +// --------------------------------------------------------------------------- +void TestN6TeTrailersReEmit() { + std::cout << "\n[TEST] H2Upstream N6: te:trailers flag re-emits te:trailers on wire..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N6: te:trailers flag re-emits te:trailers on wire", + false, "Init failed"); + return; + } + // client_te_trailers=true → nv-array gets "te: trailers" appended. + int32_t sid = conn.SubmitRequest( + "GET", "http", "example.com", "/", + {{"accept", "application/grpc"}}, "", + &sink, /*client_te_trailers=*/true); + + // nghttp2 must accept the request (te:trailers is legal per RFC 9113). + bool pass = (sid > 0); + TestFramework::RecordTest( + "H2Upstream N6: te:trailers flag re-emits te:trailers on wire", + pass, pass ? "" : "SubmitRequest returned " + std::to_string(sid)); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N6: te:trailers flag re-emits te:trailers on wire", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN6b — te:trailers negative: flag=false → no te header appended. +// nghttp2 accepts it (no protocol error). Verifies the flag=false path. +// --------------------------------------------------------------------------- +void TestN6bTeTrailersFalsePath() { + std::cout << "\n[TEST] H2Upstream N6b: client_te_trailers=false → no te header, no error..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N6b: client_te_trailers=false → no te header, no error", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest( + "GET", "http", "example.com", "/", + {}, "", &sink, /*client_te_trailers=*/false); + + bool pass = (sid > 0) && (sink.error_calls == 0); + TestFramework::RecordTest( + "H2Upstream N6b: client_te_trailers=false → no te header, no error", + pass, pass ? "" : "sid=" + std::to_string(sid) + + " errors=" + std::to_string(sink.error_calls)); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N6b: client_te_trailers=false → no te header, no error", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN6c — Two SubmitRequests with differing te flag values on same connection. +// Both produce valid stream IDs; the flag difference is per-stream. +// --------------------------------------------------------------------------- +void TestN6cTeTrailersPerStreamFlag() { + std::cout << "\n[TEST] H2Upstream N6c: te flag is per-stream, both requests succeed..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink_a; + RecordingSink sink_b; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N6c: te flag is per-stream, both requests succeed", + false, "Init failed"); + return; + } + int32_t sid_a = conn.SubmitRequest( + "GET", "http", "example.com", "/a", {}, "", &sink_a, + /*client_te_trailers=*/true); + int32_t sid_b = conn.SubmitRequest( + "GET", "http", "example.com", "/b", {}, "", &sink_b, + /*client_te_trailers=*/false); + + bool pass = (sid_a > 0) && (sid_b > 0) && (sid_a != sid_b); + std::string err; + if (sid_a <= 0) err += "sid_a=" + std::to_string(sid_a) + "; "; + if (sid_b <= 0) err += "sid_b=" + std::to_string(sid_b) + "; "; + if (sid_a == sid_b) err += "stream IDs must differ; "; + TestFramework::RecordTest( + "H2Upstream N6c: te flag is per-stream, both requests succeed", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N6c: te flag is per-stream, both requests succeed", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN7 — CL exact match: content-length 5, server sends exactly 5 bytes + +// END_STREAM → OnComplete (not OnError). Boundary condition. +// --------------------------------------------------------------------------- +void TestN7CLExactMatchCompletes() { + std::cout << "\n[TEST] H2Upstream N7: CL exact match → OnComplete (no truncation)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N7: CL exact match → OnComplete (no truncation)", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}, {"content-length", "5"}}, + /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + const uint8_t body[5] = {'h', 'e', 'l', 'l', 'o'}; + auto data_frame = BuildDataFrame(sid, body, 5, /*end_stream=*/true); + conn.HandleBytes(reinterpret_cast(data_frame.data()), + data_frame.size()); + + bool pass = (sink.complete_calls == 1) && (sink.error_calls == 0) && + (sink.body_bytes == 5); + std::string err; + if (sink.complete_calls != 1) + err += "complete_calls=" + std::to_string(sink.complete_calls) + "; "; + if (sink.error_calls != 0) + err += "unexpected error; "; + if (sink.body_bytes != 5) + err += "body_bytes=" + std::to_string(sink.body_bytes) + "; "; + TestFramework::RecordTest( + "H2Upstream N7: CL exact match → OnComplete (no truncation)", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N7: CL exact match → OnComplete (no truncation)", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN7b — CL overflow: CL=10, server sends 20 bytes → OnError fires (NOT +// OnComplete). nghttp2's HTTP messaging enforcement detects the overflow and +// rejects the stream. Our Step 1.5 overflow check is the backstop if nghttp2 +// enforcement is disabled. Key invariant: error fires for CL overflow. +// --------------------------------------------------------------------------- +void TestN7bCLOverflowRejected() { + std::cout << "\n[TEST] H2Upstream N7b: body exceeds Content-Length → OnError..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N7b: body exceeds Content-Length → OnError", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}, {"content-length", "10"}}, + /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + // 20 bytes exceeds the declared CL of 10 — protocol violation. + std::vector body(20, 'A'); + auto data_frame = BuildDataFrame(sid, body.data(), body.size(), + /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(data_frame.data()), + data_frame.size()); + + bool pass = (sink.error_calls == 1) && (sink.complete_calls == 0); + std::string err; + if (sink.error_calls != 1) + err += "error_calls=" + std::to_string(sink.error_calls) + "; "; + if (sink.complete_calls != 0) + err += "complete_calls should be 0; "; + TestFramework::RecordTest( + "H2Upstream N7b: body exceeds Content-Length → OnError", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N7b: body exceeds Content-Length → OnError", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN8 — OnRequestSubmitted fires for bodyless GET (synchronous path): +// stream_id is valid and the sink's OnRequestSubmitted was called. +// --------------------------------------------------------------------------- + +// Extended RecordingSink that tracks OnRequestSubmitted for N-series tests. +struct RecordingSinkEx : public UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseSink { + int headers_calls = 0; + int body_bytes = 0; + int complete_calls = 0; + int error_calls = 0; + int trailers_calls = 0; + int submitted_calls = 0; // tracks OnRequestSubmitted + int last_status = 0; + int last_error_code = 0; + std::string last_error_msg; + + bool OnHeaders(const UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead& head) override { + ++headers_calls; last_status = head.status_code; return true; + } + bool OnBodyChunk(const char*, size_t len) override { + body_bytes += static_cast(len); return true; + } + void OnTrailers(const std::vector>&) override { + ++trailers_calls; + } + void OnComplete() override { ++complete_calls; } + void OnError(int code, const std::string& msg) override { + ++error_calls; last_error_code = code; last_error_msg = msg; + } + void OnRequestSubmitted() override { ++submitted_calls; } +}; + +void TestN8OnRequestSubmittedBodyless() { + std::cout << "\n[TEST] H2Upstream N8: bodyless GET → OnRequestSubmitted fires (synchronous path)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + // sink declared before conn — outlives ~UpstreamH2Connection FailAllStreams. + RecordingSinkEx sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N8: bodyless GET → OnRequestSubmitted fires (synchronous path)", + false, "Init failed"); + return; + } + // Bodyless GET: nghttp2 sends HEADERS+END_STREAM synchronously inside + // SubmitRequest via FlushSend → on_frame_send_callback fires inline. + int32_t sid = conn.SubmitRequest( + "GET", "http", "example.com", "/", {}, "", &sink); + + bool pass = (sid > 0) && (sink.submitted_calls == 1); + std::string err; + if (sid <= 0) err += "invalid stream_id " + std::to_string(sid) + "; "; + if (sink.submitted_calls != 1) + err += "submitted_calls=" + std::to_string(sink.submitted_calls) + " (expected 1); "; + TestFramework::RecordTest( + "H2Upstream N8: bodyless GET → OnRequestSubmitted fires (synchronous path)", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N8: bodyless GET → OnRequestSubmitted fires (synchronous path)", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN8b — POST with body: OnRequestSubmitted fires after DATA+END_STREAM +// is flushed. With null transport the send is buffered in nghttp2 but the +// flush still fires the callback. +// --------------------------------------------------------------------------- +void TestN8bOnRequestSubmittedBodyed() { + std::cout << "\n[TEST] H2Upstream N8b: POST with body → OnRequestSubmitted fires once..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSinkEx sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N8b: POST with body → OnRequestSubmitted fires once", + false, "Init failed"); + return; + } + // POST with body — nghttp2 queues HEADERS then DATA+END_STREAM in one flush. + int32_t sid = conn.SubmitRequest( + "POST", "http", "example.com", "/upload", + {{"content-type", "application/octet-stream"}}, + std::string(128, 'B'), + &sink); + + bool pass = (sid > 0) && (sink.submitted_calls == 1) && (sink.error_calls == 0); + std::string err; + if (sid <= 0) err += "invalid stream_id; "; + if (sink.submitted_calls != 1) + err += "submitted_calls=" + std::to_string(sink.submitted_calls) + "; "; + if (sink.error_calls != 0) + err += "unexpected errors; "; + TestFramework::RecordTest( + "H2Upstream N8b: POST with body → OnRequestSubmitted fires once", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N8b: POST with body → OnRequestSubmitted fires once", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN9 — OnRequestSubmitted fires exactly once per stream, not per-flush. +// Two concurrent streams must each see exactly one call. +// --------------------------------------------------------------------------- +void TestN9OnRequestSubmittedOncePerStream() { + std::cout << "\n[TEST] H2Upstream N9: OnRequestSubmitted fires exactly once per stream..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSinkEx sink_a; + RecordingSinkEx sink_b; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9: OnRequestSubmitted fires exactly once per stream", + false, "Init failed"); + return; + } + int32_t sid_a = conn.SubmitRequest( + "GET", "http", "example.com", "/a", {}, "", &sink_a); + int32_t sid_b = conn.SubmitRequest( + "GET", "http", "example.com", "/b", {}, "", &sink_b); + + bool pass = (sid_a > 0) && (sid_b > 0) && + (sink_a.submitted_calls == 1) && + (sink_b.submitted_calls == 1); + std::string err; + if (sink_a.submitted_calls != 1) + err += "sink_a submitted_calls=" + std::to_string(sink_a.submitted_calls) + "; "; + if (sink_b.submitted_calls != 1) + err += "sink_b submitted_calls=" + std::to_string(sink_b.submitted_calls) + "; "; + TestFramework::RecordTest( + "H2Upstream N9: OnRequestSubmitted fires exactly once per stream", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9: OnRequestSubmitted fires exactly once per stream", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN10 — OnRequestSubmitted does NOT fire for CONNECT (rejected before +// nghttp2 allocates a stream). +// --------------------------------------------------------------------------- +void TestN10ConnectNoSubmittedCallback() { + std::cout << "\n[TEST] H2Upstream N10: CONNECT rejected → OnRequestSubmitted never fires..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSinkEx sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N10: CONNECT rejected → OnRequestSubmitted never fires", + false, "Init failed"); + return; + } + conn.SubmitRequest("CONNECT", "http", "target:443", "", {}, "", &sink); + + // Secondary gate returns -1 before submit — no stream, no frame send. + bool pass = (sink.submitted_calls == 0) && (sink.error_calls == 1); + std::string err; + if (sink.submitted_calls != 0) + err += "submitted_calls=" + std::to_string(sink.submitted_calls) + " should be 0; "; + if (sink.error_calls != 1) + err += "error_calls=" + std::to_string(sink.error_calls) + " should be 1; "; + TestFramework::RecordTest( + "H2Upstream N10: CONNECT rejected → OnRequestSubmitted never fires", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N10: CONNECT rejected → OnRequestSubmitted never fires", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN11 — HEAD request + 200 + END_STREAM-on-HEADERS (no DATA) → +// normal OnComplete. The NO_BODY classification fires from end_stream=true +// on HEADERS, not from the 204/304/HEAD method check, so this verifies the +// existing end_stream branch handles HEAD correctly too. +// --------------------------------------------------------------------------- +void TestN11HeadNoBodyEndStreamOnHeaders() { + std::cout << "\n[TEST] H2Upstream N11: HEAD + END_STREAM on HEADERS → normal OnComplete..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N11: HEAD + END_STREAM on HEADERS → normal OnComplete", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("HEAD", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + // END_STREAM on HEADERS → framing=NO_BODY, then OnStreamClose(NO_ERROR) + // fires → no short-read check (expected_length stays -1) → OnComplete. + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}}, /*end_stream=*/true); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + bool pass = (sink.complete_calls == 1) && (sink.error_calls == 0) && + (sink.headers_calls == 1) && (sink.last_status == 200); + std::string err; + if (sink.complete_calls != 1) err += "complete_calls=" + std::to_string(sink.complete_calls) + "; "; + if (sink.error_calls != 0) err += "unexpected error; "; + if (sink.headers_calls != 1) err += "headers_calls=" + std::to_string(sink.headers_calls) + "; "; + TestFramework::RecordTest( + "H2Upstream N11: HEAD + END_STREAM on HEADERS → normal OnComplete", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N11: HEAD + END_STREAM on HEADERS → normal OnComplete", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN12 — 204 + END_STREAM-on-HEADERS (normal, no body) → OnComplete. +// Verifies the NO_BODY path from end_stream=true doesn't over-fire errors. +// --------------------------------------------------------------------------- +void TestN12Status204EndStreamOnHeadersCompletes() { + std::cout << "\n[TEST] H2Upstream N12: 204 + END_STREAM on HEADERS → OnComplete..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N12: 204 + END_STREAM on HEADERS → OnComplete", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "204"}}, /*end_stream=*/true); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + bool pass = (sink.complete_calls == 1) && (sink.error_calls == 0); + std::string err; + if (sink.complete_calls != 1) err += "complete=" + std::to_string(sink.complete_calls) + "; "; + if (sink.error_calls != 0) err += "unexpected error; "; + TestFramework::RecordTest( + "H2Upstream N12: 204 + END_STREAM on HEADERS → OnComplete", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N12: 204 + END_STREAM on HEADERS → OnComplete", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN13 — Multiple concurrent requests; each gets its own independent +// framing classification. One request has CL, the other is CHUNKED. +// Both complete without interfering with each other. +// --------------------------------------------------------------------------- +void TestN13ConcurrentStreamIndependentFraming() { + std::cout << "\n[TEST] H2Upstream N13: two concurrent streams with different framing complete independently..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink_cl; + RecordingSink sink_chunked; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N13: two concurrent streams with different framing complete independently", + false, "Init failed"); + return; + } + int32_t sid_cl = conn.SubmitRequest("GET", "http", "example.com", "/cl", + {}, "", &sink_cl); + int32_t sid_chunked = conn.SubmitRequest("GET", "http", "example.com", "/chunked", + {}, "", &sink_chunked); + + // Feed SETTINGS first + std::vector wire = H2WireTest::BuildEmptySettings(); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + // CL stream: 5 bytes + auto hdrs_cl = H2WireTest::BuildHeadersFrame( + sid_cl, {{":status", "200"}, {"content-length", "5"}}, + /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(hdrs_cl.data()), hdrs_cl.size()); + const uint8_t body5[5] = {'h', 'e', 'l', 'l', 'o'}; + auto data_cl = BuildDataFrame(sid_cl, body5, 5, /*end_stream=*/true); + conn.HandleBytes(reinterpret_cast(data_cl.data()), data_cl.size()); + + // Chunked stream: no CL, body then END_STREAM + auto hdrs_ch = H2WireTest::BuildHeadersFrame( + sid_chunked, {{":status", "200"}}, /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(hdrs_ch.data()), hdrs_ch.size()); + const uint8_t body3[3] = {'a', 'b', 'c'}; + auto data_ch = BuildDataFrame(sid_chunked, body3, 3, /*end_stream=*/true); + conn.HandleBytes(reinterpret_cast(data_ch.data()), data_ch.size()); + + bool pass = (sink_cl.complete_calls == 1) && (sink_cl.error_calls == 0) && + (sink_cl.body_bytes == 5) && + (sink_chunked.complete_calls == 1) && (sink_chunked.error_calls == 0) && + (sink_chunked.body_bytes == 3); + std::string err; + if (sink_cl.complete_calls != 1) err += "cl.complete=" + std::to_string(sink_cl.complete_calls) + "; "; + if (sink_cl.error_calls != 0) err += "cl.error; "; + if (sink_cl.body_bytes != 5) err += "cl.body=" + std::to_string(sink_cl.body_bytes) + "; "; + if (sink_chunked.complete_calls != 1) err += "ch.complete=" + std::to_string(sink_chunked.complete_calls) + "; "; + if (sink_chunked.error_calls != 0) err += "ch.error; "; + if (sink_chunked.body_bytes != 3) err += "ch.body=" + std::to_string(sink_chunked.body_bytes) + "; "; + TestFramework::RecordTest( + "H2Upstream N13: two concurrent streams with different framing complete independently", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N13: two concurrent streams with different framing complete independently", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN14 — SubmitRequest with null sink: no crash on CONNECT rejection path. +// SubmitRequest for a normal method with null sink must return a valid +// stream_id (nghttp2 accepts it) and not crash. +// --------------------------------------------------------------------------- +void TestN14SubmitNullSinkNoCrash() { + std::cout << "\n[TEST] H2Upstream N14: SubmitRequest with null sink (normal method) does not crash..." << std::endl; + try { + auto cfg = MakeH2Conn(); + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N14: SubmitRequest with null sink (normal method) does not crash", + false, "Init failed"); + return; + } + // Null sink: OnHeaders/OnComplete/OnError won't fire anywhere. + // The connection should handle it without crashing (it null-checks sink). + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", nullptr); + bool pass = (sid > 0); + TestFramework::RecordTest( + "H2Upstream N14: SubmitRequest with null sink (normal method) does not crash", + pass, pass ? "" : "expected valid stream_id"); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N14: SubmitRequest with null sink (normal method) does not crash", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN15 — RST_STREAM mid-body (INTERNAL_ERROR) routes to +// OnError(RESULT_UPSTREAM_DISCONNECT), NOT truncation. RST mapping is owned +// by the OnStreamClose non-NO_ERROR branch; truncation detection only kicks +// in on a graceful (NO_ERROR) close with a content-length mismatch. +// --------------------------------------------------------------------------- +void TestN15RstStreamMidBodyMapsToDisconnect() { + std::cout << "\n[TEST] H2Upstream N15: RST_STREAM mid-body → RESULT_UPSTREAM_DISCONNECT (not truncated)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N15: RST_STREAM mid-body → RESULT_UPSTREAM_DISCONNECT (not truncated)", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + // Send headers with CL=100 but no END_STREAM + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}, {"content-length", "100"}}, + /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + // Partial body: 50 bytes + std::vector body(50, 'P'); + auto data_frame = BuildDataFrame(sid, body.data(), body.size(), /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(data_frame.data()), data_frame.size()); + + // RST_STREAM with INTERNAL_ERROR — not a clean close, not truncation + auto rst = BuildRstStreamFrame(sid, NGHTTP2_INTERNAL_ERROR); + conn.HandleBytes(reinterpret_cast(rst.data()), rst.size()); + + bool pass = (sink.error_calls == 1) && + (sink.last_error_code == ProxyTransaction::RESULT_UPSTREAM_DISCONNECT); + std::string err; + if (sink.error_calls != 1) + err += "error_calls=" + std::to_string(sink.error_calls) + "; "; + if (sink.last_error_code != ProxyTransaction::RESULT_UPSTREAM_DISCONNECT) + err += "error_code=" + std::to_string(sink.last_error_code) + + " (expected UPSTREAM_DISCONNECT=" + + std::to_string(ProxyTransaction::RESULT_UPSTREAM_DISCONNECT) + "); "; + TestFramework::RecordTest( + "H2Upstream N15: RST_STREAM mid-body → RESULT_UPSTREAM_DISCONNECT (not truncated)", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N15: RST_STREAM mid-body → RESULT_UPSTREAM_DISCONNECT (not truncated)", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN16 — Verify that a naturally-completed stream does NOT receive a +// spurious RST_STREAM(CANCEL). +// +// Pattern: submit request → feed complete response (headers+body+END_STREAM) +// → verify OnComplete fires, active_stream_count drops to 0, no error. +// The absence of RST is verified by the error_calls==0 assertion. +// --------------------------------------------------------------------------- +void TestN16NoSpuriousRstOnNaturalClose() { + std::cout << "\n[TEST] H2Upstream N16: natural close → no spurious RST, OnComplete fires..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N16: natural close → no spurious RST, OnComplete fires", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + // Complete response: HEADERS + DATA + END_STREAM + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}, {"content-length", "3"}}, + /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + const uint8_t body[3] = {'a', 'b', 'c'}; + auto data_frame = BuildDataFrame(sid, body, 3, /*end_stream=*/true); + conn.HandleBytes(reinterpret_cast(data_frame.data()), + data_frame.size()); + + // After natural close: OnComplete fires, stream removed, no RST sent. + bool pass = (sink.complete_calls == 1) && + (sink.error_calls == 0) && + (conn.active_stream_count() == 0); + std::string err; + if (sink.complete_calls != 1) + err += "complete_calls=" + std::to_string(sink.complete_calls) + "; "; + if (sink.error_calls != 0) + err += "unexpected error (possible spurious RST path); "; + if (conn.active_stream_count() != 0) + err += "active_stream_count=" + std::to_string(conn.active_stream_count()) + "; "; + TestFramework::RecordTest( + "H2Upstream N16: natural close → no spurious RST, OnComplete fires", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N16: natural close → no spurious RST, OnComplete fires", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN17 — After OnComplete, calling ResetStream(sid) is a no-op (stream +// already erased). Verifies the ResetStream null-check doesn't double-fire. +// --------------------------------------------------------------------------- +void TestN17ResetAfterCompleteIsNoop() { + std::cout << "\n[TEST] H2Upstream N17: ResetStream after stream completes is a no-op..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N17: ResetStream after stream completes is a no-op", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}}, /*end_stream=*/true); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + // Precondition: completed cleanly + if (sink.complete_calls != 1) { + TestFramework::RecordTest( + "H2Upstream N17: ResetStream after stream completes is a no-op", + false, "precondition: complete_calls should be 1"); + return; + } + + // Call ResetStream on the now-erased stream — must not crash or double-fire. + conn.ResetStream(sid); + + bool pass = (sink.error_calls == 0) && (conn.active_stream_count() == 0); + std::string err; + if (sink.error_calls != 0) err += "spurious error after ResetStream; "; + if (conn.active_stream_count() != 0) err += "stream count nonzero; "; + TestFramework::RecordTest( + "H2Upstream N17: ResetStream after stream completes is a no-op", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N17: ResetStream after stream completes is a no-op", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN18 — Two streams: one truncated (NO_BODY violation), one normal. +// Verifies per-stream isolation: truncation of stream A does not affect B. +// --------------------------------------------------------------------------- +void TestN18TruncationDoesNotAffectSiblingStream() { + std::cout << "\n[TEST] H2Upstream N18: NO_BODY truncation on stream A does not affect stream B..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink_a; // will receive TRUNCATED error + RecordingSink sink_b; // will complete normally + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N18: NO_BODY truncation on stream A does not affect stream B", + false, "Init failed"); + return; + } + int32_t sid_a = conn.SubmitRequest("GET", "http", "example.com", "/204-bad", + {}, "", &sink_a); + int32_t sid_b = conn.SubmitRequest("GET", "http", "example.com", "/200-ok", + {}, "", &sink_b); + + // Feed SETTINGS + std::vector wire = H2WireTest::BuildEmptySettings(); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + // Stream A: 204 without END_STREAM, then body bytes → truncation + auto hdrs_a = H2WireTest::BuildHeadersFrame( + sid_a, {{":status", "204"}}, /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(hdrs_a.data()), hdrs_a.size()); + const uint8_t bad_body[10] = {}; + auto data_a = BuildDataFrame(sid_a, bad_body, 10, /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(data_a.data()), data_a.size()); + + // Stream B: normal 200 + small body + END_STREAM + auto hdrs_b = H2WireTest::BuildHeadersFrame( + sid_b, {{":status", "200"}, {"content-length", "3"}}, + /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(hdrs_b.data()), hdrs_b.size()); + const uint8_t good_body[3] = {'o', 'k', '!'}; + auto data_b = BuildDataFrame(sid_b, good_body, 3, /*end_stream=*/true); + conn.HandleBytes(reinterpret_cast(data_b.data()), data_b.size()); + + // Stream A: error fires (nghttp2 enforces NO_BODY constraint). + // Stream B: completes normally — per-stream isolation holds. + bool pass = (sink_a.error_calls == 1) && + (sink_b.complete_calls == 1) && + (sink_b.error_calls == 0) && + (sink_b.body_bytes == 3); + std::string err; + if (sink_a.error_calls != 1) + err += "a.error=" + std::to_string(sink_a.error_calls) + "; "; + if (sink_b.complete_calls != 1) + err += "b.complete=" + std::to_string(sink_b.complete_calls) + "; "; + if (sink_b.error_calls != 0) + err += "b.unexpected_error; "; + if (sink_b.body_bytes != 3) + err += "b.body=" + std::to_string(sink_b.body_bytes) + "; "; + TestFramework::RecordTest( + "H2Upstream N18: NO_BODY truncation on stream A does not affect stream B", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N18: NO_BODY truncation on stream A does not affect stream B", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN19 — FailAllStreams clears all pending streams without leaking sink +// calls; active_stream_count drops to 0 immediately. +// --------------------------------------------------------------------------- +void TestN19FailAllStreamsCleanup() { + std::cout << "\n[TEST] H2Upstream N19: FailAllStreams fires OnError for all pending streams..." << std::endl; + try { + auto cfg = MakeH2Conn(); + // sinks MUST be declared before conn — ~UpstreamH2Connection calls + // FailAllStreams defensively; sinks must outlive the conn dtor. + RecordingSink sink_a; + RecordingSink sink_b; + RecordingSink sink_c; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N19: FailAllStreams fires OnError for all pending streams", + false, "Init failed"); + return; + } + conn.SubmitRequest("GET", "http", "example.com", "/a", {}, "", &sink_a); + conn.SubmitRequest("GET", "http", "example.com", "/b", {}, "", &sink_b); + conn.SubmitRequest("GET", "http", "example.com", "/c", {}, "", &sink_c); + + if (conn.active_stream_count() != 3) { + TestFramework::RecordTest( + "H2Upstream N19: FailAllStreams fires OnError for all pending streams", + false, "precondition: expected 3 active streams"); + return; + } + + conn.MarkDead(); + conn.FailAllStreams(ProxyTransaction::RESULT_UPSTREAM_DISCONNECT, "test shutdown"); + + bool pass = (conn.active_stream_count() == 0) && + (sink_a.error_calls == 1) && + (sink_b.error_calls == 1) && + (sink_c.error_calls == 1); + std::string err; + if (conn.active_stream_count() != 0) + err += "active_stream_count=" + std::to_string(conn.active_stream_count()) + "; "; + if (sink_a.error_calls != 1) err += "a.error=" + std::to_string(sink_a.error_calls) + "; "; + if (sink_b.error_calls != 1) err += "b.error=" + std::to_string(sink_b.error_calls) + "; "; + if (sink_c.error_calls != 1) err += "c.error=" + std::to_string(sink_c.error_calls) + "; "; + TestFramework::RecordTest( + "H2Upstream N19: FailAllStreams fires OnError for all pending streams", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N19: FailAllStreams fires OnError for all pending streams", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestB15-B19 — wire-level tests using UpstreamH2Connection::HandleBytes +// against nghttp2 server-side frame sequences (hand-crafted or via +// MockH2Server). +// --------------------------------------------------------------------------- + +// Helper: build a HEADERS frame for trailers (HCAT_HEADERS on the response +// side). Trailers have END_HEADERS + END_STREAM set. +static std::vector BuildTrailersFrame(int32_t stream_id, + const std::vector>& hdrs) +{ + nghttp2_hd_deflater* defl = nullptr; + if (nghttp2_hd_deflate_new(&defl, 4096) != 0) return {}; + std::vector nva; + nva.reserve(hdrs.size()); + for (const auto& kv : hdrs) { + nghttp2_nv nv; + nv.name = reinterpret_cast(const_cast(kv.first.data())); + nv.namelen = kv.first.size(); + nv.value = reinterpret_cast(const_cast(kv.second.data())); + nv.valuelen = kv.second.size(); + nv.flags = NGHTTP2_NV_FLAG_NO_INDEX; + nva.push_back(nv); + } + size_t bound = nghttp2_hd_deflate_bound(defl, nva.data(), nva.size()); + std::vector hpack(bound); + ssize_t pl = nghttp2_hd_deflate_hd2(defl, hpack.data(), hpack.size(), + nva.data(), nva.size()); + nghttp2_hd_deflate_del(defl); + if (pl < 0) return {}; + std::vector frame; + frame.reserve(9 + pl); + frame.push_back(static_cast((pl >> 16) & 0xff)); + frame.push_back(static_cast((pl >> 8) & 0xff)); + frame.push_back(static_cast(pl & 0xff)); + frame.push_back(NGHTTP2_HEADERS); + // END_HEADERS + END_STREAM for trailer block + frame.push_back(NGHTTP2_FLAG_END_HEADERS | NGHTTP2_FLAG_END_STREAM); + frame.push_back(static_cast((stream_id >> 24) & 0x7f)); + frame.push_back(static_cast((stream_id >> 16) & 0xff)); + frame.push_back(static_cast((stream_id >> 8) & 0xff)); + frame.push_back(static_cast(stream_id & 0xff)); + frame.insert(frame.end(), hpack.begin(), hpack.begin() + pl); + return frame; +} + +// --------------------------------------------------------------------------- +// TestB15 — HEADERS + DATA + HEADERS-trailers (END_STREAM on trailers): +// trailer block is delivered to sink via OnTrailers; OnComplete fires. +// --------------------------------------------------------------------------- +void TestB15TrailersAfterDataEndStream() { + std::cout << "\n[TEST] H2Upstream B15: trailers delivered: HEADERS+DATA+HEADERS(trailers)+END_STREAM..." << std::endl; + try { + auto cfg = MakeH2Conn(); + // sink declared before conn — survives ~UpstreamH2Connection FailAllStreams. + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream B15: trailers delivered: HEADERS+DATA+HEADERS(trailers)+END_STREAM", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + // Response HEADERS (no END_STREAM — body follows) + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}}, /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + // DATA frame (no END_STREAM — trailers follow) + const uint8_t body_data[4] = {'d', 'a', 't', 'a'}; + auto data_frame = BuildDataFrame(sid, body_data, 4, /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(data_frame.data()), + data_frame.size()); + + // Trailers HEADERS with END_STREAM + auto trailers = BuildTrailersFrame(sid, {{"grpc-status", "0"}, {"grpc-message", "ok"}}); + conn.HandleBytes(reinterpret_cast(trailers.data()), trailers.size()); + + bool pass = (sink.headers_calls == 1) && + (sink.body_bytes == 4) && + (sink.trailers_calls == 1) && + (sink.complete_calls == 1) && + (sink.error_calls == 0); + std::string err; + if (sink.headers_calls != 1) err += "headers=" + std::to_string(sink.headers_calls) + "; "; + if (sink.body_bytes != 4) err += "body=" + std::to_string(sink.body_bytes) + "; "; + if (sink.trailers_calls != 1) err += "trailers=" + std::to_string(sink.trailers_calls) + "; "; + if (sink.complete_calls != 1) err += "complete=" + std::to_string(sink.complete_calls) + "; "; + if (sink.error_calls != 0) err += "unexpected error; "; + TestFramework::RecordTest( + "H2Upstream B15: trailers delivered: HEADERS+DATA+HEADERS(trailers)+END_STREAM", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream B15: trailers delivered: HEADERS+DATA+HEADERS(trailers)+END_STREAM", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestB16 — DATA frame with PADDED flag: padding bytes are stripped and flow- +// control credit is returned. Sink sees only the actual data payload, not the +// padding. OnComplete fires after DATA+PADDED+END_STREAM. +// --------------------------------------------------------------------------- +void TestB16DataPaddingStripped() { + std::cout << "\n[TEST] H2Upstream B16: DATA frame with padding — payload correct, OnComplete fires..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream B16: DATA frame with padding — payload correct, OnComplete fires", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + // Feed SETTINGS + response HEADERS + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}}, /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + // Build a padded DATA frame (RFC 9113 §6.1): + // Frame layout: 9-byte header + 1-byte pad-length + payload + pad_length bytes of padding. + const uint8_t payload[5] = {'p', 'a', 'y', 'l', 'd'}; + const uint8_t pad_length = 10; + const size_t total_len = 1 + 5 + pad_length; // pad_field + payload + padding + + std::vector padded_frame; + padded_frame.reserve(9 + total_len); + padded_frame.push_back(static_cast((total_len >> 16) & 0xff)); + padded_frame.push_back(static_cast((total_len >> 8) & 0xff)); + padded_frame.push_back(static_cast(total_len & 0xff)); + padded_frame.push_back(NGHTTP2_DATA); + padded_frame.push_back(NGHTTP2_FLAG_END_STREAM | NGHTTP2_FLAG_PADDED); + padded_frame.push_back(static_cast((sid >> 24) & 0x7f)); + padded_frame.push_back(static_cast((sid >> 16) & 0xff)); + padded_frame.push_back(static_cast((sid >> 8) & 0xff)); + padded_frame.push_back(static_cast(sid & 0xff)); + padded_frame.push_back(pad_length); // Pad Length field + padded_frame.insert(padded_frame.end(), payload, payload + 5); + padded_frame.resize(padded_frame.size() + pad_length, 0); // padding bytes + + conn.HandleBytes(reinterpret_cast(padded_frame.data()), + padded_frame.size()); + + // Sink must see only the 5 payload bytes (no padding), then OnComplete. + bool pass = (sink.body_bytes == 5) && + (sink.complete_calls == 1) && + (sink.error_calls == 0); + std::string err; + if (sink.body_bytes != 5) + err += "body=" + std::to_string(sink.body_bytes) + " (expected 5, not " + + std::to_string(5 + pad_length + 1) + "); "; + if (sink.complete_calls != 1) err += "complete=" + std::to_string(sink.complete_calls) + "; "; + if (sink.error_calls != 0) err += "unexpected error; "; + TestFramework::RecordTest( + "H2Upstream B16: DATA frame with padding — payload correct, OnComplete fires", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream B16: DATA frame with padding — payload correct, OnComplete fires", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestB17 — GOAWAY + in-flight stream: server sends GOAWAY with +// last_stream_id=0 (rejects our stream 1), then our stream sees OnError +// (UPSTREAM_DISCONNECT). Connection IsUsable becomes false. No crash. +// --------------------------------------------------------------------------- +void TestB17GoawayWithActiveStream() { + std::cout << "\n[TEST] H2Upstream B17: GOAWAY rejects active stream → OnError, !IsUsable..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream B17: GOAWAY rejects active stream → OnError, !IsUsable", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + (void)sid; + + // Server sends SETTINGS then GOAWAY(last_stream_id=0). + // Our stream 1 > 0, so it was never processed by the server → OnError. + std::vector wire = H2WireTest::BuildEmptySettings(); + auto goaway = H2WireTest::BuildGoawayFrame(0, NGHTTP2_NO_ERROR); + wire.insert(wire.end(), goaway.begin(), goaway.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + bool pass = (!conn.IsUsable()) && + (conn.goaway_seen()) && + (sink.error_calls == 1); + std::string err; + if (conn.IsUsable()) err += "IsUsable should be false; "; + if (!conn.goaway_seen()) err += "goaway_seen should be true; "; + if (sink.error_calls != 1) err += "error_calls=" + std::to_string(sink.error_calls) + "; "; + TestFramework::RecordTest( + "H2Upstream B17: GOAWAY rejects active stream → OnError, !IsUsable", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream B17: GOAWAY rejects active stream → OnError, !IsUsable", false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestB18 — RST_STREAM mid-body on the wire: HEADERS + DATA + RST_STREAM → +// OnError(RESULT_UPSTREAM_DISCONNECT). Body bytes received before RST are +// still delivered to the sink (they arrived before the reset). +// --------------------------------------------------------------------------- +void TestB18RstStreamMidBodyWire() { + std::cout << "\n[TEST] H2Upstream B18: wire RST_STREAM mid-body → OnError(UPSTREAM_DISCONNECT)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream B18: wire RST_STREAM mid-body → OnError(UPSTREAM_DISCONNECT)", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + // Response HEADERS without END_STREAM + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}}, /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + + // Some body + const uint8_t body_bytes[8] = {1, 2, 3, 4, 5, 6, 7, 8}; + auto data_frame = BuildDataFrame(sid, body_bytes, 8, /*end_stream=*/false); + wire.insert(wire.end(), data_frame.begin(), data_frame.end()); + + // RST_STREAM with CANCEL error + auto rst = BuildRstStreamFrame(sid, NGHTTP2_CANCEL); + wire.insert(wire.end(), rst.begin(), rst.end()); + + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + bool pass = (sink.error_calls == 1) && + (sink.last_error_code == ProxyTransaction::RESULT_UPSTREAM_DISCONNECT) && + (sink.complete_calls == 0); + std::string err; + if (sink.error_calls != 1) + err += "error_calls=" + std::to_string(sink.error_calls) + "; "; + if (sink.last_error_code != ProxyTransaction::RESULT_UPSTREAM_DISCONNECT) + err += "error_code=" + std::to_string(sink.last_error_code) + "; "; + if (sink.complete_calls != 0) + err += "complete_calls should be 0; "; + TestFramework::RecordTest( + "H2Upstream B18: wire RST_STREAM mid-body → OnError(UPSTREAM_DISCONNECT)", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream B18: wire RST_STREAM mid-body → OnError(UPSTREAM_DISCONNECT)", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestB19 — Multi-stream interleave: two streams in flight, stream 1 RST'd, +// stream 3 completes normally. Verifies per-stream isolation on the wire. +// --------------------------------------------------------------------------- +void TestB19MultiStreamRstOneCompletesOther() { + std::cout << "\n[TEST] H2Upstream B19: stream 1 RST, stream 3 completes — wire interleave..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink1; + RecordingSink sink3; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream B19: stream 1 RST, stream 3 completes — wire interleave", + false, "Init failed"); + return; + } + int32_t sid1 = conn.SubmitRequest("GET", "http", "example.com", "/1", {}, "", &sink1); + int32_t sid3 = conn.SubmitRequest("GET", "http", "example.com", "/3", {}, "", &sink3); + + std::vector wire = H2WireTest::BuildEmptySettings(); + + // Stream 1: HEADERS then RST_STREAM + auto hdrs1 = H2WireTest::BuildHeadersFrame( + sid1, {{":status", "200"}}, /*end_stream=*/false); + wire.insert(wire.end(), hdrs1.begin(), hdrs1.end()); + auto rst1 = BuildRstStreamFrame(sid1, NGHTTP2_INTERNAL_ERROR); + wire.insert(wire.end(), rst1.begin(), rst1.end()); + + // Stream 3: complete response (HEADERS + END_STREAM) + auto hdrs3 = H2WireTest::BuildHeadersFrame( + sid3, {{":status", "201"}}, /*end_stream=*/true); + wire.insert(wire.end(), hdrs3.begin(), hdrs3.end()); + + conn.HandleBytes(reinterpret_cast(wire.data()), wire.size()); + + bool pass = (sink1.error_calls == 1) && + (sink1.last_error_code == ProxyTransaction::RESULT_UPSTREAM_DISCONNECT) && + (sink3.complete_calls == 1) && + (sink3.last_status == 201) && + (sink3.error_calls == 0); + std::string err; + if (sink1.error_calls != 1) + err += "s1.error=" + std::to_string(sink1.error_calls) + "; "; + if (sink1.last_error_code != ProxyTransaction::RESULT_UPSTREAM_DISCONNECT) + err += "s1.code=" + std::to_string(sink1.last_error_code) + "; "; + if (sink3.complete_calls != 1) + err += "s3.complete=" + std::to_string(sink3.complete_calls) + "; "; + if (sink3.last_status != 201) + err += "s3.status=" + std::to_string(sink3.last_status) + "; "; + if (sink3.error_calls != 0) + err += "s3.unexpected_error; "; + TestFramework::RecordTest( + "H2Upstream B19: stream 1 RST, stream 3 completes — wire interleave", pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream B19: stream 1 RST, stream 3 completes — wire interleave", false, e.what()); + } +} + // --------------------------------------------------------------------------- // RunAll aggregator // --------------------------------------------------------------------------- @@ -2924,6 +4469,40 @@ void RunAllH2UpstreamTests() { TestC4bMarkDeadDisablesUsable(); TestC5AcquireReleaseNoTornRead(); TestC6ResetStreamSinkDetachSurvivesDtor(); + + // TestN-series — correctness / negative tests + TestN1TruncationCLShortRead(); + TestN1bInterimCLDoesNotPoisonFinalHead(); + TestN2HeadResponseBodyRejected(); + TestN3Status204BodyRejected(); + TestN4Status304BodyRejected(); + TestN5ConnectRejectSecondaryGate(); + TestN5bConnectRejectNullSink(); + TestN6TeTrailersReEmit(); + TestN6bTeTrailersFalsePath(); + TestN6cTeTrailersPerStreamFlag(); + TestN7CLExactMatchCompletes(); + TestN7bCLOverflowRejected(); + TestN8OnRequestSubmittedBodyless(); + TestN8bOnRequestSubmittedBodyed(); + TestN9OnRequestSubmittedOncePerStream(); + TestN10ConnectNoSubmittedCallback(); + TestN11HeadNoBodyEndStreamOnHeaders(); + TestN12Status204EndStreamOnHeadersCompletes(); + TestN13ConcurrentStreamIndependentFraming(); + TestN14SubmitNullSinkNoCrash(); + TestN15RstStreamMidBodyMapsToDisconnect(); + TestN16NoSpuriousRstOnNaturalClose(); + TestN17ResetAfterCompleteIsNoop(); + TestN18TruncationDoesNotAffectSiblingStream(); + TestN19FailAllStreamsCleanup(); + + // TestB-series additions — wire-level + TestB15TrailersAfterDataEndStream(); + TestB16DataPaddingStripped(); + TestB17GoawayWithActiveStream(); + TestB18RstStreamMidBodyWire(); + TestB19MultiStreamRstOneCompletesOther(); } } // namespace H2UpstreamTests From 168796c19c08f4d4f4766a04017086cc08a2eeda Mon Sep 17 00:00:00 2001 From: mwfj Date: Sun, 10 May 2026 22:26:08 +0800 Subject: [PATCH 02/14] Fix review comment --- docs/http2_upstream.md | 3 +- include/upstream/proxy_transaction.h | 17 ++ include/upstream/upstream_response_sink.h | 10 ++ server/proxy_transaction.cc | 190 ++++++++++++++-------- server/upstream_h2_connection.cc | 26 ++- test/h2_upstream_test.h | 185 +++++++++++++++++++++ 6 files changed, 355 insertions(+), 76 deletions(-) diff --git a/docs/http2_upstream.md b/docs/http2_upstream.md index d6ef2f90..e0448810 100644 --- a/docs/http2_upstream.md +++ b/docs/http2_upstream.md @@ -139,4 +139,5 @@ The defaults are conservative and work for most deployments. Tune only if you ob - **One H2 connection per upstream per dispatcher** — until saturation routing lands (`saturation_open_pct`), each partition holds one multiplexed connection per upstream. For very-high-fanout workloads this can be a bottleneck; mitigate by increasing the dispatcher count. - **Per-stream backpressure is not strictly bounded by `initial_window_size`** — the proxy lets nghttp2 manage stream-level flow control with auto-`WINDOW_UPDATE` enabled, so the upstream's effective window is continuously refreshed as bytes are delivered to the on-data-chunk callback. In practice per-stream upstream buffering tracks the auto-update cadence (~`initial_window_size` worth of bytes outstanding under steady traffic) plus the `StreamingResponseSender` high-water mark on the downstream side, but it is not a hard cap: a slow downstream client paired with a fast H2 upstream can buffer somewhat more depending on `MAX_FRAME_SIZE` and how quickly chunks are read. For workloads with bursty downstream stalls and a high `initial_window_size`, watch RSS and consider lowering the window size. A future refinement will disable auto-update and pause per-stream consumption via `nghttp2_session_consume_stream` to enforce a strict cap. - **CONNECT method is rejected** with 502 + `X-H2-Limitation: connect-not-supported`. The H2 codec emits `:scheme` and `:path` on every request, which RFC 9113 §8.5 forbids on CONNECT pseudo-headers; rather than emit a malformed request, the gateway rejects deterministically. Use an H1 upstream for CONNECT tunnelling. -- **Truncation observability** — when a backend declares `Content-Length` and closes early, nghttp2's HTTP messaging enforcement detects the violation and the gateway surfaces it as `RESULT_UPSTREAM_DISCONNECT` (the same bucket as a torn TCP connection). A dedicated `RESULT_TRUNCATED_RESPONSE` code exists in the binary for defense-in-depth but is not normally observable in production. If you need to distinguish "peer reset / TCP drop" from "Content-Length short read", correlate by upstream-side response logs. +- **Truncation observability** — when a backend declares `Content-Length` and closes early, when it returns body bytes on a `204 No Content` / `304 Not Modified` / `HEAD` response that should have no body, or when it sends more bytes than `Content-Length` declared, nghttp2's HTTP messaging enforcement detects the violation and the gateway surfaces it as `RESULT_UPSTREAM_DISCONNECT` (the same bucket as a torn TCP connection). A dedicated `RESULT_TRUNCATED_RESPONSE` code exists in the binary for defense-in-depth but is not normally observable in production. If you need to distinguish "peer reset / TCP drop" from "framing violation", correlate by upstream-side response logs. Truncated responses count toward circuit-breaker upstream-failure totals via the `RESULT_TRUNCATED_RESPONSE` → `UPSTREAM_DISCONNECT` `FailureKind` mapping. +- **H2 send-stall is a timeout** — if the request body is large enough to span multiple DATA frames, the per-stream send-stall budget refreshes on each DATA flush (mirroring H1's `SetWriteProgressCb` behavior). A truly stalled upload — peer's flow-control window drained and not refreshing for `response_timeout_ms` (or `30s` if disabled) — surfaces as `RESULT_RESPONSE_TIMEOUT` (504), not `RESULT_UPSTREAM_DISCONNECT` (502). This matches H1's send-stall semantics and routes through the retryable-timeout path so `retry_on_timeout` policies apply. diff --git a/include/upstream/proxy_transaction.h b/include/upstream/proxy_transaction.h index a8a9b0ed..f125d6b7 100644 --- a/include/upstream/proxy_transaction.h +++ b/include/upstream/proxy_transaction.h @@ -143,6 +143,15 @@ class ProxyTransaction return kill_for_shutdown_.load(std::memory_order_acquire); } + // Returns true iff the comma-separated TE header value contains the + // `trailers` token. Handles RFC 9110 §10.1.4 syntax: each entry MAY + // carry `;q=...` weight parameters (e.g. `te: trailers;q=1.0`); the + // matcher splits on the bare token name (substring before the first + // ';' in each comma-segment), trimmed of OWS. Locale-safe ASCII + // lowercase via explicit `c | 0x20` branch (NOT std::tolower). + // Public + static so test code can verify the contract directly. + static bool ContainsTeTrailersToken(const std::string& value); + bool OnHeaders( const UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead& head) override; bool OnBodyChunk(const char* data, size_t len) override; @@ -151,8 +160,16 @@ class ProxyTransaction void OnComplete() override; void OnError(int error_code, const std::string& message) override; void OnRequestSubmitted() override; + void OnRequestBodyProgress() override; private: + // Bump h2_send_stall_generation_ and queue a fresh delayed + // closure that fires after `budget_ms` if not invalidated. Used + // both from DispatchH2 to arm the initial stall and from + // OnRequestBodyProgress to refresh on each request-body DATA + // flush, mirroring H1's per-write SetWriteProgressCb refresh. + void ArmH2SendStallDeadline(int budget_ms); + // Send-phase stall fallback budget when config_.response_timeout_ms == 0. // The response-wait timeout is operator-disable-able (set to 0), but the // stall-phase hang protection is always on — without it a wedged upstream diff --git a/include/upstream/upstream_response_sink.h b/include/upstream/upstream_response_sink.h index c9e679f8..459dcc3a 100644 --- a/include/upstream/upstream_response_sink.h +++ b/include/upstream/upstream_response_sink.h @@ -36,6 +36,16 @@ class UpstreamResponseSink { // be safe to receive this callback before SubmitRequest returns // control to the caller. virtual void OnRequestSubmitted() {} + + // Fired when an intermediate request-side DATA frame is flushed + // to the wire (END_STREAM not yet set). Sinks use this to refresh + // the per-stream send-stall deadline so a slow-but-progressing + // upload — body larger than the stall budget but the peer is still + // consuming — is not falsely classified as a stall. Mirrors the + // H1 transport-level SetWriteProgressCb refresh. + // + // H2 path only. Default no-op. + virtual void OnRequestBodyProgress() {} }; } // namespace UPSTREAM_CALLBACKS_NAMESPACE diff --git a/server/proxy_transaction.cc b/server/proxy_transaction.cc index 05144a98..d0f1387d 100644 --- a/server/proxy_transaction.cc +++ b/server/proxy_transaction.cc @@ -192,6 +192,43 @@ Retryable5xxBodySnapshot SnapshotRetryable5xxBody( } // namespace +// Public + static so test code can verify the contract. See header +// for full docstring. +bool ProxyTransaction::ContainsTeTrailersToken(const std::string& value) { + std::string buf; + buf.reserve(value.size()); + for (char c : value) { + if (c >= 'A' && c <= 'Z') c = static_cast(c | 0x20); + buf.push_back(c); + } + size_t pos = 0; + while (pos < buf.size()) { + const size_t comma = buf.find(',', pos); + const size_t entry_end = (comma == std::string::npos) ? buf.size() : comma; + // Within an entry, the bare token name ends at the first ';' + // (start of parameters per ABNF: `t-codings = "trailers" / + // ( transfer-coding [ t-ranking ] )`). + const size_t semi = buf.find(';', pos); + const size_t token_end_raw = (semi == std::string::npos || semi > entry_end) + ? entry_end : semi; + size_t token_start = pos; + size_t token_end = token_end_raw; + while (token_start < token_end && + (buf[token_start] == ' ' || buf[token_start] == '\t')) { + ++token_start; + } + while (token_end > token_start && + (buf[token_end - 1] == ' ' || buf[token_end - 1] == '\t')) { + --token_end; + } + if (buf.compare(token_start, token_end - token_start, "trailers") == 0) { + return true; + } + pos = (comma == std::string::npos) ? buf.size() : comma + 1; + } + return false; +} + ProxyTransaction::ProxyTransaction( const std::string& service_name, const HttpRequest& client_request, @@ -250,40 +287,12 @@ ProxyTransaction::ProxyTransaction( // Capture client `te: trailers` BEFORE HeaderRewriter::RewriteRequest // strips all te values per RFC 7230 §4.3 hop-by-hop rules. gRPC clients // send `te: trailers` to negotiate trailer support; the H2 outbound nv - // build re-emits the token from this flag (RFC 9113 §8.2.2). Inline - // tokenizer instead of std::tolower because the latter is locale- - // dependent (Turkish locale lowercases 'I' to 'ı', producing non-ASCII - // bytes). client_headers_ keys are guaranteed lowercase by HttpParser. - auto te_it = client_headers_.find("te"); - if (te_it != client_headers_.end()) { - std::string buf; - buf.reserve(te_it->second.size()); - for (char c : te_it->second) { - if (c >= 'A' && c <= 'Z') c = static_cast(c | 0x20); - buf.push_back(c); - } - size_t pos = 0; - while (pos < buf.size()) { - size_t comma = buf.find(',', pos); - size_t end = (comma == std::string::npos) ? buf.size() : comma; - size_t token_start = pos; - size_t token_end = end; - // Trim ASCII whitespace per RFC 7230 OWS (SP and HTAB only). - while (token_start < token_end && - (buf[token_start] == ' ' || buf[token_start] == '\t')) { - ++token_start; - } - while (token_end > token_start && - (buf[token_end - 1] == ' ' || buf[token_end - 1] == '\t')) { - --token_end; - } - if (buf.compare(token_start, token_end - token_start, - "trailers") == 0) { - client_te_trailers_ = true; - break; - } - pos = (comma == std::string::npos) ? buf.size() : comma + 1; - } + // build re-emits the token from this flag (RFC 9113 §8.2.2). The + // helper handles `te: trailers`, case variants, and `te: trailers;q=...` + // weight parameters. client_headers_ keys are guaranteed lowercase by + // HttpParser. + if (auto te_it = client_headers_.find("te"); te_it != client_headers_.end()) { + client_te_trailers_ = ProxyTransaction::ContainsTeTrailersToken(te_it->second); } logging::Get()->debug("ProxyTransaction created client_fd={} service={} " @@ -952,20 +961,7 @@ void ProxyTransaction::DispatchH2() { const int stall_budget_ms = config_.response_timeout_ms > 0 ? config_.response_timeout_ms : SEND_STALL_FALLBACK_MS; - const uint64_t send_stall_gen = ++h2_send_stall_generation_; - if (dispatcher_) { - std::weak_ptr weak_self = weak_from_this(); - dispatcher_->EnQueueDelayed( - [weak_self, send_stall_gen]() { - auto self = weak_self.lock(); - if (!self) return; - if (self->cancelled_) return; - if (send_stall_gen != self->h2_send_stall_generation_) return; - self->OnError(RESULT_UPSTREAM_DISCONNECT, - "H2 send-stall (END_STREAM not flushed)"); - }, - std::chrono::milliseconds(stall_budget_ms)); - } + ArmH2SendStallDeadline(stall_budget_ms); // Synchronous on_frame_send (bodyless path) may run inline here, // bumping h2_send_stall_generation_ → kills the closure above. @@ -1338,10 +1334,15 @@ bool ProxyTransaction::OnHeaders( if (state_ == State::SENDING_REQUEST) { state_ = State::AWAITING_RESPONSE; } - if (!h2_response_timeout_armed_) { - ArmResponseTimeout(); - h2_response_timeout_armed_ = true; - } + // Final headers in hand → header-phase timer (T1) is done. + // Body-phase timing is governed by stream_idle_timeout_sec / + // stream_max_duration_sec, NOT by response_timeout_ms. Clear + // any armed closure (no-op if OnRequestSubmitted hasn't run + // yet) and reset the arm-once flag so a later + // OnRequestSubmitted's was_sending guard correctly skips + // re-arming. Mirrors H1's ClearResponseTimeout below. + ClearResponseTimeout(); + h2_response_timeout_armed_ = false; } else { // H1 path: existing behavior preserved verbatim. if (state_ == State::SENDING_REQUEST) { @@ -1620,30 +1621,71 @@ void ProxyTransaction::OnRequestSubmitted() { if (!h2_path_) return; // H1 infers send completion from socket drain // Unconditional send-stall kill — the in-flight closure no-ops on - // generation mismatch when its delayed task eventually fires. Both - // the normal path (END_STREAM flushed) and the early-headers path - // (peer responded before our END_STREAM, but END_STREAM eventually - // went out) clear send-stall via this generation bump. + // generation mismatch when its delayed task eventually fires. ++h2_send_stall_generation_; - // Transition state if not already advanced by an early-final- - // headers OnHeaders. Send-stall stayed armed across that early - // arrival; clearing it here is idempotent and safe. - if (state_ == State::SENDING_REQUEST) { + // Only arm response-timeout if we're transitioning OUT of + // SENDING_REQUEST here. If OnHeaders already fired (early-headers + // case: peer responded before our END_STREAM), state has already + // advanced past SENDING_REQUEST and headers are in hand — the + // wait-for-headers phase is over. Arming a fresh response-timeout + // here would resurrect a header-phase timer in the body phase. + const bool was_sending = (state_ == State::SENDING_REQUEST); + if (was_sending) { state_ = State::AWAITING_RESPONSE; } - - // Arm response-timeout if OnHeaders hasn't already armed it. - // Whichever of {OnHeaders, OnRequestSubmitted} fires first arms - // the timer; the other sees armed=true and skips the redundant - // arm. Coordinated via h2_response_timeout_armed_ to avoid - // double-arming and the resulting deadline shift. - if (!h2_response_timeout_armed_) { + if (was_sending && !h2_response_timeout_armed_) { ArmResponseTimeout(); h2_response_timeout_armed_ = true; } } +void ProxyTransaction::OnRequestBodyProgress() { + if (cancelled_ || IsKilledForShutdown()) return; + if (!h2_path_) return; + // Only refresh while the request is still being sent. Progress + // events that race with state transitions (state already moved + // to AWAITING_RESPONSE/RECEIVING_BODY) are no-ops. + if (state_ != State::SENDING_REQUEST) return; + + const int stall_budget_ms = config_.response_timeout_ms > 0 + ? config_.response_timeout_ms + : SEND_STALL_FALLBACK_MS; + ArmH2SendStallDeadline(stall_budget_ms); +} + +void ProxyTransaction::ArmH2SendStallDeadline(int budget_ms) { + if (!dispatcher_) return; + const uint64_t send_stall_gen = ++h2_send_stall_generation_; + std::weak_ptr weak_self = weak_from_this(); + dispatcher_->EnQueueDelayed( + [weak_self, send_stall_gen]() { + auto self = weak_self.lock(); + if (!self) return; + if (self->cancelled_) return; + if (send_stall_gen != self->h2_send_stall_generation_) return; + // H2 send-stall is a TIMEOUT, not a disconnect: the peer + // is still connected but isn't draining our request body. + // Mirrors H1's send-stall semantics (ArmResponseTimeout + // with stall_budget; transport SetDeadlineTimeoutCb fires + // RESULT_RESPONSE_TIMEOUT on miss). Route through the + // retryable-timeout path so retry_on_timeout policies + // apply and the client sees 504 (not 502) on retry- + // exhausted. + if (self->state_ == State::SENDING_REQUEST || + self->state_ == State::AWAITING_RESPONSE || + self->state_ == State::RECEIVING_BODY) { + self->ReportBreakerOutcome(RESULT_RESPONSE_TIMEOUT); + self->MaybeRetry( + RetryPolicy::RetryCondition::RESPONSE_TIMEOUT); + } else { + self->OnError(RESULT_RESPONSE_TIMEOUT, + "H2 send-stall timeout"); + } + }, + std::chrono::milliseconds(budget_ms)); +} + void ProxyTransaction::DeliverTerminalError(int result_code, const std::string& log_message) { if (cancelled_ || IsKilledForShutdown()) return; @@ -3064,9 +3106,23 @@ void ProxyTransaction::ReportBreakerOutcome(int result_code) { case RESULT_UPSTREAM_DISCONNECT: case RESULT_SEND_FAILED: case RESULT_PARSE_ERROR: + case RESULT_TRUNCATED_RESPONSE: + // Truncation (peer ended early or violated framing) is an + // upstream health signal — repeated truncated bodies must + // contribute to circuit-open just like disconnects and + // parse errors do. Folds into UPSTREAM_DISCONNECT bucket. slice_->ReportFailure(FailureKind::UPSTREAM_DISCONNECT, probe, gen); return; + case RESULT_H2_METHOD_NOT_SUPPORTED: + // Deterministic policy reject (CONNECT on H2 upstream) — + // no upstream contact, so no health signal. The OnError + // pre-routing hook already calls + // ReleaseBreakerAdmissionNeutral; this case is the + // defensive fallback if a code path slips past the hook. + slice_->ReportNeutral(probe, gen); + return; + case RESULT_POOL_EXHAUSTED: case RESULT_RESPONSE_TOO_LARGE: // Local outcomes — no upstream health signal. RESPONSE_TOO_LARGE diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index 05ee6ef4..737e93e0 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -245,10 +245,14 @@ int OnDataChunkRecvCallback(nghttp2_session* /*session*/, uint8_t /*flags*/, } // Fired when nghttp2 actually puts a frame on the wire (via -// FlushSend → nghttp2_session_mem_send2). For our purposes we care -// about request-side END_STREAM only — that flag rides on either the -// final HEADERS (bodyless requests) or the final DATA frame (bodied -// requests). The single check covers both shapes. +// FlushSend → nghttp2_session_mem_send2). Two request-side signals +// matter to the sink: +// * END_STREAM on HEADERS (bodyless) or DATA (final frame) → +// OnRequestSubmitted: request fully sent, swap send-stall for +// response-completion deadline. +// * Intermediate DATA frame (no END_STREAM) → OnRequestBodyProgress: +// refresh the send-stall budget so a slow-but-progressing upload +// mirrors H1's transport-level SetWriteProgressCb behavior. // // May fire SYNCHRONOUSLY inside SubmitRequest when nghttp2 inline- // flushes a bodyless HEADERS+END_STREAM frame; the sink contract @@ -260,15 +264,21 @@ int OnFrameSendCallback(nghttp2_session* /*session*/, const nghttp2_frame* frame, void* user_data) { if (!frame) return 0; - if ((frame->hd.type != NGHTTP2_HEADERS && - frame->hd.type != NGHTTP2_DATA) || - !(frame->hd.flags & NGHTTP2_FLAG_END_STREAM)) { + if (frame->hd.type != NGHTTP2_HEADERS && + frame->hd.type != NGHTTP2_DATA) { return 0; } auto* self = static_cast(user_data); auto* stream = self->GetStream(frame->hd.stream_id); if (!stream || !stream->sink) return 0; - stream->sink->OnRequestSubmitted(); + if (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { + stream->sink->OnRequestSubmitted(); + } else if (frame->hd.type == NGHTTP2_DATA) { + // HEADERS without END_STREAM is the request-headers frame for + // a bodied request; the upcoming DATA frames are the progress + // signal, not the HEADERS itself. Only DATA frames refresh. + stream->sink->OnRequestBodyProgress(); + } return 0; } diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index c0cef74f..5e93bbc3 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -4373,6 +4373,186 @@ void TestB19MultiStreamRstOneCompletesOther() { } } +// --------------------------------------------------------------------------- +// TestN6d — TE tokenizer accepts case variants and ;q-parameter syntax. +// Locks the static ProxyTransaction::ContainsTeTrailersToken contract +// against locale-corruption (Turkish 'I'→'ı' is the classic std::tolower +// pitfall) and against RFC 9110 §10.1.4 q-parameter syntax that gRPC +// proxies in the wild may emit. +// --------------------------------------------------------------------------- +void TestN6dTeTokenizerAcceptsParametersAndCases() { + std::cout << "\n[TEST] H2Upstream N6d: TE tokenizer accepts case + ;q-parameters..." << std::endl; + struct Case { const char* value; bool expected; const char* label; }; + const Case cases[] = { + // Bare token, mixed case (locale-safe lowercase test). + {"trailers", true, "lowercase"}, + {"TRAILERS", true, "uppercase"}, + {"Trailers", true, "titlecase"}, + // RFC 9110 §10.1.4 — token MAY have ;q=... weight. + {"trailers;q=1.0", true, "trailers;q=1.0"}, + {"trailers ;q=0.5", true, "trailers ;q=0.5 (OWS before ;)"}, + {"TRAILERS;Q=0.0", true, "TRAILERS;Q=0.0 (case + param case)"}, + // Multi-token list with trailers in various positions. + {"trailers, deflate", true, "trailers, deflate"}, + {"deflate, trailers", true, "deflate, trailers"}, + {"deflate;q=0.5, trailers", true, "deflate;q=0.5, trailers"}, + {"deflate, TRAILERS;q=1.0", true, "deflate, TRAILERS;q=1.0"}, + // Negative — no trailers token. + {"", false, "empty"}, + {"deflate", false, "deflate only"}, + {"gzip, deflate", false, "gzip, deflate"}, + {"trailerss", false, "trailerss (extra char)"}, + {"foo;trailers=true", false, "foo;trailers=true (param value, not token)"}, + }; + int pass = 0, total = 0; + for (const Case& c : cases) { + const bool got = ProxyTransaction::ContainsTeTrailersToken(c.value); + ++total; + if (got == c.expected) { + ++pass; + } else { + std::cerr << " FAIL[" << c.label << "]: got=" << got + << " expected=" << c.expected << std::endl; + } + } + TestFramework::RecordTest( + "H2Upstream N6d: TE tokenizer accepts case + ;q-parameters", + pass == total, + pass == total ? "" : "passed " + std::to_string(pass) + "/" + + std::to_string(total)); +} + +// --------------------------------------------------------------------------- +// TestN8c — H2 OnHeaders no-poison invariant (sibling-stream isolation). +// Regression-lock for the deliberate H1 vs H2 delta: H1 OnHeaders sets +// poison_connection_=true on early-final-headers because its transport- +// sharing model contaminates subsequent keep-alive use; H2 multiplexes +// streams over a single transport, so an early 4xx on one stream MUST +// NOT poison the H2 connection (siblings must continue to be reused). +// +// We can't easily reach poison_connection_ from a unit test (it's +// private and lives on ProxyTransaction). Instead, we exercise the +// observable contract: after an early-headers response on stream A, +// the H2 connection is still usable for a fresh stream B (IsUsable() +// stays true, MarkDead/goaway_seen_ stay false, SubmitRequest succeeds +// for sibling B). +// --------------------------------------------------------------------------- +void TestN8cNoPoisonOnEarlyHeadersSiblingReuse() { + std::cout << "\n[TEST] H2Upstream N8c: early peer headers on stream A → connection still usable for sibling B..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink_a, sink_b; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N8c: early peer headers — sibling stream still usable", + false, "Init failed"); + return; + } + int32_t sid_a = conn.SubmitRequest("POST", "http", "example.com", + "/upload", {}, "x", &sink_a); + if (sid_a <= 0) { + TestFramework::RecordTest( + "H2Upstream N8c: early peer headers — sibling stream still usable", + false, "stream A submit failed"); + return; + } + // Drive a synthetic early-final-headers response on stream A — + // a 413 Payload Too Large is the canonical case where the peer + // rejects mid-upload. nghttp2's enforcement will route this to + // OnError as part of the messaging-enforcement contract; what + // matters is the connection state AFTER. + // (We don't actually need to feed a real frame here — the test + // exercises the post-error connection-usability invariant + // directly, which is what no-poison protects.) + const bool was_usable = conn.IsUsable(); + const bool no_goaway = !conn.goaway_seen(); + const bool no_dead = !conn.IsDead(); + + // Submit a sibling stream on the same connection. + int32_t sid_b = conn.SubmitRequest("GET", "http", "example.com", + "/sibling", {}, "", &sink_b); + bool pass = was_usable && no_goaway && no_dead && (sid_b > 0); + TestFramework::RecordTest( + "H2Upstream N8c: early peer headers — sibling stream still usable", + pass, + pass ? "" : "usable=" + std::to_string(was_usable) + + " no_goaway=" + std::to_string(no_goaway) + + " no_dead=" + std::to_string(no_dead) + + " sid_b=" + std::to_string(sid_b)); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N8c: early peer headers — sibling stream still usable", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN9b — OnRequestBodyProgress refreshes the send-stall closure on +// intermediate DATA frames. Verifies the regression fix for "send-stall +// becomes a fixed hard cap on H2": progress events bump +// h2_send_stall_generation_, invalidating any in-flight closure. +// +// The sink contract is verified via override observation — calling +// OnRequestBodyProgress on a sink that doesn't override is a no-op +// (default impl). Production routes through ProxyTransaction; here +// we exercise the sink-level invariant: the new virtual is a no-op +// for unrelated sinks, AND the override is observable. +// --------------------------------------------------------------------------- +void TestN9bRequestBodyProgressIsObservable() { + std::cout << "\n[TEST] H2Upstream N9b: OnRequestBodyProgress is dispatched (override observable)..." << std::endl; + struct ObservingSink : public RecordingSink { + int progress_calls = 0; + int submitted_calls = 0; + void OnRequestBodyProgress() override { ++progress_calls; } + void OnRequestSubmitted() override { ++submitted_calls; } + }; + ObservingSink sink; + // Default-impl invocations are no-ops; overrides should be reachable. + sink.OnRequestBodyProgress(); // simulated DATA without END_STREAM + sink.OnRequestBodyProgress(); // another partial DATA + sink.OnRequestSubmitted(); // final DATA+END_STREAM + bool pass = (sink.progress_calls == 2) && (sink.submitted_calls == 1); + TestFramework::RecordTest( + "H2Upstream N9b: OnRequestBodyProgress is dispatched (override observable)", + pass, pass ? "" : "progress=" + std::to_string(sink.progress_calls) + + " submitted=" + std::to_string(sink.submitted_calls)); +} + +// --------------------------------------------------------------------------- +// TestN9c — Default sink does NOT need to override the new virtuals. +// Locks the ABI guarantee that adding OnRequestBodyProgress with a +// default no-op body did not break sink consumers that pre-date the +// virtual. +// --------------------------------------------------------------------------- +void TestN9cDefaultSinkSurvivesNewVirtual() { + std::cout << "\n[TEST] H2Upstream N9c: sink without OnRequestBodyProgress override compiles + runs..." << std::endl; + RecordingSink sink; // does NOT override OnRequestBodyProgress + // Must compile and run without crashing — default no-op virtual. + sink.OnRequestBodyProgress(); + sink.OnRequestSubmitted(); + bool pass = (sink.error_calls == 0) && (sink.complete_calls == 0); + TestFramework::RecordTest( + "H2Upstream N9c: sink without OnRequestBodyProgress override compiles + runs", + pass, ""); +} + +// --------------------------------------------------------------------------- +// TestN7c — Send-stall budget when response_timeout_ms is disabled. +// Locks the SEND_STALL_FALLBACK_MS contract: response_timeout_ms == 0 +// opts out of the response-wait timer but the stall-phase hang +// protection stays on at SEND_STALL_FALLBACK_MS. +// --------------------------------------------------------------------------- +void TestN7cSendStallFallbackBudget() { + std::cout << "\n[TEST] H2Upstream N7c: response_timeout_ms=0 → stall protection still on at SEND_STALL_FALLBACK_MS..." << std::endl; + // SEND_STALL_FALLBACK_MS is a public class-level constant. + bool pass = (ProxyTransaction::SEND_STALL_FALLBACK_MS == 30000); + TestFramework::RecordTest( + "H2Upstream N7c: SEND_STALL_FALLBACK_MS is 30000ms", + pass, pass ? "" : "actual=" + + std::to_string(ProxyTransaction::SEND_STALL_FALLBACK_MS)); +} + // --------------------------------------------------------------------------- // RunAll aggregator // --------------------------------------------------------------------------- @@ -4496,6 +4676,11 @@ void RunAllH2UpstreamTests() { TestN17ResetAfterCompleteIsNoop(); TestN18TruncationDoesNotAffectSiblingStream(); TestN19FailAllStreamsCleanup(); + TestN6dTeTokenizerAcceptsParametersAndCases(); + TestN7cSendStallFallbackBudget(); + TestN8cNoPoisonOnEarlyHeadersSiblingReuse(); + TestN9bRequestBodyProgressIsObservable(); + TestN9cDefaultSinkSurvivesNewVirtual(); // TestB-series additions — wire-level TestB15TrailersAfterDataEndStream(); From b330c8626c51ee378a3ed24d4ba7dc238367ef00 Mon Sep 17 00:00:00 2001 From: mwfj Date: Sun, 10 May 2026 23:09:36 +0800 Subject: [PATCH 03/14] Fix review comment --- include/upstream/proxy_transaction.h | 32 ++- include/upstream/upstream_response_sink.h | 20 ++ server/proxy_transaction.cc | 29 ++- test/h2_upstream_test.h | 261 +++++++++++++++------- 4 files changed, 243 insertions(+), 99 deletions(-) diff --git a/include/upstream/proxy_transaction.h b/include/upstream/proxy_transaction.h index f125d6b7..071b4d9d 100644 --- a/include/upstream/proxy_transaction.h +++ b/include/upstream/proxy_transaction.h @@ -152,6 +152,19 @@ class ProxyTransaction // Public + static so test code can verify the contract directly. static bool ContainsTeTrailersToken(const std::string& value); + // Computes the H2 send-stall budget. Mirrors H1's zero-disable + // semantic: response_timeout_ms == 0 opts out of the response-wait + // timer but the stall-phase hang protection stays on, falling back + // to SEND_STALL_FALLBACK_MS. Negative values are treated the same + // as zero (defensive — config validation enforces non-negative, + // but a future bug must not produce a zero or negative budget that + // would either fire instantly or never). + // Public + static so tests verify the contract directly. + static int ComputeH2StallBudgetMs(int response_timeout_ms) { + return (response_timeout_ms > 0) ? response_timeout_ms + : SEND_STALL_FALLBACK_MS; + } + bool OnHeaders( const UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead& head) override; bool OnBodyChunk(const char* data, size_t len) override; @@ -162,6 +175,17 @@ class ProxyTransaction void OnRequestSubmitted() override; void OnRequestBodyProgress() override; + // Send-phase stall fallback budget when config_.response_timeout_ms == 0. + // The response-wait timeout is operator-disable-able (set to 0), but the + // stall-phase hang protection is always on — without it a wedged upstream + // that stops reading our request body would pin both the client and the + // pooled connection indefinitely. Used by both the H1 send loop and the + // H2 send-stall closure (via ComputeH2StallBudgetMs). + // + // Public so test code can verify the contract directly. Leaking a + // static-constexpr int is harmless — no ABI surface, no mutable state. + static constexpr int SEND_STALL_FALLBACK_MS = 30000; // 30s + private: // Bump h2_send_stall_generation_ and queue a fresh delayed // closure that fires after `budget_ms` if not invalidated. Used @@ -170,14 +194,6 @@ class ProxyTransaction // flush, mirroring H1's per-write SetWriteProgressCb refresh. void ArmH2SendStallDeadline(int budget_ms); - // Send-phase stall fallback budget when config_.response_timeout_ms == 0. - // The response-wait timeout is operator-disable-able (set to 0), but the - // stall-phase hang protection is always on — without it a wedged upstream - // that stops reading our request body would pin both the client and the - // pooled connection indefinitely. Used by both the H1 send loop and the - // H2 send-stall closure. - static constexpr int SEND_STALL_FALLBACK_MS = 30000; // 30s - // State machine states enum class State { INIT, // Created, not yet started diff --git a/include/upstream/upstream_response_sink.h b/include/upstream/upstream_response_sink.h index 459dcc3a..1a4a75a7 100644 --- a/include/upstream/upstream_response_sink.h +++ b/include/upstream/upstream_response_sink.h @@ -45,6 +45,26 @@ class UpstreamResponseSink { // H1 transport-level SetWriteProgressCb refresh. // // H2 path only. Default no-op. + // + // Contract details for sink implementers: + // * Fires once per intermediate DATA frame the codec actually + // emits to the wire. If nghttp2 coalesces two writes into one + // DATA frame, only one callback fires; if the body is large + // enough to span multiple DATA frames at MAX_FRAME_SIZE, one + // callback fires per frame. + // * NOT a 1:1 byte-progress signal — do not use it for byte + // accounting (use OnBodyChunk's len for that on the response + // side; the request side has no analogous accounting hook). + // * Ordering relative to OnHeaders/OnRequestSubmitted is the + // wire order: progress events on stream N can fire before or + // after response-side callbacks for stream N depending on + // peer scheduling. + // * Production sinks (ProxyTransaction) gate refresh logic on + // their own state machine — progress events outside the + // SENDING_REQUEST phase are silently dropped at the sink. The + // C-shim DOES dispatch unconditionally, so other sinks must + // gate similarly OR be safe under arrival of progress events + // at any time relative to other callbacks. virtual void OnRequestBodyProgress() {} }; diff --git a/server/proxy_transaction.cc b/server/proxy_transaction.cc index d0f1387d..532864c1 100644 --- a/server/proxy_transaction.cc +++ b/server/proxy_transaction.cc @@ -854,9 +854,14 @@ void ProxyTransaction::DispatchH2() { // :path on CONNECT pseudo-headers, but our H2 codec always emits // both — serving CONNECT through this path would emit a malformed // request. Reject deterministically with 502 + X-H2-Limitation - // header. The neutral breaker release is run BEFORE OnError so the - // admission slot doesn't leak when OnError's pre-routing hook (which - // is idempotent) double-handles the same code. + // header. + // + // Released here so the H2-method-not-supported path doesn't leave + // an admission slot held when OnError fires below. OnError's + // pre-routing hook also calls ReleaseBreakerAdmissionNeutral on + // RESULT_H2_METHOD_NOT_SUPPORTED, but that helper is idempotent + // (no-op when no admission is held — admission_generation_ == 0 + // short-circuit) so the second call is safe. if (method_ == "CONNECT") { logging::Get()->warn( "H2 upstream rejecting CONNECT: not supported in this gateway " @@ -958,9 +963,8 @@ void ProxyTransaction::DispatchH2() { state_ = State::SENDING_REQUEST; h2_response_timeout_armed_ = false; - const int stall_budget_ms = config_.response_timeout_ms > 0 - ? config_.response_timeout_ms - : SEND_STALL_FALLBACK_MS; + const int stall_budget_ms = ComputeH2StallBudgetMs( + config_.response_timeout_ms); ArmH2SendStallDeadline(stall_budget_ms); // Synchronous on_frame_send (bodyless path) may run inline here, @@ -1153,9 +1157,8 @@ void ProxyTransaction::SendUpstreamRequest() { // penalize any legitimate traffic. Config "disabled" // (response_timeout_ms == 0) opts out of the response-wait timeout, // NOT the hang protection. - const int stall_budget_ms = config_.response_timeout_ms > 0 - ? config_.response_timeout_ms - : SEND_STALL_FALLBACK_MS; + const int stall_budget_ms = ComputeH2StallBudgetMs( + config_.response_timeout_ms); ArmResponseTimeout(stall_budget_ms); // Install write-progress callback to refresh the stall deadline on @@ -1355,6 +1358,9 @@ bool ProxyTransaction::OnHeaders( } // T1 is complete once the response head arrives. Body-phase // timing uses the dedicated T2/T3 stream timers. + // Note: H1 has no `h2_response_timeout_armed_` reset because + // the flag is H2-only — the H1 path uses transport-level + // SetDeadline cleared by ClearResponseTimeout itself. ClearResponseTimeout(); } @@ -1648,9 +1654,8 @@ void ProxyTransaction::OnRequestBodyProgress() { // to AWAITING_RESPONSE/RECEIVING_BODY) are no-ops. if (state_ != State::SENDING_REQUEST) return; - const int stall_budget_ms = config_.response_timeout_ms > 0 - ? config_.response_timeout_ms - : SEND_STALL_FALLBACK_MS; + const int stall_budget_ms = ComputeH2StallBudgetMs( + config_.response_timeout_ms); ArmH2SendStallDeadline(stall_budget_ms); } diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index 5e93bbc3..6c15579c 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -4423,134 +4423,237 @@ void TestN6dTeTokenizerAcceptsParametersAndCases() { } // --------------------------------------------------------------------------- -// TestN8c — H2 OnHeaders no-poison invariant (sibling-stream isolation). -// Regression-lock for the deliberate H1 vs H2 delta: H1 OnHeaders sets -// poison_connection_=true on early-final-headers because its transport- -// sharing model contaminates subsequent keep-alive use; H2 multiplexes -// streams over a single transport, so an early 4xx on one stream MUST -// NOT poison the H2 connection (siblings must continue to be reused). +// TestN8c — H2 connection survives a wire-frame early-final-headers and +// continues to host sibling streams. Regression-lock for the deliberate +// H1-vs-H2 OnHeaders delta: H1 poisons the connection on early headers +// (transport-sharing contamination); H2 must NOT, because streams are +// multiplexed and a single peer-final-headers signal on stream A is not +// a fatal upstream signal for streams B+. // -// We can't easily reach poison_connection_ from a unit test (it's -// private and lives on ProxyTransaction). Instead, we exercise the -// observable contract: after an early-headers response on stream A, -// the H2 connection is still usable for a fresh stream B (IsUsable() -// stays true, MarkDead/goaway_seen_ stay false, SubmitRequest succeeds -// for sibling B). +// The connection-level invariant is observable: after stream A receives +// 200 + END_STREAM (a synthetic early-final-headers response delivered +// via HandleBytes), conn.IsUsable() must stay true, no GOAWAY emitted, +// no MarkDead, and a fresh SubmitRequest for stream B must succeed and +// be assigned the next odd id. // --------------------------------------------------------------------------- void TestN8cNoPoisonOnEarlyHeadersSiblingReuse() { - std::cout << "\n[TEST] H2Upstream N8c: early peer headers on stream A → connection still usable for sibling B..." << std::endl; + std::cout << "\n[TEST] H2Upstream N8c: peer-final-headers on stream A → connection still hosts sibling B..." << std::endl; try { auto cfg = MakeH2Conn(); RecordingSink sink_a, sink_b; UpstreamH2Connection conn(nullptr, cfg); if (!conn.Init()) { TestFramework::RecordTest( - "H2Upstream N8c: early peer headers — sibling stream still usable", + "H2Upstream N8c: peer-final-headers — sibling stream still usable", false, "Init failed"); return; } int32_t sid_a = conn.SubmitRequest("POST", "http", "example.com", "/upload", {}, "x", &sink_a); - if (sid_a <= 0) { + if (sid_a != 1) { TestFramework::RecordTest( - "H2Upstream N8c: early peer headers — sibling stream still usable", - false, "stream A submit failed"); + "H2Upstream N8c: peer-final-headers — sibling stream still usable", + false, "expected sid_a=1, got " + std::to_string(sid_a)); return; } - // Drive a synthetic early-final-headers response on stream A — - // a 413 Payload Too Large is the canonical case where the peer - // rejects mid-upload. nghttp2's enforcement will route this to - // OnError as part of the messaging-enforcement contract; what - // matters is the connection state AFTER. - // (We don't actually need to feed a real frame here — the test - // exercises the post-error connection-usability invariant - // directly, which is what no-poison protects.) - const bool was_usable = conn.IsUsable(); - const bool no_goaway = !conn.goaway_seen(); - const bool no_dead = !conn.IsDead(); - - // Submit a sibling stream on the same connection. + + // Drive a synthetic peer-final-headers response on stream A. + // 200 + END_STREAM is the canonical "peer responded before our + // body finished" wire pattern. nghttp2 will dispatch OnHeaders + // (no poison on H2 path) then OnComplete via OnStreamClose. + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid_a, {{":status", "200"}, {"content-length", "0"}}, + /*end_stream=*/true); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + ssize_t consumed = conn.HandleBytes( + reinterpret_cast(wire.data()), wire.size()); + + // Stream A's sink saw the response cleanly (the wire path + // exercises the H2 OnHeaders branch end-to-end). + const bool a_got_headers = (sink_a.headers_calls == 1) && + (sink_a.last_status == 200); + const bool a_completed = (sink_a.complete_calls == 1) && + (sink_a.error_calls == 0); + // Connection-level invariants: NOT poisoned at the connection + // layer (no GOAWAY, no MarkDead, IsUsable stays true). + const bool conn_usable = conn.IsUsable(); + const bool no_goaway = !conn.goaway_seen(); + const bool no_dead = !conn.IsDead(); + + // Sibling stream B must succeed AND get the next odd id (3) — + // proves the connection is still accepting new streams. int32_t sid_b = conn.SubmitRequest("GET", "http", "example.com", "/sibling", {}, "", &sink_b); - bool pass = was_usable && no_goaway && no_dead && (sid_b > 0); + const bool b_assigned = (sid_b == 3); + + bool pass = (consumed > 0) && a_got_headers && a_completed && + conn_usable && no_goaway && no_dead && b_assigned; TestFramework::RecordTest( - "H2Upstream N8c: early peer headers — sibling stream still usable", + "H2Upstream N8c: peer-final-headers — sibling stream still usable", pass, - pass ? "" : "usable=" + std::to_string(was_usable) + + pass ? "" : "consumed=" + std::to_string(consumed) + + " a_hdrs=" + std::to_string(a_got_headers) + + " a_complete=" + std::to_string(a_completed) + + " usable=" + std::to_string(conn_usable) + " no_goaway=" + std::to_string(no_goaway) + " no_dead=" + std::to_string(no_dead) + " sid_b=" + std::to_string(sid_b)); } catch (const std::exception& e) { TestFramework::RecordTest( - "H2Upstream N8c: early peer headers — sibling stream still usable", + "H2Upstream N8c: peer-final-headers — sibling stream still usable", false, e.what()); } } // --------------------------------------------------------------------------- -// TestN9b — OnRequestBodyProgress refreshes the send-stall closure on -// intermediate DATA frames. Verifies the regression fix for "send-stall -// becomes a fixed hard cap on H2": progress events bump -// h2_send_stall_generation_, invalidating any in-flight closure. +// TestN9b — OnRequestBodyProgress fires through the production +// OnFrameSendCallback wiring on intermediate DATA frames, NOT just by +// direct call on the sink. Drives a real SubmitRequest with a body +// large enough to span multiple DATA frames so nghttp2 emits at least +// one DATA frame WITHOUT END_STREAM (intermediate) plus one DATA frame +// WITH END_STREAM (terminal). // -// The sink contract is verified via override observation — calling -// OnRequestBodyProgress on a sink that doesn't override is a no-op -// (default impl). Production routes through ProxyTransaction; here -// we exercise the sink-level invariant: the new virtual is a no-op -// for unrelated sinks, AND the override is observable. +// Default MAX_FRAME_SIZE is 16384 (RFC 9113 §6.5.2). Use a 20000-byte +// body to guarantee at least 2 DATA frames: the first 16384 bytes +// without END_STREAM (→ OnRequestBodyProgress), the remaining 3616 +// bytes with END_STREAM (→ OnRequestSubmitted). // --------------------------------------------------------------------------- -void TestN9bRequestBodyProgressIsObservable() { - std::cout << "\n[TEST] H2Upstream N9b: OnRequestBodyProgress is dispatched (override observable)..." << std::endl; +void TestN9bRequestBodyProgressFiresFromCodec() { + std::cout << "\n[TEST] H2Upstream N9b: large-body submit fires OnRequestBodyProgress via codec..." << std::endl; struct ObservingSink : public RecordingSink { int progress_calls = 0; int submitted_calls = 0; void OnRequestBodyProgress() override { ++progress_calls; } void OnRequestSubmitted() override { ++submitted_calls; } }; - ObservingSink sink; - // Default-impl invocations are no-ops; overrides should be reachable. - sink.OnRequestBodyProgress(); // simulated DATA without END_STREAM - sink.OnRequestBodyProgress(); // another partial DATA - sink.OnRequestSubmitted(); // final DATA+END_STREAM - bool pass = (sink.progress_calls == 2) && (sink.submitted_calls == 1); - TestFramework::RecordTest( - "H2Upstream N9b: OnRequestBodyProgress is dispatched (override observable)", - pass, pass ? "" : "progress=" + std::to_string(sink.progress_calls) + - " submitted=" + std::to_string(sink.submitted_calls)); + try { + auto cfg = MakeH2Conn(); + ObservingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9b: codec dispatches OnRequestBodyProgress on large body", + false, "Init failed"); + return; + } + // 20000 bytes > 1 frame (MAX_FRAME_SIZE=16384) — guarantees + // at least one intermediate DATA frame. + std::string body(20000, 'x'); + int32_t sid = conn.SubmitRequest( + "POST", "http", "example.com", "/upload", {}, body, &sink); + if (sid <= 0) { + TestFramework::RecordTest( + "H2Upstream N9b: codec dispatches OnRequestBodyProgress on large body", + false, "submit failed"); + return; + } + // After SubmitRequest's inline FlushSend, nghttp2 has emitted + // the HEADERS + DATA chunks. With a 20KB body, expect at + // least one OnRequestBodyProgress (intermediate DATA without + // END_STREAM) and exactly one OnRequestSubmitted (final DATA + // with END_STREAM). + bool pass = (sink.progress_calls >= 1) && + (sink.submitted_calls == 1); + TestFramework::RecordTest( + "H2Upstream N9b: codec dispatches OnRequestBodyProgress on large body", + pass, pass ? "" : "progress=" + std::to_string(sink.progress_calls) + + " submitted=" + std::to_string(sink.submitted_calls)); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9b: codec dispatches OnRequestBodyProgress on large body", + false, e.what()); + } } // --------------------------------------------------------------------------- -// TestN9c — Default sink does NOT need to override the new virtuals. -// Locks the ABI guarantee that adding OnRequestBodyProgress with a -// default no-op body did not break sink consumers that pre-date the -// virtual. +// TestN9c — Default sink ABI: a sink that does NOT override +// OnRequestBodyProgress must still compile and operate — locks the +// no-op default contract that prevents binary-compat breakage for +// pre-existing sink consumers. Drives a real submit-with-large-body +// so the codec's OnFrameSendCallback dispatches the new virtual +// against the unmodified RecordingSink (no override). // --------------------------------------------------------------------------- void TestN9cDefaultSinkSurvivesNewVirtual() { - std::cout << "\n[TEST] H2Upstream N9c: sink without OnRequestBodyProgress override compiles + runs..." << std::endl; - RecordingSink sink; // does NOT override OnRequestBodyProgress - // Must compile and run without crashing — default no-op virtual. - sink.OnRequestBodyProgress(); - sink.OnRequestSubmitted(); - bool pass = (sink.error_calls == 0) && (sink.complete_calls == 0); - TestFramework::RecordTest( - "H2Upstream N9c: sink without OnRequestBodyProgress override compiles + runs", - pass, ""); + std::cout << "\n[TEST] H2Upstream N9c: pre-existing sink survives codec firing OnRequestBodyProgress..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; // does NOT override OnRequestBodyProgress + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9c: pre-existing sink survives codec firing OnRequestBodyProgress", + false, "Init failed"); + return; + } + std::string body(20000, 'y'); + int32_t sid = conn.SubmitRequest( + "POST", "http", "example.com", "/upload", {}, body, &sink); + // The sink doesn't override OnRequestBodyProgress; the default + // no-op runs. Submit must succeed AND no spurious OnError must + // fire (the new virtual must not have side effects on the + // base implementation). + bool pass = (sid > 0) && (sink.error_calls == 0) && + (sink.complete_calls == 0); + TestFramework::RecordTest( + "H2Upstream N9c: pre-existing sink survives codec firing OnRequestBodyProgress", + pass, pass ? "" : "sid=" + std::to_string(sid) + + " errors=" + std::to_string(sink.error_calls)); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9c: pre-existing sink survives codec firing OnRequestBodyProgress", + false, e.what()); + } } // --------------------------------------------------------------------------- -// TestN7c — Send-stall budget when response_timeout_ms is disabled. -// Locks the SEND_STALL_FALLBACK_MS contract: response_timeout_ms == 0 -// opts out of the response-wait timer but the stall-phase hang -// protection stays on at SEND_STALL_FALLBACK_MS. +// TestN7c — H2 send-stall budget computation. Locks the public +// ProxyTransaction::ComputeH2StallBudgetMs contract: response_timeout_ms +// == 0 (operator-disabled) opts out of the response-wait timer but +// the stall-phase hang protection stays on at SEND_STALL_FALLBACK_MS. +// Negative values defensively fall through to the same fallback — +// config validation enforces non-negative, but a bug producing zero +// or negative must not produce a zero-or-negative budget that would +// either fire instantly or never. // --------------------------------------------------------------------------- void TestN7cSendStallFallbackBudget() { - std::cout << "\n[TEST] H2Upstream N7c: response_timeout_ms=0 → stall protection still on at SEND_STALL_FALLBACK_MS..." << std::endl; - // SEND_STALL_FALLBACK_MS is a public class-level constant. - bool pass = (ProxyTransaction::SEND_STALL_FALLBACK_MS == 30000); + std::cout << "\n[TEST] H2Upstream N7c: ComputeH2StallBudgetMs zero-disable contract..." << std::endl; + struct Case { int input; int expected; const char* label; }; + const Case cases[] = { + // Operator-disabled response timeout: stall protection STAYS + // on at fallback budget. This is the original P1-bug case. + {0, ProxyTransaction::SEND_STALL_FALLBACK_MS, "0 (disabled)"}, + // Defensive: negative values must not fire-instantly or + // never-fire — fall through to fallback. + {-1, ProxyTransaction::SEND_STALL_FALLBACK_MS, "-1 (defensive)"}, + {-1000, ProxyTransaction::SEND_STALL_FALLBACK_MS, "-1000 (defensive)"}, + // Positive: pass-through. + {1, 1, "1 (pass-through)"}, + {1000, 1000, "1000ms"}, + {30000, 30000, "30s explicit"}, + {120000, 120000, "120s explicit"}, + }; + int pass = 0, total = 0; + for (const Case& c : cases) { + const int got = ProxyTransaction::ComputeH2StallBudgetMs(c.input); + ++total; + if (got == c.expected) { + ++pass; + } else { + std::cerr << " FAIL[" << c.label << "]: input=" << c.input + << " got=" << got << " expected=" << c.expected + << std::endl; + } + } + // Constant-shape lock as the secondary check. + const bool fallback_correct = + (ProxyTransaction::SEND_STALL_FALLBACK_MS == 30000); + bool ok = (pass == total) && fallback_correct; TestFramework::RecordTest( - "H2Upstream N7c: SEND_STALL_FALLBACK_MS is 30000ms", - pass, pass ? "" : "actual=" + - std::to_string(ProxyTransaction::SEND_STALL_FALLBACK_MS)); + "H2Upstream N7c: ComputeH2StallBudgetMs zero-disable + fallback constant", + ok, ok ? "" : "passed " + std::to_string(pass) + "/" + + std::to_string(total) + ", fallback=" + + std::to_string(ProxyTransaction::SEND_STALL_FALLBACK_MS)); } // --------------------------------------------------------------------------- @@ -4679,7 +4782,7 @@ void RunAllH2UpstreamTests() { TestN6dTeTokenizerAcceptsParametersAndCases(); TestN7cSendStallFallbackBudget(); TestN8cNoPoisonOnEarlyHeadersSiblingReuse(); - TestN9bRequestBodyProgressIsObservable(); + TestN9bRequestBodyProgressFiresFromCodec(); TestN9cDefaultSinkSurvivesNewVirtual(); // TestB-series additions — wire-level From 67453e504ee3307e638ff05707866073ed32533d Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 00:24:42 +0800 Subject: [PATCH 04/14] Fix review comment --- include/upstream/proxy_transaction.h | 27 ++++++++ include/upstream/upstream_response_sink.h | 2 + server/proxy_transaction.cc | 44 ++++++++---- server/upstream_h2_connection.cc | 25 ++++--- test/h2_upstream_test.h | 82 +++++++++++++++++++++++ 5 files changed, 159 insertions(+), 21 deletions(-) diff --git a/include/upstream/proxy_transaction.h b/include/upstream/proxy_transaction.h index 071b4d9d..90f6398f 100644 --- a/include/upstream/proxy_transaction.h +++ b/include/upstream/proxy_transaction.h @@ -206,6 +206,10 @@ class ProxyTransaction }; State state_ = State::INIT; + // After H2 SubmitRequest failure rollback, state_ briefly reads + // SENDING_REQUEST while h2_path_ is already false. AttemptCheckout + // (retry path) and DeliverTerminalError (no-retry path) reset it; + // no live reader observes the gap. int attempt_ = 0; // Current attempt number (0 = first try) // Set by Cancel() — short-circuits checkout / retry / response // delivery paths so the transaction is torn down even if an @@ -373,6 +377,29 @@ class ProxyTransaction // Reset by Cleanup so retry attempts arm fresh. bool h2_response_timeout_armed_ = false; + // True once OnRequestSubmitted has fired. OnRequestBodyProgress + // gates refresh on this rather than state_ — request-side and + // response-side phases diverge on the early-final-headers path. + // Reset by DispatchH2 init + Cleanup. + bool h2_request_fully_sent_ = false; + + // Last time the H2 send-stall closure was armed. Used by + // OnRequestBodyProgress to debounce re-arming: closures are + // queued via EnQueueDelayed and live in the dispatcher's + // min-heap until their deadline expires. A naive per-DATA-frame + // re-arm produces O(num_data_frames) live closures (e.g. ~6400 + // dead entries for a 100 MB upload at 16 KB/frame). The + // debounce skips re-arm when less than budget/2 has elapsed, + // bounding the heap to at most 2 in-flight closures per + // transaction. Worst-case stall-detection latency: 1.5 * budget. + std::chrono::steady_clock::time_point h2_send_stall_armed_at_{}; + + // Cached send-stall budget for this attempt. Computed once in + // DispatchH2 from response_timeout_ms (with the zero-disable + // fallback) so OnRequestBodyProgress doesn't recompute per DATA + // frame. + int h2_stall_budget_ms_ = 0; + // H2 response timeout uses a dispatcher-scheduled task instead of a // transport-level deadline: the transport is shared across every // stream on the multiplexed session, so SetDeadline would tear down diff --git a/include/upstream/upstream_response_sink.h b/include/upstream/upstream_response_sink.h index 1a4a75a7..2dd14f12 100644 --- a/include/upstream/upstream_response_sink.h +++ b/include/upstream/upstream_response_sink.h @@ -4,6 +4,8 @@ namespace UPSTREAM_CALLBACKS_NAMESPACE { +// New virtuals must have default no-op bodies to preserve embedder +// ABI; pure-virtual additions break every existing sink subclass. class UpstreamResponseSink { public: virtual ~UpstreamResponseSink() = default; diff --git a/server/proxy_transaction.cc b/server/proxy_transaction.cc index 532864c1..a28d7045 100644 --- a/server/proxy_transaction.cc +++ b/server/proxy_transaction.cc @@ -962,10 +962,12 @@ void ProxyTransaction::DispatchH2() { h2_conn_weak_ = h2; state_ = State::SENDING_REQUEST; h2_response_timeout_armed_ = false; + h2_request_fully_sent_ = false; - const int stall_budget_ms = ComputeH2StallBudgetMs( + h2_stall_budget_ms_ = ComputeH2StallBudgetMs( config_.response_timeout_ms); - ArmH2SendStallDeadline(stall_budget_ms); + h2_send_stall_armed_at_ = std::chrono::steady_clock::now(); + ArmH2SendStallDeadline(h2_stall_budget_ms_); // Synchronous on_frame_send (bodyless path) may run inline here, // bumping h2_send_stall_generation_ → kills the closure above. @@ -984,6 +986,7 @@ void ProxyTransaction::DispatchH2() { ++h2_response_timeout_generation_; h2_path_ = false; h2_response_timeout_armed_ = false; + h2_request_fully_sent_ = false; h2_conn_weak_.reset(); logging::Get()->warn( "ProxyTransaction H2 submit failed client_fd={} service={} " @@ -1626,8 +1629,10 @@ void ProxyTransaction::OnRequestSubmitted() { if (cancelled_ || IsKilledForShutdown()) return; if (!h2_path_) return; // H1 infers send completion from socket drain - // Unconditional send-stall kill — the in-flight closure no-ops on - // generation mismatch when its delayed task eventually fires. + // Set BEFORE generation bump: a late OnRequestBodyProgress + // dispatched in the same callback chain sees the flag and + // skips re-arming the just-killed closure. + h2_request_fully_sent_ = true; ++h2_send_stall_generation_; // Only arm response-timeout if we're transitioning OUT of @@ -1649,14 +1654,19 @@ void ProxyTransaction::OnRequestSubmitted() { void ProxyTransaction::OnRequestBodyProgress() { if (cancelled_ || IsKilledForShutdown()) return; if (!h2_path_) return; - // Only refresh while the request is still being sent. Progress - // events that race with state transitions (state already moved - // to AWAITING_RESPONSE/RECEIVING_BODY) are no-ops. - if (state_ != State::SENDING_REQUEST) return; + if (h2_request_fully_sent_) return; - const int stall_budget_ms = ComputeH2StallBudgetMs( - config_.response_timeout_ms); - ArmH2SendStallDeadline(stall_budget_ms); + // Debounce: only re-arm if at least budget/2 has elapsed since + // the last arm. Without this, a large multi-frame upload would + // queue one EnQueueDelayed closure per DATA frame, accumulating + // O(num_frames) dead entries in the dispatcher's min-heap. + const auto now = std::chrono::steady_clock::now(); + const auto half_budget = + std::chrono::milliseconds(h2_stall_budget_ms_ / 2); + if (now - h2_send_stall_armed_at_ < half_budget) return; + + h2_send_stall_armed_at_ = now; + ArmH2SendStallDeadline(h2_stall_budget_ms_); } void ProxyTransaction::ArmH2SendStallDeadline(int budget_ms) { @@ -1684,8 +1694,15 @@ void ProxyTransaction::ArmH2SendStallDeadline(int budget_ms) { self->MaybeRetry( RetryPolicy::RetryCondition::RESPONSE_TIMEOUT); } else { - self->OnError(RESULT_RESPONSE_TIMEOUT, - "H2 send-stall timeout"); + // Unreachable — Cleanup bumps the generation before + // any terminal-state transition. Log loud and drop + // rather than fire OnError on a terminal transaction. + logging::Get()->error( + "ProxyTransaction H2 send-stall closure fired in " + "unexpected state={} client_fd={} service={} — " + "dropped (invariant break)", + static_cast(self->state_), + self->client_fd_, self->service_name_); } }, std::chrono::milliseconds(budget_ms)); @@ -2149,6 +2166,7 @@ void ProxyTransaction::Cleanup() { // first OnHeaders/OnRequestSubmitted would skip ArmResponseTimeout // because the flag is left over from the prior attempt). h2_response_timeout_armed_ = false; + h2_request_fully_sent_ = false; // Reset h2_path_ so a subsequent retry attempt that lands on H1 // (e.g. ALPN renegotiated, or the H2 connection died and prefer=auto's // next probe selects http/1.1) goes through the H1 lease-release diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index 737e93e0..2da1563e 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -220,11 +220,23 @@ int OnDataChunkRecvCallback(nghttp2_session* /*session*/, uint8_t /*flags*/, // the in_receive_data_ guard defers the inline FlushSend; the // post-receive flush in HandleBytes picks up the queued frame. using Framing = UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead::Framing; - if (stream->response_head.framing == Framing::NO_BODY && len > 0) { - stream->sink->OnError( - ProxyTransaction::RESULT_TRUNCATED_RESPONSE, - "body bytes on NO_BODY response"); + // Detach the sink BEFORE calling OnError. The sink's OnError can + // synchronously trigger ProxyTransaction's terminal path → Cleanup + // → h2->ResetStream, which both detaches the sink and submits + // RST_STREAM. Without pre-detach, the outer self->ResetStream + // call below would submit a SECOND nghttp2_submit_rst_stream + // against a stream nghttp2 has already torn down, producing a + // spurious warn log for every truncation/NO_BODY firing. + auto reject_truncation = [&](const char* msg) { + auto* sink = stream->sink; + stream->sink = nullptr; + if (sink) { + sink->OnError(ProxyTransaction::RESULT_TRUNCATED_RESPONSE, msg); + } self->ResetStream(stream_id); + }; + if (stream->response_head.framing == Framing::NO_BODY && len > 0) { + reject_truncation("body bytes on NO_BODY response"); return 0; } if (stream->response_head.framing == Framing::CONTENT_LENGTH && @@ -232,10 +244,7 @@ int OnDataChunkRecvCallback(nghttp2_session* /*session*/, uint8_t /*flags*/, static_cast(len) > stream->response_head.expected_length - stream->body_bytes_received) { - stream->sink->OnError( - ProxyTransaction::RESULT_TRUNCATED_RESPONSE, - "body exceeds Content-Length"); - self->ResetStream(stream_id); + reject_truncation("body exceeds Content-Length"); return 0; } diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index 6c15579c..59df4a84 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -4656,6 +4656,87 @@ void TestN7cSendStallFallbackBudget() { std::to_string(ProxyTransaction::SEND_STALL_FALLBACK_MS)); } +// --------------------------------------------------------------------------- +// TestN7e — Send-stall keeps refreshing on intermediate DATA frames +// after an early peer-final-headers transition. Submits a 30000-byte +// body (≥2 DATA frames) and feeds a 413+END_STREAM mid-upload; +// asserts the codec still dispatches OnRequestBodyProgress for +// intermediate DATA frames regardless of response-side state_. +// --------------------------------------------------------------------------- +void TestN7eRefreshContinuesAfterEarlyHeaders() { + std::cout << "\n[TEST] H2Upstream N7e: refresh continues after early-final-headers..." << std::endl; + struct ObservingSink : public RecordingSink { + int progress_calls_total = 0; + int progress_calls_post_headers = 0; + bool headers_seen = false; + bool OnHeaders( + const UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead& head) + override + { + const bool ok = RecordingSink::OnHeaders(head); + headers_seen = true; + return ok; + } + void OnRequestBodyProgress() override { + ++progress_calls_total; + if (headers_seen) { + ++progress_calls_post_headers; + } + } + }; + try { + auto cfg = MakeH2Conn(); + ObservingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N7e: refresh continues after early-final-headers", + false, "Init failed"); + return; + } + // 30000 bytes > MAX_FRAME_SIZE=16384 → guarantees ≥2 DATA frames. + std::string body(30000, 'z'); + int32_t sid = conn.SubmitRequest( + "POST", "http", "example.com", "/upload", {}, body, &sink); + if (sid <= 0) { + TestFramework::RecordTest( + "H2Upstream N7e: refresh continues after early-final-headers", + false, "submit failed sid=" + std::to_string(sid)); + return; + } + // After SubmitRequest's inline FlushSend, the codec has + // emitted HEADERS + at least one DATA chunk. Now feed an + // early peer-final-headers (413 + END_STREAM) on stream A — + // this is the canonical "peer rejects mid-upload" pattern. + std::vector wire = H2WireTest::BuildEmptySettings(); + auto early_hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "413"}, {"content-length", "0"}}, + /*end_stream=*/true); + wire.insert(wire.end(), early_hdrs.begin(), early_hdrs.end()); + ssize_t consumed = conn.HandleBytes( + reinterpret_cast(wire.data()), wire.size()); + + // The no-real-transport setup drains the body in the initial + // FlushSend before the early-headers wire bytes are fed, so + // the post-headers progress count is observable only with a + // real socketpair (out of scope here). We lock the wiring: + // headers are observed AND ≥1 intermediate DATA progress event + // fires. TestN9b/N9c lock the post-completion no-refresh side. + bool pass = (consumed > 0) && sink.headers_seen && + (sink.progress_calls_total >= 1); + TestFramework::RecordTest( + "H2Upstream N7e: refresh continues after early-final-headers", + pass, + pass ? "" : "consumed=" + std::to_string(consumed) + + " headers_seen=" + std::to_string(sink.headers_seen) + + " progress_total=" + std::to_string(sink.progress_calls_total)); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N7e: refresh continues after early-final-headers", + false, e.what()); + } +} + // --------------------------------------------------------------------------- // RunAll aggregator // --------------------------------------------------------------------------- @@ -4784,6 +4865,7 @@ void RunAllH2UpstreamTests() { TestN8cNoPoisonOnEarlyHeadersSiblingReuse(); TestN9bRequestBodyProgressFiresFromCodec(); TestN9cDefaultSinkSurvivesNewVirtual(); + TestN7eRefreshContinuesAfterEarlyHeaders(); // TestB-series additions — wire-level TestB15TrailersAfterDataEndStream(); From b5a08fbb561160d5831c5a81750e07e468f7aa14 Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 01:01:34 +0800 Subject: [PATCH 05/14] Fix review comment --- include/upstream/proxy_transaction.h | 41 +++++++++-------- server/proxy_transaction.cc | 68 ++++++++++++++++++---------- server/upstream_h2_connection.cc | 13 +++--- test/h2_upstream_test.h | 27 ++++++----- 4 files changed, 87 insertions(+), 62 deletions(-) diff --git a/include/upstream/proxy_transaction.h b/include/upstream/proxy_transaction.h index 90f6398f..ce441aa5 100644 --- a/include/upstream/proxy_transaction.h +++ b/include/upstream/proxy_transaction.h @@ -187,13 +187,22 @@ class ProxyTransaction static constexpr int SEND_STALL_FALLBACK_MS = 30000; // 30s private: - // Bump h2_send_stall_generation_ and queue a fresh delayed - // closure that fires after `budget_ms` if not invalidated. Used - // both from DispatchH2 to arm the initial stall and from - // OnRequestBodyProgress to refresh on each request-body DATA - // flush, mirroring H1's per-write SetWriteProgressCb refresh. + // Bump h2_send_stall_generation_ and queue a fresh send-stall + // closure for the full budget. Called from DispatchH2 at attempt + // start. OnRequestBodyProgress does NOT call this directly — + // refreshes flow through h2_last_progress_at_ + the closure's + // self-rescheduling check. void ArmH2SendStallDeadline(int budget_ms); + // Queue (or re-queue) the send-stall closure with the given + // generation and delay. Called by ArmH2SendStallDeadline (initial + // arm with a fresh generation) and by the closure itself on + // observed progress (re-queue with the current generation for the + // remaining budget). Same-generation re-queue is correct because + // Cleanup / OnRequestSubmitted bump the generation, invalidating + // any in-flight closure regardless of who queued it. + void QueueH2SendStallClosure(uint64_t generation, int delay_ms); + // State machine states enum class State { INIT, // Created, not yet started @@ -383,21 +392,17 @@ class ProxyTransaction // Reset by DispatchH2 init + Cleanup. bool h2_request_fully_sent_ = false; - // Last time the H2 send-stall closure was armed. Used by - // OnRequestBodyProgress to debounce re-arming: closures are - // queued via EnQueueDelayed and live in the dispatcher's - // min-heap until their deadline expires. A naive per-DATA-frame - // re-arm produces O(num_data_frames) live closures (e.g. ~6400 - // dead entries for a 100 MB upload at 16 KB/frame). The - // debounce skips re-arm when less than budget/2 has elapsed, - // bounding the heap to at most 2 in-flight closures per - // transaction. Worst-case stall-detection latency: 1.5 * budget. - std::chrono::steady_clock::time_point h2_send_stall_armed_at_{}; + // Last time the H2 codec emitted a request-side DATA frame. + // Updated by OnRequestBodyProgress; inspected by the single + // in-flight send-stall closure on fire. The closure re-queues + // itself for the remaining budget if progress was observed, + // otherwise it fires the timeout. This keeps the dispatcher's + // min-heap bounded to one closure per request regardless of + // upload size, while preserving refresh-on-every-DATA semantics. + std::chrono::steady_clock::time_point h2_last_progress_at_{}; // Cached send-stall budget for this attempt. Computed once in - // DispatchH2 from response_timeout_ms (with the zero-disable - // fallback) so OnRequestBodyProgress doesn't recompute per DATA - // frame. + // DispatchH2 so the closure's progress check doesn't recompute. int h2_stall_budget_ms_ = 0; // H2 response timeout uses a dispatcher-scheduled task instead of a diff --git a/server/proxy_transaction.cc b/server/proxy_transaction.cc index a28d7045..377b44a9 100644 --- a/server/proxy_transaction.cc +++ b/server/proxy_transaction.cc @@ -966,7 +966,7 @@ void ProxyTransaction::DispatchH2() { h2_stall_budget_ms_ = ComputeH2StallBudgetMs( config_.response_timeout_ms); - h2_send_stall_armed_at_ = std::chrono::steady_clock::now(); + h2_last_progress_at_ = std::chrono::steady_clock::now(); ArmH2SendStallDeadline(h2_stall_budget_ms_); // Synchronous on_frame_send (bodyless path) may run inline here, @@ -1655,38 +1655,56 @@ void ProxyTransaction::OnRequestBodyProgress() { if (cancelled_ || IsKilledForShutdown()) return; if (!h2_path_) return; if (h2_request_fully_sent_) return; - - // Debounce: only re-arm if at least budget/2 has elapsed since - // the last arm. Without this, a large multi-frame upload would - // queue one EnQueueDelayed closure per DATA frame, accumulating - // O(num_frames) dead entries in the dispatcher's min-heap. - const auto now = std::chrono::steady_clock::now(); - const auto half_budget = - std::chrono::milliseconds(h2_stall_budget_ms_ / 2); - if (now - h2_send_stall_armed_at_ < half_budget) return; - - h2_send_stall_armed_at_ = now; - ArmH2SendStallDeadline(h2_stall_budget_ms_); + // Pure-timestamp refresh: the in-flight send-stall closure + // inspects this on fire and re-queues itself if progress was + // observed. No EnQueueDelayed call here — the heap stays at + // one closure per request regardless of upload size. + h2_last_progress_at_ = std::chrono::steady_clock::now(); } void ProxyTransaction::ArmH2SendStallDeadline(int budget_ms) { - if (!dispatcher_) return; const uint64_t send_stall_gen = ++h2_send_stall_generation_; + QueueH2SendStallClosure(send_stall_gen, budget_ms); +} + +void ProxyTransaction::QueueH2SendStallClosure(uint64_t generation, + int delay_ms) { + if (!dispatcher_) return; std::weak_ptr weak_self = weak_from_this(); dispatcher_->EnQueueDelayed( - [weak_self, send_stall_gen]() { + [weak_self, generation]() { auto self = weak_self.lock(); if (!self) return; if (self->cancelled_) return; - if (send_stall_gen != self->h2_send_stall_generation_) return; - // H2 send-stall is a TIMEOUT, not a disconnect: the peer - // is still connected but isn't draining our request body. - // Mirrors H1's send-stall semantics (ArmResponseTimeout - // with stall_budget; transport SetDeadlineTimeoutCb fires - // RESULT_RESPONSE_TIMEOUT on miss). Route through the - // retryable-timeout path so retry_on_timeout policies - // apply and the client sees 504 (not 502) on retry- - // exhausted. + if (generation != self->h2_send_stall_generation_) return; + + // Progress check: if we've seen a DATA flush within the + // budget, the upload is healthy — re-queue ourselves + // for the remaining time without bumping the generation. + // Cleanup / OnRequestSubmitted will still invalidate us + // by bumping the generation; the same-generation re- + // queue stays valid until they do. + const auto now = std::chrono::steady_clock::now(); + const auto budget = std::chrono::milliseconds( + self->h2_stall_budget_ms_); + const auto since_progress = now - self->h2_last_progress_at_; + if (since_progress < budget) { + const auto remaining = + std::chrono::duration_cast( + budget - since_progress); + // Clamp to at least 1ms so we don't busy-loop on + // floating-point edge cases (since_progress == 0). + const int remaining_ms = std::max( + 1, static_cast(remaining.count())); + self->QueueH2SendStallClosure(generation, remaining_ms); + return; + } + + // Real stall: peer connected but not draining body. + // Surface as RESULT_RESPONSE_TIMEOUT to mirror H1's + // SetDeadline-driven semantic; route through the + // retryable-timeout path so retry_on_timeout applies and + // the client sees 504, not 502. if (self->state_ == State::SENDING_REQUEST || self->state_ == State::AWAITING_RESPONSE || self->state_ == State::RECEIVING_BODY) { @@ -1705,7 +1723,7 @@ void ProxyTransaction::ArmH2SendStallDeadline(int budget_ms) { self->client_fd_, self->service_name_); } }, - std::chrono::milliseconds(budget_ms)); + std::chrono::milliseconds(delay_ms)); } void ProxyTransaction::DeliverTerminalError(int result_code, diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index 2da1563e..4de17209 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -220,13 +220,12 @@ int OnDataChunkRecvCallback(nghttp2_session* /*session*/, uint8_t /*flags*/, // the in_receive_data_ guard defers the inline FlushSend; the // post-receive flush in HandleBytes picks up the queued frame. using Framing = UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead::Framing; - // Detach the sink BEFORE calling OnError. The sink's OnError can - // synchronously trigger ProxyTransaction's terminal path → Cleanup - // → h2->ResetStream, which both detaches the sink and submits - // RST_STREAM. Without pre-detach, the outer self->ResetStream - // call below would submit a SECOND nghttp2_submit_rst_stream - // against a stream nghttp2 has already torn down, producing a - // spurious warn log for every truncation/NO_BODY firing. + // Detach the sink BEFORE calling OnError so the synchronous + // teardown chain (ProxyTransaction OnError → Cleanup → h2-> + // ResetStream) does not re-dispatch sink->OnError on an + // already-failed path. The outer self->ResetStream below is the + // safety net for paths where Cleanup doesn't run; nghttp2 dedups + // the duplicate RST submission silently in production. auto reject_truncation = [&](const char* msg) { auto* sink = stream->sink; stream->sink = nullptr; diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index 59df4a84..2e097d50 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -4657,14 +4657,17 @@ void TestN7cSendStallFallbackBudget() { } // --------------------------------------------------------------------------- -// TestN7e — Send-stall keeps refreshing on intermediate DATA frames -// after an early peer-final-headers transition. Submits a 30000-byte -// body (≥2 DATA frames) and feeds a 413+END_STREAM mid-upload; -// asserts the codec still dispatches OnRequestBodyProgress for -// intermediate DATA frames regardless of response-side state_. +// TestN7e — Wire-driven smoke test that early-peer-final-headers does +// not GOAWAY/RST the connection nor stop intermediate-DATA codec +// dispatch on the request side. Locks the codec wiring; the deeper +// "no false stall after early headers" semantic is enforced at the +// production layer by gating OnRequestBodyProgress on +// h2_request_fully_sent_ rather than response-side state_, and by +// the timestamp-driven self-rescheduling closure (no test in this +// file exercises the timing under wall-clock load). // --------------------------------------------------------------------------- -void TestN7eRefreshContinuesAfterEarlyHeaders() { - std::cout << "\n[TEST] H2Upstream N7e: refresh continues after early-final-headers..." << std::endl; +void TestN7eWiringEarlyHeadersThenIntermediateDataDispatch() { + std::cout << "\n[TEST] H2Upstream N7e: wire-level dispatch after early peer-final-headers..." << std::endl; struct ObservingSink : public RecordingSink { int progress_calls_total = 0; int progress_calls_post_headers = 0; @@ -4690,7 +4693,7 @@ void TestN7eRefreshContinuesAfterEarlyHeaders() { UpstreamH2Connection conn(nullptr, cfg); if (!conn.Init()) { TestFramework::RecordTest( - "H2Upstream N7e: refresh continues after early-final-headers", + "H2Upstream N7e: wire-level dispatch after early peer-final-headers", false, "Init failed"); return; } @@ -4700,7 +4703,7 @@ void TestN7eRefreshContinuesAfterEarlyHeaders() { "POST", "http", "example.com", "/upload", {}, body, &sink); if (sid <= 0) { TestFramework::RecordTest( - "H2Upstream N7e: refresh continues after early-final-headers", + "H2Upstream N7e: wire-level dispatch after early peer-final-headers", false, "submit failed sid=" + std::to_string(sid)); return; } @@ -4725,14 +4728,14 @@ void TestN7eRefreshContinuesAfterEarlyHeaders() { bool pass = (consumed > 0) && sink.headers_seen && (sink.progress_calls_total >= 1); TestFramework::RecordTest( - "H2Upstream N7e: refresh continues after early-final-headers", + "H2Upstream N7e: wire-level dispatch after early peer-final-headers", pass, pass ? "" : "consumed=" + std::to_string(consumed) + " headers_seen=" + std::to_string(sink.headers_seen) + " progress_total=" + std::to_string(sink.progress_calls_total)); } catch (const std::exception& e) { TestFramework::RecordTest( - "H2Upstream N7e: refresh continues after early-final-headers", + "H2Upstream N7e: wire-level dispatch after early peer-final-headers", false, e.what()); } } @@ -4865,7 +4868,7 @@ void RunAllH2UpstreamTests() { TestN8cNoPoisonOnEarlyHeadersSiblingReuse(); TestN9bRequestBodyProgressFiresFromCodec(); TestN9cDefaultSinkSurvivesNewVirtual(); - TestN7eRefreshContinuesAfterEarlyHeaders(); + TestN7eWiringEarlyHeadersThenIntermediateDataDispatch(); // TestB-series additions — wire-level TestB15TrailersAfterDataEndStream(); From 28af3cc5599d15b06b879e4192723445f31cce3e Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 10:49:37 +0800 Subject: [PATCH 06/14] Fix review comment --- include/upstream/proxy_transaction.h | 4 +- include/upstream/upstream_response_sink.h | 50 ++-------- server/proxy_transaction.cc | 107 ++++++++-------------- server/upstream_h2_connection.cc | 63 ++++--------- 4 files changed, 67 insertions(+), 157 deletions(-) diff --git a/include/upstream/proxy_transaction.h b/include/upstream/proxy_transaction.h index ce441aa5..bd6117b3 100644 --- a/include/upstream/proxy_transaction.h +++ b/include/upstream/proxy_transaction.h @@ -46,8 +46,8 @@ class ProxyTransaction static constexpr int RESULT_POOL_EXHAUSTED = -6; // Local capacity → 503 static constexpr int RESULT_RESPONSE_TOO_LARGE = -9; // Local buffering cap → 502 // Circuit breaker rejected this attempt before it touched the upstream. - // Carries Retry-After + X-Circuit-Breaker headers (§12.1). - // Terminal — retry loop MUST NOT retry this outcome (§8). + // Carries Retry-After + X-Circuit-Breaker headers. + // Terminal — retry loop MUST NOT retry this outcome. static constexpr int RESULT_CIRCUIT_OPEN = -7; // Retry budget exhausted. No Retry-After; distinct header // X-Retry-Budget-Exhausted so operators can tell the two 503s apart diff --git a/include/upstream/upstream_response_sink.h b/include/upstream/upstream_response_sink.h index 2dd14f12..14dbbc7a 100644 --- a/include/upstream/upstream_response_sink.h +++ b/include/upstream/upstream_response_sink.h @@ -22,51 +22,15 @@ class UpstreamResponseSink { virtual void OnComplete() = 0; virtual void OnError(int error_code, const std::string& message) = 0; - // Fired when the upstream stream's request side is fully sent - // (END_STREAM flag observed on either DATA or HEADERS — bodyless - // requests trigger END_STREAM on HEADERS). Sinks use this to swap - // a per-stream send-stall deadline for a response-completion - // deadline. - // - // H2 path only. The H1 path infers send completion from socket - // buffer drain and does not call this. Default no-op so non-H2 - // sinks can ignore. - // - // Invocation timing: may fire SYNCHRONOUSLY from within - // UpstreamH2Connection::SubmitRequest for bodyless requests where - // nghttp2 inline-flushes the HEADERS+END_STREAM frame. Sinks MUST - // be safe to receive this callback before SubmitRequest returns - // control to the caller. + // Fired on END_STREAM (HEADERS for bodyless, DATA otherwise). H2 + // only. May fire synchronously from within SubmitRequest for + // bodyless requests — sinks must be reentrant relative to submit. virtual void OnRequestSubmitted() {} - // Fired when an intermediate request-side DATA frame is flushed - // to the wire (END_STREAM not yet set). Sinks use this to refresh - // the per-stream send-stall deadline so a slow-but-progressing - // upload — body larger than the stall budget but the peer is still - // consuming — is not falsely classified as a stall. Mirrors the - // H1 transport-level SetWriteProgressCb refresh. - // - // H2 path only. Default no-op. - // - // Contract details for sink implementers: - // * Fires once per intermediate DATA frame the codec actually - // emits to the wire. If nghttp2 coalesces two writes into one - // DATA frame, only one callback fires; if the body is large - // enough to span multiple DATA frames at MAX_FRAME_SIZE, one - // callback fires per frame. - // * NOT a 1:1 byte-progress signal — do not use it for byte - // accounting (use OnBodyChunk's len for that on the response - // side; the request side has no analogous accounting hook). - // * Ordering relative to OnHeaders/OnRequestSubmitted is the - // wire order: progress events on stream N can fire before or - // after response-side callbacks for stream N depending on - // peer scheduling. - // * Production sinks (ProxyTransaction) gate refresh logic on - // their own state machine — progress events outside the - // SENDING_REQUEST phase are silently dropped at the sink. The - // C-shim DOES dispatch unconditionally, so other sinks must - // gate similarly OR be safe under arrival of progress events - // at any time relative to other callbacks. + // Fired per intermediate request-side DATA frame on the wire (not + // a per-byte signal — frame coalescing collapses adjacent writes). + // H2 only. Sinks should gate refresh logic on their own state + // machine; the codec dispatches unconditionally. virtual void OnRequestBodyProgress() {} }; diff --git a/server/proxy_transaction.cc b/server/proxy_transaction.cc index 377b44a9..94188376 100644 --- a/server/proxy_transaction.cc +++ b/server/proxy_transaction.cc @@ -504,8 +504,8 @@ bool ProxyTransaction::PrepareAttemptAdmission() { // Circuit breaker gate — consulted before every attempt (first try and // retries both). Each attempt gets a fresh admission stamped with the // slice's current generation. If the slice rejects with REJECTED_OPEN, - // ConsultBreaker delivers the §12.1 response and returns false; the - // retry loop treats RESULT_CIRCUIT_OPEN as terminal (§8) so a rejected + // ConsultBreaker delivers the circuit-open response and returns false; + // the retry loop treats RESULT_CIRCUIT_OPEN as terminal so a rejected // retry produces a single 503 to the client, not a nested retry. // Dry-run reject logs inside TryAcquire and returns ADMITTED through // the decision enum (REJECTED_OPEN_DRYRUN), so ConsultBreaker proceeds. @@ -945,19 +945,13 @@ void ProxyTransaction::DispatchH2() { path_with_query.append(query_); } - // Initialize H2 state BEFORE h2->SubmitRequest. SubmitRequest may - // synchronously fire on_frame_send_callback (via FlushSend) for - // bodyless requests where nghttp2 inline-flushes HEADERS+END_STREAM, - // dispatching OnRequestSubmitted INSIDE the call. If we deferred - // h2_path_/state_/generation init to after the call, the - // OnRequestSubmitted override's `if (!h2_path_) return;` guard - // would silently drop the kill of the send-stall closure → spurious - // OnError(UPSTREAM_DISCONNECT) ~30s later. - // - // Send-stall budget mirrors the H1 zero-disable semantic in the - // H1 send loop above: response_timeout_ms == 0 opts out of the - // response-wait timer but the stall protection stays on, falling - // back to SEND_STALL_FALLBACK_MS. + // Initialize H2 state BEFORE SubmitRequest. Bodyless requests can + // inline-flush HEADERS+END_STREAM and fire OnRequestSubmitted + // synchronously; the override's `!h2_path_` guard would otherwise + // drop the kill of the just-queued send-stall closure. + // Budget mirrors the H1 zero-disable semantic: response_timeout_ms + // == 0 opts out of the response-wait timer; stall protection stays + // on via SEND_STALL_FALLBACK_MS. h2_path_ = true; h2_conn_weak_ = h2; state_ = State::SENDING_REQUEST; @@ -1038,8 +1032,8 @@ void ProxyTransaction::OnCheckoutError(int error_code) { if (error_code == CIRCUIT_OPEN) { // Drain path: breaker tripped while this transaction was queued. // Do NOT Report success/failure to the slice — our own reject - // must not feed back into the failure math. Emit the §12.1 - // circuit-open response directly. + // must not feed back into the failure math. Emit the circuit-open + // response (Retry-After + X-Circuit-Breaker headers) directly. logging::Get()->info( "ProxyTransaction checkout drained by circuit breaker " "client_fd={} service={}", @@ -1323,47 +1317,27 @@ bool ProxyTransaction::OnHeaders( } if (h2_path_) { - // H2 final-headers branch. The send-stall closure stays armed - // until OnRequestSubmitted (or Cleanup) bumps the generation — - // an early peer-final-headers (e.g. 413) before our END_STREAM - // is sent does not cancel the stall protection. Response timer - // is armed at the FIRST of {OnHeaders, OnRequestSubmitted} via - // the arm-once flag; whichever fires later sees armed=true and - // skips the redundant arm. - // - // No poison_connection_=true here on the early-final-headers - // path: H1 poisons because its transport-sharing model - // contaminates subsequent keep-alive use, but H2 multiplexes - // streams over a single transport — an early 413 on one stream - // is not a fatal upstream signal and should not block sibling - // stream reuse. + // H2: stall closure stays armed until OnRequestSubmitted / + // Cleanup bumps the generation — an early peer-final-headers + // (e.g. 413) before our END_STREAM keeps stall protection. No + // poison_connection_ here: H2 multiplexes streams, so an early + // status on one stream is not a transport-fatal signal. if (state_ == State::SENDING_REQUEST) { state_ = State::AWAITING_RESPONSE; } - // Final headers in hand → header-phase timer (T1) is done. - // Body-phase timing is governed by stream_idle_timeout_sec / - // stream_max_duration_sec, NOT by response_timeout_ms. Clear - // any armed closure (no-op if OnRequestSubmitted hasn't run - // yet) and reset the arm-once flag so a later - // OnRequestSubmitted's was_sending guard correctly skips - // re-arming. Mirrors H1's ClearResponseTimeout below. + // Header phase done; body phase is governed by stream timers. + // Reset the arm-once flag so a later OnRequestSubmitted skips + // re-arming. ClearResponseTimeout(); h2_response_timeout_armed_ = false; } else { - // H1 path: existing behavior preserved verbatim. if (state_ == State::SENDING_REQUEST) { - // Early response: the request write is no longer the active - // phase. If the upstream later finishes flushing the request - // bytes, that callback must not re-arm the response-header - // timer or move us back into the pre-headers state machine. + // Early response: subsequent request-write completion must + // not re-arm the header timer or move us back to the + // pre-headers state. state_ = State::AWAITING_RESPONSE; poison_connection_ = true; } - // T1 is complete once the response head arrives. Body-phase - // timing uses the dedicated T2/T3 stream timers. - // Note: H1 has no `h2_response_timeout_armed_` reset because - // the flag is H2-only — the H1 path uses transport-level - // SetDeadline cleared by ClearResponseTimeout itself. ClearResponseTimeout(); } @@ -1646,7 +1620,13 @@ void ProxyTransaction::OnRequestSubmitted() { state_ = State::AWAITING_RESPONSE; } if (was_sending && !h2_response_timeout_armed_) { - ArmResponseTimeout(); + // Pass the cached fallback budget explicitly. When + // response_timeout_ms == 0 (operator-disabled), ArmResponseTimeout() + // without an explicit budget is a no-op and the post-submit + // watchdog would vanish — leaving a transport-stuck upload + // governed only by PING liveness. Mirrors the H1 send-loop + // pattern which also passes a non-zero stall budget. + ArmResponseTimeout(h2_stall_budget_ms_); h2_response_timeout_armed_ = true; } } @@ -1675,7 +1655,7 @@ void ProxyTransaction::QueueH2SendStallClosure(uint64_t generation, [weak_self, generation]() { auto self = weak_self.lock(); if (!self) return; - if (self->cancelled_) return; + if (self->cancelled_ || self->IsKilledForShutdown()) return; if (generation != self->h2_send_stall_generation_) return; // Progress check: if we've seen a DATA flush within the @@ -2904,23 +2884,15 @@ HttpResponse ProxyTransaction::MakeErrorResponse(int result_code) { return MakeRetryBudgetResponse(); } if (result_code == RESULT_CIRCUIT_OPEN) { - // The static factory has no `this`, so it cannot build the - // fully §12.1-compliant response (Retry-After derived from - // slice state, X-Upstream-Host). All in-class paths for - // CIRCUIT_OPEN use the non-static MakeCircuitOpenResponse() - // — reaching this branch means a future caller forgot that - // rule. Log loudly so the mistake shows up in logs instead - // of producing a stealth regression against the contract. - // - // Still emit `X-Circuit-Breaker: open` + `Connection: close` - // so the response remains self-identifying as a circuit-open - // reject. Clients inspecting that header will correctly back - // off via their own client-side logic rather than treating - // this as an anonymous 503. + // Static factory has no `this`, so it cannot derive Retry-After + // from slice state or attach X-Upstream-Host. All in-class paths + // use the non-static MakeCircuitOpenResponse(); reaching this + // branch means a future caller forgot. Log loud and emit the + // self-identifying headers we can build without context. logging::Get()->error( "ProxyTransaction::MakeErrorResponse(RESULT_CIRCUIT_OPEN) " "invoked from static context — use MakeCircuitOpenResponse() " - "to emit §12.1-compliant headers"); + "to emit full circuit-open headers"); HttpResponse resp = HttpResponse::ServiceUnavailable(); resp.Header("X-Circuit-Breaker", "open"); resp.Header("Connection", "close"); @@ -3016,9 +2988,8 @@ HttpResponse ProxyTransaction::MakeCircuitOpenResponse() const { // Hint operators (not clients) at which upstream tripped. Useful // when a gateway fronts multiple backends; without this header, a // 503 is opaque. - // §5.5.1: render authority via FormatAuthority so IPv6 literals get - // RFC 3986 §3.2.2 bracket wrapping. Byte-identical for hostnames - // and IPv4 literals. + // Render authority via FormatAuthority so IPv6 literals get RFC 3986 + // §3.2.2 bracket wrapping. Byte-identical for hostnames and IPv4. resp.Header("X-Upstream-Host", NET_DNS_NAMESPACE::DnsResolver::FormatAuthority( upstream_host_, upstream_port_, /*omit_port=*/false)); @@ -3055,7 +3026,7 @@ bool ProxyTransaction::ConsultBreaker() { if (admission.decision == CIRCUIT_BREAKER_NAMESPACE::Decision::REJECTED_OPEN) { // Hard reject — slice counted it, logged it, and we must not - // touch the upstream. Emit §12.1 response and DO NOT Report + // touch the upstream. Emit circuit-open response and DO NOT Report // back (would create a feedback loop — our own reject counting // as a failure against the already-OPEN slice). if (ResumeHeldRetryable5xxResponse("circuit_open")) { diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index 4de17209..91a82e0d 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -203,29 +203,19 @@ int OnDataChunkRecvCallback(nghttp2_session* /*session*/, uint8_t /*flags*/, int32_t stream_id, const uint8_t* data, size_t len, void* user_data) { - // Look up via the connection's own stream table rather than casting - // nghttp2's raw user_data pointer directly. auto* self = static_cast(user_data); auto* stream = self->GetStream(stream_id); if (!stream || !stream->sink) return 0; - // Defense-in-depth pre-dispatch validation. In production this code - // is unreachable: nghttp2's HTTP messaging enforcement (default-on) - // intercepts NO_BODY and Content-Length violations before this - // callback fires, routing them through OnStreamClose with a - // non-NO_ERROR code. These checks remain active as a backstop for - // future code paths that opt out of enforcement (e.g. - // nghttp2_option_set_no_http_messaging for streaming protocols). - // RST through ResetStream (not nghttp2_submit_rst_stream directly) so - // the in_receive_data_ guard defers the inline FlushSend; the - // post-receive flush in HandleBytes picks up the queued frame. + // Defense-in-depth: nghttp2's HTTP-messaging enforcement normally + // catches NO_BODY / Content-Length violations before we get here. + // Kept active as a backstop for callers that opt out of enforcement. + // ResetStream (not raw nghttp2_submit_rst_stream) so in_receive_data_ + // defers the inline FlushSend until the post-receive flush. using Framing = UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead::Framing; - // Detach the sink BEFORE calling OnError so the synchronous - // teardown chain (ProxyTransaction OnError → Cleanup → h2-> - // ResetStream) does not re-dispatch sink->OnError on an - // already-failed path. The outer self->ResetStream below is the - // safety net for paths where Cleanup doesn't run; nghttp2 dedups - // the duplicate RST submission silently in production. + // Detach sink before OnError so a synchronous teardown chain + // (sink OnError → Cleanup → ResetStream) does not re-dispatch on + // an already-failed path. auto reject_truncation = [&](const char* msg) { auto* sink = stream->sink; stream->sink = nullptr; @@ -252,22 +242,10 @@ int OnDataChunkRecvCallback(nghttp2_session* /*session*/, uint8_t /*flags*/, return 0; } -// Fired when nghttp2 actually puts a frame on the wire (via -// FlushSend → nghttp2_session_mem_send2). Two request-side signals -// matter to the sink: -// * END_STREAM on HEADERS (bodyless) or DATA (final frame) → -// OnRequestSubmitted: request fully sent, swap send-stall for -// response-completion deadline. -// * Intermediate DATA frame (no END_STREAM) → OnRequestBodyProgress: -// refresh the send-stall budget so a slow-but-progressing upload -// mirrors H1's transport-level SetWriteProgressCb behavior. -// -// May fire SYNCHRONOUSLY inside SubmitRequest when nghttp2 inline- -// flushes a bodyless HEADERS+END_STREAM frame; the sink contract -// (OnRequestSubmitted docstring on UpstreamResponseSink) requires -// callers to be safe under that ordering. ProxyTransaction handles -// the synchronous case by initializing all H2 state BEFORE invoking -// h2->SubmitRequest. +// Dispatches request-side sink signals from frame-serialization edges. +// END_STREAM (HEADERS or DATA) → OnRequestSubmitted; intermediate DATA +// → OnRequestBodyProgress. May fire SYNCHRONOUSLY inside SubmitRequest +// for bodyless requests — sink contract requires reentrancy. int OnFrameSendCallback(nghttp2_session* /*session*/, const nghttp2_frame* frame, void* user_data) { @@ -282,9 +260,8 @@ int OnFrameSendCallback(nghttp2_session* /*session*/, if (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { stream->sink->OnRequestSubmitted(); } else if (frame->hd.type == NGHTTP2_DATA) { - // HEADERS without END_STREAM is the request-headers frame for - // a bodied request; the upcoming DATA frames are the progress - // signal, not the HEADERS itself. Only DATA frames refresh. + // Only DATA frames are body-progress signals; the HEADERS frame + // itself (no END_STREAM, bodied request) is request-header data. stream->sink->OnRequestBodyProgress(); } return 0; @@ -807,13 +784,11 @@ int32_t UpstreamH2Connection::SubmitRequest( { if (!IsUsable()) return -1; - // Secondary CONNECT-rejection gate. The primary gate in - // ProxyTransaction::DispatchH2 catches CONNECT before it reaches the - // codec, but a future code path bypassing DispatchH2 (or a unit test - // exercising this method directly) would otherwise emit a malformed - // H2 request. nghttp2 is left untouched: no stream allocation, no - // submit, no frame queue. Sink receives the deterministic policy - // reject so the caller's MaybeRetry sees a terminal code. + // Secondary CONNECT-rejection gate. Primary gate lives in + // ProxyTransaction::DispatchH2; this catches direct callers that + // bypass it (unit tests / future code paths). Sink->OnError fires + // here, so callers that also run their own rollback on a -1 return + // must be idempotent on the error path. if (method == "CONNECT") { const std::string host = transport_ ? transport_->upstream_host() : std::string("?"); From e1be7461ec8f92d0f1bfd98ad7613313d527cc79 Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 13:00:00 +0800 Subject: [PATCH 07/14] Fix review comment --- docs/http2_upstream.md | 2 +- include/upstream/upstream_h2_connection.h | 51 ++++ server/pool_partition.cc | 19 ++ server/proxy_transaction.cc | 15 +- server/upstream_h2_connection.cc | 140 ++++++++++- test/h2_upstream_test.h | 287 ++++++++++++++++++---- 6 files changed, 441 insertions(+), 73 deletions(-) diff --git a/docs/http2_upstream.md b/docs/http2_upstream.md index e0448810..5ed13c02 100644 --- a/docs/http2_upstream.md +++ b/docs/http2_upstream.md @@ -140,4 +140,4 @@ The defaults are conservative and work for most deployments. Tune only if you ob - **Per-stream backpressure is not strictly bounded by `initial_window_size`** — the proxy lets nghttp2 manage stream-level flow control with auto-`WINDOW_UPDATE` enabled, so the upstream's effective window is continuously refreshed as bytes are delivered to the on-data-chunk callback. In practice per-stream upstream buffering tracks the auto-update cadence (~`initial_window_size` worth of bytes outstanding under steady traffic) plus the `StreamingResponseSender` high-water mark on the downstream side, but it is not a hard cap: a slow downstream client paired with a fast H2 upstream can buffer somewhat more depending on `MAX_FRAME_SIZE` and how quickly chunks are read. For workloads with bursty downstream stalls and a high `initial_window_size`, watch RSS and consider lowering the window size. A future refinement will disable auto-update and pause per-stream consumption via `nghttp2_session_consume_stream` to enforce a strict cap. - **CONNECT method is rejected** with 502 + `X-H2-Limitation: connect-not-supported`. The H2 codec emits `:scheme` and `:path` on every request, which RFC 9113 §8.5 forbids on CONNECT pseudo-headers; rather than emit a malformed request, the gateway rejects deterministically. Use an H1 upstream for CONNECT tunnelling. - **Truncation observability** — when a backend declares `Content-Length` and closes early, when it returns body bytes on a `204 No Content` / `304 Not Modified` / `HEAD` response that should have no body, or when it sends more bytes than `Content-Length` declared, nghttp2's HTTP messaging enforcement detects the violation and the gateway surfaces it as `RESULT_UPSTREAM_DISCONNECT` (the same bucket as a torn TCP connection). A dedicated `RESULT_TRUNCATED_RESPONSE` code exists in the binary for defense-in-depth but is not normally observable in production. If you need to distinguish "peer reset / TCP drop" from "framing violation", correlate by upstream-side response logs. Truncated responses count toward circuit-breaker upstream-failure totals via the `RESULT_TRUNCATED_RESPONSE` → `UPSTREAM_DISCONNECT` `FailureKind` mapping. -- **H2 send-stall is a timeout** — if the request body is large enough to span multiple DATA frames, the per-stream send-stall budget refreshes on each DATA flush (mirroring H1's `SetWriteProgressCb` behavior). A truly stalled upload — peer's flow-control window drained and not refreshing for `response_timeout_ms` (or `30s` if disabled) — surfaces as `RESULT_RESPONSE_TIMEOUT` (504), not `RESULT_UPSTREAM_DISCONNECT` (502). This matches H1's send-stall semantics and routes through the retryable-timeout path so `retry_on_timeout` policies apply. +- **H2 send-stall is a timeout** — the per-stream send-stall budget refreshes on each DATA frame that actually drains off the transport buffer (not when nghttp2 serializes a frame into its internal buffer). The gateway tracks every outbound HEADERS/DATA frame in a per-session drain queue inside `UpstreamH2Connection`; the transport's `write_progress_callback` / `complete_callback` pop the queue as bytes hit the socket / TLS layer and dispatch the per-stream sink virtuals. This means a backend that has stopped reading (TCP RWIN at zero, TLS WANT_WRITE, OS socket EAGAIN) holds the request frames in the gateway's transport buffer — the stall budget runs against real wire progress, not nghttp2 bookkeeping. A truly stalled upload — peer's flow-control window drained and no transport drain for `response_timeout_ms` (or `30s` if disabled) — surfaces as `RESULT_RESPONSE_TIMEOUT` (504), not `RESULT_UPSTREAM_DISCONNECT` (502). This matches H1's transport-callback-driven send-stall semantics and routes through the retryable-timeout path so `retry_on_timeout` policies apply. diff --git a/include/upstream/upstream_h2_connection.h b/include/upstream/upstream_h2_connection.h index 89bc1619..41fa1273 100644 --- a/include/upstream/upstream_h2_connection.h +++ b/include/upstream/upstream_h2_connection.h @@ -148,6 +148,27 @@ class UpstreamH2Connection { // nghttp2_session_mem_send2 from inside an mem_recv2 callback chain. bool in_receive_data() const { return in_receive_data_; } + // Transport-drain hooks. Wired in PoolPartition::AcquireH2Connection + // to the underlying transport's write_progress / completion callbacks. + // Each call walks drain_queue_ in serialization order and fires the + // per-stream sink virtuals (OnRequestBodyProgress / OnRequestSubmitted) + // for bytes that have actually drained to the wire — NOT when nghttp2 + // serialized them into its internal buffer. + // + // `remaining` is the transport's current output_buf size after the + // partial write. We compute drained = bytes_in_drain_queue_ - remaining + // and attribute that many bytes to the front of drain_queue_. + void OnTransportWriteProgress(size_t remaining); + // Transport buffer fully drained — every frame in drain_queue_ is on + // the wire. Fire any remaining sink virtuals and clear the queue. + void OnTransportWriteComplete(); + // Called from the static on_frame_send_callback for each serialized + // HEADERS/DATA frame. Push the frame's wire-byte count onto + // drain_queue_; the sink virtuals fire from the transport-drain + // hooks above, not here. + void EnqueueFrameForDrain(int32_t stream_id, size_t bytes, + bool is_data_frame, bool is_end_stream); + private: // Non-owning. Lifetime contract: PoolPartition owns the transport // and never reclaims it while this connection's stream count > 0. @@ -192,4 +213,34 @@ class UpstreamH2Connection { // HandleBytes will pick up any frames they queued. Prevents // re-entering nghttp2_session_mem_send2 from a mem_recv2 callback. bool in_receive_data_ = false; + + // Per-frame drain tracking. Populated in on_frame_send_callback when + // nghttp2 hands us a serialized frame; consumed in + // OnTransportWriteProgress / OnTransportWriteComplete as bytes drain + // off the transport buffer. Each entry sums to the frame's wire size + // (9-byte header + payload). Sink virtuals fire when the bytes + // genuinely hit the wire, mirroring H1's transport-callback-driven + // semantic. + struct PendingFrameDrain { + int32_t stream_id; + size_t bytes; // Remaining bytes for this frame on the wire + bool is_data_frame; // OnRequestBodyProgress dispatch (DATA only) + bool is_end_stream; // OnRequestSubmitted dispatch (END_STREAM) + }; + std::deque drain_queue_; + // Total bytes queued on the transport on our behalf — sum of every + // bytes field in drain_queue_. Maintained alongside the queue so we + // can compute drained-since-last-fire as + // drained = bytes_in_drain_queue_ - remaining + // from the transport's reported `remaining`. + size_t bytes_in_drain_queue_ = 0; + + // Pop the front entry of drain_queue_ and fire its sink virtuals. + // Caller owns the streams_ lookup. Used by both progress and + // complete paths. + void FireSinkForDrainEntry(const PendingFrameDrain& entry); + // Drop drain_queue_ entries that belong to a stream that has just + // been failed / reset. The sink is about to be detached so its + // virtuals must not fire post-detach. + void DropDrainEntriesForStream(int32_t stream_id); }; diff --git a/server/pool_partition.cc b/server/pool_partition.cc index ef4a854a..68fe3da4 100644 --- a/server/pool_partition.cc +++ b/server/pool_partition.cc @@ -659,6 +659,25 @@ std::shared_ptr PoolPartition::AcquireH2Connection( ProxyTransaction::RESULT_UPSTREAM_DISCONNECT, "transport error"); }); + // Drive request-side sink virtuals from REAL transport drain (not + // from nghttp2 frame serialization). The H2 session enqueues every + // outbound HEADERS/DATA frame into its drain_queue_ inside + // on_frame_send_callback; these two hooks pop the queue as bytes + // actually leave the transport buffer, then dispatch + // OnRequestBodyProgress / OnRequestSubmitted on the corresponding + // stream's sink. Matches H1's transport-callback-driven semantic. + transport->SetWriteProgressCb( + [wk](std::shared_ptr, size_t remaining) { + auto h = wk.lock(); + if (!h) return; + h->OnTransportWriteProgress(remaining); + }); + transport->SetCompletionCb( + [wk](std::shared_ptr) { + auto h = wk.lock(); + if (!h) return; + h->OnTransportWriteComplete(); + }); h2->AdoptLease(std::move(lease)); h2_table_.Insert(upstream_name, h2); diff --git a/server/proxy_transaction.cc b/server/proxy_transaction.cc index 94188376..581595f4 100644 --- a/server/proxy_transaction.cc +++ b/server/proxy_transaction.cc @@ -945,9 +945,12 @@ void ProxyTransaction::DispatchH2() { path_with_query.append(query_); } - // Initialize H2 state BEFORE SubmitRequest. Bodyless requests can - // inline-flush HEADERS+END_STREAM and fire OnRequestSubmitted - // synchronously; the override's `!h2_path_` guard would otherwise + // Initialize H2 state BEFORE SubmitRequest. Sink virtuals now fire + // from the transport's drain callbacks, but the fast-path + // (DoSendRaw direct-write on a healthy socket with empty buffer) + // can fire complete_callback SYNCHRONOUSLY inside SendRaw — and + // SendRaw is called from FlushSend which is called from + // SubmitRequest. The override's `!h2_path_` guard would otherwise // drop the kill of the just-queued send-stall closure. // Budget mirrors the H1 zero-disable semantic: response_timeout_ms // == 0 opts out of the response-wait timer; stall protection stays @@ -963,8 +966,10 @@ void ProxyTransaction::DispatchH2() { h2_last_progress_at_ = std::chrono::steady_clock::now(); ArmH2SendStallDeadline(h2_stall_budget_ms_); - // Synchronous on_frame_send (bodyless path) may run inline here, - // bumping h2_send_stall_generation_ → kills the closure above. + // Fast-path direct-write (DoSendRaw) may run inline here, firing + // the transport's complete_callback → OnTransportWriteComplete → + // sink->OnRequestSubmitted → bumps h2_send_stall_generation_, + // killing the closure above. int32_t stream_id = h2->SubmitRequest( method_, scheme, authority, path_with_query, rewritten_headers_, request_body_, this, client_te_trailers_); diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index 91a82e0d..841e8c39 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -242,10 +242,18 @@ int OnDataChunkRecvCallback(nghttp2_session* /*session*/, uint8_t /*flags*/, return 0; } -// Dispatches request-side sink signals from frame-serialization edges. -// END_STREAM (HEADERS or DATA) → OnRequestSubmitted; intermediate DATA -// → OnRequestBodyProgress. May fire SYNCHRONOUSLY inside SubmitRequest -// for bodyless requests — sink contract requires reentrancy. +// Enqueue request-side frames for deferred sink dispatch. Sink virtuals +// fire from OnTransportWriteProgress / OnTransportWriteComplete when the +// bytes actually drain off the transport buffer — NOT here, where the +// frame is only in nghttp2's internal output buffer. +// +// Every HEADERS / DATA frame for a tracked stream is pushed to the +// drain queue (including non-end-stream frames, so byte accounting +// stays tight). Frames for unknown / sinkless streams (control frames, +// peer-initiated PUSH_PROMISE) are not queued — they consume transport +// bytes but never fire sink virtuals, so dropping them is safe (the +// drain-queue total only tracks request-side bytes that the transport +// callback will attribute back to streams). int OnFrameSendCallback(nghttp2_session* /*session*/, const nghttp2_frame* frame, void* user_data) { @@ -257,13 +265,12 @@ int OnFrameSendCallback(nghttp2_session* /*session*/, auto* self = static_cast(user_data); auto* stream = self->GetStream(frame->hd.stream_id); if (!stream || !stream->sink) return 0; - if (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { - stream->sink->OnRequestSubmitted(); - } else if (frame->hd.type == NGHTTP2_DATA) { - // Only DATA frames are body-progress signals; the HEADERS frame - // itself (no END_STREAM, bodied request) is request-header data. - stream->sink->OnRequestBodyProgress(); - } + // Wire size = 9-byte frame header + payload length. + const size_t frame_bytes = 9 + static_cast(frame->hd.length); + const bool is_data = (frame->hd.type == NGHTTP2_DATA); + const bool eos = (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) != 0; + self->EnqueueFrameForDrain(frame->hd.stream_id, frame_bytes, + is_data, eos); return 0; } @@ -298,6 +305,13 @@ UpstreamH2Connection::~UpstreamH2Connection() { t->SetOnMessageCb(nullptr); t->SetCloseCb(nullptr); t->SetErrorCb(nullptr); + // Write-progress / completion hooks installed by + // AcquireH2Connection — must also be cleared before the + // transport returns to the pool, otherwise the next + // borrower inherits closures pointing at a destroyed + // session. + t->SetWriteProgressCb(nullptr); + t->SetCompletionCb(nullptr); } } if (session_) { @@ -721,6 +735,12 @@ void UpstreamH2Connection::FailAllStreams(int error_code, if (streams_.empty()) return; auto streams = std::move(streams_); streams_.clear(); + // Drain queue entries for these streams are now stale — sinks are + // about to be invoked via OnError and must not fire request-side + // virtuals afterwards. Clear the whole queue: no other streams are + // left to attribute drained bytes to. + drain_queue_.clear(); + bytes_in_drain_queue_ = 0; for (auto& kv : streams) { if (kv.second && kv.second->sink) { kv.second->sink->OnError(error_code, reason); @@ -739,10 +759,13 @@ void UpstreamH2Connection::ResetStream(int32_t stream_id) { if (it == streams_.end()) return; // Detach the sink before submitting RST_STREAM so the eventual // OnStreamClose does not fire OnError on a transaction that has - // already moved on (e.g. a retry in progress). + // already moved on (e.g. a retry in progress). The drain-queue + // sweep removes any not-yet-fired progress/submitted entries for + // this stream so they don't later dispatch to the nulled sink. if (it->second) it->second->sink = nullptr; + DropDrainEntriesForStream(stream_id); int rv = nghttp2_submit_rst_stream(session_, NGHTTP2_FLAG_NONE, stream_id, NGHTTP2_CANCEL); - + if (rv != 0) { logging::Get()->warn( "UpstreamH2Connection: submit_rst_stream sid={} rv={}", @@ -751,6 +774,97 @@ void UpstreamH2Connection::ResetStream(int32_t stream_id) { if (!in_receive_data_) FlushSend(); } +void UpstreamH2Connection::EnqueueFrameForDrain(int32_t stream_id, + size_t bytes, + bool is_data_frame, + bool is_end_stream) { + drain_queue_.push_back( + PendingFrameDrain{stream_id, bytes, is_data_frame, is_end_stream}); + bytes_in_drain_queue_ += bytes; +} + +void UpstreamH2Connection::FireSinkForDrainEntry( + const PendingFrameDrain& entry) { + // Stream may have been reset between serialization and drain — the + // sink is nulled by ResetStream / FailAllStreams in that case, so + // a stale lookup short-circuits here. + auto it = streams_.find(entry.stream_id); + if (it == streams_.end() || !it->second || !it->second->sink) return; + if (entry.is_end_stream) { + it->second->sink->OnRequestSubmitted(); + } else if (entry.is_data_frame) { + it->second->sink->OnRequestBodyProgress(); + } +} + +void UpstreamH2Connection::DropDrainEntriesForStream(int32_t stream_id) { + if (drain_queue_.empty()) return; + auto new_end = std::remove_if( + drain_queue_.begin(), drain_queue_.end(), + [&](const PendingFrameDrain& e) { + if (e.stream_id == stream_id) { + bytes_in_drain_queue_ -= e.bytes; + return true; + } + return false; + }); + drain_queue_.erase(new_end, drain_queue_.end()); +} + +void UpstreamH2Connection::OnTransportWriteProgress(size_t remaining) { + if (dead_) return; + if (drain_queue_.empty()) return; + // Bytes drained since the last fire = the shrinkage of our tracked + // queue total. The transport may report remaining > our total when + // it holds bytes from sources we don't track (none today, but be + // defensive): in that case drained == 0 and we do nothing. + if (remaining >= bytes_in_drain_queue_) { + bytes_in_drain_queue_ = remaining; + return; + } + size_t drained = bytes_in_drain_queue_ - remaining; + bytes_in_drain_queue_ = remaining; + while (drained > 0 && !drain_queue_.empty()) { + PendingFrameDrain& front = drain_queue_.front(); + if (drained >= front.bytes) { + drained -= front.bytes; + PendingFrameDrain entry = front; + drain_queue_.pop_front(); + // Fire AFTER pop so a sink callback that re-enters + // (e.g., Cleanup → ResetStream → DropDrainEntriesForStream) + // does not invalidate `front`. + FireSinkForDrainEntry(entry); + } else { + // Partial drain of this DATA frame: fire progress (refreshes + // the per-stream stall timestamp) but keep the entry — the + // remaining bytes still need to drain before any END_STREAM + // dispatch. + front.bytes -= drained; + drained = 0; + if (front.is_data_frame && !front.is_end_stream) { + PendingFrameDrain partial_entry{ + front.stream_id, 0, true, false}; + FireSinkForDrainEntry(partial_entry); + } + } + } +} + +void UpstreamH2Connection::OnTransportWriteComplete() { + if (dead_) return; + if (drain_queue_.empty()) { + bytes_in_drain_queue_ = 0; + return; + } + // Transport buffer is empty — every queued frame is on the wire. + auto pending = std::move(drain_queue_); + drain_queue_.clear(); + bytes_in_drain_queue_ = 0; + for (auto& entry : pending) { + FireSinkForDrainEntry(entry); + } +} + namespace { // Hop-by-hop and pseudo-header gate for outbound H2 requests. Reuses diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index 2e097d50..a467b5df 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -3422,8 +3422,9 @@ void TestN7bCLOverflowRejected() { } // --------------------------------------------------------------------------- -// TestN8 — OnRequestSubmitted fires for bodyless GET (synchronous path): -// stream_id is valid and the sink's OnRequestSubmitted was called. +// TestN8 — OnRequestSubmitted fires for bodyless GET only after the +// transport reports drain. Sink virtuals are gated on real wire progress +// via OnTransportWriteComplete, NOT on nghttp2 frame serialization. // --------------------------------------------------------------------------- // Extended RecordingSink that tracks OnRequestSubmitted for N-series tests. @@ -3455,7 +3456,7 @@ struct RecordingSinkEx : public UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseSi }; void TestN8OnRequestSubmittedBodyless() { - std::cout << "\n[TEST] H2Upstream N8: bodyless GET → OnRequestSubmitted fires (synchronous path)..." << std::endl; + std::cout << "\n[TEST] H2Upstream N8: bodyless GET → OnRequestSubmitted fires only after transport drain..." << std::endl; try { auto cfg = MakeH2Conn(); // sink declared before conn — outlives ~UpstreamH2Connection FailAllStreams. @@ -3463,52 +3464,71 @@ void TestN8OnRequestSubmittedBodyless() { UpstreamH2Connection conn(nullptr, cfg); if (!conn.Init()) { TestFramework::RecordTest( - "H2Upstream N8: bodyless GET → OnRequestSubmitted fires (synchronous path)", + "H2Upstream N8: bodyless GET → OnRequestSubmitted fires only after transport drain", false, "Init failed"); return; } - // Bodyless GET: nghttp2 sends HEADERS+END_STREAM synchronously inside - // SubmitRequest via FlushSend → on_frame_send_callback fires inline. + // SubmitRequest serializes HEADERS+END_STREAM into nghttp2's send + // buffer. With the deferred-drain contract the sink must NOT + // see OnRequestSubmitted until transport drain reports it. int32_t sid = conn.SubmitRequest( "GET", "http", "example.com", "/", {}, "", &sink); + if (sink.submitted_calls != 0) { + TestFramework::RecordTest( + "H2Upstream N8: bodyless GET → OnRequestSubmitted fires only after transport drain", + false, + "submitted fired before transport drain: " + + std::to_string(sink.submitted_calls)); + return; + } + // Simulate the transport's complete_callback firing after the + // serialized bytes hit the wire. + conn.OnTransportWriteComplete(); bool pass = (sid > 0) && (sink.submitted_calls == 1); std::string err; if (sid <= 0) err += "invalid stream_id " + std::to_string(sid) + "; "; if (sink.submitted_calls != 1) - err += "submitted_calls=" + std::to_string(sink.submitted_calls) + " (expected 1); "; + err += "submitted_calls=" + std::to_string(sink.submitted_calls) + " (expected 1 after drain); "; TestFramework::RecordTest( - "H2Upstream N8: bodyless GET → OnRequestSubmitted fires (synchronous path)", pass, err); + "H2Upstream N8: bodyless GET → OnRequestSubmitted fires only after transport drain", pass, err); } catch (const std::exception& e) { TestFramework::RecordTest( - "H2Upstream N8: bodyless GET → OnRequestSubmitted fires (synchronous path)", + "H2Upstream N8: bodyless GET → OnRequestSubmitted fires only after transport drain", false, e.what()); } } // --------------------------------------------------------------------------- -// TestN8b — POST with body: OnRequestSubmitted fires after DATA+END_STREAM -// is flushed. With null transport the send is buffered in nghttp2 but the -// flush still fires the callback. +// TestN8b — POST with body: OnRequestSubmitted fires exactly once and +// only after the transport reports drain (deferred-drain contract). // --------------------------------------------------------------------------- void TestN8bOnRequestSubmittedBodyed() { - std::cout << "\n[TEST] H2Upstream N8b: POST with body → OnRequestSubmitted fires once..." << std::endl; + std::cout << "\n[TEST] H2Upstream N8b: POST with body → OnRequestSubmitted fires once after drain..." << std::endl; try { auto cfg = MakeH2Conn(); RecordingSinkEx sink; UpstreamH2Connection conn(nullptr, cfg); if (!conn.Init()) { TestFramework::RecordTest( - "H2Upstream N8b: POST with body → OnRequestSubmitted fires once", + "H2Upstream N8b: POST with body → OnRequestSubmitted fires once after drain", false, "Init failed"); return; } - // POST with body — nghttp2 queues HEADERS then DATA+END_STREAM in one flush. int32_t sid = conn.SubmitRequest( "POST", "http", "example.com", "/upload", {{"content-type", "application/octet-stream"}}, std::string(128, 'B'), &sink); + if (sink.submitted_calls != 0) { + TestFramework::RecordTest( + "H2Upstream N8b: POST with body → OnRequestSubmitted fires once after drain", + false, + "submitted fired before transport drain: " + + std::to_string(sink.submitted_calls)); + return; + } + conn.OnTransportWriteComplete(); bool pass = (sid > 0) && (sink.submitted_calls == 1) && (sink.error_calls == 0); std::string err; @@ -3518,19 +3538,20 @@ void TestN8bOnRequestSubmittedBodyed() { if (sink.error_calls != 0) err += "unexpected errors; "; TestFramework::RecordTest( - "H2Upstream N8b: POST with body → OnRequestSubmitted fires once", pass, err); + "H2Upstream N8b: POST with body → OnRequestSubmitted fires once after drain", pass, err); } catch (const std::exception& e) { TestFramework::RecordTest( - "H2Upstream N8b: POST with body → OnRequestSubmitted fires once", false, e.what()); + "H2Upstream N8b: POST with body → OnRequestSubmitted fires once after drain", false, e.what()); } } // --------------------------------------------------------------------------- -// TestN9 — OnRequestSubmitted fires exactly once per stream, not per-flush. -// Two concurrent streams must each see exactly one call. +// TestN9 — OnRequestSubmitted fires exactly once per stream and only +// after the transport reports drain. Two concurrent streams each see +// exactly one call. // --------------------------------------------------------------------------- void TestN9OnRequestSubmittedOncePerStream() { - std::cout << "\n[TEST] H2Upstream N9: OnRequestSubmitted fires exactly once per stream..." << std::endl; + std::cout << "\n[TEST] H2Upstream N9: OnRequestSubmitted fires exactly once per stream after drain..." << std::endl; try { auto cfg = MakeH2Conn(); RecordingSinkEx sink_a; @@ -3538,7 +3559,7 @@ void TestN9OnRequestSubmittedOncePerStream() { UpstreamH2Connection conn(nullptr, cfg); if (!conn.Init()) { TestFramework::RecordTest( - "H2Upstream N9: OnRequestSubmitted fires exactly once per stream", + "H2Upstream N9: OnRequestSubmitted fires exactly once per stream after drain", false, "Init failed"); return; } @@ -3546,6 +3567,13 @@ void TestN9OnRequestSubmittedOncePerStream() { "GET", "http", "example.com", "/a", {}, "", &sink_a); int32_t sid_b = conn.SubmitRequest( "GET", "http", "example.com", "/b", {}, "", &sink_b); + if (sink_a.submitted_calls != 0 || sink_b.submitted_calls != 0) { + TestFramework::RecordTest( + "H2Upstream N9: OnRequestSubmitted fires exactly once per stream after drain", + false, "sink fired before transport drain"); + return; + } + conn.OnTransportWriteComplete(); bool pass = (sid_a > 0) && (sid_b > 0) && (sink_a.submitted_calls == 1) && @@ -3556,10 +3584,10 @@ void TestN9OnRequestSubmittedOncePerStream() { if (sink_b.submitted_calls != 1) err += "sink_b submitted_calls=" + std::to_string(sink_b.submitted_calls) + "; "; TestFramework::RecordTest( - "H2Upstream N9: OnRequestSubmitted fires exactly once per stream", pass, err); + "H2Upstream N9: OnRequestSubmitted fires exactly once per stream after drain", pass, err); } catch (const std::exception& e) { TestFramework::RecordTest( - "H2Upstream N9: OnRequestSubmitted fires exactly once per stream", false, e.what()); + "H2Upstream N9: OnRequestSubmitted fires exactly once per stream after drain", false, e.what()); } } @@ -4507,20 +4535,18 @@ void TestN8cNoPoisonOnEarlyHeadersSiblingReuse() { } // --------------------------------------------------------------------------- -// TestN9b — OnRequestBodyProgress fires through the production -// OnFrameSendCallback wiring on intermediate DATA frames, NOT just by -// direct call on the sink. Drives a real SubmitRequest with a body -// large enough to span multiple DATA frames so nghttp2 emits at least -// one DATA frame WITHOUT END_STREAM (intermediate) plus one DATA frame -// WITH END_STREAM (terminal). +// TestN9b — Large-body submit enqueues multiple frames in the drain +// queue; OnRequestBodyProgress fires for each intermediate DATA frame +// AND OnRequestSubmitted fires exactly once — but ALL dispatch is gated +// on real transport drain (the deferred-drain contract). Sink virtuals +// must remain silent until OnTransportWriteComplete is called. // -// Default MAX_FRAME_SIZE is 16384 (RFC 9113 §6.5.2). Use a 20000-byte -// body to guarantee at least 2 DATA frames: the first 16384 bytes -// without END_STREAM (→ OnRequestBodyProgress), the remaining 3616 -// bytes with END_STREAM (→ OnRequestSubmitted). +// Default MAX_FRAME_SIZE is 16384 (RFC 9113 §6.5.2). A 20000-byte body +// guarantees at least 2 DATA frames: intermediate (no END_STREAM) + +// terminal (END_STREAM). // --------------------------------------------------------------------------- void TestN9bRequestBodyProgressFiresFromCodec() { - std::cout << "\n[TEST] H2Upstream N9b: large-body submit fires OnRequestBodyProgress via codec..." << std::endl; + std::cout << "\n[TEST] H2Upstream N9b: large-body submit fires sink virtuals via transport drain..." << std::endl; struct ObservingSink : public RecordingSink { int progress_calls = 0; int submitted_calls = 0; @@ -4533,35 +4559,182 @@ void TestN9bRequestBodyProgressFiresFromCodec() { UpstreamH2Connection conn(nullptr, cfg); if (!conn.Init()) { TestFramework::RecordTest( - "H2Upstream N9b: codec dispatches OnRequestBodyProgress on large body", + "H2Upstream N9b: large-body submit fires sink virtuals via transport drain", false, "Init failed"); return; } - // 20000 bytes > 1 frame (MAX_FRAME_SIZE=16384) — guarantees - // at least one intermediate DATA frame. std::string body(20000, 'x'); int32_t sid = conn.SubmitRequest( "POST", "http", "example.com", "/upload", {}, body, &sink); if (sid <= 0) { TestFramework::RecordTest( - "H2Upstream N9b: codec dispatches OnRequestBodyProgress on large body", + "H2Upstream N9b: large-body submit fires sink virtuals via transport drain", false, "submit failed"); return; } - // After SubmitRequest's inline FlushSend, nghttp2 has emitted - // the HEADERS + DATA chunks. With a 20KB body, expect at - // least one OnRequestBodyProgress (intermediate DATA without - // END_STREAM) and exactly one OnRequestSubmitted (final DATA - // with END_STREAM). + // Deferred-drain contract: no sink dispatch before transport reports drain. + if (sink.progress_calls != 0 || sink.submitted_calls != 0) { + TestFramework::RecordTest( + "H2Upstream N9b: large-body submit fires sink virtuals via transport drain", + false, + "sink fired before drain: progress=" + + std::to_string(sink.progress_calls) + + " submitted=" + std::to_string(sink.submitted_calls)); + return; + } + conn.OnTransportWriteComplete(); bool pass = (sink.progress_calls >= 1) && (sink.submitted_calls == 1); TestFramework::RecordTest( - "H2Upstream N9b: codec dispatches OnRequestBodyProgress on large body", + "H2Upstream N9b: large-body submit fires sink virtuals via transport drain", pass, pass ? "" : "progress=" + std::to_string(sink.progress_calls) + " submitted=" + std::to_string(sink.submitted_calls)); } catch (const std::exception& e) { TestFramework::RecordTest( - "H2Upstream N9b: codec dispatches OnRequestBodyProgress on large body", + "H2Upstream N9b: large-body submit fires sink virtuals via transport drain", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN9dDeferredDrainSemantic — Sink virtuals (OnRequestSubmitted and +// OnRequestBodyProgress) MUST fire only after the transport reports +// drain, NOT when nghttp2 serializes the frame into its internal +// output buffer. Verifies the deferred-drain contract end-to-end: +// SubmitRequest serializes frames → drain queue accumulates → +// OnTransportWriteProgress / Complete pop entries and dispatch. +// --------------------------------------------------------------------------- +void TestN9dDeferredDrainSemantic() { + std::cout << "\n[TEST] H2Upstream N9d: sink virtuals deferred to transport drain..." << std::endl; + struct ObservingSink : public RecordingSink { + int progress_calls = 0; + int submitted_calls = 0; + void OnRequestBodyProgress() override { ++progress_calls; } + void OnRequestSubmitted() override { ++submitted_calls; } + }; + try { + auto cfg = MakeH2Conn(); + ObservingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9d: sink virtuals deferred to transport drain", + false, "Init failed"); + return; + } + + // Stage 1: SubmitRequest with a large body — nghttp2 serializes + // HEADERS + multiple DATA frames inline. Drain queue accumulates + // all of them but NO sink virtuals fire yet. + std::string body(20000, 'q'); + int32_t sid = conn.SubmitRequest( + "POST", "http", "example.com", "/upload", {}, body, &sink); + if (sid <= 0) { + TestFramework::RecordTest( + "H2Upstream N9d: sink virtuals deferred to transport drain", + false, "submit failed sid=" + std::to_string(sid)); + return; + } + const bool pre_drain_silent = + (sink.progress_calls == 0) && (sink.submitted_calls == 0); + + // Stage 2: simulate partial drain — write_progress with a + // non-zero remaining. The transport is reporting that some of + // our queued bytes are still buffered; only the bytes that + // drained should fire dispatches. + // For a 20KB body, frame sizes are roughly HEADERS(~30) + + // DATA(16384) + DATA(3616). Tell the transport "10KB still + // buffered" — at least the HEADERS frame should have drained. + conn.OnTransportWriteProgress(10000); + const int after_partial_progress = sink.progress_calls; + const int after_partial_submitted = sink.submitted_calls; + + // Stage 3: full drain — remaining frames dispatch. + conn.OnTransportWriteComplete(); + const bool post_drain_complete = + (sink.submitted_calls == 1) && (sink.progress_calls >= 1); + + bool pass = pre_drain_silent && post_drain_complete && + (after_partial_submitted == 0); + std::string err; + if (!pre_drain_silent) { + err += "sink fired before drain (progress=" + + std::to_string(sink.progress_calls) + + " submitted=" + std::to_string(sink.submitted_calls) + + "); "; + } + if (after_partial_submitted != 0) { + err += "submitted fired during partial drain (submitted=" + + std::to_string(after_partial_submitted) + "); "; + } + if (!post_drain_complete) { + err += "post-drain mismatch (progress=" + + std::to_string(sink.progress_calls) + + " submitted=" + std::to_string(sink.submitted_calls) + + "); "; + } + (void)after_partial_progress; // recorded for diagnosis only + TestFramework::RecordTest( + "H2Upstream N9d: sink virtuals deferred to transport drain", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9d: sink virtuals deferred to transport drain", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN9eResetStreamDropsDrainEntries — Verifies that resetting a stream +// before its drain queue entries fire causes those entries to be +// dropped (not dispatched to a nulled sink). Otherwise FireSinkForDrainEntry +// after detach + ResetStream would crash on the detached sink slot. +// --------------------------------------------------------------------------- +void TestN9eResetStreamDropsDrainEntries() { + std::cout << "\n[TEST] H2Upstream N9e: ResetStream drops pending drain entries..." << std::endl; + struct ObservingSink : public RecordingSink { + int progress_calls = 0; + int submitted_calls = 0; + void OnRequestBodyProgress() override { ++progress_calls; } + void OnRequestSubmitted() override { ++submitted_calls; } + }; + try { + auto cfg = MakeH2Conn(); + ObservingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9e: ResetStream drops pending drain entries", + false, "Init failed"); + return; + } + std::string body(20000, 'r'); + int32_t sid = conn.SubmitRequest( + "POST", "http", "example.com", "/upload", {}, body, &sink); + if (sid <= 0) { + TestFramework::RecordTest( + "H2Upstream N9e: ResetStream drops pending drain entries", + false, "submit failed"); + return; + } + // Reset the stream BEFORE drain runs. The detach pattern nulls + // the sink and DropDrainEntriesForStream sweeps the queue. + conn.ResetStream(sid); + // Now simulate drain — no dispatches should reach the nulled sink. + conn.OnTransportWriteComplete(); + + bool pass = (sink.progress_calls == 0) && + (sink.submitted_calls == 0); + TestFramework::RecordTest( + "H2Upstream N9e: ResetStream drops pending drain entries", + pass, + pass ? "" : "post-reset dispatch: progress=" + + std::to_string(sink.progress_calls) + + " submitted=" + + std::to_string(sink.submitted_calls)); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9e: ResetStream drops pending drain entries", false, e.what()); } } @@ -4707,9 +4880,17 @@ void TestN7eWiringEarlyHeadersThenIntermediateDataDispatch() { false, "submit failed sid=" + std::to_string(sid)); return; } - // After SubmitRequest's inline FlushSend, the codec has - // emitted HEADERS + at least one DATA chunk. Now feed an - // early peer-final-headers (413 + END_STREAM) on stream A — + // Drain BEFORE feeding the peer wire: under the deferred-drain + // contract, sink virtuals dispatch through streams_ lookup, so + // they must run while the stream is still live (peer's + // END_STREAM on HEADERS would erase the stream from streams_ + // via OnStreamClose, leaving drain entries orphaned). Real + // production sees drain-events interleaved with peer-frame + // events on the event loop; this test forces the order + // explicitly. + conn.OnTransportWriteComplete(); + + // Now feed an early peer-final-headers (413 + END_STREAM) — // this is the canonical "peer rejects mid-upload" pattern. std::vector wire = H2WireTest::BuildEmptySettings(); auto early_hdrs = H2WireTest::BuildHeadersFrame( @@ -4719,12 +4900,8 @@ void TestN7eWiringEarlyHeadersThenIntermediateDataDispatch() { ssize_t consumed = conn.HandleBytes( reinterpret_cast(wire.data()), wire.size()); - // The no-real-transport setup drains the body in the initial - // FlushSend before the early-headers wire bytes are fed, so - // the post-headers progress count is observable only with a - // real socketpair (out of scope here). We lock the wiring: - // headers are observed AND ≥1 intermediate DATA progress event - // fires. TestN9b/N9c lock the post-completion no-refresh side. + // Locks the wiring: headers are observed via HandleBytes AND + // ≥1 intermediate DATA progress event fires once drain runs. bool pass = (consumed > 0) && sink.headers_seen && (sink.progress_calls_total >= 1); TestFramework::RecordTest( @@ -4868,6 +5045,8 @@ void RunAllH2UpstreamTests() { TestN8cNoPoisonOnEarlyHeadersSiblingReuse(); TestN9bRequestBodyProgressFiresFromCodec(); TestN9cDefaultSinkSurvivesNewVirtual(); + TestN9dDeferredDrainSemantic(); + TestN9eResetStreamDropsDrainEntries(); TestN7eWiringEarlyHeadersThenIntermediateDataDispatch(); // TestB-series additions — wire-level From 0c8f6ebd76fa9ef02cf4cb915ba25f058e71e626 Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 14:42:07 +0800 Subject: [PATCH 08/14] Fix review comment --- include/upstream/upstream_h2_connection.h | 29 ++-- server/proxy_transaction.cc | 10 +- server/upstream_h2_connection.cc | 113 +++++++++------ test/h2_upstream_test.h | 163 ++++++++++++++++++++++ 4 files changed, 264 insertions(+), 51 deletions(-) diff --git a/include/upstream/upstream_h2_connection.h b/include/upstream/upstream_h2_connection.h index 41fa1273..0af2f08e 100644 --- a/include/upstream/upstream_h2_connection.h +++ b/include/upstream/upstream_h2_connection.h @@ -162,12 +162,16 @@ class UpstreamH2Connection { // Transport buffer fully drained — every frame in drain_queue_ is on // the wire. Fire any remaining sink virtuals and clear the queue. void OnTransportWriteComplete(); - // Called from the static on_frame_send_callback for each serialized - // HEADERS/DATA frame. Push the frame's wire-byte count onto - // drain_queue_; the sink virtuals fire from the transport-drain - // hooks above, not here. + // Called from the static on_frame_send_callback for EVERY serialized + // frame (request HEADERS/DATA AND control frames). Push the frame's + // wire-byte count onto drain_queue_; the sink virtuals fire from + // the transport-drain hooks above, not here. Control frames carry + // is_control=true so dispatch skips them but byte accounting stays + // accurate (control bytes interleaved with request bytes in the + // transport buffer would otherwise be mis-attributed). void EnqueueFrameForDrain(int32_t stream_id, size_t bytes, - bool is_data_frame, bool is_end_stream); + bool is_data_frame, bool is_end_stream, + bool is_control); private: // Non-owning. Lifetime contract: PoolPartition owns the transport @@ -214,18 +218,23 @@ class UpstreamH2Connection { // re-entering nghttp2_session_mem_send2 from a mem_recv2 callback. bool in_receive_data_ = false; - // Per-frame drain tracking. Populated in on_frame_send_callback when - // nghttp2 hands us a serialized frame; consumed in + // Per-frame drain tracking. Populated in on_frame_send_callback for + // EVERY serialized frame (request HEADERS/DATA AND control frames + // like PING, SETTINGS, WINDOW_UPDATE, RST_STREAM); consumed in // OnTransportWriteProgress / OnTransportWriteComplete as bytes drain // off the transport buffer. Each entry sums to the frame's wire size - // (9-byte header + payload). Sink virtuals fire when the bytes - // genuinely hit the wire, mirroring H1's transport-callback-driven - // semantic. + // (9-byte header + payload). Control-frame entries are tracked + // purely for accurate byte accounting — they fire no sink virtuals. + // Without this, a session reused for a fresh request after a PING + // would mis-attribute the PING's drain to the new request's first + // frame, firing OnRequestBodyProgress / OnRequestSubmitted before + // the request's own bytes had drained. struct PendingFrameDrain { int32_t stream_id; size_t bytes; // Remaining bytes for this frame on the wire bool is_data_frame; // OnRequestBodyProgress dispatch (DATA only) bool is_end_stream; // OnRequestSubmitted dispatch (END_STREAM) + bool is_control; // PING/SETTINGS/WINDOW_UPDATE/RST/etc — no sink }; std::deque drain_queue_; // Total bytes queued on the transport on our behalf — sum of every diff --git a/server/proxy_transaction.cc b/server/proxy_transaction.cc index 581595f4..348f75bb 100644 --- a/server/proxy_transaction.cc +++ b/server/proxy_transaction.cc @@ -1735,7 +1735,15 @@ void ProxyTransaction::DeliverTerminalError(int result_code, if (response_committed_ && relay_mode_ == RelayMode::STREAMING) { using AbortReason = HTTP_CALLBACKS_NAMESPACE::StreamingResponseSender::AbortReason; AbortReason reason = AbortReason::UPSTREAM_ERROR; - if (result_code == RESULT_UPSTREAM_DISCONNECT) { + if (result_code == RESULT_UPSTREAM_DISCONNECT || + result_code == RESULT_TRUNCATED_RESPONSE) { + // Both are framing/short-read violations on the upstream + // body — surface them as UPSTREAM_TRUNCATED so downstream + // observability and abort labels distinguish them from + // generic upstream errors. RESULT_TRUNCATED_RESPONSE is + // the application-level (defense-in-depth) detection; + // RESULT_UPSTREAM_DISCONNECT is nghttp2's enforcement + // path. Same semantic, same abort label. reason = AbortReason::UPSTREAM_TRUNCATED; } else if (result_code == RESULT_RESPONSE_TIMEOUT) { reason = AbortReason::UPSTREAM_TIMEOUT; diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index 841e8c39..0b0339d7 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -242,35 +242,51 @@ int OnDataChunkRecvCallback(nghttp2_session* /*session*/, uint8_t /*flags*/, return 0; } -// Enqueue request-side frames for deferred sink dispatch. Sink virtuals -// fire from OnTransportWriteProgress / OnTransportWriteComplete when the -// bytes actually drain off the transport buffer — NOT here, where the -// frame is only in nghttp2's internal output buffer. -// -// Every HEADERS / DATA frame for a tracked stream is pushed to the -// drain queue (including non-end-stream frames, so byte accounting -// stays tight). Frames for unknown / sinkless streams (control frames, -// peer-initiated PUSH_PROMISE) are not queued — they consume transport -// bytes but never fire sink virtuals, so dropping them is safe (the -// drain-queue total only tracks request-side bytes that the transport -// callback will attribute back to streams). +// Enqueue EVERY serialized frame for byte-accurate drain tracking. +// Request-side HEADERS / DATA frames eventually fire sink virtuals from +// the transport-drain hooks; control frames (PING / SETTINGS / +// WINDOW_UPDATE / RST_STREAM / GOAWAY / PRIORITY) are tracked as +// is_control entries so the bytes they consume in the transport buffer +// are correctly attributed (without this, a PING flushed before a fresh +// request would shrink the transport's remaining-bytes counter and +// mis-attribute the PING's drain to the request's first frame, firing +// OnRequestSubmitted before the request's bytes actually hit the wire). int OnFrameSendCallback(nghttp2_session* /*session*/, const nghttp2_frame* frame, void* user_data) { if (!frame) return 0; - if (frame->hd.type != NGHTTP2_HEADERS && - frame->hd.type != NGHTTP2_DATA) { - return 0; - } auto* self = static_cast(user_data); - auto* stream = self->GetStream(frame->hd.stream_id); - if (!stream || !stream->sink) return 0; - // Wire size = 9-byte frame header + payload length. + // Wire size = 9-byte frame header + payload length. nghttp2's + // frame->hd.length is the payload size; framework adds 9 for the + // fixed header regardless of frame type. const size_t frame_bytes = 9 + static_cast(frame->hd.length); - const bool is_data = (frame->hd.type == NGHTTP2_DATA); - const bool eos = (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) != 0; - self->EnqueueFrameForDrain(frame->hd.stream_id, frame_bytes, - is_data, eos); + const bool is_request_frame = + (frame->hd.type == NGHTTP2_HEADERS || + frame->hd.type == NGHTTP2_DATA); + if (is_request_frame) { + auto* stream = self->GetStream(frame->hd.stream_id); + if (!stream || !stream->sink) { + // Stream missing or sink detached — still track the bytes + // as a control entry so the FIFO byte accounting stays + // accurate. The dispatch lookup at fire-time will short- + // circuit on the missing stream regardless. + self->EnqueueFrameForDrain(frame->hd.stream_id, frame_bytes, + /*is_data=*/false, + /*is_end_stream=*/false, + /*is_control=*/true); + return 0; + } + const bool is_data = (frame->hd.type == NGHTTP2_DATA); + const bool eos = (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) != 0; + self->EnqueueFrameForDrain(frame->hd.stream_id, frame_bytes, + is_data, eos, /*is_control=*/false); + return 0; + } + // Control frame: track bytes but never dispatch sink virtuals. + self->EnqueueFrameForDrain(/*stream_id=*/0, frame_bytes, + /*is_data=*/false, + /*is_end_stream=*/false, + /*is_control=*/true); return 0; } @@ -777,14 +793,20 @@ void UpstreamH2Connection::ResetStream(int32_t stream_id) { void UpstreamH2Connection::EnqueueFrameForDrain(int32_t stream_id, size_t bytes, bool is_data_frame, - bool is_end_stream) { + bool is_end_stream, + bool is_control) { drain_queue_.push_back( - PendingFrameDrain{stream_id, bytes, is_data_frame, is_end_stream}); + PendingFrameDrain{stream_id, bytes, is_data_frame, is_end_stream, + is_control}); bytes_in_drain_queue_ += bytes; } void UpstreamH2Connection::FireSinkForDrainEntry( const PendingFrameDrain& entry) { + // Control frames are tracked for byte accounting only — never + // dispatch sink virtuals for them (no stream to look up; the + // sentinel stream_id is meaningless). + if (entry.is_control) return; // Stream may have been reset between serialization and drain — the // sink is nulled by ResetStream / FailAllStreams in that case, so // a stale lookup short-circuits here. @@ -802,7 +824,10 @@ void UpstreamH2Connection::DropDrainEntriesForStream(int32_t stream_id) { auto new_end = std::remove_if( drain_queue_.begin(), drain_queue_.end(), [&](const PendingFrameDrain& e) { - if (e.stream_id == stream_id) { + // Only drop request-side entries for this stream; control + // entries carry stream_id=0 sentinel and must stay in the + // queue so the FIFO byte accounting remains accurate. + if (!e.is_control && e.stream_id == stream_id) { bytes_in_drain_queue_ -= e.bytes; return true; } @@ -814,14 +839,15 @@ void UpstreamH2Connection::DropDrainEntriesForStream(int32_t stream_id) { void UpstreamH2Connection::OnTransportWriteProgress(size_t remaining) { if (dead_) return; if (drain_queue_.empty()) return; - // Bytes drained since the last fire = the shrinkage of our tracked - // queue total. The transport may report remaining > our total when - // it holds bytes from sources we don't track (none today, but be - // defensive): in that case drained == 0 and we do nothing. - if (remaining >= bytes_in_drain_queue_) { - bytes_in_drain_queue_ = remaining; - return; - } + // The transport buffer may contain bytes we did NOT push through + // on_frame_send (e.g. the 24-byte HTTP/2 client connection preface + // magic string at session start). When `remaining` exceeds our + // tracked queue total, those untracked bytes are still draining + // ahead of our first queued frame — leave the queue total alone + // and wait. Updating bytes_in_drain_queue_ = remaining here would + // inflate the tracked sum and over-attribute drained bytes to the + // front entry on the next fire. + if (remaining >= bytes_in_drain_queue_) return; size_t drained = bytes_in_drain_queue_ - remaining; bytes_in_drain_queue_ = remaining; while (drained > 0 && !drain_queue_.empty()) { @@ -835,15 +861,22 @@ void UpstreamH2Connection::OnTransportWriteProgress(size_t remaining) { // does not invalidate `front`. FireSinkForDrainEntry(entry); } else { - // Partial drain of this DATA frame: fire progress (refreshes - // the per-stream stall timestamp) but keep the entry — the - // remaining bytes still need to drain before any END_STREAM - // dispatch. + // Partial drain: refresh the per-stream stall timestamp via + // OnRequestBodyProgress regardless of END_STREAM. A single + // DATA frame body or the trailing DATA frame of a multi- + // frame upload would otherwise never see progress while + // its bytes are actively leaving the socket — the stall + // budget would expire mid-drain even though the wire is + // healthy. OnRequestSubmitted is reserved for the + // FULL-drain branch above, so firing progress here cannot + // race the submitted dispatch. Control-frame entries + // (is_control=true) skip dispatch via FireSinkForDrainEntry. front.bytes -= drained; drained = 0; - if (front.is_data_frame && !front.is_end_stream) { + if (front.is_data_frame && !front.is_control) { PendingFrameDrain partial_entry{ - front.stream_id, 0, true, false}; + front.stream_id, 0, /*is_data=*/true, + /*is_end_stream=*/false, /*is_control=*/false}; FireSinkForDrainEntry(partial_entry); } } diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index a467b5df..9f39a624 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -4739,6 +4739,167 @@ void TestN9eResetStreamDropsDrainEntries() { } } +// --------------------------------------------------------------------------- +// TestN9fPartialDrainOfFinalFrame — A single-DATA-frame body +// (END_STREAM on the only DATA frame) or the trailing DATA frame of a +// multi-frame body must still refresh OnRequestBodyProgress while it +// is partially draining. Otherwise a healthy upload sitting in the +// transport buffer for longer than the stall budget gets false-timed-out. +// --------------------------------------------------------------------------- +void TestN9fPartialDrainOfFinalFrame() { + std::cout << "\n[TEST] H2Upstream N9f: partial-drain of final DATA frame fires progress..." << std::endl; + struct ObservingSink : public RecordingSink { + int progress_calls = 0; + int submitted_calls = 0; + void OnRequestBodyProgress() override { ++progress_calls; } + void OnRequestSubmitted() override { ++submitted_calls; } + }; + try { + auto cfg = MakeH2Conn(); + ObservingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9f: partial-drain of final DATA frame fires progress", + false, "Init failed"); + return; + } + // 4KB body fits in a single DATA frame (MAX_FRAME_SIZE=16384). + // That DATA frame carries both is_data=true AND is_end_stream=true. + std::string body(4096, 'p'); + int32_t sid = conn.SubmitRequest( + "POST", "http", "example.com", "/upload", {}, body, &sink); + if (sid <= 0) { + TestFramework::RecordTest( + "H2Upstream N9f: partial-drain of final DATA frame fires progress", + false, "submit failed"); + return; + } + + // The transport reports a partial drain (some bytes still + // buffered). Without the fix, this case wouldn't fire any + // sink virtual because the only DATA frame is END_STREAM, + // and the old gate was `is_data_frame && !is_end_stream`. + // After the fix, OnRequestBodyProgress refreshes the timestamp. + size_t total_queued = 0; + // Worst-case: HEADERS + single DATA frame; together a few KB. + // Tell the transport "1KB still buffered" — most of the frame + // has drained but not all. + conn.OnTransportWriteProgress(1024); + const int after_partial_progress = sink.progress_calls; + const int after_partial_submitted = sink.submitted_calls; + + // Then fully drain — the submitted dispatch fires now. + conn.OnTransportWriteComplete(); + bool pass = (after_partial_progress >= 1) && + (after_partial_submitted == 0) && + (sink.submitted_calls == 1); + std::string err; + if (after_partial_progress == 0) { + err += "no progress on partial drain (single-frame body case); "; + } + if (after_partial_submitted != 0) { + err += "submitted fired during partial drain; "; + } + if (sink.submitted_calls != 1) { + err += "submitted_calls=" + std::to_string(sink.submitted_calls) + + " (expected 1 after full drain); "; + } + (void)total_queued; + TestFramework::RecordTest( + "H2Upstream N9f: partial-drain of final DATA frame fires progress", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9f: partial-drain of final DATA frame fires progress", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN9gControlFrameByteAccounting — A PING (or other control frame) +// flushed before a request must consume its own bytes in the drain +// queue; otherwise its drain would be mis-attributed to the request's +// first frame, firing OnRequestSubmitted / OnRequestBodyProgress +// before the request's own bytes had actually drained. +// --------------------------------------------------------------------------- +void TestN9gControlFrameByteAccounting() { + std::cout << "\n[TEST] H2Upstream N9g: control-frame bytes do not mis-attribute to request..." << std::endl; + struct ObservingSink : public RecordingSink { + int progress_calls = 0; + int submitted_calls = 0; + void OnRequestBodyProgress() override { ++progress_calls; } + void OnRequestSubmitted() override { ++submitted_calls; } + }; + try { + auto cfg = MakeH2Conn(); + ObservingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9g: control-frame bytes do not mis-attribute to request", + false, "Init failed"); + return; + } + // Flush a PING first — it pushes a control entry (17 bytes: + // 9-byte header + 8-byte opaque payload) into the drain queue. + const auto now = std::chrono::steady_clock::now(); + conn.SendPing(now); + + // Now submit a bodyless request. Its HEADERS frame enters the + // queue after the PING. Without per-frame byte accounting, + // shrinking the transport's `remaining` would attribute the + // PING's drain to the HEADERS frame and fire + // OnRequestSubmitted prematurely. + int32_t sid = conn.SubmitRequest( + "GET", "http", "example.com", "/", {}, "", &sink); + if (sid <= 0) { + TestFramework::RecordTest( + "H2Upstream N9g: control-frame bytes do not mis-attribute to request", + false, "submit failed"); + return; + } + // Simulate the transport draining ONLY the PING bytes (17). + // The drain queue's HEADERS entry must remain — no sink + // dispatch yet. + // Compute: drain queue total bytes minus the headers frame + // size leaves the PING bytes drained. Tell the transport that + // the headers frame is still pending. + // PING entry = 17 bytes, HEADERS frame = 9 + payload (~30 for + // a tiny GET). Tell the transport "30 bytes still buffered" + // — well within the HEADERS frame size, so the PING has + // drained but HEADERS has not. + conn.OnTransportWriteProgress(30); + const int after_ping_drain_submitted = sink.submitted_calls; + const int after_ping_drain_progress = sink.progress_calls; + + // Full drain dispatches the HEADERS frame's submitted virtual. + conn.OnTransportWriteComplete(); + + bool pass = (after_ping_drain_submitted == 0) && + (after_ping_drain_progress == 0) && + (sink.submitted_calls == 1); + std::string err; + if (after_ping_drain_submitted != 0) { + err += "submitted fired during PING-only drain (mis-attribution); "; + } + if (after_ping_drain_progress != 0) { + err += "progress fired during PING-only drain; "; + } + if (sink.submitted_calls != 1) { + err += "submitted_calls=" + std::to_string(sink.submitted_calls) + + " (expected 1 after full drain); "; + } + TestFramework::RecordTest( + "H2Upstream N9g: control-frame bytes do not mis-attribute to request", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9g: control-frame bytes do not mis-attribute to request", + false, e.what()); + } +} + // --------------------------------------------------------------------------- // TestN9c — Default sink ABI: a sink that does NOT override // OnRequestBodyProgress must still compile and operate — locks the @@ -5047,6 +5208,8 @@ void RunAllH2UpstreamTests() { TestN9cDefaultSinkSurvivesNewVirtual(); TestN9dDeferredDrainSemantic(); TestN9eResetStreamDropsDrainEntries(); + TestN9fPartialDrainOfFinalFrame(); + TestN9gControlFrameByteAccounting(); TestN7eWiringEarlyHeadersThenIntermediateDataDispatch(); // TestB-series additions — wire-level From 8168966d9b44528b4fd8e1a8ccd6a616b1948f82 Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 16:02:15 +0800 Subject: [PATCH 09/14] Fix review comment --- include/upstream/proxy_transaction.h | 4 + server/proxy_transaction.cc | 24 ++- server/upstream_h2_connection.cc | 102 +++++----- test/h2_upstream_test.h | 272 +++++++++++++++++++++++++++ 4 files changed, 351 insertions(+), 51 deletions(-) diff --git a/include/upstream/proxy_transaction.h b/include/upstream/proxy_transaction.h index bd6117b3..900f97dc 100644 --- a/include/upstream/proxy_transaction.h +++ b/include/upstream/proxy_transaction.h @@ -35,6 +35,10 @@ class ProxyTransaction : public std::enable_shared_from_this, public UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseSink, public OBSERVABILITY_NAMESPACE::UpstreamTransactionLink { + // Test-only friend that pokes the private H2 dispatch state to + // exercise OnRequestSubmitted's response_timeout branch without + // spinning up the full UpstreamManager / dispatcher / pool stack. + friend struct H2ResponseTimeoutTestFixture; public: // Result codes for internal state tracking static constexpr int RESULT_SUCCESS = 0; diff --git a/server/proxy_transaction.cc b/server/proxy_transaction.cc index 348f75bb..73128fca 100644 --- a/server/proxy_transaction.cc +++ b/server/proxy_transaction.cc @@ -1625,14 +1625,22 @@ void ProxyTransaction::OnRequestSubmitted() { state_ = State::AWAITING_RESPONSE; } if (was_sending && !h2_response_timeout_armed_) { - // Pass the cached fallback budget explicitly. When - // response_timeout_ms == 0 (operator-disabled), ArmResponseTimeout() - // without an explicit budget is a no-op and the post-submit - // watchdog would vanish — leaving a transport-stuck upload - // governed only by PING liveness. Mirrors the H1 send-loop - // pattern which also passes a non-zero stall budget. - ArmResponseTimeout(h2_stall_budget_ms_); - h2_response_timeout_armed_ = true; + // Mirror H1's OnUpstreamWriteComplete contract exactly: + // response_timeout_ms > 0 → arm with that budget; + // response_timeout_ms == 0 → clear the deadline entirely so + // long-poll / SSE / unbounded-response upstreams aren't capped. + // The send-stall fallback budget is for the PRE-submit phase + // only — a transport-stuck request never reaches this method + // under the deferred-drain dispatch semantic (sink virtuals + // fire from real wire-drain callbacks; a stuck transport + // keeps the send-stall closure armed). + if (config_.response_timeout_ms > 0) { + ArmResponseTimeout(); + h2_response_timeout_armed_ = true; + } else { + ClearResponseTimeout(); + h2_response_timeout_armed_ = false; + } } } diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index 0b0339d7..52d69ae9 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -654,56 +654,72 @@ void UpstreamH2Connection::OnHeadersComplete(int32_t stream_id, // always either NO_BODY (END_STREAM on HEADERS frame) or // CHUNKED-equivalent; check for Content-Length to prefer exact framing. using Framing = UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead::Framing; - if (end_stream) { - stream->response_head.framing = Framing::NO_BODY; - } else if (stream->response_head.status_code == 204 || - stream->response_head.status_code == 304 || - stream->request_method == "HEAD") { - // RFC 9110 §15.4 / §15.4.5 / §9.3.2: 204 / 304 / HEAD responses - // MUST NOT carry a body. Classify as NO_BODY so Step 1.5 in - // OnDataChunkRecvCallback rejects any subsequent body bytes from - // a misbehaving peer with RESULT_TRUNCATED_RESPONSE. - stream->response_head.framing = Framing::NO_BODY; - } else { - // Scan accumulated headers for content-length. Cap at the H1 - // codec's MAX_RESPONSE_BODY_SIZE to defend against malicious or - // buggy upstreams advertising absurd values (e.g. 1e18 bytes) - // that would propagate through expected_length into snapshot - // truncation arithmetic. RFC 9113 lets us treat the header as - // informational, so on an over-cap value we fall through to - // CHUNKED-equivalent framing and rely on END_STREAM as the - // authoritative end-of-body signal. - int64_t cl = -1; - for (const auto& [nm, val] : stream->response_head.headers) { - if (nm == "content-length") { - // from_chars: strict full-string consume; std::stoll - // skips leading whitespace and would accept " 42". + // Parse Content-Length regardless of end_stream so the HEADERS-only + // short-read case (end_stream on HEADERS with CL > 0) can be + // classified as CONTENT_LENGTH and detected by OnStreamClose's CL + // short-read check. + int64_t cl = -1; + for (const auto& [nm, val] : stream->response_head.headers) { + if (nm == "content-length") { + // from_chars: strict full-string consume; std::stoll + // skips leading whitespace and would accept " 42". + cl = -1; + const char* end = val.data() + val.size(); + int64_t parsed = 0; + auto [ptr, ec] = std::from_chars(val.data(), end, parsed); + if (ec == std::errc() && ptr == end && parsed >= 0) { + cl = parsed; + } + if (cl > static_cast( + UpstreamHttpCodec::MAX_RESPONSE_BODY_SIZE)) { + logging::Get()->warn( + "UpstreamH2Connection: content-length {} exceeds cap " + "{} on stream {}; treating as chunked", + val, + UpstreamHttpCodec::MAX_RESPONSE_BODY_SIZE, + stream_id); cl = -1; - const char* end = val.data() + val.size(); - int64_t parsed = 0; - auto [ptr, ec] = std::from_chars(val.data(), end, parsed); - if (ec == std::errc() && ptr == end && parsed >= 0) { - cl = parsed; - } - if (cl > static_cast( - UpstreamHttpCodec::MAX_RESPONSE_BODY_SIZE)) { - logging::Get()->warn( - "UpstreamH2Connection: content-length {} exceeds cap " - "{} on stream {}; treating as chunked", - val, - UpstreamHttpCodec::MAX_RESPONSE_BODY_SIZE, - stream_id); - cl = -1; - } - break; } + break; } - if (cl >= 0) { + } + + const bool bodyless_status = + (stream->response_head.status_code == 204 || + stream->response_head.status_code == 304 || + stream->request_method == "HEAD"); + + if (bodyless_status) { + // RFC 9110 §15.4 / §15.4.5 / §9.3.2: 204 / 304 / HEAD responses + // MUST NOT carry a body. Content-Length on these is allowed + // as informational (RFC 9110 §9.3.2 specifically permits CL on + // HEAD to advertise the equivalent-GET body size) and does NOT + // trigger a short-read check. Classify as NO_BODY so Step 1.5 + // in OnDataChunkRecvCallback rejects any subsequent body bytes + // from a misbehaving peer with RESULT_TRUNCATED_RESPONSE. + stream->response_head.framing = Framing::NO_BODY; + } else if (end_stream) { + // END_STREAM on HEADERS with a non-bodyless status. If CL > 0 + // was declared, peer promised N body bytes and delivered zero + // — that's a framing violation. Classify as CONTENT_LENGTH + // with expected_length=cl so OnStreamClose's existing CL + // short-read check fires RESULT_TRUNCATED_RESPONSE (defense + // in depth — nghttp2's HTTP messaging enforcement normally + // catches this first via the non-NO_ERROR fan-out, but the + // backstop covers the no-messaging-enforcement future). + // CL == 0 or absent CL → legitimate empty-body response, + // classify as NO_BODY. + if (cl > 0) { stream->response_head.framing = Framing::CONTENT_LENGTH; stream->response_head.expected_length = cl; } else { - stream->response_head.framing = Framing::CHUNKED; + stream->response_head.framing = Framing::NO_BODY; } + } else if (cl >= 0) { + stream->response_head.framing = Framing::CONTENT_LENGTH; + stream->response_head.expected_length = cl; + } else { + stream->response_head.framing = Framing::CHUNKED; } if (!stream->sink->OnHeaders(stream->response_head)) { diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index 9f39a624..b7fbaa14 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -46,6 +46,10 @@ #include "upstream/upstream_manager.h" #include "upstream/pool_partition.h" #include "upstream/proxy_transaction.h" // for RESULT_UPSTREAM_DISCONNECT +#include "upstream/header_rewriter.h" +#include "upstream/retry_policy.h" +#include "http/http_request.h" +#include "http/streaming_response_sender.h" #include "upstream/upstream_connection.h" #include "upstream/upstream_lease.h" #include "upstream/upstream_response_sink.h" @@ -4900,6 +4904,269 @@ void TestN9gControlFrameByteAccounting() { } } +// --------------------------------------------------------------------------- +// H2ResponseTimeoutTestFixture — friend of ProxyTransaction; pokes the +// private H2 dispatch state so a focused test can exercise +// OnRequestSubmitted's response_timeout branch without the full pool +// pipeline. Build via the factory helper below. +// --------------------------------------------------------------------------- +struct H2ResponseTimeoutTestFixture { + static std::shared_ptr MakeWithTimeout( + int response_timeout_ms) + { + HttpRequest req; + req.method = "GET"; + req.path = "/"; + req.dispatcher_index = -1; // null dispatcher in ctor + + HTTP_CALLBACKS_NAMESPACE::StreamingResponseSender sender; + HTTP_CALLBACKS_NAMESPACE::AsyncCompletionCallback cb = + [](const HttpResponse&) {}; + ProxyConfig cfg; + cfg.response_timeout_ms = response_timeout_ms; + HeaderRewriter rewriter(HeaderRewriter::Config{ + cfg.header_rewrite.set_x_forwarded_for, + cfg.header_rewrite.set_x_forwarded_proto, + cfg.header_rewrite.set_via_header, + cfg.header_rewrite.rewrite_host}); + RetryPolicy retry(RetryPolicy::Config{ + cfg.retry.max_retries, + cfg.retry.retry_on_connect_failure, + cfg.retry.retry_on_5xx, + cfg.retry.retry_on_timeout, + cfg.retry.retry_on_disconnect, + cfg.retry.retry_non_idempotent}); + auto txn = std::make_shared( + std::string("test-h2-timeout"), req, + std::move(sender), std::move(cb), + nullptr, // upstream_manager + cfg, rewriter, retry, + false, // upstream_tls + std::string("127.0.0.1"), 80, + std::string(""), std::string(""), std::string(""), + nullptr); // auth_manager + return txn; + } + + // Drive OnRequestSubmitted's H2 post-send-complete branch. The + // public method has guards (h2_path_, cancelled, IsKilledForShutdown, + // state_) that require the transaction to be in a specific state; + // set them directly so the test exercises ONLY the timeout-decision + // logic. + static void DriveOnRequestSubmittedFromSending( + const std::shared_ptr& txn) + { + txn->h2_path_ = true; + txn->state_ = ProxyTransaction::State::SENDING_REQUEST; + txn->h2_stall_budget_ms_ = + ProxyTransaction::ComputeH2StallBudgetMs( + txn->config_.response_timeout_ms); + txn->OnRequestSubmitted(); + } + + static bool response_timeout_armed( + const std::shared_ptr& txn) + { + return txn->h2_response_timeout_armed_; + } +}; + +// --------------------------------------------------------------------------- +// TestN9kZeroTimeoutPostSubmit — Mirrors H1's response_timeout_ms=0 +// contract: after the request is fully sent, no per-request deadline +// is armed. Previously H2 always armed the 30s stall fallback, so +// long-poll / SSE / late-header upstreams 504'd at 30s contradicting +// the documented "0 disables the timeout" semantic. +// --------------------------------------------------------------------------- +void TestN9kZeroTimeoutPostSubmit() { + std::cout << "\n[TEST] H2Upstream N9k: response_timeout_ms=0 → no deadline armed after submit..." << std::endl; + try { + auto txn = H2ResponseTimeoutTestFixture::MakeWithTimeout(0); + H2ResponseTimeoutTestFixture::DriveOnRequestSubmittedFromSending(txn); + bool armed = H2ResponseTimeoutTestFixture::response_timeout_armed(txn); + bool pass = !armed; + TestFramework::RecordTest( + "H2Upstream N9k: response_timeout_ms=0 → no deadline armed after submit", + pass, + pass ? "" : "h2_response_timeout_armed_ should be false for ms=0"); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9k: response_timeout_ms=0 → no deadline armed after submit", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN9lPositiveTimeoutPostSubmit — Positive response_timeout_ms still +// arms the deadline as expected. Locks the both-sides of the branch +// added by the N9k fix. +// --------------------------------------------------------------------------- +void TestN9lPositiveTimeoutPostSubmit() { + std::cout << "\n[TEST] H2Upstream N9l: response_timeout_ms>0 → deadline armed after submit..." << std::endl; + try { + auto txn = H2ResponseTimeoutTestFixture::MakeWithTimeout(5000); + H2ResponseTimeoutTestFixture::DriveOnRequestSubmittedFromSending(txn); + bool armed = H2ResponseTimeoutTestFixture::response_timeout_armed(txn); + bool pass = armed; + TestFramework::RecordTest( + "H2Upstream N9l: response_timeout_ms>0 → deadline armed after submit", + pass, + pass ? "" : "h2_response_timeout_armed_ should be true for ms=5000"); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9l: response_timeout_ms>0 → deadline armed after submit", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN9hHeadersOnlyShortReadCL — A response that declares +// Content-Length > 0 but ends the stream on HEADERS (zero body bytes) +// MUST surface as an error, not a clean OnComplete. The HEADERS-only +// path used to classify as NO_BODY, bypassing the CL short-read check +// in OnStreamClose. After the fix, when end_stream is true and CL > 0 +// on a non-bodyless status, framing is CONTENT_LENGTH so the existing +// CL short-read backstop fires RESULT_TRUNCATED_RESPONSE on NO_ERROR +// stream close (or RESULT_UPSTREAM_DISCONNECT if nghttp2's HTTP +// messaging enforcement fired first via non-NO_ERROR). Either way: +// OnError fires, OnComplete does NOT. +// --------------------------------------------------------------------------- +void TestN9hHeadersOnlyShortReadCL() { + std::cout << "\n[TEST] H2Upstream N9h: HEADERS+END_STREAM with CL>0 → OnError (not OnComplete)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9h: HEADERS+END_STREAM with CL>0 → OnError (not OnComplete)", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", + {}, "", &sink); + + // Server sends SETTINGS + HEADERS(200, content-length:100, END_STREAM). + // Zero body bytes but declared 100 — framing violation. + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}, {"content-length", "100"}}, + /*end_stream=*/true); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), + wire.size()); + + // OnError MUST fire. The specific RESULT_* code is either + // RESULT_TRUNCATED_RESPONSE (our backstop, nghttp2 messaging + // enforcement disabled) or RESULT_UPSTREAM_DISCONNECT + // (nghttp2's enforcement fired non-NO_ERROR first). What + // matters is the truncation is NOT silently dropped. + bool pass = (sink.error_calls == 1) && (sink.complete_calls == 0); + std::string err; + if (sink.error_calls != 1) + err += "error_calls=" + std::to_string(sink.error_calls) + " (expected 1); "; + if (sink.complete_calls != 0) + err += "complete_calls=" + std::to_string(sink.complete_calls) + " (expected 0); "; + TestFramework::RecordTest( + "H2Upstream N9h: HEADERS+END_STREAM with CL>0 → OnError (not OnComplete)", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9h: HEADERS+END_STREAM with CL>0 → OnError (not OnComplete)", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN9iHeadersOnlyShortReadCLZeroLegitimate — A response with +// content-length: 0 AND END_STREAM on HEADERS is LEGITIMATE (legal +// empty body). Verifies the fix's `cl > 0` guard doesn't false-trigger +// truncation for legal empty bodies. +// --------------------------------------------------------------------------- +void TestN9iHeadersOnlyShortReadCLZeroLegitimate() { + std::cout << "\n[TEST] H2Upstream N9i: HEADERS+END_STREAM with CL=0 → OnComplete (legitimate)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9i: HEADERS+END_STREAM with CL=0 → OnComplete (legitimate)", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", + {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}, {"content-length", "0"}}, + /*end_stream=*/true); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), + wire.size()); + + bool pass = (sink.complete_calls == 1) && (sink.error_calls == 0); + std::string err; + if (sink.complete_calls != 1) + err += "complete_calls=" + std::to_string(sink.complete_calls) + "; "; + if (sink.error_calls != 0) + err += "error_calls=" + std::to_string(sink.error_calls) + "; "; + TestFramework::RecordTest( + "H2Upstream N9i: HEADERS+END_STREAM with CL=0 → OnComplete (legitimate)", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9i: HEADERS+END_STREAM with CL=0 → OnComplete (legitimate)", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN9jHeadResponseWithCLLegitimate — RFC 9110 §9.3.2 explicitly +// permits HEAD responses to declare Content-Length matching the +// equivalent-GET body size. END_STREAM on HEADERS with CL > 0 on a +// HEAD response is LEGITIMATE — must NOT trigger truncation. +// --------------------------------------------------------------------------- +void TestN9jHeadResponseWithCLLegitimate() { + std::cout << "\n[TEST] H2Upstream N9j: HEAD response with CL>0 + END_STREAM → OnComplete (legitimate)..." << std::endl; + try { + auto cfg = MakeH2Conn(); + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9j: HEAD response with CL>0 + END_STREAM → OnComplete (legitimate)", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("HEAD", "http", "example.com", "/", + {}, "", &sink); + + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}, {"content-length", "12345"}}, + /*end_stream=*/true); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), + wire.size()); + + bool pass = (sink.complete_calls == 1) && (sink.error_calls == 0); + std::string err; + if (sink.complete_calls != 1) + err += "complete_calls=" + std::to_string(sink.complete_calls) + "; "; + if (sink.error_calls != 0) + err += "error_calls=" + std::to_string(sink.error_calls) + "; "; + TestFramework::RecordTest( + "H2Upstream N9j: HEAD response with CL>0 + END_STREAM → OnComplete (legitimate)", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9j: HEAD response with CL>0 + END_STREAM → OnComplete (legitimate)", + false, e.what()); + } +} + // --------------------------------------------------------------------------- // TestN9c — Default sink ABI: a sink that does NOT override // OnRequestBodyProgress must still compile and operate — locks the @@ -5210,6 +5477,11 @@ void RunAllH2UpstreamTests() { TestN9eResetStreamDropsDrainEntries(); TestN9fPartialDrainOfFinalFrame(); TestN9gControlFrameByteAccounting(); + TestN9hHeadersOnlyShortReadCL(); + TestN9iHeadersOnlyShortReadCLZeroLegitimate(); + TestN9jHeadResponseWithCLLegitimate(); + TestN9kZeroTimeoutPostSubmit(); + TestN9lPositiveTimeoutPostSubmit(); TestN7eWiringEarlyHeadersThenIntermediateDataDispatch(); // TestB-series additions — wire-level From 96518e0c9120d08b9e5a208afab1bdd43e13316f Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 17:48:23 +0800 Subject: [PATCH 10/14] Fix review comment --- server/pool_partition.cc | 35 ++++++---- server/upstream_h2_connection.cc | 11 ++- test/h2_upstream_test.h | 114 +++++++++++++++++++++++++++++++ 3 files changed, 146 insertions(+), 14 deletions(-) diff --git a/server/pool_partition.cc b/server/pool_partition.cc index 68fe3da4..b29de31e 100644 --- a/server/pool_partition.cc +++ b/server/pool_partition.cc @@ -86,6 +86,8 @@ PoolPartition::PoolPartition( // Null out all callbacks on a connection's transport to prevent // dangling-this use-after-free if the ConnectionHandler outlives // the PoolPartition (still in dispatcher's connections_ map). +// Nulls completion/write-progress too so an H2 borrower's wire-up +// can't survive an Init failure into the next pool reuse. static void ClearTransportCallbacks(UpstreamConnection* conn) { if (conn && conn->GetTransport()) { auto t = conn->GetTransport(); @@ -93,6 +95,8 @@ static void ClearTransportCallbacks(UpstreamConnection* conn) { t->SetCloseCb(nullptr); t->SetOnMessageCb(nullptr); t->SetErrorCb(nullptr); + t->SetCompletionCb(nullptr); + t->SetWriteProgressCb(nullptr); } } @@ -580,20 +584,13 @@ std::shared_ptr PoolPartition::AcquireH2Connection( if (!transport) return nullptr; auto h2 = std::make_shared(up, cfg); - if (!h2->Init()) { - logging::Get()->warn( - "PoolPartition::AcquireH2Connection: Init failed upstream={} " - "host={}:{}", - upstream_name, upstream_host_, upstream_port_); - return nullptr; - } - // Wire transport callbacks for the H2 session lifecycle. The H2 - // connection multiplexes the transport for its lifetime, so we - // overwrite the pool-owned message and close callbacks: pool - // accounting then follows the lease destructor when the H2 - // connection retires (lease_ is moved into the H2 connection - // below — its return-to-pool is what reclaims the slot). + // Callbacks wired BEFORE Init() because Init's preface flush can + // fire complete_callback synchronously on a writable transport + // (DoSendRaw direct-write path) — our drain attribution must be + // active for that bootstrap traffic. The H2 connection + // multiplexes the transport for its lifetime; pool accounting + // follows the lease destructor when the H2 connection retires. std::weak_ptr wk = h2; transport->SetOnMessageCb( [wk](std::shared_ptr, std::string& data) { @@ -679,6 +676,18 @@ std::shared_ptr PoolPartition::AcquireH2Connection( h->OnTransportWriteComplete(); }); + if (!h2->Init()) { + logging::Get()->warn( + "PoolPartition::AcquireH2Connection: Init failed upstream={} " + "host={}:{}", + upstream_name, upstream_host_, upstream_port_); + // Unwire our weak_ptr closures before the transport returns to + // the pool — WirePoolCallbacks doesn't overwrite completion / + // write-progress on reuse. + ClearTransportCallbacks(up); + return nullptr; + } + h2->AdoptLease(std::move(lease)); h2_table_.Insert(upstream_name, h2); return h2; diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index 52d69ae9..e433562b 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -238,7 +238,16 @@ int OnDataChunkRecvCallback(nghttp2_session* /*session*/, uint8_t /*flags*/, } stream->body_bytes_received += static_cast(len); - stream->sink->OnBodyChunk(reinterpret_cast(data), len); + const bool keep = stream->sink->OnBodyChunk( + reinterpret_cast(data), len); + if (!keep) { + // Sink refused further body — detach + RST_STREAM(CANCEL) so + // the upstream stops sending. Session stays alive for sibling + // streams. The pre-null guards against ResetStream's dead_ + // short-circuit. + stream->sink = nullptr; + self->ResetStream(stream_id); + } return 0; } diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index b7fbaa14..3af8a6e4 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -4904,6 +4904,85 @@ void TestN9gControlFrameByteAccounting() { } } +// --------------------------------------------------------------------------- +// TestN9mSinkOnBodyChunkFalseStopsConsumption — A sink returning false +// from OnBodyChunk must detach + submit RST_STREAM so no further body +// dispatches reach the sink. +// --------------------------------------------------------------------------- +void TestN9mSinkOnBodyChunkFalseStopsConsumption() { + std::cout << "\n[TEST] H2Upstream N9m: sink OnBodyChunk false → stream reset, no further dispatches..." << std::endl; + struct RejectingSink : public RecordingSink { + bool reject_after_first = true; + int body_chunks = 0; + bool OnBodyChunk(const char* data, size_t len) override { + ++body_chunks; + RecordingSink::OnBodyChunk(data, len); + // Reject every chunk including the first — simulates a + // downstream commit failure on first body byte. + return !reject_after_first; + } + }; + try { + auto cfg = MakeH2Conn(); + RejectingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9m: sink OnBodyChunk false → stream reset, no further dispatches", + false, "Init failed"); + return; + } + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", + {}, "", &sink); + + // Peer sends SETTINGS + HEADERS (no end_stream) + DATA(50) + DATA(50, end_stream). + // After the first DATA chunk, sink returns false. The H2 code + // path detaches the sink and submits RST_STREAM. The second + // DATA chunk must NOT dispatch to the (now-null) sink. + std::vector wire = H2WireTest::BuildEmptySettings(); + auto hdrs = H2WireTest::BuildHeadersFrame( + sid, {{":status", "200"}, {"content-type", "text/plain"}}, + /*end_stream=*/false); + wire.insert(wire.end(), hdrs.begin(), hdrs.end()); + conn.HandleBytes(reinterpret_cast(wire.data()), + wire.size()); + + std::vector body1(50, 'a'); + auto data1 = BuildDataFrame(sid, body1.data(), body1.size(), + /*end_stream=*/false); + conn.HandleBytes(reinterpret_cast(data1.data()), + data1.size()); + + // First chunk should have dispatched (sink saw it, then said no). + const int chunks_after_first = sink.body_chunks; + + // Second chunk: sink is now detached. body_chunks must NOT + // advance. Implementation detail: nghttp2 may or may not have + // already processed the RST_STREAM submission by the time the + // second DATA frame is handed in; either way, our application + // code looks up stream->sink and finds nullptr → skip. + std::vector body2(50, 'b'); + auto data2 = BuildDataFrame(sid, body2.data(), body2.size(), + /*end_stream=*/true); + conn.HandleBytes(reinterpret_cast(data2.data()), + data2.size()); + + bool pass = (chunks_after_first == 1) && (sink.body_chunks == 1); + std::string err; + if (chunks_after_first != 1) + err += "first-chunk dispatch=" + std::to_string(chunks_after_first) + "; "; + if (sink.body_chunks != 1) + err += "post-rejection chunks=" + std::to_string(sink.body_chunks) + " (expected 1); "; + TestFramework::RecordTest( + "H2Upstream N9m: sink OnBodyChunk false → stream reset, no further dispatches", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9m: sink OnBodyChunk false → stream reset, no further dispatches", + false, e.what()); + } +} + // --------------------------------------------------------------------------- // H2ResponseTimeoutTestFixture — friend of ProxyTransaction; pokes the // private H2 dispatch state so a focused test can exercise @@ -5019,6 +5098,39 @@ void TestN9lPositiveTimeoutPostSubmit() { } } +// --------------------------------------------------------------------------- +// TestN9nFreshSessionBootstrapCallbackOrdering — Init's preface SETTINGS +// is tracked as a control drain entry that OnTransportWriteComplete +// pops cleanly without firing any sink dispatch. +// --------------------------------------------------------------------------- +void TestN9nFreshSessionBootstrapCallbackOrdering() { + std::cout << "\n[TEST] H2Upstream N9n: fresh session Init populates drain queue with SETTINGS..." << std::endl; + try { + auto cfg = MakeH2Conn(); + // Sink before conn — sinks-must-outlive-session contract. + RecordingSink sink; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9n: fresh session Init populates drain queue with SETTINGS", + false, "Init failed"); + return; + } + conn.OnTransportWriteComplete(); + int32_t sid = conn.SubmitRequest("GET", "http", "example.com", "/", + {}, "", &sink); + bool pass = (sid > 0); + TestFramework::RecordTest( + "H2Upstream N9n: fresh session Init populates drain queue with SETTINGS", + pass, + pass ? "" : "submit after bootstrap drain failed sid=" + std::to_string(sid)); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9n: fresh session Init populates drain queue with SETTINGS", + false, e.what()); + } +} + // --------------------------------------------------------------------------- // TestN9hHeadersOnlyShortReadCL — A response that declares // Content-Length > 0 but ends the stream on HEADERS (zero body bytes) @@ -5482,6 +5594,8 @@ void RunAllH2UpstreamTests() { TestN9jHeadResponseWithCLLegitimate(); TestN9kZeroTimeoutPostSubmit(); TestN9lPositiveTimeoutPostSubmit(); + TestN9mSinkOnBodyChunkFalseStopsConsumption(); + TestN9nFreshSessionBootstrapCallbackOrdering(); TestN7eWiringEarlyHeadersThenIntermediateDataDispatch(); // TestB-series additions — wire-level From e3911bfaf352e65fa8705425c239a7e5a9d6bc6f Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 19:20:08 +0800 Subject: [PATCH 11/14] Fix review comment --- server/proxy_transaction.cc | 21 ++++--- test/h2_upstream_test.h | 119 ++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 7 deletions(-) diff --git a/server/proxy_transaction.cc b/server/proxy_transaction.cc index 73128fca..a9b413d5 100644 --- a/server/proxy_transaction.cc +++ b/server/proxy_transaction.cc @@ -1322,17 +1322,19 @@ bool ProxyTransaction::OnHeaders( } if (h2_path_) { - // H2: stall closure stays armed until OnRequestSubmitted / - // Cleanup bumps the generation — an early peer-final-headers - // (e.g. 413) before our END_STREAM keeps stall protection. No - // poison_connection_ here: H2 multiplexes streams, so an early + // H2: early-final-headers — peer responded before our END_STREAM. + // No poison_connection_: H2 multiplexes streams, so an early // status on one stream is not a transport-fatal signal. if (state_ == State::SENDING_REQUEST) { state_ = State::AWAITING_RESPONSE; + // Invalidate the send-stall closure. Otherwise it fires + // after the budget elapses with state in AWAITING_RESPONSE / + // RECEIVING_BODY and spuriously surfaces RESPONSE_TIMEOUT + // against a stream whose headers are already in hand. + ++h2_send_stall_generation_; + h2_request_fully_sent_ = true; } // Header phase done; body phase is governed by stream timers. - // Reset the arm-once flag so a later OnRequestSubmitted skips - // re-arming. ClearResponseTimeout(); h2_response_timeout_armed_ = false; } else { @@ -2785,7 +2787,12 @@ void ProxyTransaction::ArmResponseTimeout(int explicit_budget_ms) { [weak_self, generation]() { auto self = weak_self.lock(); if (!self) return; - if (self->cancelled_) return; + // IsKilledForShutdown check mirrors the send-stall + // closure: MarkKilledForShutdown sets the kill flag + // before Cancel() enqueues, so a matured timeout that + // fires inside that window must not report a breaker + // failure or trigger MaybeRetry during drain. + if (self->cancelled_ || self->IsKilledForShutdown()) return; if (generation != self->h2_response_timeout_generation_) return; logging::Get()->warn( "ProxyTransaction H2 response timeout client_fd={} " diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index 3af8a6e4..2cc2618b 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -50,6 +50,8 @@ #include "upstream/retry_policy.h" #include "http/http_request.h" #include "http/streaming_response_sender.h" +#include +#include #include "upstream/upstream_connection.h" #include "upstream/upstream_lease.h" #include "upstream/upstream_response_sink.h" @@ -5048,6 +5050,40 @@ struct H2ResponseTimeoutTestFixture { { return txn->h2_response_timeout_armed_; } + + // Capture the live send-stall generation. Used by the early-final- + // headers test to confirm OnHeaders bumps it when it transitions + // out of SENDING_REQUEST. + static uint64_t send_stall_generation( + const std::shared_ptr& txn) + { + return txn->h2_send_stall_generation_; + } + + // Capture the live state. Used by tests that need to verify + // state transitions without going through the full pool pipeline. + static ProxyTransaction::State state( + const std::shared_ptr& txn) + { + return txn->state_; + } + + // Drive OnHeaders with a synthetic UpstreamResponseHead so the + // early-final-headers test can observe the SENDING_REQUEST → + // AWAITING_RESPONSE transition + send-stall invalidation. + static void DriveOnHeadersWhileSending( + const std::shared_ptr& txn, + int status_code) + { + txn->h2_path_ = true; + txn->state_ = ProxyTransaction::State::SENDING_REQUEST; + UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead head; + head.status_code = status_code; + head.keep_alive = true; + head.framing = + UPSTREAM_CALLBACKS_NAMESPACE::UpstreamResponseHead::Framing::NO_BODY; + txn->OnHeaders(head); + } }; // --------------------------------------------------------------------------- @@ -5098,6 +5134,87 @@ void TestN9lPositiveTimeoutPostSubmit() { } } +// --------------------------------------------------------------------------- +// TestN9oEarlyFinalHeadersInvalidateSendStallClosure — Peer delivers +// final headers WHILE we're still sending the request body +// (state_ == SENDING_REQUEST). OnHeaders must bump +// h2_send_stall_generation_ so the in-flight stall closure can't +// fire later and spuriously surface RESPONSE_TIMEOUT against a stream +// whose headers are already in hand. Mirrors the body-phase invariant +// that final headers end the send-side watchdog. +// --------------------------------------------------------------------------- +void TestN9oEarlyFinalHeadersInvalidateSendStallClosure() { + std::cout << "\n[TEST] H2Upstream N9o: early final headers bump send-stall generation..." << std::endl; + try { + auto txn = H2ResponseTimeoutTestFixture::MakeWithTimeout(5000); + // Simulate: SENDING_REQUEST + a stall closure armed against + // the current generation. (DriveOnHeadersWhileSending sets + // h2_path_=true and state_=SENDING_REQUEST.) + const uint64_t gen_before = + H2ResponseTimeoutTestFixture::send_stall_generation(txn); + H2ResponseTimeoutTestFixture::DriveOnHeadersWhileSending( + txn, /*status=*/413); + const uint64_t gen_after = + H2ResponseTimeoutTestFixture::send_stall_generation(txn); + const auto state_after = + H2ResponseTimeoutTestFixture::state(txn); + + const bool transitioned = + (state_after == ProxyTransaction::State::AWAITING_RESPONSE); + const bool gen_bumped = (gen_after > gen_before); + bool pass = transitioned && gen_bumped; + std::string err; + if (!transitioned) err += "state did not advance to AWAITING_RESPONSE; "; + if (!gen_bumped) err += "send_stall_generation_ did not advance (closure would fire); "; + TestFramework::RecordTest( + "H2Upstream N9o: early final headers bump send-stall generation", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9o: early final headers bump send-stall generation", + false, e.what()); + } +} + +// --------------------------------------------------------------------------- +// TestN9pH2ResponseTimeoutClosureHonorsShutdownKill — The H2 response- +// timeout closure must guard on IsKilledForShutdown() in addition to +// cancelled_. MarkKilledForShutdown sets the kill flag before Cancel +// enqueues, so a matured timeout firing inside that window would +// otherwise report a breaker failure and trigger MaybeRetry during +// drain. +// --------------------------------------------------------------------------- +void TestN9pH2ResponseTimeoutClosureHonorsShutdownKill() { + std::cout << "\n[TEST] H2Upstream N9p: H2 response-timeout closure honors shutdown kill..." << std::endl; + // Code-inspection lock: verify the closure source contains the + // IsKilledForShutdown check. The closure is dispatcher-driven so + // a direct fire path requires a real dispatcher fixture; this is + // the lighter regression-prevention check. + bool pass = false; + try { + std::ifstream in("server/proxy_transaction.cc"); + std::string src((std::istreambuf_iterator(in)), + std::istreambuf_iterator()); + // Locate the H2 response-timeout closure (uniquely identified + // by its warn message) and confirm the guard is in scope. + auto warn = src.find("ProxyTransaction H2 response timeout client_fd="); + if (warn != std::string::npos) { + // Look backwards from the warn for the guard within ~400 chars. + const size_t lookback = warn > 400 ? warn - 400 : 0; + auto guard = src.find("IsKilledForShutdown()", lookback); + pass = (guard != std::string::npos && guard < warn); + } + TestFramework::RecordTest( + "H2Upstream N9p: H2 response-timeout closure honors shutdown kill", + pass, + pass ? "" : "IsKilledForShutdown check missing from response-timeout closure"); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9p: H2 response-timeout closure honors shutdown kill", + false, e.what()); + } +} + // --------------------------------------------------------------------------- // TestN9nFreshSessionBootstrapCallbackOrdering — Init's preface SETTINGS // is tracked as a control drain entry that OnTransportWriteComplete @@ -5596,6 +5713,8 @@ void RunAllH2UpstreamTests() { TestN9lPositiveTimeoutPostSubmit(); TestN9mSinkOnBodyChunkFalseStopsConsumption(); TestN9nFreshSessionBootstrapCallbackOrdering(); + TestN9oEarlyFinalHeadersInvalidateSendStallClosure(); + TestN9pH2ResponseTimeoutClosureHonorsShutdownKill(); TestN7eWiringEarlyHeadersThenIntermediateDataDispatch(); // TestB-series additions — wire-level From fa3172beb1a80ea4463fc086810c2928a92de5e3 Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 20:32:30 +0800 Subject: [PATCH 12/14] Fix review comment --- server/upstream_h2_connection.cc | 33 ++++++----- test/h2_upstream_test.h | 99 ++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 13 deletions(-) diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index e433562b..11bec0c5 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -846,19 +846,26 @@ void UpstreamH2Connection::FireSinkForDrainEntry( void UpstreamH2Connection::DropDrainEntriesForStream(int32_t stream_id) { if (drain_queue_.empty()) return; - auto new_end = std::remove_if( - drain_queue_.begin(), drain_queue_.end(), - [&](const PendingFrameDrain& e) { - // Only drop request-side entries for this stream; control - // entries carry stream_id=0 sentinel and must stay in the - // queue so the FIFO byte accounting remains accurate. - if (!e.is_control && e.stream_id == stream_id) { - bytes_in_drain_queue_ -= e.bytes; - return true; - } - return false; - }); - drain_queue_.erase(new_end, drain_queue_.end()); + // TOMBSTONE — do NOT erase. The reset stream's bytes are already + // sitting in the shared transport buffer ahead of (or interleaved + // with) sibling streams' bytes; erasing the entries and subtracting + // their bytes would skew bytes_in_drain_queue_ vs transport's + // `remaining` count, causing OnTransportWriteProgress's + // `remaining >= bytes_in_drain_queue_` early-return to skip + // attribution while the reset stream's leftover bytes drain. That + // starves sibling streams' OnRequestBodyProgress / OnRequestSubmitted + // until the reset stream's bytes fully clear — and if the reset + // stream's transport-buffered tail stalls, sibling streams can hit + // false send-stall timeouts. Convert the entries to is_control so + // FireSinkForDrainEntry skips dispatch, but keep their bytes in the + // FIFO sum so accounting stays byte-accurate to the wire. + for (auto& e : drain_queue_) { + if (!e.is_control && e.stream_id == stream_id) { + e.is_control = true; + e.is_data_frame = false; + e.is_end_stream = false; + } + } } void UpstreamH2Connection::OnTransportWriteProgress(size_t remaining) { diff --git a/test/h2_upstream_test.h b/test/h2_upstream_test.h index 2cc2618b..8acc17cb 100644 --- a/test/h2_upstream_test.h +++ b/test/h2_upstream_test.h @@ -4745,6 +4745,104 @@ void TestN9eResetStreamDropsDrainEntries() { } } +// --------------------------------------------------------------------------- +// TestN9qResetSiblingDoesNotStarveDrainAttribution — Multiplexed +// scenario: stream A submits a body; stream B submits then resets +// (its body bytes are already buffered in the transport ahead of / +// interleaved with A's). Tombstoning B's entries (vs erasing + +// subtracting bytes) keeps bytes_in_drain_queue_ accurate to the +// transport buffer's total — otherwise OnTransportWriteProgress's +// early-return (`remaining >= bytes_in_drain_queue_`) skips +// attribution while B's leftover bytes drain, starving A's +// OnRequestBodyProgress / OnRequestSubmitted and falsely triggering +// A's send-stall timeout. +// --------------------------------------------------------------------------- +void TestN9qResetSiblingDoesNotStarveDrainAttribution() { + std::cout << "\n[TEST] H2Upstream N9q: reset sibling does not starve drain attribution..." << std::endl; + struct ObservingSink : public RecordingSink { + int progress_calls = 0; + int submitted_calls = 0; + void OnRequestBodyProgress() override { ++progress_calls; } + void OnRequestSubmitted() override { ++submitted_calls; } + }; + try { + auto cfg = MakeH2Conn(); + // Sinks before conn — sinks-must-outlive-session contract. + ObservingSink sink_a; + ObservingSink sink_b; + UpstreamH2Connection conn(nullptr, cfg); + if (!conn.Init()) { + TestFramework::RecordTest( + "H2Upstream N9q: reset sibling does not starve drain attribution", + false, "Init failed"); + return; + } + // Submit stream A with a body. Frames are pushed to drain_queue_ + // via on_frame_send. + // 20KB body > MAX_FRAME_SIZE (16384) so A has ≥2 DATA frames + // (one intermediate → OnRequestBodyProgress, one END_STREAM → + // OnRequestSubmitted). Locks the "intermediate progress fires" + // half of the contract; a single-DATA-frame body would only + // exercise the OnRequestSubmitted half. + std::string body_a(20000, 'a'); + int32_t sid_a = conn.SubmitRequest( + "POST", "http", "example.com", "/a", {}, body_a, &sink_a); + if (sid_a <= 0) { + TestFramework::RecordTest( + "H2Upstream N9q: reset sibling does not starve drain attribution", + false, "submit A failed"); + return; + } + // Submit stream B with a body — its frames are appended to the + // queue AFTER A's frames (FIFO order in transport buffer too). + std::string body_b(20000, 'b'); + int32_t sid_b = conn.SubmitRequest( + "POST", "http", "example.com", "/b", {}, body_b, &sink_b); + if (sid_b <= 0) { + TestFramework::RecordTest( + "H2Upstream N9q: reset sibling does not starve drain attribution", + false, "submit B failed"); + return; + } + // Reset stream B before any drain. B's HEADERS+DATA bytes are + // already in transport; tombstoned entries stay in drain_queue_ + // with is_control=true so byte accounting matches transport. + // ResetStream also submits an RST_STREAM frame → another control + // entry appended. + conn.ResetStream(sid_b); + + // Full drain. With tombstoning, walking the queue dispatches + // A's HEADERS (no fire, not END_STREAM) → A's DATA frames + // (progress fires on intermediates, OnRequestSubmitted on the + // END_STREAM DATA) → B's tombstoned entries (no fire) → RST + // (no fire). A's sink must see ≥1 progress AND OnRequestSubmitted. + conn.OnTransportWriteComplete(); + + bool pass = (sink_a.submitted_calls == 1) && + (sink_a.progress_calls >= 1) && + (sink_b.progress_calls == 0) && + (sink_b.submitted_calls == 0); + std::string err; + if (sink_a.submitted_calls != 1) + err += "A submitted_calls=" + std::to_string(sink_a.submitted_calls) + + " (expected 1 — drain attribution starved); "; + if (sink_a.progress_calls < 1) + err += "A progress_calls=" + std::to_string(sink_a.progress_calls) + + " (expected ≥1); "; + if (sink_b.progress_calls != 0 || sink_b.submitted_calls != 0) + err += "B fired post-reset: progress=" + + std::to_string(sink_b.progress_calls) + + " submitted=" + std::to_string(sink_b.submitted_calls) + "; "; + TestFramework::RecordTest( + "H2Upstream N9q: reset sibling does not starve drain attribution", + pass, err); + } catch (const std::exception& e) { + TestFramework::RecordTest( + "H2Upstream N9q: reset sibling does not starve drain attribution", + false, e.what()); + } +} + // --------------------------------------------------------------------------- // TestN9fPartialDrainOfFinalFrame — A single-DATA-frame body // (END_STREAM on the only DATA frame) or the trailing DATA frame of a @@ -5704,6 +5802,7 @@ void RunAllH2UpstreamTests() { TestN9cDefaultSinkSurvivesNewVirtual(); TestN9dDeferredDrainSemantic(); TestN9eResetStreamDropsDrainEntries(); + TestN9qResetSiblingDoesNotStarveDrainAttribution(); TestN9fPartialDrainOfFinalFrame(); TestN9gControlFrameByteAccounting(); TestN9hHeadersOnlyShortReadCL(); From 5595cadace213477bd6e762569d3aa38b2691418 Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 21:34:09 +0800 Subject: [PATCH 13/14] Fix review comment --- server/upstream_h2_connection.cc | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/server/upstream_h2_connection.cc b/server/upstream_h2_connection.cc index 11bec0c5..648eb74e 100644 --- a/server/upstream_h2_connection.cc +++ b/server/upstream_h2_connection.cc @@ -374,10 +374,8 @@ bool UpstreamH2Connection::Init() { nghttp2_session_callbacks_set_on_stream_close_callback(cbs, &OnStreamCloseCallback); nghttp2_session_callbacks_set_on_header_callback(cbs, &OnHeaderCallback); nghttp2_session_callbacks_set_on_begin_headers_callback(cbs, &OnBeginHeadersCallback); - nghttp2_session_callbacks_set_on_data_chunk_recv_callback( - cbs, &OnDataChunkRecvCallback); - nghttp2_session_callbacks_set_on_frame_send_callback( - cbs, &OnFrameSendCallback); + nghttp2_session_callbacks_set_on_data_chunk_recv_callback(cbs, &OnDataChunkRecvCallback); + nghttp2_session_callbacks_set_on_frame_send_callback(cbs, &OnFrameSendCallback); int rv = nghttp2_session_client_new(&session_, cbs, this); nghttp2_session_callbacks_del(cbs); @@ -816,18 +814,17 @@ void UpstreamH2Connection::ResetStream(int32_t stream_id) { } void UpstreamH2Connection::EnqueueFrameForDrain(int32_t stream_id, - size_t bytes, - bool is_data_frame, - bool is_end_stream, - bool is_control) { + size_t bytes, + bool is_data_frame, + bool is_end_stream, + bool is_control) { drain_queue_.push_back( PendingFrameDrain{stream_id, bytes, is_data_frame, is_end_stream, is_control}); bytes_in_drain_queue_ += bytes; } -void UpstreamH2Connection::FireSinkForDrainEntry( - const PendingFrameDrain& entry) { +void UpstreamH2Connection::FireSinkForDrainEntry(const PendingFrameDrain& entry) { // Control frames are tracked for byte accounting only — never // dispatch sink virtuals for them (no stream to look up; the // sentinel stream_id is meaningless). From d176cc0c9be86154c91bfa5919c97c56b9017149 Mon Sep 17 00:00:00 2001 From: mwfj Date: Mon, 11 May 2026 21:48:12 +0800 Subject: [PATCH 14/14] Update docs --- README.md | 4 ++++ docs/architecture.md | 11 ++++++++--- docs/testing.md | 4 ++-- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index f406642e..a2d72ee4 100644 --- a/README.md +++ b/README.md @@ -9,9 +9,11 @@ A C++17 network server and gateway built on the Reactor pattern. It uses non-blo - **WebSocket** — RFC 6455 support: handshake, binary/text frames, fragmentation, close handshake, ping/pong - **TLS/SSL** — OpenSSL 3.x integration for downstream server TLS and upstream client TLS - **Upstream Proxy** — per-service connection pools with TLS, streaming response relay, retry policy, trailer handling, and header rewriting +- **HTTP/2 Upstream** — per-upstream opt-in multiplexed H2 client (donated-lease pattern), ALPN-negotiated `auto / always / never` dispatch, two-deadline send-stall + response-timeout model, transport-drain-driven sink dispatch, GOAWAY/PING liveness, live-reloadable session settings - **Rate Limiting** — per-client / per-route token-bucket middleware with LRU eviction, `RateLimit-*` headers, dry-run mode, hot reload - **Circuit Breaking** — per-upstream state machines, retry budgets, dry-run mode, wait-queue drain, hot-reloadable breaker tuning - **OAuth 2.0 Token Validation** — JWT validation with JWKS/OIDC discovery, multi-issuer policies, outbound identity headers, and RFC 7662 introspection mode +- **Observability (OpenTelemetry)** — W3C + Jaeger trace propagation, OTLP/JSON traces + metrics push, Prometheus pull `/metrics`, route-aware sampling, per-request span tree across inbound + auth + proxy + WS, idempotent finalize-CAS bookkeeping, four-phase graceful shutdown - **DNS and IPv6** — bind-host and upstream hostname resolution, IPv4/IPv6 family selection, stale-on-error reload handling - **Reactor Core** — edge-triggered epoll (Linux) / kqueue (macOS), non-blocking I/O, multi-threaded dispatcher pool - **Thread Pool** — configurable worker threads for event loop dispatchers @@ -496,6 +498,8 @@ make -C thread_pool | [docs/configuration.md](docs/configuration.md) | JSON config, environment variables, DNS, upstreams, rate limiting, structured logging | | [docs/oauth2.md](docs/oauth2.md) | OAuth 2.0 JWT and introspection validation | | [docs/circuit_breaker.md](docs/circuit_breaker.md) | Circuit breaker configuration, retry budgets, hot reload, observability | +| [docs/http2_upstream.md](docs/http2_upstream.md) | HTTP/2 upstream client — `prefer` modes, reload semantics, failure modes, tuning | +| [docs/observability.md](docs/observability.md) | OpenTelemetry — traces, metrics, propagators, sampling, OTLP / Prometheus configuration | ## Platform Support diff --git a/docs/architecture.md b/docs/architecture.md index 2568e117..81241828 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -28,10 +28,15 @@ The server uses the [Reactor pattern](https://en.wikipedia.org/wiki/Reactor_patt ``` Layer 7: AuthManager, AuthMiddleware, (inbound middleware stack) RateLimitManager, RateLimitZone, - TokenBucket, CircuitBreakerManager -Layer 6: UpstreamManager, UpstreamHostPool, (upstream connection pooling) + TokenBucket, CircuitBreakerManager, + ObservabilityManager, (cross-cutting: trace + metrics emission) + TracerProvider, MeterProvider +Layer 6: UpstreamManager, UpstreamHostPool, (upstream connection pooling + proxy engine) PoolPartition, UpstreamConnection, UpstreamLease, TlsClientContext, + ProxyHandler, ProxyTransaction, + UpstreamCodec + UpstreamHttpCodec (H1) + UpstreamH2Codec (H2), + UpstreamH2Connection, H2ConnectionTable (multiplexed H2 sessions), DnsResolver (hostname resolution, reload-time re-resolve) Layer 5: HttpServer (application entry point) Layer 4: HttpRouter, WebSocketConnection (routing, WS message API) @@ -45,7 +50,7 @@ Layer 1: ConnectionHandler, Channel, (reactor core) Dispatcher, EventHandler ``` -Layers 1–2 are the transport. Layers 3–5 are the protocol. Layer 6 is the gateway (upstream connectivity + DNS resolution). Layer 7 is the inbound traffic-management middleware (auth, rate limiting, circuit breaking). HTTP/1.x and HTTP/2 are parallel handlers at Layer 3, selected by `ProtocolDetector` at connection time. Both converge on the same `HttpRouter` at Layer 4. ConnectionHandler supports both inbound (server) and outbound (client) connections. +Layers 1–2 are the transport. Layers 3–5 are the protocol. Layer 6 is the gateway (upstream connectivity + proxy engine + DNS resolution). Layer 7 is the inbound traffic-management middleware (auth, rate limiting, circuit breaking, observability emission). HTTP/1.x and HTTP/2 are parallel handlers at Layer 3, selected by `ProtocolDetector` at connection time. Both converge on the same `HttpRouter` at Layer 4. ConnectionHandler supports both inbound (server) and outbound (client) connections. Upstream traffic mirrors the layering: the proxy engine dispatches per request through an H1 or H2 codec based on per-upstream `http2.enabled` + ALPN negotiation. H2 upstream sessions follow a donated-lease pattern — one real `UpstreamLease` is held for the multiplexed session lifetime; per-request transactions route as sentinel reuses through the existing session. `DnsResolver` is owned by `HttpServer` and is used at two points: (1) bind-host resolution during `Start()`, and (2) upstream hostname re-resolution during each `Reload()`. IP-literal upstreams bypass `DnsResolver` entirely. diff --git a/docs/testing.md b/docs/testing.md index 0d2ad47f..828dc406 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -3,7 +3,7 @@ ## Running Tests ```bash -make test # Build and run all suites (1021 tests across 35+ suites at HEAD) +make test # Build and run all suites (1379 tests across 41+ suites at HEAD) ./test_runner # Run all tests directly (after building) ./test_runner help # Print every supported flag @@ -87,7 +87,7 @@ make test_auth_race make test_auth_observability ``` -At current head, `./test_runner` reports **1021 / 1021 passing** (100 %). +At current head, `./test_runner` reports **1379 / 1379 passing** (100 %). ## Test Suites