Added forced debug output for EmailRecipients fetching (#16823)

refs TryGhost/Team#3229

- The issue we are observing that even though the returned amount of email recipients should not ever accede the max batch size (1000 in case of MailGun), there are rare glitches when this number is doubled and we fetch 2000 records instead.
- The fix takes it's best guess in de-duping data in the batch and then truncates it if the amount of records is still above the threshold. This ensures we at least end up sending the emails out to some of the recipients instead of none.

ghost/email-service/lib/BatchSendingService.js

@@ -1,3 +1,4 @@
+const uniqBy = require('lodash/uniqBy');
 const logging = require('@tryghost/logging');
 const ObjectID = require('bson-objectid').default;
 const errors = require('@tryghost/errors');
@@ -52,6 +53,7 @@ class BatchSendingService {
      * @param {object} [dependencies.sentry]
      * @param {object} [dependencies.BEFORE_RETRY_CONFIG]
      * @param {object} [dependencies.AFTER_RETRY_CONFIG]
+     * @param {object} [dependencies.MAILGUN_API_RETRY_CONFIG]
      */
     constructor({
         emailRenderer,
@@ -393,7 +395,7 @@ class BatchSendingService {
         let succeeded = false;
 
         try {
-            const members = await this.retryDb(
+            let members = await this.retryDb(
                 async () => {
                     const m = await this.getBatchMembers(batch.id);
 
@@ -410,6 +412,19 @@ class BatchSendingService {
                 {...this.#getBeforeRetryConfig(email), description: `getBatchMembers batch ${originalBatch.id}`}
             );
 
+            if (members.length > this.#sendingService.getMaximumRecipients()) {
+                // @NOTE the unique by member_id is a best effort to make sure we don't send the same email to the same member twice
+                logging.error(`Email batch ${originalBatch.id} has ${members.length} members, which exceeds the maximum of ${this.#sendingService.getMaximumRecipients()}. Filtering to unique members`);
+                members = uniqBy(members, 'email');
+
+                if (members.length > this.#sendingService.getMaximumRecipients()) {
+                    // @NOTE this is a best effort logic to still try sending an email batch
+                    //       even if it exceeds the maximum recipients limit of the sending service
+                    logging.error(`Email batch ${originalBatch.id} has ${members.length} members, which exceeds the maximum of ${this.#sendingService.getMaximumRecipients()}. Truncating to ${this.#sendingService.getMaximumRecipients()}`);
+                    members = members.slice(0, this.#sendingService.getMaximumRecipients());
+                }
+            }
+
             const response = await this.retryDb(async () => {
                 return await this.#sendingService.send({
                     emailId: email.id,
@@ -492,12 +507,13 @@ class BatchSendingService {
     /**
      * We don't want to pass EmailRecipient models to the sendingService.
      * So we transform them into the MemberLike interface.
-     * That keeps the sending service nicely seperated so it isn't dependent on the batch sending data structure.
+     * That keeps the sending service nicely separated so it isn't dependent on the batch sending data structure.
      * @returns {Promise<MemberLike[]>}
      */
     async getBatchMembers(batchId) {
         const models = await this.#models.EmailRecipient.findAll({filter: `batch_id:${batchId}`, withRelated: ['member', 'member.stripeSubscriptions', 'member.products']});
-        return models.map((model) => {
+
+        const mappedMemberLikes = models.map((model) => {
             // Map subscriptions
             const subscriptions = model.related('member').related('stripeSubscriptions').toJSON();
             const tiers = model.related('member').related('products').toJSON();
@@ -513,6 +529,13 @@ class BatchSendingService {
                 tiers
             };
         });
+
+        const BATCH_SIZE = this.#sendingService.getMaximumRecipients();
+        if (mappedMemberLikes.length > BATCH_SIZE) {
+            logging.error(`Batch ${batchId} has ${mappedMemberLikes.length} members, but the sending service only supports ${BATCH_SIZE} members per batch.`);
+        }
+
+        return mappedMemberLikes;
     }
 
     /**

ghost/email-service/lib/MailgunEmailProvider.js

@@ -34,7 +34,7 @@ class MailgunEmailProvider {
 
     /**
      * @param {object} dependencies
-     * @param {import('@tryghost/mailgun-client/lib/mailgun-client')} dependencies.mailgunClient - mailgun client to send emails
+     * @param {import('@tryghost/mailgun-client/lib/MailgunClient')} dependencies.mailgunClient - mailgun client to send emails
      * @param {Function} [dependencies.errorHandler] - custom error handler for logging exceptions
      */
     constructor({

ghost/email-service/test/batch-sending-service.test.js

@@ -646,7 +646,8 @@ describe('Batch Sending Service', function () {
                 }
             });
             const sendingService = {
-                send: sinon.stub().resolves({id: 'providerid@example.com'})
+                send: sinon.stub().resolves({id: 'providerid@example.com'}),
+                getMaximumRecipients: () => 5
             };
 
             const findOne = sinon.spy(EmailBatch, 'findOne');
@@ -683,7 +684,8 @@ describe('Batch Sending Service', function () {
                 }
             });
             const sendingService = {
-                send: sinon.stub().rejects(new Error('Test error'))
+                send: sinon.stub().rejects(new Error('Test error')),
+                getMaximumRecipients: () => 5
             };
 
             const findOne = sinon.spy(EmailBatch, 'findOne');
@@ -722,7 +724,8 @@ describe('Batch Sending Service', function () {
                 }
             });
             const sendingService = {
-                send: sinon.stub().rejects(new Error('Test error'))
+                send: sinon.stub().rejects(new Error('Test error')),
+                getMaximumRecipients: () => 5
             };
 
             const findOne = sinon.spy(EmailBatch, 'findOne');
@@ -780,7 +783,8 @@ describe('Batch Sending Service', function () {
                     context: `Mailgun Error 500: Test error`,
                     help: `https://ghost.org/docs/newsletters/#bulk-email-configuration`,
                     code: 'BULK_EMAIL_SEND_FAILED'
-                }))
+                })),
+                getMaximumRecipients: () => 5
             };
             const captureException = sinon.stub();
             const findOne = sinon.spy(EmailBatch, 'findOne');
@@ -825,7 +829,8 @@ describe('Batch Sending Service', function () {
                 }
             });
             const sendingService = {
-                send: sinon.stub().resolves({id: 'providerid@example.com'})
+                send: sinon.stub().resolves({id: 'providerid@example.com'}),
+                getMaximumRecipients: () => 5
             };
 
             const WrongEmailRecipient = createModelClass({
@@ -876,6 +881,115 @@ describe('Batch Sending Service', function () {
             assert.equal(members.length, 2);
         });
 
+        it('Truncates recipients if more than the maximum are returned in a batch', async function () {
+            const EmailBatch = createModelClass({
+                findOne: {
+                    status: 'pending',
+                    member_segment: null
+                }
+            });
+            const findOne = sinon.spy(EmailBatch, 'findOne');
+
+            const DoubleTheEmailRecipients = createModelClass({
+                findAll: [
+                    {
+                        member_id: '123',
+                        member_uuid: '123',
+                        member_email: 'example@example.com',
+                        member_name: 'Test User',
+                        loaded: ['member'],
+                        member: createModel({
+                            created_at: new Date(),
+                            loaded: ['stripeSubscriptions', 'products'],
+                            status: 'free',
+                            stripeSubscriptions: [],
+                            products: []
+                        })
+                    },
+                    {
+                        member_id: '124',
+                        member_uuid: '124',
+                        member_email: 'example2@example.com',
+                        member_name: 'Test User 2',
+                        loaded: ['member'],
+                        member: createModel({
+                            created_at: new Date(),
+                            status: 'free',
+                            loaded: ['stripeSubscriptions', 'products'],
+                            stripeSubscriptions: [],
+                            products: []
+                        })
+                    },
+                    {
+                        member_id: '125',
+                        member_uuid: '125',
+                        member_email: 'example3@example.com',
+                        member_name: 'Test User 3',
+                        loaded: ['member'],
+                        member: createModel({
+                            created_at: new Date(),
+                            status: 'free',
+                            loaded: ['stripeSubscriptions', 'products'],
+                            stripeSubscriptions: [],
+                            products: []
+                        })
+                    },
+                    // NOTE: one recipient with a duplicate data
+                    {
+                        member_id: '125',
+                        member_uuid: '125',
+                        member_email: 'example3@example.com',
+                        member_name: 'Test User 3',
+                        loaded: ['member'],
+                        member: createModel({
+                            created_at: new Date(),
+                            status: 'free',
+                            loaded: ['stripeSubscriptions', 'products'],
+                            stripeSubscriptions: [],
+                            products: []
+                        })
+                    }
+                ]
+            });
+
+            const sendingService = {
+                send: sinon.stub().resolves({id: 'providerid@example.com'}),
+                getMaximumRecipients: () => 2
+            };
+
+            const service = new BatchSendingService({
+                models: {EmailBatch, EmailRecipient: DoubleTheEmailRecipients},
+                sendingService,
+                BEFORE_RETRY_CONFIG: {maxRetries: 10, maxTime: 2000, sleep: 1}
+            });
+
+            const result = await service.sendBatch({
+                email: createModel({}),
+                batch: createModel({}),
+                post: createModel({}),
+                newsletter: createModel({})
+            });
+
+            assert.equal(result, true);
+            sinon.assert.calledThrice(errorLog);
+            const firstLoggedError = errorLog.firstCall.args[0];
+            const secondLoggedError = errorLog.secondCall.args[0];
+            const thirdLoggedError = errorLog.thirdCall.args[0];
+
+            assert.match(firstLoggedError, /Batch [a-f0-9]{24} has 4 members, but the sending service only supports 2 members per batch/);
+            assert.match(secondLoggedError, /Email batch [a-f0-9]{24} has 4 members, which exceeds the maximum of 2. Filtering to unique members/);
+            assert.match(thirdLoggedError, /Email batch [a-f0-9]{24} has 3 members, which exceeds the maximum of 2. Truncating to 2/);
+
+            sinon.assert.calledOnce(sendingService.send);
+            const {members} = sendingService.send.firstCall.args[0];
+            assert.equal(members.length, 2);
+
+            sinon.assert.calledOnce(findOne);
+            const batch = await findOne.firstCall.returnValue;
+            assert.equal(batch.get('status'), 'submitted');
+            assert.equal(batch.get('provider_id'), 'providerid@example.com');
+        });
+
         it('Stops retrying after the email retry cut off time', async function () {
             const EmailBatch = createModelClass({
                 findOne: {
@@ -884,7 +998,8 @@ describe('Batch Sending Service', function () {
                 }
             });
             const sendingService = {
-                send: sinon.stub().resolves({id: 'providerid@example.com'})
+                send: sinon.stub().resolves({id: 'providerid@example.com'}),
+                getMaximumRecipients: () => 5
             };
 
             const WrongEmailRecipient = createModelClass({
@@ -944,7 +1059,10 @@ describe('Batch Sending Service', function () {
             });
 
             const service = new BatchSendingService({
-                models: {EmailRecipient}
+                models: {EmailRecipient},
+                sendingService: {
+                    getMaximumRecipients: () => 5
+                }
             });
 
             const result = await service.getBatchMembers('id123');