Repository navigation
Honour the SES send rate per second and retry throttled sends #454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,24 +23,97 @@ type QueueEmailJob = TeamJob<{ | |
| isBulk?: boolean; | ||
| }>; | ||
|
|
||
| function createQueueAndWorker(region: string, quota: number, suffix: string) { | ||
| /** | ||
| * A worker for one email queue, limited to `quota` jobs per second. | ||
| * | ||
| * SES enforces its maximum send rate per second. Concurrency alone does not | ||
| * honour it: a send round trip takes a fraction of a second, so `quota` | ||
| * workers in flight send several times `quota` mails a second and SES | ||
| * throttles the rest. The limiter caps the jobs a queue's workers start per | ||
| * second (BullMQ keeps it in Redis, shared by every worker of the queue); | ||
| * the concurrency keeps that many sends in flight. | ||
| */ | ||
| function createWorker(queueName: string, quota: number) { | ||
| const connection = getRedis(); | ||
|
|
||
| const queueName = `${region}-${suffix}`; | ||
|
|
||
| const queue = new Queue(queueName, { connection, prefix: BULL_PREFIX, skipVersionCheck: true }); | ||
|
|
||
| // TODO: Add team context to job data when queueing | ||
| const worker = new Worker(queueName, createWorkerHandler(executeEmail), { | ||
| return new Worker(queueName, createWorkerHandler(executeEmail), { | ||
| concurrency: quota, | ||
| limiter: { max: quota, duration: 1000 }, | ||
| connection, | ||
| prefix: BULL_PREFIX, | ||
| skipVersionCheck: true, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * The queue `${region}-${suffix}` and its first worker at `quota` per second. | ||
| */ | ||
| function createQueueAndWorker(region: string, quota: number, suffix: string) { | ||
| const connection = getRedis(); | ||
|
|
||
| const queueName = `${region}-${suffix}`; | ||
|
|
||
| const queue = new Queue(queueName, { connection, prefix: BULL_PREFIX, skipVersionCheck: true }); | ||
|
|
||
| const worker = createWorker(queueName, quota); | ||
|
|
||
| return { queue, worker }; | ||
| } | ||
|
|
||
| /** | ||
| * Puts a worker at the new `quota` on the region's queue in place of the | ||
| * current one. | ||
| * | ||
| * A worker's limiter is fixed when it is created, so a new send rate means a | ||
| * new worker on the same queue. The old one stops taking jobs and closes | ||
| * once its in-flight sends are done; the queue and the jobs waiting in it | ||
| * are untouched. | ||
| */ | ||
| function replaceWorker( | ||
| workers: Map<string, Worker>, | ||
| region: string, | ||
| queueName: string, | ||
| quota: number, | ||
| ) { | ||
| const previous = workers.get(region); | ||
| workers.set(region, createWorker(queueName, quota)); | ||
| if (previous) { | ||
| previous.close().catch((error) => { | ||
| logger.error( | ||
| { err: error, queue: queueName }, | ||
| `[EmailQueueService]: Failed to close the replaced worker`, | ||
| ); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Job options for every email: a send SES throttled ("Maximum sending rate | ||
| * exceeded") is retried with backoff instead of failing the mail; | ||
| * executeEmail decides per error whether to hand the job back to the queue | ||
| * (isThrottled) or to fail it. | ||
| */ | ||
| const EMAIL_JOB_OPTIONS = { | ||
| ...DEFAULT_QUEUE_OPTIONS, | ||
| attempts: 3, | ||
| backoff: { type: "exponential", delay: 1000 }, | ||
| }; | ||
|
|
||
| /** | ||
| * Whether a send failed because SES refused it for the account's send rate: | ||
| * the SES error names for it, or any error the AWS SDK marks as throttling. | ||
| */ | ||
| function isThrottled(error: any): boolean { | ||
| return ( | ||
| error?.name === "TooManyRequestsException" || | ||
| error?.name === "ThrottlingException" || | ||
| error?.name === "Throttling" || | ||
| error?.$retryable?.throttling === true || | ||
| error?.$metadata?.httpStatusCode === 429 | ||
| ); | ||
| } | ||
|
|
||
| export class EmailQueueService { | ||
| private static initialized = false; | ||
| public static transactionalQueue = new Map<string, Queue<QueueEmailJob>>(); | ||
|
|
@@ -68,11 +141,12 @@ export class EmailQueueService { | |
| { region, transactionalQuota }, | ||
| `[EmailQueueService]: Updating transactional quota for region` | ||
| ); | ||
| const transactionalWorker = this.transactionalWorker.get(region); | ||
| if (transactionalWorker) { | ||
| transactionalWorker.concurrency = | ||
| transactionalQuota !== 0 ? transactionalQuota : 1; | ||
| } | ||
| replaceWorker( | ||
| this.transactionalWorker, | ||
| region, | ||
| `${region}-transaction`, | ||
| transactionalQuota !== 0 ? transactionalQuota : 1, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '100,170p;350,380p' apps/web/src/server/service/email-queue-service.ts
git diff bcf7e07e92b11f00fade54ff9a636ca7b5416b0c 0aba3fe907ee747fed248849f178124433234180 -- apps/web/src/server/service/email-queue-service.ts | head -200Repository: usesend/useSend Length of output: 9026 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- current source ---'
sed -n '1,175p;180,215p;430,555p' apps/web/src/server/service/email-queue-service.ts
printf '%s\n' '--- base source ---'
git show bcf7e07e92b11f00fade54ff9a636ca7b5416b0c:apps/web/src/server/service/email-queue-service.ts | sed -n '1,155p;180,210p;430,525p'Repository: usesend/useSend Length of output: 17076 Keep the combined queue limit within the regional quota. When SES may throttle the extra send. The retry logic limits the impact to a delayed retry, so this is a minor issue rather than a major availability issue. Use a shared regional limiter for both queues, while preserving the one-per-second treatment for zero-share jobs within that shared budget. 🤖 Prompt for AI Agents
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
✏️ Learnings added
🧠 Learnings usedYou are interacting with an AI system. |
||
| ); | ||
| } else { | ||
| logger.info( | ||
| { region, transactionalQuota }, | ||
|
|
@@ -93,10 +167,12 @@ export class EmailQueueService { | |
| { region, marketingQuota }, | ||
| `[EmailQueueService]: Updating marketing quota for region` | ||
| ); | ||
| const marketingWorker = this.marketingWorker.get(region); | ||
| if (marketingWorker) { | ||
| marketingWorker.concurrency = marketingQuota !== 0 ? marketingQuota : 1; | ||
| } | ||
| replaceWorker( | ||
| this.marketingWorker, | ||
| region, | ||
| `${region}-marketing`, | ||
| marketingQuota !== 0 ? marketingQuota : 1, | ||
| ); | ||
| } else { | ||
| logger.info( | ||
| { region, marketingQuota }, | ||
|
|
@@ -140,7 +216,7 @@ export class EmailQueueService { | |
| isBulk, | ||
| teamId, | ||
| }, | ||
| { jobId: emailId, delay, ...DEFAULT_QUEUE_OPTIONS } | ||
| { jobId: emailId, delay, ...EMAIL_JOB_OPTIONS } | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -231,7 +307,7 @@ export class EmailQueueService { | |
| opts: { | ||
| jobId: job.emailId, // Use emailId as jobId | ||
| delay: job.delay, | ||
| ...DEFAULT_QUEUE_OPTIONS, // Apply default options (attempts, backoff) | ||
| ...EMAIL_JOB_OPTIONS, // Apply default options (attempts, backoff) | ||
| }, | ||
| })); | ||
|
|
||
|
|
@@ -314,6 +390,12 @@ export class EmailQueueService { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Sends one queued email through SES and records the outcome on the email. | ||
| * A throttled send is thrown back to the queue while the job has attempts | ||
| * left, so the queue retries it after its backoff; any other error, and a | ||
| * throttle on the last attempt, marks the email FAILED. | ||
| */ | ||
| async function executeEmail(job: QueueEmailJob) { | ||
| logger.info( | ||
| { emailId: job.data.emailId, elapsed: Date.now() - job.data.timestamp }, | ||
|
|
@@ -461,6 +543,16 @@ async function executeEmail(job: QueueEmailJob) { | |
| data: { sesEmailId: messageId, text, attachments: null, headers: null }, | ||
| }); | ||
| } catch (error: any) { | ||
| const attempt = job.attemptsMade + 1; | ||
| const attempts = job.opts.attempts ?? 1; | ||
| if (isThrottled(error) && attempt < attempts) { | ||
| logger.warn( | ||
| { emailId: email.id, attempt, attempts }, | ||
| `[EmailQueueService]: SES throttled the send, the queue retries it`, | ||
| ); | ||
| // rethrowing fails this attempt, so BullMQ retries the job after its backoff | ||
| throw error; | ||
| } | ||
| await db.emailEvent.create({ | ||
| data: { | ||
| emailId: email.id, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rate-limit SES calls, not only job starts.
BullMQ applies this limiter when jobs become active.
executeEmailthen performs database work and a limit check beforesendRawEmail. If one batch waits on that work while another batch starts, their SES calls can occur in the same second and exceed the configured send rate. Apply a shared regional rate gate immediately before the SES call. (docs.bullmq.io)🤖 Prompt for AI Agents
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge usesend/useSend /tmp/coderabbit-repo-knowledge/usesend-usesend-3d828984/architecture /tmp/coderabbit-repo-knowledge/usesend-usesend-3d828984/learningsLength of output: 33576
🏁 Script executed:
Repository: usesend/useSend
Length of output: 32910
🏁 Script executed:
Repository: usesend/useSend
Length of output: 179
🏁 Script executed:
Repository: usesend/useSend
Length of output: 30096
🏁 Script executed:
Repository: usesend/useSend
Length of output: 5323
🏁 Script executed:
Repository: usesend/useSend
Length of output: 5696
🏁 Script executed:
Repository: usesend/useSend
Length of output: 20904
Account for every SES recipient in the rate limiter.
createWorkerlimits job starts, butexecuteEmailpassesto,cc, andbccarrays tosendRawEmail. SES applies the send-rate quota per recipient, so a multi-recipient job can consume multiple quota units while consuming one limiter unit. Add recipient-based rate limiting or enforce the SES per-recipient quota before sending.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked both points against the code and SES's rules.
Job starts vs the SES call. The limiter bounds how many jobs a queue's workers may start per second. What runs between a job's start and its
sendRawEmailis two or three primary-key reads and the limit check, milliseconds against a one-second window. A job whose reads straddle a second boundary lands its send in the next window, so a window can briefly hold a few more sends thanmax. That is the burst SES explicitly allows ("You can exceed this quota for short bursts, but not for sustained periods", Managing your Amazon SES sending limits), and a send that is still refused is retried by this PR. An in-process gate right before the SES call would hold for one process only; the BullMQ limiter lives in Redis and holds across every worker and instance of the queue, which is why the cap sits there. A second gate at the call would need a Redis token bucket of its own. I'd keep this PR to the limiter and the retry, and can follow up if the maintainers want that gate.Recipients. Correct: SES counts recipients, the limiter counts jobs, and
to,ccandbccgo out as one send, so a multi-recipient mail spends more rate than limiter. That was already so before this PR (nothing counted anything), and it is the case SES itself advises against (oneSendEmailper recipient, same page). A per-recipient budget needs a limiter with a per-job cost, which BullMQ's does not have; a custom Redis bucket is a larger change than this fix. Added to the PR description as a limitation, with the retry as the net under it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🤖 web_search executed:
Amazon SES managing your sending limits maximum send rate per second recipients versus messages sending quota short bursts one send per recipient official documentationLength of output: 11301
@sefasenturk95, thanks for the detailed explanation. AWS confirms that SES permits short bursts above the sending rate. I withdraw the request for a second rate gate in this PR.AWS also confirms that SES sending quotas count recipients, not messages. The BullMQ limiter still counts one job for a send with multiple
to,cc, orbccrecipients. I agree that a shared per-recipient budget is a larger change, but documenting the limitation does not remove it. I’ll leave that finding open. Would you like me to open a follow-up issue?✏️ Learnings added
You are interacting with an AI system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai Yes, please open a follow-up issue for the per-recipient rate accounting (a send with several
to,ccorbccaddresses counts once against the limiter and once per recipient at SES), so it is tracked separately from this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 427
🏁 Script executed:
Length of output: 1711
@sefasenturk95, I opened#455and assigned it to you. It tracks per-recipient SES rate accounting separately from PR#454. The review finding remains open because the code has not changed.🧠 Learnings used
You are interacting with an AI system.