perf: support activated service worker that is not handling requests

.circleci/workflows.yml

@@ -29,10 +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/dep/electron-27'
-        - 'feat/protocol_shadow_dom_support'
-        - 'publish-binary'
-        - 'em/circle2'
+        - '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
@@ -43,9 +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/dep/electron-27', << pipeline.git.branch >> ]
-    - equal: [ 'feat/protocol_shadow_dom_support', << pipeline.git.branch >> ]
-    - equal: [ 'feat/support_wds5', << pipeline.git.branch >> ]
+    - equal: [ 'mschile/service_worker_uncontrolled', << pipeline.git.branch >> ]
     - matches:
         pattern: /^release\/\d+\.\d+\.\d+$/
         value: << pipeline.git.branch >>
@@ -56,9 +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/dep/electron-27', << pipeline.git.branch >> ]
-    - equal: [ 'feat/support_wds5', << pipeline.git.branch >> ]
-    - equal: [ 'em/circle2', << pipeline.git.branch >> ]
+    - equal: [ 'mschile/service_worker_uncontrolled', << pipeline.git.branch >> ]
     - matches:
         pattern: /^release\/\d+\.\d+\.\d+$/
         value: << pipeline.git.branch >>
@@ -81,10 +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/dep/electron-27', << pipeline.git.branch >> ]
-    - equal: [ 'feat/protocol_shadow_dom_support', << pipeline.git.branch >> ]
-    - equal: [ 'lerna-optimize-tasks', << pipeline.git.branch >> ]
-    - equal: [ 'feat/support_wds5', << pipeline.git.branch >> ]
+    - equal: [ 'mschile/service_worker_uncontrolled', << pipeline.git.branch >> ]
     - matches:
         pattern: /^release\/\d+\.\d+\.\d+$/
         value: << pipeline.git.branch >>
@@ -151,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/protocol_shadow_dom_support" && "$CIRCLE_BRANCH" != "feat/support_wds5" ]]; 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
@@ -453,8 +443,10 @@ commands:
           command: yarn versions
       - unless:
           condition:
-            # stop-only does not correctly match on windows: https://github.com/bahmutov/stop-only/issues/78
-            equal: [ *windows-executor, << parameters.executor >> ]
+            or:
+              # stop-only does not correctly match on windows: https://github.com/bahmutov/stop-only/issues/78
+              - equal: [ *windows-executor, << parameters.executor >> ]
+              - equal: [ *darwin-arm64-executor, << parameters.executor >> ]
           steps:
             - run:
                 name: Stop .only

cli/CHANGELOG.md

@@ -7,6 +7,10 @@ _Released 4/23/2024 (PENDING)_
 
 - Added support for `webpack-dev-server` `v5` to `@cypress/webpack-dev-server`. Addresses [#29305](https://github.com/cypress-io/cypress/issues/29305).
 
+**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).
+
 **Misc:**
 
 - We now capture the [Semaphore](https://semaphoreci.com/) CI provider's environment variable [`SEMAPHORE_GIT_PR_NUMBER`](https://docs.semaphoreci.com/ci-cd-environment/environment-variables/#semaphore_git_pr_number) to display the linked PR number in the Cloud. Addressed in [#29314](https://github.com/cypress-io/cypress/pull/29314).

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

@@ -666,4 +666,22 @@ 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 })
+  })
 })

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 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: Set<string> = new Set()
   sweepIntervalTimer: NodeJS.Timeout
   protocolManager?: ProtocolManagerShape
 
@@ -144,6 +145,8 @@ export class PreRequests {
           debugVerbose('timed out unmatched pre-request: %o', browserPreRequest)
           metrics.unmatchedPreRequests++
 
+          this.pendingPreRequestsToRemove.delete(browserPreRequest.requestId)
+
           return false
         }
 
@@ -156,9 +159,23 @@ export class PreRequests {
     }, 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 +242,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.add(requestId)
+    } else {
+      debugVerbose('Removed pre-request %s', requestId)
+    }
+
+    return removed
   }
 
   get (req: CypressIncomingRequest, ctxDebug, callback: GetPreRequestCb) {