diff --git a/CHANGELOG.md b/CHANGELOG.md index a251d46..abe9a77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,8 @@ and this project adheres to [Conventional Commits](https://www.conventionalcommi ### Fixed - `Restart-PveVm` now uses PVE's native reboot endpoint (`POST {vmid}/status/reboot`) instead of composing a shutdown followed by a start. The two-call form raced Proxmox's own post-stop cleanup: the start won the guest's config lock, `qm cleanup` then held that lock for 30 seconds waiting on the newly started process, and the caller's next operation failed with `can't lock file '/var/lock/qemu-server/lock-.conf' - got timeout`. Reproduced in integration runs 183, 185 and 186 as a cascade of 4 failures. `Restart-PveContainer` is unchanged — LXC has no reboot endpoint. See `DECISIONS.md` D016. -- Lifecycle cmdlets with `-Wait` (`Start`/`Stop`/`Restart`/`Reset`/`Resume` for VMs and containers) also wait for the guest's config lock (the `lock:` property, e.g. `backup` or `migrate`) to clear before returning, and the post-timeout fallback tests the most recent poll rather than whether a match was ever seen. See `DECISIONS.md` D015 — note that entry misdiagnosed the failure above and is superseded by D016. +- Guest operations that Proxmox rejects with `can't lock file '/var/lock/qemu-server/lock-.conf' - got timeout` are now reissued for up to 45 seconds instead of surfacing as an error. That flock is taken by `qm cleanup` for up to 30 seconds after a guest stops and is not exposed through the API in any form, so it can only be retried past, never waited on. Covers both the synchronous form (`Set-PveVmConfig`, `Resize-PveVmDisk`, and every other call through the HTTP client) and the asynchronous form, where the request succeeds and the PVE task then fails (`Reset-PveVm`, `Copy-PveVm`). Reproduced on a client ~40% slower than CI, which failed three VM tests on a commit CI passed. (#113) See `DECISIONS.md` D020. +- Lifecycle cmdlets with `-Wait` (`Start`/`Stop`/`Restart`/`Reset`/`Resume` for VMs and containers) also wait for the guest's config lock (the `lock:` property, e.g. `backup` or `migrate`) to clear before returning, and the post-timeout fallback tests the most recent poll rather than whether a match was ever seen. See `DECISIONS.md` D015 — that guard covers the config lock only; the separate flock race is D020. - `New-PveCluster -Wait` now blocks until the cluster reports quorum, not merely until the creation task finishes. PVE's create task returns before corosync converges (~6s earlier in testing), so the natural `New-PveCluster -Wait` → `Add-PveClusterMember` sequence failed with `cluster not ready - no quorum?`. Adds `-Timeout` (seconds, default 60, range 1-3600) following the `-Wait` timeout convention used by `Stop-PveContainer` and `Reset-PveVm`. See `DECISIONS.md` D014. ## [0.2.0] - 2026-05-22 diff --git a/DECISIONS.md b/DECISIONS.md index 567b558..820a032 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -466,19 +466,20 @@ Add-PveClusterMember ... ## D015 — Lifecycle -Wait blocks until the guest config lock clears -**Status**: Superseded by D016 (2026-09-01) — the mechanism below is wrong -**Finding refs**: (none — found via integration runs 183/184, 2026-09-01) +**Status**: Active, rescoped 2026-09-01 — it guards the config lock only, never the flock +**Finding refs**: (none — found via integration runs 183/184, 2026-09-01; rescoped for #113) **Resolved in scan**: n/a -> **This entry misdiagnosed the failure it was written for.** Two different things in PVE are +> **This entry was written for a failure it does not prevent.** Two different things in PVE are > called "lock": the **config lock** (the `lock:` property — `migrate`, `backup`, `clone`, > `snapshot` — a persisted config field, exposed as `lock` in `status/current`) and the > **flock** on `/var/lock/qemu-server/lock-.conf` taken by `PVE::QemuConfig->lock_config`, > which is not exposed through the API at all. The integration failures were the flock; this > entry guards the config lock, which an ordinary start/stop never sets. Run 186 confirmed the -> check never fired — `Restart-PveVm` took 4.11 s, unchanged. The real cause and fix are in -> **D016**. The waiting behaviour described below is harmless and still applies when a genuine -> config lock is present, so the code stays. +> check never fired — `Restart-PveVm` took 4.11 s, unchanged. The flock is handled by **D016** +> (serialise server-side where an endpoint exists) and **D020** (retry where none does). The +> guard below is correct for the config lock and stays, but must never be described as covering +> the flock. ### Decision `WaitForStatusTransition` returns only when the guest reports the expected status **and** @@ -579,16 +580,19 @@ not exist, so `Restart-PveContainer` necessarily keeps shutdown + start. ### Anti-pattern (do not reintroduce) ```csharp // NEVER compose a VM restart from two client calls — it races qmeventd's cleanup -var shutdownTask = vmService.ShutdownVm(session, node, vmid, timeout); -WaitForStatusTransition(session, node, shutdownTask, vmid, "stopped", timeout); -var startTask = vmService.StartVm(session, node, vmid); +WaitForStatusTransition(session, node, () => vmService.ShutdownVm(session, node, vmid, timeout), + vmid, "stopped", timeout); +WaitForStatusTransition(session, node, () => vmService.StartVm(session, node, vmid), + vmid, "running", timeout); ``` ### Correct pattern ```csharp -var task = vmService.RebootVm(session, node, vmid, timeout); -if (Wait.IsPresent) - task = WaitForStatusTransition(session, node, task, vmid, "running", timeout); +PveTask Issue() => vmService.RebootVm(session, node, vmid, timeout); + +var task = Wait.IsPresent + ? WaitForStatusTransition(session, node, Issue, vmid, "running", timeout) + : Issue(); ``` --- @@ -778,3 +782,98 @@ removes real friction. This one removed none. A `dev.ps1`, `Makefile` target, or shell function that re-implements provisioning steps, module installation, or test invocation. If a local flow is awkward, fix it in `run-integration.sh` so CI gets the fix too. + +--- + +## D020 — The qemu-server flock is retried, never predicted + +**Status**: Active +**Finding refs**: (none — issue #113, reproduced 2026-09-01 on a Rosetta-emulated client) +**Resolved in scan**: n/a + +### Decision +An operation PVE rejects with `can't lock file '' - got timeout` is reissued +for a bounded window (`GuestLockRetry.DefaultWindow`, 45 s). Nothing in the module may attempt +to *detect* that the flock is held before acting. + +Two seams implement it, and both are required: + +- **`PveHttpClient.SendAsync`** — retries the request itself. This covers every operation PVE + serialises inside the API handler, where the failure arrives as a 500: `Set-PveVmConfig`, + `Resize-PveVmDisk`'s config writes, and every future call that goes through the client. + The private send takes a `Func` rather than a request because an + `HttpRequestMessage` cannot be sent twice. +- **`PveCmdletBase.InvokeGuestTask`** — reissues the API call *and* re-waits its task. PVE takes + the flock inside the forked worker for most guest operations (`qmreset`, `qmclone`), so the + POST returns 200 with a UPID and the failure only appears in the task's exit status. The HTTP + layer cannot see it and cannot retry it. `WaitForStatusTransition` routes through this helper, + which is why it takes a `Func` instead of an already-issued `PveTask`. + +Reissuing is safe **only** for a failure to *enter* `lock_config`, which PVE raises before the +operation does any work. `GuestLockRetry.IsLockTimeout` must keep both properties that establish +this, and no failure may be added to it without them: + +- **Path-specific.** `PVE::Tools::lock_file` emits the identical wording for storage, LVM, HA, + backup and firewall locks. Those are taken mid-worker and carry no such guarantee, so the match + names the two guest config paths (`/var/lock/qemu-server/lock-.conf`, + `/run/lock/lxc/pve-config-.lock`) rather than the generic phrasing. +- **Anchored at the start of what PVE said.** `qmclone` is the operation that makes this matter: + its worker creates and locks the target config, allocates disks, then re-locks. A timeout at one + of those later points reads the same as one at entry, and reissuing it would hit + `check_vmid_unused` — "VM already exists" — leaving an orphaned guest behind. PVE + prefixes the late form with its own context (`clone failed: ...`), so anchoring rejects it. + `Resize-PveVmDisk -Size '+1G'` is the case where getting this wrong is irreversible rather than + merely messy. + +The anchor only works against the raw text, so the predicate reads +`PveTaskFailedException.ExitStatus` and `PveApiException.ApiMessage` — never `Exception.Message`, +which both types prefix with their own context. `ApiMessage` exists for this. + +### Rationale +D015 tried to predict the lock and guarded the wrong one (see its note). D016 removed the race +for `Restart-PveVm` by handing the ordering to PVE, but that only works where a server-side +serialised endpoint exists. `Set-PveVmConfig`, `Resize-PveVmDisk` and clone have none, so for +them the choice is retry or nothing. + +The window is 45 s because `qm cleanup` holds the flock while polling `vm_running_locally` for +up to 30 s, and each rejected attempt first burns PVE's own 10 s `lock_config` timeout. It bounds +when a *new* attempt may start, not total wall clock: an attempt beginning just inside the window +still runs to its own conclusion, so the real ceiling is roughly one attempt longer. + +Two consequences are deliberate, and both are load-bearing enough to state rather than discover: + +- **`-Timeout` does not bound the retry.** It is documented as the budget for the status + transition, and `WaitForStatusTransition` starts counting it only after the operation's task + completes. Binding the retry to it would defeat the fix at exactly the values that need it — + `Reset-PveVm -Wait -Timeout 30` needed ~31 s of retrying in the run that verified this change. +- **The two seams nest.** A cmdlet operation rejected synchronously burns the HTTP layer's window + inside `InvokeGuestTask`'s. The overlap costs a longer wait before the same failure, never a + different outcome, so it is not worth threading a shared budget through both layers. + +CI never showed this. Runs 189–200 were green because the CI client is fast enough to win the +race. It reproduces on a client roughly 40% slower — the CI container image run under Docker +Desktop's Rosetta emulation on Apple Silicon — which failed `Should hard-reset a running VM`, +`Should clone a VM` and `Should resize a VM disk (Resize-PveVmDisk)` on the same commit CI +passed. **A green CI run is not evidence about this class of bug.** + +### Not yet adopted +`InvokeGuestTask` is the correct seam for every cmdlet that issues a guest operation and waits +on its task. `Remove-PveVm`, `Move-PveVm`, the snapshot and template cmdlets, and the container +equivalents still call `TaskService.WaitForTask` directly and remain exposed to the same race. +They adopt the helper as they are next touched. + +### Anti-pattern (do not reintroduce) +```csharp +// NEVER try to observe the flock — PVE does not expose it in status/current or anywhere else +if (!snapshot.Locked) + return task; // reads the config `lock:` property; says nothing about the flock +``` + +### Correct pattern +```csharp +PveTask Issue() => vmService.CloneVm(session, sourceNode, vmid, newid, name, targetNode, full); + +var task = Wait.IsPresent + ? InvokeGuestTask(session, sourceNode, Issue) + : Issue(); +``` diff --git a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs index 4da3040..337e43f 100644 --- a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs +++ b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs @@ -10,6 +10,7 @@ using System.Threading.Tasks; using Newtonsoft.Json.Linq; using PSProxmoxVE.Core.Authentication; using PSProxmoxVE.Core.Exceptions; +using PSProxmoxVE.Core.Utilities; using System.Net.Security; using System.Security.Cryptography.X509Certificates; @@ -62,7 +63,7 @@ namespace PSProxmoxVE.Core.Client /// /// Creates a bare HTTP client for pre-session use (e.g. initial authentication). - /// No auth headers are added to requests made with this constructor. + /// Requests it builds carry no authentication headers. /// internal PveHttpClient(string hostname, int port, bool skipCertificateCheck, TimeSpan? timeout = null) { @@ -95,8 +96,8 @@ namespace PSProxmoxVE.Core.Client /// Raw JSON response body public async Task GetAsync(string resource) { - var request = BuildRequest(HttpMethod.Get, resource); - return await SendAsync(request, resource, "GET").ConfigureAwait(false); + return await SendAsync(() => BuildRequest(HttpMethod.Get, resource), resource, "GET") + .ConfigureAwait(false); } /// Performs a POST request against the specified API resource path. @@ -105,10 +106,13 @@ namespace PSProxmoxVE.Core.Client /// Raw JSON response body public async Task PostAsync(string resource, Dictionary? data = null) { - var request = BuildRequest(HttpMethod.Post, resource, mutating: true); - if (data != null) - request.Content = BuildFormContent(data); - return await SendAsync(request, resource, "POST").ConfigureAwait(false); + return await SendAsync(() => + { + var request = BuildRequest(HttpMethod.Post, resource, mutating: true); + if (data != null) + request.Content = BuildFormContent(data); + return request; + }, resource, "POST").ConfigureAwait(false); } /// @@ -121,9 +125,12 @@ namespace PSProxmoxVE.Core.Client public async Task PostAsync(string resource, IEnumerable> data) { if (data == null) throw new ArgumentNullException(nameof(data)); - var request = BuildRequest(HttpMethod.Post, resource, mutating: true); - request.Content = BuildFormContent(data); - return await SendAsync(request, resource, "POST").ConfigureAwait(false); + return await SendAsync(() => + { + var request = BuildRequest(HttpMethod.Post, resource, mutating: true); + request.Content = BuildFormContent(data); + return request; + }, resource, "POST").ConfigureAwait(false); } /// Performs a PUT request against the specified API resource path. @@ -132,10 +139,13 @@ namespace PSProxmoxVE.Core.Client /// Raw JSON response body public async Task PutAsync(string resource, Dictionary? data = null) { - var request = BuildRequest(HttpMethod.Put, resource, mutating: true); - if (data != null) - request.Content = BuildFormContent(data); - return await SendAsync(request, resource, "PUT").ConfigureAwait(false); + return await SendAsync(() => + { + var request = BuildRequest(HttpMethod.Put, resource, mutating: true); + if (data != null) + request.Content = BuildFormContent(data); + return request; + }, resource, "PUT").ConfigureAwait(false); } /// Performs a DELETE request against the specified API resource path. @@ -143,8 +153,8 @@ namespace PSProxmoxVE.Core.Client /// Raw JSON response body public async Task DeleteAsync(string resource) { - var request = BuildRequest(HttpMethod.Delete, resource, mutating: true); - return await SendAsync(request, resource, "DELETE").ConfigureAwait(false); + return await SendAsync(() => BuildRequest(HttpMethod.Delete, resource, mutating: true), resource, "DELETE") + .ConfigureAwait(false); } // ------------------------------------------------------------------------- @@ -331,7 +341,7 @@ namespace PSProxmoxVE.Core.Client var request = BuildRequest(HttpMethod.Post, resource, mutating: true); request.Content = multipart; - return await SendAsync(request, resource, "POST").ConfigureAwait(false); + return await SendOnceAsync(request, resource, "POST").ConfigureAwait(false); } finally { @@ -366,7 +376,15 @@ namespace PSProxmoxVE.Core.Client return request; } - private async Task SendAsync(HttpRequestMessage request, string resource, string httpMethod) + /// + /// Sends a request, rebuilding it from for each attempt + /// while PVE rejects it for a guest's config flock. An + /// cannot be resent, which is why this takes a factory rather than a request. + /// + private Task SendAsync(Func buildRequest, string resource, string httpMethod) => + GuestLockRetry.ExecuteAsync(() => SendOnceAsync(buildRequest(), resource, httpMethod)); + + private async Task SendOnceAsync(HttpRequestMessage request, string resource, string httpMethod) { HttpResponseMessage response; try diff --git a/src/PSProxmoxVE.Core/Exceptions/PveApiException.cs b/src/PSProxmoxVE.Core/Exceptions/PveApiException.cs index deaf0bc..8ba58b2 100644 --- a/src/PSProxmoxVE.Core/Exceptions/PveApiException.cs +++ b/src/PSProxmoxVE.Core/Exceptions/PveApiException.cs @@ -15,6 +15,13 @@ namespace PSProxmoxVE.Core.Exceptions /// The HTTP method used for the request (GET, POST, PUT, DELETE). public string HttpMethod { get; } + /// + /// The error text PVE returned, without the status/resource prefix that + /// carries. Callers that match on what PVE said — + /// rather than on how this exception renders it — must read this. + /// + public string ApiMessage { get; } + /// Initializes a new instance for a failed PVE API request. /// The HTTP status code returned. /// The error message from the API. @@ -26,6 +33,7 @@ namespace PSProxmoxVE.Core.Exceptions StatusCode = statusCode; Resource = resource; HttpMethod = httpMethod; + ApiMessage = message; } /// Initializes a new instance for a failed PVE API request, with an inner exception. @@ -40,6 +48,7 @@ namespace PSProxmoxVE.Core.Exceptions StatusCode = statusCode; Resource = resource; HttpMethod = httpMethod; + ApiMessage = message; } } } diff --git a/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs b/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs new file mode 100644 index 0000000..e10fc9a --- /dev/null +++ b/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs @@ -0,0 +1,103 @@ +using System; +using System.Diagnostics; +using System.Text.RegularExpressions; +using System.Threading; +using System.Threading.Tasks; +using PSProxmoxVE.Core.Exceptions; + +namespace PSProxmoxVE.Core.Utilities +{ + /// + /// Retries an operation PVE rejected because it could not acquire a guest's config + /// flock (/var/lock/qemu-server/lock-<vmid>.conf for VMs, + /// /run/lock/lxc/pve-config-<vmid>.lock for containers). + /// + /// That flock is held by qm cleanup after a guest stops and is not exposed + /// through the API in any form, so a caller cannot wait for it — only retry past it. + /// + public static class GuestLockRetry + { + /// + /// How long and keep retrying. + /// qm cleanup polls vm_running_locally for up to 30s while holding the + /// flock, and each rejected attempt first burns PVE's own 10s lock_config timeout. + /// + public static readonly TimeSpan DefaultWindow = TimeSpan.FromSeconds(45); + + private static readonly TimeSpan RetryInterval = TimeSpan.FromSeconds(2); + + // Anchored, and specific to the two guest lock paths. `PVE::Tools::lock_file` emits this + // same wording for storage, LVM, HA and firewall locks, none of which carry the + // reissue-safety guarantee below. The anchor is what separates a failure to *enter* + // lock_config from one PVE prefixed with its own context ("clone failed: ..."), which + // means the worker had already done work. + private static readonly Regex GuestLockTimeout = new Regex( + @"^can't lock file '(?:/var/lock/qemu-server/lock-\d+\.conf|/run/lock/lxc/pve-config-\d+\.lock)' - got timeout", + RegexOptions.IgnoreCase | RegexOptions.CultureInvariant); + + /// + /// True when reports PVE failing to enter lock_config for a + /// guest. That is raised before the operation performs any work, so a call failing this + /// way is known not to have run and is safe to reissue. + /// + /// Matched against what PVE actually said — + /// and — never against the composed + /// , whose prefix would defeat the anchor. + /// + /// The exception to classify. + public static bool IsLockTimeout(Exception ex) => ex switch + { + PveTaskFailedException task => GuestLockTimeout.IsMatch((task.ExitStatus ?? string.Empty).Trim()), + PveApiException api => GuestLockTimeout.IsMatch((api.ApiMessage ?? string.Empty).Trim()), + _ => false, + }; + + /// + /// Runs , reissuing it while it fails on the guest config + /// flock and has not elapsed. Any other exception propagates + /// on the first attempt. + /// + /// The operation to run. + /// Retry budget. Defaults to . + public static T Execute(Func operation, TimeSpan? window = null) + { + if (operation == null) throw new ArgumentNullException(nameof(operation)); + + var budget = window ?? DefaultWindow; + var elapsed = Stopwatch.StartNew(); + while (true) + { + try + { + return operation(); + } + catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget) + { + Thread.Sleep(RetryInterval); + } + } + } + + /// Asynchronous counterpart of . + /// The operation to run. + /// Retry budget. Defaults to . + public static async Task ExecuteAsync(Func> operation, TimeSpan? window = null) + { + if (operation == null) throw new ArgumentNullException(nameof(operation)); + + var budget = window ?? DefaultWindow; + var elapsed = Stopwatch.StartNew(); + while (true) + { + try + { + return await operation().ConfigureAwait(false); + } + catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget) + { + await Task.Delay(RetryInterval).ConfigureAwait(false); + } + } + } + } +} diff --git a/src/PSProxmoxVE.Core/Utilities/GuestStatusSnapshot.cs b/src/PSProxmoxVE.Core/Utilities/GuestStatusSnapshot.cs index 457764a..757aca4 100644 --- a/src/PSProxmoxVE.Core/Utilities/GuestStatusSnapshot.cs +++ b/src/PSProxmoxVE.Core/Utilities/GuestStatusSnapshot.cs @@ -18,8 +18,9 @@ namespace PSProxmoxVE.Core.Utilities /// StatusMatched: the guest reports . qmpstatus is /// preferred over status when present, because PVE reports status=running with /// qmpstatus=paused for a suspended VM. - /// Locked: the guest config still carries a lock, so the next API call against it - /// would fail to acquire the lock file. + /// Locked: the guest config carries a `lock:` property (backup, clone, migrate, + /// snapshot). This is not the /var/lock/qemu-server flock, which PVE does not expose + /// through status/current or any other endpoint — see DECISIONS.md D015 and D020. /// public static (bool StatusMatched, bool Locked) Evaluate(string json, string expectedStatus) { diff --git a/src/PSProxmoxVE/Cmdlets/Containers/RestartPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/RestartPveContainerCmdlet.cs index efb3020..48a5040 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/RestartPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/RestartPveContainerCmdlet.cs @@ -56,17 +56,17 @@ namespace PSProxmoxVE.Cmdlets.Containers WriteVerbose($"Restarting container {VmId} on node '{Node}'..."); - // Graceful shutdown - var shutdownTask = containerService.ShutdownContainer(session, Node, VmId, Timeout); + PveTask Shutdown() => containerService.ShutdownContainer(session, Node, VmId, Timeout); + PveTask Start() => containerService.StartContainer(session, Node, VmId); if (Wait.IsPresent) - WaitForStatusTransition(session, Node, shutdownTask, VmId, "stopped", Timeout, isContainer: true); + WaitForStatusTransition(session, Node, Shutdown, VmId, "stopped", Timeout, isContainer: true); + else + Shutdown(); - // Start - var startTask = containerService.StartContainer(session, Node, VmId); - - if (Wait.IsPresent) - startTask = WaitForStatusTransition(session, Node, startTask, VmId, "running", Timeout, isContainer: true); + var startTask = Wait.IsPresent + ? WaitForStatusTransition(session, Node, Start, VmId, "running", Timeout, isContainer: true) + : Start(); WriteObject(startTask); } diff --git a/src/PSProxmoxVE/Cmdlets/Containers/StartPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/StartPveContainerCmdlet.cs index c9f79a7..86bdf1d 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/StartPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/StartPveContainerCmdlet.cs @@ -50,12 +50,11 @@ namespace PSProxmoxVE.Cmdlets.Containers var containerService = new ContainerService(); WriteVerbose($"Starting container {VmId} on node '{Node}'..."); - var task = containerService.StartContainer(session, Node, VmId); + PveTask Issue() => containerService.StartContainer(session, Node, VmId); - if (Wait.IsPresent) - { - task = WaitForStatusTransition(session, Node, task, VmId, "running", Timeout, isContainer: true); - } + var task = Wait.IsPresent + ? WaitForStatusTransition(session, Node, Issue, VmId, "running", Timeout, isContainer: true) + : Issue(); WriteObject(task); } diff --git a/src/PSProxmoxVE/Cmdlets/Containers/StopPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/StopPveContainerCmdlet.cs index bf67201..1729594 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/StopPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/StopPveContainerCmdlet.cs @@ -52,12 +52,11 @@ namespace PSProxmoxVE.Cmdlets.Containers var containerService = new ContainerService(); WriteVerbose($"Stopping container {VmId} on node '{Node}'..."); - var task = containerService.StopContainer(session, Node, VmId); + PveTask Issue() => containerService.StopContainer(session, Node, VmId); - if (Wait.IsPresent) - { - task = WaitForStatusTransition(session, Node, task, VmId, "stopped", Timeout, isContainer: true); - } + var task = Wait.IsPresent + ? WaitForStatusTransition(session, Node, Issue, VmId, "stopped", Timeout, isContainer: true) + : Issue(); WriteObject(task); } diff --git a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs index c07e6f9..fb4550f 100644 --- a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs +++ b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs @@ -99,7 +99,10 @@ namespace PSProxmoxVE.Cmdlets /// /// The authenticated PVE session. /// The cluster node name. - /// The task returned by the lifecycle API call. + /// + /// Issues the lifecycle API call. Invoked again on each retry, so it must be safe to + /// repeat — see . + /// /// The VM or container ID to poll. /// The expected status string (e.g. "running", "stopped", "paused"). /// Maximum seconds to wait for the status transition. Default 60. @@ -108,19 +111,13 @@ namespace PSProxmoxVE.Cmdlets protected PveTask WaitForStatusTransition( PveSession session, string node, - PveTask task, + Func issueOperation, int vmid, string expectedStatus, int timeoutSeconds = 60, bool isContainer = false) { - var taskService = new TaskService(); - - // First wait for the PVE task to complete - if (!string.IsNullOrEmpty(task.Upid)) - { - task = taskService.WaitForTask(session, node, task.Upid, null, null, null); - } + var task = InvokeGuestTask(session, node, issueOperation); // Then poll status/current until VM/container reaches the expected status. // We query the status/current endpoint directly instead of the list endpoint @@ -141,15 +138,11 @@ namespace PSProxmoxVE.Cmdlets var snapshot = GuestStatusSnapshot.Evaluate(json, expectedStatus); lastMatched = snapshot.StatusMatched; - if (snapshot.StatusMatched) - { - // PVE reports the target status before the operation releases the - // config lock. A caller that issues its next request inside that - // window gets "can't lock file '/var/lock/qemu-server/lock-.conf' - // - got timeout" from its own API call. - if (!snapshot.Locked) - return task; - } + // snapshot.Locked is the config `lock:` property (backup, clone, migrate, + // snapshot) — not the /var/lock/qemu-server flock, which PVE does not + // expose. The flock race is handled by retrying, not by waiting. + if (snapshot.StatusMatched && !snapshot.Locked) + return task; } catch (Exception ex) when (ex is not OutOfMemoryException and not StackOverflowException) { @@ -169,6 +162,33 @@ namespace PSProxmoxVE.Cmdlets TimeSpan.FromSeconds(timeoutSeconds)); } + /// + /// Issues a guest operation and waits for the task it returns, reissuing the pair while + /// PVE rejects it for the guest's config flock. + /// + /// PVE takes that flock inside the worker for most guest operations, so the failure + /// surfaces as a failed task rather than a failed request and cannot be retried at the + /// HTTP layer. lock_config raises it before doing any work, so a reissue repeats + /// nothing. + /// + /// The authenticated PVE session. + /// The node the task runs on. + /// Issues the API call; invoked again on each retry. + /// The completed task, or the issued task when the call returned no UPID. + protected PveTask InvokeGuestTask(PveSession session, string node, Func issueOperation) + { + if (issueOperation == null) throw new ArgumentNullException(nameof(issueOperation)); + + var taskService = new TaskService(); + return GuestLockRetry.Execute(() => + { + var task = issueOperation(); + return string.IsNullOrEmpty(task.Upid) + ? task + : taskService.WaitForTask(session, node, task.Upid, null, null, null); + }); + } + /// /// Extracts the node name from a UPID string (format: UPID:node:...). /// Falls back to if the UPID is empty or cannot be parsed. diff --git a/src/PSProxmoxVE/Cmdlets/Vms/CopyPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/CopyPveVmCmdlet.cs index 6b6237b..f25f679 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/CopyPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/CopyPveVmCmdlet.cs @@ -81,13 +81,11 @@ namespace PSProxmoxVE.Cmdlets.Vms WriteVerbose($"Cloning VM {VmId}..."); var newid = NewVmId ?? 0; - var task = vmService.CloneVm(session, SourceNode, VmId, newid, NewName, TargetNode, Full.IsPresent); + PveTask Issue() => vmService.CloneVm(session, SourceNode, VmId, newid, NewName, TargetNode, Full.IsPresent); - if (Wait.IsPresent) - { - var taskService = new TaskService(); - task = taskService.WaitForTask(session, SourceNode, task.Upid, null, null, null); - } + var task = Wait.IsPresent + ? InvokeGuestTask(session, SourceNode, Issue) + : Issue(); WriteObject(task); } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/ResetPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/ResetPveVmCmdlet.cs index 305e96d..e736006 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/ResetPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/ResetPveVmCmdlet.cs @@ -52,12 +52,11 @@ namespace PSProxmoxVE.Cmdlets.Vms var vmService = new VmService(); WriteVerbose($"Resetting VM {VmId} on node '{Node}'..."); - var task = vmService.ResetVm(session, Node, VmId); + PveTask Issue() => vmService.ResetVm(session, Node, VmId); - if (Wait.IsPresent) - { - task = WaitForStatusTransition(session, Node, task, VmId, "running", Timeout); - } + var task = Wait.IsPresent + ? WaitForStatusTransition(session, Node, Issue, VmId, "running", Timeout) + : Issue(); WriteObject(task); } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/ResizePveVmDiskCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/ResizePveVmDiskCmdlet.cs index 29dbf45..99e1b54 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/ResizePveVmDiskCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/ResizePveVmDiskCmdlet.cs @@ -65,13 +65,11 @@ namespace PSProxmoxVE.Cmdlets.Vms var vmService = new VmService(); WriteVerbose($"Resizing disk '{Disk}' on VM {VmId}..."); - var task = vmService.ResizeDisk(session, Node, VmId, Disk, Size); + PveTask Issue() => vmService.ResizeDisk(session, Node, VmId, Disk, Size); - if (Wait.IsPresent) - { - var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); - } + var task = Wait.IsPresent + ? InvokeGuestTask(session, Node, Issue) + : Issue(); WriteObject(task); } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/RestartPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/RestartPveVmCmdlet.cs index 8c1b049..2f19fcd 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/RestartPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/RestartPveVmCmdlet.cs @@ -57,10 +57,11 @@ namespace PSProxmoxVE.Cmdlets.Vms WriteVerbose($"Restarting VM {VmId} on node '{Node}'..."); - var task = vmService.RebootVm(session, Node, VmId, Timeout); + PveTask Issue() => vmService.RebootVm(session, Node, VmId, Timeout); - if (Wait.IsPresent) - task = WaitForStatusTransition(session, Node, task, VmId, "running", Timeout); + var task = Wait.IsPresent + ? WaitForStatusTransition(session, Node, Issue, VmId, "running", Timeout) + : Issue(); WriteObject(task); } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/ResumePveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/ResumePveVmCmdlet.cs index 9f5d4aa..60a63c9 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/ResumePveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/ResumePveVmCmdlet.cs @@ -50,12 +50,11 @@ namespace PSProxmoxVE.Cmdlets.Vms var vmService = new VmService(); WriteVerbose($"Resuming VM {VmId} on node '{Node}'..."); - var task = vmService.ResumeVm(session, Node, VmId); + PveTask Issue() => vmService.ResumeVm(session, Node, VmId); - if (Wait.IsPresent) - { - task = WaitForStatusTransition(session, Node, task, VmId, "running", Timeout); - } + var task = Wait.IsPresent + ? WaitForStatusTransition(session, Node, Issue, VmId, "running", Timeout) + : Issue(); WriteObject(task); } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/StartPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/StartPveVmCmdlet.cs index cd181a6..9a7b8e3 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/StartPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/StartPveVmCmdlet.cs @@ -50,12 +50,11 @@ namespace PSProxmoxVE.Cmdlets.Vms var vmService = new VmService(); WriteVerbose($"Starting VM {VmId} on node '{Node}'..."); - var task = vmService.StartVm(session, Node, VmId); + PveTask Issue() => vmService.StartVm(session, Node, VmId); - if (Wait.IsPresent) - { - task = WaitForStatusTransition(session, Node, task, VmId, "running", Timeout); - } + var task = Wait.IsPresent + ? WaitForStatusTransition(session, Node, Issue, VmId, "running", Timeout) + : Issue(); WriteObject(task); } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/StopPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/StopPveVmCmdlet.cs index 0b3dcb9..bf43145 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/StopPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/StopPveVmCmdlet.cs @@ -52,12 +52,11 @@ namespace PSProxmoxVE.Cmdlets.Vms var vmService = new VmService(); WriteVerbose($"Stopping VM {VmId} on node '{Node}'..."); - var task = vmService.StopVm(session, Node, VmId); + PveTask Issue() => vmService.StopVm(session, Node, VmId); - if (Wait.IsPresent) - { - task = WaitForStatusTransition(session, Node, task, VmId, "stopped", Timeout); - } + var task = Wait.IsPresent + ? WaitForStatusTransition(session, Node, Issue, VmId, "stopped", Timeout) + : Issue(); WriteObject(task); } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/SuspendPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/SuspendPveVmCmdlet.cs index 73a7c7c..522567c 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/SuspendPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/SuspendPveVmCmdlet.cs @@ -51,12 +51,11 @@ namespace PSProxmoxVE.Cmdlets.Vms var vmService = new VmService(); WriteVerbose($"Suspending VM {VmId} on node '{Node}'..."); - var task = vmService.SuspendVm(session, Node, VmId); + PveTask Issue() => vmService.SuspendVm(session, Node, VmId); - if (Wait.IsPresent) - { - task = WaitForStatusTransition(session, Node, task, VmId, "paused", Timeout); - } + var task = Wait.IsPresent + ? WaitForStatusTransition(session, Node, Issue, VmId, "paused", Timeout) + : Issue(); WriteObject(task); } diff --git a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs new file mode 100644 index 0000000..3eeb091 --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs @@ -0,0 +1,143 @@ +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Reflection; +using System.Threading; +using System.Threading.Tasks; +using PSProxmoxVE.Core.Authentication; +using PSProxmoxVE.Core.Client; +using PSProxmoxVE.Core.Exceptions; +using Xunit; + +namespace PSProxmoxVE.Core.Tests.Client +{ + public class PveHttpClientLockRetryTests + { + private const string LockTimeoutBody = + "{\"message\":\"can't lock file '/var/lock/qemu-server/lock-100.conf' - got timeout\"}"; + + private static void SetInnerHttpClient(PveHttpClient client, HttpClient newInner) + { + var field = typeof(PveHttpClient).GetField("_httpClient", + BindingFlags.Instance | BindingFlags.NonPublic)!; + ((HttpClient)field.GetValue(client)!).Dispose(); + field.SetValue(client, newInner); + } + + private static (PveHttpClient client, ScriptedHandler handler) NewClient( + params (HttpStatusCode status, string body)[] responses) + { + var session = new PveSession("pve.example.com", 8006, false, + "root@pam!token=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"); + var client = new PveHttpClient(session); + var handler = new ScriptedHandler(responses); + SetInnerHttpClient(client, new HttpClient(handler)); + return (client, handler); + } + + private static Dictionary ConfigBody() => + new Dictionary { ["scsi0"] = "local-lvm:1" }; + + [Fact] + public async Task PutAsync_ReissuesTheRequestWhilePveReportsTheGuestFlock() + { + var (client, handler) = NewClient( + (HttpStatusCode.InternalServerError, LockTimeoutBody), + (HttpStatusCode.InternalServerError, LockTimeoutBody), + (HttpStatusCode.OK, "{\"data\":null}")); + + using (client) + { + var result = await client.PutAsync("nodes/pve9a/qemu/100/config", ConfigBody()); + Assert.Equal("{\"data\":null}", result); + } + + Assert.Equal(3, handler.Bodies.Count); + } + + [Fact] + public async Task PutAsync_RebuildsTheRequestSoEveryAttemptCarriesTheSameBody() + { + var (client, handler) = NewClient( + (HttpStatusCode.InternalServerError, LockTimeoutBody), + (HttpStatusCode.OK, "{\"data\":null}")); + + using (client) + { + await client.PutAsync("nodes/pve9a/qemu/100/config", ConfigBody()); + } + + Assert.Equal(2, handler.Bodies.Count); + Assert.Equal("scsi0=local-lvm:1", handler.Bodies[0]); + Assert.Equal(handler.Bodies[0], handler.Bodies[1]); + Assert.All(handler.Methods, m => Assert.Equal(HttpMethod.Put, m)); + Assert.Equal(handler.Uris[0], handler.Uris[1]); + Assert.EndsWith("nodes/pve9a/qemu/100/config", handler.Uris[0]); + } + + [Fact] + public async Task PostAsync_DoesNotReissueApiErrorsThatAreNotTheFlock() + { + var (client, handler) = NewClient( + (HttpStatusCode.InternalServerError, "{\"message\":\"VM 100 not running\"}"), + (HttpStatusCode.OK, "{\"data\":null}")); + + using (client) + { + var ex = await Assert.ThrowsAsync( + () => client.PostAsync("nodes/pve9a/qemu/100/status/reset")); + Assert.Contains("VM 100 not running", ex.Message); + } + + Assert.Single(handler.Bodies); + } + + [Fact] + public async Task GetAsync_ReissuesWithoutCarryingContent() + { + var (client, handler) = NewClient( + (HttpStatusCode.InternalServerError, LockTimeoutBody), + (HttpStatusCode.OK, "{\"data\":{}}")); + + using (client) + { + await client.GetAsync("nodes/pve9a/qemu/100/status/current"); + } + + Assert.Equal(2, handler.Bodies.Count); + Assert.All(handler.Bodies, b => Assert.Equal(string.Empty, b)); + } + + private sealed class ScriptedHandler : HttpMessageHandler + { + private readonly (HttpStatusCode status, string body)[] _responses; + private int _index; + + public List Bodies { get; } = new List(); + public List Methods { get; } = new List(); + public List Uris { get; } = new List(); + + public ScriptedHandler((HttpStatusCode status, string body)[] responses) + { + _responses = responses; + } + + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + Bodies.Add(request.Content == null + ? string.Empty + : await request.Content.ReadAsStringAsync().ConfigureAwait(false)); + Methods.Add(request.Method); + Uris.Add(request.RequestUri!.ToString()); + + if (_index >= _responses.Length) + throw new InvalidOperationException("ScriptedHandler ran out of responses."); + + var (status, body) = _responses[_index++]; + return new HttpResponseMessage(status) { Content = new StringContent(body) }; + } + } + } +} diff --git a/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs b/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs new file mode 100644 index 0000000..f1cd612 --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs @@ -0,0 +1,160 @@ +using System; +using System.Net; +using System.Threading.Tasks; +using PSProxmoxVE.Core.Exceptions; +using PSProxmoxVE.Core.Utilities; +using Xunit; + +namespace PSProxmoxVE.Core.Tests.Utilities +{ + public class GuestLockRetryTests + { + private const string VmLockError = + "can't lock file '/var/lock/qemu-server/lock-100.conf' - got timeout"; + + private const string LxcLockError = + "can't lock file '/run/lock/lxc/pve-config-100.lock' - got timeout"; + + private static PveApiException ApiError(string message) => + new PveApiException(HttpStatusCode.InternalServerError, message, "nodes/pve9a/qemu/100/config", "PUT"); + + private static PveTaskFailedException TaskError(string exitStatus) => + new PveTaskFailedException("UPID:pve9a:00000001:qmreset:100:root@pam:", exitStatus); + + [Fact] + public void IsLockTimeout_MatchesTheVmFlockErrorFromBothSurfaces() + { + Assert.True(GuestLockRetry.IsLockTimeout(ApiError(VmLockError))); + Assert.True(GuestLockRetry.IsLockTimeout(TaskError(VmLockError))); + } + + [Fact] + public void IsLockTimeout_MatchesTheContainerFlockError() + { + Assert.True(GuestLockRetry.IsLockTimeout(TaskError(LxcLockError))); + } + + [Theory] + [InlineData("VM 100 not running")] + [InlineData("can't lock file '/var/lock/qemu-server/lock-100.conf'")] + [InlineData("got timeout")] + // PVE::Tools::lock_file uses this same wording for locks that carry no + // reissue-safety guarantee. Only the two guest config paths may retry. + [InlineData("can't lock file '/run/lock/pve-manager/pve-storage-local' - got timeout")] + [InlineData("can't lock file '/var/lock/pve-manager/pve-backup' - got timeout")] + [InlineData("can't lock file '/run/lock/lvm/V_pve' - got timeout")] + // A message PVE prefixed with its own context means the worker had already + // started; reissuing it could repeat work that landed. + [InlineData("clone failed: can't lock file '/var/lock/qemu-server/lock-100.conf' - got timeout")] + [InlineData("unable to resize: can't lock file '/var/lock/qemu-server/lock-100.conf' - got timeout")] + public void IsLockTimeout_RejectsEveryOtherFailure(string message) + { + Assert.False(GuestLockRetry.IsLockTimeout(ApiError(message))); + Assert.False(GuestLockRetry.IsLockTimeout(TaskError(message))); + } + + [Fact] + public void IsLockTimeout_ReadsWhatPveSaidRatherThanTheComposedMessage() + { + // Both exception types prefix Message with their own context, which would + // defeat the anchor if the predicate matched on Message. + var api = ApiError(VmLockError); + var task = TaskError(VmLockError); + + Assert.StartsWith("PVE API error", api.Message); + Assert.StartsWith("Task UPID:", task.Message); + Assert.True(GuestLockRetry.IsLockTimeout(api)); + Assert.True(GuestLockRetry.IsLockTimeout(task)); + } + + [Fact] + public void IsLockTimeout_RejectsExceptionTypesThatAreNotApiFailures() + { + Assert.False(GuestLockRetry.IsLockTimeout(new InvalidOperationException(VmLockError))); + } + + [Fact] + public void Execute_ReturnsWithoutRetryingWhenTheOperationSucceeds() + { + var attempts = 0; + + var result = GuestLockRetry.Execute(() => { attempts++; return 42; }); + + Assert.Equal(42, result); + Assert.Equal(1, attempts); + } + + [Fact] + public void Execute_ReissuesUntilTheLockClears() + { + var attempts = 0; + + var result = GuestLockRetry.Execute(() => + { + attempts++; + if (attempts < 2) throw TaskError(VmLockError); + return "cloned"; + }); + + Assert.Equal("cloned", result); + Assert.Equal(2, attempts); + } + + [Fact] + public void Execute_DoesNotRetryOtherFailures() + { + var attempts = 0; + + Assert.Throws(() => GuestLockRetry.Execute(() => + { + attempts++; + throw ApiError("VM 100 not running"); + })); + + Assert.Equal(1, attempts); + } + + [Fact] + public async Task ExecuteAsync_ReissuesUntilTheLockClears() + { + var attempts = 0; + + var result = await GuestLockRetry.ExecuteAsync(() => + { + attempts++; + if (attempts < 2) throw ApiError(VmLockError); + return Task.FromResult("written"); + }); + + Assert.Equal("written", result); + Assert.Equal(2, attempts); + } + + [Fact] + public async Task ExecuteAsync_DoesNotRetryOtherFailures() + { + var attempts = 0; + + await Assert.ThrowsAsync(() => GuestLockRetry.ExecuteAsync(() => + { + attempts++; + throw ApiError("can't lock file '/run/lock/lvm/V_pve' - got timeout"); + })); + + Assert.Equal(1, attempts); + } + + [Fact] + public void Execute_GivesUpAndRethrowsOnceTheWindowElapses() + { + var attempts = 0; + + var ex = Assert.Throws(() => GuestLockRetry.Execute( + () => { attempts++; throw TaskError(VmLockError); }, + TimeSpan.Zero)); + + Assert.Contains("got timeout", ex.Message); + Assert.Equal(1, attempts); + } + } +}