feat: removed pool config from knex and better closing in cli #3155
Conversation
WalkthroughThis pull request implements a series of changes focused on enhancing connection and error management across both the backend and CLI components. In the SQL database provider, the connection pool configuration has been removed and transaction handling has been refactored to use a 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
backend/src/ee/services/dynamic-secret/providers/sql-database.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (3)
backend/src/ee/services/dynamic-secret/providers/sql-database.ts (1)
121-141: Ensure the removal of pool config meets performance needs.You are using a
try/finallyblock for transaction execution and destroying thedbclient immediately after creation, which cleanly prevents resource leaks. However, removing the pool configuration means each call will create and destroy a fresh connection. For high-traffic scenarios, this can negatively affect performance. Consider whether a small pool of reusable connections is acceptable and, if so, provide a more controlled approach to connection lifecycle.cli/packages/cmd/gateway.go (1)
63-108: Consider configurable or exponential backoff for retries.You introduce a retry mechanism with a fixed 5-second interval when connecting or listening fails. While it works, use of a simple ticker may lead to repeated retries in resource-heavy environments. Consider adding:
• A configurable initial delay.
• Exponential backoff to increase wait times under recurring failures.- retryTicker := time.NewTicker(5 * time.Second) + backoffDuration := 5 * time.Second // or configurable + // Potential exponential backoff approach if desiredcli/packages/gateway/gateway.go (1)
265-277: Graceful shutdown with a 5-second wait.Signaling the accept loop and then waiting up to 5 seconds is a good compromise. For heavily loaded gateways or longer-running connections, consider making this timeout configurable or logging additional details if the wait is insufficient.
- case <-time.After(5 * time.Second): + // Consider making 5s a configurable setting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/ee/services/dynamic-secret/providers/sql-database.ts(3 hunks)cli/packages/cmd/gateway.go(1 hunks)cli/packages/gateway/gateway.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run integration test
- GitHub Check: Check TS and Lint
🔇 Additional comments (9)
backend/src/ee/services/dynamic-secret/providers/sql-database.ts (2)
157-168: Transaction scope and graceful resource cleanup look solid.Similar to the create operation, you properly encapsulate the
revokelogic inside a transaction and ensure the database connection is destroyed in thefinallyblock. This helps maintain code consistency and reliability. Keep an eye on potential performance impacts as noted previously.
192-203: Renew logic replicates the same pattern—good consistency.This
try/finallytechnique for reliable cleanup is applied consistently in therenewmethod and is a good approach. As with the other operations, just be aware of any overhead from repeatedly initializing and destroying connections in high-volume use cases, especially since pools have been removed.cli/packages/cmd/gateway.go (1)
56-61: Second-signal handling is a great fallback.Having a fallback for forceful termination upon receiving a second signal is helpful for edge cases where the first signal might fail to gracefully stop the process. Ensure developers clearly understand that any unflushed resources will be discarded if the second signal arrives too quickly.
cli/packages/gateway/gateway.go (6)
115-115: No functional changes identified.There doesn't appear to be a substantive difference here beyond a new annotation or whitespace. Proceeding without further comment.
143-144: Channel-based shutdown strategy is clear.Defining
shutdownChto control the lifecycle of permission registration and other loops is an elegant approach. This channel signaling pattern simplifies concurrency management and clarifies shutdown flow.Also applies to: 155-155
173-176: Heartbeat and relay checks align well with channel-based design.Registering your heartbeat and relay-active checks with the same
shutdownChensures that these periodic tasks end promptly. This prevents zombie goroutines after shutdown.
182-185: Non-blocking accept loop with adjustable deadlines is robust.Using a short deadline on the listener in combination with checking
ctx.Done()andshutdownChhelps keep the accept loop from permanently blocking. This approach works well for a more graceful shutdown.Also applies to: 187-191
213-218: TLS handshake and certificate checks are properly enforced.You properly set a handshake deadline and validate certificate fields (
gateway-client,cloud). This ensures secure, validated connections. Good job promptly closing the connection on mismatch.Also applies to: 231-232
238-253: Goroutine closure strategy is well-considered.A dedicated goroutine waits on
ctx.Done()orshutdownChand then closes the connection. This ensures each accepted connection can be forcibly terminated if needed without complicating the main accept loop.
Summary
🧪 2 passedWant to make Fume a regular reviewer at Infisical? 👉 let's talk |
Description 📣
Improved gateway closing and removed pool from knexjs for dynamic secret sql
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippetsSummary by CodeRabbit
Bug Fixes
Refactor