feat(servers): split Delete and Disconnect, and make Delete reliable

CHANGELOG.md

@@ -16,6 +16,14 @@ follows [Semantic Versioning](https://semver.org) guidelines.
   server every few seconds and move on once the work is done, with a spinning icon to show they're live; before, they
   stayed put until the page was refreshed.
 
+### Changed
+
+- Deleting a server and keeping its virtual machine are now separate actions. **Delete** always removes everything --
+  backups (including locked ones), the virtual machine on the node, and the server's entry in Convoy -- and when the
+  virtual machine is already gone from the node, it simply removes the Convoy entry. **Disconnect**, a new button next
+  to it, replaces the "Do not purge VM and related files" checkbox: it removes the server from Convoy and leaves the
+  virtual machine and its backups on the node. The API is unchanged; `DELETE` with `no_purge` still disconnects.
+
 ### Fixed
 
 - Fixed every error from Proxmox filling the log with three copies of the same stack trace -- about 256 lines per error,
@@ -27,6 +35,12 @@ follows [Semantic Versioning](https://semver.org) guidelines.
 - Fixed a failed reinstall of an already-failed server staying on "Installing" forever instead of going back to
   "Install failed". The failure handler wrote through the copy of the server captured when the reinstall started, and
   when that copy already read "install_failed" the write was skipped as a no-op.
+- Fixed deleting a server failing, over and over, when its virtual machine was already gone from the node or it had a
+  locked backup. A failed delete can now also be retried, or the server disconnected, straight from the "Failed to
+  Delete" screen, and a server stuck on "Deleting" can be disconnected.
+- Fixed a delete treating any error from Proxmox as "the virtual machine is gone". Only Proxmox's own "does not exist"
+  counts now, so a node that is briefly unreachable can no longer get a server removed from Convoy while its virtual
+  machine keeps running.
 
 
 ## v4.7.0-rc.2

app/Http/Controllers/Admin/ServerController.php

@@ -2,7 +2,6 @@
 
 namespace Convoy\Http\Controllers\Admin;
 
-use Convoy\Enums\Server\Status;
 use Convoy\Enums\Server\SuspensionAction;
 use Convoy\Exceptions\Repository\Proxmox\ProxmoxConnectionException;
 use Convoy\Http\Controllers\ApiController;
@@ -138,13 +137,14 @@ public function unsuspend(Server $server)
         return $this->returnNoContent();
     }
 
+    /**
+     * Deletes the server: its backups, its VM, then the Convoy entry. With
+     * `no_purge`, only disconnects it -- the Convoy entry goes and everything on
+     * the node stays.
+     */
     public function destroy(Request $request, Server $server)
     {
-        $this->connection->transaction(function () use ($server, $request) {
-            $server->update(['status' => Status::DELETING->value]);
-
-            $this->deletionService->handle($server, $request->input('no_purge', false));
-        });
+        $this->deletionService->handle($server, disconnect: $request->boolean('no_purge'));
 
         return $this->returnNoContent();
     }

app/Jobs/Server/PurgeBackupsJob.php

@@ -21,7 +21,10 @@ class PurgeBackupsJob implements ShouldQueue
 
     public int $timeout = 300;
 
-    public function __construct(protected int $serverId)
+    /**
+     * @param bool $force also delete locked backups, as deleting the server does
+     */
+    public function __construct(protected int $serverId, protected bool $force = false)
     {
         //
     }
@@ -37,6 +40,6 @@ public function handle(PurgeBackupsService $service): void
     {
         $server = Server::findOrFail($this->serverId);
 
-        $service->handle($server);
+        $service->handle($server, $this->force);
     }
 }

app/Jobs/Server/WaitUntilVmIsDeletedJob.php

@@ -35,10 +35,19 @@ public function handle(ServerBuildService $service): void
     {
         $server = Server::findOrFail($this->serverId);
 
-        $isDeleted = $service->isVmDeleted($server);
-
-        if (!$isDeleted) {
+        // Only Proxmox's "does not exist" counts as deleted. Any other error
+        // is thrown and retried: treating an outage as "gone" would let the
+        // chain drop the server from Convoy while its VM is still there.
+        if ($service->vmExists($server)) {
             $this->release(3);
         }
     }
+
+    /**
+     * Wait between retries after an error instead of re-polling at once.
+     */
+    public function backoff(): int
+    {
+        return 5;
+    }
 }

