Fix enabling per-server metrics that disables the outbound keep-alive minimum

include/iocore/net/ConnectionTracker.h

@@ -448,25 +448,23 @@ inline int
 ConnectionTracker::TxnState::reserve()
 {
   _reserved_p = true;
-  // If metric enabled, use metric as count
+  // @a _count is always the authoritative count; the metrics, if enabled, only mirror it.
+  auto count = ++_g->_count;
   if (_g->_count_metric != nullptr) {
     ts::Metrics::Gauge::increment(_g->_count_metric);
     ts::Metrics::Counter::increment(_g->_count_total_metric);
-    return _g->_count_metric->load();
   }
-  return ++_g->_count;
+  return count;
 }
 
 inline void
 ConnectionTracker::TxnState::release()
 {
   if (_reserved_p) {
     _reserved_p = false;
-    // If metric enabled, use metric as count
+    --_g->_count;
     if (_g->_count_metric != nullptr) {
       ts::Metrics::Gauge::decrement(_g->_count_metric);
-    } else {
-      --_g->_count;
     }
   }
 }

src/iocore/net/ConnectionTracker.cc

@@ -187,8 +187,8 @@ Groups_To_JSON(std::vector<std::shared_ptr<ConnectionTracker::Group const>> cons
   static const std::string_view trailer{" \n]}"};
 
   static const auto printer = [](swoc::BufferWriter &w, ConnectionTracker::Group const *g) -> swoc::BufferWriter & {
-    w.print(item_fmt, g->_match_type, g->_addr, g->_fqdn, g->_count_metric != nullptr ? g->_count_metric->load() : g->_count.load(),
-            g->_count_max.load(), g->_blocked.load(), g->get_last_alert_epoch_time());
+    w.print(item_fmt, g->_match_type, g->_addr, g->_fqdn, g->_count.load(), g->_count_max.load(), g->_blocked.load(),
+            g->get_last_alert_epoch_time());
     return w;
   };
 
@@ -522,26 +522,13 @@ ConnectionTracker::Group::should_alert(std::time_t *lat)
 void
 ConnectionTracker::Group::release()
 {
-  // If metric enabled, use metric as count
-  if (_count_metric != nullptr) {
-    if (_count_metric->load() > 0) {
+  // @a _count is always the authoritative count; the metric, if enabled, only mirrors it.
+  if (_count > 0) {
+    auto count = --_count;
+    if (_count_metric != nullptr) {
       ts::Metrics::Gauge::decrement(_count_metric);
-      if (_count_metric->load() == 0) {
-        TableSingleton             &table = _direction == DirectionType::INBOUND ? _inbound_table : _outbound_table;
-        std::lock_guard<std::mutex> lock(table._mutex); // Table lock
-        if (_count_metric->load() > 0) {
-          // Someone else grabbed the Group between our last check and taking the
-          // lock.
-          return;
-        }
-        table._table.erase(_key);
-      }
-    } else {
-      // A bit dubious, as there's no guarantee it's still negative, but even that would be interesting to know.
-      Error("Number of tracked connections should be greater than or equal to zero: %" PRId64, _count_metric->load());
     }
-  } else if (_count > 0) {
-    if (--_count == 0) {
+    if (count == 0) {
       TableSingleton             &table = _direction == DirectionType::INBOUND ? _inbound_table : _outbound_table;
       std::lock_guard<std::mutex> lock(table._mutex); // Table lock
       if (_count > 0) {
@@ -553,7 +540,7 @@ ConnectionTracker::Group::release()
     }
   } else {
     // A bit dubious, as there's no guarantee it's still negative, but even that would be interesting to know.
-    Error("Number of tracked connections should be greater than or equal to zero: %u", _count.load());
+    Error("Number of tracked connections should be greater than or equal to zero: %d", _count.load());
   }
 }
 
@@ -615,8 +602,7 @@ ConnectionTracker::dump_outbound(FILE *f)
 
     for (std::shared_ptr<Group const> g : groups) {
       swoc::LocalBufferWriter<128> w;
-      w.print("{:7} | {:5} | {:24} | {:33} | {:8} |\n", (g->_count_metric != nullptr ? g->_count_metric->load() : g->_count.load()),
-              g->_blocked.load(), g->_addr, g->_hash, g->_match_type);
+      w.print("{:7} | {:5} | {:24} | {:33} | {:8} |\n", g->_count.load(), g->_blocked.load(), g->_addr, g->_hash, g->_match_type);
       fwrite(w.data(), w.size(), 1, f);
     }
 

tests/gold_tests/origin_connection/per_server_metric_enabled.replay.yaml

@@ -0,0 +1,53 @@
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you under the Apache License, Version 2.0 (the
+#  "License"); you may not use this file except in compliance
+#  with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+# Test Notes:
+# A single transaction which leaves one keep-alive origin connection pooled
+# behind it. The test driving this replay file verifies that the outbound
+# connection tracker keeps an accurate connection count while
+# proxy.config.http.per_server.connection.metric_enabled is set, so that the
+# pooled connection is reaped once the keep alive timeout expires.
+
+meta:
+  version: "1.0"
+
+sessions:
+
+- transactions:
+
+  - client-request:
+      method: GET
+      url: /some/path/first
+      version: '1.1'
+      headers:
+        fields:
+        - [ Host, www.example.com ]
+        - [ Content-Length, 0 ]
+        - [ uuid, first-request ]
+
+    server-response:
+      status: 200
+      reason: OK
+      headers:
+        fields:
+        - [ Content-Length, 16 ]
+        - [ X-Response, first-response ]
+
+    proxy-response:
+      status: 200
+      headers:
+        fields:
+        - [ X-Response, {value: 'first-response', as: equal } ]

tests/gold_tests/origin_connection/per_server_metric_enabled.test.py

@@ -0,0 +1,113 @@
+'''
+Verify per_server connection tracking stays accurate when
+proxy.config.http.per_server.connection.metric_enabled is set.
+'''
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you under the Apache License, Version 2.0 (the
+#  "License"); you may not use this file except in compliance
+#  with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+Test.Summary = __doc__
+
+Test.SkipIf(Condition.CurlUsingUnixDomainSocket())
+
+
+class PerServerMetricEnabledTest:
+    """Verify that enabling the per_server metrics does not break the group connection count.
+
+    Enabling proxy.config.http.per_server.connection.metric_enabled used to
+    make the metric the authoritative connection count, leaving the group's
+    internal counter at zero. That made every pooled origin session look like
+    it was at or below proxy.config.http.per_server.connection.min, so
+    keep-alive origin connections were never reaped on inactivity timeout.
+    """
+
+    _replay_file: str = 'per_server_metric_enabled.replay.yaml'
+    _keep_alive_timeout: int = 2
+
+    def __init__(self) -> None:
+        """Configure the test processes in preparation for the TestRun."""
+        self._configure_server()
+        self._configure_trafficserver()
+
+    def _configure_server(self) -> None:
+        """Configure the origin server to be used in the test."""
+        self._server = Test.MakeVerifierServerProcess('metric_enabled_server', self._replay_file)
+
+    def _configure_trafficserver(self) -> None:
+        """Configure Traffic Server to be used in the test."""
+        self._ts = Test.MakeATSProcess("ts_metric_enabled")
+        self._ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{self._server.Variables.http_port}')
+        self._ts.Disk.records_config.update(
+            {
+                'proxy.config.diags.debug.enabled': 1,
+                'proxy.config.diags.debug.tags': 'http_ss|conn_track',
+                'proxy.config.http.per_server.connection.metric_enabled': 1,
+                'proxy.config.http.per_server.connection.metric_prefix': 'bar',
+                'proxy.config.http.per_server.connection.match': 'port',
+                # No minimum number of keep alive origin connections: the pooled
+                # connection should be closed once it times out.
+                'proxy.config.http.per_server.connection.min': 0,
+                'proxy.config.http.keep_alive_no_activity_timeout_out': self._keep_alive_timeout,
+                'proxy.config.http.server_session_sharing.pool': 'global',
+            })
+        # The connection count should never be decremented below zero.
+        self._ts.Disk.diags_log.Content += Testers.ExcludesExpression(
+            'Number of tracked connections should be greater than or equal to zero',
+            'Verify the group connection count is not double decremented.')
+
+    def _test_connection_is_reaped(self) -> None:
+        """Verify the idle origin connection is closed once it times out."""
+        tr = Test.AddTestRun("Verify the idle keep-alive origin connection is reaped")
+        tr.Processes.Default.Command = (
+            f'sleep {self._keep_alive_timeout * 3}; '
+            'traffic_ctl metric get proxy.process.http.current_server_connections; '
+            'traffic_ctl metric match per_server')
+        tr.Processes.Default.ReturnCode = 0
+        tr.Processes.Default.Env = self._ts.Env
+        tr.Processes.Default.Streams.All += Testers.ContainsExpression(
+            'proxy.process.http.current_server_connections 0',
+            'The idle origin connection should have been closed by the keep-alive timeout.')
+        tr.Processes.Default.Streams.All += Testers.ContainsExpression(
+            f'per_server.current_connection.bar.127.0.0.1:{self._server.Variables.http_port} 0',
+            'The per_server connection gauge should have been decremented back to zero.')
+        tr.Processes.Default.Streams.All += Testers.ContainsExpression(
+            f'per_server.total_connection.bar.127.0.0.1:{self._server.Variables.http_port} 1',
+            'A single origin connection should have been tracked.')
+
+    def _test_tracker_info(self) -> None:
+        """Verify the JSONRPC connection tracker report agrees with the metrics."""
+        tr = Test.AddTestRun("Verify the connection tracker report")
+        tr.Processes.Default.Command = "traffic_ctl rpc invoke get_connection_tracker_info -p 'table: outbound' -f json"
+        tr.Processes.Default.ReturnCode = 0
+        tr.Processes.Default.Env = self._ts.Env
+        # Once the connection is released the group count drops to zero and the
+        # group is removed from the table, so either the table is empty or the
+        # remaining group reports no current connections.
+        tr.Processes.Default.Streams.All += Testers.ContainsExpression(
+            r'"(count|current)":\s*"?0"?', 'The tracker should report no current outbound connections.')
+
+    def run(self) -> None:
+        """Configure the TestRuns."""
+        tr = Test.AddTestRun('Perform a transaction that leaves a pooled origin connection')
+        tr.Processes.Default.StartBefore(self._server)
+        tr.Processes.Default.StartBefore(self._ts)
+
+        tr.AddVerifierClientProcess('metric_enabled_client', self._replay_file, http_ports=[self._ts.Variables.port])
+
+        self._test_connection_is_reaped()
+        self._test_tracker_info()
+
+
+PerServerMetricEnabledTest().run()