From 5ef69aa9d369770443e52b05ca799865a2e30819 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Mon, 14 Jul 2025 22:14:58 -0400 Subject: [PATCH 1/7] fix(replay): Fix re-sampled sessions after a click This fixes a bug where an expired session-based replay will always force a new session-based replay after a click, regardless of the actual recording mode. What is happening is: - a session-based replay is left idle - user comes back and clicks on the page and triggers the click listener - `addBreadcrumbEvent()` is called which then calls `triggerUserActivity()` because it is a click - next, `_checkSession()` and `_refreshSession()` are called and this is where the problem starts Inside of `_refreshSession` we stop the current replay (because the session is expired), however `stop()` is async and is `await`-ed before we re-sample. So the current replay state while `stop()` is finishing has: - `recordingMode` = `session` (initial value) - `isEnabled` = false Another however, `addBreadcrumbEvent` (and everything called until `_refreshSession`) are not async and does wait for resampling (`initializeSampling()`) to occur. This means that the click breadcrumb ends up causing a flush and always starting a new replay recording. --- .../suites/replay/sessionExpiry/test.ts | 63 ++++++++++++++++++- packages/replay-internal/src/replay.ts | 6 +- 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts b/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts index 996e39fbd21c..a34a1d045bae 100644 --- a/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts @@ -13,7 +13,7 @@ import { // Session should expire after 2s - keep in sync with init.js const SESSION_TIMEOUT = 2000; -sentryTest('handles an expired session', async ({ browserName, forceFlushReplay, getLocalTestUrl, page }) => { +sentryTest('handles an expired session that re-samples to session', async ({ browserName, forceFlushReplay, getLocalTestUrl, page }) => { if (shouldSkipReplayTest() || browserName !== 'chromium') { sentryTest.skip(); } @@ -65,3 +65,64 @@ sentryTest('handles an expired session', async ({ browserName, forceFlushReplay, const stringifiedSnapshot2 = normalize(fullSnapshots2[0]); expect(stringifiedSnapshot2).toMatchSnapshot('snapshot-2.json'); }); + +sentryTest('handles an expired session that re-samples to buffer', async ({ browserName, forceFlushReplay, getLocalTestUrl, page }) => { + if (shouldSkipReplayTest() || browserName !== 'chromium') { + sentryTest.skip(); + } + + const reqPromise0 = waitForReplayRequest(page, 0); + const reqPromise1 = waitForReplayRequest(page, 1); + + const url = await getLocalTestUrl({ testDir: __dirname }); + + await page.goto(url); + const req0 = await reqPromise0; + + const replayEvent0 = getReplayEvent(req0); + expect(replayEvent0).toEqual(getExpectedReplayEvent({})); + + const fullSnapshots0 = getFullRecordingSnapshots(req0); + expect(fullSnapshots0.length).toEqual(1); + const stringifiedSnapshot = normalize(fullSnapshots0[0]); + expect(stringifiedSnapshot).toMatchSnapshot('snapshot-0.json'); + + await page.locator('#button1').click(); + await forceFlushReplay(); + const req1 = await reqPromise1; + + const replayEvent1 = getReplayEvent(req1); + expect(replayEvent1).toEqual(getExpectedReplayEvent({ segment_id: 1, urls: [] })); + + const replay = await getReplaySnapshot(page); + const oldSessionId = replay.session?.id; + + await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT)); + await page.evaluate(() => { + // @ts-expect-error - Replay is not typed + window.Replay._replay._options.errorSampleRate = 1; + // @ts-expect-error - Replay is not typed + window.Replay._replay._options.sessionSampleRate = 0; + }); + + let wasReplayFlushed = false; + page.on('request', request => { + if (request.url().includes("/api/1337/envelope/")) { + wasReplayFlushed = true + } + }) + await page.locator('#button2').click(); + await forceFlushReplay(); + + // This timeout is not ideal, but not sure of a better way to ensure replay is not flushed + await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT)); + + expect(wasReplayFlushed).toBe(false); + + const currentSessionId = await page.evaluate(() => { + // @ts-expect-error - Replay is not typed + return window.Replay._replay.session?.id; + }); + + expect(currentSessionId).not.toEqual(oldSessionId); +}); diff --git a/packages/replay-internal/src/replay.ts b/packages/replay-internal/src/replay.ts index e2a49bd0a83b..329873a9d235 100644 --- a/packages/replay-internal/src/replay.ts +++ b/packages/replay-internal/src/replay.ts @@ -503,6 +503,10 @@ export class ReplayContainer implements ReplayContainerInterface { // enter into an infinite loop when `stop()` is called while flushing. this._isEnabled = false; + // Make sure to reset `recordingMode` to `buffer` to avoid any additional + // breadcrumbs to trigger a flush (e.g. in `addUpdate()`) + this.recordingMode = 'buffer'; + try { DEBUG_BUILD && debug.log(`Stopping Replay${reason ? ` triggered by ${reason}` : ''}`); @@ -623,7 +627,7 @@ export class ReplayContainer implements ReplayContainerInterface { // If this option is turned on then we will only want to call `flush` // explicitly - if (this.recordingMode === 'buffer') { + if (this.recordingMode === 'buffer' || !this._isEnabled) { return; } From 624de963c860c2c34f3963f384fb71c4e1a2e0e3 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 15 Jul 2025 09:12:32 -0400 Subject: [PATCH 2/7] lint --- .../suites/replay/sessionExpiry/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts b/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts index a34a1d045bae..4b35b9dbf822 100644 --- a/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts @@ -107,7 +107,7 @@ sentryTest('handles an expired session that re-samples to buffer', async ({ brow let wasReplayFlushed = false; page.on('request', request => { - if (request.url().includes("/api/1337/envelope/")) { + if (request.url().includes('/api/1337/envelope/')) { wasReplayFlushed = true } }) From 166528eac03a28166c68c3cc3f129a7fd848005a Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 15 Jul 2025 09:21:28 -0400 Subject: [PATCH 3/7] this shouldnt be undefined... --- .../suites/replay/sessionExpiry/test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts b/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts index 4b35b9dbf822..05315279cb4a 100644 --- a/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts @@ -100,9 +100,9 @@ sentryTest('handles an expired session that re-samples to buffer', async ({ brow await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT)); await page.evaluate(() => { // @ts-expect-error - Replay is not typed - window.Replay._replay._options.errorSampleRate = 1; + window.Replay._replay?._options?.errorSampleRate = 1; // @ts-expect-error - Replay is not typed - window.Replay._replay._options.sessionSampleRate = 0; + window.Replay._replay?._options?.sessionSampleRate = 0; }); let wasReplayFlushed = false; From e88f56e2e84c4fd2604bf9354fd8fe711d433623 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 15 Jul 2025 10:01:15 -0400 Subject: [PATCH 4/7] set recordingMode to session in start --- packages/replay-internal/src/replay.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/replay-internal/src/replay.ts b/packages/replay-internal/src/replay.ts index 329873a9d235..8bfebcbda173 100644 --- a/packages/replay-internal/src/replay.ts +++ b/packages/replay-internal/src/replay.ts @@ -392,6 +392,7 @@ export class ReplayContainer implements ReplayContainerInterface { ); this.session = session; + this.recordingMode = 'session'; this._initializeRecording(); } From 79745ee9095d7b0226312637ec824bd89b92acf4 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 15 Jul 2025 10:01:28 -0400 Subject: [PATCH 5/7] fix test --- .../test/integration/session.test.ts | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/packages/replay-internal/test/integration/session.test.ts b/packages/replay-internal/test/integration/session.test.ts index 5cf259755e47..147e366d6ca1 100644 --- a/packages/replay-internal/test/integration/session.test.ts +++ b/packages/replay-internal/test/integration/session.test.ts @@ -207,14 +207,40 @@ describe('Integration | session', () => { await vi.advanceTimersByTimeAsync(DEFAULT_FLUSH_MIN_DELAY); await new Promise(process.nextTick); - const newTimestamp = BASE_TIMESTAMP + ELAPSED; + + // The click actually does not trigger a flush because it never gets added to event buffer because + // the session is expired. We stop recording and re-sample the session expires. + expect(replay).not.toHaveLastSentReplay(); + + // This click will trigger a flush now that the session is active + // (sessionSampleRate=1 when resampling) + domHandler({ + name: 'click', + event: new Event('click'), + }); + await vi.advanceTimersByTimeAsync(DEFAULT_FLUSH_MIN_DELAY); + await new Promise(process.nextTick); + const newTimestamp = BASE_TIMESTAMP + ELAPSED + DEFAULT_FLUSH_MIN_DELAY; expect(replay).toHaveLastSentReplay({ recordingPayloadHeader: { segment_id: 0 }, recordingData: JSON.stringify([ - { data: { isCheckout: true }, timestamp: newTimestamp, type: 2 }, + { data: { isCheckout: true }, timestamp: newTimestamp-DEFAULT_FLUSH_MIN_DELAY, type: 2 }, optionsEvent, - // the click is lost, but that's OK + { + type: 5, + timestamp: newTimestamp, + data: { + tag: 'breadcrumb', + payload: { + timestamp: newTimestamp / 1000, + type: 'default', + category: 'ui.click', + message: '', + data: {}, + }, + }, + }, ]), }); @@ -224,7 +250,7 @@ describe('Integration | session', () => { // `_context` should be reset when a new session is created expect(replay.getContext()).toEqual({ initialUrl: 'http://dummy/', - initialTimestamp: newTimestamp, + initialTimestamp: newTimestamp-DEFAULT_FLUSH_MIN_DELAY, urls: [], errorIds: new Set(), traceIds: new Set(), From 0085f571194bc9f0cb5fc312aef5d94731316cb4 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 15 Jul 2025 10:15:05 -0400 Subject: [PATCH 6/7] lint + fix test --- .../suites/replay/sessionExpiry/test.ts | 173 +++++++++--------- .../test/integration/session.test.ts | 5 +- 2 files changed, 92 insertions(+), 86 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts b/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts index 05315279cb4a..f5278adb2f78 100644 --- a/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts @@ -1,4 +1,5 @@ import { expect } from '@playwright/test'; +import type { replayIntegration as actualReplayIntegration } from '@sentry-internal/replay'; import { sentryTest } from '../../../utils/fixtures'; import { getExpectedReplayEvent } from '../../../utils/replayEventTemplates'; import { @@ -13,116 +14,122 @@ import { // Session should expire after 2s - keep in sync with init.js const SESSION_TIMEOUT = 2000; -sentryTest('handles an expired session that re-samples to session', async ({ browserName, forceFlushReplay, getLocalTestUrl, page }) => { - if (shouldSkipReplayTest() || browserName !== 'chromium') { - sentryTest.skip(); - } +sentryTest( + 'handles an expired session that re-samples to session', + async ({ browserName, forceFlushReplay, getLocalTestUrl, page }) => { + if (shouldSkipReplayTest() || browserName !== 'chromium') { + sentryTest.skip(); + } - const reqPromise0 = waitForReplayRequest(page, 0); - const reqPromise1 = waitForReplayRequest(page, 1); + const reqPromise0 = waitForReplayRequest(page, 0); + const reqPromise1 = waitForReplayRequest(page, 1); - const url = await getLocalTestUrl({ testDir: __dirname }); + const url = await getLocalTestUrl({ testDir: __dirname }); - await page.goto(url); - const req0 = await reqPromise0; + await page.goto(url); + const req0 = await reqPromise0; - const replayEvent0 = getReplayEvent(req0); - expect(replayEvent0).toEqual(getExpectedReplayEvent({})); + const replayEvent0 = getReplayEvent(req0); + expect(replayEvent0).toEqual(getExpectedReplayEvent({})); - const fullSnapshots0 = getFullRecordingSnapshots(req0); - expect(fullSnapshots0.length).toEqual(1); - const stringifiedSnapshot = normalize(fullSnapshots0[0]); - expect(stringifiedSnapshot).toMatchSnapshot('snapshot-0.json'); + const fullSnapshots0 = getFullRecordingSnapshots(req0); + expect(fullSnapshots0.length).toEqual(1); + const stringifiedSnapshot = normalize(fullSnapshots0[0]); + expect(stringifiedSnapshot).toMatchSnapshot('snapshot-0.json'); - // We wait for another segment 0 - const reqPromise2 = waitForReplayRequest(page, 0); + // We wait for another segment 0 + const reqPromise2 = waitForReplayRequest(page, 0); - await page.locator('#button1').click(); - await forceFlushReplay(); - const req1 = await reqPromise1; + await page.locator('#button1').click(); + await forceFlushReplay(); + const req1 = await reqPromise1; - const replayEvent1 = getReplayEvent(req1); - expect(replayEvent1).toEqual(getExpectedReplayEvent({ segment_id: 1, urls: [] })); + const replayEvent1 = getReplayEvent(req1); + expect(replayEvent1).toEqual(getExpectedReplayEvent({ segment_id: 1, urls: [] })); - const replay = await getReplaySnapshot(page); - const oldSessionId = replay.session?.id; + const replay = await getReplaySnapshot(page); + const oldSessionId = replay.session?.id; - await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT)); + await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT)); - await page.locator('#button2').click(); - await forceFlushReplay(); - const req2 = await reqPromise2; + await page.locator('#button2').click(); + await forceFlushReplay(); + const req2 = await reqPromise2; - const replay2 = await getReplaySnapshot(page); + const replay2 = await getReplaySnapshot(page); - expect(replay2.session?.id).not.toEqual(oldSessionId); + expect(replay2.session?.id).not.toEqual(oldSessionId); - const replayEvent2 = getReplayEvent(req2); - expect(replayEvent2).toEqual(getExpectedReplayEvent({})); + const replayEvent2 = getReplayEvent(req2); + expect(replayEvent2).toEqual(getExpectedReplayEvent({})); - const fullSnapshots2 = getFullRecordingSnapshots(req2); - expect(fullSnapshots2.length).toEqual(1); - const stringifiedSnapshot2 = normalize(fullSnapshots2[0]); - expect(stringifiedSnapshot2).toMatchSnapshot('snapshot-2.json'); -}); + const fullSnapshots2 = getFullRecordingSnapshots(req2); + expect(fullSnapshots2.length).toEqual(1); + const stringifiedSnapshot2 = normalize(fullSnapshots2[0]); + expect(stringifiedSnapshot2).toMatchSnapshot('snapshot-2.json'); + }, +); -sentryTest('handles an expired session that re-samples to buffer', async ({ browserName, forceFlushReplay, getLocalTestUrl, page }) => { - if (shouldSkipReplayTest() || browserName !== 'chromium') { - sentryTest.skip(); - } +sentryTest( + 'handles an expired session that re-samples to buffer', + async ({ browserName, forceFlushReplay, getLocalTestUrl, page }) => { + if (shouldSkipReplayTest() || browserName !== 'chromium') { + sentryTest.skip(); + } - const reqPromise0 = waitForReplayRequest(page, 0); - const reqPromise1 = waitForReplayRequest(page, 1); + const reqPromise0 = waitForReplayRequest(page, 0); + const reqPromise1 = waitForReplayRequest(page, 1); - const url = await getLocalTestUrl({ testDir: __dirname }); + const url = await getLocalTestUrl({ testDir: __dirname }); - await page.goto(url); - const req0 = await reqPromise0; + await page.goto(url); + const req0 = await reqPromise0; - const replayEvent0 = getReplayEvent(req0); - expect(replayEvent0).toEqual(getExpectedReplayEvent({})); + const replayEvent0 = getReplayEvent(req0); + expect(replayEvent0).toEqual(getExpectedReplayEvent({})); - const fullSnapshots0 = getFullRecordingSnapshots(req0); - expect(fullSnapshots0.length).toEqual(1); - const stringifiedSnapshot = normalize(fullSnapshots0[0]); - expect(stringifiedSnapshot).toMatchSnapshot('snapshot-0.json'); + const fullSnapshots0 = getFullRecordingSnapshots(req0); + expect(fullSnapshots0.length).toEqual(1); + const stringifiedSnapshot = normalize(fullSnapshots0[0]); + expect(stringifiedSnapshot).toMatchSnapshot('snapshot-0.json'); - await page.locator('#button1').click(); - await forceFlushReplay(); - const req1 = await reqPromise1; + await page.locator('#button1').click(); + await forceFlushReplay(); + const req1 = await reqPromise1; - const replayEvent1 = getReplayEvent(req1); - expect(replayEvent1).toEqual(getExpectedReplayEvent({ segment_id: 1, urls: [] })); + const replayEvent1 = getReplayEvent(req1); + expect(replayEvent1).toEqual(getExpectedReplayEvent({ segment_id: 1, urls: [] })); - const replay = await getReplaySnapshot(page); - const oldSessionId = replay.session?.id; + const replay = await getReplaySnapshot(page); + const oldSessionId = replay.session?.id; - await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT)); - await page.evaluate(() => { - // @ts-expect-error - Replay is not typed - window.Replay._replay?._options?.errorSampleRate = 1; - // @ts-expect-error - Replay is not typed - window.Replay._replay?._options?.sessionSampleRate = 0; - }); + await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT)); + await page.evaluate(() => { + const replayIntegration = (window as unknown as Window & { Replay: ReturnType }) + .Replay; + replayIntegration['_replay'].getOptions().errorSampleRate = 1.0; + replayIntegration['_replay'].getOptions().sessionSampleRate = 1.0; + }); - let wasReplayFlushed = false; - page.on('request', request => { - if (request.url().includes('/api/1337/envelope/')) { - wasReplayFlushed = true - } - }) - await page.locator('#button2').click(); - await forceFlushReplay(); + let wasReplayFlushed = false; + page.on('request', request => { + if (request.url().includes('/api/1337/envelope/')) { + wasReplayFlushed = true; + } + }); + await page.locator('#button2').click(); + await forceFlushReplay(); - // This timeout is not ideal, but not sure of a better way to ensure replay is not flushed - await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT)); + // This timeout is not ideal, but not sure of a better way to ensure replay is not flushed + await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT)); - expect(wasReplayFlushed).toBe(false); + expect(wasReplayFlushed).toBe(false); - const currentSessionId = await page.evaluate(() => { - // @ts-expect-error - Replay is not typed - return window.Replay._replay.session?.id; - }); + const currentSessionId = await page.evaluate(() => { + // @ts-expect-error - Replay is not typed + return window.Replay._replay.session?.id; + }); - expect(currentSessionId).not.toEqual(oldSessionId); -}); + expect(currentSessionId).not.toEqual(oldSessionId); + }, +); diff --git a/packages/replay-internal/test/integration/session.test.ts b/packages/replay-internal/test/integration/session.test.ts index 147e366d6ca1..f867c43efbe8 100644 --- a/packages/replay-internal/test/integration/session.test.ts +++ b/packages/replay-internal/test/integration/session.test.ts @@ -207,7 +207,6 @@ describe('Integration | session', () => { await vi.advanceTimersByTimeAsync(DEFAULT_FLUSH_MIN_DELAY); await new Promise(process.nextTick); - // The click actually does not trigger a flush because it never gets added to event buffer because // the session is expired. We stop recording and re-sample the session expires. expect(replay).not.toHaveLastSentReplay(); @@ -225,7 +224,7 @@ describe('Integration | session', () => { expect(replay).toHaveLastSentReplay({ recordingPayloadHeader: { segment_id: 0 }, recordingData: JSON.stringify([ - { data: { isCheckout: true }, timestamp: newTimestamp-DEFAULT_FLUSH_MIN_DELAY, type: 2 }, + { data: { isCheckout: true }, timestamp: newTimestamp - DEFAULT_FLUSH_MIN_DELAY, type: 2 }, optionsEvent, { type: 5, @@ -250,7 +249,7 @@ describe('Integration | session', () => { // `_context` should be reset when a new session is created expect(replay.getContext()).toEqual({ initialUrl: 'http://dummy/', - initialTimestamp: newTimestamp-DEFAULT_FLUSH_MIN_DELAY, + initialTimestamp: newTimestamp - DEFAULT_FLUSH_MIN_DELAY, urls: [], errorIds: new Set(), traceIds: new Set(), From 5b506237a2d742a6970632eaa99fb19a6e65f909 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 15 Jul 2025 12:44:26 -0400 Subject: [PATCH 7/7] wrong sample rate --- .../suites/replay/sessionExpiry/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts b/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts index f5278adb2f78..e02007bc9f37 100644 --- a/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts @@ -108,7 +108,7 @@ sentryTest( const replayIntegration = (window as unknown as Window & { Replay: ReturnType }) .Replay; replayIntegration['_replay'].getOptions().errorSampleRate = 1.0; - replayIntegration['_replay'].getOptions().sessionSampleRate = 1.0; + replayIntegration['_replay'].getOptions().sessionSampleRate = 0.0; }); let wasReplayFlushed = false;