Conversation
…an active deployment
📝 Walkthrough📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (4)
✨ 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: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1075-1088: Good implementation with room for improvement in timestamp handling.The logic correctly prevents auto-activation of older deployments, which addresses the PR objective well. However, the string comparison of timestamps could be more robust.
Consider using proper date comparison instead of string comparison for better reliability:
- if (!empty($currentStartTime) && !empty($newStartTime) && $currentStartTime > $newStartTime) { + if (!empty($currentStartTime) && !empty($newStartTime)) { + $currentStartDateTime = new \DateTime($currentStartTime); + $newStartDateTime = new \DateTime($newStartTime); + if ($currentStartDateTime > $newStartDateTime) { Console::info('Skipping auto-activation as current deployment started later'); return; + } }This approach:
- Handles timezone differences properly
- Provides better error handling for malformed timestamps
- Is more explicit about date comparison intent
- Guards against potential string comparison edge cases
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(1 hunks)
⏰ 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
✨ Benchmark results
⚡ Benchmark Comparison
|
…rite/appwrite into fix-build-activation-race-condition
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
277-281: Consider optimizing resource reloading frequency.While reloading the resource ensures data freshness, there are four separate reload operations in this method. Consider if some of these could be consolidated or cached temporarily to reduce database queries, especially in high-frequency build scenarios.
For example, you could reload once at key decision points rather than before each status update:
# Instead of reloading before each status update, consider: # 1. Reload once before the auto-activation logic # 2. Cache the result for subsequent status updates within the same executionAlso applies to: 528-532, 1066-1070, 1283-1287
📜 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 (2)
docker-compose.yml(1 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
🧬 Code Graph Analysis (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (2)
src/Appwrite/Event/Database.php (2)
getDocument(107-110)getCollection(85-88)src/Appwrite/Vcs/Comment.php (1)
isEmpty(26-29)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
1100-1100: Avoid using static access to class '\Utopia\CLI\Console' in method 'buildDeployment'. (Clean Code Rules)
(StaticAccess)
⏰ 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 (5)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (5)
277-281: LGTM: Improved consistency and data freshness.The change from
getSequence()togetInternalId()aligns with consistent deployment identification patterns, and reloading the resource ensures fresh data before status comparisons.
528-532: LGTM: Consistent pattern applied.Same improvement as the previous status update - using internal ID for comparison and ensuring resource freshness.
1066-1070: LGTM: Pattern consistently maintained.The deployment status update logic remains consistent with the previous changes.
1084-1104: Well-implemented race condition prevention logic.The timestamp comparison logic correctly prevents activation of deployments that started earlier but finished later than the currently active deployment. The conditional logic is sound:
- Checks if current active deployment started later than the deployment being activated
- AND current active deployment finished earlier than the deployment being activated
- Skips activation in this overlapping scenario
This effectively prevents the race condition described in the PR objectives.
1283-1287: LGTM: Final status update follows established pattern.The failed deployment status update maintains consistency with the other status update locations.
…rite/appwrite into fix-build-activation-race-condition
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1077-1156: Race condition prevention logic implemented correctly.The core race condition fix addresses the PR objective well. The logic properly:
- Reloads resource state to avoid stale data
- Compares deployment timestamps to detect overlapping builds
- Skips auto-activation when an older deployment would override a newer one
- Updates both resource and platform rules consistently
Consider these minor improvements:
- The
buildEndedAtassignment toDateTime::now()(line 1088-1090) might not reflect the actual build end time- The complex timestamp comparison logic could benefit from inline comments explaining the race condition scenario
📜 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 (2)
docker-compose.yml(1 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- docker-compose.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Indexes/Delete.php:40-40
Timestamp: 2025-06-25T06:25:45.054Z
Learning: In Appwrite's database module, the event label for deleting a table index intentionally uses 'update' (e.g., 'databases.[databaseId].tables.[tableId].indexes.[indexId].update') instead of 'delete', to match legacy behavior as seen in previous controller implementations.
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
🧬 Code Graph Analysis (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-97)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
1122-1122: Avoid unused local variables such as '$rulesUpdated'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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). (12)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: Benchmark
🔇 Additional comments (3)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (3)
277-279: Optimized update pattern for deployment status.Good optimization to update only the
latestDeploymentStatusfield instead of the entire resource document. This reduces database overhead and follows a more targeted update approach.
526-528: Consistent optimization pattern applied.This follows the same optimized update pattern as seen elsewhere in the file, updating only the necessary field rather than the entire document.
1062-1064: Optimization pattern completed.This completes the consistent application of the optimized update pattern for deployment status changes throughout the build lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (2)
1090-1090: Potential inconsistency: Document update with partial data.The line updates the deployment with only the
buildEndedAtfield, but the variable$deploymentused afterwards may not reflect this update. Consider using the returned document or updating the variable:-$deployment = $dbForProject->updateDocument('deployments', $deployment->getId(), new Document(['buildEndedAt' => $tentativeEndTime])); +$deployment = $dbForProject->updateDocument('deployments', $deployment->getId(), $deployment->setAttribute('buildEndedAt', $tentativeEndTime));
1122-1129: Remove unused variable flagged by static analysis.The variable
$rulesUpdatedis set but never used, as correctly identified by PHPMD. Since it doesn't serve any functional purpose, it should be removed:-$rulesUpdated = false; -$dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment, &$rulesUpdated) { - $rulesUpdated = true; +$dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment) { $rule = $rule ->setAttribute('deploymentId', $deployment->getId()) ->setAttribute('deploymentInternalId', $deployment->getSequence()); $dbForPlatform->updateDocument('rules', $rule->getId(), $rule); }, $queries);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Indexes/Delete.php:40-40
Timestamp: 2025-06-25T06:25:45.054Z
Learning: In Appwrite's database module, the event label for deleting a table index intentionally uses 'update' (e.g., 'databases.[databaseId].tables.[tableId].indexes.[indexId].update') instead of 'delete', to match legacy behavior as seen in previous controller implementations.
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (2)
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
1122-1122: Avoid unused local variables such as '$rulesUpdated'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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). (4)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Linter
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (3)
278-278: LGTM: Syntax error resolved and implementation improved.The Document constructor now correctly uses the
=>operator in the associative array, resolving the previous syntax error. The approach of updating only thelatestDeploymentStatusfield instead of the entire document is more efficient and reduces potential conflicts.Also applies to: 527-527, 1063-1063, 1284-1284
1077-1102: Well-implemented race condition prevention logic.The new logic effectively prevents activation of deployments that started earlier but finished later than the currently active deployment. The timestamp comparison logic is sound:
- Reloads the resource to get current state
- Retrieves the active deployment information
- Compares build timestamps to detect overlapping builds
- Prevents activation when current deployment is more recent
This addresses the core race condition described in the PR objectives.
1104-1156: Auto-activation logic correctly implements conditional deployment.The updated auto-activation logic properly:
- Only activates when
$activateBuildis true after race condition checks- Updates resource attributes appropriately for both functions and sites
- Handles platform database rules updates consistently
- Maintains proper separation between function and site handling
The implementation follows established patterns and maintains data consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1113-1115: Remove unused variable to clean up the code.The
$rulesUpdatedvariable is declared and set but never used. This creates unnecessary code clutter.Apply this diff to remove the unused variable:
- $rulesUpdated = false; $dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment, &$rulesUpdated) { - $rulesUpdated = true; $dbForPlatform->updateDocument('rules', $rule->getId(), new Document([ 'deploymentId' => $deployment->getId(), 'deploymentInternalId' => $deployment->getSequence(), ])); }, $queries);Also update the use clause to remove the unused reference:
- $dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment, &$rulesUpdated) { + $dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Indexes/Delete.php:40-40
Timestamp: 2025-06-25T06:25:45.054Z
Learning: In Appwrite's database module, the event label for deleting a table index intentionally uses 'update' (e.g., 'databases.[databaseId].tables.[tableId].indexes.[indexId].update') instead of 'delete', to match legacy behavior as seen in previous controller implementations.
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (2)
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
🧬 Code Graph Analysis (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-97)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
1113-1113: Avoid unused local variables such as '$rulesUpdated'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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 (5)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (5)
278-278: LGTM! Excellent refactoring to use the new Document approach.The changes correctly implement the minimal document update pattern, updating only the
latestDeploymentStatusfield instead of modifying the entire resource document. This is more efficient and follows the recommended approach.Also applies to: 527-527, 1061-1061, 1277-1277
859-859: LGTM! Proper implementation of the new Document approach.The adapter and fallbackFile update correctly uses the minimal document pattern, which is more efficient than updating the entire resource document.
1075-1094: Excellent implementation of race condition prevention.The logic correctly addresses the deployment activation race condition by:
- Reloading the resource to ensure fresh data
- Comparing
buildStartedAttimestamps between deployments- Preventing activation of older deployments over newer ones
This implementation aligns perfectly with the PR objectives and prevents the race condition where overlapping deployments could override the active deployment incorrectly.
1094-1149: LGTM! Well-implemented auto-activation logic with new Document approach.The auto-activation logic correctly:
- Uses conditional activation based on the race condition check
- Implements the new Document approach for all resource and rule updates
- Properly handles both function and site resource types
- Updates only the necessary fields for each document type
The implementation is clean and efficient.
1188-1191: LGTM! Consistent use of new Document approach for VCS rule updates.The VCS branch rule updates correctly implement the minimal document pattern, updating only the deployment-related fields. This maintains consistency with the rest of the refactoring.
Also applies to: 1204-1207
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (3)
1075-1075: Address the default value concern.Based on the previous review feedback, setting
activateBuild = trueby default could accidentally activate preview deployments. Consider setting it tofalseby default and only setting totruewhen the deployment hasactivate = trueand there are no race condition issues.
1077-1092: Consider using createdAt for race condition detection.The race condition prevention logic is well-implemented, but based on previous review feedback, consider using
createdAtinstead ofbuildStartedAtfor timestamp comparison to ensure consistency with deployment creation order.The overall approach of reloading the resource and comparing timestamps to prevent activation of older deployments is sound.
1083-1088: Use createdAt instead of buildStartedAt as previously suggested.The previous review feedback specifically recommended using
createdAtfor race condition detection, but this implementation still usesbuildStartedAt. Consider updating to usecreatedAt()for more reliable deployment ordering.- $currentActiveStartTime = $currentActiveDeployment->getAttribute('buildStartedAt', ''); - $deploymentStartTime = $deployment->getAttribute('buildStartedAt', ''); + $currentActiveStartTime = $currentActiveDeployment->getCreatedAt(); + $deploymentStartTime = $deployment->getCreatedAt();
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1113-1113: Remove unused variable.The variable
$rulesUpdatedis set but never used. Consider removing it to clean up the code.- $rulesUpdated = false; - $dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment, &$rulesUpdated) { - $rulesUpdated = true; + $dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (2)
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
🧬 Code Graph Analysis (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-97)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
1113-1113: Avoid unused local variables such as '$rulesUpdated'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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 (4)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (4)
278-278: LGTM: Efficient document update using minimal Document instance.The refactoring to use
new Document(['latestDeploymentStatus' => ...])correctly implements the minimal update approach, updating only the specific field rather than the entire document.
527-527: LGTM: Consistent implementation of minimal Document updates.All instances correctly use the minimal Document approach for efficient database updates.
Also applies to: 859-859, 1061-1061, 1276-1276
1094-1149: LGTM: Correct implementation of minimal Document updates for auto-activation.The auto-activation logic correctly uses minimal
Documentinstances for both resource and platform rule updates, making the operations more efficient.
1203-1206: LGTM: Correct implementation of minimal Document updates for VCS branch rules.The platform rules update correctly uses minimal
Documentinstances for efficient database operations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1190-1193: Fix undefined variable reference.This issue was flagged in previous reviews but remains unfixed. The
$rulevariable is undefined in this catch block scope. Use the$ruleIdvariable instead:- $rule = $dbForPlatform->updateDocument('rules', $rule->getId(), new Document([ + $rule = $dbForPlatform->updateDocument('rules', $ruleId, new Document([
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1116-1123: Remove unused variable.The
$rulesUpdatedvariable is set but never used, as flagged by static analysis. Consider removing it unless it serves a specific debugging purpose:- $rulesUpdated = false; - $dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment, &$rulesUpdated) { - $rulesUpdated = true; + $dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment) { $rule = $dbForPlatform->updateDocument('rules', $rule->getId(), new Document([ 'deploymentId' => $deployment->getId(), 'deploymentInternalId' => $deployment->getSequence(), ])); }, $queries);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (2)
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
🧬 Code Graph Analysis (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (2)
src/Appwrite/Event/Database.php (2)
getCollection(85-88)getDocument(107-110)src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-97)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
1116-1116: Avoid unused local variables such as '$rulesUpdated'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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). (4)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Linter
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (5)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (5)
278-278: LGTM! Efficient partial document updates.The refactoring to use minimal
Documentinstances for updating only thelatestDeploymentStatusfield is a good optimization that reduces database overhead compared to updating entire documents.Also applies to: 527-527, 1061-1061, 1279-1279
859-859: LGTM! Efficient partial document updates for adapter detection.Good refactoring to use minimal
Documentinstances for updating only theadapterandfallbackFilefields when auto-detecting the site configuration.
1075-1096: Verify timestamp choice for race condition detection.The race condition prevention logic is well-implemented, but there's a discrepancy with past review feedback. The code uses
getCreatedAt()timestamps, but a previous review comment suggested usingbuildStartedAttimestamps, and the PR description mentions comparingbuildStartedAttimestamps.Consider whether
buildStartedAtwould be more appropriate for determining deployment precedence:- $currentActiveStartTime = $currentActiveDeployment->getCreatedAt(); - $deploymentStartTime = $deployment->getCreatedAt(); + $currentActiveStartTime = $currentActiveDeployment->getAttribute('buildStartedAt'); + $deploymentStartTime = $deployment->getAttribute('buildStartedAt');
1097-1152: LGTM! Proper activation logic implementation.The activation logic correctly implements deployment activation for both functions and sites using efficient partial document updates. The approach of updating related platform rules through
forEachis appropriate.
1119-1122: LGTM! Consistent rule updates with minimal documents.The platform rule updates correctly follow the same efficient partial document update pattern used throughout the refactoring.
Also applies to: 1144-1147, 1206-1209
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1097-1153: Clean up unused variable.The activation logic correctly uses minimal Document updates throughout. However, the
$rulesUpdatedvariable on line 1116 is set but never used afterward.- $rulesUpdated = false; - $dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment, &$rulesUpdated) { - $rulesUpdated = true; + $dbForPlatform->forEach('rules', function (Document $rule) use ($dbForPlatform, $deployment) { $rule = $dbForPlatform->updateDocument('rules', $rule->getId(), new Document([ 'deploymentId' => $deployment->getId(), 'deploymentInternalId' => $deployment->getSequence(), ])); }, $queries);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (2)
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
1116-1116: Avoid unused local variables such as '$rulesUpdated'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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). (15)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (5)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (5)
278-278: LGTM: Optimized document updates implemented correctly.The minimal Document approach has been properly implemented across all deployment status updates, which improves performance by only updating the necessary fields instead of entire documents.
Also applies to: 527-527, 1061-1061, 1279-1279
859-859: LGTM: Site adapter update optimized.The adapter and fallback file update correctly uses the minimal Document approach while preserving the existing detection logic.
1075-1095: LGTM: Race condition prevention logic correctly implemented.The logic properly addresses the race condition by:
- Defaulting to safe behavior (no activation)
- Using creation timestamps for comparison
- Preventing older deployments from overriding newer active ones
This satisfies the PR objective of fixing build activation race conditions.
1190-1194: LGTM: VCS branch rule updates optimized.The VCS branch rule updates correctly use the minimal Document approach, maintaining consistency with the optimization pattern applied throughout the method.
Also applies to: 1206-1210
1075-1210: Excellent implementation of race condition fix and optimizations.The changes successfully address all the key objectives:
- Race condition prevention: The logic correctly prevents older deployments from overriding newer active ones by comparing creation timestamps
- Performance optimization: Consistent use of minimal Document updates throughout reduces database overhead
- Past feedback integration: All previous review suggestions have been properly implemented
The implementation is thorough, consistent, and maintains backward compatibility while solving the identified race condition issue.
…an active deployment
What does this PR do?
Don't auto-activate deployment which started earlier & ended later than the current active deployment
Test Plan
Need to test on stage
Related PRs and Issues
Checklist