diff --git a/CHANGELOG.md b/CHANGELOG.md index d185e1d..ee991f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,8 +9,27 @@ may break public API until 1.0.0 ships. ## [Unreleased] +### Changed + +- **Breaking** — `RetryPolicy`'s canonical constructor gains a fourth `Duration`, `maxTotalDelay`, + after `maxDelay` (seven components, was six). Migration: prefer the new `RetryPolicy.builder()` + (starts from `defaults()`, one setter per knob), or insert `Duration.ofMinutes(1)` — the default — + as the fourth positional argument. Deconstruction patterns over the record gain a component too. + Pre-1.0 per [ADR-019](docs/adr/019-pre-10-stability-policy.md); see + [ADR-027](docs/adr/027-retry-policy-builder-and-budget.md). + ### Added +- **`fanar-core`** — a total sleep budget for retries ([ADR-027](docs/adr/027-retry-policy-builder-and-budget.md)): + `RetryPolicy.maxTotalDelay()` (default 1 min — the worst case the other defaults already allowed, so + nothing changes at the defaults) bounds the sum of all sleeps within one call; a sleep that would exceed + it — computed back-off or honoured `Retry-After` hint — is never started, retrying ends and the exception + surfaces with the hint preserved, like the ADR-025 ceiling. Plus `RetryPolicy.builder()` and + `withMaxTotalDelay(...)`. +- **`fanar-spring-boot-4-starter`** — `fanar.retry.max-total-delay` (default `1m`); the `RetryPolicy` bean + is built through the builder, so a `max-delay` raised above the budget fails the context at startup + ([ADR-020](docs/adr/020-spring-boot-4-starter.md), amended). + - **`fanar-core`** — rate-limit visibility ([ADR-026](docs/adr/026-rate-limit-telemetry.md)): the retry boundary publishes `fanar.ratelimit.limit` / `.remaining` / `.reset` / `.policy` observation attributes from every response that carries Fanar's rate-limit headers (successes and 429s alike; the last attempt's values win), and both HTTP diff --git a/core/src/main/java/qa/fanar/core/RetryPolicy.java b/core/src/main/java/qa/fanar/core/RetryPolicy.java index 939ac5e..203c6d1 100644 --- a/core/src/main/java/qa/fanar/core/RetryPolicy.java +++ b/core/src/main/java/qa/fanar/core/RetryPolicy.java @@ -8,19 +8,25 @@ * Retry configuration for the SDK's built-in retry interceptor. * *

