feat: set fixed timeout on each stop promise in shutdown procedure #283
Conversation
|
Warning Rate limit exceeded@hlolli has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes introduce a new Changes
Sequence DiagramsequenceDiagram
participant System
participant ShutdownManager
participant Component1
participant Component2
System->>ShutdownManager: Initiate Shutdown
ShutdownManager->>Component1: Stop with Timeout
ShutdownManager->>Component2: Stop with Timeout
ShutdownManager-->>System: Log Shutdown Results
System->>System: Exit Process
The sequence diagram illustrates the new shutdown process, where the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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)
src/system.ts (3)
699-716: Use consistent logging mechanism instead ofconsole.error.Within
withTimeout, the code usesconsole.errorto log the timeout error, whereas the rest of the system logs using theloglogger. Consider replacingconsole.errorwith the project'slog.erroror another uniform logging approach for consistency.new Promise((_, reject) => setTimeout(() => { - console.error( - `Shutdown Promise timed out after ${timeoutMs}ms: ${description}`, - ); + log.error( + `Shutdown Promise timed out after ${timeoutMs}ms: ${description}`, + ); reject(new Error(`Shutdown Timeout: ${description}`)); }, timeoutMs), ),
719-723: Align function/method type definitions with intended usage.The
fnis defined as(() => any) | undefined;but is generally used as an async or promise-returning function. To ensure type safety, consider enforcing an async function or returning a Promise explicitly in the signature for better clarity.-type ShutdownEntry = { - fn: (() => any) | undefined; +type ShutdownEntry = { + fn?: () => Promise<any> | any; // Accept both async and sync description: string; };
800-810: Assess shutdown concurrency vs. sequential stop.Using
Promise.allfor parallel shutdown may mask dependency issues if a worker depends on another to halt first. If you suspect ordering constraints, switching to sequential shutdown (e.g.,for … await of) might be safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/system.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (2)
src/system.ts (2)
731-736: Consider event emitter removal timing.
eventEmitter.removeAllListeners()is called before shutting down all workers. If any of the shutdown tasks rely on events to complete gracefully, removing listeners prematurely might prevent them from handling shutdown-related events. Verify that none of your active processes require event mechanisms during shutdown.
737-799: Fix spelling ofshutdownProceadures.The key
shutdownProceadureshas a minor spelling mistake. Prefer correcting it toshutdownProceduresfor consistency and clarity.- const shutdownProceadures: ShutdownEntry[] = [ + const shutdownProcedures: ShutdownEntry[] = [Check for missing worker shutdowns.
Ensure that all newly introduced or future workers are added to this array so they also get gracefully stopped. Missing a reference could lead to dangling processes.
✅ Verification successful
Fix spelling and confirm shutdown procedures are complete
The shutdown procedures list is comprehensive and includes all components that require graceful shutdown. The only required change is fixing the spelling:
- const shutdownProceadures: ShutdownEntry[] = [ + const shutdownProcedures: ShutdownEntry[] = [🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for Worker classes and their stop methods echo "=== Worker Classes ===" ast-grep --pattern 'class $_Worker { $$$ }' echo -e "\n=== Worker Instantiations ===" ast-grep --pattern 'const $_Worker = new $_($$$)' echo -e "\n=== Stop Method Implementations ===" ast-grep --pattern 'stop() { $$$ }' echo -e "\n=== Worker-like Variable Declarations ===" rg "const \w+(?:Worker|Watcher|Indexer|Importer|Unbundler|Emitter)" -A 1Length of output: 2687
Script:
#!/bin/bash # Check if DataContentAttributeImporter has a stop method or requires shutdown echo "=== DataContentAttributeImporter Implementation ===" rg -A 10 "class DataContentAttributeImporter" echo -e "\n=== DataContentAttributeImporter Usage ===" rg "DataContentAttributeImporter" --context 3Length of output: 6514
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #283 +/- ##
========================================
Coverage 71.85% 71.85%
========================================
Files 39 39
Lines 9804 9804
Branches 563 563
========================================
Hits 7045 7045
Misses 2755 2755
Partials 4 4 ☔ View full report in Codecov by Sentry. |
hlolli
force-pushed
the
orphan/zombie-killer
branch
from
13d5e8f to
62fdec3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/system.ts (5)
719-722: Improve type safety of ShutdownEntry.Consider making the description required and specifying a more precise return type for the shutdown function.
type ShutdownEntry = { - fn: (() => any) | undefined; + fn: (() => Promise<void>) | undefined; description: string; };
731-732: Make shutdown timeout configurable.Consider moving the hardcoded timeout value to configuration for better flexibility across different environments.
- const stopTimeoutMs = 5000; + const stopTimeoutMs = config.SHUTDOWN_TIMEOUT_MS ?? 5000;
737-737: Fix typo in variable name.Correct the spelling of "Procedures" in the variable name.
- const shutdownProceadures: ShutdownEntry[] = [ + const shutdownProcedures: ShutdownEntry[] = [
800-810: Consider sequential shutdown for critical components.The current implementation shuts down all components in parallel, which might not be ideal for components with dependencies. Consider implementing a sequential shutdown for critical components to ensure proper cleanup.
Example approach:
const criticalProcedures = ['db.stop', 'server.close']; const regularProcedures = shutdownProcedures.filter( proc => !criticalProcedures.includes(proc.description) ); // First, stop critical procedures sequentially for (const proc of shutdownProcedures.filter( proc => criticalProcedures.includes(proc.description) )) { await withTimeout( typeof proc.fn === 'function' ? proc.fn() : Promise.resolve(), stopTimeoutMs, proc.description ).catch((err) => { log.error(`Error in ${proc.description}:`, err); }); } // Then stop remaining procedures in parallel await Promise.all( regularProcedures.map(...) );
806-808: Improve error handling in shutdown procedures.The current error handling simply logs the error and continues. Consider adding metrics or alerting for failed shutdowns, and potentially implementing retry logic for critical components.
).catch((err) => { log.error(`Error in ${description}:`, err); + metrics.shutdownErrorCounter.inc({ component: description }); + if (criticalProcedures.includes(description)) { + process.exit(1); // Force exit on critical component failure + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/system.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
hlolli
force-pushed
the
orphan/zombie-killer
branch
from
62fdec3 to
b733b67
Compare
No description provided.