From 1d4da9a2fc71ca2011c0d268ab208a983c27b265 Mon Sep 17 00:00:00 2001 From: Jesse Date: Fri, 15 Mar 2024 16:43:30 -0400 Subject: [PATCH] Don't retry network requests that fail with code 403 (#373) * Don't retry requests that fail with 404 Signed-off-by: Jesse Whitehouse * Fix lint error Signed-off-by: Jesse Whitehouse --------- Signed-off-by: Jesse Whitehouse --- CHANGELOG.md | 4 ++++ src/databricks/sql/auth/retry.py | 6 ++++++ tests/e2e/common/retry_test_mixins.py | 14 ++++++++++++++ 3 files changed, 24 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c972ab6..607656dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Release History +# x.x.x (TBD) + +- Don't retry requests that fail with code 403 + # 3.1.0 (2024-02-16) - Revert retry-after behavior to be exponential backoff (#349) diff --git a/src/databricks/sql/auth/retry.py b/src/databricks/sql/auth/retry.py index bbdd81c0..eee6f839 100644 --- a/src/databricks/sql/auth/retry.py +++ b/src/databricks/sql/auth/retry.py @@ -325,6 +325,7 @@ def should_retry(self, method: str, status_code: int) -> Tuple[bool, str]: default, this means ExecuteStatement is only retried for codes 429 and 503. This limit prevents automatically retrying non-idempotent commands that could be destructive. + 5. The request received a 403 response, because this can never succeed. Q: What about OSErrors and Redirects? @@ -338,6 +339,11 @@ def should_retry(self, method: str, status_code: int) -> Tuple[bool, str]: if status_code == 200: return False, "200 codes are not retried" + if status_code == 403: + raise NonRecoverableNetworkError( + "Received 403 - FORBIDDEN. Confirm your authentication credentials." + ) + # Request failed and server said NotImplemented. This isn't recoverable. Don't retry. if status_code == 501: raise NonRecoverableNetworkError("Received code 501 from server.") diff --git a/tests/e2e/common/retry_test_mixins.py b/tests/e2e/common/retry_test_mixins.py index 9b8efd1a..9a49870e 100644 --- a/tests/e2e/common/retry_test_mixins.py +++ b/tests/e2e/common/retry_test_mixins.py @@ -407,3 +407,17 @@ def test_retry_max_redirects_exceeds_max_attempts_count_warns_user(self, caplog) def test_retry_legacy_behavior_warns_user(self, caplog): with self.connection(extra_params={**self._retry_policy, "_enable_v3_retries": False}): assert "Legacy retry behavior is enabled for this connection." in caplog.text + + + def test_403_not_retried(self): + """GIVEN the server returns a code 403 + WHEN the connector receives this response + THEN nothing is retried and an exception is raised + """ + + # Code 403 is a Forbidden error + with mocked_server_response(status=403): + with pytest.raises(RequestError) as cm: + with self.connection(extra_params=self._retry_policy) as conn: + pass + assert isinstance(cm.value.args[1], NonRecoverableNetworkError) \ No newline at end of file