Prevent unsafe retries for ambiguous writes

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",

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(

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."""

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):

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")