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

perf(host): install CDC capture only when replication is configured

Browse Source 
SiteLocalDbSetup.OnReady registered all ten replicated tables
unconditionally, so a deliberately unreplicated site node (site-b and
site-c on the rig) carried the full 30-trigger CDC set forever. Every
write to those tables paid two extra INSERTs plus a json_object
serialization of the whole row, inside the caller's own transaction, and
appended to an oplog nothing ever drains. Arch-review finding #5 (High),
repo half; the library half — trigger cleanup API and O(1) backlog — is
WP3.3.

The ten RegisterReplicated calls are now behind a guard on whether the
node has LocalDb:Replication:PeerAddress OR LocalDb:Replication:ApiKey.
Either key counts, and the OR is load-bearing rather than defensive:
replication is one bidirectional stream that exactly one side dials, so
only the initiator sets PeerAddress. Verified against the rig — site-a
node-a has PeerAddress + ApiKey, site-a node-b (passive) has ApiKey
alone, site-b/site-c have no Replication section at all. Keying on
PeerAddress alone would have stripped capture from every passive node and
silently made each pair converge in one direction only.

The load-bearing ordering documented in the file is preserved: DDL still
precedes registration, and the legacy migrator still runs unconditionally
after it — an unreplicated node must still absorb its pre-Phase-1 files,
and it has no peer for those rows to be invisible to.

Known residual, documented in-file and in the topology guide: a database
file first created by an older build keeps its stale __localdb_* triggers.
The guard decides whether triggers are installed, not whether existing
ones are removed, and the library has no removal API until WP3.3. Moot on
the docker rig, where a schema-change redeploy recreates the volumes.
The inverse is also now documented: enabling replication on a site that
has run without it does not baseline existing rows, since CDC never
recorded them in __localdb_row_version and the snapshot resync streams
from that ledger.

