Only Retry Certain Errors (#1330)

* only retry certain errors (not all)

* write changelog

* Do not retry after last attempt,  improve test comments

Author: Tobias314

webknossos/Changelog.md

@@ -17,6 +17,7 @@ For upgrade instructions, please check the respective _Breaking Changes_ section
 ### Added
 
 ### Changed
+- Limit custom retries (e.g. additional to tensorstore internal retries) to certain network errors ("Too Many Requests", "GatewayTimeout"). [#1330](https://github.com/scalableminds/webknossos-libs/pull/1330)
 
 ### Fixed
 

webknossos/tests/test_utils.py

@@ -0,0 +1,70 @@
+from unittest.mock import Mock, patch
+
+import pytest
+
+from webknossos.utils import call_with_retries
+
+
+def test_call_with_retries_success() -> None:
+    """Test that a successful function call returns immediately."""
+    mock_fn = Mock(return_value="success")
+
+    result = call_with_retries(mock_fn)
+
+    assert result == "success"
+    mock_fn.call_count == 1  # Function called only once (direct success)
+
+
+@patch("time.sleep")
+def test_call_with_retries_sucess_with_retry(mock_sleep: Mock) -> None:
+    """Test retry behavior when function succeeds after retryable failures."""
+    mock_fn = Mock(
+        side_effect=[
+            Exception("Too Many Requests"),
+            Exception("GatewayTimeout"),
+            "success",
+        ]
+    )
+
+    result = call_with_retries(mock_fn, num_retries=3, backoff_factor=2.0)
+
+    assert result == "success"
+    assert mock_fn.call_count == 3  # Called once for each try
+    assert mock_sleep.call_count == 2  # Sleep called twice for retries
+
+
+@patch("time.sleep")
+def test_call_with_retries_failure_after_retry(mock_sleep: Mock) -> None:
+    """Test retry behavior when function succeeds after retryable failures."""
+    mock_fn = Mock(
+        side_effect=[
+            Exception("Too Many Requests"),
+            Exception("GatewayTimeout"),
+            "success",
+        ]
+    )
+
+    with pytest.raises(Exception):
+        call_with_retries(mock_fn, num_retries=2, backoff_factor=2.0)
+
+    assert mock_fn.call_count == 2
+    assert (
+        mock_sleep.call_count == 1
+    )  # Sleep called once after the first failure but not after the second/last failure
+
+
+@patch("time.sleep")
+def test_call_with_retries_direct_failure(mock_sleep: Mock) -> None:
+    """Test retry behavior when function succeeds after retryable failures."""
+    mock_fn = Mock(
+        side_effect=[
+            RuntimeError("Non Retryable Runtime Error"),
+            "success",
+        ]
+    )
+
+    with pytest.raises(RuntimeError):
+        call_with_retries(mock_fn, num_retries=5, backoff_factor=2.0)
+
+    assert mock_fn.call_count == 1  # Only called once, no retries
+    assert mock_sleep.call_count == 0  # Sleep not called since it failed immediately

webknossos/webknossos/utils.py

@@ -41,29 +41,41 @@
 ReturnType = TypeVar("ReturnType")
 
 
+def _is_exception_retryable(exception: Exception) -> bool:
+    exception_str_lower = str(exception).lower()
+    if "too many requests" in exception_str_lower or "gateway" in exception_str_lower:
+        return True
+    return False
+
+
 def call_with_retries(
     fn: Callable[[], ReturnType],
     num_retries: int = DEFAULT_NUM_RETRIES,
     description: str = "",
     backoff_factor: float = DEFAULT_BACKOFF_FACTOR,
 ) -> ReturnType:
-    """Call a function, retrying up to `num_retries` times on an exception during the call. Useful for retrying requests or network io."""
+    """Call a function, retrying up to `num_retries` times on common retryable (network) exceptions. Useful for retrying requests or network io."""
     last_exception = None
-    for i in range(num_retries):
+    for current_retry_number in range(num_retries):
         try:
             return fn()
         except Exception as e:  # noqa: PERF203 # allow try except in loop
-            logger.warning(
-                f"{description} attempt {i + 1}/{num_retries} failed, retrying..."
-                f"Error was: {e}"
-            )
-            # We introduce some randomness to avoid multiple processes retrying at the same time
-            random_factor = np.random.uniform(0.66, 1.5)
-            time.sleep((backoff_factor**i) * random_factor)
             last_exception = e
+            # We only sleep and retry if it was not the last attempt and the exception is retryable and.
+            if current_retry_number < num_retries - 1 and _is_exception_retryable(e):
+                logger.warning(
+                    f"{description} attempt {current_retry_number + 1}/{num_retries} failed, retrying..."
+                    f"Error was: {e}"
+                )
+                # We introduce some randomness to avoid multiple processes retrying at the same time
+                random_factor = np.random.uniform(0.66, 1.5)
+                time.sleep((backoff_factor**current_retry_number) * random_factor)
+            else:
+                break
     # If the last attempt fails, we log the error and raise it.
     # This is important to avoid silent failures.
-    logger.error(f"{description} failed after {num_retries} attempts.")
+    if current_retry_number > 0:
+        logger.error(f"{description} failed after {current_retry_number + 1} attempts.")
     assert last_exception is not None, "last_exception should never be None here"
     raise last_exception