dotCMS / dotCMS/core

fix(health): DatabaseHealthCheck has same executor+shutdownNow() race condition as pre-#34490 telemetry and uses DbConnectionFactory.getConnection() anti-pattern

Open
#34,839 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

dotCMS : Technical Debt Team : Enablement Type : Defect
Dominant language
Java
Stars
970
Forks
486
Avg merge
3d 33m
Merged PRs (30d)
170

Description

Problem Statement

DatabaseHealthCheck.performCheck() contains two structural problems that are direct parallels of issues already fixed or tracked elsewhere in the connection management epic.

Problem 1 — Same race condition as pre-#34490 telemetry (lines 67–94)

The health check uses the identical executor + shutdownNow() pattern that #34490 fixed for MetricStatsCollector:

ExecutorService executor = Executors.newSingleThreadExecutor();
Future<String> future = executor.submit(() -> {
    try (Connection conn = DbConnectionFactory.getConnection()) {  // ThreadLocal
        try (var stmt = conn.prepareStatement("SELECT 1")) { ... }
    }
});
try {
    return future.get(2, TimeUnit.SECONDS);
} catch (TimeoutException e) {
    future.cancel(true);         // interrupt to executor thread
} finally {
    executor.shutdownNow();      // same pattern as pre-fix MetricStatsCollector
}

try-with-resources is not interrupt-resilient. If shutdownNow() interrupts the executor thread while conn.close() is executing, the close is abandoned and the connection is orphaned — the same race condition that wrapConnection()'s Thread.interrupted() / restore pattern in DbConnectionFactory was specifically designed to prevent. The health check never received the equivalent fix.

Problem 2 — DbConnectionFactory.getConnection() anti-pattern (#34489)

DbConnectionFactory.getConnection() is a ThreadLocal-first lookup. For an explicit connectivity test, this is wrong:

  1. If the executor thread happens to have a connection in its ThreadLocal (possible with thread reuse), the health check validates the cached connection rather than testing a fresh network path to RDS — it could report "healthy" while connectivity is broken.
  2. After try-with-resources closes the connection, the ThreadLocal on the executor thread still holds the reference. If newSingleThreadExecutor() reuses that thread on the next probe cycle, DbConnectionFactory.connectionExists() returns true on a stale reference.

The correct source for a health check is DbConnectionFactory.getDataSource().getConnection() — direct from the pool, bypassing ThreadLocal entirely.

Problem 3 — PreparedStatement for SELECT 1 instead of Connection.isValid()

conn.prepareStatement("SELECT 1") allocates a PreparedStatement and parses a query. conn.isValid(timeoutSeconds) is the JDBC4 standard for connection health validation — pgjdbc implements it without a PreparedStatement, and it is semantically correct for this use case. The health check should use conn.isValid(2) rather than a manual SQL round-trip.


Impact

Each k8s readiness probe fires every 7 seconds per pod. With 177 StatefulSets on k8s-us-prod-1, this is ~25 probe invocations/second fleet-wide. Under the race condition (2-second timeout + shutdownNow()), a slow RDS response could orphan a connection on every probe — adding a high-frequency source of connection leaks on top of the background thread issues already tracked in this epic.


Files

  • dotCMS/src/main/java/com/dotcms/health/checks/cdi/DatabaseHealthCheck.java — lines 67–94

Acceptance Criteria

  • DbConnectionFactory.getConnection() replaced with DbConnectionFactory.getDataSource().getConnection() — bypasses ThreadLocal, always tests a real pool connection
  • try-with-resources on the connection replaced with wrapConnection() pattern (or explicit interrupt-resilient finally block matching DbConnectionFactory.wrapConnection()) so that future.cancel(true) / shutdownNow() cannot orphan the connection
  • conn.prepareStatement("SELECT 1") replaced with conn.isValid(2) — JDBC4 standard, no PreparedStatement allocation
  • executor.shutdownNow() in finally replaced with graceful executor.shutdown() + awaitTermination() + fallback shutdownNow(), consistent with the pattern in MetricStatsCollector after #34490
  • When DatabaseHealthEventManager is in event-driven mode and its cached result is fresher than the probe interval, the health check returns the cached status directly without opening a connection
  • Existing health check behaviour (2-second timeout, DOWN on failure, excluded from liveness probes) is preserved

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with dotCMS/src/main/java/com/dotcms/health/checks/cdi/DatabaseHealthCheck.java lines 67–94, then compare the shutdown and connection-handling patterns in MetricStatsCollector after #34490 and DbConnectionFactory.wrapConnection(). Check how DatabaseHealthEventManager exposes cached results. Done means the health check uses a fresh pooled connection, interrupt-safe cleanup, isValid(2), graceful executor shutdown, cached results when applicable, and preserves existing failure behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.