Skip to content

refactor(server): carve demo mode into lib/demo_mode.py (R3b) - #903

Merged
byrongamatos merged 1 commit into
mainfrom
r3b/demo-mode
Jul 11, 2026
Merged

refactor(server): carve demo mode into lib/demo_mode.py (R3b)#903
byrongamatos merged 1 commit into
mainfrom
r3b/demo-mode

Conversation

@byrongamatos

@byrongamatos byrongamatos commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Stacked on #901. lib/demo_mode.py (342). server.py 1,870 → 1,649.

The read-only request guard (its 96-entry blocked-route table + the middleware) and the hourly session janitor (registry, hook runner, thread). Bodies verbatim.

The middleware needs app, so the module takes it

_demo_mode_guard is an @app.middleware("http") and cannot exist without an app object. Rather than have a module under lib/ reach for a global, it exposes install(app) and server.py — which owns the app — hands it over.

The janitor is symmetrical: start_janitor() / stop_janitor(), called from server.py's startup and shutdown hooks, where the process lifecycle actually lives.

register_demo_janitor_hook is part of the plugin contract

It's a key in plugin_context, so plugins hold it as a live reference from setup(). server.py imports this exact object and puts it in the dict unchanged — identity preserved, and tests/test_plugin_context_contract.py (#898) fails if that ever stops being true.

This is the first carve that guard has actually protected.

stop_janitor()'s order is load-bearing

The obvious way to write it — clear the "started" flag, then join — is wrong, and I wrote it that way first. server.py's original deliberately returns early, leaving _DEMO_JANITOR_STARTED True and the thread handle intact, when the thread outlives the join:

# Leave _DEMO_JANITOR_STARTED True so a new janitor is not
# spawned by a subsequent startup while the old one is alive.

Clearing the flag first quietly reintroduces exactly the double-janitor leak the flag exists to prevent. Preserved byte-for-byte, and the reason is now written down at the function rather than only at its single call site.

A bug moved verbatim, on purpose

if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \
        and not _DEMO_JANITOR_STARTED:

and binds tighter than or, so this is A or (B and C)the not-already-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).

Verified, preserved exactly, and filed as #902. A carve whose whole value is being provably behaviour-neutral is not the place to change behaviour.

pyflakes

Three more missing imports caught on the way in (uuid, warnings). Five carves, ten missing imports, every one a NameError on a live path.

pytest 2399 · pyflakes 0 · Codex 0.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added demo mode protections that block selected API operations and provide a demo session cookie.
    • Added hourly background cleanup for demo-mode data.
    • Improved scan and rescan handling, including scan status reporting and safer concurrency controls.
  • Bug Fixes
    • Preserved saved song records during full rescans while refreshing their metadata.
    • Improved background thread shutdown and startup reliability.

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 208831b8-8944-4872-ac40-2bbd0784edb9

📥 Commits

Reviewing files that changed from the base of the PR and between 5da720b and 22c24eb.

📒 Files selected for processing (4)
  • lib/demo_mode.py
  • server.py
  • tests/test_demo_mode.py
  • tests/test_startup_status.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/test_startup_status.py
  • tests/test_demo_mode.py

📝 Walkthrough

Walkthrough

Demo mode is extracted into lib/demo_mode.py, where middleware and janitor lifecycle are implemented. server.py delegates demo-mode ownership and scan operations to dedicated modules, while tests update their state cleanup and plugin-context assertions.

Changes

Demo mode and scan lifecycle integration

Layer / File(s) Summary
Demo-mode middleware and janitor core
lib/demo_mode.py
Adds demo-mode endpoint blocking, session-cookie handling, janitor hook validation, and hourly janitor thread lifecycle management.
Server demo lifecycle and test ownership
server.py, tests/test_demo_mode.py, tests/test_startup_status.py
Installs demo mode, delegates janitor registration and startup/shutdown, and updates tests to use demo_mode state.
Scan orchestration integration
server.py
Routes startup, periodic, status, and manual scan operations through scan, while joining scan and enrichment workers during background shutdown handling.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant FastAPIApp
  participant demo_mode_guard
  participant Endpoint
  Client->>FastAPIApp: Send HTTP request
  FastAPIApp->>demo_mode_guard: Run HTTP middleware
  demo_mode_guard->>Endpoint: Forward allowed request
  Endpoint-->>Client: Return response
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 74.47% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: extracting demo-mode logic from server.py into lib/demo_mode.py.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch r3b/demo-mode

Comment @coderabbitai help to get the list of available commands.

lib/demo_mode.py (342). server.py 1,870 -> 1,649.

The read-only request guard (its 96-entry blocked-route table + the middleware) and the
hourly session janitor (registry, hook runner, thread). Bodies VERBATIM.

THE MIDDLEWARE NEEDS `app`, SO THE MODULE TAKES IT. _demo_mode_guard is an
@app.middleware("http") and cannot exist without an app object. Rather than have a module
under lib/ reach for a global, it exposes install(app) and server.py — which owns the app —
hands it over. The janitor is symmetrical: start_janitor() / stop_janitor(), called from
server.py's startup and shutdown hooks, 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(). server.py imports this exact object
and puts it in the dict unchanged — identity preserved, and
tests/test_plugin_context_contract.py (#898, merged) fails if that ever stops being true.
This is the first carve that guard has actually protected.

━━━ stop_janitor()'s ORDER IS LOAD-BEARING ━━━

The obvious way to write it — clear the "started" flag, then join — is WRONG, and I wrote
it that way first. server.py's original deliberately returns EARLY, leaving
_DEMO_JANITOR_STARTED True and the thread handle intact, when the thread outlives the join:

    # Leave _DEMO_JANITOR_STARTED True so a new janitor is not
    # spawned by a subsequent startup while the old one is alive.

Clearing the flag first quietly reintroduces exactly the double-janitor leak the flag
exists to prevent. Preserved byte-for-byte, and the reason is now written down at the
function rather than only at its single call site.

━━━ A BUG MOVED VERBATIM, ON PURPOSE ━━━

    if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \
            and not _DEMO_JANITOR_STARTED:

`and` binds tighter than `or`, so this is `A or (B and C)` — the not-already-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). Verified. Preserved exactly and filed as issue #902: a carve whose whole value
is being provably behaviour-neutral is not the place to change behaviour.

pyflakes caught three more missing imports on the way in (uuid, warnings x2). Five carves,
ten missing imports, every one a NameError on a live path.

pytest 2399, pyflakes 0, Codex 0.

Refs #48

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@byrongamatos
byrongamatos merged commit f5d448a into main Jul 11, 2026
5 checks passed
@byrongamatos
byrongamatos deleted the r3b/demo-mode branch July 11, 2026 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant