|
| 1 | +--- |
| 2 | +status: shipped |
| 3 | +date: 2026-06-23 |
| 4 | +slug: retry-policy-extraction |
| 5 | +summary: Extract a stateless _RetryPolicy decision module from the duplicated AsyncRetry/Retry __call__ loops. |
| 6 | +supersedes: null |
| 7 | +superseded_by: null |
| 8 | +pr: 76 |
| 9 | +outcome: Shipped via #76 — decision logic moved into a stateless _RetryPolicy.decide; AsyncRetry/Retry are now thin loop drivers, the ~110-line sync/async duplication is gone, behaviour byte-identical (718 tests, 100% coverage). New seam suite tests/test_retry_policy.py; promoted into architecture/resilience.md. Internal refactor — no release. |
| 10 | +--- |
| 11 | + |
| 12 | +# Design: Extract a deep `_RetryPolicy` decision module |
| 13 | + |
| 14 | +## Summary |
| 15 | + |
| 16 | +`AsyncRetry.__call__` and `Retry.__call__` hand-copy ~110 lines of retry |
| 17 | +*decision* logic — status eligibility, streaming-body refusal, exhaustion, |
| 18 | +Retry-After parsing, budget accounting, backoff — differing only in `await |
| 19 | +next` vs `next` and `asyncio.sleep` vs `time.sleep`. This change pulls the |
| 20 | +decision logic into a stateless private `_RetryPolicy` in the same module, so |
| 21 | +both wrappers shrink to a thin loop and the decision lives once. It mirrors |
| 22 | +the precedent already in the package: `CircuitBreaker`/`AsyncCircuitBreaker` |
| 23 | +share the lock-free `_CircuitBreakerState`. |
| 24 | + |
| 25 | +## Motivation |
| 26 | + |
| 27 | +- `retry.py:100-210` (`AsyncRetry.__call__`) and `retry.py:213-349` |
| 28 | + (`Retry.__call__`) are ~110 lines each, byte-identical except the `await`. |
| 29 | + Parity is hand-maintained; drift is undetectable. Both carry |
| 30 | + `# noqa: C901, PLR0912, PLR0915` to silence the complexity budget. |
| 31 | +- The package already proved the fix: `_CircuitBreakerState` |
| 32 | + (`circuit_breaker.py:131-310`) is a deep, synchronous, lock-free decision |
| 33 | + module that both breaker wrappers drive. Retry never got the same treatment. |
| 34 | +- **Depth:** the retry interface (the `Middleware` protocol — one `__call__`) |
| 35 | + is small, but the implementation is duplicated rather than deep. Moving the |
| 36 | + decision behind `_RetryPolicy.decide` concentrates it: one place to fix a |
| 37 | + retry bug (locality), one interface to test directly without `MockTransport` |
| 38 | + (leverage). |
| 39 | + |
| 40 | +## Non-goals |
| 41 | + |
| 42 | +- No behaviour change. The retry policy, defaults, events, notes, and raised |
| 43 | + exceptions stay byte-identical. |
| 44 | +- Not touching `RetryBudget`, `_backoff.full_jitter_delay`, or |
| 45 | + `_parse_retry_after` — they stay as-is. |
| 46 | +- Not unifying the sync/async wrappers themselves — the `await`/blocking split |
| 47 | + is fundamental and stays in the two thin `__call__` shells. |
| 48 | +- Not extending the same treatment to `Bulkhead` in this change. |
| 49 | + |
| 50 | +## Design |
| 51 | + |
| 52 | +### 1. `_RetryPolicy` — stateless decision module |
| 53 | + |
| 54 | +A private class in `retry.py`, holding **immutable config + the shared |
| 55 | +budget** and nothing per-call mutable. This is the faithful analog of |
| 56 | +`_CircuitBreakerState`: there the *circuit* is the shared state; here the |
| 57 | +shared state is the already-thread-safe `RetryBudget`, and `_RetryPolicy` is |
| 58 | +the decision logic around it. Because it carries no per-call field, it is |
| 59 | +trivially safe under the concurrent requests a single frozen middleware |
| 60 | +instance serves. |
| 61 | + |
| 62 | +It owns: |
| 63 | + |
| 64 | +- config: `max_attempts`, `base_delay`, `max_delay`, `retry_status_codes`, |
| 65 | + `retry_methods`, `respect_retry_after`, `budget`; |
| 66 | +- validation: `max_attempts < 1` → `ValueError` (raised when the wrapper |
| 67 | + builds the policy in `__init__`, so construction-time behaviour is |
| 68 | + unchanged); |
| 69 | +- the `_LOGGER` event emissions and PEP-678 note additions (side effects move |
| 70 | + here with the decision). |
| 71 | + |
| 72 | +### 2. The seam — one method |
| 73 | + |
| 74 | +```python |
| 75 | +def decide(self, *, attempt: int, request: httpx2.Request, exc: BaseException) -> float |
| 76 | +``` |
| 77 | + |
| 78 | +- **Returns** the `float` delay to sleep for the retry case. |
| 79 | +- **Raises** for every terminal case, having already added the note, emitted |
| 80 | + the event, and (for the budget case) constructed `RetryBudgetExhaustedError` |
| 81 | + with its `__cause__`. `decide` is called *inside* the wrapper's `except` |
| 82 | + block, so implicit `__context__` and explicit `raise ... from exc` chaining |
| 83 | + behave exactly as today — no manual `__cause__` fiddling. |
| 84 | + |
| 85 | +Classification is folded in (no separate predicate): derive `last_response` |
| 86 | +from `isinstance(exc, StatusError)`; apply method-eligibility and status-set |
| 87 | +membership; re-raise non-retryable failures unchanged; otherwise walk |
| 88 | +streaming-refusal → exhaustion → Retry-After-exceeds-`max_delay` → budget |
| 89 | +`try_withdraw` → delay (Retry-After value or `full_jitter_delay`). |
| 90 | + |
| 91 | +Rejected alternative: a `_Sleep | _Stop` sum type. It defers the raise to |
| 92 | +*after* the `except` block, losing the active exception context and forcing |
| 93 | +manual chain reconstruction — machinery that exists only to paper over that. |
| 94 | +Returning-a-delay-or-raising matches `_CircuitBreakerState.admit()`, which |
| 95 | +already raises `CircuitOpenError` rather than returning a rejected value. |
| 96 | + |
| 97 | +### 3. The wrappers shrink to a thin driver |
| 98 | + |
| 99 | +```python |
| 100 | +_RETRYABLE_EXCEPTIONS = (StatusError, NetworkError, TimeoutError) |
| 101 | + |
| 102 | +async def __call__(self, request: httpx2.Request, next: AsyncNext) -> httpx2.Response: |
| 103 | + self.budget.deposit() |
| 104 | + for attempt in range(self._policy.max_attempts): |
| 105 | + try: |
| 106 | + return await next(request) |
| 107 | + except _RETRYABLE_EXCEPTIONS as exc: |
| 108 | + delay = self._policy.decide(attempt=attempt, request=request, exc=exc) |
| 109 | + await self._sleep(delay) |
| 110 | + raise AssertionError("unreachable") # pragma: no cover |
| 111 | +``` |
| 112 | + |
| 113 | +The sync `Retry.__call__` is identical but for `next(request)` and |
| 114 | +`self._sleep(delay)`. `_RETRYABLE_EXCEPTIONS` is one module constant |
| 115 | +referenced by both — the narrow catch surface stays structural, so anything |
| 116 | +not in the tuple (e.g. `httpx2.InvalidURL`, programming errors) propagates |
| 117 | +untouched exactly as today. The `# noqa: C901, PLR0912, PLR0915` suppressions |
| 118 | +come off `__call__`; `decide` may carry its own. |
| 119 | + |
| 120 | +### 4. Preserved public contract |
| 121 | + |
| 122 | +- `AsyncRetry.__init__` / `Retry.__init__` signatures unchanged (incl. |
| 123 | + `_sleep`, `budget`). |
| 124 | +- The wrapper keeps `self.budget` (the *same object* the policy holds, so |
| 125 | + `r1.budget is r2.budget` identity tests pass) and `self._sleep`. |
| 126 | +- The six config attributes (`max_attempts`, `base_delay`, `max_delay`, |
| 127 | + `retry_status_codes`, `retry_methods`, `respect_retry_after`) are **dropped** |
| 128 | + from the wrapper instances — they live solely on `_RetryPolicy`. They are |
| 129 | + read nowhere outside `retry.py` and `docs/resilience.md` documents them only |
| 130 | + as constructor parameters, not readable attributes. |
| 131 | + |
| 132 | +## Operations |
| 133 | + |
| 134 | +None — internal refactor, no infra or external changes. |
| 135 | + |
| 136 | +## Out of scope |
| 137 | + |
| 138 | +- `Bulkhead`/`AsyncBulkhead` deduplication. |
| 139 | +- Injecting randomness into `full_jitter_delay` (see Testing — only needed if |
| 140 | + we want exact-value assertions on the jitter path). |
| 141 | + |
| 142 | +## Testing |
| 143 | + |
| 144 | +- **Parity net:** all existing `MockTransport` suites — `test_retry.py`, |
| 145 | + `test_retry_sync.py`, `test_retry_props.py`, |
| 146 | + `test_retry_budget_threadsafety.py`, `test_threading_with_shared_budget.py` |
| 147 | + — stay green unchanged. Byte-identical behaviour is the bar. |
| 148 | +- **New seam tests:** `tests/test_retry_policy.py` drives `decide` directly |
| 149 | + (no client, no `MockTransport`) across the decision matrix: retryable → |
| 150 | + returns a delay; non-retryable status / non-eligible method → re-raises the |
| 151 | + original; streaming-body refusal; exhaustion note on the last attempt; |
| 152 | + Retry-After > `max_delay`; budget refusal → `RetryBudgetExhaustedError` with |
| 153 | + `__cause__`. |
| 154 | +- The jitter path returns a random delay, so assert **bounds** |
| 155 | + (`0 ≤ delay ≤ max_delay`) for it; assert exact values only on the |
| 156 | + deterministic Retry-After path. |
| 157 | +- `just lint` and `just test` both clean. |
| 158 | + |
| 159 | +## Risk |
| 160 | + |
| 161 | +- **Behavioural drift during extraction** (likely × high): a subtle |
| 162 | + reordering changes a note string, an event payload, or which exception wins. |
| 163 | + *Mitigation:* extract under the existing green suites; they assert notes, |
| 164 | + events (via the recording sleeper / caplog), and exception types. Do not |
| 165 | + edit the test suites in this change. |
| 166 | +- **Exception-chaining regression** (low × medium): moving the raise into |
| 167 | + `decide` could drop a `__cause__`/`__context__`. *Mitigation:* `decide` is |
| 168 | + called inside the live `except`; an explicit test asserts `__cause__` on the |
| 169 | + budget-exhausted path. |
| 170 | +- **Concurrency** (low × high): a stray per-call field on `_RetryPolicy` would |
| 171 | + make a shared instance unsafe. *Mitigation:* the policy holds only immutable |
| 172 | + config + the lock-guarded budget; per-attempt state stays as wrapper locals. |
| 173 | + The property/thread-safety suites cover interleaving. |
0 commit comments