fix: Import-PveOva's dead not-found catch and missing upload timeout (#180)

* 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>
2026-09-04 03:05:32 +00:00 · 2026-09-02 20:29:46 +00:00
parent 0ec75a02e7
commit 716ecd100d
6 changed files with 271 additions and 20 deletions
@@ -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
+{
+    /// <summary>
+    /// Proves that <see cref="VmService.UploadOva"/> and <see cref="StorageService.UploadIso"/>
+    /// apply their own <c>timeout</c> parameter to the <see cref="Client.PveHttpClient"/> they
+    /// construct, instead of always inheriting <see cref="PveSession.Timeout"/>. 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.
+    /// </summary>
+    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));
+        }
+
+        /// <summary>
+        /// Starts a loopback TLS server that accepts one connection, completes the handshake,
+        /// and then holds the connection open without ever writing an HTTP response.
+        /// </summary>
+        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<PveApiException>(() =>
+                    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<PveApiException>(() =>
+                    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);
+            }
+        }
+    }
+}
@@ -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<IPveHttpClient>();
+            mockClient
+                .Setup(c => c.GetAsync($"nodes/{TestNode}/qemu"))
+                .ReturnsAsync("{\"data\":[]}");
+
+            var service = new VmService(mockClient.Object);
+
+            var ex = Assert.Throws<InvalidOperationException>(
+                () => service.GetVm(CreateSession(), TestNode, TestVmId));
+            Assert.Contains(TestVmId.ToString(), ex.Message);
+        }
+

        // ---------------------------------------------------------------------
        // GetVms multi-node aggregation: issue #142