fix(runtime): review findings — recursion-safe run cap, atomic detach counter, summary edge cases, per-row event-log fallback

2026-08-14 23:42:29 -04:00
parent b1de9dfdd4
commit 950c54c5fc
10 changed files with 626 additions and 42 deletions
@@ -260,14 +260,24 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable
    }

    /// <summary>
-    /// Commits a drained batch of pending events in one transaction. All-or-nothing per
-    /// batch: <c>site_events</c> ids are freshly minted GUIDs per <see cref="LogEventAsync"/>
-    /// call (no cross-call replay/de-dup concern the way <c>SqliteAuditWriter</c> has), so
-    /// unlike that reference there is no per-row duplicate-key swallow here — any failure
-    /// rolls back the whole batch and faults every event in it. <c>site_events</c> is a
-    /// CDC-replicated table (LocalDb Phase 1); its capture triggers fire per row on INSERT
-    /// same as before, so batching the commit changes only how many rows land per
-    /// transaction, not per-row trigger/replication behavior.
+    /// Commits a drained batch of pending events in one transaction, falling back to
+    /// row-by-row inserts if that transaction fails.
+    ///
+    /// <para>The batch is the fast path: one transaction amortises SQLite's fsync/journal cost
+    /// across the whole drain. But a batch is all-or-nothing, so before the fallback existed a
+    /// SINGLE unwritable row (an oversized <c>details</c> payload, a constraint/trigger reject)
+    /// faulted all ~<see cref="WriteBatchSize"/> of its innocent neighbours and inflated
+    /// <see cref="FailedWriteCount"/> — the health metric — by the same factor. The rollback
+    /// is therefore followed by a per-row retry: one poison row costs exactly one row, the
+    /// rest still land, and the failure count stays truthful. Row-by-row is the degraded
+    /// path only — it is entered solely after a batch has already failed.</para>
+    ///
+    /// <para><c>site_events</c> ids are freshly minted GUIDs per <see cref="LogEventAsync"/>
+    /// call (no cross-call replay/de-dup concern the way <c>SqliteAuditWriter</c> has), so a
+    /// re-insert during the fallback cannot duplicate a row the rolled-back batch had already
+    /// written. <c>site_events</c> is a CDC-replicated table (LocalDb Phase 1); its capture
+    /// triggers fire per row on INSERT either way, so neither shape changes
+    /// trigger/replication behavior.</para>
    /// </summary>
    private void FlushBatch(IReadOnlyList<PendingEvent> batch)
    {
@@ -283,10 +293,7 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable
                {
                    using var cmd = connection.CreateCommand();
                    cmd.Transaction = transaction;
-                    cmd.CommandText = """
-                        INSERT INTO site_events (id, timestamp, event_type, severity, instance_id, source, message, details)
-                        VALUES ($id, $timestamp, $event_type, $severity, $instance_id, $source, $message, $details)
-                        """;
+                    cmd.CommandText = InsertEventSql;
                    var pId = cmd.Parameters.Add("$id", SqliteType.Text);
                    var pTimestamp = cmd.Parameters.Add("$timestamp", SqliteType.Text);
                    var pEventType = cmd.Parameters.Add("$event_type", SqliteType.Text);
@@ -327,16 +334,21 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable

        if (failure is not null)
        {
-            // A write failure must be observable. Count every event in the failed
-            // batch (Health Monitoring reads FailedWriteCount) and fault each
-            // caller's Task instead of silently discarding the exception.
-            Interlocked.Add(ref _failedWriteCount, batch.Count);
-            _logger.LogError(failure, "Failed to record {Count} event(s) in batch (sqlite {SqliteError})",
-                batch.Count, DescribeSqliteError(failure));
-            foreach (var pending in batch)
+            if (batch.Count == 1)
            {
-                pending.Completion.TrySetException(failure);
+                // Nothing to isolate — retrying the single row would fail identically.
+                Interlocked.Increment(ref _failedWriteCount);
+                _logger.LogError(failure, "Failed to record 1 event (sqlite {SqliteError})",
+                    DescribeSqliteError(failure));
+                batch[0].Completion.TrySetException(failure);
+                return;
            }
+
+            // Degraded path: isolate the failure instead of charging the whole batch for it.
+            _logger.LogWarning(failure,
+                "Batch insert of {Count} site event(s) failed (sqlite {SqliteError}); retrying row by row",
+                batch.Count, DescribeSqliteError(failure));
+            FlushRowByRow(batch);
            return;
        }

@@ -352,8 +364,7 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable
            // WithConnection returns false only when the logger has been
            // disposed mid-drain; none of the batch was persisted. Fault every
            // Task instead of reporting false success for a dropped event.
-            var disposedEx = new ObjectDisposedException(nameof(SiteEventLogger),
-                "Event could not be recorded: the event logger was disposed before the write completed.");
+            var disposedEx = DisposedMidWrite();
            foreach (var pending in batch)
            {
                pending.Completion.TrySetException(disposedEx);
@@ -361,16 +372,111 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable
        }
    }

+    /// <summary>
+    /// Re-inserts a failed batch one row at a time, each in its own implicit transaction, so a
+    /// single unwritable event costs exactly one event: its own Task faults and it alone is
+    /// counted in <see cref="FailedWriteCount"/>, while every other row in the batch still
+    /// lands and completes successfully. Only reached from <see cref="FlushBatch"/> after the
+    /// batched transaction has already rolled back.
+    /// </summary>
+    /// <param name="batch">The rolled-back batch to retry row by row.</param>
+    private void FlushRowByRow(IReadOnlyList<PendingEvent> batch)
+    {
+        var failed = 0;
+
+        foreach (var pending in batch)
+        {
+            Exception? rowFailure = null;
+            bool written;
+            try
+            {
+                written = WithConnection(connection =>
+                {
+                    using var cmd = connection.CreateCommand();
+                    cmd.CommandText = InsertEventSql;
+                    cmd.Parameters.AddWithValue("$id", pending.Id);
+                    cmd.Parameters.AddWithValue("$timestamp", pending.Timestamp);
+                    cmd.Parameters.AddWithValue("$event_type", pending.EventType);
+                    cmd.Parameters.AddWithValue("$severity", pending.Severity);
+                    cmd.Parameters.AddWithValue("$instance_id", (object?)pending.InstanceId ?? DBNull.Value);
+                    cmd.Parameters.AddWithValue("$source", pending.Source);
+                    cmd.Parameters.AddWithValue("$message", pending.Message);
+                    cmd.Parameters.AddWithValue("$details", (object?)pending.Details ?? DBNull.Value);
+                    cmd.ExecuteNonQuery();
+                });
+            }
+            catch (Exception ex)
+            {
+                rowFailure = ex;
+                written = false;
+            }
+
+            if (written)
+            {
+                pending.Completion.TrySetResult();
+                continue;
+            }
+
+            failed++;
+            if (rowFailure is null)
+            {
+                // Disposed mid-fallback; the remaining rows will take this path too.
+                pending.Completion.TrySetException(DisposedMidWrite());
+                continue;
+            }
+
+            _logger.LogError(rowFailure,
+                "Failed to record event from {Source} on row-by-row retry (sqlite {SqliteError})",
+                pending.Source, DescribeSqliteError(rowFailure));
+            pending.Completion.TrySetException(rowFailure);
+        }
+
+        if (failed > 0)
+        {
+            // Health Monitoring reads FailedWriteCount: only the rows that could not be
+            // written are counted, so the metric measures poison rows, not batch size.
+            Interlocked.Add(ref _failedWriteCount, failed);
+        }
+    }
+
+    /// <summary>
+    /// The exception a pending event's Task is faulted with when the logger was disposed
+    /// before its write could complete. A fresh instance per call so each faulted Task carries
+    /// its own stack.
+    /// </summary>
+    /// <returns>The exception to fault the pending event with.</returns>
+    private static ObjectDisposedException DisposedMidWrite() =>
+        new(nameof(SiteEventLogger),
+            "Event could not be recorded: the event logger was disposed before the write completed.");
+
+    /// <summary>
+    /// The single INSERT shared by the batched fast path and the row-by-row fallback, so the
+    /// two shapes can never drift in column list or ordering.
+    /// </summary>
+    private const string InsertEventSql = """
+        INSERT INTO site_events (id, timestamp, event_type, severity, instance_id, source, message, details)
+        VALUES ($id, $timestamp, $event_type, $severity, $instance_id, $source, $message, $details)
+        """;
+
    /// <summary>
    /// Stops accepting new events, drains the write queue, and disposes the SQLite connection.
    /// </summary>
+    /// <remarks>
+    /// <c>_disposed</c> is deliberately NOT set in the first lock block. Completing
+    /// the channel writer is the whole shutdown signal: <see cref="LogEventAsync"/> observes it
+    /// (<c>TryWrite</c> returns false) and the writer loop drains what is already buffered
+    /// before exiting. Flipping <c>_disposed</c> up front would make <see cref="WithConnection"/>
+    /// return false for every remaining <see cref="FlushBatch"/>, so a graceful shutdown would
+    /// discard the entire queued backlog — the very events (shutdown diagnostics) most worth
+    /// keeping. It flips only after the loop has fully drained, where it guards the window
+    /// between drain and connection close. Mirrors <c>SqliteAuditWriter.DisposeAsync</c>.
+    /// </remarks>
    public void Dispose()
    {
-        Task? writerLoop = null;
+        Task? writerLoop;
        lock (_writeLock)
        {
            if (_disposed) return;
-            _disposed = true;
            // Stop accepting new events and let the writer loop drain.
            _writeQueue.Writer.TryComplete();
            writerLoop = _writerLoop;
@@ -390,6 +496,8 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable

        lock (_writeLock)
        {
+            if (_disposed) return;
+            _disposed = true;
            _connection.Dispose();
        }
    }