From 17832f15d9fbd452d4e6048d841b49600e5603f2 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:27:31 +0000 Subject: [PATCH] fix: Wait-PveTask delegates to TaskService.WaitForTask (#178) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WaitPveTaskCmdlet hand-rolled its own poll loop (while(true) / Thread.Sleep / JObject.Parse), the exact pattern ADR 0001 forbids. It now delegates to TaskService.WaitForTask, passing a progress callback that drives WriteProgress — the seam WaitForTask was built for and that nothing called until now. Preserves the cmdlet's own documented contract (an omitted -Timeout waits indefinitely) by passing a 100-year sentinel instead of null, since TaskService.WaitForTask treats a null timeout as its own 10-minute default, not infinite. Reuses one PveHttpClient for the whole wait instead of letting TaskService open a fresh one per poll. Also strips the redundant trailing 'null, null, null' default arguments from 12 other WaitForTask call sites so they use the short form, per the issue. Left CopyPveContainerCmdlet.cs, ImportPveOvaCmdlet.cs and NewPveVmCmdlet.cs alone — issue #135 is touching those concurrently. Adds three xUnit tests to TaskServiceTests.cs proving the behavior the inline loop got wrong: no sleep before the first status check, the 1-second MinPollInterval clamp, and the progress callback firing on every poll. Mutation-tested by breaking each behavior in turn and confirming the corresponding test fails. Closes #140 Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- .../InvokePveCloudInitRegenerateCmdlet.cs | 2 +- .../Containers/MovePveContainerCmdlet.cs | 2 +- .../MovePveContainerVolumeCmdlet.cs | 2 +- .../Containers/NewPveContainerCmdlet.cs | 2 +- .../Containers/RemovePveContainerCmdlet.cs | 2 +- .../Containers/ResumePveContainerCmdlet.cs | 2 +- .../Containers/SuspendPveContainerCmdlet.cs | 2 +- src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs | 2 +- .../Cmdlets/Tasks/WaitPveTaskCmdlet.cs | 91 +++++++---------- .../Cmdlets/Vms/ImportPveVmDiskCmdlet.cs | 2 +- .../Cmdlets/Vms/MovePveVmCmdlet.cs | 2 +- .../Cmdlets/Vms/MovePveVmDiskCmdlet.cs | 2 +- .../Cmdlets/Vms/RemovePveVmCmdlet.cs | 2 +- .../Services/TaskServiceTests.cs | 97 +++++++++++++++++++ 14 files changed, 143 insertions(+), 69 deletions(-) diff --git a/src/PSProxmoxVE/Cmdlets/CloudInit/InvokePveCloudInitRegenerateCmdlet.cs b/src/PSProxmoxVE/Cmdlets/CloudInit/InvokePveCloudInitRegenerateCmdlet.cs index c013e1a..6a6da6b 100644 --- a/src/PSProxmoxVE/Cmdlets/CloudInit/InvokePveCloudInitRegenerateCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/CloudInit/InvokePveCloudInitRegenerateCmdlet.cs @@ -46,7 +46,7 @@ namespace PSProxmoxVE.Cmdlets.CloudInit if (Wait.IsPresent && !string.IsNullOrEmpty(upid)) { - task = new TaskService().WaitForTask(session, Node, upid, null, null, null); + task = new TaskService().WaitForTask(session, Node, upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Containers/MovePveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/MovePveContainerCmdlet.cs index 3505d98..f7ce0f0 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/MovePveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/MovePveContainerCmdlet.cs @@ -54,7 +54,7 @@ namespace PSProxmoxVE.Cmdlets.Containers if (Wait.IsPresent) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Containers/MovePveContainerVolumeCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/MovePveContainerVolumeCmdlet.cs index 90924c7..3ed2ec3 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/MovePveContainerVolumeCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/MovePveContainerVolumeCmdlet.cs @@ -67,7 +67,7 @@ namespace PSProxmoxVE.Cmdlets.Containers if (Wait.IsPresent) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs index 42b4d9d..56c67d4 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs @@ -209,7 +209,7 @@ namespace PSProxmoxVE.Cmdlets.Containers if (Wait.IsPresent) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, task.Node ?? Node, task.Upid!, null, null, null); + task = taskService.WaitForTask(session, task.Node ?? Node, task.Upid!); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerCmdlet.cs index 7c6e5a3..e45a4de 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerCmdlet.cs @@ -68,7 +68,7 @@ namespace PSProxmoxVE.Cmdlets.Containers if (Wait.IsPresent) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, task.Node ?? Node, task.Upid!, null, null, null); + task = taskService.WaitForTask(session, task.Node ?? Node, task.Upid!); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Containers/ResumePveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/ResumePveContainerCmdlet.cs index 67f3a1b..ccc814c 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/ResumePveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/ResumePveContainerCmdlet.cs @@ -53,7 +53,7 @@ namespace PSProxmoxVE.Cmdlets.Containers if (Wait.IsPresent) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Containers/SuspendPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/SuspendPveContainerCmdlet.cs index 92927c8..97e74da 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/SuspendPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/SuspendPveContainerCmdlet.cs @@ -53,7 +53,7 @@ namespace PSProxmoxVE.Cmdlets.Containers if (Wait.IsPresent) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs index 0a32a99..3a3cc31 100644 --- a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs +++ b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs @@ -186,7 +186,7 @@ namespace PSProxmoxVE.Cmdlets var task = issueOperation(); return string.IsNullOrEmpty(task.Upid) ? task - : taskService.WaitForTask(session, node, task.Upid, null, null, null); + : taskService.WaitForTask(session, node, task.Upid); }, onRetry: ex => WriteVerbose($"Guest is locked, retrying: {ex.Message}")); } diff --git a/src/PSProxmoxVE/Cmdlets/Tasks/WaitPveTaskCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Tasks/WaitPveTaskCmdlet.cs index 6de8c12..1fc70e1 100644 --- a/src/PSProxmoxVE/Cmdlets/Tasks/WaitPveTaskCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Tasks/WaitPveTaskCmdlet.cs @@ -1,9 +1,9 @@ using System; using System.Management.Automation; -using Newtonsoft.Json.Linq; using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Exceptions; using PSProxmoxVE.Core.Models.Vms; +using PSProxmoxVE.Core.Services; namespace PSProxmoxVE.Cmdlets.Tasks { @@ -20,6 +20,13 @@ namespace PSProxmoxVE.Cmdlets.Tasks [OutputType(typeof(PveTask))] public sealed class WaitPveTaskCmdlet : PveCmdletBase { + /// + /// TaskService.WaitForTask treats a null timeout as "use its own 10-minute default", + /// not "wait forever" — this cmdlet's own contract is the latter, so an omitted + /// -Timeout is passed through as this instead of null. + /// + private static readonly TimeSpan NoTimeout = TimeSpan.FromDays(36500); + /// The node on which the task is running. [Parameter(Mandatory = true, Position = 0, HelpMessage = "The PVE node name.")] public string Node { get; set; } = string.Empty; @@ -48,72 +55,40 @@ namespace PSProxmoxVE.Cmdlets.Tasks { var session = GetSession(); using var client = new PveHttpClient(session); + var taskService = new TaskService(client); - var poll = PollInterval ?? TimeSpan.FromSeconds(2); - var deadline = Timeout.HasValue ? DateTime.UtcNow + Timeout.Value : DateTime.MaxValue; - - var encodedUpid = Uri.EscapeDataString(Upid); - var statusResource = $"nodes/{Uri.EscapeDataString(Node)}/tasks/{encodedUpid}/status"; - - // Derive a short human-readable description from the UPID for progress display + var activityId = Math.Abs(Upid.GetHashCode()) % 1000 + 1; var taskDesc = Upid.Length > 50 ? Upid.Substring(0, 47) + "..." : Upid; - - var activityId = Math.Abs(Upid.GetHashCode()) % 1000 + 1; var progressRecord = new ProgressRecord(activityId, $"Waiting for task on {Node}", taskDesc) { - PercentComplete = -1 // indeterminate + PercentComplete = -1 }; + var startedAt = DateTime.UtcNow; + + void ReportProgress(PveTask task) + { + if (Timeout.HasValue) + { + var totalSecs = (int)Timeout.Value.TotalSeconds; + var elapsed = (int)(DateTime.UtcNow - startedAt).TotalSeconds; + progressRecord.PercentComplete = totalSecs > 0 + ? Math.Min(99, (elapsed * 100) / totalSecs) + : -1; + progressRecord.SecondsRemaining = Math.Max(0, totalSecs - elapsed); + } + WriteProgress(progressRecord); + } + try { - while (true) - { - if (DateTime.UtcNow >= deadline) - throw new PveTaskTimeoutException(Upid, Timeout!.Value); + var task = taskService.WaitForTask(session, Node, Upid, Timeout ?? NoTimeout, PollInterval, + ReportProgress); - WriteProgress(progressRecord); + progressRecord.RecordType = ProgressRecordType.Completed; + WriteProgress(progressRecord); - var elapsed = Timeout.HasValue - ? (int)((DateTime.UtcNow - (deadline - Timeout.Value)).TotalSeconds) - : -1; - if (Timeout.HasValue && elapsed >= 0) - { - var totalSecs = (int)Timeout.Value.TotalSeconds; - progressRecord.PercentComplete = totalSecs > 0 - ? Math.Min(99, (elapsed * 100) / totalSecs) - : -1; - progressRecord.SecondsRemaining = Math.Max(0, totalSecs - elapsed); - } - - System.Threading.Thread.Sleep((int)poll.TotalMilliseconds); - - var statusJson = client.GetAsync(statusResource).GetAwaiter().GetResult(); - var statusRoot = JObject.Parse(statusJson); - var data = statusRoot["data"]; - - var status = data?["status"]?.ToString(); - var exitStatus = data?["exitstatus"]?.ToString(); - - if (status == "stopped") - { - progressRecord.RecordType = ProgressRecordType.Completed; - WriteProgress(progressRecord); - - var task = new PveTask - { - Upid = Upid, - Node = Node, - Status = status, - ExitStatus = exitStatus - }; - - if (!task.IsSuccessful && !string.IsNullOrEmpty(exitStatus) && exitStatus != "OK") - throw new PveTaskFailedException(Upid, exitStatus!); - - WriteObject(task); - return; - } - } + WriteObject(task); } catch (PveTaskTimeoutException ex) { @@ -124,6 +99,8 @@ namespace PSProxmoxVE.Cmdlets.Tasks } catch (PveTaskFailedException ex) { + progressRecord.RecordType = ProgressRecordType.Completed; + WriteProgress(progressRecord); ThrowTerminatingError(new ErrorRecord( ex, "PveTaskFailed", ErrorCategory.OperationStopped, Upid)); } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/ImportPveVmDiskCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/ImportPveVmDiskCmdlet.cs index 4ad96bb..82b5854 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/ImportPveVmDiskCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/ImportPveVmDiskCmdlet.cs @@ -85,7 +85,7 @@ namespace PSProxmoxVE.Cmdlets.Vms if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid)) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Vms/MovePveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/MovePveVmCmdlet.cs index b8e023d..383efe5 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/MovePveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/MovePveVmCmdlet.cs @@ -66,7 +66,7 @@ namespace PSProxmoxVE.Cmdlets.Vms if (Wait.IsPresent) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Vms/MovePveVmDiskCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/MovePveVmDiskCmdlet.cs index 427af7a..70c63e4 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/MovePveVmDiskCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/MovePveVmDiskCmdlet.cs @@ -74,7 +74,7 @@ namespace PSProxmoxVE.Cmdlets.Vms if (Wait.IsPresent) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Vms/RemovePveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/RemovePveVmCmdlet.cs index e5901ed..63a9bc7 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/RemovePveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/RemovePveVmCmdlet.cs @@ -68,7 +68,7 @@ namespace PSProxmoxVE.Cmdlets.Vms if (Wait.IsPresent) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/tests/PSProxmoxVE.Core.Tests/Services/TaskServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/TaskServiceTests.cs index c02017f..dd8f7d3 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/TaskServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/TaskServiceTests.cs @@ -239,6 +239,103 @@ namespace PSProxmoxVE.Core.Tests.Services Assert.Equal(timeout, ex.Timeout); } + [Fact] + public void WaitForTask_AlreadyStopped_ChecksStatusBeforeSleeping() + { + // Arrange + var json = @"{ + ""data"": { + ""upid"": ""UPID:pve1:000ABC:00000001:5F1234AB:qmstart:100:root@pam:"", + ""status"": ""stopped"", + ""exitstatus"": ""OK"", + ""user"": ""root@pam"" + } + }"; + + var mockClient = new Mock(); + mockClient.Setup(c => c.GetAsync(It.IsAny())) + .ReturnsAsync(json); + + var service = new TaskService(mockClient.Object); + var session = CreateSession(); + + // Act — a long poll interval would dominate the elapsed time if the + // implementation slept before its first status check. + var stopwatch = System.Diagnostics.Stopwatch.StartNew(); + var task = service.WaitForTask(session, TestNode, TestUpid, + timeout: TimeSpan.FromSeconds(30), + pollInterval: TimeSpan.FromSeconds(10)); + stopwatch.Stop(); + + // Assert + Assert.True(task.IsSuccessful); + Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(5), + $"Expected an immediate return for an already-stopped task, took {stopwatch.Elapsed}"); + mockClient.Verify(c => c.GetAsync(It.IsAny()), Times.Once); + } + + [Fact] + public void WaitForTask_PollIntervalBelowMinimum_IsClampedToOneSecond() + { + // Arrange — task reports "running" once, then "stopped". + var runningJson = @"{ ""data"": { ""status"": ""running"", ""user"": ""root@pam"" } }"; + var stoppedJson = @"{ + ""data"": { ""status"": ""stopped"", ""exitstatus"": ""OK"", ""user"": ""root@pam"" } + }"; + + var mockClient = new Mock(); + mockClient.SetupSequence(c => c.GetAsync(It.IsAny())) + .ReturnsAsync(runningJson) + .ReturnsAsync(stoppedJson); + + var service = new TaskService(mockClient.Object); + var session = CreateSession(); + + // Act — a zero poll interval must be clamped to the 1-second minimum, + // not passed through to Thread.Sleep(0). + var stopwatch = System.Diagnostics.Stopwatch.StartNew(); + var task = service.WaitForTask(session, TestNode, TestUpid, + timeout: TimeSpan.FromSeconds(30), + pollInterval: TimeSpan.Zero); + stopwatch.Stop(); + + // Assert + Assert.True(task.IsSuccessful); + Assert.True(stopwatch.Elapsed >= TimeSpan.FromMilliseconds(900), + $"Expected the clamp to force at least a ~1-second wait, took {stopwatch.Elapsed}"); + mockClient.Verify(c => c.GetAsync(It.IsAny()), Times.Exactly(2)); + } + + [Fact] + public void WaitForTask_ProgressCallback_InvokedOnEachPoll() + { + // Arrange — two polls report "running", the third reports "stopped". + var runningJson = @"{ ""data"": { ""status"": ""running"", ""user"": ""root@pam"" } }"; + var stoppedJson = @"{ + ""data"": { ""status"": ""stopped"", ""exitstatus"": ""OK"", ""user"": ""root@pam"" } + }"; + + var mockClient = new Mock(); + mockClient.SetupSequence(c => c.GetAsync(It.IsAny())) + .ReturnsAsync(runningJson) + .ReturnsAsync(runningJson) + .ReturnsAsync(stoppedJson); + + var service = new TaskService(mockClient.Object); + var session = CreateSession(); + var seenStatuses = new List(); + + // Act + var task = service.WaitForTask(session, TestNode, TestUpid, + timeout: TimeSpan.FromSeconds(30), + pollInterval: TimeSpan.FromSeconds(1), + progressCallback: t => seenStatuses.Add(t.Status)); + + // Assert + Assert.True(task.IsSuccessful); + Assert.Equal(new List { "running", "running", "stopped" }, seenStatuses); + } + [Fact] public void StopTask_CallsDeleteAsyncWithCorrectPath() {