diff --git a/.circleci/workflows.yml b/.circleci/workflows.yml index d4c2b551e731..a37d595f6a39 100644 --- a/.circleci/workflows.yml +++ b/.circleci/workflows.yml @@ -29,6 +29,7 @@ mainBuildFilters: &mainBuildFilters - develop - /^release\/\d+\.\d+\.\d+$/ # use the following branch as well to ensure that v8 snapshot cache updates are fully tested + - 'cacie/fix/websocket-closed' - 'app-mem-mng-flag' - 'publish-binary' @@ -41,6 +42,7 @@ macWorkflowFilters: &darwin-workflow-filters - equal: [ develop, << pipeline.git.branch >> ] # use the following branch as well to ensure that v8 snapshot cache updates are fully tested - equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ] + - equal: [ 'cacie/fix/websocket-closed', << pipeline.git.branch >> ] - equal: [ 'app-mem-mng-flag', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ @@ -52,6 +54,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters - equal: [ develop, << pipeline.git.branch >> ] # use the following branch as well to ensure that v8 snapshot cache updates are fully tested - equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ] + - equal: [ 'cacie/fix/websocket-closed', << pipeline.git.branch >> ] - equal: [ 'app-mem-mng-flag', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ @@ -75,6 +78,7 @@ windowsWorkflowFilters: &windows-workflow-filters - equal: [ develop, << pipeline.git.branch >> ] # use the following branch as well to ensure that v8 snapshot cache updates are fully tested - equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ] + - equal: [ 'cacie/fix/websocket-closed', << pipeline.git.branch >> ] - equal: [ 'app-mem-mng-flag', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ @@ -142,7 +146,7 @@ commands: name: Set environment variable to determine whether or not to persist artifacts command: | echo "Setting SHOULD_PERSIST_ARTIFACTS variable" - echo 'if ! [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "publish-binary" && "$CIRCLE_BRANCH" != "feat/protocol_shadow_dom_support" && "$CIRCLE_BRANCH" != "feat/support_wds5" ]]; then + echo 'if ! [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "publish-binary" && "$CIRCLE_BRANCH" != "feat/psrotocol_shadow_dom_support" && "$CIRCLE_BRANCH" != "feat/support_wds5" && "$CIRCLE_BRANCH" != "cacie/fix/websocket-closed" ]]; then export SHOULD_PERSIST_ARTIFACTS=true fi' >> "$BASH_ENV" # You must run `setup_should_persist_artifacts` command and be using bash before running this command diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 9b33373b3cef..350d491e2ac4 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -3,6 +3,10 @@ _Released 4/23/2024 (PENDING)_ +**Bugfixes:** + +- Fixes 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). diff --git a/packages/server/lib/browsers/cri-client.ts b/packages/server/lib/browsers/cri-client.ts index eaabd7522af4..e75e661e8f61 100644 --- a/packages/server/lib/browsers/cri-client.ts +++ b/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 | 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 diff --git a/packages/server/test/integration/cdp_spec.ts b/packages/server/test/integration/cdp_spec.ts index f2988d74445d..b42f0c71f5f2 100644 --- a/packages/server/test/integration/cdp_spec.ts +++ b/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 => { + const startWsServer = async (onConnection?: OnWSConnection): Promise => { 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) }) }) }) diff --git a/packages/server/test/unit/browsers/cri-client_spec.ts b/packages/server/test/unit/browsers/cri-client_spec.ts index 5918e56b8d19..b9958b773f83 100644 --- a/packages/server/test/unit/browsers/cri-client_spec.ts +++ b/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') + }) }) }) })