fix(notification-outbox): keyset Pending back-fill + distinct ids so a mid-claim Retry cannot double-deliver (review 2026-10-21 06/PERF-5 follow-up)

2026-10-22 09:49:19 -04:00
parent 1627513fdb
commit 5f7f889a96
2 changed files with 94 additions and 2 deletions
@@ -172,14 +172,32 @@ VALUES
        if (remaining > 0 && pending.Count == batchSize - retryingQuota)
        {
            // Retrying could not fill its share; back-fill with newer Pending rows.
+            // KEYSET, not OFFSET: the claim spans several queries with no lock between
+            // them, so a row can turn Pending in the gap — an operator Retry flips an
+            // older Parked row back to Pending with its original CreatedAt. An OFFSET
+            // back-fill (Skip(pending.Count)) would then shift by one and re-read the
+            // last row already claimed, and the sweep would deliver that notification
+            // twice in parallel. Resuming strictly after the last claimed
+            // (CreatedAt, NotificationId) — the ORDER BY's own key — cannot overlap.
+            var last = pending[^1];
+            var lastCreatedAt = last.CreatedAt;
+            var lastId = last.NotificationId;
            pending.AddRange(await DuePendingQuery()
-                .Skip(pending.Count)
+                .Where(n => n.CreatedAt > lastCreatedAt
+                    || (n.CreatedAt == lastCreatedAt && string.Compare(n.NotificationId, lastId) > 0))
                .Take(remaining)
                .ToListAsync(cancellationToken));
        }

        pending.AddRange(retrying);
-        return pending.OrderBy(n => n.CreatedAt).ToList();
+
+        // Belt and braces: a row can also change status between the Pending and
+        // Retrying reads, so never hand the dispatcher the same id twice.
+        return pending
+            .DistinctBy(n => n.NotificationId)
+            .OrderBy(n => n.CreatedAt)
+            .ThenBy(n => n.NotificationId, StringComparer.Ordinal)
+            .ToList();

        IQueryable<Notification> DuePendingQuery() => _context.Notifications
            .AsNoTracking()