From c9ac075e39ac2f5eae13ec0e2ef58986ed16ae62 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 9 Jul 2026 02:09:06 -0400 Subject: [PATCH] =?UTF-8?q?fix(site-runtime,dcl):=20low-severity=20cleanup?= =?UTF-8?q?s=20=E2=80=94=20observed=20adapter=20dispose=20(S9),=20alarm-fi?= =?UTF-8?q?lter=20overwrite=20warning=20(S10),=20bounded-channel=20comment?= =?UTF-8?q?=20(C2),=20invariant-culture=20condition=20fallback=20(C6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Actors/DataConnectionActor.cs | 18 ++++++++++- .../SiteEventLogger.cs | 10 +++--- .../Actors/ScriptActor.cs | 10 ++++-- .../DataConnectionActorAlarmTests.cs | 23 ++++++++++++++ .../Actors/ScriptActorTests.cs | 31 +++++++++++++++++++ 5 files changed, 84 insertions(+), 8 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs index d3940ff3..a390a481 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs @@ -229,7 +229,12 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers { _log.Info("DataConnectionActor [{0}] stopping — disposing adapter", _connectionName); _adapter.Disconnected -= OnAdapterDisconnected; - _ = _adapter.DisposeAsync().AsTask(); + // S9: fire-and-forget, but observe a faulted dispose so a failing adapter + // teardown is logged rather than swallowed into an unobserved task exception. + _adapter.DisposeAsync().AsTask().ContinueWith( + t => _log.Warning("[{0}] Adapter dispose faulted on stop: {1}", + _connectionName, t.Exception?.GetBaseException().Message), + TaskContinuationOptions.OnlyOnFaulted); } /// @@ -1829,6 +1834,17 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers _alarmSourceSubscribers[request.SourceReference] = subs; } subs.Add(subscriber); + // S10: the adapter feed carries a single shared condition filter per source. + // A second subscriber with a different filter silently overwrites the first + // (last-writer-wins). Surface it so a co-subscriber mismatch is diagnosable — + // co-subscribers to one source are expected to agree on the filter. + if (_alarmSourceFilter.TryGetValue(request.SourceReference, out var existingFilter) + && !string.Equals(existingFilter, request.ConditionFilter, StringComparison.Ordinal)) + { + _log.Warning( + "[{0}] Alarm condition filter for source {1} overwritten: '{2}' -> '{3}' (last subscriber wins; co-subscribers must agree)", + _connectionName, request.SourceReference, existingFilter, request.ConditionFilter); + } _alarmSourceFilter[request.SourceReference] = request.ConditionFilter; // Parse the type-name filter once; this is the authoritative client-side // gate consulted on every routed transition. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/SiteEventLogger.cs b/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/SiteEventLogger.cs index 787ca725..23ce86e5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/SiteEventLogger.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/SiteEventLogger.cs @@ -212,11 +212,11 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable // on disk I/O or on contention for the write lock. if (!_writeQueue.Writer.TryWrite(pending)) { - // The channel is unbounded, so the only way TryWrite fails is that the - // writer has been completed (logger disposed). The event cannot be - // persisted — fault the Task rather than - // reporting false success, so a caller that awaits a critical audit - // event can tell it was dropped. + // The channel is bounded with DropOldest, so TryWrite only returns false + // once the channel is completed (logger disposed) — full-buffer pressure + // evicts the oldest pending event instead of failing the write. The event + // cannot be persisted — fault the Task rather than reporting false success, + // so a caller that awaits a critical audit event can tell it was dropped. pending.Completion.TrySetException( new ObjectDisposedException(nameof(SiteEventLogger), "Event could not be recorded: the event logger has been disposed.")); diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/ScriptActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/ScriptActor.cs index 11be2a59..f4ad1648 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/ScriptActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/ScriptActor.cs @@ -475,7 +475,10 @@ public class ScriptActor : ReceiveActor, IWithTimers _scriptName, _instanceName, msg.Success); } - private static bool EvaluateCondition(ConditionalTriggerConfig config, object? value) + // internal (not private) so the culture-invariance of the non-numeric fallback + // can be unit-tested directly on the test thread — the live path evaluates on a + // dispatcher thread whose CurrentCulture the test cannot deterministically set. + internal static bool EvaluateCondition(ConditionalTriggerConfig config, object? value) { if (value == null) return false; @@ -500,7 +503,10 @@ public class ScriptActor : ReceiveActor, IWithTimers } catch { - return string.Equals(value.ToString(), config.Threshold.ToString(), StringComparison.Ordinal); + // Render the threshold with InvariantCulture to match the invariant numeric + // parse two lines up — otherwise a de-DE host renders 1.5 as "1,5" and the + // string fallback fires spuriously on locale-formatted values (C6). + return string.Equals(value.ToString(), config.Threshold.ToString(CultureInfo.InvariantCulture), StringComparison.Ordinal); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionActorAlarmTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionActorAlarmTests.cs index dd3bf5b5..80647534 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionActorAlarmTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionActorAlarmTests.cs @@ -81,6 +81,29 @@ public class DataConnectionActorAlarmTests : TestKit ExpectMsg(m => !m.Success && m.ErrorMessage != null); } + [Fact] + public void SecondAlarmSubscriber_WithDifferentFilter_LogsOverwriteWarning() + { + // The adapter alarm feed carries a single shared condition filter per source; + // a second subscriber with a different filter overwrites it (last-writer-wins). + // That silent overwrite must be surfaced as a warning so a co-subscriber + // mismatch is diagnosable (S10). + var (adapter, _) = BuildAlarmAdapter(); + var actor = Sys.ActorOf(Props.Create(() => new DataConnectionActor( + "conn", adapter, _options, _health, _factory, "OpcUa"))); + + actor.Tell(new SubscribeAlarmsRequest( + "c1", "instA", "conn", "Tank01", "AnalogLimit.Hi", DateTimeOffset.UtcNow)); + ExpectMsg(m => m.Success); + + EventFilter.Warning(contains: "condition filter").ExpectOne(() => + { + actor.Tell(new SubscribeAlarmsRequest( + "c2", "instB", "conn", "Tank01", "AnalogLimit.Lo", DateTimeOffset.UtcNow)); + ExpectMsg(m => m.Success); + }); + } + // ── M2.4 (#8): conditionFilter is now applied client-side in the actor ── [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ScriptActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ScriptActorTests.cs index 7be99989..c5965c1b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ScriptActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ScriptActorTests.cs @@ -1,3 +1,4 @@ +using System.Globalization; using Akka.Actor; using Akka.TestKit; using Akka.TestKit.Xunit2; @@ -300,6 +301,36 @@ public class ScriptActorTests : TestKit, IDisposable private AttributeValueChanged Change(string attribute, object? value) => new("TestInstance", attribute, attribute, value, "Good", DateTimeOffset.UtcNow); + [Fact] + public void ConditionalTrigger_NonNumericFallback_IsCultureInvariant() + { + // EvaluateCondition runs on a dispatcher thread in the live path, whose + // CurrentCulture a test cannot deterministically set — so the culture- + // invariance of the string fallback is verified by calling the pure helper + // directly under a forced de-DE culture. + var original = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo("de-DE"); // 1234.5 -> "1.234,5" + + // "1.234,5" is the de-DE rendering of 1234.5: it FAILS the invariant + // numeric parse (decimal point then group separator) and falls through to + // the string comparison. The old fallback compared against the culture- + // sensitive Threshold.ToString() ("1.234,5" under de-DE) and matched (fired); + // the invariant fallback compares against "1234.5" and must NOT match. + var config = new ConditionalTriggerConfig("Temp", "==", 1234.5, TriggerMode.OnTrue); + Assert.False(ScriptActor.EvaluateCondition(config, "1.234,5")); + + // Sanity: a value that genuinely equals the invariant rendering still matches. + Assert.True(ScriptActor.EvaluateCondition( + new ConditionalTriggerConfig("Temp", "==", 1234.5, TriggerMode.OnTrue), "1234.5")); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + private Script CompileTriggerExpression(string expression) => _compilationService.CompileTriggerExpression("trigger-expr", expression).CompiledScript!;