deliveryRepository.popDueRetries() is not atomic — horizontally-scaled workers can double-process the same retry

[prodbycorne]

Overview

popDueRetries() in src/repositories/deliveryRepository.js is the mechanism src/jobs/webhookRetryWorker.js uses to claim due retries from the shared webhooks:retries Redis sorted set:

async function popDueRetries(nowMs, max = 25) {
  const redis = cache.getClient();
  const ids = await redis.zrangebyscore(RETRY_QUEUE_KEY, '-inf', nowMs, 'LIMIT', 0, max);
  if (ids.length === 0) return [];
  await redis.zrem(RETRY_QUEUE_KEY, ...ids);
  return ids;
}

This is a read, then delete — two separate round trips to Redis, not an atomic operation. webhookRetryWorker.js's in-process running boolean only prevents a single process from overlapping with itself; it provides zero protection across multiple instances of the backend running concurrently (the Docker Compose setup and any real production deployment behind a load balancer would run more than one replica for availability). If two app instances both call popDueRetries(Date.now(), 25) within the same small time window before either has executed its zrem, both instances' zrangebyscore calls can return the same delivery ids, and both will then call dispatcher.attempt(id) for those ids — resulting in the same webhook delivery being POSTed to the subscriber's endpoint twice for what should be a single retry attempt, doubling attempts bookkeeping unpredictably depending on which deliveryRepo.update() call lands last, and potentially double-firing a webhook that a subscriber has already deduplicated on delivery_id server-side (since both racing attempts would actually reuse the same delivery_id, but attempt independently and race on updating attempts/status, corrupting the delivery's audit trail).

Requirements

• Replace the read-then-delete pattern with a single atomic claim operation. Options include:
  • A Lua script (EVAL) that performs ZRANGEBYSCORE + ZREM atomically in one round trip.
  • ZPOPMIN-style iteration bounded by score (ioredis supports scripting; a small Lua script is the most direct fix given the existing sorted-set structure).
• The fix must guarantee that when N instances call popDueRetries concurrently against the same Redis, the union of ids each one receives is disjoint (no id is returned to more than one caller).
• Add a regression test that simulates two concurrent callers racing against a shared mock/real Redis sorted set and asserts no id is returned twice.
• Document the atomicity guarantee in the schema comment block already present at the top of deliveryRepository.js.

Acceptance Criteria

• Two concurrent calls to popDueRetries(now, max) against the same due-retry set never return an overlapping id.
• test/webhookRepository.test.js (or a new test file) includes a concurrency test proving the race is closed, not just a single-caller happy-path test.
• cancelRetry, scheduleRetry, and listByWebhook behavior is unchanged.
• No behavioral change to single-instance operation — existing test/webhookDispatcher.test.js retry-scheduling tests continue to pass unmodified.
• A short note is added to the README or a docs/ file clarifying that webhookRetryWorker is safe to run on multiple replicas without duplicate delivery.

Additional Notes

Additional edge cases / failure modes

