From bcd2a175e669d1435ef38c449805921b5d7c55d0 Mon Sep 17 00:00:00 2001
From: Tim Fish <tim@timfish.uk>
Date: Fri, 13 Sep 2024 10:42:18 +0200
Subject: [PATCH] fix(node): Don't overwrite local variables for re-thrown
 errors (#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.
---
 .../LocalVariables/local-variables-rethrow.js | 45 +++++++++++++++++++
 .../suites/public-api/LocalVariables/test.ts  |  8 ++++
 .../integrations/local-variables/worker.ts    | 10 +++--
 3 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100644 dev-packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-rethrow.js

diff --git a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-rethrow.js b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-rethrow.js
new file mode 100644
index 000000000000..744cb747c70c
--- /dev/null
+++ b/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);
diff --git a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts
index 2640ecf94461..bf01ed999708 100644
--- a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts
+++ b/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);
   });
diff --git a/packages/node/src/integrations/local-variables/worker.ts b/packages/node/src/integrations/local-variables/worker.ts
index eb4fee87947c..f0b3e20ce9b2 100644
--- a/packages/node/src/integrations/local-variables/worker.ts
+++ b/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');
+        }
       },
     );
   });