Skip to content

Commit

Permalink
fix: websocket closed (tentative) (#29347)
Browse files Browse the repository at this point in the history
* fix: updates regex for websocket connection error to properly handle

* build binaries for this branch

* do not reconnect if reconnect in progress; treat websocket not connected errors as websocket in closing/closed state errors

* changelog

* fix changelog

* publish prerelease binaries for this branch

* build windows prerelease package

* Update cli/CHANGELOG.md

* Update cli/CHANGELOG.md

Co-authored-by: Bill Glesias <bglesias@gmail.com>

* add comment for websocket error regex examples

---------

Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Co-authored-by: Bill Glesias <bglesias@gmail.com>
  • Loading branch information
3 people authored Apr 22, 2024
1 parent 734968a commit 4e7e3a2
Show file tree
Hide file tree
Showing 5 changed files with 97 additions and 18 deletions.
6 changes: 5 additions & 1 deletion .circleci/workflows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand All @@ -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+$/
Expand All @@ -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+$/
Expand All @@ -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+$/
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
77 changes: 68 additions & 9 deletions packages/server/lib/browsers/cri-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[]
Expand Down Expand Up @@ -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 }
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) => {
Expand All @@ -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 })
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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,
}
Expand All @@ -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>

Expand Down
17 changes: 9 additions & 8 deletions packages/server/test/integration/cdp_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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,
})

Expand Down Expand Up @@ -209,7 +210,7 @@ describe('CDP Clients', () => {

const send = (commands: CDPCommands[]) => {
commands.forEach(({ command, params }) => {
criClient.send(command, params).catch(() => {})
criClient.send(command, params)
})
}

Expand Down Expand Up @@ -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)
})
})
})
Expand Down
11 changes: 11 additions & 0 deletions packages/server/test/unit/browsers/cri-client_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
})
})
})
})
Expand Down

4 comments on commit 4e7e3a2

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 4e7e3a2 Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circle has built the linux arm64 version of the Test Runner.

Learn more about this pre-release build at https://on.cypress.io/advanced-installation#Install-pre-release-version

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/13.8.1/linux-arm64/develop-4e7e3a2e27ffc5ac36b33c41a7fbf940039a1ef2/cypress.tgz

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 4e7e3a2 Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circle has built the linux x64 version of the Test Runner.

Learn more about this pre-release build at https://on.cypress.io/advanced-installation#Install-pre-release-version

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/13.8.1/linux-x64/develop-4e7e3a2e27ffc5ac36b33c41a7fbf940039a1ef2/cypress.tgz

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 4e7e3a2 Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circle has built the darwin arm64 version of the Test Runner.

Learn more about this pre-release build at https://on.cypress.io/advanced-installation#Install-pre-release-version

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/13.8.1/darwin-arm64/develop-4e7e3a2e27ffc5ac36b33c41a7fbf940039a1ef2/cypress.tgz

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 4e7e3a2 Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circle has built the darwin x64 version of the Test Runner.

Learn more about this pre-release build at https://on.cypress.io/advanced-installation#Install-pre-release-version

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/13.8.1/darwin-x64/develop-4e7e3a2e27ffc5ac36b33c41a7fbf940039a1ef2/cypress.tgz

Please sign in to comment.