Skip to content

fix(loops): lock the count+insert and the list read - #840

Merged
byrongamatos merged 1 commit into
mainfrom
fix/loops-lock-atomicity
Jul 10, 2026
Merged

fix(loops): lock the count+insert and the list read#840
byrongamatos merged 1 commit into
mainfrom
fix/loops-lock-atomicity

Conversation

@byrongamatos

@byrongamatos byrongamatos commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #839. Fixes two pre-existing races CodeRabbit flagged on the loops routes there — the verbatim extraction moved them unchanged from server.py, so they correctly weren't fixed in the move-only PR (both are byte-identical to 6c98aba:server.py).

  1. save_loop auto-naming was not atomic. COUNT(*) ran outside meta_db._lock, then the INSERT ran inside it. Two simultaneous unnamed POSTs read the same count and both mint Loop N. → COUNT + INSERT now share one lock scope.
  2. list_loops read the shared connection unlocked. The single check_same_thread=False connection is serialized through _lock by every writer, so an unlocked SELECT can overlap a POST/DELETE commit. → read under the lock.

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, so it's worth doing.

Test

tests/test_loops_concurrency.py releases 16 workers into save_loop at once via a threading.Barrier and asserts no two auto-named loops collide. Negative-checked: reverting the COUNT back outside the lock fails the uniqueness assertion 5/5 runs; the fix passes 3/3. A second test hammers list_loops against 40 concurrent writes and asserts no error.

pytest 2400 passed; on-device, two unnamed POSTs → ['Loop 1', 'Loop 2']. Codex 0 findings.

Closes the two review threads on #839.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability when saving loops concurrently.
    • Prevented duplicate automatically generated loop names.
    • Improved stability when viewing loops while saves are in progress.
  • Tests

    • Added coverage for concurrent loop creation and simultaneous reading and saving.

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>
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fd101d04-2c50-466c-b4b8-647aedae3e14

📥 Commits

Reviewing files that changed from the base of the PR and between b41361e and ea6a797.

📒 Files selected for processing (2)
  • lib/routers/loops.py
  • tests/test_loops_concurrency.py

📝 Walkthrough

Walkthrough

Loop database reads and writes now use the metadata database lock. Auto-generated names are counted and inserted within one locked transaction, with concurrency tests covering simultaneous saves and interleaved reads and writes.

Changes

Loop concurrency handling

Layer / File(s) Summary
Serialize loop database operations
lib/routers/loops.py
list_loops locks SQLite reads, while save_loop keeps auto-name generation, insertion, and commit within the same lock scope.
Validate concurrent loop access
tests/test_loops_concurrency.py
Tests verify unique generated names, expected insert counts, and error-free concurrent reads and writes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main concurrency fix in loops: locking the count+insert path and list reads.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/loops-lock-atomicity

Comment @coderabbitai help to get the list of available commands.

@byrongamatos
byrongamatos merged commit 4fd0cd4 into main Jul 10, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant