fix(outbox): resolve the delivery adapter per delivery, not per actor lifetime
Register row 45. NotificationOutboxActor.ResolveAdapters() cached adapter instances in an actor-lifetime IServiceScope (_adaptersScope), but both EmailNotificationDeliveryAdapter and SmsNotificationDeliveryAdapter are AddScoped and hold a scoped INotificationRepository over one EF DbContext. MaxParallelDeliveries (default 4) then drove up to four concurrent deliveries through that single adapter and single context, which central-b logged continuously on the rig as "InvalidOperationException: A second operation was started on this context instance before a previous operation completed". The collided delivery's row is never written and the wasted attempt counts against its retry budget, so sustained contention could park a legitimate notification. The per-delivery scope DeliverGatedAsync already opened for the outbox repository — added for exactly this hazard — did not cover the adapter. _adaptersCache, _adaptersScope and ResolveAdapters() are deleted. Delivery adapters are now resolved inside the per-delivery scope via a static ResolveAdapter(IServiceProvider, NotificationType), preserving DI's last-wins resolution semantics (the enumeration runs to completion rather than short-circuiting) and the null-adapter park path. Nothing shared is lost: the Email adapter builds a fresh SMTP client per attempt from an injected factory and the SMS adapter draws pooled connections from the singleton IHttpClientFactory, so no adapter carried state worth preserving across deliveries. The Central UI's NotificationChannelCatalog is an independent consumer of the same scoped registrations and is untouched, so the adapter-gated Type selector still enumerates exactly the deliverable channels. Tests: Dispatch_ResolvesAdaptersOnce_AcrossMultipleSweeps (which pinned the superseded caching behaviour) is replaced by ParallelDeliveries_EachGetTheirOwnAdapterAndDbContext — two parallel deliveries through a scoped repository whose simulated DbContext throws EF's exact second-operation exception on overlap, with a rendezvous that guarantees the overlap window — and Dispatch_ResolvesAdapterPerDelivery_NotOncePerActorLifetime. Both fail against the pre-fix actor (stash-verified: 1 of 2 rows persisted; 1 distinct adapter instead of 4) and pass after. Parallel-delivery ordering/throughput semantics are unchanged. Docs: Component-NotificationOutbox.md gains the scoped-adapter contract under Delivery Adapters (resolve per delivery; adapters carry no cross-delivery state; genuinely shared resources belong in a singleton/factory), and the NotificationOutbox-006 finding is marked SUPERSEDED with the reasoning. Solution build clean; NotificationOutbox (153), AuditLog (383), integration NotificationOutbox flow (2) and CentralUI notification (14) suites green.
This commit is contained in:
@@ -48,7 +48,7 @@ from `Component-NotificationOutbox.md`. No Critical findings; two High, six Medi
|
||||
| 3 | Concurrency & thread safety | Yes | Actor state mutated only on actor thread. Inherited CD-015 race on `InsertIfNotExistsAsync` (NotificationOutbox-005) is the only race; the dispatcher's in-flight guard correctly serializes sweeps. |
|
||||
| 4 | Error handling & resilience | Yes | Outer try/catch on `RunDispatchPass`/`RunPurgePass` keeps the in-flight guard sane; per-notification isolation is correct. CT not threaded into delivery (NotificationOutbox-003). |
|
||||
| 5 | Security | Yes | Inherited OAuth2 empty-user (NotificationOutbox-001) reachable through the adapter. No new credential or trust-boundary issues introduced by the outbox itself. |
|
||||
| 6 | Performance & resource management | Yes | Dispatch interval & batch size are simple polling; `ResolveAdapters` rebuilds the lookup per sweep (NotificationOutbox-006). No leaks. |
|
||||
| 6 | Performance & resource management | Yes | Dispatch interval & batch size are simple polling; `ResolveAdapters` rebuilds the lookup per sweep (NotificationOutbox-006 — its 2026-05-28 caching fix was later **superseded**, see the finding). No leaks. |
|
||||
| 7 | Design-document adherence | Yes | `NotificationOutboxOptions.DispatchBatchSize`, `DeliveredKpiWindow`, `PurgeInterval` are not in the design doc (NotificationOutbox-007). |
|
||||
| 8 | Code organization & conventions | Yes | Options class lives in the component project (correct); DI extension lives in the component (correct); adapter is `scoped`, actor singleton — interaction correctly documented in `ServiceCollectionExtensions`. No issues. |
|
||||
| 9 | Testing coverage | Yes | Solid actor-behaviour coverage. Missing tests for `FallbackMaxRetries` / empty-SMTP-config dispatch path (NotificationOutbox-008). |
|
||||
@@ -312,6 +312,8 @@ stateful intent (timeouts, circuit breakers) cannot accidentally lose state.
|
||||
|
||||
**Resolution (2026-05-28):** Cached the `NotificationType → adapter` dictionary on a new actor field `_adaptersCache`, built lazily on the first dispatch sweep that needs it. The cache is paired with an actor-lifetime `IServiceScope` (`_adaptersScope`, created on first use and disposed in `PostStop`) so the resolved scoped adapter instances live as long as the cache itself — respecting `EmailNotificationDeliveryAdapter`'s scoped lifetime without leaking captive-DbContext references across the actor's full lifetime. `RunDispatchPass` now calls `ResolveAdapters()` (instance method, no args), and the existing "resolved from the per-sweep scope" comment was rewritten to cross-reference the new cache rationale. Adapter set is static per process lifetime (confirmed against the DI registration in `AddNotificationOutbox`), so no invalidation hook is needed. Regression test `Dispatch_ResolvesAdaptersOnce_AcrossMultipleSweeps` registers a counting factory and pins the resolution count at exactly 1 across three end-to-end dispatch sweeps.
|
||||
|
||||
**SUPERSEDED 2026-08-15 (deferred register row 45).** The 2026-05-28 resolution above was **wrong and is reverted** — it was written before the dispatcher gained bounded parallel delivery (`MaxParallelDeliveries`, default 4). An actor-lifetime adapter instance is a *captive scoped service*: the adapter holds a scoped `INotificationRepository` over one EF `DbContext`, so all four concurrent deliveries ran on that single context and central-b logged `InvalidOperationException: A second operation was started on this context instance before a previous operation completed` continuously on the rig. The collided delivery's row is never written and the wasted attempt counts against its retry budget, so sustained contention could park a legitimate notification. `_adaptersCache`, `_adaptersScope` and `ResolveAdapters()` are deleted; `DeliverGatedAsync` now resolves the adapter for the notification's `Type` from the per-delivery DI scope it already opens (static `ResolveAdapter(IServiceProvider, NotificationType)`, last-wins preserved). The original NotificationOutbox-006 concern — "a future stateful adapter (rate limiter, circuit breaker) silently loses its state" — is answered by the design rule instead of by caching: an adapter must carry no cross-delivery state, and anything genuinely shared belongs in a singleton/factory it depends on (as the SMTP client factory and `IHttpClientFactory` already are). `Dispatch_ResolvesAdaptersOnce_AcrossMultipleSweeps` is replaced by `ParallelDeliveries_EachGetTheirOwnAdapterAndDbContext` (a simulated per-repository DbContext that throws EF's exact second-operation exception; fail-verified against the cached build) and `Dispatch_ResolvesAdapterPerDelivery_NotOncePerActorLifetime`. See [Component-NotificationOutbox.md](../../docs/requirements/Component-NotificationOutbox.md), Delivery Adapters.
|
||||
|
||||
### NotificationOutbox-007 — `NotificationOutboxOptions.DispatchBatchSize`, `DeliveredKpiWindow`, and `PurgeInterval` are not in the design document
|
||||
|
||||
| | |
|
||||
|
||||
Reference in New Issue
Block a user