dohertj2/scadaproj
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(overview): P3 review fixes 003-005 - no redirects on probes, per-target publish so a hung instance cannot stale the rest, probe docs (scadaproj#48)

Browse Source 
Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
This commit is contained in:
Joseph Doherty
2026-11-09 14:54:12 -05:00
parent 362ba8d75f
commit 3ef43ca926
9 changed files with 233 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Overview/findings.md
+7 -7
View File
@@ -10,7 +10,7 @@
| Last reviewed | 2026-11-09 |
| Reviewer | Claude |
| Commit reviewed | `91fb2b4` |
| Open findings | 3 |
| Open findings | 0 |
## Summary
@@ -183,7 +183,7 @@ Resolved 2026-11-09 in the commit tagged `(scadaproj#43)` — `LeaderResolver.Re
| Severity | Low |
| Priority | P3 |
| Category | Correctness & logic bugs |
| Status | Open |
| Status | Resolved |
| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReportClient.cs:112` (client config `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/PollingServiceCollectionExtensions.cs:25`) |
**Description**
@@ -216,7 +216,7 @@ so a 3xx maps to `Unknown`. Optionally, for a non-empty body that is not health
**Resolution**
_Unresolved._
Resolved 2026-11-09 in the commit tagged `(scadaproj#48)` — the named probe `HttpClient` now uses `SocketsHttpHandler { AllowAutoRedirect = false }`, so a 3xx on `/health/active` surfaces as its own status code and `ObserveActive` maps it to Unknown (the ready tier already requires a parseable body). Regression test `ProbeRedirectTests.ActiveProbe_AuthRedirectToLoginPage_IsNotReportedAsActive` (real loopback listener, 302 to a 200 login page; failed without the fix). Not done: the optional 'non-empty non-JSON 200 body is Unknown' rule — a plain-text 200 from a non-ZbHealth app on the active tier is a legitimate Active signal, so that needs an owner decision if wanted.
### Overview-004 — One slow instance gates the single sweep, so every other instance goes Stale
@@ -225,7 +225,7 @@ _Unresolved._
| Severity | Low |
| Priority | P3 |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/OverviewPollerService.cs:120` (sequential ready + active at `:155-171`; validator `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Registry/OverviewOptionsValidator.cs:69-76`) |
**Description**
@@ -257,7 +257,7 @@ registry, naming the offending override.
**Resolution**
_Unresolved._
Resolved 2026-11-09 in the commit tagged `(scadaproj#48)` — chose decoupling over a validator rule. The tick loop no longer awaits the sweep; each target is claimed (`TryClaim`) while its probe is in flight, so later ticks skip a hung target, and each probe records and publishes its own result the moment it completes (publish under a lock). `SweepAsync` still awaits its own batch and returns the final snapshot, so existing semantics for callers and tests hold. Regression test `Sweep_HungInstanceDoesNotHoldUpResultsOrLaterSweepsForTheOthers` (failed without the fix). Side effect: the heartbeat/sweep metric now describe the batch started on each tick, and a tick whose targets are all still in flight stamps the heartbeat immediately (the loop is alive; the probes are bounded by their timeouts).
### Overview-005 — Docs say `/health/active` is probed only when `HasActiveRole`; the code probes it for every reachable instance
@@ -266,7 +266,7 @@ _Unresolved._
| Severity | Low |
| Priority | P3 |
| Category | Documentation & XML docs |
| Status | Open |
| Status | Resolved |
| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Registry/OverviewOptions.cs:9` (also `:76-80`) |
**Description**
@@ -298,4 +298,4 @@ shown.
**Resolution**
_Unresolved._
Resolved 2026-11-09 in the commit tagged `(scadaproj#48)` — corrected the `OverviewOptions` remarks, the `InstanceEntry.HasActiveRole` XML doc, design doc §3/§5 and the Overview CLAUDE.md: the active tier is probed whenever ready answered and `HasActiveRole` only gates the Active/Standby badge.
code-reviews/README.md
+5 -8
View File
@@ -40,7 +40,7 @@ code-reviews/
## Summary
10 of 10 libraries reviewed. 38 pending findings across all libraries.
10 of 10 libraries reviewed. 35 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
@@ -51,8 +51,8 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low).
| Critical | 0 |
| High | 0 |
| Medium | 11 |
| Low | 27 |
| **Total** | **38** |
| Low | 24 |
| **Total** | **35** |
## 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/0/0 | 0 | 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/0/0/3 | 3 | 5 |
| [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 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/0/1 | 1 | 8 |
@@ -100,7 +100,7 @@ _None open._
| Secrets-013 | [Secrets](Secrets/findings.md) | UI add/rotate/delete: if the audit write throws after the store write succeeded, the change is reported and audited as a failure and the resolver cache is not evicted |
| Secrets-017 | [Secrets](Secrets/findings.md) | `secret set`/`rotate` take the plaintext on argv; the host builder re-parses argv as configuration and can crash with the value in the error |
### Low (27)
### Low (24)
| ID | Library | Title |
|----|---------|-------|
@@ -111,9 +111,6 @@ _None open._
| LocalDb-005 | [LocalDb](LocalDb/findings.md) | Auto-assigned rowid / AUTOINCREMENT keys collide across nodes and LWW silently drops one row |
| LocalDb-006 | [LocalDb](LocalDb/findings.md) | Column names containing `'` are captured but break the apply and snapshot SQL |
| LocalDb-007 | [LocalDb](LocalDb/findings.md) | README is stale on version, adoption, test count, `Synchronous` values and baseline scope |
| Overview-003 | [Overview](Overview/findings.md) | Status-only active probe treats any 200, including a followed redirect to a login page, as Active |
| Overview-004 | [Overview](Overview/findings.md) | One slow instance gates the single sweep, so every other instance goes Stale |
| Overview-005 | [Overview](Overview/findings.md) | Docs say `/health/active` is probed only when `HasActiveRole`; the code probes it for every reachable instance |
| Secrets-004 | [Secrets](Secrets/findings.md) | Corrupt or wrong-length crypto fields escape `Decrypt` as `ArgumentException`: not audited, and breaks the cipher contract |
| Secrets-005 | [Secrets](Secrets/findings.md) | Store-path rules are lexical: drive-relative paths pass "rooted", and `\\?\` / UNC / 8.3 / symlink spellings bypass "under content root" |
| Secrets-006 | [Secrets](Secrets/findings.md) | Resolver cache can re-cache a stale value after `Invalidate`, and expired plaintext is never evicted |