diff --git a/lib/tailwind_rebuild.py b/lib/tailwind_rebuild.py index cd957b5..f809794 100644 --- a/lib/tailwind_rebuild.py +++ b/lib/tailwind_rebuild.py @@ -1,4 +1,4 @@ -"""Regenerate ``static/tailwind.min.css`` over the full installed-plugin set. +"""Regenerate the runtime stylesheet over the full installed-plugin set. Core's committed (and image-baked) stylesheet is built scanning only the in-tree plugins. A plugin installed at runtime — into ``FEEDBACK_PLUGINS_DIR`` @@ -15,6 +15,7 @@ on a missing optional engine. from __future__ import annotations +import hashlib import json import logging import os @@ -40,12 +41,84 @@ _lock = threading.Lock() # in-flight build re-runs once more to pick up the newer plugin set instead of # every concurrent trigger stacking its own redundant build. _rerun = threading.Event() +_fingerprint_cache: dict = {} # lib/ lives at ``/lib``; the app root (static/, tailwind.config.js) is its # grandparent. APP_DIR = Path(__file__).resolve().parent.parent +def _committed_css_fingerprint() -> str: + """Content hash of the SHIPPED stylesheet, cached on (mtime, size). + + This is the marker that says WHICH CORE the runtime sheet was built against. Any change to + core's CSS regenerates static/tailwind.min.css, which changes this hash. + """ + committed = APP_DIR / "static" / "tailwind.min.css" + try: + st = committed.stat() + except OSError: + return "" + key = (st.st_mtime_ns, st.st_size) + cached = _fingerprint_cache.get("k") + if cached == key: + return _fingerprint_cache["v"] + h = hashlib.sha256(committed.read_bytes()).hexdigest() + _fingerprint_cache["k"] = key + _fingerprint_cache["v"] = h + return h + + +def runtime_meta_path() -> Path: + """Sidecar recording which core the runtime sheet was built against.""" + return runtime_css_path().with_suffix(".meta.json") + + +def runtime_css_is_current() -> bool: + """True when the runtime sheet was built against the core we are running NOW. + + WHY NOT mtime. Codex [P2] on the second cut of #911, and it was right: filesystem + timestamps are not a freshness signal across install methods. Archives and container images + routinely PRESERVE SOURCE MTIMES, so a just-shipped stylesheet can carry an OLDER mtime than + a runtime sheet a user built days ago. The mtime comparison then reports the stale sheet as + fresh and it masks the new core CSS indefinitely — permanently, if no Tailwind toolchain is + present to trigger a rebuild. + + Content answers the question timestamps only gesture at: the sidecar records the hash of the + committed sheet this runtime build was made from. Core ships new CSS -> that file changes -> + the hash changes -> the runtime sheet is correctly judged stale. + """ + try: + meta = json.loads(runtime_meta_path().read_text()) + except (OSError, ValueError): + return False + return bool(meta.get("committed_sha256")) and meta["committed_sha256"] == _committed_css_fingerprint() + + +def runtime_css_path() -> Path: + """Where the RUNTIME-augmented stylesheet is written. + + NOT ``static/tailwind.min.css``. That file is a BUILD ARTEFACT: committed, image-baked, + and generated by scanning only the in-tree plugins. This one is PER-INSTALL STATE — it + additionally scans whatever the user has installed into FEEDBACK_PLUGINS_DIR, so it differs + from machine to machine. They are different things and must not share a path. + + Writing the runtime sheet over the committed one had two costs: + + * IN A GIT CHECKOUT it silently modifies a TRACKED file. `git add -A` then sweeps a + 100KB reshuffle of minified CSS into the commit and `ci/tailwind-fresh` goes red with a + diff that explains nothing. That is issue #911, and it cost a red run on a PR whose + real diff touched no Tailwind classes at all. + * IN A DEPLOY the app directory may be read-only. Writing app state into it is wrong on + principle and fatal in practice. + + CONFIG_DIR is where per-install state already lives. + """ + cfg = (getenv_compat("CONFIG_DIR", "") or "").strip() + base = Path(cfg) if cfg else (Path.home() / ".local" / "share" / "feedback") + return base / "tailwind.min.css" + + def _user_plugins_dir() -> Path | None: raw = (getenv_compat("FEEDBACK_PLUGINS_DIR", "") or "").strip() if not raw: @@ -136,6 +209,14 @@ def _run_build(cmd_prefix: list[str], out: Path, src: Path) -> bool: cwd=str(APP_DIR), timeout=120, ) os.replace(staged, out) + # Stamp WHICH CORE this was built against. Without it, an upgraded app cannot tell a + # current runtime sheet from one that predates its new CSS. + try: + runtime_meta_path().write_text(json.dumps({ + "committed_sha256": _committed_css_fingerprint(), + })) + except OSError: + log.warning("tailwind: could not write the runtime sheet's meta sidecar") return True except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: stderr = (getattr(e, "stderr", "") or "")[-500:] @@ -153,7 +234,7 @@ def _run_build(cmd_prefix: list[str], out: Path, src: Path) -> bool: def rebuild(reason: str = "") -> bool: - """Regenerate ``static/tailwind.min.css`` over baked-in + user plugins. + """Regenerate the RUNTIME stylesheet (see runtime_css_path) over baked-in + user plugins. Returns ``True`` on a successful rebuild, ``False`` on any skip/failure. Never raises — callers treat CSS freshness as best-effort. Concurrent @@ -166,8 +247,13 @@ def rebuild(reason: str = "") -> bool: log.info("tailwind rebuild skipped — engine/inputs unavailable%s", tag) return False - out = APP_DIR / "static" / "tailwind.min.css" + out = runtime_css_path() src = APP_DIR / "static" / "_tailwind.src.css" + try: + out.parent.mkdir(parents=True, exist_ok=True) + except OSError: + log.warning("tailwind rebuild skipped — cannot create %s%s", out.parent, tag) + return False # If a rebuild is already running, flag a rerun and return instead of # queueing a redundant build behind it. diff --git a/server.py b/server.py index dd7455c..f5b236b 100644 --- a/server.py +++ b/server.py @@ -18,7 +18,7 @@ configure_logging() log = logging.getLogger("feedBack.server") -from fastapi import FastAPI, File +from fastapi import FastAPI, File, Response from fastapi.staticfiles import StaticFiles from fastapi.responses import FileResponse, JSONResponse, StreamingResponse @@ -47,6 +47,7 @@ import appstate import builtin_content import demo_mode import scan +import tailwind_rebuild # Extracted route modules. They import `appstate`, never `server` — one-way graph. from routers import audio_effects, artist_aliases, loops, playlists, ws_highway, chart, wanted, library_extras, shop, progression, profile, stats, version, diagnostics from routers import tunings as tunings_router @@ -1639,6 +1640,69 @@ class _RevalidatedStaticFiles(StaticFiles): return response +# ── The Tailwind stylesheet: runtime-augmented if there is one, else the committed build ── +# +# Two different things used to share one path (#911): +# +# static/tailwind.min.css a BUILD ARTEFACT. Committed, image-baked, generated from the +# in-tree plugins only. CI (`tailwind-fresh`) verifies it. +# the RUNTIME sheet PER-INSTALL STATE. Additionally scans whatever the user installed +# into FEEDBACK_PLUGINS_DIR, so it differs machine to machine. +# +# Writing the second over the first meant that merely RUNNING THE DEV SERVER from a git +# checkout silently modified a tracked file. `git add -A` then swept a 100KB reshuffle of +# minified CSS into the commit and ci/tailwind-fresh went red with a diff that explained +# nothing — on a PR whose real change touched no Tailwind classes at all. It also meant writing +# app state into the app directory, which is read-only in some deploys. +# +# The runtime sheet lives in CONFIG_DIR now. This route serves it when it exists and otherwise +# falls through to the committed one. It MUST be registered BEFORE the /static mount: routes +# are matched in order, and the mount would otherwise swallow the path. +def _runtime_css_if_usable() -> Path | None: + """The runtime sheet, but ONLY when it is actually the right answer. + + Codex [P2] on the first cut of #911, and it was right: a persisted sheet can outlive the + reason it existed and then MASK newer core CSS indefinitely. Two ways: + + * THE USER REMOVED THEIR PLUGINS. Startup only rebuilds when there are user plugins, so + nothing would ever overwrite the old sheet — and it still carries classes for plugins + that are gone, while missing nothing. With no user plugins the COMMITTED sheet is by + definition complete and authoritative. + * THE APP WAS UPGRADED. A new release ships new core classes in static/tailwind.min.css. + The runtime sheet on disk predates them. Serving it hides the new CSS until something + happens to trigger a rebuild — which, if the toolchain is absent (no node), is never. + + Freshness is decided by CONTENT, not mtime. Codex [P2] again, and again correct: archives + and container images routinely PRESERVE SOURCE MTIMES, so a just-shipped stylesheet can + carry an older timestamp than a runtime sheet built days ago — and an mtime check would call + the stale one fresh. tailwind_rebuild stamps each runtime build with the hash of the + committed sheet it was made from; a core upgrade changes that file, hence that hash. + + Falling back to the committed sheet is always safe: at worst it lacks a just-installed + plugin's classes for the seconds until the async rebuild lands. + """ + runtime = tailwind_rebuild.runtime_css_path() + if not runtime.is_file(): + return None + if tailwind_rebuild.user_plugin_count() == 0: + return None + if not tailwind_rebuild.runtime_css_is_current(): + return None # built against a different core — see runtime_css_is_current() + return runtime + + +@app.get("/static/tailwind.min.css") +def tailwind_css(request: Request): + target = _runtime_css_if_usable() or (STATIC_DIR / "tailwind.min.css") + if not target.is_file(): + return Response("", status_code=404) + # Same cache contract the /static mount applies (_RevalidatedStaticFiles): no-cache, so the + # browser always revalidates and picks up a rebuild without a hard refresh. + resp = FileResponse(str(target), media_type="text/css") + resp.headers["Cache-Control"] = "no-cache" + return resp + + app.mount("/static", _RevalidatedStaticFiles(directory=str(STATIC_DIR)), name="static") diff --git a/tests/test_tailwind_runtime_path.py b/tests/test_tailwind_runtime_path.py new file mode 100644 index 0000000..e332590 --- /dev/null +++ b/tests/test_tailwind_runtime_path.py @@ -0,0 +1,149 @@ +"""The runtime stylesheet must NOT be written over the committed one. (#911) + +Two different things used to share `static/tailwind.min.css`: + + the committed file a BUILD ARTEFACT — image-baked, generated from the in-tree plugins + only, and verified by CI's `tailwind-fresh` check. + the runtime sheet PER-INSTALL STATE — additionally scans whatever the user installed + into FEEDBACK_PLUGINS_DIR, so it differs machine to machine. + +Writing the second over the first meant that merely RUNNING THE DEV SERVER from a git checkout +silently modified a tracked file. `git add -A` then swept a 100KB reshuffle of minified CSS +into the commit and `ci/tailwind-fresh` went red with a diff that explained nothing — on a PR +whose real change touched no Tailwind classes at all. It also meant writing app state into the +app directory, which is read-only in some deploys. + +These tests pin the separation. The first is the one that matters: it is the exact failure that +shipped. +""" + +import importlib +import sys +from pathlib import Path + +import pytest + + +@pytest.fixture() +def tw(monkeypatch, tmp_path): + monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) + sys.modules.pop("tailwind_rebuild", None) + return importlib.import_module("tailwind_rebuild") + + +def test_the_runtime_sheet_is_not_the_committed_one(tw, tmp_path): + """THE REGRESSION. The runtime build must never target the tracked file.""" + runtime = tw.runtime_css_path() + committed = tw.APP_DIR / "static" / "tailwind.min.css" + + assert runtime != committed, ( + "the runtime stylesheet is being written over the COMMITTED one — running the dev " + "server in a checkout will silently dirty a tracked file and red-light ci/tailwind-fresh" + ) + assert committed not in runtime.parents + assert runtime.parent == tmp_path, "the runtime sheet belongs in CONFIG_DIR" + + +def test_runtime_path_follows_CONFIG_DIR(monkeypatch, tmp_path): + """It is per-install state, so it lives wherever this install keeps its state.""" + other = tmp_path / "elsewhere" + monkeypatch.setenv("CONFIG_DIR", str(other)) + sys.modules.pop("tailwind_rebuild", None) + tw = importlib.import_module("tailwind_rebuild") + assert tw.runtime_css_path() == other / "tailwind.min.css" + + +def test_runtime_path_falls_back_when_CONFIG_DIR_is_unset(monkeypatch): + monkeypatch.delenv("CONFIG_DIR", raising=False) + monkeypatch.delenv("SLOPSMITH_CONFIG_DIR", raising=False) + sys.modules.pop("tailwind_rebuild", None) + tw = importlib.import_module("tailwind_rebuild") + p = tw.runtime_css_path() + assert p.name == "tailwind.min.css" + assert "static" not in p.parts, "must not fall back into the app's static/ dir" + + +def test_rebuild_never_touches_the_committed_file(tw, tmp_path, monkeypatch): + """Belt and braces: drive rebuild() and assert the tracked file is byte-identical. + + This is the assertion that would actually have caught #911 in CI. + """ + committed = tw.APP_DIR / "static" / "tailwind.min.css" + before = committed.read_bytes() if committed.is_file() else None + + tw.rebuild("test") # best-effort; may skip if node/tailwind is absent — that is fine + + after = committed.read_bytes() if committed.is_file() else None + assert after == before, ( + "rebuild() modified the COMMITTED static/tailwind.min.css — this is #911: it dirties a " + "tracked file in any git checkout and red-lights ci/tailwind-fresh" + ) + + +# ── Codex [P2]: a persisted sheet must not outlive its reason ────────────────── +# +# A runtime sheet can survive the thing that justified it and then MASK newer core CSS — +# possibly forever, because startup only rebuilds when user plugins exist and skips entirely +# when the toolchain is absent. + +def _server(monkeypatch, tmp_path): + monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) + for m in ("server", "tailwind_rebuild"): + sys.modules.pop(m, None) + return importlib.import_module("server") + + +def test_runtime_sheet_is_ignored_when_the_user_has_no_plugins(monkeypatch, tmp_path): + """Remove your plugins and the committed sheet is authoritative again — it is complete by + definition. A leftover runtime sheet would carry classes for plugins that are gone.""" + srv = _server(monkeypatch, tmp_path) + (tmp_path / "tailwind.min.css").write_text("/* stale runtime sheet */") + monkeypatch.setattr(srv.tailwind_rebuild, "user_plugin_count", lambda: 0) + assert srv._runtime_css_if_usable() is None + + +def _stamp(srv, tmp_path, *, matching: bool): + """Write the sidecar that records WHICH CORE the runtime sheet was built against.""" + import json + h = srv.tailwind_rebuild._committed_css_fingerprint() if matching else "0" * 64 + # ask for the real path rather than hardcoding it — with_suffix('.meta.json') on + # tailwind.min.css yields tailwind.min.meta.json, not tailwind.meta.json + srv.tailwind_rebuild.runtime_meta_path().write_text(json.dumps({"committed_sha256": h})) + + +def test_runtime_sheet_is_ignored_when_it_was_built_against_a_DIFFERENT_core(monkeypatch, tmp_path): + """An upgrade ships new core classes. A runtime sheet built against the OLD core would hide + them — and with no Tailwind toolchain present, nothing would ever rebuild it. + + Freshness is decided by CONTENT, not mtime. Codex [P2] on the mtime version, and correct: + archives and container images routinely PRESERVE SOURCE MTIMES, so a just-shipped stylesheet + can carry an OLDER timestamp than a runtime sheet built days ago — and an mtime check would + then call the stale one fresh, masking the new CSS forever. + """ + srv = _server(monkeypatch, tmp_path) + (tmp_path / "tailwind.min.css").write_text("/* built against the old core */") + monkeypatch.setattr(srv.tailwind_rebuild, "user_plugin_count", lambda: 1) + _stamp(srv, tmp_path, matching=False) + + assert srv._runtime_css_if_usable() is None, ( + "a runtime sheet built against a different core must not mask the shipped CSS" + ) + + +def test_runtime_sheet_is_ignored_when_it_has_no_stamp_at_all(monkeypatch, tmp_path): + """A sheet from before this mechanism existed. Unknown provenance -> do not trust it.""" + srv = _server(monkeypatch, tmp_path) + (tmp_path / "tailwind.min.css").write_text("/* no sidecar */") + monkeypatch.setattr(srv.tailwind_rebuild, "user_plugin_count", lambda: 1) + assert srv._runtime_css_if_usable() is None + + +def test_runtime_sheet_IS_used_when_it_matches_this_core_and_plugins_exist(monkeypatch, tmp_path): + """The case it exists for.""" + srv = _server(monkeypatch, tmp_path) + runtime = tmp_path / "tailwind.min.css" + runtime.write_text("/* fresh, with plugin classes */") + monkeypatch.setattr(srv.tailwind_rebuild, "user_plugin_count", lambda: 2) + _stamp(srv, tmp_path, matching=True) + + assert srv._runtime_css_if_usable() == runtime