Support for sync-ing certificate generation status #10985
Conversation
📝 WalkthroughWalkthroughThis PR adds a new MaintenanceRules platform task that runs periodic domain verification and certificate renewal loops, new environment variables and a maintenance-rules CLI entry, and a new docker-compose service for the task. It removes the certificate renewal logic from the existing Maintenance task, registers MaintenanceRules in the Tasks service, and updates the Certificates worker to split handling into domain verification and certificate generation flows. It also adds certificate-related interface methods and exception, adds new certificate action constants on the Certificate event, and moves the Action class namespace with corresponding import updates across proxy rule modules. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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! |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Platform/Services/Tasks.php (1)
1-1: Fix import ordering.The pipeline indicates an
ordered_importsformatting issue. Please run the formatter to resolve this.
🧹 Nitpick comments (7)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (1)
112-117: Consider setting the action explicitly for consistency.When queueing certificates, other rule creation endpoints (API/Create, Function/Create, Site/Create, Redirect/Create) explicitly set the action to
Certificate::ACTION_GENERATION. While the default action isACTION_GENERATION, explicitly setting it here would improve consistency and make the intent clearer.🔎 Apply this diff for consistency:
$queueForCertificates ->setDomain(new Document([ 'domain' => $rule->getAttribute('domain'), 'domainType' => $rule->getAttribute('deploymentResourceType', $rule->getAttribute('type')), ])) + ->setAction(Certificate::ACTION_GENERATION) ->trigger();.env (1)
104-105: Consider increasing the domain verification interval.The 60-second interval for domain verification (
_APP_MAINTENANCE_RULE_DOMAIN_VERIFICATION_INTERVAL=60) may be too aggressive and could lead to excessive load, especially in environments with many domains. Consider starting with a higher value (e.g., 300-600 seconds) and adjusting based on operational needs.src/Appwrite/Certificates/Exception/CertificateStatus.php (1)
7-10: Consider extending Appwrite's custom Exception class.For consistency with the rest of the codebase, consider extending
Appwrite\Extend\Exceptioninstead of the base PHPException. The custom exception class provides structured error typing, metadata, and better error handling capabilities.🔎 Apply this diff to use the custom Exception class:
<?php namespace Appwrite\Certificates\Exceptions; -use Exception; +use Appwrite\Extend\Exception; // Exception thrown during certificate status retrieval class CertificateStatus extends Exception { + public function __construct(string $message = '', int $code = 0, ?\Throwable $previous = null) + { + parent::__construct(Exception::GENERAL_SERVER_ERROR, $message, $code, $previous); + } }src/Appwrite/Certificates/Adapter.php (1)
11-14: LGTM with a suggestion.The new interface methods are well-designed and extend the adapter contract appropriately. However, consider whether
getCertificateStatus()should return a more structured type (e.g., an enum or class constant) instead of a raw string to ensure consistency across implementations.docker-compose.yml (1)
790-827: Consider adding_APP_LOGGING_CONFIGfor consistency.Other similar task services (e.g.,
appwrite-task-maintenance,appwrite-task-stats-resources) include the_APP_LOGGING_CONFIGenvironment variable. Adding it toappwrite-task-maintenance-ruleswould enable consistent logging configuration across all maintenance tasks.🔎 Suggested addition:
- _APP_DB_USER - _APP_DB_PASS - _APP_DATABASE_SHARED_TABLES + - _APP_LOGGING_CONFIG - _APP_MAINTENANCE_RULE_DOMAIN_VERIFICATION_INTERVAL - _APP_MAINTENANCE_RULE_CERTIFICATE_RENEWAL_INTERVALsrc/Appwrite/Platform/Tasks/MaintenanceRules.php (1)
106-111: RedundantQuery::limit(1)withfindOne.The
findOnemethod already limits results to 1, makingQuery::limit(1)redundant. This appears in bothrenewCertificates(line 110) andverifyDomain(line 153, though not shown in this file).🔎 Suggested fix:
$rule = $isMd5 ? $dbForPlatform->getDocument('rules', md5($domain)) : $dbForPlatform->findOne('rules', [ Query::equal('domain', [$domain]), - Query::limit(1) ]);src/Appwrite/Platform/Workers/Certificates.php (1)
149-154: RedundantQuery::limit(1)withfindOne.Similar to the issue in
MaintenanceRules.php,Query::limit(1)is redundant when usingfindOne.🔎 Suggested fix:
$rule = System::getEnv('_APP_RULES_FORMAT') === 'md5' ? ValidatorAuthorization::skip(fn () => $dbForPlatform->getDocument('rules', md5($domain->get()))) : ValidatorAuthorization::skip(fn () => $dbForPlatform->findOne('rules', [ Query::equal('domain', [$domain->get()]), - Query::limit(1), ]));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.env(1 hunks)Dockerfile(1 hunks)bin/maintenance-rules(1 hunks)docker-compose.yml(3 hunks)src/Appwrite/Certificates/Adapter.php(1 hunks)src/Appwrite/Certificates/Exception/CertificateStatus.php(1 hunks)src/Appwrite/Certificates/LetsEncrypt.php(2 hunks)src/Appwrite/Event/Certificate.php(3 hunks)src/Appwrite/Platform/Modules/Proxy/Action.php(1 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php(2 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php(2 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Get.php(1 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php(2 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php(2 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php(1 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/XList.php(1 hunks)src/Appwrite/Platform/Services/Tasks.php(2 hunks)src/Appwrite/Platform/Tasks/Maintenance.php(0 hunks)src/Appwrite/Platform/Tasks/MaintenanceRules.php(1 hunks)src/Appwrite/Platform/Workers/Certificates.php(10 hunks)
💤 Files with no reviewable changes (1)
- src/Appwrite/Platform/Tasks/Maintenance.php
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-08T01:20:14.364Z
Learnt from: stnguyen90
Repo: appwrite/appwrite PR: 10119
File: app/controllers/api/account.php:1226-1232
Timestamp: 2025-07-08T01:20:14.364Z
Learning: In Appwrite, `_APP_DOMAIN` is a required environment variable that must always be set for the system to function properly.
Applied to files:
docker-compose.yml
📚 Learning: 2025-10-23T08:06:38.889Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10546
File: src/Appwrite/Platform/Workers/Migrations.php:144-148
Timestamp: 2025-10-23T08:06:38.889Z
Learning: In the Appwrite codebase, migration workers receive already-validated data from queued jobs. Query validation using Query::parseQueries() happens at the API endpoint level (with try-catch for QueryException) before jobs are queued, so workers in src/Appwrite/Platform/Workers/Migrations.php don't need redundant validation.
Applied to files:
src/Appwrite/Platform/Services/Tasks.phpsrc/Appwrite/Platform/Workers/Certificates.php
📚 Learning: 2025-11-12T04:07:12.576Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154
Timestamp: 2025-11-12T04:07:12.576Z
Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
Applied to files:
src/Appwrite/Platform/Workers/Certificates.php
🧬 Code graph analysis (12)
src/Appwrite/Certificates/Adapter.php (1)
src/Appwrite/Certificates/LetsEncrypt.php (2)
isInstantGeneration(88-91)getCertificateStatus(93-96)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php (2)
src/Appwrite/Platform/Modules/Proxy/Action.php (1)
Action(17-188)src/Appwrite/Event/Certificate.php (2)
setAction(102-106)Certificate(8-134)
src/Appwrite/Certificates/Exception/CertificateStatus.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-472)
src/Appwrite/Certificates/LetsEncrypt.php (2)
src/Appwrite/Certificates/Exception/CertificateStatus.php (1)
CertificateStatus(8-10)src/Appwrite/Certificates/Adapter.php (2)
isInstantGeneration(11-11)getCertificateStatus(13-13)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php (2)
src/Appwrite/Platform/Modules/Proxy/Action.php (1)
Action(17-188)src/Appwrite/Event/Certificate.php (2)
setAction(102-106)Certificate(8-134)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (2)
src/Appwrite/Platform/Modules/Proxy/Action.php (1)
Action(17-188)src/Appwrite/Event/Certificate.php (2)
setAction(102-106)Certificate(8-134)
src/Appwrite/Event/Certificate.php (5)
src/Appwrite/Platform/Workers/Certificates.php (1)
action(79-121)src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php (1)
action(73-137)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (1)
action(78-155)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php (1)
action(81-159)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php (1)
action(78-155)
src/Appwrite/Platform/Tasks/MaintenanceRules.php (2)
src/Appwrite/Platform/Services/Tasks.php (2)
Tasks(24-49)__construct(26-48)src/Appwrite/Event/Certificate.php (3)
Certificate(8-134)setDomain(32-37)setAction(102-106)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (1)
src/Appwrite/Platform/Modules/Proxy/Action.php (1)
Action(17-188)
src/Appwrite/Platform/Services/Tasks.php (1)
src/Appwrite/Platform/Tasks/MaintenanceRules.php (2)
MaintenanceRules(15-126)getName(17-20)
src/Appwrite/Platform/Workers/Certificates.php (2)
src/Appwrite/Event/Certificate.php (4)
Certificate(8-134)__construct(17-24)setDomain(32-37)setAction(102-106)src/Appwrite/Certificates/Adapter.php (2)
issueCertificate(9-9)isInstantGeneration(11-11)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php (1)
src/Appwrite/Event/Certificate.php (2)
setAction(102-106)Certificate(8-134)
🪛 dotenv-linter (4.0.0)
.env
[warning] 104-104: [UnorderedKey] The _APP_MAINTENANCE_RULE_DOMAIN_VERIFICATION_INTERVAL key should go before the _APP_MAINTENANCE_START_TIME key
(UnorderedKey)
[warning] 105-105: [UnorderedKey] The _APP_MAINTENANCE_RULE_CERTIFICATE_RENEWAL_INTERVAL key should go before the _APP_MAINTENANCE_RULE_DOMAIN_VERIFICATION_INTERVAL key
(UnorderedKey)
🪛 GitHub Actions: Linter
src/Appwrite/Certificates/Exception/CertificateStatus.php
[error] 1-1: Pint: PSR-12 formatting issue detected (likely single_blank_lines).
src/Appwrite/Platform/Services/Tasks.php
[error] 1-1: Pint: PSR-12 formatting issue detected (ordered_imports).
🪛 PHPMD (2.15.0)
src/Appwrite/Certificates/LetsEncrypt.php
88-88: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
88-88: Avoid unused parameters such as '$domainType'. (undefined)
(UnusedFormalParameter)
93-93: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
93-93: Avoid unused parameters such as '$domainType'. (undefined)
(UnusedFormalParameter)
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (25)
src/Appwrite/Platform/Modules/Proxy/Action.php (1)
3-3: LGTM! Namespace reorganization is consistent.The Action class has been moved to a broader namespace (from
Appwrite\Platform\Modules\Proxy\Http\RulestoAppwrite\Platform\Modules\Proxy), which aligns with its usage across multiple proxy rule modules.src/Appwrite/Event/Certificate.php (3)
10-13: LGTM! Action constants and property follow good practices.The introduction of action constants (
ACTION_DOMAIN_VERIFICATIONandACTION_GENERATION) enables the certificate worker to distinguish between different certificate operations. The default value ofACTION_GENERATIONmaintains backward compatibility with existing code paths.
96-116: LGTM! Accessor methods are well-documented.The setter and getter methods for the action property are properly implemented with clear documentation and fluent interface support.
130-131: LGTM! Action properly included in event payload.The action is correctly added to the payload preparation, ensuring the certificate worker receives the intended action type.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (1)
8-8: LGTM! Import path updated consistently.The import now references the Action class from its new namespace location.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/XList.php (1)
6-6: LGTM! Import added for Action class usage.The import is necessary for extending the Action class and using its constants.
bin/maintenance-rules (1)
1-3: LGTM! Script follows established patterns.The maintenance-rules wrapper script follows the same pattern as other executable scripts in the bin directory, properly forwarding arguments to the CLI entrypoint.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php (2)
8-8: LGTM! Import path updated consistently.The import now references the Action class from its new namespace location.
128-128: LGTM! Action explicitly set for certificate generation.Setting the action to
ACTION_GENERATIONmakes the intent clear and aligns with the new action-based certificate handling pattern.Dockerfile (1)
61-61: LGTM! Execute permission properly added.The maintenance-rules script is correctly given execute permission alongside other executable scripts.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Get.php (1)
6-6: LGTM! Import added for Action class usage.The import is necessary for extending the Action class and using its constants, consistent with the namespace reorganization.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (1)
8-8: LGTM!The namespace refactoring and explicit action setting for certificate generation are correctly implemented. The use of
Certificate::ACTION_GENERATIONproperly distinguishes this from domain verification workflows.Also applies to: 146-146
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php (1)
8-8: LGTM!Consistent with other rule creation paths. The namespace refactoring and explicit certificate generation action are correctly implemented.
Also applies to: 150-150
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php (1)
8-8: LGTM!Consistent implementation across all rule creation endpoints. The changes properly support the new action-based certificate workflow.
Also applies to: 146-146
src/Appwrite/Certificates/LetsEncrypt.php (1)
5-5: LGTM!The implementation of the new interface methods is appropriate for Let's Encrypt:
isInstantGeneration()returningtruecorrectly reflects that Let's Encrypt certificates are generated synchronously.getCertificateStatus()appropriately throws an exception since status retrieval is not applicable for Let's Encrypt.Note: The static analysis warnings about unused parameters are false positives—these parameters are required by the
Adapterinterface contract.Also applies to: 88-96
src/Appwrite/Platform/Services/Tasks.php (1)
10-10: LGTM!The registration of the
MaintenanceRulestask follows the established pattern and is correctly implemented.Also applies to: 33-33
src/Appwrite/Certificates/Adapter.php (1)
11-14: All Certificates/Adapter implementations include the required methods.The only class implementing the Certificates/Adapter interface is
LetsEncrypt, and it includes bothisInstantGeneration()(line 88) andgetCertificateStatus()(line 93) implementations. Verification is complete.docker-compose.yml (1)
558-558: LGTM!Adding
_APP_DOMAIN_SITESto the certificates worker environment is consistent with the new domain verification logic that needs to handle site domains.src/Appwrite/Platform/Tasks/MaintenanceRules.php (3)
52-68: LGTM on query structure.The domain verification query correctly filters by creation time, status, region, and orders by update time to prioritize longest-waiting rules. The 30-rule limit is reasonable for processing within the default 60-second interval.
39-49: Verify connection resilience strategy for long-running maintenance loops.The
$dbForPlatformconnection is captured once in closures that run indefinitely viaConsole::loop(running every 60 seconds for domain verification and every 86400 seconds for certificate renewal). While clients that remain idle for a long period can have connections closed by the server and may need to reconnect, this pattern is used consistently across multiple Appwrite maintenance tasks (Maintenance.php, StatsResources.php, ScheduleMessages.php).Verify whether:
- The database adapter has built-in stale connection handling (e.g., health checks via
ping())- Connection pooling (utopia-php/pools) automatically manages reconnection
- The frequent queries within each loop iteration keep connections alive
If none of these mechanisms are in place, consider adding explicit connection health checks or refreshing the connection within each loop iteration.
72-80: Verify thatprojectcontext is not required for certificate queue.The
queueForCertificateis triggered without callingsetProject()(lines 73 and 120). TheCertificateevent class includes'project' => $this->projectin itspreparePayload()method, which will benullin this context. Other maintenance tasks in the codebase explicitly callsetProject()(e.g.,Maintenance.php,ScheduleExecutions.php), butMaintenanceRules.phpdoes not. This appears to be intentional for system-level maintenance operations, but requires verification against the certificates worker implementation to confirm it does not require the project field.src/Appwrite/Platform/Workers/Certificates.php (4)
107-121: LGTM on action dispatching.The switch statement cleanly separates domain verification from certificate generation actions, with proper error handling for invalid actions.
360-377: LGTM on upsert logic.The certificate upsert correctly handles both create and update scenarios, with proper attribute merging that allows new values to override existing ones.
395-438: LGTM on event propagation.The method correctly skips event emission for the console project and properly chains events through webhooks, functions, and realtime using the established
from()pattern.
470-478: LGTM on main domain helper.Clean extraction of domain validation logic with proper handling of empty and localhost cases.
| // Rule not found (or) not in the expected state | ||
| if ($rule->isEmpty() || $rule->getAttribute('status') !== RULE_STATUS_CERTIFICATE_GENERATING) { | ||
| Console::warning('Certificate generation for ' . $domain->get() . ' is skipped as the associated rule is either empty or not in the expected state.'); | ||
| } |
There was a problem hiding this comment.
Missing return statement after status check causes continued execution on invalid rules.
When the rule is empty or not in the expected state, a warning is logged but execution continues. This will cause errors when trying to access $rule->getAttribute('certificateId') on an empty document, or attempting operations on rules in unexpected states.
🔎 Apply this fix:
// Rule not found (or) not in the expected state
if ($rule->isEmpty() || $rule->getAttribute('status') !== RULE_STATUS_CERTIFICATE_GENERATING) {
Console::warning('Certificate generation for ' . $domain->get() . ' is skipped as the associated rule is either empty or not in the expected state.');
+ return;
}📝 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.
| // Rule not found (or) not in the expected state | |
| if ($rule->isEmpty() || $rule->getAttribute('status') !== RULE_STATUS_CERTIFICATE_GENERATING) { | |
| Console::warning('Certificate generation for ' . $domain->get() . ' is skipped as the associated rule is either empty or not in the expected state.'); | |
| } | |
| // Rule not found (or) not in the expected state | |
| if ($rule->isEmpty() || $rule->getAttribute('status') !== RULE_STATUS_CERTIFICATE_GENERATING) { | |
| Console::warning('Certificate generation for ' . $domain->get() . ' is skipped as the associated rule is either empty or not in the expected state.'); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Workers/Certificates.php around lines 264 to 267, the
code logs a warning when $rule is empty or not in
RULE_STATUS_CERTIFICATE_GENERATING but does not stop execution; add an early
return immediately after the Console::warning(...) so the function exits when
the rule is invalid, preventing subsequent access to
$rule->getAttribute('certificateId') and other operations on an empty or
incorrectly-stated rule.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Certificates/Exception/CertificateStatus.php(1 hunks)src/Appwrite/Platform/Services/Tasks.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Appwrite/Platform/Services/Tasks.php (1)
src/Appwrite/Platform/Tasks/MaintenanceRules.php (2)
MaintenanceRules(15-126)getName(17-20)
src/Appwrite/Certificates/Exception/CertificateStatus.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-472)
⏰ 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). (14)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Platform/Services/Tasks.php (2)
8-8: LGTM! Import correctly added.The import for
MaintenanceRulesis correctly placed in alphabetical order and follows the existing pattern.
33-33: LGTM! Action registration correctly added.The
MaintenanceRulestask is properly registered and maintains alphabetical ordering in the action list. The registration pattern is consistent with other tasks in the service.
Related SER-331