webhookDispatcher.dispatch() has no idempotency protection against duplicate event_id delivery

[prodbycorne]

Overview

dispatch({ event_type, event_id, data }) in src/services/webhookDispatcher.js always creates a brand-new delivery record for every matching webhook, every time it is called — there is no check for whether a delivery already exists for a given (webhook_id, event_id) pair:

async function dispatch({ event_type: eventType, event_id: eventId, data }) {
  ...
  const targets = await webhookRepo.listActiveForEvent(eventType, events.matchesSubscription);
  if (targets.length === 0) return [];
  ...
  return Promise.all(
    targets.map((webhook) => deliverToWebhook(webhook, eventType, eventId, payload))
  );
}

async function deliverToWebhook(webhook, eventType, eventId, payload) {
  const delivery = await deliveryRepo.create({ webhook_id: webhook.id, event_id: eventId, event_type: eventType });
  ...
}

event_id is documented as a required, caller-supplied string (dispatch throws if it's missing), clearly intended to identify a specific occurrence of a domain event (e.g. a specific pool.assets_locked event for a specific pool/ledger). But nothing in deliveryRepo or dispatch prevents the same caller — or a future on-chain event producer that re-processes an event after a crash/restart, a retried job, or an at-least-once queue redelivering a message — from calling dispatch() twice with an identical event_id. Each call creates an entirely independent deliveryRepo record and sends an entirely independent signed HTTP request to every subscribed webhook, with no way for the dispatcher itself to recognize or collapse the duplicate. Subscribers receive genuine duplicate webhook deliveries (different delivery_id, same event_id), not simply the documented "at-least-once retries" of a single delivery attempt.

This matters specifically because SmartDrop intends to wire real pool lifecycle events (pool.created, pool.assets_locked, etc. — already defined in src/services/webhookEvents.js) into this exact dispatch() entrypoint, and any future indexer that re-scans a block range after a restart (a completely normal and expected recovery behavior) will re-emit the same logical event.

Requirements

• Before creating a new delivery in deliverToWebhook, check whether a delivery already exists for the (webhook_id, event_id) pair.
• If one exists and its status is success or currently pending (i.e. actively being retried), skip creating a duplicate and return the existing record instead of dispatching a fresh HTTP request.
• If one exists and is failed (retries exhausted), the desired behavior needs a decision documented in the PR: either still refuse to re-dispatch automatically (only a manual /webhooks/:id/test-style re-trigger allowed), or allow explicit re-dispatch via a new idempotent "redeliver" endpoint — pick one and justify it in the PR description; do not silently re-fire failed deliveries as a side effect of dispatch() being called again.
• Add an index/lookup structure in deliveryRepository.js keyed by (webhook_id, event_id) (the schema comment at the top of that file already models a future Postgres table — extend it to include a unique constraint mirroring this).
• This check needs to be race-safe: two near-simultaneous dispatch() calls with the same event_id must not both pass a "does it exist" check and both create records (use an atomic Redis SET ... NX-style claim, not read-then-write).

Acceptance Criteria

• Calling dispatch() twice with the same event_type/event_id results in exactly one delivery record and one outbound HTTP request per subscribed webhook, not two.
• The second call returns the existing delivery record(s) rather than undefined/an error.
• A race test (two concurrent dispatch() calls with the same event_id, mocked to resolve out of order) still produces only one delivery per webhook.
• deliveryRepository.js's schema comment is updated to document the new (webhook_id, event_id) uniqueness guarantee.
• Existing tests in test/webhookDispatcher.test.js continue to pass; new tests cover the duplicate-event_id scenario explicitly.

Additional Notes

Additional edge cases / failure modes

• sendTest() (webhookDispatcher.js:180-191) generates its own synthetic event_id (evt_test_${Date.now()}) and calls deliverToWebhook directly, bypassing dispatch()'s target-resolution but reusing the same deliverToWebhook/deliveryRepo.create path. Any idempotency key lookup added to deliverToWebhook must not accidentally treat two rapid test-sends (same millisecond Date.now(), extremely plausible under fast automated testing/CI) as duplicates of each other — either give test deliveries a distinct id namespace or accept that repeat test-sends within the same millisecond collapse (probably fine, but should be a documented, deliberate consequence rather than an accident).
• A webhook that is active: false at dispatch time but flips to active: true later — webhookRepo.listActiveForEvent filters targets at dispatch time, so a re-dispatch for the same event_id after the webhook becomes active would currently create the "first ever" delivery for that webhook, which is correct/desired, but if the idempotency key is scoped only to (webhook_id, event_id) and a webhook is deleted and a new webhook is created reusing the same generated id (astronomically unlikely given crypto.randomUUID(), but worth a one-line note) this would be a non-issue; more realistically, confirm the idempotency key doesn't leak across webhook update()s that change url/secret — an existing pending delivery record references webhook_id, and attempt() always re-reads the current webhook record (webhookRepo.findById(delivery.webhook_id)), so a URL/secret rotation mid-retry-cycle changes where a "duplicate-suppressed" retry ultimately lands — call this out as expected but non-obvious behavior.
• The claim mechanism needs a decision on TTL: an idempotency key that lives forever in Redis is consistent with deliveryRepo's current no-TTL behavior (see webhook_delivery:* records are never expired or pruned from Redis — unbounded memory growth #79) but compounds that issue's unbounded-growth problem; if webhook_delivery:* records are never expired or pruned from Redis — unbounded memory growth #79's retention window fix lands first, the idempotency lookup structure must not outlive the underlying delivery record it protects (i.e. don't create a second forever-lived key while webhook_delivery:* records are never expired or pruned from Redis — unbounded memory growth #79 fixes the first one).

Implementation sketch (approaches)

1. Atomic claim key, minimal schema change: SET webhook_delivery_idx:{webhook_id}:{event_id} {delivery_id} NX before deliveryRepo.create(); on NX failure, GET the existing delivery_id and return deliveryRepo.findById() of it instead of creating a new record. Simple, race-safe via Redis's atomic SET NX, and mirrors the TTL/retention lifecycle of the delivery record itself if given the same expiry.
2. Extend the existing per-webhook sorted-set index: add a second Redis hash webhook:{webhook_id}:event_index mapping event_id -> delivery_id, written via HSETNX (atomic, no-clobber) at the same time as zadd in deliveryRepo.create(). Slightly more schema to maintain but keeps all delivery-indexing logic colocated in deliveryRepository.js rather than introducing a new key pattern.

Either approach: deliverToWebhook checks-or-claims first, and only proceeds to deliveryRepo.create() + attempt() on a successful claim; a failed claim short-circuits to fetching and returning the existing record.

Test / reproduction plan

1. Call dispatcher.dispatch({ event_type: 'pool.assets_locked', event_id: 'evt_123', data: {} }) twice sequentially against one subscribed webhook; assert exactly one delivery record exists for that (webhook_id, event_id) and the mocked axios.post was called once.
2. Race two dispatch() calls with the same event_id via Promise.all, with the underlying claim operation's timing manipulated (e.g. via a mocked Redis client with an artificial delay on the first caller's SET NX) to force interleaving; assert only one delivery/HTTP POST results.
3. Test the failed-status re-dispatch decision explicitly per whichever policy is chosen (either: dispatch() again returns the existing failed record and does not re-attempt; or: a new redeliver endpoint is required).
4. Confirm sendTest() behavior is unaffected (or its interaction with the new claim key is explicitly tested if it shares the same code path).

