From 2c6e221db5b1b9871d47182d4d3a0d32b9894059 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Sun, 27 Sep 2026 21:09:13 +0200 Subject: [PATCH] feat(dns): release orphaned managed DNS records on a schedule An hourly command releases managed DNS records whose references all point to missing or deleted resources, or that have no references after a grace period. Each record goes through the per-hostname lock and re-check, so a hostname that a user adds back is kept. An empty reference list no longer deletes all references of a record. Co-Authored-By: Claude Opus 5.5 --- .../ReleaseOrphanedManagedDnsRecords.php | 31 ++ app/Console/Kernel.php | 5 + app/Services/Dns/ManagedDnsRecordCleanup.php | 61 ++++ .../ReleaseOrphanedManagedDnsRecordsTest.php | 287 ++++++++++++++++++ 4 files changed, 384 insertions(+) create mode 100644 app/Console/Commands/ReleaseOrphanedManagedDnsRecords.php create mode 100644 tests/Feature/ReleaseOrphanedManagedDnsRecordsTest.php diff --git a/app/Console/Commands/ReleaseOrphanedManagedDnsRecords.php b/app/Console/Commands/ReleaseOrphanedManagedDnsRecords.php new file mode 100644 index 0000000000..19fcec0e59 --- /dev/null +++ b/app/Console/Commands/ReleaseOrphanedManagedDnsRecords.php @@ -0,0 +1,31 @@ +releaseOrphanedRecords(); + if ($counts === []) { + $this->info('No orphaned managed DNS records found.'); + + return self::SUCCESS; + } + + ksort($counts); + $summary = collect($counts)->map(fn (int $count, string $outcome): string => "{$outcome}: {$count}")->implode(', '); + $this->info("Orphaned managed DNS records processed ({$summary})."); + Log::info('Orphaned managed DNS records processed.', $counts); + + return self::SUCCESS; + } +} diff --git a/app/Console/Kernel.php b/app/Console/Kernel.php index d5b0e5a9c5..32c6a2ee78 100644 --- a/app/Console/Kernel.php +++ b/app/Console/Kernel.php @@ -57,6 +57,11 @@ class Kernel extends ConsoleKernel ->withoutOverlapping(60) ->runInBackground(); $this->scheduleInstance->command('sanctum:prune-expired --hours=1')->hourly()->onOneServer(); + $this->scheduleInstance->command('dns:release-orphaned-records') + ->hourly() + ->onOneServer() + ->withoutOverlapping(60) + ->runInBackground(); $this->scheduleInstance->job(new ApiTokenExpirationWarningJob)->hourly()->onOneServer(); $this->scheduleInstance->job(new CheckMissingDatabaseBackupsJob)->hourly()->onOneServer(); diff --git a/app/Services/Dns/ManagedDnsRecordCleanup.php b/app/Services/Dns/ManagedDnsRecordCleanup.php index 06b1cecce4..2fdb4f57be 100644 --- a/app/Services/Dns/ManagedDnsRecordCleanup.php +++ b/app/Services/Dns/ManagedDnsRecordCleanup.php @@ -23,6 +23,11 @@ use Throwable; */ class ManagedDnsRecordCleanup { + /** + * A record without references is released only after this time, so a record that was just created is never touched. + */ + public const ORPHAN_GRACE_MINUTES = 60; + public function __construct( private CloudflareDnsProvider $provider, private ManagedDnsHostnameLock $hostnameLock, @@ -144,6 +149,57 @@ class ManagedDnsRecordCleanup return $successful; } + /** + * Releases orphaned records: records whose references all point to deleted (or soft-deleted) resources, and + * records without references that are older than the grace period. Records with a live reference are not touched. + * Each orphan goes through release(), so a hostname that a live resource uses again keeps its record, and + * Busy or Failed records stay for the next run. Never throws. + * + * @return array Number of records per outcome (a ManagedDnsDeletionResult value, "kept" or "forgotten"). + */ + public function releaseOrphanedRecords(): array + { + $counts = []; + $cutoff = now()->subMinutes(self::ORPHAN_GRACE_MINUTES); + + ManagedDnsRecord::query() + ->where(fn (Builder $query) => $query->has('references')->orWhere('created_at', '<=', $cutoff)) + ->with('references.resource') + ->chunkById(100, function (Collection $records) use (&$counts): void { + foreach ($records as $record) { + if ($record->references->contains(fn (ManagedDnsRecordReference $reference): bool => $reference->resource !== null)) { + continue; + } + + $outcome = $this->releaseOrphanedRecord($record); + $counts[$outcome] = ($counts[$outcome] ?? 0) + 1; + } + }); + + return $counts; + } + + private function releaseOrphanedRecord(ManagedDnsRecord $record): string + { + try { + $result = $this->release($record, [], true); + } catch (Throwable $e) { + Log::warning('Managed DNS orphaned record cleanup failed.', [ + 'managed_dns_record_id' => $record->id, + 'hostname' => $record->name, + 'error' => $e->getMessage(), + ]); + + return ManagedDnsDeletionResult::Failed->value; + } + + if ($result !== null) { + return $result->value; + } + + return ManagedDnsRecord::query()->whereKey($record->getKey())->exists() ? 'kept' : 'forgotten'; + } + /** * Runs under the hostname lock, so a concurrent create or reference for the same hostname either finishes before * the usage re-check or starts after the record is gone. When the lock is not free in time, nothing changes and @@ -333,6 +389,11 @@ class ManagedDnsRecordCleanup */ private function deleteReferences(ManagedDnsRecord $record, array $keys): void { + // An empty nested where adds no constraint and would delete every reference of the record. + if ($keys === []) { + return; + } + $this->whereResourceKeys($record->references()->getQuery(), $keys)->delete(); } diff --git a/tests/Feature/ReleaseOrphanedManagedDnsRecordsTest.php b/tests/Feature/ReleaseOrphanedManagedDnsRecordsTest.php new file mode 100644 index 0000000000..331b2a2cc7 --- /dev/null +++ b/tests/Feature/ReleaseOrphanedManagedDnsRecordsTest.php @@ -0,0 +1,287 @@ +set('app.maintenance.store', 'array'); + config()->set('app.id', 'test-instance'); + + InstanceSettings::unguarded(fn () => InstanceSettings::updateOrCreate( + ['id' => 0], + ['id' => 0, 'is_dns_validation_enabled' => false], + )); + + $this->team = Team::factory()->create(); + $this->server = Server::factory()->create(['team_id' => $this->team->id, 'ip' => '203.0.113.10']); + $this->destination = StandaloneDocker::withoutEvents(fn () => StandaloneDocker::firstOrCreate( + ['server_id' => $this->server->id, 'network' => 'coolify'], + ['uuid' => (string) Str::uuid(), 'name' => 'test-docker'], + )); + $this->project = Project::factory()->create(['team_id' => $this->team->id]); + $this->environment = Environment::factory()->create(['project_id' => $this->project->id]); + + $this->token = IntegrationToken::factory()->for($this->team)->create(['provider' => 'cloudflare', 'token' => 'secret']); + $this->zone = DnsProviderZone::factory()->for($this->token)->create(['provider_zone_id' => 'zone-1', 'name' => 'example.com']); +}); + +/** + * Creates an owned record for the application through the provider, then deletes the application without + * the model events, so the release job never runs (like a release job that gave up after a Cloudflare outage). + */ +function orphanedDnsRecord(object $test, string $hostname = 'app.example.com', bool $forceDelete = false): ManagedDnsRecord +{ + $application = createDnsTestApplication($test, "https://{$hostname}"); + $record = app(CloudflareDnsProvider::class)->createRecord($test->zone, $hostname, '203.0.113.10', $application); + Application::withoutEvents(fn () => $forceDelete ? $application->forceDelete() : $application->delete()); + + return $record; +} + +function releaseOrphanedDnsRecords(): void +{ + test()->artisan('dns:release-orphaned-records')->assertSuccessful(); +} + +test('an orphaned owned record is deleted in Cloudflare and forgotten', function (bool $forceDelete) { + $cloudflare = fakeCloudflareDns(); + $record = orphanedDnsRecord($this, forceDelete: $forceDelete); + + test()->artisan('dns:release-orphaned-records') + ->expectsOutputToContain('deleted: 1') + ->assertSuccessful(); + + expect(sentDnsRequests('DELETE'))->toBe(1) + ->and($cloudflare['records'])->toBeEmpty() + ->and($record->fresh())->toBeNull(); +})->with([ + 'soft-deleted resource' => false, + 'force-deleted resource' => true, +]); + +test('a record with a live reference is not touched', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + $deleted = createDnsTestApplication($this, 'https://app.example.com'); + $record->addReference($deleted); + Application::withoutEvents(fn () => $deleted->delete()); + $this->travel(2)->hours(); + + releaseOrphanedDnsRecords(); + + expect(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->fresh())->not->toBeNull() + ->and($record->references()->pluck('resource_id')->sort()->values()->all())->toBe([$application->id, $deleted->id]); +}); + +test('an orphan whose hostname a live resource uses again is kept and referenced again', function () { + $cloudflare = fakeCloudflareDns(); + $record = orphanedDnsRecord($this); + $readded = createDnsTestApplication($this, 'https://app.example.com'); + + releaseOrphanedDnsRecords(); + + expect(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->fresh())->not->toBeNull() + ->and($record->references()->pluck('resource_id')->all())->toBe([$readded->id]); +}); + +test('an orphan is kept for the next run while the hostname lock is held', function () { + $cloudflare = fakeCloudflareDns(); + app()->instance(ManagedDnsHostnameLock::class, new ManagedDnsHostnameLock(waitSeconds: 0)); + $record = orphanedDnsRecord($this); + $heldLock = Cache::lock(ManagedDnsHostnameLock::key($this->team->id, 'app.example.com'), 60); + expect($heldLock->get())->toBeTrue(); + + test()->artisan('dns:release-orphaned-records') + ->expectsOutputToContain('busy: 1') + ->assertSuccessful(); + + expect(sentDnsRequests('DELETE'))->toBe(0) + ->and($record->fresh())->not->toBeNull() + ->and($record->references()->count())->toBe(1); + + $heldLock->release(); + releaseOrphanedDnsRecords(); + + expect(sentDnsRequests('DELETE'))->toBe(1) + ->and($cloudflare['records'])->toBeEmpty() + ->and($record->fresh())->toBeNull(); +}); + +test('an orphan is kept for the next run when Cloudflare fails', function () { + $cloudflare = fakeCloudflareDns(); + $record = orphanedDnsRecord($this); + $cloudflareIsDown = true; + Http::fake(function () use (&$cloudflareIsDown) { + if ($cloudflareIsDown) { + throw new ConnectionException('Cloudflare is unreachable.'); + } + + return null; + }); + + test()->artisan('dns:release-orphaned-records') + ->expectsOutputToContain('failed: 1') + ->assertSuccessful(); + + expect($record->fresh())->not->toBeNull() + ->and($cloudflare['records'])->toHaveCount(1); + + $cloudflareIsDown = false; + $this->travel(2)->hours(); + releaseOrphanedDnsRecords(); + + expect($cloudflare['records'])->toBeEmpty() + ->and($record->fresh())->toBeNull(); +}); + +test('a record without references is kept during the grace period and released after it', function () { + $cloudflare = fakeCloudflareDns(); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10'); + + releaseOrphanedDnsRecords(); + + expect(sentDnsRequests('GET'))->toBe(1) + ->and(sentDnsRequests('DELETE'))->toBe(0) + ->and($record->fresh())->not->toBeNull(); + + $this->travel(ManagedDnsRecordCleanup::ORPHAN_GRACE_MINUTES + 1)->minutes(); + releaseOrphanedDnsRecords(); + + expect(sentDnsRequests('DELETE'))->toBe(1) + ->and($cloudflare['records'])->toBeEmpty() + ->and($record->fresh())->toBeNull(); +}); + +test('a reference added before the lock is taken keeps the record and the reference', function () { + $cloudflare = fakeCloudflareDns(); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10'); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $lock = new class extends ManagedDnsHostnameLock + { + public ?Closure $beforeNextRun = null; + + public function run(int $teamId, string $hostname, callable $callback): mixed + { + if ($this->beforeNextRun !== null) { + ($this->beforeNextRun)(); + $this->beforeNextRun = null; + } + + return parent::run($teamId, $hostname, $callback); + } + }; + $lock->beforeNextRun = fn () => $record->addReference($application); + app()->instance(ManagedDnsHostnameLock::class, $lock); + $this->travel(2)->hours(); + + releaseOrphanedDnsRecords(); + + expect(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->references()->pluck('resource_id')->all())->toBe([$application->id]); +}); + +test('a not owned orphan is only forgotten and never changed in Cloudflare', function () { + $cloudflare = fakeCloudflareDns([[ + 'id' => 'hand-made', 'type' => 'A', 'name' => 'app.example.com', 'content' => '203.0.113.10', + 'proxied' => true, 'ttl' => 1, 'comment' => null, + ]]); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + Application::withoutEvents(fn () => $application->delete()); + expect($record->owned)->toBeFalse(); + $requestsBefore = count(Http::recorded()); + + test()->artisan('dns:release-orphaned-records') + ->expectsOutputToContain('not_owned: 1') + ->assertSuccessful(); + + expect(count(Http::recorded()))->toBe($requestsBefore) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->fresh())->toBeNull(); +}); + +describe('instance sentinels', function () { + beforeEach(function () { + $this->rootTeam = Team::find(0) ?? Team::factory()->create(['id' => 0]); + $this->rootToken = IntegrationToken::factory()->for($this->rootTeam)->create(['provider' => 'cloudflare', 'token' => 'root-secret']); + $this->rootZone = DnsProviderZone::factory()->for($this->rootToken)->create(['provider_zone_id' => 'zone-root', 'name' => 'root.example.com']); + $rootProject = Project::factory()->create(['team_id' => 0]); + $this->rootEnvironment = Environment::factory()->create(['project_id' => $rootProject->id]); + $this->rootApplication = Application::factory()->create([ + 'id' => 0, + 'environment_id' => $this->rootEnvironment->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + 'fqdn' => 'https://coolify.root.example.com', + 'build_pack' => 'nixpacks', + ]); + $this->rootRecord = ManagedDnsRecord::factory()->owned()->create([ + 'id' => 0, 'dns_provider_zone_id' => $this->rootZone->id, 'provider_record_id' => 'record-root', + 'name' => 'coolify.root.example.com', 'content' => '203.0.113.10', + ]); + $this->rootRecord->addReference($this->rootApplication); + $this->cloudflareRecords = [[ + 'id' => 'record-root', 'type' => 'A', 'name' => 'coolify.root.example.com', 'content' => '203.0.113.10', + 'proxied' => false, 'ttl' => 1, 'comment' => $this->rootRecord->ownershipComment(), + ]]; + }); + + test('a record with id 0 of the root team is released when its resource with id 0 is gone', function () { + $cloudflare = fakeCloudflareDns($this->cloudflareRecords); + Application::withoutEvents(fn () => $this->rootApplication->delete()); + + releaseOrphanedDnsRecords(); + + expect(sentDnsRequests('DELETE'))->toBe(1) + ->and($cloudflare['records'])->toBeEmpty() + ->and(ManagedDnsRecord::query()->find(0))->toBeNull(); + }); + + test('a record with id 0 of a live root team resource with id 0 is kept and later orphans are still released', function () { + $cloudflare = fakeCloudflareDns($this->cloudflareRecords); + $orphan = orphanedDnsRecord($this); + + releaseOrphanedDnsRecords(); + + expect(sentDnsRequests('DELETE'))->toBe(1) + ->and(collect($cloudflare['records'])->keys()->all())->toBe(['record-root']) + ->and($orphan->fresh())->toBeNull() + ->and(ManagedDnsRecord::query()->find(0))->not->toBeNull() + ->and(ManagedDnsRecord::query()->find(0)->references()->pluck('resource_id')->all())->toBe([0]); + }); +}); + +test('the orphan clean-up is scheduled hourly on one server', function () { + $event = collect(app(Schedule::class)->events()) + ->first(fn ($event) => str_contains((string) $event->command, 'dns:release-orphaned-records')); + + expect($event)->not->toBeNull() + ->and($event->expression)->toBe('0 * * * *') + ->and($event->onOneServer)->toBeTrue() + ->and($event->withoutOverlapping)->toBeTrue(); +});