feat: Implement Webhook Delivery Distributed Rate-Limit Store#531
Merged
OlufunbiIK merged 2 commits intoApr 28, 2026
Conversation
Fixes OlufunbiIK#474. - Created WebhookRateLimitStore to use Redis as a shared backing store for horizontal scaling. - Updated WebhookDeliveryService to use the new rate limit store instead of an in-memory map. - Updated getRedisUrl in redis.config.ts to support instantiating ioredis. - Provided unit tests for WebhookRateLimitStore (redis + fallback modes). - Updated WebhooksModule to inject the new store.
|
@Xaxxoo is attempting to deploy a commit to the olufunbiik's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughReplaces process-local webhook rate limiting with a new distributed Changes
Sequence Diagram(s)sequenceDiagram
participant Delivery as WebhookDeliveryService
participant Store as WebhookRateLimitStore
participant Redis as Redis
participant Map as InMemoryMap
Delivery->>Store: checkRateLimit(webhookId, maxRequests, windowMs)
alt Redis available
Store->>Redis: multi() -> incr(key) / pttl(key) -> exec()
alt newly created or no ttl
Store->>Redis: pexpire(key, windowMs)
end
Redis-->>Store: [count, ttl]
else Redis unavailable or error
Store->>Map: read/update counter and resetAt
Map-->>Store: count
end
alt count <= maxRequests
Store-->>Delivery: true (allow)
else
Store-->>Delivery: false (deny)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/src/webhooks/webhook-rate-limit.store.spec.ts (1)
55-69: Use fake timers for the reset-window test to reduce flakiness.The real
setTimeout(20)window test can be timing-sensitive in CI. Prefer Jest fake timers and deterministic time advancement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/webhooks/webhook-rate-limit.store.spec.ts` around lines 55 - 69, Replace the real timer in the "should reset fallback limit after window" test with Jest fake timers: call jest.useFakeTimers() before forcing fallback ((store as any).available = false) and invoking store.checkRateLimit, then after the two checks advance time deterministically with jest.advanceTimersByTime(20) (or slightly more) and run pending timers/promises as needed (jest.runOnlyPendingTimers / await Promise.resolve()) before calling store.checkRateLimit again, and finally restore timers with jest.useRealTimers(); this will make the timing-sensitive behavior around store.checkRateLimit and the fallback reset deterministic and non-flaky.backend/src/webhooks/webhook-rate-limit.store.ts (1)
13-13: Fallback map can grow unbounded during Redis outages.
fallbackMapnever evicts cold/expired webhook keys. In a sustained outage with high webhook cardinality, this can cause memory pressure.Suggested lightweight pruning approach
private checkFallback(webhookId: string, maxRequests: number, windowMs: number): boolean { const now = Date.now(); + if (this.fallbackMap.size > 10_000) { + for (const [id, entry] of this.fallbackMap) { + if (entry.resetAt <= now) this.fallbackMap.delete(id); + } + } const limit = this.fallbackMap.get(webhookId); - if (!limit || now > limit.resetAt) { + if (!limit || now >= limit.resetAt) { this.fallbackMap.set(webhookId, { count: 1, resetAt: now + windowMs, }); return true; }Also applies to: 94-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/webhooks/webhook-rate-limit.store.ts` at line 13, The fallbackMap (Map<string, { count: number; resetAt: number }>) can grow unbounded; modify code that reads/writes this map (the methods that increment/check rate limits) to perform lightweight pruning: on each access, delete any entries whose resetAt <= Date.now(), and additionally enforce a soft max capacity (e.g., MAX_FALLBACK_MAP_SIZE) by evicting the oldest or nearest-expiry entries when exceeded; update any functions using fallbackMap to rely on this TTL-based cleanup and capacity check so cold/expired webhook keys are removed during Redis outages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/webhooks/webhook-delivery.service.spec.ts`:
- Around line 34-36: Add a test that covers the rate-limit rejection path by
making the test-specific mockRateLimitStore.checkRateLimit resolve to false,
then call the same entry method under test (the service's delivery invocation
used elsewhere in this spec, e.g., webhookDeliveryService.deliver or
deliverWebhook) and assert that the delivery persistence method (e.g.,
webhookDeliveryRepository.create) was not called and the job queue add/enqueue
method (e.g., webhookDeliveryQueue.add or .enqueue) was not invoked; ensure you
restore/reset mocks between cases so the existing tests (with checkRateLimit
true) remain unchanged.
In `@backend/src/webhooks/webhook-rate-limit.store.spec.ts`:
- Around line 73-130: Add a new test that instantiates two WebhookRateLimitStore
instances (or two variables referencing the same store class) that share the
same mocked Redis client ((store as any).redis / mockRedisInstance) and
available flag, then call checkRateLimit on the same key from both instances to
assert cross-instance quota enforcement: e.g., have mockMulti.exec return
incremental counts across calls (first call returns count within limit, second
returns count > limit) and assert the first instance allows the request while
the second denies it; also verify shared behaviors such as a single pexpire call
for the first increment and that fallbackMap is only used when multi.exec
rejects for both instances.
In `@backend/src/webhooks/webhook-rate-limit.store.ts`:
- Around line 72-81: The current block silently returns false when `results` is
null (transaction abort) and ignores `pttlError`; instead, change the logic in
the handler that reads `results` so that if `results` is falsy you throw an
error (e.g., new Error("Redis transaction aborted")) rather than `return false`,
and also check `pttlError` alongside `incrError` (throw `pttlError` if present)
before using `countVal`/`pttlVal`; this ensures any transaction aborts or TTL
command failures bubble to the catch path and invoke the existing
`checkFallback()` fallback logic.
---
Nitpick comments:
In `@backend/src/webhooks/webhook-rate-limit.store.spec.ts`:
- Around line 55-69: Replace the real timer in the "should reset fallback limit
after window" test with Jest fake timers: call jest.useFakeTimers() before
forcing fallback ((store as any).available = false) and invoking
store.checkRateLimit, then after the two checks advance time deterministically
with jest.advanceTimersByTime(20) (or slightly more) and run pending
timers/promises as needed (jest.runOnlyPendingTimers / await Promise.resolve())
before calling store.checkRateLimit again, and finally restore timers with
jest.useRealTimers(); this will make the timing-sensitive behavior around
store.checkRateLimit and the fallback reset deterministic and non-flaky.
In `@backend/src/webhooks/webhook-rate-limit.store.ts`:
- Line 13: The fallbackMap (Map<string, { count: number; resetAt: number }>) can
grow unbounded; modify code that reads/writes this map (the methods that
increment/check rate limits) to perform lightweight pruning: on each access,
delete any entries whose resetAt <= Date.now(), and additionally enforce a soft
max capacity (e.g., MAX_FALLBACK_MAP_SIZE) by evicting the oldest or
nearest-expiry entries when exceeded; update any functions using fallbackMap to
rely on this TTL-based cleanup and capacity check so cold/expired webhook keys
are removed during Redis outages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1348025-589b-4a99-b862-3f569f1f6f78
📒 Files selected for processing (6)
backend/src/config/redis.config.tsbackend/src/webhooks/webhook-delivery.service.spec.tsbackend/src/webhooks/webhook-delivery.service.tsbackend/src/webhooks/webhook-rate-limit.store.spec.tsbackend/src/webhooks/webhook-rate-limit.store.tsbackend/src/webhooks/webhooks.module.ts
Comment on lines
+34
to
+36
| const mockRateLimitStore = { | ||
| checkRateLimit: jest.fn().mockResolvedValue(true), | ||
| }; |
Contributor
There was a problem hiding this comment.
Add a test for rate-limit rejection path.
With checkRateLimit always resolving true, the branch that skips delivery is never asserted. Add a case where it resolves false, then verify no delivery record is created and no queue job is enqueued.
Example test shape
+ it('should skip queueing when rate limit is exceeded', async () => {
+ mockRateLimitStore.checkRateLimit.mockResolvedValue(false);
+ const webhooks = [{
+ id: 'webhook-1',
+ userId: 'user-1',
+ url: 'https://example.com/webhook',
+ events: [WebhookEventType.SPLIT_CREATED],
+ secret: 'secret-1',
+ isActive: true,
+ }];
+ const queryBuilder = {
+ where: jest.fn().mockReturnThis(),
+ andWhere: jest.fn().mockReturnThis(),
+ getMany: jest.fn().mockResolvedValue(webhooks),
+ };
+ mockWebhookRepository.createQueryBuilder.mockReturnValue(queryBuilder);
+
+ await service.triggerEvent(WebhookEventType.SPLIT_CREATED, { test: 'data' }, 'user-1');
+
+ expect(mockRateLimitStore.checkRateLimit).toHaveBeenCalled();
+ expect(mockDeliveryRepository.create).not.toHaveBeenCalled();
+ expect(mockQueue.add).not.toHaveBeenCalled();
+ });Also applies to: 77-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/webhooks/webhook-delivery.service.spec.ts` around lines 34 - 36,
Add a test that covers the rate-limit rejection path by making the test-specific
mockRateLimitStore.checkRateLimit resolve to false, then call the same entry
method under test (the service's delivery invocation used elsewhere in this
spec, e.g., webhookDeliveryService.deliver or deliverWebhook) and assert that
the delivery persistence method (e.g., webhookDeliveryRepository.create) was not
called and the job queue add/enqueue method (e.g., webhookDeliveryQueue.add or
.enqueue) was not invoked; ensure you restore/reset mocks between cases so the
existing tests (with checkRateLimit true) remain unchanged.
Comment on lines
+73
to
+130
| describe('checkRateLimit (redis)', () => { | ||
| let mockMulti: any; | ||
| let mockRedisInstance: any; | ||
|
|
||
| beforeEach(() => { | ||
| mockMulti = { | ||
| incr: jest.fn().mockReturnThis(), | ||
| pttl: jest.fn().mockReturnThis(), | ||
| exec: jest.fn(), | ||
| }; | ||
|
|
||
| mockRedisInstance = { | ||
| multi: jest.fn().mockReturnValue(mockMulti), | ||
| pexpire: jest.fn().mockResolvedValue(1), | ||
| on: jest.fn(), | ||
| connect: jest.fn().mockResolvedValue(undefined), | ||
| quit: jest.fn().mockResolvedValue(undefined), | ||
| }; | ||
|
|
||
| (store as any).redis = mockRedisInstance; | ||
| (store as any).available = true; | ||
| }); | ||
|
|
||
| it('should allow request if under limit', async () => { | ||
| mockMulti.exec.mockResolvedValue([ | ||
| [null, 1], // incr result | ||
| [null, -1] // pttl result | ||
| ]); | ||
|
|
||
| const result = await store.checkRateLimit('wh-1', 5, 60000); | ||
|
|
||
| expect(result).toBe(true); | ||
| expect(mockRedisInstance.pexpire).toHaveBeenCalledWith('webhook_rate_limit:wh-1', 60000); | ||
| }); | ||
|
|
||
| it('should deny request if over limit', async () => { | ||
| mockMulti.exec.mockResolvedValue([ | ||
| [null, 6], // incr result | ||
| [null, 50000] // pttl result | ||
| ]); | ||
|
|
||
| const result = await store.checkRateLimit('wh-1', 5, 60000); | ||
|
|
||
| expect(result).toBe(false); | ||
| expect(mockRedisInstance.pexpire).not.toHaveBeenCalled(); // No pexpire call for >1 count | ||
| }); | ||
|
|
||
| it('should fallback if redis multi exec fails', async () => { | ||
| mockMulti.exec.mockRejectedValue(new Error('Redis is down')); | ||
|
|
||
| const result = await store.checkRateLimit('wh-1', 5, 60000); | ||
|
|
||
| // Should fallback and succeed | ||
| expect(result).toBe(true); | ||
| // Map should have it | ||
| expect((store as any).fallbackMap.has('wh-1')).toBe(true); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Cross-instance consistency is not validated yet.
Current Redis-path tests exercise one store instance only. The acceptance criteria calls for consistency across multiple app instances; add a test with two WebhookRateLimitStore instances sharing the same Redis key-space and assert shared quota enforcement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/webhooks/webhook-rate-limit.store.spec.ts` around lines 73 - 130,
Add a new test that instantiates two WebhookRateLimitStore instances (or two
variables referencing the same store class) that share the same mocked Redis
client ((store as any).redis / mockRedisInstance) and available flag, then call
checkRateLimit on the same key from both instances to assert cross-instance
quota enforcement: e.g., have mockMulti.exec return incremental counts across
calls (first call returns count within limit, second returns count > limit) and
assert the first instance allows the request while the second denies it; also
verify shared behaviors such as a single pexpire call for the first increment
and that fallbackMap is only used when multi.exec rejects for both instances.
Comment on lines
+72
to
+81
| if (!results) return false; | ||
|
|
||
| const [incrError, countVal] = results[0]; | ||
| const [pttlError, pttlVal] = results[1]; | ||
|
|
||
| if (incrError) throw incrError; | ||
|
|
||
| const count = Number(countVal); | ||
| const pttl = Number(pttlVal); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ioredis transactions, what does multi().exec() return on abort, and how are command-level errors represented in each result tuple?
💡 Result:
In ioredis, multi.exec returns an array of result tuples on success, where each tuple is [err, result] corresponding to each queued command. On abort due to WATCH condition failure (watched key modified), it returns null. On syntax errors in queued commands (e.g., wrong arguments), it rejects the promise with a ReplyError like "EXECABORT Transaction discarded because of previous errors.", including previousErrors detailing the issues; no commands execute. Command-level errors (runtime errors like WRONGTYPE during EXEC) are represented in the result array as [ReplyError, null] for the failed command's tuple, while successful commands return [null, result]. The transaction continues executing other commands atomically.
Citations:
- 1: https://ioredis.readthedocs.io/en/stable/README/
- 2: https://github.com/luin/ioredis/
- 3: https://oneuptime.com/blog/post/2026-03-31-redis-how-to-use-redis-transactions-in-nodejs/view
- 4: https://redis.io/docs/latest/develop/using-commands/transactions/
- 5: Unexpected results from Multi Exec block redis/ioredis#739
- 6: Better multi/exec results (suggestion)? redis/ioredis#301
- 7: https://tessl.io/registry/tessl/npm-ioredis/5.7.0/files/docs/pipelining.md
🏁 Script executed:
# Check if the file exists and read the context around lines 72-81
fd -t f webhook-rate-limit.store.tsRepository: OlufunbiIK/StellarSplit
Length of output: 116
🏁 Script executed:
# Read the file to see the actual implementation
if [ -f "backend/src/webhooks/webhook-rate-limit.store.ts" ]; then
wc -l backend/src/webhooks/webhook-rate-limit.store.ts
fiRepository: OlufunbiIK/StellarSplit
Length of output: 120
🏁 Script executed:
# Get the relevant section
if [ -f "backend/src/webhooks/webhook-rate-limit.store.ts" ]; then
sed -n '60,90p' backend/src/webhooks/webhook-rate-limit.store.ts
fiRepository: OlufunbiIK/StellarSplit
Length of output: 1009
🏁 Script executed:
# Look for how this function is called to understand error handling context
if [ -f "backend/src/webhooks/webhook-rate-limit.store.ts" ]; then
# Find the function containing these lines
grep -n "const \|function \|async " backend/src/webhooks/webhook-rate-limit.store.ts | head -20
fiRepository: OlufunbiIK/StellarSplit
Length of output: 692
Redirect transaction failures to fallback instead of silent deny, and check PTTL errors.
Line 72 returns false on null results (WATCH abort), bypassing the fallback handler. Additionally, pttlError at line 75 is extracted but never checked, silently ignoring TTL command failures. Both cases should throw to trigger the catch block's checkFallback() fallback logic.
Proposed fix
- if (!results) return false;
+ if (!results) {
+ throw new Error('Redis transaction aborted');
+ }
const [incrError, countVal] = results[0];
const [pttlError, pttlVal] = results[1];
- if (incrError) throw incrError;
+ if (incrError || pttlError) {
+ throw incrError ?? pttlError;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!results) return false; | |
| const [incrError, countVal] = results[0]; | |
| const [pttlError, pttlVal] = results[1]; | |
| if (incrError) throw incrError; | |
| const count = Number(countVal); | |
| const pttl = Number(pttlVal); | |
| if (!results) { | |
| throw new Error('Redis transaction aborted'); | |
| } | |
| const [incrError, countVal] = results[0]; | |
| const [pttlError, pttlVal] = results[1]; | |
| if (incrError || pttlError) { | |
| throw incrError ?? pttlError; | |
| } | |
| const count = Number(countVal); | |
| const pttl = Number(pttlVal); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/webhooks/webhook-rate-limit.store.ts` around lines 72 - 81, The
current block silently returns false when `results` is null (transaction abort)
and ignores `pttlError`; instead, change the logic in the handler that reads
`results` so that if `results` is falsy you throw an error (e.g., new
Error("Redis transaction aborted")) rather than `return false`, and also check
`pttlError` alongside `incrError` (throw `pttlError` if present) before using
`countVal`/`pttlVal`; this ensures any transaction aborts or TTL command
failures bubble to the catch path and invoke the existing `checkFallback()`
fallback logic.
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.
Fixes #474.
Changes:
WebhookRateLimitStoreusingioredis.WebhookDeliveryServiceto utilize this new distributed store layer.Summary by CodeRabbit