fix(server): retry cloud requests on HTTP 500

cli/CHANGELOG.md

@@ -9,6 +9,7 @@
 
 - Fixed an issue where multi-origin tests using [`cy.origin`](https://docs.cypress.io/api/commands/origin) could fail to talk to a secondary origin after test isolation, when the spec-bridge iframe was already present, or when more than one secondary origin became ready around the same time. Cached spec-bridge window targets are now cleared at the correct lifecycle points, improving performance of specs with cy.origin calls. Addressed in [#33704](https://github.com/cypress-io/cypress/pull/33704).
 - Fixed an issue where a CSS selector built internally from element attributes could throw an uncaught `Syntax error, unrecognized expression` and crash the runner when an attribute value contained CSS-special characters (for example, an `<input>` with a `pattern` attribute containing regex metacharacters). Fixes [#26967](https://github.com/cypress-io/cypress/issues/26967) and [#29345](https://github.com/cypress-io/cypress/issues/29345).
+- Fixed an issue where transient HTTP 500 responses from Cypress Cloud were not retried for idempotent requests. Fixed in [#33718](https://github.com/cypress-io/cypress/pull/33718).
 
 **Misc:**
 

packages/server/lib/cloud/api/cy-prompt/get_cy_prompt_bundle.ts

@@ -69,7 +69,7 @@ export const getCyPromptBundle = async ({ cyPromptUrl, projectId, bundlePath }:
   }, {
     maxAttempts: 3,
     retryDelay: _delay,
-    shouldRetry: isRetryableError,
+    shouldRetry: (err) => isRetryableError(err, 'GET'),
   }))()
 
   if (!responseSignature) {

packages/server/lib/cloud/api/cy-prompt/post_cy_prompt_session.ts

@@ -26,6 +26,6 @@ export const postCyPromptSession = async ({ projectId }: PostCyPromptSessionOpti
   }, {
     maxAttempts: 3,
     retryDelay: _delay,
-    shouldRetry: isRetryableError,
+    shouldRetry: (err) => isRetryableError(err, 'POST'),
   }))()
 }

packages/server/lib/cloud/api/put_protocol_artifact.ts

@@ -38,6 +38,6 @@ export const putProtocolArtifact = asyncRetry(
   }, {
     maxAttempts: 3,
     retryDelay: _delay,
-    shouldRetry: isRetryableError,
+    shouldRetry: (err) => isRetryableError(err, 'PUT'),
   },
 )

packages/server/lib/cloud/api/studio/get_studio_bundle.ts

@@ -99,7 +99,7 @@ export const getStudioBundle = async ({ studioUrl, bundlePath }: { studioUrl: st
   }, {
     maxAttempts: 3,
     retryDelay: _delay,
-    shouldRetry: isRetryableError,
+    shouldRetry: (err) => isRetryableError(err, 'GET'),
   }))()
 
   if (!responseSignature) {

packages/server/lib/cloud/api/studio/post_studio_session.ts

@@ -26,6 +26,6 @@ export const postStudioSession = async ({ projectId }: PostStudioSessionOptions)
   }, {
     maxAttempts: 3,
     retryDelay: _delay,
-    shouldRetry: isRetryableError,
+    shouldRetry: (err) => isRetryableError(err, 'POST'),
   }))()
 }

packages/server/lib/cloud/network/is_retryable_error.ts

@@ -5,11 +5,25 @@ import { isNonRetriableCertErrorCode } from './non_retriable_cert_error_codes'
 
 const debug = Debug('cypress-verbose:server:is-retryable-error')
 
