fix(telemetry): DB connection leak in MetricStatsCollector — executor thread connections never returned to HikariCP pool

[spbolton]

Description

Problem

Database connections leak from the telemetry metrics collection system. Each invocation of MetricStatsCollector.getStats() orphans one connection per database. Connections accumulate indefinitely because HikariCP cannot reclaim them — close() is never called on the HikariConnection wrapper. With maxPoolSize=200, the pool drains over days until getConnection() calls block or timeout.

Surfaced at v26.01.02-01 but accumulating since v25.12.22-01 (commit e8ac669, Dec 15 2025). Production evidence: pg_stat_activity shows 30+ connections idle 11+ days, all with last_query from CountVariantsInAllArchivedExperimentsMetricType.

Root Cause

Commit e8ac669 moved metric queries onto a newSingleThreadExecutor() for timeout enforcement. DbConnectionFactory stores connections in a ThreadLocal. The executor thread opens a connection when the first query runs. shutdownNow() kills the thread without calling close(). The closeSilently() in the finally block runs on the calling thread and cannot reach the executor thread's ThreadLocal. The connection is orphaned permanently.

MetricsAPIImpl.getValue() has @CloseDBIfOpened but this annotation is implemented via ByteBuddy class instrumentation which cannot see through Weld CDI proxy subclasses — the annotation silently does nothing when invoked via the CDI proxy returned by APILocator.

HikariCP (SystemEnvDataSourceStrategy): maxLifetime=15min, idleTimeout=5min, leakDetectionThreshold=5min. None of these reclaim checked-out connections. They only apply to connections returned to the pool. leakDetectionThreshold logs a warning only — no close or reclaim.

Fix (this issue)

1. Wrap DBMetricType.getValue() default method in LocalTransaction.wrapReturn(). Each metric gets its own properly-scoped connection lifecycle, independent of @CloseDBIfOpened firing on CDI proxies. ContentTypeFieldsMetricType already uses this exact pattern in its overridden getStat(). Fixes all 130+ DBMetricType implementations at once.

2. Remove dead openDBConnection() call (line 114) and private method (lines 322-324) from MetricStatsCollector. These open and close a connection on the calling thread that no query ever uses — dead code left over from before the executor was introduced.

3. Add ArchUnit test to CodingStandardsArchTest: no CDI normal-scope bean should have methods annotated with @CloseDBIfOpened or @WrapInTransaction. ByteBuddy cannot see through Weld proxies so these annotations silently do nothing on CDI beans. 7 pre-existing violations will be surfaced.

Follow-up (separate issue)

Create CDI @interceptor versions of @CloseDBIfOpened and @WrapInTransaction. Adding @InterceptorBinding to the existing annotations and creating interceptor beans makes them work correctly on CDI-proxied beans, complementing ByteBuddy which continues to cover non-CDI classes. This is backward-compatible and clears all 7 ArchUnit violations system-wide.

Files

• dotCMS/src/main/java/com/dotcms/telemetry/collectors/DBMetricType.java (lines 19-21)
• dotCMS/src/main/java/com/dotcms/telemetry/collectors/MetricStatsCollector.java (lines 114, 322-324)
• dotCMS/src/test/java/com/dotcms/architecture/CodingStandardsArchTest.java (new test method)

Acceptance Criteria

• [Define what needs to be accomplished]

Additional Notes

[Any additional context or notes]

[spbolton]

Why the leak is slow / gradual

The leak is one connection per getStats() invocation per database. There are two invocation paths, both going through the same MetricStatsCollector.getStats() → executor thread pattern:

1. Cron job (primary source)

MetricsStatsJob is scheduled at 0 0 22 * * ? — once daily at 10pm. It collects all metrics with the FULL profile and persists them. Each run leaks one connection per database.

2. Usage dashboard API (secondary source)

UsageResource.getSummary() calls MetricStatsCollector.getStats() with the MINIMAL profile each time someone loads the usage dashboard. Each page load leaks one connection per database.

Accumulation rate

• Cron only: ~1 connection/day/DB
• Cron + dashboard: higher, depending on how often the dashboard is accessed
• The 30+ connections visible in pg_stat_activity after ~11 days (~3/day/DB) is consistent with cron running daily plus regular dashboard usage
• With maxPoolSize=200, cron-only rate would take ~200 days to exhaust the pool. Dashboard usage accelerates this significantly.

This is why the issue wasn't immediately visible after v25.12.22-01 — it accumulated quietly for weeks before pool exhaustion became noticeable.

[spbolton]

Implementation Guide for Developers

Change 1: Fix DBMetricType.getValue() default method

