Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe build worker refactors error handling for missing entrypoints and unsupported runtimes, replacing exception throws with a dedicated failure path using a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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! |
loks0n
approved these changes
Mar 9, 2026
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.31s | Logs |
TablesDBConsoleClientTest::testSpatialIndex |
1 | 240.54s | Logs |
Commit 062da7d - 2 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.25s | Logs |
RealtimeCustomClientTest::testChannelTablesDBRowUpdate |
1 | 683ms | Logs |
Commit e5c34a0 - 5 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.28s | Logs |
LegacyCustomClientTest::testInvalidDocumentStructure |
1 | 241.73s | Logs |
LegacyCustomServerTest::testNotBetween |
1 | 240.61s | Logs |
LegacyTransactionsConsoleClientTest::testMixedSingleOperations |
1 | 240.27s | Logs |
TablesDBTransactionsConsoleClientTest::testTransactionSizeLimit |
1 | 240.37s | Logs |
✨ Benchmark results
⚡ Benchmark Comparison
|
Meldiron
reviewed
Mar 10, 2026
| 'buildEndedAt' => $endTime, | ||
| 'buildDuration' => $buildDuration, | ||
| 'status' => 'failed', | ||
| 'buildLogs' => $message, |
Contributor
There was a problem hiding this comment.
This overrides any logs that might already be there.
Might be better to just append it, to be more universal in future
Meldiron
approved these changes
Mar 10, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1414-1423: VCS status update follows existing patterns but lacks error handling for missing installation.If
providerRepositoryIdis set butinstallationIdis empty or the installation document doesn't exist,$installationwill be empty andproviderInstallationIdwill be null, potentially causing issues ingithub->initializeVariables().This mirrors the existing pattern in
buildDeployment(lines 332-339), so it's consistent with the codebase. However, consider adding a guard:Optional: Add installation validation
$providerRepositoryId = $deployment->getAttribute('providerRepositoryId', ''); if (!empty($providerRepositoryId)) { $installationId = $deployment->getAttribute('installationId', ''); + if (empty($installationId)) { + return; + } $installation = $dbForPlatform->getDocument('installations', $installationId); + if ($installation->isEmpty()) { + return; + } $providerInstallationId = $installation->getAttribute('providerInstallationId');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Modules/Functions/Workers/Builds.php` around lines 1414 - 1423, The code assumes an installation exists when providerRepositoryId is present; add a guard that verifies installationId is non-empty and that $installation (result of $dbForPlatform->getDocument('installations', $installationId)) is truthy before calling $installation->getAttribute('providerInstallationId') and $github->initializeVariables(); if the installation is missing/empty, skip calling $github->initializeVariables() and skip or short-circuit the subsequent runGitAction(...) call (or log and return) to avoid passing a null providerInstallationId into github->initializeVariables() or runGitAction().
🤖 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/Modules/Functions/Workers/Builds.php`:
- Around line 252-265: The realtime queue (queueForRealtime plus its
subscribers/project/event/params) is not configured before calling
markDeploymentFailed in Builds.php, so the realtime trigger inside
markDeploymentFailed fires without proper routing; fix by either moving the
realtime configuration block (the code that sets queueForRealtime, subscribers,
project, event, params) to execute before the Entrypoint-missing check so
markDeploymentFailed sees a fully-configured queue and then remove the duplicate
configuration later, or change markDeploymentFailed to accept the realtime
configuration (pass queueForRealtime and its subscribers/project/event/params
into markDeploymentFailed) and let markDeploymentFailed initialize/use the
realtime queue internally (update its signature and call sites accordingly).
---
Nitpick comments:
In `@src/Appwrite/Platform/Modules/Functions/Workers/Builds.php`:
- Around line 1414-1423: The code assumes an installation exists when
providerRepositoryId is present; add a guard that verifie
10BC0
s installationId is
non-empty and that $installation (result of
$dbForPlatform->getDocument('installations', $installationId)) is truthy before
calling $installation->getAttribute('providerInstallationId') and
$github->initializeVariables(); if the installation is missing/empty, skip
calling $github->initializeVariables() and skip or short-circuit the subsequent
runGitAction(...) call (or log and return) to avoid passing a null
providerInstallationId into github->initializeVariables() or runGitAction().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d603da20-6996-40a2-806c-32a91a256625
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
Comment on lines
+252
to
+265
| $this->markDeploymentFailed( | ||
| $dbForProject, | ||
| $dbForPlatform, | ||
| $deployment, | ||
| $resource, | ||
| $project, | ||
| $queueForRealtime, | ||
| $queueForStatsUsage, | ||
| $github, | ||
| $platform, | ||
| $durationStart, | ||
| 'Entrypoint for your Appwrite Function is missing. Please specify it when making deployment or update the entrypoint under your function\'s "Settings" > "Configuration" > "Entrypoint".' | ||
| ); | ||
| return; |
Contributor
There was a problem hiding this comment.
Realtime queue not configured before triggering.
When markDeploymentFailed is called here, the queueForRealtime hasn't been configured yet (subscribers, project, event, and params are set at lines 291-298). The realtime trigger inside markDeploymentFailed will fire without proper event routing.
Consider either:
- Moving the realtime configuration block (lines 291-298) before this check, or
- Passing the necessary configuration to
markDeploymentFailedand having it set up the realtime queue internally.
Option 1: Move realtime configuration earlier
+ $resourceKey = match ($resource->getCollection()) {
+ 'functions' => 'functionId',
+ 'sites' => 'siteId',
+ default => throw new \Exception('Invalid resource type')
+ };
+
+ // Realtime preparation (moved earlier for early-exit paths)
+ $event = "{$resource->getCollection()}.[{$resourceKey}].deployments.[deploymentId].update";
+ $queueForRealtime
+ ->setSubscribers(['console'])
+ ->setProject($project)
+ ->setEvent($event)
+ ->setParam($resourceKey, $resource->getId())
+ ->setParam('deploymentId', $deployment->getId());
+
if ($resource->getCollection() === 'functions' && empty($deployment->getAttribute('entrypoint', ''))) {
$this->markDeploymentFailed(Then remove the duplicate realtime configuration block at lines 291-298.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Modules/Functions/Workers/Builds.php` around lines 252
- 265, The realtime queue (queueForRealtime plus its
subscribers/project/event/params) is not configured before calling
markDeploymentFailed in Builds.php, so the realtime trigger inside
markDeploymentFailed fires without proper routing; fix by either moving the
realtime configuration block (the code that sets queueForRealtime, subscribers,
project, event, params) to execute before the Entrypoint-missing check so
markDeploymentFailed sees a fully-configured queue and then remove the duplicate
configuration later, or change markDeploymentFailed to accept the realtime
configuration (pass queueForRealtime and its subscribers/project/event/params
into markDeploymentFailed) and let markDeploymentFailed initialize/use the
realtime queue internally (update its signature and call sites accordingly).
HarshMN2345
approved these changes
Mar 14, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
219-232:⚠️ Potential issue | 🟠 MajorConfigure realtime routing before early
markDeploymentFailed(...)returns.
markDeploymentFailed(...)triggers realtime immediately, but the queue event/subscribers/project/params are initialized later (Line 258+). Early exits can emit an un-routed realtime event.#!/bin/bash # Verify ordering between early failure paths and realtime configuration rg -nP --type=php -C2 'markDeploymentFailed\(|// Realtime preparation|setSubscribers\(|setProject\(|setEvent\(|setParam\(' src/Appwrite/Platform/Modules/Functions/Workers/Builds.phpExpected:
markDeploymentFailed(...)calls appear before realtime preparation block.Also applies to: 238-254, 258-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Modules/Functions/Workers/Builds.php` around lines 219 - 232, The early calls to markDeploymentFailed(...) (e.g., the branch that returns on missing entrypoint) run before realtime routing is configured, causing unrouted realtime events; move or delay emitting failure events until after the realtime/queue setup (the block that calls setSubscribers(...), setProject(...), setEvent(...), setParam(...)) has executed and $queueForRealtime/$queueForStatsUsage are initialized, or refactor markDeploymentFailed to accept/initialize the same realtime routing context so every early return uses the configured routing; update all early failure sites (including the calls around lines shown) to ensure realtime preparation happens first or that markDeploymentFailed is passed the prepared routing context.
🤖 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/Modules/Functions/Workers/Builds.php`:
- Line 1327: The local variable $key assigned from
$resource->getAttribute('runtime') inside the getRuntime method is unused;
remove the assignment (the line that creates $key) so the method no longer
defines an unused $key variable, ensuring references remain to $resource and any
other used variables in getRuntime are untouched.
- Around line 219-227: Replace the nonexistent $queueForStatsUsage usage: update
all calls to markDeploymentFailed(...) and sendUsage(...) to pass the existing
Context $usage and UsagePublisher $publisherForUsage variables instead of
$queueForStatsUsage (e.g., change markDeploymentFailed(..., $queueForStatsUsage,
...) -> markDeploymentFailed(..., $usage, $publisherForUsage) and
sendUsage($resource, $deployment, $project, $queueForStatsUsage) ->
sendUsage($resource, $deployment, $project, $usage, $publisherForUsage)); also
update the markDeploymentFailed method signature to accept (Context $usage,
UsagePublisher $publisherForUsage) in place of StatsUsage so types align;
finally remove the unused $key variable assignment (the $key declared at line
1327).
---
Duplicate comments:
In `@src/Appwrite/Platform/Modules/Functions/Workers/Builds.php`:
- Around line 219-232: The early calls to markDeploymentFailed(...) (e.g., the
branch that returns on missing entrypoint) run before realtime routing is
configured, causing unrouted realtime events; move or delay emitting failure
events until after the realtime/queue setup (the block that calls
setSubscribers(...), setProject(...), setEvent(...), setParam(...)) has executed
and $queueForRealtime/$queueForStatsUsage are initialized, or refactor
markDeploymentFailed to accept/initialize the same realtime routing context so
every early return uses the configured routing; update all early failure sites
(including the calls around lines shown) to ensure realtime preparation happens
first or that markDeploymentFailed is passed the prepared routing context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ea69ba2-c1be-469e-9183-ae17c45e4c51
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
Comment on lines
+219
to
+227
| $this->markDeploymentFailed( | ||
| $dbForProject, | ||
| $dbForPlatform, | ||
| $deployment, | ||
| $resource, | ||
| $project, | ||
| $queueForRealtime, | ||
| $queueForStatsUsage, | ||
| $github, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unresolved symbol usage and sendUsage arity mismatch in this file
rg -nP --type=php '\$queueForStatsUsage\b|StatsUsage\s+\$queueForStatsUsage|function\s+sendUsage\s*\(|sendUsage\s*\(' src/Appwrite/Platform/Modules/Functions/Workers/Builds.phpRepository: appwrite/appwrite
Length of output: 464
🏁 Script executed:
#!/bin/bash
# Check variable scope around lines 219-253 for $usage and $publisherForUsage
sed -n '180,270p' src/Appwrite/Platform/Modules/Functions/Workers/Builds.php | head -100Repository: appwrite/appwrite
Length of output: 3489
🏁 Script executed:
#!/bin/bash
# Check the complete markDeploymentFailed method signature and its implementation
sed -n '1352,1410p' src/Appwrite/Platform/Modules/Functions/Workers/Builds.phpRepository: appwrite/appwrite
Length of output: 3023
🏁 Script executed:
#!/bin/bash
# Find the method signature that contains lines 219-253
# Work backwards from line 219 to find the opening function declaration
sed -n '1,220p' src/Appwrite/Platform/Modules/Functions/Workers/Builds.php | tail -60 | head -50Repository: appwrite/appwrite
Length of output: 1718
🏁 Script executed:
#!/bin/bash
# Find all call sites of markDeploymentFailed in the file
rg -n 'markD
D616
eploymentFailed' src/Appwrite/Platform/Modules/Functions/Workers/Builds.phpRepository: appwrite/appwrite
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Verify the unused variable $key mentioned in the scratchpad
sed -n '1320,1340p' src/Appwrite/Platform/Modules/Functions/Workers/Builds.phpRepository: appwrite/appwrite
Length of output: 852
Critical: early user-error path crashes due to undefined variable and method signature mismatch.
Lines 226 and 247 pass undefined $queueForStatsUsage to markDeploymentFailed. The method signature (line 1352) expects StatsUsage $queueForStatsUsage, but this variable does not exist in scope. Additionally, line 1399 calls sendUsage($resource, $deployment, $project, $queueForStatsUsage) with 4 arguments, while the actual signature at line 1230 requires 5: (Document, Document, Document, Context, UsagePublisher).
The containing function has Context $usage and UsagePublisher $publisherForUsage parameters available. Replace the method parameter and calls with these.
Also: Line 1327 assigns unused variable $key.
Suggested fix
@@
$this->markDeploymentFailed(
$dbForProject,
$dbForPlatform,
$deployment,
$resource,
$project,
$queueForRealtime,
- $queueForStatsUsage,
+ $usage,
+ $publisherForUsage,
$github,
$platform,
$durationStart,
'Entrypoint for your Appwrite Function is missing. Please specify it when making deployment or update the entrypoint under your function\'s "Settings" > "Configuration" > "Entrypoint".'
);
@@
$this->markDeploymentFailed(
$dbForProject,
$dbForPlatform,
$deployment,
$resource,
$project,
$queueForRealtime,
- $queueForStatsUsage,
+ $usage,
+ $publisherForUsage,
$github,
$platform,
$durationStart,
'Runtime "' . $runtimeLabel . '" is not supported'
);
@@
protected function markDeploymentFailed(
Database $dbForProject,
Database $dbForPlatform,
Document $deployment,
Document $resource,
Document $project,
Realtime $queueForRealtime,
- StatsUsage $queueForStatsUsage,
+ Context $usage,
+ UsagePublisher $publisherForUsage,
GitHub $github,
array $platform,
float $durationStart,
string $message
): void {
@@
- $this->sendUsage($resource, $deployment, $project, $queueForStatsUsage);
+ $this->sendUsage($resource, $deployment, $project, $usage, $publisherForUsage);
}
@@
protected function getRuntime(Document $resource, string $version): ?array
{
$runtimes = Config::getParam($version === 'v2' ? 'runtimes-v2' : 'runtimes', []);
- $key = $resource->getAttribute('runtime');
$runtime = match ($resource->getCollection()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Modules/Functions/Workers/Builds.php` around lines 219
- 227, Replace the nonexistent $queueForStatsUsage usage: update all calls to
markDeploymentFailed(...) and sendUsage(...) to pass the existing Context $usage
and UsagePublisher $publisherForUsage variables instead of $queueForStatsUsage
(e.g., change markDeploymentFailed(..., $queueForStatsUsage, ...) ->
markDeploymentFailed(..., $usage, $publisherForUsage) and sendUsage($resource,
$deployment, $project, $queueForStatsUsage) -> sendUsage($resource, $deployment,
$project, $usage, $publisherForUsage)); also update the markDeploymentFailed
method signature to accept (Context $usage, UsagePublisher $publisherForUsage)
in place of StatsUsage so types align; finally remove the unused $key variable
assignment (the $key declared at line 1327).
Member
There was a problem hiding this comment.
its a parameter of buildDeployment() (line 203) and is passed from action() (line 148), so it is in scope at the markDeploymentFailed call sites
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify the actual method signatures in the PR branch to confirm your claim.
🏁 Script executed:
#!/bin/bash
# Check the actual buildDeployment() signature and action() signature in the PR branch
rg -n 'queueForStatsUsage|StatsUsage|function buildDeployment|function action' src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
Length of output: 287
@HarshMN2345, I checked the actual PR branch — $queueForStatsUsage does not appear in either the action() (line 82) or buildDeployment() (line 160) method signatures. It is only referenced at the two call sites (lines 226, 247) and inside markDeploymentFailed itself, meaning it is undefined in scope at those call sites.
The in-scope usage-related variables in buildDeployment() are $usage (Context) and $publisherForUsage (UsagePublisher). The fix from my original comment still applies:
- protected function markDeploymentFailed(
- ...
- Realtime $queueForRealtime,
- StatsUsage $queueForStatsUsage,
- GitHub $github,
+ protected function markDeploymentFailed(
+ ...
+ Realtime $queueForRealtime,
+ Context $usage,
+ UsagePublisher $publisherForUsage,
+ GitHub $github,
...
): void {
...
- $this->sendUsage($resource, $deployment, $project, $queueForStatsUsage);
+ $this->sendUsage($resource, $deployment, $project, $usage, $publisherForUsage);
}And at both call sites (lines 226, 247):
$this->markDeploymentFailed(
...
$queueForRealtime,
- $queueForStatsUsage,
+ $usage,
+ $publisherForUsage,
$github,
...
);🧠 Learnings used
Learnt from: Meldiron
Repo: appwrite/appwrite PR: 11448
File: app/controllers/general.php:327-327
Timestamp: 2026-03-09T11:06:53.386Z
Learning: In the Appwrite codebase (PHP), when migrating from a single `specification` attribute to separate `buildSpecification` and `runtimeSpecification` attributes, old documents are handled by the migration in `src/Appwrite/Migration/Version/V21.php`. Runtime code (e.g., `app/controllers/general.php`, `src/Appwrite/Platform/Workers/Functions.php`) does NOT need a fallback to the legacy `specification` attribute — it reads `runtimeSpecification` directly with `APP_COMPUTE_SPECIFICATION_DEFAULT` as the default. Do not suggest adding a fallback to the legacy `specification` attribute in runtime paths.
| protected function getRuntime(Document $resource, string $version): ?array | ||
| { | ||
| $runtimes = Config::getParam($version === 'v2' ? 'runtimes-v2' : 'runtimes', []); | ||
| $key = $resource->getAttribute('runtime'); |
Contributor
There was a problem hiding this comment.
Remove unused local $key in getRuntime.
Line 1327 is unused and already flagged by PHPMD.
🧹 Suggested cleanup
- $key = $resource->getAttribute('runtime');
-
$runtime = match ($resource->getCollection()) {📝 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.
Suggested change
| $key = $resource->getAttribute('runtime'); | |
| $runtime = match ($resource->getCollection()) { |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 1327-1327: Avoid unused local variables such as '$key'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Modules/Functions/Workers/Builds.php` at line 1327, The
local variable $key assigned from $resource->getAttribute('runtime') inside the
getRuntime method is unused; remove the assignment (the line that creates $key)
so the method no longer defines an unused $key variable, ensuring references
remain to $resource and any other used variables in getRuntime are untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix for user caused errors. They should report on build logs and not as system errors.