-export const isRetryableError = (error: any) => {
-  debug('is retryable error? system error: %s, httperror: %s, status: %d',
+// Per RFC 9110 §9.2.2, PUT, DELETE, and safe methods (GET, HEAD, OPTIONS, TRACE
+// from §9.2.1) are idempotent. TRACE is omitted here because it is not used by
+// any cloud API caller and is commonly disabled for security reasons.
+// https://www.rfc-editor.org/rfc/rfc9110#section-9.2.2
+const IDEMPOTENT_METHODS = new Set(['GET', 'HEAD', 'PUT', 'DELETE', 'OPTIONS'])
+
+const ALWAYS_RETRYABLE_STATUSES = [408, 429, 502, 503, 504]
+
+// Additional statuses that are only safe to retry on idempotent methods —
+// a server-side error mid-request could otherwise replay a non-idempotent
+// side effect (e.g., a POST that partially applied before failing).
+const IDEMPOTENT_RETRYABLE_STATUSES = [500]
+
+export const isRetryableError = (error: any, method?: string) => {
+  debug('is retryable error? system error: %s, httperror: %s, status: %d, method: %s',
     error && SystemError.isSystemError(error as any),
     error && HttpError.isHttpError(error as any),
-    (error as HttpError)?.status)
+    (error as HttpError)?.status,
+    method)
 
   if (SystemError.isSystemError(error)) {
     if (error.code && isNonRetriableCertErrorCode(error.code)) {
@@ -20,7 +34,15 @@ export const isRetryableError = (error: any) => {
   }
 
   if (HttpError.isHttpError(error)) {
-    return [408, 429, 502, 503, 504].includes(error.status)
+    if (ALWAYS_RETRYABLE_STATUSES.includes(error.status)) {
+      return true
+    }
+
+    if (method && IDEMPOTENT_METHODS.has(method.toUpperCase()) && IDEMPOTENT_RETRYABLE_STATUSES.includes(error.status)) {
+      return true
+    }
+
+    return false
   }
 
   return false

packages/server/test/unit/cloud/api/put_protocol_artifact_spec.ts

@@ -10,7 +10,6 @@ import { StreamActivityMonitor } from '../../../../lib/cloud/upload/stream_activ
 import { HttpError } from '../../../../lib/cloud/network/http_error'
 import { putFetch, ParseKinds } from '../../../../lib/cloud/network/fetch'
 import { linearDelay, asyncRetry } from '../../../../lib/util/async_retry'
-import { isRetryableError } from '../../../../lib/cloud/network/is_retryable_error'
 import type { putProtocolArtifact } from '../../../../lib/cloud/api/put_protocol_artifact'
 
 chai.use(chaiAsPromised).use(sinonChai)
@@ -129,8 +128,32 @@ describe('putProtocolArtifact', () => {
 
     expect(options.maxAttempts).to.eq(3)
     expect(options.retryDelay).to.be.a('function')
-    // because of mockery, the isRetryableError ref here is different than the one imported into put_protocol_artifact_spec
-    expect(options.shouldRetry?.toString()).to.eq(isRetryableError.toString())
+  })
+
+  describe('shouldRetry', () => {
+    let shouldRetry: (err: any) => boolean
+
+    beforeEach(() => {
+      shouldRetry = asyncRetryStub.firstCall.args[1].shouldRetry!
+    })
+
+    // The PUT method is idempotent, so isRetryableError also returns true
+    // for HTTP 500 in addition to the always-retryable statuses.
+    it('retries on HTTP 500 in addition to the always-retryable statuses', () => {
+      const retryableStatuses = [408, 429, 500, 502, 503, 504]
+
+      retryableStatuses.forEach((status) => {
+        const err = new HttpError('some error', 'http://some/url', status, 'status text', '', sinon.createStubInstance(Response))
+
+        expect(shouldRetry(err), `status ${status}`).to.be.true
+      })
+    })
+
+    it('does not retry on non-retryable HTTP errors', () => {
+      const err = new HttpError('some error', 'http://some/url', 400, 'Bad Request', '', sinon.createStubInstance(Response))
+
+      expect(shouldRetry(err)).to.be.false
+    })
   })
 
   describe('when provided an artifact path that does not exist', () => {

packages/server/test/unit/cloud/network/is_retryable_error_spec.ts

@@ -46,4 +46,39 @@ describe('isRetryableError', () => {
   it('returns false for other errors', () => {
     expect(isRetryableError(new Error())).to.be.false
   })
+
+  describe('with HTTP method', () => {
+    const httpError = (status: number) => new HttpError('some error', url, status, 'status text', '', sinon.createStubInstance(Response))
+
+    it('also retries 500 for idempotent methods', () => {
+      ['GET', 'HEAD', 'PUT', 'DELETE', 'OPTIONS'].forEach((method) => {
+        expect(isRetryableError(httpError(500), method), `${method} 500`).to.be.true
+      })
+    })
+
+    it('is case-insensitive for the method argument', () => {
+      expect(isRetryableError(httpError(500), 'put')).to.be.true
+      expect(isRetryableError(httpError(500), 'Get')).to.be.true
+    })
+
+    it('does not retry 500 for non-idempotent methods', () => {
+      ['POST', 'PATCH'].forEach((method) => {
+        expect(isRetryableError(httpError(500), method), `${method} 500`).to.be.false
+      })
+    })
+
+    it('still retries the always-retryable statuses regardless of method', () => {
+      ['GET', 'POST', 'PATCH', 'DELETE'].forEach((method) => {
+        [408, 429, 502, 503, 504].forEach((status) => {
+          expect(isRetryableError(httpError(status), method), `${method} ${status}`).to.be.true
+        })
+      })
+    })
+
+    it('does not retry other non-retryable statuses for idempotent methods', () => {
+      [400, 404, 501].forEach((status) => {
+        expect(isRetryableError(httpError(status), 'GET'), `GET ${status}`).to.be.false
+      })
+    })
+  })
 })

system-tests/__snapshots__/record_spec.js

@@ -3420,7 +3420,7 @@ exports['e2e record capture-protocol enabled passing retrieves the capture proto
 
 `
 
-exports['capture-protocol api errors upload 500 - does not retry continues 1'] = `
+exports['capture-protocol api errors upload 500 - tries 3 times and fails continues 1'] = `
 
 ====================================================================================================
 
@@ -3485,13 +3485,13 @@ exports['capture-protocol api errors upload 500 - does not retry continues 1'] =
   - Screenshot - Done Uploading 1 kB in Xm, Ys ZZ.ZZms 1/2 /XXX/XXX/XXX/cypress/screenshots/record_pass.cy.js/yay it passes.png
   - Test Replay - Failed Uploading after Xm, Ys ZZ.ZZms 2/2 - http://localhost:1234/capture-protocol/upload/?x-amz-credential=XXXXXXXX&x-amz-signature=XXXXXXXXXXXXX responded with 500 Internal Server Error
 
-Warning: We encountered an HTTP error while uploading the Test Replay recording for this spec.
-
-These results will not display Test Replay recordings.
+Warning: We encountered multiple errors while uploading the Test Replay recording for this spec.
 
-This error will not affect or change the exit code.
+We attempted to upload the Test Replay recording 3 times.
 
-http://localhost:1234/capture-protocol/upload/?x-amz-credential=XXXXXXXX&x-amz-signature=XXXXXXXXXXXXX responded with HTTP 500: Internal Server Error
+http://localhost:1234/capture-protocol/upload/?x-amz-credential=XXXXXXXX&x-amz-signature=XXXXXXXXXXXXX responded with 500 Internal Server Error
+http://localhost:1234/capture-protocol/upload/?x-amz-credential=XXXXXXXX&x-amz-signature=XXXXXXXXXXXXX responded with 500 Internal Server Error
+http://localhost:1234/capture-protocol/upload/?x-amz-credential=XXXXXXXX&x-amz-signature=XXXXXXXXXXXXX responded with 500 Internal Server Error
 
 ====================================================================================================
 

system-tests/test/record_spec.js

@@ -3009,7 +3009,7 @@ describe('capture-protocol api errors', () => {
     }))
   }
 
-  describe('upload 500 - does not retry', () => {
+  describe('upload 500 - tries 3 times and fails', () => {
     stubbedServerWithErrorOn('putCaptureProtocolUpload')
     it('continues', function () {
       process.env.API_RETRY_INTERVALS = '1000'
@@ -3028,9 +3028,13 @@ describe('capture-protocol api errors', () => {
         const artifactReport = getRequests().find(({ url }) => url === `PUT /instances/${instanceId}/artifacts`)?.body
 
         expect(artifactReport?.protocol).to.exist
-        expect(artifactReport?.protocol?.error).to.equal(
-          'Failed to upload Test Replay: http://localhost:1234/capture-protocol/upload/?x-amz-credential=XXXXXXXX&x-amz-signature=XXXXXXXXXXXXX responded with 500 Internal Server Error',
-        )
+
+        const expectedUrl = `http://localhost:1234/capture-protocol/upload/?x-amz-credential=XXXXXXXX&x-amz-signature=XXXXXXXXXXXXX`
+        const expectedErrorMessage = `${expectedUrl} responded with 500 Internal Server Error`
+
+        expect(artifactReport?.protocol?.error).to.equal(`Failed to upload Test Replay after 3 attempts. Errors: ${[expectedErrorMessage, expectedErrorMessage, expectedErrorMessage].join(', ')}`)
+        expect(artifactReport?.protocol?.errorStack).to.exist
+        expect(artifactReport?.protocol?.errorStack).to.not.be.empty
       })
     })
   })