fix: correlate prerequests in order instead of reverse order (#27892)

cli/CHANGELOG.md

@@ -1,4 +1,12 @@
 <!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->
+## 13.3.1
+
+_Released 10/03/2023 (PENDING)_
+
+**Bugfixes:**
+
+- Fixed an issue where requests were correlated in the wrong order in the proxy. This could cause an issue where the wrong request is used for `cy.intercept` or assets (e.g. stylesheets or images) may not properly be available in Test Replay. Addressed in [#27892](https://github.com/cypress-io/cypress/pull/27892).
+
 ## 13.3.0
 
 _Released 09/27/2023_

packages/proxy/lib/http/index.ts

@@ -331,12 +331,14 @@ export class Http {
 
     const onError = (error: Error): Promise<void> => {
       ctx.error = error
-      if (ctx.req.browserPreRequest) {
+      if (ctx.req.browserPreRequest && !ctx.req.browserPreRequest.errorHandled) {
+        ctx.req.browserPreRequest.errorHandled = true
         // browsers will retry requests in the event of network errors, but they will not send pre-requests,
         // so try to re-use the current browserPreRequest for the next retry after incrementing the ID.
         const preRequest = {
           ...ctx.req.browserPreRequest,
           requestId: getUniqueRequestId(ctx.req.browserPreRequest.requestId),
+          errorHandled: false,
         }
 
         ctx.debug('Re-using pre-request data %o', preRequest)

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

@@ -33,40 +33,40 @@ type PendingPreRequest = {
 }
 
 /**
- * Data structure that organizes items with duplicated keys into stacks.
+ * Data structure that organizes items with duplicated keys into queues.
  */
-class StackMap<T> {
-  private stacks: Record<string, Array<T>> = {}
-  push (stackKey: string, value: T) {
-    if (!this.stacks[stackKey]) this.stacks[stackKey] = []
+class QueueMap<T> {
+  private queues: Record<string, Array<T>> = {}
+  push (queueKey: string, value: T) {
+    if (!this.queues[queueKey]) this.queues[queueKey] = []
 
-    this.stacks[stackKey].push(value)
+    this.queues[queueKey].push(value)
   }
-  pop (stackKey: string): T | undefined {
-    const stack = this.stacks[stackKey]
+  shift (queueKey: string): T | undefined {
+    const queue = this.queues[queueKey]
 
-    if (!stack) return
+    if (!queue) return
 
-    const item = stack.pop()
+    const item = queue.shift()
 
-    if (stack.length === 0) delete this.stacks[stackKey]
+    if (queue.length === 0) delete this.queues[queueKey]
 
     return item
   }
   removeMatching (filterFn: (value: T) => boolean) {
-    Object.entries(this.stacks).forEach(([stackKey, stack]) => {
-      this.stacks[stackKey] = stack.filter(filterFn)
-      if (this.stacks[stackKey].length === 0) delete this.stacks[stackKey]
+    Object.entries(this.queues).forEach(([queueKey, queue]) => {
+      this.queues[queueKey] = queue.filter(filterFn)
+      if (this.queues[queueKey].length === 0) delete this.queues[queueKey]
     })
   }
-  removeExact (stackKey: string, value: T) {
-    const i = this.stacks[stackKey].findIndex((v) => v === value)
+  removeExact (queueKey: string, value: T) {
+    const i = this.queues[queueKey].findIndex((v) => v === value)
 
-    this.stacks[stackKey].splice(i, 1)
-    if (this.stacks[stackKey].length === 0) delete this.stacks[stackKey]
+    this.queues[queueKey].splice(i, 1)
+    if (this.queues[queueKey].length === 0) delete this.queues[queueKey]
   }
   get length () {
-    return Object.values(this.stacks).reduce((prev, cur) => prev + cur.length, 0)
+    return Object.values(this.queues).reduce((prev, cur) => prev + cur.length, 0)
   }
 }
 
@@ -79,8 +79,8 @@ class StackMap<T> {
 export class PreRequests {
   requestTimeout: number
   sweepInterval: number
-  pendingPreRequests = new StackMap<PendingPreRequest>()
-  pendingRequests = new StackMap<PendingRequest>()
+  pendingPreRequests = new QueueMap<PendingPreRequest>()
+  pendingRequests = new QueueMap<PendingRequest>()
   sweepIntervalTimer: NodeJS.Timeout
 
   constructor (
@@ -119,7 +119,7 @@ export class PreRequests {
   addPending (browserPreRequest: BrowserPreRequest) {
     metrics.browserPreRequestsReceived++
     const key = `${browserPreRequest.method}-${browserPreRequest.url}`
-    const pendingRequest = this.pendingRequests.pop(key)
+    const pendingRequest = this.pendingRequests.shift(key)
 
     if (pendingRequest) {
       debugVerbose('Incoming pre-request %s matches pending request. %o', key, browserPreRequest)
@@ -145,7 +145,7 @@ export class PreRequests {
   get (req: CypressIncomingRequest, ctxDebug, callback: GetPreRequestCb) {
     metrics.proxyRequestsReceived++
     const key = `${req.method}-${req.proxiedUrl}`
-    const pendingPreRequest = this.pendingPreRequests.pop(key)
+    const pendingPreRequest = this.pendingPreRequests.shift(key)
 
     if (pendingPreRequest) {
       metrics.immediatelyMatchedRequests++

packages/proxy/lib/types.ts

@@ -59,6 +59,7 @@ export type BrowserPreRequest = {
   headers: { [key: string]: string | string[] }
   resourceType: ResourceType
   originalResourceType: string | undefined
+  errorHandled?: boolean
 }
 
 /**

packages/proxy/test/unit/http/index.spec.ts

@@ -79,6 +79,37 @@ describe('http', function () {
       })
     })
 
+    it('ensures not to create fake pending browser pre requests on multiple errors', function () {
+      incomingRequest.callsFake(function () {
+        this.req.browserPreRequest = {
+          errorHandled: true,
+        }
+
+        throw new Error('oops')
+      })
+
+      error.callsFake(function () {
+        expect(this.error.message).to.eq('Internal error while proxying "GET url" in 0:\noops')
+        this.end()
+      })
+
+      const http = new Http(httpOpts)
+
+      http.addPendingBrowserPreRequest = sinon.stub()
+
+      return http
+      // @ts-ignore
+      .handleHttpRequest({ method: 'GET', proxiedUrl: 'url' }, { on, off })
+      .then(function () {
+        expect(incomingRequest).to.be.calledOnce
+        expect(incomingResponse).to.not.be.called
+        expect(http.addPendingBrowserPreRequest).to.not.be.called
+        expect(error).to.be.calledOnce
+        expect(on).to.not.be.called
+        expect(off).to.be.calledThrice
+      })
+    })
+
     it('moves to Error stack if err in IncomingResponse', function () {
       incomingRequest.callsFake(function () {
         this.incomingRes = {}

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

@@ -20,12 +20,12 @@ describe('http/util/prerequests', () => {
   })
 
   it('synchronously matches a pre-request that existed at the time of the request', () => {
-    // should match in reverse order
+    // should match in order
     preRequests.addPending({ requestId: '1234', url: 'foo', method: 'WRONGMETHOD' } as BrowserPreRequest)
-    preRequests.addPending({ requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest)
-    const thirdPreRequest = { requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest
+    const secondPreRequest = { requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest
 
-    preRequests.addPending(thirdPreRequest)
+    preRequests.addPending(secondPreRequest)
+    preRequests.addPending({ requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest)
     expectPendingCounts(0, 3)
 
     const cb = sinon.stub()
@@ -34,7 +34,7 @@ describe('http/util/prerequests', () => {
 
     const { args } = cb.getCall(0)
 
-    expect(args[0]).to.eq(thirdPreRequest)
+    expect(args[0]).to.eq(secondPreRequest)
 
     expectPendingCounts(0, 2)
   })