fix(core,browser): Delete SentryNonRecordingSpan from fetch/xhr map (#19336)

This resolves a leak where `SentryNonRecordingSpan` are pilled up when
`tracingSampleRate` is set to `0`. Theoretically
`SentryNonRecordingSpan` are still treated as spans and added to the
`spans` list, but never removed

By moving `shouldCreateSpanResult` closer to the actual span logic, this
is now resolved.

Closes #19337 (added automatically)

Author: JPeer264

packages/browser/src/tracing/request.ts

@@ -332,24 +332,28 @@ function xhrCallback(
 
   const shouldCreateSpanResult = hasSpansEnabled() && shouldCreateSpan(url);
 
-  // check first if the request has finished and is tracked by an existing span which should now end
-  if (handlerData.endTimestamp && shouldCreateSpanResult) {
+  // Handle XHR completion - clean up spans from the record
+  if (handlerData.endTimestamp) {
     const spanId = xhr.__sentry_xhr_span_id__;
     if (!spanId) return;
 
     const span = spans[spanId];
-    if (span && sentryXhrData.status_code !== undefined) {
-      setHttpStatus(span, sentryXhrData.status_code);
-      span.end();
 
-      onRequestSpanEnd?.(span, {
-        headers: createHeadersSafely(parseXhrResponseHeaders(xhr as XMLHttpRequest & SentryWrappedXMLHttpRequest)),
-        error: handlerData.error,
-      });
+    if (span) {
+      if (shouldCreateSpanResult && sentryXhrData.status_code !== undefined) {
+        setHttpStatus(span, sentryXhrData.status_code);
+        span.end();
+
+        onRequestSpanEnd?.(span, {
+          headers: createHeadersSafely(parseXhrResponseHeaders(xhr as XMLHttpRequest & SentryWrappedXMLHttpRequest)),
+          error: handlerData.error,
+        });
+      }
 
       // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
       delete spans[spanId];
     }
+
     return undefined;
   }
 

packages/core/src/fetch.ts

@@ -81,19 +81,23 @@ export function instrumentFetchRequest(
 
   const shouldCreateSpanResult = hasSpansEnabled() && shouldCreateSpan(url);
 
-  if (handlerData.endTimestamp && shouldCreateSpanResult) {
+  if (handlerData.endTimestamp) {
     const spanId = handlerData.fetchData.__span;
     if (!spanId) return;
 
     const span = spans[spanId];
-    if (span) {
-      endSpan(span, handlerData);
 
-      _callOnRequestSpanEnd(span, handlerData, spanOriginOrOptions);
+    if (span) {
+      // Only end the span and call hooks if we're actually recording
+      if (shouldCreateSpanResult) {
+        endSpan(span, handlerData);
+        _callOnRequestSpanEnd(span, handlerData, spanOriginOrOptions);
+      }
 
       // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
       delete spans[spanId];
     }
+
     return undefined;
   }
 

packages/core/test/lib/fetch.test.ts

@@ -1,10 +1,12 @@
-import { describe, expect, it, vi } from 'vitest';
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import type { HandlerDataFetch } from '../../src';
 import { _addTracingHeadersToFetchRequest, instrumentFetchRequest } from '../../src/fetch';
 import type { Span } from '../../src/types-hoist/span';
 
-const { DEFAULT_SENTRY_TRACE, DEFAULT_BAGGAGE } = vi.hoisted(() => ({
+const { DEFAULT_SENTRY_TRACE, DEFAULT_BAGGAGE, hasSpansEnabled } = vi.hoisted(() => ({
   DEFAULT_SENTRY_TRACE: 'defaultTraceId-defaultSpanId-1',
   DEFAULT_BAGGAGE: 'sentry-trace_id=defaultTraceId,sentry-sampled=true,sentry-sample_rate=0.5,sentry-sample_rand=0.232',
+  hasSpansEnabled: vi.fn(),
 }));
 
 const CUSTOM_SENTRY_TRACE = '123-abc-1';
@@ -23,7 +25,18 @@ vi.mock('../../src/utils/traceData', () => {
   };
 });
 
+vi.mock('../../src/utils/hasSpansEnabled', () => {
+  return {
+    hasSpansEnabled,
+  };
+});
+
 describe('_addTracingHeadersToFetchRequest', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    hasSpansEnabled.mockReturnValue(false);
+  });
+
   describe('when request is a string', () => {
     describe('and no request headers are set', () => {
       it.each([
@@ -412,6 +425,53 @@ describe('_addTracingHeadersToFetchRequest', () => {
 });
 
 describe('instrumentFetchRequest', () => {
+  describe('span cleanup', () => {
+    it.each([
+      { name: 'non-recording', hasTracingEnabled: false },
+      { name: 'recording', hasTracingEnabled: true },
+    ])('cleans up $name spans from the spans record when fetch completes', ({ hasTracingEnabled }) => {
+      hasSpansEnabled.mockReturnValue(hasTracingEnabled);
+
+      const spans: Record<string, Span> = {};
+
+      const handlerData = {
+        fetchData: { url: '/api/test', method: 'GET' },
+        args: ['/api/test'] as unknown[],
+        startTimestamp: Date.now(),
+      };
+
+      instrumentFetchRequest(
+        handlerData,
+        () => true,
+        () => false,
+        spans,
+        { spanOrigin: 'auto.http.fetch' },
+      );
+
+      // @ts-expect-error -- we know it exists
+      const spanId = handlerData.fetchData.__span;
+
+      expect(spanId).toBeDefined();
+      expect(spans[spanId]).toBeDefined();
+
+      const completedHandlerData: HandlerDataFetch = {
+        ...handlerData,
+        endTimestamp: Date.now() + 100,
+        response: { status: 200, headers: new Headers() } as Response,
+      };
+
+      instrumentFetchRequest(
+        completedHandlerData,
+        () => true,
+        () => false,
+        spans,
+        { spanOrigin: 'auto.http.fetch' },
+      );
+
+      expect(spans[spanId]).toBeUndefined();
+    });
+  });
+
   describe('options object mutation', () => {
     it('does not mutate the original options object', () => {
       const originalOptions = { method: 'POST', body: JSON.stringify({ data: 'test' }) };