From 4fd0cd49e7ccebe31adae6a364605a8fbb4e893b Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Fri, 10 Jul 2026 19:16:01 +0200 Subject: [PATCH] fix(loops): take the DB lock for the count+insert and the list read (#840) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- lib/routers/loops.py | 32 ++++++++----- tests/test_loops_concurrency.py | 84 +++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 12 deletions(-) create mode 100644 tests/test_loops_concurrency.py 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