Skip to content

Conversation

@0xbad0c0d3
Copy link
Contributor

@0xbad0c0d3 0xbad0c0d3 commented Jun 27, 2025

Throwing an error inside catch block will produce unhandled exception and, as a result - crash.
fixes #16749

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

Replaced the error callback to streamline behavior by removing the unnecessary error throwing. This ensures a cleaner flow while maintaining intended functionality.
@andreiborza
Copy link
Member

Hi, unfortunately this ends up swallowing errors in cron jobs. See #14163.

We need to figure out a different way.

@0xbad0c0d3
Copy link
Contributor Author

yarn build:tarball
yarn workspace @sentry-internal/e2e-tests test:run nestjs-11 //23/23 passed
yarn workspace @sentry-internal/e2e-tests test:run nestjs-basic //23/23 passed
yarn workspace @sentry-internal/e2e-tests test:run nestjs-fastify //24/24 passed

Something seems wrong with the tests

@andreiborza
Copy link
Member

Hmmm.. true, nestjs-basic should not pass 🤔

@andreiborza andreiborza reopened this Jun 30, 2025
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Error Handling Broken by Silent Error Swallowing

The error callback now silently swallows errors by removing the error parameter and the throw e; statement. This prevents error propagation, breaking error handling patterns and making debugging difficult. It leads to silent failures, particularly impacting error handling in cron jobs as confirmed by PR discussions.

packages/core/src/exports.ts#L181-L184

},
() => {
finishCheckIn('error');
},

Fix in Cursor


Was this report helpful? Give feedback by reacting with 👍 or 👎

@andreiborza
Copy link
Member

@0xbad0c0d3
Copy link
Contributor Author

Okay, so it does break the test correctly: https://github.com/getsentry/sentry-javascript/actions/runs/15971148094/job/45042700104?pr=16757#step:14:123

Yes, for some reason it does break in CI but it does not break other ways... But maybe I am doing something wrong?

@andreiborza
Copy link
Member

Mhm, hard to tell. Either way, we need to think of some other way. I'll try to bring this up today in team discussions and we'll see how to handle it.

@0xbad0c0d3 0xbad0c0d3 deleted the fix/16749 branch July 3, 2025 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

withMonitor causes server to crash when callback throws an error

2 participants