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

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>

lib/routers/loops.py

@@ -15,10 +15,15 @@ router = APIRouter()
 
 @router.get("/api/loops")
 def list_loops(filename: str):
-    rows = appstate.meta_db.conn.execute(
-        "SELECT id, name, start_time, end_time FROM loops WHERE filename = ? ORDER BY start_time",
-        (filename,)
-    ).fetchall()
+    # 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]
 
 
@@ -30,17 +35,20 @@ def save_loop(data: dict):
     end = data.get("end")
     if not filename or start is None or end is None:
         return {"error": "Missing fields"}
-    if not name:
-        count = appstate.meta_db.conn.execute(
-            "SELECT COUNT(*) FROM loops WHERE filename = ?", (filename,)
-        ).fetchone()[0]
-        name = f"Loop {count + 1}"
-    with appstate.meta_db._lock:
-        appstate.meta_db.conn.execute(
+    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))
         )
-        appstate.meta_db.conn.commit()
+        db.conn.commit()
     return {"ok": True, "name": name}
 
 

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