fix(dcl): advance the session generation on same-adapter reconnect and session-scope MxGateway subscription ids (review 2026-10-21 02/C1, 02/L1)

2026-10-22 06:23:06 -04:00
parent b811a2451b
commit 8c1bd2c3fc
7 changed files with 584 additions and 15 deletions
@@ -710,6 +710,38 @@ public class DataConnectionActor : UntypedActor, IWithTimers
        HookAdapterDisconnected();
    }

+    /// <summary>
+    /// Advances <see cref="_adapterGeneration"/> for a new SESSION on the SAME adapter — a plain
+    /// drop + reconnect, where <see cref="SwapAdapter"/> never runs (review 2026-10-21 02/C1).
+    ///
+    /// <para>
+    /// Without it, an initial subscribe issued against the dead session (in flight for up to
+    /// <see cref="DataConnectionOptions.SeedOverallTimeout"/>) carried the current generation and
+    /// was applied against the new session: landing before the re-subscribe row for its tag it
+    /// parked a dead handle under the tag's key, so the LIVE row looked redundant and was
+    /// released (the tag reads resolved and is silent until the next reconnect); landing after,
+    /// its dead handle was released against the adapter whose new session may have reused that
+    /// handle number for another tag (MxGateway); and a connection-level failure row in it
+    /// knocked the freshly reconnected link back into Reconnecting.
+    /// </para>
+    ///
+    /// <para>
+    /// The <c>Disconnected</c> handler stamps the generation it was hooked with, so it is
+    /// re-hooked for the new generation. The new handler is hooked BEFORE the old one is
+    /// removed: a disconnect raised in between is then delivered twice (old stamp dropped as
+    /// stale, new stamp acted on) rather than lost, which would wedge the actor Connected on a
+    /// dead session.
+    /// </para>
+    /// </summary>
+    private void AdvanceSessionGeneration()
+    {
+        var previousHandler = _disconnectedHandler;
+        _adapterGeneration++;
+        HookAdapterDisconnected();
+        if (previousHandler is not null)
+            _adapter.Disconnected -= previousHandler;
+    }
+
    /// <summary>
    /// Applies a new configuration to this LIVE connection <b>in place</b> (deferred register
    /// row 97) — the non-destructive alternative to stopping and re-creating the connection
@@ -1076,6 +1108,12 @@ public class DataConnectionActor : UntypedActor, IWithTimers
                    $"Connection restored on {_activeEndpoint} endpoint", null);
            }

+            // The adapter object survived the drop but its SESSION did not. Advance the
+            // generation BEFORE re-subscribing so every completion, value and alarm callback
+            // started against the dead session takes the existing stale path (review
+            // 2026-10-21 02/C1) — exactly as it would after a swap.
+            AdvanceSessionGeneration();
+
            // Transparent re-subscribe — re-establish all active subscriptions
            ReSubscribeAll();
            // Re-establish native alarm feeds (source replays a snapshot).
@@ -1405,8 +1443,19 @@ public class DataConnectionActor : UntypedActor, IWithTimers
    /// The outgoing adapter is disposed fire-and-forget by <see cref="SwapAdapter"/> and may
    /// already be gone, so a failure here is expected and logged at Debug rather than surfaced.
    /// </summary>