Tests: new SiteLocalDbCdcRegistrationTests asserts trigger presence and
absence via sqlite_master across all four config shapes (none, ApiKey
only, PeerAddress + ApiKey, and the notification-table exclusion), plus
DDL-still-runs and migrator-still-runs on the unreplicated branch.
SiteLocalDbWiringTests and the integration site-pair harness now
configure an ApiKey — mirroring the rig's passive node — so their
registration and convergence assertions still describe a replicating
node. 483/483 Host.Tests pass; the 20 offline LocalDb convergence tests
still pass.
This commit is contained in:
Joseph Doherty
2026-08-14 19:59:53 -04:00
parent ee193cd2bb
commit 7ebdcd370a
5 changed files with 358 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/deployment/topology-guide.md
+20
View File
@@ -157,6 +157,26 @@ Each site has its own two-node cluster:
- SQLite persistence: each node owns its own consolidated LocalDb database, kept in step by
asynchronous CDC replication over a gRPC sync stream (LocalDb Phase 1 + 2). The nodes do NOT
share a SQLite file.
- CDC capture triggers are installed **only on a node that has replication configured** —
`LocalDb:Replication:PeerAddress` *or* `LocalDb:Replication:ApiKey`. Either key counts, because
only the initiating half of a pair sets `PeerAddress` (one bidirectional stream, dialled by one
side); the passive half carries the key alone. A deliberately unreplicated node — site-b and
site-c on the rig — runs with no triggers at all and stops paying the per-write capture cost.
#### Turning replication ON for a site that has been running without it
Set the keys on **both** nodes and restart both. Two things to know before you do:
- **Existing rows are not baselined.** Capture is change-data-capture: rows written while the node
had no triggers were never recorded in `__localdb_row_version`, and LocalDb's snapshot resync
streams from that ledger, so it will not ship them. The pair converges on everything written
*after* the restart and stays silently divergent on everything before it. Start from a copy of one
node's database on both sides, or accept that only new writes converge.
- **A node upgraded in place keeps stale triggers.** The guard decides whether triggers are
*installed*, not whether existing ones are removed, and the library has no removal API yet. A
database file first created by a build that always registered keeps capturing until that lands.
Recreating the node's data volume clears it — which is what a schema-change redeploy does on the
docker rig, so the rig is unaffected.
### Site Pair Upgrades — stop and start BOTH nodes together
src/ZB.MOM.WW.ScadaBridge.Host/SiteLocalDbSetup.cs
+96 -31
View File
@@ -21,6 +21,14 @@ namespace ZB.MOM.WW.ScadaBridge.Host;
/// with no error anywhere.
/// </para>
/// <para>
/// <b>Registration is conditional</b> on the node actually having a replication peer —
/// see <see cref="ReplicationIsConfigured"/>. An unreplicated node used to install the
/// full CDC trigger set anyway and then pay for it on every single write, forever, for an
/// oplog nothing ever drains. The ordering rule above is unaffected: when registration is
/// skipped there is no capture to be too late for, and the legacy migrator still runs
/// (its rows simply do not replicate, which is what "unreplicated node" means).
/// </para>
/// <para>
/// Runs inside the <c>AddZbLocalDb</c> onReady callback, once, before any caller
/// receives the <see cref="ILocalDb"/> singleton. onReady is a synchronous
/// <c>Action&lt;ILocalDb&gt;</c>, hence the direct <c>CreateConnection()</c> rather than
@@ -32,11 +40,14 @@ namespace ZB.MOM.WW.ScadaBridge.Host;
public static class SiteLocalDbSetup
{
/// <summary>
/// Creates the site node's replicated tables, opts them into change capture, and
/// migrates any pre-Phase-1 databases in.
/// Creates the site node's tables, opts them into change capture when this node
/// replicates, and migrates any pre-Phase-1 databases in.
/// </summary>
/// <param name="db">The LocalDb instance being initialized.</param>
/// <param name="config">Configuration, for the legacy migrator's old path keys and node name.</param>
/// <param name="config">
/// Configuration, for the replication predicate, the legacy migrator's old path keys,
/// and the node name.
/// </param>
public static void OnReady(ILocalDb db, IConfiguration config)
{
ArgumentNullException.ThrowIfNull(db);
@@ -53,39 +64,93 @@ public static class SiteLocalDbSetup
StoreAndForwardSchema.Apply(connection);
}
// Both tables qualify: each has an explicit primary key (RegisterReplicated
// rejects tables without one) and no BLOB columns (which json_object cannot
// capture). Registration is idempotent and installs the capture triggers.
db.RegisterReplicated("OperationTracking");
db.RegisterReplicated("site_events");
if (ReplicationIsConfigured(config))
{
// Both tables qualify: each has an explicit primary key (RegisterReplicated
// rejects tables without one) and no BLOB columns (which json_object cannot
// capture). Registration is idempotent and installs the capture triggers.
db.RegisterReplicated("OperationTracking");
db.RegisterReplicated("site_events");
// Phase 2: the store-and-forward buffer and the seven site configuration tables.
// These replaced the bespoke SiteReplicationActor and StoreAndForward
// ReplicationService, which shipped hand-written Add/Remove/Park/Requeue operations
// over Akka; both were deleted in the same commit that added these lines, so the
// two mechanisms never ran at once.
//
// Both composite-PK tables are fine: RegisterReplicated orders multi-column PKs by
// ordinal. No Phase 2 table has a BLOB column, which it would reject.
db.RegisterReplicated("sf_messages");
db.RegisterReplicated("deployed_configurations");
db.RegisterReplicated("static_attribute_overrides");
db.RegisterReplicated("shared_scripts");
db.RegisterReplicated("external_systems");
db.RegisterReplicated("database_connections");
db.RegisterReplicated("data_connection_definitions");
db.RegisterReplicated("native_alarm_state");
// Phase 2: the store-and-forward buffer and the seven site configuration tables.
// These replaced the bespoke SiteReplicationActor and StoreAndForward
// ReplicationService, which shipped hand-written Add/Remove/Park/Requeue operations
// over Akka; both were deleted in the same commit that added these lines, so the
// two mechanisms never ran at once.
//
// Both composite-PK tables are fine: RegisterReplicated orders multi-column PKs by
// ordinal. No Phase 2 table has a BLOB column, which it would reject.
db.RegisterReplicated("sf_messages");
db.RegisterReplicated("deployed_configurations");
db.RegisterReplicated("static_attribute_overrides");
db.RegisterReplicated("shared_scripts");
db.RegisterReplicated("external_systems");
db.RegisterReplicated("database_connections");
db.RegisterReplicated("data_connection_definitions");
db.RegisterReplicated("native_alarm_state");
// notification_lists and smtp_configurations are created but deliberately NOT
// registered. They are permanently empty by design — the site-side write paths were
// removed on 2026-07-10, the legacy migrator skips them, and the active node's
// artifact apply purges them on every deploy. Registering them would open a standing
// replication channel whose only historical payload was plaintext SMTP passwords, in
// exchange for replicating nothing. Anyone adding them here should first establish
// that a site has a legitimate reason to hold SMTP credentials at all.
// notification_lists and smtp_configurations are created but deliberately NOT
// registered. They are permanently empty by design — the site-side write paths were
// removed on 2026-07-10, the legacy migrator skips them, and the active node's
// artifact apply purges them on every deploy. Registering them would open a standing
// replication channel whose only historical payload was plaintext SMTP passwords, in
// exchange for replicating nothing. Anyone adding them here should first establish
// that a site has a legitimate reason to hold SMTP credentials at all.
}
// AFTER registration, so migrated rows enter the oplog and reach the peer like
// any other write. Before it, they would be invisible to replication forever.
// Deliberately OUTSIDE the guard: an unreplicated node still has to absorb its
// pre-Phase-1 files, and there is no peer for its rows to be invisible to.
SiteLocalDbLegacyMigrator.Migrate(db, config);
}
/// <summary>
/// Whether this node participates in LocalDb replication, and therefore needs the CDC
/// capture triggers at all.
/// </summary>
/// <remarks>
/// <para>
/// <b>Either key counts, and that is not redundancy — it is the only predicate that is
/// true on BOTH halves of a replicated pair.</b> Replication is one bidirectional
/// stream that exactly one side dials, so only the initiator sets
/// <c>LocalDb:Replication:PeerAddress</c>; the passive node has an <c>ApiKey</c> and
/// nothing else. (The rig is the reference: site-a node-a carries PeerAddress + ApiKey,
/// site-a node-b carries ApiKey alone, and site-b/site-c carry no <c>Replication</c>
/// section at all.) Keying on PeerAddress alone would strip capture from every passive
/// node — its local writes would stop reaching the initiator, and the pair would
/// silently converge in one direction only.
/// </para>
/// <para>
/// The keys are read straight from configuration rather than through
/// <c>IOptions&lt;ReplicationOptions&gt;</c> because this runs inside the
/// <c>AddZbLocalDb</c> factory, where resolving another option would nest a service
/// resolution inside a singleton construction. The section name matches what
/// <c>AddZbLocalDbReplication</c> binds, so the two cannot disagree about which node
/// replicates.
/// </para>
/// <para>
/// <b>Known residual:</b> a database file that was registered by an OLDER build keeps
/// its stale <c>__localdb_*</c> triggers — this only decides whether new ones are
/// installed, and the library has no removal API yet (it arrives with the WP3.3 library
/// work, which will also drop them on an unconfigured node). On the docker rig this is
/// moot: a schema change recreates the volumes. On a long-lived unreplicated node
/// upgraded in place, capture continues until that lands.
/// </para>
/// <para>
/// <b>The other direction has a consequence too:</b> turning replication ON for a site
/// that has been running without it does NOT baseline the rows already in the file.
/// Capture never recorded them in <c>__localdb_row_version</c>, and the snapshot resync
/// streams from that ledger, so the pair converges only on writes made after the
/// restart. Seed both nodes from one node's database if the existing rows matter. See
/// <c>docs/deployment/topology-guide.md</c>.
/// </para>
/// </remarks>
private static bool ReplicationIsConfigured(IConfiguration config)
{
var section = config.GetSection("LocalDb:Replication");
return !string.IsNullOrWhiteSpace(section["PeerAddress"])
|| !string.IsNullOrWhiteSpace(section["ApiKey"]);
}
}
tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteLocalDbCdcRegistrationTests.cs
+227
View File
@@ -0,0 +1,227 @@
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using ZB.MOM.WW.LocalDb;
namespace ZB.MOM.WW.ScadaBridge.Host.Tests;
/// <summary>
/// WP1.3 — CDC capture is installed only on a node that actually replicates.
/// </summary>
/// <remarks>
/// <para>
/// These assert on the TRIGGERS in <c>sqlite_master</c> rather than on
/// <c>ILocalDb.ReplicatedTables</c>, because the trigger set is what costs anything: the
/// registry entry is a dictionary lookup, while each trigger runs two extra INSERTs plus a
/// <c>json_object</c> serialization of the full row inside every write transaction on the
/// table. An unreplicated node paid that on every store-and-forward enqueue, every static
/// override, every event log row, forever, for an oplog with no reader.
/// </para>
/// <para>
/// The naming <c>__localdb_{table}_{ai|au|ad}</c> is the library's
/// (<c>TriggerSqlGenerator.TriggerName</c>), matched by prefix here so a fourth trigger kind
/// would be caught rather than quietly ignored.
/// </para>
/// </remarks>
public class SiteLocalDbCdcRegistrationTests : IDisposable
{
private readonly string _root;
private readonly List<ServiceProvider> _providers = [];
public SiteLocalDbCdcRegistrationTests()
{
_root = Path.Combine(Path.GetTempPath(), $"localdb-cdc-{Guid.NewGuid():N}");
Directory.CreateDirectory(_root);
}
public void Dispose()
{
foreach (var provider in _providers)
{
try { provider.Dispose(); } catch { /* best effort */ }
}
Microsoft.Data.Sqlite.SqliteConnection.ClearAllPools();
try { Directory.Delete(_root, recursive: true); } catch { /* best effort */ }
GC.SuppressFinalize(this);
}
[Fact]
public void UnreplicatedNode_InstallsNoCaptureTriggers()
{
// No LocalDb:Replication section at all — site-b and site-c on the rig.
var db = BuildDatabase(Config());
Assert.Empty(CaptureTriggers(db));
}
[Fact]
public void UnreplicatedNode_StillCreatesEveryTable()
{
// Skipping registration must not skip the DDL: the node is fully functional
// standalone, it just has nobody to ship its changes to. A guard placed around the
// schema block instead of the registration block would fail here and nowhere else.
var db = BuildDatabase(Config());
var tables = TableNames(db);
foreach (var table in AllSiteTables)
Assert.Contains(table, tables);
}
[Fact]
public void InitiatorNode_InstallsCaptureTriggers()
{
// PeerAddress + ApiKey — site-a node-a, the half that dials.
var db = BuildDatabase(Config(
peerAddress: "http://peer:8083", apiKey: "cdc-test-key"));
AssertCaptureTriggersCoverTheReplicatedTables(db);
}
[Fact]
public void PassiveNode_WithApiKeyButNoPeerAddress_StillInstallsCaptureTriggers()
{
// The reason the predicate is an OR. Site-a node-b sets ApiKey and nothing else:
// one bidirectional stream, dialled by one side. Keying on PeerAddress alone would
// leave this node capturing nothing, so its own writes would never reach the
// initiator and the pair would converge in one direction only — silently.
var db = BuildDatabase(Config(apiKey: "cdc-test-key"));
AssertCaptureTriggersCoverTheReplicatedTables(db);
}
[Fact]
public void ReplicatedNode_InstallsNoCaptureTriggersOnTheCentralOnlyNotificationTables()
{
// The security property, restated at the trigger level: notification_lists and
// smtp_configurations exist but must never be captured, because the only payload
// they ever historically held was plaintext SMTP passwords.
var db = BuildDatabase(Config(apiKey: "cdc-test-key"));
var triggers = CaptureTriggers(db);
Assert.DoesNotContain(triggers, t => t.StartsWith("__localdb_notification_lists_", StringComparison.Ordinal));
Assert.DoesNotContain(triggers, t => t.StartsWith("__localdb_smtp_configurations_", StringComparison.Ordinal));
}
[Fact]
public void UnreplicatedNode_StillRunsTheLegacyMigrator()
{
// The migrator is deliberately outside the guard. It renames the legacy file to
// ".migrated" on success, which is the observable proof it ran without needing a
// capture trigger to look at.
var legacyPath = Path.Combine(_root, "legacy-store-and-forward.db");
SeedLegacyStoreAndForward(legacyPath);
_ = BuildDatabase(Config(storeAndForwardPath: legacyPath));
Assert.False(File.Exists(legacyPath));
Assert.True(File.Exists(legacyPath + ".migrated"));
}
// ---- helpers ----------------------------------------------------------------------
/// <summary>The ten replicated tables plus the two deliberately-unregistered ones.</summary>
private static readonly string[] AllSiteTables =
[
"OperationTracking", "site_events", "sf_messages", "deployed_configurations",
"static_attribute_overrides", "shared_scripts", "external_systems",
"database_connections", "data_connection_definitions", "native_alarm_state",
"notification_lists", "smtp_configurations",
];
private static readonly string[] ReplicatedTables =
[
"OperationTracking", "site_events", "sf_messages", "deployed_configurations",
"static_attribute_overrides", "shared_scripts", "external_systems",
"database_connections", "data_connection_definitions", "native_alarm_state",
];
private static void AssertCaptureTriggersCoverTheReplicatedTables(ILocalDb db)
{
var triggers = CaptureTriggers(db);
foreach (var table in ReplicatedTables)
{
// All three kinds, so a partial install is a failure rather than a pass.
foreach (var suffix in new[] { "ai", "au", "ad" })
Assert.Contains($"__localdb_{table}_{suffix}", triggers);
}
}
private ILocalDb BuildDatabase(IConfiguration config)
{
var provider = new ServiceCollection()
.AddZbLocalDb(config, db => SiteLocalDbSetup.OnReady(db, config))
.BuildServiceProvider();
_providers.Add(provider);
return provider.GetRequiredService<ILocalDb>();
}
private IConfiguration Config(
string? peerAddress = null, string? apiKey = null, string? storeAndForwardPath = null)
{
var values = new Dictionary<string, string?>
{
["LocalDb:Path"] = Path.Combine(_root, "consolidated.db"),
["ScadaBridge:Node:NodeName"] = "node-a",
// Every legacy path is pinned inside this test's own directory. Left unset they
// resolve CWD-relative (./data/…), and the migrator RENAMES whatever it finds —
// so an unlucky run could eat a real file from the test binary's output folder.
["ScadaBridge:StoreAndForward:SqliteDbPath"] =
storeAndForwardPath ?? Path.Combine(_root, "absent-store-and-forward.db"),
["ScadaBridge:Database:SiteDbPath"] = Path.Combine(_root, "absent-scadabridge.db"),
["ScadaBridge:SiteEventLog:DatabasePath"] = Path.Combine(_root, "absent-events.db"),
["ScadaBridge:OperationTracking:ConnectionString"] =
$"Data Source={Path.Combine(_root, "absent-tracking.db")}",
};
if (peerAddress is not null) values["LocalDb:Replication:PeerAddress"] = peerAddress;
if (apiKey is not null) values["LocalDb:Replication:ApiKey"] = apiKey;
return new ConfigurationBuilder().AddInMemoryCollection(values).Build();
}
private static void SeedLegacyStoreAndForward(string path)
{
using var connection = new Microsoft.Data.Sqlite.SqliteConnection($"Data Source={path}");
connection.Open();
ZB.MOM.WW.ScadaBridge.StoreAndForward.StoreAndForwardSchema.Apply(connection);
}
/// <summary>
/// Every LocalDb capture trigger in the file. Matched on the library's
/// <c>__localdb_</c> prefix in C# rather than with SQL <c>LIKE</c>, where the underscores
/// are single-character wildcards and the pattern would need escaping to mean itself.
/// </summary>
private static HashSet<string> CaptureTriggers(ILocalDb db)
{
using var connection = db.CreateConnection();
using var cmd = connection.CreateCommand();
cmd.CommandText = "SELECT name FROM sqlite_master WHERE type = 'trigger'";
using var reader = cmd.ExecuteReader();
var names = new HashSet<string>(StringComparer.Ordinal);
while (reader.Read())
{
var name = reader.GetString(0);
if (name.StartsWith("__localdb_", StringComparison.Ordinal)) names.Add(name);
}
return names;
}
private static HashSet<string> TableNames(ILocalDb db)
{
using var connection = db.CreateConnection();
using var cmd = connection.CreateCommand();
cmd.CommandText = "SELECT name FROM sqlite_master WHERE type = 'table'";
using var reader = cmd.ExecuteReader();
var names = new HashSet<string>(StringComparer.Ordinal);
while (reader.Read()) names.Add(reader.GetString(0));
return names;
}
}
tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteLocalDbWiringTests.cs
+9 -2
View File
@@ -56,9 +56,16 @@ public class SiteLocalDbWiringTests : IDisposable
["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551",
["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@localhost:2552",
// The consolidated site database. No Replication section at all — this
// fixture is also the default-OFF pin: storage must work standalone.
// The consolidated site database, configured exactly like the PASSIVE half of
// the rig's replicated pair (site-a node-b): an ApiKey and no PeerAddress.
//
// The key is what makes this a replicating node, and therefore what makes the
// CDC registration assertions below apply at all — capture is installed only
// when replication is configured. The absent PeerAddress keeps this fixture the
// default-OFF pin at the same time: nothing dials, so the engine must still
// resolve and idle rather than throw or report a connection.
["LocalDb:Path"] = _tempDbPath,
["LocalDb:Replication:ApiKey"] = "wiring-test-localdb-sync-key",
});
builder.Services.AddGrpc();
tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/LocalDbSitePairHarness.cs
+6
View File
@@ -113,6 +113,12 @@ public abstract class LocalDbSitePairHarness : IAsyncLifetime
{
["LocalDb:Path"] = path,
["ScadaBridge:Node:NodeName"] = nodeName,
// OnReady installs the CDC capture triggers only on a node that has
// replication configured, so the key has to be here and not only in
// ReplicationConfig below — without it these nodes would run the sync
// engine over an oplog nothing ever writes to, and every convergence
// scenario would time out with two intact but unrelated databases.
["LocalDb:Replication:ApiKey"] = SharedApiKey,
// Point the legacy migrators at paths that do not exist, so they no-op rather
// than picking up stray files from the test working directory. The two Phase 2
// defaults matter most: unlike the Phase 1 pair they resolve inside ./data/,