dohertj2/lmxopcua
1
0
Fork 0
Code Issues 5 Pull Requests Actions Packages Projects Releases Wiki Activity

Merge #868 fourth review fixes: poison only by SQL error allowlist, memory-counted acks survive restart, Stale-recovered revisions unconfirmed

Browse Source 
Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk
This commit is contained in:
Joseph Doherty
2026-11-11 13:35:42 -05:00
parent 730bb19e6b ec3b835e4a
commit c63233b654
7 changed files with 367 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Coordinators/ConfigPublishCoordinator.cs
+40 -11
View File
@@ -109,6 +109,12 @@ public sealed class ConfigPublishCoordinator : ReceiveActor, IWithTimers, IWithU
private readonly Dictionary<NodeId, PendingPersist> _unpersisted = new(NodeIdComparer);
private string? _lastPersistError;
/// <summary>
/// Acks counted without a persisted row (a rejected write, or the deferral cap) — re-sent across a restart, since
/// no row records them (fourth review M1). Dropped once a later persist of that node succeeds.
/// </summary>
private readonly Dictionary<NodeId, ApplyAck> _memoryOnlyAcks = new(NodeIdComparer);
/// <summary>When the current deadline was first deferred on an unpersisted ack (bounds the deferral).</summary>
private DateTime? _deadlineDeferredSinceUtc;
@@ -272,6 +278,9 @@ public sealed class ConfigPublishCoordinator : ReceiveActor, IWithTimers, IWithU
// Timers die with this incarnation; the mailbox survives. Re-send every ack that is still unpersisted so the
// next incarnation (which stashes while Loading) persists and counts it — its node will not resend (M2).
foreach (var pending in _unpersisted.Values) Self.Tell(pending.Ack);
// Fourth review M1: so are acks counted only FROM MEMORY (rejected writes, the deferral cap) — the outcome they
// fed has not been written yet, and without this the next incarnation would wait on their nodes forever.
foreach (var ack in _memoryOnlyAcks.Values) Self.Tell(ack);
base.PreRestart(reason, message);
}
@@ -430,8 +439,11 @@ public sealed class ConfigPublishCoordinator : ReceiveActor, IWithTimers, IWithU
// A FULL deadline (third review #3, superseding the second review's "remaining time"): a node needs its
// whole outcome timeout (DeployTimings.ApplyDeadline - ApplyOutcomeSafetyMargin) plus that margin to
// report — i.e. the full ApplyDeadline — so ANY remaining time shorter than that let the nodes apply
// and the deployment time out moments later. Bounded anyway: there is no dispatched-at column, so a
// SECOND restart recomputes from CreatedAtUtc and, past the deadline by then, closes the row instead.
// and the deployment time out moments later. Bounded anyway: there is no dispatched-at column, so the
// fresh deadline lives only in this incarnation's timer. After a SECOND restart or singleton failover,
// recovery recomputes "remaining" from Deployment.CreatedAtUtc again — by then usually past the
// deadline — so the row is no longer re-driven: if its expected set was seeded it is timed out from the
// seeded rows (acks that landed are kept), otherwise it is closed TimedOut as a lost dispatch.
HandleDispatch(new DispatchDeployment(
_current.Value, RevisionHash.Parse(inflight.RevisionHash), CorrelationId.NewId()));
}
@@ -637,6 +649,7 @@ public sealed class ConfigPublishCoordinator : ReceiveActor, IWithTimers, IWithU
"counting it from memory. Its NodeDeploymentState row stays unrecorded",
msg.NodeId, msg.DeploymentId, msg.Outcome);
DropUnpersisted(msg.NodeId);
_memoryOnlyAcks[msg.NodeId] = msg;
CountAck(msg);
return true;
}
@@ -659,6 +672,7 @@ public sealed class ConfigPublishCoordinator : ReceiveActor, IWithTimers, IWithU
}
DropUnpersisted(msg.NodeId);
_memoryOnlyAcks.Remove(msg.NodeId);
CountAck(msg);
return true;
}
@@ -680,21 +694,34 @@ public sealed class ConfigPublishCoordinator : ReceiveActor, IWithTimers, IWithU
}
/// <summary>
/// A non-transient rejection of the write itself: an EF <see cref="DbUpdateException"/> (not a concurrency
/// conflict) with no transient database error, timeout or socket failure underneath. Connectivity failures
/// surface as other exception types (or as transient <see cref="System.Data.Common.DbException"/>s) and stay
/// retryable.
/// SQL Server errors that reject the WRITE ITSELF and fail identically on every retry (fourth review M1): string
/// or binary data truncated (8152, 2628), FK/CHECK conflict (547), NULL into a NOT NULL column (515), unique
/// key/index violation (2627, 2601).
/// </summary>
internal static bool IsPoison(Exception ex)
private static readonly int[] PoisonSqlNumbers = [8152, 2628, 547, 515, 2627, 2601];
/// <summary>
/// Whether an ack write failure is POISON — never retried — by an ALLOWLIST: an EF <see cref="DbUpdateException"/>
/// (not a concurrency conflict) carrying a <see cref="Microsoft.Data.SqlClient.SqlException"/> whose error
/// numbers include one of <see cref="PoisonSqlNumbers"/>. Everything else — deadlocks (1205), timeouts (-2,
/// Win32 258), dropped connections, failovers, unknown providers — RETRIES, bounded by the deadline deferral
/// cap. (Microsoft.Data.SqlClient does not set <c>DbException.IsTransient</c>, so a deny-list on it classified
/// every SQL Server transient as poison.)
/// </summary>
/// <param name="ex">The failure.</param>
/// <returns><see langword="true"/> only for a known, deterministic rejection.</returns>
public static bool IsPoison(Exception ex)
{
if (ex is not DbUpdateException || ex is DbUpdateConcurrencyException) return false;
for (var e = ex.InnerException; e is not null; e = e.InnerException)
{
if (e is System.Data.Common.DbException { IsTransient: true }
or TimeoutException or System.Net.Sockets.SocketException or System.IO.IOException)
return false;
if (e is not Microsoft.Data.SqlClient.SqlException sql) continue;
if (Array.IndexOf(PoisonSqlNumbers, sql.Number) >= 0) return true;
foreach (Microsoft.Data.SqlClient.SqlError error in sql.Errors)
if (Array.IndexOf(PoisonSqlNumbers, error.Number) >= 0) return true;
return false;
}
return true;
return false;
}
/// <summary>A retry tick: only the node's CURRENT unpersisted ack is retried; a superseded one is dropped (L3).</summary>
@@ -818,6 +845,7 @@ public sealed class ConfigPublishCoordinator : ReceiveActor, IWithTimers, IWithU
foreach (var pending in _unpersisted.Values.ToList())
{
DropUnpersisted(pending.Ack.NodeId);
_memoryOnlyAcks[pending.Ack.NodeId] = pending.Ack;
CountAck(pending.Ack);
}
}
@@ -907,6 +935,7 @@ public sealed class ConfigPublishCoordinator : ReceiveActor, IWithTimers, IWithU
_lastPersistError = null;
ReportPersistBacklog();
}
_memoryOnlyAcks.Clear();
_current = null;
_expectedAcks.Clear();
_acks.Clear();
src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs
+26 -11
View File
@@ -290,15 +290,16 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
private RevisionHash? _currentRevision;
/// <summary>
/// The last address-space apply of <see cref="_currentRevision"/> reported DEGRADED (or no
/// outcome in time). A re-dispatch of that same revision then RE-APPLIES instead of taking the
/// <see cref="_currentRevision"/> is not known to be cleanly served: its last address-space apply reported
/// DEGRADED (or no outcome in time), or it was ADOPTED without an outcome-reporting apply (cache boot, Stale
/// recovery, a failed bootstrap restore — lmxopcua#868). A re-dispatch of that same revision then RE-APPLIES instead of taking the
/// idempotent "already at this revision" short-circuit: the short-circuit would ack a clean Applied
/// for an address space known to be partial, and the re-apply is what repairs it (a degraded
/// non-rebuild apply leaves the publish actor's composition un-advanced, so it re-diffs the missing
/// nodes). Reset by every clean outcome and by the ConfigDb boot path (an Applied row is a final outcome);
/// SET by the cache boot paths, whose cached revision may never have reported an outcome (lmxopcua#868).
/// </summary>
private bool _currentRevisionDegraded;
private bool _currentRevisionUnconfirmed;
/// <summary>Applied acks held until the address-space apply reports its outcome, keyed by
/// the apply's correlation id (echoed back in <see cref="OpcUa.OpcUaPublishActor.RebuildCompleted"/>).</summary>
@@ -1438,7 +1439,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
// Unconfirmed (lmxopcua#868 third review #2): the cache is written BEFORE the address-space outcome, so a
// restart in that window boots a revision central never got an ack for. Marking it unconfirmed makes a
// re-dispatch of it RE-APPLY (and ack the real outcome) instead of short-circuiting a clean Applied.
_currentRevisionDegraded = true;
_currentRevisionUnconfirmed = true;
BecomeSteady();
_log.Info(
@@ -1475,9 +1476,11 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
/// </para>
/// <para>
/// <b>This does not make the node current.</b> <c>_currentRevision</c> is set from the
/// cached artifact, so a subsequent dispatch of that same revision correctly no-ops,
/// while any NEW revision still requires central — the cache holds the past, not the
/// future. Dispatch handling is unchanged.
/// cached artifact but marked UNCONFIRMED (<see cref="_currentRevisionUnconfirmed"/>, lmxopcua#868): the
/// cache is written before the address-space outcome, so central may never have had an ack for it. A
/// re-dispatch of that same revision therefore RE-APPLIES — which, while the ConfigDb is still unreachable,
/// acks Failed (the deployment reads PartiallyFailed, not Sealed) — and any NEW revision still requires
/// central: the cache holds the past, not the future.
/// </para>
/// <para>
/// Never throws: a fault here must degrade to Stale, which is exactly where the node
@@ -1520,7 +1523,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
// matching how a normally-booted node behaves.
_currentRevision = revision;
// Unconfirmed, as in BootstrapFromCache (third review #2): a re-dispatch of it re-applies.
_currentRevisionDegraded = true;
_currentRevisionUnconfirmed = true;
BecomeSteady();
_isRunningFromCache = true;
@@ -2821,7 +2824,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
// The short-circuit claims a CLEAN Applied, so it is only honest while the current revision's
// address space is known-good — not after a degraded apply, and not while its outcome is still
// pending. Either way the dispatch re-applies instead, which is also what repairs a degraded apply.
var currentRevisionUnconfirmed = _currentRevisionDegraded
var currentRevisionUnconfirmed = _currentRevisionUnconfirmed
|| _pendingAcks.Values.Any(p => p.Revision == msg.RevisionHash);
if (_currentRevision is { } cur && cur == msg.RevisionHash && !currentRevisionUnconfirmed)
{
@@ -3085,7 +3088,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
{
if (_opcUaPublishActor is null)
{
_currentRevisionDegraded = false;
_currentRevisionUnconfirmed = false;
UpsertNodeDeploymentState(deploymentId, NodeDeploymentStatus.Applied, failureReason: null);
SendAck(deploymentId, ApplyAckOutcome.Applied, failureReason: null, correlation);
return;
@@ -3142,7 +3145,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
// Only the CURRENT revision's flag is ours to set — an older apply's late outcome must not mark a
// newer revision degraded (or clean).
if (_currentRevision is { } cur && cur == pending.Revision)
_currentRevisionDegraded = degraded;
_currentRevisionUnconfirmed = degraded;
// The own row has been Applying since the apply began (lmxopcua#868 review M1); the FINAL outcome is
// written here, clean or degraded. Central adds the reason from the ack only when it ACCEPTS the ack (a
@@ -3597,6 +3600,10 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
try
{
var appliedBlob = ReconcileDrivers(deploymentId);
// Null = the artifact could not be read (LoadArtifactBlob logs and swallows the failure), so the drivers
// were NOT restored: the adopted revision is unconfirmed (fourth review M2), and a re-dispatch of it
// re-applies instead of short-circuiting a clean Applied.
if (appliedBlob is null) _currentRevisionUnconfirmed = true;
_opcUaPublishActor?.Tell(new ZB.MOM.WW.OtOpcUa.Runtime.OpcUa.OpcUaPublishActor.RebuildAddressSpace(correlation, deploymentId, appliedBlob));
PushDesiredSubscriptions(deploymentId);
// Populate the node-local cache from the artifact this restored node is now serving. The
@@ -3613,6 +3620,9 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
}
catch (Exception ex)
{
// The revision was adopted from the row but its served state was NOT restored (fourth review M2): mark it
// unconfirmed so a re-dispatch of it re-applies instead of short-circuiting a clean Applied.
_currentRevisionUnconfirmed = true;
_log.Warning(ex, "DriverHost {Node}: failed to restore served state for {Id} on bootstrap", _localNode, deploymentId);
}
}
@@ -4334,6 +4344,11 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
if (latestSealed is not null)
{
_currentRevision = RevisionHash.Parse(latestSealed.RevisionHash);
// Adopted, NOT applied (fourth review M2): Stale recovery restores only the revision marker — drivers
// and the address space stay empty until a dispatch (see PushDesiredSubscriptions' note). Unconfirmed,
// so a dispatch of this same revision re-applies rather than short-circuiting a clean Applied over an
// empty address space.
_currentRevisionUnconfirmed = true;
UpsertNodeDeploymentState(new DeploymentId(latestSealed.DeploymentId),
NodeDeploymentStatus.Applied, failureReason: null);
}