http: fix http/1 client low watermark crash

source/common/http/http1/codec_impl.cc

@@ -1080,11 +1080,9 @@ void ClientConnectionImpl::onAboveHighWatermark() {
 }
 
 void ClientConnectionImpl::onBelowLowWatermark() {
-  // This can get called without an active stream/request when upstream decides to do bad things
-  // such as sending multiple responses to the same request, causing us to close the connection, but
-  // in doing so go below low watermark.
-  if (pending_response_.has_value()) {
-    ASSERT(!pending_response_done_);
+  // This can get called without an active stream/request when the response completion causes us to
+  // close the connection, but in doing so go below low watermark.
+  if (pending_response_.has_value() && !pending_response_done_) {
     pending_response_.value().encoder_.runLowWatermarkCallbacks();
   }
 }

source/common/http/http1/codec_impl.h

@@ -529,6 +529,11 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
   }
 
   absl::optional<PendingResponse> pending_response_;
+  // TODO(mattklein123): The following bool tracks whether a pending response is complete before
+  // dispatching callbacks. This is needed so that pending_response_ stays valid during callbacks
+  // in order to access the stream, but to avoid invoking callbacks that shouldn't be called once
+  // the response is complete. The existence of this variable is hard to reason about and it should
+  // be combined with pending_response_ somehow in a follow up cleanup.
   bool pending_response_done_{true};
   // Set true between receiving 100-Continue headers and receiving the spurious onMessageComplete.
   bool ignore_message_complete_for_100_continue_{};

test/common/http/http1/codec_impl_test.cc

@@ -1931,6 +1931,37 @@ TEST_F(Http1ClientConnectionImplTest, HighwatermarkMultipleResponses) {
       ->onUnderlyingConnectionBelowWriteBufferLowWatermark();
 }
 
+// Regression test for https://github.com/envoyproxy/envoy/issues/10655. Make sure we correctly
+// handle going below low watermark when closing the connection during a completion callback.
+TEST_F(Http1ClientConnectionImplTest, LowWatermarkDuringClose) {
+  initialize();
+
+  InSequence s;
+
+  NiceMock<MockResponseDecoder> response_decoder;
+  Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
+  Http::MockStreamCallbacks stream_callbacks;
+  request_encoder.getStream().addCallbacks(stream_callbacks);
+
+  TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+  request_encoder.encodeHeaders(headers, true);
+
+  // Fake a call from the underlying Network::Connection and verify the stream is notified.
+  EXPECT_CALL(stream_callbacks, onAboveWriteBufferHighWatermark());
+  static_cast<ClientConnection*>(codec_.get())
+      ->onUnderlyingConnectionAboveWriteBufferHighWatermark();
+
+  EXPECT_CALL(response_decoder, decodeHeaders_(_, true))
+      .WillOnce(Invoke([&](ResponseHeaderMapPtr&, bool) {
+        // Fake a call for going below the low watermark. Make sure no stream callbacks get called.
+        EXPECT_CALL(stream_callbacks, onBelowWriteBufferLowWatermark()).Times(0);
+        static_cast<ClientConnection*>(codec_.get())
+            ->onUnderlyingConnectionBelowWriteBufferLowWatermark();
+      }));
+  Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n");
+  codec_->dispatch(response);
+}
+
 TEST_F(Http1ServerConnectionImplTest, LargeTrailersRejected) {
   // Default limit of 60 KiB
   std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n";