-    private void ReleaseStaleAgainstOwnAdapter(string description, Func<Task> release)
+    private void ReleaseStaleAgainstOwnAdapter(object owner, string description, Func<Task> release)
    {
+        // Same-adapter reconnect (review 2026-10-21 02/C1): the generation advanced because the
+        // SESSION changed, not the adapter. The dead session's handles died with it, and the
+        // adapter now speaks for the NEW session — where a session-scoped handle number (an
+        // MxGateway int) may already belong to a live item. Releasing would tear that down.
+        if (ReferenceEquals(owner, _adapter))
+        {
+            _log.Debug("[{0}] Not releasing {1}: it belongs to a session of the current adapter that "
+                + "has since ended, so it died with that session", _connectionName, description);
+            return;
+        }
+
        try
        {
            release().ContinueWith(
@@ -1618,6 +1667,7 @@ public class DataConnectionActor : UntypedActor, IWithTimers
            // MxGateway's are plain ints that collide across sessions, so releasing by handle
            // against the replacement adapter can tear down a live subscription.
            ReleaseStaleAgainstOwnAdapter(
+                msg.Adapter,
                $"{staleIds.Count} tag subscription handle(s) from adapter generation {msg.AdapterGeneration}",
                () => UnsubscribeIdsAsync(msg.Adapter, staleIds));

@@ -3078,6 +3128,7 @@ public class DataConnectionActor : UntypedActor, IWithTimers
            // one (see ReleaseStaleAgainstOwnAdapter).
            if (msg is { Success: true, SubscriptionId: not null })
                ReleaseStaleAgainstOwnAdapter(
+                    msg.Adapter,
                    $"the alarm feed for {msg.SourceReference} from adapter generation {msg.AdapterGeneration}",
                    () => msg.Adapter.UnsubscribeAlarmsAsync(msg.SubscriptionId));

@@ -52,9 +52,16 @@ public class MxGatewayDataConnection
    // subscriptionId → (tagPath, callback) so the event loop can route updates by tag,
    // plus tagPath → subscriptionId for reverse lookup. Concurrent because the event
    // loop reads from a background thread while Subscribe/Unsubscribe mutate.
+    // Keys are SESSION-SCOPED ids ("{session}:{handle}") — see ScopeId.
    private readonly ConcurrentDictionary<string, (string TagPath, SubscriptionCallback Callback)> _subs = new();
    private readonly ConcurrentDictionary<string, string> _tagToSub = new();

+    // Incremented by every ConnectAsync (each opens a NEW gateway session). Subscription ids
+    // handed out carry the session they were minted in, because gateway item handles are plain
+    // ints that restart — i.e. collide — per session (review 2026-10-21 02/C1): a dead
+    // session's id must never parse to a live handle and unadvise someone else's item.
+    private int _session;
+
    // DataConnectionLayer mirror of OpcUaDataConnection's once-only guard: an int toggled
    // with Interlocked.Exchange so only the first caller raises Disconnected.
    // 0 = not fired, 1 = fired. Reset on (re)connect.
@@ -158,6 +165,12 @@ public class MxGatewayDataConnection
            _alarmStreamPrefix = string.Empty;
        }

+        // New session: the previous session's items died with it. Drop their routing entries
+        // (review 2026-10-21 02/L1 — they were never removed and grew per flap) and advance the
+        // session so their ids are recognised as dead from now on.
+        Interlocked.Increment(ref _session);
+        ClearSubscriptionMaps();
+
        Interlocked.Exchange(ref _disconnectFired, 0); // reset guard on (re)connect, like OPC UA
        _client = _clientFactory.Create();

@@ -240,10 +253,46 @@ public class MxGatewayDataConnection
        }
    }

