HTTP/2 to origin session pool and reliability fixes

doc/admin-guide/files/records.yaml.en.rst

@@ -5102,6 +5102,13 @@ HTTP/2 Configuration
    code of ENHANCE_YOUR_CALM.
    Any negative value configures no limit to the number of SETTINGS frames received.
 
+   SETTINGS frames carrying the ACK flag are not counted against this limit.
+   They are mandatory protocol responses to SETTINGS frames |TS| sent and
+   therefore cannot be used by a peer to flood |TS|; counting them would
+   spuriously close healthy connections (in particular outbound HTTP/2
+   sessions where :ts:cv:`proxy.config.http2.flow_control.policy_out` causes
+   |TS| to send a SETTINGS frame per outbound stream).
+
 .. ts:cv:: CONFIG proxy.config.http2.max_ping_frames_per_minute INT 60
    :reloadable:
 

include/proxy/ProxyTransaction.h

@@ -146,6 +146,22 @@ class ProxyTransaction : public VConnection
   virtual void set_rx_error_code(ProxyError e);
   virtual void set_tx_error_code(ProxyError e);
 
+  /** Whether the request on this transaction is known by the protocol layer to
+   *  be safe to retry on a fresh origin connection.
+   *
+   *  The default is @c false. A subclass should return @c true only when the
+   *  underlying protocol guarantees the origin did not process (and could not
+   *  have observed) the request -- for example, when an HTTP/2 origin sends a
+   *  GOAWAY whose last_stream_id is below this transaction's stream id, or
+   *  sends a RST_STREAM with the REFUSED_STREAM error code (RFC 9113 6.8 and
+   *  8.7). HttpSM uses this to allow retrying non-idempotent methods that
+   *  would otherwise be considered too risky to replay.
+   *
+   *  @return @c true if HttpSM may safely retry the request on a different
+   *          origin connection regardless of method.
+   */
+  virtual bool is_safe_to_retry() const;
+
   bool support_sni() const;
 
   void mark_as_tunnel_endpoint() override;

include/proxy/http2/Http2CommonSession.h

@@ -101,14 +101,14 @@ class Http2CommonSession
 
   ////////////////////
   // Accessors
-  void set_dying_event(int event);
-  int  get_dying_event() const;
-  bool ready_to_free() const;
-  bool is_recursing() const;
-  void set_half_close_local_flag(bool flag);
-  bool get_half_close_local_flag() const;
-  bool is_url_pushed(const char *url, int url_len);
-  void add_url_to_pushed_table(const char *url, int url_len);
+  void         set_dying_event(int event);
+  int          get_dying_event() const;
+  bool         ready_to_free() const;
+  bool         is_recursing() const;
+  virtual void set_half_close_local_flag(bool flag);
+  bool         get_half_close_local_flag() const;
+  bool         is_url_pushed(const char *url, int url_len);
+  void         add_url_to_pushed_table(const char *url, int url_len);
 
   // Record history from Http2ConnectionState
   void remember(const SourceLocation &location, int event, int reentrant = NO_REENTRANT);

include/proxy/http2/Http2ServerSession.h

@@ -58,6 +58,20 @@ class Http2ServerSession : public PoolableSession, public Http2CommonSession
   void add_session() override;
   void remove_session();
 
+  /** Eagerly drop a half-closed session out of the server-session pool.
+   *
+   *  Sets the half-close-local flag (delegating to the base class) and, when
+   *  the flag transitions to @c true, immediately removes this session from
+   *  the per-thread server-session pool. Once @c half_close_local is set,
+   *  every subsequent @c create_initiating_stream on this session
+   *  short-circuits with @c REFUSED_STREAM, so leaving it discoverable in
+   *  the pool produces a stream of REFUSED_STREAM aborts on otherwise
+   *  unrelated requests until the session is finally torn down. The pool
+   *  must be made aware as soon as the half-close decision is made, not at
+   *  session destruction time.
+   */
+  void set_half_close_local_flag(bool flag) override;
+
   ////////////////////
   // Accessors
   sockaddr const *get_remote_addr() const override;

