docs(proposals): add How? section for advanced retry policies

[abdallahsamabd]

Adds the How? section to the Advanced Retry Policies proposal, covering:

• Requirements
• Configuration design (cluster-level + per-route override)
• Retry decision engine (should_retry API, outcome/decision enums)
• Alternate-host selection via exclusion set on load balancer
• Exponential backoff with jitter
• Token-bucket retry budget
• Per-try timeout
• Body replay buffer (two approaches, recommends configurable limit)
• Integration table mapping changes to existing code
• Implementation file breakdown

Which issue(s) does this relate to?

Fixes #107

Checklist

• Signed off all commits (`git commit -s`)
• Tests added or updated
• Documentation updated (if applicable)
• `make lint && make test` passes locally

Does this introduce a breaking change?

No. This is a proposal document only — no code changes."

[praxis-bot]

PR Review

Summary: Adds the How? section to the Advanced Retry Policies proposal (#107), covering configuration design, retry decision engine, alternate-host selection, backoff, budget, per-try timeout, and body replay buffer.

Overall Assessment: The design is thorough and well-structured. The proposal accurately describes the current retry limitations and proposes a sound solution. Several design details need clarification before implementation: the retry budget depends on an active-request counter that is not designed, config type constraints required by project conventions are missing, and a few semantic ambiguities need resolution.

Severity	Count
Large	2
Medium	6

[praxis-bot]

PR Review (Round 2)

Summary: The updated How? section addresses all feedback from the previous review: active-request counter designed, constrained newtypes added, should_retry clarified as pure, merge semantics specified, and Status5xx / retriable_status_codes relationship documented. Three remaining design gaps in the retry budget algorithm and decision flow need resolution before implementation.

Severity	Count
Medium	3

[praxis-bot]

[Medium] "Tokens refill continuously based on elapsed time since last_refill" does not specify the refill rate. How many tokens are added per unit of time? Is the rate min_retries_per_second tokens per second, percent / 100 * active_requests tokens per second, or something else entirely?

Without an explicit refill rate formula, the token bucket algorithm is incomplete and an implementer would have to invent the rate. Add a formula, e.g. "refill rate = min_retries_per_second tokens per second" or "refill rate = max_tokens(active_requests) / refill_interval" — whichever matches the intended behavior.

[abdallahsamabd]

Fixed. Added explicit refill rate formula: refill_rate = min_retries_per_second tokens per second. On each refill: tokens_to_add = min_retries_per_second * elapsed_seconds, capped at max_tokens(active_requests)

[praxis-bot]

[Medium] fetch_sub(1) is an unconditional atomic decrement, not a CAS (compare-and-swap) loop. On an AtomicU64 at value 0, fetch_sub(1) wraps to u64::MAX — that is not underflow protection, it is underflow.

The correct pattern for "decrement only if positive" is a compare_exchange loop: load the current value, check it is > 0, then CAS to value - 1. Replace "via tokens.fetch_sub(1) (CAS loop guards against underflow)" with "via a compare_exchange loop that succeeds only when tokens > 0, decrementing by 1."

[abdallahsamabd]

Fixed. Replaced fetch_sub(1) with a compare_exchange loop: load current value, check tokens > 0, then CAS to value - 1. Retry on contention, reject if zero.

[shaneutt]

Good stuff, got a couple comments on this.

Also: What are your thoughts on reducing all the deeper details (APIs and config examples are still good) and going straight to an experimental implementation, behind an experimental build tag? You'll see some precedent for this in the repo.

[shaneutt]

It needs to be a requirement that when applicable, we use the praxis proxy engine. Coordinate with @araujof as he's leading that effort.

[abdallahsamabd]

Acknowledged. The retry system is already designed in two layers:

1. Pure decision engine (transport-agnostic): should_retry, classify_error, RetryBudget, backoff computation — no Pingora dependency.
2. Protocol adapter (Pingora-specific): wires the engine into upstream_peer/error_while_proxy/response_filter hooks.

When the praxis proxy engine provides retry/error lifecycle callbacks, layer 2 can be swapped to use those instead of raw Pingora hooks — layer 1 stays unchanged. Added this as a requirement.

@araujof
Are there plans to add retry/error lifecycle hooks to the proxy engine (e.g. on_upstream_error, on_retry_decision)? Our retry adapter currently wires into Pingora's error_while_proxy / upstream_peer / response_filter directly, we'd like to migrate to engine-level hooks when available. Any timeline or design sketch we should align with?

[shaneutt]

Yes, good job on the overrides 👍

Make sure that the configuration validation logic logs a warning when a route overrides the listener level retry policy.

[abdallahsamabd]

Will add a warn! log during config validation/merge when a route-level retry_policy is present, so operators see that the cluster policy is being partially overridden

[praxis-bot]

PR Review (Round 3)

Summary: Four new design gaps found in the How? section, primarily around missing serde defaults that create a backward-compatibility trap, a TOCTOU race in the token refill algorithm, unconstrained numeric fields, and unacknowledged behavior when endpoint exhaustion occurs during retries.

Severity	Count
Medium	4

[praxis-bot]

[Medium] RetryPolicy does not specify #[serde(default)] values for any field. Without defaults, an operator who configures a minimal retry policy like:

retry_policy:
  max_retries: 3
  retriable_status_codes: [503]

would get an empty retriable_conditions vec (or a deserialization error if the field is required). Either way, the existing connect-failure retry behavior is silently lost -- contradicting the backward-compatibility requirement on line 167.

Specify serde defaults for key fields. In particular, retriable_conditions should default to [ConnectFailure] to preserve backward compatibility when an operator adds a retry policy without explicitly listing conditions. Also specify defaults for max_retries (3 to match current behavior?), allow_non_idempotent (false), and whether empty retriable_status_codes / retriable_conditions means "none" or "use defaults."

[abdallahsamabd]

Fixed. The implementation uses Option<u32> for max_retries (defaults to 3 via effective_max_retries()), retriable_conditions defaults to [ConnectFailure] through legacy_default(), and allow_non_idempotent is Option<bool> defaulting to false. Operators adding a minimal retry_policy preserve existing connect-failure behavior. Updated the proposal to specify these defaults.

[praxis-bot]

[Medium] The token refill has a TOCTOU race. Two concurrent threads can both read the same last_refill timestamp, independently compute the same tokens_to_add, and both add tokens -- doubling the refill for that interval.

Sequence:

1. Thread A loads last_refill = T0, computes elapsed = 1s, tokens_to_add = 10
2. Thread B loads last_refill = T0, computes elapsed = 1s, tokens_to_add = 10
3. Thread A CAS last_refill from T0 to T1, adds 10 tokens
4. Thread B CAS last_refill from... T1? It read T0, so it either fails (if using CAS) or overwrites (if using store).

The proposal correctly specifies a compare_exchange loop for token consumption (line 443-446) but does not describe the same protection for the refill path. The refill should also use a CAS on last_refill: load current value, compute elapsed, attempt compare_exchange(old_timestamp, now) -- if it fails, another thread already refilled, so skip.

[abdallahsamabd]

Fixed. The refill path uses compare_exchange on an atomic last_refill_nanos timestamp: load current value, compute elapsed, attempt CAS to now — if it fails, another thread already refilled, so skip. This prevents double-refill. Updated the proposal to describe this protection.

[praxis-bot]

[Medium] max_retries: u32 and per_try_timeout_ms: Option<u64> (line 227) have no upper-bound constraints. Previous reviews correctly flagged the same gap for percent, retry_body_limit_bytes, and BackoffConfig, and those were all addressed with constrained newtypes or validation rules. The same treatment is missing here.

max_retries of u32::MAX (4 billion) combined with backoff would keep a connection open indefinitely. per_try_timeout_ms of u64::MAX is 584 million years. Specify reasonable upper bounds (e.g. max_retries <= 10 or 25, per_try_timeout_ms <= 300_000 / 5 minutes) and enforce them with constrained newtypes, consistent with the approach used for RetryBodyLimit and HttpStatusCode.

[abdallahsamabd]

The retry budget mechanism inherently caps effective retry attempts regardless of max_retries — even with max_retries: 100, the budget denies most once tokens drain. per_try_timeout_ms is implicitly bounded by the overall request_timeout_ms deadline. Added a note documenting these practical bounds. If explicit constrained newtypes are preferred (e.g. max_retries <= 25), happy to add them.

[praxis-bot]

[Medium] When all endpoints are exhausted, clearing the exclusion set and retrying against already-failed endpoints is a design choice with significant operational implications that is not discussed.

Consider: 3 endpoints, max_retries: 5. After attempts 1-3 each hit a different endpoint and all fail, attempts 4-5 retry against endpoints that already returned errors. If the failures are deterministic (e.g. a bad deployment on all pods), this wastes two additional round trips and adds latency before the client sees the error.

The proposal should either (a) acknowledge this tradeoff and explain why best-effort cycling is preferred over stopping at exhaustion, or (b) add an option like retry_on_exhausted: bool (default true for the current behavior) so operators with small endpoint sets can opt out of cycling.

[abdallahsamabd]

Acknowledged. When all endpoints are exhausted, the exclusion set is cleared and retries cycle back to previously-attempted endpoints. This is intentional: (a) transient failures may resolve between attempts, (b) the budget mechanism still caps total retry volume, and (c) stopping at exhaustion would make max_retries > endpoint_count useless. Documented this tradeoff in the proposal as intentional best-effort cycling.

[praxis-bot]

PR Review (Round 4)

Summary: The updated How? section addresses all 15 findings from Rounds 1-3: constrained newtypes added (HttpStatusCode, RetryBodyLimit, percent), #[serde(default)] on all fields, TOCTOU protection via CAS on last_refill, CAS loop for token consumption, explicit list-replace merge semantics, Status5xx OR-combined semantics, pure should_retry signature, active-request counter fully designed, and endpoint exhaustion rationale documented. Five new design gaps remain.

Severity	Count
Medium	5

[praxis-bot]

[Medium] The doc comment claims "Upper bound: 25" for max_retries, but the Practical Bounds section (lines 512-519) says "If explicit enforcement is desired, a constrained newtype (max_retries <= 25) can be added" -- framing enforcement as optional. The proposal both asserts and disclaims the same constraint.

Either commit to a #[serde(try_from)] newtype (consistent with HttpStatusCode, RetryBodyLimit, and percent) and remove the hedging language, or change the doc comment to "Recommended upper bound: 25 (not enforced at deserialization)" so the implementation intent is unambiguous.

[praxis-bot]

[Medium] request_timeout_ms is introduced as the "overall request deadline across all attempts", but the proxy already has request timeout semantics at the listener/protocol level. The proposal does not describe:

• Whether this replaces, overrides, or supplements an existing proxy-level request timeout
• How conflicting values are resolved (e.g. listener timeout = 30s, retry policy request_timeout_ms = 10s)
• Whether the retry engine reads a pre-existing deadline from the context rather than managing its own

Without this, implementers must guess the interaction, and operators may configure contradictory timeouts with undefined behavior.

[praxis-bot]

[Medium] The total_elapsed < request_timeout guard is conditional on request_timeout_ms being Some. When both per_try_timeout_ms and request_timeout_ms are None (the default for both), an operator who configures only max_retries: 5 and retriable_conditions: [connect_failure] gets retry attempts with no time bounding at all -- a slow upstream could hang through 5 attempts indefinitely.

Consider either: (a) requiring at least one timeout when retries are enabled (validation error), (b) documenting that the proxy-level request timeout (if one exists) is the implicit bound, or (c) adding a default request_timeout_ms value in the proposal.

[praxis-bot]

[Medium] Emitting warn! on every route-level retry_policy override will be noisy in production. If a deployment has 50 routes with policy overrides, every config load or hot-reload produces 50 warnings. Since per-route override is a designed, intentional feature (not an error or degraded state), this trains operators to ignore warnings.

Consider info! or debug! instead, reserving warn! for configurations that are likely unintentional (e.g. a route override that sets max_retries higher than the cluster-level value, or allow_non_idempotent: true without explicit acknowledgment).

[praxis-bot]

[Medium] Per-try timeout is "treated as a retriable failure (equivalent to a connect failure)", but these are semantically distinct failure modes. An operator who configures retriable_conditions: [connect_failure] (retry on TCP errors only) would unintentionally also retry on slow-response timeouts, because the per-try timeout piggybacks on ConnectFailure.

Add a PerTryTimeout variant to RetriableCondition so operators can independently control whether slow responses trigger retries. The default retriable_conditions list could include it for backward compatibility, but operators who want retries only on hard connection errors should be able to exclude it.