feat(client): retry transient GET failures with backoff - #37
Conversation
| response: requests.Response | None = None | ||
| for attempt in range(self._MAX_RATE_LIMIT_RETRIES + 1): | ||
| response = self._request_with_same_origin_redirects( | ||
| response = self._request_with_transient_retries( |
There was a problem hiding this comment.
(AI-assisted)
Could we enforce the advertised seven-attempt budget across the composed retry layers?
429 is correctly excluded from _RETRYABLE_GET_STATUSES, but each outer rate-limit retry calls _request_with_transient_retries() again with a fresh budget. A sequence of six 503s followed by 429, repeated across the four rate-limit rounds, therefore makes 28 physical requests for one logical GET; the 401 refresh path can repeat the composition again.
Could we use one shared attempt/deadline budget across transient, rate-limit, and auth-refresh retries, and add a mixed 503/429 regression test that asserts the total request count? That would avoid retry amplification during an outage while preserving Retry-After handling.
a575da1 to
3f3421b
Compare
| if response.status_code != 429 or not budget.can_retry(): | ||
| return response | ||
| self._sleep_for_rate_limit(response, attempt) | ||
| return response | ||
| self._sleep_for_rate_limit(response, rate_limit_round) | ||
| rate_limit_round += 1 |
There was a problem hiding this comment.
(AI-assisted)
Could we re-check the retry deadline after sleeping, before issuing the next request? can_retry() is evaluated before _sleep_for_rate_limit(), so a long Retry-After can advance past the shared deadline and the loop still performs another unconditional send. In a deterministic reproduction, a 10-second deadline plus Retry-After: 60 sent once at t=0 and again at t=60; the transient-backoff path has the same pattern. Please cap the sleep to the remaining budget and/or atomically check/consume the budget immediately before every physical send.
| budget.consume() | ||
| attempt += 1 | ||
| try: | ||
| response = self._request_with_same_origin_redirects( |
There was a problem hiding this comment.
(AI-assisted)
Could we charge the shared retry budget for every actual session.request(), including redirects? This budget.consume() runs once before _request_with_same_origin_redirects(), but that helper may perform several physical sends. A deterministic 307 → 503 sequence repeated for seven retry rounds produced 14 sends while consuming only seven units; five redirects per round can reach 42 sends. Please move the budget check/consume to the physical-send boundary so each redirect also counts.
| if method.upper() != "GET" or request_kwargs.get("stream"): | ||
| budget.consume() |
There was a problem hiding this comment.
(AI-assisted)
Nit: could we preserve the legacy four-attempt 429 cap for POSTs and streamed GETs? These requests correctly bypass transient retries, but they still consume the shared seven-unit budget in the outer 429 loop. Ten queued 429s therefore produce seven sends instead of the previous four, and the default no-header backoff grows from 7 seconds total to 63. Since the PR says existing 429 behavior is preserved, either retain the smaller cap for these request classes or explicitly document and test the change.
df4bfc3 to
0b19b45
Compare
Idempotent GETs now retry on 500/502/503/504 and transient transport errors (connection reset/refused, timeout, truncated body) with doubling backoff capped at 30s. Mutating methods are never duplicated and streamed GETs are excluded so stream consumers keep control of the open path. SSL certificate errors raise on the first attempt because they are deterministic and retrying only delays the report. The transient, 429 rate-limit, and 401 auth-refresh layers share one per-logical-request budget so a composed outage cannot multiply their per-layer limits together. That budget counts logical attempts and physical sends separately against a single 120s deadline: 7 attempts, and a pool of 14 session.request calls. The pool is the larger of two floors -- two full _MAX_REDIRECTS-deep chains (12) and two sends per logical attempt (14) -- so at current constants the attempt term is what sets it. Sends need their own pool because a redirect hop is not a retry; spending hops from the attempt count would break ordinary healthy traffic, since a chain three or more hops deep followed by one 401 refresh needs at least 8 sends with no outage involved. Pooling them still bounds worst-case amplification at 14 requests rather than attempts times chain length. Every session.request charges the send pool at the send boundary, including each same-origin redirect hop, and the check and the charge happen together immediately before the send so a long Retry-After or backoff cannot carry the clock past the deadline and still let a request out. A wait that would not fit in the remaining deadline ends the sequence instead of being slept. When the pool runs out part-way through a redirect chain, the last completed non-redirect response is reported so raise_for_status still surfaces the real backend status; only exhaustion during the very first chain, with nothing completed to report, raises RetryError, and that error names whether the deadline or the send pool was the cause. A replayable GET spends the shared budget on rate limiting too, so a sustained 429 without Retry-After now takes 7 sends and waits 1+2+4+8+16+32 = 63s; the 429 backoff is capped at 60s, which the 32s step never reaches. The 5xx path over the same 7 sends waits 1+2+4+8+16+30 = 61s under its lower 30s cap. Both stay inside the 120s deadline. Requests that cannot be replayed -- mutating methods and streamed GETs -- keep the existing four-attempt 429 cap and its 1/2/4s backoff. Every superseded 429 is closed before its backoff so a pooled connection is not held across the wait, which matters most for streamed GETs whose bodies are never read; the response actually returned to the caller is left open. Retry warnings go through the client logger; pipeline-run, artifact, pipeline hydration, published-component, and secret commands thread their --log-type logger into the client so the warnings follow the configured sink, and logger-less programmatic clients stay silent.
0b19b45 to
87295c5
Compare
| if not budget.try_consume_send(): | ||
| raise requests.exceptions.RetryError( | ||
| f"Retry budget exhausted ({budget.exhaustion_reason()}) " | ||
| f"while sending {current_method} {current_url}" |
There was a problem hiding this comment.
(AI-assisted) RetryError includes the full current_url after budget exhaustion. Following a same-origin redirect, that can preserve signed query credentials and surface them through CLI/log output. Please omit the URL or sanitize it to scheme/host/path with query and fragment removed, and add a regression that exhausts retries after a redirect to a URL carrying a secret query value.
2 participants
What
Idempotent GETs now retry on 500/502/503/504 and on transient transport failures (connection reset/refused, timeout, truncated body) with doubling backoff.
SSLErrorstill raises on the first attempt — a certificate failure is deterministic, so retrying only delays the report.Mutating methods and streamed GETs are never replayed, and keep their existing four-send 429 flow with its 1/2/4s backoff.
Why
Short backend restarts and connection resets currently fail read-only CLI operations outright, even though repeating the request is safe. The risk in adding retries is amplification: the transient, 429, and 401 auth-refresh layers would otherwise multiply their per-layer limits together during a composed outage, and a redirect chain in front of every attempt would multiply them again.
How
One budget per logical request bounds all three layers together: 7 logical attempts, a pool of 14 physical sends, and a 120-second deadline.
Sends are pooled separately from attempts because a redirect hop is not a retry. A chain three or more hops deep followed by one 401 refresh needs eight sends with no outage involved, and the client supports chains up to five hops, so charging hops against the attempt count would break ordinary traffic.
Each send is charged at the send boundary, redirect hops included, and checked and charged together immediately before it goes out — so a long
Retry-Afteror backoff cannot carry the clock past the deadline and still let a request through. A wait that would not fit in the remaining deadline ends the sequence instead.If the pool runs out mid-chain, the last completed non-redirect response is reported, so
raise_for_statussurfaces the real backend status (a 307 in front of a 503 reports 503).RetryErroris raised only when nothing completed, and it names whether the deadline or the send pool was the cause.Worst case under sustained load with no
Retry-After:The two GET figures differ because the 429 backoff caps at 60s (never reached by the 32s step) and the 5xx backoff at 30s. Both stay inside the deadline.
Superseded 429s and intermediate 5xx are closed before their backoff so a pooled connection is not held across the wait, which matters most for streamed GETs whose bodies are never read. A response returned as the final answer is left open. The one exception is a superseded response handed back after the pool runs out mid-chain: its status stays readable, and a non-streamed body stays buffered.
Retry warnings go through the configured client logger — pipeline-run, artifact, pipeline hydration, published-component, and secret commands now pass their
--log-typelogger to the client. Logger-less programmatic clients stay silent.Testing
uv run pytest— 854 passed, 1 failed. That failure,test_api_cli.py::test_official_static_command_without_schema_fails_with_actionable_error, is pre-existing and reproduces identically onmaster.uv run pytest tests/test_client.py tests/test_static_client.py tests/test_pipeline_runs_cli.py tests/test_artifacts_cli.py tests/test_components_cli.py tests/test_secrets_cli.py— 198 passed.ruff checkon the touched files,uv lock --check,git diff --check— clean.Retry tests drive a fake
time.monotonicadvanced only by sends, so nothing depends on real elapsed time. They cover the exact sleep sequences above, waits crossing the deadline, mixed auth/429/5xx composition, both exhaustion-message branches, redirect chains of zero to five hops plus a 401 refresh across GET and POST, and 307 → 503 reporting 503 rather than exhaustion. Response release is asserted on both sides: intermediates closed before each retry for plain and streamed GETs, and the returned response still open and readable.