From 2e4e41a8f73da995d6b390dd33035175da27d4cc Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 14 Aug 2026 20:13:31 -0400 Subject: [PATCH] fix(auditlog): site audit DB onto the data volume; required path + soft flush MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes WP1.2 of the arch-review remediation plan (finding #2, High): SqliteAuditWriterOptions.DatabasePath defaulted to CWD-relative "auditlog.db", which on the docker rig resolves onto the container's ephemeral overlayfs (not the mounted /app/data volume), silently discarding the pending audit forward-state backlog on every recreate; nothing in docker/ or docker-env2/ overrode it; FlushIntervalMs was validated but never read by the writer loop (one commit per event even at trickle rate); and no PRAGMA synchronous was set (SQLite's FULL default fsyncs every commit). - DatabasePath now has no default (mirrors ZB.MOM.WW.LocalDb's LocalDbOptions.Path) and is required pre-host for Site nodes only, via a new StartupValidator raw-config check (top-level "AuditLog:SiteWriter:DatabasePath", NOT nested under ScadaBridge: AddAuditLog binds that section off the configuration root). SqliteAuditWriterOptionsValidator deliberately does NOT check DatabasePath itself, because AddAuditLog runs its ValidateOnStart on both Central and Site composition roots but only Site nodes ever resolve the writer — checking it there would fail Central's boot too. - All 8 site-node appsettings under docker/ and docker-env2/ now set AuditLog:SiteWriter:DatabasePath to /app/data/auditlog.db (mounted volume, survives container recreate, same convention as LocalDb:Path); the local-dev base appsettings.Site.json sets ./data/auditlog.db to match. - The writer loop now honors FlushIntervalMs: after draining the immediately available burst, it keeps the transaction open (bounded by FlushIntervalMs from the first event) waiting for more trickle-rate events before committing, instead of flushing (and fsyncing) per event. - PRAGMA synchronous = NORMAL on the write connection — audit is best-effort by design (CLAUDE.md: "Audit-write failure NEVER aborts the user-facing action"), so NORMAL's narrower power-loss window is an acceptable trade for far fewer fsyncs; WAL mode still guarantees no corruption. - Tests: StartupValidator site-required/blank/central-exempt cases; writer trickle-load single-transaction coalescing + beyond-interval separate-transaction regression (new FlushCountForTests seam); options-validator doc updates reflecting the moved responsibility. Full suite runs green: AuditLog.Tests 368/368, Host.Tests 480/480. One-time migration note: the existing container-local auditlog.db (wherever it landed under CWD) is abandoned by this change, not migrated — already-forwarded rows are safe centrally (AuditLog is the durable copy), and any still-Pending rows on the abandoned path are lost once. This is the exact bug being fixed, not a new loss: those rows were already living outside the mounted volume and would not have survived the next container recreate regardless. Cross-reference docs/known-issues/2026-07-20-cached-telemetry-drain-hot-loop.md, which this placement bug caused. --- .../site-x-node-a/appsettings.Site.json | 11 +++ .../site-x-node-b/appsettings.Site.json | 11 +++ docker/site-a-node-a/appsettings.Site.json | 11 +++ docker/site-a-node-b/appsettings.Site.json | 11 +++ docker/site-b-node-a/appsettings.Site.json | 11 +++ docker/site-b-node-b/appsettings.Site.json | 11 +++ docker/site-c-node-a/appsettings.Site.json | 11 +++ docker/site-c-node-b/appsettings.Site.json | 11 +++ .../Site/SqliteAuditWriter.cs | 74 ++++++++++++++++++- .../Site/SqliteAuditWriterOptions.cs | 19 ++++- .../Site/SqliteAuditWriterOptionsValidator.cs | 16 ++-- .../StartupValidator.cs | 22 ++++++ .../appsettings.Site.json | 9 +++ .../SqliteAuditWriterOptionsValidatorTests.cs | 24 +++--- .../Site/SqliteAuditWriterWriteTests.cs | 70 +++++++++++++++++- .../SiteHealthEndpointTests.cs | 8 ++ .../StartupValidatorTests.cs | 41 ++++++++++ 17 files changed, 347 insertions(+), 24 deletions(-) diff --git a/docker-env2/site-x-node-a/appsettings.Site.json b/docker-env2/site-x-node-a/appsettings.Site.json index 25262f90..9caef4df 100644 --- a/docker-env2/site-x-node-a/appsettings.Site.json +++ b/docker-env2/site-x-node-a/appsettings.Site.json @@ -78,5 +78,16 @@ // Replication is opt-in and configured separately; absent = local-only. "LocalDb": { "Path": "/app/data/site-localdb.db" + }, + // arch-review remediation WP1.2: the site hot-path audit writer's SQLite file has no + // default path (mirrors LocalDb:Path above) and StartupValidator now requires it for + // Site nodes — an unset value used to fall back to a bare "auditlog.db" resolved + // relative to CWD, i.e. the container's ephemeral overlayfs, so the pending audit + // backlog was silently discarded on every recreate. On the mounted /app/data volume, + // same as LocalDb:Path, so it survives container recreate. + "AuditLog": { + "SiteWriter": { + "DatabasePath": "/app/data/auditlog.db" + } } } diff --git a/docker-env2/site-x-node-b/appsettings.Site.json b/docker-env2/site-x-node-b/appsettings.Site.json index 586d1f5a..a45cb574 100644 --- a/docker-env2/site-x-node-b/appsettings.Site.json +++ b/docker-env2/site-x-node-b/appsettings.Site.json @@ -78,5 +78,16 @@ // Replication is opt-in and configured separately; absent = local-only. "LocalDb": { "Path": "/app/data/site-localdb.db" + }, + // arch-review remediation WP1.2: the site hot-path audit writer's SQLite file has no + // default path (mirrors LocalDb:Path above) and StartupValidator now requires it for + // Site nodes — an unset value used to fall back to a bare "auditlog.db" resolved + // relative to CWD, i.e. the container's ephemeral overlayfs, so the pending audit + // backlog was silently discarded on every recreate. On the mounted /app/data volume, + // same as LocalDb:Path, so it survives container recreate. + "AuditLog": { + "SiteWriter": { + "DatabasePath": "/app/data/auditlog.db" + } } } diff --git a/docker/site-a-node-a/appsettings.Site.json b/docker/site-a-node-a/appsettings.Site.json index 47877e02..3fb91088 100644 --- a/docker/site-a-node-a/appsettings.Site.json +++ b/docker/site-a-node-a/appsettings.Site.json @@ -120,5 +120,16 @@ "MaxOplogRows": 250000, "MaxOplogAge": "2.00:00:00" } + }, + // arch-review remediation WP1.2: the site hot-path audit writer's SQLite file has no + // default path (mirrors LocalDb:Path above) and StartupValidator now requires it for + // Site nodes — an unset value used to fall back to a bare "auditlog.db" resolved + // relative to CWD, i.e. the container's ephemeral overlayfs, so the pending audit + // backlog was silently discarded on every recreate. On the mounted /app/data volume, + // same as LocalDb:Path, so it survives container recreate. + "AuditLog": { + "SiteWriter": { + "DatabasePath": "/app/data/auditlog.db" + } } } diff --git a/docker/site-a-node-b/appsettings.Site.json b/docker/site-a-node-b/appsettings.Site.json index 676696a6..bfce17f5 100644 --- a/docker/site-a-node-b/appsettings.Site.json +++ b/docker/site-a-node-b/appsettings.Site.json @@ -113,5 +113,16 @@ "MaxOplogRows": 250000, "MaxOplogAge": "2.00:00:00" } + }, + // arch-review remediation WP1.2: the site hot-path audit writer's SQLite file has no + // default path (mirrors LocalDb:Path above) and StartupValidator now requires it for + // Site nodes — an unset value used to fall back to a bare "auditlog.db" resolved + // relative to CWD, i.e. the container's ephemeral overlayfs, so the pending audit + // backlog was silently discarded on every recreate. On the mounted /app/data volume, + // same as LocalDb:Path, so it survives container recreate. + "AuditLog": { + "SiteWriter": { + "DatabasePath": "/app/data/auditlog.db" + } } } diff --git a/docker/site-b-node-a/appsettings.Site.json b/docker/site-b-node-a/appsettings.Site.json index 84b77600..e5701e9e 100644 --- a/docker/site-b-node-a/appsettings.Site.json +++ b/docker/site-b-node-a/appsettings.Site.json @@ -86,5 +86,16 @@ // Replication is opt-in and configured separately; absent = local-only. "LocalDb": { "Path": "/app/data/site-localdb.db" + }, + // arch-review remediation WP1.2: the site hot-path audit writer's SQLite file has no + // default path (mirrors LocalDb:Path above) and StartupValidator now requires it for + // Site nodes — an unset value used to fall back to a bare "auditlog.db" resolved + // relative to CWD, i.e. the container's ephemeral overlayfs, so the pending audit + // backlog was silently discarded on every recreate. On the mounted /app/data volume, + // same as LocalDb:Path, so it survives container recreate. + "AuditLog": { + "SiteWriter": { + "DatabasePath": "/app/data/auditlog.db" + } } } diff --git a/docker/site-b-node-b/appsettings.Site.json b/docker/site-b-node-b/appsettings.Site.json index 3ca78cd1..785f9642 100644 --- a/docker/site-b-node-b/appsettings.Site.json +++ b/docker/site-b-node-b/appsettings.Site.json @@ -86,5 +86,16 @@ // Replication is opt-in and configured separately; absent = local-only. "LocalDb": { "Path": "/app/data/site-localdb.db" + }, + // arch-review remediation WP1.2: the site hot-path audit writer's SQLite file has no + // default path (mirrors LocalDb:Path above) and StartupValidator now requires it for + // Site nodes — an unset value used to fall back to a bare "auditlog.db" resolved + // relative to CWD, i.e. the container's ephemeral overlayfs, so the pending audit + // backlog was silently discarded on every recreate. On the mounted /app/data volume, + // same as LocalDb:Path, so it survives container recreate. + "AuditLog": { + "SiteWriter": { + "DatabasePath": "/app/data/auditlog.db" + } } } diff --git a/docker/site-c-node-a/appsettings.Site.json b/docker/site-c-node-a/appsettings.Site.json index d58e13b5..02d482ee 100644 --- a/docker/site-c-node-a/appsettings.Site.json +++ b/docker/site-c-node-a/appsettings.Site.json @@ -86,5 +86,16 @@ // Replication is opt-in and configured separately; absent = local-only. "LocalDb": { "Path": "/app/data/site-localdb.db" + }, + // arch-review remediation WP1.2: the site hot-path audit writer's SQLite file has no + // default path (mirrors LocalDb:Path above) and StartupValidator now requires it for + // Site nodes — an unset value used to fall back to a bare "auditlog.db" resolved + // relative to CWD, i.e. the container's ephemeral overlayfs, so the pending audit + // backlog was silently discarded on every recreate. On the mounted /app/data volume, + // same as LocalDb:Path, so it survives container recreate. + "AuditLog": { + "SiteWriter": { + "DatabasePath": "/app/data/auditlog.db" + } } } diff --git a/docker/site-c-node-b/appsettings.Site.json b/docker/site-c-node-b/appsettings.Site.json index cc155b04..78e987cb 100644 --- a/docker/site-c-node-b/appsettings.Site.json +++ b/docker/site-c-node-b/appsettings.Site.json @@ -86,5 +86,16 @@ // Replication is opt-in and configured separately; absent = local-only. "LocalDb": { "Path": "/app/data/site-localdb.db" + }, + // arch-review remediation WP1.2: the site hot-path audit writer's SQLite file has no + // default path (mirrors LocalDb:Path above) and StartupValidator now requires it for + // Site nodes — an unset value used to fall back to a bare "auditlog.db" resolved + // relative to CWD, i.e. the container's ephemeral overlayfs, so the pending audit + // backlog was silently discarded on every recreate. On the mounted /app/data volume, + // same as LocalDb:Path, so it survives container recreate. + "AuditLog": { + "SiteWriter": { + "DatabasePath": "/app/data/auditlog.db" + } } } diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriter.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriter.cs index 62108a76..4247e592 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriter.cs @@ -174,6 +174,24 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable pragmaCmd.ExecuteNonQuery(); } + // synchronous=NORMAL (arch-review remediation WP1.2): SQLite's FULL default + // fsyncs on every transaction commit, which is the dominant cost of the + // per-event write path this same change coalesces via FlushIntervalMs — FULL + // would still fsync once per FLUSH even after that fix. NORMAL fsyncs at + // fewer, WAL-checkpoint-aligned points instead of every commit; the window + // this opens is a handful of the most recent commits lost on OS crash / power + // loss (WAL mode itself still guarantees no *corruption*, only a possible + // short rollback on unclean restart). That trade is acceptable here because + // audit writes are explicitly best-effort by design (CLAUDE.md: "Audit-write + // failure NEVER aborts the user-facing action — audit is best-effort, the + // action's own success/failure path is authoritative") — this is the same + // durability class as a dropped audit row, not a new risk class. + using (var pragmaCmd = _connection.CreateCommand()) + { + pragmaCmd.CommandText = "PRAGMA synchronous = NORMAL"; + pragmaCmd.ExecuteNonQuery(); + } + // Enable FK enforcement on the WRITE connection. PRAGMA foreign_keys is // a per-connection, per-session setting in SQLite — it is NOT persisted // in the database file, so every new connection that may INSERT into @@ -306,20 +324,67 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable { batch.Clear(); batch.Add(first); + var deadline = DateTime.UtcNow.AddMilliseconds(_options.FlushIntervalMs); // Pull additional ready events up to BatchSize. TryRead is non- // blocking and lets us amortise the transaction overhead across a - // burst of concurrent enqueues. - while (batch.Count < _options.BatchSize && - _writeQueue.Reader.TryRead(out var next)) + // burst of concurrent enqueues. Once the immediately-available + // burst is drained, keep the transaction open for up to + // FlushIntervalMs waiting for MORE events to trickle in — this is + // the soft-flush coalescing arch-review remediation WP1.2 adds: + // without it, a low-rate trickle of writes (one script call every + // few ms, well under BatchSize) fsyncs once PER EVENT, because the + // writer loop reaches an empty channel and flushes immediately. + // FlushIntervalMs bounds the worst-case added latency any single + // event can see from this coalescing. + while (batch.Count < _options.BatchSize) { - batch.Add(next); + if (_writeQueue.Reader.TryRead(out var next)) + { + batch.Add(next); + continue; + } + + var remaining = deadline - DateTime.UtcNow; + if (remaining <= TimeSpan.Zero) + { + break; + } + + var waitToRead = _writeQueue.Reader.WaitToReadAsync().AsTask(); + var completed = await Task.WhenAny( + waitToRead, Task.Delay(remaining)).ConfigureAwait(false); + + if (completed != waitToRead) + { + // Deadline elapsed with nothing new — flush what we have. + break; + } + + // WaitToReadAsync resolving false means the channel completed + // (Dispose) with nothing left to read; resolving true means at + // least one item is available, so the top of the loop's + // TryRead will pick it up. + if (!await waitToRead.ConfigureAwait(false)) + { + break; + } } FlushBatch(batch); } } + /// + /// Total number of transactions committed since + /// construction. Test-only observability seam (arch-review remediation + /// WP1.2) for asserting that FlushIntervalMs coalesces a trickle of + /// near-simultaneous writes into a single transaction rather than one + /// commit per event; production code never reads this. + /// + internal int FlushCountForTests => _flushCountForTests; + private int _flushCountForTests; + private void FlushBatch(IReadOnlyList batch) { lock (_writeLock) @@ -436,6 +501,7 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable } transaction.Commit(); + _flushCountForTests++; } catch (Exception ex) { diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptions.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptions.cs index 3f558a54..ab8c1f8f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptions.cs @@ -9,8 +9,23 @@ namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site; /// public sealed class SqliteAuditWriterOptions { - /// SQLite database path (or in-memory URI for tests). - public string DatabasePath { get; set; } = "auditlog.db"; + /// + /// SQLite database path (or in-memory URI for tests). No default — + /// mirrors LocalDb:Path (ZB.MOM.WW.LocalDb.LocalDbOptions.Path): a + /// bare filename here would resolve relative to the process CWD, which on the + /// docker rig is the container's ephemeral overlayfs, not the mounted + /// /app/data volume — the file (and its pending forward-state backlog) + /// would be silently discarded on every container recreate. Site nodes MUST set + /// this explicitly (e.g. /app/data/auditlog.db); StartupValidator + /// in ZB.MOM.WW.ScadaBridge.Host enforces the requirement pre-host for Site nodes only, + /// the same way it enforces Communication:GrpcPsk. Central composition roots also + /// bind this options type (AddAuditLog is shared between roles) but never resolve + /// , so the empty default is inert there and deliberately + /// left unvalidated by (which only checks + /// the role-agnostic channel/batch/flush knobs) — enforcing it there too would fail + /// central's boot via ValidateOnStart, since AddAuditLog runs on both roles. + /// + public string DatabasePath { get; set; } = ""; /// /// Capacity of the bounded write queue. Set high enough that ordinary diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs index 68c586ce..cc79e76b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs @@ -6,19 +6,25 @@ namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site; /// Validates at host startup /// (arch-review 08 round 2 NF4). The channel/batch/flush knobs feed the /// background writer task; a zero anywhere stalls it (nothing drains, nothing -/// flushes), and an empty DatabasePath leaves the SQLite writer with no -/// backing store. +/// flushes). /// is intentionally NOT validated — a non-positive value has a documented /// fall-back-to-30s contract in SiteAuditBacklogReporter. /// +/// +/// is deliberately NOT +/// validated here (arch-review remediation WP1.2). AddAuditLog runs on +/// BOTH Central and Site composition roots and binds this options type with +/// ValidateOnStart either way, but only Site nodes ever resolve +/// — requiring a non-empty path in this +/// role-agnostic validator would fail Central's boot too. The requirement is +/// instead enforced pre-host, Site-role-only, by StartupValidator in +/// ZB.MOM.WW.ScadaBridge.Host (mirrors how it gates Communication:GrpcPsk). +/// public sealed class SqliteAuditWriterOptionsValidator : OptionsValidatorBase { /// protected override void Validate(ValidationBuilder builder, SqliteAuditWriterOptions options) { - builder.RequireThat(!string.IsNullOrWhiteSpace(options.DatabasePath), - $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.DatabasePath)} must be a non-empty path."); - builder.RequireThat(options.ChannelCapacity > 0, $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.ChannelCapacity)} " + $"({options.ChannelCapacity}) must be > 0."); diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs index e5e5f1e7..1985ff29 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs @@ -159,6 +159,28 @@ public static class StartupValidator + "endpoint under ScadaBridge:Communication:CentralGrpcEndpoints " + "(e.g. http://scadabridge-central-a:8083). Central nodes leave it empty."); + // The site hot-path audit writer's SQLite file (arch-review remediation + // WP1.2). SqliteAuditWriterOptions.DatabasePath has no default (mirrors + // LocalDb:Path) — an unset value would previously fall back to a bare + // "auditlog.db" resolved relative to the process CWD, which on the docker + // rig is the container's ephemeral overlayfs, not the mounted /app/data + // volume: the file, and every pending (not-yet-forwarded) audit row in it, + // was silently discarded on every container recreate. AddAuditLog binds + // this options type on BOTH roles (SqliteAuditWriterOptionsValidator + // deliberately does not check DatabasePath there, so Central's boot is + // unaffected — see that validator's remarks), so the Site-only requirement + // lives here, the same way GrpcPsk is gated just above. NOTE: unlike every + // other key in this method, AuditLog:SiteWriter is a TOP-LEVEL config + // section (AddAuditLog binds "AuditLog:SiteWriter" off the configuration + // root, not "ScadaBridge:AuditLog:SiteWriter") — no ScadaBridge: prefix. + p.Require("AuditLog:SiteWriter:DatabasePath", + value => !string.IsNullOrWhiteSpace(value), + "is required for Site nodes: the SQLite hot-path audit writer has no " + + "default path (mirrors LocalDb:Path) — an unset value would silently " + + "resolve to a CWD-relative file on the container's ephemeral overlayfs " + + "and lose the pending audit backlog on every redeploy. Point it at the " + + "mounted data volume, e.g. /app/data/auditlog.db."); + // ScadaBridge:Database:SiteDbPath was required here until LocalDb // Phase 2. The site's tables now live in the consolidated LocalDb // database (LocalDb:Path, which SiteServiceRegistration requires), diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Site.json b/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Site.json index 397041c2..8e065162 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Site.json +++ b/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Site.json @@ -80,5 +80,14 @@ // fails to boot, so every site config must set it. "LocalDb": { "Path": "./data/site-localdb.db" + }, + // arch-review remediation WP1.2: the site hot-path audit writer's SQLite file has no + // default path (mirrors LocalDb:Path above) - AuditLog:SiteWriter:DatabasePath is + // REQUIRED and validated pre-host on Site nodes (StartupValidator), the same way + // LocalDb:Path is, so every site config must set it. + "AuditLog": { + "SiteWriter": { + "DatabasePath": "./data/auditlog.db" + } } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs index 05ba0570..ca0345ef 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs @@ -5,30 +5,30 @@ namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site; /// /// Eager startup validation for /// (arch-review 08 round 2 NF4). The site hot-path writer's channel/batch/flush -/// knobs must be positive or the background writer task cannot make progress; -/// an empty DatabasePath would leave the SQLite writer with nowhere to go. +/// knobs must be positive or the background writer task cannot make progress. /// BacklogPollIntervalSeconds is deliberately NOT validated — a /// non-positive value has a documented fall-back-to-30s contract. /// +/// +/// DatabasePath is intentionally NOT covered here (arch-review remediation +/// WP1.2): it defaults to "" (no CWD-relative default — mirrors +/// LocalDb:Path), but this validator runs on both Central and Site +/// composition roots via ValidateOnStart, and only Site nodes ever resolve +/// the writer. The Site-only requirement is enforced pre-host by +/// StartupValidator in ZB.MOM.WW.ScadaBridge.Host — see +/// StartupValidatorTests.SiteWithoutAuditLogDatabasePath_FailsValidation. +/// public class SqliteAuditWriterOptionsValidatorTests { [Fact] public void DefaultOptions_AreValid() { + // Includes the empty DatabasePath default — this validator does not check + // it (see class remarks); only StartupValidator does, and only for Site nodes. var validator = new SqliteAuditWriterOptionsValidator(); Assert.True(validator.Validate(null, new SqliteAuditWriterOptions()).Succeeded); } - [Fact] - public void EmptyDatabasePath_Fails() - { - var validator = new SqliteAuditWriterOptionsValidator(); - var result = validator.Validate(null, new SqliteAuditWriterOptions { DatabasePath = "" }); - Assert.False(result.Succeeded); - Assert.Contains(result.Failures!, - f => f.Contains(nameof(SqliteAuditWriterOptions.DatabasePath), StringComparison.Ordinal)); - } - [Fact] public void ZeroChannelCapacity_Fails() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterWriteTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterWriteTests.cs index 9ee16e6c..db18c2a6 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterWriteTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterWriteTests.cs @@ -24,7 +24,8 @@ public class SqliteAuditWriterWriteTests private static (SqliteAuditWriter writer, string dataSource) CreateWriter( string testName, int? channelCapacity = null, - INodeIdentityProvider? nodeIdentity = null) + INodeIdentityProvider? nodeIdentity = null, + int? flushIntervalMs = null) { var dataSource = $"file:{testName}-{Guid.NewGuid():N}?mode=memory&cache=shared"; var opts = new SqliteAuditWriterOptions { DatabasePath = dataSource }; @@ -32,6 +33,10 @@ public class SqliteAuditWriterWriteTests { opts.ChannelCapacity = cap; } + if (flushIntervalMs is int flush) + { + opts.FlushIntervalMs = flush; + } // Default identity provider returns null — existing tests pre-date // SourceNode stamping and have no expectation about it. New stamping @@ -181,6 +186,69 @@ public class SqliteAuditWriterWriteTests Assert.Equal(1000, Convert.ToInt64(sidecarCmd.ExecuteScalar())); } + // ----- FlushIntervalMs soft-flush coalescing (arch-review remediation WP1.2) ----- // + + /// + /// A trickle of events — well under , + /// spaced closer together than — + /// must coalesce into ONE transaction rather than fsyncing once per event. This is the + /// bug WP1.2 fixes: FlushIntervalMs was validated but never read by the writer + /// loop, so every trickle-rate write incurred its own commit (and, under SQLite's + /// default synchronous=FULL, its own fsync). + /// + [Fact] + public async Task WriteAsync_TrickleLoad_CoalescesIntoOneTransaction() + { + var (writer, dataSource) = CreateWriter( + nameof(WriteAsync_TrickleLoad_CoalescesIntoOneTransaction), + flushIntervalMs: 500); + await using var _ = writer; + + const int count = 5; + var writeTasks = new List(count); + for (int i = 0; i < count; i++) + { + writeTasks.Add(writer.WriteAsync(NewEvent())); + await Task.Delay(15); // spaced well under the 500ms flush window + } + await Task.WhenAll(writeTasks); + + using var connection = OpenVerifierConnection(dataSource); + using var cmd = connection.CreateCommand(); + cmd.CommandText = "SELECT COUNT(*) FROM audit_event;"; + Assert.Equal(count, Convert.ToInt64(cmd.ExecuteScalar())); + + // The defining assertion: all 5 trickled-in writes landed in a single + // FlushBatch transaction, not up to 5 separate commits. + Assert.Equal(1, writer.FlushCountForTests); + } + + /// + /// The flip side: two events spaced FURTHER apart than + /// must NOT be held open + /// waiting for a partner — the deadline bounds worst-case added latency, it does + /// not turn into an unbounded debounce. + /// + [Fact] + public async Task WriteAsync_EventsSpacedBeyondFlushInterval_FlushSeparately() + { + var (writer, dataSource) = CreateWriter( + nameof(WriteAsync_EventsSpacedBeyondFlushInterval_FlushSeparately), + flushIntervalMs: 50); + await using var _ = writer; + + await writer.WriteAsync(NewEvent()); + await Task.Delay(300); // well beyond the 50ms flush window + await writer.WriteAsync(NewEvent()); + + using var connection = OpenVerifierConnection(dataSource); + using var cmd = connection.CreateCommand(); + cmd.CommandText = "SELECT COUNT(*) FROM audit_event;"; + Assert.Equal(2, Convert.ToInt64(cmd.ExecuteScalar())); + + Assert.Equal(2, writer.FlushCountForTests); + } + [Fact] public async Task WriteAsync_DuplicateEventId_FirstWriteWins_NoException() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteHealthEndpointTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteHealthEndpointTests.cs index 5be47b9e..6f0718d7 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteHealthEndpointTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteHealthEndpointTests.cs @@ -27,10 +27,12 @@ public class SiteHealthEndpointTests : IDisposable private readonly List _disposables = new(); private readonly Dictionary _previousEnv = new(StringComparer.Ordinal); private readonly string _tempDbPath; + private readonly string _tempAuditDbPath; public SiteHealthEndpointTests() { _tempDbPath = Path.Combine(Path.GetTempPath(), $"scadabridge_health_ep_{Guid.NewGuid()}.db"); + _tempAuditDbPath = Path.Combine(Path.GetTempPath(), $"scadabridge_health_ep_audit_{Guid.NewGuid()}.db"); // Whole-key env overrides, the sanctioned path: supplying GrpcPsk concretely makes the // pre-host secrets expander skip appsettings.Site.json's ${secret:SB-GRPC-PSK-site-1} @@ -50,6 +52,11 @@ public class SiteHealthEndpointTests : IDisposable SetEnv("ScadaBridge__Cluster__SeedNodes__1", "akka.tcp://scadabridge@localhost:18085"); SetEnv("ScadaBridge__Communication__GrpcPsk", "test-psk-0123456789"); SetEnv("LocalDb__Path", _tempDbPath); + // arch-review remediation WP1.2: SqliteAuditWriterOptions.DatabasePath has no default + // and StartupValidator now requires it for Site nodes — same reason LocalDb__Path is + // overridden above (appsettings.Site.json's own default is CWD-relative and would + // otherwise litter the test working directory when SiteAuditBacklogReporter probes it). + SetEnv("AuditLog__SiteWriter__DatabasePath", _tempAuditDbPath); } private void SetEnv(string key, string? value) @@ -71,6 +78,7 @@ public class SiteHealthEndpointTests : IDisposable } try { File.Delete(_tempDbPath); } catch { /* best effort */ } + try { File.Delete(_tempAuditDbPath); } catch { /* best effort */ } GC.SuppressFinalize(this); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs index f00544b0..c489e9e9 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs @@ -44,6 +44,10 @@ public class StartupValidatorTests // Phase 4: gRPC (CentralControlService) is the only site→central transport, so a Site // node must list at least one central gRPC endpoint to dial (no Akka fallback remains). ["ScadaBridge:Communication:CentralGrpcEndpoints:0"] = "http://central-a:8083", + // arch-review remediation WP1.2: the site hot-path audit writer's SQLite file has no + // default path (mirrors LocalDb:Path) — required so it lands on the mounted data volume, + // not the container's ephemeral overlayfs. + ["AuditLog:SiteWriter:DatabasePath"] = "/app/data/auditlog.db", }; [Fact] @@ -124,6 +128,43 @@ public class StartupValidatorTests Assert.Null(Record.Exception(() => StartupValidator.Validate(config))); } + [Fact] + public void SiteWithoutAuditLogDatabasePath_FailsValidation() + { + // arch-review remediation WP1.2: SqliteAuditWriterOptions.DatabasePath has no default + // (mirrors LocalDb:Path). An unset value used to fall back to a bare "auditlog.db" + // resolved relative to CWD — on the docker rig, the container's ephemeral overlayfs — + // so the node booted, looked healthy, and lost its pending audit backlog on every + // recreate. Fail fast at boot instead. + var values = ValidSiteConfig(); + values.Remove("AuditLog:SiteWriter:DatabasePath"); + var config = BuildConfig(values); + + var ex = Assert.Throws(() => StartupValidator.Validate(config)); + Assert.Contains("DatabasePath", ex.Message); + } + + [Fact] + public void SiteWithBlankAuditLogDatabasePath_FailsValidation() + { + var values = ValidSiteConfig(); + values["AuditLog:SiteWriter:DatabasePath"] = " "; + var config = BuildConfig(values); + + var ex = Assert.Throws(() => StartupValidator.Validate(config)); + Assert.Contains("DatabasePath", ex.Message); + } + + [Fact] + public void CentralWithoutAuditLogDatabasePath_PassesValidation() + { + // AddAuditLog binds SqliteAuditWriterOptions on both roles, but only Site nodes ever + // resolve the writer — the requirement must not fire for Central. + var config = BuildConfig(ValidCentralConfig()); + + Assert.Null(Record.Exception(() => StartupValidator.Validate(config))); + } + [Fact] public void MissingRole_FailsValidation() {