Stop session stack pollution on repeated teardown starts

lib/mocha/asyncWrapper.js

@@ -54,7 +54,10 @@ export function test(test) {
 
   test.fn = function (done) {
     const doneFn = makeDoneCallableOnce(done)
+    let recoveryStarted = false
     recorder.errHandler(err => {
+      if (recoveryStarted) return
+      recoveryStarted = true
       recorder.session.start('teardown')
       recorder.cleanAsyncErr()
       if (test.throws) {
@@ -114,7 +117,13 @@ export function test(test) {
 export function injected(fn, suite, hookName) {
   return function (done) {
     const doneFn = makeDoneCallableOnce(done)
+    const opts = suite.opts || {}
+    const retries = opts[`retry${ucfirst(hookName)}`] || 0
+    let retryPending = retries > 0
+    let recoveryStarted = false
     const errHandler = err => {
+      if (retryPending || recoveryStarted) return
+      recoveryStarted = true
       recorder.session.start('teardown')
       recorder.cleanAsyncErr()
       if (['before', 'beforeSuite'].includes(hookName)) {
@@ -149,9 +158,6 @@ export function injected(fn, suite, hookName) {
       })
     }
 
-    const opts = suite.opts || {}
-    const retries = opts[`retry${ucfirst(hookName)}`] || 0
-
     const currentTest = hookName === 'before' || hookName === 'after' ? suite?.ctx?.currentTest : null
 
     promiseRetry(
@@ -165,8 +171,13 @@ export function injected(fn, suite, hookName) {
           retry(err)
         } finally {
           if (number < retries) {
+            // drain the failed chain so in-flight steps (sessions, listeners)
+            // finish before the next attempt starts queueing new ones
+            await recorder.promise().catch(() => {})
             recorder.stop()
             recorder.start()
+          } else {
+            retryPending = false
           }
         }
       },

lib/plugin/auth.js

@@ -303,6 +303,7 @@ export default function (config) {
 
     section.start()
 
+    const queueId = recorder.getQueueId()
     const cookies = store[`${name}_session`]
     const shouldAwait = isAsyncFunction(userSession.login) || isAsyncFunction(userSession.restore) || isAsyncFunction(userSession.check)
 
@@ -338,6 +339,7 @@ export default function (config) {
     }
     section.end()
     recorder.session.catch(err => {
+      if (recorder.getQueueId() !== queueId) return
       output.debug(`Failed auto login for ${name} due to ${err}`)
       output.debug('Logging in again')
       recorder.session.start('auto login')
@@ -355,6 +357,7 @@ export default function (config) {
         })
     })
     recorder.add(() => {
+      if (recorder.getQueueId() !== queueId) return
       recorder.session.restore('check login')
     })
 

lib/recorder.js

@@ -89,6 +89,8 @@ export default {
    */
   reset() {
     if (promise && running) this.catch()
+    else swallow(promise)
+    oldPromises.forEach(swallow)
     queueId++
     sessionId = null
     sessionStack = [] // Clear the session stack
@@ -125,6 +127,16 @@ export default {
      * @inner
      */
     start(name) {
+      if (sessionId === name) {
+        // re-starting the same session abandons the current (broken) chain
+        // without polluting the session stack
+        debug(`${currentQueue()}Session <${name}> already started, abandoning its chain`)
+        tasks.push('--->')
+        this.running = true
+        swallow(promise)
+        promise = Promise.resolve()
+        return
+      }
       if (sessionId) {
         debug(`${currentQueue()}Session already started as ${sessionId}, nesting session ${name}`)
         // Push current session to stack instead of restoring it
@@ -136,7 +148,7 @@ export default {
       }
       debug(`${currentQueue()}Starting <${name}> session`)
       tasks.push('--->')
-      oldPromises.push(promise)
+      oldPromises.push(swallow(promise))
       this.running = true
       sessionId = name
       promise = Promise.resolve()
@@ -151,7 +163,8 @@ export default {
       debug(`${currentQueue()}Finalize <${name}> session`)
       this.running = false
       this.catch(errFn)
-      promise = promise.then(() => oldPromises.pop())
+      const restored = oldPromises.pop()
+      if (restored && restored !== promise) promise = promise.then(() => restored)
 
       // Restore parent session from stack if available
       if (sessionStack.length > 0) {
@@ -426,6 +439,11 @@ export default {
   },
 }
 
+function swallow(p) {
+  if (p) p.catch(() => {})
+  return p
+}
+
 function getTimeoutPromise(timeoutMs, taskName) {
   let timer
   if (timeoutMs) debug(`Timing out in ${timeoutMs}ms`)

test/unit/recorder_test.js

@@ -28,6 +28,76 @@ describe('Recorder', () => {
       recorder.add(() => (order += 'b'))
       return recorder.promise().then(() => expect(order).is.equal('acdb'))
     })
+
+    it('re-starting the same session name does not pollute the session stack', () => {
+      let order = ''
+      recorder.add(() => (order += 'a'))
+      recorder.add(() => {
+        recorder.session.start('teardown')
+        recorder.add(() => (order += 'c'))
+        recorder.session.start('teardown') // leaked teardown re-start: no sessionStack/oldPromises push
+        recorder.add(() => (order += 'd'))
+      })
+      recorder.add(() => recorder.session.restore('teardown'))
+      recorder.add(() => (order += 'b'))
+      // steps already attached to the live chain still run; a broken one would be abandoned
+      return recorder.promise().then(() => expect(order).is.equal('acdb'))
+    })
+
+    it('does not chain a promise onto itself when restoring an unbalanced stack', async () => {
+      const unhandled = []
+      const onUnhandled = e => unhandled.push(e)
+      process.on('unhandledRejection', onUnhandled)
+      try {
+        recorder.session.start('s1')
+        recorder.add(() => 'step')
+        recorder.session.restore('s1')
+        recorder.session.restore('extra') // nothing left to pop, must not create a cycle
+        await recorder.promise()
+        await new Promise(r => setTimeout(r, 20))
+        expect(unhandled).to.have.length(0)
+      } finally {
+        process.removeListener('unhandledRejection', onUnhandled)
+      }
+    })
+
+    it('does not report unhandled rejections for chains abandoned on session re-start', async () => {
+      const unhandled = []
+      const onUnhandled = e => unhandled.push(e)
+      process.on('unhandledRejection', onUnhandled)
+      try {
+        recorder.add('failing step', () => {
+          throw new Error('boom')
+        })
+        recorder.session.start('teardown')
+        recorder.session.start('teardown') // abandons the chain holding the failing step
+        recorder.add(() => 'recovered')
+        await recorder.promise()
+        await new Promise(r => setTimeout(r, 20))
+        expect(unhandled).to.have.length(0)
+      } finally {
+        process.removeListener('unhandledRejection', onUnhandled)
+      }
+    })
+
+    it('does not report unhandled rejections for chains discarded by reset', async () => {
+      const unhandled = []
+      const onUnhandled = e => unhandled.push(e)
+      process.on('unhandledRejection', onUnhandled)
+      try {
+        recorder.add('failing step', () => {
+          throw new Error('boom')
+        })
+        recorder.stop()
+        recorder.start()
+        recorder.add(() => 'fresh')
+        await recorder.promise()
+        await new Promise(r => setTimeout(r, 20))
+        expect(unhandled).to.have.length(0)
+      } finally {
+        process.removeListener('unhandledRejection', onUnhandled)
+      }
+    })
   })
 
   describe('#add', () => {