fix: Fix race between end of session and features aborting (#1487)

Co-authored-by: Chunwai Li <cli@newrelic.com>

Author: metal-messiah

src/common/harvest/harvester.js

@@ -8,7 +8,6 @@ import { VERSION } from '../constants/env'
 import { globalScope, isWorkerScope } from '../constants/runtime'
 import { handle } from '../event-emitter/handle'
 import { eventListenerOpts } from '../event-listener/event-listener-opts'
-import { SESSION_EVENTS } from '../session/constants'
 import { now } from '../timing/now'
 import { subscribeToEOL } from '../unload/eol'
 import { cleanURL } from '../url/clean-url'
@@ -36,10 +35,6 @@ export class Harvester {
       this.initializedAggregates.forEach(aggregateInst => this.triggerHarvestFor(aggregateInst, { isFinalHarvest: true }))
       /* This callback should run in bubble phase, so that that CWV api, like "onLCP", is called before the final harvest so that emitted timings are part of last outgoing. */
     }, false)
-
-    /* Flush all buffered data if session resets and give up retries. This should be synchronous to ensure that the correct `session` value is sent.
-      Since session-reset generates a new session ID and the ID is grabbed at send-time, any delays or retries would cause the payload to be sent under the wrong session ID. */
-    agentRef.ee.on(SESSION_EVENTS.RESET, () => this.initializedAggregates.forEach(aggregateInst => this.triggerHarvestFor(aggregateInst, { forceNoRetry: true })))
   }
 
   startTimer (harvestInterval = this.agentRef.init.harvest.interval) {

src/common/session/session-entity.js

@@ -227,7 +227,10 @@ export class SessionEntity {
     // * stop recording (stn and sr)...
     // * delete the session and start over
     try {
-      if (this.initialized) this.ee.emit(SESSION_EVENTS.RESET)
+      if (this.initialized) {
+        this.ee.emit(SESSION_EVENTS.RESET)
+        this.state.numOfResets++
+      }
       this.storage.remove(this.lookupKey)
       this.inactiveTimer?.abort?.()
       this.expiresTimer?.clear?.()
@@ -239,7 +242,7 @@ export class SessionEntity {
         storage: this.storage,
         expiresMs: this.expiresMs,
         inactiveMs: this.inactiveMs,
-        numOfResets: ++this.state.numOfResets
+        numOfResets: this.state.numOfResets
       })
       return this.read()
     } catch (e) {
@@ -256,10 +259,6 @@ export class SessionEntity {
     this.write({ ...existingData, inactiveAt: this.getFutureTimestamp(this.inactiveMs) })
   }
 
-  isAfterSessionExpiry (timestamp) {
-    return this.state.numOfResets > 0 || (typeof timestamp === 'number' && typeof this.state.expiresAt === 'number' && timestamp >= this.state.expiresAt)
-  }
-
   /**
    * @param {number} timestamp
    * @returns {boolean}

src/features/session_replay/shared/recorder.js

@@ -155,10 +155,6 @@ export class Recorder {
   /** Store a payload in the buffer (this.#events).  This should be the callback to the recording lib noticing a mutation */
   store (event, isCheckout) {
     if (!event) return
-    if (this.parent.agentRef.runtime.session?.isAfterSessionExpiry(event.timestamp)) {
-      handle(SUPPORTABILITY_METRIC_CHANNEL, ['Session/Expired/SessionReplay/Seen'], undefined, FEATURE_NAMES.metrics, this.ee)
-      return
-    }
 
     if (!(this.parent instanceof AggregateBase) && this.#preloaded.length) this.currentBufferTarget = this.#preloaded[this.#preloaded.length - 1]
     else this.currentBufferTarget = this.#events

src/features/session_trace/aggregate/index.js

@@ -103,7 +103,7 @@ export class Aggregate extends AggregateBase {
   }
 
   preHarvestChecks () {
-    if (this.mode !== MODE.FULL) return // only allow harvest if running in full mode
+    if (this.blocked || this.mode !== MODE.FULL) return // only allow harvest if running in full mode
     if (!this.timeKeeper?.ready) return // this should likely never happen, but just to be safe, we should never harvest if we cant correct time
     if (!this.agentRef.runtime.session) return // session entity is required for trace to run and continue running
     if (this.sessionId !== this.agentRef.runtime.session.state.value || this.ptid !== this.agentRef.runtime.ptid) {

src/features/session_trace/aggregate/trace/storage.js

@@ -41,10 +41,6 @@ export class TraceStorage {
     this.parent = parent
   }
 
-  isAfterSessionExpiry (entryTimestamp) {
-    return this.parent.agentRef.runtime?.session?.isAfterSessionExpiry((this.parent.timeKeeper?.ready && this.parent.timeKeeper.convertRelativeTimestamp(entryTimestamp)) ?? undefined)
-  }
-
   /** Central function called by all the other store__ & addToTrace API to append a trace node. */
   storeSTN (stn) {
     if (this.parent.blocked) return
@@ -53,10 +49,6 @@ export class TraceStorage {
       const openedSpace = this.trimSTNs(ERROR_MODE_SECONDS_WINDOW) // but maybe we could make some space by discarding irrelevant nodes if we're in sessioned Error mode
       if (openedSpace === 0) return
     }
-    if (this.isAfterSessionExpiry(stn.s)) {
-      this.parent.reportSupportabilityMetric('Session/Expired/SessionTrace/Seen')
-      return
-    }
 
     if (this.trace[stn.n]) this.trace[stn.n].push(stn)
     else this.trace[stn.n] = [stn]

tests/components/session-entity.test.js

@@ -295,28 +295,3 @@ describe('syncCustomAttribute()', () => {
     expect(session.read().custom?.test).toEqual(undefined)
   })
 })
-
-describe('isAfterSessionExpiry', () => {
-  test('returns true if session has expired before', () => {
-    const now = Date.now()
-    jest.setSystemTime(now)
-    const session = new SessionEntity({ agentIdentifier, key, storage, expiresMs: 10 })
-    jest.advanceTimersByTime(11)
-    expect(session.state.numOfResets).toEqual(1)
-    expect(session.isAfterSessionExpiry()).toEqual(true)
-  })
-
-  test('returns true if event timestamp is equal to session expiry', () => {
-    const now = Date.now()
-    jest.setSystemTime(now)
-    const session = new SessionEntity({ agentIdentifier, key, storage, expiresMs: 10 })
-    expect(session.isAfterSessionExpiry(now + 10)).toEqual(true)
-  })
-
-  test('returns false if session has never expired AND event timestamp occurs before session expiry', () => {
-    const now = Date.now()
-    jest.setSystemTime(now)
-    const session = new SessionEntity({ agentIdentifier, key, storage, expiresMs: 10 })
-    expect(session.isAfterSessionExpiry(now + 9)).toEqual(false)
-  })
-})

tests/components/setup-agent.js

@@ -101,5 +101,4 @@ function resetAggregator (agentIdentifier) {
 function resetSession (agentIdentifier) {
   const agent = getNREUMInitializedAgent(agentIdentifier)
   agent.runtime.session.reset()
-  agent.runtime.session.state.numOfResets = 0 // avoid stopping harvests for session resets
 }

tests/specs/session-replay/session-state.e2e.js

@@ -48,16 +48,16 @@ describe('session manager state behavior', () => {
 
   describe('when session ends', () => {
     it('should end recording and clear the buffer', async () => {
+      const sessionReplayCapture = await browser.testHandle.createNetworkCaptures('bamServer', { test: testBlobReplayRequest })
       await browser.enableSessionReplay()
-      await browser.url(await browser.testHandle.assetURL('rrweb-record.html', { init: { session: { expiresMs: 7500 }, session_replay: { enabled: true } } }))
+      await browser.url(await browser.testHandle.assetURL('rrweb-instrumented.html', { init: { session: { expiresMs: 7500 }, session_replay: { enabled: true } } }))
         .then(() => browser.waitForSessionReplayRecording())
 
       // session has started, replay should have set mode to "FULL"
       const { agentSessions: oldSession } = await browser.getAgentSessionInfo()
       const oldSessionClass = Object.values(oldSession)[0]
       expect(oldSessionClass.sessionReplayMode).toEqual(MODE.FULL)
 
-      const sessionReplayCapture = await browser.testHandle.createNetworkCaptures('bamServer', { test: testBlobReplayRequest })
       const [sessionReplayHarvests] = await Promise.all([
         sessionReplayCapture.waitForResult({ timeout: 10000 }),
         browser.execute(function () {

tests/unit/common/harvest/harvester.test.js

@@ -25,7 +25,6 @@ test('Harvester does not start timer loop on initialization', () => {
 
   const harvester = new Harvester(fakeAgent)
   expect(mockEolCb).not.toBeUndefined()
-  expect(fakeAgent.ee.on).toHaveBeenCalledTimes(1)
   expect(harvester.agentRef).toEqual(fakeAgent)
   expect(global.setTimeout).not.toHaveBeenCalled()
 })

tests/unit/features/session_replay/shared/recorder.test.js

@@ -1,32 +1,6 @@
 import { Recorder } from '../../../../../src/features/session_replay/shared/recorder'
 
 describe('recorder', () => {
-  test('should not store event if past session expiry', async () => {
-    const event = { timestamp: 123 }
-    const recorder = new Recorder({
-      agentRef: {
-        init: {
-          session_replay: {
-            stylesheets: true
-          }
-        },
-        runtime: {
-          session: {
-            state: {
-              expiresAt: 123
-            },
-            isAfterSessionExpiry: () => true
-          }
-        }
-      },
-      reportSupportabilityMetric: jest.fn()
-    })
-
-    recorder.store(event)
-
-    expect(recorder.getEvents().events).toHaveLength(0)
-  })
-
   // it's possible for events collection to start before session is initialized
   test('should store event if no session present', async () => {
     const event = { timestamp: 123 }