fix: return core DB connections to pool from Setter (stacked on #816)#834
Merged
Conversation
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.
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.
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.
There was a problem hiding this comment.
Pull request overview
Refactors dbProcs/Setter.java to ensure pooled DB connections are reliably returned (via try-with-resources) across success/early-return/exception paths, aligning Setter-side behavior with the connection-pooling work stacked on #816 and the Getter-side leak fixes in #817. Also updates a few integration tests to the newer schema setup API and adds an integration test intended to detect core-pool connection leaks in Setter methods.
Changes:
- Convert core + challenge DB connection borrowing in
Setter.javato try-with-resources and remove manualDatabase.closeConnection(...)calls. - Reduce core-pool hold time during password operations by hashing (Argon2) before acquiring a DB connection and by splitting
updatePasswordinto phases. - Update several ITs from
executeSql(log)toensureSchemaReady(log)+reseedTestData(), and addSetterCorePoolLeakIT.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/dbProcs/Setter.java |
Refactors connection lifetimes to reliably return pooled connections; restructures password update flows to avoid holding DB connections during hashing. |
src/it/java/servlets/SetupIT.java |
Updates test DB initialization to the newer schema-ready + reseed pattern. |
src/it/java/dbProcs/SetterCorePoolLeakIT.java |
Adds an IT intended to validate Setter methods don’t leak core-pool connections under repetition. |
src/it/java/dbProcs/GetterCorePoolLeakIT.java |
Updates test DB initialization to the newer schema-ready + reseed pattern. |
src/it/java/dbProcs/GetterAuthIT.java |
Updates test DB initialization to the newer schema-ready + reseed pattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 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.
1 task
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.
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.
Comment on lines
855
to
859
| log.debug("Executing userUpdateResult"); | ||
| callstmnt.execute(); | ||
| // User Executed. Now Get the Level Name Langauge Key | ||
| result = Getter.getModuleNameLocaleKey(ApplicationRoot, moduleId); | ||
| Database.closeConnection(conn); | ||
|
|
Comment on lines
+54
to
+55
| * safely. Pre-fix, every iteration would leak a connection on the success path's manual | ||
| * closeConnection call (and on every exception path). |
Comment on lines
637
to
643
|
|
||
| boolean result = false; | ||
|
|
||
| Connection conn; | ||
| try { | ||
| conn = Database.getCoreConnection(ApplicationRoot); | ||
| } catch (SQLException e) { | ||
| log.debug("Could not get core connection: " + e.toString()); | ||
| throw new RuntimeException(e); | ||
| } | ||
|
|
||
| // Phase 1: Verify current password (Getter.authUser manages its own connection) | ||
| log.debug("Checking current password"); | ||
| String user[] = Getter.authUser(ApplicationRoot, userName, currentPassword); | ||
|
|
Comment on lines
685
to
+689
| boolean result = false; | ||
|
|
||
| log.debug("Preparing username change call from username " + userName + " to " + newUsername); | ||
| PreparedStatement prestmnt; | ||
| try { | ||
| Connection conn = Database.getCoreConnection(ApplicationRoot); | ||
|
|
||
| prestmnt = | ||
| conn.prepareStatement( | ||
| "UPDATE users SET userName = ?, tempUsername = FALSE WHERE userName = ?;"); | ||
| try (Connection conn = Database.getCoreConnection(ApplicationRoot); | ||
| PreparedStatement prestmnt = |
4 tasks
- 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 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.
2 tasks
This was referenced May 10, 2026
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
Setter.java's counterpart to #817. Pre-pooling each request opened a fresh DB connection, so leaks were invisible (the original symptom of #536 was MySQL slowly bleeding out until Tomcat was manually restarted). With HikariCP capping the core pool at 20 and timing out at 5s (#816 / #819), every leaky path now surfaces fast as
SQLTransientConnectionException: CorePool - Connection is not available, request timed out after 5005ms.This PR closes the Setter side of those leaks.
Changes
Setter.java(~41 call sites)Database.getCoreConnection/getChallengeConnectionborrows converted to try-with-resources. All manualDatabase.closeConnectioncalls removed.ConnectionisAutoCloseable— returns to the pool on every code path including thrown exceptions.updatePassword,updatePasswordAdmin,userCreateupdatePasswordGetter.authUser's own connection scope; phase 2 (hash) holds none; phase 3 (UPDATE) opens a fresh connection. Previously held one connection across all three phases and calledGetter.authUser(which opens its own) inside the outer scope.setAdminCheatStatus,setFeedbackStatus,setLockTime, etc.)Connectionoutside anytryand threwRuntimeExceptionbefore manualcloseConnection, leaking unconditionally on every settings-update failure. Now bounded by try-with-resources.SetterCorePoolLeakIT.java(new)GetterCorePoolLeakIT— hammersresetBadSubmissionandsuspendUser500x each with synthetic userIds and assertsConnectionPool.getCoreActiveConnectionsstays bounded.The first commit is a small drive-by fix for three pre-existing IT files (
GetterCorePoolLeakIT,GetterAuthIT,SetupIT) that the dev->dev#536 merge missed when migrating offTestProperties.executeSql(Logger)(the API was split in #832). Without it the branch doesn't compile. One-line swap toensureSchemaReady(log) + reseedTestData()to match the pattern used in the 35 other ITs.Verification
mvn test-compileclean.SetterCorePoolLeakITandGetterCorePoolLeakITboth pass — 500 × 2 iterations leavecoreActiveConnections == 0.SetterIT#testUpdatePassword,testUpdatePasswordAdmin,testUpdateUsername,testUpdateUserRolepass — covers the substantively refactored methods.SetterIT#testSuspendUserfails on this branch, but it was already failing on stockorigin/dev#536before this PR — pre-existing failure, not a regression.Scope
This PR is
Setter.javaonly — matching #817's narrow scope. The follow-up work (servlets,Setup, scoreboard exporters, SAML / SSO paths, challenge servlets) needs the same try-with-resources treatment file by file. Discussion welcome on whether to bundle that as one PR or split per package on this branch — it's the same mechanical transform either way.Test plan
SetterCorePoolLeakITand the update-family tests inSetterIT)