fix: screen numeric Retry-After against a strict decimal grammar

OmarAlJarrah · OmarAlJarrah · commit 79494a797668 · 2026-06-16T22:22:51.000+03:00
The fractional Retry-After support switched the numeric branch to
toDoubleOrNull(), which accepts the Java floating-point literal grammar:
"30d" parses to 30.0 and "0x1p4" to 16.0. A header like `Retry-After: 30d`
would therefore silently honour a 30-second delta instead of falling through
to the backoff schedule.

Screen the value against ^\d+(\.\d+)?$ before parsing so only the RFC 7231
delta-seconds form and the fractional extension real servers emit are honoured;
anything else (type suffixes, hex-float, signs, exponents) falls through to the
HTTP-date branch and ultimately the backoff schedule. Add parser tests for the
rejected forms.

Expand the ServerOverrideRetryPredicate tests to cover the full truthy/falsy
token matrix and the unrecognised-token defer branch, and add a case proving a
forced retry still does not re-send a non-replayable body. Clarify the
predicate KDoc that the override flips classification only and does not bypass
the attempt cap or the replayability gate.
diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/ServerOverrideRetryPredicate.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/ServerOverrideRetryPredicate.kt
@@ -27,6 +27,14 @@ import java.util.Locale
  *    the decision is delegated to [delegate] — so wiring this predicate in changes behaviour
  *    only when the server actually speaks.
  *
+ * ## What the override does and does not bypass
+ *
+ * The override flips only the *classification* decision — "is this response retryable?". It does
+ * not bypass the other gates the retry step enforces: a forced retry is still capped by
+ * [HttpRetryOptions.maxRetries], and still requires a retry-safe request (an idempotent method or
+ * a replayable body). A truthy header on a `POST` carrying a non-replayable body therefore does
+ * **not** trigger a retry — the body cannot be re-sent, so the response is returned as-is.
+ *
  * ## Opt-in
  *
  * This predicate is **not** installed by default. A caller enables it by passing an instance as
diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryAfterParser.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryAfterParser.kt
@@ -114,6 +114,19 @@ public object RetryAfterParser {
     /** Nanoseconds per second — the scale factor for the fractional `Retry-After` conversion. */
     private const val NANOS_PER_SECOND: Double = 1_000_000_000.0
 
+    /**
+     * Strict grammar for the numeric `Retry-After` delta: one or more decimal digits, optionally
+     * followed by a single decimal point and one or more decimal digits.
+     *
+     * Screening with this regex before [String.toDoubleOrNull] is deliberate: `toDoubleOrNull`
+     * accepts the Java floating-point literal grammar, which includes the type suffixes (`d`,
+     * `f`) and hexadecimal-float forms (`0x1p4`). Without the screen a header such as
+     * `Retry-After: 30d` or `Retry-After: 0x1p4` would parse to a finite delta instead of falling
+     * through to the backoff schedule. Only the RFC 7231 §7.1.3 delta-seconds form and the
+     * fractional extension real servers emit are honoured here.
+     */
+    private val NUMERIC_SECONDS = Regex("""^\d+(\.\d+)?$""")
+
     /**
      * Parses the next-attempt delay from [headers] relative to [now]. Returns `null` when no
      * recognized header is present or parseable.
@@ -197,17 +210,23 @@ public object RetryAfterParser {
      * integer (delta-seconds) form and the fractional form (`1.5`) that real servers and
      * proxies emit; the fractional part is honoured down to nanosecond resolution.
      *
-     * Returns `null` on any parse failure, on a negative value, or on a non-finite value
-     * (`NaN`, `Infinity`) — the retry layer then falls back to its backoff schedule rather
-     * than retrying immediately against a misbehaving server. A finite but absurdly large
-     * value is clamped to [MAX_DELAY] before the nanosecond conversion so the resulting
-     * [Duration] can never overflow [Duration.toNanos] downstream.
+     * The value is first screened against [NUMERIC_SECONDS] so only the plain decimal grammar
+     * is honoured: [String.toDoubleOrNull] otherwise accepts Java float literals such as `30d`
+     * and `0x1p4`, which must instead fall through to the HTTP-date branch (or, ultimately, the
+     * backoff schedule). Returns `null` on any parse failure, on a negative value, or on a
+     * non-finite value (`NaN`, `Infinity`). A finite but absurdly large value is clamped to
+     * [MAX_DELAY] before the nanosecond conversion so the resulting [Duration] can never
+     * overflow [Duration.toNanos] downstream.
      */
     private fun parseNumericSeconds(value: String): Duration? {
+        // The strict screen guarantees a non-negative decimal, so toDoubleOrNull never returns
+        // null/NaN here; an extremely long digit run can still overflow to +Infinity, which the
+        // ceiling check below absorbs by clamping rather than letting it reach the nanos multiply.
+        if (!NUMERIC_SECONDS.matches(value)) return null
         val seconds = value.toDoubleOrNull() ?: return null
-        if (seconds.isNaN() || seconds.isInfinite() || seconds < 0.0) return null
         // Clamp in the seconds domain before converting to nanos: a value beyond the ceiling
-        // would overflow the `* NANOS_PER_SECOND` multiply and the Long cast below.
+        // (or an overflow to +Infinity) would otherwise overflow the `* NANOS_PER_SECOND`
+        // multiply and the Long cast below.
         if (seconds >= MAX_DELAY_SECONDS) return MAX_DELAY
         return Duration.ofNanos((seconds * NANOS_PER_SECOND).toLong())
     }
diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryStepTest.kt
@@ -1174,6 +1174,115 @@ class RetryStepTest {
         assertEquals(2, attempts.get())
     }
 
+    @Test
+    fun `server override recognises every truthy token and forces a retry`() {
+        // Each documented truthy spelling (case-insensitive) must force a retry on an otherwise
+        // non-retryable 404. Verbatim casing variants prove the lowercase() normalisation.
+        for (token in listOf("true", "TRUE", "1", "yes", "YES", "retry", "Retry")) {
+            val opts =
+                HttpRetryOptions(
+                    maxRetries = 3,
+                    shouldRetryCondition = ServerOverrideRetryPredicate(),
+                )
+            val fake =
+                FakeHttpClient()
+                    .enqueue { status(404).header("X-Should-Retry", token) }
+                    .enqueue { status(200) }
+            val pipeline =
+                HttpPipelineBuilder(fake)
+                    .append(DefaultRetryStep(opts, zeroDelayClock()))
+                    .build()
+
+            val response = pipeline.send(getRequest())
+            assertEquals(200, response.status.code, "truthy token '$token' should force a retry")
+            assertEquals(2, fake.callCount, "truthy token '$token' should produce exactly one retry")
+        }
+    }
+
+    @Test
+    fun `server override recognises every falsy token and suppresses a retry`() {
+        // Each documented falsy spelling must suppress the retry the default classifier would
+        // otherwise allow on a 503.
+        for (token in listOf("false", "FALSE", "0", "no", "NO", "stop", "Stop")) {
+            val opts =
+                HttpRetryOptions(
+                    maxRetries = 3,
+                    shouldRetryCondition = ServerOverrideRetryPredicate(),
+                )
+            val fake =
+                FakeHttpClient()
+                    .enqueue { status(503).header("X-Should-Retry", token) }
+            val pipeline =
+                HttpPipelineBuilder(fake)
+                    .append(DefaultRetryStep(opts, zeroDelayClock()))
+                    .build()
+
+            val response = pipeline.send(getRequest())
+            assertEquals(503, response.status.code, "falsy token '$token' should suppress the retry")
+            assertEquals(1, fake.callCount, "falsy token '$token' should not retry")
+        }
+    }
+
+    @Test
+    fun `server override defers an unrecognised token to the delegate`() {
+        // An unrecognised value such as `maybe` is not a directive: the predicate falls through
+        // to the delegate. With the default classifier, a 503 still retries and a 404 still does
+        // not — proving the else branch defers rather than guessing.
+        val retryableOpts =
+            HttpRetryOptions(
+                maxRetries = 3,
+                shouldRetryCondition = ServerOverrideRetryPredicate(),
+            )
+        val retryableFake =
+            FakeHttpClient()
+                .enqueue { status(503).header("X-Should-Retry", "maybe") }
+                .enqueue { status(200) }
+        val retryablePipeline =
+            HttpPipelineBuilder(retryableFake)
+                .append(DefaultRetryStep(retryableOpts, zeroDelayClock()))
+                .build()
+        assertEquals(200, retryablePipeline.send(getRequest()).status.code)
+        assertEquals(2, retryableFake.callCount, "unrecognised token must defer: 503 still retries")
+
+        val nonRetryableOpts =
+            HttpRetryOptions(
+                maxRetries = 3,
+                shouldRetryCondition = ServerOverrideRetryPredicate(),
+            )
+        val nonRetryableFake =
+            FakeHttpClient()
+                .enqueue { status(404).header("X-Should-Retry", "maybe") }
+        val nonRetryablePipeline =
+            HttpPipelineBuilder(nonRetryableFake)
+                .append(DefaultRetryStep(nonRetryableOpts, zeroDelayClock()))
+                .build()
+        assertEquals(404, nonRetryablePipeline.send(getRequest()).status.code)
+        assertEquals(1, nonRetryableFake.callCount, "unrecognised token must defer: 404 still does not retry")
+    }
+
+    @Test
+    fun `server override does not bypass the non-replayable body gate`() {
+        // A truthy override flips the classification decision, but it does not bypass the
+        // replayability gate: a POST with a non-replayable body cannot be re-sent, so the
+        // response is returned without a retry even though the server asked for one.
+        val opts =
+            HttpRetryOptions(
+                maxRetries = 3,
+                shouldRetryCondition = ServerOverrideRetryPredicate(),
+            )
+        val fake =
+            FakeHttpClient()
+                .enqueue { status(404).header("X-Should-Retry", "true") }
+        val pipeline =
+            HttpPipelineBuilder(fake)
+                .append(DefaultRetryStep(opts, zeroDelayClock()))
+                .build()
+
+        val response = pipeline.send(nonReplayablePost())
+        assertEquals(404, response.status.code)
+        assertEquals(1, fake.callCount, "forced retry must not re-send a non-replayable body")
+    }
+
     // ----------------- Error propagation -----------------
 
     @Test
diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryAfterParserTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryAfterParserTest.kt
@@ -102,6 +102,37 @@ class RetryAfterParserTest {
         )
     }
 
