test(localdb): deregister preserves data + partial-flip fails loudly (review round)

Review round on 3e62f161. Two coverage gaps and three wording/comment nits.

IMPORTANT 1 — deregistration is now proven DATA-PRESERVING. The self-heal test
inserted a deployment_pointer row and rebooted unconfigured but never looked at
the row again, so a library regression that touched base-table data during
cleanup would have gone unnoticed — on the very tables that ARE the
boot-from-cache backstop, and on the default-OFF nodes least likely to notice.
The new DisablingReplication_PrunesBookkeepingButNeverTouchesTheCachedDataItself
asserts the row's CONTENT survives (not just its count — a count passes against
a cleanup that blanks columns) while __localdb_row_version and __localdb_oplog,
the bookkeeping deregistration does own, go to zero.

IMPORTANT 2 — mid-loop partial failure is documented and pinned. Both loops flip
four tables with no try/catch, so a throw on the third leaves two flipped and
propagates out of the AddZbLocalDb singleton factory, failing host start. That
is correct — a node that came up anyway would be silently capturing on an
arbitrary subset — and both library calls are idempotent, so the retried boot
completes the flip. Written into OnReady's remarks and pinned by
AFailureMidwayThroughRegistration_FailsBootLoudly_AndTheRetriedBootConverges,
which drives OnReady through a delegating ILocalDb that throws on the Nth
RegisterReplicated, asserts the throw propagates, asserts the wrapper actually
reached that call, then re-runs OnReady unwrapped and asserts all four converge.

MINOR 3 — the Boot helper now records that OnReady's null-logger fallback lands
on Serilog's static Log.Logger, which this assembly never assigns; assertions
about logging pass an explicit logger.
MINOR 5 — log wording: "are both unset" -> "are both absent — unset or blank",
matching IsNullOrWhiteSpace.
MINOR 4 — LocalDbDeploymentArtifactCacheTests said its setup "mirrors
LocalDbSetup.OnReady"; it reproduces the two tables only and registers them
unconditionally without baselining. Softened to say so and to name the
divergence as intentional and inert there.

Falsified both new pins. Wrapper set to failOnCall: 99 (never throws) => the
partial-flip pin is the SOLE failure, so it genuinely exercises the Nth-call
throw path rather than some incidental exception. Predicate forced constant-true
=> five pins red including the new data-preservation one. Both mutations
reverted.

