From 661b52dbe0f904c89f083d8c08294abb51e4b1e1 Mon Sep 17 00:00:00 2001
From: nexxo <nexxo@clupilot.local>
Date: Sat, 25 Jul 2026 12:44:22 +0200
Subject: [PATCH] fix(engine-b): clone recovery waits for lock; register rows
 before breadcrumb

- Clone recovery polls until the VM lock clears instead of advancing onto a
  possibly-incomplete clone.
- RegisterBackup/RegisterMonitoring create the local row BEFORE the run-resource
  breadcrumb, so a crash between them can't leave the row permanently missing.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 app/Provisioning/Steps/Customer/CloneVirtualMachine.php | 7 +++++--
 app/Provisioning/Steps/Customer/RegisterBackup.php      | 7 ++++---
 app/Provisioning/Steps/Customer/RegisterMonitoring.php  | 3 ++-
 app/Services/Proxmox/FakeProxmoxClient.php              | 8 +++++++-
 tests/Feature/Provisioning/CustomerStepsTest.php        | 4 ++++
 5 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/app/Provisioning/Steps/Customer/CloneVirtualMachine.php b/app/Provisioning/Steps/Customer/CloneVirtualMachine.php
index 4d86619..ec7d03d 100644
--- a/app/Provisioning/Steps/Customer/CloneVirtualMachine.php
+++ b/app/Provisioning/Steps/Customer/CloneVirtualMachine.php
@@ -49,9 +49,12 @@ class CloneVirtualMachine extends CustomerStep
         $upid = $run->context('clone_upid');
         if ($upid === null) {
             // Crash-recovery: if the VM already exists, a prior attempt cloned it
-            // (but lost the task ref) — don't re-clone the same vmid (Proxmox rejects).
+            // (but lost the task ref) — don't re-clone. Wait until the clone lock
+            // clears before advancing, in case the copy is still running.
             if ($pve->vmExists($node, $vmid)) {
-                return StepResult::advance();
+                return empty($pve->vmStatus($node, $vmid)['lock'])
+                    ? StepResult::advance()
+                    : StepResult::poll(10, 'finishing recovered clone');
             }
 
             $upid = $pve->cloneVm($node, $templateVmid, $vmid, $name);
diff --git a/app/Provisioning/Steps/Customer/RegisterBackup.php b/app/Provisioning/Steps/Customer/RegisterBackup.php
index 365d648..926535b 100644
--- a/app/Provisioning/Steps/Customer/RegisterBackup.php
+++ b/app/Provisioning/Steps/Customer/RegisterBackup.php
@@ -33,13 +33,14 @@ class RegisterBackup extends CustomerStep
         $jobId = $this->pve->forHost($instance->host)
             ->createBackupJob((string) $run->context('node'), (int) $instance->vmid, $schedule);
 
-        $this->recordResource($run, $instance->host, 'backup_job_id', $jobId);
-        // firstOrCreate keyed on the deterministic job id → no duplicate row if a
-        // crash re-runs this step before the breadcrumb was recorded.
+        // Create the local row BEFORE the breadcrumb — the breadcrumb is the
+        // short-circuit guard, so if it exists the row must already exist too.
+        // firstOrCreate on the deterministic job id keeps a retry duplicate-free.
         $instance->backups()->firstOrCreate(
             ['external_job_id' => $jobId],
             ['schedule' => $schedule, 'status' => 'scheduled'],
         );
+        $this->recordResource($run, $instance->host, 'backup_job_id', $jobId);
 
         return StepResult::advance();
     }
diff --git a/app/Provisioning/Steps/Customer/RegisterMonitoring.php b/app/Provisioning/Steps/Customer/RegisterMonitoring.php
index a73cc7c..67f0ae3 100644
--- a/app/Provisioning/Steps/Customer/RegisterMonitoring.php
+++ b/app/Provisioning/Steps/Customer/RegisterMonitoring.php
@@ -32,11 +32,12 @@ class RegisterMonitoring extends CustomerStep
         // Register with the monitoring service [E] before recording it.
         $targetId = $this->monitoring->registerTarget($instance->subdomain, $url);
 
-        $this->recordResource($run, $instance->host, 'monitoring_target_id', $targetId);
+        // Local row BEFORE the breadcrumb (the short-circuit guard).
         $instance->monitoringTargets()->firstOrCreate(
             ['external_id' => $targetId],
             ['url' => $url, 'status' => 'up'],
         );
+        $this->recordResource($run, $instance->host, 'monitoring_target_id', $targetId);
 
         return StepResult::advance();
     }
diff --git a/app/Services/Proxmox/FakeProxmoxClient.php b/app/Services/Proxmox/FakeProxmoxClient.php
index 314e7fd..6d5edb0 100644
--- a/app/Services/Proxmox/FakeProxmoxClient.php
+++ b/app/Services/Proxmox/FakeProxmoxClient.php
@@ -114,9 +114,15 @@ class FakeProxmoxClient implements ProxmoxClient
         return 'UPID:pve:qmstart:'.$vmid;
     }
 
+    /** Set to e.g. 'clone' to simulate a VM still locked by a running clone task. */
+    public ?string $vmLock = null;
+
     public function vmStatus(string $node, int $vmid): array
     {
-        return ['status' => in_array($vmid, $this->runningVmids, true) ? 'running' : 'stopped'];
+        return [
+            'status' => in_array($vmid, $this->runningVmids, true) ? 'running' : 'stopped',
+            'lock' => $this->vmLock,
+        ];
     }
 
     public function vmExists(string $node, int $vmid): bool
diff --git a/tests/Feature/Provisioning/CustomerStepsTest.php b/tests/Feature/Provisioning/CustomerStepsTest.php
index 39a8ea9..393e79f 100644
--- a/tests/Feature/Provisioning/CustomerStepsTest.php
+++ b/tests/Feature/Provisioning/CustomerStepsTest.php
@@ -132,6 +132,10 @@ it('recovers a clone whose task ref was lost (no duplicate clone)', function ()
 
     expect(app(CloneVirtualMachine::class)->execute($run->fresh())->type)->toBe('advance')
         ->and(count($s['pve']->clonedVmids))->toBe(1); // no second clone
+
+    // still locked (clone copy running) → poll, don't advance onto an incomplete VM
+    $s['pve']->vmLock = 'clone';
+    expect(app(CloneVirtualMachine::class)->execute($run->fresh())->type)->toBe('poll');
 });
 
 it('fails clone when the plan has no template', function () {