Merge branch 'worktree-agent-af8a44154c4e2dbba' into arch-review-remediation

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);
         }
     }
 
+    /// <summary>
+    /// Total number of <see cref="FlushBatch"/> 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.
+    /// </summary>
+    internal int FlushCountForTests => _flushCountForTests;
+    private int _flushCountForTests;
+
     private void FlushBatch(IReadOnlyList<PendingAuditEvent> batch)
     {
         lock (_writeLock)
@@ -436,6 +501,7 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
                 }
 
                 transaction.Commit();
+                _flushCountForTests++;
             }
             catch (Exception ex)
             {

src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptions.cs

@@ -9,8 +9,23 @@ namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site;
 /// </summary>
 public sealed class SqliteAuditWriterOptions
 {
-    /// <summary>SQLite database path (or in-memory URI for tests).</summary>
-    public string DatabasePath { get; set; } = "auditlog.db";
+    /// <summary>
+    /// SQLite database path (or in-memory URI for tests). No default —
+    /// mirrors <c>LocalDb:Path</c> (<c>ZB.MOM.WW.LocalDb.LocalDbOptions.Path</c>): 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
+    /// <c>/app/data</c> 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. <c>/app/data/auditlog.db</c>); <c>StartupValidator</c>
+    /// in ZB.MOM.WW.ScadaBridge.Host enforces the requirement pre-host for Site nodes only,
+    /// the same way it enforces <c>Communication:GrpcPsk</c>. Central composition roots also
+    /// bind this options type (<c>AddAuditLog</c> is shared between roles) but never resolve
+    /// <see cref="SqliteAuditWriter"/>, so the empty default is inert there and deliberately
+    /// left unvalidated by <see cref="SqliteAuditWriterOptionsValidator"/> (which only checks
+    /// the role-agnostic channel/batch/flush knobs) — enforcing it there too would fail
+    /// central's boot via <c>ValidateOnStart</c>, since <c>AddAuditLog</c> runs on both roles.
+    /// </summary>
+    public string DatabasePath { get; set; } = "";
 
     /// <summary>
     /// Capacity of the bounded write queue. Set high enough that ordinary

src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs

@@ -6,19 +6,25 @@ namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site;
 /// Validates <see cref="SqliteAuditWriterOptions"/> 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 <c>DatabasePath</c> leaves the SQLite writer with no
-/// backing store. <see cref="SqliteAuditWriterOptions.BacklogPollIntervalSeconds"/>
+/// flushes). <see cref="SqliteAuditWriterOptions.BacklogPollIntervalSeconds"/>
 /// is intentionally NOT validated — a non-positive value has a documented
 /// fall-back-to-30s contract in <c>SiteAuditBacklogReporter</c>.
 /// </summary>
+/// <remarks>
+/// <see cref="SqliteAuditWriterOptions.DatabasePath"/> is deliberately NOT
+/// validated here (arch-review remediation WP1.2). <c>AddAuditLog</c> runs on
+/// BOTH Central and Site composition roots and binds this options type with
+/// <c>ValidateOnStart</c> either way, but only Site nodes ever resolve
+/// <see cref="SqliteAuditWriter"/> — 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 <c>StartupValidator</c> in
+/// ZB.MOM.WW.ScadaBridge.Host (mirrors how it gates <c>Communication:GrpcPsk</c>).
+/// </remarks>
 public sealed class SqliteAuditWriterOptionsValidator : OptionsValidatorBase<SqliteAuditWriterOptions>
 {
     /// <inheritdoc />
     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.");

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),

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"
+    }
   }
 }