Revert "fix(lmxproxy): resolve subscribe/unsubscribe race condition on client reconnect"

This reverts commit 9e9efbecab399fd7dcfb3e7e14e8b08418c3c3fc.

lmxproxy/src/ZB.MOM.WW.LmxProxy.Host/Grpc/Services/ScadaGrpcService.cs

@@ -360,7 +360,7 @@ namespace ZB.MOM.WW.LmxProxy.Host.Grpc.Services
                 throw new RpcException(new GrpcStatus(StatusCode.Unauthenticated, "Invalid session"));
             }
 
-            var reader = await _subscriptionManager.SubscribeAsync(
+            var reader = _subscriptionManager.Subscribe(
                 request.SessionId, request.Tags, context.CancellationToken);
 
             try

lmxproxy/src/ZB.MOM.WW.LmxProxy.Host/MxAccess/MxAccessClient.EventHandlers.cs

@@ -53,21 +53,21 @@ namespace ZB.MOM.WW.LmxProxy.Host.MxAccess
                     }
                 }
 
-                // Invoke the stored subscription callback (SubscriptionManager.OnTagValueChanged).
+                // Invoke the stored subscription callback
-                // This is the single delivery path — OnTagValueChanged property is NOT invoked
-                // separately to avoid duplicate VTQ delivery.
                 Action<string, Vtq> callback;
                 lock (_lock)
                 {
                     if (!_storedSubscriptions.TryGetValue(address, out callback))
                     {
-                        // Fall back to global handler if no stored callback
+                        Log.Debug("OnDataChange for {Address} but no callback registered", address);
-                        OnTagValueChanged?.Invoke(address, vtq);
                         return;
                     }
                 }
 
                 callback.Invoke(address, vtq);
+
+                // Also route to the SubscriptionManager's global handler
+                OnTagValueChanged?.Invoke(address, vtq);
             }
             catch (Exception ex)
             {

lmxproxy/src/ZB.MOM.WW.LmxProxy.Host/Subscriptions/SubscriptionManager.cs

@@ -32,15 +32,6 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
 
         private readonly ReaderWriterLockSlim _rwLock = new ReaderWriterLockSlim();
 
-        // Serializes Subscribe and UnsubscribeClient to prevent race conditions
-        // where old-session unsubscribe removes new-session COM subscriptions.
-        private readonly SemaphoreSlim _subscriptionGate = new SemaphoreSlim(1, 1);
-
-        // Tags that failed MxAccess subscription (e.g., MxAccess was down).
-        // Retried on reconnect via RetryPendingSubscriptions().
-        private readonly HashSet<string> _pendingTags
-            = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
-
         public SubscriptionManager(IScadaClient scadaClient, int channelCapacity = 1000,
             BoundedChannelFullMode channelFullMode = BoundedChannelFullMode.DropOldest)
         {
@@ -51,14 +42,9 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
 
         /// <summary>
         ///     Creates a subscription for a client. Returns a ChannelReader to stream from.
-        ///     Serialized with UnsubscribeClient via _subscriptionGate to prevent race
-        ///     conditions during client reconnect (old unsubscribe vs new subscribe).
         /// </summary>
-        public async Task<ChannelReader<(string address, Vtq vtq)>> SubscribeAsync(
+        public ChannelReader<(string address, Vtq vtq)> Subscribe(
             string clientId, IEnumerable<string> addresses, CancellationToken ct)
-        {
-            await _subscriptionGate.WaitAsync(ct);
-            try
         {
             var channel = Channel.CreateBounded<(string address, Vtq vtq)>(
                 new BoundedChannelOptions(_channelCapacity)
@@ -97,10 +83,10 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
                 _rwLock.ExitWriteLock();
             }
 
-                // Create MxAccess COM subscriptions for newly subscribed tags (awaited, not fire-and-forget)
+            // Create MxAccess COM subscriptions for newly subscribed tags
             if (newTags.Count > 0)
             {
-                    await CreateMxAccessSubscriptionsAsync(newTags);
+                _ = CreateMxAccessSubscriptionsAsync(newTags);
             }
 
             // Register cancellation cleanup
@@ -110,11 +96,6 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
                 clientId, addressSet.Count, newTags.Count);
             return channel.Reader;
         }
-            finally
-            {
-                _subscriptionGate.Release();
-            }
-        }
 
         private async Task CreateMxAccessSubscriptionsAsync(List<string> addresses)
         {
@@ -123,25 +104,10 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
                 await _scadaClient.SubscribeAsync(
                     addresses,
                     (address, vtq) => OnTagValueChanged(address, vtq));
-
-                // Successful — remove from pending if they were there
-                lock (_pendingTags)
-                {
-                    foreach (var address in addresses)
-                        _pendingTags.Remove(address);
-                }
             }
             catch (Exception ex)
             {
-                Log.Error(ex, "Failed to create MxAccess subscriptions for {Count} tags — " +
+                Log.Error(ex, "Failed to create MxAccess subscriptions for {Count} tags", addresses.Count);
-                    "storing as pending for retry on reconnect", addresses.Count);
-
-                // Store failed addresses for retry when MxAccess reconnects
-                lock (_pendingTags)
-                {
-                    foreach (var address in addresses)
-                        _pendingTags.Add(address);
-                }
             }
         }
 
@@ -187,13 +153,9 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
 
         /// <summary>
         ///     Removes a client's subscriptions and cleans up tag subscriptions
-        ///     when the last client unsubscribes. Serialized with SubscribeAsync
+        ///     when the last client unsubscribes.
-        ///     via _subscriptionGate to prevent race conditions.
         /// </summary>
         public void UnsubscribeClient(string clientId)
-        {
-            _subscriptionGate.Wait();
-            try
         {
             if (!_clientSubscriptions.TryRemove(clientId, out var clientSub))
                 return;
@@ -226,13 +188,6 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
             // Unsubscribe tags with no remaining clients via address-based API
             if (tagsToDispose.Count > 0)
             {
-                    // Also remove from pending if they were awaiting retry
-                    lock (_pendingTags)
-                    {
-                        foreach (var address in tagsToDispose)
-                            _pendingTags.Remove(address);
-                    }
-
                 try
                 {
                     _scadaClient.UnsubscribeByAddressAsync(tagsToDispose).GetAwaiter().GetResult();
@@ -249,11 +204,6 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
             Log.Information("Client {ClientId} unsubscribed ({Delivered} delivered, {Dropped} dropped)",
                 clientId, clientSub.DeliveredCount, clientSub.DroppedCount);
         }
-            finally
-            {
-                _subscriptionGate.Release();
-            }
-        }
 
         /// <summary>
         ///     Sends a bad-quality notification to all subscribed clients for all their tags.
@@ -273,8 +223,8 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
         }
 
         /// <summary>
-        ///     Called when MxAccess reconnects. Retries any pending subscriptions
+        ///     Logs reconnection for observability. Data flow resumes automatically
-        ///     that failed during the disconnected period.
+        ///     via MxAccessClient.RecreateStoredSubscriptionsAsync callbacks.
         /// </summary>
         public void NotifyReconnection()
         {
@@ -282,24 +232,6 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
                 "data flow will resume via OnDataChange callbacks " +
                 "({ClientCount} clients, {TagCount} tags)",
                 _clientSubscriptions.Count, _tagSubscriptions.Count);
-
-            _ = RetryPendingSubscriptionsAsync();
-        }
-
-        /// <summary>
-        ///     Retries MxAccess subscriptions for tags that failed during disconnect.
-        /// </summary>
-        private async Task RetryPendingSubscriptionsAsync()
-        {
-            List<string> tagsToRetry;
-            lock (_pendingTags)
-            {
-                if (_pendingTags.Count == 0) return;
-                tagsToRetry = new List<string>(_pendingTags);
-            }
-
-            Log.Information("Retrying {Count} pending MxAccess subscriptions after reconnect", tagsToRetry.Count);
-            await CreateMxAccessSubscriptionsAsync(tagsToRetry);
         }
 
         /// <summary>Returns subscription statistics.</summary>
@@ -320,7 +252,6 @@ namespace ZB.MOM.WW.LmxProxy.Host.Subscriptions
             _clientSubscriptions.Clear();
             _tagSubscriptions.Clear();
             _rwLock.Dispose();
-            _subscriptionGate.Dispose();
         }
 
         // ── Nested types ─────────────────────────────────────────

lmxproxy/tests/ZB.MOM.WW.LmxProxy.Host.Tests/Health/HealthCheckServiceTests.cs

@@ -104,7 +104,7 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Health
             for (int i = 0; i < 101; i++)
             {
                 using var cts = new CancellationTokenSource();
-                await sm.SubscribeAsync("client-" + i, new[] { "tag1" }, cts.Token);
+                sm.Subscribe("client-" + i, new[] { "tag1" }, cts.Token);
             }
 
             var svc = new HealthCheckService(client, sm, pm);

lmxproxy/tests/ZB.MOM.WW.LmxProxy.Host.Tests/Subscriptions/SubscriptionManagerTests.cs

@@ -1,6 +1,5 @@
 using System;
 using System.Collections.Generic;
-using System.Linq;
 using System.Threading;
 using System.Threading.Channels;
 using System.Threading.Tasks;
@@ -33,34 +32,11 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
                 Task.FromResult((false, 0));
             public Task<ProbeResult> ProbeConnectionAsync(string testTagAddress, int timeoutMs, CancellationToken ct = default) =>
                 Task.FromResult(ProbeResult.Healthy(Quality.Good, DateTime.UtcNow));
+            public Task UnsubscribeByAddressAsync(IEnumerable<string> addresses) => Task.CompletedTask;
+            public Task<IAsyncDisposable> SubscribeAsync(IEnumerable<string> addresses, Action<string, Vtq> callback, CancellationToken ct = default) =>
+                Task.FromResult<IAsyncDisposable>(new FakeSubscriptionHandle());
             public ValueTask DisposeAsync() => default;
 
-            // Track subscribe/unsubscribe calls for assertions
-            public List<List<string>> SubscribeCalls { get; } = new List<List<string>>();
-            public List<List<string>> UnsubscribeCalls { get; } = new List<List<string>>();
-            public List<Action<string, Vtq>> StoredCallbacks { get; } = new List<Action<string, Vtq>>();
-
-            // When true, SubscribeAsync throws to simulate MxAccess being down
-            public bool FailSubscriptions { get; set; }
-
-            public Task UnsubscribeByAddressAsync(IEnumerable<string> addresses)
-            {
-                UnsubscribeCalls.Add(addresses.ToList());
-                return Task.CompletedTask;
-            }
-
-            public Task<IAsyncDisposable> SubscribeAsync(IEnumerable<string> addresses, Action<string, Vtq> callback, CancellationToken ct = default)
-            {
-                var addressList = addresses.ToList();
-                SubscribeCalls.Add(addressList);
-                StoredCallbacks.Add(callback);
-
-                if (FailSubscriptions)
-                    throw new InvalidOperationException("Not connected to MxAccess");
-
-                return Task.FromResult<IAsyncDisposable>(new FakeSubscriptionHandle());
-            }
-
             // Suppress unused event warning
             internal void FireEvent() => ConnectionStateChanged?.Invoke(this, null!);
 
@@ -71,11 +47,11 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
         }
 
         [Fact]
-        public async Task Subscribe_ReturnsChannelReader()
+        public void Subscribe_ReturnsChannelReader()
         {
             using var sm = new SubscriptionManager(new FakeScadaClient());
             using var cts = new CancellationTokenSource();
-            var reader = await sm.SubscribeAsync("client1", new[] { "Tag1", "Tag2" }, cts.Token);
+            var reader = sm.Subscribe("client1", new[] { "Tag1", "Tag2" }, cts.Token);
             reader.Should().NotBeNull();
         }
 
