fix(loops): take the DB lock for the count+insert and the list read

Two pre-existing races in the loops routes, flagged by CodeRabbit on #839 (the
verbatim extraction moved them unchanged from server.py, so they correctly
weren't fixed there):

1. save_loop computed `COUNT(*)` OUTSIDE meta_db._lock, then inserted inside it.
   Two simultaneous unnamed POSTs read the same count and both mint "Loop N".
   Fix: one lock scope around COUNT + INSERT.
2. list_loops read the shared single connection (check_same_thread=False) with
   no lock, so it could overlap a POST/DELETE commit. Fix: read under the lock,
   like every writer.

Low severity in context — FeedBack is single-user (Principle I), so concurrent
unnamed-loop POSTs essentially can't happen — but each fix is one lock scope.

tests/test_loops_concurrency.py pins both with a threading.Barrier that releases
16 workers into save_loop at once. Negative-checked: reverting the COUNT back
outside the lock fails the uniqueness assertion 5/5 runs; the fix passes 3/3.
pytest 2400 passed; on-device two unnamed POSTs -> ['Loop 1','Loop 2'].

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   next root-level module can't ship broken.
 
 ### Added
+- **`routers/` — extracting `server.py`'s route layer, cheapest-first (R3).** Each PR moves a cohesive route group into a `fastapi.APIRouter` under `lib/routers/`, mounted with `app.include_router(...)` at its original site (FastAPI matches in registration order; the full route table stays byte-identical). Bodies are verbatim — only the decorator receiver (`@app` → `@router`) and singleton reads (`meta_db` → `appstate.meta_db`, resolved at call time) change. So far: `audio_effects` (5), `artist_aliases` (5), `loops` (3 — saved A/B practice regions). The next cut is picked by a dependency-closure scan that ranks groups by how many `monkeypatch.setattr(server, …)` targets they'd drag along.
 - **`routers/` — the first extracted route module (R3).** The five audio-effects mapping
   endpoints move out of `server.py` into `lib/routers/audio_effects.py` as a
   `fastapi.APIRouter`, mounted with `app.include_router(...)` **at the point in the file

docs/size-exemptions.md

@@ -55,8 +55,8 @@ without a *signed* exemption" is unenforceable.
 ## Planned, NOT exempt (owned by split plans — listed so nothing falls between states)
 
 core `static/app.js` (11,852) · `static/highway.js` (4,168, whole file) · `server.py`
-(9,386 — was 14,037; ratcheted by the R3 `MetadataDB` + `AudioEffectsMappingDB`
-extractions and the first `routers/` module) ·
+(9,302 — was 14,037; ratcheted by the R3 `MetadataDB` + `AudioEffectsMappingDB`
+extractions and the first three `routers/` modules) ·
 `lib/metadata_db.py` (4,373 — new in R3; the `MetadataDB` class alone is 4,018 lines
 and is a monolith in its own right, to be split per-table once the router train
 lands) · `static/v3/songs.js` (4,134) · `static/capabilities/audio-session.js`

lib/routers/artist_aliases.py

