Skip to content

Email Delivery Hardening (PR #27 - P1/P2 concurrency fixes)

Purpose

Extends Production Communications "Email delivery" - read that first. Independent review of PR #27 found the original EmailDeliveryBackgroundService unsafe under multiple backend replicas: any number of replicas could read the same GetActionableBatchAsync result and each call SES for the same row, and a worker crash between claiming and finalizing had no recovery story. This document covers the fix.

P1: multi-replica duplicate-send race

Root cause (original design)

The original EmailDeliveryBackgroundService read a batch of Pending/Retrying rows, called SES for each, then wrote the result - with no ownership step in between. Two replicas' timers firing in the same window would both read overlapping batches and both call SES for the same row: a genuine duplicate customer-facing email.

Fix: atomic claim before any provider I/O

EmailDeliveryStatus gained two states: Pending/Retrying (unchanged) -> Processing (new, claimed) -> Sent/Retrying/Failed (unchanged) or UnknownOutcome (new, see P1/P2 below).

EmailDeliveryAttempt gained four columns: ClaimToken (Guid, unique per claim), ClaimedBy (diagnostic worker identity), ClaimedAt, LeaseExpiresAt.

Every state transition in IEmailDeliveryRepository is a single, atomic, conditional SQL UPDATE (EF Core's ExecuteUpdateAsync, translated to one UPDATE ... WHERE ... statement, returning the affected-row count) - never a load-mutate-SaveChangesAsync sequence, and never wrapped in an explicit BeginTransactionAsync. This mirrors the exact pattern TokenService.RotateRefreshTokenAsync already uses in this codebase for the identical class of problem (atomic single-row consumption under concurrent replicas) - see Infrastructure/Services/Services.cs.

TryClaimAsync(id, claimToken, claimedBy, now, leaseDuration)
  UPDATE EmailDeliveryAttempts
  SET Status='Processing', ClaimToken=@token, ClaimedBy=@worker, ClaimedAt=@now, LeaseExpiresAt=@now+lease
  WHERE Id=@id AND (Status='Pending' OR (Status='Retrying' AND NextAttemptAt<=@now))
  -- affected==1 -> this call won the claim. affected==0 -> another worker already claimed/moved
  -- it since the candidate read (GetClaimableCandidatesAsync) - routine under concurrency.

A bare ExecuteUpdateAsync auto-commits as its own atomic statement, so the calling code can safely call SES (real network I/O) between TryClaimAsync and the finalize call without ever holding a database transaction open across it - the explicit requirement this hardening pass had to satisfy. Under MySQL/InnoDB, the UPDATE ... WHERE takes a locking read on matched rows as part of applying the change; a concurrent UPDATE against the same row blocks until the first commits, then re-evaluates its own WHERE against the now-current (no longer Pending/Retrying-due) row and correctly affects zero rows. This is proven directly against real MySQL - see "Tests" below.

Why not [ConcurrencyCheck]/EF native optimistic concurrency

EF's built-in concurrency tokens only support a single-column equality check ("has this row changed since I read it"), not the multi-column, comparison-based WHERE this needs (Status IN (...), NextAttemptAt <= now, LeaseExpiresAt <= now). A raw conditional ExecuteUpdateAsync - already an established pattern in this codebase - expresses the actual required condition directly and is a single round trip, no entity tracking involved.

P1/P2: crash-after-SES ambiguity - explicit UnknownOutcome policy

This design does not claim exactly-once delivery - it cannot, and does not try to. SES can accept a message and the process can crash (or its lease can simply expire under a slow/hung call) before TryFinalizeSentAsync runs. The chosen policy makes that ambiguity explicit rather than guessing:

Claimed (Processing) --[lease expires before any finalize]--> UnknownOutcome (terminal, NOT auto-retried)
  • SweepStaleLeasesToUnknownOutcomeAsync runs every tick, from every replica, before that tick's own claims: any Processing row whose LeaseExpiresAt has passed moves straight to UnknownOutcome - never back to Pending/Retrying for an automatic re-send. Running this redundantly from multiple replicas in the same moment is safe (itself just another conditional UPDATE; whichever replica's statement lands first wins, the rest affect zero rows for the same ids).
  • GetClaimableCandidatesAsync never returns UnknownOutcome rows - this is what bounds ambiguous deliveries to zero automatic retries, by construction, not by a counter. A worker restart is just another call to this method; it cannot "blindly resend" an ambiguous message because that message is structurally unreachable from the claim step.
  • Stale-claimant protection: if the original (slow, not actually crashed) worker's SES call eventually returns and it calls TryFinalizeSentAsync/TryFinalizeFailureAsync with its now-old ClaimToken, the conditional UPDATE's WHERE ... AND ClaimToken=@token finds no match (the sweep already cleared it) and affects zero rows. The background service logs this loudly, including the ProviderMessageId when one was returned, and does not retry writing - "stale worker must not overwrite the result of a newer claimant" is enforced by the same ClaimToken check that guards every other finalize path, not a special case.
  • Diagnostics preserved: ProviderMessageId is captured whenever SES returns one, even on the finalize call that ultimately fails its ClaimToken check (logged, not persisted, in that specific race) - an operator reconciling an UnknownOutcome row checks SES/CloudWatch around ClaimedAt/LastAttemptAt for a matching send.
  • Manual recovery path: EmailDeliveryAttempt.RequeueFromUnknownOutcome() / IEmailDeliveryRepository.TryRequeueFromUnknownOutcomeAsync moves a row back to Pending. Never called automatically by the background dispatcher - only by an operator (today: a maintenance script/direct repository call; no admin UI/endpoint was built in this hardening pass, see "Remaining limitations" below) after confirming out-of-band whether the ambiguous send actually went out.

Lease duration

Marketplace:Notifications:Ses:LeaseDurationSeconds (default 120s) - deliberately generous relative to typical SES call latency (sub-second to a few seconds), so a lease expiring is a meaningful signal of a stuck/dead worker, not normal slowness. A worker that discovers the provider is disabled mid-flight (EmailSendResult.Disabled()) releases its claim immediately via TryReleaseClaimAsync back to its original status, without incrementing AttemptCount - discovering the feature is off is not a send failure and must not consume retry budget or move the backoff schedule.

P2: enqueue idempotency

EmailDeliveryAttemptConfiguration adds HasIndex(x => x.NotificationId).IsUnique() - the "email is one-attempt-per-notification" model, mirroring MarketplaceNotification.IdempotencyKey's own unique-index pattern. IEmailDeliveryRepository.EnqueueAsync became TryEnqueueAsync (bool return), catching the duplicate-key DbUpdateException and detaching only the failed entity - exactly MarketplaceNotificationRepository.TryAddAsync's established pattern - so a harmless duplicate-enqueue attempt can never abort whatever ambient (possibly checkout- or return-scoped) transaction EmailNotificationChannel.DeliverAsync is running inside.

This is defense in depth: NotifyAsync's own idempotency (unique index on MarketplaceNotification.IdempotencyKey) already prevents DeliverAsync from running twice for the same business event in practice. The NotificationId unique index closes the gap for any future code path that might call EnqueueAsync more than once for the same notification by mistake.

Migration

The original AddEmailDeliveryAttempts migration (added in the first PR #27 commit, never applied to any real database) was regenerated from scratch (dotnet ef migrations remove + dotnet ef migrations add) rather than layered with a second AlterTable migration - the table itself was introduced in this same, still-unmerged PR, so one clean CreateTable migration with the final schema is better craftsmanship than "create, then immediately alter" for something that has never been deployed anywhere. See "Tests" below for how this was verified against real MySQL despite that verification being complicated by unrelated local dev-database drift.

Tests

  • EmailDeliveryConcurrencyTests.cs (SQLite, this codebase's existing convention for "conditional UPDATE, only one caller wins" logic-correctness tests - see CouponRepositoryUsageTests.TryConsumeUsageAsync_OnlyOneCallerCanTakeFinalSlot): claim atomicity (sequential and one genuinely concurrent Task.WhenAll race via a shared-cache in-memory SQLite database with two separate connections), active-lease-not-stolen, stale-lease sweep to UnknownOutcome, active-lease-not-swept, UnknownOutcome never returned by GetClaimableCandidatesAsync (worker-restart-does-not-blindly-resend), stale claimant cannot finalize after a sweep, stale claimant cannot undo a newer claimant's already-finalized result, bounded transient retry, immediate permanent termination, successful finalize clears the claim, provider-disabled release does not count as an attempt, and duplicate-NotificationId enqueue rejection.
  • MySqlEmailDeliveryConcurrencyTests.cs ([Trait("Category", "mysql-smoke")], gated by MYSQL_SMOKE_CONNSTR, mirroring MySqlRefreshTokenConcurrencySmokeTests.cs exactly): two genuinely concurrent workers (Task.WhenAll, two separate MySQL connections) race TryClaimAsync on the same row against real InnoDB row locking - proves exactly one wins and exactly one provider call occurs (a fake IEmailSender with an Interlocked-counted call count). A second test proves the stale-lease-sweep-then-stale-finalize sequence against real MySQL.
  • The existing SesEmailDeliveryTests.cs from the original PR #27 commit is unchanged and still passes unmodified - it tests SesEmailSender/EmailTemplateCatalog, neither of which this hardening pass touched.

MySQL verification note

This dev environment's local MySQL container turned out to have significant, pre-existing schema drift unrelated to this PR - SHOW TABLES/__EFMigrationsHistory revealed migrations for WhatsAppConsentAndOtp/DeviceTokens/NotificationPreferences/etc. that do not exist anywhere in this branch's history, while conversely lacking this branch's own MarketplaceNotifications table. dotnet ef database update therefore could not cleanly reconcile against it. Rather than force a migration onto a database in an unknown, foreign state, the new migration's exact SQL was extracted (dotnet ef migrations script) and applied directly - table/index/unique-constraint shape was then verified via DESCRIBE/SHOW INDEX, and both MySQL smoke tests above were run for real against it. This is a pre-existing environment inconsistency, not something introduced by this PR, and out of scope to remediate here.

Remaining limitations (explicitly not fixed in this pass)

  • No admin UI/endpoint for UnknownOutcome reconciliation. The repository primitive (TryRequeueFromUnknownOutcomeAsync) and domain method (EmailDeliveryAttempt.RequeueFromUnknownOutcome) exist; nothing exposes them via an API/admin page yet. An operator must currently act via a direct database/repository call.
  • No backoff jitter - ComputeBackoff remains a fixed exponential schedule (unchanged from the original PR #27 commit). A herd of retries could in principle synchronize under real multi-replica load; add jitter before relying on this at scale.
  • Lease duration is a single global config value, not adaptive to observed SES latency. Fine for initial low volume; revisit if send latency becomes more variable.
  • The stale-lease sweep and claim step are not combined into a single statement - two round trips per tick (sweep, then read candidates) rather than one. Simpler to reason about and test; a minor efficiency cost, not a correctness one, given the sweep is bulk/set-based.