From ceac7ebc3b16e65c04dcb93bb6f507b79d0d081d Mon Sep 17 00:00:00 2001 From: cod1k Date: Fri, 5 Sep 2025 15:18:32 +0300 Subject: [PATCH 01/10] Introduce lock instrumentation for `context.waitUntil` Added `kFlushLock` to implement and manage a reusable `FlushLock` for the execution context. Enhanced promise handling with `Promise.withResolvers` and introduced type declarations for better developer experience. --- packages/cloudflare/src/flush.ts | 45 ++++++++++++++++++++++-------- packages/cloudflare/src/types.d.ts | 13 +++++++++ 2 files changed, 47 insertions(+), 11 deletions(-) create mode 100644 packages/cloudflare/src/types.d.ts diff --git a/packages/cloudflare/src/flush.ts b/packages/cloudflare/src/flush.ts index f38c805d0f8b..309bbff7536a 100644 --- a/packages/cloudflare/src/flush.ts +++ b/packages/cloudflare/src/flush.ts @@ -5,6 +5,23 @@ type FlushLock = { readonly finalize: () => Promise; }; +const kFlushLock = Symbol.for('kFlushLock'); + +function getInstrumentedLock(o: T): FlushLock | undefined { + const descriptor = Object.getOwnPropertyDescriptor(o, kFlushLock); + if (descriptor?.value) return descriptor.value as FlushLock; + return undefined; +} + +function storeInstrumentedLock(o: T, lock: FlushLock): void { + Object.defineProperty(o, kFlushLock, { + value: lock, + enumerable: false, + configurable: true, + writable: false, + }); +} + /** * 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 @@ -14,25 +31,31 @@ type FlushLock = { * @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; - }); + // eslint-disable-next-line @typescript-eslint/unbound-method + let lock = getInstrumentedLock(context.waitUntil); + if (lock) { + return lock; + } let pending = 0; const originalWaitUntil = context.waitUntil.bind(context) as typeof context.waitUntil; - context.waitUntil = promise => { + const { promise, resolve } = Promise.withResolvers(); + const hijackedWaitUntil: typeof originalWaitUntil = promise => { pending++; return originalWaitUntil( promise.finally(() => { - if (--pending === 0) resolveAllDone(); + if (--pending === 0) resolve(); }), ); }; - return Object.freeze({ - ready: allDone, + lock = Object.freeze({ + ready: promise, finalize: () => { - if (pending === 0) resolveAllDone(); - return allDone; + if (pending === 0) resolve(); + return promise; }, - }); + }) as FlushLock; + storeInstrumentedLock(hijackedWaitUntil, lock); + context.waitUntil = hijackedWaitUntil; + + return lock; } diff --git a/packages/cloudflare/src/types.d.ts b/packages/cloudflare/src/types.d.ts new file mode 100644 index 000000000000..0844d0c1acad --- /dev/null +++ b/packages/cloudflare/src/types.d.ts @@ -0,0 +1,13 @@ +/** + * This method exists in cloudflare workers. + * Well-documented here + * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers + * It is also possible to add support of it modifying tsconfig.json but I am not sure if it is a good idea. + */ +interface PromiseConstructor { + withResolvers(): { + promise: PromiseConstructor; + resolve: (value?: T) => void; + reject: (reason?: E) => void; + }; +} From 321872806cd44ceee2eb2d3a07c8edef5c307e62 Mon Sep 17 00:00:00 2001 From: cod1k Date: Fri, 5 Sep 2025 16:54:35 +0300 Subject: [PATCH 02/10] Fix typo in mock `waitUntil` implementation in tests Corrected the variable name from `prmise` to `promise` in the mock `waitUntil` function. Added a `props` property to `mockExecutionContext` for enhanced test completeness. --- packages/cloudflare/test/flush.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/cloudflare/test/flush.test.ts b/packages/cloudflare/test/flush.test.ts index 34714711c682..6e6f102553f5 100644 --- a/packages/cloudflare/test/flush.test.ts +++ b/packages/cloudflare/test/flush.test.ts @@ -5,10 +5,11 @@ import { makeFlushLock } from '../src/flush'; describe('Flush buffer test', () => { const waitUntilPromises: Promise[] = []; const mockExecutionContext: ExecutionContext = { - waitUntil: vi.fn(prmise => { - waitUntilPromises.push(prmise); + waitUntil: vi.fn(promise => { + waitUntilPromises.push(promise); }), passThroughOnException: vi.fn(), + props: null, }; it('should flush buffer immediately if no waitUntil were called', async () => { const { finalize } = makeFlushLock(mockExecutionContext); From 9d4bcf977f6122f3314451ad78f4778b2978cf90 Mon Sep 17 00:00:00 2001 From: cod1k Date: Fri, 5 Sep 2025 17:08:01 +0300 Subject: [PATCH 03/10] Simplify `FlushLock` instrumentation logic Refactored `getInstrumentedLock` and `storeInstrumentedLock` to use type-safe access via `Lockable` type and direct property manipulation. Removed unnecessary `Object.defineProperty` usage for better readability and maintainability. --- packages/cloudflare/src/flush.ts | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/packages/cloudflare/src/flush.ts b/packages/cloudflare/src/flush.ts index 309bbff7536a..5e4bded2966d 100644 --- a/packages/cloudflare/src/flush.ts +++ b/packages/cloudflare/src/flush.ts @@ -4,22 +4,16 @@ type FlushLock = { readonly ready: Promise; readonly finalize: () => Promise; }; +type Lockable = T & { [kFlushLock]?: FlushLock }; const kFlushLock = Symbol.for('kFlushLock'); -function getInstrumentedLock(o: T): FlushLock | undefined { - const descriptor = Object.getOwnPropertyDescriptor(o, kFlushLock); - if (descriptor?.value) return descriptor.value as FlushLock; - return undefined; +function getInstrumentedLock(o: Lockable): FlushLock | undefined { + return o[kFlushLock]; } -function storeInstrumentedLock(o: T, lock: FlushLock): void { - Object.defineProperty(o, kFlushLock, { - value: lock, - enumerable: false, - configurable: true, - writable: false, - }); +function storeInstrumentedLock(o: Lockable, lock: FlushLock): void { + o[kFlushLock] = lock; } /** @@ -34,6 +28,7 @@ export function makeFlushLock(context: ExecutionContext): FlushLock { // eslint-disable-next-line @typescript-eslint/unbound-method let lock = getInstrumentedLock(context.waitUntil); if (lock) { + // It is fine to return the same lock multiple times because this means the context has already been instrumented. return lock; } let pending = 0; From 20e6bae0f9d0e959e04db9fc1fb0cd9be1540bda Mon Sep 17 00:00:00 2001 From: cod1k Date: Mon, 8 Sep 2025 09:05:26 +0300 Subject: [PATCH 04/10] Refactor tests to simplify `waitUntil` handling Replaced custom delayed `waitUntil` implementations with `Promise.withResolvers` for improved readability and consistency. Removed unnecessary timer usage and adjusted test logic for better maintainability. --- .../cloudflare/test/durableobject.test.ts | 13 +- packages/cloudflare/test/flush.test.ts | 21 +--- packages/cloudflare/test/handler.test.ts | 114 +++++++++--------- packages/cloudflare/test/request.test.ts | 25 ++-- packages/cloudflare/test/types.d.ts | 8 ++ 5 files changed, 84 insertions(+), 97 deletions(-) create mode 100644 packages/cloudflare/test/types.d.ts diff --git a/packages/cloudflare/test/durableobject.test.ts b/packages/cloudflare/test/durableobject.test.ts index ce794dc7fb69..0b11edc74527 100644 --- a/packages/cloudflare/test/durableobject.test.ts +++ b/packages/cloudflare/test/durableobject.test.ts @@ -1,6 +1,6 @@ import type { ExecutionContext } from '@cloudflare/workers-types'; import * as SentryCore from '@sentry/core'; -import { afterEach, describe, expect, it, onTestFinished, vi } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { instrumentDurableObjectWithSentry } from '../src'; import { isInstrumented } from '../src/instrument'; @@ -122,15 +122,13 @@ describe('instrumentDurableObjectWithSentry', () => { }); 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 { promise, resolve } = Promise.withResolvers(); + process.nextTick(resolve); const testClass = vi.fn(context => ({ fetch: () => { - context.waitUntil(new Promise(res => setTimeout(res))); + context.waitUntil(promise); return new Response('test'); }, })); @@ -142,8 +140,7 @@ describe('instrumentDurableObjectWithSentry', () => { 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)); + await Promise.all(waitUntil.mock.calls.map(call => call[0])); expect(flush).toBeCalled(); }); diff --git a/packages/cloudflare/test/flush.test.ts b/packages/cloudflare/test/flush.test.ts index 6e6f102553f5..2f0d9c60e2c5 100644 --- a/packages/cloudflare/test/flush.test.ts +++ b/packages/cloudflare/test/flush.test.ts @@ -1,13 +1,10 @@ import { type ExecutionContext } from '@cloudflare/workers-types'; -import { describe, expect, it, onTestFinished, vi } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { makeFlushLock } from '../src/flush'; describe('Flush buffer test', () => { - const waitUntilPromises: Promise[] = []; const mockExecutionContext: ExecutionContext = { - waitUntil: vi.fn(promise => { - waitUntilPromises.push(promise); - }), + waitUntil: vi.fn(), passThroughOnException: vi.fn(), props: null, }; @@ -16,16 +13,10 @@ describe('Flush buffer test', () => { 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 { promise, resolve } = Promise.withResolvers(); const lock = makeFlushLock(mockExecutionContext); - mockExecutionContext.waitUntil(task); - void lock.finalize(); - vi.advanceTimersToNextTimer(); - await Promise.all(waitUntilPromises); - await expect(lock.ready).resolves.toBeUndefined(); + mockExecutionContext.waitUntil(promise); + process.nextTick(resolve); + await expect(lock.finalize()).resolves.toBeUndefined(); }); }); diff --git a/packages/cloudflare/test/handler.test.ts b/packages/cloudflare/test/handler.test.ts index ddd4b0010ec0..5c8676eb0fd3 100644 --- a/packages/cloudflare/test/handler.test.ts +++ b/packages/cloudflare/test/handler.test.ts @@ -10,7 +10,7 @@ import type { } from '@cloudflare/workers-types'; import type { Event } from '@sentry/core'; import * as SentryCore from '@sentry/core'; -import { beforeEach, describe, expect, onTestFinished, test, vi } from 'vitest'; +import { beforeEach, describe, expect, test, vi } from 'vitest'; import { CloudflareClient } from '../src/client'; import { withSentry } from '../src/handler'; import { markAsInstrumented } from '../src/instrument'; @@ -30,8 +30,29 @@ const MOCK_ENV = { SENTRY_RELEASE: '1.1.1', }; -function addDelayedWaitUntil(context: ExecutionContext) { - context.waitUntil(new Promise(resolve => setTimeout(() => resolve()))); +function makeWaitUntilAndTask< + M extends ReturnType, + T extends M & { + readonly ready: Promise; + readonly task: { readonly promise: Promise; readonly resolve: M }; + }, +>(): T { + const waitUntil = vi.fn(); + const { promise, resolve } = Promise.withResolvers(); + const resolver = vi.fn().mockImplementation(resolve).mockName('waitUntil.ready'); + Object.defineProperties(waitUntil, { + ready: { + get: () => Promise.all(waitUntil.mock.calls.map(call => call[0])).then(() => resolver), + enumerable: true, + configurable: true, + }, + task: { + value: { promise, resolve: resolver }, + enumerable: true, + configurable: true, + }, + }); + return waitUntil as T; } describe('withSentry', () => { @@ -135,27 +156,22 @@ 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 waitUntil = makeWaitUntilAndTask(); + process.nextTick(waitUntil.task.resolve); const handler = { - fetch(_request, _env, _context) { - addDelayedWaitUntil(_context); + fetch(...args) { + args[2].waitUntil(waitUntil.task.promise); 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); + const ready = await waitUntil.ready; + expect(flush).toHaveBeenCalledAfter(ready); expect(flush).toHaveBeenCalledOnce(); }); }); @@ -375,28 +391,23 @@ 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 waitUntil = makeWaitUntilAndTask(); + process.nextTick(waitUntil.task.resolve); const handler = { - scheduled(_controller, _env, _context) { - addDelayedWaitUntil(_context); + scheduled(...args) { + args[2].waitUntil(waitUntil.task.promise); 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); + await waitUntil.ready; expect(flush).toHaveBeenCalledOnce(); + expect(flush).toHaveBeenCalledAfter(waitUntil); }); }); @@ -614,28 +625,24 @@ 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 waitUntil = makeWaitUntilAndTask(); + + process.nextTick(waitUntil.task.resolve); const handler = { - email(_controller, _env, _context) { - addDelayedWaitUntil(_context); + email(...args) { + args[2].waitUntil(waitUntil.task.promise); 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); + const ready = await waitUntil.ready; expect(flush).toHaveBeenCalledOnce(); + expect(flush).toHaveBeenCalledAfter(ready); }); }); @@ -857,27 +864,22 @@ 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 waitUntil = makeWaitUntilAndTask(); + process.nextTick(waitUntil.task.resolve); const handler = { - queue(_controller, _env, _context) { - addDelayedWaitUntil(_context); + queue(...args) { + args[2].waitUntil(waitUntil.task.promise); 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).toHaveBeenCalledAfter(await waitUntil.ready); expect(flush).toHaveBeenCalledOnce(); }); }); @@ -1054,29 +1056,23 @@ 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(); - flush.mockRestore(); - }); + const waitUntil = makeWaitUntilAndTask(); + process.nextTick(waitUntil.task.resolve); const handler = { - tail(_controller, _env, _context) { - addDelayedWaitUntil(_context); + tail(...args) { + args[2].waitUntil(waitUntil.task.promise); 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); + const ready = await waitUntil.ready; expect(flush).toHaveBeenCalledOnce(); + expect(flush).toHaveBeenCalledAfter(ready); }); }); diff --git a/packages/cloudflare/test/request.test.ts b/packages/cloudflare/test/request.test.ts index eb2989437396..4a7a768419cd 100644 --- a/packages/cloudflare/test/request.test.ts +++ b/packages/cloudflare/test/request.test.ts @@ -4,7 +4,7 @@ import type { ExecutionContext } from '@cloudflare/workers-types'; import type { Event } from '@sentry/core'; import * as SentryCore from '@sentry/core'; -import { beforeAll, beforeEach, describe, expect, onTestFinished, test, vi } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, test, vi } from 'vitest'; import { setAsyncLocalStorageAsyncContextStrategy } from '../src/async'; import type { CloudflareOptions } from '../src/client'; import { CloudflareClient } from '../src/client'; @@ -14,10 +14,6 @@ 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(); @@ -70,25 +66,24 @@ 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 waits: Promise[] = []; - const waitUntil = vi.fn(promise => waits.push(promise)); + const waitUntil = vi.fn(); const context = { waitUntil, } as unknown as ExecutionContext; + const { promise, resolve } = Promise.withResolvers(); + const resolved = vi.fn(() => resolve()); + process.nextTick(resolved); + await wrapRequestHandler({ options: MOCK_OPTIONS, request: new Request('https://example.com'), context }, () => { - addDelayedWaitUntil(context); + context.waitUntil(promise); return new Response('test'); }); expect(flush).not.toBeCalled(); expect(waitUntil).toBeCalled(); - vi.advanceTimersToNextTimerAsync().then(() => vi.runAllTimers()); - await Promise.all(waits); + await Promise.all(waitUntil.mock.calls.map(call => call[0])); + expect(flush).toHaveBeenCalledAfter(resolved); expect(flush).toHaveBeenCalledOnce(); }); @@ -336,5 +331,5 @@ function createMockExecutionContext(): ExecutionContext { return { waitUntil: vi.fn(), passThroughOnException: vi.fn(), - }; + } as unknown as ExecutionContext; } diff --git a/packages/cloudflare/test/types.d.ts b/packages/cloudflare/test/types.d.ts new file mode 100644 index 000000000000..af7f34640cab --- /dev/null +++ b/packages/cloudflare/test/types.d.ts @@ -0,0 +1,8 @@ +// Not required in TypeScript 5.7+ +interface PromiseConstructor { + withResolvers(): { + promise: PromiseConstructor; + resolve: (value?: T) => void; + reject: (err: E) => void; + }; +} From 7fa361cc32fc9d9a1f2c49a3b65861be12c544e9 Mon Sep 17 00:00:00 2001 From: cod1k Date: Mon, 8 Sep 2025 09:05:38 +0300 Subject: [PATCH 05/10] Add test to ensure `waitUntil` is not wrapped twice Introduced a new test case to validate that `makeFlushLock` does not wrap the execution context more than once. Ensures consistency and prevents potential redundancy in the flush handling logic. --- packages/cloudflare/test/flush.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/cloudflare/test/flush.test.ts b/packages/cloudflare/test/flush.test.ts index 2f0d9c60e2c5..464cf06fdd3e 100644 --- a/packages/cloudflare/test/flush.test.ts +++ b/packages/cloudflare/test/flush.test.ts @@ -12,6 +12,11 @@ describe('Flush buffer test', () => { const { finalize } = makeFlushLock(mockExecutionContext); await expect(finalize()).resolves.toBeUndefined(); }); + it('waitUntil should not be wrapped mose than once', () => { + expect(makeFlushLock(mockExecutionContext), 'Execution context wrapped twice').toBe( + makeFlushLock(mockExecutionContext), + ); + }); it('should flush buffer only after all waitUntil were finished', async () => { const { promise, resolve } = Promise.withResolvers(); const lock = makeFlushLock(mockExecutionContext); From 02c0238ea6cc4f164bac826fac6363ce296576de Mon Sep 17 00:00:00 2001 From: cod1k Date: Mon, 8 Sep 2025 12:14:53 +0300 Subject: [PATCH 06/10] Clone execution context to ensure method binding Introduced the `cloneExecutionContext` utility to create shallow copies of the execution context while maintaining proper binding of key methods like `waitUntil` and `passThroughOnException`. Updated all handler proxies to use the cloned execution context, ensuring consistent behavior and preventing unintended side effects. --- packages/cloudflare/src/durableobject.ts | 5 ++++- packages/cloudflare/src/handler.ts | 21 ++++++++++++++----- .../src/utils/cloneExecutionContext.ts | 16 ++++++++++++++ 3 files changed, 36 insertions(+), 6 deletions(-) create mode 100644 packages/cloudflare/src/utils/cloneExecutionContext.ts diff --git a/packages/cloudflare/src/durableobject.ts b/packages/cloudflare/src/durableobject.ts index bda7a9aa3538..59d823f1b5c1 100644 --- a/packages/cloudflare/src/durableobject.ts +++ b/packages/cloudflare/src/durableobject.ts @@ -18,6 +18,7 @@ import { isInstrumented, markAsInstrumented } from './instrument'; import { getFinalOptions } from './options'; import { wrapRequestHandler } from './request'; import { init } from './sdk'; +import { cloneExecutionContext } from './utils/cloneExecutionContext'; type MethodWrapperOptions = { spanName?: string; @@ -192,9 +193,11 @@ export function instrumentDurableObjectWithSentry< C extends new (state: DurableObjectState, env: E) => T, >(optionsCallback: (env: E) => CloudflareOptions, DurableObjectClass: C): C { return new Proxy(DurableObjectClass, { - construct(target, [context, env]) { + construct(target, [ctx, env]) { setAsyncLocalStorageAsyncContextStrategy(); + const context = cloneExecutionContext(ctx) + const options = getFinalOptions(optionsCallback(env), env); const obj = new target(context, env); diff --git a/packages/cloudflare/src/handler.ts b/packages/cloudflare/src/handler.ts index 354233154a0b..04cfe51a2da7 100644 --- a/packages/cloudflare/src/handler.ts +++ b/packages/cloudflare/src/handler.ts @@ -14,6 +14,7 @@ import { getFinalOptions } from './options'; import { wrapRequestHandler } from './request'; import { addCloudResourceContext } from './scope-utils'; import { init } from './sdk'; +import { cloneExecutionContext } from './utils/cloneExecutionContext'; /** * Wrapper for Cloudflare handlers. @@ -37,9 +38,11 @@ export function withSentry>) { - const [request, env, context] = args; + const [request, env, ctx] = args; const options = getFinalOptions(optionsCallback(env), env); + const context = cloneExecutionContext(ctx) + args[2] = context; return wrapRequestHandler({ options, request, context }, () => target.apply(thisArg, args)); }, @@ -71,7 +74,9 @@ export function withSentry>) { - const [event, env, context] = args; + const [event, env, ctx] = args; + const context = cloneExecutionContext(ctx) + args[2] = context; return withIsolationScope(isolationScope => { const options = getFinalOptions(optionsCallback(env), env); const waitUntil = context.waitUntil.bind(context); @@ -114,7 +119,9 @@ export function withSentry>) { - const [emailMessage, env, context] = args; + const [emailMessage, env, ctx] = args; + const context = cloneExecutionContext(ctx) + args[2] = context; return withIsolationScope(isolationScope => { const options = getFinalOptions(optionsCallback(env), env); const waitUntil = context.waitUntil.bind(context); @@ -155,7 +162,9 @@ export function withSentry>) { - const [batch, env, context] = args; + const [batch, env, ctx] = args; + const context = cloneExecutionContext(ctx) + args[2] = context; return withIsolationScope(isolationScope => { const options = getFinalOptions(optionsCallback(env), env); @@ -205,7 +214,9 @@ export function withSentry>) { - const [, env, context] = args; + const [, env, ctx] = args; + const context = cloneExecutionContext(ctx) + args[2] = context; return withIsolationScope(async isolationScope => { const options = getFinalOptions(optionsCallback(env), env); diff --git a/packages/cloudflare/src/utils/cloneExecutionContext.ts b/packages/cloudflare/src/utils/cloneExecutionContext.ts new file mode 100644 index 000000000000..4e978d3d3fb8 --- /dev/null +++ b/packages/cloudflare/src/utils/cloneExecutionContext.ts @@ -0,0 +1,16 @@ +import { type ExecutionContext } from '@cloudflare/workers-types'; + +/** + * Clones the given execution context by creating a shallow copy while ensuring the binding of specific methods. + * + * @param {ExecutionContext|undefined} ctx - The execution context to clone. Can be undefined. + * @return {ExecutionContext|undefined} A cloned execution context with bound methods, or the original undefined value if no context was provided. + */ +export function cloneExecutionContext(ctx: T): T { + if (!ctx) return ctx; + return { + ...ctx, + ...('waitUntil' in ctx && { waitUntil: ctx.waitUntil.bind(ctx) }), + ...('passThroughOnException' in ctx && { passThroughOnException: ctx.passThroughOnException.bind(ctx) }), + }; +} From 4d46261174c345c0e61c1d636697a8f622d9983f Mon Sep 17 00:00:00 2001 From: cod1k Date: Mon, 8 Sep 2025 14:25:08 +0300 Subject: [PATCH 07/10] Replace `Promise.withResolvers` with `createPromiseResolver` Introduced a new utility function `createPromiseResolver` to handle promise creation alongside external resolvers. Updated all references of `Promise.withResolvers` in the codebase and tests to use the new utility, ensuring compatibility and maintaining consistency. --- packages/cloudflare/src/flush.ts | 3 ++- packages/cloudflare/src/types.d.ts | 13 ---------- .../src/utils/makePromiseResolver.ts | 26 +++++++++++++++++++ .../cloudflare/test/durableobject.test.ts | 3 ++- packages/cloudflare/test/flush.test.ts | 3 ++- packages/cloudflare/test/handler.test.ts | 3 ++- packages/cloudflare/test/request.test.ts | 3 ++- packages/cloudflare/test/types.d.ts | 8 ------ 8 files changed, 36 insertions(+), 26 deletions(-) delete mode 100644 packages/cloudflare/src/types.d.ts create mode 100644 packages/cloudflare/src/utils/makePromiseResolver.ts delete mode 100644 packages/cloudflare/test/types.d.ts diff --git a/packages/cloudflare/src/flush.ts b/packages/cloudflare/src/flush.ts index 5e4bded2966d..adb719f41079 100644 --- a/packages/cloudflare/src/flush.ts +++ b/packages/cloudflare/src/flush.ts @@ -1,4 +1,5 @@ import type { ExecutionContext } from '@cloudflare/workers-types'; +import { createPromiseResolver } from './utils/makePromiseResolver'; type FlushLock = { readonly ready: Promise; @@ -33,7 +34,7 @@ export function makeFlushLock(context: ExecutionContext): FlushLock { } let pending = 0; const originalWaitUntil = context.waitUntil.bind(context) as typeof context.waitUntil; - const { promise, resolve } = Promise.withResolvers(); + const { promise, resolve } = createPromiseResolver(); const hijackedWaitUntil: typeof originalWaitUntil = promise => { pending++; return originalWaitUntil( diff --git a/packages/cloudflare/src/types.d.ts b/packages/cloudflare/src/types.d.ts deleted file mode 100644 index 0844d0c1acad..000000000000 --- a/packages/cloudflare/src/types.d.ts +++ /dev/null @@ -1,13 +0,0 @@ -/** - * This method exists in cloudflare workers. - * Well-documented here - * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers - * It is also possible to add support of it modifying tsconfig.json but I am not sure if it is a good idea. - */ -interface PromiseConstructor { - withResolvers(): { - promise: PromiseConstructor; - resolve: (value?: T) => void; - reject: (reason?: E) => void; - }; -} diff --git a/packages/cloudflare/src/utils/makePromiseResolver.ts b/packages/cloudflare/src/utils/makePromiseResolver.ts new file mode 100644 index 000000000000..f772951406b1 --- /dev/null +++ b/packages/cloudflare/src/utils/makePromiseResolver.ts @@ -0,0 +1,26 @@ +type PromiseWithResolvers = { + readonly promise: Promise; + readonly resolve: (value?: T | PromiseLike) => void; + readonly reject: (reason?: E) => void; +}; +/** + * Creates an object containing a promise, along with its corresponding resolve and reject functions. + * + * This method provides a convenient way to create a promise and access its resolvers externally. + * + * @template T - The type of the resolved value of the promise. + * @template E - The type of the rejected value of the promise. Defaults to `unknown`. + * @return {PromiseWithResolvers} An object containing the promise and its resolve and reject functions. + */ +export function createPromiseResolver(): PromiseWithResolvers { + if ('withResolvers' in Promise && typeof Promise.withResolvers === 'function') { + return Promise.withResolvers(); + } + let resolve; + let reject; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject } as unknown as PromiseWithResolvers; +} diff --git a/packages/cloudflare/test/durableobject.test.ts b/packages/cloudflare/test/durableobject.test.ts index 0b11edc74527..4c4d5c63bfd3 100644 --- a/packages/cloudflare/test/durableobject.test.ts +++ b/packages/cloudflare/test/durableobject.test.ts @@ -3,6 +3,7 @@ import * as SentryCore from '@sentry/core'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { instrumentDurableObjectWithSentry } from '../src'; import { isInstrumented } from '../src/instrument'; +import { createPromiseResolver } from '../src/utils/makePromiseResolver'; describe('instrumentDurableObjectWithSentry', () => { afterEach(() => { @@ -124,7 +125,7 @@ describe('instrumentDurableObjectWithSentry', () => { it('flush performs after all waitUntil promises are finished', async () => { const flush = vi.spyOn(SentryCore.Client.prototype, 'flush'); const waitUntil = vi.fn(); - const { promise, resolve } = Promise.withResolvers(); + const { promise, resolve } = createPromiseResolver(); process.nextTick(resolve); const testClass = vi.fn(context => ({ fetch: () => { diff --git a/packages/cloudflare/test/flush.test.ts b/packages/cloudflare/test/flush.test.ts index 464cf06fdd3e..d6301ab27e54 100644 --- a/packages/cloudflare/test/flush.test.ts +++ b/packages/cloudflare/test/flush.test.ts @@ -1,6 +1,7 @@ import { type ExecutionContext } from '@cloudflare/workers-types'; import { describe, expect, it, vi } from 'vitest'; import { makeFlushLock } from '../src/flush'; +import { createPromiseResolver } from '../src/utils/makePromiseResolver'; describe('Flush buffer test', () => { const mockExecutionContext: ExecutionContext = { @@ -18,7 +19,7 @@ describe('Flush buffer test', () => { ); }); it('should flush buffer only after all waitUntil were finished', async () => { - const { promise, resolve } = Promise.withResolvers(); + const { promise, resolve } = createPromiseResolver(); const lock = makeFlushLock(mockExecutionContext); mockExecutionContext.waitUntil(promise); process.nextTick(resolve); diff --git a/packages/cloudflare/test/handler.test.ts b/packages/cloudflare/test/handler.test.ts index 5c8676eb0fd3..8cedb715c4fe 100644 --- a/packages/cloudflare/test/handler.test.ts +++ b/packages/cloudflare/test/handler.test.ts @@ -14,6 +14,7 @@ import { beforeEach, describe, expect, test, vi } from 'vitest'; import { CloudflareClient } from '../src/client'; import { withSentry } from '../src/handler'; import { markAsInstrumented } from '../src/instrument'; +import { createPromiseResolver } from '../src/utils/makePromiseResolver'; // Custom type for hono-like apps (cloudflare handlers) that include errorHandler and onError type HonoLikeApp = ExportedHandler< @@ -38,7 +39,7 @@ function makeWaitUntilAndTask< }, >(): T { const waitUntil = vi.fn(); - const { promise, resolve } = Promise.withResolvers(); + const { promise, resolve } = createPromiseResolver(); const resolver = vi.fn().mockImplementation(resolve).mockName('waitUntil.ready'); Object.defineProperties(waitUntil, { ready: { diff --git a/packages/cloudflare/test/request.test.ts b/packages/cloudflare/test/request.test.ts index 4a7a768419cd..2126ce8c2b75 100644 --- a/packages/cloudflare/test/request.test.ts +++ b/packages/cloudflare/test/request.test.ts @@ -9,6 +9,7 @@ import { setAsyncLocalStorageAsyncContextStrategy } from '../src/async'; import type { CloudflareOptions } from '../src/client'; import { CloudflareClient } from '../src/client'; import { wrapRequestHandler } from '../src/request'; +import { createPromiseResolver } from '../src/utils/makePromiseResolver'; const MOCK_OPTIONS: CloudflareOptions = { dsn: 'https://public@dsn.ingest.sentry.io/1337', @@ -72,7 +73,7 @@ describe('withSentry', () => { waitUntil, } as unknown as ExecutionContext; - const { promise, resolve } = Promise.withResolvers(); + const { promise, resolve } = createPromiseResolver(); const resolved = vi.fn(() => resolve()); process.nextTick(resolved); diff --git a/packages/cloudflare/test/types.d.ts b/packages/cloudflare/test/types.d.ts deleted file mode 100644 index af7f34640cab..000000000000 --- a/packages/cloudflare/test/types.d.ts +++ /dev/null @@ -1,8 +0,0 @@ -// Not required in TypeScript 5.7+ -interface PromiseConstructor { - withResolvers(): { - promise: PromiseConstructor; - resolve: (value?: T) => void; - reject: (err: E) => void; - }; -} From 2249f68bc0148b7cd7ba9cabfb165110238a9808 Mon Sep 17 00:00:00 2001 From: cod1k Date: Thu, 11 Sep 2025 16:37:02 +0300 Subject: [PATCH 08/10] Refactor and rename utilities for execution context handling Renamed `cloneExecutionContext` to `copyExecutionContext` and updated its implementation for improved binding logic and symbol handling. Moved `makeFlushLock` to `utils/flushLock` and adjusted imports accordingly. Added tests to validate `copyExecutionContext` behaviors and ensure robustness. --- packages/cloudflare/src/client.ts | 2 +- packages/cloudflare/src/durableobject.ts | 4 +- packages/cloudflare/src/handler.ts | 12 +++--- packages/cloudflare/src/sdk.ts | 2 +- .../src/utils/cloneExecutionContext.ts | 16 ------- .../src/utils/copyExecutionContext.ts | 26 ++++++++++++ .../src/{flush.ts => utils/flushLock.ts} | 10 ++--- .../test/copy-execution-context.test.ts | 42 +++++++++++++++++++ .../{flush.test.ts => flush-lock.test.ts} | 2 +- 9 files changed, 84 insertions(+), 32 deletions(-) delete mode 100644 packages/cloudflare/src/utils/cloneExecutionContext.ts create mode 100644 packages/cloudflare/src/utils/copyExecutionContext.ts rename packages/cloudflare/src/{flush.ts => utils/flushLock.ts} (80%) create mode 100644 packages/cloudflare/test/copy-execution-context.test.ts rename packages/cloudflare/test/{flush.test.ts => flush-lock.test.ts} (95%) diff --git a/packages/cloudflare/src/client.ts b/packages/cloudflare/src/client.ts index 2de5147d3d5a..6da36cc1721c 100644 --- a/packages/cloudflare/src/client.ts +++ b/packages/cloudflare/src/client.ts @@ -1,7 +1,7 @@ import type { ClientOptions, Options, ServerRuntimeClientOptions } from '@sentry/core'; import { applySdkMetadata, ServerRuntimeClient } from '@sentry/core'; -import type { makeFlushLock } from './flush'; import type { CloudflareTransportOptions } from './transport'; +import type { makeFlushLock } from './utils/flushLock'; /** * The Sentry Cloudflare SDK Client. diff --git a/packages/cloudflare/src/durableobject.ts b/packages/cloudflare/src/durableobject.ts index 59d823f1b5c1..5a9f14b06594 100644 --- a/packages/cloudflare/src/durableobject.ts +++ b/packages/cloudflare/src/durableobject.ts @@ -18,7 +18,7 @@ import { isInstrumented, markAsInstrumented } from './instrument'; import { getFinalOptions } from './options'; import { wrapRequestHandler } from './request'; import { init } from './sdk'; -import { cloneExecutionContext } from './utils/cloneExecutionContext'; +import { copyExecutionContext } from './utils/copyExecutionContext'; type MethodWrapperOptions = { spanName?: string; @@ -196,7 +196,7 @@ export function instrumentDurableObjectWithSentry< construct(target, [ctx, env]) { setAsyncLocalStorageAsyncContextStrategy(); - const context = cloneExecutionContext(ctx) + const context = copyExecutionContext(ctx); const options = getFinalOptions(optionsCallback(env), env); diff --git a/packages/cloudflare/src/handler.ts b/packages/cloudflare/src/handler.ts index 04cfe51a2da7..a032fbf135e1 100644 --- a/packages/cloudflare/src/handler.ts +++ b/packages/cloudflare/src/handler.ts @@ -14,7 +14,7 @@ import { getFinalOptions } from './options'; import { wrapRequestHandler } from './request'; import { addCloudResourceContext } from './scope-utils'; import { init } from './sdk'; -import { cloneExecutionContext } from './utils/cloneExecutionContext'; +import { copyExecutionContext } from './utils/copyExecutionContext'; /** * Wrapper for Cloudflare handlers. @@ -41,7 +41,7 @@ export function withSentry target.apply(thisArg, args)); @@ -75,7 +75,7 @@ export function withSentry>) { const [event, env, ctx] = args; - const context = cloneExecutionContext(ctx) + const context = copyExecutionContext(ctx); args[2] = context; return withIsolationScope(isolationScope => { const options = getFinalOptions(optionsCallback(env), env); @@ -120,7 +120,7 @@ export function withSentry>) { const [emailMessage, env, ctx] = args; - const context = cloneExecutionContext(ctx) + const context = copyExecutionContext(ctx); args[2] = context; return withIsolationScope(isolationScope => { const options = getFinalOptions(optionsCallback(env), env); @@ -163,7 +163,7 @@ export function withSentry>) { const [batch, env, ctx] = args; - const context = cloneExecutionContext(ctx) + const context = copyExecutionContext(ctx); args[2] = context; return withIsolationScope(isolationScope => { @@ -215,7 +215,7 @@ export function withSentry>) { const [, env, ctx] = args; - const context = cloneExecutionContext(ctx) + const context = copyExecutionContext(ctx); args[2] = context; return withIsolationScope(async isolationScope => { diff --git a/packages/cloudflare/src/sdk.ts b/packages/cloudflare/src/sdk.ts index 9d4fb8d749ae..416ae47f312f 100644 --- a/packages/cloudflare/src/sdk.ts +++ b/packages/cloudflare/src/sdk.ts @@ -12,10 +12,10 @@ import { } from '@sentry/core'; import type { CloudflareClientOptions, CloudflareOptions } from './client'; import { CloudflareClient } from './client'; -import { makeFlushLock } from './flush'; import { fetchIntegration } from './integrations/fetch'; import { setupOpenTelemetryTracer } from './opentelemetry/tracer'; import { makeCloudflareTransport } from './transport'; +import { makeFlushLock } from './utils/flushLock'; import { defaultStackParser } from './vendor/stacktrace'; /** Get the default integrations for the Cloudflare SDK. */ diff --git a/packages/cloudflare/src/utils/cloneExecutionContext.ts b/packages/cloudflare/src/utils/cloneExecutionContext.ts deleted file mode 100644 index 4e978d3d3fb8..000000000000 --- a/packages/cloudflare/src/utils/cloneExecutionContext.ts +++ /dev/null @@ -1,16 +0,0 @@ -import { type ExecutionContext } from '@cloudflare/workers-types'; - -/** - * Clones the given execution context by creating a shallow copy while ensuring the binding of specific methods. - * - * @param {ExecutionContext|undefined} ctx - The execution context to clone. Can be undefined. - * @return {ExecutionContext|undefined} A cloned execution context with bound methods, or the original undefined value if no context was provided. - */ -export function cloneExecutionContext(ctx: T): T { - if (!ctx) return ctx; - return { - ...ctx, - ...('waitUntil' in ctx && { waitUntil: ctx.waitUntil.bind(ctx) }), - ...('passThroughOnException' in ctx && { passThroughOnException: ctx.passThroughOnException.bind(ctx) }), - }; -} diff --git a/packages/cloudflare/src/utils/copyExecutionContext.ts b/packages/cloudflare/src/utils/copyExecutionContext.ts new file mode 100644 index 000000000000..4bab39ef6d0f --- /dev/null +++ b/packages/cloudflare/src/utils/copyExecutionContext.ts @@ -0,0 +1,26 @@ +import { type ExecutionContext } from '@cloudflare/workers-types'; + +const kBound = Symbol.for('kBound'); + +/** + * Clones the given execution context by creating a shallow copy while ensuring the binding of specific methods. + * + * @param {ExecutionContext|void} ctx - The execution context to clone. Can be void. + * @return {ExecutionContext|void} A cloned execution context with bound methods, or the original void value if no context was provided. + */ +export function copyExecutionContext(ctx: T): T { + if (!ctx) return ctx; + return Object.assign({}, ctx, { + ...('waitUntil' in ctx && { waitUntil: copyBound(ctx, 'waitUntil') }), + ...('passThroughOnException' in ctx && { passThroughOnException: copyBound(ctx, 'passThroughOnException') }), + }); +} + +function copyBound(obj: T, method: K): T[K] { + const method_impl = obj[method]; + if (typeof method_impl !== 'function') return method_impl; + if ((method_impl as T[K] & { [kBound]?: boolean })[kBound]) return method_impl; + + const bound = method_impl.bind(obj); + return Object.defineProperty(bound, kBound, { value: true, enumerable: false }); +} diff --git a/packages/cloudflare/src/flush.ts b/packages/cloudflare/src/utils/flushLock.ts similarity index 80% rename from packages/cloudflare/src/flush.ts rename to packages/cloudflare/src/utils/flushLock.ts index adb719f41079..dbb8d3666749 100644 --- a/packages/cloudflare/src/flush.ts +++ b/packages/cloudflare/src/utils/flushLock.ts @@ -1,25 +1,25 @@ import type { ExecutionContext } from '@cloudflare/workers-types'; -import { createPromiseResolver } from './utils/makePromiseResolver'; +import { createPromiseResolver } from './makePromiseResolver'; type FlushLock = { readonly ready: Promise; readonly finalize: () => Promise; }; -type Lockable = T & { [kFlushLock]?: FlushLock }; +type MaybeLockable = T & { [kFlushLock]?: FlushLock }; const kFlushLock = Symbol.for('kFlushLock'); -function getInstrumentedLock(o: Lockable): FlushLock | undefined { +function getInstrumentedLock(o: MaybeLockable): FlushLock | undefined { return o[kFlushLock]; } -function storeInstrumentedLock(o: Lockable, lock: FlushLock): void { +function storeInstrumentedLock(o: MaybeLockable, lock: FlushLock): void { o[kFlushLock] = lock; } /** * 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 + * 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. diff --git a/packages/cloudflare/test/copy-execution-context.test.ts b/packages/cloudflare/test/copy-execution-context.test.ts new file mode 100644 index 000000000000..93378f8ae520 --- /dev/null +++ b/packages/cloudflare/test/copy-execution-context.test.ts @@ -0,0 +1,42 @@ +import { type ExecutionContext } from '@cloudflare/workers-types'; +import { type Mocked, describe, expect, it, vi } from 'vitest'; +import { copyExecutionContext } from '../src/utils/copyExecutionContext'; + +describe('Copy of the execution context', () => { + describe.for(['waitUntil', 'passThroughOnException'])('%s', method => { + it('Was not bound more than once', async () => { + const context = makeExecutionContextMock(); + const copy = copyExecutionContext(context); + const copy_of_copy = copyExecutionContext(copy); + + expect(copy[method]).toBe(copy_of_copy[method]); + }); + it('Copied method is bound to the original', async () => { + const context = makeExecutionContextMock(); + const copy = copyExecutionContext(context); + + expect(copy[method]()).toBe(context); + }); + }); + + it('No side effects', async () => { + const context = makeExecutionContextMock(); + expect(() => copyExecutionContext(Object.freeze(context))).not.toThrow( + /Cannot define property \w+, object is not extensible/, + ); + }); + it('Respects symbols', async () => { + const s = Symbol('test'); + const context = makeExecutionContextMock(); + context[s] = {}; + const copy = copyExecutionContext(context); + expect(copy[s]).toBe(context[s]); + }); +}); + +function makeExecutionContextMock() { + return { + waitUntil: vi.fn().mockReturnThis(), + passThroughOnException: vi.fn().mockReturnThis(), + } as unknown as Mocked; +} diff --git a/packages/cloudflare/test/flush.test.ts b/packages/cloudflare/test/flush-lock.test.ts similarity index 95% rename from packages/cloudflare/test/flush.test.ts rename to packages/cloudflare/test/flush-lock.test.ts index d6301ab27e54..cc40fd3a7e03 100644 --- a/packages/cloudflare/test/flush.test.ts +++ b/packages/cloudflare/test/flush-lock.test.ts @@ -1,6 +1,6 @@ import { type ExecutionContext } from '@cloudflare/workers-types'; import { describe, expect, it, vi } from 'vitest'; -import { makeFlushLock } from '../src/flush'; +import { makeFlushLock } from '../src/utils/flushLock'; import { createPromiseResolver } from '../src/utils/makePromiseResolver'; describe('Flush buffer test', () => { From cef17b3210ddc7cd6dea76a6de84359936b103a1 Mon Sep 17 00:00:00 2001 From: cod1k Date: Fri, 12 Sep 2025 14:14:47 +0300 Subject: [PATCH 09/10] Enhance `copyExecutionContext` with binding prevention Improved the `copyExecutionContext` utility to prevent re-binding of already bound methods using a proxy. Updated test cases to validate this behavior and added support for `DurableObjectState` as a compatible context type. --- .../src/utils/copyExecutionContext.ts | 36 +++++++++++++------ .../test/copy-execution-context.test.ts | 5 +++ 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/packages/cloudflare/src/utils/copyExecutionContext.ts b/packages/cloudflare/src/utils/copyExecutionContext.ts index 4bab39ef6d0f..c8e4807db54d 100644 --- a/packages/cloudflare/src/utils/copyExecutionContext.ts +++ b/packages/cloudflare/src/utils/copyExecutionContext.ts @@ -1,26 +1,42 @@ -import { type ExecutionContext } from '@cloudflare/workers-types'; +import { type DurableObjectState, type ExecutionContext } from '@cloudflare/workers-types'; const kBound = Symbol.for('kBound'); +const defaultPropertyOptions: PropertyDescriptor = { + enumerable: true, + configurable: true, + writable: true, +}; + /** * Clones the given execution context by creating a shallow copy while ensuring the binding of specific methods. * - * @param {ExecutionContext|void} ctx - The execution context to clone. Can be void. - * @return {ExecutionContext|void} A cloned execution context with bound methods, or the original void value if no context was provided. + * @param {ExecutionContext|DurableObjectState|void} ctx - The execution context to clone. Can be void. + * @return {ExecutionContext|DurableObjectState|void} A cloned execution context with bound methods, or the original void value if no context was provided. */ -export function copyExecutionContext(ctx: T): T { +export function copyExecutionContext(ctx: T): T { if (!ctx) return ctx; - return Object.assign({}, ctx, { - ...('waitUntil' in ctx && { waitUntil: copyBound(ctx, 'waitUntil') }), - ...('passThroughOnException' in ctx && { passThroughOnException: copyBound(ctx, 'passThroughOnException') }), + return Object.create(ctx, { + waitUntil: { ...defaultPropertyOptions, value: copyBound(ctx, 'waitUntil') }, + ...('passThroughOnException' in ctx && { + passThroughOnException: { ...defaultPropertyOptions, value: copyBound(ctx, 'passThroughOnException') }, + }), }); } -function copyBound(obj: T, method: K): T[K] { +function copyBound(obj: T, method: K): T[K] { const method_impl = obj[method]; if (typeof method_impl !== 'function') return method_impl; if ((method_impl as T[K] & { [kBound]?: boolean })[kBound]) return method_impl; - const bound = method_impl.bind(obj); - return Object.defineProperty(bound, kBound, { value: true, enumerable: false }); + return new Proxy(method_impl.bind(obj), { + get: (target, key, receiver) => { + if ('bind' === key) { + return () => receiver; + } else if (kBound === key) { + return true; + } + return Reflect.get(target, key, receiver); + }, + }); } diff --git a/packages/cloudflare/test/copy-execution-context.test.ts b/packages/cloudflare/test/copy-execution-context.test.ts index 93378f8ae520..61757cf1f355 100644 --- a/packages/cloudflare/test/copy-execution-context.test.ts +++ b/packages/cloudflare/test/copy-execution-context.test.ts @@ -17,6 +17,11 @@ describe('Copy of the execution context', () => { expect(copy[method]()).toBe(context); }); + it('Copied method "rebind" prevention', async () => { + const context = makeExecutionContextMock(); + const copy = copyExecutionContext(context); + expect(copy[method].bind('test')).toBe(copy[method]); + }); }); it('No side effects', async () => { From 0dc5c8bbca505a9cda43380a3b3ff11dc5d8cc26 Mon Sep 17 00:00:00 2001 From: cod1k Date: Mon, 15 Sep 2025 14:23:48 +0300 Subject: [PATCH 10/10] Refactor `copyExecutionContext` for method binding clarity Replaced `copyBound` with `copyAndBindMethod` to simplify and clarify the binding of methods like `waitUntil` and `passThroughOnException`. Improved the implementation to ensure proper proxy handling while retaining context. Added detailed JSDoc comments to enhance maintainability and understanding. --- .../src/utils/copyExecutionContext.ts | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/packages/cloudflare/src/utils/copyExecutionContext.ts b/packages/cloudflare/src/utils/copyExecutionContext.ts index c8e4807db54d..737a622d7a1c 100644 --- a/packages/cloudflare/src/utils/copyExecutionContext.ts +++ b/packages/cloudflare/src/utils/copyExecutionContext.ts @@ -17,26 +17,31 @@ const defaultPropertyOptions: PropertyDescriptor = { export function copyExecutionContext(ctx: T): T { if (!ctx) return ctx; return Object.create(ctx, { - waitUntil: { ...defaultPropertyOptions, value: copyBound(ctx, 'waitUntil') }, + waitUntil: { ...defaultPropertyOptions, value: copyAndBindMethod(ctx, 'waitUntil') }, ...('passThroughOnException' in ctx && { - passThroughOnException: { ...defaultPropertyOptions, value: copyBound(ctx, 'passThroughOnException') }, + passThroughOnException: { ...defaultPropertyOptions, value: copyAndBindMethod(ctx, 'passThroughOnException') }, }), }); } -function copyBound(obj: T, method: K): T[K] { - const method_impl = obj[method]; - if (typeof method_impl !== 'function') return method_impl; - if ((method_impl as T[K] & { [kBound]?: boolean })[kBound]) return method_impl; +/** + * Copies a method from the given object and ensures the copied method remains bound to the original object's context. + * + * @param {object} obj - The object containing the method to be copied and bound. + * @param {string|symbol} method - The key of the method within the object to be copied and bound. + * @return {Function} - The copied and bound method, or the original property if it is not a function. + */ +function copyAndBindMethod(obj: T, method: K): T[K] { + const methodImpl = obj[method]; + if (typeof methodImpl !== 'function') return methodImpl; + if ((methodImpl as T[K] & { [kBound]?: boolean })[kBound]) return methodImpl; + const bound = methodImpl.bind(obj); - return new Proxy(method_impl.bind(obj), { - get: (target, key, receiver) => { - if ('bind' === key) { - return () => receiver; - } else if (kBound === key) { - return true; - } - return Reflect.get(target, key, receiver); + return new Proxy(bound, { + get: (target, prop, receiver) => { + if (kBound === prop) return true; + if ('bind' === prop) return () => receiver; + return Reflect.get(target, prop, receiver); }, }); }