@@ -0,0 +1,68 @@
+"""Artist aliases / Tidy-up (P4) — canonicalize messy artist tags at DISPLAY
+("ACDC" -> "AC/DC") without touching feedpak files or the scanner-derived
+songs.artist. All DB-only.
+
+Extracted verbatim from ``server.py`` (R3); only the decorator receiver
+(``@app`` -> ``@router``) and the singleton read (``meta_db`` ->
+``appstate.meta_db``) changed. The read stays a module attribute so a re-imported
+``server`` re-publishes a fresh DB into the seam — see ``appstate.py``.
+"""
+
+from fastapi import APIRouter
+from fastapi.responses import JSONResponse
+
+import appstate
+
+router = APIRouter()
+
+
+@router.get("/api/artist-aliases")
+def list_artist_aliases():
+    """Existing raw→canonical overrides (the Tidy-up 'current merges' list)."""
+    return {"aliases": appstate.meta_db.list_artist_aliases()}
+
+
+@router.get("/api/artists/raw")
+def list_raw_artists(limit: int = 2000):
+    """Distinct RAW artist names + song counts + current canonical — the Tidy-up
+    picker (you merge raw variants into one canonical)."""
+    return {"artists": appstate.meta_db.raw_artists(limit)}
+
+
+@router.post("/api/artist-aliases")
+def set_artist_alias(data: dict):
+    """Upsert one override: {raw_name, canonical_name, mb_artist_id?}. A self-alias
+    (raw == canonical) clears the row instead (un-merge)."""
+    raw = (data.get("raw_name") or "").strip()
+    canon = (data.get("canonical_name") or "").strip()
+    if not raw or not canon:
+        return JSONResponse({"error": "raw_name and canonical_name are required"}, 400)
+    result = appstate.meta_db.set_artist_alias(raw, canon, (data.get("mb_artist_id") or None))
+    if not result.get("ok"):
+        # Would form a cycle (raw → … → raw) — refuse rather than corrupt the chain.
+        return JSONResponse(
+            {"error": "alias would create a cycle", "raw_name": raw, "canonical_name": canon},
+            409)
+    return {"ok": True, "raw_name": raw, "canonical_name": result.get("canonical_name", canon)}
+
+
+@router.post("/api/artist-aliases/merge")
+def merge_artist_aliases(data: dict):
+    """Merge several raw artist variants into one canonical:
+    {raw_names: [...], canonical_name}. The canonical's own self-alias is skipped.
+    Returns {merged: N}."""
+    canon = (data.get("canonical_name") or "").strip()
+    raws = data.get("raw_names")
+    if not canon:
+        return JSONResponse({"error": "canonical_name is required"}, 400)
+    if not isinstance(raws, list) or not raws:
+        return JSONResponse({"error": "raw_names must be a non-empty array"}, 400)
+    n = appstate.meta_db.merge_artists(raws, canon)
+    return {"merged": n, "canonical_name": canon}
+
+
+@router.delete("/api/artist-aliases/{raw_name:path}")
+def delete_artist_alias(raw_name: str):
+    """Remove one override so that raw artist stands on its own again."""
+    appstate.meta_db.remove_artist_alias(raw_name)
+    return {"ok": True}

lib/routers/loops.py

@@ -0,0 +1,60 @@
+"""Practice loops — saved A/B regions per song.
+
+Extracted verbatim from ``server.py`` (R3); only the decorator receiver
+(``@app`` -> ``@router``) and the singleton reads (``meta_db`` ->
+``appstate.meta_db``) changed. See ``appstate.py`` for why the reads stay
+module attributes.
+"""
+
+from fastapi import APIRouter
+
+import appstate
+
+router = APIRouter()
+
+
+@router.get("/api/loops")
+def list_loops(filename: str):
+    # Hold the DB lock for the read: the shared single connection
+    # (check_same_thread=False) is serialized through meta_db._lock by every
+    # writer, so an unlocked SELECT here can overlap a POST/DELETE commit.
+    db = appstate.meta_db
+    with db._lock:
+        rows = db.conn.execute(
+            "SELECT id, name, start_time, end_time FROM loops WHERE filename = ? ORDER BY start_time",
+            (filename,)
+        ).fetchall()
+    return [{"id": r[0], "name": r[1], "start": r[2], "end": r[3]} for r in rows]
+
+
+@router.post("/api/loops")
+def save_loop(data: dict):
+    filename = data.get("filename", "")
+    name = data.get("name", "").strip()
+    start = data.get("start")
+    end = data.get("end")
+    if not filename or start is None or end is None:
+        return {"error": "Missing fields"}
+    db = appstate.meta_db
+    with db._lock:
+        # COUNT + INSERT under one lock so two unnamed POSTs can't read the same
+        # count and both mint "Loop N" (the count is only used to name the row).
+        if not name:
+            count = db.conn.execute(
+                "SELECT COUNT(*) FROM loops WHERE filename = ?", (filename,)
+            ).fetchone()[0]
+            name = f"Loop {count + 1}"
+        db.conn.execute(
+            "INSERT INTO loops (filename, name, start_time, end_time) VALUES (?, ?, ?, ?)",
+            (filename, name, float(start), float(end))
+        )
+        db.conn.commit()
+    return {"ok": True, "name": name}
+
+
+@router.delete("/api/loops/{loop_id}")
+def delete_loop(loop_id: int):
+    with appstate.meta_db._lock:
+        appstate.meta_db.conn.execute("DELETE FROM loops WHERE id = ?", (loop_id,))
+        appstate.meta_db.conn.commit()
+    return {"ok": True}

