fix(auditlog): page the site audit purge, bound incremental_vacuum per page, fail fast instead of parking writers on a full queue (review 2026-10-21 06/MEM-1)

2026-10-22 06:23:35 -04:00
parent 268b55e350
commit 1da5d0bac4
6 changed files with 261 additions and 56 deletions
@@ -134,10 +134,11 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
        _writeQueue = Channel.CreateBounded<PendingAuditEvent>(
            new BoundedChannelOptions(_options.ChannelCapacity)
            {
-                // The hot-path enqueue must back-pressure if the background
-                // writer falls behind; a higher-level fallback
-                // handles truly catastrophic primary failure with a drop-oldest
-                // ring buffer.
+                // Wait mode is only what the (never-awaited) async write path would
+                // do; WriteAsync uses TryWrite exclusively and fails fast with
+                // SqliteAuditWriterSaturatedException when the queue is full, so no
+                // caller ever parks on room (review 2026-10-21 06/MEM-1). The
+                // FallbackAuditWriter's drop-oldest ring absorbs the overflow.
                FullMode = BoundedChannelFullMode.Wait,
                SingleReader = true,
                SingleWriter = false,
@@ -286,7 +287,8 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable

    /// <summary>
    /// Enqueues an audit event for asynchronous batched persistence to SQLite.
-    /// Back-pressure is applied when the write channel is full.
+    /// Never parks the caller on queue room: a full queue faults the returned task with
+    /// <see cref="SqliteAuditWriterSaturatedException"/> immediately.
    /// </summary>
    /// <param name="evt">The audit event to persist.</param>
    /// <param name="ct">Cancellation token.</param>
@@ -300,35 +302,40 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
        // alongside the canonical row in the same transaction.
        var pending = new PendingAuditEvent(evt, AuditForwardState.Pending);

-        // CreateBounded(FullMode=Wait) means WriteAsync will await room rather
-        // than throw when full — exactly the hot-path back-pressure semantics
-        // we want.
-        if (!_writeQueue.Writer.TryWrite(pending))
+        if (_writeQueue.Writer.TryWrite(pending))
        {
-            // The writer is either completed (logger disposed) or the channel
-            // is at capacity. Fall back to the async path which honours the
-            // FullMode=Wait policy.
-            return WriteSlowPathAsync(pending, ct);
+            return pending.Completion.Task;
        }

-        return pending.Completion.Task;
+        // TryWrite fails for exactly two reasons: the writer is completed (disposed)
+        // or the queue is at capacity. The second used to fall into an awaited
+        // WriteAsync on the channel — one parked waiter, holding its AuditEvent, per
+        // fire-and-continue script-thread caller for as long as the background writer
+        // stayed stuck (e.g. behind a multi-minute retention purge). That count was
+        // unbounded and invisible. Fail fast instead: FallbackAuditWriter routes the
+        // event to its bounded drop-oldest ring and bumps SiteAuditWriteFailures, which
+        // is the documented best-effort degradation (review 2026-10-21 06/MEM-1).
+        if (Volatile.Read(ref _writerCompleted))
+        {
+            return Task.FromException(new ObjectDisposedException(nameof(SqliteAuditWriter),
+                "Event could not be recorded: the audit writer has been disposed."));
+        }
+
+        Interlocked.Increment(ref _saturatedRejections);
+        return Task.FromException(new SqliteAuditWriterSaturatedException(_options.ChannelCapacity));
    }

-    private async Task WriteSlowPathAsync(PendingAuditEvent pending, CancellationToken ct)
-    {
-        try
-        {
-            await _writeQueue.Writer.WriteAsync(pending, ct).ConfigureAwait(false);
-        }
-        catch (ChannelClosedException)
-        {
-            pending.Completion.TrySetException(
-                new ObjectDisposedException(nameof(SqliteAuditWriter),
-                    "Event could not be recorded: the audit writer has been disposed."));
-        }
+    /// <summary>
+    /// Number of <see cref="WriteAsync"/> calls refused because the write queue was full
+    /// since construction. Observability seam; the health-report count is the
+    /// <c>SiteAuditWriteFailures</c> bump <see cref="FallbackAuditWriter"/> makes per refusal.
+    /// </summary>
+    public long SaturatedRejectionCount => Interlocked.Read(ref _saturatedRejections);
+    private long _saturatedRejections;

-        await pending.Completion.Task.ConfigureAwait(false);
-    }
+    // Set (before the channel is completed) once DisposeAsync starts, so a refused
+    // TryWrite can tell "disposed" from "full" without racing the channel's state.
+    private bool _writerCompleted;

    private async Task ProcessWriteQueueAsync()
    {
@@ -397,6 +404,12 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
    /// commit per event; production code never reads this.
    /// </summary>
    internal int FlushCountForTests => _flushCountForTests;
+
+    /// <summary>
+    /// Test-only: the write lock FlushBatch / the flips / the purge serialise on, so a test can
+    /// park the background writer deterministically (e.g. to fill the write queue).
+    /// </summary>
+    internal object WriteLockForTests => _writeLock;
    private int _flushCountForTests;

    private void FlushBatch(IReadOnlyList<PendingAuditEvent> batch)
@@ -1181,7 +1194,52 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
    }

    /// <inheritdoc />
-    public Task<int> PurgeExpiredAsync(DateTime olderThanUtc, CancellationToken ct = default)
+    /// <remarks>
+    /// <b>Paged (review 2026-10-21 06/MEM-1).</b> The purge deletes at most
+    /// <see cref="SqliteAuditWriterOptions.PurgeBatchSize"/> rows per transaction and runs a
+    /// bounded <c>PRAGMA incremental_vacuum(N)</c> after each page, taking <c>_writeLock</c>
+    /// per page and releasing it in between, so the batched hot-path writer interleaves with
+    /// a multi-million-row purge rather than stalling behind one giant DELETE + an unbounded
+    /// vacuum. Every eligible row is still removed by one call; only the lock grain changed.
+    /// </remarks>
+    public async Task<int> PurgeExpiredAsync(DateTime olderThanUtc, CancellationToken ct = default)
+    {
+        // Cutoff uses the same round-trip "o" encoding the writer stores
+        // OccurredAtUtc in, so string comparison stays monotonic (see FlushBatch).
+        var cutoff = EnsureUtc(olderThanUtc).ToString(
+            "o", System.Globalization.CultureInfo.InvariantCulture);
+        var pageSize = Math.Max(1, _options.PurgeBatchSize);
+
+        var total = 0;
+        while (true)
+        {
+            ct.ThrowIfCancellationRequested();
+            var purged = PurgePage(cutoff, pageSize);
+            total += purged;
+            if (purged < pageSize)
+            {
+                return total;
+            }
+
+            // Lock already released by PurgePage; yield so the background writer (and any
+            // flip/read waiting on _writeLock) gets the lock before the next page.
+            await Task.Yield();
+        }
+    }
+
+    /// <summary>
+    /// Number of purge-page transactions committed since construction. Test-only
+    /// observability seam for asserting the purge is paged; production code never reads it.
+    /// </summary>
+    internal int PurgePageCountForTests => Volatile.Read(ref _purgePageCountForTests);
+    private int _purgePageCountForTests;
+
+    /// <summary>
+    /// Deletes ONE page (at most <paramref name="pageSize"/> rows) of purge-eligible rows in a
+    /// single transaction under <c>_writeLock</c>, then reclaims a bounded number of freed
+    /// pages. Returns the number of canonical rows deleted.
+    /// </summary>
+    private int PurgePage(string cutoff, int pageSize)
    {
        // Runs on the WRITE connection under _writeLock (a DELETE mutates the file),
        // mirroring MarkForwardedAsync / MarkReconciledAsync. Serialising with the
@@ -1190,24 +1248,18 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
        {
            ObjectDisposedException.ThrowIf(_disposed, this);

-            // Cutoff uses the same round-trip "o" encoding the writer stores
-            // OccurredAtUtc in, so string comparison stays monotonic (see FlushBatch).
-            var cutoff = EnsureUtc(olderThanUtc).ToString(
-                "o", System.Globalization.CultureInfo.InvariantCulture);
-
            int purged;
            using (var transaction = _connection.BeginTransaction())
            {
                try
                {
-                    // Capture the eligible EventIds from the sidecar FIRST: the
-                    // Forwarded/Reconciled + age criteria live on
-                    // audit_forward_state, and it is the FK child of audit_event, so
-                    // it must be deleted before the parent. A temp table (rather than
-                    // an IN (...) list) keeps an arbitrarily large purge set off the
-                    // SQLite 999-parameter limit. The hard invariant is encoded here:
-                    // only Forwarded/Reconciled rows qualify — Pending is never purged
-                    // on age alone.
+                    // Capture this page's eligible EventIds from the sidecar FIRST: the
+                    // Forwarded/Reconciled + age criteria live on audit_forward_state, and
+                    // it is the FK child of audit_event, so it must be deleted before the
+                    // parent. A temp table (rather than an IN (...) list) keeps the page off
+                    // the SQLite 999-parameter limit. The hard invariant is encoded here:
+                    // only Forwarded/Reconciled rows qualify — Pending (and the pull-owed
+                    // PushAbandoned/Rejected states) are never purged on age alone.
                    using (var createCmd = _connection.CreateCommand())
                    {
                        createCmd.Transaction = transaction;
@@ -1216,11 +1268,13 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
                            CREATE TEMP TABLE purge_ids AS
                                SELECT EventId FROM audit_forward_state
                                WHERE ForwardState IN ($forwarded, $reconciled)
-                                  AND OccurredAtUtc < $cutoff;
+                                  AND OccurredAtUtc < $cutoff
+                                LIMIT $limit;
                            """;
                        createCmd.Parameters.AddWithValue("$forwarded", AuditForwardState.Forwarded.ToString());
                        createCmd.Parameters.AddWithValue("$reconciled", AuditForwardState.Reconciled.ToString());
                        createCmd.Parameters.AddWithValue("$cutoff", cutoff);
+                        createCmd.Parameters.AddWithValue("$limit", pageSize);
                        createCmd.ExecuteNonQuery();
                    }

@@ -1276,6 +1330,7 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
                    }

                    transaction.Commit();
+                    Interlocked.Increment(ref _purgePageCountForTests);
                }
                catch
                {
@@ -1284,16 +1339,19 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
                }
            }

-            // Reclaim the freed pages back to the OS. auto_vacuum=INCREMENTAL (set in
-            // InitializeSchema) makes this a bounded incremental step rather than a
-            // full VACUUM rewrite. Runs outside the transaction; a vacuum failure must
-            // not fault the purge itself, so it is best-effort.
+            // Reclaim a BOUNDED number of freed pages back to the OS. A bare
+            // `PRAGMA incremental_vacuum;` empties the whole freelist in one step, which
+            // after a large purge held _writeLock for as long as the delete itself did.
+            // auto_vacuum=INCREMENTAL (set in InitializeSchema) makes this a cheap
+            // freelist trim. Runs outside the transaction; a vacuum failure must not fault
+            // the purge itself, so it is best-effort.
            if (purged > 0)
            {
                try
                {
                    using var vacuumCmd = _connection.CreateCommand();
-                    vacuumCmd.CommandText = "PRAGMA incremental_vacuum;";
+                    vacuumCmd.CommandText = string.Create(System.Globalization.CultureInfo.InvariantCulture,
+                        $"PRAGMA incremental_vacuum({Math.Max(1, _options.PurgeVacuumPagesPerBatch)});");
                    vacuumCmd.ExecuteNonQuery();
                }
                catch (Exception ex)
@@ -1303,7 +1361,7 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
                }
            }

-            return Task.FromResult(purged);
+            return purged;
        }
    }

@@ -1387,6 +1445,7 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
        lock (_writeLock)
        {
            if (_disposed) return;
+            Volatile.Write(ref _writerCompleted, true);
            // Stop accepting new events. Completing the channel writer is the
            // shutdown signal: WriteAsync calls observe the completion and
            // fault, and the writer loop drains any already-buffered items