From 7781c2da07c077ad0a88c414c3e81efa219dffd0 Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Tue, 21 Feb 2023 15:41:00 +0100 Subject: [PATCH] Implemented retrying for sending email via Mailgun API no issue Retry sending an email up to 5 times if it failed. --- .../lib/batch-sending-service.js | 65 ++++++++++++------- .../lib/mailgun-email-provider.js | 3 - .../test/batch-sending-service.test.js | 27 ++++++-- 3 files changed, 62 insertions(+), 33 deletions(-) diff --git a/ghost/email-service/lib/batch-sending-service.js b/ghost/email-service/lib/batch-sending-service.js index f807cbbb6e..4b81a1e09d 100644 --- a/ghost/email-service/lib/batch-sending-service.js +++ b/ghost/email-service/lib/batch-sending-service.js @@ -35,6 +35,7 @@ class BatchSendingService { // Retry database queries happening before sending the email #BEFORE_RETRY_CONFIG = {maxRetries: 10, maxTime: 10 * 60 * 1000, sleep: 2000}; #AFTER_RETRY_CONFIG = {maxRetries: 20, maxTime: 30 * 60 * 1000, sleep: 2000}; + #MAILGUN_API_RETRY_CONFIG = {sleep: 10 * 1000, maxRetries: 5}; /** * @param {Object} dependencies @@ -61,7 +62,8 @@ class BatchSendingService { db, sentry, BEFORE_RETRY_CONFIG, - AFTER_RETRY_CONFIG + AFTER_RETRY_CONFIG, + MAILGUN_API_RETRY_CONFIG }) { this.#emailRenderer = emailRenderer; this.#sendingService = sendingService; @@ -77,6 +79,13 @@ class BatchSendingService { if (AFTER_RETRY_CONFIG) { this.#AFTER_RETRY_CONFIG = AFTER_RETRY_CONFIG; } + if (MAILGUN_API_RETRY_CONFIG) { + this.#MAILGUN_API_RETRY_CONFIG = MAILGUN_API_RETRY_CONFIG; + } else { + if (process.env.NODE_ENV.startsWith('test')) { + this.#MAILGUN_API_RETRY_CONFIG = {maxRetries: 0}; + } + } } #getBeforeRetryConfig(email) { @@ -393,17 +402,19 @@ class BatchSendingService { {...this.#getBeforeRetryConfig(email), description: `getBatchMembers batch ${originalBatch.id}`} ); - const response = await this.#sendingService.send({ - emailId: email.id, - post, - newsletter, - segment: batch.get('member_segment'), - members - }, { - openTrackingEnabled: !!email.get('track_opens'), - clickTrackingEnabled: !!email.get('track_clicks'), - emailBodyCache - }); + const response = await this.retryDb(async () => { + return await this.#sendingService.send({ + emailId: email.id, + post, + newsletter, + segment: batch.get('member_segment'), + members + }, { + openTrackingEnabled: !!email.get('track_opens'), + clickTrackingEnabled: !!email.get('track_clicks'), + emailBodyCache + }); + }, {...this.#MAILGUN_API_RETRY_CONFIG, description: `Sending email batch ${originalBatch.id}`}); succeeded = true; await this.retryDb( @@ -420,8 +431,13 @@ class BatchSendingService { {...this.#AFTER_RETRY_CONFIG, description: `save batch ${originalBatch.id} -> submitted`} ); } catch (err) { - if (!err.code || err.code !== 'BULK_EMAIL_SEND_FAILED') { - // BULK_EMAIL_SEND_FAILED are already logged in mailgun-email-provider + if (err.code && err.code === 'BULK_EMAIL_SEND_FAILED') { + logging.error(err); + if (this.#sentry) { + // Log the original error to Sentry + this.#sentry.captureException(err); + } + } else { const ghostError = new errors.EmailError({ err, code: 'BULK_EMAIL_SEND_FAILED', @@ -533,17 +549,18 @@ class BatchSendingService { return await func(); } catch (e) { const retryCount = (options.retryCount ?? 0); - const sleep = (options.sleep ?? 0) * (retryCount + 1); + const sleep = (options.sleep ?? 0); if (retryCount >= options.maxRetries || (options.stopAfterDate && (new Date(Date.now() + sleep)) > options.stopAfterDate)) { - const ghostError = new errors.EmailError({ - err: e, - code: 'BULK_EMAIL_DB_RETRY', - message: `[BULK_EMAIL_DB_RETRY] ${options.description} - Stopped retrying`, - context: e.message - }); - - logging.error(ghostError); + if (retryCount > 0) { + const ghostError = new errors.EmailError({ + err: e, + code: 'BULK_EMAIL_DB_RETRY', + message: `[BULK_EMAIL_DB_RETRY] ${options.description} - Stopped retrying`, + context: e.message + }); + logging.error(ghostError); + } throw e; } @@ -561,7 +578,7 @@ class BatchSendingService { setTimeout(resolve, sleep); }); } - return await this.retryDb(func, {...options, retryCount: retryCount + 1}); + return await this.retryDb(func, {...options, retryCount: retryCount + 1, sleep: sleep * 2}); } } } diff --git a/ghost/email-service/lib/mailgun-email-provider.js b/ghost/email-service/lib/mailgun-email-provider.js index eb5546c321..fb12d51cd7 100644 --- a/ghost/email-service/lib/mailgun-email-provider.js +++ b/ghost/email-service/lib/mailgun-email-provider.js @@ -165,11 +165,8 @@ class MailgunEmailProvider { }); } - logging.error(ghostError); debug(`failed to send message (${Date.now() - startTime}ms)`); - // log error to custom error handler. ex sentry - this.#errorHandler(ghostError); throw ghostError; } } diff --git a/ghost/email-service/test/batch-sending-service.test.js b/ghost/email-service/test/batch-sending-service.test.js index 55acf7373d..66fafcbb07 100644 --- a/ghost/email-service/test/batch-sending-service.test.js +++ b/ghost/email-service/test/batch-sending-service.test.js @@ -661,7 +661,10 @@ describe('Batch Sending Service', function () { const findOne = sinon.spy(EmailBatch, 'findOne'); const service = new BatchSendingService({ models: {EmailBatch, EmailRecipient}, - sendingService + sendingService, + MAILGUN_API_RETRY_CONFIG: { + sleep: 10, maxRetries: 5 + } }); const result = await service.sendBatch({ @@ -672,8 +675,8 @@ describe('Batch Sending Service', function () { }); assert.equal(result, false); - sinon.assert.calledOnce(errorLog); - sinon.assert.calledOnce(sendingService.send); + sinon.assert.callCount(errorLog, 7); + sinon.assert.callCount(sendingService.send, 6); sinon.assert.calledOnce(findOne); const batch = await findOne.firstCall.returnValue; @@ -701,6 +704,9 @@ describe('Batch Sending Service', function () { sendingService, sentry: { captureException + }, + MAILGUN_API_RETRY_CONFIG: { + maxRetries: 0 } }); @@ -748,11 +754,17 @@ describe('Batch Sending Service', function () { code: 'BULK_EMAIL_SEND_FAILED' })) }; - + const captureException = sinon.stub(); const findOne = sinon.spy(EmailBatch, 'findOne'); const service = new BatchSendingService({ models: {EmailBatch, EmailRecipient}, - sendingService + sendingService, + sentry: { + captureException + }, + MAILGUN_API_RETRY_CONFIG: { + maxRetries: 0 + } }); const result = await service.sendBatch({ @@ -763,8 +775,11 @@ describe('Batch Sending Service', function () { }); assert.equal(result, false); - sinon.assert.notCalled(errorLog); + sinon.assert.calledOnce(errorLog); sinon.assert.calledOnce(sendingService.send); + sinon.assert.calledOnce(captureException); + const sentryExeption = captureException.firstCall.args[0]; + assert.equal(sentryExeption.message, 'Test error'); sinon.assert.calledOnce(findOne); const batch = await findOne.firstCall.returnValue;