Fix/academy review findings

artifacts/ai-testing-academy/src/lib/locales/en.ts

@@ -236,7 +236,7 @@ export const en = {
     errBlockedOpenUrl: '',
     errApiPrefix: 'API error (',
     errProxyBusy:
-      'The free AI allowance is used up for now. Wait a little, or connect your own provider key in Settings to keep going.',
+      'The shared AI service is busy, or its free allowance is spent. Wait a moment and try again, or connect your own provider key in Settings to keep going.',
     errProxyUnavailable:
       'The academy’s own AI key is unavailable right now. Connect your own provider key in Settings to keep going.',
     errNoJson: 'Could not parse JSON from the model response. Please try again.',

artifacts/ai-testing-academy/src/lib/locales/he.ts

@@ -232,7 +232,7 @@ export const he: Locale = {
     errBlockedOpenUrl: '',
     errApiPrefix: 'שגיאת API (',
     errProxyBusy:
-      'מכסת ה-AI החינמית נוצלה כרגע. המתינו מעט, או חברו מפתח ספק משלכם בהגדרות כדי להמשיך.',
+      'שירות ה-AI המשותף עמוס כרגע, או שהמכסה החינמית נוצלה. המתינו רגע ונסו שוב, או חברו מפתח ספק משלכם בהגדרות כדי להמשיך.',
     errProxyUnavailable:
       'מפתח ה-AI של האקדמיה אינו זמין כרגע. חברו מפתח ספק משלכם בהגדרות כדי להמשיך.',
     errNoJson: 'לא ניתן לנתח JSON מתגובת המודל. נסה שוב.',

replit.md

@@ -63,6 +63,7 @@ is absent fails closed with a controlled 4xx/5xx response. The package scripts d
 | `UPSTREAM_API_BASE_URL` | Secretless Replit API artifact relays `/api/*` to Fly | FastAPI handles routes locally |
 | `METRICS_TOKEN` | Protects production `/metrics` scrapes | Metrics are available only outside production |
 | `METRICS_ID_SALT` | HMAC-pseudonymizes user labels in metrics | Authenticated users are labeled `redacted` |
+| `TRUSTED_PROXY_HOPS` | How many rightmost `X-Forwarded-For` entries the platform appends, so the caller can be read past them | Defaults to 1. Too high trusts an entry the caller forged; too low keys everyone behind one proxy to the same quota |
 | `RATE_LIMIT_SALT` | Production quotas are counted in Postgres | **In production every rate-limited route refuses every caller with 429** — sign-in, the AI proxy and the admin seed route. `METRICS_ID_SALT` is accepted instead. `/api/readyz` names it in a `rateLimiting` field |
 
 ### API structure
@@ -89,6 +90,10 @@ Two consequences worth knowing before editing:
 
 - **Adding an AI provider** is a class in `ai_gateway.py` plus an entry in `PROVIDERS`.
   Dispatch, `/api/ai/config` and the routes all follow from the registry.
+- **An ordinary request falls through to the next provider** when the first answers 429 or
+  5xx — a provider saying "not me, not now". A 4xx below that is the request being wrong, so
+  it is not retried elsewhere. A grounded request has no fallback: search grounding is why it
+  chose that provider. The outcome names whoever answered, so metrics stay truthful.
 - **Tests substitute collaborators through `app.dependency_overrides`**, not by patching module
   globals. `server/tests/conftest.py` exposes `override_dependency` for this.
 

server/app/ai_gateway.py

@@ -35,12 +35,21 @@ class Completion:
     truncated: bool
 
 
+UNKNOWN = "unknown"
+
+
 @dataclass(frozen=True)
 class AiOutcome:
-    """A proxied result: the upstream status travels with the body deliberately."""
+    """A proxied result: the upstream status travels with the body deliberately.
+
+    `provider` and `model` name whoever actually answered, which after a
+    fallback is not the one the request started with.
+    """
 
     status: int
     payload: dict
+    provider: str = UNKNOWN
+    model: str = UNKNOWN
 
 
 class AiProvider(Protocol):
@@ -122,23 +131,29 @@ class GeminiProvider(_Provider):
     supports_grounding = True
 
     def build_request(self, model: str, key: str, body: GenerateBody) -> UpstreamRequest:
+        payload: dict = {
+            "system_instruction": {"parts": [{"text": body.system}]},
+            "contents": [
+                {
+                    "role": "model" if message.role == "assistant" else "user",
+                    "parts": [{"text": message.content}],
+                }
+                for message in body.messages
+            ],
+            "generationConfig": {"maxOutputTokens": body.maxTokens},
+        }
+        # Search grounding is what the caller asked for, not what this provider
+        # is. It was unconditional while Gemini only ever served grounded
+        # requests; as a fallback it also serves ordinary ones, and those must
+        # not quietly become searches.
+        if body.grounded:
+            payload["tools"] = [{"google_search": {}}]
         return UpstreamRequest(
             url=(
                 f"https://generativelanguage.googleapis.com/v1beta/models/{model}:generateContent"
             ),
             headers={"content-type": "application/json", "x-goog-api-key": key},
-            payload={
-                "system_instruction": {"parts": [{"text": body.system}]},
-                "contents": [
-                    {
-                        "role": "model" if message.role == "assistant" else "user",
-                        "parts": [{"text": message.content}],
-                    }
-                    for message in body.messages
-                ],
-                "generationConfig": {"maxOutputTokens": body.maxTokens},
-                "tools": [{"google_search": {}}],
-            },
+            payload=payload,
         )
 
     def read_completion(self, data: dict) -> Completion:
@@ -179,36 +194,82 @@ def advertised_config(self) -> dict[str, dict[str, object]]:
             for provider in self._providers
         }
 
+    def candidates(self, body: GenerateBody) -> tuple[_Provider, ...]:
+        """The provider for this request, then any other that could also serve it.
+
+        A grounded request has no alternative: search grounding is the reason it
+        chose that provider, and a provider without it would answer a different
+        question. An ordinary request can be served by any of them.
+        """
+        primary = self.provider_for(body)
+        if body.grounded:
+            return (primary,)
+        return (primary, *(p for p in self._providers if p is not primary))
+
+    @staticmethod
+    def _worth_another_provider(status: int) -> bool:
+        """Whether the refusal was about this provider rather than the request.
+
+        A rate limit or a fault is the provider saying "not me, not now", and
+        another one may well answer. A 4xx below that is the request itself
+        being wrong, and every provider will say the same thing — retrying it
+        elsewhere only spends a second call to be told so twice.
+        """
+        return status == 429 or status >= 500
+
     async def generate(self, body: GenerateBody) -> AiOutcome:
-        provider = self.provider_for(body)
+        outcome: AiOutcome | None = None
+        for provider in self.candidates(body):
+            attempt = await self._attempt(provider, body)
+            if attempt.status == 200 or not self._worth_another_provider(attempt.status):
+                return attempt
+            outcome = outcome or attempt
+            logger.warning(
+                "%s could not serve the request (status %s); trying the next provider",
+                provider.name,
+                attempt.status,
+            )
+        return outcome or AiOutcome(503, {"error": "No AI provider is configured."})
+
+    async def _attempt(self, provider: _Provider, body: GenerateBody) -> AiOutcome:
+        model = provider.resolve_model(body.model)
         key = provider.api_key()
         if not key:
-            return AiOutcome(503, {"error": provider.missing_key_message()})
-        request = provider.build_request(provider.resolve_model(body.model), key, body)
+            return AiOutcome(503, {"error": provider.missing_key_message()}, provider.name, model)
+        request = provider.build_request(model, key, body)
         try:
             async with httpx.AsyncClient(timeout=UPSTREAM_TIMEOUT) as client:
                 upstream = await client.post(
                     request.url, headers=request.headers, json=request.payload
                 )
             data = upstream.json()
             if not upstream.is_success:
-                return self._refused(upstream.status_code)
+                return self._refused(upstream.status_code, provider.name, model)
             completion = provider.read_completion(data)
-            return AiOutcome(200, {"text": completion.text, "truncated": completion.truncated})
+            return AiOutcome(
+                200,
+                {"text": completion.text, "truncated": completion.truncated},
+                provider.name,
+                model,
+            )
         except httpx.TimeoutException:
-            return AiOutcome(504, {"error": "Provider request timed out"})
+            return AiOutcome(504, {"error": "Provider request timed out"}, provider.name, model)
         except Exception:
             logger.exception("AI provider request failed")
-            return AiOutcome(502, {"error": "Failed to reach provider"})
+            return AiOutcome(502, {"error": "Failed to reach provider"}, provider.name, model)
 
-    def _refused(self, status: int) -> AiOutcome:
+    def _refused(self, status: int, provider: str, model: str) -> AiOutcome:
         """Upstream detail stays in the log; the caller gets a correlation id."""
         request_id = secrets.token_urlsafe(8)
-        logger.warning("AI provider refused request id=%s status=%s", request_id, status)
+        logger.warning(
+            "AI provider %s refused request id=%s status=%s", provider, request_id, status
+        )
         return AiOutcome(
             status,
             {
                 "error": "The AI provider could not complete this request.",
                 "requestId": request_id,
             },
+            provider,
+            model,
         )

server/app/database.py

@@ -127,6 +127,22 @@ def _hit_rate_limit(
     return hits <= limit, max(0, limit - hits)
 
 
+async def release_rate_limit(bucket: str, key_hash: str) -> int:
+    """Give back one hit, and report what is left. Never goes below zero."""
+    return await asyncio.to_thread(_release_rate_limit, bucket, key_hash)
+
+
+def _release_rate_limit(bucket: str, key_hash: str) -> int:
+    with psycopg.connect(database_url()) as connection:
+        row = connection.execute(
+            """UPDATE api_rate_limits SET hits = GREATEST(0, hits - 1)
+               WHERE bucket = %s AND key_hash = %s
+               RETURNING hits""",
+            (bucket, key_hash),
+        ).fetchone()
+    return int(row[0]) if row else 0
+
+
 async def find_course_access(
     subject: str | None,
     email: str | None,

server/app/dependencies.py

@@ -27,7 +27,7 @@
 from .google_auth import GoogleUser, verify_google_id_token
 from .rate_limit import SharedRateLimiter
 from .sessions import read_session
-from .settings import BURST_LIMIT, BURST_WINDOW, DAILY_QUOTA
+from .settings import BURST_LIMIT, BURST_WINDOW, DAILY_QUOTA, TRUSTED_PROXY_HOPS
 
 logger = logging.getLogger(__name__)
 
@@ -51,12 +51,41 @@ def bearer_token(authorization: str | None) -> str:
     return token.strip() if separator and scheme.lower() == "bearer" else ""
 
 
+def _forwarded_for(request: Request) -> str | None:
+    """The caller's address from X-Forwarded-For, counting from the right.
+
+    Everything a client sends itself lands at the left of this header, so the
+    only entries worth reading are the ones our own infrastructure appended.
+    `TRUSTED_PROXY_HOPS` says how many of those there are; a list too short to
+    have that many falls back to its leftmost entry, which is the closest thing
+    to the caller that exists.
+    """
+    entries = [
+        value.strip()
+        for value in request.headers.get("x-forwarded-for", "").split(",")
+        if value.strip()
+    ]
+    if not entries:
+        return None
+    index = len(entries) - 1 - TRUSTED_PROXY_HOPS
+    return entries[index] if index >= 0 else entries[0]
+
+
 def client_ip(request: Request) -> str:
-    # Fly terminates TLS and supplies this header itself. It is accepted only in
-    # production; local/test callers cannot forge a different quota identity.
+    """Who to charge a quota to, when there is no signed-in identity to use.
+
+    Read from a forwarded header only in production, where a proxy we control is
+    the one writing it; a local caller cannot forge a different quota identity.
+    The socket address is the last resort and behind a proxy it names the proxy,
+    not the caller — which is how anonymous callers ended up sharing, and
+    shuffling between, each other's quota buckets.
+    """
     if os.getenv("NODE_ENV") == "production":
+        # Fly terminates TLS and supplies this header itself.
         if forwarded := request.headers.get("fly-client-ip", "").strip():
             return forwarded
+        if forwarded_for := _forwarded_for(request):
+            return forwarded_for
     return request.client.host if request.client else "unknown"
 
 

server/app/rate_limit.py

@@ -10,7 +10,7 @@
 from typing import Literal
 
 from .config import database_url, env
-from .database import hit_rate_limit
+from .database import hit_rate_limit, release_rate_limit
 
 logger = logging.getLogger(__name__)
 
@@ -50,6 +50,13 @@ async def hit(self, key: str) -> tuple[bool, int]:
             hits.append(now)
             return True, self.limit - len(hits)
 
+    async def release(self, key: str) -> int:
+        async with self._lock:
+            hits = self._hits[key]
+            if hits:
+                hits.pop()
+            return max(0, self.limit - len(hits))
+
 
 WhenUnavailable = Literal["refuse", "degrade"]
 
@@ -88,6 +95,28 @@ def __init__(
         self.memory = MemoryRateLimiter(limit, window_seconds)
         self._warned: set[str] = set()
 
+    async def release(self, key: str) -> int | None:
+        """Give back a hit that bought the caller nothing, or None if it could not.
+
+        A quota exists to ration what the caller receives. Charging for a request
+        the provider refused spends an allowance on nothing, and with ten a day
+        that is most of a visit. Retrying is still bounded by the burst limiter,
+        so giving one back cannot become a way around the quota.
+        """
+        if os.getenv("NODE_ENV") != "production":
+            return await self.memory.release(key)
+        if shared_quota_problem():
+            return None
+        salt = env("RATE_LIMIT_SALT") or env("METRICS_ID_SALT")
+        assert salt is not None
+        digest = hmac.new(salt.encode(), key.encode(), hashlib.sha256).hexdigest()
+        try:
+            hits = await release_rate_limit(self.bucket, digest)
+        except Exception:
+            logger.exception("Could not release a rate limit hit for bucket %r", self.bucket)
+            return None
+        return max(0, self.limit - hits)
+
     async def hit(self, key: str) -> tuple[bool, int]:
         if os.getenv("NODE_ENV") != "production":
             return await self.memory.hit(key)

server/app/routes/ai.py

@@ -48,9 +48,21 @@ async def ai_generate(request: Request, ai: Ai, session: SessionUser):
         issues = validation_issues(exc.errors()) if isinstance(exc, ValidationError) else []
         return error_response("Invalid request body", 400, issues=issues, headers=headers)
 
-    provider, model = ai.target(body)
     outcome = await ai.generate(body)
-    observe_ai(request, provider=provider, model=model, email=email, status=outcome.status)
+    if outcome.status != 200:
+        # The caller received nothing, so the allowance they just spent buys
+        # them nothing either. Retrying stays bounded by the burst limiter.
+        if (remaining := await daily_limiter.release(key)) is not None:
+            headers["X-AI-Quota-Remaining"] = str(remaining)
+    # Labelled with whoever actually answered, which after a fallback is not the
+    # provider the request started with.
+    observe_ai(
+        request,
+        provider=outcome.provider,
+        model=outcome.model,
+        email=email,
+        status=outcome.status,
+    )
     return JSONResponse(outcome.payload, status_code=outcome.status, headers=headers)
 
 

server/app/settings.py

@@ -15,5 +15,14 @@
 UPSTREAM_TIMEOUT = positive_int("AI_UPSTREAM_TIMEOUT_MS", 30_000) / 1000
 MAX_REQUEST_BODY = positive_int("MAX_REQUEST_BODY_BYTES", 96 * 1024)
 
+# How many rightmost X-Forwarded-For entries are written by infrastructure we
+# control and must therefore be skipped to reach the caller. A client can put
+# anything in that header, but only ever at the left, so counting from the right
+# is what makes the value trustworthy. One matches a single load balancer that
+# appends the caller and then itself, which is the common cloud shape; set it to
+# match the deployment, because too high trusts a forged entry and too low keys
+# every caller behind one proxy to the same bucket.
+TRUSTED_PROXY_HOPS = positive_int("TRUSTED_PROXY_HOPS", 1)
+
 COURSE_SKU = "ai-testing-bootcamp"
 TERMS_VERSION = "2026-08-20"