diff --git a/lib/routers/loops.py b/lib/routers/loops.py index bb17628..7ba809b 100644 --- a/lib/routers/loops.py +++ b/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} diff --git a/tests/test_loops_concurrency.py b/tests/test_loops_concurrency.py new file mode 100644 index 0000000..61b86d7 --- /dev/null +++ b/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