From 33eb1dab7927985cae819fb2447c074c19d0efa2 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 24 Sep 2026 14:07:21 +0200 Subject: [PATCH] fix(deployments): prevent deployment queue admission races (#11987) --- bootstrap/helpers/applications.php | 89 ++++++++++--------- .../QueueApplicationDeploymentCommitTest.php | 88 +++++++++++++++++- 2 files changed, 133 insertions(+), 44 deletions(-) diff --git a/bootstrap/helpers/applications.php b/bootstrap/helpers/applications.php index 7743304c34..2ffb959058 100644 --- a/bootstrap/helpers/applications.php +++ b/bootstrap/helpers/applications.php @@ -9,6 +9,7 @@ use App\Models\ApplicationDeploymentQueue; use App\Models\EnvironmentVariable; use App\Models\Server; use App\Models\StandaloneDocker; +use Illuminate\Support\Facades\DB; use Spatie\Url\Url; function queue_application_deployment(Application $application, string $deployment_uuid, ?int $pull_request_id = 0, ?string $commit = null, bool $force_rebuild = false, bool $is_webhook = false, bool $is_api = false, bool $restart_only = false, ?string $git_type = null, bool $no_questions_asked = false, ?Server $server = null, ?StandaloneDocker $destination = null, bool $only_this_server = false, bool $rollback = false, ?string $docker_registry_image_tag = null) @@ -30,32 +31,30 @@ function queue_application_deployment(Application $application, string $deployme $destination_id = $destination->id; } - // Check if the deployment queue is full for this server - $serverForQueueCheck = $server ?? Server::find($server_id); - $queue_limit = $serverForQueueCheck->settings->deployment_queue_limit ?? 25; - $queued_count = ApplicationDeploymentQueue::where('server_id', $server_id) - ->where('status', ApplicationDeploymentStatus::QUEUED->value) - ->count(); + $admission = DB::transaction(function () use ($application, $application_id, $commit, $deployment_uuid, $deployment_url, $destination_id, $docker_registry_image_tag, $force_rebuild, $git_type, $is_api, $is_webhook, $no_questions_asked, $only_this_server, $pull_request_id, $restart_only, $rollback, $server_id, $server_name) { + // Lock stable rows because an empty deployment queue has no row to lock. + Application::query()->whereKey($application_id)->lockForUpdate()->firstOrFail(); + $serverForQueueCheck = Server::query()->whereKey($server_id)->lockForUpdate()->firstOrFail(); + $queue_limit = $serverForQueueCheck->settings->deployment_queue_limit ?? 25; + $queued_count = ApplicationDeploymentQueue::where('server_id', $server_id) + ->where('status', ApplicationDeploymentStatus::QUEUED->value) + ->count(); - if ($queued_count >= $queue_limit) { - return [ - 'status' => 'queue_full', - 'message' => 'Deployment queue is full. Please wait for existing deployments to complete.', - ]; - } + if ($queued_count >= $queue_limit) { + return [ + 'status' => 'queue_full', + 'message' => 'Deployment queue is full. Please wait for existing deployments to complete.', + ]; + } - // Check if there's already a deployment in progress or queued for this application and commit - $existing_deployment = ApplicationDeploymentQueue::where('application_id', $application_id) - ->where('commit', $commit) - ->where('pull_request_id', $pull_request_id) - ->where('docker_registry_image_tag', $docker_registry_image_tag) - ->whereIn('status', [ApplicationDeploymentStatus::IN_PROGRESS->value, ApplicationDeploymentStatus::QUEUED->value]) - ->first(); + $existing_deployment = ApplicationDeploymentQueue::where('application_id', $application_id) + ->where('commit', $commit) + ->where('pull_request_id', $pull_request_id) + ->where('docker_registry_image_tag', $docker_registry_image_tag) + ->whereIn('status', [ApplicationDeploymentStatus::IN_PROGRESS->value, ApplicationDeploymentStatus::QUEUED->value]) + ->first(); - if ($existing_deployment) { - // If force_rebuild is true or rollback is true or no_questions_asked is true, we'll still create a new deployment - if (! $force_rebuild && ! $rollback && ! $no_questions_asked) { - // Return the existing deployment's details + if ($existing_deployment && ! $force_rebuild && ! $rollback && ! $no_questions_asked) { return [ 'status' => 'skipped', 'message' => 'Deployment already queued for this commit.', @@ -63,27 +62,33 @@ function queue_application_deployment(Application $application, string $deployme 'existing_deployment' => $existing_deployment, ]; } + + return ApplicationDeploymentQueue::create([ + 'application_id' => $application_id, + 'application_name' => $application->name, + 'server_id' => $server_id, + 'server_name' => $server_name, + 'destination_id' => $destination_id, + 'deployment_uuid' => $deployment_uuid, + 'deployment_url' => $deployment_url, + 'pull_request_id' => $pull_request_id, + 'docker_registry_image_tag' => $docker_registry_image_tag, + 'force_rebuild' => $force_rebuild, + 'is_webhook' => $is_webhook, + 'is_api' => $is_api, + 'restart_only' => $restart_only, + 'commit' => $commit, + 'rollback' => $rollback, + 'git_type' => $git_type, + 'only_this_server' => $only_this_server, + ]); + }); + + if (is_array($admission)) { + return $admission; } - $deployment = ApplicationDeploymentQueue::create([ - 'application_id' => $application_id, - 'application_name' => $application->name, - 'server_id' => $server_id, - 'server_name' => $server_name, - 'destination_id' => $destination_id, - 'deployment_uuid' => $deployment_uuid, - 'deployment_url' => $deployment_url, - 'pull_request_id' => $pull_request_id, - 'docker_registry_image_tag' => $docker_registry_image_tag, - 'force_rebuild' => $force_rebuild, - 'is_webhook' => $is_webhook, - 'is_api' => $is_api, - 'restart_only' => $restart_only, - 'commit' => $commit, - 'rollback' => $rollback, - 'git_type' => $git_type, - 'only_this_server' => $only_this_server, - ]); + $deployment = $admission; if (auth()->check() && ! $is_webhook && ! $is_api && ! $rollback) { auditLog($restart_only ? 'ui.application.restarted' : 'ui.application.deployed', [ diff --git a/tests/Feature/QueueApplicationDeploymentCommitTest.php b/tests/Feature/QueueApplicationDeploymentCommitTest.php index f5a8bb110e..992856bae5 100644 --- a/tests/Feature/QueueApplicationDeploymentCommitTest.php +++ b/tests/Feature/QueueApplicationDeploymentCommitTest.php @@ -1,5 +1,6 @@ environment->id, $this->destination->id, 'HEAD'); + $initialLevel = DB::transactionLevel(); + $creationLevel = null; + + ApplicationDeploymentQueue::created(function () use (&$creationLevel): void { + $creationLevel = DB::transactionLevel(); + }); + + queue_application_deployment($application, 'atomic-admission'); + + expect($creationLevel)->toBeGreaterThan($initialLevel); + }); + + test('skips a matching commit without creating or dispatching a second deployment', function () { + $application = makeApplication($this->environment->id, $this->destination->id, 'HEAD'); + + $first = queue_application_deployment($application, 'first-admission'); + $second = queue_application_deployment($application, 'second-admission'); + + expect($first['status'])->toBe('queued') + ->and($second['status'])->toBe('skipped') + ->and($second['deployment_uuid'])->toBe('first-admission') + ->and($second['existing_deployment']->deployment_uuid)->toBe('first-admission') + ->and(ApplicationDeploymentQueue::query()->count())->toBe(1); + Bus::assertDispatchedTimes(ApplicationDeploymentJob::class, 1); + }); + + test('allows explicit duplicate bypasses while queue capacity remains available', function (string $flag) { + $application = makeApplication($this->environment->id, $this->destination->id, 'HEAD'); + + queue_application_deployment($application, 'first-admission'); + $result = queue_application_deployment($application, 'bypass-admission', ...[$flag => true]); + + expect($result['status'])->toBe('queued') + ->and(ApplicationDeploymentQueue::query()->count())->toBe(2); + })->with(['force_rebuild', 'rollback', 'no_questions_asked']); + + test('checks duplicate commits across servers but uses the target server queue', function () { + $otherServer = Server::factory()->create(['team_id' => $this->team->id]); + $otherDestination = StandaloneDocker::factory()->create([ + 'server_id' => $otherServer->id, + 'network' => 'test-network-'.fake()->unique()->word(), + ]); + $application = makeApplication($this->environment->id, $this->destination->id, 'HEAD'); + + queue_application_deployment($application, 'first-server-admission'); + $skipped = queue_application_deployment($application, 'second-server-admission', server: $otherServer, destination: $otherDestination); + $forced = queue_application_deployment($application, 'forced-second-server-admission', force_rebuild: true, server: $otherServer, destination: $otherDestination); + + expect($skipped['status'])->toBe('skipped') + ->and($skipped['deployment_uuid'])->toBe('first-server-admission') + ->and($forced['status'])->toBe('queued') + ->and(ApplicationDeploymentQueue::query()->where('server_id', $otherServer->id)->sole()->deployment_uuid)->toBe('forced-second-server-admission'); + }); + + test('rejects a full server queue before a different commit is inserted', function () { + $this->server->settings->update(['concurrent_builds' => 0, 'deployment_queue_limit' => 1]); + $application = makeApplication($this->environment->id, $this->destination->id, 'HEAD'); + + queue_application_deployment($application, 'first-admission'); + $result = queue_application_deployment($application, 'full-admission', commit: 'another-commit'); + + expect($result)->toBe([ + 'status' => 'queue_full', + 'message' => 'Deployment queue is full. Please wait for existing deployments to complete.', + ])->and(ApplicationDeploymentQueue::query()->where('status', ApplicationDeploymentStatus::QUEUED->value)->count())->toBe(1); + Bus::assertNotDispatched(ApplicationDeploymentJob::class); + }); + + test('does not let a forced deployment exceed the queued limit', function () { + $this->server->settings->update(['concurrent_builds' => 0, 'deployment_queue_limit' => 1]); + $application = makeApplication($this->environment->id, $this->destination->id, 'HEAD'); + + queue_application_deployment($application, 'first-admission'); + $result = queue_application_deployment($application, 'forced-full-admission', force_rebuild: true); + + expect($result['status'])->toBe('queue_full') + ->and(ApplicationDeploymentQueue::query()->count())->toBe(1); + Bus::assertNotDispatched(ApplicationDeploymentJob::class); + }); + test('rejects a commit with disallowed characters before creating a deployment', function () { $application = makeApplication($this->environment->id, $this->destination->id, 'HEAD'); @@ -55,7 +139,7 @@ describe('queue_application_deployment commit resolution', function () { is_webhook: true, ))->toThrow(Exception::class, 'Invalid deployment commit'); - $this->assertDatabaseMissing('application_deployment_queue', [ + $this->assertDatabaseMissing((new ApplicationDeploymentQueue)->getTable(), [ 'deployment_uuid' => 'invalid-queued-commit', ]); Bus::assertNotDispatched(ApplicationDeploymentJob::class); @@ -73,7 +157,7 @@ describe('queue_application_deployment commit resolution', function () { deployment_uuid: 'invalid-fallback-commit', ))->toThrow(Exception::class, 'Invalid deployment commit'); - $this->assertDatabaseMissing('application_deployment_queue', [ + $this->assertDatabaseMissing((new ApplicationDeploymentQueue)->getTable(), [ 'deployment_uuid' => 'invalid-fallback-commit', ]); Bus::assertNotDispatched(ApplicationDeploymentJob::class);