Skip to content

Commit e0af87f

Browse files
JPeer264claude
andcommitted
fix(core): Use WeakRef for Span-Scope circular references
Break the circular reference between Span and Scope by using WeakRef in both directions, allowing proper garbage collection of completed spans and their associated scopes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 18ecbe9 commit e0af87f

7 files changed

Lines changed: 74 additions & 88 deletions

File tree

packages/core/src/tracing/utils.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,15 @@ const SCOPE_ON_START_SPAN_FIELD = '_sentryScope';
77
const ISOLATION_SCOPE_ON_START_SPAN_FIELD = '_sentryIsolationScope';
88

99
type SpanWithScopes = Span & {
10-
[SCOPE_ON_START_SPAN_FIELD]?: Scope;
10+
[SCOPE_ON_START_SPAN_FIELD]?: MaybeWeakRef<Scope>;
1111
[ISOLATION_SCOPE_ON_START_SPAN_FIELD]?: MaybeWeakRef<Scope>;
1212
};
1313

1414
/** Store the scope & isolation scope for a span, which can the be used when it is finished. */
1515
export function setCapturedScopesOnSpan(span: Span | undefined, scope: Scope, isolationScope: Scope): void {
1616
if (span) {
1717
addNonEnumerableProperty(span, ISOLATION_SCOPE_ON_START_SPAN_FIELD, makeWeakRef(isolationScope));
18-
// We don't wrap the scope with a WeakRef here because webkit aggressively garbage collects
19-
// and scopes are not held in memory for long periods of time.
20-
addNonEnumerableProperty(span, SCOPE_ON_START_SPAN_FIELD, scope);
18+
addNonEnumerableProperty(span, SCOPE_ON_START_SPAN_FIELD, makeWeakRef(scope));
2119
}
2220
}
2321

@@ -29,7 +27,7 @@ export function getCapturedScopesOnSpan(span: Span): { scope?: Scope; isolationS
2927
const spanWithScopes = span as SpanWithScopes;
3028

3129
return {
32-
scope: spanWithScopes[SCOPE_ON_START_SPAN_FIELD],
30+
scope: derefWeakRef(spanWithScopes[SCOPE_ON_START_SPAN_FIELD]),
3331
isolationScope: derefWeakRef(spanWithScopes[ISOLATION_SCOPE_ON_START_SPAN_FIELD]),
3432
};
3533
}

packages/core/src/utils/spanOnScope.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import type { Scope } from '../scope';
22
import type { Span } from '../types/span';
33
import { addNonEnumerableProperty } from '../utils/object';
4+
import { derefWeakRef, makeWeakRef, type MaybeWeakRef } from './weakRef';
45

56
const SCOPE_SPAN_FIELD = '_sentrySpan';
67

78
type ScopeWithMaybeSpan = Scope & {
8-
[SCOPE_SPAN_FIELD]?: Span;
9+
[SCOPE_SPAN_FIELD]?: MaybeWeakRef<Span>;
910
};
1011

