From 40ab2fff29143c29b1a56fda17846a29c0e58957 Mon Sep 17 00:00:00 2001 From: JPeer264 Date: Wed, 15 Apr 2026 11:48:08 +0100 Subject: [PATCH 1/2] fix(opentelemetry): Use WeakRef for context stored on scope to prevent memory leak When using pooled connections (e.g., pg) with OTel instrumentation, patched callbacks can retain references to contexts. Previously, contexts held strong references to scopes, and scopes held strong references back to contexts, creating a circular reference that prevented garbage collection. This change: - Adds a shared WeakRef utility (makeWeakRef/derefWeakRef) in @sentry/core - Uses WeakRef for the context stored on scope, breaking the circular reference - Refactors tracing/utils.ts to use the shared utility - Adds comprehensive unit tests for the WeakRef utility Fixes #20174 Co-Authored-By: Claude Opus 4.5 --- packages/core/src/index.ts | 2 + packages/core/src/tracing/utils.ts | 44 +---- packages/core/src/utils/weakRef.ts | 65 +++++++ packages/core/test/lib/utils/weakRef.test.ts | 178 ++++++++++++++++++ .../opentelemetry/src/utils/contextData.ts | 17 +- 5 files changed, 263 insertions(+), 43 deletions(-) create mode 100644 packages/core/src/utils/weakRef.ts create mode 100644 packages/core/test/lib/utils/weakRef.test.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 219410a1a3cf..f4039244e550 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -105,6 +105,8 @@ export { getTraceData } from './utils/traceData'; export { shouldPropagateTraceForUrl } from './utils/tracePropagationTargets'; export { getTraceMetaTags } from './utils/meta'; export { debounce } from './utils/debounce'; +export { makeWeakRef, derefWeakRef } from './utils/weakRef'; +export type { MaybeWeakRef } from './utils/weakRef'; export { shouldIgnoreSpan } from './utils/should-ignore-span'; export { winterCGHeadersToDict, diff --git a/packages/core/src/tracing/utils.ts b/packages/core/src/tracing/utils.ts index 6ca5594b3da6..04274c3d2352 100644 --- a/packages/core/src/tracing/utils.ts +++ b/packages/core/src/tracing/utils.ts @@ -1,56 +1,20 @@ import type { Scope } from '../scope'; import type { Span } from '../types-hoist/span'; import { addNonEnumerableProperty } from '../utils/object'; -import { GLOBAL_OBJ } from '../utils/worldwide'; +import { derefWeakRef, makeWeakRef, type MaybeWeakRef } from '../utils/weakRef'; const SCOPE_ON_START_SPAN_FIELD = '_sentryScope'; const ISOLATION_SCOPE_ON_START_SPAN_FIELD = '_sentryIsolationScope'; -type ScopeWeakRef = { deref(): Scope | undefined } | Scope; - type SpanWithScopes = Span & { [SCOPE_ON_START_SPAN_FIELD]?: Scope; - [ISOLATION_SCOPE_ON_START_SPAN_FIELD]?: ScopeWeakRef; + [ISOLATION_SCOPE_ON_START_SPAN_FIELD]?: MaybeWeakRef; }; -/** Wrap a scope with a WeakRef if available, falling back to a direct scope. */ -function wrapScopeWithWeakRef(scope: Scope): ScopeWeakRef { - try { - // @ts-expect-error - WeakRef is not available in all environments - const WeakRefClass = GLOBAL_OBJ.WeakRef; - if (typeof WeakRefClass === 'function') { - return new WeakRefClass(scope); - } - } catch { - // WeakRef not available or failed to create - // We'll fall back to a direct scope - } - - return scope; -} - -/** Try to unwrap a scope from a potential WeakRef wrapper. */ -function unwrapScopeFromWeakRef(scopeRef: ScopeWeakRef | undefined): Scope | undefined { - if (!scopeRef) { - return undefined; - } - - if (typeof scopeRef === 'object' && 'deref' in scopeRef && typeof scopeRef.deref === 'function') { - try { - return scopeRef.deref(); - } catch { - return undefined; - } - } - - // Fallback to a direct scope - return scopeRef as Scope; -} - /** Store the scope & isolation scope for a span, which can the be used when it is finished. */ export function setCapturedScopesOnSpan(span: Span | undefined, scope: Scope, isolationScope: Scope): void { if (span) { - addNonEnumerableProperty(span, ISOLATION_SCOPE_ON_START_SPAN_FIELD, wrapScopeWithWeakRef(isolationScope)); + addNonEnumerableProperty(span, ISOLATION_SCOPE_ON_START_SPAN_FIELD, makeWeakRef(isolationScope)); // We don't wrap the scope with a WeakRef here because webkit aggressively garbage collects // and scopes are not held in memory for long periods of time. addNonEnumerableProperty(span, SCOPE_ON_START_SPAN_FIELD, scope); @@ -66,6 +30,6 @@ export function getCapturedScopesOnSpan(span: Span): { scope?: Scope; isolationS return { scope: spanWithScopes[SCOPE_ON_START_SPAN_FIELD], - isolationScope: unwrapScopeFromWeakRef(spanWithScopes[ISOLATION_SCOPE_ON_START_SPAN_FIELD]), + isolationScope: derefWeakRef(spanWithScopes[ISOLATION_SCOPE_ON_START_SPAN_FIELD]), }; } diff --git a/packages/core/src/utils/weakRef.ts b/packages/core/src/utils/weakRef.ts new file mode 100644 index 000000000000..be3420358db4 --- /dev/null +++ b/packages/core/src/utils/weakRef.ts @@ -0,0 +1,65 @@ +import { GLOBAL_OBJ } from './worldwide'; + +/** + * Interface representing a weak reference to an object. + * This matches the standard WeakRef interface but is defined here + * because WeakRef is not available in ES2020 type definitions. + */ +interface WeakRefLike { + deref(): T | undefined; +} + +/** + * A wrapper type that represents either a WeakRef-like object or a direct reference. + * Used for optional weak referencing in environments where WeakRef may not be available. + */ +export type MaybeWeakRef = WeakRefLike | T; + +/** + * Creates a weak reference to an object if WeakRef is available, + * otherwise returns the object directly. + * + * This is useful for breaking circular references while maintaining + * compatibility with environments that don't support WeakRef (e.g., older browsers). + * + * @param value - The object to create a weak reference to + * @returns A WeakRef wrapper if available, or the original object as fallback + */ +export function makeWeakRef(value: T): MaybeWeakRef { + try { + // @ts-expect-error - WeakRef may not be in the type definitions for older TS targets + const WeakRefImpl = GLOBAL_OBJ.WeakRef; + if (typeof WeakRefImpl === 'function') { + return new WeakRefImpl(value); + } + } catch { + // WeakRef not available or construction failed + } + return value; +} + +/** + * Resolves a potentially weak reference, returning the underlying object + * or undefined if the reference has been garbage collected. + * + * @param ref - A MaybeWeakRef or undefined + * @returns The referenced object, or undefined if GC'd or ref was undefined + */ +export function derefWeakRef(ref: MaybeWeakRef | undefined): T | undefined { + if (!ref) { + return undefined; + } + + // Check if this is a WeakRef (has deref method) + if (typeof ref === 'object' && 'deref' in ref && typeof ref.deref === 'function') { + try { + return ref.deref(); + } catch { + // deref() failed - treat as GC'd + return undefined; + } + } + + // Direct reference fallback + return ref as T; +} diff --git a/packages/core/test/lib/utils/weakRef.test.ts b/packages/core/test/lib/utils/weakRef.test.ts new file mode 100644 index 000000000000..cf050ccf3d6e --- /dev/null +++ b/packages/core/test/lib/utils/weakRef.test.ts @@ -0,0 +1,178 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { derefWeakRef, makeWeakRef, type MaybeWeakRef } from '../../../src/utils/weakRef'; + +describe('Unit | util | weakRef', () => { + describe('makeWeakRef', () => { + it('creates a WeakRef when available', () => { + const obj = { foo: 'bar' }; + const ref = makeWeakRef(obj); + + // Should be a WeakRef, not the direct object + expect(ref).toBeInstanceOf(WeakRef); + expect((ref as WeakRef).deref()).toBe(obj); + }); + + it('returns the object directly when WeakRef is not available', () => { + const originalWeakRef = globalThis.WeakRef; + (globalThis as any).WeakRef = undefined; + + try { + const obj = { foo: 'bar' }; + const ref = makeWeakRef(obj); + + // Should be the direct object + expect(ref).toBe(obj); + } finally { + (globalThis as any).WeakRef = originalWeakRef; + } + }); + + it('returns the object directly when WeakRef constructor throws', () => { + const originalWeakRef = globalThis.WeakRef; + (globalThis as any).WeakRef = function () { + throw new Error('WeakRef not supported'); + }; + + try { + const obj = { foo: 'bar' }; + const ref = makeWeakRef(obj); + + // Should fall back to the direct object + expect(ref).toBe(obj); + } finally { + (globalThis as any).WeakRef = originalWeakRef; + } + }); + + it('works with different object types', () => { + const plainObject = { key: 'value' }; + const array = [1, 2, 3]; + const func = () => 'test'; + const date = new Date(); + + expect(derefWeakRef(makeWeakRef(plainObject))).toBe(plainObject); + expect(derefWeakRef(makeWeakRef(array))).toBe(array); + expect(derefWeakRef(makeWeakRef(func))).toBe(func); + expect(derefWeakRef(makeWeakRef(date))).toBe(date); + }); + }); + + describe('derefWeakRef', () => { + it('returns undefined for undefined input', () => { + expect(derefWeakRef(undefined)).toBeUndefined(); + }); + + it('correctly dereferences a WeakRef', () => { + const obj = { foo: 'bar' }; + const weakRef = new WeakRef(obj); + + expect(derefWeakRef(weakRef)).toBe(obj); + }); + + it('returns the direct object when not a WeakRef', () => { + const obj = { foo: 'bar' }; + + // Passing a direct object (fallback case) + expect(derefWeakRef(obj as MaybeWeakRef)).toBe(obj); + }); + + it('returns undefined when WeakRef.deref() returns undefined (simulating GC)', () => { + const mockWeakRef = { + deref: vi.fn().mockReturnValue(undefined), + }; + + expect(derefWeakRef(mockWeakRef as MaybeWeakRef)).toBeUndefined(); + expect(mockWeakRef.deref).toHaveBeenCalled(); + }); + + it('returns undefined when WeakRef.deref() throws an error', () => { + const mockWeakRef = { + deref: vi.fn().mockImplementation(() => { + throw new Error('deref failed'); + }), + }; + + expect(derefWeakRef(mockWeakRef as MaybeWeakRef)).toBeUndefined(); + expect(mockWeakRef.deref).toHaveBeenCalled(); + }); + + it('handles objects with a non-function deref property', () => { + const objWithDerefProperty = { + deref: 'not a function', + actualData: 'test', + }; + + // Should treat it as a direct object since deref is not a function + expect(derefWeakRef(objWithDerefProperty as unknown as MaybeWeakRef)).toBe(objWithDerefProperty); + }); + }); + + describe('roundtrip (makeWeakRef + derefWeakRef)', () => { + it('preserves object identity with WeakRef available', () => { + const obj = { nested: { data: [1, 2, 3] } }; + const ref = makeWeakRef(obj); + const retrieved = derefWeakRef(ref); + + expect(retrieved).toBe(obj); + expect(retrieved?.nested.data).toEqual([1, 2, 3]); + }); + + it('preserves object identity with WeakRef unavailable', () => { + const originalWeakRef = globalThis.WeakRef; + (globalThis as any).WeakRef = undefined; + + try { + const obj = { nested: { data: [1, 2, 3] } }; + const ref = makeWeakRef(obj); + const retrieved = derefWeakRef(ref); + + expect(retrieved).toBe(obj); + expect(retrieved?.nested.data).toEqual([1, 2, 3]); + } finally { + (globalThis as any).WeakRef = originalWeakRef; + } + }); + + it('allows multiple refs to the same object', () => { + const obj = { id: 'shared' }; + const ref1 = makeWeakRef(obj); + const ref2 = makeWeakRef(obj); + + expect(derefWeakRef(ref1)).toBe(obj); + expect(derefWeakRef(ref2)).toBe(obj); + expect(derefWeakRef(ref1)).toBe(derefWeakRef(ref2)); + }); + }); + + describe('type safety', () => { + it('preserves generic type information', () => { + interface TestInterface { + id: number; + name: string; + } + + const obj: TestInterface = { id: 1, name: 'test' }; + const ref: MaybeWeakRef = makeWeakRef(obj); + const retrieved: TestInterface | undefined = derefWeakRef(ref); + + expect(retrieved?.id).toBe(1); + expect(retrieved?.name).toBe('test'); + }); + + it('works with class instances', () => { + class TestClass { + constructor(public value: string) {} + getValue(): string { + return this.value; + } + } + + const instance = new TestClass('hello'); + const ref = makeWeakRef(instance); + const retrieved = derefWeakRef(ref); + + expect(retrieved).toBeInstanceOf(TestClass); + expect(retrieved?.getValue()).toBe('hello'); + }); + }); +}); diff --git a/packages/opentelemetry/src/utils/contextData.ts b/packages/opentelemetry/src/utils/contextData.ts index 468b377f9ccd..2923faa09819 100644 --- a/packages/opentelemetry/src/utils/contextData.ts +++ b/packages/opentelemetry/src/utils/contextData.ts @@ -1,11 +1,15 @@ import type { Context } from '@opentelemetry/api'; import type { Scope } from '@sentry/core'; -import { addNonEnumerableProperty } from '@sentry/core'; +import { addNonEnumerableProperty, derefWeakRef, makeWeakRef, type MaybeWeakRef } from '@sentry/core'; import { SENTRY_SCOPES_CONTEXT_KEY } from '../constants'; import type { CurrentScopes } from '../types'; const SCOPE_CONTEXT_FIELD = '_scopeContext'; +type ScopeWithContext = Scope & { + [SCOPE_CONTEXT_FIELD]?: MaybeWeakRef; +}; + /** * Try to get the current scopes from the given OTEL context. * This requires a Context Manager that was wrapped with getWrappedContextManager. @@ -25,14 +29,21 @@ export function setScopesOnContext(context: Context, scopes: CurrentScopes): Con /** * Set the context on the scope so we can later look it up. * We need this to get the context from the scope in the `trace` functions. + * + * We use WeakRef to avoid a circular reference between the scope and the context. + * The context holds scopes (via SENTRY_SCOPES_CONTEXT_KEY), and if the scope held + * a strong reference back to the context, neither could be garbage collected even + * when the context is no longer reachable from application code (e.g., after a + * request completes but pooled connections retain patched callbacks). */ export function setContextOnScope(scope: Scope, context: Context): void { - addNonEnumerableProperty(scope, SCOPE_CONTEXT_FIELD, context); + addNonEnumerableProperty(scope, SCOPE_CONTEXT_FIELD, makeWeakRef(context)); } /** * Get the context related to a scope. + * Returns undefined if the context has been garbage collected (when WeakRef is used). */ export function getContextFromScope(scope: Scope): Context | undefined { - return (scope as { [SCOPE_CONTEXT_FIELD]?: Context })[SCOPE_CONTEXT_FIELD]; + return derefWeakRef((scope as ScopeWithContext)[SCOPE_CONTEXT_FIELD]); } From e4c64738bf1e7bc9803cac67665d1869fa45d417 Mon Sep 17 00:00:00 2001 From: JPeer264 Date: Wed, 15 Apr 2026 12:25:37 +0100 Subject: [PATCH 2/2] test(opentelemetry): Add tests for contextData utils Tests for getScopesFromContext, setScopesOnContext, setContextOnScope, and getContextFromScope including WeakRef behavior verification. Co-Authored-By: Claude Opus 4.5 --- .../test/utils/contextData.test.ts | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 packages/opentelemetry/test/utils/contextData.test.ts diff --git a/packages/opentelemetry/test/utils/contextData.test.ts b/packages/opentelemetry/test/utils/contextData.test.ts new file mode 100644 index 000000000000..597b9fa2b637 --- /dev/null +++ b/packages/opentelemetry/test/utils/contextData.test.ts @@ -0,0 +1,175 @@ +import { ROOT_CONTEXT } from '@opentelemetry/api'; +import { Scope } from '@sentry/core'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { + getContextFromScope, + getScopesFromContext, + setContextOnScope, + setScopesOnContext, +} from '../../src/utils/contextData'; +import type { CurrentScopes } from '../../src/types'; + +describe('contextData', () => { + describe('getScopesFromContext / setScopesOnContext', () => { + it('returns undefined when no scopes are set on context', () => { + const context = ROOT_CONTEXT; + expect(getScopesFromContext(context)).toBeUndefined(); + }); + + it('returns scopes that were set on context', () => { + const scope = new Scope(); + const isolationScope = new Scope(); + const scopes: CurrentScopes = { scope, isolationScope }; + + const contextWithScopes = setScopesOnContext(ROOT_CONTEXT, scopes); + + expect(getScopesFromContext(contextWithScopes)).toBe(scopes); + expect(getScopesFromContext(contextWithScopes)?.scope).toBe(scope); + expect(getScopesFromContext(contextWithScopes)?.isolationScope).toBe(isolationScope); + }); + + it('does not modify the original context', () => { + const scope = new Scope(); + const isolationScope = new Scope(); + const scopes: CurrentScopes = { scope, isolationScope }; + + const originalContext = ROOT_CONTEXT; + const newContext = setScopesOnContext(originalContext, scopes); + + expect(getScopesFromContext(originalContext)).toBeUndefined(); + expect(getScopesFromContext(newContext)).toBe(scopes); + }); + + it('allows overwriting scopes on a derived context', () => { + const scope1 = new Scope(); + const isolationScope1 = new Scope(); + const scopes1: CurrentScopes = { scope: scope1, isolationScope: isolationScope1 }; + + const scope2 = new Scope(); + const isolationScope2 = new Scope(); + const scopes2: CurrentScopes = { scope: scope2, isolationScope: isolationScope2 }; + + const context1 = setScopesOnContext(ROOT_CONTEXT, scopes1); + const context2 = setScopesOnContext(context1, scopes2); + + expect(getScopesFromContext(context1)).toBe(scopes1); + expect(getScopesFromContext(context2)).toBe(scopes2); + }); + }); + + describe('setContextOnScope / getContextFromScope', () => { + it('returns undefined when no context is set on scope', () => { + const scope = new Scope(); + expect(getContextFromScope(scope)).toBeUndefined(); + }); + + it('returns context that was set on scope', () => { + const scope = new Scope(); + const context = ROOT_CONTEXT; + + setContextOnScope(scope, context); + + expect(getContextFromScope(scope)).toBe(context); + }); + + it('stores context as non-enumerable property', () => { + const scope = new Scope(); + const context = ROOT_CONTEXT; + + setContextOnScope(scope, context); + + // The _scopeContext property should not appear in Object.keys + expect(Object.keys(scope)).not.toContain('_scopeContext'); + + // But the context should still be retrievable + expect(getContextFromScope(scope)).toBe(context); + }); + + it('allows overwriting context on scope', () => { + const scope = new Scope(); + const context1 = ROOT_CONTEXT; + const scopes: CurrentScopes = { scope: new Scope(), isolationScope: new Scope() }; + const context2 = setScopesOnContext(ROOT_CONTEXT, scopes); + + setContextOnScope(scope, context1); + expect(getContextFromScope(scope)).toBe(context1); + + setContextOnScope(scope, context2); + expect(getContextFromScope(scope)).toBe(context2); + }); + + describe('WeakRef behavior', () => { + it('uses WeakRef when available', () => { + const scope = new Scope(); + const context = ROOT_CONTEXT; + + setContextOnScope(scope, context); + + // Access the internal property to verify WeakRef is used + const scopeWithContext = scope as unknown as { _scopeContext?: unknown }; + const storedRef = scopeWithContext._scopeContext; + + // If WeakRef is available, the stored value should have a deref method + if (typeof WeakRef !== 'undefined') { + expect(storedRef).toBeDefined(); + expect(typeof (storedRef as { deref?: unknown }).deref).toBe('function'); + } + }); + + it('returns undefined when WeakRef has been garbage collected', () => { + const scope = new Scope(); + + // Simulate a garbage collected WeakRef by directly setting a mock + const mockWeakRef = { + deref: () => undefined, + }; + (scope as unknown as { _scopeContext: unknown })._scopeContext = mockWeakRef; + + expect(getContextFromScope(scope)).toBeUndefined(); + }); + + it('handles WeakRef.deref throwing an error', () => { + const scope = new Scope(); + + // Simulate a WeakRef that throws on deref + const mockWeakRef = { + deref: () => { + throw new Error('deref failed'); + }, + }; + (scope as unknown as { _scopeContext: unknown })._scopeContext = mockWeakRef; + + expect(getContextFromScope(scope)).toBeUndefined(); + }); + + it('works with direct reference fallback when WeakRef is not available', () => { + const scope = new Scope(); + const context = ROOT_CONTEXT; + + // Simulate environment without WeakRef by directly setting a non-WeakRef value + (scope as unknown as { _scopeContext: unknown })._scopeContext = context; + + expect(getContextFromScope(scope)).toBe(context); + }); + }); + }); + + describe('bidirectional relationship', () => { + it('allows navigating from context to scope and back to context', () => { + const scope = new Scope(); + const isolationScope = new Scope(); + const scopes: CurrentScopes = { scope, isolationScope }; + + // Set up bidirectional relationship + const contextWithScopes = setScopesOnContext(ROOT_CONTEXT, scopes); + setContextOnScope(scope, contextWithScopes); + + // Navigate: context -> scopes -> scope -> context + const retrievedScopes = getScopesFromContext(contextWithScopes); + expect(retrievedScopes).toBe(scopes); + + const retrievedContext = getContextFromScope(retrievedScopes!.scope); + expect(retrievedContext).toBe(contextWithScopes); + }); + }); +});