feat(api): add connection retries and request timeout to API client#251
Open
ocervell wants to merge 1 commit into
Open
feat(api): add connection retries and request timeout to API client#251ocervell wants to merge 1 commit into
ocervell wants to merge 1 commit into
Conversation
APIClient issued every request through the bare requests.* functions with no session, no retry policy, and no timeout. Transient connection/TLS failures (e.g. a reset handshake raising SSLEOFError) and brief 5xx responses failed on the first attempt, and a hung server could block forever. Route all requests through a requests.Session with a urllib3 Retry mounted on http:// and https://, plus a default per-request timeout. Retries cover connection establishment (all methods) and the retryable status codes [429, 500, 502, 503, 504] for idempotent methods only, so POST/PATCH are never replayed. raise_on_status=False preserves the existing response.raise_for_status() error semantics. All values are tunable in settings and overridable per call. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5vSjfkBuGAAHdKxHS3ySm
aronmolnar
reviewed
Jul 9, 2026
aronmolnar
left a comment
aronmolnar
left a comment
Contributor
There was a problem hiding this comment.
Thank you very much for your Pull request.
I added some notes. Would you mind looking at them?
Thank you!
| return kwargs | { | ||
| 'headers': kwargs.get('headers', {}) | self._get_headers(json_content=json_content), | ||
| 'verify': kwargs.get('verify', self.verify), | ||
| 'timeout': kwargs.get('timeout', settings.API_TIMEOUT), |
Contributor
There was a problem hiding this comment.
A default timeout of 30 seconds prevents long-running requests to fail. This would affect, for example rendering processes of large reports or project exports. Maybe the timeout should be increased for those endpoints to max(5m, API_TIMEOUT)
| API_TIMEOUT = 30 # per-request (connect, read) timeout in seconds; prevents indefinite hangs | ||
| API_MAX_RETRIES = 3 # retries for transient connection/TLS failures and retryable status codes | ||
| API_RETRY_BACKOFF_FACTOR = 0.5 # exponential backoff: 0s, 0.5s, 1s, 2s, ... | ||
| API_RETRY_STATUS_FORCELIST = [429, 500, 502, 503, 504] # only these statuses are retried |
Contributor
There was a problem hiding this comment.
The users should probably have the option to control those values (at least API_TIMEOUT) via CLI options of via their config.
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
2 participants
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.
Problem
APIClientissues every request through the bare module-levelrequests.get/post/...functions with no session, no retry policy, and no timeout. Two consequences:SSLEOFError/UNEXPECTED_EOF_WHILE_READING), a dropped connection, or a brief 502/503 from a reverse proxy propagates straight to the caller on the first attempt, with no retry.requestswith notimeout=waits indefinitely, so a stalled connection can hang a script or the CLI with no ceiling.Change
APIClientnow routes all requests through arequests.Sessionwith aurllib3Retrypolicy mounted on bothhttp://andhttps://, plus a default per-request timeout.[429, 500, 502, 503, 504]. Backoff is exponential (backoff_factor=0.5), andRetry-Afteris honored.allowed_methods), soPOST/PATCHare never replayed against the server — no double-creates.raise_on_status=Falsepreserves existing error semantics: after retries are exhausted, the response is returned and the existingresponse.raise_for_status()in_do_requestraises exactly as before.All values are tunable in
reptor/settings.pyand overridable per-call viakwargs.Files
reptor/api/APIClient.py_build_session();__init__buildsself._session;_prepare_kwargsinjects defaulttimeout;_do_requestuses the sessionreptor/settings.pyAPI_TIMEOUT,API_MAX_RETRIES,API_RETRY_BACKOFF_FACTOR,API_RETRY_STATUS_FORCELISTreptor/api/tests/test_apiclient.pyinsecure→verify=False, and that requests route through the sessionTesting
299 passedon the non-integration suite (including the 5 new tests). The only failures observed locally are pre-existing GhostWriter tests that fail identically on unmodifiedmaindue to a missing optionalgqldependency — unrelated to this change.Notes
Backward-compatible: no public API changes, all defaults overridable. This does not paper over a deterministic misconfiguration (wrong scheme/port, hard TLS-version mismatch) — those still fail after retries — but it makes the client resilient to genuinely transient failures and bounds request time.