Merge R2-06 ServerHistorian fail-fast + 0.2.0 DataType (arch-review round 2) [PR #427]
Findings 06/S-11 (fail-fast ServerHistorianOptionsValidator, no UriFormatException crash-loop), 06/U-7 (0.2.0 Boolean->Int1 explicit-presence + retype probe), 06/C-7 (keep-populating decision doc). T12 live VPN gate deferred. STATUS.md conflict (R2-12 bookkeeping) resolved additively. Build clean.
This commit is contained in:
Joseph Doherty
@@ -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` `<remarks>` + 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**.
|
||||
|
||||
@@ -245,3 +245,11 @@ Every cited file:line was opened against the working tree at `f6eaa267`. **All t
|
||||
| C-7 | **XS** (Task 10, ~5 min) | None | Comments/docs; deliberate keep-populating decision recorded |
|
||||
|
||||
**Total: ~1 hour of implementation across 3 commits, plus one operator VPN session for the live gate.** Suggested sequencing: Commit 1 (S-11) → Commit 2 (U-7) → Commit 3 (C-7) → Task 11 bookkeeping → Task 12 operator gate.
|
||||
|
||||
## Execution deviations (R2-06)
|
||||
|
||||
Executed 2026-07-13 on branch `r2/06-serverhistorian-failfast` (off master `1676c8f4`).
|
||||
|
||||
- **Commit granularity — per-task, not the plan's 3 grouped commits.** The plan text grouped the work into 3 commits (one per finding). The executor's operating method mandated one pathspec-scoped commit **per task** (with the task's `tasks.json` status flip in the same commit) for traceability/bisectability. Followed the per-task rule → 11 task commits (T1–T11) instead of 3. No content difference; strictly finer history. Reversible (squashable at merge if 3 commits are still wanted).
|
||||
- **Validator + wiring tests live in `Host.IntegrationTests` but are pure unit tests.** Per the plan (they mirror `LdapOptionsValidatorTests`, which lives there). They were verified green **filtered** (`--filter FullyQualifiedName~ServerHistorianOptionsValidatorTests`, 8/8) once during T4 — **before** a mid-run controller constraint arrived forbidding any run of `*.IntegrationTests` projects (a ~16 GB/run memory-leak in the heavy integration tests) and whole-solution `dotnet test`. After that constraint they were **not re-run**; the earlier filtered pass + a clean `dotnet build ZB.MOM.WW.OtOpcUa.slnx` (0 errors) are the standing evidence. The Historian.Gateway driver unit suite (a plain `.Tests` project) and the Runtime.Tests suite were run freely.
|
||||
- **T12 (live VPN gate) → `deferred-live`.** Genuinely external infra (VPN to `wonder-sql-vd03` + a 0.2.0 gateway with `RuntimeDb:Enabled=true` and an API key carrying `historian:read`+`write`+`tags:write`). The code (T8: fixture env var + Boolean EnsureTags leg + retype probe) is complete and offline **skip-clean** (6 LiveIntegration tests, all skipped, 0 failed). The operator runs the suite on the VPN and feeds the observed retype policy back into `docs/Historian.md` + STATUS.md.
|
||||
|
||||
@@ -5,74 +5,96 @@
|
||||
{
|
||||
"id": "T1",
|
||||
"subject": "RED: misconfig repro test — HistorianGatewayClientAdapter.Create with empty/malformed Endpoint must throw a named InvalidOperationException (currently UriFormatException; test MUST fail on current code)",
|
||||
"status": "pending",
|
||||
"status": "completed",
|
||||
"blockedBy": []
|
||||
},
|
||||
{
|
||||
"id": "T2",
|
||||
"subject": "GREEN: adapter guard — Uri.TryCreate + InvalidOperationException naming ServerHistorian:Endpoint in HistorianGatewayClientAdapter.Create (defense in depth)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T1"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T1"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T3",
|
||||
"subject": "RED: ServerHistorianOptionsValidator unit tests (7 cases incl. the AlarmHistorian-enabled/ServerHistorian-disabled corner naming both sections)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T1"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T1"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T4",
|
||||
"subject": "GREEN: implement ServerHistorianOptionsValidator (OptionsValidatorBase, ctor IConfiguration, consumed = Enabled || AlarmHistorian:Enabled, fail on empty/non-absolute/non-http(s) Endpoint)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T3"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T3"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T5",
|
||||
"subject": "Wiring: AddValidatedOptions<ServerHistorianOptions, ServerHistorianOptionsValidator> in Program.cs hasDriver block + OptionsValidationException wiring test + residual alarm-only-mode startup warning",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T4"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T4"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T6",
|
||||
"subject": "Warning parity: malformed-endpoint warning in ServerHistorianOptions.Validate() + Runtime.Tests case; commit 1 (S-11) after T1-T6 green",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T4"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T4"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T7",
|
||||
"subject": "U-7 pin: GatewayTagProvisionerTests Boolean_definition_carries_explicit_Int1_presence (asserts HistorianDataType.Int1 AND HasDataType — 0.2.0 proto3-optional presence witness; pin, no RED phase)",
|
||||
"status": "pending",
|
||||
"status": "completed",
|
||||
"blockedBy": []
|
||||
},
|
||||
{
|
||||
"id": "T8",
|
||||
"subject": "U-7 live leg: HISTGW_BOOL_SANDBOX_TAG in GatewayLiveFixture + EnsureTags_boolean_sandbox_provisions_as_Int1 + EnsureTags_type_change_outcome_is_observable retype probe (skip-clean offline; verify all-skip run)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T7"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T7"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T9",
|
||||
"subject": "U-7 docs: migration/assessment note (EnsureTags is upsert; pre-bump Float Booleans; three retype hypotheses; failed=N tally observability) in docs/Historian.md + CLAUDE.md HISTGW_BOOL_SANDBOX_TAG recipe update; commit 2 (U-7) after T7-T9",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T8"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T8"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T10",
|
||||
"subject": "C-7: keep-populating decision + dead-Quality doc comments at GatewayHistorianValueWriter.cs:63, ContinuousHistorizationRecorder.GoodQuality, docs/Historian.md (cites 0.2.0 Contracts C-002); commit 3 (C-7)",
|
||||
"status": "pending",
|
||||
"status": "completed",
|
||||
"blockedBy": []
|
||||
},
|
||||
{
|
||||
"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",
|
||||
"blockedBy": ["T6", "T9", "T10"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T6",
|
||||
"T9",
|
||||
"T10"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T12",
|
||||
"subject": "OPERATOR GATE: run Category=LiveIntegration on the VPN against a 0.2.0 gateway (all 6 tests, incl. Boolean EnsureTags + retype probe); record the observed retype policy back into docs/Historian.md + STATUS.md; also discharges 06/U-2's full documented run",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T9", "T11"]
|
||||
"status": "deferred-live",
|
||||
"blockedBy": [
|
||||
"T9",
|
||||
"T11"
|
||||
],
|
||||
"note": "needs live gateway over VPN; needs-user / serial live pass"
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
@@ -164,3 +164,9 @@ clean. **OPEN follow-ups:** (a) when a runner-reachable fleet + a real `tests/Se
|
||||
add a fresh E2E workflow WITH a zero-match guard (fail if `Category=E2E` matches nothing); (b) S-2
|
||||
option #2 — a fixture-backed integration leg (opc-plc `services:` block, `MIN_EXECUTED=1`) covering
|
||||
what the unit leg cannot; (c) migrate the 3 xunit v2 holdouts once Akka ships an xunit.v3 TestKit.
|
||||
|
||||
### 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). |
|
||||
|
||||
Reference in New Issue
Block a user