From e5ed1559c70da960115ae045f3c20f332580f7e0 Mon Sep 17 00:00:00 2001 From: Eric Wang <37554696+ericwang401@users.noreply.github.com> Date: Tue, 29 Sep 2026 04:12:50 +0000 Subject: [PATCH 1/3] fix(servers): only count Proxmox "does not exist" as a missing VM isVmDeleted() returns true for any ProxmoxConnectionException -- a 401, a 503, a proxy error -- so WaitUntilVmIsDeletedJob could conclude a VM was gone while the node was merely unreachable, and the deletion chain would then drop the server from Convoy with its VM still running. Add vmExists(), which returns false only for Proxmox's 500 "Configuration file ... does not exist" and rethrows anything else, and use it in WaitUntilVmIsDeletedJob. That job also gets a 5s backoff, so an outage is retried at a sane pace rather than in a tight loop. --- app/Jobs/Server/WaitUntilVmIsDeletedJob.php | 15 ++++++-- app/Services/Servers/ServerBuildService.php | 39 +++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/app/Jobs/Server/WaitUntilVmIsDeletedJob.php b/app/Jobs/Server/WaitUntilVmIsDeletedJob.php index d8e3a890ebb..be492b0c87b 100644 --- a/app/Jobs/Server/WaitUntilVmIsDeletedJob.php +++ b/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; + } } diff --git a/app/Services/Servers/ServerBuildService.php b/app/Services/Servers/ServerBuildService.php index a17fb522b1d..c574e672d43 100644 --- a/app/Services/Servers/ServerBuildService.php +++ b/app/Services/Servers/ServerBuildService.php @@ -7,6 +7,7 @@ use Convoy\Repositories\Proxmox\Server\ProxmoxConfigRepository; use Convoy\Repositories\Proxmox\Server\ProxmoxServerRepository; use Convoy\Exceptions\Repository\Proxmox\ProxmoxConnectionException; +use Illuminate\Http\Client\RequestException; /** * Class SnapshotService @@ -56,4 +57,42 @@ 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//qemu-server/.conf' does not exist". + */ + public static function isVmMissingError(\Throwable $e): bool + { + $request = $e->getPrevious(); + + return $request instanceof RequestException + && $request->response->status() === 500 + && str_contains((string) $request->response->json('message'), 'does not exist'); + } } From 3e5fce2155b24493fd4a105f4312b96ede077bcd Mon Sep 17 00:00:00 2001 From: Eric Wang <37554696+ericwang401@users.noreply.github.com> Date: Tue, 29 Sep 2026 04:12:51 +0000 Subject: [PATCH 2/3] feat(servers): split Delete and Disconnect, and make Delete reliable Delete now always removes everything and can't get stuck: - backups are purged with force, so a locked backup no longer fails it; - when the VM is already gone from the node (vmExists), only the Convoy entry is removed instead of failing on "does not exist"; - it's allowed from deletion_failed, so a failed delete can be retried; - it's refused (409) only while another chain is running on the server, or a backup is in progress; - the failure handler writes by ID, like build/rebuild. The "Do not purge" checkbox becomes a Disconnect button (outline, next to Delete): it removes the Convoy entry and leaves the VM and backups on the node, from any state -- the way out for a server stuck on "Deleting". The API stays compatible: DELETE with no_purge disconnects. Both buttons also show on the admin's "Deleting"/"Failed to Delete" screen, since the settings page isn't reachable in those states. Tested with Pest (chain shapes, outage leaves the server alone, retry from deletion_failed, busy states refused, disconnect from any state, failure handler, forced purge, vmExists) and on a real Proxmox node: delete with the VM present, delete with it already removed, disconnect. --- CHANGELOG.md | 19 ++ .../Controllers/Admin/ServerController.php | 12 +- app/Jobs/Server/PurgeBackupsJob.php | 7 +- .../Backups/BackupDeletionService.php | 7 +- app/Services/Backups/PurgeBackupsService.php | 6 +- .../Servers/ServerDeletionService.php | 83 ++++++--- lang/en_US/admin/servers/settings.php | 15 +- .../partials/general/DeleteServerActions.tsx | 129 ++++++++++++++ .../partials/general/DeleteServerCard.tsx | 158 +++-------------- .../scripts/routers/AdminServerRouter.tsx | 4 +- routes/api-admin.php | 6 +- routes/api-application.php | 6 +- .../Controllers/Admin/ServerDeletionTest.php | 163 ++++++++++++++++++ 13 files changed, 437 insertions(+), 178 deletions(-) create mode 100644 resources/scripts/components/admin/servers/settings/partials/general/DeleteServerActions.tsx create mode 100644 tests/Feature/Controllers/Admin/ServerDeletionTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index deb96919c74..69a26066466 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,25 @@ This file is a running track of new features and fixes to each version of the pa The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project follows [Semantic Versioning](https://semver.org) guidelines. +## Unreleased + +### 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 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 ### Fixed diff --git a/app/Http/Controllers/Admin/ServerController.php b/app/Http/Controllers/Admin/ServerController.php index 354268d34df..7a9951c1ce4 100644 --- a/app/Http/Controllers/Admin/ServerController.php +++ b/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(); } diff --git a/app/Jobs/Server/PurgeBackupsJob.php b/app/Jobs/Server/PurgeBackupsJob.php index 6d9a6bcbbf2..86012581bce 100644 --- a/app/Jobs/Server/PurgeBackupsJob.php +++ b/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); } } diff --git a/app/Services/Backups/BackupDeletionService.php b/app/Services/Backups/BackupDeletionService.php index 63d77c3f33c..ccf25c701d3 100644 --- a/app/Services/Backups/BackupDeletionService.php +++ b/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(); } diff --git a/app/Services/Backups/PurgeBackupsService.php b/app/Services/Backups/PurgeBackupsService.php index 32f6dcd8efb..fe543fe44a0 100644 --- a/app/Services/Backups/PurgeBackupsService.php +++ b/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); }); } } diff --git a/app/Services/Servers/ServerDeletionService.php b/app/Services/Servers/ServerDeletionService.php index ff42bf91437..3d1e502a348 100644 --- a/app/Services/Servers/ServerDeletionService.php +++ b/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); } } } diff --git a/lang/en_US/admin/servers/settings.php b/lang/en_US/admin/servers/settings.php index 57e0420e064..ae9809f3814 100644 --- a/lang/en_US/admin/servers/settings.php +++ b/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', diff --git a/resources/scripts/components/admin/servers/settings/partials/general/DeleteServerActions.tsx b/resources/scripts/components/admin/servers/settings/partials/general/DeleteServerActions.tsx new file mode 100644 index 00000000000..b3d31ad489a --- /dev/null +++ b/resources/scripts/components/admin/servers/settings/partials/general/DeleteServerActions.tsx @@ -0,0 +1,129 @@ +import { AdminServerContext } from '@/state/admin/server' +import { useFlashKey } from '@/util/useFlash' +import { useState } from 'react' +import { useTranslation } from 'react-i18next' +import { useNavigate } from 'react-router-dom' + +import deleteServer from '@/api/admin/servers/deleteServer' + +import Button from '@/components/elements/Button' +import FlashMessageRender from '@/components/elements/FlashMessageRenderer' +import Modal from '@/components/elements/Modal' + + +type Action = 'delete' | 'disconnect' + +interface Props { + className?: string +} + +// Delete and Disconnect, with their confirmations. Shared by the settings card +// and the admin's "Deleting"/"Failed to Delete" screen, where the settings +// aren't reachable. +const DeleteServerActions = ({ className }: Props) => { + const server = AdminServerContext.useStoreState(state => state.server.data!) + const setServer = AdminServerContext.useStoreActions( + actions => actions.server.setServer + ) + const { clearFlashes, clearAndAddHttpError } = useFlashKey( + `admin.servers.${server.uuid}.settings.general.delete` + ) + const { t: tStrings } = useTranslation('strings') + const { t } = useTranslation('admin.servers.settings') + const navigate = useNavigate() + const [confirming, setConfirming] = useState(null) + const [submitting, setSubmitting] = useState(false) + + const submit = async (action: Action) => { + clearFlashes() + setSubmitting(true) + try { + // Disconnecting is the API's no_purge: only the Convoy entry goes. + await deleteServer(server.uuid, action === 'disconnect') + + if (action === 'disconnect') { + navigate('/admin/servers') + return + } + + setServer({ ...server, status: 'deleting' }) + setConfirming(null) + } catch (error) { + clearAndAddHttpError(error as any) + setConfirming(null) + } finally { + setSubmitting(false) + } + } + + return ( + <> + +
+ + +
+ + setConfirming(null)}> + + + {confirming === 'disconnect' + ? t('deletion.disconnect_confirmation.title', { + name: server.name, + }) + : t('deletion.confirmation.title', { + name: server.name, + })} + + + + + {confirming === 'disconnect' + ? t('deletion.disconnect_confirmation.description', { + name: server.name, + }) + : t('deletion.confirmation.description', { + name: server.name, + })} + + + + setConfirming(null)}> + {tStrings('cancel')} + + confirming && submit(confirming)} + > + {confirming === 'disconnect' + ? t('deletion.disconnect') + : tStrings('delete')} + + + + + ) +} + +export default DeleteServerActions diff --git a/resources/scripts/components/admin/servers/settings/partials/general/DeleteServerCard.tsx b/resources/scripts/components/admin/servers/settings/partials/general/DeleteServerCard.tsx index 0dfc6125a96..2a9ac7d3761 100644 --- a/resources/scripts/components/admin/servers/settings/partials/general/DeleteServerCard.tsx +++ b/resources/scripts/components/admin/servers/settings/partials/general/DeleteServerCard.tsx @@ -1,147 +1,41 @@ import { AdminServerContext } from '@/state/admin/server' -import { useFlashKey } from '@/util/useFlash' -import { zodResolver } from '@hookform/resolvers/zod' -import { FormEvent, useState } from 'react' -import { FormProvider, useForm } from 'react-hook-form' import { useTranslation } from 'react-i18next' -import { z } from 'zod' -import deleteServer from '@/api/admin/servers/deleteServer' - -import Button from '@/components/elements/Button' -import FlashMessageRender from '@/components/elements/FlashMessageRenderer' import FormCard from '@/components/elements/FormCard' import MessageBox from '@/components/elements/MessageBox' -import Modal from '@/components/elements/Modal' -import CheckboxForm from '@/components/elements/forms/CheckboxForm' + +import DeleteServerActions from '@/components/admin/servers/settings/partials/general/DeleteServerActions' const DeleteServerCard = () => { const server = AdminServerContext.useStoreState(state => state.server.data!) - const setServer = AdminServerContext.useStoreActions( - actions => actions.server.setServer - ) - const { clearFlashes, clearAndAddHttpError } = useFlashKey( - `admin.servers.${server.uuid}.settings.general.delete` - ) - const { t: tStrings } = useTranslation('strings') const { t } = useTranslation('admin.servers.settings') - const [showConfirmation, setShowConfirmation] = useState(false) - - const schema = z.object({ - noPurge: z.boolean(), - }) - - const form = useForm({ - resolver: zodResolver(schema), - defaultValues: { - noPurge: false, - }, - }) - - const submit = async ({ noPurge }: z.infer) => { - clearFlashes() - try { - await deleteServer(server.uuid, noPurge) - - setServer({ - ...server, - status: 'deleting', - }) - } catch (error) { - clearAndAddHttpError(error as any) - } - } - - const triggerConfirmation = (e: FormEvent) => { - e.preventDefault() - setShowConfirmation(true) - } return ( - <> - - -
- - - {t('deletion.title')} - -
- - -

- {t('deletion.description')} -

- {server.status === 'deleting' && ( - - {t('deletion.deleting_status')} - - )} - - -
- - - - -
- - - - setShowConfirmation(false)} - > - - - {t('deletion.confirmation.title', { - name: server.name, - })} - - - - - {t('deletion.confirmation.description', { - name: server.name, - })} - - - - setShowConfirmation(false)} - > - {tStrings('cancel')} - - - {tStrings('delete')} - - - - + + + {t('deletion.title')} +
+

