Skip to content

Commit 993303c

Browse files
billyvgbillyvg
authored
fix(replay): Linked errors not resetting session id (#17854)
This PR fixes a case where we [correctly] tag an error event w/ replay id, but something occurs where the replay event does not end up being flushed. This means the existing session is still in a buffered state, and will keep its session id until a new error event is sampled and a replay is created. When this does happen, we can have a replay with a super long duration (e.g. the time between the two error replays). We now update the session immediately when we tag an error event w/ replay id so that if the replay event does not successfully flush, the session will respect its expiration date.
1 parent 0e15b3d commit 993303c

File tree

12 files changed

+369
-4
lines changed

12 files changed

+369
-4
lines changed

‎dev-packages/browser-integration-tests/suites/replay/bufferStalledRequests/init.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
window.Replay = Sentry.replayIntegration({
5+
flushMinDelay: 200,
6+
flushMaxDelay: 200,
7+
minReplayDuration: 0,
8+
stickySession: true,
9+
});
10+
11+
Sentry.init({
12+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
13+
sampleRate: 1,
14+
replaysSessionSampleRate: 0.0,
15+
replaysOnErrorSampleRate: 1.0,
16+
17+
integrations: [window.Replay],
18+
});

‎dev-packages/browser-integration-tests/suites/replay/bufferStalledRequests/subject.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
document.getElementById('error1').addEventListener('click', () => {
2+
throw new Error('First Error');
3+
});
4+
5+
document.getElementById('error2').addEventListener('click', () => {
6+
throw new Error('Second Error');
7+
});
8+
9+
document.getElementById('click').addEventListener('click', () => {
10+
// Just a click for interaction
11+
});

‎dev-packages/browser-integration-tests/suites/replay/bufferStalledRequests/template.html‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<body>
7+
<button id="error1">Throw First Error</button>
8+
<button id="error2">Throw Second Error</button>
9+
<button id="click">Click me</button>
10+
</body>
11+
</html>

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

Lines changed: 270 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,270 @@
1+
import { expect } from '@playwright/test';
2+
import { sentryTest } from '../../../utils/fixtures';
3+
import { envelopeRequestParser, waitForErrorRequest } from '../../../utils/helpers';
4+
import {
5+
getReplaySnapshot,
6+
isReplayEvent,
7+
shouldSkipReplayTest,
8+
waitForReplayRunning,
9+
} from '../../../utils/replayHelpers';
10+
11+
sentryTest(
12+
'buffer mode remains after interrupting error event ingest',
13+
async ({ getLocalTestUrl, page, browserName }) => {
14+
if (shouldSkipReplayTest() || browserName === 'webkit') {
15+
sentryTest.skip();
16+
}
17+
18+
let errorCount = 0;
19+
let replayCount = 0;
20+
const errorEventIds: string[] = [];
21+
const replayIds: string[] = [];
22+
let firstReplayEventResolved: (value?: unknown) => void = () => {};
23+
// Need TS 5.7 for withResolvers
24+
const firstReplayEventPromise = new Promise(resolve => {
25+
firstReplayEventResolved = resolve;
26+
});
27+
28+
const url = await getLocalTestUrl({ testDir: __dirname, skipDsnRouteHandler: true });
29+
30+
await page.route('https://dsn.ingest.sentry.io/**/*', async route => {
31+
const event = envelopeRequestParser(route.request());
32+
33+
// Track error events
34+
if (event && !event.type && event.event_id) {
35+
errorCount++;
36+
errorEventIds.push(event.event_id);
37+
if (event.tags?.replayId) {
38+
replayIds.push(event.tags.replayId as string);
39+
40+
if (errorCount === 1) {
41+
firstReplayEventResolved();
42+
// intentional so that it never resolves, we'll force a reload instead to interrupt the normal flow
43+
await new Promise(resolve => setTimeout(resolve, 100000));
44+
}
45+
}
46+
}
47+
48+
// Track replay events and simulate failure for the first replay
49+
if (event && isReplayEvent(event)) {
50+
replayCount++;
51+
}
52+
53+
// Success for other requests
54+
return route.fulfill({
55+
status: 200,
56+
contentType: 'application/json',
57+
body: JSON.stringify({ id: 'test-id' }),
58+
});
59+
});
60+
61+
await page.goto(url);
62+
63+
// Wait for replay to initialize
64+
await waitForReplayRunning(page);
65+
66+
waitForErrorRequest(page);
67+
await page.locator('#error1').click();
68+
69+
// This resolves, but the route doesn't get fulfilled as we want the reload to "interrupt" this flow
70+
await firstReplayEventPromise;
71+
expect(errorCount).toBe(1);
72+
expect(replayCount).toBe(0);
73+
expect(replayIds).toHaveLength(1);
74+
75+
const firstSession = await getReplaySnapshot(page);
76+
const firstSessionId = firstSession.session?.id;
77+
expect(firstSessionId).toBeDefined();
78+
expect(firstSession.session?.sampled).toBe('buffer');
79+
expect(firstSession.session?.dirty).toBe(true);
80+
expect(firstSession.recordingMode).toBe('buffer');
81+
82+
await page.reload();
83+
const secondSession = await getReplaySnapshot(page);
84+
expect(secondSession.session?.sampled).toBe('buffer');
85+
expect(secondSession.session?.dirty).toBe(true);
86+
expect(secondSession.recordingMode).toBe('buffer');
87+
expect(secondSession.session?.id).toBe(firstSessionId);
88+
expect(secondSession.session?.segmentId).toBe(0);
89+
},
90+
);
91+
92+
sentryTest('buffer mode remains after interrupting replay flush', async ({ getLocalTestUrl, page, browserName }) => {
93+
if (shouldSkipReplayTest() || browserName === 'webkit') {
94+
sentryTest.skip();
95+
}
96+
97+
let errorCount = 0;
98+
let replayCount = 0;
99+
const errorEventIds: string[] = [];
100+
const replayIds: string[] = [];
101+
let firstReplayEventResolved: (value?: unknown) => void = () => {};
102+
// Need TS 5.7 for withResolvers
103+
const firstReplayEventPromise = new Promise(resolve => {
104+
firstReplayEventResolved = resolve;
105+
});
106+
107+
const url = await getLocalTestUrl({ testDir: __dirname, skipDsnRouteHandler: true });
108+
109+
await page.route('https://dsn.ingest.sentry.io/**/*', async route => {
110+
const event = envelopeRequestParser(route.request());
111+
112+
// Track error events
113+
if (event && !event.type && event.event_id) {
114+
errorCount++;
115+
errorEventIds.push(event.event_id);
116+
if (event.tags?.replayId) {
117+
replayIds.push(event.tags.replayId as string);
118+
}
119+
}
120+
121+
// Track replay events and simulate failure for the first replay
122+
if (event && isReplayEvent(event)) {
123+
replayCount++;
124+
if (replayCount === 1) {
125+
firstReplayEventResolved();
126+
// intentional so that it never resolves, we'll force a reload instead to interrupt the normal flow
127+
await new Promise(resolve => setTimeout(resolve, 100000));
128+
}
129+
}
130+
131+
// Success for other requests
132+
return route.fulfill({
133+
status: 200,
134+
contentType: 'application/json',
135+
body: JSON.stringify({ id: 'test-id' }),
136+
});
137+
});
138+
139+
await page.goto(url);
140+
141+
// Wait for replay to initialize
142+
await waitForReplayRunning(page);
143+
144+
await page.locator('#error1').click();
145+
await firstReplayEventPromise;
146+
expect(errorCount).toBe(1);
147+
expect(replayCount).toBe(1);
148+
expect(replayIds).toHaveLength(1);
149+
150+
// Get the first session info
151+
const firstSession = await getReplaySnapshot(page);
152+
const firstSessionId = firstSession.session?.id;
153+
expect(firstSessionId).toBeDefined();
154+
expect(firstSession.session?.sampled).toBe('buffer');
155+
expect(firstSession.session?.dirty).toBe(true);
156+
expect(firstSession.recordingMode).toBe('buffer'); // But still in buffer mode
157+
158+
await page.reload();
159+
await waitForReplayRunning(page);
160+
const secondSession = await getReplaySnapshot(page);
161+
expect(secondSession.session?.sampled).toBe('buffer');
162+
expect(secondSession.session?.dirty).toBe(true);
163+
expect(secondSession.session?.id).toBe(firstSessionId);
164+
expect(secondSession.session?.segmentId).toBe(1);
165+
// Because a flush attempt was made and not allowed to complete, segmentId increased from 0,
166+
// so we resume in session mode
167+
expect(secondSession.recordingMode).toBe('session');
168+
});
169+
170+
sentryTest(
171+
'starts a new session after interrupting replay flush and session "expires"',
172+
async ({ getLocalTestUrl, page, browserName }) => {
173+
if (shouldSkipReplayTest() || browserName === 'webkit') {
174+
sentryTest.skip();
175+
}
176+
177+
let errorCount = 0;
178+
let replayCount = 0;
179+
const errorEventIds: string[] = [];
180+
const replayIds: string[] = [];
181+
let firstReplayEventResolved: (value?: unknown) => void = () => {};
182+
// Need TS 5.7 for withResolvers
183+
const firstReplayEventPromise = new Promise(resolve => {
184+
firstReplayEventResolved = resolve;
185+
});
186+
187+
const url = await getLocalTestUrl({ testDir: __dirname, skipDsnRouteHandler: true });
188+
189+
await page.route('https://dsn.ingest.sentry.io/**/*', async route => {
190+
const event = envelopeRequestParser(route.request());
191+
192+
// Track error events
193+
if (event && !event.type && event.event_id) {
194+
errorCount++;
195+
errorEventIds.push(event.event_id);
196+
if (event.tags?.replayId) {
197+
replayIds.push(event.tags.replayId as string);
198+
}
199+
}
200+
201+
// Track replay events and simulate failure for the first replay
202+
if (event && isReplayEvent(event)) {
203+
replayCount++;
204+
if (replayCount === 1) {
205+
firstReplayEventResolved();
206+
// intentional so that it never resolves, we'll force a reload instead to interrupt the normal flow
207+
await new Promise(resolve => setTimeout(resolve, 100000));
208+
}
209+
}
210+
211+
// Success for other requests
212+
return route.fulfill({
213+
status: 200,
214+
contentType: 'application/json',
215+
body: JSON.stringify({ id: 'test-id' }),
216+
});
217+
});
218+
219+
await page.goto(url);
220+
221+
// Wait for replay to initialize
222+
await waitForReplayRunning(page);
223+
224+
// Trigger first error - this should change session sampled to "session"
225+
await page.locator('#error1').click();
226+
await firstReplayEventPromise;
227+
expect(errorCount).toBe(1);
228+
expect(replayCount).toBe(1);
229+
expect(replayIds).toHaveLength(1);
230+
231+
// Get the first session info
232+
const firstSession = await getReplaySnapshot(page);
233+
const firstSessionId = firstSession.session?.id;
234+
expect(firstSessionId).toBeDefined();
235+
expect(firstSession.session?.sampled).toBe('buffer');
236+
expect(firstSession.session?.dirty).toBe(true);
237+
expect(firstSession.recordingMode).toBe('buffer'); // But still in buffer mode
238+
239+
// Now expire the session by manipulating session storage
240+
// Simulate session expiry by setting lastActivity to a time in the past
241+
await page.evaluate(() => {
242+
const replayIntegration = (window as any).Replay;
243+
const replay = replayIntegration['_replay'];
244+
245+
// Set session as expired (15 minutes ago)
246+
if (replay.session) {
247+
const fifteenMinutesAgo = Date.now() - 15 * 60 * 1000;
248+
replay.session.lastActivity = fifteenMinutesAgo;
249+
replay.session.started = fifteenMinutesAgo;
250+
251+
// Also update session storage if sticky sessions are enabled
252+
const sessionKey = 'sentryReplaySession';
253+
const sessionData = sessionStorage.getItem(sessionKey);
254+
if (sessionData) {
255+
const session = JSON.parse(sessionData);
256+
session.lastActivity = fifteenMinutesAgo;
257+
session.started = fifteenMinutesAgo;
258+
sessionStorage.setItem(sessionKey, JSON.stringify(session));
259+
}
260+
}
261+
});
262+
263+
await page.reload();
264+
const secondSession = await getReplaySnapshot(page);
265+
expect(secondSession.session?.sampled).toBe('buffer');
266+
expect(secondSession.recordingMode).toBe('buffer');
267+
expect(secondSession.session?.id).not.toBe(firstSessionId);
268+
expect(secondSession.session?.segmentId).toBe(0);
269+
},
270+
);

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { Event, EventHint } from '@sentry/core';
22
import { DEBUG_BUILD } from '../debug-build';
3+
import { saveSession } from '../session/saveSession';
34
import type { ReplayContainer } from '../types';
45
import { isErrorEvent, isFeedbackEvent, isReplayEvent, isTransactionEvent } from '../util/eventUtils';
56
import { isRrwebError } from '../util/isRrwebError';
@@ -69,6 +70,20 @@ export function handleGlobalEventListener(replay: ReplayContainer): (event: Even
6970
event.tags = { ...event.tags, replayId: replay.getSessionId() };
7071
}
7172

73+
// If we sampled this error in buffer mode, immediately mark the session as "sampled"
74+
// by changing the sampled state from 'buffer' to 'session'. Otherwise, if the application is interrupte
75+
// before `afterSendEvent` occurs, then the session would remain as "buffer" but we have an error event
76+
// that is tagged with a replay id. This could end up creating replays w/ excessive durations because
77+
// of the linked error.
78+
if (isErrorEventSampled && replay.recordingMode === 'buffer' && replay.session?.sampled === 'buffer') {
79+
const session = replay.session;
80+
session.dirty = true;
81+
// Save the session if sticky sessions are enabled to persist the state change
82+
if (replay.getOptions().stickySession) {
83+
saveSession(session);
84+
}
85+
}
86+
7287
return event;
7388
},
7489
{ id: 'Replay' },

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -606,6 +606,7 @@ export class ReplayContainer implements ReplayContainerInterface {
606606

607607
// Once this session ends, we do not want to refresh it
608608
if (this.session) {
609+
this.session.dirty = false;
609610
this._updateUserActivity(activityTime);
610611
this._updateSessionActivity(activityTime);
611612
this._maybeSaveSession();

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
1313
const segmentId = session.segmentId || 0;
1414
const sampled = session.sampled;
1515
const previousSessionId = session.previousSessionId;
16+
const dirty = session.dirty || false;
1617

1718
return {
1819
id,
@@ -21,5 +22,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
2122
segmentId,
2223
sampled,
2324
previousSessionId,
25+
dirty,
2426
};
2527
}

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,13 @@ export interface Session {
383383
* Is the session sampled? `false` if not sampled, otherwise, `session` or `buffer`
384384
*/
385385
sampled: Sampled;
386+
387+
/**
388+
* Session is dirty when its id has been linked to an event (e.g. error event).
389+
* This is helpful when a session is mistakenly stuck in "buffer" mode (e.g. network issues preventing it from being converted to "session" mode).
390+
* The dirty flag is used to prevent updating the session start time to the earliest event in the buffer so that it can be refreshed if it's been expired.
391+
*/
392+
dirty?: boolean;
386393
}
387394

388395
export type EventBufferType = 'sync' | 'worker';

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ export function getHandleRecordingEmit(replay: ReplayContainer): RecordingEmitCa
7272

7373
// When in buffer mode, make sure we adjust the session started date to the current earliest event of the buffer
7474
// this should usually be the timestamp of the checkout event, but to be safe...
75-
if (replay.recordingMode === 'buffer' && session && replay.eventBuffer) {
75+
if (replay.recordingMode === 'buffer' && session && replay.eventBuffer && !session.dirty) {
7676
const earliestEvent = replay.eventBuffer.getEarliestTimestamp();
7777
if (earliestEvent) {
7878
DEBUG_BUILD &&

0 commit comments

Comments
 (0)