fix: fix failures and correlation in proxy (#28094)

cli/CHANGELOG.md

@@ -5,6 +5,7 @@ _Released 10/24/2023 (PENDING)_
 
 **Bugfixes:**
 
+- Fixed a performance problem with proxy correlation when requests get aborted and then get miscorrelated with follow up requests. Fixed in [#28094](https://github.com/cypress-io/cypress/pull/28094).
 - Fixed a regression in [10.0.0](#10.0.0), where search would not find a spec if the file name contains "-" or "\_", but search prompt contains " " instead (e.g. search file "spec-file.cy.ts" with prompt "spec file"). Fixes [#25303](https://github.com/cypress-io/cypress/issues/25303).
 
 ## 13.3.2

packages/proxy/lib/http/index.ts

@@ -9,7 +9,7 @@ import ErrorMiddleware from './error-middleware'
 import RequestMiddleware from './request-middleware'
 import ResponseMiddleware from './response-middleware'
 import { HttpBuffers } from './util/buffers'
-import { GetPreRequestCb, PreRequests } from './util/prerequests'
+import { GetPreRequestCb, PendingRequest, PreRequests } from './util/prerequests'
 
 import type EventEmitter from 'events'
 import type CyServer from '@packages/server'
@@ -63,10 +63,12 @@ type HttpMiddlewareCtx<T> = {
   stage: HttpStages
   debug: Debug.Debugger
   middleware: HttpMiddlewareStacks
+  pendingRequest: PendingRequest | undefined
   getCookieJar: () => CookieJar
   deferSourceMapRewrite: (opts: { js: string, url: string }) => string
-  getPreRequest: (cb: GetPreRequestCb) => void
+  getPreRequest: (cb: GetPreRequestCb) => PendingRequest | undefined
   addPendingUrlWithoutPreRequest: (url: string) => void
+  removePendingRequest: (pendingRequest: PendingRequest) => void
   getAUTUrl: Http['getAUTUrl']
   setAUTUrl: Http['setAUTUrl']
   simulatedCookies: SerializableAutomationCookie[]
@@ -325,15 +327,25 @@ export class Http {
       getAUTUrl: this.getAUTUrl,
       setAUTUrl: this.setAUTUrl,
       getPreRequest: (cb) => {
-        this.preRequests.get(ctx.req, ctx.debug, cb)
+        return this.preRequests.get(ctx.req, ctx.debug, cb)
       },
       addPendingUrlWithoutPreRequest: (url) => {
         this.preRequests.addPendingUrlWithoutPreRequest(url)
       },
+      removePendingRequest: (pendingRequest: PendingRequest) => {
+        this.preRequests.removePendingRequest(pendingRequest)
+      },
       protocolManager: this.protocolManager,
     }
 
     const onError = (error: Error): Promise<void> => {
+      const pendingRequest = ctx.pendingRequest as PendingRequest | undefined
+
+      if (pendingRequest) {
+        delete ctx.pendingRequest
+        ctx.removePendingRequest(pendingRequest)
+      }
+
       ctx.error = error
       if (ctx.req.browserPreRequest && !ctx.req.browserPreRequest.errorHandled) {
         ctx.req.browserPreRequest.errorHandled = true
@@ -427,7 +439,7 @@ export class Http {
   }
 
   removePendingBrowserPreRequest (requestId: string) {
-    this.preRequests.removePending(requestId)
+    this.preRequests.removePendingPreRequest(requestId)
   }
 
   addPendingUrlWithoutPreRequest (url: string) {

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

@@ -128,7 +128,7 @@ const CorrelateBrowserPreRequest: RequestMiddleware = async function () {
   }
 
   this.debug('waiting for prerequest')
-  this.getPreRequest((({ browserPreRequest, noPreRequestExpected }) => {
+  this.pendingRequest = this.getPreRequest((({ browserPreRequest, noPreRequestExpected }) => {
     this.req.browserPreRequest = browserPreRequest
     this.req.noPreRequestExpected = noPreRequestExpected
     copyResourceTypeAndNext()
@@ -455,6 +455,13 @@ const SendRequestOutgoing: RequestMiddleware = function () {
     this.debug('request aborted')
     // if the request is aborted, close out the middleware span and http span. the response middleware did not run
 
+    const pendingRequest = this.pendingRequest
+
+    if (pendingRequest) {
+      delete this.pendingRequest
+      this.removePendingRequest(pendingRequest)
+    }
+
     this.reqMiddlewareSpan?.setAttributes({
       requestAborted: true,
     })

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

@@ -28,7 +28,8 @@ export type CorrelationInformation = {
 
 export type GetPreRequestCb = (correlationInformation: CorrelationInformation) => void
 
-type PendingRequest = {
+export type PendingRequest = {
+  key: string
   ctxDebug
   callback?: GetPreRequestCb
   timeout: NodeJS.Timeout
@@ -74,10 +75,12 @@ class QueueMap<T> {
     })
   }
   removeExact (queueKey: string, value: T) {
-    const i = this.queues[queueKey].findIndex((v) => v === value)
+    const i = this.queues[queueKey]?.findIndex((v) => v === value)
 
-    this.queues[queueKey].splice(i, 1)
-    if (this.queues[queueKey].length === 0) delete this.queues[queueKey]
+    if (i > -1) {
+      this.queues[queueKey].splice(i, 1)
+      if (this.queues[queueKey].length === 0) delete this.queues[queueKey]
+    }
   }
 
   forEach (fn: (value: T) => void) {
@@ -210,7 +213,7 @@ export class PreRequests {
     })
   }
 
-  removePending (requestId: string) {
+  removePendingPreRequest (requestId: string) {
     this.pendingPreRequests.removeMatching(({ browserPreRequest }) => {
       return (browserPreRequest.requestId.includes('-retry-') && !browserPreRequest.requestId.startsWith(`${requestId}-`)) || (!browserPreRequest.requestId.includes('-retry-') && browserPreRequest.requestId !== requestId)
     })
@@ -267,6 +270,7 @@ export class PreRequests {
     }
 
     const pendingRequest: PendingRequest = {
+      key,
       ctxDebug,
       callback,
       proxyRequestReceivedTimestamp: performance.now() + performance.timeOrigin,
@@ -283,6 +287,8 @@ export class PreRequests {
     }
 
     this.pendingRequests.push(key, pendingRequest)
+
+    return pendingRequest
   }
 
   setProtocolManager (protocolManager: ProtocolManagerShape) {
@@ -293,6 +299,12 @@ export class PreRequests {
     this.requestTimeout = requestTimeout
   }
 
+  removePendingRequest (pendingRequest: PendingRequest) {
+    this.pendingRequests.removeExact(pendingRequest.key, pendingRequest)
+    clearTimeout(pendingRequest.timeout)
+    delete pendingRequest.callback
+  }
+
   reset () {
     this.pendingPreRequests = new QueueMap<PendingPreRequest>()
 

packages/proxy/test/unit/http/util/prerequests.spec.ts

@@ -207,7 +207,7 @@ describe('http/util/prerequests', () => {
 
     expectPendingCounts(0, 3)
 
-    preRequests.removePending('1235')
+    preRequests.removePendingPreRequest('1235')
 
     expectPendingCounts(0, 2)
   })
@@ -222,7 +222,7 @@ describe('http/util/prerequests', () => {
 
     expectPendingCounts(0, 6)
 
-    preRequests.removePending('1235')
+    preRequests.removePendingPreRequest('1235')
 
     expectPendingCounts(0, 2)
   })
@@ -236,6 +236,27 @@ describe('http/util/prerequests', () => {
     expect(cbServiceWorker).to.be.calledWith()
   })
 
+  it('removes a pending request', () => {
+    const cb = sinon.stub()
+
+    const firstPreRequest = preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, cb)
+    const secondPreRequest = preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, cb)
+
+    expectPendingCounts(2, 0)
+
+    preRequests.removePendingRequest(firstPreRequest!)
+
+    expectPendingCounts(1, 0)
+
+    preRequests.removePendingRequest(firstPreRequest!)
+
+    expectPendingCounts(1, 0)
+
+    preRequests.removePendingRequest(secondPreRequest!)
+
+    expectPendingCounts(0, 0)
+  })
+
   it('resets the queues', () => {
     let callbackCalled = false
 

packages/server/test/integration/http_requests_spec.js

@@ -94,7 +94,7 @@ describe('Routes', () => {
 
     Fixtures.scaffold()
 
-    this.setup = async (initialUrl, obj = {}, spec) => {
+    this.setup = async (initialUrl, obj = {}, spec, shouldCorrelatePreRequests = false) => {
       if (_.isObject(initialUrl)) {
         obj = initialUrl
         initialUrl = null
@@ -170,6 +170,7 @@ describe('Routes', () => {
                 createRoutes,
                 testingType: 'e2e',
                 exit: false,
+                shouldCorrelatePreRequests: () => shouldCorrelatePreRequests,
               })
               .spread(async (port) => {
                 const automationStub = {
@@ -187,6 +188,8 @@ describe('Routes', () => {
                 this.session = session(this.srv)
 
                 this.proxy = `http://localhost:${port}`
+
+                this.networkProxy = this.server._networkProxy
               }),
             ])
           })
@@ -983,6 +986,108 @@ describe('Routes', () => {
       })
     })
 
+    context('basic request with correlation', () => {
+      beforeEach(function () {
+        return this.setup('http://www.github.com', undefined, undefined, true)
+      })
+
+      it('properly correlates when CDP failures come first', function () {
+        this.timeout(1500)
+
+        nock(this.server.remoteStates.current().origin)
+        .get('/')
+        .reply(200, 'hello from bar!', {
+          'Content-Type': 'text/html',
+        })
+
+        this.networkProxy.addPendingBrowserPreRequest({
+          requestId: '1',
+          method: 'GET',
+          url: 'http://www.github.com/',
+        })
+
+        this.networkProxy.removePendingBrowserPreRequest({
+          requestId: '1',
+        })
+
+        const requestPromise = this.rp({
+          url: 'http://www.github.com/',
+          headers: {
+            'Cookie': '__cypress.initial=true',
+            'Accept-Encoding': 'identity',
+          },
+        })
+
+        this.networkProxy.addPendingBrowserPreRequest({
+          requestId: '1',
+          method: 'GET',
+          url: 'http://www.github.com/',
+        })
+
+        return requestPromise.then((res) => {
+          expect(res.statusCode).to.eq(200)
+
+          expect(res.body).to.include('hello from bar!')
+        })
+      })
+
+      it('properly correlates when proxy failure come first', function () {
+        this.networkProxy.setPreRequestTimeout(50)
+        // If this takes longer than the Promise.delay and the prerequest timeout then the second
+        // call has hit the prerequest timeout which is a problem
+        this.timeout(150)
+
+        nock(this.server.remoteStates.current().origin)
+        .get('/')
+        .delay(50)
+        .once()
+        .reply(200, 'hello from bar!', {
+          'Content-Type': 'text/html',
+        })
+
+        this.rp({
+          url: 'http://www.github.com/',
+          headers: {
+            'Cookie': '__cypress.initial=true',
+            'Accept-Encoding': 'identity',
+          },
+          // Timeout needs to be less than the prerequest timeout + the nock delay
+          timeout: 75,
+        }).catch(() => {})
+
+        // Wait 100 ms to make sure the request times out
+        return Promise.delay(100).then(() => {
+          nock(this.server.remoteStates.current().origin)
+          .get('/')
+          .once()
+          .reply(200, 'hello from baz!', {
+            'Content-Type': 'text/html',
+          })
+
+          // This should not immediately correlate. If it does, then the next request will timeout
+          this.networkProxy.addPendingBrowserPreRequest({
+            requestId: '1',
+            method: 'GET',
+            url: 'http://www.github.com/',
+          })
+
+          const followupRequestPromise = this.rp({
+            url: 'http://www.github.com/',
+            headers: {
+              'Cookie': '__cypress.initial=true',
+              'Accept-Encoding': 'identity',
+            },
+          })
+
+          return followupRequestPromise.then((res) => {
+            expect(res.statusCode).to.eq(200)
+
+            expect(res.body).to.include('hello from baz!')
+          })
+        })
+      })
+    })
+
     context('gzip', () => {
       beforeEach(function () {
         return this.setup('http://www.github.com')