[Overview] One bad health response crashes the dashboard host (non-HttpRequestException escapes the poller) #29

New Issue

2026-11-09T14:39:18-05:00

dohertj2 commented

2026-11-09 14:39:18 -05:00

From the scadaproj#15 shared-library review (code-reviews/Overview/findings.md, Overview-001), verified at 91fb2b4. Update the finding's Resolution row when fixed.


Severity	High
Priority	P1
Category	Error handling & resilience
Status	Open
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

The method promises it "never throws for an unreachable endpoint". It catches only
OperationCanceledException, HttpRequestException and, around deserialization, JsonException.

Other exceptions reachable from a remote response escape ProbeAsync. The confirmed case is
response.Content.ReadAsStringAsync (:99), which throws InvalidOperationException when the
Content-Type charset is not a known encoding.

From there the exception propagates up through each layer, none of which catches it:

The probe's ProbeAsync task faults.
Task.WhenAll in SweepAsync rethrows it.
ExecuteAsync catches only OperationCanceledException when stopping.
The BackgroundService faults.
The host applies the default HostOptions.BackgroundServiceExceptionBehavior = StopHost.

A malformed answer from any one registered instance therefore terminates the entire dashboard. This
contradicts design §5, which classes a non-JSON or failed answer as an Unreachable instance,
not a fatal error.

Failure scenario

A registered BaseUrl points at something that is not a healthy family app: a port reused by another
service, a misconfigured reverse proxy, or an app that emits Content-Type: application/json; charset=<unsupported>. Verified end-to-end with WebApplicationFactory<Program> and a local
HttpListener answering charset=bogus-xyz for one of two instances. Within the first sweep the log
showed:

[ERR] BackgroundService failed — System.InvalidOperationException: The character set provided in ContentType is invalid
[FTL] ... BackgroundServiceExceptionBehavior is configured to StopHost ... the IHost instance is stopping

ApplicationStopping fired within 256 ms. On wonder, NSSM restarts the service, and it dies again on
its first sweep: a crash loop. Operators lose the fleet view because of a single bad entry.

Recommendation

In ZbHealthReportClient.SendAsync, after the OperationCanceledException filters, add a final
catch (Exception ex) that returns Failed(startedAt, statusCode?, ex.Message).
Alternatively, read the body as bytes and decode as UTF-8 yourself, which removes the charset
dependency.
Defence in depth: wrap each per-target ProbeAsync in SweepAsync so a throwing probe becomes an
Unreachable result for that instance only, and catch-and-log per sweep in ExecuteAsync so one
bad sweep cannot end the loop.
Add a test with a handler returning an invalid charset, and one whose handler throws an arbitrary
exception.

Resolution

Unresolved.

From the scadaproj#15 shared-library review (`code-reviews/Overview/findings.md`, **Overview-001**), verified at 91fb2b4. Update the finding's Resolution row when fixed. | | | |--|--| | Severity | High | | Priority | P1 | | Category | Error handling & resilience | | Status | Open | | 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** The method promises it "never throws for an unreachable endpoint". It catches only `OperationCanceledException`, `HttpRequestException` and, around deserialization, `JsonException`. Other exceptions reachable from a remote response escape `ProbeAsync`. The confirmed case is `response.Content.ReadAsStringAsync` (`:99`), which throws **`InvalidOperationException`** when the `Content-Type` charset is not a known encoding. From there the exception propagates up through each layer, none of which catches it: 1. The probe's `ProbeAsync` task faults. 2. `Task.WhenAll` in `SweepAsync` rethrows it. 3. `ExecuteAsync` catches only `OperationCanceledException` when stopping. 4. The `BackgroundService` faults. 5. The host applies the default `HostOptions.BackgroundServiceExceptionBehavior = StopHost`. A malformed answer from any *one* registered instance therefore terminates the entire dashboard. This contradicts design §5, which classes a non-JSON or failed answer as an **Unreachable** instance, not a fatal error. **Failure scenario** A registered `BaseUrl` points at something that is not a healthy family app: a port reused by another service, a misconfigured reverse proxy, or an app that emits `Content-Type: application/json; charset=<unsupported>`. Verified end-to-end with `WebApplicationFactory<Program>` and a local `HttpListener` answering `charset=bogus-xyz` for one of two instances. Within the first sweep the log showed: - `[ERR] BackgroundService failed — System.InvalidOperationException: The character set provided in ContentType is invalid` - `[FTL] ... BackgroundServiceExceptionBehavior is configured to StopHost ... the IHost instance is stopping` `ApplicationStopping` fired within 256 ms. On wonder, NSSM restarts the service, and it dies again on its first sweep: a crash loop. Operators lose the fleet view because of a single bad entry. **Recommendation** - In `ZbHealthReportClient.SendAsync`, after the `OperationCanceledException` filters, add a final `catch (Exception ex)` that returns `Failed(startedAt, statusCode?, ex.Message)`. - Alternatively, read the body as bytes and decode as UTF-8 yourself, which removes the charset dependency. - Defence in depth: wrap each per-target `ProbeAsync` in `SweepAsync` so a throwing probe becomes an `Unreachable` result for that instance only, and catch-and-log per sweep in `ExecuteAsync` so one bad sweep cannot end the loop. - Add a test with a handler returning an invalid charset, and one whose handler throws an arbitrary exception. **Resolution** _Unresolved._

dohertj2 added the type:review-finding priority:p1 labels 2026-11-09 14:39:18 -05:00

dohertj2 referenced this issue

2026-11-09 14:39:45 -05:00

[code-reviews] Review Secrets, LocalDb, GalaxyRepository and Overview; re-review libs changed since 2026-06-01 #15

dohertj2 commented

2026-11-09 14:48:52 -05:00

Fixed on main c25acd6 (Overview-001 → Resolved): an unknown charset threw InvalidOperationException out of the probe and faulted the poller. The health client now has a final catch-all that marks just that instance Unreachable, and the poller guards each probe and logs a failed sweep instead of ending (second layer proven independently — poller tests pass with the client fix reverted). 5 new tests incl. one that runs the background poller and checks it stays up. Evidence: Overview 195/195, Theme 65/65, GalaxyRepository 74/74; each regression test failed before its fix; code-reviews --check consistent.

Fixed on main c25acd6 (Overview-001 → Resolved): an unknown charset threw InvalidOperationException out of the probe and faulted the poller. The health client now has a final catch-all that marks just that instance Unreachable, and the poller guards each probe and logs a failed sweep instead of ending (second layer proven independently — poller tests pass with the client fix reverted). 5 new tests incl. one that runs the background poller and checks it stays up. Evidence: Overview 195/195, Theme 65/65, GalaxyRepository 74/74; each regression test failed before its fix; code-reviews --check consistent.

dohertj2 closed this issue

2026-11-09 14:48:52 -05:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dohertj2/scadaproj#29