From 9f8659da326fb8a0d20f740ba7ed44d9f4865c87 Mon Sep 17 00:00:00 2001
From: nexxo <nexxo@clupilot.local>
Date: Sat, 25 Jul 2026 23:53:14 +0200
Subject: [PATCH] fix(hosts): a failed DNS deletion must not orphan the record
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Swallowing it and carrying on deleted the row that held the only reference to
that record, publishing the machine's management address for good. The purge now
fails instead — and is retried, since it re-reads the host and its other steps
are idempotent — so a broken DNS provider becomes a visible failed job rather
than a silent leak.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 app/Provisioning/Jobs/PurgeHost.php          | 26 ++++++++++++-------
 app/Services/Dns/FakeHetznerDnsClient.php    |  6 +++++
 tests/Feature/Provisioning/HostStepsTest.php | 27 ++++++++++++++++++++
 3 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/app/Provisioning/Jobs/PurgeHost.php b/app/Provisioning/Jobs/PurgeHost.php
index 72dc1b8..6a13a38 100644
--- a/app/Provisioning/Jobs/PurgeHost.php
+++ b/app/Provisioning/Jobs/PurgeHost.php
@@ -4,7 +4,6 @@ namespace App\Provisioning\Jobs;
 
 use App\Models\Host;
 use App\Services\Dns\HetznerDnsClient;
-use Illuminate\Support\Facades\Log;
 use App\Models\VpnPeer;
 use Illuminate\Bus\Queueable;
 use Illuminate\Contracts\Queue\ShouldQueue;
@@ -22,7 +21,16 @@ class PurgeHost implements ShouldQueue
 {
     use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
 
-    public $tries = 1;
+    // Retried, because the DNS deletion below can fail for a minute and the job
+    // is re-entrant: it re-reads the host, already-deleted runs are simply gone,
+    // and the peer removal is idempotent.
+    public $tries = 3;
+
+    /** @return list<int> seconds between attempts */
+    public function backoff(): array
+    {
+        return [30, 120];
+    }
 
     public $timeout = 2200;
 
@@ -50,13 +58,13 @@ class PurgeHost implements ShouldQueue
         // has to happen first — otherwise the host's management address stays
         // published with nothing left to clean it up.
         if (filled($host->dns_record_id)) {
-            try {
-                app(HetznerDnsClient::class)->deleteRecord($host->dns_record_id);
-            } catch (\Throwable $e) {
-                Log::warning('could not delete host DNS record', [
-                    'host' => $host->uuid, 'record' => $host->dns_record_id, 'error' => $e->getMessage(),
-                ]);
-            }
+            // Deliberately NOT swallowed: the row about to be deleted holds the
+            // only reference to that record, so continuing past a failure would
+            // leave the machine's management address published for good with no
+            // way left to find it. Failing here lets the queue retry, and a
+            // permanently broken DNS provider becomes a visible failed job
+            // rather than a silent leak.
+            app(HetznerDnsClient::class)->deleteRecord($host->dns_record_id);
         }
 
         if (filled($host->wg_pubkey)) {
diff --git a/app/Services/Dns/FakeHetznerDnsClient.php b/app/Services/Dns/FakeHetznerDnsClient.php
index ac482d3..aa8cca1 100644
--- a/app/Services/Dns/FakeHetznerDnsClient.php
+++ b/app/Services/Dns/FakeHetznerDnsClient.php
@@ -16,6 +16,8 @@ class FakeHetznerDnsClient implements HetznerDnsClient
 
     public bool $failUpsert = false;
 
+    public bool $failDelete = false;
+
     private int $counter = 1;
 
     public function upsertRecord(string $fqdn, string $type, string $value): string
@@ -31,6 +33,10 @@ class FakeHetznerDnsClient implements HetznerDnsClient
 
     public function deleteRecord(string $recordId): void
     {
+        if ($this->failDelete) {
+            throw new RuntimeException('hetzner dns 5xx');
+        }
+
         $this->records = array_filter($this->records, fn ($id) => $id !== $recordId);
     }
 }
diff --git a/tests/Feature/Provisioning/HostStepsTest.php b/tests/Feature/Provisioning/HostStepsTest.php
index 7b09019..a247725 100644
--- a/tests/Feature/Provisioning/HostStepsTest.php
+++ b/tests/Feature/Provisioning/HostStepsTest.php
@@ -317,3 +317,30 @@ it('takes the host DNS record with it when the host is purged', function () {
     // the identifier needed to remove it deleted along with the row.
     expect($s['dns']->records)->not->toHaveKey($fqdn);
 });
+
+it('keeps the host until its DNS record is really gone', function () {
+    $s = fakeServices();
+    app()->instance(App\Services\Dns\HetznerDnsClient::class, $s['dns']);
+
+    $host = App\Models\Host::factory()->active()->create([
+        'datacenter' => 'hel', 'wg_ip' => '10.66.0.60', 'dns_name' => null,
+    ]);
+    $run = App\Models\ProvisioningRun::factory()->forHost($host)->create(['pipeline' => 'host']);
+    (new App\Provisioning\Steps\Host\RegisterHostDns($s['dns']))->execute($run);
+
+    $s['dns']->failDelete = true;
+
+    // The row carries the only reference to that record — deleting it anyway
+    // would publish the machine's management address for good.
+    expect(fn () => (new App\Provisioning\Jobs\PurgeHost($host->uuid))->handle())
+        ->toThrow(RuntimeException::class);
+
+    expect(App\Models\Host::query()->whereKey($host->id)->exists())->toBeTrue();
+
+    // Once DNS is back, the purge completes.
+    $s['dns']->failDelete = false;
+    (new App\Provisioning\Jobs\PurgeHost($host->uuid))->handle();
+
+    expect(App\Models\Host::query()->whereKey($host->id)->exists())->toBeFalse()
+        ->and($s['dns']->records)->toBe([]);
+});