diff --git a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs index e5fdcf6..2374b50 100644 --- a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs +++ b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs @@ -30,7 +30,8 @@ namespace PSProxmoxVE.Core.Client private readonly HttpClient _httpClient; private bool _disposed; - private TimeSpan _guestLockRetryWindow = GuestLockRetry.DefaultWindow; + private readonly TimeSpan _guestLockRetryWindow; + private readonly Func _guestLockRetryDelay; private const string ApiTokenPrefix = "PVEAPIToken="; private const string AuthCookieName = "PVEAuthCookie="; @@ -46,23 +47,58 @@ namespace PSProxmoxVE.Core.Client /// to disable the timeout entirely (useful for multi-GB uploads/downloads). /// public PveHttpClient(PveSession session, TimeSpan? timeoutOverride = null) + : this(session, timeoutOverride, guestLockRetryWindow: null, handler: null, guestLockRetryDelay: null) + { + } + + /// + /// Test seam: builds a client against an explicit handler, lock-retry window and/or + /// inter-attempt delay. Production code always goes through the public constructor. + /// + /// The authenticated PVE session providing credentials and base URL. + /// Optional per-instance timeout override. + /// + /// Retry budget passed to . + /// Null uses , the same as the public constructor. + /// + /// + /// Message handler to send requests through. Null builds the production + /// certificate-validation handler from . + /// + /// + /// Invoked before each guest-lock reissue instead of sleeping. Null uses + /// , the same as the public constructor. + /// + internal PveHttpClient( + PveSession session, + TimeSpan? timeoutOverride, + TimeSpan? guestLockRetryWindow, + HttpMessageHandler? handler, + Func? guestLockRetryDelay = null) { _session = session ?? throw new ArgumentNullException(nameof(session)); _baseUrl = session.BaseUrl; + _guestLockRetryWindow = guestLockRetryWindow ?? GuestLockRetry.DefaultWindow; + _guestLockRetryDelay = guestLockRetryDelay ?? Task.Delay; - var handler = new HttpClientHandler(); - if (session.SkipCertificateCheck) - { - handler.ServerCertificateCustomValidationCallback = - (HttpRequestMessage _, X509Certificate2 _, X509Chain _, SslPolicyErrors _) => true; - } - _httpClient = new HttpClient(handler); + _httpClient = new HttpClient(handler ?? CreateHandler(session.SkipCertificateCheck)); _httpClient.Timeout = timeoutOverride ?? session.Timeout; _httpClient.DefaultRequestHeaders.Accept.Add( new MediaTypeWithQualityHeaderValue("application/json")); } + private static HttpClientHandler CreateHandler(bool skipCertificateCheck) + { + var handler = new HttpClientHandler(); + if (skipCertificateCheck) + { + handler.ServerCertificateCustomValidationCallback = + (HttpRequestMessage _, X509Certificate2 _, X509Chain _, SslPolicyErrors _) => true; + } + return handler; + } + /// /// Creates a bare HTTP client for pre-session use (e.g. initial authentication). /// Requests it builds carry no authentication headers. @@ -74,14 +110,10 @@ namespace PSProxmoxVE.Core.Client _session = null; _baseUrl = $"https://{hostname}:{port}"; + _guestLockRetryWindow = GuestLockRetry.DefaultWindow; + _guestLockRetryDelay = Task.Delay; - var handler = new HttpClientHandler(); - if (skipCertificateCheck) - { - handler.ServerCertificateCustomValidationCallback = - (HttpRequestMessage _, X509Certificate2 _, X509Chain _, SslPolicyErrors _) => true; - } - _httpClient = new HttpClient(handler); + _httpClient = new HttpClient(CreateHandler(skipCertificateCheck)); if (timeout.HasValue) _httpClient.Timeout = timeout.Value; @@ -385,7 +417,7 @@ namespace PSProxmoxVE.Core.Client /// private Task SendAsync(Func buildRequest, string resource, string httpMethod) => GuestLockRetry.ExecuteAsync( - () => SendOnceAsync(buildRequest(), resource, httpMethod), _guestLockRetryWindow); + () => SendOnceAsync(buildRequest(), resource, httpMethod), _guestLockRetryWindow, _guestLockRetryDelay); private async Task SendOnceAsync(HttpRequestMessage request, string resource, string httpMethod) { diff --git a/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs b/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs index 39b8251..d27eb1f 100644 --- a/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs +++ b/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs @@ -94,9 +94,21 @@ namespace PSProxmoxVE.Core.Utilities /// Asynchronous counterpart of . /// The operation to run. /// Retry budget. Defaults to . - public static async Task ExecuteAsync(Func> operation, TimeSpan? window = null) + public static Task ExecuteAsync(Func> operation, TimeSpan? window = null) => + ExecuteAsync(operation, window, Task.Delay); + + /// + /// Test seam: same as but with the + /// inter-attempt wait replaceable, so a test can assert retry counts without paying the + /// wall-clock cost of . + /// + /// The operation to run. + /// Retry budget. Defaults to . + /// Invoked with the computed retry interval before each reissue. + internal static async Task ExecuteAsync(Func> operation, TimeSpan? window, Func delay) { if (operation == null) throw new ArgumentNullException(nameof(operation)); + if (delay == null) throw new ArgumentNullException(nameof(delay)); var budget = window ?? DefaultWindow; var elapsed = Stopwatch.StartNew(); @@ -108,7 +120,7 @@ namespace PSProxmoxVE.Core.Utilities } catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget) { - await Task.Delay(RetryInterval(budget)).ConfigureAwait(false); + await delay(RetryInterval(budget)).ConfigureAwait(false); } } } diff --git a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs index 1ec8e7f..3a03564 100644 --- a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs @@ -2,7 +2,6 @@ using System; using System.Collections.Generic; using System.Net; using System.Net.Http; -using System.Reflection; using System.Threading; using System.Threading.Tasks; using PSProxmoxVE.Core.Authentication; @@ -17,32 +16,22 @@ namespace PSProxmoxVE.Core.Tests.Client private const string LockTimeoutBody = "{\"message\":\"can't lock file '/var/lock/qemu-server/lock-100.conf' - got timeout\"}"; - 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); - } - - // The gap between attempts scales with the budget, so a short window keeps these - // tests off the 2s production sleep. - private static void SetRetryWindow(PveHttpClient client, TimeSpan window) - { - var field = typeof(PveHttpClient).GetField("_guestLockRetryWindow", - BindingFlags.Instance | BindingFlags.NonPublic)!; - field.SetValue(client, window); - } + // A no-op delay removes the retry loop's inter-attempt wait entirely, so these tests + // carry no wall-clock dependence: the production 45s window is exhausted only if the + // scripted responses themselves never resolve the lock, never by runner speed. + private static Task NoDelay(TimeSpan _) => Task.CompletedTask; private static (PveHttpClient client, ScriptedHandler handler) NewClient( - params (HttpStatusCode status, string body)[] responses) + params (HttpStatusCode status, string body)[] responses) => + NewClient(window: null, responses); + + private static (PveHttpClient client, ScriptedHandler handler) NewClient( + TimeSpan? window, params (HttpStatusCode status, string body)[] responses) { var session = new PveSession("pve.example.com", 8006, false, "root@pam!token=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"); - var client = new PveHttpClient(session); var handler = new ScriptedHandler(responses); - SetInnerHttpClient(client, new HttpClient(handler)); - SetRetryWindow(client, TimeSpan.FromMilliseconds(400)); + var client = new PveHttpClient(session, timeoutOverride: null, window, handler, NoDelay); return (client, handler); } @@ -66,6 +55,22 @@ namespace PSProxmoxVE.Core.Tests.Client Assert.Equal(3, handler.Bodies.Count); } + [Fact] + public async Task PutAsync_DoesNotReissueWhenTheRetryWindowIsAlreadySpent() + { + var (client, handler) = NewClient(TimeSpan.Zero, + (HttpStatusCode.InternalServerError, LockTimeoutBody), + (HttpStatusCode.OK, "{\"data\":null}")); + + using (client) + { + await Assert.ThrowsAsync( + () => client.PutAsync("nodes/pve9a/qemu/100/config", ConfigBody())); + } + + Assert.Single(handler.Bodies); + } + [Fact] public async Task PutAsync_RebuildsTheRequestSoEveryAttemptCarriesTheSameBody() { diff --git a/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs b/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs index 61e77c9..1f22ed9 100644 --- a/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Net; using System.Threading.Tasks; using PSProxmoxVE.Core.Exceptions; @@ -166,6 +167,50 @@ namespace PSProxmoxVE.Core.Tests.Utilities Assert.Equal(1, attempts); } + [Fact] + public async Task ExecuteAsync_InvokesTheInjectedDelayOncePerRetryWithTheComputedInterval() + { + var attempts = 0; + var delays = new List(); + + var result = await GuestLockRetry.ExecuteAsync(() => + { + attempts++; + if (attempts < 3) throw ApiError(VmLockError); + return Task.FromResult("written"); + }, GuestLockRetry.DefaultWindow, delay => + { + delays.Add(delay); + return Task.CompletedTask; + }); + + Assert.Equal("written", result); + // budget/4 capped at 2s; DefaultWindow (45s) puts the quarter above the cap. + Assert.Equal(new[] { TimeSpan.FromSeconds(2), TimeSpan.FromSeconds(2) }, delays); + } + + [Fact] + public async Task ExecuteAsync_PublicOverloadActuallyWaitsBetweenAttempts() + { + var attempts = 0; + var window = TimeSpan.FromMilliseconds(200); + var elapsed = Stopwatch.StartNew(); + + await GuestLockRetry.ExecuteAsync(() => + { + attempts++; + if (attempts < 2) throw ApiError(VmLockError); + return Task.FromResult("written"); + }, window); + + elapsed.Stop(); + // quarter of 200ms is 50ms; a no-op delay would leave this near zero. The + // threshold is well under 50ms so Task.Delay's own timer slop (it can return + // a fraction of a millisecond early) never makes this assertion itself flaky. + Assert.True(elapsed.Elapsed >= TimeSpan.FromMilliseconds(20), + $"Expected the public overload to wait close to one retry interval; took {elapsed.Elapsed}."); + } + [Fact] public void Execute_GivesUpAndRethrowsOnceTheWindowElapses() {