app/Services/Backups/BackupDeletionService.php

@@ -15,9 +15,12 @@ public function __construct(private ConnectionInterface     $connection,
     {
     }
 
-    public function handle(Backup $backup)
+    /**
+     * @param bool $force delete even a locked backup, as deleting its server does
+     */
+    public function handle(Backup $backup, bool $force = false)
     {
-        if ($backup->is_locked && ($backup->is_successful && !is_null($backup->completed_at))) {
+        if (! $force && $backup->is_locked && ($backup->is_successful && !is_null($backup->completed_at))) {
             throw new BackupLockedException();
         }
 

app/Services/Backups/PurgeBackupsService.php

@@ -15,12 +15,12 @@ public function __construct(
     {
     }
 
-    public function handle(Server $server)
+    public function handle(Server $server, bool $force = false)
     {
         $backups = $this->backupRepository->getNonFailedBackups($server)->get();
 
-        $backups->each(function (Backup $backup) {
-            $this->backupDeletionService->handle($backup);
+        $backups->each(function (Backup $backup) use ($force) {
+            $this->backupDeletionService->handle($backup, $force);
         });
     }
 }

app/Services/Servers/ServerBuildService.php

@@ -7,6 +7,8 @@
 use Convoy\Repositories\Proxmox\Server\ProxmoxConfigRepository;
 use Convoy\Repositories\Proxmox\Server\ProxmoxServerRepository;
 use Convoy\Exceptions\Repository\Proxmox\ProxmoxConnectionException;
+use Illuminate\Http\Client\RequestException;
+use Illuminate\Http\Client\Response;
 
 /**
  * Class SnapshotService
@@ -56,4 +58,45 @@ public function isVmDeleted(Server $server): bool
 
         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.
+     *
+     * @throws ProxmoxConnectionException
+     */
+    public function vmExists(Server $server): bool
+    {
+        try {
+            $this->configRepository->setServer($server)->getConfig();
+        } catch (ProxmoxConnectionException $e) {
+            if (self::isVmMissingError($e)) {
+                return false;
+            }
+
+            throw $e;
+        }
+
+        return true;
+    }
+
+    /**
+     * Proxmox answers any request about a VMID it doesn't have with a 500 and
+     * "Configuration file 'nodes/<node>/qemu-server/<vmid>.conf' does not exist".
+     */
+    public static function isVmMissingError(\Throwable $e): bool
+    {
+        // The response sits on the exception itself, or -- before it kept one --
+        // on the RequestException it wrapped.
+        $response = $e->response
+            ?? ($e->getPrevious() instanceof RequestException ? $e->getPrevious()->response : null);
+
+        return $response instanceof Response
+            && $response->status() === 500
+            && str_contains((string) $response->json('message'), 'does not exist');
+    }
 }

app/Services/Servers/ServerDeletionService.php

@@ -10,45 +10,80 @@
 
 class ServerDeletionService
 {
-    public function __construct(private ServerBuildDispatchService $buildDispatchService)
+    /**
+     * States in which a job chain is still working on the server. Deleting
+     * then would race it; disconnecting is still allowed, as the way out when
+     * one of those chains has died and left the server stuck.
+     */
+    private const BUSY_STATES = [
+        Status::INSTALLING,
+        Status::RESTORING_BACKUP,
+        Status::RESTORING_SNAPSHOT,
+        Status::DELETING,
+    ];
+
+    public function __construct(
+        private ServerBuildDispatchService $buildDispatchService,
+        private ServerBuildService         $buildService,
+    )
     {
     }
 
-    public function handle(Server $server, bool $noPurge = false)
+    /**
+     * @param bool $disconnect remove the server from Convoy only, leaving the VM
+     *                         and its backups on the node untouched
+     */
+    public function handle(Server $server, bool $disconnect = false): void
     {
+        if ($disconnect) {
+            $server->delete();
+
+            return;
+        }
+
         $this->validateStatus($server);
 
+        // Checked now rather than in the chain so the admin hears straight away
+        // when the node can't be asked: vmExists() throws on anything other than
+        // Proxmox's "does not exist", and the server is left as it was.
+        $vmExists = $this->buildService->vmExists($server);
+
         $server->update(['status' => Status::DELETING->value]);
 
-        if (! $noPurge) {
-            Bus::chain([
-                new PurgeBackupsJob($server->id),
-                ...$this->buildDispatchService->getChainedDeleteJobs($server),
-                function () use ($server) {
-                    Server::findOrFail($server->id)->delete();
-                },
-            ])
-                ->catch(fn () => $server->update(['status' => Status::DELETION_FAILED->value]))
-                ->dispatch();
+        $serverId = $server->id;
 
-            return;
-        }
-
-        $server->delete();
+        Bus::chain([
+            // Force: locked backups belong to the server being deleted too.
+            new PurgeBackupsJob($serverId, force: true),
+            // Already gone from the node -- a failed install, or deleted by hand
+            // in Proxmox -- so only the Convoy entry is left to remove.
+            ...($vmExists ? $this->buildDispatchService->getChainedDeleteJobs($server) : []),
+            static function () use ($serverId) {
+                Server::whereKey($serverId)->first()?->delete();
+            },
+        ])
+            // By ID, not the captured model: see ServerBuildDispatchService.
+            ->catch(static fn () => Server::whereKey($serverId)
+                ->update(['status' => Status::DELETION_FAILED->value]))
+            ->dispatch();
     }
 
-    public function validateStatus(Server $server, bool $verifyStatusOnly = false)
+    /**
+     * A delete can start from any state except one where a chain is still
+     * running -- including deletion_failed, so a failed delete can be retried --
+     * and not while a backup is being taken, since Proxmox holds a lock on the
+     * VM for the backup and would refuse to destroy it.
+     *
+     * @throws ServerStatusConflictException
+     */
+    public function validateStatus(Server $server): void
     {
-        if (
-            ! is_null($server->status) && $server->status !== Status::DELETING->value
-        ) {
+        if (in_array($server->status, array_map(fn (Status $s) => $s->value, self::BUSY_STATES), true)) {
             throw new ServerStatusConflictException($server);
         }
 
-        if (! $verifyStatusOnly) {
-            if ($server->backups()->whereNull('completed_at')->exists()) {
-                throw new ServerStatusConflictException($server);
-            }
+        if ($server->backups()->whereNull('completed_at')->exists()) {
+            throw new ServerStatusConflictException($server);
         }
     }
 }

lang/en_US/admin/servers/settings.php

@@ -24,15 +24,18 @@
     ],
     'deletion' => [
         'title' => 'Delete Server',
-        'description' => 'The server will be deleted from Convoy. Backups and other associated data will be
-                                destroyed. However, you can tick the checkbox below to keep the virtual machine and data
-                                on the Proxmox node.',
+        'description' => 'Delete removes the server completely: its backups, its virtual machine on the Proxmox node, and its entry in Convoy. If the virtual machine is already gone from the node, only the Convoy entry is removed. Disconnect removes the server from Convoy only, and leaves the virtual machine and its backups on the node untouched.',
         'deleting_status' => 'This server is currently being deleted.',
-        'no_purge' => 'Do not purge VM and related files',
+        'failed_status' => 'The last attempt to delete this server failed. You can delete it again, or disconnect it to remove it from Convoy only.',
+        'disconnect' => 'Disconnect',
         'confirmation' => [
             'title' => 'Delete :name',
-            'description' => 'Are you sure you want to delete :name?'
-        ]
+            'description' => 'Are you sure you want to delete :name? Its virtual machine and backups will be destroyed on the Proxmox node.'
+        ],
+        'disconnect_confirmation' => [
+            'title' => 'Disconnect :name',
+            'description' => 'Remove :name from Convoy? Its virtual machine and backups stay on the Proxmox node, and Convoy will no longer manage them.'
+        ],
     ],
     'build' => [
         'title' => 'Server Build',