1112
/**
@@ -14,7 +15,8 @@ type ScopeWithMaybeSpan = Scope & {
1415
*/
1516
export function _setSpanForScope(scope: Scope, span: Span | undefined): void {
1617
if (span) {
17-
addNonEnumerableProperty(scope as ScopeWithMaybeSpan, SCOPE_SPAN_FIELD, span);
18+
// Use WeakRef to avoid circular reference with span holding scope
19+
addNonEnumerableProperty(scope as ScopeWithMaybeSpan, SCOPE_SPAN_FIELD, makeWeakRef(span));
1820
} else {
1921
// eslint-disable-next-line @typescript-eslint/no-dynamic-delete
2022
delete (scope as ScopeWithMaybeSpan)[SCOPE_SPAN_FIELD];
@@ -26,5 +28,5 @@ export function _setSpanForScope(scope: Scope, span: Span | undefined): void {
2628
* NOTE: This should NOT be used directly, but is only used internally by the trace methods.
2729
*/
2830
export function _getSpanForScope(scope: ScopeWithMaybeSpan): Span | undefined {
29-
return scope[SCOPE_SPAN_FIELD];
31+
return derefWeakRef(scope[SCOPE_SPAN_FIELD]);
3032
}

packages/core/src/utils/spanUtils.ts

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
/* eslint-disable max-lines */
2+
13
import { getAsyncContextStrategy } from '../asyncContext';
24
import type { RawAttributes } from '../attributes';
35
import { serializeAttributes } from '../attributes';
@@ -30,6 +32,8 @@ import { timestampInSeconds } from '../utils/time';
3032
import { generateSentryTraceHeader, generateTraceparentHeader } from '../utils/tracing';
3133
import { consoleSandbox } from './debug-logger';
3234
import { _getSpanForScope } from './spanOnScope';
35+
import type { MaybeWeakRef } from './weakRef';
36+
import { derefWeakRef, makeWeakRef } from './weakRef';
3337

3438
// These are aligned with OpenTelemetry trace flags
3539
export const TRACE_FLAG_NONE = 0x0;
@@ -343,32 +347,39 @@ const CHILD_SPANS_FIELD = '_sentryChildSpans';
343347
const ROOT_SPAN_FIELD = '_sentryRootSpan';
344348

345349
type SpanWithPotentialChildren = Span & {
346-
[CHILD_SPANS_FIELD]?: Set<Span>;
350+
[CHILD_SPANS_FIELD]?: Set<MaybeWeakRef<Span>>;
347351
[ROOT_SPAN_FIELD]?: Span;
348352
};
349353

350354
/**
351355
* Adds an opaque child span reference to a span.
356+
* Uses WeakRef to allow child spans to be garbage collected when no longer needed.
352357
*/
353358
export function addChildSpanToSpan(span: SpanWithPotentialChildren, childSpan: Span): void {
354359
// We store the root span reference on the child span
355360
// We need this for `getRootSpan()` to work
356361
const rootSpan = span[ROOT_SPAN_FIELD] || span;
357362
addNonEnumerableProperty(childSpan as SpanWithPotentialChildren, ROOT_SPAN_FIELD, rootSpan);
358363

359-
// We store a list of child spans on the parent span
360-
// We need this for `getSpanDescendants()` to work
364+
const ref = makeWeakRef(childSpan);
365+
361366
if (span[CHILD_SPANS_FIELD]) {
362-
span[CHILD_SPANS_FIELD].add(childSpan);
367+
span[CHILD_SPANS_FIELD].add(ref);
363368
} else {
364-
addNonEnumerableProperty(span, CHILD_SPANS_FIELD, new Set([childSpan]));
369+
addNonEnumerableProperty(span, CHILD_SPANS_FIELD, new Set([ref]));
365370
}
366371
}
367372

368373
/** This is only used internally by Idle Spans. */
369374
export function removeChildSpanFromSpan(span: SpanWithPotentialChildren, childSpan: Span): void {
370-
if (span[CHILD_SPANS_FIELD]) {
371-
span[CHILD_SPANS_FIELD].delete(childSpan);
375+
const children = span[CHILD_SPANS_FIELD];
376+
if (children) {
377+
for (const ref of children) {
378+
if (derefWeakRef(ref) === childSpan) {
379+
children.delete(ref);
380+
break;
381+
}
382+
}
372383
}
373384
}
374385

@@ -385,9 +396,12 @@ export function getSpanDescendants(span: SpanWithPotentialChildren): Span[] {
385396
// We want to ignore unsampled spans (e.g. non recording spans)
386397
} else if (spanIsSampled(span)) {
387398
resultSet.add(span);
388-
const childSpans = span[CHILD_SPANS_FIELD] ? Array.from(span[CHILD_SPANS_FIELD]) : [];
389-
for (const childSpan of childSpans) {
390-
addSpanChildren(childSpan);
399+
const childRefs = span[CHILD_SPANS_FIELD] ? Array.from(span[CHILD_SPANS_FIELD]) : [];
400+
for (const ref of childRefs) {
401+
const childSpan = derefWeakRef(ref);
402+
if (childSpan) {
403+
addSpanChildren(childSpan);
404+
}
391405
}
392406
}
393407
}

packages/core/test/lib/tracing/trace.test.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
spanToJSON,
1414
withScope,
1515
} from '../../../src';
16+
import { derefWeakRef } from '../../../src/utils/weakRef';
1617
import { getAsyncContextStrategy } from '../../../src/asyncContext';
1718
import {
1819
continueTrace,
@@ -786,7 +787,7 @@ describe('startSpan', () => {
786787
expect.assertions(1);
787788
startSpan({ name: 'outer' }, (outerSpan: any) => {
788789
startSpan({ name: 'inner' }, innerSpan => {
789-
const childSpans = Array.from(outerSpan._sentryChildSpans);
790+
const childSpans = Array.from(outerSpan._sentryChildSpans).map(derefWeakRef);
790791
expect(childSpans).toContain(innerSpan);
791792
});
792793
});
@@ -1335,7 +1336,7 @@ describe('startSpanManual', () => {
13351336
expect.assertions(1);
13361337
startSpan({ name: 'outer' }, (outerSpan: any) => {
13371338
startSpanManual({ name: 'inner' }, innerSpan => {
1338-
const childSpans = Array.from(outerSpan._sentryChildSpans);
1339+
const childSpans = Array.from(outerSpan._sentryChildSpans).map(derefWeakRef);
13391340
expect(childSpans).toContain(innerSpan);
13401341
});
13411342
});
@@ -1802,7 +1803,7 @@ describe('startInactiveSpan', () => {
18021803
expect.assertions(1);
18031804
startSpan({ name: 'outer' }, (outerSpan: any) => {
18041805
const innerSpan = startInactiveSpan({ name: 'inner' });
1805-
const childSpans = Array.from(outerSpan._sentryChildSpans);
1806+
const childSpans = Array.from(outerSpan._sentryChildSpans).map(derefWeakRef);
18061807
expect(childSpans).toContain(innerSpan);
18071808
});
18081809
});

packages/core/test/lib/tracing/utils.test.ts

Lines changed: 22 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -44,17 +44,17 @@ describe('tracing utils', () => {
4444
expect(retrieved.isolationScope).toBeUndefined();
4545
});
4646

47-
it('uses WeakRef only for isolation scopes', () => {
47+
it('uses WeakRef for both scopes', () => {
4848
const span = createMockSpan();
4949
const scope = new Scope();
5050
const isolationScope = new Scope();
5151

5252
setCapturedScopesOnSpan(span, scope, isolationScope);
5353

54-
// Check that only isolation scope is wrapped with WeakRef
54+
// Check that both scopes are wrapped with WeakRef
5555
const spanWithScopes = span as any;
56-
expect(spanWithScopes._sentryScope).toBe(scope); // Regular scope stored directly
57-
expect(spanWithScopes._sentryIsolationScope).toBeInstanceOf(WeakRef); // Isolation scope wrapped
56+
expect(spanWithScopes._sentryScope).toBeInstanceOf(WeakRef);
57+
expect(spanWithScopes._sentryIsolationScope).toBeInstanceOf(WeakRef);
5858

5959
// Verify we can still retrieve the scopes
6060
const retrieved = getCapturedScopesOnSpan(span);
@@ -77,14 +77,8 @@ describe('tracing utils', () => {
7777

7878
// Check that both scopes are stored directly when WeakRef is not available
7979
const spanWithScopes = span as any;
80-
expect(spanWithScopes._sentryScope).toBe(scope); // Regular scope always stored directly
81-
expect(spanWithScopes._sentryIsolationScope).toBe(isolationScope); // Isolation scope falls back to direct storage
82-
83-
// When WeakRef is available, ensure regular scope is not wrapped but isolation scope would be
84-
if (originalWeakRef) {
85-
expect(spanWithScopes._sentryScope).not.toBeInstanceOf(originalWeakRef);
86-
expect(spanWithScopes._sentryIsolationScope).not.toBeInstanceOf(originalWeakRef);
87-
}
80+
expect(spanWithScopes._sentryScope).toBe(scope);
81+
expect(spanWithScopes._sentryIsolationScope).toBe(isolationScope);
8882

8983
// Verify we can still retrieve the scopes
9084
const retrieved = getCapturedScopesOnSpan(span);
@@ -98,44 +92,45 @@ describe('tracing utils', () => {
9892

9993
it('handles WeakRef deref returning undefined gracefully', () => {
10094
const span = createMockSpan();
101-
const scope = new Scope();
102-
const isolationScope = new Scope();
103-
104-
setCapturedScopesOnSpan(span, scope, isolationScope);
10595

106-
// Mock WeakRef.deref to return undefined for isolation scope (simulating garbage collection)
107-
// Regular scope is stored directly, so it should always be available
96+
// Mock WeakRef.deref to return undefined (simulating garbage collection)
10897
const spanWithScopes = span as any;
98+
const mockScopeWeakRef = {
99+
deref: vi.fn().mockReturnValue(undefined),
100+
};
109101
const mockIsolationScopeWeakRef = {
110102
deref: vi.fn().mockReturnValue(undefined),
111103
};
112104

113-
// Keep the regular scope as is (stored directly)
114-
// Only replace the isolation scope with a mock WeakRef
105+
spanWithScopes._sentryScope = mockScopeWeakRef;
115106
spanWithScopes._sentryIsolationScope = mockIsolationScopeWeakRef;
116107

117108
const retrieved = getCapturedScopesOnSpan(span);
118-
expect(retrieved.scope).toBe(scope); // Regular scope should still be available
119-
expect(retrieved.isolationScope).toBeUndefined(); // Isolation scope should be undefined due to GC
109+
expect(retrieved.scope).toBeUndefined();
110+
expect(retrieved.isolationScope).toBeUndefined();
111+
expect(mockScopeWeakRef.deref).toHaveBeenCalled();
120112
expect(mockIsolationScopeWeakRef.deref).toHaveBeenCalled();
121113
});
122114

123115
it('handles corrupted WeakRef objects gracefully', () => {
124116
const span = createMockSpan();
125-
const scope = new Scope();
126117

127-
// Set up a regular scope (stored directly) and a corrupted isolation scope WeakRef
118+
// Set up corrupted WeakRefs that throw on deref
128119
const spanWithScopes = span as any;
129-
spanWithScopes._sentryScope = scope; // Regular scope stored directly
120+
spanWithScopes._sentryScope = {
121+
deref: vi.fn().mockImplementation(() => {
122+
throw new Error('WeakRef deref failed');
123+
}),
124+
};
130125
spanWithScopes._sentryIsolationScope = {
131126
deref: vi.fn().mockImplementation(() => {
132127
throw new Error('WeakRef deref failed');
133128
}),
134129
};
135130

136131
const retrieved = getCapturedScopesOnSpan(span);
137-
expect(retrieved.scope).toBe(scope); // Regular scope should still be available
138-
expect(retrieved.isolationScope).toBeUndefined(); // Isolation scope should be undefined due to error
132+
expect(retrieved.scope).toBeUndefined();
133+
expect(retrieved.isolationScope).toBeUndefined();
139134
});
140135

141136
it('preserves scope data when using WeakRef', () => {

packages/opentelemetry/src/spanExporter.ts

Lines changed: 9 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
getCapturedScopesOnSpan,
2121
getDynamicSamplingContextFromSpan,
2222
getStatusMessage,
23+
LRUMap,
2324
SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME,
2425
SEMANTIC_ATTRIBUTE_SENTRY_OP,
2526
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
@@ -41,6 +42,7 @@ type SpanNodeCompleted = SpanNode & { span: ReadableSpan };
4142

4243
const MAX_SPAN_COUNT = 1000;
4344
const DEFAULT_TIMEOUT = 300; // 5 min
45+
const SENT_SPANS_MAX_SIZE = 10000;
4446

4547
interface FinishedSpanBucket {
4648
timestampInS: number;
@@ -71,9 +73,7 @@ export class SentrySpanExporter {
7173
private _finishedSpanBucketSize: number;
7274
private _spansToBucketEntry: WeakMap<ReadableSpan, FinishedSpanBucket>;
7375
private _lastCleanupTimestampInS: number;
74-
// Essentially a a set of span ids that are already sent. The values are expiration
75-
// times in this cache so we don't hold onto them indefinitely.
76-
private _sentSpans: Map<string, number>;
76+
private _sentSpans: LRUMap<string, number>;
7777
/* Internally, we use a debounced flush to give some wiggle room to the span processor to accumulate more spans. */
7878
private _debouncedFlush: ReturnType<typeof debounce>;
7979

@@ -85,7 +85,7 @@ export class SentrySpanExporter {
8585
this._finishedSpanBuckets = new Array(this._finishedSpanBucketSize).fill(undefined);
8686
this._lastCleanupTimestampInS = Math.floor(_INTERNAL_safeDateNow() / 1000);
8787
this._spansToBucketEntry = new WeakMap();
88-
this._sentSpans = new Map<string, number>();
88+
this._sentSpans = new LRUMap<string, number>(SENT_SPANS_MAX_SIZE);
8989
this._debouncedFlush = debounce(this.flush.bind(this), 1, { maxWait: 100 });
9090
}
9191

@@ -124,7 +124,7 @@ export class SentrySpanExporter {
124124

125125
// If the span doesn't have a local parent ID (it's a root span), we're gonna flush all the ended spans
126126
const localParentId = getLocalParentId(span);
127-
if (!localParentId || this._sentSpans.has(localParentId)) {
127+
if (!localParentId || this._sentSpans.get(localParentId)) {
128128
this._debouncedFlush();
129129
}
130130
}
@@ -137,7 +137,6 @@ export class SentrySpanExporter {
137137
public flush(): void {
138138
const finishedSpans = this._finishedSpanBuckets.flatMap(bucket => (bucket ? Array.from(bucket.spans) : []));
139139

140-
this._flushSentSpanCache();
141140
const sentSpans = this._maybeSend(finishedSpans);
142141

143142
const sentSpanCount = sentSpans.size;
@@ -147,15 +146,14 @@ export class SentrySpanExporter {
147146
`SpanExporter exported ${sentSpanCount} spans, ${remainingOpenSpanCount} spans are waiting for their parent spans to finish`,
148147
);
149148

150-
const expirationDate = _INTERNAL_safeDateNow() + DEFAULT_TIMEOUT * 1000;
151-
152149
for (const span of sentSpans) {
153-
this._sentSpans.set(span.spanContext().spanId, expirationDate);
150+
this._sentSpans.set(span.spanContext().spanId, 1);
154151
const bucketEntry = this._spansToBucketEntry.get(span);
155152
if (bucketEntry) {
156153
bucketEntry.spans.delete(span);
157154
}
158155
}
156+
159157
// Cancel a pending debounced flush, if there is one
160158
// This can be relevant if we directly flush, circumventing the debounce
161159
// in that case, we want to cancel any pending debounced flush
@@ -193,7 +191,7 @@ export class SentrySpanExporter {
193191
const transactionEvent = createTransactionForOtelSpan(span);
194192

195193
// Add an attribute to the transaction event to indicate that this transaction is an orphaned transaction
196-
if (root.parentNode && this._sentSpans.has(root.parentNode.id)) {
194+
if (root.parentNode && this._sentSpans.get(root.parentNode.id)) {
197195
const traceData = transactionEvent.contexts?.trace?.data;
198196
if (traceData) {
199197
traceData['sentry.parent_span_already_sent'] = true;
@@ -235,20 +233,9 @@ export class SentrySpanExporter {
235233
return sentSpans;
236234
}
237235

238-
/** Remove "expired" span id entries from the _sentSpans cache. */
239-
private _flushSentSpanCache(): void {
240-
const currentTimestamp = _INTERNAL_safeDateNow();
241-
// Note, it is safe to delete items from the map as we go: https://stackoverflow.com/a/35943995/90297
242-
for (const [spanId, expirationTime] of this._sentSpans.entries()) {
243-
if (expirationTime <= currentTimestamp) {
244-
this._sentSpans.delete(spanId);
245-
}
246-
}
247-
}
248-
249236
/** Check if a node is a completed root node or a node whose parent has already been sent */
250237
private _nodeIsCompletedRootNodeOrHasSentParent(node: SpanNode): node is SpanNodeCompleted {
251-
return !!node.span && (!node.parentNode || this._sentSpans.has(node.parentNode.id));
238+
return !!node.span && (!node.parentNode || !!this._sentSpans.get(node.parentNode.id));
252239
}
253240

254241
/** Get all completed root nodes from a list of nodes */

0 commit comments

Comments
 (0)