fix(replay): Ignore max session life for buffered sessions (#8258)

Theres an edge case where a buffered session becomes expired, an error
comes in, the link from error to replay is lost because a new session is
created due to the session being expired.

We should either ignore the max session life for a buffered session, or
possibly check/refresh session when an error comes in.

Author: billyvg

packages/replay/src/replay.ts

@@ -403,6 +403,8 @@ export class ReplayContainer implements ReplayContainerInterface {
       return this.flushImmediate();
     }
 
+    const activityTime = Date.now();
+
     // Allow flush to complete before resuming as a session recording, otherwise
     // the checkout from `startRecording` may be included in the payload.
     // Prefer to keep the error replay as a separate (and smaller) segment
@@ -424,6 +426,18 @@ export class ReplayContainer implements ReplayContainerInterface {
     // Once this session ends, we do not want to refresh it
     if (this.session) {
       this.session.shouldRefresh = false;
+
+      // It's possible that the session lifespan is > max session lifespan
+      // because we have been buffering beyond max session lifespan (we ignore
+      // expiration given that `shouldRefresh` is true). Since we flip
+      // `shouldRefresh`, the session could be considered expired due to
+      // lifespan, which is not what we want. Update session start date to be
+      // the current timestamp, so that session is not considered to be
+      // expired. This means that max replay duration can be MAX_SESSION_LIFE +
+      // (length of buffer), which we are ok with.
+      this._updateUserActivity(activityTime);
+      this._updateSessionActivity(activityTime);
+      this.session.started = activityTime;
       this._maybeSaveSession();
     }
 
@@ -689,7 +703,7 @@ export class ReplayContainer implements ReplayContainerInterface {
       stickySession: Boolean(this._options.stickySession),
       currentSession: this.session,
       sessionSampleRate: this._options.sessionSampleRate,
-      allowBuffering: this._options.errorSampleRate > 0,
+      allowBuffering: this._options.errorSampleRate > 0 || this.recordingMode === 'buffer',
     });
 
     // If session was newly created (i.e. was not loaded from storage), then

packages/replay/src/session/getSession.ts

@@ -34,11 +34,13 @@ export function getSession({
     // within "max session time").
     const isExpired = isSessionExpired(session, timeouts);
 
-    if (!isExpired) {
+    if (!isExpired || (allowBuffering && session.shouldRefresh)) {
       return { type: 'saved', session };
     } else if (!session.shouldRefresh) {
-      // In this case, stop
-      // This is the case if we have an error session that is completed (=triggered an error)
+      // This is the case if we have an error session that is completed
+      // (=triggered an error). Session will continue as session-based replay,
+      // and when this session is expired, it will not be renewed until user
+      // reloads.
       const discardedSession = makeSession({ sampled: false });
       return { type: 'new', session: discardedSession };
     } else {

packages/replay/src/util/handleRecordingEmit.ts

@@ -89,6 +89,10 @@ export function getHandleRecordingEmit(replay: ReplayContainer): RecordingEmitCa
         // a previous session ID. In this case, we want to buffer events
         // for a set amount of time before flushing. This can help avoid
         // capturing replays of users that immediately close the window.
+        // TODO: We should check `recordingMode` here and do nothing if it's
+        // buffer, instead of checking inside of timeout, this will make our
+        // tests a bit cleaner as we will need to wait on the delay in order to
+        // do nothing.
         setTimeout(() => replay.conditionalFlush(), options._experiments.delayFlushOnCheckout);
 
         // Cancel any previously debounced flushes to ensure there are no [near]

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

@@ -573,6 +573,7 @@ describe('Integration | errorSampleRate with delayed flush', () => {
 
   it('has correct timestamps when error occurs much later than initial pageload/checkout', async () => {
     const ELAPSED = BUFFER_CHECKOUT_TIME;
+    const TICK = 20;
     const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
     mockRecord._emitter(TEST_EVENT);
 
@@ -593,26 +594,29 @@ describe('Integration | errorSampleRate with delayed flush', () => {
     const optionsEvent = createOptionsEvent(replay);
 
     jest.runAllTimers();
-    jest.advanceTimersByTime(20);
     await new Promise(process.nextTick);
 
+    expect(replay).not.toHaveLastSentReplay();
+
     captureException(new Error('testing'));
 
     await waitForBufferFlush();
 
-    expect(replay.session?.started).toBe(BASE_TIMESTAMP + ELAPSED + 20);
+    // See comments in `handleRecordingEmit.ts`, we perform a setTimeout into a
+    // noop when it can be skipped altogether
+    expect(replay.session?.started).toBe(BASE_TIMESTAMP + ELAPSED + DEFAULT_FLUSH_MIN_DELAY + TICK + TICK);
 
     // Does not capture mouse click
     expect(replay).toHaveSentReplay({
       recordingPayloadHeader: { segment_id: 0 },
       replayEventPayload: expect.objectContaining({
         // Make sure the old performance event is thrown out
-        replay_start_timestamp: (BASE_TIMESTAMP + ELAPSED + 20) / 1000,
+        replay_start_timestamp: (BASE_TIMESTAMP + ELAPSED + TICK) / 1000,
       }),
       recordingData: JSON.stringify([
         {
           data: { isCheckout: true },
-          timestamp: BASE_TIMESTAMP + ELAPSED + 20,
+          timestamp: BASE_TIMESTAMP + ELAPSED + TICK,
           type: 2,
         },
         optionsEvent,
@@ -662,7 +666,8 @@ describe('Integration | errorSampleRate with delayed flush', () => {
     expect(replay.isEnabled()).toBe(false);
   });
 
-  it('stops replay when session exceeds max length', async () => {
+  it('stops replay when session exceeds max length after latest captured error', async () => {
+    const sessionId = replay.session?.id;
     jest.setSystemTime(BASE_TIMESTAMP);
 
     const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
@@ -674,34 +679,120 @@ describe('Integration | errorSampleRate with delayed flush', () => {
     jest.runAllTimers();
     await new Promise(process.nextTick);
 
+    jest.advanceTimersByTime(2 * MAX_SESSION_LIFE);
+
     captureException(new Error('testing'));
 
-    jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
+    // Flush due to exception
+    await new Promise(process.nextTick);
+    await waitForFlush();
+
+    expect(replay.session?.id).toBe(sessionId);
+    expect(replay).toHaveLastSentReplay({
+      recordingPayloadHeader: { segment_id: 0 },
+    });
+
+    // This comes from `startRecording()` in `sendBufferedReplayOrFlush()`
+    await waitForFlush();
+    expect(replay).toHaveLastSentReplay({
+      recordingPayloadHeader: { segment_id: 1 },
+      recordingData: JSON.stringify([
+        {
+          data: {
+            isCheckout: true,
+          },
+          timestamp: BASE_TIMESTAMP + 2 * MAX_SESSION_LIFE + DEFAULT_FLUSH_MIN_DELAY + 40,
+          type: 2,
+        },
+      ]),
+    });
+
+    // Now wait after session expires - should stop recording
+    mockRecord.takeFullSnapshot.mockClear();
+    (getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance<any>).mockClear();
+
+    jest.advanceTimersByTime(MAX_SESSION_LIFE);
+    await new Promise(process.nextTick);
+
+    mockRecord._emitter(TEST_EVENT);
+    jest.runAllTimers();
     await new Promise(process.nextTick);
 
     expect(replay).not.toHaveLastSentReplay();
+    expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0);
+    expect(replay.isEnabled()).toBe(false);
+
+    // Once the session is stopped after capturing a replay already
+    // (buffer-mode), another error will not trigger a new replay
+    captureException(new Error('testing'));
 
-    // Wait a bit, shortly before session expires
-    jest.advanceTimersByTime(MAX_SESSION_LIFE - 1000);
     await new Promise(process.nextTick);
+    jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
+    await new Promise(process.nextTick);
+    expect(replay).not.toHaveLastSentReplay();
+  });
 
+  it('does not stop replay based on earliest event in buffer', async () => {
+    jest.setSystemTime(BASE_TIMESTAMP);
+
+    const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP - 60000, type: 3 };
     mockRecord._emitter(TEST_EVENT);
-    replay.triggerUserActivity();
+
+    expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled();
+    expect(replay).not.toHaveLastSentReplay();
+
+    jest.runAllTimers();
+    await new Promise(process.nextTick);
+
+    expect(replay).not.toHaveLastSentReplay();
+    captureException(new Error('testing'));
+
+    await waitForBufferFlush();
 
     expect(replay).toHaveLastSentReplay();
 
+    // Flush from calling `stopRecording`
+    await waitForFlush();
+
     // 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);
+    expect(replay).not.toHaveLastSentReplay();
+
+    const TICKS = 80;
+
+    // We advance time so that we are on the border of expiring, taking into
+    // account that TEST_EVENT timestamp is 60000 ms before BASE_TIMESTAMP. The
+    // 3 DEFAULT_FLUSH_MIN_DELAY is to account for the `waitForFlush` that has
+    // happened, and for the next two that will happen. The first following
+    // `waitForFlush` does not expire session, but the following one will.
+    jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION - 60000 - 3 * DEFAULT_FLUSH_MIN_DELAY - TICKS);
     await new Promise(process.nextTick);
 
     mockRecord._emitter(TEST_EVENT);
-    replay.triggerUserActivity();
+    expect(replay).not.toHaveLastSentReplay();
+    await waitForFlush();
 
-    jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
-    await new Promise(process.nextTick);
+    expect(replay).not.toHaveLastSentReplay();
+    expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0);
+    expect(replay.isEnabled()).toBe(true);
+
+    mockRecord._emitter(TEST_EVENT);
+    expect(replay).not.toHaveLastSentReplay();
+    await waitForFlush();
+
+    expect(replay).not.toHaveLastSentReplay();
+    expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0);
+    expect(replay.isEnabled()).toBe(true);
+
+    // It's hard to test, but if we advance the below time less 1 ms, it should
+    // be enabled, but we can't trigger a session check via flush without
+    // incurring another DEFAULT_FLUSH_MIN_DELAY timeout.
+    jest.advanceTimersByTime(60000 - DEFAULT_FLUSH_MIN_DELAY);
+    mockRecord._emitter(TEST_EVENT);
+    expect(replay).not.toHaveLastSentReplay();
+    await waitForFlush();
 
     expect(replay).not.toHaveLastSentReplay();
     expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0);