From e9da9c29d21c4f65d0100e56a430f6e7859467d8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 18 Jun 2026 04:29:45 -0400 Subject: [PATCH] fix(galaxy): authoritative handle resolution + review cleanups Make SubscriptionRegistry.TryResolveItemHandle confirm a live subscription genuinely binds fullRef->handle (via the reverse index) rather than trusting the forward-map hint + a bare liveness check. Fixes the cross-ref-same-handle hazard (wrong-tag borrow) while preserving the legitimate multiple-subscriptions-per-tag borrow. Adds cross-ref + same-ref-multi-sub tests; drops a duplicate SubscriptionEntry ; documents the writer's supervisory-advise reconnect lifecycle. --- .../Runtime/GatewayGalaxyDataWriter.cs | 4 +- .../Runtime/SubscriptionRegistry.cs | 26 +++++++++---- .../SubscriptionRegistryHandleResolveTests.cs | 39 +++++++++++++++++++ 3 files changed, 61 insertions(+), 8 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyDataWriter.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyDataWriter.cs index bedb1a7e..d9c8f9e5 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyDataWriter.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyDataWriter.cs @@ -210,7 +210,9 @@ public sealed class GatewayGalaxyDataWriter : IGalaxyDataWriter /// a regular Advise instead; we don't log in, so we use the supervisory context.) /// The gateway client doesn't expose a typed method, so we build the /// and route through InvokeAsync (same pattern as ). - /// Idempotent per handle for the session lifetime. + /// Idempotent per handle for the session lifetime. The keyed-by-handle dedupe is reset on a + /// session reconnect via , so a fresh session always + /// re-advises — the supervisory state never outlives the handle it was taken against. /// private async Task EnsureSupervisoryAdvisedAsync( MxGatewaySession session, int serverHandle, int itemHandle, CancellationToken ct) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs index 2570a27d..ff664d9a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs @@ -124,18 +124,31 @@ internal sealed class SubscriptionRegistry /// /// Resolve the live MXAccess item handle a current subscription holds for , /// or null when no live subscription covers it. The Galaxy writer borrows this handle to skip a - /// redundant AddItem round-trip on the first write to an already-subscribed tag. Guarded by the - /// authoritative live-handle set (_subscribersByItemHandle) so a stale forward-map entry - /// can never hand out a dead handle. + /// redundant AddItem round-trip on the first write to an already-subscribed tag. /// + /// + /// The _itemHandleByFullRef forward map is only a hint — resolution is authoritative: + /// it confirms a live subscription actually binds this to + /// the candidate handle (via the per-entry reverse index), so a stale forward entry can never + /// hand the writer a dead handle, nor a handle that now belongs to a different tag. This keeps + /// a tag resolvable while ANY subscription still binds it (the legitimate + /// multiple-driver-subscriptions-per-tag case) yet returns null the moment the last one drops. + /// /// The dotted tag full reference (e.g. TestMachine_002.TestFloat). /// The live item handle, or null when none is currently subscribed. public int? TryResolveItemHandle(string fullRef) { if (fullRef is null) return null; - if (_itemHandleByFullRef.TryGetValue(fullRef, out var handle) - && _subscribersByItemHandle.ContainsKey(handle)) - return handle; + if (!_itemHandleByFullRef.TryGetValue(fullRef, out var handle)) return null; + if (!_subscribersByItemHandle.TryGetValue(handle, out var subs)) return null; // dead handle + + // Confirm a live subscription genuinely binds fullRef -> handle (not a stale forward entry + // pointing at a handle the gateway has since associated with another tag). + foreach (var subId in subs) + if (_bySubscriptionId.TryGetValue(subId, out var entry) + && entry.FullRefByItemHandle.TryGetValue(handle, out var liveRef) + && string.Equals(liveRef, fullRef, StringComparison.OrdinalIgnoreCase)) + return handle; return null; } @@ -201,7 +214,6 @@ internal sealed class SubscriptionRegistry /// (Driver.Galaxy-012). Failed bindings (item handle ≤ 0) are excluded from the /// index because the EventPump only dispatches for positive handles. /// - /// Per-subscription bookkeeping entry. private sealed class SubscriptionEntry { /// Gets the subscription identifier. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/SubscriptionRegistryHandleResolveTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/SubscriptionRegistryHandleResolveTests.cs index 96830f54..e1e71991 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/SubscriptionRegistryHandleResolveTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/SubscriptionRegistryHandleResolveTests.cs @@ -114,4 +114,43 @@ public sealed class SubscriptionRegistryHandleResolveTests // must return null regardless of whether the forward entry was cleaned up. registry.TryResolveItemHandle("Tag.A").ShouldBeNull(); } + + /// + /// A tag may legitimately appear in multiple driver subscriptions (separate OPC UA monitored + /// items on the same Galaxy attribute) — they share one gw item handle. Removing ONE of them + /// must keep the tag resolvable while another subscription still binds it, so the writer keeps + /// borrowing instead of falling back to AddItem. + /// + [Fact] + public void Remove_OneOfTwoSubscribersForSameRef_StillResolves() + { + var registry = new SubscriptionRegistry(); + registry.Register(1, [new TagBinding("Tag.A", 5)]); + registry.Register(2, [new TagBinding("Tag.A", 5)]); + + registry.Remove(1); + + // sub2 still binds Tag.A -> 5, so the borrow must still be offered. + registry.TryResolveItemHandle("Tag.A").ShouldBe(5); + } + + /// + /// Authoritative resolution: if two DIFFERENT refs ever map to the same numeric handle and the + /// subscription for one is removed, that ref must NOT resolve to the handle that now belongs to + /// the other ref — a wrong-tag write would be the worst outcome. Resolution confirms a live + /// subscription genuinely binds fullRef -> handle, not just that the handle is alive. + /// + [Fact] + public void Remove_CrossRefSameHandle_DoesNotResolveToTheOtherRefsHandle() + { + var registry = new SubscriptionRegistry(); + registry.Register(1, [new TagBinding("Tag.A", 5)]); + registry.Register(2, [new TagBinding("Tag.B", 5)]); + + registry.Remove(1); + + // Tag.A's subscription is gone; handle 5 is still alive but now only Tag.B binds it. + registry.TryResolveItemHandle("Tag.A").ShouldBeNull(); + registry.TryResolveItemHandle("Tag.B").ShouldBe(5); + } }