ref(serverless): Do not throw on flush error (#5090)

Author: Luca Forstner

MIGRATION.md

@@ -340,6 +340,7 @@ For our efforts to reduce bundle size of the SDK we had to remove and refactor p
 - Remove `SDK_NAME` export from `@sentry/browser`, `@sentry/node`, `@sentry/tracing` and `@sentry/vue` packages.
 - Removed `eventStatusFromHttpCode` to save on bundle size.
 - Replace `BrowserTracing` `maxTransactionDuration` option with `finalTimeout` option
+- Removed `ignoreSentryErrors` option from AWS lambda SDK. Errors originating from the SDK will now *always* be caught internally.
 
 ## Sentry Angular SDK Changes
 

packages/serverless/src/awslambda.ts

@@ -11,7 +11,7 @@ import {
 } from '@sentry/node';
 import { extractTraceparentData } from '@sentry/tracing';
 import { Integration } from '@sentry/types';
-import { isString, logger, SentryError } from '@sentry/utils';
+import { isString, logger } from '@sentry/utils';
 // NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil
 // eslint-disable-next-line import/no-unresolved
 import { Context, Handler } from 'aws-lambda';
@@ -54,7 +54,6 @@ export interface WrapperOptions {
    * @default false
    */
   captureAllSettledReasons: boolean;
-  ignoreSentryErrors: boolean;
 }
 
 export const defaultIntegrations: Integration[] = [...Sentry.defaultIntegrations, new AWSServices({ optional: true })];
@@ -226,7 +225,6 @@ export function wrapHandler<TEvent, TResult>(
     captureTimeoutWarning: true,
     timeoutWarningLimit: 500,
     captureAllSettledReasons: false,
-    ignoreSentryErrors: true,
     ...wrapOptions,
   };
   let timeoutWarningTimer: NodeJS.Timeout;
@@ -318,11 +316,7 @@ export function wrapHandler<TEvent, TResult>(
       transaction.finish();
       hub.popScope();
       await flush(options.flushTimeout).catch(e => {
-        if (options.ignoreSentryErrors && e instanceof SentryError) {
-          IS_DEBUG_BUILD && logger.error(e);
-          return;
-        }
-        throw e;
+        IS_DEBUG_BUILD && logger.error(e);
       });
     }
     return rv;

packages/serverless/src/gcpfunction/cloud_events.ts

@@ -61,11 +61,11 @@ function _wrapCloudEventFunction(
       transaction.finish();
 
       void flush(options.flushTimeout)
-        .then(() => {
-          callback(...args);
-        })
         .then(null, e => {
           IS_DEBUG_BUILD && logger.error(e);
+        })
+        .then(() => {
+          callback(...args);
         });
     });
 

packages/serverless/src/gcpfunction/events.ts

@@ -56,11 +56,11 @@ function _wrapEventFunction(
       transaction.finish();
 
       void flush(options.flushTimeout)
-        .then(() => {
-          callback(...args);
-        })
         .then(null, e => {
           IS_DEBUG_BUILD && logger.error(e);
+        })
+        .then(() => {
+          callback(...args);
         });
     });
 

packages/serverless/src/gcpfunction/http.ts

@@ -91,11 +91,11 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial<HttpFunctionWr
       transaction.finish();
 
       void flush(options.flushTimeout)
-        .then(() => {
-          _end.call(this, chunk, encoding, cb);
-        })
         .then(null, e => {
           IS_DEBUG_BUILD && logger.error(e);
+        })
+        .then(() => {
+          _end.call(this, chunk, encoding, cb);
         });
     };
 

packages/serverless/test/awslambda.test.ts

@@ -1,4 +1,3 @@
-import { SentryError } from '@sentry/utils';
 // NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil
 // eslint-disable-next-line import/no-unresolved
 import { Callback, Handler } from 'aws-lambda';
@@ -178,44 +177,6 @@ describe('AWSLambda', () => {
       expect(Sentry.captureException).toHaveBeenNthCalledWith(2, error2);
       expect(Sentry.captureException).toBeCalledTimes(2);
     });
