From cdaa0da45d1ecd09986f410564c756002c6828ae Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 08:17:50 -0400 Subject: [PATCH] fix(scripted-alarms): resolve Medium code-review finding (Core.ScriptedAlarms-005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add _disposed re-checks inside ReevaluateAsync and ShelvingCheckAsync after acquiring _evalGate so callbacks in flight when Dispose() runs bail out cleanly instead of mutating _alarms or writing to a disposed store. Drop the _alarms.Clear() from Dispose() — clearing outside the gate races concurrent reads and is unnecessary since the object is being discarded. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../ScriptedAlarmEngine.cs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) 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 de8301d..e23d80a 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs @@ -266,6 +266,12 @@ public sealed class ScriptedAlarmEngine : IDisposable await _evalGate.WaitAsync(ct).ConfigureAwait(false); try { + // Re-check after acquiring the gate: a Dispose() call may have + // completed between our _evalGate.WaitAsync and here. Writing to a + // disposing store or mutating _alarms after clear is unsafe. + // (Core.ScriptedAlarms-005) + if (_disposed) return; + foreach (var id in alarmIds) { if (!_alarms.TryGetValue(id, out var state)) continue; @@ -402,6 +408,13 @@ public sealed class ScriptedAlarmEngine : IDisposable await _evalGate.WaitAsync(ct).ConfigureAwait(false); try { + // Re-check after acquiring the gate: Timer.Dispose() does not wait for + // running callbacks, so a shelving-check callback that passed the _disposed + // check in RunShelvingCheck can arrive here after Dispose() has returned. + // Mutating _alarms or saving to a disposed store here is unsafe. + // (Core.ScriptedAlarms-005) + if (_disposed) return; + var now = _clock(); foreach (var id in alarmIds) { @@ -445,7 +458,11 @@ public sealed class ScriptedAlarmEngine : IDisposable _disposed = true; _shelvingTimer?.Dispose(); UnsubscribeFromUpstream(); - _alarms.Clear(); + // Do NOT clear _alarms here: Timer.Dispose() does not wait for in-flight callbacks, + // so a ShelvingCheckAsync or ReevaluateAsync can still be running inside _evalGate. + // Those paths now re-check _disposed after acquiring the gate and bail out safely. + // Clearing _alarms outside the gate would race concurrent reads and is unnecessary + // (the whole object is being discarded). (Core.ScriptedAlarms-005) _alarmsReferencing.Clear(); }