From 7e3d1cd1e8e451ea165f58839e8ff0e220af457d Mon Sep 17 00:00:00 2001 From: JPeer264 Date: Wed, 15 Apr 2026 15:12:56 +0100 Subject: [PATCH] fix(core): Call super.dispose() in ServerRuntimeClient to prevent memory leak ServerRuntimeClient.dispose() was not calling super.dispose(), which meant: - Cleanup callbacks registered via registerCleanup() were never executed - _disposeCallbacks, _eventProcessors, _hooks, and _integrations were not cleared - Client instances could not be garbage collected in per-request isolation patterns This caused a memory leak in Cloudflare Workers where CloudflareClient instances accumulated (~820 retained for 600 requests, growing heap from 1.6MB to 6.1MB). The fix calls super.dispose() first to run the base class cleanup chain, then clears server-specific state (_outcomes, _transport, _promiseBuffer). Co-Authored-By: Claude Opus 4.5 --- packages/cloudflare/src/async.ts | 5 ++- packages/cloudflare/src/flush.ts | 33 +++++++++++++-- packages/cloudflare/src/request.ts | 14 ++++--- .../cloudflare/src/wrapMethodWithSentry.ts | 16 +++++--- packages/core/src/client.ts | 40 ++++++++++++++++++- packages/core/src/index.ts | 3 +- packages/core/src/instrument/console.ts | 6 ++- packages/core/src/instrument/fetch.ts | 12 ++++-- packages/core/src/instrument/handlers.ts | 15 ++++++- packages/core/src/integrations/console.ts | 5 ++- .../core/src/integrations/conversationId.ts | 5 ++- packages/core/src/logs/console-integration.ts | 5 ++- packages/core/src/server-runtime-client.ts | 11 ++--- packages/core/src/tracing/index.ts | 2 +- packages/core/src/tracing/utils.ts | 12 ++++++ 15 files changed, 147 insertions(+), 37 deletions(-) diff --git a/packages/cloudflare/src/async.ts b/packages/cloudflare/src/async.ts index 66f2d439a3ce..4e3333e6fec2 100644 --- a/packages/cloudflare/src/async.ts +++ b/packages/cloudflare/src/async.ts @@ -50,7 +50,10 @@ export function setAsyncLocalStorageAsyncContextStrategy(): void { } function withIsolationScope(callback: (isolationScope: Scope) => T): T { - const scope = getScopes().scope; + // Clone BOTH scopes to ensure request isolation + // Without cloning the current scope, all requests would share the same scope + // which causes race conditions when setting/clearing the client + const scope = getScopes().scope.clone(); const isolationScope = getScopes().isolationScope.clone(); return asyncStorage.run({ scope, isolationScope }, () => { return callback(isolationScope); diff --git a/packages/cloudflare/src/flush.ts b/packages/cloudflare/src/flush.ts index e7f036971f4a..f286ff04f059 100644 --- a/packages/cloudflare/src/flush.ts +++ b/packages/cloudflare/src/flush.ts @@ -1,6 +1,6 @@ import type { ExecutionContext } from '@cloudflare/workers-types'; -import type { Client } from '@sentry/core'; -import { flush } from '@sentry/core'; +import type { Client, Scope, Span } from '@sentry/core'; +import { _INTERNAL_clearCapturedScopesOnSpan, _INTERNAL_setSpanForScope, flush } from '@sentry/core'; type FlushLock = { readonly ready: Promise; @@ -44,16 +44,41 @@ export function makeFlushLock(context: ExecutionContext): FlushLock { * This should be called at the end of each request to prevent memory leaks. * * @param client - The CloudflareClient instance to flush and dispose + * @param isolationScope - The isolation scope that holds a reference to the client + * @param currentScope - The current scope that also holds a reference to the client + * @param span - The active span that holds references to scopes (pass this to break circular refs) * @param timeout - Timeout in milliseconds for the flush operation * @returns A promise that resolves when flush and dispose are complete */ -export async function flushAndDispose(client: Client | undefined, timeout = 2000): Promise { +export async function flushAndDispose( + client: Client | undefined, + isolationScope?: Scope, + currentScope?: Scope, + span?: Span, + timeout = 2000, +): Promise { if (!client) { await flush(timeout); - return; } await client.flush(timeout); client.dispose(); + + // Clear all references between scopes, spans, and clients to allow garbage collection. + // The reference chain is: span -> scope -> client, and scope -> span (bidirectional). + // We must break all these references for the entire object graph to be GC'd. + + // Clear the span's captured scope references - this is critical because spans hold + // strong references to the scopes that were active when they were created. + _INTERNAL_clearCapturedScopesOnSpan(span); + + if (isolationScope) { + _INTERNAL_setSpanForScope(isolationScope, undefined); + isolationScope.setClient(undefined); + } + if (currentScope) { + _INTERNAL_setSpanForScope(currentScope, undefined); + currentScope.setClient(undefined); + } } diff --git a/packages/cloudflare/src/request.ts b/packages/cloudflare/src/request.ts index 05e870ff5d81..56866ef4bdba 100644 --- a/packages/cloudflare/src/request.ts +++ b/packages/cloudflare/src/request.ts @@ -3,6 +3,7 @@ import { captureException, continueTrace, getClient, + getCurrentScope, getHttpSpanDetailsFromUrlObject, httpHeadersToSpanAttributes, parseStringToURLObject, @@ -51,6 +52,9 @@ export function wrapRequestHandler( const client = init({ ...options, ctx: context }); isolationScope.setClient(client); + // Capture the current scope - init() sets the client on it via setCurrentClient + const currentScope = getCurrentScope(); + const urlObject = parseStringToURLObject(request.url); const [name, attributes] = getHttpSpanDetailsFromUrlObject(urlObject, 'server', 'auto.http.cloudflare', request); @@ -94,7 +98,7 @@ export function wrapRequestHandler( } throw e; } finally { - waitUntil?.(flushAndDispose(client)); + waitUntil?.(flushAndDispose(client, isolationScope, currentScope, undefined)); } } @@ -121,7 +125,7 @@ export function wrapRequestHandler( if (captureErrors) { captureException(e, { mechanism: { handled: false, type: 'auto.http.cloudflare' } }); } - waitUntil?.(flushAndDispose(client)); + waitUntil?.(flushAndDispose(client, isolationScope, currentScope, span)); throw e; } @@ -148,7 +152,7 @@ export function wrapRequestHandler( } finally { reader.releaseLock(); span.end(); - waitUntil?.(flushAndDispose(client)); + waitUntil?.(flushAndDispose(client, isolationScope, currentScope, span)); } })(); @@ -164,7 +168,7 @@ export function wrapRequestHandler( } catch (_e) { // tee() failed (e.g stream already locked) - fall back to non-streaming handling span.end(); - waitUntil?.(flushAndDispose(client)); + waitUntil?.(flushAndDispose(client, isolationScope, currentScope, span)); return res; } } @@ -178,7 +182,7 @@ export function wrapRequestHandler( if (res.status === 101) { waitUntil?.(client?.flush(2000)); } else { - waitUntil?.(flushAndDispose(client)); + waitUntil?.(flushAndDispose(client, isolationScope, currentScope, span)); } return res; }); diff --git a/packages/cloudflare/src/wrapMethodWithSentry.ts b/packages/cloudflare/src/wrapMethodWithSentry.ts index 3a7218057c4a..5e95522e0e77 100644 --- a/packages/cloudflare/src/wrapMethodWithSentry.ts +++ b/packages/cloudflare/src/wrapMethodWithSentry.ts @@ -2,6 +2,7 @@ import type { DurableObjectStorage } from '@cloudflare/workers-types'; import { captureException, getClient, + getCurrentScope, isThenable, type Scope, SEMANTIC_ATTRIBUTE_SENTRY_OP, @@ -97,12 +98,15 @@ export function wrapMethodWithSentry( const clientToDispose = scopeClient; const methodName = wrapperOptions.spanName || 'unknown'; + // Capture both scopes for cleanup - scope is the isolation scope, currentScope is the current scope + const currentScopeForCleanup = getCurrentScope(); - const teardown = async (): Promise => { + // teardown will be called with the span (if available) to ensure proper cleanup + const teardown = async (span?: import('@sentry/core').Span): Promise => { if (startNewTrace && storage) { await storeSpanContext(storage, methodName); } - await flushAndDispose(clientToDispose); + await flushAndDispose(clientToDispose, scope, currentScopeForCleanup, span); }; if (!wrapperOptions.spanName) { @@ -182,7 +186,7 @@ export function wrapMethodWithSentry( if (isThenable(result)) { return result.then( (res: unknown) => { - waitUntil?.(teardown()); + waitUntil?.(teardown(span)); return res; }, (e: unknown) => { @@ -192,12 +196,12 @@ export function wrapMethodWithSentry( handled: false, }, }); - waitUntil?.(teardown()); + waitUntil?.(teardown(span)); throw e; }, ); } else { - waitUntil?.(teardown()); + waitUntil?.(teardown(span)); return result; } } catch (e) { @@ -207,7 +211,7 @@ export function wrapMethodWithSentry( handled: false, }, }); - waitUntil?.(teardown()); + waitUntil?.(teardown(span)); throw e; } }); diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 6c3ca949f38e..54d23e7d63e8 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -211,6 +211,9 @@ export abstract class Client { protected _promiseBuffer: PromiseBuffer; + /** Cleanup functions to call on dispose */ + protected _disposeCallbacks: (() => void)[]; + /** * Initializes this client instance. * @@ -223,6 +226,7 @@ export abstract class Client { this._outcomes = {}; this._hooks = {}; this._eventProcessors = []; + this._disposeCallbacks = []; this._promiseBuffer = makePromiseBuffer(options.transportOptions?.bufferSize ?? DEFAULT_TRANSPORT_BUFFER_SIZE); if (options.dsn) { @@ -1145,6 +1149,14 @@ export abstract class Client { return {}; } + /** + * Register a cleanup function to be called when the client is disposed. + * This is useful for integrations that need to clean up global state. + */ + public registerCleanup(callback: () => void): void { + this._disposeCallbacks.push(callback); + } + /** * Disposes of the client and releases all resources. * @@ -1152,7 +1164,33 @@ export abstract class Client { * After calling dispose(), the client should not be used anymore. */ public dispose(): void { - // Base class has no cleanup logic - subclasses implement their own + // Run all registered cleanup callbacks (e.g., unsubscribe from global handlers) + for (const callback of this._disposeCallbacks) { + try { + callback(); + } catch { + // Ignore errors in cleanup callbacks + } + } + this._disposeCallbacks.length = 0; + + // Clear event processors and hooks to break circular references. + // Integration setup creates closures that capture 'client', so: + // client._eventProcessors -> closure -> client (circular) + // client._hooks -> Set -> client (circular) + // Clearing these allows the entire object graph to be GC'd. + this._eventProcessors.length = 0; + for (const hook in this._hooks) { + this._hooks[hook]?.clear(); + } + this._hooks = {}; + this._integrations = {}; + + // Clear options.integrations to release integration objects + // Integration objects have methods (setup, processEvent, etc.) that are closures + if (this._options.integrations) { + (this._options as { integrations?: unknown }).integrations = undefined; + } } /* eslint-enable @typescript-eslint/unified-signatures */ diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 219410a1a3cf..c2f521555cad 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -98,7 +98,8 @@ export { spanTimeInputToSeconds, updateSpanName, } from './utils/spanUtils'; -export { _setSpanForScope as _INTERNAL_setSpanForScope } from './utils/spanOnScope'; +export { _setSpanForScope as _INTERNAL_setSpanForScope, _getSpanForScope as _INTERNAL_getSpanForScope } from './utils/spanOnScope'; +export { clearCapturedScopesOnSpan as _INTERNAL_clearCapturedScopesOnSpan } from './tracing'; export { parseSampleRate } from './utils/parseSampleRate'; export { applySdkMetadata } from './utils/sdkMetadata'; export { getTraceData } from './utils/traceData'; diff --git a/packages/core/src/instrument/console.ts b/packages/core/src/instrument/console.ts index e96de345d202..4f4235dc10b3 100644 --- a/packages/core/src/instrument/console.ts +++ b/packages/core/src/instrument/console.ts @@ -8,14 +8,16 @@ import { addHandler, maybeInstrument, triggerHandlers } from './handlers'; /** * Add an instrumentation handler for when a console.xxx method is called. + * Returns a function to remove the handler. * * Use at your own risk, this might break without changelog notice, only used internally. * @hidden */ -export function addConsoleInstrumentationHandler(handler: (data: HandlerDataConsole) => void): void { +export function addConsoleInstrumentationHandler(handler: (data: HandlerDataConsole) => void): () => void { const type = 'console'; - addHandler(type, handler); + const removeHandler = addHandler(type, handler); maybeInstrument(type, instrumentConsole); + return removeHandler; } function instrumentConsole(): void { diff --git a/packages/core/src/instrument/fetch.ts b/packages/core/src/instrument/fetch.ts index 590830ab4e20..a3165cfbc13a 100644 --- a/packages/core/src/instrument/fetch.ts +++ b/packages/core/src/instrument/fetch.ts @@ -15,6 +15,7 @@ type FetchResource = string | { toString(): string } | { url: string }; * Add an instrumentation handler for when a fetch request happens. * The handler function is called once when the request starts and once when it ends, * which can be identified by checking if it has an `endTimestamp`. + * Returns a function to remove the handler. * * Use at your own risk, this might break without changelog notice, only used internally. * @hidden @@ -22,24 +23,27 @@ type FetchResource = string | { toString(): string } | { url: string }; export function addFetchInstrumentationHandler( handler: (data: HandlerDataFetch) => void, skipNativeFetchCheck?: boolean, -): void { +): () => void { const type = 'fetch'; - addHandler(type, handler); + const removeHandler = addHandler(type, handler); maybeInstrument(type, () => instrumentFetch(undefined, skipNativeFetchCheck)); + return removeHandler; } /** * Add an instrumentation handler for long-lived fetch requests, like consuming server-sent events (SSE) via fetch. * The handler will resolve the request body and emit the actual `endTimestamp`, so that the * span can be updated accordingly. + * Returns a function to remove the handler. * * Only used internally * @hidden */ -export function addFetchEndInstrumentationHandler(handler: (data: HandlerDataFetch) => void): void { +export function addFetchEndInstrumentationHandler(handler: (data: HandlerDataFetch) => void): () => void { const type = 'fetch-body-resolved'; - addHandler(type, handler); + const removeHandler = addHandler(type, handler); maybeInstrument(type, () => instrumentFetch(streamHandler)); + return removeHandler; } function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNativeFetchCheck: boolean = false): void { diff --git a/packages/core/src/instrument/handlers.ts b/packages/core/src/instrument/handlers.ts index 74dbc9902348..fa8214fa413a 100644 --- a/packages/core/src/instrument/handlers.ts +++ b/packages/core/src/instrument/handlers.ts @@ -18,10 +18,21 @@ export type InstrumentHandlerCallback = (data: any) => void; const handlers: { [key in InstrumentHandlerType]?: InstrumentHandlerCallback[] } = {}; const instrumented: { [key in InstrumentHandlerType]?: boolean } = {}; -/** Add a handler function. */ -export function addHandler(type: InstrumentHandlerType, handler: InstrumentHandlerCallback): void { +/** Add a handler function. Returns a function to remove the handler. */ +export function addHandler(type: InstrumentHandlerType, handler: InstrumentHandlerCallback): () => void { handlers[type] = handlers[type] || []; handlers[type].push(handler); + + // Return unsubscribe function + return () => { + const typeHandlers = handlers[type]; + if (typeHandlers) { + const index = typeHandlers.indexOf(handler); + if (index !== -1) { + typeHandlers.splice(index, 1); + } + } + }; } /** diff --git a/packages/core/src/integrations/console.ts b/packages/core/src/integrations/console.ts index dda44543cc03..be25a52e2e69 100644 --- a/packages/core/src/integrations/console.ts +++ b/packages/core/src/integrations/console.ts @@ -41,13 +41,16 @@ export const consoleIntegration = defineIntegration((options: Partial { + const unsubscribe = addConsoleInstrumentationHandler(({ args, level }) => { if (getClient() !== client || !levels.has(level)) { return; } addConsoleBreadcrumb(level, args); }); + + // Register cleanup to remove the handler when client is disposed + client.registerCleanup(unsubscribe); }, }; }); diff --git a/packages/core/src/integrations/conversationId.ts b/packages/core/src/integrations/conversationId.ts index 445e3327419b..2a58782fbf53 100644 --- a/packages/core/src/integrations/conversationId.ts +++ b/packages/core/src/integrations/conversationId.ts @@ -12,7 +12,7 @@ const _conversationIdIntegration = (() => { return { name: INTEGRATION_NAME, setup(client: Client) { - client.on('spanStart', (span: Span) => { + const unsubscribe = client.on('spanStart', (span: Span) => { const scopeData = getCurrentScope().getScopeData(); const isolationScopeData = getIsolationScope().getScopeData(); @@ -32,6 +32,9 @@ const _conversationIdIntegration = (() => { span.setAttribute(GEN_AI_CONVERSATION_ID_ATTRIBUTE, conversationId); } }); + + // Register cleanup to unsubscribe when client is disposed + client.registerCleanup(unsubscribe); }, }; }) satisfies IntegrationFn; diff --git a/packages/core/src/logs/console-integration.ts b/packages/core/src/logs/console-integration.ts index ccf14e3ebf48..143de59af426 100644 --- a/packages/core/src/logs/console-integration.ts +++ b/packages/core/src/logs/console-integration.ts @@ -31,7 +31,7 @@ const _consoleLoggingIntegration = ((options: Partial = { return; } - addConsoleInstrumentationHandler(({ args, level }) => { + const unsubscribe = addConsoleInstrumentationHandler(({ args, level }) => { if (getClient() !== client || !levels.includes(level)) { return; } @@ -66,6 +66,9 @@ const _consoleLoggingIntegration = ((options: Partial = { attributes, }); }); + + // Register cleanup to remove the handler when client is disposed + client.registerCleanup(unsubscribe); }, }; }) satisfies IntegrationFn; diff --git a/packages/core/src/server-runtime-client.ts b/packages/core/src/server-runtime-client.ts index a1958f0bcbbb..ce66cab75745 100644 --- a/packages/core/src/server-runtime-client.ts +++ b/packages/core/src/server-runtime-client.ts @@ -166,15 +166,12 @@ export class ServerRuntimeClient< * Subclasses should override this method to clean up their own resources and call `super.dispose()`. */ public override dispose(): void { - DEBUG_BUILD && debug.log('Disposing client...'); + DEBUG_BUILD && debug.log('Disposing ServerRuntimeClient...'); - for (const hookName of Object.keys(this._hooks)) { - this._hooks[hookName]?.clear(); - } + // Call base class dispose first - this runs cleanup callbacks and clears base state + super.dispose(); - this._hooks = {}; - this._eventProcessors.length = 0; - this._integrations = {}; + // Clear server-specific state this._outcomes = {}; (this as unknown as { _transport?: Transport })._transport = undefined; this._promiseBuffer = makePromiseBuffer(DEFAULT_TRANSPORT_BUFFER_SIZE); diff --git a/packages/core/src/tracing/index.ts b/packages/core/src/tracing/index.ts index 3d3736876015..3e8c8fe24874 100644 --- a/packages/core/src/tracing/index.ts +++ b/packages/core/src/tracing/index.ts @@ -1,5 +1,5 @@ export { registerSpanErrorInstrumentation } from './errors'; -export { setCapturedScopesOnSpan, getCapturedScopesOnSpan } from './utils'; +export { setCapturedScopesOnSpan, getCapturedScopesOnSpan, clearCapturedScopesOnSpan } from './utils'; export { startIdleSpan, TRACING_DEFAULTS } from './idleSpan'; export { SentrySpan } from './sentrySpan'; export { SentryNonRecordingSpan } from './sentryNonRecordingSpan'; diff --git a/packages/core/src/tracing/utils.ts b/packages/core/src/tracing/utils.ts index 6ca5594b3da6..de8e80e728dd 100644 --- a/packages/core/src/tracing/utils.ts +++ b/packages/core/src/tracing/utils.ts @@ -69,3 +69,15 @@ export function getCapturedScopesOnSpan(span: Span): { scope?: Scope; isolationS isolationScope: unwrapScopeFromWeakRef(spanWithScopes[ISOLATION_SCOPE_ON_START_SPAN_FIELD]), }; } + +/** + * Clears the captured scopes from a span to allow garbage collection. + * Call this when disposing of a request context to break circular references. + */ +export function clearCapturedScopesOnSpan(span: Span | undefined): void { + if (span) { + const spanWithScopes = span as SpanWithScopes; + delete spanWithScopes[SCOPE_ON_START_SPAN_FIELD]; + delete spanWithScopes[ISOLATION_SCOPE_ON_START_SPAN_FIELD]; + } +}