Repository navigation
feat(servers): split Delete and Disconnect, and make Delete reliable - #172
Merged
Merged
Conversation
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.
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.
Contributor License AgreementAll identified human contributors have signed the current CLA. |
This was referenced Oct 16, 2026
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.
# Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
deletion_failed. Delete is refused (409) only while another operation is running on the server or a backup is in progress.DELETEwithno_purgestill disconnects.ServerBuildService::vmExists(). Before, any Proxmox error counted, so a node that was briefly unreachable could get a server removed from Convoy while its VM kept running.Testing
deletion_failedvmExists()Notes
CHANGELOG.mdwill conflict with feat(servers): retry a failed install #171 and fix(jobs): expire overlap locks, and back off between retries #173, since each adds its own "Unreleased" section.Unrelated and already on
4.x:AllocationServiceTestfails at random, roughly 1 run in 3.Works with fix: log a Proxmox error once, with one stack trace #176, which keeps the Proxmox response on the exception instead of wrapping the original.
isVmMissingError()reads either, so the two PRs can merge in any order.