-
Notifications
You must be signed in to change notification settings - Fork 11
feat: retry HTTP 429 responses with jittered backoff #1149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dgarros
wants to merge
17
commits into
develop
Choose a base branch
from
dga/feat-409-retry-ivj0i
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+2,502
−26
Open
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
0644a5d
spec(ihs-249): SDK retry with backoff on HTTP 429 responses
dgarros fd8a9a1
plan(ihs-249): design SDK 429 retry with backoff
dgarros 57fd634
critique(ihs-249): dual-lens review + apply must-address fixes
dgarros 63862f2
tasks(ihs-249): dependency-ordered task breakdown
dgarros 0f8e806
align(ihs-249): spec/PRD alignment check — ALIGNED
dgarros a55cbaa
plan(ihs-249): make async+sync dual coverage explicit
dgarros c71238c
feat(client): add HTTP 429 retry with backoff (handler, config, drivers)
dgarros ed0457c
test: add client-level 429 retry tests for _request path (US1)
dgarros 32abfd5
test(sdk): add US2 Retry-After client tests for 429 retry driver
dgarros d309335
test(rate-limit): client-level 429 retry exhaustion tests (US3)
dgarros 1727b96
test(sdk): add US4 disable/tune/parity 429 retry tests (T016)
dgarros d68d734
test: cover all request paths and multipart body re-read for 429 retry
dgarros 8917fb9
fix: floor negative and guard overflow in Retry-After delta parsing
dgarros b7edb68
docs(ihs-249): opsmill implement report — DONE
dgarros 8691b03
ci: fix markdownlint and vale errors in ihs-249 spec docs
dgarros c01b7dd
fix(client): harden 429 retry exhaustion, streaming cleanup, and backoff
dgarros 28b3d7f
chore: stop tracking .specify/feature.json
dgarros File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ artifact_definitions | |
| artifact_name | ||
| async | ||
| Authentik | ||
| backoff | ||
| boolean | ||
| check_definitions | ||
| class_name | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,5 +2,6 @@ | |
|
|
||
| <!-- SPECKIT START --> | ||
| For additional context about technologies to be used, project structure, | ||
| shell commands, and other important information, read the current plan | ||
| shell commands, and other important information, read the current plan: | ||
| `specs/ihs-249-sdk-429-retry/plan.md` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not be part of the PR |
||
| <!-- SPECKIT END --> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should probably only be one of these changelogs |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Added transparent retry of HTTP 429 (rate-limited) responses on both `InfrahubClient` and `InfrahubClientSync`. Retries use jittered exponential backoff and honour a server-provided `Retry-After` header (delta-seconds or HTTP-date). The behaviour is tunable through four new `Config` fields (`rate_limit_retry_enabled`, `rate_limit_max_retries`, `rate_limit_backoff_base`, `rate_limit_backoff_max`), and a new `RateLimitError` exception is raised when retries are exhausted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| A persistent HTTP 429 (rate-limited) response now raises `RateLimitError` once the configured retries are exhausted, instead of surfacing the raw `httpx.HTTPStatusError`. The underlying `httpx.HTTPStatusError` remains available via the exception's `__cause__`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # Spec/Ask Alignment Check: SDK retry with backoff on HTTP 429 responses | ||
|
|
||
| **Date**: 2026-07-07 | ||
| **Feature**: [spec.md](./spec.md) | ||
|
|
||
| ## 1. Source | ||
|
|
||
| **Source PRD**: Jira IHS-249 — "SDK retry with backoff on HTTP 429 responses" | ||
| (`https://opsmill.atlassian.net/browse/IHS-249`), fetched via the Atlassian MCP tool. | ||
| The issue body is itself a full, structured PRD (Problem Statement, Solution Overview, 9 User | ||
| Stories, 3 prioritised User Journeys with acceptance criteria, FR-001…009, Key Entities, Edge | ||
| Cases, SC-001…005, Implementation/Testing Decisions, Out of Scope, one Open Question). Related | ||
| GitHub issue: opsmill/infrahub-sdk-python#1124. No secondary URLs to fetch. | ||
|
|
||
| ## 2. Verdict | ||
|
|
||
| Result: ✅ ALIGNED | ||
|
|
||
| `spec.md` faithfully carries every PRD requirement, acceptance criterion, and scope boundary. | ||
| The only additions are an expansion of an existing requirement and the authorized resolution of | ||
| the PRD's explicit open question — neither is drift under the check's definition. | ||
|
|
||
| ## 3. Findings | ||
|
|
||
| | Severity | Category | PRD reference | Spec reference | Description | | ||
| | ---------- | ---------- | --------------- | ---------------- | ------------- | | ||
| | ✅ none | missing | FR-001…009 | FR-001…009 | All nine functional requirements present, none dropped or softened (attempt cap, jittered+clamped backoff, Retry-After both forms, malformed fallback, RateLimitError with url/attempts/last-Retry-After, all request paths, per-retry logging, async/sync parity, tune+disable). | | ||
| | ✅ none | missing | Journeys P1–P3, User Stories 1–9 | US1–US4, Edge Cases | P1/P2/P3 journeys map to US1/US2/US3; PRD user story 8 (tune/disable) surfaced as US4. All acceptance scenarios preserved. | | ||
| | ✅ none | missing | SC-001…005 | SC-001…005 | Success criteria carried over with equivalent semantics. | | ||
| | ✅ none | contradicted | Out of Scope (503, server-side INFP-636/635, `retry_on_failure`) | Out of Scope | Scope boundaries reproduced verbatim; nothing contradicted. | | ||
| | ℹ️ info | added (authorized) | Open Question (chain httpx.HTTPStatusError as `__cause__`?) | FR-005, Assumptions | The PRD's single open question was resolved affirmatively (chain the transport error as `__cause__`). The parent prep flow explicitly authorizes autonomous clarification resolution; recorded as an assumption. Not drift. | | ||
| | ℹ️ info | added (derived) | FR-009 (disable via Config) | SC-006 | Spec adds SC-006 (disabled path raises immediately). This is a measurable expansion of FR-009, not new scope. | | ||
| | ℹ️ info | added (design) | Assumption: single `_request` chokepoint | plan.md R1 / data-model | Plan (not spec) records that multipart/streaming bypass `_request`, so retry is applied at three sites. This corrects a PRD *assumption* at the implementation layer while still satisfying FR-006; spec requirements unchanged. Not spec drift. | | ||
|
|
||
| No requirements are missing, no acceptance criteria dropped or softened, no requirement semantics | ||
| changed, and no off-scope scope items were introduced. The Config field defaults (enabled, 5, 0.5, | ||
| 60), the new `RateLimitError`, and the additive-only API surface all match the PRD exactly. | ||
|
|
||
| ## 4. Action | ||
|
|
||
| **Proceed.** No remediation passes required (remediation counter: 0). `tasks.md` is safe to hand to | ||
| the implementation phase. The affirmative resolution of the open question and the SC-006 derivation | ||
| are documented above for traceability. |
39 changes: 39 additions & 0 deletions
39
dev/specs/ihs-249-sdk-429-retry/checklists/requirements.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| # Specification Quality Checklist: SDK retry with backoff on HTTP 429 responses | ||
|
|
||
| **Purpose**: Validate specification completeness and quality before proceeding to planning | ||
| **Created**: 2026-07-07 | ||
| **Feature**: [spec.md](../spec.md) | ||
|
|
||
| ## Content Quality | ||
|
|
||
| - [x] No implementation details (languages, frameworks, APIs) | ||
| - [x] Focused on user value and business needs | ||
| - [x] Written for non-technical stakeholders | ||
| - [x] All mandatory sections completed | ||
|
|
||
| ## Requirement Completeness | ||
|
|
||
| - [x] No [NEEDS CLARIFICATION] markers remain | ||
| - [x] Requirements are testable and unambiguous | ||
| - [x] Success criteria are measurable | ||
| - [x] Success criteria are technology-agnostic (no implementation details) | ||
| - [x] All acceptance scenarios are defined | ||
| - [x] Edge cases are identified | ||
| - [x] Scope is clearly bounded | ||
| - [x] Dependencies and assumptions identified | ||
|
|
||
| ## Feature Readiness | ||
|
|
||
| - [x] All functional requirements have clear acceptance criteria | ||
| - [x] User scenarios cover primary flows | ||
| - [x] Feature meets measurable outcomes defined in Success Criteria | ||
| - [x] No implementation details leak into specification | ||
|
|
||
| ## Notes | ||
|
|
||
| - The PRD's single open question (whether the exhaustion error should chain the | ||
| underlying transport error as its cause) was resolved affirmatively and encoded | ||
| into FR-005 and the Assumptions section, so no [NEEDS CLARIFICATION] markers remain. | ||
| - Entity names in the spec are described in capability terms (e.g. "rate-limit retry | ||
| decision logic") rather than concrete class names to keep the spec implementation-agnostic; | ||
| concrete names (`RateLimitRetryHandler`, `RateLimitError`, `Config` fields) are deferred to plan.md. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # Contract: `Config` rate-limit fields (additive, public) | ||
|
|
||
| Added to `infrahub_sdk/config.py::ConfigBase`. All additive; no existing field changes. | ||
|
|
||
| ```python | ||
| rate_limit_retry_enabled: bool = Field( | ||
| default=True, | ||
| description="Retry requests that receive HTTP 429 using backoff. Set False to disable.", | ||
| ) | ||
| rate_limit_max_retries: int = Field( | ||
| default=5, | ||
| ge=0, | ||
| description="Maximum number of retries after the initial attempt when receiving HTTP 429.", | ||
| ) | ||
| rate_limit_backoff_base: float = Field( | ||
| default=0.5, | ||
| gt=0, | ||
| description="Base interval in seconds for exponential backoff between 429 retries.", | ||
| ) | ||
| rate_limit_backoff_max: float = Field( | ||
| default=60.0, | ||
| gt=0, | ||
| description="Maximum wait in seconds for any single 429 retry (also clamps Retry-After).", | ||
| ) | ||
| ``` | ||
|
|
||
| ## Backward compatibility | ||
|
|
||
| - Purely additive; existing code constructing `Config(...)` / `InfrahubClient(...)` is unaffected. | ||
| - Environment-variable overrides follow the existing `BaseSettings` mechanism (e.g. | ||
| `INFRAHUB_RATE_LIMIT_MAX_RETRIES`), consistent with current fields. | ||
|
|
||
| ## Guarantees | ||
|
|
||
| - `rate_limit_retry_enabled=False` ⇒ a 429 is returned/raised exactly as before this feature | ||
| (no wait, no extra attempt). (FR-009, SC-006) | ||
| - Defaults produce transparent retry for typical background workloads. (FR-001) |
41 changes: 41 additions & 0 deletions
41
dev/specs/ihs-249-sdk-429-retry/contracts/rate_limit_error.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # Contract: `RateLimitError` (new public exception) | ||
|
|
||
| Added to `infrahub_sdk/exceptions.py`. Subclass of the base `Error`. | ||
|
|
||
| ```python | ||
| class RateLimitError(Error): | ||
| def __init__( | ||
| self, | ||
| url: str, | ||
| attempts: int, | ||
| retry_after: float | None = None, | ||
| message: str | None = None, | ||
| ) -> None: | ||
| self.url = url | ||
| self.attempts = attempts | ||
| self.retry_after = retry_after | ||
| if message is None: | ||
| message = ( | ||
| f"Request to {url} was rate-limited (HTTP 429) after {attempts} attempt(s)." | ||
| ) | ||
| super().__init__(message) | ||
| ``` | ||
|
|
||
| ## Contract | ||
|
|
||
| - **Raised**: by the client retry driver when 429s persist past `rate_limit_max_retries`. (FR-005) | ||
| - **Type**: `isinstance(err, Error)` is `True` — callers catching the SDK base `Error` still catch it. | ||
| - **Distinct**: it is NOT an `httpx.HTTPStatusError`; callers can `except RateLimitError` to | ||
| distinguish rate-limit exhaustion from other HTTP failures. (User story 6) | ||
| - **Attributes**: `url: str`, `attempts: int` (= `max_retries + 1`), `retry_after: float | None` | ||
| (last observed `Retry-After` in seconds, `None` if never present or unparseable). | ||
| - **Cause chaining**: raised with `raise RateLimitError(...) from http_status_error`, so | ||
| `err.__cause__` is the underlying `httpx.HTTPStatusError` built from the final 429 response. | ||
| Callers can inspect `err.__cause__.response` for the raw response. (Open-question resolution) | ||
|
|
||
| ## Behavioural change (changelog callout) | ||
|
|
||
| Before this feature, a persistent 429 surfaced as `httpx.HTTPStatusError` (via | ||
| `raise_for_status()`). With retry enabled (default), it now surfaces as `RateLimitError` | ||
| after exhaustion. Callers relying on catching `httpx.HTTPStatusError` for 429 should either | ||
| catch `RateLimitError` or inspect `__cause__`. |
46 changes: 46 additions & 0 deletions
46
dev/specs/ihs-249-sdk-429-retry/contracts/rate_limit_retry_handler.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # Contract: `RateLimitRetryHandler` (new, pure logic) | ||
|
|
||
| New module `infrahub_sdk/rate_limit.py`. No I/O, no sleeping — deterministic and unit-testable. | ||
|
|
||
| ```python | ||
| class RateLimitRetryHandler: | ||
| def __init__(self, max_retries: int, backoff_base: float, backoff_max: float) -> None: ... | ||
|
|
||
| def parse_retry_after( | ||
| self, header: str | None, *, now: datetime | None = None | ||
| ) -> float | None: | ||
| """Return seconds to wait per Retry-After, or None if absent/malformed. | ||
| - delta-seconds: int(header) | ||
| - HTTP-date: (parsedate_to_datetime(header) - now).total_seconds(), floored at 0 | ||
| - anything unparseable: None (caller falls back to computed backoff).""" | ||
|
|
||
| def compute_backoff(self, attempt: int) -> float: | ||
| """Deterministic exponential ceiling: min(backoff_max, backoff_base * 2**attempt).""" | ||
|
|
||
| def jittered_delay(self, ceiling: float) -> float: | ||
| """Full jitter: random.uniform(0, ceiling).""" | ||
|
|
||
| def next_delay( | ||
| self, attempt: int, retry_after_header: str | None = None, *, now: datetime | None = None | ||
| ) -> float: | ||
| """Honour Retry-After if parseable (clamped to backoff_max), else jittered backoff.""" | ||
|
|
||
| def should_retry(self, attempts_made: int) -> bool: | ||
| """True while retries remain: attempts_made <= max_retries.""" | ||
| ``` | ||
|
|
||
| ## Contract guarantees (map to FR / SC) | ||
|
|
||
| - `compute_backoff` is monotonic non-decreasing in `attempt` and never exceeds `backoff_max`. (FR-002, SC-003) | ||
| - `jittered_delay(c)` ∈ `[0, c]`; two calls (or two handler instances) are extremely unlikely to | ||
| match, satisfying "differ between instances". Tests assert jitter by sampling. (SC-003) | ||
| - `next_delay` clamps every result — computed *and* `Retry-After` — to `backoff_max`. (FR-003) | ||
| - `parse_retry_after` never raises on bad input; returns `None`. (FR-004) | ||
| - Past HTTP-date ⇒ `parse_retry_after` returns `0.0`, never negative. (Edge case) | ||
| - `should_retry` yields exactly `max_retries` retries ⇒ `max_retries + 1` total sends. (FR-001, SC-004) | ||
|
|
||
| ## Determinism for tests | ||
|
|
||
| - `parse_retry_after`/`next_delay` accept an injectable `now` for HTTP-date tests. | ||
| - Jitter is the only nondeterministic element; tests either assert on `compute_backoff` | ||
| (deterministic ceiling) or assert `0 <= jittered_delay(c) <= c` and that a sample of draws varies. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover