Fix memory leak and race condition

Author: onurtemizkan

dev-packages/e2e-tests/test-applications/remix-server-timing/app/entry.server.tsx

@@ -55,9 +55,6 @@ function handleBotRequest(
 
           responseHeaders.set('Content-Type', 'text/html');
 
-          // Server-Timing header is automatically injected by Sentry SDK
-          // via wrapRequestHandler in instrumentServer.ts
-
           resolve(
             new Response(stream, {
               headers: responseHeaders,
@@ -104,9 +101,6 @@ function handleBrowserRequest(
 
           responseHeaders.set('Content-Type', 'text/html');
 
-          // Server-Timing header is automatically injected by Sentry SDK
-          // via wrapRequestHandler in instrumentServer.ts
-
           resolve(
             new Response(stream, {
               headers: responseHeaders,

packages/remix/src/client/performance.tsx

@@ -90,14 +90,16 @@ function getTransactionNameAndSource(
   return { name: routeId, source: 'route' };
 }
 
+// Flag to prevent async callback from creating a span after navigation has occurred
+let pageloadSpanStarted = false;
+
 export function startPageloadSpan(client: Client): void {
   const initPathName = getInitPathName();
 
   if (!initPathName) {
     return;
   }
 
-  // Try to parameterize the route using the route manifest
   const parameterizedRoute = maybeParameterizeRemixRoute(initPathName);
   const spanName = parameterizedRoute || initPathName;
   const source = parameterizedRoute ? 'route' : 'url';
@@ -111,22 +113,27 @@ export function startPageloadSpan(client: Client): void {
     },
   };
 
-  // Try meta tags first (contains loader span ID for precise parent linking)
+  // Priority: meta tags > Server-Timing header > async retry
   const metaTagTrace = getMetaTagTraceContext();
   if (metaTagTrace) {
+    pageloadSpanStarted = true;
     startBrowserTracingPageLoadSpan(client, spanContext, metaTagTrace);
     return;
   }
 
-  // Fall back to Server-Timing header
   const serverTimingTrace = getNavigationTraceContext();
   if (serverTimingTrace) {
+    pageloadSpanStarted = true;
     startBrowserTracingPageLoadSpan(client, spanContext, serverTimingTrace);
     return;
   }
 
-  // Async retry for slow header processing
   getNavigationTraceContextAsync(trace => {
+    // Skip if pageload span was already started (e.g., navigation occurred during retry)
+    if (pageloadSpanStarted) {
+      return;
+    }
+    pageloadSpanStarted = true;
     startBrowserTracingPageLoadSpan(client, spanContext, trace ?? undefined);
   });
 }
@@ -223,6 +230,9 @@ export function withSentry<P extends Record<string, unknown>, R extends React.Co
       }
 
       if (_instrumentNavigation && matches?.length) {
+        // Mark pageload as started to prevent async callback from firing after navigation
+        pageloadSpanStarted = true;
+
         if (activeRootSpan) {
           activeRootSpan.end();
         }

packages/remix/src/client/serverTimingTracePropagation.ts

@@ -12,11 +12,14 @@ type NavigationTraceResult =
   | { status: 'unavailable' }
   | { status: 'available'; data: ServerTimingTraceContext };
 
-// Cache for navigation trace to avoid repeated parsing
+/**
+ * Cache for navigation trace context.
+ * - undefined: Not yet attempted to retrieve
+ * - null: Attempted but unavailable (no Server-Timing data or API not supported)
+ * - ServerTimingTraceContext: Successfully retrieved trace context
+ */
 let navigationTraceCache: ServerTimingTraceContext | null | undefined;
 
-// 40 attempts × 50ms = 2 seconds max wait for Performance API to process navigation entries
-// Aligned with react-router's hydration polling pattern (see hydratedRouter.ts)
 const MAX_RETRY_ATTEMPTS = 40;
 const RETRY_INTERVAL_MS = 50;
 
@@ -45,6 +48,11 @@ export function isServerTimingSupported(): boolean {
   }
 }
 
+/**
+ * Parses Server-Timing header entries to extract Sentry trace context.
+ * Expects entries with names 'sentry-trace' and 'baggage'.
+ * Baggage is URL-decoded as it's encoded in the Server-Timing header.
+ */
 function parseServerTimingTrace(serverTiming: readonly PerformanceServerTiming[]): ServerTimingTraceContext | null {
   let sentryTrace = '';
   let baggage = '';
@@ -75,6 +83,14 @@ function parseServerTimingTrace(serverTiming: readonly PerformanceServerTiming[]
   return { sentryTrace, baggage };
 }
 
+/**
+ * Attempts to retrieve trace context from the navigation performance entry.
+ *
+ * @returns
+ * - `{ status: 'available', data }` - Trace context successfully retrieved
+ * - `{ status: 'pending' }` - Headers not yet processed (responseStart === 0), retry recommended
+ * - `{ status: 'unavailable' }` - No Server-Timing data available, don't retry
+ */
 function tryGetNavigationTraceContext(): NavigationTraceResult {
   try {
     const navEntries = WINDOW.performance.getEntriesByType('navigation');
@@ -137,27 +153,39 @@ export function getNavigationTraceContext(): ServerTimingTraceContext | null {
 /**
  * Get trace context from navigation with retry mechanism.
  * Useful during SDK init when browser may not have finished processing headers.
+ *
+ * @returns Cleanup function to cancel pending retries (e.g., on navigation)
  */
 export function getNavigationTraceContextAsync(
   callback: (trace: ServerTimingTraceContext | null) => void,
   maxAttempts: number = MAX_RETRY_ATTEMPTS,
   delayMs: number = RETRY_INTERVAL_MS,
-): void {
+): () => void {
+  const state = { cancelled: false };
+
   if (navigationTraceCache !== undefined) {
     callback(navigationTraceCache);
-    return;
+    return () => {
+      state.cancelled = true;
+    };
   }
 
   if (!isServerTimingSupported()) {
     DEBUG_BUILD && debug.log('[Server-Timing] Server-Timing API not supported');
     navigationTraceCache = null;
     callback(null);
-    return;
+    return () => {
+      state.cancelled = true;
+    };
   }
 
   let attempts = 0;
 
   const tryGet = (): void => {
+    if (state.cancelled) {
+      return;
+    }
+
     attempts++;
     const result = tryGetNavigationTraceContext();
 
@@ -171,7 +199,7 @@ export function getNavigationTraceContextAsync(
           setTimeout(tryGet, delayMs);
           return;
         }
-        DEBUG_BUILD && debug.log('[Server-Timing] Max retry attempts reached');
+        DEBUG_BUILD && debug.warn('[Server-Timing] Max retry attempts reached, trace context unavailable');
         navigationTraceCache = null;
         callback(null);
         return;
@@ -182,10 +210,14 @@ export function getNavigationTraceContextAsync(
   };
 
   tryGet();
+
+  return () => {
+    state.cancelled = true;
+  };
 }
 
 /**
- * Get trace context from meta tags (fallback for older browsers).
+ * Get trace context from meta tags.
  * Looks for `<meta name="sentry-trace">` and `<meta name="baggage">` tags.
  */
 export function getMetaTagTraceContext(): ServerTimingTraceContext | null {

packages/remix/src/server/instrumentServer.ts

@@ -104,30 +104,32 @@ export function wrapHandleErrorWithSentry(
   };
 }
 
+/**
+ * Get trace context for injection into loader response data.
+ * Prioritizes active span context to ensure client pageload continues from the loader span,
+ * not the http.server span, enabling proper trace continuity via Server-Timing headers.
+ */
 function getTraceAndBaggage(): {
   sentryTrace?: string;
   sentryBaggage?: string;
 } {
   if (isNodeEnv() || isCloudflareEnv()) {
-    // Use activeSpan (loader) so client's pageload parent is the loader, not http.server
     const activeSpan = getActiveSpan();
     if (activeSpan) {
       const sentryTrace = spanToTraceHeader(activeSpan);
       if (sentryTrace) {
         return {
           sentryTrace,
-          sentryBaggage: spanToBaggageHeader(activeSpan) || '',
+          sentryBaggage: spanToBaggageHeader(activeSpan),
         };
       }
     }
 
-    // Fall back to scope's propagation context
     const scope = getCurrentScope();
     const propagationContext = scope.getPropagationContext();
     const traceData = getTraceData();
     const spanId = propagationContext.propagationSpanId ?? propagationContext.parentSpanId;
 
-    // Only generate if we have valid IDs to avoid "traceid-undefined-1" format
     if (propagationContext.traceId && spanId) {
       const fallbackTrace = generateSentryTraceHeader(propagationContext.traceId, spanId, propagationContext.sampled);
       DEBUG_BUILD && debug.log('[getTraceAndBaggage] Falling back to propagation context:', fallbackTrace);
@@ -381,10 +383,12 @@ function wrapRequestHandler<T extends ServerBuild | (() => ServerBuild | Promise
         return (await origRequestHandler.call(this, request, loadContext)) as Response;
       }
 
+      // We update the existing http.server span (created by OTel) with Remix-specific
+      // attributes rather than creating a nested child span. This ensures proper trace
+      // hierarchy where the http.server span is the parent of loader/action spans.
       const handleRequest = async (): Promise<Response> => {
         const res = (await origRequestHandler.call(this, request, loadContext)) as Response;
 
-        // Span may be null if OTel has already ended it
         const activeSpan = getActiveSpan();
         const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined;
 
@@ -410,6 +414,8 @@ function wrapRequestHandler<T extends ServerBuild | (() => ServerBuild | Promise
         return res;
       };
 
+      // Only continue trace if there's an incoming sentry-trace header.
+      // Otherwise, start a fresh trace in the isolation scope.
       if (sentryTrace) {
         return continueTrace({ sentryTrace, baggage: baggage || '' }, handleRequest);
       }