fix: Do refresh buffered sessions (#7931)

Author: mydea

packages/browser-integration-tests/suites/replay/bufferMode/test.ts

@@ -245,7 +245,7 @@ sentryTest(
 
     await page.evaluate(async () => {
       const replayIntegration = (window as unknown as Window & { Replay: Replay }).Replay;
-      await replayIntegration.flush({continueRecording: false});
+      await replayIntegration.flush({ continueRecording: false });
     });
 
     const req0 = await reqPromise0;

packages/replay/src/replay.ts

@@ -394,6 +394,12 @@ export class ReplayContainer implements ReplayContainerInterface {
       // Reset all "capture on error" configuration before
       // starting a new recording
       this.recordingMode = 'session';
+
+      // Once this session ends, we do not want to refresh it
+      if (this.session) {
+        this.session.shouldRefresh = false;
+        this._maybeSaveSession();
+      }
       this.startRecording();
     }
   }

packages/replay/src/session/Session.ts

@@ -21,6 +21,7 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
     lastActivity,
     segmentId,
     sampled,
+    shouldRefresh: true,
   };
 }
 

packages/replay/src/session/getSession.ts

@@ -36,8 +36,9 @@ export function getSession({
 
     if (!isExpired) {
       return { type: 'saved', session };
-    } else if (session.sampled === 'buffer') {
-      // Buffered samples should not be re-created when expired, but instead we stop when the replay is done
+    } else if (!session.shouldRefresh) {
+      // In this case, stop
+      // This is the case if we have an error session that is completed (=triggered an error)
       const discardedSession = makeSession({ sampled: false });
       return { type: 'new', session: discardedSession };
     } else {

packages/replay/src/types.ts

@@ -395,6 +395,12 @@ export interface Session {
    * Is the session sampled? `false` if not sampled, otherwise, `session` or `buffer`
    */
   sampled: Sampled;
+
+  /**
+   * If this is false, the session should not be refreshed when it was inactive.
+   * This can be the case if you had a buffered session which is now recording because an error happened.
+   */
+  shouldRefresh: boolean;
 }
 
 export interface EventBuffer {

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

@@ -279,62 +279,123 @@ describe('Integration | errorSampleRate', () => {
   // sample rate of 0.0), or an error session that has no errors. Instead we
   // simply stop the session replay completely and wait for a new page load to
   // resample.
-  it('stops replay if session exceeds MAX_SESSION_LIFE and does not start a new session thereafter', async () => {
-    // Idle for 15 minutes
-    jest.advanceTimersByTime(MAX_SESSION_LIFE + 1);
+  it.each([
+    ['MAX_SESSION_LIFE', MAX_SESSION_LIFE],
+    ['SESSION_IDLE_DURATION', SESSION_IDLE_DURATION],
+  ])(
+    'stops replay if session had an error and exceeds %s and does not start a new session thereafter',
+    async (_label, waitTime) => {
+      expect(replay.session?.shouldRefresh).toBe(true);
+
+      captureException(new Error('testing'));
+
+      await new Promise(process.nextTick);
+      jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
+      await new Promise(process.nextTick);
+
+      // segment_id is 1 because it sends twice on error
+      expect(replay).toHaveLastSentReplay({
+        recordingPayloadHeader: { segment_id: 1 },
+        replayEventPayload: expect.objectContaining({
+          replay_type: 'buffer',
+        }),
+      });
+      expect(replay.session?.shouldRefresh).toBe(false);
+
+      // Idle for given time
+      jest.advanceTimersByTime(waitTime + 1);
+      await new Promise(process.nextTick);
+
+      const TEST_EVENT = {
+        data: { name: 'lost event' },
+        timestamp: BASE_TIMESTAMP,
+        type: 3,
+      };
+      mockRecord._emitter(TEST_EVENT);
 
-    const TEST_EVENT = {
-      data: { name: 'lost event' },
-      timestamp: BASE_TIMESTAMP,
-      type: 3,
-    };
-    mockRecord._emitter(TEST_EVENT);
-    expect(replay).not.toHaveLastSentReplay();
+      jest.runAllTimers();
+      await new Promise(process.nextTick);
 
-    jest.runAllTimers();
-    await new Promise(process.nextTick);
+      // We stop recording after 15 minutes of inactivity in error mode
 
-    // We stop recording after 15 minutes of inactivity in error mode
+      // still no new replay sent
+      expect(replay).toHaveLastSentReplay({
+        recordingPayloadHeader: { segment_id: 1 },
+        replayEventPayload: expect.objectContaining({
+          replay_type: 'buffer',
+        }),
+      });
 
-    expect(replay).not.toHaveLastSentReplay();
-    expect(replay.isEnabled()).toBe(false);
-    expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled();
+      expect(replay.isEnabled()).toBe(false);
 
-    domHandler({
-      name: 'click',
-    });
+      domHandler({
+        name: 'click',
+      });
 
-    // Remains disabled!
-    expect(replay.isEnabled()).toBe(false);
-  });
+      // Remains disabled!
+      expect(replay.isEnabled()).toBe(false);
+    },
+  );
 
-  // Should behave the same as above test
-  it('stops replay if user has been idle for more than SESSION_IDLE_DURATION and does not start a new session thereafter', async () => {
-    // Idle for 15 minutes
-    jest.advanceTimersByTime(SESSION_IDLE_DURATION + 1);
+  it.each([
+    ['MAX_SESSION_LIFE', MAX_SESSION_LIFE],
+    ['SESSION_IDLE_DURATION', SESSION_IDLE_DURATION],
+  ])('continues buffering replay if session had no error and exceeds %s', async (_label, waitTime) => {
+    expect(replay).not.toHaveLastSentReplay();
+
+    // Idle for given time
+    jest.advanceTimersByTime(waitTime + 1);
+    await new Promise(process.nextTick);
 
     const TEST_EVENT = {
       data: { name: 'lost event' },
       timestamp: BASE_TIMESTAMP,
       type: 3,
     };
     mockRecord._emitter(TEST_EVENT);
-    expect(replay).not.toHaveLastSentReplay();
 
     jest.runAllTimers();
     await new Promise(process.nextTick);
 
-    // We stop recording after SESSION_IDLE_DURATION of inactivity in error mode
+    // still no new replay sent
     expect(replay).not.toHaveLastSentReplay();
-    expect(replay.isEnabled()).toBe(false);
-    expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled();
+
+    expect(replay.isEnabled()).toBe(true);
+    expect(replay.isPaused()).toBe(false);
+    expect(replay.recordingMode).toBe('buffer');
 
     domHandler({
       name: 'click',
     });
 
-    // Remains disabled!
-    expect(replay.isEnabled()).toBe(false);
+    await new Promise(process.nextTick);
+    jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
+    await new Promise(process.nextTick);
+
+    expect(replay).not.toHaveLastSentReplay();
+    expect(replay.isEnabled()).toBe(true);
+    expect(replay.isPaused()).toBe(false);
+    expect(replay.recordingMode).toBe('buffer');
+
+    // should still react to errors later on
+    captureException(new Error('testing'));
+
+    await new Promise(process.nextTick);
+    jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
+    await new Promise(process.nextTick);
+
+    expect(replay).toHaveLastSentReplay({
+      recordingPayloadHeader: { segment_id: 0 },
+      replayEventPayload: expect.objectContaining({
+        replay_type: 'buffer',
+      }),
+    });
+
+    expect(replay.isEnabled()).toBe(true);
+    expect(replay.isPaused()).toBe(false);
+    expect(replay.recordingMode).toBe('session');
+    expect(replay.session?.sampled).toBe('buffer');
+    expect(replay.session?.shouldRefresh).toBe(false);
   });
 
   it('has the correct timestamps with deferred root event and last replay update', async () => {

packages/replay/test/unit/session/fetchSession.test.ts

@@ -28,6 +28,7 @@ describe('Unit | session | fetchSession', () => {
       segmentId: 0,
       sampled: 'session',
       started: 1648827162630,
+      shouldRefresh: true,
     });
   });
 
@@ -43,6 +44,7 @@ describe('Unit | session | fetchSession', () => {
       segmentId: 0,
       sampled: false,
       started: 1648827162630,
+      shouldRefresh: true,
     });
   });
 

packages/replay/test/unit/session/getSession.test.ts

@@ -24,6 +24,7 @@ function createMockSession(when: number = Date.now()) {
     lastActivity: when,
     started: when,
     sampled: 'session',
+    shouldRefresh: true,
   });
 }
 
@@ -59,6 +60,7 @@ describe('Unit | session | getSession', () => {
       lastActivity: expect.any(Number),
       sampled: 'session',
       started: expect.any(Number),
+      shouldRefresh: true,
     });
 
     // Should not have anything in storage
@@ -129,6 +131,7 @@ describe('Unit | session | getSession', () => {
       lastActivity: expect.any(Number),
       sampled: 'session',
       started: expect.any(Number),
+      shouldRefresh: true,
     });
 
     // Should not have anything in storage
@@ -138,6 +141,7 @@ describe('Unit | session | getSession', () => {
       lastActivity: expect.any(Number),
       sampled: 'session',
       started: expect.any(Number),
+      shouldRefresh: true,
     });
   });
 
@@ -164,6 +168,7 @@ describe('Unit | session | getSession', () => {
       lastActivity: now,
       sampled: 'session',
       started: now,
+      shouldRefresh: true,
     });
   });