fix(db): open the pub/sub LISTEN connection outside the Hikari pool (#36934)

dotCMS/src/main/java/com/dotcms/dotpubsub/JDBCPubSubImpl.java

@@ -9,14 +9,18 @@
 import com.dotmarketing.util.StringUtils;
 import com.dotmarketing.util.UtilMethods;
 import com.google.common.annotations.VisibleForTesting;
+import com.zaxxer.hikari.HikariDataSource;
 import io.vavr.Lazy;
 import io.vavr.control.Try;
 import org.postgresql.PGConnection;
 import org.postgresql.PGNotification;
 
+import javax.sql.DataSource;
 import javax.validation.constraints.NotNull;
 import java.sql.Connection;
+import java.sql.DriverManager;
 import java.sql.PreparedStatement;
+import java.sql.SQLException;
 import java.sql.Statement;
 import java.util.Map;
 import java.util.Set;
@@ -26,7 +30,8 @@
 
 /**
  * Provides notifications for the postgres pub/sub connection.
- * The secret sauce is that it borrows 1 DB connection and keeps it open forever.
+ * The secret sauce is that it opens 1 dedicated DB connection — outside the Hikari request pool,
+ * see {@code PGListener.listenerConnection()} — and keeps it open forever.
  * With this one long term connection, you "listen" to the topics on
  * postgres and continually do a "SELECT 1" in that connection to be notified
  * of any new messages to those topics.
@@ -118,7 +123,7 @@ class PGListener extends Thread {
 
         private RUNSTATE runstate = RUNSTATE.STARTED;
         private final Set<String> topics = ConcurrentHashMap.newKeySet();
-        private final Lazy<Connection> connection = Lazy.of(() -> Try.of(() -> DbConnectionFactory.getDataSource().getConnection()).getOrElseThrow(DotRuntimeException::new));
+        private final Lazy<Connection> connection = Lazy.of(() -> Try.of(PGListener::listenerConnection).getOrElseThrow(DotRuntimeException::new));
         private final Lazy<PGConnection> pgConnection = Lazy.of(() -> Try.of(() -> connection.get().unwrap(PGConnection.class)).getOrElseThrow(DotRuntimeException::new));
         private final Pattern validTopicRegEx = Pattern.compile("[a-z0-9_]");
 
@@ -128,6 +133,52 @@ class PGListener extends Thread {
             pgConnection.get();
         }
 
+        /**
+         * Opens the connection this listener holds open for the lifetime of the thread.
+         *
+         * <p>It is built from the pool's own JDBC coordinates rather than borrowed from the pool.
+         * A Postgres {@code LISTEN} needs a connection that is never returned, which is the one
+         * thing a request pool must not hand out: the slot is withdrawn for the lifetime of the
+         * JVM without ever showing up as in-use work, and HikariCP reports the hold as
+         * {@code Apparent connection leak detected} — a stack trace on every boot that looks like
+         * a bug and is not one (issue #36934).</p>
+         *
+         * <p>Falls back to a pooled connection when the datasource does not expose a JDBC URL — a
+         * JNDI-provided or otherwise wrapped datasource — because a listener that works while
+         * logging a spurious warning beats no listener at all.</p>
+         *
+         * @return a dedicated connection when possible, a pooled one otherwise
+         * @throws SQLException if the connection cannot be opened
+         */
+        private static Connection listenerConnection() throws SQLException {
+            return listenerConnection(DbConnectionFactory.getDataSource());
+        }
+
+        /**
+         * {@link #listenerConnection()} against an explicit datasource, so the choice between a
+         * dedicated and a pooled connection can be exercised without a running pool.
+         *
+         * @param dataSource the datasource to derive the connection from
+         * @return a dedicated connection when possible, a pooled one otherwise
+         * @throws SQLException if the connection cannot be opened
+         */
+        @VisibleForTesting
+        static Connection listenerConnection(final DataSource dataSource) throws SQLException {
+            if (dataSource instanceof HikariDataSource hikari
+                    && UtilMethods.isSet(hikari.getJdbcUrl())) {
+                Logger.info(JDBCPubSubImpl.class, () -> "Opening a dedicated connection for the"
+                        + " Postgres pub/sub listener, outside the Hikari pool.");
+                return DriverManager.getConnection(hikari.getJdbcUrl(), hikari.getUsername(),
+                        hikari.getPassword());
+            }
+
+            Logger.warn(JDBCPubSubImpl.class, "The datasource exposes no JDBC URL, so the Postgres"
+                    + " pub/sub listener has to borrow a pooled connection and hold it open. The"
+                    + " pool loses that connection for the lifetime of the JVM and HikariCP will"
+                    + " report it as an apparent connection leak.");
+            return dataSource.getConnection();
+        }
+
 
 
         private long failures = 0;

dotCMS/src/test/java/com/dotcms/dotpubsub/JDBCPubSubListenerConnectionTest.java

@@ -0,0 +1,71 @@
+package com.dotcms.dotpubsub;
+
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThrows;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import com.zaxxer.hikari.HikariDataSource;
+import java.sql.Connection;
+import java.sql.SQLException;
+import javax.sql.DataSource;
+import org.junit.Test;
+
+/**
+ * Unit tests for where the Postgres pub/sub listener gets its connection from (issue #36934).
+ *
+ * <p>The listener holds its connection open for the lifetime of the JVM, which is exactly what a
+ * request pool must not give away: the slot never comes back and HikariCP reports the hold as an
+ * apparent connection leak on every boot. These tests pin the selection — dedicated when the
+ * datasource exposes JDBC coordinates, pooled only as a fallback — without needing a live pool.</p>
+ *
+ * @author Fabrizzio Araya
+ */
+public class JDBCPubSubListenerConnectionTest {
+
+    /**
+     * Given : a datasource that exposes no JDBC URL — a JNDI-provided or otherwise wrapped one.
+     * When  : the listener asks for its connection.
+     * Then  : it falls back to the pool, because a listener that works while logging a spurious
+     *         warning beats no listener at all.
+     */
+    @Test
+    public void datasourceWithoutJdbcCoordinates_fallsBackToThePool() throws SQLException {
+
+        final DataSource pool = mock(DataSource.class);
+        final Connection pooled = mock(Connection.class);
+        when(pool.getConnection()).thenReturn(pooled);
+
+        assertSame("Without JDBC coordinates the pool is the only option left",
+                pooled, JDBCPubSubImpl.PGListener.listenerConnection(pool));
+        verify(pool).getConnection();
+    }
+
+    /**
+     * Given : a Hikari datasource that does expose a JDBC URL.
+     * When  : the listener asks for its connection.
+     * Then  : the pool is never asked for one — the connection is opened directly, so the pool
+     *         keeps its full capacity and the leak detector has nothing to report.
+     *
+     * <p>The URL points at a closed port, so opening it fails; the assertion that matters is not
+     * the failure but that it was attempted <em>instead of</em> borrowing from the pool.</p>
+     */
+    @Test
+    public void hikariWithJdbcCoordinates_neverBorrowsFromThePool() throws SQLException {
+
+        try (final HikariDataSource hikari = spy(new HikariDataSource())) {
+            hikari.setJdbcUrl("jdbc:postgresql://127.0.0.1:1/dotcms-does-not-exist");
+            hikari.setUsername("dotcmsdbuser");
+            hikari.setPassword("unused");
+
+            assertThrows("A closed port must surface as a SQLException from the direct connection",
+                    SQLException.class,
+                    () -> JDBCPubSubImpl.PGListener.listenerConnection(hikari));
+
+            verify(hikari, never()).getConnection();
+        }
+    }
+}