ref(core): Add safeCallback helper for isolating user-provided callbacks

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: msonnb

packages/core/src/shared-exports.ts

@@ -94,6 +94,7 @@ export {
 export { safeSetSpanJSONAttributes } from './tracing/spans/captureSpan';
 export { isSentryRequestUrl } from './utils/isSentryRequestUrl';
 export { handleCallbackErrors } from './utils/handleCallbackErrors';
+export { safeCallback } from './utils/safeCallback';
 export { parameterize, fmt } from './utils/parameterize';
 export type { HandleTunnelRequestOptions } from './utils/tunnel';
 export { handleTunnelRequest } from './utils/tunnel';

packages/core/src/tracing/spans/beforeSendSpan.ts

@@ -1,8 +1,8 @@
-import { DEBUG_BUILD } from '../../debug-build';
 import type { BeforeSendStaticSpanCallback, BeforeSendStreamedSpanCallback } from '../../types/options';
 import type { SpanJSON, StreamedSpanJSON } from '../../types/span';
 import { addNonEnumerableProperty } from '../../utils/object';
-import { consoleSandbox, debug } from '../../utils/debug-logger';
+import { consoleSandbox } from '../../utils/debug-logger';
+import { safeCallback } from '../../utils/safeCallback';
 
 /**
  * A wrapper to use the static, transaction-based span format in your `beforeSendSpan` callback.
@@ -64,25 +64,25 @@ export function applyBeforeSendSpanCallback<T extends StreamedSpanJSON | SpanJSO
   span: T,
   beforeSendSpan: (span: T) => T,
 ): T {
-  try {
-    const modifedSpan = beforeSendSpan(span);
-    if (!modifedSpan) {
-      if (!hasShownSpanDropWarning) {
-        consoleSandbox(() => {
-          // eslint-disable-next-line no-console
-          console.warn(
-            '[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly or use `ignoreSpans`.',
-          );
-        });
-        hasShownSpanDropWarning = true;
-      }
-      return span;
-    }
-    return modifedSpan;
-  } catch (error) {
-    // Spans are captured synchronously when they end, so a throwing callback would otherwise
-    // propagate into whatever user code ended the span.
-    DEBUG_BUILD && debug.error('The `beforeSendSpan` callback threw an error, sending the span unmodified:', error);
-    return span;
+  // Spans are captured synchronously when they end, so a throwing callback would otherwise
+  // propagate into whatever user code ended the span.
+  const modifiedSpan = safeCallback(
+    'The `beforeSendSpan` callback threw an error, sending the span unmodified:',
+    () => beforeSendSpan(span),
+    () => span,
+  );
+  if (modifiedSpan) {
+    return modifiedSpan;
   }
+
+  if (!hasShownSpanDropWarning) {
+    consoleSandbox(() => {
+      // eslint-disable-next-line no-console
+      console.warn(
+        '[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly or use `ignoreSpans`.',
+      );
+    });
+    hasShownSpanDropWarning = true;
+  }
+  return span;
 }

packages/core/src/utils/safeCallback.ts

@@ -0,0 +1,34 @@
+import { DEBUG_BUILD } from '../debug-build';
+import { debug } from './debug-logger';
+import { isThenable } from './is';
+
+/**
+ * Invokes a user-provided callback (e.g. `beforeSend`, `tracesSampler`, an integration hook) so that
+ * neither a synchronous throw nor a rejected promise escapes into the caller. On failure the error is
+ * logged and `fallback(error)` supplies the result instead.
+ *
+ * Not for `startSpan` bodies: those must re-throw and are handled by `handleCallbackErrors`.
+ *
+ * @param message - Logged via `debug.error` together with the error, e.g. "The `beforeSend` callback threw an error, dropping the event:".
+ * @param fn - Invokes the callback.
+ * @param fallback - Produces the result to use when the callback throws or rejects.
+ */
+export function safeCallback<T>(message: string, fn: () => T, fallback: (error: unknown) => T): T {
+  let result: T;
+  try {
+    result = fn();
+  } catch (error) {
+    return recover(message, error, fallback);
+  }
+
+  if (isThenable(result)) {
+    return result.then(undefined, (error: unknown) => recover(message, error, fallback)) as T;
+  }
+
+  return result;
+}
+
+function recover<T>(message: string, error: unknown, fallback: (error: unknown) => T): T {
+  DEBUG_BUILD && debug.error(message, error);
+  return fallback(error);
+}

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

@@ -0,0 +1,70 @@
+import { afterEach, describe, expect, it, vi } from 'vitest';
+import { debug } from '../../../src/utils/debug-logger';
+import { safeCallback } from '../../../src/utils/safeCallback';
+
+describe('safeCallback', () => {
+  const debugErrorSpy = vi.spyOn(debug, 'error').mockImplementation(() => undefined);
+
+  afterEach(() => {
+    debugErrorSpy.mockClear();
+  });
+
+  it('returns the result of a sync callback', () => {
+    const fallback = vi.fn(() => 'fallback');
+
+    expect(safeCallback('callback threw:', () => 'value', fallback)).toBe('value');
+    expect(fallback).not.toHaveBeenCalled();
+    expect(debugErrorSpy).not.toHaveBeenCalled();
+  });
+
+  it('returns the fallback and logs when a sync callback throws', () => {
+    const error = new Error('boom');
+    const fallback = vi.fn(() => 'fallback');
+
+    expect(
+      safeCallback(
+        'callback threw:',
+        () => {
+          throw error;
+        },
+        fallback,
+      ),
+    ).toBe('fallback');
+    expect(fallback).toHaveBeenCalledWith(error);
+    expect(debugErrorSpy).toHaveBeenCalledWith('callback threw:', error);
+  });
+
+  it('resolves to the result of an async callback', async () => {
+    const fallback = vi.fn(async () => 'fallback');
+
+    const result = safeCallback('callback threw:', async () => 'value', fallback);
+
+    expect(result).toBeInstanceOf(Promise);
+    await expect(result).resolves.toBe('value');
+    expect(fallback).not.toHaveBeenCalled();
+    expect(debugErrorSpy).not.toHaveBeenCalled();
+  });
+
+  it('resolves to the fallback and logs when an async callback rejects', async () => {
+    const error = new Error('boom');
+    const fallback = vi.fn(async () => 'fallback');
+
+    const result = safeCallback('callback threw:', () => Promise.reject(error), fallback);
+
+    await expect(result).resolves.toBe('fallback');
+    expect(fallback).toHaveBeenCalledWith(error);
+    expect(debugErrorSpy).toHaveBeenCalledWith('callback threw:', error);
+  });
+
+  it('does not treat non-thenable objects as promises', () => {
+    const value = { then: 'not a function' };
+
+    expect(
+      safeCallback(
+        'callback threw:',
+        () => value,
+        () => ({ then: 'fallback' }),
+      ),
+    ).toBe(value);
+  });
+});

packages/node/src/integrations/node-fetch/undici-instrumentation.ts

@@ -29,6 +29,7 @@ import {
   isTracingSuppressed,
   LRUMap,
   parseUrl,
+  safeCallback,
   SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME,
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
   SPAN_STATUS_ERROR,
@@ -110,16 +111,6 @@ export function instrumentUndici(config: NodeFetchOptions = {}): void {
   subscribeToChannel('undici:request:error', message => onError(message as RequestErrorMessage));
 }
 
-/** Replaces OTel's `safeExecuteInTheMiddle`: run `fn`, route any error to `onError`, and swallow it. */
-function safeExecute<T>(fn: () => T, onError: (error: unknown) => void): T | undefined {
-  try {
-    return fn();
-  } catch (error) {
-    onError(error);
-    return undefined;
-  }
-}
-
 function subscribeToChannel(
   diagnosticChannel: string,
   onMessage: (message: unknown, name: string | symbol) => void,
@@ -177,9 +168,10 @@ function parseRequestHeaders(request: UndiciRequest): Map<string, string | strin
 function onRequestCreated(config: NodeFetchOptions, { request }: RequestMessage): void {
   const url = getAbsoluteUrl(request.origin, request.path);
 
-  const ignoredByCallback = safeExecute(
+  const ignoredByCallback = safeCallback(
+    'The `ignoreOutgoingRequests` callback threw an error, not ignoring the request:',
     () => !!config.ignoreOutgoingRequests?.(url),
-    e => e && DEBUG_BUILD && debug.error('caught ignoreOutgoingRequests error: ', e),
+    () => false,
   );
 
   // Breadcrumbs & span-less trace propagation are additionally skipped when tracing is suppressed.
@@ -277,9 +269,10 @@ function onRequestCreated(config: NodeFetchOptions, { request }: RequestMessage)
   });
 
   // Execute the request hook if defined
-  safeExecute(
+  safeCallback(
+    'The `requestHook` callback threw an error:',
     () => config.requestHook?.(span, request),
-    e => e && DEBUG_BUILD && debug.error('caught requestHook error: ', e),
+    () => undefined,
   );
 
   // Context propagation goes last so no hook can tamper the propagation headers.
@@ -345,9 +338,10 @@ function onResponseHeaders(config: NodeFetchOptions, { request, response }: Resp
   };
 
   // Execute the response hook if defined
-  safeExecute(
+  safeCallback(
+    'The `responseHook` callback threw an error:',
     () => config.responseHook?.(span, { request, response }),
-    e => e && DEBUG_BUILD && debug.error('caught responseHook error: ', e),
+    () => undefined,
   );
 
   if (config.headersToSpanAttributes?.responseHeaders) {