fix: prevent cleanup exceptions from marking successful deployments as failed #7460
Conversation
…s failed Fixes #7439 where successful deployments were being marked as FAILED due to exceptions during old container cleanup. Root cause: Commit 97550f4 wrapped stop_running_container() in try-catch that re-throws ALL exceptions as DeploymentException. When old containers are already removed (a common scenario), the "No such container" error propagates and marks successful deployments as failed. Solution: Check if deployment has already succeeded (newVersionIsHealthy || force) before re-throwing exceptions from cleanup operations. Cleanup failures are logged but don't fail the deployment. - Add conditional handling in stop_running_container() catch block - Log cleanup warnings with hidden: true to avoid UI clutter - Only re-throw exceptions if deployment hasn't succeeded yet - Preserves backward compatibility and expected behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
WalkthroughThe change updates error handling in ApplicationDeploymentJob::stop_running_container. In the catch block, if the newly deployed version is healthy or a force flag is set, the code logs a warning and returns without rethrowing; otherwise it preserves the prior behavior of throwing a DeploymentException. No public signatures were changed. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.php📄 CodeRabbit inference engine (CLAUDE.md)
Files:
app/Jobs/**/*.php📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)app/Jobs/ApplicationDeploymentJob.php (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
app/Jobs/ApplicationDeploymentJob.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: Use PHP 8.4 constructor property promotion and typed properties
Follow PSR-12 coding standards and run./vendor/bin/pintbefore committing
Use Eloquent ORM for database interactions, avoid raw queries
Use Laravel's built-in mocking and Mockery for testing external services and dependencies
Use database transactions for critical operations that modify multiple related records
Leverage query scopes in Eloquent models for reusable, chainable query logic
Never log or expose sensitive data (passwords, tokens, API keys, SSH keys) in logs or error messages
Always validate user input using Form Requests, Rules, or explicit validation methods
UsehandleError()helper for consistent error handling and logging
Use eager loading (with(), load()) to prevent N+1 queries when accessing related models
Use chunking for large data operations to avoid memory exhaustion
Implement caching for frequently accessed data using Laravel's cache helpers
Write descriptive variable and method names that clearly express intent
Keep methods small and focused on a single responsibility
Document complex logic with clear comments explaining the 'why' not just the 'what'Always run code formatting with
./vendor/bin/pintbefore committing code
Files:
app/Jobs/ApplicationDeploymentJob.php
app/Jobs/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Queue heavy operations with Laravel Horizon instead of executing synchronously
Files:
app/Jobs/ApplicationDeploymentJob.php
🧬 Code graph analysis (1)
app/Jobs/ApplicationDeploymentJob.php (1)
app/Models/ApplicationDeploymentQueue.php (1)
addLogEntry(127-162)
🪛 PHPMD (2.15.0)
app/Jobs/ApplicationDeploymentJob.php
3197-3197: Avoid using static access to class '\Log' in method 'stop_running_container'. (undefined)
(StaticAccess)
There was a problem hiding this comment.
Pull request overview
This PR attempts to prevent cleanup exceptions from failing deployments that have already succeeded by adding conditional exception handling in the stop_running_container() method. The fix checks if the new version is healthy or if the $force parameter is true before suppressing cleanup exceptions.
Key changes:
- Added conditional exception handling to check
$this->newVersionIsHealthy || $forcebefore suppressing cleanup exceptions - Logged cleanup warnings with
hidden: trueto avoid cluttering deployment logs - Only re-throws exceptions if the deployment hasn't succeeded yet
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If new version is healthy, this is just cleanup - don't fail the deployment | ||
| if ($this->newVersionIsHealthy || $force) { | ||
| $this->application_deployment_queue->addLogEntry( | ||
| "Warning: Could not remove old container: {$e->getMessage()}", | ||
| 'stderr', | ||
| hidden: true | ||
| ); | ||
|
|
||
| return; // Don't re-throw - cleanup failures shouldn't fail successful deployments | ||
| } | ||
|
|
||
| // Only re-throw if deployment hasn't succeeded yet | ||
| throw new DeploymentException("Failed to stop running container: {$e->getMessage()}", $e->getCode(), $e); |
There was a problem hiding this comment.
The new exception handling logic in stop_running_container() lacks test coverage. Given the critical nature of this code (it determines whether deployments succeed or fail), tests should be added to verify:
- Cleanup exceptions are suppressed when the new version is healthy (rolling update scenario)
- Cleanup exceptions fail the deployment when called with
force=truebefore the new container starts - The logging behavior with
hidden: trueworks correctly
Similar deployment logic has test coverage in tests/Unit/ApplicationDeploymentErrorLoggingTest.php, which could serve as a reference.
| } | ||
| } catch (Exception $e) { | ||
| // If new version is healthy, this is just cleanup - don't fail the deployment | ||
| if ($this->newVersionIsHealthy || $force) { |
There was a problem hiding this comment.
Logic error: The condition incorrectly suppresses cleanup exceptions when $force=true. When stop_running_container(force: true) is called at lines 723 and 1744, it happens BEFORE the new container starts, so newVersionIsHealthy is still false. If cleanup fails at that point (e.g., Docker daemon unavailable), the deployment should fail since we haven't successfully started anything yet. The correct condition should be if ($this->newVersionIsHealthy && !$force) - only suppress exceptions when the new version is confirmed healthy AND we're in the post-health-check cleanup phase (when $force=false, as at line 1751).
| if ($this->newVersionIsHealthy || $force) { | |
| if ($this->newVersionIsHealthy && !$force) { |
Changes
stop_running_container()catch block to detect when deployment has already succeededhidden: trueto avoid cluttering deployment logsIssues
Description
Successful deployments were being marked as FAILED due to exceptions during old container cleanup. When old containers are already removed (a common scenario), the "No such container" error was propagating and failing the entire deployment.
The fix checks if the new version is healthy before re-throwing cleanup exceptions, allowing the deployment to succeed while still logging the cleanup issues for debugging.