From 57ae69a05e27267a7c66d103e8206943af78b364 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 16:21:22 -0400 Subject: [PATCH 01/10] fix(client): release a capped HistoryRead cursor, read (not release) follow-up pages, detach deleted subscriptions R08 1.4: raw and modified history reads share HistoryReadPaging. A read stopped by the caller's cap releases the server continuation point. The raw path also passed releaseContinuationPoints = (cp != null), which released page two instead of reading it, so every raw read was silently truncated to its first server page. R08 1.5: DefaultSubscriptionAdapter removes its Subscription from the SDK Session on delete/dispose (and a failed CreateAsync removes it too), so re-targeted alarm subscriptions no longer accumulate in Session.Subscriptions. Refs #747 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Adapters/DefaultSessionAdapter.cs | 145 +++++++----------- .../Adapters/DefaultSubscriptionAdapter.cs | 34 +++- .../Helpers/HistoryReadPaging.cs | 66 ++++++++ .../DefaultSubscriptionAdapterTests.cs | 57 +++++++ .../Helpers/HistoryReadPagingTests.cs | 144 +++++++++++++++++ 5 files changed, 355 insertions(+), 91 deletions(-) create mode 100644 src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs create mode 100644 tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/DefaultSubscriptionAdapterTests.cs create mode 100644 tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs index dd871594..c414f1c1 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs @@ -164,46 +164,16 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter ReturnBounds = false }; - var nodesToRead = new HistoryReadValueIdCollection - { - new HistoryReadValueId { NodeId = nodeId } - }; - - var allValues = new List(); - byte[]? continuationPoint = null; - - do - { - if (continuationPoint != null) - nodesToRead[0].ContinuationPoint = continuationPoint; - - // Use the async overload so this method is genuinely asynchronous, - // honors the cancellation token, and does not block the caller's thread - // (which would block the UI dispatcher for client.ui consumers). - var response = await _session.HistoryReadAsync( - null, - new ExtensionObject(details), - TimestampsToReturn.Source, - continuationPoint != null, - nodesToRead, - ct).ConfigureAwait(false); - - var results = response.Results; - if (results == null || results.Count == 0) - break; - - var result = results[0]; - // A refused read (BadHistoryOperationUnsupported, BadUserAccessDenied, ...) must surface; only - // BadNoData is an empty result. Swallowing it printed "0 values returned" for a refusal. - HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); - - if (result.HistoryData is ExtensionObject ext && ext.Body is HistoryData historyData) - allValues.AddRange(historyData.DataValues); - - continuationPoint = result.ContinuationPoint; - } while (continuationPoint != null && continuationPoint.Length > 0 && allValues.Count < maxValues); - - return allValues; + // Continuation pages are READ, never released, until the server finishes or the cap is reached; a read stopped + // by the cap releases the server's cursor (HistoryReadPaging). This path used to pass + // `releaseContinuationPoints: continuationPoint != null`, which RELEASED the second page instead of reading it, + // silently truncating every raw read to its first server page. + return await HistoryReadPaging.ReadAllAsync( + (cp, release, token) => ReadHistoryPageAsync(details, nodeId, cp, release, TimestampsToReturn.Source, token), + result => result.HistoryData is ExtensionObject { Body: HistoryData historyData } + ? historyData.DataValues + : [], + maxValues, nodeId, ct).ConfigureAwait(false); } /// @@ -219,59 +189,41 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter ReturnBounds = false }; + return await HistoryReadPaging.ReadAllAsync( + (cp, release, token) => ReadHistoryPageAsync(details, nodeId, cp, release, TimestampsToReturn.Both, token), + ToModifiedEntries, maxValues, nodeId, ct).ConfigureAwait(false); + } + + private static IEnumerable ToModifiedEntries(HistoryReadResult result) + { + if (result.HistoryData is not ExtensionObject { Body: HistoryModifiedData modified }) yield break; + for (var i = 0; i < modified.DataValues.Count; i++) + { + var info = i < modified.ModificationInfos.Count ? modified.ModificationInfos[i] : null; + yield return new Models.HistoryModifiedEntry( + modified.DataValues[i], + info?.ModificationTime ?? DateTime.MinValue, + info?.UpdateType ?? default, + info?.UserName); + } + } + + /// Issues one single-node HistoryRead (or a release of its continuation point). + private async Task ReadHistoryPageAsync( + ReadRawModifiedDetails details, NodeId nodeId, byte[]? continuationPoint, bool release, + TimestampsToReturn timestamps, CancellationToken ct) + { var nodesToRead = new HistoryReadValueIdCollection { - new HistoryReadValueId { NodeId = nodeId } + new HistoryReadValueId { NodeId = nodeId, ContinuationPoint = continuationPoint } }; - var all = new List(); - byte[]? continuationPoint = null; + // The async overload honours the token and does not block the caller's thread (the UI dispatcher for + // client.ui consumers). + var response = await _session.HistoryReadAsync( + null, new ExtensionObject(details), timestamps, release, nodesToRead, ct).ConfigureAwait(false); - do - { - if (continuationPoint != null) - nodesToRead[0].ContinuationPoint = continuationPoint; - - var response = await _session.HistoryReadAsync( - null, - new ExtensionObject(details), - TimestampsToReturn.Both, - false, - nodesToRead, - ct).ConfigureAwait(false); - - var results = response.Results; - if (results == null || results.Count == 0) - break; - - var result = results[0]; - HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); - - if (result.HistoryData is ExtensionObject ext && ext.Body is HistoryModifiedData modified) - { - for (var i = 0; i < modified.DataValues.Count; i++) - { - var info = i < modified.ModificationInfos.Count ? modified.ModificationInfos[i] : null; - all.Add(new Models.HistoryModifiedEntry( - modified.DataValues[i], - info?.ModificationTime ?? DateTime.MinValue, - info?.UpdateType ?? default, - info?.UserName)); - } - } - - continuationPoint = result.ContinuationPoint; - } while (continuationPoint != null && continuationPoint.Length > 0 && all.Count < maxValues); - - // A read stopped by the client's own cap leaves a server cursor behind; release it. - if (continuationPoint is { Length: > 0 }) - { - nodesToRead[0].ContinuationPoint = continuationPoint; - await _session.HistoryReadAsync(null, new ExtensionObject(details), TimestampsToReturn.Both, true, - nodesToRead, ct).ConfigureAwait(false); - } - - return all; + return response.Results is { Count: > 0 } results ? results[0] : null; } /// @@ -327,9 +279,22 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter }; _session.AddSubscription(subscription); - await subscription.CreateAsync(ct); + try + { + await subscription.CreateAsync(ct); + } + catch + { + // A subscription the server never created must not linger in Session.Subscriptions either. + try { await _session.RemoveSubscriptionAsync(subscription, CancellationToken.None); } + catch (Exception ex) { Logger.Warning(ex, "Error removing a subscription that failed to create"); } + throw; + } - return new DefaultSubscriptionAdapter(subscription); + // Deleting the adapter also removes the subscription from the SDK session, so a re-targeted alarm/data + // subscription does not accumulate dead Subscription objects in Session.Subscriptions. + return new DefaultSubscriptionAdapter(subscription, + async (s, token) => await _session.RemoveSubscriptionAsync(s, token).ConfigureAwait(false)); } /// diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs index deb15dd5..927e0094 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs @@ -12,14 +12,23 @@ internal sealed class DefaultSubscriptionAdapter : ISubscriptionAdapter private static readonly ILogger Logger = Log.ForContext(); private readonly Dictionary _monitoredItems = new(); private readonly Subscription _subscription; + private readonly Func? _detach; + private int _detached; /// /// Wraps a live OPC UA subscription so client code can manage monitored items through a testable abstraction. /// /// The underlying OPC UA subscription that owns monitored items for this client workflow. - public DefaultSubscriptionAdapter(Subscription subscription) + /// + /// Removes the subscription from its owning SDK Session once it is deleted. Without it a deleted + /// subscription stays in Session.Subscriptions until the session is disposed, so a UI that re-targets + /// alarms repeatedly accumulates dead subscriptions the SDK still iterates (transfer / recreate on reconnect). + /// + public DefaultSubscriptionAdapter(Subscription subscription, + Func? detach = null) { _subscription = subscription; + _detach = detach; } /// @@ -113,9 +122,24 @@ internal sealed class DefaultSubscriptionAdapter : ISubscriptionAdapter Logger.Warning(ex, "Error deleting subscription"); } + // Detach whether or not the server-side delete succeeded: the adapter is finished with it either way. + await DetachAsync(ct); _monitoredItems.Clear(); } + private async Task DetachAsync(CancellationToken ct) + { + if (_detach == null || Interlocked.Exchange(ref _detached, 1) != 0) return; + try + { + await _detach(_subscription, ct); + } + catch (Exception ex) + { + Logger.Warning(ex, "Error removing subscription from its session"); + } + } + /// /// Releases the wrapped OPC UA subscription and clears tracked monitored items held by the adapter. /// @@ -129,6 +153,14 @@ internal sealed class DefaultSubscriptionAdapter : ISubscriptionAdapter { } + try + { + DisposeBridge.Run(DetachAsync); + } + catch + { + } + _monitoredItems.Clear(); } } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs new file mode 100644 index 00000000..2ec33e93 --- /dev/null +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs @@ -0,0 +1,66 @@ +using Opc.Ua; +using Serilog; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Helpers; + +/// +/// The client-side HistoryRead paging loop shared by the raw and modified reads. Continuation pages are +/// READ (releaseContinuationPoints = false) while the caller still wants values; a read stopped by the +/// caller's own cap while the server still holds a cursor releases that cursor so it does not occupy one of the +/// session's continuation-point slots until eviction. +/// +internal static class HistoryReadPaging +{ + private static readonly ILogger Logger = Log.ForContext(typeof(HistoryReadPaging)); + + /// Reads (or, with , releases) one page. + /// The cursor to resume, or for the first page. + /// to release the cursor instead of reading. + /// The caller's cancellation token. + /// The per-node result, or when the response carried none. + public delegate Task PageReader( + byte[]? continuationPoint, bool releaseContinuationPoints, CancellationToken ct); + + /// Reads pages until the server finishes or the caller's cap is reached. + /// The element type extracted from each page. + /// Issues one HistoryRead for the single node. + /// Extracts the page's elements from a result. + /// The caller's cap; <= 0 means no cap (follow every continuation point). + /// The node being read, for refusal messages. + /// The caller's cancellation token. + /// Every element read. + /// A page was refused (see ). + public static async Task> ReadAllAsync( + PageReader readPage, Func> extract, int maxValues, NodeId nodeId, + CancellationToken ct) + { + var all = new List(); + byte[]? continuationPoint = null; + + while (true) + { + var result = await readPage(continuationPoint, false, ct).ConfigureAwait(false); + if (result == null) return all; + + HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); + all.AddRange(extract(result)); + + continuationPoint = result.ContinuationPoint; + if (continuationPoint is not { Length: > 0 }) return all; + if (maxValues > 0 && all.Count >= maxValues) break; + } + + // Stopped by the client's own cap: the server still holds a cursor for this session. Release it; failing to + // is not worth failing a read whose values are already in hand. + try + { + await readPage(continuationPoint, true, ct).ConfigureAwait(false); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + Logger.Warning(ex, "Releasing the HistoryRead continuation point for {NodeId} failed", nodeId); + } + + return all; + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/DefaultSubscriptionAdapterTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/DefaultSubscriptionAdapterTests.cs new file mode 100644 index 00000000..07980b36 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/DefaultSubscriptionAdapterTests.cs @@ -0,0 +1,57 @@ +using Opc.Ua.Client; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Adapters; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Adapters; + +/// +/// A deleted subscription must also leave the SDK session's Subscriptions list, or every alarm re-target in the UI +/// leaves a dead behind until the session is disposed. +/// +public class DefaultSubscriptionAdapterTests +{ + /// DeleteAsync detaches the wrapped subscription exactly once, even when Dispose follows. + [Fact] + public async Task DeleteAsync_detaches_the_subscription_from_its_session_once() + { + var subscription = new Subscription(); + var detached = new List(); + var adapter = new DefaultSubscriptionAdapter(subscription, (s, _) => + { + lock (detached) detached.Add(s); + return Task.CompletedTask; + }); + + await adapter.DeleteAsync(TestContext.Current.CancellationToken); + adapter.Dispose(); + + detached.ShouldHaveSingleItem().ShouldBeSameAs(subscription); + } + + /// Dispose alone (no prior DeleteAsync) detaches too. + [Fact] + public void Dispose_detaches_the_subscription_from_its_session() + { + var detached = 0; + var adapter = new DefaultSubscriptionAdapter(new Subscription(), (_, _) => + { + Interlocked.Increment(ref detached); + return Task.CompletedTask; + }); + + adapter.Dispose(); + + detached.ShouldBe(1); + } + + /// A failing detach is logged, not thrown: teardown stays best effort. + [Fact] + public async Task A_failing_detach_does_not_throw_from_DeleteAsync() + { + var adapter = new DefaultSubscriptionAdapter(new Subscription(), + (_, _) => throw new InvalidOperationException("boom")); + + await Should.NotThrowAsync(() => adapter.DeleteAsync(TestContext.Current.CancellationToken)); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs new file mode 100644 index 00000000..0f9240b0 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs @@ -0,0 +1,144 @@ +using Opc.Ua; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Helpers; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Helpers; + +/// +/// Pins the client-side HistoryRead paging loop shared by the raw and modified reads: continuation pages are read +/// (never released) while the caller wants more, and a read stopped by the caller's own cap releases the server cursor. +/// +public class HistoryReadPagingTests +{ + private static readonly NodeId Node = new("t", 2); + + /// One recorded call to the page reader. + private sealed record Call(byte[]? ContinuationPoint, bool Release); + + /// A scripted server: each read returns the next page; a release returns an empty Good result. + private sealed class ScriptedServer + { + private readonly Queue<(int Count, byte[]? Cp)> _pages; + public List Calls { get; } = []; + + public ScriptedServer(params (int Count, byte[]? Cp)[] pages) => _pages = new Queue<(int, byte[]?)>(pages); + + public Task ReadPageAsync(byte[]? cp, bool release, CancellationToken ct) + { + Calls.Add(new Call(cp, release)); + if (release) return Task.FromResult(new HistoryReadResult { StatusCode = StatusCodes.Good }); + var (count, next) = _pages.Dequeue(); + var data = new HistoryData(); + for (var i = 0; i < count; i++) data.DataValues.Add(new DataValue(new Variant(i))); + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, + ContinuationPoint = next, + HistoryData = new ExtensionObject(data) + }); + } + } + + private static IEnumerable Values(HistoryReadResult r) => + r.HistoryData?.Body is HistoryData d ? d.DataValues : []; + + /// + /// A server page limit below the caller's cap hands back a continuation point: the follow-up must READ the next + /// page. Releasing it instead (the old raw path passed release = cp != null) silently truncated every raw read + /// to its first server page. + /// + [Fact] + public async Task Continuation_pages_are_read_not_released_until_the_server_finishes() + { + var server = new ScriptedServer((3, [1]), (3, [2]), (2, null)); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 100, Node, + CancellationToken.None); + + values.Count.ShouldBe(8); + server.Calls.Select(c => c.Release).ShouldAllBe(r => r == false); + server.Calls.Select(c => c.ContinuationPoint?.FirstOrDefault()).ShouldBe(new byte?[] { null, 1, 2 }); + } + + /// A read stopped by the caller's cap while the server still holds a cursor releases that cursor. + [Fact] + public async Task A_read_stopped_by_the_cap_releases_the_outstanding_continuation_point() + { + var server = new ScriptedServer((5, [7])); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 5, Node, + CancellationToken.None); + + values.Count.ShouldBe(5); + server.Calls.Count.ShouldBe(2); + server.Calls[1].Release.ShouldBeTrue(); + server.Calls[1].ContinuationPoint.ShouldBe(new byte[] { 7 }); + } + + /// + /// A read the server finished (no continuation point) has nothing to release — and the positive control above + /// proves a release would have been recorded had one been owed. + /// + [Fact] + public async Task A_read_the_server_finished_issues_no_release() + { + var server = new ScriptedServer((5, null)); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 5, Node, + CancellationToken.None); + + values.Count.ShouldBe(5); + server.Calls.ShouldHaveSingleItem().Release.ShouldBeFalse(); + } + + /// An uncapped read (maxValues <= 0) follows every continuation point to the end. + [Fact] + public async Task An_uncapped_read_follows_every_page() + { + var server = new ScriptedServer((2, [1]), (2, [2]), (1, null)); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 0, Node, + CancellationToken.None); + + values.Count.ShouldBe(5); + server.Calls.ShouldAllBe(c => !c.Release); + } + + /// A failed release is best effort: the values already read are still returned. + [Fact] + public async Task A_failed_release_does_not_lose_the_values_read() + { + var calls = 0; + Task Read(byte[]? cp, bool release, CancellationToken ct) + { + calls++; + if (release) throw new ServiceResultException(StatusCodes.BadSessionClosed); + var data = new HistoryData(); + data.DataValues.Add(new DataValue(new Variant(1))); + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, ContinuationPoint = [9], HistoryData = new ExtensionObject(data) + }); + } + + var values = await HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 1, Node, CancellationToken.None); + + values.Count.ShouldBe(1); + calls.ShouldBe(2); + } + + /// A refused page still surfaces as an exception, not an empty read. + [Fact] + public async Task A_refused_page_throws() + { + Task Read(byte[]? cp, bool release, CancellationToken ct) => + Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.BadHistoryOperationUnsupported + }); + + await Should.ThrowAsync(() => + HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 10, Node, CancellationToken.None)); + } +} From a74a18e49a368d0339d0b6b4843efef78cae9e3a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 16:24:18 -0400 Subject: [PATCH 02/10] fix(client): offer exactly the served aggregates in the CLI and UI R08 2.3: AggregateType drops StandardDeviation (no backend serves it; always BadAggregateNotSupported) and gains Range, Delta, MinimumActualTime, MaximumActualTime, PercentGood and PercentBad. A parity test pins the enum to AggregateFamilies.Served and each member to its same-named Part 13 ObjectId. The CLI parses every enum name (plus aliases) and the UI picker lists them all. Existing members keep their numeric values; 6 is retired. Refs #747 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Commands/HistoryReadCommand.cs | 41 +++++---- .../Helpers/AggregateTypeMapper.cs | 7 +- .../Models/AggregateType.cs | 53 +++++++---- .../ViewModels/HistoryViewModel.cs | 18 +--- .../HistoryReadAggregateParsingTests.cs | 87 +++++++++++++++++++ .../Helpers/AggregateTypeMapperTests.cs | 39 ++++++--- .../Models/ModelConstructionTests.cs | 4 +- ....MOM.WW.OtOpcUa.Client.Shared.Tests.csproj | 2 + .../HistoryViewModelTests.cs | 11 ++- 9 files changed, 200 insertions(+), 62 deletions(-) create mode 100644 tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadAggregateParsingTests.cs diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs index b19e20fb..64d5d665 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs @@ -47,7 +47,7 @@ public class HistoryReadCommand : CommandBase /// /// Gets the optional aggregate name used when the operator wants processed history instead of raw samples. /// - [CommandOption("aggregate", Description = "Aggregate function: Average, Minimum, Maximum, Count, Start, End, StandardDeviation, TimeAverage, TimeAverage2, Total, Total2, Interpolative")] + [CommandOption("aggregate", Description = "Aggregate function: Average, Minimum, Maximum, Count, Start, End, TimeAverage, TimeAverage2, Total, Total2, Interpolative, Range, Delta, MinimumActualTime, MaximumActualTime, PercentGood, PercentBad")] public string? Aggregate { get; init; } /// @@ -199,24 +199,33 @@ public class HistoryReadCommand : CommandBase } } + private static readonly string SupportedAggregates = string.Join(", ", Enum.GetNames()); + private static AggregateType ParseAggregateType(string name) { - return name.Trim().ToLowerInvariant() switch + var trimmed = name.Trim(); + var alias = trimmed.ToLowerInvariant() switch { - "average" or "avg" => AggregateType.Average, - "minimum" or "min" => AggregateType.Minimum, - "maximum" or "max" => AggregateType.Maximum, - "count" => AggregateType.Count, - "start" or "first" => AggregateType.Start, - "end" or "last" => AggregateType.End, - "standarddeviation" or "stddev" or "stdev" => AggregateType.StandardDeviation, - "timeaverage" or "timeavg" => AggregateType.TimeAverage, - "timeaverage2" or "timeavg2" => AggregateType.TimeAverage2, - "total" => AggregateType.Total, - "total2" => AggregateType.Total2, - "interpolative" or "interp" => AggregateType.Interpolative, - _ => throw new ArgumentException( - $"Unknown aggregate: '{name}'. Supported: Average, Minimum, Maximum, Count, Start, End, StandardDeviation, TimeAverage, TimeAverage2, Total, Total2, Interpolative") + "avg" => AggregateType.Average, + "min" => AggregateType.Minimum, + "max" => AggregateType.Maximum, + "first" => AggregateType.Start, + "last" => AggregateType.End, + "timeavg" => AggregateType.TimeAverage, + "timeavg2" => AggregateType.TimeAverage2, + "interp" => AggregateType.Interpolative, + "mintime" => AggregateType.MinimumActualTime, + "maxtime" => AggregateType.MaximumActualTime, + _ => (AggregateType?)null }; + if (alias is { } a) return a; + + // Every enum name (the served set; a parity test pins it), case-insensitive. Numeric strings are refused: + // Enum.TryParse would accept "6" or "99" as an undefined value. + if (!int.TryParse(trimmed, out _) && + Enum.TryParse(trimmed, ignoreCase: true, out var parsed) && Enum.IsDefined(parsed)) + return parsed; + + throw new ArgumentException($"Unknown aggregate: '{name}'. Supported: {SupportedAggregates}"); } } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs index 44462199..23b70ede 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs @@ -23,12 +23,17 @@ public static class AggregateTypeMapper AggregateType.Count => ObjectIds.AggregateFunction_Count, AggregateType.Start => ObjectIds.AggregateFunction_Start, AggregateType.End => ObjectIds.AggregateFunction_End, - AggregateType.StandardDeviation => ObjectIds.AggregateFunction_StandardDeviationPopulation, AggregateType.TimeAverage => ObjectIds.AggregateFunction_TimeAverage, AggregateType.TimeAverage2 => ObjectIds.AggregateFunction_TimeAverage2, AggregateType.Total => ObjectIds.AggregateFunction_Total, AggregateType.Total2 => ObjectIds.AggregateFunction_Total2, AggregateType.Interpolative => ObjectIds.AggregateFunction_Interpolative, + AggregateType.Range => ObjectIds.AggregateFunction_Range, + AggregateType.Delta => ObjectIds.AggregateFunction_Delta, + AggregateType.MinimumActualTime => ObjectIds.AggregateFunction_MinimumActualTime, + AggregateType.MaximumActualTime => ObjectIds.AggregateFunction_MaximumActualTime, + AggregateType.PercentGood => ObjectIds.AggregateFunction_PercentGood, + AggregateType.PercentBad => ObjectIds.AggregateFunction_PercentBad, _ => throw new ArgumentOutOfRangeException(nameof(aggregate), aggregate, "Unknown AggregateType value.") }; } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs index b7f48ab7..17dddc51 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs @@ -1,43 +1,64 @@ namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Models; /// -/// Aggregate functions for processed history reads. +/// Aggregate functions for processed history reads — exactly the seventeen the server's SQL historian backends +/// serve (AggregateFamilies.Served; a parity test pins it). The Gateway backend serves a subset and refuses +/// the rest with BadAggregateNotSupported. /// +/// +/// Values are explicit so the pre-existing members keep their numbers. 6 (the former StandardDeviation, +/// which no backend serves) is retired; do not reuse it. +/// public enum AggregateType { /// Average of values in the interval. - Average, + Average = 0, /// Minimum value in the interval. - Minimum, + Minimum = 1, /// Maximum value in the interval. - Maximum, + Maximum = 2, /// Count of values in the interval. - Count, + Count = 3, /// First value in the interval. - Start, + Start = 4, /// Last value in the interval. - End, - - /// Population standard deviation of values in the interval. - StandardDeviation, + End = 5, /// Time-weighted average, interpolated bounds (Part 13 TimeAverage). - TimeAverage, + TimeAverage = 7, /// Time-weighted average, stepped bounds (Part 13 TimeAverage2). - TimeAverage2, + TimeAverage2 = 8, /// Time-weighted integral over the interval (Part 13 Total). - Total, + Total = 9, /// Time-weighted integral, stepped bounds (Part 13 Total2). - Total2, + Total2 = 10, /// Interpolated value at the interval start (Part 13 Interpolative). - Interpolative -} \ No newline at end of file + Interpolative = 11, + + /// Maximum minus minimum in the interval (Part 13 Range). + Range = 12, + + /// Last minus first value in the interval (Part 13 Delta). + Delta = 13, + + /// Minimum value with its own timestamp (Part 13 MinimumActualTime). + MinimumActualTime = 14, + + /// Maximum value with its own timestamp (Part 13 MaximumActualTime). + MaximumActualTime = 15, + + /// Percentage of the interval with Good data (Part 13 PercentGood). + PercentGood = 16, + + /// Percentage of the interval with Bad data (Part 13 PercentBad). + PercentBad = 17 +} diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs index aab55811..b883be03 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs @@ -43,23 +43,9 @@ public partial class HistoryViewModel : ObservableObject _dispatcher = dispatcher; } - /// Available aggregate types (null means "Raw"). + /// Available aggregate types (null means "Raw"): every client aggregate, which is the served set. public IReadOnlyList AggregateTypes { get; } = - [ - null, - AggregateType.Average, - AggregateType.Minimum, - AggregateType.Maximum, - AggregateType.Count, - AggregateType.Start, - AggregateType.End, - AggregateType.StandardDeviation, - AggregateType.TimeAverage, - AggregateType.TimeAverage2, - AggregateType.Total, - AggregateType.Total2, - AggregateType.Interpolative - ]; + [null, .. Enum.GetValues().Select(a => (AggregateType?)a)]; /// Gets a value indicating whether an aggregate read is selected. public bool IsAggregateRead => SelectedAggregateType != null; diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadAggregateParsingTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadAggregateParsingTests.cs new file mode 100644 index 00000000..8d150ae2 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadAggregateParsingTests.cs @@ -0,0 +1,87 @@ +using CliFx.Exceptions; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; + +namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests; + +/// +/// historyread --aggregate accepts every aggregate the client offers (which is exactly the served set), so the +/// gate recipes in docs/Historian.md that need Range / Delta / PercentGood / ... can run from this CLI. +/// +public class HistoryReadAggregateParsingTests +{ + /// Every client aggregate, by its name. + /// Each value. + public static TheoryData AllAggregates() => new(Enum.GetValues()); + + /// Every aggregate name (in any case) reaches the service as that aggregate. + /// The aggregate. + [Theory] + [MemberData(nameof(AllAggregates))] + public async Task Every_aggregate_name_parses(AggregateType aggregate) + { + var fakeService = new FakeOpcUaClientService(); + var command = new HistoryReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=HistNode", + Aggregate = aggregate.ToString().ToUpperInvariant() + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + fakeService.HistoryReadAggregateCalls.ShouldHaveSingleItem().Aggregate.ShouldBe(aggregate); + } + + /// The short aliases still parse. + /// The alias. + /// The aggregate it names. + [Theory] + [InlineData("avg", AggregateType.Average)] + [InlineData("min", AggregateType.Minimum)] + [InlineData("last", AggregateType.End)] + [InlineData("interp", AggregateType.Interpolative)] + [InlineData("mintime", AggregateType.MinimumActualTime)] + [InlineData("maxtime", AggregateType.MaximumActualTime)] + public async Task Aliases_parse(string alias, AggregateType expected) + { + var fakeService = new FakeOpcUaClientService(); + var command = new HistoryReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=HistNode", + Aggregate = alias + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + fakeService.HistoryReadAggregateCalls.ShouldHaveSingleItem().Aggregate.ShouldBe(expected); + } + + /// + /// StandardDeviation is no longer offered (no backend serves it): it is refused before connecting, naming the + /// supported set. + /// + [Fact] + public async Task StandardDeviation_is_refused_before_connecting() + { + var fakeService = new FakeOpcUaClientService(); + var command = new HistoryReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=HistNode", + Aggregate = "stddev" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + + ex.Message.ShouldContain("PercentGood"); + fakeService.HistoryReadAggregateCalls.ShouldBeEmpty(); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs index 663ab473..267baf02 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs @@ -3,6 +3,7 @@ using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Client.Shared.Helpers; using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; +using ZB.MOM.WW.OtOpcUa.Driver.Historian.Sql.Common; namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Helpers; @@ -17,7 +18,6 @@ public class AggregateTypeMapperTests [InlineData(AggregateType.Count)] [InlineData(AggregateType.Start)] [InlineData(AggregateType.End)] - [InlineData(AggregateType.StandardDeviation)] [InlineData(AggregateType.TimeAverage)] [InlineData(AggregateType.TimeAverage2)] [InlineData(AggregateType.Total)] @@ -72,14 +72,6 @@ public class AggregateTypeMapperTests AggregateTypeMapper.ToNodeId(AggregateType.End).ShouldBe(ObjectIds.AggregateFunction_End); } - /// Verifies that StandardDeviation aggregate type maps to the correct OPC UA node ID. - [Fact] - public void ToNodeId_StandardDeviation_MapsCorrectly() - { - AggregateTypeMapper.ToNodeId(AggregateType.StandardDeviation) - .ShouldBe(ObjectIds.AggregateFunction_StandardDeviationPopulation); - } - /// Verifies that invalid aggregate type value throws ArgumentOutOfRangeException. [Fact] public void ToNodeId_InvalidValue_Throws() @@ -98,4 +90,31 @@ public class AggregateTypeMapperTests AggregateTypeMapper.ToNodeId(AggregateType.Total2).ShouldBe(ObjectIds.AggregateFunction_Total2); AggregateTypeMapper.ToNodeId(AggregateType.Interpolative).ShouldBe(ObjectIds.AggregateFunction_Interpolative); } -} \ No newline at end of file + + /// + /// The client offers exactly the aggregates the server's SQL backends serve (): + /// nothing the server always refuses (the old StandardDeviation), nothing served left unreachable + /// (Range, Delta, Min/MaxActualTime, PercentGood/Bad). + /// + [Fact] + public void Client_aggregates_are_exactly_the_served_set() + { + Enum.GetNames().Order() + .ShouldBe(AggregateFamilies.Served.Select(a => a.ToString()).Order()); + } + + /// Each aggregate maps to the Part 13 ObjectId of the same name. + /// The aggregate. + [Theory] + [MemberData(nameof(AllAggregates))] + public void ToNodeId_maps_to_the_same_named_AggregateFunction(AggregateType aggregate) + { + var field = typeof(ObjectIds).GetField("AggregateFunction_" + aggregate); + field.ShouldNotBeNull(); + AggregateTypeMapper.ToNodeId(aggregate).ShouldBe((NodeId)field.GetValue(null)!); + } + + /// Every client aggregate. + /// Each value. + public static TheoryData AllAggregates() => new(Enum.GetValues()); +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs index 3d3a32a4..a9f01b51 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs @@ -132,10 +132,10 @@ public class ModelConstructionTests ((int)ConnectionState.Reconnecting).ShouldBe(3); } - /// Verifies that AggregateType enum has the expected number of values. + /// AggregateType holds the seventeen served aggregates (the parity test pins which). [Fact] public void AggregateType_Enum_HasExpectedValues() { - Enum.GetValues().Length.ShouldBe(12); + Enum.GetValues().Length.ShouldBe(17); } } \ No newline at end of file diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj index 903b39f9..e588e7af 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj @@ -21,6 +21,8 @@ + + diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs index e056f78b..a333e699 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs @@ -141,7 +141,16 @@ public class HistoryViewModelTests public void AggregateTypes_ContainsNullForRaw() { _vm.AggregateTypes.ShouldContain((AggregateType?)null); - _vm.AggregateTypes.Count.ShouldBe(13); // null + 12 enum values + _vm.AggregateTypes.Count.ShouldBe(Enum.GetValues().Length + 1); // null + every aggregate + } + + /// The picker offers every client aggregate (the served set), including Range/Delta/PercentGood/... + [Fact] + public void AggregateTypes_offers_every_aggregate() + { + _vm.AggregateTypes.Where(a => a != null).Select(a => a!.Value) + .ShouldBe(Enum.GetValues(), ignoreOrder: true); + _vm.AggregateTypes.ShouldContain(AggregateType.PercentGood); } /// Verifies that the Clear method resets the view model state. From 540b1632c5ddf14a9669a16b8af9fabb9a8a54ab Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 16:27:24 -0400 Subject: [PATCH 03/10] perf(client): lazy child detection in BrowseAsync, NodeId-indexed UI notifications R08 3.6: BrowseAsync no longer issues one HasChildren Browse per Object child; an Object reports HasChildren (may have children) and is confirmed when expanded or recursed. ISessionAdapter.HasChildrenAsync is removed. The UI subscriptions panel routes each notification through a NodeId index kept in step with ActiveSubscriptions (add/remove/replace/reset) instead of scanning every row. Refs #747 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Adapters/DefaultSessionAdapter.cs | 16 ----- .../Adapters/ISessionAdapter.cs | 8 --- .../Models/BrowseResult.cs | 4 +- .../OpcUaClientService.cs | 11 ++-- .../ViewModels/SubscriptionsViewModel.cs | 52 ++++++++++++--- .../Fakes/FakeSessionAdapter.cs | 11 ---- .../OpcUaClientServiceTests.cs | 26 ++++++-- .../SubscriptionsViewModelTests.cs | 64 +++++++++++++++++++ 8 files changed, 135 insertions(+), 57 deletions(-) diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs index c414f1c1..b07efa97 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs @@ -115,22 +115,6 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter return (nextCp, nextRefs ?? []); } - /// - public async Task HasChildrenAsync(NodeId nodeId, CancellationToken ct) - { - var (_, _, references) = await _session.BrowseAsync( - null, - null, - nodeId, - 1u, - BrowseDirection.Forward, - ReferenceTypeIds.HierarchicalReferences, - true, - 0u); - - return references != null && references.Count > 0; - } - /// public async Task HistoryUpdateAsync(HistoryUpdateDetails details, CancellationToken ct) { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs index f3d81e64..8734c50b 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs @@ -90,14 +90,6 @@ internal interface ISessionAdapter : IDisposable Task<(byte[]? ContinuationPoint, ReferenceDescriptionCollection References)> BrowseNextAsync( byte[] continuationPoint, CancellationToken ct = default); - /// - /// Checks whether a node has any forward hierarchical child references. - /// - /// The node to inspect for child objects or variables. - /// The cancellation token that aborts the child lookup. - /// true when the node has at least one forward hierarchical child reference; otherwise false. - Task HasChildrenAsync(NodeId nodeId, CancellationToken ct = default); - /// /// Reads raw historical data. /// diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs index 4e3658f8..d0b06838 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs @@ -34,7 +34,9 @@ public sealed class BrowseResult public string NodeClass { get; } /// - /// Whether the node has child references. + /// Whether the node MAY have child references: for every Object, confirmed lazily when the + /// caller browses it (an expanded empty folder simply yields no children). Variables and Methods report + /// . /// public bool HasChildren { get; } } \ No newline at end of file diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs index c1d87129..fd3f1971 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs @@ -224,18 +224,15 @@ public sealed class OpcUaClientService : IOpcUaClientService while (references.Count > 0) { + // Child detection is lazy: an Object MAY have children and is confirmed when the caller expands or + // recurses into it. Probing each Object child with its own Browse made one level of N folders cost N + 1 + // round trips, and a recursive subscribe over a 50k-node tree ~50k extra Browse calls. foreach (var reference in references) - { - var childNodeId = ExpandedNodeId.ToNodeId(reference.NodeId, _session.NamespaceUris); - var hasChildren = reference.NodeClass == NodeClass.Object && - await _session.HasChildrenAsync(childNodeId, ct); - results.Add(new BrowseResult( reference.NodeId.ToString(), reference.DisplayName?.Text ?? string.Empty, reference.NodeClass.ToString(), - hasChildren)); - } + reference.NodeClass == NodeClass.Object)); if (continuationPoint is not { Length: > 0 } pending) break; (continuationPoint, references) = await _session.BrowseNextAsync(pending, ct); diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs index 381719a4..18d2bee4 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs @@ -1,4 +1,5 @@ using System.Collections.ObjectModel; +using System.Collections.Specialized; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using Opc.Ua; @@ -19,6 +20,11 @@ public partial class SubscriptionsViewModel : ObservableObject private readonly IUiDispatcher _dispatcher; private readonly IOpcUaClientService _service; + // NodeId -> its rows, kept in step with ActiveSubscriptions by OnActiveSubscriptionsChanged, so a notification is + // one lookup instead of a scan of every row on the UI thread (2 000 items at 1 Hz was 4M string compares/s). + // Touched only on the UI thread, like the collection itself. + private readonly Dictionary> _rowsByNodeId = new(StringComparer.Ordinal); + /// /// Last user-visible status message — set when a subscribe/unsubscribe operation fails so the /// shell can surface the diagnostic instead of silently dropping the error. Cleared on success. @@ -49,9 +55,13 @@ public partial class SubscriptionsViewModel : ObservableObject { _service = service; _dispatcher = dispatcher; + ActiveSubscriptions.CollectionChanged += OnActiveSubscriptionsChanged; _service.DataChanged += OnDataChanged; } + /// The number of distinct NodeIds in the notification index (test seam). + internal int IndexedNodeCount => _rowsByNodeId.Count; + /// Currently active subscriptions. public ObservableCollection ActiveSubscriptions { get; } = []; @@ -62,16 +72,42 @@ public partial class SubscriptionsViewModel : ObservableObject { _dispatcher.Post(() => { - foreach (var item in ActiveSubscriptions) - if (item.NodeId == e.NodeId) - { - item.Value = Helpers.ValueFormatter.Format(e.Value.Value); - item.Status = Helpers.StatusCodeFormatter.Format(e.Value.StatusCode); - item.Timestamp = e.Value.SourceTimestamp.ToString("O"); - } + if (!_rowsByNodeId.TryGetValue(e.NodeId, out var rows)) return; + foreach (var item in rows) + { + item.Value = Helpers.ValueFormatter.Format(e.Value.Value); + item.Status = Helpers.StatusCodeFormatter.Format(e.Value.StatusCode); + item.Timestamp = e.Value.SourceTimestamp.ToString("O"); + } }); } + private void OnActiveSubscriptionsChanged(object? sender, NotifyCollectionChangedEventArgs e) + { + if (e.Action == NotifyCollectionChangedAction.Reset) + { + _rowsByNodeId.Clear(); + foreach (var item in ActiveSubscriptions) Index(item); + return; + } + + if (e.OldItems != null) + foreach (SubscriptionItemViewModel item in e.OldItems) + if (_rowsByNodeId.TryGetValue(item.NodeId, out var rows) && rows.Remove(item) && rows.Count == 0) + _rowsByNodeId.Remove(item.NodeId); + + if (e.NewItems != null) + foreach (SubscriptionItemViewModel item in e.NewItems) + Index(item); + } + + private void Index(SubscriptionItemViewModel item) + { + if (!_rowsByNodeId.TryGetValue(item.NodeId, out var rows)) + _rowsByNodeId[item.NodeId] = rows = []; + rows.Add(item); + } + private bool CanAddSubscription() { return IsConnected && !string.IsNullOrWhiteSpace(NewNodeIdText); @@ -149,7 +185,7 @@ public partial class SubscriptionsViewModel : ObservableObject if (!IsConnected || string.IsNullOrWhiteSpace(nodeIdStr)) return; // Skip if already subscribed - if (ActiveSubscriptions.Any(s => s.NodeId == nodeIdStr)) return; + if (_rowsByNodeId.ContainsKey(nodeIdStr)) return; try { diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs index a73a9f65..14f1df52 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs @@ -28,8 +28,6 @@ internal sealed class FakeSessionAdapter : ISessionAdapter public int BrowseCount { get; private set; } /// Gets the number of times BrowseNextAsync has been called. public int BrowseNextCount { get; private set; } - /// Gets the number of times HasChildrenAsync has been called. - public int HasChildrenCount { get; private set; } /// Gets the number of times HistoryReadRawAsync has been called. public int HistoryReadRawCount { get; private set; } /// Gets the number of times HistoryReadAggregateAsync has been called. @@ -57,8 +55,6 @@ internal sealed class FakeSessionAdapter : ISessionAdapter public ReferenceDescriptionCollection BrowseNextResponse { get; set; } = []; /// Gets or sets the continuation point for browse-next operations. public byte[]? BrowseNextContinuationPoint { get; set; } - /// Gets or sets the result returned by has-children operations. - public bool HasChildrenResponse { get; set; } = false; /// Gets or sets the historical values returned by raw history-read operations. public List HistoryReadRawResponse { get; set; } = []; @@ -150,13 +146,6 @@ internal sealed class FakeSessionAdapter : ISessionAdapter return Task.FromResult((BrowseNextContinuationPoint, BrowseNextResponse)); } - /// - public Task HasChildrenAsync(NodeId nodeId, CancellationToken ct) - { - HasChildrenCount++; - return Task.FromResult(HasChildrenResponse); - } - /// public Task> HistoryReadRawAsync( NodeId nodeId, DateTime startTime, DateTime endTime, int maxValues, CancellationToken ct) diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs index a604fb08..f1035075 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs @@ -384,10 +384,12 @@ public class OpcUaClientServiceTests : IDisposable } /// - /// Verifies that object nodes trigger child-detection checks so the client can mark expandable branches. + /// R08 3.6: browsing a level issues ONE Browse (plus BrowseNext pages) however many Object children it holds. Child + /// detection is lazy: an Object may have children and is confirmed when it is expanded/recursed, instead of one + /// extra Browse per Object child (~50 000 calls for a recursive subscribe over a 50k-node raw tree). /// [Fact] - public async Task BrowseAsync_ObjectNode_ChecksHasChildren() + public async Task BrowseAsync_ObjectChildren_MayHaveChildren_WithoutAPerChildBrowse() { var session = new FakeSessionAdapter { @@ -398,17 +400,29 @@ public class OpcUaClientServiceTests : IDisposable NodeId = new ExpandedNodeId("ns=2;s=Folder1"), DisplayName = new LocalizedText("Folder1"), NodeClass = NodeClass.Object + }, + new ReferenceDescription + { + NodeId = new ExpandedNodeId("ns=2;s=Folder2"), + DisplayName = new LocalizedText("Folder2"), + NodeClass = NodeClass.Object + }, + new ReferenceDescription + { + NodeId = new ExpandedNodeId("ns=2;s=Tag"), + DisplayName = new LocalizedText("Tag"), + NodeClass = NodeClass.Variable } - ], - HasChildrenResponse = true + ] }; _sessionFactory.EnqueueSession(session); await _service.ConnectAsync(ValidSettings()); var results = await _service.BrowseAsync(); - results[0].HasChildren.ShouldBeTrue(); - session.HasChildrenCount.ShouldBe(1); + results.Select(r => r.HasChildren).ShouldBe([true, true, false]); + session.BrowseCount.ShouldBe(1); + session.BrowseNextCount.ShouldBe(0); } /// diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs index 8c4fd3f5..44ca937a 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs @@ -111,6 +111,70 @@ public class SubscriptionsViewModelTests _vm.ActiveSubscriptions[0].Value.ShouldBeNull(); } + /// + /// R08 3.6: notifications are routed through a NodeId index, not a scan of every row. The index follows every + /// mutation of (add, remove, replace, clear). + /// + [Fact] + public void NodeId_index_follows_every_collection_mutation() + { + var a = new SubscriptionItemViewModel("ns=2;s=A", 1000); + var b = new SubscriptionItemViewModel("ns=2;s=B", 1000); + var c = new SubscriptionItemViewModel("ns=2;s=C", 1000); + _vm.ActiveSubscriptions.Add(a); + _vm.ActiveSubscriptions.Add(b); + _vm.ActiveSubscriptions.Add(c); + _vm.IndexedNodeCount.ShouldBe(3); + + _vm.ActiveSubscriptions.Remove(b); + _vm.IndexedNodeCount.ShouldBe(2); + + _vm.ActiveSubscriptions[0] = new SubscriptionItemViewModel("ns=2;s=D", 1000); // replaces A + Raise("ns=2;s=A", 1); + Raise("ns=2;s=D", 2); + a.Value.ShouldBeNull(); + _vm.ActiveSubscriptions[0].Value.ShouldBe("2"); + + _vm.ActiveSubscriptions.Clear(); + _vm.IndexedNodeCount.ShouldBe(0); + } + + /// A removed row stops receiving values while its sibling (the positive control) still does. + [Fact] + public void DataChanged_after_remove_updates_only_the_remaining_rows() + { + var a = new SubscriptionItemViewModel("ns=2;s=A", 1000); + var b = new SubscriptionItemViewModel("ns=2;s=B", 1000); + _vm.ActiveSubscriptions.Add(a); + _vm.ActiveSubscriptions.Add(b); + _vm.ActiveSubscriptions.Remove(a); + + Raise("ns=2;s=A", 7); + Raise("ns=2;s=B", 42); + + a.Value.ShouldBeNull(); + b.Value.ShouldBe("42"); + } + + /// Two rows for the same node (no dedup on the manual-add path) both update, as before. + [Fact] + public void DataChanged_updates_every_row_for_the_node() + { + var first = new SubscriptionItemViewModel("ns=2;s=A", 1000); + var second = new SubscriptionItemViewModel("ns=2;s=A", 500); + _vm.ActiveSubscriptions.Add(first); + _vm.ActiveSubscriptions.Add(second); + + Raise("ns=2;s=A", 5); + + first.Value.ShouldBe("5"); + second.Value.ShouldBe("5"); + } + + private void Raise(string nodeId, int value) => + _service.RaiseDataChanged(new DataChangedEventArgs(nodeId, + new DataValue(new Variant(value), StatusCodes.Good, DateTime.UtcNow))); + /// Verifies that Clear removes all subscriptions. [Fact] public void Clear_RemovesAllSubscriptions() From 2d8bb037f58686f557b53e31e37ffa2d468a1afa Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 16:30:35 -0400 Subject: [PATCH 04/10] fix(client): failover runs under the subscription gate; ConnectAsync refuses when connected R08 4.5: every subscription operation that awaits I/O and the failover's teardown + reconnect + replay share one async gate, and every read/write of the subscription fields (failover's null-outs included) is under _subscriptionLock. A subscribe in flight when a failover starts is recorded first and then replayed onto the new session instead of landing on the dying session's subscription. The dead session is disposed before the gate is taken so a blocked call fails fast. A DisconnectAsync during a failover is no longer undone by it. ConnectAsync on a connected or reconnecting client throws instead of overwriting (and leaking) the live session. Refs #747 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../OpcUaClientService.cs | 277 ++++++++++++------ .../Fakes/FakeSessionAdapter.cs | 20 +- .../OpcUaClientServiceTests.cs | 91 ++++++ 3 files changed, 292 insertions(+), 96 deletions(-) diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs index fd3f1971..9d78961e 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs @@ -15,16 +15,17 @@ public sealed class OpcUaClientService : IOpcUaClientService { private static readonly ILogger Logger = Log.ForContext(); - // Guards all access to the subscription-bookkeeping state below - // (_activeDataSubscriptions and _activeAlarmSubscription). The dictionary - // and tuple are mutated from the caller thread, the keep-alive failover - // path, and DisconnectAsync, so every read/write must be inside this lock. + // Guards all access to the subscription state below (_activeDataSubscriptions, _activeAlarmSubscription, + // _dataSubscription, _alarmSubscription). They are touched from the caller thread, the keep-alive failover path + // and DisconnectAsync, so every read/write is inside this lock — including failover's null-outs. private readonly object _subscriptionLock = new(); - // Serialises SubscribeAlarmsAsync / UnsubscribeAlarmsAsync so concurrent callers - // cannot both pass the _alarmSubscription == null check and create duplicate event - // subscriptions. Capacity 1 = at most one waiter; async-friendly (no thread-block). - private readonly SemaphoreSlim _alarmSubscribeSemaphore = new(1, 1); + // Serialises every subscription operation that awaits I/O (Subscribe/Unsubscribe, SubscribeAlarms/ + // UnsubscribeAlarms) AND the failover's teardown + reconnect + replay. A subscribe is therefore either + // wholly before a failover (recorded, then replayed onto the new session) or wholly after it (on the new + // session) — never half on the dying session. Async-friendly (no thread block). Deliberately never disposed: + // async-only waits hold no kernel handle, and a failover still running at Dispose releases it afterwards. + private readonly SemaphoreSlim _subscriptionGate = new(1, 1); // Track active data subscriptions for replay after failover private readonly Dictionary _activeDataSubscriptions = new(); @@ -34,6 +35,9 @@ public sealed class OpcUaClientService : IOpcUaClientService // run at a time. 0 = idle, 1 = failover in progress (Interlocked-managed). private int _failoverInProgress; + /// The most recent keep-alive-triggered failover task (test seam: lets a test await it). + internal Task? LastFailover { get; private set; } + private readonly IApplicationConfigurationFactory _configFactory; private readonly IEndpointDiscovery _endpointDiscovery; @@ -99,6 +103,12 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfDisposed(); settings.Validate(); + // Overwriting a live (or reconnecting) session would leak it and orphan its subscriptions and keep-alive + // handler. The caller must disconnect first. + if (_state != ConnectionState.Disconnected || _session != null) + throw new InvalidOperationException( + $"Already {_state.ToString().ToLowerInvariant()}; call DisconnectAsync before connecting again."); + _settings = settings; _allEndpointUrls = FailoverUrlParser.Parse(settings.EndpointUrl, settings.FailoverUrls); _currentEndpointIndex = 0; @@ -112,7 +122,7 @@ public sealed class OpcUaClientService : IOpcUaClientService session.RegisterKeepAliveHandler(isGood => { - if (!isGood) _ = HandleKeepAliveFailureAsync(); + if (!isGood) LastFailover = HandleKeepAliveFailureAsync(); }); CurrentConnectionInfo = BuildConnectionInfo(session); @@ -248,20 +258,39 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfNotConnected(); var nodeIdStr = nodeId.ToString(); - lock (_subscriptionLock) + await _subscriptionGate.WaitAsync(ct); + try { - if (_activeDataSubscriptions.ContainsKey(nodeIdStr)) - return; // Already subscribed + // A failover may have run (and failed) while this call waited for the gate. + ThrowIfNotConnected(); + + ISubscriptionAdapter? subscription; + ISessionAdapter session; + lock (_subscriptionLock) + { + if (_activeDataSubscriptions.ContainsKey(nodeIdStr)) + return; // Already subscribed + subscription = _dataSubscription; + session = _session!; + } + + if (subscription == null) + { + subscription = await session.CreateSubscriptionAsync(intervalMs, ct); + lock (_subscriptionLock) _dataSubscription = subscription; + } + + var handle = await subscription.AddDataChangeMonitoredItemAsync( + nodeId, intervalMs, OnDataChangeNotification, ct); + + lock (_subscriptionLock) + { + _activeDataSubscriptions[nodeIdStr] = (nodeId, intervalMs, handle); + } } - - if (_dataSubscription == null) _dataSubscription = await _session!.CreateSubscriptionAsync(intervalMs, ct); - - var handle = await _dataSubscription.AddDataChangeMonitoredItemAsync( - nodeId, intervalMs, OnDataChangeNotification, ct); - - lock (_subscriptionLock) + finally { - _activeDataSubscriptions[nodeIdStr] = (nodeId, intervalMs, handle); + _subscriptionGate.Release(); } Logger.Debug("Subscribed to data changes on {NodeId}", nodeId); @@ -273,18 +302,28 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfDisposed(); var nodeIdStr = nodeId.ToString(); - (NodeId NodeId, int IntervalMs, uint Handle) sub; - lock (_subscriptionLock) + await _subscriptionGate.WaitAsync(ct); + try { - if (!_activeDataSubscriptions.TryGetValue(nodeIdStr, out sub)) - return; // Not subscribed, safe to ignore + (NodeId NodeId, int IntervalMs, uint Handle) sub; + ISubscriptionAdapter? subscription; + lock (_subscriptionLock) + { + if (!_activeDataSubscriptions.TryGetValue(nodeIdStr, out sub)) + return; // Not subscribed, safe to ignore + subscription = _dataSubscription; + } + + if (subscription != null) await subscription.RemoveMonitoredItemAsync(sub.Handle, ct); + + lock (_subscriptionLock) + { + _activeDataSubscriptions.Remove(nodeIdStr); + } } - - if (_dataSubscription != null) await _dataSubscription.RemoveMonitoredItemAsync(sub.Handle, ct); - - lock (_subscriptionLock) + finally { - _activeDataSubscriptions.Remove(nodeIdStr); + _subscriptionGate.Release(); } Logger.Debug("Unsubscribed from data changes on {NodeId}", nodeId); @@ -300,17 +339,25 @@ public sealed class OpcUaClientService : IOpcUaClientService // Serialise check-and-create under the semaphore: concurrent // callers cannot both pass the null-check and create duplicate alarm subscriptions. // The semaphore is async-friendly; the subscription I/O runs inside the critical section. - await _alarmSubscribeSemaphore.WaitAsync(ct); + await _subscriptionGate.WaitAsync(ct); try { - if (_alarmSubscription != null) - return; // Already subscribed to alarms + ThrowIfNotConnected(); + + ISessionAdapter session; + lock (_subscriptionLock) + { + if (_alarmSubscription != null) + return; // Already subscribed to alarms + session = _session!; + } var monitorNode = sourceNodeId ?? ObjectIds.Server; - _alarmSubscription = await _session!.CreateSubscriptionAsync(intervalMs, ct); + var subscription = await session.CreateSubscriptionAsync(intervalMs, ct); + lock (_subscriptionLock) _alarmSubscription = subscription; var filter = CreateAlarmEventFilter(); - await _alarmSubscription.AddEventMonitoredItemAsync( + await subscription.AddEventMonitoredItemAsync( monitorNode, intervalMs, filter, OnAlarmEventNotification, ct); lock (_subscriptionLock) @@ -322,7 +369,7 @@ public sealed class OpcUaClientService : IOpcUaClientService } finally { - _alarmSubscribeSemaphore.Release(); + _subscriptionGate.Release(); } } @@ -331,15 +378,27 @@ public sealed class OpcUaClientService : IOpcUaClientService { ThrowIfDisposed(); - if (_alarmSubscription == null) - return; - - await _alarmSubscription.DeleteAsync(ct); - _alarmSubscription = null; - - lock (_subscriptionLock) + await _subscriptionGate.WaitAsync(ct); + try { - _activeAlarmSubscription = null; + ISubscriptionAdapter? subscription; + lock (_subscriptionLock) + { + subscription = _alarmSubscription; + if (subscription == null) return; + } + + await subscription.DeleteAsync(ct); + + lock (_subscriptionLock) + { + _alarmSubscription = null; + _activeAlarmSubscription = null; + } + } + finally + { + _subscriptionGate.Release(); } Logger.Debug("Unsubscribed from alarm events"); @@ -351,10 +410,12 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfDisposed(); ThrowIfNotConnected(); - if (_alarmSubscription == null) + ISubscriptionAdapter? subscription; + lock (_subscriptionLock) subscription = _alarmSubscription; + if (subscription == null) throw new InvalidOperationException("No alarm subscription is active."); - await _alarmSubscription.ConditionRefreshAsync(ct); + await subscription.ConditionRefreshAsync(ct); Logger.Debug("Condition refresh requested"); } @@ -643,7 +704,6 @@ public sealed class OpcUaClientService : IOpcUaClientService _dataSubscription?.Dispose(); _alarmSubscription?.Dispose(); _session?.Dispose(); - _alarmSubscribeSemaphore.Dispose(); lock (_subscriptionLock) { @@ -710,69 +770,93 @@ public sealed class OpcUaClientService : IOpcUaClientService if (_state == ConnectionState.Reconnecting || _state == ConnectionState.Disconnected) return; - var oldEndpoint = _session?.EndpointUrl ?? string.Empty; + var oldSession = _session; + var oldEndpoint = oldSession?.EndpointUrl ?? string.Empty; TransitionState(ConnectionState.Reconnecting, oldEndpoint); Logger.Warning("Session lost on {EndpointUrl}. Attempting failover...", oldEndpoint); - // Close old session - if (_session != null) + // Dispose the dead session BEFORE taking the gate: an in-flight subscribe holding the gate is blocked on + // that session, and disposing it is what makes the call fail fast and release the gate. + if (oldSession != null) { try { - _session.Dispose(); + oldSession.Dispose(); } catch { } - - _session = null; } - _dataSubscription = null; - _alarmSubscription = null; - - if (_settings == null || _allEndpointUrls == null) + // The rest runs under the subscription gate (and its null-outs under _subscriptionLock): no subscribe can + // interleave with the teardown, the reconnect or the replay. + await _subscriptionGate.WaitAsync(); + try { - TransitionState(ConnectionState.Disconnected, oldEndpoint); - return; - } - - // Try each endpoint - for (var attempt = 0; attempt < _allEndpointUrls.Length; attempt++) - { - _currentEndpointIndex = (_currentEndpointIndex + 1) % _allEndpointUrls.Length; - var url = _allEndpointUrls[_currentEndpointIndex]; - - try + lock (_subscriptionLock) { - Logger.Information("Failover attempt to {EndpointUrl}", url); - var session = await ConnectToEndpointAsync(_settings, url, CancellationToken.None); - _session = session; + if (ReferenceEquals(_session, oldSession)) _session = null; + _dataSubscription = null; + _alarmSubscription = null; + } - session.RegisterKeepAliveHandler(isGood => - { - if (!isGood) _ = HandleKeepAliveFailureAsync(); - }); - - CurrentConnectionInfo = BuildConnectionInfo(session); - TransitionState(ConnectionState.Connected, url); - Logger.Information("Failover succeeded to {EndpointUrl}", url); - - // Replay subscriptions - await ReplaySubscriptionsAsync(); + if (_settings == null || _allEndpointUrls == null) + { + TransitionState(ConnectionState.Disconnected, oldEndpoint); return; } - catch (Exception ex) - { - Logger.Warning(ex, "Failover to {EndpointUrl} failed", url); - } - } - Logger.Error("All failover endpoints unreachable"); - TransitionState(ConnectionState.Disconnected, oldEndpoint); + // Try each endpoint + for (var attempt = 0; attempt < _allEndpointUrls.Length; attempt++) + { + _currentEndpointIndex = (_currentEndpointIndex + 1) % _allEndpointUrls.Length; + var url = _allEndpointUrls[_currentEndpointIndex]; + + try + { + Logger.Information("Failover attempt to {EndpointUrl}", url); + var session = await ConnectToEndpointAsync(_settings, url, CancellationToken.None); + + // A DisconnectAsync / Dispose that ran while this attempt was connecting wins: do not resurrect + // the connection behind the caller's back. + if (_disposed || _state == ConnectionState.Disconnected) + { + session.Dispose(); + return; + } + + _session = session; + + session.RegisterKeepAliveHandler(isGood => + { + if (!isGood) LastFailover = HandleKeepAliveFailureAsync(); + }); + + CurrentConnectionInfo = BuildConnectionInfo(session); + TransitionState(ConnectionState.Connected, url); + Logger.Information("Failover succeeded to {EndpointUrl}", url); + + // Replay subscriptions + await ReplaySubscriptionsAsync(session); + return; + } + catch (Exception ex) + { + Logger.Warning(ex, "Failover to {EndpointUrl} failed", url); + } + } + + Logger.Error("All failover endpoints unreachable"); + TransitionState(ConnectionState.Disconnected, oldEndpoint); + } + finally + { + _subscriptionGate.Release(); + } } - private async Task ReplaySubscriptionsAsync() + /// Re-creates the recorded subscriptions on . Runs under the subscription gate. + private async Task ReplaySubscriptionsAsync(ISessionAdapter session) { // Snapshot the bookkeeping state under the lock, then clear it so the // replayed handles can be recorded fresh as each monitored item is @@ -788,13 +872,17 @@ public sealed class OpcUaClientService : IOpcUaClientService } // Replay data subscriptions + ISubscriptionAdapter? dataSubscription = null; foreach (var (nodeIdStr, (nodeId, intervalMs, _)) in subscriptions) try { - if (_dataSubscription == null) - _dataSubscription = await _session!.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + if (dataSubscription == null) + { + dataSubscription = await session.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + lock (_subscriptionLock) _dataSubscription = dataSubscription; + } - var handle = await _dataSubscription.AddDataChangeMonitoredItemAsync( + var handle = await dataSubscription.AddDataChangeMonitoredItemAsync( nodeId, intervalMs, OnDataChangeNotification, CancellationToken.None); lock (_subscriptionLock) @@ -814,9 +902,10 @@ public sealed class OpcUaClientService : IOpcUaClientService try { var monitorNode = sourceNodeId ?? ObjectIds.Server; - _alarmSubscription = await _session!.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + var replayedAlarms = await session.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + lock (_subscriptionLock) _alarmSubscription = replayedAlarms; var filter = CreateAlarmEventFilter(); - await _alarmSubscription.AddEventMonitoredItemAsync( + await replayedAlarms.AddEventMonitoredItemAsync( monitorNode, intervalMs, filter, OnAlarmEventNotification, CancellationToken.None); lock (_subscriptionLock) diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs index 14f1df52..27504765 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs @@ -167,15 +167,31 @@ internal sealed class FakeSessionAdapter : ISessionAdapter return Task.FromResult>(HistoryReadAggregateResponse); } + /// + /// Optional gate that, when set, holds open until completed, so a test can keep + /// a subscribe in flight while something else (a failover) happens. Signalled via . + /// + public TaskCompletionSource? CreateSubscriptionGate { get; set; } + + /// Completed when is entered while a gate is set. + public TaskCompletionSource CreateSubscriptionEntered { get; } = + new(TaskCreationOptions.RunContinuationsAsynchronously); + /// - public Task CreateSubscriptionAsync(int publishingIntervalMs, CancellationToken ct) + public async Task CreateSubscriptionAsync(int publishingIntervalMs, CancellationToken ct) { + if (CreateSubscriptionGate is { } gate) + { + CreateSubscriptionEntered.TrySetResult(); + await gate.Task; + } + lock (_createdSubscriptions) { var sub = NextSubscription ?? new FakeSubscriptionAdapter(); NextSubscription = null; _createdSubscriptions.Add(sub); - return Task.FromResult(sub); + return sub; } } diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs index f1035075..a0b89bb9 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs @@ -1446,6 +1446,97 @@ public class OpcUaClientServiceTests : IDisposable _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe("opc.tcp://backup:4840"); } + /// + /// R08 4.5: a ConnectAsync on an already-connected client is refused instead of overwriting (and leaking) the live + /// session. The positive control: after a disconnect the same client connects again. + /// + [Fact] + public async Task ConnectAsync_WhenAlreadyConnected_IsRefused_AndKeepsTheLiveSession() + { + var session1 = new FakeSessionAdapter(); + _sessionFactory.EnqueueSession(session1); + await _service.ConnectAsync(ValidSettings()); + + await Should.ThrowAsync(() => _service.ConnectAsync(ValidSettings())); + + _sessionFactory.CreateCallCount.ShouldBe(1); + session1.Disposed.ShouldBeFalse(); + _service.IsConnected.ShouldBeTrue(); + + await _service.DisconnectAsync(); + await _service.ConnectAsync(ValidSettings()); + _sessionFactory.CreateCallCount.ShouldBe(2); + _service.IsConnected.ShouldBeTrue(); + } + + /// + /// R08 4.5: a SubscribeAsync in flight when a keep-alive failover starts must not be lost. Failover runs under the + /// subscription gate, so it waits for the in-flight subscribe to record its item and then replays it onto the new + /// session. Before the fix, failover nulled the subscription state outside the lock and finished first; the subscribe + /// then completed onto the dying session's subscription and was never replayed. + /// + [Fact] + public async Task SubscribeAsync_in_flight_during_failover_is_replayed_onto_the_new_session() + { + var session1 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://primary:4840" }; + var session2 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://backup:4840" }; + _sessionFactory.EnqueueSession(session1); + _sessionFactory.EnqueueSession(session2); + var settings = ValidSettings("opc.tcp://primary:4840"); + settings.FailoverUrls = ["opc.tcp://backup:4840"]; + await _service.ConnectAsync(settings); + + var gate = new TaskCompletionSource(); + session1.CreateSubscriptionGate = gate; + var subscribe = _service.SubscribeAsync(new NodeId("ns=2;s=Racer"), 1000); + await session1.CreateSubscriptionEntered.Task.WaitAsync(TimeSpan.FromSeconds(10)); + + session1.SimulateKeepAlive(false); + var failover = _service.LastFailover.ShouldNotBeNull(); + + gate.SetResult(); + await subscribe.WaitAsync(TimeSpan.FromSeconds(10)); + await failover.WaitAsync(TimeSpan.FromSeconds(10)); + + _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe("opc.tcp://backup:4840"); + var replayed = session2.CreatedSubscriptions.ShouldHaveSingleItem(); + replayed.AddDataChangeCount.ShouldBe(1); + + // And it is the live subscription: unsubscribing removes it from session2's subscription, not session1's. + await _service.UnsubscribeAsync(new NodeId("ns=2;s=Racer")); + replayed.RemoveCount.ShouldBe(1); + replayed.ActiveHandles.ShouldBeEmpty(); + } + + /// + /// A DisconnectAsync issued while a failover is still connecting wins: the failover disposes the session it just + /// opened instead of resurrecting the connection. The positive control is the plain failover test above, where the + /// same gated attempt ends Connected. + /// + [Fact] + public async Task DisconnectAsync_during_failover_is_not_undone_by_the_failover() + { + var session1 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://primary:4840" }; + var session2 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://backup:4840" }; + _sessionFactory.EnqueueSession(session1); + _sessionFactory.EnqueueSession(session2); + var settings = ValidSettings("opc.tcp://primary:4840"); + settings.FailoverUrls = ["opc.tcp://backup:4840"]; + await _service.ConnectAsync(settings); + + var gate = new TaskCompletionSource(); + _sessionFactory.CreateGate = gate; + session1.SimulateKeepAlive(false); + var failover = _service.LastFailover.ShouldNotBeNull(); + + await _service.DisconnectAsync(); + gate.SetResult(); + await failover.WaitAsync(TimeSpan.FromSeconds(10)); + + _service.IsConnected.ShouldBeFalse(); + session2.Disposed.ShouldBeTrue(); + } + /// /// Regression for Client.Shared-005: concurrent subscribe/unsubscribe calls mutating the /// active-subscription bookkeeping must not corrupt the dictionary or throw. From f9fbd4ee0d8d25c658326431c5f97549c754d17e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 16:34:48 -0400 Subject: [PATCH 05/10] fix(cli): AbCip subscribe serialises and guards console writes; Ctrl-C exits cleanly; arrays render R07 U7: Driver.Cli.Common gains RunCancellableAsync (moved up from AbLegacy) and CreateChangeLineWriter (one lock-serialised line per change; a failing console write is logged and swallowed so it cannot fault the poll loop). AbCip probe/read/write print Cancelled. on Ctrl-C instead of a stack trace, and AbCip subscribe uses the shared writer. SnapshotFormatter renders arrays/lists as [a, b, c] instead of System.Int32[]. Refs #747 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Commands/ProbeCommand.cs | 24 ++-- .../Commands/ReadCommand.cs | 10 +- .../Commands/SubscribeCommand.cs | 23 ++-- .../Commands/WriteCommand.cs | 10 +- .../AbLegacyCommandBase.cs | 21 ---- .../DriverCommandBase.cs | 56 +++++++++ .../SnapshotFormatter.cs | 20 +++- .../CancellationHandlingTests.cs | 50 ++++++++ .../CliHelpersU7Tests.cs | 111 ++++++++++++++++++ 9 files changed, 277 insertions(+), 48 deletions(-) create mode 100644 tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs create mode 100644 tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs index 804ff1bc..c914d160 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs @@ -44,17 +44,21 @@ public sealed class ProbeCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync(["__probe"], ct); - var health = driver.GetHealth(); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync(["__probe"], ct); + var health = driver.GetHealth(); - await console.Output.WriteLineAsync($"Gateway: {Gateway}"); - await console.Output.WriteLineAsync($"Family: {Family}"); - await console.Output.WriteLineAsync($"Health: {health.State}"); - if (health.LastError is { } err) - await console.Output.WriteLineAsync($"Last error: {err}"); - await console.Output.WriteLineAsync(); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + await console.Output.WriteLineAsync($"Gateway: {Gateway}"); + await console.Output.WriteLineAsync($"Family: {Family}"); + await console.Output.WriteLineAsync($"Health: {health.State}"); + if (health.LastError is { } err) + await console.Output.WriteLineAsync($"Last error: {err}"); + await console.Output.WriteLineAsync(); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs index 0d0bbb80..0e4b5ad4 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs @@ -45,9 +45,13 @@ public sealed class ReadCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync([tagName], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync([tagName], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs index b266fa3d..399b3a0e 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs @@ -50,23 +50,22 @@ public sealed class SubscribeCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); ISubscriptionHandle? handle = null; + // The poll engine raises OnDataChange on a background thread: serialise its lines against each other and + // against this thread's own, and never let a failed console write fault the poll loop (review 07 U7). + var writeLock = new object(); try { await driver.InitializeAsync("{}", ct); - await console.Output.WriteLineAsync( - $"Subscribed to {TagPath} @ {IntervalMs}ms. Ctrl+C to stop."); - - driver.OnDataChange += (_, e) => - { - var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + - $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + - $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; - console.Output.WriteLine(line); - }; + driver.OnDataChange += CreateChangeLineWriter(console.Output, writeLock); handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct); + lock (writeLock) + { + console.Output.WriteLine($"Subscribed to {TagPath} @ {IntervalMs}ms. Ctrl+C to stop."); + } + try { await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct); @@ -76,6 +75,10 @@ public sealed class SubscribeCommand : AbCipCommandBase // Expected on Ctrl+C. } } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Ctrl+C while still connecting/subscribing: a clean exit, not a stack trace (review 07 U7). + } finally { if (handle is not null) diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs index e4dadd38..b4473e02 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs @@ -57,9 +57,13 @@ public sealed class WriteCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(TagPath, results[0])); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(TagPath, results[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs index 157caf2d..e96b1a75 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs @@ -32,27 +32,6 @@ public abstract class AbLegacyCommandBase : DriverCommandBase "LogixPccc unverified).")] public AbLegacyCliTransport Transport { get; init; } = AbLegacyCliTransport.Default; - /// - /// Runs a one-shot command's driver work so a Ctrl-C (cancellation of ) prints - /// Cancelled. instead of an unhandled stack trace (review 07 U7). - /// Any other exception — and an OCE the operator did not ask for — still propagates. - /// - /// The CLI console. - /// The console's Ctrl-C token. - /// The command body. - /// A task that completes when the body has finished or was cancelled. - internal static async ValueTask RunCancellableAsync(CliFx.Infrastructure.IConsole console, CancellationToken ct, Func body) - { - try - { - await body(); - } - catch (OperationCanceledException) when (ct.IsCancellationRequested) - { - await console.Error.WriteLineAsync("Cancelled."); - } - } - /// Gets the per-operation timeout in milliseconds. [CommandOption("timeout-ms", Description = "Per-operation timeout in ms (default 5000).")] public int TimeoutMs { get; init; } = 5000; diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs index d77bff85..d5a087fd 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs @@ -79,5 +79,61 @@ public abstract class DriverCommandBase : ICommand /// protected static void FlushLogging() => Log.CloseAndFlush(); + /// + /// Runs a one-shot command's driver work so a Ctrl-C (cancellation of ) prints + /// Cancelled. instead of an unhandled stack trace (review 07 U7). + /// Any other exception — and an OCE the operator did not ask for — still propagates. + /// + /// The CLI console. + /// The console's Ctrl-C token. + /// The command body. + /// A task that completes when the body has finished or was cancelled. + public static async ValueTask RunCancellableAsync(IConsole console, CancellationToken ct, Func body) + { + try + { + await body(); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + await console.Error.WriteLineAsync("Cancelled."); + } + } + + /// + /// Builds the OnDataChange handler a subscribe command attaches: one + /// [HH:mm:ss.fff] ref = value (status) line per change. Writes are serialised on + /// (the poll engine raises from a background thread, concurrently with the + /// command thread's own lines), and a failing console write (closed pipe, redirected stream error) is logged + /// and swallowed so it cannot fault the driver's poll loop (review 07 U7). + /// + /// The console output writer. + /// The lock every console write of the command takes. + /// The event handler. + public static EventHandler CreateChangeLineWriter( + TextWriter output, object writeLock) + { + ArgumentNullException.ThrowIfNull(output); + ArgumentNullException.ThrowIfNull(writeLock); + return (_, e) => + { + try + { + var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + + $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + + $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; + lock (writeLock) + { + output.WriteLine(line); + } + } + catch (Exception ex) + { + Log.Logger.Warning(ex, "subscribe: console write failed for {Tag}; continuing the poll loop.", + e.FullReference); + } + }; + } + private bool _loggingConfigured; } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs index 906fee8e..9a49acdb 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs @@ -101,7 +101,11 @@ public static class SnapshotFormatter return sb.ToString().TrimEnd(); } - /// Formats a value for console output, handling null, bool, string, and formattable types. + /// + /// Formats a value for console output, handling null, bool, string, formattable and array/list types. An + /// array renders its elements ([1, 2, 3], each formatted as a scalar) rather than its CLR type name + /// (System.Int32[]). + /// /// The value to format. /// A formatted string representation of the value. public static string FormatValue(object? value) => value switch @@ -110,9 +114,23 @@ public static class SnapshotFormatter bool b => b ? "true" : "false", string s => $"\"{s}\"", IFormattable f => f.ToString(null, CultureInfo.InvariantCulture), + System.Collections.IEnumerable items => FormatSequence(items), _ => value.ToString() ?? "", }; + private static string FormatSequence(System.Collections.IEnumerable items) + { + var sb = new System.Text.StringBuilder("["); + var first = true; + foreach (var item in items) + { + if (!first) sb.Append(", "); + sb.Append(FormatValue(item)); + first = false; + } + return sb.Append(']').ToString(); + } + /// Formats an OPC UA status code as a hexadecimal string with named severity classification. /// The OPC UA status code to format. /// A formatted status string like "0x00000000 (Good)" or "0x80050000 (BadCommunicationError)". diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs new file mode 100644 index 00000000..cad52821 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs @@ -0,0 +1,50 @@ +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests; + +/// +/// Review 07 U7: the AbCip probe/read/write commands had no arm, so a +/// Ctrl-C printed a stack trace. A command cancelled by the operator now exits cleanly with Cancelled.. +/// The gateway is TEST-NET (never answers), so nothing is sent to a real device. +/// +[Trait("Category", "Unit")] +public sealed class CancellationHandlingTests +{ + private const string Blackhole = "ab://192.0.2.1/1,0"; + + private static async Task RunCancelledAsync(AbCipCommandBase command) + { + using var console = new FakeInMemoryConsole(); + console.RequestCancellation(); + + await Should.NotThrowAsync(async () => await command.ExecuteAsync(console)); + return console.ReadErrorString(); + } + + /// A cancelled read prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_read_prints_Cancelled() + { + (await RunCancelledAsync(new ReadCommand { Gateway = Blackhole, TagPath = "Motor01" })) + .ShouldContain("Cancelled."); + } + + /// A cancelled probe prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_probe_prints_Cancelled() + { + (await RunCancelledAsync(new ProbeCommand { Gateway = Blackhole, TagPath = "Motor01" })) + .ShouldContain("Cancelled."); + } + + /// A cancelled write prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_write_prints_Cancelled() + { + (await RunCancelledAsync(new WriteCommand { Gateway = Blackhole, TagPath = "Motor01", Value = "1" })) + .ShouldContain("Cancelled."); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs new file mode 100644 index 00000000..ecdf93f5 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs @@ -0,0 +1,111 @@ +using System.Text; +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests; + +/// +/// Review 07 U7, shared half: arrays render their elements (not System.Int32[]), Ctrl-C prints +/// Cancelled., and the subscribe change-line writer is serialised and never throws into the poll loop. +/// +[Trait("Category", "Unit")] +public sealed class CliHelpersU7Tests +{ + private sealed record Handle(string DiagnosticId) : ISubscriptionHandle; + + private static DataChangeEventArgs Change(string reference, object? value) => + new(new Handle("h"), reference, new DataValueSnapshot(value, 0u, DateTime.UtcNow, DateTime.UtcNow)); + + /// An array value renders as its elements, each formatted like a scalar. + [Fact] + public void FormatValue_renders_array_elements() + { + SnapshotFormatter.FormatValue(new[] { 1, 2, 3 }).ShouldBe("[1, 2, 3]"); + SnapshotFormatter.FormatValue(new[] { 1.5f, -2f }).ShouldBe("[1.5, -2]"); + SnapshotFormatter.FormatValue(new[] { true, false }).ShouldBe("[true, false]"); + SnapshotFormatter.FormatValue(new[] { "a", "b" }).ShouldBe("[\"a\", \"b\"]"); + SnapshotFormatter.FormatValue(new List { 7 }).ShouldBe("[7]"); + SnapshotFormatter.FormatValue(Array.Empty()).ShouldBe("[]"); + } + + /// A string is still a scalar, not a sequence of chars (positive control for the array arm). + [Fact] + public void FormatValue_keeps_a_string_scalar() + { + SnapshotFormatter.FormatValue("abc").ShouldBe("\"abc\""); + } + + /// Operator Ctrl-C prints Cancelled. instead of a stack trace. + [Fact] + public async Task RunCancellableAsync_operator_cancellation_prints_Cancelled() + { + using var console = new FakeInMemoryConsole(); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await Should.NotThrowAsync(async () => + await DriverCommandBase.RunCancellableAsync(console, cts.Token, () => Task.FromCanceled(cts.Token))); + + console.ReadErrorString().ShouldContain("Cancelled."); + } + + /// A cancellation the operator did not request still propagates. + [Fact] + public async Task RunCancellableAsync_unrequested_cancellation_propagates() + { + using var console = new FakeInMemoryConsole(); + + await Should.ThrowAsync(async () => + await DriverCommandBase.RunCancellableAsync(console, CancellationToken.None, + () => throw new OperationCanceledException())); + } + + /// A change line carries the reference, the formatted value and the status. + [Fact] + public void ChangeLineWriter_writes_one_formatted_line_per_change() + { + var output = new StringWriter(); + var write = DriverCommandBase.CreateChangeLineWriter(output, new object()); + + write(this, Change("Tag:DInt", new[] { 4, 5 })); + + var line = output.ToString().TrimEnd(); + line.ShouldContain("Tag:DInt = [4, 5] (0x00000000 (Good))"); + } + + /// Concurrent poll-thread changes produce whole lines, never interleaved fragments. + [Fact] + public async Task ChangeLineWriter_serialises_concurrent_writes() + { + var output = new StringWriter(new StringBuilder()); + var write = DriverCommandBase.CreateChangeLineWriter(output, new object()); + + await Task.WhenAll(Enumerable.Range(0, 8).Select(t => Task.Run(() => + { + for (var i = 0; i < 200; i++) write(this, Change($"T{t}", i)); + }))); + + var lines = output.ToString().Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries); + lines.Length.ShouldBe(1600); + lines.ShouldAllBe(l => l.StartsWith('[') && l.EndsWith("(Good))")); + } + + /// A failing console (closed pipe) is swallowed so it cannot fault the driver's poll loop. + [Fact] + public void ChangeLineWriter_swallows_a_console_write_failure() + { + var write = DriverCommandBase.CreateChangeLineWriter(new ThrowingWriter(), new object()); + + Should.NotThrow(() => write(this, Change("T", 1))); + } + + private sealed class ThrowingWriter : TextWriter + { + public override Encoding Encoding => Encoding.UTF8; + public override void Write(char value) => throw new IOException("pipe closed"); + public override void WriteLine(string? value) => throw new IOException("pipe closed"); + } +} From dfc62cef14e30c3561b9d2a00afdbc2bb836d522 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 16:42:57 -0400 Subject: [PATCH 06/10] fix(ab-transport): map CIP 0x1C/0x20/0xFF-0x2104 as refusals; remap the PCCC STS table K-F1 (AbCip #737 F1): FromCip maps general 0x1C and 0x20 to Unsupported and 0xFF/0x2104 to OutOfBounds, so a controller refusal is per-tag Bad instead of a BadReply comms fault. A Forward Open refused with one of them is still BadConnection. Every CIP code is now listed with its meaning; 0x01/0x07 stay opaque on purpose. K-F3 (AbLegacy F8, review 07 r0 I4): FromPccc follows the DF1 STS table: 0x20/0x30/0x90 -> BadConnection (a bridge reporting an unreachable END node), 0x50 and EXT 0x06 -> NotFound, EXT 0x07 -> OutOfBounds, 0x60 -> NotAllowed, 0x70/0xB0 -> DeviceStateConflict. Golden ParseReply tests per STS, and an end-to-end AbLegacy test: STS 0x30 degrades with LastSuccessfulRead preserved; the 0x60 positive control stays Healthy and advances it. Both were red before. Driver edits are doc remarks and the two runtime tests the old table pinned. Refs #747 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbStatusCodes.cs | 76 ++++++++++++---- .../Cip/ConnectionManagerServices.cs | 11 ++- .../AbCipRefusals.cs | 8 +- .../AbLegacyDriver.cs | 8 +- .../AbStatusCodesTests.cs | 43 +++++++-- .../Cip/ConnectionManagerServicesTests.cs | 24 +++++ .../Pccc/PcccServicesTests.cs | 26 ++++++ .../NativeAbCipTagRuntimeTests.cs | 5 + .../NativeAbLegacyTagRuntimeTests.cs | 22 ++++- .../NativePcccBridgeStatusHealthTests.cs | 91 +++++++++++++++++++ 10 files changed, 278 insertions(+), 36 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs index b9d8d674..39c5b41d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs @@ -1,39 +1,81 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +/// +/// Maps wire statuses (CIP general/extended, PCCC STS/EXT STS) to the transport-neutral the +/// drivers classify. The class an lands in is load-bearing for driver health (#527/#737/#738): +/// an ANSWERED refusal (NotFound / NotAllowed / OutOfBounds / TooLarge / TooSmall / TypeMismatch / +/// DeviceStateConflict, and for AbCip Unsupported) proves the device replied; +/// is the no-reply comms class; / are a reply that +/// cannot be classified. Every code not named below is deliberately . +/// public static class AbStatusCodes { /// Maps a CIP general status (and optional extended status words) to an . /// The CIP general status byte. /// The extended status words, if any. /// The mapped status. + /// + /// 0x01 (connection failure, incl. a routing hop's unconnected-send timeout) and 0x07 (connection lost) stay + /// on purpose: a bridge reports a dead END device that way, so they are neither an + /// answer nor proof of no reply (AbLegacy #738 "opaque"). A Forward Open refused with one of the request-level + /// codes (0x1C / 0x20 / 0xFF-0x2104) stays a connection failure — see + /// ConnectionManagerServices.ParseForwardOpenReply. + /// public static AbStatus FromCip(byte general, ReadOnlySpan extended) => general switch { - 0x00 or 0x0B => AbStatus.Ok, - 0x04 or 0x05 or 0x16 => AbStatus.NotFound, - 0x06 => AbStatus.Partial, - 0x08 => AbStatus.Unsupported, - 0x0A => AbStatus.OutOfBounds, - 0x0C or 0x10 => AbStatus.DeviceStateConflict, - 0x0E or 0x0F => AbStatus.NotAllowed, - 0x13 => AbStatus.TooSmall, - 0x15 => AbStatus.TooLarge, - 0xFF when extended.Length > 0 && extended[0] == 0x2105 => AbStatus.OutOfBounds, - 0xFF when extended.Length > 0 && extended[0] == 0x2107 => AbStatus.TypeMismatch, + 0x00 or 0x0B => AbStatus.Ok, // success / already in requested mode + 0x04 or 0x05 or 0x16 => AbStatus.NotFound, // path segment error / path unknown / object does not exist + 0x06 => AbStatus.Partial, // partial transfer (fragmented read continues) + 0x08 => AbStatus.Unsupported, // service not supported + 0x0A => AbStatus.OutOfBounds, // attribute list error + 0x0C or 0x10 => AbStatus.DeviceStateConflict, // object state conflict / device state conflict + 0x0E or 0x0F => AbStatus.NotAllowed, // attribute not settable / privilege violation + 0x13 => AbStatus.TooSmall, // not enough data + 0x15 => AbStatus.TooLarge, // too much data + // Lane K F1 (#737): the controller answered and refused this one request — not a garbled reply, so not a + // comms fault that degrades the driver and evicts the runtime. + 0x1C => AbStatus.Unsupported, // missing attribute list entry data + 0x20 => AbStatus.Unsupported, // invalid parameter + 0xFF when extended.Length > 0 && extended[0] == 0x2104 => AbStatus.OutOfBounds, // offset beyond end of tag + 0xFF when extended.Length > 0 && extended[0] == 0x2105 => AbStatus.OutOfBounds, // beyond end of the object + 0xFF when extended.Length > 0 && extended[0] == 0x2107 => AbStatus.TypeMismatch, // data type mismatch _ => AbStatus.BadReply, }; + /// + /// True for the CIP statuses Lane K F1 moved from to a request-level refusal. + /// A Forward Open refused with one of them is still a connection failure. + /// + /// The CIP general status byte. + /// The extended status words, if any. + /// for 0x1C, 0x20 and 0xFF/0x2104. + internal static bool IsRequestLevelRefusal(byte general, ReadOnlySpan extended) => + general is 0x1C or 0x20 || (general == 0xFF && extended.Length > 0 && extended[0] == 0x2104); + /// Maps a PCCC status byte (and optional extended status) to an . /// The PCCC STS byte. /// The extended status byte, when present. /// The mapped status. + /// + /// Per the DF1/PCCC STS table (1770-6.5.16). Lane K F3 (AbLegacy F8, review 07 r0 I4): 0x20, 0x30 and 0x90 are + /// reported by the HOST or a BRIDGE (a DHRIO, a 1761-NET-ENI) when the END device cannot be reached, so they are + /// the no-reply comms class. Mapping them to an answered refusal made a dead DH+/DH-485 node behind a bridge count + /// as reachable forever — no fail-fast backoff, no comms-loss watchdog. + /// public static AbStatus FromPccc(byte sts, byte? extSts) => sts switch { 0x00 => AbStatus.Ok, - 0x10 => AbStatus.Unsupported, - 0x20 => AbStatus.NotFound, - 0x30 => AbStatus.NotAllowed, - 0x40 or 0x50 => AbStatus.DeviceStateConflict, - 0xF0 when extSts is 0x06 or 0x07 => AbStatus.NotFound, - _ => AbStatus.BadReply, + 0x10 => AbStatus.Unsupported, // illegal command or format + 0x20 => AbStatus.BadConnection, // host has a problem and will not communicate + 0x30 => AbStatus.BadConnection, // remote node host missing, disconnected or shut down + 0x40 => AbStatus.DeviceStateConflict, // host could not complete: hardware fault + 0x50 => AbStatus.NotFound, // addressing problem or memory protect rungs + 0x60 => AbStatus.NotAllowed, // function not allowed due to command protection + 0x70 => AbStatus.DeviceStateConflict, // processor is in Program mode + 0x90 => AbStatus.BadConnection, // remote node cannot buffer command + 0xB0 => AbStatus.DeviceStateConflict, // remote node problem due to download + 0xF0 when extSts is 0x06 => AbStatus.NotFound, // EXT: address does not point to something usable + 0xF0 when extSts is 0x07 => AbStatus.OutOfBounds, // EXT: file is wrong size + _ => AbStatus.BadReply, // 0x80, 0xA0, 0xC0, other EXT STS: unclassified }; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs index 95d2dee7..91d59600 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs @@ -85,8 +85,15 @@ public static class ConnectionManagerServices public static ForwardOpenReply ParseForwardOpenReply(CipReply reply) { if (reply.GeneralStatus != 0) - throw new ForwardOpenRefusedException(reply.Status == AbStatus.BadReply ? AbStatus.BadConnection : reply.Status, - reply.GeneralStatus, reply.ExtendedStatus); + { + // An unclassified refusal, or one of the request-level codes AbStatusCodes maps to a per-request refusal + // for TAG services (0x1C / 0x20 / 0xFF-0x2104), is a connection failure here: no tag can be served. + var status = reply.Status == AbStatus.BadReply + || AbStatusCodes.IsRequestLevelRefusal(reply.GeneralStatus, reply.ExtendedStatus) + ? AbStatus.BadConnection + : reply.Status; + throw new ForwardOpenRefusedException(status, reply.GeneralStatus, reply.ExtendedStatus); + } var r = new CipReader(reply.Data.Span); var ot = r.U32(); var to = r.U32(); r.U16(); r.U16(); r.U32(); return new ForwardOpenReply(ot, to, r.U32(), r.U32()); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs index 334568a1..2bded247 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs @@ -13,10 +13,10 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// never writes Degraded, never opens the device-wide connect backoff, and warns once per tag. Everything /// else (timeout, bad connection, a reply that failed to parse, partial, abort, …) is a comms fault. /// -/// The native transport currently folds CIP general statuses 0x1C, 0x20 and 0xFF/0x2104 into -/// (AbStatusCodes.FromCip, owned by Driver.Ab.Transport), which -/// this classification cannot tell from a garbled reply; until the transport maps them explicitly they stay -/// comms faults. +/// The native transport maps CIP general statuses 0x1C (missing attribute list entry) and 0x20 (invalid +/// parameter) to and 0xFF/0x2104 (offset beyond end) to +/// (AbStatusCodes.FromCip, Lane K F1), so they classify here as +/// device refusals. A Forward Open refused with one of them stays a connection failure. /// /// internal static class AbCipRefusals diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 2444f3c1..4d7657d7 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -838,10 +838,10 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag /// reachability (backoff reset, LastSuccessfulRead, Healthy). /// /// - /// ⚠️ Inherits the native transport's PCCC STS table (AbStatusCodes.FromPccc, Lane K): it maps STS 0x30 - /// ("remote node missing" — a dead DH+/DH-485 node behind a bridge) and 0x20 to NotAllowed/NotFound, - /// so on the NATIVE transport such a dead node still reads as answered. Follow-up F8 (review 07 r0 I4); this - /// classification needs no change once it lands. + /// Inherits the native transport's PCCC STS table (AbStatusCodes.FromPccc). Since Lane K F3 (F8, review + /// 07 r0 I4) STS 0x20 / 0x30 / 0x90 — the host or a bridge reporting an unreachable END device — map to + /// (no reply), so a dead DH+/DH-485 node behind a bridge is a comms fault, + /// not an answer (NativePcccBridgeStatusHealthTests). /// /// The transport status. /// for a device refusal. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs index 905585bc..76ee84fb 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs @@ -21,10 +21,15 @@ public sealed class AbStatusCodesTests [InlineData((byte)0x0E, AbStatus.NotAllowed)] [InlineData((byte)0x1E, AbStatus.BadReply)] [InlineData((byte)0x01, AbStatus.BadReply)] + [InlineData((byte)0x07, AbStatus.BadReply)] // connection lost: opaque on purpose (AbLegacy #738) + // Lane K F1 (#737): the controller answered and refused this request — not a garbled reply. + [InlineData((byte)0x1C, AbStatus.Unsupported)] // missing attribute list entry data + [InlineData((byte)0x20, AbStatus.Unsupported)] // invalid parameter public void Cip_general_status_maps(byte general, AbStatus expected) => AbStatusCodes.FromCip(general, ReadOnlySpan.Empty).ShouldBe(expected); [Theory] + [InlineData((ushort)0x2104, AbStatus.OutOfBounds)] // offset beyond end of the tag (Lane K F1) [InlineData((ushort)0x2105, AbStatus.OutOfBounds)] [InlineData((ushort)0x2107, AbStatus.TypeMismatch)] [InlineData((ushort)0x9999, AbStatus.BadReply)] @@ -37,16 +42,42 @@ public sealed class AbStatusCodesTests [Theory] [InlineData((byte)0x00, null, AbStatus.Ok)] - [InlineData((byte)0x10, null, AbStatus.Unsupported)] - [InlineData((byte)0x20, null, AbStatus.NotFound)] - [InlineData((byte)0x30, null, AbStatus.NotAllowed)] - [InlineData((byte)0x40, null, AbStatus.DeviceStateConflict)] - [InlineData((byte)0xF0, (byte)0x06, AbStatus.NotFound)] - [InlineData((byte)0xF0, (byte)0x07, AbStatus.NotFound)] + [InlineData((byte)0x10, null, AbStatus.Unsupported)] // illegal command or format + // Lane K F3 (AbLegacy F8, review 07 r0 I4), per the DF1/PCCC STS table (1770-6.5.16): + [InlineData((byte)0x20, null, AbStatus.BadConnection)] // host has a problem and will not communicate + [InlineData((byte)0x30, null, AbStatus.BadConnection)] // remote node host missing / disconnected / shut down + [InlineData((byte)0x40, null, AbStatus.DeviceStateConflict)] // host hardware fault (the host answered) + [InlineData((byte)0x50, null, AbStatus.NotFound)] // addressing problem or memory protect rungs + [InlineData((byte)0x60, null, AbStatus.NotAllowed)] // function not allowed due to command protection + [InlineData((byte)0x70, null, AbStatus.DeviceStateConflict)] // processor is in Program mode + [InlineData((byte)0x80, null, AbStatus.BadReply)] // compatibility-mode file missing: unclassified + [InlineData((byte)0x90, null, AbStatus.BadConnection)] // remote node cannot buffer command + [InlineData((byte)0xB0, null, AbStatus.DeviceStateConflict)] // remote node problem due to download + [InlineData((byte)0xF0, (byte)0x06, AbStatus.NotFound)] // address does not point to something usable + [InlineData((byte)0xF0, (byte)0x07, AbStatus.OutOfBounds)] // file is wrong size [InlineData((byte)0xF0, (byte)0x0A, AbStatus.BadReply)] + [InlineData((byte)0xF0, null, AbStatus.BadReply)] public void Pccc_status_maps(byte sts, byte? ext, AbStatus expected) => AbStatusCodes.FromPccc(sts, ext).ShouldBe(expected); + /// + /// A dead DH+/DH-485 node behind a bridge answers STS 0x30 (and a busy one 0x90). Neither proves the END + /// device answered, so they are the no-reply comms class, never one of the "answered" refusals — that + /// classification made such a device count as reachable forever (no backoff, no comms-loss watchdog). + /// + /// The PCCC STS byte. + [Theory] + [InlineData((byte)0x20)] + [InlineData((byte)0x30)] + [InlineData((byte)0x90)] + public void Pccc_no_reply_statuses_are_never_answered_refusals(byte sts) + { + var status = AbStatusCodes.FromPccc(sts, null); + status.ShouldBe(AbStatus.BadConnection); + status.ShouldNotBeOneOf(AbStatus.NotFound, AbStatus.NotAllowed, AbStatus.OutOfBounds, + AbStatus.DeviceStateConflict, AbStatus.TooLarge, AbStatus.TooSmall, AbStatus.TypeMismatch); + } + [Fact] public void Ok_is_zero_and_every_error_is_negative() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs index 0d4c2003..ec362206 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs @@ -115,6 +115,30 @@ public sealed class ConnectionManagerServicesTests ex.Status.ShouldBe(AbStatus.BadConnection); // general 0x01 has no specific mapping } + /// + /// Lane K F1 remapped general 0x1C / 0x20 and 0xFF/0x2104 for TAG requests (a per-request refusal). A Forward + /// Open refused with one of them is still a connection failure, exactly as before: the whole connection is + /// unusable, so it must not read as a per-tag refusal. + /// + /// The refused Forward Open reply. + [Theory] + [InlineData("DB001C00")] + [InlineData("DB002000")] + [InlineData("DB00FF010421")] + public void Forward_open_refused_with_a_request_level_status_is_still_BadConnection(string hex) + { + var ex = Should.Throw(() => + ConnectionManagerServices.ParseForwardOpenReply(CipReply.Parse(Convert.FromHexString(hex)))); + ex.Status.ShouldBe(AbStatus.BadConnection); + } + + /// Positive control: a Forward Open refusal that was already specifically mapped keeps its status. + [Fact] + public void Forward_open_refused_service_not_supported_stays_Unsupported() => + Should.Throw(() => + ConnectionManagerServices.ParseForwardOpenReply(CipReply.Parse(Convert.FromHexString("DB000800")))) + .Status.ShouldBe(AbStatus.Unsupported); + [Fact] public void Refused_forward_open_exception_is_not_aliased_to_the_reply_array() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs index b8d36f68..d8c6edb7 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs @@ -90,6 +90,32 @@ public sealed class PcccServicesTests [Fact] public void Plc5_write_is_not_shipped_and_flagged() => PcccServices.Plc5WriteVerified.ShouldBeFalse(); + // ---- ParseReply: golden STS replies (Lane K F3 / AbLegacy F8) ---- + + /// + /// Wire-level golden replies: the Execute PCCC reply bytes a device (or a bridge in front of it) sends for each + /// STS, through , to the status the drivers classify. + /// + /// The reply's STS (+ EXT STS) bytes as hex. + /// The status ParseReply raises. + [Theory] + [InlineData("20", AbStatus.BadConnection)] + [InlineData("30", AbStatus.BadConnection)] + [InlineData("90", AbStatus.BadConnection)] + [InlineData("50", AbStatus.NotFound)] + [InlineData("60", AbStatus.NotAllowed)] + [InlineData("70", AbStatus.DeviceStateConflict)] + [InlineData("B0", AbStatus.DeviceStateConflict)] + [InlineData("F0|06", AbStatus.NotFound)] + [InlineData("F0|07", AbStatus.OutOfBounds)] + public void ParseReply_golden_sts(string stsHex, AbStatus expected) + { + var parts = stsHex.Split('|'); + var hex = Id + "4F" + parts[0] + "9987" + (parts.Length > 1 ? parts[1] : ""); + Should.Throw(() => PcccServices.ParseReply(Reply(hex), 0x8799)) + .Status.ShouldBe(expected); + } + // ---- ParseReply ---- private static CipReply Reply(string hex, byte general = 0) => new(0x4B, general, [], Hex(hex)); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs index 028d6e27..03434b0a 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs @@ -297,6 +297,11 @@ public sealed class NativeAbCipTagRuntimeTests [InlineData("D2000A00", AbStatus.OutOfBounds)] [InlineData("D2000F00", AbStatus.NotAllowed)] [InlineData("D200FF010521", AbStatus.OutOfBounds)] + // Lane K F1 (#737): these used to arrive as BadReply, i.e. a comms fault (Degraded + evict) for a tag the + // controller had simply refused. AbCipRefusals.IsDeviceRefusal classifies the new statuses with no driver change. + [InlineData("D2001C00", AbStatus.Unsupported)] // missing attribute list entry data + [InlineData("D2002000", AbStatus.Unsupported)] // invalid parameter + [InlineData("D200FF010421", AbStatus.OutOfBounds)] // offset beyond end of the tag public async Task A_refused_tag_completes_initialize_and_reports_the_refusal_on_read(string reply, AbStatus expected) { var ch = new FakeCipChannel { Respond = _ => Task.FromResult(Convert.FromHexString(reply)) }; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs index 8d5185fe..845a3a07 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs @@ -220,14 +220,30 @@ public sealed class NativeAbLegacyTagRuntimeTests } [Fact] - public async Task STS_0x20_on_initialize_is_NotFound() + public async Task STS_0x50_on_initialize_is_NotFound() { - await using var h = new Harness("slc500", "N99:0").Responder((_, _) => (0x20, null, [])); + await using var h = new Harness("slc500", "N99:0").Responder((_, _) => (0x50, null, [])); var ex = await Should.ThrowAsync(() => h.Runtime.InitializeAsync(CancellationToken.None)); ex.Status.ShouldBe(AbStatus.NotFound); h.Runtime.GetStatus().ShouldBe((int)AbStatus.NotFound); } + /// + /// Lane K F3 (F8): STS 0x20 / 0x30 come from the host or a bridge when the END device cannot be reached, so they + /// are the no-reply class (BadConnection), not an answered refusal. + /// + /// The PCCC STS. + [Theory] + [InlineData((byte)0x20)] + [InlineData((byte)0x30)] + public async Task STS_from_a_bridge_for_an_unreachable_node_is_BadConnection(byte sts) + { + await using var h = new Harness("slc500", "N7:0").Responder((_, _) => (sts, null, [])); + var ex = await Should.ThrowAsync(() => h.Runtime.InitializeAsync(CancellationToken.None)); + ex.Status.ShouldBe(AbStatus.BadConnection); + h.Runtime.GetStatus().ShouldBe((int)AbStatus.BadConnection); + } + [Fact] public async Task Initialize_performs_exactly_one_read() { @@ -305,7 +321,7 @@ public sealed class NativeAbLegacyTagRuntimeTests public async Task A_failed_second_write_chunk_is_reported_as_failed_not_Good() { await using var h = new Harness("slc500", "N7:0", count: 130).Responder((_, n) => - n == 1 ? (0x30, null, []) : (0x00, null, [])); + n == 1 ? (0x60, null, []) : (0x00, null, [])); var ex = await Should.ThrowAsync(() => h.Runtime.WriteAsync(CancellationToken.None)); ex.Status.ShouldBe(AbStatus.NotAllowed); h.Runtime.GetStatus().ShouldBe((int)AbStatus.NotAllowed); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs new file mode 100644 index 00000000..4829c654 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs @@ -0,0 +1,91 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Lane K F3 (AbLegacy F8, review 07 r0 I4), end to end over a real socket with the PRODUCTION +/// : a bridge (DHRIO / 1761-NET-ENI) in front of a dead DH+ node answers every +/// PCCC request with STS 0x30 "remote node host missing". That is NOT the device answering, so the driver must treat +/// it as a comms fault — Degraded, LastSuccessfulRead not advanced — or the comms-loss watchdog never fires +/// for it. The positive control is a live device refusing a tag (STS 0x60, command protection): per-tag Bad, Healthy, +/// LastSuccessfulRead advanced. +/// +[Trait("Category", "Unit")] +public sealed class NativePcccBridgeStatusHealthTests +{ + private static readonly byte[] FixedId = [0x07, 0x3D, 0xF3, 0x45, 0x43, 0x50, 0x21]; + + /// A PCCC fake that answers every request with one fixed STS. + private static FakeEipServer StsServer(byte sts) + { + var server = new FakeEipServer { ZeroReplyContext = true }; + server.Handler = req => + { + if (FakeEipServer.Command(req) != Ab.Transport.Eip.EncapCommand.SendRRData) return null; + var cip = FakeEipServer.RequestCip(req); + byte[] head = [0xCB, 0x00, 0x00, 0x00, .. FixedId, 0x4F]; + return FakeEipServer.RRReply(req, [.. head, sts, cip[15], cip[16]]); + }; + return server; + } + +#pragma warning disable OTOPCUA0001 // the driver is called directly to pin its OWN classification (no resilience wrap) + private static async Task DriverAsync(FakeEipServer server) + { + var host = $"ab://127.0.0.1:{server.Port}/1,0"; + var driver = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(host, AbLegacyPlcFamily.Slc500)], + RawTags = AbLegacyRawTags.Entries([new AbLegacyTagDefinition("T", host, "N7:0", AbLegacyDataType.Int)]), + Timeout = TimeSpan.FromSeconds(5), + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-pccc-bridge", new NativeAbLegacyTagFactory(new DeviceSessionPool())); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + return driver; + } + + /// STS 0x30 from a bridge: a comms fault, never evidence the device answered. + [Fact] + public async Task A_dead_node_behind_a_bridge_degrades_and_does_not_advance_LastSuccessfulRead() + { + var ct = TestContext.Current.CancellationToken; + await using var server = StsServer(0x30); + var driver = await DriverAsync(server); + var before = driver.GetHealth().LastSuccessfulRead; + + var result = (await driver.ReadAsync(["T"], ct)).Single(); + + result.StatusCode.ShouldBe(AbLegacyStatusMapper.BadCommunicationError); + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Degraded); + health.LastSuccessfulRead.ShouldBe(before); + await driver.ShutdownAsync(ct); + server.HandlerErrors.ShouldBeEmpty(); + } + + /// Positive control: STS 0x60 is the live device refusing the tag — answered, so Healthy and advanced. + [Fact] + public async Task A_live_device_refusing_the_tag_stays_Healthy_and_advances_LastSuccessfulRead() + { + var ct = TestContext.Current.CancellationToken; + await using var server = StsServer(0x60); + var driver = await DriverAsync(server); + var before = driver.GetHealth().LastSuccessfulRead; + + var result = (await driver.ReadAsync(["T"], ct)).Single(); + + result.StatusCode.ShouldBe(AbOpcUaStatusMap.BadNotWritable); + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Healthy); + health.LastSuccessfulRead.ShouldNotBeNull(); + health.LastSuccessfulRead.ShouldNotBe(before); + await driver.ShutdownAsync(ct); + server.HandlerErrors.ShouldBeEmpty(); + } +#pragma warning restore OTOPCUA0001 +} From abbaac3615df4eb458f44fca32067223e5a85408 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 16:52:26 -0400 Subject: [PATCH 07/10] perf(ab-transport): pool CipWriter, reassembly and frame-header buffers R07:L5 (AB half): CipWriter rents its working buffer from ArrayPool and returns it in ToArray (which now completes the writer; later writes throw). A single-reply Logix read is one copy of the data (no List + ToArray); a fragmented read reassembles in a pooled buffer. EncapsulationClient reads the frame header into a per-session buffer (reads only happen under the session gate) instead of a new array per reply. Measured per op (scratch A/B, Release): frame encode 784 -> 200 B, Logix read 1994 -> 1655 B, 3-fragment read 11.9 -> 8.2 KB, EIP round trip 2.75 -> 2.15 KB; encode time 41-46 -> 28-39 ms per 500k, other timings within noise; Ab.Transport.Tests 3 s before and after. Modbus transports are left to a follow-up (its lane is still in review). Refs #747 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Eip/EncapsulationClient.cs | 13 +++- .../Logix/LogixTagClient.cs | 78 +++++++++++++------ .../Wire/CipWriter.cs | 48 ++++++++++-- .../Wire/CipWriterTests.cs | 57 ++++++++++++++ 4 files changed, 160 insertions(+), 36 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs index b746745e..8c911779 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs @@ -47,6 +47,10 @@ public sealed class EncapsulationClient : IAsyncDisposable private readonly SemaphoreSlim _gate = new(1, 1); private long _ctx; private ushort _seq; // connected sequence count; only touched under _gate + + // R07:L5: the 24-byte header of the frame being read. Frames are read only by RunExchangeAsync, which holds _gate + // for the whole exchange, so one buffer per session serves every read (it used to be a fresh array per reply). + private readonly byte[] _head = new byte[EncapsulationHeader.Size]; private volatile bool _faulted; private int _disposed; @@ -282,11 +286,12 @@ public sealed class EncapsulationClient : IAsyncDisposable private async Task ReadFrameAsync(CancellationToken ct) { - var head = new byte[EncapsulationHeader.Size]; - await _stream.ReadExactlyAsync(head, ct).ConfigureAwait(false); - var len = head[2] | (head[3] << 8); + // Only called under _gate (RunExchangeAsync), so the shared header buffer is never read concurrently. The frame + // itself is allocated exactly: the matched reply's CIP payload is returned as a slice of it and outlives the call. + await _stream.ReadExactlyAsync(_head, ct).ConfigureAwait(false); + var len = _head[2] | (_head[3] << 8); var frame = new byte[EncapsulationHeader.Size + len]; - head.CopyTo(frame, 0); + _head.CopyTo(frame, 0); await _stream.ReadExactlyAsync(frame.AsMemory(EncapsulationHeader.Size), ct).ConfigureAwait(false); return frame; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs index ba63cc38..85987cce 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs @@ -1,3 +1,4 @@ +using System.Buffers; using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Cip; namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Logix; @@ -52,34 +53,61 @@ public sealed class LogixTagClient(RequestScheduler scheduler, ICipChannel chann var reply = await _scheduler.SendAsync(LogixServices.ReadTagFragmented(path, elements, 0), ct, expectedReplyBytes: ExpectedReadReplyBytes(elements, elementBytes, 0)).ConfigureAwait(false); var (type, handle, first) = LogixServices.ParseReadReply(reply); // throws on any status but 0x00/0x06 - var data = new List(first.Length); - data.AddRange(first.Span); - var lastChunk = first.Length; - while (reply.GeneralStatus == StatusPartial) + // R07:L5: the common single-reply read is one copy of the reply data (it used to go through a List and a + // second ToArray); a fragmented read reassembles in a pooled buffer and copies out once at the end. + if (reply.GeneralStatus != StatusPartial) return (type, handle, first.ToArray()); + + var data = ArrayPool.Shared.Rent(Math.Max(first.Length * 4, 1024)); + var count = 0; + try { - if (lastChunk == 0) - throw new AbTransportException(AbStatus.BadReply, "Partial read reply carried no data; the fragment loop cannot progress."); - reply = await _scheduler.SendAsync( - LogixServices.ReadTagFragmented(path, elements, (uint)data.Count), ct, - expectedReplyBytes: ExpectedReadReplyBytes(elements, elementBytes, data.Count)).ConfigureAwait(false); - var (fragmentType, fragmentHandle, chunk) = LogixServices.ParseReadReply(reply); - if (fragmentType != type) - throw new AbTransportException(AbStatus.BadReply, - $"Read fragment type 0x{fragmentType:X4} differs from the first reply's 0x{type:X4}."); - // A mid-read change of the structure handle means the tag's type changed under us (a - // download / online edit); stitching the halves would yield a value of neither type. - if (fragmentHandle != handle) - throw new AbTransportException(AbStatus.BadReply, - $"Read fragment structure handle {FormatHandle(fragmentHandle)} differs from the first reply's {FormatHandle(handle)}; the tag changed mid-read."); - data.AddRange(chunk.Span); - lastChunk = chunk.Length; - if (data.Count > MaxReadBytes) - throw new AbTransportException(AbStatus.TooLarge, - $"Fragmented read exceeded {MaxReadBytes} bytes; the controller kept answering Partial."); - } + Append(ref data, ref count, first.Span); + var lastChunk = first.Length; - return (type, handle, data.ToArray()); + while (reply.GeneralStatus == StatusPartial) + { + if (lastChunk == 0) + throw new AbTransportException(AbStatus.BadReply, "Partial read reply carried no data; the fragment loop cannot progress."); + reply = await _scheduler.SendAsync( + LogixServices.ReadTagFragmented(path, elements, (uint)count), ct, + expectedReplyBytes: ExpectedReadReplyBytes(elements, elementBytes, count)).ConfigureAwait(false); + var (fragmentType, fragmentHandle, chunk) = LogixServices.ParseReadReply(reply); + if (fragmentType != type) + throw new AbTransportException(AbStatus.BadReply, + $"Read fragment type 0x{fragmentType:X4} differs from the first reply's 0x{type:X4}."); + // A mid-read change of the structure handle means the tag's type changed under us (a + // download / online edit); stitching the halves would yield a value of neither type. + if (fragmentHandle != handle) + throw new AbTransportException(AbStatus.BadReply, + $"Read fragment structure handle {FormatHandle(fragmentHandle)} differs from the first reply's {FormatHandle(handle)}; the tag changed mid-read."); + Append(ref data, ref count, chunk.Span); + lastChunk = chunk.Length; + if (count > MaxReadBytes) + throw new AbTransportException(AbStatus.TooLarge, + $"Fragmented read exceeded {MaxReadBytes} bytes; the controller kept answering Partial."); + } + + return (type, handle, data.AsSpan(0, count).ToArray()); + } + finally + { + ArrayPool.Shared.Return(data); + } + } + + /// Appends to a pooled buffer, growing it (rent bigger, copy, return) as needed. + private static void Append(ref byte[] buffer, ref int count, ReadOnlySpan chunk) + { + if (count + chunk.Length > buffer.Length) + { + var bigger = ArrayPool.Shared.Rent(Math.Max(buffer.Length * 2, count + chunk.Length)); + buffer.AsSpan(0, count).CopyTo(bigger); + ArrayPool.Shared.Return(buffer); + buffer = bigger; + } + chunk.CopyTo(buffer.AsSpan(count)); + count += chunk.Length; } private static string FormatHandle(ushort? handle) => handle is { } h ? $"0x{h:X4}" : "(none)"; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs index 8c86f186..94b77a79 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs @@ -1,17 +1,38 @@ +using System.Buffers; using System.Buffers.Binary; namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Wire; /// Growable little-endian writer for CIP / EIP frames. +/// +/// R07:L5: the working buffer is rented from (a 256-byte array per request used +/// to be allocated and thrown away) and handed back by , which therefore COMPLETES the writer: +/// the result is the caller's own array, a second returns another copy of it, and any further +/// write throws (the buffer belongs to the pool again). A writer abandoned +/// before (a builder that threw) simply leaves its buffer to the GC. +/// public sealed class CipWriter { - private byte[] _buf = new byte[256]; + private const int InitialCapacity = 256; + private byte[]? _buf = ArrayPool.Shared.Rent(InitialCapacity); + private byte[]? _result; + /// Gets the number of bytes written so far. public int Length { get; private set; } + private byte[] Buffer => _buf ?? throw new InvalidOperationException( + "This CipWriter was completed by ToArray(); its buffer has been returned to the pool."); + private Span Take(int n) { - if (Length + n > _buf.Length) Array.Resize(ref _buf, Math.Max(_buf.Length * 2, Length + n)); - var s = _buf.AsSpan(Length, n); + var buf = Buffer; + if (Length + n > buf.Length) + { + var bigger = ArrayPool.Shared.Rent(Math.Max(buf.Length * 2, Length + n)); + buf.AsSpan(0, Length).CopyTo(bigger); + ArrayPool.Shared.Return(buf); + _buf = buf = bigger; + } + var s = buf.AsSpan(Length, n); Length += n; return s; } @@ -45,14 +66,27 @@ public sealed class CipWriter /// The replacement value. /// The two bytes at are not already /// written (an offset in the spare buffer capacity would otherwise be patched silently and lost). + /// The writer was completed by . public void PatchU16(int offset, ushort v) { + var buf = Buffer; if (offset < 0 || offset > Length - 2) throw new ArgumentOutOfRangeException(nameof(offset), offset, $"Cannot patch 2 bytes at {offset}: only {Length} bytes written."); - BinaryPrimitives.WriteUInt16LittleEndian(_buf.AsSpan(offset, 2), v); + BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(offset, 2), v); } - /// Copies the written bytes to a new array. - /// The bytes written so far. - public byte[] ToArray() => _buf.AsSpan(0, Length).ToArray(); + /// + /// Copies the written bytes to a new array and returns the working buffer to the pool, completing the writer + /// (see the remarks on ). + /// + /// The bytes written, in an array the caller owns. + public byte[] ToArray() + { + if (_result is { } done) return (byte[])done.Clone(); + var buf = Buffer; + _result = buf.AsSpan(0, Length).ToArray(); + _buf = null; + ArrayPool.Shared.Return(buf); + return _result; + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs index d196d294..cf4a1965 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs @@ -25,3 +25,60 @@ public sealed class CipWriterTests w.ToArray().ShouldBe(new byte[] { 0, 0, 7 }); // nothing was patched } } + +/// +/// R07:L5: works in a buffer rented from and hands +/// it back on . The returned array must be the caller's own (a later writer reusing +/// the pooled buffer cannot change it), growth must keep every byte, and a writer used after ToArray must refuse +/// rather than write into a buffer it no longer owns. +/// +public sealed class CipWriterPoolingTests +{ + [Fact] + public void Growth_past_the_initial_buffer_keeps_every_byte() + { + var w = new CipWriter(); + for (var i = 0; i < 1000; i++) w.U8((byte)i); + var bytes = w.ToArray(); + bytes.Length.ShouldBe(1000); + bytes.Select((b, i) => b == (byte)i).ShouldAllBe(ok => ok); + } + + [Fact] + public void A_returned_array_is_not_affected_by_a_later_writer_reusing_the_pooled_buffer() + { + var first = new CipWriter().U32(0xAABBCCDD).ToArray(); + for (var i = 0; i < 50; i++) new CipWriter().U32(0x11111111).U32(0x22222222).ToArray(); + first.ShouldBe(new byte[] { 0xDD, 0xCC, 0xBB, 0xAA }); + } + + [Fact] + public void ToArray_twice_returns_equal_independent_copies() + { + var w = new CipWriter().U16(0x0102); + var a = w.ToArray(); + var b = w.ToArray(); + b.ShouldBe(a); + b.ShouldNotBeSameAs(a); + } + + [Fact] + public void Writing_after_ToArray_is_refused() + { + var w = new CipWriter().U8(1); + _ = w.ToArray(); + Should.Throw(() => w.U8(2)); + Should.Throw(() => w.PatchU16(0, 1)); + w.Length.ShouldBe(1); + } + + [Fact] + public void Nested_writers_produce_independent_frames() + { + // The Execute PCCC / encapsulation builders build an inner writer while an outer one is live. + var outer = new CipWriter().U8(0xAA); + var inner = new CipWriter().U8(0x01).U8(0x02).ToArray(); + var frame = outer.Bytes(inner).U8(0xBB).ToArray(); + frame.ShouldBe(new byte[] { 0xAA, 0x01, 0x02, 0xBB }); + } +} From 0c9e4f4e3d1fd0cb8fd74a874d51338904e93743 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 17:18:26 -0400 Subject: [PATCH 08/10] fix(client): a failover abandons itself once the user disconnects or reconnects; paging and teardown minors Review I1: ConnectAsync, DisconnectAsync and Dispose each start a new connection epoch under _subscriptionLock. A keep-alive failover runs under the epoch it started in and abandons itself the moment it moves: it installs its session only if the epoch still matches (checked and assigned under the lock, otherwise the session is disposed), its state transitions are epoch-guarded, it stops trying endpoints, and it never flips a user's new connection to Disconnected. A late keep-alive from a previous connection's session starts no failover. Gate-driven tests cover the success and all-fail arms, the stale keep-alive, with a genuine failover as the positive control (4 of them red before). Minors: m8 only the CompareExchange winner sets LastFailover; m5 the endpoint loop stops early, and a failed Unsubscribe still drops its bookkeeping; m1 a read cancelled mid-paging releases its cursor (bounded token) and rethrows, and the cap release no longer rides the caller's token; m2 results are trimmed to the cap; m3 a non-positive cap is refused (CLI and UI already bound it to >= 1); m6 the subscription adapter's Dispose uses one bounded bridge for delete + detach. Refs #747 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Adapters/DefaultSubscriptionAdapter.cs | 17 +- .../Helpers/HistoryReadPaging.cs | 72 ++++--- .../OpcUaClientService.cs | 143 ++++++++++---- .../FailoverEpochTests.cs | 177 ++++++++++++++++++ .../Fakes/FakeSessionFactory.cs | 13 +- .../Fakes/FakeSubscriptionAdapter.cs | 5 + .../Helpers/HistoryReadPagingTests.cs | 89 ++++++++- 7 files changed, 442 insertions(+), 74 deletions(-) create mode 100644 tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/FailoverEpochTests.cs diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs index 927e0094..6473abf6 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs @@ -145,17 +145,16 @@ internal sealed class DefaultSubscriptionAdapter : ISubscriptionAdapter /// public void Dispose() { + // One bounded bridge (5 s) for delete + detach, so a dispose never blocks longer than before (review m6). + // DetachAsync swallows its own failures; a failed delete must not skip the detach. try { - DisposeBridge.Run(ct => _subscription.DeleteAsync(true, ct)); - } - catch - { - } - - try - { - DisposeBridge.Run(DetachAsync); + DisposeBridge.Run(async ct => + { + try { await _subscription.DeleteAsync(true, ct); } + catch (Exception ex) { Logger.Warning(ex, "Error deleting subscription"); } + await DetachAsync(ct); + }); } catch { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs index 2ec33e93..16bb72b6 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs @@ -21,46 +21,76 @@ internal static class HistoryReadPaging public delegate Task PageReader( byte[]? continuationPoint, bool releaseContinuationPoints, CancellationToken ct); + /// The bound on a best-effort release that does not ride the caller's (possibly cancelled) token. + internal static readonly TimeSpan ReleaseTimeout = TimeSpan.FromSeconds(5); + /// Reads pages until the server finishes or the caller's cap is reached. /// The element type extracted from each page. /// Issues one HistoryRead for the single node. /// Extracts the page's elements from a result. - /// The caller's cap; <= 0 means no cap (follow every continuation point). + /// The caller's cap; must be positive. The result is trimmed to it. /// The node being read, for refusal messages. /// The caller's cancellation token. - /// Every element read. + /// At most elements. + /// is not positive: an uncapped client + /// read would materialise a tag's whole window (the client side of review 08 1.1). /// A page was refused (see ). + /// was cancelled mid-read; the outstanding + /// cursor is released (best effort) first. public static async Task> ReadAllAsync( PageReader readPage, Func> extract, int maxValues, NodeId nodeId, CancellationToken ct) { + ArgumentOutOfRangeException.ThrowIfNegativeOrZero(maxValues); + var all = new List(); byte[]? continuationPoint = null; - while (true) - { - var result = await readPage(continuationPoint, false, ct).ConfigureAwait(false); - if (result == null) return all; - - HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); - all.AddRange(extract(result)); - - continuationPoint = result.ContinuationPoint; - if (continuationPoint is not { Length: > 0 }) return all; - if (maxValues > 0 && all.Count >= maxValues) break; - } - - // Stopped by the client's own cap: the server still holds a cursor for this session. Release it; failing to - // is not worth failing a read whose values are already in hand. try { - await readPage(continuationPoint, true, ct).ConfigureAwait(false); + while (true) + { + var result = await readPage(continuationPoint, false, ct).ConfigureAwait(false); + if (result == null) return Trim(all, maxValues); + + HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); + all.AddRange(extract(result)); + + continuationPoint = result.ContinuationPoint; + if (continuationPoint is not { Length: > 0 }) return Trim(all, maxValues); + if (all.Count >= maxValues) break; + } } - catch (Exception ex) when (ex is not OperationCanceledException) + catch (OperationCanceledException) + { + // Cancelled between pages (or during page N+1) with a cursor outstanding: release it rather than leave it + // occupying one of the session's continuation-point slots, then surface the cancellation. + if (continuationPoint is { Length: > 0 }) await ReleaseAsync(readPage, continuationPoint, nodeId); + throw; + } + + // Stopped by the client's own cap: the server still holds a cursor for this session. Release it — on its own + // bounded token, so a cancellation racing the release cannot lose the values already in hand. + await ReleaseAsync(readPage, continuationPoint, nodeId); + return Trim(all, maxValues); + } + + private static List Trim(List all, int maxValues) + { + if (all.Count > maxValues) all.RemoveRange(maxValues, all.Count - maxValues); + return all; + } + + private static async Task ReleaseAsync(PageReader readPage, byte[] continuationPoint, NodeId nodeId) + { + try + { + using var cts = new CancellationTokenSource(ReleaseTimeout); + await readPage(continuationPoint, true, cts.Token).ConfigureAwait(false); + } + catch (Exception ex) { Logger.Warning(ex, "Releasing the HistoryRead continuation point for {NodeId} failed", nodeId); } - - return all; } } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs index 9d78961e..272e3695 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs @@ -35,6 +35,12 @@ public sealed class OpcUaClientService : IOpcUaClientService // run at a time. 0 = idle, 1 = failover in progress (Interlocked-managed). private int _failoverInProgress; + // The connection epoch (review I1): ConnectAsync and DisconnectAsync each start a new one (under + // _subscriptionLock). A failover runs under the epoch it started in and abandons itself — disposing any session it + // opened, making no state transition — the moment the epoch moves, so a user Disconnect + Connect during a + // failover is never undone by it. A keep-alive from a previous connection's session cannot start one either. + private long _connectionEpoch; + /// The most recent keep-alive-triggered failover task (test seam: lets a test await it). internal Task? LastFailover { get; private set; } @@ -109,6 +115,9 @@ public sealed class OpcUaClientService : IOpcUaClientService throw new InvalidOperationException( $"Already {_state.ToString().ToLowerInvariant()}; call DisconnectAsync before connecting again."); + long epoch; + lock (_subscriptionLock) epoch = ++_connectionEpoch; + _settings = settings; _allEndpointUrls = FailoverUrlParser.Parse(settings.EndpointUrl, settings.FailoverUrls); _currentEndpointIndex = 0; @@ -118,11 +127,11 @@ public sealed class OpcUaClientService : IOpcUaClientService try { var session = await ConnectToEndpointAsync(settings, _allEndpointUrls[0], ct); - _session = session; + lock (_subscriptionLock) _session = session; session.RegisterKeepAliveHandler(isGood => { - if (!isGood) LastFailover = HandleKeepAliveFailureAsync(); + if (!isGood) OnKeepAliveFailed(epoch); }); CurrentConnectionInfo = BuildConnectionInfo(session); @@ -143,6 +152,9 @@ public sealed class OpcUaClientService : IOpcUaClientService if (_state == ConnectionState.Disconnected) return; + // A new epoch first: a failover still connecting abandons itself instead of resurrecting the connection. + lock (_subscriptionLock) _connectionEpoch++; + var endpointUrl = _session?.EndpointUrl ?? _settings?.EndpointUrl ?? string.Empty; // Snapshot the subscription adapter references under the lock so a concurrent @@ -314,11 +326,18 @@ public sealed class OpcUaClientService : IOpcUaClientService subscription = _dataSubscription; } - if (subscription != null) await subscription.RemoveMonitoredItemAsync(sub.Handle, ct); - - lock (_subscriptionLock) + try { - _activeDataSubscriptions.Remove(nodeIdStr); + if (subscription != null) await subscription.RemoveMonitoredItemAsync(sub.Handle, ct); + } + finally + { + // The caller asked to stop: drop the bookkeeping even when the server-side remove failed (a session + // that died under us), or a later failover would replay the item onto the new session (review m5). + lock (_subscriptionLock) + { + _activeDataSubscriptions.Remove(nodeIdStr); + } } } finally @@ -698,8 +717,12 @@ public sealed class OpcUaClientService : IOpcUaClientService /// public void Dispose() { - if (_disposed) return; - _disposed = true; + lock (_subscriptionLock) + { + if (_disposed) return; + _disposed = true; + _connectionEpoch++; // a failover still running abandons itself + } _dataSubscription?.Dispose(); _alarmSubscription?.Dispose(); @@ -745,19 +768,27 @@ public sealed class OpcUaClientService : IOpcUaClientService ct); } - private async Task HandleKeepAliveFailureAsync() + /// A bad keep-alive from the session of connection . + private void OnKeepAliveFailed(long epoch) { - // Serialize failover: the OPC UA stack raises KeepAlive repeatedly - // while a session is down, so multiple bad keep-alives can fire before - // the first failover loop finishes. CompareExchange atomically claims - // the failover slot; a re-entrant call sees 1 and returns immediately, - // guaranteeing exactly one failover loop runs at a time. + // A late keep-alive from a previous connection's session is not about the current one. + if (!IsCurrentEpoch(epoch)) return; + + // Serialize failover: the OPC UA stack raises KeepAlive repeatedly while a session is down, so several bad + // keep-alives can fire before the first failover loop finishes. CompareExchange claims the failover slot; a + // re-entrant call sees 1 and returns, so exactly one loop runs — and only the winner sets the test seam + // (review m8: a re-entrant call used to overwrite it with an already-completed no-op). if (Interlocked.CompareExchange(ref _failoverInProgress, 1, 0) != 0) return; + LastFailover = HandleKeepAliveFailureAsync(epoch); + } + + private async Task HandleKeepAliveFailureAsync(long epoch) + { try { - await RunFailoverAsync(); + await RunFailoverAsync(epoch); } finally { @@ -765,14 +796,39 @@ public sealed class OpcUaClientService : IOpcUaClientService } } - private async Task RunFailoverAsync() + private bool IsCurrentEpoch(long epoch) + { + lock (_subscriptionLock) return _connectionEpoch == epoch && !_disposed; + } + + /// + /// A state transition made only while is still current (checked and applied under + /// , the event raised outside it). + /// + /// when the epoch has moved on (the caller must abandon). + private bool TransitionStateIfCurrent(long epoch, ConnectionState newState, string endpointUrl) + { + ConnectionState oldState; + lock (_subscriptionLock) + { + if (_connectionEpoch != epoch || _disposed) return false; + oldState = _state; + if (oldState == newState) return true; + _state = newState; + } + + ConnectionStateChanged?.Invoke(this, new ConnectionStateChangedEventArgs(oldState, newState, endpointUrl)); + return true; + } + + private async Task RunFailoverAsync(long epoch) { if (_state == ConnectionState.Reconnecting || _state == ConnectionState.Disconnected) return; var oldSession = _session; var oldEndpoint = oldSession?.EndpointUrl ?? string.Empty; - TransitionState(ConnectionState.Reconnecting, oldEndpoint); + if (!TransitionStateIfCurrent(epoch, ConnectionState.Reconnecting, oldEndpoint)) return; Logger.Warning("Session lost on {EndpointUrl}. Attempting failover...", oldEndpoint); // Dispose the dead session BEFORE taking the gate: an in-flight subscribe holding the gate is blocked on @@ -793,47 +849,60 @@ public sealed class OpcUaClientService : IOpcUaClientService await _subscriptionGate.WaitAsync(); try { + // The user's settings / endpoint list as of THIS epoch: a Connect during the failover replaces the fields. + ConnectionSettings? settings; + string[]? endpoints; lock (_subscriptionLock) { + if (_connectionEpoch != epoch || _disposed) return; // the user disconnected / reconnected meanwhile if (ReferenceEquals(_session, oldSession)) _session = null; _dataSubscription = null; _alarmSubscription = null; + settings = _settings; + endpoints = _allEndpointUrls; } - if (_settings == null || _allEndpointUrls == null) + if (settings == null || endpoints == null) { - TransitionState(ConnectionState.Disconnected, oldEndpoint); + TransitionStateIfCurrent(epoch, ConnectionState.Disconnected, oldEndpoint); return; } // Try each endpoint - for (var attempt = 0; attempt < _allEndpointUrls.Length; attempt++) + for (var attempt = 0; attempt < endpoints.Length; attempt++) { - _currentEndpointIndex = (_currentEndpointIndex + 1) % _allEndpointUrls.Length; - var url = _allEndpointUrls[_currentEndpointIndex]; + if (!IsCurrentEpoch(epoch)) return; // stop trying once the user has moved on (m5) + _currentEndpointIndex = (_currentEndpointIndex + 1) % endpoints.Length; + var url = endpoints[_currentEndpointIndex]; try { Logger.Information("Failover attempt to {EndpointUrl}", url); - var session = await ConnectToEndpointAsync(_settings, url, CancellationToken.None); + var session = await ConnectToEndpointAsync(settings, url, CancellationToken.None); - // A DisconnectAsync / Dispose that ran while this attempt was connecting wins: do not resurrect - // the connection behind the caller's back. - if (_disposed || _state == ConnectionState.Disconnected) + // A Disconnect / Connect / Dispose that ran while this attempt was connecting wins: never + // resurrect the old connection or overwrite the user's new session. Checked and installed + // atomically against the epoch. + bool installed; + lock (_subscriptionLock) + { + installed = _connectionEpoch == epoch && !_disposed; + if (installed) _session = session; + } + + if (!installed) { session.Dispose(); return; } - _session = session; - session.RegisterKeepAliveHandler(isGood => { - if (!isGood) LastFailover = HandleKeepAliveFailureAsync(); + if (!isGood) OnKeepAliveFailed(epoch); }); CurrentConnectionInfo = BuildConnectionInfo(session); - TransitionState(ConnectionState.Connected, url); + if (!TransitionStateIfCurrent(epoch, ConnectionState.Connected, url)) return; Logger.Information("Failover succeeded to {EndpointUrl}", url); // Replay subscriptions @@ -846,8 +915,8 @@ public sealed class OpcUaClientService : IOpcUaClientService } } - Logger.Error("All failover endpoints unreachable"); - TransitionState(ConnectionState.Disconnected, oldEndpoint); + if (TransitionStateIfCurrent(epoch, ConnectionState.Disconnected, oldEndpoint)) + Logger.Error("All failover endpoints unreachable"); } finally { @@ -1023,9 +1092,13 @@ public sealed class OpcUaClientService : IOpcUaClientService private void TransitionState(ConnectionState newState, string endpointUrl) { - var oldState = _state; - if (oldState == newState) return; - _state = newState; + ConnectionState oldState; + lock (_subscriptionLock) + { + oldState = _state; + if (oldState == newState) return; + _state = newState; + } ConnectionStateChanged?.Invoke(this, new ConnectionStateChangedEventArgs(oldState, newState, endpointUrl)); } diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/FailoverEpochTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/FailoverEpochTests.cs new file mode 100644 index 00000000..362bc456 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/FailoverEpochTests.cs @@ -0,0 +1,177 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests; + +/// +/// Lane K review I1: a keep-alive failover belongs to the connection it started under. A user Disconnect + Connect +/// while it is still connecting starts a new connection epoch; the stale failover must abandon itself — never +/// overwrite the user's new session, never flip its state. Every step is gate-driven (no sleeps). +/// +public sealed class FailoverEpochTests : IDisposable +{ + private const string Primary = "opc.tcp://primary:4840"; + private const string Backup = "opc.tcp://backup:4840"; + private static readonly TimeSpan Wait = TimeSpan.FromSeconds(10); + + private readonly FakeSessionFactory _factory = new(); + private readonly OpcUaClientService _service; + private readonly List _states = []; + + public FailoverEpochTests() + { + _service = new OpcUaClientService(new FakeApplicationConfigurationFactory(), new FakeEndpointDiscovery(), _factory); + _service.ConnectionStateChanged += (_, e) => { lock (_states) _states.Add(e.NewState); }; + } + + public void Dispose() => _service.Dispose(); + + private static ConnectionSettings Settings() => new() + { + EndpointUrl = Primary, FailoverUrls = [Backup], SessionTimeoutSeconds = 60 + }; + + /// Connects S1, then holds a keep-alive failover inside its connect attempt. + private async Task<(FakeSessionAdapter S1, Task Failover, TaskCompletionSource Gate)> StartHeldFailoverAsync() + { + var s1 = new FakeSessionAdapter(); + _factory.EnqueueSession(s1); + await _service.ConnectAsync(Settings()); + + var gate = new TaskCompletionSource(); + _factory.CreateGate = gate; + s1.SimulateKeepAlive(false); + var failover = _service.LastFailover.ShouldNotBeNull(); + await _factory.CreateEntered.Task.WaitAsync(Wait); + return (s1, failover, gate); + } + + /// The stale failover's attempt SUCCEEDS after the user reconnected: its session is disposed, the user's stays. + [Fact] + public async Task A_stale_failover_that_connects_does_not_replace_the_users_new_session() + { + var (_, failover, gate) = await StartHeldFailoverAsync(); + var userSession = new FakeSessionAdapter(); + var failoverSession = new FakeSessionAdapter(); + _factory.EnqueueSession(userSession); // dequeued by the user's Connect (the failover is still held) + _factory.EnqueueSession(failoverSession); // dequeued by the failover once released + + await _service.DisconnectAsync(); + await _service.ConnectAsync(Settings()); + lock (_states) _states.Clear(); + + gate.SetResult(); + await failover.WaitAsync(Wait); + + failoverSession.Disposed.ShouldBeTrue(); + userSession.Disposed.ShouldBeFalse(); + _service.IsConnected.ShouldBeTrue(); + _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe(Primary); + lock (_states) _states.ShouldBeEmpty(); // the stale failover made no state transition at all + } + + /// The stale failover's attempts all FAIL after the user reconnected: the user's connection stays Connected. + [Fact] + public async Task A_stale_failover_that_fails_does_not_flip_the_users_connection_to_Disconnected() + { + var (_, failover, gate) = await StartHeldFailoverAsync(); + _factory.EnqueueSession(new FakeSessionAdapter()); + + await _service.DisconnectAsync(); + await _service.ConnectAsync(Settings()); + lock (_states) _states.Clear(); + var createsBefore = _factory.CreateCallCount; + + _factory.ThrowOnCreate = true; + gate.SetResult(); + await failover.WaitAsync(Wait); + + _service.IsConnected.ShouldBeTrue(); + lock (_states) _states.ShouldBeEmpty(); + // It stopped at the abandoned attempt instead of trying every remaining endpoint (review m5). + _factory.CreateCallCount.ShouldBe(createsBefore); + // And the user can still disconnect / reconnect normally afterwards. + _factory.ThrowOnCreate = false; + await _service.DisconnectAsync(); + await _service.ConnectAsync(Settings()); + _service.IsConnected.ShouldBeTrue(); + } + + /// Positive control: an undisturbed failover still reconnects to the backup. + [Fact] + public async Task A_genuine_failover_still_reconnects() + { + var (_, failover, gate) = await StartHeldFailoverAsync(); + _factory.EnqueueSession(new FakeSessionAdapter()); + + gate.SetResult(); + await failover.WaitAsync(Wait); + + _service.IsConnected.ShouldBeTrue(); + _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe(Backup); + lock (_states) _states.ShouldContain(ConnectionState.Reconnecting); + } + + /// + /// A late bad keep-alive from the PREVIOUS connection's session must not start a failover against the new one. + /// Positive control: the same signal from the current session does (the genuine-failover test above). + /// + [Fact] + public async Task A_keep_alive_from_a_previous_connection_does_not_fail_over_the_current_one() + { + var s1 = new FakeSessionAdapter(); + _factory.EnqueueSession(s1); + await _service.ConnectAsync(Settings()); + await _service.DisconnectAsync(); + _factory.EnqueueSession(new FakeSessionAdapter()); + await _service.ConnectAsync(Settings()); + var creates = _factory.CreateCallCount; + lock (_states) _states.Clear(); + + s1.SimulateKeepAlive(false); + if (_service.LastFailover is { } t) await t.WaitAsync(Wait); + + _factory.CreateCallCount.ShouldBe(creates); + lock (_states) _states.ShouldBeEmpty(); + _service.IsConnected.ShouldBeTrue(); + } + + /// Review m8: re-entrant bad keep-alives do not overwrite the seam with an already-completed no-op. + [Fact] + public async Task Reentrant_keep_alives_leave_the_seam_on_the_running_failover() + { + var (s1, failover, gate) = await StartHeldFailoverAsync(); + s1.SimulateKeepAlive(false); + s1.SimulateKeepAlive(false); + + _service.LastFailover.ShouldBeSameAs(failover); + failover.IsCompleted.ShouldBeFalse(); + _factory.EnqueueSession(new FakeSessionAdapter()); + gate.SetResult(); + await failover.WaitAsync(Wait); + } + + /// + /// Review m5: an Unsubscribe whose server-side remove fails (the session died under it) still drops the + /// bookkeeping, so a failover does not replay an item the caller asked to stop. Proven by a re-subscribe of the + /// same node creating a fresh monitored item instead of returning "already subscribed". + /// + [Fact] + public async Task A_failed_unsubscribe_still_forgets_the_item() + { + var sub = new FakeSubscriptionAdapter(); + _factory.EnqueueSession(new FakeSessionAdapter { NextSubscription = sub }); + await _service.ConnectAsync(Settings()); + var node = new Opc.Ua.NodeId("ns=2;s=X"); + await _service.SubscribeAsync(node); + + sub.ThrowOnRemove = true; + await Should.ThrowAsync(() => _service.UnsubscribeAsync(node)); + sub.ThrowOnRemove = false; + + await _service.SubscribeAsync(node); + sub.AddDataChangeCount.ShouldBe(2); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs index c1189a78..24cbd13a 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs @@ -21,6 +21,9 @@ internal sealed class FakeSessionFactory : ISessionFactory /// public TaskCompletionSource? CreateGate { get; set; } + /// Completed when a call observes . + public TaskCompletionSource CreateEntered { get; } = new(TaskCreationOptions.RunContinuationsAsynchronously); + /// Gets the list of sessions created via CreateSessionAsync. public IReadOnlyList CreatedSessions => _createdSessions; @@ -39,8 +42,14 @@ internal sealed class FakeSessionFactory : ISessionFactory CreateCallCount++; LastEndpointUrl = endpoint.EndpointUrl; - if (CreateGate != null) - await CreateGate.Task; + // One-shot: the gate holds only the call that observed it, so a later call (e.g. a user's Connect while a + // failover is held) runs freely. + if (CreateGate is { } gate) + { + CreateGate = null; + CreateEntered.TrySetResult(); + await gate.Task; + } if (ThrowOnCreate) throw new InvalidOperationException("FakeSessionFactory configured to fail."); diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs index 0c7c4233..1e442575 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs @@ -37,6 +37,9 @@ internal sealed class FakeSubscriptionAdapter : ISubscriptionAdapter /// Gets the number of times AddEventMonitoredItemAsync was called. public int AddEventCount { get; private set; } + /// Gets or sets a value indicating whether RemoveMonitoredItemAsync throws (a session that died). + public bool ThrowOnRemove { get; set; } + /// Gets the number of times RemoveMonitoredItemAsync was called. public int RemoveCount { get; private set; } @@ -70,6 +73,8 @@ internal sealed class FakeSubscriptionAdapter : ISubscriptionAdapter /// public Task RemoveMonitoredItemAsync(uint clientHandle, CancellationToken ct) { + if (ThrowOnRemove) + throw new ServiceResultException(StatusCodes.BadSessionClosed, "Session closed"); lock (_itemsLock) { RemoveCount++; diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs index 0f9240b0..bd8c3d1c 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs @@ -92,17 +92,92 @@ public class HistoryReadPagingTests server.Calls.ShouldHaveSingleItem().Release.ShouldBeFalse(); } - /// An uncapped read (maxValues <= 0) follows every continuation point to the end. - [Fact] - public async Task An_uncapped_read_follows_every_page() + /// + /// Review m3: a non-positive cap is refused. An uncapped client read would materialise a tag's whole window + /// (the client side of R08 1.1), and (uint)-1 on the wire asked the server for everything. + /// + /// The cap. + [Theory] + [InlineData(0)] + [InlineData(-1)] + public async Task A_non_positive_cap_is_refused_before_any_read(int max) { - var server = new ScriptedServer((2, [1]), (2, [2]), (1, null)); + var server = new ScriptedServer(); - var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 0, Node, + await Should.ThrowAsync(() => + HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, max, Node, CancellationToken.None)); + server.Calls.ShouldBeEmpty(); + } + + /// Review m2: server pages that overshoot the cap are trimmed to it (60 + 60 with a cap of 100 is 100). + [Fact] + public async Task The_result_is_trimmed_to_the_cap() + { + var server = new ScriptedServer((60, [1]), (60, [2])); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 100, Node, CancellationToken.None); - values.Count.ShouldBe(5); - server.Calls.ShouldAllBe(c => !c.Release); + values.Count.ShouldBe(100); + server.Calls.Last().Release.ShouldBeTrue(); // the outstanding cursor is still released + } + + /// + /// Review m1: a read cancelled between pages releases the outstanding cursor (best effort, not on the caller's + /// cancelled token) and still surfaces the cancellation. Positive control: the release IS recorded. + /// + [Fact] + public async Task A_read_cancelled_mid_paging_releases_the_cursor_and_rethrows() + { + using var cts = new CancellationTokenSource(); + var calls = new List<(byte[]? Cp, bool Release, bool TokenCancelled)>(); + Task Read(byte[]? cp, bool release, CancellationToken ct) + { + calls.Add((cp, release, ct.IsCancellationRequested)); + if (release) return Task.FromResult(new HistoryReadResult { StatusCode = StatusCodes.Good }); + if (cp != null) { cts.Cancel(); ct.ThrowIfCancellationRequested(); } + var data = new HistoryData(); + data.DataValues.Add(new DataValue(new Variant(1))); + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, ContinuationPoint = [5], HistoryData = new ExtensionObject(data) + }); + } + + await Should.ThrowAsync(() => + HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 10, Node, cts.Token)); + + var release = calls.Where(c => c.Release).ShouldHaveSingleItem(); + release.Cp.ShouldBe(new byte[] { 5 }); + release.TokenCancelled.ShouldBeFalse(); + } + + /// A cancellation that lands while the cap release is in flight does not lose the values read. + [Fact] + public async Task The_cap_release_does_not_ride_the_callers_token() + { + using var cts = new CancellationTokenSource(); + var releaseSawCancelled = (bool?)null; + Task Read(byte[]? cp, bool release, CancellationToken ct) + { + if (release) + { + releaseSawCancelled = ct.IsCancellationRequested; + return Task.FromResult(new HistoryReadResult { StatusCode = StatusCodes.Good }); + } + var data = new HistoryData(); + data.DataValues.Add(new DataValue(new Variant(1))); + cts.Cancel(); // the caller gives up right as the capped page arrives + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, ContinuationPoint = [5], HistoryData = new ExtensionObject(data) + }); + } + + var values = await HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 1, Node, cts.Token); + + values.Count.ShouldBe(1); + releaseSawCancelled.ShouldBe(false); } /// A failed release is best effort: the values already read are still returned. From 004c09fa13b4d99dececc7673f897674bf84713c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 17:21:42 -0400 Subject: [PATCH 09/10] fix(ablegacy): an Unsupported the device replied with is an answered refusal, not a local one Review I2: Driver.Ab.Transport raises statuses that came off the wire in an Execute PCCC reply (envelope CIP status, PCCC STS) as AbDeviceReplyException. AbLegacy's IsLocalRefusal now excludes it, and the new IsAnsweredRefusal (used by the read, write, probe and indirect-index paths) counts a wire Unsupported - CIP 0x08/0x1C/0x20 on the envelope, STS 0x10 - as answered: per-tag BadNotSupported, Healthy, LastSuccessfulRead advanced. Before, they landed in the 'refused before the wire' neutral class. Pre-wire refusals are unchanged (still neutral). A socket test covers all four codes (red before); transport tests pin the marker and a pre-wire builder refusal as the positive control. The AbCip Forward Open BadConnection path is untouched. Refs #747 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbDeviceReplyException.cs | 12 ++++++ .../Pccc/PcccServices.cs | 4 +- .../AbLegacyDriver.cs | 28 ++++++++++--- .../Pccc/PcccServicesTests.cs | 18 ++++++++- .../NativePcccBridgeStatusHealthTests.cs | 40 +++++++++++++++++++ 5 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbDeviceReplyException.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbDeviceReplyException.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbDeviceReplyException.cs new file mode 100644 index 00000000..e68d4daf --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbDeviceReplyException.cs @@ -0,0 +1,12 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; + +/// +/// An whose status came OFF THE WIRE: the device (or the bridge's Execute PCCC +/// object) replied with a non-zero CIP envelope status or PCCC STS. It lets a driver tell a refusal the device +/// ANSWERED from the same raised locally before anything was sent — most importantly +/// , which a native runtime also throws for operations it refuses pre-wire +/// (review lane K I2). +/// +/// The status mapped from the reply. +/// The exception message. +public sealed class AbDeviceReplyException(AbStatus status, string message) : AbTransportException(status, message); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs index edd193ee..326dca03 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs @@ -113,7 +113,7 @@ public static class PcccServices public static ReadOnlyMemory ParseReply(CipReply reply, ushort expectedTns, byte[]? expectedRequestorId = null) { if (reply.GeneralStatus != 0) - throw new AbTransportException(reply.Status, $"Execute PCCC CIP status 0x{reply.GeneralStatus:X2}."); + throw new AbDeviceReplyException(reply.Status, $"Execute PCCC CIP status 0x{reply.GeneralStatus:X2}."); var r = new CipReader(reply.Data.Span); var idLen = r.U8(); if (idLen < 1) throw new AbTransportException(AbStatus.BadReply, "Empty requestor id in PCCC reply."); @@ -131,7 +131,7 @@ public static class PcccServices throw new AbTransportException(AbStatus.BadReply, $"PCCC TNS {tns} != {expectedTns}."); byte? ext = sts == 0xF0 ? r.U8() : null; if (sts != 0) - throw new AbTransportException(AbStatusCodes.FromPccc(sts, ext), + throw new AbDeviceReplyException(AbStatusCodes.FromPccc(sts, ext), $"PCCC STS 0x{sts:X2}{(ext is { } e ? $" EXT 0x{e:X2}" : "")}."); return reply.Data[r.Position..]; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 4d7657d7..9fdcddf9 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -789,9 +789,9 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag results[item.Index] = new DataValueSnapshot(null, AbLegacyStatusMapper.BadNotSupported, null, now); return (OpVerdict.Neutral, null); } - catch (AbTransportException ex) when (IsDeviceRefusal(ex.Status)) + catch (AbTransportException ex) when (IsAnsweredRefusal(ex)) { - // The native runtime raises a device's refusal (e.g. STS 0x20 "file not found") as an exception. + // The native runtime raises a device's refusal (e.g. STS 0x50 "addressing problem") as an exception. WarnRefusalOnce(device, def, "read", ex.Message, AbLegacyStatusMapper.MapStatus(ex.Status)); results[item.Index] = new DataValueSnapshot(null, AbLegacyStatusMapper.MapStatus(ex.Status), null, now); return (OpVerdict.Answered, null); @@ -1035,7 +1035,7 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag results[i] = new WriteResult(AbLegacyStatusMapper.BadCommunicationError); outcome.FaultMessage ??= tex.Message; } - catch (AbTransportException ex) when (IsDeviceRefusal(ex.Status)) + catch (AbTransportException ex) when (IsAnsweredRefusal(ex)) { device.RecordAnswered(); outcome.AnyAnswered = true; @@ -1210,7 +1210,7 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag try { probeRuntime?.Dispose(); } catch { } probeRuntime = null; state.ProbeInitialized = false; - if (ex is AbTransportException ate && IsDeviceRefusal(ate.Status)) state.RecordAnswered(); + if (IsAnsweredRefusal(ex)) state.RecordAnswered(); else if (ex is not AbTransportException { Status: var st } || IsNoReply(st)) state.RecordCommsFault(Clock()); } @@ -1478,7 +1478,7 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag { return (null, AbLegacyStatusMapper.BadNotSupported, false, false); } - catch (AbTransportException ex) when (IsDeviceRefusal(ex.Status)) + catch (AbTransportException ex) when (IsAnsweredRefusal(ex)) { // The device answered: the index file/element does not exist. Per-tag, not a comms fault. return (null, AbLegacyStatusMapper.MapStatus(ex.Status), false, false); @@ -1617,10 +1617,26 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag /// all PLC-5 writes), and libplctag's ErrorUnsupported maps to it too. It says nothing about the /// device's reachability, so it must not evict the handle or degrade driver health. /// + /// + /// An Unsupported that came OFF THE WIRE (: CIP 0x08 / 0x1C / 0x20 on the + /// Execute PCCC envelope, PCCC STS 0x10) is NOT local — the device answered — and is classified by + /// instead (review lane K I2). + /// /// The exception to classify. /// if the exception is a local refusal that never reached the wire. internal static bool IsLocalRefusal(Exception ex) => - ex is AbTransportException { Status: AbStatus.Unsupported }; + ex is AbTransportException { Status: AbStatus.Unsupported } and not AbDeviceReplyException; + + /// + /// True for an exception proving the device ANSWERED and refused this operation: a device-refusal status + /// (), or an that came off the wire + /// (). Per-tag Bad, keeps the handle, advances LastSuccessfulRead. + /// + /// The exception to classify. + /// for an answered refusal. + internal static bool IsAnsweredRefusal(Exception ex) => + ex is AbTransportException ate + && (IsDeviceRefusal(ate.Status) || ate is AbDeviceReplyException { Status: AbStatus.Unsupported }); /// /// Initializes a freshly created runtime. A NATIVE runtime whose init (one read) is refused locally is diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs index d8c6edb7..fbac7f56 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs @@ -148,7 +148,23 @@ public sealed class PcccServicesTests [Fact] public void ParseReply_cip_error_status_throws() => - Should.Throw(() => PcccServices.ParseReply(Reply("", 0x08), 1)).Status.ShouldBe(AbStatus.Unsupported); + Should.Throw(() => PcccServices.ParseReply(Reply("", 0x08), 1)).Status.ShouldBe(AbStatus.Unsupported); + + /// Review I2: a status the device REPLIED with is marked as such (envelope CIP status and PCCC STS). + [Fact] + public void ParseReply_marks_device_statuses_as_device_replies() + { + Should.Throw(() => PcccServices.ParseReply(Reply("", 0x20), 1)).Status.ShouldBe(AbStatus.Unsupported); + Should.Throw(() => PcccServices.ParseReply(Reply(Id + "4F10 9987"), 0x8799)).Status.ShouldBe(AbStatus.Unsupported); + } + + /// Positive control: a refusal raised by a builder before anything is sent is NOT a device reply. + [Fact] + public void A_pre_wire_builder_refusal_is_not_a_device_reply() + { + var ex = Should.Throw(() => PcccServices.SlcTypedRead(1, A("N7:0"), 999)); + ex.ShouldNotBeOfType(); + } [Fact] public void ParseReply_wrong_command_byte_is_BadReply() => diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs index 4829c654..58eecee7 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs @@ -20,6 +20,16 @@ public sealed class NativePcccBridgeStatusHealthTests { private static readonly byte[] FixedId = [0x07, 0x3D, 0xF3, 0x45, 0x43, 0x50, 0x21]; + /// A fake whose Execute PCCC envelope itself is refused with CIP general status . + private static FakeEipServer CipEnvelopeServer(byte general) + { + var server = new FakeEipServer { ZeroReplyContext = true }; + server.Handler = req => FakeEipServer.Command(req) == Ab.Transport.Eip.EncapCommand.SendRRData + ? FakeEipServer.RRReply(req, [0xCB, 0x00, general, 0x00]) + : null; + return server; + } + /// A PCCC fake that answers every request with one fixed STS. private static FakeEipServer StsServer(byte sts) { @@ -87,5 +97,35 @@ public sealed class NativePcccBridgeStatusHealthTests await driver.ShutdownAsync(ct); server.HandlerErrors.ShouldBeEmpty(); } + + /// + /// Review I2: a refusal that came OFF THE WIRE — the device (or the bridge's Execute PCCC object) answered with + /// CIP 0x08 / 0x1C / 0x20 on the envelope, or PCCC STS 0x10 — is an answered refusal: per-tag BadNotSupported, + /// Healthy, LastSuccessfulRead advanced. It must not land in the "refused before the wire" neutral class + /// (which advances nothing). The local-refusal counterpart stays neutral (NativeAbLegacyRefusedWriteTests). + /// + /// The CIP general status of the envelope reply, or 0 for a PCCC STS 0x10 reply. + [Theory] + [InlineData((byte)0x08)] + [InlineData((byte)0x1C)] + [InlineData((byte)0x20)] + [InlineData((byte)0x00)] + public async Task A_wire_Unsupported_reply_is_an_answered_refusal(byte general) + { + var ct = TestContext.Current.CancellationToken; + await using var server = general == 0 ? StsServer(0x10) : CipEnvelopeServer(general); + var driver = await DriverAsync(server); + var before = driver.GetHealth().LastSuccessfulRead; + + var result = (await driver.ReadAsync(["T"], ct)).Single(); + + result.StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported); + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Healthy); + health.LastSuccessfulRead.ShouldNotBeNull(); + health.LastSuccessfulRead.ShouldNotBe(before); + await driver.ShutdownAsync(ct); + server.HandlerErrors.ShouldBeEmpty(); + } #pragma warning restore OTOPCUA0001 } From 7e958cd750dba3ebb531a5bf2458f4f2f8dbf034 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 17:28:53 -0400 Subject: [PATCH 10/10] perf(modbus): pooled request ADUs, a reused MBAP header and single-buffer RTU deframing R07:L5 (Modbus half, former F2): ModbusTcpTransport builds the MBAP ADU in a pooled buffer and reads the reply header into one per-transport buffer (sends are single-flight under the gate). ModbusRtuOverTcpTransport writes its ADU into a pooled buffer (ModbusRtuFraming.WriteAdu, byte-identical to BuildAdu), and ReadResponsePduAsync reads and validates the whole frame in one pooled buffer instead of five arrays. Only the returned PDU is allocated. The exchange stays one async method: splitting it cost a state-machine box that ate the saving. Scratch A/B per op: RTU deframe 352 -> 184 B, RTU send 640 -> 408 B, TCP send 864 -> 800 B; timings within noise; Driver.Modbus.Tests 2 s before and after. NormalizeWordOrder lives in ModbusDriver (not a transport) and is left alone. Refs #747 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../ModbusRtuFraming.cs | 118 ++++++++++-------- .../ModbusRtuOverTcpTransport.cs | 15 ++- .../ModbusTcpTransport.cs | 25 +++- .../ModbusPooledFramingTests.cs | 62 +++++++++ 4 files changed, 163 insertions(+), 57 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusPooledFramingTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs index 9fd9c682..bec5055d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs @@ -1,3 +1,4 @@ +using System.Buffers; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; /// @@ -55,12 +56,33 @@ public static class ModbusRtuFraming /// A new array: [unitId, ...pdu, crcLo, crcHi]. public static byte[] BuildAdu(byte unitId, ReadOnlySpan pdu) { - var frame = new byte[1 + pdu.Length]; - frame[0] = unitId; - pdu.CopyTo(frame.AsSpan(1)); - return ModbusCrc.Append(frame); + var frame = new byte[AduLength(pdu.Length)]; + WriteAdu(unitId, pdu, frame); + return frame; } + /// The length of the RTU ADU for a PDU of bytes (unit id + PDU + CRC). + /// The bare PDU length. + /// The ADU length. + internal static int AduLength(int pduLength) => 1 + pduLength + 2; + + /// Writes the RTU ADU into (exactly bytes). + /// The RTU slave/unit address. + /// The bare PDU. + /// The ADU buffer, exactly (.Length) long. + internal static void WriteAdu(byte unitId, ReadOnlySpan pdu, Span destination) + { + destination[0] = unitId; + pdu.CopyTo(destination[1..]); + var body = destination[..(1 + pdu.Length)]; + var crc = ModbusCrc.Compute(body); + destination[1 + pdu.Length] = (byte)(crc & 0xFF); // CRC low byte first + destination[2 + pdu.Length] = (byte)(crc >> 8); + } + + /// The largest RTU response frame: addr + fc + byteCount + 255 data bytes + CRC. + private const int MaxResponseFrame = 3 + 255 + 2; + /// /// Reads a single RTU response frame from and returns its bare PDU /// ([fc, ...data]), with the leading unit-id and trailing CRC stripped. @@ -82,58 +104,56 @@ public static class ModbusRtuFraming public static async Task ReadResponsePduAsync( Stream stream, byte expectedUnit, CancellationToken ct) { - // Header: addr(1) + fc(1). The function code selects how many more bytes to read. - var header = new byte[2]; - await ReadExactlyAsync(stream, header, ct).ConfigureAwait(false); - var fc = header[1]; + // R07:L5: the whole frame is read into ONE pooled buffer and validated in place; only the returned PDU is + // allocated (it used to be five arrays: header, byteCount, rest, the reassembled frame and the PDU). + var frame = ArrayPool.Shared.Rent(MaxResponseFrame); + try + { + // Header: addr(1) + fc(1). The function code selects how many more bytes to read. + await ReadExactlyAsync(stream, frame.AsMemory(0, 2), ct).ConfigureAwait(false); + var fc = frame[1]; - int trailing; // bytes after addr+fc, up to and including the 2 CRC bytes. - if ((fc & 0x80) != 0) - { - // Exception frame: excCode(1) + CRC(2). - trailing = 1 + 2; - } - else if (fc is 0x01 or 0x02 or 0x03 or 0x04 or 0x17) - { - // Read response (and FC23 Read/Write Multiple, whose reply is a read): byteCount(1) then - // byteCount data bytes + CRC(2). - var bc = new byte[1]; - await ReadExactlyAsync(stream, bc, ct).ConfigureAwait(false); - var byteCount = bc[0]; - var rest = new byte[byteCount + 2]; - await ReadExactlyAsync(stream, rest, ct).ConfigureAwait(false); - return ValidateAndStrip(expectedUnit, header, bc, rest); - } - else - { - // Fixed 4-byte echo: correct for the write FCs this driver emits (05/06/0F/10). FC23 is - // variable-length and handled above; any NEW variable-length response FC must be added there - // or it is mis-sized here and surfaces as a desync. - trailing = 4 + 2; - } + int length; // the whole frame, addr through CRC + var already = 2; // bytes of the frame read so far + if ((fc & 0x80) != 0) + { + // Exception frame: excCode(1) + CRC(2). + length = 2 + 1 + 2; + } + else if (fc is 0x01 or 0x02 or 0x03 or 0x04 or 0x17) + { + // Read response (and FC23 Read/Write Multiple, whose reply is a read): byteCount(1) then + // byteCount data bytes + CRC(2). + await ReadExactlyAsync(stream, frame.AsMemory(2, 1), ct).ConfigureAwait(false); + length = 3 + frame[2] + 2; + already = 3; + } + else + { + // Fixed 4-byte echo: correct for the write FCs this driver emits (05/06/0F/10). FC23 is + // variable-length and handled above; any NEW variable-length response FC must be added there + // or it is mis-sized here and surfaces as a desync. + length = 2 + 4 + 2; + } - var tail = new byte[trailing]; - await ReadExactlyAsync(stream, tail, ct).ConfigureAwait(false); - return ValidateAndStrip(expectedUnit, header, ReadOnlySpan.Empty, tail); + await ReadExactlyAsync(stream, frame.AsMemory(already, length - already), ct).ConfigureAwait(false); + return ValidateAndStrip(expectedUnit, frame.AsSpan(0, length)); + } + finally + { + ArrayPool.Shared.Return(frame); + } } /// - /// Assembles the full frame from its pieces, validates the trailing CRC over everything - /// except the CRC bytes, then validates the response address against - /// and strips addr + CRC to return the bare PDU. + /// Validates the trailing CRC over everything except the CRC bytes, then validates the response address + /// against and strips addr + CRC to return the bare PDU. /// A CRC-valid exception PDU throws . /// - private static byte[] ValidateAndStrip( - byte expectedUnit, ReadOnlySpan header, ReadOnlySpan middle, ReadOnlySpan tail) + private static byte[] ValidateAndStrip(byte expectedUnit, ReadOnlySpan frame) { - // Reassemble the whole frame: header(addr,fc) + middle(optional byteCount) + tail. - var frame = new byte[header.Length + middle.Length + tail.Length]; - header.CopyTo(frame); - middle.CopyTo(frame.AsSpan(header.Length)); - tail.CopyTo(frame.AsSpan(header.Length + middle.Length)); - // CRC covers everything except the trailing 2 CRC bytes. - var body = frame.AsSpan(0, frame.Length - 2); + var body = frame[..^2]; var expected = ModbusCrc.Compute(body); var actual = (ushort)(frame[^2] | (frame[^1] << 8)); if (actual != expected) @@ -172,12 +192,12 @@ public static class ModbusRtuFraming /// so a length-less RTU frame that arrives /// short is classified as a desync rather than a bare . /// - private static async Task ReadExactlyAsync(Stream stream, byte[] buf, CancellationToken ct) + private static async Task ReadExactlyAsync(Stream stream, Memory buf, CancellationToken ct) { var read = 0; while (read < buf.Length) { - var n = await stream.ReadAsync(buf.AsMemory(read), ct).ConfigureAwait(false); + var n = await stream.ReadAsync(buf[read..], ct).ConfigureAwait(false); if (n == 0) throw new ModbusTransportDesyncException( ModbusTransportDesyncException.DesyncReason.TruncatedFrame, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs index 7dcc5dff..e2f4047e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs @@ -1,3 +1,4 @@ +using System.Buffers; using System.Net.Sockets; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; @@ -150,14 +151,17 @@ public sealed class ModbusRtuOverTcpTransport : IModbusTransport var stream = CurrentStream; if (stream is null) throw new InvalidOperationException("Transport not connected"); - // RTU ADU: [unitId] + PDU + CRC — no MBAP header, no TxId. - var adu = ModbusRtuFraming.BuildAdu(unitId, pdu); + // RTU ADU: [unitId] + PDU + CRC — no MBAP header, no TxId. Built in a pooled buffer (R07:L5); the same bytes + // ModbusRtuFraming.BuildAdu produces (pinned by a test). + var aduLength = ModbusRtuFraming.AduLength(pdu.Length); + var adu = ArrayPool.Shared.Rent(aduLength); + ModbusRtuFraming.WriteAdu(unitId, pdu, adu.AsSpan(0, aduLength)); using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); cts.CancelAfter(_timeout); try { - await stream.WriteAsync(adu.AsMemory(), cts.Token).ConfigureAwait(false); + await stream.WriteAsync(adu.AsMemory(0, aduLength), cts.Token).ConfigureAwait(false); await stream.FlushAsync(cts.Token).ConfigureAwait(false); // Framing owns the length-less RTU deframe + CRC + unit-address validation. A CRC-valid @@ -181,6 +185,11 @@ public sealed class ModbusRtuOverTcpTransport : IModbusTransport ModbusTransportDesyncException.DesyncReason.Timeout, $"Modbus per-op response timeout after {_timeout.TotalMilliseconds:0}ms"); } + finally + { + // The write has completed, or was cancelled and the stream torn down, so nothing still reads it. + ArrayPool.Shared.Return(adu); + } } /// Asynchronously disposes the transport and underlying socket resources. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs index f21ce678..6889b1e2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs @@ -1,3 +1,4 @@ +using System.Buffers; using System.Net.Sockets; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; @@ -59,6 +60,10 @@ public sealed class ModbusTcpTransport : IModbusTransport /// The socket lifecycle this transport composes (test seam for its clock). internal ModbusSocketLifecycle Lifecycle => _life; + // R07:L5: the 7-byte MBAP header of the reply being read. SendOnceAsync runs only under _gate (single-flight), so + // one buffer per transport serves every transaction (it used to be a fresh array per reply). + private readonly byte[] _header = new byte[7]; + /// public Task ConnectAsync(CancellationToken ct) => _life.ConnectAsync(ct); @@ -139,11 +144,16 @@ public sealed class ModbusTcpTransport : IModbusTransport if (stream is null) throw new InvalidOperationException("Transport not connected"); var txId = ++_nextTx; - // MBAP: [TxId(2)][Proto=0(2)][Length(2)][UnitId(1)] + PDU - var adu = new byte[7 + pdu.Length]; + // MBAP: [TxId(2)][Proto=0(2)][Length(2)][UnitId(1)] + PDU. Built in a pooled buffer (R07:L5): the request + // never escapes, and the write has completed (or been cancelled and the socket torn down) before the finally + // hands it back. Only the reply PDU, which the caller keeps, is allocated. + // (One async method on purpose: a second awaited helper costs a state-machine box per call, which ate the saving.) + var aduLength = 7 + pdu.Length; + var adu = ArrayPool.Shared.Rent(aduLength); adu[0] = (byte)(txId >> 8); adu[1] = (byte)(txId & 0xFF); - // protocol id already zero + adu[2] = 0; // protocol id + adu[3] = 0; var len = (ushort)(1 + pdu.Length); // unit id + pdu adu[4] = (byte)(len >> 8); adu[5] = (byte)(len & 0xFF); @@ -154,10 +164,10 @@ public sealed class ModbusTcpTransport : IModbusTransport cts.CancelAfter(_timeout); try { - await stream.WriteAsync(adu.AsMemory(), cts.Token).ConfigureAwait(false); + await stream.WriteAsync(adu.AsMemory(0, aduLength), cts.Token).ConfigureAwait(false); await stream.FlushAsync(cts.Token).ConfigureAwait(false); - var header = new byte[7]; + var header = _header; await ReadExactlyAsync(stream, header, cts.Token).ConfigureAwait(false); var respTxId = (ushort)((header[0] << 8) | header[1]); if (respTxId != txId) @@ -211,6 +221,11 @@ public sealed class ModbusTcpTransport : IModbusTransport ModbusTransportDesyncException.DesyncReason.Timeout, $"Modbus per-op response timeout after {_timeout.TotalMilliseconds:0}ms"); } + finally + { + // The write has completed, or was cancelled and the socket torn down, so nothing still reads it. + ArrayPool.Shared.Return(adu); + } } private static async Task ReadExactlyAsync(Stream s, byte[] buf, CancellationToken ct) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusPooledFramingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusPooledFramingTests.cs new file mode 100644 index 00000000..6cf0450b --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusPooledFramingTests.cs @@ -0,0 +1,62 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// R07:L5 (Modbus half): the RTU ADU is now written into a pooled buffer and the response is deframed in one pooled +/// buffer. The wire bytes and the decoded PDUs must be exactly what the allocating code produced, and a reused pooled +/// buffer must never leak a previous frame's bytes into the next result. +/// +[Trait("Category", "Unit")] +public sealed class ModbusPooledFramingTests +{ + /// The pooled ADU writer matches the reference construction (unit + PDU, CRC appended low byte first). + /// The PDU as hex. + [Theory] + [InlineData("0300000001")] + [InlineData("10000A0002040001000200")] + [InlineData("06")] + public void WriteAdu_matches_the_reference_ADU(string hex) + { + var pdu = Convert.FromHexString(hex); + var reference = ModbusCrc.Append([0x11, .. pdu]); + + var pooled = new byte[ModbusRtuFraming.AduLength(pdu.Length) + 7]; // oversized, like a rented array + ModbusRtuFraming.WriteAdu(0x11, pdu, pooled.AsSpan(0, ModbusRtuFraming.AduLength(pdu.Length))); + + pooled.AsSpan(0, reference.Length).ToArray().ShouldBe(reference); + ModbusRtuFraming.BuildAdu(0x11, pdu).ShouldBe(reference); + } + + /// + /// A long read reply followed by a short echo and an exception frame, each deframed through the pooled buffer: + /// every result is exactly its own PDU (no stale tail from the previous, longer frame). + /// + [Fact] + public async Task Consecutive_deframes_through_the_pooled_buffer_do_not_leak_bytes() + { + var ct = TestContext.Current.CancellationToken; + byte[] longRead = [0x03, 200, .. Enumerable.Range(0, 200).Select(i => (byte)(i + 1))]; + byte[] echo = [0x06, 0x00, 0x01, 0x00, 0x2A]; + + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, longRead)), 1, ct)) + .ShouldBe(longRead); + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, echo)), 1, ct)) + .ShouldBe(echo); + var ex = await Should.ThrowAsync(() => + ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, [0x83, 0x02])), 1, ct)); + ex.ExceptionCode.ShouldBe((byte)0x02); + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, [0x03, 2, 9, 8])), 1, ct)) + .ShouldBe(new byte[] { 0x03, 2, 9, 8 }); + } + + /// A byte-count-255 read (the largest RTU reply) fits the pooled frame. + [Fact] + public async Task The_largest_read_reply_deframes() + { + byte[] max = [0x04, 255, .. new byte[255]]; + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, max)), 1, + TestContext.Current.CancellationToken)).ShouldBe(max); + } +}