feat(client): retry transient GET failures with backoff - #37
Conversation
task-status prints the task-name to status map for a root execution by walking its child executions; container state falls back to graph-state reduction for nested pipelines. task-wait polls until every task is terminal. SUCCEEDED and SKIPPED are non-failure terminals; failures exit 2 with a stderr summary and the full status map on stdout. Wait bounds are validated up front and the deadline applies while resolving children. Transient 5xx on the per-poll root details fetch are retried within the wait deadline instead of aborting the wait; unbounded waits give up after a few consecutive failures. Also adjusts the shared status reducer used by the existing run-level status/wait: WAITING_FOR_UPSTREAM and UNINITIALIZED now count as active so a run is not reported terminal while a task still waits upstream, mixed terminal aggregates reduce to the failure terminal ahead of SKIPPED/SUCCEEDED, and wait bounds reject NaN/inf.
Add --stream to tangle sdk pipeline-runs logs to follow container logs live instead of fetching a one-shot snapshot. Opening the stream has its own bounded retry budget for transport errors and retryable 5xx responses, spent before the first line is yielded. An established stream is never reopened, so output cannot be duplicated; mid-stream drops, open failures, BrokenPipe and Ctrl-C all exit without tracebacks.
Support binding a pipeline input to a Tangle secret via
`--arg-secret INPUT=SECRET_NAME` (repeatable) and a matching
`arg_secrets` config mapping. Each reference encodes the OSS
dynamic-data payload {"dynamicData":{"secret":{"name":...}}} under
the input in root_task arguments.
Validation runs before any file read or network call: inputs and
secret names must be non-empty after trimming, duplicates are
rejected, and an input supplied both as a plain value and a secret
reference is rejected rather than silently overwritten. CLI
`--arg-secret` overrides config `arg_secrets`. Existing --arg,
--args-json, config args, hydration, dry-run, and submit-recovery
behavior are unchanged.
submit_and_wait_prepared_body submits a prepared run body and waits for terminal state in one call. Wait parameters are validated before creating a run, max_wait=None remains unbounded, and submit_recovery_attempts passes through to failed-submit recovery so a run registered before a transport failure can be adopted instead of duplicated. The caller body is never mutated. Also fixes a pre-existing KeyError in submit_prepared_body on locator-style bodies without an inline componentRef.spec, and tightens wait_for_completion to reject non-finite max_wait/poll_interval instead of treating inf as a never-firing deadline.
| 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.
6416855 to
429fe5a
Compare
Introduce a single TLS verification contract shared by the requests client, the httpx schema/operation transport, and the dynamic-discovery client. TANGLE_API_CA_BUNDLE verifies against a custom CA bundle and TANGLE_API_VERIFY_TLS toggles verification, with verification enabled by default. Precedence is explicit argument, CA bundle, verify flag, then the secure default; unset settings preserve requests' environment and caller-supplied session behavior.
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.
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.
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. RetryError renders its destination credential-safely: only the method and scheme://host[:port]/path survive, with query, fragment, and authority userinfo removed, because a same-origin redirect can land on a signed URL (access_token, X-Amz-Signature) and the exhaustion error flows into CLI output and logs. Authorities the parser rejects, such as an unclosed IPv6 bracket, degrade to a placeholder instead of letting error formatting raise, and malformed ports pass through untouched since the port is never parsed.
87295c5 to
d6eea86
Compare
…c-submit-and-wait feat(pipeline-run): add programmatic submit_and_wait
…ution-logs feat(pipeline-run): stream execution logs
…secret Add --arg-secret to pipeline-runs submit for secret-backed inputs
…-wait feat(pipeline-run): add task-status and task-wait commands
Assisted-By: devx/3facbcec-c222-4a3d-be16-c136d12fb965
Add configurable TLS verification for all HTTP transports
Preserve transient GET retry and credential-safe exhaustion behavior while retaining the current streaming, timeout, pipeline, TLS, and CI contracts.
Volv-G
left a comment
Volv-G
left a comment
There was a problem hiding this comment.
AI-assisted exact-head review on commit 2c679b8984fa0f18aea0ca1fcfab2133856139f6. The credential/retry semantic integration received a registered zero-finding local review and preserves credential redaction, retry budgets, streaming, and timeout behavior. Targeted tests (172) and full suites (1,089) passed on Python 3.12 and 3.13; lock/diff checks passed with no Ruff regression. GitHub CI run 32199476529 executed and passed test (3.12), test (3.13), and aggregate test. Approval submitted with explicit authorization from Volv Grebennikov.
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. Because exhaustion can happen after a same-origin redirect onto a signed URL, the error renders only the method and scheme/host/path — query, fragment, and userinfo are dropped so signed credentials (access_token,X-Amz-Signature, …) cannot leak into CLI output or logs, and an authority the parser rejects degrades to a placeholder instead of raising during error formatting.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— 883 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— 205 passed.ruff checkon the touched files (no new findings),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.New regressions exhaust the send pool and the deadline after a redirect onto URLs carrying
access_token,X-Amz-Signature, fragment credentials, userinfo, and a malformed port, asserting the secrets are absent from the error while the destination host and path remain, and that malformed authorities never make error formatting raise.