fix: Wait-PveTask delegates to TaskService.WaitForTask (#178)

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>
This commit is contained in:
goodolclint-claude[bot]
2026-09-02 19:27:31 +00:00
committed by GitHub
parent 8ec09c2b84
commit 17832f15d9
14 changed files with 143 additions and 69 deletions
@@ -46,7 +46,7 @@ namespace PSProxmoxVE.Cmdlets.CloudInit
if (Wait.IsPresent && !string.IsNullOrEmpty(upid)) 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); WriteObject(task);
@@ -54,7 +54,7 @@ namespace PSProxmoxVE.Cmdlets.Containers
if (Wait.IsPresent) if (Wait.IsPresent)
{ {
var taskService = new TaskService(); var taskService = new TaskService();
task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); task = taskService.WaitForTask(session, Node, task.Upid);
} }
WriteObject(task); WriteObject(task);
@@ -67,7 +67,7 @@ namespace PSProxmoxVE.Cmdlets.Containers
if (Wait.IsPresent) if (Wait.IsPresent)
{ {
var taskService = new TaskService(); var taskService = new TaskService();
task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); task = taskService.WaitForTask(session, Node, task.Upid);
} }
WriteObject(task); WriteObject(task);
@@ -209,7 +209,7 @@ namespace PSProxmoxVE.Cmdlets.Containers
if (Wait.IsPresent) if (Wait.IsPresent)
{ {
var taskService = new TaskService(); 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); WriteObject(task);
@@ -68,7 +68,7 @@ namespace PSProxmoxVE.Cmdlets.Containers
if (Wait.IsPresent) if (Wait.IsPresent)
{ {
var taskService = new TaskService(); 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); WriteObject(task);
@@ -53,7 +53,7 @@ namespace PSProxmoxVE.Cmdlets.Containers
if (Wait.IsPresent) if (Wait.IsPresent)
{ {
var taskService = new TaskService(); var taskService = new TaskService();
task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); task = taskService.WaitForTask(session, Node, task.Upid);
} }
WriteObject(task); WriteObject(task);
@@ -53,7 +53,7 @@ namespace PSProxmoxVE.Cmdlets.Containers
if (Wait.IsPresent) if (Wait.IsPresent)
{ {
var taskService = new TaskService(); var taskService = new TaskService();
task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); task = taskService.WaitForTask(session, Node, task.Upid);
} }
WriteObject(task); WriteObject(task);
+1 -1
View File
@@ -186,7 +186,7 @@ namespace PSProxmoxVE.Cmdlets
var task = issueOperation(); var task = issueOperation();
return string.IsNullOrEmpty(task.Upid) return string.IsNullOrEmpty(task.Upid)
? task ? 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}")); onRetry: ex => WriteVerbose($"Guest is locked, retrying: {ex.Message}"));
} }
@@ -1,9 +1,9 @@
using System; using System;
using System.Management.Automation; using System.Management.Automation;
using Newtonsoft.Json.Linq;
using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Client;
using PSProxmoxVE.Core.Exceptions; using PSProxmoxVE.Core.Exceptions;
using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Models.Vms;
using PSProxmoxVE.Core.Services;
namespace PSProxmoxVE.Cmdlets.Tasks namespace PSProxmoxVE.Cmdlets.Tasks
{ {
@@ -20,6 +20,13 @@ namespace PSProxmoxVE.Cmdlets.Tasks
[OutputType(typeof(PveTask))] [OutputType(typeof(PveTask))]
public sealed class WaitPveTaskCmdlet : PveCmdletBase public sealed class WaitPveTaskCmdlet : PveCmdletBase
{ {
/// <summary>
/// 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.
/// </summary>
private static readonly TimeSpan NoTimeout = TimeSpan.FromDays(36500);
/// <summary>The node on which the task is running.</summary> /// <summary>The node on which the task is running.</summary>
[Parameter(Mandatory = true, Position = 0, HelpMessage = "The PVE node name.")] [Parameter(Mandatory = true, Position = 0, HelpMessage = "The PVE node name.")]
public string Node { get; set; } = string.Empty; public string Node { get; set; } = string.Empty;
@@ -48,72 +55,40 @@ namespace PSProxmoxVE.Cmdlets.Tasks
{ {
var session = GetSession(); var session = GetSession();
using var client = new PveHttpClient(session); using var client = new PveHttpClient(session);
var taskService = new TaskService(client);
var poll = PollInterval ?? TimeSpan.FromSeconds(2); var activityId = Math.Abs(Upid.GetHashCode()) % 1000 + 1;
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 taskDesc = Upid.Length > 50 ? Upid.Substring(0, 47) + "..." : Upid; 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) 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 try
{ {
while (true) var task = taskService.WaitForTask(session, Node, Upid, Timeout ?? NoTimeout, PollInterval,
{ ReportProgress);
if (DateTime.UtcNow >= deadline)
throw new PveTaskTimeoutException(Upid, Timeout!.Value);
WriteProgress(progressRecord); progressRecord.RecordType = ProgressRecordType.Completed;
WriteProgress(progressRecord);
var elapsed = Timeout.HasValue WriteObject(task);
? (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;
}
}
} }
catch (PveTaskTimeoutException ex) catch (PveTaskTimeoutException ex)
{ {
@@ -124,6 +99,8 @@ namespace PSProxmoxVE.Cmdlets.Tasks
} }
catch (PveTaskFailedException ex) catch (PveTaskFailedException ex)
{ {
progressRecord.RecordType = ProgressRecordType.Completed;
WriteProgress(progressRecord);
ThrowTerminatingError(new ErrorRecord( ThrowTerminatingError(new ErrorRecord(
ex, "PveTaskFailed", ErrorCategory.OperationStopped, Upid)); ex, "PveTaskFailed", ErrorCategory.OperationStopped, Upid));
} }
@@ -85,7 +85,7 @@ namespace PSProxmoxVE.Cmdlets.Vms
if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid)) if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid))
{ {
var taskService = new TaskService(); var taskService = new TaskService();
task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); task = taskService.WaitForTask(session, Node, task.Upid);
} }
WriteObject(task); WriteObject(task);
@@ -66,7 +66,7 @@ namespace PSProxmoxVE.Cmdlets.Vms
if (Wait.IsPresent) if (Wait.IsPresent)
{ {
var taskService = new TaskService(); var taskService = new TaskService();
task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); task = taskService.WaitForTask(session, Node, task.Upid);
} }
WriteObject(task); WriteObject(task);
@@ -74,7 +74,7 @@ namespace PSProxmoxVE.Cmdlets.Vms
if (Wait.IsPresent) if (Wait.IsPresent)
{ {
var taskService = new TaskService(); var taskService = new TaskService();
task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); task = taskService.WaitForTask(session, Node, task.Upid);
} }
WriteObject(task); WriteObject(task);
@@ -68,7 +68,7 @@ namespace PSProxmoxVE.Cmdlets.Vms
if (Wait.IsPresent) if (Wait.IsPresent)
{ {
var taskService = new TaskService(); var taskService = new TaskService();
task = taskService.WaitForTask(session, Node, task.Upid, null, null, null); task = taskService.WaitForTask(session, Node, task.Upid);
} }
WriteObject(task); WriteObject(task);
@@ -239,6 +239,103 @@ namespace PSProxmoxVE.Core.Tests.Services
Assert.Equal(timeout, ex.Timeout); 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<IPveHttpClient>();
mockClient.Setup(c => c.GetAsync(It.IsAny<string>()))
.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<string>()), 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<IPveHttpClient>();
mockClient.SetupSequence(c => c.GetAsync(It.IsAny<string>()))
.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<string>()), 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<IPveHttpClient>();
mockClient.SetupSequence(c => c.GetAsync(It.IsAny<string>()))
.ReturnsAsync(runningJson)
.ReturnsAsync(runningJson)
.ReturnsAsync(stoppedJson);
var service = new TaskService(mockClient.Object);
var session = CreateSession();
var seenStatuses = new List<string?>();
// 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<string?> { "running", "running", "stopped" }, seenStatuses);
}
[Fact] [Fact]
public void StopTask_CallsDeleteAsyncWithCorrectPath() public void StopTask_CallsDeleteAsyncWithCorrectPath()
{ {