Conversation
Start retrying HTTPError exceptions from code `408` instead of `500` as it was before. With that we hope to catch missing relevant statues in which is worth retrying Refers to SPSTRAT-725 Signed-off-by: Jonathan Gangi <jgangi@redhat.com>
Asked cursor to implement the unit-tests. Description: Here is what was added and how it behaves. requests_mock replaces requests.Session.send, so requests never use urllib3’s normal path and HTTPAdapter retries do not run. The new tests use a real thread-local HTTP server and mount the same HTTPS retry adapter on http:// so urllib3 retry behaves like production. 1. test_default_status_forcelist_is_408_through_511 – Checks the mounted adapter’s Retry: total == 5, backoff_factor == 1, and status_forcelist == tuple(range(408, 512)) (i.e. 408–511). 2. GET/PUT success after retry – Parametrized GET (408, 429, 500, 511) and one PUT (503): first response error from the forcelist, second 200, asserts two server hits. 3. No retry outside forcelist – GET returns 403 or 407 then would-be 200: only one hit; client keeps the 403/407 response. 4. 512 – range(408, 512) stops at 511, so 512 is not retried (one hit, status 512). 5. test_urllib3_retry_forcelist_covers_408_through_511 – Uses urllib3’s Retry.is_retry(method="GET", ...) so every code 408–511 is treated as retryable and 407 / 512 are not. 6. test_post_does_not_retry_by_default_urllib3 – Documents urllib3’s default: POST is not in allowed_methods, so 500 then 200 still yields a single attempt with status 500. _get_token is patched so tests never call Microsoft login; backoff_factor=0 keeps waits negligible. Refers to SPSTRAT-725 Signed-off-by: Jonathan Gangi <jgangi@redhat.com> Assisted-by: Cursor/Gemini
Collaborator
Author
Reviewer's GuideExtends PartnerPortalSession’s HTTP retry policy to cover HTTP 408–511 responses and adds end-to-end tests using a local HTTP server to verify urllib3 Retry behavior across GET, PUT, and POST methods, including which status codes are retried or not. Sequence diagram for PartnerPortalSession HTTP request with expanded retry rangesequenceDiagram
actor Tester
participant TestCode
participant PartnerPortalSession
participant RequestsSession
participant AzureEndpoint
Tester->>TestCode: trigger operation()
TestCode->>PartnerPortalSession: get_resource()
PartnerPortalSession->>RequestsSession: request(GET /resource)
RequestsSession->>AzureEndpoint: HTTP GET /resource
AzureEndpoint-->>RequestsSession: 408 Request Timeout
RequestsSession-->>PartnerPortalSession: HTTPError(408)
PartnerPortalSession->>PartnerPortalSession: Retry(status_forcelist=408-511)
PartnerPortalSession->>RequestsSession: retry request(GET /resource)
RequestsSession->>AzureEndpoint: HTTP GET /resource
AzureEndpoint-->>RequestsSession: 200 OK
RequestsSession-->>PartnerPortalSession: Response(200)
PartnerPortalSession-->>TestCode: Response(200)
TestCode-->>Tester: success
Updated class diagram for PartnerPortalSession retry configurationclassDiagram
class PartnerPortalSession {
- requests_Session session
- int total_retries
- float backoff_factor
- tuple status_forcelist
+ __init__(total_retries, backoff_factor, status_forcelist)
+ request(method, url, headers, data, params)
}
class Retry {
- int total
- float backoff_factor
- tuple status_forcelist
+ __init__(total, backoff_factor, status_forcelist)
+ increment(method, url, response, error)
}
PartnerPortalSession --> Retry : configures
%% Default behavior after this PR
PartnerPortalSession : status_forcelist = range(408,512)
Retry : status_forcelist = range(408,512)
Flow diagram for retry decision on HTTP status codesflowchart LR
A[Start HTTP request] --> B[Receive HTTP response]
B --> C{Is exception HTTPError?}
C -- No --> Z[Return response]
C -- Yes --> D[Get status code]
D --> E{status code in
408-511?}
E -- No --> Z
E -- Yes --> F[Check remaining retries]
F --> G{retries left > 0?}
G -- No --> Z
G -- Yes --> H[Apply backoff]
H --> I[Retry HTTP request]
I --> B
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
PartnerPortalSession'sRetryconfiguration, consider explicitly settingallowed_methodsinstead of relying on urllib3 defaults, so POST non-retry behavior is stable even if urllib3 changes its defaults in the future. - The test helpers that start a local HTTP server repeat try/finally shutdown logic in multiple tests; you could factor this into a context manager or pytest fixture to centralize server lifecycle management and reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PartnerPortalSession`'s `Retry` configuration, consider explicitly setting `allowed_methods` instead of relying on urllib3 defaults, so POST non-retry behavior is stable even if urllib3 changes its defaults in the future.
- The test helpers that start a local HTTP server repeat try/finally shutdown logic in multiple tests; you could factor this into a context manager or pytest fixture to centralize server lifecycle management and reduce duplication.
## Individual Comments
### Comment 1
<location path="tests/ms_azure/test_session.py" line_range="317-321" />
<code_context>
+ assert retry.backoff_factor == 1
+ assert retry.status_forcelist == tuple(range(408, 512))
+
+ @pytest.mark.parametrize(
+ "first_status",
+ [408, 429, 500, 511],
+ )
+ def test_get_retries_on_forcelist_status_then_succeeds(
+ self,
+ auth_dict: Dict[str, str],
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case where retries are exhausted without a successful response to verify the retry limit behavior.
Currently we only cover the case where a retry eventually succeeds. Please add a test where the server keeps returning a forcelist status (e.g., 500) for more than `total_retries`, and assert both the final status and the number of calls to confirm we honor the retry limit exactly (no infinite or under-retrying).
Suggested implementation:
```python
retry = adapter.max_retries
assert retry.total == 5
assert retry.backoff_factor == 1
assert retry.status_forcelist == tuple(range(408, 512))
@pytest.mark.parametrize(
"status_code",
[408, 429, 500, 511],
)
def test_get_retries_exhausted_on_forcelist_status(
auth_dict: Dict[str, str],
port: int,
) -> None:
"""Verify that we honor the retry limit exactly when the server never recovers.
The server keeps returning a forcelist status so all retries are exhausted.
We assert both the final status and the exact number of calls performed.
"""
# Build a session that targets a local HTTP server with the retry adapter.
session = _build_local_partner_portal_session(auth_dict=auth_dict, port=port)
# The adapter is configured with a total of 5 retries (see test above),
# which means we should see 1 initial request + 5 retries = 6 calls.
expected_total_retries = session.get_adapter("http://").max_retries.total
expected_calls = expected_total_retries + 1
call_count = 0
def _failing_handler(request):
nonlocal call_count
call_count += 1
# Always return a status code from the forcelist so that retries are triggered.
from requests import Response
response = Response()
response.status_code = status_code
response._content = b"forced failure" # type: ignore[attr-defined]
response.url = request.url # type: ignore[attr-defined]
return response
# Configure the local server to always respond with the given status_code.
# This assumes the existing test infrastructure already exposes a way
# to register a handler for the local HTTP server that listens on `port`.
_register_handler_for_local_server(port=port, handler=_failing_handler)
# Perform a GET to trigger the retry logic. The request path and params
# should match whatever is used in the success-case test so we exercise
# the same code path here.
response = session.get("")
# We never succeed, so the final status should be the forcelist status.
assert response.status_code == status_code
# Ensure we honored the retry limit exactly: one initial call plus `total` retries.
assert call_count == expected_calls
```
To make this compile and integrate cleanly, adjust the following to match your existing test helpers and fixtures:
1. Replace `_build_local_partner_portal_session` with the actual helper function you are defining just below the shown snippet (the one that builds a `PartnerPortalSession` for a local HTTP server). The signature I assumed is:
```python
def _build_local_partner_portal_session(
auth_dict: Dict[str, str],
port: int,
**session_kwargs: Any,
) -> PartnerPortalSession:
...
```
If the name or arguments differ, update the call in `test_get_retries_exhausted_on_forcelist_status` accordingly.
2. Implement `_register_handler_for_local_server(port: int, handler: Callable)` or replace this with the existing mechanism you already use in `test_get_retries_on_forcelist_status_then_succeeds` to control responses from the local HTTP server. The intent is:
- For every request received on the test server listening on `port`, invoke `handler(request)` and return its `Response`.
- Ensure all responses during this test use the forcelist status so retries never succeed.
3. Ensure the request path and query parameters in `session.get("")` (or equivalent) match what your production code uses and what the success-case test uses, so that the retry adapter is actually exercised.
4. If you already have a more direct way to count calls (e.g., a counter on the server fixture), replace the `call_count` logic with that, but keep the assertion `calls == total_retries + 1`.
5. If your project already imports `Response` or uses a different way to construct responses for the local server, adjust the `_failing_handler` implementation accordingly to stay consistent with the rest of the test file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+317
to
+321
| @pytest.mark.parametrize( | ||
| "first_status", | ||
| [408, 429, 500, 511], | ||
| ) | ||
| def test_get_retries_on_forcelist_status_then_succeeds( |
There was a problem hiding this comment.
suggestion (testing): Add a test case where retries are exhausted without a successful response to verify the retry limit behavior.
Currently we only cover the case where a retry eventually succeeds. Please add a test where the server keeps returning a forcelist status (e.g., 500) for more than total_retries, and assert both the final status and the number of calls to confirm we honor the retry limit exactly (no infinite or under-retrying).
Suggested implementation:
retry = adapter.max_retries
assert retry.total == 5
assert retry.backoff_factor == 1
assert retry.status_forcelist == tuple(range(408, 512))
@pytest.mark.parametrize(
"status_code",
[408, 429, 500, 511],
)
def test_get_retries_exhausted_on_forcelist_status(
auth_dict: Dict[str, str],
port: int,
) -> None:
"""Verify that we honor the retry limit exactly when the server never recovers.
The server keeps returning a forcelist status so all retries are exhausted.
We assert both the final status and the exact number of calls performed.
"""
# Build a session that targets a local HTTP server with the retry adapter.
session = _build_local_partner_portal_session(auth_dict=auth_dict, port=port)
# The adapter is configured with a total of 5 retries (see test above),
# which means we should see 1 initial request + 5 retries = 6 calls.
expected_total_retries = session.get_adapter("http://").max_retries.total
expected_calls = expected_total_retries + 1
call_count = 0
def _failing_handler(request):
nonlocal call_count
call_count += 1
# Always return a status code from the forcelist so that retries are triggered.
from requests import Response
response = Response()
response.status_code = status_code
response._content = b"forced failure" # type: ignore[attr-defined]
response.url = request.url # type: ignore[attr-defined]
return response
# Configure the local server to always respond with the given status_code.
# This assumes the existing test infrastructure already exposes a way
# to register a handler for the local HTTP server that listens on `port`.
_register_handler_for_local_server(port=port, handler=_failing_handler)
# Perform a GET to trigger the retry logic. The request path and params
# should match whatever is used in the success-case test so we exercise
# the same code path here.
response = session.get("")
# We never succeed, so the final status should be the forcelist status.
assert response.status_code == status_code
# Ensure we honored the retry limit exactly: one initial call plus `total` retries.
assert call_count == expected_callsTo make this compile and integrate cleanly, adjust the following to match your existing test helpers and fixtures:
-
Replace
_build_local_partner_portal_sessionwith the actual helper function you are defining just below the shown snippet (the one that builds aPartnerPortalSessionfor a local HTTP server). The signature I assumed is:def _build_local_partner_portal_session( auth_dict: Dict[str, str], port: int, **session_kwargs: Any, ) -> PartnerPortalSession: ...
If the name or arguments differ, update the call in
test_get_retries_exhausted_on_forcelist_statusaccordingly. -
Implement
_register_handler_for_local_server(port: int, handler: Callable)or replace this with the existing mechanism you already use intest_get_retries_on_forcelist_status_then_succeedsto control responses from the local HTTP server. The intent is:- For every request received on the test server listening on
port, invokehandler(request)and return itsResponse. - Ensure all responses during this test use the forcelist status so retries never succeed.
- For every request received on the test server listening on
-
Ensure the request path and query parameters in
session.get("")(or equivalent) match what your production code uses and what the success-case test uses, so that the retry adapter is actually exercised. -
If you already have a more direct way to count calls (e.g., a counter on the server fixture), replace the
call_countlogic with that, but keep the assertioncalls == total_retries + 1. -
If your project already imports
Responseor uses a different way to construct responses for the local server, adjust the_failing_handlerimplementation accordingly to stay consistent with the rest of the test file.
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
1 participant
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.
Changes
1 - Increase the HTTPError retry range
Start retrying HTTPError exceptions from code
408instead of500as it was before.With that we hope to catch missing relevant statues in which is worth retrying
2 - Ensure retrying mechanism works
Asked cursor to implement the unit-tests to ensure the current retrying mechanism is effective
JIRA
Refers to SPSTRAT-725
Summary by Sourcery
Broaden Azure PartnerPortalSession HTTP retry status codes and add tests to validate the retry behavior end to end.
Bug Fixes:
Enhancements:
Tests: