Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/node-core/src/integrations/local-variables/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ export interface LocalVariablesIntegrationOptions {
* Maximum number of exceptions to capture local variables for per second before rate limiting is triggered.
*/
maxExceptionsPerSecond?: number;
/**
* When true, local variables will be captured for all frames, including those that are not in_app.
*
* Defaults to `false`.
*/
includeOutOfAppFrames?: boolean;
}

export interface LocalVariablesWorkerArgs extends LocalVariablesIntegrationOptions {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ export const localVariablesAsyncIntegration = defineIntegration(((
if (
// We need to have vars to add
frameLocalVariables.vars === undefined ||
// We're not interested in frames that are not in_app because the vars are not relevant
frame.in_app === false ||
// Only skip out-of-app frames if includeOutOfAppFrames is not true
(frame.in_app === false && integrationOptions.includeOutOfAppFrames !== true) ||
// The function names need to match
!functionNamesMatch(frame.function, frameLocalVariables.function)
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {
Variables,
} from './common';
import { createRateLimiter, functionNamesMatch } from './common';
import { localVariablesTestHelperMethods } from './test-helpers';

/** Creates a unique hash from stack frames */
export function hashFrames(frames: StackFrame[] | undefined): string | undefined {
Expand Down Expand Up @@ -268,8 +269,8 @@ const _localVariablesSyncIntegration = ((
if (
// We need to have vars to add
cachedFrameVariable.vars === undefined ||
// We're not interested in frames that are not in_app because the vars are not relevant
frameVariable.in_app === false ||
// Only skip out-of-app frames if includeOutOfAppFrames is not true
(!frameVariable.in_app && !options.includeOutOfAppFrames) ||
// The function names need to match
!functionNamesMatch(frameVariable.function, cachedFrameVariable.function)
) {
Expand All @@ -288,6 +289,8 @@ const _localVariablesSyncIntegration = ((
return event;
}

const testHelperMethods = localVariablesTestHelperMethods(cachedFrames);

return {
name: INTEGRATION_NAME,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential bug: Changing setupOnce to async violates the integration interface contract. The framework calls it synchronously, reintroducing a race condition where events are silently ignored during initialization.
  • Description: The Integration interface defines setupOnce as a synchronous method, but it was changed to be async. The integration framework calls this method without await, causing the initialization logic inside to run in the background. While this async operation is pending, a shouldProcessEvent flag remains false. Consequently, any events received by processEvent during this startup window are silently ignored instead of being processed. This reintroduces the exact race condition the change was intended to fix, causing the integration to silently fail to capture variables for an indeterminate period.

  • Suggested fix: Revert setupOnce to a synchronous function to adhere to the interface contract. If asynchronous initialization is required, consider an alternative approach, such as queuing events within the integration until the async setup is complete, rather than making the setupOnce hook itself asynchronous.
    severity: 0.85, confidence: 0.98

Did we get this right? 👍 / 👎 to inform future reviews.

async setupOnce() {
Expand All @@ -312,96 +315,95 @@ const _localVariablesSyncIntegration = ((
return;
}

AsyncSession.create(sessionOverride).then(
session => {
function handlePaused(
stackParser: StackParser,
{ params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
complete: () => void,
): void {
if (reason !== 'exception' && reason !== 'promiseRejection') {
complete();
return;
}

rateLimiter?.();

// data.description contains the original error.stack
const exceptionHash = hashFromStack(stackParser, data.description);

if (exceptionHash == undefined) {
complete();
return;
}

const { add, next } = createCallbackList<FrameVariables[]>(frames => {
cachedFrames.set(exceptionHash, frames);
complete();
});
try {
const session = await AsyncSession.create(sessionOverride);

const handlePaused = (
stackParser: StackParser,
{ params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
complete: () => void,
): void => {
if (reason !== 'exception' && reason !== 'promiseRejection') {
complete();
return;
}

// Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack
// For this reason we only attempt to get local variables for the first 5 frames
for (let i = 0; i < Math.min(callFrames.length, 5); i++) {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const { scopeChain, functionName, this: obj } = callFrames[i]!;
rateLimiter?.();

const localScope = scopeChain.find(scope => scope.type === 'local');
// data.description contains the original error.stack
const exceptionHash = hashFromStack(stackParser, data.description);

// obj.className is undefined in ESM modules
const fn =
obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`;
if (exceptionHash == undefined) {
complete();
return;
}

if (localScope?.object.objectId === undefined) {
add(frames => {
frames[i] = { function: fn };
const { add, next } = createCallbackList<FrameVariables[]>(frames => {
cachedFrames.set(exceptionHash, frames);
complete();
});

// Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack
// For this reason we only attempt to get local variables for the first 5 frames
for (let i = 0; i < Math.min(callFrames.length, 5); i++) {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const { scopeChain, functionName, this: obj } = callFrames[i]!;

const localScope = scopeChain.find(scope => scope.type === 'local');

// obj.className is undefined in ESM modules
const fn =
obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`;

if (localScope?.object.objectId === undefined) {
add(frames => {
frames[i] = { function: fn };
next(frames);
});
} else {
const id = localScope.object.objectId;
add(frames =>
session.getLocalVariables(id, vars => {
frames[i] = { function: fn, vars };
next(frames);
});
} else {
const id = localScope.object.objectId;
add(frames =>
session.getLocalVariables(id, vars => {
frames[i] = { function: fn, vars };
next(frames);
}),
);
}
}),
);
}

next([]);
}

const captureAll = options.captureAllExceptions !== false;
next([]);
}

session.configureAndConnect(
(ev, complete) =>
handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
captureAll,
const captureAll = options.captureAllExceptions !== false;

session.configureAndConnect(
(ev, complete) =>
handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
captureAll,
);

if (captureAll) {
const max = options.maxExceptionsPerSecond || 50;

rateLimiter = createRateLimiter(
max,
() => {
debug.log('Local variables rate-limit lifted.');
session.setPauseOnExceptions(true);
},
seconds => {
debug.log(
`Local variables rate-limit exceeded. Disabling capturing of caught exceptions for ${seconds} seconds.`,
);
session.setPauseOnExceptions(false);
},
);
}

if (captureAll) {
const max = options.maxExceptionsPerSecond || 50;

rateLimiter = createRateLimiter(
max,
() => {
debug.log('Local variables rate-limit lifted.');
session.setPauseOnExceptions(true);
},
seconds => {
debug.log(
`Local variables rate-limit exceeded. Disabling capturing of caught exceptions for ${seconds} seconds.`,
);
session.setPauseOnExceptions(false);
},
);
}

shouldProcessEvent = true;
},
error => {
debug.log('The `LocalVariables` integration failed to start.', error);
},
);
shouldProcessEvent = true;
} catch (error) {
debug.log('The `LocalVariables` integration failed to start.', error);
}
},
processEvent(event: Event): Event {
if (shouldProcessEvent) {
Expand All @@ -410,13 +412,7 @@ const _localVariablesSyncIntegration = ((

return event;
},
// These are entirely for testing
_getCachedFramesCount(): number {
return cachedFrames.size;
},
_getFirstCachedFrame(): FrameVariables[] | undefined {
return cachedFrames.values()[0];
},
...testHelperMethods,
};
}) satisfies IntegrationFn;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// // TEST-ONLY: allow tests to access the cache

import type { LRUMap } from '@sentry/core';
import type { FrameVariables } from './common';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we didn't do this (and not add unit tests). Instead I'd like us to add tests under our node integration tests to validate this: https://github.com/getsentry/sentry-javascript/tree/develop/dev-packages/node-integration-tests/suites/public-api/LocalVariables


/**
* Provides test helper methods for interacting with the local variables cache.
* These methods are intended for use in unit tests to inspect and manipulate
* the internal cache of frame variables used by the LocalVariables integration.
*
* @param cachedFrames - The LRUMap instance storing cached frame variables.
* @returns An object containing helper methods for cache inspection and mutation.
*/
export function localVariablesTestHelperMethods(cachedFrames: LRUMap<string, FrameVariables[]>): {
_getCachedFramesCount: () => number;
_getFirstCachedFrame: () => FrameVariables[] | undefined;
_setCachedFrame: (hash: string, frames: FrameVariables[]) => void;
} {
/**
* Returns the number of entries in the local variables cache.
*/
function _getCachedFramesCount(): number {
return cachedFrames.size;
}

/**
* Returns the first set of cached frame variables, or undefined if the cache is empty.
*/
function _getFirstCachedFrame(): FrameVariables[] | undefined {
return cachedFrames.values()[0];
}

/**
* Sets the cached frame variables for a given stack hash.
*
* @param hash - The stack hash to associate with the cached frames.
* @param frames - The frame variables to cache.
*/
function _setCachedFrame(hash: string, frames: FrameVariables[]): void {
cachedFrames.set(hash, frames);
}

return {
_getCachedFramesCount,
_getFirstCachedFrame,
_setCachedFrame,
};
}
73 changes: 73 additions & 0 deletions packages/node-core/test/integrations/local-variables-async.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import type { Event, EventHint, StackFrame } from '@sentry/core';
import { beforeEach, describe, expect, it } from 'vitest';
import { getCurrentScope, NodeClient } from '../../src';
import type { FrameVariables } from '../../src/integrations/local-variables/common';
import { LOCAL_VARIABLES_KEY } from '../../src/integrations/local-variables/common';
import { localVariablesAsyncIntegration } from '../../src/integrations/local-variables/local-variables-async';
import { getDefaultNodeClientOptions } from '../helpers/getDefaultNodeClientOptions';

describe('LocalVariablesAsync', () => {
beforeEach(() => {
const options = getDefaultNodeClientOptions({
includeLocalVariables: true,
dsn: 'https://[email protected]/1337',
});
const client = new NodeClient(options);
getCurrentScope().setClient(client);
});

it('does not add local variables to out of app frames by default', async () => {
const eventName = 'test-exclude-LocalVariables-out-of-app-frames';
const event = getTestEvent(eventName);
const integration = localVariablesAsyncIntegration({});
await integration.setup?.(getCurrentScope().getClient<NodeClient>()!);

const hint: EventHint = {
originalException: {
[LOCAL_VARIABLES_KEY]: [{ function: eventName, vars: { foo: 'bar' } } as FrameVariables],
},
};

const processedEvent = integration.processEvent?.(event, hint, getCurrentScope().getClient<NodeClient>()!) as Event;

expect(processedEvent.exception?.values?.[0]?.stacktrace?.frames?.[0]?.vars).toBeUndefined();
});

it('adds local variables to out of app frames when includeOutOfAppFrames is true', async () => {
const eventName = 'test-include-LocalVariables-out-of-app-frames';
const event = getTestEvent(eventName);
const integration = localVariablesAsyncIntegration({ includeOutOfAppFrames: true });
await integration.setup?.(getCurrentScope().getClient<NodeClient>()!);

const hint: EventHint = {
originalException: {
[LOCAL_VARIABLES_KEY]: [{ function: eventName, vars: { foo: 'bar' } } as FrameVariables],
},
};

const processedEvent = integration.processEvent?.(event, hint, getCurrentScope().getClient<NodeClient>()!) as Event;

expect(processedEvent.exception?.values?.[0]?.stacktrace?.frames?.[0]?.vars).toEqual({ foo: 'bar' });
});
});

function getTestEvent(fnName = 'test'): Event {
return {
exception: {
values: [
{
stacktrace: {
frames: [
{
in_app: false,
function: fnName,
lineno: 1,
colno: 1,
} as StackFrame,
],
},
},
],
},
};
}
Loading