+    /// <summary>Drops every subscription routing entry (the session they belong to is gone).</summary>
+    private void ClearSubscriptionMaps()
+    {
+        _subs.Clear();
+        _tagToSub.Clear();
+    }
+
+    /// <summary>Formats a gateway item handle as a subscription id scoped to <paramref name="session"/>.</summary>
+    private static string ScopeId(int session, string handle) => $"{session}:{handle}";
+
+    /// <summary>
+    /// Extracts the raw gateway handle from a session-scoped id, or returns <c>null</c> when the
+    /// id was minted by a session other than the live one (or is not one of ours) — such a handle
+    /// must never be sent to the gateway.
+    /// </summary>
+    private string? CurrentSessionHandle(string subscriptionId)
+    {
+        var sep = subscriptionId.IndexOf(':');
+        if (sep <= 0
+            || !int.TryParse(subscriptionId.AsSpan(0, sep), System.Globalization.NumberStyles.Integer,
+                System.Globalization.CultureInfo.InvariantCulture, out var session)
+            || session != Volatile.Read(ref _session))
+            return null;
+        return subscriptionId[(sep + 1)..];
+    }
+
+    /// <summary>Removes an id's routing entries; returns true if it was a known subscription.</summary>
+    private bool ForgetSubscription(string subscriptionId)
+    {
+        if (!_subs.TryRemove(subscriptionId, out var s))
+            return false;
+        ((ICollection<KeyValuePair<string, string>>)_tagToSub).Remove(new(s.TagPath, subscriptionId));
+        return true;
+    }
+
    /// <inheritdoc />
    public async Task DisconnectAsync(CancellationToken cancellationToken = default)
    {
        _eventLoopCts?.Cancel();
+        ClearSubscriptionMaps();
        lock (_alarmLock)
        {
            _alarmCts?.Cancel();
@@ -261,18 +310,26 @@ public class MxGatewayDataConnection
    /// <inheritdoc />
    public async Task<string> SubscribeAsync(string tagPath, SubscriptionCallback callback, CancellationToken cancellationToken = default)
    {
-        var subId = await _client!.SubscribeAsync(tagPath, cancellationToken);
-        _subs[subId] = (tagPath, callback);
-        _tagToSub[tagPath] = subId;
+        // Capture the session with the client it belongs to: a reconnect landing while this
+        // call is in flight must not route the dead session's handle into the new maps.
+        var session = Volatile.Read(ref _session);
+        var handle = await _client!.SubscribeAsync(tagPath, cancellationToken);
+        var subId = ScopeId(session, handle);
+        if (session == Volatile.Read(ref _session))
+        {
+            _subs[subId] = (tagPath, callback);
+            _tagToSub[tagPath] = subId;
+        }
        return subId;
    }

    /// <inheritdoc />
    public async Task UnsubscribeAsync(string subscriptionId, CancellationToken cancellationToken = default)
    {
-        if (_subs.TryRemove(subscriptionId, out var s))
-            ((ICollection<KeyValuePair<string, string>>)_tagToSub).Remove(new(s.TagPath, subscriptionId));
-        await _client!.UnsubscribeAsync(subscriptionId, cancellationToken);
+        ForgetSubscription(subscriptionId);
+        // A dead session's handle is never sent: the gateway may have reused the number.
+        if (CurrentSessionHandle(subscriptionId) is { } handle)
+            await _client!.UnsubscribeAsync(handle, cancellationToken);
    }

    /// <inheritdoc />
@@ -281,18 +338,25 @@ public class MxGatewayDataConnection
    {
        // ONE gateway round trip per batch (SubscribeBulk = AddItem + Advise per tag in a
        // single worker pass) instead of the historical 2 RPCs per tag.
+        var session = Volatile.Read(ref _session);
        var outcomes = await _client!.SubscribeBulkAsync(tagPaths, cancellationToken);
+        var current = session == Volatile.Read(ref _session);
        var results = new List<TagSubscribeResult>(outcomes.Count);
        foreach (var outcome in outcomes)
        {
+            string? subId = null;
            if (outcome is { Success: true, SubscriptionId: not null })
            {
-                _subs[outcome.SubscriptionId] = (outcome.TagPath, callback);
-                _tagToSub[outcome.TagPath] = outcome.SubscriptionId;
+                subId = ScopeId(session, outcome.SubscriptionId);
+                if (current)
+                {
+                    _subs[subId] = (outcome.TagPath, callback);
+                    _tagToSub[outcome.TagPath] = subId;
+                }
            }

            results.Add(new TagSubscribeResult(
-                outcome.TagPath, outcome.Success, outcome.SubscriptionId, outcome.Error));
+                outcome.TagPath, outcome.Success, subId, outcome.Error));
        }

        return results;
@@ -302,13 +366,17 @@ public class MxGatewayDataConnection
    public async Task UnsubscribeBatchAsync(
        IReadOnlyList<string> subscriptionIds, CancellationToken cancellationToken = default)
    {
+        var handles = new List<string>(subscriptionIds.Count);
        foreach (var id in subscriptionIds)
        {
-            if (_subs.TryRemove(id, out var s))
-                ((ICollection<KeyValuePair<string, string>>)_tagToSub).Remove(new(s.TagPath, id));
+            ForgetSubscription(id);
+            // A dead session's handle is never sent: the gateway may have reused the number.
+            if (CurrentSessionHandle(id) is { } handle)
+                handles.Add(handle);
        }

-        await _client!.UnsubscribeBulkAsync(subscriptionIds, cancellationToken);
+        if (handles.Count > 0)
+            await _client!.UnsubscribeBulkAsync(handles, cancellationToken);
    }

    /// <inheritdoc />
@@ -371,7 +439,11 @@ public class MxGatewayDataConnection
    {
        lock (_alarmLock)
        {
-            _alarmSubSources.Remove(subscriptionId);
+            // An id this adapter does not know (a dead session's — ConnectAsync resets the
+            // registry) must not decrement the live count: reaching zero tears down the shared
+            // stream every live subscriber depends on.
+            if (!_alarmSubSources.Remove(subscriptionId))
+                return Task.CompletedTask;
            if (_alarmSubCount > 0)
                _alarmSubCount--;
            // Deliberately no stream restart here: dropping a source can only leave the
@@ -444,6 +516,7 @@ public class MxGatewayDataConnection
    public async ValueTask DisposeAsync()
    {
        _eventLoopCts?.Cancel();
+        ClearSubscriptionMaps();
        // The DataConnectionActor disposes adapters
        // fire-and-forget on failover/stop without necessarily calling
        // DisconnectAsync first, so tear down the alarm stream here too — otherwise