Bugfix/job generator db connections lfx 1830 #2709
Conversation
WalkthroughThe pull request introduces several modifications across multiple files to enhance error handling and resource management within the database operations. Key changes include the addition of a Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
backend/src/middlewares/databaseMiddleware.ts (2)
11-14: Consider potential concurrency issues with shared QuestDB connectionThe shared
qdbvariable could lead to race conditions or connection pool exhaustion in concurrent scenarios. Consider:
- Moving the connection to request scope
- Implementing proper connection pooling
- Adding connection timeout handling
Line range hint
17-21: Improve error handling and resource cleanupThe current error handling swallows errors without proper cleanup:
- Database connection might remain open on error
- Error is logged but not propagated
Consider this implementation:
} catch (error) { log.error(error, 'Database connection error!') + if (req.database) { + await req.database.sequelize.close().catch(closeError => + log.error(closeError, 'Error closing database connection')) + } + if (req.qdb) { + await req.qdb.close().catch(closeError => + log.error(closeError, 'Error closing QuestDB connection')) + } + throw error // Propagate error to error handler middleware } finally { next() }backend/src/bin/jobs/refreshMaterializedViews.ts (1)
Line range hint
18-33: Address potential race condition in view refresh checkThe current implementation might have a race condition between checking for running queries and starting the refresh:
Consider using a transaction or advisory lock:
try { + const lockId = BigInt('0x' + Buffer.from(view).toString('hex')) % (2n ** 31n); + await database.sequelize.query('SELECT pg_advisory_lock($1)', { + bind: [lockId], + type: QueryTypes.SELECT, + }); const refreshQuery = `refresh materialized view concurrently "${view}"` const runningQuery = await database.sequelize.query( // ... existing query ... ) if (runningQuery.length > 0) { log.warn( `Materialized view will not be refreshed because there's already an ongoing refresh for it!`, ) + await database.sequelize.query('SELECT pg_advisory_unlock($1)', { + bind: [lockId], + type: QueryTypes.SELECT, + }); return false } // ... rest of the function ... + await database.sequelize.query('SELECT pg_advisory_unlock($1)', { + bind: [lockId], + type: QueryTypes.SELECT, + }); } catch (err) { log.error({ error: err }, 'Error while refreshing materialized view!') + await database.sequelize.query('SELECT pg_advisory_unlock($1)', { + bind: [lockId], + type: QueryTypes.SELECT, + }).catch(unlockErr => + log.error(unlockErr, 'Failed to release advisory lock')); return false }backend/src/database/models/index.ts (1)
Line range hint
50-104: Review connection pool and timeout configurationsThe connection pool and timeout settings are critical for the job generator service:
- Connection pool size differs between API (20) and other services (10)
- Multiple timeout settings are in place:
connectionTimeoutMillis: 15squery_timeout: parameterizedidle_in_transaction_session_timeout: 20s- Pool acquire timeout: 50s
- Pool idle timeout: 10s
Consider:
- Documenting the rationale for these timeout values
- Evaluating if the pool size of 10 is sufficient for the job generator service
- Monitoring connection pool utilization to ensure these settings are optimal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
backend/src/bin/jobs/refreshMaterializedViews.ts(2 hunks)backend/src/database/databaseConnection.ts(2 hunks)backend/src/database/models/index.ts(3 hunks)backend/src/middlewares/databaseMiddleware.ts(1 hunks)
🔇 Additional comments (3)
backend/src/database/databaseConnection.ts (1)
Line range hint 13-21: Review cached connection pattern for potential memory leaks
The cached connection pattern might prevent proper garbage collection and cleanup of unused connections. Consider:
- Adding a mechanism to invalidate the cache on errors
- Implementing connection pool lifecycle management
backend/src/database/models/index.ts (2)
105-117: Verify alternative query monitoring
With the removal of query profiling logic, we should ensure that alternative monitoring mechanisms are in place for debugging performance issues in production.
Consider implementing database monitoring through:
- Application Performance Monitoring (APM) tools
- Database-level monitoring and logging
- Sequelize's built-in logging capabilities (already configured in the options)
50-50: Verify removal of query profiling parameter
The simplified function signature aligns with the PR objectives. However, we should verify that removing the profileQueries parameter doesn't impact existing debugging capabilities.
✅ Verification successful
Removal of profileQueries parameter is safe
The verification shows that all existing calls to the models() function in databaseConnection.ts are already using only the two remaining parameters (queryTimeoutMilliseconds and databaseHostnameOverride). No other callers were found in the codebase, including test files. The removal of the profileQueries parameter is a safe change that doesn't impact any existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to profileQueries parameter
# to ensure all callers have been updated
# Search for any remaining calls passing profileQueries
rg "models\([^)]*profileQueries"
# Search for any remaining references to profileQueries in database-related files
fd -e ts -e js --full-path "database|models" --exec rg -l "profileQueries"
Length of output: 148
Script:
#!/bin/bash
# Let's check the actual usage of the models function across the codebase
rg "models\(" -A 2
# Also check for any test files that might be affected
fd -e test.ts -e spec.ts --exec rg "models\(" -A 2
Length of output: 778
| export async function databaseClose(database) { | ||
| await database.sequelize.close() | ||
| } |
There was a problem hiding this comment.
Enhance databaseClose with type safety and error handling
The databaseClose function lacks type safety and error handling:
Consider this implementation:
- export async function databaseClose(database) {
+ interface Database {
+ sequelize: {
+ close(): Promise<void>;
+ };
+ }
+ export async function databaseClose(database: Database) {
+ if (!database?.sequelize) {
+ throw new Error('Invalid database instance provided');
+ }
try {
await database.sequelize.close();
+ } catch (error) {
+ throw new Error(`Failed to close database connection: ${error.message}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function databaseClose(database) { | |
| await database.sequelize.close() | |
| } | |
| interface Database { | |
| sequelize: { | |
| close(): Promise<void>; | |
| }; | |
| } | |
| export async function databaseClose(database: Database) { | |
| if (!database?.sequelize) { | |
| throw new Error('Invalid database instance provided'); | |
| } | |
| try { | |
| await database.sequelize.close(); | |
| } catch (error) { | |
| throw new Error(`Failed to close database connection: ${error.message}`); | |
| } | |
| } |
| try { | ||
| await refreshMaterializedView('member_segments_mv', database, log) | ||
| await refreshMaterializedView('organization_segments_mv', database, log) | ||
| } catch (err) { | ||
| log.error(err, 'Error while refreshing materialized views!') | ||
| } finally { | ||
| await databaseClose(database) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling granularity and connection timeout
While the resource cleanup is good, consider these improvements:
- The 15-minute database timeout might mask connection issues
- Error handling could be more specific to different failure scenarios
Consider this implementation:
const database = await databaseInit(1000 * 60 * 5, true) // Reduce to 5 minutes
const log = getServiceChildLogger('RefreshMVJob')
try {
- await refreshMaterializedView('member_segments_mv', database, log)
- await refreshMaterializedView('organization_segments_mv', database, log)
+ const results = await Promise.allSettled([
+ refreshMaterializedView('member_segments_mv', database, log),
+ refreshMaterializedView('organization_segments_mv', database, log)
+ ]);
+ results.forEach((result, index) => {
+ if (result.status === 'rejected') {
+ log.error(result.reason, `Failed to refresh view ${index}`);
+ }
+ });
} catch (err) {
log.error(err, 'Error while refreshing materialized views!')
} finally {
await databaseClose(database)
}Committable suggestion skipped: line range outside the PR's diff.
| // if (profileQueries) { | ||
| // const oldQuery = sequelize.query | ||
| // sequelize.query = async (query, options) => { | ||
| // const { replacements } = options || {} | ||
| // const result = await logExecutionTimeV2( | ||
| // () => oldQuery.apply(sequelize, [query, options]), | ||
| // log, | ||
| // `DB Query:\n${query}\n${replacements ? `Params: ${JSON.stringify(replacements)}` : ''}`, | ||
| // ) | ||
|
|
||
| // return result | ||
| // } | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented out code
Instead of keeping commented out code in the codebase, it should be removed as it's preserved in version control history. This improves code maintainability and reduces confusion.
- // if (profileQueries) {
- // const oldQuery = sequelize.query
- // sequelize.query = async (query, options) => {
- // const { replacements } = options || {}
- // const result = await logExecutionTimeV2(
- // () => oldQuery.apply(sequelize, [query, options]),
- // log,
- // `DB Query:\n${query}\n${replacements ? `Params: ${JSON.stringify(replacements)}` : ''}`,
- // )
-
- // return result
- // }
- // }
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
Bug Fixes
New Features
Documentation