Reader views perf: Stage 4 — Phase F cleanups bundled

codex/views/reader/_archive_cache.py

@@ -1,35 +1,39 @@
 """
-Process-wide cache of open Comicbox archives for the reader page endpoint.
-
-Codex runs Granian in single-worker embed mode (see ``codex/run.py``) so a
-single in-process LRU is shared by every request thread. Without this
-cache every page hit re-opens the archive — for a sequential read of an
-N-page comic that's N opens, and the web reader's prefetch (current +
-next + prev) plus the optional ``cacheBook`` setting (whole-book
-prefetch) compound the redundancy further (3-N concurrent opens of the
-same archive within seconds).
-
-The cache trades one process-wide concern (cap on memory + file
-descriptors held by open archives) for the much larger win of
-collapsing repeat opens. Each cached entry holds:
-
-* one open ``Comicbox`` instance — typically 1-3 MB resident for CBZ,
-  larger for PDF (libpoppler state).
-* one file descriptor.
-* a per-archive ``threading.Lock`` because ZipFile / RarFile / PDF
-  backends are NOT documented as thread-safe under concurrent
-  ``read`` calls. Extraction serializes per archive; the cache
-  structure itself is guarded by a separate short-held lock so
-  unrelated archives proceed in parallel.
-
-Configuration knobs (env vars; module-level defaults are conservative
-to suit the constrained-NAS deployment shape — see
-``tasks/reader-views-perf/stage3.md`` for telemetry-driven sizing
-rationale):
+Process-wide caches for the reader page endpoint.
+
+Two related caches live in this module — both keyed on a comic pk and
+both wired into ``ReaderPageView`` — sized independently because they
+guard different costs:
+
+1. ``ArchiveCache`` (``archive_cache``) — open ``Comicbox`` instances
+   keyed on file path. Without it, every page hit re-opens the
+   archive; a 200-page sequential read = 200 opens, and the web
+   reader's prev / curr / next prefetch + the opt-in ``cacheBook``
+   whole-book prefetch compound the redundancy. Per-archive
+   ``threading.Lock`` serializes extraction because ZipFile / RarFile
+   / PDF backends are not documented as thread-safe under concurrent
+   ``read`` calls; the cache structure itself is guarded by a separate
+   short-held lock so unrelated archives proceed in parallel.
+
+2. ``PageAclCache`` (``page_acl_cache``) — ``(auth_key, comic_pk) →
+   (path, file_type)``. Skips the per-page ACL-filter SQL within a
+   short TTL window during a single read-through (sub-plan 03 #2 /
+   Tier 4 #15).
+
+Codex runs Granian in single-worker embed mode (see ``codex/run.py``)
+so a single in-process LRU is shared by every request thread. Both
+caches' defaults suit the constrained-NAS deployment shape (1-2 GB
+RAM, ARM SBC etc.) — see ``tasks/reader-views-perf/stage3.md`` for
+the telemetry-driven sizing rationale.
+
+Configuration knobs (env vars):
 
 * ``CODEX_READER_ARCHIVE_CACHE_SIZE`` — max open archives (default 4).
 * ``CODEX_READER_ARCHIVE_CACHE_TTL`` — idle expiry in seconds (default 30).
 * ``CODEX_READER_ARCHIVE_CACHE_DISABLE`` — bypass entirely (default off).
+* ``CODEX_READER_PAGE_ACL_CACHE_SIZE`` — max ACL entries (default 64).
+* ``CODEX_READER_PAGE_ACL_CACHE_TTL`` — TTL in seconds (default 60).
+* ``CODEX_READER_PAGE_ACL_CACHE_DISABLE`` — bypass entirely (default off).
 """
 
 from __future__ import annotations
@@ -191,3 +195,75 @@ def shutdown(self) -> None:
 )
 
 atexit.register(archive_cache.shutdown)
