perf: support activated service worker that is not handling requests (#…

…29349)

.circleci/cache-version.txt

@@ -1,3 +1,3 @@
 # Bump this version to force CI to re-create the cache from scratch.
 
-04-15-24-macstadium-3
+04-22-24

.circleci/workflows.yml

@@ -29,9 +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'
+        - 'update-v8-snapshot-cache-on-develop'
 
 # usually we don't build Mac app - it takes a long time
 # but sometimes we want to really confirm we are doing the right thing
@@ -42,8 +40,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 >> ]
+    - equal: [ 'mschile/service_worker_uncontrolled', << pipeline.git.branch >> ]
     - matches:
         pattern: /^release\/\d+\.\d+\.\d+$/
         value: << pipeline.git.branch >>
@@ -54,8 +51,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 >> ]
+    - equal: [ 'mschile/service_worker_uncontrolled', << pipeline.git.branch >> ]
     - matches:
         pattern: /^release\/\d+\.\d+\.\d+$/
         value: << pipeline.git.branch >>
@@ -78,8 +74,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 >> ]
+    - equal: [ 'mschile/service_worker_uncontrolled', << pipeline.git.branch >> ]
     - matches:
         pattern: /^release\/\d+\.\d+\.\d+$/
         value: << pipeline.git.branch >>
@@ -146,7 +141,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/psrotocol_shadow_dom_support" && "$CIRCLE_BRANCH" != "feat/support_wds5" && "$CIRCLE_BRANCH" != "cacie/fix/websocket-closed" ]]; then
+            echo 'if ! [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "mschile/service_worker_uncontrolled" ]]; 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

cli/CHANGELOG.md

@@ -3,9 +3,13 @@
 
 _Released 4/23/2024 (PENDING)_
 
+**Performance:**
+
+- 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).
+- Fixed a regression 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 successful, due to an unhandled WebSocket exception (`Error: WebSocket connection closed`). Addresses [#28523](https://github.com/cypress-io/cypress/issues/28523).
 - Fixed an issue where Cypress would hang on some commands when an invalid `timeout` option was provided. Fixes [#29323](https://github.com/cypress-io/cypress/issues/29323).
 
 **Misc:**

packages/driver/cypress/e2e/e2e/service-worker.cy.js

@@ -666,4 +666,51 @@ describe('service workers', { defaultCommandTimeout: 1000, pageLoadTimeout: 1000
     cy.get('#output').should('have.text', 'done')
     validateFetchHandlers({ listenerCount: 1 })
   })
+
+  it('supports a service worker that is activated but not handling fetch events', () => {
+    const script = () => {
+      self.addEventListener('fetch', function (event) {
+        event.respondWith(fetch(event.request))
+      })
+    }
+
+    cy.intercept('/fixtures/service-worker.js', (req) => {
+      req.reply(`(${script})()`,
+        { 'Content-Type': 'application/javascript' })
+    })
+
+    cy.visit('fixtures/service-worker.html?skipReload')
+
+    cy.get('#output').should('have.text', 'done')
+    validateFetchHandlers({ listenerCount: 1 })
+  })
+
+  it('supports a redirected request', () => {
+    const script = () => {
+      self.addEventListener('fetch', function (event) {
+        return
+      })
+    }
+
+    cy.intercept('/fixtures/service-worker.js', (req) => {
+      req.reply(`(${script})()`,
+        { 'Content-Type': 'application/javascript' })
+    })
+
+    cy.intercept('/fixtures/1mb*', (req) => {
+      req.reply({
+        statusCode: 302,
+        headers: {
+          location: '/fixtures/redirected',
+        },
+      })
+    })
+
+    cy.intercept('/fixtures/redirected', (req) => {
+      req.reply('redirected')
+    })
+
+    cy.visit('fixtures/service-worker.html')
+    cy.get('#output').should('have.text', 'done')
+  })
 })

packages/driver/cypress/fixtures/service-worker.html

