fix(browser-utils): Fix soft navigation vital correlation race

packages/browser-utils/src/web-vitals/softNavs.ts

@@ -29,8 +29,15 @@ interface PendingNavigation {
   interactionTimestamp: number;
 }
 
+interface PendingInteraction {
+  interactionId: number;
+  interactionTimestamp: number;
+}
+
 // The navigation span whose triggering interaction we haven't identified yet.
 let _pendingNavigation: PendingNavigation | undefined;
+// The interaction whose Event Timing entry arrived before any navigation span claimed it.
+let _pendingInteraction: PendingInteraction | undefined;
 // The timestamp of the most recent trusted click/keydown, i.e. our best guess at the interaction
 // that a history change happening right now was driven by.
 let _lastInteractionTimestamp: number | undefined;
@@ -40,6 +47,14 @@ const _navigationIdToNavigationSpan = new LRUMap<number, Span>(MAX_TRACKED_NAVIG
 
 let _correlationStarted = false;
 
+/**
+ * Whether an Event Timing entry's `startTime` and a DOM event's `timeStamp` name the same
+ * interaction.
+ */
+function interactionMatches(entryStartTime: number, interactionTimestamp: number): boolean {
+  return Math.abs(entryStartTime - interactionTimestamp) <= INTERACTION_MATCH_TOLERANCE_MS;
+}
+
 /**
  * Whether the browser can report web vitals for soft navigations.
  *
@@ -104,23 +119,50 @@ export function startSoftNavigationCorrelation(client: Client): void {
 
     // A navigation with no preceding interaction can't produce a soft navigation, so there is
     // nothing to wait for. Dropping the pending span here also keeps us from binding a stale one.
-    _pendingNavigation =
-      _lastInteractionTimestamp != null ? { span, interactionTimestamp: _lastInteractionTimestamp } : undefined;
+    _pendingNavigation = undefined;
+    const interactionTimestamp = _lastInteractionTimestamp;
+    if (interactionTimestamp == null) {
+      return;
+    }
+
+    // The interaction's entry may already be here: the router code that starts this span races the
+    // paint that flushes the entry, so either one can win.
+    if (_pendingInteraction?.interactionTimestamp === interactionTimestamp) {
+      _interactionIdToNavigationSpan.set(_pendingInteraction.interactionId, span);
+      _pendingInteraction = undefined;
+      return;
+    }
+
+    _pendingNavigation = { span, interactionTimestamp };
   });
 
   const bindInteractionToNavigationSpan = ({ entries }: { entries: PerformanceEntry[] }): void => {
     for (const entry of entries) {
+      if (!isPerformanceEventTiming(entry) || !entry.interactionId) {
+        continue;
+      }
+
       const pending = _pendingNavigation;
-      if (!pending || !isPerformanceEventTiming(entry) || !entry.interactionId) {
+      if (pending && interactionMatches(entry.startTime, pending.interactionTimestamp)) {
+        _interactionIdToNavigationSpan.set(entry.interactionId, pending.span);
+        _pendingNavigation = undefined;
         continue;
       }
 
-      if (Math.abs(entry.startTime - pending.interactionTimestamp) > INTERACTION_MATCH_TOLERANCE_MS) {
+      // Once a navigation span has claimed this interaction, only a span that is still waiting can
+      // rebind it, which the check above already allows. Holding on to the interaction's remaining
+      // entries would instead let an unrelated later navigation claim it.
+      if (_interactionIdToNavigationSpan.get(entry.interactionId)) {
         continue;
       }
 
-      _interactionIdToNavigationSpan.set(entry.interactionId, pending.span);
-      _pendingNavigation = undefined;
+      // The navigation span this interaction drove may still be on its way, so hold on to the
+      // interaction instead of dropping it. Only the most recent one is worth keeping:
+      // `_lastInteractionTimestamp` is what `spanStart` matches against and it only moves forward,
+      // so an entry that doesn't match it now can never match it later.
+      if (_lastInteractionTimestamp != null && interactionMatches(entry.startTime, _lastInteractionTimestamp)) {
+        _pendingInteraction = { interactionId: entry.interactionId, interactionTimestamp: _lastInteractionTimestamp };
+      }
     }
   };
 

packages/browser-utils/test/web-vitals/softNavs.test.ts

@@ -56,6 +56,7 @@ describe('soft navigation correlation', () => {
 
   afterEach(() => {
     vi.unstubAllGlobals();
+    vi.restoreAllMocks();
     vi.clearAllMocks();
   });
 
@@ -76,6 +77,92 @@ describe('soft navigation correlation', () => {
     expect(getNavigationSpanForMetric({ navigationType: 'soft-navigation', navigationId: 7 })).toBe(navigationSpan);
   });
 
+  it('correlates when the interaction entry is delivered before the navigation span starts', async () => {
+    const { getNavigationSpanForMetric, startSoftNavigationCorrelation } = await loadSoftNavs();
+    const { client, startSpan } = createMockClient();
+
+    startSoftNavigationCorrelation(client as never);
+
+    windowListeners.get('click')?.({ isTrusted: true, timeStamp: 1234 });
+    // The router code that starts the span has not run yet, so the entry gets here first.
+    performanceHandlers.get('event')?.({ entries: [{ duration: 8, startTime: 1234, interactionId: 42 }] });
+
+    const navigationSpan = createMockSpan('navigation');
+    startSpan(navigationSpan);
+
+    performanceHandlers.get('soft-navigation')?.({ entries: [{ navigationId: 7, interactionId: 42 }] });
+
+    expect(navigationSpan.setAttribute).toHaveBeenCalledWith(BROWSER_NAVIGATION_ID, 7);
+    expect(getNavigationSpanForMetric({ navigationType: 'soft-navigation', navigationId: 7 })).toBe(navigationSpan);
+  });
+
+  it('does not let a later navigation steal an interaction a navigation already claimed', async () => {
+    const { getNavigationSpanForMetric, startSoftNavigationCorrelation } = await loadSoftNavs();
+    const { client, startSpan } = createMockClient();
+
+    startSoftNavigationCorrelation(client as never);
+
+    windowListeners.get('click')?.({ isTrusted: true, timeStamp: 1000 });
+    const navigationSpan = createMockSpan('navigation');
+    startSpan(navigationSpan);
+
+    // One interaction produces several entries. The first binds; the rest are delivered after the
+    // span is no longer pending.
+    performanceHandlers.get('event')?.({
+      entries: [
+        { duration: 8, startTime: 1000, interactionId: 42 },
+        { duration: 8, startTime: 999, interactionId: 42 },
+      ],
+    });
+
+    // A programmatic navigation, with no interaction of its own, must not claim interaction 42.
+    startSpan(createMockSpan('navigation'));
+
+    expect(
+      getNavigationSpanForMetric({ navigationType: 'soft-navigation', navigationId: 7, navigationInteractionId: 42 }),
+    ).toBe(navigationSpan);
+  });
+
+  it('correlates when the interaction handler ran long before the navigation span started', async () => {
+    const { getNavigationSpanForMetric, startSoftNavigationCorrelation } = await loadSoftNavs();
+    const { client, startSpan } = createMockClient();
+
+    startSoftNavigationCorrelation(client as never);
+
+    // A click whose handler blocks for seconds. These are the worst INP values on the page, so
+    // they're the ones that matter most, and the span still starts before the entry is delivered.
+    windowListeners.get('click')?.({ isTrusted: true, timeStamp: 1000 });
+    vi.spyOn(performance, 'now').mockReturnValue(3500);
+
+    const navigationSpan = createMockSpan('navigation');
+    startSpan(navigationSpan);
+    performanceHandlers.get('event')?.({ entries: [{ duration: 2500, startTime: 1000, interactionId: 42 }] });
+
+    expect(
+      getNavigationSpanForMetric({ navigationType: 'soft-navigation', navigationId: 7, navigationInteractionId: 42 }),
+    ).toBe(navigationSpan);
+  });
+
+  it('does not bind an early entry to a navigation from a different interaction', async () => {
+    const { getNavigationSpanForMetric, startSoftNavigationCorrelation } = await loadSoftNavs();
+    const { client, startSpan } = createMockClient();
+
+    startSoftNavigationCorrelation(client as never);
+
+    windowListeners.get('click')?.({ isTrusted: true, timeStamp: 500 });
+    performanceHandlers.get('event')?.({ entries: [{ duration: 8, startTime: 500, interactionId: 1 }] });
+
+    // A second click, whose own entry has not arrived, is what this navigation happened during.
+    windowListeners.get('click')?.({ isTrusted: true, timeStamp: 1234 });
+    const navigationSpan = createMockSpan('navigation');
+    startSpan(navigationSpan);
+
+    performanceHandlers.get('soft-navigation')?.({ entries: [{ navigationId: 7, interactionId: 1 }] });
+
+    expect(navigationSpan.setAttribute).not.toHaveBeenCalled();
+    expect(getNavigationSpanForMetric({ navigationType: 'soft-navigation', navigationId: 7 })).toBeUndefined();
+  });
+
   it('falls back to the interaction id when the soft navigation entry has not been observed yet', async () => {
     const { getNavigationSpanForMetric, startSoftNavigationCorrelation } = await loadSoftNavs();
     const { client, startSpan } = createMockClient();