diff --git a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs index 337e43f..e5fdcf6 100644 --- a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs +++ b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs @@ -30,6 +30,8 @@ namespace PSProxmoxVE.Core.Client private readonly HttpClient _httpClient; private bool _disposed; + private TimeSpan _guestLockRetryWindow = GuestLockRetry.DefaultWindow; + private const string ApiTokenPrefix = "PVEAPIToken="; private const string AuthCookieName = "PVEAuthCookie="; private const string CsrfHeaderName = "CSRFPreventionToken"; @@ -382,7 +384,8 @@ namespace PSProxmoxVE.Core.Client /// cannot be resent, which is why this takes a factory rather than a request. /// private Task SendAsync(Func buildRequest, string resource, string httpMethod) => - GuestLockRetry.ExecuteAsync(() => SendOnceAsync(buildRequest(), resource, httpMethod)); + GuestLockRetry.ExecuteAsync( + () => SendOnceAsync(buildRequest(), resource, httpMethod), _guestLockRetryWindow); 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 e10fc9a..39b8251 100644 --- a/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs +++ b/src/PSProxmoxVE.Core/Utilities/GuestLockRetry.cs @@ -24,7 +24,15 @@ namespace PSProxmoxVE.Core.Utilities /// public static readonly TimeSpan DefaultWindow = TimeSpan.FromSeconds(45); - private static readonly TimeSpan RetryInterval = TimeSpan.FromSeconds(2); + private static readonly TimeSpan MaxRetryInterval = TimeSpan.FromSeconds(2); + + // The gap must never eat a meaningful share of a short budget: a caller passing a + // small window wants a fast answer, not one long sleep. + private static TimeSpan RetryInterval(TimeSpan budget) + { + var quarter = TimeSpan.FromMilliseconds(budget.TotalMilliseconds / 4); + return quarter < MaxRetryInterval ? quarter : MaxRetryInterval; + } // Anchored, and specific to the two guest lock paths. `PVE::Tools::lock_file` emits this // same wording for storage, LVM, HA and firewall locks, none of which carry the @@ -59,7 +67,11 @@ namespace PSProxmoxVE.Core.Utilities /// /// The operation to run. /// Retry budget. Defaults to . - public static T Execute(Func operation, TimeSpan? window = null) + /// + /// Invoked with the rejection before each reissue. A caller with somewhere to report + /// progress should pass one — a wait this long is otherwise indistinguishable from a hang. + /// + public static T Execute(Func operation, TimeSpan? window = null, Action? onRetry = null) { if (operation == null) throw new ArgumentNullException(nameof(operation)); @@ -73,7 +85,8 @@ namespace PSProxmoxVE.Core.Utilities } catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget) { - Thread.Sleep(RetryInterval); + onRetry?.Invoke(ex); + Thread.Sleep(RetryInterval(budget)); } } } @@ -95,7 +108,7 @@ namespace PSProxmoxVE.Core.Utilities } catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget) { - await Task.Delay(RetryInterval).ConfigureAwait(false); + await Task.Delay(RetryInterval(budget)).ConfigureAwait(false); } } } diff --git a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs index fb4550f..0a32a99 100644 --- a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs +++ b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs @@ -180,13 +180,15 @@ namespace PSProxmoxVE.Cmdlets if (issueOperation == null) throw new ArgumentNullException(nameof(issueOperation)); var taskService = new TaskService(); - return GuestLockRetry.Execute(() => - { - var task = issueOperation(); - return string.IsNullOrEmpty(task.Upid) - ? task - : taskService.WaitForTask(session, node, task.Upid, null, null, null); - }); + return GuestLockRetry.Execute( + () => + { + var task = issueOperation(); + return string.IsNullOrEmpty(task.Upid) + ? task + : taskService.WaitForTask(session, node, task.Upid, null, null, null); + }, + onRetry: ex => WriteVerbose($"Guest is locked, retrying: {ex.Message}")); } /// diff --git a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs index 3eeb091..1ec8e7f 100644 --- a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientLockRetryTests.cs @@ -25,6 +25,15 @@ namespace PSProxmoxVE.Core.Tests.Client 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); + } + private static (PveHttpClient client, ScriptedHandler handler) NewClient( params (HttpStatusCode status, string body)[] responses) { @@ -33,6 +42,7 @@ namespace PSProxmoxVE.Core.Tests.Client var client = new PveHttpClient(session); var handler = new ScriptedHandler(responses); SetInnerHttpClient(client, new HttpClient(handler)); + SetRetryWindow(client, TimeSpan.FromMilliseconds(400)); return (client, handler); } diff --git a/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs b/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs index f1cd612..61e77c9 100644 --- a/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Utilities/GuestLockRetryTests.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Net; using System.Threading.Tasks; using PSProxmoxVE.Core.Exceptions; @@ -15,6 +16,10 @@ namespace PSProxmoxVE.Core.Tests.Utilities private const string LxcLockError = "can't lock file '/run/lock/lxc/pve-config-100.lock' - got timeout"; + // The gap between attempts scales with the budget, so a short window keeps the + // retrying tests off a 2s production sleep. + private static readonly TimeSpan ShortWindow = TimeSpan.FromMilliseconds(400); + private static PveApiException ApiError(string message) => new PveApiException(HttpStatusCode.InternalServerError, message, "nodes/pve9a/qemu/100/config", "PUT"); @@ -73,6 +78,23 @@ namespace PSProxmoxVE.Core.Tests.Utilities Assert.False(GuestLockRetry.IsLockTimeout(new InvalidOperationException(VmLockError))); } + [Fact] + public void Execute_ReportsEachReissueToTheOnRetryHook() + { + var attempts = 0; + var reported = new List(); + + GuestLockRetry.Execute(() => + { + attempts++; + if (attempts < 3) throw TaskError(VmLockError); + return 0; + }, ShortWindow, onRetry: reported.Add); + + Assert.Equal(2, reported.Count); + Assert.All(reported, e => Assert.IsType(e)); + } + [Fact] public void Execute_ReturnsWithoutRetryingWhenTheOperationSucceeds() { @@ -94,7 +116,7 @@ namespace PSProxmoxVE.Core.Tests.Utilities attempts++; if (attempts < 2) throw TaskError(VmLockError); return "cloned"; - }); + }, ShortWindow); Assert.Equal("cloned", result); Assert.Equal(2, attempts); @@ -124,7 +146,7 @@ namespace PSProxmoxVE.Core.Tests.Utilities attempts++; if (attempts < 2) throw ApiError(VmLockError); return Task.FromResult("written"); - }); + }, ShortWindow); Assert.Equal("written", result); Assert.Equal(2, attempts);