From fc38cee00e65c964644e702379058cc9ecc8433c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:07:00 -0400 Subject: [PATCH] docs(r2-06): record S-11/U-7/C-7 remediation in STATUS.md + 06 report rows (06/S-11,U-7,C-7) --- archreview/06-gateway-integrations.md | 8 +++++++- .../R2-06-serverhistorian-failfast-plan.md.tasks.json | 2 +- archreview/plans/STATUS.md | 6 ++++++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/archreview/06-gateway-integrations.md b/archreview/06-gateway-integrations.md index ab9c6b44..512b8d37 100644 --- a/archreview/06-gateway-integrations.md +++ b/archreview/06-gateway-integrations.md @@ -43,7 +43,7 @@ | U-5 (Low) | Outbox serializer has no version/format byte | **STILL OPEN** — `HistorizationOutboxEntrySerializer.cs:14-16` unchanged. | | U-6 | Test-coverage thin spots (advise-failure status, out-of-order `RemoveAsync`, S-3 hang, pump filter) | **STILL OPEN** — zero test changes in the three gateway test projects since `9cad9ed0`. | -**New findings this pass:** S-11 (Medium), U-7 (Medium), C-7 (Low) — see body. +**New findings this pass:** S-11 (Medium), U-7 (Medium), C-7 (Low) — see body. **All three REMEDIATED on `r2/06-serverhistorian-failfast` (R2-06, 2026-07-13); the U-7 live EnsureTags/retype leg is operator-gated pending a VPN run.** --- @@ -138,6 +138,8 @@ The read synthesis (`GalaxyDriver.cs:738-755`) fills pending snapshots with `Bad #### S-11 (Medium, NEW) — An enabled-but-misconfigured `ServerHistorian` section crash-loops the Host with a raw `UriFormatException`; one corner gets no warning at all +> **REMEDIATED — `r2/06-serverhistorian-failfast` (R2-06, 2026-07-13; live leg operator-gated).** New fail-fast `ServerHistorianOptionsValidator` (Host, wired via `AddValidatedOptions`) consumer-gates on `ServerHistorian:Enabled` **OR** `AlarmHistorian:Enabled` (closing the zero-warning corner) and fails startup with a named, aggregated `OptionsValidationException` on an empty / non-absolute / non-http(s) `Endpoint`. Adapter defense-in-depth replaces the bare `new Uri(...)` with `Uri.TryCreate`→named `InvalidOperationException`. `Validate()` gains malformed-endpoint warning parity; an alarm-only deployment logs a startup warning. Also directly answers C-4's "give the warnings style a fail-fast tier". + `HistorianGatewayClientAdapter.Create` eagerly does `new Uri(options.Endpoint)` (`HistorianGatewayClientAdapter.cs:45`). `ServerHistorianOptions.Validate()` produces exactly the right warning for an empty Endpoint (`ServerHistorianOptions.cs:78-79`) — but it is **warnings-only**: registration proceeds (`Runtime/ServiceCollectionExtensions.cs:135-139`, `Host/Program.cs:123-127`) and the first DI resolution throws an unhandled `UriFormatException`, crashing the Host at bring-up. This is not theoretical — it made docker-dev unbootable until the config-side fix `7233e2ba` (whose commit message documents the exact failure). The code path is untouched. The sharper corner: **`AddAlarmHistorian` gates only on `AlarmHistorian:Enabled`** (`Runtime/ServiceCollectionExtensions.cs:87`) but sources its connection from the `ServerHistorian` section (`Program.cs:127`). With `AlarmHistorian:Enabled=true` and `ServerHistorian:Enabled=false` (Endpoint legitimately empty), `Validate()` early-returns on `!Enabled` (`ServerHistorianOptions.cs:77`) — so the operator gets **zero warning**, then the same `UriFormatException` out of `CreateAlarmWriter` when the sink singleton is built. Same exposure for the continuous-historization writer (`Program.cs:187`, though that one is co-gated on `ServerHistorian.Enabled`, so only a *malformed* non-empty Endpoint reaches it). @@ -212,6 +214,8 @@ Galaxy: throw-on-construct for required fields; DataAnnotations attributes on th #### C-7 (Low, NEW) — The live-value writer populates a documented-dead `Quality` field +> **REMEDIATED — `r2/06-serverhistorian-failfast` (R2-06, 2026-07-13).** Decision: **keep populating** the field (the "stop populating so it's visible" alternative was rejected — proto3 sends `0` = Bad when unset, planting a future bug the instant a gateway version honors the field, whereas the recorder's `192` is correct). Visibility achieved with doc comments instead: `GatewayHistorianValueWriter` `` + inline note at the mapping, `ContinuousHistorizationRecorder.GoodQuality` guard note, and a `docs/Historian.md` sentence — all citing 0.2.0 Contracts C-002. + `GatewayHistorianValueWriter` maps `HistorizationValue.Quality` onto `HistorianLiveValue.Quality` (`GatewayHistorianValueWriter.cs:63`), but the 0.2.0 contract now documents that field (and `QualityDetail`) as **silently discarded on the `WriteLiveValues` SQL path** — the gateway's SQL login is column-permission-denied on the server-managed Quality column, so the value is accepted on the wire and dropped. Harmless today because the recorder hardcodes `GoodQuality = 192` for every captured value (`ContinuousHistorizationRecorder.cs:45,256` — the outbox round-trips it, `:419`), so nothing meaningful is lost. The hazard is forward-looking: if the recorder is ever extended to capture *real* node quality (a natural next step), it will appear to work and silently persist nothing — quality will remain server-stamped. Add a comment at the mapping site citing the 0.2.0 contract note (Contracts C-002), or stop populating the field so the dead input is visible. ### 4. Underdeveloped Areas @@ -237,6 +241,8 @@ The `Category=LiveIntegration` suite is complete and skip-clean: `GatewayLiveFix #### U-7 (Medium, NEW) — The 0.2.0 bump's impact analysis missed the `DataType` presence change that alters Boolean tag provisioning +> **REMEDIATED (code + docs) — `r2/06-serverhistorian-failfast` (R2-06, 2026-07-13); LIVE LEG OPERATOR-GATED.** Pinned the 0.2.0 Boolean→Int1 explicit-presence semantics at two levels (`GatewayTagProvisionerTests.Boolean_definition_carries_explicit_Int1_presence` asserts `DataType==Int1` **and** `HasDataType` — the 0.2.0 presence witness; a package downgrade breaks the compile, a dropped-presence contract change breaks the assert). Added a written upsert/retype migration note to `docs/Historian.md` (three retype hypotheses + `failed=N` tally observability). Added the env-gated live Boolean EnsureTags leg + a **retype probe** (`HISTGW_BOOL_SANDBOX_TAG`) that records the deployed historian's in-place-retype policy. **Pending:** the live VPN run on a 0.2.0 gateway (feeds the observed retype answer back into the doc note; also discharges U-2's full documented run). + The bump commit (`f6eaa267`) gates only on the M3 `opc_quality` change ("checked and CLEAR" — correct, verified: OtOpcUa never writes `HistorianHistoricalValue`). But 0.2.0 also made **`HistorianTagDefinition.DataType` proto3-optional** (contracts review M-2), and that one lands squarely on `GatewayTagProvisioner`: - The provisioner always sets `DataType` explicitly (`GatewayTagProvisioner.cs:57`), and `Boolean → HistorianDataType.Int1` (`HistorianTypeMapper.cs:28`) — where **Int1 is wire value 0**. diff --git a/archreview/plans/R2-06-serverhistorian-failfast-plan.md.tasks.json b/archreview/plans/R2-06-serverhistorian-failfast-plan.md.tasks.json index a4cdd1fa..e56bee82 100644 --- a/archreview/plans/R2-06-serverhistorian-failfast-plan.md.tasks.json +++ b/archreview/plans/R2-06-serverhistorian-failfast-plan.md.tasks.json @@ -79,7 +79,7 @@ { "id": "T11", "subject": "Bookkeeping: update archreview/plans/STATUS.md + 06-gateway-integrations.md prior-finding rows for S-11/U-7/C-7 when the work lands", - "status": "pending", + "status": "completed", "blockedBy": [ "T6", "T9", diff --git a/archreview/plans/STATUS.md b/archreview/plans/STATUS.md index 0271f9e9..4b6d282c 100644 --- a/archreview/plans/STATUS.md +++ b/archreview/plans/STATUS.md @@ -150,3 +150,9 @@ surface), 03/S2/S3 block-bridging, 03/P1 surgical adds (guard prerequisite in pl per-plan task counts, effort, and suggested execution order: [`00-INDEX.md`](00-INDEX.md) §"Round 2 plans". 193 bite-sized TDD tasks total, ~18–24 dev-days end-to-end; R2-01/02/03 (the re-review's new-and-sharp items) are ~2 days combined. + +### Round-2 remediation landed + +| Plan | Findings | Branch | Commits | Status | +|---|---|---|---|---| +| **R2-06** | 06/S-11 (rank 7), 06/U-7 (rank 8), 06/C-7 | `r2/06-serverhistorian-failfast` (off `1676c8f4`) | `3a049a13`…`4640ecb1` (10 task commits) | **Code-complete, offline-verified.** S-11: fail-fast `ServerHistorianOptionsValidator` (consumer-gated on `ServerHistorian:Enabled` OR `AlarmHistorian:Enabled`, fails empty/non-http(s) Endpoint at startup) + `AddValidatedOptions` wiring + adapter defense-in-depth (`TryCreate`→named `InvalidOperationException`, no more raw `UriFormatException` crash-loop) + `Validate()` warning parity + alarm-only-mode startup warning. U-7: pinned Boolean→Int1 explicit-presence (`HasDataType`) + `HISTGW_BOOL_SANDBOX_TAG` live Boolean EnsureTags leg + retype probe (skip-clean offline) + upsert/retype migration note (`docs/Historian.md`). C-7: keep-populating decision + dead-`Quality` doc comments (0.2.0 C-002). **Live leg operator-gated (T12) — pending a VPN run on a 0.2.0 gateway** (records the observed in-place retype policy back into `docs/Historian.md`; also discharges 06/U-2's full documented run). |