Repository: ably/ably-python
Pull request #598 (2025-04-22 12:31:33)

[ECO-5305] fix: rest retry logic
- retry requests with 500..504
- cloudfront errors with >= 400 status

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit

- **Bug Fixes**
	- Enhanced retry logic for network requests to better handle server errors, CloudFront-specific errors, and timeouts, improving reliability with fallback to secondary hosts.
- **Tests**
	- Added tests verifying fallback to secondary hosts when primary REST endpoints fail due to server errors, timeouts, or CloudFront errors.
	- Simplified existing tests to more accurately simulate HTTP error responses and retry behavior.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

coderabbitai[bot]: <!-- This is an auto-generated comment: summarize by coderabbit.ai -->
<!-- This is an auto-generated comment: review in progress by coderabbit.ai -->

> [!NOTE]
> Currently processing new changes in this PR. This may take a few minutes, please wait...
> 
> <details>
> <summary>📥 Commits</summary>
> 
> Reviewing files that changed from the base of the PR and between 8f5c1cb9e92d1f9ca5887a410184711a7fb96fa5 and 5006068eb1a8ffbc46bee0f61bbd9bb7647ba163.
> 
> </details>
> 
> <details>
> <summary>📒 Files selected for processing (2)</summary>
> 
> * `ably/http/http.py` (3 hunks)
> * `test/ably/rest/restrequest_test.py` (1 hunks)
> 
> </details>
> 
> ```ascii
>  _____________________________
> < My other GPU is a Blackwell >
>  -----------------------------
>   \
>    \   (\__/)
>        (•ㅅ•)
>        / 　 づ
> ```

<!-- end of auto-generated comment: review in progress by coderabbit.ai -->
<!-- usage_tips_start -->

> [!TIP]
> <details>
> <summary>You can generate walkthrough in a markdown collapsible section to save space.</summary>
> 
> Enable the `reviews.collapse_walkthrough` setting in your project's settings in CodeRabbit to generate walkthrough in a markdown collapsible section.
> 
> </details>

<!-- usage_tips_end -->
<!-- tips_start -->

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

<details>
<summary>❤️ Share</summary>

- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai)
- [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai)
- [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai)
- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

</details>

<details>
<summary>🪧 Tips</summary>

### Chat

There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=ably/ably-python&utm_content=598):

- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  - `I pushed a fix in commit <commit_id>, please review it.`
  - `Generate unit testing code for this file.`
  - `Open a follow-up GitHub issue for this discussion.`
- Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples:
  - `@coderabbitai generate unit testing code for this file.`
  -	`@coderabbitai modularize this function.`
- PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  - `@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.`
  - `@coderabbitai read src/utils.ts and generate unit testing code.`
  - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.`
  - `@coderabbitai help me debug CodeRabbit configuration file.`

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

### CodeRabbit Commands (Invoked using PR comments)

- `@coderabbitai pause` to pause the reviews on a PR.
- `@coderabbitai resume` to resume the paused reviews.
- `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
- `@coderabbitai full review` to do a full review from scratch and review all the files again.
- `@coderabbitai summary` to regenerate the summary of the PR.
- `@coderabbitai generate docstrings` to [generate docstrings](https://docs.coderabbit.ai/finishing-touches/docstrings) for this PR.
- `@coderabbitai resolve` resolve all the CodeRabbit review comments.
- `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository.
- `@coderabbitai help` to get help.

### Other keywords and placeholders

- Add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed.
- Add `@coderabbitai summary` to generate the high-level summary at a specific location in the PR description.
- Add `@coderabbitai` anywhere in the PR title to generate the title automatically.

### CodeRabbit Configuration File (`.coderabbit.yaml`)

- You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository.
- Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json`

### Documentation and Community

- Visit our [Documentation](https://docs.coderabbit.ai) for detailed information on how to use CodeRabbit.
- Join our [Discord Community](http://discord.gg/coderabbit) to get help, request features, and share feedback.
- Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.

</details>

<!-- tips_end -->

Patch:
diff --git a/ably/http/http.py b/ably/http/http.py
index 6a5a4f7..8314da0 100644
--- a/ably/http/http.py
+++ b/ably/http/http.py
@@ -188,6 +188,11 @@ class Http:
 
         hosts = self.get_rest_hosts()
         for retry_count, host in enumerate(hosts):
+            def should_stop_retrying():
+                time_passed = time.time() - requested_at
+                # if it's the last try or cumulative timeout is done, we stop retrying
+                return retry_count == len(hosts) - 1 or time_passed > http_max_retry_duration
+
             base_url = "%s://%s:%d" % (self.preferred_scheme,
                                        host,
                                        self.preferred_port)
@@ -204,15 +209,25 @@ class Http:
             try:
                 response = await self.__client.send(request)
             except Exception as e:
-                # if last try or cumulative timeout is done, throw exception up
-                time_passed = time.time() - requested_at
-                if retry_count == len(hosts) - 1 or time_passed > http_max_retry_duration:
+                if should_stop_retrying():
                     raise e
             else:
+                # RSC15l4
+                cloud_front_error = (response.headers.get('Server', '').lower() == 'cloudfront'
+                                     and response.status_code >= 400)
+                # RSC15l3
+                retryable_server_error = response.status_code >= 500 and response.status_code <= 504
+                # Resending requests that have failed for other failure conditions will not fix the problem
+                # and will simply increase the load on other datacenters unnecessarily
+                should_fallback = cloud_front_error or retryable_server_error
+
                 try:
                     if raise_on_error:
                         AblyException.raise_for_response(response)
 
+                    if should_fallback and not should_stop_retrying():
+                        continue
+
                     # Keep fallback host for later (RSC15f)
                     if retry_count > 0 and host != self.options.get_rest_host():
                         self.__host = host
@@ -220,12 +235,7 @@ class Http:
 
                     return Response(response)
                 except AblyException as e:
-                    if not e.is_server_error:
-                        raise e
-
-                    # if last try or cumulative timeout is done, throw exception up
-                    time_passed = time.time() - requested_at
-                    if retry_count == len(hosts) - 1 or time_passed > http_max_retry_duration:
+                    if should_stop_retrying() or not should_fallback:
                         raise e
 
     async def delete(self, url, headers=None, skip_auth=False, timeout=None):


Test patch:
diff --git a/test/ably/rest/resthttp_test.py b/test/ably/rest/resthttp_test.py
index b5a2fa4..b6df6be 100644
--- a/test/ably/rest/resthttp_test.py
+++ b/test/ably/rest/resthttp_test.py
@@ -151,6 +151,7 @@ class TestRestHttp(BaseAsyncTestCase):
 
         await ably.close()
 
+    @respx.mock
     async def test_500_errors(self):
         """
         Raise error if all the servers reply with a 5xx error.
@@ -159,16 +160,13 @@ class TestRestHttp(BaseAsyncTestCase):
 
         ably = AblyRest(token="foo")
 
-        def raise_ably_exception(*args, **kwargs):
-            raise AblyException(message="", status_code=500, code=50000)
+        mock_request = respx.route().mock(return_value=httpx.Response(500, text="Internal Server Error"))
 
-        with mock.patch('httpx.Request', wraps=httpx.Request):
-            with mock.patch('ably.util.exceptions.AblyException.raise_for_response',
-                            side_effect=raise_ably_exception) as send_mock:
-                with pytest.raises(AblyException):
-                    await ably.http.make_request('GET', '/', skip_auth=True)
+        with pytest.raises(AblyException):
+            await ably.http.make_request('GET', '/', skip_auth=True)
+
+        assert mock_request.call_count == 3
 
-                assert send_mock.call_count == 3
         await ably.close()
 
     def test_custom_http_timeouts(self):
diff --git a/test/ably/rest/restrequest_test.py b/test/ably/rest/restrequest_test.py
index d0c9ad9..0f0cd62 100644
--- a/test/ably/rest/restrequest_test.py
+++ b/test/ably/rest/restrequest_test.py
@@ -126,6 +126,101 @@ class TestRestRequest(BaseAsyncTestCase, metaclass=VaryByProtocolTestsMetaclass)
             await ably.request('GET', '/time', version=Defaults.protocol_version)
         await ably.close()
 
+    # RSC15l3
+    @dont_vary_protocol
+    async def test_503_status_fallback(self):
+        default_endpoint = 'https://sandbox-rest.ably.io/time'
+        fallback_host = 'sandbox-a-fallback.ably-realtime.com'
+        fallback_endpoint = f'https://{fallback_host}/time'
+        ably = await TestApp.get_ably_rest(fallback_hosts=[fallback_host])
+        with respx.mock:
+            default_route = respx.get(default_endpoint)
+            fallback_route = respx.get(fallback_endpoint)
+            headers = {
+                "Content-Type": "application/json"
+            }
+            default_route.return_value = httpx.Response(503, headers=headers)
+            fallback_route.return_value = httpx.Response(200, headers=headers, text='[123]')
+            result = await ably.request('GET', '/time', version=Defaults.protocol_version)
+            assert default_route.called
+            assert result.status_code == 200
+            assert result.items[0] == 123
+        await ably.close()
+
+    # RSC15l2
+    @dont_vary_protocol
+    async def test_httpx_timeout_fallback(self):
+        default_endpoint = 'https://sandbox-rest.ably.io/time'
+        fallback_host = 'sandbox-a-fallback.ably-realtime.com'
+        fallback_endpoint = f'https://{fallback_host}/time'
+        ably = await TestApp.get_ably_rest(fallback_hosts=[fallback_host])
+        with respx.mock:
+            default_route = respx.get(default_endpoint)
+            fallback_route = respx.get(fallback_endpoint)
+            headers = {
+                "Content-Type": "application/json"
+            }
+            default_route.side_effect = httpx.ReadTimeout
+            fallback_route.return_value = httpx.Response(200, headers=headers, text='[123]')
+            result = await ably.request('GET', '/time', version=Defaults.protocol_version)
+            assert default_route.called
+            assert result.status_code == 200
+            assert result.items[0] == 123
+        await ably.close()
+
+    # RSC15l3
+    @dont_vary_protocol
+    async def test_503_status_fallback_on_publish(self):
+        default_endpoint = 'https://sandbox-rest.ably.io/channels/test/messages'
+        fallback_host = 'sandbox-a-fallback.ably-realtime.com'
+        fallback_endpoint = f'https://{fallback_host}/channels/test/messages'
+
+        fallback_response_text = (
+            '{"id": "unique_id:0", "channel": "test", "name": "test", "data": "data", '
+            '"clientId": null, "connectionId": "connection_id", "timestamp": 1696944145000, '
+            '"encoding": null}'
+        )
+
+        ably = await TestApp.get_ably_rest(fallback_hosts=[fallback_host])
+        with respx.mock:
+            default_route = respx.post(default_endpoint)
+            fallback_route = respx.post(fallback_endpoint)
+            headers = {
+                "Content-Type": "application/json"
+            }
+            default_route.return_value = httpx.Response(503, headers=headers)
+            fallback_route.return_value = httpx.Response(
+                200,
+                headers=headers,
+                text=fallback_response_text,
+            )
+            message_response = await ably.channels['test'].publish('test', 'data')
+            assert default_route.called
+            assert message_response.to_native()['data'] == 'data'
+        await ably.close()
+
+    # RSC15l4
+    @dont_vary_protocol
+    async def test_400_cloudfront_fallback(self):
+        default_endpoint = 'https://sandbox-rest.ably.io/time'
+        fallback_host = 'sandbox-a-fallback.ably-realtime.com'
+        fallback_endpoint = f'https://{fallback_host}/time'
+        ably = await TestApp.get_ably_rest(fallback_hosts=[fallback_host])
+        with respx.mock:
+            default_route = respx.get(default_endpoint)
+            fallback_route = respx.get(fallback_endpoint)
+            headers = {
+                "Server": "CloudFront",
+                "Content-Type": "application/json",
+            }
+            default_route.return_value = httpx.Response(400, headers=headers, text='[456]')
+            fallback_route.return_value = httpx.Response(200, headers=headers, text='[123]')
+            result = await ably.request('GET', '/time', version=Defaults.protocol_version)
+            assert default_route.called
+            assert result.status_code == 200
+            assert result.items[0] == 123
+        await ably.close()
+
     async def test_version(self):
         version = "150"  # chosen arbitrarily
         result = await self.ably.request('GET', '/time', "150")