File: dotCMS/src/main/java/com/dotcms/telemetry/collectors/DBMetricType.java
Lines: 19-21

Before (current code):

@Override
default Optional<Object> getValue() {
    return APILocator.getMetricsAPI().getValue(getSqlQuery());
}

After (fixed):

@Override
default Optional<Object> getValue() {
    try {
        return LocalTransaction.wrapReturn(() ->
                APILocator.getMetricsAPI().getValue(getSqlQuery())
        );
    } catch (Exception e) {
        throw new DotRuntimeException(e);
    }
}

Why: LocalTransaction.wrapReturn() opens a connection (if none exists), executes the delegate, commits/rollbacks, and closes the connection in a finally block if it was newly opened. This is the established pattern — ContentTypeFieldsMetricType.getStat() already uses it. This fixes the leak for all 130+ DBMetricType implementations at once, independent of whether @CloseDBIfOpened fires on the CDI proxy.

Import needed: Add import com.dotmarketing.db.LocalTransaction; and import com.dotmarketing.exception.DotRuntimeException; to the file.

---

Change 2: Remove dead code from MetricStatsCollector

File: dotCMS/src/main/java/com/dotcms/telemetry/collectors/MetricStatsCollector.java

Line 114 — Delete this call:

openDBConnection();  // DELETE THIS LINE

Lines 322-324 — Delete this entire private method:

private void openDBConnection() {  // DELETE THIS ENTIRE METHOD
    DbConnectionFactory.getConnection();
}

Why: This was dead code left over from before commit e8ac669. It opens a connection on the calling thread that no query ever uses — all queries run on the executor thread now. With Change 1 in place, each metric properly manages its own connection, so this is unnecessary.

---

Change 3: Add ArchUnit test to prevent future regressions

File: dotCMS/src/test/java/com/dotcms/architecture/CodingStandardsArchTest.java

Add this new test method to the existing CodingStandardsArchTest class:

@Test
public void cdiNormalScopeBeansShouldNotUseBytebuddyTransactionAnnotations() {
    ArchRule rule = noClasses()
            .that().areAnnotatedWith(ApplicationScoped.class)
            .or().areAnnotatedWith(SessionScoped.class)
            .or().areAnnotatedWith(RequestScoped.class)
            .or().areAnnotatedWith(ConversationScoped.class)
            .should().haveAnyMethodsThat(are(annotatedWith(CloseDBIfOpened.class).or(annotatedWith(WrapInTransaction.class))))
            .because("@CloseDBIfOpened and @WrapInTransaction are implemented via ByteBuddy class instrumentation " +
                    "which cannot see through Weld CDI proxy subclasses — the annotations silently do nothing on CDI beans. " +
                    "Use LocalTransaction.wrapReturn() explicitly instead, or wait for the CDI interceptor follow-up issue.");

    rule.check(getProductionClasses());
}

Imports needed:

import javax.enterprise.context.ApplicationScoped;
import javax.enterprise.context.SessionScoped;
import javax.enterprise.context.RequestScoped;
import javax.enterprise.context.ConversationScoped;
import com.dotcms.business.CloseDBIfOpened;
import com.dotcms.business.WrapInTransaction;

Expected result: This test will fail on the first run, exposing 7 pre-existing violations:

• UniqueFieldDataBaseUtil.java — @ApplicationScoped, 20+ methods
• DBUniqueFieldValidationStrategy.java — @ApplicationScoped, 3 methods
• JobQueueManagerAPIImpl.java — @ApplicationScoped, 17+ methods
• MetricsAPIImpl.java — @ApplicationScoped, 3 methods
• CustomAttributeAPIImpl.java — @ApplicationScoped, 3 methods
• HostWebAPIImpl.java — @ApplicationScoped, 2 methods
• PermissionSaveHelper.java — @ApplicationScoped (verify actual violation)

For this PR: Suppress these 7 violations using ArchUnit's .ignoreDependency() or by excluding these specific classes from the rule. They predate this bug and will be fixed by the CDI interceptor follow-up issue.

Alternative: If you want to fix them now instead of suppressing, wrap the violating methods' DB calls in LocalTransaction.wrapReturn() or LocalTransaction.wrap() as appropriate.

---

Testing

1. Build:

   ./mvnw install -pl :dotcms-core -DskipTests

2. Unit test (if you add one — recommended but not required):

   • Mock 3+ DBMetricType implementations
   • Call MetricStatsCollector.getStats()
   • Verify no connections remain open: DbConnectionFactory.connectionExists() should return false

