worker: Graceful shutdown on SIGTERM #9890
Conversation
WalkthroughThis update introduces a graceful shutdown mechanism for worker processes by handling the SIGTERM signal in Changes
Sequence Diagram(s)sequenceDiagram
participant ShellScript
participant PHPProcess
participant Worker
participant SwooleProcess
participant SwooleTimer
ShellScript->>PHPProcess: exec php app/worker.php ...
PHPProcess->>Worker: Start worker
Worker->>SwooleProcess: Register SIGTERM handler
Note right of Worker: Worker runs and processes jobs
SwooleProcess-->>Worker: Receive SIGTERM
Worker->>Worker: Log "Stopping worker"
Worker->>Worker: stop()
Worker->>SwooleTimer: clearAll()
Worker->>PHPProcess: Exit gracefully
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/worker.php (1)
1-1:⚠️ Potential issueFix PSR 12 style issues before merging.
The pipeline indicates style issues that need to be resolved:
single_space_around_constructandsingle_blank_line_at_eof.Please run the suggested fix:
vendor/bin/pint🧰 Tools
🪛 GitHub Actions: Linter
[error] 1-1: PSR 12 style issues detected: single_space_around_construct and single_blank_line_at_eof. Run 'vendor/bin/pint' to fix.
🧹 Nitpick comments (14)
bin/queue-count-success (1)
3-3: Nit: Quote the positional parameters
To safely handle arguments containing spaces or special characters, consider using"$@"instead of$@. For example:-exec php /usr/src/code/app/cli.php queue-count --type=success $@ +exec php /usr/src/code/app/cli.php queue-count --type=success "$@"bin/queue-retry (1)
3-3: Nit: Quote the positional parameters
Switching to"$@"preserves any whitespace in arguments. Suggested diff:-exec php /usr/src/code/app/cli.php queue-retry $@ +exec php /usr/src/code/app/cli.php queue-retry "$@"bin/migrate (1)
3-3: Nit: Quote the positional parameters
For robust argument handling, use"$@"as shown:-exec php /usr/src/code/app/cli.php migrate $@ +exec php /usr/src/code/app/cli.php migrate "$@"bin/vars (1)
3-3: Nit: Quote the positional parameters
To avoid splitting arguments incorrectly, prefer"$@"over$@:-exec php /usr/src/code/app/cli.php vars $@ +exec php /usr/src/code/app/cli.php vars "$@"bin/queue-count-failed (1)
3-3: Nit: Quote the positional parameters
Adopt"$@"to safely convey all arguments, including those with spaces:-exec php /usr/src/code/app/cli.php queue-count --type=failed $@ +exec php /usr/src/code/app/cli.php queue-count --type=failed "$@"bin/worker-audits (1)
3-3: Quote$@to preserve argument boundaries
Unquoted$@can lead to word splitting for arguments containing whitespace.-exec php /usr/src/code/app/worker.php audits $@ +exec php /usr/src/code/app/worker.php audits "$@"bin/worker-stats-usage (1)
3-3: Quote$@to preserve argument boundaries
Unquoted$@can lead to word splitting for arguments containing whitespace.-exec php /usr/src/code/app/worker.php stats-usage $@ +exec php /usr/src/code/app/worker.php stats-usage "$@"bin/worker-deletes (1)
3-3: Quote$@to preserve argument boundaries
Unquoted$@can lead to word splitting for arguments containing whitespace.-exec php /usr/src/code/app/worker.php deletes $@ +exec php /usr/src/code/app/worker.php deletes "$@"bin/worker-functions (1)
3-3: Quote$@to preserve argument boundaries
Unquoted$@can lead to word splitting for arguments containing whitespace.-exec php /usr/src/code/app/worker.php functions $@ +exec php /usr/src/code/app/worker.php functions "$@"bin/worker-webhooks (1)
3-3: Quote$@to preserve argument boundaries
Unquoted$@can lead to word splitting for arguments containing whitespace.-exec php /usr/src/code/app/worker.php webhooks $@ +exec php /usr/src/code/app/worker.php webhooks "$@"bin/worker-databases (1)
3-3: Useexecfor proper signal propagation
Replacing the intermediate shell with the PHP worker ensures thatSIGTERMis delivered directly to the PHP process (no orphaned shell). This aligns with your PR’s goal of graceful shutdown.Quote
"$@"to preserve whitespace in arguments
Wrap the positional parameters in quotes ("$@") to avoid word-splitting if any argument contains spaces.bin/worker-stats-resources (1)
3-3: Useexecfor proper signal propagation
Good call replacing the shell with the PHP process—this lets the worker receive termination signals directly and prevents any extra shell layer from interfering.Quote
"$@"to prevent argument splitting
Change$@to"$@"to ensure that each argument, even if it contains spaces, is passed intact.bin/worker-migrations (1)
3-3: Useexecfor direct process replacement
This change ensures the PHP migration worker inherits the script’s PID and receives shutdown signals immediately, supporting your graceful‐shutdown objective.Wrap parameters in
"$@"
Recommend quoting the positional arguments ("$@") to avoid unintended word splitting when arguments include spaces.bin/worker-messaging (1)
3-3: Replace shell with PHP process viaexec
Excellent – usingexechands off control (and signals) to the PHP messaging worker directly, preventing any intermediate shell from hanging around.Prefer
"$@"over$@
Switch to"$@"to securely forward each argument without splitting on whitespace.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
app/worker.php(2 hunks)bin/doctor(1 hunks)bin/install(1 hunks)bin/maintenance(1 hunks)bin/migrate(1 hunks)bin/queue-count-failed(1 hunks)bin/queue-count-processing(1 hunks)bin/queue-count-success(1 hunks)bin/queue-retry(1 hunks)bin/realtime(1 hunks)bin/schedule-executions(1 hunks)bin/schedule-functions(1 hunks)bin/schedule-messages(1 hunks)bin/screenshot(1 hunks)bin/sdks(1 hunks)bin/specs(1 hunks)bin/ssl(1 hunks)bin/stats-resources(1 hunks)bin/test(1 hunks)bin/upgrade(1 hunks)bin/vars(1 hunks)bin/worker-audits(1 hunks)bin/worker-builds(1 hunks)bin/worker-certificates(1 hunks)bin/worker-databases(1 hunks)bin/worker-deletes(1 hunks)bin/worker-functions(1 hunks)bin/worker-mails(1 hunks)bin/worker-messaging(1 hunks)bin/worker-migrations(1 hunks)bin/worker-stats-resources(1 hunks)bin/worker-stats-usage(1 hunks)bin/worker-webhooks(1 hunks)composer.json(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linter
app/worker.php
[error] 1-1: PSR 12 style issues detected: single_space_around_construct and single_blank_line_at_eof. Run 'vendor/bin/pint' to fix.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (25)
bin/test (1)
3-3: Excellent improvement for signal propagation.Using
execensures that signals like SIGTERM are properly propagated to the PHPUnit process instead of being intercepted by the shell. This eliminates the intermediate shell process and aligns perfectly with the PR's graceful shutdown objectives.bin/specs (1)
3-3: Consistent application of exec pattern for proper signal handling.The use of
execensures the PHP CLI process receives signals directly, supporting the graceful shutdown functionality. This change is consistent with the pattern applied across all bin scripts in this PR.bin/queue-count-processing (1)
3-3: Critical improvement for queue operation signal handling.The
execusage is particularly important for queue-related operations, ensuring that queue counting processes can be gracefully interrupted when shutdown signals are received. This directly supports the PR's objective of implementing graceful shutdown for queue workers.bin/schedule-functions (1)
3-3: LGTM! Proper exec usage for signal propagation.The use of
execcorrectly replaces the shell process with the PHP process, ensuring that SIGTERM signals are properly propagated for graceful shutdown. This eliminates the intermediate shell process that could interfere with signal handling.bin/realtime (1)
3-3: LGTM! Critical for realtime service signal handling.The exec command ensures that SIGTERM signals reach the realtime PHP process directly, which is especially important for services that maintain persistent connections and need graceful shutdown handling.
bin/schedule-executions (1)
3-3: LGTM! Consistent implementation for scheduler process.The exec usage follows the established pattern and ensures proper signal propagation for the scheduler process, enabling graceful handling of running executions during shutdown.
bin/install (1)
3-3: LGTM! Completes the consistent signal handling pattern.The exec implementation is consistent with all other bin scripts in this PR, ensuring uniform signal propagation behavior across all CLI entry points. This supports clean interruption of installation processes when needed.
bin/stats-resources (1)
3-3: LGTM! Excellent improvement for signal handling.The use of
execproperly replaces the shell process with the PHP process, ensuring that SIGTERM signals are delivered directly to the PHP process rather than being intercepted by the shell. This directly supports the PR objective of enabling graceful shutdown.bin/doctor (1)
3-3: Consistent and correct implementation.The
execusage properly eliminates the shell wrapper process, ensuring signals reach the PHP process directly. This change is consistent with the systematic approach being applied across all bin scripts.bin/maintenance (1)
3-3: Good continuation of the consistent pattern.The
execimplementation correctly replaces the shell process, supporting the graceful shutdown improvements. The consistency across all bin scripts demonstrates a well-planned systematic change.bin/sdks (1)
3-3: Excellent completion of the systematic improvement.The
execimplementation is correct and completes the consistent pattern across all bin scripts. This systematic approach ensures reliable signal propagation for graceful shutdown across the entire CLI interface.bin/upgrade (1)
3-3: Excellent improvement for signal handling!Adding
execensures that the PHP process replaces the shell process, allowing SIGTERM signals to be sent directly to the PHP process instead of being potentially intercepted by the shell. This is crucial for graceful shutdown in containerized environments.bin/ssl (1)
3-3: Consistent improvement for signal propagation.The addition of
execfollows the same beneficial pattern as other bin scripts, ensuring that SIGTERM signals reach the PHP process directly for graceful shutdown of SSL operations.bin/screenshot (1)
3-3: Good consistency with signal handling improvements.The
execaddition maintains consistency with other bin scripts and ensures screenshot operations can receive and handle SIGTERM signals appropriately for graceful shutdown.bin/schedule-messages (1)
3-3: Perfect completion of the signal handling pattern.The
execaddition is particularly valuable for message scheduling operations, ensuring that long-running scheduling processes can be gracefully terminated when receiving SIGTERM signals. This completes the consistent pattern across all bin scripts.bin/queue-count-success (1)
3-3: Proper exec usage for graceful shutdown
Switching toexec php …correctly replaces the shell with the PHP process, ensuring SIGTERM is delivered directly and not intercepted by an intermediate shell. This aligns with the standardized pattern across other CLI scripts.bin/queue-retry (1)
3-3: Proper exec usage for graceful shutdown
Usingexec php …here ensures the worker retry command will receive termination signals directly without a parent shell lingering. This matches the other updated scripts.bin/migrate (1)
3-3: Proper exec usage for graceful shutdown
Replacing the shell with the PHP migration process viaexecensures clean signal handling and resource usage. Consistent with other CLI utilities.bin/vars (1)
3-3: Proper exec usage for graceful shutdown
The shift toexec php …is correct for replacing the shell process and improving signal propagation for the CLI vars command.bin/queue-count-failed (1)
3-3: Proper exec usage for graceful shutdown
Usingexechere ensures the failed-count script terminates directly under PHP, enabling reliable SIGTERM handling and eliminating the intermediate shell.app/worker.php (2)
21-23: LGTM! Signal handling imports are correctly added.The
Swoole\ProcessandSwoole\Timerimports are necessary for the signal handling functionality and are properly placed with other Swoole imports.
485-494: Excellent graceful shutdown implementation!The signal handler implementation is well-designed:
- Properly registers SIGTERM handler using
Process::signal()- Logs shutdown information for monitoring
- Calls
$worker->stop()to gracefully stop processing- Clears all timers with
Timer::clearAll()to prevent resource leaksThis ensures the worker finishes current events before stopping and prevents new event consumption during shutdown.
bin/worker-builds (1)
3-3: Perfect! Using exec for proper signal propagation.The change from
phptoexec phpis excellent because:
- Replaces the shell process with the PHP process directly
- Ensures SIGTERM signals are properly propagated to the worker
- Eliminates the intermediate shell process that could intercept signals
- Aligns perfectly with the graceful shutdown implementation in
app/worker.phpbin/worker-certificates (1)
3-3: Consistent and correct implementation!The
exec phpchange is properly applied here, maintaining consistency with other worker scripts and ensuring proper signal handling for the certificates worker.bin/worker-mails (1)
3-3:✅ Verification successful
Excellent consistency across worker scripts!
The
exec phpimplementation is correctly applied. This consistent pattern across all worker scripts ensures uniform signal handling behavior.Let's verify that all worker scripts in the bin directory have been consistently updated:
🏁 Script executed:
#!/bin/bash # Description: Check all worker scripts use exec php pattern consistently # Find all worker scripts and check their PHP invocation pattern echo "=== Worker scripts using exec php ===" fd "worker-" bin/ --type f --exec grep -l "exec php" {} echo -e "\n=== Worker scripts NOT using exec php ===" fd "worker-" bin/ --type f --exec grep -L "exec php" {} | xargs -I {} sh -c 'echo "File: {}"; cat "{}"'Length of output: 600
All worker scripts consistently use
exec phpVerified that every
bin/worker-*script invokes PHP withexec php, ensuring uniform signal handling across all workers. No remaining scripts without this pattern.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/worker.php (1)
485-494: Well-implemented graceful shutdown signal handler.The SIGTERM signal handler implementation is clean and follows good practices:
- Properly captures required variables (
$worker,$workerName) in the closure- Logs the shutdown action for visibility
- Calls
$worker->stop()for graceful worker shutdown- Clears all timers with
Timer::clearAll()to prevent resource leaksConsider adding basic error handling around the
$worker->stop()call to ensure the shutdown process is robust even if the stop method encounters issues.Optional enhancement: Add error handling to the shutdown process.
Process::signal(SIGTERM, function () use ($worker, $workerName) { Console::info("Stopping worker $workerName."); - - $worker->stop(); - Timer::clearAll(); + + try { + $worker->stop(); + Timer::clearAll(); + } catch (Throwable $e) { + Console::error("Error during worker shutdown: " . $e->getMessage()); + } });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
app/worker.php(2 hunks)bin/screenshot(1 hunks)composer.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: scan
🔇 Additional comments (2)
bin/screenshot (1)
3-3: LGTM! Excellent improvement for signal handling.The use of
execensures that SIGTERM and other signals are properly propagated to the PHP process instead of being handled by the intermediate shell process. This change aligns perfectly with the PR objective of implementing graceful shutdown handling by ensuring signals reach the actual worker process directly.app/worker.php (1)
21-21: LGTM: Required imports for signal handling functionality.The new imports for
Swoole\ProcessandSwoole\Timerare correctly added and necessary for the SIGTERM signal handling implementation.Also applies to: 23-23
f9e0327 to
762353b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
app/worker.php(2 hunks)bin/doctor(1 hunks)bin/install(1 hunks)bin/maintenance(1 hunks)bin/migrate(1 hunks)bin/queue-count-failed(1 hunks)bin/queue-count-processing(1 hunks)bin/queue-count-success(1 hunks)bin/queue-retry(1 hunks)bin/realtime(1 hunks)bin/schedule-executions(1 hunks)bin/schedule-functions(1 hunks)bin/schedule-messages(1 hunks)bin/screenshot(1 hunks)bin/sdks(1 hunks)bin/specs(1 hunks)bin/ssl(1 hunks)bin/stats-resources(1 hunks)bin/test(1 hunks)bin/upgrade(1 hunks)bin/vars(1 hunks)bin/worker-audits(1 hunks)bin/worker-builds(1 hunks)bin/worker-certificates(1 hunks)bin/worker-databases(1 hunks)bin/worker-deletes(1 hunks)bin/worker-functions(1 hunks)bin/worker-mails(1 hunks)bin/worker-messaging(1 hunks)bin/worker-migrations(1 hunks)bin/worker-stats-resources(1 hunks)bin/worker-stats-usage(1 hunks)bin/worker-webhooks(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- bin/worker-stats-usage
🚧 Files skipped from review as they are similar to previous changes (31)
- bin/worker-builds
- bin/worker-mails
- bin/doctor
- bin/queue-count-processing
- bin/worker-certificates
- bin/ssl
- bin/sdks
- bin/worker-databases
- bin/worker-webhooks
- bin/stats-resources
- bin/test
- bin/install
- bin/maintenance
- bin/worker-functions
- bin/upgrade
- bin/realtime
- bin/vars
- bin/queue-count-failed
- bin/worker-messaging
- bin/worker-deletes
- bin/screenshot
- bin/schedule-messages
- bin/worker-stats-resources
- bin/schedule-functions
- bin/queue-count-success
- bin/worker-audits
- bin/worker-migrations
- bin/migrate
- bin/schedule-executions
- bin/queue-retry
- bin/specs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build SDK
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
- GitHub Check: scan
🔇 Additional comments (1)
app/worker.php (1)
21-21: LGTM: Required imports for signal handling.The imports for
Swoole\ProcessandSwoole\Timerare correctly added to support the graceful shutdown functionality.Also applies to: 23-23
| ->action(function () use ($worker, $workerName) { | ||
| Console::info("Worker $workerName started"); | ||
|
|
||
| Process::signal(SIGTERM, function () use ($worker, $workerName) { | ||
| Console::info("Stopping worker $workerName."); | ||
|
|
||
| $worker->stop(); | ||
| Timer::clearAll(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance signal handler robustness and prevent race conditions.
The signal handler implementation is functionally correct but could benefit from additional safeguards:
- Race condition prevention: Multiple SIGTERM signals could trigger the handler multiple times
- Error handling: No exception handling for
worker->stop()orTimer::clearAll() - Signal handler state: No mechanism to prevent duplicate shutdown attempts
Consider this improved implementation:
- ->action(function () use ($worker, $workerName) {
+ ->action(function () use ($worker, $workerName) {
Console::info("Worker $workerName started");
+
+ $shutdownInProgress = false;
- Process::signal(SIGTERM, function () use ($worker, $workerName) {
+ Process::signal(SIGTERM, function () use ($worker, $workerName, &$shutdownInProgress) {
+ if ($shutdownInProgress) {
+ return; // Prevent duplicate shutdown attempts
+ }
+ $shutdownInProgress = true;
+
Console::info("Stopping worker $workerName.");
- $worker->stop();
- Timer::clearAll();
+ try {
+ $worker->stop();
+ Timer::clearAll();
+ } catch (Throwable $e) {
+ Console::error("Error during graceful shutdown: " . $e->getMessage());
+ }
});
});📝 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.
| ->action(function () use ($worker, $workerName) { | |
| Console::info("Worker $workerName started"); | |
| Process::signal(SIGTERM, function () use ($worker, $workerName) { | |
| Console::info("Stopping worker $workerName."); | |
| $worker->stop(); | |
| Timer::clearAll(); | |
| }); | |
| }); | |
| ->action(function () use ($worker, $workerName) { | |
| Console::info("Worker $workerName started"); | |
| $shutdownInProgress = false; | |
| Process::signal(SIGTERM, function () use ($worker, $workerName, &$shutdownInProgress) { | |
| if ($shutdownInProgress) { | |
| return; // Prevent duplicate shutdown attempts | |
| } | |
| $shutdownInProgress = true; | |
| Console::info("Stopping worker $workerName."); | |
| try { | |
| $worker->stop(); | |
| Timer::clearAll(); | |
| } catch (Throwable $e) { | |
| Console::error("Error during graceful shutdown: " . $e->getMessage()); | |
| } | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In app/worker.php around lines 485 to 494, improve the SIGTERM signal handler by
adding a flag to prevent multiple executions and wrap the shutdown logic in a
try-catch block to handle exceptions from worker->stop() and Timer::clearAll().
Initialize a boolean variable outside the handler to track if shutdown has
started, check this flag at the beginning of the handler to avoid duplicate
shutdown attempts, and log any exceptions caught during the shutdown process to
enhance robustness and prevent race conditions.
Installs a Signal handler that stops the worker on
SIGTERM. Updates queue library to fix shutdown handling.This makes sure we finish consuming the current event and stop consuming new events.
Update all
binfiles to useexecto ensure the shutdown signal is not swallowed by bash.Summary by CodeRabbit
New Features
Refactor