Fix LRO poller to treat HTTP 404/406 as transient during transaction-status polling#46310
Fix LRO poller to treat HTTP 404/406 as transient during transaction-status polling#46310
Conversation
…ion-status polling Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-python/sessions/6789a40b-e0da-4d5e-9cce-6cd42b9b4bd1 Co-authored-by: PallabPaul <20746357+PallabPaul@users.noreply.github.com>
…handling Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-python/sessions/dbd04bba-1b0d-4c00-92c0-5ee3d41d6653 Co-authored-by: PallabPaul <20746357+PallabPaul@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adjusts the Confidential Ledger LRO poller behavior so that HTTP 404 and 406 during transaction-status polling are treated as transient (with different retry semantics), and adds regression tests to lock in the expected behavior.
Changes:
- Treat
HttpResponseErrorwith status code 406 as transient during polling whenretry_not_found=True(sync + async). - Restore/maintain 404 retry counter behavior (give up after 3 404s) while keeping
InvalidTransactionIdas non-retryable. - Add new pytest suite covering sync/async 404 + 406 retry scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sdk/confidentialledger/azure-confidentialledger/tests/test_polling.py | Adds sync + async regression tests for transient 404/406 polling behavior. |
| sdk/confidentialledger/azure-confidentialledger/azure/confidentialledger/_operations/_patch.py | Adds explicit HttpResponseError handling for 406 in sync poller loop. |
| sdk/confidentialledger/azure-confidentialledger/azure/confidentialledger/aio/_operations/_patch.py | Adds explicit HttpResponseError handling for 406 in async poller loop. |
| if not_retryable or self._not_found_count >= 3: | ||
| raise |
There was a problem hiding this comment.
The code enforces the 404 retry limit using a monotonic _not_found_count, but the tests/docstrings describe the behavior as '3 consecutive 404s'. As implemented, non-consecutive 404s (e.g., 404 → 406 → 404 → 406 → 404) will still eventually hit the limit and fail. Please either (a) reset _not_found_count after any successful poll / any non-404 attempt (including the 406 transient path) to make it truly 'consecutive', or (b) update the docstrings/tests to explicitly state the limit is on the total number of 404s during a polling run.
| return {"state": "Committed"} | ||
|
|
||
| poller = StatePollingMethod(operation, "Committed", 0, retry_not_found=True) | ||
| poller._status = "polling" |
There was a problem hiding this comment.
Tests are directly mutating the internal _status field, which makes them brittle to internal refactors (e.g., if the polling method changes how it tracks lifecycle state). Prefer driving the poller via its public surface (e.g., calling the appropriate initializer/start mechanism on the polling method/poller) or using a helper factory that performs whatever public setup is required, so the tests validate behavior rather than internal implementation details.
| def operation(): | ||
| raise _make_resource_not_found() | ||
|
|
||
| poller = StatePollingMethod(operation, "Committed", 0, retry_not_found=True) | ||
| poller._status = "polling" | ||
|
|
||
| with pytest.raises(ResourceNotFoundError): | ||
| poller.run() |
There was a problem hiding this comment.
This test intends to validate the 'give up after 3 retries' behavior, but it never asserts how many times operation() was invoked. Consider adding a call_count counter (as done in other tests) and asserting it equals 3 (or 4, depending on whether your definition is 'retries' vs 'attempts'), so the test actually guards the retry-limit semantics rather than only asserting that a failure eventually occurs.
_not_found_countfield inBaseStatePollingMethodResourceNotFoundErrorretry logic (with counter) in syncStatePollingMethod.run()HttpResponseErrorcatch for 406 in syncStatePollingMethod.run()AsyncStatePollingMethod.run()