Skip to content

Commit ac3e6e5

Browse files
billyvgbillyvg
authored
fix(replay): Fix re-sampled sessions after a click (#17008)
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()`](https://github.com/getsentry/sentry-javascript/blob/develop/packages/replay-internal/src/coreHandlers/util/addBreadcrumbEvent.ts#L9) is called which then calls [`triggerUserActivity()`](https://github.com/getsentry/sentry-javascript/blob/develop/packages/replay-internal/src/coreHandlers/util/addBreadcrumbEvent.ts#L15) 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](https://github.com/getsentry/sentry-javascript/blob/develop/packages/replay-internal/src/replay.ts#L917-L918). 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 because we only check that [`recordingMode` is `buffer](https://github.com/getsentry/sentry-javascript/blob/develop/packages/replay-internal/src/replay.ts#L626)`. ## Solution When we call `stop()`, reset the `recordingMode` to `buffer` (this should be safe default as it is more restrictive behaviorally than `session`) and in `addUpdate`, add a check to see if `isEnabled()` is true (recording is enabled).
1 parent cd041e9 commit ac3e6e5

File tree

3 files changed

+140
-42
lines changed

3 files changed

+140
-42
lines changed

‎dev-packages/browser-integration-tests/suites/replay/sessionExpiry/test.ts‎

Lines changed: 105 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { expect } from '@playwright/test';
2+
import type { replayIntegration as actualReplayIntegration } from '@sentry-internal/replay';
23
import { sentryTest } from '../../../utils/fixtures';
34
import { getExpectedReplayEvent } from '../../../utils/replayEventTemplates';
45
import {
@@ -13,55 +14,122 @@ import {
1314
// Session should expire after 2s - keep in sync with init.js
1415
const SESSION_TIMEOUT = 2000;
1516

16-
sentryTest('handles an expired session', async ({ browserName, forceFlushReplay, getLocalTestUrl, page }) => {
17-
if (shouldSkipReplayTest() || browserName !== 'chromium') {
18-
sentryTest.skip();
19-
}
17+
sentryTest(
18+
'handles an expired session that re-samples to session',
19+
async ({ browserName, forceFlushReplay, getLocalTestUrl, page }) => {
20+
if (shouldSkipReplayTest() || browserName !== 'chromium') {
21+
sentryTest.skip();
22+
}
2023

21-
const reqPromise0 = waitForReplayRequest(page, 0);
22-
const reqPromise1 = waitForReplayRequest(page, 1);
24+
const reqPromise0 = waitForReplayRequest(page, 0);
25+
const reqPromise1 = waitForReplayRequest(page, 1);
2326

24-
const url = await getLocalTestUrl({ testDir: __dirname });
27+
const url = await getLocalTestUrl({ testDir: __dirname });
2528

26-
await page.goto(url);
27-
const req0 = await reqPromise0;
29+
await page.goto(url);
30+
const req0 = await reqPromise0;
2831

29-
const replayEvent0 = getReplayEvent(req0);
30-
expect(replayEvent0).toEqual(getExpectedReplayEvent({}));
32+
const replayEvent0 = getReplayEvent(req0);
33+
expect(replayEvent0).toEqual(getExpectedReplayEvent({}));
3134

32-
const fullSnapshots0 = getFullRecordingSnapshots(req0);
33-
expect(fullSnapshots0.length).toEqual(1);
34-
const stringifiedSnapshot = normalize(fullSnapshots0[0]);
35-
expect(stringifiedSnapshot).toMatchSnapshot('snapshot-0.json');
35+
const fullSnapshots0 = getFullRecordingSnapshots(req0);
36+
expect(fullSnapshots0.length).toEqual(1);
37+
const stringifiedSnapshot = normalize(fullSnapshots0[0]);
38+
expect(stringifiedSnapshot).toMatchSnapshot('snapshot-0.json');
3639

37-
// We wait for another segment 0
38-
const reqPromise2 = waitForReplayRequest(page, 0);
40+
// We wait for another segment 0
41+
const reqPromise2 = waitForReplayRequest(page, 0);
3942

40-
await page.locator('#button1').click();
41-
await forceFlushReplay();
42-
const req1 = await reqPromise1;
43+
await page.locator('#button1').click();
44+
await forceFlushReplay();
45+
const req1 = await reqPromise1;
4346

44-
const replayEvent1 = getReplayEvent(req1);
45-
expect(replayEvent1).toEqual(getExpectedReplayEvent({ segment_id: 1, urls: [] }));
47+
const replayEvent1 = getReplayEvent(req1);
48+
expect(replayEvent1).toEqual(getExpectedReplayEvent({ segment_id: 1, urls: [] }));
4649

47-
const replay = await getReplaySnapshot(page);
48-
const oldSessionId = replay.session?.id;
50+
const replay = await getReplaySnapshot(page);
51+
const oldSessionId = replay.session?.id;
4952

50-
await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT));
53+
await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT));
5154

52-
await page.locator('#button2').click();
53-
await forceFlushReplay();
54-
const req2 = await reqPromise2;
55+
await page.locator('#button2').click();
56+
await forceFlushReplay();
57+
const req2 = await reqPromise2;
5558

56-
const replay2 = await getReplaySnapshot(page);
59+
const replay2 = await getReplaySnapshot(page);
5760

58-
expect(replay2.session?.id).not.toEqual(oldSessionId);
61+
expect(replay2.session?.id).not.toEqual(oldSessionId);
5962

60-
const replayEvent2 = getReplayEvent(req2);
61-
expect(replayEvent2).toEqual(getExpectedReplayEvent({}));
63+
const replayEvent2 = getReplayEvent(req2);
64+
expect(replayEvent2).toEqual(getExpectedReplayEvent({}));
6265

63-
const fullSnapshots2 = getFullRecordingSnapshots(req2);
64-
expect(fullSnapshots2.length).toEqual(1);
65-
const stringifiedSnapshot2 = normalize(fullSnapshots2[0]);
66-
expect(stringifiedSnapshot2).toMatchSnapshot('snapshot-2.json');
67-
});
66+
const fullSnapshots2 = getFullRecordingSnapshots(req2);
67+
expect(fullSnapshots2.length).toEqual(1);
68+
const stringifiedSnapshot2 = normalize(fullSnapshots2[0]);
69+
expect(stringifiedSnapshot2).toMatchSnapshot('snapshot-2.json');
70+
},
71+
);
72+
73+
sentryTest(
74+
'handles an expired session that re-samples to buffer',
75+
async ({ browserName, forceFlushReplay, getLocalTestUrl, page }) => {
76+
if (shouldSkipReplayTest() || browserName !== 'chromium') {
77+
sentryTest.skip();
78+
}
79+
80+
const reqPromise0 = waitForReplayRequest(page, 0);
81+
const reqPromise1 = waitForReplayRequest(page, 1);
82+
83+
const url = await getLocalTestUrl({ testDir: __dirname });
84+
85+
await page.goto(url);
86+
const req0 = await reqPromise0;
87+
88+
const replayEvent0 = getReplayEvent(req0);
89+
expect(replayEvent0).toEqual(getExpectedReplayEvent({}));
90+
91+
const fullSnapshots0 = getFullRecordingSnapshots(req0);
92+
expect(fullSnapshots0.length).toEqual(1);
93+
const stringifiedSnapshot = normalize(fullSnapshots0[0]);
94+
expect(stringifiedSnapshot).toMatchSnapshot('snapshot-0.json');
95+
96+
await page.locator('#button1').click();
97+
await forceFlushReplay();
98+
const req1 = await reqPromise1;
99+
100+
const replayEvent1 = getReplayEvent(req1);
101+
expect(replayEvent1).toEqual(getExpectedReplayEvent({ segment_id: 1, urls: [] }));
102+
103+
const replay = await getReplaySnapshot(page);
104+
const oldSessionId = replay.session?.id;
105+
106+
await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT));
107+
await page.evaluate(() => {
108+
const replayIntegration = (window as unknown as Window & { Replay: ReturnType<typeof actualReplayIntegration> })
109+
.Replay;
110+
replayIntegration['_replay'].getOptions().errorSampleRate = 1.0;
111+
replayIntegration['_replay'].getOptions().sessionSampleRate = 0.0;
112+
});
113+
114+
let wasReplayFlushed = false;
115+
page.on('request', request => {
116+
if (request.url().includes('/api/1337/envelope/')) {
117+
wasReplayFlushed = true;
118+
}
119+
});
120+
await page.locator('#button2').click();
121+
await forceFlushReplay();
122+
123+
// This timeout is not ideal, but not sure of a better way to ensure replay is not flushed
124+
await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT));
125+
126+
expect(wasReplayFlushed).toBe(false);
127+
128+
const currentSessionId = await page.evaluate(() => {
129+
// @ts-expect-error - Replay is not typed
130+
return window.Replay._replay.session?.id;
131+
});
132+
133+
expect(currentSessionId).not.toEqual(oldSessionId);
134+
},
135+
);

