fix(notifications): list sort index + CAS operator Retry/Discard (P5-3, P5-4)
P5-3: add IX_Notifications_Created_NotificationId (CreatedAt DESC, NotificationId DESC) — the Central UI Notification Outbox list page sorts on exactly that pair, but no existing index leads unconditionally on CreatedAt, so an unfiltered/lightly filtered page fell back to a full clustered-index scan + sort on every COUNT + page request. Mirrors the Medium wave's IX_SiteCalls_Created_TrackedOperationId migration + SQL twin pattern faithfully, including the handwritten docs/plans/sql script. P5-4: operator Retry/Discard was read-check-write keyed on NotificationId only — a detached PipeTo task interleaving with the dispatcher's own write (or a second concurrent operator action) could land between the read and the write and get silently clobbered by the unconditional UpdateAsync, stamping a Delivered row Discarded with stale fields. Adds INotificationOutboxRepository.TryUpdateFromParkedAsync, a single-statement compare-and-swap (WHERE Status == Parked) mirroring SecuredWriteRepository.TryMarkApprovedAsync's CAS shape. NotificationOutboxActor's RetryAsync/DiscardAsync now use the CAS write and, on a lost race, re-read to report "notification already progressed to <Status>" instead of silently overwriting whatever is there now. Proof of failure: reverting only the actor's CAS usage while keeping the repository addition reproduces the silent-overwrite bug — 7 tests fail, including two new deterministic (non-sleep) race-proof tests that mock the exact GetByIdAsync -> lost-CAS -> re-read sequence the finding describes. P5-3 is perf-only; verified via migration/SQL-twin parity instead (no runtime proof-of-failure is meaningful for an index).
This commit is contained in:
Joseph Doherty
@@ -573,6 +573,154 @@ public class NotificationOutboxRepositoryTests : IDisposable
|
||||
Assert.False(await _repository.UpdateAsync(loaded));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task TryUpdateFromParkedAsync_StatusIsParked_UpdatesAndReturnsTrue()
|
||||
{
|
||||
var id = Guid.NewGuid().ToString();
|
||||
_context.Notifications.Add(MakeNotification(id, NotificationStatus.Parked));
|
||||
await _context.SaveChangesAsync();
|
||||
_context.ChangeTracker.Clear();
|
||||
|
||||
var loaded = await _repository.GetByIdAsync(id);
|
||||
loaded!.Status = NotificationStatus.Pending;
|
||||
loaded.RetryCount = 0;
|
||||
loaded.NextAttemptAt = null;
|
||||
loaded.LastError = null;
|
||||
Assert.True(await _repository.TryUpdateFromParkedAsync(loaded));
|
||||
|
||||
_context.ChangeTracker.Clear();
|
||||
var reloaded = await _context.Notifications.FindAsync(id);
|
||||
Assert.Equal(NotificationStatus.Pending, reloaded!.Status);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Direct CAS-miss: the row is currently Delivered (never Parked in this
|
||||
/// scenario), so the conditional WHERE Status == Parked matches nothing and
|
||||
/// the row is left completely untouched — arch-review 2026-08-19 P5-4.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task TryUpdateFromParkedAsync_StatusIsNotParked_ReturnsFalse_AndLeavesRowUnchanged()
|
||||
{
|
||||
var id = Guid.NewGuid().ToString();
|
||||
var deliveredAt = new DateTimeOffset(2026, 5, 19, 9, 0, 0, TimeSpan.Zero);
|
||||
_context.Notifications.Add(MakeNotification(
|
||||
id, NotificationStatus.Delivered, deliveredAt: deliveredAt));
|
||||
await _context.SaveChangesAsync();
|
||||
_context.ChangeTracker.Clear();
|
||||
|
||||
var staleDiscard = MakeNotification(id, NotificationStatus.Discarded);
|
||||
Assert.False(await _repository.TryUpdateFromParkedAsync(staleDiscard));
|
||||
|
||||
_context.ChangeTracker.Clear();
|
||||
var reloaded = await _context.Notifications.FindAsync(id);
|
||||
Assert.Equal(NotificationStatus.Delivered, reloaded!.Status);
|
||||
Assert.Equal(deliveredAt, reloaded.DeliveredAt);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task TryUpdateFromParkedAsync_RowPurgedBeforeWrite_ReturnsFalse()
|
||||
{
|
||||
var id = Guid.NewGuid().ToString();
|
||||
_context.Notifications.Add(MakeNotification(id, NotificationStatus.Parked));
|
||||
await _context.SaveChangesAsync();
|
||||
_context.ChangeTracker.Clear();
|
||||
|
||||
var loaded = await _repository.GetByIdAsync(id);
|
||||
Assert.NotNull(loaded);
|
||||
|
||||
await _context.Notifications.Where(n => n.NotificationId == id).ExecuteDeleteAsync();
|
||||
_context.ChangeTracker.Clear();
|
||||
|
||||
loaded!.Status = NotificationStatus.Discarded;
|
||||
Assert.False(await _repository.TryUpdateFromParkedAsync(loaded));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Deterministic (non-sleep) proof of the arch-review 2026-08-19 P5-4 race:
|
||||
/// the operator reads a Parked row, but before its write lands the
|
||||
/// dispatcher (or a concurrent second operator action) has already moved
|
||||
/// the row to Delivered. <see cref="TryUpdateFromParkedAsync"/>'s CAS must
|
||||
/// refuse the stale write and leave the Delivered row exactly as it is.
|
||||
/// The companion fact below proves the general-purpose
|
||||
/// <see cref="NotificationOutboxRepository.UpdateAsync"/> — the method the
|
||||
/// operator path used PRE-FIX — has no such guard, which is the mechanism
|
||||
/// the finding describes.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task TryUpdateFromParkedAsync_ConcurrentlyDelivered_ReturnsFalse_AndDoesNotOverwrite()
|
||||
{
|
||||
var id = Guid.NewGuid().ToString();
|
||||
_context.Notifications.Add(MakeNotification(id, NotificationStatus.Parked));
|
||||
await _context.SaveChangesAsync();
|
||||
_context.ChangeTracker.Clear();
|
||||
|
||||
// The operator's read (this snapshot is carried forward stale below).
|
||||
var operatorRead = await _repository.GetByIdAsync(id);
|
||||
Assert.NotNull(operatorRead);
|
||||
|
||||
// The dispatcher's own write lands first, in the window between the
|
||||
// operator's read and write.
|
||||
var deliveredAt = new DateTimeOffset(2026, 5, 19, 9, 0, 0, TimeSpan.Zero);
|
||||
var dispatcherRead = await _repository.GetByIdAsync(id);
|
||||
dispatcherRead!.Status = NotificationStatus.Delivered;
|
||||
dispatcherRead.DeliveredAt = deliveredAt;
|
||||
Assert.True(await _repository.UpdateAsync(dispatcherRead));
|
||||
_context.ChangeTracker.Clear();
|
||||
|
||||
// The operator's stale in-memory copy still says Parked; it attempts a Discard.
|
||||
operatorRead!.Status = NotificationStatus.Discarded;
|
||||
Assert.False(await _repository.TryUpdateFromParkedAsync(operatorRead));
|
||||
|
||||
_context.ChangeTracker.Clear();
|
||||
var reloaded = await _context.Notifications.FindAsync(id);
|
||||
Assert.Equal(NotificationStatus.Delivered, reloaded!.Status);
|
||||
Assert.Equal(deliveredAt, reloaded.DeliveredAt);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Companion to <see cref="TryUpdateFromParkedAsync_ConcurrentlyDelivered_ReturnsFalse_AndDoesNotOverwrite"/>:
|
||||
/// runs the IDENTICAL interleaving through the unconditional
|
||||
/// <see cref="NotificationOutboxRepository.UpdateAsync"/> instead — the
|
||||
/// method the operator Retry/Discard path called pre-fix — and shows it
|
||||
/// silently clobbers the Delivered row. This documents the exact defect
|
||||
/// arch-review 2026-08-19 P5-4 fixed by routing the operator path through
|
||||
/// <see cref="TryUpdateFromParkedAsync"/>: UpdateAsync itself is unchanged
|
||||
/// (the dispatcher still needs an unconditional write), so this scenario
|
||||
/// remains "true" by design — it is the operator actor's call-site choice
|
||||
/// that was the bug, proven fixed by the actor-level tests in
|
||||
/// NotificationOutbox.Tests (Discard_ConcurrentlyDelivered_...).
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task UpdateAsync_Unconditional_WouldOverwriteConcurrentlyDeliveredRow_DemonstratingP5_4Mechanism()
|
||||
{
|
||||
var id = Guid.NewGuid().ToString();
|
||||
_context.Notifications.Add(MakeNotification(id, NotificationStatus.Parked));
|
||||
await _context.SaveChangesAsync();
|
||||
_context.ChangeTracker.Clear();
|
||||
|
||||
var operatorRead = await _repository.GetByIdAsync(id);
|
||||
Assert.NotNull(operatorRead);
|
||||
|
||||
var deliveredAt = new DateTimeOffset(2026, 5, 19, 9, 0, 0, TimeSpan.Zero);
|
||||
var dispatcherRead = await _repository.GetByIdAsync(id);
|
||||
dispatcherRead!.Status = NotificationStatus.Delivered;
|
||||
dispatcherRead.DeliveredAt = deliveredAt;
|
||||
Assert.True(await _repository.UpdateAsync(dispatcherRead));
|
||||
_context.ChangeTracker.Clear();
|
||||
|
||||
operatorRead!.Status = NotificationStatus.Discarded;
|
||||
// The unconditional UPDATE has no status guard, so it "succeeds" here —
|
||||
// this is the bug. It clobbers the Delivered row to Discarded and drops
|
||||
// DeliveredAt, which is exactly the silent-overwrite hazard P5-4 closes
|
||||
// by moving the operator path to TryUpdateFromParkedAsync.
|
||||
Assert.True(await _repository.UpdateAsync(operatorRead));
|
||||
|
||||
_context.ChangeTracker.Clear();
|
||||
var reloaded = await _context.Notifications.FindAsync(id);
|
||||
Assert.Equal(NotificationStatus.Discarded, reloaded!.Status);
|
||||
Assert.Null(reloaded.DeliveredAt);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task QueryAsync_AppliesFilters_OrdersByCreatedAtDescending_AndPaginates()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user