From 0ec75a02e7114a84f4378e732e32c6e13825a4c8 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:21:00 +0000 Subject: [PATCH] fix: surface swallowed errors in status polling and per-node listing (#181) * fix: surface swallowed errors in status polling and per-node listing WaitForStatusTransition's poll loop caught every exception except OOM/ StackOverflow and discarded it silently. An expired ticket, a deleted VM, or a wrong node name under -Wait -Timeout spun for the full timeout and then raised PveTaskTimeoutException instead of the real 401/403/404. The loop now catches only PveApiException (excluding 401/403/404, which propagate) and HttpRequestException, and WriteVerbose's what it swallows. VmService.GetVms and ContainerService.GetContainers caught PveApiException of any status per node and continued, so a permission problem or an unreachable node looked identical to "no VMs". The per-node catch now narrows to a 5xx/408/connectivity failure (IsNodeUnreachable) and takes an optional onNodeSkipped callback; Get-PveVm and Get-PveContainer wire it to WriteWarning. Any other status (401/403/404 included) propagates. Adds xUnit coverage for the per-node aggregation path (403 propagates, 500/ 408/connectivity failures are skipped and reported, the other nodes' results still come back), reaching NodeService's internal client via reflection since it is not otherwise constructor-injectable from VmService/ ContainerService. Closes #142 * test: add per-node aggregation coverage and wire onNodeSkipped Adds the remaining changes: VmService/ContainerService per-node catch narrowing plus onNodeSkipped callback, the Get-PveVm/Get-PveContainer WriteWarning wiring, and the xUnit coverage for the aggregation loop. --------- Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- .../Services/ContainerService.cs | 29 +++- src/PSProxmoxVE.Core/Services/VmService.cs | 29 +++- .../Containers/GetPveContainerCmdlet.cs | 3 +- src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs | 11 +- src/PSProxmoxVE/Cmdlets/Vms/GetPveVmCmdlet.cs | 3 +- .../Services/ContainerServiceTests.cs | 126 ++++++++++++++++++ .../Services/VmServiceTests.cs | 125 +++++++++++++++++ 7 files changed, 316 insertions(+), 10 deletions(-) diff --git a/src/PSProxmoxVE.Core/Services/ContainerService.cs b/src/PSProxmoxVE.Core/Services/ContainerService.cs index 341f1f0..67a5be3 100644 --- a/src/PSProxmoxVE.Core/Services/ContainerService.cs +++ b/src/PSProxmoxVE.Core/Services/ContainerService.cs @@ -34,7 +34,13 @@ namespace PSProxmoxVE.Core.Services /// /// Returns containers. If is null, queries every cluster node. /// - public PveContainer[] GetContainers(PveSession session, string? node = null) + /// + /// Optional callback invoked with the node name and the exception when a node is + /// skipped because it is unreachable (connectivity failure or a 5xx from that node). + /// A 401/403/404 or any other non-5xx + /// propagates instead of being swallowed. + /// + public PveContainer[] GetContainers(PveSession session, string? node = null, Action? onNodeSkipped = null) { if (session == null) throw new ArgumentNullException(nameof(session)); @@ -52,14 +58,31 @@ namespace PSProxmoxVE.Core.Services ct.Node ??= n.Name; all.AddRange(containers); } - catch (Exception ex) when (ex is PSProxmoxVE.Core.Exceptions.PveApiException or System.Net.Http.HttpRequestException) + catch (Exception ex) when (IsNodeUnreachable(ex)) { - // Skip offline/inaccessible nodes + onNodeSkipped?.Invoke(n.Name, ex); } } return all.ToArray(); } + /// + /// True for a connectivity failure or a 5xx PVE API response — the cases where the + /// node itself is unreachable rather than the request being rejected. A 401/403/404 + /// (or any other non-5xx status) means the request was understood and refused, which + /// is not something a per-node listing loop should hide. + /// + private static bool IsNodeUnreachable(Exception ex) => ex switch + { + System.Net.Http.HttpRequestException => true, + // PveHttpClient wraps a connection failure as 503 and a client-side timeout as + // 408 (Client/PveHttpClient.cs SendOnceAsync) — both mean the node did not answer, + // not that it rejected the request. + PSProxmoxVE.Core.Exceptions.PveApiException apiEx => + apiEx.StatusCode == System.Net.HttpStatusCode.RequestTimeout || (int)apiEx.StatusCode >= 500, + _ => false + }; + private PveContainer[] GetContainersOnNode(PveSession session, string node) { IPveHttpClient client = _injectedClient ?? new PveHttpClient(session); diff --git a/src/PSProxmoxVE.Core/Services/VmService.cs b/src/PSProxmoxVE.Core/Services/VmService.cs index 8df89d0..013140c 100644 --- a/src/PSProxmoxVE.Core/Services/VmService.cs +++ b/src/PSProxmoxVE.Core/Services/VmService.cs @@ -37,7 +37,13 @@ namespace PSProxmoxVE.Core.Services /// /// The authenticated PVE session. /// Optional cluster node name to filter VMs by node. - public PveVm[] GetVms(PveSession session, string? node = null) + /// + /// Optional callback invoked with the node name and the exception when a node is + /// skipped because it is unreachable (connectivity failure or a 5xx from that node). + /// A 401/403/404 or any other non-5xx + /// propagates instead of being swallowed. + /// + public PveVm[] GetVms(PveSession session, string? node = null, Action? onNodeSkipped = null) { if (session == null) throw new ArgumentNullException(nameof(session)); @@ -57,14 +63,31 @@ namespace PSProxmoxVE.Core.Services vm.Node ??= n.Name; all.AddRange(vms); } - catch (Exception ex) when (ex is PSProxmoxVE.Core.Exceptions.PveApiException or System.Net.Http.HttpRequestException) + catch (Exception ex) when (IsNodeUnreachable(ex)) { - // Skip nodes that are offline or inaccessible + onNodeSkipped?.Invoke(n.Name, ex); } } return all.ToArray(); } + /// + /// True for a connectivity failure or a 5xx PVE API response — the cases where the + /// node itself is unreachable rather than the request being rejected. A 401/403/404 + /// (or any other non-5xx status) means the request was understood and refused, which + /// is not something a per-node listing loop should hide. + /// + private static bool IsNodeUnreachable(Exception ex) => ex switch + { + System.Net.Http.HttpRequestException => true, + // PveHttpClient wraps a connection failure as 503 and a client-side timeout as + // 408 (Client/PveHttpClient.cs SendOnceAsync) — both mean the node did not answer, + // not that it rejected the request. + PSProxmoxVE.Core.Exceptions.PveApiException apiEx => + apiEx.StatusCode == System.Net.HttpStatusCode.RequestTimeout || (int)apiEx.StatusCode >= 500, + _ => false + }; + private PveVm[] GetVmsOnNode(PveSession session, string node) { IPveHttpClient client = _injectedClient ?? new PveHttpClient(session); diff --git a/src/PSProxmoxVE/Cmdlets/Containers/GetPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/GetPveContainerCmdlet.cs index 30d00d2..6c7e5b4 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/GetPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/GetPveContainerCmdlet.cs @@ -59,7 +59,8 @@ namespace PSProxmoxVE.Cmdlets.Containers WriteVerbose("Getting containers..."); var service = new ContainerService(); - IEnumerable containers = service.GetContainers(session, Node); + IEnumerable containers = service.GetContainers(session, Node, + onNodeSkipped: (nodeName, ex) => WriteWarning($"Skipping node '{nodeName}': {ex.Message}")); if (VmId.HasValue) containers = containers.Where(c => c.VmId == VmId.Value); diff --git a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs index 3a3cc31..c3bf6f8 100644 --- a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs +++ b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs @@ -144,9 +144,16 @@ namespace PSProxmoxVE.Cmdlets if (snapshot.StatusMatched && !snapshot.Locked) return task; } - catch (Exception ex) when (ex is not OutOfMemoryException and not StackOverflowException) + catch (PSProxmoxVE.Core.Exceptions.PveApiException ex) when ( + ex.StatusCode != System.Net.HttpStatusCode.Unauthorized + && ex.StatusCode != System.Net.HttpStatusCode.Forbidden + && ex.StatusCode != System.Net.HttpStatusCode.NotFound) { - // Ignore transient errors during polling + WriteVerbose($"Status poll failed, retrying: {ex.Message}"); + } + catch (System.Net.Http.HttpRequestException ex) + { + WriteVerbose($"Status poll failed, retrying: {ex.Message}"); } System.Threading.Thread.Sleep(2000); diff --git a/src/PSProxmoxVE/Cmdlets/Vms/GetPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/GetPveVmCmdlet.cs index 4b79eaf..7e1a325 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/GetPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/GetPveVmCmdlet.cs @@ -75,7 +75,8 @@ namespace PSProxmoxVE.Cmdlets.Vms WriteVerbose("Getting VMs..."); var service = new VmService(); - IEnumerable vms = service.GetVms(session, Node); + IEnumerable vms = service.GetVms(session, Node, + onNodeSkipped: (nodeName, ex) => WriteWarning($"Skipping node '{nodeName}': {ex.Message}")); if (VmId.HasValue) vms = vms.Where(v => v.VmId == VmId.Value); diff --git a/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs index 1e99458..533a547 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs @@ -1,7 +1,12 @@ +using System; using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Reflection; using Moq; using PSProxmoxVE.Core.Authentication; using PSProxmoxVE.Core.Client; +using PSProxmoxVE.Core.Exceptions; using PSProxmoxVE.Core.Services; using Xunit; @@ -113,5 +118,126 @@ namespace PSProxmoxVE.Core.Tests.Services Assert.NotNull(captured); Assert.Equal("305", captured!["newid"]); } + + // --------------------------------------------------------------------- + // GetContainers multi-node aggregation: issue #142 + // --------------------------------------------------------------------- + + /// + /// Points the private NodeService field a ContainerService constructs at a mock + /// client, so the "list all nodes" call the multi-node overload issues is reachable + /// without a real HTTP connection. ContainerService(client) only injects the client + /// used for the per-node lxc calls; NodeService is never constructor-injectable from + /// ContainerService, so this is the only offline path to the aggregation loop. + /// + private static void InjectNodeServiceClient(ContainerService service, IPveHttpClient client) + { + var field = typeof(ContainerService).GetField("_nodeService", BindingFlags.NonPublic | BindingFlags.Instance) + ?? throw new InvalidOperationException("ContainerService._nodeService field not found."); + field.SetValue(service, new NodeService(client)); + } + + private static Mock SetupTwoNodeCluster() + { + var mockClient = new Mock(); + mockClient + .Setup(c => c.GetAsync("nodes")) + .ReturnsAsync("{\"data\":[{\"node\":\"pve1\"},{\"node\":\"pve2\"}]}"); + return mockClient; + } + + [Fact] + public void GetContainers_AllNodes_A500OnOneNodeIsSkippedAndReportedButOtherNodeResultsReturn() + { + var mockClient = SetupTwoNodeCluster(); + mockClient + .Setup(c => c.GetAsync("nodes/pve1/lxc")) + .ReturnsAsync("{\"data\":[{\"vmid\":100}]}"); + mockClient + .Setup(c => c.GetAsync("nodes/pve2/lxc")) + .ThrowsAsync(new PveApiException(HttpStatusCode.InternalServerError, "internal error", "nodes/pve2/lxc", "GET")); + + var service = new ContainerService(mockClient.Object); + InjectNodeServiceClient(service, mockClient.Object); + + var skipped = new List(); + var containers = service.GetContainers(CreateSession(), onNodeSkipped: (node, ex) => skipped.Add(node)); + + var ct = Assert.Single(containers); + Assert.Equal(100, ct.VmId); + Assert.Equal(new[] { "pve2" }, skipped); + } + + [Fact] + public void GetContainers_AllNodes_A403OnOneNodePropagatesInsteadOfBeingSwallowed() + { + var mockClient = SetupTwoNodeCluster(); + mockClient + .Setup(c => c.GetAsync("nodes/pve1/lxc")) + .ReturnsAsync("{\"data\":[{\"vmid\":100}]}"); + mockClient + .Setup(c => c.GetAsync("nodes/pve2/lxc")) + .ThrowsAsync(new PveApiException(HttpStatusCode.Forbidden, "permission denied", "nodes/pve2/lxc", "GET")); + + var service = new ContainerService(mockClient.Object); + InjectNodeServiceClient(service, mockClient.Object); + + var ex = Assert.Throws(() => service.GetContainers(CreateSession())); + Assert.Equal(HttpStatusCode.Forbidden, ex.StatusCode); + } + + [Fact] + public void GetContainers_AllNodes_ConnectivityFailureOnOneNodeIsSkippedAndReported() + { + // PveHttpClient.SendOnceAsync never lets a raw HttpRequestException escape — it + // wraps one as PveApiException(ServiceUnavailable, ..., inner: HttpRequestException). + // That is the shape a real connectivity failure takes by the time it reaches + // ContainerService, so that is what this test throws. + var mockClient = SetupTwoNodeCluster(); + mockClient + .Setup(c => c.GetAsync("nodes/pve1/lxc")) + .ReturnsAsync("{\"data\":[{\"vmid\":100}]}"); + mockClient + .Setup(c => c.GetAsync("nodes/pve2/lxc")) + .ThrowsAsync(new PveApiException(HttpStatusCode.ServiceUnavailable, "connection refused", + "nodes/pve2/lxc", "GET", new HttpRequestException("connection refused"))); + + var service = new ContainerService(mockClient.Object); + InjectNodeServiceClient(service, mockClient.Object); + + var skipped = new List(); + var containers = service.GetContainers(CreateSession(), onNodeSkipped: (node, ex) => skipped.Add(node)); + + var ct = Assert.Single(containers); + Assert.Equal(100, ct.VmId); + Assert.Equal(new[] { "pve2" }, skipped); + } + + [Fact] + public void GetContainers_AllNodes_ClientTimeoutOnOneNodeIsSkippedAndReported() + { + // PveHttpClient.SendOnceAsync wraps an HttpClient timeout as + // PveApiException(RequestTimeout) — the case of a powered-off or + // firewall-blackholed node, which must be skipped like any other + // unreachable node rather than aborting the whole listing. + var mockClient = SetupTwoNodeCluster(); + mockClient + .Setup(c => c.GetAsync("nodes/pve1/lxc")) + .ReturnsAsync("{\"data\":[{\"vmid\":100}]}"); + mockClient + .Setup(c => c.GetAsync("nodes/pve2/lxc")) + .ThrowsAsync(new PveApiException(HttpStatusCode.RequestTimeout, "Request timed out after 100s.", + "nodes/pve2/lxc", "GET")); + + var service = new ContainerService(mockClient.Object); + InjectNodeServiceClient(service, mockClient.Object); + + var skipped = new List(); + var containers = service.GetContainers(CreateSession(), onNodeSkipped: (node, ex) => skipped.Add(node)); + + var ct = Assert.Single(containers); + Assert.Equal(100, ct.VmId); + Assert.Equal(new[] { "pve2" }, skipped); + } } } diff --git a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs index be05cbb..d7f9e74 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs @@ -1,9 +1,13 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Net; +using System.Net.Http; +using System.Reflection; using Moq; using PSProxmoxVE.Core.Authentication; using PSProxmoxVE.Core.Client; +using PSProxmoxVE.Core.Exceptions; using PSProxmoxVE.Core.Services; using Xunit; @@ -268,5 +272,126 @@ namespace PSProxmoxVE.Core.Tests.Services Assert.Equal("305", captured!["newid"]); } + + // --------------------------------------------------------------------- + // GetVms multi-node aggregation: issue #142 + // --------------------------------------------------------------------- + + /// + /// Points the private NodeService field a VmService constructs at a mock client, + /// so the "list all nodes" call the multi-node overload issues is reachable + /// without a real HTTP connection. VmService(client) only injects the client used + /// for the per-node qemu calls; NodeService is never constructor-injectable from + /// VmService, so this is the only offline path to the aggregation loop. + /// + private static void InjectNodeServiceClient(VmService service, IPveHttpClient client) + { + var field = typeof(VmService).GetField("_nodeService", BindingFlags.NonPublic | BindingFlags.Instance) + ?? throw new InvalidOperationException("VmService._nodeService field not found."); + field.SetValue(service, new NodeService(client)); + } + + private static Mock SetupTwoNodeCluster() + { + var mockClient = new Mock(); + mockClient + .Setup(c => c.GetAsync("nodes")) + .ReturnsAsync("{\"data\":[{\"node\":\"pve1\"},{\"node\":\"pve2\"}]}"); + return mockClient; + } + + [Fact] + public void GetVms_AllNodes_A500OnOneNodeIsSkippedAndReportedButOtherNodeResultsReturn() + { + var mockClient = SetupTwoNodeCluster(); + mockClient + .Setup(c => c.GetAsync("nodes/pve1/qemu")) + .ReturnsAsync("{\"data\":[{\"vmid\":100}]}"); + mockClient + .Setup(c => c.GetAsync("nodes/pve2/qemu")) + .ThrowsAsync(new PveApiException(HttpStatusCode.InternalServerError, "internal error", "nodes/pve2/qemu", "GET")); + + var service = new VmService(mockClient.Object); + InjectNodeServiceClient(service, mockClient.Object); + + var skipped = new List(); + var vms = service.GetVms(CreateSession(), onNodeSkipped: (node, ex) => skipped.Add(node)); + + var vm = Assert.Single(vms); + Assert.Equal(100, vm.VmId); + Assert.Equal(new[] { "pve2" }, skipped); + } + + [Fact] + public void GetVms_AllNodes_A403OnOneNodePropagatesInsteadOfBeingSwallowed() + { + var mockClient = SetupTwoNodeCluster(); + mockClient + .Setup(c => c.GetAsync("nodes/pve1/qemu")) + .ReturnsAsync("{\"data\":[{\"vmid\":100}]}"); + mockClient + .Setup(c => c.GetAsync("nodes/pve2/qemu")) + .ThrowsAsync(new PveApiException(HttpStatusCode.Forbidden, "permission denied", "nodes/pve2/qemu", "GET")); + + var service = new VmService(mockClient.Object); + InjectNodeServiceClient(service, mockClient.Object); + + var ex = Assert.Throws(() => service.GetVms(CreateSession())); + Assert.Equal(HttpStatusCode.Forbidden, ex.StatusCode); + } + + [Fact] + public void GetVms_AllNodes_ConnectivityFailureOnOneNodeIsSkippedAndReported() + { + // PveHttpClient.SendOnceAsync never lets a raw HttpRequestException escape — it + // wraps one as PveApiException(ServiceUnavailable, ..., inner: HttpRequestException). + // That is the shape a real connectivity failure takes by the time it reaches + // VmService, so that is what this test throws. + var mockClient = SetupTwoNodeCluster(); + mockClient + .Setup(c => c.GetAsync("nodes/pve1/qemu")) + .ReturnsAsync("{\"data\":[{\"vmid\":100}]}"); + mockClient + .Setup(c => c.GetAsync("nodes/pve2/qemu")) + .ThrowsAsync(new PveApiException(HttpStatusCode.ServiceUnavailable, "connection refused", + "nodes/pve2/qemu", "GET", new HttpRequestException("connection refused"))); + + var service = new VmService(mockClient.Object); + InjectNodeServiceClient(service, mockClient.Object); + + var skipped = new List(); + var vms = service.GetVms(CreateSession(), onNodeSkipped: (node, ex) => skipped.Add(node)); + + var vm = Assert.Single(vms); + Assert.Equal(100, vm.VmId); + Assert.Equal(new[] { "pve2" }, skipped); + } + + [Fact] + public void GetVms_AllNodes_ClientTimeoutOnOneNodeIsSkippedAndReported() + { + // PveHttpClient.SendOnceAsync wraps an HttpClient timeout as + // PveApiException(RequestTimeout) — the case of a powered-off or + // firewall-blackholed node, which must be skipped like any other + // unreachable node rather than aborting the whole listing. + var mockClient = SetupTwoNodeCluster(); + mockClient + .Setup(c => c.GetAsync("nodes/pve1/qemu")) + .ReturnsAsync("{\"data\":[{\"vmid\":100}]}"); + mockClient + .Setup(c => c.GetAsync("nodes/pve2/qemu")) + .ThrowsAsync(new PveApiException(HttpStatusCode.RequestTimeout, "Request timed out after 100s.", + "nodes/pve2/qemu", "GET")); + + var service = new VmService(mockClient.Object); + InjectNodeServiceClient(service, mockClient.Object); + + var skipped = new List(); + var vms = service.GetVms(CreateSession(), onNodeSkipped: (node, ex) => skipped.Add(node)); + + var vm = Assert.Single(vms); + Assert.Equal(100, vm.VmId); + Assert.Equal(new[] { "pve2" }, skipped); + } } }