dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(dcl): P2-5 seed probe-resolved tags; P2-6 close the Connecting/Reconnecting AdapterDisconnected gap

Browse Source 
P2-5 — the tag-resolution PROBE was the only subscribe path that did not seed.
HandleSubscribe carries seeds back on SubscribeCompleted and ReSubscribeAll re-seeds
after a reconnect, because a fresh advise only yields a value on the source's next
CHANGE. HandleRetryTagResolution did neither, so a tag that resolved LATE (device still
booting, item created after deployment) and then never changed had no value path at all.
OPC UA masks it with the initial data-change notification on subscription creation; an
MxGateway advise can legitimately return VT_EMPTY, so a static tag stayed Uncertain for
the life of the connection. New SeedNewlyResolvedTags reads the round's newly resolved
tags and delivers them through the normal generation-guarded TagValueReceived path —
directly rather than on the completion message (the probe only covers tags already
registered in _instancesByTag, so the initial subscribe's ordering hazard does not
apply), and as its own fire-and-forget task so the seed reads never delay the
BatchSubscribeCompleted that re-arms the retry timer.

P2-6 — only the Connected state handled AdapterDisconnected; in Connecting and
Reconnecting it fell through to Unhandled. A connect that succeeds and drops again before
its piped ConnectResult is applied therefore delivered the disconnect FIRST, where it was
discarded, and ConnectResult(true) then declared the connection up on a dead adapter. An
adapter raises Disconnected at most once per successful connect (OpcUaDataConnection's
once-only _disconnectFired, reset only by the next successful ConnectAsync), so a
connection with no subscriptions — nothing to fail at connection level and drive the
reconnect machine — sat Connected on a dead adapter until the process restarted. Both
states now record the disconnect in _disconnectDuringConnectAttempt (cleared at the top of
HandleAttemptConnect, so it only ever refers to the attempt in flight) and both result
handlers demote such a "success" to the ordinary connect-FAILURE tail — counting toward
the failover budget and re-arming the retry timer. A flag rather than Stash: the stash has
no selective drop, so a stashed disconnect would be replayed after the next successful
connect and cost a spurious reconnect cycle.

AdapterDisconnected is now stamped with the adapter generation (hooked via a
per-adapter closure instead of a method group; SwapAdapter hooks AFTER the bump), and
IsStaleDisconnect drops a notification from a swapped-out adapter in all three states —
the disconnect twin of IsStaleConnectResult. SwapAdapter's unhook cannot stop an event
already executing on a background thread, so without the fence an endpoint the operator
has just reconfigured AWAY from could knock the healthy replacement into a reconnect.

