You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
ci: fix markdownlint and vale errors in ihs-249 spec docs
markdown-lint: pad table delimiter rows (MD060 compact style), add
blank lines around headings/tables/lists (MD022/MD032/MD058), and
replace emphasis-as-heading with plain text (MD036) in the generated
spec artifacts under dev/specs/ihs-249-sdk-429-retry/.
vale: add "backoff" to the spelling exception vocabulary so the
generated config.mdx retry docs pass Infrahub.spelling.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| P1 | ✅ | Problem is clear and evidenced (background workloads are the traffic most likely rate-limited; callers currently hand-roll retries). No gap. | None. |
30
31
31
32
### User Value Assessment
33
+
32
34
| ID | Severity | Finding | Suggestion |
33
-
|----|----------|---------|------------|
35
+
|----|----------|---------|------------|
34
36
| P2 | ✅ | Every user story maps to value; MVP is cleanly P1 (transparent retry-and-succeed). | None. |
35
37
36
38
### Alternative Approaches
39
+
37
40
| ID | Severity | Finding | Suggestion |
38
-
|----|----------|---------|------------|
41
+
|----|----------|---------|------------|
39
42
| P3 | 💡 | The plan doesn't record *why* a custom handler beats off-the-shelf options (`tenacity`, httpx transport-level `retries`). httpx transport retries are connection-level only (not status-code aware) and `tenacity` is a new dependency (out of scope). | Add one line to research.md for the record so reviewers don't re-litigate. (Applied.) |
40
43
41
44
### Edge Cases & UX
45
+
42
46
| ID | Severity | Finding | Suggestion |
43
-
|----|----------|---------|------------|
47
+
|----|----------|---------|------------|
44
48
| P4 | 💡 | Worst-case cumulative blocking time (~`max_retries × backoff_max` ≈ 300 s with defaults) is bounded but undocumented; an interactive caller could block ~5 min. | Document the worst-case total wait and note interactive callers can lower `rate_limit_max_retries`/`rate_limit_backoff_max` or disable. (Applied to plan.) |
45
49
46
50
### Success Measurement
51
+
47
52
| ID | Severity | Finding | Suggestion |
48
-
|----|----------|---------|------------|
53
+
|----|----------|---------|------------|
49
54
| P5 | ✅ | SC-001..006 are measurable and mapped to acceptance scenarios and quickstart validations. | None. |
50
55
51
56
---
52
57
53
58
## Engineering Lens Findings 🔬
54
59
55
60
### Architecture Soundness
61
+
56
62
| ID | Severity | Finding | Suggestion |
57
-
|----|----------|---------|------------|
63
+
|----|----------|---------|------------|
58
64
| E1 | ✅ | Pure handler + thin async/sync drivers is the right shape; single logic contract satisfies FR-008. Multi-site chokepoint already documented (research R1). | None. |
59
65
60
66
### Failure Mode Analysis
67
+
61
68
| ID | Severity | Finding | Suggestion |
62
-
|----|----------|---------|------------|
69
+
|----|----------|---------|------------|
63
70
| E2 | 🎯 |**Multipart retry can re-send a consumed body.**`_request_multipart` receives a `files` dict that may hold open file handles / streams. On the first send httpx reads them to EOF; a retry re-sends the *same* handles, uploading empty or truncated data — a silent data-corruption bug that only manifests under rate-limiting. | The multipart send site MUST rewind (`seek(0)`) or re-materialize the payload before each retry, or the driver must accept a payload *factory* that produces a fresh body per attempt. Add this constraint to plan/data-model and a dedicated task + regression test. (Applied.) |
64
71
| E3 | 🤔 | Retrying mutations relies on the PRD assumption that a 429 is always a pre-processing rejection (no partial write). If the server ever emits 429 after partial processing, a retried POST double-writes. | Accept the PRD's explicit assumption (rate-limit 429 = pre-processing) for this scope; recorded as an assumption in spec.md. Revisit only if server semantics change. (Resolved — no change.) |
65
72
66
73
### Security & Privacy
74
+
67
75
| ID | Severity | Finding | Suggestion |
68
-
|----|----------|---------|------------|
76
+
|----|----------|---------|------------|
69
77
| E4 | 💡 | Retry logging must not leak secrets. The login/refresh paths carry `Authorization: Bearer …` headers and username/password payloads; logging is spec'd to include only URL/attempt/delay, but this should be stated as an explicit constraint so it isn't broadened later. | State in research R8 that retry logs MUST include only URL, attempt number, and delay — never headers or payload. (Applied.) |
70
78
71
79
### Performance & Scalability
80
+
72
81
| ID | Severity | Finding | Suggestion |
73
-
|----|----------|---------|------------|
82
+
|----|----------|---------|------------|
74
83
| E5 | ✅ | Full jitter de-correlates concurrent clients (thundering-herd mitigation). No hot paths introduced. | None. |
75
84
76
85
### Testing Strategy
86
+
77
87
| ID | Severity | Finding | Suggestion |
78
-
|----|----------|---------|------------|
88
+
|----|----------|---------|------------|
79
89
| E6 | 💡 | Test matrix is comprehensive but has no guard for E2 (multipart body re-read). | Add a client-level test: a multipart upload that gets 429→200 must re-send the *full* body on the retry (assert bytes received on attempt 2 equal attempt 1). (Applied to quickstart + will be a task.) |
80
90
81
91
### Operational Readiness
92
+
82
93
| ID | Severity | Finding | Suggestion |
83
-
|----|----------|---------|------------|
94
+
|----|----------|---------|------------|
84
95
| E7 | ✅ | WARNING-level per-retry logging via the existing module logger is appropriate for a library; host apps control handlers. | None. |
85
96
86
97
### Dependencies & Integration
98
+
87
99
| ID | Severity | Finding | Suggestion |
88
-
|----|----------|---------|------------|
100
+
|----|----------|---------|------------|
89
101
| E8 | ✅ | No new dependencies (httpx + stdlib). `retry_on_failure` left untouched (out of scope). Additive Config + one exception; behavioural break documented for changelog. | None. |
| X1 | Multipart body re-read (E2) | A "successful" upload that silently uploaded nothing is worse than a visible failure — directly harms the P1 "transparent success" promise. | Silent data corruption under load; hard to diagnose. | Make re-readable payload a hard requirement before implementation. (Applied.) |
| 3 | Phase 4 US2 | T013 (1) | 1 | 0 | 0 |`32abfd5`| HTTP-date case asserted with an inclusive time window (driver parses against `datetime.now`); fixed-form cases assert exactly. |
@@ -33,7 +33,7 @@ None. All T001–T021 completed and ticked.
33
33
Runner: `uv run pytest`. Environment for every row: **Python 3.12.13, pytest 9.0.3, pytest-httpx 0.36.0, asyncio mode=AUTO, project `.venv` (`uv sync --all-groups --all-extras`); no external infrastructure required** (all tests run locally; no E2E deferred). Rows are grouped by test function; the bracketed count is the number of parametrized variants, all PASSED.
34
34
35
35
| Test id | Type | Run command | Passed at (ISO 8601) | Env context | Verbatim pass line |
|`test_rate_limit.py` handler suite (17 tests: compute_backoff growth/clamp, jittered_delay bounds/variance, parse_retry_after delta/http-date/past-zero/malformed×5, next_delay honour/fallback/clamp, should_retry budget×2) | unit (pure) |`uv run pytest tests/unit/test_rate_limit.py tests/unit/sdk/test_rate_limit_retry.py -v`| 2026-07-07T12:20:50Z | as above |`17 passed in 0.02s`|
38
38
|`test_request_retries_429_then_succeeds`[standard, sync]| unit (client) |`uv run pytest tests/unit/sdk/test_rate_limit_retry.py -v`| 2026-07-07T12:24:28Z | as above |`6 passed in 0.02s`|
39
39
|`test_request_passes_non_429_through_untouched`[standard-200, standard-500, sync-200, sync-500]| unit (client) |`uv run pytest tests/unit/sdk/test_rate_limit_retry.py -v`| 2026-07-07T12:24:28Z | as above |`6 passed in 0.02s`|
@@ -58,7 +58,7 @@ Runner: `uv run pytest`. Environment for every row: **Python 3.12.13, pytest 9.0
58
58
Reviewed the full diff `a55cbaa..HEAD` with three independent agents (code correctness + async/sync parity + type design; error handling / silent failures; test coverage quality).
0 commit comments