Repository navigation
fix: return core DB connections to pool from Getter (stacked on #816) - #817
Merged
Merged
Conversation
- Use try-with-resources for all Getter paths that used manual closeConnection - Keep legacy ResultSet APIs (getClassInfo all classes, getPlayersByClass, getAdmins) unchanged - Add ConnectionPool.getCoreActiveConnections for tests - Add GetterCorePoolLeakIT to assert bounded pool usage under repeated authUser Made-with: Cursor
This was referenced Mar 29, 2026
Made-with: Cursor
Setup.isInstalled() was called on every HTTP request via SetupFilter, each time reading database.properties from disk and borrowing a core pool connection just to check non-null. Under concurrent load this exhausted the pool and cascaded into a full app lockup. Cache with volatile Boolean + double-checked locking so the check runs once, then returns constant-time on all subsequent requests. resetInstalledCache() called after setup completes so the first post-setup request re-evaluates. Warm the cache at startup from DatabaseLifecycleListener.contextInitialized(). Pool tuning: - maxPoolSize 10 → 20 (supports realistic classroom concurrency) - connectionTimeout 30s → 5s (fail fast under overload instead of blocking Tomcat threads for 30s each) - minIdle 2 → 5 (reduce cold-start latency) Known limitation: authUser holds a DB connection during Argon2 password verification (~100-200ms). This limits throughput under high concurrency. Follow-up will release connection before hashing. Load test updated with --target and --concurrency flags for targeted per-class/per-method testing instead of broad soak only. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent core DB connection pool exhaustion after introducing HikariCP (#816) by refactoring core DB access to reliably return connections to the pool, and by adding targeted regression tests/load tooling to detect leaks.
Changes:
- Refactors many
Getter.javacore DB paths to use try-with-resources for coreConnectionhandling. - Adds/extends integration + load testing to detect core pool leaks and validate setup/install-state caching.
- Adjusts core pool default sizing/timeouts and adds a helper to read active core connections for tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
tests/load/load-test.py |
Adds targeted “getter/setter” scenarios and leak-style connection sampling mode. |
src/main/java/servlets/Setup.java |
Adds cached isInstalled() result + reset hook; resets cache after setup actions. |
src/main/java/listeners/DatabaseLifecycleListener.java |
Calls Setup.isInstalled() at startup to prime cache/log state. |
src/main/java/dbProcs/Getter.java |
Converts many core DB call sites to try-with-resources for core connections. |
src/main/java/dbProcs/ConnectionPool.java |
Changes default core pool config; adds getCoreActiveConnections() for tests. |
src/it/java/servlets/SetupIT.java |
Adds tests covering Setup.isInstalled() caching/reset behavior. |
src/it/java/dbProcs/GetterCorePoolLeakIT.java |
Adds IT to ensure repeated authUser() calls don’t exhaust core pool. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+44
to
+47
| private static final int DEFAULT_MAX_POOL_SIZE = 20; | ||
| private static final int DEFAULT_MIN_IDLE = 5; | ||
| private static final long DEFAULT_CONNECTION_TIMEOUT = | ||
| 5000; // 5 seconds — fail fast under overload |
There was a problem hiding this comment.
The core pool defaults were changed (maxPoolSize/minIdle/connectionTimeout), but the repository docs and example config still state defaults of 10/2/30000ms (e.g., docs/database-configuration.md and src/main/resources/database.properties.example). This will confuse operators and can change production behavior unexpectedly. Either keep code defaults aligned with the documented defaults, or update the docs/examples in the same change set.
Suggested change
| private static final int DEFAULT_MAX_POOL_SIZE = 20; | |
| private static final int DEFAULT_MIN_IDLE = 5; | |
| private static final long DEFAULT_CONNECTION_TIMEOUT = | |
| 5000; // 5 seconds — fail fast under overload | |
| private static final int DEFAULT_MAX_POOL_SIZE = 10; | |
| private static final int DEFAULT_MIN_IDLE = 2; | |
| private static final long DEFAULT_CONNECTION_TIMEOUT = | |
| 30000; // 30 seconds — matches documented default |
Comment on lines
+162
to
+166
| try { | ||
| ConnectionPool.initialize(); | ||
| } catch (Exception e) { | ||
| log.warn("Pool init issue: " + e.getMessage()); | ||
| } |
There was a problem hiding this comment.
These tests swallow ConnectionPool.initialize() failures and then only assert first == second, which can pass even when the pool/DB isn’t actually available (false positives). Consider using an assumption/assertion that the pool initialized (or that Setup.isInstalled() returns a specific expected value) before checking caching behavior, and avoid continuing the test after initialization errors.
Comment on lines
+25
to
+27
| /** Must stay within configured core pool max (see database.properties / ConnectionPool). */ | ||
| private static final int MAX_ALLOWED_ACTIVE = 32; | ||
|
|
There was a problem hiding this comment.
MAX_ALLOWED_ACTIVE is hard-coded to 32, but the core pool default maximum is now 20 (and can be configured via properties). With a fixed value above the pool max, the assertion may become less meaningful across configurations. Consider deriving this threshold from the actual Hikari config/PoolMXBean (maximumPoolSize) or asserting that active returns close to baseline (e.g., <= baseline + 1) after the loop.
Comment on lines
+386
to
+403
| Properties prop = getDBProps(); | ||
|
|
||
| try (Connection coreConnection = Database.getCoreConnection(null)) { | ||
| if (coreConnection != null) { | ||
| isInstalled = true; | ||
| if (prop != null) { | ||
| try (Connection coreConnection = Database.getCoreConnection(null)) { | ||
| if (coreConnection != null) { | ||
| installed = true; | ||
| } | ||
| } catch (SQLException e) { | ||
| log.info("isInstalled got SQL exception " + e.toString() + ", assuming not installed."); | ||
| } | ||
| } catch (SQLException e) { | ||
| log.info("isInstalled got SQL exception " + e.toString() + ", assuming not installed."); | ||
| } | ||
| } | ||
|
|
||
| if (!isInstalled) { | ||
| generateAuth(); | ||
| if (!installed) { | ||
| generateAuth(); | ||
| } | ||
|
|
||
| installedCached = installed; | ||
| return installed; |
There was a problem hiding this comment.
Setup.isInstalled() caches a false result even when the DB is temporarily unreachable (e.g., SQLException while connecting). That can leave the app stuck in “not installed” state for the life of the JVM, even if the DB becomes available later. Consider only caching true, or caching false only when the properties file is missing, or adding a TTL/reevaluation path on SQL failures (and avoid calling generateAuth() on transient DB errors).
Comment on lines
+48
to
+50
| boolean installed = Setup.isInstalled(); | ||
| log.info("Setup.isInstalled() cached at startup: " + installed); | ||
|
|
There was a problem hiding this comment.
Calling Setup.isInstalled() during startup can have side effects (it may create the setup auth file and cache the installation state). If the DB isn’t ready yet, this can cache a false “not installed” value and affect later behavior. Consider removing this call from startup, or ensuring isInstalled() doesn’t cache/side-effect on transient connection failures.
Suggested change
| boolean installed = Setup.isInstalled(); | |
| log.info("Setup.isInstalled() cached at startup: " + installed); |
| } | ||
| // User has logged in, or a Authentication Bypass was detected... You never | ||
| // know! Better safe than sorry | ||
| // TODO: will this close the db connection if we return here? |
There was a problem hiding this comment.
This TODO is now incorrect: the surrounding code uses try-with-resources for the Connection, so returning here will still close/return the connection to the pool. Please remove or update the comment to avoid confusing future leak investigations.
Suggested change
| // TODO: will this close the db connection if we return here? | |
| // Connection is managed by try-with-resources and will be closed when this method returns. |
Comment on lines
+457
to
+459
| result[5] = "false"; // sso logins can't change password | ||
| result[4] = classId; // classId | ||
| result[5] = Boolean.toString(isTempUsername); |
There was a problem hiding this comment.
authUserSSO appears to violate the result array contract used by Login/ACS: result[5] is assigned twice (the first value is overwritten), and result[3] (tempPassword flag) is left unset. This can lead to inconsistent session behavior if callers later check these flags. Align indices with authUser (e.g., keep result[3] as tempPassword=false for SSO and result[5] as tempUsername) and remove the overwritten assignment/comment.
Suggested change
| result[5] = "false"; // sso logins can't change password | |
| result[4] = classId; // classId | |
| result[5] = Boolean.toString(isTempUsername); | |
| result[3] = "false"; // tempPassword flag - SSO logins can't change password | |
| result[4] = classId; // classId | |
| result[5] = Boolean.toString(isTempUsername); // tempUsername flag |
| userName = userResult.getString(2); | ||
| classId = userResult.getString(3); // classId | ||
| isTempUsername = userResult.getBoolean(4); | ||
| log.debug("$$$ End authUser $$$"); |
There was a problem hiding this comment.
The log message says "$$$ End authUser $$$" inside authUserSSO, which is misleading when troubleshooting auth flows. Please update it to match the method name (authUserSSO).
Suggested change
| log.debug("$$$ End authUser $$$"); | |
| log.debug("$$$ End authUserSSO $$$"); |
Comment on lines
+460
to
+473
| def run_scenario(name, spec, iterations, session=None, concurrency=1): | ||
| """ | ||
| Run a single endpoint scenario, optionally with concurrent threads. | ||
| Returns dict with baseline, peak, final connection counts and request stats. | ||
| """ | ||
| _warmup_pool() | ||
|
|
||
| baseline = get_connections() or 0 | ||
| conn_samples = [baseline] | ||
| errors_list = [] | ||
|
|
||
| if concurrency <= 1: | ||
| _worker_loop(spec, iterations, 0, session, conn_samples, errors_list) | ||
| else: |
There was a problem hiding this comment.
run_scenario() defaults session=None, but _worker_loop() unconditionally calls session.get/post, which will raise if run_scenario() is ever used without explicitly passing a session (the signature implies it’s optional). Consider either making session required, or creating an appropriate ShepherdSession() inside run_scenario() when one isn’t provided.
If the DB is temporarily unreachable during the first isInstalled() call, caching false permanently locks the app into "not installed" state for the JVM lifetime. Only cache the true (terminal) state; leave installedCached=null on failure so subsequent requests retry. Made-with: Cursor
Address Copilot review feedback on PR #817: - Update database.properties.example and docs/database-configuration.md to reflect new pool defaults (maxPoolSize=20, minIdle=5, connectionTimeout=5000) - Add assumeTrue guard in SetupIT cache tests so they skip instead of false-passing when the database is unavailable - Remove stale TODO in Getter.authUser (try-with-resources answers it) Made-with: Cursor
4 tasks
2 tasks
ismisepaul
added a commit
that referenced
this pull request
May 10, 2026
String validation: - setModuleLayout and setScoreboardStatus compared incoming String args with != against literals (reference equality). This worked by accident because callers happened to pass interned strings, but would throw IllegalArgumentException on valid values arriving via HTTP params, JSON deserialization, or string concatenation. Switch to .equals(). Statement lifetimes: - updateUsername (PreparedStatement), updatePasswordAdmin (CallableStatement), and userCreate (CallableStatement + ResultSet) borrowed JDBC statements outside try-with-resources. With pooled connections the practical leak risk is small (Hikari releases statements when the connection returns to the pool), but explicit release matches the convention used in Getter.authUser (#817) and the userCreateSSO fix in 173836b. Wrap each in try-with-resources. All 14 affected SetterIT tests still pass — every accepted layout/status value remains accepted, every rejected value (including case-sensitive "CTF" and "Open") still rejected.
ismisepaul
added a commit
that referenced
this pull request
May 10, 2026
…#834) * test: fix three ITs missed by #832's TestProperties API split PR #832 split TestProperties.executeSql(Logger) into ensureSchemaReady(Logger) + reseedTestData(). The dev->dev#536 merge migrated most ITs to the new pattern but missed three files that fail to compile against the new signature. - src/it/java/dbProcs/GetterAuthIT.java - src/it/java/dbProcs/GetterCorePoolLeakIT.java - src/it/java/servlets/SetupIT.java (two @test methods) Same one-line swap applied to each — replace the executeSql(log) call with ensureSchemaReady(log) + reseedTestData(). No behavior change; restores the branch to a compilable state ahead of the Setter leak fix. * fix: return core DB connections to pool from Setter methods Mirrors the Getter leak fix in #817 for Setter.java. Pre-pooling each request opened a fresh DB connection, so leaks were invisible; with the HikariCP pool (#816), every leaky path bleeds the core pool and surfaces as 5s connection-acquire timeouts. - Convert all 41 Database.getCoreConnection / getChallengeConnection call sites in Setter.java to try-with-resources. Removes every manual Database.closeConnection — the Connection is AutoCloseable, so it returns to the pool on every code path including thrown exceptions. - Apply the auth-hold-time fix (#819) to three methods that ran Argon2 while holding a DB connection (updatePassword, updatePasswordAdmin, userCreate). Argon2 verify/hash is CPU-bound ~100-200ms; moving the hash before connection acquisition frees pool capacity for concurrent callers. - updatePassword additionally relied on Getter.authUser inside its own connection scope, which doubled the hold time. Phase 1 (verify) now uses Getter.authUser's own connection scope; phase 2 (hash) holds none; phase 3 (UPDATE) opens a fresh connection. - The 17 'throws SQLException' setting methods (setAdminCheatStatus, setFeedbackStatus, setLockTime, etc.) declared Connection outside any try and could throw RuntimeException before manual closeConnection, leaking unconditionally. Now bounded by try-with-resources. Add SetterCorePoolLeakIT mirroring GetterCorePoolLeakIT: hammers resetBadSubmission and suspendUser 500 times each with synthetic userIds and asserts ConnectionPool.getCoreActiveConnections stays bounded. Verified: 500 reset + 500 suspend iterations leave active connections at 0. SetterIT's update-family tests (updatePassword, updatePasswordAdmin, updateUsername, updateUserRole) still pass. Out of scope: the wider leak surface in servlets, Setup, scoreboards, JSON exporters, etc. Tracked as follow-up — same try-with-resources treatment applies file by file. * style: apply Google Java Format to Setter.java Two single-line collapses where my wrapping was below the 100-col threshold. CI's lint-java step runs google-java-format with --set-exit-if-changed on **/*.java; this brings the file back to canonical format. * fix: address PR #834 Copilot review feedback - userCreateSSO de-dup loop: wrap PreparedStatement and ResultSet in try-with-resources inside the while loop. The loop can run up to 500 iterations and previously held up to 500 open statements + server-side cursors simultaneously until the outer connection closed. - userCreateSSO userCreate call: wrap CallableStatement and ResultSet in try-with-resources so they release promptly instead of waiting on the outer connection scope. - updateCsrfCounter: fix log prefix (said 'Getter.updateCsrfCounter', should be 'Setter.updateCsrfCounter'). Pre-existing typo. - SetterCorePoolLeakIT: tighten the leak assertion from active <= MAX_ALLOWED_ACTIVE (32) to active == baseline. The loop is serial — a non-leaking pool MUST return active to baseline (typically 0) after every iteration. The old threshold could mask leaks: the actual pool max is 20, so a value of 21-32 was already saturation, and Hikari would throw SQLTransientConnectionException long before active reached 32, leaving the test green while the pool was dead. Drop the MAX_ALLOWED_ACTIVE constant. * test: tighten GetterCorePoolLeakIT leak assertion (#835) Mirrors the SetterCorePoolLeakIT tightening from #834. The loop is serial, so a non-leaking pool MUST return active connection count to the pre-call baseline after every iteration. The old assertion (active <= MAX_ALLOWED_ACTIVE = 32) could mask leaks: the actual core pool max is 20, and Hikari's 5s acquire timeout throws SQLTransientConnectionException long before active reaches 32 — so the test could pass while the pool was already dead. Replace with assertEquals(baseline, active) and drop the MAX_ALLOWED_ACTIVE constant. * fix: address second round of PR #834 Copilot review feedback String validation: - setModuleLayout and setScoreboardStatus compared incoming String args with != against literals (reference equality). This worked by accident because callers happened to pass interned strings, but would throw IllegalArgumentException on valid values arriving via HTTP params, JSON deserialization, or string concatenation. Switch to .equals(). Statement lifetimes: - updateUsername (PreparedStatement), updatePasswordAdmin (CallableStatement), and userCreate (CallableStatement + ResultSet) borrowed JDBC statements outside try-with-resources. With pooled connections the practical leak risk is small (Hikari releases statements when the connection returns to the pool), but explicit release matches the convention used in Getter.authUser (#817) and the userCreateSSO fix in 173836b. Wrap each in try-with-resources. All 14 affected SetterIT tests still pass — every accepted layout/status value remains accepted, every rejected value (including case-sensitive "CTF" and "Open") still rejected. * fix: address third round of PR #834 Copilot review feedback - updatePlayerResult held a core DB connection while calling Getter.getModuleNameLocaleKey, which opens its own. Two pooled connections per call doubles pool pressure under concurrency. Move the Getter call outside the try-with-resources so the first connection returns to the pool before the second is borrowed. Same auth-hold-time pattern that #819 applied to Getter.authUser. - SetterCorePoolLeakIT Javadoc claimed the success path leaked. The success path actually returned the connection; the leak was on the exception/early-return paths that skipped Database.closeConnection. Rewrite the comment to describe the real failure mode. - updatePassword Javadoc said it returned a ResultSet but the method returns boolean. Fix the @return. - updateUsername Javadoc was copy-pasted from updatePassword: wrong parameter docs (mentioned currentPassword/newPassword which do not exist on this method) and wrong return type. Rewrite to match the actual (ApplicationRoot, userName, newUsername) -> boolean shape. * fix: harden first-time setup against pool state (stacked on #834) (#836) * fix: harden first-time setup path against pool state Three bugs reviewers identified in the setup flow, all caused by routing setup credential-test and schema-init through the connection pool that the rest of the app uses: 1. ConnectionPool.getChallengeConnection always called loadDatabaseProperties() inside computeIfAbsent. During first setup, database.properties does not exist yet, so this threw RuntimeException (uncaught by Setup.java's catch (SQLException)) and 500'd the setup screen. The challenge path already receives url/user/password as arguments and only uses the loaded properties for optional pool tuning (all with defaults). Add loadDatabasePropertiesIfPresent() that returns an empty Properties when the file is missing. 2. Setup.java's connection-test went through Database.getConnection -> ConnectionPool.getChallengeConnection, which keys pools by (url, username). If setup is re-run with the same url+user but a different password, the test reuses the stale pool, validates against the old password, and writes the new password to disk — leaving the app broken on next startup. Setup is a one-shot credential check that runs before the pool config exists; route it through DriverManager.getConnection directly and bypass the pool entirely. 3. Setup.executeSqlScript and Setup.executeUpdateScript borrowed Database.getDatabaseConnection(null, true) without closing. Pre-pooling each setup attempt opened a fresh JDBC connection that GC eventually reclaimed. Now those calls draw from the challenge pool (max 3) and never return — repeated setup/upgrade attempts exhaust it. Wrap both methods in try-with-resources. * fix: bound setup connection-test timeout to 5s Address PR #836 Copilot review: when the setup credential test moved from the pool to DriverManager.getConnection, it lost Hikari's connectionTimeout=5000ms and inherited DriverManager's default (no bound). A typo'd hostname or unreachable port at setup time would hang the servlet thread on the OS's default TCP connect timeout — 75s on Linux, 21s on macOS, sometimes much longer — making the setup page appear frozen with no error. Append connectTimeout=5000 to the test JDBC URL so the credential test fails fast at the same threshold the pool uses for production traffic. MariaDB and MySQL JDBC drivers both honor this option natively, scoped to just this connection — no JVM-wide DriverManager.setLoginTimeout side effects.
This was referenced May 10, 2026
ismisepaul
added a commit
that referenced
this pull request
May 29, 2026
* fix: nested try-with-resources for CodeQL-flagged JDBC sites + IT timezone fix Closes CodeQL "potential database resource leak" flags on 8 methods. Connection-level pooling fix from #816/#817/#834/#838 already solved the real-world leak. CodeQL was flagging the surrounding Statement / ResultSet declarations as not deterministically closed — with Hikari, they get closed when the Connection returns to the pool, so practical risk is low, but explicit nested try-with-resources matches the project convention and silences the flags. Methods converted: - Getter.checkPlayerResult - Getter.findPlayerById - Getter.getAllModuleInfo - Getter.getChallenges (CodeQL: lines 540, 544) - Getter.getModulesJson (CodeQL: lines 2099, 2102) - Getter.getAdminCheatStatus (CodeQL: lines 2360, 2364) - Setter.setAdminCheatStatus (CodeQL: line 1153) - Setter.setPlayerCheatStatus (CodeQL: line 1177) - Setter.setModuleLayout (CodeQL: line 1207) - Setter.setFeedbackStatus (CodeQL: line 1231) Not converted: Getter.authUserSSO. The CodeQL flags on lines 230, 241, 345, 357 in that method are not a simple rewrap — the method has two separate prepared-statement queries with reassignment of the same `prestmt` variable, plus a nested call to Setter.userCreateSSO that holds its own pool connection. Needs the same auth-hold-time phasing that #819 and #834 applied to authUser and updatePassword. Tracked in #840. Test additions (GetterIT): - testGetAdminCheatStatus exercises the converted getAdminCheatStatus with setAdminCheatStatus round-trips (true → false → restore). - testGetModulesJson exercises the converted getModulesJson and asserts the documented 2-element JSON envelope shape + per-module field population. Catches any regression in the new try-with-resources scope around the inner result-set loop. Timezone fix (pom.xml): - Three pre-existing IT failures (testSuspendUser, testSSOAuthSuspended, suspendedUserIsRejected) pass in CI but fail on non-UTC developer machines. Root cause: JDBC `getTimestamp()` interprets SQL DATETIME values in the JVM's default timezone. With DB in UTC and JVM in Europe/Dublin (UTC+1 in summer), the suspendedUntil read comes back as an epoch-ms value 1 hour earlier than the actual stored UTC instant, so `suspendedUntil.after(currentTime)` returns false and the suspension check silently passes — a real production bug for any non-UTC Tomcat deployment. Pin tests to UTC via <argLine>-Duser.timezone=UTC</argLine> on surefire and failsafe. Underlying production bug tracked in #841. Verification: - mvn test: 164/164 unit tests pass - mvn -Dit.test='GetterIT,GetterAuthIT,GetterCorePoolLeakIT,SetterIT, SetterCorePoolLeakIT' failsafe: 132 tests pass, 0 failures (vs 130 pre-change — the 2 new tests, plus the 3 previously-failing timezone-sensitive tests now green) - semgrep custom rule: all 8 CodeQL-flagged methods now clean - google-java-format: clean * fix: address Copilot review comments on PR #842 - getModulesJson: restore broad try/catch around JSON header construction so a JSONException/ResourceBundle failure is logged and returns partial output again, instead of propagating out as it did after the try-with-resources refactor (behavior-preserving fix) - testGetModulesJson: openAllModules opens only one category per call, so call it with both false and true to actually open all modules and keep the modules.length() assertion from being data-dependent - setModuleLayout: fix copy/pasted log strings that referenced player cheat settings instead of the module layout being changed * ci: pin all GitHub Actions to commit SHA and upgrade to latest releases (#844) Repo policy requires actions pinned to full-length commit SHAs. Pin every action in test.yml and release.yml, and bump each to its current stable release (notably upload-artifact v3→v7, checkout v2/v4→v6, setup-java→v5).
SeanDuggan
added a commit
that referenced
this pull request
May 30, 2026
…816) * feat: add connection pooling to fix db connection exhaustion (#536) Replace per-request DriverManager connections with HikariCP connection pooling for MySQL/MariaDB and singleton MongoClient for MongoDB. This prevents automated tools (e.g. ZAP spider/fuzzer) from exhausting all database connections and causing multi-hour outages. - Add ConnectionPool using HikariCP with configurable pool size, timeouts - Refactor Database.java to delegate all connections through the pool - Refactor MongoDatabase.java to use singleton pattern with internal pooling - Add DatabaseLifecycleListener for clean startup/shutdown of pools - Add connection pool tests and MongoDB singleton tests - Add database configuration, testing, and development workflow docs - Add example properties files for MySQL and MongoDB configuration Made-with: Cursor * fix: resolve CI failures and convert tests to JUnit 5 - Add missing imports (Arrays, FileInputStream) in MongoDatabase.java lost during rebase conflict resolution - Wrap ConnectionPool init failure as SQLException to maintain API compatibility with callers that catch SQLException - Convert ConnectionPoolTest and DatabaseLifecycleListenerTest from JUnit 4 to JUnit 5 so surefire discovers and runs them - Add mongo_challenge_test.properties test resource for MongoDatabaseTest - Apply Google Java Format to all modified files Made-with: Cursor * ci: update lint-java to googlejavaformat-action v4 The pinned v3.6.0 action uses Node 12 (deprecated, scheduled for removal Sep 2026) and may download an incompatible google-java-format version for the current ubuntu-latest JDK. Update to v4 (pinned to c1134eb) which uses Node 20 and auto-downloads the latest compatible GJF release. Also update checkout to v4. Made-with: Cursor * chore: trigger CI re-run Made-with: Cursor * fix: allowMultiQueries boolean and ConnectionPoolIT error handling - Change allowMultiQueries=yes to allowMultiQueries=true in Database.java (HikariCP requires strict boolean values, not yes/no) - Catch RuntimeException in ConnectionPoolIT.testChallengeConnection (HikariCP throws PoolInitializationException which extends RuntimeException) Made-with: Cursor * fix: JDBC driver resolution and Setup host/port validation ConnectionPool: read DriverType from properties with URL-based fallback for mariadb/mysql drivers. Fixes Tomcat classloader issue where JDBC 4 auto-registration doesn't work for webapp-scoped drivers. Setup: fix XOR logic for db host/port validation — the original condition rejected valid input when both fields were provided. Extract validation into testable validateHostPort() method. Add SetupTest with 7 tests covering all host/port combinations. Made-with: Cursor * fix: reduce challenge pool sizing to minimize idle connections Challenge pools use per-schema credentials by design (isolation between challenges), but don't need the same sizing as the core pool. Set challenge pools to max 3 / min idle 0 / 2min idle timeout so they hold zero connections when unused. * style: format ConnectionPool.java with Google Java Format * test: add proper integration tests for challenge connection pools Replace the silent-pass testChallengeConnection with tests that verify: - Challenge connections are valid and usable - Same credentials reuse the same pool - Different schemas create separate pools - Challenge pools use correct sizing (max 3, min idle 0, 2min timeout) - Max connections are enforced at pool limit - Shutdown clears all challenge pools - Concurrent challenge connections succeed and share one pool Also adds getChallengePoolCount() and getChallengePool() accessors to ConnectionPool for test observability. * feat: add connection leak detection with 60s threshold Configures HikariCP's leakDetectionThreshold to log a warning when a connection is held for longer than 60 seconds without being closed. Helps identify connection leaks that could lead to pool exhaustion. * docs: add troubleshooting for stale Docker DB image cache Documents the MariaDB init failure caused by Docker caching an image before the DELIMITER conversion script runs. Includes the fix (--no-cache rebuild + volume cleanup) in both AGENTS.md and docs/database-configuration.md. * docs: add first-time setup instructions for Security Shepherd Documents the setup.jsp flow required after docker compose up, including how to get the auth token and the correct database hostname (container name, not localhost). * docs: fix setup instructions with correct parameter names The setup servlet uses 'dbauth' (not 'authToken') and requires MongoDB params (mhost, mport) even when not using mongo challenges. Documents the correct curl command and all parameter names for both agents and human contributors. * test: add load test simulating normal + aggressive users Python load test that boots the stack, configures the platform, registers 20 users, and simulates 17 normal users browsing alongside 3 aggressive users doing rapid automated scanning. Monitors DB connections and app responsiveness, reports pass/fail based on connection bounds and response times. * fix: return core DB connections to pool from Getter (stacked on #816) (#817) * fix: return core DB connections to pool from Getter methods - Use try-with-resources for all Getter paths that used manual closeConnection - Keep legacy ResultSet APIs (getClassInfo all classes, getPlayersByClass, getAdmins) unchanged - Add ConnectionPool.getCoreActiveConnections for tests - Add GetterCorePoolLeakIT to assert bounded pool usage under repeated authUser * style: format Getter.java with Google Java Format * fix: cache isInstalled(), tune pool for stability under load Setup.isInstalled() was called on every HTTP request via SetupFilter, each time reading database.properties from disk and borrowing a core pool connection just to check non-null. Under concurrent load this exhausted the pool and cascaded into a full app lockup. Cache with volatile Boolean + double-checked locking so the check runs once, then returns constant-time on all subsequent requests. resetInstalledCache() called after setup completes so the first post-setup request re-evaluates. Warm the cache at startup from DatabaseLifecycleListener.contextInitialized(). Pool tuning: - maxPoolSize 10 → 20 (supports realistic classroom concurrency) - connectionTimeout 30s → 5s (fail fast under overload instead of blocking Tomcat threads for 30s each) - minIdle 2 → 5 (reduce cold-start latency) Known limitation: authUser holds a DB connection during Argon2 password verification (~100-200ms). This limits throughput under high concurrency. Follow-up will release connection before hashing. Load test updated with --target and --concurrency flags for targeted per-class/per-method testing instead of broad soak only. * fix: only cache isInstalled()=true, not transient false If the DB is temporarily unreachable during the first isInstalled() call, caching false permanently locks the app into "not installed" state for the JVM lifetime. Only cache the true (terminal) state; leave installedCached=null on failure so subsequent requests retry. * chore: update pool docs, harden SetupIT, remove stale TODO Address Copilot review feedback on PR #817: - Update database.properties.example and docs/database-configuration.md to reflect new pool defaults (maxPoolSize=20, minIdle=5, connectionTimeout=5000) - Add assumeTrue guard in SetupIT cache tests so they skip instead of false-passing when the database is unavailable - Remove stale TODO in Getter.authUser (try-with-resources answers it) * fix: release DB connection before Argon2 verification in authUser (#819) (#821) * fix: release DB connection before Argon2 verification in authUser Split authUser into three phases so the connection is only held during actual DB work, not during CPU-bound Argon2 hashing: 1. Fetch user record into local variables (~1-5ms DB hold) 2. Verify password with Argon2 (no connection held, ~100-200ms) 3. Re-acquire connection briefly for userBadLoginReset if needed Previously the entire method ran inside a single try-with-resources block, holding a pool connection for ~200ms per login attempt. Under 20 concurrent logins this exhausted the pool and blocked unrelated endpoints (challenge lookups, scoreboards, module status). Auth decision flow and side effects are identical to previous implementation; only connection scope changes. No schema changes or stored procedure modifications required. Closes #819 Made-with: Cursor * fix: null-safe loginType check, remove unused variable - Use "login".equals(loginType) to prevent NPE if loginType is null - Remove unused userFound variable left over from refactor Made-with: Cursor * fix: check suspension and login type before Argon2 verification Move loginType and suspendedUntil checks before the expensive Argon2 hash so suspended and SSO users are rejected cheaply. Also adds a null guard on suspendedUntil to prevent NPE when the column is NULL. * test: add integration tests for authUser fail-fast checks Add GetterAuthIT with JUnit 5 tests verifying that: - suspended users are rejected before Argon2 verification - SSO users cannot authenticate via password login These cover the fail-fast checks moved before Argon2 in the previous commit. * ci: add concurrency group to deduplicate workflow runs (#827) When pushing to a feature branch with an open PR, both push and pull_request triggers fire. The concurrency group ensures only one run proceeds per PR/branch, cancelling the duplicate. * Remove Trello link from README (#822) Removed Trello link from the README. * fix: migrate all integration tests to JUnit 5 and fix revealed failures (#826) * fix: migrate all integration tests from JUnit 4 to JUnit 5 (#825) 37 IT files were using JUnit 4 annotations (org.junit.Test, @before, @BeforeClass, etc.) but the project only has junit-jupiter-engine — no junit-vintage-engine. The JUnit Platform silently skipped them all, reporting 0 tests run with no warning. Migrated all IT files to JUnit 5: updated imports, annotations, and converted @test(expected=...) to assertThrows(). Closes #825 * style: apply google-java-format to SetterIT.java * fix: resolve 8 integration test failures revealed by JUnit 5 migration - CountdownHandler.saveCountdowns: fix copy-paste bug where startTime was saved as lockTime, corrupting DB state - CountdownHandler: add forceReload() so cached static state can be invalidated when tests reset the database - TestProperties.executeSql: call forceReload() after DB reset to prevent stale CountdownHandler state across test classes - EnableModuleBlock: fix error message casing ("received" → "Received") - XxeChallenge1IT: use XxeChallenge1OldWebService (the XML-based challenge endpoint) instead of XxeLesson (which blocks challenge file content with its "Wrong File" check) * fix: add missing module-open check to XxeChallenge1OldWebService XxeChallenge1OldWebService was processing requests even when the module was closed, unlike its sibling XxeChallenge1 which properly checks Getter.isModuleOpen(). This caused XxeChallenge1IT testLevelWhenUnsafeLevelsAreDisabled to fail. * fix: add missing JUnit 5 imports and fix assertTrue argument order in SetupIT assertEquals and assumeTrue were used but not imported after the merge conflict resolution dropped them. Also fix assertTrue(String, boolean) to assertTrue(boolean, String) for JUnit 5 compatibility. * fix: return core DB connections to pool from Setter (stacked on #816) (#834) * test: fix three ITs missed by #832's TestProperties API split PR #832 split TestProperties.executeSql(Logger) into ensureSchemaReady(Logger) + reseedTestData(). The dev->dev#536 merge migrated most ITs to the new pattern but missed three files that fail to compile against the new signature. - src/it/java/dbProcs/GetterAuthIT.java - src/it/java/dbProcs/GetterCorePoolLeakIT.java - src/it/java/servlets/SetupIT.java (two @test methods) Same one-line swap applied to each — replace the executeSql(log) call with ensureSchemaReady(log) + reseedTestData(). No behavior change; restores the branch to a compilable state ahead of the Setter leak fix. * fix: return core DB connections to pool from Setter methods Mirrors the Getter leak fix in #817 for Setter.java. Pre-pooling each request opened a fresh DB connection, so leaks were invisible; with the HikariCP pool (#816), every leaky path bleeds the core pool and surfaces as 5s connection-acquire timeouts. - Convert all 41 Database.getCoreConnection / getChallengeConnection call sites in Setter.java to try-with-resources. Removes every manual Database.closeConnection — the Connection is AutoCloseable, so it returns to the pool on every code path including thrown exceptions. - Apply the auth-hold-time fix (#819) to three methods that ran Argon2 while holding a DB connection (updatePassword, updatePasswordAdmin, userCreate). Argon2 verify/hash is CPU-bound ~100-200ms; moving the hash before connection acquisition frees pool capacity for concurrent callers. - updatePassword additionally relied on Getter.authUser inside its own connection scope, which doubled the hold time. Phase 1 (verify) now uses Getter.authUser's own connection scope; phase 2 (hash) holds none; phase 3 (UPDATE) opens a fresh connection. - The 17 'throws SQLException' setting methods (setAdminCheatStatus, setFeedbackStatus, setLockTime, etc.) declared Connection outside any try and could throw RuntimeException before manual closeConnection, leaking unconditionally. Now bounded by try-with-resources. Add SetterCorePoolLeakIT mirroring GetterCorePoolLeakIT: hammers resetBadSubmission and suspendUser 500 times each with synthetic userIds and asserts ConnectionPool.getCoreActiveConnections stays bounded. Verified: 500 reset + 500 suspend iterations leave active connections at 0. SetterIT's update-family tests (updatePassword, updatePasswordAdmin, updateUsername, updateUserRole) still pass. Out of scope: the wider leak surface in servlets, Setup, scoreboards, JSON exporters, etc. Tracked as follow-up — same try-with-resources treatment applies file by file. * style: apply Google Java Format to Setter.java Two single-line collapses where my wrapping was below the 100-col threshold. CI's lint-java step runs google-java-format with --set-exit-if-changed on **/*.java; this brings the file back to canonical format. * fix: address PR #834 Copilot review feedback - userCreateSSO de-dup loop: wrap PreparedStatement and ResultSet in try-with-resources inside the while loop. The loop can run up to 500 iterations and previously held up to 500 open statements + server-side cursors simultaneously until the outer connection closed. - userCreateSSO userCreate call: wrap CallableStatement and ResultSet in try-with-resources so they release promptly instead of waiting on the outer connection scope. - updateCsrfCounter: fix log prefix (said 'Getter.updateCsrfCounter', should be 'Setter.updateCsrfCounter'). Pre-existing typo. - SetterCorePoolLeakIT: tighten the leak assertion from active <= MAX_ALLOWED_ACTIVE (32) to active == baseline. The loop is serial — a non-leaking pool MUST return active to baseline (typically 0) after every iteration. The old threshold could mask leaks: the actual pool max is 20, so a value of 21-32 was already saturation, and Hikari would throw SQLTransientConnectionException long before active reached 32, leaving the test green while the pool was dead. Drop the MAX_ALLOWED_ACTIVE constant. * test: tighten GetterCorePoolLeakIT leak assertion (#835) Mirrors the SetterCorePoolLeakIT tightening from #834. The loop is serial, so a non-leaking pool MUST return active connection count to the pre-call baseline after every iteration. The old assertion (active <= MAX_ALLOWED_ACTIVE = 32) could mask leaks: the actual core pool max is 20, and Hikari's 5s acquire timeout throws SQLTransientConnectionException long before active reaches 32 — so the test could pass while the pool was already dead. Replace with assertEquals(baseline, active) and drop the MAX_ALLOWED_ACTIVE constant. * fix: address second round of PR #834 Copilot review feedback String validation: - setModuleLayout and setScoreboardStatus compared incoming String args with != against literals (reference equality). This worked by accident because callers happened to pass interned strings, but would throw IllegalArgumentException on valid values arriving via HTTP params, JSON deserialization, or string concatenation. Switch to .equals(). Statement lifetimes: - updateUsername (PreparedStatement), updatePasswordAdmin (CallableStatement), and userCreate (CallableStatement + ResultSet) borrowed JDBC statements outside try-with-resources. With pooled connections the practical leak risk is small (Hikari releases statements when the connection returns to the pool), but explicit release matches the convention used in Getter.authUser (#817) and the userCreateSSO fix in 173836b. Wrap each in try-with-resources. All 14 affected SetterIT tests still pass — every accepted layout/status value remains accepted, every rejected value (including case-sensitive "CTF" and "Open") still rejected. * fix: address third round of PR #834 Copilot review feedback - updatePlayerResult held a core DB connection while calling Getter.getModuleNameLocaleKey, which opens its own. Two pooled connections per call doubles pool pressure under concurrency. Move the Getter call outside the try-with-resources so the first connection returns to the pool before the second is borrowed. Same auth-hold-time pattern that #819 applied to Getter.authUser. - SetterCorePoolLeakIT Javadoc claimed the success path leaked. The success path actually returned the connection; the leak was on the exception/early-return paths that skipped Database.closeConnection. Rewrite the comment to describe the real failure mode. - updatePassword Javadoc said it returned a ResultSet but the method returns boolean. Fix the @return. - updateUsername Javadoc was copy-pasted from updatePassword: wrong parameter docs (mentioned currentPassword/newPassword which do not exist on this method) and wrong return type. Rewrite to match the actual (ApplicationRoot, userName, newUsername) -> boolean shape. * fix: harden first-time setup against pool state (stacked on #834) (#836) * fix: harden first-time setup path against pool state Three bugs reviewers identified in the setup flow, all caused by routing setup credential-test and schema-init through the connection pool that the rest of the app uses: 1. ConnectionPool.getChallengeConnection always called loadDatabaseProperties() inside computeIfAbsent. During first setup, database.properties does not exist yet, so this threw RuntimeException (uncaught by Setup.java's catch (SQLException)) and 500'd the setup screen. The challenge path already receives url/user/password as arguments and only uses the loaded properties for optional pool tuning (all with defaults). Add loadDatabasePropertiesIfPresent() that returns an empty Properties when the file is missing. 2. Setup.java's connection-test went through Database.getConnection -> ConnectionPool.getChallengeConnection, which keys pools by (url, username). If setup is re-run with the same url+user but a different password, the test reuses the stale pool, validates against the old password, and writes the new password to disk — leaving the app broken on next startup. Setup is a one-shot credential check that runs before the pool config exists; route it through DriverManager.getConnection directly and bypass the pool entirely. 3. Setup.executeSqlScript and Setup.executeUpdateScript borrowed Database.getDatabaseConnection(null, true) without closing. Pre-pooling each setup attempt opened a fresh JDBC connection that GC eventually reclaimed. Now those calls draw from the challenge pool (max 3) and never return — repeated setup/upgrade attempts exhaust it. Wrap both methods in try-with-resources. * fix: bound setup connection-test timeout to 5s Address PR #836 Copilot review: when the setup credential test moved from the pool to DriverManager.getConnection, it lost Hikari's connectionTimeout=5000ms and inherited DriverManager's default (no bound). A typo'd hostname or unreachable port at setup time would hang the servlet thread on the OS's default TCP connect timeout — 75s on Linux, 21s on macOS, sometimes much longer — making the setup page appear frozen with no error. Append connectTimeout=5000 to the test JDBC URL so the credential test fails fast at the same threshold the pool uses for production traffic. MariaDB and MySQL JDBC drivers both honor this option natively, scoped to just this connection — no JVM-wide DriverManager.setLoginTimeout side effects. * test: bootstrap DB props in DowngradeAdminsIT and DeletePlayersIT (#837) These two ITs called into Getter/Setter without first writing the test database.properties file. Pre-pool (#816) this worked because the application code read the properties file lazily on every call, and the test runtime arranged for the file to exist via other means. Post- pool, ConnectionPool.initialize() is called once per JVM on the first getConnection() and reads the props file exactly once; if it is missing, the pool is marked unusable for the entire JVM and every subsequent test in that fork throws "Connection pool not available". Symptom in CI: every test method in both classes FATAL'd with a long FileNotFoundException stack trace pointing at target/test-classes/conf/database.properties, drowning out any real leak signal in the IT log. Add the standard @BeforeAll bootstrap used by the other 42 IT files (createMysqlResource + ensureSchemaReady + reseedTestData) to both files. Keep the existing @beforeeach for per-test request/response reset. Verified locally: 8 tests pass across the two files (4 each); prior to this fix all 8 failed at pool init. * fix: return DB connections from Getter ResultSet methods (CachedRowSet) (#838) * fix: return DB connections from Getter ResultSet-returning methods Three methods returned live JDBC ResultSets while leaving their owning Connection and CallableStatement borrowed from the core pool: - Getter.getClassInfo(String) — all classes - Getter.getPlayersByClass(String, String) - Getter.getAdmins(String) The caller could not return them because Connection ownership was private to the method. Under load this slowly bled the core pool until CI hit "CorePool - Connection is not available, request timed out after 5005ms". Materialize the results into a CachedRowSet (javax.sql.rowset) inside try-with-resources on Connection / CallableStatement / ResultSet. The caller still receives a ResultSet-compatible object but DB resources are released immediately. Result sizes here are all bounded (one row per class / per admin / per player in a class) so the in-memory cost is negligible. CachedRowSet.populate() leaves the cursor after the last row, so call beforeFirst() before returning so callers that do `while (rs.next())` without a prior beforeFirst() see all rows. Two latent bugs surfaced by this change: 1. The original getPlayersByClass had `resultSet.next()` before returning — a one-row cursor advance. The only production caller (GetPlayersByClass.playersInOptionTags) does `beforeFirst()` then `while (next())` so it iterates all rows; the advance was masked. The IT (testGetPlayersByClass) didn't call beforeFirst() and so counted N-1 rows, which the test compensated for by creating 10 users and expecting 9. Production semantics is "all rows"; the test was wrong. Fix the test to expect 10/10. 2. The IT had `if (i != 9) { if (i < 9) ... else if (i > 9) ... else { fail("Then surely the number WAS 9? How did this happen") } }` — the else branch was unreachable. Removed. Verified: GetterCorePoolLeakIT, SetterCorePoolLeakIT, GetterIT#testGetPlayersByClass, testGetAdmins, testGetClassInfoString, and testGetClassInfoStringString all pass. * fix: cap CachedRowSet materialization at 10k rows Address PR #838 Copilot review feedback: CachedRowSet.populate() materializes the entire result set in memory, and the backing stored procs (classesGetData, playersByClass, playersWithoutClass, adminGetAll) have no LIMIT clause. Realistic Security Shepherd workloads are well under this bound (class rosters of tens to a few hundred), but a runaway query — accidental or malicious — could OOM the JVM. Add CallableStatement.setMaxRows(MAX_ROWSET_ROWS) where MAX_ROWSET_ROWS = 10_000 to fail fast on the pathological case while remaining well above realistic use. Proper bounded-collection conversion (return List<DTO> instead of ResultSet) is tracked in #839 as part of the broader DAO refactor scope. Verified: GetterCorePoolLeakIT, SetterCorePoolLeakIT, GetterIT#testGetPlayersByClass, testGetAdmins, testGetClassInfoString, testGetClassInfoStringString all pass. * fix: address CodeQL JDBC resource-leak flags + IT timezone fix (#842) * fix: nested try-with-resources for CodeQL-flagged JDBC sites + IT timezone fix Closes CodeQL "potential database resource leak" flags on 8 methods. Connection-level pooling fix from #816/#817/#834/#838 already solved the real-world leak. CodeQL was flagging the surrounding Statement / ResultSet declarations as not deterministically closed — with Hikari, they get closed when the Connection returns to the pool, so practical risk is low, but explicit nested try-with-resources matches the project convention and silences the flags. Methods converted: - Getter.checkPlayerResult - Getter.findPlayerById - Getter.getAllModuleInfo - Getter.getChallenges (CodeQL: lines 540, 544) - Getter.getModulesJson (CodeQL: lines 2099, 2102) - Getter.getAdminCheatStatus (CodeQL: lines 2360, 2364) - Setter.setAdminCheatStatus (CodeQL: line 1153) - Setter.setPlayerCheatStatus (CodeQL: line 1177) - Setter.setModuleLayout (CodeQL: line 1207) - Setter.setFeedbackStatus (CodeQL: line 1231) Not converted: Getter.authUserSSO. The CodeQL flags on lines 230, 241, 345, 357 in that method are not a simple rewrap — the method has two separate prepared-statement queries with reassignment of the same `prestmt` variable, plus a nested call to Setter.userCreateSSO that holds its own pool connection. Needs the same auth-hold-time phasing that #819 and #834 applied to authUser and updatePassword. Tracked in #840. Test additions (GetterIT): - testGetAdminCheatStatus exercises the converted getAdminCheatStatus with setAdminCheatStatus round-trips (true → false → restore). - testGetModulesJson exercises the converted getModulesJson and asserts the documented 2-element JSON envelope shape + per-module field population. Catches any regression in the new try-with-resources scope around the inner result-set loop. Timezone fix (pom.xml): - Three pre-existing IT failures (testSuspendUser, testSSOAuthSuspended, suspendedUserIsRejected) pass in CI but fail on non-UTC developer machines. Root cause: JDBC `getTimestamp()` interprets SQL DATETIME values in the JVM's default timezone. With DB in UTC and JVM in Europe/Dublin (UTC+1 in summer), the suspendedUntil read comes back as an epoch-ms value 1 hour earlier than the actual stored UTC instant, so `suspendedUntil.after(currentTime)` returns false and the suspension check silently passes — a real production bug for any non-UTC Tomcat deployment. Pin tests to UTC via <argLine>-Duser.timezone=UTC</argLine> on surefire and failsafe. Underlying production bug tracked in #841. Verification: - mvn test: 164/164 unit tests pass - mvn -Dit.test='GetterIT,GetterAuthIT,GetterCorePoolLeakIT,SetterIT, SetterCorePoolLeakIT' failsafe: 132 tests pass, 0 failures (vs 130 pre-change — the 2 new tests, plus the 3 previously-failing timezone-sensitive tests now green) - semgrep custom rule: all 8 CodeQL-flagged methods now clean - google-java-format: clean * fix: address Copilot review comments on PR #842 - getModulesJson: restore broad try/catch around JSON header construction so a JSONException/ResourceBundle failure is logged and returns partial output again, instead of propagating out as it did after the try-with-resources refactor (behavior-preserving fix) - testGetModulesJson: openAllModules opens only one category per call, so call it with both false and true to actually open all modules and keep the modules.length() assertion from being data-dependent - setModuleLayout: fix copy/pasted log strings that referenced player cheat settings instead of the module layout being changed * ci: pin all GitHub Actions to commit SHA and upgrade to latest releases (#844) Repo policy requires actions pinned to full-length commit SHAs. Pin every action in test.yml and release.yml, and bump each to its current stable release (notably upload-artifact v3→v7, checkout v2/v4→v6, setup-java→v5). * fix: try-with-resources for authUserSSO (#840) + Setter JDBC statements (#845) * fix: try-with-resources for authUserSSO (#840) + Setter JDBC statements Resolves the remaining CodeQL "potential database resource leak" flags on the dev#536 connection-pooling work (stacked on #816). Getter.authUserSSO (#840): - Split into three phases, each opening its own pooled Connection in try-with-resources. The connection is no longer held across the Setter.userCreateSSO call (Phase 2), reducing peak pool usage from 2 concurrent connections per call to 1 - same anti-pattern #834 fixed in Setter.updatePassword/updatePlayerResult. - All PreparedStatements and ResultSets are now in nested try-with-resources, clearing CodeQL flags at Getter.java:230/241/345/357. - Behavior preserved: existing-user, new-user, duplicate-username and suspension paths are unchanged. The known #820 result-array quirks (result[3] unset, result[5] double-assigned) are intentionally left as-is and remain tracked by #820. One intentional improvement: a SQLException while reading the Phase-1 result set now propagates instead of being silently treated as "user not found". Setter.java: - Convert 17 settings/admin methods from bare Connection try-with-resources with an inner un-closed PreparedStatement/CallableStatement to combined try-with-resources. - Structurally complex methods (reassigned statements, conditional branches, statement+ResultSet, challenge-connection sites) are deferred to a separate follow-up to keep this change reviewable. Tests: - Add GetterIT.testSSOAuthExistingUserRelogin covering the Phase 1 (found) -> skip create -> Phase 3 path. Existing SSO ITs cover the other phases. - Unit tests pass locally (164 run, 0 failures). ITs validated in CI. * fix: address review feedback + lint on #845 - Narrow Getter.authUserSSO Phase 1 SELECT to `suspendedUntil` only (Copilot): existence comes from ResultSet.next(); avoids fetching userPass and 5 other unused columns. - Fix "forefully" -> "forcefully" typo in authUserSSO (Copilot). - Wrap two long lines in testSSOAuthExistingUserRelogin to satisfy google-java-format (lint-java CI fix). * fix: try-with-resources for remaining Setter.java JDBC sites (#846) (#852) Wrap the remaining structural JDBC sites in Setter.java with try-with-resources so PreparedStatement/CallableStatement/ResultSet are deterministically closed on all paths. SQL and branching are unchanged; this only changes resource management. - Cat A (statement reassignment): openAllModules, openOnlyMobileCategories, userDelete - Cat B (statement + ResultSet): updatePlayerClass, updatePlayerClassToNull, updateUserRole - Cat C (manual .close()): setCsrfChallengeFourCsrfToken, setCsrfChallengeSevenCsrfToken - Settings: setModuleCategoryStatusOpen, setLockTime, setEndTimeStatus, setEndTime Addresses the "Potential database resource leak" code-scanning flags in Setter.java (also surfaced on #816). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * refactor: try-with-resources for remaining Getter.java JDBC sites (#855) (#856) * refactor: try-with-resources for Getter.java JDBC sites (batch 1-2, #855) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: try-with-resources for remaining Getter.java JDBC sites (#855) Wrap all remaining PreparedStatement/CallableStatement/ResultSet sites in Getter.java with try-with-resources so JDBC resources are deterministically closed on every path. SQL strings, parameter bindings, and control flow are unchanged (verified: SQL literals byte-identical, 58 param-binding calls before and after). Notable cases: - getJsonScore: statement is conditionally assigned (totalScoreboard vs classScoreboard), so it cannot go in the Connection TWR header; registered as a closeable resource via `try (CallableStatement x = callstmnt; ResultSet rs = callstmnt.executeQuery())`. - isModuleOpen: converted statement + ResultSet to nested TWR, removed the now-redundant manual rs.close(). Completes the JDBC resource-leak cleanup for the dbProcs layer alongside Setter.java (#846 / PR #852). Addresses the recurring code-scanning "Potential database resource leak" flags on #816. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: SeanDuggan <sean.duggan4@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactors
Getter.javato use try-with-resources for core connections so Hikari returns connections to the pool under early returns and exceptions.Base branch
Targets
dev#536on OWASP — merge after / on top of #816 (connection pooling).Changes
ResultSetAPIs unchanged (getClassInfoall classes,getPlayersByClass,getAdmins).ConnectionPool.getCoreActiveConnections()for tests.GetterCorePoolLeakIT— bounded active connections under repeatedauthUsercalls.Test plan
mvn testGetterCorePoolLeakIT,GetterIT)