From a23e18b899290ce8bf8d3d4b27e80de2b6bb207e Mon Sep 17 00:00:00 2001 From: cod1k Date: Wed, 2 Jul 2025 18:49:16 +0300 Subject: [PATCH 1/7] Clear Sentry cache before running E2E tests Add a command to delete the Sentry-related cache during the E2E test setup process. This ensures a clean testing environment and prevents potential issues caused by stale cache. This is why running tests locally after any change might be wrong. Not sure this is the best solution, or no. But, at least results are constant now --- dev-packages/e2e-tests/run.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/e2e-tests/run.ts b/dev-packages/e2e-tests/run.ts index e8901eede1b9..44f0bc06dca7 100644 --- a/dev-packages/e2e-tests/run.ts +++ b/dev-packages/e2e-tests/run.ts @@ -66,6 +66,7 @@ async function run(): Promise { } 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/` }); From 74ea94a7445c8d02c2f950c507774d81e452c69b Mon Sep 17 00:00:00 2001 From: cod1k Date: Wed, 2 Jul 2025 18:50:50 +0300 Subject: [PATCH 2/7] Add sync cron error handling and associated tests Introduced a new `testSyncCronError` method to handle sync cron job errors and updated the async method for clarity. Updated tests to cover both async and sync cron scenarios, ensuring proper error handling and reporting to Sentry. --- .../nestjs-basic/src/app.service.ts | 17 ++++++++++--- .../nestjs-basic/tests/cron-decorator.test.ts | 24 +++++++++++++++---- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts index 242b4c778a0e..a9f89152d56d 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts +++ b/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) { diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/cron-decorator.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/cron-decorator.test.ts index 7896603b3bd9..c193a94911c1 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/cron-decorator.test.ts +++ b/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`); }); From 6828d30b22e2eeb17d9d9e3f73a5e776f21243d2 Mon Sep 17 00:00:00 2001 From: cod1k Date: Wed, 2 Jul 2025 18:53:03 +0300 Subject: [PATCH 3/7] Handle promise rejection and synchronous errors in monitoring Added error capturing for both synchronous and asynchronous exceptions in monitored methods. Incorporated `isThenable` checks to handle promises properly, ensuring robust error tracking with Sentry in both cases. --- packages/core/src/exports.ts | 7 ++++--- packages/nestjs/src/decorators.ts | 17 +++++++++++++++-- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/core/src/exports.ts b/packages/core/src/exports.ts index 92e4d09d4d81..3261102746e8 100644 --- a/packages/core/src/exports.ts +++ b/packages/core/src/exports.ts @@ -175,15 +175,16 @@ export function withMonitor( } if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then( - () => { + return maybePromiseResult.then( + r => { finishCheckIn('ok'); + return r; }, e => { finishCheckIn('error'); throw e; }, - ); + ) as T; } else { finishCheckIn('ok'); } diff --git a/packages/nestjs/src/decorators.ts b/packages/nestjs/src/decorators.ts index b11b69046c8c..9ac7315dabd8 100644 --- a/packages/nestjs/src/decorators.ts +++ b/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, ); From 2346b827baa246c3749bf9b03588907cc17ead50 Mon Sep 17 00:00:00 2001 From: cod1k Date: Thu, 3 Jul 2025 08:12:29 +0300 Subject: [PATCH 4/7] Refactor cron monitor Added JSDoc callback refference Removed redundant else in if-else block --- packages/core/src/exports.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/exports.ts b/packages/core/src/exports.ts index 3261102746e8..5d36ffedf43f 100644 --- a/packages/core/src/exports.ts +++ b/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. */ @@ -185,9 +186,8 @@ export function withMonitor( throw e; }, ) as T; - } else { - finishCheckIn('ok'); } + finishCheckIn('ok'); return maybePromiseResult; }); From fda28e8fb05f595abc2f315cd9a94f62b7a54327 Mon Sep 17 00:00:00 2001 From: cod1k Date: Thu, 3 Jul 2025 08:14:11 +0300 Subject: [PATCH 5/7] Fix possible undefined access in error test assertions Updated the test to safely handle potential undefined values in the stacktrace frames. This prevents runtime errors during test execution and ensures robust validation of error handling logic. --- .../test-applications/node-express/tests/errors.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts index bf0c5c5fb6b2..1c502d30eead 100644 --- a/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts @@ -37,6 +37,6 @@ 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(); }); From a5c68110740af21c8e4012af1a03013c940b31f7 Mon Sep 17 00:00:00 2001 From: cod1k Date: Thu, 3 Jul 2025 10:37:29 +0300 Subject: [PATCH 6/7] Add endpoint and tests for withMonitor error handling to cover issue:16749 Introduce a new endpoint `/crash-in-with-monitor/:id` that demonstrates error handling using Sentry's `withMonitor` API. Added tests to ensure the app gracefully handles exceptions and validates consistent PID behavior during monitoring. --- .../test-applications/node-express/src/app.ts | 12 ++++++++++++ .../node-express/tests/errors.test.ts | 11 +++++++++++ 2 files changed, 23 insertions(+) diff --git a/dev-packages/e2e-tests/test-applications/node-express/src/app.ts b/dev-packages/e2e-tests/test-applications/node-express/src/app.ts index 76a02ea2c255..35b21a97b9aa 100644 --- a/dev-packages/e2e-tests/test-applications/node-express/src/app.ts +++ b/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' }); }); diff --git a/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts index 1c502d30eead..27f2fa53ca32 100644 --- a/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts @@ -40,3 +40,14 @@ test('Should record caught exceptions with local variable', async ({ baseURL }) const frames = errorEvent.exception?.values?.[0]?.stacktrace?.frames; expect(frames?.[frames.length - 1]?.vars?.randomVariableToRecord).toBeDefined(); }); + +test('To not crush 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 +}); From 383ae525d1a03821f6689d7eac87ee753a5e5144 Mon Sep 17 00:00:00 2001 From: Andrei <168741329+andreiborza@users.noreply.github.com> Date: Thu, 3 Jul 2025 10:55:42 +0200 Subject: [PATCH 7/7] Update dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts --- .../test-applications/node-express/tests/errors.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts index 27f2fa53ca32..a4faaf137eb7 100644 --- a/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts @@ -41,7 +41,7 @@ test('Should record caught exceptions with local variable', async ({ baseURL }) expect(frames?.[frames.length - 1]?.vars?.randomVariableToRecord).toBeDefined(); }); -test('To not crush app from withMonitor', async ({ baseURL }) => { +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();