|
| 1 | +--- |
| 2 | +summary: Share Bulkhead/AsyncBulkhead's hand-duplicated constructor validation and rejection-event-plus-BulkheadFullError construction via two module-level functions. |
| 3 | +--- |
| 4 | + |
| 5 | +# Design: Share Bulkhead's validation and rejection logic |
| 6 | + |
| 7 | +## Summary |
| 8 | + |
| 9 | +`AsyncBulkhead` and `Bulkhead` (`src/httpware/middleware/resilience/bulkhead.py`, |
| 10 | +185 lines) hand-duplicate two blocks: constructor validation |
| 11 | +(`max_concurrent`/`acquire_timeout` range checks) and the on-timeout |
| 12 | +rejection path (emit a `bulkhead.rejected` event, construct |
| 13 | +`BulkheadFullError`). Only the semaphore type and acquire-timeout |
| 14 | +mechanics genuinely differ. This change extracts both blocks into two |
| 15 | +module-level functions in the same file — `_validate_bulkhead_config` and |
| 16 | +`_emit_bulkhead_rejected` — closing a deferral this repo's own planning |
| 17 | +history named for itself. No behavior change. |
| 18 | + |
| 19 | +## Motivation |
| 20 | + |
| 21 | +- `planning/changes/2026-06-23.01-retry-policy-extraction.md`'s |
| 22 | + Out-of-scope section states: "Not extending the same treatment to |
| 23 | + Bulkhead in this change." That change gave `circuit_breaker.py` and |
| 24 | + `retry.py` shared decision objects (`_CircuitBreakerState`, |
| 25 | + `_RetryPolicy`); `bulkhead.py` never got the equivalent treatment. |
| 26 | +- **Verified duplication:** `AsyncBulkhead.__init__` (`:62-76`) and |
| 27 | + `Bulkhead.__init__` (`:143-155`) run the identical two `if`/`raise |
| 28 | + ValueError` checks. `AsyncBulkhead.__call__`'s `except TimeoutError` |
| 29 | + branch (`:107-123`) and `Bulkhead.__call__`'s `if not acquired:` branch |
| 30 | + (`:163-179`) build the identical `_emit_event(...)` call (same event |
| 31 | + name, level, message, attributes shape) and the identical |
| 32 | + `BulkheadFullError(max_concurrent=..., acquire_timeout=...)` |
| 33 | + construction. |
| 34 | +- **Deletion test:** delete the two shared functions and both blocks |
| 35 | + reappear verbatim at all four call sites (2 `__init__`s, 2 `__call__`s) |
| 36 | + — real, load-bearing logic, not a pass-through. |
| 37 | + |
| 38 | +## Non-goals |
| 39 | + |
| 40 | +- No behavior change. Validation error messages, the emitted event's |
| 41 | + name/level/message/attributes, and `BulkheadFullError`'s fields stay |
| 42 | + byte-identical. |
| 43 | +- Not a stateful class (unlike `_CircuitBreakerState`/`_RetryPolicy`). |
| 44 | + Both extracted operations are one-shot/per-call pure functions with no |
| 45 | + config or evolving state reused across calls — a class would be a |
| 46 | + namespace with no benefit, the same reasoning already applied to |
| 47 | + `client.py`'s request-assembly extraction earlier this session. |
| 48 | +- Not moving the functions to `_internal/`. `bulkhead.py`'s logic is used |
| 49 | + only by its own two classes — same situation as `_CircuitBreakerState` |
| 50 | + in `circuit_breaker.py` and `_RetryPolicy` in `retry.py`, both of which |
| 51 | + stayed local to their file. |
| 52 | +- Not touching `AsyncBulkhead._check_loop`, the semaphore acquire |
| 53 | + mechanics, or anything else in either `__call__`/`__init__` beyond the |
| 54 | + two blocks named above. |
| 55 | + |
| 56 | +## Design |
| 57 | + |
| 58 | +### 1. `_validate_bulkhead_config` |
| 59 | + |
| 60 | +```python |
| 61 | +def _validate_bulkhead_config(*, max_concurrent: int, acquire_timeout: float | None) -> None: |
| 62 | + if max_concurrent < 1: |
| 63 | + raise ValueError(_MAX_CONCURRENT_INVALID) |
| 64 | + if acquire_timeout is not None and acquire_timeout < 0: |
| 65 | + raise ValueError(_ACQUIRE_TIMEOUT_INVALID) |
| 66 | +``` |
| 67 | + |
| 68 | +Replaces the two `if`/`raise` lines in both `__init__`s with: |
| 69 | + |
| 70 | +```python |
| 71 | +_validate_bulkhead_config(max_concurrent=max_concurrent, acquire_timeout=acquire_timeout) |
| 72 | +``` |
| 73 | + |
| 74 | +Uses `-> None` (not `typing.NoReturn`), matching the existing house idiom |
| 75 | +for validation helpers that raise conditionally |
| 76 | +(`_validate_max_response_body_bytes`, `_validate_httpx2_client_conflict`) |
| 77 | +— no precedent for `NoReturn` exists in this codebase, and this function |
| 78 | +doesn't always raise (only on the invalid branches), so `None` is also the |
| 79 | +technically correct annotation here regardless of style. |
| 80 | + |
| 81 | +### 2. `_emit_bulkhead_rejected` — builds and returns, does not raise |
| 82 | + |
| 83 | +```python |
| 84 | +def _emit_bulkhead_rejected( |
| 85 | + request: httpx2.Request, |
| 86 | + *, |
| 87 | + max_concurrent: int, |
| 88 | + acquire_timeout: float | None, |
| 89 | +) -> BulkheadFullError: |
| 90 | + _emit_event( |
| 91 | + _LOGGER, |
| 92 | + "bulkhead.rejected", |
| 93 | + level=logging.WARNING, |
| 94 | + message="bulkhead rejected request — acquire_timeout exceeded", |
| 95 | + attributes={ |
| 96 | + "max_concurrent": max_concurrent, |
| 97 | + "acquire_timeout": acquire_timeout, |
| 98 | + "method": request.method, |
| 99 | + "url": str(request.url), |
| 100 | + }, |
| 101 | + ) |
| 102 | + return BulkheadFullError(max_concurrent=max_concurrent, acquire_timeout=acquire_timeout) |
| 103 | +``` |
| 104 | + |
| 105 | +**Deliberately returns the exception rather than raising it.** The two |
| 106 | +call sites need different exception-chaining behavior: `AsyncBulkhead`'s |
| 107 | +rejection fires inside `except TimeoutError as exc:` and today does |
| 108 | +`raise BulkheadFullError(...) from exc` (explicit `__cause__`); |
| 109 | +`Bulkhead`'s rejection fires inside `if not acquired:` — no active |
| 110 | +exception, bare `raise BulkheadFullError(...)`. If the function raised |
| 111 | +internally, the async call site's explicit `from exc` would be lost, |
| 112 | +silently downgrading to implicit `__context__` chaining (a different |
| 113 | +traceback presentation — "the above exception was the direct cause of" |
| 114 | +vs "during handling of the above exception"). Returning the constructed |
| 115 | +exception lets each call site keep its own `raise` statement exactly as |
| 116 | +today: |
| 117 | + |
| 118 | +```python |
| 119 | +# AsyncBulkhead.__call__, inside except TimeoutError as exc: |
| 120 | +raise _emit_bulkhead_rejected( |
| 121 | + request, max_concurrent=self._max_concurrent, acquire_timeout=self._acquire_timeout, |
| 122 | +) from exc |
| 123 | + |
| 124 | +# Bulkhead.__call__, inside if not acquired: |
| 125 | +raise _emit_bulkhead_rejected( |
| 126 | + request, max_concurrent=self._max_concurrent, acquire_timeout=self._acquire_timeout, |
| 127 | +) |
| 128 | +``` |
| 129 | + |
| 130 | +### 3. Placement |
| 131 | + |
| 132 | +Both functions go in `bulkhead.py`, module level, alongside the existing |
| 133 | +module constants (`_MAX_CONCURRENT_INVALID`, `_ACQUIRE_TIMEOUT_INVALID`, |
| 134 | +`_LOGGER`) — before `class AsyncBulkhead:`. |
| 135 | + |
| 136 | +## Testing |
| 137 | + |
| 138 | +- **Parity net:** existing `tests/test_bulkhead.py` and |
| 139 | + `tests/test_bulkhead_sync.py` stay green with the assertions they |
| 140 | + already have (message content, `BulkheadFullError` field values, |
| 141 | + `bulkhead.rejected` event via `caplog`) — byte-identical behavior is the |
| 142 | + bar. |
| 143 | +- **New chaining tests** (the one subtlety this refactor is careful to |
| 144 | + preserve, and no existing test asserts it): in `test_bulkhead.py`, add |
| 145 | + an assertion that the raised `BulkheadFullError`'s `__cause__` is the |
| 146 | + `TimeoutError` that triggered it; in `test_bulkhead_sync.py`, add an |
| 147 | + assertion that `__cause__` is `None`. Mirrors how `DecodeError.__cause__` |
| 148 | + is explicitly tested in `test_errors.py`. |
| 149 | +- **No new direct-unit-test file** for the two shared functions — the |
| 150 | + existing construction-error tests already exercise |
| 151 | + `_validate_bulkhead_config` fully (both branches, both classes), and |
| 152 | + the existing `BulkheadFullError`-field/event tests plus the new |
| 153 | + chaining tests already exercise `_emit_bulkhead_rejected` fully. |
| 154 | +- `just lint && just test` both clean; 100% coverage maintained. |
| 155 | + |
| 156 | +## Risk |
| 157 | + |
| 158 | +- **Chaining regression** (the main risk this design addresses directly): |
| 159 | + *Mitigation:* returning rather than raising keeps each `raise` statement |
| 160 | + exactly where it is today; the new chaining tests make a future |
| 161 | + regression (e.g. someone "simplifying" the sync call site to also use |
| 162 | + `from exc` where no exception exists, or vice versa) fail loudly. |
| 163 | +- **Behavioral drift in the shared validation/event logic** (unlikely × |
| 164 | + low): *Mitigation:* extract verbatim under the existing green suites, |
| 165 | + which assert exact message text and field values; do not edit those |
| 166 | + assertions in this change (only add the two new chaining assertions). |
0 commit comments