fix(api): address second-round CodeRabbit findings

Aureliolo · Aureliolo · commit 5aa49ced9037 · 2026-03-16T20:54:38.000+01:00
- Use None-check instead of falsy-or for auth.exclude_paths (preserves
  explicitly empty tuples)
- Use math.ceil for expires_in to prevent floor-to-zero on sub-second TTLs
- Log ticket limit exceeded before raising ConflictError
- Guard against non-dict payload in WS event filter matching
- Split _handle_message into _parse_ws_message + _validate_ws_fields +
  dispatch (all functions now &lt;50 lines)
- Add @pytest.mark.timeout(30) to all new test classes
- Add concurrent consumer test for single-use ticket guarantee
diff --git a/src/synthorg/api/app.py b/src/synthorg/api/app.py
@@ -649,12 +649,16 @@ def _build_middleware(api_config: ApiConfig) -> list[Middleware]:
     auth = api_config.auth
     prefix = api_config.api_prefix
     ws_path = f"^{prefix}/ws$"
-    exclude_paths = auth.exclude_paths or (
-        f"^{prefix}/health$",
-        "^/docs",
-        "^/api$",
-        f"^{prefix}/auth/setup$",
-        f"^{prefix}/auth/login$",
+    exclude_paths = (
+        auth.exclude_paths
+        if auth.exclude_paths is not None
+        else (
+            f"^{prefix}/health$",
+            "^/docs",
+            "^/api$",
+            f"^{prefix}/auth/setup$",
+            f"^{prefix}/auth/login$",
+        )
     )
     # Always ensure the WS upgrade path is excluded — the WS handler
     # performs its own ticket-based auth, so the JWT middleware must
diff --git a/src/synthorg/api/auth/controller.py b/src/synthorg/api/auth/controller.py
@@ -1,5 +1,6 @@
 """Authentication controller — setup, login, password change, me, ws-ticket."""
 
+import math
 import uuid
 from datetime import UTC, datetime
 from typing import Any, Self
@@ -459,14 +460,19 @@ async def ws_ticket(
         try:
             ticket = app_state.ticket_store.create(ws_user)
         except TicketLimitExceededError:
+            logger.warning(
+                API_AUTH_FAILED,
+                reason="ws_ticket_limit_exceeded",
+                user_id=auth_user.user_id,
+            )
             msg = "Too many pending tickets — wait for existing tickets to expire"
             raise ConflictError(msg)  # noqa: B904
 
         return Response(
             content=ApiResponse(
                 data=WsTicketResponse(
                     ticket=ticket,
-                    expires_in=int(app_state.ticket_store.ttl_seconds),
+                    expires_in=max(1, math.ceil(app_state.ticket_store.ttl_seconds)),
                 ),
             ),
         )
diff --git a/src/synthorg/api/controllers/ws.py b/src/synthorg/api/controllers/ws.py
@@ -153,6 +153,8 @@ async def _on_event(
     channel_filters = filters.get(channel)
     if channel_filters:
         payload = event.get("payload", {})
+        if not isinstance(payload, dict):
+            return
         if not all(payload.get(k) == v for k, v in channel_filters.items()):
             return
 
@@ -236,21 +238,10 @@ async def _receive_loop(
         raise
 
 
-def _handle_message(  # noqa: PLR0911
+def _parse_ws_message(
     data: str,
-    subscribed: set[str],
-    filters: dict[str, dict[str, str]],
-) -> str:
-    """Parse and dispatch a single client message.
-
-    Args:
-        data: Raw JSON string from the client.
-        subscribed: Mutable set of subscribed channel names.
-        filters: Mutable per-channel payload filters.
-
-    Returns:
-        JSON acknowledgement or error string.
-    """
+) -> dict[str, Any] | str:
+    """Parse raw JSON from the client, returning a dict or an error string."""
     if len(data.encode()) > _MAX_WS_MESSAGE_BYTES:
         return json.dumps({"error": "Message too large"})
 
@@ -271,7 +262,18 @@ def _handle_message(  # noqa: PLR0911
     if not isinstance(msg, dict):
         return json.dumps({"error": "Expected JSON object"})
 
-    action = msg.get("action")
+    return msg
+
+
+def _validate_ws_fields(
+    msg: dict[str, Any],
+) -> tuple[str, list[str], dict[str, Any]] | str:
+    """Extract and validate action, channels, and filters from a parsed message.
+
+    Returns ``(action, channels, client_filters)`` on success, or a
+    JSON error string on validation failure.
+    """
+    action = str(msg.get("action", ""))
     channels = msg.get("channels", [])
     client_filters = msg.get("filters", {})
 
@@ -280,6 +282,25 @@ def _handle_message(  # noqa: PLR0911
     if not isinstance(client_filters, dict):
         return json.dumps({"error": "filters must be an object"})
 
+    return (action, channels, client_filters)
+
+
+def _handle_message(
+    data: str,
+    subscribed: set[str],
+    filters: dict[str, dict[str, str]],
+) -> str:
+    """Parse, validate, and dispatch a single client message."""
+    parsed = _parse_ws_message(data)
+    if isinstance(parsed, str):
+        return parsed
+
+    fields = _validate_ws_fields(parsed)
+    if isinstance(fields, str):
+        return fields
+
+    action, channels, client_filters = fields
+
     if action == "subscribe":
         return _handle_subscribe(channels, client_filters, subscribed, filters)
 
diff --git a/tests/unit/api/auth/test_controller.py b/tests/unit/api/auth/test_controller.py
@@ -327,6 +327,7 @@ def test_rejects_unknown_user_type(self) -> None:
             require_password_changed(connection, None)
 
 
+@pytest.mark.timeout(30)
 @pytest.mark.unit
 class TestWsTicket:
     def test_ws_ticket_returns_ticket_and_expires_in(
diff --git a/tests/unit/api/auth/test_ticket_store.py b/tests/unit/api/auth/test_ticket_store.py
@@ -25,6 +25,7 @@ def _make_user(
     )
 
 
+@pytest.mark.timeout(30)
 @pytest.mark.unit
 class TestWsTicketStoreCreate:
     """Tests for ticket creation."""
@@ -90,6 +91,7 @@ def test_per_user_ticket_cap_different_users(self) -> None:
         assert isinstance(ticket, str)
 
 
+@pytest.mark.timeout(30)
 @pytest.mark.unit
 class TestWsTicketStoreValidateAndConsume:
     """Tests for ticket validation and consumption."""
@@ -118,6 +120,28 @@ def test_validate_and_consume_single_use(self) -> None:
         assert first is not None
         assert second is None
 
+    def test_validate_and_consume_single_use_concurrent(self) -> None:
+        """Exactly one concurrent consumer wins the ticket."""
+        import threading
+        from concurrent.futures import ThreadPoolExecutor
+
+        store = WsTicketStore()
+        user = _make_user()
+        ticket = store.create(user)
+
+        barrier = threading.Barrier(10)
+
+        def consume() -> AuthenticatedUser | None:
+            barrier.wait()
+            return store.validate_and_consume(ticket)
+
+        with ThreadPoolExecutor(max_workers=10) as pool:
+            results = list(pool.map(lambda _: consume(), range(10)))
+
+        winners = [r for r in results if r is not None]
+        assert len(winners) == 1
+        assert winners[0].user_id == user.user_id
+
     def test_validate_and_consume_expired(self) -> None:
         store = WsTicketStore(ttl_seconds=10.0)
         user = _make_user()
@@ -203,6 +227,7 @@ def test_custom_ttl_expired(self) -> None:
         assert result is None
 
 
+@pytest.mark.timeout(30)
 @pytest.mark.unit
 class TestWsTicketStoreCleanup:
     """Tests for expired ticket cleanup."""
diff --git a/tests/unit/api/controllers/test_ws.py b/tests/unit/api/controllers/test_ws.py
@@ -189,6 +189,7 @@ def test_non_dict_json_returns_error(self, value: object) -> None:
         assert data["error"] == "Expected JSON object"
 
 
+@pytest.mark.timeout(30)
 @pytest.mark.unit
 class TestWsTicketAuth:
     """Tests for ticket-based WebSocket authentication logic.
