chore: add graceful shutdown for database connections - #67
Conversation
|
Warning Review limit reached
More reviews will be available in 26 minutes and 50 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughAdds graceful shutdown handling to ChangesGraceful DB Shutdown
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 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 `@lib/db.ts`:
- Around line 85-88: The signal handlers for SIGTERM and SIGINT are using
process.on which allows repeated invocations of the shutdown function if the
signal is delivered multiple times, causing duplicate logging and concurrent
raw.end() calls. Replace process.on with process.once for both signal handlers
to ensure they execute only once, and add an idempotency guard within the
shutdown function (such as a flag or state check) to prevent multiple executions
even if called directly multiple times, ensuring the function only performs
cleanup operations on the first invocation.
- Around line 71-83: The shutdown function's raw.end() promise call lacks error
handling for rejections and does not include the recommended timeout option from
the postgres library. Add a .catch() handler after the existing .then() to
handle any rejection from raw.end(), ensuring the timeout is cleared and an
error is logged appropriately. Additionally, pass a timeout option object to the
raw.end() method call to leverage the postgres library's built-in timeout
mechanism for preventing hangs during shutdown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| function shutdown(signal: string) { | ||
| singletonLogger.info("shutting down database connections", { signal }); | ||
| const timeout = setTimeout(() => { | ||
| singletonLogger.warn("database shutdown timed out, forcing exit"); | ||
| process.exit(1); | ||
| }, SHUTDOWN_TIMEOUT_MS); | ||
| timeout.unref(); | ||
|
|
||
| raw.end().then(() => { | ||
| clearTimeout(timeout); | ||
| singletonLogger.info("database connections closed", { signal }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -name "db.ts" -o -name "db.js" | head -20Repository: uniwayorg/UniWay
Length of output: 186
🏁 Script executed:
cat -n lib/db.ts | head -100Repository: uniwayorg/UniWay
Length of output: 3574
🌐 Web query:
postgres npm library end() method promise rejection error handling
💡 Result:
In the postgres.js library (npm package postgres), the .end() method is used to gracefully close connections [1][2]. When called, it rejects new queries and returns a Promise that resolves once all active queries are finished and the underlying connections are closed [1][2]. Regarding error handling and potential rejections: 1. Handling Hangs (Connection Issues): If a connection experiences an error (such as ECONNRESET) before .end() is called, the library may in some versions experience a hang where the .end() promise never resolves [3][4]. This occurs because the library may be waiting for a state that cannot be reached if the underlying socket was closed unexpectedly [4]. This issue has been subject to fixes (e.g., in PR #1142) to ensure that connection errors properly clear the query state, allowing .end() to complete [4]. If you encounter hangs during shutdown, ensure your library version is up-to-date [4]. 2. Using Timeouts: To prevent hangs during shutdown, it is recommended to use the { timeout } option when calling .end() [1][2]. sql.end({ timeout: 5 }); // Rejects pending queries and forces closure after 5 seconds [1][2] Providing a timeout ensures that even if a query is stuck or the server is unresponsive, the .end() promise will eventually reject or resolve rather than hanging indefinitely [1][2]. 3. General Best Practices: Always wrap your .end() call in a try/catch block to handle potential rejections during the shutdown process [3]. try { await sql.end({ timeout: 5 }); } catch (err) { console.error("Error during pool shutdown:", err); } Note: If you are using node-postgres (the pg package), the behavior differs; client.end() simply disconnects the client, and errors during the process or while idle are typically handled by attaching an error listener to the client instance (client.on('error', ...)), as the connection is long-lived [5]. Always verify whether you are using postgres (porsager/postgres) or pg (node-postgres), as their APIs and error handling strategies are distinct [5][1].
Citations:
- 1: https://github.com/porsager/postgres
- 2: https://github.com/porsager/postgres/
- 3: Calling sql.end() or sql.close() after a server disconnection causes a permanent hang porsager/postgres#1097
- 4: fix: end() hangs forever after ECONNRESET error porsager/postgres#1142
- 5: https://node-postgres.com/apis/client
🏁 Script executed:
cat -n lib/db.ts | sed -n '85,88p'Repository: uniwayorg/UniWay
Length of output: 347
🏁 Script executed:
rg "postgres" lib/db.ts | head -5Repository: uniwayorg/UniWay
Length of output: 321
🏁 Script executed:
# Check package.json for postgres version
cat package.json | grep -A 5 '"postgres"' || echo "Not found in simple grep"Repository: uniwayorg/UniWay
Length of output: 280
🏁 Script executed:
rg "process\.on\(" --type ts --type js -A 3 lib/Repository: uniwayorg/UniWay
Length of output: 288
🏁 Script executed:
# Check if there are any async signal handlers in the codebase
rg "process\.on\(" --type ts --type js -B 2 -A 5 | head -40Repository: uniwayorg/UniWay
Length of output: 154
🏁 Script executed:
node --help 2>&1 | grep -i signal || echo "Checking Node.js behavior with signal handlers"Repository: uniwayorg/UniWay
Length of output: 629
🏁 Script executed:
# Check if Node.js version or type hints give constraints
cat -n lib/db.ts | sed -n '1,15p'Repository: uniwayorg/UniWay
Length of output: 840
🏁 Script executed:
cat > /tmp/test_signal.js << 'EOF'
// Test whether signal handlers await async results
process.on("SIGTERM", async () => {
console.log("Handler called");
await new Promise(r => setTimeout(r, 1000));
console.log("This may not execute");
});
process.on("SIGINT", () => {
console.log("SIGINT received");
process.exit(0);
});
console.log("Process ready, send SIGTERM");
setTimeout(() => {
console.log("Sending SIGTERM to self");
process.kill(process.pid, "SIGTERM");
}, 100);
EOF
node /tmp/test_signal.js 2>&1 || echo "Script completed"Repository: uniwayorg/UniWay
Length of output: 206
🏁 Script executed:
# Verify postgres version supports { timeout } option
rg "raw\.end" lib/db.ts -A 2 -B 2Repository: uniwayorg/UniWay
Length of output: 295
Add rejection handler for raw.end() and use timeout option.
raw.end() is only handled on success. If it rejects (e.g., from hung connections or errors), the promise rejection will be unhandled. Additionally, the postgres library recommends passing a { timeout } option to prevent hangs during shutdown.
Proposed fix
function shutdown(signal: string) {
singletonLogger.info("shutting down database connections", { signal });
const timeout = setTimeout(() => {
singletonLogger.warn("database shutdown timed out, forcing exit");
process.exit(1);
}, SHUTDOWN_TIMEOUT_MS);
timeout.unref();
- raw.end().then(() => {
+ raw.end({ timeout: SHUTDOWN_TIMEOUT_MS }).then(() => {
clearTimeout(timeout);
singletonLogger.info("database connections closed", { signal });
+ }).catch((error) => {
+ singletonLogger.error("failed to close database connections", { signal, error });
+ process.exitCode = 1;
});
}🤖 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 `@lib/db.ts` around lines 71 - 83, The shutdown function's raw.end() promise
call lacks error handling for rejections and does not include the recommended
timeout option from the postgres library. Add a .catch() handler after the
existing .then() to handle any rejection from raw.end(), ensuring the timeout is
cleared and an error is logged appropriately. Additionally, pass a timeout
option object to the raw.end() method call to leverage the postgres library's
built-in timeout mechanism for preventing hangs during shutdown.
| if (typeof process !== "undefined" && !process.env.VITEST) { | ||
| process.on("SIGTERM", () => shutdown("SIGTERM")); | ||
| process.on("SIGINT", () => shutdown("SIGINT")); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, locate and inspect lib/db.ts
cat -n lib/db.ts | head -100Repository: uniwayorg/UniWay
Length of output: 3574
🏁 Script executed:
# Get outline to see function definitions
ast-grep outline lib/db.ts --view expandedRepository: uniwayorg/UniWay
Length of output: 733
Make shutdown signal handling one-shot and idempotent.
Using process.on allows repeated signal delivery to invoke shutdown() multiple times, causing duplicate logging, multiple timeout handlers, and concurrent raw.end() calls. Guard the shutdown path and use one-shot listeners.
Proposed fix
+let isShuttingDown = false;
+
function shutdown(signal: string) {
+ if (isShuttingDown) return;
+ isShuttingDown = true;
singletonLogger.info("shutting down database connections", { signal });
const timeout = setTimeout(() => {
singletonLogger.warn("database shutdown timed out, forcing exit");
process.exit(1);
}, SHUTDOWN_TIMEOUT_MS);
timeout.unref();
raw.end().then(() => {
clearTimeout(timeout);
singletonLogger.info("database connections closed", { signal });
});
}
if (typeof process !== "undefined" && !process.env.VITEST) {
- process.on("SIGTERM", () => shutdown("SIGTERM"));
- process.on("SIGINT", () => shutdown("SIGINT"));
+ process.once("SIGTERM", () => shutdown("SIGTERM"));
+ process.once("SIGINT", () => shutdown("SIGINT"));
}🤖 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 `@lib/db.ts` around lines 85 - 88, The signal handlers for SIGTERM and SIGINT
are using process.on which allows repeated invocations of the shutdown function
if the signal is delivered multiple times, causing duplicate logging and
concurrent raw.end() calls. Replace process.on with process.once for both signal
handlers to ensure they execute only once, and add an idempotency guard within
the shutdown function (such as a flag or state check) to prevent multiple
executions even if called directly multiple times, ensuring the function only
performs cleanup operations on the first invocation.
- Add SIGTERM/SIGINT handlers that call sql.end() with 5s timeout - Guard against Vitest test environment - Log shutdown lifecycle via structured logger Fixes UNI-42
- Add SIGTERM/SIGINT handlers that call sql.end() with 5s timeout - Guard against Vitest test environment with v8 ignore for untestable lines - Log shutdown lifecycle via structured logger Fixes UNI-42
16a6578 to
c837ade
Compare
1 participant
Add SIGTERM/SIGINT handlers that call
sql.end()with a 5-second timeout fallback. Guarded against Vitest test environments. Logs shutdown lifecycle via structured logger.Fixes UNI-42
Summary by CodeRabbit