feat(client): retry transient failures, optionally indefinitely - #1323
Conversation
github-actions
Bot
added
the
type/documentation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## stable #1323 +/- ##
==========================================
+ Coverage 84.24% 84.88% +0.63%
==========================================
Files 147 148 +1
Lines 13068 13288 +220
Branches 1940 1955 +15
==========================================
+ Hits 11009 11279 +270
+ Misses 1494 1441 -53
- Partials 565 568 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Deploying infrahub-sdk-python with
|
| Latest commit: |
a0a185d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b26954fe.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://fac-retry-on-error-7mwrc.infrahub-sdk-python.pages.dev |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| retry_delay: int = Field( | ||
| default=5, | ||
| ge=0, | ||
| description=( | ||
| "Base delay in seconds before retrying a request that failed with a transient error. " | ||
| "The delay doubles after every attempt, with jitter, up to the maximum retry delay." | ||
| ), | ||
| ) | ||
| retry_max_delay: int = Field( | ||
| default=60, | ||
| ge=0, | ||
| description="Maximum delay in seconds between two retries of a request that failed with a transient error.", | ||
| ) |
There was a problem hiding this comment.
both of these settings use ge=0, and compute_backoff returns min(max_delay, ...), so either zero makes every retry sleep 0 seconds. Managed to make 20000+ attempts in one second against a persistent 502.
There was a problem hiding this comment.
Valid point, thanks for measuring it. Both settings are now gt=0 in 9e4f8ce; as integers that makes 1 second the floor. max_retry_duration keeps ge=0 since 0 means unlimited there.
| while not task.done(): | ||
| try: | ||
| await asyncio.wait({task}) | ||
| except asyncio.CancelledError: | ||
| continue |
There was a problem hiding this comment.
What happens if the source stream never returns from read()? It feels like things could get stuck here even at default config, with retries off.
There was a problem hiding this comment.
Same as before this PR: httpx reads the source synchronously while sending the body, so a read() that never returns hung the upload already, and in the async client the event loop with it. The copy moves that read to a worker thread. It runs regardless of the retry settings because the 429 backoff and the 401 re-login also re-send the body. What did get worse was cancellation, which waited for the worker: in 9e4f8ce a cancel surfaces at once and the worker closes the temporary file itself once its pending read returns.
| TRANSIENT_EXCEPTIONS = (ServerNotReachableError, ServerNotResponsiveError) | ||
| """Client-side failures (connection error, read timeout) that are always considered transient.""" | ||
|
|
||
| CONNECTION_LOST_EXCEPTIONS = (httpx.NetworkError, httpx.RemoteProtocolError) |
There was a problem hiding this comment.
Should ConnectTimeout, WriteTimeout and PoolTimeout be in that sequence to be retried too? WriteTimeout is used by the upload path this PR tries to harden.
There was a problem hiding this comment.
Yes to all three, in 9e4f8ce. ConnectTimeout joins the connection-lost set (a handshake left hanging by a load balancer dropping packets), and every catch site now takes httpx.TimeoutException, so write and pool timeouts map to ServerNotResponsiveError like read timeouts. Covered for both clients.
| except CONNECTION_LOST_EXCEPTIONS as exc: | ||
| raise ServerNotReachableError(address=self.address) from exc |
There was a problem hiding this comment.
Is this the right exception for a connection lost mid-download?
The catch around the yield now includes RemoteProtocolError, so a transfer truncated while the caller reads the body surfaces as "Unable to connect to ..." with no retry, and file_handler leaves a partial file with no hint, even though the server was reachable in the first place.
There was a problem hiding this comment.
Agreed, it was not. In 9e4f8ce a body breaking off raises ServerNotResponsiveError saying the connection was lost while reading the response, the file handler restarts the download on the shared retry budget when retries are on, and the partial file is removed either way.
| @@ -0,0 +1 @@ | |||
| With `retry_on_failure` enabled, the client now retries every transient failure instead of only connection errors: a connection dropped before any response arrives (the shape a load balancer failover or a server restart takes on an in-flight request), read timeouts, HTTP `500`/`502`/`503`/`504` responses and GraphQL errors the server flags with one of those statuses (for example a database that became unavailable mid-run). Retries apply to every request path, including multipart uploads, streamed downloads and REST endpoints such as `query_gql_query` used by generators, and use exponential backoff with jitter (`retry_delay` as the base, capped by the new `retry_max_delay`) instead of retrying HTTP 5xx responses in a tight loop without any delay. The new `retry_status_codes` setting tunes which statuses count as transient, and `max_retry_duration=0` retries indefinitely so long-running generators can survive an outage rather than abort. Retries are logged with the attempt number and elapsed time, escalating from `WARNING` to `ERROR` after five minutes, and once the budget is exhausted the original error is raised instead of a generic one. | |||
There was a problem hiding this comment.
This can probably be shorten without losing its meaning.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| try: | ||
| async with await anyio.Path(dest).open("wb") as f: | ||
| async for chunk in resp.aiter_bytes(chunk_size=65536): | ||
| await f.write(chunk) | ||
| bytes_written += len(chunk) | ||
| return bytes_written | ||
| except ServerNotReachableError: | ||
| self._client.log.error(f"Unable to connect to {self._client.address}") | ||
| raise | ||
| except BaseException: | ||
| await anyio.Path(dest).unlink(missing_ok=True) | ||
| raise | ||
| return bytes_written |
There was a problem hiding this comment.
A failed open("wb") now also triggers the unlink, so a pre-existing dest gets deleted by an attempt that never wrote to it (to reproduce: dest with mode 0444, open raises PermissionError, the file is gone, on stable it survived). The file could be opened before the try, and the unlink wrapped in suppress(OSError) so a failed cleanup does not replace the real error. Same in the sync version of this code.
There was a problem hiding this comment.
Good catch, the open should not have been inside the try. In 980932f the file is opened first so a dest that cannot be opened is left untouched, and the unlink runs after the file is closed, inside suppress(OSError), so the transfer error stays the one raised. Same in the sync handler, with tests on both clients for a pre-existing dest that cannot be opened and for a cleanup that fails.
| while True: | ||
| try: | ||
| return await self._download_to_file(url=url, dest=dest, retry_state=retry_state) | ||
| except (ServerNotReachableError, ServerNotResponsiveError) as exc: |
There was a problem hiding this comment.
This tuple of error classes could be replaced by TRANSIENT_EXCEPTIONS
There was a problem hiding this comment.
It was TRANSIENT_EXCEPTIONS at first, but ruff's DOC501 cannot resolve an imported tuple against the documented Raises entries and flags the bare raise, whereas the same pattern passes in retry.py because the tuple is local there. Listing the two classes keeps the except clause aligned with the docstring without a noqa.
retry_on_failure only covered connection errors inside execute_graphql, gave up after max_retry_duration, spun without delay on HTTP 5xx, and never looked at REST calls or at GraphQL error envelopes, so a generator running for hours aborted on the first transient infrastructure error. A TransientRetryHandler now drives every request path of both clients: GraphQL queries and mutations, REST endpoints such as query_gql_query and schema loading, multipart uploads and streamed downloads. It retries connection errors, connections dropped before a response arrives (the shape a load balancer failover takes on an in-flight request), every httpx timeout, HTTP 500/502/503/504 responses and GraphQL envelopes whose errors all carry one of those statuses, sharing one time budget and attempt counter across the transport and envelope layers. Anything else still fails fast. Retries use exponential backoff with jitter from retry_delay up to the new retry_max_delay; a zero for either is rejected since it would turn every retry into a tight loop. retry_status_codes tunes which statuses count as transient, and max_retry_duration=0 retries indefinitely. Each retry is logged with its attempt number and elapsed time, escalating from WARNING to ERROR after five minutes, and once the budget is spent the original error is raised. retry_on_failure and retry_delay are properties so they can be toggled at runtime. Uploads from a stream that cannot be rewound are copied once to a temporary file, off the event loop in the async client, so a retried send carries the full body. A streamed download whose connection drops mid-body raises ServerNotResponsiveError, is restarted on the shared budget by the file handler, and never leaves a partial file behind. Opt-in integration tests (INFRAHUB_TESTING_FAILOVER=1) restart HAProxy and the API servers mid-mutation against a real deployment. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
980932f to
a0a185d
Compare
2 participants
Summary
Generators and other long-running SDK jobs abort as soon as a single GraphQL call hits a transient infrastructure error, even though the task worker already runs the client with
retry_on_failure=True. That switch only covered connection errors insideexecute_graphql, gave up after 5 minutes, spun without any delay on HTTP 5xx, and never looked at REST calls or at GraphQL error envelopes. This PR turnsretry_on_failureinto a real transient-error policy and lets operators opt into retrying indefinitely, so a multi-hour generator survives a database failover or a network outage instead of failing.Key changes
retry_on_failureenabled, connection errors, timeouts, HTTP 500/502/503/504 responses and GraphQL errors the server flags with one of those statuses are retried. Everything else still fails fast, so a bad query or a schema error surfaces immediately even in unlimited mode.RemoteProtocolErrorrather than a network error, so it used to escape the retry handler entirely; it is now mapped toServerNotReachableErrorlike any other lost connection, on all six transport paths of both clients. See the section below for how this was found.ReadTimeout.ConnectTimeoutis a handshake left hanging by a load balancer dropping packets mid-failover and maps toServerNotReachableError;WriteTimeout(an upload the server stopped draining) andPoolTimeoutmap toServerNotResponsiveErrorlike a read timeout. Previously all three escaped the retry handler as raw httpx exceptions.query_gql_querydata collection, schema loading and the other REST endpoints. The GraphQL-envelope retry shares one time budget and attempt counter with the transport layer.retry_delayis the base and the newretry_max_delaycaps it. This also fixes the 5xx busy loop.max_retry_duration=0now means retry indefinitely. On the task worker,INFRAHUB_MAX_RETRY_DURATION=0is enough to opt in because the worker already setsretry_on_failure=True.retry_status_codessetting controls which statuses count as transient. 500 is included because Infrahub reports some transient database errors (Neo4j transient or session errors exhausting the server-side retries) without further classification.client.retry_on_failureandclient.retry_delaybecome properties so they can still be toggled at runtime, for example by a generator.Verified against a real failover
The policy was exercised against a real Infrahub deployment from
infrahub-testcontainers(two API replicas behind HAProxy, plus Neo4j, RabbitMQ, Redis and Prefect) rather than against mocks alone. The server's/api/response-delayendpoint makes every GraphQL request sit for 10 seconds, which is a wide enough window to kill a container while a mutation is genuinely in flight.That found the feature aborting on the exact scenario it exists for. With
retry_on_failure=Trueandmax_retry_duration=0, restarting HAProxy during a mutation killed the operation after a single attempt:A load balancer that goes away mid-request closes the socket before sending a single byte of the response, and httpx reports that as
RemoteProtocolError, aProtocolErrorrather than aNetworkError. OnlyNetworkErrorandReadTimeoutwere mapped to the SDK exceptions the retry handler treats as transient, so a failover bypassed the handler entirely: zero retries, unlimited budget notwithstanding. With the mapping added here, the same failover only delays the mutation:A failover reaches the client in one of two shapes, depending on which side of the load balancer goes away. Both are now covered:
RemoteProtocolError)retry_status_codes, which already workedThe at-least-once caveat below turned out not to be theoretical. The first run used a plain create and the retry came back with
Violates uniqueness constraint 'name': the server keeps processing a request whose client has gone away, so attempt one had already written the node before the socket closed. The integration tests usesave(allow_upsert=True)for that reason.Caveats
retry_status_codesto fail fast on it..infrahub.ymlneeds an Infrahub-side change to forward the flag to the client and is left as a follow-up.Documentation
docs/docs/python-sdk/guides/client.mdx: new "Retry transient failures" section under advanced use cases, listing a dropped connection among the transient failures.docs/docs/python-sdk/reference/config.mdxanddocs/docs/python-sdk/sdk_ref/infrahub_sdk/client.mdx.changelog/+transient-retry.added.md,changelog/+retry-5xx-busy-loop.fixed.mdandchangelog/+retry-connection-dropped-mid-request.fixed.md.Test plan
uv run pytest tests/unit/sdk/test_retry.py: 78 tests covering classification, backoff, budget and unlimited mode, the shared budget across layers, the opt-in default, runtime toggling, environment plumbing and async/sync parity. 12 of them are new and drive a lost connection through httpx itself, so theRemoteProtocolErrormapping is covered without needing Docker.uv run pytest tests/integration/test_retry_on_failover.py: 4 tests against the deployment described above, 3m30s. Three restart HAProxy mid-mutation (retries disabled, async retry-forever, sync retry-forever) and all three fail without the mapping, so they are a real regression test rather than a demonstration. The fourth restarts the API servers instead and asserts the retry happened on a 502, not merely that the mutation eventually succeeded.uv run invoke lint-codepasses.uv run invoke lint-docspasses rumdl; vale was not available locally.tests/unit/ctlfailures.🤖 Generated with Claude Code
Summary by cubic
Turns
retry_on_failurein theinfrahub_sdkclient into a real transient-error policy. Previously it retried only connection errors insideexecute_graphql, capped at 5 minutes, and skipped REST calls, GraphQL error envelopes, the no-delay 5xx busy loop, and connections dropped before a response arrived. Now connection errors, dropped connections, every httpx timeout, HTTP 500/502/503/504 responses, and GraphQL envelopes flagged with those statuses are retried on every request path—multipart uploads and streamed downloads included—with exponential backoff and jitter, optionally forever.Key changes
retry_max_delaycaps the backoff;retry_status_codestunes which statuses count as transient (500 is in the default set because Infrahub reports some transient DB errors without classification). A zeroretry_delayorretry_max_delayis rejected, since either made every retry a tight loop.retry_on_failureandretry_delaycan now be toggled at runtime.ServerNotReachableErrorand retried;ConnectTimeoutmaps there too and the remaining timeouts (write, pool) toServerNotResponsiveError. Opt-in integration tests (INFRAHUB_TESTING_FAILOVER=1) restart HAProxy mid-mutation to cover it, with the sync case time-bounded.ServerNotResponsiveError, restarted on the shared retry budget, and the partial file is removed after a failed attempt. Streamed transient responses are only pre-read when a retry will actually happen, so a spent budget returns the response open instead of fetching its body. A destination that cannot be opened is left untouched, and a failed cleanup never replaces the transfer error.Caveats
retry_status_codesif you want genuine bugs to fail fast instead of retrying until the budget expires..infrahub.ymlis left as a follow-up.Written for commit a0a185d. Summary will update on new commits.