server.py

@@ -71,7 +71,7 @@ from audio_effects_db import AudioEffectsMappingDB
 # Lives in lib/ because that is the one core dir every packaging path copies.
 import appstate
 # Extracted route modules. They import `appstate`, never `server` — one-way graph.
-from routers import audio_effects
+from routers import audio_effects, artist_aliases, loops
 import sloppak as sloppak_mod
 import drums as drums_mod
 import notation as notation_mod
@@ -4847,59 +4847,9 @@ def list_tags():
 
 
 # ── Artist aliases / Tidy-up (P4) ────────────────────────────────────────────
-# Canonicalize messy artist tags at DISPLAY ("ACDC" → "AC/DC") without touching
-# the feedpak files or the scanner-derived songs.artist. All DB-only.
-
-@app.get("/api/artist-aliases")
-def list_artist_aliases():
-    """Existing raw→canonical overrides (the Tidy-up 'current merges' list)."""
-    return {"aliases": meta_db.list_artist_aliases()}
-
-
-@app.get("/api/artists/raw")
-def list_raw_artists(limit: int = 2000):
-    """Distinct RAW artist names + song counts + current canonical — the Tidy-up
-    picker (you merge raw variants into one canonical)."""
-    return {"artists": meta_db.raw_artists(limit)}
-
-
-@app.post("/api/artist-aliases")
-def set_artist_alias(data: dict):
-    """Upsert one override: {raw_name, canonical_name, mb_artist_id?}. A self-alias
-    (raw == canonical) clears the row instead (un-merge)."""
-    raw = (data.get("raw_name") or "").strip()
-    canon = (data.get("canonical_name") or "").strip()
-    if not raw or not canon:
-        return JSONResponse({"error": "raw_name and canonical_name are required"}, 400)
-    result = meta_db.set_artist_alias(raw, canon, (data.get("mb_artist_id") or None))
-    if not result.get("ok"):
-        # Would form a cycle (raw → … → raw) — refuse rather than corrupt the chain.
-        return JSONResponse(
-            {"error": "alias would create a cycle", "raw_name": raw, "canonical_name": canon},
-            409)
-    return {"ok": True, "raw_name": raw, "canonical_name": result.get("canonical_name", canon)}
-
-
-@app.post("/api/artist-aliases/merge")
-def merge_artist_aliases(data: dict):
-    """Merge several raw artist variants into one canonical:
-    {raw_names: [...], canonical_name}. The canonical's own self-alias is skipped.
-    Returns {merged: N}."""
-    canon = (data.get("canonical_name") or "").strip()
-    raws = data.get("raw_names")
-    if not canon:
-        return JSONResponse({"error": "canonical_name is required"}, 400)
-    if not isinstance(raws, list) or not raws:
-        return JSONResponse({"error": "raw_names must be a non-empty array"}, 400)
-    n = meta_db.merge_artists(raws, canon)
-    return {"merged": n, "canonical_name": canon}
-
-
-@app.delete("/api/artist-aliases/{raw_name:path}")
-def delete_artist_alias(raw_name: str):
-    """Remove one override so that raw artist stands on its own again."""
-    meta_db.remove_artist_alias(raw_name)
-    return {"ok": True}
+# Mounted here, where these routes used to be defined (FastAPI matches in
+# registration order). Implementation in lib/routers/artist_aliases.py.
+app.include_router(artist_aliases.router)
 
 
 # ── Artist pages (launch charrette PR-B) ──────────────────────────────────────
@@ -5953,44 +5903,9 @@ def api_remove_wanted(wanted_id: int):
 
 
 # ── Loops API ────────────────────────────────────────────────────────────────