include/proxy/http2/Http2Stream.h

@@ -156,13 +156,27 @@ class Http2Stream : public ProxyTransaction
   void            set_rx_error_code(ProxyError e) override;
   void            set_tx_error_code(ProxyError e) override;
 
+  bool is_safe_to_retry() const override;
+
+  /** Mark this stream as known-not-processed by the origin.
+   *
+   *  Called by Http2ConnectionState when the origin has explicitly indicated
+   *  that this stream's request was not (and will not be) processed -- either
+   *  because a GOAWAY arrived whose @c last_stream_id is below this stream's
+   *  id, or because a RST_STREAM with REFUSED_STREAM was received for it (RFC
+   *  9113 sections 6.8 and 8.7). HttpSM consults @c is_safe_to_retry() when
+   *  deciding whether a non-idempotent request may be retried.
+   */
+  void set_safe_to_retry();
+
   bool        has_request_body(int64_t content_length, bool is_chunked_set) const override;
   HTTPVersion get_version(HTTPHdr &hdr) const override;
 
   void mark_milestone(Http2StreamMilestone type);
 
   void             increment_data_length(uint64_t length);
   bool             payload_length_is_valid() const;
+  void             cache_send_request_for_response_validation();
   bool             is_write_vio_done() const;
   void             update_sent_count(unsigned num_bytes);
   Http2StreamId    get_id() const;
@@ -260,6 +274,13 @@ class Http2Stream : public ProxyTransaction
   /** Whether the stream has been registered with the connection state. */
   bool _registered_stream = true;
 
+  // Set by Http2ConnectionState when the origin has explicitly indicated
+  // (via GOAWAY whose last_stream_id is below this stream's id, or via
+  // RST_STREAM with REFUSED_STREAM) that this stream's request was not
+  // processed and may be safely retried even for non-idempotent methods.
+  // See Http2Stream::is_safe_to_retry / set_safe_to_retry.
+  bool _safe_to_retry = false;
+
   // A brief discussion of similar flags and state variables:  _state, closed, terminate_stream
   //
   // _state tracks the HTTP2 state of the stream.  This field completely coincides with the H2 spec.
@@ -286,6 +307,17 @@ class Http2Stream : public ProxyTransaction
   uint64_t data_length = 0;
   uint64_t bytes_sent  = 0;
 
+  // Snapshot of the send-side request taken before `_send_header` is destroyed
+  // in `update_write_request`. These are used by `payload_length_is_valid` to
+  // apply the [RFC 9110] 8.6 / [RFC 7230] 3.3.2 payload preclusion rules to
+  // origin responses on outbound streams. Without this snapshot the request
+  // method (e.g. HEAD) and the presence of conditional request headers would
+  // already be lost by the time the response is validated, causing valid HEAD
+  // and 304 responses with non-zero Content-Length to be rejected as protocol
+  // errors.
+  int      _cached_send_method_wksidx     = -1;
+  uint64_t _cached_send_conditional_field = 0;
+
   ssize_t _peer_rwnd  = 0;
   ssize_t _local_rwnd = 0;
 
@@ -398,18 +430,50 @@ Http2Stream::increment_data_length(uint64_t length)
   data_length += length;
 }
 
