fix(core): Ensure errors thrown in async cron jobs bubble up (#14182)

d045ba1
adds tests that fail:

https://github.com/getsentry/sentry-javascript/actions/runs/11682775381/job/32530893436?pr=14182#step:14:217


be36a22
adds the fix


Closes: #14163

Author: andreiborza

dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.controller.ts

@@ -77,9 +77,9 @@ export class AppController {
     return { result: await this.appService.testSpanDecoratorSync() };
   }
 
-  @Get('kill-test-cron')
-  async killTestCron() {
-    this.appService.killTestCron();
+  @Get('kill-test-cron/:job')
+  async killTestCron(@Param('job') job: string) {
+    this.appService.killTestCron(job);
   }
 
   @Get('flush')

dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts

@@ -80,8 +80,19 @@ export class AppService {
     console.log('Test cron!');
   }
 
-  async killTestCron() {
-    this.schedulerRegistry.deleteCronJob('test-cron-job');
+  /*
+  Actual cron schedule differs from schedule defined in config because Sentry
+  only supports minute granularity, but we don't want to wait (worst case) a
+  full minute for the tests to finish.
+  */
+  @Cron('*/5 * * * * *', { name: 'test-cron-error' })
+  @SentryCron('test-cron-error-slug', monitorConfig)
+  async testCronError() {
+    throw new Error('Test error from cron job');
+  }
+
+  async killTestCron(job: string) {
+    this.schedulerRegistry.deleteCronJob(job);
   }
 
   use() {

dev-packages/e2e-tests/test-applications/nestjs-basic/tests/cron-decorator.test.ts

@@ -1,13 +1,21 @@
 import { expect, test } from '@playwright/test';
-import { waitForEnvelopeItem } from '@sentry-internal/test-utils';
+import { waitForEnvelopeItem, waitForError } from '@sentry-internal/test-utils';
 
 test('Cron job triggers send of in_progress envelope', async ({ baseURL }) => {
   const inProgressEnvelopePromise = waitForEnvelopeItem('nestjs-basic', envelope => {
-    return envelope[0].type === 'check_in' && envelope[1]['status'] === 'in_progress';
+    return (
+      envelope[0].type === 'check_in' &&
+      envelope[1]['monitor_slug'] === 'test-cron-slug' &&
+      envelope[1]['status'] === 'in_progress'
+    );
   });
 
   const okEnvelopePromise = waitForEnvelopeItem('nestjs-basic', envelope => {
-    return envelope[0].type === 'check_in' && envelope[1]['status'] === 'ok';
+    return (
+      envelope[0].type === 'check_in' &&
+      envelope[1]['monitor_slug'] === 'test-cron-slug' &&
+      envelope[1]['status'] === 'ok'
+    );
   });
 
   const inProgressEnvelope = await inProgressEnvelopePromise;
@@ -51,5 +59,23 @@ test('Cron job triggers send of in_progress envelope', async ({ baseURL }) => {
   );
 
   // kill cron so tests don't get stuck
-  await fetch(`${baseURL}/kill-test-cron`);
+  await fetch(`${baseURL}/kill-test-cron/test-cron-job`);
+});
+
+test('Sends exceptions to Sentry on error in cron job', async ({ baseURL }) => {
+  const errorEventPromise = waitForError('nestjs-basic', event => {
+    return !event.type && event.exception?.values?.[0]?.value === 'Test error from cron job';
+  });
+
+  const errorEvent = await errorEventPromise;
+
+  expect(errorEvent.exception?.values).toHaveLength(1);
+  expect(errorEvent.exception?.values?.[0]?.value).toBe('Test error from cron job');
+  expect(errorEvent.contexts?.trace).toEqual({
+    trace_id: expect.any(String),
+    span_id: expect.any(String),
+  });
+
+  // kill cron so tests don't get stuck
+  await fetch(`${baseURL}/kill-test-cron/test-cron-error`);
 });

packages/core/src/exports.ts

@@ -189,8 +189,9 @@ export function withMonitor<T>(
         () => {
           finishCheckIn('ok');
         },
-        () => {
+        e => {
           finishCheckIn('error');
+          throw e;
         },
       );
     } else {

packages/core/test/lib/base.test.ts

@@ -9,6 +9,7 @@ import {
   lastEventId,
   makeSession,
   setCurrentClient,
+  withMonitor,
 } from '../../src';
 import * as integrationModule from '../../src/integration';
 import { TestClient, getDefaultTestClientOptions } from '../mocks/client';
@@ -2090,4 +2091,48 @@ describe('BaseClient', () => {
       expect(callback).toBeCalledWith(errorEvent, { statusCode: 200 });
     });
   });
+
+  describe('withMonitor', () => {
+    test('handles successful synchronous operations', () => {
+      const result = 'foo';
+      const callback = jest.fn().mockReturnValue(result);
+
+      const returnedResult = withMonitor('test-monitor', callback);
+
+      expect(returnedResult).toBe(result);
+      expect(callback).toHaveBeenCalledTimes(1);
+    });
+
+    test('handles synchronous errors', () => {
+      const error = new Error('Test error');
+      const callback = jest.fn().mockImplementation(() => {
+        throw error;
+      });
+
+      expect(() => withMonitor('test-monitor', callback)).toThrowError(error);
+    });
+
+    test('handles successful asynchronous operations', async () => {
+      const result = 'foo';
+      const callback = jest.fn().mockResolvedValue(result);
+
+      const promise = withMonitor('test-monitor', callback);
+      await expect(promise).resolves.toEqual(result);
+    });
+
+    // This test is skipped because jest keeps retrying ad infinitum
+    // when encountering an unhandled rejections.
+    // We could set "NODE_OPTIONS='--unhandled-rejections=warn' but it
+    // would affect the entire test suite.
+    // Maybe this can be re-enabled when switching to vitest.
+    //
+    // eslint-disable-next-line @sentry-internal/sdk/no-skipped-tests
+    test.skip('handles asynchronous errors', async () => {
+      const error = new Error('Test error');
+      const callback = jest.fn().mockRejectedValue(error);
+
+      const promise = await withMonitor('test-monitor', callback);
+      await expect(promise).rejects.toThrowError(error);
+    });
+  });
 });