perf(notification-outbox): bounded parallel delivery within a dispatch sweep (default 4)

2026-07-09 09:41:48 -04:00
parent acd8a6b8b5
commit 6567216e14
4 changed files with 216 additions and 26 deletions
@@ -20,7 +20,7 @@ Central cluster. The `NotificationOutboxActor` is a **singleton on the active ce
 - Compute delivery KPIs from the `Notifications` table for the Health Monitoring dashboard and the Central UI.
 - Purge terminal rows daily after a configurable retention window.

-SMTP and HTTP delivery is blocking I/O. Delivery work runs on a **dedicated blocking-I/O dispatcher**, the same pattern used by Script Execution Actors, so delivery never blocks the actor's dispatcher loop.
+SMTP and HTTP delivery is blocking I/O. A dispatch sweep runs **off the actor thread** on the thread pool (the sweep task pipes its completion back to the actor), so delivery never blocks the actor's message loop. Within a sweep, deliveries run with **bounded per-sweep parallelism** — a `SemaphoreSlim` caps the number of concurrent deliveries at `MaxParallelDeliveries` (default 4; set to 1 for strictly sequential delivery). Each concurrent delivery uses its own DI scope/repository, so there is no shared-`DbContext` contention.

 ## End-to-End Flow

@@ -117,7 +117,7 @@ The dispatcher loop runs on a fixed interval. On each tick the `NotificationOutb

 1. Polls the `Notifications` table for **due rows** — `Pending` rows, and `Retrying` rows whose `NextAttemptAt` has passed.
 2. Resolves the target notification list to its recipients/targets at central, at delivery time.
-3. Hands the notification to the delivery adapter registered for its `Type`, running on the dedicated blocking-I/O dispatcher.
+3. Hands the notification to the delivery adapter registered for its `Type`. Deliveries within the sweep run with bounded parallelism (`MaxParallelDeliveries`, default 4), each on its own DI scope/repository, off the actor thread.
 4. Applies the result:
   - **success** → `Delivered`, set `DeliveredAt`, snapshot `ResolvedTargets`.
   - **transient failure** → `Retrying`, increment `RetryCount`, set `NextAttemptAt`, record `LastError`; once retries are exhausted → `Parked`.
@@ -318,25 +318,83 @@ public class NotificationOutboxActor : ReceiveActor, IWithTimers

            var (maxRetries, retryDelay) = await ResolveRetryPolicyAsync(notificationRepository);

+            // Deliver the claimed batch with bounded parallelism. Delivery is I/O-bound
+            // (multi-second SMTP/Twilio round trips), so a strictly sequential sweep caps
+            // throughput at ~1/attempt-latency — an alarm-storm backlog behind one slow
+            // host would drain far too slowly (arch-review 04). A SemaphoreSlim caps the
+            // number of in-flight deliveries; each delivery runs on its OWN DI scope/
+            // repository (the EF DbContext is not thread-safe and rows are distinct per
+            // notification, so concurrent deliveries never contend). The cached adapters
+            // and the lifetime-scoped audit writer are already safe for concurrent use.
+            // MaxParallelDeliveries = 1 preserves the strictly sequential behaviour.
+            using var deliveryGate = new SemaphoreSlim(
+                _options.ResolvedMaxParallelDeliveries, _options.ResolvedMaxParallelDeliveries);
+            var deliveries = new List<Task>(due.Count);
            foreach (var notification in due)
            {
-                // Between deliveries, observe shutdown so we don't kick off a fresh
-                // SMTP send when the actor is already tearing down.
-                if (cancellationToken.IsCancellationRequested)
+                deliveries.Add(DeliverGatedAsync(
+                    notification, now, maxRetries, retryDelay, adapters, deliveryGate, cancellationToken));
+            }
+
+            // Each DeliverGatedAsync isolates its own faults, so WhenAll never throws.
+            await Task.WhenAll(deliveries);
+        }
+        catch (Exception ex)
        {
+            // Scope/service resolution or retry-policy resolution faulted; swallow and log so
+            // the returned task completes normally and the in-flight guard is always cleared.
+            _logger.LogError(ex, "Dispatch sweep failed unexpectedly.");
+        }
+    }
+
+    /// <summary>
+    /// Delivers one notification under the sweep's concurrency gate. Acquires a permit,
+    /// runs <see cref="DeliverOneAsync"/> on a fresh per-notification DI scope (so parallel
+    /// deliveries never share an EF <c>DbContext</c>), and always releases the permit.
+    /// Faults are isolated here — shutdown cancellation is swallowed (the row stays in its
+    /// pre-attempt state for the next active sweep) and any other exception is logged — so
+    /// one bad delivery never aborts the rest of the batch and <c>Task.WhenAll</c> never faults.
+    /// </summary>
+    private async Task DeliverGatedAsync(
+        Notification notification,
+        DateTimeOffset now,
+        int maxRetries,
+        TimeSpan retryDelay,
+        IReadOnlyDictionary<NotificationType, INotificationDeliveryAdapter> adapters,
+        SemaphoreSlim gate,
+        CancellationToken cancellationToken)
+    {
+        try
+        {
+            await gate.WaitAsync(cancellationToken).ConfigureAwait(false);
+        }
+        catch (OperationCanceledException)
+        {
+            // Shutdown cancelled before this delivery acquired a permit; the row stays
+            // Pending and the next active node's sweep re-claims it. No permit to release.
            return;
        }

        try
        {
+            if (cancellationToken.IsCancellationRequested)
+            {
+                return;
+            }
+
+            // Each delivery owns its DI scope + repository so parallel UpdateAsync calls
+            // never share a DbContext. The adapters map + audit writer are already safe
+            // for concurrent use.
+            using var scope = _serviceProvider.CreateScope();
+            var outboxRepository = scope.ServiceProvider.GetRequiredService<INotificationOutboxRepository>();
            await DeliverOneAsync(
-                        notification, now, maxRetries, retryDelay, outboxRepository, adapters, cancellationToken);
+                    notification, now, maxRetries, retryDelay, outboxRepository, adapters, cancellationToken)
+                .ConfigureAwait(false);
        }
        catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
        {
-                    // In-flight delivery interrupted by shutdown. Row remains in its
-                    // pre-attempt state; next active sweep retries.
-                    return;
+            // In-flight delivery interrupted by shutdown. Row remains in its pre-attempt
+            // state; next active sweep retries.
        }
        catch (Exception ex)
        {
@@ -344,13 +402,9 @@ public class NotificationOutboxActor : ReceiveActor, IWithTimers
            _logger.LogError(
                ex, "Dispatch failed for notification {NotificationId}.", notification.NotificationId);
        }
-            }
-        }
-        catch (Exception ex)
+        finally
        {
-            // Scope/service resolution or retry-policy resolution faulted; swallow and log so
-            // the returned task completes normally and the in-flight guard is always cleared.
-            _logger.LogError(ex, "Dispatch sweep failed unexpectedly.");
+            gate.Release();
        }
    }

@@ -48,4 +48,23 @@ public class NotificationOutboxOptions
    /// Default: 1 minute.
    /// </summary>
    public TimeSpan DeliveredKpiWindow { get; set; } = TimeSpan.FromMinutes(1);
+
+    /// <summary>
+    /// Maximum number of notifications delivered concurrently within a single dispatch
+    /// sweep. Delivery is I/O-bound (SMTP / Twilio round trips of seconds each), so a
+    /// purely sequential sweep caps throughput at ~1/attempt-latency regardless of batch
+    /// size — an alarm storm queued behind one slow SMTP host would drain far too slowly
+    /// (arch-review 04). Each concurrent delivery runs on its own DI scope/repository, so
+    /// there is no shared-DbContext contention. Set to <c>1</c> to preserve the strictly
+    /// sequential behaviour. Clamped to a minimum of 1 via <see cref="ResolvedMaxParallelDeliveries"/>.
+    /// Default: 4.
+    /// </summary>
+    public int MaxParallelDeliveries { get; set; } = 4;
+
+    /// <summary>
+    /// Resolves the effective delivery parallelism, clamped to at least 1 so a
+    /// misconfigured <c>0</c>/negative value cannot stall the dispatcher (a zero-count
+    /// semaphore would deadlock the sweep).
+    /// </summary>
+    public int ResolvedMaxParallelDeliveries => MaxParallelDeliveries < 1 ? 1 : MaxParallelDeliveries;
 }
@@ -567,4 +567,121 @@ public class NotificationOutboxActorDispatchTests : TestKit
                Arg.Any<DateTimeOffset>(), Arg.Any<int>(), Arg.Any<CancellationToken>()),
            duration: TimeSpan.FromSeconds(2));
    }
+
+    // ── Task 25: bounded parallel delivery ───────────────────────────────────
+
+    /// <summary>
+    /// Adapter that records the concurrent-invocation high-water mark. Each
+    /// <see cref="DeliverAsync"/> increments a live counter, notes the peak, then blocks
+    /// on a shared gate until <paramref name="releaseWhen"/> deliveries are concurrently
+    /// in flight — proving real overlap. A safety timeout releases the gate so a
+    /// parallelism-1 run (which can never reach <c>releaseWhen</c> concurrently) still
+    /// drains instead of deadlocking.
+    /// </summary>
+    private sealed class ConcurrencyHighWaterAdapter : INotificationDeliveryAdapter
+    {
+        private readonly int _releaseWhen;
+        private readonly TaskCompletionSource _gate =
+            new(TaskCreationOptions.RunContinuationsAsynchronously);
+        private int _current;
+        private int _highWater;
+        private int _entered;
+
+        public ConcurrencyHighWaterAdapter(int releaseWhen) => _releaseWhen = releaseWhen;
+
+        public NotificationType Type => NotificationType.Email;
+
+        public int HighWater => Volatile.Read(ref _highWater);
+
+        public async Task<DeliveryOutcome> DeliverAsync(
+            Notification notification, CancellationToken cancellationToken = default)
+        {
+            var current = Interlocked.Increment(ref _current);
+            int hw;
+            while ((hw = Volatile.Read(ref _highWater)) < current
+                && Interlocked.CompareExchange(ref _highWater, current, hw) != hw)
+            {
+                // retry until our peak is recorded
+            }
+
+            if (Interlocked.Increment(ref _entered) >= _releaseWhen)
+            {
+                _gate.TrySetResult();
+            }
+
+            // Overlap window: wait until enough deliveries are concurrently in flight,
+            // or a short safety timeout (so the strictly-sequential config still drains).
+            await Task.WhenAny(_gate.Task, Task.Delay(TimeSpan.FromMilliseconds(400), cancellationToken))
+                .ConfigureAwait(false);
+
+            Interlocked.Decrement(ref _current);
+            return DeliveryOutcome.Success("ops@example.com");
+        }
+    }
+
+    private void SetupDueBatchOnce(int count)
+    {
+        var batch = Enumerable.Range(0, count).Select(_ => MakeNotification()).ToArray();
+        var claimed = 0;
+        _outboxRepository.GetDueAsync(Arg.Any<DateTimeOffset>(), Arg.Any<int>(), Arg.Any<CancellationToken>())
+            .Returns(_ => Interlocked.Increment(ref claimed) == 1
+                ? batch
+                : Array.Empty<Notification>());
+    }
+
+    [Fact]
+    public void Dispatch_WithMaxParallelFour_DeliversConcurrently_AndAllReachTerminal()
+    {
+        SetupSmtpRetryPolicy(maxRetries: 5, retryDelay: TimeSpan.FromMinutes(1));
+        SetupDueBatchOnce(8);
+        var adapter = new ConcurrencyHighWaterAdapter(releaseWhen: 2);
+        var actor = CreateActor(
+            [adapter],
+            new NotificationOutboxOptions
+            {
+                DispatchInterval = TimeSpan.FromHours(1),
+                MaxParallelDeliveries = 4,
+            });
+
+        actor.Tell(InternalMessages.DispatchTick.Instance);
+
+        AwaitAssert(
+            () =>
+            {
+                // All 8 notifications reached a terminal Delivered update.
+                _outboxRepository.Received(8).UpdateAsync(
+                    Arg.Is<Notification>(n => n.Status == NotificationStatus.Delivered),
+                    Arg.Any<CancellationToken>());
+                // At least two deliveries overlapped — the sweep is no longer sequential.
+                Assert.True(adapter.HighWater >= 2,
+                    $"expected concurrent deliveries (high-water >= 2), got {adapter.HighWater}");
+            },
+            duration: TimeSpan.FromSeconds(10));
+    }
+
+    [Fact]
+    public void Dispatch_WithMaxParallelOne_DeliversSequentially_AndAllReachTerminal()
+    {
+        SetupSmtpRetryPolicy(maxRetries: 5, retryDelay: TimeSpan.FromMinutes(1));
+        SetupDueBatchOnce(8);
+        var adapter = new ConcurrencyHighWaterAdapter(releaseWhen: 2);
+        var actor = CreateActor(
+            [adapter],
+            new NotificationOutboxOptions
+            {
+                DispatchInterval = TimeSpan.FromHours(1),
+                MaxParallelDeliveries = 1,
+            });
+
+        actor.Tell(InternalMessages.DispatchTick.Instance);
+
+        AwaitAssert(
+            () => _outboxRepository.Received(8).UpdateAsync(
+                Arg.Is<Notification>(n => n.Status == NotificationStatus.Delivered),
+                Arg.Any<CancellationToken>()),
+            duration: TimeSpan.FromSeconds(10));
+
+        // Strictly sequential: never more than one delivery in flight at once.
+        Assert.Equal(1, adapter.HighWater);
+    }
 }