From 55b12dc2170c1cdc02df1e15ceb6cb0fdc44390c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:53:41 -0400 Subject: [PATCH 1/3] fix(historian-gateway): map alarm severity numerically; dispose provisioner's gRPC channel Driver.Historian.Gateway-001 (High): AlarmEventMapper wrote the alarm's Severity wire property as the .NET enum name (e.g. "High"), but EventMapper's read side int.TryParse()s that same property and silently falls back to the minimum severity on any unparseable value - so every alarm event's severity round-tripped through this module's own mappers collapsed to 1 (Low), contradicting AlarmHistorianEvent.Severity's own documented contract ("mapped to Historian's numeric priority"). Now mapped through a numeric priority mirroring the existing NativeAlarmProjector.MapSeverity convention (200/500/700/900). Driver.Historian.Gateway-002 (Low): GatewayTagProvisioner never disposed its IHistorianGatewayClient, unlike its GatewayHistorianDataSource / GatewayAlarmHistorianWriter siblings. Now implements IAsyncDisposable so the DI-owned singleton's gRPC channel closes gracefully at shutdown. Both TDD: new/corrected tests confirmed RED against the pre-fix code. --- .../GatewayTagProvisioner.cs | 11 ++++++- .../Mapping/AlarmEventMapper.cs | 22 ++++++++++++- .../GatewayTagProvisionerTests.cs | 21 +++++++++++++ .../Mapping/AlarmEventMapperTests.cs | 31 ++++++++++++++++++- 4 files changed, 82 insertions(+), 3 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs index 986f82ab..b160f04f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs @@ -17,7 +17,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway; /// as and returns. The method never throws and never /// logs tag values, hostnames, or credentials. /// -public sealed class GatewayTagProvisioner : IHistorianProvisioning +public sealed class GatewayTagProvisioner : IHistorianProvisioning, IAsyncDisposable { /// /// Storage rate (ms) stamped on every provisioned analog tag. The gateway's EnsureTags path @@ -104,4 +104,13 @@ public sealed class GatewayTagProvisioner : IHistorianProvisioning return new HistorianProvisionResult(requests.Count, Ensured: 0, Skipped: skipped, Failed: definitions.Count); } } + + /// + /// Disposes the underlying gateway client (and its gRPC channel). The DI container owns this + /// provisioner as a singleton (AddHistorianProvisioning), so this fires once at host + /// shutdown — closing the channel gracefully, matching the read () + /// and alarm-write () paths' dispose ownership. + /// + /// A task that represents the asynchronous operation. + public ValueTask DisposeAsync() => _client.DisposeAsync(); } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs index a0112dcd..bdda3fc5 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs @@ -1,5 +1,6 @@ using Google.Protobuf.WellKnownTypes; using ZB.MOM.WW.HistorianGateway.Contracts.Grpc; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian; namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Mapping; @@ -10,6 +11,25 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Mapping; /// internal static class AlarmEventMapper { + /// + /// Maps to the numeric OPC UA Part 9 priority (1-1000) carried in the + /// wire event's Severity property, per the + /// contract ("Mapped to Historian's numeric priority on the sink side"). The read side + /// () int.TryParses this same property, so it + /// must stay a numeric string — the enum's .ToString() name (e.g. "High") is NOT + /// parseable and used to silently collapse every read-back severity to the minimum (1). The + /// values mirror the existing native-alarm convention + /// (ZB.MOM.WW.OtOpcUa.Runtime.Drivers.NativeAlarmProjector.MapSeverity). + /// + private static string ToNumericSeverity(AlarmSeverity severity) => severity switch + { + AlarmSeverity.Low => "200", + AlarmSeverity.Medium => "500", + AlarmSeverity.High => "700", + AlarmSeverity.Critical => "900", + _ => "500", + }; + /// Maps an alarm historian event to a gateway event. /// The driver-agnostic alarm event. /// The gateway wire event ready for SendEvent. @@ -36,7 +56,7 @@ internal static class AlarmEventMapper historianEvent.Properties["AlarmId"] = alarm.AlarmId; historianEvent.Properties["AlarmName"] = alarm.AlarmName; historianEvent.Properties["EventKind"] = alarm.EventKind; - historianEvent.Properties["Severity"] = alarm.Severity.ToString(); + historianEvent.Properties["Severity"] = ToNumericSeverity(alarm.Severity); historianEvent.Properties["User"] = alarm.User; historianEvent.Properties["Message"] = alarm.Message; if (alarm.Comment is not null) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/GatewayTagProvisionerTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/GatewayTagProvisionerTests.cs index b03a9af9..798a0dca 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/GatewayTagProvisionerTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/GatewayTagProvisionerTests.cs @@ -149,6 +149,27 @@ public sealed class GatewayTagProvisionerTests Assert.Equal(1, result.Failed); // non-blocking: no throw } + /// + /// Driver.Historian.Gateway-002: unlike its siblings (GatewayHistorianDataSource, + /// GatewayAlarmHistorianWriter), had no dispose path at + /// all — the gRPC channel GatewayHistorian.CreateProvisioner opens was never closed. The + /// provisioner is registered as a process-lifetime DI singleton + /// (AddHistorianProvisioning), and the built-in container disposes any singleton instance + /// it creates that implements / — regardless + /// of the registered service-interface type — so implementing it here is sufficient for a clean + /// shutdown with no other wiring change. + /// + [Fact] + public async Task DisposeAsync_disposes_underlying_client() + { + var fake = new FakeHistorianGatewayClient(); + var p = Provisioner(fake); + + await ((IAsyncDisposable)p).DisposeAsync(); + + Assert.Equal(1, fake.DisposeCallCount); + } + [Fact] public async Task Ensured_count_reflects_successful_results() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Mapping/AlarmEventMapperTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Mapping/AlarmEventMapperTests.cs index f26784ec..e0b489b1 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Mapping/AlarmEventMapperTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Mapping/AlarmEventMapperTests.cs @@ -23,12 +23,41 @@ public sealed class AlarmEventMapperTests Assert.Equal("A1", e.Properties["AlarmId"]); Assert.Equal("HiHi", e.Properties["AlarmName"]); Assert.Equal("Activated", e.Properties["EventKind"]); - Assert.Equal("High", e.Properties["Severity"]); + // Severity is the AlarmHistorianEvent.Severity XML doc contract: "Mapped to Historian's + // numeric priority on the sink side" — NOT the .NET enum name. EventMapper.ParseSeverity on + // the read side int.TryParse()s this same property, so a non-numeric string (the old bug: + // alarm.Severity.ToString() == "High") silently always read back as the minimum severity (1). + Assert.Equal("700", e.Properties["Severity"]); Assert.Equal("operator1", e.Properties["User"]); Assert.Equal("ack note", e.Properties["Comment"]); Assert.Equal("Temp high", e.Properties["Message"]); } + /// + /// Driver.Historian.Gateway-001 regression: the write side () and + /// the read side () must agree on the wire representation of severity — + /// a numeric OPC UA Part 9 priority string, matching the convention already used elsewhere in the + /// codebase for the same enum (NativeAlarmProjector.MapSeverity: Low=200/Medium=500/ + /// High=700/Critical=900). Round-tripping through both mappers must reproduce the numeric severity, + /// not collapse to the minimum every time. + /// + [Theory] + [InlineData(AlarmSeverity.Low, (ushort)200)] + [InlineData(AlarmSeverity.Medium, (ushort)500)] + [InlineData(AlarmSeverity.High, (ushort)700)] + [InlineData(AlarmSeverity.Critical, (ushort)900)] + public void Severity_round_trips_through_EventMapper_as_numeric_priority(AlarmSeverity severity, ushort expected) + { + var a = new AlarmHistorianEvent("A1", "Area/Line/Pump1", "HiHi", "LimitAlarm", + severity, "Activated", "m", "operator1", null, + new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc)); + + var wire = AlarmEventMapper.ToHistorianEvent(a); + var readBack = EventMapper.ToHistoricalEvent(wire); + + Assert.Equal(expected, readBack.Severity); + } + [Fact] public void Null_comment_is_omitted_not_null() { From a86a09767c430822afddb77c90c65ca38d41ebd8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:53:47 -0400 Subject: [PATCH 2/3] test(historian-gateway): cover GatewayHistorianValueWriter + HistorizationOutboxEntrySerializer offline Driver.Historian.Gateway-003 (Medium): both were exercised only by the env-gated Category=LiveIntegration suite (skips cleanly offline) or only indirectly through FasterLogHistorizationOutboxTests with single-char ASCII tags. Adds direct offline unit coverage for the value writer's ack-to-bool mapping, timestamp presence/absence, and non-throwing contract (via the existing FakeHistorianGatewayClient double), and for the serializer's Serialize/Deserialize round trip incl. a multi-byte UTF-8 tag, an empty tag, and ushort/double boundaries. --- .../GatewayHistorianValueWriterTests.cs | 149 ++++++++++++++++++ ...HistorizationOutboxEntrySerializerTests.cs | 73 +++++++++ 2 files changed, 222 insertions(+) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/GatewayHistorianValueWriterTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/HistorizationOutboxEntrySerializerTests.cs diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/GatewayHistorianValueWriterTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/GatewayHistorianValueWriterTests.cs new file mode 100644 index 00000000..3367cfa2 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/GatewayHistorianValueWriterTests.cs @@ -0,0 +1,149 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; +using ZB.MOM.WW.HistorianGateway.Contracts.Grpc; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Recorder; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests.Recorder; + +/// +/// Driver.Historian.Gateway-003 (offline unit coverage for the continuous-historization write path): +/// before this file, — the recorder's WriteLiveValues +/// adapter, in production the write path CLAUDE.md's #491/#537/#538 live-gate history bit hard — was +/// exercised only by the env-gated Category=LiveIntegration suite, which skips cleanly offline. +/// These tests use the same the rest of the module's offline +/// tests share. +/// +public sealed class GatewayHistorianValueWriterTests +{ + private static GatewayHistorianValueWriter Writer(FakeHistorianGatewayClient fake) => + new(fake, NullLogger.Instance); + + [Fact] + public async Task Success_ack_returns_true() + { + var fake = new FakeHistorianGatewayClient { WriteLiveValuesResult = new WriteAck { Success = true } }; + var values = new[] { new HistorizationValue(DateTime.UtcNow, 42.0, 192) }; + + var ok = await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + Assert.True(ok); + Assert.Equal(1, fake.WriteLiveValuesCallCount); + Assert.Equal("T", fake.LastWriteLiveTag); + } + + [Fact] + public async Task Queued_ack_is_treated_as_success() + { + // A value the gateway durably store-forward-queued must not be re-drained by the recorder. + var fake = new FakeHistorianGatewayClient + { + WriteLiveValuesResult = new WriteAck { Success = false, Queued = true }, + }; + var values = new[] { new HistorizationValue(DateTime.UtcNow, 1.0, 192) }; + + var ok = await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + Assert.True(ok); + } + + [Fact] + public async Task Soft_failure_ack_returns_false_for_retry() + { + var fake = new FakeHistorianGatewayClient + { + WriteLiveValuesResult = new WriteAck { Success = false, Queued = false }, + }; + var values = new[] { new HistorizationValue(DateTime.UtcNow, 1.0, 192) }; + + var ok = await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + Assert.False(ok); + } + + [Fact] + public async Task Gateway_exception_is_swallowed_and_returns_false() + { + // NEVER throw out of the writer — the recorder's drain loop expects a plain bool. + var fake = new FakeHistorianGatewayClient { WriteLiveValuesThrows = new InvalidOperationException("boom") }; + var values = new[] { new HistorizationValue(DateTime.UtcNow, 1.0, 192) }; + + var ok = await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + Assert.False(ok); + } + + [Fact] + public async Task Empty_batch_is_trivially_acked_without_calling_the_gateway() + { + var fake = new FakeHistorianGatewayClient(); + + var ok = await Writer(fake).WriteLiveValuesAsync( + "T", Array.Empty(), TestContext.Current.CancellationToken); + + Assert.True(ok); + Assert.Equal(0, fake.WriteLiveValuesCallCount); + } + + [Fact] + public async Task Present_timestamp_is_sent_and_absent_timestamp_leaves_the_proto_field_unset() + { + var fake = new FakeHistorianGatewayClient(); + var stamped = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + var values = new[] + { + new HistorizationValue(stamped, 1.0, 192), + new HistorizationValue(null, 2.0, 192), + }; + + await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + var sent = fake.LastWriteLiveValues!; + Assert.Equal(2, sent.Count); + Assert.Equal(stamped, sent[0].Timestamp!.ToDateTime()); + Assert.Null(sent[1].Timestamp); // absent -> unset proto field -> gateway server-stamps now + } + + [Fact] + public async Task Quality_and_numeric_value_are_forwarded_unchanged() + { + var fake = new FakeHistorianGatewayClient(); + var values = new[] { new HistorizationValue(null, 123.5, 192) }; + + await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + var sent = fake.LastWriteLiveValues!; + Assert.Equal(123.5, sent[0].NumericValue); + Assert.Equal(192u, sent[0].Quality); + } + + [Fact] + public async Task Null_tag_throws() + { + var fake = new FakeHistorianGatewayClient(); + var values = new[] { new HistorizationValue(null, 1.0, 192) }; + + await Assert.ThrowsAsync(() => + Writer(fake).WriteLiveValuesAsync(null!, values, TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task Null_values_throws() + { + var fake = new FakeHistorianGatewayClient(); + + await Assert.ThrowsAsync(() => + Writer(fake).WriteLiveValuesAsync("T", null!, TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task DisposeAsync_disposes_underlying_client() + { + var fake = new FakeHistorianGatewayClient(); + var writer = Writer(fake); + + await writer.DisposeAsync(); + + Assert.Equal(1, fake.DisposeCallCount); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/HistorizationOutboxEntrySerializerTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/HistorizationOutboxEntrySerializerTests.cs new file mode 100644 index 00000000..0efdc6ae --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/HistorizationOutboxEntrySerializerTests.cs @@ -0,0 +1,73 @@ +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Recorder; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests.Recorder; + +/// +/// Driver.Historian.Gateway-003 (offline unit coverage): +/// exercises only indirectly, and only with short +/// ASCII tag names ("A"/"B"/"C"). The hand-rolled little-endian layout computes its tag byte-length +/// from Encoding.UTF8.GetByteCount before writing, so a multi-byte tag exercises the one part +/// of the layout arithmetic the indirect coverage never reaches. This asserts the (de)serializer +/// directly, in isolation from FasterLog. +/// +public sealed class HistorizationOutboxEntrySerializerTests +{ + [Fact] + public void Round_trips_all_fields() + { + var entry = new HistorizationOutboxEntry( + Guid.NewGuid(), "Pump1.Temp", 123.456, 192, + new DateTime(2026, 1, 1, 12, 34, 56, DateTimeKind.Utc)); + + var bytes = HistorizationOutboxEntrySerializer.Serialize(entry); + var round = HistorizationOutboxEntrySerializer.Deserialize(bytes); + + Assert.Equal(entry.Id, round.Id); + Assert.Equal(entry.Tag, round.Tag); + Assert.Equal(entry.NumericValue, round.NumericValue); + Assert.Equal(entry.Quality, round.Quality); + Assert.Equal(entry.TimestampUtc, round.TimestampUtc); + Assert.Equal(entry.TimestampUtc.Kind, round.TimestampUtc.Kind); // ToBinary/FromBinary round-trips Kind + } + + [Fact] + public void Round_trips_a_multi_byte_utf8_tag() + { + // The tag length prefix is a UTF-8 BYTE count, not a char count — a tag whose char count and + // byte count diverge (e.g. Ω is 2 bytes, 日 is 3) is the one case that would desync the layout + // if GetByteCount/GetBytes ever disagreed with what Deserialize slices out. + var entry = new HistorizationOutboxEntry( + Guid.NewGuid(), "Pumpe_Ω_日本語_Temp", -1.5, 0, DateTime.UtcNow); + + var round = HistorizationOutboxEntrySerializer.Deserialize( + HistorizationOutboxEntrySerializer.Serialize(entry)); + + Assert.Equal(entry.Tag, round.Tag); + Assert.Equal(entry.NumericValue, round.NumericValue); + } + + [Fact] + public void Round_trips_an_empty_tag() + { + var entry = new HistorizationOutboxEntry(Guid.NewGuid(), string.Empty, 0.0, 0, DateTime.UtcNow); + + var round = HistorizationOutboxEntrySerializer.Deserialize( + HistorizationOutboxEntrySerializer.Serialize(entry)); + + Assert.Equal(string.Empty, round.Tag); + } + + [Fact] + public void Round_trips_boundary_quality_and_negative_value() + { + var entry = new HistorizationOutboxEntry(Guid.NewGuid(), "T", double.MinValue, ushort.MaxValue, DateTime.UtcNow); + + var round = HistorizationOutboxEntrySerializer.Deserialize( + HistorizationOutboxEntrySerializer.Serialize(entry)); + + Assert.Equal(ushort.MaxValue, round.Quality); + Assert.Equal(double.MinValue, round.NumericValue); + } +} From 19feb1b2898e1a17b20abfa7d1e9f9e94824b68b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:54:25 -0400 Subject: [PATCH 3/3] 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.