improve coverage

Author: sanderegg

packages/service-library/src/servicelib/redis/_semaphore.py

@@ -16,9 +16,9 @@
     computed_field,
     field_validator,
 )
-from servicelib.logging_errors import create_troubleshootting_log_kwargs
 
 from ..background_task import periodic
+from ..logging_errors import create_troubleshootting_log_kwargs
 from ..logging_utils import log_catch
 from ._client import RedisClientSDK
 from ._constants import (
@@ -32,7 +32,7 @@
 _logger = logging.getLogger(__name__)
 
 
-async def renew_semaphore_entry(semaphore: "DistributedSemaphore") -> None:
+async def _renew_semaphore_entry(semaphore: "DistributedSemaphore") -> None:
     """
     Manually renew a semaphore entry by updating its timestamp and TTL.
 
@@ -44,8 +44,6 @@ async def renew_semaphore_entry(semaphore: "DistributedSemaphore") -> None:
     Raises:
         Exception: If the renewal operation fails
     """
-    if not semaphore.is_acquired():
-        return  # Nothing to renew if not acquired
 
     current_time = asyncio.get_event_loop().time()
     ttl_seconds = semaphore.ttl.total_seconds()
@@ -99,7 +97,7 @@ class DistributedSemaphore(BaseModel):
     blocking: Annotated[
         bool, Field(description="Whether acquire() should block until available")
     ] = True
-    timeout: Annotated[
+    blocking_timeout: Annotated[
         datetime.timedelta | None,
         Field(description="Maximum time to wait when blocking"),
     ] = DEFAULT_SOCKET_TIMEOUT
@@ -161,7 +159,9 @@ async def acquire(self) -> bool:
             return True
 
         start_time = asyncio.get_event_loop().time()
-        timeout_seconds = self.timeout.total_seconds() if self.timeout else None
+        timeout_seconds = (
+            self.blocking_timeout.total_seconds() if self.blocking_timeout else None
+        )
 
         while True:
             # Try to acquire using Redis sorted set for atomic operations
@@ -361,7 +361,7 @@ async def _wrapper(*args: P.args, **kwargs: P.kwargs) -> R:
                 capacity=semaphore_capacity,
                 ttl=ttl,
                 blocking=blocking,
-                timeout=blocking_timeout,
+                blocking_timeout=blocking_timeout,
             )
 
             # Acquire the semaphore
@@ -379,7 +379,7 @@ async def _wrapper(*args: P.args, **kwargs: P.kwargs) -> R:
                     # Create auto-renewal task
                     @periodic(interval=ttl / 3, raise_on_error=True)
                     async def _periodic_renewer() -> None:
-                        await renew_semaphore_entry(semaphore)
+                        await _renew_semaphore_entry(semaphore)
                         started_event.set()
 
                     # Start the renewal task

packages/service-library/tests/redis/test_semaphore.py

@@ -81,7 +81,7 @@ async def test_semaphore_initialization(
     )
 
 
-async def test_semaphore_invalid_capacity_raises(
+async def test_invalid_semaphore_initialization(
     redis_client_sdk: RedisClientSDK,
     semaphore_name: str,
 ):
@@ -95,6 +95,23 @@ async def test_semaphore_invalid_capacity_raises(
             redis_client=redis_client_sdk, key=semaphore_name, capacity=-1
         )
 
+    with pytest.raises(ValueError, match="TTL must be positive"):
+        DistributedSemaphore(
+            redis_client=redis_client_sdk,
+            key=semaphore_name,
+            capacity=1,
+            ttl=datetime.timedelta(seconds=0),
+        )
+    with pytest.raises(ValueError, match="Timeout must be positive"):
+        DistributedSemaphore(
+            redis_client=redis_client_sdk,
+            key=semaphore_name,
+            capacity=1,
+            ttl=datetime.timedelta(seconds=10),
+            blocking=True,
+            blocking_timeout=datetime.timedelta(seconds=0),
+        )
+
 
 async def test_semaphore_acquire_release_single(
     redis_client_sdk: RedisClientSDK,
@@ -216,7 +233,7 @@ async def test_semaphore_blocking_timeout(
             redis_client=redis_client_sdk,
             key=semaphore_name,
             capacity=capacity,
-            timeout=timeout,
+            blocking_timeout=timeout,
         )
 
         with pytest.raises(
@@ -340,7 +357,7 @@ async def test_decorator_auto_renewal_failure_propagation(
     monkeypatch: pytest.MonkeyPatch,
 ):
     """Test that auto-renewal failures properly propagate as exceptions in the decorator"""
-    from servicelib.redis._semaphore import renew_semaphore_entry
+    from servicelib.redis._semaphore import _renew_semaphore_entry
 
     class RenewalFailureError(Exception):
         """Custom exception for testing renewal failures"""
@@ -349,7 +366,7 @@ class RenewalFailureError(Exception):
 
     # Mock the renewal function to fail after first call
     call_count = 0
-    original_renew = renew_semaphore_entry
+    original_renew = _renew_semaphore_entry
 
     async def failing_renew_semaphore_entry(semaphore):
         nonlocal call_count
@@ -362,7 +379,7 @@ async def failing_renew_semaphore_entry(semaphore):
             raise RenewalFailureError("Simulated renewal failure")
 
     monkeypatch.setattr(
-        "servicelib.redis._semaphore.renew_semaphore_entry",
+        "servicelib.redis._semaphore._renew_semaphore_entry",
         failing_renew_semaphore_entry,
     )
 
@@ -646,16 +663,16 @@ class UserFunctionError(Exception):
 
     # Track that auto-renewal is actually happening
     original_renew = __import__(
-        "servicelib.redis._semaphore", fromlist=["renew_semaphore_entry"]
-    ).renew_semaphore_entry
+        "servicelib.redis._semaphore", fromlist=["_renew_semaphore_entry"]
+    )._renew_semaphore_entry
 
     async def tracking_renew_semaphore_entry(semaphore):
         nonlocal renewal_count
         renewal_count += 1
         await original_renew(semaphore)
 
     with mock.patch(
-        "servicelib.redis._semaphore.renew_semaphore_entry",
+        "servicelib.redis._semaphore._renew_semaphore_entry",
         side_effect=tracking_renew_semaphore_entry,
     ):