From 716ecd100dfd45df19b3ef20d929bf7808e87aa3 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:29:46 +0000 Subject: [PATCH] fix: Import-PveOva's dead not-found catch and missing upload timeout (#180) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: retarget Import-PveOva's not-found catch and add upload timeouts VmService.GetVm throws InvalidOperationException when the VM is not yet listed on the node, never PveApiException(NotFound) — the API call itself returns 200. Import-PveOva's tail catch was for the exception GetVm never throws, so a successful import without -Wait raised the InvalidOperationException unhandled instead of falling back to a basic PveVm. Retarget the catch, and narrow its try region to the GetVm call only so the fallback can no longer fire for a WriteObject failure on an already-retrieved VM. VmService.UploadOva and StorageService.UploadIso built their PveHttpClient with no timeout override, so large OVA/ISO uploads inherited the session's 100s default and aborted mid-transfer. Both now take a TimeSpan? timeout (default 30 minutes), matching Send-PveFile. Import-PveOva gains -TimeoutSeconds mirroring Send-PveFileCmdlet's parameter. Also drops the trailing null, null, null on WaitForTask calls in ImportPveOvaCmdlet.cs and NewPveVmCmdlet.cs, left over from #140. Closes #139 * fix: give UploadOva/UploadIso a timeout override and add coverage Completes the #139 fix: VmService.UploadOva and StorageService.UploadIso now take a TimeSpan? timeout (default 30 minutes) instead of always using the session's 100s default. Adds a GetVm not-found regression test and a timeout-propagation test suite for both upload methods. * test: release the upload before deleting its temp file The two default-timeout tests left the upload in flight and then deleted the file it still had open. Windows refuses that, so both build-and-test legs failed on windows-latest. --------- Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- .../Services/StorageService.cs | 9 +- src/PSProxmoxVE.Core/Services/VmService.cs | 9 +- .../Cmdlets/Vms/ImportPveOvaCmdlet.cs | 50 +++-- src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs | 2 +- .../Services/UploadTimeoutTests.cs | 202 ++++++++++++++++++ .../Services/VmServiceTests.cs | 19 ++ 6 files changed, 271 insertions(+), 20 deletions(-) create mode 100644 tests/PSProxmoxVE.Core.Tests/Services/UploadTimeoutTests.cs diff --git a/src/PSProxmoxVE.Core/Services/StorageService.cs b/src/PSProxmoxVE.Core/Services/StorageService.cs index 3838850..5b4162e 100644 --- a/src/PSProxmoxVE.Core/Services/StorageService.cs +++ b/src/PSProxmoxVE.Core/Services/StorageService.cs @@ -113,6 +113,10 @@ namespace PSProxmoxVE.Core.Services /// Optional checksum value. /// Optional checksum algorithm (e.g. "sha256"). /// Optional callback with (bytesSent, totalBytes). + /// + /// HTTP timeout override for this upload. Defaults to 30 minutes, overriding the + /// session's default 100-second timeout so that large files have time to transfer. + /// public PveTask UploadIso( PveSession session, string node, @@ -120,7 +124,8 @@ namespace PSProxmoxVE.Core.Services string filePath, string? checksum = null, string? checksumAlgorithm = null, - Action? progressCallback = null) + Action? progressCallback = null, + TimeSpan? timeout = null) { if (session == null) throw new ArgumentNullException(nameof(session)); if (string.IsNullOrWhiteSpace(node)) throw new ArgumentNullException(nameof(node)); @@ -132,7 +137,7 @@ namespace PSProxmoxVE.Core.Services ["content"] = "iso" }; - IPveHttpClient client = _injectedClient ?? new PveHttpClient(session); + IPveHttpClient client = _injectedClient ?? new PveHttpClient(session, timeout ?? TimeSpan.FromMinutes(30)); try { var response = client.UploadFileAsync( diff --git a/src/PSProxmoxVE.Core/Services/VmService.cs b/src/PSProxmoxVE.Core/Services/VmService.cs index 013140c..347acfc 100644 --- a/src/PSProxmoxVE.Core/Services/VmService.cs +++ b/src/PSProxmoxVE.Core/Services/VmService.cs @@ -945,12 +945,17 @@ namespace PSProxmoxVE.Core.Services /// Optional callback invoked periodically with (bytesSent, totalBytes). /// May be called from a background thread. /// + /// + /// HTTP timeout override for this upload. Defaults to 30 minutes, overriding the + /// session's default 100-second timeout so that large OVA files have time to transfer. + /// public PveTask UploadOva( PveSession session, string node, string storage, string ovaPath, - Action? progressCallback = null) + Action? progressCallback = null, + TimeSpan? timeout = null) { if (session == null) throw new ArgumentNullException(nameof(session)); if (string.IsNullOrWhiteSpace(node)) throw new ArgumentNullException(nameof(node)); @@ -962,7 +967,7 @@ namespace PSProxmoxVE.Core.Services ["content"] = "import" }; - IPveHttpClient client = _injectedClient ?? new PveHttpClient(session); + IPveHttpClient client = _injectedClient ?? new PveHttpClient(session, timeout ?? TimeSpan.FromMinutes(30)); try { var response = client.UploadFileAsync( diff --git a/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs index 5b89ef8..2c74acd 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs @@ -2,8 +2,6 @@ using System; using System.Collections.Generic; using System.IO; using System.Management.Automation; -using System.Net; -using PSProxmoxVE.Core.Exceptions; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; @@ -83,6 +81,15 @@ namespace PSProxmoxVE.Cmdlets.Vms [Parameter(Mandatory = false, HelpMessage = "Wait for all tasks to complete before returning.")] public SwitchParameter Wait { get; set; } + /// + /// HTTP timeout for the OVA upload, in seconds. Pass 0 for infinite (no timeout). + /// When omitted, defaults to 30 minutes — overriding the session timeout so that + /// large OVA uploads do not trip the default 100-second HttpClient timeout. + /// + [Parameter(Mandatory = false, HelpMessage = "HTTP timeout in seconds (0 = infinite). Defaults to 1800 (30 min).")] + [ValidateRange(0, int.MaxValue)] + public int? TimeoutSeconds { get; set; } + protected override void ProcessRecord() { // Validate the OVA file exists @@ -164,11 +171,24 @@ namespace PSProxmoxVE.Cmdlets.Vms $"Importing OVA to {Node}", $"Uploading {fileName}..."); + TimeSpan uploadTimeout; + if (TimeoutSeconds.HasValue) + { + uploadTimeout = TimeoutSeconds.Value == 0 + ? System.Threading.Timeout.InfiniteTimeSpan + : TimeSpan.FromSeconds(TimeoutSeconds.Value); + } + else + { + uploadTimeout = TimeSpan.FromMinutes(30); + } + long progressBytes = 0; var uploadTask = System.Threading.Tasks.Task.Run(() => vmService.UploadOva(session, Node, Storage, Path, (bytesSent, _) => - System.Threading.Interlocked.Exchange(ref progressBytes, bytesSent))); + System.Threading.Interlocked.Exchange(ref progressBytes, bytesSent), + uploadTimeout)); // Poll progress on the pipeline thread while (!uploadTask.IsCompleted) @@ -193,7 +213,7 @@ namespace PSProxmoxVE.Cmdlets.Vms if (!string.IsNullOrEmpty(uploadResult.Upid)) { WriteVerbose("Waiting for OVA upload task to complete on PVE..."); - var completedUpload = taskService.WaitForTask(session, Node, uploadResult.Upid, null, null, null); + var completedUpload = taskService.WaitForTask(session, Node, uploadResult.Upid); if (completedUpload.ExitStatus != null && completedUpload.ExitStatus != "OK") { ThrowTerminatingError(new ErrorRecord( @@ -278,7 +298,7 @@ namespace PSProxmoxVE.Cmdlets.Vms if (Wait.IsPresent && !string.IsNullOrEmpty(createTask.Upid)) { WriteVerbose("Waiting for VM creation + disk import to complete..."); - var completedCreate = taskService.WaitForTask(session, Node, createTask.Upid, null, null, null); + var completedCreate = taskService.WaitForTask(session, Node, createTask.Upid); if (completedCreate.ExitStatus != null && completedCreate.ExitStatus != "OK") { ThrowTerminatingError(new ErrorRecord( @@ -292,23 +312,23 @@ namespace PSProxmoxVE.Cmdlets.Vms // Step 8: Output the created VM WriteVerbose("Retrieving created VM..."); + PveVm? vm = null; try { - var vm = vmService.GetVm(session, Node, vmId); - WriteObject(vm); + vm = vmService.GetVm(session, Node, vmId); } - catch (PveApiException ex) when (ex.StatusCode == HttpStatusCode.NotFound) + catch (InvalidOperationException ex) { // VM not yet queryable (e.g. disk import still in progress); return basic info WriteVerbose($"VM retrieval failed, returning basic info: {ex.Message}"); - WriteObject(new PveVm - { - VmId = vmId, - Name = vmName, - Node = Node, - Status = "stopped" - }); } + WriteObject(vm ?? new PveVm + { + VmId = vmId, + Name = vmName, + Node = Node, + Status = "stopped" + }); } } } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs index f3d4fb3..e898dbb 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs @@ -255,7 +255,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/UploadTimeoutTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/UploadTimeoutTests.cs new file mode 100644 index 0000000..23d298b --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Services/UploadTimeoutTests.cs @@ -0,0 +1,202 @@ +using System; +using System.Diagnostics; +using System.IO; +using System.Net; +using System.Net.Security; +using System.Net.Sockets; +using System.Security.Cryptography; +using System.Security.Cryptography.X509Certificates; +using System.Threading.Tasks; +using PSProxmoxVE.Core.Authentication; +using PSProxmoxVE.Core.Exceptions; +using PSProxmoxVE.Core.Services; +using Xunit; + +namespace PSProxmoxVE.Core.Tests.Services +{ + /// + /// Proves that and + /// apply their own timeout parameter to the they + /// construct, instead of always inheriting . Each test runs a + /// real TLS server on loopback that completes the handshake and then never answers the HTTP + /// request, so the request can only end via the client-side timeout. + /// + public class UploadTimeoutTests + { + private static X509Certificate2 CreateSelfSignedServerCert() + { + using var rsa = RSA.Create(2048); + var request = new CertificateRequest("CN=127.0.0.1", rsa, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1); + var cert = request.CreateSelfSigned(DateTimeOffset.UtcNow.AddDays(-1), DateTimeOffset.UtcNow.AddDays(1)); + // Re-import as PKCS12 — an ephemeral CertificateRequest key set is not usable by + // SslStream's server-auth path on every platform without this round-trip. + return new X509Certificate2(cert.Export(X509ContentType.Pkcs12)); + } + + /// + /// Starts a loopback TLS server that accepts one connection, completes the handshake, + /// and then holds the connection open without ever writing an HTTP response. + /// + private static (TcpListener listener, int port) StartSilentTlsServer(X509Certificate2 cert, TimeSpan hold) + { + var listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + var port = ((IPEndPoint)listener.LocalEndpoint).Port; + + _ = Task.Run(async () => + { + try + { + using var tcpClient = await listener.AcceptTcpClientAsync().ConfigureAwait(false); + using var sslStream = new SslStream(tcpClient.GetStream(), leaveInnerStreamOpen: false); + await sslStream.AuthenticateAsServerAsync(cert, clientCertificateRequired: false, + checkCertificateRevocation: false).ConfigureAwait(false); + // Never read the request or write a response — the client is left waiting + // until its own HttpClient.Timeout fires, or until the test tears the + // listener down. + await Task.Delay(hold).ConfigureAwait(false); + } + catch + { + // The test disposes the listener once it has its result; ignore the resulting + // teardown exceptions on this background task. + } + }); + + return (listener, port); + } + + private static PveSession NewSessionWithTimeout(int port, TimeSpan sessionTimeout) + { + var session = new PveSession("127.0.0.1", port, skipCertificateCheck: true, + apiToken: "root@pam!test=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"); + session.Timeout = sessionTimeout; + return session; + } + + [Fact] + public async Task UploadOva_ExplicitTimeout_FiresBeforeSessionDefault() + { + using var cert = CreateSelfSignedServerCert(); + var (listener, port) = StartSilentTlsServer(cert, TimeSpan.FromSeconds(60)); + var tempFile = Path.GetTempFileName(); + try + { + // Session default is 100s (PveSession's own built-in default); the explicit + // override below must be what actually governs this call. + var session = NewSessionWithTimeout(port, TimeSpan.FromSeconds(100)); + var service = new VmService(); + + var sw = Stopwatch.StartNew(); + var ex = await Assert.ThrowsAsync(() => + Task.Run(() => service.UploadOva(session, "pve1", "local", tempFile, + timeout: TimeSpan.FromSeconds(1)))); + sw.Stop(); + + Assert.Equal(HttpStatusCode.RequestTimeout, ex.StatusCode); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(30), + $"expected the 1s override to fire, not the 100s session default; took {sw.Elapsed}"); + } + finally + { + listener.Stop(); + File.Delete(tempFile); + } + } + + [Fact] + public async Task UploadIso_ExplicitTimeout_FiresBeforeSessionDefault() + { + using var cert = CreateSelfSignedServerCert(); + var (listener, port) = StartSilentTlsServer(cert, TimeSpan.FromSeconds(60)); + var tempFile = Path.GetTempFileName(); + try + { + var session = NewSessionWithTimeout(port, TimeSpan.FromSeconds(100)); + var service = new StorageService(); + + var sw = Stopwatch.StartNew(); + var ex = await Assert.ThrowsAsync(() => + Task.Run(() => service.UploadIso(session, "pve1", "local", tempFile, + progressCallback: null, timeout: TimeSpan.FromSeconds(1)))); + sw.Stop(); + + Assert.Equal(HttpStatusCode.RequestTimeout, ex.StatusCode); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(30), + $"expected the 1s override to fire, not the 100s session default; took {sw.Elapsed}"); + } + finally + { + listener.Stop(); + File.Delete(tempFile); + } + } + + [Fact] + public async Task UploadOva_NoTimeoutGiven_UsesThirtyMinuteDefaultNotSessionTimeout() + { + using var cert = CreateSelfSignedServerCert(); + var (listener, port) = StartSilentTlsServer(cert, TimeSpan.FromSeconds(4)); + var tempFile = Path.GetTempFileName(); + Task? uploadTask = null; + try + { + // Session default is deliberately shorter than the wait window below. If + // UploadOva let the session's own timeout govern the request (the #139 bug), + // this would throw within that window instead of still being in flight. + var session = NewSessionWithTimeout(port, TimeSpan.FromMilliseconds(300)); + var service = new VmService(); + + uploadTask = Task.Run(() => service.UploadOva(session, "pve1", "local", tempFile)); + var completedFirst = await Task.WhenAny(uploadTask, Task.Delay(TimeSpan.FromSeconds(3))) + .ConfigureAwait(false); + + Assert.NotSame(uploadTask, completedFirst); + } + finally + { + listener.Stop(); + // The upload still holds tempFile open until the server drops the connection; + // Windows refuses to delete an open file, so wait for the task to end first. + if (uploadTask != null) + { + try { await uploadTask.ConfigureAwait(false); } + catch (Exception) { } + } + File.Delete(tempFile); + } + } + + [Fact] + public async Task UploadIso_NoTimeoutGiven_UsesThirtyMinuteDefaultNotSessionTimeout() + { + using var cert = CreateSelfSignedServerCert(); + var (listener, port) = StartSilentTlsServer(cert, TimeSpan.FromSeconds(4)); + var tempFile = Path.GetTempFileName(); + Task? uploadTask = null; + try + { + var session = NewSessionWithTimeout(port, TimeSpan.FromMilliseconds(300)); + var service = new StorageService(); + + uploadTask = Task.Run(() => service.UploadIso(session, "pve1", "local", tempFile)); + var completedFirst = await Task.WhenAny(uploadTask, Task.Delay(TimeSpan.FromSeconds(3))) + .ConfigureAwait(false); + + Assert.NotSame(uploadTask, completedFirst); + } + finally + { + listener.Stop(); + // The upload still holds tempFile open until the server drops the connection; + // Windows refuses to delete an open file, so wait for the task to end first. + if (uploadTask != null) + { + try { await uploadTask.ConfigureAwait(false); } + catch (Exception) { } + } + File.Delete(tempFile); + } + } + } +} diff --git a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs index d7f9e74..01ef2e8 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs @@ -272,6 +272,25 @@ namespace PSProxmoxVE.Core.Tests.Services Assert.Equal("305", captured!["newid"]); } + // ----------------------------------------------------------------- + // GetVm + // ----------------------------------------------------------------- + + [Fact] + public void GetVm_VmNotInNodeListing_ThrowsInvalidOperationException() + { + var mockClient = new Mock(); + mockClient + .Setup(c => c.GetAsync($"nodes/{TestNode}/qemu")) + .ReturnsAsync("{\"data\":[]}"); + + var service = new VmService(mockClient.Object); + + var ex = Assert.Throws( + () => service.GetVm(CreateSession(), TestNode, TestVmId)); + Assert.Contains(TestVmId.ToString(), ex.Message); + } + // --------------------------------------------------------------------- // GetVms multi-node aggregation: issue #142