diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index eefc9880bb..be7d747c46 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -87,8 +87,6 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue public static int $batch_counter = 0; - private bool $newVersionIsHealthy = false; - private ApplicationDeploymentQueue $application_deployment_queue; private Application $application; @@ -2725,7 +2723,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue // Implement healthcheck for swarm } else { if ($this->application->isHealthcheckDisabled() && $this->application->custom_healthcheck_found === false) { - $this->newVersionIsHealthy = true; + $this->markNewVersionHealthy(true); return; } @@ -2773,12 +2771,12 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue $healthCheckStatus = $this->trimmedSavedOutput('health_check')->replace('"', '')->value(); if ($healthCheckStatus === 'healthy') { - $this->newVersionIsHealthy = true; + $this->markNewVersionHealthy(true); $this->application->update(['status' => 'running']); $this->application_deployment_queue->addLogEntry('New container is healthy.'); break; } elseif ($healthCheckStatus === 'unhealthy') { - $this->newVersionIsHealthy = false; + $this->markNewVersionHealthy(false); $this->application_deployment_queue->addLogEntry('New container is unhealthy.', type: 'error'); $this->query_logs(); break; @@ -2800,6 +2798,24 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue } } + /** + * Laravel runs failed() on a fresh copy of the job, so the health of the new version is kept outside the job instance. + */ + private function markNewVersionHealthy(bool $healthy): void + { + $key = "deployment-new-version-healthy:{$this->deployment_uuid}"; + if ($healthy) { + Cache::put($key, true, now()->addDay()); + } else { + Cache::forget($key); + } + } + + private function newVersionIsHealthy(): bool + { + return Cache::has("deployment-new-version-healthy:{$this->deployment_uuid}"); + } + private function query_logs() { $escapedContainerName = escapeshellarg($this->container_name); @@ -2830,7 +2846,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue if ($this->use_build_server) { $this->server = $this->build_server; } - $this->newVersionIsHealthy = true; + $this->markNewVersionHealthy(true); $this->generate_image_names(); $this->application_deployment_queue->addLogEntry("Starting pull request (#{$this->pull_request_id}) deployment of {$this->customRepository}:{$this->application->git_branch}."); $this->prepare_builder_image(); @@ -4944,7 +4960,7 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); { try { $this->application_deployment_queue->addLogEntry('Removing old containers.'); - if ($this->newVersionIsHealthy || $force) { + if ($this->newVersionIsHealthy() || $force) { if ($this->application->settings->is_consistent_container_name_enabled) { $containers = getCurrentApplicationContainerStatus($this->server, $this->application, $this->pull_request_id); $this->containerNamesToRemove($containers)->each(function (string $containerName) { @@ -4973,7 +4989,7 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); } } catch (Exception $e) { // If new version is healthy, this is just cleanup - don't fail the deployment - if ($this->newVersionIsHealthy || $force) { + if ($this->newVersionIsHealthy() || $force) { $this->application_deployment_queue->addLogEntry( "Warning: Could not remove old container: {$e->getMessage()}", 'stderr', @@ -6144,7 +6160,7 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); // 69420 means failed to push the image to the registry, so we don't need to remove the new version as it is the currently running one if ($this->application->settings->is_consistent_container_name_enabled || $this->pull_request_id !== 0) { // do not remove already running container for PR deployments - } elseif ($this->newVersionIsHealthy) { + } elseif ($this->newVersionIsHealthy()) { // The new container passed its health check, so the old container may already be removed. // A later failure (additional destinations, queue advancement, ...) must not remove the only running version. $this->application_deployment_queue->addLogEntry('Deployment failed after the new version became healthy. Keeping the new version of your application running.', 'stderr'); diff --git a/tests/Feature/ApplicationDeploymentFailedKeepsHealthyContainerTest.php b/tests/Feature/ApplicationDeploymentFailedKeepsHealthyContainerTest.php new file mode 100644 index 0000000000..8b3fd142f5 --- /dev/null +++ b/tests/Feature/ApplicationDeploymentFailedKeepsHealthyContainerTest.php @@ -0,0 +1,94 @@ + InstanceSettings::firstOrCreate(['id' => 0])); + + $team = Team::factory()->create(); + $privateKey = PrivateKey::factory()->create(['team_id' => $team->id]); + $server = Server::factory()->create(['team_id' => $team->id, 'private_key_id' => $privateKey->id]); + $destination = StandaloneDocker::where('server_id', $server->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = $project->environments()->first() + ?? Environment::factory()->create(['project_id' => $project->id]); + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + 'build_pack' => 'dockerfile', + ]); + + $this->deployment = ApplicationDeploymentQueue::create([ + 'application_id' => $application->id, + 'deployment_uuid' => fake()->uuid(), + 'status' => 'in_progress', + 'server_id' => $server->id, + 'destination_id' => $destination->id, + 'commit' => 'HEAD', + 'pull_request_id' => 0, + ]); + + Process::fake([ + '*State.Health.Status*' => Process::result(output: "\"healthy\"\n"), + '*' => Process::result(output: ''), + ]); + Queue::fake(); + Notification::fake(); + Sleep::fake(); +}); + +/** + * Laravel calls failed() on a copy of the job that it unserializes from the payload taken at dispatch, + * so state that handle() keeps only in memory is lost there. + */ +function failQueuedCopy(string $queuedCopy): void +{ + unserialize($queuedCopy)->failed(new RuntimeException('Late failure')); +} + +function removesContainer(string $containerName): Closure +{ + return fn (PendingProcess $process) => str_contains((string) $process->command, 'docker rm -f') + && str_contains((string) $process->command, $containerName); +} + +it('keeps the new container when a deployment fails after the new container became healthy', function () { + $job = new ApplicationDeploymentJob($this->deployment->id); + $queuedCopy = serialize(clone $job); + $containerName = (fn () => $this->container_name)->call($job); + (fn () => $this->health_check())->call($job); + failQueuedCopy($queuedCopy); + + Process::assertNotRan(removesContainer($containerName)); + expect($this->deployment->fresh()->logs)->toContain('Keeping the new version of your application running.'); +}); + +it('removes the new container when a deployment fails before the new container became healthy', function () { + $job = new ApplicationDeploymentJob($this->deployment->id); + $queuedCopy = serialize(clone $job); + $containerName = (fn () => $this->container_name)->call($job); + + failQueuedCopy($queuedCopy); + + Process::assertRan(removesContainer($containerName)); + expect($this->deployment->fresh()->logs)->toContain('Removing the new version of your application.'); +}); diff --git a/tests/Unit/ApplicationDeploymentErrorLoggingTest.php b/tests/Unit/ApplicationDeploymentErrorLoggingTest.php index 39ee65b4d3..4c94ebd3b5 100644 --- a/tests/Unit/ApplicationDeploymentErrorLoggingTest.php +++ b/tests/Unit/ApplicationDeploymentErrorLoggingTest.php @@ -6,6 +6,9 @@ use App\Models\Application; use App\Models\ApplicationDeploymentQueue; use Illuminate\Contracts\Queue\Job; use Illuminate\Queue\TimeoutExceededException; +use Tests\TestCase; + +uses(TestCase::class); /** * Test to verify that deployment errors are properly logged with comprehensive details. @@ -91,6 +94,7 @@ it('logs comprehensive error details when failed() is called', function () { $containerNameProperty = $reflection->getProperty('container_name'); $containerNameProperty->setAccessible(true); $containerNameProperty->setValue($job, 'test-container'); + $reflection->getProperty('deployment_uuid')->setValue($job, 'error-logging-test'); // Mock the failDeployment method to prevent errors $job->shouldReceive('failDeployment')->andReturn(); @@ -197,6 +201,7 @@ it('handles exceptions with no message gracefully', function () { $containerNameProperty = $reflection->getProperty('container_name'); $containerNameProperty->setAccessible(true); $containerNameProperty->setValue($job, 'test-container'); + $reflection->getProperty('deployment_uuid')->setValue($job, 'error-logging-test'); $job->shouldReceive('failDeployment')->andReturn(); $job->shouldReceive('execute_remote_command')->andReturn(); @@ -288,6 +293,7 @@ it('logs error code 0 correctly', function () { $containerNameProperty = $reflection->getProperty('container_name'); $containerNameProperty->setAccessible(true); $containerNameProperty->setValue($job, 'test-container'); + $reflection->getProperty('deployment_uuid')->setValue($job, 'error-logging-test'); $job->shouldReceive('failDeployment')->andReturn(); $job->shouldReceive('execute_remote_command')->andReturn(); @@ -393,6 +399,7 @@ it('logs the configured timeout when the deployment times out', function () { $containerNameProperty = $reflection->getProperty('container_name'); $containerNameProperty->setValue($job, 'test-container'); + $reflection->getProperty('deployment_uuid')->setValue($job, 'error-logging-test'); $job->shouldReceive('failDeployment')->andReturn(); $job->shouldReceive('execute_remote_command')->andReturn(); diff --git a/tests/Unit/ApplicationDeploymentFailedContainerCleanupTest.php b/tests/Unit/ApplicationDeploymentFailedContainerCleanupTest.php index 39d5b5c278..6fe9a7ef4e 100644 --- a/tests/Unit/ApplicationDeploymentFailedContainerCleanupTest.php +++ b/tests/Unit/ApplicationDeploymentFailedContainerCleanupTest.php @@ -4,11 +4,14 @@ use App\Jobs\ApplicationDeploymentJob; use App\Models\Application; use App\Models\ApplicationDeploymentQueue; use App\Models\ApplicationSetting; +use Tests\TestCase; + +uses(TestCase::class); /** * @return array> */ -function runFailedDeploymentCleanup(bool $consistentContainerName, ?string $customInternalName, int $pullRequestId, bool $newVersionIsHealthy = false): array +function runFailedDeploymentCleanup(bool $consistentContainerName, ?string $customInternalName, int $pullRequestId): array { $settings = new ApplicationSetting; $settings->setRawAttributes([ @@ -36,8 +39,8 @@ function runFailedDeploymentCleanup(bool $consistentContainerName, ?string $cust $reflection->getProperty('application_deployment_queue')->setValue($job, $queue); $reflection->getProperty('application')->setValue($job, $application); $reflection->getProperty('pull_request_id')->setValue($job, $pullRequestId); + $reflection->getProperty('deployment_uuid')->setValue($job, 'cleanup-test-deployment'); $reflection->getProperty('container_name')->setValue($job, 'new-container-20261001T120000'); - $reflection->getProperty('newVersionIsHealthy')->setValue($job, $newVersionIsHealthy); $job->failed(new RuntimeException('Health check failed')); @@ -62,14 +65,3 @@ it('keeps the running container when consistent container names are enabled', fu it('keeps the running container for pull request deployments', function () { expect(runFailedDeploymentCleanup(consistentContainerName: false, customInternalName: 'my-api', pullRequestId: 12))->toBeEmpty(); }); - -it('keeps the new container when the deployment fails after the new container became healthy', function () { - expect(runFailedDeploymentCleanup(consistentContainerName: false, customInternalName: null, pullRequestId: 0, newVersionIsHealthy: true))->toBeEmpty(); -}); - -it('removes the new container when the deployment fails before the new container became healthy', function () { - $remoteCommands = runFailedDeploymentCleanup(consistentContainerName: false, customInternalName: null, pullRequestId: 0, newVersionIsHealthy: false); - - expect($remoteCommands)->toHaveCount(1) - ->and($remoteCommands[0][0])->toContain('new-container-20261001T120000'); -});