fix(node): Don't overwrite local variables for re-thrown errors (getsentry#13644)

Another way to fix this issue would be to check via the debugger if the property exists already on the error object and bail early before we have local variables. However, this would add an extra round trip call via the debugger for every error. Since re-thrown errors are far less likely, I decided instead to just not overwrite existing local variables. This PR also adds a `Debugger.resume` call in the catch case to ensure that they debugger will always resume even if we get errors while debugging. It's worth noting that this only fixes the issue in Node v20+ where we use the async debugging interface.

Author: timfish

dev-packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-rethrow.js

@@ -0,0 +1,45 @@
+/* eslint-disable no-unused-vars */
+const Sentry = require('@sentry/node');
+const { loggingTransport } = require('@sentry-internal/node-integration-tests');
+
+Sentry.init({
+ dsn: 'https://public@dsn.ingest.sentry.io/1337',
+ includeLocalVariables: true,
+ transport: loggingTransport,
+});
+
+class Some {
+ two(name) {
+ throw new Error('Enough!');
+ }
+}
+
+function one(name) {
+ const arr = [1, '2', null];
+ const obj = {
+ name,
+ num: 5,
+ };
+ const bool = false;
+ const num = 0;
+ const str = '';
+ const something = undefined;
+ const somethingElse = null;
+
+ const ty = new Some();
+
+ ty.two(name);
+}
+
+setTimeout(() => {
+ try {
+ try {
+ one('some name');
+ } catch (e) {
+ const more = 'here';
+ throw e;
+ }
+ } catch (e) {
+ Sentry.captureException(e);
+ }
+}, 1000);

dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts

@@ -78,6 +78,14 @@ conditionalTest({ min: 18 })('LocalVariables integration', () => {
  });
  });
 
+ conditionalTest({ min: 20 })('Node v20+', () => {
+ test('Should retain original local variables when error is re-thrown', done => {
+ createRunner(__dirname, 'local-variables-rethrow.js')
+ .expect({ event: EXPECTED_LOCAL_VARIABLES_EVENT })
+ .start(done);
+ });
+ });
+
  test('Includes local variables for caught exceptions when enabled', done => {
  createRunner(__dirname, 'local-variables-caught.js').expect({ event: EXPECTED_LOCAL_VARIABLES_EVENT }).start(done);
  });

packages/node/src/integrations/local-variables/worker.ts

@@ -118,7 +118,9 @@ async function handlePaused(
  // We write the local variables to a property on the error object. These can be read by the integration as the error
  // event pass through the SDK event pipeline
  await session.post('Runtime.callFunctionOn', {
- functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(frames)}; }`,
+ functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = this.${LOCAL_VARIABLES_KEY} || ${JSON.stringify(
+ frames,
+ )}; }`,
  silent: true,
  objectId,
  });
@@ -156,8 +158,10 @@ async function startDebugger(): Promise<void> {
  }, 1_000);
  }
  },
- _ => {
- // ignore any errors
+ async _ => {
+ if (isPaused) {
+ await session.post('Debugger.resume');
+ }
  },
  );
  });