feat: graceful worker #11014
feat: graceful worker #11014
Conversation
📝 WalkthroughWalkthroughThis pull request makes four factual changes: replaces a numeric signal literal with the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
c0471e6 to
650e4d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
bin/worker-mails (1)
3-3: Same concern as bin/worker-messaging.The
execusage is correct. The unquoted$@has the same robustness consideration mentioned in the previous file review.bin/queue-retry (1)
3-3: Same concern as bin/worker-messaging.The
execusage is correct. The unquoted$@has the same robustness consideration mentioned in the previous file review.bin/worker-stats-resources (1)
3-3: Same concern as bin/worker-messaging.The
execusage is correct. The unquoted$@has the same robustness consideration mentioned in the previous file review.bin/test (1)
3-3: Same concern as bin/worker-messaging.The
execusage is correct. The unquoted$@has the same robustness consideration mentioned in the previous file review.bin/worker-migrations (1)
3-3: Same concern as bin/worker-messaging.The
execusage is correct. The unquoted$@has the same robustness consideration mentioned in the previous file review.bin/worker-databases (1)
3-3: Same concern as bin/worker-messaging.The
execusage is correct. The unquoted$@has the same robustness consideration mentioned in the previous file review.bin/worker-functions (1)
3-3: Same concern as bin/worker-messaging.The
execusage is correct. The unquoted$@has the same robustness consideration mentioned in the previous file review.
🧹 Nitpick comments (2)
bin/worker-messaging (1)
3-3: LGTM! Consider quoting$@for robustness.The use of
execto replace the shell process with the PHP process is correct and aligns with the PR's graceful worker shutdown objectives. This ensures signals are delivered directly to the PHP process rather than the intermediate shell.For robustness, consider quoting the argument expansion:
"$@"instead of$@. While this likely works fine for typical CLI usage, unquoted expansion can incorrectly split arguments containing whitespace.Suggested improvement (applies to all similar bin/* scripts)
-exec php /usr/src/code/app/worker.php messaging $@ +exec php /usr/src/code/app/worker.php messaging "$@"bin/queue-count-failed (1)
3-3: LGTM! Consider quoting$@for robustness.The exec replacement is correct. As a minor shell best practice, consider using
"$@"(quoted) to properly handle arguments containing spaces or special characters, though the current usage works for typical invocations.🔎 Optional: Shell best practice for argument forwarding
-exec php /usr/src/code/app/cli.php queue-count --type=failed $@ +exec php /usr/src/code/app/cli.php queue-count --type=failed "$@"This pattern can be applied to all similar wrapper scripts if desired.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
app/http.phpapp/worker.phpbin/doctorbin/installbin/maintenancebin/migratebin/queue-count-failedbin/queue-count-processingbin/queue-count-successbin/queue-retrybin/realtimebin/schedule-executionsbin/schedule-functionsbin/schedule-messagesbin/screenshotbin/sdksbin/specsbin/sslbin/stats-resourcesbin/testbin/upgradebin/varsbin/worker-auditsbin/worker-buildsbin/worker-certificatesbin/worker-databasesbin/worker-deletesbin/worker-functionsbin/worker-mailsbin/worker-messagingbin/worker-migrationsbin/worker-stats-resourcesbin/worker-stats-usagebin/worker-webhookscomposer.json
💤 Files with no reviewable changes (1)
- app/worker.php
🚧 Files skipped from review as they are similar to previous changes (21)
- bin/ssl
- bin/queue-count-success
- bin/queue-count-processing
- bin/worker-webhooks
- bin/maintenance
- composer.json
- bin/realtime
- bin/schedule-executions
- bin/schedule-functions
- app/http.php
- bin/worker-builds
- bin/install
- bin/vars
- bin/worker-stats-usage
- bin/upgrade
- bin/migrate
- bin/worker-deletes
- bin/specs
- bin/schedule-messages
- bin/worker-audits
- bin/doctor
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V1)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: CodeQL
- GitHub Check: scan
🔇 Additional comments (4)
bin/sdks (1)
3-3: LGTM! Clean exec replacement for signal propagation.Using
execcorrectly replaces the shell process with the PHP CLI process, ensuring signals (SIGTERM, SIGINT) reach the PHP process directly for graceful shutdown. This aligns perfectly with the PR's graceful worker objectives.bin/worker-certificates (1)
3-3: LGTM! Worker wrapper correctly updated.The exec replacement ensures the PHP worker process receives shutdown signals directly, enabling graceful termination.
bin/stats-resources (1)
3-3: LGTM! Consistent exec pattern applied.bin/screenshot (1)
3-3: LGTM! Exec pattern correctly applied.
650e4d8 to
374bdbe
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
app/http.phpapp/worker.phpbin/doctorbin/installbin/maintenancebin/migratebin/queue-count-failedbin/queue-count-processingbin/queue-count-successbin/queue-retrybin/realtimebin/schedule-executionsbin/schedule-functionsbin/schedule-messagesbin/screenshotbin/sdksbin/specsbin/sslbin/stats-resourcesbin/testbin/upgradebin/varsbin/worker-auditsbin/worker-buildsbin/worker-certificatesbin/worker-databasesbin/worker-deletesbin/worker-functionsbin/worker-mailsbin/worker-messagingbin/worker-migrationsbin/worker-stats-resourcesbin/worker-stats-usagebin/worker-webhookscomposer.json
💤 Files with no reviewable changes (1)
- app/worker.php
🚧 Files skipped from review as they are similar to previous changes (22)
- bin/specs
- app/http.php
- bin/worker-stats-usage
- bin/maintenance
- bin/queue-count-processing
- bin/queue-count-failed
- bin/worker-builds
- bin/install
- bin/worker-certificates
- bin/schedule-functions
- bin/migrate
- bin/worker-databases
- bin/schedule-executions
- bin/ssl
- bin/schedule-messages
- bin/realtime
- bin/worker-functions
- bin/worker-migrations
- bin/worker-deletes
- bin/screenshot
- bin/worker-mails
- bin/worker-webhooks
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In Appwrite's codebase, Utopia\Queue\Broker\Pool (aliased as BrokerPool) is a readonly class that implements both Publisher and Consumer interfaces, allowing it to be used directly in constructors expecting either interface type. This is extensively used throughout the codebase for queue operations.
Applied to files:
composer.json
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In the Utopia Queue system, the Pool class implements both Publisher and Consumer interfaces, so BrokerPool (Pool) can be passed directly to constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher).
Applied to files:
composer.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V1)
- GitHub Check: Unit Test
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V2)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: scan
🔇 Additional comments (8)
bin/worker-stats-resources (1)
3-3: LGTM! Improves signal handling for graceful shutdown.Using
execreplaces the shell process with the PHP process, ensuring signals (e.g., SIGTERM, SIGINT) are delivered directly to the worker for proper graceful shutdown. This is a best practice for wrapper scripts and aligns with the PR's objective.bin/test (1)
3-3: Excellent change for graceful shutdown support.The use of
execto replace the shell process with the PHP/phpunit process is a best practice that enables:
- Direct signal delivery (SIGTERM, SIGINT) to the PHP process without shell intermediation
- Proper exit code propagation
- Correct PID handling in containerized environments
This change is essential for the graceful worker shutdown feature and aligns with the PR objectives.
bin/queue-retry (1)
3-3: Excellent change for graceful shutdown support.The use of
execto replace the shell process with the PHP process is a best practice that enables:
- Direct signal delivery (SIGTERM, SIGINT) to the PHP process without shell intermediation
- Proper exit code propagation
- Correct PID handling in containerized environments
This change is essential for the graceful worker shutdown feature and aligns with the PR objectives.
bin/upgrade (1)
3-3: Excellent change for graceful shutdown support.The use of
execto replace the shell process with the PHP process is a best practice that enables:
- Direct signal delivery (SIGTERM, SIGINT) to the PHP process without shell intermediation
- Proper exit code propagation
- Correct PID handling in containerized environments
This change is essential for the graceful worker shutdown feature and aligns with the PR objectives.
bin/doctor (1)
3-3: Excellent change for graceful shutdown support.The use of
execto replace the shell process with the PHP process is a best practice that enables:
- Direct signal delivery (SIGTERM, SIGINT) to the PHP process without shell intermediation
- Proper exit code propagation
- Correct PID handling in containerized environments
This change is essential for the graceful worker shutdown feature and aligns with the PR objectives.
bin/queue-count-success (1)
3-3: Excellent change for graceful shutdown support.The use of
execto replace the shell process with the PHP process is a best practice that enables:
- Direct signal delivery (SIGTERM, SIGINT) to the PHP process without shell intermediation
- Proper exit code propagation
- Correct PID handling in containerized environments
This change is essential for the graceful worker shutdown feature and aligns with the PR objectives.
bin/worker-messaging (1)
3-3: Excellent change for graceful shutdown support—particularly critical for worker processes.The use of
execto replace the shell process with the PHP worker process is a best practice that enables:
- Direct signal delivery (SIGTERM, SIGINT) to the worker process without shell intermediation
- Proper exit code propagation
- Correct PID handling in containerized environments
This is especially important for worker processes that need to gracefully complete ongoing tasks before shutdown.
bin/sdks (1)
3-3: Excellent change for graceful shutdown support.The use of
execto replace the shell process with the PHP process is a best practice that enables:
- Direct signal delivery (SIGTERM, SIGINT) to the PHP process without shell intermediation
- Proper exit code propagation
- Correct PID handling in containerized environments
This change is essential for the graceful worker shutdown feature and aligns with the PR objectives.
| #!/bin/sh | ||
|
|
||
| php /usr/src/code/app/cli.php stats-resources $@ | ||
| exec php /usr/src/code/app/cli.php stats-resources $@ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Quote $@ to preserve argument boundaries.
Consistent with other scripts—use "$@" to handle arguments with spaces or special characters correctly.
Proposed fix
-exec php /usr/src/code/app/cli.php stats-resources $@
+exec php /usr/src/code/app/cli.php stats-resources "$@"📝 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.
| exec php /usr/src/code/app/cli.php stats-resources $@ | |
| exec php /usr/src/code/app/cli.php stats-resources "$@" |
🤖 Prompt for AI Agents
In bin/stats-resources at line 3, the exec invocation uses unquoted $@ which can
break argument boundaries for params with spaces or special characters; update
the exec call to use "$@" instead (i.e., replace $@ with "$@") so all passed
arguments are preserved exactly when forwarding to php /usr/src/code/app/cli.php
stats-resources.
| #!/bin/sh | ||
|
|
||
| php /usr/src/code/app/cli.php vars $@ | ||
| exec php /usr/src/code/app/cli.php vars $@ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Quote $@ to preserve argument boundaries.
Using exec is the right approach for signal propagation. However, $@ should be quoted to correctly handle arguments containing spaces or special characters.
Proposed fix
-exec php /usr/src/code/app/cli.php vars $@
+exec php /usr/src/code/app/cli.php vars "$@"📝 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.
| exec php /usr/src/code/app/cli.php vars $@ | |
| exec php /usr/src/code/app/cli.php vars "$@" |
🤖 Prompt for AI Agents
In bin/vars around line 3, the exec invocation uses an unquoted $@ which can
break arguments with spaces or special characters; update the exec command to
pass the positional parameters as a single quoted expansion (i.e., use "$@") so
all arguments are preserved and signal propagation via exec remains intact.
| #!/bin/sh | ||
|
|
||
| php /usr/src/code/app/worker.php audits $@ | ||
| exec php /usr/src/code/app/worker.php audits $@ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Quote $@ to preserve argument boundaries.
Same issue as other scripts—unquoted $@ will break on arguments with spaces.
Proposed fix
-exec php /usr/src/code/app/worker.php audits $@
+exec php /usr/src/code/app/worker.php audits "$@"📝 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.
| exec php /usr/src/code/app/worker.php audits $@ | |
| exec php /usr/src/code/app/worker.php audits "$@" |
🤖 Prompt for AI Agents
In bin/worker-audits around line 3, the script calls exec php
/usr/src/code/app/worker.php audits $@ without quoting $@ which breaks arguments
containing spaces; update the exec invocation to use quoted "$@" so each
original argument is preserved as a separate parameter (i.e., replace $@ with
"$@" in the exec line).
| "utopia-php/pools": "0.8.*", | ||
| "utopia-php/preloader": "0.2.*", | ||
| "utopia-php/queue": "0.11.*", | ||
| "utopia-php/queue": "dev-feat-workerstop as 0.11.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the utopia-php/queue dev branch exists and check its recent activity
echo "=== Checking if the dev-feat-workerstop branch exists ==="
gh api repos/utopia-php/queue/branches/feat-workerstop --jq '.name' 2>&1 || echo "Branch check failed"
echo -e "\n=== Recent commits on feat-workerstop branch ==="
gh api repos/utopia-php/queue/commits?sha=feat-workerstop --jq '.[0:3] | .[] | "\(.commit.author.date) - \(.commit.message | split("\n")[0])"' 2>&1 || echo "Could not fetch commits"
echo -e "\n=== Check if version 0.11.1 tag exists ==="
gh api repos/utopia-php/queue/git/refs/tags/0.11.1 --jq '.ref' 2>&1 || echo "Tag 0.11.1 does not exist (expected for dev alias)"
echo -e "\n=== List recent tags in utopia-php/queue ==="
gh api repos/utopia-php/queue/tags --jq '.[0:5] | .[] | "\(.name)"' 2>&1 || echo "Could not fetch tags"Repository: appwrite/appwrite
Length of output: 503
Consider using a stable version instead of dev branch alias, or clarify why newer releases cannot be used.
The feat-workerstop branch exists and is actively maintained (most recent commit Dec 16, 2025), which is positive for testing. However, there are more recent stable versions available:
- 0.14.0 (latest)
- 0.13.0
- 0.12.1
- 0.12.0
- 0.11.3
Instead of using dev-feat-workerstop as 0.11.1, either:
- Use a recent stable version (e.g.,
^0.14.0or^0.13.0) if the graceful worker stop feature is already included - If this feature only exists on the dev branch, clarify in the PR description why it cannot be found in the recent stable releases and plan the upgrade path
Dev branch dependencies remain mutable and should not reach production.
🤖 Prompt for AI Agents
In composer.json around line 72, the dependency "utopia-php/queue":
"dev-feat-workerstop as 0.11.1" pins a dev branch under an alias; replace it
with a stable release or justify keeping the dev branch: either set the version
to a stable constraint (e.g., "^0.14.0" or "^0.13.0") if the required graceful
worker stop feature exists there, or if the feature only exists on
feat-workerstop, update the PR description to explain why stable releases cannot
be used, add a composer.json comment or developer note documenting this
temporary decision, and ensure composer.lock is committed (or consider forking
and tagging a stable release) so production does not pull a mutable dev branch.
No description provided.