Related issues in this batch

• deliveryRepository.popDueRetries() is not atomic — horizontally-scaled workers can double-process the same retry #76 (popDueRetries atomicity) — both issues are instances of the same root problem (non-atomic Redis read-then-write patterns in the webhook delivery pipeline); the Lua-script/atomic-claim technique used to fix deliveryRepository.popDueRetries() is not atomic — horizontally-scaled workers can double-process the same retry #76 is directly reusable for this issue's race-safety requirement.
• webhookDispatcher.dispatch() uses Promise.all — one failing target can obscure delivery outcome for every other subscriber #77 (Promise.all masking partial failures) — once idempotency is added, a caller retrying a dispatch() call that partially failed (per webhookDispatcher.dispatch() uses Promise.all — one failing target can obscure delivery outcome for every other subscriber #77) needs the retry to skip the targets that already succeeded and only re-attempt the ones that didn't — this issue's per-(webhook_id, event_id) granularity is what makes that safe.
• webhook_delivery:* records are never expired or pruned from Redis — unbounded memory growth #79 (delivery record pruning) — the idempotency claim key's lifecycle must stay consistent with whatever retention/TTL policy webhook_delivery:* records are never expired or pruned from Redis — unbounded memory growth #79 introduces for the underlying delivery records.
• Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98 (leader election for background jobs) — once real on-chain event indexing exists, an indexer replica re-scanning after a crash is the concrete trigger scenario this issue's Overview describes; Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98's leader-election design and this issue's idempotency design should be considered together at the indexer layer even though this issue's fix lives at the dispatcher layer.

[Dickson2015]

I would love to work on this issue I deliver very timely.