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,10 @@ 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 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,39 @@ 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]);
+      }
     }
 
     return event;
@@ -96,10 +88,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 +127,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,7 +1,7 @@
 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 { LRUMap, logger } from '@sentry/utils';
+import type { Event, EventHint, Exception, IntegrationFn } from '@sentry/types';
+import { logger } from '@sentry/utils';
 
 import { NODE_MAJOR } from '../../nodeVersion';
 import type { NodeClient } from '../../sdk/client';
@@ -12,7 +12,8 @@
   RateLimitIncrement,
   Variables,
 } from './common';
-import { createRateLimiter, functionNamesMatch, hashFrames, hashFromStack } from './common';
+import { LOCAL_VARIABLES_KEY } from './common';
+import { createRateLimiter, functionNamesMatch } from './common';
 
 type OnPauseEvent = InspectorNotification<Debugger.PausedEventDataType>;
 export interface DebugSession {
@@ -22,6 +23,8 @@
   setPauseOnExceptions(captureAll: boolean): void;
   /** Gets local variables for an objectId */
   getLocalVariables(objectId: string, callback: (vars: Variables) => void): void;
+  /** Sets the local variables on the error object for later retrieval */
+  setLocalVarsOnError(objectId: string, localVariables: FrameVariables[]): void;
 }
 
 type Next<T> = (result: T) => void;
@@ -128,6 +131,13 @@
     });
   }
 
+  public setLocalVarsOnError(objectId: string, localVariables: FrameVariables[]): void {
+    this._session.post('Runtime.callFunctionOn', {
+      functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(localVariables)}; }`,
+      objectId,
+    });
+  }
+
   /**
    * Gets all the PropertyDescriptors of an object
    */
@@ -209,25 +219,10 @@
   options: LocalVariablesIntegrationOptions = {},
   sessionOverride?: DebugSession,
 ) => {
-  const cachedFrames: LRUMap<string, FrameVariables[]> = new LRUMap(20);
   let rateLimiter: RateLimitIncrement | undefined;
   let shouldProcessEvent = false;
 
-  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');
@@ -236,32 +231,39 @@
       // 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];
 
       // Drop out if we run out of frames to match up
-      if (!frameVariable || !cachedFrameVariable) {
+      if (!frame || !frameLocalVariables) {
         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]);
+      }
     }
 
     return event;
@@ -289,7 +291,6 @@
       AsyncSession.create(sessionOverride).then(
         session => {
           function handlePaused(
-            stackParser: StackParser,
             { params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
             complete: () => void,
           ): void {
@@ -300,16 +301,15 @@
 
             rateLimiter?.();
 
-            // data.description contains the original error.stack
-            const exceptionHash = hashFromStack(stackParser, data?.description);
+            const objectId = data?.objectId;
 
-            if (exceptionHash == undefined) {
+            if (objectId == undefined) {
               complete();
               return;
             }
 
             const { add, next } = createCallbackList<FrameVariables[]>(frames => {
-              cachedFrames.set(exceptionHash, frames);
+              session.setLocalVarsOnError(objectId, frames);
               complete();
             });
 
@@ -347,8 +347,7 @@
           const captureAll = options.captureAllExceptions !== false;
 
           session.configureAndConnect(
-            (ev, complete) =>
-              handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
+            (ev, complete) => handlePaused(ev as InspectorNotification<PausedExceptionEvent>, complete),
             captureAll,
           );
 
@@ -377,20 +376,13 @@
         },
       );
     },
-    processEvent(event: Event): Event {
+    processEvent(event: Event, hint: EventHint): Event {
       if (shouldProcessEvent) {
-        return addLocalVariablesToEvent(event);
+        return addLocalVariablesToEvent(event, hint);
       }
 
       return event;
     },
-    // These are entirely for testing
-    _getCachedFramesCount(): number {
-      return cachedFrames.size;
-    },
-    _getFirstCachedFrame(): FrameVariables[] | undefined {
-      return cachedFrames.values()[0];
-    },
   };
 }) satisfies IntegrationFn;