LocalDbSetupTests 15/15. Host.IntegrationTests 226 passed / 0 failed / 5
skipped. Runtime.Tests LocalDbDeploymentArtifactCacheTests 12/12.
This commit is contained in:
Joseph Doherty
2026-08-18 07:42:06 -04:00
parent 3e62f1617b
commit df53391646
3 changed files with 134 additions and 9 deletions
@@ -67,6 +67,14 @@ public static class LocalDbSetup
/// precisely the node that has no peer to capture for.
/// </para>
/// <para>
/// <b>A failure part-way through either loop fails host start, and that is deliberate.</b>
/// Neither branch catches, so a throw on the third of four tables leaves two flipped and
/// propagates out of the <c>AddZbLocalDb</c> singleton factory — a node that came up
/// anyway would be capturing (or not) on an arbitrary subset, which is silently partial
/// replication with nothing in the log to say so. Both library calls are idempotent, so
/// the retried boot simply completes the flip rather than compounding it.
/// </para>
/// <para>
/// The alarm buffer's tables are created unconditionally, regardless of whether this
/// node has <c>AlarmHistorian:Enabled</c> set, and regardless of replication. Creating
/// one lazily when the sink first appears would mean a node that enables the historian
@@ -229,7 +237,8 @@ public static class LocalDbSetup
{
logger.Information(
"LocalDb: removed stale change-capture from {TableCount} table(s) — this node has no "
+ "replication configured ({PeerAddressKey}/{ApiKeyKey} are both unset), so the capture "
+ "replication configured ({PeerAddressKey}/{ApiKeyKey} are both absent — unset or blank), "
+ "so the capture "
+ "triggers an earlier build installed have been dropped and their oplog/row-version "
+ "rows pruned.",
cleaned,
@@ -6,6 +6,7 @@ using Serilog.Events;
using Shouldly;
using Xunit;
using ZB.MOM.WW.LocalDb;
using ZB.MOM.WW.LocalDb.Registration;
using ZB.MOM.WW.OtOpcUa.Host.Configuration;
namespace ZB.MOM.WW.OtOpcUa.Host.IntegrationTests;
@@ -70,10 +71,20 @@ public sealed class LocalDbSetupTests : IDisposable
/// Boots <see cref="LocalDbSetup.OnReady"/> over the test's database file.
/// </summary>
/// <remarks>
/// Successive calls reopen the <b>same</b> file, which is how the self-heal and late-opt-in
/// cases are expressed: boot once under one replication configuration, write rows, then boot
/// again under another. That is exactly the sequence an operator performs by editing config
/// and restarting the node.
/// <para>
/// Successive calls reopen the <b>same</b> file, which is how the self-heal and
/// late-opt-in cases are expressed: boot once under one replication configuration, write
/// rows, then boot again under another. That is exactly the sequence an operator
/// performs by editing config and restarting the node.
/// </para>
/// <para>
/// A null <paramref name="logger"/> lets <c>OnReady</c> fall back to Serilog's static
/// <c>Log.Logger</c>, which in this assembly is never assigned and is therefore the
/// library's silent default. That is a load-bearing assumption for the two cases that
/// DO pass a logger: were anything here to install a static logger, they would still
/// pass (they assert on their own sink) but the silent cases would start writing to it.
/// Pass a logger whenever the assertion is about logging.
/// </para>
/// </remarks>
private ILocalDb Boot(
KeyValuePair<string, string?>[]? replication = null,
@@ -294,6 +305,36 @@ public sealed class LocalDbSetupTests : IDisposable
.ShouldBe(0);
}
[Fact]
public async Task DisablingReplication_PrunesBookkeepingButNeverTouchesTheCachedDataItself()
{
// Deregistration prunes the table's oplog and row-version rows — BOOKKEEPING. It must not
// touch the base table, because deployment_pointer/deployment_artifacts ARE the
// boot-from-cache backstop: a node whose central SQL is unreachable serves its address space
// from exactly these rows. A library regression that cleaned the base table along with the
// ledger would take that safety net out silently, on the very nodes that never replicate
// (central, site-b) and so would never notice a replication fault either.
var replicated = Boot(InitiatorReplication);
await InsertPointerAsync(replicated, "SITE-A");
(await ScalarAsync(replicated, "SELECT COUNT(*) FROM __localdb_row_version")).ShouldBeGreaterThan(0);
var unreplicated = Boot();
// The row survives, and with its content intact — a count alone would pass against a
// cleanup that blanked the columns it does not own.
var pointers = await unreplicated.QueryAsync(
"SELECT cluster_id || '|' || deployment_id || '|' || revision_hash FROM deployment_pointer",
static r => r.GetString(0),
parameters: null,
TestContext.Current.CancellationToken);
pointers.ShouldBe([$"SITE-A|0123456789abcdef0123456789abcdef|{new string('a', 64)}"]);
// …while the bookkeeping it DOES own is gone.
(await ScalarAsync(unreplicated, "SELECT COUNT(*) FROM __localdb_row_version")).ShouldBe(0);
(await ScalarAsync(unreplicated, "SELECT COUNT(*) FROM __localdb_oplog")).ShouldBe(0);
}
[Fact]
public async Task DisablingReplication_LogsOnceWhenItActuallyCleanedSomething()
{
@@ -321,6 +362,77 @@ public sealed class LocalDbSetupTests : IDisposable
sink.Events.ShouldBeEmpty();
}
[Fact]
public void AFailureMidwayThroughRegistration_FailsBootLoudly_AndTheRetriedBootConverges()
{
// OnReady flips four tables in a loop with no try/catch, so a failure on the third leaves
// two registered and two not. That is the RIGHT behaviour and this pins both halves of why:
// the throw propagates out of the AddZbLocalDb singleton factory and fails host start
// (a node half-capturing is a node whose replication is silently partial), and because both
// library APIs are idempotent the retried boot completes the flip rather than compounding it.
//
// Swallowing the throw would leave the node up, serving, and replicating two of four tables
// with nothing in the log to say so.
var real = Boot(InitiatorReplication);
var configuration = new ConfigurationBuilder()
.AddInMemoryCollection([new KeyValuePair<string, string?>("LocalDb:Path", _dbPath), .. InitiatorReplication])
.Build();
var faulty = new FailsOnNthRegistration(real, failOnCall: 3);
Should.Throw<InvalidOperationException>(() => LocalDbSetup.OnReady(faulty, configuration));
// The wrapper really did reach the third call — otherwise the assertion above would be
// pinning some unrelated throw, and the "partial flip" it claims to model never happened.
faulty.Calls.ShouldBe(3);
// The retry: same file, no wrapper. Idempotent re-registration converges on all four.
LocalDbSetup.OnReady(real, configuration);
real.ReplicatedTables.Keys.OrderBy(k => k, StringComparer.Ordinal)
.ShouldBe(["alarm_condition_state", "alarm_sf_events", "deployment_artifacts", "deployment_pointer"]);
}
/// <summary>
/// Passes everything through to a real database except the <c>failOnCall</c>-th
/// <c>RegisterReplicated</c>, which throws — the mid-loop partial-flip shape.
/// </summary>
private sealed class FailsOnNthRegistration(ILocalDb inner, int failOnCall) : ILocalDb
{
/// <summary>How many registrations were attempted, successful or not.</summary>
public int Calls { get; private set; }
public IReadOnlyDictionary<string, ReplicatedTable> ReplicatedTables => inner.ReplicatedTables;
public SqliteConnection CreateConnection() => inner.CreateConnection();
public Task<int> ExecuteAsync(string sql, object? parameters, CancellationToken cancellationToken) =>
inner.ExecuteAsync(sql, parameters, cancellationToken);
public Task<IReadOnlyList<T>> QueryAsync<T>(
string sql, Func<SqliteDataReader, T> map, object? parameters, CancellationToken cancellationToken) =>
inner.QueryAsync(sql, map, parameters, cancellationToken);
public Task<ILocalDbTransaction> BeginTransactionAsync(CancellationToken cancellationToken) =>
inner.BeginTransactionAsync(cancellationToken);
public ReplicatedTable RegisterReplicated(string tableName) =>
RegisterReplicated(tableName, baselineExistingRows: false);
public ReplicatedTable RegisterReplicated(string tableName, bool baselineExistingRows)
{
Calls++;
if (Calls == failOnCall)
throw new InvalidOperationException($"simulated registration failure on '{tableName}'");
return inner.RegisterReplicated(tableName, baselineExistingRows);
}
public bool DeregisterReplicated(string tableName) => inner.DeregisterReplicated(tableName);
}
private static Serilog.ILogger BuildLogger(ILogEventSink sink) =>
new Serilog.LoggerConfiguration().MinimumLevel.Verbose().WriteTo.Sink(sink).CreateLogger();
@@ -23,10 +23,14 @@ namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.DeploymentCache;
/// semantics — live entirely in SQL, so a mocked seam would assert nothing about them.
/// </para>
/// <para>
/// This project cannot reference the Host, so the schema setup here mirrors
/// <c>LocalDbSetup.OnReady</c> rather than calling it. The load-bearing
/// <c>DDL → RegisterReplicated</c> ordering is pinned by the Host's own
/// <c>LocalDbSetupTests</c>; what matters here is only that the tables exist.
/// This project cannot reference the Host, so the setup here reproduces only the two
/// cache <b>tables</b> — not <c>LocalDbSetup.OnReady</c>'s behaviour. In particular it
/// registers them unconditionally and without baselining, whereas the production callback
/// registers only when the node is configured to replicate and deregisters when it is not.
/// That divergence is intentional and inert here: these tests need a database whose tables
/// exist and capture, and nothing below asserts anything about registration. The
/// load-bearing DDL-then-RegisterReplicated ordering and the conditional-registration rules
/// are pinned by the Host's own <c>LocalDbSetupTests</c>.
/// </para>
/// </remarks>
public sealed class LocalDbDeploymentArtifactCacheTests : IDisposable