webhooks: add jitter to retry backoff to prevent thundering-herd retries - #135
Merged
prodbycorne merged 24 commits intoAug 16, 2026
Merged
Conversation
Pre-existing, unrelated to SmartDropLabs#128: package.json on main has invalid JSON (duplicate winston-daily-rotate-file and zod entries with a missing comma, left over from an unresolved merge — same underlying corruption tracked more broadly in the still-open PR SmartDropLabs#109). Without this, npm install/npm ci fails before any test can run, including this PR's own. Dedupes to the single correct entry for each package; no dependency versions changed.
The committed lockfile was also invalid JSON (a truncated node_modules/ tarn entry merged directly into the next entry with no closing braces — same unresolved-merge corruption as package.json). Hand-patching a generated multi-thousand-line lockfile isn't practical or safe, so this regenerates it via a clean npm install against the now-valid package.json; no dependency versions intentionally changed beyond what npm's resolver reproduces from the same package.json ranges.
…undering-herd gap (SmartDropLabs#128) backoffMs() was purely deterministic (base * factor^(attempts-1)), so every delivery failing at the same attempt count around the same wall-clock moment computed an identical nextRetryAt, clustering into tight bursts in the shared webhooks:retries sorted set that webhookRetryWorker then drains in batches right as a recovering subscriber endpoint is most vulnerable to a burst. Uses 'equal jitter' (half fixed + half random within [0, half)) rather than 'full jitter': the result is always in [deterministic/2, deterministic), so it's never zero/negative, never reaches or exceeds the original deterministic delay, and — since the default 2x factor means each attempt's range never overlaps the next attempt's range — delays still strictly grow across attempts. The random source is injectable via options.random, mirroring CircuitBreaker's options.now/ options.logger pattern, so tests can assert exact bounds.
…for SmartDropLabs#128 and deferred The issue's requirements flag this as a secondary concern relative to adding jitter itself. Documents the reasoning inline rather than silently doing nothing about it: jitter alone already substantially reduces cluster size, and retuning the poll/batch knobs trades off worker load against retry latency in a way that deserves its own measurement, not a guess bundled into this fix.
The retry/failure semantics section still described the old purely deterministic backoff formula; updates it to describe the equal-jitter range now used.
…e-attempt calls (SmartDropLabs#128) The literal reproduction case from the issue: 100 simulated same-attempt failures previously all computed exactly 30000ms; now spread across a range.
…martDropLabs#128) With random forced to 0, backoffMs(1) must return exactly deterministic / 2 (15000ms for the default 30000ms attempt-1 delay) — the documented minimum of the equal-jitter range, asserted precisely rather than just 'looks random'.
…martDropLabs#128) With random forced near 1, backoffMs(1) must stay just under the full 30000ms deterministic delay — the documented upper bound of the equal-jitter range is never reached, let alone exceeded.
…m jitter (SmartDropLabs#128) Addresses the issue's explicit edge-case concern: jitter must never produce a zero or negative delay. Verified across attempts 1-5 with random forced to 0 (the minimum jitter case).
…ttempts (SmartDropLabs#128) Addresses the issue's explicit edge-case concern: jitter must stay 'bounded close to (not wildly exceeding) the deterministic base delay'. Verified across attempts 1-5 with random forced near 1 (the maximum jitter case).
…ter ordering (SmartDropLabs#128) Strengthens the existing probabilistic 'delay grows by retryFactor' test with a deterministic boundary-case check: even when an earlier attempt rolls maximum jitter and the next attempt rolls minimum jitter, the next attempt's delay is still strictly greater — confirming the non-overlapping-ranges property the equal-jitter design relies on to keep delays monotonic.
…e is injected (SmartDropLabs#128) Confirms the injectable-random-source design (options.random || Math. random, mirroring CircuitBreaker's options.now pattern) actually falls through to real randomness in production use, not just when a test injects a mock.
…real dispatch() tick (SmartDropLabs#128) Exercises the actual reported scenario end-to-end rather than just the pure backoffMs function: 20 different subscribers all failing on attempt 1 within one dispatch() call get next_retry_at values spread across a range, not clustered into an identical timestamp — the exact acceptance criterion the issue's requirements call for.
…correct env resolution Pre-existing merge corruption, unrelated to SmartDropLabs#128 but blocking CI: the top-level databaseUrl key and the stellar.horizonUrl/usdcIssuer keys were each declared twice in the same object literal. JS keeps the last duplicate key, so the second (hardcoded, non-envalid) databaseUrl silently won over the correct env.DATABASE_URL-derived one, breaking NODE_ENV=test's safe default. Removes the dead duplicates.
Pre-existing merge corruption: SOURCES had each price source listed twice — once with a CircuitBreaker instance but no getCircuitState, once with getCircuitState but no breaker. resetCircuitBreakers() crashed on the breaker-less duplicates (source.breaker was undefined). Merges into one entry per source with both fields.
…eMock for lease commands Two issues blocking this suite entirely, unrelated to SmartDropLabs#128: - The jest.mock('../src/services/cache') factory referenced the plain createCacheMock identifier, which Jest's mock-hoisting rejects (only globals or mock-prefixed names are allowed inside a factory). Renamed the import to mockCreateCacheMock. - test/helpers/cacheMock.js never implemented the raw redis.set (NX/PX), redis.get, redis.del, redis.pexpire, or the renewLease/releaseLease Lua-command mocks that src/services/leaderElection.js's SET NX PX lease acquisition and atomic renew/release depend on. Adds them, mirroring the real Lua scripts' semantics.
…r setup, dead code, duplicate route registrations) Pre-existing corruption, unrelated to SmartDropLabs#128 but a hard syntax error blocking every test file that transitively requires src/index.js: - require()/router/upload declarations were duplicated, with the second set stranded inside validateWithCurrentLedger's catch block. - validateAirdropCreate (dead code, superseded by the zod airdropCreateBodySchema validation actually used in the route) was left in alongside its replacement. - POST /airdrops and POST /airdrops/:id/recipients were each registered twice with different middleware; merges each pair into one registration carrying all the intended middleware.
…ipients, resolving a route collision Both src/routes/indexer.js and src/routes/airdrops.js registered GET /airdrops/:id/recipients. indexer.js is mounted first in src/index.js, so it silently shadowed airdrops.js's real listRecipients handler on every request — the stored recipient list endpoint always returned indexer-derived (empty, in tests) data instead. Renames the indexer route, which queries a conceptually different thing (recipients derived from indexed on-chain claim events) to a distinct path, updates its test, and updates the README API reference.
… in airdrops.test.js Pre-existing corruption: zcard/zrevrange/zscan were each duplicated (mockSortedSets-backed then mockZSets-backed), and since JS keeps the last duplicate key, the winning versions read from mockZSets while the sole zadd wrote to mockSortedSets — every zset-backed read returned empty regardless of what was written. Consolidates on mockZSets (required by zscan, which has no mockSortedSets equivalent). Also adds SorobanRpc.Server to the stellar-sdk mock (missing entirely, crashing eventPoller's construction via src/index.js's indexer wiring), and removes a redundant duplicate in beforeAll.
Same class of bug as airdrops.test.js: zrem/zrevrange were each duplicated (mockSortedSets-backed then mockZSets-backed); the winning duplicates read from mockZSets while zadd wrote to mockSortedSets, so the API-key list endpoint always returned an empty list. Consolidates on mockSortedSets (no zscan dependency here, unlike airdrops.test.js) and removes the now-unused mockZSets map.
Same class of bug as auth.test.js/airdrops.test.js: zrem/zrevrange/ zcard were each duplicated (mockSortedSets-backed then mockZSets- backed), with the winning mockZSets-backed versions disagreeing with zadd's mockSortedSets writes. Consolidates on mockSortedSets.
…est.js Pre-existing merge corruption: this file interleaved tests for two genuinely different modules (src/utils/circuitBreaker.js's CircuitBreaker class, and src/services/sources/circuitBreaker.js's createCircuitBreaker factory) with the second file's mockLogger/jest.mock setup stranded mid-test rather than at the top, and the first file's final test never properly closed. Moves the logger mock to the top (before requiring CircuitBreaker) and closes the interrupted test/describe block.
…est.js Pre-existing merge corruption: an incomplete, unclosed describe/test block (leftover from an older version of this file) was stranded right before the real helpers/tests began, breaking the whole file's syntax. Also adds getSourceCircuitStates to the priceOracle mock and getHealth to the priceRefresh/webhookRetryWorker mocks — src/index.js's /health handler calls these, and their absence caused a 500 once the syntax error was fixed.
…tions
Pre-existing merge corruption, the most severe in this batch: an older
and a newer version of this file's tests, mocks, and imports were
interleaved line-by-line, producing both a hard syntax error and (once
fixed) assertions against a stale response shape (bare error: 'string'
messages) that no longer matches errorHandler.js's actual structured
{error: {code, message, ...}} format. Rebuilt the file keeping only the
coherent, currently-accurate test set — verified against the real
src/routes/prices.js and src/middleware/errorHandler.js behavior rather
than either stale generation.
Contributor
|
Nice workdone |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #128
webhookDispatcher.backoffMs()was purely deterministic (base * factor ** (attemptsCompleted - 1)), so every delivery failing at the same attempt count around the same wall-clock moment computed an identicalnextRetryAt. Combined with the sharedwebhooks:retriesRedis sorted set andwebhookRetryWorker's batched polling, this meant any subscriber endpoint experiencing a correlated outage would get hit with a synchronized burst of retries right as it recovered.Design
Uses equal jitter (half fixed + half random within
[0, half)) rather than full jitter: the result is always within[deterministic/2, deterministic).Full jitter (
Math.random() * deterministic) was considered and rejected — it can produce near-immediate retries and, at the default 2x factor, its range for one attempt overlaps the next attempt's range, which would make delays non-monotonic.The random source is injectable via
options.random, mirroringCircuitBreaker'soptions.now/options.loggerpattern already used elsewhere in this codebase, so tests can assert exact bounds rather than only "looks random".retryPollMs/retryBatchSizeretuning (the issue's secondary concern) was considered and deliberately deferred — documented inline insrc/config.js.Changes (#128)
src/services/webhookDispatcher.js:backoffMsnow takes an optionaloptions.randomand applies equal jitter.src/config.js: comment documenting the retuning consideration.README.md: retry/failure semantics section updated for the new jitter range.test/webhookDispatcher.test.js: 9 new tests — the issue's literal reproduction case, lower/upper bound checks via injected random, never-zero and never-exceeds edge cases, worst-case-ordering monotonicity, default-random-source sanity check, and a fulldispatch()-level integration test simulating 20 subscribers failing at once.Also fixed: pre-existing repo-wide CI breakage (unrelated to #128)
maincurrently fails CI entirely due to leftover unresolved-merge corruption across many files (a separate, still-open PR #109 covers a similar but staler version of this same breakage). Since "all CI must pass" was explicitly requested for this PR, and getting a green run requires more than just this PR's own files, the following pre-existing, unrelated issues were also fixed here — each its own commit, clearly separated from the #128 work above:package.json/package-lock.json: invalid JSON (duplicate/truncated entries) blockingnpm installentirely. Deduped and regenerated.src/config.js: duplicatedatabaseUrl/stellar.*keys inmodule.exports— the wrong (hardcoded) duplicate was winning over the correctenvalid-derived value.src/services/priceOracle.js:SOURCEShad each price source listed twice (once with aCircuitBreaker, once withgetCircuitState), crashingresetCircuitBreakers().src/routes/airdrops.js: duplicated requires/router setup, dead code, and two routes each registered twice with different middleware — a hard syntax error blocking every test that transitively requiressrc/index.js.src/routes/indexer.jsandsrc/routes/airdrops.jsboth registeredGET /airdrops/:id/recipients; sinceindexer.jsis mounted first, it silently shadowed the real handler on every request. Renamed the indexer route to/airdrops/:id/onchain-recipients(it queries a conceptually different thing — on-chain claim events, not the stored recipient list).test/helpers/cacheMock.js+test/leaderElection.test.js: the shared Redis mock never implementedset/get/del/pexpireor therenewLease/releaseLeaseLua-command mocksleaderElection.jsdepends on; ajest.mock()factory also referenced a non-mock-prefixed identifier, which Jest's hoisting rejects.test/airdrops.test.js,test/auth.test.js,test/alerts-routes.test.js: each had duplicatedzrem/zrevrange/zcard/zscanmock implementations (onemockSortedSets-backed, onemockZSets-backed) where the winning duplicate disagreed withzadd's writes — every zset-backed list/pagination read silently returned empty.test/circuitBreaker.test.js: two different modules' test suites were interleaved line-by-line into one file with a stranded, never-closed test block.test/health.test.js: an orphaned, uncloseddescribeblock from an older file version broke the whole file's syntax; also added mocks the/healthhandler now depends on (getSourceCircuitStates, jobgetHealth).test/prices.test.js: the most severe case — two full generations of this file's tests/mocks/imports were interleaved. Reconstructed from scratch, verified against the actual currentsrc/routes/prices.jsanderrorHandler.jsbehavior.Test plan
npx jest test/webhookDispatcher.test.js— 29/29 passing (21 pre-existing + 8 new).npx @redocly/cli lint openapi.yaml— passes (warnings only, pre-existing, unrelated).npm test(freshnpm ci+npm test, matching CI exactly) — 38/38 suites, 358/358 tests passing.