diff --git a/sdk-core/api/sdk-core.api b/sdk-core/api/sdk-core.api index de3a5d14..f136a561 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -945,6 +945,21 @@ public abstract class org/dexpace/sdk/core/http/pipeline/steps/RetryStep : org/d public final fun getStage ()Lorg/dexpace/sdk/core/http/pipeline/Stage; } +public final class org/dexpace/sdk/core/http/pipeline/steps/ServerOverrideRetryPredicate : org/dexpace/sdk/core/http/pipeline/steps/HttpRetryConditionPredicate { + public static final field Companion Lorg/dexpace/sdk/core/http/pipeline/steps/ServerOverrideRetryPredicate$Companion; + public static final field DEFAULT_HEADER_NAME Lorg/dexpace/sdk/core/http/common/HttpHeaderName; + public fun ()V + public fun (Lorg/dexpace/sdk/core/http/common/HttpHeaderName;)V + public fun (Lorg/dexpace/sdk/core/http/common/HttpHeaderName;Lorg/dexpace/sdk/core/http/pipeline/steps/HttpRetryConditionPredicate;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/common/HttpHeaderName;Lorg/dexpace/sdk/core/http/pipeline/steps/HttpRetryConditionPredicate;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public final fun getDelegate ()Lorg/dexpace/sdk/core/http/pipeline/steps/HttpRetryConditionPredicate; + public final fun getHeaderName ()Lorg/dexpace/sdk/core/http/common/HttpHeaderName; + public fun shouldRetry (Lorg/dexpace/sdk/core/http/pipeline/steps/HttpRetryCondition;)Z +} + +public final class org/dexpace/sdk/core/http/pipeline/steps/ServerOverrideRetryPredicate$Companion { +} + public final class org/dexpace/sdk/core/http/pipeline/steps/SetDateStep : org/dexpace/sdk/core/http/pipeline/HttpStep { public fun ()V public fun (Lorg/dexpace/sdk/core/util/Clock;)V @@ -2112,9 +2127,10 @@ public final class org/dexpace/sdk/core/pipeline/step/retry/RetrySettings { public static final field DEFAULT_RETRYABLE_METHODS Ljava/util/Set; public static final field DEFAULT_RETRYABLE_STATUSES Ljava/util/Set; public static final field DEFAULT_TOTAL_TIMEOUT Ljava/time/Duration; - public synthetic fun (Ljava/time/Duration;Ljava/time/Duration;DLjava/time/Duration;IDLjava/util/Set;Ljava/util/Set;Ljava/util/concurrent/ScheduledExecutorService;Lkotlin/jvm/internal/DefaultConstructorMarker;)V + public synthetic fun (Ljava/time/Duration;Ljava/time/Duration;DLjava/time/Duration;IDLjava/util/Set;Ljava/util/Set;Ljava/util/concurrent/ScheduledExecutorService;Lorg/dexpace/sdk/core/http/common/HttpHeaderName;Lkotlin/jvm/internal/DefaultConstructorMarker;)V public static final fun builder ()Lorg/dexpace/sdk/core/pipeline/step/retry/RetrySettings$RetrySettingsBuilder; public static final fun defaults ()Lorg/dexpace/sdk/core/pipeline/step/retry/RetrySettings; + public final fun getAttemptHeaderName ()Lorg/dexpace/sdk/core/http/common/HttpHeaderName; public final fun getDelayMultiplier ()D public final fun getInitialDelay ()Ljava/time/Duration; public final fun getJitter ()D @@ -2135,6 +2151,7 @@ public final class org/dexpace/sdk/core/pipeline/step/retry/RetrySettings$Compan public final class org/dexpace/sdk/core/pipeline/step/retry/RetrySettings$RetrySettingsBuilder : org/dexpace/sdk/core/generics/Builder { public fun ()V public fun (Lorg/dexpace/sdk/core/pipeline/step/retry/RetrySettings;)V + public final fun attemptHeaderName (Lorg/dexpace/sdk/core/http/common/HttpHeaderName;)Lorg/dexpace/sdk/core/pipeline/step/retry/RetrySettings$RetrySettingsBuilder; public synthetic fun build ()Ljava/lang/Object; public fun build ()Lorg/dexpace/sdk/core/pipeline/step/retry/RetrySettings; public final fun delayMultiplier (D)Lorg/dexpace/sdk/core/pipeline/step/retry/RetrySettings$RetrySettingsBuilder; 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 new file mode 100644 index 00000000..31b98079 --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/ServerOverrideRetryPredicate.kt @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2026 dexpace and Omar Aljarrah + * + * Licensed under the MIT License. See LICENSE in the project root. + * SPDX-License-Identifier: MIT + */ + +package org.dexpace.sdk.core.http.pipeline.steps + +import org.dexpace.sdk.core.http.common.HttpHeaderName +import java.util.Locale + +/** + * Composable [HttpRetryConditionPredicate] that lets the server steer the retry decision via an + * explicit response header (`X-Should-Retry` by default). + * + * Some APIs annotate responses with a definitive "retry / do not retry" signal that the client + * cannot infer from the status code alone — for example a `409 Conflict` that is genuinely + * transient, or a `503` the server knows will not recover. This predicate honours that signal + * when, and only when, the header is present: + * + * - A **truthy** value (`true`, `1`, `yes`, `retry`, case-insensitive) forces a retry, even for + * a status the default classifier would not retry. + * - A **falsy** value (`false`, `0`, `no`, `stop`, case-insensitive) suppresses a retry, even + * for a status the default classifier would retry. + * - When the header is **absent**, unrecognised, or there is no response (the exception path), + * 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 + * [HttpRetryOptions.shouldRetryCondition] (and/or [HttpRetryOptions.shouldRetryException]). The + * SDK's default retryable-status set is unchanged — in particular `409 Conflict` stays out of + * it; this predicate is the mechanism by which a server can opt a `409` (or any other status) + * into a retry, rather than widening the default set for everyone. + * + * ## Composition + * + * [delegate] defaults to the SDK's standard classifier, dispatched per path: the response + * classifier (`408 / 429 / 5xx`, except `501` / `505`) on the response path, and the exception + * classifier (`IOException` / `TimeoutException` anywhere in the cause chain) on the exception + * path. Because the override header only appears on responses, wiring this predicate into + * [HttpRetryOptions.shouldRetryException] leaves the exception-path decision entirely to the + * delegate. Supply a different delegate to layer the server override on top of bespoke retry + * logic, or [HttpRetryConditionPredicate] `{ false }` to make the server header the sole + * authority. + * + * ## Thread-safety + * + * Immutable and stateless — safe to share across concurrent requests. + * + * @property headerName The response header consulted for the override signal. Defaults to + * `X-Should-Retry`. + * @property delegate Fallback predicate consulted when the header is absent or unrecognised, or + * on the exception path. Defaults to the standard per-path classifier. + */ +public class ServerOverrideRetryPredicate + @JvmOverloads + constructor( + public val headerName: HttpHeaderName = DEFAULT_HEADER_NAME, + public val delegate: HttpRetryConditionPredicate = + HttpRetryConditionPredicate(::defaultClassifier), + ) : HttpRetryConditionPredicate { + override fun shouldRetry(condition: HttpRetryCondition): Boolean { + val response = condition.response ?: return delegate.shouldRetry(condition) + val raw = response.headers.get(headerName) ?: return delegate.shouldRetry(condition) + return when (raw.trim().lowercase(Locale.US)) { + in TRUTHY -> true + in FALSY -> false + // An unrecognised value is not a directive — defer rather than guess. + else -> delegate.shouldRetry(condition) + } + } + + public companion object { + /** Default override header (`X-Should-Retry`). */ + @JvmField + public val DEFAULT_HEADER_NAME: HttpHeaderName = HttpHeaderName.fromString("X-Should-Retry") + + /** Header values that force a retry. */ + private val TRUTHY: Set = setOf("true", "1", "yes", "retry") + + /** Header values that suppress a retry. */ + private val FALSY: Set = setOf("false", "0", "no", "stop") + } + } + +/** + * Default [ServerOverrideRetryPredicate.delegate]: applies the SDK's standard classification for + * whichever path the [condition] represents — the response classifier when a response is present, + * the exception classifier otherwise. This keeps the override predicate's fall-through behaviour + * identical to the stock [HttpRetryOptions] defaults on both the response and exception paths. + */ +private fun defaultClassifier(condition: HttpRetryCondition): Boolean = + if (condition.response != null) { + defaultShouldRetryResponse(condition) + } else { + defaultShouldRetryException(condition) + } 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 index 1323a18e..6f94ec95 100644 --- 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 @@ -28,7 +28,9 @@ import java.util.concurrent.ThreadLocalRandom * * ## Recognized header forms * - * 1. `Retry-After: ` — RFC 7231 §7.1.3 numeric (delta-seconds) form. + * 1. `Retry-After: ` — RFC 7231 §7.1.3 numeric (delta-seconds) form. Both integer + * and fractional values (e.g. `1.5`) are accepted; the fractional part is honoured to + * nanosecond resolution. * 2. `Retry-After: ` — RFC 7231 §7.1.3 absolute-date form (parsed via * [DateTimeRfc1123], which tolerates an informational weekday per RFC 7231 §7.1.1.1). * 3. `retry-after-ms: ` — millisecond delta variant. @@ -106,6 +108,25 @@ public object RetryAfterParser { */ private val MAX_DELAY: Duration = Duration.ofDays(365) + /** [MAX_DELAY] expressed in whole seconds — the clamp threshold for fractional parsing. */ + private val MAX_DELAY_SECONDS: Double = MAX_DELAY.seconds.toDouble() + + /** 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. @@ -185,14 +206,29 @@ public object RetryAfterParser { } /** - * Parses [value] as a non-negative integer count of seconds. Returns `null` on any parse - * failure, including negative values — the retry layer falls back to its backoff - * schedule rather than retrying immediately against a misbehaving server. + * Parses [value] as a non-negative count of seconds. Accepts both the RFC 7231 §7.1.3 + * 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. + * + * 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? { - val seconds = value.toLongOrNull() ?: return null - if (seconds < 0L) return null - return Duration.ofSeconds(seconds) + // 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 + // Clamp in the seconds domain before converting to nanos: a value beyond the ceiling + // (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/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetrySettings.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetrySettings.kt index e7654933..d54f5bb7 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetrySettings.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetrySettings.kt @@ -8,6 +8,7 @@ package org.dexpace.sdk.core.pipeline.step.retry import org.dexpace.sdk.core.generics.Builder +import org.dexpace.sdk.core.http.common.HttpHeaderName import org.dexpace.sdk.core.http.request.Method import java.time.Duration import java.util.Collections @@ -37,6 +38,7 @@ private val MAX_NANO_REPRESENTABLE_DELAY: Duration = Duration.ofNanos(Long.MAX_V * - [retryableMethods] = `{GET, HEAD, OPTIONS, PUT, DELETE}` — safe-by-method per RFC 9110. * `POST`/`PATCH`/etc. retry only when the request body is replayable. * - [scheduler] = `null` — fall back to the lazy daemon scheduler created by [RetryStep]. + * - [attemptHeaderName] = `null` — no per-attempt header is stamped (opt-in; see the property). * * ## Thread-safety * @@ -61,6 +63,12 @@ private val MAX_NANO_REPRESENTABLE_DELAY: Duration = Duration.ofNanos(Long.MAX_V * RFC). Non-idempotent methods (`POST`, `PATCH`) only retry when the body is replayable. * @property scheduler Optional caller-provided scheduler. When `null` [RetryStep] uses a * process-wide lazy daemon scheduler. + * @property attemptHeaderName Optional request header stamped on each attempt [RetryStep] + * dispatches, carrying the 1-based attempt ordinal (`1` for the original send, `2` for the + * first retry, and so on) so servers and proxies can observe the retry count. `null` (the + * default) disables the header entirely. The header is set on a per-attempt copy of the + * request, never on the immutable template. Any idempotency key the caller stamps stays + * stable across retries — only this attempt header changes per send. */ public class RetrySettings // The 9-arg constructor lives behind a `private` modifier — public construction goes @@ -78,6 +86,7 @@ public class RetrySettings public val retryableStatuses: Set, public val retryableMethods: Set, public val scheduler: ScheduledExecutorService?, + public val attemptHeaderName: HttpHeaderName?, ) { /** Returns a fresh [RetrySettingsBuilder] preloaded with this instance's values. */ public fun newBuilder(): RetrySettingsBuilder = RetrySettingsBuilder(this) @@ -96,6 +105,7 @@ public class RetrySettings private var retryableStatuses: Set = DEFAULT_RETRYABLE_STATUSES private var retryableMethods: Set = DEFAULT_RETRYABLE_METHODS private var scheduler: ScheduledExecutorService? = null + private var attemptHeaderName: HttpHeaderName? = null /** Creates an empty builder populated with the SDK defaults. */ public constructor() @@ -111,6 +121,7 @@ public class RetrySettings this.retryableStatuses = settings.retryableStatuses this.retryableMethods = settings.retryableMethods this.scheduler = settings.scheduler + this.attemptHeaderName = settings.attemptHeaderName } /** Sets [RetrySettings.totalTimeout]. Must be non-negative. */ @@ -186,6 +197,16 @@ public class RetrySettings this.scheduler = scheduler } + /** + * Sets [RetrySettings.attemptHeaderName]. When non-null, [RetryStep] stamps this + * header (carrying the 1-based attempt ordinal) on each attempt's request copy. + * `null` (the default) leaves attempts unstamped. + */ + public fun attemptHeaderName(attemptHeaderName: HttpHeaderName?): RetrySettingsBuilder = + apply { + this.attemptHeaderName = attemptHeaderName + } + /** Builds the immutable [RetrySettings] instance. */ override fun build(): RetrySettings = RetrySettings( @@ -198,6 +219,7 @@ public class RetrySettings retryableStatuses = Collections.unmodifiableSet(LinkedHashSet(retryableStatuses)), retryableMethods = Collections.unmodifiableSet(LinkedHashSet(retryableMethods)), scheduler = scheduler, + attemptHeaderName = attemptHeaderName, ) } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt index 8d50c8ec..f5b2e030 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt @@ -56,6 +56,14 @@ import java.util.concurrent.TimeUnit * is a deliberate departure from Square's `RetryInterceptor`, which retries on status alone and * silently double-sends a body it cannot replay on transport timeouts. * + * ## Per-attempt header + * + * When [RetrySettings.attemptHeaderName] is configured (it is `null` by default), every send is + * stamped with that header carrying the 1-based attempt ordinal (`1` for the original send, `2` + * for the first retry, …) on a fresh per-attempt copy of the request, so servers and proxies can + * observe the retry count. The header is set on a copy via [Request.newBuilder]; the captured + * template is never mutated, and any idempotency key already on the request is left untouched. + * * ## Cancellation * * Waits between attempts use [CompletableFuture.get] backed by [scheduler] — never @@ -142,9 +150,11 @@ public class RetryStep */ @Throws(Throwable::class) public fun attempt(): Response { + // The original send is attempt ordinal 1 — stamp it when the feature is enabled so + // the very first request carries the same observable counter the retries will. val initial: ResponseOutcome = try { - ResponseOutcome.Success(httpClient.execute(request)) + ResponseOutcome.Success(httpClient.execute(stampAttempt(request, attemptOrdinal = 1))) } catch (t: Throwable) { ResponseOutcome.Failure(t) } @@ -172,7 +182,9 @@ public class RetryStep is AttemptStep.Abort -> return ResponseOutcome.Failure(readyState.error) is AttemptStep.Proceed -> { state.attempt += 1 - val outcome = executeOnce() + // state.attempt is now the 1-based ordinal of the send about to happen: + // the original was 1, so the first retry dispatched here is 2. + val outcome = executeOnce(state.attempt) if (outcome is ResponseOutcome.Success) return outcome val nextError = (outcome as ResponseOutcome.Failure).error if (!isClassifiedRetryable(nextError)) return outcome @@ -268,10 +280,13 @@ public class RetryStep * Executes the request once via the captured transport, converting any throwable * raised by the transport into a [ResponseOutcome.Failure]. Preserves the interrupt * flag if the transport raises an [InterruptedException] mid-send. + * + * @param attemptOrdinal The 1-based ordinal of this send, stamped onto the per-attempt + * request copy when [RetrySettings.attemptHeaderName] is configured. */ - private fun executeOnce(): ResponseOutcome = + private fun executeOnce(attemptOrdinal: Int): ResponseOutcome = try { - ResponseOutcome.Success(httpClient.execute(request)) + ResponseOutcome.Success(httpClient.execute(stampAttempt(request, attemptOrdinal))) } catch (e: InterruptedException) { Thread.currentThread().interrupt() ResponseOutcome.Failure(e) @@ -279,6 +294,25 @@ public class RetryStep ResponseOutcome.Failure(t) } + /** + * Returns a per-attempt copy of [request] carrying the configured + * [RetrySettings.attemptHeaderName] set to [attemptOrdinal]. When no attempt header is + * configured the original [request] is returned unchanged — the no-op path allocates + * nothing, so the common (feature-off) case pays no cost. The returned copy is built via + * [Request.newBuilder] so the immutable template the step captured is never mutated; any + * idempotency key the caller stamped is preserved verbatim because only this single + * header is replaced. + */ + private fun stampAttempt( + request: Request, + attemptOrdinal: Int, + ): Request { + val header = settings.attemptHeaderName ?: return request + return request.newBuilder() + .setHeader(header.caseSensitiveName, attemptOrdinal.toString()) + .build() + } + /** * Blocks the calling thread for [delay] without pinning a carrier thread under Loom. * Uses [ScheduledExecutorService.schedule] + [CompletableFuture.get] so the wait can 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 index 12fd9b1b..16d9e0a6 100644 --- 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 @@ -1038,6 +1038,251 @@ class RetryStepTest { assertTrue(closes.get() >= 1, "retryable response must be closed when the delay override throws") } + // ----------------- ServerOverrideRetryPredicate (X-Should-Retry) ----------------- + + @Test + fun `server override is not consulted by default`() { + // Off-by-default: the stock options never look at X-Should-Retry, so a 200 carrying + // `X-Should-Retry: true` is still returned without a retry. + val fake = + FakeHttpClient() + .enqueue { status(200).header("X-Should-Retry", "true") } + + val pipeline = retryPipeline(fake) + val response = pipeline.send(getRequest()) + assertEquals(200, response.status.code) + assertEquals(1, fake.callCount) + } + + @Test + fun `server override forces a retry on an otherwise non-retryable status`() { + // 404 is not in the default retryable set, but an explicit server signal forces a retry + // when the predicate is wired in. + val opts = + HttpRetryOptions( + maxRetries = 3, + shouldRetryCondition = ServerOverrideRetryPredicate(), + ) + val fake = + FakeHttpClient() + .enqueue { status(404).header("X-Should-Retry", "true") } + .enqueue { status(200) } + + val pipeline = + HttpPipelineBuilder(fake) + .append(DefaultRetryStep(opts, zeroDelayClock())) + .build() + + val response = pipeline.send(getRequest()) + assertEquals(200, response.status.code) + assertEquals(2, fake.callCount) + } + + @Test + fun `server override suppresses a retry on an otherwise retryable status`() { + // 503 is normally retried, but the server explicitly tells us not to. + val opts = + HttpRetryOptions( + maxRetries = 3, + shouldRetryCondition = ServerOverrideRetryPredicate(), + ) + val fake = + FakeHttpClient() + .enqueue { status(503).header("X-Should-Retry", "false") } + + val pipeline = + HttpPipelineBuilder(fake) + .append(DefaultRetryStep(opts, zeroDelayClock())) + .build() + + val response = pipeline.send(getRequest()) + assertEquals(503, response.status.code) + assertEquals(1, fake.callCount) + } + + @Test + fun `server override falls back to the wrapped predicate when the header is absent`() { + // No X-Should-Retry header → defer to the delegate (here, the default classifier), + // so a 503 still retries and a 404 still does not. + val opts = + HttpRetryOptions( + maxRetries = 3, + shouldRetryCondition = ServerOverrideRetryPredicate(), + ) + val fake = + FakeHttpClient() + .enqueue { status(503) } + .enqueue { status(200) } + + val pipeline = + HttpPipelineBuilder(fake) + .append(DefaultRetryStep(opts, zeroDelayClock())) + .build() + + val response = pipeline.send(getRequest()) + assertEquals(200, response.status.code) + assertEquals(2, fake.callCount) + } + + @Test + fun `server override honours a configurable header name`() { + val opts = + HttpRetryOptions( + maxRetries = 3, + shouldRetryCondition = + ServerOverrideRetryPredicate(headerName = HttpHeaderName.fromString("X-Retry-Me")), + ) + val fake = + FakeHttpClient() + .enqueue { status(404).header("X-Retry-Me", "true") } + .enqueue { status(200) } + + val pipeline = + HttpPipelineBuilder(fake) + .append(DefaultRetryStep(opts, zeroDelayClock())) + .build() + + val response = pipeline.send(getRequest()) + assertEquals(200, response.status.code) + assertEquals(2, fake.callCount) + } + + @Test + fun `server override defers on the exception path`() { + // The header lives on responses; on the exception path the predicate defers to the + // delegate. With the default delegate, a retryable IOException still retries. + val opts = + HttpRetryOptions( + maxRetries = 3, + shouldRetryException = ServerOverrideRetryPredicate(), + ) + val attempts = AtomicInteger(0) + val client = + object : HttpClient { + override fun execute(request: Request): Response { + if (attempts.getAndIncrement() == 0) throw IOException("transient") + return okResponse(request) + } + } + val pipeline = + HttpPipelineBuilder(client) + .append(DefaultRetryStep(opts, zeroDelayClock())) + .build() + + val response = pipeline.send(getRequest()) + assertEquals(200, response.status.code) + 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 index dd714195..378ed411 100644 --- 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 @@ -58,6 +58,81 @@ class RetryAfterParserTest { assertEquals(Duration.ofSeconds(7), RetryAfterParser.parse(headers("Retry-After" to " 7 "), now)) } + @Test + fun `fractional Retry-After parses as sub-second delay`() { + // RFC 7231 delta-seconds is integer-only, but real servers and proxies emit `1.5`. + // 1.5 s == 1_500 ms. + assertEquals(Duration.ofMillis(1500), RetryAfterParser.parse(headers("Retry-After" to "1.5"), now)) + } + + @Test + fun `fractional Retry-After below one second parses`() { + // 0.25 s == 250 ms — the integer parser would have rejected this entirely. + assertEquals(Duration.ofMillis(250), RetryAfterParser.parse(headers("Retry-After" to "0.25"), now)) + } + + @Test + fun `fractional Retry-After negative returns null`() { + // The existing negative clamp must survive the switch to fractional parsing. + assertNull(RetryAfterParser.parse(headers("Retry-After" to "-1.5"), now)) + } + + @Test + fun `fractional Retry-After NaN returns null`() { + assertNull(RetryAfterParser.parse(headers("Retry-After" to "NaN"), now)) + } + + @Test + fun `fractional Retry-After infinity returns null`() { + assertNull(RetryAfterParser.parse(headers("Retry-After" to "Infinity"), now)) + } + + @Test + fun `fractional Retry-After far-future value is clamped to 365 days`() { + // A value larger than the 365-day ceiling must clamp, not overflow toNanos(). + val tenYearsSeconds = Duration.ofDays(3650).seconds.toString() + assertEquals(Duration.ofDays(365), RetryAfterParser.parse(headers("Retry-After" to tenYearsSeconds), now)) + } + + @Test + fun `parseHeaderValue dispatches fractional Retry-After to sub-second delay`() { + assertEquals( + Duration.ofMillis(2500), + RetryAfterParser.parseHeaderValue(HttpHeaderName.RETRY_AFTER, "2.5", now), + ) + } + + @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 -- @@ -278,10 +353,13 @@ class RetryAfterParserTest { } @Test - fun `numeric Retry-After of Long MAX_VALUE does not throw`() { - // Duration.ofSeconds(Long.MAX_VALUE) is representable; the downstream consumer clamps. + fun `numeric Retry-After of Long MAX_VALUE clamps to 365 days`() { + // The numeric form now shares the 365-day ceiling applied to the HTTP-date and + // Unix-epoch forms: a far-future delta is clamped before the nanosecond conversion so + // a later Duration.toNanos() can never overflow. No real pacing header asks a client to + // wait centuries, so clamping here loses nothing operationally. val result = RetryAfterParser.parse(headers("Retry-After" to Long.MAX_VALUE.toString()), now) - assertEquals(Duration.ofSeconds(Long.MAX_VALUE), result) + assertEquals(Duration.ofDays(MAX_CLAMP_DAYS), result) } // endregion diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStepTest.kt index 0b5b3ec3..62970e36 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStepTest.kt @@ -9,6 +9,7 @@ package org.dexpace.sdk.core.pipeline.step.retry import org.dexpace.sdk.core.client.HttpClient import org.dexpace.sdk.core.http.common.Headers +import org.dexpace.sdk.core.http.common.HttpHeaderName import org.dexpace.sdk.core.http.common.Protocol import org.dexpace.sdk.core.http.request.Method import org.dexpace.sdk.core.http.request.Request @@ -21,6 +22,7 @@ import org.dexpace.sdk.core.http.response.exception.NetworkException import org.dexpace.sdk.core.pipeline.ResponseOutcome import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertNull import org.junit.jupiter.api.Assertions.assertSame import org.junit.jupiter.api.Assertions.assertThrows import org.junit.jupiter.api.Assertions.assertTrue @@ -597,12 +599,115 @@ class RetryStepTest { // endregion + // region -- per-attempt retry-count header -- + + @Test + fun `attempt header is absent by default`() { + // Opt-in feature: with the default settings no attempt header is stamped on retries. + val client = + FakeClient( + listOf( + Canned.Err(httpException(SC_SERVICE_UNAVAILABLE)), + Canned.Ok(response(SC_OK)), + ), + ) + val step = RetryStep(client, zeroDelaySettings(InstantScheduler()), requestGet()) + val out = step.invoke(ResponseOutcome.Failure(httpException(SC_SERVICE_UNAVAILABLE))) + assertTrue(out is ResponseOutcome.Success) + // Two retried sends were made; neither carries the attempt header. + assertEquals(2, client.calls.size) + client.calls.forEach { call -> + assertNull(call.headers.get(DEFAULT_ATTEMPT_HEADER), "no attempt header should be stamped by default") + } + } + + @Test + fun `attempt header is stamped with the attempt ordinal on each send via attempt()`() { + // attempt() drives the initial send too, so we can observe the full ordinal sequence: + // the original send is attempt 1, the first retry is 2, the second retry is 3. + val client = + FakeClient( + listOf( + Canned.Err(httpException(SC_SERVICE_UNAVAILABLE)), + Canned.Err(httpException(SC_SERVICE_UNAVAILABLE)), + Canned.Ok(response(SC_OK)), + ), + ) + val settings = + zeroDelaySettings(InstantScheduler()) + .newBuilder() + .attemptHeaderName(DEFAULT_ATTEMPT_HEADER) + .build() + val step = RetryStep(client, settings, requestGet()) + + val response = step.attempt() + assertEquals(SC_OK, response.status.code) + assertEquals(3, client.calls.size) + assertEquals("1", client.calls[0].headers.get(DEFAULT_ATTEMPT_HEADER)) + assertEquals("2", client.calls[1].headers.get(DEFAULT_ATTEMPT_HEADER)) + assertEquals("3", client.calls[2].headers.get(DEFAULT_ATTEMPT_HEADER)) + } + + @Test + fun `attempt header uses the configured header name`() { + val custom = HttpHeaderName.fromString("X-My-Retry-Count") + val client = + FakeClient( + listOf( + Canned.Err(httpException(SC_SERVICE_UNAVAILABLE)), + Canned.Ok(response(SC_OK)), + ), + ) + val settings = + zeroDelaySettings(InstantScheduler()) + .newBuilder() + .attemptHeaderName(custom) + .build() + val step = RetryStep(client, settings, requestGet()) + + val out = step.invoke(ResponseOutcome.Failure(httpException(SC_SERVICE_UNAVAILABLE))) + assertTrue(out is ResponseOutcome.Success) + // invoke() does not control the external initial send (ordinal 1), so the retried sends + // it dispatches start at ordinal 2. Two retries fire here under the configured name. + assertEquals(2, client.calls.size) + assertEquals("2", client.calls[0].headers.get(custom)) + assertEquals("3", client.calls[1].headers.get(custom)) + } + + @Test + fun `attempt header is stamped on a per-attempt copy and does not mutate the template request`() { + val client = + FakeClient( + listOf( + Canned.Err(httpException(SC_SERVICE_UNAVAILABLE)), + Canned.Ok(response(SC_OK)), + ), + ) + val request = requestGet() + val settings = + zeroDelaySettings(InstantScheduler()) + .newBuilder() + .attemptHeaderName(DEFAULT_ATTEMPT_HEADER) + .build() + val step = RetryStep(client, settings, request) + + step.invoke(ResponseOutcome.Failure(httpException(SC_SERVICE_UNAVAILABLE))) + // The retried request copy carries the header; the immutable template never gains it. + assertEquals("2", client.calls[0].headers.get(DEFAULT_ATTEMPT_HEADER)) + assertNull(request.headers.get(DEFAULT_ATTEMPT_HEADER), "template request must stay unmodified") + } + + // endregion + private companion object { // HTTP status code constants for tests. private const val SC_OK = 200 private const val SC_NOT_FOUND = 404 private const val SC_SERVICE_UNAVAILABLE = 503 + // Header used by the per-attempt retry-count tests. + private val DEFAULT_ATTEMPT_HEADER: HttpHeaderName = HttpHeaderName.fromString("X-Retry-Attempt") + private const val DEFAULT_MAX_ATTEMPTS = 3 private const val DEFAULT_MAX_ATTEMPTS_TIMEOUT = 3