From 2ac5969d90e9574cf0f36284222d1a125b4045ad Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 29 Sep 2026 23:05:34 +0200 Subject: [PATCH] fix(docker): stop matching containers by numeric resource id labels Numeric coolify.{type}Id labels can collide with resources on another Coolify instance managing the same server, so they no longer select or resolve container owners. Drop the legacyId fallback from container lookups in DeleteService and ComplexStatusCheck. Legacy application containers without a UUID label are now resolved by their compose service name, which starts with the application UUID. --- app/Actions/Service/DeleteService.php | 2 +- app/Actions/Shared/ComplexStatusCheck.php | 2 +- bootstrap/helpers/docker.php | 40 +++++++--------- .../Feature/ContainerOwnerUuidLabelsTest.php | 47 +++++++++++-------- .../DeleteResourceJobAtomicityTest.php | 4 +- 5 files changed, 49 insertions(+), 46 deletions(-) diff --git a/app/Actions/Service/DeleteService.php b/app/Actions/Service/DeleteService.php index 7c544692b5..1ecc029c43 100644 --- a/app/Actions/Service/DeleteService.php +++ b/app/Actions/Service/DeleteService.php @@ -67,7 +67,7 @@ class DeleteService // One sh -c line, so non-root servers run the whole script with sudo. A leading variable // assignment would become "sudo container_ids=...", which sudo rejects. - $script = containerIdsByOwnerScript('service', $service->uuid, $filters, legacyExtraFilters: $legacyFilters, legacyId: (int) $service->id) + $script = containerIdsByOwnerScript('service', $service->uuid, $filters, legacyExtraFilters: $legacyFilters) .'; [ -z "$container_ids" ] || docker rm -f $container_ids'; instant_remote_process(['sh -c '.escapeshellarg($script)], $service->server); } diff --git a/app/Actions/Shared/ComplexStatusCheck.php b/app/Actions/Shared/ComplexStatusCheck.php index 3c400e634b..db77745a26 100644 --- a/app/Actions/Shared/ComplexStatusCheck.php +++ b/app/Actions/Shared/ComplexStatusCheck.php @@ -29,7 +29,7 @@ class ComplexStatusCheck continue; } } - $script = containerIdsByOwnerScript('application', $application->uuid, ['label=coolify.pullRequestId=0'], all: false, legacyId: (int) $application->id) + $script = containerIdsByOwnerScript('application', $application->uuid, ['label=coolify.pullRequestId=0'], all: false) .'; [ -z "$container_ids" ] || docker container inspect $container_ids --format \'{{json .}}\''; $containers = instant_remote_process(['sh -c '.escapeshellarg($script)], $server, false); $containers = format_docker_command_output_to_json($containers); diff --git a/bootstrap/helpers/docker.php b/bootstrap/helpers/docker.php index 52b401e23a..e2f5947dbe 100644 --- a/bootstrap/helpers/docker.php +++ b/bootstrap/helpers/docker.php @@ -65,8 +65,7 @@ function caddyTrafficAppKey(string $uuid, ?string $serviceName = null): string * New containers carry `coolify.{type}Uuid`. Containers created before that label carry a * numeric `coolify.{type}Id` instead. Ids change when a server moves to another instance, so * for those containers the owner is read from the compose project (Swarm: stack namespace), - * which is the resource UUID for deployments since mid-2024. Use resolveContainerOwner() to - * also fall back to the numeric id for older containers. + * which is the resource UUID for deployments since mid-2024. * * @param 'application'|'service'|'database' $type */ @@ -89,11 +88,11 @@ function containerOwnerUuid(Collection|array|string $labels, string $type): ?str } /** - * Resource that owns a container, from the given candidates. - * - * Order: UUID label, compose project / stack namespace (see containerOwnerUuid()), then the - * numeric id label of containers without a UUID label. The id is only correct on the instance - * that created the container, so it is the last fallback. + * Resource that owns a container, from the given candidates, by UUID label or compose project / + * stack namespace (see containerOwnerUuid()). Applications deployed before mid-2024 used the + * deployment directory as compose project; their compose service (the container name) starts + * with the application UUID. The numeric id label is never used: another Coolify instance that + * manages the same server can have a resource with the same id. * * @template TResource of \Illuminate\Database\Eloquent\Model * @@ -107,13 +106,13 @@ function resolveContainerOwner(Collection $resources, Collection|array|string $l $uuid = containerOwnerUuid($labels, $type); $owner = $uuid ? $resources->firstWhere('uuid', $uuid) : null; - if ($owner || filled($labels->get("coolify.{$type}Uuid"))) { + if ($owner || $type !== 'application' || filled($labels->get('coolify.applicationUuid')) || ! $labels->has('coolify.applicationId')) { return $owner; } - $legacyId = $labels->get("coolify.{$type}Id"); + $composeService = (string) $labels->get('com.docker.compose.service'); - return is_numeric($legacyId) ? $resources->first(fn ($resource) => (int) $resource->id === (int) $legacyId) : null; + return $resources->first(fn ($resource) => $composeService === $resource->uuid || str_starts_with($composeService, $resource->uuid.'-')); } /** @@ -131,13 +130,8 @@ function resolveContainerApplicationId(Collection $applications, Collection|arra $uuid = containerOwnerUuid($labels, 'application'); $id = $uuid ? Application::withTrashed()->where('uuid', $uuid)->value('id') : null; - if ($id !== null || filled($labels->get('coolify.applicationUuid'))) { - return $id !== null ? (int) $id : null; - } - $legacyId = $labels->get('coolify.applicationId'); - - return is_numeric($legacyId) ? (int) $legacyId : null; + return $id !== null ? (int) $id : null; } /** @@ -189,10 +183,9 @@ function resolveServiceContainerOwner(Collection $services, Collection|array $fl * @param 'application'|'service'|'database' $type * @param list $extraFilters Additional `--filter` values, e.g. `label=coolify.pullRequestId=0`. * @param list|null $legacyExtraFilters Filters for containers without the UUID label; defaults to $extraFilters. - * @param int|null $legacyId Local numeric id: also match older containers by `coolify.{type}Id` (may repeat a container). * @return list */ -function dockerPsByOwnerCommands(string $type, string $uuid, array $extraFilters = [], string $format = '{{json .}}', bool $all = true, ?array $legacyExtraFilters = null, ?int $legacyId = null): array +function dockerPsByOwnerCommands(string $type, string $uuid, array $extraFilters = [], string $format = '{{json .}}', bool $all = true, ?array $legacyExtraFilters = null): array { $filters = fn (array $values) => collect($values)->map(fn (string $filter) => '--filter '.escapeshellarg($filter))->implode(' '); $base = 'docker ps'.($all ? ' -a' : ''); @@ -203,8 +196,9 @@ function dockerPsByOwnerCommands(string $type, string $uuid, array $extraFilters $base.' '.$filters(["label=coolify.{$type}Uuid={$uuid}", ...$extraFilters]).' '.$formatArgument, $base.' '.$filters(["label=coolify.{$type}Id", "label=com.docker.compose.project={$uuid}", ...$legacyFilters]).' '.$formatArgument, ]; - if ($legacyId !== null) { - $commands[] = $base.' '.$filters(["label=coolify.{$type}Id={$legacyId}", ...$legacyFilters]).' '.$formatArgument; + if ($type === 'application') { + // Deployed before mid-2024 (see resolveContainerOwner()): the container name starts with the UUID. + $commands[] = $base.' '.$filters(['label=coolify.applicationId', "name=^{$uuid}", ...$legacyFilters]).' '.$formatArgument; } return $commands; @@ -217,7 +211,7 @@ function dockerPsByOwnerCommands(string $type, string $uuid, array $extraFilters */ function containersOwnedBy(Server $server, string $type, Model $resource): Collection { - $output = instant_remote_process(dockerPsByOwnerCommands($type, $resource->uuid, legacyId: (int) $resource->id), $server); + $output = instant_remote_process(dockerPsByOwnerCommands($type, $resource->uuid), $server); return format_docker_command_output_to_json($output)->filter()->unique('ID')->values(); } @@ -229,9 +223,9 @@ function containersOwnedBy(Server $server, string $type, Model $resource): Colle * @param list $extraFilters * @param list|null $legacyExtraFilters */ -function containerIdsByOwnerScript(string $type, string $uuid, array $extraFilters = [], bool $all = true, ?array $legacyExtraFilters = null, ?int $legacyId = null): string +function containerIdsByOwnerScript(string $type, string $uuid, array $extraFilters = [], bool $all = true, ?array $legacyExtraFilters = null): string { - $commands = dockerPsByOwnerCommands($type, $uuid, $extraFilters, '{{.ID}}', $all, $legacyExtraFilters, $legacyId); + $commands = dockerPsByOwnerCommands($type, $uuid, $extraFilters, '{{.ID}}', $all, $legacyExtraFilters); return 'container_ids=$({ '.implode('; ', $commands).'; } | sort -u)'; } diff --git a/tests/Feature/ContainerOwnerUuidLabelsTest.php b/tests/Feature/ContainerOwnerUuidLabelsTest.php index d8d93a175b..18155c2a63 100644 --- a/tests/Feature/ContainerOwnerUuidLabelsTest.php +++ b/tests/Feature/ContainerOwnerUuidLabelsTest.php @@ -67,7 +67,7 @@ describe('label producers', function () { }); describe('container lookups for services and databases', function () { - test('database and service lookups match the UUID label, the compose project, and the older id label', function () { + test('database and service lookups match the UUID label and the compose project, never a numeric id', function () { $privateKey = PrivateKey::factory()->create(['team_id' => $this->team->id]); $this->server->update(['private_key_id' => $privateKey->id]); $this->server->settings()->update(['is_reachable' => true, 'is_usable' => true]); @@ -97,13 +97,13 @@ describe('container lookups for services and databases', function () { expect($commands->implode("\n")) ->toContain("label=coolify.databaseUuid={$database->uuid}") ->toContain("label=com.docker.compose.project={$database->uuid}") - ->toContain("label=coolify.databaseId={$database->id}") + ->not->toContain("label=coolify.databaseId={$database->id}") ->toContain("label=coolify.serviceUuid={$service->uuid}") ->toContain("label=com.docker.compose.project={$service->uuid}") - ->toContain("label=coolify.serviceId={$service->id}"); + ->not->toContain("label=coolify.serviceId={$service->id}"); }); - test('service part containers are matched by UUID label, compose service name, then older id', function () { + test('service part containers are matched by UUID label, then compose service name', function () { $service = Service::factory()->create([ 'environment_id' => $this->environment->id, 'server_id' => $this->server->id, @@ -117,8 +117,8 @@ describe('container lookups for services and databases', function () { expect($partOf(['coolify.serviceUuid' => $service->uuid, 'coolify.service.subType' => 'application', 'coolify.service.subUuid' => $part->uuid]))->toBe($part->id) // Moved from another instance: foreign ids, but compose project and service name still match. ->and($partOf(['coolify.serviceId' => '999', 'coolify.service.subType' => 'application', 'coolify.service.subId' => '999', 'com.docker.compose.project' => $service->uuid, 'com.docker.compose.service' => 'web']))->toBe($part->id) - // Older container with another compose project: local ids. - ->and($partOf(['coolify.serviceId' => (string) $service->id, 'coolify.service.subType' => 'application', 'coolify.service.subId' => (string) $part->id, 'com.docker.compose.project' => 'other']))->toBe($part->id); + // Another instance's container with the same numeric ids: not ours. + ->and($partOf(['coolify.serviceId' => (string) $service->id, 'coolify.service.subType' => 'application', 'coolify.service.subId' => (string) $part->id, 'com.docker.compose.project' => 'other']))->toBeNull(); }); }); @@ -138,26 +138,35 @@ describe('owner resolution', function () { ->toBe('app-uuid'); }); - test('containers from before mid-2024 fall back to their local numeric id', function () { + test('a local numeric id never makes another instance\'s container ours', function () { $applications = collect([$this->application]); - $olderContainer = ['coolify.applicationId' => (string) $this->application->id, 'com.docker.compose.project' => 'deployment-uuid']; + $localId = (string) $this->application->id; - expect(resolveContainerOwner($applications, $olderContainer, 'application')?->id)->toBe($this->application->id) - // A UUID label is never overridden by the numeric id. - ->and(resolveContainerOwner($applications, ['coolify.applicationUuid' => 'unknown', 'coolify.applicationId' => (string) $this->application->id], 'application'))->toBeNull() - ->and(dockerPsByOwnerCommands('application', 'app-uuid', legacyId: 42))->toHaveCount(3) - ->and(dockerPsByOwnerCommands('application', 'app-uuid', legacyId: 42)[2])->toContain("--filter 'label=coolify.applicationId=42'"); + expect(resolveContainerOwner($applications, ['coolify.applicationId' => $localId, 'com.docker.compose.project' => 'other-instance-uuid'], 'application'))->toBeNull() + ->and(resolveContainerOwner($applications, ['coolify.applicationId' => $localId], 'application'))->toBeNull() + ->and(resolveContainerOwner($applications, ['coolify.applicationUuid' => 'unknown', 'coolify.applicationId' => $localId], 'application'))->toBeNull() + ->and(resolveContainerApplicationId($applications, ['coolify.applicationId' => $localId, 'com.docker.compose.project' => 'other-instance-uuid']))->toBeNull() + ->and(resolveContainerOwner($applications, ['coolify.applicationId' => '999999', 'com.docker.compose.project' => $this->application->uuid], 'application')?->id)->toBe($this->application->id) + // Deployed before mid-2024: compose project was the deployment directory, the compose service starts with the UUID. + ->and(resolveContainerOwner($applications, ['coolify.applicationId' => '999999', 'com.docker.compose.project' => 'deployment-dir', 'com.docker.compose.service' => $this->application->uuid.'-104512123456'], 'application')?->id)->toBe($this->application->id) + ->and(resolveContainerOwner($applications, ['coolify.applicationId' => $localId, 'com.docker.compose.project' => 'deployment-dir', 'com.docker.compose.service' => 'other-instance-uuid-104512123456'], 'application'))->toBeNull() + ->and(implode("\n", dockerPsByOwnerCommands('application', 'app-uuid')))->not->toContain('label=coolify.applicationId='); }); test('docker ps commands match the UUID label and the compose project of older containers', function () { $commands = dockerPsByOwnerCommands('application', 'app-uuid', ['label=coolify.pullRequestId=0']); - expect($commands)->toHaveCount(2) + expect($commands)->toHaveCount(3) ->and($commands[0])->toContain("--filter 'label=coolify.applicationUuid=app-uuid'") ->toContain("--filter 'label=coolify.pullRequestId=0'") ->and($commands[1])->toContain("--filter 'label=coolify.applicationId'") ->toContain("--filter 'label=com.docker.compose.project=app-uuid'") - ->toContain("--filter 'label=coolify.pullRequestId=0'"); + ->toContain("--filter 'label=coolify.pullRequestId=0'") + // Deployed before mid-2024: the container name starts with the application UUID. + ->and($commands[2])->toContain("--filter 'label=coolify.applicationId'") + ->toContain("--filter 'name=^app-uuid'") + ->toContain("--filter 'label=coolify.pullRequestId=0'") + ->and(dockerPsByOwnerCommands('service', 'service-uuid'))->toHaveCount(2); }); }); @@ -207,7 +216,7 @@ describe('status updates after a server transfer', function () { ->and($this->application->fresh()->status)->toStartWith('running'); }); - test('Sentinel push and status check still match containers from before mid-2024 by their local id', function () { + test('Sentinel push and status check still match containers from before mid-2024 by the UUID in their name', function () { $labels = [ 'coolify.managed' => 'true', 'coolify.applicationId' => (string) $this->application->id, @@ -269,8 +278,8 @@ describe('orphaned preview cleanup', function () { expect($isOrphaned(['Labels' => "coolify.applicationId=999999,coolify.pullRequestId=7,com.docker.compose.project={$this->application->uuid}"]))->toBeFalse() ->and($isOrphaned(['Labels' => "coolify.applicationUuid={$this->application->uuid},coolify.pullRequestId=7"]))->toBeFalse() ->and($isOrphaned(['Labels' => "coolify.applicationUuid={$this->application->uuid},coolify.pullRequestId=8"]))->toBeTrue() - // Containers from before mid-2024: another compose project, so the local id is used, as before. - ->and($isOrphaned(['Labels' => "coolify.applicationId={$this->application->id},coolify.pullRequestId=7,com.docker.compose.project=deployment-uuid"]))->toBeFalse() - ->and($isOrphaned(['Labels' => 'coolify.applicationId=999999,coolify.pullRequestId=7']))->toBeTrue(); + // Another instance's preview container with the same numeric id: not ours, so never removed. + ->and($isOrphaned(['Labels' => "coolify.applicationId={$this->application->id},coolify.pullRequestId=8,com.docker.compose.project=other-instance-uuid"]))->toBeFalse() + ->and($isOrphaned(['Labels' => 'coolify.applicationId=999999,coolify.pullRequestId=7']))->toBeFalse(); }); }); diff --git a/tests/Feature/DeleteResourceJobAtomicityTest.php b/tests/Feature/DeleteResourceJobAtomicityTest.php index 7696a2ffbd..5ceb748e16 100644 --- a/tests/Feature/DeleteResourceJobAtomicityTest.php +++ b/tests/Feature/DeleteResourceJobAtomicityTest.php @@ -147,8 +147,8 @@ it('removes service containers before its volumes and local metadata', function expect($commandList) ->toContain("label=coolify.serviceUuid={$service->uuid}") ->toContain("label=com.docker.compose.project={$service->uuid}") - // Containers from before mid-2024 may have another compose project; the local id still finds them. - ->toContain("label=coolify.serviceId={$service->id}") + // A numeric id can belong to another Coolify instance on the same server, so it never selects containers. + ->not->toContain("label=coolify.serviceId={$service->id}") ->toContain('sort -u') ->toContain('docker rm -f $container_ids') ->toContain("docker volume rm -f '{$service->uuid}_web-data'")