-
-    test('ignoreSentryErrors - with successful handler', async () => {
-      const sentryError = new SentryError('HTTP Error (429)');
-      jest.spyOn(Sentry, 'flush').mockRejectedValueOnce(sentryError);
-
-      const handledError = new Error('handled error, and we want to monitor it');
-      const expectedSuccessStatus = { status: 'success', reason: 'we handled error, so success' };
-      const handler = () => {
-        Sentry.captureException(handledError);
-        return Promise.resolve(expectedSuccessStatus);
-      };
-      const wrappedHandlerWithoutIgnoringSentryErrors = wrapHandler(handler, { ignoreSentryErrors: false });
-      const wrappedHandlerWithIgnoringSentryErrors = wrapHandler(handler, { ignoreSentryErrors: true });
-
-      await expect(wrappedHandlerWithoutIgnoringSentryErrors(fakeEvent, fakeContext, fakeCallback)).rejects.toThrow(
-        sentryError,
-      );
-      await expect(wrappedHandlerWithIgnoringSentryErrors(fakeEvent, fakeContext, fakeCallback)).resolves.toBe(
-        expectedSuccessStatus,
-      );
-    });
-
-    test('ignoreSentryErrors - with failed handler', async () => {
-      const sentryError = new SentryError('HTTP Error (429)');
-      jest.spyOn(Sentry, 'flush').mockRejectedValueOnce(sentryError);
-
-      const criticalUnhandledError = new Error('critical unhandled error ');
-      const handler = () => Promise.reject(criticalUnhandledError);
-      const wrappedHandlerWithoutIgnoringSentryErrors = wrapHandler(handler, { ignoreSentryErrors: false });
-      const wrappedHandlerWithIgnoringSentryErrors = wrapHandler(handler, { ignoreSentryErrors: true });
-
-      await expect(wrappedHandlerWithoutIgnoringSentryErrors(fakeEvent, fakeContext, fakeCallback)).rejects.toThrow(
-        sentryError,
-      );
-      await expect(wrappedHandlerWithIgnoringSentryErrors(fakeEvent, fakeContext, fakeCallback)).rejects.toThrow(
-        criticalUnhandledError,
-      );
-    });
   });
 
   describe('wrapHandler() on sync handler', () => {
@@ -345,6 +306,21 @@ describe('AWSLambda', () => {
         expect(Sentry.flush).toBeCalled();
       }
     });
+
+    test('should not throw when flush rejects', async () => {
+      const handler: Handler = async () => {
+        // Friendly handler with no errors :)
+        return 'some string';
+      };
+
+      const wrappedHandler = wrapHandler(handler);
+
+      jest.spyOn(Sentry, 'flush').mockImplementationOnce(async () => {
+        throw new Error();
+      });
+
+      await expect(wrappedHandler(fakeEvent, fakeContext, fakeCallback)).resolves.toBe('some string');
+    });
   });
 
   describe('wrapHandler() on async handler with a callback method (aka incorrect usage)', () => {

packages/serverless/test/gcpfunction.test.ts

@@ -148,6 +148,34 @@ describe('GCPFunction', () => {
       expect(Sentry.fakeTransaction.finish).toBeCalled();
       expect(Sentry.flush).toBeCalled();
     });
+
+    test('should not throw when flush rejects', async () => {
+      expect.assertions(2);
+
+      const handler: HttpFunction = async (_req, res) => {
+        res.statusCode = 200;
+        res.end();
+      };
+
+      const wrappedHandler = wrapHttpFunction(handler);
+
+      const request = {
+        method: 'POST',
+        url: '/path?q=query',
+        headers: { host: 'hostname', 'content-type': 'application/json' },
+        body: { foo: 'bar' },
+      } as Request;
+
+      const mockEnd = jest.fn();
+      const response = { end: mockEnd } as unknown as Response;
+
+      jest.spyOn(Sentry, 'flush').mockImplementationOnce(async () => {
+        throw new Error();
+      });
+
+      await expect(wrappedHandler(request, response)).resolves.toBeUndefined();
+      expect(mockEnd).toHaveBeenCalledTimes(1);
+    });
   });
 
   test('wrapHttpFunction request data', async () => {