fix: queue deployment on multiple app from same repo sometimes failed #4554
Conversation
Signed-off-by: David Anyatonwu <davidanyatonwu@gmail.com>
|
@coderabbitai review |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request refines the deployment process across two areas. In the deployment job, it adds validation checks on critical entities, enhances error handling, introduces database transaction management and refactors container operations, along with improved logging and notifications. In parallel, the helper functions for application deployments are updated to wrap their operations in transactions to ensure atomicity and prevent race conditions during concurrent deployments. Changes
Sequence Diagram(s)sequenceDiagram
participant J as ApplicationDeploymentJob
participant DB as Database
participant C as ContainerManager
participant N as Notification System
J->>J: initialize_deployment()
J->>DB: DB.beginTransaction()
J->>J: Verify deployment status (IN_PROGRESS)
alt Valid Status
J->>C: Execute container management operations
J->>N: Log actions and dispatch success notification
J->>DB: DB.commit()
else Invalid Status
J->>DB: DB.rollBack()
J->>N: Log error and dispatch failure notification
end
sequenceDiagram
participant F as Deployment Helper Functions
participant DB as Database
participant Q as Deployment Queue
F->>DB: DB.beginTransaction()
F->>Q: Lock/update deployment record
alt Deployment meets criteria
F->>DB: Commit transaction
else Issue detected
F->>DB: Roll back transaction
end
Assessment against linked issues
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
bootstrap/helpers/applications.php (2)
96-124: Inconsistent usage of the deployment status constants.Here you rely on
'status', 'in_progress'whereas in other places (lines 34, 37, etc.) the enum constantApplicationDeploymentStatus::IN_PROGRESS->valueis used. Consider unifying for clarity.
129-153: Unify'in_progress'vs.ApplicationDeploymentStatus::IN_PROGRESS->value.Passing string literals in some subqueries while using the enum in others can lead to confusion. Keeping them consistent prevents accidental mismatches later.
app/Jobs/ApplicationDeploymentJob.php (2)
391-430:$cleanup_attemptedlogic always set to true.Because
$cleanup_attemptedis unconditionally set to true at line 393, theif (! $cleanup_attempted)branch at line 420 is never triggered. Revisit this logic if the intent was to invokequeue_next_deployment($this->application)under certain cleanup failures.
432-478: Possible code duplication ininitialize_deployment().This method reimplements setup steps also present in the constructor. If
initialize_deployment()is not explicitly called, consider removing dead code or unifying initialization to follow DRY principles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Jobs/ApplicationDeploymentJob.php(5 hunks)bootstrap/helpers/applications.php(1 hunks)
🔇 Additional comments (12)
bootstrap/helpers/applications.php (4)
9-9: Good addition for transaction handling.No issues here. Using the DB facade is appropriate to support atomic operations and prevent race conditions.
14-72: Potential null reference for$application->destination->server.While the transaction-based concurrency approach using
DB::transaction()and->lockForUpdate()is solid, consider adding a guard to ensure$application->destinationand$application->destination->serverare not null to avoid unexpected runtime errors in lines 19–21.
76-90: Force deployment logic is well-isolated.The transaction ensures concurrency is properly handled when force-starting a deployment. This change prevents race conditions while allowing an override, which looks intentional and correct.
158-184: Good transaction-based flow innext_after_cancel.Locking the queued deployments for update and verifying concurrency before marking it as in-progress ensures correct coordination of deployments. This is an excellent way to handle race conditions.
app/Jobs/ApplicationDeploymentJob.php (8)
29-29: DB facade import is properly aligned with the new transaction logic.No issues found. This import supports the transactional approach introduced in the handle method.
176-182: Constructor validation for the deployment queue.Ensuring the queue record exists up front prevents avoidable downstream failures. A solid defensive approach.
196-203: Explicit check for the application instance.Verifying that
$this->applicationis valid and adjusting$this->restart_onlyis a robust way to avoid hidden edge cases.
211-216: Loading and validating the server.This step preempts potential null references and clarifies that a valid server is mandatory for deployment tasks.
219-222: Destination validation prevents misconfiguration issues.Throwing an exception for a missing destination helps fail fast in case of bad input or data corruption.
270-360: Transactional concurrency checks for deployment queue.Wrapping concurrency validations in a transaction effectively ensures only one job can set up or confirm deployment status at once. This is well-structured for preventing race conditions.
365-369: Marking deployment as FINISHED on success.Officially completing the deployment and firing
ApplicationStatusChangedis consistent with a successful termination flow.
370-390: Clean rollback on error.Catching exceptions, rolling back, and marking the queue status as FAILED properly reflects a failed deployment scenario. The re-throw ensures the job is flagged as failed in the queue system.
|
is there any updates on this ? |
Description
This PR fixes issue #4391 where queue deployment on multiple apps from the same repo sometimes fails.
/claim #4391
Fixes: #4391