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

fix(siteruntime): arbitrate DCL connection config — definitions store wins (rows 89 + 90)

Browse Source 
Closes deferred-work-register rows 89 and 90, and corrects their premise twice.

THE RULE
The `data_connection_definitions` store wins wherever it has an opinion; an
instance's flattened `Connections` entry is a deploy-time snapshot and is
authoritative only for names the store has never owned (the deploy-only site,
where that table is legitimately empty).

PREMISE CORRECTION 1 — the peer's DCL is EMPTY, not stale
`DeploymentManagerActor` is a `site-{SiteId}`-scoped singleton and the only
sender of Create/RemoveConnectionCommand, to its OWN node's `dcl-manager`
(which exists on every node but is populated only where the singleton runs).
So the standby serves nothing. The real defect is that `_createdConnections`
is in-memory per singleton incarnation: a restart or failover rebuilds every
connection from the flattened snapshots and silently discards a newer
connection-only artifact deploy. Row 89 is therefore create-time, not
message-delivery, and NOT replication-specific — the same loss happens on an
unreplicated single-node restart.

PREMISE CORRECTION 2 — a changed config never reached a LIVE connection either
`DataConnectionManagerActor.HandleCreateConnection` is create-only (warns and
returns on a name it holds), so `EnsureDclConnection`'s documented "re-issues
the create so the DCL adopts the new configuration" has never been true on any
node. Now surfaced with a Warning saying exactly that, once per changed config.
Not fixed here: stop-and-recreate drops every tag subscription and InstanceActor
cancels its subscribe-retry timer on first success, so nothing re-subscribes.

MECHANISM
`data_connection_definitions` gains its first production reader. It is read on
the SAME off-thread task as the deployed configs (`ReadStoreSnapshotAsync`), by
the boot load and every sweep tick, into an actor-thread-confined arbitration
overlay applied BEFORE the boot batch and BEFORE the sweep's repair loop. A
CDC-delivered definition change converges within one tick on whichever node runs
the singleton — no new cross-node message. A failed definitions read yields null
and leaves the overlay untouched (distinct from an empty list, which is a real
answer); clearing it would reintroduce the ping-pong through the error path.

THE PING-PONG, KILLED BY CONSTRUCTION
§7.4 of the sweep memo rejected a store-level pass because two sources would
flip the single per-name hash slot, recreating the actor every tick. That is a
property of WHERE the hash is computed: every ensure path now resolves the
winner BEFORE hashing, so only one configuration can ever occupy the slot. The
artifact-deploy path writes its rows into the overlay in the same pass, closing
the same flip arriving through the other door.

REMOVAL (row 90)
Both removal paths share `RemoveConnectionCentrally` and record an in-memory
tombstone, so a centrally removed connection is not resurrected by the next
instance start — a pre-existing bug. Tombstones are deliberately not persisted;
a definition row reappearing clears one.

TESTS
tests/.../Actors/DclConnectionArbitrationTests.cs — 8 tests. Fail-before
(behavioral revert: overlay inert + tombstone check disabled): 5 fail, 3 pass;
the 3 passers are the two deploy-only regression guards and the ping-pong guard.