-
-@app.get("/api/loops")
-def list_loops(filename: str):
-    rows = meta_db.conn.execute(
-        "SELECT id, name, start_time, end_time FROM loops WHERE filename = ? ORDER BY start_time",
-        (filename,)
-    ).fetchall()
-    return [{"id": r[0], "name": r[1], "start": r[2], "end": r[3]} for r in rows]
-
-
-@app.post("/api/loops")
-def save_loop(data: dict):
-    filename = data.get("filename", "")
-    name = data.get("name", "").strip()
-    start = data.get("start")
-    end = data.get("end")
-    if not filename or start is None or end is None:
-        return {"error": "Missing fields"}
-    if not name:
-        count = meta_db.conn.execute(
-            "SELECT COUNT(*) FROM loops WHERE filename = ?", (filename,)
-        ).fetchone()[0]
-        name = f"Loop {count + 1}"
-    with meta_db._lock:
-        meta_db.conn.execute(
-            "INSERT INTO loops (filename, name, start_time, end_time) VALUES (?, ?, ?, ?)",
-            (filename, name, float(start), float(end))
-        )
-        meta_db.conn.commit()
-    return {"ok": True, "name": name}
-
-
-@app.delete("/api/loops/{loop_id}")
-def delete_loop(loop_id: int):
-    with meta_db._lock:
-        meta_db.conn.execute("DELETE FROM loops WHERE id = ?", (loop_id,))
-        meta_db.conn.commit()
-    return {"ok": True}
+# Mounted here, where these routes used to be defined (FastAPI matches in
+# registration order). Implementation in lib/routers/loops.py.
+app.include_router(loops.router)
 
 
 # ── Audio Effects Mapping API ───────────────────────────────────────────────

tests/test_loops_concurrency.py

@@ -0,0 +1,84 @@
+"""Concurrent unnamed loop saves must get unique names.
+
+`save_loop` auto-names an unnamed loop `Loop {count+1}`. If the `COUNT(*)` runs
+outside the DB lock (as it did before the fix), two simultaneous unnamed POSTs
+read the same count and both mint the same name. A `threading.Barrier` releases
+all workers into `save_loop` at once to force that interleave.
+
+Single-user app, so this race is unlikely in practice — but the fix is one lock
+scope, and the test pins it.
+"""
+
+import threading
+
+import pytest
+
+import appstate
+from metadata_db import MetadataDB
+from routers import loops
+
+
+@pytest.fixture()
+def meta_db(tmp_path):
+    prev = appstate.meta_db
+    db = MetadataDB(tmp_path)
+    appstate.configure(meta_db=db)
+    yield db
+    db.conn.close()
+    appstate.configure(meta_db=prev)
+
+
+def test_concurrent_unnamed_saves_get_unique_names(meta_db):
+    workers = 16
+    barrier = threading.Barrier(workers)
+    names, errors = [], []
+    lock = threading.Lock()
+
+    def save():
+        try:
+            barrier.wait()                       # release all at once
+            r = loops.save_loop({"filename": "song.feedpak", "start": 0.0, "end": 1.0})
+            with lock:
+                names.append(r["name"])
+        except Exception as e:                   # noqa: BLE001 — surface any thread error
+            with lock:
+                errors.append(e)
+
+    threads = [threading.Thread(target=save) for _ in range(workers)]
+    for t in threads:
+        t.start()
+    for t in threads:
+        t.join()
+
+    assert not errors, errors
+    assert len(names) == workers
+    # The load-bearing assertion: no two auto-named loops collide.
+    assert len(set(names)) == workers, f"duplicate loop names: {sorted(names)}"
+    # And the DB agrees — every insert landed.
+    stored = meta_db.conn.execute(
+        "SELECT COUNT(*) FROM loops WHERE filename = ?", ("song.feedpak",)
+    ).fetchone()[0]
+    assert stored == workers
+
+
+def test_list_and_save_do_not_error_under_interleave(meta_db):
+    """A read overlapping writes must not raise (shared connection, one lock)."""
+    stop = threading.Event()
+    errors = []
+
+    def reader():
+        while not stop.is_set():
+            try:
+                loops.list_loops("song.feedpak")
+            except Exception as e:               # noqa: BLE001
+                errors.append(e)
+
+    r = threading.Thread(target=reader)
+    r.start()
+    try:
+        for i in range(40):
+            loops.save_loop({"filename": "song.feedpak", "name": f"n{i}", "start": 0.0, "end": 1.0})
+    finally:
+        stop.set()
+        r.join()
+    assert not errors, errors