Immutable and thread-safe. Construct via {@link #defaults()} or {@link #disabled()} for the - * canonical presets, or via the record constructor for full control. Derive variants through the - * {@code with*} methods — each returns a new record.

+ * canonical presets, via {@link #builder()} (which starts from the defaults) for full control, or + * via the record constructor. Derive variants through the {@code with*} methods — each returns a + * new record. The builder is the stable way to set knobs: the canonical constructor changes + * arity whenever a knob is added (ADR-027).

* *

This type holds only the configuration. The retry loop — applying backoff, honoring * {@code Retry-After}, computing jitter — lives in the SDK's internal retry interceptor.

* *

Default policy

*

{@link #defaults()} returns 3 attempts, exponential backoff with - * {@link JitterStrategy#FULL full jitter}, base 500 ms, cap 30 s, multiplier 2.0, and - * the {@link #isDefaultRetryable default retryable predicate} (transient server-side errors and - * transport failures — never client-side or content-filter errors). The cap also bounds - * honoured server {@code Retry-After} hints: a hint above it ends retrying and the exception - * surfaces immediately with the hint preserved (ADR-025).

+ * {@link JitterStrategy#FULL full jitter}, base 500 ms, cap 30 s, a total sleep budget of + * 1 min, multiplier 2.0, and the {@link #isDefaultRetryable default retryable predicate} + * (transient server-side errors and transport failures — never client-side or content-filter + * errors). The cap also bounds honoured server {@code Retry-After} hints: a hint above it ends + * retrying and the exception surfaces immediately with the hint preserved (ADR-025). The budget + * bounds the sum of all sleeps within one call: a sleep that would push the total over it + * is never taken — retrying ends and the exception surfaces, hint preserved (ADR-027). At the + * defaults the budget equals the worst case the other knobs allow (two sleeps of at most 30 s), + * so it only bites once {@code maxAttempts} or {@code maxDelay} are raised.

* *

Validation

*

The canonical constructor validates all invariants at construction time. {@code with*} @@ -34,12 +40,18 @@ * {@code Retry-After} hints — a hint above it ends retrying and the * exception surfaces with the hint preserved (ADR-025); must be * positive, ≥ {@code baseDelay}, and representable in milliseconds + * @param maxTotalDelay budget for the sum of all sleeps within one call — the next sleep + * (computed back-off or honoured hint) is taken only if the total stays + * within it, otherwise retrying ends and the exception surfaces with + * the hint preserved (ADR-027); must be positive, ≥ {@code maxDelay}, + * and representable in milliseconds * @param backoffMultiplier factor applied to the backoff on each retry; must be ≥ 1.0 * @param jitter jitter policy applied to the computed backoff * @param retryable predicate deciding whether a given exception is worth retrying. * Consulted only while the policy can still honour a retry: the - * attempt budget ({@code maxAttempts}) and the delay ceiling - * ({@code maxDelay}) end retrying regardless of its answer + * attempt budget ({@code maxAttempts}), the delay ceiling + * ({@code maxDelay}) and the total sleep budget ({@code maxTotalDelay}) + * end retrying regardless of its answer * * @author Oussama Mahjoub */ @@ -47,6 +59,7 @@ public record RetryPolicy( int maxAttempts, Duration baseDelay, Duration maxDelay, + Duration maxTotalDelay, double backoffMultiplier, JitterStrategy jitter, Predicate retryable @@ -78,6 +91,18 @@ public record RetryPolicy( throw new IllegalArgumentException( "maxDelay must be representable in milliseconds, got " + maxDelay); } + Objects.requireNonNull(maxTotalDelay, "maxTotalDelay"); + if (maxTotalDelay.isNegative() || maxTotalDelay.isZero()) { + throw new IllegalArgumentException("maxTotalDelay must be positive, got " + maxTotalDelay); + } + if (maxTotalDelay.compareTo(maxDelay) < 0) { + throw new IllegalArgumentException( + "maxTotalDelay (" + maxTotalDelay + ") must be >= maxDelay (" + maxDelay + ")"); + } + if (maxTotalDelay.compareTo(MAX_REPRESENTABLE_DELAY) > 0) { + throw new IllegalArgumentException( + "maxTotalDelay must be representable in milliseconds, got " + maxTotalDelay); + } if (backoffMultiplier < 1.0) { throw new IllegalArgumentException( "backoffMultiplier must be >= 1.0, got " + backoffMultiplier); @@ -88,7 +113,7 @@ public record RetryPolicy( /** * The SDK's default retry policy: 3 attempts, exponential backoff with full jitter, base 500 ms, - * cap 30 s, multiplier 2.0, and the default retryable predicate. + * cap 30 s, total sleep budget 1 min, multiplier 2.0, and the default retryable predicate. * * @return a new policy instance with the documented defaults */ @@ -97,6 +122,7 @@ public static RetryPolicy defaults() { 3, Duration.ofMillis(500), Duration.ofSeconds(30), + Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, RetryPolicy::isDefaultRetryable); @@ -113,6 +139,7 @@ public static RetryPolicy disabled() { 1, Duration.ofMillis(500), Duration.ofSeconds(30), + Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, RetryPolicy::isDefaultRetryable); @@ -142,27 +169,35 @@ public static boolean isDefaultRetryable(FanarException e) { /** @return a new policy with the given {@code maxAttempts}, all other fields unchanged */ public RetryPolicy withMaxAttempts(int maxAttempts) { - return new RetryPolicy(maxAttempts, baseDelay, maxDelay, backoffMultiplier, jitter, retryable); + return new RetryPolicy(maxAttempts, baseDelay, maxDelay, maxTotalDelay, backoffMultiplier, jitter, retryable); } /** @return a new policy with the given {@code baseDelay}, all other fields unchanged */ public RetryPolicy withBaseDelay(Duration baseDelay) { - return new RetryPolicy(maxAttempts, baseDelay, maxDelay, backoffMultiplier, jitter, retryable); + return new RetryPolicy(maxAttempts, baseDelay, maxDelay, maxTotalDelay, backoffMultiplier, jitter, retryable); } /** @return a new policy with the given {@code maxDelay}, all other fields unchanged */ public RetryPolicy withMaxDelay(Duration maxDelay) { - return new RetryPolicy(maxAttempts, baseDelay, maxDelay, backoffMultiplier, jitter, retryable); + return new RetryPolicy(maxAttempts, baseDelay, maxDelay, maxTotalDelay, backoffMultiplier, jitter, retryable); + } + + /** + * @return a new policy with the given {@code maxTotalDelay}, all other fields unchanged + * @since 0.4.0 + */ + public RetryPolicy withMaxTotalDelay(Duration maxTotalDelay) { + return new RetryPolicy(maxAttempts, baseDelay, maxDelay, maxTotalDelay, backoffMultiplier, jitter, retryable); } /** @return a new policy with the given {@code backoffMultiplier}, all other fields unchanged */ public RetryPolicy withBackoffMultiplier(double backoffMultiplier) { - return new RetryPolicy(maxAttempts, baseDelay, maxDelay, backoffMultiplier, jitter, retryable); + return new RetryPolicy(maxAttempts, baseDelay, maxDelay, maxTotalDelay, backoffMultiplier, jitter, retryable); } /** @return a new policy with the given {@code jitter}, all other fields unchanged */ public RetryPolicy withJitter(JitterStrategy jitter) { - return new RetryPolicy(maxAttempts, baseDelay, maxDelay, backoffMultiplier, jitter, retryable); + return new RetryPolicy(maxAttempts, baseDelay, maxDelay, maxTotalDelay, backoffMultiplier, jitter, retryable); } /** @@ -170,6 +205,97 @@ public RetryPolicy withJitter(JitterStrategy jitter) { * the attempt budget and the delay ceiling still apply regardless of its answer */ public RetryPolicy withRetryable(Predicate retryable) { - return new RetryPolicy(maxAttempts, baseDelay, maxDelay, backoffMultiplier, jitter, retryable); + return new RetryPolicy(maxAttempts, baseDelay, maxDelay, maxTotalDelay, backoffMultiplier, jitter, retryable); + } + + /** + * Begin building a policy from {@link #defaults()}: every knob starts at its default and only + * the ones set change. Validation happens in {@link Builder#build()}, exactly as for the + * canonical constructor. + * + * @return a new builder + * @since 0.4.0 + */ + public static Builder builder() { + return new Builder(defaults()); + } + + /** + * Fluent builder for {@link RetryPolicy} (ADR-027). Not thread-safe; build once. + * + * @since 0.4.0 + */ + public static final class Builder { + + private int maxAttempts; + private Duration baseDelay; + private Duration maxDelay; + private Duration maxTotalDelay; + private double backoffMultiplier; + private JitterStrategy jitter; + private Predicate retryable; + + private Builder(RetryPolicy start) { + this.maxAttempts = start.maxAttempts(); + this.baseDelay = start.baseDelay(); + this.maxDelay = start.maxDelay(); + this.maxTotalDelay = start.maxTotalDelay(); + this.backoffMultiplier = start.backoffMultiplier(); + this.jitter = start.jitter(); + this.retryable = start.retryable(); + } + + /** @param maxAttempts total attempts including the first; {@code 1} disables retries */ + public Builder maxAttempts(int maxAttempts) { + this.maxAttempts = maxAttempts; + return this; + } + + /** @param baseDelay initial backoff delay */ + public Builder baseDelay(Duration baseDelay) { + this.baseDelay = baseDelay; + return this; + } + + /** @param maxDelay cap on one sleep and ceiling on honoured {@code Retry-After} hints (ADR-025) */ + public Builder maxDelay(Duration maxDelay) { + this.maxDelay = maxDelay; + return this; + } + + /** @param maxTotalDelay budget for the sum of all sleeps within one call (ADR-027) */ + public Builder maxTotalDelay(Duration maxTotalDelay) { + this.maxTotalDelay = maxTotalDelay; + return this; + } + + /** @param backoffMultiplier factor applied to the backoff on each retry */ + public Builder backoffMultiplier(double backoffMultiplier) { + this.backoffMultiplier = backoffMultiplier; + return this; + } + + /** @param jitter jitter policy applied to the computed backoff */ + public Builder jitter(JitterStrategy jitter) { + this.jitter = jitter; + return this; + } + + /** @param retryable predicate deciding whether an exception is worth retrying */ + public Builder retryable(Predicate retryable) { + this.retryable = retryable; + return this; + } + + /** + * Validate and build. + * + * @return the policy + * @throws IllegalArgumentException or {@link NullPointerException} on an invalid knob, + * exactly as the canonical constructor + */ + public RetryPolicy build() { + return new RetryPolicy(maxAttempts, baseDelay, maxDelay, maxTotalDelay, backoffMultiplier, jitter, retryable); + } } } diff --git a/core/src/main/java/qa/fanar/core/internal/retry/RetryInterceptor.java b/core/src/main/java/qa/fanar/core/internal/retry/RetryInterceptor.java index a38f94d..10588f4 100644 --- a/core/src/main/java/qa/fanar/core/internal/retry/RetryInterceptor.java +++ b/core/src/main/java/qa/fanar/core/internal/retry/RetryInterceptor.java @@ -43,6 +43,10 @@ * sleep uses the hint instead of the computed back-off curve. A hint above {@code maxDelay} * ends retrying immediately — no sleep, no burned attempt — and the exception surfaces with * the hint preserved (ADR-025). + *

  • Never sleeps past {@link RetryPolicy#maxTotalDelay()} in total for one call: when the next + * sleep — computed back-off or honoured hint — would push the cumulative sleep over the + * budget, retrying ends without sleeping and the exception surfaces with the hint preserved + * (ADR-027).
    • *
  • Otherwise sleeps for {@code baseDelay * multiplier^(attempt-1)} capped at {@code maxDelay}, * with {@link JitterStrategy} applied (none / full / equal).
    • *
  • Records {@link FanarObservationAttributes#HTTP_STATUS_CODE} for every response received @@ -80,6 +84,7 @@ public RetryInterceptor(RetryPolicy policy) { @Override public HttpResponse intercept(HttpRequest request, Chain chain) { int attempt = 0; + Duration slept = Duration.ZERO; while (true) { attempt++; try { @@ -97,11 +102,15 @@ public HttpResponse intercept(HttpRequest request, Chain chain) { if (attempt >= policy.maxAttempts() || !policy.retryable().test(e) || (hint != null && hint.compareTo(policy.maxDelay()) > 0)) { - recordRetryCount(chain, attempt - 1); - throw e; + throw surfaced(chain, attempt, e); + } + Duration delay = hint != null ? hint : backoff(attempt); + if (slept.plus(delay).compareTo(policy.maxTotalDelay()) > 0) { + throw surfaced(chain, attempt, e); } chain.observation().event("retry_attempt"); - sleepOrAbort(hint != null ? hint : backoff(attempt)); + sleepOrAbort(delay); + slept = slept.plus(delay); } } } @@ -164,6 +173,12 @@ private static void recordRateLimit(Chain chain, HttpResponse response) { } } + /** Retrying ends here: record the exit and hand the exception back to be thrown. */ + private static FanarException surfaced(Chain chain, int attempt, FanarException e) { + recordRetryCount(chain, attempt - 1); + return e; + } + private static void recordRetryCount(Chain chain, int retries) { chain.observation().attribute(FanarObservationAttributes.FANAR_RETRY_COUNT, retries); } diff --git a/core/src/test/java/qa/fanar/core/FanarClientRetryIntegrationTest.java b/core/src/test/java/qa/fanar/core/FanarClientRetryIntegrationTest.java index 1320ecf..bf393b3 100644 --- a/core/src/test/java/qa/fanar/core/FanarClientRetryIntegrationTest.java +++ b/core/src/test/java/qa/fanar/core/FanarClientRetryIntegrationTest.java @@ -209,6 +209,38 @@ void disabledPolicyStillMapsErrorsButNeverRetries() { assertEquals(1, server.hits()); } + // --- total sleep budget (ADR-027) --------------------------------------------------------- + + @Test + void retryAfterHintsBeyondTheTotalBudgetEndRetrying() { + // Two 429s each asking for 1 s: each within the 1 s ceiling, so ADR-025 alone would sleep + // both. A 1 s total budget admits the first hint (exactly the budget) and refuses the second + // — the exception surfaces after one retry, hint preserved, no further request is made. + server.enqueue( + Reply.of(429, "slow down", Map.of("Retry-After", "1")), + Reply.of(429, "slow down again", Map.of("Retry-After", "1"))); + RecordingObservability obs = new RecordingObservability(); + RetryPolicy budgeted = RetryPolicy.builder() + .maxDelay(Duration.ofSeconds(1)) + .maxTotalDelay(Duration.ofSeconds(1)) + .build(); + + long started = System.nanoTime(); + FanarRateLimitException ex; + try (FanarClient client = client(budgeted, obs)) { + ex = assertThrows(FanarRateLimitException.class, () -> client.chat().send(ping())); + } + long elapsedMs = (System.nanoTime() - started) / 1_000_000L; + + assertEquals(2, server.hits(), "one retry, then the budget ends it"); + assertEquals(Duration.ofSeconds(1), ex.retryAfter(), "the refused hint is preserved"); + assertTrue(elapsedMs >= 950, "the first hint must be slept, elapsed " + elapsedMs + " ms"); + assertTrue(elapsedMs < 5_000, "the second must not, elapsed " + elapsedMs + " ms"); + assertEquals(List.of("retry_attempt"), obs.events); + assertEquals(1, obs.attributes.get(FanarObservationAttributes.FANAR_RETRY_COUNT)); + assertSame(ex, obs.errors.getFirst()); + } + // --- rate-limit visibility (ADR-026) ------------------------------------------------------ @Test diff --git a/core/src/test/java/qa/fanar/core/RetryPolicyTest.java b/core/src/test/java/qa/fanar/core/RetryPolicyTest.java index 0f17f8d..83dd1cf 100644 --- a/core/src/test/java/qa/fanar/core/RetryPolicyTest.java +++ b/core/src/test/java/qa/fanar/core/RetryPolicyTest.java @@ -1,6 +1,7 @@ package qa.fanar.core; import java.time.Duration; +import java.util.function.Predicate; import java.time.temporal.ChronoUnit; import java.util.stream.Stream; @@ -26,6 +27,7 @@ void defaultsMatchAdr014() { assertEquals(3, p.maxAttempts()); assertEquals(Duration.ofMillis(500), p.baseDelay()); assertEquals(Duration.ofSeconds(30), p.maxDelay()); + assertEquals(Duration.ofMinutes(1), p.maxTotalDelay(), "the worst case the other defaults allow (ADR-027)"); assertEquals(2.0, p.backoffMultiplier()); assertEquals(JitterStrategy.FULL, p.jitter()); } @@ -54,56 +56,56 @@ void disabledIsStillValid() { @Test void rejectsMaxAttemptsBelowOne() { assertThrows(IllegalArgumentException.class, () -> - new RetryPolicy(0, Duration.ofMillis(500), Duration.ofSeconds(30), 2.0, + new RetryPolicy(0, Duration.ofMillis(500), Duration.ofSeconds(30), Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, e -> true)); } @Test void rejectsNullBaseDelay() { assertThrows(NullPointerException.class, () -> - new RetryPolicy(3, null, Duration.ofSeconds(30), 2.0, + new RetryPolicy(3, null, Duration.ofSeconds(30), Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, e -> true)); } @Test void rejectsNegativeBaseDelay() { assertThrows(IllegalArgumentException.class, () -> - new RetryPolicy(3, Duration.ofMillis(-500), Duration.ofSeconds(30), 2.0, + new RetryPolicy(3, Duration.ofMillis(-500), Duration.ofSeconds(30), Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, e -> true)); } @Test void rejectsZeroBaseDelay() { assertThrows(IllegalArgumentException.class, () -> - new RetryPolicy(3, Duration.ZERO, Duration.ofSeconds(30), 2.0, + new RetryPolicy(3, Duration.ZERO, Duration.ofSeconds(30), Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, e -> true)); } @Test void rejectsNullMaxDelay() { assertThrows(NullPointerException.class, () -> - new RetryPolicy(3, Duration.ofMillis(500), null, 2.0, + new RetryPolicy(3, Duration.ofMillis(500), null, Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, e -> true)); } @Test void rejectsZeroMaxDelay() { assertThrows(IllegalArgumentException.class, () -> - new RetryPolicy(3, Duration.ofMillis(500), Duration.ZERO, 2.0, + new RetryPolicy(3, Duration.ofMillis(500), Duration.ZERO, Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, e -> true)); } @Test void rejectsNegativeMaxDelay() { assertThrows(IllegalArgumentException.class, () -> - new RetryPolicy(3, Duration.ofMillis(500), Duration.ofMillis(-30), 2.0, + new RetryPolicy(3, Duration.ofMillis(500), Duration.ofMillis(-30), Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, e -> true)); } @Test void rejectsMaxDelayLessThanBaseDelay() { assertThrows(IllegalArgumentException.class, () -> - new RetryPolicy(3, Duration.ofSeconds(30), Duration.ofMillis(500), 2.0, + new RetryPolicy(3, Duration.ofSeconds(30), Duration.ofMillis(500), Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, e -> true)); } @@ -115,28 +117,67 @@ void rejectsMaxDelayNotRepresentableInMilliseconds() { RetryPolicy.defaults().withMaxDelay(ChronoUnit.FOREVER.getDuration())); assertThrows(IllegalArgumentException.class, () -> RetryPolicy.defaults().withMaxDelay(Duration.ofSeconds(Long.MAX_VALUE))); + // The budget must keep up with the ceiling (ADR-027): raise it first. assertEquals(Duration.ofMillis(Long.MAX_VALUE - 1), - RetryPolicy.defaults().withMaxDelay(Duration.ofMillis(Long.MAX_VALUE - 1)).maxDelay()); + RetryPolicy.defaults() + .withMaxTotalDelay(Duration.ofMillis(Long.MAX_VALUE - 1)) + .withMaxDelay(Duration.ofMillis(Long.MAX_VALUE - 1)) + .maxDelay()); + } + + @Test + void rejectsNullMaxTotalDelay() { + assertThrows(NullPointerException.class, () -> + new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), null, 2.0, + JitterStrategy.FULL, e -> true)); + } + + @Test + void rejectsZeroMaxTotalDelay() { + assertThrows(IllegalArgumentException.class, () -> + new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), Duration.ZERO, 2.0, + JitterStrategy.FULL, e -> true)); + } + + @Test + void rejectsNegativeMaxTotalDelay() { + assertThrows(IllegalArgumentException.class, () -> + new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), Duration.ofSeconds(-1), 2.0, + JitterStrategy.FULL, e -> true)); + } + + @Test + void rejectsMaxTotalDelayBelowMaxDelay() { + assertThrows(IllegalArgumentException.class, () -> + new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), Duration.ofSeconds(29), 2.0, + JitterStrategy.FULL, e -> true)); + } + + @Test + void rejectsMaxTotalDelayNotRepresentableInMilliseconds() { + assertThrows(IllegalArgumentException.class, () -> + new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), Duration.ofMillis(Long.MAX_VALUE), 2.0, + JitterStrategy.FULL, e -> true)); } @Test void rejectsBackoffMultiplierBelowOne() { assertThrows(IllegalArgumentException.class, () -> - new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), 0.5, + new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), Duration.ofMinutes(1), 0.5, JitterStrategy.FULL, e -> true)); } @Test void rejectsNullJitter() { assertThrows(NullPointerException.class, () -> - new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), 2.0, + new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), Duration.ofMinutes(1), 2.0, null, e -> true)); } @Test void rejectsNullRetryable() { assertThrows(NullPointerException.class, () -> - new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), 2.0, + new RetryPolicy(3, Duration.ofMillis(500), Duration.ofSeconds(30), Duration.ofMinutes(1), 2.0, JitterStrategy.FULL, null)); } @@ -149,6 +190,7 @@ void withMaxAttemptsReplacesOnlyThatField() { assertEquals(5, q.maxAttempts()); assertEquals(p.baseDelay(), q.baseDelay()); assertEquals(p.maxDelay(), q.maxDelay()); + assertEquals(p.maxTotalDelay(), q.maxTotalDelay()); assertEquals(p.backoffMultiplier(), q.backoffMultiplier()); assertEquals(p.jitter(), q.jitter()); assertSame(p.retryable(), q.retryable()); @@ -171,6 +213,17 @@ void withMaxDelayReplacesOnlyThatField() { assertEquals(p.maxAttempts(), q.maxAttempts()); } + @Test + void withMaxTotalDelayReplacesOnlyThatField() { + RetryPolicy p = RetryPolicy.defaults(); + RetryPolicy q = p.withMaxTotalDelay(Duration.ofMinutes(5)); + assertEquals(Duration.ofMinutes(5), q.maxTotalDelay()); + assertEquals(p.maxDelay(), q.maxDelay()); + assertEquals(p.maxAttempts(), q.maxAttempts()); + assertThrows(IllegalArgumentException.class, () -> p.withMaxTotalDelay(Duration.ofSeconds(1)), + "a budget below maxDelay is rejected on the derived record too"); + } + @Test void withBackoffMultiplierReplacesOnlyThatField() { RetryPolicy p = RetryPolicy.defaults(); @@ -205,6 +258,37 @@ void withMethodsRevalidate() { RetryPolicy.defaults().withJitter(null)); } + // --- builder (ADR-027) + + @Test + void builderStartsFromTheDefaults() { + assertEquals(RetryPolicy.defaults(), RetryPolicy.builder().build()); + } + + @Test + void builderSetsEveryKnob() { + Predicate never = e -> false; + RetryPolicy p = RetryPolicy.builder() + .maxAttempts(5) + .baseDelay(Duration.ofMillis(200)) + .maxDelay(Duration.ofSeconds(10)) + .maxTotalDelay(Duration.ofSeconds(25)) + .backoffMultiplier(3.0) + .jitter(JitterStrategy.EQUAL) + .retryable(never) + .build(); + assertEquals(new RetryPolicy(5, Duration.ofMillis(200), Duration.ofSeconds(10), Duration.ofSeconds(25), + 3.0, JitterStrategy.EQUAL, never), p); + } + + @Test + void builderValidatesOnBuildLikeTheConstructor() { + assertThrows(IllegalArgumentException.class, () -> RetryPolicy.builder().maxAttempts(0).build()); + assertThrows(IllegalArgumentException.class, () -> RetryPolicy.builder().maxTotalDelay(Duration.ofSeconds(1)).build(), + "below the default maxDelay of 30 s"); + assertThrows(NullPointerException.class, () -> RetryPolicy.builder().retryable(null).build()); + } + // --- isDefaultRetryable matrix (ADR-014) @ParameterizedTest(name = "{0}") diff --git a/core/src/test/java/qa/fanar/core/internal/retry/RetryInterceptorTest.java b/core/src/test/java/qa/fanar/core/internal/retry/RetryInterceptorTest.java index 943ec7e..a502420 100644 --- a/core/src/test/java/qa/fanar/core/internal/retry/RetryInterceptorTest.java +++ b/core/src/test/java/qa/fanar/core/internal/retry/RetryInterceptorTest.java @@ -248,7 +248,7 @@ void fullJitterOnZeroBackoffYieldsZero() { }; RetryPolicy policy = new RetryPolicy( 2, - Duration.ofNanos(1), Duration.ofNanos(1), 1.0, + Duration.ofNanos(1), Duration.ofNanos(1), Duration.ofNanos(1), 1.0, JitterStrategy.FULL, RetryPolicy::isDefaultRetryable); new RetryInterceptor(policy, sleeper, throwingRng).intercept(baseRequest(), chain); @@ -286,7 +286,7 @@ void equalJitterOnZeroBackoffYieldsZero() { }; RetryPolicy policy = new RetryPolicy( 2, - Duration.ofNanos(1), Duration.ofNanos(1), 1.0, + Duration.ofNanos(1), Duration.ofNanos(1), Duration.ofNanos(1), 1.0, JitterStrategy.EQUAL, RetryPolicy::isDefaultRetryable); new RetryInterceptor(policy, sleeper, throwingRng).intercept(baseRequest(), chain); @@ -431,6 +431,78 @@ void predicateOptingIntoQuotaGetsTheSameHintSemantics() { assertEquals(0, noSleep.sleepCount()); } + // --- total sleep budget (ADR-027) + + @Test + void totalDelayBudgetExactlyReachedIsStillHonoured() { + RecordingChain chain = new RecordingChain(List.of( + new FanarOverloadedException("busy"), + new FanarOverloadedException("still busy"), + stubResponse())); + RecordingSleeper sleeper = new RecordingSleeper(); + RetryPolicy policy = RetryPolicy.builder() + .jitter(JitterStrategy.NONE) + .baseDelay(Duration.ofMillis(100)) + .maxDelay(Duration.ofMillis(100)) + .maxTotalDelay(Duration.ofMillis(200)) + .build(); + + new RetryInterceptor(policy, sleeper, deterministicRandom()).intercept(baseRequest(), chain); + + assertEquals(List.of(Duration.ofMillis(100), Duration.ofMillis(100)), sleeper.sleeps(), + "two sleeps summing to exactly the budget are both taken"); + assertEquals(3, chain.calls()); + assertEquals(2, chain.recorder().retryCount()); + } + + @Test + void totalDelayBudgetExceededAbortsBeforeSleeping() { + FanarOverloadedException second = new FanarOverloadedException("still busy"); + RecordingChain chain = new RecordingChain(List.of( + new FanarOverloadedException("busy"), + second, + stubResponse())); + RecordingSleeper sleeper = new RecordingSleeper(); + RetryPolicy policy = RetryPolicy.builder() + .jitter(JitterStrategy.NONE) + .baseDelay(Duration.ofMillis(100)) + .maxDelay(Duration.ofMillis(100)) + .maxTotalDelay(Duration.ofMillis(150)) + .build(); + + FanarOverloadedException thrown = assertThrows(FanarOverloadedException.class, () -> + new RetryInterceptor(policy, sleeper, deterministicRandom()).intercept(baseRequest(), chain)); + + assertSame(second, thrown, "the exception that would have been retried surfaces"); + assertEquals(List.of(Duration.ofMillis(100)), sleeper.sleeps(), + "the second sleep would take the total to 200 ms > 150 ms: never started"); + assertEquals(2, chain.calls(), "the attempt after the abort is never made"); + assertEquals(List.of("retry_attempt"), chain.recorder().events(), "one retry happened, the abort is not one"); + assertEquals(1, chain.recorder().retryCount(), "the exit is recorded like every other"); + } + + @Test + void retryAfterHintBeyondTheRemainingBudgetSurfacesWithTheHintPreserved() { + // Each hint is within maxDelay (30 s), so ADR-025 alone would sleep twice; the 30 s total + // budget admits the first 20 s hint and refuses the second (40 s > 30 s). + Duration hint = Duration.ofSeconds(20); + FanarRateLimitException second = new FanarRateLimitException("slow down again", hint); + RecordingChain chain = new RecordingChain(List.of( + new FanarRateLimitException("slow down", hint), + second, + stubResponse())); + RecordingSleeper sleeper = new RecordingSleeper(); + RetryPolicy policy = RetryPolicy.defaults().withMaxTotalDelay(Duration.ofSeconds(30)); + + FanarRateLimitException thrown = assertThrows(FanarRateLimitException.class, () -> + new RetryInterceptor(policy, sleeper, deterministicRandom()).intercept(baseRequest(), chain)); + + assertSame(second, thrown); + assertEquals(hint, thrown.retryAfter(), "hint preserved for caller-side scheduling"); + assertEquals(List.of(hint), sleeper.sleeps(), "only the first hint was slept"); + assertEquals(2, chain.calls()); + } + // --- rate-limit visibility (ADR-026) @Test diff --git a/docs/API_SKETCH.md b/docs/API_SKETCH.md index 19b62ac..c1df68c 100644 --- a/docs/API_SKETCH.md +++ b/docs/API_SKETCH.md @@ -372,16 +372,25 @@ Three adapters ship: `fanar-obs-slf4j`, `fanar-obs-otel`, `fanar-obs-micrometer` ## 10. Custom retry policy ```java +import qa.fanar.core.JitterStrategy; import qa.fanar.core.RetryPolicy; import java.time.Duration; -RetryPolicy aggressive = RetryPolicy.defaults() - .withMaxAttempts(5) - .withBaseDelay(Duration.ofMillis(200)) - .withMaxDelay(Duration.ofSeconds(10)); // also caps honoured Retry-After hints (ADR-025) +RetryPolicy aggressive = RetryPolicy.builder() // starts from defaults() (ADR-027) + .maxAttempts(5) + .baseDelay(Duration.ofMillis(200)) + .maxDelay(Duration.ofSeconds(10)) // also caps honoured Retry-After hints (ADR-025) + .maxTotalDelay(Duration.ofSeconds(30)) // never sleep more than 30 s in total for one call + .build(); + +// Bridge a full per-minute window instead of failing fast above 30 s — the budget must keep up: +RetryPolicy patient = RetryPolicy.builder() + .maxDelay(Duration.ofSeconds(60)) + .maxTotalDelay(Duration.ofMinutes(2)) + .build(); -// Bridge a full per-minute window instead of failing fast above 30 s: -RetryPolicy patient = RetryPolicy.defaults().withMaxDelay(Duration.ofSeconds(60)); +// One-off variants of an existing policy: +RetryPolicy quieter = patient.withJitter(JitterStrategy.EQUAL); FanarClient client = FanarClient.builder() .apiKey(System.getenv("FANAR_API_KEY")) @@ -416,6 +425,7 @@ fanar: max-attempts: 3 initial-backoff: 500ms max-delay: 30s # also the ceiling on honoured Retry-After hints (ADR-025) + max-total-delay: 1m # budget for the sum of all sleeps within one call (ADR-027) wire-logging: level: BASIC # NONE | BASIC | HEADERS | BODY ``` diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 877a0f1..ce58045 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -208,7 +208,7 @@ Streamed TTS (`client.audio().speechStream(request)`) follows the same shape wit | JSON codec | `FanarJsonCodec` via `.jsonCodec(codec)` or `ServiceLoader` | `ServiceLoader` discovery; **loud error** at `build()` if none found | | Observability | `ObservabilityPlugin` via `.observability(plugin)` | No-op plugin | | Interceptors | `.addInterceptor(i)` (registration order = chain order) | `RetryInterceptor` (outermost; also the error boundary) + `BearerTokenInterceptor` (if apiKey set) | -| Retry policy | `.retryPolicy(policy)` | `RetryPolicy.defaults()` — 3 attempts, exponential + full jitter, 30 s cap (also the `Retry-After` ceiling, ADR-025) | +| Retry policy | `.retryPolicy(policy)` | `RetryPolicy.defaults()` — 3 attempts, exponential + full jitter, 30 s cap (also the `Retry-After` ceiling, ADR-025), 1 min total sleep budget per call (ADR-027); build variants with `RetryPolicy.builder()` | | User-Agent | `.userAgent(ua)` | `fanar-java/` | | Default headers | `.defaultHeader(name, value)` (repeated) | none | @@ -272,7 +272,7 @@ zone (ADR-018). | Extension SPIs | `qa.fanar.core.spi` | **implemented** (FanarJsonCodec, Interceptor+Chain, ObservabilityPlugin, ObservationHandle, FanarObservationAttributes) | | Default no-op observability | `qa.fanar.core.internal.observability` | **implemented** (NoopObservabilityPlugin, NoopObservationHandle) | | Composite observability | `qa.fanar.core.internal.observability.CompositeObservabilityPlugin` | **implemented** — produced by `ObservabilityPlugin.compose(...)`; fans out `start` / `attribute` / `event` / `error` / `child` to N children, merges `propagationHeaders` (last-write-wins on key collision) | -| Retry policy (public) | `qa.fanar.core.RetryPolicy` + `qa.fanar.core.JitterStrategy` | **implemented** — record + enum; validated at construction; `maxDelay` doubles as the `Retry-After` ceiling (ADR-025). The loop is `RetryInterceptor` below | +| Retry policy (public) | `qa.fanar.core.RetryPolicy` + `qa.fanar.core.JitterStrategy` | **implemented** — record + enum + `RetryPolicy.Builder`; validated at construction; `maxDelay` doubles as the `Retry-After` ceiling (ADR-025); `maxTotalDelay` budgets the sum of one call's sleeps (ADR-027). The loop is `RetryInterceptor` below | | HTTP transport | `qa.fanar.core.internal.transport` (`HttpTransport`, `DefaultHttpTransport`, `InterceptorChainImpl`, `ExceptionMapper`, `ErrorEnvelope`, `RateLimitHeaders`) | **implemented** — `RateLimitHeaders` is the one parser behind both `rateLimit()` and the `fanar.ratelimit.*` attributes (ADR-026) | | Bearer-token interceptor impl | `qa.fanar.core.internal.transport.BearerTokenInterceptor` | **implemented** — per-call `Supplier` for token rotation | | SSE parser | `qa.fanar.core.internal.sse` (`SseFrameAssembler`, `StreamEventDecoder`, `SseStreamPublisher`) | **implemented** — line-oriented accumulator, shape-routed decode, single-subscriber `Flow.Publisher` on a virtual thread | diff --git a/docs/PROJECT_STATE.md b/docs/PROJECT_STATE.md index 15ccbf7..92b4d69 100644 --- a/docs/PROJECT_STATE.md +++ b/docs/PROJECT_STATE.md @@ -21,9 +21,10 @@ does where it differs from the spec, each row pinned by a live test, plus the li the wire-log throw path (a `<-- failed` line when the chain throws, exception rethrown unchanged; ADR-012 amended), and rate-limit visibility (ADR-026: the `fanar.ratelimit.*` observation attributes on every response that carries the headers, `RateLimitInfo` via `rateLimit()` on both 429 exceptions, -Micrometer's cardinality rule). Still to come in 0.4.0: `RetryPolicy` builder + total retry budget -(ADR-027), facade plumbing consolidation, live-suite budget hygiene + nightly run, Maven Central -readiness. +Micrometer's cardinality rule), and the `RetryPolicy` total sleep budget + builder (ADR-027: +`maxTotalDelay`, `fanar.retry.max-total-delay`; the canonical constructor's arity changed — the +cycle's one breaking change). Still to come in 0.4.0: facade plumbing consolidation, live-suite +budget hygiene + nightly run, Maven Central readiness. ## Planned diff --git a/docs/adr/014-retry-policy-defaults.md b/docs/adr/014-retry-policy-defaults.md index 075c55b..54318f0 100644 --- a/docs/adr/014-retry-policy-defaults.md +++ b/docs/adr/014-retry-policy-defaults.md @@ -1,6 +1,6 @@ # ADR-014 — Retry policy defaults -- **Status**: Accepted (amended 2026-08-28 — see [Amendments](#amendments)) +- **Status**: Accepted (amended 2026-08-28 and 2026-08-29 — see [Amendments](#amendments)) - **Date**: 2026-04-23 - **Deciders**: @omahjoub (initial design) @@ -151,6 +151,17 @@ retry the handshake only, never re-request mid-stream — is unchanged and now p named in each section. This is the 0.4.0 rule: an ADR names the `*IntegrationTest` that proves what it promises (CONTRIBUTING → Testing). +### 2026-08-29 — a total sleep budget and a builder (0.4.0, ADR-027) + +The policy bounded each sleep and the number of attempts but not their sum. ADR-027 adds +`maxTotalDelay` (default 1 min — the worst case the other defaults already allowed, so nothing +changes at the defaults): a sleep that would push one call's cumulative sleep over the budget is +never started, retrying ends and the exception surfaces with its hint preserved, mirroring the +ADR-025 ceiling. The customization API gains `RetryPolicy.builder()` (from `defaults()`) and +`withMaxTotalDelay`; the canonical constructor's arity changes — a pre-1.0 break under ADR-019, +recorded in the changelog. The bounds that end retrying regardless of `retryable` are therefore +three: `maxAttempts`, `maxDelay`, `maxTotalDelay`. + ## References - ADR-006 Unchecked exception hierarchy (typed `ErrorCode` mapping) diff --git a/docs/adr/020-spring-boot-4-starter.md b/docs/adr/020-spring-boot-4-starter.md index 986e6ca..48d6dc7 100644 --- a/docs/adr/020-spring-boot-4-starter.md +++ b/docs/adr/020-spring-boot-4-starter.md @@ -1,6 +1,6 @@ # ADR-020 — Spring Boot 4 starter shape -- **Status**: Accepted (amended 2026-08-28 — see [Amendments](#amendments)) +- **Status**: Accepted (amended 2026-08-28 and 2026-08-29 — see [Amendments](#amendments)) - **Date**: 2026-04-26 - **Deciders**: @omahjoub @@ -77,6 +77,15 @@ a `@ConditionalOnMissingBean RetryPolicy` bean consumed by the `FanarClient` bea `FanarClient` bean is no longer the only escape. The sample `application.yml` and the API sketch list the new knob. +### 2026-08-29 — `fanar.retry.max-total-delay` (0.4.0, ADR-027) + +The retry budget of ADR-027 reaches Spring configuration as `fanar.retry.max-total-delay` +(default `1m`). The `fanarRetryPolicy` bean is now built through `RetryPolicy.builder()` — the +construction path that survives future knobs — so the four knobs validate together: a +`max-delay` raised above the budget fails the context at startup with the policy's own message +instead of misconfiguring the client silently. The sample `application.yml` and the API sketch +list the knob. + ## References - [`fanar-spring-boot-4-starter`](../../spring-boot-4-starter/) — the module. diff --git a/docs/adr/027-retry-policy-builder-and-budget.md b/docs/adr/027-retry-policy-builder-and-budget.md new file mode 100644 index 0000000..d509375 --- /dev/null +++ b/docs/adr/027-retry-policy-builder-and-budget.md @@ -0,0 +1,106 @@ +# ADR-027 — `RetryPolicy`: a total sleep budget and a builder + +- **Status**: Accepted +- **Date**: 2026-08-29 +- **Deciders**: @omahjoub + +## Context + +`RetryPolicy` bounds each sleep (`maxDelay`, also the `Retry-After` ceiling since ADR-025) and the +number of attempts, but nothing bounds the *sum*. With the defaults the worst case is two sleeps of +30 s; a user who raises `maxDelay` to bridge a full per-minute window (the ADR-025 recipe) and +`maxAttempts` to five can hold the caller's thread for four minutes of sleeping, and every honoured +`Retry-After` hint adds its full length. The SDK is sync-primary (ADR-004): that thread is the +caller's. + +`RetryPolicy` is a record whose canonical constructor is public API: every knob added changes its +arity, so adding a budget is a breaking change by construction. The `with*` methods do not help a +first-time construction, and the starter builds its bean positionally too. Pre-1.0 (ADR-019) is +the window in which this costs the least — the library is not yet on Maven Central. + +## Decision + +1. **`maxTotalDelay`, a new record component.** The budget for the sum of all sleeps within one + call. Before each sleep the retry loop checks `slept + next ≤ maxTotalDelay`, where `next` is + the computed back-off (jitter applied) or the honoured `Retry-After` hint; a sleep that would + exceed the budget is never started — retrying ends and the exception surfaces with the hint + preserved, exactly like the ADR-025 ceiling. A total that lands exactly on the budget is + honoured. The exit is observable as every other: `fanar.retry_count` is recorded, no + `retry_attempt` event is emitted for the refused sleep. +2. **Default 1 minute** — the worst case the other defaults already allowed (two sleeps of at most + 30 s). No behaviour changes at the defaults; the budget bites only once `maxAttempts` or + `maxDelay` is raised. +3. **Validation**: positive, ≥ `maxDelay` (so at least one maximal sleep fits — a budget below the + ceiling would make `maxDelay` unreachable), representable in milliseconds. Validated in the + compact constructor with the other invariants; `with*` and the builder revalidate. +4. **`RetryPolicy.builder()`**, starting from `defaults()`, with one setter per knob and `build()` + validating through the canonical constructor. `withMaxTotalDelay` joins the `with*` family. The + builder is the recommended way to construct a policy from now on: knobs added later change the + canonical constructor's arity, never the builder. +5. **The canonical constructor changes arity** (`maxTotalDelay` after `maxDelay`). **Breaking** + under ADR-019, called out in the changelog with the migration (use the builder, or insert the + new argument). No compatibility constructor: keeping the six-argument one would keep the + positional trap public beside the builder that exists to end it. +6. **Starter**: `fanar.retry.max-total-delay` (default `1m`) on `FanarProperties.Retry`; the + `fanarRetryPolicy` bean is built through the builder so the four knobs validate together and a + `max-delay` raised above the budget fails the context at startup instead of silently + misconfiguring the client (amends ADR-020). + +`retryable` stays `Predicate`; an attempt- or elapsed-aware predicate is parked +(plan, out of scope). + +## Alternatives considered + +- **Defer to the 1.0 API-freeze pass.** *Rejected*: the constructor break only gets more expensive + with each consumer; the feature is small and its default is invisible. +- **Keep a six-argument compatibility constructor.** *Rejected*: it preserves the positional + constructor that made every knob a breaking change, next to the builder introduced to end that. + ADR-019 exists for exactly this kind of break. +- **A wall-clock deadline including request time** (`maxElapsed`). *Rejected*: request time is + governed by the transport's connect and request timeouts; folding it into the retry budget + double-counts and makes the guarantee depend on the server's latency rather than the SDK's own + sleeping. +- **Clamp the last sleep to the remaining budget instead of refusing it.** *Rejected*: it would + retry a `Retry-After` hint early — the same premature re-request ADR-025 refused. +- **Count the budget across calls (a client-wide token bucket).** *Rejected*: policy the SDK should + not choose; a user interceptor with the ADR-026 data can implement one. + +## Consequences + +### Positive +- An upper bound on the time one call spends sleeping, independent of how the other knobs are set + and of how many hints the server sends. +- A stable construction path (`builder()`) for every future knob. + +### Negative / Trade-offs +- The canonical constructor break: positional `new RetryPolicy(...)` calls need a seventh argument + or a move to the builder. Zero known external consumers at the time of the change. +- One more invariant (`maxTotalDelay ≥ maxDelay`): raising `maxDelay` past 1 min now requires + raising the budget too — deliberate, and validated loudly at construction (and at Spring startup). + +### Neutral +- `RetryPolicy.disabled()` carries the default budget like every other unused knob. +- `equals` / `hashCode` include the new component; `defaults()` stays equal to + `builder().build()`. + +## Proved by + +- `FanarClientRetryIntegrationTest.retryAfterHintsBeyondTheTotalBudgetEndRetrying` — two 429s with + `Retry-After: 1` under a 1 s ceiling and a 1 s budget through the public builder: the first hint + is slept (exactly the budget), the second is refused, the exception surfaces with the hint, no + further request is made. +- `RetryInterceptorTest.totalDelayBudgetExactlyReachedIsStillHonoured`, + `.totalDelayBudgetExceededAbortsBeforeSleeping`, + `.retryAfterHintBeyondTheRemainingBudgetSurfacesWithTheHintPreserved`. +- `RetryPolicyTest` — validation (`rejects*MaxTotalDelay*`), `withMaxTotalDelayReplacesOnlyThatField`, + the `builder*` cases (`defaults()` equals `builder().build()`). +- `FanarAutoConfigurationTest` — the knob's default and override, `retryKnobsAreValidatedTogetherAtStartup`. + +## References + +- ADR-004 Sync-primary API with async sugar (why a sleeping thread is the caller's) +- ADR-014 Retry policy defaults (amended 2026-08-29 by this record) +- ADR-019 Pre-1.0 stability policy (the constructor break) +- ADR-020 Spring Boot 4 starter shape (amended 2026-08-29: `fanar.retry.max-total-delay`) +- ADR-025 Retry-After handling (the per-sleep ceiling this budget complements) +- ADR-026 Rate-limit visibility (the data a client-wide throttle would use instead) diff --git a/docs/adr/INDEX.md b/docs/adr/INDEX.md index 506b590..e420054 100644 --- a/docs/adr/INDEX.md +++ b/docs/adr/INDEX.md @@ -48,6 +48,7 @@ Every ADR follows an extended Michael Nygard template: - [014 — Retry policy defaults](014-retry-policy-defaults.md) - [025 — Retry-After handling: ceiling, normalisation, and the quota hint](025-retry-after-handling.md) - [026 — Rate-limit visibility: observation attributes and `RateLimitInfo` on the 429s](026-rate-limit-telemetry.md) +- [027 — `RetryPolicy`: a total sleep budget and a builder](027-retry-policy-builder-and-budget.md) ### Build, distribution, governance diff --git a/spring-boot-4-sample/src/main/resources/application.yml b/spring-boot-4-sample/src/main/resources/application.yml index d1df5ae..f4503a7 100644 --- a/spring-boot-4-sample/src/main/resources/application.yml +++ b/spring-boot-4-sample/src/main/resources/application.yml @@ -10,6 +10,7 @@ fanar: # max-attempts: 3 # initial-backoff: 500ms # max-delay: 30s # also the ceiling on honoured Retry-After hints (ADR-025) + # max-total-delay: 1m # budget for the sum of all sleeps within one call (ADR-027) wire-logging: level: BASIC # NONE | BASIC | HEADERS | BODY — auto-wires the WireLoggingInterceptor diff --git a/spring-boot-4-starter/src/main/java/qa/fanar/spring/boot/v4/FanarAutoConfiguration.java b/spring-boot-4-starter/src/main/java/qa/fanar/spring/boot/v4/FanarAutoConfiguration.java index e047c6a..c4d1fc0 100644 --- a/spring-boot-4-starter/src/main/java/qa/fanar/spring/boot/v4/FanarAutoConfiguration.java +++ b/spring-boot-4-starter/src/main/java/qa/fanar/spring/boot/v4/FanarAutoConfiguration.java @@ -54,21 +54,20 @@ FanarJsonCodec fanarJsonCodec() { * Retry policy from the {@code fanar.retry.*} knobs on top of {@link RetryPolicy#defaults()}. * Replaced if the user declares their own {@link RetryPolicy} bean — the route to a custom * {@code retryable} predicate, jitter strategy or multiplier without re-wiring the client. - * Built through the canonical constructor so the three knobs are validated together (a - * raised {@code initial-backoff} needs a {@code max-delay} at least as large). + * Built through {@link RetryPolicy#builder()} so the four knobs are validated together (a + * raised {@code initial-backoff} needs a {@code max-delay} at least as large, and a raised + * {@code max-delay} a {@code max-total-delay} at least as large — ADR-027). */ @Bean @ConditionalOnMissingBean RetryPolicy fanarRetryPolicy(FanarProperties props) { FanarProperties.Retry retry = props.retry(); - RetryPolicy defaults = RetryPolicy.defaults(); - return new RetryPolicy( - retry.maxAttempts(), - retry.initialBackoff(), - retry.maxDelay(), - defaults.backoffMultiplier(), - defaults.jitter(), - defaults.retryable()); + return RetryPolicy.builder() + .maxAttempts(retry.maxAttempts()) + .baseDelay(retry.initialBackoff()) + .maxDelay(retry.maxDelay()) + .maxTotalDelay(retry.maxTotalDelay()) + .build(); } @Bean diff --git a/spring-boot-4-starter/src/main/java/qa/fanar/spring/boot/v4/FanarProperties.java b/spring-boot-4-starter/src/main/java/qa/fanar/spring/boot/v4/FanarProperties.java index bda6d52..0b5daf0 100644 --- a/spring-boot-4-starter/src/main/java/qa/fanar/spring/boot/v4/FanarProperties.java +++ b/spring-boot-4-starter/src/main/java/qa/fanar/spring/boot/v4/FanarProperties.java @@ -39,7 +39,7 @@ public record FanarProperties( /** * Retry policy knobs. Maps to {@link qa.fanar.core.RetryPolicy}; the defaults are the SDK's - * own ({@code RetryPolicy.defaults()}). Anything beyond these three — jitter, multiplier, a + * own ({@code RetryPolicy.defaults()}). Anything beyond these four — jitter, multiplier, a * custom retryable predicate — is a {@code RetryPolicy} bean, see * {@link FanarAutoConfiguration}. * @@ -50,11 +50,15 @@ public record FanarProperties( * @param maxDelay cap on the computed backoff and ceiling on honoured server * {@code Retry-After} hints — defaults to 30s; a hint above it ends * retrying and the exception surfaces with the hint preserved (ADR-025) + * @param maxTotalDelay budget for the sum of all sleeps within one call — defaults to 1m; a + * sleep that would exceed it is never taken and the exception surfaces + * with the hint preserved (ADR-027); must be at least {@code maxDelay} */ public record Retry( @DefaultValue("3") int maxAttempts, @DefaultValue("500ms") Duration initialBackoff, - @DefaultValue("30s") Duration maxDelay + @DefaultValue("30s") Duration maxDelay, + @DefaultValue("1m") Duration maxTotalDelay ) { } /** diff --git a/spring-boot-4-starter/src/test/java/qa/fanar/spring/boot/v4/FanarAutoConfigurationTest.java b/spring-boot-4-starter/src/test/java/qa/fanar/spring/boot/v4/FanarAutoConfigurationTest.java index a51947b..ba3c9be 100644 --- a/spring-boot-4-starter/src/test/java/qa/fanar/spring/boot/v4/FanarAutoConfigurationTest.java +++ b/spring-boot-4-starter/src/test/java/qa/fanar/spring/boot/v4/FanarAutoConfigurationTest.java @@ -50,6 +50,7 @@ void defaultsApply() { assertThat(props.retry().maxAttempts()).isEqualTo(3); assertThat(props.retry().initialBackoff().toMillis()).isEqualTo(500); assertThat(props.retry().maxDelay().toSeconds()).isEqualTo(30); + assertThat(props.retry().maxTotalDelay()).isEqualTo(Duration.ofMinutes(1)); assertThat(props.wireLogging().level().name()).isEqualTo("NONE"); }); } @@ -64,6 +65,7 @@ void yamlOverridesAreApplied() { "fanar.retry.max-attempts=5", "fanar.retry.initial-backoff=250ms", "fanar.retry.max-delay=45s", + "fanar.retry.max-total-delay=5m", "fanar.wire-logging.level=BODY") .run(ctx -> { FanarProperties props = ctx.getBean(FanarProperties.class); @@ -73,6 +75,7 @@ void yamlOverridesAreApplied() { assertThat(props.retry().maxAttempts()).isEqualTo(5); assertThat(props.retry().initialBackoff().toMillis()).isEqualTo(250); assertThat(props.retry().maxDelay().toSeconds()).isEqualTo(45); + assertThat(props.retry().maxTotalDelay()).isEqualTo(Duration.ofMinutes(5)); assertThat(props.wireLogging().level().name()).isEqualTo("BODY"); }); } @@ -85,23 +88,40 @@ void defaultRetryPolicyBeanMirrorsTheSdkDefaults() { @Test void retryPolicyBeanReflectsTheRetryKnobs() { - // initial-backoff above the SDK's default cap used to fail at startup; the three knobs are - // now validated together. + // initial-backoff above the SDK's default cap used to fail at startup; the four knobs are + // now validated together (a max-delay of 90 s also needs max-total-delay >= 90 s). runner.withPropertyValues( "fanar.api-key=test-key", "fanar.retry.max-attempts=5", "fanar.retry.initial-backoff=45s", - "fanar.retry.max-delay=90s") + "fanar.retry.max-delay=90s", + "fanar.retry.max-total-delay=10m") .run(ctx -> { RetryPolicy policy = ctx.getBean(RetryPolicy.class); assertThat(policy.maxAttempts()).isEqualTo(5); assertThat(policy.baseDelay()).isEqualTo(Duration.ofSeconds(45)); assertThat(policy.maxDelay()).isEqualTo(Duration.ofSeconds(90)); + assertThat(policy.maxTotalDelay()).isEqualTo(Duration.ofMinutes(10)); assertThat(policy.jitter()).isEqualTo(RetryPolicy.defaults().jitter()); assertThat(policy.backoffMultiplier()).isEqualTo(RetryPolicy.defaults().backoffMultiplier()); }); } + @Test + void retryKnobsAreValidatedTogetherAtStartup() { + // max-delay raised above the default 1 m budget without raising max-total-delay: the + // policy's own validation fails the context, loudly, instead of a silent mis-configuration. + runner.withPropertyValues( + "fanar.api-key=test-key", + "fanar.retry.max-delay=2m") + .run(ctx -> { + assertThat(ctx).hasFailed(); + assertThat(ctx.getStartupFailure()).rootCause() + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("maxTotalDelay"); + }); + } + @Test void userDefinedRetryPolicyBeanWins() { runner.withPropertyValues("fanar.api-key=test-key", "fanar.retry.max-attempts=7")