refactor(reporting): harden the dedicated reporting pool (PR #304 review)

Addresses SScholefield + Rylan-cgi:

- Fast-fail: the reporting pool gets its own short connection-timeout
  (SPRING_DATASOURCE_REPORTING_CONNECTION_TIMEOUT, default 5000ms) so the
  Nth queued render fails quickly instead of parking a Tomcat thread 30s.
- DRY: both pools now share applyCommonHikari(...) for the URL/credentials/
  driver/lifetimes/validation, so a shared setting can't drift between them;
  reportingHikariConfig layers only the reporting overrides.
- Observability: bind both pools to the MeterRegistry (setMetricRegistry via
  ObjectProvider) so hikaricp_connections_active/pending are exposed — the
  hand-built + DelegatingDataSource-wrapped pools aren't picked up by Boot's
  binder otherwise.
- read-only wording softened everywhere (javadoc / yml / IT): setReadOnly is a
  Hikari HINT, not Oracle-enforced write-prevention.
- Isolation IT strengthened into a real regression guard: reporting max is set
  ABOVE the primary (6 > 5) via @TestPropertySource, so saturating it is only
  possible with a separate pool; asserts the primary pool's active connections
  stay 0 and it remains acquirable. Dropped the coin-flip connection.isReadOnly()
  assertion (asserts the pool config instead). Best-effort connection close.

Verified: unit test + checkstyle green locally; ReportingDataSourceIsolationIT
green in WSL Oracle Testcontainers (Tests run: 2, 0 failures).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: gpascucci

backend/src/main/java/ca/bc/gov/nrs/ilcr/configuration/DataSourceConfiguration.java

@@ -2,7 +2,9 @@
 
 import com.zaxxer.hikari.HikariConfig;
 import com.zaxxer.hikari.HikariDataSource;
+import io.micrometer.core.instrument.MeterRegistry;
 import javax.sql.DataSource;
+import org.springframework.beans.factory.ObjectProvider;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
 import org.springframework.context.annotation.Bean;
@@ -35,36 +37,37 @@ public DataSource dataSource(
             @Value("${spring.datasource.hikari.max-lifetime:180000}") long maxLifetime,
             @Value("${spring.datasource.hikari.keepalive-time:60000}") long keepaliveTime,
             @Value("${spring.datasource.hikari.leak-detection-threshold:60000}") long leakDetectionThreshold,
-            @Value("${ilcr.datasource.validation-query:SELECT 1 FROM DUAL}") String validationQuery
+            @Value("${ilcr.datasource.validation-query:SELECT 1 FROM DUAL}") String validationQuery,
+            ObjectProvider<MeterRegistry> meterRegistry
     ) {
         requireProperty("spring.datasource.url", url);
         requireProperty("spring.datasource.username", username);
         requireProperty("spring.datasource.password", password);
 
         HikariConfig config = new HikariConfig();
-        config.setJdbcUrl(url);
-        config.setUsername(username);
-        config.setPassword(password);
-        config.setDriverClassName(driverClassName);
+        applyCommonHikari(config, url, username, password, driverClassName, idleTimeout, maxLifetime,
+                keepaliveTime, validationQuery);
         config.setPoolName(poolName);
         config.setMaximumPoolSize(maximumPoolSize);
         config.setMinimumIdle(minimumIdle);
         config.setConnectionTimeout(connectionTimeout);
-        config.setIdleTimeout(idleTimeout);
-        config.setMaxLifetime(maxLifetime);
-        config.setKeepaliveTime(keepaliveTime);
         config.setLeakDetectionThreshold(leakDetectionThreshold);
-        config.setConnectionTestQuery(validationQuery);
+        // Expose hikaricp_connections_* so pool saturation (this story's whole concern) is alertable,
+        // not inferred from latency. The hand-built pool + DelegatingDataSource wrapper means Boot's
+        // Hikari metrics binder doesn't pick it up, so bind it here.
+        meterRegistry.ifAvailable(config::setMetricRegistry);
 
         return new ValidatingDataSource(new HikariDataSource(config), validationQuery);
     }
 
     /**
-     * A SEPARATE, read-only, small Hikari pool dedicated to the Jasper report-fill path (Story 29.1).
-     * The Schedule 9 SQL-in-template fill borrows a JDBC connection for the ENTIRE render (embedded SQL
-     * + PDF formatting) — the longest-held connection in the app. Drawing it from the {@link Primary}
+     * A SEPARATE, small Hikari pool dedicated to the Jasper report-fill path (Story 29.1). The Schedule 9
+     * SQL-in-template fill borrows a JDBC connection for the ENTIRE render (embedded SQL + PDF
+     * formatting) — the longest-held connection in the app. Drawing it from the {@link Primary}
      * transactional pool (default max 5) means a handful of concurrent reports can starve ordinary
-     * schedule requests. This bean isolates report fills so they can only exhaust their own small pool.
+     * schedule requests. This bean isolates report fills so they can only exhaust their own small pool,
+     * with a short connection-timeout so the Nth queued render fast-fails instead of parking a Tomcat
+     * thread for the full 30s.
      *
      * <p>Deliberately NOT {@link Primary}: the {@code jdbcTemplate}/{@code namedParameterJdbcTemplate}/
      * {@code transactionManager} beans keep binding to the transactional pool. Only {@code ReportService}
@@ -79,11 +82,12 @@ public DataSource reportingDataSource(
             @Value("${spring.datasource.reporting.hikari.pool-name:ILCRReportingPool}") String poolName,
             @Value("${spring.datasource.reporting.hikari.maximum-pool-size:3}") int maximumPoolSize,
             @Value("${spring.datasource.reporting.hikari.minimum-idle:0}") int minimumIdle,
-            @Value("${spring.datasource.hikari.connection-timeout:30000}") long connectionTimeout,
+            @Value("${spring.datasource.reporting.hikari.connection-timeout:5000}") long connectionTimeout,
             @Value("${spring.datasource.hikari.idle-timeout:60000}") long idleTimeout,
             @Value("${spring.datasource.hikari.max-lifetime:180000}") long maxLifetime,
             @Value("${spring.datasource.hikari.keepalive-time:60000}") long keepaliveTime,
-            @Value("${ilcr.datasource.validation-query:SELECT 1 FROM DUAL}") String validationQuery
+            @Value("${ilcr.datasource.validation-query:SELECT 1 FROM DUAL}") String validationQuery,
+            ObjectProvider<MeterRegistry> meterRegistry
     ) {
         requireProperty("spring.datasource.url", url);
         requireProperty("spring.datasource.username", username);
@@ -92,37 +96,53 @@ public DataSource reportingDataSource(
         HikariConfig config = reportingHikariConfig(url, username, password, driverClassName, poolName,
                 maximumPoolSize, minimumIdle, connectionTimeout, idleTimeout, maxLifetime, keepaliveTime,
                 validationQuery);
+        meterRegistry.ifAvailable(config::setMetricRegistry);
         return new ValidatingDataSource(new HikariDataSource(config), validationQuery);
     }
 
     /**
-     * Build the read-only reporting {@link HikariConfig}. Extracted (package-private) so its
-     * isolation-critical invariants — read-only, its own pool name, a small dedicated ceiling, and NO
-     * leak-detection (a render legitimately holds its connection for the whole fill) — are unit-testable
-     * without opening a real database connection.
+     * Build the reporting {@link HikariConfig}: the connection settings shared with the primary pool
+     * (via {@link #applyCommonHikari}) plus the reporting-specific overrides — its own pool name + small
+     * ceiling, a short connection-timeout (fast-fail rather than parking a Tomcat thread on the Nth
+     * queued render), NO leak-detection (a render legitimately holds its connection for the whole fill),
+     * and the read-only HINT. Extracted (package-private) so those invariants are unit-testable without
+     * opening a database connection.
+     *
+     * <p>NOTE: {@code setReadOnly(true)} is a Hikari/JDBC <em>hint</em>, not an enforced privilege — on
+     * Oracle it does not by itself prevent writes. It is intent-signalling + defence-in-depth only; true
+     * write-prevention would be a read-only database account for the reporting connection.
      */
     static HikariConfig reportingHikariConfig(
             String url, String username, String password, String driverClassName, String poolName,
             int maximumPoolSize, int minimumIdle, long connectionTimeout, long idleTimeout,
             long maxLifetime, long keepaliveTime, String validationQuery) {
         HikariConfig config = new HikariConfig();
-        config.setJdbcUrl(url);
-        config.setUsername(username);
-        config.setPassword(password);
-        config.setDriverClassName(driverClassName);
+        applyCommonHikari(config, url, username, password, driverClassName, idleTimeout, maxLifetime,
+                keepaliveTime, validationQuery);
         config.setPoolName(poolName);
         config.setMaximumPoolSize(maximumPoolSize);
         config.setMinimumIdle(minimumIdle);
         config.setConnectionTimeout(connectionTimeout);
+        config.setReadOnly(true);
+        return config;
+    }
+
+    /**
+     * The connection settings both pools share (URL / credentials / driver / lifetimes / validation),
+     * factored out so a change to one pool's shared setting can't silently drift from the other.
+     * Pool-specific settings (name, sizes, timeout, leak-detection, read-only) are applied by the caller.
+     */
+    private static void applyCommonHikari(HikariConfig config, String url, String username,
+            String password, String driverClassName, long idleTimeout, long maxLifetime,
+            long keepaliveTime, String validationQuery) {
+        config.setJdbcUrl(url);
+        config.setUsername(username);
+        config.setPassword(password);
+        config.setDriverClassName(driverClassName);
         config.setIdleTimeout(idleTimeout);
         config.setMaxLifetime(maxLifetime);
         config.setKeepaliveTime(keepaliveTime);
         config.setConnectionTestQuery(validationQuery);
-        // Read-only: report fills never write. Leak-detection intentionally left unset (0/off): a Jasper
-        // render holds its connection for the full fill+format, which would trip a leak warning; the
-        // dedicated small pool bounds the exposure instead.
-        config.setReadOnly(true);
-        return config;
     }
 
     @Bean

backend/src/main/java/ca/bc/gov/nrs/ilcr/reporting/ReportService.java

@@ -75,9 +75,10 @@ public class ReportService {
   private final Map<ScheduleKey, JasperReport> compiledTemplates = new ConcurrentHashMap<>();
 
   /**
-   * @param dataSource the dedicated read-only reporting datasource (Story 29.1) the Schedule 9 fill
-   *     borrows from — isolated from the {@code @Primary} transactional pool so a burst of report
-   *     renders cannot starve ordinary schedule requests
+   * @param dataSource the dedicated reporting datasource (Story 29.1) the Schedule 9 fill borrows from
+   *     — its own small pool, isolated from the {@code @Primary} transactional pool so a burst of report
+   *     renders cannot starve ordinary schedule requests (its connections are read-only as a hint, not
+   *     an enforced privilege)
    * @param schedule5Service the Schedule 5 read (bean-datasource feed)
    * @param schedule6Service the Schedule 6 read (bean-datasource feed)
    * @param schedule7aService the Schedule 7A read (bean-datasource feed)

backend/src/main/resources/application.yml

@@ -25,17 +25,20 @@ spring:
       max-lifetime: ${SPRING_DATASOURCE_MAX_LIFETIME:180000}
       keepalive-time: ${SPRING_DATASOURCE_KEEPALIVE_TIME:60000}
       leak-detection-threshold: ${SPRING_DATASOURCE_LEAK_DETECTION_THRESHOLD:60000}
-    # Story 29.1 — a SEPARATE, read-only, small pool dedicated to the Jasper report-fill path
-    # (the Schedule 9 SQL-in-template fill borrows a JDBC connection for the whole render). It shares
-    # the same DB (url/username/password above) but its own small ceiling, so a burst of concurrent
-    # report renders can only exhaust THIS pool — never the transactional pool that serves ordinary
-    # schedule requests. Size it to peak concurrent report renders (default 3). No leak-detection: a
-    # render legitimately holds its connection for the full fill+format.
+    # Story 29.1 — a SEPARATE, small pool dedicated to the Jasper report-fill path (the Schedule 9
+    # SQL-in-template fill borrows a JDBC connection for the whole render). Same DB (url/username/
+    # password above) but its own small ceiling, so a burst of concurrent report renders can only
+    # exhaust THIS pool — never the transactional pool that serves ordinary schedule requests. Size it
+    # to peak concurrent report renders (default 3). Short connection-timeout (5s) so the Nth queued
+    # render fast-fails instead of parking a Tomcat thread for 30s. No leak-detection: a render
+    # legitimately holds its connection for the full fill+format. Connections are marked read-only as a
+    # HINT / intent signal (defence-in-depth) — it is not enforced write-prevention on Oracle.
     reporting:
       hikari:
         pool-name: ${SPRING_DATASOURCE_REPORTING_POOL_NAME:ILCRReportingPool}
         minimum-idle: ${SPRING_DATASOURCE_REPORTING_MIN_IDLE:0}
         maximum-pool-size: ${SPRING_DATASOURCE_REPORTING_MAX_POOL_SIZE:3}
+        connection-timeout: ${SPRING_DATASOURCE_REPORTING_CONNECTION_TIMEOUT:5000}
 
 server:
   port: ${SERVER_PORT:8080}

backend/src/test/java/ca/bc/gov/nrs/ilcr/configuration/DataSourceConfigurationTest.java

@@ -14,7 +14,7 @@
 class DataSourceConfigurationTest {
 
   @Test
-  void reportingHikariConfigIsReadOnlyWithItsOwnSmallPoolAndNoLeakDetection() {
+  void reportingHikariConfigIsReadOnlyWithItsOwnSmallFastFailingPoolAndNoLeakDetection() {
     HikariConfig config = DataSourceConfiguration.reportingHikariConfig(
         "jdbc:oracle:thin:@//db.example:1521/ILCR",
         "THE",
@@ -23,22 +23,24 @@ void reportingHikariConfigIsReadOnlyWithItsOwnSmallPoolAndNoLeakDetection() {
         "ILCRReportingPool",
         3,
         0,
-        30000L,
+        5000L,
         60000L,
         180000L,
         60000L,
         "SELECT 1 FROM DUAL");
 
-    // Report fills never write.
+    // Read-only HINT (defence-in-depth; not Oracle-enforced write-prevention).
     assertThat(config.isReadOnly()).isTrue();
     // A distinct, small pool of its own — not the @Primary transactional pool.
     assertThat(config.getPoolName()).isEqualTo("ILCRReportingPool");
     assertThat(config.getMaximumPoolSize()).isEqualTo(3);
     assertThat(config.getMinimumIdle()).isEqualTo(0);
+    // Short connection-timeout: the Nth queued render fast-fails rather than parking a Tomcat thread 30s.
+    assertThat(config.getConnectionTimeout()).isEqualTo(5000L);
     // Deliberately unset (0 = disabled): a render holds its connection for the full fill+format, which
     // would otherwise trip a leak warning; the small dedicated pool bounds the exposure instead.
     assertThat(config.getLeakDetectionThreshold()).isZero();
-    // Credentials/URL/validation still wired through.
+    // Credentials/URL/validation still wired through the shared builder.
     assertThat(config.getJdbcUrl()).isEqualTo("jdbc:oracle:thin:@//db.example:1521/ILCR");
     assertThat(config.getConnectionTestQuery()).isEqualTo("SELECT 1 FROM DUAL");
   }

backend/src/test/java/ca/bc/gov/nrs/ilcr/reporting/ReportingDataSourceIsolationIT.java

@@ -5,6 +5,7 @@
 import ca.bc.gov.nrs.ilcr.support.AbstractOracleIT;
 import com.zaxxer.hikari.HikariDataSource;
 import java.sql.Connection;
+import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.List;
 import javax.sql.DataSource;
@@ -14,11 +15,19 @@
 import org.springframework.test.context.TestPropertySource;
 
 /**
- * Story 29.1: proves the Jasper report-fill path draws from a SEPARATE, read-only Hikari pool, so a
- * burst of report renders can only exhaust that dedicated pool and never starve the {@code @Primary}
+ * Story 29.1: proves the Jasper report-fill path draws from a SEPARATE Hikari pool, so a burst of
+ * report renders can only exhaust that dedicated pool and never starve the {@code @Primary}
  * transactional pool serving ordinary schedule requests.
+ *
+ * <p>The reporting pool is sized ABOVE the primary pool here (6 &gt; the default 5) so the isolation
+ * test is a real regression guard: if someone dropped the {@code @Qualifier} on {@code ReportService}
+ * and it fell back to the shared primary pool of 5, saturating "the reporting pool" with 6 connections
+ * would be impossible (the 6th would block to the timeout and fail) — so the test would go red.
  */
-@TestPropertySource(properties = "ilcr.security.enabled=false")
+@TestPropertySource(properties = {
+    "ilcr.security.enabled=false",
+    "spring.datasource.reporting.hikari.maximum-pool-size=6"
+})
 class ReportingDataSourceIsolationIT extends AbstractOracleIT {
 
   @Autowired private DataSource primaryDataSource; // resolves to the @Primary bean
@@ -28,37 +37,50 @@ class ReportingDataSourceIsolationIT extends AbstractOracleIT {
   private DataSource reportingDataSource;
 
   @Test
-  void reportingDataSourceIsADistinctReadOnlyPool() throws Exception {
+  void reportingDataSourceIsADistinctPoolWithItsOwnName() throws Exception {
     assertThat(reportingDataSource).isNotSameAs(primaryDataSource);
 
     HikariDataSource reportingHikari = reportingDataSource.unwrap(HikariDataSource.class);
     assertThat(reportingHikari.getPoolName()).isEqualTo("ILCRReportingPool");
+    // The read-only flag is a config/pool HINT (not an Oracle-enforced privilege), so assert the pool
+    // config rather than connection.isReadOnly() — the driver may report that inconsistently.
     assertThat(reportingHikari.isReadOnly()).isTrue();
-
-    try (Connection connection = reportingDataSource.getConnection()) {
-      assertThat(connection.isReadOnly()).isTrue();
-    }
+    assertThat(reportingHikari.getMaximumPoolSize()).isGreaterThan(primaryMaxPoolSize());
   }
 
   @Test
-  void saturatingTheReportingPoolLeavesTheTransactionalPoolAcquirable() throws Exception {
-    int reportingMax = reportingDataSource.unwrap(HikariDataSource.class).getMaximumPoolSize();
+  void saturatingTheReportingPoolLeavesTheTransactionalPoolUntouched() throws Exception {
+    HikariDataSource reportingHikari = reportingDataSource.unwrap(HikariDataSource.class);
+    HikariDataSource primaryHikari = primaryDataSource.unwrap(HikariDataSource.class);
+    int reportingMax = reportingHikari.getMaximumPoolSize();
 
     List<Connection> held = new ArrayList<>();
     try {
-      // Hold every connection the reporting pool can hand out — the stand-in for concurrent renders,
-      // each of which pins a connection for its whole fill.
+      // Hold EVERY connection the reporting pool can hand out (the stand-in for concurrent renders, each
+      // pinning a connection for its whole fill). Because reportingMax (6) > the primary ceiling (5),
+      // completing this loop is itself proof of a separate pool — the shared pool could never hand out 6.
       for (int i = 0; i < reportingMax; i++) {
         held.add(reportingDataSource.getConnection());
       }
-      // The transactional pool is untouched: an ordinary request still gets a connection right away.
+
+      // The transactional pool is provably untouched by the reporting saturation...
+      assertThat(primaryHikari.getHikariPoolMXBean().getActiveConnections()).isZero();
+      // ...and an ordinary request still gets a connection right away.
       try (Connection primary = primaryDataSource.getConnection()) {
         assertThat(primary.isValid(2)).isTrue();
       }
     } finally {
       for (Connection connection : held) {
-        connection.close();
+        try {
+          connection.close();
+        } catch (SQLException ignored) {
+          // Best-effort cleanup — keep closing the rest so a single failure can't leak sessions.
+        }
       }
     }
   }
+
+  private int primaryMaxPoolSize() throws SQLException {
+    return primaryDataSource.unwrap(HikariDataSource.class).getMaximumPoolSize();
+  }
 }