feat(#3164): add retry on 5xx for Testing Farm API calls - #3165
feat(#3164): add retry on 5xx for Testing Farm API calls#3165fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
Replace the plain integer max_retries=5 on the HTTPAdapter with a proper urllib3 Retry strategy that retries on HTTP 500, 502, 503, and 504 status codes with exponential backoff (backoff_factor=1). The previous configuration only retried transport-level connection errors, not server-side HTTP error responses. The retry strategy explicitly allows GET, POST, and DELETE methods so that TF request submission and cancellation are also retried. Closes #3164
|
🤖 Finished Review · ✅ Success · Started 12:08 PM UTC · Completed 12:25 PM UTC Commit: |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 59s |
ReviewFindingsMedium
Low
Labels: PR modifies Testing Farm API client retry logic in packit_service/worker/helpers/testing_farm_client.py Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
packit_service/worker/helpers/testing_farm_client.py(file-level): Line 97 · [medium] error handling gap
The send_testing_farm_request method catches only requests.exceptions.ConnectionError, but with the new Retry strategy and its default raise_on_status=True, exhausted retries on a 5xx response raise requests.exceptions.RetryError (wrapping urllib3 MaxRetryError). RetryError inherits from RequestException, not ConnectionError, so it propagates uncaught. This is a behavior change: previously a 5xx was returned as a normal response for callers to check via status code; now it raises an exception after retry exhaustion. In cancel(), a RetryError during cancel_running_tests() would abort remaining cancellations. In get_request_details(), the exception propagates instead of returning {}.
Suggested fix: Set raise_on_status=False in the Retry constructor so that after retry exhaustion the last 5xx response is returned normally (preserving the existing control flow), or add requests.exceptions.RetryError to the except clause in send_testing_farm_request.
packit_service/worker/helpers/testing_farm_client.py:37: [low] edge case
The retry strategy with backoff_factor=1 and total=5 produces exponential backoff sleeps of 0s, 2s, 4s, 8s, 16s (~30s total). Combined with HTTP_REQUEST_TIMEOUT per attempt, a persistently failing endpoint could block a worker for roughly 5 * HTTP_REQUEST_TIMEOUT + 30 seconds.
fullsend-ai-review
Bot
added
the
area/testing-farm
| total=5, | ||
| backoff_factor=1, | ||
| status_forcelist=[500, 502, 503, 504], | ||
| allowed_methods=["GET", "POST", "DELETE"], |
There was a problem hiding this comment.
I am unsure how safe it would be to just blindly retry POST requests. I would remove it as it seems to me that all POST requests are already covered by existing retry mechanisms (unless I am mistaken).
I can see that TestingFarmJobHelper.prepare_and_send_tf_request() makes a POST request and the method's dosctring states:
Prepare the payload that will be sent to Testing Farm, submit it to
TF API and handle the response (report whether the request was sent
successfully, store the new TF run in DB or retry if needed).
It uses the def _retry_on_submit_failure() method for this. Same goes for DownstreamTestingFarmJobHelper.prepare_and_send_tf_request().
There was a problem hiding this comment.
I don't think having retries at multiple levels hurts anything, I think it could even save some resources. But perhaps @thrix can confirm if retrying POST requests on 5xx errors is safe.
3 participants
Summary
urllib3.util.retry.Retrystrategy on theTestingFarmClientHTTP session to automatically retry API requests that return 5xx status codes (500, 502, 503, 504) with exponential backoff.HTTPAdapter(max_retries=5)with a plain integer only retried transport-level connection errors, not server-side HTTP error responses like the HTTP 500s observed in TF API.Testing
TestTestingFarmClientRetrytest class with two tests verifying the retry strategy configuration (status codes, backoff factor, allowed methods).Closes #3164
Post-script verification
agent/3164-tf-api-retry-mechanism)b828c3d7b987c760aa3c09a82d99e69c04e23b68..HEAD)