From db416a3f03641df6a4681272ac0ce7ee4c70e9be Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 21:19:05 -0500 Subject: [PATCH 1/2] fix: retry the qemu-server flock instead of predicting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WaitForStatusTransition refused to return while snapshot.Locked, and its comment quoted the exact error it was meant to prevent. Locked reads the guest config's lock: property; the failure is the flock on /var/lock/qemu-server/lock-.conf, which PVE exposes nowhere. The flock cannot be observed, so it is retried. GuestLockRetry reissues an operation for a bounded 45s while PVE reports failing to enter lock_config for a guest, which it raises before doing any work. Two seams, because the failure has two surfaces. PveHttpClient.SendAsync retries the request for operations PVE serialises in the API handler; it takes a request factory because an HttpRequestMessage cannot be resent. PveCmdletBase.InvokeGuestTask reissues the call and re-waits its task for operations serialised in the forked worker, where the POST returns 200 and only the task fails. WaitForStatusTransition routes through the latter, hence Func. The predicate is path-specific and anchored at the start of what PVE said: lock_file uses identical wording for storage, LVM and HA locks, and a qmclone that fails after allocating disks must not be reissued into "VM already exists". That requires the raw text, so it reads PveTaskFailedException.ExitStatus and PveApiException.ApiMessage. The Locked check stays — it is correct for the config lock — with a comment that says so. Closes #113 --- CHANGELOG.md | 3 +- DECISIONS.md | 123 ++++++++++++-- src/PSProxmoxVE.Core/Client/PveHttpClient.cs | 54 ++++-- .../Exceptions/PveApiException.cs | 9 + .../Utilities/GuestLockRetry.cs | 103 +++++++++++ .../Utilities/GuestStatusSnapshot.cs | 5 +- .../Containers/RestartPveContainerCmdlet.cs | 16 +- .../Containers/StartPveContainerCmdlet.cs | 9 +- .../Containers/StopPveContainerCmdlet.cs | 9 +- src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs | 56 ++++-- .../Cmdlets/Vms/CopyPveVmCmdlet.cs | 10 +- .../Cmdlets/Vms/ResetPveVmCmdlet.cs | 9 +- .../Cmdlets/Vms/ResizePveVmDiskCmdlet.cs | 10 +- .../Cmdlets/Vms/RestartPveVmCmdlet.cs | 7 +- .../Cmdlets/Vms/ResumePveVmCmdlet.cs | 9 +- .../Cmdlets/Vms/StartPveVmCmdlet.cs | 9 +- .../Cmdlets/Vms/StopPveVmCmdlet.cs | 9 +- .../Cmdlets/Vms/SuspendPveVmCmdlet.cs | 9 +- .../Client/PveHttpClientLockRetryTests.cs | 143 ++++++++++++++++ .../Utilities/GuestLockRetryTests.cs | 160 ++++++++++++++++++ 20 files changed, 653 insertions(+), 109 deletions(-) create mode 100644 src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs create mode 100644 tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs create mode 100644 tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs 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); + } + } +} From 1e94d4188c4ffcbea7a3d3dff1fd0fceb7abb4c2 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 21:49:00 -0500 Subject: [PATCH 2/2] fix: report each flock reissue, and scale the gap to the retry budget Two non-blocking review observations. A 45s retry is indistinguishable from a hang with nothing on the wire, so GuestLockRetry.Execute takes an onRetry hook and InvokeGuestTask reports each reissue through WriteVerbose. The gap between attempts now scales with the budget, capped at the 2s production value. A caller passing a short window wants a fast answer rather than one long sleep, which also takes the retrying unit tests off a real 2s sleep each: the xUnit run drops from 8s to 4s. PveHttpClient's window becomes a field so those tests can shorten it too. --- src/PSProxmoxVE.Core/Client/PveHttpClient.cs | 5 +++- .../Utilities/GuestLockRetry.cs | 21 ++++++++++++--- src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs | 16 +++++++----- .../Client/PveHttpClientLockRetryTests.cs | 10 +++++++ .../Utilities/GuestLockRetryTests.cs | 26 +++++++++++++++++-- 5 files changed, 64 insertions(+), 14 deletions(-) diff --git a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs index 337e43f..e5fdcf6 100644 --- a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs +++ b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs @@ -30,6 +30,8 @@ namespace PSProxmoxVE.Core.Client private readonly HttpClient _httpClient; private bool _disposed; + private TimeSpan _guestLockRetryWindow = GuestLockRetry.DefaultWindow; + private const string ApiTokenPrefix = "PVEAPIToken="; private const string AuthCookieName = "PVEAuthCookie="; private const string CsrfHeaderName = "CSRFPreventionToken"; @@ -382,7 +384,8 @@ namespace PSProxmoxVE.Core.Client /// 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)); + GuestLockRetry.ExecuteAsync( + () => SendOnceAsync(buildRequest(), resource, httpMethod), _guestLockRetryWindow); private async Task SendOnceAsync(HttpRequestMessage request, string resource, string httpMethod) { diff --git a/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs b/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs index e10fc9a..39b8251 100644 --- a/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs +++ b/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs @@ -24,7 +24,15 @@ namespace PSProxmoxVE.Core.Utilities /// public static readonly TimeSpan DefaultWindow = TimeSpan.FromSeconds(45); - private static readonly TimeSpan RetryInterval = TimeSpan.FromSeconds(2); + private static readonly TimeSpan MaxRetryInterval = TimeSpan.FromSeconds(2); + + // The gap must never eat a meaningful share of a short budget: a caller passing a + // small window wants a fast answer, not one long sleep. + private static TimeSpan RetryInterval(TimeSpan budget) + { + var quarter = TimeSpan.FromMilliseconds(budget.TotalMilliseconds / 4); + return quarter < MaxRetryInterval ? quarter : MaxRetryInterval; + } // 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 @@ -59,7 +67,11 @@ namespace PSProxmoxVE.Core.Utilities /// /// The operation to run. /// Retry budget. Defaults to . - public static T Execute(Func operation, TimeSpan? window = null) + /// + /// Invoked with the rejection before each reissue. A caller with somewhere to report + /// progress should pass one — a wait this long is otherwise indistinguishable from a hang. + /// + public static T Execute(Func operation, TimeSpan? window = null, Action? onRetry = null) { if (operation == null) throw new ArgumentNullException(nameof(operation)); @@ -73,7 +85,8 @@ namespace PSProxmoxVE.Core.Utilities } catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget) { - Thread.Sleep(RetryInterval); + onRetry?.Invoke(ex); + Thread.Sleep(RetryInterval(budget)); } } } @@ -95,7 +108,7 @@ namespace PSProxmoxVE.Core.Utilities } catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget) { - await Task.Delay(RetryInterval).ConfigureAwait(false); + await Task.Delay(RetryInterval(budget)).ConfigureAwait(false); } } } diff --git a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs index fb4550f..0a32a99 100644 --- a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs +++ b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs @@ -180,13 +180,15 @@ namespace PSProxmoxVE.Cmdlets 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); - }); + return GuestLockRetry.Execute( + () => + { + var task = issueOperation(); + return string.IsNullOrEmpty(task.Upid) + ? task + : taskService.WaitForTask(session, node, task.Upid, null, null, null); + }, + onRetry: ex => WriteVerbose($"Guest is locked, retrying: {ex.Message}")); } /// diff --git a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs index 3eeb091..1ec8e7f 100644 --- a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs @@ -25,6 +25,15 @@ namespace PSProxmoxVE.Core.Tests.Client field.SetValue(client, newInner); } + // The gap between attempts scales with the budget, so a short window keeps these + // tests off the 2s production sleep. + private static void SetRetryWindow(PveHttpClient client, TimeSpan window) + { + var field = typeof(PveHttpClient).GetField("_guestLockRetryWindow", + BindingFlags.Instance | BindingFlags.NonPublic)!; + field.SetValue(client, window); + } + private static (PveHttpClient client, ScriptedHandler handler) NewClient( params (HttpStatusCode status, string body)[] responses) { @@ -33,6 +42,7 @@ namespace PSProxmoxVE.Core.Tests.Client var client = new PveHttpClient(session); var handler = new ScriptedHandler(responses); SetInnerHttpClient(client, new HttpClient(handler)); + SetRetryWindow(client, TimeSpan.FromMilliseconds(400)); return (client, handler); } diff --git a/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs b/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs index f1cd612..61e77c9 100644 --- a/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Net; using System.Threading.Tasks; using PSProxmoxVE.Core.Exceptions; @@ -15,6 +16,10 @@ namespace PSProxmoxVE.Core.Tests.Utilities private const string LxcLockError = "can't lock file '/run/lock/lxc/pve-config-100.lock' - got timeout"; + // The gap between attempts scales with the budget, so a short window keeps the + // retrying tests off a 2s production sleep. + private static readonly TimeSpan ShortWindow = TimeSpan.FromMilliseconds(400); + private static PveApiException ApiError(string message) => new PveApiException(HttpStatusCode.InternalServerError, message, "nodes/pve9a/qemu/100/config", "PUT"); @@ -73,6 +78,23 @@ namespace PSProxmoxVE.Core.Tests.Utilities Assert.False(GuestLockRetry.IsLockTimeout(new InvalidOperationException(VmLockError))); } + [Fact] + public void Execute_ReportsEachReissueToTheOnRetryHook() + { + var attempts = 0; + var reported = new List(); + + GuestLockRetry.Execute(() => + { + attempts++; + if (attempts < 3) throw TaskError(VmLockError); + return 0; + }, ShortWindow, onRetry: reported.Add); + + Assert.Equal(2, reported.Count); + Assert.All(reported, e => Assert.IsType(e)); + } + [Fact] public void Execute_ReturnsWithoutRetryingWhenTheOperationSucceeds() { @@ -94,7 +116,7 @@ namespace PSProxmoxVE.Core.Tests.Utilities attempts++; if (attempts < 2) throw TaskError(VmLockError); return "cloned"; - }); + }, ShortWindow); Assert.Equal("cloned", result); Assert.Equal(2, attempts); @@ -124,7 +146,7 @@ namespace PSProxmoxVE.Core.Tests.Utilities attempts++; if (attempts < 2) throw ApiError(VmLockError); return Task.FromResult("written"); - }); + }, ShortWindow); Assert.Equal("written", result); Assert.Equal(2, attempts);