diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/SiteLocalDbSetup.cs b/src/ZB.MOM.WW.ScadaBridge.Host/SiteLocalDbSetup.cs index bd89ff77..9d0aeb1e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/SiteLocalDbSetup.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/SiteLocalDbSetup.cs @@ -1,3 +1,4 @@ +using Microsoft.Data.Sqlite; using Microsoft.Extensions.Configuration; using ZB.MOM.WW.LocalDb; using ZB.MOM.WW.ScadaBridge.SiteEventLogging; @@ -234,4 +235,69 @@ public static class SiteLocalDbSetup return !string.IsNullOrWhiteSpace(section["PeerAddress"]) || !string.IsNullOrWhiteSpace(section["ApiKey"]); } + + /// + /// Sets auto_vacuum = INCREMENTAL on the consolidated LocalDb file — arch-review + /// 2026-08-19 P4-6 — but ONLY when that file does not exist yet. + /// + /// + /// + /// The defect this closes. SQLite only honors a change to auto_vacuum while + /// a database is still completely table-less; once one table exists the pragma is a + /// silent no-op. SqliteLocalDb's constructor runs LocalDbSchema.EnsureCreated + /// on its own master connection before — or anything else — ever + /// gets a connection to the file, so the pragma + /// used to issue had never once taken effect, in any real call path in this repo: by the + /// time it ran, LocalDb's own bookkeeping tables already existed. + /// + /// + /// The fix, and why it needs no LocalDb API change. AddZbLocalDb registers + /// as a LAZY singleton — nothing touches the file until it is + /// first resolved. This method is called from SiteServiceRegistration.Configure, + /// synchronously, BEFORE services.AddZbLocalDb(...) is even reached in that method + /// — i.e. well before the host is built and anything could resolve + /// — so it is free to open its own short-lived connection to the not-yet-existing file and + /// set the pragma there. Empirically verified (not merely inferred from the pragma's + /// documented "unless a subsequent operation modifies the database" caveat): opening a + /// connection to a fresh path and executing this single PRAGMA is sufficient by itself — + /// Microsoft.Data.Sqlite persists it to the file header on close, with no throwaway + /// CREATE/DROP TABLE and no VACUUM required. A LATER session — SqliteLocalDb's own + /// constructor, opening the SAME path — sees the flag already baked in and every table it + /// (and everyone downstream) subsequently creates lives under incremental-vacuum mode. + /// + /// + /// Only helps FRESH files — say so honestly, always. A file that already exists + /// already has tables, so this method deliberately does nothing to it: retrofitting + /// auto_vacuum onto a populated database needs SQLite's own VACUUM rebuild, which + /// plan decision 2 (2026-08-19) explicitly rejects running unconditionally against a live + /// ~1 GB file on every boot. Consequence: every node on the docker rig today, and any + /// site database created before this fix shipped, stays permanently + /// auto_vacuum=NONE — PRAGMA incremental_vacuum, the per-trim call in + /// , + /// remains a harmless no-op there until an operator opts into a one-time offline VACUUM. + /// Only a NEWLY created site database benefits, from the moment this ships onward. + /// + /// + /// Configuration, read for LocalDb:Path only. + internal static void EnsureAutoVacuumOnFreshFile(IConfiguration config) + { + var path = config["LocalDb:Path"]; + if (string.IsNullOrWhiteSpace(path)) + return; // AddZbLocalDb's own options validation reports a missing path loudly; nothing to pre-create here. + + if (File.Exists(path)) + return; // Already has tables (or is at least a pre-existing file) — the pragma cannot help it now. + + var directory = Path.GetDirectoryName(path); + if (!string.IsNullOrEmpty(directory)) + Directory.CreateDirectory(directory); + + var connectionString = new SqliteConnectionStringBuilder { DataSource = path }.ToString(); + using var connection = new SqliteConnection(connectionString); + connection.Open(); + + using var pragmaCmd = connection.CreateCommand(); + pragmaCmd.CommandText = "PRAGMA auto_vacuum = INCREMENTAL"; + pragmaCmd.ExecuteNonQuery(); + } } diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs b/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs index 9992f5dc..a3b5e54f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs @@ -91,6 +91,16 @@ public static class SiteServiceRegistration // failure, and the default site path is the relative "./data/site-localdb.db". // The shim is gone; SiteLocalDbDirectoryTests still pins the outcome here, since // what this host needs is the guarantee, not any particular owner of it. + // + // arch-review 2026-08-19 P4-6: MUST run before AddZbLocalDb — its singleton + // factory is lazy, but the ordering still has to be enforced here rather than + // relied upon, because it is what makes "PRAGMA auto_vacuum = INCREMENTAL" take + // effect at all. SQLite only honors that pragma on a table-less database, and + // AddZbLocalDb's own SqliteLocalDb constructor creates LocalDb's bookkeeping + // tables the moment it is first resolved — see + // SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile for the full mechanism, and note + // it only helps a file that does not exist yet. + SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile(config); services.AddZbLocalDb(config, db => SiteLocalDbSetup.OnReady(db, config)); // The replication engine, likewise unconditional but INERT by default: with no diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/EventLogPurgeService.cs b/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/EventLogPurgeService.cs index 947fa310..c81c8f49 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/EventLogPurgeService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/EventLogPurgeService.cs @@ -164,6 +164,19 @@ public class EventLogPurgeService : BackgroundService _options.RetentionDays, _options.MaxStorageMb, _options.PurgeInterval, _options.MaxPurgedRowsPerTick > 0 ? _options.MaxPurgedRowsPerTick.ToString() : "unlimited"); + // arch-review 2026-08-19 P4-5: yield BEFORE the initial (potentially ~17s + // ceiling-limited, ~285s unpaced worst case) RunPurge() rather than calling it + // as the first synchronous statement. .NET 8+'s BackgroundService already invokes + // ExecuteAsync via Task.Run, so a synchronous prelude here does not actually block + // IHostedService.StartAsync on this repo's net10.0 target (verified empirically — + // it did not reproduce). This Task.Yield() is still the correct fix rather than a + // no-op: it makes the "purge never blocks the calling thread" contract a property of + // THIS method, not an artifact of the host's scheduling internals — reflection-invoking + // ExecuteAsync directly (as EventLogPurgeServiceTests does, bypassing BackgroundService + // entirely) would otherwise still run the initial purge fully synchronously, and any + // future caller that awaits ExecuteAsync directly (not through StartAsync) would block. + await Task.Yield(); + // Run an initial purge on startup var ceilingHit = RunPurge(); @@ -213,9 +226,18 @@ public class EventLogPurgeService : BackgroundService } catch (Exception checkEx) { - _logger.LogDebug(checkEx, - "Active-node check threw during purge tick; treating as active for logging purposes"); - isActive = true; + // arch-review 2026-08-19 P4-5: fail CLOSED, not "active". This predicate no + // longer gates any work (P4-4 — both nodes purge their own file every tick + // regardless), so a wrong value here cannot skip a purge; but silently + // reporting "active" for a check that just threw is dishonest, and an + // operator correlating this line against the real active-node state would be + // misled into thinking the check succeeded. Warning (not Debug) because the + // check itself is broken and is worth noticing on its own. + _logger.LogWarning(checkEx, + "Active-node check threw during purge tick; logging this tick as inactive " + + "rather than claiming an active status the check could not confirm. The purge " + + "itself is unaffected — every node purges its own file regardless of this flag"); + isActive = false; } _logger.LogDebug("Running event log purge tick (active={IsActive})", isActive); @@ -513,9 +535,16 @@ public class EventLogPurgeService : BackgroundService """; var rows = cmd.ExecuteNonQuery(); - // Reclaim free pages so page_count/freelist measurement reflects the - // delete. Effective because auto_vacuum = INCREMENTAL is set at schema - // creation; harmless otherwise. + // Reclaim free pages so page_count/freelist measurement reflects the delete. + // arch-review 2026-08-19 P4-6: this is effective ONLY on a consolidated + // LocalDb file that was FRESH when SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile + // ran (Host/SiteLocalDbSetup.cs) — auto_vacuum can only be enabled before any + // table exists, and SqliteLocalDb's constructor creates LocalDb's own + // bookkeeping tables before this component ever gets a connection. Every file + // that predates that fix — including every node on the docker rig today — + // stays permanently auto_vacuum=NONE, and this PRAGMA is a harmless, silent + // no-op on it (retrofitting one requires an offline VACUUM, deliberately not + // attempted here against a live ~1 GB file — plan decision 2, 2026-08-19). using var vacuumCmd = connection.CreateCommand(); vacuumCmd.CommandText = "PRAGMA incremental_vacuum"; vacuumCmd.ExecuteNonQuery(); diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/SiteEventLogSchema.cs b/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/SiteEventLogSchema.cs index cde099b0..6c778946 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/SiteEventLogSchema.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/SiteEventLogSchema.cs @@ -36,15 +36,24 @@ public static class SiteEventLogSchema { ArgumentNullException.ThrowIfNull(connection); - // auto_vacuum must be set before any table is created for it to take effect - // on a fresh database. With INCREMENTAL mode, PRAGMA incremental_vacuum can - // later reclaim free pages so the storage-cap purge can shrink the file. - using (var pragmaCmd = connection.CreateCommand()) - { - pragmaCmd.CommandText = "PRAGMA auto_vacuum = INCREMENTAL"; - pragmaCmd.ExecuteNonQuery(); - } - + // arch-review 2026-08-19 P4-6: this method used to also set + // "PRAGMA auto_vacuum = INCREMENTAL" here, on the theory that setting it right + // before CREATE TABLE would make it take effect on a fresh database. That was + // wrong in every real call path: SQLite only honors an auto_vacuum change on a + // database that has NO tables yet, and by the time this method runs — via the + // Host's AddZbLocalDb onReady callback (SiteLocalDbSetup.OnReady) OR via + // SiteEventLogger's own InitializeSchema fallback for directly-constructed + // instances — the consolidated file already holds LocalDb's own bookkeeping + // tables (SqliteLocalDb's constructor runs LocalDbSchema.EnsureCreated on its + // master connection before either caller ever gets a connection). So the pragma + // here had never once taken effect; it was a silent, permanent no-op. The real + // fix now lives in Host/SiteLocalDbSetup.cs + // (EnsureAutoVacuumOnFreshFile), which pre-creates a genuinely fresh LocalDb file + // with the pragma set BEFORE AddZbLocalDb's lazy singleton ever constructs + // SqliteLocalDb — and, being a Host-layer, one-time, pre-file-existence step, it + // helps ONLY brand-new site databases; every file created before this fix shipped + // stays auto_vacuum=NONE forever (see the PurgeByStorageCap remarks in + // EventLogPurgeService for the operational consequence). using var cmd = connection.CreateCommand(); cmd.CommandText = """ CREATE TABLE IF NOT EXISTS site_events ( diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteLocalDbAutoVacuumTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteLocalDbAutoVacuumTests.cs new file mode 100644 index 00000000..bc4839a2 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteLocalDbAutoVacuumTests.cs @@ -0,0 +1,171 @@ +using Microsoft.Data.Sqlite; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.LocalDb; + +namespace ZB.MOM.WW.ScadaBridge.Host.Tests; + +/// +/// Arch-review 2026-08-19 P4-6 — PRAGMA auto_vacuum = INCREMENTAL only takes effect on +/// a table-less database, and SqliteLocalDb's constructor creates LocalDb's own +/// bookkeeping tables before anything else ever gets a connection to the consolidated site +/// file. is the fix: it pre-creates +/// a genuinely fresh file with the pragma set BEFORE AddZbLocalDb's lazy singleton ever +/// constructs SqliteLocalDb. +/// +/// +/// These tests exercise directly +/// (it is internal, visible here via InternalsVisibleTo) rather than through the +/// full composition root — the mechanism under test is a +/// pure ordering/pragma concern that does not depend on the rest of the site graph, and +/// already proves the composition root wires +/// SiteLocalDbSetup in at all. +/// +public class SiteLocalDbAutoVacuumTests +{ + [Fact] + public void FreshFile_ReportsIncrementalAutoVacuum() + { + // The plan's own acceptance test: a fresh file reports PRAGMA auto_vacuum = 2 + // (INCREMENTAL) after the pre-create step runs. + var path = TempDbPath(); + try + { + var config = ConfigFor(path); + + SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile(config); + + Assert.Equal(2L, ReadAutoVacuum(path)); + } + finally + { + Cleanup(path); + } + } + + [Fact] + public void FreshFile_AutoVacuumSurvives_LocalDbCreatingItsOwnBookkeepingTablesAfterward() + { + // The real end-to-end claim: the pre-create step has to run BEFORE AddZbLocalDb ever + // constructs SqliteLocalDb (which creates LocalDb's own tables first, unconditionally, + // in its constructor) for the ordering fix to mean anything. This proves the flag + // baked in by EnsureAutoVacuumOnFreshFile survives that later table creation — the + // exact sequence SiteServiceRegistration.Configure now runs in production. + var path = TempDbPath(); + try + { + var config = ConfigFor(path); + + SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile(config); + + var services = new ServiceCollection(); + services.AddZbLocalDb(config, db => SiteLocalDbSetup.OnReady(db, config)); + using var provider = services.BuildServiceProvider(); + using var scope = provider.CreateScope(); + + // Resolving ILocalDb is what triggers SqliteLocalDb's constructor (LocalDbSchema + // .EnsureCreated) and then OnReady (SiteEventLogSchema.Apply, etc.) — i.e. every + // real table this file will ever hold. This config carries no + // LocalDb:Replication section, so OnReady takes the deregistration branch and + // db.ReplicatedTables stays empty — check the raw file instead to confirm the + // table genuinely exists. + var db = scope.ServiceProvider.GetRequiredService(); + Assert.NotNull(db); + Assert.Contains("site_events", TableNames(path)); + + Assert.Equal(2L, ReadAutoVacuum(path)); + } + finally + { + SqliteConnection.ClearAllPools(); + Cleanup(path); + } + } + + [Fact] + public void ExistingFile_IsLeftAlone_AutoVacuumStaysNone() + { + // Honesty requirement (plan decision 2): the fix only helps a file that does not exist + // yet. A file that already has tables — every node on the docker rig today, and any + // site database created before this fix shipped — is untouched, and stays + // auto_vacuum = NONE forever short of an offline VACUUM this task deliberately does + // not attempt. + var path = TempDbPath(); + try + { + // Pre-populate the file exactly like a real (pre-fix) LocalDb file: at least one + // table exists before EnsureAutoVacuumOnFreshFile ever runs. + using (var seed = new SqliteConnection(new SqliteConnectionStringBuilder { DataSource = path }.ToString())) + { + seed.Open(); + using var cmd = seed.CreateCommand(); + cmd.CommandText = "CREATE TABLE legacy_marker (id INTEGER PRIMARY KEY)"; + cmd.ExecuteNonQuery(); + } + + Assert.Equal(0L, ReadAutoVacuum(path)); // NONE, the SQLite default. + + var config = ConfigFor(path); + SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile(config); + + // Still NONE — the method must not attempt VACUUM against a populated file. + Assert.Equal(0L, ReadAutoVacuum(path)); + } + finally + { + SqliteConnection.ClearAllPools(); + Cleanup(path); + } + } + + [Fact] + public void MissingLocalDbPath_DoesNotThrow() + { + // AddZbLocalDb's own options validation is the authoritative place a missing path is + // reported; this pre-create step must defer to it rather than throwing its own, + // differently-worded error first. + var config = new ConfigurationBuilder().AddInMemoryCollection( + new Dictionary()).Build(); + + var ex = Record.Exception(() => SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile(config)); + + Assert.Null(ex); + } + + private static string TempDbPath() => + Path.Combine(Path.GetTempPath(), $"scadabridge_autovacuum_{Guid.NewGuid():N}.db"); + + private static IConfiguration ConfigFor(string path) => + new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { ["LocalDb:Path"] = path }) + .Build(); + + private static long ReadAutoVacuum(string path) + { + using var connection = new SqliteConnection(new SqliteConnectionStringBuilder { DataSource = path }.ToString()); + connection.Open(); + using var cmd = connection.CreateCommand(); + cmd.CommandText = "PRAGMA auto_vacuum"; + return Convert.ToInt64(cmd.ExecuteScalar()); + } + + private static HashSet TableNames(string path) + { + using var connection = new SqliteConnection(new SqliteConnectionStringBuilder { DataSource = path }.ToString()); + connection.Open(); + using var cmd = connection.CreateCommand(); + cmd.CommandText = "SELECT name FROM sqlite_master WHERE type = 'table'"; + using var reader = cmd.ExecuteReader(); + var names = new HashSet(StringComparer.Ordinal); + while (reader.Read()) names.Add(reader.GetString(0)); + return names; + } + + private static void Cleanup(string path) + { + foreach (var suffix in new[] { "", "-wal", "-shm" }) + { + try { File.Delete(path + suffix); } catch { /* best effort */ } + } + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteEventLogging.Tests/EventLogPurgeServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteEventLogging.Tests/EventLogPurgeServiceTests.cs index 0b61260a..325b59b3 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteEventLogging.Tests/EventLogPurgeServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteEventLogging.Tests/EventLogPurgeServiceTests.cs @@ -1,3 +1,4 @@ +using System.Reflection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; @@ -355,6 +356,92 @@ public class EventLogPurgeServiceTests : IDisposable await purge.StopAsync(CancellationToken.None); } + [Fact] + public async Task ExecuteAsync_YieldsBeforeRunningTheInitialPurgeSynchronously() + { + // arch-review 2026-08-19 P4-5 — timing-independent companion to + // StartAsync_DoesNotBlock_OnTheInitialPurge above. + // + // That test times BackgroundService.StartAsync as a whole and passes even on the + // UNFIXED code, because .NET 8+ already invokes ExecuteAsync via Task.Run — the + // synchronous prelude runs on a pool thread, not the caller's, so StartAsync itself + // never blocks regardless of whether ExecuteAsync yields first. Verified empirically + // against this repo's actual net10.0 target (a BackgroundService whose ExecuteAsync + // does Thread.Sleep(1500) before its first await still returns from StartAsync in + // ~2 ms). So P4-5's literal "StartAsync blocks" symptom does not reproduce here, and + // a wall-clock test cannot prove the fix. + // + // What CAN be proven without any timing is the ExecuteAsync-level contract itself: + // invoking the protected ExecuteAsync directly via reflection — bypassing + // BackgroundService's Task.Run wrapper entirely — still runs it synchronously, on + // the calling thread, up through its first genuinely-suspending await (that is what + // "async Task method" means; MethodInfo.Invoke does not change it). On the unfixed + // code ExecuteAsync's first statement is RunPurge() with no await before it, so + // Invoke would not return until the initial purge had fully run — deleting the + // expired row before this method's Assert ever executes. With Task.Yield() as the + // very first statement, Invoke returns as soon as MoveNext() hits the (always + // IsCompleted == false) yield point, before RunPurge has been called at all. + InsertEventWithTimestamp(DateTimeOffset.UtcNow.AddDays(-31)); + Assert.Equal(1, GetEventCount()); + + var purge = CreatePurgeService(); + var executeAsync = typeof(EventLogPurgeService).GetMethod( + "ExecuteAsync", BindingFlags.NonPublic | BindingFlags.Instance); + Assert.NotNull(executeAsync); + + using var cts = new CancellationTokenSource(); + var task = (Task)executeAsync!.Invoke(purge, [cts.Token])!; + + // Reflection Invoke has already returned control here — a synchronous + // RunPurge() ahead of any await would have already deleted the row by now. + Assert.Equal(1, GetEventCount()); + + // The initial purge still runs, just after control has been yielded back. + var deadline = DateTime.UtcNow.AddSeconds(5); + while (GetEventCount() != 0 && DateTime.UtcNow < deadline) + { + await Task.Delay(25); + } + Assert.Equal(0, GetEventCount()); + + await cts.CancelAsync(); + try + { + await task; + } + catch (OperationCanceledException) + { + // Expected: the loop's Task.Delay observes the cancellation. + } + } + + [Fact] + public void RunPurge_ActiveNodeCheckThrows_TickLogsAsInactive_NotActive() + { + // arch-review 2026-08-19 P4-5: a throwing isActive predicate must fail CLOSED + // (logged as inactive) rather than silently claiming "active". The predicate no + // longer gates any purge work (P4-4 — every node purges its own file every tick + // regardless), so a wrong value here cannot skip work; but an operator correlating + // this line against the real active-node state must not be misled into thinking the + // check actually succeeded and reported active. + var logger = new CapturingLogger(); + var purge = CreatePurgeService( + isActiveNode: () => throw new InvalidOperationException("boom"), + logger: logger); + + purge.RunPurge(); + + var warnLine = Assert.Single( + logger.Entries, + e => e.Message.Contains("Active-node check threw", StringComparison.Ordinal)); + Assert.Equal(LogLevel.Warning, warnLine.Level); + + var tickLine = Assert.Single( + logger.Entries, + e => e.Message.Contains("Running event log purge tick", StringComparison.Ordinal)); + Assert.Contains("active=False", tickLine.Message, StringComparison.Ordinal); + } + [Fact] public async Task PurgeByStorageCap_ConcurrentWritesDoNotCorruptConnection() {