lmxopcua/code-reviews/Driver.Historian.Gateway/findings.md

# 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<T>` 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.