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;