diff --git a/CLAUDE.md b/CLAUDE.md index 2c7cfa73..38f14f0a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -75,6 +75,7 @@ ruff format . # formatter — available, not yet CI-gated (see - Display names are resolved via `services/profiles.py` (`get_display_name`/`get_display_names`), which decrypts off `user_profiles` — don't read name columns off `users`. - All current LLM calls route through `services/gemini_service.py` (`call_gemini`, `call_gemini_json`, `call_gemini_multiturn`). New LLM-driven code should be written as Pydantic AI agents in `backend/agents/` rather than extending `gemini_service.py`. - Knowledge-graph mutations go through `services/graph_service.py::apply_graph_update` — routes never write `graph_nodes`/`graph_edges` directly. +- `functools.lru_cache` is reserved for **deterministic, per-process reads** (#98) — either immutable mappings that never need invalidation (e.g. `academics.offering_course_id`) or reads with a matching `clear_*_cache()` hook that every mutator calls (e.g. `course_context_service.get_course_context` is cleared by `update_course_context`). Cache only hashable-arg functions; return a deep copy if the cached value is mutable; never cache without a clear invalidation story. The autouse `_clear_lru_caches` fixture in `tests/conftest.py` resets these between tests. - Backend tests live in `backend/tests/` and run via `pytest`; shared fixtures (mock Supabase, mock Gemini) are in `tests/conftest.py`. - Routers are mounted in `main.py` with `/api/` prefixes; new routes follow that pattern. diff --git a/backend/services/academics.py b/backend/services/academics.py index c4f08a6f..42a747a3 100644 --- a/backend/services/academics.py +++ b/backend/services/academics.py @@ -15,8 +15,10 @@ """ from __future__ import annotations +import copy import uuid from datetime import date +from functools import lru_cache from db.connection import table @@ -93,8 +95,13 @@ def resolve_offering( return any_off[0]["id"] if any_off else None +@lru_cache(maxsize=4096) def offering_course_id(offering_id: str) -> str | None: - """The abstract course id an offering belongs to (offering → graph bridge).""" + """The abstract course id an offering belongs to (offering → graph bridge). + + Cached per-process (#98): an offering's ``course_id`` is set at creation and + never changes, so this is a deterministic immutable mapping — no invalidation + hook needed. Returns an immutable ``str``/``None`` (safe to share).""" if not offering_id: return None rows = table("course_offerings").select( @@ -121,8 +128,8 @@ def user_offering_ids_for_course(user_id: str, course_id: str) -> list[str]: return [e["offering_id"] for e in enr if e.get("offering_id") in off_ids] -def term_for_offering(offering_id: str) -> dict | None: - """The term row for an offering (for semester labels).""" +@lru_cache(maxsize=4096) +def _term_for_offering_cached(offering_id: str) -> dict | None: if not offering_id: return None rows = table("course_offerings").select( @@ -137,6 +144,24 @@ def term_for_offering(offering_id: str) -> dict | None: return terms[0] if terms else None +def term_for_offering(offering_id: str) -> dict | None: + """The term row for an offering (for semester labels). + + Cached per-process (#98): the offering→term mapping is immutable and terms + are seeded reference data that don't change at runtime. Returns a deep copy + so callers can't mutate the shared cached row.""" + cached = _term_for_offering_cached(offering_id) + return copy.deepcopy(cached) if cached is not None else None + + +def clear_academics_caches() -> None: + """Clear the per-process academics caches. Called from test setup (so mocked + DB state doesn't leak across tests); rarely needed at runtime since the + cached mappings are immutable.""" + offering_course_id.cache_clear() + _term_for_offering_cached.cache_clear() + + def user_enrollment_ids(user_id: str) -> list[dict]: """The user's enrollments as ``{id, offering_id}`` rows (read + scoping helper).""" if not user_id: diff --git a/backend/services/course_context_service.py b/backend/services/course_context_service.py index 1f6a9f35..92c797f8 100644 --- a/backend/services/course_context_service.py +++ b/backend/services/course_context_service.py @@ -11,9 +11,11 @@ - offering_summary: class-wide summary with Gemini-generated text (per offering) """ +import copy import json import hashlib from datetime import datetime, timezone +from functools import lru_cache from db.connection import table from services.gemini_service import call_gemini @@ -65,14 +67,8 @@ def _generate_summary_with_gemini( ) -def get_course_context(offering_id: str) -> dict: - """ - Return the cached class context for an offering: summary + concept stats. - Offering-scoped (one class instance in one term). Returns {} if not found. - """ - if not offering_id: - return {} - +@lru_cache(maxsize=512) +def _get_course_context_cached(offering_id: str) -> dict: try: # Get the offering summary summary_rows = table("offering_summary").select( @@ -106,6 +102,26 @@ def get_course_context(offering_id: str) -> dict: return {} +def get_course_context(offering_id: str) -> dict: + """ + Return the cached class context for an offering: summary + concept stats. + Offering-scoped (one class instance in one term). Returns {} if not found. + + Cached per-process (#98) and invalidated by ``update_course_context`` (which + runs after graph updates). Returns a deep copy so callers can't mutate the + shared cached value. + """ + if not offering_id: + return {} + return copy.deepcopy(_get_course_context_cached(offering_id)) + + +def clear_course_context_cache() -> None: + """Drop the per-process course-context cache. Called whenever the underlying + aggregates change (``update_course_context``) and from test setup.""" + _get_course_context_cached.cache_clear() + + def update_course_context(offering_id: str) -> None: """ Aggregate mastery + quiz data for all students enrolled in an **offering** @@ -127,6 +143,7 @@ def update_course_context(offering_id: str) -> None: # No students enrolled — purge any stale aggregates table("offering_concept_stats").delete({"offering_id": f"eq.{offering_id}"}) table("offering_summary").delete({"offering_id": f"eq.{offering_id}"}) + clear_course_context_cache() # #98: aggregates changed → drop cached read return user_ids = [r["user_id"] for r in enrollment_rows] @@ -335,3 +352,6 @@ def _parse_quiz_context_to_arrays(ctx_rows: list) -> tuple[list, list, list]: }, on_conflict="offering_id", ) + + # #98: the aggregates this offering's context reads from just changed. + clear_course_context_cache() diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 4a9e3269..f0b9feee 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -32,6 +32,18 @@ def pytest_configure(config): ) +@pytest.fixture(autouse=True) +def _clear_lru_caches(): + """#98: reset the per-process lru_caches around every test so one test's + mocked DB state can't leak into another via a cached read.""" + from services import academics, course_context_service + academics.clear_academics_caches() + course_context_service.clear_course_context_cache() + yield + academics.clear_academics_caches() + course_context_service.clear_course_context_cache() + + @pytest.fixture(autouse=True) def _hermetic_supabase_client(request, monkeypatch): """Hermetic safety net (#210): no test may make a real Supabase call. diff --git a/backend/tests/test_lru_cache.py b/backend/tests/test_lru_cache.py new file mode 100644 index 00000000..63ee07f3 --- /dev/null +++ b/backend/tests/test_lru_cache.py @@ -0,0 +1,112 @@ +"""Tests for the per-process lru_caches (#98): academics resolvers and +course-context, including the invalidation hook. + +The autouse `_clear_lru_caches` fixture in conftest clears these before each +test, so each starts with an empty cache.""" +from unittest.mock import MagicMock, patch + +from services import academics, course_context_service + + +class TestOfferingCourseIdCache: + def test_second_call_served_from_cache(self): + calls = {"n": 0} + + def _table(name): + m = MagicMock() + def _select(*a, **k): + calls["n"] += 1 + return [{"course_id": "c1"}] + m.select.side_effect = _select + return m + + with patch("services.academics.table", side_effect=_table): + assert academics.offering_course_id("off1") == "c1" + assert academics.offering_course_id("off1") == "c1" + assert calls["n"] == 1 # only the first call hit the DB + + def test_distinct_offerings_not_conflated(self): + def _table(name): + m = MagicMock() + def _select(cols, filters=None, **k): + off = filters["id"].split(".")[-1] + return [{"course_id": f"c-{off}"}] + m.select.side_effect = _select + return m + + with patch("services.academics.table", side_effect=_table): + assert academics.offering_course_id("A") == "c-A" + assert academics.offering_course_id("B") == "c-B" + + +class TestTermForOfferingCache: + def _table(self, name): + m = MagicMock() + if name == "course_offerings": + m.select.return_value = [{"term_id": "t1"}] + elif name == "terms": + m.select.return_value = [{"id": "t1", "name": "Fall 2026"}] + else: + m.select.return_value = [] + return m + + def test_returns_defensive_copy(self): + with patch("services.academics.table", side_effect=self._table): + t1 = academics.term_for_offering("off1") + t1["name"] = "MUTATED" # caller mutates the returned dict + t2 = academics.term_for_offering("off1") + assert t2["name"] == "Fall 2026" # cache not corrupted + + +class TestCourseContextCacheInvalidation: + def _ctx_table(self, summary_text): + def _table(name): + m = MagicMock() + if name == "offering_summary": + m.select.return_value = [{ + "offering_id": "o1", "student_count": 1, + "avg_class_mastery": 0.5, "summary_text": summary_text, + "updated_at": "t", + }] + else: + m.select.return_value = [] + return m + return _table + + def test_cached_until_explicitly_cleared(self): + with patch("services.course_context_service.table", side_effect=self._ctx_table("v1")): + c1 = course_context_service.get_course_context("o1") + assert c1["course_summary"]["summary_text"] == "v1" + + # DB now returns v2, but the cache still serves v1 until cleared. + with patch("services.course_context_service.table", side_effect=self._ctx_table("v2")): + cached = course_context_service.get_course_context("o1") + assert cached["course_summary"]["summary_text"] == "v1" + course_context_service.clear_course_context_cache() + fresh = course_context_service.get_course_context("o1") + assert fresh["course_summary"]["summary_text"] == "v2" + + def test_update_course_context_invalidates(self): + # Prime the cache with v1. + with patch("services.course_context_service.table", side_effect=self._ctx_table("v1")): + course_context_service.get_course_context("o1") + + # update_course_context with no enrollments → purge path → cache_clear. + def _no_enrollments(name): + m = MagicMock() + m.select.return_value = [] + return m + with patch("services.course_context_service.table", side_effect=_no_enrollments): + course_context_service.update_course_context("o1") + + # Cache was invalidated, so the next read re-fetches (now v2). + with patch("services.course_context_service.table", side_effect=self._ctx_table("v2")): + after = course_context_service.get_course_context("o1") + assert after["course_summary"]["summary_text"] == "v2" + + def test_returns_defensive_copy(self): + with patch("services.course_context_service.table", side_effect=self._ctx_table("v1")): + a = course_context_service.get_course_context("o1") + a["course_summary"]["summary_text"] = "MUTATED" + b = course_context_service.get_course_context("o1") + assert b["course_summary"]["summary_text"] == "v1" # cache intact diff --git a/specs/98-lru-cache.md b/specs/98-lru-cache.md new file mode 100644 index 00000000..4dea1039 --- /dev/null +++ b/specs/98-lru-cache.md @@ -0,0 +1,48 @@ +# Spec: functools.lru_cache for hot deterministic reads (#98) + +## Scope + +Milestone #2 (perf). Per-process (per-worker) caching of hot, deterministic reads with a clear +invalidation story. Composes with the HTTP cache (#99) and a future cross-worker Redis layer (#97). + +## Requirements + +### R1 — Cache immutable academics resolvers (no invalidation needed) +- `services/academics.offering_course_id(offering_id)` — `@lru_cache`. An offering's `course_id` is set + at creation and never changes → deterministic immutable mapping; returns an immutable `str`/`None`. +- `services/academics.term_for_offering(offering_id)` — `@lru_cache` on the private body; the public + function returns a `copy.deepcopy` (mutable dict) so callers can't corrupt the cache. Offering→term is + immutable and terms are seeded reference data. +- `services/academics.clear_academics_caches()` clears both (for tests). + +### R2 — Cache course-context with an invalidation hook +- `services/course_context_service.get_course_context(offering_id)` — `@lru_cache` on the private body; + the public function returns a `copy.deepcopy`. +- `clear_course_context_cache()` is called from `update_course_context` on **every** write path (the + no-enrollment purge and the final upsert). `update_course_context` is the choke point that + `apply_graph_update` and the doc/grade post-rolls funnel through, so a graph/doc/grade change that + moves the aggregates always drops the stale cached read. + +### R3 — Test isolation +- An autouse `_clear_lru_caches` fixture in `tests/conftest.py` clears all these caches around every test + so one test's mocked DB state can't leak into another via a cached read. + +### R4 — Docs +- CLAUDE.md Conventions: `lru_cache` is reserved for deterministic per-process reads — immutable mappings + (no invalidation) or reads with a matching `clear_*_cache()` every mutator calls; hashable args only; + deep-copy mutable returns. + +## Acceptance +1. 3+ functions cached, each provably safe (immutable, or hooked + deep-copied). +2. `clear_course_context_cache()` invoked from `update_course_context`'s write paths. +3. CLAUDE.md documents the convention. +4. Tests: cache hit avoids a 2nd DB read; distinct keys not conflated; deep-copy immunity; mutate via + `update_course_context` → next read returns fresh. +5. `pytest tests/ -q` shows no new failures vs `main` (proves the conftest fixture stops cache leakage); + `ruff` clean. + +## Deliberately NOT cached (documented risk) +- Graph reads (`get_graph`) — large mutable structures with hot, per-turn invalidation; deep-copy cost + + invalidation surface outweigh the win here. Revisit with the Redis layer (#97). +- Token decode / `require_self` — security-sensitive and needs a TTL bounded to token lifetime; out of + scope for a plain `lru_cache`.