DOCS
New decision memo docs/plans/2026-08-17-dcl-connection-arbitration-decision.md
(topology, rule, ping-pong mechanism, tombstones, residuals); Component-SiteRuntime,
Component-DataConnectionLayer + their docs/components mirrors; the §7.4 residual in
the sweep memo marked closed with its premise corrected.
This commit is contained in:
Joseph Doherty
2026-08-17 05:22:42 -04:00
parent 15e5887b52
commit c57e74e660
8 changed files with 1133 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs
+321 -26
View File
@@ -82,6 +82,17 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
/// </summary>
private readonly Func<Task<List<DeployedInstance>>> _configLoader;
/// <summary>
/// Reads the durable <c>data_connection_definitions</c> rows (deferred register rows 89 + 90).
/// Defaults to <see cref="SiteStorageService.GetAllDataConnectionDefinitionsAsync"/>;
/// overridable via the ctor so a test can drive a read failure.
/// <para>
/// This table had NO production reader before rows 89/90 — it was written by the artifact
/// deploy and never read back, which is precisely why a definition change survived only as
/// long as the singleton incarnation that applied it.
/// </para>
/// </summary>
private readonly Func<Task<List<StoredDataConnectionDefinition>>> _connectionDefinitionLoader;
/// <summary>
/// Set once the startup deployed-config load has succeeded. Guards against a
/// stale <see cref="RetryStartupLoad"/> re-running the load after success.
/// </summary>
@@ -246,6 +257,12 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
/// towards <see cref="ScriptExecutionScheduler.ComputeTargetThreads"/> on every instance-
/// count change; null uses the process-wide shared scheduler.
/// </param>
/// <param name="connectionDefinitionLoader">
/// Rows 89 + 90: optional override for reading the durable
/// <c>data_connection_definitions</c> rows that arbitrate the live DCL configuration;
/// defaults to reading from <paramref name="storage"/>. Primarily for tests that need to
/// drive a read failure.
/// </param>
public DeploymentManagerActor(
SiteStorageService storage,
ScriptCompilationService compilationService,
@@ -261,7 +278,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
TimeSpan? startupLoadRetryInterval = null,
Func<Task<List<DeployedInstance>>>? configLoader = null,
ExternalSystemDefinitionCache? externalSystemCache = null,
ScriptExecutionScheduler? scriptScheduler = null)
ScriptExecutionScheduler? scriptScheduler = null,
Func<Task<List<StoredDataConnectionDefinition>>>? connectionDefinitionLoader = null)
{
_scriptScheduler = scriptScheduler;
_storage = storage;
@@ -280,6 +298,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
? TimeSpan.FromSeconds(options.InstanceSweepIntervalSeconds)
: TimeSpan.Zero;
_configLoader = configLoader ?? (() => _storage.GetAllDeployedConfigsAsync());
_connectionDefinitionLoader = connectionDefinitionLoader
?? (() => _storage.GetAllDataConnectionDefinitionsAsync());
_logger = logger;
// Reuse a single logger factory for all Instance Actors.
// Prefer an explicitly injected factory, fall back to one resolved from
@@ -478,14 +498,50 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
/// </summary>
private void LoadDeployedConfigs()
{
_configLoader().ContinueWith(t =>
ReadStoreSnapshotAsync().ContinueWith(t =>
{
if (t.IsCompletedSuccessfully)
return new StartupConfigsLoaded(t.Result, null);
return new StartupConfigsLoaded(t.Result.Configs, null, t.Result.Definitions);
return new StartupConfigsLoaded([], t.Exception?.GetBaseException().Message);
}).PipeTo(Self);
}
/// <summary>
/// Reads the two durable tables the Deployment Manager arbitrates over — the deployed
/// instance configs and the <c>data_connection_definitions</c> rows — in ONE off-thread
/// task, so the boot load and every sweep tick see a single consistent snapshot and the
/// actor thread is never blocked on SQLite.
///
/// <para>
/// <b>The two reads fail differently on purpose.</b> A failed CONFIG read propagates: it is
/// the existing S5 path and re-arms the whole load at a fixed interval. A failed DEFINITIONS
/// read is swallowed into a <see langword="null"/> result, because the DCL arbitration overlay
/// must be left EXACTLY as it was rather than cleared — an overlay cleared by a transient
/// read failure would hand arbitration back to the flattened snapshots for one pass and then
/// take it away again on the next successful read, which is the recreate-on-every-flip
/// ping-pong the whole design exists to make impossible.
/// </para>
/// </summary>
private async Task<(List<DeployedInstance> Configs, IReadOnlyList<StoredDataConnectionDefinition>? Definitions)>
ReadStoreSnapshotAsync()
{
var configs = await _configLoader().ConfigureAwait(false);
IReadOnlyList<StoredDataConnectionDefinition>? definitions = null;
try
{
definitions = await _connectionDefinitionLoader().ConfigureAwait(false);
}
catch (Exception ex)
{
_logger.LogWarning(
ex, "Could not read the data connection definitions — the DCL arbitration "
+ "overlay keeps its previous contents until the next read succeeds");
}
return (configs, definitions);
}
/// <inheritdoc />
protected override void PostStop()
{
@@ -544,6 +600,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
// Mark the startup load complete so a stale retry timer is ignored.
_startupLoadCompleted = true;
// Rows 89 + 90: seed the DCL arbitration overlay BEFORE any Instance Actor is created,
// so the boot batch's EnsureDclConnections already resolves to the definition-owned
// config. This is the failover half of the row-89 fix: a singleton incarnation starting
// on the peer node rebuilds the whole DCL from flattened snapshots, and without the
// overlay in place first it would rebuild them STALE and only correct itself a sweep
// tick later — a needless create-then-diverge on every failover.
ApplyConnectionDefinitionOverlay(msg.ConnectionDefinitions);
var enabledConfigs = msg.Configs.Where(c => c.IsEnabled).ToList();
_deployedInstanceNames.Clear();
foreach (var c in msg.Configs)
@@ -1563,8 +1627,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
_instanceSweepInFlight = true;
_configLoader().ContinueWith(t => t.IsCompletedSuccessfully
? new InstanceSweepLoaded(t.Result, null)
ReadStoreSnapshotAsync().ContinueWith(t => t.IsCompletedSuccessfully
? new InstanceSweepLoaded(t.Result.Configs, null, t.Result.Definitions)
: new InstanceSweepLoaded([], t.Exception?.GetBaseException().Message))
.PipeTo(Self);
}
@@ -1589,6 +1653,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
return;
}
// Rows 89 + 90: refresh the DCL arbitration overlay FIRST, so any instance this same
// sweep pass goes on to start resolves its connections against the current definitions
// rather than against the snapshot frozen into its flattened config. This is the
// convergence path for a definition row that arrived by LocalDb CDC replication: no new
// cross-node message exists (or is needed) — whichever node runs the singleton reads the
// row out of its own file on the next tick.
ApplyConnectionDefinitionOverlay(msg.ConnectionDefinitions);
foreach (var config in msg.Configs)
{
var instanceName = config.InstanceUniqueName;
@@ -2090,10 +2162,156 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
private readonly Dictionary<string, string> _createdConnections = new();
/// <summary>
/// Sets up DCL connections from the flattened config. Idempotent on unchanged
/// configuration, but re-issues the create command when a connection's endpoint,
/// credentials, backup endpoint, or failover retry count has changed since it was
/// last sent.
/// <b>The DCL connection arbitration overlay</b> (deferred register rows 89 + 90) — the
/// <c>data_connection_definitions</c> rows this singleton incarnation currently believes the
/// site store owns, keyed by connection name.
///
/// <para>
/// <b>The rule: the definitions store wins wherever it has an opinion.</b> A connection name
/// with a row here is configured from that row; the identically-named entry inside an
/// instance's flattened config is a deploy-time SNAPSHOT and never overrides it. A name with
/// no row here falls through to the flattened config exactly as before — that is not a
/// fallback, it is the correct answer for a deploy-only site, where this table is legitimately
/// EMPTY because it is written only by the system-wide artifact deploy while the connections
/// that matter live inline in the instance configs.
/// </para>
///
/// <para>
/// <b>This overlay is an arbiter, not a creation source.</b> A definition row for a name no
/// deployed instance references creates nothing: there is no connection actor wanted for it.
/// The overlay only decides WHICH configuration a connection the runtime already wants is
/// created from — which is what makes it safe to consult on every sweep tick.
/// </para>
///
/// <para>
/// Refreshed by <see cref="ApplyConnectionDefinitionOverlay"/> from the boot load and from
/// every sweep tick, and kept consistent in-flight by
/// <see cref="HandleApplyArtifactDataConnectionsToDcl"/>. Actor-thread-confined.
/// </para>
/// </summary>
private readonly Dictionary<string, StoredDataConnectionDefinition> _connectionDefinitions =
new(StringComparer.Ordinal);
/// <summary>
/// Connection names this singleton incarnation has seen REMOVED centrally (deferred register
/// row 90) — either by an artifact reconcile it processed itself, or by a definition row that
/// disappeared from the store between two sweep reads.
///
/// <para>
/// Without it the removal does not stick for as long as one tick: the removal evicts the name
/// from <see cref="_createdConnections"/>, and the very next instance start whose flattened
/// config still names that connection re-creates it — the flattened snapshot resurrecting a
/// connection central has deleted. That is not hypothetical; the sweep starts instances.
/// </para>
///
/// <para>
/// <b>Deliberately in-memory only.</b> Persisting tombstones was rejected: a durable
/// "this connection is gone" marker is a second, divergent authority over the same names, and
/// the flattened configs already resurrect a removed connection across a restart today — that
/// pre-existing semantic is accepted and unchanged, and is recorded as the residual in
/// <c>docs/plans/2026-08-17-dcl-connection-arbitration-decision.md</c>. Cleared the moment a
/// definition row for the name reappears, so a centrally re-added connection comes straight
/// back.
/// </para>
/// </summary>
private readonly HashSet<string> _centrallyRemovedConnections = new(StringComparer.Ordinal);
/// <summary>
/// Refreshes <see cref="_connectionDefinitions"/> from a store read and reconciles the live
/// DCL against it (deferred register rows 89 + 90).
///
/// <para>
/// <paramref name="definitions"/> is <see langword="null"/> when the read FAILED, which is
/// deliberately distinct from an empty list: a failed read leaves the overlay untouched,
/// while an empty list is a real answer (the deploy-only site shape, and also what a site
/// looks like after central deletes every data connection).
/// </para>
///
/// <para>
/// <b>Disappearance detection, and exactly how far it reaches.</b> A name that WAS in this
/// overlay and no longer has a row is a removal this incarnation can prove: the store owned
/// it a moment ago and does not now. It is tombstoned and its live connection actor stopped.
/// A name that was never in the overlay is indistinguishable from one the store has never
/// owned — a fresh incarnation cannot tell "central deleted this" from "this site keeps its
/// connections inline" — so it is left to the flattened config. That asymmetry is the
/// documented residual of rows 89/90, not an oversight.
/// </para>
/// </summary>
private void ApplyConnectionDefinitionOverlay(
IReadOnlyList<StoredDataConnectionDefinition>? definitions)
{
if (definitions == null)
return;
var disappeared = _connectionDefinitions.Keys
.Where(name => !definitions.Any(d => string.Equals(d.Name, name, StringComparison.Ordinal)))
.ToList();
foreach (var definition in definitions)
{
// A definition row is the store re-asserting ownership: any tombstone for the name
// is stale, so a centrally re-added connection is creatable again immediately.
_centrallyRemovedConnections.Remove(definition.Name);
_connectionDefinitions[definition.Name] = definition;
// Push only to connections the runtime already holds. A definition nothing
// references stays inert — see the overlay's "arbiter, not a creation source" note.
if (_createdConnections.ContainsKey(definition.Name))
{
EnsureDclConnection(
definition.Name,
definition.Protocol,
definition.ConfigurationJson,
definition.BackupConfigurationJson,
definition.FailoverRetryCount);
}
}
foreach (var name in disappeared)
RemoveConnectionCentrally(name, "its definition row is no longer in the site store");
}
/// <summary>
/// Applies a central removal of a data connection: tombstones the name for this singleton
/// incarnation, drops the overlay row and the hash-cache slot, and stops the live DCL
/// connection actor. Shared by the artifact-reconcile removal and the sweep's
/// disappearance detection so both produce identical state.
///
/// <para>
/// A connection still referenced by a deployed instance's config no longer exists centrally
/// either — its attributes go bad quality, the standard disconnected signal, until the
/// instance is redeployed against current central config. That is the pre-existing removal
/// semantic and it is unchanged here; what IS new is that the removal now survives the next
/// instance start instead of being undone by it.
/// </para>
/// </summary>
private void RemoveConnectionCentrally(string name, string because)
{
_connectionDefinitions.Remove(name);
_centrallyRemovedConnections.Add(name);
_createdConnections.Remove(name);
_dclManager?.Tell(new DataConnectionLayer.Actors.RemoveConnectionCommand(name));
_logger.LogWarning(
"Data connection '{Name}' removed centrally ({Because}); its DCL connection actor "
+ "has been stopped and will not be recreated from an instance's flattened config",
name, because);
}
/// <summary>
/// Sets up DCL connections from the flattened config, <b>arbitrated against
/// <see cref="_connectionDefinitions"/></b> (deferred register rows 89 + 90). Idempotent on
/// unchanged configuration.
///
/// <para>
/// <b>Why the arbitration has to happen HERE, before the hash is computed.</b>
/// <see cref="_createdConnections"/> holds one hash slot per connection name. The flattened
/// snapshot and the definition row for the same name can legitimately differ — that is the
/// supported version-skew case, an artifact deploy newer than the instance deploys that
/// reference it. Two callers hashing from two different sources would flip that one slot back
/// and forth, and every flip is a fresh <c>CreateConnectionCommand</c>. Resolving the WINNER
/// first means only one configuration can ever be hashed for a name, whichever path calls, so
/// the flip is not merely avoided by convention — it is unrepresentable.
/// </para>
/// </summary>
private void EnsureDclConnections(string configJson)
{
@@ -2106,6 +2324,29 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
foreach (var (name, connConfig) in config.Connections)
{
// Row 90: never resurrect a name the artifact reconcile removed.
if (_centrallyRemovedConnections.Contains(name))
{
_logger.LogDebug(
"Skipping DCL connection '{Connection}' named by a flattened config — it "
+ "was removed centrally in this incarnation", name);
continue;
}
if (_connectionDefinitions.TryGetValue(name, out var owned))
{
// Rule 1: the store has an opinion, so the store wins.
EnsureDclConnection(
name,
owned.Protocol,
owned.ConfigurationJson,
owned.BackupConfigurationJson,
owned.FailoverRetryCount);
continue;
}
// Rule 2: no definition row has ever owned this name — the flattened config is
// the source, exactly as before. This is the whole of a deploy-only site.
EnsureDclConnection(
name,
connConfig.Protocol,
@@ -2156,9 +2397,29 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
var changed = _createdConnections.ContainsKey(name);
_createdConnections[name] = configHash;
if (changed)
{
// Be honest about what a re-issued create does. DataConnectionManagerActor's
// HandleCreateConnection is CREATE-ONLY: it warns and returns for a name it already
// holds, so a live connection actor does NOT adopt the new configuration here. The
// change becomes effective the next time the connection is created from scratch —
// the connection is removed and re-added, or this node restarts / the singleton fails
// over, at which point the arbitration overlay hands out the current configuration.
// Emitted once per changed configuration (the hash guard above suppresses repeats),
// so a healthy site stays silent. See the "live reconfiguration" residual in
// docs/plans/2026-08-17-dcl-connection-arbitration-decision.md.
_logger.LogWarning(
"DCL connection {Connection} (protocol={Protocol}) has a new configuration, but a "
+ "live connection actor is not reconfigured in place — it keeps serving its "
+ "current configuration until it is recreated (node restart, singleton failover, "
+ "or the connection being removed and re-added)",
name, protocol);
return;
}
_logger.LogInformation(
"{Action} DCL connection {Connection} (protocol={Protocol})",
changed ? "Updated" : "Created", name, protocol);
"Created DCL connection {Connection} (protocol={Protocol})", name, protocol);
}
/// <summary>
@@ -2793,6 +3054,21 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
{
foreach (var dc in msg.DataConnections)
{
// Rows 89 + 90: the rows have just been committed to data_connection_definitions, so
// update the arbitration overlay in the same pass rather than waiting for the next
// sweep read. Skipping this would leave a window in which an instance start resolves
// the name to its flattened snapshot while the artifact path has already pushed the
// definition — the exact two-sources-one-hash-slot flip the overlay exists to prevent.
_centrallyRemovedConnections.Remove(dc.Name);
_connectionDefinitions[dc.Name] = new StoredDataConnectionDefinition
{
Name = dc.Name,
Protocol = dc.Protocol,
ConfigurationJson = dc.PrimaryConfigurationJson,
BackupConfigurationJson = dc.BackupConfigurationJson,
FailoverRetryCount = dc.FailoverRetryCount
};
EnsureDclConnection(
dc.Name,
dc.Protocol,
@@ -2801,20 +3077,12 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
dc.FailoverRetryCount);
}
// Reconciled removals: the definition row is already gone from SQLite; evict
// the hash cache (so a later re-add with the same name recreates cleanly) and
// stop the live DCL connection actor. A connection still referenced by a
// deployed instance's config no longer exists centrally either — its attributes
// go bad quality, the standard disconnected signal, until the instance is
// redeployed against current central config.
// Reconciled removals: the definition row is already gone from SQLite; drop the overlay
// row, tombstone the name for this incarnation (row 90 — otherwise the next instance
// start recreates it from that instance's flattened config), evict the hash cache so a
// later re-add recreates cleanly, and stop the live DCL connection actor.
foreach (var name in msg.RemovedConnectionNames)
{
_createdConnections.Remove(name);
_dclManager?.Tell(new DataConnectionLayer.Actors.RemoveConnectionCommand(name));
_logger.LogWarning(
"Artifact reconcile removed data connection '{Name}' (deleted centrally); " +
"its DCL connection actor has been stopped", name);
}
RemoveConnectionCentrally(name, "deleted centrally, reconciled by an artifact deploy");
}
/// <summary>
@@ -2861,7 +3129,24 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
/// </summary>
internal sealed record SiteNodeJoined(Address Address);
internal record StartupConfigsLoaded(List<DeployedInstance> Configs, string? Error);
/// <summary>
/// Carries the boot-time store read back onto the actor thread.
///
/// <para>
/// <paramref name="ConnectionDefinitions"/> (deferred register rows 89 + 90) is the
/// <c>data_connection_definitions</c> snapshot read in the SAME off-thread task, so the DCL
/// arbitration overlay is populated BEFORE the boot batch creates any Instance Actor — a
/// connection whose definition row is newer than the flattened snapshot inside the instance
/// config is therefore created from the definition on the very first push, not created stale
/// and corrected a sweep later. <see langword="null"/> means the definitions read FAILED and
/// the previous overlay must be kept; an EMPTY list means the table is genuinely empty, which
/// is the deploy-only site shape (see <see cref="ApplyConnectionDefinitionOverlay"/>).
/// </para>
/// </summary>
internal record StartupConfigsLoaded(
List<DeployedInstance> Configs,
string? Error,
IReadOnlyList<StoredDataConnectionDefinition>? ConnectionDefinitions = null);
/// <summary>
/// Internal singleton signal that re-attempts the startup deployed-config load
@@ -2946,8 +3231,18 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
/// <summary>
/// Carries one sweep's off-thread read of the durable deployed-config rows back onto the
/// actor thread, where every comparison and repair happens.
///
/// <para>
/// <paramref name="ConnectionDefinitions"/> (rows 89 + 90) rides the same read so the DCL
/// arbitration overlay is refreshed on every tick — that is what makes a definition row
/// delivered by LocalDb CDC converge without any new cross-node message. Same null-vs-empty
/// contract as <see cref="StartupConfigsLoaded"/>: null = read failed, keep the overlay.
/// </para>
/// </summary>
internal record InstanceSweepLoaded(List<DeployedInstance> Configs, string? Error);
internal record InstanceSweepLoaded(
List<DeployedInstance> Configs,
string? Error,
IReadOnlyList<StoredDataConnectionDefinition>? ConnectionDefinitions = null);
internal record DeployPersistenceResult(
string DeploymentId, string InstanceName, bool Success, string? Error,