@@ -4,7 +4,7 @@
   <title></title>
   <script>
     const makeRequest = () => {
-      fetch(`/fixtures/1mb?j=${Math.random()}`).then((response) => {
+      fetch(`/fixtures/1mb?j=${Math.random()}#hash`).then((response) => {
         document.getElementById('output').innerText = 'done'
       }).catch(() => {})
     }
@@ -14,15 +14,22 @@
       const serviceWorker = registration.installing || registration.waiting
       if (serviceWorker) {
         let controllerChangeCalled = false
+        // called when clients.claim() is called in the service worker
         navigator.serviceWorker.addEventListener('controllerchange', (e) => {
           controllerChangeCalled = true
           makeRequest()
         })
 
         serviceWorker.addEventListener('statechange', (e) => {
           if (e.target.state === 'activated' && !controllerChangeCalled) {
-            // service worker activated, reload page so it can intercept requests
-            location.reload()
+            // service worker activated, either skip reloading the page
+            // so we can test when an activated service worker is not intercepting requests
+            // or reload the page so it can intercept requests
+            if (location.search.includes('skipReload')) {
+              makeRequest()
+            } else {
+              location.reload()
+            }
           }
         })
       } else {

packages/proxy/lib/http/index.ts

@@ -26,7 +26,7 @@ import type { Request, Response } from 'express'
 import type { RemoteStates } from '@packages/server/lib/remote_states'
 import type { CookieJar, SerializableAutomationCookie } from '@packages/server/lib/util/cookies'
 import type { ResourceTypeAndCredentialManager } from '@packages/server/lib/util/resourceTypeAndCredentialManager'
-import type { ProtocolManagerShape } from '@packages/types'
+import type { FoundBrowser, ProtocolManagerShape } from '@packages/types'
 import type Protocol from 'devtools-protocol'
 import type { ServiceWorkerClientEvent } from './util/service-worker-manager'
 
@@ -101,6 +101,7 @@ export type ServerCtx = Readonly<{
   socket: CyServer.Socket
   request: any
   serverBus: EventEmitter
+  getCurrentBrowser: () => FoundBrowser
 }>
 
 const READONLY_MIDDLEWARE_KEYS: (keyof HttpMiddlewareThis<{}>)[] = [
@@ -272,6 +273,7 @@ export class Http {
   middleware: HttpMiddlewareStacks
   netStubbingState: NetStubbingState
   preRequests: PreRequests = new PreRequests()
+  getCurrentBrowser: () => FoundBrowser
   request: any
   socket: CyServer.Socket
   serverBus: EventEmitter
@@ -296,6 +298,7 @@ export class Http {
     this.serverBus = opts.serverBus
     this.resourceTypeAndCredentialManager = opts.resourceTypeAndCredentialManager
     this.getCookieJar = opts.getCookieJar
+    this.getCurrentBrowser = opts.getCurrentBrowser
 
     if (typeof opts.middleware === 'undefined') {
       this.middleware = defaultMiddleware
@@ -361,6 +364,7 @@ export class Http {
         this.preRequests.removePendingRequest(pendingRequest)
       },
       protocolManager: this.protocolManager,
+      getCurrentBrowser: this.getCurrentBrowser,
     }
 
     const onError = (error: Error): Promise<void> => {

packages/proxy/lib/http/response-middleware.ts

@@ -839,7 +839,8 @@ const MaybeInjectServiceWorker: ResponseMiddleware = function () {
 
   span?.setAttributes({ hasServiceWorkerHeader: hasHeader })
 
-  if (!hasHeader) {
+  // skip if we don't have the header or we're not in chromium
+  if (!hasHeader || this.getCurrentBrowser().family !== 'chromium') {
     span?.end()
 
     return this.next()

packages/proxy/lib/http/util/prerequests.ts

@@ -116,6 +116,7 @@ export class PreRequests {
   pendingPreRequests = new QueueMap<PendingPreRequest>()
   pendingRequests = new QueueMap<PendingRequest>()
   pendingUrlsWithoutPreRequests = new QueueMap<PendingUrlWithoutPreRequest>()
+  pendingPreRequestsToRemove: Map<string, number> = new Map()
   sweepIntervalTimer: NodeJS.Timeout
   protocolManager?: ProtocolManagerShape
 
@@ -144,21 +145,43 @@ export class PreRequests {
           debugVerbose('timed out unmatched pre-request: %o', browserPreRequest)
           metrics.unmatchedPreRequests++
 
+          this.pendingPreRequestsToRemove.delete(browserPreRequest.requestId)
+
           return false
         }
 
         return true
       })
 
+      this.pendingPreRequestsToRemove.forEach((timestamp, requestId) => {
+        if (timestamp + this.sweepInterval < now) {
+          this.pendingPreRequestsToRemove.delete(requestId)
+        }
+      })
+
       this.pendingUrlsWithoutPreRequests.removeMatching(({ timestamp }) => {
         return timestamp + this.sweepInterval >= now
       })
     }, this.sweepInterval)
   }
 
+  checkAndRemovePendingPreRequest (requestId: string) {
+    return this.pendingPreRequestsToRemove.delete(requestId)
+  }
+
   addPending (browserPreRequest: BrowserPreRequest) {
     const key = `${browserPreRequest.method}-${tryDecodeURI(browserPreRequest.url)}`
 
+    // if we have a pending request to remove, we should remove it now and return
+    // since we don't want to proceed with this pre-request anymore. In practice,
+    // this typically happens when we receive a cached response while executing
+    // the async function to add the pre-request
+    if (this.checkAndRemovePendingPreRequest(browserPreRequest.requestId)) {
+      debugVerbose('Received previous request to remove pre-request %s, skipping adding', browserPreRequest.requestId)
+
+      return
+    }
+
     metrics.browserPreRequestsReceived++
     const pendingRequest = this.pendingRequests.shift(key)
 
@@ -225,9 +248,30 @@ export class PreRequests {
   }
 
   removePendingPreRequest (requestId: string) {
+    let removed = false
+
     this.pendingPreRequests.removeMatching(({ browserPreRequest }) => {
-      return (browserPreRequest.requestId.includes('-retry-') && !browserPreRequest.requestId.startsWith(`${requestId}-`)) || (!browserPreRequest.requestId.includes('-retry-') && browserPreRequest.requestId !== requestId)
+      const matches = (browserPreRequest.requestId.includes('-retry-') && browserPreRequest.requestId.startsWith(`${requestId}-`)) ||
+        (!browserPreRequest.requestId.includes('-retry-') && browserPreRequest.requestId === requestId)
+
+      if (matches && !removed) {
+        removed = true
+      }
+
+      return !matches
     })
+
+    // if we didn't find a pre-request to remove, add it to the pending list so we can remove it later,
+    // this typically happens when we receive a cached response while executing
+    // the async function to add the pre-request
+    if (!removed) {
+      debugVerbose('No pre-request found to remove, adding to pending list: %s', requestId)
+      this.pendingPreRequestsToRemove.set(requestId, Date.now())
+    } else {
+      debugVerbose('Removed pre-request %s', requestId)
+    }
+
+    return removed
   }
 
   get (req: CypressIncomingRequest, ctxDebug, callback: GetPreRequestCb) {
@@ -322,5 +366,6 @@ export class PreRequests {
 
     this.pendingRequests = new QueueMap<PendingRequest>()
     this.pendingUrlsWithoutPreRequests = new QueueMap<PendingUrlWithoutPreRequest>()
+    this.pendingPreRequestsToRemove = new Map()
   }
 }