From c564e317ed38a96bcfd04ae3f3afe0f0a7c699e2 Mon Sep 17 00:00:00 2001 From: ducoterra Date: Mon, 31 Aug 2026 01:35:28 -0400 Subject: [PATCH] =?UTF-8?q?fix(web):=20never=20304=20a=20rewritten=20page?= =?UTF-8?q?=20=E2=80=94=20pages=20drop=20conditional=20validators,=20asset?= =?UTF-8?q?s=20keep=20them?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../todo/53_stale_saved_chats/00_phase.md | 56 ---- README.md | 45 ++- app/core/caching.py | 96 +++++-- tests/e2e/test_asset_cache_revalidation.py | 158 +++++++++++ .../integration/test_caching_revalidation.py | 200 +++++++++++++ tests/unit/test_caching.py | 268 +++++++++++++++++- 6 files changed, 742 insertions(+), 81 deletions(-) delete mode 100644 .agent/phases/todo/53_stale_saved_chats/00_phase.md create mode 100644 tests/e2e/test_asset_cache_revalidation.py create mode 100644 tests/integration/test_caching_revalidation.py diff --git a/.agent/phases/todo/53_stale_saved_chats/00_phase.md b/.agent/phases/todo/53_stale_saved_chats/00_phase.md deleted file mode 100644 index 9c2e3cb..0000000 --- a/.agent/phases/todo/53_stale_saved_chats/00_phase.md +++ /dev/null @@ -1,56 +0,0 @@ -# Phase 53 — Invalidate Saved Chats on Sources Sync - -**Source:** `TODO.md` L4 — "Make sure the saved chats are invalidated if the docs are synced, that way it generates a new answer with new data" -**Story:** n/a (TODO-derived — owner instruction 2026-08-30: convert without confirmation) -**Context:** Phase 50 stores explicitly saved conversations in `saved_chats` (JSONB `bor.chat.v1` records; admin-only CRUD under `/api/chats` in `app/api/chats.py`; the History page is a full-width table per AGENTS.md rule 5; `/?chat=` re-opens a row pixel-identical through the phase-14 restore path). Phase 51 added the public snapshot read (`/shared/` → `SharedChatOut`, no admin dependency). Sources are synced through two canonical paths: the admin **Sync** button (`POST /api/sync` → `app/api/sync.py::_run_sync`: `check_models` → resolve effective sources → clone/pull or local-dir re-verify → `import_sources(prune=True)` → change-gated `regenerate_overview`) and the CLI/quadlet `scripts/import_docs.py` (same `import_sources`; `--limit` debug runs and unchanged re-imports are change-gated on `added + updated`). Neither path records *when the KB last changed*, so a saved answer can silently predate the current index. `retryLastTurn(wrap)` in `frontend/assets/app.js` (phase 49) is the existing redo-in-place mechanism for the LAST brain bubble (re-asks the preceding user question, `runTurn(text, { reask: true })`, no user append, no scroll) — the Regenerate action reuses it. Single-row table precedent: `kb_overview` (id = 1, `app/models.py::KbOverview`). - -## Objective -Every sync that actually changes the knowledge base bumps a sources version; saved chats stamp that version at save time; a chat saved against an older version is surfaced as **stale** (History table badge + a banner when opened) with a **Regenerate** action that re-asks the last question against the new index and re-saves the row — a stale answer can no longer masquerade as current. - -## Dependencies -- `50_chat_history` (complete) — the `saved_chats` row, `/api/chats` CRUD, the `?chat=` boot load, the linked-row Save upsert (`saveCurrentChat` in `app.js`). -- `51_share_chat` (complete) — the public `/shared/` snapshot read (stays a frozen snapshot; untouched by this phase). -- `49_retry_answer` (complete) — the `retryLastTurn` redo-in-place the Regenerate action drives. -- `52_pinned_composer` (todo, preceding) — the chat-page layout is stable while the banner lands; ordering keeps the chat UI quiet (no shared-file conflict beyond `styles.css`/`app.js` regions). - -## Tasks -1. `01_sources_version_and_migration.md` — the single-row `sources_meta` table + `saved_chats.sources_version` (migration 0010) + the current/bump helpers + the migration test. -2. `02_sync_version_bump.md` — the version bump on both sync paths (Sync button + CLI), change-gated. -3. `03_chats_api_staleness.md` — stamp-on-save + the `stale` flag on the list/detail responses. -4. `04_history_stale_badge.md` — the Stale column on the History table. -5. `05_stale_banner_and_regenerate.md` — the chat-page banner + Regenerate (`retryLastTurn` reuse) + the auto re-save. -6. `06_e2e_stale_saved_chats.md` — the story Playwright suite + regressions + commit. - -## Testing & Quality -- Unit: `tests/unit/test_sources_meta.py` (helpers: absent row → 0, first bump → 1, second bump → 2, idempotent reads); the sync bump gates — extend the existing sync coverage (`tests/integration/test_sync_api.py` + the `tests/fakes.py` override patterns) and the CLI coverage (`tests/integration/test_import_docs_overview.py` pins the change-gating pattern; the bump asserts sit alongside). -- Integration: `tests/integration/test_migration_0010.py` (house pattern, from `test_migration_0009.py`); extend `tests/integration/test_chats_api.py` (stamp + `stale` flag + share-unshare version immunity). -- Frontend source pins (house style): `history.js` stale-cell branch, `app.js` banner reveal / Regenerate wiring / post-regenerate persist / no-brain-bubble guard (the `test_history_page.py` / `test_save_chat_ui.py` pin patterns). -- Coverage: **>90%** on `app/` (validate.sh gate). -- E2E (mandatory, A16): `tests/e2e/test_stale_saved_chats.py`, run in isolation. - -## Completion Criteria -- [ ] A KB-changing sync (button or CLI) bumps `sources_meta.version` exactly once; an unchanged re-run, a `--limit` debug run, and a FAILED sync never bump. -- [ ] A Save/Re-Save stamps the row's `sources_version`; `GET /api/chats` + `GET /api/chats/` expose `stale` (true iff the row's version is behind the current one); share/unshare never touch the version. -- [ ] The History table shows the Stale marker exactly on rows saved before the last KB-changing sync (full-width table geometry unchanged, AGENTS.md rule 5). -- [ ] Opening a stale `/?chat=` shows the banner; Regenerate re-streams the last answer in place against the new index and re-saves the row — the banner clears, `GET /api/chats/` reports `stale: false`, the History marker is gone. -- [ ] A `/shared/` page is unchanged — the public snapshot carries no staleness surface. -- [ ] `uv run pytest` green; coverage TOTAL >90%. -- [ ] `uv run pytest tests/e2e/test_stale_saved_chats.py -v --no-cov` green in isolation (DB up). -- [ ] Regression E2E suites green in isolation: `test_chat_history.py`, `test_share_chat.py`, `test_sync_button.py`, `test_retry_answer.py`, `test_chat_persistence.py`. -- [ ] `uv run ruff check . && uv run pyright` clean. -- [ ] One `--no-gpg-sign` commit; phase dir moved to `.agent/phases/complete/`. - -## Locked decisions -- **Recorded assumptions (TODO conversion, 2026-08-30 — owner asked for no confirmation):** - 1. The invalidation marker is a monotonically increasing `sources_meta.version` (single row, the `kb_overview` id=1 precedent), bumped ONLY when a sync changed the KB — the gate is `added + updated + pruned > 0` (pruned counts: a deleted doc can invalidate an answer that cited it — deliberately broader than the overview gate's `added + updated > 0`). - 2. `stale = row.sources_version < current`, computed server-side in the chats API; the client never computes staleness. Pre-existing rows stamp 0 (the pre-counter KB) and go stale on the first bump. - 3. Regenerate = the phase-49 redo-in-place of the LAST brain bubble (re-ask the last user question with the full conversation context) followed by an auto re-save of the linked row — the owner does not press Save again; a stale chat with no brain answer shows the banner text without a Regenerate button. - 4. The public shared snapshot (`/shared/`) is deliberately untouched — a frozen snapshot by design; an owner who regenerates can re-share afterwards. - 5. A failed sync (git/embed/import error) aborts BEFORE the bump — a failed sync never invalidates chats; the bump commits even if the best-effort overview regeneration then fails (the index really did change). -- **A10 honoured** — `/api/chat` stays stateless; staleness is a property of the explicitly saved row, not of the chat endpoint. -- **A16/A17 honoured** — one story E2E suite, one atomic commit. - -## Commit -```bash -git add -A .agent/ app/ alembic/versions/ scripts/ frontend/ tests/ && git commit --no-gpg-sign -m "feat(chat): invalidate saved chats on sources sync — versioned stamps, stale marker, Regenerate against the new index" -``` diff --git a/README.md b/README.md index 1d2a63f..d988af8 100644 --- a/README.md +++ b/README.md @@ -509,16 +509,23 @@ refresh** (the "the pages are too sticky" problem, phase 33). One Starlette middleware (`app/core/caching.py`) applies the rule at the transport layer: -- **HTML pages always revalidate.** Every page (`/`, `/sources.html`, - `/document.html`, `/login.html`, `/tuning.html`) ships - `Cache-Control: no-cache`, so each visit re-checks the page with the - server — a page never lingers in the browser's cache unchecked. +- **HTML pages always revalidate — and never 304.** Every page + (`/`, `/index.html`, `/sources.html`, `/document.html`, `/login.html`, + `/tuning.html`, `/git-sources.html`, `/history.html`, `/shared.html`, + plus the dynamic share page `/shared/`) ships + `Cache-Control: no-cache`, **no `etag`, no `last-modified`**, so each + visit re-checks the page with the server and always gets a fresh 200 + body — a page never lingers in the browser's cache unchecked, and a + revalidation can never be answered "not modified" (see *Cache busting + below* for why). - **Assets are versioned and cached for a year.** The pages reference their CSS/JS with a token (`/assets/styles.css?v=`), and every `/assets/*` response ships `Cache-Control: public, max-age=31536000, immutable`. The token is what identifies the content, so long-term caching is safe: a new token means - a new URL, which the browser fetches fresh. + a new URL, which the browser fetches fresh. A conditional `GET` on a + versioned asset URL may still be answered `304` — the URL already + encodes the version, so that is safe. - **The token is the deploy.** In a git checkout (the normal case) it is the short SHA of `HEAD` (`git rev-parse --short HEAD`), computed once per process start — so **every commit/deploy flips the token** and the @@ -531,7 +538,7 @@ transport layer: endpoint's own `Cache-Control: no-cache` is set by the endpoint itself. No CDN, no new services, no build-step change: the middleware rewrites -the asset references of the five known pages in flight. The unversioned +the asset references of the known pages in flight. The unversioned asset paths keep working too (the static mount ignores the query string), so old tabs and direct links to `/assets/…` still resolve. @@ -540,6 +547,32 @@ so old tabs and direct links to `/assets/…` still resolve. > requesting the versioned assets; every commit after that is picked up > automatically. +### Cache busting: why pages never 304 (phase 54) + +The `?v=` token flips on the **next process start** — a +commit is a deploy, so the restarted server's HTML references new asset +URLs, and the browser fetches them fresh into its year-long asset cache. + +- **`/assets/*`** is cached `immutable` for a year *under the versioned + URL* — a conditional `GET` may 304, because the URL already encodes + the version. +- **HTML pages** are served `no-cache` and **never 304**, publishing no + `etag` / `last-modified`. The reason: the page body the browser + receives is *rewritten per process* (its asset refs gain + `?v=`), so the static file's upstream validators would describe + the *file*, not the *bytes served* — a conditional `GET` matching them + would 304 out of the rewrite and leave the browser on HTML pointing at + the **previous** commit's CSS/JS, which the year-long asset cache + serves until a hard reload (the hole measured in phase 54). Pages + therefore revalidate against the bytes actually served: always a full + 200. + +**Local development:** a `git` commit changes the token on the next +server restart. If a browser still shows an old layout after a restart, +hard-reload once (`Ctrl/Cmd-Shift-R`) — the phase-54 fix guarantees the +*next* navigation is current, but it cannot un-pin a document that a +pre-54 deploy already 304'd into the browser's cache. + ## Checking retrieval quality Ask the *real* pipeline (live aipi embeddings + the current KB) whether a diff --git a/app/core/caching.py b/app/core/caching.py index 7eef10f..72e9cbd 100644 --- a/app/core/caching.py +++ b/app/core/caching.py @@ -7,12 +7,27 @@ Two layers, one module: * **Response middleware** (``CachingMiddleware`` / ``configure_caching``) — applies the caching behavior at the transport layer: the known HTML pages (``HTML_PAGES``) and the dynamic share page - ``/shared/`` (phase 51) are always revalidated (``no-cache``) - and their local asset references are rewritten to carry + ``/shared/`` (phase 51) are always revalidated with a full + 200 body (``no-cache``, never a 304) publishing no validators, and + their local asset references are rewritten to carry ``?v=``; ``/assets/*`` is served ``immutable`` for a year; everything else — all of ``/api/*``, including the SSE chat stream — passes through byte-identical. +Why pages must never 304 (phase 54): the static file's validators +(``etag`` / ``last-modified``) describe the *file*, but the bytes the +browser receives are the *rewritten body* this process built from its +own token. A conditional request that matched those validators would +304 out of the rewrite — the browser keeps the HTML it already has, +whose ``?v=`` points at the previous commit's CSS/JS, which are cached +``immutable`` for a year. So on page paths the middleware strips +``if-none-match`` / ``if-modified-since`` from the inbound request +(before the downstream ``FileResponse`` can act on them) and drops the +outbound validators. The asymmetry is deliberate: a 304 on +``/assets/*`` is safe because the URL itself encodes the version, while +a 304 on an HTML page is never safe — the served body depends on the +process token, which the validator ignores. + The token is computed **once per process** (``functools.cache``, i.e. ``lru_cache(maxsize=None)``) — zero per-request git/file cost. It changes when a new commit lands (git path) or the frontend tree's mtimes/sizes @@ -40,6 +55,7 @@ import subprocess from pathlib import Path from fastapi import FastAPI +from starlette.datastructures import MutableHeaders from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint from starlette.requests import Request from starlette.responses import Response @@ -153,6 +169,17 @@ HTML_CACHE_CONTROL = "no-cache" _ASSET_REF_RE = re.compile(r'((?:href|src)="(?:/)?assets/[^"?#]+)(")') +def _is_known_page(path: str) -> bool: + """One source of truth for the known-page contract (phase 54). + + True for ``HTML_PAGES`` and for the dynamic share page + ``/shared/`` (phase 51) by path prefix. Shared by the + inbound conditional-header strip and the outbound rewrite branch so + the two can never drift apart. + """ + return path in HTML_PAGES or path.startswith("/shared/") + + def rewrite_asset_refs(html: str, token: str) -> str: """Append ``?v=`` before the closing quote of every local ``assets/…`` ``href``/``src`` reference. @@ -186,24 +213,33 @@ async def _read_body(response: Response) -> bytes: def _no_cache_headers(response: Response) -> dict[str, str]: """Copy the original headers, drop the stale ``content-length`` (the - body size changes on rewrite), and force ``Cache-Control: no-cache``.""" + body size changes on rewrite), drop the upstream validators — + ``etag`` / ``last-modified`` describe the static file, not the + rewritten body, so a page response must never be revalidated against + them later (phase 54) — and force ``Cache-Control: no-cache``.""" headers = {k: v for k, v in response.headers.items()} headers.pop("content-length", None) + for name in ("etag", "last-modified"): + headers.pop(name, None) headers["Cache-Control"] = HTML_CACHE_CONTROL return headers class CachingMiddleware(BaseHTTPMiddleware): - """Transport-layer cache busting (phase 33). + """Transport-layer cache busting (phase 33, revalidation fix phase 54). Touches exactly two response shapes: * ``/assets/*`` — ``Cache-Control: public, max-age=31536000, immutable`` - (header only — the body is never read). + (header only — the body is never read; conditional requests may + still 304, which is safe because the URL carries ``?v=``). * the known HTML pages (``HTML_PAGES``) plus the dynamic share page - ``/shared/`` (phase 51, by path prefix) — - ``Cache-Control: no-cache``, and (for ``text/html`` bodies) every - local asset reference gains ``?v=``. + ``/shared/`` (phase 51, by path prefix) — always revalidated + with a full 200 body (no 304), publishing no validators: the inbound + ``if-none-match`` / ``if-modified-since`` are stripped before the + downstream app runs, ``Cache-Control: no-cache`` is set, the outbound + ``etag`` / ``last-modified`` are dropped, and (for ``text/html`` + bodies) every local asset reference gains ``?v=``. Everything else — all of ``/api/*`` (including the SSE chat stream) — passes through byte-identical: no header changes, the body stream is @@ -213,24 +249,50 @@ class CachingMiddleware(BaseHTTPMiddleware): async def dispatch( self, request: Request, call_next: RequestResponseEndpoint ) -> Response: - response = await call_next(request) path = request.url.path + # Phase 54: on known page paths, strip the conditional request + # headers BEFORE the downstream app runs. The 304 is produced + # downstream (Starlette's ``FileResponse``/``StaticFiles`` honours + # ``If-None-Match`` / ``If-Modified-Since`` before this middleware + # sees a response), so a validator match would return a bodiless + # 304 whose body can never be rewritten to carry the current + # ``?v=`` refs — the browser would keep HTML pointing at + # the previous commit's assets, immutable-cached for a year. + # ``MutableHeaders`` mutates ``request.scope["raw_headers"]`` in + # place, so the downstream app only ever sees the full 200. The + # strip is scoped to page paths only — ``/assets/*`` 304s are safe + # (the URL is versioned) and ``/api/*`` (incl. the SSE stream) + # must stay byte-identical. + if _is_known_page(path): + headers = MutableHeaders(scope=request.scope) + for name in ("if-none-match", "if-modified-since"): + if name in headers: + del headers[name] + + response = await call_next(request) + if path.startswith(ASSETS_PREFIX): response.headers["Cache-Control"] = ASSET_CACHE_CONTROL return response - # Phase 51: the dynamic share page — ``/shared/`` is a - # REAL route (not a static file) serving ``shared.html``, so it - # joins the known-page contract by path prefix: no-cache + - # ``?v=`` asset rewrite. (``/api/shared/`` — the JSON - # read — starts with ``/api/`` and passes through below.) - is_known_page = path in HTML_PAGES or path.startswith("/shared/") - - if not is_known_page: + if not _is_known_page(path): # /api/* (incl. SSE), /favicon.ico, unknown paths: untouched. return response + # Phase 54: a bodiless downstream status (204/304) on a page + # path is NEVER rewritten — starlette forbids a body on those + # statuses, and the rewrite path would build exactly that. + # Belt-and-braces: after the inbound conditional-header strip, + # StaticFiles cannot 304 these paths; a future route or proxy + # could still, so pass through with no-cache and no validators. + if response.status_code in (204, 304): + response.headers["Cache-Control"] = HTML_CACHE_CONTROL + for name in ("etag", "last-modified"): + if name in response.headers: + del response.headers[name] + return response + content_type = response.headers.get("content-type", "") if content_type.split(";", 1)[0].strip().lower() != "text/html": # A page path with a non-HTML body (e.g. the 404 JSON served diff --git a/tests/e2e/test_asset_cache_revalidation.py b/tests/e2e/test_asset_cache_revalidation.py new file mode 100644 index 0000000..13f1541 --- /dev/null +++ b/tests/e2e/test_asset_cache_revalidation.py @@ -0,0 +1,158 @@ +"""Phase 54 E2E (Playwright): the browser can never be 304'd onto stale +HTML. + +Run in isolation (DB must be up: ``podman compose up -d db``): + + uv run pytest tests/e2e/test_asset_cache_revalidation.py -v --no-cov + +Phase 33's cache busting had a hole (measured 2026-08-30): the HTML page +response carried the *static file's* ``etag`` / ``last-modified``, while +the *served bytes* were the per-process rewritten body — so a conditional +revalidation 304'd out of the rewrite and the browser kept HTML whose +``?v=`` pointed at the previous commit's CSS/JS, immutable-cached for a +year. Phase 54 closed the hole: on known page paths the middleware strips +``if-none-match`` / ``if-modified-since`` inbound and drops the outbound +validators — a page always 200s with the current ``?v=`` refs. +``/assets/*`` keeps its immutable + validator behavior (a 304 there is +safe — the URL encodes the version). + +These assertions are against a real browser: the document it receives is +always the current one (200, current token, no validators), the CSS that +actually renders is this tree's, and the assets stay immutable. The suite +only loads pages — it asks no questions, so no KB seeding is needed. The +E2E app is a uvicorn subprocess of the SAME checkout (``conftest.py`` → +``app_server``), so the expected token is deterministic: the git short +SHA of this checkout. +""" +from __future__ import annotations + +import subprocess +from pathlib import Path + +from playwright.sync_api import Page, Response + +REPO = Path(__file__).resolve().parents[2] + + +def _expected_token() -> str: + """The version token the app process appends to its asset URLs. + + The E2E app is a subprocess of this very checkout, and in a git + checkout ``asset_version()`` is ``git rev-parse --short HEAD`` — + computed exactly the way the app does it, so the expected value is + deterministic for this run. + """ + return subprocess.run( + ["git", "-C", str(REPO), "rev-parse", "--short", "HEAD"], + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + +def _assert_current_document(resp: Response, token: str) -> None: + """The page-contract assertions (phase 54): 200 with the current + ``?v=`` references, no validators, ``no-cache`` — so the + browser can never revalidate its way back to a previous commit's + HTML. + + ``resp`` is the main document's ``Response`` returned by + ``page.goto()`` — Playwright Python treats a bare string in + ``expect_response(...)`` as a URL glob, so the navigation return + value is the assertion target for the document itself.""" + assert resp.status == 200, f"the document must 200 (never 304): {resp.status}" + headers = resp.headers + assert "etag" not in headers, "a page response must publish no etag" + assert "last-modified" not in headers, ( + "a page response must publish no last-modified" + ) + assert headers["cache-control"] == "no-cache" + body = resp.text() + assert f"styles.css?v={token}" in body, ( + f"the served HTML must reference the current token's CSS " + f"(styles.css?v={token})" + ) + + +def test_document_is_200_current_token_no_validators( + page: Page, app_url: str, db_ready: None +) -> None: + """`GET /` through a real browser: 200, no `etag`/`last-modified`, + `no-cache`, and the body references the current commit's CSS and JS. + On the pre-fix app a browser that has seen the page once gets a 304 + here instead — this is the suite's core assertion.""" + token = _expected_token() + assert token, "the version token must be non-empty" + + resp = page.goto(app_url) + assert resp is not None + _assert_current_document(resp, token) + # The chat page's two local refs, both carrying the current token. + assert f"app.js?v={token}" in resp.text() + + +def test_second_navigation_is_still_200_not_304( + page: Page, app_url: str, db_ready: None +) -> None: + """The exact shape of the reported symptom: a plain re-navigation. + After the first load the browser's cache holds the first document — + the second `goto` must still receive a fresh 200 with the current + token, not a 304 that keeps the previous commit's ``?v=``.""" + token = _expected_token() + assert token + + page.goto(app_url) # first visit — primes the browser's document cache + resp = page.goto(app_url) # the browser's own revalidation + assert resp is not None + _assert_current_document(resp, token) + + +def test_browser_renders_current_css_not_stale( + page: Page, app_url: str, db_ready: None +) -> None: + """End-to-end consequence of the 304 hole: the CSS the browser is + ACTUALLY rendering is the current tree's. The phase-52 rule + (`.messages { flex: 1 1 auto }` → computed `flex-grow: 1`) exists only + in the current `styles.css`, so a green here proves the browser is + not sitting on a stale, immutable-pinned asset from an earlier + commit.""" + page.goto(app_url) + flex_grow = page.evaluate( + "() => getComputedStyle(document.querySelector('#messages')).flexGrow" + ) + assert flex_grow == "1", ( + f"#messages must carry the current tree's `flex: 1 1 auto` " + f"(flex-grow 1) — got {flex_grow!r}; the browser is on stale CSS" + ) + + +def test_other_pages_carry_the_contract( + page: Page, app_url: str, db_ready: None +) -> None: + """/sources.html (another HTML_PAGES entry): the same document + contract as `/` — 200, no validators, `no-cache`, current token.""" + token = _expected_token() + assert token + + resp = page.goto(app_url + "/sources.html") + assert resp is not None + _assert_current_document(resp, token) + + +def test_assets_still_immutable_with_validators( + page: Page, app_url: str, db_ready: None +) -> None: + """The asymmetry is deliberate: `/assets/*` keeps its validators — + a conditional `GET` on a versioned asset URL may still 304, because + the URL already encodes the version. Only the HTML pages dropped + them.""" + with page.expect_response("**/assets/styles.css**") as info: + page.goto(app_url) + resp = info.value + assert resp.status == 200 + assert ( + resp.headers["cache-control"] == "public, max-age=31536000, immutable" + ) + assert "etag" in resp.headers, ( + "the asset keeps its validators — only pages dropped them" + ) diff --git a/tests/integration/test_caching_revalidation.py b/tests/integration/test_caching_revalidation.py new file mode 100644 index 0000000..8024a28 --- /dev/null +++ b/tests/integration/test_caching_revalidation.py @@ -0,0 +1,200 @@ +"""Integration: the phase-54 revalidation contract against the REAL app. + +Phase 33's two cache-busting layers (``asset_version()`` + +``CachingMiddleware``) rewrite the known HTML pages to carry +``?v=`` asset references — but the ``etag`` / ``last-modified`` +validators Starlette publishes for a page describe the STATIC FILE, not +the rewritten body this process built from its own token. A conditional +GET that matched those validators used to 304 out of the rewrite: the +browser kept the HTML it already had, whose ``?v=`` pinned the PREVIOUS +commit's CSS/JS — cached ``immutable`` for a year. This suite pins the +fix end-to-end (real ``app.main:app``, real ``StaticFiles`` mount on the +real ``frontend/`` tree, real ``asset_version()`` token — no mocks): + +* every known page (``HTML_PAGES``) AND the dynamic ``/shared/`` + page 200s on a conditional GET, always with the current + ``?v=`` body and no validators; +* ``/assets/*`` is untouched: ``immutable`` for a year, validators + intact, a conditional GET on the versioned URL still 304s (that 304 + is safe — the URL itself carries the version); +* ``/api/*`` stays byte-identical: no ``cache-control`` injected, no + validators, conditional headers pass through (the SSE chat stream's + pass-through is pinned by ``test_chat_api.py`` — the regression run + below). + +Requires: podman compose up -d db +""" +from __future__ import annotations + +import os +import re +from collections.abc import Iterator +from pathlib import Path + +import httpx +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import text +from sqlalchemy.orm import Session + +from app.core.caching import ( + ASSET_CACHE_CONTROL, + HTML_CACHE_CONTROL, + HTML_PAGES, + asset_version, +) + +REPO = Path(__file__).resolve().parents[2] +FRONTEND = REPO / "frontend" + + +def _token_ref_re(token: str) -> re.Pattern[str]: + """A local ``assets/…`` href/src reference that already carries + ``?v=`` (the middleware's rewrite output).""" + return re.compile(r'(?:src|href)="(?:/)?assets/[^"?#]*\?v=' + re.escape(token) + r'"') + + +def file_validators(page_file: Path) -> tuple[str, str]: + """The etag / last-modified Starlette would stamp on the underlying + static file — exactly what a browser revalidates against. + + ``stat_result`` is passed up front: starlette 1.x's ``FileResponse`` + defers the stat to ``__call__``, so without it the headers carry no + validators yet (same pattern as the unit suite's ``_file_validators``). + """ + from starlette.responses import FileResponse + + headers = FileResponse(page_file, stat_result=os.stat(page_file)).headers + return headers["etag"], headers["last-modified"] + + +def _page_file(path: str) -> Path: + """The static file backing a page path (``/`` → ``index.html``).""" + name = path.lstrip("/") or "index.html" + file = FRONTEND / name + assert file.is_file(), f"missing page file for {path}: {file}" + return file + + +def _assert_page_contract(response: httpx.Response, token: str) -> None: + """The phase-54 page contract on any known page: a full 200 with the + CURRENT process token on the asset refs, ``Cache-Control: no-cache``, + and NO validators (a page must never be revalidated against a + validator this process published).""" + assert response.status_code == 200, ( + f"a page path must never 304 (got {response.status_code})" + ) + assert response.headers["cache-control"] == HTML_CACHE_CONTROL + assert "etag" not in response.headers + assert "last-modified" not in response.headers + assert f"?v={token}" in response.text + assert _token_ref_re(token).search(response.text), ( + "no local assets/ ref carries ?v=" + ) + + +@pytest.fixture(autouse=True) +def clean_chats(db: Session) -> Iterator[None]: + """``saved_chats`` is global state — the share test writes one row + (house pattern from ``test_chats_api.py``).""" + db.execute(text("TRUNCATE saved_chats")) + db.commit() + yield + db.execute(text("TRUNCATE saved_chats")) + db.commit() + + +def test_every_known_page_200s_on_conditional_get(client: TestClient) -> None: + """THE phase-54 regression, real app: for EVERY known page, a plain + GET sets the contract, then a conditional GET carrying the static + FILE's etag (what a browser captured pre-fix) must still 200 with + the SAME rewritten body — pre-fix the loop failed on the very first + 304, pinning the browser on the previous commit's immutable assets.""" + token = asset_version() + for path in HTML_PAGES: + plain = client.get(path) + _assert_page_contract(plain, token) + + etag, _ = file_validators(_page_file(path)) + conditional = client.get(path, headers={"if-none-match": etag}) + _assert_page_contract(conditional, token) + assert conditional.content == plain.content, ( + f"{path}: the conditional 200 must serve the same rewritten body" + ) + + +def test_dynamic_shared_page_200s_on_conditional_get(admin_client: TestClient) -> None: + """The dynamic /shared/ page (phase 51) gets the same contract + via the real save+share flow: a conditional GET carrying the + ``shared.html`` file's etag must still 200 with the rewritten body — + the route's ``FileResponse`` honours conditional headers, so without + the inbound strip this page 304'd too.""" + token = asset_version() + created = admin_client.post( + "/api/chats", + json={ + "messages": [ + {"who": "user", "text": "How did I install gitlab?"}, + {"who": "brain", "text": "You've got this!"}, + ], + "share": True, + }, + ) + assert created.status_code == 201 + share_url = created.json()["share_url"] + + plain = admin_client.get(share_url) + _assert_page_contract(plain, token) + + etag, _ = file_validators(FRONTEND / "shared.html") + conditional = admin_client.get(share_url, headers={"if-none-match": etag}) + _assert_page_contract(conditional, token) + assert conditional.content == plain.content + + +def test_assets_keep_immutable_validators_and_304(client: TestClient) -> None: + """The inbound strip must NOT have widened to /assets/*: the versioned + asset URL keeps its validators and still 304s on a conditional GET — + that 304 is safe because the URL itself carries ?v=.""" + token = asset_version() + url = f"/assets/styles.css?v={token}" + + plain = client.get(url) + assert plain.status_code == 200 + assert plain.headers["cache-control"] == ASSET_CACHE_CONTROL + assert "etag" in plain.headers + assert "last-modified" in plain.headers + + conditional = client.get(url, headers={"if-none-match": plain.headers["etag"]}) + assert conditional.status_code == 304 # versioned-URL 304s stay safe + assert conditional.content == b"" + assert conditional.headers["cache-control"] == ASSET_CACHE_CONTROL + + +def test_api_paths_get_no_cache_headers_and_untouched_stream(client: TestClient) -> None: + """/api/* stays byte-identical: no cache-control injected, no + validators published, and conditional headers pass through to the + route untouched (the SSE chat stream's pass-through is pinned by + ``tests/integration/test_chat_api.py`` — the regression run below).""" + plain = client.get("/api/health") + assert plain.status_code == 200 + assert "cache-control" not in plain.headers + assert "etag" not in plain.headers + assert "last-modified" not in plain.headers + + conditional = client.get("/api/health", headers={"if-none-match": "x"}) + assert conditional.status_code == 200 # pass-through — the strip is page-scoped + assert conditional.json() == plain.json() + assert "cache-control" not in conditional.headers + + +def test_page_token_matches_process_token(client: TestClient) -> None: + """The token embedded in the served page equals ``asset_version()`` — + the per-process ``functools.cache`` contract: one page load can never + mix two versions (phase 54, assumption 5).""" + token = asset_version() + response = client.get("/") + assert response.status_code == 200 + match = re.search(r'styles\.css\?v=([^"]+)"', response.text) + assert match is not None, "styles.css ref not found in the served page" + assert match.group(1) == token diff --git a/tests/unit/test_caching.py b/tests/unit/test_caching.py index d64dc41..f26f3ad 100644 --- a/tests/unit/test_caching.py +++ b/tests/unit/test_caching.py @@ -9,7 +9,10 @@ Covers the four token paths: * empty -> a missing or empty static dir yields ``"dev"``. Plus the asset-reference rewrite (``rewrite_asset_refs``) and the -CachingMiddleware fallback branches (task 02). +CachingMiddleware fallback branches (task 02), and the phase-54 +conditional-request pins on a ``StaticFiles(html=True)``-backed app +(known pages 200 on conditional requests and publish no validators; +``/assets/*`` and ``/api/*`` keep their conditional behavior). """ from __future__ import annotations @@ -22,9 +25,11 @@ from collections.abc import AsyncIterator, Iterator from pathlib import Path import pytest -from fastapi import FastAPI +from fastapi import FastAPI, Request, Response from fastapi.responses import HTMLResponse, JSONResponse from fastapi.testclient import TestClient +from starlette.responses import FileResponse +from starlette.staticfiles import StaticFiles import app.core.caching as caching from app.config import Settings @@ -452,3 +457,262 @@ def test_middleware_leaves_unknown_paths_untouched() -> None: assert r.status_code == 200 assert "cache-control" not in r.headers assert r.json() == {"ok": True} + + +# --------------------------------------------------------------------------- +# Phase 54: conditional requests on page paths (StaticFiles-backed app) +# --------------------------------------------------------------------------- +# +# The 304 hole: the validators published for an HTML page describe the +# static FILE, but the bytes the browser receives are the REWRITTEN body +# this process built from its own token. Starlette's ``StaticFiles`` +# honours ``If-None-Match`` / ``If-Modified-Since`` *before* the middleware +# can see a response, so a matching conditional request used to 304 out of +# the rewrite — the browser kept HTML whose ``?v=`` pinned the previous +# commit's immutable assets. The middleware now strips those headers on +# page paths (inbound) and drops the validators (outbound); ``/assets/*`` +# and ``/api/*`` keep their conditional behavior. + + +def _file_validators(path: Path) -> dict[str, str]: + """The ``etag`` / ``last-modified`` the StaticFiles mount will publish + for ``path`` — derived straight from the file. The post-fix page + response publishes no validators to capture, so the browser-side etag + is reconstructed the way the mount computes it (starlette 1.x's + ``FileResponse`` defers the stat to ``__call__`` unless + ``stat_result`` is passed).""" + file_response = FileResponse(path, stat_result=os.stat(path)) + return { + "etag": file_response.headers["etag"], + "last-modified": file_response.headers["last-modified"], + } + + +def _static_page_app( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> tuple[TestClient, Path]: + """A ``StaticFiles(html=True)``-backed app + the middleware — the same + mounting shape as ``app/main.py`` (routes + the catch-all static mount) + — so conditional requests are exercised against REAL + ``FileResponse``/``StaticFiles`` validators, the exact layer that + produced the pre-fix 304. Returns ``(TestClient(app), frontend root)``. + + The version token is pinned to ``"unit-token"`` so the assertions are + exact: ``asset_version`` is the module-level call the middleware makes, + and its ``functools.cache``d result can only be a git short SHA, a + 12-hex content hash, or ``"dev"`` — so it is monkeypatched directly. + The ``get_settings`` stub (mirroring + ``test_default_static_dir_comes_from_settings``) steers any unpatched + token computation to the tmp tree instead of the checkout. + """ + frontend = tmp_path / "frontend" + assets = frontend / "assets" + assets.mkdir(parents=True) + (frontend / "index.html").write_text( + "" + '' + '' + "static index" + ) + (assets / "app.js").write_text("x") + (assets / "app.css").write_text("y") + + monkeypatch.setattr( + caching, "get_settings", lambda: Settings(static_dir=str(frontend)) + ) + monkeypatch.setattr(caching, "asset_version", lambda: "unit-token") + + app = FastAPI() + + @app.get("/api/ping") + def ping(request: Request) -> Response: + # Honours a conditional header, exactly like StaticFiles does — + # proves the inbound strip does NOT widen to /api/*. + if "if-none-match" in request.headers: + return Response(status_code=304) + return JSONResponse({"status": "ok"}) + + @app.get("/shared/{token}") + def shared_page(request: Request, token: str) -> Response: + # Like the phase-51 route: serves text/html AND honours a + # conditional header — without the inbound strip this route would + # 304 a conditional GET, so the strip is observable here. + if "if-none-match" in request.headers: + return Response(status_code=304) + return HTMLResponse( + "" + '' + "shared" + ) + + app.mount("/", StaticFiles(directory=frontend, html=True), name="static") + caching.configure_caching(app) + return TestClient(app), frontend + + +def test_conditional_get_page_returns_200_with_rewrite( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """THE phase-54 regression: a conditional GET / that matches the + static file's etag used to 304 out of the rewrite (StaticFiles + honours If-None-Match before the middleware can see a body), leaving + the browser on HTML whose ``?v=`` pinned the previous commit's + immutable assets. Post-fix the same request gets a full 200 with the + current ``?v=`` refs — and the page publishes no validators. + (Fails with a 304 on the pre-fix code — the defect reproduction.)""" + client, index_html = _static_page_app(tmp_path, monkeypatch) + + first = client.get("/") + assert first.status_code == 200 + assert "?v=unit-token" in first.text + assert "etag" not in first.headers # the fix drops it + + # Derive the *file* validator the way a browser would have captured + # it — the post-fix page response publishes no etag to capture. + etag = _file_validators(index_html)["etag"] + + r = client.get("/", headers={"if-none-match": etag}) + assert r.status_code == 200 # never 304 on a page path + assert 'href="/assets/app.js?v=unit-token"' in r.text + assert 'src="/assets/app.js?v=unit-token"' in r.text + assert r.headers["cache-control"] == "no-cache" + assert "etag" not in r.headers + assert "last-modified" not in r.headers + + +def test_conditional_get_page_with_if_modified_since_returns_200( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Same pin via If-Modified-Since: round-tripping the mount's + last-modified used to 304 (pre-fix); the inbound strip now guarantees + the full rewritten 200.""" + client, index_html = _static_page_app(tmp_path, monkeypatch) + + last_modified = _file_validators(index_html)["last-modified"] + r = client.get("/", headers={"if-modified-since": last_modified}) + assert r.status_code == 200 # never 304 on a page path + assert 'href="/assets/app.js?v=unit-token"' in r.text + assert r.headers["cache-control"] == "no-cache" + assert "etag" not in r.headers + assert "last-modified" not in r.headers + + +def test_page_response_publishes_no_validators( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Every known page (the static mount's ``/`` and the dynamic + ``/shared/``) publishes NO validators — combined with + ``Cache-Control: no-cache`` the browser can never revalidate a page + against a validator this process published.""" + client, _ = _static_page_app(tmp_path, monkeypatch) + for path in ("/", "/shared/abc123"): + r = client.get(path) + assert r.status_code == 200 + assert r.headers["cache-control"] == "no-cache" + assert "etag" not in r.headers + assert "last-modified" not in r.headers + + +@pytest.mark.parametrize("status_code", [304, 204], ids=["304", "204"]) +def test_downstream_bodiless_status_on_page_is_passed_bodiless(status_code: int) -> None: + """A downstream that still returns a bodiless status on a page path is + passed through BODIESS — starlette forbids a body on 204/304, and the + pre-guard rewrite path would build exactly that + (``Response(content=…, status_code=304)``). Belt-and-braces: after the + inbound strip StaticFiles cannot 304 page paths, but a future route or + proxy could — the passthrough keeps no-cache and no validators.""" + app = FastAPI() + + @app.get("/") + def index(request: Request) -> Response: + # The inbound strip already removed if-none-match / + # if-modified-since from the scope, so a conditional route can no + # longer 304 a page path — trigger the bodiless status with a + # header the strip does not touch, and publish validators to pin + # the outbound drop. + if request.headers.get("x-bodiless") == "1": + return Response( + status_code=status_code, + headers={ + "etag": '"stale"', + "last-modified": "Wed, 01 Jan 2024 00:00:00 GMT", + }, + ) + return HTMLResponse( + "" + '' + "" + ) + + caching.configure_caching(app) + client = TestClient(app) + + # The plain path still gets the full rewritten page (the guard only + # kicks in for bodiless statuses). + full = client.get("/") + assert full.status_code == 200 + assert 'href="/assets/styles.css?v=' in full.text + assert full.headers["cache-control"] == "no-cache" + + r = client.get("/", headers={"x-bodiless": "1", "if-none-match": "whatever"}) + assert r.status_code == status_code + assert r.content == b"" # never a body on a bodiless status + assert r.headers["cache-control"] == "no-cache" + assert "etag" not in r.headers + assert "last-modified" not in r.headers + + +def test_shared_page_path_ignores_conditional_headers( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The dynamic /shared/ page (phase 51) gets the same inbound + strip: the conditional route in ``_static_page_app`` would 304 if it + saw if-none-match — the strip hides it, so the page always 200s with + the current ``?v=`` refs.""" + client, _ = _static_page_app(tmp_path, monkeypatch) + r = client.get("/shared/abc123", headers={"if-none-match": "whatever"}) + assert r.status_code == 200 # the route would have 304'd pre-strip + assert 'href="/assets/app.css?v=unit-token"' in r.text + assert r.headers["cache-control"] == "no-cache" + assert "etag" not in r.headers + + +def test_api_path_keeps_conditional_headers( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The strip must NOT widen to /api/*: /api/ping still sees + if-none-match (it 304s on it, like any conditional route), and the + middleware passes that 304 through byte-identical — no cache-control + injected.""" + client, _ = _static_page_app(tmp_path, monkeypatch) + + plain = client.get("/api/ping") + assert plain.status_code == 200 + assert plain.json() == {"status": "ok"} + assert "cache-control" not in plain.headers + + r = client.get("/api/ping", headers={"if-none-match": "whatever"}) + assert r.status_code == 304 # the route saw the header — no strip + assert r.content == b"" + assert "cache-control" not in r.headers + + +def test_assets_path_keeps_validators_and_304( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """/assets/* keeps its conditional behavior completely intact: the + immutable caching is header-only, the validators stay, and a 304 on a + versioned asset URL is SAFE (the URL itself carries ``?v=``).""" + client, _ = _static_page_app(tmp_path, monkeypatch) + + r = client.get("/assets/app.css?v=unit-token") + assert r.status_code == 200 + assert r.headers["cache-control"] == caching.ASSET_CACHE_CONTROL + assert "etag" in r.headers + assert "last-modified" in r.headers + etag = r.headers["etag"] + + r304 = client.get("/assets/app.css?v=unit-token", headers={"if-none-match": etag}) + assert r304.status_code == 304 # versioned-URL 304s stay safe + assert r304.content == b"" + assert r304.headers["cache-control"] == caching.ASSET_CACHE_CONTROL