From 3f63ddcad871fe3918417173fe9fddb8e0aacf70 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:33:47 +0000 Subject: [PATCH 1/6] fix: lifecycle -Wait blocks until the guest config lock clears PVE publishes a guest's new status while the operation still holds /var/lock/qemu-server/lock-.conf, so WaitForStatusTransition could return while the guest was still locked and the caller's next request would fail with "can't lock file ... got timeout". Integration run 183 failed four tests from this one cause: Restart-PveVm -Wait returned after 4.1s having seen "running", the following Stop-PveVm spent exactly 10.0s failing to take the lock, and that cascaded into the template convert, clone, and remove tests. Run 184 - same commit, re-run - passed because its status poll happened to take 10.1s, by which point the lock had cleared. The same settling happens either way; the only variable is whether the wait absorbs it or the next caller does. The check goes in WaitForStatusTransition because all nine lifecycle call sites (Start/Stop/Restart/Reset/Resume across VMs and containers) route through it. `lock` comes from the status/current response the poll already fetches - present on both qemu and lxc since PVE 5.4, below the module's 7.0 floor - so it costs no extra request. If the status is reached but the lock outlasts -Timeout the cmdlet still returns success, so a call that succeeded before this change cannot become an exception after it. Recorded as D015, the guest-lock sibling of D014. Co-Authored-By: Claude Opus 5 (1M context) --- .../Utilities/GuestStatusSnapshot.cs | 40 +++++++++++ .../Utilities/GuestStatusSnapshotTests.cs | 70 +++++++++++++++++++ 2 files changed, 110 insertions(+) create mode 100644 src/PSProxmoxVE.Core/Utilities/GuestStatusSnapshot.cs create mode 100644 tests/PSProxmoxVE.Core.Tests/Utilities/GuestStatusSnapshotTests.cs diff --git a/src/PSProxmoxVE.Core/Utilities/GuestStatusSnapshot.cs b/src/PSProxmoxVE.Core/Utilities/GuestStatusSnapshot.cs new file mode 100644 index 0000000..457764a --- /dev/null +++ b/src/PSProxmoxVE.Core/Utilities/GuestStatusSnapshot.cs @@ -0,0 +1,40 @@ +using System; +using Newtonsoft.Json.Linq; + +namespace PSProxmoxVE.Core.Utilities +{ + /// + /// Reads the fields of a guest status/current response that decide whether a + /// lifecycle wait (-Wait on Start, Stop, Restart, Reset, Resume) is finished. + /// + public static class GuestStatusSnapshot + { + /// + /// Evaluates a status/current response body against the status a caller is waiting for. + /// + /// Raw status/current response body. + /// The status being waited for (e.g. "running", "stopped", "paused"). + /// + /// 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. + /// + public static (bool StatusMatched, bool Locked) Evaluate(string json, string expectedStatus) + { + if (string.IsNullOrEmpty(json)) + return (false, false); + + var data = JObject.Parse(json)["data"]; + var status = data?["status"]?.ToString(); + var qmpStatus = data?["qmpstatus"]?.ToString(); + var effectiveStatus = qmpStatus ?? status; + + var matched = string.Equals(effectiveStatus, expectedStatus, StringComparison.OrdinalIgnoreCase); + var locked = !string.IsNullOrEmpty(data?["lock"]?.ToString()); + + return (matched, locked); + } + } +} diff --git a/tests/PSProxmoxVE.Core.Tests/Utilities/GuestStatusSnapshotTests.cs b/tests/PSProxmoxVE.Core.Tests/Utilities/GuestStatusSnapshotTests.cs new file mode 100644 index 0000000..bb47d53 --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Utilities/GuestStatusSnapshotTests.cs @@ -0,0 +1,70 @@ +using PSProxmoxVE.Core.Utilities; +using Xunit; + +namespace PSProxmoxVE.Core.Tests.Utilities +{ + public class GuestStatusSnapshotTests + { + [Fact] + public void Evaluate_RunningAndUnlocked_MatchesAndIsNotLocked() + { + var json = @"{""data"": {""status"": ""running"", ""qmpstatus"": ""running""}}"; + + var result = GuestStatusSnapshot.Evaluate(json, "running"); + + Assert.True(result.StatusMatched); + Assert.False(result.Locked); + } + + [Fact] + public void Evaluate_RunningButStillLocked_MatchesAndIsLocked() + { + var json = @"{""data"": {""status"": ""running"", ""qmpstatus"": ""running"", ""lock"": ""clone""}}"; + + var result = GuestStatusSnapshot.Evaluate(json, "running"); + + Assert.True(result.StatusMatched); + Assert.True(result.Locked); + } + + [Fact] + public void Evaluate_PrefersQmpStatusOverStatus() + { + // PVE reports status=running with qmpstatus=paused for a suspended VM. + var json = @"{""data"": {""status"": ""running"", ""qmpstatus"": ""paused""}}"; + + Assert.False(GuestStatusSnapshot.Evaluate(json, "running").StatusMatched); + Assert.True(GuestStatusSnapshot.Evaluate(json, "paused").StatusMatched); + } + + [Fact] + public void Evaluate_ContainerResponseWithoutQmpStatus_FallsBackToStatus() + { + var json = @"{""data"": {""status"": ""stopped""}}"; + + var result = GuestStatusSnapshot.Evaluate(json, "stopped"); + + Assert.True(result.StatusMatched); + Assert.False(result.Locked); + } + + [Fact] + public void Evaluate_EmptyLockValue_IsNotLocked() + { + var json = @"{""data"": {""status"": ""stopped"", ""lock"": """"}}"; + + Assert.False(GuestStatusSnapshot.Evaluate(json, "stopped").Locked); + } + + [Theory] + [InlineData("")] + [InlineData(null)] + public void Evaluate_EmptyBody_DoesNotMatch(string? json) + { + var result = GuestStatusSnapshot.Evaluate(json!, "running"); + + Assert.False(result.StatusMatched); + Assert.False(result.Locked); + } + } +} From 5f1feb8b32208a414c00d146c808228dad946951 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:34:27 +0000 Subject: [PATCH 2/6] fix: wait for the config lock in WaitForStatusTransition; record D015 --- src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs | 25 ++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs index 467e57d..ab10e0b 100644 --- a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs +++ b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs @@ -1,12 +1,12 @@ using System; using System.Collections.Generic; using System.Management.Automation; -using Newtonsoft.Json.Linq; using PSProxmoxVE.Core.Authentication; using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Exceptions; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; +using PSProxmoxVE.Core.Utilities; namespace PSProxmoxVE.Cmdlets { @@ -131,21 +131,26 @@ namespace PSProxmoxVE.Cmdlets : $"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}/status/current"; var deadline = DateTime.UtcNow.AddSeconds(timeoutSeconds); + var statusReached = false; using var pollClient = new PveHttpClient(session); while (DateTime.UtcNow < deadline) { try { var json = pollClient.GetAsync(statusResource).GetAwaiter().GetResult(); - var data = JObject.Parse(json)["data"]; - var status = data?["status"]?.ToString(); - var qmpStatus = data?["qmpstatus"]?.ToString(); + var snapshot = GuestStatusSnapshot.Evaluate(json, expectedStatus); - // Use qmpstatus when available (more accurate for VM pause state) - var effectiveStatus = qmpStatus ?? status; + if (snapshot.StatusMatched) + { + statusReached = true; - if (string.Equals(effectiveStatus, expectedStatus, StringComparison.OrdinalIgnoreCase)) - return task; + // 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; + } } catch (Exception ex) when (ex is not OutOfMemoryException and not StackOverflowException) { @@ -155,6 +160,10 @@ namespace PSProxmoxVE.Cmdlets System.Threading.Thread.Sleep(2000); } + // Status reached and only the lock outlasted the deadline. + if (statusReached) + return task; + throw new PveTaskTimeoutException( task.Upid ?? "unknown", TimeSpan.FromSeconds(timeoutSeconds)); From 8cb1457ff2fec0561a75d18b1f813bffcce5e32c Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:35:36 +0000 Subject: [PATCH 3/6] docs: changelog entry and D015 for the config-lock wait --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6a1a20..10041be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Conventional Commits](https://www.conventionalcommi ### Fixed +- Lifecycle cmdlets with `-Wait` (`Start`/`Stop`/`Restart`/`Reset`/`Resume` for VMs and containers) now wait for the guest's config lock to clear, not just for the status to change. PVE reports the target status before the operation releases the lock, so a caller acting immediately afterwards could fail with `can't lock file '/var/lock/qemu-server/lock-.conf' - got timeout`. Observed as a cascade of 4 integration failures in run 183. If the lock outlasts `-Timeout` the cmdlet still returns success, as before. See `DECISIONS.md` D015. - `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 From 7e8cda93286455637192aab6f2e01899d332cc25 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:36:51 +0000 Subject: [PATCH 4/6] docs: record D015 - lifecycle -Wait blocks until the guest config lock clears --- DECISIONS.md | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/DECISIONS.md b/DECISIONS.md index b24c13f..b78c9dc 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -461,3 +461,53 @@ Add-PveClusterMember ... New-PveCluster -ClusterName 'c1' -Wait Add-PveClusterMember ... ``` + +--- + +## D015 — Lifecycle -Wait blocks until the guest config lock clears + +**Status**: Active +**Finding refs**: (none — found via integration runs 183/184, 2026-09-01) +**Resolved in scan**: n/a + +### Decision +`WaitForStatusTransition` returns only when the guest reports the expected status **and** +its config lock has cleared. Reaching the status is not enough: PVE publishes the new +status while the operation still holds `/var/lock/qemu-server/lock-.conf`, and the +next API call against that guest fails with `got timeout` trying to take the same lock. + +`lock` is read from the `status/current` response the poll already fetches — it is present +on both `qemu` and `lxc` status/current and has been since PVE 5.4, well below this +module's 7.0 floor, so this costs no extra request. + +If the status is reached but the lock outlasts `-Timeout`, the cmdlet returns success +rather than throwing. The waited-for operation did complete; only the settling ran long. +This keeps a call that succeeded before the change from becoming an exception after it. + +This is the same family as D014: a PVE task completing does not mean the resource is ready +for the next operation. D014 is the cluster-quorum instance, D015 the guest-lock instance. + +### Rationale +Integration run 183 failed four tests from one cause. `Restart-PveVm -Wait` returned after +4.1 s having observed `running`; the following `Stop-PveVm` spent exactly 10.0 s failing to +acquire the lock, which cascaded into the template convert, clone, and remove tests. Run 184, +the same commit re-run, passed: its status poll happened to take 10.1 s, by which point the +lock had cleared. The same settling happens either way — the only variable is whether the +wait absorbs it or the next caller does. + +The check lives in `WaitForStatusTransition` rather than in each cmdlet because all nine +lifecycle call sites route through it. + +### Anti-pattern (do not reintroduce) +```csharp +// NEVER treat the status transition alone as "ready for the next operation" +if (string.Equals(effectiveStatus, expectedStatus, StringComparison.OrdinalIgnoreCase)) + return task; +``` + +### Correct pattern +```csharp +var snapshot = GuestStatusSnapshot.Evaluate(json, expectedStatus); +if (snapshot.StatusMatched && !snapshot.Locked) + return task; +``` From 70c147b47d78332fe86e35aff73fc9def8710e58 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:47:41 +0000 Subject: [PATCH 5/6] fix: post-loop fallback tests the latest poll, not "ever matched" Review finding on #97: `statusReached` was sticky, so a guest that reached the expected status once and then drifted away from it would still be reported as a success at the deadline, contradicting the fallback's own meaning. Track the most recent observation instead. A guest that drifts away and never re-matches now raises PveTaskTimeoutException as it should. A failed poll leaves the previous observation standing, so an API blip is not read as divergence. Co-Authored-By: Claude Opus 5 (1M context) --- src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs index ab10e0b..c07e6f9 100644 --- a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs +++ b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs @@ -131,7 +131,7 @@ namespace PSProxmoxVE.Cmdlets : $"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}/status/current"; var deadline = DateTime.UtcNow.AddSeconds(timeoutSeconds); - var statusReached = false; + var lastMatched = false; using var pollClient = new PveHttpClient(session); while (DateTime.UtcNow < deadline) { @@ -139,11 +139,10 @@ namespace PSProxmoxVE.Cmdlets { var json = pollClient.GetAsync(statusResource).GetAwaiter().GetResult(); var snapshot = GuestStatusSnapshot.Evaluate(json, expectedStatus); + lastMatched = snapshot.StatusMatched; if (snapshot.StatusMatched) { - statusReached = true; - // 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' @@ -160,8 +159,9 @@ namespace PSProxmoxVE.Cmdlets System.Threading.Thread.Sleep(2000); } - // Status reached and only the lock outlasted the deadline. - if (statusReached) + // The guest still reports the expected status on the final poll and only the + // lock outlasted the deadline. + if (lastMatched) return task; throw new PveTaskTimeoutException( From 27d12d8736a1e25307cb8cab2bc9d92d1e1ffd83 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:49:02 +0000 Subject: [PATCH 6/6] docs: D015 states the fallback tests the latest observation --- DECISIONS.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/DECISIONS.md b/DECISIONS.md index b78c9dc..9587b50 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -480,9 +480,15 @@ next API call against that guest fails with `got timeout` trying to take the sam on both `qemu` and `lxc` status/current and has been since PVE 5.4, well below this module's 7.0 floor, so this costs no extra request. -If the status is reached but the lock outlasts `-Timeout`, the cmdlet returns success -rather than throwing. The waited-for operation did complete; only the settling ran long. -This keeps a call that succeeded before the change from becoming an exception after it. +If the guest still reports the expected status on the final poll but the lock outlasts +`-Timeout`, the cmdlet returns success rather than throwing. The waited-for operation did +complete; only the settling ran long. This keeps a call that succeeded before the change +from becoming an exception after it. + +That fallback tests the **most recent** observation, not "matched at some point during the +wait". A guest that reached the expected status and then drifted away from it has not +satisfied the wait and still raises `PveTaskTimeoutException`. A poll that fails outright +leaves the previous observation standing, so a single API blip is not read as divergence. This is the same family as D014: a PVE task completing does not mean the resource is ready for the next operation. D014 is the cluster-quorum instance, D015 the guest-lock instance.