From 9b45d97dbe5784ea1604fc4cf4d04acdc7b57967 Mon Sep 17 00:00:00 2001 From: JPeer264 Date: Thu, 11 Dec 2025 13:39:11 +0100 Subject: [PATCH] fix(cloudflare): Missing events inside waitUntil --- .../cloudflare-workers/src/index.ts | 21 ++- .../cloudflare-workers/tests/index.test.ts | 102 +++++++++++++- packages/cloudflare/src/client.ts | 16 ++- packages/cloudflare/src/flush.ts | 14 +- packages/cloudflare/src/request.ts | 125 +++++++++--------- .../src/utils/endSpanAfterWaitUntil.ts | 17 +++ 6 files changed, 229 insertions(+), 66 deletions(-) create mode 100644 packages/cloudflare/src/utils/endSpanAfterWaitUntil.ts diff --git a/dev-packages/e2e-tests/test-applications/cloudflare-workers/src/index.ts b/dev-packages/e2e-tests/test-applications/cloudflare-workers/src/index.ts index ab438432a004..5534aeb486e3 100644 --- a/dev-packages/e2e-tests/test-applications/cloudflare-workers/src/index.ts +++ b/dev-packages/e2e-tests/test-applications/cloudflare-workers/src/index.ts @@ -81,7 +81,7 @@ export default Sentry.withSentry( }, }), { - async fetch(request, env) { + async fetch(request, env, ctx) { const url = new URL(request.url); switch (url.pathname) { case '/rpc/throwException': @@ -96,6 +96,25 @@ export default Sentry.withSentry( } } break; + case '/waitUntil': + console.log('waitUntil called'); + + const longRunningTask = async () => { + await new Promise(resolve => setTimeout(resolve, 2000)); + + console.log('ʕっ•ᴥ•ʔっ'); + Sentry.captureException(new Error('ʕノ•ᴥ•ʔノ ︵ ┻━┻')); + + return Sentry.startSpan({ name: 'longRunningTask' }, async () => { + await new Promise(resolve => setTimeout(resolve, 1000)); + console.log(' /|\ ^._.^ /|\ '); + }); + }; + + ctx.waitUntil(longRunningTask()); + + return new Response(null, { status: 200 }); + case '/throwException': throw new Error('To be recorded in Sentry.'); default: diff --git a/dev-packages/e2e-tests/test-applications/cloudflare-workers/tests/index.test.ts b/dev-packages/e2e-tests/test-applications/cloudflare-workers/tests/index.test.ts index 8c09693c81ed..f3252caede5e 100644 --- a/dev-packages/e2e-tests/test-applications/cloudflare-workers/tests/index.test.ts +++ b/dev-packages/e2e-tests/test-applications/cloudflare-workers/tests/index.test.ts @@ -1,5 +1,5 @@ import { expect, test } from '@playwright/test'; -import { waitForError, waitForRequest } from '@sentry-internal/test-utils'; +import { waitForError, waitForRequest, waitForTransaction } from '@sentry-internal/test-utils'; import { SDK_VERSION } from '@sentry/cloudflare'; import { WebSocket } from 'ws'; @@ -82,3 +82,103 @@ test('sends user-agent header with SDK name and version in envelope requests', a 'user-agent': `sentry.javascript.cloudflare/${SDK_VERSION}`, }); }); + +test.only('waitUntil', async ({ baseURL }) => { + const errorWaiter = waitForError( + 'cloudflare-workers', + event => event.exception?.values?.[0]?.value === 'ʕノ•ᴥ•ʔノ ︵ ┻━┻', + ); + const httpTransactionWaiter = waitForTransaction( + 'cloudflare-workers', + transactionEvent => transactionEvent.contexts?.trace?.op === 'http.server', + ); + + const response = await fetch(`${baseURL}/waitUntil`); + + expect(response.status).toBe(200); + + const [errorEvent, transactionEvent] = await Promise.all([errorWaiter, httpTransactionWaiter]); + + // ===== Error Event Assertions ===== + expect(errorEvent.exception?.values?.[0]).toMatchObject({ + type: 'Error', + value: 'ʕノ•ᴥ•ʔノ ︵ ┻━┻', + mechanism: { + type: 'generic', + handled: true, + }, + }); + + // Error should have trace context linking it to the transaction + expect(errorEvent.contexts?.trace?.trace_id).toBeDefined(); + expect(errorEvent.contexts?.trace?.span_id).toBeDefined(); + + // Error should have cloudflare-specific contexts + expect(errorEvent.contexts?.cloud_resource).toEqual({ 'cloud.provider': 'cloudflare' }); + expect(errorEvent.contexts?.runtime).toEqual({ name: 'cloudflare' }); + + // Error should have request data + expect(errorEvent.request).toMatchObject({ + method: 'GET', + url: expect.stringContaining('/waitUntil'), + }); + + // Error should have console breadcrumbs from before the error + expect(errorEvent.breadcrumbs).toEqual([ + expect.objectContaining({ category: 'console', message: 'waitUntil called' }), + expect.objectContaining({ category: 'console', message: 'ʕっ•ᴥ•ʔっ' }), + ]); + + // ===== Transaction Event Assertions ===== + expect(transactionEvent.transaction).toBe('GET /waitUntil'); + expect(transactionEvent.type).toBe('transaction'); + expect(transactionEvent.transaction_info?.source).toBe('url'); + + // Transaction trace context (root span - no status/response code, those are on the fetch child span) + expect(transactionEvent.contexts?.trace).toMatchObject({ + op: 'http.server', + status: 'ok', + origin: 'auto.http.cloudflare', + data: expect.objectContaining({ + 'sentry.op': 'http.server', + 'sentry.origin': 'auto.http.cloudflare', + 'http.request.method': 'GET', + 'url.path': '/waitUntil', + 'http.response.status_code': 200, + }), + }); + + expect(transactionEvent.spans).toEqual([ + expect.objectContaining({ + description: 'fetch', + op: 'http.server', + origin: 'auto.http.cloudflare', + parent_span_id: transactionEvent.contexts?.trace?.span_id, + }), + expect.objectContaining({ + description: 'waitUntil', + op: 'cloudflare.wait_until', + origin: 'manual', + parent_span_id: transactionEvent.spans?.[0]?.span_id, + }), + expect.objectContaining({ + description: 'longRunningTask', + origin: 'manual', + parent_span_id: transactionEvent.spans?.[0]?.span_id, + }), + ]); + + // Transaction should have all console breadcrumbs including the one after the span completes + expect(transactionEvent.breadcrumbs).toEqual([ + expect.objectContaining({ category: 'console', message: 'waitUntil called' }), + expect.objectContaining({ category: 'console', message: 'ʕっ•ᴥ•ʔっ' }), + expect.objectContaining({ category: 'console', message: ' /|\ ^._.^ /|\ ' }), + ]); + + // ===== Cross-event Assertions ===== + // Error and transaction should share the same trace_id + expect(transactionEvent.contexts?.trace?.trace_id).toBe(errorEvent.contexts?.trace?.trace_id); + + // The error's span_id should match the fetch span's span_id (error captured during waitUntil execution) + expect(errorEvent.contexts?.trace?.span_id).toBe(transactionEvent.spans?.[0]?.span_id); +}); diff --git a/packages/cloudflare/src/client.ts b/packages/cloudflare/src/client.ts index 3332f71dab90..9d0bf63a2d01 100644 --- a/packages/cloudflare/src/client.ts +++ b/packages/cloudflare/src/client.ts @@ -63,6 +63,18 @@ export class CloudflareClient extends ServerRuntimeClient { }); } + /** + * Returns a promise that resolves when all waitUntil promises have completed. + * This allows the root span to stay open until all waitUntil work is done. + * + * @return {Promise} A promise that resolves when all waitUntil promises are done. + */ + public async waitUntilDone(): Promise { + if (this._flushLock) { + await this._flushLock.finalize(); + } + } + /** * Flushes pending operations and ensures all data is processed. * If a timeout is provided, the operation will be completed within the specified time limit. @@ -73,9 +85,7 @@ export class CloudflareClient extends ServerRuntimeClient { * @return {Promise} 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(); - } + await this.waitUntilDone(); if (this._pendingSpans.size > 0 && this._spanCompletionPromise) { DEBUG_BUILD && diff --git a/packages/cloudflare/src/flush.ts b/packages/cloudflare/src/flush.ts index f38c805d0f8b..b524be1c0b78 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 { startSpan } from '@sentry/core'; type FlushLock = { readonly ready: Promise; @@ -22,9 +23,18 @@ export function makeFlushLock(context: ExecutionContext): FlushLock { const originalWaitUntil = context.waitUntil.bind(context) as typeof context.waitUntil; context.waitUntil = promise => { pending++; + return originalWaitUntil( - promise.finally(() => { - if (--pending === 0) resolveAllDone(); + // Wrap the promise in a new scope and transaction so spans created inside + // waitUntil callbacks are properly isolated from the HTTP request transaction + startSpan({ op: 'cloudflare.wait_until', name: 'waitUntil' }, async () => { + // By awaiting the promise inside the new scope, all of its continuations + // will execute in this isolated scope + await promise; + }).finally(() => { + if (--pending === 0) { + resolveAllDone(); + } }), ); }; diff --git a/packages/cloudflare/src/request.ts b/packages/cloudflare/src/request.ts index c404e57d01d8..42b04655e7d2 100644 --- a/packages/cloudflare/src/request.ts +++ b/packages/cloudflare/src/request.ts @@ -16,6 +16,7 @@ import { import type { CloudflareOptions } from './client'; import { addCloudResourceContext, addCultureContext, addRequest } from './scope-utils'; import { init } from './sdk'; +import { endSpanAfterWaitUntil } from './utils/endSpanAfterWaitUntil'; import { classifyResponseStreaming } from './utils/streaming'; interface RequestHandlerWrapperOptions { @@ -107,73 +108,79 @@ export function wrapRequestHandler( // See: https://developers.cloudflare.com/workers/runtime-apis/performance/ // Use startSpanManual to control when span ends (needed for streaming responses) - return startSpanManual({ name, attributes }, async span => { - let res: Response; - - try { - res = await handler(); - setHttpStatus(span, res.status); - - // After the handler runs, the span name might have been updated by nested instrumentation - // (e.g., Remix parameterizing routes). The span should already have the correct name - // from that instrumentation, so we don't need to do anything here. - } catch (e) { - span.end(); - if (captureErrors) { - captureException(e, { mechanism: { handled: false, type: 'auto.http.cloudflare' } }); - } - waitUntil?.(flush(2000)); - throw e; - } + return startSpanManual({ name, attributes }, async rootSpan => { + return startSpanManual({ name: 'fetch', attributes }, async fetchSpan => { + const finishSpansAndWaitUntil = (): void => { + fetchSpan.end(); + waitUntil?.(flush(2000)); + waitUntil?.(endSpanAfterWaitUntil(rootSpan)); + }; - // Classify response to detect actual streaming - const classification = classifyResponseStreaming(res); + let res: Response; - if (classification.isStreaming && res.body) { - // Streaming response detected - monitor consumption to keep span alive try { - const [clientStream, monitorStream] = res.body.tee(); + res = await handler(); + setHttpStatus(rootSpan, res.status); - // Monitor stream consumption and end span when complete - const streamMonitor = (async () => { - const reader = monitorStream.getReader(); + // After the handler runs, the span name might have been updated by nested instrumentation + // (e.g., Remix parameterizing routes). The span should already have the correct name + // from that instrumentation, so we don't need to do anything here. + } catch (e) { + // For errors, we still wait for waitUntil promises before ending the span + // so that any spans created in waitUntil callbacks are captured + if (captureErrors) { + captureException(e, { mechanism: { handled: false, type: 'auto.http.cloudflare' } }); + } + finishSpansAndWaitUntil(); + throw e; + } - try { - let done = false; - while (!done) { - const result = await reader.read(); - done = result.done; + // Classify response to detect actual streaming + const classification = classifyResponseStreaming(res); + + if (classification.isStreaming && res.body) { + // Streaming response detected - monitor consumption to keep span alive + try { + const [clientStream, monitorStream] = res.body.tee(); + + // Monitor stream consumption and end span when complete + const streamMonitor = (async () => { + const reader = monitorStream.getReader(); + + try { + let done = false; + while (!done) { + const result = await reader.read(); + done = result.done; + } + } catch { + // Stream error or cancellation - will end span in finally + } finally { + reader.releaseLock(); + finishSpansAndWaitUntil(); } - } catch { - // Stream error or cancellation - will end span in finally - } finally { - reader.releaseLock(); - span.end(); - waitUntil?.(flush(2000)); - } - })(); - - // Keep worker alive until stream monitoring completes (otherwise span won't end) - waitUntil?.(streamMonitor); - - // Return response with client stream - return new Response(clientStream, { - status: res.status, - statusText: res.statusText, - headers: res.headers, - }); - } catch (e) { - // tee() failed (e.g stream already locked) - fall back to non-streaming handling - span.end(); - waitUntil?.(flush(2000)); - return res; + })(); + + // Keep worker alive until stream monitoring completes (otherwise span won't end) + waitUntil?.(streamMonitor); + + // Return response with client stream + return new Response(clientStream, { + status: res.status, + statusText: res.statusText, + headers: res.headers, + }); + } catch (e) { + // tee() failed (e.g stream already locked) - fall back to non-streaming handling + finishSpansAndWaitUntil(); + return res; + } } - } - // Non-streaming response - end span immediately and return original - span.end(); - waitUntil?.(flush(2000)); - return res; + // Non-streaming response - end span after all waitUntil promises complete + finishSpansAndWaitUntil(); + return res; + }); }); }, ); diff --git a/packages/cloudflare/src/utils/endSpanAfterWaitUntil.ts b/packages/cloudflare/src/utils/endSpanAfterWaitUntil.ts new file mode 100644 index 000000000000..69df17df10f7 --- /dev/null +++ b/packages/cloudflare/src/utils/endSpanAfterWaitUntil.ts @@ -0,0 +1,17 @@ +import { flush, getClient, type Span } from '@sentry/core'; +import type { CloudflareClient } from '../client'; + +/** + * Helper to end span after all waitUntil promises complete. + * This ensures spans created in waitUntil callbacks are captured in the same transaction. + */ +export const endSpanAfterWaitUntil = async (span: Span): Promise => { + const cloudflareClient = getClient(); + + if (cloudflareClient) { + await cloudflareClient.waitUntilDone(); + } + + span.end(); + await flush(2000); +};