fix(servers): check for the VM with vmExists() on reinstall, drop isVmDeleted()

CHANGELOG.md

@@ -26,6 +26,9 @@ follows [Semantic Versioning](https://semver.org) guidelines.
 
 ### Fixed
 
+- Fixed reinstalling a server while its node couldn't be reached being taken as "there's no VM": the reinstall then
+  skipped removing the old VM and failed later on its taken VMID. It now fails straight away with the node's error, so
+  it can just be retried.
 - Fixed every error from Proxmox filling the log with three copies of the same stack trace -- about 256 lines per error,
   every few seconds while a page polled a server whose VM was gone. An error is now logged once, with one trace, and its
   message is Proxmox's own ("Proxmox returned 500: Configuration file ... does not exist").

app/Services/Servers/ServerBuildDispatchService.php

@@ -49,12 +49,13 @@ public function rebuild(ServerDeploymentData $deployment): void
         // A retry after a failed install can find no VM at all -- the build
         // may have died before or during the clone. Stopping and deleting a VM
         // that does not exist fails on Proxmox's "does not exist", which would
-        // fail the whole chain, so go straight to building instead. If the VM
-        // does exist after all, the clone refuses its VMID and nothing is lost.
+        // fail the whole chain, so go straight to building instead. vmExists()
+        // throws on any other Proxmox error, so a node that can't be asked fails
+        // the reinstall up front rather than being taken for "no VM".
         $jobs = [
-            ...($this->buildService->isVmDeleted($deployment->server)
-                ? []
-                : $this->getChainedDeleteJobs($deployment->server)),
+            ...($this->buildService->vmExists($deployment->server)
+                ? $this->getChainedDeleteJobs($deployment->server)
+                : []),
             ...$this->getChainedBuildJobs($deployment),
         ];
 

app/Services/Servers/ServerBuildService.php

@@ -48,24 +48,13 @@ public function isVmCreated(Server $server): bool
         return true;
     }
 
-    public function isVmDeleted(Server $server): bool
-    {
-        try {
-            $this->configRepository->setServer($server)->getConfig();
-        } catch (ProxmoxConnectionException $e) {
-            return true;
-        }
-
-        return false;
-    }
-
     /**
      * Whether the server's VM exists on its node.
      *
-     * Unlike isVmDeleted(), only Proxmox's own "does not exist" answer counts as
-     * missing. Any other failure -- the node unreachable, a bad token, a 5xx --
-     * is rethrown, so an outage can never be mistaken for a VM that's gone and
-     * lead to a server being dropped from Convoy while its VM keeps running.
+     * Only Proxmox's own "does not exist" answer counts as missing. Any other
+     * failure -- the node unreachable, a bad token, a 5xx -- is rethrown, so an
+     * outage can never be mistaken for a VM that's gone and lead to a server
+     * being dropped from Convoy while its VM keeps running.
      *
      * @throws ProxmoxConnectionException
      */

tests/Feature/Controllers/Client/Servers/RetryInstallTest.php

@@ -117,3 +117,20 @@ function fakeMissingVm(): void
 
     Queue::assertNothingPushed();
 });
+
+it('fails a retry up front when the node cannot be asked', function () {
+    Http::fake(['*' => Http::response(['data' => null, 'message' => 'proxy error'], 503)]);
+
+    [$user, $_, $node, $server] = createServerModel();
+    $server->update(['status' => Status::INSTALL_FAILED->value]);
+
+    $this->actingAs($user)->postJson("/api/client/servers/{$server->uuid}/settings/reinstall", [
+        'template_uuid' => createTemplateFor($node)->uuid,
+        'account_password' => 'Rt-Aa1!bcdefgh9',
+        'start_on_completion' => false,
+    ])->assertServerError();
+
+    // Not taken for "no VM": nothing is queued and the server is left as it was.
+    expect($server->fresh()->status)->toBe(Status::INSTALL_FAILED->value);
+    Queue::assertNothingPushed();
+});