diff --git a/src/sapsf_shared/__init__.py b/src/sapsf_shared/__init__.py index 14d8301..5c7f2b3 100644 --- a/src/sapsf_shared/__init__.py +++ b/src/sapsf_shared/__init__.py @@ -13,7 +13,7 @@ ) from sapsf_shared.client import SFClient from sapsf_shared.config import SFEnvConfig, load_config, load_yaml -from sapsf_shared.exceptions import SFClientError, SFConfigError, SFError +from sapsf_shared.exceptions import AmbiguousWriteError, SFClientError, SFConfigError, SFError from sapsf_shared.logging_config import CredentialRedactionFilter, setup_logging from sapsf_shared.permissions import ( PermissionAnalyzer, @@ -33,6 +33,7 @@ __all__ = [ "audit", "audit_log", + "AmbiguousWriteError", "AuthConfig", "AuthError", "BasicAuth", diff --git a/src/sapsf_shared/client.py b/src/sapsf_shared/client.py index 7e63dbc..7531e03 100644 --- a/src/sapsf_shared/client.py +++ b/src/sapsf_shared/client.py @@ -21,7 +21,7 @@ from requests.adapters import HTTPAdapter from sapsf_shared.auth import AuthConfig, build_requests_auth -from sapsf_shared.exceptions import SFClientError +from sapsf_shared.exceptions import AmbiguousWriteError, SFClientError from sapsf_shared.utils import odata_escape logger = logging.getLogger(__name__) @@ -30,6 +30,7 @@ RETRY_STATUS_CODES = frozenset({429, 500, 502, 503, 504}) MAX_RETRIES = 3 BACKOFF_SECONDS = (1, 2, 4) +RETRYABLE_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) class SFClient: @@ -95,13 +96,29 @@ def _request_with_retry( url: str, **kwargs: Any, ) -> requests.Response: - """Execute an HTTP request with retry logic on transient errors.""" + """Execute a request, retrying transient failures only for safe methods. + + A failed write response is ambiguous: the server may have committed the + mutation before the response was lost. Replaying it here could create a + duplicate, so callers must reconcile target state instead. + """ + method = method.upper() + may_retry = method in RETRYABLE_METHODS last_exc: Exception | None = None for attempt in range(MAX_RETRIES): try: resp = self._session.request(method, url, timeout=self.config.timeout_sec, **kwargs) if resp.status_code not in RETRY_STATUS_CODES: return resp + if not may_retry: + raise AmbiguousWriteError( + f"{method} outcome is unknown after HTTP {resp.status_code}; " + "reconcile target state before retrying", + method=method, + status_code=resp.status_code, + body=resp.text[:2000], + url=url, + ) wait = BACKOFF_SECONDS[min(attempt, len(BACKOFF_SECONDS) - 1)] logger.warning( "HTTP %s from %s (attempt %d/%d) - retrying in %ds", @@ -111,9 +128,19 @@ def _request_with_retry( MAX_RETRIES, wait, ) - time.sleep(wait) last_exc = None + if attempt < MAX_RETRIES - 1: + time.sleep(wait) + except AmbiguousWriteError: + raise except requests.exceptions.RequestException as exc: + if not may_retry: + raise AmbiguousWriteError( + f"{method} outcome is unknown after a network error; " + "reconcile target state before retrying", + method=method, + url=url, + ) from exc wait = BACKOFF_SECONDS[min(attempt, len(BACKOFF_SECONDS) - 1)] logger.warning( "Request error on %s (attempt %d/%d): %s - retrying in %ds", @@ -124,7 +151,8 @@ def _request_with_retry( wait, ) last_exc = exc - time.sleep(wait) + if attempt < MAX_RETRIES - 1: + time.sleep(wait) if last_exc: raise SFClientError( diff --git a/src/sapsf_shared/exceptions.py b/src/sapsf_shared/exceptions.py index 08cc678..06de334 100644 --- a/src/sapsf_shared/exceptions.py +++ b/src/sapsf_shared/exceptions.py @@ -33,5 +33,26 @@ def __init__( self.url = url +class AmbiguousWriteError(SFClientError): + """Raised when a write may have succeeded but no definitive response was received. + + Retrying such a request automatically could duplicate a create or apply a + mutation twice. Callers should reconcile the target state before deciding + whether to retry. + """ + + def __init__( + self, + message: str, + *, + method: str, + status_code: int | None = None, + body: str = "", + url: str | None = None, + ) -> None: + super().__init__(message, status_code=status_code, body=body, url=url) + self.method = method + + class AuthError(SFError): """Raised when authentication cannot be established.""" diff --git a/tests/test_client.py b/tests/test_client.py index d97459c..bd0e81d 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -4,10 +4,11 @@ from unittest.mock import MagicMock, patch import pytest +import requests from sapsf_shared.auth import AuthConfig from sapsf_shared.client import SFClient -from sapsf_shared.exceptions import SFClientError +from sapsf_shared.exceptions import AmbiguousWriteError, SFClientError @pytest.fixture @@ -83,6 +84,35 @@ def test_all_retries_exhausted(self, mock_request, auth_config): assert resp.status_code == 503 assert mock_request.call_count == 3 + @patch("sapsf_shared.client.time.sleep") + @patch("sapsf_shared.client.requests.Session.request") + def test_post_network_error_is_not_retried(self, mock_request, mock_sleep, auth_config): + mock_request.side_effect = requests.exceptions.Timeout("response lost") + + client = SFClient(auth_config) + with pytest.raises(AmbiguousWriteError) as exc: + client.post("Users", {"userId": "2"}) + + assert exc.value.method == "POST" + assert "reconcile target state" in str(exc.value) + mock_request.assert_called_once() + mock_sleep.assert_not_called() + + @patch("sapsf_shared.client.time.sleep") + @patch("sapsf_shared.client.requests.Session.request") + def test_post_retryable_status_is_not_retried(self, mock_request, mock_sleep, auth_config): + mock_request.return_value.status_code = 503 + mock_request.return_value.text = "Service unavailable" + + client = SFClient(auth_config) + with pytest.raises(AmbiguousWriteError) as exc: + client.post("Users", {"userId": "2"}) + + assert exc.value.status_code == 503 + assert exc.value.body == "Service unavailable" + mock_request.assert_called_once() + mock_sleep.assert_not_called() + class TestCheckResponse: def test_401_raises(self, auth_config): diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index bd28a39..e71ef32 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -1,6 +1,7 @@ """Tests for sapsf_shared.exceptions.""" from sapsf_shared.exceptions import ( + AmbiguousWriteError, AuthError, SFClientError, SFConfigError, @@ -23,6 +24,7 @@ def test_inheritance(self): assert issubclass(SFConfigError, SFError) assert issubclass(AuthError, SFError) assert issubclass(SFClientError, SFError) + assert issubclass(AmbiguousWriteError, SFClientError) class TestSFClientError: @@ -40,6 +42,20 @@ def test_body(self): assert exc.body == "Internal server error" +class TestAmbiguousWriteError: + def test_method_and_client_error_fields(self): + exc = AmbiguousWriteError( + "Outcome unknown", + method="POST", + status_code=503, + body="Unavailable", + url="https://api.example.com/odata/v2/Users", + ) + assert exc.method == "POST" + assert exc.status_code == 503 + assert exc.body == "Unavailable" + + class TestSFConfigError: def test_message(self): exc = SFConfigError("Missing base_url")