Fix/nestjs cron issues (#16792)

Nestjs sync task has the same issue as async tasks - they don't care
about exceptions because nestjs `@Cron()` decorator automatically wraps
it to `try-catch` block and logs an error. That's why we should capture
exception in our `@SentryCron()` decorator.
Fixes also #16749

- [ ] If you've added code that should be tested, please add tests.
- [x] Ensure your code lints and the test suite passes (`yarn lint`) &
(`yarn test`).

---------

Co-authored-by: cod1k <cod1k@centro.team>
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>

Author: 3 people

dev-packages/e2e-tests/run.ts

@@ -66,6 +66,7 @@ async function run(): Promise<void> {
     }
 
     await asyncExec('pnpm clean:test-applications', { env, cwd: __dirname });
+    await asyncExec('pnpm cache delete "@sentry/*"', { env, cwd: __dirname });
 
     const testAppPaths = appName ? [appName.trim()] : globSync('*', { cwd: `${__dirname}/test-applications/` });
 

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

@@ -85,10 +85,21 @@ export class AppService {
   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)
+  @Cron('*/5 * * * * *', { name: 'test-async-cron-error' })
+  @SentryCron('test-async-cron-error-slug', monitorConfig)
   async testCronError() {
-    throw new Error('Test error from cron job');
+    throw new Error('Test error from cron async 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-sync-cron-error' })
+  @SentryCron('test-sync-cron-error-slug', monitorConfig)
+  testSyncCronError() {
+    throw new Error('Test error from cron sync job');
   }
 
   async killTestCron(job: string) {

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

@@ -62,20 +62,36 @@ test('Cron job triggers send of in_progress envelope', async ({ baseURL }) => {
   await fetch(`${baseURL}/kill-test-cron/test-cron-job`);
 });
 
-test('Sends exceptions to Sentry on error in cron job', async ({ baseURL }) => {
+test('Sends exceptions to Sentry on error in async cron job', async ({ baseURL }) => {
   const errorEventPromise = waitForError('nestjs-basic', event => {
-    return !event.type && event.exception?.values?.[0]?.value === 'Test error from cron job';
+    return !event.type && event.exception?.values?.[0]?.value === 'Test error from cron async 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.stringMatching(/[a-f0-9]{32}/),
     span_id: expect.stringMatching(/[a-f0-9]{16}/),
   });
 
   // kill cron so tests don't get stuck
-  await fetch(`${baseURL}/kill-test-cron/test-cron-error`);
+  await fetch(`${baseURL}/kill-test-cron/test-async-cron-error`);
+});
+
+test('Sends exceptions to Sentry on error in sync cron job', async ({ baseURL }) => {
+  const errorEventPromise = waitForError('nestjs-basic', event => {
+    return !event.type && event.exception?.values?.[0]?.value === 'Test error from cron sync job';
+  });
+
+  const errorEvent = await errorEventPromise;
+
+  expect(errorEvent.exception?.values).toHaveLength(1);
+  expect(errorEvent.contexts?.trace).toEqual({
+    trace_id: expect.stringMatching(/[a-f0-9]{32}/),
+    span_id: expect.stringMatching(/[a-f0-9]{16}/),
+  });
+
+  // kill cron so tests don't get stuck
+  await fetch(`${baseURL}/kill-test-cron/test-sync-cron-error`);
 });

dev-packages/e2e-tests/test-applications/node-express/src/app.ts

@@ -29,6 +29,18 @@ const port = 3030;
 
 app.use(mcpRouter);
 
+app.get('/crash-in-with-monitor/:id', async (req, res) => {
+  try {
+    await Sentry.withMonitor('express-crash', async () => {
+      throw new Error(`This is an exception withMonitor: ${req.params.id}`);
+    });
+    res.sendStatus(200);
+  } catch (error: any) {
+    res.status(500);
+    res.send({ message: error.message, pid: process.pid });
+  }
+});
+
 app.get('/test-success', function (req, res) {
   res.send({ version: 'v1' });
 });

dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts

@@ -37,6 +37,17 @@ test('Should record caught exceptions with local variable', async ({ baseURL })
 
   const errorEvent = await errorEventPromise;
 
-  const frames = errorEvent.exception?.values?.[0].stacktrace?.frames;
-  expect(frames?.[frames.length - 1].vars?.randomVariableToRecord).toBeDefined();
+  const frames = errorEvent.exception?.values?.[0]?.stacktrace?.frames;
+  expect(frames?.[frames.length - 1]?.vars?.randomVariableToRecord).toBeDefined();
+});
+
+test('To not crash app from withMonitor', async ({ baseURL }) => {
+  const doRequest = async (id: number) => {
+    const response = await fetch(`${baseURL}/crash-in-with-monitor/${id}`)
+    return response.json();
+  }
+  const [response1, response2] = await Promise.all([doRequest(1), doRequest(2)])
+  expect(response1.message).toBe('This is an exception withMonitor: 1')
+  expect(response2.message).toBe('This is an exception withMonitor: 2')
+  expect(response1.pid).toBe(response2.pid) //Just to double-check, TBS
 });

packages/core/src/exports.ts

@@ -150,6 +150,7 @@ export function captureCheckIn(checkIn: CheckIn, upsertMonitorConfig?: MonitorCo
  * Wraps a callback with a cron monitor check in. The check in will be sent to Sentry when the callback finishes.
  *
  * @param monitorSlug The distinct slug of the monitor.
+ * @param callback Callback to be monitored
  * @param upsertMonitorConfig An optional object that describes a monitor config. Use this if you want
  * to create a monitor automatically when sending a check in.
  */
@@ -175,18 +176,18 @@ export function withMonitor<T>(
     }
 
     if (isThenable(maybePromiseResult)) {
-      Promise.resolve(maybePromiseResult).then(
-        () => {
+      return maybePromiseResult.then(
+        r => {
           finishCheckIn('ok');
+          return r;
         },
         e => {
           finishCheckIn('error');
           throw e;
         },
-      );
-    } else {
-      finishCheckIn('ok');
+      ) as T;
     }
+    finishCheckIn('ok');
 
     return maybePromiseResult;
   });

packages/nestjs/src/decorators.ts

@@ -1,5 +1,5 @@
 import type { MonitorConfig } from '@sentry/core';
-import { captureException } from '@sentry/core';
+import { captureException, isThenable } from '@sentry/core';
 import * as Sentry from '@sentry/node';
 import { startSpan } from '@sentry/node';
 import { isExpectedError } from './helpers';
@@ -15,7 +15,20 @@ export const SentryCron = (monitorSlug: string, monitorConfig?: MonitorConfig):
       return Sentry.withMonitor(
         monitorSlug,
         () => {
-          return originalMethod.apply(this, args);
+          let result;
+          try {
+            result = originalMethod.apply(this, args);
+          } catch (e) {
+            captureException(e);
+            throw e;
+          }
+          if (isThenable(result)) {
+            return result.then(undefined, e => {
+              captureException(e);
+              throw e;
+            });
+          }
+          return result;
         },
         monitorConfig,
       );