From d14564839ed446a285f041819a61dd32783035fe Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:45:55 -0400 Subject: [PATCH] fix(driver-galaxy): resolve Medium code-review finding (Driver.Galaxy-006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HashSet.First() enumeration order is unspecified and unstable across mutations, so the "owner" handle attached to alarm events was non-deterministic when multiple alarm subscriptions were active. Change _alarmSubscriptions from HashSet to List (preserving insertion order) and pick [0] — the earliest-registered handle — as the deterministic owner. The server routes transitions by SourceNodeId, not by handle, so the choice of handle does not affect delivery to active subscribers. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.Galaxy/findings.md | 4 ++-- .../GalaxyDriver.cs | 18 ++++++++++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/code-reviews/Driver.Galaxy/findings.md b/code-reviews/Driver.Galaxy/findings.md index a964ca7..60b22f5 100644 --- a/code-reviews/Driver.Galaxy/findings.md +++ b/code-reviews/Driver.Galaxy/findings.md @@ -108,13 +108,13 @@ | Severity | Medium | | Category | Concurrency & thread safety | | Location | `GalaxyDriver.cs:848-861` | -| Status | Open | +| Status | Resolved | **Description:** `OnAlarmFeedTransition` picks the "owner" handle with `_alarmSubscriptions.First()` under `_alarmHandlersLock`. `HashSet.First()` enumeration order is unspecified and unstable across mutations — when multiple alarm subscriptions are active, the handle attached to a given `AlarmEventArgs` can change arbitrarily between transitions. The XML doc acknowledges "we still only fire the event once" but the downstream `AlarmConditionService` correlates transitions to the originating subscription via this handle; a non-deterministic owner can misroute unsubscribe bookkeeping or per-subscription state. **Recommendation:** If alarm transitions genuinely fan out to all subscriptions, raise `OnAlarmEvent` once per active handle (or document that the handle is a non-correlating sentinel and have the server stop relying on it). If a single owner is required, make the choice deterministic (e.g. the earliest-created handle) and stable. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — changed `_alarmSubscriptions` from `HashSet` to `List` so insertion order is preserved; `OnAlarmFeedTransition` now picks `[0]` (earliest-registered handle) instead of `First()` on a HashSet, making the owner selection deterministic and stable across mutations. Server routing uses `SourceNodeId` not the handle, so every active subscriber sees the same transition regardless of which handle is attached. ### Driver.Galaxy-007 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs index f687b03..4db3309 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs @@ -75,7 +75,10 @@ public sealed class GalaxyDriver private readonly Lock _alarmHandlersLock = new(); private readonly Lock _alarmFeedLock = new(); private bool _alarmFeedWired; - private readonly HashSet _alarmSubscriptions = new(); + // List preserves insertion order so OnAlarmFeedTransition always picks the + // earliest-registered handle — a deterministic choice that doesn't vary as + // handles are added/removed (Driver.Galaxy-006 fix: HashSet.First() is unstable). + private readonly List _alarmSubscriptions = new(); // PR 4.W — production runtime owned by InitializeAsync. The driver builds these // when it opens a real gw session; tests bypass them by injecting seams via the @@ -964,12 +967,15 @@ public sealed class GalaxyDriver GalaxyAlarmSubscriptionHandle? handle; lock (_alarmHandlersLock) { - // Pick any active subscription handle as the "owner" of the event. The - // server-side state machine doesn't multiplex by handle today; if multiple - // alarm subscriptions are active we still only fire the event once and - // the AlarmConditionService dispatches per-source-node downstream. + // Pick the earliest-registered handle as the event owner. The server routes + // by SourceNodeId (not by handle), so every active subscriber sees the same + // transition regardless of which handle is attached here. Using the first + // insertion-order entry is deterministic and stable as long as at least one + // subscription remains — HashSet.First() was unstable across mutations + // (Driver.Galaxy-006 fix). _alarmSubscriptions is a List, so [0] is always + // the earliest-registered handle. handle = _alarmSubscriptions.Count > 0 - ? _alarmSubscriptions.First() + ? _alarmSubscriptions[0] : null; } if (handle is null) return;