From a1d9550d83324eeb0d708f21ec6f172126c41695 Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Wed, 20 May 2026 18:17:31 -0500 Subject: [PATCH] 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); + } } } }