feat: orchestrator build backend with Deployments module#12479
feat: orchestrator build backend with Deployments module#12479deepshekhardas wants to merge 1 commit into
Conversation
…#12441) - Add Deployments module with Build/Action, Build/Update, Source/Get, Events/Create endpoints - Update Builds worker for orchestrator-driven deployment pipelines - Add build artifacts action for deployment tracking - Register new HTTP routes in Deployments/Services/Http.php
Greptile SummaryThis PR introduces an orchestrator-driven build backend for Appwrite deployments, adding a new
Confidence Score: 2/5Not safe to merge without fixes: premature event emission on partial chunk uploads and missing authorization skips in the new worker methods are active defects on the changed paths. Three active defects were found on changed code paths: (1) both Functions and Sites deployment creation now fire events after every chunk upload instead of only the final one, which can trigger build logic against an incomplete source archive; (2) the new orchestrator event handler and its polling helpers fetch from the database without the getAuthorization()->skip() wrappers that every other read in the same worker uses, so they may silently return empty documents under the project authorization context; (3) the ?? '' null coalesce was dropped when $outputDirectory was hoisted out of the closure, meaning a null is now passed to the executor when neither buildOutput nor outputDirectory is configured. Builds.php (new orchestrator methods), Functions/Http/Deployments/Create.php, and Sites/Http/Deployments/Create.php need the most attention.
|
| Filename | Overview |
|---|---|
| src/Appwrite/Platform/Modules/Functions/Workers/Builds.php | Core build worker extended with 834 lines of orchestrator support; missing auth-skip wrappers on new DB fetches and a null-coalesce regression in the executor path. |
| src/Appwrite/Platform/Modules/Functions/Http/Deployments/Create.php | Condition for emitting deployment events changed from $chunksUploaded === $chunks to !$deployment->isEmpty(), which fires events on every chunk upload instead of only the final one. |
| src/Appwrite/Platform/Modules/Sites/Http/Deployments/Create.php | Same premature-event bug as Functions/Create.php — condition changed to always-true !$deployment->isEmpty(). |
| src/Appwrite/Platform/Modules/Deployments/Http/Deployments/Events/Create.php | New endpoint verifying orchestrator HMAC signatures; callback secret falls back to the global OpenSSL key, which conflates two separate secrets. |
| src/Appwrite/Platform/Modules/Deployments/Http/Deployments/Artifacts/Build/Action.php | New chunked upload handler for build artifacts with idempotency via locking and cache; logic looks correct. |
| src/Appwrite/Platform/Modules/Deployments/Http/Deployments/Artifacts/Source/Get.php | New endpoint streaming source artifacts to the orchestrator; auth and resource ownership checks look solid. |
| app/init/resources/request.php | Token lookup changed from getParam to getQuery; new deploymentArtifacts token resolver added with correct format validation. |
| src/Appwrite/Platform/Modules/Functions/Http/Deployments/Status/Update.php | Cancellation now appends a log line, cleans up artifact tokens, and routes cleanup to the orchestrator or executor depending on backend config. |
| src/Appwrite/Platform/Modules/Sites/Http/Deployments/Status/Update.php | Mirror of Functions Status/Update.php; same cancellation improvements applied correctly to the sites deployment path. |
Comments Outside Diff (1)
-
src/Appwrite/Platform/Modules/Functions/Http/Deployments/Create.php, line 369-373 (link)Events fire on every chunk, not just the final upload.
$deploymentis always non-empty at this point (it was just created or updated a few lines above), so the condition is effectively always-true. The original guard$chunksUploaded === $chunksexisted to prevent the event — which triggers downstream build logic — from firing for incomplete, partial-chunk uploads. Removing it means the event will fire after chunk 1 of N, potentially starting a build before the source archive is complete. The identical change inSites/Http/Deployments/Create.phpline 441 has the same problem.
Reviews (1): Last reviewed commit: "feat: orchestrator build backend with De..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $deployment = $dbForProject->getDocument('deployments', $deployment->getId()); | ||
| if ($deployment->isEmpty()) { | ||
| throw new \Exception('Deployment not found'); | ||
| } | ||
|
|
||
| $resource = $dbForProject->getDocument($resource->getCollection(), $resource->getId()); |
There was a problem hiding this comment.
getAuthorization()->skip() wrappers on database fetches. Every getDocument call in the existing executeBuild method that reads deployments or resources uses $dbForProject->getAuthorization()->skip(fn () => ...), because the project-level authorization scope must be bypassed in a worker context. The two fetches here — $deployment and $resource — are bare getDocument calls, inconsistent with that established pattern. The same inconsistency appears in waitForOrchestratorBuildPath (bare find) and waitForOrchestratorSiteDetectionLogs (bare getDocument). If authorization is active in the worker, these calls will silently return empty documents, causing the handler to bail out as though the deployment or resource were deleted.
| outputDirectory: $outputDirectory | ||
| ); | ||
|
|
||
| } catch (ExecutorTimeout $error) { |
There was a problem hiding this comment.
?? '' was removed when the $outputDirectory assignment was hoisted out of the closure. $deployment->getAttribute('buildOutput') and $resource->getAttribute('outputDirectory') can both return null, leaving $outputDirectory as null when neither attribute is set. Previously the ?? '' at the call site prevented null from reaching the executor. If the executor library's createRuntime has a non-nullable string type for outputDirectory, this becomes a TypeError on sites or functions that don't configure an output directory.
| outputDirectory: $outputDirectory | |
| ); | |
| } catch (ExecutorTimeout $error) { | |
| outputDirectory: $outputDirectory ?? '' | |
| ); | |
| } catch (ExecutorTimeout $error) { |
| BuildPublisher $publisherForBuilds | ||
| ) { | ||
| $body = $request->getRawPayload(); | ||
| $secret = System::getEnv('_APP_ORCHESTRATOR_CALLBACK_SECRET', System::getEnv('_APP_OPENSSL_KEY_V1', '')); |
There was a problem hiding this comment.
_APP_OPENSSL_KEY_V1, which is the application-wide encryption key used to protect sensitive stored data. If this fallback is taken in production (because the dedicated env var is not set), the same key signs outbound callbacks AND encrypts sensitive data at rest. A leak of the callback secret — e.g., via an attacker who can read orchestrator logs — would also compromise the encryption key. Deploying a separate secret via _APP_ORCHESTRATOR_CALLBACK_SECRET should be enforced, or the fallback should use a distinct derived value.
| $secret = System::getEnv('_APP_ORCHESTRATOR_CALLBACK_SECRET', System::getEnv('_APP_OPENSSL_KEY_V1', '')); | |
| $secret = System::getEnv('_APP_ORCHESTRATOR_CALLBACK_SECRET', ''); | |
| if (empty($secret)) { | |
| throw new Exception(Exception::GENERAL_SERVER_ERROR, 'Orchestrator callback secret is not configured.'); | |
| } |
1 participant
Changes
Closes #12441