feat: Migrate public mail API to BullMQ for asynchronous delivery - #157
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughHidden review stack artifactWalkthroughAdds a BullMQ-backed public email queue and worker (BYOK-capable), exports them from common, enqueues public emails from the mail controller, starts the worker at app boot, and updates tests to assert enqueue behavior and quota rollback on terminal failures. ChangesPublic Email Queue Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/public-api/src/controllers/mail.controller.js (1)
282-295: ⚡ Quick winMove key resolution fully to worker; avoid decrypting BYOK in controller path.
Now that sending is async, this pre-enqueue decrypt/key check is redundant and keeps secret handling on the API request path. Let the worker resolve credentials and keep the controller focused on validation + enqueue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/public-api/src/controllers/mail.controller.js` around lines 282 - 295, The controller currently decrypts and validates a BYOK and selects a clientKey (symbols: encryptedByokKey, decryptedByokKey, usingByok, clientKey, decrypt) which moves secret handling into the request path; remove that logic so the controller no longer calls decrypt or inspects/derives clientKey or returns a 500 when no env key is present. Instead, keep validation and enqueue the job payload including project.resendApiKey (pass through the encrypted object or null) and let the worker resolve/decrypt credentials and pick fallback env keys. Ensure no decrypt(...) call or clientKey check remains in the controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/public-api/src/__tests__/mail.controller.test.js`:
- Around line 223-227: The test currently invokes failedHandler only if it
exists, allowing a false pass when the handler was never registered; add an
explicit assertion that failedHandler is defined before calling it (e.g.,
expect(failedHandler).toBeDefined()) so the test fails if the handler wasn't
wired, then proceed to call failedHandler(mockJob, new Error("Temporary
failure")) and assert mockRedis.decr was not called; reference the failedHandler
symbol and the mockRedis.decr expectation to locate where to insert the
assertion.
In `@packages/common/src/queues/publicEmailQueue.js`:
- Around line 53-59: The logs in the public email worker expose PII by printing
finalPayload.to; update the logging around the send call (the console.log before
calling resend.emails.send and the console.error in the catch path) to avoid raw
recipient addresses—either omit the address or log a masked version (e.g.,
replace the local part with asterisks or show only domain/first char) and
include contextual info like a message id or status; ensure you still throw the
Error as before but do not include the unmasked finalPayload.to in any log
output.
- Around line 80-84: The quota refund can drive Redis keys negative because
connection.decr on a missing key yields -1; change the logic around
job.data.consumedQuotaKey to only decrement when the stored value is > 0: either
(a) read the current value with connection.get(job.data.consumedQuotaKey),
parseInt it, and call connection.decr(...) only if the parsed value > 0 (and
optionally set to "0" if it's negative/NaN), or (b) replace the two-step
approach with an atomic Lua eval that decrements the key only if its current
value > 0 (use connection.eval with job.data.consumedQuotaKey to check and
decrement atomically); update the block containing job.opts?.attempts,
job.attemptsMade and connection.decr to use one of these guarded approaches.
---
Nitpick comments:
In `@apps/public-api/src/controllers/mail.controller.js`:
- Around line 282-295: The controller currently decrypts and validates a BYOK
and selects a clientKey (symbols: encryptedByokKey, decryptedByokKey, usingByok,
clientKey, decrypt) which moves secret handling into the request path; remove
that logic so the controller no longer calls decrypt or inspects/derives
clientKey or returns a 500 when no env key is present. Instead, keep validation
and enqueue the job payload including project.resendApiKey (pass through the
encrypted object or null) and let the worker resolve/decrypt credentials and
pick fallback env keys. Ensure no decrypt(...) call or clientKey check remains
in the controller.
🪄 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: 059013ac-3ad9-4f2c-ad98-229a3b18ba88
📒 Files selected for processing (5)
apps/public-api/src/__tests__/mail.controller.test.jsapps/public-api/src/app.jsapps/public-api/src/controllers/mail.controller.jspackages/common/src/index.jspackages/common/src/queues/publicEmailQueue.js
| if (failedHandler) { | ||
| await failedHandler(mockJob, new Error("Temporary failure")); | ||
| } | ||
|
|
||
| expect(mockRedis.decr).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Non-terminal failure test can false-pass if handler is never wired.
Because invocation is guarded by if (failedHandler), this test passes when failedHandler is undefined. Assert handler registration first to make the test meaningful.
Suggested change
- if (failedHandler) {
- await failedHandler(mockJob, new Error("Temporary failure"));
- }
+ expect(typeof failedHandler).toBe('function');
+ await failedHandler(mockJob, new Error("Temporary failure"));📝 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.
| if (failedHandler) { | |
| await failedHandler(mockJob, new Error("Temporary failure")); | |
| } | |
| expect(mockRedis.decr).not.toHaveBeenCalled(); | |
| expect(typeof failedHandler).toBe('function'); | |
| await failedHandler(mockJob, new Error("Temporary failure")); | |
| expect(mockRedis.decr).not.toHaveBeenCalled(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/public-api/src/__tests__/mail.controller.test.js` around lines 223 -
227, The test currently invokes failedHandler only if it exists, allowing a
false pass when the handler was never registered; add an explicit assertion that
failedHandler is defined before calling it (e.g.,
expect(failedHandler).toBeDefined()) so the test fails if the handler wasn't
wired, then proceed to call failedHandler(mockJob, new Error("Temporary
failure")) and assert mockRedis.decr was not called; reference the failedHandler
symbol and the mockRedis.decr expectation to locate where to insert the
assertion.
There was a problem hiding this comment.
Pull request overview
This PR migrates the public /api/mail/send endpoint in apps/public-api from synchronous Resend calls to an asynchronous BullMQ-backed delivery flow, adding a dedicated publicEmailQueue worker and adapting controller/tests accordingly.
Changes:
- Added
publicEmailQueue+initPublicEmailWorker()(BullMQ worker with rate limiting and BYOK key resolution inside the worker). - Updated
/api/mail/sendto enqueue jobs and return a job ID immediately instead of blocking on the provider call. - Updated public-api mail controller tests to mock BullMQ queueing and to verify quota refunds on terminal worker failures.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/src/queues/publicEmailQueue.js | Introduces BullMQ queue/worker for public email delivery, including BYOK resolution and quota refund on terminal failure. |
| packages/common/src/index.js | Exports the new queue and worker initializer from the common package. |
| apps/public-api/src/controllers/mail.controller.js | Switches /api/mail/send to enqueue email jobs and return the BullMQ job ID. |
| apps/public-api/src/app.js | Initializes the new public email worker during public-api startup (non-test env). |
| apps/public-api/src/tests/mail.controller.test.js | Refactors tests to assert queueing behavior and to cover worker quota-refund logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| console.log(`[Queue] Processing public email to: ${finalPayload.to}`); | ||
|
|
||
| const { data, error } = await resend.emails.send(finalPayload); |
| if (job && job.data && job.data.consumedQuotaKey) { | ||
| const maxAttempts = job.opts?.attempts || 1; | ||
| if (job.attemptsMade >= maxAttempts) { | ||
| await connection.decr(job.data.consumedQuotaKey).catch(() => {}); | ||
| } |
| const job = await publicEmailQueue.add("send-public-email", { | ||
| projectId, | ||
| payload, | ||
| usingByok, | ||
| consumedQuotaKey | ||
| }); |
|
@copilot apply changes based on the comments in this thread |
…queue Agent-Logs-Url: https://github.com/geturbackend/urBackend/sessions/091132d9-772a-45c6-9e9f-74b313afd713 Co-authored-by: yash-pouranik <172860064+yash-pouranik@users.noreply.github.com>
…r short local parts Agent-Logs-Url: https://github.com/geturbackend/urBackend/sessions/091132d9-772a-45c6-9e9f-74b313afd713 Co-authored-by: yash-pouranik <172860064+yash-pouranik@users.noreply.github.com>
Render
Destroyed
Applied all three review suggestions in commits
All 124 tests pass. |
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me fetch the review thread to understand the comments. 🏁 Script executed: Length of output: 16649 Let me check the current state of the relevant files. 🏁 Script executed: Length of output: 7482
Here's my plan to fix the two remaining items:
🏁 Script executed: Length of output: 1289
Fix 1 —
|
This PR migrates the public
/api/mail/sendendpoint to a Redis-backed BullMQ queue (publicEmailQueue). Previously, the API executed synchronous HTTP calls to Resend, which blocked the thread and caused users to hit429 Too Many Requestswhen sending bulk emails concurrently (e.g., KBC Project bulk emails).By offloading the execution to a background worker, the API now returns a fast
200 OKwith a queue Job ID, while emails are processed steadily and securely in the background.Key Changes:
publicEmailQueueto decouple the HTTP response from the email delivery.10 emails / secondlimiter on the worker to perfectly respect Resend's free-tier rate limits.worker.on('failed')). If an email permanently fails to send, the consumed monthly mail quota is accurately refunded to the project via itsconsumedQuotaKey.attemptsMade >= maxAttempts, preventing over-refunding drift on temporary network retries.Testing & Verification (P3):
apps/public-api/src/__tests__/mail.controller.test.jstests to mockBullMQinstead ofResend.failedevent listener to guarantee refunds on terminal failures.npx jest --testPathPatterns=src/ --runInBand).Type of Change:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests