From a56a1ecfd10c2fe4a143219a65cb67e357acb929 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Tue, 8 Sep 2026 11:03:48 +0200 Subject: [PATCH 1/4] fix(replay): Defer buffered replay upload until after error sampling In buffer (on-error) mode, the replay was flushed inside the beforeSend wrapper. Since @sentry/core 10.70.0 error sampleRate runs AFTER beforeSend, so a replay was uploaded even for errors later dropped by sampleRate, orphaning the replay and consuming quota (#6598). Split the work: link the event to the buffered replay id in beforeSend (read-only, no flush) and flush the native replay in an afterSendEvent hook, which only fires for events that survive sampling. To link before flushing, the current replay id must be readable while a buffer replay is still recording. On Android getCurrentReplayId now prefers ReplayController.getReplayId(); iOS relies on the matching sentry-cocoa change to SentrySDK.internal.replay.replayId. Co-Authored-By: Claude Opus 4.8 --- .../io/sentry/react/RNSentryModuleImpl.java | 11 ++ .../io/sentry/react/RNSentryReplayIdTest.java | 117 ++++++++++++++++ packages/core/src/js/replay/mobilereplay.ts | 103 +++++++++++--- .../test/replay/mobilereplay.sampling.test.ts | 128 ++++++++++++++++++ .../core/test/replay/mobilereplay.test.ts | 111 +++++++++++---- 5 files changed, 425 insertions(+), 45 deletions(-) create mode 100644 packages/core/android/src/test/java/io/sentry/react/RNSentryReplayIdTest.java create mode 100644 packages/core/test/replay/mobilereplay.sampling.test.ts diff --git a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java index 63ba50cfca..9e3ac7b1c0 100644 --- a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java +++ b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java @@ -485,6 +485,17 @@ public void captureReplay(boolean isHardCrash, Promise promise) { } public @Nullable String getCurrentReplayId() { + // Prefer the replay controller's id: it is assigned when recording starts + // (buffer or session) and is therefore available BEFORE a replay is + // flushed. The scope's replayId is only populated once a replay is sent, so + // it stays empty while a buffer (on-error) replay is recording (issue + // #6598). + final @NotNull SentryId controllerId = + Sentry.getCurrentScopes().getOptions().getReplayController().getReplayId(); + if (controllerId != SentryId.EMPTY_ID) { + return controllerId.toString(); + } + final @Nullable IScope scope = InternalSentrySdk.getCurrentScope(); if (scope == null) { return null; diff --git a/packages/core/android/src/test/java/io/sentry/react/RNSentryReplayIdTest.java b/packages/core/android/src/test/java/io/sentry/react/RNSentryReplayIdTest.java new file mode 100644 index 0000000000..1a439c606e --- /dev/null +++ b/packages/core/android/src/test/java/io/sentry/react/RNSentryReplayIdTest.java @@ -0,0 +1,117 @@ +package io.sentry.react; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import android.content.pm.PackageInfo; +import android.content.pm.PackageManager; +import com.facebook.react.bridge.ReactApplicationContext; +import io.sentry.IScope; +import io.sentry.IScopes; +import io.sentry.ReplayController; +import io.sentry.Sentry; +import io.sentry.SentryOptions; +import io.sentry.android.core.InternalSentrySdk; +import io.sentry.protocol.SentryId; +import org.junit.Before; +import org.junit.Test; +import org.mockito.MockedStatic; + +/** + * Coverage for {@link RNSentryModuleImpl#getCurrentReplayId()} and its buffer (on-error) replay + * lookup added for https://github.com/getsentry/sentry-react-native/issues/6598. + * + *

A buffer replay's id is assigned by the {@link ReplayController} when recording starts, but + * the scope's replayId is only populated once a replay is sent. The JS mobile replay integration + * must be able to read the buffered id BEFORE the replay is flushed, so {@code + * getCurrentReplayId()} prefers the controller's id and only falls back to the scope. + */ +public class RNSentryReplayIdTest { + + private RNSentryModuleImpl module; + + @Before + public void setUp() throws Exception { + ReactApplicationContext reactContext = mock(ReactApplicationContext.class); + PackageManager packageManager = mock(PackageManager.class); + when(packageManager.getPackageInfo(anyString(), anyInt())).thenReturn(new PackageInfo()); + when(reactContext.getPackageManager()).thenReturn(packageManager); + when(reactContext.getPackageName()).thenReturn("com.test.app"); + module = new RNSentryModuleImpl(reactContext); + } + + /** + * Wires {@code Sentry.getCurrentScopes().getOptions().getReplayController()} to return the id. + */ + private void stubControllerReplayId(final MockedStatic sentry, final SentryId id) { + final ReplayController replayController = mock(ReplayController.class); + when(replayController.getReplayId()).thenReturn(id); + final SentryOptions options = mock(SentryOptions.class); + when(options.getReplayController()).thenReturn(replayController); + final IScopes scopes = mock(IScopes.class); + when(scopes.getOptions()).thenReturn(options); + sentry.when(Sentry::getCurrentScopes).thenReturn(scopes); + } + + @Test + public void prefersReplayControllerIdWhenBuffering() { + // A buffer replay is recording: the controller exposes its id even though the scope has none. + final SentryId bufferedId = new SentryId(); + + try (MockedStatic sentry = mockStatic(Sentry.class); + MockedStatic internal = mockStatic(InternalSentrySdk.class)) { + stubControllerReplayId(sentry, bufferedId); + // Scope has no replay id yet — the fix must not depend on it. + final IScope scope = mock(IScope.class); + when(scope.getReplayId()).thenReturn(SentryId.EMPTY_ID); + internal.when(InternalSentrySdk::getCurrentScope).thenReturn(scope); + + assertEquals(bufferedId.toString(), module.getCurrentReplayId()); + } + } + + @Test + public void fallsBackToScopeIdWhenControllerEmpty() { + // A full-session replay was sent: the controller reports empty, the id lives on the scope. + final SentryId scopeId = new SentryId(); + + try (MockedStatic sentry = mockStatic(Sentry.class); + MockedStatic internal = mockStatic(InternalSentrySdk.class)) { + stubControllerReplayId(sentry, SentryId.EMPTY_ID); + final IScope scope = mock(IScope.class); + when(scope.getReplayId()).thenReturn(scopeId); + internal.when(InternalSentrySdk::getCurrentScope).thenReturn(scope); + + assertEquals(scopeId.toString(), module.getCurrentReplayId()); + } + } + + @Test + public void returnsNullWhenControllerEmptyAndScopeEmpty() { + try (MockedStatic sentry = mockStatic(Sentry.class); + MockedStatic internal = mockStatic(InternalSentrySdk.class)) { + stubControllerReplayId(sentry, SentryId.EMPTY_ID); + final IScope scope = mock(IScope.class); + when(scope.getReplayId()).thenReturn(SentryId.EMPTY_ID); + internal.when(InternalSentrySdk::getCurrentScope).thenReturn(scope); + + assertNull(module.getCurrentReplayId()); + } + } + + @Test + public void returnsNullWhenControllerEmptyAndScopeNull() { + try (MockedStatic sentry = mockStatic(Sentry.class); + MockedStatic internal = mockStatic(InternalSentrySdk.class)) { + stubControllerReplayId(sentry, SentryId.EMPTY_ID); + internal.when(InternalSentrySdk::getCurrentScope).thenReturn(null); + + assertNull(module.getCurrentReplayId()); + } + } +} diff --git a/packages/core/src/js/replay/mobilereplay.ts b/packages/core/src/js/replay/mobilereplay.ts index 2ea2bed89f..0cd6d9a399 100644 --- a/packages/core/src/js/replay/mobilereplay.ts +++ b/packages/core/src/js/replay/mobilereplay.ts @@ -345,7 +345,42 @@ export const mobileReplayIntegration = (initOptions: MobileReplayOptions = defau return nativeReplayId; } - async function processEvent(event: ErrorEvent, hint: EventHint): Promise { + // Error `sampleRate` sampling runs AFTER `beforeSend` in `@sentry/core` + // (since 10.70.0, getsentry/sentry-javascript#22819). Flushing the buffered + // replay inside `beforeSend` therefore uploads a replay even for errors that + // are then dropped by `sampleRate`, orphaning the replay (issue #6598). + // + // The work is split in two: + // 1. `tagEventWithReplayId` runs in the `beforeSend` wrapper and only links + // the event to the buffered replay id (no flush). + // 2. `flushReplayForSentEvent` runs in `afterSendEvent`, which fires only + // for events that survive sampling and are actually sent, and performs + // the native flush there. + // + // Trade-off: the link is tagged optimistically from the buffered id before the + // native `replaysOnErrorSampleRate` roll (which still happens at flush time in + // `captureReplay`). If that roll misses, the event carries a `replay_id` for a + // replay that is never uploaded. A fully-correct fix requires the native SDKs + // to decouple the on-error sampling decision from the buffer upload. + const MAX_PENDING_REPLAY_FLUSHES = 100; + + // event_ids linked to a buffered replay in `beforeSend` that still need the + // native replay flushed once the event survives sampling (in `afterSendEvent`). + const eventsPendingReplayFlush = new Set(); + + function markEventPendingReplayFlush(eventId: string): void { + // Bound the set: errors dropped by sampling never reach `afterSendEvent`, + // so their entries would otherwise accumulate. Evict oldest-first. + if (eventsPendingReplayFlush.size >= MAX_PENDING_REPLAY_FLUSHES) { + const oldest = eventsPendingReplayFlush.values().next().value; + if (oldest !== undefined) { + eventsPendingReplayFlush.delete(oldest); + } + } + eventsPendingReplayFlush.add(eventId); + } + + function tagEventWithReplayId(event: ErrorEvent, hint: EventHint): ErrorEvent { const hasException = event.exception?.values && event.exception.values.length > 0; if (!hasException) { // Event is not an error, will not capture replay @@ -370,39 +405,54 @@ export const mobileReplayIntegration = (initOptions: MobileReplayOptions = defau } } - const replayId = await NATIVE.captureReplay(isHardCrash(event)); + // Read the buffered replay id WITHOUT flushing. The native bridge returns + // the id assigned when recording started (buffer or full session), so the + // event can be linked to the replay that will be flushed after sampling. + const replayId = NATIVE.getCurrentReplayId(); if (replayId) { updateCachedReplayId(replayId); - debug.log( - `[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} Captured recording replay ${replayId} for event ${event.event_id}.`, - ); // Add replay_id to error event contexts to link replays to events/traces event.contexts = event.contexts || {}; event.contexts.replay = { ...event.contexts.replay, replay_id: replayId, }; + if (event.event_id) { + markEventPendingReplayFlush(event.event_id); + } + debug.log( + `[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} linked replay ${replayId} to event ${event.event_id}; flush deferred until after sampling.`, + ); } else { - // Check if there's an ongoing recording and update cache if found - const recordingReplayId = NATIVE.getCurrentReplayId(); - if (recordingReplayId) { - updateCachedReplayId(recordingReplayId); + updateCachedReplayId(null); + debug.log(`[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} no active recording for event ${event.event_id}.`); + } + + return event; + } + + async function flushReplayForSentEvent(event: Event): Promise { + const eventId = event.event_id; + if (!eventId || !eventsPendingReplayFlush.has(eventId)) { + return; + } + eventsPendingReplayFlush.delete(eventId); + + try { + const replayId = await NATIVE.captureReplay(isHardCrash(event)); + if (replayId) { + updateCachedReplayId(replayId); debug.log( - `[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} assign already recording replay ${recordingReplayId} for event ${event.event_id}.`, + `[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} flushed recording replay ${replayId} for sent event ${eventId}.`, ); - // Add replay_id to error event contexts to link replays to events/traces - event.contexts = event.contexts || {}; - event.contexts.replay = { - ...event.contexts.replay, - replay_id: recordingReplayId, - }; } else { - updateCachedReplayId(null); - debug.log(`[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} not sampled for event ${event.event_id}.`); + debug.log( + `[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} not sampled for event ${eventId} (replaysOnErrorSampleRate).`, + ); } + } catch (error) { + debug.error(`[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} Failed to flush replay for sent event ${eventId}`, error); } - - return event; } function setup(client: Client): void { @@ -498,12 +548,21 @@ export const mobileReplayIntegration = (initOptions: MobileReplayOptions = defau } } try { - return await processEvent(result, hint); + return tagEventWithReplayId(result, hint); } catch (error) { - debug.error(`[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} Failed to process event for replay`, error); + debug.error(`[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} Failed to link event to replay`, error); return result; } }; + + // Flush the buffered replay only for events that survive sampling. This hook + // fires after the error `sampleRate` roll in `@sentry/core`, so an error + // dropped by sampling never triggers a replay upload (issue #6598). + client.on('afterSendEvent', (event: Event) => { + flushReplayForSentEvent(event).then(undefined, () => { + // errors are logged inside flushReplayForSentEvent + }); + }); } function getReplayId(): string | null { diff --git a/packages/core/test/replay/mobilereplay.sampling.test.ts b/packages/core/test/replay/mobilereplay.sampling.test.ts new file mode 100644 index 0000000000..37ec7c7d7a --- /dev/null +++ b/packages/core/test/replay/mobilereplay.sampling.test.ts @@ -0,0 +1,128 @@ +import type { Event } from '@sentry/core'; + +import { afterEach, beforeEach, describe, expect, it, jest } from '@jest/globals'; +import { Client, createTransport } from '@sentry/core'; + +import { mobileReplayIntegration } from '../../src/js/replay/mobilereplay'; +import * as environment from '../../src/js/utils/environment'; +import { NATIVE } from '../../src/js/wrapper'; + +jest.mock('../../src/js/wrapper'); + +/** + * Regression coverage for https://github.com/getsentry/sentry-react-native/issues/6598 + * + * Drives the REAL @sentry/core capture pipeline so the production ordering + * applies: _prepareEvent -> beforeSend -> sampleRate drop -> sendEvent + * (-> afterSendEvent). The mobile replay integration links the event to the + * buffered replay id inside the wrapped `beforeSend`, but defers the native + * replay flush to `afterSendEvent`, which only fires for events that survive + * sampling and are actually sent. So an error dropped by `sampleRate` must not + * flush (and orphan) a replay. + */ + +const BUFFERED_REPLAY_ID = 'buffered-replay-id'; + +// Minimal concrete client over the real @sentry/core base pipeline. +class TestClient extends Client { + public eventFromException(exception: any): PromiseLike { + return Promise.resolve({ + event_id: 'test-event-id', + exception: { values: [{ type: 'Error', value: String(exception?.message ?? exception) }] }, + }); + } + public eventFromMessage(message: string): PromiseLike { + return Promise.resolve({ event_id: 'test-event-id', message }); + } +} + +function makeClient(sampleRate: number, sentEnvelopes: unknown[]): TestClient { + return new TestClient({ + dsn: 'https://public@example.com/1', + enableSend: true, + sampleRate, + integrations: [], + stackParser: () => [], + transport: opts => + createTransport(opts, req => { + sentEnvelopes.push(req.body); + return Promise.resolve({ statusCode: 200 }); + }), + }); +} + +describe('Issue 6598 — on-error replay must not orphan when sampleRate drops the error', () => { + let mockCaptureReplay: jest.MockedFunction; + let mockGetCurrentReplayId: jest.MockedFunction; + + beforeEach(() => { + jest.clearAllMocks(); + jest.spyOn(environment, 'isExpoGo').mockReturnValue(false); + jest.spyOn(environment, 'notMobileOs').mockReturnValue(false); + mockCaptureReplay = NATIVE.captureReplay as jest.MockedFunction; + mockCaptureReplay.mockResolvedValue('test-replay-id'); + mockGetCurrentReplayId = NATIVE.getCurrentReplayId as jest.MockedFunction; + // A buffer (on-error) replay is recording: the native bridge returns the + // buffered id, but nothing has been flushed/uploaded yet. + mockGetCurrentReplayId.mockReturnValue(BUFFERED_REPLAY_ID); + }); + + afterEach(() => jest.restoreAllMocks()); + + it('control: sampleRate = 1.0 → event sent, replay flushed, and event linked to the replay', async () => { + const sent: unknown[] = []; + const client = makeClient(1.0, sent); + + let sentEvent: Event | undefined; + client.on('beforeSendEvent', (event: Event) => { + sentEvent = event; + }); + + const integration = mobileReplayIntegration(); + integration.setup?.(client); + + client.captureException(new Error('boom')); + await client.flush(2000); + + // Event survived sampling and reached the transport... + expect(sent.length).toBe(1); + // ...the event was linked to the buffered replay in beforeSend... + expect(sentEvent?.contexts?.replay?.replay_id).toBe(BUFFERED_REPLAY_ID); + // ...and the buffered replay was flushed exactly once (in afterSendEvent). + expect(mockCaptureReplay).toHaveBeenCalledTimes(1); + }); + + it('fix: sampleRate = 0.0 → event dropped and replay is NOT flushed (no orphan)', async () => { + const sent: unknown[] = []; + const client = makeClient(0.0, sent); // 0.0 => always dropped by sampleRate + + const integration = mobileReplayIntegration(); + integration.setup?.(client); + + client.captureException(new Error('boom')); + await client.flush(2000); + + // The event never reached the transport (dropped by sampleRate after beforeSend)... + expect(sent.length).toBe(0); + // ...and crucially the native replay buffer was NOT flushed, so no dangling + // replay is uploaded and no replay quota is burned. This is the fix. + expect(mockCaptureReplay).not.toHaveBeenCalled(); + }); + + it('does not flush when there is no active recording', async () => { + mockGetCurrentReplayId.mockReturnValue(null); + const sent: unknown[] = []; + const client = makeClient(1.0, sent); + + const integration = mobileReplayIntegration(); + integration.setup?.(client); + + client.captureException(new Error('boom')); + await client.flush(2000); + + // Event is still sent, but with no buffered replay there is nothing to link + // or flush. + expect(sent.length).toBe(1); + expect(mockCaptureReplay).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/test/replay/mobilereplay.test.ts b/packages/core/test/replay/mobilereplay.test.ts index f7421d29ee..ed071c6ecf 100644 --- a/packages/core/test/replay/mobilereplay.test.ts +++ b/packages/core/test/replay/mobilereplay.test.ts @@ -49,8 +49,22 @@ describe('Mobile Replay Integration', () => { jest.restoreAllMocks(); }); + // Let deferred async work (the native flush in `afterSendEvent`) settle. + const flushAsync = (): Promise => new Promise(resolve => setImmediate(resolve)); + + // The native replay flush now happens in the `afterSendEvent` client hook, + // which only fires for events that survive sampling and are actually sent. + // Fire it explicitly to simulate an event being sent, then wait for the + // deferred flush to settle. + async function fireAfterSendEvent(event: Event): Promise { + const call = mockOn.mock.calls.find(c => c[0] === 'afterSendEvent'); + const handler = call?.[1] as ((event: Event, response?: unknown) => void) | undefined; + handler?.(event); + await flushAsync(); + } + describe('beforeSend wrapping', () => { - it('should capture replay after beforeSend processes the event', async () => { + it('links the event to the buffered replay in beforeSend and flushes it on send', async () => { const integration = mobileReplayIntegration(); integration.setup?.(mockClient); @@ -64,9 +78,14 @@ describe('Mobile Replay Integration', () => { const result = await clientOptions.beforeSend?.(event, hint); + // beforeSend only links the event to the buffered replay; it does not flush. expect(result).toBeDefined(); - expect(mockCaptureReplay).toHaveBeenCalled(); expect(result?.contexts?.replay?.replay_id).toBe('test-replay-id'); + expect(mockCaptureReplay).not.toHaveBeenCalled(); + + // The flush happens once the event survives sampling and is sent. + await fireAfterSendEvent(result as Event); + expect(mockCaptureReplay).toHaveBeenCalledTimes(1); }); it('should not capture replay when beforeSend returns null', async () => { @@ -115,9 +134,11 @@ describe('Mobile Replay Integration', () => { expect(result).toBeDefined(); expect(userBeforeSend).toHaveBeenCalledWith(event, hint); - expect(mockCaptureReplay).toHaveBeenCalled(); expect(result?.tags).toEqual({ modified: 'true' }); expect(result?.contexts?.replay?.replay_id).toBe('test-replay-id'); + + await fireAfterSendEvent(result as Event); + expect(mockCaptureReplay).toHaveBeenCalled(); }); it('should work when no user beforeSend is provided', async () => { @@ -135,8 +156,10 @@ describe('Mobile Replay Integration', () => { const result = await clientOptions.beforeSend?.(event, hint); expect(result).toBeDefined(); - expect(mockCaptureReplay).toHaveBeenCalled(); expect(result?.contexts?.replay?.replay_id).toBe('test-replay-id'); + + await fireAfterSendEvent(result as Event); + expect(mockCaptureReplay).toHaveBeenCalled(); }); it('should not process non-error events', async () => { @@ -156,9 +179,11 @@ describe('Mobile Replay Integration', () => { expect(result?.contexts?.replay).toBeUndefined(); }); - it('should handle errors in processEvent and return original event', async () => { - // Mock captureReplay to throw an error BEFORE setting up integration - mockCaptureReplay.mockRejectedValue(new Error('Native bridge error')); + it('should handle errors while linking the replay and return the original event', async () => { + // First call (during setup) succeeds; the call inside beforeSend throws. + mockGetCurrentReplayId.mockReturnValueOnce('test-replay-id').mockImplementation(() => { + throw new Error('Native bridge error'); + }); const integration = mobileReplayIntegration(); integration.setup?.(mockClient); @@ -173,10 +198,9 @@ describe('Mobile Replay Integration', () => { const result = await clientOptions.beforeSend?.(event, hint); - // Should return the original event even when processEvent fails + // Should return the original event even when linking fails expect(result).toBeDefined(); expect(result?.event_id).toBe('test-event-id'); - expect(mockCaptureReplay).toHaveBeenCalled(); }); it('should not crash the event pipeline when processEvent throws', async () => { @@ -219,6 +243,8 @@ describe('Mobile Replay Integration', () => { expect(result).toBeDefined(); expect(beforeErrorSampling).toHaveBeenCalledWith(event, hint); + + await fireAfterSendEvent(result as Event); expect(mockCaptureReplay).toHaveBeenCalled(); }); @@ -262,6 +288,8 @@ describe('Mobile Replay Integration', () => { expect(result).toBeDefined(); expect(beforeErrorSampling).toHaveBeenCalledWith(event, hint); + + await fireAfterSendEvent(result as Event); expect(mockCaptureReplay).toHaveBeenCalled(); }); @@ -280,6 +308,8 @@ describe('Mobile Replay Integration', () => { const result = await clientOptions.beforeSend?.(event, hint); expect(result).toBeDefined(); + + await fireAfterSendEvent(result as Event); expect(mockCaptureReplay).toHaveBeenCalled(); }); @@ -292,6 +322,11 @@ describe('Mobile Replay Integration', () => { const integration = mobileReplayIntegration({ beforeErrorSampling }); integration.setup?.(mockClient); + // Capture the afterSendEvent handler before the mid-test mock clear wipes + // the registration record. + const afterSendEventCall = mockOn.mock.calls.find(c => c[0] === 'afterSendEvent'); + const afterSendEvent = afterSendEventCall![1] as (event: Event) => void; + // Test with handled error const handledEvent = { event_id: 'handled-event-id', @@ -311,6 +346,10 @@ describe('Mobile Replay Integration', () => { expect(result1).toBeDefined(); expect(beforeErrorSampling).toHaveBeenCalledWith(handledEvent, hint); + + // The handled error was filtered out, so it is never linked or flushed. + afterSendEvent(result1 as Event); + await flushAsync(); expect(mockCaptureReplay).not.toHaveBeenCalled(); jest.clearAllMocks(); @@ -333,6 +372,9 @@ describe('Mobile Replay Integration', () => { expect(result2).toBeDefined(); expect(beforeErrorSampling).toHaveBeenCalledWith(unhandledEvent, hint); + + afterSendEvent(result2 as Event); + await flushAsync(); expect(mockCaptureReplay).toHaveBeenCalled(); }); @@ -373,7 +415,9 @@ describe('Mobile Replay Integration', () => { expect(result).toBeDefined(); expect(beforeErrorSampling).toHaveBeenCalledWith(event, hint); + // Should proceed with replay capture despite callback error + await fireAfterSendEvent(result as Event); expect(mockCaptureReplay).toHaveBeenCalled(); }); @@ -393,9 +437,12 @@ describe('Mobile Replay Integration', () => { const hint: EventHint = {}; // Should not throw - await expect(clientOptions.beforeSend?.(event, hint)).resolves.toBeDefined(); + const result = await clientOptions.beforeSend?.(event, hint); + expect(result).toBeDefined(); expect(beforeErrorSampling).toHaveBeenCalled(); + + await fireAfterSendEvent(result as Event); expect(mockCaptureReplay).toHaveBeenCalled(); }); @@ -425,9 +472,11 @@ describe('Mobile Replay Integration', () => { expect(result).toBeDefined(); expect(userBeforeSend).toHaveBeenCalledWith(event, hint); expect(beforeErrorSampling).toHaveBeenCalled(); - expect(mockCaptureReplay).toHaveBeenCalled(); expect(result?.tags).toEqual({ modified: 'true' }); expect(result?.contexts?.replay?.replay_id).toBe('test-replay-id'); + + await fireAfterSendEvent(result as Event); + expect(mockCaptureReplay).toHaveBeenCalled(); }); it('should not capture replay when user beforeSend drops event even if beforeErrorSampling returns true', async () => { @@ -456,8 +505,8 @@ describe('Mobile Replay Integration', () => { }); }); - describe('captureReplay returns null (native capture failed)', () => { - it('should not set replay_id when captureReplay returns null and no ongoing recording', async () => { + describe('native replay flush on send', () => { + it('does not link or flush when there is no active recording', async () => { mockCaptureReplay.mockResolvedValue(null); mockGetCurrentReplayId.mockReturnValue(null); @@ -480,14 +529,18 @@ describe('Mobile Replay Integration', () => { const result = await clientOptions.beforeSend?.(event, hint); + // No buffered recording => nothing to link... expect(result).toBeDefined(); - expect(mockCaptureReplay).toHaveBeenCalledWith(true); // isHardCrash expect(result?.contexts?.replay?.replay_id).toBeUndefined(); + + // ...and nothing to flush, even after the event is sent. + await fireAfterSendEvent(result as Event); + expect(mockCaptureReplay).not.toHaveBeenCalled(); }); - it('should use ongoing recording when captureReplay returns null but recording exists', async () => { + it('links the event to the ongoing recording and flushes it as a hard crash on send', async () => { mockCaptureReplay.mockResolvedValue(null); - // First call during setup returns initial ID, second call during processEvent returns ongoing ID + // First call during setup returns no ID, the call inside beforeSend returns the ongoing ID. mockGetCurrentReplayId.mockReturnValueOnce(null).mockReturnValue('ongoing-replay-id'); const integration = mobileReplayIntegration(); @@ -509,15 +562,18 @@ describe('Mobile Replay Integration', () => { const result = await clientOptions.beforeSend?.(event, hint); + // The event is linked to the ongoing recording in beforeSend. expect(result).toBeDefined(); - expect(mockCaptureReplay).toHaveBeenCalled(); - // Should fall back to ongoing recording ID expect(result?.contexts?.replay?.replay_id).toBe('ongoing-replay-id'); + + // The flush happens on send, propagating the hard-crash flag. + await fireAfterSendEvent(result as Event); + expect(mockCaptureReplay).toHaveBeenCalledWith(true); // isHardCrash }); - it('should set replay_id when captureReplay succeeds', async () => { + it('updates the cached replay id from the flushed replay after send', async () => { mockCaptureReplay.mockResolvedValue('new-replay-id'); - mockGetCurrentReplayId.mockReturnValue(null); + mockGetCurrentReplayId.mockReturnValue('buffered-replay-id'); const integration = mobileReplayIntegration(); integration.setup?.(mockClient); @@ -538,9 +594,14 @@ describe('Mobile Replay Integration', () => { const result = await clientOptions.beforeSend?.(event, hint); + // The event is linked to the buffered id at beforeSend time (stable through flush). expect(result).toBeDefined(); + expect(result?.contexts?.replay?.replay_id).toBe('buffered-replay-id'); + + // Once flushed on send, the cache reflects the id returned by the native flush. + await fireAfterSendEvent(result as Event); expect(mockCaptureReplay).toHaveBeenCalled(); - expect(result?.contexts?.replay?.replay_id).toBe('new-replay-id'); + expect(integration.getReplayId()).toBe('new-replay-id'); }); }); @@ -851,9 +912,13 @@ describe('Mobile Replay Integration', () => { } as ErrorEvent; const hint: EventHint = {}; - await clientOptions.beforeSend?.(event, hint); + const result = await clientOptions.beforeSend?.(event, hint); + + // Before the flush, the cache holds the buffered id linked in beforeSend. + expect(integration.getReplayId()).toBe(initialReplayId); - // Verify cache was updated by checking getReplayId + // The flush on send returns the final id and updates the cache. + await fireAfterSendEvent(result as Event); expect(integration.getReplayId()).toBe(newReplayId); // Extract the createDsc handler BEFORE clearing mocks From 04f6aa80c447f08d6a46cae6a7fa6a15c893bb48 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Tue, 8 Sep 2026 11:04:35 +0200 Subject: [PATCH 2/4] docs: Add changelog entry for buffered replay sampling fix Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8436b20dec..ebdd0bb55a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ - Fix visionOS compilation ([#6676](https://github.com/getsentry/sentry-react-native/pull/6676)) - A throwing `beforeBreadcrumb` now drops the breadcrumb, and a throwing `tracesSampler` now falls back to the configured `tracesSampleRate` ([#6675](https://github.com/getsentry/sentry-react-native/pull/6675)) +- Stop uploading on-error Session Replays for errors dropped by `sampleRate` ([#6685](https://github.com/getsentry/sentry-react-native/pull/6685)) ### Internal From a676da9a917e8281144331733baf75cbc927a06f Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Tue, 8 Sep 2026 11:57:19 +0200 Subject: [PATCH 3/4] fix(replay): Align Android buffered replay flush with iOS and harden cache Address review findings on the buffered-replay sampling fix: - Android captureReplay now resolves with the scope's replayId (populated only when a replay was actually sent) instead of the controller's buffered id, so an on-error sampling miss resolves null, matching iOS. - getCurrentReplayId still prefers the controller id for linking in beforeSend. - Drop the bounded pending-flush set in favour of keying the flush decision on the event's own linked replay context, removing an eviction race. - Re-read the current recording id when the native flush uploads nothing, so the cache no longer exposes an id that was never uploaded. Co-Authored-By: Claude Opus 4.8 --- .../io/sentry/react/RNSentryModuleImpl.java | 16 +++-- .../io/sentry/react/RNSentryReplayIdTest.java | 53 ++++++++++++++++- packages/core/src/js/replay/mobilereplay.ts | 31 +++------- .../core/test/replay/mobilereplay.test.ts | 58 +++++++++++++++++++ 4 files changed, 130 insertions(+), 28 deletions(-) diff --git a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java index 9e3ac7b1c0..e2bfbaf159 100644 --- a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java +++ b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java @@ -481,21 +481,29 @@ public void fetchNativeFramesDelay( public void captureReplay(boolean isHardCrash, Promise promise) { Sentry.getCurrentScopes().getOptions().getReplayController().captureReplay(isHardCrash); - promise.resolve(getCurrentReplayId()); + // Resolve with the scope's replayId, which is populated only when a replay + // was actually sent. Returning the controller's buffered id here would make + // JS treat an on-error sampling miss as a successful flush, diverging from + // iOS (which resolves nil unless a replay was captured). + promise.resolve(getReplayIdFromScope()); } public @Nullable String getCurrentReplayId() { // Prefer the replay controller's id: it is assigned when recording starts // (buffer or session) and is therefore available BEFORE a replay is - // flushed. The scope's replayId is only populated once a replay is sent, so - // it stays empty while a buffer (on-error) replay is recording (issue - // #6598). + // flushed, so a buffered (on-error) replay can be linked to the event in + // `beforeSend`. The scope's replayId is only populated once a replay is + // sent, so it stays empty while a buffer replay is recording (issue #6598). final @NotNull SentryId controllerId = Sentry.getCurrentScopes().getOptions().getReplayController().getReplayId(); if (controllerId != SentryId.EMPTY_ID) { return controllerId.toString(); } + return getReplayIdFromScope(); + } + + private @Nullable String getReplayIdFromScope() { final @Nullable IScope scope = InternalSentrySdk.getCurrentScope(); if (scope == null) { return null; diff --git a/packages/core/android/src/test/java/io/sentry/react/RNSentryReplayIdTest.java b/packages/core/android/src/test/java/io/sentry/react/RNSentryReplayIdTest.java index 1a439c606e..c621837fc1 100644 --- a/packages/core/android/src/test/java/io/sentry/react/RNSentryReplayIdTest.java +++ b/packages/core/android/src/test/java/io/sentry/react/RNSentryReplayIdTest.java @@ -6,10 +6,12 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import android.content.pm.PackageInfo; import android.content.pm.PackageManager; +import com.facebook.react.bridge.Promise; import com.facebook.react.bridge.ReactApplicationContext; import io.sentry.IScope; import io.sentry.IScopes; @@ -46,9 +48,11 @@ public void setUp() throws Exception { } /** - * Wires {@code Sentry.getCurrentScopes().getOptions().getReplayController()} to return the id. + * Wires {@code Sentry.getCurrentScopes().getOptions().getReplayController()} to return the id and + * hands back the mocked controller so callers can verify interactions with it. */ - private void stubControllerReplayId(final MockedStatic sentry, final SentryId id) { + private ReplayController stubControllerReplayId( + final MockedStatic sentry, final SentryId id) { final ReplayController replayController = mock(ReplayController.class); when(replayController.getReplayId()).thenReturn(id); final SentryOptions options = mock(SentryOptions.class); @@ -56,6 +60,7 @@ private void stubControllerReplayId(final MockedStatic sentry, final Sen final IScopes scopes = mock(IScopes.class); when(scopes.getOptions()).thenReturn(options); sentry.when(Sentry::getCurrentScopes).thenReturn(scopes); + return replayController; } @Test @@ -114,4 +119,48 @@ public void returnsNullWhenControllerEmptyAndScopeNull() { assertNull(module.getCurrentReplayId()); } } + + @Test + public void captureReplayResolvesNullOnSamplingMissEvenWhileBuffering() { + // On an on-error sampling miss the controller still holds the buffered id, but the scope has + // none because nothing was uploaded. captureReplay must resolve null (matching iOS), rather + // than leak the buffered id as if a replay had been sent. + final SentryId bufferedId = new SentryId(); + + try (MockedStatic sentry = mockStatic(Sentry.class); + MockedStatic internal = mockStatic(InternalSentrySdk.class)) { + final ReplayController replayController = stubControllerReplayId(sentry, bufferedId); + final IScope scope = mock(IScope.class); + when(scope.getReplayId()).thenReturn(SentryId.EMPTY_ID); + internal.when(InternalSentrySdk::getCurrentScope).thenReturn(scope); + + final Promise promise = mock(Promise.class); + module.captureReplay(true, promise); + + verify(replayController).captureReplay(true); + verify(promise).resolve(null); + } + } + + @Test + public void captureReplayResolvesScopeIdWhenReplayWasSent() { + // When a replay is actually sent the scope carries its id; captureReplay resolves that, not the + // controller's id, so JS learns the real uploaded replay id. + final SentryId bufferedId = new SentryId(); + final SentryId scopeId = new SentryId(); + + try (MockedStatic sentry = mockStatic(Sentry.class); + MockedStatic internal = mockStatic(InternalSentrySdk.class)) { + final ReplayController replayController = stubControllerReplayId(sentry, bufferedId); + final IScope scope = mock(IScope.class); + when(scope.getReplayId()).thenReturn(scopeId); + internal.when(InternalSentrySdk::getCurrentScope).thenReturn(scope); + + final Promise promise = mock(Promise.class); + module.captureReplay(false, promise); + + verify(replayController).captureReplay(false); + verify(promise).resolve(scopeId.toString()); + } + } } diff --git a/packages/core/src/js/replay/mobilereplay.ts b/packages/core/src/js/replay/mobilereplay.ts index 0cd6d9a399..3129542af7 100644 --- a/packages/core/src/js/replay/mobilereplay.ts +++ b/packages/core/src/js/replay/mobilereplay.ts @@ -362,23 +362,6 @@ export const mobileReplayIntegration = (initOptions: MobileReplayOptions = defau // `captureReplay`). If that roll misses, the event carries a `replay_id` for a // replay that is never uploaded. A fully-correct fix requires the native SDKs // to decouple the on-error sampling decision from the buffer upload. - const MAX_PENDING_REPLAY_FLUSHES = 100; - - // event_ids linked to a buffered replay in `beforeSend` that still need the - // native replay flushed once the event survives sampling (in `afterSendEvent`). - const eventsPendingReplayFlush = new Set(); - - function markEventPendingReplayFlush(eventId: string): void { - // Bound the set: errors dropped by sampling never reach `afterSendEvent`, - // so their entries would otherwise accumulate. Evict oldest-first. - if (eventsPendingReplayFlush.size >= MAX_PENDING_REPLAY_FLUSHES) { - const oldest = eventsPendingReplayFlush.values().next().value; - if (oldest !== undefined) { - eventsPendingReplayFlush.delete(oldest); - } - } - eventsPendingReplayFlush.add(eventId); - } function tagEventWithReplayId(event: ErrorEvent, hint: EventHint): ErrorEvent { const hasException = event.exception?.values && event.exception.values.length > 0; @@ -417,9 +400,6 @@ export const mobileReplayIntegration = (initOptions: MobileReplayOptions = defau ...event.contexts.replay, replay_id: replayId, }; - if (event.event_id) { - markEventPendingReplayFlush(event.event_id); - } debug.log( `[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} linked replay ${replayId} to event ${event.event_id}; flush deferred until after sampling.`, ); @@ -433,10 +413,13 @@ export const mobileReplayIntegration = (initOptions: MobileReplayOptions = defau async function flushReplayForSentEvent(event: Event): Promise { const eventId = event.event_id; - if (!eventId || !eventsPendingReplayFlush.has(eventId)) { + // Only flush for events that were linked to a buffered replay in + // `beforeSend`. The link lives on the event itself, so no shared bookkeeping + // is needed: events dropped by sampling never reach this hook and cannot + // affect the flush decision for other events. + if (!eventId || !event.contexts?.replay?.replay_id) { return; } - eventsPendingReplayFlush.delete(eventId); try { const replayId = await NATIVE.captureReplay(isHardCrash(event)); @@ -446,6 +429,10 @@ export const mobileReplayIntegration = (initOptions: MobileReplayOptions = defau `[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} flushed recording replay ${replayId} for sent event ${eventId}.`, ); } else { + // No replay was uploaded (e.g. an on-error sampling miss). Re-read the + // current recording id so the cache stops exposing an id that was never + // uploaded; it resolves to the still-active buffer id, or null. + updateCachedReplayId(NATIVE.getCurrentReplayId()); debug.log( `[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} not sampled for event ${eventId} (replaysOnErrorSampleRate).`, ); diff --git a/packages/core/test/replay/mobilereplay.test.ts b/packages/core/test/replay/mobilereplay.test.ts index ed071c6ecf..f13e604ba9 100644 --- a/packages/core/test/replay/mobilereplay.test.ts +++ b/packages/core/test/replay/mobilereplay.test.ts @@ -603,6 +603,64 @@ describe('Mobile Replay Integration', () => { expect(mockCaptureReplay).toHaveBeenCalled(); expect(integration.getReplayId()).toBe('new-replay-id'); }); + + it('re-reads the current recording id when the flush uploads nothing (on-error sampling miss)', async () => { + // A buffered replay is linked in beforeSend, but the native flush uploads + // nothing (an on-error sampling miss resolves null). The cache must not be + // left exposing the linked id as if it had been uploaded. + mockGetCurrentReplayId.mockReturnValue('buffered-replay-id'); + mockCaptureReplay.mockResolvedValue(null); + + const integration = mobileReplayIntegration(); + integration.setup?.(mockClient); + + const event = { + event_id: 'test-event-id', + exception: { + values: [{ type: 'Error', value: 'Test error', mechanism: { handled: false, type: 'onerror' } }], + }, + } as ErrorEvent; + + const result = await clientOptions.beforeSend?.(event, {}); + expect(result?.contexts?.replay?.replay_id).toBe('buffered-replay-id'); + + // After the miss, the still-active recording reports a fresh id. + mockGetCurrentReplayId.mockReturnValue('still-recording-id'); + await fireAfterSendEvent(result as Event); + + // The cache was refreshed from the current recording, not left stale. + expect(integration.getReplayId()).toBe('still-recording-id'); + }); + + it('flushes a sent event regardless of how many other events were linked first', async () => { + // Regression for the eviction race: the flush decision must live on the + // event itself, so a linked event still flushes after many other events + // (e.g. errors later dropped by sampling that never reach afterSendEvent) + // were linked in beforeSend. + mockGetCurrentReplayId.mockReturnValue('buffered-replay-id'); + + const integration = mobileReplayIntegration(); + integration.setup?.(mockClient); + + const makeEvent = (id: string) => + ({ + event_id: id, + exception: { values: [{ type: 'Error', value: 'Test error' }] }, + }) as ErrorEvent; + + // Link the first event but do not send it yet. + const first = await clientOptions.beforeSend?.(makeEvent('first-event-id'), {}); + expect(first?.contexts?.replay?.replay_id).toBe('buffered-replay-id'); + + // Link many more events afterwards without sending them. + for (let i = 0; i < 200; i++) { + await clientOptions.beforeSend?.(makeEvent(`event-${i}`), {}); + } + + // The first event is finally sent: it must still flush its replay. + await fireAfterSendEvent(first as Event); + expect(mockCaptureReplay).toHaveBeenCalledTimes(1); + }); }); describe('network detail feature markers', () => { From 649f560894dbc10a931a578e104578fafada0502 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Thu, 10 Sep 2026 12:01:19 +0200 Subject: [PATCH 4/4] fix(android): Use value equality for SentryId empty checks in replay id Reference equality (`!=` / `==` against SentryId.EMPTY_ID) would misclassify a freshly-constructed empty SentryId as non-empty, handing JS an all-zeros replay id. Compare by value via SentryId.EMPTY_ID.equals(...) instead, which is also null-safe as the left operand. Co-Authored-By: Claude Opus 4.8 --- .../src/main/java/io/sentry/react/RNSentryModuleImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java index e2bfbaf159..77ea3c0297 100644 --- a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java +++ b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java @@ -496,7 +496,7 @@ public void captureReplay(boolean isHardCrash, Promise promise) { // sent, so it stays empty while a buffer replay is recording (issue #6598). final @NotNull SentryId controllerId = Sentry.getCurrentScopes().getOptions().getReplayController().getReplayId(); - if (controllerId != SentryId.EMPTY_ID) { + if (!SentryId.EMPTY_ID.equals(controllerId)) { return controllerId.toString(); } @@ -510,7 +510,7 @@ public void captureReplay(boolean isHardCrash, Promise promise) { } final @NotNull SentryId id = scope.getReplayId(); - if (id == SentryId.EMPTY_ID) { + if (SentryId.EMPTY_ID.equals(id)) { return null; } return id.toString();