Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis diff replaces Swoole coroutine Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR migrates all usages of Swoole's Key changes:
Confidence Score: 3/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $groupedDeployments = $dbForProject->find('deployments', [ | ||
| Query::equal('resourceType', [RESOURCE_TYPE_FUNCTIONS]), | ||
| Query::groupBy(['resourceInternalId']), | ||
| Query::count('*', 'deploymentsCount'), | ||
| Query::sum('sourceSize', 'deploymentsStorage'), | ||
| Query::sum('buildSize', 'buildsStorage'), | ||
| ]); |
There was a problem hiding this comment.
groupBy find() lacks pagination — silently drops data for large projects
The old implementation used foreachDocument(), which pages through all results automatically and processes every function's deployments. The new find() call with groupBy will be subject to the database's default result limit (typically 5000). For projects that have more distinct resourceInternalId values than this limit, stats for the remaining functions will be silently zeroed out in the next stats run.
The same issue applies to:
countForSites()at the$groupedDeploymentsquery (line ~403)- The
$runtimeGroupsquery (line ~368) - The
$frameworkGroupsquery incountForSites()(line ~405)
Consider either using foreachDocument again (with the groupBy queries as the inner query) or adding an explicit limit with a pagination loop, similar to the original approach.
| try { | ||
| $files = $dbForProject->count('bucket_' . $bucket->getSequence()); | ||
| $bucketResults = Promise::map([ | ||
| 'files' => fn () => $dbForProject->count('bucket_' . $bucket->getSequence()), | ||
| 'storage' => fn () => $dbForProject->sum('bucket_' . $bucket->getSequence(), 'sizeActual'), | ||
| ])->await(); | ||
| } catch (Throwable $th) { | ||
| call_user_func_array($this->logError, [$th, "StatsResources", "count_for_bucket_{$bucket->getSequence()}"]); | ||
| return; | ||
| } |
There was a problem hiding this comment.
try/catch loses error-source granularity
The original code had two separate try/catch blocks — one for count (labelled "count_for_bucket_...") and one for sum (labelled "sum_for_bucket_..."). Now both operations are covered by a single catch block that always logs "count_for_bucket_...", regardless of whether the failure came from count or sum. This makes it harder to pinpoint the failing operation from logs.
} catch (Throwable $th) {
call_user_func_array($this->logError, [$th, "StatsResources", "bucket_{$bucket->getSequence()}"]);
return;
}Using a more neutral label (e.g. "bucket_...") would at least avoid the misleading implication that the error always originates from the count operation.
# Conflicts: # src/Appwrite/Databases/TransactionState.php # src/Appwrite/Platform/Workers/Deletes.php # src/Appwrite/Platform/Workers/StatsResources.php
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/Appwrite/Databases/TransactionState.php (1)
175-175: Consider filtering out existingQuery::select()before appending the$idprojection.If the caller passes a
Query::select()within$queries, appending anotherQuery::select(['$id'])will result in the appended selection being silently ignored by utopia-php/database (only the first select clause is processed). This means the optimization to fetch only IDs may not take effect, and you'll retrieve additional fields unnecessarily.While this doesn't break functionality, it defeats the performance intent. To ensure consistent ID-only retrieval, filter out any existing
TYPE_SELECTqueries before appending the new one:♻️ Proposed refactor
-$committedDocs = $this->dbForProject->find($collectionId, \array_merge($queries, [Query::select(['$id'])])); +$idOnlyQueries = []; +foreach ($queries as $query) { + if ($query->getMethod() !== Query::TYPE_SELECT) { + $idOnlyQueries[] = $query; + } +} +$idOnlyQueries[] = Query::select(['$id']); +$committedDocs = $this->dbForProject->find($collectionId, $idOnlyQueries);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Databases/TransactionState.php` at line 175, The current call building $committedDocs uses array_merge($queries, [Query::select(['$id'])]) which can be silently overridden if $queries already contains a Query::select; update the logic in TransactionState (the method that calls $this->dbForProject->find with $collectionId and $queries) to first filter out any existing Query::TYPE_SELECT entries from the $queries array, then append Query::select(['$id']) and pass that cleaned array to find so the ID-only projection is guaranteed; reference Query::select, Query::TYPE_SELECT, $queries, $committedDocs and $this->dbForProject->find when locating the change.src/Appwrite/Platform/Modules/Functions/Workers/Screenshots.php (1)
155-195: Avoid shared error state across concurrent captures.Both closures mutate the same
$screenshotError, so if light and dark capture fail differently the thrown exception depends on completion order and loses which variant failed. Returning per-task error metadata keeps failures deterministic and easier to debug.♻️ Proposed refactor
- $screenshotError = null; - $screenshots = Promise::map(\array_map(function ($key) use ($configs, $apiKey, $site, $client, &$screenshotError) { - return function () use ($key, $configs, $apiKey, $site, $client, &$screenshotError) { + $screenshots = Promise::map(\array_map(function ($key) use ($configs, $apiKey, $site, $client) { + return function () use ($key, $configs, $apiKey, $site, $client) { try { $config = $configs[$key]; @@ - return ['key' => $key, 'screenshot' => $screenshot]; + return ['key' => $key, 'screenshot' => $screenshot, 'error' => null]; } catch (\Throwable $th) { - $screenshotError = $th->getMessage(); - return; + return ['key' => $key, 'screenshot' => null, 'error' => $th->getMessage()]; } }; }, \array_keys($configs)))->await(); - if (!\is_null($screenshotError)) { - throw new \Exception($screenshotError); + foreach ($screenshots as $result) { + if ($result['error'] !== null) { + throw new \Exception("{$result['key']} capture failed: {$result['error']}"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Modules/Functions/Workers/Screenshots.php` around lines 155 - 195, The code uses a shared $screenshotError mutated inside each async closure (in Promise::map) which creates a race and nondeterministic failure reporting; change each closure (the one created over array_keys($configs)) to return per-task status (e.g., ['key'=> $key, 'screenshot'=> $screenshot, 'error'=> null] on success or ['key'=> $key, 'screenshot'=> null, 'error'=> $th->getMessage()] on failure) instead of writing to $screenshotError, then after $screenshots = Promise::map(...)->await() iterate the returned results to detect any entries with a non-null 'error' and throw a deterministic exception (or aggregate errors) referencing the failing key; remove the shared $screenshotError variable and update the post-processing check that currently inspects $screenshotError.app/controllers/api/messaging.php (1)
2920-2929: Batch-hydrate subscribers instead of parallelizing the N+1.
Promise::map()executes all subscriber closures concurrently, so this path still does per-subscribertargetsanduserslookups, just as a DB burst instead of sequentially. Collect the distinct target IDs and user IDs, fetch them with two bulk queries, and enrich in memory; that keeps latency predictable without scaling connection pressure with page size. (github.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/messaging.php` around lines 2920 - 2929, The current use of Promise::map with per-subscriber closures (the block that calls $dbForProject->getDocument('targets', $subscriber->getAttribute('targetId')) and ->getDocument('users', $target->getAttribute('userId')) for each $subscriber) causes an N+1 burst of DB requests; instead collect distinct target IDs and user IDs from $subscribers, perform two bulk fetches for targets and users via $dbForProject (or its bulk-get API), build lookup maps by id, and then iterate $subscribers once to setAttribute('target', $targetLookup[$id]) and setAttribute('userName', $userLookup[$userId]->getAttribute('name')); remove Promise::map concurrency for this enrichment so lookups are served from memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Platform/Workers/Deletes.php`:
- Around line 1091-1099: The deletion of deployments (via deleteByGroup with the
callback that appends to $deploymentInternalIds and calls
deleteDeploymentFiles/deleteBuildFiles) races with the runtime cleanup branch
that re-queries the deployments collection, causing missed runtimes; modify the
runtime cleanup to use the captured $deploymentInternalIds populated in the
deleteByGroup callback instead of re-reading the collection (or run the runtime
cleanup sequentially after the deleteByGroup completes) so runtime removal
operates on the exact set of deleted deployment IDs and not a concurrent query
result.
- Around line 1014-1017: The site-scoped cleanup queries are using the singular
'site' for the resourceType filter which prevents matching stored plural
resourceType values; update the Query::equal calls inside the deleteByGroup
callbacks (notably the variables and repositories cleanup closures that call
deleteByGroup and the repository-related VCS comment cleanup) to use 'sites'
instead of 'site' (e.g., replace Query::equal('resourceType', ['site']) with
Query::equal('resourceType', ['sites']) in the deleteByGroup invocations and any
related repository callback filters).
- Around line 1085-1089: The cleanup queries use the singular 'function' for
resourceType which violates the plural convention and causes leftover records;
update the Query::equal('resourceType', [...]) calls in the delete closures to
use 'functions' (e.g., the lambda passed to $this->deleteByGroup for 'variables'
and the similar deleteByGroup call for repositories/deployments) so both
occurrences use 'functions' instead of 'function' to match the other branches
and the required plural resourceType format.
In `@src/Appwrite/Platform/Workers/StatsResources.php`:
- Around line 347-365: groupBy on 'resourceInternalId' only returns resources
with >=1 deployment so resources that dropped to zero keep stale metrics; fix by
first building a map from $groupedDeployments (keyed by resourceInternalId with
deploymentsCount, deploymentsStorage, buildsStorage) and then iterate the
current functions collection (the same place you query 'functions' earlier) and
for each resourceInternalId that is missing from the map call
createStatsDocuments($region, METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS, 0),
createStatsDocuments(...METRIC_RESOURCE_TYPE_ID_BUILDS, 0),
createStatsDocuments(...METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS_STORAGE, 0) and
createStatsDocuments(...METRIC_RESOURCE_TYPE_ID_BUILDS_STORAGE, 0) as
appropriate; apply the same map-and-emit-zero logic to the analogous sites block
(the code around the other groupBy usage mentioned) so both FUNCTIONS and SITES
metrics are backfilled to zero when absent from $groupedDeployments.
- Around line 287-290: The Promise::map(...)->await() call that populates
$collectionResults can throw and currently aborts the whole run; wrap that call
in a try/catch (mirroring the pattern used in countForBuckets()) so exceptions
for a single collection are caught, log the error (including context:
$collectionName and $databaseName/$projectId) and set safe defaults for
documents/storage (e.g., 0) or skip processing for that collection, then
continue the loop so one bad collection doesn't stop stats generation; update
the block referencing Promise::map, $collectionResults, and the surrounding
collection-processing loop in StatsResources.php accordingly.
- Around line 119-120: The DAU cutoff uses a malformed timestamp format — update
the code that builds $last24Hours (used by the usersDAU closure and the
Query::greaterThanEqual('accessedAt', $last24Hours) call) to use a
24-hour/hour:minute format; replace format('Y-m-d h:m:00') with format('Y-m-d
H:i:00') so hours and minutes are formatted correctly.
---
Nitpick comments:
In `@app/controllers/api/messaging.php`:
- Around line 2920-2929: The current use of Promise::map with per-subscriber
closures (the block that calls $dbForProject->getDocument('targets',
$subscriber->getAttribute('targetId')) and ->getDocument('users',
$target->getAttribute('userId')) for each $subscriber) causes an N+1 burst of DB
requests; instead collect distinct target IDs and user IDs from $subscribers,
perform two bulk fetches for targets and users via $dbForProject (or its
bulk-get API), build lookup maps by id, and then iterate $subscribers once to
setAttribute('target', $targetLookup[$id]) and setAttribute('userName',
$userLookup[$userId]->getAttribute('name')); remove Promise::map concurrency for
this enrichment so lookups are served from memory.
In `@src/Appwrite/Databases/TransactionState.php`:
- Line 175: The current call building $committedDocs uses array_merge($queries,
[Query::select(['$id'])]) which can be silently overridden if $queries already
contains a Query::select; update the logic in TransactionState (the method that
calls $this->dbForProject->find with $collectionId and $queries) to first filter
out any existing Query::TYPE_SELECT entries from the $queries array, then append
Query::select(['$id']) and pass that cleaned array to find so the ID-only
projection is guaranteed; reference Query::select, Query::TYPE_SELECT, $queries,
$committedDocs and $this->dbForProject->find when locating the change.
In `@src/Appwrite/Platform/Modules/Functions/Workers/Screenshots.php`:
- Around line 155-195: The code uses a shared $screenshotError mutated inside
each async closure (in Promise::map) which creates a race and nondeterministic
failure reporting; change each closure (the one created over
array_keys($configs)) to return per-task status (e.g., ['key'=> $key,
'screenshot'=> $screenshot, 'error'=> null] on success or ['key'=> $key,
'screenshot'=> null, 'error'=> $th->getMessage()] on failure) instead of writing
to $screenshotError, then after $screenshots = Promise::map(...)->await()
iterate the returned results to detect any entries with a non-null 'error' and
throw a deterministic exception (or aggregate errors) referencing the failing
key; remove the shared $screenshotError variable and update the post-processing
check that currently inspects $screenshotError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9ded997-4a8f-4b54-b8ce-d8f28caf903f
📒 Files selected for processing (24)
app/controllers/api/messaging.phpapp/controllers/api/project.phpapp/controllers/api/users.phpsrc/Appwrite/Databases/TransactionState.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Usage/Get.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Usage/Get.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Usage/XList.phpsrc/Appwrite/Platform/Modules/Databases/Workers/Databases.phpsrc/Appwrite/Platform/Modules/Functions/Http/Usage/Get.phpsrc/Appwrite/Platform/Modules/Functions/Http/Usage/XList.phpsrc/Appwrite/Platform/Modules/Functions/Workers/Screenshots.phpsrc/Appwrite/Platform/Modules/Sites/Http/Usage/Get.phpsrc/Appwrite/Platform/Modules/Sites/Http/Usage/XList.phpsrc/Appwrite/Platform/Modules/Storage/Http/Usage/Get.phpsrc/Appwrite/Platform/Modules/Storage/Http/Usage/XList.phpsrc/Appwrite/Platform/Modules/Teams/Http/Memberships/Delete.phpsrc/Appwrite/Platform/Modules/Teams/Http/Memberships/Update.phpsrc/Appwrite/Platform/Modules/VCS/Http/Installations/Repositories/XList.phpsrc/Appwrite/Platform/Tasks/Interval.phpsrc/Appwrite/Platform/Workers/Deletes.phpsrc/Appwrite/Platform/Workers/Functions.phpsrc/Appwrite/Platform/Workers/Messaging.phpsrc/Appwrite/Platform/Workers/StatsResources.phptests/e2e/Services/Databases/DatabasesBase.php
| fn () => $this->deleteByGroup('variables', [ | ||
| Query::equal('resourceType', ['site']), | ||
| Query::equal('resourceInternalId', [$siteInternalId]) | ||
| ], $dbForProject), |
There was a problem hiding this comment.
Use plural resourceType values in the site cleanup queries.
Line 1015 and Line 1041 use 'site', but this method already treats site-scoped resources as 'sites' on Line 1021. With the singular filter, site variables/repositories — and the nested VCS comment cleanup behind the repository callback — can be skipped.
🛠️ Suggested fix
fn () => $this->deleteByGroup('variables', [
- Query::equal('resourceType', ['site']),
+ Query::equal('resourceType', ['sites']),
Query::equal('resourceInternalId', [$siteInternalId])
], $dbForProject),
@@
fn () => $this->deleteByGroup('repositories', [
Query::equal('projectInternalId', [$project->getSequence()]),
Query::equal('resourceInternalId', [$siteInternalId]),
- Query::equal('resourceType', ['site']),
+ Query::equal('resourceType', ['sites']),
], $dbForPlatform, function (Document $document) use ($dbForPlatform) {Also applies to: 1038-1042
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Workers/Deletes.php` around lines 1014 - 1017, The
site-scoped cleanup queries are using the singular 'site' for the resourceType
filter which prevents matching stored plural resourceType values; update the
Query::equal calls inside the deleteByGroup callbacks (notably the variables and
repositories cleanup closures that call deleteByGroup and the repository-related
VCS comment cleanup) to use 'sites' instead of 'site' (e.g., replace
Query::equal('resourceType', ['site']) with Query::equal('resourceType',
['sites']) in the deleteByGroup invocations and any related repository callback
filters).
| fn () => $this->deleteByGroup('variables', [ | ||
| Query::equal('resourceInternalId', [$functionInternalId]), | ||
| Query::equal('resourceType', ['function']), | ||
| Query::orderAsc() | ||
| ], $dbForProject), |
There was a problem hiding this comment.
Use plural resourceType values in the function cleanup queries.
Line 1087 and Line 1111 use 'function', while the deployment/execution branches in the same method already query 'functions'. That mismatch can leave function variables and repositories behind during cleanup.
🛠️ Suggested fix
fn () => $this->deleteByGroup('variables', [
Query::equal('resourceInternalId', [$functionInternalId]),
- Query::equal('resourceType', ['function']),
+ Query::equal('resourceType', ['functions']),
Query::orderAsc()
], $dbForProject),
@@
fn () => $this->deleteByGroup('repositories', [
Query::equal('projectInternalId', [$project->getSequence()]),
Query::equal('resourceInternalId', [$functionInternalId]),
- Query::equal('resourceType', ['function']),
+ Query::equal('resourceType', ['functions']),
Query::orderAsc()
], $dbForPlatform, function (Document $document) use ($dbForPlatform) {Also applies to: 1108-1113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Workers/Deletes.php` around lines 1085 - 1089, The
cleanup queries use the singular 'function' for resourceType which violates the
plural convention and causes leftover records; update the
Query::equal('resourceType', [...]) calls in the delete closures to use
'functions' (e.g., the lambda passed to $this->deleteByGroup for 'variables' and
the similar deleteByGroup call for repositories/deployments) so both occurrences
use 'functions' instead of 'function' to match the other branches and the
required plural resourceType format.
| fn () => $this->deleteByGroup('deployments', [ | ||
| Query::equal('resourceInternalId', [$functionInternalId]), | ||
| Query::equal('resourceType', ['functions']), | ||
| Query::orderAsc() | ||
| ], $dbForProject, function (Document $document) use ($dbForPlatform, $project, $certificates, $deviceForFunctions, $deviceForBuilds, &$deploymentInternalIds) { | ||
| $deploymentInternalIds[] = $document->getSequence(); | ||
| $this->deleteDeploymentFiles($deviceForFunctions, $document); | ||
| $this->deleteBuildFiles($deviceForBuilds, $document); | ||
| }), |
There was a problem hiding this comment.
Don't run runtime cleanup in parallel with deployment deletion.
Line 1091 starts deleting deployments, while Line 1124 re-reads deployments to decide which runtimes to remove. In this fan-out, the runtime branch can miss documents the delete branch has already removed, leaving orphaned containers behind.
🛠️ Suggested fix
+ $this->deleteRuntimes($getProjectDB, $document, $project, $executor);
+
Promise::map([
// Delete rules
fn () => $this->deleteByGroup('rules', [
@@
- // Request executor to delete all deployment containers
- fn () => $this->deleteRuntimes($getProjectDB, $document, $project, $executor),
])->await();Also applies to: 1123-1125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Workers/Deletes.php` around lines 1091 - 1099, The
deletion of deployments (via deleteByGroup with the callback that appends to
$deploymentInternalIds and calls deleteDeploymentFiles/deleteBuildFiles) races
with the runtime cleanup branch that re-queries the deployments collection,
causing missed runtimes; modify the runtime cleanup to use the captured
$deploymentInternalIds populated in the deleteByGroup callback instead of
re-reading the collection (or run the runtime cleanup sequentially after the
deleteByGroup completes) so runtime removal operates on the exact set of deleted
deployment IDs and not a concurrent query result.
| 'usersDAU' => fn () => $dbForProject->count('users', [ | ||
| Query::greaterThanEqual('accessedAt', $last24Hours) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "StatsResources.php" -type f 2>/dev/null | head -5Repository: appwrite/appwrite
Length of output: 227
🏁 Script executed:
cat -n src/Appwrite/Platform/Workers/StatsResources.php | sed -n '90,125p'Repository: appwrite/appwrite
Length of output: 2369
🌐 Web query:
PHP DateTime::format() h i m H tokens meaning Y-m-d
💡 Result:
In PHP DateTime::format, the format tokens have the following meanings: - h: 12-hour format of an hour with leading zeros (01 to 12) - i: Minutes with leading zeros (00 to 59) - m: Month as a zero-padded decimal number (01 to 12) - H: 24-hour format of an hour with leading zeros (00 to 23) - Y: Full numeric year (e.g., 2026) - -: Literal hyphen - d: Day of the month, 2 digits with leading zeros (01 to 31) 'Y-m-d' produces a date string like '2026-03-25'.
Citations:
- 1: https://www.php.net/manual/en/datetime.format.php
- 2: https://www.php.net/datetime.format
- 3: https://www.w3schools.com/php/func_date_date_format.asp
- 4: https://docs.huihoo.com/php/php-manual-2011-en/function.date.html
Fix the malformed 24-hour timestamp format in the DAU query.
Line 95 uses format('Y-m-d h:m:00') which produces an incorrect timestamp: h represents 12-hour format (should be H for 24-hour), and m represents the month (should be i for minutes). This causes the DAU query on Line 120 to use a wrong cutoff time. Change it to format('Y-m-d H:i:00').
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Workers/StatsResources.php` around lines 119 - 120, The
DAU cutoff uses a malformed timestamp format — update the code that builds
$last24Hours (used by the usersDAU closure and the
Query::greaterThanEqual('accessedAt', $last24Hours) call) to use a
24-hour/hour:minute format; replace format('Y-m-d h:m:00') with format('Y-m-d
H:i:00') so hours and minutes are formatted correctly.
| $collectionResults = Promise::map([ | ||
| 'documents' => fn () => $dbForProject->count($collectionName), | ||
| 'storage' => fn () => $dbForProject->getSizeOfCollection($collectionName), | ||
| ])->await(); |
There was a problem hiding this comment.
Keep one bad collection from aborting the entire database pass.
Promise::map(...)->await() can throw here, and Appwrite\Platform\Action::foreachDocument() does not catch callback exceptions on the non-concurrent path. One failing collection will stop stats generation for every remaining collection/database in this run. Mirror the local try/catch pattern already used in countForBuckets().
🛠️ Proposed fix
- $collectionResults = Promise::map([
- 'documents' => fn () => $dbForProject->count($collectionName),
- 'storage' => fn () => $dbForProject->getSizeOfCollection($collectionName),
- ])->await();
+ try {
+ $collectionResults = Promise::map([
+ 'documents' => fn () => $dbForProject->count($collectionName),
+ 'storage' => fn () => $dbForProject->getSizeOfCollection($collectionName),
+ ])->await();
+ } catch (Throwable $th) {
+ call_user_func_array($this->logError, [$th, "StatsResources", "count_for_collection_{$database->getSequence()}_{$collection->getSequence()}"]);
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Workers/StatsResources.php` around lines 287 - 290, The
Promise::map(...)->await() call that populates $collectionResults can throw and
currently aborts the whole run; wrap that call in a try/catch (mirroring the
pattern used in countForBuckets()) so exceptions for a single collection are
caught, log the error (including context: $collectionName and
$databaseName/$projectId) and set safe defaults for documents/storage (e.g., 0)
or skip processing for that collection, then continue the loop so one bad
collection doesn't stop stats generation; update the block referencing
Promise::map, $collectionResults, and the surrounding collection-processing loop
in StatsResources.php accordingly.
| $groupedDeployments = $dbForProject->find('deployments', [ | ||
| Query::equal('resourceType', [RESOURCE_TYPE_FUNCTIONS]), | ||
| Query::groupBy(['resourceInternalId']), | ||
| Query::count('*', 'deploymentsCount'), | ||
| Query::sum('sourceSize', 'deploymentsStorage'), | ||
| Query::sum('buildSize', 'buildsStorage'), | ||
| ]); | ||
| $this->createStatsDocuments($region, str_replace("{resourceType}", RESOURCE_TYPE_FUNCTIONS, METRIC_RESOURCE_TYPE_DEPLOYMENTS), $deployments); | ||
| $this->createStatsDocuments($region, str_replace("{resourceType}", RESOURCE_TYPE_FUNCTIONS, METRIC_RESOURCE_TYPE_BUILDS), $deployments); | ||
|
|
||
|
|
||
| // Count runtimes | ||
| $runtimes = []; | ||
|
|
||
| $this->foreachDocument($dbForProject, 'functions', [], function (Document $function) use ($dbForProject, $region, &$runtimes) { | ||
| $functionDeploymentsStorage = $dbForProject->sum('deployments', 'sourceSize', [ | ||
| Query::equal('resourceInternalId', [$function->getSequence()]), | ||
| Query::equal('resourceType', [RESOURCE_TYPE_FUNCTIONS]), | ||
| ]); | ||
| $this->createStatsDocuments($region, str_replace(['{resourceType}','{resourceInternalId}'], [RESOURCE_TYPE_FUNCTIONS,$function->getSequence()], METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS_STORAGE), $functionDeploymentsStorage); | ||
|
|
||
| $functionDeployments = $dbForProject->count('deployments', [ | ||
| Query::equal('resourceInternalId', [$function->getSequence()]), | ||
| Query::equal('resourceType', [RESOURCE_TYPE_FUNCTIONS]), | ||
| ]); | ||
| $this->createStatsDocuments($region, str_replace(['{resourceType}','{resourceInternalId}'], [RESOURCE_TYPE_FUNCTIONS,$function->getSequence()], METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS), $functionDeployments); | ||
|
|
||
| /** | ||
| * As deployments and builds have 1-1 relationship, | ||
| * the count for one should match the other | ||
| */ | ||
| $this->createStatsDocuments($region, str_replace(['{resourceType}','{resourceInternalId}'], [RESOURCE_TYPE_FUNCTIONS,$function->getSequence()], METRIC_RESOURCE_TYPE_ID_BUILDS), $functionDeployments); | ||
| foreach ($groupedDeployments as $row) { | ||
| $resourceInternalId = $row->getAttribute('resourceInternalId'); | ||
| $deploymentsCount = $row->getAttribute('deploymentsCount', 0); | ||
| $deploymentsStorage = $row->getAttribute('deploymentsStorage', 0); | ||
| $buildsStorage = $row->getAttribute('buildsStorage', 0); | ||
|
|
||
| $functionBuildsStorage = 0; | ||
|
|
||
| $this->foreachDocument($dbForProject, 'deployments', [ | ||
| Query::equal('resourceInternalId', [$function->getSequence()]), | ||
| Query::equal('resourceType', [RESOURCE_TYPE_FUNCTIONS]), | ||
| ], function (Document $deployment) use (&$functionBuildsStorage): void { | ||
| $functionBuildsStorage += $deployment->getAttribute('buildSize', 0); | ||
| }); | ||
| $this->createStatsDocuments($region, str_replace(['{resourceType}', '{resourceInternalId}'], [RESOURCE_TYPE_FUNCTIONS, $resourceInternalId], METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS_STORAGE), $deploymentsStorage); | ||
| $this->createStatsDocuments($region, str_replace(['{resourceType}', '{resourceInternalId}'], [RESOURCE_TYPE_FUNCTIONS, $resourceInternalId], METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS), $deploymentsCount); | ||
| $this->createStatsDocuments($region, str_replace(['{resourceType}', '{resourceInternalId}'], [RESOURCE_TYPE_FUNCTIONS, $resourceInternalId], METRIC_RESOURCE_TYPE_ID_BUILDS), $deploymentsCount); | ||
| $this->createStatsDocuments($region, str_replace(['{resourceType}', '{resourceInternalId}'], [RESOURCE_TYPE_FUNCTIONS, $resourceInternalId], METRIC_RESOURCE_TYPE_ID_BUILDS_STORAGE), $buildsStorage); | ||
| } |
There was a problem hiding this comment.
Backfill zero-deployment stats for resources missing from the aggregate result.
groupBy(['resourceInternalId']) only returns functions/sites that currently have at least one deployment. Because writeDocuments() only upserts metrics produced in this run, a function/site that drops to zero deployments will keep its previous METRIC_RESOURCE_TYPE_ID_* values instead of being reset. Build a map from the grouped rows, then iterate the current functions/sites collections and emit 0 for missing IDs.
Also applies to: 403-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Workers/StatsResources.php` around lines 347 - 365,
groupBy on 'resourceInternalId' only returns resources with >=1 deployment so
resources that dropped to zero keep stale metrics; fix by first building a map
from $groupedDeployments (keyed by resourceInternalId with deploymentsCount,
deploymentsStorage, buildsStorage) and then iterate the current functions
collection (the same place you query 'functions' earlier) and for each
resourceInternalId that is missing from the map call
createStatsDocuments($region, METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS, 0),
createStatsDocuments(...METRIC_RESOURCE_TYPE_ID_BUILDS, 0),
createStatsDocuments(...METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS_STORAGE, 0) and
createStatsDocuments(...METRIC_RESOURCE_TYPE_ID_BUILDS_STORAGE, 0) as
appropriate; apply the same map-and-emit-zero logic to the analogous sites block
(the code around the other groupBy usage mentioned) so both FUNCTIONS and SITES
metrics are backfilled to zero when absent from $groupedDeployments.
This comment has been minimized.
This comment has been minimized.
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
DocumentsDBConsoleClientTest::testTimeout |
1 | 122.31s | Logs |
DocumentsDBCustomClientTest::testTimeout |
1 | 120.56s | Logs |
DocumentsDBCustomServerTest::testTimeout |
1 | 120.18s | Logs |
LegacyConsoleClientTest::testPatchAttribute |
1 | 240.54s | Logs |
LegacyCustomClientTest::testCreateAttributes |
1 | 240.77s | Logs |
VectorsDBConsoleClientTest::testGetDatabaseUsage |
1 | 27ms | Logs |
VectorsDBConsoleClientTest::testGetCollectionUsage |
1 | 29ms | Logs |
DatabasesConsoleClientTest::testGetDatabaseUsage |
1 | 18ms | Logs |
DatabasesConsoleClientTest::testGetCollectionUsage |
1 | 23ms | Logs |
Commit ec71ff2 - 5 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
LegacyConsoleClientTest::testListDocumentsWithCache |
1 | 1.02s | Logs |
LegacyConsoleClientTest::testTimeout |
1 | 124.05s | Logs |
LegacyCustomClientTest::testCreateIndexes |
1 | 241.86s | Logs |
LegacyCustomServerTest::testAttributeResponseModels |
1 | 242.09s | Logs |
LegacyPermissionsMemberTest::testReadDocuments with data set #7 |
1 | 120.02s | Logs |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Databases/Workers/Databases.php (1)
583-583: Use PSR-12 spacing for concatenation at Line 583.Please format the concatenation with spaces around
.for consistency/readability:
'"relatedCollection":"' . $collectionId . '"'As per coding guidelines "Follow PSR-12 coding standard".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Modules/Databases/Workers/Databases.php` at line 583, The string concatenation passed to Query::containsAny that builds the '"relatedCollection":"...'" value should follow PSR-12 spacing: add spaces around the concatenation operator between the string literal, the $collectionId variable, and the trailing string literal so the expression reads with spaces on both sides of each '.' when combining '"relatedCollection":"', $collectionId and the ending quote.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/api/users.php`:
- Line 31: The code imports and calls a non-existent Utopia\Async\Promise and
uses Promise::map(...)->await(), which will fatal; replace the import with the
actual Promise implementation (Appwrite\Promises\Promise) and change the call
from Promise::map($tasks)->await() to use Promise::all($tasks) and handle
results via then()/catch() (or create()/then()) instead of await; update any
references to Utopia\Async\Promise to Appwrite\Promises\Promise and adjust the
surrounding logic to process results inside the then() callback (or
synchronous-style continuation) so the code no longer relies on map() or
await().
---
Nitpick comments:
In `@src/Appwrite/Platform/Modules/Databases/Workers/Databases.php`:
- Line 583: The string concatenation passed to Query::containsAny that builds
the '"relatedCollection":"...'" value should follow PSR-12 spacing: add spaces
around the concatenation operator between the string literal, the $collectionId
variable, and the trailing string literal so the expression reads with spaces on
both sides of each '.' when combining '"relatedCollection":"', $collectionId and
the ending quote.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16de0f7a-18be-41cf-a2b2-8a5d0ac9a3e5
📒 Files selected for processing (9)
app/controllers/api/messaging.phpapp/controllers/api/project.phpapp/controllers/api/users.phpsrc/Appwrite/Databases/TransactionState.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Usage/Get.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Usage/Get.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Usage/XList.phpsrc/Appwrite/Platform/Modules/Databases/Workers/Databases.phpsrc/Appwrite/Platform/Modules/VCS/Http/Installations/Repositories/XList.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Databases/TransactionState.php
| use Appwrite\Utopia\Request; | ||
| use Appwrite\Utopia\Response; | ||
| use MaxMind\Db\Reader; | ||
| use Utopia\Async\Promise; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Promise class definition and map method signature
ast-grep --pattern $'class Promise {
$$$
static function map($$$) {
$$$
}
$$$
}'
# Alternative: search in vendor for Promise implementation
fd -e php 'Promise' --exec grep -l 'function map' {} 2>/dev/null | head -5 | xargs -I{} grep -A 10 'function map' {}Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Find Promise class files
find . -type f -name "*.php" | xargs grep -l "class Promise" 2>/dev/null | head -10Repository: appwrite/appwrite
Length of output: 96
🏁 Script executed:
# Look for Promise in vendor directory
fd -e php "Promise.php" . 2>/dev/null | head -10Repository: appwrite/appwrite
Length of output: 96
🏁 Script executed:
# Search for Utopia\Async\Promise
rg "class Promise" -A 30 --type php | head -50Repository: appwrite/appwrite
Length of output: 1942
🏁 Script executed:
# Get the actual refactored code at lines 2753-2781
sed -n '2753,2781p' app/controllers/api/users.phpRepository: appwrite/appwrite
Length of output: 1327
🏁 Script executed:
cat -n ./src/Appwrite/Promises/Promise.phpRepository: appwrite/appwrite
Length of output: 5922
🏁 Script executed:
# Search for map method in Promise class
rg "function map\|static.*map" ./src/Appwrite/Promises/ -A 15Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Find Utopia Promise class
find . -type f -name "*.php" | xargs grep -l "Utopia.*Async.*Promise\|class Promise" | grep -i utopiaRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Search in vendor for Utopia
fd -e php "Promise" ./vendor --exec grep -l "namespace Utopia" {} 2>/dev/null | head -5Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Look for Utopia async in composer.json
grep -i "utopia" composer.jsonRepository: appwrite/appwrite
Length of output: 1478
🏁 Script executed:
# Search for Utopia\Async in src directory
rg "Utopia.*Async\|namespace Utopia\\\\Async" ./src -A 5 -B 5Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Look for any async-related files
fd -e php "async\|promise" ./src --ignore-caseRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Check if Utopia\Async is defined anywhere in the codebase
grep -r "namespace Utopia" ./src --include="*.php" | grep -i asyncRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
head -50 app/controllers/api/users.phpRepository: appwrite/appwrite
Length of output: 1749
🏁 Script executed:
# Search for all Promise-related imports and uses in users.php
rg "Promise|use.*Async" app/controllers/api/users.php -B 2 -A 2Repository: appwrite/appwrite
Length of output: 302
🏁 Script executed:
# Search in vendor for Utopia Async Promise
find ./vendor -type f -path "*Utopia*Async*Promise*" 2>/dev/nullRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Search for Promise class with map method in vendor
find ./vendor -type f -name "*.php" -exec grep -l "class Promise" {} \; 2>/dev/null | xargs grep -l "function map\|static.*map" 2>/dev/nullRepository: appwrite/appwrite
Length of output: 45
🏁 Script executed:
# Search more broadly in vendor for Utopia/Async namespace
find ./vendor -type d -name "Async" 2>/dev/nullRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# List what Utopia packages are available in vendor
ls -la vendor/utopia-php/ 2>/dev/null || ls -la vendor/ | grep -i utopiaRepository: appwrite/appwrite
Length of output: 115
🏁 Script executed:
# Try searching directly for async in vendor
find ./vendor -type f -name "*.php" | xargs grep -l "namespace.*Async" 2>/dev/null | head -5Repository: appwrite/appwrite
Length of output: 104
🏁 Script executed:
# Check if vendor directory exists and what's in utopia
test -d vendor/utopia-php && ls vendor/utopia-php/ || echo "vendor/utopia-php not found"Repository: appwrite/appwrite
Length of output: 88
🏁 Script executed:
# Check composer.json for utopia-php/async or similar
cat composer.json | grep -i asyncRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Search for any references to Promise in composer files
grep -r "Promise\|Async" composer.* 2>/dev/nullRepository: appwrite/appwrite
Length of output: 43
🌐 Web query:
utopia-php async promise library
💡 Result:
There is no official async promise library from utopia-php. Utopia PHP (github.com/utopia-php) is a collection of micro-libraries including HTTP framework, queue, and Swoole support for async servers, but no dedicated Promise library. The closest match is m3m0r7/async-promise, a general PHP async promise library compatible with Swoole (used by Utopia), but unrelated to utopia-php.
Citations:
- 1: https://github.com/m3m0r7/async-promise
- 2: https://github.com/utopia-php/servers
- 3: https://github.com/utopia-php/http
- 4: https://github.com/utopia-php
- 5: https://github.com/orgs/utopia-php/repositories
- 6: https://github.com/utopia-php/swoole
🏁 Script executed:
# Search in src directory for Utopia/Async directory or files
find ./src -type d -name "Async" 2>/dev/nullRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Broader search for any Utopia Async references in src
grep -r "Utopia.*Async\|namespace.*Async" ./src 2>/dev/null | head -20Repository: appwrite/appwrite
Length of output: 1296
🏁 Script executed:
# Check if users.php file actually compiles/has valid syntax around line 31 and 2753
sed -n '28,35p' app/controllers/api/users.phpRepository: appwrite/appwrite
Length of output: 276
🏁 Script executed:
# Search for the actual Promise class definition in src
grep -r "class Promise\|interface Promise" ./src --include="*.php" | head -10Repository: appwrite/appwrite
Length of output: 119
🏁 Script executed:
# Look for the Async namespace definition
grep -r "namespace.*Async" ./src --include="*.php" | head -10Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Get lines 2750-2785 to see the exact Promise::map usage
sed -n '2750,2785p' app/controllers/api/users.phpRepository: appwrite/appwrite
Length of output: 1468
🏁 Script executed:
# Check if there's an actual Utopia\Async\Promise somewhere we missed
find . -name "*.php" -type f | xargs grep -l "namespace Utopia.Async" 2>/dev/nullRepository: appwrite/appwrite
Length of output: 45
🏁 Script executed:
# List all files that use Promise::map to see pattern
grep -r "Promise::map" ./src --include="*.php" | head -5Repository: appwrite/appwrite
Length of output: 681
🏁 Script executed:
# Check one of the other files using Promise::map
head -40 ./src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Usage/Get.php | grep -A 5 -B 5 "Utopia"Repository: appwrite/appwrite
Length of output: 920
🏁 Script executed:
# Search for any class alias or use statement that might alias Utopia\Async\Promise
grep -r "class_alias\|use.*as.*Promise" ./src --include="*.php" | head -10Repository: appwrite/appwrite
Length of output: 357
🏁 Script executed:
# Check if there's a bootstrap or autoload file that might define Utopia\Async\Promise
find . -name "*.php" -type f \( -name "bootstrap*" -o -name "autoload*" -o -name "init*" \) | xargs grep -l "Utopia.Async" 2>/dev/nullRepository: appwrite/appwrite
Length of output: 45
🏁 Script executed:
# Check if there's a namespace or class alias defined
grep -r "Utopia.Async\|namespace Utopia" ./src ./app --include="*.php" 2>/dev/null | head -20Repository: appwrite/appwrite
Length of output: 1624
🏁 Script executed:
# Check if Appwrite\Promises\Promise might be aliased as Utopia\Async\Promise
grep -r "class_alias.*Promise\|namespace.*Utopia.*Async" . --include="*.php" 2>/dev/nullRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Look for any configuration that might map namespaces
find . -name "*.json" -o -name "*.php" | xargs grep -l "Utopia.Async" 2>/dev/null | head -5Repository: appwrite/appwrite
Length of output: 316
🏁 Script executed:
# Check if Appwrite\Promises\Promise has been extended or has map/await methods
grep -n "function map\|function await" ./src/Appwrite/Promises/Promise.phpRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Check all Promise-related files
find ./src -name "*Promise*" -type fRepository: appwrite/appwrite
Length of output: 96
🏁 Script executed:
# Look for any trait that might add map method
grep -r "trait.*Promise\|function map\|function await" ./src --include="*.php" | head -10Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Check if there's a swoole-specific Promise implementation
find ./src -name "*.php" -type f -exec grep -l "class.*Promise\|function map" {} \; | xargs catRepository: appwrite/appwrite
Length of output: 8777
🏁 Script executed:
# Look at the full Promises directory
ls -la ./src/Appwrite/Promises/Repository: appwrite/appwrite
Length of output: 297
🏁 Script executed:
# Check if there are subclasses of Promise
grep -r "extends.*Promise\|implements.*Promise" ./src --include="*.php" | head -10Repository: appwrite/appwrite
Length of output: 217
Critical issue: Non-existent Promise class and methods
The code uses use Utopia\Async\Promise and calls Promise::map($tasks)->await(), but neither the Utopia\Async\Promise class nor the map() and await() methods exist in the codebase or its dependencies.
The only Promise implementation available is Appwrite\Promises\Promise (and its Swoole subclass), which provides create(), resolve(), reject(), all(), then(), and catch() methods—but not map() or await(). This code would fail at runtime with fatal errors about undefined methods and classes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/api/users.php` at line 31, The code imports and calls a
non-existent Utopia\Async\Promise and uses Promise::map(...)->await(), which
will fatal; replace the import with the actual Promise implementation
(Appwrite\Promises\Promise) and change the call from
Promise::map($tasks)->await() to use Promise::all($tasks) and handle results via
then()/catch() (or create()/then()) instead of await; update any references to
Utopia\Async\Promise to Appwrite\Promises\Promise and adjust the surrounding
logic to process results inside the then() callback (or synchronous-style
continuation) so the code no longer relies on map() or await().
✨ Benchmark results
⚡ Benchmark Comparison
|
|
Found 1 test failure on Blacksmith runners: Failure
|
1 participant
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist