Merge branch 'develop' into mschile/service_worker_uncontrolled

cli/CHANGELOG.md

@@ -7,6 +7,10 @@ _Released 4/23/2024 (PENDING)_
 
 - Fixed a performance issue with activated service workers that aren't controlling clients which could lead to correlation timeouts. Fixes [#29333](https://github.com/cypress-io/cypress/issues/29333) and [#29126](https://github.com/cypress-io/cypress/issues/29126).
 
+**Bugfixes:**
+
+- Fixed a bug introduced in [`13.6.0`](https://docs.cypress.io/guides/references/changelog#13-6-0) where Cypress would occasionally exit with status code 1, even when a test run was successfully, due to an unhandled WebSocket exception (`Error: WebSocket connection closed`). Addresses [#28523](https://github.com/cypress-io/cypress/issues/28523).
+
 **Dependency Updates:**
 
 - Updated zod from `3.20.3` to `3.22.5`. Addressed in [#29367](https://github.com/cypress-io/cypress/pull/29367).

packages/server/lib/browsers/cri-client.ts

@@ -11,12 +11,23 @@ import type { SendDebuggerCommand, OnFn, CdpCommand, CdpEvent } from './cdp_auto
 import type { ProtocolManagerShape } from '@packages/types'
 
 const debug = debugModule('cypress:server:browsers:cri-client')
+const debugVerbose = debugModule('cypress-verbose:server:browsers:cri-client')
 // debug using cypress-verbose:server:browsers:cri-client:send:*
-const debugVerboseSend = debugModule('cypress-verbose:server:browsers:cri-client:send:[-->]')
+const debugVerboseSend = debugModule(`${debugVerbose.namespace}:send:[-->]`)
 // debug using cypress-verbose:server:browsers:cri-client:recv:*
-const debugVerboseReceive = debugModule('cypress-verbose:server:browsers:cri-client:recv:[<--]')
-
-const WEBSOCKET_NOT_OPEN_RE = /^WebSocket is (?:not open|already in CLOSING or CLOSED state)/
+const debugVerboseReceive = debugModule(`${debugVerbose.namespace}:recv:[<--]`)
+// debug using cypress-verbose:server:browsers:cri-client:err:*
+const debugVerboseLifecycle = debugModule(`${debugVerbose.namespace}:ws`)
+
+/**
+ * There are three error messages we can encounter which should not be re-thrown, but
+ * should trigger a reconnection attempt if one is not in progress, and enqueue the
+ * command that errored. This regex is used in client.send to check for:
+ * - WebSocket connection closed
+ * - WebSocket not open
+ * - WebSocket already in CLOSING or CLOSED state
+ */
+const WEBSOCKET_NOT_OPEN_RE = /^WebSocket (?:connection closed|is (?:not open|already in CLOSING or CLOSED state))/
 
 type QueuedMessages = {
   enableCommands: EnableCommand[]
@@ -136,6 +147,20 @@ const maybeDebugCdpMessages = (cri: CDPClient) => {
       return send.call(cri._ws, data, callback)
     }
   }
+
+  if (debugVerboseLifecycle.enabled) {
+    cri._ws.addEventListener('open', (event) => {
+      debugVerboseLifecycle(`[OPEN] %o`, event)
+    })
+
+    cri._ws.addEventListener('close', (event) => {
+      debugVerboseLifecycle(`[CLOSE] %o`, event)
+    })
+
+    cri._ws.addEventListener('error', (event) => {
+      debugVerboseLifecycle(`[ERROR] %o`, event)
+    })
+  }
 }
 
 type DeferredPromise = { resolve: Function, reject: Function }
@@ -167,6 +192,7 @@ export const create = async ({
   let closed = false // has the user called .close on this?
   let connected = false // is this currently connected to CDP?
   let crashed = false // has this crashed?
+  let reconnection: Promise<void> | undefined = undefined
 
   let cri: CDPClient
   let client: CriClient
@@ -195,8 +221,25 @@ export const create = async ({
 
     // '*.enable' commands need to be resent on reconnect or any events in
     // that namespace will no longer be received
-    await Promise.all(enableCommands.map(({ command, params, sessionId }) => {
-      return cri.send(command, params, sessionId)
+    await Promise.all(enableCommands.map(async ({ command, params, sessionId }) => {
+      // these commands may have been enqueued, so we need to resolve those promises and remove
+      // them from the queue when we send here
+      const isInFlightCommand = (candidate: EnqueuedCommand) => {
+        return candidate.command === command && candidate.params === params && candidate.sessionId === sessionId
+      }
+      const enqueued = enqueuedCommands.find(isInFlightCommand)
+
+      try {
+        const response = await cri.send(command, params, sessionId)
+
+        enqueued?.p.resolve(response)
+      } catch (e) {
+        enqueued?.p.reject(e)
+      } finally {
+        enqueuedCommands = enqueuedCommands.filter((candidate) => {
+          return !isInFlightCommand(candidate)
+        })
+      }
     }))
 
     enqueuedCommands.forEach((cmd) => {
@@ -216,13 +259,23 @@ export const create = async ({
   }
 
   const retryReconnect = async () => {
+    if (reconnection) {
+      debug('reconnection in progress; not starting new process, returning promise for in-flight reconnection attempt')
+
+      return reconnection
+    }
+
     debug('disconnected, starting retries to reconnect... %o', { closed, target })
 
     const retry = async (retryIndex = 0) => {
       retryIndex++
 
       try {
-        return await reconnect(retryIndex)
+        const attempt = await reconnect(retryIndex)
+
+        reconnection = undefined
+
+        return attempt
       } catch (err) {
         if (closed) {
           debug('could not reconnect because client is closed %o', { closed, target })
@@ -244,11 +297,14 @@ export const create = async ({
 
         // If we cannot reconnect to CDP, we will be unable to move to the next set of specs since we use CDP to clean up and close tabs. Marking this as fatal
         cdpError.isFatalApiErr = true
+        reconnection = undefined
         onAsynchronousError(cdpError)
       }
     }
 
-    return retry()
+    reconnection = retry()
+
+    return reconnection
   }
 
   const connect = async () => {
@@ -358,6 +414,7 @@ export const create = async ({
       // Keep track of '*.enable' commands so they can be resent when
       // reconnecting
       if (command.endsWith('.enable') || ['Runtime.addBinding', 'Target.setDiscoverTargets'].includes(command)) {
+        debug('registering enable command', command)
         const obj: EnableCommand = {
           command,
         }
@@ -377,15 +434,17 @@ export const create = async ({
         try {
           return await cri.send(command, params, sessionId)
         } catch (err) {
+          debug('Encountered error on send %o', { command, params, sessionId, err })
           // This error occurs when the browser has been left open for a long
           // time and/or the user's computer has been put to sleep. The
           // socket disconnects and we need to recreate the socket and
           // connection
           if (!WEBSOCKET_NOT_OPEN_RE.test(err.message)) {
+            debug('error classified as not WEBSOCKET_NOT_OPEN_RE, rethrowing')
             throw err
           }
 
-          debug('encountered closed websocket on send %o', { command, params, sessionId, err })
+          debug('error classified as WEBSOCKET_NOT_OPEN_RE; enqueuing and attempting to reconnect')
 
           const p = enqueue() as Promise<any>
 

packages/server/test/integration/cdp_spec.ts

@@ -2,12 +2,13 @@ process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'
 
 import Debug from 'debug'
 import _ from 'lodash'
-import { Server as WebSocketServer } from 'ws'
+import WebSocket from 'ws'
 import { CdpCommand, CdpEvent } from '../../lib/browsers/cdp_automation'
 import * as CriClient from '../../lib/browsers/cri-client'
 import { expect, nock } from '../spec_helper'
 
-import type { SinonStub } from 'sinon'
+import sinon from 'sinon'
+
 // import Bluebird from 'bluebird'
 
 const debug = Debug('cypress:server:tests')
@@ -29,14 +30,14 @@ type OnWSConnection = (wsClient: WebSocket) => void
 describe('CDP Clients', () => {
   require('mocha-banner').register()
 
-  let wsSrv: WebSocketServer
+  let wsSrv: WebSocket.Server
   let criClient: CriClient.CriClient
   let messages: object[]
-  let onMessage: SinonStub
+  let onMessage: sinon.SinonStub
 
-  const startWsServer = async (onConnection?: OnWSConnection): Promise<WebSocketServer> => {
+  const startWsServer = async (onConnection?: OnWSConnection): Promise<WebSocket.Server> => {
     return new Promise((resolve, reject) => {
-      const srv = new WebSocketServer({
+      const srv = new WebSocket.Server({
         port: wsServerPort,
       })
 
@@ -209,7 +210,7 @@ describe('CDP Clients', () => {
 
         const send = (commands: CDPCommands[]) => {
           commands.forEach(({ command, params }) => {
-            criClient.send(command, params).catch(() => {})
+            criClient.send(command, params)
           })
         }
 
@@ -319,7 +320,7 @@ describe('CDP Clients', () => {
       })
       .then((stub) => {
         expect(criClient.closed).to.be.true
-        expect((stub as SinonStub).callCount).to.be.eq(3)
+        expect((stub as sinon.SinonStub).callCount).to.be.eq(3)
       })
     })
   })

packages/server/test/unit/browsers/cri-client_spec.ts

@@ -130,6 +130,17 @@ describe('lib/browsers/cri-client', function () {
 
           expect(client.send('DOM.getDocument', { depth: -1 })).to.be.rejectedWith('DOM.getDocument will not run as browser CRI connection was reset')
         })
+
+        it(`when socket is closed mid send ('WebSocket connection closed' variant)`, async function () {
+          const err = new Error('WebSocket connection closed')
+
+          send.onFirstCall().rejects(err)
+          const client = await getClient()
+
+          await client.close()
+
+          expect(client.send('DOM.getDocument', { depth: -1 })).to.be.rejectedWith('DOM.getDocument will not run as browser CRI connection was reset')
+        })
       })
     })
   })