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.TryParses 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.