From 205b07f6aab5713b109cba818b08571e77146410 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:21:46 -0400 Subject: [PATCH 1/5] fix(driver-historian-wonderware): resolve Medium code-review finding (Driver.Historian.Wonderware-002) Normalise req.Events to Array.Empty() immediately after MessagePack deserialization in HandleWriteAlarmEventsAsync. MessagePack deserializes an absent or explicit-nil array field as null, not Array.Empty, so a peer that sends a null Events array would trigger a NullReferenceException on either .Length dereference (no-writer branch or catch block), leaving the client correlation-id wait hanging with no reply frame. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.Historian.Wonderware/findings.md | 4 ++-- .../Ipc/HistorianFrameHandler.cs | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/code-reviews/Driver.Historian.Wonderware/findings.md b/code-reviews/Driver.Historian.Wonderware/findings.md index 86c70d1..bbfa113 100644 --- a/code-reviews/Driver.Historian.Wonderware/findings.md +++ b/code-reviews/Driver.Historian.Wonderware/findings.md @@ -63,7 +63,7 @@ the reconnect path can re-open with `ReadOnly = false`) or at minimum as | Severity | Medium | | Category | Correctness and logic bugs | | Location | `Ipc/HistorianFrameHandler.cs:162`, `:181` | -| Status | Open | +| Status | Resolved | **Description:** `HandleWriteAlarmEventsAsync` dereferences `req.Events.Length` in both the `_alarmWriter is null` branch (line 162) and the catch block (line @@ -79,7 +79,7 @@ already null-guards `events`; the frame handler does not. immediately after deserialization (or guard each `.Length` access), consistent with the null-tolerance the writer already has. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — normalise `req.Events` to `Array.Empty()` immediately after deserialization so all subsequent `.Length` accesses are safe against null frames. ### Driver.Historian.Wonderware-003 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Ipc/HistorianFrameHandler.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Ipc/HistorianFrameHandler.cs index da85830..3779803 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Ipc/HistorianFrameHandler.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Ipc/HistorianFrameHandler.cs @@ -153,6 +153,11 @@ public sealed class HistorianFrameHandler : IFrameHandler private async Task HandleWriteAlarmEventsAsync(byte[] body, FrameWriter writer, CancellationToken ct) { var req = MessagePackSerializer.Deserialize(body); + + // MessagePack deserializes an absent or explicit-nil array as null, not Array.Empty. + // Normalise here so every path below can safely dereference .Length without an NRE. + req.Events ??= Array.Empty(); + var reply = new WriteAlarmEventsReply { CorrelationId = req.CorrelationId }; if (_alarmWriter is null) From 02daacbfd007f81afa78e66beab25c8c2ec51358 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:23:00 -0400 Subject: [PATCH 2/5] fix(driver-historian-wonderware): resolve Medium code-review finding (Driver.Historian.Wonderware-003) Extract the string-vs-numeric value selection from raw and at-time read loops into a SelectValue helper method. aahClientManaged's HistoryQueryResult has no data-type field in the bound SDK version, so the heuristic (prefer StringValue when non-empty and Value==0) is unavoidable; the helper now documents the limitation explicitly in its XML doc so the known edge case (numeric tag at exactly zero with a formatted StringValue) is self-evident. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Driver.Historian.Wonderware/findings.md | 4 +- .../Backend/HistorianDataSource.cs | 39 ++++++++++++------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/code-reviews/Driver.Historian.Wonderware/findings.md b/code-reviews/Driver.Historian.Wonderware/findings.md index bbfa113..d154c7b 100644 --- a/code-reviews/Driver.Historian.Wonderware/findings.md +++ b/code-reviews/Driver.Historian.Wonderware/findings.md @@ -88,7 +88,7 @@ with the null-tolerance the writer already has. | Severity | Medium | | Category | Correctness and logic bugs | | Location | `Backend/HistorianDataSource.cs:320-323`, `:457-460` | -| Status | Open | +| Status | Resolved | **Description:** Raw and at-time reads decide whether a sample is a string or a numeric with `if (!string.IsNullOrEmpty(result.StringValue) && result.Value == 0)`. @@ -106,7 +106,7 @@ field rather than from `Value == 0`. If the type field is genuinely unavailable the bound SDK version, document the limitation explicitly and prefer numeric for analog/integer tags. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — extracted the heuristic into a `SelectValue` helper with a detailed XML doc comment explaining the SDK limitation (`HistoryQueryResult` has no data type field in the bound `aahClientManaged` version); the existing `Value == 0` discriminator is preserved as the best available heuristic with the known edge-case documented. ### Driver.Historian.Wonderware-004 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Backend/HistorianDataSource.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Backend/HistorianDataSource.cs index 93b796c..770e7cb 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Backend/HistorianDataSource.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Backend/HistorianDataSource.cs @@ -316,15 +316,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Backend var result = query.QueryResult; var timestamp = DateTime.SpecifyKind(result.StartDateTime, DateTimeKind.Utc); - object? value; - if (!string.IsNullOrEmpty(result.StringValue) && result.Value == 0) - value = result.StringValue; - else - value = result.Value; - results.Add(new HistorianSample { - Value = value, + Value = SelectValue(result), TimestampUtc = timestamp, Quality = (byte)(result.OpcQuality & 0xFF), }); @@ -453,15 +447,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Backend if (query.MoveNext(out error)) { var result = query.QueryResult; - object? value; - if (!string.IsNullOrEmpty(result.StringValue) && result.Value == 0) - value = result.StringValue; - else - value = result.Value; - results.Add(new HistorianSample { - Value = value, + Value = SelectValue(result), TimestampUtc = DateTime.SpecifyKind(timestamp, DateTimeKind.Utc), Quality = (byte)(result.OpcQuality & 0xFF), }); @@ -574,6 +562,29 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Backend #pragma warning restore CS0618 } + /// + /// Selects the typed value from a row. + /// + /// SDK limitation: HistoryQueryResult exposes only Value + /// (double) and StringValue (string) — there is no tag data-type field on + /// the result. The correct approach would be to branch on the tag's declared + /// data type, but the bound version of aahClientManaged does not surface + /// it per query result. The heuristic below is the best available: prefer + /// StringValue only when it is non-empty AND Value is zero, + /// because string tags in the Historian SDK always project to Value=0 + /// while numeric tags may legitimately sample to zero (in which case the SDK + /// does not populate StringValue). A numeric tag at exactly zero with a + /// non-empty formatted StringValue (e.g. "0.00") would be mis-reported + /// as a string; this is a known edge case of the SDK binding. + /// + /// + private static object? SelectValue(HistoryQueryResult result) + { + if (!string.IsNullOrEmpty(result.StringValue) && result.Value == 0) + return result.StringValue; + return result.Value; + } + internal static double? ExtractAggregateValue(AnalogSummaryQueryResult result, string column) { switch (column) From 74746319920cecec087e88c5d49e4b245cc46c5f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:23:42 -0400 Subject: [PATCH 3/5] fix(driver-historian-wonderware): resolve Medium code-review finding (Driver.Historian.Wonderware-006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add exponential backoff (250 ms → 500 ms → 1 s → 2 s → 4 s → 8 s cap) to PipeServer.RunAsync after each connection-loop exception, replacing the spin loop that previously pegged a CPU core and flooded the log on persistent errors such as a duplicate pipe name or a failing PipeAcl.Create. After 20 consecutive failures the method re-throws so the SCM / NSSM supervisor can restart the sidecar cleanly. A clean connection (even a short-lived one) resets the counter. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Driver.Historian.Wonderware/findings.md | 4 +- .../Ipc/PipeServer.cs | 51 +++++++++++++++++-- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/code-reviews/Driver.Historian.Wonderware/findings.md b/code-reviews/Driver.Historian.Wonderware/findings.md index d154c7b..4052a95 100644 --- a/code-reviews/Driver.Historian.Wonderware/findings.md +++ b/code-reviews/Driver.Historian.Wonderware/findings.md @@ -161,7 +161,7 @@ lock), so the snapshot is internally consistent. | Severity | Medium | | Category | Error handling and resilience | | Location | `Ipc/PipeServer.cs:120-128` | -| Status | Open | +| Status | Resolved | **Description:** `RunAsync` re-accepts connections in a `while` loop. If `RunOneConnectionAsync` throws synchronously and immediately on every iteration @@ -175,7 +175,7 @@ seconds) before re-accepting after a caught exception, and consider a consecutive-failure threshold that escalates to a fatal exit so the supervisor can restart the sidecar cleanly. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added exponential backoff (250 ms → 8 s, six steps) after each connection-loop failure and a `MaxConsecutiveFailures=20` threshold that re-throws so the SCM/NSSM supervisor can restart the sidecar cleanly. ### Driver.Historian.Wonderware-007 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Ipc/PipeServer.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Ipc/PipeServer.cs index 8737bc9..d20fe49 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Ipc/PipeServer.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Ipc/PipeServer.cs @@ -113,17 +113,62 @@ public sealed class PipeServer : IDisposable } } + // Backoff sequence for consecutive RunOneConnection failures: 250 ms → 500 ms → + // 1 000 ms → 2 000 ms → 4 000 ms → capped at 8 000 ms thereafter. + private static readonly TimeSpan[] BackoffSteps = + { + TimeSpan.FromMilliseconds(250), + TimeSpan.FromMilliseconds(500), + TimeSpan.FromSeconds(1), + TimeSpan.FromSeconds(2), + TimeSpan.FromSeconds(4), + TimeSpan.FromSeconds(8), + }; + + /// + /// Maximum consecutive failures before the server gives up and lets the process exit + /// so the supervisor (NSSM / SCM) can restart the sidecar cleanly. + /// + private const int MaxConsecutiveFailures = 20; + /// /// Runs the server continuously, handling one connection at a time. When a connection - /// ends (clean or error), accepts the next. + /// ends (clean or error), waits with exponential backoff before accepting the next. + /// If consecutive failures occur the method + /// throws so the supervisor can restart the sidecar. /// public async Task RunAsync(IFrameHandler handler, CancellationToken ct) { + var consecutiveFailures = 0; + while (!ct.IsCancellationRequested) { - try { await RunOneConnectionAsync(handler, ct).ConfigureAwait(false); } + try + { + await RunOneConnectionAsync(handler, ct).ConfigureAwait(false); + consecutiveFailures = 0; // a clean connection (even a short-lived one) resets the counter + } catch (OperationCanceledException) { break; } - catch (Exception ex) { _logger.Error(ex, "Sidecar IPC connection loop error — accepting next"); } + catch (Exception ex) + { + consecutiveFailures++; + + if (consecutiveFailures >= MaxConsecutiveFailures) + { + _logger.Fatal(ex, + "Sidecar IPC connection loop failed {Count} consecutive times — giving up so supervisor can restart", + consecutiveFailures); + throw; + } + + var delay = BackoffSteps[Math.Min(consecutiveFailures - 1, BackoffSteps.Length - 1)]; + _logger.Error(ex, + "Sidecar IPC connection loop error (consecutive failure {Count}/{Max}) — retrying in {Delay}", + consecutiveFailures, MaxConsecutiveFailures, delay); + + try { await Task.Delay(delay, ct).ConfigureAwait(false); } + catch (OperationCanceledException) { break; } + } } } From 1723f5d5cd5bb6e9a8e70add4076bba42835eb17 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:24:25 -0400 Subject: [PATCH 4/5] fix(driver-historian-wonderware): resolve Medium code-review finding (Driver.Historian.Wonderware-009) Apply _config.MaxValuesPerRead as a bucket cap in ReadAggregateAsync, mirroring the existing cap in ReadRawAsync. Without this guard a processed read over a wide time range with a small IntervalMs could accumulate an unbounded HistorianAggregateSample list; if the serialised reply exceeded the 16 MiB FrameWriter frame cap WriteAsync would throw and the client correlation-id wait would hang. Truncation now logs a Warning with a hint to widen IntervalMs or reduce the time range. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Driver.Historian.Wonderware/findings.md | 4 ++-- .../Backend/HistorianDataSource.cs | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/code-reviews/Driver.Historian.Wonderware/findings.md b/code-reviews/Driver.Historian.Wonderware/findings.md index 4052a95..70d9b9d 100644 --- a/code-reviews/Driver.Historian.Wonderware/findings.md +++ b/code-reviews/Driver.Historian.Wonderware/findings.md @@ -235,7 +235,7 @@ treat an SDK error as an empty history. | Severity | Medium | | Category | Performance and resource management | | Location | `Backend/HistorianDataSource.cs:382-395`, `Ipc/Contracts.cs:85-99` | -| Status | Open | +| Status | Resolved | **Description:** `ReadAggregateAsync` drains `query.MoveNext` into `results` with no upper bound, unlike `ReadRawAsync`, which honours `maxValues` / @@ -252,7 +252,7 @@ sidecar holds the whole result set in memory. `ReadProcessedRequest`. Reject or truncate result sets that would exceed the frame cap with an explicit error reply rather than letting `WriteAsync` throw. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — applied `_config.MaxValuesPerRead` as a bucket cap in `ReadAggregateAsync` mirroring the raw-read path; truncation logs a Warning with the limit and a hint to widen `IntervalMs` or reduce the time range. ### Driver.Historian.Wonderware-010 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Backend/HistorianDataSource.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Backend/HistorianDataSource.cs index 770e7cb..4e0def7 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Backend/HistorianDataSource.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware/Backend/HistorianDataSource.cs @@ -373,6 +373,12 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Backend return Task.FromResult(results); } + // Apply the same bucket cap as the raw-read path so a wide time range with a + // small IntervalMs cannot produce an unbounded result set that would overflow + // the 16 MiB FrameWriter frame cap and lose the entire reply. + var bucketLimit = _config.MaxValuesPerRead; + var bucketCount = 0; + while (query.MoveNext(out error)) { ct.ThrowIfCancellationRequested(); @@ -386,6 +392,15 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Backend Value = value, TimestampUtc = timestamp, }); + + bucketCount++; + if (bucketLimit > 0 && bucketCount >= bucketLimit) + { + Log.Warning( + "HistoryRead aggregate ({Aggregate}): {Tag} truncated at {Limit} buckets — widen IntervalMs or reduce time range", + aggregateColumn, tagName, bucketLimit); + break; + } } query.EndQuery(out _); From 0d10d30b7d6321585793b2d0920648e85eb3f41c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:24:36 -0400 Subject: [PATCH 5/5] fix(driver-historian-wonderware): update findings.md open count after resolving -002 -003 -006 -009 Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.Historian.Wonderware/findings.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code-reviews/Driver.Historian.Wonderware/findings.md b/code-reviews/Driver.Historian.Wonderware/findings.md index 70d9b9d..38be375 100644 --- a/code-reviews/Driver.Historian.Wonderware/findings.md +++ b/code-reviews/Driver.Historian.Wonderware/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 11 | +| Open findings | 7 | ## Checklist coverage