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

fix(dcl): generation-fence ConnectResult + roll back failed OPC UA subscribe batches (arch-review 2026-08-19 P2-3 + P2-4)

Browse Source 
P2-3. ConnectResult was the last piped completion with no adapter generation on
it, so a connect launched against generation N and answered after the adapter had
been swapped (failover, or the row-97 in-place reconfigure) was applied to its
REPLACEMENT. A stale success drove ReSubscribeAll + BecomeConnected against an
adapter that had never connected, and the replacement's own result then landed in
the Connected state where nothing handles it. With a bad new endpoint the batch
subscribes failed at connection level with a log line only, no AdapterDisconnected
ever fired, and the idempotent-reconfigure guard refused a re-send of the same
configuration — only a restart recovered the connection. A stale FAILURE was as
wrong the other way: it counted toward failover and re-armed the reconnect timer
underneath the attempt genuinely in flight.

HandleAttemptConnect now stamps the generation and both handlers open with the
same guard the subscribe leg got in c4d3c8b8, dropping a mismatched result whole.
The adapter instance is deliberately NOT carried: unlike a subscribe completion a
connect result holds no adapter-scoped handles to release, and SwapAdapter has
already disposed the outgoing adapter. Dropping cannot wedge the machine — every
generation bump (CountFailureAndMaybeFailover, BecomeReconnecting's failover
branch, HandleReconfigure through it) arms an AttemptConnect for the new
generation, and the state stays Connecting/Reconnecting until that answers.

