feat(shutdown): graceful shutdown timeout with force exit | 优雅关闭超时#227
feat(shutdown): graceful shutdown timeout with force exit | 优雅关闭超时#227mechanic-Q wants to merge 1 commit intorohitg00:mainfrom
Conversation
Add AGENTMEMORY_SHUTDOWN_TIMEOUT_MS env var (default 10000ms). If shutdown takes longer than the timeout (e.g. stuck sdk connection), the process force-exits instead of hanging indefinitely. The timeout uses timer.unref() so it doesn't prevent the process from exiting naturally if shutdown completes within the timeout window. 添加优雅关闭超时机制，防止因 sdk 连接卡死导致进程僵死。 默认 10 秒超时，可通过 AGENTMEMORY_SHUTDOWN_TIMEOUT_MS 配置。
|
@mechanic-Q is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdded a shutdown timeout mechanism to the shutdown routine that enforces an overall timeout via the ChangesShutdown Timeout Mechanism
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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)
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. Review rate limit: 5/8 reviews remaining, refill in 22 minutes and 28 seconds.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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
417-437:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake shutdown idempotent.
SIGINTandSIGTERMcan both reach this handler while the first invocation is still awaitingviewerServer.close(),indexPersistence.save(), orsdk.shutdown(). A second pass would start duplicate cleanup and a second force-exit timer concurrently.🔁 Suggested fix
+ let shuttingDown = false; + const shutdown = async () => { + if (shuttingDown) return; + shuttingDown = true; console.log(`\n[agentmemory] Shutting down...`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 417 - 437, Make the shutdown handler idempotent by guarding the async shutdown() function with a single-invocation flag (e.g., let isShuttingDown = false) so repeated SIGINT/SIGTERM calls return immediately; only create the forceExit timer and call forceExit.unref() on the first invocation, run healthMonitor.stop(), dedupMap.stop(), indexPersistence.stop(), await viewerServer.close(), await indexPersistence.save().catch(...), await sdk.shutdown(), then clearTimeout(forceExit) and exit; ensure the guard is checked at the top of shutdown() and set true before starting async work so concurrent signals don't start duplicate cleanup or timers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 419-424: The code uses parseInt on
process.env["AGENTMEMORY_SHUTDOWN_TIMEOUT_MS"] to compute timeoutMs but doesn't
validate NaN, which causes setTimeout(NaN) to behave incorrectly; update the
initialization of timeoutMs to parse the env var then validate it's a positive
finite integer (fallback to 10000 on missing, non-numeric, negative, zero, or
NaN), then use that validated timeoutMs when calling setTimeout for forceExit
(referencing timeoutMs, AGENTMEMORY_SHUTDOWN_TIMEOUT_MS, forceExit, and
setTimeout in the change).
---
Outside diff comments:
In `@src/index.ts`:
- Around line 417-437: Make the shutdown handler idempotent by guarding the
async shutdown() function with a single-invocation flag (e.g., let
isShuttingDown = false) so repeated SIGINT/SIGTERM calls return immediately;
only create the forceExit timer and call forceExit.unref() on the first
invocation, run healthMonitor.stop(), dedupMap.stop(), indexPersistence.stop(),
await viewerServer.close(), await indexPersistence.save().catch(...), await
sdk.shutdown(), then clearTimeout(forceExit) and exit; ensure the guard is
checked at the top of shutdown() and set true before starting async work so
concurrent signals don't start duplicate cleanup or timers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const timeoutMs = parseInt(process.env["AGENTMEMORY_SHUTDOWN_TIMEOUT_MS"] || "10000", 10); | ||
| const forceExit = setTimeout(() => { | ||
| console.warn(`[agentmemory] Shutdown timed out after ${timeoutMs}ms, forcing exit`); | ||
| process.exit(0); | ||
| }, timeoutMs); | ||
| forceExit.unref(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/index.ts | sed -n '410,440p'Repository: rohitg00/agentmemory
Length of output: 1405
🏁 Script executed:
# Verify parseInt behavior with NaN
node <<'EOF'
const timeoutMs = parseInt(process.env["AGENTMEMORY_SHUTDOWN_TIMEOUT_MS"] || "10000", 10);
console.log("timeoutMs with default:", timeoutMs);
// Test NaN case
const testNaN = parseInt(undefined || "10000", 10);
console.log("setTimeout with valid default:", testNaN);
// Test actual NaN scenario
const envUndefined = parseInt(undefined, 10);
console.log("parseInt(undefined, 10):", envUndefined);
// Test setTimeout behavior with NaN
console.log("Testing setTimeout with NaN:");
const timer = setTimeout(() => {
console.log("Timeout fired");
}, NaN);
console.log("Timer object created:", typeof timer);
clearTimeout(timer);
EOFRepository: rohitg00/agentmemory
Length of output: 388
🏁 Script executed:
# Check package.json for module type
cat package.json | grep -A2 -B2 '"type"'Repository: rohitg00/agentmemory
Length of output: 391
Validate the shutdown timeout to prevent malformed env vars from breaking the graceful-shutdown window.
parseInt() on invalid input returns NaN. When process.env["AGENTMEMORY_SHUTDOWN_TIMEOUT_MS"] is set to an invalid value (e.g., empty string, non-numeric text), parseInt() returns NaN, and setTimeout(NaN) triggers a Node.js warning and sets the timeout to 1ms instead of the intended graceful-shutdown duration. The || "10000" fallback only handles the missing case; it does not validate invalid env var values.
🔧 Suggested fix
- const timeoutMs = parseInt(process.env["AGENTMEMORY_SHUTDOWN_TIMEOUT_MS"] || "10000", 10);
+ const parsedTimeoutMs = Number.parseInt(process.env.AGENTMEMORY_SHUTDOWN_TIMEOUT_MS ?? "", 10);
+ const timeoutMs =
+ Number.isFinite(parsedTimeoutMs) && parsedTimeoutMs > 0 ? parsedTimeoutMs : 10_000;📝 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.
| const timeoutMs = parseInt(process.env["AGENTMEMORY_SHUTDOWN_TIMEOUT_MS"] || "10000", 10); | |
| const forceExit = setTimeout(() => { | |
| console.warn(`[agentmemory] Shutdown timed out after ${timeoutMs}ms, forcing exit`); | |
| process.exit(0); | |
| }, timeoutMs); | |
| forceExit.unref(); | |
| const parsedTimeoutMs = Number.parseInt(process.env.AGENTMEMORY_SHUTDOWN_TIMEOUT_MS ?? "", 10); | |
| const timeoutMs = | |
| Number.isFinite(parsedTimeoutMs) && parsedTimeoutMs > 0 ? parsedTimeoutMs : 10_000; | |
| const forceExit = setTimeout(() => { | |
| console.warn(`[agentmemory] Shutdown timed out after ${timeoutMs}ms, forcing exit`); | |
| process.exit(0); | |
| }, timeoutMs); | |
| forceExit.unref(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 419 - 424, The code uses parseInt on
process.env["AGENTMEMORY_SHUTDOWN_TIMEOUT_MS"] to compute timeoutMs but doesn't
validate NaN, which causes setTimeout(NaN) to behave incorrectly; update the
initialization of timeoutMs to parse the env var then validate it's a positive
finite integer (fallback to 10000 on missing, non-numeric, negative, zero, or
NaN), then use that validated timeoutMs when calling setTimeout for forceExit
(referencing timeoutMs, AGENTMEMORY_SHUTDOWN_TIMEOUT_MS, forceExit, and
setTimeout in the change).
|
Thanks @mechanic-Q — the timeout itself is a real improvement and the Two things to fix before merge — both flagged by CodeRabbit's outside-diff review and worth addressing while you're here: 1. Make the shutdown handler idempotent. let shuttingDown = false;
const shutdown = async () => {
if (shuttingDown) return;
shuttingDown = true;
// ... existing body
};2. Validate the env var. const raw = parseInt(process.env["AGENTMEMORY_SHUTDOWN_TIMEOUT_MS"] || "10000", 10);
const timeoutMs = Number.isFinite(raw) && raw > 0 ? raw : 10000;Optional but appreciated: add Happy to merge once those land. |
2 participants
Summary | 概述
Add a shutdown timeout that force-exits the process if graceful shutdown hangs, preventing zombie processes that require manual kill.
添加关闭超时机制，防止进程因 sdk 连接卡死而僵死。
Motivation | 动机
When the iii-engine WebSocket connection is stuck or the viewer server refuses to close, the shutdown handler hangs indefinitely. SIGTERM from systemd times out after 90s and sends SIGKILL, but during those 90s the process is unresponsive. Adding a configurable force-exit timeout ensures clean termination in bounded time.
当 iii-engine 连接卡死或 viewer 服务器拒绝关闭时，shutdown 流程永久挂起，形成僵尸进程。
Changes | 改动
AGENTMEMORY_SHUTDOWN_TIMEOUT_MSenv var (default: 10000ms)process.exit(0)if shutdown exceeds the timeoutunref()so it doesn't prevent normal exit if shutdown completes quicklyclearTimeoutwhen shutdown completes normallyBackwards Compatibility | 向后兼容
Default timeout is 10s. Completely transparent when shutdown works normally — the timer is cleared before firing.
Summary by CodeRabbit