From 19feb1b2898e1a17b20abfa7d1e9f9e94824b68b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:54:25 -0400 Subject: [PATCH] docs(code-review): add Driver.Historian.Gateway findings Per-module review at b990adae (this branch's base): all 10 checklist categories completed, 3 findings recorded and resolved in the two prior commits on this branch (55b12dc2, a86a0976). Not run through regen-readme.py per the review task's instructions - the aggregated README.md is unchanged. --- .../Driver.Historian.Gateway/findings.md | 239 ++++++++++++++++++ 1 file changed, 239 insertions(+) create mode 100644 code-reviews/Driver.Historian.Gateway/findings.md diff --git a/code-reviews/Driver.Historian.Gateway/findings.md b/code-reviews/Driver.Historian.Gateway/findings.md new file mode 100644 index 00000000..76ce846a --- /dev/null +++ b/code-reviews/Driver.Historian.Gateway/findings.md @@ -0,0 +1,239 @@ +# Code Review — Driver.Historian.Gateway + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway` | +| Reviewer | Claude Code | +| Review date | 2026-08-10 | +| Commit reviewed | `b990adae` | +| Status | Reviewed | +| Open findings | 0 | + +## Scope + +The sole historian backend: a thin gRPC seam (`IHistorianGatewayClient` / +`HistorianGatewayClientAdapter`) over the `ZB.MOM.WW.HistorianGateway.Client` package, plus +`GatewayHistorianDataSource` (HistoryRead), `GatewayAlarmHistorianWriter` (alarm `SendEvent`), +`GatewayTagProvisioner` (`EnsureTags` upsert), the `Mapping/` pure mappers, and the +`Recorder/` continuous-historization write path (`FasterLogHistorizationOutbox`, +`GatewayHistorianValueWriter`, `HistorizationOutboxEntrySerializer`). 17 source files, ~1,850 LOC +(excluding generated/obj); test suite `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests` +(130 tests: 124 offline unit + 6 env-gated `Category=LiveIntegration`, skip cleanly offline). + +Read against `CLAUDE.md`'s "Historian / HistoryRead" section in full before reviewing — several +things that look like defects on first read are deliberate, documented decisions (Boolean→Int2, +the dead `Quality` field on `WriteLiveValues`, the per-path-own-channel design, the two `EnsureTags` +gotchas, the #491/#537/#538 live-gate history). None of those were re-litigated here. + +## Checklist coverage + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | 1 finding (Driver.Historian.Gateway-001, High) — Resolved | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | No issues found | +| 5 | Security | No issues found | +| 6 | Performance & resource management | 1 finding (Driver.Historian.Gateway-002, Low) — Resolved | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | 1 finding (Driver.Historian.Gateway-003, Medium) — Resolved | +| 10 | Documentation & comments | No issues found | + +### Category notes + +**1. Correctness & logic bugs.** See Driver.Historian.Gateway-001 (Severity write/read mismatch). +Everything else checked out: `SampleMapper`/`GatewayQualityMapper` quality-byte bucketing is exact +and tested per-byte; `AggregateModeMapper`/`HistorianTypeMapper` are exhaustive-matrix-guarded +(`Every_..._member_is_classified` tests fail on a future unmapped enum member rather than silently +defaulting); `GatewayReadEventsTests` confirms the truncation/continuation-point boundary +(`collected.Count == maxEvents` checked *before* `Add`, so an exact-cap read has no continuation but +one more matching event sets it); `AlignAtTimeSnapshots` correctly keeps the first of a duplicate +timestamp and Bad-fills gaps at the *requested* time, not the nearest returned one. + +**2. OtOpcUa conventions.** Driver-level code contains no OPC UA SDK reference (`uint` status codes +via the named `HistorianStatusCodes`, not the SDK's `Opc.Ua.StatusCodes` — matches the repo's stated +pattern of keeping the driver layer SDK-free so `StatusCodeParityTests` elsewhere can reflect-guard +it). No authorization decisions are made in this driver — HistoryRead access-level gating happens at +materialization in the server layer per CLAUDE.md, and this module is a pure translation seam. +Serilog-style `ILogger` used throughout via constructor injection. + +**3. Concurrency & thread safety.** `GatewayHistorianDataSource`'s health counters +(`_totalQueries`/`_totalSuccesses`/`_totalFailures`/etc.) are all mutated and read under one +`_healthLock`, preserving the `TotalQueries == TotalSuccesses + TotalFailures` invariant at every +observed snapshot (per its own XML remarks and `GatewayHealthSnapshotTests`). +`FasterLogHistorizationOutbox` documents an explicit single-writer assumption (the recorder actor +processes messages sequentially) and its `_state` lock protects only the in-memory FIFO index, never +held across an `await` — correct, since FasterLog itself tolerates concurrent enqueue/scan. No shared +mutable state was found that lacked a lock or a documented ownership model. + +**4. Error handling & resilience.** `GatewayAlarmHistorianWriter` and `GatewayHistorianValueWriter` +both honor their "never throws" contracts (verified by dedicated exception-classification tests +covering the typed client hierarchy, a defensive raw `RpcException` path, and an unclassifiable +exception default). Cancellation is handled as a first-class, non-poisoning case in both the alarm +writer (mid-drain cancellation yields `RetryPlease`, never `PermanentFail`) and the tag provisioner +(a shutdown `OperationCanceledException` is logged quietly and counted `Failed`, never thrown). The +fire-and-forget `EnsureTagsAsync` hook is genuinely non-blocking end to end — no `await` in its +caller's synchronous path can escape as an exception (verified by tracing `AddressSpaceApplier`'s +fire-and-forget dispatch, outside this module's scope but confirmed non-blocking here). + +**5. Security.** No API key, tag value, hostname, or credential is ever passed to a logger call +anywhere in the module (audited every `_logger.Log*` call site) — failures are logged by exception +*type name* only. `HistorianGatewayClientAdapter.Create` echoes the `Endpoint` in its +`InvalidOperationException` (deliberately, documented as non-secret) but never the `ApiKey`. TLS / +certificate-validation options (`UseTls`, `AllowUntrustedServerCertificate`, `CaCertificatePath`) map +onto the package client with the documented inverted `RequireCertificateValidation` semantics and are +correctly *not* forwarded on a plaintext h2c connection (pinned by +`Adapter_ignores_tls_only_settings_on_a_plaintext_endpoint`). No SQL or other injectable surface in +this module. + +**6. Performance & resource management.** See Driver.Historian.Gateway-002 (missing dispose on +`GatewayTagProvisioner`). Everything else: `GatewayHistorianDataSource` and +`GatewayAlarmHistorianWriter` both correctly dispose their owned `IHistorianGatewayClient` (verified +by dedicated tests); the three `GatewayHistorian.Create*` factories deliberately open independent +gRPC channels per path rather than sharing one, a documented and reasonable trade (each channel's +lifetime stays simple; the gateway pools/amortizes historian sessions server-side). Channel +construction is lazy (no network I/O at construction, confirmed by the adapter's own tests) so a +misconfigured/disabled path never dials. Per-call deadlines exist via +`ServerHistorianOptions.CallTimeout` → `HistorianGatewayClientOptions.DefaultCallTimeout`, and every +seam method threads a `CancellationToken` through to the package client — no unbounded call was found. + +**7. Design-document adherence.** Matches `CLAUDE.md`'s "Historian / HistoryRead" section point for +point: gateway is the sole backend; Boolean→Int2 is implemented exactly as documented (with an +explicit-presence pin test guarding the proto3-optional contract); the continuous-historization +outbox/writer/serializer shape matches the FasterLog-outbox design; the `Quality` field is +deliberately populated despite being a documented-dead input on the SQL write path, with the +reasoning captured in the class's own remarks (proto3 default-0 would be a worse lie than the +correct-but-ignored 192). No stale documentation was found in this module. + +**8. Code organization & conventions.** Clean separation: `Mapping/` holds pure, stateless, +internal-visibility mappers (each independently unit-tested); `Recorder/` holds the +continuous-historization write path; the top-level files are the three public seams +(`IHistorianGatewayClient`, the adapter, and the `GatewayHistorian` factory facade the Host consumes). +`InternalsVisibleTo` is scoped to the test project only. The `ServerHistorianOptions` type living in +`Runtime` rather than this driver (creating the documented Host→{Runtime, Gateway}, Gateway→Runtime +diamond) is explained inline in the `.csproj` and is a reasonable ownership call, not a layering +violation. + +**9. Testing coverage.** See Driver.Historian.Gateway-003 (two offline-coverage gaps, now closed). +Otherwise the suite is thorough: every mapper has a matrix-guard test for unmapped enum members, +every writer's outcome-classification switch is covered by every named branch (including the +FIFO-truncation semantics of the FasterLog outbox — append/peek/remove/restart-durability/capacity +drop-oldest/periodic-commit-recovery all have dedicated tests), and the six env-gated +`Category=LiveIntegration` tests (skip cleanly without `HISTGW_*` env vars, confirmed offline) cover +the write/read/alarm/EnsureTags round trips against a real gateway per `docs/Historian.md`'s +re-run recipe. + +**10. Documentation & comments.** XML docs are extensive, accurate against the code they describe, +and several carry load-bearing historical context (e.g. why each `GatewayHistorian.Create*` factory +opens its own channel, why `Quality` is populated despite being server-ignored, why +`HistorianEvent.Id` is deliberately left unset). No misleading or stale comment was found. + +## Findings + +### Driver.Historian.Gateway-001 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs:39` (write side) and `Mapping/EventMapper.cs:62` (read side) | +| Status | Resolved | + +**Description:** `AlarmEventMapper.ToHistorianEvent` wrote the alarm's `Severity` wire property as +`alarm.Severity.ToString()` — the .NET enum's *name* (`"Low"`/`"Medium"`/`"High"`/`"Critical"`). +`AlarmHistorianEvent.Severity`'s own XML doc states the contract: "Mapped to Historian's numeric +priority on the sink side." The read side, `EventMapper.ParseSeverity`, `int.TryParse`s that exact +property and falls back to the minimum severity (1) on any unparseable value — which an enum name +always is. So round-tripping any alarm event's severity through this module's own write mapper then +its own read mapper silently collapsed every severity to the minimum, regardless of the real +severity: a `Critical` alarm and a `Low` alarm would read back identically as severity 1. This is a +self-contained defect (both mappers live in this module) independent of whether the gateway's own +storage happens to echo arbitrary custom event properties back on `ReadEvents` — the write and read +mappers disagreed with each other and with their own documented contract, and the existing test +(`AlarmEventMapperTests.Maps_source_time_type_and_rich_properties`) asserted the *wrong* value +(`"High"`) as correct, so no test caught it. + +**Recommendation:** Map `AlarmSeverity` to a numeric OPC UA Part 9 priority string before writing it +to the wire property, using the same severity convention already established elsewhere in the +codebase for this exact enum (`ZB.MOM.WW.OtOpcUa.Runtime.Drivers.NativeAlarmProjector.MapSeverity`: +Low=200, Medium=500, High=700, Critical=900). + +**Resolution:** `55b12dc2` (branch `review/historian-gateway`), 2026-08-10. Added a private +`AlarmEventMapper.ToNumericSeverity` mirroring `NativeAlarmProjector.MapSeverity`'s convention and +routed the `Severity` property through it instead of `alarm.Severity.ToString()`. TDD: added +`AlarmEventMapperTests.Severity_round_trips_through_EventMapper_as_numeric_priority` (a +`[Theory]` over all four `AlarmSeverity` values, round-tripping through both mappers) and corrected +the existing assertion to `"700"`; both were RED against the pre-fix code (confirmed by running the +suite before the source change) and are GREEN after it. All 130 module tests pass +(124 run + 6 skipped `LiveIntegration`). + +### Driver.Historian.Gateway-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Performance & resource management | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs` | +| Status | Resolved | + +**Description:** Unlike its two siblings — `GatewayHistorianDataSource` (`IAsyncDisposable`, +disposes its `IHistorianGatewayClient`) and `GatewayAlarmHistorianWriter` (`IDisposable`, same) — +`GatewayTagProvisioner` implemented neither `IDisposable` nor `IAsyncDisposable`, so the gRPC channel +`GatewayHistorian.CreateProvisioner` opens for it was never closed. `IHistorianProvisioning` (the +interface it's registered as) does not declare disposal either, so nothing in the codebase had any +way to close that channel. Impact is bounded — `AddHistorianProvisioning` registers it as a single +process-lifetime DI singleton (built once at startup, not per-deploy), so this was not an +accumulating leak — but it is a real gap in graceful-shutdown hygiene and an inconsistency with the +established pattern in this same module. + +**Recommendation:** Implement `IAsyncDisposable` on `GatewayTagProvisioner`, disposing the +underlying `IHistorianGatewayClient` — the built-in `Microsoft.Extensions.DependencyInjection` +container tracks and disposes any singleton instance it constructs that implements +`IDisposable`/`IAsyncDisposable`, regardless of the declared service-interface type, so no other +wiring change is required. + +**Resolution:** `55b12dc2` (branch `review/historian-gateway`), 2026-08-10. Added +`GatewayTagProvisioner : IHistorianProvisioning, IAsyncDisposable` with a `DisposeAsync()` delegating +to `_client.DisposeAsync()`. TDD: added +`GatewayTagProvisionerTests.DisposeAsync_disposes_underlying_client`, confirmed RED (compile error — +`GatewayTagProvisioner` did not implement `IAsyncDisposable`) before the fix, GREEN after. + +### Driver.Historian.Gateway-003 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Testing coverage | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Recorder/GatewayHistorianValueWriter.cs`, `Recorder/HistorizationOutboxEntrySerializer.cs` | +| Status | Resolved | + +**Description:** Two production-critical pieces of the continuous-historization write path had no +offline unit coverage: + +- `GatewayHistorianValueWriter` — the recorder's `WriteLiveValues` adapter, and per CLAUDE.md's + "KNOWN LIMITATION 2" history the exact class where the #491 `ConfigureAwait(false)`-destroys-`Self` + defect and the #537/#538 gate findings were discovered live — was exercised only by the env-gated + `Category=LiveIntegration` suite (`tests/.../Live/GatewayLiveIntegrationTests.cs`), which skips + cleanly offline. Its ack-to-bool mapping (`Success || Queued`), timestamp-presence handling (a null + timestamp must leave the proto field unset, not send a default), and non-throwing exception + contract had zero offline verification. +- `HistorizationOutboxEntrySerializer` — a hand-rolled fixed-layout little-endian binary + (de)serializer with manual span-offset arithmetic — was only exercised *indirectly* through + `FasterLogHistorizationOutboxTests`, and only ever with single-character ASCII tag names ("A"/"B"/ + "C"), never testing the one part of the layout arithmetic (the UTF-8 *byte*-length prefix vs. the + tag's *char* length) that a multi-byte tag name would exercise. + +**Recommendation:** Add direct offline unit tests for `GatewayHistorianValueWriter` (reusing the +existing `FakeHistorianGatewayClient` test double already used by every other seam consumer in this +module) and for `HistorizationOutboxEntrySerializer` (direct Serialize/Deserialize round trips, +including a multi-byte UTF-8 tag name, an empty tag, and boundary `ushort`/`double` values). + +**Resolution:** `a86a0976` (branch `review/historian-gateway`), 2026-08-10. Added +`tests/.../Recorder/GatewayHistorianValueWriterTests.cs` (10 tests: success/queued/soft-failure ack +mapping, exception swallowing, empty-batch short-circuit, timestamp presence/absence, quality/value +forwarding, null-argument guards, dispose delegation) and +`tests/.../Recorder/HistorizationOutboxEntrySerializerTests.cs` (4 tests: full round trip incl. +`DateTimeKind` preservation, a multi-byte UTF-8 tag `"Pumpe_Ω_日本語_Temp"`, an empty tag, and +`double.MinValue`/`ushort.MaxValue` boundaries). All 14 new tests pass; full module suite is +124 passed / 6 skipped (LiveIntegration) / 0 failed.