From d78bb0fae54f4032fee59e0e718cfa1fb81152b3 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 18 Sep 2026 19:38:00 -0700 Subject: [PATCH 1/3] fix(browser-utils): Fix soft navigation vital correlation race Soft navigation CLS, LCP, and INP are joined to their navigation span through the interaction that triggered the navigation. The join only worked in one direction: `spanStart` parked the span in `_pendingNavigation`, and the Event Timing handler consumed it. An entry that arrived before the span saw no pending navigation, skipped, and was never reconsidered, so its `interactionId` never reached `_interactionIdToNavigationSpan` and all three vitals for that navigation were dropped. The two events race and neither is under the SDK's control. Entry delivery follows the paint after the interaction, while the navigation span starts from framework router code on the main thread. Under load the router code can slip behind the paint. Make the join work from either side. Entries with no matching pending navigation now go into a capped list, and `spanStart` claims a matching one before parking the span. The match rule and the 5ms tolerance are unchanged, so this does not loosen what counts as a match. It only drops the requirement that the span be registered first. This also fixes a second miss. It would discard any entry that failed the match against the current `_pendingNavigation`. A navigation whose entry never arrived left a stale pending span behind, and the next navigation's early entry was then thrown away against it. ref: #24354, #24366 fix: #24480 Co-Authored-By: Claude Opus 5 (1M context) --- .../browser-utils/src/web-vitals/softNavs.ts | 58 +++++++++++++++-- .../test/web-vitals/softNavs.test.ts | 64 +++++++++++++++++++ 2 files changed, 115 insertions(+), 7 deletions(-) diff --git a/packages/browser-utils/src/web-vitals/softNavs.ts b/packages/browser-utils/src/web-vitals/softNavs.ts index 9e397a60d4f6..653ab6f4c14b 100644 --- a/packages/browser-utils/src/web-vitals/softNavs.ts +++ b/packages/browser-utils/src/web-vitals/softNavs.ts @@ -18,6 +18,13 @@ const MAX_TRACKED_NAVIGATIONS = 5; */ const INTERACTION_MATCH_TOLERANCE_MS = 5; +/** + * How many interactions whose Event Timing entry outran the navigation span we keep around. Only + * the interaction a navigation happens during can match it, so a handful is plenty, and the cap + * stops a page with many interactions and no navigations from growing the list. + */ +const MAX_UNBOUND_INTERACTIONS = 20; + interface SoftNavMetric { navigationType: string; navigationId: number; @@ -29,8 +36,15 @@ interface PendingNavigation { interactionTimestamp: number; } +interface UnboundInteraction { + interactionId: number; + startTime: number; +} + // The navigation span whose triggering interaction we haven't identified yet. let _pendingNavigation: PendingNavigation | undefined; +// Interactions we have an Event Timing entry for but no navigation span yet, most recent last. +const _unboundInteractions: UnboundInteraction[] = []; // 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 +54,14 @@ const _navigationIdToNavigationSpan = new LRUMap(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 +126,45 @@ 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. + const unbound = _unboundInteractions.find(({ startTime }) => interactionMatches(startTime, interactionTimestamp)); + if (!unbound) { + _pendingNavigation = { span, interactionTimestamp }; + return; + } + + _interactionIdToNavigationSpan.set(unbound.interactionId, span); + // Every remaining entry is from an interaction at or before this one, so none of them can match + // a later navigation. + _unboundInteractions.length = 0; }); const bindInteractionToNavigationSpan = ({ entries }: { entries: PerformanceEntry[] }): void => { for (const entry of entries) { - const pending = _pendingNavigation; - if (!pending || !isPerformanceEventTiming(entry) || !entry.interactionId) { + if (!isPerformanceEventTiming(entry) || !entry.interactionId) { continue; } - if (Math.abs(entry.startTime - pending.interactionTimestamp) > INTERACTION_MATCH_TOLERANCE_MS) { + const pending = _pendingNavigation; + if (pending && interactionMatches(entry.startTime, pending.interactionTimestamp)) { + _interactionIdToNavigationSpan.set(entry.interactionId, pending.span); + _pendingNavigation = undefined; 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 entry + // instead of dropping it. + if (_unboundInteractions.length === MAX_UNBOUND_INTERACTIONS) { + _unboundInteractions.shift(); + } + _unboundInteractions.push({ interactionId: entry.interactionId, startTime: entry.startTime }); } }; diff --git a/packages/browser-utils/test/web-vitals/softNavs.test.ts b/packages/browser-utils/test/web-vitals/softNavs.test.ts index 8ccc70f89e07..abafba4f084e 100644 --- a/packages/browser-utils/test/web-vitals/softNavs.test.ts +++ b/packages/browser-utils/test/web-vitals/softNavs.test.ts @@ -76,6 +76,70 @@ 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 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('caps how many unbound interaction entries it holds', async () => { + const { getNavigationSpanForMetric, startSoftNavigationCorrelation } = await loadSoftNavs(); + const { client, startSpan } = createMockClient(); + + startSoftNavigationCorrelation(client as never); + + windowListeners.get('click')?.({ isTrusted: true, timeStamp: 1 }); + performanceHandlers.get('event')?.({ entries: [{ duration: 8, startTime: 1, interactionId: 1 }] }); + + // 20 later interactions push the first one out of the list. + for (let i = 0; i < 20; i++) { + const startTime = 100 + i * 100; + windowListeners.get('click')?.({ isTrusted: true, timeStamp: startTime }); + performanceHandlers.get('event')?.({ entries: [{ duration: 8, startTime, interactionId: i + 2 }] }); + } + + // A navigation for the evicted interaction can no longer find it. + windowListeners.get('click')?.({ isTrusted: true, timeStamp: 1 }); + startSpan(createMockSpan('navigation')); + + expect( + getNavigationSpanForMetric({ navigationType: 'soft-navigation', navigationId: 7, navigationInteractionId: 1 }), + ).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(); From c58b673ce97ec4d58bafa4d7eee4f81ea3dce371 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 18 Sep 2026 19:54:31 -0700 Subject: [PATCH 2/3] fix: bugbot finding, fix staleness bound and prevent stealing --- .../browser-utils/src/web-vitals/softNavs.ts | 18 ++++++- .../test/web-vitals/softNavs.test.ts | 49 +++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/packages/browser-utils/src/web-vitals/softNavs.ts b/packages/browser-utils/src/web-vitals/softNavs.ts index 653ab6f4c14b..b0d8fc24a93d 100644 --- a/packages/browser-utils/src/web-vitals/softNavs.ts +++ b/packages/browser-utils/src/web-vitals/softNavs.ts @@ -18,6 +18,14 @@ const MAX_TRACKED_NAVIGATIONS = 5; */ const INTERACTION_MATCH_TOLERANCE_MS = 5; +/** + * How long after an interaction a navigation can still be attributed to it. Without this bound a + * navigation that no interaction drove, such as a programmatic `router.push`, could claim the last + * interaction on the page however long ago it happened. `browserTracingIntegration` uses the same + * 1.5s window to decide whether a navigation followed a click, see its `REDIRECT_THRESHOLD`. + */ +const MAX_INTERACTION_AGE_MS = 1500; + /** * How many interactions whose Event Timing entry outran the navigation span we keep around. Only * the interaction a navigation happens during can match it, so a handful is plenty, and the cap @@ -128,7 +136,7 @@ export function startSoftNavigationCorrelation(client: Client): void { // nothing to wait for. Dropping the pending span here also keeps us from binding a stale one. _pendingNavigation = undefined; const interactionTimestamp = _lastInteractionTimestamp; - if (interactionTimestamp == null) { + if (interactionTimestamp == null || performance.now() - interactionTimestamp > MAX_INTERACTION_AGE_MS) { return; } @@ -159,6 +167,14 @@ export function startSoftNavigationCorrelation(client: Client): void { continue; } + // 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 the interaction's remaining + // entries would instead let an unrelated later navigation claim it through a stale + // `_lastInteractionTimestamp`. + if (_interactionIdToNavigationSpan.get(entry.interactionId)) { + continue; + } + // The navigation span this interaction drove may still be on its way, so hold on to the entry // instead of dropping it. if (_unboundInteractions.length === MAX_UNBOUND_INTERACTIONS) { diff --git a/packages/browser-utils/test/web-vitals/softNavs.test.ts b/packages/browser-utils/test/web-vitals/softNavs.test.ts index abafba4f084e..92fc2420cf87 100644 --- a/packages/browser-utils/test/web-vitals/softNavs.test.ts +++ b/packages/browser-utils/test/web-vitals/softNavs.test.ts @@ -52,10 +52,13 @@ describe('soft navigation correlation', () => { windowListeners.clear(); performanceHandlers.clear(); vi.stubGlobal('PerformanceObserver', { supportedEntryTypes: ['event', 'soft-navigation'] }); + // Pinned so the fixtures' interaction timestamps below stay inside `MAX_INTERACTION_AGE_MS`. + vi.spyOn(performance, 'now').mockReturnValue(1500); }); afterEach(() => { vi.unstubAllGlobals(); + vi.restoreAllMocks(); vi.clearAllMocks(); }); @@ -95,6 +98,52 @@ describe('soft navigation correlation', () => { 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('does not bind a navigation to an interaction that is too old to have driven it', async () => { + const { getNavigationSpanForMetric, startSoftNavigationCorrelation } = await loadSoftNavs(); + const { client, startSpan } = createMockClient(); + + startSoftNavigationCorrelation(client as never); + + // A click that drove no navigation, so its entries stay unbound. + windowListeners.get('click')?.({ isTrusted: true, timeStamp: 1000 }); + performanceHandlers.get('event')?.({ entries: [{ duration: 8, startTime: 1000, interactionId: 42 }] }); + + // Well past `MAX_INTERACTION_AGE_MS`, a programmatic navigation starts. + vi.spyOn(performance, 'now').mockReturnValue(4000); + startSpan(createMockSpan('navigation')); + + expect( + getNavigationSpanForMetric({ navigationType: 'soft-navigation', navigationId: 7, navigationInteractionId: 42 }), + ).toBeUndefined(); + }); + it('does not bind an early entry to a navigation from a different interaction', async () => { const { getNavigationSpanForMetric, startSoftNavigationCorrelation } = await loadSoftNavs(); const { client, startSpan } = createMockClient(); From 4f0f537111fb49dfea42430777c1ce9798e7e89f Mon Sep 17 00:00:00 2001 From: isaacs Date: Sun, 20 Sep 2026 16:37:28 -0700 Subject: [PATCH 3/3] fix: review comments fix the three concerns raised by @logaretm --- .../browser-utils/src/web-vitals/softNavs.ts | 52 ++++++------------- .../test/web-vitals/softNavs.test.ts | 42 +++------------ 2 files changed, 25 insertions(+), 69 deletions(-) diff --git a/packages/browser-utils/src/web-vitals/softNavs.ts b/packages/browser-utils/src/web-vitals/softNavs.ts index b0d8fc24a93d..addab70c50b0 100644 --- a/packages/browser-utils/src/web-vitals/softNavs.ts +++ b/packages/browser-utils/src/web-vitals/softNavs.ts @@ -18,21 +18,6 @@ const MAX_TRACKED_NAVIGATIONS = 5; */ const INTERACTION_MATCH_TOLERANCE_MS = 5; -/** - * How long after an interaction a navigation can still be attributed to it. Without this bound a - * navigation that no interaction drove, such as a programmatic `router.push`, could claim the last - * interaction on the page however long ago it happened. `browserTracingIntegration` uses the same - * 1.5s window to decide whether a navigation followed a click, see its `REDIRECT_THRESHOLD`. - */ -const MAX_INTERACTION_AGE_MS = 1500; - -/** - * How many interactions whose Event Timing entry outran the navigation span we keep around. Only - * the interaction a navigation happens during can match it, so a handful is plenty, and the cap - * stops a page with many interactions and no navigations from growing the list. - */ -const MAX_UNBOUND_INTERACTIONS = 20; - interface SoftNavMetric { navigationType: string; navigationId: number; @@ -44,15 +29,15 @@ interface PendingNavigation { interactionTimestamp: number; } -interface UnboundInteraction { +interface PendingInteraction { interactionId: number; - startTime: number; + interactionTimestamp: number; } // The navigation span whose triggering interaction we haven't identified yet. let _pendingNavigation: PendingNavigation | undefined; -// Interactions we have an Event Timing entry for but no navigation span yet, most recent last. -const _unboundInteractions: UnboundInteraction[] = []; +// 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; @@ -136,22 +121,19 @@ export function startSoftNavigationCorrelation(client: Client): void { // nothing to wait for. Dropping the pending span here also keeps us from binding a stale one. _pendingNavigation = undefined; const interactionTimestamp = _lastInteractionTimestamp; - if (interactionTimestamp == null || performance.now() - interactionTimestamp > MAX_INTERACTION_AGE_MS) { + 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. - const unbound = _unboundInteractions.find(({ startTime }) => interactionMatches(startTime, interactionTimestamp)); - if (!unbound) { - _pendingNavigation = { span, interactionTimestamp }; + if (_pendingInteraction?.interactionTimestamp === interactionTimestamp) { + _interactionIdToNavigationSpan.set(_pendingInteraction.interactionId, span); + _pendingInteraction = undefined; return; } - _interactionIdToNavigationSpan.set(unbound.interactionId, span); - // Every remaining entry is from an interaction at or before this one, so none of them can match - // a later navigation. - _unboundInteractions.length = 0; + _pendingNavigation = { span, interactionTimestamp }; }); const bindInteractionToNavigationSpan = ({ entries }: { entries: PerformanceEntry[] }): void => { @@ -168,19 +150,19 @@ export function startSoftNavigationCorrelation(client: Client): void { } // 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 the interaction's remaining - // entries would instead let an unrelated later navigation claim it through a stale - // `_lastInteractionTimestamp`. + // 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; } - // The navigation span this interaction drove may still be on its way, so hold on to the entry - // instead of dropping it. - if (_unboundInteractions.length === MAX_UNBOUND_INTERACTIONS) { - _unboundInteractions.shift(); + // 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 }; } - _unboundInteractions.push({ interactionId: entry.interactionId, startTime: entry.startTime }); } }; diff --git a/packages/browser-utils/test/web-vitals/softNavs.test.ts b/packages/browser-utils/test/web-vitals/softNavs.test.ts index 92fc2420cf87..a6b621b8a3b3 100644 --- a/packages/browser-utils/test/web-vitals/softNavs.test.ts +++ b/packages/browser-utils/test/web-vitals/softNavs.test.ts @@ -52,8 +52,6 @@ describe('soft navigation correlation', () => { windowListeners.clear(); performanceHandlers.clear(); vi.stubGlobal('PerformanceObserver', { supportedEntryTypes: ['event', 'soft-navigation'] }); - // Pinned so the fixtures' interaction timestamps below stay inside `MAX_INTERACTION_AGE_MS`. - vi.spyOn(performance, 'now').mockReturnValue(1500); }); afterEach(() => { @@ -125,23 +123,24 @@ describe('soft navigation correlation', () => { ).toBe(navigationSpan); }); - it('does not bind a navigation to an interaction that is too old to have driven it', async () => { + 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 that drove no navigation, so its entries stay unbound. + // 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 }); - performanceHandlers.get('event')?.({ entries: [{ duration: 8, startTime: 1000, interactionId: 42 }] }); + vi.spyOn(performance, 'now').mockReturnValue(3500); - // Well past `MAX_INTERACTION_AGE_MS`, a programmatic navigation starts. - vi.spyOn(performance, 'now').mockReturnValue(4000); - startSpan(createMockSpan('navigation')); + 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 }), - ).toBeUndefined(); + ).toBe(navigationSpan); }); it('does not bind an early entry to a navigation from a different interaction', async () => { @@ -164,31 +163,6 @@ describe('soft navigation correlation', () => { expect(getNavigationSpanForMetric({ navigationType: 'soft-navigation', navigationId: 7 })).toBeUndefined(); }); - it('caps how many unbound interaction entries it holds', async () => { - const { getNavigationSpanForMetric, startSoftNavigationCorrelation } = await loadSoftNavs(); - const { client, startSpan } = createMockClient(); - - startSoftNavigationCorrelation(client as never); - - windowListeners.get('click')?.({ isTrusted: true, timeStamp: 1 }); - performanceHandlers.get('event')?.({ entries: [{ duration: 8, startTime: 1, interactionId: 1 }] }); - - // 20 later interactions push the first one out of the list. - for (let i = 0; i < 20; i++) { - const startTime = 100 + i * 100; - windowListeners.get('click')?.({ isTrusted: true, timeStamp: startTime }); - performanceHandlers.get('event')?.({ entries: [{ duration: 8, startTime, interactionId: i + 2 }] }); - } - - // A navigation for the evicted interaction can no longer find it. - windowListeners.get('click')?.({ isTrusted: true, timeStamp: 1 }); - startSpan(createMockSpan('navigation')); - - expect( - getNavigationSpanForMetric({ navigationType: 'soft-navigation', navigationId: 7, navigationInteractionId: 1 }), - ).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();