diff --git a/packages/cloudflare/src/client.ts b/packages/cloudflare/src/client.ts index 224865e3731e..15e7da4dc575 100644 --- a/packages/cloudflare/src/client.ts +++ b/packages/cloudflare/src/client.ts @@ -1,5 +1,6 @@ import type { ClientOptions, Options, ServerRuntimeClientOptions } from '@sentry/core'; import { applySdkMetadata, ServerRuntimeClient } from '@sentry/core'; +import type { makeFlushLock } from './flush'; import type { CloudflareTransportOptions } from './transport'; /** @@ -8,7 +9,9 @@ import type { CloudflareTransportOptions } from './transport'; * @see CloudflareClientOptions for documentation on configuration options. * @see ServerRuntimeClient for usage documentation. */ -export class CloudflareClient extends ServerRuntimeClient { +export class CloudflareClient extends ServerRuntimeClient { + private readonly _flushLock: ReturnType | void; + /** * Creates a new Cloudflare SDK instance. * @param options Configuration options for this SDK. @@ -16,9 +19,10 @@ export class CloudflareClient extends ServerRuntimeClient} A promise that resolves to a boolean indicating whether the flush operation was successful. + */ + public async flush(timeout?: number): Promise { + if (this._flushLock) { + await this._flushLock.finalize(); + } + return super.flush(timeout); } } @@ -37,11 +56,15 @@ interface BaseCloudflareOptions {} * * @see @sentry/core Options for more information. */ -export interface CloudflareOptions extends Options, BaseCloudflareOptions {} +export interface CloudflareOptions extends Options, BaseCloudflareOptions { + ctx?: ExecutionContext; +} /** * Configuration options for the Sentry Cloudflare SDK Client class * * @see CloudflareClient for more information. */ -export interface CloudflareClientOptions extends ClientOptions, BaseCloudflareOptions {} +export interface CloudflareClientOptions extends ClientOptions, BaseCloudflareOptions { + flushLock?: ReturnType; +} diff --git a/packages/cloudflare/src/durableobject.ts b/packages/cloudflare/src/durableobject.ts index 35fbb5096a41..0e919977025d 100644 --- a/packages/cloudflare/src/durableobject.ts +++ b/packages/cloudflare/src/durableobject.ts @@ -47,9 +47,11 @@ function wrapMethodWithSentry any>( // see: https://github.com/getsentry/sentry-javascript/issues/13217 const context = wrapperOptions.context as ExecutionContext | undefined; + const waitUntil = context?.waitUntil?.bind?.(context); + const currentClient = scope.getClient(); if (!currentClient) { - const client = init(wrapperOptions.options); + const client = init({ ...wrapperOptions.options, ctx: context }); scope.setClient(client); } @@ -68,7 +70,7 @@ function wrapMethodWithSentry any>( }); throw e; } finally { - context?.waitUntil(flush(2000)); + waitUntil?.(flush(2000)); } } @@ -92,7 +94,7 @@ function wrapMethodWithSentry any>( }); throw e; } finally { - context?.waitUntil(flush(2000)); + waitUntil?.(flush(2000)); } }); }); diff --git a/packages/cloudflare/src/flush.ts b/packages/cloudflare/src/flush.ts new file mode 100644 index 000000000000..f38c805d0f8b --- /dev/null +++ b/packages/cloudflare/src/flush.ts @@ -0,0 +1,38 @@ +import type { ExecutionContext } from '@cloudflare/workers-types'; + +type FlushLock = { + readonly ready: Promise; + readonly finalize: () => Promise; +}; + +/** + * Enhances the given execution context by wrapping its `waitUntil` method with a proxy + * to monitor pending tasks, and provides a flusher function to ensure all tasks + * have been completed before executing any subsequent logic. + * + * @param {ExecutionContext} context - The execution context to be enhanced. If no context is provided, the function returns undefined. + * @return {FlushLock} Returns a flusher function if a valid context is provided, otherwise undefined. + */ +export function makeFlushLock(context: ExecutionContext): FlushLock { + let resolveAllDone: () => void = () => undefined; + const allDone = new Promise(res => { + resolveAllDone = res; + }); + let pending = 0; + const originalWaitUntil = context.waitUntil.bind(context) as typeof context.waitUntil; + context.waitUntil = promise => { + pending++; + return originalWaitUntil( + promise.finally(() => { + if (--pending === 0) resolveAllDone(); + }), + ); + }; + return Object.freeze({ + ready: allDone, + finalize: () => { + if (pending === 0) resolveAllDone(); + return allDone; + }, + }); +} diff --git a/packages/cloudflare/src/handler.ts b/packages/cloudflare/src/handler.ts index d3d1f80dbbd5..3640d3cf7229 100644 --- a/packages/cloudflare/src/handler.ts +++ b/packages/cloudflare/src/handler.ts @@ -74,8 +74,9 @@ export function withSentry { const options = getFinalOptions(optionsCallback(env), env); + const waitUntil = context.waitUntil.bind(context); - const client = init(options); + const client = init({ ...options, ctx: context }); isolationScope.setClient(client); addCloudResourceContext(isolationScope); @@ -99,7 +100,7 @@ export function withSentry { const options = getFinalOptions(optionsCallback(env), env); + const waitUntil = context.waitUntil.bind(context); - const client = init(options); + const client = init({ ...options, ctx: context }); isolationScope.setClient(client); addCloudResourceContext(isolationScope); @@ -139,7 +141,7 @@ export function withSentry { const options = getFinalOptions(optionsCallback(env), env); + const waitUntil = context.waitUntil.bind(context); - const client = init(options); + const client = init({ ...options, ctx: context }); isolationScope.setClient(client); addCloudResourceContext(isolationScope); @@ -185,7 +188,7 @@ export function withSentry { const options = getFinalOptions(optionsCallback(env), env); - const client = init(options); + const waitUntil = context.waitUntil.bind(context); + + const client = init({ ...options, ctx: context }); isolationScope.setClient(client); addCloudResourceContext(isolationScope); @@ -215,7 +220,7 @@ export function withSentry { + it('instrumentDurableObjectWithSentry generic functionality', () => { + const options = vi.fn(); + const instrumented = instrumentDurableObjectWithSentry(options, vi.fn()); + expect(instrumented).toBeTypeOf('function'); + expect(() => Reflect.construct(instrumented, [])).not.toThrow(); + expect(options).toHaveBeenCalledOnce(); + }); + it('all available durable object methods are instrumented', () => { + const testClass = vi.fn(() => ({ + customMethod: vi.fn(), + fetch: vi.fn(), + alarm: vi.fn(), + webSocketMessage: vi.fn(), + webSocketClose: vi.fn(), + webSocketError: vi.fn(), + })); + const instrumented = instrumentDurableObjectWithSentry(vi.fn(), testClass as any); + const dObject: any = Reflect.construct(instrumented, []); + for (const method of Object.getOwnPropertyNames(dObject)) { + expect(isInstrumented(dObject[method]), `Method ${method} is instrumented`).toBeTruthy(); + } + }); + it('flush performs after all waitUntil promises are finished', async () => { + vi.useFakeTimers(); + onTestFinished(() => { + vi.useRealTimers(); + }); + const flush = vi.spyOn(SentryCore.Client.prototype, 'flush'); + const waitUntil = vi.fn(); + const testClass = vi.fn(context => ({ + fetch: () => { + context.waitUntil(new Promise(res => setTimeout(res))); + return new Response('test'); + }, + })); + const instrumented = instrumentDurableObjectWithSentry(vi.fn(), testClass as any); + const context = { + waitUntil, + } as unknown as ExecutionContext; + const dObject: any = Reflect.construct(instrumented, [context, {} as any]); + expect(() => dObject.fetch(new Request('https://example.com'))).not.toThrow(); + expect(flush).not.toBeCalled(); + expect(waitUntil).toHaveBeenCalledOnce(); + vi.advanceTimersToNextTimer(); + await Promise.all(waitUntil.mock.calls.map(([p]) => p)); + expect(flush).toBeCalled(); + }); +}); diff --git a/packages/cloudflare/test/flush.test.ts b/packages/cloudflare/test/flush.test.ts new file mode 100644 index 000000000000..34714711c682 --- /dev/null +++ b/packages/cloudflare/test/flush.test.ts @@ -0,0 +1,30 @@ +import { type ExecutionContext } from '@cloudflare/workers-types'; +import { describe, expect, it, onTestFinished, vi } from 'vitest'; +import { makeFlushLock } from '../src/flush'; + +describe('Flush buffer test', () => { + const waitUntilPromises: Promise[] = []; + const mockExecutionContext: ExecutionContext = { + waitUntil: vi.fn(prmise => { + waitUntilPromises.push(prmise); + }), + passThroughOnException: vi.fn(), + }; + it('should flush buffer immediately if no waitUntil were called', async () => { + const { finalize } = makeFlushLock(mockExecutionContext); + await expect(finalize()).resolves.toBeUndefined(); + }); + it('should flush buffer only after all waitUntil were finished', async () => { + vi.useFakeTimers(); + onTestFinished(() => { + vi.useRealTimers(); + }); + const task = new Promise(resolve => setTimeout(resolve, 100)); + const lock = makeFlushLock(mockExecutionContext); + mockExecutionContext.waitUntil(task); + void lock.finalize(); + vi.advanceTimersToNextTimer(); + await Promise.all(waitUntilPromises); + await expect(lock.ready).resolves.toBeUndefined(); + }); +}); diff --git a/packages/cloudflare/test/handler.test.ts b/packages/cloudflare/test/handler.test.ts index bced0fdbe277..2e5c0f836e89 100644 --- a/packages/cloudflare/test/handler.test.ts +++ b/packages/cloudflare/test/handler.test.ts @@ -1,10 +1,16 @@ // Note: These tests run the handler in Node.js, which has some differences to the cloudflare workers runtime. // Although this is not ideal, this is the best we can do until we have a better way to test cloudflare workers. -import type { ForwardableEmailMessage, MessageBatch, ScheduledController, TraceItem } from '@cloudflare/workers-types'; +import type { + ExecutionContext, + ForwardableEmailMessage, + MessageBatch, + ScheduledController, + TraceItem, +} from '@cloudflare/workers-types'; import type { Event } from '@sentry/core'; import * as SentryCore from '@sentry/core'; -import { beforeEach, describe, expect, test, vi } from 'vitest'; +import { beforeEach, describe, expect, onTestFinished, test, vi } from 'vitest'; import { CloudflareClient } from '../src/client'; import { withSentry } from '../src/handler'; import { markAsInstrumented } from '../src/instrument'; @@ -24,6 +30,10 @@ const MOCK_ENV = { SENTRY_RELEASE: '1.1.1', }; +function addDelayedWaitUntil(context: ExecutionContext) { + context.waitUntil(new Promise(resolve => setTimeout(() => resolve()))); +} + describe('withSentry', () => { beforeEach(() => { vi.clearAllMocks(); @@ -122,6 +132,32 @@ describe('withSentry', () => { expect(sentryEvent.release).toEqual('2.0.0'); }); + + test('flush must be called when all waitUntil are done', async () => { + const flush = vi.spyOn(SentryCore.Client.prototype, 'flush'); + vi.useFakeTimers(); + onTestFinished(() => { + vi.useRealTimers(); + }); + const handler = { + fetch(_request, _env, _context) { + addDelayedWaitUntil(_context); + return new Response('test'); + }, + } satisfies ExportedHandler; + + const wrappedHandler = withSentry(vi.fn(), handler); + const waits: Promise[] = []; + const waitUntil = vi.fn(promise => waits.push(promise)); + await wrappedHandler.fetch?.(new Request('https://example.com'), MOCK_ENV, { + waitUntil, + } as unknown as ExecutionContext); + expect(flush).not.toBeCalled(); + expect(waitUntil).toBeCalled(); + vi.advanceTimersToNextTimer().runAllTimers(); + await Promise.all(waits); + expect(flush).toHaveBeenCalledOnce(); + }); }); describe('scheduled handler', () => { @@ -198,13 +234,12 @@ describe('withSentry', () => { } satisfies ExportedHandler; const context = createMockExecutionContext(); + const waitUntilSpy = vi.spyOn(context, 'waitUntil'); const wrappedHandler = withSentry(env => ({ dsn: env.SENTRY_DSN }), handler); await wrappedHandler.scheduled?.(createMockScheduledController(), MOCK_ENV, context); - // eslint-disable-next-line @typescript-eslint/unbound-method - expect(context.waitUntil).toHaveBeenCalledTimes(1); - // eslint-disable-next-line @typescript-eslint/unbound-method - expect(context.waitUntil).toHaveBeenLastCalledWith(expect.any(Promise)); + expect(waitUntilSpy).toHaveBeenCalledTimes(1); + expect(waitUntilSpy).toHaveBeenLastCalledWith(expect.any(Promise)); }); test('creates a cloudflare client and sets it on the handler', async () => { @@ -337,6 +372,32 @@ describe('withSentry', () => { }); }); }); + + test('flush must be called when all waitUntil are done', async () => { + const flush = vi.spyOn(SentryCore.Client.prototype, 'flush'); + vi.useFakeTimers(); + onTestFinished(() => { + vi.useRealTimers(); + }); + const handler = { + scheduled(_controller, _env, _context) { + addDelayedWaitUntil(_context); + return; + }, + } satisfies ExportedHandler; + + const wrappedHandler = withSentry(vi.fn(), handler); + const waits: Promise[] = []; + const waitUntil = vi.fn(promise => waits.push(promise)); + await wrappedHandler.scheduled?.(createMockScheduledController(), MOCK_ENV, { + waitUntil, + } as unknown as ExecutionContext); + expect(flush).not.toBeCalled(); + expect(waitUntil).toBeCalled(); + vi.advanceTimersToNextTimer().runAllTimers(); + await Promise.all(waits); + expect(flush).toHaveBeenCalledOnce(); + }); }); describe('email handler', () => { @@ -413,13 +474,12 @@ describe('withSentry', () => { } satisfies ExportedHandler; const context = createMockExecutionContext(); + const waitUntilSpy = vi.spyOn(context, 'waitUntil'); const wrappedHandler = withSentry(env => ({ dsn: env.SENTRY_DSN }), handler); await wrappedHandler.email?.(createMockEmailMessage(), MOCK_ENV, context); - // eslint-disable-next-line @typescript-eslint/unbound-method - expect(context.waitUntil).toHaveBeenCalledTimes(1); - // eslint-disable-next-line @typescript-eslint/unbound-method - expect(context.waitUntil).toHaveBeenLastCalledWith(expect.any(Promise)); + expect(waitUntilSpy).toHaveBeenCalledTimes(1); + expect(waitUntilSpy).toHaveBeenLastCalledWith(expect.any(Promise)); }); test('creates a cloudflare client and sets it on the handler', async () => { @@ -551,6 +611,32 @@ describe('withSentry', () => { }); }); }); + + test('flush must be called when all waitUntil are done', async () => { + const flush = vi.spyOn(SentryCore.Client.prototype, 'flush'); + vi.useFakeTimers(); + onTestFinished(() => { + vi.useRealTimers(); + }); + const handler = { + email(_controller, _env, _context) { + addDelayedWaitUntil(_context); + return; + }, + } satisfies ExportedHandler; + + const wrappedHandler = withSentry(vi.fn(), handler); + const waits: Promise[] = []; + const waitUntil = vi.fn(promise => waits.push(promise)); + await wrappedHandler.email?.(createMockEmailMessage(), MOCK_ENV, { + waitUntil, + } as unknown as ExecutionContext); + expect(flush).not.toBeCalled(); + expect(waitUntil).toBeCalled(); + vi.advanceTimersToNextTimer().runAllTimers(); + await Promise.all(waits); + expect(flush).toHaveBeenCalledOnce(); + }); }); describe('queue handler', () => { @@ -627,13 +713,12 @@ describe('withSentry', () => { } satisfies ExportedHandler; const context = createMockExecutionContext(); + const waitUntilSpy = vi.spyOn(context, 'waitUntil'); const wrappedHandler = withSentry(env => ({ dsn: env.SENTRY_DSN }), handler); await wrappedHandler.queue?.(createMockQueueBatch(), MOCK_ENV, context); - // eslint-disable-next-line @typescript-eslint/unbound-method - expect(context.waitUntil).toHaveBeenCalledTimes(1); - // eslint-disable-next-line @typescript-eslint/unbound-method - expect(context.waitUntil).toHaveBeenLastCalledWith(expect.any(Promise)); + expect(waitUntilSpy).toHaveBeenCalledTimes(1); + expect(waitUntilSpy).toHaveBeenLastCalledWith(expect.any(Promise)); }); test('creates a cloudflare client and sets it on the handler', async () => { @@ -769,6 +854,32 @@ describe('withSentry', () => { }); }); }); + + test('flush must be called when all waitUntil are done', async () => { + const flush = vi.spyOn(SentryCore.Client.prototype, 'flush'); + vi.useFakeTimers(); + onTestFinished(() => { + vi.useRealTimers(); + }); + const handler = { + queue(_controller, _env, _context) { + addDelayedWaitUntil(_context); + return; + }, + } satisfies ExportedHandler; + + const wrappedHandler = withSentry(vi.fn(), handler); + const waits: Promise[] = []; + const waitUntil = vi.fn(promise => waits.push(promise)); + await wrappedHandler.queue?.(createMockQueueBatch(), MOCK_ENV, { + waitUntil, + } as unknown as ExecutionContext); + expect(flush).not.toBeCalled(); + expect(waitUntil).toBeCalled(); + vi.advanceTimersToNextTimer().runAllTimers(); + await Promise.all(waits); + expect(flush).toHaveBeenCalledOnce(); + }); }); describe('tail handler', () => { @@ -845,13 +956,12 @@ describe('withSentry', () => { } satisfies ExportedHandler; const context = createMockExecutionContext(); + const waitUntilSpy = vi.spyOn(context, 'waitUntil'); const wrappedHandler = withSentry(env => ({ dsn: env.SENTRY_DSN }), handler); await wrappedHandler.tail?.(createMockTailEvent(), MOCK_ENV, context); - // eslint-disable-next-line @typescript-eslint/unbound-method - expect(context.waitUntil).toHaveBeenCalledTimes(1); - // eslint-disable-next-line @typescript-eslint/unbound-method - expect(context.waitUntil).toHaveBeenLastCalledWith(expect.any(Promise)); + expect(waitUntilSpy).toHaveBeenCalledTimes(1); + expect(waitUntilSpy).toHaveBeenLastCalledWith(expect.any(Promise)); }); test('creates a cloudflare client and sets it on the handler', async () => { @@ -941,6 +1051,33 @@ describe('withSentry', () => { expect(thrownError).toBe(error); }); }); + + test('flush must be called when all waitUntil are done', async () => { + const flush = vi.spyOn(SentryCore.Client.prototype, 'flush'); + vi.useFakeTimers(); + onTestFinished(() => { + vi.useRealTimers(); + flush.mockRestore(); + }); + const handler = { + tail(_controller, _env, _context) { + addDelayedWaitUntil(_context); + return; + }, + } satisfies ExportedHandler; + + const wrappedHandler = withSentry(vi.fn(), handler); + const waits: Promise[] = []; + const waitUntil = vi.fn(promise => waits.push(promise)); + await wrappedHandler.tail?.(createMockTailEvent(), MOCK_ENV, { + waitUntil, + } as unknown as ExecutionContext); + expect(flush).not.toBeCalled(); + expect(waitUntil).toBeCalled(); + vi.advanceTimersToNextTimer().runAllTimers(); + await Promise.all(waits); + expect(flush).toHaveBeenCalledOnce(); + }); }); describe('hono errorHandler', () => { diff --git a/packages/cloudflare/test/integrations/fetch.test.ts b/packages/cloudflare/test/integrations/fetch.test.ts index 795b3e8c931c..724ff39c7dde 100644 --- a/packages/cloudflare/test/integrations/fetch.test.ts +++ b/packages/cloudflare/test/integrations/fetch.test.ts @@ -1,6 +1,5 @@ import type { HandlerDataFetch, Integration } from '@sentry/core'; import * as sentryCore from '@sentry/core'; -import * as sentryUtils from '@sentry/core'; import { createStackParser } from '@sentry/core'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { CloudflareClient } from '../../src/client'; @@ -12,7 +11,7 @@ class FakeClient extends CloudflareClient { } } -const addFetchInstrumentationHandlerSpy = vi.spyOn(sentryUtils, 'addFetchInstrumentationHandler'); +const addFetchInstrumentationHandlerSpy = vi.spyOn(sentryCore, 'addFetchInstrumentationHandler'); const instrumentFetchRequestSpy = vi.spyOn(sentryCore, 'instrumentFetchRequest'); const addBreadcrumbSpy = vi.spyOn(sentryCore, 'addBreadcrumb'); diff --git a/packages/cloudflare/test/request.test.ts b/packages/cloudflare/test/request.test.ts index 4fc9b308ec54..32bc8068ba6d 100644 --- a/packages/cloudflare/test/request.test.ts +++ b/packages/cloudflare/test/request.test.ts @@ -1,9 +1,10 @@ // Note: These tests run the handler in Node.js, which has some differences to the cloudflare workers runtime. // Although this is not ideal, this is the best we can do until we have a better way to test cloudflare workers. +import type { ExecutionContext } from '@cloudflare/workers-types'; import type { Event } from '@sentry/core'; import * as SentryCore from '@sentry/core'; -import { beforeAll, beforeEach, describe, expect, test, vi } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, onTestFinished, test, vi } from 'vitest'; import { setAsyncLocalStorageAsyncContextStrategy } from '../src/async'; import type { CloudflareOptions } from '../src/client'; import { CloudflareClient } from '../src/client'; @@ -13,6 +14,10 @@ const MOCK_OPTIONS: CloudflareOptions = { dsn: 'https://public@dsn.ingest.sentry.io/1337', }; +function addDelayedWaitUntil(context: ExecutionContext) { + context.waitUntil(new Promise(resolve => setTimeout(() => resolve()))); +} + describe('withSentry', () => { beforeAll(() => { setAsyncLocalStorageAsyncContextStrategy(); @@ -33,15 +38,14 @@ describe('withSentry', () => { test('flushes the event after the handler is done using the cloudflare context.waitUntil', async () => { const context = createMockExecutionContext(); + const waitUntilSpy = vi.spyOn(context, 'waitUntil'); await wrapRequestHandler( { options: MOCK_OPTIONS, request: new Request('https://example.com'), context }, () => new Response('test'), ); - // eslint-disable-next-line @typescript-eslint/unbound-method - expect(context.waitUntil).toHaveBeenCalledTimes(1); - // eslint-disable-next-line @typescript-eslint/unbound-method - expect(context.waitUntil).toHaveBeenLastCalledWith(expect.any(Promise)); + expect(waitUntilSpy).toHaveBeenCalledTimes(1); + expect(waitUntilSpy).toHaveBeenLastCalledWith(expect.any(Promise)); }); test("doesn't error if context is undefined", () => { @@ -64,6 +68,30 @@ describe('withSentry', () => { expect(initAndBindSpy).toHaveBeenLastCalledWith(CloudflareClient, expect.any(Object)); }); + test('flush must be called when all waitUntil are done', async () => { + const flush = vi.spyOn(SentryCore.Client.prototype, 'flush'); + vi.useFakeTimers(); + onTestFinished(() => { + vi.useRealTimers(); + }); + const waits: Promise[] = []; + const waitUntil = vi.fn(promise => waits.push(promise)); + + const context = { + waitUntil, + } as unknown as ExecutionContext; + + await wrapRequestHandler({ options: MOCK_OPTIONS, request: new Request('https://example.com'), context }, () => { + addDelayedWaitUntil(context); + return new Response('test'); + }); + expect(flush).not.toBeCalled(); + expect(waitUntil).toBeCalled(); + vi.advanceTimersToNextTimerAsync().then(() => vi.runAllTimers()); + await Promise.all(waits); + expect(flush).toHaveBeenCalledOnce(); + }); + describe('scope instrumentation', () => { test('adds cloud resource context', async () => { let sentryEvent: Event = {};