3. Integration test (if available):

   • Run MetricsStatsJob multiple times
   • Check no idle connections accumulate in the pool

4. Production validation (after deploy):

   • Monitor pg_stat_activity for connections with state = 'idle' and state_change age > 1 hour
   • Should drop to zero for telemetry-related queries

---

Build command

./mvnw install -pl :dotcms-core -DskipTests

[wezell]

I would move away from using the hand annotations or their classes here and just use the try-with-resources pattern to insure the connections are being properly managed. There is no need for the questionable "magic" that the LocalTransaction.wrapReturn brings here.

The code/methods should look like this:

@ApplicationScoped
public class MetricsFactoryImpl implements MetricsFactory {

    @Override
    public Optional<Object> getValue(final String sqlQuery) throws DotDataException {
        try(Connection conn = DbConnectionFactory.getConnection()) {
            final List<Map<String, Object>> loadObjectResults = new DotConnect().setSQL(sqlQuery).loadObjectResults(conn);
            if (loadObjectResults.isEmpty()) {
                return Optional.empty();
            }
            return Optional.ofNullable(loadObjectResults.get(0).get("value"));
        }catch(SQLException e) {
            throw new DotDataException(e);
        }
    }

[spbolton]

I would move away from using the hand annotations or their classes here and just use the try-with-resources pattern to insure the connections are being properly managed. There is no need for the questionable "magic" that the LocalTransaction.wrapReturn brings here.

The code/methods should look like this:

@ApplicationScoped
public class MetricsFactoryImpl implements MetricsFactory {

@Override
public Optional<Object> getValue(final String sqlQuery) throws DotDataException {
    try(Connection conn = DbConnectionFactory.getConnection()) {
        final List<Map<String, Object>> loadObjectResults = new DotConnect().setSQL(sqlQuery).loadObjectResults(conn);
        if (loadObjectResults.isEmpty()) {
            return Optional.empty();
        }
        return Optional.ofNullable(loadObjectResults.get(0).get("value"));
    }catch(SQLException e) {
        throw new DotDataException(e);
    }
}

There is a fatal flaw that prevents the above from working properly especially within our current infrastructure. If the caller has created a connection then it is stored in the thread local. When this code is called, DbConnectionFactory.getConnection() will return the same connection as the caller as there is a current open connection there, but when the try-with-resource finishes it will close that connection, when the method now returns the parent method has a closed connection.

The Annotations and the LocalTransaction.wrapReturn are there to check specifically if there is an existing connection and create one if there is not, it will only close the connection on return if it created it, if the caller already had the connection open it will not close it, this follows the best practice pattern that whoever creates a connection is responsible for closing it.

Now saying that the code I had swapped out the @CloseDBIfOpened that only deals with managing the connection whereas the LocalTransaction.wrapReturn does the same as the @WrapInTransaction class that also creates and maintains a transaction if not already in one and should not be used when needing read only. The equivalent code to @CloseDbIfOpened would be the following

   @Override
    default Optional<Object> getValue() {
        // Check if connection exists before we execute
        final boolean isNewConnection = !DbConnectionFactory.connectionExists();

        try {
            return APILocator.getMetricsAPI().getValue(getSqlQuery());
        } finally {
            // Only close if we opened the connection (matches @CloseDBIfOpened behavior)
            if (isNewConnection && DbConnectionFactory.connectionExists()) {
                DbConnectionFactory.closeSilently();
            }
        }
    }

If having to do this in many places this boilerplate becomes cumbersome and hard to manage so where it is not possible to used @CloseDBIfOpened and improvment would be to create a similar wrap connection to remove the boilerplate. e.g.

public List<User> findActiveUsers() throws DotDataException {                                                                                                                                                                     
      return DbConnectionFactory.wrapConnection(() -> {                                                                                                                                                                             
          DotConnect dc = new DotConnect();                                                                                                                                                                                         
          dc.setSQL("SELECT * FROM user_ WHERE active = true");                                                                                                                                                                     
          return dc.loadObjectResults();                                                                                                                                                                                            
      });                                                                                                                                                                                                                           
  }       

If @CloseDBIfOpened is available everywhere, the annotation is the preferable case, but to be able to wrap like this is useful when that is not possible or in tests.

It would be possible to use the try-with-resource pattern saftely but we would have to make DbConnectionFactory.getConnection() return a special wrapped connection that when the close is called does not actually close the underlying connection if it was there before the initial get.

[spbolton]

This issue is being addressed in PR #34490

[github-actions]

PRs linked to this issue

• fix(telemetry): fixtelemetry DB connection leak in MetricStatsColl (#34480) by @spbolton