From d27070b215fa96edf0fcae2650e6a2b6dd9e11a2 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 8 Dec 2025 17:10:39 +0100 Subject: [PATCH 1/4] fix: Add comprehensive PR cleanup to GitLab, Bitbucket, and Gitea webhooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create a shared CleanupPreviewDeployment action that unifies PR cleanup logic across all Git providers. Previously, GitHub had comprehensive cleanup (cancels active deployments, kills helper containers, removes all PR containers), while GitLab, Bitbucket, and Gitea only did basic cleanup (delete preview record and remove one container by name). This fix ensures all providers properly clean up orphaned PR containers when a PR is closed/merged, preventing security issues and resource waste. Also fixes early return bug in GitLab webhook handler. Fixes #2610 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../Application/CleanupPreviewDeployment.php | 175 ++++++++++++++++++ app/Http/Controllers/Webhook/Bitbucket.php | 8 +- app/Http/Controllers/Webhook/Gitea.php | 8 +- app/Http/Controllers/Webhook/Github.php | 88 +-------- app/Http/Controllers/Webhook/Gitlab.php | 23 +-- 5 files changed, 205 insertions(+), 97 deletions(-) create mode 100644 app/Actions/Application/CleanupPreviewDeployment.php diff --git a/app/Actions/Application/CleanupPreviewDeployment.php b/app/Actions/Application/CleanupPreviewDeployment.php new file mode 100644 index 000000000..83f729959 --- /dev/null +++ b/app/Actions/Application/CleanupPreviewDeployment.php @@ -0,0 +1,175 @@ + 0, + 'killed_containers' => 0, + 'status' => 'success', + ]; + + $server = $application->destination->server; + + if (! $server->isFunctional()) { + return [ + ...$result, + 'status' => 'failed', + 'message' => 'Server is not functional', + ]; + } + + // Step 1: Cancel all active deployments for this PR and kill helper containers + $result['cancelled_deployments'] = $this->cancelActiveDeployments( + $application, + $pull_request_id, + $server + ); + + // Step 2: Stop and remove all running PR containers + $result['killed_containers'] = $this->stopRunningContainers( + $application, + $pull_request_id, + $server + ); + + // Step 3: Find or use provided preview, then dispatch cleanup job for thorough cleanup + if (! $preview) { + $preview = ApplicationPreview::where('application_id', $application->id) + ->where('pull_request_id', $pull_request_id) + ->first(); + } + + if ($preview) { + DeleteResourceJob::dispatch($preview); + } + + return $result; + } + + /** + * Cancel all active (QUEUED/IN_PROGRESS) deployments for this PR. + */ + private function cancelActiveDeployments( + Application $application, + int $pull_request_id, + $server + ): int { + $activeDeployments = ApplicationDeploymentQueue::where('application_id', $application->id) + ->where('pull_request_id', $pull_request_id) + ->whereIn('status', [ + ApplicationDeploymentStatus::QUEUED->value, + ApplicationDeploymentStatus::IN_PROGRESS->value, + ]) + ->get(); + + $cancelled = 0; + foreach ($activeDeployments as $deployment) { + try { + // Mark deployment as cancelled + $deployment->update([ + 'status' => ApplicationDeploymentStatus::CANCELLED_BY_USER->value, + ]); + + // Add cancellation log entry + $deployment->addLogEntry('Deployment cancelled: Pull request closed.', 'stderr'); + + // Try to kill helper container if it exists + $this->killHelperContainer($deployment->deployment_uuid, $server); + $cancelled++; + } catch (\Throwable $e) { + \Log::warning("Failed to cancel deployment {$deployment->id}: {$e->getMessage()}"); + } + } + + return $cancelled; + } + + /** + * Kill the helper container used during deployment. + */ + private function killHelperContainer(string $deployment_uuid, $server): void + { + try { + $escapedUuid = escapeshellarg($deployment_uuid); + $checkCommand = "docker ps -a --filter name={$escapedUuid} --format '{{.Names}}'"; + $containerExists = instant_remote_process([$checkCommand], $server); + + if ($containerExists && str($containerExists)->trim()->isNotEmpty()) { + instant_remote_process(["docker rm -f {$escapedUuid}"], $server); + } + } catch (\Throwable $e) { + // Silently handle - container may already be gone + } + } + + /** + * Stop and remove all running containers for this PR. + */ + private function stopRunningContainers( + Application $application, + int $pull_request_id, + $server + ): int { + $killed = 0; + + try { + if ($server->isSwarm()) { + $escapedStackName = escapeshellarg("{$application->uuid}-{$pull_request_id}"); + instant_remote_process(["docker stack rm {$escapedStackName}"], $server); + $killed++; + } else { + $containers = getCurrentApplicationContainerStatus( + $server, + $application->id, + $pull_request_id + ); + + if ($containers->isNotEmpty()) { + foreach ($containers as $container) { + $containerName = data_get($container, 'Names'); + if ($containerName) { + instant_remote_process( + ["docker rm -f $containerName"], + $server + ); + $killed++; + } + } + } + } + } catch (\Throwable $e) { + \Log::warning("Error stopping containers for PR #{$pull_request_id}: {$e->getMessage()}"); + } + + return $killed; + } +} diff --git a/app/Http/Controllers/Webhook/Bitbucket.php b/app/Http/Controllers/Webhook/Bitbucket.php index 2f228119d..d322452d3 100644 --- a/app/Http/Controllers/Webhook/Bitbucket.php +++ b/app/Http/Controllers/Webhook/Bitbucket.php @@ -2,6 +2,7 @@ namespace App\Http\Controllers\Webhook; +use App\Actions\Application\CleanupPreviewDeployment; use App\Http\Controllers\Controller; use App\Models\Application; use App\Models\ApplicationPreview; @@ -167,9 +168,10 @@ public function manual(Request $request) if ($x_bitbucket_event === 'pullrequest:rejected' || $x_bitbucket_event === 'pullrequest:fulfilled') { $found = ApplicationPreview::where('application_id', $application->id)->where('pull_request_id', $pull_request_id)->first(); if ($found) { - $found->delete(); - $container_name = generateApplicationContainerName($application, $pull_request_id); - instant_remote_process(["docker rm -f $container_name"], $application->destination->server); + // Use comprehensive cleanup that cancels active deployments, + // kills helper containers, and removes all PR containers + CleanupPreviewDeployment::run($application, $pull_request_id, $found); + $return_payloads->push([ 'application' => $application->name, 'status' => 'success', diff --git a/app/Http/Controllers/Webhook/Gitea.php b/app/Http/Controllers/Webhook/Gitea.php index e41825aba..f85d14089 100644 --- a/app/Http/Controllers/Webhook/Gitea.php +++ b/app/Http/Controllers/Webhook/Gitea.php @@ -2,6 +2,7 @@ namespace App\Http\Controllers\Webhook; +use App\Actions\Application\CleanupPreviewDeployment; use App\Http\Controllers\Controller; use App\Models\Application; use App\Models\ApplicationPreview; @@ -192,9 +193,10 @@ public function manual(Request $request) if ($action === 'closed') { $found = ApplicationPreview::where('application_id', $application->id)->where('pull_request_id', $pull_request_id)->first(); if ($found) { - $found->delete(); - $container_name = generateApplicationContainerName($application, $pull_request_id); - instant_remote_process(["docker rm -f $container_name"], $application->destination->server); + // Use comprehensive cleanup that cancels active deployments, + // kills helper containers, and removes all PR containers + CleanupPreviewDeployment::run($application, $pull_request_id, $found); + $return_payloads->push([ 'application' => $application->name, 'status' => 'success', diff --git a/app/Http/Controllers/Webhook/Github.php b/app/Http/Controllers/Webhook/Github.php index 2402b71ae..93f225773 100644 --- a/app/Http/Controllers/Webhook/Github.php +++ b/app/Http/Controllers/Webhook/Github.php @@ -2,10 +2,10 @@ namespace App\Http\Controllers\Webhook; +use App\Actions\Application\CleanupPreviewDeployment; use App\Enums\ProcessStatus; use App\Http\Controllers\Controller; use App\Jobs\ApplicationPullRequestUpdateJob; -use App\Jobs\DeleteResourceJob; use App\Jobs\GithubAppPermissionJob; use App\Models\Application; use App\Models\ApplicationPreview; @@ -221,41 +221,10 @@ public function manual(Request $request) if ($action === 'closed') { $found = ApplicationPreview::where('application_id', $application->id)->where('pull_request_id', $pull_request_id)->first(); if ($found) { - // Cancel any active deployments for this PR immediately - $activeDeployment = \App\Models\ApplicationDeploymentQueue::where('application_id', $application->id) - ->where('pull_request_id', $pull_request_id) - ->whereIn('status', [ - \App\Enums\ApplicationDeploymentStatus::QUEUED->value, - \App\Enums\ApplicationDeploymentStatus::IN_PROGRESS->value, - ]) - ->first(); + // Use comprehensive cleanup that cancels active deployments, + // kills helper containers, and removes all PR containers + CleanupPreviewDeployment::run($application, $pull_request_id, $found); - if ($activeDeployment) { - try { - // Mark deployment as cancelled - $activeDeployment->update([ - 'status' => \App\Enums\ApplicationDeploymentStatus::CANCELLED_BY_USER->value, - ]); - - // Add cancellation log entry - $activeDeployment->addLogEntry('Deployment cancelled: Pull request closed.', 'stderr'); - - // Check if helper container exists and kill it - $deployment_uuid = $activeDeployment->deployment_uuid; - $server = $application->destination->server; - $checkCommand = "docker ps -a --filter name={$deployment_uuid} --format '{{.Names}}'"; - $containerExists = instant_remote_process([$checkCommand], $server); - - if ($containerExists && str($containerExists)->trim()->isNotEmpty()) { - instant_remote_process(["docker rm -f {$deployment_uuid}"], $server); - $activeDeployment->addLogEntry('Deployment container stopped.'); - } - } catch (\Throwable $e) { - // Silently handle errors during deployment cancellation - } - } - - DeleteResourceJob::dispatch($found); $return_payloads->push([ 'application' => $application->name, 'status' => 'success', @@ -466,53 +435,12 @@ public function normal(Request $request) if ($action === 'closed' || $action === 'close') { $found = ApplicationPreview::where('application_id', $application->id)->where('pull_request_id', $pull_request_id)->first(); if ($found) { - // Cancel any active deployments for this PR immediately - $activeDeployment = \App\Models\ApplicationDeploymentQueue::where('application_id', $application->id) - ->where('pull_request_id', $pull_request_id) - ->whereIn('status', [ - \App\Enums\ApplicationDeploymentStatus::QUEUED->value, - \App\Enums\ApplicationDeploymentStatus::IN_PROGRESS->value, - ]) - ->first(); - - if ($activeDeployment) { - try { - // Mark deployment as cancelled - $activeDeployment->update([ - 'status' => \App\Enums\ApplicationDeploymentStatus::CANCELLED_BY_USER->value, - ]); - - // Add cancellation log entry - $activeDeployment->addLogEntry('Deployment cancelled: Pull request closed.', 'stderr'); - - // Check if helper container exists and kill it - $deployment_uuid = $activeDeployment->deployment_uuid; - $server = $application->destination->server; - $checkCommand = "docker ps -a --filter name={$deployment_uuid} --format '{{.Names}}'"; - $containerExists = instant_remote_process([$checkCommand], $server); - - if ($containerExists && str($containerExists)->trim()->isNotEmpty()) { - instant_remote_process(["docker rm -f {$deployment_uuid}"], $server); - $activeDeployment->addLogEntry('Deployment container stopped.'); - } - - } catch (\Throwable $e) { - // Silently handle errors during deployment cancellation - } - } - - // Clean up any deployed containers - $containers = getCurrentApplicationContainerStatus($application->destination->server, $application->id, $pull_request_id); - if ($containers->isNotEmpty()) { - $containers->each(function ($container) use ($application) { - $container_name = data_get($container, 'Names'); - instant_remote_process(["docker rm -f $container_name"], $application->destination->server); - }); - } - + // Delete the PR comment on GitHub (GitHub-specific feature) ApplicationPullRequestUpdateJob::dispatchSync(application: $application, preview: $found, status: ProcessStatus::CLOSED); - DeleteResourceJob::dispatch($found); + // Use comprehensive cleanup that cancels active deployments, + // kills helper containers, and removes all PR containers + CleanupPreviewDeployment::run($application, $pull_request_id, $found); $return_payloads->push([ 'application' => $application->name, diff --git a/app/Http/Controllers/Webhook/Gitlab.php b/app/Http/Controllers/Webhook/Gitlab.php index 56a9c0d1b..9062d2875 100644 --- a/app/Http/Controllers/Webhook/Gitlab.php +++ b/app/Http/Controllers/Webhook/Gitlab.php @@ -2,6 +2,7 @@ namespace App\Http\Controllers\Webhook; +use App\Actions\Application\CleanupPreviewDeployment; use App\Http\Controllers\Controller; use App\Models\Application; use App\Models\ApplicationPreview; @@ -224,22 +225,22 @@ public function manual(Request $request) } elseif ($action === 'closed' || $action === 'close' || $action === 'merge') { $found = ApplicationPreview::where('application_id', $application->id)->where('pull_request_id', $pull_request_id)->first(); if ($found) { - $found->delete(); - $container_name = generateApplicationContainerName($application, $pull_request_id); - instant_remote_process(["docker rm -f $container_name"], $application->destination->server); + // Use comprehensive cleanup that cancels active deployments, + // kills helper containers, and removes all PR containers + CleanupPreviewDeployment::run($application, $pull_request_id, $found); + $return_payloads->push([ 'application' => $application->name, 'status' => 'success', - 'message' => 'Preview Deployment closed', + 'message' => 'Preview deployment closed.', + ]); + } else { + $return_payloads->push([ + 'application' => $application->name, + 'status' => 'failed', + 'message' => 'No preview deployment found.', ]); - - return response($return_payloads); } - $return_payloads->push([ - 'application' => $application->name, - 'status' => 'failed', - 'message' => 'No Preview Deployment found', - ]); } else { $return_payloads->push([ 'application' => $application->name, From 945cce95870b2f18b13f8f509677ad3823d2b97f Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 8 Dec 2025 17:15:52 +0100 Subject: [PATCH 2/4] feat: Add scheduled job to cleanup orphaned PR containers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add CleanupOrphanedPreviewContainersJob that runs daily to find and remove any PR preview containers that weren't properly cleaned up when their PR was closed. The job: - Scans all functional servers for containers with coolify.pullRequestId label - Checks if the corresponding ApplicationPreview record exists in the database - Removes containers where the preview record no longer exists (truly orphaned) - Acts as a safety net for webhook failures or race conditions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- app/Console/Kernel.php | 5 +- .../CleanupOrphanedPreviewContainersJob.php | 202 ++++++++++++++++++ 2 files changed, 206 insertions(+), 1 deletion(-) create mode 100644 app/Jobs/CleanupOrphanedPreviewContainersJob.php diff --git a/app/Console/Kernel.php b/app/Console/Kernel.php index 9fb5e8a19..8687104e0 100644 --- a/app/Console/Kernel.php +++ b/app/Console/Kernel.php @@ -6,6 +6,7 @@ use App\Jobs\CheckHelperImageJob; use App\Jobs\CheckTraefikVersionJob; use App\Jobs\CleanupInstanceStuffsJob; +use App\Jobs\CleanupOrphanedPreviewContainersJob; use App\Jobs\PullChangelog; use App\Jobs\PullTemplatesFromCDN; use App\Jobs\RegenerateSslCertJob; @@ -17,7 +18,6 @@ use App\Models\Team; use Illuminate\Console\Scheduling\Schedule; use Illuminate\Foundation\Console\Kernel as ConsoleKernel; -use Illuminate\Support\Facades\Log; class Kernel extends ConsoleKernel { @@ -87,6 +87,9 @@ protected function schedule(Schedule $schedule): void $this->scheduleInstance->command('cleanup:database --yes')->daily(); $this->scheduleInstance->command('uploads:clear')->everyTwoMinutes(); + + // Cleanup orphaned PR preview containers daily + $this->scheduleInstance->job(new CleanupOrphanedPreviewContainersJob)->daily()->onOneServer(); } } diff --git a/app/Jobs/CleanupOrphanedPreviewContainersJob.php b/app/Jobs/CleanupOrphanedPreviewContainersJob.php new file mode 100644 index 000000000..790ad1489 --- /dev/null +++ b/app/Jobs/CleanupOrphanedPreviewContainersJob.php @@ -0,0 +1,202 @@ +expireAfter(600)->dontRelease()]; + } + + public function handle(): void + { + try { + $servers = $this->getServersToCheck(); + + foreach ($servers as $server) { + $this->cleanupOrphanedContainersOnServer($server); + } + } catch (\Throwable $e) { + Log::error('CleanupOrphanedPreviewContainersJob failed: '.$e->getMessage()); + send_internal_notification('CleanupOrphanedPreviewContainersJob failed with error: '.$e->getMessage()); + } + } + + /** + * Get all functional servers to check for orphaned containers. + */ + private function getServersToCheck(): \Illuminate\Support\Collection + { + $query = Server::whereRelation('settings', 'is_usable', true) + ->whereRelation('settings', 'is_reachable', true) + ->where('ip', '!=', '1.2.3.4'); + + if (isCloud()) { + $query = $query->whereRelation('team.subscription', 'stripe_invoice_paid', true); + } + + return $query->get()->filter(fn ($server) => $server->isFunctional()); + } + + /** + * Find and clean up orphaned PR containers on a specific server. + */ + private function cleanupOrphanedContainersOnServer(Server $server): void + { + try { + $prContainers = $this->getPRContainersOnServer($server); + + if ($prContainers->isEmpty()) { + return; + } + + $orphanedCount = 0; + foreach ($prContainers as $container) { + if ($this->isOrphanedContainer($container)) { + $this->removeContainer($container, $server); + $orphanedCount++; + } + } + + if ($orphanedCount > 0) { + Log::info("CleanupOrphanedPreviewContainersJob - Removed {$orphanedCount} orphaned PR containers", [ + 'server' => $server->name, + ]); + } + } catch (\Throwable $e) { + Log::warning("CleanupOrphanedPreviewContainersJob - Error on server {$server->name}: {$e->getMessage()}"); + } + } + + /** + * Get all PR containers on a server (containers with pullRequestId > 0). + */ + private function getPRContainersOnServer(Server $server): \Illuminate\Support\Collection + { + try { + $output = instant_remote_process([ + "docker ps -a --filter 'label=coolify.pullRequestId' --format '{{json .}}'", + ], $server, false); + + if (empty($output)) { + return collect(); + } + + return format_docker_command_output_to_json($output) + ->filter(function ($container) { + // Only include PR containers (pullRequestId > 0) + $prId = $this->extractPullRequestId($container); + + return $prId !== null && $prId > 0; + }); + } catch (\Throwable $e) { + Log::debug("Failed to get PR containers on server {$server->name}: {$e->getMessage()}"); + + return collect(); + } + } + + /** + * Extract pull request ID from container labels. + */ + private function extractPullRequestId($container): ?int + { + $labels = data_get($container, 'Labels', ''); + if (preg_match('/coolify\.pullRequestId=(\d+)/', $labels, $matches)) { + return (int) $matches[1]; + } + + return null; + } + + /** + * Extract application ID from container labels. + */ + private function extractApplicationId($container): ?int + { + $labels = data_get($container, 'Labels', ''); + if (preg_match('/coolify\.applicationId=(\d+)/', $labels, $matches)) { + return (int) $matches[1]; + } + + return null; + } + + /** + * Check if a container is orphaned (no corresponding ApplicationPreview record). + */ + private function isOrphanedContainer($container): bool + { + $applicationId = $this->extractApplicationId($container); + $pullRequestId = $this->extractPullRequestId($container); + + if ($applicationId === null || $pullRequestId === null) { + return false; + } + + // Check if ApplicationPreview record exists (including soft-deleted) + $previewExists = ApplicationPreview::withTrashed() + ->where('application_id', $applicationId) + ->where('pull_request_id', $pullRequestId) + ->exists(); + + // If preview exists (even soft-deleted), container should be handled by DeleteResourceJob + // If preview doesn't exist at all, it's truly orphaned + return ! $previewExists; + } + + /** + * Remove an orphaned container from the server. + */ + private function removeContainer($container, Server $server): void + { + $containerName = data_get($container, 'Names'); + $applicationId = $this->extractApplicationId($container); + $pullRequestId = $this->extractPullRequestId($container); + + Log::info('CleanupOrphanedPreviewContainersJob - Removing orphaned container', [ + 'container' => $containerName, + 'application_id' => $applicationId, + 'pull_request_id' => $pullRequestId, + 'server' => $server->name, + ]); + + try { + instant_remote_process( + ["docker rm -f {$containerName}"], + $server, + false + ); + } catch (\Throwable $e) { + Log::warning("Failed to remove orphaned container {$containerName}: {$e->getMessage()}"); + } + } +} From 86a02a12e664c7e1c737978240fa71fda495362b Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 8 Dec 2025 17:35:13 +0100 Subject: [PATCH 3/4] Update app/Actions/Application/CleanupPreviewDeployment.php Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- app/Actions/Application/CleanupPreviewDeployment.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/Actions/Application/CleanupPreviewDeployment.php b/app/Actions/Application/CleanupPreviewDeployment.php index 83f729959..74e2ff615 100644 --- a/app/Actions/Application/CleanupPreviewDeployment.php +++ b/app/Actions/Application/CleanupPreviewDeployment.php @@ -157,8 +157,9 @@ private function stopRunningContainers( foreach ($containers as $container) { $containerName = data_get($container, 'Names'); if ($containerName) { + $escapedContainerName = escapeshellarg($containerName); instant_remote_process( - ["docker rm -f $containerName"], + ["docker rm -f {$escapedContainerName}"], $server ); $killed++; From ebac90097a60a855b6e358233445e2b7a9b1a3ba Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 8 Dec 2025 20:09:00 +0100 Subject: [PATCH 4/4] fix: Escape container name in orphaned PR cleanup job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add shell escaping with escapeshellarg() for container names in the docker rm command to prevent command injection. Also add validation to skip containers with missing names and log a warning. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- app/Jobs/CleanupOrphanedPreviewContainersJob.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/app/Jobs/CleanupOrphanedPreviewContainersJob.php b/app/Jobs/CleanupOrphanedPreviewContainersJob.php index 790ad1489..5d3bed457 100644 --- a/app/Jobs/CleanupOrphanedPreviewContainersJob.php +++ b/app/Jobs/CleanupOrphanedPreviewContainersJob.php @@ -179,6 +179,16 @@ private function isOrphanedContainer($container): bool private function removeContainer($container, Server $server): void { $containerName = data_get($container, 'Names'); + + if (empty($containerName)) { + Log::warning('CleanupOrphanedPreviewContainersJob - Cannot remove container: missing container name', [ + 'container_data' => $container, + 'server' => $server->name, + ]); + + return; + } + $applicationId = $this->extractApplicationId($container); $pullRequestId = $this->extractPullRequestId($container); @@ -189,9 +199,11 @@ private function removeContainer($container, Server $server): void 'server' => $server->name, ]); + $escapedContainerName = escapeshellarg($containerName); + try { instant_remote_process( - ["docker rm -f {$containerName}"], + ["docker rm -f {$escapedContainerName}"], $server, false );