fix(node): Remove ambiguity and race conditions when matching local variables to exceptions

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

@@ -1,10 +1,14 @@
 import type { Debugger } from 'node:inspector';
-import type { StackFrame, StackParser } from '@sentry/types';
 
 export type Variables = Record<string, unknown>;
 
 export type RateLimitIncrement = () => void;
 
+/**
+ * The key used to store the local variables on the error object.
+ */
+export const LOCAL_VARIABLES_KEY = '__SENTRY_ERROR_LOCAL_VARIABLES__';
+
 /**
  * Creates a rate limiter that will call the disable callback when the rate limit is reached and the enable callback
  * when a timeout has occurred.
@@ -55,6 +59,7 @@ export type PausedExceptionEvent = Debugger.PausedEventDataType & {
   data: {
     // This contains error.stack
     description: string;
+    objectId?: string;
   };
 };
 
@@ -68,28 +73,6 @@ export function functionNamesMatch(a: string | undefined, b: string | undefined)
   return a === b || (isAnonymous(a) && isAnonymous(b));
 }
 
-/** Creates a unique hash from stack frames */
-export function hashFrames(frames: StackFrame[] | undefined): string | undefined {
-  if (frames === undefined) {
-    return;
-  }
-
-  // Only hash the 10 most recent frames (ie. the last 10)
-  return frames.slice(-10).reduce((acc, frame) => `${acc},${frame.function},${frame.lineno},${frame.colno}`, '');
-}
-
-/**
- * We use the stack parser to create a unique hash from the exception stack trace
- * This is used to lookup vars when the exception passes through the event processor
- */
-export function hashFromStack(stackParser: StackParser, stack: string | undefined): string | undefined {
-  if (stack === undefined) {
-    return undefined;
-  }
-
-  return hashFrames(stackParser(stack, 1));
-}
-
 export interface FrameVariables {
   function: string;
   vars?: Variables;

packages/node/src/integrations/local-variables/inspector.d.ts

@@ -20,6 +20,14 @@ declare module 'node:inspector/promises' {
       method: 'Runtime.getProperties',
       params: Runtime.GetPropertiesParameterType,
     ): Promise<Runtime.GetPropertiesReturnType>;
+    public post(
+      method: 'Runtime.callFunctionOn',
+      params: Runtime.CallFunctionOnParameterType,
+    ): Promise<Runtime.CallFunctionOnReturnType>;
+    public post(
+      method: 'Runtime.releaseObject',
+      params: Runtime.ReleaseObjectParameterType,
+    ): Promise<Runtime.ReleaseObjectReturnType>;
 
     public on(
       event: 'Debugger.paused',

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

@@ -1,11 +1,12 @@
 import { Worker } from 'node:worker_threads';
 import { defineIntegration } from '@sentry/core';
-import type { Event, Exception, IntegrationFn } from '@sentry/types';
-import { LRUMap, logger } from '@sentry/utils';
+import type { Event, EventHint, Exception, IntegrationFn } from '@sentry/types';
+import { logger } from '@sentry/utils';
 
 import type { NodeClient } from '../../sdk/client';
 import type { FrameVariables, LocalVariablesIntegrationOptions, LocalVariablesWorkerArgs } from './common';
-import { functionNamesMatch, hashFrames } from './common';
+import { LOCAL_VARIABLES_KEY } from './common';
+import { functionNamesMatch } from './common';
 
 // This string is a placeholder that gets overwritten with the worker code.
 export const base64WorkerScript = '###LocalVariablesWorkerScript###';
@@ -20,23 +21,7 @@ function log(...args: unknown[]): void {
 export const localVariablesAsyncIntegration = defineIntegration(((
   integrationOptions: LocalVariablesIntegrationOptions = {},
 ) => {
-  const cachedFrames: LRUMap<string, FrameVariables[]> = new LRUMap(20);
-
-  function addLocalVariablesToException(exception: Exception): void {
-    const hash = hashFrames(exception?.stacktrace?.frames);
-
-    if (hash === undefined) {
-      return;
-    }
-
-    // Check if we have local variables for an exception that matches the hash
-    // remove is identical to get but also removes the entry from the cache
-    const cachedFrame = cachedFrames.remove(hash);
-
-    if (cachedFrame === undefined) {
-      return;
-    }
-
+  function addLocalVariablesToException(exception: Exception, localVariables: FrameVariables[]): void {
     // Filter out frames where the function name is `new Promise` since these are in the error.stack frames
     // but do not appear in the debugger call frames
     const frames = (exception.stacktrace?.frames || []).filter(frame => frame.function !== 'new Promise');
@@ -45,32 +30,41 @@ export const localVariablesAsyncIntegration = defineIntegration(((
       // Sentry frames are in reverse order
       const frameIndex = frames.length - i - 1;
 
-      const cachedFrameVariable = cachedFrame[i];
-      const frameVariable = frames[frameIndex];
+      const frameLocalVariables = localVariables[i];
+      const frame = frames[frameIndex];
 
-      if (!frameVariable || !cachedFrameVariable) {
+      if (!frame || !frameLocalVariables) {
         // Drop out if we run out of frames to match up
         break;
       }
 
       if (
         // We need to have vars to add
-        cachedFrameVariable.vars === undefined ||
+        frameLocalVariables.vars === undefined ||
         // We're not interested in frames that are not in_app because the vars are not relevant
-        frameVariable.in_app === false ||
+        frame.in_app === false ||
         // The function names need to match
-        !functionNamesMatch(frameVariable.function, cachedFrameVariable.function)
+        !functionNamesMatch(frame.function, frameLocalVariables.function)
       ) {
         continue;
       }
 
-      frameVariable.vars = cachedFrameVariable.vars;
+      frame.vars = frameLocalVariables.vars;
     }
   }
 
-  function addLocalVariablesToEvent(event: Event): Event {
-    for (const exception of event.exception?.values || []) {
-      addLocalVariablesToException(exception);
+  function addLocalVariablesToEvent(event: Event, hint: EventHint): Event {
+    if (
+      hint.originalException &&
+      typeof hint.originalException === 'object' &&
+      LOCAL_VARIABLES_KEY in hint.originalException &&
+      Array.isArray(hint.originalException[LOCAL_VARIABLES_KEY])
+    ) {
+      for (const exception of event.exception?.values || []) {
+        addLocalVariablesToException(exception, hint.originalException[LOCAL_VARIABLES_KEY]);
+      }
+
+      hint.originalException[LOCAL_VARIABLES_KEY] = undefined;
     }
 
     return event;
@@ -96,10 +90,6 @@ export const localVariablesAsyncIntegration = defineIntegration(((
       worker.terminate();
     });
 
-    worker.on('message', ({ exceptionHash, frames }) => {
-      cachedFrames.set(exceptionHash, frames);
-    });
-
     worker.once('error', (err: Error) => {
       log('Worker error', err);
     });
@@ -139,8 +129,8 @@ export const localVariablesAsyncIntegration = defineIntegration(((
         },
       );
     },
-    processEvent(event: Event): Event {
-      return addLocalVariablesToEvent(event);
+    processEvent(event: Event, hint: EventHint): Event {
+      return addLocalVariablesToEvent(event, hint);
     },
   };
 }) satisfies IntegrationFn);

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

@@ -1,6 +1,6 @@
 import type { Debugger, InspectorNotification, Runtime, Session } from 'node:inspector';
 import { defineIntegration, getClient } from '@sentry/core';
-import type { Event, Exception, IntegrationFn, StackParser } from '@sentry/types';
+import type { Event, Exception, IntegrationFn, StackFrame, StackParser } from '@sentry/types';
 import { LRUMap, logger } from '@sentry/utils';
 
 import { NODE_MAJOR } from '../../nodeVersion';
@@ -12,7 +12,29 @@ import type {
   RateLimitIncrement,
   Variables,
 } from './common';
-import { createRateLimiter, functionNamesMatch, hashFrames, hashFromStack } from './common';
+import { createRateLimiter, functionNamesMatch } from './common';
+
+/** Creates a unique hash from stack frames */
+export function hashFrames(frames: StackFrame[] | undefined): string | undefined {
+  if (frames === undefined) {
+    return;
+  }
+
+  // Only hash the 10 most recent frames (ie. the last 10)
+  return frames.slice(-10).reduce((acc, frame) => `${acc},${frame.function},${frame.lineno},${frame.colno}`, '');
+}
+
+/**
+ * We use the stack parser to create a unique hash from the exception stack trace
+ * This is used to lookup vars when the exception passes through the event processor
+ */
+export function hashFromStack(stackParser: StackParser, stack: string | undefined): string | undefined {
+  if (stack === undefined) {
+    return undefined;
+  }
+
+  return hashFrames(stackParser(stack, 1));
+}
 
 type OnPauseEvent = InspectorNotification<Debugger.PausedEventDataType>;
 export interface DebugSession {

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

@@ -1,16 +1,12 @@
 import type { Debugger, InspectorNotification, Runtime } from 'node:inspector';
 import { Session } from 'node:inspector/promises';
-import { parentPort, workerData } from 'node:worker_threads';
-import type { StackParser } from '@sentry/types';
-import { createStackParser, nodeStackLineParser } from '@sentry/utils';
-import { createGetModuleFromFilename } from '../../utils/module';
+import { workerData } from 'node:worker_threads';
 import type { LocalVariablesWorkerArgs, PausedExceptionEvent, RateLimitIncrement, Variables } from './common';
-import { createRateLimiter, hashFromStack } from './common';
+import { LOCAL_VARIABLES_KEY } from './common';
+import { createRateLimiter } from './common';
 
 const options: LocalVariablesWorkerArgs = workerData;
 
-const stackParser = createStackParser(nodeStackLineParser(createGetModuleFromFilename(options.basePath)));
-
 function log(...args: unknown[]): void {
   if (options.debug) {
     // eslint-disable-next-line no-console
@@ -88,19 +84,15 @@ let rateLimiter: RateLimitIncrement | undefined;
 
 async function handlePaused(
   session: Session,
-  stackParser: StackParser,
-  { reason, data, callFrames }: PausedExceptionEvent,
-): Promise<void> {
+  { reason, data: { objectId }, callFrames }: PausedExceptionEvent,
+): Promise<string | undefined> {
   if (reason !== 'exception' && reason !== 'promiseRejection') {
     return;
   }
 
   rateLimiter?.();
 
-  // data.description contains the original error.stack
-  const exceptionHash = hashFromStack(stackParser, data?.description);
-
-  if (exceptionHash == undefined) {
+  if (objectId == undefined) {
     return;
   }
 
@@ -123,7 +115,15 @@ async function handlePaused(
     }
   }
 
-  parentPort?.postMessage({ exceptionHash, frames });
+  // 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)}; }`,
+    silent: true,
+    objectId,
+  });
+
+  return objectId;
 }
 
 async function startDebugger(): Promise<void> {
@@ -141,13 +141,23 @@ async function startDebugger(): Promise<void> {
   session.on('Debugger.paused', (event: InspectorNotification<Debugger.PausedEventDataType>) => {
     isPaused = true;
 
-    handlePaused(session, stackParser, event.params as PausedExceptionEvent).then(
-      () => {
+    handlePaused(session, event.params as PausedExceptionEvent).then(
+      async objectId => {
         // After the pause work is complete, resume execution!
-        return isPaused ? session.post('Debugger.resume') : Promise.resolve();
+        if (isPaused) {
+          await session.post('Debugger.resume');
+        }
+
+        if (objectId) {
+          // The object must be released after the debugger has resumed or we get a memory leak.
+          // For node v20, setImmediate is enough here but for v22 a longer delay is required
+          setTimeout(async () => {
+            await session.post('Runtime.releaseObject', { objectId });
+          }, 1_000);
+        }
       },
       _ => {
-        // ignore
+        // ignore any errors
       },
     );
   });