Repository navigation
fix(functions): commit a deployment's ready status with its activation - #14525
HarshMN2345 wants to merge 1 commit into
Conversation
The builds worker pointed the resource at a new deployment and only then marked it ready, so executions during a redeploy hit build_not_ready for the deployment that was still building. Write the status and the resource's deploymentId in one project-database transaction, keeping the guarantee that a deployment reported ready is the one executions reach. Manual rules live in the platform database and are still repointed before that commit, so the router serves the resource's active deployment while a manual rule's deployment is not ready yet.
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR makes deployment finalization atomic by committing the deployment status and the resource deploymentId pointer together inside a transaction, and adds a router fallback that keeps serving the previous ready deployment while platform rules catch up.
Key findings
- 🟡 Rule repoint and bus events escape the transaction — Jobs.php:751
| $applied = $dbForProject->withTransaction(function () use ($dbForProject, $dbForPlatform, $project, $resource, $deployment, $bus, $success, $terminal): int { | ||
| $applied = $dbForProject->updateDocuments('deployments', new Document(['status' => $terminal]), [ | ||
| Query::equal('$id', [$deployment->getId()]), | ||
| Query::notEqual('status', 'canceled'), | ||
| ]); | ||
|
|
||
| if ($applied > 0 && $success && $deployment->getAttribute('activate') === true && ! $resource->isEmpty()) { | ||
| $this->activate($dbForProject, $dbForPlatform, $project, $resource, $deployment, $bus); | ||
| } | ||
|
|
||
| return $applied; | ||
| }); |
There was a problem hiding this comment.
🟡 bug · medium
Rule repoint and bus events escape the transaction
activate() inside the withTransaction closure writes manual-rule pointers to the platform database and dispatches RuleUpdated events, neither of which the transaction on dbForProject covers (src/Appwrite/Platform/Modules/Functions/Workers/Jobs.php:751-762). If the transaction rolls back after activate() has run, the platform rules stay repointed at a deployment whose status update never committed, while the resource's deploymentId reverts to the previous deployment; on a first-ever activation there is no previous ready deployment for the router fallback to serve, so the custom domain routes to a still-building deployment. Consumers of the dispatched RuleUpdated events likewise act on an activation that was rolled back.
📋 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Functions/Workers/Jobs.php around lines 750-762 (finalize), the withTransaction closure calls activate(), which writes rules via $dbForPlatform and dispatches RuleUpdated bus events — side effects the $dbForProject transaction cannot roll back. Restructure so the closure only commits the project-DB writes (deployment status plus the resource's deploymentId/live fields from activate), and run the platform-rule repointing and bus dispatch after the transaction returns successfully, with an explicit error path if they fail.
Security rulesNo new WARNING or ERROR findings from security rules. 19 existing findings tracked in
|
🔵 Tier A · Mergeable after minor fixes
Deployment finalization now commits the terminal status and active resource pointer together. The router falls back to the resource’s ready deployment while a manual rule points to a deployment that is not ready yet.
Fix with agent prompt### Issue 1
src/Appwrite/Platform/Modules/Functions/Workers/Jobs.php:751
**Add coverage for the deployment handover**
This transaction is the core guarantee that a redeploy cannot expose a building deployment to executions, but the existing worker tests only cover rule activation and the deployment e2e tests do not exercise requests during a handover. Please add regression coverage with an existing ready deployment and verify that executions or routed requests keep using it until the new deployment becomes ready.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 2
Reviewed |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
| Query::equal('$id', [$deployment->getId()]), | ||
| Query::notEqual('status', 'canceled'), | ||
| ]); | ||
| $applied = $dbForProject->withTransaction(function () use ($dbForProject, $dbForPlatform, $project, $resource, $deployment, $bus, $success, $terminal): int { |
There was a problem hiding this comment.
Add coverage for the deployment handover
This transaction is the core guarantee that a redeploy cannot expose a building deployment to executions, but the existing worker tests only cover rule activation and the deployment e2e tests do not exercise requests during a handover. Please add regression coverage with an existing ready deployment and verify that executions or routed requests keep using it until the new deployment becomes ready.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Platform/Modules/Functions/Workers/Jobs.php
Line: 751
Comment:
**Add coverage for the deployment handover**
This transaction is the core guarantee that a redeploy cannot expose a building deployment to executions, but the existing worker tests only cover rule activation and the deployment e2e tests do not exercise requests during a handover. Please add regression coverage with an existing ready deployment and verify that executions or routed requests keep using it until the new deployment becomes ready.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · testing · Reply if this doesn't apply.
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after) |
This branch has not been deployed
1 participant
What does this PR do?
During a redeploy with
activate=true, the builds worker pointed the function at the new deployment and only then marked itready(the order #13741 introduced). Executions in between gotbuild_not_ready.Jobs::finalize()now writes the terminal status and runsactivate()in onedbForProjecttransaction, so the resource'sdeploymentIdandreadybecome visible together. A deployment reported ready is still the one executions reach, so the #13741 guarantee holds. A build canceled before that write is no longer activated.Rules live in the platform database and cannot join that transaction, so they are still repointed before the commit. This keeps the guarantee that a site's
deploymentIdimplies its domain serves that deployment. To close the router window for sites and function domains, a manual rule whose deployment is not ready yet now serves the resource's active deployment if that one is ready.Test Plan
php -landvendor/bin/pint --teston both changed files.Closes #14421
Related: #13741