+
+
+# ──────────────────────────────────────────────────────────────────────
+# Page-endpoint ACL decision cache (sub-plan 03 #2 / Tier 4 #15)
+# ──────────────────────────────────────────────────────────────────────
+#
+# A second, smaller cache keyed on ``(auth_key, comic_pk)`` →
+# ``(path, file_type)``. Sequential reads of an N-page comic hit the
+# page endpoint N times for the same ``(user, comic)``; without this
+# cache each one re-runs the ACL filter SQL just to fetch path +
+# file_type. Same trade-off as the archive cache (60 s TTL bounds
+# staleness on ACL revocation / comic deletion) but a separate
+# concern with separate sizing.
+
+_PAGE_ACL_DEFAULT_SIZE = 64
+_PAGE_ACL_DEFAULT_TTL = 60.0
+
+
+class PageAclCache:
+    """Process-wide LRU of (auth_key, comic_pk) → (path, file_type)."""
+
+    def __init__(
+        self,
+        max_entries: int = _PAGE_ACL_DEFAULT_SIZE,
+        ttl: float = _PAGE_ACL_DEFAULT_TTL,
+        *,
+        enabled: bool = True,
+    ) -> None:
+        self.max_entries = max_entries
+        self.ttl = ttl
+        self.enabled = enabled
+        self._lock = threading.Lock()
+        # Values stored as ``(path, file_type, expires_at)`` tuples.
+        self._cache: OrderedDict[tuple, tuple[str, str | None, float]] = OrderedDict()
+
+    def get(self, key: tuple, now: float) -> tuple[str, str | None] | None:
+        """Return cached ``(path, file_type)`` or ``None`` if missing/expired."""
+        if not self.enabled:
+            return None
+        with self._lock:
+            entry = self._cache.get(key)
+            if entry is None:
+                return None
+            path, file_type, expires_at = entry
+            if now >= expires_at:
+                self._cache.pop(key, None)
+                return None
+            self._cache.move_to_end(key)
+            return path, file_type
+
+    def put(self, key: tuple, path: str, file_type: str | None, now: float) -> None:
+        """Insert / refresh ``(path, file_type)`` for ``key``."""
+        if not self.enabled:
+            return
+        expires_at = now + self.ttl
+        with self._lock:
+            self._cache[key] = (path, file_type, expires_at)
+            self._cache.move_to_end(key)
+            while len(self._cache) > self.max_entries:
+                self._cache.popitem(last=False)
+
+    def clear(self) -> None:
+        """Drop every entry. Useful for tests."""
+        with self._lock:
+            self._cache.clear()
+
+
+page_acl_cache = PageAclCache(
+    max_entries=_env_int("CODEX_READER_PAGE_ACL_CACHE_SIZE", _PAGE_ACL_DEFAULT_SIZE),
+    ttl=float(_env_int("CODEX_READER_PAGE_ACL_CACHE_TTL", int(_PAGE_ACL_DEFAULT_TTL))),
+    enabled=not _env_bool("CODEX_READER_PAGE_ACL_CACHE_DISABLE", default=False),
+)

codex/views/reader/arcs.py

@@ -1,17 +1,17 @@
 """Reader get Arcs methods."""
 
-from datetime import UTC, datetime
 from functools import cached_property
 from typing import TYPE_CHECKING
 
+from django.db.models import Max
+
 from codex.choices.admin import AdminFlagChoices
 from codex.models import AdminFlag
 from codex.models.comic import Comic
 from codex.models.functions import JsonGroupArray
 from codex.models.named import StoryArc
 from codex.util import max_none
 from codex.views.const import (
-    EPOCH_START,
     STORY_ARC_GROUP,
 )
 from codex.views.reader.params import ReaderParamsView
@@ -20,8 +20,6 @@
     from collections.abc import Mapping
 
 _COMIC_ARC_FIELD_NAMES = ("series", "volume", "parent_folder")
-_STORY_ARC_ONLY = ("name", "ids", "updated_ats")
-_UPDATED_ATS_DATE_FORMAT_STR = "%Y-%m-%d %H:%M:%S.%f"
 
 
 class ReaderArcsView(ReaderParamsView):
@@ -85,28 +83,26 @@ def _get_story_arcs(self, comic: Comic, arcs, max_mtime: int | None):
         if not qs.exists():
             return max_mtime
 
