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

[serrislew]

ConnectionTracker::Group kept its connection count in two mutually exclusive places: TxnState::reserve() returned the metric gauge instead of incrementing Group::_count when metric_enabled is set, and never touched _count at all. release() had the same split.

This was internally consistent — every read inside the tracker also branched on _count_metric != nullptr, so connection-max enforcement and the JSON/dump output all saw the right number. But _count has one consumer outside the tracker, in ServerSessionPool::eventHandler(): https://github.com/apache/trafficserver/blob/master/src/proxy/http/HttpSessionManager.cc#L316

bool connection_count_below_min = s->conn_track_group->_count <= s->conn_track_group->_min_keep_alive_conns;

With proxy.config.http.per_server.connection.metric_enabled non-zero, _count stays at 0, so this is always true and an idle keep-alive session is never closed on inactivity timeout — per_server.connection.min is effectively unbounded and idle origin connections accumulate.

Scope: the keep-alive minimum is the only affected behavior. Connection-max enforcement, blocking, and the exported metric values are unchanged.

Introduced from #12250

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes connection tracking so _count is always maintained even when per-server connection metrics are enabled, preventing outbound keep-alive minimum enforcement from becoming effectively unbounded.

Changes:

• Make _count the authoritative connection count and have the metric gauge mirror it (instead of replacing it).
• Update JSON/dump output to always report _count rather than branching on _count_metric.
• Simplify Group::release() logic to decrement _count first, then mirror into the metric gauge.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/iocore/net/ConnectionTracker.cc	Switches JSON/dump reporting and Group::release() logic to rely on _count as authoritative.
include/iocore/net/ConnectionTracker.h	Updates TxnState::reserve()/release() to always maintain _count and mirror into metrics when enabled.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

include/iocore/net/ConnectionTracker.h:455

• This change fixes a subtle behavioral bug (per-server connection metrics previously left Group::_count at 0, affecting keep-alive minimum enforcement). There doesn’t appear to be an AuTest covering proxy.config.http.per_server.connection.min behavior specifically when proxy.config.http.per_server.connection.metric_enabled is enabled, so this regression could slip back in. Consider adding/expanding a gold test under tests/gold_tests/origin_connection/ to assert that idle pooled origin connections above the configured minimum are closed on inactivity timeout when metrics are enabled, while connections at/below the minimum are kept alive.

  // @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);

[bneradt]

I think the change makes sense. Can we add an autest for this though?

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/iocore/net/ConnectionTracker.cc:543

• _count is std::atomic<int>, but this log line uses the PRId64 format macro and passes _count.load() (an int) to a variadic printf-style function. That’s undefined behavior on some platforms; use %d (or cast to int64_t if you want to keep PRId64).

    Error("Number of tracked connections should be greater than or equal to zero: %" PRId64, _count.load());

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.