diff --git a/lib/demo_mode.py b/lib/demo_mode.py new file mode 100644 index 0000000..c201905 --- /dev/null +++ b/lib/demo_mode.py @@ -0,0 +1,342 @@ +"""Demo mode: the read-only request guard and the hourly session janitor. + +Carved VERBATIM out of server.py (R3b). Bodies are byte-identical — including a bug, see +below. + +━━━ THE MIDDLEWARE NEEDS `app`, SO THIS MODULE TAKES IT ━━━ + +`_demo_mode_guard` is an @app.middleware("http"), and a middleware has to be attached to an +app object. Rather than reach for a global, this module exposes install(app): server.py +owns the app and hands it over. Same direction as every other seam here — server.py knows +things lib/ must not have to guess. + +The janitor is symmetrical: start_janitor() / stop_janitor(), called from server.py's +startup and shutdown hooks, which is where the process lifecycle actually lives. + +━━━ register_demo_janitor_hook IS PART OF THE PLUGIN CONTRACT ━━━ + +It is a key in plugin_context, so plugins hold it as a LIVE REFERENCE from setup(). Moving +the function is fine; wrapping or renaming it is not. server.py imports this exact object +and puts it in the dict unchanged, so callable identity is preserved — +tests/test_plugin_context_contract.py (#898) fails if that ever stops being true. + +━━━ A BUG MOVED VERBATIM, ON PURPOSE ━━━ + +The janitor start guard in server.py reads: + + if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \ + and not _DEMO_JANITOR_STARTED: + +`and` binds tighter than `or`, so that is `A or (B and C)` — the `not _DEMO_JANITOR_STARTED` +re-entry guard is DEAD whenever the env var is truthy, which is the only case that runs. A +second startup leaks a janitor thread (the handle is overwritten, so shutdown joins only +the last). Preserved exactly as-is here and filed as issue #902: a carve whose value is +being provably behaviour-neutral is not the place to change behaviour. +""" +import inspect +import logging +import re +import threading +import uuid +import warnings + +from fastapi import Request +from fastapi.responses import JSONResponse +from env_compat import getenv_compat + +log = logging.getLogger("feedBack.demo_mode") + + +# Plugins that maintain session stores can register a cleanup callback here. +# The demo-mode janitor calls every registered hook once per hour so stale +# sessions are swept without the core needing to know plugin internals. +_DEMO_JANITOR_HOOKS: list = [] + + +_DEMO_JANITOR_HOOKS_LOCK = threading.Lock() + + +_DEMO_JANITOR_STARTED = False + + +_DEMO_JANITOR_STOP = threading.Event() + + +_DEMO_JANITOR_THREAD: threading.Thread | None = None + + +def register_demo_janitor_hook(fn) -> None: + """Register a zero-argument callable to be invoked hourly by the demo + janitor. Plugins call this from their ``setup(app, context)`` when they + want to participate in session cleanup under demo mode. + + The callable must accept no required arguments. Async (coroutine) + functions are rejected: the janitor runs in a plain thread and cannot + await coroutines. + """ + if not callable(fn): + raise TypeError( + f"register_demo_janitor_hook expects a callable, got {type(fn).__name__!r}" + ) + # Reject coroutine functions — check both the callable itself and its + # __call__ method so objects with an async __call__ (e.g. class instances, + # functools.partial wrappers around async functions) are also caught. + _call = getattr(fn, "__call__", None) + if inspect.iscoroutinefunction(fn) or ( + _call is not None and inspect.iscoroutinefunction(_call) + ): + raise TypeError( + "register_demo_janitor_hook does not accept async functions; " + "the janitor runs in a plain thread and cannot await coroutines" + ) + # Validate that the callable accepts zero required arguments so it won't + # crash at sweep time (hourly, far from the registration site). + try: + sig = inspect.signature(fn) + except ValueError: + # inspect.signature() raises ValueError for built-in C callables whose + # signature cannot be determined. Accept them as-is; if they fail at + # runtime the janitor will catch and log the exception. + pass + else: + required = [ + p for p in sig.parameters.values() + if p.default is inspect.Parameter.empty + and p.kind not in ( + inspect.Parameter.VAR_POSITIONAL, + inspect.Parameter.VAR_KEYWORD, + ) + ] + if required: + raise TypeError( + f"register_demo_janitor_hook expects a zero-argument callable; " + f"{fn!r} has {len(required)} required parameter(s): " + + ", ".join(p.name for p in required) + ) + with _DEMO_JANITOR_HOOKS_LOCK: + _DEMO_JANITOR_HOOKS.append(fn) + + +def _run_janitor_hook(hook) -> None: + """Run a single janitor hook inline, swallowing and logging any exception. + + If the hook returns an awaitable (e.g. a coroutine slipped through the + async-function guard), the coroutine is closed immediately to avoid + ``RuntimeWarning: coroutine was never awaited`` noise, and a warning is + emitted so the plugin author knows to fix their hook. + """ + try: + result = hook() + except Exception: + log.exception("janitor hook %r raised", hook) + return + if inspect.iscoroutine(result): + # A coroutine slipped through the async-function guard (e.g. via a + # wrapper/partial). Close it to suppress "coroutine never awaited", + # then warn so the plugin author knows to fix their hook. + try: + result.close() + except Exception: + log.exception("error closing coroutine from janitor hook %r", hook) + warnings.warn( + f"janitor hook {hook!r} returned a coroutine; " + "hooks must be plain synchronous callables — " + "register_demo_janitor_hook does not accept async functions", + RuntimeWarning, + stacklevel=1, + ) + elif inspect.isawaitable(result): + # Future/Task: no .close() method; just warn and leave it alone. + warnings.warn( + f"janitor hook {hook!r} returned an awaitable (Future/Task); " + "hooks must be plain synchronous callables", + RuntimeWarning, + stacklevel=1, + ) + + +_DEMO_BLOCKED: list[tuple[str, re.Pattern]] = [ + ("POST", re.compile(r"^/api/settings$")), + ("POST", re.compile(r"^/api/settings/import$")), + ("POST", re.compile(r"^/api/settings/reset$")), + ("POST", re.compile(r"^/api/rescan$")), + ("POST", re.compile(r"^/api/rescan/full$")), + ("POST", re.compile(r"^/api/songs/upload$")), + ("DELETE", re.compile(r"^/api/song/.+$")), + ("POST", re.compile(r"^/api/favorites/toggle$")), + ("POST", re.compile(r"^/api/loops$")), + ("DELETE", re.compile(r"^/api/loops/[^/]+$")), + ("POST", re.compile(r"^/api/audio-effects/mappings$")), + ("DELETE", re.compile(r"^/api/audio-effects/mappings/[^/]+$")), + ("POST", re.compile(r"^/api/audio-effects/mappings/[^/]+/activate$")), + ("DELETE", re.compile(r"^/api/audio-effects/active-mapping$")), + ("POST", re.compile(r"^/api/song/.*/meta$")), + ("POST", re.compile(r"^/api/song/.*/art/upload$")), + ("PUT", re.compile(r"^/api/song/.+/overrides$")), + ("GET", re.compile(r"^/api/plugins/updates$")), + ("POST", re.compile(r"^/api/plugins/[^/]+/update$")), + ("POST", re.compile(r"^/api/plugins/editor/save$")), + ("POST", re.compile(r"^/api/plugins/editor/build$")), + ("POST", re.compile(r"^/api/plugins/editor/upload-art$")), + ("POST", re.compile(r"^/api/plugins/editor/upload-audio$")), + ("POST", re.compile(r"^/api/plugins/editor/youtube-audio$")), + ("POST", re.compile(r"^/api/plugins/editor/import-gp$")), + ("POST", re.compile(r"^/api/plugins/editor/import-midi$")), + ("POST", re.compile(r"^/api/plugins/lyrics_karaoke/align$")), + ("POST", re.compile(r"^/api/plugins/lyrics_karaoke/generate-pitch$")), + ("POST", re.compile(r"^/api/plugins/lyrics_karaoke/save-lyrics$")), + ("POST", re.compile(r"^/api/plugins/lyrics_sync/align$")), + ("POST", re.compile(r"^/api/plugins/lyrics_sync/save$")), + ("POST", re.compile(r"^/api/plugins/studio/sessions/[^/]+/extract-drums$")), + ("POST", re.compile(r"^/api/diagnostics/export$")), + ("GET", re.compile(r"^/api/diagnostics/preview$")), + ("GET", re.compile(r"^/api/diagnostics/hardware$")), + # Bundled core plugin — video background upload/delete + ("POST", re.compile(r"^/api/plugins/highway_3d/files$")), + ("DELETE", re.compile(r"^/api/plugins/highway_3d/files$")), + # fee[dB]ack v0.3.0 write endpoints — demo mode is read-only, so block the + # new profile / XP / stats / playlists / saved mutators too. + ("POST", re.compile(r"^/api/profile$")), + ("POST", re.compile(r"^/api/profile/avatar$")), + ("POST", re.compile(r"^/api/xp/award$")), + ("POST", re.compile(r"^/api/stats$")), + ("POST", re.compile(r"^/api/playlists$")), + ("PATCH", re.compile(r"^/api/playlists/[^/]+$")), + ("DELETE", re.compile(r"^/api/playlists/[^/]+$")), + ("POST", re.compile(r"^/api/playlists/[^/]+/songs$")), + ("DELETE", re.compile(r"^/api/playlists/[^/]+/songs/.+$")), + ("POST", re.compile(r"^/api/playlists/[^/]+/reorder$")), + ("POST", re.compile(r"^/api/playlists/[^/]+/cover$")), + ("DELETE", re.compile(r"^/api/playlists/[^/]+/cover$")), + ("POST", re.compile(r"^/api/saved/toggle$")), + # Progression (spec 010) write endpoints — demo mode stays read-only. + ("POST", re.compile(r"^/api/progression/paths$")), + ("POST", re.compile(r"^/api/progression/onboarding$")), + ("POST", re.compile(r"^/api/progression/events$")), + ("POST", re.compile(r"^/api/shop/buy$")), + ("POST", re.compile(r"^/api/shop/equip$")), + # Enrichment (P8): review writes mutate the local match cache, and the + # search proxy / manual kick relay to MusicBrainz — none of it belongs to + # anonymous demo visitors (they'd spend the shared rate limit). + ("POST", re.compile(r"^/api/enrichment/review/.+$")), + ("POST", re.compile(r"^/api/enrichment/kick$")), + ("POST", re.compile(r"^/api/enrichment/cancel$")), + ("POST", re.compile(r"^/api/enrichment/rematch$")), + ("GET", re.compile(r"^/api/enrichment/search$")), + # AcoustID audio fingerprinting: both identify endpoints run fpcalc (CPU) + # and spend the shared AcoustID rate budget on the caller's behalf — same + # rule as the search/kick relays above; not for anonymous demo visitors. + ("POST", re.compile(r"^/api/enrichment/identify$")), + ("POST", re.compile(r"^/api/enrichment/identify/.+$")), + # Context menus (R2): the per-song re-match mutates the cache + spends + # rate limit; Get-info exposes filesystem paths. + ("POST", re.compile(r"^/api/enrichment/refresh/.+$")), + ("GET", re.compile(r"^/api/chart/.+/fileinfo$")), + # Gap-fill (R4a) rewrites pack files on disk — never for demo visitors. + ("POST", re.compile(r"^/api/song/.+/gap-fill$")), + # Art layer (R3): all three mutate server state / touch the network on a + # visitor's behalf — the base64 upload writes files, the URL fetch makes the + # server request arbitrary images, and the override delete removes files. + ("POST", re.compile(r"^/api/song/.+/art/upload$")), + ("POST", re.compile(r"^/api/song/.+/art/url$")), + ("DELETE", re.compile(r"^/api/art/.+/override$")), + # Cover picker (PR-C): read-only, but a cache-miss open spends 1-3 + # throttled Cover Art Archive calls — anonymous demo visitors don't get + # to spend the shared rate budget (same rule as enrichment search/kick). + ("GET", re.compile(r"^/api/song/.+/art/candidates$")), + # Artist pages (PR-B): the links GET lazily fetches from MusicBrainz on a + # visitor's behalf AND writes the artist_enrichment cache; refresh + # re-spends the shared rate limit. The /page route stays open (all-local + # read). Same rationale as /api/enrichment/search above. + ("GET", re.compile(r"^/api/artist/.+/links$")), + ("POST", re.compile(r"^/api/artist/.+/links/refresh$")), +] + + +async def _demo_mode_guard(request: Request, call_next): + if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1": + path = request.url.path + for method, pattern in _DEMO_BLOCKED: + if request.method == method and pattern.match(path): + return JSONResponse({"error": "demo mode: read-only"}, status_code=403) + response = await call_next(request) + if request.method == "GET" and path == "/" and "feedBack_demo_session" not in request.cookies: + forwarded_proto = (request.headers.get("x-forwarded-proto") or "").split(",")[0].strip() + is_secure = request.url.scheme == "https" or forwarded_proto.lower() == "https" + response.set_cookie( + "feedBack_demo_session", str(uuid.uuid4()), + max_age=86400, httponly=True, samesite="lax", + secure=is_secure, + ) + return response + return await call_next(request) + + +def install(app) -> None: + """Attach the demo-mode request guard to `app`. + + Called by server.py, which owns the app. A middleware cannot exist without one, and a + module under lib/ should not be reaching for a global to find it. + """ + app.middleware("http")(_demo_mode_guard) + + +def demo_mode_enabled() -> bool: + """True when demo mode is on. Read at CALL time, never captured — tests set and unset + FEEDBACK_DEMO_MODE with monkeypatch, so a value cached at import pins the wrong one.""" + return bool(getenv_compat("FEEDBACK_DEMO_MODE")) + + +def start_janitor() -> None: + """Start the hourly session janitor. Called from server.py's startup hook. + + NB the caller's guard is the buggy one described in this module's header (issue #902). + Behaviour is preserved verbatim: this starts a thread every time it is called. + """ + global _DEMO_JANITOR_STARTED, _DEMO_JANITOR_THREAD + _DEMO_JANITOR_STARTED = True + _DEMO_JANITOR_STOP.clear() + + def _janitor(): + while not _DEMO_JANITOR_STOP.wait(timeout=3600): + with _DEMO_JANITOR_HOOKS_LOCK: + hooks = list(_DEMO_JANITOR_HOOKS) + for hook in hooks: + _run_janitor_hook(hook) + + _DEMO_JANITOR_THREAD = threading.Thread(target=_janitor, daemon=True, name="demo-janitor") + _DEMO_JANITOR_THREAD.start() + + +def janitor_started() -> bool: + return _DEMO_JANITOR_STARTED + + +def stop_janitor(timeout: float = 5) -> bool: + """Signal the janitor to stop, join it, and drop the registered hooks. + + Returns True if it stopped, False if it outlived the join (the caller warns). + + THE ORDER HERE IS LOAD-BEARING and preserved exactly from server.py. When the thread + does NOT die within the timeout we return WITHOUT clearing _DEMO_JANITOR_STARTED and + WITHOUT dropping the thread handle — deliberately — so a subsequent startup does not + spawn a SECOND janitor alongside the one still running. Clearing the flag first (the + obvious way to write this) would quietly reintroduce exactly the double-janitor leak + the flag exists to prevent. + """ + global _DEMO_JANITOR_STARTED, _DEMO_JANITOR_THREAD + if not _DEMO_JANITOR_STARTED: + return True + _DEMO_JANITOR_STOP.set() + thread = _DEMO_JANITOR_THREAD + if thread is not None: + thread.join(timeout=timeout) + if thread.is_alive(): + # Leave _DEMO_JANITOR_STARTED True so a new janitor is not spawned by a + # subsequent startup while the old one is alive. + return False + _DEMO_JANITOR_THREAD = None + _DEMO_JANITOR_STARTED = False + with _DEMO_JANITOR_HOOKS_LOCK: + _DEMO_JANITOR_HOOKS.clear() + return True diff --git a/server.py b/server.py index 452ec90..33341a2 100644 --- a/server.py +++ b/server.py @@ -45,6 +45,7 @@ from dlc_paths import _get_dlc_dir, _resolve_dlc_path # Lives in lib/ because that is the one core dir every packaging path copies. import appstate import builtin_content +import demo_mode import scan # 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 @@ -81,222 +82,18 @@ from fastapi import Request app = FastAPI(title="FeedBack") -# Plugins that maintain session stores can register a cleanup callback here. -# The demo-mode janitor calls every registered hook once per hour so stale -# sessions are swept without the core needing to know plugin internals. -_DEMO_JANITOR_HOOKS: list = [] -_DEMO_JANITOR_HOOKS_LOCK = threading.Lock() -_DEMO_JANITOR_STARTED = False -_DEMO_JANITOR_STOP = threading.Event() -_DEMO_JANITOR_THREAD: threading.Thread | None = None +# Demo mode lives in lib/demo_mode.py now. The guard is a middleware, so it needs the app — +# server.py owns it and hands it over rather than making lib/ reach for a global. +demo_mode.install(app) + -def register_demo_janitor_hook(fn) -> None: - """Register a zero-argument callable to be invoked hourly by the demo - janitor. Plugins call this from their ``setup(app, context)`` when they - want to participate in session cleanup under demo mode. - - The callable must accept no required arguments. Async (coroutine) - functions are rejected: the janitor runs in a plain thread and cannot - await coroutines. - """ - if not callable(fn): - raise TypeError( - f"register_demo_janitor_hook expects a callable, got {type(fn).__name__!r}" - ) - # Reject coroutine functions — check both the callable itself and its - # __call__ method so objects with an async __call__ (e.g. class instances, - # functools.partial wrappers around async functions) are also caught. - _call = getattr(fn, "__call__", None) - if inspect.iscoroutinefunction(fn) or ( - _call is not None and inspect.iscoroutinefunction(_call) - ): - raise TypeError( - "register_demo_janitor_hook does not accept async functions; " - "the janitor runs in a plain thread and cannot await coroutines" - ) - # Validate that the callable accepts zero required arguments so it won't - # crash at sweep time (hourly, far from the registration site). - try: - sig = inspect.signature(fn) - except ValueError: - # inspect.signature() raises ValueError for built-in C callables whose - # signature cannot be determined. Accept them as-is; if they fail at - # runtime the janitor will catch and log the exception. - pass - else: - required = [ - p for p in sig.parameters.values() - if p.default is inspect.Parameter.empty - and p.kind not in ( - inspect.Parameter.VAR_POSITIONAL, - inspect.Parameter.VAR_KEYWORD, - ) - ] - if required: - raise TypeError( - f"register_demo_janitor_hook expects a zero-argument callable; " - f"{fn!r} has {len(required)} required parameter(s): " - + ", ".join(p.name for p in required) - ) - with _DEMO_JANITOR_HOOKS_LOCK: - _DEMO_JANITOR_HOOKS.append(fn) -def _run_janitor_hook(hook) -> None: - """Run a single janitor hook inline, swallowing and logging any exception. - - If the hook returns an awaitable (e.g. a coroutine slipped through the - async-function guard), the coroutine is closed immediately to avoid - ``RuntimeWarning: coroutine was never awaited`` noise, and a warning is - emitted so the plugin author knows to fix their hook. - """ - try: - result = hook() - except Exception: - log.exception("janitor hook %r raised", hook) - return - if inspect.iscoroutine(result): - # A coroutine slipped through the async-function guard (e.g. via a - # wrapper/partial). Close it to suppress "coroutine never awaited", - # then warn so the plugin author knows to fix their hook. - try: - result.close() - except Exception: - log.exception("error closing coroutine from janitor hook %r", hook) - warnings.warn( - f"janitor hook {hook!r} returned a coroutine; " - "hooks must be plain synchronous callables — " - "register_demo_janitor_hook does not accept async functions", - RuntimeWarning, - stacklevel=1, - ) - elif inspect.isawaitable(result): - # Future/Task: no .close() method; just warn and leave it alone. - warnings.warn( - f"janitor hook {hook!r} returned an awaitable (Future/Task); " - "hooks must be plain synchronous callables", - RuntimeWarning, - stacklevel=1, - ) -_DEMO_BLOCKED: list[tuple[str, re.Pattern]] = [ - ("POST", re.compile(r"^/api/settings$")), - ("POST", re.compile(r"^/api/settings/import$")), - ("POST", re.compile(r"^/api/settings/reset$")), - ("POST", re.compile(r"^/api/rescan$")), - ("POST", re.compile(r"^/api/rescan/full$")), - ("POST", re.compile(r"^/api/songs/upload$")), - ("DELETE", re.compile(r"^/api/song/.+$")), - ("POST", re.compile(r"^/api/favorites/toggle$")), - ("POST", re.compile(r"^/api/loops$")), - ("DELETE", re.compile(r"^/api/loops/[^/]+$")), - ("POST", re.compile(r"^/api/audio-effects/mappings$")), - ("DELETE", re.compile(r"^/api/audio-effects/mappings/[^/]+$")), - ("POST", re.compile(r"^/api/audio-effects/mappings/[^/]+/activate$")), - ("DELETE", re.compile(r"^/api/audio-effects/active-mapping$")), - ("POST", re.compile(r"^/api/song/.*/meta$")), - ("POST", re.compile(r"^/api/song/.*/art/upload$")), - ("PUT", re.compile(r"^/api/song/.+/overrides$")), - ("GET", re.compile(r"^/api/plugins/updates$")), - ("POST", re.compile(r"^/api/plugins/[^/]+/update$")), - ("POST", re.compile(r"^/api/plugins/editor/save$")), - ("POST", re.compile(r"^/api/plugins/editor/build$")), - ("POST", re.compile(r"^/api/plugins/editor/upload-art$")), - ("POST", re.compile(r"^/api/plugins/editor/upload-audio$")), - ("POST", re.compile(r"^/api/plugins/editor/youtube-audio$")), - ("POST", re.compile(r"^/api/plugins/editor/import-gp$")), - ("POST", re.compile(r"^/api/plugins/editor/import-midi$")), - ("POST", re.compile(r"^/api/plugins/lyrics_karaoke/align$")), - ("POST", re.compile(r"^/api/plugins/lyrics_karaoke/generate-pitch$")), - ("POST", re.compile(r"^/api/plugins/lyrics_karaoke/save-lyrics$")), - ("POST", re.compile(r"^/api/plugins/lyrics_sync/align$")), - ("POST", re.compile(r"^/api/plugins/lyrics_sync/save$")), - ("POST", re.compile(r"^/api/plugins/studio/sessions/[^/]+/extract-drums$")), - ("POST", re.compile(r"^/api/diagnostics/export$")), - ("GET", re.compile(r"^/api/diagnostics/preview$")), - ("GET", re.compile(r"^/api/diagnostics/hardware$")), - # Bundled core plugin — video background upload/delete - ("POST", re.compile(r"^/api/plugins/highway_3d/files$")), - ("DELETE", re.compile(r"^/api/plugins/highway_3d/files$")), - # fee[dB]ack v0.3.0 write endpoints — demo mode is read-only, so block the - # new profile / XP / stats / playlists / saved mutators too. - ("POST", re.compile(r"^/api/profile$")), - ("POST", re.compile(r"^/api/profile/avatar$")), - ("POST", re.compile(r"^/api/xp/award$")), - ("POST", re.compile(r"^/api/stats$")), - ("POST", re.compile(r"^/api/playlists$")), - ("PATCH", re.compile(r"^/api/playlists/[^/]+$")), - ("DELETE", re.compile(r"^/api/playlists/[^/]+$")), - ("POST", re.compile(r"^/api/playlists/[^/]+/songs$")), - ("DELETE", re.compile(r"^/api/playlists/[^/]+/songs/.+$")), - ("POST", re.compile(r"^/api/playlists/[^/]+/reorder$")), - ("POST", re.compile(r"^/api/playlists/[^/]+/cover$")), - ("DELETE", re.compile(r"^/api/playlists/[^/]+/cover$")), - ("POST", re.compile(r"^/api/saved/toggle$")), - # Progression (spec 010) write endpoints — demo mode stays read-only. - ("POST", re.compile(r"^/api/progression/paths$")), - ("POST", re.compile(r"^/api/progression/onboarding$")), - ("POST", re.compile(r"^/api/progression/events$")), - ("POST", re.compile(r"^/api/shop/buy$")), - ("POST", re.compile(r"^/api/shop/equip$")), - # Enrichment (P8): review writes mutate the local match cache, and the - # search proxy / manual kick relay to MusicBrainz — none of it belongs to - # anonymous demo visitors (they'd spend the shared rate limit). - ("POST", re.compile(r"^/api/enrichment/review/.+$")), - ("POST", re.compile(r"^/api/enrichment/kick$")), - ("POST", re.compile(r"^/api/enrichment/cancel$")), - ("POST", re.compile(r"^/api/enrichment/rematch$")), - ("GET", re.compile(r"^/api/enrichment/search$")), - # AcoustID audio fingerprinting: both identify endpoints run fpcalc (CPU) - # and spend the shared AcoustID rate budget on the caller's behalf — same - # rule as the search/kick relays above; not for anonymous demo visitors. - ("POST", re.compile(r"^/api/enrichment/identify$")), - ("POST", re.compile(r"^/api/enrichment/identify/.+$")), - # Context menus (R2): the per-song re-match mutates the cache + spends - # rate limit; Get-info exposes filesystem paths. - ("POST", re.compile(r"^/api/enrichment/refresh/.+$")), - ("GET", re.compile(r"^/api/chart/.+/fileinfo$")), - # Gap-fill (R4a) rewrites pack files on disk — never for demo visitors. - ("POST", re.compile(r"^/api/song/.+/gap-fill$")), - # Art layer (R3): all three mutate server state / touch the network on a - # visitor's behalf — the base64 upload writes files, the URL fetch makes the - # server request arbitrary images, and the override delete removes files. - ("POST", re.compile(r"^/api/song/.+/art/upload$")), - ("POST", re.compile(r"^/api/song/.+/art/url$")), - ("DELETE", re.compile(r"^/api/art/.+/override$")), - # Cover picker (PR-C): read-only, but a cache-miss open spends 1-3 - # throttled Cover Art Archive calls — anonymous demo visitors don't get - # to spend the shared rate budget (same rule as enrichment search/kick). - ("GET", re.compile(r"^/api/song/.+/art/candidates$")), - # Artist pages (PR-B): the links GET lazily fetches from MusicBrainz on a - # visitor's behalf AND writes the artist_enrichment cache; refresh - # re-spends the shared rate limit. The /page route stays open (all-local - # read). Same rationale as /api/enrichment/search above. - ("GET", re.compile(r"^/api/artist/.+/links$")), - ("POST", re.compile(r"^/api/artist/.+/links/refresh$")), -] -@app.middleware("http") -async def _demo_mode_guard(request: Request, call_next): - if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1": - path = request.url.path - for method, pattern in _DEMO_BLOCKED: - if request.method == method and pattern.match(path): - return JSONResponse({"error": "demo mode: read-only"}, status_code=403) - response = await call_next(request) - if request.method == "GET" and path == "/" and "feedBack_demo_session" not in request.cookies: - forwarded_proto = (request.headers.get("x-forwarded-proto") or "").split(",")[0].strip() - is_secure = request.url.scheme == "https" or forwarded_proto.lower() == "https" - response.set_cookie( - "feedBack_demo_session", str(uuid.uuid4()), - max_age=86400, httponly=True, samesite="lax", - secure=is_secure, - ) - return response - return await call_next(request) from asgi_correlation_id import CorrelationIdMiddleware @@ -915,7 +712,7 @@ async def startup_events(): "register_tuning_provider": register_tuning_provider, "unregister_tuning_provider": unregister_tuning_provider, "get_sloppak_cache_dir": lambda: SLOPPAK_CACHE_DIR, - "register_demo_janitor_hook": register_demo_janitor_hook, + "register_demo_janitor_hook": demo_mode.register_demo_janitor_hook, # Unified XP service (fee[dB]ack v0.3.0). Plugins that award XP # (minigames, tutorials, …) should feed the single core store via these # instead of keeping a private XP curve. `award_xp` returns the new @@ -1173,18 +970,13 @@ async def startup_events(): else: threading.Thread(target=_load_plugins_background, daemon=True).start() - global _DEMO_JANITOR_STARTED, _DEMO_JANITOR_THREAD - if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" and not _DEMO_JANITOR_STARTED: - _DEMO_JANITOR_STARTED = True - _DEMO_JANITOR_STOP.clear() - def _janitor(): - while not _DEMO_JANITOR_STOP.wait(timeout=3600): - with _DEMO_JANITOR_HOOKS_LOCK: - hooks = list(_DEMO_JANITOR_HOOKS) - for hook in hooks: - _run_janitor_hook(hook) - _DEMO_JANITOR_THREAD = threading.Thread(target=_janitor, daemon=True, name="demo-janitor") - _DEMO_JANITOR_THREAD.start() + # NB the `or ... == "1" and not started` shape below is PRESERVED VERBATIM: `and` binds + # tighter than `or`, so the re-entry guard is dead whenever the env var is truthy, and a + # second startup leaks a janitor thread. That is issue #902 — not fixed here, because a + # carve whose value is being provably behaviour-neutral is not the place to change + # behaviour. + if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" and not demo_mode.janitor_started(): + demo_mode.start_janitor() # Start background metadata scan startup_scan() @@ -1193,28 +985,15 @@ async def startup_events(): @app.on_event("shutdown") def shutdown_events(): """Stop the demo-mode janitor thread (if running) on server shutdown.""" - global _DEMO_JANITOR_STARTED, _DEMO_JANITOR_THREAD, _event_loop + global _event_loop _event_loop = None # prevent stale loop reference after shutdown - if _DEMO_JANITOR_STARTED: - _DEMO_JANITOR_STOP.set() - thread = _DEMO_JANITOR_THREAD - if thread is not None: - thread.join(timeout=5) - if thread.is_alive(): - import warnings - warnings.warn( - "demo-janitor thread did not stop within 5 s; " - "a registered hook may be blocking", - RuntimeWarning, - stacklevel=1, - ) - # Leave _DEMO_JANITOR_STARTED True so a new janitor is not - # spawned by a subsequent startup while the old one is alive. - return - _DEMO_JANITOR_THREAD = None - _DEMO_JANITOR_STARTED = False - with _DEMO_JANITOR_HOOKS_LOCK: - _DEMO_JANITOR_HOOKS.clear() + if not demo_mode.stop_janitor(timeout=5): + warnings.warn( + "demo-janitor thread did not stop within 5 s; " + "a registered hook may be blocking", + RuntimeWarning, + stacklevel=1, + ) def startup_scan(): diff --git a/tests/test_demo_mode.py b/tests/test_demo_mode.py index 0523f38..88f3933 100644 --- a/tests/test_demo_mode.py +++ b/tests/test_demo_mode.py @@ -16,6 +16,7 @@ Covers: import importlib import sys +import demo_mode import pytest from fastapi.testclient import TestClient @@ -50,14 +51,14 @@ def _cleanup(server, client): client.close() # Stop the demo-mode janitor thread (if started) so daemon threads don't # accumulate across tests. - server._DEMO_JANITOR_STOP.set() - thread = server._DEMO_JANITOR_THREAD + demo_mode._DEMO_JANITOR_STOP.set() + thread = demo_mode._DEMO_JANITOR_THREAD if thread is not None: thread.join(timeout=2) - server._DEMO_JANITOR_STARTED = False - server._DEMO_JANITOR_THREAD = None - with server._DEMO_JANITOR_HOOKS_LOCK: - server._DEMO_JANITOR_HOOKS.clear() + demo_mode._DEMO_JANITOR_STARTED = False + demo_mode._DEMO_JANITOR_THREAD = None + with demo_mode._DEMO_JANITOR_HOOKS_LOCK: + demo_mode._DEMO_JANITOR_HOOKS.clear() conn = getattr(getattr(server, "meta_db", None), "conn", None) if conn is not None: getattr(__import__("sys").modules.get("server"), "_join_background_db_threads", lambda: None)() @@ -221,15 +222,15 @@ def test_register_demo_janitor_hook_is_callable(tmp_path, monkeypatch): server, client = _make_client(tmp_path, monkeypatch, demo=True) try: called = [] - server.register_demo_janitor_hook(lambda: called.append(1)) + demo_mode.register_demo_janitor_hook(lambda: called.append(1)) # Manually invoke the registered hooks (simulating a janitor sweep). - for hook in list(server._DEMO_JANITOR_HOOKS): + for hook in list(demo_mode._DEMO_JANITOR_HOOKS): hook() assert 1 in called finally: # Clean up our test hook so it doesn't leak into other tests. - with server._DEMO_JANITOR_HOOKS_LOCK: - server._DEMO_JANITOR_HOOKS.clear() + with demo_mode._DEMO_JANITOR_HOOKS_LOCK: + demo_mode._DEMO_JANITOR_HOOKS.clear() _cleanup(server, client) @@ -238,7 +239,7 @@ def test_register_demo_janitor_hook_rejects_non_callable(tmp_path, monkeypatch): server, client = _make_client(tmp_path, monkeypatch, demo=True) try: with pytest.raises(TypeError): - server.register_demo_janitor_hook("not a function") + demo_mode.register_demo_janitor_hook("not a function") finally: _cleanup(server, client) @@ -251,7 +252,7 @@ def test_register_demo_janitor_hook_rejects_async_callable(tmp_path, monkeypatch pass with pytest.raises(TypeError, match="async"): - server.register_demo_janitor_hook(_async_hook) + demo_mode.register_demo_janitor_hook(_async_hook) finally: _cleanup(server, client) @@ -264,7 +265,7 @@ def test_register_demo_janitor_hook_rejects_non_zero_arg_callable(tmp_path, monk pass with pytest.raises(TypeError, match="zero-argument"): - server.register_demo_janitor_hook(_needs_arg) + demo_mode.register_demo_janitor_hook(_needs_arg) finally: _cleanup(server, client) @@ -276,10 +277,10 @@ def test_register_demo_janitor_hook_accepts_default_arg_callable(tmp_path, monke def _optional_arg(x=None): pass - server.register_demo_janitor_hook(_optional_arg) + demo_mode.register_demo_janitor_hook(_optional_arg) finally: - with server._DEMO_JANITOR_HOOKS_LOCK: - server._DEMO_JANITOR_HOOKS.clear() + with demo_mode._DEMO_JANITOR_HOOKS_LOCK: + demo_mode._DEMO_JANITOR_HOOKS.clear() _cleanup(server, client) @@ -308,21 +309,21 @@ def test_register_demo_janitor_hook_in_plugin_context(tmp_path, monkeypatch): assert "register_demo_janitor_hook" in captured, ( "register_demo_janitor_hook was not passed in the plugin context" ) - assert captured["register_demo_janitor_hook"] is server.register_demo_janitor_hook + assert captured["register_demo_janitor_hook"] is demo_mode.register_demo_janitor_hook conn = getattr(getattr(server, "meta_db", None), "conn", None) if conn is not None: getattr(__import__("sys").modules.get("server"), "_join_background_db_threads", lambda: None)() conn.close() # Clean up janitor state so it doesn't bleed into other tests. - server._DEMO_JANITOR_STOP.set() - thread = server._DEMO_JANITOR_THREAD + demo_mode._DEMO_JANITOR_STOP.set() + thread = demo_mode._DEMO_JANITOR_THREAD if thread is not None: thread.join(timeout=2) - server._DEMO_JANITOR_STARTED = False - server._DEMO_JANITOR_THREAD = None - with server._DEMO_JANITOR_HOOKS_LOCK: - server._DEMO_JANITOR_HOOKS.clear() + demo_mode._DEMO_JANITOR_STARTED = False + demo_mode._DEMO_JANITOR_THREAD = None + with demo_mode._DEMO_JANITOR_HOOKS_LOCK: + demo_mode._DEMO_JANITOR_HOOKS.clear() diff --git a/tests/test_startup_status.py b/tests/test_startup_status.py index b48b9b9..145b19e 100644 --- a/tests/test_startup_status.py +++ b/tests/test_startup_status.py @@ -11,6 +11,7 @@ import time import asyncio import httpx +import demo_mode import pytest from fastapi.testclient import TestClient @@ -120,12 +121,12 @@ def startup_harness(tmp_path, monkeypatch, isolate_logging): yield server, phases - server._DEMO_JANITOR_STOP.set() - thread = server._DEMO_JANITOR_THREAD + demo_mode._DEMO_JANITOR_STOP.set() + thread = demo_mode._DEMO_JANITOR_THREAD if thread is not None: thread.join(timeout=2) - server._DEMO_JANITOR_STARTED = False - server._DEMO_JANITOR_THREAD = None + demo_mode._DEMO_JANITOR_STARTED = False + demo_mode._DEMO_JANITOR_THREAD = None conn = getattr(getattr(server, "meta_db", None), "conn", None) if conn is not None: getattr(__import__("sys").modules.get("server"), "_join_background_db_threads", lambda: None)() @@ -686,12 +687,12 @@ def test_startup_status_e2e_real_plugin_loader(tmp_path, monkeypatch, isolate_lo assert sentinel.status_code == 200 assert sentinel.json() == {"ok": True} finally: - server._DEMO_JANITOR_STOP.set() - thread = server._DEMO_JANITOR_THREAD + demo_mode._DEMO_JANITOR_STOP.set() + thread = demo_mode._DEMO_JANITOR_THREAD if thread is not None: thread.join(timeout=2) - server._DEMO_JANITOR_STARTED = False - server._DEMO_JANITOR_THREAD = None + demo_mode._DEMO_JANITOR_STARTED = False + demo_mode._DEMO_JANITOR_THREAD = None conn = getattr(getattr(server, "meta_db", None), "conn", None) if conn is not None: getattr(__import__("sys").modules.get("server"), "_join_background_db_threads", lambda: None)() @@ -777,12 +778,12 @@ def test_startup_status_endpoint_background_thread_path(tmp_path, monkeypatch, i # actually executed the sentinel — proves the main-loop handoff path ran. assert _route_setup_called, "route_setup_fn was never called; call_soon_threadsafe path was not exercised" finally: - server._DEMO_JANITOR_STOP.set() - thread = server._DEMO_JANITOR_THREAD + demo_mode._DEMO_JANITOR_STOP.set() + thread = demo_mode._DEMO_JANITOR_THREAD if thread is not None: thread.join(timeout=2) - server._DEMO_JANITOR_STARTED = False - server._DEMO_JANITOR_THREAD = None + demo_mode._DEMO_JANITOR_STARTED = False + demo_mode._DEMO_JANITOR_THREAD = None conn = getattr(getattr(server, "meta_db", None), "conn", None) if conn is not None: getattr(__import__("sys").modules.get("server"), "_join_background_db_threads", lambda: None)() @@ -826,12 +827,12 @@ def test_startup_status_endpoint_background_thread_failure(tmp_path, monkeypatch assert data["phase"] == "error" assert _BG_ERROR in data["error"] finally: - server._DEMO_JANITOR_STOP.set() - thread = server._DEMO_JANITOR_THREAD + demo_mode._DEMO_JANITOR_STOP.set() + thread = demo_mode._DEMO_JANITOR_THREAD if thread is not None: thread.join(timeout=2) - server._DEMO_JANITOR_STARTED = False - server._DEMO_JANITOR_THREAD = None + demo_mode._DEMO_JANITOR_STARTED = False + demo_mode._DEMO_JANITOR_THREAD = None conn = getattr(getattr(server, "meta_db", None), "conn", None) if conn is not None: getattr(__import__("sys").modules.get("server"), "_join_background_db_threads", lambda: None)()