Skip to content

Fix/nestjs cron issues #16792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 3, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: Is this necessary? Did you run into issues without it? I think we should not add this in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest - no, it is not necessary to be here in this PR. But yes, you are going to run into an issue without this. The issue is that if you are doing any changes to any package and want to run e2e tests locally with yarn workspace @sentry-internal/e2e-tests test:run app-name your very first test will be executed as expected. But any other changes are not going to be reflected by pnpm install because of the cache.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also rather get rid of this and if necessary address this in a different PR.


const testAppPaths = appName ? [appName.trim()] : globSync('*', { cwd: `${__dirname}/test-applications/` });

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`);
});
7 changes: 4 additions & 3 deletions packages/core/src/exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,15 +175,16 @@ export function withMonitor<T>(
}

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');
}
Expand Down
17 changes: 15 additions & 2 deletions packages/nestjs/src/decorators.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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,
);
Expand Down
Loading