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>

src/PSProxmoxVE.Core/Services/ContainerService.cs

@@ -34,7 +34,13 @@ namespace PSProxmoxVE.Core.Services
         /// <summary>
         /// Returns containers. If <paramref name="node"/> is null, queries every cluster node.
         /// </summary>
-        public PveContainer[] GetContainers(PveSession session, string? node = null)
+        /// <param name="onNodeSkipped">
+        /// 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 <see cref="PSProxmoxVE.Core.Exceptions.PveApiException"/>
+        /// propagates instead of being swallowed.
+        /// </param>
+        public PveContainer[] GetContainers(PveSession session, string? node = null, Action<string, Exception>? 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();
         }
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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);

src/PSProxmoxVE.Core/Services/VmService.cs

@@ -37,7 +37,13 @@ namespace PSProxmoxVE.Core.Services
         /// </summary>
         /// <param name="session">The authenticated PVE session.</param>
         /// <param name="node">Optional cluster node name to filter VMs by node.</param>
-        public PveVm[] GetVms(PveSession session, string? node = null)
+        /// <param name="onNodeSkipped">
+        /// 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 <see cref="PSProxmoxVE.Core.Exceptions.PveApiException"/>
+        /// propagates instead of being swallowed.
+        /// </param>
+        public PveVm[] GetVms(PveSession session, string? node = null, Action<string, Exception>? 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();
         }
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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);

src/PSProxmoxVE/Cmdlets/Containers/GetPveContainerCmdlet.cs

@@ -59,7 +59,8 @@ namespace PSProxmoxVE.Cmdlets.Containers
             WriteVerbose("Getting containers...");
             var service = new ContainerService();
 
-            IEnumerable<PveContainer> containers = service.GetContainers(session, Node);
+            IEnumerable<PveContainer> 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);

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);

src/PSProxmoxVE/Cmdlets/Vms/GetPveVmCmdlet.cs

@@ -75,7 +75,8 @@ namespace PSProxmoxVE.Cmdlets.Vms
             WriteVerbose("Getting VMs...");
             var service = new VmService();
 
-            IEnumerable<PveVm> vms = service.GetVms(session, Node);
+            IEnumerable<PveVm> 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);

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
+        // ---------------------------------------------------------------------
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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<IPveHttpClient> SetupTwoNodeCluster()
+        {
+            var mockClient = new Mock<IPveHttpClient>();
+            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<string>();
+            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<PveApiException>(() => 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<string>();
+            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<string>();
+            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);
+        }
     }
 }

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
+        // ---------------------------------------------------------------------
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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<IPveHttpClient> SetupTwoNodeCluster()
+        {
+            var mockClient = new Mock<IPveHttpClient>();
+            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<string>();
+            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<PveApiException>(() => 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<string>();
+            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<string>();
+            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);
+        }
     }
 }