Repository navigation
Handle SQL connection initialization failures safely - #111
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86aa5f5e54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // disconnects explicitly when construction is aborted instead. | ||
| } | ||
|
|
||
| if (ok && emitDiagnostics && config.isDebug()) { |
There was a problem hiding this comment.
When config.isDebug() is true and ConnectionManager.open() returns false, the existing public connect(config) path used to invoke the subclass's failure logger, but this success-only condition now returns false silently. getLastConnectFailure() does not preserve that callback behavior for existing consumers; apply emitDiagnostics to both outcomes while keeping the constructor-only call silent.
AGENTS.md reference: AGENTS.md:L33-L36
Useful? React with 👍 / 👎.
| } catch (SQLException | RuntimeException | Error failure) { | ||
| try { mysql.disconnect(); } | ||
| catch (RuntimeException | Error cleanupFailure) { failure.addSuppressed(cleanupFailure); } | ||
| throw new IllegalStateException("Failed to initialize SQL database selection", failure); |
There was a problem hiding this comment.
If a JDBC driver throws a RuntimeException or the JVM raises an Error while preparing or executing USE, this catch converts it to IllegalStateException; previously those unchecked failures propagated unchanged, and wrapping an Error can also defeat fatal-error handling. Perform cleanup but rethrow the original unchecked failure, reserving the initialization wrapper for the checked SQLException path.
AGENTS.md reference: AGENTS.md:L44-L46
Useful? React with 👍 / 👎.
A rejected SQL pool currently allows table construction to continue and invoke subclass logging before subclass fields are initialized. Abort connection/database-selection setup with the original exception, skip subsequent queries, and disconnect resources owned by an aborted table constructor. Successful external connection diagnostics and legacy asynchronous recovery after a temporary connection failure remain supported.
The existing public entry point remains available. The additive diagnostics-free overload allows constructor setup without virtual logging callbacks. Database identifiers use the existing dialect-aware escaping helper. No storage fallback, schema change, dependency upgrade or configuration migration.
Validation (JDK 21 target, isolated local Maven repository):
mvn -B -o -ntp -f SimpleAPI/pom.xml clean install: 464 passed, one optional skip, zero failures/errors; full and shared JARs produced and inspected.-Plegacy-mysql-tls-test -Dtest=MariaDbFallbackTlsTest test: nine passed.git diff --checkpassed.Authentication rejection is simulated by an in-process JDBC driver through real Hikari initialization; tests cover original causes, no premature hooks/queries, failed database selection, quoted database names and successful asynchronous update/batch recovery. This is automated testing, not a live external MySQL or visually verified gameplay test.
Build/dependency order: SimpleAPI first, then the paired AdvancedCore fix, then the final VotingPlugin JAR. Paired fixes: AdvancedCore #361 and VotingPlugin #1701. Consumer PRs remain draft until corrected dependencies are available to hosted builds. Changes are modern repositories only.
Follow-up regressions also verify failed public-connect debug output without credentials and unchanged runtime/fatal JDBC exception identity after cleanup. Hosted initial build passed; the new head is awaiting its own checks.
SQL worker cleanup follow-up (
cd3f650): disconnect now attempts worker retirement even if pool closure throws, preserving the original failure and suppressing a second cleanup failure. Regression coverage includes successful cleanup, RuntimeException/Error failures from each resource, both failing, pre-connect shutdown and repeated shutdown.Validation against the exact corrected producer/consumer artifacts:
git diff --checkpassed.These are automated mocked/unit and artifact checks, not a live MySQL authentication test. AdvancedCore #361 and VotingPlugin #1701 still require the corrected dependency artifacts to be available to their hosted builds. No release or deployment was performed.