• ZPOPMIN (mentioned as an alternative in the Requirements) pops the lowest-score members regardless of whether they're actually due yet — it does not take a score ceiling argument, so using it directly would require popping unconditionally and pushing back any items whose score is still in the future, which is itself a second race-prone operation. The Lua-script (EVAL) approach is the more direct fit here since it can combine the score-bounded ZRANGEBYSCORE and the ZREM in one atomic round trip without needing to "un-pop" anything — the Requirements list both options but this distinction is worth resolving explicitly rather than leaving ZPOPMIN as an equally-valid-looking choice in the PR.
• max = 25 (the batch size, config.webhooks.retryBatchSize) means each worker claims a bounded batch — with the atomic fix in place, confirm that N concurrently-running workers each still make progress (i.e. the retry queue overall throughput scales, not just correctness) rather than every worker repeatedly winning/losing a scramble for the same front-of-queue batch; this matters once retry queue depth (Missing production metrics for webhook delivery outcomes, retry-queue depth, and price-source failure rates #93 mentions metrics for this) is large enough that 25-per-tick isn't enough to keep up.
• The Lua script needs to be loaded via defineCommand (ioredis's mechanism, e.g. redis.defineCommand('popDueRetries', { numberOfKeys: 1, lua: '...' })) once at startup, not re-sent as an inline EVAL string on every call — inline EVAL still works but loses the EVALSHA caching benefit and re-parses the script every tick; prefer defineCommand so cache.getClient()'s existing client is extended once.
• Existing behavior when ids.length === 0 (empty due set) returns [] immediately without touching Redis again — the atomic version should preserve this short-circuit so the common "nothing due yet" case (most ticks, given WEBHOOK_RETRY_POLL_MS is presumably shorter than typical backoff windows) doesn't pay for a Lua EVAL round trip it doesn't need; the Lua script itself can just return an empty table cheaply, so this is likely a non-issue but worth confirming in the benchmark/test.
• webhookRetryWorker.js's own in-process running guard (tick()) becomes purely a local-overlap guard once cross-instance safety is handled by this fix — make sure the two mechanisms are understood as solving different problems (local re-entrancy vs. cross-replica double-claim) so a future refactor doesn't mistakenly remove one thinking the other makes it redundant.

Implementation sketch

// deliveryRepository.js
const POP_DUE_RETRIES_LUA = `
  local ids = redis.call('ZRANGEBYSCORE', KEYS[1], '-inf', ARGV[1], 'LIMIT', 0, ARGV[2])
  if #ids > 0 then
    redis.call('ZREM', KEYS[1], unpack(ids))
  end
  return ids
`;

function ensureScriptLoaded(redis) {
  if (!redis.popDueRetriesAtomic) {
    redis.defineCommand('popDueRetriesAtomic', { numberOfKeys: 1, lua: POP_DUE_RETRIES_LUA });
  }
}

async function popDueRetries(nowMs, max = 25) {
  const redis = cache.getClient();
  ensureScriptLoaded(redis);
  return redis.popDueRetriesAtomic(RETRY_QUEUE_KEY, nowMs, max);
}

Test / reproduction plan

1. Against a real (or ioredis-mock-backed, if it supports Lua scripting — verify first, may need a real Redis test container) Redis instance, seed the retry queue with 50 due ids; call popDueRetries(now, 25) twice concurrently via Promise.all; assert the two returned arrays are disjoint and their union has exactly 25+25=50 unique ids (or fewer if fewer than 50 were due, but never overlapping).
2. Simulate the pre-fix race directly (call zrangebyscore and zrem as two separate awaited steps with an injected delay between them, mimicking the old code) to first demonstrate duplicate ids are returned, then show the atomic version does not reproduce it — a good regression-test structure per the acceptance criteria's "not just a single-caller happy path" requirement.
3. Confirm cancelRetry/scheduleRetry still function against the same RETRY_QUEUE_KEY untouched by the Lua script change.

Related issues in this batch

• webhookDispatcher.dispatch() has no idempotency protection against duplicate event_id delivery #75 (dispatch idempotency) — the same class of "read-then-write across separate Redis round trips" race; the Lua-script/SET NX atomic-claim pattern proposed here is directly reusable for that issue's (webhook_id, event_id) claim.
• webhook_delivery:* records are never expired or pruned from Redis — unbounded memory growth #79 (unbounded delivery record growth) — a delivery id that's popped from the retry queue but whose underlying webhook_delivery:{id} record has since been pruned/expired is already handled gracefully by dispatcher.attempt()'s missing-delivery check; confirm that behavior is unaffected by switching to the atomic pop.
• Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98 (leader election for background jobs) — this issue is explicitly a lighter-weight alternative to leader election for the retry worker specifically (many workers safely sharing one queue via atomic claims, rather than exactly one worker running at all); worth noting in the PR that this fix is why the retry worker does not need leader election even if the price-refresh cron job (per Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98) does.

[boluwacodes]

I will solve this by replacing the non-atomic ZRANGEBYSCORE + ZREM implementation in popDueRetries() with a single atomic Redis Lua (EVAL) script that performs both operations in one execution, ensuring each retry ID is claimed by only one worker even when multiple webhookRetryWorker instances run concurrently. I will then add a concurrency regression test that invokes popDueRetries() simultaneously from two callers against the same Redis sorted set and verifies that no delivery ID is returned twice. Finally, I will update the documentation in deliveryRepository.js to describe the atomic claim guarantee and add a short README/docs note confirming that webhookRetryWorker is safe to run across multiple replicas without duplicate webhook deliveries, while keeping the behavior of scheduleRetry, cancelRetry, listByWebhook, and existing single-instance tests unchanged.

[grantfox-oss]

🦊 GrantFox — @boluwacodes has been assigned to this issue as part of the Official Campaign | FWC26 campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #76)
2. Your PR will be reviewed by the SmartDropLabs maintainers

Good luck! Track your progress on GrantFox.