+    @Test
+    fun `Retry-After with a Java float type suffix is not honoured as numeric`() {
+        // `"30d".toDoubleOrNull()` returns 30.0, but `30d` is not a delta-seconds value. The
+        // strict numeric screen rejects it, and it is not a valid HTTP-date either, so it must
+        // fall through to null rather than parse as 30 seconds.
+        assertNull(RetryAfterParser.parse(headers("Retry-After" to "30d"), now))
+        assertNull(RetryAfterParser.parse(headers("Retry-After" to "30f"), now))
+        assertNull(RetryAfterParser.parse(headers("Retry-After" to "30D"), now))
+    }
+
+    @Test
+    fun `Retry-After hex-float form is not honoured as numeric`() {
+        // `"0x1p4".toDoubleOrNull()` returns 16.0; the strict screen rejects the hex-float
+        // grammar so it does not silently parse as a 16-second delay.
+        assertNull(RetryAfterParser.parse(headers("Retry-After" to "0x1p4"), now))
+    }
+
+    @Test
+    fun `Retry-After with a leading plus or exponent is not honoured as numeric`() {
+        // Signs and scientific notation are outside the delta-seconds grammar and must be
+        // rejected by the strict screen.
+        assertNull(RetryAfterParser.parse(headers("Retry-After" to "+30"), now))
+        assertNull(RetryAfterParser.parse(headers("Retry-After" to "1e3"), now))
+        assertNull(RetryAfterParser.parse(headers("Retry-After" to ".5"), now))
+    }
+
+    @Test
+    fun `parseHeaderValue rejects a Retry-After float type suffix`() {
+        assertNull(RetryAfterParser.parseHeaderValue(HttpHeaderName.RETRY_AFTER, "30d", now))
+    }
+
     // endregion
 
     // region -- HTTP-date Retry-After --
