Retry all 5xx server errors (MAGE-694) (#485)

* Retry all 5xx server errors (MAGE-694)

Widen the retryable classification from {429,500,502,503,504} to 429 plus the
full 5xx range (500-599). This covers transient CDN/edge failures such as
Cloudflare's 520-527 codes, which were observed during the
cannot-access-klaviyo-com incident and had zero overlap with the old allowlist,
causing data loss for the duration of the outage. 4xx codes (including 403
load-shed) remain non-retryable so the backend can still shed load.

Adds tests for Cloudflare 522/525 and the 599 upper bound.

🤖 Generated with Reca

* refine(analytics): exclude 501/505 from retryable 5xx + add bounds tests (MAGE-694)

501 (Not Implemented) and 505 (HTTP Version Not Supported) are permanent,
deterministic server errors — retrying the identical request always fails the
same way — so classify them as non-retryable (Failed) like a 4xx, via a
501, 505 -> branch placed before the in 500..599 retryable branch. The rest of
5xx (429 + 500..599 except 501/505) stays retryable.

Adds tests: 501/505 non-retryable, plus 499 (lower-bound) and 600 (upper-bound)
sanity checks that just-outside-5xx codes remain non-retryable.

-Claude

* refactor(analytics): name 5xx status constants + dedup boundary tests (CodeRabbit)

Extract named constants for the previously-inlined HTTP status literals in
parseResponse's when(responseCode): HTTP_NOT_IMPLEMENTED (501),
HTTP_VERSION_NOT_SUPPORTED (505), and the HTTP_5XX_RETRYABLE_RANGE (500..599)
range, placed alongside the existing HTTP_RETRY const. Behavior is identical.

De-dup the single-send status-code tests behind a shared
assertStatusForResponseCode(responseCode, expected) helper, removing the
repeated mock/send/assert boilerplate while keeping the distinct, readable
per-code test methods and their explanatory comments. Coverage is unchanged.

-Claude

* revert(analytics): retry entire 5xx range incl 501/505 (drop permanent-error exclusion) (MAGE-694)

An earlier commit excluded 501 (Not Implemented) and 505 (HTTP Version Not
Supported) as "permanent" server errors. Reverting that: for the SDK's fixed
request shapes over the HTTP versions Klaviyo supports, a genuine origin 501/505
is effectively unreachable, so any 5xx we actually observe is almost certainly
edge/CDN/proxy noise during an incident — exactly what MAGE-694 wants to retry.
The incident data-retention benefit outweighs the negligible wasted-retry cost
on codes we will never legitimately see.

- Drop the 501/505 -> Status.Failed when-branch so they fall through to the
  retryable HTTP_5XX_RETRYABLE_RANGE branch like the rest of 500–599.
- Remove the now-unused HTTP_NOT_IMPLEMENTED / HTTP_VERSION_NOT_SUPPORTED
  constants; keep HTTP_5XX_RETRYABLE_RANGE and HTTP_RETRY.
- Flip the 501/505 tests to assert PendingRetry and document the deliberate
  retry. 499/600 bounds tests remain non-retryable. 4xx (incl. 403) unchanged.

-Claude

---------

Co-authored-by: Reca <reca@klaviyo.com>

Author: evan-masseau

sdk/analytics/src/main/java/com/klaviyo/analytics/networking/requests/KlaviyoApiRequest.kt

@@ -69,6 +69,7 @@ internal open class KlaviyoApiRequest(
         const val HTTP_MULT_CHOICE = HttpURLConnection.HTTP_MULT_CHOICE
         const val HTTP_RETRY = 429 // oddly not a const in HttpURLConnection
         const val HTTP_BAD_REQUEST = HttpURLConnection.HTTP_BAD_REQUEST
+        val HTTP_5XX_RETRYABLE_RANGE = 500..599
 
         // JSON keys for persistence
         const val TYPE_JSON_KEY = "request_type"
@@ -432,8 +433,16 @@ internal open class KlaviyoApiRequest(
 
         status = when (responseCode) {
             in successCodes -> Status.Complete
-            // 429 rate limit, 500, 502, 503 and 504 are all treated as retryable
-            HTTP_RETRY, 500, in 502..504 -> {
+            // 429 rate limit plus the entire 5xx range (500–599) are treated as retryable.
+            // Widening from the legacy {500,502,503,504} allowlist to the full range covers
+            // transient CDN/edge failures such as Cloudflare's 520–527 codes, which were the
+            // codes observed during the cannot-access-klaviyo-com incident. We deliberately
+            // retry even 501 (Not Implemented) and 505 (HTTP Version Not Supported): for the
+            // SDK's fixed request shapes a genuine origin 501/505 is effectively unreachable,
+            // so any 5xx we actually observe is almost certainly edge/proxy noise during an
+            // incident — exactly what we want to retry. 4xx codes (including 403 load-shed)
+            // remain non-retryable so the backend can shed load.
+            HTTP_RETRY, in HTTP_5XX_RETRYABLE_RANGE -> {
                 if (attempts < maxAttempts) {
                     Status.PendingRetry
                 } else {

sdk/analytics/src/test/java/com/klaviyo/analytics/networking/requests/KlaviyoApiRequestTest.kt

@@ -75,6 +75,20 @@ internal class KlaviyoApiRequestTest : BaseApiRequestTest<KlaviyoApiRequest>() {
         return connectionSpy
     }
 
+    /**
+     * Sends a single request against a connection stubbed to return [responseCode] and asserts the
+     * resulting [KlaviyoApiRequest.Status] and that exactly one attempt was made. Shared by the
+     * single-send status-code tests to keep their boilerplate DRY.
+     */
+    private fun assertStatusForResponseCode(responseCode: Int, expected: KlaviyoApiRequest.Status) {
+        val connectionMock = withConnectionMock(URL(expectedFullUrl))
+        every { connectionMock.responseCode } returns responseCode
+
+        val request = makeTestRequest()
+        assertEquals(expected, request.send())
+        assertEquals(1, request.attempts)
+    }
+
     @Before
     override fun setup() {
         super.setup()
@@ -722,42 +736,68 @@ internal class KlaviyoApiRequestTest : BaseApiRequestTest<KlaviyoApiRequest>() {
 
     @Test
     fun `500 response code returns PendingRetry when under max attempts`() {
-        val connectionMock = withConnectionMock(URL(expectedFullUrl))
-        every { connectionMock.responseCode } returns 500
-
-        val request = makeTestRequest()
-        assertEquals(KlaviyoApiRequest.Status.PendingRetry, request.send())
-        assertEquals(1, request.attempts)
+        assertStatusForResponseCode(500, KlaviyoApiRequest.Status.PendingRetry)
     }
 
     @Test
     fun `502 response code returns PendingRetry when under max attempts`() {
-        val connectionMock = withConnectionMock(URL(expectedFullUrl))
-        every { connectionMock.responseCode } returns 502
-
-        val request = makeTestRequest()
-        assertEquals(KlaviyoApiRequest.Status.PendingRetry, request.send())
-        assertEquals(1, request.attempts)
+        assertStatusForResponseCode(502, KlaviyoApiRequest.Status.PendingRetry)
     }
 
     @Test
     fun `503 response code returns PendingRetry when under max attempts`() {
-        val connectionMock = withConnectionMock(URL(expectedFullUrl))
-        every { connectionMock.responseCode } returns 503
-
-        val request = makeTestRequest()
-        assertEquals(KlaviyoApiRequest.Status.PendingRetry, request.send())
-        assertEquals(1, request.attempts)
+        assertStatusForResponseCode(503, KlaviyoApiRequest.Status.PendingRetry)
     }
 
     @Test
     fun `504 response code returns PendingRetry when under max attempts`() {
-        val connectionMock = withConnectionMock(URL(expectedFullUrl))
-        every { connectionMock.responseCode } returns 504
+        assertStatusForResponseCode(504, KlaviyoApiRequest.Status.PendingRetry)
+    }
 
-        val request = makeTestRequest()
-        assertEquals(KlaviyoApiRequest.Status.PendingRetry, request.send())
-        assertEquals(1, request.attempts)
+    @Test
+    fun `522 Cloudflare response code returns PendingRetry when under max attempts`() {
+        // 522 (connection timed out) is a Cloudflare edge code outside the legacy allowlist
+        // and was one of the codes observed during the cannot-access-klaviyo-com incident.
+        assertStatusForResponseCode(522, KlaviyoApiRequest.Status.PendingRetry)
+    }
+
+    @Test
+    fun `525 Cloudflare response code returns PendingRetry when under max attempts`() {
+        assertStatusForResponseCode(525, KlaviyoApiRequest.Status.PendingRetry)
+    }
+
+    @Test
+    fun `599 upper-bound 5xx response code returns PendingRetry when under max attempts`() {
+        assertStatusForResponseCode(599, KlaviyoApiRequest.Status.PendingRetry)
+    }
+
+    @Test
+    fun `501 response code returns PendingRetry when under max attempts`() {
+        // 501 (Not Implemented) is inside the full 5xx retryable range and we deliberately
+        // retry it: for the SDK's fixed request shapes a genuine origin 501 is effectively
+        // unreachable, so an observed 501 is almost certainly edge/proxy noise worth retrying.
+        assertStatusForResponseCode(501, KlaviyoApiRequest.Status.PendingRetry)
+    }
+
+    @Test
+    fun `505 response code returns PendingRetry when under max attempts`() {
+        // 505 (HTTP Version Not Supported) is inside the full 5xx retryable range and we
+        // deliberately retry it: for the SDK's fixed request shapes a genuine origin 505 is
+        // effectively unreachable, so an observed 505 is almost certainly edge/proxy noise
+        // worth retrying.
+        assertStatusForResponseCode(505, KlaviyoApiRequest.Status.PendingRetry)
+    }
+
+    @Test
+    fun `499 response code returns Failed immediately`() {
+        // Lower-bound sanity: 499 sits just below the 5xx range and must stay non-retryable.
+        assertStatusForResponseCode(499, KlaviyoApiRequest.Status.Failed)
+    }
+
+    @Test
+    fun `600 response code returns Failed immediately`() {
+        // Upper-bound sanity: 600 sits just above the 5xx range and must stay non-retryable.
+        assertStatusForResponseCode(600, KlaviyoApiRequest.Status.Failed)
     }
 
     @Test
@@ -778,41 +818,21 @@ internal class KlaviyoApiRequestTest : BaseApiRequestTest<KlaviyoApiRequest>() {
 
     @Test
     fun `400 response code returns Failed immediately`() {
-        val connectionMock = withConnectionMock(URL(expectedFullUrl))
-        every { connectionMock.responseCode } returns 400
-
-        val request = makeTestRequest()
-        assertEquals(KlaviyoApiRequest.Status.Failed, request.send())
-        assertEquals(1, request.attempts)
+        assertStatusForResponseCode(400, KlaviyoApiRequest.Status.Failed)
     }
 
     @Test
     fun `401 response code returns Failed immediately`() {
-        val connectionMock = withConnectionMock(URL(expectedFullUrl))
-        every { connectionMock.responseCode } returns 401
-
-        val request = makeTestRequest()
-        assertEquals(KlaviyoApiRequest.Status.Failed, request.send())
-        assertEquals(1, request.attempts)
+        assertStatusForResponseCode(401, KlaviyoApiRequest.Status.Failed)
     }
 
     @Test
     fun `403 response code returns Failed immediately`() {
-        val connectionMock = withConnectionMock(URL(expectedFullUrl))
-        every { connectionMock.responseCode } returns 403
-
-        val request = makeTestRequest()
-        assertEquals(KlaviyoApiRequest.Status.Failed, request.send())
-        assertEquals(1, request.attempts)
+        assertStatusForResponseCode(403, KlaviyoApiRequest.Status.Failed)
     }
 
     @Test
     fun `404 response code returns Failed immediately`() {
-        val connectionMock = withConnectionMock(URL(expectedFullUrl))
-        every { connectionMock.responseCode } returns 404
-
-        val request = makeTestRequest()
-        assertEquals(KlaviyoApiRequest.Status.Failed, request.send())
-        assertEquals(1, request.attempts)
+        assertStatusForResponseCode(404, KlaviyoApiRequest.Status.Failed)
     }
 }