‎packages/replay-internal/src/replay.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -392,6 +392,7 @@ export class ReplayContainer implements ReplayContainerInterface {
392392
);
393393

394394
this.session = session;
395+
this.recordingMode = 'session';
395396

396397
this._initializeRecording();
397398
}
@@ -503,6 +504,10 @@ export class ReplayContainer implements ReplayContainerInterface {
503504
// enter into an infinite loop when `stop()` is called while flushing.
504505
this._isEnabled = false;
505506

507+
// Make sure to reset `recordingMode` to `buffer` to avoid any additional
508+
// breadcrumbs to trigger a flush (e.g. in `addUpdate()`)
509+
this.recordingMode = 'buffer';
510+
506511
try {
507512
DEBUG_BUILD && debug.log(`Stopping Replay${reason ? ` triggered by ${reason}` : ''}`);
508513

@@ -623,7 +628,7 @@ export class ReplayContainer implements ReplayContainerInterface {
623628

624629
// If this option is turned on then we will only want to call `flush`
625630
// explicitly
626-
if (this.recordingMode === 'buffer') {
631+
if (this.recordingMode === 'buffer' || !this._isEnabled) {
627632
return;
628633
}
629634

‎packages/replay-internal/test/integration/session.test.ts‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -207,14 +207,39 @@ describe('Integration | session', () => {
207207
await vi.advanceTimersByTimeAsync(DEFAULT_FLUSH_MIN_DELAY);
208208
await new Promise(process.nextTick);
209209

210-
const newTimestamp = BASE_TIMESTAMP + ELAPSED;
210+
// The click actually does not trigger a flush because it never gets added to event buffer because
211+
// the session is expired. We stop recording and re-sample the session expires.
212+
expect(replay).not.toHaveLastSentReplay();
213+
214+
// This click will trigger a flush now that the session is active
215+
// (sessionSampleRate=1 when resampling)
216+
domHandler({
217+
name: 'click',
218+
event: new Event('click'),
219+
});
220+
await vi.advanceTimersByTimeAsync(DEFAULT_FLUSH_MIN_DELAY);
221+
await new Promise(process.nextTick);
222+
const newTimestamp = BASE_TIMESTAMP + ELAPSED + DEFAULT_FLUSH_MIN_DELAY;
211223

212224
expect(replay).toHaveLastSentReplay({
213225
recordingPayloadHeader: { segment_id: 0 },
214226
recordingData: JSON.stringify([
215-
{ data: { isCheckout: true }, timestamp: newTimestamp, type: 2 },
227+
{ data: { isCheckout: true }, timestamp: newTimestamp - DEFAULT_FLUSH_MIN_DELAY, type: 2 },
216228
optionsEvent,
217-
// the click is lost, but that's OK
229+
{
230+
type: 5,
231+
timestamp: newTimestamp,
232+
data: {
233+
tag: 'breadcrumb',
234+
payload: {
235+
timestamp: newTimestamp / 1000,
236+
type: 'default',
237+
category: 'ui.click',
238+
message: '<unknown>',
239+
data: {},
240+
},
241+
},
242+
},
218243
]),
219244
});
220245

@@ -224,7 +249,7 @@ describe('Integration | session', () => {
224249
// `_context` should be reset when a new session is created
225250
expect(replay.getContext()).toEqual({
226251
initialUrl: 'http://dummy/',
227-
initialTimestamp: newTimestamp,
252+
initialTimestamp: newTimestamp - DEFAULT_FLUSH_MIN_DELAY,
228253
urls: [],
229254
errorIds: new Set(),
230255
traceIds: new Set(),

0 commit comments

Comments
 (0)