You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
webhookDispatcher.js's retry backoff is purely deterministic — the same attempt count always produces the exact same delay — with no randomization (jitter) at all. Combined with webhookRetryWorker's fixed 5-second poll interval and 25-item batch size, this creates a textbook synchronized-retry thundering-herd risk against any subscriber whose endpoint experiences an outage affecting multiple in-flight deliveries at once.
With the documented defaults (WEBHOOK_RETRY_BASE_MS=30000, WEBHOOK_RETRY_FACTOR=2), every delivery that fails on its first attempt computes nextRetryAt = now + 30000ms — exactly. Every delivery that fails on its second attempt (regardless of which delivery, which webhook, or which subscriber) computes nextRetryAt = now + 60000ms — exactly. There is no + Math.random() * jitterMs term anywhere in this function or anywhere else in the retry path (attempt(), deliveryRepo.scheduleRetry).
Concretely: if a subscriber's endpoint goes down for, say, 90 seconds during a deploy or a brief outage, every delivery attempted against it during that window fails at roughly the same wall-clock moments (driven by real, independent event timing — new pool.*/airdrop.failed events as they occur), and — because backoffMs is a pure function of attempt count only, not of the delivery's specific identity or any random offset — every one of those failed deliveries computes the same backoff delay for the same attempt number, converging their nextRetryAt values into tight clusters. deliveryRepo.scheduleRetry stores these in a single shared Redis sorted set (webhooks:retries), and webhookRetryWorker.tick() drains up to retryBatchSize (default 25) due items every retryPollMs (default 5000ms) — so when a cluster of retries becomes due, the worker processes them in a burst of up to 25 near-simultaneous outbound POST requests to what may be the same recovering subscriber endpoint, right as (or before) it's likely still coming back up from whatever caused the original outage. This is the exact self-inflicted thundering-herd pattern jitter exists to prevent, and it applies with equal force to every subscriber in the system whenever their endpoint has any correlated multi-delivery outage — not a rare edge case, but the expected behavior for the single most common failure mode a webhook consumer will ever experience.
Requirements
Add jitter to backoffMs() — a randomized offset (commonly "full jitter": Math.random() * computedDelay, or "decorrelated jitter") so that deliveries failing at the same attempt count and around the same wall-clock moment do not all compute identical nextRetryAt values.
Ensure the jitter is deterministic-enough for testability (inject a random source, as this codebase already does elsewhere — e.g. CircuitBreaker's options.now/options.logger injection pattern in utils/circuitBreaker.js — rather than calling Math.random() directly and making the function hard to test precisely).
Consider whether webhookRetryWorker's retryBatchSize/retryPollMs need retuning once jitter spreads retries out more evenly, though that's a secondary concern to adding the jitter itself.
Acceptance Criteria
backoffMs(attemptsCompleted) no longer returns an identical value for every call with the same attemptsCompleted — repeated calls with the same input produce a distribution of delays within a documented, bounded range around the previous deterministic value.
A test simulates many deliveries failing at the same attempt count within the same tick and asserts their computed nextRetryAt values are spread across a range, not identical.
The jitter source is injectable/mockable for deterministic test assertions (min/max bounds respected, not just "looks random").
shouldRetry/maxAttempts semantics are unaffected — only the timing of retries changes, not whether/how-many retries occur.
Additional Notes
More precise references
src/services/webhookDispatcher.js:13-17 (backoffMs): confirmed the exact formula base * factor ** (attemptsCompleted - 1), confirmed no randomization term anywhere in the function.
src/services/webhookDispatcher.js:108-118 (attempt, retry-scheduling branch): confirmed const delayMs = backoffMs(attempts); const nextRetryAt = new Date(Date.now() + delayMs).toISOString(); await deliveryRepo.scheduleRetry(delivery.id, Date.now() + delayMs); — confirmed Date.now() + delayMs is the only source of variation between two deliveries at the same attempt count, and that variation is purely a function of when each delivery happened to fail, not of anything designed to spread retries apart.
src/repositories/deliveryRepository.js:114-117 (scheduleRetry): confirmed this writes to a single shared Redis sorted set webhooks:retries, keyed by nextRetryAtMs as the score — confirming that clustered nextRetryAt values across many different deliveries/webhooks genuinely collide into the same narrow score range in one shared structure, not just conceptually but in the literal data structure a single worker drains from.
src/config.jswebhooks.retryBaseMs/retryFactor: confirmed defaults 30000/2, giving deterministic delays of exactly 30s, 60s, 120s for attempts 1, 2, 3 (with maxAttempts default 3, meaning realistically most deliveries only ever see the 30s and 60s tiers before exhausting retries).
Additional edge cases
Jitter needs to be careful not to ever produce a negative or zero delay, and should probably stay bounded close to (not wildly exceeding) the deterministic base delay, so retry timing remains roughly predictable for operators reasoning about worst-case delivery latency — "full jitter" (Math.random() * delay) versus "jitter added on top" (delay + Math.random() * someSpreadMs) have different tradeoffs here and are worth deciding deliberately rather than picking arbitrarily.
This compounds with the already-open No ordering guarantee for webhook deliveries to the same endpoint across independent dispatch() calls #78 ("No ordering guarantee for webhook deliveries to the same endpoint across independent dispatch() calls") in that both concern the timing behavior of the retry/delivery system as experienced by a subscriber — worth a shared review pass, though the fixes are largely independent.
Test/reproduction plan
constdelays=newSet();for(leti=0;i<100;i++){delays.add(webhookDispatcher.backoffMs(1));// simulate 100 deliveries all failing on attempt 1 "at once"}expect(delays.size).toBeGreaterThan(1);// currently fails: delays.size === 1, every call returns exactly 30000
Overview
webhookDispatcher.js's retry backoff is purely deterministic — the same attempt count always produces the exact same delay — with no randomization (jitter) at all. Combined withwebhookRetryWorker's fixed 5-second poll interval and 25-item batch size, this creates a textbook synchronized-retry thundering-herd risk against any subscriber whose endpoint experiences an outage affecting multiple in-flight deliveries at once.With the documented defaults (
WEBHOOK_RETRY_BASE_MS=30000,WEBHOOK_RETRY_FACTOR=2), every delivery that fails on its first attempt computesnextRetryAt = now + 30000ms— exactly. Every delivery that fails on its second attempt (regardless of which delivery, which webhook, or which subscriber) computesnextRetryAt = now + 60000ms— exactly. There is no+ Math.random() * jitterMsterm anywhere in this function or anywhere else in the retry path (attempt(),deliveryRepo.scheduleRetry).Concretely: if a subscriber's endpoint goes down for, say, 90 seconds during a deploy or a brief outage, every delivery attempted against it during that window fails at roughly the same wall-clock moments (driven by real, independent event timing — new
pool.*/airdrop.failedevents as they occur), and — becausebackoffMsis a pure function of attempt count only, not of the delivery's specific identity or any random offset — every one of those failed deliveries computes the same backoff delay for the same attempt number, converging theirnextRetryAtvalues into tight clusters.deliveryRepo.scheduleRetrystores these in a single shared Redis sorted set (webhooks:retries), andwebhookRetryWorker.tick()drains up toretryBatchSize(default 25) due items everyretryPollMs(default 5000ms) — so when a cluster of retries becomes due, the worker processes them in a burst of up to 25 near-simultaneous outbound POST requests to what may be the same recovering subscriber endpoint, right as (or before) it's likely still coming back up from whatever caused the original outage. This is the exact self-inflicted thundering-herd pattern jitter exists to prevent, and it applies with equal force to every subscriber in the system whenever their endpoint has any correlated multi-delivery outage — not a rare edge case, but the expected behavior for the single most common failure mode a webhook consumer will ever experience.Requirements
backoffMs()— a randomized offset (commonly "full jitter":Math.random() * computedDelay, or "decorrelated jitter") so that deliveries failing at the same attempt count and around the same wall-clock moment do not all compute identicalnextRetryAtvalues.CircuitBreaker'soptions.now/options.loggerinjection pattern inutils/circuitBreaker.js— rather than callingMath.random()directly and making the function hard to test precisely).webhookRetryWorker'sretryBatchSize/retryPollMsneed retuning once jitter spreads retries out more evenly, though that's a secondary concern to adding the jitter itself.Acceptance Criteria
backoffMs(attemptsCompleted)no longer returns an identical value for every call with the sameattemptsCompleted— repeated calls with the same input produce a distribution of delays within a documented, bounded range around the previous deterministic value.nextRetryAtvalues are spread across a range, not identical.shouldRetry/maxAttemptssemantics are unaffected — only the timing of retries changes, not whether/how-many retries occur.Additional Notes
More precise references
src/services/webhookDispatcher.js:13-17(backoffMs): confirmed the exact formulabase * factor ** (attemptsCompleted - 1), confirmed no randomization term anywhere in the function.src/services/webhookDispatcher.js:108-118(attempt, retry-scheduling branch): confirmedconst delayMs = backoffMs(attempts); const nextRetryAt = new Date(Date.now() + delayMs).toISOString(); await deliveryRepo.scheduleRetry(delivery.id, Date.now() + delayMs);— confirmedDate.now() + delayMsis the only source of variation between two deliveries at the same attempt count, and that variation is purely a function of when each delivery happened to fail, not of anything designed to spread retries apart.src/repositories/deliveryRepository.js:114-117(scheduleRetry): confirmed this writes to a single shared Redis sorted setwebhooks:retries, keyed bynextRetryAtMsas the score — confirming that clusterednextRetryAtvalues across many different deliveries/webhooks genuinely collide into the same narrow score range in one shared structure, not just conceptually but in the literal data structure a single worker drains from.src/jobs/webhookRetryWorker.js:21(tick): confirmeddeliveryRepo.popDueRetries(Date.now(), config.webhooks.retryBatchSize)— defaultretryBatchSize = 25persrc/config.js(WEBHOOK_RETRY_BATCHdefault25), confirmedretryPollMsdefault5000.src/config.jswebhooks.retryBaseMs/retryFactor: confirmed defaults30000/2, giving deterministic delays of exactly 30s, 60s, 120s for attempts 1, 2, 3 (withmaxAttemptsdefault3, meaning realistically most deliveries only ever see the 30s and 60s tiers before exhausting retries).Additional edge cases
Math.random() * delay) versus "jitter added on top" (delay + Math.random() * someSpreadMs) have different tradeoffs here and are worth deciding deliberately rather than picking arbitrarily.Test/reproduction plan
Cross-references