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() {