From 8525b81170d4e1602ca542a6f3b7cf7a5b7029bb Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:37:36 -0400 Subject: [PATCH 1/2] feat(quiz): rate limit, daily spend guard, generation timeout, failure events (#544) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Workstream F of the pre-revamp quiz repair batch (epic #537): F1 — generate is no longer an unbounded LLM call behind a button: a per-user sliding-window limit (8 per 5 minutes, sized for a human comparing difficulties) returns 429 QUIZ_RATE_LIMITED with Retry-After, and a daily per-user LLM spend ceiling ($2, read off the llm_usage ledger agents/usage.py already writes) returns 429 QUIZ_DAILY_LIMIT_REACHED before the model runs. Both guards sit AFTER the ownership check, so probing a stranger's concept can't consume their quota, and the spend check fails OPEN — a usage-table blip must not deny every student. F2 — the whole generation (agent run + tools + the E2 top-up) is bounded by QUIZ_GENERATION_TIMEOUT_SEC and maps to its own QUIZ_GENERATION_TIMEOUT code, so the client can say "that took too long" instead of the generic failure. F3 — quiz.generation_failed events (category=error, with a reason: timeout / agent_guardrail / agent_error) join the pinned taxonomy, so a 502 the student saw is a 502 an admin can count in the errors feed #548 widened. A throttled student is deliberately NOT an error event. F4 — deep-link scoping tests: a foreign concept 404s before the agent runs, and a student's OWN concept from a past semester still generates (scoping is by ownership, not active semester — pinned so a future "scope to active semester" change can't silently break revision). Also: the process-global rate-limit state now resets between tests via a conftest autouse fixture, same reasoning as the lru_cache reset — without it one test's burst throttles every later test hitting the same route. Co-Authored-By: Claude Fable 5 --- backend/main.py | 8 +- backend/routes/quiz.py | 116 ++++++- backend/services/events_service.py | 4 + backend/services/quiz_config.py | 25 ++ backend/services/quiz_errors.py | 11 +- backend/tests/conftest.py | 13 + backend/tests/test_event_capture_seams.py | 1 + .../tests/test_quiz_cost_observability_f.py | 317 ++++++++++++++++++ 8 files changed, 483 insertions(+), 12 deletions(-) create mode 100644 backend/tests/test_quiz_cost_observability_f.py diff --git a/backend/main.py b/backend/main.py index 0d8c9eff..51328b28 100644 --- a/backend/main.py +++ b/backend/main.py @@ -201,10 +201,16 @@ async def http_exception_handler(request: Request, exc: StarletteHTTPException): code=getattr(exc, "code", None), machine_detail=getattr(exc, "machine_detail", None), ) + # Preserve headers the raise site set (e.g. Retry-After on a 429) — + # dropping them would strip the only machine-readable part of a + # throttling response. + headers = dict(getattr(exc, "headers", None) or {}) + if rid: + headers["X-Request-ID"] = rid return JSONResponse( status_code=exc.status_code, content=content, - headers={"X-Request-ID": rid} if rid else {}, + headers=headers, ) diff --git a/backend/routes/quiz.py b/backend/routes/quiz.py index 28b95909..f1ea6522 100644 --- a/backend/routes/quiz.py +++ b/backend/routes/quiz.py @@ -25,12 +25,17 @@ from services.quiz_config import ( CONCRETE_DIFFICULTIES, QUIZ_ATTEMPT_ABANDON_TTL_HOURS, + QUIZ_DAILY_SPEND_CAP_USD, + QUIZ_GENERATE_RATE_LIMIT, + QUIZ_GENERATE_RATE_WINDOW_SEC, + QUIZ_GENERATION_TIMEOUT_SEC, QUIZ_TOPUP_DROP_RATIO, QUIZ_TOPUP_MAX_RETRIES, REQUESTED_DIFFICULTIES, mastery_after, quiz_config_payload, ) +from services.request_limits import check_rate_limit from services.quiz_errors import QuizAPIError, QuizErrorCode from services.profiles import get_display_name from services.encryption import encrypt_json, decrypt_json_column @@ -87,6 +92,43 @@ def _load_prompt(name: str) -> str: _MAX_HISTORY_OFFSET = 1_000_000 +def _daily_spend_exceeded(user_id: str) -> bool: + """True if this user is past the daily LLM spend ceiling (#544 F1). + + Reads the llm_usage ledger agents/usage.py already writes. Fails OPEN + on any error — this is a cost control, not a correctness gate, and + denying every student because a usage read blipped is worse than the + spend it would save. + """ + try: + since = (datetime.now(timezone.utc) - timedelta(days=1)).isoformat() + rows = table("llm_usage").select( + "cost_usd", + filters={"user_id": f"eq.{user_id}", "created_at": f"gte.{since}"}, + ) or [] + spent = sum(float(r.get("cost_usd") or 0.0) for r in rows) + return spent >= QUIZ_DAILY_SPEND_CAP_USD + except Exception: + logger.exception("quiz: daily spend check failed user=%s; allowing", user_id) + return False + + +def _log_generation_failed(body, request_id: str | None, reason: str) -> None: + """#544 F3: make a 502 the student saw a 502 an admin can count.""" + events_service.log_event( + "quiz.generation_failed", + category="error", + user_id=body.user_id, + request_id=request_id, + payload={ + "concept_node_id": body.concept_node_id, + "difficulty": body.difficulty, + "num_questions": body.num_questions, + "reason": reason, + }, + ) + + def _abandon_cutoff() -> datetime: return datetime.now(timezone.utc) - timedelta( hours=QUIZ_ATTEMPT_ABANDON_TTL_HOURS @@ -652,26 +694,79 @@ async def generate_quiz(body: GenerateQuizBody, request: Request): or str(uuid.uuid4()) ) + # #544 F1: cost guards run AFTER ownership (a stranger's node 404s + # first, so probing can't consume a victim's quota) and BEFORE the + # model call. Neither rejection is a backend failure, so neither emits + # quiz.generation_failed. + retry_after = check_rate_limit( + f"quiz_generate:{body.user_id}", + limit=QUIZ_GENERATE_RATE_LIMIT, + window_sec=QUIZ_GENERATE_RATE_WINDOW_SEC, + ) + if retry_after is not None: + raise QuizAPIError( + status_code=429, + code=QuizErrorCode.QUIZ_RATE_LIMITED, + message=( + "You've generated a lot of quizzes just now — " + "take a moment and try again shortly." + ), + headers={"Retry-After": str(retry_after)}, + ) + if _daily_spend_exceeded(body.user_id): + logger.warning( + "quiz: daily spend cap reached user=%s request_id=%s", + body.user_id, request_id, + ) + raise QuizAPIError( + status_code=429, + code=QuizErrorCode.QUIZ_DAILY_LIMIT_REACHED, + message=( + "You've reached today's limit for AI-generated study " + "material. It resets tomorrow." + ), + ) + try: - questions = await _quiz_via_agent( - user_id=body.user_id, - course_id=course_id, - concept_node_id=body.concept_node_id, - concept_name=concept_name, - num_questions=body.num_questions, - difficulty=body.difficulty, - use_shared_context=body.use_shared_context, - request_id=request_id, - model_pref=body.model_pref, + # #544 F2: bound the whole generation (agent run + its tool calls + + # the bounded top-up). Past this the student is watching a spinner + # and the request is holding a worker slot for nothing. + questions = await asyncio.wait_for( + _quiz_via_agent( + user_id=body.user_id, + course_id=course_id, + concept_node_id=body.concept_node_id, + concept_name=concept_name, + num_questions=body.num_questions, + difficulty=body.difficulty, + use_shared_context=body.use_shared_context, + request_id=request_id, + model_pref=body.model_pref, + ), + timeout=QUIZ_GENERATION_TIMEOUT_SEC, ) except HTTPException: # The 404 for an unknown concept node is raised before the agent call; # never swallow a known HTTP state. raise + except (asyncio.TimeoutError, TimeoutError) as e: + # #544 F2: distinct from a generic failure — the client can say + # "that took too long" and offering a retry obviously makes sense. + logger.warning( + "quiz: generation timed out after %ss request_id=%s", + QUIZ_GENERATION_TIMEOUT_SEC, request_id, + ) + _log_generation_failed(body, request_id, "timeout") + raise QuizAPIError( + status_code=502, + code=QuizErrorCode.QUIZ_GENERATION_TIMEOUT, + message="Quiz generation took too long. Please try again.", + ) from e except (UsageLimitExceeded, UnexpectedModelBehavior) as e: # The raw-Gemini legacy fallback was retired in #145; degrade to 502 # rather than serving a quiz from a second LLM path. logger.warning("Quiz agent guardrails tripped; returning 502", exc_info=e) + _log_generation_failed(body, request_id, "agent_guardrail") raise QuizAPIError( status_code=502, code=QuizErrorCode.QUIZ_GENERATION_FAILED, @@ -679,6 +774,7 @@ async def generate_quiz(body: GenerateQuizBody, request: Request): ) from e except Exception as e: logger.exception("Unexpected quiz-agent failure; returning 502") + _log_generation_failed(body, request_id, "agent_error") raise QuizAPIError( status_code=502, code=QuizErrorCode.QUIZ_GENERATION_FAILED, diff --git a/backend/services/events_service.py b/backend/services/events_service.py index fce2dff4..780c528b 100644 --- a/backend/services/events_service.py +++ b/backend/services/events_service.py @@ -86,6 +86,10 @@ # surfaces in admin analytics — this failure was invisible for months # precisely because nothing emitted when the background task died. "quiz.context_write_failed", + # #544/F3: generation failed (agent error, timeout, or every question + # dropped). Same reasoning: a 502 the student sees should be a 502 an + # admin can count. + "quiz.generation_failed", "chat.message_sent", "note.created", "session.started", diff --git a/backend/services/quiz_config.py b/backend/services/quiz_config.py index d1b5d98f..b2e21341 100644 --- a/backend/services/quiz_config.py +++ b/backend/services/quiz_config.py @@ -79,6 +79,31 @@ def mastery_after(before: float, *, score: int, total: int) -> float: return max(0.0, min(1.0, raw)) +# ── Cost + abuse guards (#544 F1/F2) ──────────────────────────────────────── +# +# Generation is an unbounded LLM call behind a button: before #544 nothing +# stopped a held-down key or a scripted loop from spending real money. +# +# The rate limit is sized for a human: a student comparing difficulties or +# retaking a concept might legitimately generate a handful of quizzes in a +# few minutes; nobody legitimately generates 10 in one. +QUIZ_GENERATE_RATE_LIMIT = 8 +QUIZ_GENERATE_RATE_WINDOW_SEC = 300 # 5 minutes + +# Daily per-user LLM spend ceiling across ALL features (read off llm_usage, +# which agents/usage.py::record_agent_usage already writes). A generation on +# the default flash-lite tier costs well under a cent, so this is ~2 orders +# of magnitude above any real study day — it exists to bound a runaway, not +# to ration normal use. Deliberately fail-OPEN: if the usage read errors we +# serve the quiz rather than denying every student on a table blip. +QUIZ_DAILY_SPEND_CAP_USD = 2.00 + +# Wall-clock ceiling on one generation (agent run incl. its tool calls). +# Past this the student is staring at a spinner and would rather be told to +# try again; the request also stops holding a worker slot. +QUIZ_GENERATION_TIMEOUT_SEC = 90 + + # ── Generation honesty (#543 E2) ──────────────────────────────────────────── # # Questions whose correct_answer doesn't match an option verbatim are diff --git a/backend/services/quiz_errors.py b/backend/services/quiz_errors.py index d7591faf..7dd813a3 100644 --- a/backend/services/quiz_errors.py +++ b/backend/services/quiz_errors.py @@ -46,6 +46,14 @@ class QuizErrorCode(str, Enum): QUIZ_QUESTION_INVALID = "QUIZ_QUESTION_INVALID" QUIZ_NOT_AUTHORIZED = "QUIZ_NOT_AUTHORIZED" QUIZ_GENERATION_FAILED = "QUIZ_GENERATION_FAILED" + # #544 F2: the generation exceeded its wall-clock budget. Distinct from + # the generic failure so the client can say "that took too long" and a + # retry is obviously worth offering. + QUIZ_GENERATION_TIMEOUT = "QUIZ_GENERATION_TIMEOUT" + # #544 F1: too many generations in the rate window. + QUIZ_RATE_LIMITED = "QUIZ_RATE_LIMITED" + # #544 F1: this account's daily LLM spend ceiling is reached. + QUIZ_DAILY_LIMIT_REACHED = "QUIZ_DAILY_LIMIT_REACHED" QUIZ_INTERNAL_ERROR = "QUIZ_INTERNAL_ERROR" # Uncoded HTTP errors that aren't one of the semantic states above — # router 404s/405s on version-skewed clients, library-raised @@ -80,8 +88,9 @@ def __init__( code: QuizErrorCode, message: str, machine_detail=None, + headers: dict[str, str] | None = None, ): - super().__init__(status_code=status_code, detail=message) + super().__init__(status_code=status_code, detail=message, headers=headers) self.code = code self.machine_detail = machine_detail diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 1248bbd6..6b7a50f7 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -47,6 +47,19 @@ def pytest_configure(config): ) +@pytest.fixture(autouse=True) +def _clear_rate_limit_state(): + """#544: services/request_limits keeps its sliding windows in a + process-global dict, so one test's burst of requests would throttle + every later test that hits the same route as the same user. Same + reasoning as the lru_cache reset below.""" + from services import request_limits + + request_limits._rate_state.clear() + yield + request_limits._rate_state.clear() + + @pytest.fixture(autouse=True) def _clear_lru_caches(): """#98: reset the per-process lru_caches around every test so one test's diff --git a/backend/tests/test_event_capture_seams.py b/backend/tests/test_event_capture_seams.py index 9edbd03c..faf0f8a0 100644 --- a/backend/tests/test_event_capture_seams.py +++ b/backend/tests/test_event_capture_seams.py @@ -93,6 +93,7 @@ def test_event_taxonomy_is_pinned(): "quiz.started", "quiz.completed", "quiz.context_write_failed", # #529/B3 + "quiz.generation_failed", # #544/F3 "chat.message_sent", "note.created", "session.started", diff --git a/backend/tests/test_quiz_cost_observability_f.py b/backend/tests/test_quiz_cost_observability_f.py new file mode 100644 index 00000000..805ca530 --- /dev/null +++ b/backend/tests/test_quiz_cost_observability_f.py @@ -0,0 +1,317 @@ +""" +Workstream F of the pre-revamp quiz repair batch (#544, epic #537): +cost, abuse, observability. + +- F1: per-user rate limit on generate + a daily LLM spend guard. + Generation is an unbounded LLM call behind a button; nothing stopped a + loop. +- F2: explicit timeout on the agent call, mapped to a 502 taxonomy + rather than one generic code. +- F3: quiz.generation_failed events so backend failures reach admin + analytics (quiz.context_write_failed landed with #529/B3). +- F4: ownership + active-semester scoping on ?concept= deep links. +""" +import asyncio +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +from fastapi.testclient import TestClient + +from main import app +from agents.quiz import Quiz, QuizQuestion + +client = TestClient(app) + + +def _quiz(): + return Quiz(questions=[ + QuizQuestion( + question="Q?", type="multiple_choice", difficulty="easy", + options=["a", "b", "c", "d"], correct_answer="a", + explanation="x", concept="Loops", + ), + ]) + + +def _factory(*, spend_rows=None): + def factory(name): + mock = MagicMock() + if name == "graph_nodes": + mock.select.return_value = [{ + "id": "node1", "course_id": "course1", + "concept_name": "Loops", "mastery_score": 0.5, + }] + elif name == "quiz_attempts": + mock.insert.return_value = [{"id": "quiz-generated"}] + elif name == "llm_usage": + mock.select.return_value = spend_rows if spend_rows is not None else [] + else: + mock.select.return_value = [] + mock.insert.return_value = [] + return mock + + return factory + + +def _generate(user_id="user_andres"): + return client.post("/api/quiz/generate", json={ + "user_id": user_id, + "concept_node_id": "node1", + "num_questions": 1, + "difficulty": "easy", + "use_shared_context": False, + }) + + +# ── F1: rate limit + spend guard ──────────────────────────────────────────── + + +class TestGenerateRateLimit: + def test_burst_past_the_limit_is_rejected(self): + from services.quiz_config import ( + QUIZ_GENERATE_RATE_LIMIT, + QUIZ_GENERATE_RATE_WINDOW_SEC, + ) + + assert QUIZ_GENERATE_RATE_WINDOW_SEC > 0 + run = AsyncMock(return_value=SimpleNamespace(output=_quiz())) + with ( + patch("routes.quiz.table", side_effect=_factory()), + patch("routes.quiz.quiz_agent.run", new=run), + ): + allowed = [_generate() for _ in range(QUIZ_GENERATE_RATE_LIMIT)] + blocked = _generate() + + assert all(r.status_code == 200 for r in allowed) + assert blocked.status_code == 429 + body = blocked.json() + assert body["error"]["code"] == "QUIZ_RATE_LIMITED" + assert "Retry-After" in blocked.headers + # The blocked request must not have reached the model. + assert run.call_count == QUIZ_GENERATE_RATE_LIMIT + + def test_limit_is_per_user(self): + run = AsyncMock(return_value=SimpleNamespace(output=_quiz())) + from services.quiz_config import QUIZ_GENERATE_RATE_LIMIT + + with ( + patch("routes.quiz.table", side_effect=_factory()), + patch("routes.quiz.quiz_agent.run", new=run), + ): + for _ in range(QUIZ_GENERATE_RATE_LIMIT): + _generate("user_andres") + # A different user starts with a fresh window. require_self is + # stubbed per-request from the body's user_id in conftest. + other = _generate("user_beatriz") + assert other.status_code == 200 + + +class TestDailySpendGuard: + def test_over_budget_user_is_refused_before_the_model_runs(self): + from services.quiz_config import QUIZ_DAILY_SPEND_CAP_USD + + spent = [{"cost_usd": QUIZ_DAILY_SPEND_CAP_USD + 1.0}] + run = AsyncMock(return_value=SimpleNamespace(output=_quiz())) + with ( + patch("routes.quiz.table", side_effect=_factory(spend_rows=spent)), + patch("routes.quiz.quiz_agent.run", new=run), + ): + r = _generate() + assert r.status_code == 429 + assert r.json()["error"]["code"] == "QUIZ_DAILY_LIMIT_REACHED" + run.assert_not_called() + + def test_under_budget_passes(self): + from services.quiz_config import QUIZ_DAILY_SPEND_CAP_USD + + spent = [{"cost_usd": QUIZ_DAILY_SPEND_CAP_USD / 2}] + run = AsyncMock(return_value=SimpleNamespace(output=_quiz())) + with ( + patch("routes.quiz.table", side_effect=_factory(spend_rows=spent)), + patch("routes.quiz.quiz_agent.run", new=run), + ): + r = _generate() + assert r.status_code == 200 + + def test_spend_lookup_failure_never_blocks_a_quiz(self): + """The guard is a cost control, not a correctness gate — if the + usage table is unreachable we let the quiz through rather than + failing closed on every student.""" + def factory(name): + mock = MagicMock() + if name == "graph_nodes": + mock.select.return_value = [{ + "id": "node1", "course_id": "course1", + "concept_name": "Loops", "mastery_score": 0.5, + }] + elif name == "llm_usage": + mock.select.side_effect = RuntimeError("db down") + else: + mock.select.return_value = [] + mock.insert.return_value = [{"id": "q"}] + return mock + + with ( + patch("routes.quiz.table", side_effect=factory), + patch("routes.quiz.quiz_agent.run", + new=AsyncMock(return_value=SimpleNamespace(output=_quiz()))), + ): + r = _generate() + assert r.status_code == 200 + + +# ── F2: timeout → its own 502 code ────────────────────────────────────────── + + +class TestGenerationTimeout: + def test_agent_timeout_maps_to_its_own_code(self): + from services.quiz_config import QUIZ_GENERATION_TIMEOUT_SEC + + assert QUIZ_GENERATION_TIMEOUT_SEC > 0 + + async def _never(*a, **k): + await asyncio.sleep(3600) + + with ( + patch("routes.quiz.table", side_effect=_factory()), + patch("routes.quiz.quiz_agent.run", new=_never), + patch("routes.quiz.QUIZ_GENERATION_TIMEOUT_SEC", 0.05), + ): + r = _generate() + assert r.status_code == 502 + assert r.json()["error"]["code"] == "QUIZ_GENERATION_TIMEOUT" + + def test_ordinary_failure_keeps_the_generic_code(self): + with ( + patch("routes.quiz.table", side_effect=_factory()), + patch("routes.quiz.quiz_agent.run", + new=AsyncMock(side_effect=RuntimeError("boom"))), + ): + r = _generate() + assert r.status_code == 502 + assert r.json()["error"]["code"] == "QUIZ_GENERATION_FAILED" + + +# ── F3: failure events reach admin analytics ──────────────────────────────── + + +class TestGenerationFailureEvent: + def test_generation_failure_emits_an_event(self): + events = [] + with ( + patch("routes.quiz.table", side_effect=_factory()), + patch("routes.quiz.quiz_agent.run", + new=AsyncMock(side_effect=RuntimeError("boom"))), + patch("routes.quiz.events_service.log_event", + side_effect=lambda *a, **k: events.append((a, k))), + ): + r = _generate() + assert r.status_code == 502 + failed = [(a, k) for a, k in events if a and a[0] == "quiz.generation_failed"] + assert len(failed) == 1, ( + "a generation failure must be visible in admin analytics, not just logs" + ) + _, kwargs = failed[0] + assert kwargs["category"] == "error" + assert kwargs["user_id"] == "user_andres" + payload = kwargs["payload"] + assert payload["concept_node_id"] == "node1" + assert payload["reason"] == "agent_error" + + def test_event_is_in_the_pinned_taxonomy(self): + from services import events_service + + assert "quiz.generation_failed" in events_service.EVENT_TAXONOMY + + def test_rate_limit_rejection_is_not_an_error_event(self): + """A throttled student isn't a backend failure — it must not + pollute the error feed.""" + from services.quiz_config import QUIZ_GENERATE_RATE_LIMIT + + events = [] + with ( + patch("routes.quiz.table", side_effect=_factory()), + patch("routes.quiz.quiz_agent.run", + new=AsyncMock(return_value=SimpleNamespace(output=_quiz()))), + patch("routes.quiz.events_service.log_event", + side_effect=lambda *a, **k: events.append((a, k))), + ): + for _ in range(QUIZ_GENERATE_RATE_LIMIT + 1): + _generate() + assert not [a for a, _ in events if a and a[0] == "quiz.generation_failed"] + + +# ── F4: ownership + semester scoping on deep links ────────────────────────── + + +class TestConceptScoping: + """`?concept=` deep links hand the route an arbitrary node id. The + owner-scoped read is the gate: a node belonging to someone else, or to + a course the student isn't enrolled in, must 404 before the agent runs + — no content leak, no mastery write.""" + + def _scoped_select(self, *, owned_by="user_andres", course_id="course1"): + def _select(columns="*", filters=None, **_): + filters = filters or {} + if filters.get("id") != "eq.node_x": + return [] + if filters.get("user_id") not in (None, f"eq.{owned_by}"): + return [] + return [{ + "id": "node_x", "user_id": owned_by, "course_id": course_id, + "concept_name": "Someone Else's Concept", "mastery_score": 0.4, + }] + + return _select + + def _post(self, user_id): + return client.post("/api/quiz/generate", json={ + "user_id": user_id, "concept_node_id": "node_x", + "num_questions": 1, "difficulty": "easy", + "use_shared_context": False, + }) + + def test_foreign_concept_404s_before_the_agent_runs(self): + run = AsyncMock() + + def factory(name): + mock = MagicMock() + if name == "graph_nodes": + mock.select.side_effect = self._scoped_select(owned_by="user_beatriz") + else: + mock.select.return_value = [] + return mock + + with ( + patch("routes.quiz.table", side_effect=factory), + patch("routes.quiz.quiz_agent.run", new=run), + ): + r = self._post("user_andres") + assert r.status_code == 404 + assert r.json()["error"]["code"] == "QUIZ_CONCEPT_NOT_FOUND" + run.assert_not_called() + + def test_own_concept_from_a_past_semester_still_generates(self): + """Scoping is by OWNERSHIP, not by active semester: a student + revising last term's concept is legitimate. Pinning this stops a + future 'scope to active semester' change from silently breaking + revision — if that becomes desired it needs its own decision.""" + run = AsyncMock(return_value=SimpleNamespace(output=_quiz())) + + def factory(name): + mock = MagicMock() + if name == "graph_nodes": + mock.select.side_effect = self._scoped_select(course_id="old-course") + elif name == "quiz_attempts": + mock.insert.return_value = [{"id": "q"}] + else: + mock.select.return_value = [] + return mock + + with ( + patch("routes.quiz.table", side_effect=factory), + patch("routes.quiz.quiz_agent.run", new=run), + ): + r = self._post("user_andres") + assert r.status_code == 200 + run.assert_called_once() From 1485478875a725ea829d2d460b5f33eb2346cbcb Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Thu, 13 Aug 2026 02:11:55 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(quiz):=20address=20#552=20review=20?= =?UTF-8?q?=E2=80=94=20each=20guardrail=20was=20undercutting=20itself?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three F guards failed at what they were added to do, each confirmed by execution in review: - The daily spend cap summed an UNPAGED llm_usage select. PostgREST caps a response at max_rows (1000) and answers 206 — a 2xx — so the sum plateaued and the ceiling could never trip for the runaway user it targets. It pages now (with an early exit once the cap is crossed), and db/connection.py::select gained the `offset` it needed. - Wrapping the whole generation in asyncio.wait_for raised CancelledError — a BaseException — straight past #543's serve-what-we-have handler, so a timed-out top-up threw away a valid partial quiz and returned 502. Each agent run is bounded individually now, so a top-up timeout is an ordinary TimeoutError the existing handler degrades from; a completed 3-question generation is served instead of discarded. - The rate-limit slot was claimed before generation and never refunded, so eight backend 502s locked a student out for five minutes with a message saying they'd generated too many quizzes — having received none. Every failure path now refunds the slot (services/request_limits.py::refund_rate_limit). Also from the review: - The timeout branch caught the builtin OSError-family TimeoutError as well, relabelling transport socket timeouts as wall-clock timeouts. It catches only asyncio.TimeoutError now. - The spend-cap comment claimed cross-feature enforcement it doesn't provide; it now says what's actually true (the spend measured is cross-feature, the ceiling is enforced on quiz generation only). - main.py forwards exc.headers as of this branch, which falsified the comments in routes/extract.py and routes/gradescope.py explaining why they couldn't send Retry-After. Both now send it. Known and accepted: a cancelled agent run never reaches record_agent_usage, so a timed-out generation's tokens don't land in llm_usage. Capturing usage from a cancelled pydantic-ai run isn't available at this seam; the per-run timeout narrows the window considerably versus cancelling the whole request. Co-Authored-By: Claude Fable 5 --- backend/db/connection.py | 7 + backend/routes/extract.py | 7 +- backend/routes/gradescope.py | 6 +- backend/routes/quiz.py | 111 +++++++++++----- backend/services/quiz_config.py | 15 ++- backend/services/request_limits.py | 14 ++ .../tests/test_quiz_cost_observability_f.py | 120 ++++++++++++++++++ 7 files changed, 239 insertions(+), 41 deletions(-) diff --git a/backend/db/connection.py b/backend/db/connection.py index 089dfba5..f62db3b3 100644 --- a/backend/db/connection.py +++ b/backend/db/connection.py @@ -34,7 +34,12 @@ def select( filters: Optional[dict] = None, order: Optional[str] = None, limit: Optional[int] = None, + offset: Optional[int] = None, ) -> list: + """Read rows. Pass `limit`/`offset` to page — PostgREST caps a + response at `max_rows` (1000) and answers 206 Partial Content, + which is a 2xx, so an unpaged read over that many rows truncates + silently.""" params: dict = {"select": columns} if filters: params.update(filters) @@ -42,6 +47,8 @@ def select( params["order"] = order if limit: params["limit"] = str(limit) + if offset is not None: + params["offset"] = str(offset) r = _client.get(self.url, params=params) r.raise_for_status() return r.json() diff --git a/backend/routes/extract.py b/backend/routes/extract.py index 8f8bd9e2..f23f2545 100644 --- a/backend/routes/extract.py +++ b/backend/routes/extract.py @@ -30,12 +30,13 @@ def _enforce_ocr_limits(request: Request) -> str: user_id = get_session_user_id(request) retry = check_rate_limit(f"ocr:{user_id}", limit=_OCR_RATE_LIMIT, window_sec=_OCR_RATE_WINDOW) if retry is not None: - # NB: the app's global HTTPException handler (main.py) doesn't forward - # exc.headers, so the retry budget is conveyed in the detail string - # rather than a Retry-After header. + # main.py's HTTPException handler forwards exc.headers as of #544, so + # the budget rides in a real Retry-After. It stays in the detail + # string too — clients that only surface the message keep working. raise HTTPException( status_code=429, detail=f"Too many OCR requests. Retry in {retry}s.", + headers={"Retry-After": str(retry)}, ) return user_id diff --git a/backend/routes/gradescope.py b/backend/routes/gradescope.py index 6e29333a..e61af339 100644 --- a/backend/routes/gradescope.py +++ b/backend/routes/gradescope.py @@ -202,11 +202,13 @@ def _enforce_gs_rate_limit(user_id: str, action: str, *, limit: int, window_sec: f"gradescope:{action}:{user_id}", limit=limit, window_sec=window_sec ) if retry is not None: - # main.py's HTTPException handler drops exc.headers, so the retry budget - # rides in the detail string rather than a Retry-After header. + # main.py's HTTPException handler forwards exc.headers as of #544, so + # the budget rides in a real Retry-After. It stays in the detail + # string too — clients that only surface the message keep working. raise HTTPException( status_code=429, detail=f"Too many Gradescope {action} requests. Retry in {retry}s.", + headers={"Retry-After": str(retry)}, ) diff --git a/backend/routes/quiz.py b/backend/routes/quiz.py index f1ea6522..c137aa3f 100644 --- a/backend/routes/quiz.py +++ b/backend/routes/quiz.py @@ -35,7 +35,7 @@ mastery_after, quiz_config_payload, ) -from services.request_limits import check_rate_limit +from services.request_limits import check_rate_limit, refund_rate_limit from services.quiz_errors import QuizAPIError, QuizErrorCode from services.profiles import get_display_name from services.encryption import encrypt_json, decrypt_json_column @@ -92,27 +92,64 @@ def _load_prompt(name: str) -> str: _MAX_HISTORY_OFFSET = 1_000_000 +# supabase/config.toml sets PostgREST's max_rows = 1000, and an over-cap +# response is 206 Partial Content — a 2xx, so raise_for_status never fires +# and the truncation is silent. Same constant and same reasoning as +# achievement_service._daily_totals; page to completion or the sum is a lie. +_USAGE_PAGE = 1000 + + def _daily_spend_exceeded(user_id: str) -> bool: """True if this user is past the daily LLM spend ceiling (#544 F1). - Reads the llm_usage ledger agents/usage.py already writes. Fails OPEN - on any error — this is a cost control, not a correctness gate, and - denying every student because a usage read blipped is worse than the - spend it would save. + Reads the llm_usage ledger agents/usage.py already writes, PAGED: an + unpaged read stops at max_rows, so a heavy user's sum plateaus below + the cap and the guard never trips for exactly the runaway it targets. + Stops early once the ceiling is crossed — the common case is a couple + of rows, and a user past the cap doesn't need an exact total. + + Fails OPEN on any error: this is a cost control, not a correctness + gate, and denying every student because a usage read blipped is worse + than the spend it would save. """ try: since = (datetime.now(timezone.utc) - timedelta(days=1)).isoformat() - rows = table("llm_usage").select( - "cost_usd", - filters={"user_id": f"eq.{user_id}", "created_at": f"gte.{since}"}, - ) or [] - spent = sum(float(r.get("cost_usd") or 0.0) for r in rows) - return spent >= QUIZ_DAILY_SPEND_CAP_USD + spent = 0.0 + offset = 0 + while True: + rows = table("llm_usage").select( + "cost_usd", + filters={"user_id": f"eq.{user_id}", "created_at": f"gte.{since}"}, + limit=_USAGE_PAGE, + offset=offset, + ) or [] + spent += sum(float(r.get("cost_usd") or 0.0) for r in rows) + if spent >= QUIZ_DAILY_SPEND_CAP_USD: + return True + if len(rows) < _USAGE_PAGE: + return False + offset += _USAGE_PAGE except Exception: logger.exception("quiz: daily spend check failed user=%s; allowing", user_id) return False +def _refund_generate_slot(user_id: str) -> None: + """Hand back the rate-limit slot a failed generation consumed (#544 F1). + + The slot is claimed BEFORE the model runs (so a burst can't get past + the gate concurrently), which means a backend failure would otherwise + spend the student's quota: eight 502s in two minutes would lock them + out for five with a message saying they'd generated too many quizzes, + having received none. A failure the student didn't cause shouldn't + cost them anything, and the 502 explicitly invites a retry. + """ + try: + refund_rate_limit(f"quiz_generate:{user_id}") + except Exception: + logger.exception("quiz: rate-limit refund failed user=%s", user_id) + + def _log_generation_failed(body, request_id: str | None, reason: str) -> None: """#544 F3: make a 502 the student saw a 502 an admin can count.""" events_service.log_event( @@ -552,8 +589,17 @@ async def _quiz_via_agent( run_kwargs["model"] = model_override async def _run(message: str, limits) -> Quiz: + # #544 F2: bound EACH agent run rather than the whole function. + # Wrapping the outer coroutine cancelled it mid-flight, and + # CancelledError is a BaseException — it flew straight past the + # top-up's serve-what-we-have handler and threw away questions the + # student had already paid for. Timing out one run raises an + # ordinary TimeoutError the existing handlers can reason about. result = record_agent_usage( - await quiz_agent.run(message, usage_limits=limits, **run_kwargs), + await asyncio.wait_for( + quiz_agent.run(message, usage_limits=limits, **run_kwargs), + timeout=QUIZ_GENERATION_TIMEOUT_SEC, + ), feature="quiz", task="quiz", user_id=deps.user_id, ) return result.output @@ -624,7 +670,7 @@ def _absorb(quiz: Quiz) -> None: ) try: _absorb(await _run(topup_msg, TOPUP_LIMITS)) - except Exception as e: + except (Exception, asyncio.TimeoutError) as e: # The request deliberately SUCCEEDS from here — serve the # short quiz with an honest count. No traceback: the E2E # logscan oracle reports those as findings, and this path @@ -728,34 +774,37 @@ async def generate_quiz(body: GenerateQuizBody, request: Request): ) try: - # #544 F2: bound the whole generation (agent run + its tool calls + - # the bounded top-up). Past this the student is watching a spinner - # and the request is holding a worker slot for nothing. - questions = await asyncio.wait_for( - _quiz_via_agent( - user_id=body.user_id, - course_id=course_id, - concept_node_id=body.concept_node_id, - concept_name=concept_name, - num_questions=body.num_questions, - difficulty=body.difficulty, - use_shared_context=body.use_shared_context, - request_id=request_id, - model_pref=body.model_pref, - ), - timeout=QUIZ_GENERATION_TIMEOUT_SEC, + # Each agent run inside is individually bounded by + # QUIZ_GENERATION_TIMEOUT_SEC (see _run) — cancelling the whole + # coroutine here would discard a partial quiz the top-up handler + # is designed to serve. + questions = await _quiz_via_agent( + user_id=body.user_id, + course_id=course_id, + concept_node_id=body.concept_node_id, + concept_name=concept_name, + num_questions=body.num_questions, + difficulty=body.difficulty, + use_shared_context=body.use_shared_context, + request_id=request_id, + model_pref=body.model_pref, ) except HTTPException: # The 404 for an unknown concept node is raised before the agent call; # never swallow a known HTTP state. + _refund_generate_slot(body.user_id) raise - except (asyncio.TimeoutError, TimeoutError) as e: + except asyncio.TimeoutError as e: # #544 F2: distinct from a generic failure — the client can say # "that took too long" and offering a retry obviously makes sense. + # NB: only asyncio.TimeoutError. The builtin TimeoutError is in the + # OSError family, so catching it too would relabel a transport + # socket timeout as a wall-clock generation timeout. logger.warning( "quiz: generation timed out after %ss request_id=%s", QUIZ_GENERATION_TIMEOUT_SEC, request_id, ) + _refund_generate_slot(body.user_id) _log_generation_failed(body, request_id, "timeout") raise QuizAPIError( status_code=502, @@ -766,6 +815,7 @@ async def generate_quiz(body: GenerateQuizBody, request: Request): # The raw-Gemini legacy fallback was retired in #145; degrade to 502 # rather than serving a quiz from a second LLM path. logger.warning("Quiz agent guardrails tripped; returning 502", exc_info=e) + _refund_generate_slot(body.user_id) _log_generation_failed(body, request_id, "agent_guardrail") raise QuizAPIError( status_code=502, @@ -774,6 +824,7 @@ async def generate_quiz(body: GenerateQuizBody, request: Request): ) from e except Exception as e: logger.exception("Unexpected quiz-agent failure; returning 502") + _refund_generate_slot(body.user_id) _log_generation_failed(body, request_id, "agent_error") raise QuizAPIError( status_code=502, diff --git a/backend/services/quiz_config.py b/backend/services/quiz_config.py index b2e21341..fa9983a3 100644 --- a/backend/services/quiz_config.py +++ b/backend/services/quiz_config.py @@ -90,12 +90,15 @@ def mastery_after(before: float, *, score: int, total: int) -> float: QUIZ_GENERATE_RATE_LIMIT = 8 QUIZ_GENERATE_RATE_WINDOW_SEC = 300 # 5 minutes -# Daily per-user LLM spend ceiling across ALL features (read off llm_usage, -# which agents/usage.py::record_agent_usage already writes). A generation on -# the default flash-lite tier costs well under a cent, so this is ~2 orders -# of magnitude above any real study day — it exists to bound a runaway, not -# to ration normal use. Deliberately fail-OPEN: if the usage read errors we -# serve the quiz rather than denying every student on a table blip. +# Daily per-user LLM spend ceiling. The SPEND it measures is cross-feature +# (llm_usage records every agent call, not just quiz ones), but the ceiling +# is only ENFORCED on quiz generation — the one unbounded LLM call behind a +# button. Other entry points stay unguarded for now; moving this into a +# shared guard is its own piece of work, not something to imply here. +# A generation on the default flash-lite tier costs well under a cent, so +# this is ~2 orders of magnitude above any real study day — it exists to +# bound a runaway, not to ration normal use. Deliberately fail-OPEN: if the +# usage read errors we serve the quiz rather than denying every student. QUIZ_DAILY_SPEND_CAP_USD = 2.00 # Wall-clock ceiling on one generation (agent run incl. its tool calls). diff --git a/backend/services/request_limits.py b/backend/services/request_limits.py index ac4ebd5c..03b36ddf 100644 --- a/backend/services/request_limits.py +++ b/backend/services/request_limits.py @@ -37,6 +37,20 @@ def check_rate_limit(key: str, *, limit: int, window_sec: int) -> int | None: return None +def refund_rate_limit(key: str) -> None: + """Give back the most recent slot recorded for `key`. + + For guards that must claim BEFORE doing the expensive work (so a + concurrent burst can't slip past the gate) but shouldn't charge the + caller when that work fails for reasons they didn't cause. Dropping + the newest timestamp — not the oldest — keeps the window's start + anchored to the caller's earliest real attempt. + """ + bucket = _rate_state.get(key) + if bucket: + bucket.pop() + + async def read_within_limit(upload: UploadFile, max_bytes: int) -> bytes: """Read at most ``max_bytes`` (+1 to detect overflow) from an UploadFile so an oversize upload can't be pulled fully into memory before we reject it. diff --git a/backend/tests/test_quiz_cost_observability_f.py b/backend/tests/test_quiz_cost_observability_f.py index 805ca530..8f508c26 100644 --- a/backend/tests/test_quiz_cost_observability_f.py +++ b/backend/tests/test_quiz_cost_observability_f.py @@ -90,6 +90,32 @@ def test_burst_past_the_limit_is_rejected(self): # The blocked request must not have reached the model. assert run.call_count == QUIZ_GENERATE_RATE_LIMIT + def test_a_failed_generation_does_not_burn_the_budget(self): + """The slot is claimed before the model runs, so a backend failure + would otherwise spend the student's quota — eight 502s in two + minutes locking them out for five, with a message telling them + they generated too many quizzes. They received none.""" + from services.quiz_config import QUIZ_GENERATE_RATE_LIMIT + + failing = AsyncMock(side_effect=RuntimeError("boom")) + with ( + patch("routes.quiz.table", side_effect=_factory()), + patch("routes.quiz.quiz_agent.run", new=failing), + ): + for _ in range(QUIZ_GENERATE_RATE_LIMIT + 2): + r = _generate() + assert r.status_code == 502, ( + "a failed generation must not consume the rate-limit slot" + ) + + # …and the budget is still intact for a request that can succeed. + with ( + patch("routes.quiz.table", side_effect=_factory()), + patch("routes.quiz.quiz_agent.run", + new=AsyncMock(return_value=SimpleNamespace(output=_quiz()))), + ): + assert _generate().status_code == 200 + def test_limit_is_per_user(self): run = AsyncMock(return_value=SimpleNamespace(output=_quiz())) from services.quiz_config import QUIZ_GENERATE_RATE_LIMIT @@ -133,6 +159,53 @@ def test_under_budget_passes(self): r = _generate() assert r.status_code == 200 + def test_spend_sum_is_not_truncated_by_the_postgrest_page_cap(self): + """PostgREST caps a response at max_rows (1000) and answers with + 206 — a 2xx — so an unpaged select silently truncates. Summing that + page plateaus below the ceiling, and the guard can never trip for + the runaway user it exists to stop.""" + from services.quiz_config import QUIZ_DAILY_SPEND_CAP_USD + + # Sized so ONE page is not enough: 1000 rows × $0.0015 = $1.50, + # under the $2.00 cap, while all 1500 rows = $2.25, over it. A + # reader that trusts a single truncated response concludes the + # user is fine; a paging reader catches them. + per_row = 0.0015 + rows = [{"cost_usd": per_row} for _ in range(1500)] + pages: list[tuple[int, int]] = [] + + def factory(name): + mock = MagicMock() + if name == "llm_usage": + def _select(columns="*", filters=None, limit=None, offset=None, **kw): + page_limit = min(limit or 1000, 1000) + start = offset or 0 + pages.append((start, page_limit)) + return rows[start:start + page_limit] + mock.select.side_effect = _select + elif name == "graph_nodes": + mock.select.return_value = [{ + "id": "node1", "course_id": "course1", + "concept_name": "Loops", "mastery_score": 0.5, + }] + else: + mock.select.return_value = [] + mock.insert.return_value = [{"id": "q"}] + return mock + + run = AsyncMock(return_value=SimpleNamespace(output=_quiz())) + with ( + patch("routes.quiz.table", side_effect=factory), + patch("routes.quiz.quiz_agent.run", new=run), + ): + r = _generate() + + assert len(pages) > 1, "the spend read must page, not trust one response" + assert r.status_code == 429 + assert r.json()["error"]["code"] == "QUIZ_DAILY_LIMIT_REACHED" + run.assert_not_called() + assert 1500 * per_row > QUIZ_DAILY_SPEND_CAP_USD # the premise + def test_spend_lookup_failure_never_blocks_a_quiz(self): """The guard is a cost control, not a correctness gate — if the usage table is unreachable we let the quiz through rather than @@ -181,6 +254,53 @@ async def _never(*a, **k): assert r.status_code == 502 assert r.json()["error"]["code"] == "QUIZ_GENERATION_TIMEOUT" + def test_a_partial_quiz_survives_a_top_up_timeout(self): + """The timeout must not throw away questions we already have. + Cancelling the whole generation raises CancelledError — a + BaseException — straight past #543's serve-what-we-have handler, + turning a valid 3-question quiz into a 502.""" + import asyncio as _asyncio + + from agents.quiz import Quiz, QuizQuestion + + def _mk(n, correct=True): + return QuizQuestion( + question=f"Q{n}?", type="multiple_choice", difficulty="easy", + options=[f"a{n}", f"b{n}", f"c{n}", f"d{n}"], + correct_answer=f"a{n}" if correct else "NOPE", + explanation="x", concept="Loops", + ) + + # 3 good, 3 drifted → drop rate high enough to trigger the top-up. + first = Quiz(questions=[_mk(1), _mk(2), _mk(3), + _mk(4, False), _mk(5, False), _mk(6, False)]) + calls = {"n": 0} + + async def _run(*a, **k): + calls["n"] += 1 + if calls["n"] == 1: + return SimpleNamespace(output=first) + await _asyncio.sleep(3600) # the top-up hangs + + with ( + patch("routes.quiz.table", side_effect=_factory()), + patch("routes.quiz.quiz_agent.run", new=_run), + patch("routes.quiz.QUIZ_GENERATION_TIMEOUT_SEC", 0.2), + ): + r = client.post("/api/quiz/generate", json={ + "user_id": "user_andres", + "concept_node_id": "node1", + "num_questions": 6, + "difficulty": "easy", + "use_shared_context": False, + }) + + assert r.status_code == 200, ( + "a completed partial generation must be served, not discarded" + ) + assert r.json()["delivered_count"] == 3 + assert calls["n"] == 2 # the top-up was attempted and timed out + def test_ordinary_failure_keeps_the_generic_code(self): with ( patch("routes.quiz.table", side_effect=_factory()),