The `onUnhandledRejectionIntegration` does not exit gracefully in strict mode

[WesCossick]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.5.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

const sentryConfig: Sentry.NodeOptions = {
	dsn: process.env.SENTRY_DSN,
	environment: process.env.ENVIRONMENT,
	integrations: [
		Sentry.onUnhandledRejectionIntegration({
			mode: 'strict',
		}),
	],
};

Sentry.init(sentryConfig);

Steps to Reproduce

Trigger an unhandled promise rejection while using the onUnhandledRejectionIntegration in mode: 'strict'.

Expected Result

For the app to exit gracefully.

Actual Result

When Sentry's onUnhandledRejectionIntegration catches an unhandled promise rejection in strict mode, it is currently designed to exit immediately:

sentry-javascript/packages/node/src/utils/errorhandling.ts

Line 34 in 005f40d

global.process.exit(1);

This does not allow the app to shut things down and exit gracefully, because it bypasses any SIGTERM listeners. The code should really be designed like so:

if (process.listenerCount('SIGTERM')) {
  global.process.kill(process.pid, 'SIGTERM');
} else {
  global.process.exit(1);
}

This will detect if there are any SIGTERM listeners, and if so, rely on those to handle shut down and exit.

[WesCossick]

Note, a temporary fix for anyone else encountering this would be to not use mode: 'strict' and instead add your own unhandledRejection listener after calling Sentry.init():

process.on('unhandledRejection', () => {
  if (process.listenerCount('SIGTERM')) {
    process.kill(process.pid, 'SIGTERM');
  } else {
    process.exit(1);
  }
});

[lforst]

Hi, thanks for writing in. Would you mind walking me through your thought process?

This is how I currently look at this issue: You manually set mode: 'strict' meaning you told the SDK to exit the process within its unhandledRejection handler. You also mentioned SIGTERM listeners not being called. As far as I understand, SIGTERM listeners are not invoked by Node when it crashes through unhandled promise rejections. I verified with the following snippet:

process.once('SIGTERM', () => {
  console.log('sigterm received'); // not called
});

setTimeout(() => {
  console.log('healthy'); // not called
}, 4000);

Promise.reject();

As far as I can tell, you simply shouldn't set the mode to 'strict' if you don't want the SDK to exit, and instead use 'warn' (which is the default) or 'none'.

[WesCossick]

I can see your reasoning, and while I'd normally say adhering to the behavior of a platform like Node.js is a good idea, when it comes to exiting gracefully and signal handling, Node.js isn't the best example; there are a number of flaws with how Node.js handles signal listeners and graceful shutdown.

Essentially, my reasoning is that if a program registers SIGTERM listeners, then it wants to clean things up when it's time to exit. So using process.kill(process.pid, 'SIGTERM') to shut down is the way to trigger a graceful exit. While it deviates from Node.js's behavior, I'd argue that this is a better exiting strategy because it allows for cleanup tasks to be executed.

The workaround I suggested avoids using strict, but it introduces slightly extra complexity that wouldn't be necessary if Sentry's SDK terminated more gracefully.

[lforst]

Thanks for elaborating. I think we will not change this behaviour. I don't intend to dismiss your point but there is some history 😄 From my last research on whether we can be more accurate here, I don't think there is a way to exactly match Node.js's exit behaviour when intending run IO (in our case to flush data). We certainly do not want to "improve" Node.js' behaviours - that is up to our users.

I'll close this to keep our issue stream clean but feel free to reach out if you have any questions or concerns!