From dbaf4858703aa8919c09bf5534827a21d07cd5ab Mon Sep 17 00:00:00 2001
From: Eric Wang <37554696+ericwang401@users.noreply.github.com>
Date: Fri, 16 Oct 2026 01:15:27 +0000
Subject: [PATCH] fix(servers): check for the VM with vmExists() on reinstall,
 drop isVmDeleted()

rebuild() decided whether to stop and delete the old VM with
isVmDeleted(), which returns true for any ProxmoxConnectionException. A
reinstall while the node was unreachable or erroring was therefore taken
as "no VM": it skipped the delete, and the clone failed later on the
VMID still in use, leaving the server on install_failed with a confusing
error.

It now uses vmExists(), which only counts Proxmox's "does not exist" as
missing and rethrows anything else, so the reinstall request fails up
front, inside the controller's transaction, and the server keeps its
status. isVmDeleted() has no callers left and is removed.
---
 CHANGELOG.md                                  |  3 +++
 .../Servers/ServerBuildDispatchService.php    | 11 ++++++-----
 app/Services/Servers/ServerBuildService.php   | 19 ++++---------------
 .../Client/Servers/RetryInstallTest.php       | 17 +++++++++++++++++
 4 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a35002d2b82..e55df3825b8 100644
--- a/CHANGELOG.md
+++ b/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").
diff --git a/app/Services/Servers/ServerBuildDispatchService.php b/app/Services/Servers/ServerBuildDispatchService.php
index e430400383c..dbd89b5c585 100644
--- a/app/Services/Servers/ServerBuildDispatchService.php
+++ b/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),
         ];
 
diff --git a/app/Services/Servers/ServerBuildService.php b/app/Services/Servers/ServerBuildService.php
index 134efb71e69..40cb8154065 100644
--- a/app/Services/Servers/ServerBuildService.php
+++ b/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
      */
diff --git a/tests/Feature/Controllers/Client/Servers/RetryInstallTest.php b/tests/Feature/Controllers/Client/Servers/RetryInstallTest.php
index 307b73ef6dc..4ee3482e705 100644
--- a/tests/Feature/Controllers/Client/Servers/RetryInstallTest.php
+++ b/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();
+});