Merge pull request #19968 from mozilla/dont-fail-on-email-send-error

polish(auth): Only fail hard on email send for emails with codes

Author: dschom

libs/accounts/email-sender/src/email-sender.spec.ts

@@ -226,7 +226,7 @@ describe('EmailSender', () => {
         new Error('Temporary error')
       );
 
-      await expect(emailSender.send(defaultMockEmail)).rejects.toThrow(
+      await expect(emailSender.send(defaultMockEmail, true)).rejects.toThrow(
         'Temporary error'
       );
 
@@ -260,7 +260,7 @@ describe('EmailSender', () => {
         new Error('Temporary error')
       );
 
-      await expect(emailSender.send(defaultMockEmail)).rejects.toThrow(
+      await expect(emailSender.send(defaultMockEmail, true)).rejects.toThrow(
         'Temporary error'
       );
 
@@ -294,7 +294,7 @@ describe('EmailSender', () => {
       const error = new Error('SMTP connection failed');
       mockTransport.sendMail.mockRejectedValueOnce(error);
 
-      await expect(emailSender.send(defaultMockEmail)).rejects.toThrow(
+      await expect(emailSender.send(defaultMockEmail, true)).rejects.toThrow(
         'SMTP connection failed'
       );
 
@@ -331,7 +331,7 @@ describe('EmailSender', () => {
       const appError = AppError.emailBouncedHard(100_000_000_000);
       mockTransport.sendMail.mockRejectedValueOnce(appError);
 
-      await expect(emailSender.send(defaultMockEmail)).rejects.toThrow();
+      await expect(emailSender.send(defaultMockEmail, true)).rejects.toThrow();
 
       expect(mockTransport.sendMail).toHaveBeenCalledTimes(1);
       expect(mockLogger.error).toHaveBeenCalledWith(
@@ -378,7 +378,7 @@ describe('EmailSender', () => {
 
       mockTransport.sendMail.mockRejectedValueOnce(smtpError);
 
-      await expect(emailSender.send(emailWithCc)).rejects.toThrow();
+      await expect(emailSender.send(emailWithCc, true)).rejects.toThrow();
 
       expect(mockLogger.error).toHaveBeenCalledWith(
         'mailer.send.error',
@@ -621,7 +621,7 @@ describe('EmailSender', () => {
         mockLogger
       );
 
-      await expect(emailSender.send(defaultMockEmail)).rejects.toThrow(
+      await expect(emailSender.send(defaultMockEmail, true)).rejects.toThrow(
         'Temporary SMTP error'
       );
 
@@ -640,6 +640,28 @@ describe('EmailSender', () => {
         }
       );
       expect(mockStatsd.increment).toHaveBeenCalledTimes(4);
+
+      // Make sure issue is still sent to sentry
+      expect(mockSentryCaptureException).toHaveBeenCalledWith(error);
+    });
+
+    it('suppresses error with directive', async () => {
+      const error = new Error('Temporary SMTP error');
+
+      mockTransport.sendMail.mockRejectedValue(error);
+
+      emailSender = new EmailSender(
+        configWithRetry,
+        mockBounces,
+        mockStatsd,
+        mockLogger
+      );
+
+      // Shouldn't error...
+      await emailSender.send(defaultMockEmail, false);
+
+      // But should capture
+      expect(mockSentryCaptureException).toHaveBeenCalledWith(error);
     });
   });
   describe('calculateBackoffDelay', () => {

libs/accounts/email-sender/src/email-sender.ts

@@ -222,17 +222,18 @@ export class EmailSender {
   /**
    * Send out an email.
    * @param email Email to send
+   * @param
    * @returns
    */
-  async send(email: Email) {
+  async send(email: Email, throwOnFailure = false) {
     // Always check for email bounces. Don't send if there bounce errors
     if (await this.hasBounceErrors(email)) {
       return {
         sent: false,
         message: 'Has bounce errors!',
       };
     }
-    return await this.sendMail(email);
+    return await this.sendMail(email, throwOnFailure);
   }
 
   /**
@@ -274,6 +275,7 @@ export class EmailSender {
 
   private async sendMail(
     email: Email,
+    throwOnFailure: boolean,
     attempt = 0
   ): Promise<{
     sent: boolean;
@@ -372,7 +374,7 @@ export class EmailSender {
 
         // Wait for backoff period, then retry
         await new Promise((resolve) => setTimeout(resolve, backoffDelay));
-        return this.sendMail(email, ++attempt);
+        return this.sendMail(email, throwOnFailure, ++attempt);
       }
 
       const smtpErrorDetails: Record<string, unknown> = {
@@ -415,7 +417,11 @@ export class EmailSender {
       }
 
       // Throw error back to calling code.
-      throw err;
+      if (throwOnFailure) {
+        throw err;
+      }
+
+      return { sent: false };
     }
   }
 }

packages/fxa-auth-server/lib/senders/fxa-mailer.ts

@@ -156,7 +156,7 @@ export class FxaMailer extends FxaEmailRenderer {
       ...opts,
       ...links,
     });
-    return this.sendEmail(opts, headers, rendered);
+    return this.sendEmail(opts, headers, rendered, true);
   }
 
   /**
@@ -189,7 +189,7 @@ export class FxaMailer extends FxaEmailRenderer {
       ...opts,
       ...links,
     });
-    return this.sendEmail(opts, headers, rendered);
+    return this.sendEmail(opts, headers, rendered, true);
   }
 
   async sendPostVerifySecondaryEmail(
@@ -1030,7 +1030,7 @@ export class FxaMailer extends FxaEmailRenderer {
       ...opts,
       ...links,
     });
-    return this.sendEmail(opts, headers, rendered);
+    return this.sendEmail(opts, headers, rendered, true);
   }
 
   async sendVerifyShortCodeEmail(
@@ -1067,7 +1067,7 @@ export class FxaMailer extends FxaEmailRenderer {
       ...opts,
       ...links,
     });
-    return this.sendEmail(opts, headers, rendered);
+    return this.sendEmail(opts, headers, rendered, true);
   }
 
   async sendVerifyEmail(
@@ -1686,19 +1686,23 @@ export class FxaMailer extends FxaEmailRenderer {
   private async sendEmail(
     opts: { to: string; cc?: string[]; cmsRpFromName?: string },
     headers: Record<string, string>,
-    rendered: RenderedTemplate
+    rendered: RenderedTemplate,
+    throwErrorOnSendFailure = true
   ) {
     const { cmsRpFromName, to, cc } = opts;
     const from = cmsRpFromName
       ? `${cmsRpFromName} <${this.mailerConfig.sender}>`
       : this.mailerConfig.sender;
 
-    return this.emailSender.send({
-      to,
-      cc,
-      from,
-      headers,
-      ...rendered,
-    });
+    return this.emailSender.send(
+      {
+        to,
+        cc,
+        from,
+        headers,
+        ...rendered,
+      },
+      throwErrorOnSendFailure
+    );
   }
 }