+        # ``Max("updated_at")`` returns a single typed datetime per arc
+        # via the field's ``from_db_value`` hook — replaces the prior
+        # ``JsonGroupArray("updated_at")`` + per-row Python ``strptime``
+        # loop (sub-plan 01 #4 / Tier 3 #8). SQLite stores datetimes as
+        # ISO strings, so any aggregate that yields a typed datetime
+        # bypasses the manual parse.
         qs = qs.group_by("sort_name")  # pyright: ignore[reportAttributeAccessIssue]
         qs = qs.annotate(
             ids=JsonGroupArray("id", distinct=True, order_by="id"),
-            updated_ats=JsonGroupArray(
-                "updated_at", distinct=True, order_by="updated_at"
-            ),
+            mtime=Max("updated_at"),
         )
         qs = qs.order_by("sort_name").only("name")
 
         arcs[STORY_ARC_GROUP] = {}
 
         for sa in qs:
-            arc = {"name": sa.name}
             ids = tuple(sorted(set(sa.ids)))
-            updated_ats = (
-                datetime.strptime(ua, _UPDATED_ATS_DATE_FORMAT_STR).replace(tzinfo=UTC)
-                for ua in sa.updated_ats
-            )
-            mtime = max_none(EPOCH_START, *updated_ats)
-            arc["mtime"] = mtime
+            mtime = sa.mtime
+            arcs[STORY_ARC_GROUP][ids] = {"name": sa.name, "mtime": mtime}
             max_mtime = max_none(max_mtime, mtime)
-            arcs[STORY_ARC_GROUP][ids] = arc
         return max_mtime
 
     def _set_selected_arc(self, arcs) -> None:

codex/views/reader/page.py

@@ -1,5 +1,7 @@
 """Views for reading comic books."""
 
+import time
+
 from django.http import HttpResponse
 from drf_spectacular.types import OpenApiTypes
 from drf_spectacular.utils import OpenApiParameter, extend_schema
@@ -14,7 +16,7 @@
 from codex.settings import FALSY
 from codex.views.auth import AuthFilterAPIView
 from codex.views.bookmark import BookmarkAuthMixin
-from codex.views.reader._archive_cache import archive_cache
+from codex.views.reader._archive_cache import archive_cache, page_acl_cache
 
 _PDF_MIME_TYPE = "application/pdf"
 _PDF_FORMAT_NON_PDF_TYPES = frozenset(
@@ -45,15 +47,39 @@ def _update_bookmark(self) -> None:
         task = BookmarkUpdateTask(auth_filter, comic_pks, updates)
         LIBRARIAN_QUEUE.put(task)
 
-    def _get_page_image(self) -> tuple:
-        """Get the image data and content type."""
-        # ``.get(pk=...)`` collapses any duplicates an ACL JOIN might
-        # introduce; the explicit ``.distinct()`` on a single-row
-        # fetch is redundant (sub-plan 03 #6).
+    def _resolve_path_and_type(self, pk) -> tuple[str, str | None]:
+        """
+        Resolve ``(path, file_type)`` for the requested comic, ACL-filtered.
+
+        Caches the result per ``(auth_key, comic_pk)`` for a short TTL
+        so a sequential read-through doesn't fire the ACL filter SQL on
+        every page (sub-plan 03 #2 / Tier 4 #15). Cache misses fall
+        through to ``Comic.objects.filter(acl_filter).get(pk=pk)``;
+        ``.get`` collapses any duplicates an ACL JOIN might introduce,
+        making the explicit ``.distinct()`` on a single-row fetch
+        redundant (sub-plan 03 #6).
+        """
+        auth_filter = self.get_bookmark_auth_filter()
+        # ``auth_filter`` is one of ``{"user_id": pk}`` or
+        # ``{"session_id": key}``; flatten to a hashable tuple.
+        auth_key = next(iter(auth_filter.items()))
+        cache_key = (auth_key, pk)
+        now = time.monotonic()
+        cached = page_acl_cache.get(cache_key, now)
+        if cached is not None:
+            return cached
         acl_filter = self.get_acl_filter(Comic, self.request.user)
         qs = Comic.objects.filter(acl_filter).only("path", "file_type")
-        pk = self.kwargs.get("pk")
         comic = qs.get(pk=pk)
+        path = comic.path
+        file_type = comic.file_type
+        page_acl_cache.put(cache_key, path, file_type, now)
+        return path, file_type
+
+    def _get_page_image(self) -> tuple:
+        """Get the image data and content type."""
+        pk = self.kwargs.get("pk")
+        path, file_type = self._resolve_path_and_type(pk)
 
         # page_image
         page = self.kwargs.get("page")
@@ -69,14 +95,14 @@ def _get_page_image(self) -> tuple:
         # re-opens the archive (sub-plan 03 #1). The per-archive lock
         # held inside ``archive_cache.open(...)`` serializes extraction
         # because ZipFile / RarFile / PDF backends aren't thread-safe.
-        with archive_cache.open(comic.path) as cb:
+        with archive_cache.open(path) as cb:
             page_image = cb.get_page_by_index(page, pdf_format=pdf_format)
         if not page_image:
             page_image = b""
 
         # content type
         if (
-            comic.file_type == FileTypeChoices.PDF.value  # pyright: ignore[reportAttributeAccessIssue], # ty: ignore[unresolved-attribute]
+            file_type == FileTypeChoices.PDF.value  # pyright: ignore[reportAttributeAccessIssue], # ty: ignore[unresolved-attribute]
             and pdf_format not in _PDF_FORMAT_NON_PDF_TYPES
         ):
             content_type = _PDF_MIME_TYPE

codex/views/reader/settings.py

@@ -2,10 +2,8 @@
 
 from functools import cache
 from types import MappingProxyType
-from typing import TYPE_CHECKING
 
 from drf_spectacular.utils import extend_schema
-from loguru import logger
 from rest_framework.exceptions import ValidationError
 from rest_framework.response import Response
 from rest_framework.serializers import BaseSerializer
@@ -17,11 +15,9 @@
     ReaderScopedUpdateSerializer,
     ReaderSettingsSerializer,
 )
