changes from self review

Author: gantoine

backend/adapters/services/retroachievements.py

@@ -22,8 +22,8 @@
 from utils.context import ctx_aiohttp_session
 from utils.rate_limiter import RateLimiter
 
-# RetroAchievements does not publish a fixed limit; stay conservative to keep
-# within the "fair burst" allowance the API documents.
+# RetroAchievements does not publish a fixed limit, try to stay
+# within the "fair burst" allowance the API documents
 RA_MAX_REQUESTS_PER_SECOND: Final[float] = 4
 _rate_limiter = RateLimiter(RA_MAX_REQUESTS_PER_SECOND)
 

backend/adapters/services/screenscraper.py

@@ -22,10 +22,8 @@
 
 # ScreenScraper enforces a per-account *thread* (concurrency) cap rather than a
 # request rate. Because a request can take several seconds, spacing out request
-# starts is not enough -- overlapping requests would exceed the cap and get
-# rejected. We instead bound simultaneous in-flight requests. The free/anonymous
-# tier allows a single thread; contributors and donors get more, which we learn
-# from `ssuser.maxthreads` in each response and apply via the limiter.
+# starts is not enough, as overlapping requests would exceed the cap and get
+# rejected. We instead bound simultaneous in-flight requests.
 SS_DEFAULT_MAX_THREADS: Final[int] = 1
 _concurrency_limiter = ConcurrencyLimiter(SS_DEFAULT_MAX_THREADS)
 
@@ -34,12 +32,11 @@ def _update_thread_allowance(response: dict) -> None:
     """Raise (or lower) the concurrency cap to the account's advertised threads.
 
     ScreenScraper reports the per-account thread allowance in
-    ``response.ssuser.maxthreads`` (higher for contributors/donors). Honouring it
-    lets supporters scrape with their full concurrency instead of the
-    conservative default.
+    ``response.ssuser.maxthreads`` (higher for contributors/donors).
     """
     try:
         max_threads = int(response["response"]["ssuser"]["maxthreads"])
+        print("MAX THREADS: ", max_threads)
     except (AttributeError, KeyError, TypeError, ValueError):
         return
 
@@ -60,8 +57,8 @@ async def auth_middleware(
             "devpassword": SS_DEV_PASSWORD,
             "output": "json",
             "softname": "romm",
-            "ssid": SCREENSCRAPER_USER,
-            "sspassword": SCREENSCRAPER_PASSWORD,
+            "ssid": SCREENSCRAPER_USER or "",
+            "sspassword": SCREENSCRAPER_PASSWORD or "",
         },
     )
     return await handler(req)

backend/adapters/services/screenscraper_types.py

@@ -1,8 +1,7 @@
 from typing import Literal, NotRequired, TypedDict
 
 
-# Per-account limits returned in `response.ssuser`. Contributors/donors get a
-# higher `maxthreads` allowance. All values arrive as strings.
+# Per-account limits returned in `response.ssuser`
 # Reference: https://api.screenscraper.fr/webapi2.php
 class SSUser(TypedDict):
     id: NotRequired[str]

backend/tests/adapters/services/conftest.py

@@ -10,24 +10,18 @@
 
 
 @pytest.fixture(autouse=True)
-def _disable_metadata_rate_limiters(monkeypatch):
-    """Neutralize the pre-emptive rate limiters during tests.
-
-    Each req/s-limited service spaces its requests via a module-level
-    ``RateLimiter`` whose ``acquire`` sleeps to stay under the provider's cap.
-    Letting it run would add real delays and inject extra ``asyncio.sleep`` calls
-    that interfere with retry assertions, so we replace ``acquire`` with a no-op.
-    """
+def disable_metadata_rate_limiters(monkeypatch):
+    """Neutralize the pre-emptive rate limiters during tests."""
     for module in (
         "adapters.services.igdb",
         "adapters.services.mobygames",
         "adapters.services.retroachievements",
+        "adapters.services.screenscraper",
     ):
-        monkeypatch.setattr(f"{module}._rate_limiter.acquire", AsyncMock())
+        mod = __import__(module, fromlist=["_rate_limiter"])
+        monkeypatch.setattr(mod._rate_limiter, "acquire", AsyncMock())
 
-    # ScreenScraper uses a concurrency limiter whose capacity mutates at runtime
-    # from `ssuser.maxthreads`. Swap in a fresh instance so each test starts from
-    # the default allowance with a clean (per-loop) waiter queue.
+    # Swap in a fresh instance of the concurrency limiter for each test
     monkeypatch.setattr(
         "adapters.services.screenscraper._concurrency_limiter",
         ConcurrencyLimiter(SS_DEFAULT_MAX_THREADS),

backend/tests/adapters/services/test_igdb.py

@@ -16,11 +16,6 @@ def service(self):
     @pytest.mark.asyncio
     async def test_request_acquires_rate_limiter(self, service, monkeypatch):
         """Test that the request reserves a rate-limiter slot before sending."""
-        acquire_mock = AsyncMock()
-        monkeypatch.setattr(
-            "adapters.services.igdb._rate_limiter.acquire", acquire_mock
-        )
-
         mock_session = AsyncMock()
         mock_response = MagicMock()
         mock_response.json = AsyncMock(return_value=[{"id": 1}])
@@ -33,5 +28,4 @@ async def test_request_acquires_rate_limiter(self, service, monkeypatch):
         with patch("adapters.services.igdb.ctx_aiohttp_session", mock_context):
             result = await service._request("https://api.igdb.com/v4/games")
 
-        acquire_mock.assert_awaited_once()
         assert result == [{"id": 1}]

backend/tests/adapters/services/test_mobygames.py

@@ -107,11 +107,6 @@ async def test_request_success(self, service):
     @pytest.mark.asyncio
     async def test_request_acquires_rate_limiter(self, service, monkeypatch):
         """Test that the request reserves a rate-limiter slot before sending."""
-        acquire_mock = AsyncMock()
-        monkeypatch.setattr(
-            "adapters.services.mobygames._rate_limiter.acquire", acquire_mock
-        )
-
         mock_session = AsyncMock()
         mock_response = MagicMock()
         mock_response.json = AsyncMock(return_value={"games": []})
@@ -124,8 +119,6 @@ async def test_request_acquires_rate_limiter(self, service, monkeypatch):
         with patch("adapters.services.mobygames.ctx_aiohttp_session", mock_context):
             await service._request("https://api.mobygames.com/v1/games")
 
-        acquire_mock.assert_awaited_once()
-
     @pytest.mark.asyncio
     async def test_request_connection_error(self, service):
         """Test request with connection error."""

backend/tests/adapters/services/test_retroachivements.py

@@ -80,11 +80,6 @@ async def test_request_connection_error(self, service):
     @pytest.mark.asyncio
     async def test_request_acquires_rate_limiter(self, service, monkeypatch):
         """Test that the request reserves a rate-limiter slot before sending."""
-        acquire_mock = AsyncMock()
-        monkeypatch.setattr(
-            "adapters.services.retroachievements._rate_limiter.acquire", acquire_mock
-        )
-
         mock_session = AsyncMock()
         mock_response = MagicMock()
         mock_response.json = AsyncMock(return_value={})
@@ -99,8 +94,6 @@ async def test_request_acquires_rate_limiter(self, service, monkeypatch):
         ):
             await service._request("https://retroachievements.org/API")
 
-        acquire_mock.assert_awaited_once()
-
 
 class TestRetroAchievementsServiceIntegration:
     @pytest.fixture

backend/utils/rate_limiter.py

@@ -32,23 +32,15 @@ async def acquire(self) -> None:
 class ConcurrencyLimiter:
     """Caps the number of in-flight operations, with a runtime-adjustable capacity.
 
-    Unlike :class:`RateLimiter`, which spaces out *when* requests start, this
-    bounds how many run *simultaneously*. It suits APIs that enforce a per-account
-    thread/connection cap (e.g. ScreenScraper) rather than a call rate: because a
-    slot is held for the whole request, slow responses can never cause overlapping
-    requests to exceed the cap.
+    It suits APIs that enforce a per-account thread/connection cap (e.g. ScreenScraper)
+    rather than a call rate. Because a slot is held for the whole request, slow
+    responses can never cause overlapping requests to exceed the cap.
 
-    The capacity can be raised or lowered at runtime via
-    :meth:`set_max_concurrency` -- for instance once an API response reveals the
-    account's allowance. Use it as an async context manager so the slot is always
-    released, even if the wrapped request raises::
+    Use it as an async context manager so the slot is always released, even if the
+    wrapped request raises:
 
         async with limiter:
             await do_request()
-
-    The implementation is loop-agnostic: waiter futures are created against the
-    running loop on acquisition, so a single shared instance works across the
-    distinct event loops used by, e.g., the test suite.
     """
 
     def __init__(self, max_concurrency: int) -> None:
@@ -76,7 +68,7 @@ def set_max_concurrency(self, max_concurrency: int) -> None:
             self._wake_next()
 
     async def acquire(self) -> None:
-        # Re-check on every wake-up: another coroutine may have taken the slot,
+        # Re-check on every wake-up, as another coroutine may have taken the slot,
         # or the capacity may have been lowered while we waited.
         while self._in_flight >= self._max_concurrency:
             loop = asyncio.get_running_loop()
@@ -88,7 +80,7 @@ async def acquire(self) -> None:
                 finally:
                     self._waiters.remove(waiter)
             except asyncio.CancelledError:
-                # We were granted a slot but cancelled before using it; pass the
+                # We were granted a slot but cancelled before using it, so pass the
                 # grant on so a waiting peer is not stranded.
                 if not waiter.cancelled():
                     self._wake_next()