From f3b06171b2f150be4f16be86fb61bc8805ad93d1 Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Wed, 20 May 2026 18:08:26 -0500 Subject: [PATCH 1/3] fix: add -TimeoutSeconds for long-running HTTP calls PveHttpClient was constructed without setting HttpClient.Timeout, so .NET's 100s default applied to every request. Multi-GB ISO uploads via Send-PveFile on a real LAN reliably tripped this with TaskCanceledException after 100 seconds, and there was no way to override it. - PveSession gains a Timeout (TimeSpan) property, defaulting to 100s. - PveHttpClient accepts an optional per-instance timeout override that takes precedence over the session timeout. - Connect-PveServer exposes -TimeoutSeconds to set the session default. - Send-PveFile and Invoke-PveStorageDownload expose -TimeoutSeconds with a 30-minute implicit default so large uploads/downloads do not trip the 100s default. -TimeoutSeconds 0 means Timeout.InfiniteTimeSpan. Tracked as F087. Closes #59. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/review/findings.json | 35 ++++++++++ .../Authentication/PveAuthenticator.cs | 29 +++++++- .../Authentication/PveSession.cs | 7 ++ src/PSProxmoxVE.Core/Client/PveHttpClient.cs | 12 +++- .../Connection/ConnectPveServerCmdlet.cs | 22 +++++- .../Storage/InvokePveStorageDownloadCmdlet.cs | 24 ++++++- .../Cmdlets/Storage/SendPveFileCmdlet.cs | 22 +++++- .../Authentication/PveSessionTests.cs | 9 +++ .../Client/PveHttpClientTimeoutTests.cs | 68 +++++++++++++++++++ .../Connection/Connect-PveServer.Tests.ps1 | 12 ++++ .../Storage/Send-PveFile.Tests.ps1 | 14 ++++ .../Storage/StorageDownload.Tests.ps1 | 11 +++ 12 files changed, 256 insertions(+), 9 deletions(-) create mode 100644 tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientTimeoutTests.cs diff --git a/docs/review/findings.json b/docs/review/findings.json index bec7f9b..5a82291 100644 --- a/docs/review/findings.json +++ b/docs/review/findings.json @@ -2815,6 +2815,41 @@ "evidence": "Replaced JArray? Nodelist with List>? + NativeListConverter, and JObject? Totem with Dictionary? + NativeDictionaryConverter. Removed Newtonsoft.Json.Linq dependency.", "verified_by": "dotnet build + dotnet test (382 passed)" } + }, + { + "id": "F087", + "title": "HttpClient uses 100s default timeout; Send-PveFile and other long-running calls fail on large payloads", + "category": "reliability", + "severity": "high", + "status": "resolved", + "first_detected": "2026-05-20", + "github_issue": 59, + "files": [ + "src/PSProxmoxVE.Core/Client/PveHttpClient.cs", + "src/PSProxmoxVE.Core/Authentication/PveSession.cs", + "src/PSProxmoxVE.Core/Authentication/PveAuthenticator.cs", + "src/PSProxmoxVE/Cmdlets/Storage/SendPveFileCmdlet.cs", + "src/PSProxmoxVE/Cmdlets/Storage/InvokePveStorageDownloadCmdlet.cs", + "src/PSProxmoxVE/Cmdlets/Connection/ConnectPveServerCmdlet.cs" + ], + "description": "PveHttpClient constructs HttpClient without setting Timeout, so .NET's 100s default applies to every request. Send-PveFile, Invoke-PveStorageDownload, and Connect-PveServer expose no way to override it, so multi-GB ISO uploads on a real LAN reliably trip the 100s timeout with TaskCanceledException.", + "scan_history": [ + { + "scan_date": "2026-05-20", + "local_id": null, + "status": "new" + }, + { + "scan_date": "2026-05-20", + "local_id": null, + "status": "fixed" + } + ], + "resolution": { + "scan_date": "2026-05-20", + "evidence": "Added PveSession.Timeout (default 100s) and a TimeSpan? override on PveHttpClient. Connect-PveServer exposes -TimeoutSeconds to set the session-default; Send-PveFile and Invoke-PveStorageDownload expose -TimeoutSeconds with a 30-minute implicit default so large uploads/downloads do not trip the 100s HttpClient default. -TimeoutSeconds 0 means Timeout.InfiniteTimeSpan.", + "verified_by": "dotnet build + dotnet test (550 passed, 5 new timeout tests)" + } } ] } diff --git a/src/PSProxmoxVE.Core/Authentication/PveAuthenticator.cs b/src/PSProxmoxVE.Core/Authentication/PveAuthenticator.cs index 5bcbfa7..7436c02 100644 --- a/src/PSProxmoxVE.Core/Authentication/PveAuthenticator.cs +++ b/src/PSProxmoxVE.Core/Authentication/PveAuthenticator.cs @@ -18,12 +18,22 @@ namespace PSProxmoxVE.Core.Authentication /// Authenticates using username and password, obtaining a ticket and CSRF token. /// The username must be in the form user@realm (e.g. root@pam). /// + /// Hostname or IP address of the Proxmox VE server. + /// TCP port of the Proxmox VE API. + /// When true, skips TLS certificate validation. + /// Username including realm (e.g. root@pam). + /// Plain-text password for the user. + /// + /// Optional HTTP timeout to apply both to the authentication call and to subsequent + /// requests made with this session. When null, the default 100s applies. + /// public static PveSession AuthenticateWithCredentials( string hostname, int port, bool skipCertificateCheck, string username, - string password) + string password, + TimeSpan? timeout = null) { if (string.IsNullOrWhiteSpace(hostname)) throw new ArgumentException("Hostname cannot be null or empty.", nameof(hostname)); @@ -41,7 +51,7 @@ namespace PSProxmoxVE.Core.Authentication }; string responseBody; - using (var httpClient = new PveHttpClient(hostname, port, skipCertificateCheck)) + using (var httpClient = new PveHttpClient(hostname, port, skipCertificateCheck, timeout)) { responseBody = httpClient.Post("/api2/json/access/ticket", formData); } @@ -57,6 +67,8 @@ namespace PSProxmoxVE.Core.Authentication var ticketExpiry = DateTime.UtcNow.AddHours(2); var session = new PveSession(hostname, port, skipCertificateCheck, ticket, csrfToken, ticketExpiry); + if (timeout.HasValue) + session.Timeout = timeout.Value; session.ServerVersion = GetVersion(session); @@ -67,11 +79,20 @@ namespace PSProxmoxVE.Core.Authentication /// Authenticates using a Proxmox VE API token. /// The token must be in the format USER@REALM!TOKENID=UUID (e.g. root@pam!mytoken=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx). /// + /// Hostname or IP address of the Proxmox VE server. + /// TCP port of the Proxmox VE API. + /// When true, skips TLS certificate validation. + /// API token in USER@REALM!TOKENID=UUID format. + /// + /// Optional HTTP timeout to apply to requests made with this session. + /// When null, the default 100s applies. + /// public static PveSession AuthenticateWithApiToken( string hostname, int port, bool skipCertificateCheck, - string apiToken) + string apiToken, + TimeSpan? timeout = null) { if (string.IsNullOrWhiteSpace(hostname)) throw new ArgumentException("Hostname cannot be null or empty.", nameof(hostname)); @@ -83,6 +104,8 @@ namespace PSProxmoxVE.Core.Authentication nameof(apiToken)); var session = new PveSession(hostname, port, skipCertificateCheck, apiToken); + if (timeout.HasValue) + session.Timeout = timeout.Value; session.ServerVersion = GetVersion(session); diff --git a/src/PSProxmoxVE.Core/Authentication/PveSession.cs b/src/PSProxmoxVE.Core/Authentication/PveSession.cs index 80bb098..10c3f6e 100644 --- a/src/PSProxmoxVE.Core/Authentication/PveSession.cs +++ b/src/PSProxmoxVE.Core/Authentication/PveSession.cs @@ -33,6 +33,13 @@ namespace PSProxmoxVE.Core.Authentication /// The Proxmox VE version detected on the server at connection time. public PveVersion? ServerVersion { get; internal set; } + /// + /// The default HTTP request timeout applied to clients created with this session. + /// Defaults to 100 seconds (HttpClient's built-in default). Cmdlets that perform + /// long-running operations (e.g. Send-PveFile) may override this per-call. + /// + public TimeSpan Timeout { get; internal set; } = TimeSpan.FromSeconds(100); + /// Returns true if the ticket has expired (only relevant for Ticket auth mode) public bool IsExpired { diff --git a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs index 84d1209..e9d4145 100644 --- a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs +++ b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs @@ -37,7 +37,12 @@ namespace PSProxmoxVE.Core.Client /// Creates an HTTP client authenticated with the specified PVE session. /// /// The authenticated PVE session providing credentials and base URL. - public PveHttpClient(PveSession session) + /// + /// Optional per-instance timeout override. When supplied, takes precedence over + /// . Pass + /// to disable the timeout entirely (useful for multi-GB uploads/downloads). + /// + public PveHttpClient(PveSession session, TimeSpan? timeoutOverride = null) { _session = session ?? throw new ArgumentNullException(nameof(session)); _baseUrl = session.BaseUrl; @@ -49,6 +54,7 @@ namespace PSProxmoxVE.Core.Client (HttpRequestMessage _, X509Certificate2 _, X509Chain _, SslPolicyErrors _) => true; } _httpClient = new HttpClient(handler); + _httpClient.Timeout = timeoutOverride ?? session.Timeout; _httpClient.DefaultRequestHeaders.Accept.Add( new MediaTypeWithQualityHeaderValue("application/json")); @@ -58,7 +64,7 @@ namespace PSProxmoxVE.Core.Client /// Creates a bare HTTP client for pre-session use (e.g. initial authentication). /// No auth headers are added to requests made with this constructor. /// - internal PveHttpClient(string hostname, int port, bool skipCertificateCheck) + internal PveHttpClient(string hostname, int port, bool skipCertificateCheck, TimeSpan? timeout = null) { if (string.IsNullOrWhiteSpace(hostname)) throw new ArgumentException("Hostname cannot be null or empty.", nameof(hostname)); @@ -73,6 +79,8 @@ namespace PSProxmoxVE.Core.Client (HttpRequestMessage _, X509Certificate2 _, X509Chain _, SslPolicyErrors _) => true; } _httpClient = new HttpClient(handler); + if (timeout.HasValue) + _httpClient.Timeout = timeout.Value; _httpClient.DefaultRequestHeaders.Accept.Add( new MediaTypeWithQualityHeaderValue("application/json")); diff --git a/src/PSProxmoxVE/Cmdlets/Connection/ConnectPveServerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Connection/ConnectPveServerCmdlet.cs index f311b49..83b6210 100644 --- a/src/PSProxmoxVE/Cmdlets/Connection/ConnectPveServerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Connection/ConnectPveServerCmdlet.cs @@ -48,6 +48,17 @@ namespace PSProxmoxVE.Cmdlets.Connection [Parameter(Mandatory = false, HelpMessage = "Skip TLS certificate validation.")] public SwitchParameter SkipCertificateCheck { get; set; } + /// + /// HTTP request timeout in seconds for all calls made with this session. + /// Defaults to 100 seconds (HttpClient's built-in default). Pass 0 to disable + /// the timeout entirely. Cmdlets that perform long-running operations + /// (Send-PveFile, Invoke-PveStorageDownload) accept their own -TimeoutSeconds + /// that overrides this value per-call. + /// + [Parameter(Mandatory = false, HelpMessage = "HTTP timeout in seconds (0 = infinite). Default 100s.")] + [ValidateRange(0, int.MaxValue)] + public int? TimeoutSeconds { get; set; } + /// Deprecated — session is now always output. Kept for backwards compatibility. [Parameter(Mandatory = false, DontShow = true)] public SwitchParameter PassThru { get; set; } @@ -59,6 +70,13 @@ namespace PSProxmoxVE.Cmdlets.Connection protected override void ProcessRecord() { PveSession session; + TimeSpan? timeout = null; + if (TimeoutSeconds.HasValue) + { + timeout = TimeoutSeconds.Value == 0 + ? System.Threading.Timeout.InfiniteTimeSpan + : TimeSpan.FromSeconds(TimeoutSeconds.Value); + } switch (ParameterSetName) { @@ -77,7 +95,7 @@ namespace PSProxmoxVE.Cmdlets.Connection try { session = PveAuthenticator.AuthenticateWithCredentials( - Server, Port, SkipCertificateCheck.IsPresent, username, password); + Server, Port, SkipCertificateCheck.IsPresent, username, password, timeout); } catch (Exception ex) { @@ -96,7 +114,7 @@ namespace PSProxmoxVE.Cmdlets.Connection try { session = PveAuthenticator.AuthenticateWithApiToken( - Server, Port, SkipCertificateCheck.IsPresent, ApiToken!); + Server, Port, SkipCertificateCheck.IsPresent, ApiToken!, timeout); } catch (Exception ex) { diff --git a/src/PSProxmoxVE/Cmdlets/Storage/InvokePveStorageDownloadCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Storage/InvokePveStorageDownloadCmdlet.cs index 9637391..1ee3259 100644 --- a/src/PSProxmoxVE/Cmdlets/Storage/InvokePveStorageDownloadCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Storage/InvokePveStorageDownloadCmdlet.cs @@ -44,6 +44,16 @@ namespace PSProxmoxVE.Cmdlets.Storage [Parameter(Mandatory = false, HelpMessage = "Wait for the task to complete before returning.")] public SwitchParameter Wait { get; set; } + /// + /// HTTP timeout for issuing the download request, in seconds. Pass 0 for infinite. + /// When omitted, defaults to 30 minutes. Note: this only bounds the API call that + /// schedules the download; the actual file transfer runs server-side and is tracked + /// by the returned task. + /// + [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() { if (!ShouldProcess($"{Node}/{Storage}/{Filename}", $"Download from {Url}")) @@ -51,7 +61,19 @@ namespace PSProxmoxVE.Cmdlets.Storage var session = GetSession(); RequireVersion(session, "Storage URL download", 7, 0); - using var client = new PveHttpClient(session); + + TimeSpan timeout; + if (TimeoutSeconds.HasValue) + { + timeout = TimeoutSeconds.Value == 0 + ? System.Threading.Timeout.InfiniteTimeSpan + : TimeSpan.FromSeconds(TimeoutSeconds.Value); + } + else + { + timeout = TimeSpan.FromMinutes(30); + } + using var client = new PveHttpClient(session, timeout); WriteVerbose($"Downloading '{Url}' to {Node}/{Storage}..."); var resource = $"nodes/{Uri.EscapeDataString(Node)}/storage/{Uri.EscapeDataString(Storage)}/download-url"; diff --git a/src/PSProxmoxVE/Cmdlets/Storage/SendPveFileCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Storage/SendPveFileCmdlet.cs index 776d2e5..a2d4431 100644 --- a/src/PSProxmoxVE/Cmdlets/Storage/SendPveFileCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Storage/SendPveFileCmdlet.cs @@ -60,6 +60,15 @@ namespace PSProxmoxVE.Cmdlets.Storage [Parameter(Mandatory = false, HelpMessage = "Wait for the task to complete before returning.")] public SwitchParameter Wait { get; set; } + /// + /// HTTP timeout for this upload, in seconds. Pass 0 for infinite (no timeout). + /// When omitted, defaults to 30 minutes — overriding the session timeout so that + /// large file 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() { var fileName = System.IO.Path.GetFileName(Path); @@ -75,7 +84,18 @@ namespace PSProxmoxVE.Cmdlets.Storage + $"Connected server is PVE {session.ServerVersion}. The upload will proceed without checksum verification."); } - using var client = new PveHttpClient(session); + TimeSpan timeout; + if (TimeoutSeconds.HasValue) + { + timeout = TimeoutSeconds.Value == 0 + ? System.Threading.Timeout.InfiniteTimeSpan + : TimeSpan.FromSeconds(TimeoutSeconds.Value); + } + else + { + timeout = TimeSpan.FromMinutes(30); + } + using var client = new PveHttpClient(session, timeout); WriteVerbose($"Uploading {fileName} to {Node}/{Storage} (content={ContentType})..."); var resource = $"nodes/{Uri.EscapeDataString(Node)}/storage/{Uri.EscapeDataString(Storage)}/upload"; diff --git a/tests/PSProxmoxVE.Core.Tests/Authentication/PveSessionTests.cs b/tests/PSProxmoxVE.Core.Tests/Authentication/PveSessionTests.cs index 819a264..cd70cd9 100644 --- a/tests/PSProxmoxVE.Core.Tests/Authentication/PveSessionTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Authentication/PveSessionTests.cs @@ -100,5 +100,14 @@ namespace PSProxmoxVE.Core.Tests.Authentication Assert.True(session.SkipCertificateCheck); } + + [Fact] + public void Timeout_DefaultIs100Seconds() + { + var session = new PveSession(TestHostname, TestPort, false, + "root@pam!mytoken=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"); + + Assert.Equal(TimeSpan.FromSeconds(100), session.Timeout); + } } } diff --git a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientTimeoutTests.cs b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientTimeoutTests.cs new file mode 100644 index 0000000..ae0d9dc --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientTimeoutTests.cs @@ -0,0 +1,68 @@ +using System; +using System.Net.Http; +using System.Reflection; +using System.Threading; +using PSProxmoxVE.Core.Authentication; +using PSProxmoxVE.Core.Client; +using Xunit; + +namespace PSProxmoxVE.Core.Tests.Client +{ + public class PveHttpClientTimeoutTests + { + private static HttpClient GetInnerHttpClient(PveHttpClient client) + { + var field = typeof(PveHttpClient).GetField("_httpClient", + BindingFlags.Instance | BindingFlags.NonPublic)!; + return (HttpClient)field.GetValue(client)!; + } + + private static PveSession NewSession() + { + return new PveSession("pve.example.com", 8006, false, + "root@pam!token=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"); + } + + [Fact] + public void Constructor_UsesSessionTimeoutByDefault() + { + var session = NewSession(); + session.Timeout = TimeSpan.FromSeconds(42); + + using var client = new PveHttpClient(session); + + Assert.Equal(TimeSpan.FromSeconds(42), GetInnerHttpClient(client).Timeout); + } + + [Fact] + public void Constructor_OverrideTakesPrecedenceOverSessionTimeout() + { + var session = NewSession(); + session.Timeout = TimeSpan.FromSeconds(42); + + using var client = new PveHttpClient(session, TimeSpan.FromMinutes(30)); + + Assert.Equal(TimeSpan.FromMinutes(30), GetInnerHttpClient(client).Timeout); + } + + [Fact] + public void Constructor_InfiniteTimeSpanIsAccepted() + { + var session = NewSession(); + + using var client = new PveHttpClient(session, Timeout.InfiniteTimeSpan); + + Assert.Equal(Timeout.InfiniteTimeSpan, GetInnerHttpClient(client).Timeout); + } + + [Fact] + public void DefaultSessionTimeoutIs100Seconds() + { + var session = NewSession(); + + using var client = new PveHttpClient(session); + + Assert.Equal(TimeSpan.FromSeconds(100), GetInnerHttpClient(client).Timeout); + } + } +} diff --git a/tests/PSProxmoxVE.Tests/Connection/Connect-PveServer.Tests.ps1 b/tests/PSProxmoxVE.Tests/Connection/Connect-PveServer.Tests.ps1 index 7c0c13e..68848d0 100644 --- a/tests/PSProxmoxVE.Tests/Connection/Connect-PveServer.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Connection/Connect-PveServer.Tests.ps1 @@ -112,5 +112,17 @@ Describe 'Connect-PveServer' { $overlap = $credSets | Where-Object { $tokenSets -contains $_ } $overlap | Should -BeNullOrEmpty } + + It 'Should have a TimeoutSeconds parameter' { + $script:Cmd.Parameters.ContainsKey('TimeoutSeconds') | Should -BeTrue + } + + It 'TimeoutSeconds should reject negative values' { + $securePass = ConvertTo-SecureString 'hunter2' -AsPlainText -Force + $cred = [System.Management.Automation.PSCredential]::new('root@pam', $securePass) + { + Connect-PveServer -Server 'pve.example.com' -Credential $cred -TimeoutSeconds -1 -ErrorAction Stop + } | Should -Throw + } } } diff --git a/tests/PSProxmoxVE.Tests/Storage/Send-PveFile.Tests.ps1 b/tests/PSProxmoxVE.Tests/Storage/Send-PveFile.Tests.ps1 index fbe7984..d729ca2 100644 --- a/tests/PSProxmoxVE.Tests/Storage/Send-PveFile.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Storage/Send-PveFile.Tests.ps1 @@ -154,6 +154,20 @@ Describe 'Send-PveFile' { if (-not $script:CmdExists) { Set-ItResult -Skipped -Because 'Not yet compiled'; return } $script:Cmd.Parameters.ContainsKey('Session') | Should -BeTrue } + + It 'Should have TimeoutSeconds parameter' { + if (-not $script:CmdExists) { Set-ItResult -Skipped -Because 'Not yet compiled'; return } + $script:Cmd.Parameters.ContainsKey('TimeoutSeconds') | Should -BeTrue + } + + It 'TimeoutSeconds should reject negative values' { + if (-not $script:CmdExists) { Set-ItResult -Skipped -Because 'Not yet compiled'; return } + $tmpIso = [System.IO.Path]::GetTempFileName() + try { + { Send-PveFile -Node 'n' -Storage 's' -Path $tmpIso -TimeoutSeconds -1 -Confirm:$false -ErrorAction Stop } | + Should -Throw + } finally { Remove-Item $tmpIso -ErrorAction SilentlyContinue } + } } Context 'Without active session' { diff --git a/tests/PSProxmoxVE.Tests/Storage/StorageDownload.Tests.ps1 b/tests/PSProxmoxVE.Tests/Storage/StorageDownload.Tests.ps1 index 1a49b2a..4d17b59 100644 --- a/tests/PSProxmoxVE.Tests/Storage/StorageDownload.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Storage/StorageDownload.Tests.ps1 @@ -150,6 +150,17 @@ Describe 'Invoke-PveStorageDownload' { $script:Cmd.Parameters.ContainsKey('Wait') | Should -BeTrue $script:Cmd.Parameters['Wait'].SwitchParameter | Should -BeTrue } + + It 'Should have TimeoutSeconds parameter' { + Skip-IfMissing 'Invoke-PveStorageDownload' + $script:Cmd.Parameters.ContainsKey('TimeoutSeconds') | Should -BeTrue + } + + It 'TimeoutSeconds should reject negative values' { + Skip-IfMissing 'Invoke-PveStorageDownload' + { Invoke-PveStorageDownload -Node 'pve1' -Storage 'local' -Url 'https://example.com/test.iso' -Filename 'test.iso' -TimeoutSeconds -1 -Confirm:$false -ErrorAction Stop } | + Should -Throw + } } Context 'Session parameter' { From a1d9550d83324eeb0d708f21ec6f172126c41695 Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Wed, 20 May 2026 18:17:31 -0500 Subject: [PATCH 2/3] fix: surface HttpClient timeouts as PveApiException(RequestTimeout) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When HttpClient.Timeout elapses, .NET throws TaskCanceledException — not HttpRequestException — so the existing catch in PveHttpClient.SendAsync missed it and callers got a raw stack trace. With -TimeoutSeconds now configurable and documented, this gap became user-visible. In .NET 5+ HttpClient surfaces transport timeouts as TaskCanceledException with a TimeoutException inner; user-driven token cancellation does not. Catch by that inner-type signature and rethrow as PveApiException with HttpStatusCode.RequestTimeout, the resource path, and a message that reports the configured timeout. Adds SendAsync_TimeoutFires_ThrowsPveApiExceptionWithRequestTimeout which swaps in a delaying HttpMessageHandler with a 50ms timeout to exercise the path deterministically. Drops the redundant DefaultSessionTimeoutIs100Seconds test (covered by PveSessionTests.Timeout_DefaultIs100Seconds and the existing flow-through test). Addresses PR #61 review feedback. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/PSProxmoxVE.Core/Client/PveHttpClient.cs | 11 ++++++ .../Client/PveHttpClientTimeoutTests.cs | 39 +++++++++++++++++-- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs index e9d4145..7edc624 100644 --- a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs +++ b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs @@ -353,6 +353,17 @@ namespace PSProxmoxVE.Core.Client { response = await _httpClient.SendAsync(request).ConfigureAwait(false); } + catch (TaskCanceledException ex) when (ex.InnerException is TimeoutException) + { + // HttpClient.Timeout elapsed. In .NET 5+, HttpClient surfaces transport + // timeouts as TaskCanceledException with a TimeoutException inner — + // distinguishing them from caller-driven CancellationToken cancellation. + var seconds = _httpClient.Timeout == System.Threading.Timeout.InfiniteTimeSpan + ? "infinite" + : _httpClient.Timeout.TotalSeconds.ToString("0", System.Globalization.CultureInfo.InvariantCulture) + "s"; + throw new PveApiException(HttpStatusCode.RequestTimeout, + $"Request timed out after {seconds}.", resource, httpMethod, ex); + } catch (HttpRequestException ex) { throw new PveApiException(HttpStatusCode.ServiceUnavailable, diff --git a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientTimeoutTests.cs b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientTimeoutTests.cs index ae0d9dc..68200a8 100644 --- a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientTimeoutTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientTimeoutTests.cs @@ -1,9 +1,12 @@ using System; +using System.Net; using System.Net.Http; using System.Reflection; using System.Threading; +using System.Threading.Tasks; using PSProxmoxVE.Core.Authentication; using PSProxmoxVE.Core.Client; +using PSProxmoxVE.Core.Exceptions; using Xunit; namespace PSProxmoxVE.Core.Tests.Client @@ -17,6 +20,14 @@ namespace PSProxmoxVE.Core.Tests.Client return (HttpClient)field.GetValue(client)!; } + private static void SetInnerHttpClient(PveHttpClient client, HttpClient newInner) + { + var field = typeof(PveHttpClient).GetField("_httpClient", + BindingFlags.Instance | BindingFlags.NonPublic)!; + ((HttpClient)field.GetValue(client)!).Dispose(); + field.SetValue(client, newInner); + } + private static PveSession NewSession() { return new PveSession("pve.example.com", 8006, false, @@ -56,13 +67,35 @@ namespace PSProxmoxVE.Core.Tests.Client } [Fact] - public void DefaultSessionTimeoutIs100Seconds() + public async Task SendAsync_TimeoutFires_ThrowsPveApiExceptionWithRequestTimeout() { var session = NewSession(); - using var client = new PveHttpClient(session); - Assert.Equal(TimeSpan.FromSeconds(100), GetInnerHttpClient(client).Timeout); + // Swap in an HttpClient with a delaying handler and a 50ms timeout so + // HttpClient.Timeout fires reliably without any real network. + var delayingClient = new HttpClient(new DelayingHandler(TimeSpan.FromSeconds(30))) + { + Timeout = TimeSpan.FromMilliseconds(50) + }; + SetInnerHttpClient(client, delayingClient); + + var ex = await Assert.ThrowsAsync(() => client.GetAsync("version")); + Assert.Equal(HttpStatusCode.RequestTimeout, ex.StatusCode); + Assert.Contains("timed out", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + private sealed class DelayingHandler : HttpMessageHandler + { + private readonly TimeSpan _delay; + public DelayingHandler(TimeSpan delay) { _delay = delay; } + + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + await Task.Delay(_delay, cancellationToken).ConfigureAwait(false); + return new HttpResponseMessage(HttpStatusCode.OK); + } } } } From cb97a86c9f715f9bc3eb1146a049113a11d8eaa4 Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Wed, 20 May 2026 18:22:47 -0500 Subject: [PATCH 3/3] fix: drop net48-incompatible TimeoutException filter in timeout catch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The when filter (ex.InnerException is TimeoutException) only matches on .NET 5+. On .NET Framework 4.8 — which CI exercises via the test project's net48 target — HttpClient.Timeout throws a bare TaskCanceledException with no inner exception, so CI failed: Expected: typeof(PSProxmoxVE.Core.Exceptions.PveApiException) Actual: typeof(System.Threading.Tasks.TaskCanceledException) ---- System.Threading.Tasks.TaskCanceledException : A task was canceled. PveHttpClient.SendAsync never passes a CancellationToken to the inner HttpClient.SendAsync, so the only way a TaskCanceledException can reach this catch is HttpClient.Timeout firing — true on net48, .NET Core, and .NET 5+. Drop the filter and wrap unconditionally. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/PSProxmoxVE.Core/Client/PveHttpClient.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs index 7edc624..3729b10 100644 --- a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs +++ b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs @@ -353,11 +353,12 @@ namespace PSProxmoxVE.Core.Client { response = await _httpClient.SendAsync(request).ConfigureAwait(false); } - catch (TaskCanceledException ex) when (ex.InnerException is TimeoutException) + catch (TaskCanceledException ex) { - // HttpClient.Timeout elapsed. In .NET 5+, HttpClient surfaces transport - // timeouts as TaskCanceledException with a TimeoutException inner — - // distinguishing them from caller-driven CancellationToken cancellation. + // PveHttpClient.SendAsync passes no CancellationToken to HttpClient.SendAsync, + // so a TaskCanceledException reaching here can only be HttpClient.Timeout + // firing — on .NET Framework, on .NET Core, and on .NET 5+ (where it also + // carries a TimeoutException inner). Wrap it uniformly across frameworks. var seconds = _httpClient.Timeout == System.Threading.Timeout.InfiniteTimeSpan ? "infinite" : _httpClient.Timeout.TotalSeconds.ToString("0", System.Globalization.CultureInfo.InvariantCulture) + "s";