diff --git a/dev-packages/cloudflare-integration-tests/suites/double-instrumentation/index.ts b/dev-packages/cloudflare-integration-tests/suites/double-instrumentation/index.ts new file mode 100644 index 000000000000..0844a32ddbd4 --- /dev/null +++ b/dev-packages/cloudflare-integration-tests/suites/double-instrumentation/index.ts @@ -0,0 +1,21 @@ +import * as Sentry from '@sentry/cloudflare'; + +interface Env { + SENTRY_DSN: string; +} + +const handler = { + async fetch(request: Request, _env: Env, _ctx: ExecutionContext) { + if (request.url.includes('/error')) { + throw new Error('Test error from double-instrumented worker'); + } + return new Response('ok'); + }, +}; + +// Deliberately call withSentry twice on the same handler object. +// This simulates scenarios where the module is re-evaluated or the handler +// is wrapped multiple times. The SDK should handle this gracefully +// without double-wrapping (which would cause duplicate error reports). +const once = Sentry.withSentry((env: Env) => ({ dsn: env.SENTRY_DSN }), handler); +export default Sentry.withSentry((env: Env) => ({ dsn: env.SENTRY_DSN }), once); diff --git a/dev-packages/cloudflare-integration-tests/suites/double-instrumentation/test.ts b/dev-packages/cloudflare-integration-tests/suites/double-instrumentation/test.ts new file mode 100644 index 000000000000..b5cba3bd5b08 --- /dev/null +++ b/dev-packages/cloudflare-integration-tests/suites/double-instrumentation/test.ts @@ -0,0 +1,38 @@ +import { expect, it } from 'vitest'; +import { eventEnvelope } from '../../expect'; +import { createRunner } from '../../runner'; + +it('Only sends one error event when withSentry is called twice', async ({ signal }) => { + const runner = createRunner(__dirname) + .expect( + eventEnvelope({ + level: 'error', + exception: { + values: [ + { + type: 'Error', + value: 'Test error from double-instrumented worker', + stacktrace: { + frames: expect.any(Array), + }, + mechanism: { type: 'auto.http.cloudflare', handled: false }, + }, + ], + }, + request: { + headers: expect.any(Object), + method: 'GET', + url: expect.any(String), + }, + }), + ) + .start(signal); + await runner.makeRequest('get', '/error', { expectError: true }); + await runner.completed(); +}); + +it('Successful response works when withSentry is called twice', async ({ signal }) => { + const runner = createRunner(__dirname).start(signal); + const response = await runner.makeRequest('get', '/'); + expect(response).toBe('ok'); +}); diff --git a/dev-packages/cloudflare-integration-tests/suites/double-instrumentation/wrangler.jsonc b/dev-packages/cloudflare-integration-tests/suites/double-instrumentation/wrangler.jsonc new file mode 100644 index 000000000000..d6be01281f0c --- /dev/null +++ b/dev-packages/cloudflare-integration-tests/suites/double-instrumentation/wrangler.jsonc @@ -0,0 +1,6 @@ +{ + "name": "worker-name", + "compatibility_date": "2025-06-17", + "main": "index.ts", + "compatibility_flags": ["nodejs_compat"], +} diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/index.ts b/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/index.ts new file mode 100644 index 000000000000..a7729d8af7f8 --- /dev/null +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/index.ts @@ -0,0 +1,46 @@ +import * as Sentry from '@sentry/cloudflare'; +import { DurableObject } from 'cloudflare:workers'; + +interface Env { + SENTRY_DSN: string; + TEST_DURABLE_OBJECT: DurableObjectNamespace; +} + +class TestDurableObjectBase extends DurableObject { + public constructor(ctx: DurableObjectState, env: Env) { + super(ctx, env); + } + + // eslint-disable-next-line @typescript-eslint/explicit-member-accessibility + async doWork(): Promise { + const results: string[] = []; + + for (let i = 1; i <= 5; i++) { + await Sentry.startSpan({ name: `task-${i}`, op: 'task' }, async () => { + // Simulate async work + await new Promise(resolve => setTimeout(resolve, 1)); + results.push(`done-${i}`); + }); + } + + return results.join(','); + } +} + +export const TestDurableObject = Sentry.instrumentDurableObjectWithSentry( + (env: Env) => ({ + dsn: env.SENTRY_DSN, + tracesSampleRate: 1.0, + instrumentPrototypeMethods: true, + }), + TestDurableObjectBase, +); + +export default { + async fetch(_request: Request, env: Env): Promise { + const id: DurableObjectId = env.TEST_DURABLE_OBJECT.idFromName('test'); + const stub = env.TEST_DURABLE_OBJECT.get(id) as unknown as TestDurableObjectBase; + const result = await stub.doWork(); + return new Response(result); + }, +}; diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/test.ts b/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/test.ts new file mode 100644 index 000000000000..795eb03e27c2 --- /dev/null +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/test.ts @@ -0,0 +1,54 @@ +import { expect, it } from 'vitest'; +import { createRunner } from '../../../runner'; + +// Regression test for https://github.com/getsentry/sentry-javascript/issues/20030 +// When a Durable Object method calls Sentry.startSpan multiple times, those spans +// must appear as children of the DO transaction. The first invocation always worked; +// the second invocation on the same DO instance previously lost its child spans +// because the client was disposed after the first call. +it('sends child spans on repeated Durable Object calls', async ({ signal }) => { + function assertDoWorkEnvelope(envelope: unknown): void { + const transactionEvent = (envelope as any)[1]?.[0]?.[1]; + + expect(transactionEvent).toEqual( + expect.objectContaining({ + transaction: 'doWork', + contexts: expect.objectContaining({ + trace: expect.objectContaining({ + op: 'rpc', + origin: 'auto.faas.cloudflare.durable_object', + }), + }), + }), + ); + + // All 5 child spans should be present + expect(transactionEvent.spans).toHaveLength(5); + expect(transactionEvent.spans).toEqual( + expect.arrayContaining([ + expect.objectContaining({ description: 'task-1', op: 'task' }), + expect.objectContaining({ description: 'task-2', op: 'task' }), + expect.objectContaining({ description: 'task-3', op: 'task' }), + expect.objectContaining({ description: 'task-4', op: 'task' }), + expect.objectContaining({ description: 'task-5', op: 'task' }), + ]), + ); + + // All child spans share the root trace_id + const rootTraceId = transactionEvent.contexts?.trace?.trace_id; + expect(rootTraceId).toBeDefined(); + for (const span of transactionEvent.spans) { + expect(span.trace_id).toBe(rootTraceId); + } + } + + // Expect 5 transaction envelopes — one per call. + const runner = createRunner(__dirname).expectN(5, assertDoWorkEnvelope).start(signal); + + await runner.makeRequest('get', '/'); + await runner.makeRequest('get', '/'); + await runner.makeRequest('get', '/'); + await runner.makeRequest('get', '/'); + await runner.makeRequest('get', '/'); + await runner.completed(); +}); diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/wrangler.jsonc b/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/wrangler.jsonc new file mode 100644 index 000000000000..8a544e1bdf6b --- /dev/null +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/wrangler.jsonc @@ -0,0 +1,20 @@ +{ + "name": "worker-name", + "main": "index.ts", + "compatibility_date": "2025-06-17", + "migrations": [ + { + "new_sqlite_classes": ["TestDurableObject"], + "tag": "v1", + }, + ], + "durable_objects": { + "bindings": [ + { + "class_name": "TestDurableObject", + "name": "TEST_DURABLE_OBJECT", + }, + ], + }, + "compatibility_flags": ["nodejs_als"], +} diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject/wrangler.jsonc b/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject/wrangler.jsonc index 31cf0ff361ea..8a544e1bdf6b 100644 --- a/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject/wrangler.jsonc +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/durableobject/wrangler.jsonc @@ -17,7 +17,4 @@ ], }, "compatibility_flags": ["nodejs_als"], - "vars": { - "SENTRY_DSN": "https://932e620ee3921c3b4a61c72558ad88ce@o447951.ingest.us.sentry.io/4509553159831552", - }, } diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/worker-service-binding/wrangler-sub-worker.jsonc b/dev-packages/cloudflare-integration-tests/suites/tracing/worker-service-binding/wrangler-sub-worker.jsonc index 30bd95322560..2ba9611e4c48 100644 --- a/dev-packages/cloudflare-integration-tests/suites/tracing/worker-service-binding/wrangler-sub-worker.jsonc +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/worker-service-binding/wrangler-sub-worker.jsonc @@ -3,7 +3,4 @@ "main": "index-sub-worker.ts", "compatibility_date": "2025-06-17", "compatibility_flags": ["nodejs_als"], - "vars": { - "SENTRY_DSN": "https://932e620ee3921c3b4a61c72558ad88ce@o447951.ingest.us.sentry.io/4509553159831552", - }, } diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/worker-service-binding/wrangler.jsonc b/dev-packages/cloudflare-integration-tests/suites/tracing/worker-service-binding/wrangler.jsonc index 69bc9f6e6e99..a2d52620d268 100644 --- a/dev-packages/cloudflare-integration-tests/suites/tracing/worker-service-binding/wrangler.jsonc +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/worker-service-binding/wrangler.jsonc @@ -3,9 +3,6 @@ "main": "index.ts", "compatibility_date": "2025-06-17", "compatibility_flags": ["nodejs_als"], - "vars": { - "SENTRY_DSN": "https://932e620ee3921c3b4a61c72558ad88ce@o447951.ingest.us.sentry.io/4509553159831552", - }, "services": [ { "binding": "ANOTHER_WORKER", diff --git a/packages/cloudflare/src/durableobject.ts b/packages/cloudflare/src/durableobject.ts index fc07cb46ca00..af60ab5e59e0 100644 --- a/packages/cloudflare/src/durableobject.ts +++ b/packages/cloudflare/src/durableobject.ts @@ -3,7 +3,7 @@ import { captureException } from '@sentry/core'; import type { DurableObject } from 'cloudflare:workers'; import { setAsyncLocalStorageAsyncContextStrategy } from './async'; import type { CloudflareOptions } from './client'; -import { isInstrumented, markAsInstrumented } from './instrument'; +import { ensureInstrumented, getInstrumented, markAsInstrumented } from './instrument'; import { getFinalOptions } from './options'; import { wrapRequestHandler } from './request'; import { instrumentContext } from './utils/instrumentContext'; @@ -67,16 +67,18 @@ export function instrumentDurableObjectWithSentry< // Any other public methods on the Durable Object instance are RPC calls. - if (obj.fetch && typeof obj.fetch === 'function' && !isInstrumented(obj.fetch)) { - obj.fetch = new Proxy(obj.fetch, { - apply(target, thisArg, args) { - return wrapRequestHandler({ options, request: args[0], context }, () => - Reflect.apply(target, thisArg, args), - ); - }, - }); - - markAsInstrumented(obj.fetch); + if (obj.fetch && typeof obj.fetch === 'function') { + obj.fetch = ensureInstrumented( + obj.fetch, + original => + new Proxy(original, { + apply(target, thisArg, args) { + return wrapRequestHandler({ options, request: args[0], context }, () => { + return Reflect.apply(target, thisArg, args); + }); + }, + }), + ); } if (obj.alarm && typeof obj.alarm === 'function') { @@ -177,7 +179,18 @@ function instrumentPrototype(target: T, methodsToInst methodNames.forEach(methodName => { const originalMethod = (proto as Record)[methodName]; - if (!originalMethod || isInstrumented(originalMethod)) { + if (!originalMethod) { + return; + } + + const existingInstrumented = getInstrumented(originalMethod); + if (existingInstrumented) { + Object.defineProperty(proto, methodName, { + value: existingInstrumented, + enumerable: false, + writable: true, + configurable: true, + }); return; } @@ -216,6 +229,11 @@ function instrumentPrototype(target: T, methodsToInst return wrapper.apply(this, args); }; + // Only mark wrappedMethod as instrumented (not originalMethod → wrappedMethod). + // originalMethod must stay unmapped because wrappedMethod calls + // wrapMethodWithSentry(options, originalMethod) on each invocation to create + // a per-instance proxy. If originalMethod mapped to wrappedMethod, that call + // would return wrappedMethod itself, causing infinite recursion. markAsInstrumented(wrappedMethod); // Replace the prototype method diff --git a/packages/cloudflare/src/instrument.ts b/packages/cloudflare/src/instrument.ts index 1ebe4262644a..5a3a472b306e 100644 --- a/packages/cloudflare/src/instrument.ts +++ b/packages/cloudflare/src/instrument.ts @@ -1,25 +1,84 @@ -type SentryInstrumented = T & { - __SENTRY_INSTRUMENTED__?: boolean; -}; +// Use a global WeakMap to track instrumented objects across module instances. +// This is important for Cloudflare Workers where the module may be bundled +// separately for workers and Durable Objects, but they share the same global scope. +// The WeakMap stores original -> instrumented mappings so we can retrieve the +// instrumented version even if we only have a reference to the original. +const GLOBAL_KEY = '__SENTRY_INSTRUMENTED_MAP__' as const; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +function getInstrumentedMap(): WeakMap { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const globalObj = globalThis as typeof globalThis & { [GLOBAL_KEY]?: WeakMap }; + if (!globalObj[GLOBAL_KEY]) { + globalObj[GLOBAL_KEY] = new WeakMap(); + } + return globalObj[GLOBAL_KEY]; +} /** - * Mark an object as instrumented. + * Check if a value can be used as a WeakMap key. + * WeakMap keys must be objects or non-registered symbols. */ -export function markAsInstrumented(obj: T): void { +function isWeakMapKey(value: unknown): value is object { + return (typeof value === 'object' && value !== null) || typeof value === 'function'; +} + +/** + * Mark an object as instrumented, storing the instrumented version. + * @param original The original uninstrumented object + * @param instrumented The instrumented version (defaults to original if not provided) + */ +export function markAsInstrumented(original: T, instrumented?: T): void { try { - (obj as SentryInstrumented).__SENTRY_INSTRUMENTED__ = true; + if (isWeakMapKey(original)) { + // Store mapping from original to instrumented version + // If instrumented is not provided, store original (for backwards compat) + getInstrumentedMap().set(original, instrumented ?? original); + } + // Also mark the instrumented version itself so we recognize it + if (isWeakMapKey(instrumented) && instrumented !== original) { + getInstrumentedMap().set(instrumented, instrumented); + } } catch { // ignore errors here } } /** - * Check if an object is instrumented. + * Get the instrumented version of an object, if available. + * Returns the instrumented version if the object was previously instrumented, + * or undefined if not found. */ -export function isInstrumented(obj: T): boolean | undefined { +export function getInstrumented(obj: T): T | undefined { try { - return (obj as SentryInstrumented).__SENTRY_INSTRUMENTED__; + if (isWeakMapKey(obj)) { + return getInstrumentedMap().get(obj) as T | undefined; + } + + return undefined; } catch { - return false; + return undefined; + } +} + +/** + * Returns the already-instrumented version of `original` if one exists, + * otherwise calls `instrumentFn` to create it, marks the mapping, and returns it. + * + * @param noMark - If true, skips storing the original→instrumented mapping. + */ +export function ensureInstrumented(original: T, instrumentFn: (original: T) => T, noMark?: boolean): T { + const existing = getInstrumented(original); + + if (existing) { + return existing; } + + const instrumented = instrumentFn(original); + + if (!noMark) { + markAsInstrumented(original, instrumented); + } + + return instrumented; } diff --git a/packages/cloudflare/src/instrumentations/worker/instrumentEmail.ts b/packages/cloudflare/src/instrumentations/worker/instrumentEmail.ts index 8c91bf2cb3d2..ed4c7779b251 100644 --- a/packages/cloudflare/src/instrumentations/worker/instrumentEmail.ts +++ b/packages/cloudflare/src/instrumentations/worker/instrumentEmail.ts @@ -8,7 +8,7 @@ import { } from '@sentry/core'; import type { CloudflareOptions } from '../../client'; import { flushAndDispose } from '../../flush'; -import { isInstrumented, markAsInstrumented } from '../../instrument'; +import { ensureInstrumented } from '../../instrument'; import { getFinalOptions } from '../../options'; import { addCloudResourceContext } from '../../scope-utils'; import { init } from '../../sdk'; @@ -63,21 +63,23 @@ export function instrumentExportedHandlerEmail>[1]) => CloudflareOptions | undefined, ): void { - if (!('email' in handler) || typeof handler.email !== 'function' || isInstrumented(handler.email)) { + if (!('email' in handler) || typeof handler.email !== 'function') { return; } - handler.email = new Proxy(handler.email, { - apply(target, thisArg, args: Parameters>) { - const [emailMessage, env, ctx] = args; - const context = instrumentContext(ctx); - args[2] = context; + handler.email = ensureInstrumented( + handler.email, + original => + new Proxy(original, { + apply(target, thisArg, args: Parameters>) { + const [emailMessage, env, ctx] = args; + const context = instrumentContext(ctx); + args[2] = context; - const options = getFinalOptions(optionsCallback(env), env); + const options = getFinalOptions(optionsCallback(env), env); - return wrapEmailHandler(emailMessage, options, context, () => target.apply(thisArg, args)); - }, - }); - - markAsInstrumented(handler.email); + return wrapEmailHandler(emailMessage, options, context, () => target.apply(thisArg, args)); + }, + }), + ); } diff --git a/packages/cloudflare/src/instrumentations/worker/instrumentFetch.ts b/packages/cloudflare/src/instrumentations/worker/instrumentFetch.ts index be58fa07e18f..eb681bb8230e 100644 --- a/packages/cloudflare/src/instrumentations/worker/instrumentFetch.ts +++ b/packages/cloudflare/src/instrumentations/worker/instrumentFetch.ts @@ -1,6 +1,6 @@ import type { ExportedHandler } from '@cloudflare/workers-types'; import type { CloudflareOptions } from '../../client'; -import { isInstrumented, markAsInstrumented } from '../../instrument'; +import { ensureInstrumented } from '../../instrument'; import { getFinalOptions } from '../../options'; import { wrapRequestHandler } from '../../request'; import { instrumentContext } from '../../utils/instrumentContext'; @@ -13,21 +13,23 @@ export function instrumentExportedHandlerFetch>[1]) => CloudflareOptions | undefined, ): void { - if (!('fetch' in handler) || typeof handler.fetch !== 'function' || isInstrumented(handler.fetch)) { + if (!('fetch' in handler) || typeof handler.fetch !== 'function') { return; } - handler.fetch = new Proxy(handler.fetch, { - apply(target, thisArg, args: Parameters>) { - const [request, env, ctx] = args; - const context = instrumentContext(ctx); - args[2] = context; + handler.fetch = ensureInstrumented( + handler.fetch, + original => + new Proxy(original, { + apply(target, thisArg, args: Parameters>) { + const [request, env, ctx] = args; + const context = instrumentContext(ctx); + args[2] = context; - const options = getFinalOptions(optionsCallback(env), env); + const options = getFinalOptions(optionsCallback(env), env); - return wrapRequestHandler({ options, request, context }, () => target.apply(thisArg, args)); - }, - }); - - markAsInstrumented(handler.fetch); + return wrapRequestHandler({ options, request, context }, () => target.apply(thisArg, args)); + }, + }), + ); } diff --git a/packages/cloudflare/src/instrumentations/worker/instrumentQueue.ts b/packages/cloudflare/src/instrumentations/worker/instrumentQueue.ts index 366fb7e98f51..08435db8f90a 100644 --- a/packages/cloudflare/src/instrumentations/worker/instrumentQueue.ts +++ b/packages/cloudflare/src/instrumentations/worker/instrumentQueue.ts @@ -9,7 +9,7 @@ import { } from '@sentry/core'; import type { CloudflareOptions } from '../../client'; import { flushAndDispose } from '../../flush'; -import { isInstrumented, markAsInstrumented } from '../../instrument'; +import { ensureInstrumented } from '../../instrument'; import { getFinalOptions } from '../../options'; import { addCloudResourceContext } from '../../scope-utils'; import { init } from '../../sdk'; @@ -69,21 +69,23 @@ export function instrumentExportedHandlerQueue>[1]) => CloudflareOptions | undefined, ): void { - if (!('queue' in handler) || typeof handler.queue !== 'function' || isInstrumented(handler.queue)) { + if (!('queue' in handler) || typeof handler.queue !== 'function') { return; } - handler.queue = new Proxy(handler.queue, { - apply(target, thisArg, args: Parameters>) { - const [batch, env, ctx] = args; - const context = instrumentContext(ctx); - args[2] = context; + handler.queue = ensureInstrumented( + handler.queue, + original => + new Proxy(original, { + apply(target, thisArg, args: Parameters>) { + const [batch, env, ctx] = args; + const context = instrumentContext(ctx); + args[2] = context; - const options = getFinalOptions(optionsCallback(env), env); + const options = getFinalOptions(optionsCallback(env), env); - return wrapQueueHandler(batch, options, context, () => target.apply(thisArg, args)); - }, - }); - - markAsInstrumented(handler.queue); + return wrapQueueHandler(batch, options, context, () => target.apply(thisArg, args)); + }, + }), + ); } diff --git a/packages/cloudflare/src/instrumentations/worker/instrumentScheduled.ts b/packages/cloudflare/src/instrumentations/worker/instrumentScheduled.ts index 2ef682829bcb..58f06e8aa5eb 100644 --- a/packages/cloudflare/src/instrumentations/worker/instrumentScheduled.ts +++ b/packages/cloudflare/src/instrumentations/worker/instrumentScheduled.ts @@ -8,7 +8,7 @@ import { } from '@sentry/core'; import type { CloudflareOptions } from '../../client'; import { flushAndDispose } from '../../flush'; -import { isInstrumented, markAsInstrumented } from '../../instrument'; +import { ensureInstrumented } from '../../instrument'; import { getFinalOptions } from '../../options'; import { addCloudResourceContext } from '../../scope-utils'; import { init } from '../../sdk'; @@ -65,21 +65,23 @@ export function instrumentExportedHandlerScheduled>[1]) => CloudflareOptions | undefined, ): void { - if (!('scheduled' in handler) || typeof handler.scheduled !== 'function' || isInstrumented(handler.scheduled)) { + if (!('scheduled' in handler) || typeof handler.scheduled !== 'function') { return; } - handler.scheduled = new Proxy(handler.scheduled, { - apply(target, thisArg, args: Parameters>) { - const [controller, env, ctx] = args; - const context = instrumentContext(ctx); - args[2] = context; + handler.scheduled = ensureInstrumented( + handler.scheduled, + original => + new Proxy(original, { + apply(target, thisArg, args: Parameters>) { + const [controller, env, ctx] = args; + const context = instrumentContext(ctx); + args[2] = context; - const options = getFinalOptions(optionsCallback(env), env); + const options = getFinalOptions(optionsCallback(env), env); - return wrapScheduledHandler(controller, options, context, () => target.apply(thisArg, args)); - }, - }); - - markAsInstrumented(handler.scheduled); + return wrapScheduledHandler(controller, options, context, () => target.apply(thisArg, args)); + }, + }), + ); } diff --git a/packages/cloudflare/src/instrumentations/worker/instrumentTail.ts b/packages/cloudflare/src/instrumentations/worker/instrumentTail.ts index f6b2e4492106..5a03965ba969 100644 --- a/packages/cloudflare/src/instrumentations/worker/instrumentTail.ts +++ b/packages/cloudflare/src/instrumentations/worker/instrumentTail.ts @@ -2,7 +2,7 @@ import type { ExportedHandler } from '@cloudflare/workers-types'; import { captureException, withIsolationScope } from '@sentry/core'; import type { CloudflareOptions } from '../../client'; import { flushAndDispose } from '../../flush'; -import { isInstrumented, markAsInstrumented } from '../../instrument'; +import { ensureInstrumented } from '../../instrument'; import { getFinalOptions } from '../../options'; import { addCloudResourceContext } from '../../scope-utils'; import { init } from '../../sdk'; @@ -40,21 +40,23 @@ export function instrumentExportedHandlerTail>[1]) => CloudflareOptions | undefined, ): void { - if (!('tail' in handler) || typeof handler.tail !== 'function' || isInstrumented(handler.tail)) { + if (!('tail' in handler) || typeof handler.tail !== 'function') { return; } - handler.tail = new Proxy(handler.tail, { - apply(target, thisArg, args: Parameters>) { - const [, env, ctx] = args; - const context = instrumentContext(ctx); - args[2] = context; - - const options = getFinalOptions(optionsCallback(env), env); - - return wrapTailHandler(options, context, () => target.apply(thisArg, args)); - }, - }); - - markAsInstrumented(handler.tail); + handler.tail = ensureInstrumented( + handler.tail, + original => + new Proxy(original, { + apply(target, thisArg, args: Parameters>) { + const [, env, ctx] = args; + const context = instrumentContext(ctx); + args[2] = context; + + const options = getFinalOptions(optionsCallback(env), env); + + return wrapTailHandler(options, context, () => target.apply(thisArg, args)); + }, + }), + ); } diff --git a/packages/cloudflare/src/withSentry.ts b/packages/cloudflare/src/withSentry.ts index 4655ab1a154d..695905f14704 100644 --- a/packages/cloudflare/src/withSentry.ts +++ b/packages/cloudflare/src/withSentry.ts @@ -1,7 +1,7 @@ import type { env } from 'cloudflare:workers'; import { setAsyncLocalStorageAsyncContextStrategy } from './async'; import type { CloudflareOptions } from './client'; -import { isInstrumented, markAsInstrumented } from './instrument'; +import { ensureInstrumented } from './instrument'; import { instrumentExportedHandlerEmail } from './instrumentations/worker/instrumentEmail'; import { instrumentExportedHandlerFetch } from './instrumentations/worker/instrumentFetch'; import { instrumentExportedHandlerQueue } from './instrumentations/worker/instrumentQueue'; @@ -49,22 +49,19 @@ export function withSentry< // eslint-disable-next-line @typescript-eslint/no-explicit-any function instrumentHonoErrorHandler>(handler: T): void { - if ( - 'onError' in handler && - 'errorHandler' in handler && - typeof handler.errorHandler === 'function' && - !isInstrumented(handler.errorHandler) - ) { - handler.errorHandler = new Proxy(handler.errorHandler, { - apply(target, thisArg, args) { - const [err, context] = args; + if ('onError' in handler && 'errorHandler' in handler && typeof handler.errorHandler === 'function') { + handler.errorHandler = ensureInstrumented( + handler.errorHandler, + original => + new Proxy(original, { + apply(target, thisArg, args) { + const [err, context] = args; - getHonoIntegration()?.handleHonoException(err, context); + getHonoIntegration()?.handleHonoException(err, context); - return Reflect.apply(target, thisArg, args); - }, - }); - - markAsInstrumented(handler.errorHandler); + return Reflect.apply(target, thisArg, args); + }, + }), + ); } } diff --git a/packages/cloudflare/src/wrapMethodWithSentry.ts b/packages/cloudflare/src/wrapMethodWithSentry.ts index 2361ee5b718d..4080017526a2 100644 --- a/packages/cloudflare/src/wrapMethodWithSentry.ts +++ b/packages/cloudflare/src/wrapMethodWithSentry.ts @@ -12,7 +12,7 @@ import { } from '@sentry/core'; import type { CloudflareOptions } from './client'; import { flushAndDispose } from './flush'; -import { isInstrumented, markAsInstrumented } from './instrument'; +import { ensureInstrumented } from './instrument'; import { init } from './sdk'; /** Extended DurableObjectState with originalStorage exposed by instrumentContext */ @@ -46,126 +46,126 @@ export function wrapMethodWithSentry( callback?: (...args: Parameters) => void, noMark?: true, ): T { - if (isInstrumented(handler)) { - return handler; - } - - if (!noMark) { - markAsInstrumented(handler); - } - - return new Proxy(handler, { - apply(target, thisArg, args: Parameters) { - const currentClient = getClient(); - // if a client is already set, use withScope, otherwise use withIsolationScope - const sentryWithScope = currentClient ? withScope : withIsolationScope; - - const wrappedFunction = (scope: Scope): unknown => { - // In certain situations, the passed context can become undefined. - // For example, for Astro while prerendering pages at build time. - // see: https://github.com/getsentry/sentry-javascript/issues/13217 - const context = wrapperOptions.context as InstrumentedDurableObjectState | undefined; - - const waitUntil = context?.waitUntil?.bind?.(context); - - let currentClient = scope.getClient(); - // Check if client exists AND is still usable (transport not disposed) - // This handles the case where a previous handler disposed the client - // but the scope still holds a reference to it (e.g., alarm handlers in Durable Objects) - if (!currentClient?.getTransport()) { - const client = init({ ...wrapperOptions.options, ctx: context as unknown as ExecutionContext | undefined }); - scope.setClient(client); - currentClient = client; - } + return ensureInstrumented( + handler, + original => + new Proxy(original, { + apply(target, thisArg, args: Parameters) { + const currentClient = getClient(); + // if a client is already set, use withScope, otherwise use withIsolationScope + const sentryWithScope = currentClient ? withScope : withIsolationScope; + + const wrappedFunction = (scope: Scope): unknown => { + // In certain situations, the passed context can become undefined. + // For example, for Astro while prerendering pages at build time. + // see: https://github.com/getsentry/sentry-javascript/issues/13217 + const context = wrapperOptions.context as InstrumentedDurableObjectState | undefined; + + const waitUntil = context?.waitUntil?.bind?.(context); + + let currentClient = scope.getClient(); + // Check if client exists AND is still usable (transport not disposed) + // This handles the case where a previous handler disposed the client + // but the scope still holds a reference to it (e.g., alarm handlers in Durable Objects) + if (!currentClient?.getTransport()) { + const client = init({ + ...wrapperOptions.options, + ctx: context as unknown as ExecutionContext | undefined, + }); + scope.setClient(client); + currentClient = client; + } - const clientToDispose = currentClient; + const clientToDispose = currentClient; - if (!wrapperOptions.spanName) { - try { - if (callback) { - callback(...args); - } - const result = Reflect.apply(target, thisArg, args); + if (!wrapperOptions.spanName) { + try { + if (callback) { + callback(...args); + } + const result = Reflect.apply(target, thisArg, args); - if (isThenable(result)) { - return result.then( - (res: unknown) => { - waitUntil?.(flushAndDispose(clientToDispose)); - return res; - }, - (e: unknown) => { - captureException(e, { - mechanism: { - type: 'auto.faas.cloudflare.durable_object', - handled: false, + if (isThenable(result)) { + return result.then( + (res: unknown) => { + waitUntil?.(flushAndDispose(clientToDispose)); + return res; }, - }); + (e: unknown) => { + captureException(e, { + mechanism: { + type: 'auto.faas.cloudflare.durable_object', + handled: false, + }, + }); + waitUntil?.(flushAndDispose(clientToDispose)); + throw e; + }, + ); + } else { waitUntil?.(flushAndDispose(clientToDispose)); - throw e; - }, - ); - } else { - waitUntil?.(flushAndDispose(clientToDispose)); - return result; - } - } catch (e) { - captureException(e, { - mechanism: { - type: 'auto.faas.cloudflare.durable_object', - handled: false, - }, - }); - waitUntil?.(flushAndDispose(clientToDispose)); - throw e; - } - } - - const attributes = wrapperOptions.spanOp - ? { - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: wrapperOptions.spanOp, - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.faas.cloudflare.durable_object', + return result; + } + } catch (e) { + captureException(e, { + mechanism: { + type: 'auto.faas.cloudflare.durable_object', + handled: false, + }, + }); + waitUntil?.(flushAndDispose(clientToDispose)); + throw e; + } } - : {}; - return startSpan({ name: wrapperOptions.spanName, attributes }, () => { - try { - const result = Reflect.apply(target, thisArg, args); - - if (isThenable(result)) { - return result.then( - (res: unknown) => { - waitUntil?.(flushAndDispose(clientToDispose)); - return res; - }, - (e: unknown) => { - captureException(e, { - mechanism: { - type: 'auto.faas.cloudflare.durable_object', - handled: false, + const attributes = wrapperOptions.spanOp + ? { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: wrapperOptions.spanOp, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.faas.cloudflare.durable_object', + } + : {}; + + return startSpan({ name: wrapperOptions.spanName, attributes }, () => { + try { + const result = Reflect.apply(target, thisArg, args); + + if (isThenable(result)) { + return result.then( + (res: unknown) => { + waitUntil?.(flushAndDispose(clientToDispose)); + return res; }, - }); + (e: unknown) => { + captureException(e, { + mechanism: { + type: 'auto.faas.cloudflare.durable_object', + handled: false, + }, + }); + waitUntil?.(flushAndDispose(clientToDispose)); + throw e; + }, + ); + } else { waitUntil?.(flushAndDispose(clientToDispose)); - throw e; - }, - ); - } else { - waitUntil?.(flushAndDispose(clientToDispose)); - return result; - } - } catch (e) { - captureException(e, { - mechanism: { - type: 'auto.faas.cloudflare.durable_object', - handled: false, - }, + return result; + } + } catch (e) { + captureException(e, { + mechanism: { + type: 'auto.faas.cloudflare.durable_object', + handled: false, + }, + }); + waitUntil?.(flushAndDispose(clientToDispose)); + throw e; + } }); - waitUntil?.(flushAndDispose(clientToDispose)); - throw e; - } - }); - }; + }; - return sentryWithScope(wrappedFunction); - }, - }); + return sentryWithScope(wrappedFunction); + }, + }), + noMark, + ); } diff --git a/packages/cloudflare/test/durableobject.test.ts b/packages/cloudflare/test/durableobject.test.ts index d665abf95c86..efce592a6cdd 100644 --- a/packages/cloudflare/test/durableobject.test.ts +++ b/packages/cloudflare/test/durableobject.test.ts @@ -2,7 +2,7 @@ import type { ExecutionContext } from '@cloudflare/workers-types'; import * as SentryCore from '@sentry/core'; import { afterEach, describe, expect, it, onTestFinished, vi } from 'vitest'; import { instrumentDurableObjectWithSentry } from '../src'; -import { isInstrumented } from '../src/instrument'; +import { getInstrumented } from '../src/instrument'; describe('instrumentDurableObjectWithSentry', () => { afterEach(() => { @@ -111,7 +111,7 @@ describe('instrumentDurableObjectWithSentry', () => { 'webSocketError', 'rpcMethod', ]) { - expect(isInstrumented((obj as any)[method_name]), `Method ${method_name} is instrumented`).toBeTruthy(); + expect(getInstrumented((obj as any)[method_name]), `Method ${method_name} is instrumented`).toBeTruthy(); } }); @@ -176,7 +176,7 @@ describe('instrumentDurableObjectWithSentry', () => { const instrumented = instrumentDurableObjectWithSentry(options, testClass as any); const obj = Reflect.construct(instrumented, []); - expect(isInstrumented(obj.prototypeMethod)).toBeFalsy(); + expect(getInstrumented(obj.prototypeMethod)).toBeFalsy(); }); it('does not instrument prototype methods when option is false', () => { @@ -189,7 +189,7 @@ describe('instrumentDurableObjectWithSentry', () => { const instrumented = instrumentDurableObjectWithSentry(options, testClass as any); const obj = Reflect.construct(instrumented, []); - expect(isInstrumented(obj.prototypeMethod)).toBeFalsy(); + expect(getInstrumented(obj.prototypeMethod)).toBeFalsy(); }); it('instruments all prototype methods when option is true', () => { @@ -205,8 +205,8 @@ describe('instrumentDurableObjectWithSentry', () => { const instrumented = instrumentDurableObjectWithSentry(options, testClass as any); const obj = Reflect.construct(instrumented, []); - expect(isInstrumented(obj.methodOne)).toBeTruthy(); - expect(isInstrumented(obj.methodTwo)).toBeTruthy(); + expect(getInstrumented(obj.methodOne)).toBeTruthy(); + expect(getInstrumented(obj.methodTwo)).toBeTruthy(); }); it('instruments only specified methods when option is array', () => { @@ -225,9 +225,9 @@ describe('instrumentDurableObjectWithSentry', () => { const instrumented = instrumentDurableObjectWithSentry(options, testClass as any); const obj = Reflect.construct(instrumented, []); - expect(isInstrumented(obj.methodOne)).toBeTruthy(); - expect(isInstrumented(obj.methodTwo)).toBeFalsy(); - expect(isInstrumented(obj.methodThree)).toBeTruthy(); + expect(getInstrumented(obj.methodOne)).toBeTruthy(); + expect(getInstrumented(obj.methodTwo)).toBeFalsy(); + expect(getInstrumented(obj.methodThree)).toBeTruthy(); }); it('still instruments instance methods regardless of prototype option', () => { @@ -245,12 +245,12 @@ describe('instrumentDurableObjectWithSentry', () => { const obj = Reflect.construct(instrumented, []); // Instance methods should still be instrumented - expect(isInstrumented(obj.propertyFunction)).toBeTruthy(); - expect(isInstrumented(obj.fetch)).toBeTruthy(); - expect(isInstrumented(obj.alarm)).toBeTruthy(); - expect(isInstrumented(obj.webSocketMessage)).toBeTruthy(); - expect(isInstrumented(obj.webSocketClose)).toBeTruthy(); - expect(isInstrumented(obj.webSocketError)).toBeTruthy(); + expect(getInstrumented(obj.propertyFunction)).toBeTruthy(); + expect(getInstrumented(obj.fetch)).toBeTruthy(); + expect(getInstrumented(obj.alarm)).toBeTruthy(); + expect(getInstrumented(obj.webSocketMessage)).toBeTruthy(); + expect(getInstrumented(obj.webSocketClose)).toBeTruthy(); + expect(getInstrumented(obj.webSocketError)).toBeTruthy(); }); }); }); diff --git a/packages/cloudflare/test/instrument.test.ts b/packages/cloudflare/test/instrument.test.ts new file mode 100644 index 000000000000..659fdeec6374 --- /dev/null +++ b/packages/cloudflare/test/instrument.test.ts @@ -0,0 +1,196 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { getInstrumented, markAsInstrumented } from '../src/instrument'; + +// Clean up the global WeakMap between tests to avoid cross-test pollution +const GLOBAL_KEY = '__SENTRY_INSTRUMENTED_MAP__' as const; + +describe('instrument', () => { + beforeEach(() => { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete (globalThis as Record)[GLOBAL_KEY]; + }); + + afterEach(() => { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete (globalThis as Record)[GLOBAL_KEY]; + }); + + describe('markAsInstrumented', () => { + it('marks an object with itself when no instrumented version is provided', () => { + const obj = { name: 'test' }; + markAsInstrumented(obj); + + expect(getInstrumented(obj)).toBe(obj); + }); + + it('stores original -> instrumented mapping', () => { + const original = { name: 'original' }; + const instrumented = { name: 'instrumented' }; + markAsInstrumented(original, instrumented); + + expect(getInstrumented(original)).toBe(instrumented); + }); + + it('also marks the instrumented version as instrumented', () => { + const original = { name: 'original' }; + const instrumented = { name: 'instrumented' }; + markAsInstrumented(original, instrumented); + + expect(getInstrumented(instrumented)).toBe(instrumented); + }); + + it('works with functions', () => { + const original = function () {}; + const instrumented = function () {}; + markAsInstrumented(original, instrumented); + + expect(getInstrumented(original)).toBe(instrumented); + expect(getInstrumented(instrumented)).toBe(instrumented); + }); + + it('works with arrow functions', () => { + const original = () => {}; + const instrumented = () => {}; + markAsInstrumented(original, instrumented); + + expect(getInstrumented(original)).toBe(instrumented); + }); + + it('works with Proxy objects wrapping functions', () => { + const original = () => 'original'; + const proxy = new Proxy(original, { + apply(target, thisArg, args) { + return Reflect.apply(target, thisArg, args); + }, + }); + markAsInstrumented(original, proxy); + + expect(getInstrumented(original)).toBe(proxy); + }); + + it('ignores primitive values', () => { + // These should not throw + markAsInstrumented(null as unknown); + markAsInstrumented(undefined as unknown); + markAsInstrumented(42 as unknown); + markAsInstrumented('string' as unknown); + markAsInstrumented(true as unknown); + }); + }); + + describe('getInstrumented', () => { + it('returns undefined for unknown objects', () => { + expect(getInstrumented({ name: 'unknown' })).toBeUndefined(); + }); + + it('returns undefined for unknown functions', () => { + expect(getInstrumented(() => {})).toBeUndefined(); + }); + + it('returns the instrumented version for a marked original', () => { + const original = () => {}; + const instrumented = () => {}; + markAsInstrumented(original, instrumented); + + expect(getInstrumented(original)).toBe(instrumented); + }); + + it('returns itself for the instrumented version', () => { + const original = () => {}; + const instrumented = () => {}; + markAsInstrumented(original, instrumented); + + expect(getInstrumented(instrumented)).toBe(instrumented); + }); + + it('returns the object itself when marked without instrumented version', () => { + const obj = { name: 'test' }; + markAsInstrumented(obj); + expect(getInstrumented(obj)).toBe(obj); + }); + + it('returns undefined for null', () => { + expect(getInstrumented(null)).toBeUndefined(); + }); + + it('returns undefined for undefined', () => { + expect(getInstrumented(undefined)).toBeUndefined(); + }); + + it('returns undefined for primitives', () => { + expect(getInstrumented(42)).toBeUndefined(); + expect(getInstrumented('string')).toBeUndefined(); + }); + }); + + describe('WeakMap global isolation', () => { + it('uses a global WeakMap stored on globalThis', () => { + const obj = { name: 'test' }; + markAsInstrumented(obj); + + // The global key should exist + expect((globalThis as Record)[GLOBAL_KEY]).toBeDefined(); + expect((globalThis as Record)[GLOBAL_KEY]).toBeInstanceOf(WeakMap); + }); + + it('reuses the same WeakMap across calls', () => { + const obj1 = { name: 'test1' }; + const obj2 = { name: 'test2' }; + markAsInstrumented(obj1); + markAsInstrumented(obj2); + + expect(getInstrumented(obj1)).toBeDefined(); + expect(getInstrumented(obj2)).toBeDefined(); + }); + + it('uses existing WeakMap if already on globalThis', () => { + const existingMap = new WeakMap(); + const obj = { name: 'pre-existing' }; + existingMap.set(obj, obj); + (globalThis as Record)[GLOBAL_KEY] = existingMap; + + // Should find the pre-existing entry + expect(getInstrumented(obj)).toBeDefined(); + + // Adding new entries should use the same map + const newObj = { name: 'new' }; + markAsInstrumented(newObj); + expect(existingMap.has(newObj)).toBe(true); + }); + }); + + describe('double instrumentation prevention', () => { + it('getInstrumented returns cached proxy when original is re-encountered', () => { + const original = () => 'original'; + const proxy = new Proxy(original, { + apply(target, thisArg, args) { + return Reflect.apply(target, thisArg, args); + }, + }); + markAsInstrumented(original, proxy); + + // Simulates a second request encountering the same original + const cached = getInstrumented(original); + expect(cached).toBe(proxy); + + // The proxy should also be recognized + const cachedProxy = getInstrumented(proxy); + expect(cachedProxy).toBe(proxy); + }); + + it('prevents double-wrapping by recognizing instrumented functions', () => { + const original = () => 'original'; + const proxy = new Proxy(original, { + apply(target, thisArg, args) { + return Reflect.apply(target, thisArg, args); + }, + }); + markAsInstrumented(original, proxy); + + // Simulating the simplified handler pattern: + // if getInstrumented returns something, use it; otherwise create new proxy + const existing = getInstrumented(proxy); + expect(existing).toBe(proxy); + }); + }); +}); diff --git a/packages/cloudflare/test/instrumentations/worker/instrumentEmail.test.ts b/packages/cloudflare/test/instrumentations/worker/instrumentEmail.test.ts index 5d2f01b428df..5deb79d7d1c1 100644 --- a/packages/cloudflare/test/instrumentations/worker/instrumentEmail.test.ts +++ b/packages/cloudflare/test/instrumentations/worker/instrumentEmail.test.ts @@ -46,6 +46,23 @@ describe('instrumentEmail', () => { vi.clearAllMocks(); }); + test('does not double-wrap when withSentry is called twice', async () => { + const originalEmail = vi.fn(); + const handler = { + email: originalEmail, + } satisfies ExportedHandler; + + const optionsCallback = vi.fn().mockReturnValue({ dsn: MOCK_ENV.SENTRY_DSN }); + + const wrappedHandler1 = withSentry(optionsCallback, handler); + const firstEmail = wrappedHandler1.email; + + const wrappedHandler2 = withSentry(optionsCallback, handler); + const secondEmail = wrappedHandler2.email; + + expect(firstEmail).toBe(secondEmail); + }); + test('executes options callback with env', async () => { const handler = { email(_message, _env, _context) { diff --git a/packages/cloudflare/test/instrumentations/worker/instrumentFetch.test.ts b/packages/cloudflare/test/instrumentations/worker/instrumentFetch.test.ts index 1ae4b965d238..5486addb405c 100644 --- a/packages/cloudflare/test/instrumentations/worker/instrumentFetch.test.ts +++ b/packages/cloudflare/test/instrumentations/worker/instrumentFetch.test.ts @@ -32,6 +32,25 @@ describe('instrumentFetch', () => { vi.clearAllMocks(); }); + test('does not double-wrap when withSentry is called twice', async () => { + const originalFetch = vi.fn().mockReturnValue(new Response('test')); + const handler = { + fetch: originalFetch, + } satisfies ExportedHandler; + + const optionsCallback = vi.fn().mockReturnValue({ dsn: MOCK_ENV.SENTRY_DSN }); + + // First call instruments the handler + const wrappedHandler1 = withSentry(optionsCallback, handler); + const firstFetch = wrappedHandler1.fetch; + + // Second call should reuse the instrumented version + const wrappedHandler2 = withSentry(optionsCallback, handler); + const secondFetch = wrappedHandler2.fetch; + + expect(firstFetch).toBe(secondFetch); + }); + test('executes options callback with env', async () => { const handler = { fetch(_request, _env, _context) { diff --git a/packages/cloudflare/test/instrumentations/worker/instrumentQueue.test.ts b/packages/cloudflare/test/instrumentations/worker/instrumentQueue.test.ts index a38a8f24d79e..6930ff7180df 100644 --- a/packages/cloudflare/test/instrumentations/worker/instrumentQueue.test.ts +++ b/packages/cloudflare/test/instrumentations/worker/instrumentQueue.test.ts @@ -59,6 +59,23 @@ describe('instrumentQueue', () => { vi.clearAllMocks(); }); + test('does not double-wrap when withSentry is called twice', async () => { + const originalQueue = vi.fn(); + const handler = { + queue: originalQueue, + } satisfies ExportedHandler; + + const optionsCallback = vi.fn().mockReturnValue({ dsn: MOCK_ENV.SENTRY_DSN }); + + const wrappedHandler1 = withSentry(optionsCallback, handler); + const firstQueue = wrappedHandler1.queue; + + const wrappedHandler2 = withSentry(optionsCallback, handler); + const secondQueue = wrappedHandler2.queue; + + expect(firstQueue).toBe(secondQueue); + }); + test('executes options callback with env', async () => { const handler = { queue(_batch, _env, _context) { diff --git a/packages/cloudflare/test/instrumentations/worker/instrumentScheduled.test.ts b/packages/cloudflare/test/instrumentations/worker/instrumentScheduled.test.ts index 64833d70ddfb..6b0107a68aef 100644 --- a/packages/cloudflare/test/instrumentations/worker/instrumentScheduled.test.ts +++ b/packages/cloudflare/test/instrumentations/worker/instrumentScheduled.test.ts @@ -41,6 +41,23 @@ describe('instrumentScheduled', () => { vi.clearAllMocks(); }); + test('does not double-wrap when withSentry is called twice', async () => { + const originalScheduled = vi.fn(); + const handler = { + scheduled: originalScheduled, + } satisfies ExportedHandler; + + const optionsCallback = vi.fn().mockReturnValue({ dsn: MOCK_ENV.SENTRY_DSN }); + + const wrappedHandler1 = withSentry(optionsCallback, handler); + const firstScheduled = wrappedHandler1.scheduled; + + const wrappedHandler2 = withSentry(optionsCallback, handler); + const secondScheduled = wrappedHandler2.scheduled; + + expect(firstScheduled).toBe(secondScheduled); + }); + test('executes options callback with env', async () => { const handler = { scheduled(_controller, _env, _context) { diff --git a/packages/cloudflare/test/instrumentations/worker/instrumentTail.test.ts b/packages/cloudflare/test/instrumentations/worker/instrumentTail.test.ts index f85507e2c734..4f47dc3c62c7 100644 --- a/packages/cloudflare/test/instrumentations/worker/instrumentTail.test.ts +++ b/packages/cloudflare/test/instrumentations/worker/instrumentTail.test.ts @@ -60,6 +60,23 @@ describe('instrumentTail', () => { vi.clearAllMocks(); }); + test('does not double-wrap when withSentry is called twice', async () => { + const originalTail = vi.fn(); + const handler = { + tail: originalTail, + } satisfies ExportedHandler; + + const optionsCallback = vi.fn().mockReturnValue({ dsn: MOCK_ENV.SENTRY_DSN }); + + const wrappedHandler1 = withSentry(optionsCallback, handler); + const firstTail = wrappedHandler1.tail; + + const wrappedHandler2 = withSentry(optionsCallback, handler); + const secondTail = wrappedHandler2.tail; + + expect(firstTail).toBe(secondTail); + }); + test('executes options callback with env', async () => { const handler = { tail(_event, _env, _context) { diff --git a/packages/cloudflare/test/withSentry.test.ts b/packages/cloudflare/test/withSentry.test.ts index 95f312c9ec54..c4e1ed789d9f 100644 --- a/packages/cloudflare/test/withSentry.test.ts +++ b/packages/cloudflare/test/withSentry.test.ts @@ -102,5 +102,25 @@ describe('withSentry', () => { honoApp.errorHandler?.(error); expect(captureExceptionSpy).not.toHaveBeenCalled(); }); + + test('does not double-wrap errorHandler when withSentry is called twice', async () => { + const honoApp: HonoLikeApp = { + fetch(_request, _env, _context) { + return new Response('test'); + }, + onError() {}, + errorHandler(err: Error) { + return new Response(`Error: ${err.message}`, { status: 500 }); + }, + }; + + withSentry(env => ({ dsn: env.SENTRY_DSN }), honoApp); + const firstErrorHandler = honoApp.errorHandler; + + withSentry(env => ({ dsn: env.SENTRY_DSN }), honoApp); + const secondErrorHandler = honoApp.errorHandler; + + expect(firstErrorHandler).toBe(secondErrorHandler); + }); }); }); diff --git a/packages/cloudflare/test/wrapMethodWithSentry.test.ts b/packages/cloudflare/test/wrapMethodWithSentry.test.ts index 6c530da521c5..fdd3475d24c4 100644 --- a/packages/cloudflare/test/wrapMethodWithSentry.test.ts +++ b/packages/cloudflare/test/wrapMethodWithSentry.test.ts @@ -1,6 +1,6 @@ import * as sentryCore from '@sentry/core'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { isInstrumented } from '../src/instrument'; +import { getInstrumented } from '../src/instrument'; import * as sdk from '../src/sdk'; import { wrapMethodWithSentry } from '../src/wrapMethodWithSentry'; @@ -115,11 +115,11 @@ describe('wrapMethodWithSentry', () => { context: createMockContext(), }; - expect(isInstrumented(handler)).toBeUndefined(); + expect(getInstrumented(handler)).toBe(undefined); wrapMethodWithSentry(options, handler); - expect(isInstrumented(handler)).toBe(true); + expect(getInstrumented(handler)).toBeDefined(); }); it('does not re-wrap already instrumented handler', () => { @@ -134,6 +134,7 @@ describe('wrapMethodWithSentry', () => { // Should return the same wrapped function expect(wrapped2).toBe(wrapped1); + expect(getInstrumented(handler)).toBe(wrapped1); }); it('does not mark handler when noMark is true', () => { @@ -145,7 +146,7 @@ describe('wrapMethodWithSentry', () => { wrapMethodWithSentry(options, handler, undefined, true); - expect(isInstrumented(handler)).toBeFalsy(); + expect(getInstrumented(handler)).toBeUndefined(); }); });