Merge pull request #97 from GoodOlClint/fix/vm-config-lock-wait

fix: lifecycle -Wait blocks until the guest config lock clears

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-<vmid>.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

DECISIONS.md

@@ -461,3 +461,59 @@ 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-<vmid>.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 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.
+
+### 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;
+```

src/PSProxmoxVE.Core/Utilities/GuestStatusSnapshot.cs

@@ -0,0 +1,40 @@
+using System;
+using Newtonsoft.Json.Linq;
+
+namespace PSProxmoxVE.Core.Utilities
+{
+    /// <summary>
+    /// Reads the fields of a guest status/current response that decide whether a
+    /// lifecycle wait (-Wait on Start, Stop, Restart, Reset, Resume) is finished.
+    /// </summary>
+    public static class GuestStatusSnapshot
+    {
+        /// <summary>
+        /// Evaluates a status/current response body against the status a caller is waiting for.
+        /// </summary>
+        /// <param name="json">Raw status/current response body.</param>
+        /// <param name="expectedStatus">The status being waited for (e.g. "running", "stopped", "paused").</param>
+        /// <returns>
+        /// StatusMatched: the guest reports <paramref name="expectedStatus"/>. 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.
+        /// </returns>
+        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);
+        }
+    }
+}

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,25 @@ namespace PSProxmoxVE.Cmdlets
                 : $"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}/status/current";
 
             var deadline = DateTime.UtcNow.AddSeconds(timeoutSeconds);
+            var lastMatched = 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);
+                    lastMatched = snapshot.StatusMatched;
 
-                    // Use qmpstatus when available (more accurate for VM pause state)
-                    var effectiveStatus = qmpStatus ?? status;
-
-                    if (string.Equals(effectiveStatus, expectedStatus, StringComparison.OrdinalIgnoreCase))
-                        return task;
+                    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-<vmid>.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 +159,11 @@ namespace PSProxmoxVE.Cmdlets
                 System.Threading.Thread.Sleep(2000);
             }
 
+            // 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(
                 task.Upid ?? "unknown",
                 TimeSpan.FromSeconds(timeoutSeconds));

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);
+        }
+    }
+}