Skip to content

Commit f2e000a

Browse files
lesnik512claude
andcommitted
audit: consolidate dryrun findings into chunk 1 + prep remaining dims
- Lift the 5 confirmed correctness findings from the dryrun smoke test into the real audit file as Chunk 1 (correctness). - Drop planning/audit/_dryrun.md (committed temporarily in 51ee08c). - Commit planning/audit/_discover.json (module map) for reuse by remaining chunks 1-4 runs. - Workflow.mjs refactor: normalize args parameter via JSON.parse fallback (the workflow harness was receiving args as a string in some paths). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 63ea69b commit f2e000a

4 files changed

Lines changed: 788 additions & 142 deletions

File tree

planning/audit/2026-06-07-deep-audit.md

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,123 @@ _Counts updated after final merge._
1515
- Nits: —
1616

1717
<!-- chunk sections appended below in order: 1, 2, 3, 4 -->
18+
19+
## Chunk 1 — Correctness (from dry-run smoke test)
20+
21+
5 confirmed correctness findings reviewed, 5 survived verifier consensus, dominant area is the resilience retry middleware (4/5 land in `middleware/resilience/`, the fifth in `client.py`). The cluster points at a single subsystem — Finagle-style retry budgeting and the retry decision tree — where small arithmetic and ordering bugs compound into materially different behavior than documentation implies. No blockers; the bar between low and medium turns on whether default settings expose the defect to typical users, and in each case a non-default knob (floor=0, max_delay much smaller than server Retry-After, custom retry_methods) is required to observe the bug. Triaged into 3 low + 2 nit.
22+
23+
### Low
24+
25+
#### RetryBudget deposit fires per attempt instead of per original request
26+
27+
`src/httpware/middleware/resilience/retry.py:105`
28+
29+
`deposit()` lives inside the per-attempt `for` loop, so a request that retries twice contributes three deposits and two withdrawals. The Finagle budget contract is `withdrawals / deposits <= percent_can_retry` where the denominator counts original requests; inflating it lets through ~2x the configured retry rate when every request retries. The identical placement is present in the sync `Retry` class at line 236.
30+
31+
```python
32+
for attempt in range(self.max_attempts):
33+
is_last = attempt + 1 >= self.max_attempts
34+
self.budget.deposit()
35+
try:
36+
return await next(request)
37+
except StatusError as exc:
38+
retryable_status = exc.response.status_code in self.retry_status_codes
39+
if not method_eligible or not retryable_status:
40+
if retryable_status and request.extensions.get(STREAMING_BODY_MARKER):
41+
exc.add_note(_STREAMING_BODY_REFUSAL_NOTE)
42+
raise
43+
last_exc = exc
44+
last_response = exc.response
45+
```
46+
47+
Verifier consensus: 2/3 (code_reality + reproducer). Suggested direction: hoist `self.budget.deposit()` above the loop so it runs exactly once per call to the middleware; mirror the change in `Retry`.
48+
49+
#### Retry-After silently capped to max_delay
50+
51+
`src/httpware/middleware/resilience/retry.py:189`
52+
53+
When `respect_retry_after=True` and a server responds with `Retry-After: 120` while the client is configured with `max_delay=5.0`, the computed delay collapses to `min(120, 5.0) = 5.0` and the retry fires after 5 s — almost certainly receiving the same 429/503 again and burning an attempt. The option name implies the header is honored; in practice it is silently overridden by `max_delay`. Affects async and sync paths identically.
54+
55+
```python
56+
if retry_after is not None:
57+
delay = min(retry_after, self.max_delay)
58+
else:
59+
delay = full_jitter_delay(
60+
attempt,
61+
base_delay=self.base_delay,
62+
max_delay=self.max_delay,
63+
)
64+
```
65+
66+
Verifier consensus: 2/3 (code_reality confirmed twice). Suggested direction: when `retry_after > max_delay`, choose explicitly — either give up (and raise the underlying `StatusError` with a note explaining the Retry-After exceeded the cap) or document that `respect_retry_after` is bounded by `max_delay`. Whichever path is chosen, surface the decision in docs and the docstring.
67+
68+
#### RetryBudget ceiling truncates rather than rounds
69+
70+
`src/httpware/middleware/resilience/budget.py:67`
71+
72+
`ceiling = int(len(self._deposits) * self._percent_can_retry) + floor` truncates instead of rounding, so 4 deposits at `percent_can_retry=0.2` yield `int(0.8) = 0`. With the default `floor` (from `min_retries_per_sec=100`) this is invisible, but with `floor=0` and low traffic the budget refuses every retry until the deposit count crosses the next integer threshold. This is an off-by-one against the configured percentage for any deposit count not a clean multiple of `1 / percent_can_retry`.
73+
74+
```python
75+
def try_withdraw(self) -> bool:
76+
now = self._now()
77+
with self._lock:
78+
self._purge(now)
79+
floor = int(self._min_retries_per_sec * self._ttl)
80+
ceiling = int(len(self._deposits) * self._percent_can_retry) + floor
81+
if len(self._withdrawn) >= ceiling:
82+
return False
83+
self._withdrawn.append(now)
84+
return True
85+
```
86+
87+
Verifier consensus: 2/3 (code_reality + reproducer). Suggested direction: replace `int(...)` with `math.ceil(...)` so the configured percentage is reached at the first deposit-count where it is mathematically expressible, and add a Hypothesis property that asserts `withdrawals / deposits` stays close to `percent_can_retry` within `±1/deposits`.
88+
89+
### Nit
90+
91+
#### Streaming-body refusal note attached when method ineligibility is the gating reason
92+
93+
`src/httpware/middleware/resilience/retry.py:111`
94+
95+
Inside the `if not method_eligible or not retryable_status:` early-out, the streaming-body note is added whenever `retryable_status and request.extensions.get(STREAMING_BODY_MARKER)` — even when the actual reason for not retrying is that the method is excluded from `retry_methods`. A `POST` with a streaming body that gets a 503 receives a note saying the stream cannot replay, when the real fix is to add `POST` to `retry_methods`. Same pattern in `Retry` at lines 242 and the `NetworkError`/`TimeoutError` arm at 249.
96+
97+
```python
98+
except StatusError as exc:
99+
retryable_status = exc.response.status_code in self.retry_status_codes
100+
if not method_eligible or not retryable_status:
101+
if retryable_status and request.extensions.get(STREAMING_BODY_MARKER):
102+
exc.add_note(_STREAMING_BODY_REFUSAL_NOTE)
103+
raise
104+
last_exc = exc
105+
last_response = exc.response
106+
except (NetworkError, TimeoutError) as exc:
107+
if not method_eligible:
108+
if request.extensions.get(STREAMING_BODY_MARKER):
109+
exc.add_note(_STREAMING_BODY_REFUSAL_NOTE)
110+
raise
111+
```
112+
113+
Verifier consensus: 2/3 (code_reality + reproducer). Suggested direction: only attach the streaming-body note when the method *is* eligible and the status *is* retryable — i.e., move the note into the branch where streaming is the actual blocker. Otherwise emit a different note that names the real reason, or omit the note.
114+
115+
#### RuntimeError mapping to TransportError uses substring match on "closed"
116+
117+
`src/httpware/client.py:135`
118+
119+
`if "closed" in str(exc)` is a substring check against an external library's error string. Any `RuntimeError` whose message happens to contain "closed" — from any transport layer, plugin, or future httpx2 change — gets converted to `TransportError`; conversely, if httpx2 rewords the message ("shut down", "disposed", etc.) the typed mapping silently breaks and a raw `RuntimeError` escapes to the caller. Same pattern in sync `Client._terminal`.
120+
121+
```python
122+
async def _terminal(self, request: httpx2.Request) -> httpx2.Response:
123+
try:
124+
async with _httpx2_exception_mapper():
125+
response = await self._httpx2_client.send(request)
126+
except RuntimeError as exc:
127+
if "closed" in str(exc):
128+
raise TransportError(str(exc)) from exc
129+
raise
130+
_raise_on_status_error(response)
131+
return response
132+
```
133+
134+
Verifier consensus: 2/3 (code_reality + reproducer). Suggested direction: gate the mapping on a more stable signal — check `self._httpx2_client.is_closed` (or whatever the public API exposes) before the call, or whitelist the message via a single module-level constant that lives next to a test asserting the current httpx2 wording.
135+
136+
<!-- chunk 1: concurrency + error_contract appended below by targeted run -->
137+

0 commit comments

Comments
 (0)