Repository navigation
Conversation
Every session call in InfisicalRequests ran without a timeout argument. The requests default of timeout=None leaves the socket with no deadline. A server that accepts a connection and never replies blocks the caller indefinitely. NETWORK_ERRORS already lists ReadTimeout and ConnectTimeout, so those retry branches were unreachable. Add a timeout parameter to InfisicalRequests and InfisicalSDKClient. It defaults to 30 seconds and is passed to every request. Passing None restores the previous behavior.
Contributor
|
| Filename | Overview |
|---|---|
| infisical_sdk/client.py | Adds a nullable timeout constructor option, documents its per-attempt semantics, and forwards it to the shared transport. |
| infisical_sdk/infisical_requests.py | Applies the timeout to every HTTP method and excludes read timeouts from write retries to avoid replaying uncertain writes. |
| tests/test_infisical_requests.py | Verifies timeout propagation and retry classification while mocking retry sleeps to keep tests fast. |
Reviews (4): Last reviewed commit: "test: mock the retry backoff so the suit..." | Re-trigger Greptile
The new default made ReadTimeout reachable for the first time. It is in NETWORK_ERRORS, and post, patch and delete carry the same retry decorator as get, so a write whose response was slow would be resent up to four times after the server may already have applied it. Writes now retry on a set that excludes ReadTimeout. A connect timeout still retries on every verb, because the request never reached the server. A read timeout on a write reaches the caller. Also annotate the client timeout as Optional[float], since None is a documented value, and say in the docstring that the value applies to the connection and to the response on each attempt.
codechrl
marked this pull request as draft
September 7, 2026 06:54
codechrl
marked this pull request as ready for review
September 7, 2026 06:54
Author
|
@greptileai fixes pushed, please re-review |
The retry tests exercised the real exponential backoff, which added about a minute to the run for no added coverage.
Author
|
@greptileai fixes pushed, please re-review |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
InfisicalRequestscalledsession.get,post,patchanddeletewithout atimeout, so every request ran with therequestsdefault oftimeout=None. A server that accepts the connection and never replies leaves the caller blocked with no deadline and no exception.NETWORK_ERRORSalready listsReadTimeoutandConnectTimeout, so thosewith_retrybranches could never be reached.InfisicalRequestsandInfisicalSDKClientnow take atimeoutparameter that defaults to 30 seconds and is passed to every request. A caller that wants the old behavior passestimeout=None, and one on a slow network can raise it.No new dependencies. The default applies to existing callers without a code change on their side.