fix(replay): Do not renew session in error mode (#6948)

Author: mydea

packages/replay/src/replay.ts

@@ -155,7 +155,9 @@ export class ReplayContainer implements ReplayContainerInterface {
   public start(): void {
     this._setInitialState();
 
-    this._loadSession({ expiry: SESSION_IDLE_DURATION });
+    if (!this._loadAndCheckSession()) {
+      return;
+    }
 
     // If there is no session, then something bad has happened - can't continue
     if (!this.session) {
@@ -234,6 +236,10 @@ export class ReplayContainer implements ReplayContainerInterface {
    * does not support a teardown
    */
   public stop(): void {
+    if (!this._isEnabled) {
+      return;
+    }
+
     try {
       __DEBUG_BUILD__ && logger.log('[Replay] Stopping Replays');
       this._isEnabled = false;
@@ -264,6 +270,10 @@ export class ReplayContainer implements ReplayContainerInterface {
    * new DOM checkout.`
    */
   public resume(): void {
+    if (!this._loadAndCheckSession()) {
+      return;
+    }
+
     this._isPaused = false;
     this.startRecording();
   }
@@ -310,7 +320,9 @@ export class ReplayContainer implements ReplayContainerInterface {
     if (!this._stopRecording) {
       // Create a new session, otherwise when the user action is flushed, it
       // will get rejected due to an expired session.
-      this._loadSession({ expiry: SESSION_IDLE_DURATION });
+      if (!this._loadAndCheckSession()) {
+        return;
+      }
 
       // Note: This will cause a new DOM checkout
       this.resume();
@@ -348,7 +360,7 @@ export class ReplayContainer implements ReplayContainerInterface {
    * Returns true if session is not expired, false otherwise.
    * @hidden
    */
-  public checkAndHandleExpiredSession({ expiry = SESSION_IDLE_DURATION }: { expiry?: number } = {}): boolean | void {
+  public checkAndHandleExpiredSession(expiry?: number): boolean | void {
     const oldSessionId = this.getSessionId();
 
     // Prevent starting a new session if the last user activity is older than
@@ -363,7 +375,9 @@ export class ReplayContainer implements ReplayContainerInterface {
 
     // --- There is recent user activity --- //
     // This will create a new session if expired, based on expiry length
-    this._loadSession({ expiry });
+    if (!this._loadAndCheckSession(expiry)) {
+      return;
+    }
 
     // Session was expired if session ids do not match
     const expired = oldSessionId !== this.getSessionId();
@@ -388,10 +402,10 @@ export class ReplayContainer implements ReplayContainerInterface {
   }
 
   /**
-   * Loads a session from storage, or creates a new one if it does not exist or
-   * is expired.
+   * Loads (or refreshes) the current session.
+   * Returns false if session is not recorded.
    */
-  private _loadSession({ expiry }: { expiry: number }): void {
+  private _loadAndCheckSession(expiry = SESSION_IDLE_DURATION): boolean {
     const { type, session } = getSession({
       expiry,
       stickySession: Boolean(this._options.stickySession),
@@ -412,6 +426,13 @@ export class ReplayContainer implements ReplayContainerInterface {
     }
 
     this.session = session;
+
+    if (!this.session.sampled) {
+      this.stop();
+      return false;
+    }
+
+    return true;
   }
 
   /**
@@ -618,9 +639,7 @@ export class ReplayContainer implements ReplayContainerInterface {
       return;
     }
 
-    const isSessionActive = this.checkAndHandleExpiredSession({
-      expiry: VISIBILITY_CHANGE_TIMEOUT,
-    });
+    const isSessionActive = this.checkAndHandleExpiredSession(VISIBILITY_CHANGE_TIMEOUT);
 
     if (!isSessionActive) {
       // If the user has come back to the page within VISIBILITY_CHANGE_TIMEOUT

packages/replay/src/session/getSession.ts

@@ -4,6 +4,7 @@ import type { Session, SessionOptions } from '../types';
 import { isSessionExpired } from '../util/isSessionExpired';
 import { createSession } from './createSession';
 import { fetchSession } from './fetchSession';
+import { makeSession } from './Session';
 
 interface GetSessionParams extends SessionOptions {
   /**
@@ -38,6 +39,10 @@ export function getSession({
 
     if (!isExpired) {
       return { type: 'saved', session };
+    } else if (session.sampled === 'error') {
+      // Error samples should not be re-created when expired, but instead we stop when the replay is done
+      const discardedSession = makeSession({ sampled: false });
+      return { type: 'new', session: discardedSession };
     } else {
       __DEBUG_BUILD__ && logger.log('[Replay] Session has expired');
     }

packages/replay/test/integration/errorSampleRate.test.ts

@@ -1,8 +1,9 @@
-import { captureException } from '@sentry/core';
+import { captureException, getCurrentHub } from '@sentry/core';
 
 import {
   DEFAULT_FLUSH_MIN_DELAY,
   ERROR_CHECKOUT_TIME,
+  MAX_SESSION_LIFE,
   REPLAY_SESSION_KEY,
   VISIBILITY_CHANGE_TIMEOUT,
   WINDOW,
@@ -264,12 +265,10 @@ describe('Integration | errorSampleRate', () => {
     expect(replay).not.toHaveLastSentReplay();
   });
 
-  it('does not upload if user has been idle for more than 15 minutes and comes back to move their mouse', async () => {
+  it('stops replay if user has been idle for more than 15 minutes and comes back to move their mouse', async () => {
     // Idle for 15 minutes
     jest.advanceTimersByTime(15 * 60000);
 
-    // TBD: We are currently deciding that this event will get dropped, but
-    // this could/should change in the future.
     const TEST_EVENT = {
       data: { name: 'lost event' },
       timestamp: BASE_TIMESTAMP,
@@ -281,15 +280,11 @@ describe('Integration | errorSampleRate', () => {
     jest.runAllTimers();
     await new Promise(process.nextTick);
 
-    // Instead of recording the above event, a full snapshot will occur.
-    //
-    // TODO: We could potentially figure out a way to save the last session,
-    // and produce a checkout based on a previous checkout + updates, and then
-    // replay the event on top. Or maybe replay the event on top of a refresh
-    // snapshot.
+    // We stop recording after 15 minutes of inactivity in error mode
 
     expect(replay).not.toHaveLastSentReplay();
-    expect(mockRecord.takeFullSnapshot).toHaveBeenCalledWith(true);
+    expect(replay.isEnabled()).toBe(false);
+    expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled();
   });
 
   it('has the correct timestamps with deferred root event and last replay update', async () => {
@@ -375,6 +370,52 @@ describe('Integration | errorSampleRate', () => {
       ]),
     });
   });
+
+  it('stops replay when session expires', async () => {
+    jest.setSystemTime(BASE_TIMESTAMP);
+
+    const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
+    mockRecord._emitter(TEST_EVENT);
+
+    expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled();
+    expect(replay).not.toHaveLastSentReplay();
+
+    jest.runAllTimers();
+    await new Promise(process.nextTick);
+
+    captureException(new Error('testing'));
+
+    jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
+    await new Promise(process.nextTick);
+
+    expect(replay).toHaveLastSentReplay();
+
+    // Wait a bit, shortly before session expires
+    jest.advanceTimersByTime(MAX_SESSION_LIFE - 1000);
+    await new Promise(process.nextTick);
+
+    mockRecord._emitter(TEST_EVENT);
+    replay.triggerUserActivity();
+
+    expect(replay).toHaveLastSentReplay();
+
+    // Now wait after session expires - should stop recording
+    mockRecord.takeFullSnapshot.mockClear();
+    (getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance<any>).mockClear();
+
+    jest.advanceTimersByTime(10_000);
+    await new Promise(process.nextTick);
+
+    mockRecord._emitter(TEST_EVENT);
+    replay.triggerUserActivity();
+
+    jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
+    await new Promise(process.nextTick);
+
+    expect(replay).not.toHaveLastSentReplay();
+    expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0);
+    expect(replay.isEnabled()).toBe(false);
+  });
 });
 
 /**

packages/replay/test/integration/events.test.ts

@@ -40,7 +40,7 @@ describe('Integration | events', () => {
     // Create a new session and clear mocks because a segment (from initial
     // checkout) will have already been uploaded by the time the tests run
     clearSession(replay);
-    replay['_loadSession']({ expiry: 0 });
+    replay['_loadAndCheckSession'](0);
     mockTransportSend.mockClear();
   });
 
@@ -93,7 +93,7 @@ describe('Integration | events', () => {
 
   it('has correct timestamps when there are events earlier than initial timestamp', async function () {
     clearSession(replay);
-    replay['_loadSession']({ expiry: 0 });
+    replay['_loadAndCheckSession'](0);
     mockTransportSend.mockClear();
     Object.defineProperty(document, 'visibilityState', {
       configurable: true,

packages/replay/test/integration/flush.test.ts

@@ -95,7 +95,7 @@ describe('Integration | flush', () => {
     jest.setSystemTime(new Date(BASE_TIMESTAMP));
     sessionStorage.clear();
     clearSession(replay);
-    replay['_loadSession']({ expiry: SESSION_IDLE_DURATION });
+    replay['_loadAndCheckSession'](SESSION_IDLE_DURATION);
     mockRecord.takeFullSnapshot.mockClear();
     Object.defineProperty(WINDOW, 'location', {
       value: prevLocation,

packages/replay/test/integration/rateLimiting.test.ts

@@ -46,7 +46,7 @@ describe('Integration | rate-limiting behaviour', () => {
     // Create a new session and clear mocks because a segment (from initial
     // checkout) will have already been uploaded by the time the tests run
     clearSession(replay);
-    replay['_loadSession']({ expiry: 0 });
+    replay['_loadAndCheckSession'](0);
 
     mockSendReplayRequest.mockClear();
   });
@@ -57,7 +57,7 @@ describe('Integration | rate-limiting behaviour', () => {
     jest.setSystemTime(new Date(BASE_TIMESTAMP));
     clearSession(replay);
     jest.clearAllMocks();
-    replay['_loadSession']({ expiry: SESSION_IDLE_DURATION });
+    replay['_loadAndCheckSession'](SESSION_IDLE_DURATION);
   });
 
   afterAll(() => {

packages/replay/test/integration/sendReplayEvent.test.ts

@@ -59,7 +59,7 @@ describe('Integration | sendReplayEvent', () => {
     // Create a new session and clear mocks because a segment (from initial
     // checkout) will have already been uploaded by the time the tests run
     clearSession(replay);
-    replay['_loadSession']({ expiry: 0 });
+    replay['_loadAndCheckSession'](0);
 
     mockSendReplayRequest.mockClear();
   });
@@ -69,7 +69,7 @@ describe('Integration | sendReplayEvent', () => {
     await new Promise(process.nextTick);
     jest.setSystemTime(new Date(BASE_TIMESTAMP));
     clearSession(replay);
-    replay['_loadSession']({ expiry: SESSION_IDLE_DURATION });
+    replay['_loadAndCheckSession'](SESSION_IDLE_DURATION);
   });
 
   afterAll(() => {

packages/replay/test/integration/session.test.ts

@@ -451,7 +451,7 @@ describe('Integration | session', () => {
 
   it('increases segment id after each event', async () => {
     clearSession(replay);
-    replay['_loadSession']({ expiry: 0 });
+    replay['_loadAndCheckSession'](0);
 
     Object.defineProperty(document, 'visibilityState', {
       configurable: true,

packages/replay/test/integration/stop.test.ts

@@ -44,7 +44,7 @@ describe('Integration | stop', () => {
     jest.setSystemTime(new Date(BASE_TIMESTAMP));
     sessionStorage.clear();
     clearSession(replay);
-    replay['_loadSession']({ expiry: SESSION_IDLE_DURATION });
+    replay['_loadAndCheckSession'](SESSION_IDLE_DURATION);
     mockRecord.takeFullSnapshot.mockClear();
     mockAddInstrumentationHandler.mockClear();
     Object.defineProperty(WINDOW, 'location', {