+ {t('deletion.description')} +

+ {server.status === 'deleting' && ( + + {t('deletion.deleting_status')} + + )} + {server.status === 'deletion_failed' && ( + + {t('deletion.failed_status')} + + )} +
+ + + + + ) } -export default DeleteServerCard \ No newline at end of file +export default DeleteServerCard diff --git a/resources/scripts/routers/AdminServerRouter.tsx b/resources/scripts/routers/AdminServerRouter.tsx index c178957569b..aa2b77e7376 100644 --- a/resources/scripts/routers/AdminServerRouter.tsx +++ b/resources/scripts/routers/AdminServerRouter.tsx @@ -13,6 +13,7 @@ import Spinner from '@/components/elements/Spinner' import { NavigationBarContext } from '@/components/elements/navigation/NavigationBar' import RestoreAccessButton from '@/components/admin/servers/RestoreAccessButton' +import DeleteServerActions from '@/components/admin/servers/settings/partials/general/DeleteServerActions' export const routes: Route[] = [ { @@ -157,7 +158,7 @@ const AdminServerRouter = () => { message={ server.status === 'deleting' ? "This server is being deleted. If you think this is an error, click below to make the server accessible. Clicking the button will not stop the server from being deleted if it's in progress" - : 'This server failed to delete. if you think this is an error, click below to make the server accessible. The server MAY not be in a usable state.' + : 'This server failed to delete. Delete it again, disconnect it to remove it from Convoy only, or make it accessible again if you think this is an error. The server MAY not be in a usable state.' } title={ server.status === 'deleting' @@ -166,6 +167,7 @@ const AdminServerRouter = () => { } > + ) : ( diff --git a/routes/api-admin.php b/routes/api-admin.php index 58455370f0a..a3a2c468fa4 100644 --- a/routes/api-admin.php +++ b/routes/api-admin.php @@ -135,7 +135,11 @@ function () { Route::patch('/', [Admin\ServerController::class, 'update'])->withoutMiddleware( ValidateServerStatusMiddleware::class, ); - Route::delete('/', [Admin\ServerController::class, 'destroy']); + // The deletion service checks the status itself: it has to allow + // deletion_failed (to retry) and disconnecting from any state. + Route::delete('/', [Admin\ServerController::class, 'destroy'])->withoutMiddleware( + ValidateServerStatusMiddleware::class, + ); Route::prefix('/settings')->group(function () { Route::patch('/build', [Admin\ServerController::class, 'updateBuild']); diff --git a/routes/api-application.php b/routes/api-application.php index 1e60198cc63..ed7c61e011a 100644 --- a/routes/api-application.php +++ b/routes/api-application.php @@ -125,7 +125,11 @@ function () { Route::patch('/', [Admin\ServerController::class, 'update'])->withoutMiddleware( ValidateServerStatusMiddleware::class, ); - Route::delete('/', [Admin\ServerController::class, 'destroy']); + // The deletion service checks the status itself: it has to allow + // deletion_failed (to retry) and disconnecting from any state. + Route::delete('/', [Admin\ServerController::class, 'destroy'])->withoutMiddleware( + ValidateServerStatusMiddleware::class, + ); Route::prefix('/settings')->group(function () { Route::patch('/build', [Admin\ServerController::class, 'updateBuild']); diff --git a/tests/Feature/Controllers/Admin/ServerDeletionTest.php b/tests/Feature/Controllers/Admin/ServerDeletionTest.php new file mode 100644 index 00000000000..eefb12a96e4 --- /dev/null +++ b/tests/Feature/Controllers/Admin/ServerDeletionTest.php @@ -0,0 +1,163 @@ + Http::response( + file_get_contents(base_path('tests/Fixtures/Repositories/Server/GetServerConfigData.json')), + 200, + ), + '*' => Http::response(['data' => 'dummy-upid'], 200), + ]); +} + +function fakeVmGone(): void +{ + Http::fake([ + '*/config' => Http::response([ + 'data' => null, + 'message' => "Configuration file 'nodes/proxmox/qemu-server/100.conf' does not exist\n", + ], 500), + '*' => Http::response(['data' => 'dummy-upid'], 200), + ]); +} + +function deleteAsAdmin(Server $server, array $data = []) +{ + return test()->actingAs(User::factory()->create(['root_admin' => true])) + ->deleteJson("/api/admin/servers/{$server->uuid}", $data); +} + +it('deletes the backups, then the VM, then the Convoy entry', function () { + fakeVmPresent(); + [$_, $_, $_, $server] = createServerModel(); + + deleteAsAdmin($server)->assertNoContent(); + + expect($server->fresh()->status)->toBe(Status::DELETING->value); + Queue::assertPushedWithChain(PurgeBackupsJob::class, [ + SendPowerCommandJob::class, + MonitorStateJob::class, + DeleteServerJob::class, + WaitUntilVmIsDeletedJob::class, + CallQueuedClosure::class, + ]); +}); + +it('only removes the Convoy entry when the VM is already gone from the node', function () { + fakeVmGone(); + [$_, $_, $_, $server] = createServerModel(); + + deleteAsAdmin($server)->assertNoContent(); + + Queue::assertPushedWithChain(PurgeBackupsJob::class, [CallQueuedClosure::class]); +}); + +it('leaves the server alone when the node cannot be asked', function () { + Http::fake(['*' => Http::response(['data' => null, 'message' => 'proxy error'], 503)]); + [$_, $_, $_, $server] = createServerModel(); + + deleteAsAdmin($server)->assertServerError(); + + // An outage must not read as "VM already gone" and drop the server. + expect($server->fresh())->not->toBeNull() + ->and($server->fresh()->status)->toBeNull(); + Queue::assertNothingPushed(); +}); + +it('can retry a delete that failed', function () { + fakeVmPresent(); + [$_, $_, $_, $server] = createServerModel(); + $server->update(['status' => Status::DELETION_FAILED->value]); + + deleteAsAdmin($server)->assertNoContent(); + + expect($server->fresh()->status)->toBe(Status::DELETING->value); +}); + +it("won't delete while another operation is running on the server", function (Status $status) { + fakeVmPresent(); + [$_, $_, $_, $server] = createServerModel(); + $server->update(['status' => $status->value]); + + deleteAsAdmin($server)->assertConflict(); + + Queue::assertNothingPushed(); +})->with([Status::INSTALLING, Status::DELETING, Status::RESTORING_BACKUP]); + +it('disconnects without touching the node, from any state', function (?Status $status) { + Http::fake(); + [$_, $_, $_, $server] = createServerModel(); + $server->update(['status' => $status?->value]); + + deleteAsAdmin($server, ['no_purge' => true])->assertNoContent(); + + expect(Server::find($server->id))->toBeNull(); + Http::assertNothingSent(); + Queue::assertNothingPushed(); +})->with([null, Status::DELETING, Status::DELETION_FAILED, Status::INSTALLING]); + +it('marks the server deletion_failed when the chain fails', function () { + fakeVmPresent(); + Queue::fake()->serializeAndRestore(); + [$_, $_, $_, $server] = createServerModel(); + + deleteAsAdmin($server)->assertNoContent(); + + Queue::pushed(PurgeBackupsJob::class)->first() + ->invokeChainCatchCallbacks(new RuntimeException('destroy failed')); + + expect($server->fresh()->status)->toBe(Status::DELETION_FAILED->value); +}); + +it('purges locked backups too when the server is deleted', function () { + Http::fake(['*' => Http::response(['data' => 'dummy-upid'], 200)]); + [$_, $_, $_, $server] = createServerModel(); + $backup = Backup::factory()->for($server)->create([ + 'is_locked' => true, + 'is_successful' => true, + 'completed_at' => now(), + ]); + + app(PurgeBackupsService::class)->handle($server, force: true); + + expect(Backup::find($backup->id))->toBeNull(); +}); + +it('sees a VM that answers as existing', function () { + fakeVmPresent(); + [$_, $_, $_, $server] = createServerModel(); + + expect(app(ServerBuildService::class)->vmExists($server))->toBeTrue(); +}); + +it('sees Proxmox "does not exist" as a missing VM', function () { + fakeVmGone(); + [$_, $_, $_, $server] = createServerModel(); + + expect(app(ServerBuildService::class)->vmExists($server))->toBeFalse(); +}); + +it('throws on any other Proxmox failure rather than guess', function () { + Http::fake(['*' => Http::response(['data' => null, 'message' => 'proxy error'], 503)]); + [$_, $_, $_, $server] = createServerModel(); + + expect(fn () => app(ServerBuildService::class)->vmExists($server)) + ->toThrow(ProxmoxConnectionException::class); +}); From 7b1405f61108651f3d3511abbd6b330584bc4ae3 Mon Sep 17 00:00:00 2001 From: Eric Wang <37554696+ericwang401@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:50:17 +0000 Subject: [PATCH 3/3] fix(servers): read Proxmox's response wherever the exception keeps it isVmMissingError() read the response off the RequestException that ProxmoxConnectionException wrapped. #176 stops wrapping it and keeps the response on the exception instead, so read $e->response first and fall back to the wrapped one. The "does not exist" check then works whichever of the two PRs merges first; #172's tests pass against both versions of the exception. --- app/Services/Servers/ServerBuildService.php | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/app/Services/Servers/ServerBuildService.php b/app/Services/Servers/ServerBuildService.php index c574e672d43..134efb71e69 100644 --- a/app/Services/Servers/ServerBuildService.php +++ b/app/Services/Servers/ServerBuildService.php @@ -8,6 +8,7 @@ use Convoy\Repositories\Proxmox\Server\ProxmoxServerRepository; use Convoy\Exceptions\Repository\Proxmox\ProxmoxConnectionException; use Illuminate\Http\Client\RequestException; +use Illuminate\Http\Client\Response; /** * Class SnapshotService @@ -89,10 +90,13 @@ public function vmExists(Server $server): bool */ public static function isVmMissingError(\Throwable $e): bool { - $request = $e->getPrevious(); - - return $request instanceof RequestException - && $request->response->status() === 500 - && str_contains((string) $request->response->json('message'), 'does not exist'); + // 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'); } }