fix(hosts): reserve the DNS name under the lock, never reuse it, clean it up

Codex was right that my never-reuse claim did not hold:

- The lock was released with only a candidate in hand, so two concurrent
  onboardings in one datacenter could pick the same name and overwrite each
  other's record. The name is now persisted on the host while still locked.
- The number was derived from the hosts that happen to exist, so removing the
  highest one handed that number straight back out — a cached name would then
  resolve to a different machine. The counter is stored per datacenter, floored
  by what is actually in use so a wiped settings store cannot reissue live names.
- PurgeHost deleted the row that carried the record id, leaving the machine's
  management address published with nothing left to clean it up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

app/Provisioning/Jobs/PurgeHost.php

@@ -3,6 +3,8 @@
 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;
@@ -44,6 +46,19 @@ class PurgeHost implements ShouldQueue
             Cache::lock('run:'.$run->uuid, 2200)->block(2100, fn () => $run->delete());
         }
 
+        // The record identifier lives on the row about to be deleted, so this
+        // 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(),
+                ]);
+            }
+        }
+
         if (filled($host->wg_pubkey)) {
             // Tombstone the console's view of this peer as well. The FK only
             // nulls host_id, which would leave an enabled-but-absent row behind:

app/Provisioning/Steps/Host/RegisterHostDns.php

@@ -6,6 +6,7 @@ use App\Models\Host;
 use App\Models\ProvisioningRun;
 use App\Provisioning\StepResult;
 use App\Services\Dns\HetznerDnsClient;
+use App\Support\Settings;
 use Illuminate\Support\Facades\Cache;
 use Throwable;
 
@@ -37,7 +38,10 @@ class RegisterHostDns extends HostStep
             return StepResult::fail('The host has no management address to publish.');
         }
 
-        $name = $host->dns_name ?: $this->allocateName($host);
+        // Reserved and persisted before anything is published: releasing the
+        // lock with only a candidate in hand let two concurrent onboardings pick
+        // the same name and overwrite each other's record.
+        $name = $host->dns_name ?: $this->reserveName($host);
         $fqdn = $name.'.node.'.config('provisioning.dns.zone', 'clupilot.com');
 
         try {
@@ -52,29 +56,40 @@ class RegisterHostDns extends HostStep
             return StepResult::advance();
         }
 
-        $host->update(['dns_name' => $name, 'dns_record_id' => $recordId]);
+        $host->update(['dns_record_id' => $recordId]);
 
         return StepResult::advance();
     }
 
     /**
-     * <datacenter>-<nn>, numbered per datacenter and never reused: the number
+     * <datacenter>-<nn>, numbered per datacenter and never reused.
-     * is one past the highest ever issued there, so removing a host does not
+     *
-     * renumber the others.
+     * The counter is stored rather than derived from the hosts that happen to
+     * exist: deriving it hands the highest number straight back out after that
+     * host is removed, so a cached name would resolve to a different machine.
      */
-    private function allocateName(Host $host): string
+    private function reserveName(Host $host): string
     {
-        return Cache::lock('host-dns-name:'.$host->datacenter, 15)->block(10, function () use ($host) {
+        return Cache::lock('host-dns-name:'.$host->datacenter, 30)->block(10, function () use ($host) {
-            $taken = Host::query()
+            $key = 'dns.sequence.'.$host->datacenter;
+            $next = ((int) Settings::get($key, 0)) + 1;
+
+            // Never below what is already in use — a settings store that was
+            // wiped must not start handing out live names again.
+            $inUse = Host::query()
                 ->where('datacenter', $host->datacenter)
                 ->whereNotNull('dns_name')
-                ->pluck('dns_name');
+                ->pluck('dns_name')
-
-            $highest = $taken
                 ->map(fn (string $name) => (int) substr($name, strrpos($name, '-') + 1))
                 ->max() ?? 0;
+            $next = max($next, $inUse + 1);
 
-            return sprintf('%s-%02d', $host->datacenter, $highest + 1);
+            $name = sprintf('%s-%02d', $host->datacenter, $next);
+
+            Settings::set($key, $next);
+            $host->update(['dns_name' => $name]);   // reserved while still locked
+
+            return $name;
         });
     }
 }

tests/Feature/Provisioning/HostStepsTest.php

@@ -276,8 +276,10 @@ it('numbers hosts per datacenter and never reuses a number', function () {
 
     expect($names)->toBe(['fsn-01', 'fsn-02', 'hel-01']);
 
-    // Removing fsn-01 must not renumber fsn-02 onto its name.
+    // Removing the HIGHEST one is the dangerous case: deriving the number from
-    App\Models\Host::query()->where('dns_name', 'fsn-01')->delete();
+    // live hosts would hand fsn-02 straight back out, and a cached name would
+    // then point at a different machine.
+    App\Models\Host::query()->where('dns_name', 'fsn-02')->delete();
     $host = App\Models\Host::factory()->active()->create(['datacenter' => 'fsn', 'wg_ip' => '10.66.0.30', 'dns_name' => null]);
     $run = App\Models\ProvisioningRun::factory()->forHost($host)->create(["pipeline" => "host"]);
     (new App\Provisioning\Steps\Host\RegisterHostDns($s['dns']))->execute($run);
@@ -295,3 +297,23 @@ it('does not strand an onboarding because DNS was unreachable', function () {
     expect((new App\Provisioning\Steps\Host\RegisterHostDns($s['dns']))->execute($run)->type)->toBe("advance");
     expect($run->events()->where('step', 'register_host_dns')->exists())->toBeTrue();
 });
+
+it('takes the host DNS record with it when the host is purged', function () {
+    $s = fakeServices();
+    app()->instance(App\Services\Dns\HetznerDnsClient::class, $s['dns']);
+
+    $host = App\Models\Host::factory()->active()->create([
+        'datacenter' => 'fsn', 'wg_ip' => '10.66.0.50', 'dns_name' => null,
+    ]);
+    $run = App\Models\ProvisioningRun::factory()->forHost($host)->create(['pipeline' => 'host']);
+    (new App\Provisioning\Steps\Host\RegisterHostDns($s['dns']))->execute($run);
+
+    $fqdn = $host->fresh()->dns_name.'.node.clupilot.com';
+    expect($s['dns']->records)->toHaveKey($fqdn);
+
+    (new App\Provisioning\Jobs\PurgeHost($host->uuid))->handle();
+
+    // Otherwise the machine's management address stays published for good, with
+    // the identifier needed to remove it deleted along with the row.
+    expect($s['dns']->records)->not->toHaveKey($fqdn);
+});