diff --git a/docs/requirements/Component-DataConnectionLayer.md b/docs/requirements/Component-DataConnectionLayer.md index 15f2966c..e203e4b1 100644 --- a/docs/requirements/Component-DataConnectionLayer.md +++ b/docs/requirements/Component-DataConnectionLayer.md @@ -402,7 +402,7 @@ The DCL manages connection lifecycle automatically: 3. **Connection state transitions**: The DCL tracks each connection's state as `connected`, `disconnected`, or `reconnecting`. All transitions are logged to Site Event Logging (event type `connection`): the initial connect (`Connection established on … endpoint`, Info — added by review 2026-10-21, it previously logged nothing), connection lost / reconfigured (`Warning` / `Info`), restored on reconnect (`Info`) and failover (`Warning`). 4. **Transparent re-subscribe**: On successful reconnection, the DCL automatically re-establishes all previously active subscriptions for that connection. Instance Actors require no action — they simply see quality return to `good` as fresh values arrive from restored subscriptions. 5. **Clean client replacement on (re)connect**: On every (re)connect the adapter detaches and disposes the previous protocol client (session, keep-alive timers, event loop) before creating a new one — a stale client can neither leak nor signal a disconnect against the new session. -6. **Session generation on same-adapter reconnect** (review 2026-10-21, finding C1): a plain drop + reconnect reuses the adapter object, so `SwapAdapter`'s generation bump never runs. On a successful reconnect the actor therefore advances `_adapterGeneration` itself — **before** `ReSubscribeAll` — and re-hooks the adapter's `Disconnected` handler for the new generation (new handler hooked before the old one is removed, so a disconnect in between is never lost). Every subscribe/alarm completion, value callback and seed started against the dead session then takes the existing stale path. That path releases a stale completion's handles against the adapter that created them, **except** when that adapter is still the current one: the handles died with the dead session, and releasing by handle against the adapter's NEW session could tear down a live item. Before the fix a dead-session initial-subscribe completion (in flight up to `SeedOverallTimeout`) was applied against the new session — parking a dead handle that made the live re-subscribe row look redundant and get released (the tag read resolved and stayed silent), or releasing a handle number the new MxGateway session had reused, and a connection-level failure row in it knocked the fresh link back into Reconnecting. +6. **Session generation on same-adapter reconnect** (review 2026-10-21, finding C1): a plain drop + reconnect reuses the adapter object, so `SwapAdapter`'s generation bump never runs. Each **reconnect attempt** therefore advances `_adapterGeneration` itself — before its `ConnectAsync` is issued — and re-hooks the adapter's `Disconnected` handler for the new generation (new handler hooked before the old one is removed). The attempt's `ConnectResult`, the new session's `Disconnected` and the `ReSubscribeAll` that follows a success all share that generation, so a disconnect queued right behind a successful result is still acted on (advancing the generation only when the result was applied would have dropped it as stale and wedged the actor Connected on a dead session — the adapter raises `Disconnected` once per session). The initial connect does not advance it: no session has existed yet. Every subscribe/alarm completion, value callback and seed started against the dead session then takes the existing stale path. That path releases a stale completion's handles against the adapter that created them, **except** when that adapter is still the current one: the handles died with the dead session, and releasing by handle against the adapter's NEW session could tear down a live item. Before the fix a dead-session initial-subscribe completion (in flight up to `SeedOverallTimeout`) was applied against the new session — parking a dead handle that made the live re-subscribe row look redundant and get released (the tag read resolved and stayed silent), or releasing a handle number the new MxGateway session had reused, and a connection-level failure row in it knocked the fresh link back into Reconnecting. 7. **Session-scoped MxGateway subscription ids** (findings C1 + L1): gateway item handles are plain ints that restart per session, so `MxGatewayDataConnection` hands out ids of the form `"{session}:{handle}"` and only ever sends a handle to the gateway when its session is the live one — a dead session's id is dropped silently instead of unadvising whatever the new session gave that number. The adapter's routing maps are cleared on every `ConnectAsync`, `DisconnectAsync` and `DisposeAsync` (they previously kept every dead session's entries, growing per flap), and a subscribe completing after a reconnect does not repopulate them. An alarm unsubscribe for an id the adapter does not know no longer decrements the shared alarm stream's reference count. ### Disconnect Detection Pattern diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs index 54fd1376..0cc79f85 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs @@ -269,10 +269,12 @@ public class DataConnectionActor : UntypedActor, IWithTimers /// /// /// - /// Residual (accepted, and in the safe direction): a disconnect raised by the PREVIOUS - /// session but delivered after the next attempt has already cleared the flag is attributed to - /// the new attempt, costing one extra reconnect cycle. Being wedged Connected on a dead - /// adapter is the failure this trades against. + /// Each attempt advances the session generation before it is issued, so a disconnect the + /// PREVIOUS session already raised (stamped with its generation) is dropped as stale rather + /// than attributed to the new attempt. Residual (accepted, and in the safe direction): a + /// previous-session disconnect raised only AFTER the next attempt re-hooked the handler is + /// attributed to the new attempt, costing one extra reconnect cycle. Being wedged Connected + /// on a dead adapter is the failure this trades against. /// /// private bool _disconnectDuringConnectAttempt; @@ -408,7 +410,7 @@ public class DataConnectionActor : UntypedActor, IWithTimers switch (message) { case AttemptConnect: - HandleAttemptConnect(); + HandleAttemptConnect(advanceSession: false); break; case ConnectResult result: HandleConnectResult(result); @@ -736,6 +738,9 @@ public class DataConnectionActor : UntypedActor, IWithTimers /// /// Advances for a new SESSION on the SAME adapter — a plain /// drop + reconnect, where never runs (review 2026-10-21 02/C1). + /// Called by before each RECONNECT attempt is issued, so the + /// attempt's result, the new session's Disconnected and everything started after it + /// share one generation, and anything started against an earlier session is stale. /// /// /// Without it, an initial subscribe issued against the dead session (in flight for up to @@ -751,9 +756,9 @@ public class DataConnectionActor : UntypedActor, IWithTimers /// /// The Disconnected 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. + /// removed, so no window exists with no handler at all. Because this runs before the + /// attempt's ConnectAsync, the only disconnects that can carry the OLD stamp belong + /// to the previous (already-dead) session and are correctly dropped as stale. /// /// private void AdvanceSessionGeneration() @@ -852,7 +857,7 @@ public class DataConnectionActor : UntypedActor, IWithTimers switch (message) { case AttemptConnect: - HandleAttemptConnect(); + HandleAttemptConnect(advanceSession: true); break; case ConnectResult result: HandleReconnectResult(result); @@ -944,13 +949,27 @@ public class DataConnectionActor : UntypedActor, IWithTimers // ── Connection Management ── - private void HandleAttemptConnect() + /// + /// True from Reconnecting: a session has existed on this adapter, so the attempt opens a NEW + /// one and gets its own generation (see ). False from + /// the initial Connecting state, where no session has ever been established and nothing is + /// in flight against one, so the generation the handler and result share is already right. + /// + private void HandleAttemptConnect(bool advanceSession) { _log.Debug("[{0}] Attempting connection...", _connectionName); // Scope the pending-disconnect flag to THIS attempt. Anything noted before now // belongs to a session that is already over and must not be replayed against the one // about to start. _disconnectDuringConnectAttempt = false; + // Every reconnect attempt opens a NEW session, so it gets its own generation (review 2026-10-21 + // 02/C1), advanced — and the Disconnected handler re-hooked — BEFORE ConnectAsync is + // issued. The new session's Disconnected therefore always carries the same generation + // as its ConnectResult: a disconnect that lands in the mailbox right behind a successful + // result is acted on, not dropped as stale (which would wedge the actor Connected on a + // dead session — the adapter raises Disconnected only once per session). + if (advanceSession) + AdvanceSessionGeneration(); var self = Self; // Stamp the generation this attempt belongs to, exactly as every other piped // completion does. A failover or an in-place reconfigure landing while ConnectAsync @@ -1146,12 +1165,9 @@ 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(); - + // The session generation was already advanced when this attempt was issued + // (HandleAttemptConnect), so ReSubscribeAll below runs on the NEW session's + // generation and every completion from the dead one takes the stale path. // Transparent re-subscribe — re-establish all active subscriptions ReSubscribeAll(); // Re-establish native alarm feeds (source replays a snapshot). diff --git a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionSameAdapterReconnectTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionSameAdapterReconnectTests.cs index 84808ada..56c72b86 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionSameAdapterReconnectTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionSameAdapterReconnectTests.cs @@ -187,6 +187,58 @@ public class DataConnectionSameAdapterReconnectTests : TestKit Assert.DoesNotContain("live-2", adapter.ReleasedIds); } + /// + /// The connect-result/disconnect race (adversarial review of the C1 fix). The (re)connect + /// succeeds and its ConnectResult is enqueued; before the actor processes it the new + /// session dies, so the adapter's once-only Disconnected lands in the mailbox AFTER the + /// result. That disconnect belongs to the session the result describes and must drive the + /// actor back to Reconnecting — if it were stamped with a generation the result's processing + /// then advanced past, it would be dropped as stale and the actor would sit Connected on a + /// dead session that never raises Disconnected again. + /// + /// False: the initial connect; true: a same-adapter reconnect. + [Theory] + [InlineData(false)] + [InlineData(true)] + public void DisconnectQueuedBehindASuccessfulConnectResult_IsNotLost(bool reconnect) + { + var name = reconnect ? "race-reconnect" : "race-initial"; + var adapter = new GatedAdapter(); + var connectGate = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + adapter.NextConnectGate = connectGate.Task; + var actor = CreateConnectionActor(name, adapter); + var expectedConnectCalls = 1; + + if (reconnect) + { + connectGate.SetResult(); + AwaitConnectedCount(name, 1); + connectGate = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + adapter.NextConnectGate = connectGate.Task; + adapter.RaiseDisconnected(); + expectedConnectCalls = 2; + } + + AwaitCondition(() => Volatile.Read(ref adapter.ConnectCalls) == expectedConnectCalls, TimeSpan.FromSeconds(5)); + var disconnectsBefore = HealthTransitions(name, ConnectionHealth.Disconnected); + + // Hold the actor inside a health-report read … + using var hold = new ManualResetEventSlim(false); + adapter.HoldStatus = hold; + actor.Tell(new DataConnectionActor.GetHealthReport()); + Assert.True(adapter.StatusEntered.Wait(TimeSpan.FromSeconds(5))); + + // … while the connect succeeds (its result queues behind the held message) … + connectGate.SetResult(); + Thread.Sleep(300); + // … and the new session dies at once, queuing its disconnect behind the result. + adapter.RaiseDisconnected(); + hold.Set(); + + AwaitCondition(() => HealthTransitions(name, ConnectionHealth.Disconnected) > disconnectsBefore, + TimeSpan.FromSeconds(5), TimeSpan.FromMilliseconds(20)); + } + /// One gated SubscribeBatchAsync call. internal sealed class PendingBatch { @@ -219,7 +271,33 @@ public class DataConnectionSameAdapterReconnectTests : TestKit public readonly ConcurrentQueue ReleasedIds = new(); - public ConnectionHealth Status { get; private set; } = ConnectionHealth.Disconnected; + private ConnectionHealth _status = ConnectionHealth.Disconnected; + + /// When set, the NEXT read of signals and blocks on it — a latch that holds the actor mid-message. + public ManualResetEventSlim? HoldStatus; + + /// Set once a held read has started. + public readonly ManualResetEventSlim StatusEntered = new(false); + + /// When set, the next returns this task instead of completing at once. + public Task? NextConnectGate; + + /// Number of calls so far. + public int ConnectCalls; + + public ConnectionHealth Status + { + get + { + if (Interlocked.Exchange(ref HoldStatus, null) is { } hold) + { + StatusEntered.Set(); + hold.Wait(TimeSpan.FromSeconds(10)); + } + return _status; + } + private set => _status = value; + } public event Action? Disconnected; @@ -244,8 +322,9 @@ public class DataConnectionSameAdapterReconnectTests : TestKit public Task ConnectAsync(IDictionary connectionDetails, CancellationToken cancellationToken = default) { + Interlocked.Increment(ref ConnectCalls); Status = ConnectionHealth.Connected; - return System.Threading.Tasks.Task.CompletedTask; + return Interlocked.Exchange(ref NextConnectGate, null) ?? System.Threading.Tasks.Task.CompletedTask; } public Task DisconnectAsync(CancellationToken cancellationToken = default) =>