+from codex.views.bookmark import BookmarkAuthMixin
 from codex.views.settings import NULL_VALUES, SettingsBaseView
 
-if TYPE_CHECKING:
-    from rest_framework.request import Request
-
 # scope letter → (SettingsReader FK field, Comic FK for auto-resolve, Model for name)
 # "g" = global (no FK).  "c" = comic.
 # p/i/s/v all resolve to series scope.  "f" = folder.  "a" = story_arc.
@@ -55,7 +51,7 @@
 )
 
 
-class ReaderSettingsBaseView(SettingsBaseView):
+class ReaderSettingsBaseView(BookmarkAuthMixin, SettingsBaseView):
     """Reader settings — model config, defaults, reset, and scope lookups."""
 
     MODEL = SettingsReader
@@ -98,23 +94,15 @@ def reset_reader_settings(cls, instance: SettingsReader) -> dict:
         instance.save()
         return defaults
 
-    # ── Auth + scope lookups ────────────────────────────────────────
-    # (inlined from the former _ReaderSettingsAuthMixin / BookmarkAuthMixin)
-
-    def _get_bookmark_auth_filter(self) -> dict[str, int | str | None]:
-        """Filter only the current user's settings rows."""
-        if TYPE_CHECKING:
-            self.request: Request
-        if self.request.user.is_authenticated:
-            return {"user_id": self.request.user.pk}
-        if not self.request.session or not self.request.session.session_key:
-            logger.debug("no session, make one")
-            self.request.session.save()
-        return {"session_id": self.request.session.session_key}
+    # ── Scope lookups ───────────────────────────────────────────────
 
     def _get_settings_lookup(self, **extra):
         """Build the base lookup for a SettingsReader query."""
-        auth_filter = self._get_bookmark_auth_filter()
+        # ``get_bookmark_auth_filter`` from BookmarkAuthMixin returns the
+        # same {"user_id"|"session_id": ...} shape; consolidating here
+        # closes the duplicated copy that used to live on this class
+        # (sub-plan 02 #5 / Tier 4 #12).
+        auth_filter = self.get_bookmark_auth_filter()
         return {"client": ClientChoices.API, **auth_filter, **extra}
 
     @staticmethod

tasks/reader-views-perf/99-summary.md