+inline void
+Http2Stream::cache_send_request_for_response_validation()
+{
+  // On outbound streams `_send_header` is destroyed in `update_write_request`
+  // immediately after the request HEADERS frame is encoded and sent. Capture
+  // the request method and the presence of any conditional request headers
+  // here so that the response-side `payload_length_is_valid` check can still
+  // honor the [RFC 9110] 8.6 payload preclusion rules for HEAD responses and
+  // for 304 responses to conditional GETs.
+  if (!this->is_outbound_connection() || !_send_header.valid() || _send_header.type_get() != HTTPType::REQUEST) {
+    return;
+  }
+  uint64_t const conditional_mask = (MIME_PRESENCE_IF_UNMODIFIED_SINCE | MIME_PRESENCE_IF_MODIFIED_SINCE | MIME_PRESENCE_IF_RANGE |
+                                     MIME_PRESENCE_IF_MATCH | MIME_PRESENCE_IF_NONE_MATCH);
+  _cached_send_method_wksidx      = _send_header.method_get_wksidx();
+  _cached_send_conditional_field  = _send_header.presence(conditional_mask);
+}
+
 inline bool
 Http2Stream::payload_length_is_valid() const
 {
-  uint32_t content_length = _receive_header.get_content_length();
-  uint64_t mask           = (MIME_PRESENCE_IF_UNMODIFIED_SINCE | MIME_PRESENCE_IF_MODIFIED_SINCE | MIME_PRESENCE_IF_RANGE |
-                   MIME_PRESENCE_IF_MATCH | MIME_PRESENCE_IF_NONE_MATCH);
-
-  // Skip Content-Length check on [RFC 7230] 3.3.2 conditions
-  bool is_payload_precluded =
-    this->is_outbound_connection() && (_send_header.method_get_wksidx() == HTTP_WKSIDX_HEAD ||
-                                       (_send_header.method_get_wksidx() == HTTP_WKSIDX_GET && _send_header.presence(mask) &&
-                                        _receive_header.status_get() == HTTPStatus::NOT_MODIFIED));
+  uint32_t const content_length = _receive_header.get_content_length();
+
+  // Apply the [RFC 9110] 8.6 / [RFC 7230] 3.3.2 payload preclusion rules to
+  // origin responses on outbound streams. The send-side `_send_header` may
+  // already have been torn down by this point, so consult the cached
+  // request metadata captured by `cache_send_request_for_response_validation`.
+  bool is_payload_precluded = false;
+  if (this->is_outbound_connection()) {
+    if (_cached_send_method_wksidx == HTTP_WKSIDX_HEAD) {
+      is_payload_precluded = true;
+    } else if (_cached_send_method_wksidx == HTTP_WKSIDX_GET && _cached_send_conditional_field != 0) {
+      // `HTTPHdr::status_get()` asserts the underlying header has response
+      // polarity, but on the outbound origin-response path `_receive_header`
+      // is still in HTTP/2 form at this point and has not yet been converted,
+      // so the polarity may not be set. Read the `:status` pseudo-header
+      // directly to detect a 304 response to a conditional GET.
+      if (MIMEField const *const status_field = _receive_header.field_find(PSEUDO_HEADER_STATUS); status_field != nullptr) {
+        auto const       sv{status_field->value_get()};
+        HTTPStatus const status = http_parse_status(sv.data(), sv.data() + sv.length());
+        is_payload_precluded    = (status == HTTPStatus::NOT_MODIFIED);
+      }
+    }
+  }
 
   if (content_length != 0 && !is_payload_precluded && content_length != data_length) {
     Warning("Bad payload length content_length=%d data_legnth=%d session_id=%" PRId64, content_length,

src/proxy/ProxyTransaction.cc

@@ -95,6 +95,12 @@ ProxyTransaction::set_tx_error_code(ProxyError e)
   }
 }
 
+bool
+ProxyTransaction::is_safe_to_retry() const
+{
+  return false;
+}
+
 NetVConnection *
 ProxyTransaction::get_netvc() const
 {

src/proxy/http/HttpSM.cc

@@ -6430,7 +6430,26 @@ HttpSM::handle_server_setup_error(int event, void *data)
   [[maybe_unused]] UnixNetVConnection *dbg_vc = nullptr;
   switch (event) {
   case VC_EVENT_EOS:
-    t_state.current.state = HttpTransact::CONNECTION_CLOSED;
+    // If the underlying transport (e.g. an HTTP/2 stream) has signaled that
+    // the origin guaranteed it never processed this request -- because of a
+    // GOAWAY whose last_stream_id is below this stream's id, or a RST_STREAM
+    // with REFUSED_STREAM (RFC 9113 6.8 and 8.7) -- treat the failure as a
+    // connection-level error so HttpTransact::is_request_retryable allows
+    // retrying even non-idempotent methods such as POST. Otherwise this
+    // would surface to the client as ERR_CLIENT_ABORT despite the request
+    // being explicitly safe to replay on a fresh connection.
+    if (server_txn != nullptr && server_txn->is_safe_to_retry()) {
+      t_state.current.state = HttpTransact::CONNECTION_ERROR;
+      // A retry with a request body needs to use the complete copy retained
+      // by request buffering. The first origin attempt clears
+      // is_buffering_request_body after consuming that copy, so restore the
+      // flag before setting up the retry's request-body tunnel.
+      if (this->is_postbuf_valid() && this->get_postbuf_done()) {
+        is_buffering_request_body = true;
+      }
+    } else {
+      t_state.current.state = HttpTransact::CONNECTION_CLOSED;
+    }
     t_state.set_connect_fail(EPIPE);
     break;
   case VC_EVENT_ERROR:

src/proxy/http/HttpSessionManager.cc

@@ -420,19 +420,27 @@ HttpSessionManager::acquire_session(HttpSM *sm, sockaddr const *ip, const char *
     to_return = nullptr;
   }
 
-  // Otherwise, check the thread pool first
-  if (this->get_pool_type() == TS_SERVER_SESSION_SHARING_POOL_THREAD ||
-      this->get_pool_type() == TS_SERVER_SESSION_SHARING_POOL_HYBRID) {
-    retval = _acquire_session(ip, hostname_hash, sm, match_style, TS_SERVER_SESSION_SHARING_POOL_THREAD);
-  }
+  // Always check the thread-local pool first. Multiplexing server sessions
+  // (HTTP/2, HTTP/3) cannot be safely shared across threads -- their state is
+  // owned by the EThread that drives their connection -- so they are filed
+  // exclusively in the per-thread pool by `Http2ServerSession::add_session`
+  // (and analogous code for HTTP/3). If the configured pool type is `global`
+  // or `global_locked`, only the global pool would otherwise be consulted,
+  // which means an existing H/2 origin connection on this thread is invisible
+  // to the lookup. Each new request would then open a fresh TCP+TLS+H/2
+  // handshake to the origin, defeating multiplexing entirely. Trying the
+  // thread-local pool first restores within-thread H/2 origin reuse without
+  // changing behavior for HTTP/1.x sessions, which fall through to the
+  // configured pool below on a thread-local miss.
+  retval = _acquire_session(ip, hostname_hash, sm, match_style, TS_SERVER_SESSION_SHARING_POOL_THREAD);
 
-  //  If you didn't get a match, and the global pool is an option go there.
   if (retval != HSMresult_t::DONE) {
     if (TS_SERVER_SESSION_SHARING_POOL_GLOBAL == this->get_pool_type() ||
         TS_SERVER_SESSION_SHARING_POOL_HYBRID == this->get_pool_type()) {
       retval = _acquire_session(ip, hostname_hash, sm, match_style, TS_SERVER_SESSION_SHARING_POOL_GLOBAL);
-    } else if (TS_SERVER_SESSION_SHARING_POOL_GLOBAL_LOCKED == this->get_pool_type())
+    } else if (TS_SERVER_SESSION_SHARING_POOL_GLOBAL_LOCKED == this->get_pool_type()) {
       retval = _acquire_session(ip, hostname_hash, sm, match_style, TS_SERVER_SESSION_SHARING_POOL_GLOBAL_LOCKED);
+    }
   }
 
   return retval;