From 7c5a843ace54a5fa3fe88d7148b373246d99843e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 04:32:53 -0400 Subject: [PATCH] docs(code): close-out comment fixes from the follow-ups review round --- .../Observability/OtOpcUaTelemetry.cs | 4 +++- .../ScriptedAlarmEngine.cs | 6 ++++++ .../Security/SessionAclDecisionCache.cs | 14 +++++++++++--- .../Drivers/DriverHostActor.cs | 3 +++ .../AdminUiSqlServerFixture.cs | 5 +++++ 5 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs index 6187acaf..09950d24 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs @@ -47,7 +47,9 @@ public static class OtOpcUaTelemetry /// PollGroupEngine.OverrunCount. Core.Abstractions deliberately carries no metrics /// dependency, so each poll driver wires this counter to the engine's onOverrun callback; /// this is the production consumer that surface was always missing. Tagged - /// driver.type (a DriverTypeNames constant) + driver.instance. + /// (driver_type, a DriverTypeNames constant) + /// + (driver_instance) — snake_case, per the + /// convention those two consts document. /// A non-zero and climbing rate means the device cannot sustain its configured poll rate — the /// loop is already running flat out. /// diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs index fe75952a..2acbb9cd 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs @@ -1036,6 +1036,9 @@ public sealed class ScriptedAlarmEngine : IDisposable Task work; try { + // The null coalesce is defensive only: no current caller can return a null Task + // (every factory is an async lambda or returns a started Task). It costs nothing and + // keeps a future non-async factory from NREing the continuation wiring below. work = factory() ?? Task.CompletedTask; } catch (Exception ex) @@ -1057,6 +1060,9 @@ public sealed class ScriptedAlarmEngine : IDisposable // so Dispose's WhenAll still observes (and logs) a background failure. work.ContinueWith(t => { + // Removing BEFORE completing the placeholder is safe (not a window where Dispose could + // miss still-running work): this continuation only runs once the REAL task has already + // finished, so anything a concurrent Dispose could still be waiting for is done. lock (_inFlightLock) { _inFlight.Remove(placeholder.Task); } if (t.IsFaulted) { diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs index 1a5f3c75..fa190a03 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs @@ -57,9 +57,11 @@ public sealed class SessionAclDecisionCache /// , which takes every lock in the table — a /// price the store path would pay on every miss to answer a question that only matters at the /// ceiling. The counter lags — an insert is in its shard's dictionary a moment before it is booked - /// — but it does not drift: every entry is booked exactly once and unbooked exactly once, when the - /// shard holding it is sealed. See for how that survives an eviction - /// racing an insert. + /// — but it does not drift: every booked entry is unbooked exactly once, when the shard + /// holding it is sealed. (Not every entry is booked: one whose loses + /// the compare-exchange to a seal is never booked at all, because it is already unreachable in a + /// displaced shard.) See for how that survives an eviction racing an + /// insert. /// public const int MaxEntries = 100_000; @@ -209,6 +211,12 @@ public sealed class SessionAclDecisionCache { // The outer bound: no single session is over its own ceiling, so there is nothing narrower to // drop. Re-acquire the shard — ClearAll drops the shard objects themselves. + // + // Deliberately NOT elected: concurrent callers crossing the ceiling together all run ClearAll, + // and that is safe rather than merely tolerated. ClearAll displaces each shard with a per-shard + // TryRemove, so exactly one thread wins each shard and seals it exactly once; the losers just + // re-enumerate an emptying dictionary. The redundancy costs an enumeration, not correctness, + // and a CAS-elected clear would add hot-path state for a branch that is a rare fallback. ClearAll(); shard = _shards.GetOrAdd(sessionKey, static _ => new Shard()); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs index af024a99..e95ca092 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs @@ -2119,6 +2119,9 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS // // Guarded on IsRelational: the timeout is a relational-only concept and the extension throws // on a non-relational provider (the in-memory provider the actor tests run against). + // ⚠️ Consequently the relational branch itself is UNTESTED in-suite — every actor test runs + // InMemory, so the suite only ever exercises the guard's false side. The 30 s bound is pinned + // by the EF 10.0.7 verification recorded above, not by a test. if (db.Database.IsRelational()) db.Database.SetCommandTimeout(TimeSpan.FromSeconds(30)); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs index 99390735..26a8b7bb 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs @@ -32,6 +32,11 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests; /// the same one-shot probe in , so a session probes once. /// /// +/// The probe being one-shot bounds what "stays green offline" guarantees: it covers SQL +/// being unreachable before the run. A connectivity blip during a run is never +/// re-probed, so the affected tests surface as SqlException failures, not skips. +/// +/// /// Every test in the collection shares ONE database (migrating is the expensive part), so tests /// must isolate themselves by seeding uniquely-named rows, or by clearing the table they measure. /// xunit serialises a collection's tests, so a clear-then-seed is safe.