diff --git a/codereviews/review.md b/codereviews/review.md index 7a97e39..c281922 100644 --- a/codereviews/review.md +++ b/codereviews/review.md @@ -1,41 +1,75 @@ -# Full Review: `src` +# Full Review Update: `src` -Overall verdict: **patch is incorrect** +Overall verdict: **patch is still incorrect** -I reviewed the current implementation under `src/` as a code review, not just the Git diff. The main issues are correctness bugs in the OPC UA subscription path and MXAccess recovery/write handling. +This pass verified the previous findings against the current implementation, then performed another full review of `src/` with emphasis on service-mode reliability, continuous OPC UA publishing, and recovery from MXAccess disconnects. -## Findings +## Verified Completed -### [P1] OPC UA monitored items never trigger MXAccess subscriptions -File: `src/ZB.MOM.WW.LmxOpcUa.Host/OpcUa/LmxNodeManager.cs:327-345` +### [DONE] OPC UA monitored items now start MXAccess subscriptions +Verified in `src/ZB.MOM.WW.LmxOpcUa.Host/OpcUa/LmxNodeManager.cs:355-423`. -`LmxNodeManager` has a `SubscribeTag` helper, but nothing in the node manager calls it when OPC UA clients create monitored items. There is also no monitored-item lifecycle override in this class. The result is that browsing or subscribing from an OPC UA client does not call `_mxAccessClient.SubscribeAsync(...)`, so live MXAccess data changes are never started for those tags. Clients can still do synchronous reads, but any client expecting pushed value updates will see stale `BadWaitingForInitialData` or last-cached values instead of runtime updates. +The node manager now hooks monitored-item create/delete callbacks and translates them into ref-counted `SubscribeTag` / `UnsubscribeTag` calls. This closes the original gap where OPC UA subscriptions never triggered runtime MX subscriptions. -### [P1] Write timeouts are reported as successful writes -File: `src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.ReadWrite.cs:101-105` +Validation: +- `MultiClientTests.MultipleClients_SubscribeToSameTag_AllReceiveDataChanges` passed -On write timeout, the cancellation callback completes the pending task with `true`. That means a missing `OnWriteComplete` callback is treated as a successful write, and `LmxNodeManager.Write(...)` will return `ServiceResult.Good` upstream. In any timeout scenario where MXAccess never acknowledged the write, OPC UA clients will be told the write succeeded even though the runtime value may never have changed. +### [DONE] Write timeout no longer reports success +Verified in `src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.ReadWrite.cs:101-107`. -### [P1] Auto-reconnect stops permanently after one failed reconnect attempt +The timeout path now completes the write with `false` instead of `true`, so OPC UA writes are no longer acknowledged when MXAccess never confirms them. + +### [DONE] Monitor retries from `Error` state as well as `Disconnected` +Verified in `src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.Monitor.cs:38-43`. + +The reconnect loop now treats `ConnectionState.Error` as reconnectable, which closes the prior issue where one failed reconnect left the client permanently stuck. + +Validation: +- `MxAccessClientMonitorTests.Monitor_ReconnectsOnDisconnect` passed +- `MxAccessClientMonitorTests.Monitor_ProbeStale_ForcesReconnect` passed + +### [DONE] Address-space construction no longer depends on input ordering +Verified in `src/ZB.MOM.WW.LmxOpcUa.Host/OpcUa/LmxNodeManager.cs:75-77` and `:174-199`. + +The added topological sort ensures parents are materialized before children even when the hierarchy input is unsorted. + +### [DONE] Startup no longer forces an immediate redundant rebuild +Verified in `src/ZB.MOM.WW.LmxOpcUa.Host/OpcUaService.cs:143-184` and `src/ZB.MOM.WW.LmxOpcUa.Host/GalaxyRepository/ChangeDetectionService.cs:25-30`. + +The initial deploy time is now captured during startup and passed into `ChangeDetectionService`, preventing the unconditional first-poll rebuild when startup already loaded the same deploy. + +Validation: +- `ChangeDetectionServiceTests` passed + +## New Findings + +### [P1] Service never recovers if MXAccess is unavailable during startup Files: -- `src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.Connection.cs:101-110` -- `src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.Monitor.cs:38-45` +- `src/ZB.MOM.WW.LmxOpcUa.Host/OpcUaService.cs:107-123` +- `src/ZB.MOM.WW.LmxOpcUa.Host/OpcUaService.cs:139-140` -The monitor only retries when `_state == ConnectionState.Disconnected`, but both `ConnectAsync` and `ReconnectAsync` move the client to `ConnectionState.Error` on a failed attempt. After the first reconnect failure, the monitor loop no longer matches its reconnect condition, so temporary outages become permanent until the whole service is restarted. +If the initial `MxAccessClient.ConnectAsync()` fails, startup disposes the real client and STA thread, replaces them with `NullMxAccessClient`, and still starts the OPC UA server. After that point there is no code path that ever retries the real MXAccess connection, so a temporary LMX outage at service startup becomes a permanent loss of runtime publishing until the whole Windows service is restarted. -### [P2] Address-space construction depends on parent-before-child ordering -File: `src/ZB.MOM.WW.LmxOpcUa.Host/OpcUa/LmxNodeManager.cs:98-104` - -`BuildAddressSpace` attaches each object to `rootFolder` whenever its parent is not already in `nodeMap`. That only works if the input hierarchy is topologically ordered with every parent appearing before every descendant. The method does not enforce that ordering itself, so any unsorted hierarchy list will silently build the wrong OPC UA tree by promoting children to the root level. - -### [P3] Startup always performs an unnecessary second rebuild +### [P1] Address-space rebuild loses live subscription bookkeeping Files: -- `src/ZB.MOM.WW.LmxOpcUa.Host/OpcUaService.cs:142-182` -- `src/ZB.MOM.WW.LmxOpcUa.Host/GalaxyRepository/ChangeDetectionService.cs:46-60` +- `src/ZB.MOM.WW.LmxOpcUa.Host/OpcUa/LmxNodeManager.cs:163-169` +- `src/ZB.MOM.WW.LmxOpcUa.Host/OpcUa/LmxNodeManager.cs:390-423` -`OpcUaService.Start()` already reads the Galaxy hierarchy/attributes and builds the initial address space before change detection starts. `ChangeDetectionService` then unconditionally fires `OnGalaxyChanged` on its first poll, even when the deploy timestamp has not changed, causing an immediate second full DB fetch and address-space rebuild on every startup. That doubles startup load and rebuild latency for no functional gain. +`RebuildAddressSpace()` clears `_subscriptionRefCounts` without unsubscribing the existing MXAccess subscriptions or rebuilding the ref counts for still-active monitored items. If a Galaxy rebuild happens while clients are subscribed, later deletes no longer call `_mxAccessClient.UnsubscribeAsync(...)`, and any future subscription to the same tag starts from zero and creates another runtime subscription. In a long-running service this leaks live subscriptions and can duplicate data-change delivery after successive rebuilds. -## Notes +### [P2] Failed reads and writes are still recorded as successful operations +Files: +- `src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.ReadWrite.cs:36-42` +- `src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.ReadWrite.cs:97-109` +- `src/ZB.MOM.WW.LmxOpcUa.Host/Status/HealthCheckService.cs:24-38` -- This was a static review of the `src` tree. -- I attempted `dotnet test ZB.MOM.WW.LmxOpcUa.slnx`, but the run timed out after about 124 seconds, so I did not rely on a full green test pass for validation. +`PerformanceMetrics` only gets marked unsuccessful when an exception is thrown. A read timeout returns `Vtq.Bad(...)` and a rejected or timed-out write returns `false`, but neither path calls `scope.SetSuccess(false)`. Since the dashboard health logic uses those success rates to detect degraded behavior, sustained runtime failures can still leave the service reporting healthy even while reads or writes are failing. + +## Test Notes + +Focused verification completed successfully: +- `dotnet test tests/ZB.MOM.WW.LmxOpcUa.Tests/ZB.MOM.WW.LmxOpcUa.Tests.csproj --filter "FullyQualifiedName~MultiClientTests.MultipleClients_SubscribeToSameTag_AllReceiveDataChanges"` +- `dotnet test tests/ZB.MOM.WW.LmxOpcUa.Tests/ZB.MOM.WW.LmxOpcUa.Tests.csproj --filter "FullyQualifiedName~MxAccessClientMonitorTests.Monitor_ReconnectsOnDisconnect|FullyQualifiedName~MxAccessClientMonitorTests.Monitor_ProbeStale_ForcesReconnect"` +- `dotnet test tests/ZB.MOM.WW.LmxOpcUa.Tests/ZB.MOM.WW.LmxOpcUa.Tests.csproj --filter "FullyQualifiedName~ChangeDetectionServiceTests"` + +I did not rerun the entire solution test suite in this pass. A previous full-solution run had timed out, so verification here was focused on the behavior touched by the prior fixes and the service-reliability paths reviewed above. diff --git a/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.Connection.cs b/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.Connection.cs index 85e9f06..1e0f68e 100644 --- a/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.Connection.cs +++ b/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.Connection.cs @@ -18,8 +18,7 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess { _connectionHandle = await _staThread.RunAsync(() => { - _proxy.OnDataChange += HandleOnDataChange; - _proxy.OnWriteComplete += HandleOnWriteComplete; + AttachProxyEvents(); return _proxy.Register(_config.ClientName); }); @@ -34,12 +33,21 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess { _probeTag = _config.ProbeTag; _lastProbeValueTime = DateTime.UtcNow; - await SubscribeInternalAsync(_probeTag); + await SubscribeInternalAsync(_probeTag!); Log.Information("Probe tag subscribed: {ProbeTag}", _probeTag); } } catch (Exception ex) { + try + { + await _staThread.RunAsync(DetachProxyEvents); + } + catch (Exception cleanupEx) + { + Log.Warning(cleanupEx, "Failed to detach proxy events after connection failure"); + } + Log.Error(ex, "MxAccess connection failed"); SetState(ConnectionState.Error, ex.Message); throw; @@ -70,8 +78,7 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess } // Unwire events before unregister - _proxy.OnDataChange -= HandleOnDataChange; - _proxy.OnWriteComplete -= HandleOnWriteComplete; + DetachProxyEvents(); // Unregister try { _proxy.Unregister(_connectionHandle); } @@ -80,6 +87,7 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess _handleToAddress.Clear(); _addressToHandle.Clear(); + _pendingReadsByAddress.Clear(); _pendingWrites.Clear(); } catch (Exception ex) @@ -109,5 +117,21 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess SetState(ConnectionState.Error, ex.Message); } } + + private void AttachProxyEvents() + { + if (_proxyEventsAttached) return; + _proxy.OnDataChange += HandleOnDataChange; + _proxy.OnWriteComplete += HandleOnWriteComplete; + _proxyEventsAttached = true; + } + + private void DetachProxyEvents() + { + if (!_proxyEventsAttached) return; + _proxy.OnDataChange -= HandleOnDataChange; + _proxy.OnWriteComplete -= HandleOnWriteComplete; + _proxyEventsAttached = false; + } } } diff --git a/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.EventHandlers.cs b/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.EventHandlers.cs index e994b31..eec170e 100644 --- a/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.EventHandlers.cs +++ b/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.EventHandlers.cs @@ -50,6 +50,12 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess callback(address, vtq); } + if (_pendingReadsByAddress.TryGetValue(address, out var pendingReads)) + { + foreach (var pendingRead in pendingReads.Values) + pendingRead.TrySetResult(vtq); + } + // Global handler OnTagValueChanged?.Invoke(address, vtq); } diff --git a/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.ReadWrite.cs b/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.ReadWrite.cs index 96821b7..b432311 100644 --- a/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.ReadWrite.cs +++ b/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.ReadWrite.cs @@ -19,9 +19,6 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess using var scope = _metrics.BeginOperation("Read"); var tcs = new TaskCompletionSource(); - // Subscribe, get first value, unsubscribe - void OnValue(string addr, Vtq vtq) => tcs.TrySetResult(vtq); - var itemHandle = await _staThread.RunAsync(() => { var h = _proxy.AddItem(_connectionHandle, fullTagReference); @@ -29,9 +26,10 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess return h; }); + var pendingReads = _pendingReadsByAddress.GetOrAdd(fullTagReference, + _ => new System.Collections.Concurrent.ConcurrentDictionary>()); + pendingReads[itemHandle] = tcs; _handleToAddress[itemHandle] = fullTagReference; - _addressToHandle[fullTagReference] = itemHandle; - _storedSubscriptions[fullTagReference] = OnValue; try { @@ -39,7 +37,11 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess cts.CancelAfter(TimeSpan.FromSeconds(_config.ReadTimeoutSeconds)); cts.Token.Register(() => tcs.TrySetResult(Vtq.Bad(Quality.BadCommFailure))); - return await tcs.Task; + var result = await tcs.Task; + if (result.Quality != Quality.Good) + scope.SetSuccess(false); + + return result; } catch { @@ -48,9 +50,14 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess } finally { - _storedSubscriptions.TryRemove(fullTagReference, out _); + if (_pendingReadsByAddress.TryGetValue(fullTagReference, out var reads)) + { + reads.TryRemove(itemHandle, out _); + if (reads.IsEmpty) + _pendingReadsByAddress.TryRemove(fullTagReference, out _); + } + _handleToAddress.TryRemove(itemHandle, out _); - _addressToHandle.TryRemove(fullTagReference, out _); try { @@ -89,7 +96,6 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess }); _handleToAddress[itemHandle] = fullTagReference; - _addressToHandle[fullTagReference] = itemHandle; var tcs = new TaskCompletionSource(); _pendingWrites[itemHandle] = tcs; @@ -106,7 +112,11 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess tcs.TrySetResult(false); }); - return await tcs.Task; + var success = await tcs.Task; + if (!success) + scope.SetSuccess(false); + + return success; } catch (Exception ex) { @@ -118,7 +128,6 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess { _pendingWrites.TryRemove(itemHandle, out _); _handleToAddress.TryRemove(itemHandle, out _); - _addressToHandle.TryRemove(fullTagReference, out _); try { diff --git a/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.cs b/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.cs index 4bcc5fe..7ecaea0 100644 --- a/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.cs +++ b/src/ZB.MOM.WW.LmxOpcUa.Host/MxAccess/MxAccessClient.cs @@ -27,6 +27,7 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess private int _connectionHandle; private volatile ConnectionState _state = ConnectionState.Disconnected; + private bool _proxyEventsAttached; private CancellationTokenSource? _monitorCts; // Handle mappings @@ -40,6 +41,8 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.MxAccess // Pending writes private readonly ConcurrentDictionary> _pendingWrites = new ConcurrentDictionary>(); + private readonly ConcurrentDictionary>> _pendingReadsByAddress + = new ConcurrentDictionary>>(StringComparer.OrdinalIgnoreCase); // Probe private string? _probeTag; diff --git a/src/ZB.MOM.WW.LmxOpcUa.Host/OpcUa/LmxNodeManager.cs b/src/ZB.MOM.WW.LmxOpcUa.Host/OpcUa/LmxNodeManager.cs index 063f2c9..6851aec 100644 --- a/src/ZB.MOM.WW.LmxOpcUa.Host/OpcUa/LmxNodeManager.cs +++ b/src/ZB.MOM.WW.LmxOpcUa.Host/OpcUa/LmxNodeManager.cs @@ -145,15 +145,21 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.OpcUa lock (Lock) { Log.Information("Rebuilding address space..."); + var activeSubscriptions = new Dictionary(_subscriptionRefCounts, StringComparer.OrdinalIgnoreCase); - // Remove all predefined nodes - var nodesToRemove = new List(); - foreach (var kvp in _nodeIdToTagReference) + foreach (var tagRef in activeSubscriptions.Keys) { - var nodeId = new NodeId(kvp.Key, NamespaceIndex); - nodesToRemove.Add(nodeId); + try + { + _mxAccessClient.UnsubscribeAsync(tagRef).GetAwaiter().GetResult(); + } + catch (Exception ex) + { + Log.Warning(ex, "Failed to unsubscribe {TagRef} during rebuild", tagRef); + } } + // Remove all predefined nodes foreach (var nodeId in PredefinedNodes.Keys.ToList()) { try { DeleteNode(SystemContext, nodeId); } @@ -167,6 +173,23 @@ namespace ZB.MOM.WW.LmxOpcUa.Host.OpcUa // Rebuild BuildAddressSpace(hierarchy, attributes); + + foreach (var kvp in activeSubscriptions) + { + if (!_tagToVariableNode.ContainsKey(kvp.Key)) + continue; + + try + { + _mxAccessClient.SubscribeAsync(kvp.Key, (_, _) => { }).GetAwaiter().GetResult(); + _subscriptionRefCounts[kvp.Key] = kvp.Value; + } + catch (Exception ex) + { + Log.Warning(ex, "Failed to restore subscription for {TagRef} after rebuild", kvp.Key); + } + } + Log.Information("Address space rebuild complete"); } } diff --git a/src/ZB.MOM.WW.LmxOpcUa.Host/OpcUaService.cs b/src/ZB.MOM.WW.LmxOpcUa.Host/OpcUaService.cs index 66cd414..589c551 100644 --- a/src/ZB.MOM.WW.LmxOpcUa.Host/OpcUaService.cs +++ b/src/ZB.MOM.WW.LmxOpcUa.Host/OpcUaService.cs @@ -109,14 +109,21 @@ namespace ZB.MOM.WW.LmxOpcUa.Host _staThread = new StaComThread(); _staThread.Start(); _mxAccessClient = new MxAccessClient(_staThread, _mxProxy, _config.MxAccess, _metrics); - _mxAccessClient.ConnectAsync(_cts.Token).GetAwaiter().GetResult(); + try + { + _mxAccessClient.ConnectAsync(_cts.Token).GetAwaiter().GetResult(); + } + catch (Exception ex) + { + Log.Warning(ex, "MxAccess connection failed at startup - monitor will continue retrying in the background"); + } - // Step 6: Start monitor loop + // Step 6: Start monitor loop even if initial connect failed _mxAccessClient.StartMonitor(); } catch (Exception ex) { - Log.Warning(ex, "MxAccess connection failed — continuing without runtime data access"); + Log.Warning(ex, "MxAccess initialization failed - continuing without runtime data access"); _mxAccessClient?.Dispose(); _mxAccessClient = null; _staThread?.Dispose(); diff --git a/tests/ZB.MOM.WW.LmxOpcUa.Tests/Helpers/FakeMxProxy.cs b/tests/ZB.MOM.WW.LmxOpcUa.Tests/Helpers/FakeMxProxy.cs index aa08758..c5a8ade 100644 --- a/tests/ZB.MOM.WW.LmxOpcUa.Tests/Helpers/FakeMxProxy.cs +++ b/tests/ZB.MOM.WW.LmxOpcUa.Tests/Helpers/FakeMxProxy.cs @@ -29,6 +29,7 @@ namespace ZB.MOM.WW.LmxOpcUa.Tests.Helpers public int UnregisterCallCount { get; private set; } public bool ShouldFailRegister { get; set; } public bool ShouldFailWrite { get; set; } + public bool SkipWriteCompleteCallback { get; set; } public int WriteCompleteStatus { get; set; } = 0; // 0 = success public int Register(string clientName) @@ -87,7 +88,8 @@ namespace ZB.MOM.WW.LmxOpcUa.Tests.Helpers status[0].success = 0; status[0].detail = (short)WriteCompleteStatus; } - OnWriteComplete?.Invoke(_connectionHandle, itemHandle, ref status); + if (!SkipWriteCompleteCallback) + OnWriteComplete?.Invoke(_connectionHandle, itemHandle, ref status); } /// diff --git a/tests/ZB.MOM.WW.LmxOpcUa.Tests/Integration/AddressSpaceRebuildTests.cs b/tests/ZB.MOM.WW.LmxOpcUa.Tests/Integration/AddressSpaceRebuildTests.cs index 9609b82..fbe8815 100644 --- a/tests/ZB.MOM.WW.LmxOpcUa.Tests/Integration/AddressSpaceRebuildTests.cs +++ b/tests/ZB.MOM.WW.LmxOpcUa.Tests/Integration/AddressSpaceRebuildTests.cs @@ -259,5 +259,32 @@ namespace ZB.MOM.WW.LmxOpcUa.Tests.Integration await fixture.DisposeAsync(); } } + + [Fact] + public async Task Rebuild_PreservesSubscriptionBookkeeping_ForSurvivingNodes() + { + var fixture = OpcUaServerFixture.WithFakeMxAccessClient(); + await fixture.InitializeAsync(); + try + { + var nodeManager = fixture.Service.NodeManagerInstance!; + var mxClient = fixture.MxAccessClient!; + + nodeManager.SubscribeTag("TestMachine_001.MachineID"); + mxClient.ActiveSubscriptionCount.ShouldBe(1); + + fixture.Service.TriggerRebuild(); + await Task.Delay(200); + + mxClient.ActiveSubscriptionCount.ShouldBe(1); + + nodeManager.UnsubscribeTag("TestMachine_001.MachineID"); + mxClient.ActiveSubscriptionCount.ShouldBe(0); + } + finally + { + await fixture.DisposeAsync(); + } + } } } diff --git a/tests/ZB.MOM.WW.LmxOpcUa.Tests/MxAccess/MxAccessClientReadWriteTests.cs b/tests/ZB.MOM.WW.LmxOpcUa.Tests/MxAccess/MxAccessClientReadWriteTests.cs index 5e894ad..c4193d2 100644 --- a/tests/ZB.MOM.WW.LmxOpcUa.Tests/MxAccess/MxAccessClientReadWriteTests.cs +++ b/tests/ZB.MOM.WW.LmxOpcUa.Tests/MxAccess/MxAccessClientReadWriteTests.cs @@ -69,6 +69,20 @@ namespace ZB.MOM.WW.LmxOpcUa.Tests.MxAccess result.Quality.ShouldBe(Quality.BadCommFailure); } + [Fact] + public async Task Read_Timeout_RecordsFailedMetrics() + { + await _client.ConnectAsync(); + + var result = await _client.ReadAsync("TestTag.Attr"); + result.Quality.ShouldBe(Quality.BadCommFailure); + + var stats = _metrics.GetStatistics(); + stats.ShouldContainKey("Read"); + stats["Read"].TotalCount.ShouldBe(1); + stats["Read"].SuccessCount.ShouldBe(0); + } + [Fact] public async Task Write_NotConnected_ReturnsFalse() { @@ -97,6 +111,21 @@ namespace ZB.MOM.WW.LmxOpcUa.Tests.MxAccess result.ShouldBe(false); } + [Fact] + public async Task Write_Timeout_ReturnsFalse_AndRecordsFailedMetrics() + { + await _client.ConnectAsync(); + _proxy.SkipWriteCompleteCallback = true; + + var result = await _client.WriteAsync("TestTag.Attr", 42); + result.ShouldBe(false); + + var stats = _metrics.GetStatistics(); + stats.ShouldContainKey("Write"); + stats["Write"].TotalCount.ShouldBe(1); + stats["Write"].SuccessCount.ShouldBe(0); + } + [Fact] public async Task Read_RecordsMetrics() { diff --git a/tests/ZB.MOM.WW.LmxOpcUa.Tests/MxAccess/MxAccessClientSubscriptionTests.cs b/tests/ZB.MOM.WW.LmxOpcUa.Tests/MxAccess/MxAccessClientSubscriptionTests.cs index d5e5730..5c0c142 100644 --- a/tests/ZB.MOM.WW.LmxOpcUa.Tests/MxAccess/MxAccessClientSubscriptionTests.cs +++ b/tests/ZB.MOM.WW.LmxOpcUa.Tests/MxAccess/MxAccessClientSubscriptionTests.cs @@ -102,6 +102,42 @@ namespace ZB.MOM.WW.LmxOpcUa.Tests.MxAccess callbackInvoked.ShouldBe(true); } + [Fact] + public async Task OneShotRead_DoesNotRemovePersistentSubscription_OnReconnect() + { + await _client.ConnectAsync(); + var callbackInvoked = false; + await _client.SubscribeAsync("TestTag.Attr", (_, _) => callbackInvoked = true); + + var readTask = _client.ReadAsync("TestTag.Attr"); + await Task.Delay(50); + _proxy.SimulateDataChangeByAddress("TestTag.Attr", 42, 192); + (await readTask).Value.ShouldBe(42); + callbackInvoked = false; + + await _client.ReconnectAsync(); + + _proxy.SimulateDataChangeByAddress("TestTag.Attr", "after_reconnect", 192); + callbackInvoked.ShouldBe(true); + _client.ActiveSubscriptionCount.ShouldBe(1); + } + + [Fact] + public async Task OneShotWrite_DoesNotBreakPersistentUnsubscribe() + { + await _client.ConnectAsync(); + await _client.SubscribeAsync("TestTag.Attr", (_, _) => { }); + _proxy.Items.Values.ShouldContain("TestTag.Attr"); + + var writeResult = await _client.WriteAsync("TestTag.Attr", 7); + writeResult.ShouldBe(true); + + await _client.UnsubscribeAsync("TestTag.Attr"); + + _client.ActiveSubscriptionCount.ShouldBe(0); + _proxy.Items.Values.ShouldNotContain("TestTag.Attr"); + } + [Fact] public async Task ProbeTag_SubscribedOnConnect() { diff --git a/tests/ZB.MOM.WW.LmxOpcUa.Tests/Wiring/ServiceStartupSequenceTest.cs b/tests/ZB.MOM.WW.LmxOpcUa.Tests/Wiring/ServiceStartupSequenceTest.cs index 12ac6a9..6d954ae 100644 --- a/tests/ZB.MOM.WW.LmxOpcUa.Tests/Wiring/ServiceStartupSequenceTest.cs +++ b/tests/ZB.MOM.WW.LmxOpcUa.Tests/Wiring/ServiceStartupSequenceTest.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Threading.Tasks; using Shouldly; using Xunit; using ZB.MOM.WW.LmxOpcUa.Host; @@ -69,5 +70,60 @@ namespace ZB.MOM.WW.LmxOpcUa.Tests.Wiring service.Stop(); } } + + [Fact] + public async Task Start_WhenMxAccessIsInitiallyDown_MonitorReconnectsInBackground() + { + var config = new AppConfiguration + { + OpcUa = new OpcUaConfiguration + { + Port = 14841, + GalaxyName = "TestGalaxy", + EndpointPath = "/LmxOpcUa" + }, + MxAccess = new MxAccessConfiguration + { + ClientName = "Test", + MonitorIntervalSeconds = 1, + AutoReconnect = true + }, + GalaxyRepository = new GalaxyRepositoryConfiguration(), + Dashboard = new DashboardConfiguration { Enabled = false } + }; + + var proxy = new FakeMxProxy { ShouldFailRegister = true }; + var repo = new FakeGalaxyRepository + { + Hierarchy = new List + { + new GalaxyObjectInfo { GobjectId = 1, TagName = "TestObj", BrowseName = "TestObj", ParentGobjectId = 0, IsArea = false } + }, + Attributes = new List + { + new GalaxyAttributeInfo { GobjectId = 1, TagName = "TestObj", AttributeName = "TestAttr", FullTagReference = "TestObj.TestAttr", MxDataType = 5, IsArray = false } + } + }; + + var service = new OpcUaService(config, proxy, repo); + service.Start(); + + try + { + service.ServerHost.ShouldNotBeNull(); + service.MxClient.ShouldNotBeNull(); + service.MxClient!.State.ShouldBe(ConnectionState.Error); + + proxy.ShouldFailRegister = false; + await Task.Delay(2500); + + service.MxClient.State.ShouldBe(ConnectionState.Connected); + proxy.RegisterCallCount.ShouldBeGreaterThan(1); + } + finally + { + service.Stop(); + } + } } }