From 1e94d4188c4ffcbea7a3d3dff1fd0fceb7abb4c2 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 21:49:00 -0500 Subject: [PATCH] fix: report each flock reissue, and scale the gap to the retry budget Two non-blocking review observations. A 45s retry is indistinguishable from a hang with nothing on the wire, so GuestLockRetry.Execute takes an onRetry hook and InvokeGuestTask reports each reissue through WriteVerbose. The gap between attempts now scales with the budget, capped at the 2s production value. A caller passing a short window wants a fast answer rather than one long sleep, which also takes the retrying unit tests off a real 2s sleep each: the xUnit run drops from 8s to 4s. PveHttpClient's window becomes a field so those tests can shorten it too. --- src/PSProxmoxVE.Core/Client/PveHttpClient.cs | 5 +++- .../Utilities/GuestLockRetry.cs | 21 ++++++++++++--- src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs | 16 +++++++----- .../Client/PveHttpClientLockRetryTests.cs | 10 +++++++ .../Utilities/GuestLockRetryTests.cs | 26 +++++++++++++++++-- 5 files changed, 64 insertions(+), 14 deletions(-) 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);