feat: add timeout decorator to connection and table schema controllers; configure knex connection pool#1783
Conversation
📝 WalkthroughWalkthroughThis PR applies performance tuning across two layers: endpoint-level request timeouts and database-level connection pooling. The connection diagram and table schema endpoints now have 90-second execution windows, while PostgreSQL Knex instances are provisioned with a bounded connection pool (min: 0, max: 3) across all initialization paths. ChangesPerformance Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
There was a problem hiding this comment.
Pull request overview
Adds a per-request timeout decorator (90s) to two long-running endpoints — the table schema controller (class-level) and the connection diagram endpoint — and configures an explicit Knex connection pool (min: 0, max: 3) for all PostgreSQL Knex instances in the shared knex manager.
Changes:
- Apply
@Timeout(90000)at the class level ofTableSchemaControllerand at the method level ofgetConnectionDiagram. - Configure a shared
{ min: 0, max: 3 }pool for the test, Heroku-style, and default Postgres Knex client constructions. - Reorder/reformat imports in
knex-manager.ts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| backend/src/entities/table-schema/table-schema.controller.ts | Adds class-level 90s timeout decorator to all table-schema endpoints. |
| backend/src/entities/connection/connection.controller.ts | Adds 90s timeout decorator to the connection diagram endpoint. |
| shared-code/src/knex-manager/knex-manager.ts | Sets explicit Knex pool {min:0,max:3} for all Postgres connection variants and reorders imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shared-code/src/knex-manager/knex-manager.ts (1)
141-141: 💤 Low valueConsider explaining why only PostgreSQL receives pool configuration.
The pool limits are applied consistently across all three PostgreSQL initialization paths (test, host match, and default), which is good. However, MySQL (line 182), MSSQL (line 198), and Oracle (line 219) Knex instances do not receive pool configuration.
If PostgreSQL needs bounded pooling for resource management, the other databases likely do as well. If there's a specific reason PostgreSQL is treated differently (e.g., it's the primary/production database), consider documenting this in a comment.
Also applies to: 160-160, 175-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared-code/src/knex-manager/knex-manager.ts` at line 141, The pool configuration is only being applied to PostgreSQL initialization paths (the places passing pool into the Knex config), leaving MySQL, MSSQL and Oracle Knex instances without bounded pooling; update the Knex creation calls for MySQL, MSSQL and Oracle in knex-manager (the create/initialization blocks that construct their Knex instances) to include the same pool configuration variable (pool) or, if omission was intentional, add a clear comment next to each non-Postgres create call explaining why pooling is omitted and why Postgres is special; ensure you reference the same pool object/shape used for Postgres so all DB clients have consistent resource limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/entities/table-schema/table-schema.controller.ts`:
- Line 44: Remove the class-level `@Timeout`(90000) decorator from the controller
and instead add method-level `@Timeout`(90000) only to the SQL-heavy endpoints
(approve, rollback, approveBatch, rollbackBatch) so short-lived routes like get,
list, getBatch, reject, and rejectBatch use the default shorter timeout; leave
generate's existing override as-is. Locate the controller class and the methods
named get, list, getBatch, reject, rejectBatch, approve, rollback, approveBatch,
rollbackBatch, remove the class decorator, and add `@Timeout`(90000) above the
approve/rollback/approveBatch/rollbackBatch method definitions.
In `@shared-code/src/knex-manager/knex-manager.ts`:
- Line 129: The pool is hardcoded with max: 3 in knex-manager (variable pool)
which may be too small given TableSchemaController endpoints use `@Timeout`(90000)
and long-running concurrent operations; update the knex connection pool to use a
higher default max (for example 10) and/or make it configurable via
environment/config (read from config key or process.env) so it can be tuned
per-deployment; adjust where the pool object is constructed in knex-manager (the
pool variable and the knex initialization code that consumes it) and ensure the
new config value is validated and documented.
---
Nitpick comments:
In `@shared-code/src/knex-manager/knex-manager.ts`:
- Line 141: The pool configuration is only being applied to PostgreSQL
initialization paths (the places passing pool into the Knex config), leaving
MySQL, MSSQL and Oracle Knex instances without bounded pooling; update the Knex
creation calls for MySQL, MSSQL and Oracle in knex-manager (the
create/initialization blocks that construct their Knex instances) to include the
same pool configuration variable (pool) or, if omission was intentional, add a
clear comment next to each non-Postgres create call explaining why pooling is
omitted and why Postgres is special; ensure you reference the same pool
object/shape used for Postgres so all DB clients have consistent resource
limits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73609417-9d87-414c-9708-db82a68c694c
📒 Files selected for processing (3)
backend/src/entities/connection/connection.controller.tsbackend/src/entities/table-schema/table-schema.controller.tsshared-code/src/knex-manager/knex-manager.ts
| } from './use-cases/table-schema-use-cases.interface.js'; | ||
|
|
||
| @UseInterceptors(SentryInterceptor) | ||
| @Timeout(90000) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Class-level timeout may be too broad for read and reject operations.
The 90-second timeout applies to all routes in this controller except generate (which has its own override). Several operations are unlikely to need 90 seconds:
- Read operations:
get(line 168),list(line 152),getBatch(line 228) - State-only changes:
reject(line 127),rejectBatch(line 202)
Operations that execute SQL migrations (approve, rollback, approveBatch, rollbackBatch) are the ones that justify extended timeouts. Consider applying @Timeout(90000) selectively at the method level for those endpoints instead of at the class level.
⏱️ Suggested approach: Apply timeouts selectively
Remove the class-level @Timeout(90000) and apply it only to SQL-executing operations:
`@UseInterceptors`(SentryInterceptor)
-@Timeout(90000)
`@Controller`()
`@ApiBearerAuth`()
`@ApiTags`('Table Schema Changes')Then add method-level decorators:
`@ApiOperation`({ summary: 'Approve and apply a pending schema change.' })
`@ApiParam`({ name: 'changeId', type: String })
`@ApiBody`({ type: ApproveSchemaChangeDto, required: false })
`@ApiResponse`({ status: 200, type: SchemaChangeResponseDto })
`@UseGuards`(SchemaChangeOwnershipGuard)
+@Timeout(90000)
`@Post`('/table-schema/change/:changeId/approve')Apply the same pattern to rollback, approveBatch, and rollbackBatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/entities/table-schema/table-schema.controller.ts` at line 44,
Remove the class-level `@Timeout`(90000) decorator from the controller and instead
add method-level `@Timeout`(90000) only to the SQL-heavy endpoints (approve,
rollback, approveBatch, rollbackBatch) so short-lived routes like get, list,
getBatch, reject, and rejectBatch use the default shorter timeout; leave
generate's existing override as-is. Locate the controller class and the methods
named get, list, getBatch, reject, rejectBatch, approve, rollback, approveBatch,
rollbackBatch, remove the class decorator, and add `@Timeout`(90000) above the
approve/rollback/approveBatch/rollbackBatch method definitions.
|
|
||
| private static getPostgresKnex(connection: ConnectionParams): Knex<any, any[]> { | ||
| const { host, username, password, database, port, type, cert, ssl } = connection; | ||
| const pool = { min: 0, max: 3 }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find PostgreSQL connection usage patterns to assess concurrency needs
# Find all controller methods that might use database connections
echo "=== Endpoints that could use PostgreSQL connections ==="
rg -n -A 2 '@(Get|Post|Put|Delete)\(' backend/src/entities/connection/connection.controller.ts backend/src/entities/table-schema/table-schema.controller.ts | head -50
# Check if there are use cases that might hold connections for extended periods
echo -e "\n=== Use cases that might perform long-running database operations ==="
rg -n 'getConnectionDiagram|generate.*schema|approve.*schema|rollback.*schema' backend/src/entities/ -A 3 | head -30Repository: rocket-admin/rocketadmin
Length of output: 9756
🏁 Script executed:
# Read the knex-manager.ts file to see full context
cat -n shared-code/src/knex-manager/knex-manager.ts | head -150Repository: rocket-admin/rocketadmin
Length of output: 5688
🏁 Script executed:
# Search for 90-second timeout references in the PR
rg '90|9000|timeout' backend/src/entities/table-schema/ --type ts -A 2 -B 2Repository: rocket-admin/rocketadmin
Length of output: 2489
🏁 Script executed:
# Check if there are other pool configurations in the codebase
rg 'pool.*max|max.*pool|connectionLimit' shared-code/ backend/ --type ts -B 2 -A 2Repository: rocket-admin/rocketadmin
Length of output: 561
🏁 Script executed:
# Look for any concurrency or load testing documentation
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l "concurrent\|concurrency\|load\|pool\|connection" {} \; | head -20Repository: rocket-admin/rocketadmin
Length of output: 397
🏁 Script executed:
# Look for deployment or load testing documentation
find . -type f \( -name "*.md" -o -name "*.yaml" -o -name "*.yml" -o -name "*.json" \) | xargs grep -l "deployment\|scale\|concurrent\|load\|pool" 2>/dev/null | head -20Repository: rocket-admin/rocketadmin
Length of output: 448
🏁 Script executed:
# Check if there are environment-based pool configurations
rg 'pool|max.*connection|CONNECTION' backend/src/ shared-code/src/ --type ts --type json -B 2 -A 2 | grep -A 3 -B 3 'env\|ENV'Repository: rocket-admin/rocketadmin
Length of output: 1046
🏁 Script executed:
# Check the connection cache implementation to understand scope
cat -n shared-code/src/caching/lru-storage.ts | head -100Repository: rocket-admin/rocketadmin
Length of output: 5631
Verify that max: 3 connections is sufficient for expected concurrency.
The @Timeout(90000) decorator on TableSchemaController confirms 90-second timeouts on all schema endpoints, and multiple endpoints can hold connections simultaneously: POST /table-schema/:connectionId/generate, approval/rejection/rollback operations (single and batch), and list/get endpoints. With a pool maximum of 3 connections per database, concurrent requests against the same database connection could exhaust the pool, causing requests to queue or timeout.
Consider:
- Typical concurrent user count against each database connection
- Long-running operations (schema generation, table changes) holding connections for up to 90 seconds
- Whether 3 connections provides sufficient headroom for simultaneous operations
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared-code/src/knex-manager/knex-manager.ts` at line 129, The pool is
hardcoded with max: 3 in knex-manager (variable pool) which may be too small
given TableSchemaController endpoints use `@Timeout`(90000) and long-running
concurrent operations; update the knex connection pool to use a higher default
max (for example 10) and/or make it configurable via environment/config (read
from config key or process.env) so it can be tuned per-deployment; adjust where
the pool object is constructed in knex-manager (the pool variable and the knex
initialization code that consumes it) and ensure the new config value is
validated and documented.
Summary by CodeRabbit