From 6cec98caefecea93114f062e23121dbdb515a38c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 08:24:08 -0400 Subject: [PATCH] fix(core): resolve Medium code-review finding (Core-006) BuildAddressSpaceAsync now checks _disposed (throws ObjectDisposedException) and tears down the previous alarm forwarder + clears the sink registry before re-walking, so a Galaxy-redeploy rebuild does not leak the old forwarder and double-deliver alarm transitions. Three regression tests added: double-build does not double-fire, sink count is correct after rebuild, and post-dispose call throws. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../OpcUa/GenericDriverNodeManager.cs | 17 +++++- .../GenericDriverNodeManagerTests.cs | 55 +++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/OpcUa/GenericDriverNodeManager.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/OpcUa/GenericDriverNodeManager.cs index 8712107..56fd429 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/OpcUa/GenericDriverNodeManager.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/OpcUa/GenericDriverNodeManager.cs @@ -36,12 +36,25 @@ public class GenericDriverNodeManager(IDriver driver) : IDisposable /// Populates the address space by streaming nodes from the driver into the supplied builder, /// wraps the builder so alarm-condition sinks are captured, subscribes to the driver's /// alarm event stream, and routes each transition to the matching sink by SourceNodeId. - /// Driver exceptions are isolated per decision #12 — the driver's subtree is marked Faulted, - /// but other drivers remain available. + /// If called a second time (e.g. Galaxy redeploy via IRediscoverable.OnRediscoveryNeeded) + /// the previous alarm subscription is torn down and the sink registry is cleared before + /// re-walking, preventing double delivery of alarm transitions. + /// Exception isolation (marking the driver's subtree Faulted) is the caller's responsibility — + /// exceptions from propagate to the caller. /// public async Task BuildAddressSpaceAsync(IAddressSpaceBuilder builder, CancellationToken ct) { ArgumentNullException.ThrowIfNull(builder); + ObjectDisposedException.ThrowIf(_disposed, this); + + // Tear down any previous alarm subscription before re-walking so a second call (e.g. on + // Galaxy redeploy) does not leave the old forwarder subscribed and double-fire events. + if (_alarmForwarder is not null && Driver is IAlarmSource existingSource) + { + existingSource.OnAlarmEvent -= _alarmForwarder; + _alarmForwarder = null; + } + _alarmSinks.Clear(); if (Driver is not ITagDiscovery discovery) throw new NotSupportedException($"Driver '{Driver.DriverInstanceId}' does not implement ITagDiscovery."); diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/GenericDriverNodeManagerTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/GenericDriverNodeManagerTests.cs index 6d47a4f..ed8423a 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/GenericDriverNodeManagerTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/GenericDriverNodeManagerTests.cs @@ -88,6 +88,61 @@ public sealed class GenericDriverNodeManagerTests builder.Alarms["Tank.HiHi"].Received.Count.ShouldBe(0); } + /// + /// Core-006 regression: a second call to BuildAddressSpaceAsync (e.g. on Galaxy redeploy) + /// must unsubscribe the old alarm forwarder and clear the sink registry before re-walking, + /// so alarm transitions are not delivered twice. + /// + [Fact] + public async Task Second_BuildAddressSpaceAsync_Does_Not_Double_Fire_Alarms() + { + var driver = new FakeDriver(); + var builder1 = new RecordingBuilder(); + var builder2 = new RecordingBuilder(); + using var nm = new GenericDriverNodeManager(driver); + + await nm.BuildAddressSpaceAsync(builder1, CancellationToken.None); + await nm.BuildAddressSpaceAsync(builder2, CancellationToken.None); // redeploy + + driver.RaiseAlarm(new AlarmEventArgs( + new FakeHandle("s1"), "Tank.HiHi", "c", "t", "m", AlarmSeverity.High, DateTime.UtcNow)); + + // Only the second builder's sink should have received the event. + builder2.Alarms["Tank.HiHi"].Received.Count.ShouldBe(1, + "second BuildAddressSpaceAsync must replace the subscription — not add to it"); + + // The first builder's sink should NOT have received it (old forwarder was detached). + builder1.Alarms.TryGetValue("Tank.HiHi", out var oldSink); + (oldSink?.Received.Count ?? 0).ShouldBe(0, + "the original alarm forwarder must be unsubscribed on the second build"); + } + + [Fact] + public async Task Second_BuildAddressSpaceAsync_Clears_Old_Sink_Registry() + { + var driver = new FakeDriver(); + using var nm = new GenericDriverNodeManager(driver); + + await nm.BuildAddressSpaceAsync(new RecordingBuilder(), CancellationToken.None); + var countAfterFirst = nm.TrackedAlarmSources.Count; + await nm.BuildAddressSpaceAsync(new RecordingBuilder(), CancellationToken.None); + var countAfterSecond = nm.TrackedAlarmSources.Count; + + countAfterFirst.ShouldBe(2, "FakeDriver registers 2 alarm sources"); + countAfterSecond.ShouldBe(2, "second build must re-register exactly the same sources, not accumulate"); + } + + [Fact] + public async Task BuildAddressSpaceAsync_After_Dispose_Throws_ObjectDisposedException() + { + var driver = new FakeDriver(); + var nm = new GenericDriverNodeManager(driver); + nm.Dispose(); + + await Should.ThrowAsync(() => + nm.BuildAddressSpaceAsync(new RecordingBuilder(), CancellationToken.None)); + } + // --- test doubles --- private sealed class FakeDriver : IDriver, ITagDiscovery, IAlarmSource