mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-07-22 21:01:40 +00:00
4fd0cd49e7
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>
85 lines
2.6 KiB
Python
85 lines
2.6 KiB
Python
"""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
|