Tests: DataConnectionProbeSeedTests + DataConnectionDisconnectRaceTests (4 new). The
race is reproduced by ORDERING, not timing — the connect is held on a gate, Disconnected
is raised while the actor sits idle, and a GetHealthReport round trip is the mailbox
barrier; no sleeps. All 4 were proved to fail against the pre-fix behaviour via a
targeted revert of the four behaviour sites (the stale-generation test cannot be proved
by a plain revert — the record carried no generation before this change, so the test
would not compile; noted in its doc comment). Full DCL suite 346/346 green.
This commit is contained in:
Joseph Doherty
2026-08-19 16:24:30 -04:00
parent 65b3c23239
commit 6823e7e3fa
3 changed files with 612 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs
+264 -11
View File
@@ -234,6 +234,51 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
/// </summary>
private IActorRef _self = null!;
/// <summary>
/// The delegate currently hooked to <c>_adapter.Disconnected</c>. It closes over the
/// <see cref="_adapterGeneration"/> in force when it was hooked, so the resulting
/// <see cref="AdapterDisconnected"/> carries the generation of the adapter that raised it —
/// a method group could not. Kept in a field so <see cref="SwapAdapter"/> and
/// <see cref="PostStop"/> unhook exactly the instance they hooked.
/// </summary>
private Action? _disconnectedHandler;
/// <summary>
/// Arch-review 2026-08-19 <b>P2-6</b>. Set when an <see cref="AdapterDisconnected"/> for the
/// CURRENT adapter generation is received while the actor is still Connecting/Reconnecting —
/// i.e. the adapter connected and dropped again before the piped
/// <see cref="ConnectResult"/> for that attempt reached the mailbox.
///
/// <para>
/// Neither state used to handle the message at all, so it fell through to
/// <c>Unhandled</c> and the subsequent <c>ConnectResult(true)</c> declared the connection up
/// on an adapter that was already dead. Nothing recovers from there: an adapter fires
/// <c>Disconnected</c> at most once per successful connect (<c>OpcUaDataConnection</c>'s
/// once-only <c>_disconnectFired</c> guard, reset only by the next successful
/// <c>ConnectAsync</c>), so a connection with zero subscriptions — nothing to fail at
/// connection level and drive the reconnect machine — sat Connected on a dead adapter until
/// the process restarted.
/// </para>
///
/// <para>
/// It is cleared at the top of <see cref="HandleAttemptConnect"/>, so it only ever refers to
/// the attempt currently in flight: a disconnect belonging to a session that ended BEFORE
/// this attempt began (e.g. the adapter's <c>Disconnected</c> trailing a connection-level
/// subscribe failure that already drove the actor into Reconnecting) is discarded rather than
/// replayed against the fresh session. That is also why the flag is used instead of
/// <c>Stash</c>: the stash has no selective drop, so a stashed disconnect would be replayed
/// after the next successful connect and cost a spurious reconnect cycle.
/// </para>
///
/// <para>
/// 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.
/// </para>
/// </summary>
private bool _disconnectDuringConnectAttempt;
/// <summary>
/// Initializes the data connection actor with its adapter and configuration.
/// </summary>
@@ -283,23 +328,43 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
_self = Self;
// Listen for unexpected adapter disconnections
_adapter.Disconnected += OnAdapterDisconnected;
HookAdapterDisconnected();
BecomeConnecting();
}
private void OnAdapterDisconnected()
/// <summary>
/// Subscribes to the CURRENT adapter's <c>Disconnected</c> event with a delegate that closes
/// over the generation in force right now, so the marshalled
/// <see cref="AdapterDisconnected"/> identifies the adapter that raised it. Callers must
/// have bumped <see cref="_adapterGeneration"/> for the new adapter first
/// (see <see cref="SwapAdapter"/>).
/// </summary>
private void HookAdapterDisconnected()
{
// Marshal the event onto the actor's message loop using captured _self reference.
// This runs on a background thread (gRPC stream reader), so Self would throw.
_self.Tell(new AdapterDisconnected());
var generation = _adapterGeneration;
var self = _self;
// Marshal the event onto the actor's message loop using the captured _self reference.
// This runs on a background thread (keep-alive / gRPC stream reader), so Self would throw.
_disconnectedHandler = () => self.Tell(new AdapterDisconnected(generation));
_adapter.Disconnected += _disconnectedHandler;
}
/// <summary>Unhooks the delegate installed by <see cref="HookAdapterDisconnected"/>, if any.</summary>
private void UnhookAdapterDisconnected()
{
if (_disconnectedHandler is null)
return;
_adapter.Disconnected -= _disconnectedHandler;
_disconnectedHandler = null;
}
/// <inheritdoc />
protected override void PostStop()
{
_log.Info("DataConnectionActor [{0}] stopping — disposing adapter", _connectionName);
_adapter.Disconnected -= OnAdapterDisconnected;
UnhookAdapterDisconnected();
// S9: fire-and-forget, but observe a faulted dispose so a failing adapter
// teardown is logged rather than swallowed into an unobserved task exception.
_adapter.DisposeAsync().AsTask().ContinueWith(
@@ -340,6 +405,14 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
case ConnectResult result:
HandleConnectResult(result);
break;
case AdapterDisconnected disconnected:
// P2-6 — the adapter connected and dropped again before this attempt's
// ConnectResult was applied. Remember it (see _disconnectDuringConnectAttempt)
// rather than acting now: acting would put the actor into Reconnecting and the
// in-flight ConnectResult(true) would then be applied THERE, re-subscribing and
// declaring the same dead adapter Connected anyway.
NotePendingDisconnect(disconnected);
break;
case SubscribeTagsRequest:
case WriteTagRequest:
case WriteTagBatchRequest:
@@ -467,8 +540,13 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
case QualityFlushTick:
FlushQualityCountersIfDirty();
break;
case AdapterDisconnected:
HandleDisconnect();
case AdapterDisconnected disconnected:
// P2-6 — fence the disconnect on its adapter generation. The outgoing adapter's
// event can fire concurrently with SwapAdapter's unhook, so a Disconnected from
// the endpoint we have just failed over (or reconfigured) AWAY from could arrive
// here and knock the healthy replacement back into a reconnect cycle.
if (!IsStaleDisconnect(disconnected))
HandleDisconnect();
break;
case ReconfigureConnectionCommand rc:
HandleReconfigure(rc);
@@ -600,14 +678,17 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
/// </summary>
private void SwapAdapter(IDictionary<string, string> newConfig)
{
_adapter.Disconnected -= OnAdapterDisconnected;
UnhookAdapterDisconnected();
_ = _adapter.DisposeAsync().AsTask();
_adapter = _factory.Create(_protocolType, newConfig);
_connectionDetails = newConfig;
_adapter.Disconnected += OnAdapterDisconnected;
_adapterGeneration++;
// Hooked AFTER the bump so the new adapter's handler stamps the NEW generation; the
// outgoing adapter's handler was captured with the old one and is now recognised as
// stale by IsStaleDisconnect even if it fires concurrently with the unhook above.
HookAdapterDisconnected();
}
/// <summary>
@@ -702,6 +783,12 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
case ConnectResult result:
HandleReconnectResult(result);
break;
case AdapterDisconnected disconnected:
// P2-6 — same gap as Connecting: a reconnect that succeeds and drops again before
// its ConnectResult is applied would otherwise be declared Connected on a dead
// adapter that will never raise Disconnected a second time.
NotePendingDisconnect(disconnected);
break;
case SubscribeTagsRequest:
case WriteTagRequest:
case WriteTagBatchRequest:
@@ -777,6 +864,10 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
private void HandleAttemptConnect()
{
_log.Debug("[{0}] Attempting connection...", _connectionName);
// P2-6 — 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;
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
@@ -827,6 +918,75 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
return true;
}
/// <summary>
/// True when this <see cref="AdapterDisconnected"/> was raised by an adapter that has since
/// been swapped out (failover or in-place reconfigure), in which case it is dropped — the
/// disconnect twin of <see cref="IsStaleConnectResult"/> (arch-review 2026-08-19 P2-6).
/// <see cref="SwapAdapter"/> unhooks the outgoing adapter, but the event can already be
/// executing on a background thread when it does, so the message can still be delivered
/// after the swap.
/// </summary>
private bool IsStaleDisconnect(AdapterDisconnected disconnected)
{
if (disconnected.AdapterGeneration == _adapterGeneration)
return false;
_log.Debug(
"[{0}] Dropping disconnect notification from adapter generation {1} (current {2}) — "
+ "the adapter that raised it has been replaced",
_connectionName, disconnected.AdapterGeneration, _adapterGeneration);
return true;
}
/// <summary>
/// Records a current-generation disconnect that arrived while a connect attempt was still in
/// flight, so <see cref="HandleConnectResult"/> / <see cref="HandleReconnectResult"/> can
/// refuse to declare the connection up. See <see cref="_disconnectDuringConnectAttempt"/>.
/// </summary>
private void NotePendingDisconnect(AdapterDisconnected disconnected)
{
if (IsStaleDisconnect(disconnected))
return;
_log.Warning(
"[{0}] Adapter reported a disconnect while a connect attempt was still in flight — "
+ "the attempt's result will be treated as a failed connection", _connectionName);
_disconnectDuringConnectAttempt = true;
}
/// <summary>
/// Reads and clears <see cref="_disconnectDuringConnectAttempt"/>, logging the demotion. True
/// means the successful connect result being applied belongs to a session the adapter has
/// already reported as dropped.
/// </summary>
private bool ConsumeDisconnectDuringConnectAttempt()
{
if (!_disconnectDuringConnectAttempt)
return false;
_disconnectDuringConnectAttempt = false;
_log.Warning(
"[{0}] Connect succeeded but the adapter had already reported the session lost — "
+ "treating the attempt as a failed connect", _connectionName);
return true;
}
/// <summary>
/// Applies a demoted connect success (see <see cref="ConsumeDisconnectDuringConnectAttempt"/>)
/// exactly as a connect FAILURE: the session the "success" describes is already gone, so it
/// must count toward the failover budget and re-arm the retry timer rather than declare the
/// connection up. Verbatim the tail of both result handlers' failure branches, which is the
/// point — a flapping endpoint that drops inside every connect window still reaches its
/// backup instead of retrying the same dead primary forever.
/// </summary>
private void DemoteConnectAttemptToFailure()
{
_consecutiveFailures++;
CountFailureAndMaybeFailover(
"adapter reported the session lost before the connect result was applied");
Timers.StartSingleTimer("reconnect", new AttemptConnect(), _options.ReconnectInterval);
}
private void HandleConnectResult(ConnectResult result)
{
if (IsStaleConnectResult(result))
@@ -834,6 +994,16 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
if (result.Success)
{
// P2-6 — a disconnect that raced ahead of this result means the session this
// "success" describes is already gone; going Connected here would strand the
// connection on a dead adapter that never raises Disconnected again. Stay in
// Connecting and retry, exactly as a failed connect would.
if (ConsumeDisconnectDuringConnectAttempt())
{
DemoteConnectAttemptToFailure();
return;
}
_log.Info("[{0}] Connection established", _connectionName);
BecomeConnected();
}
@@ -858,6 +1028,15 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
if (result.Success)
{
// P2-6 — same demotion as the initial connect: the reconnect landed but the adapter
// has already reported the new session lost, so re-subscribing and going Connected
// would wedge on a dead adapter. Stay in Reconnecting and retry.
if (ConsumeDisconnectDuringConnectAttempt())
{
DemoteConnectAttemptToFailure();
return;
}
_log.Info("[{0}] Reconnected successfully on {1} endpoint", _connectionName, _activeEndpoint);
_consecutiveFailures = 0;
@@ -2131,10 +2310,79 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
}));
}
// NOTE: `self` is the Self reference captured on the actor thread above — this runs on
// a background task, where Akka's Self property is invalid.
SeedNewlyResolvedTags(self, adapter, generation, rows);
return new BatchSubscribeCompleted(rows, generation, BatchSubscribeSource.ResolutionProbe);
}).PipeTo(self);
}
/// <summary>
/// Reads the current value of every tag this probe round has just resolved and delivers it
/// through the normal generation-guarded <see cref="TagValueReceived"/> path — the same seed
/// the initial subscribe (<see cref="HandleSubscribe"/>) and the reconnect re-subscribe
/// (<see cref="ReSubscribeAll"/>) already perform, which the resolution probe was missing
/// (arch-review 2026-08-19 <b>P2-5</b>).
///
/// <para>
/// A newly advised subscription only starts producing values on the source's next CHANGE.
/// OPC UA masks that with an initial data-change notification on subscription creation, but
/// an MxGateway advise can legitimately return <c>VT_EMPTY</c>, so a tag that resolved LATE
/// (device still booting, item created after deployment) and then never changes had no value
/// path at all — its attribute stayed Uncertain for the life of the connection, healed only
/// by an unrelated reconnect.
/// </para>
///
/// <para>
/// Unlike <see cref="HandleSubscribe"/>, the seeds are delivered directly rather than carried
/// back on the completion message: the probe only ever covers tags already present in
/// <see cref="_subscriptionsByInstance"/> / <see cref="_instancesByTag"/>, so the
/// registration-ordering hazard that forces the initial subscribe to defer does not exist
/// here. It runs as its own fire-and-forget task (mirroring the reconnect re-seed) so the
/// seed reads — bounded by <c>SeedOverallTimeout</c>, not by the subscribe — never delay the
/// <see cref="BatchSubscribeCompleted"/> that carries the round's resolution bookkeeping and
/// re-arms the retry timer. Values that arrive after a failover or a drop to Reconnecting are
/// discarded by the generation guard / state handler exactly like any other tag value.
/// </para>
/// </summary>
/// <param name="self">
/// The actor's own reference, captured on the actor thread by the caller — this method is
/// invoked from a background task, where Akka's <c>Self</c> property is invalid.
/// </param>
/// <param name="adapter">The adapter the probe round ran against.</param>
/// <param name="generation">The adapter generation the probe round was stamped with.</param>
/// <param name="rows">The probe round's per-tag subscribe results.</param>
private void SeedNewlyResolvedTags(
IActorRef self, IDataConnection adapter, int generation, IReadOnlyList<SubscribeTagResult> rows)
{
var resolved = new List<string>(rows.Count);
foreach (var row in rows)
{
if (row is { Success: true, SubscriptionId: not null })
resolved.Add(row.TagPath);
}
if (resolved.Count == 0)
return;
_ = Task.Run(async () =>
{
try
{
var seeds = await SeedTagsAsync(adapter, resolved);
foreach (var seed in seeds)
self.Tell(new TagValueReceived(seed.TagPath, seed.Value, generation));
}
catch (Exception ex)
{
// Fire-and-forget: guarantee a trace rather than an unobserved task fault.
// SeedTagsAsync already swallows per-read errors, so reaching here is unexpected.
_log.Warning("[{0}] Tag-resolution re-seed task faulted: {1}", _connectionName, ex.Message);
}
});
}
/// <summary>
/// Arms the single-shot tag-resolution retry timer at the current backoff interval
/// when unresolved tags remain, and cancels it when none do. Single-shot (rescheduled
@@ -2961,7 +3209,12 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers
/// rather than applied to its replacement (see <see cref="IsStaleConnectResult"/>).
/// </summary>
internal record ConnectResult(bool Success, string? Error, int AdapterGeneration);
internal record AdapterDisconnected;
/// <summary>
/// The adapter's <c>Disconnected</c> event marshalled onto the actor thread, stamped with the
/// <see cref="_adapterGeneration"/> the raising adapter was hooked at so a notification from a
/// swapped-out adapter can be dropped (<see cref="IsStaleDisconnect"/>).
/// </summary>
internal record AdapterDisconnected(int AdapterGeneration);
internal record TagValueReceived(string TagPath, TagValue Value, int AdapterGeneration);
internal record RetryTagResolution;