@@ -84,7 +60,7 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
         {
             using var sm = new SubscriptionManager(new FakeScadaClient());
             using var cts = new CancellationTokenSource();
-            var reader = await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
+            var reader = sm.Subscribe("client1", new[] { "Motor.Speed" }, cts.Token);
 
             var vtq = Vtq.Good(42.0);
             sm.OnTagValueChanged("Motor.Speed", vtq);
@@ -100,8 +76,8 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
         {
             using var sm = new SubscriptionManager(new FakeScadaClient());
             using var cts = new CancellationTokenSource();
-            var reader1 = await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
+            var reader1 = sm.Subscribe("client1", new[] { "Motor.Speed" }, cts.Token);
-            var reader2 = await sm.SubscribeAsync("client2", new[] { "Motor.Speed" }, cts.Token);
+            var reader2 = sm.Subscribe("client2", new[] { "Motor.Speed" }, cts.Token);
 
             sm.OnTagValueChanged("Motor.Speed", Vtq.Good(99.0));
 
@@ -112,11 +88,11 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
         }
 
         [Fact]
-        public async Task OnTagValueChanged_NonSubscribedTag_NoDelivery()
+        public void OnTagValueChanged_NonSubscribedTag_NoDelivery()
         {
             using var sm = new SubscriptionManager(new FakeScadaClient());
             using var cts = new CancellationTokenSource();
-            var reader = await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
+            var reader = sm.Subscribe("client1", new[] { "Motor.Speed" }, cts.Token);
 
             sm.OnTagValueChanged("Motor.Torque", Vtq.Good(10.0));
 
@@ -125,11 +101,11 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
         }
 
         [Fact]
-        public async Task UnsubscribeClient_CompletesChannel()
+        public void UnsubscribeClient_CompletesChannel()
         {
             using var sm = new SubscriptionManager(new FakeScadaClient());
             using var cts = new CancellationTokenSource();
-            var reader = await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
+            var reader = sm.Subscribe("client1", new[] { "Motor.Speed" }, cts.Token);
 
             sm.UnsubscribeClient("client1");
 
@@ -138,11 +114,11 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
         }
 
         [Fact]
-        public async Task UnsubscribeClient_RemovesFromTagSubscriptions()
+        public void UnsubscribeClient_RemovesFromTagSubscriptions()
         {
             using var sm = new SubscriptionManager(new FakeScadaClient());
             using var cts = new CancellationTokenSource();
-            await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
+            sm.Subscribe("client1", new[] { "Motor.Speed" }, cts.Token);
 
             sm.UnsubscribeClient("client1");
 
@@ -152,12 +128,12 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
         }
 
         [Fact]
-        public async Task RefCounting_LastClientUnsubscribeRemovesTag()
+        public void RefCounting_LastClientUnsubscribeRemovesTag()
         {
             using var sm = new SubscriptionManager(new FakeScadaClient());
             using var cts = new CancellationTokenSource();
-            await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
+            sm.Subscribe("client1", new[] { "Motor.Speed" }, cts.Token);
-            await sm.SubscribeAsync("client2", new[] { "Motor.Speed" }, cts.Token);
+            sm.Subscribe("client2", new[] { "Motor.Speed" }, cts.Token);
 
             sm.GetStats().TotalTags.Should().Be(1);
 
@@ -169,11 +145,11 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
         }
 
         [Fact]
-        public async Task NotifyDisconnection_SendsBadQualityToAll()
+        public void NotifyDisconnection_SendsBadQualityToAll()
         {
             using var sm = new SubscriptionManager(new FakeScadaClient());
             using var cts = new CancellationTokenSource();
-            var reader = await sm.SubscribeAsync("client1", new[] { "Motor.Speed", "Motor.Torque" }, cts.Token);
+            var reader = sm.Subscribe("client1", new[] { "Motor.Speed", "Motor.Torque" }, cts.Token);
 
             sm.NotifyDisconnection();
 
@@ -185,11 +161,11 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
         }
 
         [Fact]
-        public async Task Backpressure_DropOldest_DropsWhenFull()
+        public void Backpressure_DropOldest_DropsWhenFull()
         {
             using var sm = new SubscriptionManager(new FakeScadaClient(), channelCapacity: 3);
             using var cts = new CancellationTokenSource();
-            var reader = await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
+            var reader = sm.Subscribe("client1", new[] { "Motor.Speed" }, cts.Token);
 
             // Fill the channel beyond capacity
             for (int i = 0; i < 10; i++)
@@ -204,123 +180,17 @@ namespace ZB.MOM.WW.LmxProxy.Host.Tests.Subscriptions
         }
 
         [Fact]
-        public async Task GetStats_ReturnsCorrectCounts()
+        public void GetStats_ReturnsCorrectCounts()
         {
             using var sm = new SubscriptionManager(new FakeScadaClient());
             using var cts = new CancellationTokenSource();
-            await sm.SubscribeAsync("c1", new[] { "Tag1", "Tag2" }, cts.Token);
+            sm.Subscribe("c1", new[] { "Tag1", "Tag2" }, cts.Token);
-            await sm.SubscribeAsync("c2", new[] { "Tag2", "Tag3" }, cts.Token);
+            sm.Subscribe("c2", new[] { "Tag2", "Tag3" }, cts.Token);
 
             var stats = sm.GetStats();
             stats.TotalClients.Should().Be(2);
             stats.TotalTags.Should().Be(3); // Tag1, Tag2, Tag3
             stats.ActiveSubscriptions.Should().Be(4); // c1:Tag1, c1:Tag2, c2:Tag2, c2:Tag3
         }
-
-        // ── New tests for race condition fix ──────────────────────────
-
-        [Fact]
-        public async Task SubscribeAfterUnsubscribe_CreatesMxAccessSubscriptions()
-        {
-            // Verifies FIX 1: when a client disconnects and reconnects with the same tags,
-            // the new subscribe must create fresh MxAccess subscriptions (not skip them
-            // because old handles still exist).
-            var fake = new FakeScadaClient();
-            using var sm = new SubscriptionManager(fake);
-            using var cts = new CancellationTokenSource();
-
-            // First client subscribes
-            await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
-            fake.SubscribeCalls.Should().HaveCount(1);
-            fake.SubscribeCalls[0].Should().Contain("Motor.Speed");
-
-            // Client disconnects — unsubscribe removes the tag (ref count → 0)
-            sm.UnsubscribeClient("client1");
-            fake.UnsubscribeCalls.Should().HaveCount(1);
-            fake.UnsubscribeCalls[0].Should().Contain("Motor.Speed");
-
-            // Same client reconnects — must create a NEW MxAccess subscription
-            await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
-            fake.SubscribeCalls.Should().HaveCount(2, "new subscribe must create fresh MxAccess subscription");
-            fake.SubscribeCalls[1].Should().Contain("Motor.Speed");
-        }
-
-        [Fact]
-        public async Task SubscribeAfterUnsubscribe_SerializedByGate()
-        {
-            // Verifies FIX 1: subscribe and unsubscribe are serialized so they cannot
-            // interleave and cause the race condition.
-            var fake = new FakeScadaClient();
-            using var sm = new SubscriptionManager(fake);
-            using var cts = new CancellationTokenSource();
-
-            var tags = new[] { "Tag.A", "Tag.B", "Tag.C" };
-
-            // Subscribe, unsubscribe, re-subscribe in sequence
-            await sm.SubscribeAsync("session1", tags, cts.Token);
-            sm.UnsubscribeClient("session1");
-            await sm.SubscribeAsync("session2", tags, cts.Token);
-
-            // Both subscribes should have called SubscribeAsync on the scada client
-            fake.SubscribeCalls.Should().HaveCount(2);
-            // The unsubscribe in between should have cleaned up
-            fake.UnsubscribeCalls.Should().HaveCount(1);
-
-            // Data should flow to the new session
-            var reader = await sm.SubscribeAsync("session3", tags, cts.Token);
-            sm.OnTagValueChanged("Tag.A", Vtq.Good(1.0));
-            var result = await reader.ReadAsync(cts.Token);
-            result.vtq.Value.Should().Be(1.0);
-        }
-
-        [Fact]
-        public async Task OnTagValueChanged_NoDuplicateDelivery()
-        {
-            // Verifies FIX 2: each OnDataChange produces exactly one VTQ per client,
-            // not two (which happened when both stored callback and OnTagValueChanged
-            // property were invoked).
-            var fake = new FakeScadaClient();
-            using var sm = new SubscriptionManager(fake);
-            using var cts = new CancellationTokenSource();
-
-            var reader = await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
-
-            // Deliver one update
-            sm.OnTagValueChanged("Motor.Speed", Vtq.Good(42.0));
-
-            // Should receive exactly one message
-            reader.TryRead(out var msg).Should().BeTrue();
-            msg.vtq.Value.Should().Be(42.0);
-
-            // No duplicate
-            reader.TryRead(out _).Should().BeFalse("each update should be delivered exactly once");
-        }
-
-        [Fact]
-        public async Task FailedSubscription_StoredAsPending_RetriedOnReconnect()
-        {
-            // Verifies FIX 3: when MxAccess is down during subscribe, tags are stored
-            // as pending and retried when NotifyReconnection is called.
-            var fake = new FakeScadaClient();
-            fake.FailSubscriptions = true;
-            using var sm = new SubscriptionManager(fake);
-            using var cts = new CancellationTokenSource();
-
-            // Subscribe while MxAccess is "down" — should not throw (errors are logged)
-            var reader = await sm.SubscribeAsync("client1", new[] { "Motor.Speed" }, cts.Token);
-            reader.Should().NotBeNull();
-            fake.SubscribeCalls.Should().HaveCount(1);
-
-            // MxAccess comes back up
-            fake.FailSubscriptions = false;
-            sm.NotifyReconnection();
-
-            // Give the async retry a moment to complete
-            await Task.Delay(100);
-
-            // Should have retried the subscription
-            fake.SubscribeCalls.Should().HaveCount(2, "pending subscriptions should be retried on reconnect");
-            fake.SubscribeCalls[1].Should().Contain("Motor.Speed");
-        }
     }
 }