From c25acd6093ed448fa48cb0f69e467d094f12a50d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:42:23 -0500 Subject: [PATCH] fix(overview): one bad health response no longer stops the dashboard host (scadaproj#29) ZbHealthReportClient caught only HttpRequestException/OperationCanceledException, so an unknown Content-Type charset (InvalidOperationException from ReadAsStringAsync) escaped the probe, faulted Task.WhenAll and the poller BackgroundService, and StopHost took the dashboard down. - client: final catch-all returns an Unreachable result for that instance - poller: per-target Guarded probe + catch-and-log per sweep in ExecuteAsync - tests: unknown charset (both tiers), arbitrary handler exception, sweep with one bad instance, and the BackgroundService survival loop - code-reviews: Overview-001 Resolved; README regenerated Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- .../Polling/OverviewPollerService.cs | 33 ++++++++- .../Polling/ZbHealthReportClient.cs | 21 +++++- .../OverviewPollerServiceTests.cs | 71 +++++++++++++++++++ .../ZbHealthReportClientTests.cs | 44 ++++++++++++ code-reviews/Overview/findings.md | 6 +- code-reviews/README.md | 14 ++-- 6 files changed, 172 insertions(+), 17 deletions(-) diff --git a/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/OverviewPollerService.cs b/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/OverviewPollerService.cs index 79b04c1..8ca4893 100644 --- a/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/OverviewPollerService.cs +++ b/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/OverviewPollerService.cs @@ -85,7 +85,17 @@ public sealed class OverviewPollerService : BackgroundService { do { - await SweepAsync(stoppingToken).ConfigureAwait(false); + try + { + await SweepAsync(stoppingToken).ConfigureAwait(false); + } + catch (Exception ex) when (!stoppingToken.IsCancellationRequested) + { + // One bad sweep must not end the loop: a faulted BackgroundService stops the + // whole host under the default StopHost policy (Overview-001). The heartbeat is + // not stamped, so the dashboard's own health check still notices the gap. + _logger.LogError(ex, "Overview poll sweep failed; retrying on the next tick."); + } } while (await timer.WaitForNextTickAsync(stoppingToken).ConfigureAwait(false)); } @@ -152,7 +162,7 @@ public sealed class OverviewPollerService : BackgroundService CancellationToken cancellationToken) { var timeout = target.Timings.Timeout; - var ready = await probe.ProbeAsync(target.ReadyUrl, timeout, cancellationToken).ConfigureAwait(false); + var ready = await Guarded(probe.ProbeAsync(target.ReadyUrl, timeout, cancellationToken), cancellationToken).ConfigureAwait(false); // Skip the active probe when nothing answered the ready probe: it would spend a second full // timeout to learn what we already know, and ObserveActive maps a missing probe to Unknown @@ -168,7 +178,7 @@ public sealed class OverviewPollerService : BackgroundService // not exist. So probe unconditionally and let ObserveActive stay role-gated below. HealthProbeResult? active = null; if (ready.Reachable) - active = await probe.ProbeStatusAsync(target.ActiveUrl, timeout, cancellationToken).ConfigureAwait(false); + active = await Guarded(probe.ProbeStatusAsync(target.ActiveUrl, timeout, cancellationToken), cancellationToken).ConfigureAwait(false); var observed = StatusDerivation.Observe(ready); var (status, failures) = StatusDerivation.Damp( @@ -190,6 +200,23 @@ public sealed class OverviewPollerService : BackgroundService }; } + /// + /// Defence in depth behind 's own catch-all: a probe that + /// throws anything but a shutdown cancellation becomes an unreachable result for THIS instance + /// only, instead of faulting the sweep's Task.WhenAll (Overview-001). + /// + private static async Task Guarded(Task probe, CancellationToken cancellationToken) + { + try + { + return await probe.ConfigureAwait(false); + } + catch (Exception ex) when (!(ex is OperationCanceledException && cancellationToken.IsCancellationRequested)) + { + return new HealthProbeResult(false, null, null, TimeSpan.Zero, $"probe failed: {ex.GetType().Name}: {ex.Message}"); + } + } + private OverviewSnapshot BuildSnapshot(DateTimeOffset now) { var applications = _targets diff --git a/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReportClient.cs b/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReportClient.cs index 4e82cd3..329f783 100644 --- a/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReportClient.cs +++ b/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReportClient.cs @@ -48,7 +48,10 @@ public sealed class ZbHealthReportClient /// Absolute URL of a health endpoint. /// Per-request timeout. /// Token cancelled when the host is shutting down. - /// The probe result; this never throws for an unreachable endpoint. + /// + /// The probe result. This never throws for anything the remote end does — only for a + /// cancellation of (host shutdown). + /// public Task ProbeAsync( string url, TimeSpan timeout, @@ -62,7 +65,10 @@ public sealed class ZbHealthReportClient /// Absolute URL of a health endpoint. /// Per-request timeout. /// Token cancelled when the host is shutting down. - /// The probe result; this never throws for an unreachable endpoint. + /// + /// The probe result. This never throws for anything the remote end does — only for a + /// cancellation of (host shutdown). + /// /// /// For the active tier the status code IS the answer — 200 means active, 503 means standby — /// and the body carries nothing the dashboard uses. Demanding a parseable body here would @@ -89,6 +95,7 @@ public sealed class ZbHealthReportClient // timed-out instance. using var timeoutSource = new CancellationTokenSource(timeout); using var linked = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutSource.Token); + int? statusCode = null; try { @@ -96,8 +103,8 @@ public sealed class ZbHealthReportClient .GetAsync(url, HttpCompletionOption.ResponseContentRead, linked.Token) .ConfigureAwait(false); + statusCode = (int)response.StatusCode; var body = await response.Content.ReadAsStringAsync(linked.Token).ConfigureAwait(false); - var statusCode = (int)response.StatusCode; ZbHealthReport? report; try @@ -132,6 +139,14 @@ public sealed class ZbHealthReportClient { return Failed(startedAt, null, ex.Message); } + catch (Exception ex) + { + // Anything else a remote answer can provoke — e.g. InvalidOperationException from + // ReadAsStringAsync when the Content-Type names an unknown charset — is still a bad + // answer from ONE instance. Letting it escape faulted the poller BackgroundService and + // the host's StopHost policy took the whole dashboard down (Overview-001). + return Failed(startedAt, statusCode, $"unreadable response: {ex.GetType().Name}: {ex.Message}"); + } } private static HealthProbeResult Failed(long startedAt, int? statusCode, string error) => diff --git a/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/OverviewPollerServiceTests.cs b/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/OverviewPollerServiceTests.cs index 6f1be7a..bdb836e 100644 --- a/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/OverviewPollerServiceTests.cs +++ b/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/OverviewPollerServiceTests.cs @@ -1,4 +1,5 @@ using System.Net; +using System.Text; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using ZB.MOM.WW.Overview.Observability; @@ -434,6 +435,76 @@ public class OverviewPollerServiceTests Assert.True(polled.IsStale(Start.AddSeconds(46))); } + [Fact] + public async Task Sweep_OneInstanceWithAnUnreadableBody_DoesNotFaultTheSweep() + { + // Overview-001 (scadaproj#29): one instance answering with an unknown charset faulted the + // Task.WhenAll, the BackgroundService and then the host. It must cost that instance only. + var routes = new RoutingHandler() + .Json("http://good:8080/health/ready", HttpStatusCode.OK, HealthBody.Ready("Healthy")) + .Status("http://good:8080/health/active", HttpStatusCode.OK); + var handler = new BadCharsetForHost("bad", routes); + var (poller, _) = Build( + Registry(App("App", Node("good", "http://good:8080"), Node("bad", "http://bad:8080"))), + handler, + new FakeTimeProvider(Start)); + + var snapshot = await poller.SweepAsync(); + + var good = snapshot.AllInstances.Single(i => i.Instance == "good"); + var bad = snapshot.AllInstances.Single(i => i.Instance == "bad"); + Assert.Equal(InstanceStatus.Up, good.Status); + Assert.Equal(InstanceStatus.Unreachable, bad.Status); + Assert.NotNull(bad.Error); + } + + [Fact] + public async Task ExecuteLoop_SurvivesAnInstanceWithAnUnreadableBody() + { + // The end-to-end shape of the finding: the BackgroundService must keep running. + var routes = new RoutingHandler(); + var handler = new BadCharsetForHost("bad", routes); + var (poller, _) = Build( + Registry(App("App", Node("bad", "http://bad:8080"))), + handler, + new FakeTimeProvider(Start)); + + await poller.StartAsync(CancellationToken.None); + try + { + for (var i = 0; i < 100 && handler.Calls == 0; i++) + await Task.Delay(20); + await Task.Delay(100); + + Assert.True(handler.Calls > 0); + Assert.False(poller.ExecuteTask!.IsFaulted, poller.ExecuteTask.Exception?.ToString()); + } + finally + { + await poller.StopAsync(CancellationToken.None); + } + } + + /// Answers every request to host with a 200 carrying an unknown charset. + private sealed class BadCharsetForHost(string host, RoutingHandler inner) : HttpMessageHandler + { + private int _calls; + + public int Calls => Volatile.Read(ref _calls); + + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + if (!string.Equals(request.RequestUri!.Host, host, StringComparison.OrdinalIgnoreCase)) + return inner.HandleAsync(request); + + Interlocked.Increment(ref _calls); + var content = new ByteArrayContent(Encoding.UTF8.GetBytes(HealthBody.Ready("Healthy"))); + content.Headers.TryAddWithoutValidation("Content-Type", "application/json; charset=bogus-xyz"); + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) { Content = content }); + } + } + [Fact] public async Task Sweep_HostShutdown_AbandonsTheSweepInsteadOfRecordingFailures() { diff --git a/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/ZbHealthReportClientTests.cs b/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/ZbHealthReportClientTests.cs index 7e33ae8..83e6854 100644 --- a/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/ZbHealthReportClientTests.cs +++ b/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/ZbHealthReportClientTests.cs @@ -210,6 +210,38 @@ public class ZbHealthReportClientTests Assert.Null(result.StatusCode); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task Probe_UnknownCharset_IsUnreachableNotThrown(bool bodyRequired) + { + // Overview-001 (scadaproj#29): ReadAsStringAsync throws InvalidOperationException for a + // Content-Type charset it cannot map to an Encoding. That escaped ProbeAsync, faulted the + // poller BackgroundService and the default StopHost policy took the whole dashboard down. + var client = new ZbHealthReportClient(new HttpClient(new BogusCharsetHandler())); + + var result = bodyRequired + ? await client.ProbeAsync("http://node/health/ready", ProbeTimeout) + : await client.ProbeStatusAsync("http://node/health/active", ProbeTimeout); + + Assert.False(result.Reachable); + Assert.Equal(200, result.StatusCode); + Assert.NotNull(result.Error); + } + + [Fact] + public async Task Probe_ArbitraryHandlerException_IsUnreachableNotThrown() + { + var client = new ZbHealthReportClient( + new HttpClient(new ThrowingHandler(new InvalidOperationException("boom")))); + + var result = await client.ProbeAsync("http://node/health/ready", ProbeTimeout); + + Assert.False(result.Reachable); + Assert.Null(result.StatusCode); + Assert.Contains("boom", result.Error, StringComparison.Ordinal); + } + [Fact] public async Task Probe_ReadyTierStillDemandsAParseableBody() { @@ -233,6 +265,18 @@ public class ZbHealthReportClientTests }); } + /// A 200 whose Content-Type names a charset no Encoding maps to. + internal sealed class BogusCharsetHandler : HttpMessageHandler + { + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + var content = new ByteArrayContent(Encoding.UTF8.GetBytes("""{"status":"Healthy","entries":{}}""")); + content.Headers.TryAddWithoutValidation("Content-Type", "application/json; charset=bogus-xyz"); + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) { Content = content }); + } + } + private sealed class ThrowingHandler(Exception exception) : HttpMessageHandler { protected override Task SendAsync( diff --git a/code-reviews/Overview/findings.md b/code-reviews/Overview/findings.md index 3433f91..bbf5b8c 100644 --- a/code-reviews/Overview/findings.md +++ b/code-reviews/Overview/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 | | Reviewer | Claude | | Commit reviewed | `91fb2b4` | -| Open findings | 5 | +| Open findings | 4 | ## Summary @@ -71,7 +71,7 @@ live-acceptance checks). | Severity | High | | Priority | P1 | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReportClient.cs:99` (catch list `:122-134`; unguarded loop `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/OverviewPollerService.cs:86-95`) | **Description** @@ -125,7 +125,7 @@ its first sweep: a crash loop. Operators lose the fleet view because of a single **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#29)` — `ZbHealthReportClient.SendAsync` now ends with a catch-all that returns an Unreachable result (keeping the status code when one was received), so an unknown `charset` or any other non-shutdown exception costs only that instance. Defence in depth: `OverviewPollerService` wraps each per-target probe (`Guarded`) and catches-and-logs per sweep in `ExecuteAsync`, so neither a throwing probe nor a throwing sweep can fault the `BackgroundService`. Regression tests: `ZbHealthReportClientTests.Probe_UnknownCharset_IsUnreachableNotThrown` (both tiers), `Probe_ArbitraryHandlerException_IsUnreachableNotThrown`, `OverviewPollerServiceTests.Sweep_OneInstanceWithAnUnreadableBody_DoesNotFaultTheSweep` and `ExecuteLoop_SurvivesAnInstanceWithAnUnreadableBody` — all five fail without the fix (the loop test with the same `InvalidOperationException` from the finding); the poller tests also pass with the client fix reverted, proving the per-target guard independently. ### Overview-002 — Leader address is resolved against every instance of the application, not the group, so co-located groups steal each other's chip diff --git a/code-reviews/README.md b/code-reviews/README.md index 5749126..21bdd6a 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 49 pending findings across all libraries. +10 of 10 libraries reviewed. 48 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -49,10 +49,10 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | Severity | Open findings | |----------|---------------| | Critical | 0 | -| High | 1 | +| High | 0 | | Medium | 15 | | Low | 33 | -| **Total** | **49** | +| **Total** | **48** | ## Library Status @@ -64,7 +64,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | [GalaxyRepository](GalaxyRepository/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/2/6 | 8 | 8 | | [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/1 | 1 | 7 | | [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/4/3 | 7 | 7 | -| [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/1/1/3 | 5 | 5 | +| [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/3 | 4 | 5 | | [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/6/13 | 19 | 19 | | [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/3 | 3 | 11 | | [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/1 | 2 | 8 | @@ -80,11 +80,9 @@ description, location, recommendation — lives in the library's `findings.md`. _None open._ -### High (1) +### High (0) -| ID | Library | Title | -|----|---------|-------| -| Overview-001 | [Overview](Overview/findings.md) | Any non-`HttpRequestException` from one instance faults the poller and stops the whole dashboard | +_None open._ ### Medium (15)