Skip to content

feat: add timeout decorator to connection and table schema controllers; configure knex connection pool #1783

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

Merged
Artuomka merged 3 commits into from
May 18, 2026
Merged

feat: add timeout decorator to connection and table schema controllers; configure knex connection pool #1783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98896e0
feat: add timeout decorator to connection and table schema controller…
Artuomka May 18, 2026
799ab74
Merge branch 'main' into backend_table_schema_timeouts
Artuomka May 18, 2026
e15f40f
Merge branch 'main' into backend_table_schema_timeouts
Artuomka May 18, 2026
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
1 change: 1 addition & 0 deletions backend/src/entities/connection/connection.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,7 @@ export class ConnectionController {
type: ConnectionDiagramResponseDTO,
})
@UseGuards(ConnectionDiagramGuard)
@Timeout(90000)
@Get('/connection/diagram/:connectionId')
async getConnectionDiagram(
@SlugUuid('connectionId') connectionId: string,
Expand Down
1 change: 1 addition & 0 deletions backend/src/entities/table-schema/table-schema.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
} from './use-cases/table-schema-use-cases.interface.js';

@UseInterceptors(SentryInterceptor)
@Timeout(90000)
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

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

🛠️ 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.

@Controller()
@ApiBearerAuth()
@ApiTags('Table Schema Changes')
Expand Down
8 changes: 6 additions & 2 deletions shared-code/src/knex-manager/knex-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import knex, { Knex } from 'knex';
import getPort from 'get-port';
import knex, { Knex } from 'knex';
import { LRUStorage } from '../caching/lru-storage.js';
import { ConnectionParams } from '../data-access-layer/shared/data-structures/connections-params.ds.js';
import { getTunnel } from '../helpers/get-ssh-tunnel.js';
import { LRUStorage } from '../caching/lru-storage.js';
export class KnexManager {
static knexStorage() {
const knexMap = new Map<ConnectionParams['type'], (connection: ConnectionParams) => Promise<Knex<any, any[]>>>();
Expand Down Expand Up @@ -126,6 +126,7 @@ export class KnexManager {

private static getPostgresKnex(connection: ConnectionParams): Knex<any, any[]> {
const { host, username, password, database, port, type, cert, ssl } = connection;
const pool = { min: 0, max: 3 };
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 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 -30

Repository: 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 -150

Repository: 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 2

Repository: 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 2

Repository: 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 -20

Repository: 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 -20

Repository: 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 -100

Repository: 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.

if (process.env.NODE_ENV === 'test') {
const newKnex = knex({
client: type,
Expand All @@ -137,6 +138,7 @@ export class KnexManager {
port: port,
application_name: 'rocketadmin',
},
pool,
});
return newKnex;
}
Expand All @@ -155,6 +157,7 @@ export class KnexManager {
rejectUnauthorized: false,
},
},
pool,
});
return newKnex;
}
Expand All @@ -169,6 +172,7 @@ export class KnexManager {
application_name: 'rocketadmin',
ssl: ssl ? { ca: cert ?? undefined, rejectUnauthorized: !cert } : false,
},
pool,
});
return newKnex;
}
Expand Down
Loading