P2-4. CreateSubscriptionsCoreAsync registers each monitored item as it places it
(_callbacks, _itemShard, shard budget, the SDK subscription's item list) and only
then issues ApplyChanges, so a throw there propagated with every registration
still in place and nothing holding a handle to release them. The actor reads the
throw as a whole-chunk fault and re-issues the SAME batch on its backoff timer, so
each probe grew the bookkeeping again. Placement + apply moved to
PlaceAndApplyAsync behind a try/catch that unwinds via the existing
ReleaseFailedItem (per-item try, so one failure neither stops the rest nor
replaces the original fault); no ApplyChanges is re-issued, and items a partial
apply did reach the server with are reclaimed when the shard and session are torn
down on reconnect.

That reconnect is the other half: IsConnectionLevelFailure knew nothing about
ServiceResultException, which is how the SDK reports a dead session — so a dead
session classified as a per-tag resolution failure for the whole chunk. It now
also matches a narrow set of session/channel status codes (BadSessionIdInvalid,
BadSessionClosed, BadSessionNotActivated, BadSecureChannelClosed,
BadSecureChannelIdInvalid, BadServerNotConnected, BadConnectionClosed,
BadCommunicationError, BadNotConnected). Per-node codes such as BadNodeIdUnknown
stay per-tag — that is exactly the device-still-booting case the resolution timer
exists for.

Tests: DataConnectionStaleConnectResultTests (stale success neither connects nor
re-subscribes against the new adapter; stale failure does not disturb the current
attempt; the current generation's result is still processed after a stale drop) —
all 3 fail with the guards removed. DataConnectionOpcUaFaultClassificationTests
(bad-session drives reconnect — fails unfixed; bad-node-id stays per-tag).
RealOpcUaClientBatchRollbackTests (no residual registrations after an ApplyChanges
throw; repeated failed batches do not grow the bookkeeping) — both fail with the
rollback removed. Full DCL suite 342/342 green.
This commit is contained in:
Joseph Doherty
2026-08-19 14:40:56 -04:00
parent e2d149089c
commit 26ab6f8f09
5 changed files with 632 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Adapters/RealOpcUaClient.cs
+111 -40
View File
@@ -621,48 +621,21 @@ public class RealOpcUaClient : IOpcUaClient
var created = new List<(int Index, string NodeId, string Handle, MonitoredItem Item)>(resolved.Count);
var touched = new List<DataShard>();
foreach (var (index, nodeId, startNode) in resolved)
try
{
var shard = await PlaceOnDataShardAsync(cancellationToken);
var handle = Guid.NewGuid().ToString();
var monitoredItem = new MonitoredItem(shard.Subscription.DefaultItem)
{
DisplayName = nodeId,
StartNodeId = startNode,
AttributeId = Attributes.Value,
SamplingInterval = _options.SamplingIntervalMs,
QueueSize = (uint)_options.QueueSize,
DiscardOldest = _options.DiscardOldest,
Filter = BuildDataChangeFilter(_options.Deadband)
};
_callbacks[handle] = onValueChanged;
monitoredItem.Notification += (_, e) =>
{
if (e.NotificationValue is MonitoredItemNotification notification)
{
var value = notification.Value?.Value;
var timestamp = notification.Value?.SourceTimestamp ?? DateTime.UtcNow;
var statusCode = notification.Value?.StatusCode.Code ?? 0;
if (_callbacks.TryGetValue(handle, out var cb))
{
cb(nodeId, value, timestamp, statusCode);
}
}
};
shard.Subscription.AddItem(monitoredItem);
shard.ItemCount++;
_itemShard[handle] = shard.Subscription;
created.Add((index, nodeId, handle, monitoredItem));
if (!touched.Contains(shard))
touched.Add(shard);
await PlaceAndApplyAsync(resolved, onValueChanged, created, touched, cancellationToken);
}
catch
{
// Anything thrown from placement or ApplyChanges (a dead session is the common
// one) leaves every item created so far registered in _callbacks / _itemShard and
// counted against its shard's budget, with nothing to ever release them: the
// actor sees the throw as a whole-chunk fault and re-issues the SAME batch on its
// backoff timer, so each probe grew the bookkeeping again (arch-review 2026-08-19
// P2-4). Unwind before propagating.
UnwindCreatedItems(created);
throw;
}
// ONE ApplyChanges per touched shard — the whole point of the batch seam.
foreach (var shard in touched)
await shard.Subscription.ApplyChangesAsync(cancellationToken);
foreach (var (index, nodeId, handle, item) in created)
{
@@ -687,6 +660,104 @@ public class RealOpcUaClient : IOpcUaClient
}, cancellationToken);
}
/// <summary>
/// Places every resolved node on a data shard and issues ONE ApplyChanges per touched shard.
/// Callers must hold <see cref="_applyGate"/>. <paramref name="created"/> and
/// <paramref name="touched"/> are filled as the work proceeds so a throw can be unwound by
/// <see cref="UnwindCreatedItems"/>.
/// </summary>
private async Task PlaceAndApplyAsync(
List<(int Index, string NodeId, NodeId Resolved)> resolved,
Action<string, object?, DateTime, uint> onValueChanged,
List<(int Index, string NodeId, string Handle, MonitoredItem Item)> created,
List<DataShard> touched,
CancellationToken cancellationToken)
{
foreach (var (index, nodeId, startNode) in resolved)
{
var shard = await PlaceOnDataShardAsync(cancellationToken);
var handle = Guid.NewGuid().ToString();
var monitoredItem = new MonitoredItem(shard.Subscription.DefaultItem)
{
DisplayName = nodeId,
StartNodeId = startNode,
AttributeId = Attributes.Value,
SamplingInterval = _options.SamplingIntervalMs,
QueueSize = (uint)_options.QueueSize,
DiscardOldest = _options.DiscardOldest,
Filter = BuildDataChangeFilter(_options.Deadband)
};
_callbacks[handle] = onValueChanged;
monitoredItem.Notification += (_, e) =>
{
if (e.NotificationValue is MonitoredItemNotification notification)
{
var value = notification.Value?.Value;
var timestamp = notification.Value?.SourceTimestamp ?? DateTime.UtcNow;
var statusCode = notification.Value?.StatusCode.Code ?? 0;
if (_callbacks.TryGetValue(handle, out var cb))
{
cb(nodeId, value, timestamp, statusCode);
}
}
};
shard.Subscription.AddItem(monitoredItem);
shard.ItemCount++;
_itemShard[handle] = shard.Subscription;
created.Add((index, nodeId, handle, monitoredItem));
if (!touched.Contains(shard))
touched.Add(shard);
}
// ONE ApplyChanges per touched shard — the whole point of the batch seam.
foreach (var shard in touched)
await shard.Subscription.ApplyChangesAsync(cancellationToken);
}
/// <summary>
/// Rolls back the monitored items a FAILED batch had already placed, so a throw leaves no
/// residue behind: no <c>_callbacks</c> entry (a leak that also keeps a dead callback
/// reachable from the SDK's publish thread), no <c>_itemShard</c> entry, no phantom shard
/// occupancy, and no item left on the shard's local item list. Callers must hold
/// <see cref="_applyGate"/>.
///
/// <para>
/// Best-effort and exception-safe by design: it runs on the failure path, so one item that
/// cannot be released must not stop the rest from being released and must not replace the
/// original fault. No ApplyChanges is re-issued — the session that just threw is generally
/// dead, and where a partial apply DID reach the server the items it created are reclaimed
/// when the shard and session are torn down on the reconnect that the connection-level
/// classification of that fault now drives.
/// </para>
/// </summary>
private void UnwindCreatedItems(
List<(int Index, string NodeId, string Handle, MonitoredItem Item)> created)
{
if (created.Count == 0)
return;
foreach (var (_, nodeId, handle, item) in created)
{
try
{
ReleaseFailedItem(handle, item);
}
catch (Exception ex)
{
_logger.LogWarning(ex,
"OPC UA batch subscribe rollback could not release the monitored item for {NodeId}",
nodeId);
}
}
_logger.LogWarning(
"OPC UA batch subscribe failed — rolled back {Count} partially created monitored item(s)",
created.Count);
}
/// <summary>
/// Returns the shard this item belongs on per
/// <see cref="MonitoredItemShardPlanner"/> — first shard with free capacity, else a