diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index e9203cbab4..6a3101e1cc 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -310,6 +310,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue $this->application_deployment_queue->refresh(); if ($this->application_deployment_queue->status === ApplicationDeploymentStatus::CANCELLED_BY_USER->value) { $this->application_deployment_queue->addLogEntry('Deployment was cancelled before starting.'); + $this->handleCancelledDeployment(); return; } @@ -659,7 +660,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue /** * An inline Dockerfile image has no commit tag. The main server builds it and pushes it with the * `latest` tag just before it queues the additional servers, so an additional server pulls that - * image instead of building its own. A failed pull fails the deployment. + * image instead of building its own. A missing image fails the deployment with a clear message. */ private function pull_image_for_additional_server(): bool { @@ -667,11 +668,28 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return false; } + $image = escapeshellarg($this->production_image_name); $this->application_deployment_queue->addLogEntry("Pulling image ({$this->production_image_name}) that the main server pushed. Build step skipped."); - $this->execute_remote_command([ - 'docker pull '.escapeshellarg($this->production_image_name), - 'hidden' => true, - ]); + $this->execute_remote_command( + [ + "docker pull {$image}", + 'hidden' => true, + 'ignore_errors' => true, + ], + [ + "docker images -q {$image} 2>/dev/null", + 'hidden' => true, + 'save' => 'local_image_found', + ], + ); + + if (str($this->saved_outputs->get('local_image_found'))->isEmpty()) { + $message = "Image ({$this->production_image_name}) not found in the registry; the main server must push it first."; + if (! $this->server->canBuildApplications()) { + $message .= " The server ({$this->server->name}) is set to deployments only and cannot build it."; + } + throw new DeploymentException($message); + } return true; } @@ -5614,12 +5632,24 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); if ($this->application_deployment_queue->status === ApplicationDeploymentStatus::CANCELLED_BY_USER->value) { $this->application_deployment_queue->addLogEntry('Deployment cancelled by user, stopping execution.'); + $this->handleCancelledDeployment(); throw new DeploymentException('Deployment cancelled by user', 69420); } return false; } + /** + * A cancelled deployment on an additional server can be the last one of a multi-server + * deployment, so it must also trigger the combined notification. + */ + private function handleCancelledDeployment(): void + { + if (filled($this->application_deployment_queue->parent_deployment_uuid)) { + $this->sendMultiServerDeploymentNotification($this->application_deployment_queue->parent_deployment_uuid); + } + } + /** * Update the deployment status in the database. */ diff --git a/bootstrap/helpers/applications.php b/bootstrap/helpers/applications.php index 8dc623af41..8fe4c0d393 100644 --- a/bootstrap/helpers/applications.php +++ b/bootstrap/helpers/applications.php @@ -63,7 +63,7 @@ function queue_application_deployment(Application $application, string $deployme ]; } - return ApplicationDeploymentQueue::create([ + $deployment = ApplicationDeploymentQueue::create([ 'application_id' => $application_id, 'application_name' => $application->name, 'server_id' => $server_id, @@ -83,13 +83,24 @@ function queue_application_deployment(Application $application, string $deployme 'only_this_server' => $only_this_server, 'parent_deployment_uuid' => $parent_deployment_uuid, ]); + + // Decide whether to start while the application and server rows are still locked, + // so concurrent requests cannot both see an idle queue and start two deployments. + $started = $no_questions_asked || next_queuable($server_id, $application_id, $commit, $pull_request_id); + if ($started) { + $deployment->update([ + 'status' => ApplicationDeploymentStatus::IN_PROGRESS->value, + ]); + } + + return ['deployment' => $deployment, 'started' => $started]; }); - if (is_array($admission)) { + if (! isset($admission['deployment'])) { return $admission; } - $deployment = $admission; + $deployment = $admission['deployment']; if (auth()->check() && ! $is_webhook && ! $is_api && ! $rollback) { auditLog($restart_only ? 'ui.application.restarted' : 'ui.application.deployed', [ @@ -100,17 +111,7 @@ function queue_application_deployment(Application $application, string $deployme ]); } - if ($no_questions_asked) { - $deployment->update([ - 'status' => ApplicationDeploymentStatus::IN_PROGRESS->value, - ]); - ApplicationDeploymentJob::dispatch( - application_deployment_queue_id: $deployment->id, - ); - } elseif (next_queuable($server_id, $application_id, $commit, $pull_request_id)) { - $deployment->update([ - 'status' => ApplicationDeploymentStatus::IN_PROGRESS->value, - ]); + if ($admission['started']) { ApplicationDeploymentJob::dispatch( application_deployment_queue_id: $deployment->id, ); @@ -141,19 +142,46 @@ function queue_next_deployment(Application $application) ->sortBy('created_at'); foreach ($queued_deployments as $next_deployment) { - // Check if this queued deployment can actually run - if (next_queuable($next_deployment->server_id, $next_deployment->application_id, $next_deployment->commit, $next_deployment->pull_request_id)) { - $next_deployment->update([ - 'status' => ApplicationDeploymentStatus::IN_PROGRESS->value, - ]); - - ApplicationDeploymentJob::dispatch( - application_deployment_queue_id: $next_deployment->id, - ); - } + start_queued_deployment($next_deployment); } } +/** + * Start a queued deployment if it can run now. The check and the status change run under + * the same application and server row locks as queue admission, so two workers cannot both + * start the same deployment or exceed the per-application and per-server limits. + */ +function start_queued_deployment(ApplicationDeploymentQueue $deployment): bool +{ + $started = DB::transaction(function () use ($deployment): bool { + Application::query()->whereKey($deployment->application_id)->lockForUpdate()->first(); + Server::query()->whereKey($deployment->server_id)->lockForUpdate()->first(); + $current = ApplicationDeploymentQueue::query()->whereKey($deployment->id)->lockForUpdate()->first(); + + if (! $current || $current->status !== ApplicationDeploymentStatus::QUEUED->value) { + return false; + } + + if (! next_queuable($current->server_id, $current->application_id, $current->commit, $current->pull_request_id)) { + return false; + } + + $current->update([ + 'status' => ApplicationDeploymentStatus::IN_PROGRESS->value, + ]); + + return true; + }); + + if ($started) { + ApplicationDeploymentJob::dispatch( + application_deployment_queue_id: $deployment->id, + ); + } + + return $started; +} + function next_queuable(string $server_id, string $application_id, string $commit = 'HEAD', int $pull_request_id = 0): bool { // Check if there's already a deployment in progress for this application with the same pull_request_id @@ -188,19 +216,8 @@ function next_after_cancel(?Server $server = null) ->get() ->sortBy('created_at'); - if ($next_found->count() > 0) { - foreach ($next_found as $next) { - // Use next_queuable to properly check if this deployment can run - if (next_queuable($next->server_id, $next->application_id, $next->commit, $next->pull_request_id)) { - $next->update([ - 'status' => ApplicationDeploymentStatus::IN_PROGRESS->value, - ]); - - ApplicationDeploymentJob::dispatch( - application_deployment_queue_id: $next->id, - ); - } - } + foreach ($next_found as $next) { + start_queued_deployment($next); } } } diff --git a/tests/Feature/MultiServerApplicationTest.php b/tests/Feature/MultiServerApplicationTest.php index 13cac656f7..260c5747d7 100644 --- a/tests/Feature/MultiServerApplicationTest.php +++ b/tests/Feature/MultiServerApplicationTest.php @@ -273,6 +273,46 @@ describe('combined deployment notification', function () { Notification::assertNotSentTo($this->team, DeploymentFailed::class); }); + test('sends the summary when the last additional server deployment is cancelled while running', function () { + $parentUuid = new_public_id(); + Cache::put("multi-server-deployment-queued:{$parentUuid}", true); + createMultiServerDeployment($this->application, $this->extraServer, ApplicationDeploymentStatus::FINISHED->value, $parentUuid); + $cancelled = createMultiServerDeployment($this->application, $this->extraServer, ApplicationDeploymentStatus::CANCELLED_BY_USER->value, $parentUuid); + $job = makeMultiServerDeploymentJob(['application' => $this->application, 'application_deployment_queue' => $cancelled]); + + expect(fn () => callMultiServerDeploymentJob($job, 'failDeployment')) + ->toThrow(DeploymentException::class, 'Deployment cancelled by user'); + + Notification::assertSentToTimes($this->team, DeploymentFailed::class, 1); + Notification::assertSentTo($this->team, DeploymentFailed::class, fn (DeploymentFailed $notification) => $notification->deployment_uuid === $cancelled->deployment_uuid); + Notification::assertNotSentTo($this->team, DeploymentSuccess::class); + }); + + test('sends the summary when the last additional server deployment is cancelled before it starts', function () { + $parentUuid = new_public_id(); + Cache::put("multi-server-deployment-queued:{$parentUuid}", true); + createMultiServerDeployment($this->application, $this->extraServer, ApplicationDeploymentStatus::FINISHED->value, $parentUuid); + $cancelled = createMultiServerDeployment($this->application, $this->extraServer, ApplicationDeploymentStatus::CANCELLED_BY_USER->value, $parentUuid); + [$job] = makeMultiServerDeploymentJob(['application' => $this->application, 'application_deployment_queue' => $cancelled]); + + $job->handle(); + + Notification::assertSentToTimes($this->team, DeploymentFailed::class, 1); + Notification::assertSentTo($this->team, DeploymentFailed::class, fn (DeploymentFailed $notification) => $notification->deployment_uuid === $cancelled->deployment_uuid); + }); + + test('does not notify when a cancelled additional server deployment is not the last one', function () { + $parentUuid = new_public_id(); + Cache::put("multi-server-deployment-queued:{$parentUuid}", true); + createMultiServerDeployment($this->application, $this->extraServer, ApplicationDeploymentStatus::IN_PROGRESS->value, $parentUuid); + $cancelled = createMultiServerDeployment($this->application, $this->extraServer, ApplicationDeploymentStatus::CANCELLED_BY_USER->value, $parentUuid); + [$job] = makeMultiServerDeploymentJob(['application' => $this->application, 'application_deployment_queue' => $cancelled]); + + $job->handle(); + + Notification::assertNothingSent(); + }); + test('sends a failed notification that links to the failed server deployment', function () { $parentUuid = new_public_id(); Cache::put("multi-server-deployment-queued:{$parentUuid}", true); @@ -297,15 +337,21 @@ class MultiServerRecordingDeploymentJob extends ApplicationDeploymentJob public function __construct() {} + /** Output that `docker images -q` returns; empty means the image is missing. */ + public string $localImageOutput = 'sha256:0123456789ab'; + public function execute_remote_command(...$commands): void { foreach ($commands as $command) { $this->recordedCommands[] = $command['command'] ?? $command[0]; + if (isset($command['save'])) { + (new ReflectionProperty(ApplicationDeploymentJob::class, 'saved_outputs'))->getValue($this)->put($command['save'], $this->localImageOutput); + } } } } -function makeInlineDockerfileJob(Application $application, bool $isAdditionalServer): array +function makeInlineDockerfileJob(Application $application, bool $isAdditionalServer, ?Server $server = null): array { $reflection = new ReflectionClass(ApplicationDeploymentJob::class); $job = new MultiServerRecordingDeploymentJob; @@ -316,6 +362,8 @@ function makeInlineDockerfileJob(Application $application, bool $isAdditionalSer 'application_deployment_queue' => $queue, 'is_this_additional_server' => $isAdditionalServer, 'pull_request_id' => 0, + 'saved_outputs' => collect(), + 'server' => $server ?? $application->destination->server, ] as $name => $value) { $reflection->getProperty($name)->setValue($job, $value); } @@ -336,7 +384,19 @@ describe('inline Dockerfile image on additional servers', function () { [$job, $reflection] = makeInlineDockerfileJob($this->application->fresh(), true); expect($reflection->getMethod('pull_image_for_additional_server')->invoke($job))->toBeTrue() - ->and($job->recordedCommands)->toBe(["docker pull 'registry.example.com/nginx-multi:latest'"]); + ->and($job->recordedCommands)->toBe([ + "docker pull 'registry.example.com/nginx-multi:latest'", + "docker images -q 'registry.example.com/nginx-multi:latest' 2>/dev/null", + ]); + }); + + test('a deployments only server explains that the main server must push a missing image', function () { + $this->extraServer->settings->update(['server_role' => 'deployment']); + [$job, $reflection] = makeInlineDockerfileJob($this->application->fresh(), true, $this->extraServer->fresh()); + $job->localImageOutput = ''; + + expect(fn () => $reflection->getMethod('pull_image_for_additional_server')->invoke($job)) + ->toThrow(DeploymentException::class, 'Image (registry.example.com/nginx-multi:latest) not found in the registry; the main server must push it first. The server (extra-server) is set to deployments only and cannot build it.'); }); test('the main server builds the image', function () { diff --git a/tests/Feature/QueueApplicationDeploymentCommitTest.php b/tests/Feature/QueueApplicationDeploymentCommitTest.php index 992856bae5..422047979b 100644 --- a/tests/Feature/QueueApplicationDeploymentCommitTest.php +++ b/tests/Feature/QueueApplicationDeploymentCommitTest.php @@ -61,6 +61,62 @@ describe('queue_application_deployment commit resolution', function () { expect($creationLevel)->toBeGreaterThan($initialLevel); }); + test('starts a deployment only while the admission transaction is open', function () { + $application = makeApplication($this->environment->id, $this->destination->id, 'HEAD'); + $initialLevel = DB::transactionLevel(); + $startLevel = null; + + ApplicationDeploymentQueue::updated(function (ApplicationDeploymentQueue $deployment) use (&$startLevel): void { + if ($deployment->status === ApplicationDeploymentStatus::IN_PROGRESS->value) { + $startLevel = DB::transactionLevel(); + } + }); + + $result = queue_application_deployment($application, 'atomic-start'); + + expect($result['status'])->toBe('queued') + ->and($startLevel)->toBeGreaterThan($initialLevel); + Bus::assertDispatchedTimes(ApplicationDeploymentJob::class, 1); + }); + + test('starts the next queued deployment only while a transaction is open', function (string $trigger) { + $application = makeApplication($this->environment->id, $this->destination->id, 'HEAD'); + $this->server->settings->update(['concurrent_builds' => 0]); + queue_application_deployment($application, 'waiting-deployment'); + $this->server->settings->update(['concurrent_builds' => 1]); + $initialLevel = DB::transactionLevel(); + $startLevel = null; + + ApplicationDeploymentQueue::updated(function (ApplicationDeploymentQueue $deployment) use (&$startLevel): void { + if ($deployment->status === ApplicationDeploymentStatus::IN_PROGRESS->value) { + $startLevel = DB::transactionLevel(); + } + }); + + $trigger === 'queue_next_deployment' + ? queue_next_deployment($application) + : next_after_cancel($this->server); + + expect($startLevel)->toBeGreaterThan($initialLevel) + ->and(ApplicationDeploymentQueue::query()->sole()->status)->toBe(ApplicationDeploymentStatus::IN_PROGRESS->value); + Bus::assertDispatchedTimes(ApplicationDeploymentJob::class, 1); + })->with(['queue_next_deployment', 'next_after_cancel']); + + test('does not start a queued deployment that another worker already started', function () { + $application = makeApplication($this->environment->id, $this->destination->id, 'HEAD'); + $this->server->settings->update(['concurrent_builds' => 0]); + queue_application_deployment($application, 'stale-deployment'); + $this->server->settings->update(['concurrent_builds' => 5]); + $stale = ApplicationDeploymentQueue::query()->sole(); + + ApplicationDeploymentQueue::query()->whereKey($stale->id)->update([ + 'status' => ApplicationDeploymentStatus::IN_PROGRESS->value, + ]); + + expect(start_queued_deployment($stale))->toBeFalse(); + Bus::assertNotDispatched(ApplicationDeploymentJob::class); + }); + test('skips a matching commit without creating or dispatching a second deployment', function () { $application = makeApplication($this->environment->id, $this->destination->id, 'HEAD');