Skip to content

Fix knex reinitialization after destroy #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
numbpill3d wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix knex reinitialization after destroy #47

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions server/utils/db-health.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ const getHealthStatus = (knex = null) => {
* @returns {Promise<Object>} Maintenance result
*/
const performMaintenance = async (knex = null) => {
const kInstance = knex || knexInstance || global.knex;
let kInstance = knex || knexInstance || global.knex;

if (!kInstance) {
console.error('ERROR: No knex instance available for maintenance');
Copy link
Copy Markdown
Contributor

@codereviewbot-ai codereviewbot-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling in performMaintenance function could be improved by throwing an exception instead of just logging and returning an error object. This change would allow the calling function to handle the error more dynamically, which is particularly useful in asynchronous operations where the state and flow of execution can be more complex to manage.

Suggested Change:

if (!kInstance) {
  throw new Error('No knex instance available for maintenance');
}

Expand All @@ -124,9 +124,16 @@ const performMaintenance = async (knex = null) => {
if (healthCheckStatus.consecutiveFailures >= 3) {
console.log('Performing database pool reset due to consecutive failures');

// Destroy and recreate the pool
// Destroy existing instance and create a fresh one using the same config
const config = kInstance.client && kInstance.client.config ? kInstance.client.config : null;
await kInstance.destroy();
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Potential missing reinitialization when config is null

If config is null, destroying the pool leaves knex unusable. Consider throwing an error or adding a fallback initialization.

await kInstance.initialize();

if (config) {
const newKnex = require('knex')(config);
kInstance = newKnex;
knexInstance = newKnex;
global.knex = newKnex;
Copy link
Copy Markdown
Contributor

@codereviewbot-ai codereviewbot-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct manipulation of the global state within performMaintenance function can lead to hard-to-track bugs and makes the code less modular and more difficult to test. Consider using dependency injection for providing the knex instance throughout your application. This approach would make the code more modular, easier to test, and reduce side effects that can arise from altering global state.

Suggested Change:
Remove the line global.knex = newKnex; and ensure that knex instances are passed explicitly where needed.

}

// Reset health check status
healthCheckStatus.consecutiveFailures = 0;
Expand Down