Webhook retry sweep is not concurrency-safe across replicas: subscribers receive duplicate deliveries

[Xhristin3]

Problem

The webhook retry sweep is not safe across API replicas, so subscribers receive duplicate deliveries whenever more than one API instance runs. WebhooksService.sweepRetries() (api/src/webhooks/webhooks.service.ts) is a @Interval-scheduled method that runs in every API process (the k8s Deployment k8s/20-api.yaml and the README's horizontal-scaling story assume replicas). Each sweep calls:

const due = await this.deliveries.findDuePending(RETRY_SWEEP_BATCH_SIZE)

findDuePending (api/src/webhooks/repository/webhook-deliveries-db.repository.ts) selects rows with status = 'pending' AND next_attempt_at <= CURRENT_TIMESTAMP with no locking and no claim state. Two replicas sweeping in the same minute both select the same due deliveries and both POST to the subscriber — attemptDelivery records each attempt independently, so webhook_deliveries.attempt_count increments twice as fast as intended and the subscriber receives the same webhook body twice (or more, with N replicas). The same race exists in the fire-and-forget path: dispatchStreamEvent creates a delivery row and calls attemptDelivery in every replica that handles the triggering status transition, so a single PATCH /streams/:id fan-out can be delivered once per replica.

Consequence: webhook subscribers (the integration point the platform sells, issue #392) cannot rely on at-most-once semantics; retry backoff ("up to 5 retries over 24 hours") collapses under replica count, and a replica restart mid-sweep can re-deliver a batch that another replica already delivered. With no idempotency key on the wire (X-Webhook-Signature proves authenticity, not uniqueness), subscribers must dedupe themselves or receive duplicates.

Root cause

// api/src/webhooks/repository/webhook-deliveries-db.repository.ts — findDuePending()
SELECT ... FROM webhook_deliveries
WHERE status = 'pending' AND next_attempt_at <= CURRENT_TIMESTAMP
ORDER BY created_at ASC
LIMIT $1                      // ← no FOR UPDATE, no claim, no per-row atomicity

// api/src/webhooks/webhooks.service.ts — sweepRetries()
const due = await this.deliveries.findDuePending(RETRY_SWEEP_BATCH_SIZE)
for (const delivery of due) { ... await this.attemptDelivery(subscription, delivery) }  // ← every replica does this

Why this is architecturally hard

1. The fix requires an atomic claim, not a bigger batch: each due delivery must be claimed by exactly one replica before the POST. The Postgres-native pattern is UPDATE webhook_deliveries SET status = 'claiming' (or next_attempt_at = NOW() + lease) WHERE id = ANY($1) AND status = 'pending' RETURNING id — or SELECT ... FOR UPDATE SKIP LOCKED inside a transaction — and the design must decide whether a crashed replica's in-flight delivery is recoverable (a lease timestamp that another sweep can take over) or risk being stuck forever.
2. The status column is constrained by webhook_deliveries_status_check to ('pending', 'success', 'failed') (database/schema.sql), so a new intermediate state (e.g. delivering) requires a migration to widen the check. Alternatively the lease can live in next_attempt_at without a new status — a design choice with schema impact either way.
3. dispatchStreamEvent's fire-and-forget path races too: deliveries.create() then attemptDelivery() per replica. The claim must apply at creation time (or the fan-out must be made idempotent per (subscription, event, payload)), which changes the meaning of a delivery row: one row per event per subscription, claimed once, versus one row per replica attempt.
4. Any claim mechanism must survive recordAttempt's update (which bumps attempt_count and rewrites next_attempt_at) without deadlocking two replicas on the same row — the existing UPDATE is unguarded and would happily overwrite a claim made by another replica.

Acceptance criteria

Behaviour

• With two API replicas sweeping concurrently (integration test against the test database), each due delivery is POSTed to the subscriber exactly once per sweep cycle.
• A single PATCH /streams/:id status transition with two replicas results in exactly one delivery row and one POST per matching subscription.
• A replica that crashes after claiming a delivery does not strand it: the delivery is retried by a later sweep after a bounded lease (no permanent pending stuck state).

Tests

• Integration test: seed a due pending delivery, run two concurrent sweeps (or two service instances against one DB), assert the subscriber endpoint receives exactly one POST and attempt_count increments by one.
• Test that the lease expiry path recovers a crashed replica's claim.
• Existing webhook specs (api/src/webhooks/webhooks.service.spec.ts) pass with the new claim semantics.

Documentation

• The delivery semantics (at-least-once, per-event claim) are documented in the WebhooksService JSDoc and the webhook README/docs so subscribers know what to expect.

Out of scope

An idempotency key for subscribers (wire change, separate decision), and delivery retry policy changes (the 1min/5min/30min/3h/20h schedule stays).

Getting started

Real files in scope: api/src/webhooks/webhooks.service.ts (sweepRetries, dispatchStreamEvent, attemptDelivery), api/src/webhooks/repository/webhook-deliveries-db.repository.ts (findDuePending, recordAttempt, create), database/schema.sql (webhook_deliveries status check), database/migrations/ (new migration if a status widens), api/src/webhooks/webhooks.service.spec.ts.

Verify with:

cd api && npm run typecheck && npm test

Good first files to read: api/src/webhooks/webhooks.service.ts (sweepRetries), api/src/webhooks/repository/webhook-deliveries-db.repository.ts (findDuePending), database/schema.sql (the webhook_deliveries_status_check constraint).

[boss572]

Hello maintainer please I will like to work on this issue

[sebas11042]

Hi! I’m a software developer and open-source contributor with experience in backend development, databases, retries, idempotency, and distributed processing flows. I’ve been focusing on concurrency scenarios where multiple workers or replicas can race over the same piece of work and accidentally execute an external side effect more than once.

For this issue, I’d introduce an atomic claim/lease mechanism for due webhook deliveries so only one replica can own and POST each delivery at a time, while still allowing another replica to recover it after a bounded lease if the original worker crashes. I’d apply the same uniqueness guarantees to the initial dispatch path, keep retry bookkeeping consistent with the active claim, and add integration tests with concurrent service instances proving one delivery row and one POST per subscription, plus safe recovery after lease expiry.

[grantfox-oss]

🦊 GrantFox — @boss572 has been assigned to this issue as part of the Third Campaign campaign!

Next steps:

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

Good luck! Track your progress on GrantFox.