Skip to content

Commit

Permalink
fix(replay): Ensure buffer sessions end after capturing an error (#8713)
Browse files Browse the repository at this point in the history
We haven't been persisting the `shouldRefresh` property of the session.
This combined with the fact that we do not update the `sampled` field on
the session to `session` when an error occurs, but keep it at `buffer`,
means that if a user reloads the page or the session is otherwise
re-fetched from sessionStorage, if previously an error occurs, we'll
keep buffering forever again (like for a "fresh" buffer session), and if
an error happens we convert it again to a `session` session, but since
the session ID was never updated this will be "added" to the previous
session instead.

I made a reproduction test that failed before and works after this fix.

Fixes #8400
  • Loading branch information
mydea committed Aug 3, 2023
1 parent 60c1081 commit 2748691
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 1 deletion.
3 changes: 2 additions & 1 deletion packages/replay/src/session/Session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
const lastActivity = session.lastActivity || now;
const segmentId = session.segmentId || 0;
const sampled = session.sampled;
const shouldRefresh = typeof session.shouldRefresh === 'boolean' ? session.shouldRefresh : true;

return {
id,
started,
lastActivity,
segmentId,
sampled,
shouldRefresh: true,
shouldRefresh,
};
}
79 changes: 79 additions & 0 deletions packages/replay/test/integration/errorSampleRate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -819,6 +819,85 @@ describe('Integration | errorSampleRate', () => {
});
});

/**
* If an error happens, we switch the recordingMode to `session`, set `shouldRefresh=false` on the session,
* but keep `sampled=buffer`.
* This test should verify that if we load such a session from sessionStorage, the session is eventually correctly ended.
*/
it('handles buffer sessions that previously had an error', async () => {
// Pretend that a session is already saved before loading replay
WINDOW.sessionStorage.setItem(
REPLAY_SESSION_KEY,
`{"segmentId":0,"id":"fd09adfc4117477abc8de643e5a5798a","sampled":"buffer","started":${BASE_TIMESTAMP},"lastActivity":${BASE_TIMESTAMP},"shouldRefresh":false}`,
);
const { mockRecord, replay, integration } = await resetSdkMock({
replayOptions: {
stickySession: true,
},
sentryOptions: {
replaysOnErrorSampleRate: 1.0,
},
autoStart: false,
});
integration['_initialize']();

jest.runAllTimers();

await new Promise(process.nextTick);
const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
mockRecord._emitter(TEST_EVENT);

expect(replay).not.toHaveLastSentReplay();

// Waiting for max life should eventually stop recording
// We simulate a full checkout which would otherwise be done automatically
for (let i = 0; i < MAX_SESSION_LIFE / 60_000; i++) {
jest.advanceTimersByTime(60_000);
await new Promise(process.nextTick);
mockRecord.takeFullSnapshot(true);
}

expect(replay).not.toHaveLastSentReplay();
expect(replay.isEnabled()).toBe(false);
});

it('handles buffer sessions that never had an error', async () => {
// Pretend that a session is already saved before loading replay
WINDOW.sessionStorage.setItem(
REPLAY_SESSION_KEY,
`{"segmentId":0,"id":"fd09adfc4117477abc8de643e5a5798a","sampled":"buffer","started":${BASE_TIMESTAMP},"lastActivity":${BASE_TIMESTAMP}}`,
);
const { mockRecord, replay, integration } = await resetSdkMock({
replayOptions: {
stickySession: true,
},
sentryOptions: {
replaysOnErrorSampleRate: 1.0,
},
autoStart: false,
});
integration['_initialize']();

jest.runAllTimers();

await new Promise(process.nextTick);
const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
mockRecord._emitter(TEST_EVENT);

expect(replay).not.toHaveLastSentReplay();

// Waiting for max life should eventually stop recording
// We simulate a full checkout which would otherwise be done automatically
for (let i = 0; i < MAX_SESSION_LIFE / 60_000; i++) {
jest.advanceTimersByTime(60_000);
await new Promise(process.nextTick);
mockRecord.takeFullSnapshot(true);
}

expect(replay).not.toHaveLastSentReplay();
expect(replay.isEnabled()).toBe(true);
});

/**
* This is testing a case that should only happen with error-only sessions.
* Previously we had assumed that loading a session from session storage meant
Expand Down

0 comments on commit 2748691

Please sign in to comment.