@@ -84,19 +84,19 @@ land.
 
 | #   | Change | Sub-plan | Impact | Effort | Risk | Status |
 | --- | ------ | -------- | ------ | ------ | ---- | ------ |
-| 8   | **Replace `JsonGroupArray("updated_at")` + Python strptime loop with `Max("updated_at")`.** Story-arc mtime computation parses datetime strings per row in Python. SQL aggregation returns a single datetime per arc that Django converts via the field's `from_db_value`. | 01 #4 | Low (cleanup; small wins on heavily-tagged story-arc comics) | S | L | ⏳ Open |
+| 8   | **Replace `JsonGroupArray("updated_at")` + Python strptime loop with `Max("updated_at")`.** Story-arc mtime computation parses datetime strings per row in Python. SQL aggregation returns a single datetime per arc that Django converts via the field's `from_db_value`. | 01 #4 | Low (cleanup; small wins on heavily-tagged story-arc comics) | S | L | ✅ Stage 4 |
 | 9   | **Convert two-query get-or-create to `Model.objects.get_or_create`.** `_get_global_settings` and `_get_or_create_scoped_settings` both filter-then-create. Django ORM has the atomic primitive. | 02 #2 | Low (saves 1 query on cold-create paths) | S | L | ✅ Stage 0 |
 | 10  | **Cache `get_reader_default_params` at class load.** Pure model metadata; doesn't change at runtime. | 02 #4 | Trivial | XS | L | ✅ Stage 0 |
-| 11  | **Audit `_get_comics_list` annotation pyramid for prev/next dead fields.** Annotations applied to every row in the iteration — but prev/next entries don't need the same level of detail as current. Slimming the SELECT shrinks per-row I/O. | 01 #7 | Low | S | M | ⏳ Open |
+| 11  | **Audit `_get_comics_list` annotation pyramid for prev/next dead fields.** Annotations applied to every row in the iteration — but prev/next entries don't need the same level of detail as current. Slimming the SELECT shrinks per-row I/O. | 01 #7 | Low | S | M | ❌ Won't fix (superseded by Stage 1's `values_list("pk")` + `pk__in` window — annotation pyramid only fires for 1-3 rows now; slimming further would shave microseconds) |
 
 ### Tier 4 — clean-ups / small wins
 
 | #   | Change | Sub-plan | Impact | Effort | Risk | Status |
 | --- | ------ | -------- | ------ | ------ | ---- | ------ |
-| 12  | **De-duplicate `_get_bookmark_auth_filter` between `ReaderSettingsBaseView` and `BookmarkAuthMixin`.** Currently inlined in two places. | 02 #5 | Code health | S | L | ⏳ Open |
+| 12  | **De-duplicate `_get_bookmark_auth_filter` between `ReaderSettingsBaseView` and `BookmarkAuthMixin`.** Currently inlined in two places. | 02 #5 | Code health | S | L | ✅ Stage 4 |
 | 13  | **Pre-build `frozenset(arc_ids)` in `_set_selected_arc` once outside the loop.** Sub-plan 01 #6. Trivial. | 01 #6 | Trivial | XS | L | ✅ Stage 0 (also fixed a no-match correctness bug — see [stage0.md #13](stage0.md#13--pre-build-frozenset-in-_set_selected_arc)) |
 | 14  | **Drop redundant `.distinct()` on the page-endpoint comic queryset.** `.get(pk=pk)` LIMIT 1 collapses duplicates anyway. | 03 #6 | Trivial | XS | L | ✅ Stage 0 |
-| 15  | **Cache the (user, comic_pk) ACL decision** for the page endpoint. Per-process LRU keyed on the pair, 60-second TTL. Skips the per-page ACL check during a single read-through. | 03 #2 | Low-Medium (depends on ACL filter cost in profile) | S-M | M | ⏳ Open |
+| 15  | **Cache the (user, comic_pk) ACL decision** for the page endpoint. Per-process LRU keyed on the pair, 60-second TTL. Skips the per-page ACL check during a single read-through. | 03 #2 | Low-Medium (depends on ACL filter cost in profile) | S-M | M | ✅ Stage 4 (warm: 8 → 2 queries / 10 → 3.6 ms; -64%) |
 
 ### Tier 5 — high-risk / needs investigation before scheduling